fix: address review findings from reviews #2955 and #2958

- Convert handleResponse to package-level function (unused receiver) [#17955] - Add clarifying comment for nil resp on transport error [#17956] - Use consistent %w wrapping in dual-unmarshal error path [#17957] - Add SafeError() method to APIError for safe logging [#17964] - Enforce safe CheckRedirect policy in SetHTTPClient [#17965] - Add tests for SafeError and SetHTTPClient enforcement
refactor(github): extract handleResponse for safe defer body close
2026-05-12 21:19:01 -07:00 · 2026-05-12 20:47:59 -07:00 · 2026-05-12 20:28:52 -07:00 · 2026-05-12 19:40:30 -07:00 · 2026-05-12 19:29:06 -07:00 · 2026-05-12 18:41:44 -07:00
14 changed files with 1437 additions and 1160 deletions
@@ -1,232 +0,0 @@
 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)
 }
@@ -1,388 +0,0 @@
 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")
 	}
 }
@@ -86,9 +86,6 @@ 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.
@@ -1,3 +1,5 @@
 //go:build phase2
 package gitea_test
 import (
@@ -5,6 +7,19 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
-// Compile-time interface conformance assertion.
+// Compile-time interface conformance assertions.
-// The Adapter (not the raw Client) satisfies the full vcs.Client interface.
+// These will verify gitea.Client satisfies vcs interfaces once the Phase 2
-var _ vcs.Client = (*gitea.Adapter)(nil)
+// adapter bridges the method signature gaps:
 //
 //   - PRReader: GetPullRequest returns *gitea.PullRequest (needs *vcs.PullRequest)
 //   - PRReader: GetPullRequestFiles returns []gitea.ChangedFile (needs []vcs.ChangedFile)
 //   - FileReader: GetFileContent lacks ref parameter
 //   - Reviewer: PostReview uses (event, body, comments) instead of vcs.ReviewRequest
 //
 // Remove the phase2 build tag once the adapter is complete.
 var (
 	_ vcs.PRReader   = (*gitea.Client)(nil)
 	_ vcs.FileReader = (*gitea.Client)(nil)
 	_ vcs.Reviewer   = (*gitea.Client)(nil)
 	_ vcs.Identity   = (*gitea.Client)(nil)
 )
@@ -1,190 +0,0 @@
 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
 }
@@ -1,274 +0,0 @@
 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)
 	}
 }
@@ -21,10 +21,6 @@ const (
 	// maxResponseBytes limits successful response body reads to 10 MiB.
 	maxResponseBytes = 10 * 1024 * 1024
 	// maxRetryAttempts is the number of times doRequest will attempt a request.
 	// The retry backoff slice must have length maxRetryAttempts-1.
 	maxRetryAttempts = 3
 )
 // APIError represents an HTTP error response from the GitHub API.
@@ -51,6 +47,13 @@ func (e *APIError) Error() string {
 	return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
 }
 // SafeError returns the error string without response body content,
 // suitable for logging in contexts where upstream response data should
 // not be exposed.
 func (e *APIError) SafeError() string {
 	return fmt.Sprintf("HTTP %d", e.StatusCode)
 }
 // IsNotFound reports whether an error is an API 404 response.
 func IsNotFound(err error) bool {
 	if apiErr, ok := asAPIError(err); ok {
@@ -176,39 +179,36 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
 			Timeout:       30 * time.Second,
 			CheckRedirect: defaultCheckRedirect,
 		}
 	} else if hc.CheckRedirect == nil {
 		// Enforce safe redirect policy when caller provides a client without one.
 		// The default net/http behavior follows up to 10 redirects and forwards
 		// all headers (including Authorization) to any host, which can leak
 		// credentials on cross-host redirects.
 		hc.CheckRedirect = defaultCheckRedirect
 	}
 	c.httpClient = hc
 }
 // SetRetryBackoff configures the retry backoff durations for testing.
 // It must be called before any goroutines issue requests.
 // 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 {
+func (c *Client) SetRetryBackoff(d []time.Duration) {
 	if len(d) != maxRetryAttempts-1 {
 		return fmt.Errorf("github: backoff length %d does not match maxRetryAttempts-1 (%d)", len(d), maxRetryAttempts-1)
 	}
 	c.retryBackoff = d
 	return nil
 }
 // doRequest performs an HTTP request with retry on 429 rate limit responses.
 // It respects the Retry-After header when present (capped at maxRetryAfter).
 // Transport errors (network failures, context cancellation) are not retried.
 func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
 	const maxAttempts = 3
 	const maxRetryAfter = 120 * time.Second
 	// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
 	// Length must be maxRetryAttempts-1 (one entry per retry gap).
 	// SetRetryBackoff validates at configuration time; the default is always valid.
 	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
 	var backoff []time.Duration
-	if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
+	if c.retryBackoff != nil {
 		backoff = make([]time.Duration, len(c.retryBackoff))
 		copy(backoff, c.retryBackoff)
 	} else {
-		backoff = make([]time.Duration, len(defaultBackoff))
+		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
 		copy(backoff, defaultBackoff)
 	}
 	// maxErrorBodyBytes limits how much of an error response body is stored.
@@ -228,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	}
 	var lastErr error
