From 3d1260d3b28595f5960b645731c8ed9f203314cc Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 21:26:39 -0700
Subject: [PATCH] fix(github): clarify response ownership and validate backoff
 length
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 github/client.go | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/github/client.go b/github/client.go
index 7f5c7f9..be72d19 100644
--- a/github/client.go
+++ b/github/client.go
@@ -190,6 +190,9 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	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 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 {
 		backoff = make([]time.Duration, len(c.retryBackoff))
@@ -197,6 +200,9 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	} 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))
+	}
 
 	// maxErrorBodyBytes limits how much of an error response body is stored.
 	// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
@@ -255,6 +261,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 +272,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 < maxAttempts-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 {