fix(github): address review findings - remove panic, validate at config time

- MAJOR #1: Replace panic in doRequest with safe default fallback.
  Validation now happens in SetRetryBackoff (returns error on invalid
  length). doRequest gracefully falls back to default backoff if the
  configured slice is somehow invalid.

- MINOR #2: SetRetryBackoff validates slice length at configuration
  time, making the coupling between maxRetryAttempts and backoff
  explicit and catching mismatches early with a clear error.

- MINOR #4: Reword oversized response error to remove '(truncated)'
  which implied truncated data was returned when actually only an
  error is returned.

- MINOR #5: Functional options kept as-is - idiomatic Go pattern
  that allows future growth without breaking the API.

cmd/review-bot/main.go

@@ -838,9 +838,9 @@ func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path
 	return result, nil
 }
 
-func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
+func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
 	if ref != "" {
-		return a.client.GetFileContentRef(ctx, owner, repo, filepath, ref)
+		return a.client.GetFileContentRef(ctx, owner, repo, filePath, ref)
 	}
-	return a.client.GetFileContent(ctx, owner, repo, filepath)
+	return a.client.GetFileContent(ctx, owner, repo, filePath)
 }

gitea/adapter.go

@@ -0,0 +1,232 @@
+package gitea
+
+import (
+	"context"
+	"fmt"
+
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+// Adapter wraps a gitea.Client and satisfies the vcs.Client interface.
+// It handles translation between GitHub-canonical diff positions and Gitea
+// line numbers, and between canonical review event strings and Gitea-native values.
+type Adapter struct {
+	client *Client
+}
+
+// Compile-time interface conformance assertion.
+var _ vcs.Client = (*Adapter)(nil)
+
+// NewAdapter creates a new Adapter wrapping the given gitea Client.
+func NewAdapter(client *Client) *Adapter {
+	return &Adapter{client: client}
+}
+
+// Underlying returns the wrapped gitea.Client for Gitea-specific operations
+// that have no vcs.Client equivalent (resolve comment, timeline, supersede flow).
+func (a *Adapter) Underlying() *Client {
+	return a.client
+}
+
+// --- PRReader ---
+
+// GetPullRequest maps gitea.PullRequest to vcs.PullRequest.
+func (a *Adapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.PullRequest, error) {
+	pr, err := a.client.GetPullRequest(ctx, owner, repo, number)
+	if err != nil {
+		return nil, fmt.Errorf("get pull request: %w", err)
+	}
+	return &vcs.PullRequest{
+		Number: number,
+		Title:  pr.Title,
+		Body:   pr.Body,
+		Head: vcs.HeadRef{
+			SHA: pr.Head.Sha,
+			Ref: pr.Head.Ref,
+		},
+		Base: vcs.BaseRef{
+			Ref: pr.Base.Ref,
+		},
+	}, nil
+}
+
+// GetPullRequestDiff is a pass-through to the underlying client.
+func (a *Adapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
+	return a.client.GetPullRequestDiff(ctx, owner, repo, number)
+}
+
+// GetPullRequestFiles maps []gitea.ChangedFile to []vcs.ChangedFile.
+// Patch field is omitted (zero-value) since Gitea's /pulls/{n}/files does not return patch text.
+func (a *Adapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
+	files, err := a.client.GetPullRequestFiles(ctx, owner, repo, number)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]vcs.ChangedFile, len(files))
+	for i, f := range files {
+		result[i] = vcs.ChangedFile{
+			Filename: f.Filename,
+			Status:   f.Status,
+		}
+	}
+	return result, nil
+}
+
+// GetFileContentAtRef is a pass-through to the underlying client.
+func (a *Adapter) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
+	return a.client.GetFileContentAtRef(ctx, owner, repo, path, ref)
+}
+
+// GetCommitStatuses maps []gitea.CommitStatus to []vcs.CommitStatus.
+func (a *Adapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
+	statuses, err := a.client.GetCommitStatuses(ctx, owner, repo, sha)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]vcs.CommitStatus, len(statuses))
+	for i, s := range statuses {
+		result[i] = vcs.CommitStatus{
+			Status:      s.Status,
+			Context:     s.Context,
+			Description: s.Description,
+			TargetURL:   s.TargetURL,
+		}
+	}
+	return result, nil
+}
+
+// --- FileReader ---
+
+// GetFileContent delegates to the underlying client, routing to the ref-aware
+// variant when ref is non-empty.
+func (a *Adapter) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
+	if ref != "" {
+		return a.client.GetFileContentRef(ctx, owner, repo, path, ref)
+	}
+	return a.client.GetFileContent(ctx, owner, repo, path)
+}
+
+// ListContents maps []gitea.ContentEntry to []vcs.ContentEntry.
+func (a *Adapter) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
+	entries, err := a.client.ListContents(ctx, owner, repo, path)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]vcs.ContentEntry, len(entries))
+	for i, e := range entries {
+		result[i] = vcs.ContentEntry{
+			Name: e.Name,
+			Path: e.Path,
+			Type: e.Type,
+		}
+	}
+	return result, nil
+}
+
+// --- Reviewer ---
+
+// translateEvent translates a vcs.ReviewEvent (GitHub-canonical) to a Gitea-native event string.
+func translateEvent(event vcs.ReviewEvent) string {
+	switch event {
+	case vcs.ReviewEventApprove:
+		return "APPROVED"
+	case vcs.ReviewEventRequestChanges:
+		return "REQUEST_CHANGES"
+	case vcs.ReviewEventComment:
+		return "COMMENT"
+	default:
+		// Unknown events pass through as-is. This is intentional: new event types
+		// added to vcs.ReviewEvent will still be forwarded without a code change here,
+		// and Gitea will reject truly invalid values with a clear API error.
+		return string(event)
+	}
+}
+
+// PostReview translates vcs.ReviewRequest to the Gitea-native format.
+// It fetches the PR diff, builds a position-to-line map, and translates each
+// ReviewComment.Position (GitHub diff-position) to a Gitea new_position (line number).
+func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
+	event := translateEvent(req.Event)
+
+	var giteaComments []ReviewComment
+	if len(req.Comments) > 0 {
+		// Fetch diff to build position → line number map.
+		// The diff is fetched unconditionally when comments exist. This adds latency
+		// for reviews with inline comments but keeps the implementation simple — caching
+		// the diff across calls would add complexity for minimal gain since PostReview
+		// is called at most once per review cycle.
+		diff, err := a.client.GetPullRequestDiff(ctx, owner, repo, number)
+		if err != nil {
+			return nil, fmt.Errorf("fetch diff for position translation: %w", err)
+		}
+
+		posMap := BuildPositionToLineMap(diff)
+
+		for _, c := range req.Comments {
+			lineNum, err := posMap.Translate(c.Path, c.Position)
+			if err != nil {
+				return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err)
+			}
+			// CommitID from vcs.ReviewComment is intentionally not forwarded:
+			// Gitea review comments are pinned to the PR head SHA automatically,
+			// and the CreatePullReview API has no per-comment commit_id field.
+			giteaComments = append(giteaComments, ReviewComment{
+				Path:        c.Path,
+				NewPosition: int64(lineNum),
+				Body:        c.Body,
+			})
+		}
+	}
+
+	review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments)
+	if err != nil {
+		return nil, fmt.Errorf("post review: %w", err)
+	}
+
+	return &vcs.Review{
+		ID:       review.ID,
+		Body:     review.Body,
+		User:     vcs.UserInfo{Login: review.User.Login},
+		State:    review.State,
+		Stale:    review.Stale,
+		CommitID: review.CommitID,
+	}, nil
+}
+
+// ListReviews maps []gitea.Review to []vcs.Review.
+func (a *Adapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
+	reviews, err := a.client.ListReviews(ctx, owner, repo, number)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]vcs.Review, len(reviews))
+	for i, r := range reviews {
+		result[i] = vcs.Review{
+			ID:       r.ID,
+			Body:     r.Body,
+			User:     vcs.UserInfo{Login: r.User.Login},
+			State:    r.State,
+			Stale:    r.Stale,
+			CommitID: r.CommitID,
+		}
+	}
+	return result, nil
+}
+
+// DeleteReview is a pass-through to the underlying client.
+func (a *Adapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
+	return a.client.DeleteReview(ctx, owner, repo, number, reviewID)
+}
+
+// DismissReview deletes the review. Gitea supports full deletion of any review state.
+// The message parameter is intentionally unused — Gitea deletion has no dismissal message.
+func (a *Adapter) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
+	return a.client.DeleteReview(ctx, owner, repo, number, reviewID)
+}
+
+// --- Identity ---
+
+// GetAuthenticatedUser is a pass-through to the underlying client.
+func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
+	return a.client.GetAuthenticatedUser(ctx)
+}

