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
+6 -5
2 changed files with 6 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit af72c64b7f - Show all commits
github/client.go
+5 -4
View File
64
 @@ -159,6 +159,11 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
const maxErrorBodyBytes = 64 * 1024
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 22:22:13 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doRequest mutates the shared RetryBackoff slice based on Retry-After. Because Client is used concurrently, this shared-state mutation can cause a data race and persistently alter backoff behavior across goroutines. A malicious or misconfigured upstream could set very large Retry-After values, degrading performance for unrelated requests (potential DoS).

**[MINOR]** doRequest mutates the shared RetryBackoff slice based on Retry-After. Because Client is used concurrently, this shared-state mutation can cause a data race and persistently alter backoff behavior across goroutines. A malicious or misconfigured upstream could set very large Retry-After values, degrading performance for unrelated requests (potential DoS).
sonnet-review-bot commented 2026-05-12 23:26:36 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] After the timer fires, timer.Stop() is called redundantly. When <-timer.C succeeds, the timer has already expired and Stop() returns false with no effect. It's harmless but misleading — the pattern from the Go docs is to call Stop() only in the cancellation case. Consider removing the redundant timer.Stop() on the success branch, or document why it's there.

**[MINOR]** After the timer fires, `timer.Stop()` is called redundantly. When `<-timer.C` succeeds, the timer has already expired and `Stop()` returns false with no effect. It's harmless but misleading — the pattern from the Go docs is to call Stop() only in the cancellation case. Consider removing the redundant `timer.Stop()` on the success branch, or document why it's there.
// Reject non-HTTPS URLs early since the URL is immutable across retries.
gpt-review-bot commented 2026-05-12 23:51:13 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] Authorization header uses the "Bearer" scheme unconditionally (req.Header.Set("Authorization", "Bearer "+c.token)). GitHub REST v3 typically expects "token " for personal access tokens; using Bearer may cause 401s. Consider using "token" by default or making the scheme configurable.

**[MAJOR]** Authorization header uses the "Bearer" scheme unconditionally (req.Header.Set("Authorization", "Bearer "+c.token)). GitHub REST v3 typically expects "token <PAT>" for personal access tokens; using Bearer may cause 401s. Consider using "token" by default or making the scheme configurable.
if c.token != "" && !c.allowInsecureHTTP && !strings.HasPrefix(url, "https://") {
sonnet-review-bot commented 2026-05-12 22:17:41 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Retry-After handling mutates the backoff slice in-place: backoff[attempt] = time.Duration(seconds) * time.Second. When c.RetryBackoff is non-nil (e.g. in tests), this modifies the caller's slice, which is surprising and could cause test pollution if the same slice is reused. A local copy should be made before mutation, or the mutation should only apply to the local backoff variable (which it does when RetryBackoff is nil since a new slice is allocated, but not when it's non-nil).

