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 <[email protected]>
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 <[email protected]>
rodin
changed title from test to feat: implement GitHub API methods and VCS routing (issue #130)2026-05-14 20:44:26 +00:00
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.
~~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 -->
[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).
[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.
[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.
[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.
[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.
[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.
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
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.
~~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 -->
[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
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.
~~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 -->
[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.
[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
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
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)
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)
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.
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
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.
# 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*
[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.
[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).
[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.
[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.
[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
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.
# 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 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.
# 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*
[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.
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.
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 <[email protected]>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.