-	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
+	for attempt := 0; attempt < maxAttempts; attempt++ {
 		if attempt > 0 {
 			var delay time.Duration
 			if attempt-1 < len(backoff) {
@@ -265,24 +265,21 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		resp, err := c.httpClient.Do(req)
 		if err != nil {
 			// Transport errors (DNS, TLS, timeout) yield nil resp; no body to close.
 			return nil, fmt.Errorf("do request: %w", err)
 		}
-		// Capture response metadata before handleResponse takes body ownership.
+		body, done, err := handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		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 {
+		if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
 			// Check for Retry-After header and override backoff if present.
 			// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
-			if ra := retryAfterHeader; ra != "" {
+			if ra := resp.Header.Get("Retry-After"); ra != "" {
 				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
 					delay := time.Duration(seconds) * time.Second
 					if delay > maxRetryAfter {
@@ -317,7 +314,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 // 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) {
+func handleResponse(resp *http.Response, maxRespBytes int, maxErrBytes int) ([]byte, bool, error) {
 	defer resp.Body.Close()
 	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
@@ -326,7 +323,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
 			return nil, true, fmt.Errorf("read response body: %w", err)
 		}
 		if len(body) > maxRespBytes {
-			return nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes)
+			return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes)
 		}
 		return body, true, nil
 	}
@@ -83,9 +83,7 @@ func TestDoRequest_429Retry(t *testing.T) {
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil {
+	c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
@@ -110,9 +108,7 @@ func TestDoRequest_429ExhaustsRetries(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, 1 * time.Millisecond})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
@@ -222,9 +218,7 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
 	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 {
+	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	start := time.Now()
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -265,9 +259,7 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(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, 1 * time.Millisecond})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
@@ -305,9 +297,7 @@ func TestDoRequest_429RetryAfterHTTPDate(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, 1 * time.Millisecond})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	start := time.Now()
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -348,9 +338,7 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil {
+	c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
 		t.Fatalf("SetRetryBackoff: %v", err)
 	}
 	start := time.Now()
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -567,28 +555,45 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
 	}
 }
-
+func TestSetHTTPClient_NilCheckRedirectEnforcesDefault(t *testing.T) {
 func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
 	c := NewClient("token", "https://api.github.com")
-
+	// Provide a client with nil CheckRedirect — should get default policy enforced.
-	// Too short
+	hc := &http.Client{Timeout: 5 * time.Second}
-	err := c.SetRetryBackoff([]time.Duration{1 * time.Second})
+	c.SetHTTPClient(hc)
-	if err == nil {
+	if c.httpClient.CheckRedirect == nil {
-		t.Fatal("expected error for backoff length 1")
+		t.Fatal("expected CheckRedirect to be enforced when caller provides nil")
 	}
-	if !strings.Contains(err.Error(), "backoff length 1") {
+	if c.httpClient.Timeout != 5*time.Second {
-		t.Errorf("unexpected error message: %v", err)
+		t.Errorf("expected caller's timeout preserved, got %v", c.httpClient.Timeout)
-	}
+	}
-
+}
-	// Too long
+
-	err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second, 3 * time.Second})
+func TestSetHTTPClient_PreservesCustomCheckRedirect(t *testing.T) {
-	if err == nil {
+	c := NewClient("token", "https://api.github.com")
-		t.Fatal("expected error for backoff length 3")
+	called := false
-	}
+	hc := &http.Client{
-
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-	// Correct length succeeds
+			called = true
-	err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second})
+			return nil
-	if err != nil {
+		},
-		t.Fatalf("unexpected error for valid backoff: %v", err)
+	}
 	c.SetHTTPClient(hc)
 	// Invoke the redirect to verify original is preserved
 	_ = c.httpClient.CheckRedirect(nil, []*http.Request{{}})
 	if !called {
 		t.Fatal("expected custom CheckRedirect to be preserved")
 	}
 }
 func TestAPIError_SafeError(t *testing.T) {
 	e := &APIError{StatusCode: 403, Body: "some sensitive body content"}
 	got := e.SafeError()
 	if got != "HTTP 403" {
 		t.Errorf("SafeError() = %q, want %q", got, "HTTP 403")
 	}
 	// Ensure Error() still includes body
 	full := e.Error()
 	if full != "HTTP 403: some sensitive body content" {
 		t.Errorf("Error() = %q, unexpected", full)
 	}
 }
@@ -0,0 +1,13 @@
 package github_test
 import (
 	"gitea.weiker.me/rodin/review-bot/github"
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 // Compile-time interface conformance assertions.
 // These verify github.Client satisfies vcs.PRReader and vcs.FileReader.
 var (
 	_ vcs.PRReader   = (*github.Client)(nil)
 	_ vcs.FileReader = (*github.Client)(nil)
 )
@@ -0,0 +1,135 @@
 package github
 import (
 	"context"
 	"encoding/base64"
 	"encoding/json"
 	"fmt"
 	"net/url"
 	"strings"
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 // GetFileContent fetches a file from a repo at the given ref.
 // Delegates to GetFileContentAtRef with the provided ref.
 func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
 	return c.GetFileContentAtRef(ctx, owner, repo, path, ref)
 }
 // GetFileContentAtRef fetches a file at a specific ref from a repo.
 // If ref is empty, the query parameter is omitted (uses default branch).
 //
 // Note: dot-segments ("." and "..") in the path are silently removed to
 // prevent path traversal. This means a path like "foo/../bar" resolves
 // to "foo/bar" rather than "bar".
 func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
 	if ref != "" {
 		reqURL += "?ref=" + url.QueryEscape(ref)
 	}
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch file %s: %w", path, err)
 	}
 	var resp struct {
 		Content  string `json:"content"`
 		Encoding string `json:"encoding"`
 	}
 	if err := json.Unmarshal(body, &resp); err != nil {
 		return "", fmt.Errorf("parse file content JSON: %w", err)
 	}
 	if resp.Encoding != "base64" {
 		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
 	}
 	decoded, err := decodeBase64Content(resp.Content)
 	if err != nil {
 		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
 	}
 	return decoded, nil
 }
 // ListContents lists files and directories at a given path in a repo.
 // Returns the directory listing from the GitHub contents API.
 // If the path points to a single file (not a directory), the API returns
 // a JSON object instead of an array; this is handled by returning a
 // single-element slice.
 //
 // Note: dot-segments ("." and "..") in the path are silently removed to
 // prevent path traversal. This means a path like "foo/../bar" resolves
 // to "foo/bar" rather than "bar".
 func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("list contents %s: %w", path, err)
 	}
 	type entry struct {
 		Name string `json:"name"`
 		Path string `json:"path"`
 		Type string `json:"type"`
 	}
 	// The GitHub contents API returns an array for directories and an object
 	// for single files. Try array first (common case), then fall back to object.
 	// An empty array ([]) is valid — it represents an empty directory — and
 	// results in a zero-length slice returned without error.
 	var entries []entry
 	if err := json.Unmarshal(body, &entries); err != nil {
 		var single entry
 		if err2 := json.Unmarshal(body, &single); err2 != nil {
 			return nil, fmt.Errorf("parse contents JSON: as array: %w; as object: %w", err, err2)
 		}
 		// Guard against empty objects ({}) or unexpected shapes that
 		// unmarshal successfully but carry no useful data.
 		if single.Name == "" && single.Path == "" && single.Type == "" {
 			return nil, fmt.Errorf("parse contents JSON: unexpected response format")
 		}
 		entries = []entry{single}
 	}
 	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
 }
 // escapePath escapes each segment of a relative file path for use in URLs.
 // Slashes are preserved as path separators; other special characters are escaped.
 // Dot-segments ("." and "..") and empty segments (from consecutive slashes like
 // "a//b") are silently removed to prevent path traversal and produce canonical
 // paths. This is intentional: callers may receive a different path than requested
 // without error. The function is package-private, and all callers
 // (GetFileContentAtRef, ListContents) already handle missing-file errors from the
 // API if the cleaned path doesn't match what the caller intended.
 func escapePath(p string) string {
 	parts := strings.Split(p, "/")
 	var clean []string
 	for _, part := range parts {
 		if part == "." || part == ".." || part == "" {
 			continue
 		}
 		clean = append(clean, url.PathEscape(part))
 	}
 	return strings.Join(clean, "/")
 }
 // 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.
 func decodeBase64Content(encoded string) (string, error) {
 	// GitHub inserts newlines in base64 content
 	cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
 	decoded, err := base64.StdEncoding.DecodeString(cleaned)
 	if err != nil {
 		return "", err
 	}
 	return string(decoded), nil
 }
