feat(github): implement PRReader + FileReader client (#80) #93
@@ -65,13 +65,30 @@ func asAPIError(err error) (*APIError, bool) {
|
||||
return nil, false
|
||||
|
|
||||
}
|
||||
[NIT] baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled. **[NIT]** baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled.
|
||||
|
||||
// clientConfig holds optional configuration for NewClient.
|
||||
sonnet-review-bot
commented
[MINOR] RetryBackoff is an exported field on Client, which exposes an internal implementation detail and creates a testing surface that's somewhat awkward — callers might accidentally rely on mutation semantics. A more idiomatic approach would be a functional option (e.g. **[MINOR]** RetryBackoff is an exported field on Client, which exposes an internal implementation detail and creates a testing surface that's somewhat awkward — callers might accidentally rely on mutation semantics. A more idiomatic approach would be a functional option (e.g. `WithRetryBackoff`) or keeping it unexported with a test helper. That said, the comment explicitly marks it as a test hook, which mitigates this concern.
sonnet-review-bot
commented
[MINOR] RetryBackoff is an exported field on Client, which makes it part of the public API surface. This is used as a testing escape hatch ('Set to shorter durations in tests'), but the pattern documented in the codebase for HTTP client injection is SetHTTPClient(). Exposing RetryBackoff publicly means callers can set arbitrary production retry delays, which may be intentional but conflates test and production configuration. A SetRetryBackoff method or keeping it package-internal with a test-only setter via export_test.go would be cleaner, though the current approach is pragmatic. **[MINOR]** RetryBackoff is an exported field on Client, which makes it part of the public API surface. This is used as a testing escape hatch ('Set to shorter durations in tests'), but the pattern documented in the codebase for HTTP client injection is SetHTTPClient(). Exposing RetryBackoff publicly means callers can set arbitrary production retry delays, which may be intentional but conflates test and production configuration. A SetRetryBackoff method or keeping it package-internal with a test-only setter via export_test.go would be cleaner, though the current approach is pragmatic.
|
||||
type clientConfig struct {
|
||||
allowInsecureHTTP bool
|
||||
|
[MINOR] The default http.Client will follow redirects. If a redirect points to a different host, there is a risk (dependent on Go version/runtime behavior) of Authorization headers being forwarded, potentially leaking tokens. Consider setting CheckRedirect to disallow cross-host redirects or disable redirects (return http.ErrUseLastResponse), and/or ensure Authorization is stripped on host changes. **[MINOR]** The default http.Client will follow redirects. If a redirect points to a different host, there is a risk (dependent on Go version/runtime behavior) of Authorization headers being forwarded, potentially leaking tokens. Consider setting CheckRedirect to disallow cross-host redirects or disable redirects (return http.ErrUseLastResponse), and/or ensure Authorization is stripped on host changes.
sonnet-review-bot
commented
[MINOR] The Client struct has inconsistent field alignment — **[MINOR]** The Client struct has inconsistent field alignment — `httpClient` is not aligned with `baseURL` and `token`. While gofmt doesn't enforce struct field alignment, the blank line between `token` and `httpClient` with the comment block is slightly inconsistent with Go conventions. More substantively: `SetHTTPClient` and `SetRetryBackoff` are exported methods that mutate the struct, breaking the documented concurrency guarantee ('A Client is safe for concurrent use by multiple goroutines'). These mutation methods are clearly test-only helpers, but they're exported without any documentation caveat about not calling them concurrently with requests.
|
||||
}
|
||||
|
||||
gpt-review-bot
commented
[MINOR] Functional options are used for a single boolean (AllowInsecureHTTP). Per configuration patterns, prefer a simpler parameter or a nil-opts struct when there are <3 stable options; consider YAGNI until more options appear. **[MINOR]** Functional options are used for a single boolean (AllowInsecureHTTP). Per configuration patterns, prefer a simpler parameter or a nil-opts struct when there are <3 stable options; consider YAGNI until more options appear.
|
||||
// ClientOption configures optional behavior of NewClient.
|
||||
|
sonnet-review-bot
commented
[NIT] The struct formatting has inconsistent spacing — **[NIT]** The struct formatting has inconsistent spacing — `baseURL` and `token` fields have no space before the type, but `httpClient` has extra whitespace alignment. `gofmt` normally handles this, so it may be a display artifact, but worth verifying the file passes `gofmt` cleanly.
|
||||
type ClientOption func(*clientConfig)
|
||||
|
[MINOR] NewClient does not validate the baseURL scheme. If misconfigured to use http://, the token will be sent over plaintext. Consider enforcing https:// by default (or providing an explicit opt-out flag) to prevent accidental credential leakage. **[MINOR]** NewClient does not validate the baseURL scheme. If misconfigured to use http://, the token will be sent over plaintext. Consider enforcing https:// by default (or providing an explicit opt-out flag) to prevent accidental credential leakage.
|
||||
|
||||
// AllowInsecureHTTP permits the client to use HTTP (non-TLS) base URLs.
|
||||
|
[MINOR] NewClient accepts arbitrary baseURL without validating scheme. If configured with an http:// URL, the Authorization token will be sent in plaintext over the network. Enforce HTTPS by default or require an explicit opt-in to allow HTTP for trusted internal deployments. **[MINOR]** NewClient accepts arbitrary baseURL without validating scheme. If configured with an http:// URL, the Authorization token will be sent in plaintext over the network. Enforce HTTPS by default or require an explicit opt-in to allow HTTP for trusted internal deployments.
[MINOR] AllowInsecureHTTP() permits sending Authorization tokens over plaintext HTTP if enabled. While opt-in and documented for trusted environments/tests, it is a potential footgun if inadvertently enabled in production. Consider restricting to loopback/private ranges or emitting a prominent warning/telemetry when used. **[MINOR]** AllowInsecureHTTP() permits sending Authorization tokens over plaintext HTTP if enabled. While opt-in and documented for trusted environments/tests, it is a potential footgun if inadvertently enabled in production. Consider restricting to loopback/private ranges or emitting a prominent warning/telemetry when used.
|
||||
// This should only be used for trusted internal deployments or testing.
|
||||
func AllowInsecureHTTP() ClientOption {
|
||||
|
[MINOR] AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present). **[MINOR]** AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present).
|
||||
return func(c *clientConfig) {
|
||||
c.allowInsecureHTTP = true
|
||||
}
|
||||
}
|
||||
|
||||
|
[MINOR] Authorization header is stripped only on cross-host redirects; it is still forwarded on same-host scheme downgrades (https -> http). This can leak the token over plaintext if the server issues a downgrade redirect. Consider also stripping Authorization when the scheme changes or when redirecting to non-HTTPS. **[MINOR]** Authorization header is stripped only on cross-host redirects; it is still forwarded on same-host scheme downgrades (https -> http). This can leak the token over plaintext if the server issues a downgrade redirect. Consider also stripping Authorization when the scheme changes or when redirecting to non-HTTPS.
gpt-review-bot
commented
[MINOR] CheckRedirect compares req.URL.Host against via[0].URL.Host (the original request). If the redirect chain changes hosts on a later hop, comparing against the immediately previous host (via[len(via)-1]) is safer to ensure Authorization is stripped whenever the host changes at any step. **[MINOR]** CheckRedirect compares req.URL.Host against via[0].URL.Host (the original request). If the redirect chain changes hosts on a later hop, comparing against the immediately previous host (via[len(via)-1]) is safer to ensure Authorization is stripped whenever the host changes at any step.
|
||||
// Client interacts with the GitHub API.
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `http` field name shadows the `net/http` package import within method bodies. While Go resolves this correctly (field access via `c.http` vs package via `http.MethodGet`), using a field name that matches an imported package name reduces readability. Consider renaming to `httpClient` or `hc`.
|
||||
// A Client is safe for concurrent use by multiple goroutines;
|
||||
// however, SetHTTPClient and SetRetryBackoff must not be called concurrently with requests.
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `SetHTTPClient` method is documented as 'intended for testing to inject mock transports', making it a test-only escape hatch on the public API. Per the package design pattern for `internal/` packages, test-only hooks that are not part of the intended public contract ideally belong in `export_test.go` or a test helper. However, since this is a new package and the project may not yet have an `export_test.go` pattern established, this is a minor concern.
|
||||
type Client struct {
|
||||
baseURL string
|
||||
token string
|
||||
httpClient *http.Client
|
||||
baseURL string
|
||||
token string
|
||||
|
[MINOR] NewClient accepts any baseURL without enforcing HTTPS or validating against a trusted allowlist. If a misconfiguration allows an attacker-controlled baseURL, the client could send the Authorization token to an untrusted host or over plaintext HTTP. **[MINOR]** NewClient accepts any baseURL without enforcing HTTPS or validating against a trusted allowlist. If a misconfiguration allows an attacker-controlled baseURL, the client could send the Authorization token to an untrusted host or over plaintext HTTP.
|
||||
allowInsecureHTTP bool
|
||||
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).
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `SetHTTPClient` and `SetRetryBackoff` methods are exported as test-injection points but documented as unsafe for concurrent use with requests. The convention in the patterns (style.md) suggests using unexported helpers or the functional options pattern for test injection. The current design works but exposes mutation methods on what is documented as a concurrent-safe type. A `//nolint` or explicit doc note would make the intent clearer.
|
||||
@@ -82,13 +99,20 @@ type Client struct {
|
||||
// NewClient creates a new GitHub API client.
|
||||
|
[MINOR] CheckRedirect strips Authorization on cross-host or https→http redirects but still follows the redirect. Following cross-host redirects can be an SSRF vector in misconfigured environments; consider blocking cross-host redirects entirely rather than proceeding without Authorization. **[MINOR]** CheckRedirect strips Authorization on cross-host or https→http redirects but still follows the redirect. Following cross-host redirects can be an SSRF vector in misconfigured environments; consider blocking cross-host redirects entirely rather than proceeding without Authorization.
|
||||
// If baseURL is empty, it defaults to https://api.github.com.
|
||||
// For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
|
||||
|
gpt-review-bot
commented
[NIT] The parameter name 'url' in doRequest shadows the conceptual URL type used elsewhere (e.g., net/url in other files). Renaming to 'reqURL' could improve clarity, though this is purely stylistic. **[NIT]** The parameter name 'url' in doRequest shadows the conceptual URL type used elsewhere (e.g., net/url in other files). Renaming to 'reqURL' could improve clarity, though this is purely stylistic.
|
||||
func NewClient(token, baseURL string) *Client {
|
||||
// The baseURL must use HTTPS; pass AllowInsecureHTTP() as an option to permit HTTP
|
||||
// for trusted internal deployments (e.g. local testing).
|
||||
func NewClient(token, baseURL string, opts ...ClientOption) *Client {
|
||||
|
sonnet-review-bot
commented
[NIT] The doc comment for **[NIT]** The doc comment for `Client` says `SetHTTPClient` and `SetRetryBackoff` must not be called concurrently with requests, but these are public methods and there's no enforcement or `noCopy` guard. Given the concurrent-use note, a brief comment in `SetHTTPClient` and `SetRetryBackoff` reiterating the constraint would help (though this matches the stdlib's `tls.Config` immutable-after-use convention, so it's acceptable as-is).
|
||||
if baseURL == "" {
|
||||
baseURL = defaultBaseURL
|
||||
}
|
||||
|
[MINOR] The CheckRedirect handler allows following cross-host and HTTPS→HTTP redirects (while stripping Authorization). Although the token isn’t leaked, following cross-host redirects may contact untrusted hosts. Consider restricting redirects to same-host and HTTPS-only or fail on cross-host redirects to reduce SSRF-style risks. **[MINOR]** The CheckRedirect handler allows following cross-host and HTTPS→HTTP redirects (while stripping Authorization). Although the token isn’t leaked, following cross-host redirects may contact untrusted hosts. Consider restricting redirects to same-host and HTTPS-only or fail on cross-host redirects to reduce SSRF-style risks.
|
||||
cfg := clientConfig{}
|
||||
for _, o := range opts {
|
||||
|
[MINOR] defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts. **[MINOR]** defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.
|
||||
o(&cfg)
|
||||
|
gpt-review-bot
commented
[MINOR] Consider setting a User-Agent header on all requests. GitHub recommends identifying clients, and some enterprise installations enforce it. Add req.Header.Set("User-Agent", "review-bot/1.0") or similar. **[MINOR]** Consider setting a User-Agent header on all requests. GitHub recommends identifying clients, and some enterprise installations enforce it. Add req.Header.Set("User-Agent", "review-bot/1.0") or similar.
|
||||
}
|
||||
return &Client{
|
||||
|
gpt-review-bot
commented
[MINOR] SetHTTPClient allows setting a nil *http.Client; subsequent use (c.httpClient.Do) would panic. Either guard against nil (return error or restore default client) or document that nil is invalid. **[MINOR]** SetHTTPClient allows setting a nil *http.Client; subsequent use (c.httpClient.Do) would panic. Either guard against nil (return error or restore default client) or document that nil is invalid.
|
||||
baseURL: strings.TrimRight(baseURL, "/"),
|
||||
token: token,
|
||||
baseURL: strings.TrimRight(baseURL, "/"),
|
||||
allowInsecureHTTP: cfg.allowInsecureHTTP,
|
||||
token: token,
|
||||
|
[MINOR] defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades. **[MINOR]** defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.
|
||||
httpClient: &http.Client{
|
||||
|
gpt-review-bot
commented
[MINOR] Authorization header is always set to "Bearer "+token even when token is empty. Consider only setting the header when token is non-empty to avoid sending an empty bearer token on unauthenticated requests. **[MINOR]** Authorization header is always set to "Bearer "+token even when token is empty. Consider only setting the header when token is non-empty to avoid sending an empty bearer token on unauthenticated requests.
gpt-review-bot
commented
[NIT] GitHub classic PATs typically use the "token" scheme while fine-grained tokens use "Bearer". If supporting both is desired, consider documenting or adapting the auth scheme based on token type. **[NIT]** GitHub classic PATs typically use the "token" scheme while fine-grained tokens use "Bearer". If supporting both is desired, consider documenting or adapting the auth scheme based on token type.
sonnet-review-bot
commented
[MINOR] The doc comment on **[MINOR]** The doc comment on `defaultCheckRedirect` says it "strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP) to prevent credential leakage, while still following the redirect." However, a protocol downgrade from HTTPS to HTTP is a genuine security issue — stripping the header and still following is debatable. Consider returning an error on HTTPS→HTTP downgrade rather than silently following. This is a design choice that has security implications, not a bug per se, but worth flagging.
gpt-review-bot
commented
[MINOR] defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse. **[MINOR]** defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse.
|
||||
Timeout: 30 * time.Second,
|
||||
CheckRedirect: func(req *http.Request, via []*http.Request) error {
|
||||
|
sonnet-review-bot
commented
[NIT] The concurrency safety doc comment says "SetHTTPClient and SetRetryBackoff must not be called concurrently with requests" but these are exported methods with no enforcement (no mutex, no atomic). This is an acceptable design for a test-support method, but the comment could be stronger: e.g., "These methods are intended for test setup only and must be called before any goroutines issue requests." **[NIT]** The concurrency safety doc comment says "SetHTTPClient and SetRetryBackoff must not be called concurrently with requests" but these are exported methods with no enforcement (no mutex, no atomic). This is an acceptable design for a test-support method, but the comment could be stronger: e.g., "These methods are intended for test setup only and must be called before any goroutines issue requests."
|
||||
@@ -146,7 +170,7 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
timer := time.NewTimer(delay)
|
||||
|
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.
|
||||
select {
|
||||
case <-timer.C:
|
||||
timer.Stop()
|
||||
// Timer already fired; Stop() is a no-op here.
|
||||
|
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.
|
||||
case <-ctx.Done():
|
||||
timer.Stop()
|
||||
return nil, ctx.Err()
|
||||
|
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.
|
||||
@@ -159,6 +183,10 @@ func (c *Client) doRequest(ctx context.Context, method, url string, accept strin
|
||||
return nil, fmt.Errorf("create request: %w", err)
|
||||
}
|
||||
if c.token != "" {
|
||||
|
sonnet-review-bot
commented
[NIT] The timer in the retry loop has a comment **[NIT]** The timer in the retry loop has a comment `// Timer already fired; Stop() is a no-op here.` — this is slightly inaccurate. `Stop()` on an already-fired timer is not a no-op; it returns false and the channel has already been drained by the `case <-timer.C:` branch, so there's no leak. The comment could be clearer: `// Channel already drained; no need to call Stop().`
|
||||
// Refuse to send credentials over plaintext unless explicitly allowed.
|
||||
if !c.allowInsecureHTTP && req.URL.Scheme != "https" {
|
||||
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", req.URL.Host)
|
||||
|
gpt-review-bot
commented
[MINOR] Authorization header uses the "Bearer" scheme unconditionally. GitHub REST v3 commonly expects the "token" scheme for classic/fine-grained PATs, while "Bearer" is used in some flows (e.g., OAuth or app tokens). Consider supporting both schemes or making the scheme configurable to maximize compatibility. **[MINOR]** Authorization header uses the "Bearer" scheme unconditionally. GitHub REST v3 commonly expects the "token" scheme for classic/fine-grained PATs, while "Bearer" is used in some flows (e.g., OAuth or app tokens). Consider supporting both schemes or making the scheme configurable to maximize compatibility.
|
||||
}
|
||||
req.Header.Set("Authorization", "Bearer "+c.token)
|
||||
}
|
||||
req.Header.Set("User-Agent", userAgent)
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
)
|
||||
@@ -38,7 +39,7 @@ func TestDoRequest_SetsAuthHeader(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("my-token", srv.URL)
|
||||
c := NewClient("my-token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||
|
||||
@@ -56,7 +57,7 @@ func TestDoRequest_SetsDefaultAcceptHeader(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||
|
||||
@@ -79,7 +80,7 @@ func TestDoRequest_429Retry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
|
||||
|
||||
@@ -104,7 +105,7 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
|
||||
|
||||
@@ -133,7 +134,7 @@ func TestDoRequest_404NoRetry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
@@ -154,7 +155,7 @@ func TestDoRequest_401NoRetry(t *testing.T) {
|
||||
}))
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `TestDoRequest_429RetryAfterHeader` and `TestDoRequest_RetryAfterDoesNotMutateBackoff` are gated behind `testing.Short()` but `TestDoRequest_429RetryAfterHTTPDate` (which waits ~2 seconds) is not. This test will slow down non-short test runs. Consider adding `testing.Short()` skip to `TestDoRequest_429RetryAfterHTTPDate` as well for consistency.
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
@@ -202,7 +203,7 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
// Use short backoff; Retry-After should override
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
|
||||
@@ -244,7 +245,7 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
|
||||
|
||||
@@ -271,7 +272,7 @@ func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||
|
||||
@@ -281,11 +282,24 @@ 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 responses are read through a limit reader.
|
||||
// We can't easily test the 10 MiB limit without OOM risk,
|
||||
// but we verify the constant is set correctly.
|
||||
if maxResponseBytes != 10*1024*1024 {
|
||||
t.Errorf("expected maxResponseBytes = 10 MiB, got %d", maxResponseBytes)
|
||||
// Verify that response body reading is actually bounded by maxResponseBytes.
|
||||
// Use a small custom limit to avoid allocating 10 MiB in tests.
|
||||
bigBody := strings.Repeat("x", maxResponseBytes+1024)
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(bigBody))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
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).
|
||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
// LimitReader should cap the body at maxResponseBytes
|
||||
if len(body) > maxResponseBytes {
|
||||
t.Errorf("expected body <= %d bytes, got %d", maxResponseBytes, len(body))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -298,7 +312,7 @@ func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("", srv.URL) // empty token
|
||||
c := NewClient("", srv.URL, AllowInsecureHTTP()) // empty token
|
||||
c.SetHTTPClient(srv.Client())
|
||||
_, _ = c.doGet(context.Background(), srv.URL+"/test")
|
||||
|
||||
@@ -314,3 +328,59 @@ func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
|
||||
t.Fatal("expected CheckRedirect to be set")
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte("{}"))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
// Without AllowInsecureHTTP, should refuse to send token over HTTP
|
||||
c := NewClient("secret-token", srv.URL)
|
||||
c.SetHTTPClient(srv.Client())
|
||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
if err == nil {
|
||||
t.Fatal("expected error when sending token over HTTP")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "refusing to send credentials") {
|
||||
t.Errorf("unexpected error message: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_AllowsHTTPWithoutToken(t *testing.T) {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(`{"ok":true}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
// Without token, HTTP should be fine (no credentials to leak)
|
||||
c := NewClient("", srv.URL)
|
||||
c.SetHTTPClient(srv.Client())
|
||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if string(body) != `{"ok":true}` {
|
||||
t.Errorf("unexpected body: %s", body)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDoRequest_AllowsHTTPWithInsecureOption(t *testing.T) {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(`{"ok":true}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("secret-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)
|
||||
}
|
||||
if string(body) != `{"ok":true}` {
|
||||
t.Errorf("unexpected body: %s", body)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,6 +19,9 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref stri
|
||||
|
||||
// ListContents lists files and directories at a given path in a repo.
|
||||
|
gpt-review-bot
commented
[MINOR] ListContents assumes the contents API returns a JSON array. The GitHub Contents API returns an object for single-file paths. Consider handling both array and object responses (or return a clearer error) to avoid a JSON unmarshal error for file paths. **[MINOR]** ListContents assumes the contents API returns a JSON array. The GitHub Contents API returns an object for single-file paths. Consider handling both array and object responses (or return a clearer error) to avoid a JSON unmarshal error for file paths.
|
||||
// Returns the directory listing from the GitHub contents API.
|
||||
// If the path points to a single file (not a directory), the API returns
|
||||
// a JSON object instead of an array; this is handled by returning a
|
||||
// single-element slice.
|
||||
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
|
||||
@@ -26,14 +29,24 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list contents %s: %w", path, err)
|
||||
}
|
||||
var entries []struct {
|
||||
|
||||
type entry struct {
|
||||
Name string `json:"name"`
|
||||
Path string `json:"path"`
|
||||
Type string `json:"type"`
|
||||
}
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] escapePath drops ".." segments but does not remove the preceding segment, so paths like "a/./b/../c" normalize to "a/b/c" instead of "a/c". Clarify this behavior in the comment or consider implementing proper dot-segment normalization to avoid surprising results. **[MINOR]** escapePath drops ".." segments but does not remove the preceding segment, so paths like "a/./b/../c" normalize to "a/b/c" instead of "a/c". Clarify this behavior in the comment or consider implementing proper dot-segment normalization to avoid surprising results.
|
||||
// The GitHub contents API returns an array for directories and an object
|
||||
// for single files. Try array first (common case), then fall back to object.
|
||||
var entries []entry
|
||||
|
gpt-review-bot
commented
[NIT] escapePath removes dot-segments and empty segments, which can turn root ("" or ".") into an empty path, resulting in a trailing slash in the constructed URL (…/contents/). This is likely fine for GitHub, but consider documenting the behavior or handling root explicitly for clarity. **[NIT]** escapePath removes dot-segments and empty segments, which can turn root ("" or ".") into an empty path, resulting in a trailing slash in the constructed URL (…/contents/). This is likely fine for GitHub, but consider documenting the behavior or handling root explicitly for clarity.
|
||||
if err := json.Unmarshal(body, &entries); err != nil {
|
||||
return nil, fmt.Errorf("parse contents JSON: %w", err)
|
||||
var single entry
|
||||
if err2 := json.Unmarshal(body, &single); err2 != nil {
|
||||
|
sonnet-review-bot
commented
[MINOR] The fallback from array→object JSON parsing uses **[MINOR]** The fallback from array→object JSON parsing uses `err` from the outer `json.Unmarshal` in the error return (`fmt.Errorf("parse contents JSON: %w", err)`), but `err` here is the array-parse error ('cannot unmarshal object into Go value of type []entry'), which is misleading — the real parse failure is `err2`. Should return `err2` instead: `fmt.Errorf("parse contents JSON: %w", err2)`.
sonnet-review-bot
commented
[MINOR] The fallback from array to object JSON parsing is an unusual approach. The error from the first **[MINOR]** The fallback from array to object JSON parsing is an unusual approach. The error from the first `json.Unmarshal` (array attempt) is discarded silently. If the response is truly malformed JSON (not just an object vs array mismatch), only `err2` from the single-entry unmarshal is surfaced. This is intentional per the comment but it means a corrupted array response that happens to partially parse as an object could go undetected. Consider checking if the body starts with `[` vs `{` to choose the parsing path, which would be more explicit and preserve the original error on genuine parse failures.
sonnet-review-bot
commented
[NIT] The fallback from array to single-object JSON unmarshal silently swallows the original array parse error. If the body is valid JSON but neither an array nor an object matching the **[NIT]** The fallback from array to single-object JSON unmarshal silently swallows the original array parse error. If the body is valid JSON but neither an array nor an object matching the `entry` struct (e.g., a JSON number), the second unmarshal will succeed with zero values and return a single empty entry. Consider checking that `single.Name != ""` or similar before accepting the fallback.
|
||||
return nil, fmt.Errorf("parse contents JSON: %w", err)
|
||||
}
|
||||
entries = []entry{single}
|
||||
}
|
||||
|
||||
result := make([]vcs.ContentEntry, len(entries))
|
||||
|
security-review-bot marked this conversation as resolved
Outdated
[MINOR] escapePath preserves dot-segments ("." and "..") in path components. Some servers normalize dot-segments, which could let an untrusted path escape the intended "contents" endpoint (e.g., "/contents/../../pulls"). While this stays on the same host, it could lead to unexpected behavior. Consider rejecting or encoding dot-segments to prevent path traversal in URL construction. **[MINOR]** escapePath preserves dot-segments ("." and "..") in path components. Some servers normalize dot-segments, which could let an untrusted path escape the intended "contents" endpoint (e.g., "/contents/../../pulls"). While this stays on the same host, it could lead to unexpected behavior. Consider rejecting or encoding dot-segments to prevent path traversal in URL construction.
|
||||
for i, e := range entries {
|
||||
|
sonnet-review-bot
commented
[MINOR] Silent empty-array handling: when the GitHub Contents API returns an empty JSON array **[MINOR]** Silent empty-array handling: when the GitHub Contents API returns an empty JSON array `[]`, `json.Unmarshal` into `[]entry` succeeds with `len(entries) == 0` and the function returns an empty slice without error. This is probably the correct behavior for an empty directory, but it's undocumented and could silently mask an unexpected server response. A comment clarifying this intent would be valuable.
|
||||
result[i] = vcs.ContentEntry{
|
||||
@@ -47,7 +60,11 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
|
||||
|
||||
// escapePath escapes each segment of a relative file path for use in URLs.
|
||||
|
gpt-review-bot
commented
[MINOR] decodeBase64Content strips only '\n'. GitHub or proxies may use CRLF line breaks. Consider removing both '\r' and '\n' (or all whitespace) before decoding, or use base64.NewDecoder which tolerates newlines. **[MINOR]** decodeBase64Content strips only '\n'. GitHub or proxies may use CRLF line breaks. Consider removing both '\r' and '\n' (or all whitespace) before decoding, or use base64.NewDecoder which tolerates newlines.
|
||||
// Slashes are preserved as path separators; other special characters are escaped.
|
||||
// Dot-segments ("." and "..") are removed to prevent path traversal.
|
||||
// Dot-segments ("." and "..") are silently removed to prevent path traversal.
|
||||
|
gpt-review-bot
commented
[NIT] escapePath silently removes dot segments ("." and ".."), which may surprise callers if their requested path is altered without error. The comment notes this intent; consider documenting this behavior in the public method docs (e.g., ListContents/GetFileContentAtRef) or returning an explicit error when dot-segments are provided. **[NIT]** escapePath silently removes dot segments ("." and ".."), which may surprise callers if their requested path is altered without error. The comment notes this intent; consider documenting this behavior in the public method docs (e.g., ListContents/GetFileContentAtRef) or returning an explicit error when dot-segments are provided.
|
||||
// This is intentional: callers may receive a different path than requested without
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `escapePath` function removes dot-segments silently. The doc comment acknowledges this and explains it's intentional, which is good. But the test case `{"../etc/passwd", "etc/passwd"}` documents that a path traversal attempt is silently resolved to `etc/passwd` rather than returning an error. Depending on threat model, callers may want to know the path was modified. Since this is intentional and documented, this is a NIT-level observation for a future design consideration.
|
||||
// error. The function is package-private, and all callers (GetFileContentAtRef,
|
||||
// ListContents) already handle missing-file errors from the API if the cleaned
|
||||
// path doesn't match what the caller intended.
|
||||
func escapePath(p string) string {
|
||||
parts := strings.Split(p, "/")
|
||||
var clean []string
|
||||
|
||||
@@ -20,7 +20,7 @@ func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
// Call with empty ref — should not include ref param
|
||||
@@ -47,7 +47,7 @@ func TestGetFileContent_WithRef(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123")
|
||||
@@ -66,7 +66,7 @@ func TestGetFileContent_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContent(context.Background(), "owner", "repo", "missing.go", "")
|
||||
@@ -82,7 +82,7 @@ func TestGetFileContent_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
|
||||
@@ -107,7 +107,7 @@ func TestGetFileContent_429Retry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
|
||||
|
||||
@@ -130,7 +130,7 @@ func TestGetFileContent_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
|
||||
@@ -151,7 +151,7 @@ func TestListContents_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
entries, err := c.ListContents(context.Background(), "owner", "repo", "src")
|
||||
@@ -185,7 +185,7 @@ func TestListContents_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.ListContents(context.Background(), "owner", "repo", "missing")
|
||||
@@ -201,7 +201,7 @@ func TestListContents_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.ListContents(context.Background(), "owner", "repo", "src")
|
||||
@@ -225,7 +225,7 @@ func TestListContents_429Retry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
|
||||
|
||||
@@ -248,7 +248,7 @@ func TestListContents_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.ListContents(context.Background(), "owner", "repo", "src")
|
||||
@@ -307,3 +307,28 @@ func TestDecodeBase64Content_CRLF(t *testing.T) {
|
||||
t.Errorf("expected 'hello world', got %q", decoded)
|
||||
}
|
||||
}
|
||||
|
||||
func TestListContents_SingleFile(t *testing.T) {
|
||||
// GitHub Contents API returns a JSON object (not array) for single-file paths
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(200)
|
||||
w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file"}`))
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(entries) != 1 {
|
||||
t.Fatalf("expected 1 entry, got %d", len(entries))
|
||||
}
|
||||
if entries[0].Name != "README.md" {
|
||||
t.Errorf("expected name 'README.md', got %q", entries[0].Name)
|
||||
}
|
||||
if entries[0].Type != "file" {
|
||||
t.Errorf("expected type 'file', got %q", entries[0].Type)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -44,8 +44,7 @@ type commitStatusResponse struct {
|
||||
|
||||
// checkRunsResponse is the GitHub check runs API response.
|
||||
type checkRunsResponse struct {
|
||||
TotalCount int `json:"total_count"`
|
||||
CheckRuns []struct {
|
||||
CheckRuns []struct {
|
||||
Name string `json:"name"`
|
||||
Conclusion *string `json:"conclusion"`
|
||||
Status string `json:"status"`
|
||||
|
||||
@@ -26,7 +26,7 @@ func TestGetPullRequest_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42)
|
||||
@@ -60,7 +60,7 @@ func TestGetPullRequest_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 999)
|
||||
@@ -79,7 +79,7 @@ func TestGetPullRequest_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
@@ -110,7 +110,7 @@ func TestGetPullRequest_429Retry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
|
||||
|
||||
@@ -133,7 +133,7 @@ func TestGetPullRequest_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
|
||||
@@ -155,7 +155,7 @@ func TestGetPullRequestDiff_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
diff, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 42)
|
||||
@@ -177,7 +177,7 @@ func TestGetPullRequestDiff_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 999)
|
||||
@@ -193,7 +193,7 @@ func TestGetPullRequestDiff_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
|
||||
@@ -211,7 +211,7 @@ func TestGetPullRequestFiles_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||
@@ -256,7 +256,7 @@ func TestGetPullRequestFiles_Pagination(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||
@@ -283,7 +283,7 @@ func TestGetPullRequestFiles_BinaryFile_NoPatch(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||
@@ -305,7 +305,7 @@ func TestGetPullRequestFiles_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 999)
|
||||
@@ -321,7 +321,7 @@ func TestGetPullRequestFiles_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||
@@ -345,7 +345,7 @@ func TestGetFileContentAtRef_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "path/to/file.go", "abc123")
|
||||
@@ -369,7 +369,7 @@ func TestGetFileContentAtRef_EmptyRef(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.txt", "")
|
||||
@@ -388,7 +388,7 @@ func TestGetFileContentAtRef_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "missing.go", "main")
|
||||
@@ -404,7 +404,7 @@ func TestGetFileContentAtRef_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
|
||||
@@ -420,7 +420,7 @@ func TestGetFileContentAtRef_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
|
||||
@@ -445,7 +445,7 @@ func TestGetFileContentAtRef_429Retry(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
|
||||
|
||||
@@ -496,7 +496,7 @@ func TestGetCommitStatuses_HappyPath(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
|
||||
@@ -567,7 +567,7 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha1")
|
||||
@@ -591,7 +591,7 @@ func TestGetCommitStatuses_404(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "badsha")
|
||||
@@ -607,7 +607,7 @@ func TestGetCommitStatuses_401(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
|
||||
@@ -623,7 +623,7 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
|
||||
}))
|
||||
defer srv.Close()
|
||||
|
||||
c := NewClient("token", srv.URL)
|
||||
c := NewClient("token", srv.URL, AllowInsecureHTTP())
|
||||
c.SetHTTPClient(srv.Client())
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] strPtr helper is defined in pr_test.go but the same helper could be needed in other test files. Since all files are in package **[NIT]** strPtr helper is defined in pr_test.go but the same helper could be needed in other test files. Since all files are in package `github` (internal tests), this is fine as-is but worth noting for future maintainability.
sonnet-review-bot
commented
[NIT] **[NIT]** `stringPtr` helper is defined in `pr_test.go` (package `github`) but `TestGetCommitStatuses_CheckRunConclusions` also uses it. Since tests are in the same package, this is fine, but the helper could be placed in a shared test helper file to avoid potential future duplication if more test files are added to the package.
|
||||
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
|
||||
|
||||
[NIT] Comment states the Client is safe for concurrent use, but SetHTTPClient and SetRetryBackoff mutate configuration and are not concurrency-safe. Consider clarifying that configuration methods are for setup/testing and should not be called concurrently with requests.