**[MINOR]** The Retry-After handling mutates the `backoff` slice in-place: `backoff[attempt] = time.Duration(seconds) * time.Second`. When `c.RetryBackoff` is non-nil (e.g. in tests), this modifies the caller's slice, which is surprising and could cause test pollution if the same slice is reused. A local copy should be made before mutation, or the mutation should only apply to the local `backoff` variable (which it does when `RetryBackoff` is nil since a new slice is allocated, but not when it's non-nil).
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", url)
}
var lastErr error
for attempt := 0; attempt < maxAttempts; attempt++ {
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.
if attempt > 0 {
6
 @@ -183,10 +188,6 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
return nil, fmt.Errorf("create request: %w", err)
gpt-review-bot commented 2026-05-12 23:57:57 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Authorization header uses the "Bearer" scheme unconditionally. GitHub REST v3 commonly expects the "token" scheme for classic/fine-grained PATs, while "Bearer" is used in some flows (e.g., OAuth or app tokens). Consider supporting both schemes or making the scheme configurable to maximize compatibility.

**[MINOR]** Authorization header uses the "Bearer" scheme unconditionally. GitHub REST v3 commonly expects the "token" scheme for classic/fine-grained PATs, while "Bearer" is used in some flows (e.g., OAuth or app tokens). Consider supporting both schemes or making the scheme configurable to maximize compatibility.
}
if c.token != "" {
// Refuse to send credentials over plaintext unless explicitly allowed.
if !c.allowInsecureHTTP && req.URL.Scheme != "https" {
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", req.URL.Host)
}
req.Header.Set("Authorization", "Bearer "+c.token)
}
req.Header.Set("User-Agent", userAgent)
gpt-review-bot commented 2026-05-12 23:41:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Retry-After parsing only supports integer seconds. The HTTP spec allows an HTTP-date as well; consider parsing both forms to be more robust (fallback to date parsing when Atoi fails).

**[MINOR]** Retry-After parsing only supports integer seconds. The HTTP spec allows an HTTP-date as well; consider parsing both forms to be more robust (fallback to date parsing when Atoi fails).
18
github/files.go
+1 -1
View File
9
 @@ -42,7 +42,7 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
if err := json.Unmarshal(body, &entries); err != nil {
var single entry
if err2 := json.Unmarshal(body, &single); err2 != nil {
sonnet-review-bot commented 2026-05-12 23:40:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The fallback from array→object JSON parsing uses err from the outer json.Unmarshal in the error return (fmt.Errorf("parse contents JSON: %w", err)), but err here is the array-parse error ('cannot unmarshal object into Go value of type []entry'), which is misleading — the real parse failure is err2. Should return err2 instead: fmt.Errorf("parse contents JSON: %w", err2).

**[MINOR]** The fallback from array→object JSON parsing uses `err` from the outer `json.Unmarshal` in the error return (`fmt.Errorf("parse contents JSON: %w", err)`), but `err` here is the array-parse error ('cannot unmarshal object into Go value of type []entry'), which is misleading — the real parse failure is `err2`. Should return `err2` instead: `fmt.Errorf("parse contents JSON: %w", err2)`.
sonnet-review-bot commented 2026-05-12 23:49:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The fallback from array to object JSON parsing is an unusual approach. The error from the first json.Unmarshal (array attempt) is discarded silently. If the response is truly malformed JSON (not just an object vs array mismatch), only err2 from the single-entry unmarshal is surfaced. This is intentional per the comment but it means a corrupted array response that happens to partially parse as an object could go undetected. Consider checking if the body starts with [ vs { to choose the parsing path, which would be more explicit and preserve the original error on genuine parse failures.

**[MINOR]** The fallback from array to object JSON parsing is an unusual approach. The error from the first `json.Unmarshal` (array attempt) is discarded silently. If the response is truly malformed JSON (not just an object vs array mismatch), only `err2` from the single-entry unmarshal is surfaced. This is intentional per the comment but it means a corrupted array response that happens to partially parse as an object could go undetected. Consider checking if the body starts with `[` vs `{` to choose the parsing path, which would be more explicit and preserve the original error on genuine parse failures.
sonnet-review-bot commented 2026-05-13 00:14:07 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The fallback from array to single-object JSON unmarshal silently swallows the original array parse error. If the body is valid JSON but neither an array nor an object matching the entry struct (e.g., a JSON number), the second unmarshal will succeed with zero values and return a single empty entry. Consider checking that single.Name != "" or similar before accepting the fallback.

**[NIT]** The fallback from array to single-object JSON unmarshal silently swallows the original array parse error. If the body is valid JSON but neither an array nor an object matching the `entry` struct (e.g., a JSON number), the second unmarshal will succeed with zero values and return a single empty entry. Consider checking that `single.Name != ""` or similar before accepting the fallback.
return nil, fmt.Errorf("parse contents JSON: %w", err)
return nil, fmt.Errorf("parse contents JSON: %w", err2)
}
entries = []entry{single}
}
13