fix(github): address sonnet review feedback on PR #113

- hasHTTPSScheme: use strings.EqualFold instead of ToLower+HasPrefix
- slog.Warn: move hint into structured attribute for idiomatic usage
- client_test.go: fix blank line formatting between test functions
- clientConfig.testBypass: strengthen comment to reference export_test.go

.gitea/actions/review/action.yml

@@ -124,38 +124,14 @@ runs:
         else
           VERSION="${{ inputs.version }}"
         fi
-
-        # Detect OS and architecture for platform-specific binary download
-        OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
-        case "$OS_RAW" in
-          linux)  OS="linux" ;;
-          darwin) OS="darwin" ;;
-          *)
-            echo "Error: unsupported OS: $(uname -s)" >&2
-            exit 1
-            ;;
-        esac
-
-        RAW_ARCH=$(uname -m)
-        case "$RAW_ARCH" in
-          x86_64)           ARCH="amd64" ;;
-          aarch64 | arm64)  ARCH="arm64" ;;
-          *)
-            echo "Error: unsupported architecture: $RAW_ARCH" >&2
-            exit 1
-            ;;
-        esac
-
         echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
-        echo "os=${OS}" >> "$GITHUB_OUTPUT"
-        echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
 
     - name: Cache review-bot binary
       id: cache
       uses: actions/cache@v4
       with:
         path: ${{ runner.temp }}/review-bot
-        key: review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-${{ steps.version.outputs.version }}
+        key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
 
     - name: Install review-bot
       if: steps.cache.outputs.cache-hit != 'true'
@@ -164,7 +140,7 @@ runs:
         GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
         REPO="${{ inputs.repo || 'rodin/review-bot' }}"
         VERSION="${{ steps.version.outputs.version }}"
-        BINARY="review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}"
+        BINARY="review-bot-linux-amd64"
 
         curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
           -o "${{ runner.temp }}/review-bot"
@@ -173,13 +149,8 @@ runs:
 
         # Verify SHA-256 checksum
         cd "${{ runner.temp }}"
-        EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
-        # sha256sum (GNU) is not available on macOS; use shasum -a 256 on darwin.
-        if [ "${{ steps.version.outputs.os }}" = "darwin" ]; then
-          ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')
-        else
-          ACTUAL=$(sha256sum review-bot | awk '{print $1}')
-        fi
+        EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
+        ACTUAL=$(sha256sum review-bot | awk '{print $1}')
 
         if [ -z "$EXPECTED" ]; then
           echo "Error: no checksum found for ${BINARY}" >&2
@@ -193,7 +164,7 @@ runs:
         fi
 
         chmod +x "${{ runner.temp }}/review-bot"
-        echo "Installed review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }} ${VERSION} (checksum verified)"
+        echo "Installed review-bot ${VERSION} (checksum verified)"
 
     - name: Run review
       shell: bash

cmd/review-bot/integration_test.go

@@ -130,7 +130,7 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
 	// Post a test review
 	sentinel := "<!-- review-bot:integration-test -->"
 	testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, "", nil)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil)
 	if err != nil {
 		t.Fatalf("PostReview: %v", err)
 	}

cmd/review-bot/main.go

@@ -444,7 +444,7 @@ func main() {
 
 	// POST new review
 	slog.Info("posting review", "event", event, "pr", prNumber)
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
 	if err != nil {
 		slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
 		os.Exit(1)

gitea/client.go

@@ -262,22 +262,18 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
 }
 
 // PostReview submits a review to a PR and returns the created review.
-// event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
-// commitID anchors the review to a specific commit SHA. If empty, Gitea
-// defaults to the current PR head.
+// event should be "APPROVED" or "REQUEST_CHANGES".
 // comments are optional inline comments attached to specific lines.
-func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
+func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
 	payload := struct {
 		Body     string          `json:"body"`
 		Event    string          `json:"event"`
-		CommitID string          `json:"commit_id,omitempty"`
 		Comments []ReviewComment `json:"comments,omitempty"`
 	}{
 		Body:     body,
 		Event:    event,
-		CommitID: commitID,
 		Comments: comments,
 	}
 

gitea/client_test.go

@@ -117,9 +117,8 @@ func TestPostReview(t *testing.T) {
 		}
 
 		var payload struct {
-			Body     string `json:"body"`
-			Event    string `json:"event"`
-			CommitID string `json:"commit_id"`
+			Body  string `json:"body"`
+			Event string `json:"event"`
 		}
 		if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
 			t.Fatalf("failed to decode payload: %v", err)
@@ -130,16 +129,14 @@ func TestPostReview(t *testing.T) {
 		if payload.Event != "APPROVED" {
 			t.Errorf("expected event %q, got %q", "APPROVED", payload.Event)
 		}
-		if payload.CommitID != "abc123def" {
-			t.Errorf("expected commit_id %q, got %q", "abc123def", payload.CommitID)
-		}
+
 		w.WriteHeader(http.StatusOK)
 		w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
 	}))
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
+	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -186,35 +183,12 @@ func TestPostReview_Non200(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
 	if err == nil {
 		t.Fatal("expected error for 403, got nil")
 	}
 }
 
