feat(github): implement FileReader interface (#80)

Implement FileReader conformance on the GitHub client: GetFileContent,
ListContents, path helpers, base64 decode. Includes compile-time
conformance checks for both PRReader and FileReader.

Requires PR B (#102). Part 3 of 3 for #80.

gitea/position.go

@@ -11,7 +11,6 @@ import (
 type PositionMap struct {
 	// files maps filename → (position → new-file line number).
 	// Deletion lines are mapped to -1 (no new-file line).
-	// Hunk-header lines are mapped to 0 (no new-file line).
 	files map[string]map[int]int
 	// maxPositions caches the highest position number per file,
 	// tracked during construction to avoid O(n) scans at translate time.
@@ -20,8 +19,8 @@ type PositionMap struct {
 
 // Translate converts a GitHub diff-position to a new-file line number for a given file.
 // Returns an error if the file is not in the diff or the position is out of range.
-// If the position targets a deletion or hunk-header line, it maps to the nearest
+// If the position targets a deletion line, it maps to the nearest non-deletion line below;
-// context/addition line below; if no such line exists, returns an error.
+// if no such line exists, returns an error.
 func (pm *PositionMap) Translate(file string, position int) (int, error) {
 	if pm == nil || pm.files == nil {
 		return 0, fmt.Errorf("empty position map")
@@ -42,18 +41,14 @@ func (pm *PositionMap) Translate(file string, position int) (int, error) {
 	}
 
 	// lineNum == -1 means this position is a deletion line.
-	// lineNum == 0 means this position is a hunk-header line.
+	// Map to the nearest non-deletion line below.
-	// Both map to the nearest context/addition line below.
+	if lineNum == -1 {
-	if lineNum <= 0 {
 		maxPos := pm.maxPosition(file)
 		for p := position + 1; p <= maxPos; p++ {
 			if ln, exists := fileMap[p]; exists && ln > 0 {
 				return ln, nil
 			}
 		}
-		if lineNum == 0 {
-			return 0, fmt.Errorf("position %d targets a hunk-header line with no subsequent new-file line in %q", position, file)
-		}
 		return 0, fmt.Errorf("position %d targets a deletion line with no subsequent new-file line in %q", position, file)
 	}
 
@@ -75,7 +70,6 @@ func (pm *PositionMap) maxPosition(file string) int {
 //   - A new @@ hunk within the same file continues incrementing (does not reset)
 //   - Position maps to the new file line number for additions and context lines
 //   - Deletion lines have a position but no new-file line number (stored as -1)
-//   - Hunk-header lines have a position but no new-file line number (stored as 0)
 func BuildPositionToLineMap(diff string) *PositionMap {
 	pm := &PositionMap{
 		files:        make(map[string]map[int]int),
@@ -132,7 +126,6 @@ func BuildPositionToLineMap(diff string) *PositionMap {
 		// Parse hunk headers
 		if strings.HasPrefix(line, "@@") && currentFile != "" {
 			position++
-			pm.files[currentFile][position] = 0 // sentinel: hunk-header has no new-file line
 			pm.maxPositions[currentFile] = position
 			newLine = parseHunkStart(line)
 			continue

gitea/position_test.go

@@ -272,112 +272,3 @@ diff --git a/b.go b/b.go
 		t.Errorf("Translate(b.go, 3) = %d, want 2", got)
 	}
 }
-
-func TestTranslate_HunkHeaderPosition_SingleHunk(t *testing.T) {
-	// Position 1 is the @@ hunk-header line.
-	// It should resolve to the first context/addition line below (new line 16).
-	diff := `diff --git a/file.go b/file.go
-index abc..def 100644
---- a/file.go
-+++ b/file.go
-@@ -16,4 +16,5 @@ func example() {
- context line
--deleted line
-+added line
- context after
-`
-	pm := BuildPositionToLineMap(diff)
-
-	got, err := pm.Translate("file.go", 1)
-	if err != nil {
-		t.Fatalf("Translate(file.go, 1): unexpected error: %v", err)
-	}
-	if got != 16 {
-		t.Errorf("Translate(file.go, 1) = %d, want 16 (first context/addition line in hunk)", got)
-	}
-}
-
-func TestTranslate_HunkHeaderPosition_MultiHunk(t *testing.T) {
-	// First hunk: @@ is pos 1, then " line1" (pos 2), "-old" (pos 3), "+new" (pos 4)
-	// Second hunk: @@ is pos 5, then " func foo() {" (pos 6), "+// added" (pos 7), etc.
-	// Translating position 5 (second @@) should resolve to new line 10.
-	diff := `diff --git a/file.go b/file.go
---- a/file.go
-+++ b/file.go
-@@ -1,3 +1,3 @@ package main
- line1
--old
-+new
-@@ -10,3 +10,4 @@ func foo() {
- func foo() {
-+	// added
- 	return
- }
-`
-	pm := BuildPositionToLineMap(diff)
-
-	// Position 5 is the second @@ hunk-header — should resolve to new line 10
-	got, err := pm.Translate("file.go", 5)
-	if err != nil {
-		t.Fatalf("Translate(file.go, 5): unexpected error: %v", err)
-	}
-	if got != 10 {
-		t.Errorf("Translate(file.go, 5) = %d, want 10 (first context/addition line in second hunk)", got)
-	}
-
-	// Also verify first hunk header at position 1 resolves to new line 1
-	got, err = pm.Translate("file.go", 1)
-	if err != nil {
-		t.Fatalf("Translate(file.go, 1): unexpected error: %v", err)
-	}
-	if got != 1 {
-		t.Errorf("Translate(file.go, 1) = %d, want 1 (first context/addition line in first hunk)", got)
-	}
-}
-
-func TestTranslate_HunkHeaderPosition_NewFile(t *testing.T) {
-	// New file: @@ -0,0 +1,3 @@ is position 1.
-	// Should resolve to new line 1 (the first addition).
-	diff := `diff --git a/new.go b/new.go
-new file mode 100644
---- /dev/null
-+++ b/new.go
-@@ -0,0 +1,3 @@
-+package main
-+
-+func init() {}
-`
-	pm := BuildPositionToLineMap(diff)
-
-	got, err := pm.Translate("new.go", 1)
-	if err != nil {
-		t.Fatalf("Translate(new.go, 1): unexpected error: %v", err)
-	}
-	if got != 1 {
-		t.Errorf("Translate(new.go, 1) = %d, want 1 (first addition line)", got)
-	}
-}
-
-func TestTranslate_HunkHeaderAtEnd(t *testing.T) {
-	// A hunk-header at the last position with no subsequent new-file line should error.
-	// This is the hunk-header equivalent of TestBuildPositionToLineMap_DeletionAtEnd.
-	diff := `diff --git a/file.go b/file.go
---- a/file.go
-+++ b/file.go
-@@ -1,2 +1,2 @@ package main
- line1
--old
-+new
-@@ -10,2 +10,1 @@ func foo() {
--removed
-`
-	pm := BuildPositionToLineMap(diff)
-
-	// Position 5 is the second @@ hunk-header; the only line after it (pos 6) is a
-	// deletion (lineNum == -1), so there's no positive new-file line to resolve to.
-	// The hunk-header lookup should fail.
-	_, err := pm.Translate("file.go", 5)
-	if err == nil {
-		t.Error("expected error for hunk-header at end with no subsequent new-file line")
-	}
-}

github/client.go

@@ -4,7 +4,6 @@
 package github
 
 import (
-	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -193,25 +192,10 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
 	return nil
 }
 
-// requestOptions holds per-request configuration for doRequestCore.
+// doRequest performs an HTTP request with retry on 429 rate limit responses.
-type requestOptions struct {
+// It respects the Retry-After header when present (capped at maxRetryAfter).
-	// bodyFn returns a fresh io.Reader for the request body on each attempt.
+// Transport errors (network failures, context cancellation) are not retried.
-	// Must be non-nil for any request that carries a body (POST, PUT, PATCH,
+func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
-	// or DELETE when a body is required by the API).
-	// Returning a fresh reader on each call allows retries to re-send the body.
-	bodyFn func() io.Reader
-
-	// accept overrides the default Accept header. Empty means "application/vnd.github+json".
-	accept string
-
-	// extraHeaders are additional headers to set on each request attempt.
-	extraHeaders map[string]string
-}
-
-// doRequestCore is the shared implementation for all HTTP requests with retry
-// on 429 rate limit responses. It respects the Retry-After header when present
-// (capped at maxRetryAfter). Transport errors are not retried.
-func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
 	const maxRetryAfter = 120 * time.Second
 
 	// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
@@ -262,11 +246,7 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
 			}
 		}
 
-		var body io.Reader
+		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
-		if opts.bodyFn != nil {
-			body = opts.bodyFn()
-		}
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, body)
 		if err != nil {
 			return nil, fmt.Errorf("create request: %w", err)
 		}
@@ -277,14 +257,11 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
 			req.Header.Set("Authorization", "Bearer "+c.token)
 		}
 		req.Header.Set("User-Agent", userAgent)
-		if opts.accept != "" {
+		if accept != "" {
-			req.Header.Set("Accept", opts.accept)
+			req.Header.Set("Accept", accept)
 		} else {
 			req.Header.Set("Accept", "application/vnd.github+json")
 		}
-		for k, v := range opts.extraHeaders {
-			req.Header.Set(k, v)
-		}
 
 		resp, err := c.httpClient.Do(req)
 		if err != nil {
@@ -295,11 +272,11 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
 		respStatus := resp.StatusCode
 		retryAfterHeader := resp.Header.Get("Retry-After")
 
-		respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
+		body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		if done {
-			return respBody, handleErr
+			return body, err
 		}
-		lastErr = handleErr
+		lastErr = err
 
 		// Retry on 429 rate limit
 		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
@@ -337,13 +314,6 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
 	return nil, lastErr
 }
 
-// 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) {
-	return c.doRequestCore(ctx, method, reqURL, requestOptions{accept: accept})
-}
-
 // 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.
@@ -372,15 +342,3 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 	return c.doRequest(ctx, http.MethodGet, reqURL, "")
 }
-
-// doRequestWithBody is like doRequest but sends a request body.
-// It accepts the raw body bytes and sets Content-Type to application/json.
-// Retry semantics match doRequest (retries on 429 with Retry-After support).
-func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, reqBody []byte) ([]byte, error) {
-	var opts requestOptions
-	if reqBody != nil {
-		opts.bodyFn = func() io.Reader { return bytes.NewReader(reqBody) }
-		opts.extraHeaders = map[string]string{"Content-Type": "application/json"}
-	}
-	return c.doRequestCore(ctx, method, reqURL, opts)
-}

github/conformance_test.go

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

github/files.go

@@ -6,7 +6,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/url"
-	"path"
 	"strings"
 
 	"gitea.weiker.me/rodin/review-bot/vcs"
@@ -14,28 +13,25 @@ import (
 
 // 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, filePath, ref string) (string, error) {
+func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
-	return c.GetFileContentAtRef(ctx, owner, repo, filePath, ref)
+	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).
 //
-// Returns an error if the path contains dot-segments (".", "..") or
+// Note: dot-segments ("." and "..") in the path are silently removed to
-// attempts to traverse above the repository root.
+// prevent path traversal. This means a path like "foo/../bar" resolves
-func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
+// to "foo/bar" rather than "bar".
-	escaped, err := escapePath(filePath)
+func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
-	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), escaped)
+		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", filePath, err)
+		return "", fmt.Errorf("fetch file %s: %w", path, err)
 	}
 	var resp struct {
 		Content  string `json:"content"`
@@ -45,11 +41,11 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath,
 		return "", fmt.Errorf("parse file content JSON: %w", err)
 	}
 	if resp.Encoding != "base64" {
-		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, filePath)
+		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", filePath, err)
+		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
 	}
 	return decoded, nil
 }
