rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(github): implement PRReader + FileReader client (#80) #93

Closed
rodin wants to merge 16 commits from review-bot-issue-80 into feature/github-support
pull from: review-bot-issue-80
merge into: rodin:feature/github-support
Conversation 78 Commits 16 Files Changed 8
+5 -1
2 changed files with 5 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c10bb72117 - Show all commits
github/client.go
+3 -1
View File
13
 @@ -66,7 +66,8 @@ func asAPIError(err error) (*APIError, bool) {
}
security-review-bot commented 2026-05-12 22:46:23 +00:00
Review
Copy Link
Copy Source

[NIT] baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled.

**[NIT]** baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled.
// Client interacts with the GitHub API.
sonnet-review-bot commented 2026-05-12 22:19:53 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] RetryBackoff is an exported field on Client, which exposes an internal implementation detail and creates a testing surface that's somewhat awkward — callers might accidentally rely on mutation semantics. A more idiomatic approach would be a functional option (e.g. WithRetryBackoff) or keeping it unexported with a test helper. That said, the comment explicitly marks it as a test hook, which mitigates this concern.

**[MINOR]** RetryBackoff is an exported field on Client, which exposes an internal implementation detail and creates a testing surface that's somewhat awkward — callers might accidentally rely on mutation semantics. A more idiomatic approach would be a functional option (e.g. `WithRetryBackoff`) or keeping it unexported with a test helper. That said, the comment explicitly marks it as a test hook, which mitigates this concern.
sonnet-review-bot commented 2026-05-12 23:01:09 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] RetryBackoff is an exported field on Client, which makes it part of the public API surface. This is used as a testing escape hatch ('Set to shorter durations in tests'), but the pattern documented in the codebase for HTTP client injection is SetHTTPClient(). Exposing RetryBackoff publicly means callers can set arbitrary production retry delays, which may be intentional but conflates test and production configuration. A SetRetryBackoff method or keeping it package-internal with a test-only setter via export_test.go would be cleaner, though the current approach is pragmatic.

**[MINOR]** RetryBackoff is an exported field on Client, which makes it part of the public API surface. This is used as a testing escape hatch ('Set to shorter durations in tests'), but the pattern documented in the codebase for HTTP client injection is SetHTTPClient(). Exposing RetryBackoff publicly means callers can set arbitrary production retry delays, which may be intentional but conflates test and production configuration. A SetRetryBackoff method or keeping it package-internal with a test-only setter via export_test.go would be cleaner, though the current approach is pragmatic.
// A Client is safe for concurrent use by multiple goroutines.
// A Client is safe for concurrent use by multiple goroutines;
// however, SetHTTPClient and SetRetryBackoff must not be called concurrently with requests.
security-review-bot commented 2026-05-12 22:46:23 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The default http.Client will follow redirects. If a redirect points to a different host, there is a risk (dependent on Go version/runtime behavior) of Authorization headers being forwarded, potentially leaking tokens. Consider setting CheckRedirect to disallow cross-host redirects or disable redirects (return http.ErrUseLastResponse), and/or ensure Authorization is stripped on host changes.

**[MINOR]** The default http.Client will follow redirects. If a redirect points to a different host, there is a risk (dependent on Go version/runtime behavior) of Authorization headers being forwarded, potentially leaking tokens. Consider setting CheckRedirect to disallow cross-host redirects or disable redirects (return http.ErrUseLastResponse), and/or ensure Authorization is stripped on host changes.
sonnet-review-bot commented 2026-05-12 23:12:58 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Client struct has inconsistent field alignment — httpClient is not aligned with baseURL and token. While gofmt doesn't enforce struct field alignment, the blank line between token and httpClient with the comment block is slightly inconsistent with Go conventions. More substantively: SetHTTPClient and SetRetryBackoff are exported methods that mutate the struct, breaking the documented concurrency guarantee ('A Client is safe for concurrent use by multiple goroutines'). These mutation methods are clearly test-only helpers, but they're exported without any documentation caveat about not calling them concurrently with requests.

