feat(github): implement GitHub API client foundation #101

Merged
aweiker merged 3 commits from issue-80-a-client into feature/github-support 2026-05-13 04:46:46 +00:00
Owner

Closes #98. Part of #80.

Implement the base Client struct, NewClient with configurable base URL/GHE support, HTTP helpers with 429 retry and Retry-After handling. Also adds Patch field to vcs.ChangedFile.

Files: vcs/types.go, github/client.go, github/client_test.go

Part 1 of 3 — see #99 (PRReader) and #100 (FileReader) for follow-up work.

Closes #98. Part of #80. Implement the base `Client` struct, `NewClient` with configurable base URL/GHE support, HTTP helpers with 429 retry and Retry-After handling. Also adds `Patch` field to `vcs.ChangedFile`. Files: `vcs/types.go`, `github/client.go`, `github/client_test.go` Part 1 of 3 — see #99 (PRReader) and #100 (FileReader) for follow-up work.
rodin added 1 commit 2026-05-13 04:12:42 +00:00
feat(github): implement GitHub API client foundation (#80)
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 41s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m7s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m11s
0e7e12a99c
Add GitHub API client with configurable base URL and GHE support,
HTTP helpers with 429 retry and Retry-After handling.
Also adds Patch field to vcs.ChangedFile.

Part 1 of 3 for #80.
sonnet-review-bot approved these changes 2026-05-13 04:13:41 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 0e7e12a9)

Sonnet Review

Summary

Well-implemented GitHub API client foundation with thorough security considerations (auth header stripping on cross-host redirects, HTTPS enforcement, log injection prevention) and comprehensive test coverage. The code is idiomatic Go and follows the documented patterns correctly. A few minor issues worth noting but none blocking.

Findings

# Severity File Line Finding
1 [MINOR] github/client.go 251 After handleResponse returns done=false (the 429 case that will be retried), resp is already closed by handleResponse's defer. The subsequent resp.StatusCode == http.StatusTooManyRequests check on line 258 reads from a closed response, but this is safe because resp is a struct value (not a stream) and StatusCode was already populated. However, the design where handleResponse owns the response lifecycle (via defer) but the caller still needs to inspect resp.StatusCode afterward is a subtle coupling. The caller reads resp.StatusCode on line 258 after handleResponse has already closed the body — this works but is fragile. Consider returning the status code from handleResponse or restructuring to make the ownership clearer.
2 [MINOR] github/client.go 258 The retry condition attempt < maxAttempts-1 is evaluated after handleResponse returns done=false. This means on the final attempt (attempt==2), a 429 response falls through to return nil, lastErr correctly. However, the continue on a 429 means the loop increments attempt and hits the backoff sleep at the top — but backoff has only 2 entries for 3 attempts. The indexing backoff[attempt-1] for attempt=1 gives backoff[0] and for attempt=2 gives backoff[1], which is correct. But the guard attempt < maxAttempts-1 (i.e., attempt < 2) means we only continue for attempts 0 and 1, never attempt 2. On attempt 2 a 429 hits return nil, lastErr. This is correct behavior but the interaction between the backoff slice length and maxAttempts is implicit and could silently break if maxAttempts is changed without updating the default backoff.
3 [NIT] github/client_test.go 179 The test TestDoRequest_RetryAfterDoesNotMutateBackoff skips in short mode with t.Skip("skipping slow retry test in short mode") but the test sets a Retry-After: 1 header which causes a ~1 second delay. This is intentional for the actual retry test, but the 'does not mutate' property could be verified without the real delay by using RetryAfter: 0 or a past date. Minor since the skip guard is appropriate.
4 [NIT] github/client.go 230 The timer pattern uses timer.Stop() after <-timer.C with a comment 'no-op after fire; kept for symmetry'. The standard Go idiom from the docs recommends this for correctness in case of concurrent resets, but for a non-reset timer this is indeed a no-op. The comment explains the intent — this is fine as written.
5 [NIT] github/client_test.go 195 TestDoRequest_RetryAfterDoesNotMutateBackoff accesses c.retryBackoff directly (a private field) from within the github package test — this works because the test is in package github (white-box test). This is intentional and correct given the test file is in the same package.

Recommendation

APPROVE — Approve. The implementation is high quality — security-conscious redirect handling, clean separation of concerns, idiomatic error types with errors.As, proper copy of the backoff slice to prevent mutation, and thorough test coverage including edge cases (past HTTP-dates, exact-at-limit body sizes, empty tokens). The minor issues identified are design observations rather than bugs: the resp lifetime coupling between handleResponse and the caller's status check is the most noteworthy, but it works correctly as written. The conventions around testing.Short() for slow tests are followed. CI passed. Ready to merge.


Review by sonnet


Evaluated against 0e7e12a9

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2972) for up-to-date findings. <details><summary>Previous findings (commit 0e7e12a9)</summary> # Sonnet Review ## Summary Well-implemented GitHub API client foundation with thorough security considerations (auth header stripping on cross-host redirects, HTTPS enforcement, log injection prevention) and comprehensive test coverage. The code is idiomatic Go and follows the documented patterns correctly. A few minor issues worth noting but none blocking. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `github/client.go` | 251 | After `handleResponse` returns `done=false` (the 429 case that will be retried), `resp` is already closed by `handleResponse`'s defer. The subsequent `resp.StatusCode == http.StatusTooManyRequests` check on line 258 reads from a closed response, but this is safe because `resp` is a struct value (not a stream) and `StatusCode` was already populated. However, the design where `handleResponse` owns the response lifecycle (via defer) but the caller still needs to inspect `resp.StatusCode` afterward is a subtle coupling. The caller reads `resp.StatusCode` on line 258 after `handleResponse` has already closed the body — this works but is fragile. Consider returning the status code from `handleResponse` or restructuring to make the ownership clearer. | | 2 | [MINOR] | `github/client.go` | 258 | The retry condition `attempt < maxAttempts-1` is evaluated after `handleResponse` returns `done=false`. This means on the final attempt (attempt==2), a 429 response falls through to `return nil, lastErr` correctly. However, the `continue` on a 429 means the loop increments `attempt` and hits the backoff sleep at the top — but `backoff` has only 2 entries for 3 attempts. The indexing `backoff[attempt-1]` for attempt=1 gives backoff[0] and for attempt=2 gives backoff[1], which is correct. But the guard `attempt < maxAttempts-1` (i.e., attempt < 2) means we only continue for attempts 0 and 1, never attempt 2. On attempt 2 a 429 hits `return nil, lastErr`. This is correct behavior but the interaction between the backoff slice length and maxAttempts is implicit and could silently break if maxAttempts is changed without updating the default backoff. | | 3 | [NIT] | `github/client_test.go` | 179 | The test `TestDoRequest_RetryAfterDoesNotMutateBackoff` skips in short mode with `t.Skip("skipping slow retry test in short mode")` but the test sets a `Retry-After: 1` header which causes a ~1 second delay. This is intentional for the actual retry test, but the 'does not mutate' property could be verified without the real delay by using `RetryAfter: 0` or a past date. Minor since the skip guard is appropriate. | | 4 | [NIT] | `github/client.go` | 230 | The timer pattern uses `timer.Stop()` after `<-timer.C` with a comment 'no-op after fire; kept for symmetry'. The standard Go idiom from the docs recommends this for correctness in case of concurrent resets, but for a non-reset timer this is indeed a no-op. The comment explains the intent — this is fine as written. | | 5 | [NIT] | `github/client_test.go` | 195 | `TestDoRequest_RetryAfterDoesNotMutateBackoff` accesses `c.retryBackoff` directly (a private field) from within the `github` package test — this works because the test is in `package github` (white-box test). This is intentional and correct given the test file is in the same package. | ## Recommendation **APPROVE** — Approve. The implementation is high quality — security-conscious redirect handling, clean separation of concerns, idiomatic error types with `errors.As`, proper copy of the backoff slice to prevent mutation, and thorough test coverage including edge cases (past HTTP-dates, exact-at-limit body sizes, empty tokens). The minor issues identified are design observations rather than bugs: the `resp` lifetime coupling between `handleResponse` and the caller's status check is the most noteworthy, but it works correctly as written. The conventions around `testing.Short()` for slow tests are followed. CI passed. Ready to merge. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 0e7e12a9* </details> <!-- review-bot:sonnet -->
@@ -0,0 +227,4 @@
case <-timer.C:
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
case <-ctx.Done():
timer.Stop()
First-time contributor

