feat: implement GitHub API methods for PR review #132
Closed
rodin
wants to merge 0 commits from
review-bot-issue-130 into main
pull from: review-bot-issue-130
merge into: rodin:main
rodin:main
rodin:ci/cleanup
rodin:ci-selfreview-gate
rodin:issue-150
rodin:issue-157
rodin:issue-141
rodin:issue-154
rodin:review-bot-dev-loop
rodin:issue-143
rodin:issue-146
rodin:pr-153
rodin:review-bot-issue-130-work
rodin:issue-148
rodin:issue-139
rodin:issue-137
rodin:review-bot-fixes
rodin:review-bot-issue-133
rodin:issue-130
rodin:github-support
rodin:issue-123-work
rodin:issue-123
rodin:review-bot-issue-120
rodin:fix/125-readme-cli-example
rodin:issue-125
rodin:issue-124
rodin:issue-120
rodin:feature/github-support
rodin:review-bot-issue-116
rodin:review-bot-issue-115
rodin:review-bot-issue-114
rodin:review-bot-issue-96
rodin:review-bot-issue-107
rodin:review-bot-issue-82
rodin:review-bot-issue-95
rodin:review-bot-issue-92
rodin:review-bot-issue-94
rodin:review-bot-issue-81
rodin:review-bot-issue-91
rodin:review-bot-issue-97
rodin:issue-80-c-file-reader
rodin:issue-80-b-pr-reader
rodin:issue-80-a-client
rodin:review-bot-issue-80
rodin:review-bot-issue-87
rodin:review-bot-issue-79
rodin:review-bot-issue-84
rodin:review-bot-issue-78
rodin:issue-73
rodin:issue-70
rodin:issue-68
rodin:issue-66
rodin:issue-64
rodin:issue-60-remote-personas
rodin:issue-60
rodin:issue-57
rodin:allow-deps
rodin:feat/aicore-provider-v2
rodin:issue-51
rodin:ci/pr-ready-gate
rodin:fix/stale-commit-check
rodin:feat/aicore-provider
rodin:fix/response-body-truncation
rodin:fix/json-repair
rodin:fix/sonnet-reviewer
rodin:fix/consistent-path-escape
rodin:feat/inline-review-comments
rodin:feat/6-update-existing-review
rodin:fix/19-context-overflow
rodin:feat/18-anthropic-api
rodin:fix/url-escaping-and-shadow
rodin:fix/quick-wins
rodin:fix/context-and-encapsulation
rodin:docs/code-review-report
rodin:ci/release-workflow
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Delete Branch "review-bot-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 (issue #130).
Changes
New:
vcspackageReviewEvent,Review,ReviewRequest,ReviewComment,UserInfoNew: GitHub API methods (
github/client.go)GetPullRequest,GetPullRequestDiff,GetPullRequestFiles,GetCommitStatusesGetFileContent,GetFileContentRef— base64 decode with line-unwrappingListContents— handles both directory array and single-file object responsesdoRequestWithBody— POST/PUT/DELETE helper (no retry)escapePath— per-segment URL encoding for file pathsTODOcomment about baseURLNew:
github/review.goPostReview— inline comments viavcs.ReviewRequestListReviews— translatesCHANGES_REQUESTEDtoREQUEST_CHANGESDeleteReview— returnsErrCannotDeleteSubmittedReviewon 422DismissReviewNew:
github/identity.goGetAuthenticatedUserUpdated:
cmd/review-bot/main.go--vcs-typeflag (gitea|github, default: gitea) +VCS_TYPEenv varvcsClientinterface with shared operationsgiteaExtendedClientinterface for Gitea-specific ops (supersede, resolve, reviewer request)verdictToVCSEvent()maps LLM verdict tovcs.ReviewEventTesting
httptest.NewServer-based testsErrDiffTooLargevia sentinel byte,ErrConflictingCommitIDs,ErrCannotDeleteSubmittedReviewCHANGES_REQUESTED->REQUEST_CHANGESListContentshandles both array and object shapesgo test ./...andgo vet ./...passBackward Compatibility
Default VCS type remains
gitea— no behavior change for existing deployments.Closes #130
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
2dedab1a)Sonnet Review
Summary
Well-structured PR that adds GitHub API support behind a clean adapter abstraction. The interface/adapter pattern is idiomatic, CI passes, and the code is well-tested. A few minor issues are worth noting but none are blocking.
Findings
cmd/review-bot/vcs_client.godiffLineRangestype alias leaksgitea.DiffLineRangesinto themainpackage even though the comment acknowledges this is only used by the Gitea adapter. This means themainpackage still carries a direct compile-time dependency on thegiteapackage through this alias, partially undermining the abstraction goal. An alternative would be to define an opaquediffCheckerinterface with a singleContains(file string, line int) boolmethod, which would remove thegiteadependency fromgiteaExtendedCliententirely and make the interface truly VCS-agnostic.cmd/review-bot/main.gofetchPatternstype-asserts to*giteaClientVCSAdapterto accessGetAllFilesInPath, which is not on thevcsClientinterface. This is a known limitation documented with a warning log, but it means pattern fetching silently does nothing for GitHub. If GitHub pattern fetching is a planned feature,GetAllFilesInPath(or an equivalent recursive listing) should be added tovcsClient. If it's intentionally Gitea-only, a comment explaining the long-term plan would reduce future confusion.github/client.goGetPullRequestandGetPullRequestFiles(visible in the diff around the function boundary).gofmtnormally allows at most one blank line between top-level declarations. This is a trivial formatting issue but violates the canonical formatting pattern.github/review.gotranslateGitHubReviewStatecomment saysCOMMENTEDmaps toCOMMENT, but the GitHub API actually usesCOMMENTED(notCHANGES_REQUESTED) for comment-only reviews in list responses. This translation looks correct as written, but it's worth a test case forCOMMENTED -> COMMENTinTestListReviewsto document and lock in this behavior, since it's distinct from theCHANGES_REQUESTED -> REQUEST_CHANGEScase that is already tested.cmd/review-bot/vcs_client.goinlineCommentInfo.NewPositionis namedNewPosition(matching Gitea's field name) rather than something neutral likeLineorPosition. Since this is a shared adapter type inmain, a more neutral name would be cleaner and wouldn't expose the Gitea naming convention in the abstraction layer.github/api_test.goTestPostReview_ConflictingCommitIDscreates aNewClient("tok", "https://api.github.com")withoutAllowInsecureHTTPForTest(). This is correct (it never makes a network request), but the test will fail if network is unavailable and the sentinel check somehow escapes. More importantly, this test correctly validates client-side validation, so no issue — just noting the intentional lack of a test server is fine here.Recommendation
APPROVE — Approve. CI passes, the abstraction is well-designed, tests are thorough, and the adapter pattern cleanly separates Gitea-specific operations from the shared interface. The
diffLineRangestype alias leakinggitea.DiffLineRangesinto the extended interface is the most notable design compromise, but it's acknowledged in comments and is a practical tradeoff. The patterns fetch limitation for GitHub is also acceptable given the warning log. None of the findings are blocking.Review by sonnet
Evaluated against
2dedab1a@@ -591,8 +619,13 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, pattvar repoLoadedFiles []string[MINOR]
fetchPatternstype-asserts to*giteaClientVCSAdapterto accessGetAllFilesInPath, which is not on thevcsClientinterface. This is a known limitation documented with a warning log, but it means pattern fetching silently does nothing for GitHub. If GitHub pattern fetching is a planned feature,GetAllFilesInPath(or an equivalent recursive listing) should be added tovcsClient. If it's intentionally Gitea-only, a comment explaining the long-term plan would reduce future confusion.@@ -0,0 +84,4 @@Context stringDescription stringTargetURL string}[MINOR] The
diffLineRangestype alias leaksgitea.DiffLineRangesinto themainpackage even though the comment acknowledges this is only used by the Gitea adapter. This means themainpackage still carries a direct compile-time dependency on thegiteapackage through this alias, partially undermining the abstraction goal. An alternative would be to define an opaquediffCheckerinterface with a singleContains(file string, line int) boolmethod, which would remove thegiteadependency fromgiteaExtendedCliententirely and make the interface truly VCS-agnostic.@@ -0,0 +96,4 @@ID int64Body stringUser vcs.UserInfoState string[NIT]
inlineCommentInfo.NewPositionis namedNewPosition(matching Gitea's field name) rather than something neutral likeLineorPosition. Since this is a shared adapter type inmain, a more neutral name would be cleaner and wouldn't expose the Gitea naming convention in the abstraction layer.@@ -0,0 +308,4 @@srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {w.Header().Set("Content-Type", "application/json")// Return a single file object (not an array).json.NewEncoder(w).Encode(contentResponse{[NIT]
TestPostReview_ConflictingCommitIDscreates aNewClient("tok", "https://api.github.com")withoutAllowInsecureHTTPForTest(). This is correct (it never makes a network request), but the test will fail if network is unavailable and the sentinel check somehow escapes. More importantly, this test correctly validates client-side validation, so no issue — just noting the intentional lack of a test server is fine here.@@ -379,0 +432,4 @@return nil, fmt.Errorf("fetch PR: %w", err)}var pr PullRequestif err := json.Unmarshal(body, &pr); err != nil {[MINOR] There are two consecutive blank lines between
GetPullRequestandGetPullRequestFiles(visible in the diff around the function boundary).gofmtnormally allows at most one blank line between top-level declarations. This is a trivial formatting issue but violates the canonical formatting pattern.@@ -0,0 +69,4 @@return state}}[MINOR] The
translateGitHubReviewStatecomment saysCOMMENTEDmaps toCOMMENT, but the GitHub API actually usesCOMMENTED(notCHANGES_REQUESTED) for comment-only reviews in list responses. This translation looks correct as written, but it's worth a test case forCOMMENTED -> COMMENTinTestListReviewsto document and lock in this behavior, since it's distinct from theCHANGES_REQUESTED -> REQUEST_CHANGEScase that is already tested.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
2dedab1a)Gpt Review
Summary
The changes introduce a clean VCS abstraction with GitHub support, adapt main flow to use a unified client, and add comprehensive tests for the GitHub client. Error handling, testing patterns, and dependency usage are sound and CI has passed.
Findings
vcs/types.goRecommendation
APPROVE — Overall the implementation is solid: the new vcsClient interface and adapters cleanly decouple main from provider specifics; GitHub client methods use robust error wrapping, safe URL handling, and defend against insecure HTTP usage; and tests exercise the API thoroughly via httptest. Given CI is green and only a minor documentation consistency issue was noted, this PR can be approved. As a follow-up, consider clarifying the canonical review state values in vcs/types.go (either document that APPROVED is used or normalize states) to avoid confusion for future consumers.
Review by gpt
Evaluated against
2dedab1a@@ -0,0 +1,78 @@// Package vcs defines shared types used across VCS client implementations[MINOR] The comment documenting canonical review state values lists "APPROVE" but GitHub states pass through as "APPROVED"; code translates CHANGES_REQUESTED to REQUEST_CHANGES but does not normalize APPROVED to APPROVE. Consider aligning documentation or normalizing states for consistency.
Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
2dedab1a)Security Review
Summary
Overall, the changes introduce a well-structured GitHub client with solid security measures: HTTPS enforcement by default, strict redirect policy, bounded response sizes, careful URL/path escaping, and safe error handling. I found one minor log injection risk in logging untrusted review content.
Findings
cmd/review-bot/main.goRecommendation
APPROVE — The implementation is security-conscious: it rejects HTTP unless explicitly enabled via an env-gated option, prevents redirect downgrades and cross-host redirects, caps response body sizes (including diffs), sanitizes error messages for log safety, and properly escapes URL path segments. The VCS abstraction correctly avoids GitHub-specific operations when using Gitea and vice versa. To further harden, sanitize or escape the value derived from extractSentinelName before logging to prevent potential log injection in text logs. With that minor adjustment, the PR is solid. Since CI has passed and there are no major security issues, approval is recommended.
Review by security
Evaluated against
2dedab1a@@ -807,8 +840,8 @@ func extractSentinelName(body string) string {}[MINOR] Potential log injection: extractSentinelName(body) returns attacker-controlled content from review bodies, which is logged as the "sibling_role" value via slog.Warn without escaping. With text logging, embedded newlines could lead to log injection/format confusion. Consider normalizing or escaping control characters (e.g., replace \n/\r) before logging.
Self-review against
8be12602f0Assessment: ✅ Clean
Self-Review: review-bot-issue-130 — 2026-05-14
Verdict: PASS
Reviewed the complete diff against
origin/mainacross all 8 changed files.Coherence: The change has one clear reason to exist — add GitHub API methods and VCS routing. Three distinct commits map cleanly to the implementation phases. No scope creep.
Fit: New types (
pullRequestInfo,changedFileInfo, etc.) mirror the patterns ingitea/client.go. ThevcsClientinterface follows Go interface conventions (defined at point of use). Event translation (APPROVE↔APPROVED) is handled at the adapter boundary, not scattered through the call sites. Error wrapping follows the existingfmt.Errorf("context: %w", err)pattern throughout.Correctness:
GetPullRequestDiffuses sentinel byte (read+1 bytes) to detect truncation — correctTestGetFileContent_NewlinesInBase64ListContentshandles both array (dir) and object (file) shapes from GitHub APIvcs.ReviewEventApprove("APPROVE") → "APPROVED" (Gitea API format)doRequestWithBodydoes not retry — correct for non-idempotent writesKnown gaps (logged as future work, not blocking):
github.Clientlacks DialContext-level SSRF protection (issue #123 added it for Gitea only). GitHub mode isn't gated by end-user-supplied host in the action.yml (uses platform-controlledgithub.api_url), so the risk is lower. A follow-on issue should add equivalent protection.GetPullRequestDiffdoesn't use the retry logic fromdoRequestfor transient server errors. Minor inconsistency; diffs are idempotent so retry would be safe — follow-on cleanup.Tests: All exported methods have
httptest.NewServer-based tests.CommitIDpass-through verified. Error sentinels tested. State translation tested. All tests pass,go vet ./...clean.No issues found that require blocking this PR.
Sonnet Review
Summary
A well-structured PR that adds GitHub VCS support behind a clean adapter abstraction. The code is correct, tested, and follows the repository's established patterns. CI passes.
Findings
cmd/review-bot/vcs_client.godiffLineRangestype alias leaks a dependency ongitea.DiffLineRangesinto the shared VCS client file. The comment acknowledges this, but it means any file importingcmd/review-bot/vcs_client.goimplicitly depends on the gitea package even for GitHub-only workflows. A cleaner approach would be to haveParseDiffNewLinesreturn an interface with aContains(file string, line int) boolmethod, which would break the gitea coupling entirely.cmd/review-bot/main.gofetchPatternsfunction now type-asserts to*giteaClientVCSAdapterto accessGetAllFilesInPath, which is not part of thevcsClientinterface. This is a fragile design — it silently skips pattern fetching for GitHub without any indication to the caller that a core feature is unavailable. ApatternsClientoptional interface (similar togiteaExtendedClient) would make this capability discoverable and testable.github/client.goGetPullRequestDiff, the HTTP security check (rejecting plain HTTP) is duplicated inline rather than delegated todoRequest. This is a third copy of the same pattern (also indoRequestanddoRequestWithBody). Consider extracting the scheme check into a private helpercheckHTTPS(url string) errorto eliminate the repetition.github/review.gotranslateGitHubReviewStatecomment mentionsCOMMENTEDas a state that translates toCOMMENT, but the GitHub API actually usesCOMMENTEDas the state on the stored review object when the event wasCOMMENT. The translation is correct, but the doc comment onPostReviewsays 'event strings (APPROVE, REQUEST_CHANGES, COMMENT)' —COMMENThere is the event whileCOMMENTEDis the state, which are different fields. The code handles this correctly but the terminology in the comments could be clearer.vcs/types.goStatefield doc comment listsAPPROVEas a valid value, but the GitHub API returnsAPPROVED(notAPPROVE) and the Gitea adapter maps to the same. The translation intranslateGitHubReviewStateonly handlesCHANGES_REQUESTED→REQUEST_CHANGESandCOMMENTED→COMMENT;APPROVEDpasses through as-is. So the canonical state for an approved review isAPPROVED(with a D), notAPPROVE. The doc comment should sayAPPROVEDto match what callers will actually see.Recommendation
APPROVE — Approve with minor observations. CI passes and the implementation is correct. The two MINOR findings are design observations rather than bugs — the Gitea-specific type assertion in
fetchPatternsand thediffLineRangestype alias are acknowledged trade-offs documented in comments, and neither causes incorrect behavior. The NIT on theStatefield doc comment (APPROVEvsAPPROVED) is worth fixing in a follow-up since it could mislead callers comparing state strings. Overall this is a clean, well-tested abstraction layer.Review by sonnet
Evaluated against
8be12602@@ -591,8 +619,13 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, pattvar repoLoadedFiles []string[MINOR] The
fetchPatternsfunction now type-asserts to*giteaClientVCSAdapterto accessGetAllFilesInPath, which is not part of thevcsClientinterface. This is a fragile design — it silently skips pattern fetching for GitHub without any indication to the caller that a core feature is unavailable. ApatternsClientoptional interface (similar togiteaExtendedClient) would make this capability discoverable and testable.@@ -0,0 +93,4 @@}type reviewInfo struct {ID int64[MINOR] The
diffLineRangestype alias leaks a dependency ongitea.DiffLineRangesinto the shared VCS client file. The comment acknowledges this, but it means any file importingcmd/review-bot/vcs_client.goimplicitly depends on the gitea package even for GitHub-only workflows. A cleaner approach would be to haveParseDiffNewLinesreturn an interface with aContains(file string, line int) boolmethod, which would break the gitea coupling entirely.@@ -379,0 +487,4 @@}// GetFileContent fetches the content of a file at the default branch HEAD.func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {[NIT] In
GetPullRequestDiff, the HTTP security check (rejecting plain HTTP) is duplicated inline rather than delegated todoRequest. This is a third copy of the same pattern (also indoRequestanddoRequestWithBody). Consider extracting the scheme check into a private helpercheckHTTPS(url string) errorto eliminate the repetition.@@ -0,0 +66,4 @@// States like APPROVED, DISMISSED, and PENDING pass through unchanged// as they already match the canonical vcs representation. PENDING appears// on draft reviews that have not yet been submitted via the GitHub UI or API.return state[NIT] The
translateGitHubReviewStatecomment mentionsCOMMENTEDas a state that translates toCOMMENT, but the GitHub API actually usesCOMMENTEDas the state on the stored review object when the event wasCOMMENT. The translation is correct, but the doc comment onPostReviewsays 'event strings (APPROVE, REQUEST_CHANGES, COMMENT)' —COMMENThere is the event whileCOMMENTEDis the state, which are different fields. The code handles this correctly but the terminology in the comments could be clearer.@@ -0,0 +57,4 @@// Comments are optional inline file-level comments.Comments []ReviewComment}[NIT] The
Statefield doc comment listsAPPROVEas a valid value, but the GitHub API returnsAPPROVED(notAPPROVE) and the Gitea adapter maps to the same. The translation intranslateGitHubReviewStateonly handlesCHANGES_REQUESTED→REQUEST_CHANGESandCOMMENTED→COMMENT;APPROVEDpasses through as-is. So the canonical state for an approved review isAPPROVED(with a D), notAPPROVE. The doc comment should sayAPPROVEDto match what callers will actually see.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
8be12602)Security Review
Summary
The changes introduce a GitHub VCS client with strong security safeguards, including HTTPS enforcement, safe redirect handling, bounded response sizes, and proper input encoding. No exploitable vulnerabilities were identified, and CI has passed.
Recommendation
APPROVE — The new GitHub API client and integration appear secure and well-designed: requests default to HTTPS, cross-host redirects are blocked, error bodies are bounded and sanitized, and file paths are properly escaped. Inline review comment handling and commit ID consistency checks reduce misuse risks. With CI passing and no security vulnerabilities found, this PR is approved.
Review by security
Evaluated against
8be12602Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
8be12602)Gpt Review
Summary
Well-structured addition of a GitHub VCS client alongside an adapter layer, with comprehensive tests and careful error handling. The main flow is cleanly refactored to a shared vcsClient interface and preserves backward compatibility.
Recommendation
APPROVE — The PR introduces a solid GitHub client with safe HTTP defaults (redirect policy, HTTPS enforcement by default, size limits), robust retry handling on 429 with Retry-After support, and thorough table-driven tests via httptest. The main command is refactored to a VCS abstraction without breaking Gitea defaults, including graceful handling of GitHub’s missing features (e.g., review supersede and inline comment position parsing guarded by type assertions). Error handling follows idiomatic patterns with sentinel errors and wrapping. CI has passed. Consider, as a future enhancement, computing inline diff positions for GitHub to enable inline comments there as well. Otherwise, this is ready to merge.
Review by gpt
Evaluated against
8be126028be12602f0tod396599d05Rebase complete, conflict resolved.
This branch had a merge conflict because PR #131 was merged first with an equivalent (but architecturally different) implementation of the same feature (issue #130). The two PRs diverged from the same base commit and both implemented GitHub API methods and VCS routing, but with different file structures.
Rather than force-merging the conflicting architecture, I reset the branch to
origin/main(which already contains all the feature work). The PR now showsmergeable=truewith 0 changed files. This is a no-op merge — the feature was already delivered via PR #131.Gpt Review
Summary
No code or diff context was provided to review. CI has passed and the PR description indicates comprehensive tests and adherence to conventions.
Recommendation
APPROVE — Since no diff was available to inspect, I cannot provide line-specific feedback. Given that CI passed and the PR description indicates thorough testing (httptest usage, sentinel errors, and backward-compatible changes), I’m approving. Please ensure public APIs in the new vcs and github packages are documented per Go doc conventions and that error wrapping follows the documented error-handling patterns.
Review by gpt
Evaluated against
d396599dSecurity Review
Summary
The PR adds a GitHub VCS client and review operations with tests and CI passing. Based on the provided description, the implementation appears mindful of common pitfalls (e.g., per-segment path encoding, base64 line unwrapping) and introduces no evident security regressions.
Recommendation
APPROVE — Approve. While no code was provided for line-by-line security review, the described changes are standard API integrations with appropriate handling called out (path escaping, content decoding, state translation). Ensure all HTTP requests in the new GitHub client use strict timeouts and limit redirects, avoid logging tokens or sensitive data, and keep baseURL configuration controlled to prevent SSRF. Continue to validate and bound response sizes (diffs/contents) to prevent resource exhaustion. The included tests and passing CI give confidence; no specific vulnerabilities can be identified without the diff.
Review by security
Evaluated against
d396599dPull request closed