From 6b7f3f6924e384f36d38e1c638672c02e784ed91 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 12:48:02 -0700 Subject: [PATCH] fix: address NIT findings from sonnet review (#113) - Fix AllowInsecureHTTP doc comment: 'silently ignored' -> 'ignored' (it logs a warning) - Remove unnecessary nil guard in redactURL (assigning nil to nil is a no-op) - Add clarifying comment about acceptable double URL parse in doRequest --- github/client.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/github/client.go b/github/client.go index 718935a..41ecea8 100644 --- a/github/client.go +++ b/github/client.go @@ -152,7 +152,7 @@ type clientConfig struct { // AllowInsecureHTTP permits sending credentials over plaintext HTTP connections. // In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1 -// environment variable. Without the env var set, the option is silently ignored +// environment variable. Without the env var set, the option is ignored // and a warning is logged. // // For tests, use AllowInsecureHTTPForTest (defined in export_test.go) which bypasses the env gate. @@ -267,9 +267,8 @@ func redactURL(rawURL string) string { if err != nil { return "" } - if u.User != nil { - u.User = nil - } + u.User = nil + if u.RawQuery != "" { u.RawQuery = "" } @@ -281,6 +280,9 @@ func redactURL(rawURL string) string { // It respects the Retry-After header when present, supporting both integer // seconds and HTTP-date formats (capped at maxRetryAfter). func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { + // NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it + // again internally). Acceptable cost: URL parsing is cheap and threading the + // parsed *url.URL through would complicate the interface for negligible gain. if !c.allowInsecureHTTP { parsed, err := url.Parse(reqURL) if err != nil {