@@ -59,16 +55,16 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath,
 // If the path points to a single file (not a directory), the API returns
 // a JSON object instead of an array; this is handled by returning a
 // single-element slice.
-func (c *Client) ListContents(ctx context.Context, owner, repo, filePath string) ([]vcs.ContentEntry, error) {
+//
-	escaped, err := escapePath(filePath)
+// Note: dot-segments ("." and "..") in the path are silently removed to
-	if err != nil {
+// prevent path traversal. This means a path like "foo/../bar" resolves
-		return nil, fmt.Errorf("invalid file path: %w", err)
+// 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), escaped)
+		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", filePath, err)
+		return nil, fmt.Errorf("list contents %s: %w", path, err)
 	}
 
 	type entry struct {
@@ -106,55 +102,34 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, filePath string)
 	return result, nil
 }
 
-// escapePath validates and encodes a slash-separated file path for use in
+// escapePath escapes each segment of a relative file path for use in URLs.
-// GitHub API URLs. Returns an error if the path contains dot-segments ("."
+// Slashes are preserved as path separators; other special characters are escaped.
-// or "..") or resolves to a path outside the repository root.
+// Dot-segments ("." and "..") and empty segments (from consecutive slashes like
-func escapePath(p string) (string, error) {
+// "a//b") are silently removed to prevent path traversal and produce canonical
-	// Reject paths containing dot-segments rather than silently rewriting them.
+// paths. This is intentional: callers may receive a different path than requested
-	for _, seg := range strings.Split(p, "/") {
+// without error. The function is package-private, and all callers
-		if seg == "." || seg == ".." {
+// (GetFileContentAtRef, ListContents) already handle missing-file errors from the
-			return "", fmt.Errorf("path contains dot-segment %q: %s", seg, p)
+// 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 == "" {
+		if part == "." || part == ".." || part == "" {
 			continue
 		}
-		encoded = append(encoded, url.PathEscape(part))
+		clean = append(clean, url.PathEscape(part))
 	}
-	return strings.Join(encoded, "/"), nil
+	return strings.Join(clean, "/")
 }
 
-// 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

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
 	"time"
 )
