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
+52 -40
5 changed files with 52 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit ae91c8aef5 - Show all commits
github/client.go
+28 -19
View File
@@ -14,11 +14,13 @@ import (
"time" "time"
gpt-review-bot commented 2026-05-12 22:46:04 +00:00
Review
Copy Link
Copy Source

[NIT] Package comment mentions "review submission" but this PR doesn't include review endpoints. Consider updating the comment or adding a TODO/ref to avoid misleading users.

**[NIT]** Package comment mentions "review submission" but this PR doesn't include review endpoints. Consider updating the comment or adding a TODO/ref to avoid misleading users.
) )
const defaultBaseURL = "https://api.github.com" const (
const userAgent = "review-bot/1.0" defaultBaseURL = "https://api.github.com"
sonnet-review-bot commented 2026-05-12 23:01:09 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Three separate const declarations could be grouped into a single const (...) block per the style pattern, though this is a pure style nit and not a correctness issue.

**[NIT]** Three separate `const` declarations could be grouped into a single `const (...)` block per the style pattern, though this is a pure style nit and not a correctness issue.
userAgent = "review-bot/1.0"
// maxResponseBytes limits successful response body reads to 10 MiB. // maxResponseBytes limits successful response body reads to 10 MiB.
const maxResponseBytes = 10 * 1024 * 1024 maxResponseBytes = 10 * 1024 * 1024
)
// APIError represents an HTTP error response from the GitHub API. // APIError represents an HTTP error response from the GitHub API.
// It carries the status code so callers can distinguish between // It carries the status code so callers can distinguish between
14
 @@ -68,12 +70,12 @@ func asAPIError(err error) (*APIError, bool) {
type Client struct { type Client struct {
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.
baseURL string baseURL string
token string token 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.
http *http.Client httpClient *http.Client
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.
security-review-bot commented 2026-05-12 23:02:48 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] NewClient does not validate the baseURL scheme. If misconfigured to use http://, the token will be sent over plaintext. Consider enforcing https:// by default (or providing an explicit opt-out flag) to prevent accidental credential leakage.

**[MINOR]** NewClient does not validate the baseURL scheme. If misconfigured to use http://, the token will be sent over plaintext. Consider enforcing https:// by default (or providing an explicit opt-out flag) to prevent accidental credential leakage.
// RetryBackoff defines the delays between retry attempts for 429 responses. // retryBackoff defines the delays between retry attempts for 429 responses.
// RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
security-review-bot commented 2026-05-12 23:28:17 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] NewClient accepts arbitrary baseURL without validating scheme. If configured with an http:// URL, the Authorization token will be sent in plaintext over the network. Enforce HTTPS by default or require an explicit opt-in to allow HTTP for trusted internal deployments.

**[MINOR]** NewClient accepts arbitrary baseURL without validating scheme. If configured with an http:// URL, the Authorization token will be sent in plaintext over the network. Enforce HTTPS by default or require an explicit opt-in to allow HTTP for trusted internal deployments.
security-review-bot commented 2026-05-12 23:41:37 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] AllowInsecureHTTP() permits sending Authorization tokens over plaintext HTTP if enabled. While opt-in and documented for trusted environments/tests, it is a potential footgun if inadvertently enabled in production. Consider restricting to loopback/private ranges or emitting a prominent warning/telemetry when used.

**[MINOR]** AllowInsecureHTTP() permits sending Authorization tokens over plaintext HTTP if enabled. While opt-in and documented for trusted environments/tests, it is a potential footgun if inadvertently enabled in production. Consider restricting to loopback/private ranges or emitting a prominent warning/telemetry when used.
// If nil, defaults to {1s, 2s}. Set to shorter durations in tests. // If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff.
RetryBackoff []time.Duration retryBackoff []time.Duration
security-review-bot commented 2026-05-13 00:15:37 +00:00
Review
Copy Link
Copy Source

[MINOR] AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present).

