fix(gitea): guard against empty response in ListContents fallback

Add defensive check for empty Name and Path fields when unmarshaling
a single ContentEntry in the fallback path. While Gitea API won't
return empty objects for valid file paths, this guard:
- Explicitly documents the invariant we expect
- Catches potential API behavior changes early
- Costs nothing at runtime

Addresses [MINOR] from sonnet-review-bot on PR #74.

.gitea/workflows/ci.yml

@@ -38,6 +38,8 @@ jobs:
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
             model: gpt-5
+            patterns_repo: rodin/security-patterns
+            patterns_files: "."
             system_prompt_file: SECURITY_REVIEW.md
     steps:
       - uses: actions/checkout@v4
@@ -60,8 +62,8 @@ jobs:
           AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
           AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
           CONVENTIONS_FILE: "CONVENTIONS.md"
-          PATTERNS_REPO: "rodin/go-patterns"
-          PATTERNS_FILES: "README.md,patterns/"
+          PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
+          PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
           LLM_TIMEOUT: "600"
           SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
         run: ./review-bot

README.md

@@ -329,11 +329,12 @@ All flags have environment variable equivalents:
 ### Token Scopes Required
 
 | Scope | Purpose |
-|-------|---------|
+|-------|--------|
 | `write:issue` | Post and delete reviews |
 | `write:repository` | Read PR diffs, file content, commit statuses |
+| `read:user` | Self-request as reviewer (optional but recommended) |
 
-No `read:user` scope needed — the bot identifies itself from the review response.
+Without `read:user`, the bot still works but cannot add itself to the PR's reviewer list.
 
 ## Development
 
@@ -459,41 +460,6 @@ YAML is the recommended format for personas because it supports:
 
 JSON is also supported for backwards compatibility—just use `.json` extension.
 
-### Repository Personas (Auto-Discovery)
-
-Repositories can ship their own personas in `.review-bot/personas/`. When you specify `--persona <name>`, review-bot will:
-
-1. **Try to load from the target repo** — Checks `.review-bot/personas/<name>.yaml` (or `.yml`)
-2. **Fall back to built-in** — If not found in repo, uses the built-in persona
-
-This lets each repo define domain-specific personas without modifying CI config:
-
-```
-my-trading-repo/
-├── .review-bot/
-│   └── personas/
-│       ├── trading.yaml      # Custom trading persona
-│       └── regulatory.yaml   # Compliance-focused reviews
-├── lib/
-└── ...
-```
-
-```yaml
-# CI config (no persona-file needed)
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: trading
-    persona: trading           # Will find .review-bot/personas/trading.yaml
-    ...
-```
-
-**Priority order:**
-1. Repo's `.review-bot/personas/<name>.yaml`
-2. Built-in persona with matching name
-3. Error if neither exists
-
-This allows repos to override built-in personas (e.g., a custom `security` persona that adds project-specific rules) while keeping the simple `persona: security` syntax in CI.
-
 
 ### Persona vs system-prompt-file
 

cmd/review-bot/main.go

@@ -115,9 +115,7 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Persona loading is deferred until after giteaClient is initialized,
-	// so we can try loading from the target repo first.
-	var persona *review.Persona
+	// NOTE: Persona loading deferred until after Gitea client init to support repo personas
 
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
@@ -175,23 +173,22 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
 	defer cancel()
 