@@ -110,9 +109,7 @@ func TestGetFileContent_429Retry(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
-		t.Fatalf("SetRetryBackoff: %v", err)
-	}
 
 	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err != nil {
@@ -230,11 +227,9 @@ func TestListContents_429Retry(t *testing.T) {
 
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
-		t.Fatalf("SetRetryBackoff: %v", err)
-	}
 
-	entries, err := c.ListContents(context.Background(), "owner", "repo", "src")
+	entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -262,6 +257,57 @@ func TestListContents_MalformedJSON(t *testing.T) {
 	}
 }
 
+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) {
+	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) {
@@ -286,120 +332,3 @@ func TestListContents_SingleFile(t *testing.T) {
 		t.Errorf("expected type 'file', got %q", entries[0].Type)
 	}
 }
-
-func TestEscapePath_ValidPaths(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name string
-		path string
-		want string
-	}{
-		{"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 {
-		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 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"},
-	}
-	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 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) {
-		t.Fatal("server should not have been called")
-	}))
-	defer srv.Close()
-
-	c := NewClient("token", srv.URL, AllowInsecureHTTP())
-	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
-	if err == nil {
-		t.Fatal("expected error for path with dot-segments")
-	}
-	if !strings.Contains(err.Error(), "invalid file path") {
-		t.Errorf("expected 'invalid file path' error, got: %v", err)
-	}
-}
-
-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 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 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/helpers_test.go