@@ -0,0 +1,334 @@
 package github
 import (
 	"context"
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 	"time"
 )
 func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
 	var gotRef string
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		gotRef = r.URL.Query().Get("ref")
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "dGVzdA==", // "test" in base64
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	// Call with empty ref — should not include ref param
 	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if content != "test" {
 		t.Errorf("expected 'test', got %q", content)
 	}
 	if gotRef != "" {
 		t.Errorf("expected empty ref, got %q", gotRef)
 	}
 }
 func TestGetFileContent_WithRef(t *testing.T) {
 	var gotRef string
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		gotRef = r.URL.Query().Get("ref")
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "dGVzdA==",
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if gotRef != "abc123" {
 		t.Errorf("expected ref 'abc123', got %q", gotRef)
 	}
 }
 func TestGetFileContent_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContent(context.Background(), "owner", "repo", "missing.go", "")
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestGetFileContent_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 }
 func TestGetFileContent_429Retry(t *testing.T) {
 	attempts := 0
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
 			w.WriteHeader(429)
 			w.Write([]byte(`{"message":"rate limit"}`))
 			return
 		}
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "b2s=",
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
 	content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if content != "ok" {
 		t.Errorf("expected 'ok', got %q", content)
 	}
 	if attempts != 2 {
 		t.Errorf("expected 2 attempts, got %d", attempts)
 	}
 }
 func TestGetFileContent_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`not json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 }
 func TestListContents_HappyPath(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path != "/repos/owner/repo/contents/src" {
 			t.Errorf("unexpected path: %s", r.URL.Path)
 		}
 		json.NewEncoder(w).Encode([]map[string]string{
 			{"name": "main.go", "path": "src/main.go", "type": "file"},
 			{"name": "lib", "path": "src/lib", "type": "dir"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	entries, err := c.ListContents(context.Background(), "owner", "repo", "src")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(entries) != 2 {
 		t.Fatalf("expected 2 entries, got %d", len(entries))
 	}
 	if entries[0].Name != "main.go" {
 		t.Errorf("expected name 'main.go', got %q", entries[0].Name)
 	}
 	if entries[0].Path != "src/main.go" {
 		t.Errorf("expected path 'src/main.go', got %q", entries[0].Path)
 	}
 	if entries[0].Type != "file" {
 		t.Errorf("expected type 'file', got %q", entries[0].Type)
 	}
 	if entries[1].Name != "lib" {
 		t.Errorf("expected name 'lib', got %q", entries[1].Name)
 	}
 	if entries[1].Type != "dir" {
 		t.Errorf("expected type 'dir', got %q", entries[1].Type)
 	}
 }
 func TestListContents_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.ListContents(context.Background(), "owner", "repo", "missing")
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestListContents_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 }
 func TestListContents_429Retry(t *testing.T) {
 	attempts := 0
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
 			w.WriteHeader(429)
 			w.Write([]byte(`{"message":"rate limit"}`))
 			return
 		}
 		json.NewEncoder(w).Encode([]map[string]string{
 			{"name": "file.go", "path": "file.go", "type": "file"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
 	entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(entries) != 1 {
 		t.Fatalf("expected 1 entry, got %d", len(entries))
 	}
 	if attempts != 2 {
 		t.Errorf("expected 2 attempts, got %d", attempts)
 	}
 }
 func TestListContents_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`not json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.ListContents(context.Background(), "owner", "repo", "src")
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 }
 func TestDecodeBase64Content(t *testing.T) {
 	// Test with newlines (GitHub's format)
 	encoded := "cGFja2FnZSBt\nYWlu"
 	decoded, err := decodeBase64Content(encoded)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if decoded != "package main" {
 		t.Errorf("expected 'package main', got %q", decoded)
 	}
 }
 func TestDecodeBase64Content_Invalid(t *testing.T) {
 	_, err := decodeBase64Content("not!!!valid!!!base64")
 	if err == nil {
 		t.Fatal("expected error for invalid base64")
 	}
 }
 func TestEscapePath_RejectsDotSegments(t *testing.T) {
 	tests := []struct {
 		input string
 		want  string
 	}{
 		{"src/main.go", "src/main.go"},
 		{"../etc/passwd", "etc/passwd"},
 		{"./src/../main.go", "src/main.go"},
 		{"a/b/c", "a/b/c"},
 		{"file with spaces.go", "file%20with%20spaces.go"},
 		{"a/./b/../c", "a/b/c"},
 	}
 	for _, tt := range tests {
 		got := escapePath(tt.input)
 		if got != tt.want {
 			t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want)
 		}
 	}
 }
 func TestDecodeBase64Content_CRLF(t *testing.T) {
 	// Base64 of "hello world" with CRLF line breaks inserted
 	encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ="
 	decoded, err := decodeBase64Content(encoded)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if decoded != "hello world" {
 		t.Errorf("expected 'hello world', got %q", decoded)
 	}
 }
 func TestListContents_SingleFile(t *testing.T) {
 	// GitHub Contents API returns a JSON object (not array) for single-file paths
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(entries) != 1 {
 		t.Fatalf("expected 1 entry, got %d", len(entries))
 	}
 	if entries[0].Name != "README.md" {
 		t.Errorf("expected name 'README.md', got %q", entries[0].Name)
 	}
 	if entries[0].Type != "file" {
 		t.Errorf("expected type 'file', got %q", entries[0].Type)
 	}
 }