[NIT] The timer pattern uses timer.Stop() after <-timer.C with a comment 'no-op after fire; kept for symmetry'. The standard Go idiom from the docs recommends this for correctness in case of concurrent resets, but for a non-reset timer this is indeed a no-op. The comment explains the intent — this is fine as written.

**[NIT]** The timer pattern uses `timer.Stop()` after `<-timer.C` with a comment 'no-op after fire; kept for symmetry'. The standard Go idiom from the docs recommends this for correctness in case of concurrent resets, but for a non-reset timer this is indeed a no-op. The comment explains the intent — this is fine as written.
@@ -0,0 +248,4 @@
req.Header.Set("Accept", accept)
} else {
req.Header.Set("Accept", "application/vnd.github+json")
}
First-time contributor

[MINOR] After handleResponse returns done=false (the 429 case that will be retried), resp is already closed by handleResponse's defer. The subsequent resp.StatusCode == http.StatusTooManyRequests check on line 258 reads from a closed response, but this is safe because resp is a struct value (not a stream) and StatusCode was already populated. However, the design where handleResponse owns the response lifecycle (via defer) but the caller still needs to inspect resp.StatusCode afterward is a subtle coupling. The caller reads resp.StatusCode on line 258 after handleResponse has already closed the body — this works but is fragile. Consider returning the status code from handleResponse or restructuring to make the ownership clearer.

**[MINOR]** After `handleResponse` returns `done=false` (the 429 case that will be retried), `resp` is already closed by `handleResponse`'s defer. The subsequent `resp.StatusCode == http.StatusTooManyRequests` check on line 258 reads from a closed response, but this is safe because `resp` is a struct value (not a stream) and `StatusCode` was already populated. However, the design where `handleResponse` owns the response lifecycle (via defer) but the caller still needs to inspect `resp.StatusCode` afterward is a subtle coupling. The caller reads `resp.StatusCode` on line 258 after `handleResponse` has already closed the body — this works but is fragile. Consider returning the status code from `handleResponse` or restructuring to make the ownership clearer.
@@ -0,0 +255,4 @@
return nil, fmt.Errorf("do request: %w", err)
}
body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
First-time contributor

[MINOR] The retry condition attempt < maxAttempts-1 is evaluated after handleResponse returns done=false. This means on the final attempt (attempt==2), a 429 response falls through to return nil, lastErr correctly. However, the continue on a 429 means the loop increments attempt and hits the backoff sleep at the top — but backoff has only 2 entries for 3 attempts. The indexing backoff[attempt-1] for attempt=1 gives backoff[0] and for attempt=2 gives backoff[1], which is correct. But the guard attempt < maxAttempts-1 (i.e., attempt < 2) means we only continue for attempts 0 and 1, never attempt 2. On attempt 2 a 429 hits return nil, lastErr. This is correct behavior but the interaction between the backoff slice length and maxAttempts is implicit and could silently break if maxAttempts is changed without updating the default backoff.

**[MINOR]** The retry condition `attempt < maxAttempts-1` is evaluated after `handleResponse` returns `done=false`. This means on the final attempt (attempt==2), a 429 response falls through to `return nil, lastErr` correctly. However, the `continue` on a 429 means the loop increments `attempt` and hits the backoff sleep at the top — but `backoff` has only 2 entries for 3 attempts. The indexing `backoff[attempt-1]` for attempt=1 gives backoff[0] and for attempt=2 gives backoff[1], which is correct. But the guard `attempt < maxAttempts-1` (i.e., attempt < 2) means we only continue for attempts 0 and 1, never attempt 2. On attempt 2 a 429 hits `return nil, lastErr`. This is correct behavior but the interaction between the backoff slice length and maxAttempts is implicit and could silently break if maxAttempts is changed without updating the default backoff.
@@ -0,0 +176,4 @@
err2 := &APIError{StatusCode: 500, Body: "server error"}
if IsNotFound(err2) {
t.Error("expected IsNotFound to return false for 500")
}
First-time contributor

[NIT] The test TestDoRequest_RetryAfterDoesNotMutateBackoff skips in short mode with t.Skip("skipping slow retry test in short mode") but the test sets a Retry-After: 1 header which causes a ~1 second delay. This is intentional for the actual retry test, but the 'does not mutate' property could be verified without the real delay by using RetryAfter: 0 or a past date. Minor since the skip guard is appropriate.

**[NIT]** The test `TestDoRequest_RetryAfterDoesNotMutateBackoff` skips in short mode with `t.Skip("skipping slow retry test in short mode")` but the test sets a `Retry-After: 1` header which causes a ~1 second delay. This is intentional for the actual retry test, but the 'does not mutate' property could be verified without the real delay by using `RetryAfter: 0` or a past date. Minor since the skip guard is appropriate.
@@ -0,0 +192,4 @@
if strings.Contains(msg, "\n") || strings.Contains(msg, "\r") {
t.Errorf("expected newlines to be sanitized, got: %q", msg)
}
if !strings.Contains(msg, "line1 injected more") {
First-time contributor

[NIT] TestDoRequest_RetryAfterDoesNotMutateBackoff accesses c.retryBackoff directly (a private field) from within the github package test — this works because the test is in package github (white-box test). This is intentional and correct given the test file is in the same package.

**[NIT]** `TestDoRequest_RetryAfterDoesNotMutateBackoff` accesses `c.retryBackoff` directly (a private field) from within the `github` package test — this works because the test is in `package github` (white-box test). This is intentional and correct given the test file is in the same package.
security-review-bot requested review from security-review-bot 2026-05-13 04:14:07 +00:00
security-review-bot approved these changes 2026-05-13 04:14:07 +00:00
security-review-bot left a comment
Collaborator

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 0e7e12a9)

Security Review

Summary

The new GitHub API client is implemented with sound security practices: HTTPS enforcement for credentials, safe redirect handling to prevent token leakage, bounded response reading, and cautious error reporting. CI has passed and no exploitable vulnerabilities were identified.

Recommendation

APPROVE — Proceed with merge. The client’s protections against HTTPS-to-HTTP downgrades, cross-host Authorization leakage, and oversized responses are appropriate. The retry behavior respects Retry-After with sane caps, and error messages are sanitized and bounded. No additional security issues were found in this change.


Review by security


Evaluated against 0e7e12a9

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2975) for up-to-date findings. <details><summary>Previous findings (commit 0e7e12a9)</summary> # Security Review ## Summary The new GitHub API client is implemented with sound security practices: HTTPS enforcement for credentials, safe redirect handling to prevent token leakage, bounded response reading, and cautious error reporting. CI has passed and no exploitable vulnerabilities were identified. ## Recommendation **APPROVE** — Proceed with merge. The client’s protections against HTTPS-to-HTTP downgrades, cross-host Authorization leakage, and oversized responses are appropriate. The retry behavior respects Retry-After with sane caps, and error messages are sanitized and bounded. No additional security issues were found in this change. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 0e7e12a9* </details> <!-- review-bot:security -->
gpt-review-bot approved these changes 2026-05-13 04:14:11 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 0e7e12a9)

