feat: implement GitHub API methods and VCS routing (issue #130) #131

Merged
rodin merged 3 commits from issue-130 into main 2026-05-14 22:06:21 +00:00
Owner

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 methods

Implements all higher-level methods that were TODO since issue #120:

Method GitHub API
GetPullRequest GET /repos/{owner}/{repo}/pulls/{number}
GetPullRequestDiff Same URL with Accept: application/vnd.github.diff
GetPullRequestFiles GET /repos/{owner}/{repo}/pulls/{number}/files (paginated)
GetCommitStatuses GET /repos/{owner}/{repo}/commits/{sha}/statuses
GetFileContent GET /repos/{owner}/{repo}/contents/{path} (base64 decoded)
GetFileContentRef Same with ?ref=
ListContents Same endpoint, normalizes single-file to slice
GetAllFilesInPath Recursive file fetch
PostReview POST /repos/{owner}/{repo}/pulls/{number}/reviews
ListReviews GET /repos/{owner}/{repo}/pulls/{number}/reviews (paginated)
DeleteReview DELETE .../reviews/{id} (GitHub: PENDING only)
GetAuthenticatedUser GET /user
RequestReviewer POST .../requested_reviewers

Edge cases handled:

  • GitHub embeds \n in base64 content — stripped before decode
  • ListContents on a file path returns a single object (not array) — normalized
  • DeleteReview documents GitHub's PENDING-only constraint

Phase 2: cmd/review-bot/vcs.go + main.go — VCS routing

New vcs.go:

  • vcsClient interface — consumer-defined (Go idiom), covers all PR ops
  • giteaExtClient interface — Gitea-specific extension (supersede, comment resolution)
  • giteaVCSAdapter — wraps gitea.Client, satisfies both interfaces
  • githubVCSAdapter — wraps github.Client, satisfies vcsClient

Updated main.go:

  • VCS type detection: VCS_TYPE env var > URL heuristic (contains github.com)
  • GitHub API URL derived: github.comapi.github.com, GHES → /api/v3
  • All review flow uses vcsClient interface
  • Gitea-specific supersede gated with giteaExtClient type assertion
  • GitHub: logs info when supersede is skipped (not supported by GitHub API)
  • Removes old giteaClientAdapter (replaced by giteaVCSAdapter)

Test Coverage

22 new test cases in github/client_test.go:

  • All API methods covered
  • Pagination (GetPullRequestFiles, 100-item boundary)
  • Error cases (404/IsNotFound, 401/IsUnauthorized, 422)
  • Base64 with embedded newlines
  • Single-file vs directory ListContents
  • escapePath with special characters

Existing tests updated for type changes (gitea types → vcs types).

GitHub Limitations

Feature Gitea GitHub
Review supersede (edit old review body) (GitHub doesn't allow editing submitted reviews)
Comment resolution (no API equivalent)
Review deletion ⚠️ PENDING only

All limitations handled gracefully — logged as info, review-bot continues.

Checklist

  • All API methods implemented and tested
  • Base64 decode handles GitHub's embedded newlines
  • VCS routing via VCS_TYPE env var
  • GitHub Enterprise Server supported (/api/v3 URL pattern)
  • go test ./... passes
  • go vet ./... passes clean
  • TODO comment removed from github/client.go
  • GitHub limitations documented and handled
  • Existing Gitea functionality unchanged

Closes #130

## 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 methods Implements all higher-level methods that were TODO since issue #120: | Method | GitHub API | |--------|------------| | `GetPullRequest` | `GET /repos/{owner}/{repo}/pulls/{number}` | | `GetPullRequestDiff` | Same URL with `Accept: application/vnd.github.diff` | | `GetPullRequestFiles` | `GET /repos/{owner}/{repo}/pulls/{number}/files` (paginated) | | `GetCommitStatuses` | `GET /repos/{owner}/{repo}/commits/{sha}/statuses` | | `GetFileContent` | `GET /repos/{owner}/{repo}/contents/{path}` (base64 decoded) | | `GetFileContentRef` | Same with `?ref=` | | `ListContents` | Same endpoint, normalizes single-file to slice | | `GetAllFilesInPath` | Recursive file fetch | | `PostReview` | `POST /repos/{owner}/{repo}/pulls/{number}/reviews` | | `ListReviews` | `GET /repos/{owner}/{repo}/pulls/{number}/reviews` (paginated) | | `DeleteReview` | `DELETE .../reviews/{id}` (GitHub: PENDING only) | | `GetAuthenticatedUser` | `GET /user` | | `RequestReviewer` | `POST .../requested_reviewers` | Edge cases handled: - GitHub embeds `\n` in base64 content — stripped before decode - `ListContents` on a file path returns a single object (not array) — normalized - `DeleteReview` documents GitHub's PENDING-only constraint ### Phase 2: `cmd/review-bot/vcs.go` + `main.go` — VCS routing **New `vcs.go`:** - `vcsClient` interface — consumer-defined (Go idiom), covers all PR ops - `giteaExtClient` interface — Gitea-specific extension (supersede, comment resolution) - `giteaVCSAdapter` — wraps `gitea.Client`, satisfies both interfaces - `githubVCSAdapter` — wraps `github.Client`, satisfies `vcsClient` **Updated `main.go`:** - VCS type detection: `VCS_TYPE` env var > URL heuristic (contains `github.com`) - GitHub API URL derived: `github.com` → `api.github.com`, GHES → `/api/v3` - All review flow uses `vcsClient` interface - Gitea-specific supersede gated with `giteaExtClient` type assertion - GitHub: logs info when supersede is skipped (not supported by GitHub API) - Removes old `giteaClientAdapter` (replaced by `giteaVCSAdapter`) ## Test Coverage 22 new test cases in `github/client_test.go`: - All API methods covered - Pagination (GetPullRequestFiles, 100-item boundary) - Error cases (404/IsNotFound, 401/IsUnauthorized, 422) - Base64 with embedded newlines - Single-file vs directory ListContents - `escapePath` with special characters Existing tests updated for type changes (gitea types → vcs types). ## GitHub Limitations | Feature | Gitea | GitHub | |---------|-------|--------| | Review supersede (edit old review body) | ✅ | ❌ (GitHub doesn't allow editing submitted reviews) | | Comment resolution | ✅ | ❌ (no API equivalent) | | Review deletion | ✅ | ⚠️ PENDING only | All limitations handled gracefully — logged as info, review-bot continues. ## Checklist - [x] All API methods implemented and tested - [x] Base64 decode handles GitHub's embedded newlines - [x] VCS routing via `VCS_TYPE` env var - [x] GitHub Enterprise Server supported (`/api/v3` URL pattern) - [x] `go test ./...` passes - [x] `go vet ./...` passes clean - [x] TODO comment removed from `github/client.go` - [x] GitHub limitations documented and handled - [x] Existing Gitea functionality unchanged Closes #130
rodin added 2 commits 2026-05-14 20:44:20 +00:00
Implement the higher-level GitHub API methods that were TODO since
issue #120. The github package now provides:

- GetPullRequest: PR metadata (title, body, head SHA/ref, draft)
- GetPullRequestDiff: unified diff via Accept: application/vnd.github.diff
- GetPullRequestFiles: changed files list (paginated, 100/page)
- GetCommitStatuses: CI statuses (GitHub uses 'state' field, normalized)
- GetFileContent: file content with base64 decode (strips embedded newlines)
- GetFileContentRef: file at a specific ref
- ListContents: directory listing or single-file normalization
- GetAllFilesInPath: recursive file fetching
- PostReview: submit review with event/body/commit/inline comments
- ListReviews: list PR reviews (paginated)
- DeleteReview: delete review (GitHub only allows PENDING deletion)
- GetAuthenticatedUser: returns login of the authed user
- RequestReviewer: add a user as requested reviewer

API types added: PullRequest, CommitStatus, ChangedFile, ReviewComment,
Review, ContentEntry.

Notable edge cases handled:
- GitHub embeds newlines in base64 content; stripped before decode
- GetFileContent returns error for non-file paths (type=dir)
- ListContents normalizes single-file response to a slice
- DeleteReview documents GitHub's PENDING-only constraint

Removes TODO comment from baseURL field (now consumed by all methods).

Closes part of #130.

Co-authored-by: Rodin <rodin@forgedthought.ai>
feat(cmd): add VCS routing for GitHub PR reviews
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m23s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m38s
10ef451c20
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>
rodin changed title from test to feat: implement GitHub API methods and VCS routing (issue #130) 2026-05-14 20:44:26 +00:00
rodin added the needs-review label 2026-05-14 20:44:54 +00:00
sonnet-review-bot approved these changes 2026-05-14 20:45:20 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee 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

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 178 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).
2 [MINOR] cmd/review-bot/vcs.go 24 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.
3 [MINOR] cmd/review-bot/vcs.go 53 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.
4 [NIT] cmd/review-bot/vcs.go 298 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.
5 [NIT] github/client_test.go 817 In TestGetFileContent_Base64WithNewlines, the variable encoded is declared and then immediately suppressed with _ = encoded. The variable is unused because the test hardcodes the JSON string inline. Either remove the encoded variable declaration entirely or use it to construct the response body.
6 [NIT] github/client.go 432 getFileContentAtRef uses filepath as a parameter name, which shadows the standard library package path/filepath. While filepath is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider filePath or path for 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

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/131#pullrequestreview-3755) for up-to-date findings. <details><summary>Previous findings (commit 10ef451c)</summary> # 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 | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 178 | 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). | | 2 | [MINOR] | `cmd/review-bot/vcs.go` | 24 | 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. | | 3 | [MINOR] | `cmd/review-bot/vcs.go` | 53 | 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. | | 4 | [NIT] | `cmd/review-bot/vcs.go` | 298 | 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. | | 5 | [NIT] | `github/client_test.go` | 817 | In TestGetFileContent_Base64WithNewlines, the variable `encoded` is declared and then immediately suppressed with `_ = encoded`. The variable is unused because the test hardcodes the JSON string inline. Either remove the `encoded` variable declaration entirely or use it to construct the response body. | | 6 | [NIT] | `github/client.go` | 432 | getFileContentAtRef uses `filepath` as a parameter name, which shadows the standard library package `path/filepath`. While `filepath` is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider `filePath` or `path` for 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* <!-- review-bot:sonnet --> --- *Evaluated against 10ef451c* </details> <!-- review-bot:sonnet -->
@@ -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.
First-time contributor

[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).

**[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")
First-time contributor

[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.

**[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 {
vcsClient
GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error)
EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error
First-time contributor

[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.

**[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
First-time contributor

[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.

**[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 {
First-time contributor

[NIT] getFileContentAtRef uses filepath as a parameter name, which shadows the standard library package path/filepath. While filepath is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider filePath or path for consistency.

**[NIT]** getFileContentAtRef uses `filepath` as a parameter name, which shadows the standard library package `path/filepath`. While `filepath` is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider `filePath` or `path` for consistency.
@@ -659,0 +814,4 @@
w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"` + encoded + `","encoding":"base64"}`))
}))
defer srv.Close()
First-time contributor

[NIT] In TestGetFileContent_Base64WithNewlines, the variable encoded is declared and then immediately suppressed with _ = encoded. The variable is unused because the test hardcodes the JSON string inline. Either remove the encoded variable declaration entirely or use it to construct the response body.

**[NIT]** In TestGetFileContent_Base64WithNewlines, the variable `encoded` is declared and then immediately suppressed with `_ = encoded`. The variable is unused because the test hardcodes the JSON string inline. Either remove the `encoded` variable declaration entirely or use it to construct the response body.
Author
Owner

Self-review against 10ef451c20

Assessment: Clean

Self-Review: issue-130 — 2026-05-14

Verdict: PASS

No blocking issues found. Three non-blocking notes for awareness:

[completeness — pre-existing] findOwnReview has 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 add PATTERNS_VCS_URL for cross-server patterns. Noted in PR description.

[fit — minor] The URL heuristic in main.go includes github.concur.com as a hardcoded GitHub Enterprise hostname. This is specific to one deployment environment. However, the composite action sets VCS_TYPE explicitly 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.

Self-review against 10ef451c20d6e4740b8d26b83c3fc8a0e0d03d97 Assessment: ✅ Clean ## Self-Review: issue-130 — 2026-05-14 ### Verdict: PASS No blocking issues found. Three non-blocking notes for awareness: **[completeness — pre-existing]** `findOwnReview` has 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 add `PATTERNS_VCS_URL` for cross-server patterns. Noted in PR description. **[fit — minor]** The URL heuristic in main.go includes `github.concur.com` as a hardcoded GitHub Enterprise hostname. This is specific to one deployment environment. However, the composite action sets `VCS_TYPE` explicitly 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.
security-review-bot requested review from security-review-bot 2026-05-14 20:46:02 +00:00
security-review-bot requested changes 2026-05-14 20:46:02 +00:00
security-review-bot left a comment
Collaborator

Original review

Supersededsee 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

# Severity File Line Finding
1 [MAJOR] github/client.go 430 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.

Recommendation

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

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/131#pullrequestreview-3758) for up-to-date findings. <details><summary>Previous findings (commit 10ef451c)</summary> # 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 | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `github/client.go` | 430 | 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. | ## Recommendation **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* <!-- review-bot:security --> --- *Evaluated against 10ef451c* </details> <!-- review-bot:security -->
@@ -379,0 +427,4 @@
} `json:"user"`
State string `json:"state"`
}
Collaborator

[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.

**[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.
security-review-bot marked this conversation as resolved
rodin added the self-reviewed label 2026-05-14 20:46:05 +00:00
gpt-review-bot approved these changes 2026-05-14 20:46:17 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee 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

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 178 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.
2 [NIT] cmd/review-bot/vcs.go 14 ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code.
3 [NIT] cmd/review-bot/vcs.go 259 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.

Recommendation

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

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/131#pullrequestreview-3756) for up-to-date findings. <details><summary>Previous findings (commit 10ef451c)</summary> # 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 | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 178 | 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. | | 2 | [NIT] | `cmd/review-bot/vcs.go` | 14 | ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code. | | 3 | [NIT] | `cmd/review-bot/vcs.go` | 259 | 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. | ## Recommendation **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* <!-- review-bot:gpt --> --- *Evaluated against 10ef451c* </details> <!-- review-bot:gpt -->
@@ -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.
First-time contributor

[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.

**[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"
First-time contributor

[NIT] ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code.

**[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)
First-time contributor

[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.

**[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.
rodin added the wip label 2026-05-14 21:06:20 +00:00
Author
Owner

Fix Plan against 10ef451c20

Security Issue: Inconsistent HTTPS Enforcement

Current State:

  • doRequest includes scheme validation that rejects http:// unless c.allowInsecureHTTP is true
  • PostReview, DeleteReview, RequestReviewer all call c.httpClient.Do(req) directly, bypassing this guard
  • If baseURL is misconfigured as http://, these methods will send the Authorization: Bearer <token> header over plaintext

Approach: Introduce doRequestWithBody(ctx, method, url string, body []byte) ([]byte, error) in client.go that applies the same scheme check before executing. Refactor PostReview, DeleteReview, RequestReviewer to use it.

Changes

  1. Add doRequestWithBody in github/client.go:

    • Same scheme validation as doRequest (reject http:// unless c.allowInsecureHTTP)
    • Accepts optional body ([]byte; nil for bodyless requests like DELETE)
    • Sets Content-Type: application/json when body is non-nil
    • Sets Authorization: Bearer and Accept: application/vnd.github+json
    • Returns ([]byte, error) — consistent with doRequest/doGet
    • No retry logic (write methods are not idempotent; retry belongs to callers)
  2. Refactor in github/client.go (PostReview, DeleteReview, RequestReviewer):

    • Replace http.NewRequestWithContext + manual header set + c.httpClient.Do with c.doRequestWithBody call
    • Remove now-redundant local imports (bytes.NewReader)
  3. Update tests in github/client_test.go:

    • Add scheme validation tests for PostReview, DeleteReview, RequestReviewer with http:// base URL
    • Verify they return the same scheme error as doRequest

Scope

github/client.go and github/client_test.go only. No API surface changes. No impact on Gitea client.

Open Questions

  • No retry logic in doRequestWithBody — this matches the intent for write methods which should not be retried blindly. Correct approach given current usage.
  • doRequestWithBody does not take an accept override — callers always use application/vnd.github+json. We can add it later if needed.
## Fix Plan against 10ef451c20d6e4740b8d26b83c3fc8a0e0d03d97 ### Security Issue: Inconsistent HTTPS Enforcement **Current State:** - `doRequest` includes scheme validation that rejects `http://` unless `c.allowInsecureHTTP` is true - `PostReview`, `DeleteReview`, `RequestReviewer` all call `c.httpClient.Do(req)` directly, bypassing this guard - If `baseURL` is misconfigured as `http://`, these methods will send the `Authorization: Bearer <token>` header over plaintext **Approach:** Introduce `doRequestWithBody(ctx, method, url string, body []byte) ([]byte, error)` in `client.go` that applies the same scheme check before executing. Refactor PostReview, DeleteReview, RequestReviewer to use it. ### Changes 1. **Add `doRequestWithBody` in `github/client.go`:** - Same scheme validation as `doRequest` (reject `http://` unless `c.allowInsecureHTTP`) - Accepts optional body (`[]byte`; nil for bodyless requests like DELETE) - Sets `Content-Type: application/json` when body is non-nil - Sets `Authorization: Bearer` and `Accept: application/vnd.github+json` - Returns `([]byte, error)` — consistent with `doRequest`/`doGet` - No retry logic (write methods are not idempotent; retry belongs to callers) 2. **Refactor in `github/client.go`** (PostReview, DeleteReview, RequestReviewer): - Replace `http.NewRequestWithContext` + manual header set + `c.httpClient.Do` with `c.doRequestWithBody` call - Remove now-redundant local imports (`bytes.NewReader`) 3. **Update tests in `github/client_test.go`:** - Add scheme validation tests for PostReview, DeleteReview, RequestReviewer with `http://` base URL - Verify they return the same scheme error as `doRequest` ### Scope `github/client.go` and `github/client_test.go` only. No API surface changes. No impact on Gitea client. ### Open Questions - No retry logic in `doRequestWithBody` — this matches the intent for write methods which should not be retried blindly. Correct approach given current usage. - `doRequestWithBody` does not take an `accept` override — callers always use `application/vnd.github+json`. We can add it later if needed.
rodin added 1 commit 2026-05-14 21:11:20 +00:00
fix(#130): enforce HTTPS scheme consistently in GitHub client write methods
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 16s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m38s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m49s
d545abe392
PostReview, DeleteReview, and RequestReviewer were calling c.httpClient.Do
directly, bypassing the scheme check in doRequest that rejects http:// URLs
unless AllowInsecureHTTP is explicitly enabled.

Introduce doRequestWithBody(ctx, method, url, body) with the same HTTPS guard,
and refactor all three write methods to use it. This ensures tokens are never
sent over plaintext regardless of which API path is exercised.

Add scheme validation tests for each method.
rodin removed the wip label 2026-05-14 21:11:23 +00:00
sonnet-review-bot approved these changes 2026-05-14 21:12:18 +00:00
sonnet-review-bot left a comment
First-time contributor

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

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 175 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 than github.concur.com won't be detected. The more robust approach would be to rely on the explicit VCS_TYPE env var for all GHES instances, or detect GHES by checking for /api/v3 in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets VCS_TYPE explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.
2 [MINOR] cmd/review-bot/vcs.go 22 ErrNotSupported is declared but never used — the comment on giteaExtClient says "GitHub implementations return ErrNotSupported for those", but githubVCSAdapter never 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 (though go vet won't).
3 [MINOR] github/client.go 617 In GetAllFilesInPath, when ListContents returns a 404, the code falls back to GetFileContent. But ListContents on a file path returns the file as a single-object response (normalized to a []ContentEntry slice) — 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 directly is accurate for Gitea but not for this GitHub client where ListContents already handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.
4 [NIT] cmd/review-bot/vcs.go 87 vcsPullRequest uses an anonymous struct for Head rather than a named type. This is minor but means the struct literal syntax r.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.
5 [NIT] github/client_test.go 775 In TestGetFileContent_Base64WithNewlines, the encoded variable is declared and assigned but only used via _ = encoded (a suppress-unused comment). The actual test hardcodes the string in the body literal. 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.com domain is the most significant issue for portability and should be replaced with reliance on VCS_TYPE for GHES, (2) ErrNotSupported should either be used or removed, and (3) the dead-code 404-fallback path in GetAllFilesInPath should 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

# 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 | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 175 | 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 than `github.concur.com` won't be detected. The more robust approach would be to rely on the explicit `VCS_TYPE` env var for all GHES instances, or detect GHES by checking for `/api/v3` in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets `VCS_TYPE` explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments. | | 2 | [MINOR] | `cmd/review-bot/vcs.go` | 22 | `ErrNotSupported` is declared but never used — the comment on `giteaExtClient` says "GitHub implementations return ErrNotSupported for those", but `githubVCSAdapter` never 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 (though `go vet` won't). | | 3 | [MINOR] | `github/client.go` | 617 | In `GetAllFilesInPath`, when `ListContents` returns a 404, the code falls back to `GetFileContent`. But `ListContents` on a file path returns the file as a single-object response (normalized to a `[]ContentEntry` slice) — 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 directly` is accurate for Gitea but not for this GitHub client where `ListContents` already handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested. | | 4 | [NIT] | `cmd/review-bot/vcs.go` | 87 | `vcsPullRequest` uses an anonymous struct for `Head` rather than a named type. This is minor but means the struct literal syntax `r.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. | | 5 | [NIT] | `github/client_test.go` | 775 | In `TestGetFileContent_Base64WithNewlines`, the `encoded` variable is declared and assigned but only used via `_ = encoded` (a suppress-unused comment). The actual test hardcodes the string in the `body` literal. 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.com` domain is the most significant issue for portability and should be replaced with reliance on `VCS_TYPE` for GHES, (2) `ErrNotSupported` should either be used or removed, and (3) the dead-code 404-fallback path in `GetAllFilesInPath` should 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* <!-- review-bot: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 == "" {
First-time contributor

[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 than github.concur.com won't be detected. The more robust approach would be to rely on the explicit VCS_TYPE env var for all GHES instances, or detect GHES by checking for /api/v3 in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets VCS_TYPE explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.

**[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 than `github.concur.com` won't be detected. The more robust approach would be to rely on the explicit `VCS_TYPE` env var for all GHES instances, or detect GHES by checking for `/api/v3` in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets `VCS_TYPE` explicitly, 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
First-time contributor

[MINOR] ErrNotSupported is declared but never used — the comment on giteaExtClient says "GitHub implementations return ErrNotSupported for those", but githubVCSAdapter never 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 (though go vet won't).

**[MINOR]** `ErrNotSupported` is declared but never used — the comment on `giteaExtClient` says "GitHub implementations return ErrNotSupported for those", but `githubVCSAdapter` never 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 (though `go vet` won't).
@@ -0,0 +84,4 @@
// vcsReviewComment is an inline review comment.
type vcsReviewComment struct {
Path string
NewPosition int64 // Gitea: absolute line; GitHub: diff hunk position
First-time contributor

[NIT] vcsPullRequest uses an anonymous struct for Head rather than a named type. This is minor but means the struct literal syntax r.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.

**[NIT]** `vcsPullRequest` uses an anonymous struct for `Head` rather than a named type. This is minor but means the struct literal syntax `r.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
}
First-time contributor

[MINOR] In GetAllFilesInPath, when ListContents returns a 404, the code falls back to GetFileContent. But ListContents on a file path returns the file as a single-object response (normalized to a []ContentEntry slice) — 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 directly is accurate for Gitea but not for this GitHub client where ListContents already handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.

**[MINOR]** In `GetAllFilesInPath`, when `ListContents` returns a 404, the code falls back to `GetFileContent`. But `ListContents` on a file path returns the file as a single-object response (normalized to a `[]ContentEntry` slice) — 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 directly` is accurate for Gitea but not for this GitHub client where `ListContents` already 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))
First-time contributor

[NIT] In TestGetFileContent_Base64WithNewlines, the encoded variable is declared and assigned but only used via _ = encoded (a suppress-unused comment). The actual test hardcodes the string in the body literal. This is confusing — either use the variable to build the response body, or remove the variable entirely.

**[NIT]** In `TestGetFileContent_Base64WithNewlines`, the `encoded` variable is declared and assigned but only used via `_ = encoded` (a suppress-unused comment). The actual test hardcodes the string in the `body` literal. This is confusing — either use the variable to build the response body, or remove the variable entirely.
gpt-review-bot approved these changes 2026-05-14 21:13:13 +00:00
gpt-review-bot left a comment
First-time contributor

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 d545abe3

# 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* <!-- review-bot:gpt --> --- *Evaluated against d545abe3*
security-review-bot requested review from security-review-bot 2026-05-14 21:13:15 +00:00
security-review-bot approved these changes 2026-05-14 21:13:15 +00:00
security-review-bot left a comment
Collaborator

Security 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

# Severity File Line Finding
1 [MINOR] cmd/review-bot/main.go 692 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.

Recommendation

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

# Security 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 | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `cmd/review-bot/main.go` | 692 | 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. | ## Recommendation **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* <!-- review-bot:security --> --- *Evaluated against d545abe3*
@@ -654,6 +692,19 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin
return true, "all checks passed"
Collaborator

[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.

**[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.
rodin added ready and removed needs-review labels 2026-05-14 21:21:31 +00:00
aweiker was assigned by rodin 2026-05-14 21:21:34 +00:00
rodin removed the ready label 2026-05-14 21:36:50 +00:00
aweiker was unassigned by rodin 2026-05-14 21:36:50 +00:00
rodin self-assigned this 2026-05-14 21:36:50 +00:00
rodin added the wip label 2026-05-14 21:37:02 +00:00
rodin added ready and removed wip labels 2026-05-14 21:56:18 +00:00
rodin merged commit c53a07b230 into main 2026-05-14 22:06:21 +00:00
Sign in to join this conversation.
4 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#131