-	// Load persona: try remote repo first, then fall back to built-in
+	// Load persona if specified (after Gitea client init to support repo personas)
+	var persona *review.Persona
 	if *personaName != "" {
-		// Try loading from target repo's .review-bot/personas/ directory
-		fetcher := &giteaFetcher{client: giteaClient}
-		remotePersonas, err := review.LoadRemotePersonas(ctx, fetcher, owner, repoName)
+		// Try loading from repo first, then fall back to built-in
+		repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
 		if err != nil {
-			slog.Warn("could not load remote personas", "repo", fmt.Sprintf("%s/%s", owner, repoName), "error", err)
-			// Assign empty map so the lookup below doesn't panic
-			remotePersonas = map[string]*review.Persona{}
+			slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
+			// Continue with built-in personas only.
+			// NOTE: repoPersonas is nil here, but map indexing on a nil map is safe in Go
+			// (returns the zero value), so the fallback to built-in below works correctly.
 		}
-
-		if p, ok := remotePersonas[*personaName]; ok {
+		if p, ok := repoPersonas[*personaName]; ok {
 			persona = p
-			slog.Info("loaded persona from target repo", "persona", persona.Name, "display", persona.DisplayName)
+			slog.Info("loaded repo persona", "persona", persona.Name, "display", persona.DisplayName, "repo", owner+"/"+repoName)
 		} else {
-			// Fall back to built-in persona
-			var err error
+			// Fall back to built-in
 			persona, err = review.LoadBuiltinPersona(*personaName)
 			if err != nil {
 				slog.Error("failed to load persona", "persona", *personaName, "error", err)
@@ -205,12 +202,11 @@ func main() {
 			slog.Error("invalid persona-file path", "error", err)
 			os.Exit(1)
 		}
-		loadedPersona, loadErr := review.LoadPersona(resolvedPath)
-		if loadErr != nil {
-			slog.Error("failed to load persona file", "file", *personaFile, "error", loadErr)
+		persona, err = review.LoadPersona(resolvedPath)
+		if err != nil {
+			slog.Error("failed to load persona file", "file", *personaFile, "error", err)
 			os.Exit(1)
 		}
-		persona = loadedPersona
 		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
 	}
 
@@ -548,6 +544,9 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
 		}
 		owner, repo := parts[0], parts[1]
 
+		var repoLoadedFiles []string
+		var repoSkippedFiles []string
+
 		for _, path := range paths {
 			path = strings.TrimSpace(path)
 			if path == "" {
@@ -563,11 +562,22 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
 			for filePath, content := range files {
 				// Only include markdown and text files as patterns
 				if !isPatternFile(filePath) {
+					repoSkippedFiles = append(repoSkippedFiles, filePath)
 					continue
 				}
+				repoLoadedFiles = append(repoLoadedFiles, filePath)
 				sb.WriteString(fmt.Sprintf("### %s/%s\n\n%s\n\n", repoRef, filePath, content))
 			}
 		}
+
+		if len(repoLoadedFiles) > 0 {
+			slog.Info("loaded pattern files", "repo", repoRef, "count", len(repoLoadedFiles), "files", repoLoadedFiles)
+		} else {
+			slog.Warn("no pattern files loaded", "repo", repoRef, "paths", paths)
+		}
+		if len(repoSkippedFiles) > 0 {
+			slog.Debug("skipped non-pattern files", "repo", repoRef, "count", len(repoSkippedFiles), "files", repoSkippedFiles)
+		}
 	}
 	return sb.String()
 }
@@ -802,17 +812,20 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
 	return evaluatedSHA != currentSHA
 }
 
-// giteaFetcher adapts gitea.Client to review.PersonaFetcher interface.
-type giteaFetcher struct {
+// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface.
+type giteaClientAdapter struct {
 	client *gitea.Client
 }
 
-func (f *giteaFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
-	entries, err := f.client.ListContents(ctx, owner, repo, path)
+func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
+	return &giteaClientAdapter{client: c}
+}
+
+func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
+	entries, err := a.client.ListContents(ctx, owner, repo, path)
 	if err != nil {
 		return nil, err
 	}
-	// Convert gitea.ContentEntry to review.ContentEntry
 	result := make([]review.ContentEntry, len(entries))
 	for i, e := range entries {
 		result[i] = review.ContentEntry{
@@ -824,6 +837,6 @@ func (f *giteaFetcher) ListContents(ctx context.Context, owner, repo, path strin
 	return result, nil
 }
 
-func (f *giteaFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return f.client.GetFileContent(ctx, owner, repo, filepath)
+func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	return a.client.GetFileContent(ctx, owner, repo, filepath)
 }

gitea/client.go

@@ -11,9 +11,11 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"net"
 	"net/http"
 	"net/url"
 	"strings"
+	"syscall"
 	"time"
 )
 
@@ -39,12 +41,26 @@ func IsNotFound(err error) bool {
 	return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
 }
 
+// IsServerError reports whether an error is an API 5xx response.
+func IsServerError(err error) bool {
+	var apiErr *APIError
+	return errors.As(err, &apiErr) && apiErr.StatusCode >= 500 && apiErr.StatusCode < 600
+}
+
 // Client interacts with the Gitea API.
 // A Client is safe for concurrent use by multiple goroutines.
 type Client struct {
 	baseURL string
 	token   string
 	http    *http.Client
+
+	// RetryBackoff defines the delays between retry attempts.
+	// RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
+	// If nil, defaults to {1s, 2s}. Set to shorter durations in tests.
+	//
+	// This field must be configured before the first request is made.
+	// Modifying it while requests are in flight is not safe.
+	RetryBackoff []time.Duration
 }
 
 // NewClient creates a new Gitea API client.
@@ -56,6 +72,12 @@ func NewClient(baseURL, token string) *Client {
 	}
 }
 
+// SetHTTPClient sets the underlying HTTP client used for requests.
+// This is intended for testing to inject mock transports.
+func (c *Client) SetHTTPClient(hc *http.Client) {
+	c.http = hc
+}
+
 // PullRequest holds relevant PR metadata.
 type PullRequest struct {
 	Title string `json:"title"`
@@ -210,24 +232,185 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 	return &review, nil
 }
 
+// isTemporaryNetError reports whether err is a temporary network error worth retrying.
+// This includes connection refused, network unreachable, connection reset, and DNS
+// timeouts. It explicitly excludes permanent errors like permission denied or
+// "no such host" DNS failures.
+func isTemporaryNetError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for OpError and inspect the underlying syscall error.
+	// Not all OpErrors are transient — permission denied, for example, is permanent.
+	var opErr *net.OpError
+	if errors.As(err, &opErr) {
+		return isRetriableSyscallError(opErr.Err)
+	}
+
+	// DNS errors: only retry on timeout, not on "no such host" which is permanent.
+	var dnsErr *net.DNSError
+	if errors.As(err, &dnsErr) {
+		return dnsErr.IsTimeout
+	}
+
+	// Check for net.Error with Timeout() (Temporary is deprecated)
+	var netErr net.Error
+	if errors.As(err, &netErr) {
+		return netErr.Timeout()
+	}
+
+	return false
+}
+
+// isRetriableSyscallError reports whether the underlying error from a net.OpError
+// is a transient syscall error worth retrying.
+func isRetriableSyscallError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for syscall.Errno directly or wrapped
+	var errno syscall.Errno
+	if errors.As(err, &errno) {
+		switch errno {
+		case syscall.ECONNREFUSED, // connection refused — server not listening
+			syscall.ECONNRESET,   // connection reset by peer
+			syscall.ENETUNREACH,  // network unreachable
+			syscall.EHOSTUNREACH, // host unreachable
+			syscall.ETIMEDOUT:    // connection timed out
+			return true
+		default:
+			// EACCES, EPERM, etc. are permanent — don't retry
+			return false
+		}
+	}
+
+	// If we can't identify the specific syscall error, be conservative and retry.
+	// This handles wrapped errors or platform-specific error types.
+	// The retry count is limited, so erring on the side of retrying is safe.
+	return true
+}
+
+// redactURL strips query parameters from a URL for safe logging.
+// This prevents accidental exposure of sensitive data that future callers
+// might pass via query strings.
+func redactURL(rawURL string) string {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		// If we cannot parse it, return a safe placeholder rather than
+		// potentially logging something sensitive.
+		return "[invalid URL]"
+	}
+	if parsed.RawQuery != "" {
+		parsed.RawQuery = "[redacted]"
+	}
+	return parsed.String()
+}
+
+// sanitizeErrorForLog returns a loggable version of an error that omits
+// potentially sensitive content like response bodies. For APIError, only
+// the status code is included; for other errors, the type is preserved.
+func sanitizeErrorForLog(err error) string {
+	if err == nil {
+		return "<nil>"
+	}
+	var apiErr *APIError
+	if errors.As(err, &apiErr) {
+		return fmt.Sprintf("HTTP %d", apiErr.StatusCode)
+	}
+	return err.Error()
+}
+
+// doGet performs an HTTP GET request with retry on 5xx errors and temporary
+// network errors. Retries up to 3 times with exponential backoff (1s, 2s delays
+// by default; configurable via Client.RetryBackoff for testing).
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil)
-	if err != nil {
-		return nil, err
+	const maxAttempts = 3
+	// backoff[i] is the delay before attempt i+1 (i.e., after attempt i fails).
+	// First attempt (i=0) has no delay; retries wait 1s then 2s by default.
+	backoff := c.RetryBackoff
+	if backoff == nil {
+		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
 	}
-	req.Header.Set("Authorization", "token "+c.token)
 
-	resp, err := c.http.Do(req)
-	if err != nil {
-		return nil, err
-	}
-	defer resp.Body.Close()
+	// maxErrorBodyBytes limits how much of an error response body we read
+	// to protect against malicious servers sending unbounded data.
+	const maxErrorBodyBytes = 64 * 1024 // 64 KB
 
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		body, _ := io.ReadAll(resp.Body)
-		return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
+	var lastErr error
+	for attempt := 0; attempt < maxAttempts; attempt++ {
+		if attempt > 0 {
+			// Determine delay: use backoff slice if available, otherwise retry immediately.
+			// An empty RetryBackoff slice means "retry without delay" — this is intentional
+			// as the caller explicitly configured no delays.
+			var delay time.Duration
+			if attempt-1 < len(backoff) {
+				delay = backoff[attempt-1]
+			}
+
+			if delay > 0 {
+				slog.Warn("retrying request after error",
+					"attempt", attempt+1,
+					"url", redactURL(reqURL),
+					"delay", delay.String(),
+					"lastError", sanitizeErrorForLog(lastErr))
+
+				timer := time.NewTimer(delay)
+				select {
+				case <-timer.C:
+				case <-ctx.Done():
+					timer.Stop()
+					return nil, ctx.Err()
+				}
+			}
+		}
+
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil)
+		if err != nil {
+			return nil, err
+		}
+		req.Header.Set("Authorization", "token "+c.token)
+
+		resp, err := c.http.Do(req)
+		if err != nil {
+			// Always capture the error for consistent return at loop end.
+			// This ensures both network errors and HTTP 5xx return lastErr.
+			lastErr = err
+
+			// Only retry temporary network errors when attempts remain.
+			if attempt < maxAttempts-1 && isTemporaryNetError(err) {
+				slog.Warn("temporary network error, will retry",
+					"attempt", attempt+1,
+					"url", redactURL(reqURL),
+					"error", err)
+				continue
+			}
+			// Non-retryable network error or final attempt exhausted.
+			return nil, lastErr
+		}
+		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
+			body, err := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			if err != nil {
+				return nil, err
+			}
+			return body, nil
+		}
+
+		// Error path: limit how much we read from potentially malicious server
+		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		resp.Body.Close()
+
+		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
+
+		// Only retry on 5xx server errors
+		if resp.StatusCode < 500 || resp.StatusCode >= 600 {
+			return nil, lastErr
+		}
 	}
-	return io.ReadAll(resp.Body)
+
+	return nil, lastErr
 }
 
 // escapePath escapes each segment of a relative file path for use in URLs.