Gpt Review

Summary

Well-designed GitHub client foundation with safe redirect handling, sensible retry logic (including Retry-After support), and thorough tests. Changes to VCS types are minimal and appropriate.

Recommendation

APPROVE — The implementation follows idiomatic Go patterns: clear package docs, concrete return type from constructor, small helper methods, and robust error handling with structured APIError and errors.As. Redirect policy prevents credential leakage, and the 429 retry logic respects Retry-After with caps and preserves the original backoff slice. Tests are comprehensive and cover headers, retries, limits, and security checks. CI passed. Proceed to merge.


Review by gpt


Evaluated against 0e7e12a9

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2973) for up-to-date findings. <details><summary>Previous findings (commit 0e7e12a9)</summary> # Gpt Review ## Summary Well-designed GitHub client foundation with safe redirect handling, sensible retry logic (including Retry-After support), and thorough tests. Changes to VCS types are minimal and appropriate. ## Recommendation **APPROVE** — The implementation follows idiomatic Go patterns: clear package docs, concrete return type from constructor, small helper methods, and robust error handling with structured APIError and errors.As. Redirect policy prevents credential leakage, and the 429 retry logic respects Retry-After with caps and preserves the original backoff slice. Tests are comprehensive and cover headers, retries, limits, and security checks. CI passed. Proceed to merge. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 0e7e12a9* </details> <!-- review-bot:gpt -->
rodin added the wip label 2026-05-13 04:24:15 +00:00
rodin added 1 commit 2026-05-13 04:26:44 +00:00
fix(github): clarify response ownership and validate backoff length
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m22s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m51s
3d1260d3b2
Address review feedback on PR #101:

1. Capture resp.StatusCode and Retry-After header *before* passing resp
   to handleResponse, making ownership transfer explicit. Previously the
   caller read resp.StatusCode after handleResponse had closed the body —
   correct but fragile coupling.

2. Add panic guard ensuring backoff slice length equals maxAttempts-1.
   Previously the relationship was implicit and could silently break if
   maxAttempts were changed without updating the default backoff.
rodin reviewed 2026-05-13 04:27:06 +00:00
rodin left a comment
Author
Owner

Addressed the two MINOR findings. See commit 3d1260d.

Addressed the two MINOR findings. See commit 3d1260d.
github/client.go Outdated
@@ -0,0 +200,4 @@
} else {
backoff = []time.Duration{1 * time.Second, 2 * time.Second}
}
if len(backoff) != maxAttempts-1 {
Author
Owner

Fixed. Added a panic guard ensuring len(backoff) == maxAttempts-1. If someone changes maxAttempts without updating the default backoff, this fires immediately in tests rather than silently producing incorrect retry behavior.

Fixed. Added a panic guard ensuring `len(backoff) == maxAttempts-1`. If someone changes `maxAttempts` without updating the default backoff, this fires immediately in tests rather than silently producing incorrect retry behavior.
github/client.go Outdated
@@ -0,0 +261,4 @@
return nil, fmt.Errorf("do request: %w", err)
}
// Capture response metadata before handleResponse takes body ownership.
Author
Owner

Fixed. Now capturing resp.StatusCode and Retry-After header into local variables before calling handleResponse, making the ownership transfer explicit. The caller no longer touches resp after handing it off.

Fixed. Now capturing `resp.StatusCode` and `Retry-After` header into local variables *before* calling `handleResponse`, making the ownership transfer explicit. The caller no longer touches `resp` after handing it off.
Author
Owner

Re NITs:

  • timer.Stop() after fire (id:18031): Agreed, kept as-is — the comment explains the intent.
  • RetryAfterDoesNotMutateBackoff skip guard (id:18030): Fair point about using Retry-After: 0, but the skip guard handles the slow path cleanly. Leaving as-is.
  • White-box test accessing private field (id:18032): Intentional, no change needed.
Re NITs: - **timer.Stop() after fire** (id:18031): Agreed, kept as-is — the comment explains the intent. - **RetryAfterDoesNotMutateBackoff skip guard** (id:18030): Fair point about using `Retry-After: 0`, but the skip guard handles the slow path cleanly. Leaving as-is. - **White-box test accessing private field** (id:18032): Intentional, no change needed.
rodin removed the wip label 2026-05-13 04:27:18 +00:00
sonnet-review-bot approved these changes 2026-05-13 04:27:43 +00:00
sonnet-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3d1260d3)

Sonnet Review

Summary

Well-structured GitHub API client foundation with thorough security measures (HTTPS enforcement, auth-stripping on cross-host redirects, log injection sanitization), comprehensive retry logic with Retry-After support, and excellent test coverage. The code follows established Go patterns throughout.

Findings

# Severity File Line Finding
1 [MINOR] github/client.go 197 The panic on backoff length mismatch is a deliberate 'catch misconfiguration in tests' guard, but panicking in a library function violates the repo convention ('Return errors; never panic'). The comment justifies it as a programming error caught early, but it means any test that calls SetRetryBackoff with the wrong length (e.g., during future refactoring that changes maxAttempts) will panic rather than getting a useful error. Consider returning an error from doRequest or validating at SetRetryBackoff time instead.
2 [MINOR] github/client.go 248 When delay == 0 (e.g., a past HTTP-date Retry-After), the timer block is skipped entirely due to the if delay > 0 guard, which is correct. However, if attempt-1 >= len(backoff) (impossible given the length check, but defensive readers will note it), delay stays 0 and is also skipped. The logic is correct but the attempt-1 < len(backoff) guard (line 245) is now dead code because the panic above guarantees len(backoff) == maxAttempts-1 == 2 and attempt ranges 1..2, so attempt-1 is always 0 or 1, always in bounds. The dead branch is harmless but slightly confusing.
3 [MINOR] github/client.go 116 The SetHTTPClient and SetRetryBackoff methods are documented as 'test setup only' and 'must be called before any goroutines issue requests'. Consider either unexported test helpers via an export_test.go bridge (following the stdlib export_test.go pattern documented in the testing patterns) to avoid exposing these mutation methods on the public API surface, or at minimum adding a //nolint or similar signal. As-is, exported mutation methods on a type documented as 'safe for concurrent use' is a mild API design tension.
4 [NIT] github/client_test.go 167 TestDoRequest_429ExhaustsRetries uses a direct type assertion err.(*APIError) rather than errors.As. While this works since doRequest returns *APIError unwrapped, using errors.As would be more idiomatic per the error-handling patterns and would remain correct if error wrapping is added later.
5 [NIT] github/client.go 298 Minor timer resource leak: when delay == 0, the timer is never created but timer.Stop() is only called after the select fires. This is fine. However, in the case <-timer.C branch, timer.Stop() is called after the channel has already fired — the comment acknowledges it's a no-op. This is correct per Go docs but the symmetry comment is slightly misleading; timer.Stop() after drain is genuinely a no-op and serves no purpose beyond visual symmetry. Not a bug.

Recommendation

APPROVE — Approve with minor notes. The implementation is high quality: security-conscious (HTTPS enforcement, credential stripping, log injection sanitization), well-tested (27 tests covering happy paths, retry exhaustion, header behavior, Retry-After formats, body size limits, redirect policy), and follows Go patterns throughout. The one convention deviation worth flagging is the panic in doRequest for backoff length mismatch — the repo convention says 'Return errors; never panic' — but this is a programming-error guard that will only fire when a developer misconfigures the backoff array, and is effectively dead in correctly-configured code. The test-setup methods exposed on the public API (SetHTTPClient, SetRetryBackoff) are a mild design tension with the 'safe for concurrent use' guarantee, but are clearly documented. These are not blocking issues given CI passes.