@@ -0,0 +1,212 @@
 package github
 import (
 	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/url"
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 // pullRequestResponse is the GitHub API response for a pull request.
 type pullRequestResponse struct {
 	Number int    `json:"number"`
 	Title  string `json:"title"`
 	Body   string `json:"body"`
 	Head   struct {
 		SHA string `json:"sha"`
 		Ref string `json:"ref"`
 	} `json:"head"`
 	Base struct {
 		Ref string `json:"ref"`
 	} `json:"base"`
 }
 // changedFileResponse is the GitHub API response for a changed file in a PR.
 type changedFileResponse struct {
 	Filename string `json:"filename"`
 	Status   string `json:"status"`
 	Patch    string `json:"patch"`
 }
 // commitStatusResponse is the GitHub combined status API response.
 type commitStatusResponse struct {
 	Statuses []struct {
 		Context     string `json:"context"`
 		State       string `json:"state"`
 		Description string `json:"description"`
 		TargetURL   string `json:"target_url"`
 	} `json:"statuses"`
 }
 // checkRunsResponse is the GitHub check runs API response.
 type checkRunsResponse struct {
 	CheckRuns []struct {
 		Name       string  `json:"name"`
 		Conclusion *string `json:"conclusion"`
 		Status     string  `json:"status"`
 		HTMLURL    string  `json:"html_url"`
 	} `json:"check_runs"`
 }
 // GetPullRequest fetches PR metadata.
 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)
 	if err != nil {
 		return nil, fmt.Errorf("fetch PR: %w", err)
 	}
 	var resp pullRequestResponse
 	if err := json.Unmarshal(body, &resp); err != nil {
 		return nil, fmt.Errorf("parse PR JSON: %w", err)
 	}
 	return &vcs.PullRequest{
 		Number: resp.Number,
 		Title:  resp.Title,
 		Body:   resp.Body,
 		Head:   vcs.HeadRef{SHA: resp.Head.SHA, Ref: resp.Head.Ref},
 		Base:   vcs.BaseRef{Ref: resp.Base.Ref},
 	}, nil
 }
 // GetPullRequestDiff fetches the unified diff for a PR.
 // Uses Accept: application/vnd.github.diff to get raw diff text.
 func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doRequest(ctx, http.MethodGet, reqURL, "application/vnd.github.diff")
 	if err != nil {
 		return "", fmt.Errorf("fetch diff: %w", err)
 	}
 	return string(body), nil
 }
 // maxPages is the upper bound on pagination loops to prevent unbounded iteration
 // in case the server returns a full page indefinitely.
 const maxPages = 100
 // GetPullRequestFiles fetches the list of files changed in a PR.
 // Paginates through all pages (100 per page) to collect all files.
 // Returns nil (not an empty slice) when the PR has no changed files.
 // Callers can safely range over or check len() on a nil slice.
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	var allFiles []vcs.ChangedFile
 	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)
 		if err != nil {
 			return nil, fmt.Errorf("fetch PR files page %d: %w", page, err)
 		}
 		var files []changedFileResponse
 		if err := json.Unmarshal(body, &files); err != nil {
 			return nil, fmt.Errorf("parse PR files JSON: %w", err)
 		}
 		if len(files) == 0 {
 			break
 		}
 		for _, f := range files {
 			allFiles = append(allFiles, vcs.ChangedFile{
 				Filename: f.Filename,
 				Status:   f.Status,
 				Patch:    f.Patch,
 			})
 		}
 		if len(files) < 100 {
 			break
 		}
 	}
 	return allFiles, nil
 }
 // GetCommitStatuses fetches both commit statuses and check runs for a SHA,
 // merging them into a unified []vcs.CommitStatus slice.
 // Returns nil (not an empty slice) when there are no statuses or check runs.
 // If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
 // function returns immediately without attempting the check-runs endpoint.
 // If the check-runs endpoint fails after statuses were fetched successfully,
 // the function returns an error (not a partial result) so callers always get
 // either a complete view or a clear error signal.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
 	var result []vcs.CommitStatus
 	// Fetch commit statuses
 	statusURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/status",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
 	statusBody, err := c.doGet(ctx, statusURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch commit statuses: %w", err)
 	}
 	var statusResp commitStatusResponse
 	if err := json.Unmarshal(statusBody, &statusResp); err != nil {
 		return nil, fmt.Errorf("parse commit statuses JSON: %w", err)
 	}
 	for _, s := range statusResp.Statuses {
 		result = append(result, vcs.CommitStatus{
 			Context:     s.Context,
 			Status:      s.State,
 			Description: s.Description,
 			TargetURL:   s.TargetURL,
 		})
 	}
 	// Fetch check runs (paginated)
 	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)
 		if err != nil {
 			return nil, fmt.Errorf("fetch check runs page %d: %w", checkPage, err)
 		}
 		var checkResp checkRunsResponse
 		if err := json.Unmarshal(checkBody, &checkResp); err != nil {
 			return nil, fmt.Errorf("parse check runs JSON: %w", err)
 		}
 		for _, cr := range checkResp.CheckRuns {
 			result = append(result, vcs.CommitStatus{
 				Context:     cr.Name,
 				Status:      mapCheckRunStatus(cr.Conclusion),
 				Description: derefString(cr.Conclusion),
 				TargetURL:   cr.HTMLURL,
 			})
 		}
 		if len(checkResp.CheckRuns) < 100 {
 			break
 		}
 	}
 	return result, nil
 }
 // mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
 // Conclusion alone determines the mapped state: nil conclusion means the run is
 // still in progress (pending), regardless of the status field value.
 func mapCheckRunStatus(conclusion *string) string {
 	if conclusion == nil {
 		// Still running or queued
 		return "pending"
 	}
 	switch *conclusion {
 	case "success":
 		return "success"
 	case "failure", "action_required", "timed_out":
 		return "failure"
 	case "cancelled", "skipped", "neutral":
 		return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
 	case "stale", "waiting":
 		return "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
 }
@@ -0,0 +1,637 @@
 package github
 import (
 	"context"
 	"encoding/json"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"strings"
 	"testing"
 	"time"
 )
 func TestGetPullRequest_HappyPath(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path != "/repos/owner/repo/pulls/42" {
 			t.Errorf("unexpected path: %s", r.URL.Path)
 		}
 		json.NewEncoder(w).Encode(map[string]interface{}{
 			"number": 42,
 			"title":  "Test PR",
 			"body":   "Description",
 			"head":   map[string]string{"sha": "abc123", "ref": "feature-branch"},
 			"base":   map[string]string{"ref": "main"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if pr.Number != 42 {
 		t.Errorf("expected number 42, got %d", pr.Number)
 	}
 	if pr.Title != "Test PR" {
 		t.Errorf("expected title 'Test PR', got %q", pr.Title)
 	}
 	if pr.Body != "Description" {
 		t.Errorf("expected body 'Description', got %q", pr.Body)
 	}
 	if pr.Head.SHA != "abc123" {
 		t.Errorf("expected head SHA 'abc123', got %q", pr.Head.SHA)
 	}
 	if pr.Head.Ref != "feature-branch" {
 		t.Errorf("expected head ref 'feature-branch', got %q", pr.Head.Ref)
 	}
 	if pr.Base.Ref != "main" {
 		t.Errorf("expected base ref 'main', got %q", pr.Base.Ref)
 	}
 }
 func TestGetPullRequest_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequest(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 TestGetPullRequest_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 	if !IsUnauthorized(err) {
 		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
 	}
 }
 func TestGetPullRequest_429Retry(t *testing.T) {
 	attempts := 0
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
 			w.WriteHeader(429)
 			w.Write([]byte(`{"message":"rate limit"}`))
 			return
 		}
 		json.NewEncoder(w).Encode(map[string]interface{}{
 			"number": 1,
 			"title":  "PR",
 			"body":   "",
 			"head":   map[string]string{"sha": "abc", "ref": "br"},
 			"base":   map[string]string{"ref": "main"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
 	pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if pr.Number != 1 {
 		t.Errorf("expected number 1, got %d", pr.Number)
 	}
 	if attempts != 2 {
 		t.Errorf("expected 2 attempts, got %d", attempts)
 	}
 }
 func TestGetPullRequest_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`{invalid json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 	if !strings.Contains(err.Error(), "parse PR JSON") {
 		t.Errorf("expected parse error, got: %v", err)
 	}
 }
 func TestGetPullRequestDiff_HappyPath(t *testing.T) {
 	expectedDiff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,4 @@\n+// new line\n"
 	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(expectedDiff))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	diff, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 42)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if diff != expectedDiff {
 		t.Errorf("unexpected diff: %q", diff)
 	}
 	if gotAccept != "application/vnd.github.diff" {
 		t.Errorf("expected diff Accept header, got %q", gotAccept)
 	}
 }
 func TestGetPullRequestDiff_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 999)
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestGetPullRequestDiff_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 }
 func TestGetPullRequestFiles_HappyPath(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		json.NewEncoder(w).Encode([]map[string]interface{}{
 			{"filename": "main.go", "status": "modified", "patch": "@@ -1,3 +1,4 @@\n+line"},
 			{"filename": "test.go", "status": "added", "patch": "@@ -0,0 +1,5 @@\n+new file"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(files) != 2 {
 		t.Fatalf("expected 2 files, got %d", len(files))
 	}
 	if files[0].Filename != "main.go" {
 		t.Errorf("expected filename 'main.go', got %q", files[0].Filename)
 	}
 	if files[0].Status != "modified" {
 		t.Errorf("expected status 'modified', got %q", files[0].Status)
 	}
 	if files[0].Patch != "@@ -1,3 +1,4 @@\n+line" {
 		t.Errorf("unexpected patch: %q", files[0].Patch)
 	}
 }
 func TestGetPullRequestFiles_Pagination(t *testing.T) {
 	// Simulate > 100 files requiring pagination
 	page1Files := make([]map[string]string, 100)
 	for i := 0; i < 100; i++ {
 		page1Files[i] = map[string]string{
 			"filename": fmt.Sprintf("file%d.go", i),
 			"status":   "modified",
 			"patch":    fmt.Sprintf("patch%d", i),
 		}
 	}
 	page2Files := []map[string]string{
 		{"filename": "file100.go", "status": "added", "patch": "patch100"},
 	}
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		page := r.URL.Query().Get("page")
 		if page == "" || page == "1" {
 			json.NewEncoder(w).Encode(page1Files)
 		} else {
 			json.NewEncoder(w).Encode(page2Files)
 		}
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(files) != 101 {
 		t.Errorf("expected 101 files (paginated), got %d", len(files))
 	}
 	if files[100].Filename != "file100.go" {
 		t.Errorf("expected last file 'file100.go', got %q", files[100].Filename)
 	}
 	if files[100].Patch != "patch100" {
 		t.Errorf("expected last patch 'patch100', got %q", files[100].Patch)
 	}
 }
 func TestGetPullRequestFiles_BinaryFile_NoPatch(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		// Binary files have no patch field in GitHub response
 		json.NewEncoder(w).Encode([]map[string]interface{}{
 			{"filename": "image.png", "status": "added"},
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(files) != 1 {
 		t.Fatalf("expected 1 file, got %d", len(files))
 	}
 	if files[0].Patch != "" {
 		t.Errorf("expected empty patch for binary file, got %q", files[0].Patch)
 	}
 }
 func TestGetPullRequestFiles_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 999)
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestGetPullRequestFiles_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`not json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 }
 func TestGetFileContentAtRef_HappyPath(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path != "/repos/owner/repo/contents/path/to/file.go" {
 			t.Errorf("unexpected path: %s", r.URL.Path)
 		}
 		if r.URL.Query().Get("ref") != "abc123" {
 			t.Errorf("unexpected ref: %s", r.URL.Query().Get("ref"))
 		}
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "cGFja2FnZSBtYWlu", // "package main" in base64
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "path/to/file.go", "abc123")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if content != "package main" {
 		t.Errorf("expected 'package main', got %q", content)
 	}
 }
 func TestGetFileContentAtRef_EmptyRef(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Query().Get("ref") != "" {
 			t.Errorf("expected no ref param, got %q", r.URL.Query().Get("ref"))
 		}
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "aGVsbG8=", // "hello" in base64
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.txt", "")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if content != "hello" {
 		t.Errorf("expected 'hello', got %q", content)
 	}
 }
 func TestGetFileContentAtRef_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "missing.go", "main")
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestGetFileContentAtRef_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 }
 func TestGetFileContentAtRef_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`not valid json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 }
 func TestGetFileContentAtRef_429Retry(t *testing.T) {
 	attempts := 0
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		attempts++
 		if attempts == 1 {
 			w.WriteHeader(429)
 			w.Write([]byte(`{"message":"rate limit"}`))
 			return
 		}
 		json.NewEncoder(w).Encode(map[string]string{
 			"content":  "b2s=", // "ok" in base64
 			"encoding": "base64",
 		})
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	c.SetRetryBackoff([]time.Duration{1 * time.Millisecond})
 	content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if content != "ok" {
 		t.Errorf("expected 'ok', got %q", content)
 	}
 	if attempts != 2 {
 		t.Errorf("expected 2 attempts, got %d", attempts)
 	}
 }
 func TestGetCommitStatuses_HappyPath(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch {
 		case strings.Contains(r.URL.Path, "/status"):
 			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"):
 			conclusion := "success"
 			json.NewEncoder(w).Encode(map[string]interface{}{
 				"total_count": 1,
 				"check_runs": []map[string]interface{}{
 					{
 						"name":       "lint",
 						"conclusion": &conclusion,
 						"status":     "completed",
 						"html_url":   "https://github.com/check/1",
 					},
 				},
 			})
 		default:
 			t.Errorf("unexpected path: %s", r.URL.Path)
 			w.WriteHeader(404)
 		}
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
 	if len(statuses) != 2 {
 		t.Fatalf("expected 2 statuses, got %d", len(statuses))
 	}
 	// First should be from commit statuses
 	if statuses[0].Context != "ci/build" {
 		t.Errorf("expected context 'ci/build', got %q", statuses[0].Context)
 	}
 	if statuses[0].Status != "success" {
 		t.Errorf("expected status 'success', got %q", statuses[0].Status)
 	}
 	// Second should be from check runs
 	if statuses[1].Context != "lint" {
 		t.Errorf("expected context 'lint', got %q", statuses[1].Context)
 	}
 	if statuses[1].Status != "success" {
 		t.Errorf("expected status 'success', got %q", statuses[1].Status)
 	}
 }
 func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
 	tests := []struct {
 		conclusion *string
 		status     string
 		want       string
 	}{
 		{stringPtr("success"), "completed", "success"},
 		{stringPtr("failure"), "completed", "failure"},
 		{stringPtr("action_required"), "completed", "failure"},
 		{stringPtr("timed_out"), "completed", "failure"},
 		{stringPtr("cancelled"), "completed", "success"},
 		{stringPtr("skipped"), "completed", "success"},
 		{stringPtr("neutral"), "completed", "success"},
 		{nil, "in_progress", "pending"},
 		{nil, "queued", "pending"},
 	}
 	for _, tt := range tests {
 		name := "nil"
 		if tt.conclusion != nil {
 			name = *tt.conclusion
 		}
 		t.Run(name, func(t *testing.T) {
 			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{}{
 						"state":    "success",
 						"statuses": []interface{}{},
 					})
 					return
 				}
 				json.NewEncoder(w).Encode(map[string]interface{}{
 					"total_count": 1,
 					"check_runs": []map[string]interface{}{
 						{
 							"name":       "check",
 							"conclusion": tt.conclusion,
 							"status":     tt.status,
 							"html_url":   "https://github.com/check/1",
 						},
 					},
 				})
 			}))
 			defer srv.Close()
 			c := NewClient("token", srv.URL, AllowInsecureHTTP())
 			c.SetHTTPClient(srv.Client())
 			statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha1")
 			if err != nil {
 				t.Fatalf("unexpected error: %v", err)
 			}
 			if len(statuses) != 1 {
 				t.Fatalf("expected 1 status, got %d", len(statuses))
 			}
 			if statuses[0].Status != tt.want {
 				t.Errorf("expected status %q, got %q", tt.want, statuses[0].Status)
 			}
 		})
 	}
 }
 func TestGetCommitStatuses_404(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(404)
 		w.Write([]byte(`{"message":"Not Found"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "badsha")
 	if err == nil {
 		t.Fatal("expected error for 404")
 	}
 }
 func TestGetCommitStatuses_401(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(401)
 		w.Write([]byte(`{"message":"Bad credentials"}`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
 	if err == nil {
 		t.Fatal("expected error for 401")
 	}
 }
 func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(200)
 		w.Write([]byte(`not json`))
 	}))
 	defer srv.Close()
 	c := NewClient("token", srv.URL, AllowInsecureHTTP())
 	c.SetHTTPClient(srv.Client())
 	_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
 	if err == nil {
 		t.Fatal("expected error for malformed JSON")
 	}
 }
 func stringPtr(s string) *string {
 	return &s
 }
