fix: address self-review findings

- Remove dead code: findOwnReview (replaced by findAllOwnReviews)
- Check SetRetryBackoff return value in doJSONRequest tests
- Extract doWithRetry shared helper to eliminate ~100 lines of
  duplicated 429-retry/backoff/Retry-After logic between doRequest
  and doJSONRequest
- Fix import order: context before encoding/json (goimports)
- Add slog.Warn when ListReviews hits maxReviewPages limit

cmd/review-bot/main.go

@@ -891,22 +891,6 @@ func extractSentinelName(body string) string {
 	return name
 }
 
-// findOwnReview locates the most recent non-superseded review matching the sentinel.
-func findOwnReview(reviews []vcs.Review, sentinel string) *vcs.Review {
-	var best *vcs.Review
-	for i := range reviews {
-		if !strings.Contains(reviews[i].Body, sentinel) {
-			continue
-		}
-		if strings.Contains(reviews[i].Body, "~~Original review~~") {
-			continue
-		}
-		if best == nil || reviews[i].ID > best.ID {
-			best = &reviews[i]
-		}
-	}
-	return best
-}
 
 // findAllOwnReviews returns all non-superseded reviews matching the sentinel.
 func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review {

cmd/review-bot/main_test.go

@@ -210,91 +210,6 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
 	}
 }
 