Review by sonnet


Evaluated against 3d1260d3

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2976) for up-to-date findings. <details><summary>Previous findings (commit 3d1260d3)</summary> # Sonnet Review ## Summary Well-structured GitHub API client foundation with thorough security measures (HTTPS enforcement, auth-stripping on cross-host redirects, log injection sanitization), comprehensive retry logic with Retry-After support, and excellent test coverage. The code follows established Go patterns throughout. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `github/client.go` | 197 | The panic on backoff length mismatch is a deliberate 'catch misconfiguration in tests' guard, but panicking in a library function violates the repo convention ('Return errors; never panic'). The comment justifies it as a programming error caught early, but it means any test that calls SetRetryBackoff with the wrong length (e.g., during future refactoring that changes maxAttempts) will panic rather than getting a useful error. Consider returning an error from doRequest or validating at SetRetryBackoff time instead. | | 2 | [MINOR] | `github/client.go` | 248 | When `delay == 0` (e.g., a past HTTP-date Retry-After), the timer block is skipped entirely due to the `if delay > 0` guard, which is correct. However, if `attempt-1 >= len(backoff)` (impossible given the length check, but defensive readers will note it), delay stays 0 and is also skipped. The logic is correct but the `attempt-1 < len(backoff)` guard (line 245) is now dead code because the panic above guarantees `len(backoff) == maxAttempts-1 == 2` and `attempt` ranges 1..2, so `attempt-1` is always 0 or 1, always in bounds. The dead branch is harmless but slightly confusing. | | 3 | [MINOR] | `github/client.go` | 116 | The `SetHTTPClient` and `SetRetryBackoff` methods are documented as 'test setup only' and 'must be called before any goroutines issue requests'. Consider either unexported test helpers via an export_test.go bridge (following the stdlib export_test.go pattern documented in the testing patterns) to avoid exposing these mutation methods on the public API surface, or at minimum adding a `//nolint` or similar signal. As-is, exported mutation methods on a type documented as 'safe for concurrent use' is a mild API design tension. | | 4 | [NIT] | `github/client_test.go` | 167 | TestDoRequest_429ExhaustsRetries uses a direct type assertion `err.(*APIError)` rather than `errors.As`. While this works since doRequest returns *APIError unwrapped, using `errors.As` would be more idiomatic per the error-handling patterns and would remain correct if error wrapping is added later. | | 5 | [NIT] | `github/client.go` | 298 | Minor timer resource leak: when `delay == 0`, the timer is never created but `timer.Stop()` is only called after the select fires. This is fine. However, in the `case <-timer.C` branch, `timer.Stop()` is called after the channel has already fired — the comment acknowledges it's a no-op. This is correct per Go docs but the symmetry comment is slightly misleading; `timer.Stop()` after drain is genuinely a no-op and serves no purpose beyond visual symmetry. Not a bug. | ## Recommendation **APPROVE** — Approve with minor notes. The implementation is high quality: security-conscious (HTTPS enforcement, credential stripping, log injection sanitization), well-tested (27 tests covering happy paths, retry exhaustion, header behavior, Retry-After formats, body size limits, redirect policy), and follows Go patterns throughout. The one convention deviation worth flagging is the `panic` in `doRequest` for backoff length mismatch — the repo convention says 'Return errors; never panic' — but this is a programming-error guard that will only fire when a developer misconfigures the backoff array, and is effectively dead in correctly-configured code. The test-setup methods exposed on the public API (`SetHTTPClient`, `SetRetryBackoff`) are a mild design tension with the 'safe for concurrent use' guarantee, but are clearly documented. These are not blocking issues given CI passes. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 3d1260d3* </details> <!-- review-bot:sonnet -->
@@ -0,0 +113,4 @@
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}
First-time contributor

[MINOR] The SetHTTPClient and SetRetryBackoff methods are documented as 'test setup only' and 'must be called before any goroutines issue requests'. Consider either unexported test helpers via an export_test.go bridge (following the stdlib export_test.go pattern documented in the testing patterns) to avoid exposing these mutation methods on the public API surface, or at minimum adding a //nolint or similar signal. As-is, exported mutation methods on a type documented as 'safe for concurrent use' is a mild API design tension.

**[MINOR]** The `SetHTTPClient` and `SetRetryBackoff` methods are documented as 'test setup only' and 'must be called before any goroutines issue requests'. Consider either unexported test helpers via an export_test.go bridge (following the stdlib export_test.go pattern documented in the testing patterns) to avoid exposing these mutation methods on the public API surface, or at minimum adding a `//nolint` or similar signal. As-is, exported mutation methods on a type documented as 'safe for concurrent use' is a mild API design tension.
@@ -0,0 +194,4 @@
// Length must be maxAttempts-1 (one entry per retry gap). Panic early on misconfiguration
// so a maxAttempts change without a matching backoff update is caught in tests, not production.
var backoff []time.Duration
if c.retryBackoff != nil {
First-time contributor

[MINOR] The panic on backoff length mismatch is a deliberate 'catch misconfiguration in tests' guard, but panicking in a library function violates the repo convention ('Return errors; never panic'). The comment justifies it as a programming error caught early, but it means any test that calls SetRetryBackoff with the wrong length (e.g., during future refactoring that changes maxAttempts) will panic rather than getting a useful error. Consider returning an error from doRequest or validating at SetRetryBackoff time instead.

**[MINOR]** The panic on backoff length mismatch is a deliberate 'catch misconfiguration in tests' guard, but panicking in a library function violates the repo convention ('Return errors; never panic'). The comment justifies it as a programming error caught early, but it means any test that calls SetRetryBackoff with the wrong length (e.g., during future refactoring that changes maxAttempts) will panic rather than getting a useful error. Consider returning an error from doRequest or validating at SetRetryBackoff time instead.
@@ -0,0 +245,4 @@
}
if c.token != "" {
// Bearer is the OAuth2 standard and is accepted by GitHub for both
// classic PATs and fine-grained tokens. The alternative "token" scheme
First-time contributor

[MINOR] When delay == 0 (e.g., a past HTTP-date Retry-After), the timer block is skipped entirely due to the if delay > 0 guard, which is correct. However, if attempt-1 >= len(backoff) (impossible given the length check, but defensive readers will note it), delay stays 0 and is also skipped. The logic is correct but the attempt-1 < len(backoff) guard (line 245) is now dead code because the panic above guarantees len(backoff) == maxAttempts-1 == 2 and attempt ranges 1..2, so attempt-1 is always 0 or 1, always in bounds. The dead branch is harmless but slightly confusing.

**[MINOR]** When `delay == 0` (e.g., a past HTTP-date Retry-After), the timer block is skipped entirely due to the `if delay > 0` guard, which is correct. However, if `attempt-1 >= len(backoff)` (impossible given the length check, but defensive readers will note it), delay stays 0 and is also skipped. The logic is correct but the `attempt-1 < len(backoff)` guard (line 245) is now dead code because the panic above guarantees `len(backoff) == maxAttempts-1 == 2` and `attempt` ranges 1..2, so `attempt-1` is always 0 or 1, always in bounds. The dead branch is harmless but slightly confusing.
@@ -0,0 +295,4 @@
if attempt < len(backoff) {
backoff[attempt] = delay
}
}
First-time contributor

[NIT] Minor timer resource leak: when delay == 0, the timer is never created but timer.Stop() is only called after the select fires. This is fine. However, in the case <-timer.C branch, timer.Stop() is called after the channel has already fired — the comment acknowledges it's a no-op. This is correct per Go docs but the symmetry comment is slightly misleading; timer.Stop() after drain is genuinely a no-op and serves no purpose beyond visual symmetry. Not a bug.