@@ -1,23 +0,0 @@
-package github
-
-import (
-	"net/http"
-	"net/http/httptest"
-	"testing"
-	"time"
-)
-
-// newTestClient creates a *Client backed by an httptest.Server running the
-// given handler. The server is automatically closed when the test finishes.
-// Shared across test files in package github.
-func newTestClient(t *testing.T, handler http.HandlerFunc) *Client {
-	t.Helper()
-	srv := httptest.NewServer(handler)
-	t.Cleanup(srv.Close)
-	c := NewClient("test-token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
-		t.Fatalf("SetRetryBackoff: %v", err)
-	}
-	return c
-}

github/identity.go

@@ -1,29 +0,0 @@
-package github
-
-import (
-	"context"
-	"encoding/json"
-	"fmt"
-)
-
-// userResponse is the GitHub API response for the authenticated user.
-type userResponse struct {
-	Login string `json:"login"`
-}
-
-// GetAuthenticatedUser returns the login of the currently authenticated user.
-func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
-	reqURL := fmt.Sprintf("%s/user", c.baseURL)
-
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return "", fmt.Errorf("get authenticated user: %w", err)
-	}
-
-	var resp userResponse
-	if err := json.Unmarshal(body, &resp); err != nil {
-		return "", fmt.Errorf("parse user response: %w", err)
-	}
-
-	return resp.Login, nil
-}

github/identity_test.go

@@ -1,46 +0,0 @@
-package github
-
-import (
-	"context"
-	"encoding/json"
-	"net/http"
-	"testing"
-)
-
-func TestGetAuthenticatedUser_HappyPath(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "GET" {
-			t.Errorf("expected GET, got %s", r.Method)
-		}
-		if r.URL.Path != "/user" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
-		}
-		if r.Header.Get("Authorization") != "Bearer test-token" {
-			t.Errorf("unexpected auth header: %s", r.Header.Get("Authorization"))
-		}
-		json.NewEncoder(w).Encode(map[string]string{"login": "review-bot"})
-	})
-
-	login, err := c.GetAuthenticatedUser(context.Background())
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if login != "review-bot" {
-		t.Errorf("expected login 'review-bot', got %q", login)
-	}
-}
-
-func TestGetAuthenticatedUser_401(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	})
-
-	_, err := c.GetAuthenticatedUser(context.Background())
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-	if !IsUnauthorized(err) {
-		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
-	}
-}

github/pr.go

@@ -51,10 +51,7 @@ type checkRunsResponse struct {
 	} `json:"check_runs"`
 }
 
-// GetPullRequest fetches PR metadata from the GitHub API.
+// GetPullRequest fetches PR metadata.
-// 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)
@@ -85,15 +82,9 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
 	return string(body), nil
 }
 
-const (
+// maxPages is the upper bound on pagination loops to prevent unbounded iteration
-	// maxFilesPages is the upper bound on pagination loops for PR file listing,
+// in case the server returns a full page indefinitely.
-	// preventing unbounded iteration if the server always returns a full page.
+const maxPages = 100
-	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.
@@ -102,7 +93,7 @@ const (
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	var allFiles []vcs.ChangedFile
 
-	for page := 1; page <= maxFilesPages; page++ {
+	for page := 1; page <= maxPages; 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)
@@ -163,7 +154,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 	}
 
 	// Fetch check runs (paginated)
-	for checkPage := 1; checkPage <= maxCheckRunPages; checkPage++ {
+	for checkPage := 1; checkPage <= maxPages; 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)
@@ -178,7 +169,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: "", // check runs have no human-readable description; conclusion is captured in Status
+				Description: derefString(cr.Conclusion),
 				TargetURL:   cr.HTMLURL,
 			})
 		}
