fix: address review feedback on PRReader implementation

- Add maxFileContentSize (10 MB) limit to decodeBase64Content to prevent
  resource exhaustion from oversized file content (security MINOR)
- Fix reversed NewClient arg order in TestGetFileContentAtRef_DotSegmentError
  (GPT MINOR + Sonnet NIT)
- Remove 'waiting' from mapCheckRunStatus conclusion cases since it is a
  status value not a conclusion, update comment (GPT NIT)
- Add TestDecodeBase64Content_SizeLimit test

github/client.go

@@ -21,6 +21,10 @@ const (
 
 	// maxResponseBytes limits successful response body reads to 10 MiB.
 	maxResponseBytes = 10 * 1024 * 1024
+
+	// maxRetryAttempts is the number of times doRequest will attempt a request.
+	// The retry backoff slice must have length maxRetryAttempts-1.
+	maxRetryAttempts = 3
 )
 
 // APIError represents an HTTP error response from the GitHub API.
@@ -178,24 +182,33 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
 
 // SetRetryBackoff configures the retry backoff durations for testing.
 // It must be called before any goroutines issue requests.
+// The slice must have exactly maxRetryAttempts-1 entries (one delay per retry gap).
 // In production the default {1s, 2s} applies.
-func (c *Client) SetRetryBackoff(d []time.Duration) {
+func (c *Client) SetRetryBackoff(d []time.Duration) error {
+	if len(d) != maxRetryAttempts-1 {
+		return fmt.Errorf("github: backoff length %d does not match maxRetryAttempts-1 (%d)", len(d), maxRetryAttempts-1)
+	}
 	c.retryBackoff = d
+	return nil
 }
 
 // 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) {
-	const maxAttempts = 3
 	const maxRetryAfter = 120 * time.Second
 
+	// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
+	// Length must be maxRetryAttempts-1 (one entry per retry gap).
+	// SetRetryBackoff validates at configuration time; the default is always valid.
+	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
 	var backoff []time.Duration
-	if c.retryBackoff != nil {
+	if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
 		backoff = make([]time.Duration, len(c.retryBackoff))
 		copy(backoff, c.retryBackoff)
 	} else {
-		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
+		backoff = make([]time.Duration, len(defaultBackoff))
+		copy(backoff, defaultBackoff)
 	}
 
 	// maxErrorBodyBytes limits how much of an error response body is stored.
@@ -215,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	}
 
 	var lastErr error
-	for attempt := 0; attempt < maxAttempts; attempt++ {
+	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
 		if attempt > 0 {
 			var delay time.Duration
 			if attempt-1 < len(backoff) {
@@ -255,6 +268,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			return nil, fmt.Errorf("do request: %w", err)
 		}
 
+		// Capture response metadata before handleResponse takes body ownership.
+		respStatus := resp.StatusCode
+		retryAfterHeader := resp.Header.Get("Retry-After")
+
 		body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		if done {
 			return body, err
@@ -262,10 +279,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		lastErr = err
 
 		// Retry on 429 rate limit
-		if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
+		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
 			// Check for Retry-After header and override backoff if present.
 			// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
-			if ra := resp.Header.Get("Retry-After"); ra != "" {
+			if ra := retryAfterHeader; ra != "" {
 				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
 					delay := time.Duration(seconds) * time.Second
 					if delay > maxRetryAfter {
@@ -309,7 +326,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
 			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 nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes)
 		}
 		return body, true, nil
 	}

github/client_test.go

@@ -83,7 +83,9 @@ func TestDoRequest_429Retry(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
+	if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
@@ -108,7 +110,9 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
@@ -218,7 +222,9 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
 	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})
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	start := time.Now()
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -259,7 +265,9 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
@@ -297,7 +305,9 @@ func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	start := time.Now()
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -338,7 +348,9 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
+	if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
 
 	start := time.Now()
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -554,3 +566,29 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
 		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
 	}
 }
