diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 4e0487b..79a3af0 100644 --- a/cmd/review-bot/main.go +++ b/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 { diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 67071c2..b97c58e 100644 --- a/cmd/review-bot/main_test.go +++ b/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: "", - wantNil: true, - }, - { - name: "found by sentinel", - reviews: []vcs.Review{ - makeReview(42, "bot", "APPROVED", false, "review body\n"), - }, - sentinel: "", - wantID: 42, - }, - { - name: "wrong sentinel", - reviews: []vcs.Review{ - makeReview(42, "bot", "APPROVED", false, "body\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "multiple reviews, returns first match", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "old\n"), - makeReview(20, "bot", "APPROVED", false, "new\n"), - }, - sentinel: "", - wantID: 20, - }, - { - name: "skips superseded review", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n"), - makeReview(20, "bot", "APPROVED", false, "fresh review\n"), - }, - sentinel: "", - wantID: 20, - }, - { - name: "only superseded reviews exist", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "picks highest ID among matches", - reviews: []vcs.Review{ - makeReview(50, "bot", "APPROVED", false, "v1\n"), - makeReview(30, "bot", "APPROVED", false, "v0\n"), - }, - sentinel: "", - 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 { diff --git a/github/client.go b/github/client.go index 345f06c..9342e5f 100644 --- a/github/client.go +++ b/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) } diff --git a/github/client_test.go b/github/client_test.go index 7701e92..8bf02f0 100644 --- a/github/client_test.go +++ b/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 { diff --git a/github/reviews.go b/github/reviews.go index b6aa3b4..158ef0d 100644 --- a/github/reviews.go +++ b/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