fix(github): address review findings - remove panic, validate at config time

- 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.

github/client.go

@@ -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.
@@ -178,24 +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
 }
 
 // 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.
 func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
-	const maxAttempts = 3
 	const maxRetryAfter = 120 * time.Second
 
+	// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
+	// Length must be maxRetryAttempts-1 (one entry per retry gap).
+	// SetRetryBackoff validates at configuration time; the default is always valid.
+	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
 	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}
+		backoff = make([]time.Duration, len(defaultBackoff))
+		copy(backoff, defaultBackoff)
 	}
 
 	// maxErrorBodyBytes limits how much of an error response body is stored.
@@ -215,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	}
 
 	var lastErr error
-	for attempt := 0; attempt < maxAttempts; attempt++ {
+	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
 		if attempt > 0 {
 			var delay time.Duration
 			if attempt-1 < len(backoff) {
@@ -255,6 +268,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			return nil, fmt.Errorf("do request: %w", err)
 		}
 
+		// Capture response metadata before handleResponse takes body ownership.
+		respStatus := resp.StatusCode
+		retryAfterHeader := resp.Header.Get("Retry-After")
+
 		body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		if done {
 			return body, err
@@ -262,10 +279,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		lastErr = err
 
 		// Retry on 429 rate limit
-		if resp.StatusCode == 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 := resp.Header.Get("Retry-After"); ra != "" {
+			if ra := retryAfterHeader; ra != "" {
 				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
 					delay := time.Duration(seconds) * time.Second
 					if delay > maxRetryAfter {
@@ -309,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

@@ -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 {
@@ -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")
@@ -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)
+	}
+}