**[MINOR]** AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present).
} }
// NewClient creates a new GitHub API client. // NewClient creates a new GitHub API client.
4
 @@ -86,16 +88,17 @@ func NewClient(token, baseURL string) *Client {
return &Client{ return &Client{
baseURL: strings.TrimRight(baseURL, "/"), baseURL: strings.TrimRight(baseURL, "/"),
security-review-bot commented 2026-05-12 23:14:27 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] NewClient accepts any baseURL without enforcing HTTPS or validating against a trusted allowlist. If a misconfiguration allows an attacker-controlled baseURL, the client could send the Authorization token to an untrusted host or over plaintext HTTP.

**[MINOR]** NewClient accepts any baseURL without enforcing HTTPS or validating against a trusted allowlist. If a misconfiguration allows an attacker-controlled baseURL, the client could send the Authorization token to an untrusted host or over plaintext HTTP.
token: token, token: token,
http: &http.Client{ httpClient: &http.Client{
Timeout: 30 * time.Second, Timeout: 30 * time.Second,
CheckRedirect: func(req *http.Request, via []*http.Request) error { CheckRedirect: func(req *http.Request, via []*http.Request) error {
// Prevent forwarding Authorization header to different hosts on redirect.
if len(via) > 0 && req.URL.Host != via[0].URL.Host {
req.Header.Del("Authorization")
}
if len(via) >= 10 { if len(via) >= 10 {
sonnet-review-bot commented 2026-05-12 23:56:34 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The SetHTTPClient and SetRetryBackoff methods are exported as test-injection points but documented as unsafe for concurrent use with requests. The convention in the patterns (style.md) suggests using unexported helpers or the functional options pattern for test injection. The current design works but exposes mutation methods on what is documented as a concurrent-safe type. A //nolint or explicit doc note would make the intent clearer.

**[NIT]** The `SetHTTPClient` and `SetRetryBackoff` methods are exported as test-injection points but documented as unsafe for concurrent use with requests. The convention in the patterns (style.md) suggests using unexported helpers or the functional options pattern for test injection. The current design works but exposes mutation methods on what is documented as a concurrent-safe type. A `//nolint` or explicit doc note would make the intent clearer.
return fmt.Errorf("stopped after 10 redirects") return fmt.Errorf("stopped after 10 redirects")
} }
// Strip Authorization on cross-host redirect or protocol downgrade (https→http).
prev := via[len(via)-1]
if req.URL.Host != prev.URL.Host || (prev.URL.Scheme == "https" && req.URL.Scheme == "http") {
security-review-bot commented 2026-05-12 23:14:27 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] CheckRedirect strips Authorization on cross-host or https→http redirects but still follows the redirect. Following cross-host redirects can be an SSRF vector in misconfigured environments; consider blocking cross-host redirects entirely rather than proceeding without Authorization.

**[MINOR]** CheckRedirect strips Authorization on cross-host or https→http redirects but still follows the redirect. Following cross-host redirects can be an SSRF vector in misconfigured environments; consider blocking cross-host redirects entirely rather than proceeding without Authorization.
req.Header.Del("Authorization")
}
gpt-review-bot commented 2026-05-12 23:03:22 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The parameter name 'url' in doRequest shadows the conceptual URL type used elsewhere (e.g., net/url in other files). Renaming to 'reqURL' could improve clarity, though this is purely stylistic.

**[NIT]** The parameter name 'url' in doRequest shadows the conceptual URL type used elsewhere (e.g., net/url in other files). Renaming to 'reqURL' could improve clarity, though this is purely stylistic.
return nil return nil
}, },
}, },
sonnet-review-bot commented 2026-05-12 23:40:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doc comment for Client says SetHTTPClient and SetRetryBackoff must not be called concurrently with requests, but these are public methods and there's no enforcement or noCopy guard. Given the concurrent-use note, a brief comment in SetHTTPClient and SetRetryBackoff reiterating the constraint would help (though this matches the stdlib's tls.Config immutable-after-use convention, so it's acceptable as-is).

**[NIT]** The doc comment for `Client` says `SetHTTPClient` and `SetRetryBackoff` must not be called concurrently with requests, but these are public methods and there's no enforcement or `noCopy` guard. Given the concurrent-use note, a brief comment in `SetHTTPClient` and `SetRetryBackoff` reiterating the constraint would help (though this matches the stdlib's `tls.Config` immutable-after-use convention, so it's acceptable as-is).
1
 @@ -105,7 +108,13 @@ func NewClient(token, baseURL string) *Client {
// SetHTTPClient sets the underlying HTTP client used for requests. // SetHTTPClient sets the underlying HTTP client used for requests.
// This is intended for testing to inject mock transports. // This is intended for testing to inject mock transports.
security-review-bot commented 2026-05-13 01:44:32 +00:00
Review
Copy Link
Copy Source

[MINOR] defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.

**[MINOR]** defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.
func (c *Client) SetHTTPClient(hc *http.Client) { func (c *Client) SetHTTPClient(hc *http.Client) {
gpt-review-bot commented 2026-05-12 22:46:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Consider setting a User-Agent header on all requests. GitHub recommends identifying clients, and some enterprise installations enforce it. Add req.Header.Set("User-Agent", "review-bot/1.0") or similar.

**[MINOR]** Consider setting a User-Agent header on all requests. GitHub recommends identifying clients, and some enterprise installations enforce it. Add req.Header.Set("User-Agent", "review-bot/1.0") or similar.
c.http = hc c.httpClient = hc
}
gpt-review-bot commented 2026-05-12 23:51:13 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] SetHTTPClient allows setting a nil *http.Client; subsequent use (c.httpClient.Do) would panic. Either guard against nil (return error or restore default client) or document that nil is invalid.

**[MINOR]** SetHTTPClient allows setting a nil *http.Client; subsequent use (c.httpClient.Do) would panic. Either guard against nil (return error or restore default client) or document that nil is invalid.
// SetRetryBackoff configures the retry backoff durations for testing.
// In production the default {1s, 2s} applies.
security-review-bot commented 2026-05-13 01:44:32 +00:00
Review
Copy Link
Copy Source

[MINOR] defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.

**[MINOR]** defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.
func (c *Client) SetRetryBackoff(d []time.Duration) {
gpt-review-bot commented 2026-05-12 22:21:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header is always set to "Bearer "+token even when token is empty. Consider only setting the header when token is non-empty to avoid sending an empty bearer token on unauthenticated requests.

**[MINOR]** Authorization header is always set to "Bearer "+token even when token is empty. Consider only setting the header when token is non-empty to avoid sending an empty bearer token on unauthenticated requests.
gpt-review-bot commented 2026-05-12 22:21:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] GitHub classic PATs typically use the "token" scheme while fine-grained tokens use "Bearer". If supporting both is desired, consider documenting or adapting the auth scheme based on token type.

**[NIT]** GitHub classic PATs typically use the "token" scheme while fine-grained tokens use "Bearer". If supporting both is desired, consider documenting or adapting the auth scheme based on token type.
sonnet-review-bot commented 2026-05-13 01:42:53 +00:00
Review
Copy Link
Copy Source

[MINOR] The doc comment on defaultCheckRedirect says it "strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP) to prevent credential leakage, while still following the redirect." However, a protocol downgrade from HTTPS to HTTP is a genuine security issue — stripping the header and still following is debatable. Consider returning an error on HTTPS→HTTP downgrade rather than silently following. This is a design choice that has security implications, not a bug per se, but worth flagging.

**[MINOR]** The doc comment on `defaultCheckRedirect` says it "strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP) to prevent credential leakage, while still following the redirect." However, a protocol downgrade from HTTPS to HTTP is a genuine security issue — stripping the header and still following is debatable. Consider returning an error on HTTPS→HTTP downgrade rather than silently following. This is a design choice that has security implications, not a bug per se, but worth flagging.
gpt-review-bot commented 2026-05-13 02:31:14 +00:00
Review
Copy Link
Copy Source

[MINOR] defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse.

**[MINOR]** defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse.
c.retryBackoff = d
} }
sonnet-review-bot commented 2026-05-13 01:17:39 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The concurrency safety doc comment says "SetHTTPClient and SetRetryBackoff must not be called concurrently with requests" but these are exported methods with no enforcement (no mutex, no atomic). This is an acceptable design for a test-support method, but the comment could be stronger: e.g., "These methods are intended for test setup only and must be called before any goroutines issue requests."

