feat: implement GitHub API methods and VCS routing (issue #130) #131
Reference in New Issue
Block a user
Delete Branch "issue-130"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements GitHub API methods for PR review and wires up VCS type routing, closing issue #130.
review-bot can now review PRs on both Gitea and GitHub (including GitHub Enterprise Server).
Changes
Phase 1:
github/client.go— API methodsImplements all higher-level methods that were TODO since issue #120:
GetPullRequestGET /repos/{owner}/{repo}/pulls/{number}GetPullRequestDiffAccept: application/vnd.github.diffGetPullRequestFilesGET /repos/{owner}/{repo}/pulls/{number}/files(paginated)GetCommitStatusesGET /repos/{owner}/{repo}/commits/{sha}/statusesGetFileContentGET /repos/{owner}/{repo}/contents/{path}(base64 decoded)GetFileContentRef?ref=ListContentsGetAllFilesInPathPostReviewPOST /repos/{owner}/{repo}/pulls/{number}/reviewsListReviewsGET /repos/{owner}/{repo}/pulls/{number}/reviews(paginated)DeleteReviewDELETE .../reviews/{id}(GitHub: PENDING only)GetAuthenticatedUserGET /userRequestReviewerPOST .../requested_reviewersEdge cases handled:
\nin base64 content — stripped before decodeListContentson a file path returns a single object (not array) — normalizedDeleteReviewdocuments GitHub's PENDING-only constraintPhase 2:
cmd/review-bot/vcs.go+main.go— VCS routingNew
vcs.go:vcsClientinterface — consumer-defined (Go idiom), covers all PR opsgiteaExtClientinterface — Gitea-specific extension (supersede, comment resolution)giteaVCSAdapter— wrapsgitea.Client, satisfies both interfacesgithubVCSAdapter— wrapsgithub.Client, satisfiesvcsClientUpdated
main.go:VCS_TYPEenv var > URL heuristic (containsgithub.com)github.com→api.github.com, GHES →/api/v3vcsClientinterfacegiteaExtClienttype assertiongiteaClientAdapter(replaced bygiteaVCSAdapter)Test Coverage
22 new test cases in
github/client_test.go:escapePathwith special charactersExisting tests updated for type changes (gitea types → vcs types).
GitHub Limitations
All limitations handled gracefully — logged as info, review-bot continues.
Checklist
VCS_TYPEenv var/api/v3URL pattern)go test ./...passesgo vet ./...passes cleangithub/client.goCloses #130
Wire up the new GitHub API methods to the review-bot CLI via VCS type detection. review-bot can now review PRs on both Gitea and GitHub (including GitHub Enterprise Server). Changes: - vcs.go: define vcsClient interface with all PR operations - giteaVCSAdapter: wraps gitea.Client, satisfies vcsClient + giteaExtClient - githubVCSAdapter: wraps github.Client, satisfies vcsClient - giteaExtClient: Gitea-specific extension (supersede, comment resolution) - main.go: detect VCS type via VCS_TYPE env var (auto-detects github.com URLs) - Creates appropriate client (gitea or github) based on vcs_type - GitHub API URL derived from server URL (github.com → api.github.com, GHES → /api/v3) - All main flow uses vcsClient interface - Gitea-specific supersede operations gated via giteaExtClient type assertion - GitHub: logs info when skipping supersede (not supported) - Removes old giteaClientAdapter (replaced by giteaVCSAdapter in vcs.go) - giteaVCSAdapter satisfies review.GiteaClient for persona loading GitHub limitations handled gracefully: - Review supersede skipped (GitHub doesn't allow editing submitted reviews) - DeleteReview returns error for non-pending reviews (documented in adapter) - Inline comments use absolute line + side='RIGHT' instead of diff position Closes #130. Co-authored-by: Rodin <rodin@forgedthought.ai>testto feat: implement GitHub API methods and VCS routing (issue #130)Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
10ef451c)Sonnet Review
Summary
This PR correctly implements the GitHub API client methods and VCS routing abstraction. The code is well-structured, follows Go idioms (consumer-defined interfaces, adapter pattern), and is comprehensively tested. CI passes.
Findings
cmd/review-bot/main.gocmd/review-bot/vcs.gocmd/review-bot/vcs.gocmd/review-bot/vcs.gogithub/client_test.goencodedis declared and then immediately suppressed with_ = encoded. The variable is unused because the test hardcodes the JSON string inline. Either remove theencodedvariable declaration entirely or use it to construct the response body.github/client.gofilepathas a parameter name, which shadows the standard library packagepath/filepath. Whilefilepathis not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; considerfilePathorpathfor consistency.Recommendation
APPROVE — Approve. The implementation is correct, well-designed, and follows established Go patterns (consumer-defined interfaces per interfaces.md, adapter pattern, small focused interfaces). The VCS routing abstraction cleanly separates Gitea-specific functionality via the giteaExtClient type assertion rather than polluting the shared interface. Error handling follows project conventions (fmt.Errorf with %w). Test coverage is comprehensive. The findings are all minor/nit-level and don't block merging — the hardcoded 'github.concur.com' heuristic is the most noteworthy but is mitigated by VCS_TYPE taking precedence.
Review by sonnet
Evaluated against
10ef451c@@ -173,0 +175,4 @@if vcsType == "" {// Heuristic: if the URL looks like github.com or a GitHub Enterprise host,// default to GitHub. The composite action sets VCS_TYPE explicitly, so this// is a fallback for manual invocations.[MINOR] The URL heuristic for VCS type detection hardcodes 'github.concur.com' as a special case alongside 'github.com'. This company-specific hostname leaks internal infrastructure details into open-source code and is brittle — any other GHES instance won't be detected. Since VCS_TYPE env var is the recommended path and the comment acknowledges this is a fallback for manual invocations, consider dropping the hardcoded GHES heuristic and just matching 'github.com', or document the pattern more generically (e.g., check for '/api/v3' in the URL).
@@ -0,0 +21,4 @@// ErrNotSupported is returned by VCS methods that have no implementation for// a particular VCS backend (e.g., Gitea-specific timeline APIs on GitHub).var ErrNotSupported = errors.New("operation not supported on this VCS backend")[MINOR] ErrNotSupported is declared but never used anywhere in the codebase (the giteaExtClient type assertion in main.go gates calls cleanly without needing this error). Either use it (e.g., have githubVCSAdapter return it from a hypothetical future extension method) or remove it to avoid dead code. The comment on the type says 'GitHub implementations return ErrNotSupported for those' but githubVCSAdapter doesn't implement giteaExtClient at all, so this sentinel serves no purpose currently.
@@ -0,0 +50,4 @@type giteaExtClient interface {vcsClientGetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error)EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error[MINOR] The giteaExtClient interface exposes []gitea.ReviewComment (a concrete gitea package type) through its ListReviewComments method signature. This breaks the abstraction layer — the interface in the consumer package (cmd/review-bot) now directly depends on the gitea package's internal types. Since giteaExtClient is Gitea-specific by design, this is pragmatically acceptable, but it's worth noting as a design smell. If a second Gitea-compatible VCS were added, the return type would need to change.
@@ -0,0 +295,4 @@func (a *githubVCSAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {entries, err := a.c.ListContents(ctx, owner, repo, path)if err != nil {return nil, err[NIT] The comment on githubVCSAdapter.PostReview mentions that 'NewPosition from gitea diff parsing gives absolute line numbers, which will not match GitHub's position values' — this is accurate but the code proceeds to pass them anyway with Line+Side. Consider adding a TODO noting that proper GitHub diff hunk position mapping is needed for reliable inline comments, rather than just noting the limitation in a comment.
@@ -379,0 +429,4 @@}// contentResponse is the GitHub contents API response for a single file.type contentResponse struct {[NIT] getFileContentAtRef uses
filepathas a parameter name, which shadows the standard library packagepath/filepath. Whilefilepathis not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; considerfilePathorpathfor consistency.@@ -659,0 +814,4 @@w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"` + encoded + `","encoding":"base64"}`))}))defer srv.Close()[NIT] In TestGetFileContent_Base64WithNewlines, the variable
encodedis declared and then immediately suppressed with_ = encoded. The variable is unused because the test hardcodes the JSON string inline. Either remove theencodedvariable declaration entirely or use it to construct the response body.Self-review against
10ef451c20Assessment: ✅ Clean
Self-Review: issue-130 — 2026-05-14
Verdict: PASS
No blocking issues found. Three non-blocking notes for awareness:
[completeness — pre-existing]
findOwnReviewhas no production callers (only test callers). This predates this PR. Not introduced here, not fixing here to avoid scope creep.[solution — pre-existing] Pattern repo fetching uses the same VCS client as the PR target. With
VCS_TYPE=github, pattern repos must also be on GitHub. This is a pre-existing constraint (Gitea had the same: patterns must be on the same server). A follow-up issue should addPATTERNS_VCS_URLfor cross-server patterns. Noted in PR description.[fit — minor] The URL heuristic in main.go includes
github.concur.comas a hardcoded GitHub Enterprise hostname. This is specific to one deployment environment. However, the composite action setsVCS_TYPEexplicitly anyway, making the heuristic a fallback for manual invocations only. Low risk.All other checks pass: error handling propagates correctly, tests cover edge cases, no stale docs, no dead code introduced, interface placement follows codebase convention (consumer package), Go vet clean.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
10ef451c)Security Review
Summary
Overall implementation is solid and CI passed, but there is a critical security issue in the GitHub client. Several methods bypass the HTTPS enforcement present in doRequest, risking credential leakage over plaintext HTTP if misconfigured.
Findings
github/client.goRecommendation
REQUEST_CHANGES — Reject this PR until HTTPS enforcement is consistent across all GitHub client methods. Specifically, refactor PostReview, DeleteReview, and RequestReviewer to either (1) use a generalized version of doRequest that accepts a request body and validates scheme, or (2) add the same scheme check used in doRequest (reject http unless AllowInsecureHTTP is explicitly enabled with the env gate) before executing c.httpClient.Do. This ensures tokens are never sent over plaintext by misconfiguration. After this fix, re-run tests to confirm behavior, and consider adding tests that verify these methods reject HTTP when AllowInsecureHTTP is not enabled.
Review by security
Evaluated against
10ef451c@@ -379,0 +427,4 @@} `json:"user"`State string `json:"state"`}[MAJOR] PostReview, DeleteReview, and RequestReviewer construct and execute HTTP requests directly (c.httpClient.Do) without the HTTPS-only guard used in doRequest. If baseURL is accidentally configured with an http:// scheme (e.g., GHES on HTTP), these methods will send the Authorization header over plaintext, exposing tokens. Enforce scheme checks consistently or reuse a common request helper for non-GET methods.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
10ef451c)Gpt Review
Summary
Solid implementation of GitHub client methods and VCS routing with thorough tests. The consumer-defined interface and adapters are idiomatic, error handling is careful, and security-conscious HTTP behavior is in place.
Findings
cmd/review-bot/main.gocmd/review-bot/vcs.gocmd/review-bot/vcs.goRecommendation
APPROVE — Overall, the changes are well-structured and idiomatic. The introduction of a consumer-defined vcsClient interface with concrete adapters cleanly separates concerns and enables routing between Gitea and GitHub backends. The GitHub client includes robust retry logic, safe redirect policy, base64 handling, and paginated fetches. Tests are comprehensive and cover edge cases. Two small cleanups are suggested: remove or defer the unused ErrNotSupported variable, and either revise the comment about omitting unmappable inline comments or add logic to filter them if necessary. Additionally, consider making the VCS detection heuristic less brittle by avoiding hard-coded organization domains and encouraging use of VCS_TYPE for GHES. With these minor tweaks, the PR is ready to merge.
Review by gpt
Evaluated against
10ef451c@@ -173,0 +175,4 @@if vcsType == "" {// Heuristic: if the URL looks like github.com or a GitHub Enterprise host,// default to GitHub. The composite action sets VCS_TYPE explicitly, so this// is a fallback for manual invocations.[MINOR] VCS type heuristic hard-codes an organization-specific domain ("github.concur.com"). This is brittle and may misclassify GitHub Enterprise instances without that substring; consider relying on VCS_TYPE or a more general host-based detection.
@@ -0,0 +11,4 @@// its respective client package's types.import ("context"[NIT] ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code.
@@ -0,0 +256,4 @@}func (a *githubVCSAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {return a.c.GetPullRequestDiff(ctx, owner, repo, number)[NIT] Comment in githubVCSAdapter.PostReview says comments that cannot be mapped will be omitted, but the code unconditionally includes comments with Line+Side. Consider aligning the comment with behavior or adding filtering if needed.
Fix Plan against
10ef451c20Security Issue: Inconsistent HTTPS Enforcement
Current State:
doRequestincludes scheme validation that rejectshttp://unlessc.allowInsecureHTTPis truePostReview,DeleteReview,RequestReviewerall callc.httpClient.Do(req)directly, bypassing this guardbaseURLis misconfigured ashttp://, these methods will send theAuthorization: Bearer <token>header over plaintextApproach: Introduce
doRequestWithBody(ctx, method, url string, body []byte) ([]byte, error)inclient.gothat applies the same scheme check before executing. Refactor PostReview, DeleteReview, RequestReviewer to use it.Changes
Add
doRequestWithBodyingithub/client.go:doRequest(rejecthttp://unlessc.allowInsecureHTTP)[]byte; nil for bodyless requests like DELETE)Content-Type: application/jsonwhen body is non-nilAuthorization: BearerandAccept: application/vnd.github+json([]byte, error)— consistent withdoRequest/doGetRefactor in
github/client.go(PostReview, DeleteReview, RequestReviewer):http.NewRequestWithContext+ manual header set +c.httpClient.Dowithc.doRequestWithBodycallbytes.NewReader)Update tests in
github/client_test.go:http://base URLdoRequestScope
github/client.goandgithub/client_test.goonly. No API surface changes. No impact on Gitea client.Open Questions
doRequestWithBody— this matches the intent for write methods which should not be retried blindly. Correct approach given current usage.doRequestWithBodydoes not take anacceptoverride — callers always useapplication/vnd.github+json. We can add it later if needed.Sonnet Review
Summary
This PR correctly implements GitHub API methods, wires up VCS-type routing via a clean adapter pattern, and maintains all existing Gitea functionality. The code follows Go idioms well: interfaces defined in the consumer package, small adapter types, proper error wrapping, and comprehensive test coverage.
Findings
cmd/review-bot/main.gostrings.Contains(*vcsURL, "github.concur.com")is a hardcoded company-specific domain baked into an open-source tool. This is fragile — any GitHub Enterprise host other thangithub.concur.comwon't be detected. The more robust approach would be to rely on the explicitVCS_TYPEenv var for all GHES instances, or detect GHES by checking for/api/v3in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action setsVCS_TYPEexplicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.cmd/review-bot/vcs.goErrNotSupportedis declared but never used — the comment ongiteaExtClientsays "GitHub implementations return ErrNotSupported for those", butgithubVCSAdapternever returns it (those methods simply don't exist on the adapter). Either remove the variable or use it if the intent is to provide stub implementations. As-is, a linter would flag this as an unused export (thoughgo vetwon't).github/client.goGetAllFilesInPath, whenListContentsreturns a 404, the code falls back toGetFileContent. ButListContentson a file path returns the file as a single-object response (normalized to a[]ContentEntryslice) — it does NOT return 404 for files. The 404 fallback path is dead code for GitHub and misleads readers. The comment// 404 means path may be a file — try fetching directlyis accurate for Gitea but not for this GitHub client whereListContentsalready handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.cmd/review-bot/vcs.govcsPullRequestuses an anonymous struct forHeadrather than a named type. This is minor but means the struct literal syntaxr.Head.Sha = ...across both adapters is slightly awkward. A named struct would be marginally cleaner, though the current approach matches what the gitea/github packages themselves do.github/client_test.goTestGetFileContent_Base64WithNewlines, theencodedvariable is declared and assigned but only used via_ = encoded(a suppress-unused comment). The actual test hardcodes the string in thebodyliteral. This is confusing — either use the variable to build the response body, or remove the variable entirely.Recommendation
APPROVE — The PR is well-structured and ready to merge. The MINOR findings are worth addressing but don't block functionality: (1) the hardcoded
github.concur.comdomain is the most significant issue for portability and should be replaced with reliance onVCS_TYPEfor GHES, (2)ErrNotSupportedshould either be used or removed, and (3) the dead-code 404-fallback path inGetAllFilesInPathshould have a clarifying comment or be removed. The NITs are cosmetic. CI passes and all the core logic (adapter pattern, interface routing, type assertion gating for Gitea-specific ops) is correct.Review by sonnet
Evaluated against
d545abe3@@ -172,1 +173,3 @@giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)// Detect VCS type: explicit flag > env var > URL heuristic (default: gitea).vcsType := envOrDefault("VCS_TYPE", "")if vcsType == "" {[MINOR] The URL heuristic
strings.Contains(*vcsURL, "github.concur.com")is a hardcoded company-specific domain baked into an open-source tool. This is fragile — any GitHub Enterprise host other thangithub.concur.comwon't be detected. The more robust approach would be to rely on the explicitVCS_TYPEenv var for all GHES instances, or detect GHES by checking for/api/v3in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action setsVCS_TYPEexplicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.@@ -0,0 +19,4 @@"gitea.weiker.me/rodin/review-bot/review")// ErrNotSupported is returned by VCS methods that have no implementation for[MINOR]
ErrNotSupportedis declared but never used — the comment ongiteaExtClientsays "GitHub implementations return ErrNotSupported for those", butgithubVCSAdapternever returns it (those methods simply don't exist on the adapter). Either remove the variable or use it if the intent is to provide stub implementations. As-is, a linter would flag this as an unused export (thoughgo vetwon't).@@ -0,0 +84,4 @@// vcsReviewComment is an inline review comment.type vcsReviewComment struct {Path stringNewPosition int64 // Gitea: absolute line; GitHub: diff hunk position[NIT]
vcsPullRequestuses an anonymous struct forHeadrather than a named type. This is minor but means the struct literal syntaxr.Head.Sha = ...across both adapters is slightly awkward. A named struct would be marginally cleaner, though the current approach matches what the gitea/github packages themselves do.@@ -379,0 +614,4 @@return "", fmt.Errorf("decode base64 content for %s: %w", filepath, err)}return string(decoded), nil}[MINOR] In
GetAllFilesInPath, whenListContentsreturns a 404, the code falls back toGetFileContent. ButListContentson a file path returns the file as a single-object response (normalized to a[]ContentEntryslice) — it does NOT return 404 for files. The 404 fallback path is dead code for GitHub and misleads readers. The comment// 404 means path may be a file — try fetching directlyis accurate for Gitea but not for this GitHub client whereListContentsalready handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.@@ -659,0 +772,4 @@t.Fatalf("unexpected error: %v", err)}if len(files) != 101 {t.Errorf("len(files) = %d, want 101", len(files))[NIT] In
TestGetFileContent_Base64WithNewlines, theencodedvariable is declared and assigned but only used via_ = encoded(a suppress-unused comment). The actual test hardcodes the string in thebodyliteral. This is confusing — either use the variable to build the response body, or remove the variable entirely.Gpt Review
Summary
Solid implementation of GitHub API client methods and a clean VCS routing layer. The adapters and shared VCS types are idiomatic, errors are wrapped with context, and comprehensive tests cover success, pagination, error cases, and security constraints.
Recommendation
APPROVE — The changes cleanly introduce a consumer-defined VCS interface with adapters for Gitea and GitHub, enabling dual-backend support without complicating main. The GitHub client is robust, enforces HTTPS by default (with an explicit, gated opt-out), implements pagination, and handles GitHub-specific edge cases. Tests are thorough, including retry behavior, header checks, base64 decoding with newlines, and URL redaction. CI has passed. Consider, if needed, future enhancements like supporting GitHub check runs in CI evaluation and refining inline comment mapping for GitHub when practical, but these are non-blocking. Approve as is.
Review by gpt
Evaluated against
d545abe3Security Review
Summary
Security posture is solid: HTTPS enforcement, safe redirect policy, bounded reads, and careful handling of API errors are all implemented. VCS routing and GitHub client methods avoid common pitfalls and do not expose secrets in logs.
Findings
cmd/review-bot/main.goRecommendation
APPROVE — Approve the changes. The implementation enforces HTTPS by default, blocks cross-host and HTTPS→HTTP redirects to prevent credential leakage, limits response body sizes to mitigate DoS, and sanitizes error messages for safe logging. GitHub API methods properly escape paths and decode base64 content, and the VCS adapter pattern keeps GitHub/Gitea behavior separated with clear capability boundaries. As a defense-in-depth improvement, consider tightening GitHub API URL derivation by requiring explicit VCS_TYPE for GitHub or validating the server host against a configured allowlist to avoid sending tokens to unintended domains in case of operator misconfiguration. Otherwise, the changes are sound from a security perspective.
Review by security
Evaluated against
d545abe3@@ -654,6 +692,19 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strinreturn true, "all checks passed"[MINOR] githubAPIURL derives the API host directly from the provided server URL. With the heuristic that detects GitHub when the URL contains "github.com", a misconfigured or malicious host like "https://github.com.evil.com" would be treated as GHES and receive the Authorization bearer token. While this is an operator configuration input (not user-controlled), consider hardening by requiring explicit VCS_TYPE and/or validating the host against an allowlist to reduce the chance of credential leakage due to misconfiguration.