-func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		body, _ := io.ReadAll(r.Body)
-		var raw map[string]interface{}
-		if err := json.Unmarshal(body, &raw); err != nil {
-			t.Fatalf("failed to decode payload: %v", err)
-		}
-		if _, exists := raw["commit_id"]; exists {
-			t.Errorf("expected commit_id to be omitted from payload when empty, but it was present")
-		}
-
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte(`{"id":200,"user":{"login":"bot"},"state":"APPROVED","stale":false}`))
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "ok", "", nil)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-}
-
 func TestGetFileContent(t *testing.T) {
 	expected := "# Conventions\n- Be nice\n"
 
@@ -971,6 +945,8 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
 		t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
 	}
 }
+
+
 // mockTransport is a test helper that returns errors for the first N calls,
 // then delegates to a real server.
 type mockTransport struct {

gitea/post_review_comments_test.go

@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
 		{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
 	}
 
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", "", comments)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

github/client.go

@@ -10,7 +10,6 @@ import (
 	"io"
 	"log/slog"
 	"net/http"
-	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -34,6 +33,9 @@ const (
 	// maxResponseBodyBytes limits how much of a successful response body we read
 	// for defense-in-depth against servers returning excessively large payloads.
 	maxResponseBodyBytes = 10 * 1024 * 1024 // 10 MB
+
+	// envAllowInsecure is the environment variable that gates the WithAllowInsecureHTTP option.
+	envAllowInsecure = "REVIEW_BOT_ALLOW_INSECURE"
 )
 
 // APIError represents an HTTP error response from the GitHub API.
@@ -87,6 +89,27 @@ func asAPIError(err error) (*APIError, bool) {
 	return nil, false
 }
 
+// clientConfig holds optional configuration for NewClient.
+type clientConfig struct {
+	allowInsecureHTTP bool
+	testBypass        bool // skip env gate; only WithAllowInsecureHTTPForTest (export_test.go) should set this
+}
+
+// ClientOption configures optional behavior of NewClient.
+type ClientOption func(*clientConfig)
+
+// WithAllowInsecureHTTP permits the client to send credentials over HTTP (non-TLS) URLs.
+// This is gated by the REVIEW_BOT_ALLOW_INSECURE=1 environment variable as a
+// defense-in-depth safeguard. If the env var is not set, the option is ignored
+// and a warning is logged.
+//
+// For production use on trusted internal networks only.
+func WithAllowInsecureHTTP() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+	}
+}
+
 // Client interacts with the GitHub API.
 // A Client is safe for concurrent use by multiple goroutines.
 // SetHTTPClient and SetRetryBackoff are intended for test setup only and must
@@ -96,13 +119,10 @@ type Client struct {
 	// Higher-level exported methods (GetPullRequest, etc.) will use it to
 	// construct request URLs; remove this field if those methods end up
 	// accepting full URLs instead.
-	baseURL    string
-	token      string
-	httpClient *http.Client
-
-	// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
-	// When false, doRequest rejects URLs with an http:// scheme.
+	baseURL           string
+	token             string
 	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).
@@ -142,53 +162,39 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
 	return nil
 }
 
-// ClientOption configures optional behavior of a Client.
-type ClientOption func(*clientConfig)
-
-type clientConfig struct {
-	allowInsecureHTTP    bool
-	insecureIsTestBypass bool
-}
-
-// 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 ignored
-// and a warning is logged.
-//
-// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.
-func AllowInsecureHTTP() ClientOption {
-	return func(cfg *clientConfig) {
-		cfg.allowInsecureHTTP = true
-	}
-}
-
 // NewClient creates a new GitHub API client.
 // 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).