**[NIT]** The concurrency safety doc comment says "SetHTTPClient and SetRetryBackoff must not be called concurrently with requests" but these are exported methods with no enforcement (no mutex, no atomic). This is an acceptable design for a test-support method, but the comment could be stronger: e.g., "These methods are intended for test setup only and must be called before any goroutines issue requests."
// doRequest performs an HTTP request with retry on 429 rate limit responses. // doRequest performs an HTTP request with retry on 429 rate limit responses.
2
 @@ -116,9 +125,9 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
const maxRetryAfter = 120 * time.Second const maxRetryAfter = 120 * time.Second
gpt-review-bot commented 2026-05-12 23:57:57 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] SetHTTPClient(nil) restores a basic http.Client with only Timeout set and drops the custom CheckRedirect policy configured in NewClient. This diverges from the initial default behavior and could unintentionally allow auth headers on redirects. Either preserve the CheckRedirect behavior when resetting or adjust the comment to clarify the change.

**[MINOR]** SetHTTPClient(nil) restores a basic http.Client with only Timeout set and drops the custom CheckRedirect policy configured in NewClient. This diverges from the initial default behavior and could unintentionally allow auth headers on redirects. Either preserve the CheckRedirect behavior when resetting or adjust the comment to clarify the change.
security-review-bot commented 2026-05-13 01:18:57 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] SetHTTPClient accepts an arbitrary *http.Client and does not enforce the safe redirect policy. If a caller supplies a client without a CheckRedirect that strips Authorization on cross-host or downgrade redirects, credentials could leak during redirects.