**[MINOR]** The Client struct has inconsistent field alignment — `httpClient` is not aligned with `baseURL` and `token`. While gofmt doesn't enforce struct field alignment, the blank line between `token` and `httpClient` with the comment block is slightly inconsistent with Go conventions. More substantively: `SetHTTPClient` and `SetRetryBackoff` are exported methods that mutate the struct, breaking the documented concurrency guarantee ('A Client is safe for concurrent use by multiple goroutines'). These mutation methods are clearly test-only helpers, but they're exported without any documentation caveat about not calling them concurrently with requests.
type Client struct {
baseURL string
gpt-review-bot commented 2026-05-12 23:41:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Functional options are used for a single boolean (AllowInsecureHTTP). Per configuration patterns, prefer a simpler parameter or a nil-opts struct when there are <3 stable options; consider YAGNI until more options appear.

**[MINOR]** Functional options are used for a single boolean (AllowInsecureHTTP). Per configuration patterns, prefer a simpler parameter or a nil-opts struct when there are <3 stable options; consider YAGNI until more options appear.
token string
sonnet-review-bot commented 2026-05-12 23:26:36 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The struct formatting has inconsistent spacing — baseURL and token fields have no space before the type, but httpClient has extra whitespace alignment. gofmt normally handles this, so it may be a display artifact, but worth verifying the file passes gofmt cleanly.

**[NIT]** The struct formatting has inconsistent spacing — `baseURL` and `token` fields have no space before the type, but `httpClient` has extra whitespace alignment. `gofmt` normally handles this, so it may be a display artifact, but worth verifying the file passes `gofmt` cleanly.
35
 @@ -145,6 +146,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
timer := time.NewTimer(delay)
gpt-review-bot commented 2026-05-12 23:27:58 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Authorization scheme is hardcoded to "Bearer". While modern GitHub tokens support this, some environments still use the "token" scheme. Consider documenting the expected token type or allowing the scheme to be configurable if needed.

**[NIT]** Authorization scheme is hardcoded to "Bearer". While modern GitHub tokens support this, some environments still use the "token" scheme. Consider documenting the expected token type or allowing the scheme to be configurable if needed.
select {
case <-timer.C:
gpt-review-bot commented 2026-05-12 23:14:40 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header always uses the "Bearer" scheme. Some GitHub token types (classic PAT) historically use the "token" scheme. Consider making the auth scheme configurable or auto-detectable to maximize compatibility.

**[MINOR]** Authorization header always uses the "Bearer" scheme. Some GitHub token types (classic PAT) historically use the "token" scheme. Consider making the auth scheme configurable or auto-detectable to maximize compatibility.
timer.Stop()
security-review-bot commented 2026-05-12 23:28:17 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header is sent to whatever baseURL is configured. If baseURL can be influenced by untrusted input, this could leak tokens to an attacker-controlled host (SSRF/token exfiltration). Ensure baseURL is treated as trusted configuration and consider allowlisting expected hosts at a higher layer.

**[MINOR]** Authorization header is sent to whatever baseURL is configured. If baseURL can be influenced by untrusted input, this could leak tokens to an attacker-controlled host (SSRF/token exfiltration). Ensure baseURL is treated as trusted configuration and consider allowlisting expected hosts at a higher layer.
case <-ctx.Done():
sonnet-review-bot commented 2026-05-13 00:14:07 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The CheckRedirect lambda is duplicated verbatim in both NewClient and SetHTTPClient(nil). Extract it to a package-level function (e.g., defaultCheckRedirect) to eliminate the duplication and ensure both code paths stay in sync when the policy changes.

**[MINOR]** The CheckRedirect lambda is duplicated verbatim in both NewClient and SetHTTPClient(nil). Extract it to a package-level function (e.g., `defaultCheckRedirect`) to eliminate the duplication and ensure both code paths stay in sync when the policy changes.
timer.Stop()
gpt-review-bot commented 2026-05-12 22:21:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Retry-After parsing only handles delta-seconds via Atoi. RFC 7231 allows an HTTP-date format; optionally support parsing HTTP-date to fully respect server guidance.

**[NIT]** Retry-After parsing only handles delta-seconds via Atoi. RFC 7231 allows an HTTP-date format; optionally support parsing HTTP-date to fully respect server guidance.
return nil, ctx.Err()
23
github/pr.go
+2
View File
31
 @@ -220,6 +220,8 @@ func mapCheckRunStatus(conclusion *string, _ string) string {
return "failure"
case "cancelled", "skipped", "neutral":
return "success" // non-blocking
case "stale", "waiting":
return "pending"
default:
return "pending"
}