+// The baseURL must use HTTPS unless WithAllowInsecureHTTP() or WithAllowInsecureHTTPForTest() (test-only)
+// is passed as an option.
 func NewClient(token, baseURL string, opts ...ClientOption) *Client {
 	if baseURL == "" {
 		baseURL = defaultBaseURL
 	}
-
 	var cfg clientConfig
-	for _, opt := range opts {
-		opt(&cfg)
+	for _, o := range opts {
+		o(&cfg)
 	}
 
-	if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass {
-		if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" {
-			slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")
-			cfg.allowInsecureHTTP = false
+	// Environment gate for WithAllowInsecureHTTP (defense-in-depth).
+	// WithAllowInsecureHTTPForTest bypasses this gate for test convenience.
+	allowInsecure := false
+	if cfg.allowInsecureHTTP {
+		if cfg.testBypass {
+			allowInsecure = true
+		} else if os.Getenv(envAllowInsecure) == "1" {
+			allowInsecure = true
+			slog.Warn("WithAllowInsecureHTTP enabled — credentials may be sent over plaintext",
+				"env", envAllowInsecure+"=1")
 		} else {
-			slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
-				"env", "REVIEW_BOT_ALLOW_INSECURE=1")
+			slog.Warn("WithAllowInsecureHTTP option ignored", "hint", "set "+envAllowInsecure+"=1 to enable")
 		}
 	}
 
 	return &Client{
 		baseURL:           strings.TrimRight(baseURL, "/"),
 		token:             token,
-		allowInsecureHTTP: cfg.allowInsecureHTTP,
+		allowInsecureHTTP: allowInsecure,
 		httpClient: &http.Client{
 			Timeout:       30 * time.Second,
 			CheckRedirect: defaultCheckRedirect,
@@ -259,36 +265,27 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 	return 0, false
 }
 
-// redactURL redacts sensitive components from a URL for safe inclusion in error
-// messages and log output. It removes userinfo (e.g., user:pass@) and replaces
-// query parameters with a placeholder.
-func redactURL(rawURL string) string {
-	u, err := url.Parse(rawURL)
-	if err != nil {
-		return "<unparseable URL>"
-	}
-	u.User = nil
-
-	if u.RawQuery != "" {
-		u.RawQuery = "<redacted>"
-	}
-	return u.String()
+// hasHTTPSScheme reports whether rawURL starts with "https://" (case-insensitive).
+// It avoids the allocation of url.Parse for a simple scheme check.
+func hasHTTPSScheme(rawURL string) bool {
+	const prefix = "https://"
+	return len(rawURL) >= len(prefix) && strings.EqualFold(rawURL[:len(prefix)], prefix)
 }
 
 // doRequest performs an HTTP request with retry on 429 rate limit responses.
 // 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 {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if strings.EqualFold(parsed.Scheme, "http") {
-			return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
+	// Reject non-HTTPS URLs when credentials are present and insecure mode is not enabled.
+	// Uses a string prefix check instead of url.Parse to avoid per-request allocations.
+	if c.token != "" && !c.allowInsecureHTTP {
+		if !hasHTTPSScheme(reqURL) {
+			// Redact query parameters to avoid leaking sensitive data in error messages.
+			sanitized := reqURL
+			if i := strings.Index(reqURL, "?"); i >= 0 {
+				sanitized = reqURL[:i] + "?[REDACTED]"
+			}
+			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use WithAllowInsecureHTTP option for trusted networks)", sanitized)
 		}
 	}
 
@@ -310,7 +307,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 				timer := time.NewTimer(delay)
 				select {
 				case <-timer.C:
-					timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
+					timer.Stop()
 				case <-ctx.Done():
 					timer.Stop()
 					return nil, ctx.Err()

github/client_test.go

@@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("test-token", srv.URL, WithAllowInsecureHTTPForTest())
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -60,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -94,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.now = func() time.Time { return fixedNow }
 	// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
 	c.SetRetryBackoff([]time.Duration{0, 0})
@@ -130,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.now = func() time.Time { return fixedNow }
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
@@ -157,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -187,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -208,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
 		t.Fatal("expected error, got nil")
@@ -230,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
 		t.Fatal("expected error, got nil")
@@ -260,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	c := NewClient("tok", srv.URL, WithAllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
 
 	ctx, cancel := context.WithCancel(context.Background())
@@ -512,147 +512,129 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
 	}
 }
 
-func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) {
+func TestDoRequest_RejectsHTTPWithToken(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("ok"))
+		w.WriteHeader(200)
+		w.Write([]byte("{}"))
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	// Without WithAllowInsecureHTTP, should refuse to send token over HTTP
+	c := NewClient("secret-token", srv.URL)
+	_, 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_RejectsHTTPWithToken_RedactsQueryParams(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(200)
+		w.Write([]byte("{}"))
+	}))
+	defer srv.Close()
+
+	c := NewClient("secret-token", srv.URL)
+	_, err := c.doGet(context.Background(), srv.URL+"/test?secret=hunter2&token=abc")
+	if err == nil {
+		t.Fatal("expected error when sending token over HTTP")
+	}
+	errMsg := err.Error()
+	if strings.Contains(errMsg, "hunter2") || strings.Contains(errMsg, "token=abc") {
+		t.Errorf("error message should not contain query params, got: %v", errMsg)
+	}
+	if !strings.Contains(errMsg, "?[REDACTED]") {
+		t.Errorf("error message should contain redacted marker, got: %v", errMsg)
+	}
+}
+
+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)
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if string(body) != "ok" {
-		t.Errorf("body = %q, want %q", body, "ok")
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
 	}
 }
 
