Revert "ci: disable setup-go cache (cache server unreachable)"

This reverts commit 8f564ea4f8.

README.md

@@ -459,41 +459,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)
 	}
 
@@ -802,17 +798,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 +823,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)
 }

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)
+			}
+		})
+	}
+}