@@ -190,17 +181,9 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 	return result, nil
 }
 
-// mapCheckRunStatus maps a GitHub check run conclusion to a vcs.CommitStatus status string.
+// 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.
-//
-// 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
@@ -213,10 +196,17 @@ 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":
+	case "stale", "waiting":
 		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,7 +545,6 @@ 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{}{
@@ -633,44 +632,6 @@ 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
 }

github/review.go

@@ -1,212 +0,0 @@
-package github
-
-import (
-	"context"
-	"encoding/json"
-	"errors"
-	"fmt"
-	"net/http"
-	"net/url"
-
-	"gitea.weiker.me/rodin/review-bot/vcs"
-)
-
-// ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on
-// a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT).
-// GitHub only allows deletion of PENDING reviews. Callers that need to replace
-// a submitted review should use DismissReview instead.
-var ErrCannotDeleteSubmittedReview = errors.New("cannot delete submitted review: use DismissReview instead")
-
-// ErrConflictingCommitIDs is returned when PostReview receives comments with
-// differing non-empty CommitIDs. The GitHub API accepts only a single commit_id
-// per review submission; callers must ensure all comments target the same commit.
-var ErrConflictingCommitIDs = errors.New("comments contain conflicting commit IDs: all must target the same commit")
-
-// postReviewRequest is the GitHub API request body for creating a review.
-type postReviewRequest struct {
-	CommitID string               `json:"commit_id,omitempty"`
-	Body     string               `json:"body"`
-	Event    string               `json:"event"`
-	Comments []reviewCommentEntry `json:"comments,omitempty"`
-}
-
-// reviewCommentEntry is a single inline comment in a review creation request.
-type reviewCommentEntry struct {
-	Path     string `json:"path"`
-	Position int    `json:"position"`
-	Body     string `json:"body"`
-}
-
-// reviewResponse is the GitHub API response for a review.
-type reviewResponse struct {
-	ID       int64  `json:"id"`
-	Body     string `json:"body"`
-	State    string `json:"state"`
-	CommitID string `json:"commit_id"`
-	User     struct {
-		Login string `json:"login"`
-	} `json:"user"`
-}
-
-// dismissReviewRequest is the GitHub API request body for dismissing a review.
-type dismissReviewRequest struct {
-	Message string `json:"message"`
-	Event   string `json:"event"`
-}
-
-// translateGitHubReviewState translates a GitHub API review state to the
-// canonical vcs.Review.State value.
-func translateGitHubReviewState(state string) string {
-	switch state {
-	case "CHANGES_REQUESTED":
-		return "REQUEST_CHANGES"
-	case "COMMENTED":
-		return "COMMENT"
-	default:
-		// States like APPROVED, DISMISSED, and PENDING pass through unchanged
-		// as they already match the canonical vcs representation. PENDING appears
-		// on draft reviews that have not yet been submitted via the GitHub UI or API.
-		return state
-	}
-}
-
-// PostReview submits a review on a pull request.
-//
-// The vcs.ReviewEvent constants (ReviewEventApprove, ReviewEventRequestChanges,
-// ReviewEventComment) have string values that match GitHub's wire-format event
-// strings (APPROVE, REQUEST_CHANGES, COMMENT), so Event is cast directly to
-// string without translation.
-//
-// ReviewComment.Position maps directly to the GitHub API position field.
-// When req.Comments is empty, the payload omits the comments field entirely
-// (via the omitempty tag on postReviewRequest.Comments).
-//
-// The GitHub API accepts a single commit_id per review submission. PostReview
-// extracts it from the first comment with a non-empty CommitID. If any subsequent
-// comment specifies a different CommitID, PostReview returns ErrConflictingCommitIDs.
-// Comments with an empty CommitID are allowed and inherit the review-level value.
-func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
-
-	payload := postReviewRequest{
-		Body:  req.Body,
-		Event: string(req.Event),
-	}
-
-	// Build the payload in one pass. The GitHub API accepts a single commit_id
-	// per review; we extract it from the first comment that supplies one and
-	// reject the request if any other comment disagrees.
-	for _, comment := range req.Comments {
-		if comment.CommitID != "" {
-			if payload.CommitID == "" {
-				payload.CommitID = comment.CommitID
-			} else if payload.CommitID != comment.CommitID {
-				return nil, ErrConflictingCommitIDs
-			}
-		}
-		payload.Comments = append(payload.Comments, reviewCommentEntry{
-			Path:     comment.Path,
-			Position: comment.Position,
-			Body:     comment.Body,
-		})
-	}
-
-	data, err := json.Marshal(payload)
-	if err != nil {
-		return nil, fmt.Errorf("marshal review request: %w", err)
-	}
-
-	body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
-	if err != nil {
-		return nil, fmt.Errorf("post review: %w", err)
-	}
-
-	var resp reviewResponse
-	if err := json.Unmarshal(body, &resp); err != nil {
-		return nil, fmt.Errorf("parse review response: %w", err)
-	}
-
-	return &vcs.Review{
-		ID:       resp.ID,
-		Body:     resp.Body,
-		User:     vcs.UserInfo{Login: resp.User.Login},
-		State:    translateGitHubReviewState(resp.State),
-		CommitID: resp.CommitID,
-	}, nil
-}
-
-// ListReviews retrieves all reviews for a pull request.
-// GitHub review states are translated to canonical vcs values.
-func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
-
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return nil, fmt.Errorf("list reviews: %w", err)
-	}
-
-	var responses []reviewResponse
-	if err := json.Unmarshal(body, &responses); err != nil {
-		return nil, fmt.Errorf("parse reviews response: %w", err)
-	}
-
-	reviews := make([]vcs.Review, len(responses))
-	for i, r := range responses {
-		reviews[i] = vcs.Review{
-			ID:       r.ID,
-			Body:     r.Body,
-			User:     vcs.UserInfo{Login: r.User.Login},
-			State:    translateGitHubReviewState(r.State),
-			CommitID: r.CommitID,
-		}
-	}
-	return reviews, nil
-}
-
-// DeleteReview deletes a pull request review.
-// Only PENDING reviews can be deleted; attempting to delete a submitted review
-// (APPROVED, CHANGES_REQUESTED, or COMMENTED per GitHub API naming) returns
-// ErrCannotDeleteSubmittedReview.
-func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
-
-	// nil body: the GitHub DELETE endpoint for reviews requires no request body.
-	_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
-	if err != nil {
-		var apiErr *APIError
-		if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
-			return fmt.Errorf("delete review: %w", ErrCannotDeleteSubmittedReview)
-		}
-		return fmt.Errorf("delete review: %w", err)
-	}
-	return nil
-}
-
-// DismissReview dismisses a submitted review on a pull request.
-// This is the correct way to "remove" a submitted review (APPROVED, REQUEST_CHANGES).
-// GitHub does not allow deleting submitted reviews — they must be dismissed.
-func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals",
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
-
-	payload := dismissReviewRequest{
-		Message: message,
-		// Event is required by the GitHub API for dismissal requests, even though
-		// "DISMISS" is the only valid value for this endpoint.
-		Event: "DISMISS",
-	}
-
-	data, err := json.Marshal(payload)
-	if err != nil {
-		return fmt.Errorf("marshal dismiss request: %w", err)
-	}
-
-	_, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data)
-	if err != nil {
-		return fmt.Errorf("dismiss review: %w", err)
-	}
-	return nil
-}