gitea/adapter_test.go

@@ -0,0 +1,388 @@
+package gitea_test
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+
+	"gitea.weiker.me/rodin/review-bot/gitea"
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+func TestAdapter_GetPullRequest(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]any{
+			"title": "Test PR",
+			"body":  "PR body",
+			"head": map[string]any{
+				"sha": "abc123",
+				"ref": "feature-branch",
+			},
+			"base": map[string]any{
+				"ref": "main",
+			},
+		})
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	pr, err := adapter.GetPullRequest(context.Background(), "owner", "repo", 42)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if pr.Number != 42 {
+		t.Errorf("Number = %d, want 42", pr.Number)
+	}
+	if pr.Title != "Test PR" {
+		t.Errorf("Title = %q, want %q", pr.Title, "Test PR")
+	}
+	if pr.Body != "PR body" {
+		t.Errorf("Body = %q, want %q", pr.Body, "PR body")
+	}
+	if pr.Head.SHA != "abc123" {
+		t.Errorf("Head.SHA = %q, want %q", pr.Head.SHA, "abc123")
+	}
+	if pr.Head.Ref != "feature-branch" {
+		t.Errorf("Head.Ref = %q, want %q", pr.Head.Ref, "feature-branch")
+	}
+	if pr.Base.Ref != "main" {
+		t.Errorf("Base.Ref = %q, want %q", pr.Base.Ref, "main")
+	}
+}
+
+func TestAdapter_GetPullRequestFiles(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode([]map[string]any{
+			{"filename": "main.go", "status": "modified"},
+			{"filename": "new.go", "status": "added"},
+		})
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	files, err := adapter.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(files) != 2 {
+		t.Fatalf("got %d files, want 2", len(files))
+	}
+	if files[0].Filename != "main.go" || files[0].Status != "modified" {
+		t.Errorf("files[0] = %+v", files[0])
+	}
+	if files[1].Filename != "new.go" || files[1].Status != "added" {
+		t.Errorf("files[1] = %+v", files[1])
+	}
+}
+
+func TestAdapter_ListReviews(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode([]map[string]any{
+			{
+				"id":        1,
+				"body":      "LGTM",
+				"user":      map[string]any{"login": "reviewer1"},
+				"state":     "APPROVED",
+				"stale":     false,
+				"commit_id": "abc123",
+			},
+			{
+				"id":        2,
+				"body":      "Needs work",
+				"user":      map[string]any{"login": "reviewer2"},
+				"state":     "REQUEST_CHANGES",
+				"stale":     true,
+				"commit_id": "def456",
+			},
+		})
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	reviews, err := adapter.ListReviews(context.Background(), "owner", "repo", 1)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(reviews) != 2 {
+		t.Fatalf("got %d reviews, want 2", len(reviews))
+	}
+	if reviews[0].ID != 1 || reviews[0].Body != "LGTM" || reviews[0].User.Login != "reviewer1" {
+		t.Errorf("reviews[0] = %+v", reviews[0])
+	}
+	if reviews[0].State != "APPROVED" || reviews[0].Stale || reviews[0].CommitID != "abc123" {
+		t.Errorf("reviews[0] state/stale/commit = %v/%v/%v", reviews[0].State, reviews[0].Stale, reviews[0].CommitID)
+	}
+	if reviews[1].ID != 2 || !reviews[1].Stale || reviews[1].State != "REQUEST_CHANGES" {
+		t.Errorf("reviews[1] = %+v", reviews[1])
+	}
+}
+
+func TestAdapter_GetCommitStatuses(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode([]map[string]any{
+			{
+				"status":      "success",
+				"context":     "ci/test",
+				"description": "All tests pass",
+				"target_url":  "https://ci.example.com/1",
+			},
+		})
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	statuses, err := adapter.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(statuses) != 1 {
+		t.Fatalf("got %d statuses, want 1", len(statuses))
+	}
+	if statuses[0].Status != "success" {
+		t.Errorf("Status = %q, want %q", statuses[0].Status, "success")
+	}
+	if statuses[0].Context != "ci/test" {
+		t.Errorf("Context = %q, want %q", statuses[0].Context, "ci/test")
+	}
+	if statuses[0].Description != "All tests pass" {
+		t.Errorf("Description = %q, want %q", statuses[0].Description, "All tests pass")
+	}
+	if statuses[0].TargetURL != "https://ci.example.com/1" {
+		t.Errorf("TargetURL = %q, want %q", statuses[0].TargetURL, "https://ci.example.com/1")
+	}
+}
+
+func TestAdapter_PostReview_EventTranslation(t *testing.T) {
+	tests := []struct {
+		name      string
+		event     vcs.ReviewEvent
+		wantEvent string
+	}{
+		{"APPROVE becomes APPROVED", vcs.ReviewEventApprove, "APPROVED"},
+		{"REQUEST_CHANGES stays", vcs.ReviewEventRequestChanges, "REQUEST_CHANGES"},
+		{"COMMENT stays", vcs.ReviewEventComment, "COMMENT"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var gotEvent string
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				var payload struct {
+					Event string `json:"event"`
+				}
+				json.NewDecoder(r.Body).Decode(&payload)
+				gotEvent = payload.Event
+				json.NewEncoder(w).Encode(map[string]any{
+					"id":   1,
+					"body": "test",
+					"user": map[string]any{"login": "bot"},
+				})
+			}))
+			defer server.Close()
+
+			client := gitea.NewClient(server.URL, "token")
+			adapter := gitea.NewAdapter(client)
+
+			_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
+				Body:  "test",
+				Event: tt.event,
+				// No comments → no diff fetch needed
+			})
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if gotEvent != tt.wantEvent {
+				t.Errorf("event = %q, want %q", gotEvent, tt.wantEvent)
+			}
+		})
+	}
+}
+
+func TestAdapter_PostReview_WithComments_PositionTranslation(t *testing.T) {
+	diff := `diff --git a/main.go b/main.go
+--- a/main.go
++++ b/main.go
+@@ -1,3 +1,4 @@
+ package main
+ 
++// new comment at line 3
+ func main() {}
+`
+	var gotComments []struct {
+		Path        string `json:"path"`
+		NewPosition int64  `json:"new_position"`
+		Body        string `json:"body"`
+	}
+
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		if strings.HasSuffix(r.URL.Path, ".diff") {
+			// Diff request
+			w.Write([]byte(diff))
+			return
+		}
+		if strings.HasSuffix(r.URL.Path, "/reviews") {
+			// Review post
+			var payload struct {
+				Comments []struct {
+					Path        string `json:"path"`
+					NewPosition int64  `json:"new_position"`
+					Body        string `json:"body"`
+				} `json:"comments"`
+			}
+			json.NewDecoder(r.Body).Decode(&payload)
+			gotComments = payload.Comments
+			json.NewEncoder(w).Encode(map[string]any{
+				"id":   1,
+				"body": "review",
+				"user": map[string]any{"login": "bot"},
+			})
+			return
+		}
+		t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path)
+		w.WriteHeader(http.StatusNotFound)
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	// Position 4 in this diff is "+// new comment at line 3" → new line 3
+	_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
+		Body:  "review",
+		Event: vcs.ReviewEventRequestChanges,
+		Comments: []vcs.ReviewComment{
+			{
+				Path:     "main.go",
+				Position: 4,
+				CommitID: "abc123",
+				Body:     "needs fix",
+			},
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(gotComments) != 1 {
+		t.Fatalf("got %d comments, want 1", len(gotComments))
+	}
+	if gotComments[0].Path != "main.go" {
+		t.Errorf("path = %q, want %q", gotComments[0].Path, "main.go")
+	}
+	if gotComments[0].NewPosition != 3 {
+		t.Errorf("new_position = %d, want 3", gotComments[0].NewPosition)
+	}
+	if gotComments[0].Body != "needs fix" {
+		t.Errorf("body = %q, want %q", gotComments[0].Body, "needs fix")
+	}
+}
+
+func TestAdapter_DismissReview(t *testing.T) {
+	var deleteCalled bool
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodDelete {
+			deleteCalled = true
+			w.WriteHeader(204)
+			return
+		}
+		w.WriteHeader(404)
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	err := adapter.DismissReview(context.Background(), "owner", "repo", 1, 99, "stale review")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !deleteCalled {
+		t.Error("expected delete to be called")
+	}
+}
+
+func TestAdapter_Underlying(t *testing.T) {
+	client := gitea.NewClient("http://example.com", "token")
+	adapter := gitea.NewAdapter(client)
+	if adapter.Underlying() != client {
+		t.Error("Underlying() should return the wrapped client")
+	}
+}
+
+func TestAdapter_ListContents(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode([]map[string]any{
+			{"name": "main.go", "path": "src/main.go", "type": "file"},
+			{"name": "util", "path": "src/util", "type": "dir"},
+		})
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	entries, err := adapter.ListContents(context.Background(), "owner", "repo", "src")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(entries) != 2 {
+		t.Fatalf("got %d entries, want 2", len(entries))
+	}
+	if entries[0].Name != "main.go" || entries[0].Type != "file" {
+		t.Errorf("entries[0] = %+v", entries[0])
+	}
+	if entries[1].Name != "util" || entries[1].Type != "dir" {
+		t.Errorf("entries[1] = %+v", entries[1])
+	}
+}
+
+func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// When ref is provided, the URL should contain ?ref=
+		if r.URL.RawQuery != "" && strings.Contains(r.URL.RawQuery, "ref=") {
+			w.Write([]byte("content-at-ref"))
+		} else {
+			w.Write([]byte("content-default"))
+		}
+	}))
+	defer server.Close()
+
+	client := gitea.NewClient(server.URL, "token")
+	adapter := gitea.NewAdapter(client)
+
+	// Empty ref → routes to GetFileContent (no ?ref= query param)
+	got, err := adapter.GetFileContent(context.Background(), "owner", "repo", "main.go", "")
+	if err != nil {
+		t.Fatalf("GetFileContent(ref=\"\"): %v", err)
+	}
+	if got != "content-default" {
+		t.Errorf("GetFileContent(ref=\"\") = %q, want %q", got, "content-default")
+	}
+
+	// Non-empty ref → routes to GetFileContentRef (with ?ref= query param)
+	got, err = adapter.GetFileContent(context.Background(), "owner", "repo", "main.go", "abc123")
+	if err != nil {
+		t.Fatalf("GetFileContent(ref=\"abc123\"): %v", err)
+	}
+	if got != "content-at-ref" {
+		t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref")
+	}
+}

