feat(github): implement PRReader + FileReader client (#80)

Implement the GitHub API client with PRReader and FileReader interface
conformance for both github.com and GitHub Enterprise.

New files:
- github/client.go: Client struct, NewClient with configurable base URL,
  HTTP helpers with 429 retry and Retry-After support
- github/pr.go: GetPullRequest, GetPullRequestDiff (per-request Accept
  header), GetPullRequestFiles (paginated, populates Patch field),
  GetFileContentAtRef (base64 decode), GetCommitStatuses (merges commit
  statuses + check runs with conclusion mapping)
- github/files.go: GetFileContent (delegates to GetFileContentAtRef),
  ListContents, escapePath, decodeBase64Content helpers

Type changes:
- vcs/types.go: Add Patch field to ChangedFile struct

Tests cover: happy path, 404, 401, 429+retry, malformed response,
pagination, binary files, check run conclusion mapping, base64 decoding.

Compile-time checks:
  var _ vcs.PRReader = (*Client)(nil)
  var _ vcs.FileReader = (*Client)(nil)

Exit criteria met:
- go test ./github/... passes (all methods)
- NewClient with empty baseURL uses https://api.github.com
- NewClient with GHE URL targets correctly
- GetFileContent delegates to GetFileContentAtRef with empty ref
- GetPullRequestFiles paginates and populates Patch field
- GetCommitStatuses merges both commit statuses and check-runs

github/client.go

@@ -1,6 +1,6 @@
 // Package github provides a client for the GitHub API.
-// It supports pull request operations, file content retrieval, CI status checks,
-// and directory listing for both github.com and GitHub Enterprise.
+// It supports pull request operations, file content retrieval,
+// and review submission for both github.com and GitHub Enterprise.
 package github
 
 import (
@@ -9,28 +9,16 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"net/url"
 	"strconv"
 	"strings"
 	"time"
 )
 
-const (
-	defaultBaseURL = "https://api.github.com"
-	userAgent      = "review-bot/1.0"
-
-	// maxResponseBytes limits successful response body reads to 10 MiB.
-	maxResponseBytes = 10 * 1024 * 1024
-)
+const defaultBaseURL = "https://api.github.com"
 
 // APIError represents an HTTP error response from the GitHub API.
 // It carries the status code so callers can distinguish between
 // different failure modes (e.g. 404 vs 500).
-//
-// The Body field stores up to 4 KiB of the raw response for programmatic
-// inspection. Error() truncates to 200 bytes for safe logging, but callers
-// should avoid logging or propagating Body directly in production since it may
-// contain sensitive details from the upstream server.
 type APIError struct {
 	StatusCode int
 	Body       string
@@ -41,19 +29,9 @@ func (e *APIError) Error() string {
 	if len(body) > 200 {
 		body = body[:200] + "...(truncated)"
 	}
-	// Sanitize newlines to prevent log injection from upstream response bodies.
-	body = strings.ReplaceAll(body, "\n", " ")
-	body = strings.ReplaceAll(body, "\r", " ")
 	return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
 }
 
-// SafeError returns the error string without response body content,
-// suitable for logging in contexts where upstream response data should
-// not be exposed.
-func (e *APIError) SafeError() string {
-	return fmt.Sprintf("HTTP %d", e.StatusCode)
-}
-
 // IsNotFound reports whether an error is an API 404 response.
 func IsNotFound(err error) bool {
 	if apiErr, ok := asAPIError(err); ok {
@@ -81,151 +59,49 @@ func asAPIError(err error) (*APIError, bool) {
 	return nil, false
 }
 
-// clientConfig holds optional configuration for NewClient.
-type clientConfig struct {
-	allowInsecureHTTP bool
-}
-
-// ClientOption configures optional behavior of NewClient.
-type ClientOption func(*clientConfig)
-
-// AllowInsecureHTTP permits the client to use HTTP (non-TLS) base URLs.
-// This should only be used for trusted internal deployments or testing.
-func AllowInsecureHTTP() 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
-// be called before any goroutines issue requests; they have no synchronization.
 type Client struct {
-	baseURL           string
-	token             string
-	allowInsecureHTTP bool
-	httpClient        *http.Client
+	baseURL string
+	token   string
+	http    *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).
-	// If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff.
-	retryBackoff []time.Duration
-}
-
-// defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil).
-// It rejects HTTPS→HTTP protocol downgrades (to prevent plaintext leakage) and strips
-// the Authorization header on cross-host redirects to prevent credential leakage to
-// third-party hosts (e.g. CDN redirects from GitHub).
-func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
-	if len(via) >= 10 {
-		return fmt.Errorf("stopped after 10 redirects")
-	}
-	// Guard: net/http guarantees len(via) >= 1 but this is undocumented;
-	// defend against zero-length to avoid panic on index out of range.
-	if len(via) == 0 {
-		return nil
-	}
-	prev := via[len(via)-1]
-	// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
-	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
-		return fmt.Errorf("refusing redirect from HTTPS to HTTP (%s → %s)", prev.URL.Host, req.URL.Host)
-	}
-	// Strip Authorization on cross-host redirect to avoid leaking credentials
-	// to third-party hosts (GitHub legitimately redirects to CDN hosts).
-	if req.URL.Host != prev.URL.Host {
-		req.Header.Del("Authorization")
-	}
-	return nil
+	// RetryBackoff defines the delays between retry attempts for 429 responses.
+	// RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
+	// If nil, defaults to {1s, 2s}. Set to shorter durations in tests.
+	RetryBackoff []time.Duration
 }
 
 // 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; pass AllowInsecureHTTP() as an option to permit HTTP
-// for trusted internal deployments (e.g. local testing).
-func NewClient(token, baseURL string, opts ...ClientOption) *Client {
+func NewClient(token, baseURL string) *Client {
 	if baseURL == "" {
 		baseURL = defaultBaseURL
 	}
-	cfg := clientConfig{}
-	for _, o := range opts {
-		o(&cfg)
-	}
 	return &Client{
-		baseURL:           strings.TrimRight(baseURL, "/"),
-		allowInsecureHTTP: cfg.allowInsecureHTTP,
-		token:             token,
-		httpClient: &http.Client{
-			Timeout:       30 * time.Second,
-			CheckRedirect: defaultCheckRedirect,
-		},
+		baseURL: strings.TrimRight(baseURL, "/"),
+		token:   token,
+		http:    &http.Client{Timeout: 30 * time.Second},
 	}
 }
 
 // SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for test setup only to inject mock transports; it must be
-// called before any goroutines issue requests.
-//
-// Passing nil restores the default client (30s timeout + auth-stripping
-// CheckRedirect policy matching NewClient).
-//
-// Callers providing a non-nil client are responsible for configuring a safe
-// CheckRedirect policy. Without one, the default net/http behavior will follow
-// redirects and may forward the Authorization header to untrusted hosts.
+// This is intended for testing to inject mock transports.
 func (c *Client) SetHTTPClient(hc *http.Client) {
-	if hc == nil {
-		hc = &http.Client{
-			Timeout:       30 * time.Second,
-			CheckRedirect: defaultCheckRedirect,
-		}
-	} else if hc.CheckRedirect == nil {
-		// Enforce safe redirect policy when caller provides a client without one.
-		// The default net/http behavior follows up to 10 redirects and forwards
-		// all headers (including Authorization) to any host, which can leak
-		// credentials on cross-host redirects.
-		hc.CheckRedirect = defaultCheckRedirect
-	}
-	c.httpClient = hc
-}
-
-// SetRetryBackoff configures the retry backoff durations for testing.
-// It must be called before any goroutines issue requests.
-// In production the default {1s, 2s} applies.
-func (c *Client) SetRetryBackoff(d []time.Duration) {
-	c.retryBackoff = d
+	c.http = hc
 }
 
 // doRequest performs an HTTP request with retry on 429 rate limit responses.
-// It respects the Retry-After header when present (capped at maxRetryAfter).
-// Transport errors (network failures, context cancellation) are not retried.
-func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
+// It respects the Retry-After header when present.
+func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
 	const maxAttempts = 3
-	const maxRetryAfter = 120 * time.Second
-
-	var backoff []time.Duration
-	if c.retryBackoff != nil {
-		backoff = make([]time.Duration, len(c.retryBackoff))
-		copy(backoff, c.retryBackoff)
-	} else {
+	backoff := c.RetryBackoff
+	if backoff == nil {
 		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
 	}
 
-	// maxErrorBodyBytes limits how much of an error response body is stored.
-	// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
-	// log APIError.Body directly. Error() further truncates to 200 bytes.
-	const maxErrorBodyBytes = 4 * 1024
-
-	// Reject non-HTTPS URLs early since the URL is immutable across retries.
-	if c.token != "" && !c.allowInsecureHTTP {
-		parsed, err := url.Parse(reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if !strings.EqualFold(parsed.Scheme, "https") {
-			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
-		}
-	}
+	const maxErrorBodyBytes = 64 * 1024
 
 	var lastErr error
 	for attempt := 0; attempt < maxAttempts; attempt++ {
@@ -238,7 +114,6 @@ 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
 				case <-ctx.Done():
 					timer.Stop()
 					return nil, ctx.Err()
@@ -246,58 +121,43 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			}
 		}
 
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
+		req, err := http.NewRequestWithContext(ctx, method, url, nil)
 		if err != nil {
 			return nil, fmt.Errorf("create request: %w", err)
 		}
-		if c.token != "" {
-			// Bearer is the OAuth2 standard and is accepted by GitHub for both
-			// classic PATs and fine-grained tokens. The alternative "token" scheme
-			// is GitHub-specific and offers no additional compatibility.
-			req.Header.Set("Authorization", "Bearer "+c.token)
-		}
-		req.Header.Set("User-Agent", userAgent)
+		req.Header.Set("Authorization", "Bearer "+c.token)
 		if accept != "" {
 			req.Header.Set("Accept", accept)
 		} else {
 			req.Header.Set("Accept", "application/vnd.github+json")
 		}
 
-		resp, err := c.httpClient.Do(req)
+		resp, err := c.http.Do(req)
 		if err != nil {
-			// Transport errors (DNS, TLS, timeout) yield nil resp; no body to close.
 			return nil, fmt.Errorf("do request: %w", err)
 		}
 
-		body, done, err := handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
-		if done {
-			return body, err
+		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
+			body, err := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			if err != nil {
+				return nil, fmt.Errorf("read response body: %w", err)
+			}
+			return body, nil
 		}
-		lastErr = err
+
+		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		resp.Body.Close()
+
+		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
 
 		// Retry on 429 rate limit
 		if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
-			// Check for Retry-After header and override backoff if present.
-			// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
+			// Check for Retry-After header and override backoff if present
 			if ra := resp.Header.Get("Retry-After"); ra != "" {
 				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
-					delay := time.Duration(seconds) * time.Second
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
 					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				} else if retryAt, err := http.ParseTime(ra); err == nil {
-					delay := time.Until(retryAt)
-					if delay < 0 {
-						delay = 0
-					}
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
+						backoff[attempt] = time.Duration(seconds) * time.Second
 					}
 				}
 			}
@@ -311,31 +171,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	return nil, lastErr
 }
 