**[MINOR]** SetHTTPClient accepts an arbitrary *http.Client and does not enforce the safe redirect policy. If a caller supplies a client without a CheckRedirect that strips Authorization on cross-host or downgrade redirects, credentials could leak during redirects.
var backoff []time.Duration var backoff []time.Duration
if c.RetryBackoff != nil { if c.retryBackoff != nil {
backoff = make([]time.Duration, len(c.RetryBackoff)) backoff = make([]time.Duration, len(c.retryBackoff))
copy(backoff, c.RetryBackoff) copy(backoff, c.retryBackoff)
sonnet-review-bot commented 2026-05-12 22:19:53 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Retry-After override mutates the backoff slice in-place. When c.RetryBackoff is non-nil (set by tests or callers), backoff is a reference to the same slice. This means a Retry-After header on attempt 0 will permanently modify c.RetryBackoff[0] on the caller's struct, which is surprising shared-state mutation. Either copy the slice at the start of doRequest, or use a local variable for the overridden delay rather than mutating the slice.

**[MINOR]** The Retry-After override mutates the `backoff` slice in-place. When `c.RetryBackoff` is non-nil (set by tests or callers), `backoff` is a reference to the same slice. This means a Retry-After header on attempt 0 will permanently modify `c.RetryBackoff[0]` on the caller's struct, which is surprising shared-state mutation. Either copy the slice at the start of doRequest, or use a local variable for the overridden delay rather than mutating the slice.
security-review-bot commented 2026-05-12 22:46:23 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Successful responses are read with io.ReadAll without a size limit. If the configured baseURL responds with a very large body, this could lead to memory exhaustion (DoS). Consider bounding successful response sizes or streaming with explicit limits.

**[MINOR]** Successful responses are read with io.ReadAll without a size limit. If the configured baseURL responds with a very large body, this could lead to memory exhaustion (DoS). Consider bounding successful response sizes or streaming with explicit limits.
} else { } else {
gpt-review-bot commented 2026-05-12 23:41:47 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doRequest parameter is named 'url', which can be confusing since 'url' is also a common package import name. Consider renaming to 'reqURL' for clarity and to avoid future shadowing if net/url is imported.

**[NIT]** The doRequest parameter is named 'url', which can be confusing since 'url' is also a common package import name. Consider renaming to 'reqURL' for clarity and to avoid future shadowing if net/url is imported.
backoff = []time.Duration{1 * time.Second, 2 * time.Second} backoff = []time.Duration{1 * time.Second, 2 * time.Second}
} }
gpt-review-bot commented 2026-05-12 23:27:58 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Calling timer.Stop() after the timer has already fired is unnecessary. You could simplify the delay logic with select { case <-time.After(delay): case <-ctx.Done(): } or only Stop in the ctx.Done branch.