@@ -1,3 +1,5 @@
 //go:build phase2
 package vcs_test
 import (
@@ -5,7 +7,21 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
-// Compile-time assertion: the gitea.Adapter satisfies vcs.Client.
+// Compile-time assertion: documents the gap between gitea.Client and vcs.Client.
-// (The raw gitea.Client does NOT satisfy vcs.Client due to signature differences;
+// Guarded by the "phase2" build tag — enable once the Gitea adapter bridges these gaps:
-// the Adapter bridges them.)
+//
-var _ vcs.Client = (*gitea.Adapter)(nil)
+//  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)
Author	SHA1	Message	Date
claw	6316007eb1	fix: address review findings from reviews #2955 and #2958 - Convert handleResponse to package-level function (unused receiver) [#17955] - Add clarifying comment for nil resp on transport error [#17956] - Use consistent %w wrapping in dual-unmarshal error path [#17957] - Add SafeError() method to APIError for safe logging [#17964] - Enforce safe CheckRedirect policy in SetHTTPClient [#17965] - Add tests for SafeError and SetHTTPClient enforcement	2026-05-12 21:19:01 -07:00
claw	b380e7fcae	refactor(github): extract handleResponse for safe defer body close PR Ready Gate / clear-labels (pull_request) Successful in 1s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m16s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m44s Details Address review findings #1 and #2: the response body was closed explicitly rather than via defer, which could leak if future code paths were added. Extract handleResponse helper method that uses defer resp.Body.Close() to guarantee cleanup. This avoids the loop-defer antipattern (defer inside a for loop accumulates defers until function exit) by isolating the body handling into its own function scope.	2026-05-12 20:47:59 -07:00
claw	30798ff023	fix: address sonnet review MINOR findings (#2916 ) PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 18s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 46s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 59s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m6s Details - client.go: fix misleading timer.Stop() comment (finding #1) - pr.go: document all-or-nothing semantics for GetCommitStatuses when check-runs endpoint fails after statuses succeed (finding #2) - files.go: include both array and object unmarshal errors in ListContents fallback error message (finding #3) - pr.go: expand mapCheckRunStatus comment to explain non-blocking policy decision (finding #4)	2026-05-12 20:28:52 -07:00
claw	6e8e744816	fix(github): address self-review findings from `1194bc75` PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 51s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m22s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m36s Details - Handle io.ReadAll error on error body read (client.go:265) - Remove unused State field from commitStatusResponse (pr.go) - Guard via slice access in defaultCheckRedirect (client.go:117) - Move GetFileContentAtRef from pr.go to files.go (logical home)	2026-05-12 19:40:30 -07:00
claw	1194bc758c	fix(github): address review findings from rounds 2884/2885/2887 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 18s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m18s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m44s Details - Fix response body limit check: read maxResponseBytes+1 and use > to distinguish exactly-at-limit from truncated (sonnet finding #1) - Reject HTTPS→HTTP redirects outright instead of stripping auth and following; prevents plaintext metadata leakage (sonnet #2, security #1) - Sanitize newlines in APIError.Error to prevent log injection from upstream response bodies (security #2) - Add nil-return documentation to GetCommitStatuses (sonnet #3) - Gate TestDoRequest_429RetryAfterHTTPDate behind testing.Short (sonnet #6) - Add tests for redirect policy, exact-at-limit body, and error sanitization	2026-05-12 19:29:06 -07:00
claw	80af5037b2	fix(github): address review findings from round 2880/2883 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 24s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 43s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m16s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m21s Details Sonnet MINOR #1: Stop timer after <-timer.C fires for idiomatic cleanup. Sonnet MINOR #2: Document that empty array from contents API is valid (empty dir). Sonnet MINOR #3: Document that GetPullRequestFiles returns nil for no files. Sonnet NIT #4: Strengthen SetHTTPClient/SetRetryBackoff docs to clarify test-only intent. Sonnet NIT #5: Document GetCommitStatuses fail-fast behavior. Sonnet NIT #6: Document double-slash collapsing in escapePath. Security MINOR #1: Document redirect policy responsibility when providing custom client. Security MINOR #2: Reduce maxErrorBodyBytes from 64KB to 4KB to limit sensitive data exposure.	2026-05-12 18:41:44 -07:00
claw	5b2fa0b9af	refactor(github): address review findings from round 2872 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m53s Details - client.go: clarify timer drain comment (finding #1) - client.go: rename t -> retryAt for time.Time clarity (finding #2) - pr.go: remove dead _ string parameter from mapCheckRunStatus (finding #3) - files.go: add inline comment explaining zero-value guard (finding #4) Findings #5 (NIT, no code change) and #6 (NIT, defer vs t.Cleanup in t.Run closures) pushed back — see PR comment.	2026-05-12 18:16:43 -07:00
claw	491df7cb1f	fix(github): address review findings from rounds 2867/2870 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 18s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 41s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m20s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s Details - Extract duplicated CheckRedirect lambda to defaultCheckRedirect function (sonnet #1: eliminate duplication between NewClient and SetHTTPClient) - Remove unnecessary int64 cast in response size check (sonnet #3) - Validate fallback unmarshal in ListContents to reject zero-value entries (sonnet #5: prevent accepting unexpected JSON formats silently) - Rename strPtr to stringPtr for consistency (sonnet #6) - Add doc comment about APIError.Error body exposure (security #3) Deferred to separate issues: - #95: Reject cross-host redirects entirely (security #1) - #96: Add safeguards for AllowInsecureHTTP (security #2)	2026-05-12 17:30:24 -07:00
claw	1fcc0b738a	fix(github): address MINOR/NIT findings from review #2866 PR Ready Gate / clear-labels (pull_request) Successful in 1s Details CI / test (pull_request) Successful in 18s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m30s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m8s Details - SetHTTPClient(nil): preserve CheckRedirect auth-stripping policy instead of restoring a plain http.Client that loses cross-host protection. - Authorization header: add comment documenting why Bearer scheme is correct (OAuth2 standard, works for both classic PATs and fine-grained tokens). - Retry-After parsing: support HTTP-date format (RFC 7231) in addition to integer seconds. GitHub only sends integers today, but the implementation is now spec-compliant. - escapePath dot-segment removal: document the behavior in public API doc comments for ListContents and GetFileContentAtRef so callers are aware without reading the internal helper.	2026-05-12 17:13:07 -07:00
claw	fce5f2d184	fix(github): address review findings on client.go PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m23s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m15s Details - Use net/url.Parse for HTTPS scheme check (case-insensitive) - Guard SetHTTPClient against nil (restores default 30s client) - Rename 'url' param to 'reqURL' in doRequest/doGet for clarity - Return error when response exceeds maxResponseBytes instead of silently truncating Finding #1 (Bearer auth scheme) intentionally kept: GitHub REST API officially supports and recommends Bearer for all token types. See: https://docs.github.com/en/rest/authentication/authenticating-to-the-rest-api	2026-05-12 16:55:32 -07:00
claw	af72c64b7f	fix(github): correct ListContents error wrapping and move HTTPS guard before retry loop PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m11s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m11s Details	2026-05-12 16:48:39 -07:00
claw	1bc3f206ba	fix: address review findings from rounds 2843-2846 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 41s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m13s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m23s Details - Remove redundant timer.Stop() after timer fires (Sonnet #1, GPT #2) - Remove unused TotalCount field from checkRunsResponse (Sonnet #2) - Improve escapePath doc comment to explain deliberate silent stripping (Sonnet #3) - Fix ListContents to handle both array (directory) and object (single file) responses from GitHub Contents API (GPT #3) - Add HTTPS enforcement: refuse to send credentials over non-HTTPS URLs unless AllowInsecureHTTP() option is passed (Security #1) - Replace constant-value test with actual behavior test for response body limiting (Sonnet #6) - Run gofmt for consistent formatting (Sonnet #4) - Add tests for HTTPS enforcement and ListContents single-file handling	2026-05-12 16:39:01 -07:00
claw	c10bb72117	fix: address self-review NIT findings on PR #93 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 22s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 37s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m9s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m17s Details - Add timer.Stop() on happy path in retry loop (idiomatic) - Add concurrency caveat to Client doc comment for SetHTTPClient/SetRetryBackoff - Add explicit 'stale'/'waiting' cases to mapCheckRunStatus	2026-05-12 16:25:32 -07:00
claw	ae91c8aef5	fix: address review findings from rounds 2834-2838 PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 49s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m6s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m19s Details - Unexport RetryBackoff, add SetRetryBackoff method (#17286) - Rename http field to httpClient to avoid shadowing (#17289) - Group const blocks into single declaration (#17291) - Fix CheckRedirect to compare against previous hop, not first (#17302) - Strip auth header on protocol downgrade https→http (#17297) - Add maxPages safeguard to pagination loops (#17299, #17300) - Document mapCheckRunStatus unused second parameter (#17287, #17303)	2026-05-12 16:11:58 -07:00
claw	75f65fbf5d	fix: address MINOR review findings on PR #93 (round 2) PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / test (pull_request) Successful in 17s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 38s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m50s Details - Add User-Agent header to all requests (gpt-review-bot) - Limit successful response body to 10 MiB via io.LimitReader (security-review-bot) - Add CheckRedirect to strip Authorization on cross-host redirects (security-review-bot) - Fix decodeBase64Content to strip both \r and \n (gpt-review-bot) - Document that transport errors are not retried (sonnet-review-bot) - Update package doc to reflect current scope (no review submission yet) - Add tests for User-Agent, empty-token auth skip, CRLF base64, CheckRedirect	2026-05-12 16:00:09 -07:00
claw	5b43afc6d4	fix: address review feedback on PR #93 PR Ready Gate / clear-labels (pull_request) Successful in 1s Details CI / test (pull_request) Successful in 23s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m48s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m7s Details - Fix Retry-After slice mutation: copy c.RetryBackoff before modifying to prevent permanent mutation of the shared slice (sonnet#1, security#1) - Cap Retry-After to 120s maximum to prevent excessive sleeps (security#2) - Guard auth header: only set Authorization when token is non-empty (gpt#2) - Fix GetFileContent doc comment to match actual behavior (sonnet#3, gpt#1) - Remove dead 'in_progress/queued' case in mapCheckRunStatus (sonnet#4) - Add testing.Short() guard to slow retry test (sonnet#5) - Reject dot-segments in escapePath to prevent path traversal (security#3) - Add regression tests for non-mutation and escapePath safety	2026-05-12 15:43:45 -07:00
claw	d1ef1e21e5	feat(github): implement PRReader + FileReader client (#80 ) CI / test (pull_request) Successful in 18s Details PR Ready Gate / clear-labels (pull_request) Successful in 2s Details CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s Details CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m45s Details CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m56s Details Implement the GitHub API client with PRReader and FileReader interface conformance for both github.com and GitHub Enterprise. New files: - github/client.go: Client struct, NewClient with configurable base URL, HTTP helpers with 429 retry and Retry-After support - github/pr.go: GetPullRequest, GetPullRequestDiff (per-request Accept header), GetPullRequestFiles (paginated, populates Patch field), GetFileContentAtRef (base64 decode), GetCommitStatuses (merges commit statuses + check runs with conclusion mapping) - github/files.go: GetFileContent (delegates to GetFileContentAtRef), ListContents, escapePath, decodeBase64Content helpers Type changes: - vcs/types.go: Add Patch field to ChangedFile struct Tests cover: happy path, 404, 401, 429+retry, malformed response, pagination, binary files, check run conclusion mapping, base64 decoding. Compile-time checks: var _ vcs.PRReader = (Client)(nil) var _ vcs.FileReader = (Client)(nil) Exit criteria met: - go test ./github/... passes (all methods) - NewClient with empty baseURL uses https://api.github.com - NewClient with GHE URL targets correctly - GetFileContent delegates to GetFileContentAtRef with empty ref - GetPullRequestFiles paginates and populates Patch field - GetCommitStatuses merges both commit statuses and check-runs	2026-05-12 15:18:55 -07:00