-// handleResponse reads and closes the response body, returning the result.
-// It uses defer to ensure the body is always closed regardless of code path.
-// Returns (body, done, err) where done=true means the caller should return immediately.
-func handleResponse(resp *http.Response, maxRespBytes int, maxErrBytes int) ([]byte, bool, error) {
-	defer resp.Body.Close()
-
-	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-		body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxRespBytes)+1))
-		if err != nil {
-			return nil, true, fmt.Errorf("read response body: %w", err)
-		}
-		if len(body) > maxRespBytes {
-			return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes)
-		}
-		return body, true, nil
-	}
-
-	errBody, readErr := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrBytes)))
-	if readErr != nil && len(errBody) == 0 {
-		errBody = []byte(fmt.Sprintf("[error reading response body: %v]", readErr))
-	}
-	return nil, false, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
-}
-
 // doGet is a convenience wrapper for GET requests with the default Accept header.
-func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
-	return c.doRequest(ctx, http.MethodGet, reqURL, "")
+func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
+	return c.doRequest(ctx, http.MethodGet, url, "")
 }

github/client_test.go

@@ -4,8 +4,6 @@ import (
 	"context"
 	"net/http"
 	"net/http/httptest"
-	"net/url"
-	"strings"
 	"testing"
 	"time"
 )