+
+
+func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
+	c := NewClient("token", "https://api.github.com")
+
+	// Too short
+	err := c.SetRetryBackoff([]time.Duration{1 * time.Second})
+	if err == nil {
+		t.Fatal("expected error for backoff length 1")
+	}
+	if !strings.Contains(err.Error(), "backoff length 1") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+
+	// Too long
+	err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second, 3 * time.Second})
+	if err == nil {
+		t.Fatal("expected error for backoff length 3")
+	}
+
+	// Correct length succeeds
+	err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second})
+	if err != nil {
+		t.Fatalf("unexpected error for valid backoff: %v", err)
+	}
+}

github/conformance_test.go

@@ -5,9 +5,6 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 
-// Compile-time interface conformance assertions.
-// These verify github.Client satisfies vcs.PRReader and vcs.FileReader.
-var (
-	_ vcs.PRReader   = (*github.Client)(nil)
-	_ vcs.FileReader = (*github.Client)(nil)
-)
+// Compile-time interface conformance assertion.
+// Verifies github.Client satisfies vcs.PRReader.
+var _ vcs.PRReader = (*github.Client)(nil)

github/files.go

@@ -6,32 +6,28 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/url"
+	"path"
 	"strings"
-
-	"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.
-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) {
+// Returns an error if the path contains dot-segments (".", "..") or
+// attempts to traverse above the repository root.
+func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
+	escaped, err := escapePath(filePath)
+	if err != nil {
+		return "", fmt.Errorf("invalid file path: %w", err)
+	}
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escaped)
 	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)
+		return "", fmt.Errorf("fetch file %s: %w", filePath, err)
 	}
 	var resp struct {
 		Content  string `json:"content"`
@@ -41,95 +37,64 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref
 		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)