@@ -251,7 +434,13 @@ type ContentEntry struct {
 
 // ListContents lists files and directories at a given path in a repo.
 // Pass an empty path to list the repository root.
+// If the path points to a file (not a directory), Gitea returns a single
+// object instead of an array; this method normalizes both cases to a slice.
 func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
+	// Normalize "." to empty string — Gitea API rejects "." with 500
+	if path == "." {
+		path = ""
+	}
 	var reqURL string
 	if path == "" {
 		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
@@ -264,7 +453,16 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 	}
 	var entries []ContentEntry
 	if err := json.Unmarshal(body, &entries); err != nil {
-		return nil, fmt.Errorf("parse contents JSON: %w", err)
+		// Gitea returns a single object (not an array) when path is a file
+		var single ContentEntry
+		if err2 := json.Unmarshal(body, &single); err2 != nil {
+			return nil, fmt.Errorf("parse contents JSON: %w", err)
+		}
+		// Guard against empty/malformed responses
+		if single.Name == "" && single.Path == "" {
+			return nil, fmt.Errorf("parse contents JSON: empty response for path %q", path)
+		}
+		entries = []ContentEntry{single}
 	}
 	return entries, nil
 }
@@ -317,9 +515,9 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
 
 // Review represents a pull request review from the Gitea API.
 type Review struct {
-	ID       int64  `json:"id"`
-	Body     string `json:"body"`
-	User     struct {
+	ID   int64  `json:"id"`
+	Body string `json:"body"`
+	User struct {
 		Login string `json:"login"`
 	} `json:"user"`
 	State    string `json:"state"`

gitea/client_test.go

@@ -6,10 +6,14 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"strings"
+	"sync/atomic"
+	"syscall"
 	"testing"
+	"time"
 )
 
 func TestGetPullRequest(t *testing.T) {
@@ -276,11 +280,64 @@ func TestListContents(t *testing.T) {
 	}
 }
 
+func TestListContents_DotPath(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// "." should be normalized to empty path, which hits the root contents endpoint
+		if r.URL.Path != "/api/v1/repos/owner/repo/contents" {
+			t.Errorf("expected root contents path, got: %s", r.URL.Path)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprintf(w, `[{"name":"README.md","path":"README.md","type":"file"}]`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	entries, err := client.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 entries[0].Name != "README.md" {
+		t.Errorf("expected README.md, got %s", entries[0].Name)
+	}
+}
+
+func TestListContents_FilePath(t *testing.T) {
+	// Gitea returns a single object (not an array) when path is a file
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/repos/owner/repo/contents/README.md" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		// Single object, not an array
+		fmt.Fprintf(w, `{"name":"README.md","path":"README.md","type":"file"}`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	entries, err := client.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 README.md, got %s", entries[0].Name)
+	}
+	if entries[0].Type != "file" {
+		t.Errorf("expected type file, got %s", entries[0].Type)
+	}
+}
+
 func TestGetAllFilesInPath_File(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" {
-			// Gitea returns 404 for contents API on files (it's not a dir)
-			http.NotFound(w, r)
+			// Gitea returns a single object (not array) when path is a file
+			w.Header().Set("Content-Type", "application/json")
+			fmt.Fprintf(w, `{"name":"README.md","path":"README.md","type":"file"}`)
 			return
 		}
 		if r.URL.Path == "/api/v1/repos/owner/repo/raw/README.md" {
@@ -584,9 +641,9 @@ func TestGetAllFilesInPath_403Propagates(t *testing.T) {
 
 func TestIsNotFound(t *testing.T) {
 	tests := []struct {
-		name   string
-		err    error
-		want   bool
+		name string
+		err  error
+		want bool
 	}{
 		{"nil error", nil, false},
 		{"non-API error", fmt.Errorf("network timeout"), false},
@@ -743,3 +800,347 @@ func TestResolveComment_Error(t *testing.T) {
 		t.Fatal("expected error for 404 response")
 	}
 }
+
+func TestIsServerError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil error", nil, false},
+		{"non-API error", fmt.Errorf("network timeout"), false},
+		{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, false},
+		{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, true},
+		{"502 APIError", &APIError{StatusCode: 502, Body: "bad gateway"}, true},
+		{"503 APIError", &APIError{StatusCode: 503, Body: "unavailable"}, true},
+		{"599 APIError", &APIError{StatusCode: 599, Body: "edge case"}, true},
+		{"600 not server error", &APIError{StatusCode: 600, Body: "edge"}, false},
+		{"400 not server error", &APIError{StatusCode: 400, Body: "bad request"}, false},
+		{"wrapped 500", fmt.Errorf("fetch: %w", &APIError{StatusCode: 500, Body: "err"}), true},
+		{"wrapped 404", fmt.Errorf("fetch: %w", &APIError{StatusCode: 404, Body: "err"}), false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := IsServerError(tt.err)
+			if got != tt.want {
+				t.Errorf("IsServerError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestDoGet_RetriesOn500(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		if attempts < 3 {
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte(`{"message":"transient error"}`))
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"data":"success"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	// Use short backoff for fast tests
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	body, err := client.doGet(context.Background(), server.URL+"/test")
+	if err != nil {
+		t.Fatalf("expected success after retry, got error: %v", err)
+	}
+	if string(body) != `{"data":"success"}` {
+		t.Errorf("body = %q, want %q", string(body), `{"data":"success"}`)
+	}
+	if attempts != 3 {
+		t.Errorf("attempts = %d, want 3", attempts)
+	}
+}
+
+func TestDoGet_FailsAfterMaxRetries(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(`{"message":"persistent error"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	// Use short backoff for fast tests
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	_, err := client.doGet(context.Background(), server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error after max retries")
+	}
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected APIError, got: %v", err)
+	}
+	if apiErr.StatusCode != http.StatusInternalServerError {
+		t.Errorf("status = %d, want 500", apiErr.StatusCode)
+	}
+	if attempts != 3 {
+		t.Errorf("attempts = %d, want 3 (max retries)", attempts)
+	}
+}
+
+func TestDoGet_NoRetryOn4xx(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusForbidden)
+		w.Write([]byte(`{"message":"forbidden"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	_, err := client.doGet(context.Background(), server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error for 403")
+	}
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected APIError, got: %v", err)
+	}
+	if apiErr.StatusCode != http.StatusForbidden {
+		t.Errorf("status = %d, want 403", apiErr.StatusCode)
+	}
+	if attempts != 1 {
+		t.Errorf("attempts = %d, want 1 (no retry on 4xx)", attempts)
+	}
+}
+
+func TestDoGet_RespectsContextCancellation(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(`{"message":"error"}`))
+	}))
+	defer server.Close()
+
+	ctx, cancel := context.WithCancel(context.Background())
+
+	client := NewClient(server.URL, "test-token")
+	// Use longer backoff to give us time to cancel during the wait
+	client.RetryBackoff = []time.Duration{100 * time.Millisecond, 100 * time.Millisecond}
+
+	// Cancel after first attempt returns and retry begins
+	go func() {
+		time.Sleep(20 * time.Millisecond)
+		cancel()
+	}()
+
+	_, err := client.doGet(ctx, server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error on context cancellation")
+	}
+	// Should have made 1 attempt, then context cancelled during backoff
+	if attempts != 1 {
+		t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
+	}
+}
+
+
+// mockTransport is a test helper that returns errors for the first N calls,
+// then delegates to a real server.
+type mockTransport struct {
+	failCount    int32 // number of failures remaining (atomic)
+	failErr      error // error to return on failure
+	realServer   *httptest.Server
+	attemptsMade atomic.Int32 // tracks total attempts
+}
+
+func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	m.attemptsMade.Add(1)
+	remaining := atomic.AddInt32(&m.failCount, -1)
+	if remaining >= 0 {
+		// Still have failures to return
+		return nil, m.failErr
+	}
+	// Redirect to real server
+	req.URL.Host = m.realServer.Listener.Addr().String()
+	req.URL.Scheme = "http"
+	return http.DefaultTransport.RoundTrip(req)
+}
+
+func TestDoGet_RetriesOnTemporaryNetError(t *testing.T) {
+	// Real server that will handle successful requests
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"status":"ok"}`))
+	}))
+	defer server.Close()
+
+	// Mock transport: fail twice with ECONNREFUSED, then succeed
+	mt := &mockTransport{
+		failCount:  2,
+		failErr:    &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED},
+		realServer: server,
+	}
+
+	client := NewClient("http://fake-host/", "test-token")
+	client.SetHTTPClient(&http.Client{Transport: mt})
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	body, err := client.doGet(context.Background(), "http://fake-host/test")
+	if err != nil {
+		t.Fatalf("expected success after retries, got error: %v", err)
+	}
+	if string(body) != `{"status":"ok"}` {
+		t.Errorf("body = %q, want %q", string(body), `{"status":"ok"}`)
+	}
+
+	// Should have made exactly 3 attempts: 2 failures + 1 success
+	if got := mt.attemptsMade.Load(); got != 3 {
+		t.Errorf("attempts = %d, want 3 (2 failures + 1 success)", got)
+	}
+}
+
+func TestIsTemporaryNetError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil error", nil, false},
+		{"plain error", fmt.Errorf("some error"), false},
+		// OpError with retriable syscall errors
+		{"OpError ECONNREFUSED", &net.OpError{Op: "dial", Err: syscall.ECONNREFUSED}, true},
+		{"OpError ECONNRESET", &net.OpError{Op: "read", Err: syscall.ECONNRESET}, true},
+		{"OpError ENETUNREACH", &net.OpError{Op: "dial", Err: syscall.ENETUNREACH}, true},
+		{"OpError EHOSTUNREACH", &net.OpError{Op: "dial", Err: syscall.EHOSTUNREACH}, true},
+		{"OpError ETIMEDOUT", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}, true},
+		// OpError with permanent syscall errors — should NOT retry
+		{"OpError EACCES", &net.OpError{Op: "dial", Err: syscall.EACCES}, false},
+		{"OpError EPERM", &net.OpError{Op: "dial", Err: syscall.EPERM}, false},
+		// OpError with unknown inner error — conservative retry
+		{"OpError unknown inner", &net.OpError{Op: "dial", Err: fmt.Errorf("unknown")}, true},
+		// DNS errors
+		{"DNS timeout", &net.DNSError{IsTimeout: true}, true},
+		{"DNS no such host", &net.DNSError{IsTimeout: false, Name: "bad.host"}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isTemporaryNetError(tt.err)
+			if got != tt.want {
+				t.Errorf("isTemporaryNetError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestIsRetriableSyscallError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil", nil, false},
+		{"ECONNREFUSED", syscall.ECONNREFUSED, true},
+		{"ECONNRESET", syscall.ECONNRESET, true},
+		{"ENETUNREACH", syscall.ENETUNREACH, true},
+		{"EHOSTUNREACH", syscall.EHOSTUNREACH, true},
+		{"ETIMEDOUT", syscall.ETIMEDOUT, true},
+		{"EACCES (permanent)", syscall.EACCES, false},
+		{"EPERM (permanent)", syscall.EPERM, false},
+		{"ENOENT (permanent)", syscall.ENOENT, false},
+		{"unknown error", fmt.Errorf("something"), true}, // conservative retry
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isRetriableSyscallError(tt.err)
+			if got != tt.want {
+				t.Errorf("isRetriableSyscallError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestRedactURL(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{
+			name:  "no query params",
+			input: "https://gitea.example.com/api/v1/repos/owner/repo/pulls/1",
+			want:  "https://gitea.example.com/api/v1/repos/owner/repo/pulls/1",
+		},
+		{
+			name:  "with query params - redacts",
+			input: "https://gitea.example.com/api/v1/repos/owner/repo/raw/file?ref=main",
+			want:  "https://gitea.example.com/api/v1/repos/owner/repo/raw/file?[redacted]",
+		},
+		{
+			name:  "multiple query params",
+			input: "https://example.com/path?token=secret&page=1",
+			want:  "https://example.com/path?[redacted]",
+		},
+		{
+			name:  "invalid URL",
+			input: "://invalid",
+			want:  "[invalid URL]",
+		},
+		{
+			name:  "empty string",
+			input: "",
+			want:  "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := redactURL(tt.input)
+			if got != tt.want {
+				t.Errorf("redactURL(%q) = %q, want %q", tt.input, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestSanitizeErrorForLog(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{
+			name: "nil error",
+			err:  nil,
+			want: "<nil>",
+		},
+		{
+			name: "APIError omits body",
+			err:  &APIError{StatusCode: 500, Body: "internal error: database connection failed"},
+			want: "HTTP 500",
+		},
+		{
+			name: "APIError with large body still only shows status",
+			err:  &APIError{StatusCode: 502, Body: strings.Repeat("x", 1000)},
+			want: "HTTP 502",
+		},
+		{
+			name: "non-API error preserved",
+			err:  fmt.Errorf("connection refused"),
+			want: "connection refused",
+		},
+		{
+			name: "wrapped APIError",
+			err:  fmt.Errorf("request failed: %w", &APIError{StatusCode: 503, Body: "service unavailable"}),
+			want: "HTTP 503",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := sanitizeErrorForLog(tt.err)
+			if got != tt.want {
+				t.Errorf("sanitizeErrorForLog() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}

review/formatter.go

@@ -2,7 +2,6 @@ package review
 
 import (
 	"fmt"
-	"regexp"
 	"strings"
 )
 
@@ -23,29 +22,10 @@ func GiteaEvent(verdict string) string {
 	}
 }
 
-// markdownSpecialChars matches characters that have special meaning in Markdown.
-// We escape these to prevent untrusted input from breaking formatting.
-// Uses a quoted string since raw strings can't contain backticks.
-var markdownSpecialChars = regexp.MustCompile("([\\\\*_`\\[\\]()#<>|~])")
-
-// sanitizeMarkdownText escapes special Markdown characters in untrusted text.
-// This prevents markdown injection attacks where a malicious display name could
-// break formatting, inject links, or create unexpected rendering.
-func sanitizeMarkdownText(s string) string {
-	// First, remove any control characters and null bytes
-	cleaned := strings.Map(func(r rune) rune {
-		if r < 32 && r != '\t' && r != '\n' {
-			return -1 // drop the character
-		}
-		return r
-	}, s)
-	// Escape special Markdown characters by prepending backslash
-	return markdownSpecialChars.ReplaceAllString(cleaned, `\$1`)
-}
-
 // FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
-// displayName is sanitized to prevent Markdown injection from untrusted remote persona metadata.
-// sentinelName is used for the cleanup sentinel comment (machine-readable, not rendered).
+// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
+// Persona display names are controlled by repo owners (trusted input).
+// displayName is used for the header title, sentinelName is used for the cleanup sentinel.
 // If displayName is empty, sentinelName is used for both.
 func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string {
 	var sb strings.Builder
@@ -57,8 +37,7 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	}
 
 	if headerName != "" {
-		// Sanitize the header name to prevent Markdown injection
-		title := CapitalizeFirst(sanitizeMarkdownText(headerName))
+		title := CapitalizeFirst(headerName)
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 
@@ -82,8 +61,7 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
 
 	if sentinelName != "" {
-		// Sanitize headerName for the footer as well
-		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", sanitizeMarkdownText(headerName)))
+		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", headerName))
 		// Hidden sentinel for identifying this bot's reviews during cleanup
 		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", sentinelName))
 	}

review/formatter_test.go

@@ -214,71 +214,3 @@ func TestFormatMarkdownWithDisplay(t *testing.T) {
 		}
 	})
 }
-
-func TestSanitizeMarkdownText(t *testing.T) {
-	tests := []struct {
-		name  string
-		input string
-		want  string
-	}{
-		{
-			name:  "plain text unchanged",
-			input: "Security Specialist",
-			want:  "Security Specialist",
-		},
-		{
-			name:  "escapes asterisks",
-			input: "**bold** attack",
-			want:  `\*\*bold\*\* attack`,
-		},
-		{
-			name:  "escapes brackets for links",
-			input: "[click me](http://evil.com)",
-			want:  `\[click me\]\(http://evil.com\)`,
-		},
-		{
-			name:  "escapes backticks",
-			input: "`code` injection",
-			want:  "\\`code\\` injection",
-		},
-		{
-			name:  "escapes angle brackets",
-			input: "<script>alert(1)</script>",
-			want:  `\<script\>alert\(1\)\</script\>`,
-		},
-		{
-			name:  "escapes hash for headers",
-			input: "# Fake Header",
-			want:  `\# Fake Header`,
-		},
-		{
-			name:  "escapes pipe for tables",
-			input: "col1 | col2",
-			want:  `col1 \| col2`,
-		},
-		{
-			name:  "removes control characters",
-			input: "hello\x00world\x1f",
-			want:  "helloworld",
-		},
-		{
-			name:  "preserves tabs and newlines",
-			input: "line1\n\tindented",
-			want:  "line1\n\tindented",
-		},
-		{
-			name:  "escapes tilde for strikethrough",
-			input: "~~strikethrough~~",
-			want:  `\~\~strikethrough\~\~`,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := sanitizeMarkdownText(tt.input)
-			if got != tt.want {
-				t.Errorf("sanitizeMarkdownText(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}

review/persona.go

@@ -224,6 +224,13 @@ func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*ya
 	return nil
 }
 
+// ParsePersonaBytes parses persona data from bytes with a source label for errors.
+// This is useful for parsing personas fetched from external sources (e.g., Gitea API)
+// without requiring filesystem access. Format is detected by source extension.
+func ParsePersonaBytes(data []byte, source string) (*Persona, error) {
+	return parsePersona(data, source)
+}
+
 func validatePersona(p *Persona, source string) error {
 	if p.Name == "" {
 		return fmt.Errorf("persona %s: name is required", source)

review/remote_persona.go

@@ -1,171 +0,0 @@
-package review
-
-import (
-	"context"
-	"fmt"
-	"log/slog"
-	"sort"
-	"strings"
-)
-
-// PersonaFetcher abstracts fetching files from a remote repository.
-// This allows persona loading to work with any Git host API.
-type PersonaFetcher interface {
-	// ListContents returns file/directory entries at a path.
-	// Returns an error if the path doesn't exist or isn't accessible.
-	ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
-
-	// GetFileContent returns the raw content of a file from the default branch.
-	GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
-}
-
-// ContentEntry represents a file or directory entry.
-type ContentEntry struct {
-	Name string // filename or directory name
-	Path string // full path from repo root
-	Type string // "file" or "dir"
-}
-
-// DefaultPersonasPath is the conventional location for repo-specific personas.
-const DefaultPersonasPath = ".review-bot/personas"
-
-// LoadRemotePersonas fetches personas from a remote repository's .review-bot/personas/ directory.
-// Returns a map of persona name to Persona. If the directory doesn't exist or is empty,
-// returns an empty map with no error (graceful fallback to built-in personas).
-//
-// Files larger than MaxPersonaFileSize are logged and skipped.
-// Invalid YAML files are logged and skipped (partial success model).
-// Only .yaml and .yml files are processed; other files are ignored.
-func LoadRemotePersonas(ctx context.Context, fetcher PersonaFetcher, owner, repo string) (map[string]*Persona, error) {
-	return LoadRemotePersonasFromPath(ctx, fetcher, owner, repo, DefaultPersonasPath)
-}
-
-// LoadRemotePersonasFromPath loads personas from a custom path in a remote repository.
-// It behaves the same as LoadRemotePersonas but allows specifying a path other than
-// the default .review-bot/personas directory.
-func LoadRemotePersonasFromPath(ctx context.Context, fetcher PersonaFetcher, owner, repo, path string) (map[string]*Persona, error) {
-	entries, err := fetcher.ListContents(ctx, owner, repo, path)
-	if err != nil {
-		// 404 is expected when repo doesn't have personas — return empty, not error
-		if isNotFoundError(err) {
-			slog.Debug("no remote personas directory found", "repo", fmt.Sprintf("%s/%s", owner, repo), "path", path)
-			return map[string]*Persona{}, nil
-		}
-		return nil, fmt.Errorf("list remote personas: %w", err)
-	}
-
-	// Cap the number of files to process to prevent resource exhaustion
-	// from repos with thousands of small files.
-	const maxPersonaFiles = 50
-
-	result := make(map[string]*Persona)
-	processed := 0
-	for _, entry := range entries {
-		if processed >= maxPersonaFiles {
-			slog.Warn("persona file limit reached", "limit", maxPersonaFiles, "repo", fmt.Sprintf("%s/%s", owner, repo))
-			break
-		}
-		if ctx.Err() != nil {
-			return nil, ctx.Err()
-		}
-
-		// Skip directories and non-YAML files
-		if entry.Type != "file" {
-			continue
-		}
-		if !isYAMLFile(entry.Name) {
-			continue
-		}
-
-		content, err := fetcher.GetFileContent(ctx, owner, repo, entry.Path)
-		if err != nil {
-			slog.Warn("could not fetch remote persona file", "file", entry.Path, "error", err)
-			continue
-		}
-
-		// Check size before parsing (defense in depth)
-		if len(content) > MaxPersonaFileSize {
-			slog.Warn("remote persona file exceeds size limit", "file", entry.Path, "size", len(content), "limit", MaxPersonaFileSize)
-			continue
-		}
-
-		// YAML parsing uses parsePersona which has defenses against YAML DoS attacks:
-		// - MaxPersonaFileSize (above) caps raw input size before any parsing
-		// - maxPersonaFiles (above) limits the number of files processed per repo
-		// - unmarshalYAMLWithDepthLimit enforces MaxYAMLDepth to prevent stack exhaustion
-		// - checkYAMLDepth tracks node counts (MaxYAMLNodes) against "billion laughs" expansion
-		// - Alias cycles are detected and capped by seen-node tracking
-		// See persona.go for the implementation details.
-		persona, err := parsePersona([]byte(content), entry.Path)
-		if err != nil {
-			slog.Warn("could not parse remote persona file", "file", entry.Path, "error", err)
-			continue
-		}
-
-		result[persona.Name] = persona
-		processed++
-		slog.Debug("loaded remote persona", "name", persona.Name, "file", entry.Path)
-	}
-
-	return result, nil
-}
-
-// MergePersonas combines remote and built-in personas.
-// Remote personas take precedence on name collision.
-// Returns the merged map and a list of persona names in sorted order.
-func MergePersonas(remote, builtin map[string]*Persona) (map[string]*Persona, []string) {
-	merged := make(map[string]*Persona)
-
-	// Add built-in first
-	for name, p := range builtin {
-		merged[name] = p
-	}
-
-	// Remote overrides built-in on collision
-	for name, p := range remote {
-		if _, exists := merged[name]; exists {
-			slog.Debug("remote persona overrides built-in", "name", name)
-		}
-		merged[name] = p
-	}
-
-	// Collect sorted names
-	names := make([]string, 0, len(merged))
-	for name := range merged {
-		names = append(names, name)
-	}
-	sort.Strings(names)
-
-	return merged, names
-}
-
-// LoadAllBuiltinPersonas loads all built-in personas into a map.
-func LoadAllBuiltinPersonas() map[string]*Persona {
-	result := make(map[string]*Persona)
-	for _, name := range ListBuiltinPersonas() {
-		p, err := LoadBuiltinPersona(name)
-		if err != nil {
-			slog.Warn("could not load built-in persona", "name", name, "error", err)
-			continue
-		}
-		result[name] = p
-	}
-	return result
-}
-
-// isYAMLFile returns true if the filename has a YAML extension.
-func isYAMLFile(name string) bool {
-	lower := strings.ToLower(name)
-	return strings.HasSuffix(lower, ".yaml") || strings.HasSuffix(lower, ".yml")
-}
-
-// isNotFoundError checks if an error indicates a 404 response.
-// This is a simple string check to avoid importing the gitea package
-// (which would create a circular dependency).
-func isNotFoundError(err error) bool {
-	if err == nil {
-		return false
-	}
-	errStr := err.Error()
-	return strings.Contains(errStr, "HTTP 404")
-}

review/remote_persona_test.go

@@ -1,394 +0,0 @@
-package review
-
-import (
-	"context"
-	"errors"
-	"testing"
-)
-
-// mockFetcher implements PersonaFetcher for testing.
-type mockFetcher struct {
-	contents     map[string][]ContentEntry // path -> entries
-	files        map[string]string         // path -> content
-	listErr      error                     // error to return from ListContents
-	getFileErr   map[string]error          // path -> error for GetFileContent
-	listNotFound bool                      // return 404-style error
-}
-
-func newMockFetcher() *mockFetcher {
-	return &mockFetcher{
-		contents:   make(map[string][]ContentEntry),
-		files:      make(map[string]string),
-		getFileErr: make(map[string]error),
-	}
-}
-
-func (m *mockFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
-	if m.listNotFound {
-		return nil, errors.New("HTTP 404: not found")
-	}
-	if m.listErr != nil {
-		return nil, m.listErr
-	}
-	entries, ok := m.contents[path]
-	if !ok {
-		return nil, errors.New("HTTP 404: not found")
-	}
-	return entries, nil
-}
-
-func (m *mockFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	if err, ok := m.getFileErr[filepath]; ok {
-		return "", err
-	}
-	content, ok := m.files[filepath]
-	if !ok {
-		return "", errors.New("HTTP 404: file not found")
-	}
-	return content, nil
-}
-
-func TestLoadRemotePersonas_NoDirectory(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.listNotFound = true
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("expected no error for missing directory, got: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected empty map, got %d personas", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_EmptyDirectory(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{}
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected empty map, got %d personas", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SinglePersona(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/trading.yaml"] = `
-name: trading
-display_name: Trading Expert
-identity: You are a trading systems expert.
-focus:
-  - order execution
-  - market data
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(result))
-	}
-	if result["trading"] == nil {
-		t.Fatal("expected 'trading' persona")
-	}
-	if result["trading"].DisplayName != "Trading Expert" {
-		t.Errorf("expected display name 'Trading Expert', got %q", result["trading"].DisplayName)
-	}
-}
-
-func TestLoadRemotePersonas_MultiplePersonas(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "one.yaml", Path: ".review-bot/personas/one.yaml", Type: "file"},
-		{Name: "two.yml", Path: ".review-bot/personas/two.yml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/one.yaml"] = `
-name: one
-identity: First persona.
-`
-	fetcher.files[".review-bot/personas/two.yml"] = `
-name: two
-identity: Second persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 2 {
-		t.Fatalf("expected 2 personas, got %d", len(result))
-	}
-	if result["one"] == nil || result["two"] == nil {
-		t.Error("expected both personas to be loaded")
-	}
-}
-
-func TestLoadRemotePersonas_SkipsNonYAML(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "readme.md", Path: ".review-bot/personas/readme.md", Type: "file"},
-		{Name: "config.json", Path: ".review-bot/personas/config.json", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping non-YAML), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsDirectories(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping dir), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsInvalidYAML(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-	fetcher.files[".review-bot/personas/invalid.yaml"] = `
-this is not valid yaml: [unclosed bracket
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping invalid), got %d", len(result))
-	}
-	if result["valid"] == nil {
-		t.Error("expected valid persona to be loaded")
-	}
-}
-
-func TestLoadRemotePersonas_SkipsOversizedFiles(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
-	}
-	// Create content larger than MaxPersonaFileSize (64KB)
-	fetcher.files[".review-bot/personas/huge.yaml"] = `
-name: huge
-identity: ` + string(make([]byte, MaxPersonaFileSize+1000))
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected 0 personas (oversized file skipped), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsFetchErrors(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "error.yaml", Path: ".review-bot/personas/error.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-	fetcher.getFileErr[".review-bot/personas/error.yaml"] = errors.New("network error")
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping error), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_ListContentsError(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.listErr = errors.New("server error")
-
-	_, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err == nil {
-		t.Fatal("expected error for list contents failure")
-	}
-}
-
-func TestLoadRemotePersonas_ContextCancellation(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel() // Cancel immediately
-
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "one.yaml", Path: ".review-bot/personas/one.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/one.yaml"] = `
-name: one
-identity: One.
-`
-
-	_, err := LoadRemotePersonas(ctx, fetcher, "owner", "repo")
-	if err == nil {
-		t.Fatal("expected context cancellation error")
-	}
-}
-
-func TestMergePersonas_NoOverlap(t *testing.T) {
-	remote := map[string]*Persona{
-		"trading": {Name: "trading", Identity: "Trading expert."},
-	}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Security expert."},
-	}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 2 {
-		t.Fatalf("expected 2 personas, got %d", len(merged))
-	}
-	if len(names) != 2 {
-		t.Fatalf("expected 2 names, got %d", len(names))
-	}
-	// Names should be sorted
-	if names[0] != "security" || names[1] != "trading" {
-		t.Errorf("expected sorted names [security, trading], got %v", names)
-	}
-}
-
-func TestMergePersonas_RemoteOverridesBuiltin(t *testing.T) {
-	remote := map[string]*Persona{
-		"security": {Name: "security", Identity: "Custom security expert."},
-	}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Default security expert."},
-	}
-
-	merged, _ := MergePersonas(remote, builtin)
-
-	if merged["security"].Identity != "Custom security expert." {
-		t.Errorf("expected remote to override builtin, got identity: %q", merged["security"].Identity)
-	}
-}
-
-func TestMergePersonas_EmptyRemote(t *testing.T) {
-	remote := map[string]*Persona{}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Security."},
-	}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(merged))
-	}
-	if names[0] != "security" {
-		t.Errorf("expected 'security', got %q", names[0])
-	}
-}
-
-func TestMergePersonas_EmptyBuiltin(t *testing.T) {
-	remote := map[string]*Persona{
-		"trading": {Name: "trading", Identity: "Trading."},
-	}
-	builtin := map[string]*Persona{}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(merged))
-	}
-	if names[0] != "trading" {
-		t.Errorf("expected 'trading', got %q", names[0])
-	}
-}
-
-func TestLoadAllBuiltinPersonas(t *testing.T) {
-	personas := LoadAllBuiltinPersonas()
-
-	// Should load at least the known built-in personas
-	expected := []string{"architect", "docs", "security"}
-	for _, name := range expected {
-		if personas[name] == nil {
-			t.Errorf("expected built-in persona %q to be loaded", name)
-		}
-	}
-}
-
-func TestIsYAMLFile(t *testing.T) {
-	tests := []struct {
-		name     string
-		expected bool
-	}{
-		{"test.yaml", true},
-		{"test.yml", true},
-		{"test.YAML", true},
-		{"test.YML", true},
-		{"test.json", false},
-		{"test.md", false},
-		{"yaml", false},
-		{"", false},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			if got := isYAMLFile(tc.name); got != tc.expected {
-				t.Errorf("isYAMLFile(%q) = %v, want %v", tc.name, got, tc.expected)
-			}
-		})
-	}
-}
-
-func TestIsNotFoundError(t *testing.T) {
-	tests := []struct {
-		name     string
-		err      error
-		expected bool
-	}{
-		{"nil error", nil, false},
-		{"HTTP 404", errors.New("HTTP 404: not found"), true},
-		{"not found text", errors.New("path not found"), false},
-		{"server error", errors.New("server error"), false},
-		{"HTTP 500", errors.New("HTTP 500: internal error"), false},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			if got := isNotFoundError(tc.err); got != tc.expected {
-				t.Errorf("isNotFoundError(%v) = %v, want %v", tc.err, got, tc.expected)
-			}
-		})
-	}
-}

review/repo_persona.go

@@ -0,0 +1,150 @@
+package review
+
+import (
+	"context"
+	"log/slog"
+	"strings"
+)
+
+// RepoPersonaPath is the directory path where repo-specific personas are stored.
+const RepoPersonaPath = ".review-bot/personas"
+
+// GiteaClient defines the subset of gitea.Client methods needed for loading repo personas.
+// This interface allows for easier testing and decouples the review package from gitea.
+type GiteaClient interface {
+	ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
+	GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
+}
+
+// ContentEntry represents a file or directory entry from the contents API.
+// This mirrors gitea.ContentEntry to avoid import cycles.
+type ContentEntry struct {
+	Name string `json:"name"`
+	Path string `json:"path"`
+	Type string `json:"type"` // "file" or "dir"
+}
+
+// LoadRepoPersonas fetches personas from a repository's .review-bot/personas/ directory.
+// Returns an empty map (not nil) if the directory doesn't exist or is empty.
+// Individual parse failures are logged and skipped; the remaining personas are still returned.
+// Auth errors and other non-404 errors are propagated.
+// Files exceeding MaxPersonaFileSize are rejected to prevent resource exhaustion.
+func LoadRepoPersonas(ctx context.Context, client GiteaClient, owner, repo string) (map[string]*Persona, error) {
+	result := make(map[string]*Persona)
+
+	entries, err := client.ListContents(ctx, owner, repo, RepoPersonaPath)
+	if err != nil {
+		// Check if this is a 404 (directory doesn't exist) - expected case
+		if isNotFoundError(err) {
+			slog.Debug("no repo personas directory found", "repo", owner+"/"+repo)
+			return result, nil
+		}
+		// Other errors (auth, server) should propagate
+		return nil, err
+	}
+
+	if len(entries) == 0 {
+		slog.Debug("repo personas directory is empty", "repo", owner+"/"+repo)
+		return result, nil
+	}
+
+	for _, entry := range entries {
+		if entry.Type != "file" {
+			continue
+		}
+		// Only process YAML files
+		if !isYAMLFile(entry.Name) {
+			continue
+		}
+
+		content, err := client.GetFileContent(ctx, owner, repo, entry.Path)
+		if err != nil {
+			slog.Warn("could not fetch repo persona file",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"error", err)
+			continue
+		}
+
+		// Enforce size limit before parsing to prevent resource exhaustion
+		if len(content) > MaxPersonaFileSize {
+			slog.Warn("repo persona file exceeds maximum size",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"size", len(content),
+				"max", MaxPersonaFileSize)
+			continue
+		}
+
+		persona, err := ParsePersonaBytes([]byte(content), entry.Path)
+		if err != nil {
+			slog.Warn("could not parse repo persona file",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"error", err)
+			continue
+		}
+
+		result[persona.Name] = persona
+		slog.Debug("loaded repo persona",
+			"name", persona.Name,
+			"file", entry.Path,
+			"repo", owner+"/"+repo)
+	}
+
+	return result, nil
+}
+
+// MergePersonas combines built-in personas with repo personas.
+// Repo personas take precedence on name collision.
+// Returns a new map; inputs are not modified.
+func MergePersonas(builtin, repo map[string]*Persona) map[string]*Persona {
+	result := make(map[string]*Persona, len(builtin)+len(repo))
+
+	// Copy built-in personas first
+	for name, p := range builtin {
+		result[name] = p
+	}
+
+	// Overlay repo personas (override on collision)
+	for name, p := range repo {
+		if _, exists := result[name]; exists {
+			slog.Debug("repo persona overrides built-in", "name", name)
+		}
+		result[name] = p
+	}
+
+	return result
+}
+
+// GetBuiltinPersonasMap returns all built-in personas as a map keyed by name.
+// Returns an empty map (not nil) if loading fails.
+func GetBuiltinPersonasMap() map[string]*Persona {
+	result := make(map[string]*Persona)
+	for _, name := range ListBuiltinPersonas() {
+		p, err := LoadBuiltinPersona(name)
+		if err != nil {
+			slog.Warn("could not load built-in persona", "name", name, "error", err)
+			continue
+		}
+		result[name] = p
+	}
+	return result
+}
+
+// isYAMLFile checks if a filename has a YAML extension.
+func isYAMLFile(name string) bool {
+	lower := strings.ToLower(name)
+	return strings.HasSuffix(lower, ".yaml") || strings.HasSuffix(lower, ".yml")
+}
+
+// isNotFoundError checks if an error represents a 404 response.
+// This uses a specific "HTTP 404" substring match rather than a generic "not found"
+// match to avoid masking authentication failures or transport errors that might
+// contain "not found" in their message.
+func isNotFoundError(err error) bool {
+	if err == nil {
+		return false
+	}
+	return strings.Contains(err.Error(), "HTTP 404")
+}

review/repo_persona_test.go

@@ -0,0 +1,443 @@
+package review
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+)
+
+func TestParsePersonaBytes(t *testing.T) {
+	tests := []struct {
+		name       string
+		data       string
+		source     string
+		wantName   string
+		wantErr    string
+	}{
+		{
+			name: "valid yaml",
+			data: `name: test
+identity: test identity
+focus:
+  - testing
+`,
+			source:   "test.yaml",
+			wantName: "test",
+		},
+		{
+			name:    "missing name",
+			data:    "identity: test\n",
+			source:  "test.yaml",
+			wantErr: "name is required",
+		},
+		{
+			name:    "invalid yaml",
+			data:    "not: valid:\n  yaml: [broken",
+			source:  "test.yaml",
+			wantErr: "parse",
+		},
+		{
+			name: "json format by extension",
+			data: `{"name": "jsontest", "identity": "json identity"}`,
+			source:   "test.json",
+			wantName: "jsontest",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p, err := ParsePersonaBytes([]byte(tt.data), tt.source)
+			if tt.wantErr != "" {
+				if err == nil {
+					t.Fatalf("expected error containing %q, got nil", tt.wantErr)
+				}
+				if !strings.Contains(err.Error(), tt.wantErr) {
+					t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if p.Name != tt.wantName {
+				t.Errorf("Name = %q, want %q", p.Name, tt.wantName)
+			}
+		})
+	}
+}
+
+// mockGiteaClient implements GiteaClient for testing.
+type mockGiteaClient struct {
+	contents map[string][]ContentEntry // path -> entries
+	files    map[string]string         // path -> content
+	listErr  error
+	fileErr  map[string]error // path -> error
+}
+
+func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
+	if m.listErr != nil {
+		return nil, m.listErr
+	}
+	entries, ok := m.contents[path]
+	if !ok {
+		return nil, errors.New("list contents .review-bot/personas: HTTP 404: not found")
+	}
+	return entries, nil
+}
+
+func (m *mockGiteaClient) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	if m.fileErr != nil {
+		if err, ok := m.fileErr[filepath]; ok {
+			return "", err
+		}
+	}
+	content, ok := m.files[filepath]
+	if !ok {
+		return "", errors.New("HTTP 404: file not found")
+	}
+	return content, nil
+}
+
+func TestLoadRepoPersonas(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("directory not found returns empty map", func(t *testing.T) {
+		client := &mockGiteaClient{} // No contents configured -> 404
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if personas == nil {
+			t.Error("expected empty map, got nil")
+		}
+		if len(personas) != 0 {
+			t.Errorf("expected 0 personas, got %d", len(personas))
+		}
+	})
+
+	t.Run("empty directory returns empty map", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {},
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 0 {
+			t.Errorf("expected 0 personas, got %d", len(personas))
+		}
+	})
+
+	t.Run("loads valid personas", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
+					{Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/trading.yaml": `name: trading
+display_name: Trading Expert
+identity: You are a trading expert.
+focus:
+  - order handling
+  - risk management
+`,
+				".review-bot/personas/crypto.yaml": `name: crypto
+display_name: Crypto Expert
+identity: You are a cryptography expert.
+focus:
+  - key management
+  - encryption
+`,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 2 {
+			t.Fatalf("expected 2 personas, got %d", len(personas))
+		}
+		if personas["trading"] == nil {
+			t.Error("expected trading persona")
+		}
+		if personas["crypto"] == nil {
+			t.Error("expected crypto persona")
+		}
+		if personas["trading"].DisplayName != "Trading Expert" {
+			t.Errorf("trading display name = %q, want %q", personas["trading"].DisplayName, "Trading Expert")
+		}
+	})
+
+	t.Run("skips invalid persona files", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
+					{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/valid.yaml": `name: valid
+identity: Valid persona
+`,
+				".review-bot/personas/invalid.yaml": "not valid yaml: [broken",
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		// Should have the valid one, skip the invalid
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped invalid), got %d", len(personas))
+		}
+		if personas["valid"] == nil {
+			t.Error("expected valid persona")
+		}
+	})
+
+	t.Run("skips non-yaml files", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
+					{Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"},
+					{Name: "notes.txt", Path: ".review-bot/personas/notes.txt", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/persona.yaml": `name: test
+identity: Test persona
+`,
+				".review-bot/personas/README.md": "# Personas\n\nPut your personas here.",
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (yaml only), got %d", len(personas))
+		}
+	})
+
+	t.Run("skips subdirectories", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
+					{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/persona.yaml": `name: test
+identity: Test persona
+`,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (files only), got %d", len(personas))
+		}
+	})
+
+	t.Run("propagates auth errors", func(t *testing.T) {
+		client := &mockGiteaClient{
+			listErr: errors.New("HTTP 401: unauthorized"),
+		}
+		_, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err == nil {
+			t.Fatal("expected error for auth failure")
+		}
+		if !strings.Contains(err.Error(), "401") {
+			t.Errorf("error = %q, want containing '401'", err.Error())
+		}
+	})
+
+	t.Run("skips files that fail to fetch", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"},
+					{Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/good.yaml": `name: good
+identity: Good persona
+`,
+			},
+			fileErr: map[string]error{
+				".review-bot/personas/bad.yaml": errors.New("HTTP 500: internal server error"),
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped failed fetch), got %d", len(personas))
+		}
+	})
+
+	t.Run("skips oversized files", func(t *testing.T) {
+		// Create a content string that exceeds MaxPersonaFileSize (64KB)
+		oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1)
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"},
+					{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/normal.yaml": `name: normal
+identity: Normal sized persona
+`,
+				".review-bot/personas/huge.yaml": oversizedContent,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		// Should have the normal one, skip the oversized
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped oversized), got %d", len(personas))
+		}
+		if personas["normal"] == nil {
+			t.Error("expected normal persona")
+		}
+	})
+}
+
+func TestMergePersonas(t *testing.T) {
+	builtin := map[string]*Persona{
+		"security": {Name: "security", Identity: "Built-in security"},
+		"docs":     {Name: "docs", Identity: "Built-in docs"},
+	}
+	repo := map[string]*Persona{
+		"security": {Name: "security", Identity: "Repo security override"},
+		"trading":  {Name: "trading", Identity: "Repo trading"},
+	}
+
+	merged := MergePersonas(builtin, repo)
+
+	t.Run("repo overrides builtin on collision", func(t *testing.T) {
+		if merged["security"].Identity != "Repo security override" {
+			t.Errorf("security identity = %q, want repo override", merged["security"].Identity)
+		}
+	})
+
+	t.Run("builtin preserved when no collision", func(t *testing.T) {
+		if merged["docs"].Identity != "Built-in docs" {
+			t.Errorf("docs identity = %q, want built-in", merged["docs"].Identity)
+		}
+	})
+
+	t.Run("repo-only persona added", func(t *testing.T) {
+		if merged["trading"] == nil {
+			t.Error("expected trading persona from repo")
+		}
+		if merged["trading"].Identity != "Repo trading" {
+			t.Errorf("trading identity = %q, want repo", merged["trading"].Identity)
+		}
+	})
+
+	t.Run("original maps not modified", func(t *testing.T) {
+		if builtin["trading"] != nil {
+			t.Error("builtin map was modified")
+		}
+		if len(repo) != 2 {
+			t.Error("repo map was modified")
+		}
+	})
+}
+
+func TestGetBuiltinPersonasMap(t *testing.T) {
+	personas := GetBuiltinPersonasMap()
+
+	if len(personas) == 0 {
+		t.Fatal("expected at least one built-in persona")
+	}
+
+	// Verify expected personas exist
+	expected := []string{"security", "architect", "docs"}
+	for _, name := range expected {
+		if personas[name] == nil {
+			t.Errorf("expected built-in persona %q", name)
+		}
+	}
+
+	// Verify personas are valid
+	for name, p := range personas {
+		if p.Name != name {
+			t.Errorf("persona %q has mismatched name %q", name, p.Name)
+		}
+		if p.Identity == "" {
+			t.Errorf("persona %q has empty identity", name)
+		}
+	}
+}
+
+func TestIsYAMLFile(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		{"test.yaml", true},
+		{"test.yml", true},
+		{"test.YAML", true},
+		{"test.YML", true},
+		{"test.json", false},
+		{"test.md", false},
+		{"test.txt", false},
+		{"yaml", false},
+		{"yaml.md", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isYAMLFile(tt.name); got != tt.want {
+				t.Errorf("isYAMLFile(%q) = %v, want %v", tt.name, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestIsNotFoundError(t *testing.T) {
+	tests := []struct {
+		err  error
+		want bool
+	}{
+		{nil, false},
+		{errors.New("HTTP 404: not found"), true},
+		{errors.New("HTTP 404"), true},
+		// Intentionally false: generic "not found" could mask auth/transport errors.
+		// Only explicit HTTP 404 responses should be treated as "directory doesn't exist".
+		{errors.New("something not found"), false},
+		{errors.New("HTTP 401: unauthorized"), false},
+		{errors.New("connection refused"), false},
+	}
+
+	for _, tt := range tests {
+		name := "nil"
+		if tt.err != nil {
+			name = tt.err.Error()
+		}
+		t.Run(name, func(t *testing.T) {
+			if got := isNotFoundError(tt.err); got != tt.want {
+				t.Errorf("isNotFoundError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}