**[NIT]** Calling timer.Stop() after the timer has already fired is unnecessary. You could simplify the delay logic with select { case <-time.After(delay): case <-ctx.Done(): } or only Stop in the ctx.Done branch.
17
 @@ -157,7 +166,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
req.Header.Set("Accept", "application/vnd.github+json") req.Header.Set("Accept", "application/vnd.github+json")
} }
sonnet-review-bot commented 2026-05-12 23:40:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] After a successful response is read, resp.Body.Close() is called directly after io.ReadAll. If io.ReadAll returns an error (e.g. partial read), the body is still closed via the subsequent line, which is fine. However, the pattern is slightly inconsistent with the error path below it — consider using defer resp.Body.Close() paired with a drain before close on the error path for symmetry. This is purely stylistic; the current approach is correct.

**[MINOR]** After a successful response is read, `resp.Body.Close()` is called directly after `io.ReadAll`. If `io.ReadAll` returns an error (e.g. partial read), the body is still closed via the subsequent line, which is fine. However, the pattern is slightly inconsistent with the error path below it — consider using `defer resp.Body.Close()` paired with a drain before close on the error path for symmetry. This is purely stylistic; the current approach is correct.
resp, err := c.http.Do(req) resp, err := c.httpClient.Do(req)
if err != nil { if err != nil {
gpt-review-bot commented 2026-05-12 23:41:47 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Authorization header uses the 'Bearer' scheme. GitHub PATs typically use the 'token' scheme; while 'Bearer' may work for some token types, consider aligning with GitHub’s documented scheme or making it configurable.

**[NIT]** Authorization header uses the 'Bearer' scheme. GitHub PATs typically use the 'token' scheme; while 'Bearer' may work for some token types, consider aligning with GitHub’s documented scheme or making it configurable.
return nil, fmt.Errorf("do request: %w", err) return nil, fmt.Errorf("do request: %w", err)
} }
12
github/client_test.go
+10 -10
View File
@@ -81,7 +81,7 @@ func TestDoRequest_429Retry(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{10 * time.Millisecond, 10 * time.Millisecond} c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
body, err := c.doGet(context.Background(), srv.URL+"/test") body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil { if err != nil {
@@ -106,7 +106,7 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
_, err := c.doGet(context.Background(), srv.URL+"/test") _, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil { if err == nil {
3
 @@ -205,7 +205,7 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
// Use short backoff; Retry-After should override // Use short backoff; Retry-After should override
c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
start := time.Now() start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test") body, err := c.doGet(context.Background(), srv.URL+"/test")
1
 @@ -246,19 +246,19 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
_, err := c.doGet(context.Background(), srv.URL+"/test") _, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil { if err != nil {
t.Fatalf("unexpected error: %v", err) t.Fatalf("unexpected error: %v", err)
} }
// Verify the original RetryBackoff slice was not mutated // Verify the original retryBackoff slice was not mutated
if c.RetryBackoff[0] != 1*time.Millisecond { if c.retryBackoff[0] != 1*time.Millisecond {
t.Errorf("RetryBackoff[0] was mutated: got %v, want 1ms", c.RetryBackoff[0]) t.Errorf("retryBackoff[0] was mutated: got %v, want 1ms", c.retryBackoff[0])
} }
if c.RetryBackoff[1] != 1*time.Millisecond { if c.retryBackoff[1] != 1*time.Millisecond {
t.Errorf("RetryBackoff[1] was mutated: got %v, want 1ms", c.RetryBackoff[1]) t.Errorf("retryBackoff[1] was mutated: got %v, want 1ms", c.retryBackoff[1])
} }
} }
2
 @@ -310,7 +310,7 @@ func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) { func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
// Verify the CheckRedirect function is configured // Verify the CheckRedirect function is configured
c := NewClient("secret-token", "https://api.github.com") c := NewClient("secret-token", "https://api.github.com")
if c.http.CheckRedirect == nil { if c.httpClient.CheckRedirect == nil {
t.Fatal("expected CheckRedirect to be set") t.Fatal("expected CheckRedirect to be set")
} }
} }
github/files_test.go
+2 -2
View File
@@ -109,7 +109,7 @@ func TestGetFileContent_429Retry(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
if err != nil { if err != nil {
@@ -227,7 +227,7 @@ func TestListContents_429Retry(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
entries, err := c.ListContents(context.Background(), "owner", "repo", ".") entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
if err != nil { if err != nil {
github/pr.go
+10 -7
View File
5
 @@ -84,13 +84,16 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
return string(body), nil return string(body), nil
} }
security-review-bot commented 2026-05-12 23:02:48 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] GetPullRequestFiles paginates until a page returns fewer than 100 items. If a misbehaving or malicious server keeps returning 100 items indefinitely, this could cause unbounded requests (potential DoS). Consider honoring Link headers, total counts, or imposing a reasonable max pages limit.

**[MINOR]** GetPullRequestFiles paginates until a page returns fewer than 100 items. If a misbehaving or malicious server keeps returning 100 items indefinitely, this could cause unbounded requests (potential DoS). Consider honoring Link headers, total counts, or imposing a reasonable max pages limit.
// maxPages is the upper bound on pagination loops to prevent unbounded iteration
// in case the server returns a full page indefinitely.
sonnet-review-bot commented 2026-05-12 23:12:58 +00:00
Review
Copy Link
Copy Source

[MINOR] The maxPages = 100 constant caps pagination at 100 pages × 100 files = 10,000 files for PRs, and 100 pages × 100 check runs = 10,000 check runs. This constant is shared between two very different concerns (PR files and check runs). A PR with 10,000 files is pathological but possible in generated-code repos; silently truncating without returning an error or warning could cause incorrect reviews. Consider either documenting this limit explicitly or returning an error when the cap is hit.

**[MINOR]** The `maxPages = 100` constant caps pagination at 100 pages × 100 files = 10,000 files for PRs, and 100 pages × 100 check runs = 10,000 check runs. This constant is shared between two very different concerns (PR files and check runs). A PR with 10,000 files is pathological but possible in generated-code repos; silently truncating without returning an error or warning could cause incorrect reviews. Consider either documenting this limit explicitly or returning an error when the cap is hit.
const maxPages = 100
// GetPullRequestFiles fetches the list of files changed in a PR. // GetPullRequestFiles fetches the list of files changed in a PR.
// Paginates through all pages (100 per page) to collect all files. // Paginates through all pages (100 per page) to collect all files.
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) { func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
var allFiles []vcs.ChangedFile var allFiles []vcs.ChangedFile
page := 1
for { for page := 1; page <= maxPages; page++ {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d", reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page) c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
body, err := c.doGet(ctx, reqURL) body, err := c.doGet(ctx, reqURL)
3
 @@ -114,7 +117,6 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
if len(files) < 100 { if len(files) < 100 {
break break
} }
page++
} }
return allFiles, nil return allFiles, nil
10
 @@ -175,8 +177,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
} }
// Fetch check runs (paginated) // Fetch check runs (paginated)
checkPage := 1 for checkPage := 1; checkPage <= maxPages; checkPage++ {
for {
checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d", checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage) c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage)
checkBody, err := c.doGet(ctx, checkURL) checkBody, err := c.doGet(ctx, checkURL)
7
 @@ -198,13 +199,15 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
if len(checkResp.CheckRuns) < 100 { if len(checkResp.CheckRuns) < 100 {
break break
} }
checkPage++
} }
sonnet-review-bot commented 2026-05-12 23:12:58 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The mapCheckRunStatus function maps cancelled, skipped, and neutral to success (non-blocking). This is a meaningful policy decision that warrants a more explicit comment explaining why these are treated as non-failures from the review bot's perspective, rather than just // non-blocking. The current comment doesn't explain the reasoning to future maintainers.

**[MINOR]** The `mapCheckRunStatus` function maps `cancelled`, `skipped`, and `neutral` to `success` (non-blocking). This is a meaningful policy decision that warrants a more explicit comment explaining *why* these are treated as non-failures from the review bot's perspective, rather than just `// non-blocking`. The current comment doesn't explain the reasoning to future maintainers.
return result, nil return result, nil
} }
sonnet-review-bot commented 2026-05-13 01:42:53 +00:00
Review
Copy Link
Copy Source