+		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, filePath)
 	}
 	decoded, err := decodeBase64Content(resp.Content)
 	if err != nil {
-		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
+		return "", fmt.Errorf("decode base64 content for %s: %w", filePath, 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))
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return nil, fmt.Errorf("list contents %s: %w", path, err)
-	}
-
-	type entry 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: %v; 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}
-	}
-
-	result := make([]vcs.ContentEntry, len(entries))
-	for i, e := range entries {
-		result[i] = vcs.ContentEntry{
-			Name: e.Name,
-			Path: e.Path,
-			Type: e.Type,
+// escapePath validates and encodes a slash-separated file path for use in
+// GitHub API URLs. Returns an error if the path contains dot-segments ("."
+// or "..") or resolves to a path outside the repository root.
+func escapePath(p string) (string, error) {
+	// Reject paths containing dot-segments rather than silently rewriting them.
+	for _, seg := range strings.Split(p, "/") {
+		if seg == "." || seg == ".." {
+			return "", fmt.Errorf("path contains dot-segment %q: %s", seg, p)
 		}
 	}
-	return result, nil
-}
 
-// 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
+	// Use path.Clean for canonical form, then verify it doesn't escape root.
+	cleaned := path.Clean(p)
+	if cleaned == "." || strings.HasPrefix(cleaned, "..") {
+		return "", fmt.Errorf("path resolves outside repository root: %s", p)
+	}
+
+	// Encode each segment individually.
+	parts := strings.Split(cleaned, "/")
+	var encoded []string
 	for _, part := range parts {
-		if part == "." || part == ".." || part == "" {
+		if part == "" {
 			continue
 		}
-		clean = append(clean, url.PathEscape(part))
+		encoded = append(encoded, url.PathEscape(part))
 	}
-	return strings.Join(clean, "/")
+	return strings.Join(encoded, "/"), nil
 }
 
+// maxFileContentSize is the maximum decoded file size (10 MB) to prevent
+// resource exhaustion when decoding base64 content from the API.
+const maxFileContentSize = 10 * 1024 * 1024
+
 // 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.
+// Returns an error if the decoded content exceeds maxFileContentSize.
 func decodeBase64Content(encoded string) (string, error) {
-	// GitHub inserts newlines in base64 content
 	cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
+	// Check estimated decoded size before allocating.
+	// Base64 encodes 3 bytes into 4 chars, so decoded ~ len*3/4.
+	if len(cleaned)*3/4 > maxFileContentSize {
+		return "", fmt.Errorf("file content too large: estimated %d bytes exceeds limit of %d", len(cleaned)*3/4, maxFileContentSize)
+	}
 	decoded, err := base64.StdEncoding.DecodeString(cleaned)
 	if err != nil {
 		return "", err
 	}
+	if len(decoded) > maxFileContentSize {
+		return "", fmt.Errorf("file content too large: %d bytes exceeds limit of %d", len(decoded), maxFileContentSize)
+	}
 	return string(decoded), nil
 }

github/files_test.go

@@ -2,333 +2,95 @@ package github
 
 import (
 	"context"
-	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
-	"time"
 )
 
-func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
-	var gotRef string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		gotRef = r.URL.Query().Get("ref")
-		json.NewEncoder(w).Encode(map[string]string{
-			"content":  "dGVzdA==", // "test" in base64
-			"encoding": "base64",
-		})
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	// Call with empty ref — should not include ref param
-	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if content != "test" {
-		t.Errorf("expected 'test', got %q", content)
-	}
-	if gotRef != "" {
-		t.Errorf("expected empty ref, got %q", gotRef)
-	}
-}
-
-func TestGetFileContent_WithRef(t *testing.T) {
-	var gotRef string
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		gotRef = r.URL.Query().Get("ref")
-		json.NewEncoder(w).Encode(map[string]string{
-			"content":  "dGVzdA==",
-			"encoding": "base64",
-		})
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if gotRef != "abc123" {
-		t.Errorf("expected ref 'abc123', got %q", gotRef)
-	}
-}
-
-func TestGetFileContent_404(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(404)
-		w.Write([]byte(`{"message":"Not Found"}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.GetFileContent(context.Background(), "owner", "repo", "missing.go", "")
-	if err == nil {
-		t.Fatal("expected error for 404")
-	}
-}
-
-func TestGetFileContent_401(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-}
-
-func TestGetFileContent_429Retry(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		json.NewEncoder(w).Encode(map[string]string{
-			"content":  "b2s=",
-			"encoding": "base64",
-		})
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
-
-	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if content != "ok" {
-		t.Errorf("expected 'ok', got %q", content)
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-}
-
-func TestGetFileContent_MalformedJSON(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(200)
-		w.Write([]byte(`not json`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
-	if err == nil {
-		t.Fatal("expected error for malformed JSON")
-	}
-}
-
-func TestListContents_HappyPath(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/contents/src" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
-		}
-		json.NewEncoder(w).Encode([]map[string]string{
-			{"name": "main.go", "path": "src/main.go", "type": "file"},
-			{"name": "lib", "path": "src/lib", "type": "dir"},
-		})
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	entries, err := c.ListContents(context.Background(), "owner", "repo", "src")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(entries) != 2 {
-		t.Fatalf("expected 2 entries, got %d", len(entries))
-	}
-	if entries[0].Name != "main.go" {
-		t.Errorf("expected name 'main.go', got %q", entries[0].Name)
-	}
-	if entries[0].Path != "src/main.go" {
-		t.Errorf("expected path 'src/main.go', got %q", entries[0].Path)
-	}
-	if entries[0].Type != "file" {
-		t.Errorf("expected type 'file', got %q", entries[0].Type)
-	}
-	if entries[1].Name != "lib" {
-		t.Errorf("expected name 'lib', got %q", entries[1].Name)
-	}
-	if entries[1].Type != "dir" {
-		t.Errorf("expected type 'dir', got %q", entries[1].Type)
-	}
-}
-
-func TestListContents_404(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(404)
-		w.Write([]byte(`{"message":"Not Found"}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.ListContents(context.Background(), "owner", "repo", "missing")
-	if err == nil {
-		t.Fatal("expected error for 404")
-	}
-}
-
-func TestListContents_401(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-}
-
-func TestListContents_429Retry(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.WriteHeader(429)
-			w.Write([]byte(`{"message":"rate limit"}`))
-			return
-		}
-		json.NewEncoder(w).Encode([]map[string]string{
-			{"name": "file.go", "path": "file.go", "type": "file"},
-		})
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
-
-	entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(entries) != 1 {
-		t.Fatalf("expected 1 entry, got %d", len(entries))
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-}
-
-func TestListContents_MalformedJSON(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(200)
-		w.Write([]byte(`not json`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-
-	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
-	if err == nil {
-		t.Fatal("expected error for malformed JSON")
-	}
-}
-
-func TestDecodeBase64Content(t *testing.T) {
-	// Test with newlines (GitHub's format)
-	encoded := "cGFja2FnZSBt\nYWlu"
-	decoded, err := decodeBase64Content(encoded)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if decoded != "package main" {
-		t.Errorf("expected 'package main', got %q", decoded)
-	}
-}
-
-func TestDecodeBase64Content_Invalid(t *testing.T) {
-	_, err := decodeBase64Content("not!!!valid!!!base64")
-	if err == nil {
-		t.Fatal("expected error for invalid base64")
-	}
-}
-
-func TestEscapePath_RejectsDotSegments(t *testing.T) {
+func TestEscapePath_ValidPaths(t *testing.T) {
+	t.Parallel()
 	tests := []struct {
-		input string
-		want  string
+		name string
+		path 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"},
+		{"simple file", "file.go", "file.go"},
+		{"nested path", "path/to/file.go", "path/to/file.go"},
+		{"special chars", "path/to/my file.go", "path/to/my%20file.go"},
+		{"leading slash stripped", "/path/to/file.go", "path/to/file.go"},
 	}
 	for _, tt := range tests {
-		got := escapePath(tt.input)
-		if got != tt.want {
-			t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := escapePath(tt.path)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if got != tt.want {
+				t.Errorf("escapePath(%q) = %q, want %q", tt.path, 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)
+func TestEscapePath_DotSegments(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name string
+		path string
+	}{
+		{"single dot", "./file.go"},
+		{"double dot", "../file.go"},
+		{"dot in middle", "path/./file.go"},
+		{"parent traversal", "path/../file.go"},
+		{"only dots", ".."},
+		{"nested parent traversal", "a/b/../../c"},
 	}
-	if decoded != "hello world" {
-		t.Errorf("expected 'hello world', got %q", decoded)
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			_, err := escapePath(tt.path)
+			if err == nil {
+				t.Fatalf("expected error for path %q, got nil", tt.path)
+			}
+			if !strings.Contains(err.Error(), "dot-segment") {
+				t.Errorf("expected error about dot-segment, got: %v", err)
+			}
+		})
 	}
 }
 
-func TestListContents_SingleFile(t *testing.T) {
-	// GitHub Contents API returns a JSON object (not array) for single-file paths
+func TestGetFileContentAtRef_DotSegmentError(t *testing.T) {
+	// Server should never be called — the error is caught before the request.
 	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"}`))
+		t.Fatal("server should not have been called")
 	}))
 	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)
+	c := NewClient("token", srv.URL)
+	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
+	if err == nil {
+		t.Fatal("expected error for path with dot-segments")
 	}
-	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)
+	if !strings.Contains(err.Error(), "invalid file path") {
+		t.Errorf("expected 'invalid file path' error, got: %v", err)
 	}
 }
+
+func TestDecodeBase64Content_SizeLimit(t *testing.T) {
+	t.Parallel()
+	// Create base64 content that would decode to > maxFileContentSize.
+	// maxFileContentSize is 10MB. Base64 of 11MB worth of zeros.
+	// We just need something big enough to trigger the estimated size check.
+	// 14MB of base64 chars (decodes to ~10.5MB).
+	huge := strings.Repeat("A", 14*1024*1024)
+	_, err := decodeBase64Content(huge)
+	if err == nil {
+		t.Fatal("expected error for oversized content")
+	}
+	if !strings.Contains(err.Error(), "too large") {
+		t.Errorf("expected 'too large' error, got: %v", err)
+	}
+}
+

github/pr.go

@@ -51,7 +51,10 @@ type checkRunsResponse struct {
 	} `json:"check_runs"`
 }
 
-// GetPullRequest fetches PR metadata.
+// GetPullRequest fetches PR metadata from the GitHub API.
+// Returns an *APIError wrapping the HTTP status on non-2xx responses (e.g.
+// IsNotFound for 404, IsUnauthorized for 401). Network and context errors
+// are wrapped but not typed as *APIError.
 func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.PullRequest, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
@@ -82,9 +85,15 @@ 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
+const (
+	// maxFilesPages is the upper bound on pagination loops for PR file listing,
+	// preventing unbounded iteration if the server always returns a full page.
+	maxFilesPages = 100
+
+	// maxCheckRunPages is the upper bound on pagination loops for check-run listing,
+	// preventing unbounded iteration if the server always returns a full page.
+	maxCheckRunPages = 100
+)
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
 // Paginates through all pages (100 per page) to collect all files.
@@ -93,7 +102,7 @@ const maxPages = 100
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	var allFiles []vcs.ChangedFile
 
-	for page := 1; page <= maxPages; page++ {
+	for page := 1; page <= maxFilesPages; page++ {
 		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)
@@ -154,7 +163,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 	}
 
 	// Fetch check runs (paginated)
-	for checkPage := 1; checkPage <= maxPages; checkPage++ {
+	for checkPage := 1; checkPage <= maxCheckRunPages; checkPage++ {
 		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)
@@ -169,7 +178,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 			result = append(result, vcs.CommitStatus{
 				Context:     cr.Name,
 				Status:      mapCheckRunStatus(cr.Conclusion),
-				Description: derefString(cr.Conclusion),
+				Description: "", // check runs have no human-readable description; conclusion is captured in Status
 				TargetURL:   cr.HTMLURL,
 			})
 		}
@@ -181,9 +190,17 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 	return result, nil
 }
 
-// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
+// mapCheckRunStatus maps a GitHub 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.
+//
+// Mapping rules:
+//   - nil → "pending" (run still in progress or queued)
+//   - "success" → "success"
+//   - "failure", "action_required", "timed_out" → "failure"
+//   - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics)
+//   - "stale" → "pending" (check run became stale before completing)
+//   - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
 func mapCheckRunStatus(conclusion *string) string {
 	if conclusion == nil {
 		// Still running or queued
@@ -196,17 +213,10 @@ func mapCheckRunStatus(conclusion *string) string {
 		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":
+	case "stale":
 		return "pending"
 	default:
 		return "pending"
 	}
 }
 
-// derefString safely dereferences a string pointer, returning empty string if nil.
-func derefString(s *string) string {
-	if s == nil {
-		return ""
-	}
-	return *s
-}

github/pr_test.go

@@ -545,6 +545,7 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
 			name = *tt.conclusion
 		}
 		t.Run(name, func(t *testing.T) {
+			t.Parallel()
 			srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				if strings.Contains(r.URL.Path, "/status") {
 					json.NewEncoder(w).Encode(map[string]interface{}{
@@ -632,6 +633,44 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
 	}
 }
 
+func TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case strings.Contains(r.URL.Path, "/status"):
+			// Statuses succeed
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"state": "success",
+				"statuses": []map[string]string{
+					{
+						"context":     "ci/build",
+						"state":       "success",
+						"description": "Build passed",
+						"target_url":  "https://ci.example.com/1",
+					},
+				},
+			})
+		case strings.Contains(r.URL.Path, "/check-runs"):
+			// Check runs fail with 500
+			w.WriteHeader(500)
+			w.Write([]byte(`{"message":"Internal Server Error"}`))
+		default:
+			w.WriteHeader(404)
+		}
+	}))
+	defer srv.Close()
+
+	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+
+	_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
+	if err == nil {
+		t.Fatal("expected error when check-runs endpoint fails after statuses succeed")
+	}
+	if !strings.Contains(err.Error(), "fetch check runs") {
+		t.Errorf("expected check runs error, got: %v", err)
+	}
+}
+
 func stringPtr(s string) *string {
 	return &s
 }