github/review_test.go

@@ -1,391 +0,0 @@
-package github
-
-import (
-	"context"
-	"encoding/json"
-	"errors"
-	"io"
-	"net/http"
-	"strings"
-	"testing"
-
-	"gitea.weiker.me/rodin/review-bot/vcs"
-)
-
-// --- PostReview tests ---
-
-func TestPostReview_HappyPath(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "POST" {
-			t.Fatalf("expected POST, got %s", r.Method)
-		}
-		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews" {
-			t.Fatalf("unexpected path: %s", r.URL.Path)
-		}
-		if r.Header.Get("Content-Type") != "application/json" {
-			t.Errorf("expected Content-Type application/json, got %q", r.Header.Get("Content-Type"))
-		}
-
-		// Verify request body
-		body, _ := io.ReadAll(r.Body)
-		var req postReviewRequest
-		if err := json.Unmarshal(body, &req); err != nil {
-			t.Fatalf("unmarshal request: %v", err)
-		}
-		if req.Event != "APPROVE" {
-			t.Errorf("expected event APPROVE, got %q", req.Event)
-		}
-		if req.Body != "LGTM" {
-			t.Errorf("expected body 'LGTM', got %q", req.Body)
-		}
-		if req.CommitID != "abc123" {
-			t.Errorf("expected commit_id 'abc123', got %q", req.CommitID)
-		}
-		if len(req.Comments) != 1 {
-			t.Fatalf("expected 1 comment, got %d", len(req.Comments))
-		}
-		if req.Comments[0].Path != "main.go" {
-			t.Errorf("expected comment path 'main.go', got %q", req.Comments[0].Path)
-		}
-		if req.Comments[0].Position != 4 {
-			t.Errorf("expected comment position 4, got %d", req.Comments[0].Position)
-		}
-
-		json.NewEncoder(w).Encode(map[string]interface{}{
-			"id":        100,
-			"body":      "LGTM",
-			"state":     "APPROVED",
-			"commit_id": "abc123",
-			"user":      map[string]string{"login": "reviewer"},
-		})
-	})
-
-	review, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
-		Body:  "LGTM",
-		Event: vcs.ReviewEventApprove,
-		Comments: []vcs.ReviewComment{
-			{Path: "main.go", Position: 4, CommitID: "abc123", Body: "nit: rename"},
-		},
-	})
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if review.ID != 100 {
-		t.Errorf("expected ID 100, got %d", review.ID)
-	}
-	if review.Body != "LGTM" {
-		t.Errorf("expected body 'LGTM', got %q", review.Body)
-	}
-	if review.State != "APPROVED" {
-		t.Errorf("expected state 'APPROVED', got %q", review.State)
-	}
-	if review.User.Login != "reviewer" {
-		t.Errorf("expected user 'reviewer', got %q", review.User.Login)
-	}
-	if review.CommitID != "abc123" {
-		t.Errorf("expected commit_id 'abc123', got %q", review.CommitID)
-	}
-}
-
-func TestPostReview_401(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	})
-
-	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
-		Body:  "LGTM",
-		Event: vcs.ReviewEventApprove,
-	})
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-	if !IsUnauthorized(err) {
-		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
-	}
-}
-
-func TestPostReview_422(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(422)
-		w.Write([]byte(`{"message":"Unprocessable Entity"}`))
-	})
-
-	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
-		Body:  "LGTM",
-		Event: vcs.ReviewEventApprove,
-	})
-	if err == nil {
-		t.Fatal("expected error for 422")
-	}
-	// 422 should surface as a wrapped APIError
-	var apiErr *APIError
-	if !errors.As(err, &apiErr) {
-		t.Fatalf("expected *APIError, got %T: %v", err, err)
-	}
-	if apiErr.StatusCode != 422 {
-		t.Errorf("expected status 422, got %d", apiErr.StatusCode)
-	}
-}
-
-func TestPostReview_MalformedResponse(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.Write([]byte(`not json`))
-	})
-
-	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
-		Body:  "LGTM",
-		Event: vcs.ReviewEventApprove,
-	})
-	if err == nil {
-		t.Fatal("expected error for malformed response")
-	}
-	if !strings.Contains(err.Error(), "parse review response") {
-		t.Errorf("expected parse error, got: %v", err)
-	}
-}
-
-// --- ListReviews tests ---
-
-func TestListReviews_HappyPath(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "GET" {
-			t.Fatalf("expected GET, got %s", r.Method)
-		}
-		if r.URL.Path != "/repos/owner/repo/pulls/3/reviews" {
-			t.Fatalf("unexpected path: %s", r.URL.Path)
-		}
-		json.NewEncoder(w).Encode([]map[string]interface{}{
-			{
-				"id":        1,
-				"body":      "Approved",
-				"state":     "APPROVED",
-				"commit_id": "sha1",
-				"user":      map[string]string{"login": "user1"},
-			},
-			{
-				"id":        2,
-				"body":      "Needs work",
-				"state":     "CHANGES_REQUESTED",
-				"commit_id": "sha2",
-				"user":      map[string]string{"login": "user2"},
-			},
-			{
-				"id":        3,
-				"body":      "Comment only",
-				"state":     "COMMENTED",
-				"commit_id": "sha3",
-				"user":      map[string]string{"login": "user3"},
-			},
-			{
-				"id":        4,
-				"body":      "Old review",
-				"state":     "DISMISSED",
-				"commit_id": "sha4",
-				"user":      map[string]string{"login": "user4"},
-			},
-		})
-	})
-
-	reviews, err := c.ListReviews(context.Background(), "owner", "repo", 3)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(reviews) != 4 {
-		t.Fatalf("expected 4 reviews, got %d", len(reviews))
-	}
-
-	// Check state translation
-	expected := []struct {
-		id    int64
-		state string
-	}{
-		{1, "APPROVED"},
-		{2, "REQUEST_CHANGES"},
-		{3, "COMMENT"},
-		{4, "DISMISSED"},
-	}
-	for i, e := range expected {
-		if reviews[i].ID != e.id {
-			t.Errorf("review[%d]: expected ID %d, got %d", i, e.id, reviews[i].ID)
-		}
-		if reviews[i].State != e.state {
-			t.Errorf("review[%d]: expected state %q, got %q", i, e.state, reviews[i].State)
-		}
-	}
-}
-
-func TestListReviews_404(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(404)
-		w.Write([]byte(`{"message":"Not Found"}`))
-	})
-
-	_, err := c.ListReviews(context.Background(), "owner", "repo", 999)
-	if err == nil {
-		t.Fatal("expected error for 404")
-	}
-	if !IsNotFound(err) {
-		t.Errorf("expected IsNotFound=true, got error: %v", err)
-	}
-}
-
-func TestListReviews_401(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	})
-
-	_, err := c.ListReviews(context.Background(), "owner", "repo", 3)
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-	if !IsUnauthorized(err) {
-		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
-	}
-}
-
-// --- DeleteReview tests ---
-
-func TestDeleteReview_HappyPath(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "DELETE" {
-			t.Fatalf("expected DELETE, got %s", r.Method)
-		}
-		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/42" {
-			t.Fatalf("unexpected path: %s", r.URL.Path)
-		}
-		w.WriteHeader(204)
-	})
-
-	err := c.DeleteReview(context.Background(), "owner", "repo", 5, 42)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-}
-
-func TestDeleteReview_422_SubmittedReview(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(422)
-		w.Write([]byte(`{"message":"Can not delete a non pending review"}`))
-	})
-
-	err := c.DeleteReview(context.Background(), "owner", "repo", 5, 42)
-	if err == nil {
-		t.Fatal("expected error for 422")
-	}
-	if !errors.Is(err, ErrCannotDeleteSubmittedReview) {
-		t.Errorf("expected ErrCannotDeleteSubmittedReview, got: %v", err)
-	}
-}
-
-// --- DismissReview tests ---
-
-func TestDismissReview_HappyPath(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != "PUT" {
-			t.Fatalf("expected PUT, got %s", r.Method)
-		}
-		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/10/dismissals" {
-			t.Fatalf("unexpected path: %s", r.URL.Path)
-		}
-
-		body, _ := io.ReadAll(r.Body)
-		var req dismissReviewRequest
-		if err := json.Unmarshal(body, &req); err != nil {
-			t.Fatalf("unmarshal request: %v", err)
-		}
-		if req.Message != "Superseded by new review" {
-			t.Errorf("expected message 'Superseded by new review', got %q", req.Message)
-		}
-		if req.Event != "DISMISS" {
-			t.Errorf("expected event 'DISMISS', got %q", req.Event)
-		}
-
-		json.NewEncoder(w).Encode(map[string]interface{}{
-			"id":    10,
-			"state": "DISMISSED",
-		})
-	})
-
-	err := c.DismissReview(context.Background(), "owner", "repo", 5, 10, "Superseded by new review")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-}
-
-func TestDismissReview_404(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(404)
-		w.Write([]byte(`{"message":"Not Found"}`))
-	})
-
-	err := c.DismissReview(context.Background(), "owner", "repo", 5, 999, "dismiss")
-	if err == nil {
-		t.Fatal("expected error for 404")
-	}
-	if !IsNotFound(err) {
-		t.Errorf("expected IsNotFound=true, got error: %v", err)
-	}
-}
-
-func TestDismissReview_401(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		w.WriteHeader(401)
-		w.Write([]byte(`{"message":"Bad credentials"}`))
-	})
-
-	err := c.DismissReview(context.Background(), "owner", "repo", 5, 10, "dismiss")
-	if err == nil {
-		t.Fatal("expected error for 401")
-	}
-	if !IsUnauthorized(err) {
-		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
-	}
-}
-
-// --- State translation tests ---
-
-func TestTranslateGitHubReviewState(t *testing.T) {
-	tests := []struct {
-		name  string
-		input string
-		want  string
-	}{
-		{"approved passes through", "APPROVED", "APPROVED"},
-		{"changes_requested maps to REQUEST_CHANGES", "CHANGES_REQUESTED", "REQUEST_CHANGES"},
-		{"commented maps to COMMENT", "COMMENTED", "COMMENT"},
-		{"dismissed passes through", "DISMISSED", "DISMISSED"},
-		{"unknown state passes through", "UNKNOWN_STATE", "UNKNOWN_STATE"},
-		{"empty string passes through", "", ""},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := translateGitHubReviewState(tt.input)
-			if got != tt.want {
-				t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}
-
-func TestPostReview_ConflictingCommitIDs(t *testing.T) {
-	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
-		t.Fatal("request should not be sent when commit IDs conflict")
-	})
-
-	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
-		Body:  "Review",
-		Event: vcs.ReviewEventComment,
-		Comments: []vcs.ReviewComment{
-			{Path: "a.go", Position: 1, CommitID: "sha-1", Body: "first"},
-			{Path: "b.go", Position: 2, CommitID: "sha-2", Body: "second"},
-		},
-	})
-	if err == nil {
-		t.Fatal("expected error for conflicting commit IDs")
-	}
-	if !errors.Is(err, ErrConflictingCommitIDs) {
-		t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
-	}
-}