-func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
+func TestDoRequest_AllowsHTTPWithInsecureTestOption(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		t.Fatal("request should not have been sent")
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err == nil {
-		t.Fatal("expected error for HTTP request without AllowInsecureHTTP")
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTPForTest())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
 	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
 	}
 }
 
-func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
-	// Verify case-insensitive scheme check (RFC 3986).
-	c := NewClient("tok", "HTTP://127.0.0.1:1")
-	_, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test")
-	if err == nil {
-		t.Fatal("expected error for uppercase HTTP scheme")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
-	// Verify mixed case like "Http://" is also rejected.
-	c := NewClient("tok", "Http://127.0.0.1:1")
-	_, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test")
-	if err == nil {
-		t.Fatal("expected error for mixed-case HTTP scheme")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
+func TestWithAllowInsecureHTTP_RequiresEnvVar(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		t.Fatal("request should not have been sent")
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
 	}))
 	defer srv.Close()
 
+	// Without the env var, AllowInsecureHTTP should be ignored
 	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
-
-	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
-		t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
+		t.Fatal("expected error: WithAllowInsecureHTTP without env var should be rejected")
 	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
+	if !strings.Contains(err.Error(), "refusing to send credentials") {
+		t.Errorf("unexpected error: %v", err)
 	}
 }
 
-func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) {
+func TestWithAllowInsecureHTTP_WithEnvVar(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("insecure-ok"))
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
 	}))
 	defer srv.Close()
 
 	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
-
-	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if string(body) != "insecure-ok" {
-		t.Errorf("body = %q, want %q", body, "insecure-ok")
+	if string(body) != `{"ok":true}` {
+		t.Errorf("unexpected body: %s", body)
 	}
 }
 
-func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) {
+func TestWithAllowInsecureHTTP_EnvVarMustBeExactlyOne(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		t.Fatal("request should not have been sent")
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
 	}))
 	defer srv.Close()
 
-	// "true" is not "1" — strict check
+	// "true" is not "1" — should be rejected
 	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
-
-	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	c := NewClient("secret-token", srv.URL, WithAllowInsecureHTTP())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
-		t.Fatal("expected error: env var 'true' is not '1'")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestRedactURL_WithQuery(t *testing.T) {
-	got := redactURL("http://localhost:1234/path?secret=token&foo=bar")
-	want := "http://localhost:1234/path?<redacted>"
-	if got != want {
-		t.Errorf("redactURL = %q, want %q", got, want)
-	}
-}
-
-func TestRedactURL_NoQuery(t *testing.T) {
-	got := redactURL("http://localhost:1234/path")
-	want := "http://localhost:1234/path"
-	if got != want {
-		t.Errorf("redactURL = %q, want %q", got, want)
-	}
-}
-
-func TestRedactURL_Userinfo(t *testing.T) {
-	got := redactURL("http://user:pass@localhost:1234/path")
-	want := "http://localhost:1234/path"
-	if got != want {
-		t.Errorf("redactURL = %q, want %q", got, want)
-	}
-}
-
-func TestRedactURL_UserinfoWithQuery(t *testing.T) {
-	got := redactURL("http://user:pass@localhost:1234/path?secret=token")
-	want := "http://localhost:1234/path?<redacted>"
-	if got != want {
-		t.Errorf("redactURL = %q, want %q", got, want)
+		t.Fatal("expected error: env var must be exactly \"1\" to satisfy gate")
 	}
 }

github/export_test.go

@@ -1,13 +1,14 @@
 package github
 
-// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
+// WithAllowInsecureHTTPForTest permits the client to send credentials over HTTP
 // without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
-// This is intended exclusively for test code using httptest.Server.
+// This is intended exclusively for tests using httptest.Server.
 //
-// Defined in a _test.go file so it is only available to test binaries.
-func AllowInsecureHTTPForTest() ClientOption {
-	return func(cfg *clientConfig) {
-		cfg.allowInsecureHTTP = true
-		cfg.insecureIsTestBypass = true
+// This function lives in export_test.go so it is only available to test
+// binaries and does not appear in the production API surface.
+func WithAllowInsecureHTTPForTest() ClientOption {
+	return func(c *clientConfig) {
+		c.allowInsecureHTTP = true
+		c.testBypass = true
 	}
 }