gitea/client.go

@@ -86,6 +86,9 @@ type PullRequest struct {
 		Sha string `json:"sha"`
 		Ref string `json:"ref"`
 	} `json:"head"`
+	Base struct {
+		Ref string `json:"ref"`
+	} `json:"base"`
 }
 
 // CommitStatus represents a single CI status entry.
@@ -835,7 +838,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment
 // DismissReview dismisses a review on a pull request.
 // This is a stub for the vcs.Reviewer interface; full implementation is Phase 2.
 func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
-	return fmt.Errorf("DismissReview: not implemented")
+	return fmt.Errorf("dismiss review %d on %s/%s#%d: %w", reviewID, owner, repo, number, errors.ErrUnsupported)
 }
 
 // GetFileContentAtRef fetches a file at a specific ref from a repo.

gitea/conformance_test.go

@@ -0,0 +1,10 @@
+package gitea_test
+
+import (
+	"gitea.weiker.me/rodin/review-bot/gitea"
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+// Compile-time interface conformance assertion.
+// The Adapter (not the raw Client) satisfies the full vcs.Client interface.
+var _ vcs.Client = (*gitea.Adapter)(nil)

gitea/position.go

@@ -0,0 +1,190 @@
+package gitea
+
+import (
+	"fmt"
+	"strconv"
+	"strings"
+)
+
+// PositionMap holds a per-file mapping of GitHub diff-position to new-file line number.
+// Position is a 1-indexed offset from the @@ hunk header line in the unified diff.
+type PositionMap struct {
+	// files maps filename → (position → new-file line number).
+	// Deletion lines are mapped to -1 (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.
+	maxPositions map[string]int
+}
+
+// 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 line, it maps to the nearest non-deletion line below;
+// 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")
+	}
+
+	fileMap, ok := pm.files[file]
+	if !ok {
+		return 0, fmt.Errorf("file %q not found in diff", file)
+	}
+
+	if position < 1 {
+		return 0, fmt.Errorf("position %d out of range (must be >= 1)", position)
+	}
+
+	lineNum, ok := fileMap[position]
+	if !ok {
+		return 0, fmt.Errorf("position %d out of range for file %q", position, file)
+	}
+
+	// lineNum == -1 means this position is a deletion line.
+	// Map to the nearest non-deletion line below.
+	if lineNum == -1 {
+		maxPos := pm.maxPosition(file)
+		for p := position + 1; p <= maxPos; p++ {
+			if ln, exists := fileMap[p]; exists && ln > 0 {
+				return ln, nil
+			}
+		}
+		return 0, fmt.Errorf("position %d targets a deletion line with no subsequent new-file line in %q", position, file)
+	}
+
+	return lineNum, nil
+}
+
+// maxPosition returns the highest position number for a file.
+// O(1) — the maximum is tracked during map construction.
+func (pm *PositionMap) maxPosition(file string) int {
+	return pm.maxPositions[file]
+}
+
+// BuildPositionToLineMap parses a unified diff and builds a PositionMap
+// mapping diff-position → new-file line number per file.
+//
+// Diff-position counting rules (GitHub spec):
+//   - The @@ hunk header line is position 1 for the file's first hunk
+//   - Every subsequent line increments position by 1 — context, additions, AND deletions
+//   - 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)
+func BuildPositionToLineMap(diff string) *PositionMap {
+	pm := &PositionMap{
+		files:        make(map[string]map[int]int),
+		maxPositions: make(map[string]int),
+	}
+
+	lines := strings.Split(diff, "\n")
+	var currentFile string
+	var position int
+	var newLine int
+
+	for _, line := range lines {
+		// Detect new file in diff.
+		// "+++ b/" is checked before "+++ /dev/null" — the two prefixes are
+		// non-overlapping ("+++ /dev/null" does not start with "+++ b/"), so
+		// ordering is independent. Checking the common case first for clarity.
+		if strings.HasPrefix(line, "+++ b/") {
+			currentFile = strings.TrimPrefix(line, "+++ b/")
+			position = 0
+			newLine = 0
+			if pm.files[currentFile] == nil {
+				pm.files[currentFile] = make(map[int]int)
+			}
+			continue
+		}
+
+		// Deleted file: +++ /dev/null means the file is being deleted
+		if strings.HasPrefix(line, "+++ /dev/null") {
+			currentFile = ""
+			continue
+		}
+
+		// Skip --- lines (old file header)
+		if strings.HasPrefix(line, "--- ") {
+			continue
+		}
+
+		// Skip diff --git lines
+		if strings.HasPrefix(line, "diff --git") {
+			continue
+		}
+
+		// Skip index lines
+		if strings.HasPrefix(line, "index ") {
+			continue
+		}
+
+		// Binary file detection
+		if strings.HasPrefix(line, "Binary files") {
+			currentFile = ""
+			continue
+		}
+
+		// Parse hunk headers
+		if strings.HasPrefix(line, "@@") && currentFile != "" {
+			position++
+			pm.maxPositions[currentFile] = position
+			newLine = parseHunkStart(line)
+			continue
+		}
+
+		if currentFile == "" {
+			continue
+		}
+
+		// Skip "\ No newline at end of file" markers
+		if strings.HasPrefix(line, `\`) {
+			continue
+		}
+
+		// Process diff content lines
+		if strings.HasPrefix(line, "+") {
+			// Addition: has a new-file line number
+			position++
+			pm.files[currentFile][position] = newLine
+			pm.maxPositions[currentFile] = position
+			newLine++
+		} else if strings.HasPrefix(line, "-") {
+			// Deletion: has a position but no new-file line number
+			position++
+			pm.files[currentFile][position] = -1
+			pm.maxPositions[currentFile] = position
+		} else if strings.HasPrefix(line, " ") {
+			// Context line
+			position++
+			pm.files[currentFile][position] = newLine
+			pm.maxPositions[currentFile] = position
+			newLine++
+		}
+	}
+
+	return pm
+}
+
+// parseHunkStart extracts the new-file starting line number from a hunk header.
+// Format: @@ -old_start[,old_count] +new_start[,new_count] @@
+func parseHunkStart(hunkLine string) int {
+	plusIdx := strings.Index(hunkLine, "+")
+	if plusIdx < 0 {
+		return 1
+	}
+	rest := hunkLine[plusIdx+1:]
+
+	endIdx := 0
+	for endIdx < len(rest) && rest[endIdx] >= '0' && rest[endIdx] <= '9' {
+		endIdx++
+	}
+
+	if endIdx == 0 {
+		return 1
+	}
+
+	n, err := strconv.Atoi(rest[:endIdx])
+	if err != nil {
+		return 1
+	}
+	return n
+}

gitea/position_test.go

@@ -0,0 +1,274 @@
+package gitea
+
+import (
+	"testing"
+)
+
+func TestBuildPositionToLineMap_SingleHunk(t *testing.T) {
+	// @@ -16,4 +16,5 @@  ← position 1
+	// context           ← position 2, new line 16
+	//-deleted           ← position 3, no new line
+	//+added             ← position 4, new line 17
+	// context           ← position 5, new line 18
+	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)
+
+	tests := []struct {
+		pos      int
+		wantLine int
+	}{
+		{2, 16}, // context line -> new line 16
+		{4, 17}, // added line -> new line 17
+		{5, 18}, // context after -> new line 18
+	}
+	for _, tt := range tests {
+		got, err := pm.Translate("file.go", tt.pos)
+		if err != nil {
+			t.Errorf("Translate(file.go, %d): unexpected error: %v", tt.pos, err)
+			continue
+		}
+		if got != tt.wantLine {
+			t.Errorf("Translate(file.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
+		}
+	}
+}
+
+func TestBuildPositionToLineMap_MultipleHunks(t *testing.T) {
+	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)
+
+	tests := []struct {
+		pos      int
+		wantLine int
+	}{
+		// First hunk: @@ is pos 1
+		{2, 1},  // " line1" -> new line 1
+		{4, 2},  // "+new" -> new line 2
+		// Second hunk: @@ is pos 5 (continues from 4)
+		// Wait: first hunk has pos 1(@@ hdr), 2(" line1"), 3("-old"), 4("+new")
+		// Second hunk @@ is pos 5
+		{6, 10}, // " func foo() {" -> new line 10
+		{7, 11}, // "+\t// added" -> new line 11
+		{8, 12}, // " \treturn" -> new line 12
+		{9, 13}, // " }" -> new line 13
+	}
+	for _, tt := range tests {
+		got, err := pm.Translate("file.go", tt.pos)
+		if err != nil {
+			t.Errorf("Translate(file.go, %d): unexpected error: %v", tt.pos, err)
+			continue
+		}
+		if got != tt.wantLine {
+			t.Errorf("Translate(file.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
+		}
+	}
+}
+
+func TestBuildPositionToLineMap_DeletionTargeted(t *testing.T) {
+	diff := `diff --git a/file.go b/file.go
+--- a/file.go
++++ b/file.go
+@@ -1,4 +1,3 @@ package main
+ line1
+-deleted
+ line3
+`
+	pm := BuildPositionToLineMap(diff)
+
+	// Position 3 is the deletion line "-deleted" — should map to nearest below
+	// Position 4 is " line3" which is new line 2
+	got, err := pm.Translate("file.go", 3)
+	if err != nil {
+		t.Fatalf("Translate(file.go, 3): unexpected error: %v", err)
+	}
+	if got != 2 {
+		t.Errorf("Translate(file.go, 3) = %d, want 2 (nearest non-deletion below)", got)
+	}
+}
+
+func TestBuildPositionToLineMap_DeletionAtEnd(t *testing.T) {
+	// If a deletion line is at the end with no subsequent non-deletion line, error
+	diff := `diff --git a/file.go b/file.go
+--- a/file.go
++++ b/file.go
+@@ -1,3 +1,2 @@ package main
+ line1
+ line2
+-deleted at end
+`
+	pm := BuildPositionToLineMap(diff)
+
+	_, err := pm.Translate("file.go", 4)
+	if err == nil {
+		t.Error("expected error for deletion at end with no subsequent line")
+	}
+}
+
+func TestBuildPositionToLineMap_NewFile(t *testing.T) {
+	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)
+
+	tests := []struct {
+		pos      int
+		wantLine int
+	}{
+		{2, 1}, // "+package main" -> line 1
+		{3, 2}, // "+" (empty line) -> line 2
+		{4, 3}, // "+func init() {}" -> line 3
+	}
+	for _, tt := range tests {
+		got, err := pm.Translate("new.go", tt.pos)
+		if err != nil {
+			t.Errorf("Translate(new.go, %d): unexpected error: %v", tt.pos, err)
+			continue
+		}
+		if got != tt.wantLine {
+			t.Errorf("Translate(new.go, %d) = %d, want %d", tt.pos, got, tt.wantLine)
+		}
+	}
+}
+
+func TestBuildPositionToLineMap_DeletedFile(t *testing.T) {
+	diff := `diff --git a/old.go b/old.go
+deleted file mode 100644
+--- a/old.go
++++ /dev/null
+@@ -1,3 +0,0 @@
+-package main
+-
+-func old() {}
+`
+	pm := BuildPositionToLineMap(diff)
+
+	// Deleted file has no new-file lines; positions should error
+	_, err := pm.Translate("old.go", 2)
+	if err == nil {
+		t.Error("expected error for deleted file position")
+	}
+}
+
+func TestBuildPositionToLineMap_BinaryFile(t *testing.T) {
+	diff := `diff --git a/image.png b/image.png
+Binary files /dev/null and b/image.png differ
+diff --git a/code.go b/code.go
+--- a/code.go
++++ b/code.go
+@@ -1,2 +1,3 @@
+ package main
++// added
+ func main() {}
+`
+	pm := BuildPositionToLineMap(diff)
+
+	// Binary file should not be in the map
+	_, err := pm.Translate("image.png", 1)
+	if err == nil {
+		t.Error("expected error for binary file")
+	}
+
+	// code.go should still work
+	got, err := pm.Translate("code.go", 3)
+	if err != nil {
+		t.Fatalf("Translate(code.go, 3): unexpected error: %v", err)
+	}
+	if got != 2 {
+		t.Errorf("Translate(code.go, 3) = %d, want 2", got)
+	}
+}
+
+func TestBuildPositionToLineMap_OutOfRange(t *testing.T) {
+	diff := `diff --git a/file.go b/file.go
+--- a/file.go
++++ b/file.go
+@@ -1,2 +1,2 @@
+ line1
+-old
++new
+`
+	pm := BuildPositionToLineMap(diff)
+
+	// Position 0 is invalid
+	_, err := pm.Translate("file.go", 0)
+	if err == nil {
+		t.Error("expected error for position 0")
+	}
+
+	// Position 5 is out of range (only positions 1-4 exist)
+	_, err = pm.Translate("file.go", 5)
+	if err == nil {
+		t.Error("expected error for position 5 (out of range)")
+	}
+
+	// Unknown file
+	_, err = pm.Translate("unknown.go", 1)
+	if err == nil {
+		t.Error("expected error for unknown file")
+	}
+}
+
+func TestBuildPositionToLineMap_MultipleFiles(t *testing.T) {
+	diff := `diff --git a/a.go b/a.go
+--- a/a.go
++++ b/a.go
+@@ -1,2 +1,3 @@
+ package a
++// file a
+ func aFunc() {}
+diff --git a/b.go b/b.go
+--- a/b.go
++++ b/b.go
+@@ -1,2 +1,3 @@
+ package b
++// file b
+ func bFunc() {}
+`
+	pm := BuildPositionToLineMap(diff)
+
+	// a.go: pos 3 is "+// file a" -> new line 2
+	got, err := pm.Translate("a.go", 3)
+	if err != nil {
+		t.Fatalf("Translate(a.go, 3): %v", err)
+	}
+	if got != 2 {
+		t.Errorf("Translate(a.go, 3) = %d, want 2", got)
+	}
+
+	// b.go: pos 3 is "+// file b" -> new line 2
+	// Note: position resets per file
+	got, err = pm.Translate("b.go", 3)
+	if err != nil {
+		t.Fatalf("Translate(b.go, 3): %v", err)
+	}
+	if got != 2 {
+		t.Errorf("Translate(b.go, 3) = %d, want 2", got)
+	}
+}

github/client.go

@@ -0,0 +1,344 @@
+// 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.
+package github
+
+import (
+	"context"
+	"errors"
+	"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
+
+	// 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.
+// 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
+}
+
+func (e *APIError) Error() string {
+	body := e.Body
+	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)
+}
+
+// IsNotFound reports whether an error is an API 404 response.
+func IsNotFound(err error) bool {
+	if apiErr, ok := asAPIError(err); ok {
+		return apiErr.StatusCode == http.StatusNotFound
+	}
+	return false
+}
+
+// IsUnauthorized reports whether an error is an API 401 response.
+func IsUnauthorized(err error) bool {
+	if apiErr, ok := asAPIError(err); ok {
+		return apiErr.StatusCode == http.StatusUnauthorized
+	}
+	return false
+}
+
+func asAPIError(err error) (*APIError, bool) {
+	if err == nil {
+		return nil, false
+	}
+	var target *APIError
+	if errors.As(err, &target) {
+		return target, true
+	}
+	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
+
+	// 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
+}
+
+// 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 {
+	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,
+		},
+	}
+}
+
+// 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.
+func (c *Client) SetHTTPClient(hc *http.Client) {
+	if hc == nil {
+		hc = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
+	}
+	c.httpClient = hc
+}
+
+// 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) 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 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 && len(c.retryBackoff) == maxRetryAttempts-1 {
+		backoff = make([]time.Duration, len(c.retryBackoff))
+		copy(backoff, c.retryBackoff)
+	} else {
+		backoff = make([]time.Duration, len(defaultBackoff))
+		copy(backoff, defaultBackoff)
+	}
+
+	// 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)
+		}
+	}
+
+	var lastErr error
+	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
+		if attempt > 0 {
+			var delay time.Duration
+			if attempt-1 < len(backoff) {
+				delay = backoff[attempt-1]
+			}
+			if delay > 0 {
+				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()
+				}
+			}
+		}
+
+		req, err := http.NewRequestWithContext(ctx, method, reqURL, 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)
+		if accept != "" {
+			req.Header.Set("Accept", accept)
+		} else {
+			req.Header.Set("Accept", "application/vnd.github+json")
+		}
+
+		resp, err := c.httpClient.Do(req)
+		if err != nil {
+			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
+		}
+		lastErr = err
+
+		// Retry on 429 rate limit
+		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 := retryAfterHeader; 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
+					}
+				}
+			}
+			continue
+		}
+
+		// Don't retry other errors
+		return nil, lastErr
+	}
+
+	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 (c *Client) 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", 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, "")
+}

github/client_test.go

@@ -0,0 +1,594 @@
+package github
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestNewClient_DefaultBaseURL(t *testing.T) {
+	c := NewClient("test-token", "")
+	if c.baseURL != "https://api.github.com" {
+		t.Errorf("expected default base URL, got %q", c.baseURL)
+	}
+}
+
+func TestNewClient_CustomBaseURL(t *testing.T) {
+	c := NewClient("test-token", "https://github.concur.com/api/v3")
+	if c.baseURL != "https://github.concur.com/api/v3" {
+		t.Errorf("expected custom base URL, got %q", c.baseURL)
+	}
+}
+
+func TestNewClient_TrimsTrailingSlash(t *testing.T) {
+	c := NewClient("test-token", "https://github.concur.com/api/v3/")
+	if c.baseURL != "https://github.concur.com/api/v3" {
+		t.Errorf("expected trailing slash trimmed, got %q", c.baseURL)
+	}
+}
+
+func TestDoRequest_SetsAuthHeader(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("my-token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+	_, _ = c.doGet(context.Background(), srv.URL+"/test")
+
+	if gotAuth != "Bearer my-token" {
+		t.Errorf("expected Bearer auth, got %q", gotAuth)
+	}
+}
+
+func TestDoRequest_SetsDefaultAcceptHeader(t *testing.T) {
+	var gotAccept string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		gotAccept = r.Header.Get("Accept")
+		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 gotAccept != "application/vnd.github+json" {
+		t.Errorf("expected default Accept header, got %q", gotAccept)
+	}
+}
+
+func TestDoRequest_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
+		}
+		w.WriteHeader(200)
+		w.Write([]byte(`{"ok":true}`))
+	}))
+	defer srv.Close()
+
+	c := NewClient("token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+	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 {
+		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)
+	}
+}
+
+func TestDoRequest_429ExhaustsRetries(t *testing.T) {
+	attempts := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(429)
+		w.Write([]byte(`{"message":"rate limit"}`))
+	}))
+	defer srv.Close()
+
+	c := NewClient("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)
+	}
+
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error after exhausting retries")
+	}
+	apiErr, ok := err.(*APIError)
+	if !ok {
+		t.Fatalf("expected *APIError, got %T", err)
+	}
+	if apiErr.StatusCode != 429 {
+		t.Errorf("expected 429, got %d", apiErr.StatusCode)
+	}
+	if attempts != 3 {
+		t.Errorf("expected 3 attempts, got %d", attempts)
+	}
+}
+
+func TestDoRequest_404NoRetry(t *testing.T) {
+	attempts := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(404)
+		w.Write([]byte(`{"message":"not found"}`))
+	}))
+	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 404")
+	}
+	if attempts != 1 {
+		t.Errorf("expected 1 attempt (no retry on 404), got %d", attempts)
+	}
+}
+
+func TestDoRequest_401NoRetry(t *testing.T) {
+	attempts := 0
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(401)
+		w.Write([]byte(`{"message":"bad credentials"}`))
+	}))
+	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 401")
+	}
+	if attempts != 1 {
+		t.Errorf("expected 1 attempt (no retry on 401), got %d", attempts)
+	}
+}
+
+func TestIsNotFound(t *testing.T) {
+	err := &APIError{StatusCode: 404, Body: "not found"}
+	if !IsNotFound(err) {
+		t.Error("expected IsNotFound to return true for 404")
+	}
+	err2 := &APIError{StatusCode: 500, Body: "server error"}
+	if IsNotFound(err2) {
+		t.Error("expected IsNotFound to return false for 500")
+	}
+}
+
+func TestIsUnauthorized(t *testing.T) {
+	err := &APIError{StatusCode: 401, Body: "bad credentials"}
+	if !IsUnauthorized(err) {
+		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
+	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")
+	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())
+	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 {
+		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())
+	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")
+	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())
+	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")
+	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 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)
+	}
+}

vcs/check_test.go

@@ -1,5 +1,3 @@
-//go:build phase2
-
 package vcs_test
 
 import (
@@ -7,21 +5,7 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 
-// Compile-time assertion: documents the gap between gitea.Client and vcs.Client.
-// Guarded by the "phase2" build tag — enable once the Gitea adapter bridges these gaps:
-//
-//  1. PostReview signature mismatch:
-//     gitea.Client:  PostReview(ctx, owner, repo, number, event, body string, comments []gitea.ReviewComment)
-//     vcs.Reviewer:  PostReview(ctx, owner, repo, number, req vcs.ReviewRequest)
-//
-//  2. GetFileContent signature mismatch:
-//     gitea.Client:  GetFileContent(ctx, owner, repo, filepath string)  [no ref; uses default branch]
-//     vcs.FileReader: GetFileContent(ctx, owner, repo, path, ref string)
-//     (gitea.Client has GetFileContentRef for the ref variant)
-//
-//  3. ReviewComment type mismatch:
-//     gitea.ReviewComment uses NewPosition int64 (Gitea line-number convention)
-//     vcs.ReviewComment uses Position int (GitHub diff-position convention)
-//
-// The Gitea adapter (Phase 2) will wrap gitea.Client to bridge these gaps.
-var _ vcs.Client = (*gitea.Client)(nil)
+// Compile-time assertion: the gitea.Adapter satisfies vcs.Client.
+// (The raw gitea.Client does NOT satisfy vcs.Client due to signature differences;
+// the Adapter bridges them.)
+var _ vcs.Client = (*gitea.Adapter)(nil)

vcs/types.go

@@ -44,6 +44,7 @@ type PullRequest struct {
 type ChangedFile struct {
 	Filename string `json:"filename"`
 	Status   string `json:"status"`
+	Patch    string `json:"patch"`
 }
 
 // ContentEntry represents a file or directory entry from the contents API.

vcs/util.go

@@ -3,39 +3,82 @@ package vcs
 import (
 	"context"
 	"fmt"
+	"strconv"
 	"strings"
 )
 
+const (
+	// maxFilesInPath is the maximum number of files GetAllFilesInPath will fetch.
+	// Prevents unbounded resource consumption on very large directory trees.
+	maxFilesInPath = 10000
+
+	// maxTotalBytesInPath is the maximum total bytes GetAllFilesInPath will accumulate.
+	// Prevents memory exhaustion when fetching large repositories.
+	maxTotalBytesInPath = 100 * 1024 * 1024 // 100 MB
+)
+
 // GetAllFilesInPath recursively fetches all file contents under a path using the
 // provided FileReader. Returns a map of filepath -> content for all files found.
 // If the path points to an empty directory, returns an empty map.
+//
+// This function uses fail-fast error handling: any error from ListContents or
+// GetFileContent aborts the entire traversal and returns the error immediately.
+// This differs from gitea.Client.GetAllFilesInPath, which logs errors and continues.
+// The fail-fast contract ensures callers can trust that a nil error means all files
+// were successfully fetched.
+//
+// Resource limits: the traversal is bounded by maxFilesInPath (file count) and
+// maxTotalBytesInPath (total accumulated bytes). The context is checked before each
+// recursive call and file fetch to respect cancellation.
 func GetAllFilesInPath(ctx context.Context, client FileReader, owner, repo, path string) (map[string]string, error) {
 	results := make(map[string]string)
+	totalBytes := 0
 
-	entries, err := client.ListContents(ctx, owner, repo, path)
-	if err != nil {
-		return nil, fmt.Errorf("list contents %q: %w", path, err)
-	}
+	var walk func(string) error
+	walk = func(dir string) error {
+		if err := ctx.Err(); err != nil {
+			return fmt.Errorf("context canceled during traversal: %w", err)
+		}
 
-	for _, entry := range entries {
-		switch entry.Type {
-		case "file":
-			content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "")
-			if err != nil {
-				return nil, fmt.Errorf("get file %q: %w", entry.Path, err)
+		entries, err := client.ListContents(ctx, owner, repo, dir)
+		if err != nil {
+			return fmt.Errorf("list contents %q: %w", dir, err)
+		}
+
+		for _, entry := range entries {
+			if err := ctx.Err(); err != nil {
+				return fmt.Errorf("context canceled during traversal: %w", err)
 			}
-			results[entry.Path] = content
-		case "dir":
-			subResults, err := GetAllFilesInPath(ctx, client, owner, repo, entry.Path)
-			if err != nil {
-				return nil, fmt.Errorf("recurse into %q: %w", entry.Path, err)
-			}
-			for k, v := range subResults {
-				results[k] = v
+
+			switch entry.Type {
+			case "file":
+				if len(results) >= maxFilesInPath {
+					return fmt.Errorf("exceeded max file count (%d) in path %q", maxFilesInPath, path)
+				}
+
+				content, err := client.GetFileContent(ctx, owner, repo, entry.Path, "")
+				if err != nil {
+					return fmt.Errorf("get file %q: %w", entry.Path, err)
+				}
+
+				totalBytes += len(content)
+				if totalBytes > maxTotalBytesInPath {
+					return fmt.Errorf("exceeded max total bytes (%d) in path %q", maxTotalBytesInPath, path)
+				}
+
+				results[entry.Path] = content
+			case "dir":
+				if err := walk(entry.Path); err != nil {
+					return err
+				}
 			}
 		}
+		return nil
 	}
 
+	if err := walk(path); err != nil {
+		return nil, err
+	}
 	return results, nil
 }
 
@@ -92,6 +135,12 @@ func BuildLineToPositionMap(diff string) map[string]map[int]int {
 			continue
 		}
 
+		// Skip "\ No newline at end of file" markers — these are git diff
+		// metadata and not part of the file content.
+		if strings.HasPrefix(line, `\`) {
+			continue
+		}
+
 		// Process diff content lines
 		if strings.HasPrefix(line, "+") {
 			position++
@@ -101,7 +150,10 @@ func BuildLineToPositionMap(diff string) map[string]map[int]int {
 			position++
 			// Deletion lines don't map to new line numbers
 		} else if strings.HasPrefix(line, " ") {
-			// Context line (space-prefixed)
+			// Context line (space-prefixed).
+			// Only map if position > 0, which means we've seen a hunk header.
+			// Lines before the first hunk header (position == 0) are not part
+			// of any diff hunk and should be skipped.
 			if position > 0 {
 				position++
 				result[currentFile][newLine] = position
@@ -123,23 +175,19 @@ func parseHunkNewStart(hunkLine string) int {
 	}
 	rest := hunkLine[plusIdx+1:]
 
-	// Read digits until comma or space
-	var numStr string
-	for _, ch := range rest {
-		if ch >= '0' && ch <= '9' {
-			numStr += string(ch)
-		} else {
-			break
-		}
+	// Find the end of the number (first non-digit after +)
+	endIdx := 0
+	for endIdx < len(rest) && rest[endIdx] >= '0' && rest[endIdx] <= '9' {
+		endIdx++
 	}
 
-	if numStr == "" {
+	if endIdx == 0 {
 		return 1
 	}
 
-	n := 0
-	for _, ch := range numStr {
-		n = n*10 + int(ch-'0')
+	n, err := strconv.Atoi(rest[:endIdx])
+	if err != nil {
+		return 1
 	}
 	return n
 }

vcs/util_test.go

@@ -3,6 +3,7 @@ package vcs_test
 import (
 	"context"
 	"fmt"
+	"strings"
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/vcs"
@@ -248,3 +249,83 @@ func TestBuildLineToPositionMap(t *testing.T) {
 		}
 	})
 }
+
+func TestGetAllFilesInPath_ErrorPropagation(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("ListContents error propagates", func(t *testing.T) {
+		client := &mockFileReader{
+			contents: map[string][]vcs.ContentEntry{
+				// "src" not in map, so ListContents will fail
+			},
+		}
+		_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		if !strings.Contains(err.Error(), "list contents") {
+			t.Errorf("expected error about list contents, got: %v", err)
+		}
+	})
+
+	t.Run("GetFileContent error propagates", func(t *testing.T) {
+		client := &mockFileReader{
+			contents: map[string][]vcs.ContentEntry{
+				"src": {
+					{Name: "main.go", Path: "src/main.go", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				// "src/main.go" not in files map, so GetFileContent will fail
+			},
+		}
+		_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		if !strings.Contains(err.Error(), "get file") {
+			t.Errorf("expected error about get file, got: %v", err)
+		}
+	})
+
+	t.Run("nested ListContents error propagates", func(t *testing.T) {
+		client := &mockFileReader{
+			contents: map[string][]vcs.ContentEntry{
+				"src": {
+					{Name: "pkg", Path: "src/pkg", Type: "dir"},
+				},
+				// "src/pkg" not in map, so recursive ListContents will fail
+			},
+		}
+		_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
+		if err == nil {
+			t.Fatal("expected error, got nil")
+		}
+		if !strings.Contains(err.Error(), "list contents") {
+			t.Errorf("expected error about list contents, got: %v", err)
+		}
+	})
+
+	t.Run("canceled context propagates", func(t *testing.T) {
+		ctx, cancel := context.WithCancel(context.Background())
+		cancel() // Cancel immediately
+
+		client := &mockFileReader{
+			contents: map[string][]vcs.ContentEntry{
+				"src": {
+					{Name: "main.go", Path: "src/main.go", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				"src/main.go": "package main",
+			},
+		}
+		_, err := vcs.GetAllFilesInPath(ctx, client, "owner", "repo", "src")
+		if err == nil {
+			t.Fatal("expected error from canceled context, got nil")
+		}
+		if !strings.Contains(err.Error(), "context canceled") {
+			t.Errorf("expected context cancellation error, got: %v", err)
+		}
+	})
+}