From 528d29b63d04db9f633544712c7390002a0e4d52 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:54:37 -0700 Subject: [PATCH] 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 | 16 ----- cmd/review-bot/main_test.go | 85 -------------------------- github/client.go | 117 +++--------------------------------- github/client_test.go | 8 ++- 4 files changed, 14 insertions(+), 212 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index db2ab71..940ee7e 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -900,22 +900,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 094d8fd..370bc52 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 bc302a4..8d41ee7 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" @@ -215,6 +215,11 @@ type requestOptions struct { func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]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. @@ -228,11 +233,6 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts 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) @@ -387,114 +387,13 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r } // 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 doRequestWithBody. // 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() - } - } - } - - req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } - if c.token != "" { - req.Header.Set("Authorization", "Bearer "+c.token) - } - 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 nil, lastErr + return c.doRequestWithBody(ctx, method, reqURL, jsonBody) } 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 {