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

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
Conversation 14 Commits 3 Files Changed 3
+63 -18
2 changed files with 63 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 61819ac3e3 - Show all commits
github/client.go
+19 -12
View File
@@ -21,6 +21,10 @@ const (
// maxResponseBytes limits successful response body reads to 10 MiB.
maxResponseBytes = 10 * 1024 * 1024
// maxRetryAttempts is the number of times doRequest will attempt a request.
// The retry backoff slice must have length maxRetryAttempts-1.
maxRetryAttempts = 3
)
// APIError represents an HTTP error response from the GitHub API.
6
 @@ -178,30 +182,33 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
// SetRetryBackoff configures the retry backoff durations for testing.
// It must be called before any goroutines issue requests.
// The slice must have exactly maxRetryAttempts-1 entries (one delay per retry gap).
// In production the default {1s, 2s} applies.
func (c *Client) SetRetryBackoff(d []time.Duration) {
func (c *Client) SetRetryBackoff(d []time.Duration) error {
if len(d) != maxRetryAttempts-1 {
return fmt.Errorf("github: backoff length %d does not match maxRetryAttempts-1 (%d)", len(d), maxRetryAttempts-1)
}
c.retryBackoff = d
return nil
}
gpt-review-bot commented 2026-05-13 04:28:25 +00:00
Outdated
Review
Copy Link
Copy Source

[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.
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present (capped at maxRetryAfter).
// Transport errors (network failures, context cancellation) are not retried.
sonnet-review-bot commented 2026-05-13 04:27:43 +00:00
Review
Copy Link
Copy Source

[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.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
const maxAttempts = 3
const maxRetryAfter = 120 * time.Second
sonnet-review-bot commented 2026-05-13 04:32:44 +00:00
Review
Copy Link
Copy Source

[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.
// 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
// so a maxAttempts change without a matching backoff update is caught in tests, not production.
// Length must be maxRetryAttempts-1 (one entry per retry gap).
// SetRetryBackoff validates at configuration time; the default is always valid.
rodin commented 2026-05-13 04:27:06 +00:00
Outdated
Review
Copy Link
Copy Source

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.
defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-13 04:28:53 +00:00
Outdated
Review
Copy Link
Copy Source

[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.
var backoff []time.Duration
if c.retryBackoff != nil {
if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
backoff = make([]time.Duration, len(c.retryBackoff))
copy(backoff, c.retryBackoff)
} else {
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))
backoff = make([]time.Duration, len(defaultBackoff))
copy(backoff, defaultBackoff)
}
// maxErrorBodyBytes limits how much of an error response body is stored.
2
 @@ -221,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
}
var lastErr error
sonnet-review-bot commented 2026-05-13 04:13:41 +00:00
Review
Copy Link
Copy Source

[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.
for attempt := 0; attempt < maxAttempts; attempt++ {
for attempt := 0; attempt < maxRetryAttempts; attempt++ {
if attempt > 0 {
var delay time.Duration
if attempt-1 < len(backoff) {
6
 @@ -272,7 +279,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
lastErr = err
// Retry on 429 rate limit
if respStatus == http.StatusTooManyRequests && attempt < maxAttempts-1 {
if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
// Check for Retry-After header and override backoff if present.
// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
if ra := retryAfterHeader; ra != "" {
1
 @@ -319,7 +326,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
return nil, true, fmt.Errorf("read response body: %w", err)
}
if len(body) > maxRespBytes {
return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes)
return nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes)
}
return body, true, nil
}
github/client_test.go
+44 -6
View File
@@ -83,7 +83,9 @@ func TestDoRequest_429Retry(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
@@ -108,7 +110,9 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
3
 @@ -218,7 +222,9 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
// Use short backoff; Retry-After should override
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -259,7 +265,9 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
@@ -297,7 +305,9 @@ func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -338,7 +348,9 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now()
_, err := c.doGet(context.Background(), srv.URL+"/test")
1
 @@ -554,3 +566,29 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
}
}
func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
c := NewClient("token", "https://api.github.com")
// Too short
err := c.SetRetryBackoff([]time.Duration{1 * time.Second})
if err == nil {
t.Fatal("expected error for backoff length 1")
}
if !strings.Contains(err.Error(), "backoff length 1") {
t.Errorf("unexpected error message: %v", err)
}
// Too long
err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second, 3 * time.Second})
if err == nil {
t.Fatal("expected error for backoff length 3")
}
// Correct length succeeds
err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second})
if err != nil {
t.Fatalf("unexpected error for valid backoff: %v", err)
}
}