**[NIT]** Minor timer resource leak: when `delay == 0`, the timer is never created but `timer.Stop()` is only called after the select fires. This is fine. However, in the `case <-timer.C` branch, `timer.Stop()` is called after the channel has already fired — the comment acknowledges it's a no-op. This is correct per Go docs but the symmetry comment is slightly misleading; `timer.Stop()` after drain is genuinely a no-op and serves no purpose beyond visual symmetry. Not a bug.
@@ -0,0 +164,4 @@
t.Fatal("expected error for 401")
}
if attempts != 1 {
t.Errorf("expected 1 attempt (no retry on 401), got %d", attempts)
First-time contributor

[NIT] TestDoRequest_429ExhaustsRetries uses a direct type assertion err.(*APIError) rather than errors.As. While this works since doRequest returns *APIError unwrapped, using errors.As would be more idiomatic per the error-handling patterns and would remain correct if error wrapping is added later.

**[NIT]** TestDoRequest_429ExhaustsRetries uses a direct type assertion `err.(*APIError)` rather than `errors.As`. While this works since doRequest returns *APIError unwrapped, using `errors.As` would be more idiomatic per the error-handling patterns and would remain correct if error wrapping is added later.
gpt-review-bot requested changes 2026-05-13 04:28:25 +00:00
Dismissed
gpt-review-bot left a comment
First-time contributor

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3d1260d3)

Gpt Review

Summary

Solid foundation for a GitHub API client with comprehensive tests and sensible defaults. However, the client panics on a backoff misconfiguration, which violates the repository’s error-handling policy and Go best practices.

Findings

# Severity File Line Finding
1 [MAJOR] github/client.go 166 doRequest panics if the computed backoff length does not equal maxAttempts-1. Repository conventions require returning errors and never panicking; this should be handled gracefully (e.g., defaulting, validation at setter time, or returning an error).
2 [MINOR] github/client.go 152 The strict coupling between maxAttempts and backoff length could be relaxed. Consider validating the slice in SetRetryBackoff (returning an error) or tolerating mismatched lengths by using available entries and treating missing entries as zero delays, instead of enforcing equality at request time.
3 [NIT] github/client.go 194 Calling timer.Stop after the timer has already fired is unnecessary. It’s harmless but can be removed for clarity.
4 [MINOR] github/client.go 277 The error message references "(truncated)" for oversized success responses, but the function returns an error without returning a truncated body. Consider rewording to avoid implying truncated data was returned.
5 [MINOR] github/client.go 75 Functional options are used for a single AllowInsecureHTTP option. While acceptable if you plan to add more options soon, the documented configuration patterns suggest avoiding functional options for <3 options unless growth is expected.

Recommendation

REQUEST_CHANGES — Replace the panic in doRequest with a non-panicking approach consistent with the repository’s error-handling policy. Options include: (1) validate and enforce the expected backoff length in SetRetryBackoff by returning an error to the caller; (2) tolerate mismatched lengths by using provided values and treating missing entries as zero-delay; or (3) default to an internal safe backoff when invalid. This ensures the library never panics in production. Additionally, consider minor cleanups: remove the unnecessary timer.Stop call post-fire, and tweak the oversized-body error message to avoid implying a truncated body was returned. If no further options are foreseen, consider whether functional options are necessary; otherwise, keeping them is fine if more options are planned.


Review by gpt


Evaluated against 3d1260d3

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2977) for up-to-date findings. <details><summary>Previous findings (commit 3d1260d3)</summary> # Gpt Review ## Summary Solid foundation for a GitHub API client with comprehensive tests and sensible defaults. However, the client panics on a backoff misconfiguration, which violates the repository’s error-handling policy and Go best practices. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MAJOR] | `github/client.go` | 166 | doRequest panics if the computed backoff length does not equal maxAttempts-1. Repository conventions require returning errors and never panicking; this should be handled gracefully (e.g., defaulting, validation at setter time, or returning an error). | | 2 | [MINOR] | `github/client.go` | 152 | The strict coupling between maxAttempts and backoff length could be relaxed. Consider validating the slice in SetRetryBackoff (returning an error) or tolerating mismatched lengths by using available entries and treating missing entries as zero delays, instead of enforcing equality at request time. | | 3 | [NIT] | `github/client.go` | 194 | Calling timer.Stop after the timer has already fired is unnecessary. It’s harmless but can be removed for clarity. | | 4 | [MINOR] | `github/client.go` | 277 | The error message references "(truncated)" for oversized success responses, but the function returns an error without returning a truncated body. Consider rewording to avoid implying truncated data was returned. | | 5 | [MINOR] | `github/client.go` | 75 | Functional options are used for a single AllowInsecureHTTP option. While acceptable if you plan to add more options soon, the documented configuration patterns suggest avoiding functional options for <3 options unless growth is expected. | ## Recommendation **REQUEST_CHANGES** — Replace the panic in doRequest with a non-panicking approach consistent with the repository’s error-handling policy. Options include: (1) validate and enforce the expected backoff length in SetRetryBackoff by returning an error to the caller; (2) tolerate mismatched lengths by using provided values and treating missing entries as zero-delay; or (3) default to an internal safe backoff when invalid. This ensures the library never panics in production. Additionally, consider minor cleanups: remove the unnecessary timer.Stop call post-fire, and tweak the oversized-body error message to avoid implying a truncated body was returned. If no further options are foreseen, consider whether functional options are necessary; otherwise, keeping them is fine if more options are planned. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 3d1260d3* </details> <!-- review-bot:gpt -->
@@ -0,0 +72,4 @@
return target, true
}
return nil, false
}
First-time contributor

[MINOR] Functional options are used for a single AllowInsecureHTTP option. While acceptable if you plan to add more options soon, the documented configuration patterns suggest avoiding functional options for <3 options unless growth is expected.

