feat(github): implement PRReader + FileReader client (#80) #93
@@ -9,6 +9,7 @@ import (
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strconv"
|
||||
"strings"
|
||||
|
|
||||
"time"
|
||||
@@ -132,7 +133,11 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client {
|
||||
|
||||
gpt-review-bot
commented
[NIT] Calling timer.Stop() after the timer has already fired is unnecessary. You could simplify the delay logic with select { case <-time.After(delay): case <-ctx.Done(): } or only Stop in the ctx.Done branch. **[NIT]** Calling timer.Stop() after the timer has already fired is unnecessary. You could simplify the delay logic with select { case <-time.After(delay): case <-ctx.Done(): } or only Stop in the ctx.Done branch.
|
||||
// SetHTTPClient sets the underlying HTTP client used for requests.
|
||||
// This is intended for testing to inject mock transports.
|
||||
// Passing nil will restore the default client with a 30s timeout.
|
||||
|
gpt-review-bot
commented
[MINOR] Retry-After is parsed only as delta-seconds; per RFC 7231 it may also be an HTTP-date. Consider falling back to parsing an HTTP-date when Atoi fails to honor server guidance more robustly. **[MINOR]** Retry-After is parsed only as delta-seconds; per RFC 7231 it may also be an HTTP-date. Consider falling back to parsing an HTTP-date when Atoi fails to honor server guidance more robustly.
|
||||
func (c *Client) SetHTTPClient(hc *http.Client) {
|
||||
|
gpt-review-bot
commented
[MINOR] Non-HTTPS rejection checks strings.HasPrefix(url, "https://"). This is case-sensitive and string-based; safer to parse with net/url and check URL.Scheme case-insensitively (e.g., u.Scheme == "https"). **[MINOR]** Non-HTTPS rejection checks strings.HasPrefix(url, "https://"). This is case-sensitive and string-based; safer to parse with net/url and check URL.Scheme case-insensitively (e.g., u.Scheme == "https").
|
||||
if hc == nil {
|
||||
hc = &http.Client{Timeout: 30 * time.Second}
|
||||
}
|
||||
c.httpClient = hc
|
||||
}
|
||||
|
||||
sonnet-review-bot
commented
[NIT] The **[NIT]** The `doRequest` method signature uses `url` as a parameter name, which shadows the imported `net/url` package. While `net/url` is not imported in `client.go` (it's only in `pr.go` and `files.go`), this naming is a minor style concern since `url` as a local variable name is idiomatic but shadows the package name if ever imported. Not a bug here, just worth noting.
|
||||
@@ -145,7 +150,7 @@ func (c *Client) SetRetryBackoff(d []time.Duration) {
|
||||
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
||||
|
sonnet-review-bot
commented
[MINOR] The CheckRedirect lambda is duplicated verbatim in both NewClient and SetHTTPClient(nil). Extract it to a package-level function (e.g., **[MINOR]** The CheckRedirect lambda is duplicated verbatim in both NewClient and SetHTTPClient(nil). Extract it to a package-level function (e.g., `defaultCheckRedirect`) to eliminate the duplication and ensure both code paths stay in sync when the policy changes.
|
||||
// It respects the Retry-After header when present (capped at maxRetryAfter).
|
||||
|
gpt-review-bot
commented
[NIT] Retry-After parsing only handles delta-seconds via Atoi. RFC 7231 allows an HTTP-date format; optionally support parsing HTTP-date to fully respect server guidance. **[NIT]** Retry-After parsing only handles delta-seconds via Atoi. RFC 7231 allows an HTTP-date format; optionally support parsing HTTP-date to fully respect server guidance.
|
||||
// Transport errors (network failures, context cancellation) are not retried.
|
||||
func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
|
||||
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
|
||||
|
sonnet-review-bot
commented
[MINOR] When **[MINOR]** When `c.http.Do(req)` returns an error (network failure, context cancellation), the function returns immediately without retrying. For transient network errors, a retry could be valuable. The current behavior is reasonable for the stated scope (only retry on 429), but the comment says 'It respects the Retry-After header when present' without mentioning the no-retry-on-transport-error behavior. This is a documentation gap rather than a bug.
|
||||
const maxAttempts = 3
|
||||
const maxRetryAfter = 120 * time.Second
|
||||
|
sonnet-review-bot
commented
[MINOR] The security check **[MINOR]** The security check `if !c.allowInsecureHTTP && req.URL.Scheme != "https"` is performed inside the retry loop, meaning it will fail on every retry attempt rather than being checked once before the loop starts. Since the URL doesn't change between retries, this is wasteful and the error message is slightly misleading (it mentions `req.URL.Host` but the real issue is the scheme). Moving the check before the retry loop or to `NewClient` would be cleaner.
|
||||
|
||||
@@ -160,8 +165,14 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
const maxErrorBodyBytes = 64 * 1024
|
||||
|
||||
// Reject non-HTTPS URLs early since the URL is immutable across retries.
|
||||
if c.token != "" && !c.allowInsecureHTTP && !strings.HasPrefix(url, "https://") {
|
||||
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", url)
|
||||
if c.token != "" && !c.allowInsecureHTTP {
|
||||
|
sonnet-review-bot
commented
[MINOR] After a successful response is read, **[MINOR]** After a successful response is read, `resp.Body.Close()` is called directly after `io.ReadAll`. If `io.ReadAll` returns an error (e.g. partial read), the body is still closed via the subsequent line, which is fine. However, the pattern is slightly inconsistent with the error path below it — consider using `defer resp.Body.Close()` paired with a drain before close on the error path for symmetry. This is purely stylistic; the current approach is correct.
|
||||
parsed, err := url.Parse(reqURL)
|
||||
if err != nil {
|
||||
|
gpt-review-bot
commented
[NIT] Authorization header uses the 'Bearer' scheme. GitHub PATs typically use the 'token' scheme; while 'Bearer' may work for some token types, consider aligning with GitHub’s documented scheme or making it configurable. **[NIT]** Authorization header uses the 'Bearer' scheme. GitHub PATs typically use the 'token' scheme; while 'Bearer' may work for some token types, consider aligning with GitHub’s documented scheme or making it configurable.
|
||||
return nil, fmt.Errorf("parse request URL: %w", err)
|
||||
}
|
||||
if !strings.EqualFold(parsed.Scheme, "https") {
|
||||
|
sonnet-review-bot
commented
[MINOR] The comment **[MINOR]** The comment `// Timer already fired; Stop() is a no-op here.` is misleading. `time.NewTimer` creates a timer that fires after the duration; when the select case `<-timer.C` fires, the timer has already expired so `Stop()` does return false, but the comment conflates 'fired' with 'Stop is no-op'. More importantly, there's no `defer timer.Stop()` before the select — while technically fine here because the goroutine either drains the channel or returns, the pattern is fragile and deviates from the canonical `defer timer.Stop()` idiom. The standard pattern is: `timer := time.NewTimer(delay); defer timer.Stop(); select { case <-timer.C: ... case <-ctx.Done(): ... }`. The current code avoids the leak correctly but via an atypical path.
|
||||
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
|
||||
}
|
||||
}
|
||||
|
gpt-review-bot
commented
[MINOR] Successful responses are truncated to maxResponseBytes without signaling truncation. If truncation is possible for some endpoints (e.g., very large diffs), consider documenting this behavior or returning an explicit error when the limit is reached. **[MINOR]** Successful responses are truncated to maxResponseBytes without signaling truncation. If truncation is possible for some endpoints (e.g., very large diffs), consider documenting this behavior or returning an explicit error when the limit is reached.
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] Retry-After parsing only supports integer seconds; per RFC 7231, servers may send an HTTP-date. Consider supporting both seconds and HTTP-date formats for broader interoperability. **[MINOR]** Retry-After parsing only supports integer seconds; per RFC 7231, servers may send an HTTP-date. Consider supporting both seconds and HTTP-date formats for broader interoperability.
|
||||
var lastErr error
|
||||
@@ -183,7 +194,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
}
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] The timer drain comment '// Timer already fired; Stop() is a no-op here.' is slightly misleading. When the timer fires and we receive from **[MINOR]** The timer drain comment '// Timer already fired; Stop() is a no-op here.' is slightly misleading. When the timer fires and we receive from `timer.C`, `Stop()` returns false but calling it is still harmless — however the comment may cause confusion since we never call Stop() in this branch. More importantly, after receiving from `timer.C`, Stop() genuinely IS a no-op (the timer has already fired), but the code never calls Stop() here at all, which is correct. The comment is about something that isn't happening. This is fine but the comment could be clearer or removed.
sonnet-review-bot
commented
[NIT] The **[NIT]** The `doRequest` method signature uses a positional `accept string` parameter rather than a functional option or an options struct. For internal use only (called from `doGet` and `GetPullRequestDiff`) this is fine, but if the API surface grows (e.g. needing custom headers), this approach will require signature changes. No action needed now, but a comment noting it's intentionally internal would be helpful.
|
||||
req, err := http.NewRequestWithContext(ctx, method, url, nil)
|
||||
req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("create request: %w", err)
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `doRequest` method has `accept string` as the last parameter. Per Go idiom (style.md receiver naming / api-conventions), named parameters for Accept headers are fine, but the parameter name `accept` conflicts with the common Go pattern of using `accept` only in request context. Not a correctness issue — purely style.
|
||||
}
|
||||
@@ -208,6 +219,9 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("read response body: %w", err)
|
||||
|
sonnet-review-bot
commented
[NIT] The timer pattern **[NIT]** The timer pattern `timer.Stop() // no-op after fire; kept for symmetry` in the `case <-timer.C` branch is slightly misleading. `Stop()` after a channel receive is indeed a no-op since the channel has already fired, but the comment could be clearer: `timer.Stop()` after `<-timer.C` does nothing and could simply be omitted. This is a readability nit, not a correctness issue.
|
||||
}
|
||||
[MINOR] APIError stores up to 64KB of error body in the Body field. While Error() truncates to 200 bytes, exposing the full Body increases the risk of sensitive data leakage if callers log or propagate it. Consider further limiting or redacting Body contents. **[MINOR]** APIError stores up to 64KB of error body in the Body field. While Error() truncates to 200 bytes, exposing the full Body increases the risk of sensitive data leakage if callers log or propagate it. Consider further limiting or redacting Body contents.
|
||||
if int64(len(body)) >= maxResponseBytes {
|
||||
return nil, fmt.Errorf("response body exceeded %d bytes (truncated)", maxResponseBytes)
|
||||
|
gpt-review-bot
commented
[MINOR] Retry-After parsing handles only integer seconds. Per RFC 7231, Retry-After may be an HTTP-date as well. Supporting HTTP-date parsing would improve compliance with servers that return a date rather than seconds. **[MINOR]** Retry-After parsing handles only integer seconds. Per RFC 7231, Retry-After may be an HTTP-date as well. Supporting HTTP-date parsing would improve compliance with servers that return a date rather than seconds.
|
||||
}
|
||||
return body, nil
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The timer leak on the happy path is benign but slightly untidy. When **[MINOR]** The timer leak on the happy path is benign but slightly untidy. When `<-timer.C` fires, `timer.Stop()` returns false and the channel is already drained, so it's a no-op — the comment acknowledges this. Consider using `timer.Reset` pattern or noting it more explicitly, but this is purely cosmetic.
|
||||
|
||||
@@ -241,6 +255,6 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
}
|
||||
|
||||
// doGet is a convenience wrapper for GET requests with the default Accept header.
|
||||
func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
|
||||
return c.doRequest(ctx, http.MethodGet, url, "")
|
||||
func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
|
||||
return c.doRequest(ctx, http.MethodGet, reqURL, "")
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `int64` cast in `if int64(len(body)) >= maxResponseBytes` is unnecessary since `maxResponseBytes` is an untyped constant and `len()` returns `int`. Both sides of the comparison are `int`-typed. The cast is harmless but adds noise.
|
||||
}
|
||||
|
||||
@@ -282,8 +282,7 @@ func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
|
||||
}
|
||||
|
||||
func TestDoRequest_LimitsResponseBody(t *testing.T) {
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `TestDoRequest_LimitsResponseBody` tests a constant value rather than actual behavior. The comment acknowledges this limitation. This is acceptable as a documentation-style test, but it adds no real safety guarantee — if `maxResponseBytes` is set correctly but the `io.LimitReader` call is removed, the test would still pass. Consider removing it or replacing with a test that actually sends a response exceeding the limit.
|
||||
// Verify that response body reading is actually bounded by maxResponseBytes.
|
||||
// Use a small custom limit to avoid allocating 10 MiB in tests.
|
||||
// Verify that oversized responses return an error rather than silently truncating.
|
||||
bigBody := strings.Repeat("x", maxResponseBytes+1024)
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
@@ -293,13 +292,12 @@ func TestDoRequest_LimitsResponseBody(t *testing.T) {
|
||||
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `TestDoRequest_LimitsResponseBody` doesn't actually test the limit behavior — it only checks the constant value. The comment acknowledges this. This is a weak test; consider removing it or replacing it with an actual test using a small limit (e.g., setting `maxResponseBytes` to a small value via a test helper, or accepting that this particular boundary isn't testable without refactoring).
|
||||
if err == nil {
|
||||
t.Fatal("expected error for oversized response body")
|
||||
}
|
||||
// LimitReader should cap the body at maxResponseBytes
|
||||
if len(body) > maxResponseBytes {
|
||||
t.Errorf("expected body <= %d bytes, got %d", maxResponseBytes, len(body))
|
||||
if !strings.Contains(err.Error(), "exceeded") {
|
||||
t.Errorf("expected truncation error, got: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -384,3 +382,14 @@ func TestDoRequest_AllowsHTTPWithInsecureOption(t *testing.T) {
|
||||
t.Errorf("unexpected body: %s", body)
|
||||
}
|
||||
}
|
||||
|
||||
func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
|
||||
c := NewClient("token", "https://api.github.com")
|
||||
c.SetHTTPClient(nil)
|
||||
if c.httpClient == nil {
|
||||
t.Fatal("expected non-nil httpClient after SetHTTPClient(nil)")
|
||||
}
|
||||
if c.httpClient.Timeout != 30*time.Second {
|
||||
t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout)
|
||||
}
|
||||
}
|
||||
|
||||
[NIT] Package comment mentions "review submission" but this PR doesn't include review endpoints. Consider updating the comment or adding a TODO/ref to avoid misleading users.