-func TestFindOwnReview(t *testing.T) {
-	tests := []struct {
-		name     string
-		reviews  []vcs.Review
-		sentinel string
-		wantID   int64
-		wantNil  bool
-	}{
-		{
-			name:     "no reviews",
-			reviews:  nil,
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantNil:  true,
-		},
-		{
-			name: "found by sentinel",
-			reviews: []vcs.Review{
-				makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantID:   42,
-		},
-		{
-			name: "wrong sentinel",
-			reviews: []vcs.Review{
-				makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantNil:  true,
-		},
-		{
-			name: "multiple reviews, returns first match",
-			reviews: []vcs.Review{
-				makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
-				makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantID:   20,
-		},
-		{
-			name: "skips superseded review",
-			reviews: []vcs.Review{
-				makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
-				makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantID:   20,
-		},
-		{
-			name: "only superseded reviews exist",
-			reviews: []vcs.Review{
-				makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantNil:  true,
-		},
-		{
-			name: "picks highest ID among matches",
-			reviews: []vcs.Review{
-				makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
-				makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
-			},
-			sentinel: "<!-- review-bot:sonnet -->",
-			wantID:   50,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := findOwnReview(tc.reviews, tc.sentinel)
-			if tc.wantNil {
-				if got != nil {
-					t.Errorf("findOwnReview() = %v, want nil", got)
-				}
-			} else {
-				if got == nil {
-					t.Fatal("findOwnReview() = nil, want non-nil")
-				}
-				if got.ID != tc.wantID {
-					t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID)
-				}
-			}
-		})
-	}
-}
 
 func TestHasSharedToken(t *testing.T) {
 	tests := []struct {

github/client.go

@@ -5,8 +5,8 @@ package github
 
 import (
 	"bytes"
-	"encoding/json"
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -194,12 +194,19 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
 	return nil
 }
 
-// doRequest performs an HTTP request with retry on 429 rate limit responses.
+// doWithRetry performs an HTTP request with retry on 429 rate limit responses.
+// It delegates request construction to buildReq, which is called on each attempt
+// to produce a fresh *http.Request (allowing body re-reads for POST/PUT).
 // 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) {
+func (c *Client) doWithRetry(ctx context.Context, reqURL string, buildReq func() (*http.Request, error)) ([]byte, error) {
 	const maxRetryAfter = 120 * time.Second
 
+	// 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
+	// log APIError.Body directly. Error() further truncates to 200 bytes.
+	const maxErrorBodyBytes = 4 * 1024
+
 	// 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.
@@ -213,11 +220,6 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		copy(backoff, defaultBackoff)
 	}
 
-	// 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
-	// log APIError.Body directly. Error() further truncates to 200 bytes.
-	const maxErrorBodyBytes = 4 * 1024
-
 	// Reject non-HTTPS URLs early since the URL is immutable across retries.
 	if c.token != "" && !c.allowInsecureHTTP {
 		parsed, err := url.Parse(reqURL)
@@ -248,22 +250,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			}
 		}
 
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
+		req, err := buildReq()
 		if err != nil {
 			return nil, fmt.Errorf("create request: %w", err)
 		}
-		if c.token != "" {
-			// Bearer is the OAuth2 standard and is accepted by GitHub for both
-			// classic PATs and fine-grained tokens. The alternative "token" scheme
-			// is GitHub-specific and offers no additional compatibility.
-			req.Header.Set("Authorization", "Bearer "+c.token)
-		}
-		req.Header.Set("User-Agent", userAgent)
-		if accept != "" {
-			req.Header.Set("Accept", accept)
-		} else {
-			req.Header.Set("Accept", "application/vnd.github+json")
-		}
 
 		resp, err := c.httpClient.Do(req)
 		if err != nil {
@@ -274,11 +264,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		respStatus := resp.StatusCode
 		retryAfterHeader := resp.Header.Get("Retry-After")
 
-		body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
+		body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		if done {
-			return body, err
+			return body, handleErr
 		}
-		lastErr = err
+		lastErr = handleErr
 
 		// Retry on 429 rate limit
 		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
@@ -316,6 +306,32 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	return nil, lastErr
 }
 
+// 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) {
+	buildReq := func() (*http.Request, error) {
+		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
+		if err != nil {
+			return nil, err
+		}
+		if c.token != "" {
+			// Bearer is the OAuth2 standard and is accepted by GitHub for both
+			// classic PATs and fine-grained tokens. The alternative "token" scheme
+			// is GitHub-specific and offers no additional compatibility.
+			req.Header.Set("Authorization", "Bearer "+c.token)
+		}
+		req.Header.Set("User-Agent", userAgent)
+		if accept != "" {
+			req.Header.Set("Accept", accept)
+		} else {
+			req.Header.Set("Accept", "application/vnd.github+json")
+		}
+		return req, nil
+	}
+	return c.doWithRetry(ctx, reqURL, buildReq)
+}
+
 // handleResponse reads and closes the response body, returning the result.
 // It uses defer to ensure the body is always closed regardless of code path.
 // Returns (body, done, err) where done=true means the caller should return immediately.
@@ -346,63 +362,19 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 }
 
 // doJSONRequest performs an HTTP request with a JSON body and returns the response body.
-// It handles HTTPS validation, authentication, response reading, and retries on HTTP 429
-// rate limit responses (matching the retry behavior of doRequest).
+// It delegates retry/backoff/429 handling to doWithRetry.
 // This is a general-purpose helper used by any method that needs to send JSON payloads
 // (e.g. PostReview, DismissReview).
 func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
-	const (
-		maxErrorBodyBytes = 4 * 1024
-		maxRetryAfter     = 120 * time.Second
-	)
-
 	jsonBody, err := json.Marshal(payload)
 	if err != nil {
 		return nil, fmt.Errorf("marshal request body: %w", err)
 	}
 
-	if c.token != "" && !c.allowInsecureHTTP {
-		parsed, err := url.Parse(reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if !strings.EqualFold(parsed.Scheme, "https") {
-			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
-		}
-	}
-
-	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
-	var backoff []time.Duration
-	if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
-		backoff = make([]time.Duration, len(c.retryBackoff))
-		copy(backoff, c.retryBackoff)
-	} else {
-		backoff = make([]time.Duration, len(defaultBackoff))
-		copy(backoff, defaultBackoff)
-	}
-
-	var lastErr error
-	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
-		if attempt > 0 {
-			var delay time.Duration
-			if attempt-1 < len(backoff) {
-				delay = backoff[attempt-1]
-			}
-			if delay > 0 {
-				timer := time.NewTimer(delay)
-				select {
-				case <-timer.C:
-					timer.Stop()
-				case <-ctx.Done():
-					timer.Stop()
-					return nil, ctx.Err()
-				}
-			}
-		}
-
+	buildReq := func() (*http.Request, error) {
 		req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody))
 		if err != nil {
-			return nil, fmt.Errorf("create request: %w", err)
+			return nil, err
 		}
 		if c.token != "" {
 			req.Header.Set("Authorization", "Bearer "+c.token)
@@ -410,50 +382,7 @@ func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, paylo
 		req.Header.Set("User-Agent", userAgent)
 		req.Header.Set("Accept", "application/vnd.github+json")
 		req.Header.Set("Content-Type", "application/json")
-
-		resp, err := c.httpClient.Do(req)
-		if err != nil {
-			return nil, fmt.Errorf("do request: %w", err)
-		}
-
-		respStatus := resp.StatusCode
-		retryAfterHeader := resp.Header.Get("Retry-After")
-
-		body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
-		if done {
-			return body, handleErr
-		}
-		lastErr = handleErr
-
-		// Retry on 429 rate limit
-		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
-			if ra := retryAfterHeader; ra != "" {
-				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
-					delay := time.Duration(seconds) * time.Second
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				} else if retryAt, err := http.ParseTime(ra); err == nil {
-					delay := time.Until(retryAt)
-					if delay < 0 {
-						delay = 0
-					}
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				}
-			}
-			continue
-		}
-
-		return nil, lastErr
+		return req, nil
 	}
-
-	return nil, lastErr
+	return c.doWithRetry(ctx, reqURL, buildReq)
 }

github/client_test.go

@@ -609,7 +609,9 @@ func TestDoJSONRequest_429Retry(t *testing.T) {
 	defer ts.Close()
 
 	c := NewClient("token", ts.URL, AllowInsecureHTTP())
-	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)
+	}
 
 	body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
 	if err != nil {
@@ -631,7 +633,9 @@ func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) {
 	defer ts.Close()
 
 	c := NewClient("token", ts.URL, AllowInsecureHTTP())
-	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.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
 	if err == nil {

github/reviews.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"log/slog"
 	"net/http"
 	"net/url"
 
@@ -139,6 +140,11 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
 		if len(reviews) < reviewsPerPage {
 			break
 		}
+		if page == maxReviewPages {
+			slog.Warn("ListReviews hit page limit; results may be truncated",
+				"owner", owner, "repo", repo, "pr", number,
+				"maxPages", maxReviewPages, "reviewsFetched", len(allReviews))
+		}
 	}
 
 	return allReviews, nil