**[MINOR]** Functional options are used for a single AllowInsecureHTTP option. While acceptable if you plan to add more options soon, the documented configuration patterns suggest avoiding functional options for <3 options unless growth is expected.
@@ -0,0 +149,4 @@
baseURL: strings.TrimRight(baseURL, "/"),
allowInsecureHTTP: cfg.allowInsecureHTTP,
token: token,
httpClient: &http.Client{
First-time contributor

[MINOR] The strict coupling between maxAttempts and backoff length could be relaxed. Consider validating the slice in SetRetryBackoff (returning an error) or tolerating mismatched lengths by using available entries and treating missing entries as zero delays, instead of enforcing equality at request time.

**[MINOR]** The strict coupling between maxAttempts and backoff length could be relaxed. Consider validating the slice in SetRetryBackoff (returning an error) or tolerating mismatched lengths by using available entries and treating missing entries as zero delays, instead of enforcing equality at request time.
@@ -0,0 +163,4 @@
// Passing nil restores the default client (30s timeout + auth-stripping
// CheckRedirect policy matching NewClient).
//
// Callers providing a non-nil client are responsible for configuring a safe
First-time contributor

[MAJOR] doRequest panics if the computed backoff length does not equal maxAttempts-1. Repository conventions require returning errors and never panicking; this should be handled gracefully (e.g., defaulting, validation at setter time, or returning an error).

**[MAJOR]** doRequest panics if the computed backoff length does not equal maxAttempts-1. Repository conventions require returning errors and never panicking; this should be handled gracefully (e.g., defaulting, validation at setter time, or returning an error).
github/client.go Outdated
@@ -0,0 +191,4 @@
const maxRetryAfter = 120 * time.Second
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
// Length must be maxAttempts-1 (one entry per retry gap). Panic early on misconfiguration
First-time contributor

[NIT] Calling timer.Stop after the timer has already fired is unnecessary. It’s harmless but can be removed for clarity.

**[NIT]** Calling timer.Stop after the timer has already fired is unnecessary. It’s harmless but can be removed for clarity.
@@ -0,0 +274,4 @@
// Retry on 429 rate limit
if respStatus == http.StatusTooManyRequests && attempt < maxAttempts-1 {
// Check for Retry-After header and override backoff if present.
// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
First-time contributor

[MINOR] The error message references "(truncated)" for oversized success responses, but the function returns an error without returning a truncated body. Consider rewording to avoid implying truncated data was returned.

**[MINOR]** The error message references "(truncated)" for oversized success responses, but the function returns an error without returning a truncated body. Consider rewording to avoid implying truncated data was returned.
security-review-bot requested review from security-review-bot 2026-05-13 04:28:53 +00:00
security-review-bot approved these changes 2026-05-13 04:28:53 +00:00
security-review-bot left a comment
Collaborator

Original review

Supersededsee current review for up-to-date findings.

Previous findings (commit 3d1260d3)

Security Review

Summary

Solid foundational GitHub client with secure defaults: enforces HTTPS for tokens by default, strips Authorization on cross-host redirects, caps response sizes, and honors Retry-After with bounds. Tests cover critical behaviors. One minor availability concern noted.

Findings

# Severity File Line Finding
1 [MINOR] github/client.go 204 doRequest panics if the computed backoff length doesn't match maxAttempts-1. While intended to catch misconfiguration in tests, panics can crash the process and create a denial-of-service if this method is misused in production. Prefer returning an error or falling back to a safe default to avoid process termination.

Recommendation

APPROVE — The implementation follows secure patterns: it avoids sending credentials over plaintext by default, strips Authorization on cross-host redirects to prevent credential leakage, caps response body sizes to prevent resource exhaustion, and bounds Retry-After to mitigate long delays. CI passed and tests cover key security behaviors. Consider replacing the panic on backoff length mismatch with a safe default or an explicit error to prevent potential availability issues if misused. Otherwise, the changes are sound from a security perspective and can be merged.


Review by security


Evaluated against 3d1260d3

~~Original review~~ **Superseded** — [see current review](https://gitea.weiker.me/rodin/review-bot/pulls/101#pullrequestreview-2979) for up-to-date findings. <details><summary>Previous findings (commit 3d1260d3)</summary> # Security Review ## Summary Solid foundational GitHub client with secure defaults: enforces HTTPS for tokens by default, strips Authorization on cross-host redirects, caps response sizes, and honors Retry-After with bounds. Tests cover critical behaviors. One minor availability concern noted. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `github/client.go` | 204 | doRequest panics if the computed backoff length doesn't match maxAttempts-1. While intended to catch misconfiguration in tests, panics can crash the process and create a denial-of-service if this method is misused in production. Prefer returning an error or falling back to a safe default to avoid process termination. | ## Recommendation **APPROVE** — The implementation follows secure patterns: it avoids sending credentials over plaintext by default, strips Authorization on cross-host redirects to prevent credential leakage, caps response body sizes to prevent resource exhaustion, and bounds Retry-After to mitigate long delays. CI passed and tests cover key security behaviors. Consider replacing the panic on backoff length mismatch with a safe default or an explicit error to prevent potential availability issues if misused. Otherwise, the changes are sound from a security perspective and can be merged. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 3d1260d3* </details> <!-- review-bot:security -->
github/client.go Outdated
@@ -0,0 +201,4 @@
backoff = []time.Duration{1 * time.Second, 2 * time.Second}
}
if len(backoff) != maxAttempts-1 {
panic(fmt.Sprintf("github: backoff length %d does not match maxAttempts-1 (%d)", len(backoff), maxAttempts-1))
Collaborator

[MINOR] doRequest panics if the computed backoff length doesn't match maxAttempts-1. While intended to catch misconfiguration in tests, panics can crash the process and create a denial-of-service if this method is misused in production. Prefer returning an error or falling back to a safe default to avoid process termination.

**[MINOR]** doRequest panics if the computed backoff length doesn't match maxAttempts-1. While intended to catch misconfiguration in tests, panics can crash the process and create a denial-of-service if this method is misused in production. Prefer returning an error or falling back to a safe default to avoid process termination.
security-review-bot marked this conversation as resolved
rodin added the wip label 2026-05-13 04:29:02 +00:00
rodin added 1 commit 2026-05-13 04:31:49 +00:00
fix(github): address review findings - remove panic, validate at config time
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m35s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m7s
61819ac3e3
- MAJOR #1: Replace panic in doRequest with safe default fallback.
  Validation now happens in SetRetryBackoff (returns error on invalid
  length). doRequest gracefully falls back to default backoff if the
  configured slice is somehow invalid.

- MINOR #2: SetRetryBackoff validates slice length at configuration
  time, making the coupling between maxRetryAttempts and backoff
  explicit and catching mismatches early with a clear error.

- MINOR #4: Reword oversized response error to remove '(truncated)'
  which implied truncated data was returned when actually only an
  error is returned.

- MINOR #5: Functional options kept as-is - idiomatic Go pattern
  that allows future growth without breaking the API.
Author
Owner

Re gpt-review-bot findings (commit 61819ac):

  • MAJOR #1 (panic on backoff misconfiguration): Fixed — removed the panic from doRequest. Validation now happens in SetRetryBackoff which returns an error on invalid length. doRequest defensively falls back to the safe default backoff {1s, 2s} if somehow misconfigured.

  • MINOR #2 (backoff/maxAttempts coupling): Fixed — SetRetryBackoff validates length against maxRetryAttempts-1 at configuration time, making the coupling explicit and catching mismatches early with a clear error message.

  • MINOR #4 (truncated error message): Fixed — reworded from "exceeded N bytes (truncated)" to "exceeded N bytes" since no truncated body is returned.

  • MINOR #5 (functional options): Keeping as-is. This is idiomatic Go for constructor options and allows future growth without breaking the API. The pattern costs nothing at runtime and one option is a valid starting point.

Added TestSetRetryBackoff_RejectsInvalidLength to verify the validation. All tests pass.

Re gpt-review-bot findings (commit 61819ac): - **MAJOR #1 (panic on backoff misconfiguration)**: Fixed — removed the panic from `doRequest`. Validation now happens in `SetRetryBackoff` which returns an error on invalid length. `doRequest` defensively falls back to the safe default backoff `{1s, 2s}` if somehow misconfigured. - **MINOR #2 (backoff/maxAttempts coupling)**: Fixed — `SetRetryBackoff` validates length against `maxRetryAttempts-1` at configuration time, making the coupling explicit and catching mismatches early with a clear error message. - **MINOR #4 (truncated error message)**: Fixed — reworded from "exceeded N bytes (truncated)" to "exceeded N bytes" since no truncated body is returned. - **MINOR #5 (functional options)**: Keeping as-is. This is idiomatic Go for constructor options and allows future growth without breaking the API. The pattern costs nothing at runtime and one option is a valid starting point. Added `TestSetRetryBackoff_RejectsInvalidLength` to verify the validation. All tests pass.
rodin removed the wip label 2026-05-13 04:32:05 +00:00
sonnet-review-bot approved these changes 2026-05-13 04:32:44 +00:00
sonnet-review-bot left a comment
First-time contributor

Sonnet Review

Summary

This is a well-implemented GitHub API client foundation. The code is idiomatic Go, follows the established patterns, has thorough test coverage with good edge case handling, and CI passes. A few minor observations worth noting but none blocking.

Findings

# Severity File Line Finding
1 [NIT] github/client.go 200 The defaultBackoff local variable is declared but serves only as a template for the else branch. The variable name and the copy-pattern are fine, but it could be simplified slightly: the if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 guard duplicates the invariant already enforced by SetRetryBackoff. A comment noting that the length check here is a defensive guard against direct struct construction (bypassing the setter) would clarify the intent.
2 [NIT] github/client.go 271 Transport errors from c.httpClient.Do(req) are returned immediately without setting lastErr, meaning if a transport error occurs on attempt > 0 after successful 429 retries, the function correctly returns the error — but if the intent is to return the last meaningful error in all cases, the transport error path bypasses lastErr. This is fine as-is since transport errors are not retried per the documented contract, but a brief inline comment noting 'transport errors are non-retryable, return immediately' would improve clarity.
3 [NIT] github/client.go 120 The defaultCheckRedirect function uses len(via) >= 10 to cap redirects, matching net/http's own default of 10. However, the stdlib's default CheckRedirect returns ErrUseLastResponse-based behavior; this returns a plain fmt.Errorf. This is functionally equivalent (both cause the redirect to fail) but the error message won't be recognized as a redirect limit error by callers using errors.Is. Since this is internal behavior and unlikely to be checked by callers, it's a minor style point only.
4 [NIT] github/client_test.go 489 Double blank line between TestSetHTTPClient_NilRestoresDefault and TestSetRetryBackoff_RejectsInvalidLength. Minor formatting — gofmt would normalize this to a single blank line.
5 [NIT] github/client.go 93 The allowInsecureHTTP field is stored on both clientConfig and Client. While this is a deliberate copy (the option is read once at construction and stored for use in doRequest), adding a brief comment on the Client.allowInsecureHTTP field noting it is set from ClientOption at construction time would make the data flow clearer for future maintainers.

Recommendation

APPROVE — Approve. The implementation is solid: correct error handling with %w wrapping and errors.As, proper use of io.LimitReader for body size enforcement, defensive copy of the backoff slice to prevent mutation, security-conscious redirect policy, and comprehensive test coverage including edge cases (past HTTP-date, exact-at-limit body, no-token HTTP, backoff mutation guard). All findings are nits. The code follows the repository's conventions and the established Go patterns throughout.


Review by sonnet


Evaluated against 61819ac3

# Sonnet Review ## Summary This is a well-implemented GitHub API client foundation. The code is idiomatic Go, follows the established patterns, has thorough test coverage with good edge case handling, and CI passes. A few minor observations worth noting but none blocking. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [NIT] | `github/client.go` | 200 | The `defaultBackoff` local variable is declared but serves only as a template for the else branch. The variable name and the copy-pattern are fine, but it could be simplified slightly: the `if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1` guard duplicates the invariant already enforced by `SetRetryBackoff`. A comment noting that the length check here is a defensive guard against direct struct construction (bypassing the setter) would clarify the intent. | | 2 | [NIT] | `github/client.go` | 271 | Transport errors from `c.httpClient.Do(req)` are returned immediately without setting `lastErr`, meaning if a transport error occurs on attempt > 0 after successful 429 retries, the function correctly returns the error — but if the intent is to return the *last* meaningful error in all cases, the transport error path bypasses `lastErr`. This is fine as-is since transport errors are not retried per the documented contract, but a brief inline comment noting 'transport errors are non-retryable, return immediately' would improve clarity. | | 3 | [NIT] | `github/client.go` | 120 | The `defaultCheckRedirect` function uses `len(via) >= 10` to cap redirects, matching net/http's own default of 10. However, the stdlib's default `CheckRedirect` returns `ErrUseLastResponse`-based behavior; this returns a plain `fmt.Errorf`. This is functionally equivalent (both cause the redirect to fail) but the error message won't be recognized as a redirect limit error by callers using `errors.Is`. Since this is internal behavior and unlikely to be checked by callers, it's a minor style point only. | | 4 | [NIT] | `github/client_test.go` | 489 | Double blank line between `TestSetHTTPClient_NilRestoresDefault` and `TestSetRetryBackoff_RejectsInvalidLength`. Minor formatting — `gofmt` would normalize this to a single blank line. | | 5 | [NIT] | `github/client.go` | 93 | The `allowInsecureHTTP` field is stored on both `clientConfig` and `Client`. While this is a deliberate copy (the option is read once at construction and stored for use in `doRequest`), adding a brief comment on the `Client.allowInsecureHTTP` field noting it is set from `ClientOption` at construction time would make the data flow clearer for future maintainers. | ## Recommendation **APPROVE** — Approve. The implementation is solid: correct error handling with `%w` wrapping and `errors.As`, proper use of `io.LimitReader` for body size enforcement, defensive copy of the backoff slice to prevent mutation, security-conscious redirect policy, and comprehensive test coverage including edge cases (past HTTP-date, exact-at-limit body, no-token HTTP, backoff mutation guard). All findings are nits. The code follows the repository's conventions and the established Go patterns throughout. --- *Review by sonnet* <!-- review-bot:sonnet --> --- *Evaluated against 61819ac3*
@@ -0,0 +90,4 @@
// This should only be used for trusted internal deployments or testing.
func AllowInsecureHTTP() ClientOption {
return func(c *clientConfig) {
c.allowInsecureHTTP = true
First-time contributor

[NIT] The allowInsecureHTTP field is stored on both clientConfig and Client. While this is a deliberate copy (the option is read once at construction and stored for use in doRequest), adding a brief comment on the Client.allowInsecureHTTP field noting it is set from ClientOption at construction time would make the data flow clearer for future maintainers.

**[NIT]** The `allowInsecureHTTP` field is stored on both `clientConfig` and `Client`. While this is a deliberate copy (the option is read once at construction and stored for use in `doRequest`), adding a brief comment on the `Client.allowInsecureHTTP` field noting it is set from `ClientOption` at construction time would make the data flow clearer for future maintainers.
@@ -0,0 +117,4 @@
func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return fmt.Errorf("stopped after 10 redirects")
}
First-time contributor

[NIT] The defaultCheckRedirect function uses len(via) >= 10 to cap redirects, matching net/http's own default of 10. However, the stdlib's default CheckRedirect returns ErrUseLastResponse-based behavior; this returns a plain fmt.Errorf. This is functionally equivalent (both cause the redirect to fail) but the error message won't be recognized as a redirect limit error by callers using errors.Is. Since this is internal behavior and unlikely to be checked by callers, it's a minor style point only.

**[NIT]** The `defaultCheckRedirect` function uses `len(via) >= 10` to cap redirects, matching net/http's own default of 10. However, the stdlib's default `CheckRedirect` returns `ErrUseLastResponse`-based behavior; this returns a plain `fmt.Errorf`. This is functionally equivalent (both cause the redirect to fail) but the error message won't be recognized as a redirect limit error by callers using `errors.Is`. Since this is internal behavior and unlikely to be checked by callers, it's a minor style point only.
@@ -0,0 +197,4 @@
// Transport errors (network failures, context cancellation) are not retried.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
const maxRetryAfter = 120 * time.Second
First-time contributor

[NIT] The defaultBackoff local variable is declared but serves only as a template for the else branch. The variable name and the copy-pattern are fine, but it could be simplified slightly: the if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 guard duplicates the invariant already enforced by SetRetryBackoff. A comment noting that the length check here is a defensive guard against direct struct construction (bypassing the setter) would clarify the intent.

**[NIT]** The `defaultBackoff` local variable is declared but serves only as a template for the else branch. The variable name and the copy-pattern are fine, but it could be simplified slightly: the `if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1` guard duplicates the invariant already enforced by `SetRetryBackoff`. A comment noting that the length check here is a defensive guard against direct struct construction (bypassing the setter) would clarify the intent.
@@ -0,0 +268,4 @@
return nil, fmt.Errorf("do request: %w", err)
}
// Capture response metadata before handleResponse takes body ownership.
First-time contributor

[NIT] Transport errors from c.httpClient.Do(req) are returned immediately without setting lastErr, meaning if a transport error occurs on attempt > 0 after successful 429 retries, the function correctly returns the error — but if the intent is to return the last meaningful error in all cases, the transport error path bypasses lastErr. This is fine as-is since transport errors are not retried per the documented contract, but a brief inline comment noting 'transport errors are non-retryable, return immediately' would improve clarity.

**[NIT]** Transport errors from `c.httpClient.Do(req)` are returned immediately without setting `lastErr`, meaning if a transport error occurs on attempt > 0 after successful 429 retries, the function correctly returns the error — but if the intent is to return the *last* meaningful error in all cases, the transport error path bypasses `lastErr`. This is fine as-is since transport errors are not retried per the documented contract, but a brief inline comment noting 'transport errors are non-retryable, return immediately' would improve clarity.
@@ -0,0 +486,4 @@
prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
req := &http.Request{
URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
Header: http.Header{"Authorization": []string{"Bearer token"}},
First-time contributor

[NIT] Double blank line between TestSetHTTPClient_NilRestoresDefault and TestSetRetryBackoff_RejectsInvalidLength. Minor formatting — gofmt would normalize this to a single blank line.

**[NIT]** Double blank line between `TestSetHTTPClient_NilRestoresDefault` and `TestSetRetryBackoff_RejectsInvalidLength`. Minor formatting — `gofmt` would normalize this to a single blank line.
gpt-review-bot approved these changes 2026-05-13 04:33:42 +00:00
gpt-review-bot left a comment
First-time contributor

Gpt Review

Summary

Solid, idiomatic foundation for a GitHub API client with clear error handling, redirect safety, and robust 429 retry logic. Tests are comprehensive and adhere to repository conventions; CI passed.

Recommendation

APPROVE — The implementation follows Go patterns well: a clear NewClient constructor, safe redirect policy, structured APIError, context-aware requests, and bounded body reads. Retry-After handling for both seconds and HTTP-date formats is correct and non-mutating to the configured backoff, and the early refusal to send credentials over HTTP is a good safety guard (with an override for trusted environments). The addition of Patch to vcs.ChangedFile is straightforward and backwards-compatible for JSON decoding. No changes requested.


Review by gpt


Evaluated against 61819ac3

# Gpt Review ## Summary Solid, idiomatic foundation for a GitHub API client with clear error handling, redirect safety, and robust 429 retry logic. Tests are comprehensive and adhere to repository conventions; CI passed. ## Recommendation **APPROVE** — The implementation follows Go patterns well: a clear NewClient constructor, safe redirect policy, structured APIError, context-aware requests, and bounded body reads. Retry-After handling for both seconds and HTTP-date formats is correct and non-mutating to the configured backoff, and the early refusal to send credentials over HTTP is a good safety guard (with an override for trusted environments). The addition of Patch to vcs.ChangedFile is straightforward and backwards-compatible for JSON decoding. No changes requested. --- *Review by gpt* <!-- review-bot:gpt --> --- *Evaluated against 61819ac3*
security-review-bot requested review from security-review-bot 2026-05-13 04:34:14 +00:00
security-review-bot approved these changes 2026-05-13 04:34:15 +00:00
security-review-bot left a comment
Collaborator

Security Review

Summary

Solid, security-conscious GitHub API client foundation with HTTPS-by-default, redirect hardening, response size limits, and rate-limit retry behavior. No exploitable vulnerabilities identified in this change set.

Findings

# Severity File Line Finding
1 [MINOR] github/client.go 220 doRequest validates HTTPS when a token is present but does not ensure the request host matches the configured baseURL. If future internal callers accidentally pass an arbitrary URL, the Authorization header could be sent to an unintended host. Consider enforcing that reqURL's host equals the client's baseURL host (or only accept relative paths) to prevent credential leakage by misuse.
2 [NIT] github/client.go 226 The error message for refusing non-HTTPS includes the full URL. If this error is logged upstream, URL query strings could leak sensitive parameters. Consider eliding or redacting query components in error messages.

Recommendation

APPROVE — Approve the PR. The client defaults to secure behavior (HTTPS-only with tokens, redirect policy that strips Authorization on cross-host and blocks HTTPS→HTTP, response size limits, and bounded Retry-After handling). For defense in depth, consider adding a check to ensure request hosts match the configured baseURL to avoid accidental credential leakage if a future internal call passes an arbitrary URL, and consider redacting query strings from URL-containing error messages.


Review by security


Evaluated against 61819ac3

# Security Review ## Summary Solid, security-conscious GitHub API client foundation with HTTPS-by-default, redirect hardening, response size limits, and rate-limit retry behavior. No exploitable vulnerabilities identified in this change set. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `github/client.go` | 220 | doRequest validates HTTPS when a token is present but does not ensure the request host matches the configured baseURL. If future internal callers accidentally pass an arbitrary URL, the Authorization header could be sent to an unintended host. Consider enforcing that reqURL's host equals the client's baseURL host (or only accept relative paths) to prevent credential leakage by misuse. | | 2 | [NIT] | `github/client.go` | 226 | The error message for refusing non-HTTPS includes the full URL. If this error is logged upstream, URL query strings could leak sensitive parameters. Consider eliding or redacting query components in error messages. | ## Recommendation **APPROVE** — Approve the PR. The client defaults to secure behavior (HTTPS-only with tokens, redirect policy that strips Authorization on cross-host and blocks HTTPS→HTTP, response size limits, and bounded Retry-After handling). For defense in depth, consider adding a check to ensure request hosts match the configured baseURL to avoid accidental credential leakage if a future internal call passes an arbitrary URL, and consider redacting query strings from URL-containing error messages. --- *Review by security* <!-- review-bot:security --> --- *Evaluated against 61819ac3*
@@ -0,0 +217,4 @@
const maxErrorBodyBytes = 4 * 1024
// Reject non-HTTPS URLs early since the URL is immutable across retries.
if c.token != "" && !c.allowInsecureHTTP {
Collaborator

[MINOR] doRequest validates HTTPS when a token is present but does not ensure the request host matches the configured baseURL. If future internal callers accidentally pass an arbitrary URL, the Authorization header could be sent to an unintended host. Consider enforcing that reqURL's host equals the client's baseURL host (or only accept relative paths) to prevent credential leakage by misuse.

**[MINOR]** doRequest validates HTTPS when a token is present but does not ensure the request host matches the configured baseURL. If future internal callers accidentally pass an arbitrary URL, the Authorization header could be sent to an unintended host. Consider enforcing that reqURL's host equals the client's baseURL host (or only accept relative paths) to prevent credential leakage by misuse.
@@ -0,0 +223,4 @@
return nil, fmt.Errorf("parse request URL: %w", err)
}
if !strings.EqualFold(parsed.Scheme, "https") {
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
Collaborator

[NIT] The error message for refusing non-HTTPS includes the full URL. If this error is logged upstream, URL query strings could leak sensitive parameters. Consider eliding or redacting query components in error messages.

**[NIT]** The error message for refusing non-HTTPS includes the full URL. If this error is logged upstream, URL query strings could leak sensitive parameters. Consider eliding or redacting query components in error messages.
aweiker was assigned by rodin 2026-05-13 04:36:46 +00:00
rodin added the ready label 2026-05-13 04:36:46 +00:00
aweiker approved these changes 2026-05-13 04:37:57 +00:00
Dismissed
aweiker approved these changes 2026-05-13 04:46:39 +00:00
aweiker merged commit 377da8ca3a into feature/github-support 2026-05-13 04:46:46 +00:00
Sign in to join this conversation.
5 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: rodin/review-bot#101