@@ -40,7 +38,7 @@ func TestDoRequest_SetsAuthHeader(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("my-token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("my-token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 	_, _ = c.doGet(context.Background(), srv.URL+"/test")
 
@@ -58,7 +56,7 @@ func TestDoRequest_SetsDefaultAcceptHeader(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 	_, _ = c.doGet(context.Background(), srv.URL+"/test")
 
@@ -72,6 +70,7 @@ func TestDoRequest_429Retry(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
+			w.Header().Set("Retry-After", "1")
 			w.WriteHeader(429)
 			w.Write([]byte(`{"message":"rate limit"}`))
 			return
@@ -81,9 +80,9 @@ func TestDoRequest_429Retry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{10 * time.Millisecond, 10 * time.Millisecond}
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
@@ -106,9 +105,9 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
 
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
@@ -135,7 +134,7 @@ func TestDoRequest_404NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -156,7 +155,7 @@ func TestDoRequest_401NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -185,415 +184,3 @@ func TestIsUnauthorized(t *testing.T) {
 		t.Error("expected IsUnauthorized to return true for 401")
 	}
 }
-
-func TestAPIError_SanitizesNewlines(t *testing.T) {
-	err := &APIError{StatusCode: 500, Body: "line1\ninjected\rmore"}
-	msg := err.Error()
-	if strings.Contains(msg, "\n") || strings.Contains(msg, "\r") {
-		t.Errorf("expected newlines to be sanitized, got: %q", msg)
-	}
-	if !strings.Contains(msg, "line1 injected more") {
-		t.Errorf("expected sanitized body, got: %q", msg)
-	}
-}
-
-func TestDoRequest_429RetryAfterHeader(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping slow retry test in short mode")
-	}
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", "1")
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		w.WriteHeader(200)
-		w.Write([]byte(`{"ok":true}`))
-	}))
-	defer srv.Close()
-
-	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})
-
-	start := time.Now()
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	elapsed := time.Since(start)
-
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != `{"ok":true}` {
-		t.Errorf("unexpected body: %s", body)
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-	// Retry-After: 1 means at least 1 second delay
-	if elapsed < 900*time.Millisecond {
-		t.Errorf("expected ~1s delay from Retry-After, got %v", elapsed)
-	}
-}
-
-func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping slow retry test in short mode")
-	}
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", "1")
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		w.WriteHeader(200)
-		w.Write([]byte(`{"ok":true}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
-
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-
-	// Verify the original retryBackoff slice was not mutated
-	if c.retryBackoff[0] != 1*time.Millisecond {
-		t.Errorf("retryBackoff[0] was mutated: got %v, want 1ms", c.retryBackoff[0])
-	}
-	if c.retryBackoff[1] != 1*time.Millisecond {
-		t.Errorf("retryBackoff[1] was mutated: got %v, want 1ms", c.retryBackoff[1])
-	}
-}
-
-func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
-	if testing.Short() {
-		t.Skip("skipping slow Retry-After HTTP-date test in short mode")
-	}
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			// Use HTTP-date format (RFC 7231) — a time 2 seconds in the future.
-			future := time.Now().Add(2 * time.Second).UTC()
-			w.Header().Set("Retry-After", future.Format(http.TimeFormat))
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		w.WriteHeader(200)
-		w.Write([]byte(`{"ok":true}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
-
-	start := time.Now()
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	elapsed := time.Since(start)
-
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != `{"ok":true}` {
-		t.Errorf("unexpected body: %s", body)
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-	// HTTP-date was ~2s in the future; by the time client processes it,
-	// time.Until gives ~1-2s. Verify it's meaningfully delayed (not instant).
-	if elapsed < 500*time.Millisecond {
-		t.Errorf("expected meaningful delay from HTTP-date Retry-After, got %v", elapsed)
-	}
-}
-
-func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			// Use a time in the past — should result in zero/immediate retry.
-			past := time.Now().Add(-10 * time.Second).UTC()
-			w.Header().Set("Retry-After", past.Format(http.TimeFormat))
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		w.WriteHeader(200)
-		w.Write([]byte(`{"ok":true}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
-
-	start := time.Now()
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	elapsed := time.Since(start)
-
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-	// Past date should override the 5s backoff to ~0
-	if elapsed > 500*time.Millisecond {
-		t.Errorf("expected near-instant retry for past HTTP-date, got %v", elapsed)
-	}
-}
-
-func TestDoRequest_SetsUserAgentHeader(t *testing.T) {
-	var gotUA string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		gotUA = r.Header.Get("User-Agent")
-		w.WriteHeader(200)
-		w.Write([]byte("{}"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	_, _ = c.doGet(context.Background(), srv.URL+"/test")
-
-	if gotUA != "review-bot/1.0" {
-		t.Errorf("expected User-Agent 'review-bot/1.0', got %q", gotUA)
-	}
-}
-
-func TestDoRequest_LimitsResponseBody(t *testing.T) {
-	// 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)
-		w.Write([]byte(bigBody))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err == nil {
-		t.Fatal("expected error for oversized response body")
-	}
-	if !strings.Contains(err.Error(), "exceeded") {
-		t.Errorf("expected truncation error, got: %v", err)
-	}
-}
-
-func TestDoRequest_AcceptsExactlyAtLimit(t *testing.T) {
-	// A response body exactly equal to maxResponseBytes should succeed (not error).
-	exactBody := strings.Repeat("x", maxResponseBytes)
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(200)
-		w.Write([]byte(exactBody))
-	}))
-	defer srv.Close()
-
-	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 for exactly-at-limit body: %v", err)
-	}
-	if len(body) != maxResponseBytes {
-		t.Errorf("expected body length %d, got %d", maxResponseBytes, len(body))
-	}
-}
-
-func TestDoRequest_SkipsAuthWhenTokenEmpty(t *testing.T) {
-	var gotAuth string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		gotAuth = r.Header.Get("Authorization")
-		w.WriteHeader(200)
-		w.Write([]byte("{}"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("", srv.URL, AllowInsecureHTTP()) // empty token
-	c.SetHTTPClient(srv.Client())
-	_, _ = c.doGet(context.Background(), srv.URL+"/test")
-
-	if gotAuth != "" {
-		t.Errorf("expected no Authorization header with empty token, got %q", gotAuth)
-	}
-}
-
-func TestNewClient_CheckRedirectStripsAuthOnCrossHost(t *testing.T) {
-	// Verify the CheckRedirect function is configured
-	c := NewClient("secret-token", "https://api.github.com")
-	if c.httpClient.CheckRedirect == nil {
-		t.Fatal("expected CheckRedirect to be set")
-	}
-}
-
-func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
-	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
-	req := &http.Request{
-		URL:    &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"},
-		Header: http.Header{"Authorization": []string{"Bearer token"}},
-	}
-	err := defaultCheckRedirect(req, []*http.Request{prev})
-	if err == nil {
-		t.Fatal("expected error on HTTPS→HTTP redirect")
-	}
-	if !strings.Contains(err.Error(), "refusing redirect from HTTPS to HTTP") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestDefaultCheckRedirect_StripsAuthOnCrossHost(t *testing.T) {
-	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
-	req := &http.Request{
-		URL:    &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"},
-		Header: http.Header{"Authorization": []string{"Bearer token"}},
-	}
-	err := defaultCheckRedirect(req, []*http.Request{prev})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if auth := req.Header.Get("Authorization"); auth != "" {
-		t.Errorf("expected Authorization header to be stripped, got %q", auth)
-	}
-}
-
-func TestDefaultCheckRedirect_PreservesAuthOnSameHost(t *testing.T) {
-	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
-	req := &http.Request{
-		URL:    &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
-		Header: http.Header{"Authorization": []string{"Bearer token"}},
-	}
-	err := defaultCheckRedirect(req, []*http.Request{prev})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if auth := req.Header.Get("Authorization"); auth != "Bearer token" {
-		t.Errorf("expected Authorization to be preserved, got %q", auth)
-	}
-}
-
-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)
-	}
-}
-
-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)
-	}
-	if c.httpClient.CheckRedirect == nil {
-		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
-	}
-}
-
-func TestSetHTTPClient_NilCheckRedirectEnforcesDefault(t *testing.T) {
-	c := NewClient("token", "https://api.github.com")
-	// Provide a client with nil CheckRedirect — should get default policy enforced.
-	hc := &http.Client{Timeout: 5 * time.Second}
-	c.SetHTTPClient(hc)
-	if c.httpClient.CheckRedirect == nil {
-		t.Fatal("expected CheckRedirect to be enforced when caller provides nil")
-	}
-	if c.httpClient.Timeout != 5*time.Second {
-		t.Errorf("expected caller's timeout preserved, got %v", c.httpClient.Timeout)
-	}
-}
-
-func TestSetHTTPClient_PreservesCustomCheckRedirect(t *testing.T) {
-	c := NewClient("token", "https://api.github.com")
-	called := false
-	hc := &http.Client{
-		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-			called = true
-			return nil
-		},
-	}
-	c.SetHTTPClient(hc)
-	// Invoke the redirect to verify original is preserved
-	_ = c.httpClient.CheckRedirect(nil, []*http.Request{{}})
-	if !called {
-		t.Fatal("expected custom CheckRedirect to be preserved")
-	}
-}
-
-func TestAPIError_SafeError(t *testing.T) {
-	e := &APIError{StatusCode: 403, Body: "some sensitive body content"}
-	got := e.SafeError()
-	if got != "HTTP 403" {
-		t.Errorf("SafeError() = %q, want %q", got, "HTTP 403")
-	}
-	// Ensure Error() still includes body
-	full := e.Error()
-	if full != "HTTP 403: some sensitive body content" {
-		t.Errorf("Error() = %q, unexpected", full)
-	}
-}

github/files.go

@@ -11,54 +11,14 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 
-// GetFileContent fetches a file from a repo at the given ref.
-// Delegates to GetFileContentAtRef with the provided ref.
+// GetFileContent fetches a file from the default branch of a repo.
+// Delegates to GetFileContentAtRef with an empty ref.
 func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
 	return c.GetFileContentAtRef(ctx, owner, repo, path, ref)
 }
 
-// GetFileContentAtRef fetches a file at a specific ref from a repo.
-// If ref is empty, the query parameter is omitted (uses default branch).
-//
-// Note: dot-segments ("." and "..") in the path are silently removed to
-// prevent path traversal. This means a path like "foo/../bar" resolves
-// to "foo/bar" rather than "bar".
-func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
-	if ref != "" {
-		reqURL += "?ref=" + url.QueryEscape(ref)
-	}
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return "", fmt.Errorf("fetch file %s: %w", path, err)
-	}
-	var resp struct {
-		Content  string `json:"content"`
-		Encoding string `json:"encoding"`
-	}
-	if err := json.Unmarshal(body, &resp); err != nil {
-		return "", fmt.Errorf("parse file content JSON: %w", err)
-	}
-	if resp.Encoding != "base64" {
-		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
-	}
-	decoded, err := decodeBase64Content(resp.Content)
-	if err != nil {
-		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
-	}
-	return decoded, nil
-}
-
 // ListContents lists files and directories at a given path in a repo.
 // 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.
-//
-// Note: dot-segments ("." and "..") in the path are silently removed to
-// prevent path traversal. This means a path like "foo/../bar" resolves
-// to "foo/bar" rather than "bar".
 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))
@@ -66,31 +26,14 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 	if err != nil {
 		return nil, fmt.Errorf("list contents %s: %w", path, err)
 	}
-
-	type entry struct {
+	var entries []struct {
 		Name string `json:"name"`
 		Path string `json:"path"`
 		Type string `json:"type"`
 	}
-
-	// 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.
-	// An empty array ([]) is valid — it represents an empty directory — and
-	// results in a zero-length slice returned without error.
-	var entries []entry
 	if err := json.Unmarshal(body, &entries); err != nil {
-		var single entry
-		if err2 := json.Unmarshal(body, &single); err2 != nil {
-			return nil, fmt.Errorf("parse contents JSON: as array: %w; as object: %w", err, err2)
-		}
-		// Guard against empty objects ({}) or unexpected shapes that
-		// unmarshal successfully but carry no useful data.
-		if single.Name == "" && single.Path == "" && single.Type == "" {
-			return nil, fmt.Errorf("parse contents JSON: unexpected response format")
-		}
-		entries = []entry{single}
+		return nil, fmt.Errorf("parse contents JSON: %w", err)
 	}
-
 	result := make([]vcs.ContentEntry, len(entries))
 	for i, e := range entries {
 		result[i] = vcs.ContentEntry{
@@ -104,29 +47,19 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 
 // escapePath escapes each segment of a relative file path for use in URLs.
 // Slashes are preserved as path separators; other special characters are escaped.
-// Dot-segments ("." and "..") and empty segments (from consecutive slashes like
-// "a//b") are silently removed to prevent path traversal and produce canonical
-// paths. This is intentional: callers may receive a different path than requested
-// without 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
-	for _, part := range parts {
-		if part == "." || part == ".." || part == "" {
-			continue
-		}
-		clean = append(clean, url.PathEscape(part))
+	for i, part := range parts {
+		parts[i] = url.PathEscape(part)
 	}
-	return strings.Join(clean, "/")
+	return strings.Join(parts, "/")
 }
 
 // decodeBase64Content decodes base64-encoded content from the GitHub contents API.
-// GitHub returns base64 content with line breaks for formatting; we strip \r and \n before decoding.
+// GitHub returns base64 content with newlines for formatting, which we strip before decoding.
 func decodeBase64Content(encoded string) (string, error) {
 	// GitHub inserts newlines in base64 content
-	cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
+	cleaned := strings.ReplaceAll(encoded, "\n", "")
 	decoded, err := base64.StdEncoding.DecodeString(cleaned)
 	if err != nil {
 		return "", err

github/files_test.go

@@ -20,7 +20,7 @@ func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
@@ -107,9 +107,9 @@ func TestGetFileContent_429Retry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{1 * time.Millisecond}
 
 	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err != nil {
@@ -130,7 +130,7 @@ func TestGetFileContent_MalformedJSON(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
@@ -225,9 +225,9 @@ func TestListContents_429Retry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{1 * time.Millisecond}
 
 	entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
 	if err != nil {
@@ -248,7 +248,7 @@ func TestListContents_MalformedJSON(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
@@ -275,60 +275,3 @@ func TestDecodeBase64Content_Invalid(t *testing.T) {
 		t.Fatal("expected error for invalid base64")
 	}
 }
-
-func TestEscapePath_RejectsDotSegments(t *testing.T) {
-	tests := []struct {
-		input string
-		want  string
-	}{
-		{"src/main.go", "src/main.go"},
-		{"../etc/passwd", "etc/passwd"},
-		{"./src/../main.go", "src/main.go"},
-		{"a/b/c", "a/b/c"},
-		{"file with spaces.go", "file%20with%20spaces.go"},
-		{"a/./b/../c", "a/b/c"},
-	}
-	for _, tt := range tests {
-		got := escapePath(tt.input)
-		if got != tt.want {
-			t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want)
-		}
-	}
-}
-
-func TestDecodeBase64Content_CRLF(t *testing.T) {
-	// Base64 of "hello world" with CRLF line breaks inserted
-	encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ="
-	decoded, err := decodeBase64Content(encoded)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if decoded != "hello world" {
-		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)
-	}
-}

github/pr.go

@@ -33,6 +33,7 @@ type changedFileResponse struct {
 
 // commitStatusResponse is the GitHub combined status API response.
 type commitStatusResponse struct {
+	State    string `json:"state"`
 	Statuses []struct {
 		Context     string `json:"context"`
 		State       string `json:"state"`
@@ -43,7 +44,8 @@ type commitStatusResponse struct {
 
 // checkRunsResponse is the GitHub check runs API response.
 type checkRunsResponse struct {
-	CheckRuns []struct {
+	TotalCount int `json:"total_count"`
+	CheckRuns  []struct {
 		Name       string  `json:"name"`
 		Conclusion *string `json:"conclusion"`
 		Status     string  `json:"status"`
@@ -82,18 +84,13 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
 	return string(body), nil
 }
 
-// maxPages is the upper bound on pagination loops to prevent unbounded iteration
-// in case the server returns a full page indefinitely.
-const maxPages = 100
-
 // GetPullRequestFiles fetches the list of files changed in a PR.
 // Paginates through all pages (100 per page) to collect all files.
-// Returns nil (not an empty slice) when the PR has no changed files.
-// Callers can safely range over or check len() on a nil slice.
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	var allFiles []vcs.ChangedFile
+	page := 1
 
-	for page := 1; page <= maxPages; page++ {
+	for {
 		reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d",
 			c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
 		body, err := c.doGet(ctx, reqURL)
@@ -117,19 +114,43 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
 		if len(files) < 100 {
 			break
 		}
+		page++
 	}
 
 	return allFiles, nil
 }
 
+// GetFileContentAtRef fetches a file at a specific ref from a repo.
+// If ref is empty, the query parameter is omitted (uses default branch).
+func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
+	reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
+	if ref != "" {
+		reqURL += "?ref=" + url.QueryEscape(ref)
+	}
+	body, err := c.doGet(ctx, reqURL)
+	if err != nil {
+		return "", fmt.Errorf("fetch file %s: %w", path, err)
+	}
+	var resp struct {
+		Content  string `json:"content"`
+		Encoding string `json:"encoding"`
+	}
+	if err := json.Unmarshal(body, &resp); err != nil {
+		return "", fmt.Errorf("parse file content JSON: %w", err)
+	}
+	if resp.Encoding != "base64" {
+		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
+	}
+	decoded, err := decodeBase64Content(resp.Content)
+	if err != nil {
+		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
+	}
+	return decoded, nil
+}
+
 // GetCommitStatuses fetches both commit statuses and check runs for a SHA,
 // merging them into a unified []vcs.CommitStatus slice.
-// Returns nil (not an empty slice) when there are no statuses or check runs.
-// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
-// function returns immediately without attempting the check-runs endpoint.
-// If the check-runs endpoint fails after statuses were fetched successfully,
-// the function returns an error (not a partial result) so callers always get
-// either a complete view or a clear error signal.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
 	var result []vcs.CommitStatus
 
@@ -154,7 +175,8 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 	}
 
 	// Fetch check runs (paginated)
-	for checkPage := 1; checkPage <= maxPages; checkPage++ {
+	checkPage := 1
+	for {
 		checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d",
 			c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage)
 		checkBody, err := c.doGet(ctx, checkURL)
@@ -168,7 +190,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 		for _, cr := range checkResp.CheckRuns {
 			result = append(result, vcs.CommitStatus{
 				Context:     cr.Name,
-				Status:      mapCheckRunStatus(cr.Conclusion),
+				Status:      mapCheckRunStatus(cr.Conclusion, cr.Status),
 				Description: derefString(cr.Conclusion),
 				TargetURL:   cr.HTMLURL,
 			})
@@ -176,15 +198,14 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 		if len(checkResp.CheckRuns) < 100 {
 			break
 		}
+		checkPage++
 	}
 
 	return result, nil
 }
 
-// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
-// Conclusion alone determines the mapped state: nil conclusion means the run is
-// still in progress (pending), regardless of the status field value.
-func mapCheckRunStatus(conclusion *string) string {
+// mapCheckRunStatus maps a check run conclusion+status to a vcs.CommitStatus status string.
+func mapCheckRunStatus(conclusion *string, status string) string {
 	if conclusion == nil {
 		// Still running or queued
 		return "pending"
@@ -195,8 +216,8 @@ func mapCheckRunStatus(conclusion *string) string {
 	case "failure", "action_required", "timed_out":
 		return "failure"
 	case "cancelled", "skipped", "neutral":
-		return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
-	case "stale", "waiting":
+		return "success" // non-blocking
+	case "in_progress", "queued":
 		return "pending"
 	default:
 		return "pending"

github/pr_test.go

@@ -26,7 +26,7 @@ func TestGetPullRequest_HappyPath(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
@@ -110,9 +110,9 @@ func TestGetPullRequest_429Retry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{1 * time.Millisecond}
 
 	pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err != nil {
@@ -133,7 +133,7 @@ func TestGetPullRequest_MalformedJSON(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
@@ -445,9 +445,9 @@ func TestGetFileContentAtRef_429Retry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
+	c.RetryBackoff = []time.Duration{1 * time.Millisecond}
 
 	content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
 	if err != nil {
@@ -496,7 +496,7 @@ func TestGetCommitStatuses_HappyPath(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
@@ -528,13 +528,13 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
 		status     string
 		want       string
 	}{
-		{stringPtr("success"), "completed", "success"},
-		{stringPtr("failure"), "completed", "failure"},
-		{stringPtr("action_required"), "completed", "failure"},
-		{stringPtr("timed_out"), "completed", "failure"},
-		{stringPtr("cancelled"), "completed", "success"},
-		{stringPtr("skipped"), "completed", "success"},
-		{stringPtr("neutral"), "completed", "success"},
+		{strPtr("success"), "completed", "success"},
+		{strPtr("failure"), "completed", "failure"},
+		{strPtr("action_required"), "completed", "failure"},
+		{strPtr("timed_out"), "completed", "failure"},
+		{strPtr("cancelled"), "completed", "success"},
+		{strPtr("skipped"), "completed", "success"},
+		{strPtr("neutral"), "completed", "success"},
 		{nil, "in_progress", "pending"},
 		{nil, "queued", "pending"},
 	}
@@ -567,7 +567,7 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
 			}))
 			defer srv.Close()
 
-			c := NewClient("token", srv.URL, AllowInsecureHTTP())
+			c := NewClient("token", srv.URL)
 			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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	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, AllowInsecureHTTP())
+	c := NewClient("token", srv.URL)
 	c.SetHTTPClient(srv.Client())
 
 	_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
@@ -632,6 +632,6 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
 	}
 }
 
-func stringPtr(s string) *string {
+func strPtr(s string) *string {
 	return &s
 }