Skip to content

feat(forge): add forge.Client interface for forge-agnostic API access#602

Merged
nextlevelshit merged 2 commits intomainfrom
feat/forge-client-abstraction
Mar 26, 2026
Merged

feat(forge): add forge.Client interface for forge-agnostic API access#602
nextlevelshit merged 2 commits intomainfrom
feat/forge-client-abstraction

Conversation

@nextlevelshit
Copy link
Copy Markdown
Collaborator

Summary

  • Introduces forge.Client interface with forge-neutral domain types (Issue, PullRequest) and options types
  • Implements GitHub adapter wrapping existing *github.Client with type conversion layer
  • Adds UnsupportedClient stub for GitLab/Bitbucket/Gitea (returns ErrNotSupported)
  • Centralizes token resolution (ResolveToken) for all 4 forge types, eliminating 3 duplicate resolveGitHubToken() functions
  • Replaces *github.Client in webui server, doctor, and TUI with forge.Client
  • Uses forge.DetectFromGitRemotes() + ForgeInfo.Slug() to replace detectRepoSlug()

Test plan

  • go test -race ./... — all 36 packages pass
  • New tests: forge/client_test.go (interface satisfaction, UnsupportedClient)
  • New tests: forge/github_test.go (conversion functions, nil handling, IsPR flag)
  • New tests: forge/token_test.go (env var resolution for all 4 forge types, factory)
  • Existing webui/doctor/tui tests pass unchanged (nil client path)

Closes #600

Introduce a forge.Client interface with forge-neutral domain types
(Issue, PullRequest) and a GitHub adapter wrapping the existing
github.Client. Includes token resolution for GitHub, GitLab, Bitbucket,
and Gitea, plus an UnsupportedClient stub for non-GitHub forges.

Replace all *github.Client usage in webui, doctor, and TUI packages
with forge.Client, consolidating duplicated resolveGitHubToken() and
detectRepoSlug() functions into the forge package.

Closes #600
@nextlevelshit
Copy link
Copy Markdown
Collaborator Author

Code Review (Wave Pipeline)

Overall Verdict: REQUEST_CHANGES

This PR introduces a well-designed forge.Client abstraction that decouples issue/PR operations from GitHub-specific types, enabling future GitLab/Bitbucket/Gitea support. The implementation is clean, naming is consistent, and it delivers a net reduction in code duplication by consolidating three separate token resolution implementations. Security review found no critical vulnerabilities.

However, one issue must be fixed before merge.


Critical Issues (must fix)

1. UnsupportedClient breaks nil-guard pattern for non-GitHub users

internal/forge/token.go:73-78 — When a non-GitHub forge user has a valid token, NewClient returns an UnsupportedClient (non-nil) that errors on every call. All consumers use if forgeClient == nil guards, so these users now see cryptic "not supported" runtime errors instead of the previous graceful "not configured" message. This is a behavioral regression.

Fix: Return nil for unsupported forge types so existing nil-guards work, or add an IsSupported() bool method to the Client interface and check it at call sites.


Suggested Improvements

Design concerns (recommend addressing before merge or as immediate follow-up)

  • forgeIssueToGitHub round-trip conversion (internal/doctor/codebase.go:131-145) — Converting GitHub → forge → GitHub is lossy (e.g., PullRequest sub-struct is dropped). Currently safe because callers filter PRs before hitting the analyzer, but fragile. Consider making github.Analyzer accept forge.Issue directly.

  • Silent error swallowing in AnalyzeCodebase (internal/doctor/codebase.go:78-89) — ListPullRequests and ListIssues errors are silently discarded. Auth errors or rate limits result in silently incomplete reports. Pre-existing but touched by this PR — log at minimum.

Minor items (follow-up is fine)

  • NewGitHubClient(nil) will panic on use (internal/forge/github.go:288) — Add nil validation in the constructor or document the constraint.
  • TestNewClient_NoToken is non-deterministic (internal/forge/token_test.go:94-103) — Doesn't assert anything meaningful; behavior depends on whether gh is authenticated on the machine.
  • ResolveToken subprocess has no timeout (internal/forge/token.go:37-42) — Use exec.CommandContext with a short timeout to prevent indefinite blocking if gh hangs.
  • ErrNotConfigured is defined but unused (internal/forge/client.go:10) — Remove or use it.
  • TUI double-detects forge (internal/tui/app.go:158-164) — Could use DetectFromGitRemotes() directly like webui does, avoiding redundant remote parsing.
  • No tests for forgeIssueToGitHub or integration-level tests exercising forge option mapping against a mock HTTP server.
  • Minor formatting: inconsistent struct field alignment in internal/webui/server.go:101-108 (run gofmt).

Positive Observations

  • Consolidates three token resolution implementations into a single forge.ResolveToken() — net security and maintainability win.
  • Thorough nil-safety in type conversionsconvertGitHubIssue and convertGitHubPR properly nil-check all pointer fields before dereferencing, with test coverage for nil cases.
  • Context timeouts preserved — All webui forge API calls correctly use context.WithTimeout, preventing unbounded calls.
  • Clean interface design — The forge.Client interface is minimal and well-scoped. Naming is consistent across all touched files.
  • No security vulnerabilities — No SQL injection, XSS, hardcoded secrets, or credential exposure found. Tokens are not logged.
  • Good test coverage for new code — Interface conformance tests, conversion function tests, and forge detection tests are solid.

Generated by Wave pr-review pipeline

…lient

Fixes review finding: UnsupportedClient broke nil-guard pattern causing
cryptic errors for non-GitHub forge users. Also removes unused
ErrNotConfigured, adds nil validation to NewGitHubClient, and fixes
gofmt alignment.
@nextlevelshit nextlevelshit merged commit 8c7cbe5 into main Mar 26, 2026
2 checks passed
nextlevelshit added a commit that referenced this pull request Mar 26, 2026
- Remove duplicate replaceBoth block in context.go (lines 110-113)
- Hoist 4 regexes to package-level vars in context.go (compiled once)
- Log forge API errors in doctor instead of silently swallowing
- Add assertions to SSH-with-port test (was a no-op)
- Add 5s timeout to `gh auth token` subprocess via CommandContext
- Simplify TUI forge detection (remove redundant Detect call)
nextlevelshit added a commit that referenced this pull request Mar 26, 2026
fix: address PR review tech debt from #602 and #603
@nextlevelshit nextlevelshit deleted the feat/forge-client-abstraction branch March 30, 2026 16:22
nextlevelshit added a commit that referenced this pull request Apr 12, 2026
feat(forge): add forge.Client interface for forge-agnostic API access
nextlevelshit added a commit that referenced this pull request Apr 12, 2026
- Remove duplicate replaceBoth block in context.go (lines 110-113)
- Hoist 4 regexes to package-level vars in context.go (compiled once)
- Log forge API errors in doctor instead of silently swallowing
- Add assertions to SSH-with-port test (was a no-op)
- Add 5s timeout to `gh auth token` subprocess via CommandContext
- Simplify TUI forge detection (remove redundant Detect call)
nextlevelshit added a commit that referenced this pull request Apr 12, 2026
fix: address PR review tech debt from #602 and #603
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WebUI: replace hardcoded GitHub client with forge-agnostic abstraction

1 participant