fix(github): address review feedback on Retry-After implementation

- Fix data race: copy retryBackoff slice per-request to prevent
  concurrent doRequest calls from racing on shared state
- Fix parseRetryAfter: trim whitespace before parsing for robustness
- Fix parseRetryAfter: treat delta-seconds of 0 as valid per RFC 7231
- Add bounded read on success path (10 MB limit) for defense-in-depth
- Clean up TestDoRequest_429_RetryAfter_IntegerSeconds: remove dead
  code block and commented-out reasoning
- Fix import ordering: context before errors (goimports compliance)

github/client.go

@@ -27,6 +27,10 @@ const (
 	// maxErrorBodyBytes limits how much of an error response body we read
 	// to protect against malicious servers sending unbounded data.
 	maxErrorBodyBytes = 64 * 1024 // 64 KB
+
+	// maxResponseBodyBytes limits how much of a successful response body we read
+	// for defense-in-depth against servers returning excessively large payloads.
+	maxResponseBodyBytes = 10 * 1024 * 1024 // 10 MB
 )
 
 // APIError represents an HTTP error response from the GitHub API.
@@ -136,8 +140,11 @@ func (c *Client) SetRetryBackoff(backoff []time.Duration) {
 //
 // Returns (0, false) if the value cannot be parsed as either format.
 func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
+	value = strings.TrimSpace(value)
+
 	// Try integer seconds first (most common from GitHub).
-	if seconds, err := strconv.Atoi(value); err == nil && seconds > 0 {
+	// RFC 7231 allows delta-seconds of 0 to indicate immediate retry.
+	if seconds, err := strconv.Atoi(value); err == nil && seconds >= 0 {
 		return time.Duration(seconds) * time.Second, true
 	}
 
@@ -158,8 +165,10 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 // It respects the Retry-After header when present, supporting both integer
 // seconds and HTTP-date formats (capped at maxRetryAfter).
 func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
-	backoff := c.retryBackoff
+	var backoff []time.Duration
-	if backoff == nil {
+	if c.retryBackoff != nil {
+		backoff = append([]time.Duration(nil), c.retryBackoff...)
+	} else {
 		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
 	}
 
@@ -198,7 +207,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		}
 
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			body, err := io.ReadAll(resp.Body)
+			body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes))
 			resp.Body.Close()
 			if err != nil {
 				return nil, fmt.Errorf("read response body: %w", err)

github/client_test.go

@@ -1,8 +1,8 @@
 package github
 
 import (
-	"errors"
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -48,7 +48,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
-			w.Header().Set("Retry-After", "3")
+			w.Header().Set("Retry-After", "1")
 			w.WriteHeader(http.StatusTooManyRequests)
 			w.Write([]byte("rate limited"))
 			return
@@ -59,52 +59,9 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
 	defer srv.Close()
 
 	c := NewClient("tok", srv.URL)
-	// Use zero backoff so test doesn't wait — the Retry-After override only
-	// affects backoff[attempt] which is used on the NEXT iteration. Since
-	// we only have one retry, we set backoff[0] to 0 initially, then
-	// the 429 handler overrides it. To avoid waiting, we cancel quickly.
-	// Actually: the flow is attempt=0 gets 429, handler overrides backoff[0],
-	// then attempt=1 reads backoff[0]. So we need backoff[0] to be small after override.
-	// With Retry-After: 3, backoff[0] becomes 3s. Let's use context timeout.
-	// Better approach: just set backoff large enough and use very short timeout.
-	// Simplest: verify parsing works via parseRetryAfter unit tests and keep
-	// the integration test fast by not actually waiting.
-
-	// For integration: set backoff to 0 initially. The 429 handler will override
-	// backoff[0] to 3s. To avoid waiting 3s, we'll just verify it retried.
-	// Actually we need to accept the 3s wait OR use a different test strategy.
-
-	// Best approach: use a 1ms initial backoff that gets overridden, but we
-	// check correctness via the parseRetryAfter unit tests. For the integration
-	// test, use Retry-After: 0 edge case OR just test that retry happens.
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
-	// The handler sets Retry-After: 3, which will override backoff[0] to 3s.
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	// But since we start with backoff[0]=0, the first attempt runs immediately,
-	// then on 429 the code does: backoff[0] = 3s. The retry loop then uses
-	// backoff[attempt-1] = backoff[0] = 3s for the delay before attempt 1.
-	// To keep the test fast, let's just test a small value.
-	srv.Close()
-
-	// Recreate with small Retry-After
-	attempts = 0
-	srv2 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", "1")
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv2.Close()
-
-	c2 := NewClient("tok", srv2.URL)
-	c2.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c2.doGet(context.Background(), srv2.URL+"/test")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -320,9 +277,12 @@ func TestParseRetryAfter_IntegerSeconds(t *testing.T) {
 
 func TestParseRetryAfter_ZeroSeconds(t *testing.T) {
 	c := NewClient("tok", "")
-	_, ok := c.parseRetryAfter("0")
+	delay, ok := c.parseRetryAfter("0")
-	if ok {
+	if !ok {
-		t.Error("expected ok=false for zero seconds")
+		t.Fatal("expected ok=true for zero seconds (RFC 7231 allows immediate retry)")
+	}
+	if delay != 0 {
+		t.Errorf("delay = %v, want 0", delay)
 	}
 }