rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

fix: address self-review findings

Browse Source 
- 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
This commit is contained in:
claw
2026-05-13 03:54:37 -07:00
parent c81698efb1
commit ce1d9760d9
5 changed files with 57 additions and 219 deletions
Download Patch File Download Diff File Expand all files Collapse all files
github/client.go
+45 -116
View File
@@ -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)
}