fix: address review feedback on PR #102

- Separate maxPages into maxFilesPages and maxCheckRunPages constants
  for clarity (sonnet MINOR #1)
- Add parallel to CheckRunConclusions subtests (sonnet MINOR #2)
- Add TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed test
  covering check-runs 500 after statuses succeed (sonnet MINOR #2)
- Expand mapCheckRunStatus doc comment with full mapping rules including
  cancelled/skipped/neutral rationale and unknown value behavior
  (sonnet MINOR #3, gpt MINOR #1)
- Expand GetPullRequest doc comment to mention error types returned
  (sonnet NIT #4)
- Add inline comment on Description field clarifying it holds raw
  conclusion value (gpt NIT #3)

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)
+	}
+}