[NIT] In GetCommitStatuses, the check run pagination loop uses checkPage as the variable name while the PR files loop uses page. Naming is internally consistent within each function, but the inconsistency between the two sibling pagination loops is a minor style nit.

**[NIT]** In `GetCommitStatuses`, the check run pagination loop uses `checkPage` as the variable name while the PR files loop uses `page`. Naming is internally consistent within each function, but the inconsistency between the two sibling pagination loops is a minor style nit.
// mapCheckRunStatus maps a check run conclusion+status to a vcs.CommitStatus status string. // mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
sonnet-review-bot commented 2026-05-12 22:19:53 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] mapCheckRunStatus has a dead code path: case "in_progress", "queued" will never be reached because conclusion == nil is checked first (returning "pending"), and when conclusion is non-nil it cannot be "in_progress" or "queued" (those are status values, not conclusion values in the GitHub API). This case is unreachable and misleading.

**[MINOR]** mapCheckRunStatus has a dead code path: `case "in_progress", "queued"` will never be reached because `conclusion == nil` is checked first (returning "pending"), and when conclusion is non-nil it cannot be "in_progress" or "queued" (those are status values, not conclusion values in the GitHub API). This case is unreachable and misleading.
// The second parameter (check run status field, e.g. "completed", "in_progress") is
// unused because conclusion alone determines the mapped state: nil conclusion means
// the run is still in progress (pending), regardless of the status field value.
sonnet-review-bot commented 2026-05-12 22:17:41 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In mapCheckRunStatus, the cases "in_progress" and "queued" are in the switch *conclusion block, but conclusion is non-nil when this switch is reached. GitHub's API sets conclusion to null for in-progress/queued check runs, not to the strings "in_progress" or "queued" — those are status field values. So these cases are dead code and will never be reached. The nil-conclusion check at the top already handles the pending case correctly.

**[MINOR]** In `mapCheckRunStatus`, the cases `"in_progress"` and `"queued"` are in the `switch *conclusion` block, but `conclusion` is non-nil when this switch is reached. GitHub's API sets `conclusion` to null for in-progress/queued check runs, not to the strings `"in_progress"` or `"queued"` — those are `status` field values. So these cases are dead code and will never be reached. The nil-conclusion check at the top already handles the pending case correctly.
func mapCheckRunStatus(conclusion *string, _ string) string { func mapCheckRunStatus(conclusion *string, _ string) string {
if conclusion == nil { if conclusion == nil {
// Still running or queued // Still running or queued
github/pr_test.go
+2 -2
View File
@@ -112,7 +112,7 @@ func TestGetPullRequest_429Retry(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1) pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
if err != nil { if err != nil {
2
 @@ -447,7 +447,7 @@ func TestGetFileContentAtRef_429Retry(t *testing.T) {
c := NewClient("token", srv.URL) c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond} c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main") content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
if err != nil { if err != nil {
7