From ae91c8aef53911ad8e178fd379cd58d84a6e5689 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 16:11:58 -0700 Subject: [PATCH] fix: address review findings from rounds 2834-2838 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Unexport RetryBackoff, add SetRetryBackoff method (#17286) - Rename http field to httpClient to avoid shadowing (#17289) - Group const blocks into single declaration (#17291) - Fix CheckRedirect to compare against previous hop, not first (#17302) - Strip auth header on protocol downgrade https→http (#17297) - Add maxPages safeguard to pagination loops (#17299, #17300) - Document mapCheckRunStatus unused second parameter (#17287, #17303) --- github/client.go | 47 ++++++++++++++++++++++++++----------------- github/client_test.go | 20 +++++++++--------- github/files_test.go | 4 ++-- github/pr.go | 17 +++++++++------- github/pr_test.go | 4 ++-- 5 files changed, 52 insertions(+), 40 deletions(-) diff --git a/github/client.go b/github/client.go index e0f5dfc..293cc17 100644 --- a/github/client.go +++ b/github/client.go @@ -14,11 +14,13 @@ import ( "time" ) -const defaultBaseURL = "https://api.github.com" -const userAgent = "review-bot/1.0" +const ( + defaultBaseURL = "https://api.github.com" + userAgent = "review-bot/1.0" -// maxResponseBytes limits successful response body reads to 10 MiB. -const maxResponseBytes = 10 * 1024 * 1024 + // maxResponseBytes limits successful response body reads to 10 MiB. + maxResponseBytes = 10 * 1024 * 1024 +) // APIError represents an HTTP error response from the GitHub API. // It carries the status code so callers can distinguish between @@ -68,12 +70,12 @@ func asAPIError(err error) (*APIError, bool) { type Client struct { baseURL string token string - http *http.Client + httpClient *http.Client - // RetryBackoff defines the delays between retry attempts for 429 responses. - // RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails). - // If nil, defaults to {1s, 2s}. Set to shorter durations in tests. - RetryBackoff []time.Duration + // retryBackoff defines the delays between retry attempts for 429 responses. + // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). + // If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff. + retryBackoff []time.Duration } // NewClient creates a new GitHub API client. @@ -86,16 +88,17 @@ func NewClient(token, baseURL string) *Client { return &Client{ baseURL: strings.TrimRight(baseURL, "/"), token: token, - http: &http.Client{ + httpClient: &http.Client{ Timeout: 30 * time.Second, CheckRedirect: func(req *http.Request, via []*http.Request) error { - // Prevent forwarding Authorization header to different hosts on redirect. - if len(via) > 0 && req.URL.Host != via[0].URL.Host { - req.Header.Del("Authorization") - } if len(via) >= 10 { return fmt.Errorf("stopped after 10 redirects") } + // Strip Authorization on cross-host redirect or protocol downgrade (https→http). + prev := via[len(via)-1] + if req.URL.Host != prev.URL.Host || (prev.URL.Scheme == "https" && req.URL.Scheme == "http") { + req.Header.Del("Authorization") + } return nil }, }, @@ -105,7 +108,13 @@ func NewClient(token, baseURL string) *Client { // SetHTTPClient sets the underlying HTTP client used for requests. // This is intended for testing to inject mock transports. func (c *Client) SetHTTPClient(hc *http.Client) { - c.http = hc + c.httpClient = hc +} + +// SetRetryBackoff configures the retry backoff durations for testing. +// In production the default {1s, 2s} applies. +func (c *Client) SetRetryBackoff(d []time.Duration) { + c.retryBackoff = d } // doRequest performs an HTTP request with retry on 429 rate limit responses. @@ -116,9 +125,9 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin const maxRetryAfter = 120 * time.Second var backoff []time.Duration - if c.RetryBackoff != nil { - backoff = make([]time.Duration, len(c.RetryBackoff)) - copy(backoff, c.RetryBackoff) + if c.retryBackoff != nil { + backoff = make([]time.Duration, len(c.retryBackoff)) + copy(backoff, c.retryBackoff) } else { backoff = []time.Duration{1 * time.Second, 2 * time.Second} } @@ -157,7 +166,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin req.Header.Set("Accept", "application/vnd.github+json") } - resp, err := c.http.Do(req) + resp, err := c.httpClient.Do(req) if err != nil { return nil, fmt.Errorf("do request: %w", err) } diff --git a/github/client_test.go b/github/client_test.go index 794df2f..d59edc7 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -81,7 +81,7 @@ func TestDoRequest_429Retry(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{10 * time.Millisecond, 10 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}) body, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { @@ -106,7 +106,7 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) _, err := c.doGet(context.Background(), srv.URL+"/test") if err == nil { @@ -205,7 +205,7 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) // Use short backoff; Retry-After should override - c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) start := time.Now() body, err := c.doGet(context.Background(), srv.URL+"/test") @@ -246,19 +246,19 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) _, err := c.doGet(context.Background(), srv.URL+"/test") if err != nil { t.Fatalf("unexpected error: %v", err) } - // Verify the original RetryBackoff slice was not mutated - if c.RetryBackoff[0] != 1*time.Millisecond { - t.Errorf("RetryBackoff[0] was mutated: got %v, want 1ms", c.RetryBackoff[0]) + // Verify the original retryBackoff slice was not mutated + if c.retryBackoff[0] != 1*time.Millisecond { + t.Errorf("retryBackoff[0] was mutated: got %v, want 1ms", c.retryBackoff[0]) } - if c.RetryBackoff[1] != 1*time.Millisecond { - t.Errorf("RetryBackoff[1] was mutated: got %v, want 1ms", c.RetryBackoff[1]) + if c.retryBackoff[1] != 1*time.Millisecond { + t.Errorf("retryBackoff[1] was mutated: got %v, want 1ms", c.retryBackoff[1]) } } @@ -310,7 +310,7 @@ func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) { func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) { // Verify the CheckRedirect function is configured c := NewClient("secret-token", "https://api.github.com") - if c.http.CheckRedirect == nil { + if c.httpClient.CheckRedirect == nil { t.Fatal("expected CheckRedirect to be set") } } diff --git a/github/files_test.go b/github/files_test.go index 3c6d889..2c8d80f 100644 --- a/github/files_test.go +++ b/github/files_test.go @@ -109,7 +109,7 @@ func TestGetFileContent_429Retry(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "") if err != nil { @@ -227,7 +227,7 @@ func TestListContents_429Retry(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) entries, err := c.ListContents(context.Background(), "owner", "repo", ".") if err != nil { diff --git a/github/pr.go b/github/pr.go index 0d1046f..c26f061 100644 --- a/github/pr.go +++ b/github/pr.go @@ -84,13 +84,16 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num return string(body), nil } +// maxPages is the upper bound on pagination loops to prevent unbounded iteration +// in case the server returns a full page indefinitely. +const maxPages = 100 + // GetPullRequestFiles fetches the list of files changed in a PR. // Paginates through all pages (100 per page) to collect all files. func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) { var allFiles []vcs.ChangedFile - page := 1 - for { + for page := 1; page <= maxPages; page++ { reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page) body, err := c.doGet(ctx, reqURL) @@ -114,7 +117,6 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu if len(files) < 100 { break } - page++ } return allFiles, nil @@ -175,8 +177,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) } // Fetch check runs (paginated) - checkPage := 1 - for { + for checkPage := 1; checkPage <= maxPages; checkPage++ { checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage) checkBody, err := c.doGet(ctx, checkURL) @@ -198,13 +199,15 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) if len(checkResp.CheckRuns) < 100 { break } - checkPage++ } return result, nil } -// mapCheckRunStatus maps a check run conclusion+status to a vcs.CommitStatus status string. +// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string. +// The second parameter (check run status field, e.g. "completed", "in_progress") is +// unused because conclusion alone determines the mapped state: nil conclusion means +// the run is still in progress (pending), regardless of the status field value. func mapCheckRunStatus(conclusion *string, _ string) string { if conclusion == nil { // Still running or queued diff --git a/github/pr_test.go b/github/pr_test.go index 78366b7..7dbd2ad 100644 --- a/github/pr_test.go +++ b/github/pr_test.go @@ -112,7 +112,7 @@ func TestGetPullRequest_429Retry(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1) if err != nil { @@ -447,7 +447,7 @@ func TestGetFileContentAtRef_429Retry(t *testing.T) { c := NewClient("token", srv.URL) c.SetHTTPClient(srv.Client()) - c.RetryBackoff = []time.Duration{1 * time.Millisecond} + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond}) content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main") if err != nil {