feat: load personas from target repo .review-bot/personas/

- Add RepoContentFetcher interface for fetching repo files
- Add LoadRepoPersonas() to load custom personas from repo
- Add LoadPersonaWithFallback() to check repo then built-in
- Add ListAllPersonas() to merge repo + built-in persona names
- Repo personas take precedence over built-ins with same name

Closes #60

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

@@ -79,6 +79,7 @@ func main() {
 	aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
 	aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
 
+	flag.Parse()
 	flag.Parse()
 
 	if *versionFlag {
@@ -115,9 +116,29 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Persona loading is deferred until after giteaClient is initialized,
-	// so we can try loading from the target repo first.
+	// Load persona if specified
 	var persona *review.Persona
+	if *personaName != "" {
+		var err error
+		persona, err = review.LoadBuiltinPersona(*personaName)
+		if err != nil {
+			slog.Error("failed to load persona", "persona", *personaName, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
+	} else if *personaFile != "" {
+		resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
+		if err != nil {
+			slog.Error("invalid persona-file path", "error", err)
+			os.Exit(1)
+		}
+		persona, err = review.LoadPersona(resolvedPath)
+		if err != nil {
+			slog.Error("failed to load persona file", "file", *personaFile, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
+	}
 
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
@@ -175,45 +196,6 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
 	defer cancel()
 
-	// Load persona: try remote repo first, then fall back to built-in
-	if *personaName != "" {
-		// Try loading from target repo's .review-bot/personas/ directory
-		fetcher := &giteaFetcher{client: giteaClient}
-		remotePersonas, err := review.LoadRemotePersonas(ctx, fetcher, 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{}
-		}
-
-		if p, ok := remotePersonas[*personaName]; ok {
-			persona = p
-			slog.Info("loaded persona from target repo", "persona", persona.Name, "display", persona.DisplayName)
-		} else {
-			// Fall back to built-in persona
-			var err error
-			persona, err = review.LoadBuiltinPersona(*personaName)
-			if err != nil {
-				slog.Error("failed to load persona", "persona", *personaName, "error", err)
-				os.Exit(1)
-			}
-			slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
-		}
-	} else if *personaFile != "" {
-		resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
-		if err != nil {
-			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)
-			os.Exit(1)
-		}
-		persona = loadedPersona
-		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
-	}
-
 	slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
 
 	// Step 1: Fetch PR metadata
@@ -801,29 +783,3 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
 	}
 	return evaluatedSHA != currentSHA
 }
-
-// giteaFetcher adapts gitea.Client to review.PersonaFetcher interface.
-type giteaFetcher 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)
-	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{
-			Name: e.Name,
-			Path: e.Path,
-			Type: e.Type,
-		}
-	}
-	return result, nil
-}
-
-func (f *giteaFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return f.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

@@ -2,10 +2,12 @@ package review
 
 import (
 	"bytes"
+	"context"
 	"embed"
 	"encoding/json"
 	"fmt"
 	"os"
+	"path/filepath"
 	"sort"
 	"strings"
 	"unicode/utf8"
@@ -28,6 +30,9 @@ const MaxYAMLDepth = 20
 // This prevents DoS via wide-but-shallow structures that bypass depth limits.
 const MaxYAMLNodes = 1000
 
+// RepoPersonasPath is the path within a repository where custom personas are stored.
+const RepoPersonasPath = ".review-bot/personas"
+
 // Persona defines a specialized review role with focused expertise.
 type Persona struct {
 	Name         string   `json:"name" yaml:"name"`
@@ -48,6 +53,20 @@ type Severity struct {
 	Nit   string `json:"nit" yaml:"nit"`
 }
 
+// RepoContentFetcher is an interface for fetching file content from a repository.
+// This allows LoadRepoPersonas to work with any client that can list and fetch files.
+type RepoContentFetcher 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 (mirrors gitea.ContentEntry).
+type ContentEntry struct {
+	Name string `json:"name"`
+	Path string `json:"path"`
+	Type string `json:"type"` // "file" or "dir"
+}
+
 // LoadPersona loads a persona from a JSON or YAML file path.
 // Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON.
 // Files larger than MaxPersonaFileSize are rejected.
@@ -130,6 +149,92 @@ func ListBuiltinPersonas() []string {
 	return names
 }
 
+// LoadRepoPersonas loads custom personas from a repository's .review-bot/personas/ directory.
+// Returns an empty map if the directory doesn't exist or is empty.
+// Repo personas take precedence over built-in personas with the same name.
+func LoadRepoPersonas(ctx context.Context, client RepoContentFetcher, owner, repo string) (map[string]*Persona, error) {
+	personas := make(map[string]*Persona)
+
+	entries, err := client.ListContents(ctx, owner, repo, RepoPersonasPath)
+	if err != nil {
+		// Directory doesn't exist — not an error, just no custom personas
+		return personas, nil
+	}
+
+	for _, entry := range entries {
+		if entry.Type != "file" {
+			continue
+		}
+		// Only load YAML files
+		ext := strings.ToLower(filepath.Ext(entry.Name))
+		if ext != ".yaml" && ext != ".yml" {
+			continue
+		}
+
+		content, err := client.GetFileContent(ctx, owner, repo, entry.Path)
+		if err != nil {
+			// Log but don't fail — one bad persona shouldn't break the whole review
+			continue
+		}
+
+		// Validate file size
+		if len(content) > MaxPersonaFileSize {
+			continue
+		}
+
+		persona, err := parsePersona([]byte(content), "repo:"+entry.Path)
+		if err != nil {
+			// Log but don't fail
+			continue
+		}
+
+		personas[persona.Name] = persona
+	}
+
+	return personas, nil
+}
+
+// LoadPersonaWithFallback loads a persona by name, checking the repo first, then built-ins.
+// This is the primary entry point for loading personas during a review.
+func LoadPersonaWithFallback(ctx context.Context, client RepoContentFetcher, owner, repo, name string) (*Persona, error) {
+	// Try repo personas first
+	repoPersonas, err := LoadRepoPersonas(ctx, client, owner, repo)
+	if err == nil {
+		if p, ok := repoPersonas[name]; ok {
+			return p, nil
+		}
+	}
+
+	// Fall back to built-in
+	return LoadBuiltinPersona(name)
+}
+
+// ListAllPersonas returns a merged list of available personas (repo + built-in).
+// Repo personas take precedence over built-ins with the same name.
+func ListAllPersonas(ctx context.Context, client RepoContentFetcher, owner, repo string) []string {
+	seen := make(map[string]bool)
+
+	// Built-ins first
+	for _, name := range ListBuiltinPersonas() {
+		seen[name] = true
+	}
+
+	// Repo personas override
+	repoPersonas, err := LoadRepoPersonas(ctx, client, owner, repo)
+	if err == nil {
+		for name := range repoPersonas {
+			seen[name] = true
+		}
+	}
+
+	names := make([]string, 0, len(seen))
+	for name := range seen {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return names
+}
+
 // parsePersona parses persona data from JSON or YAML format.
 // Format is detected by the source file extension.
 func parsePersona(data []byte, source string) (*Persona, error) {

review/persona_test.go

@@ -1,6 +1,7 @@
 package review
 
 import (
+	"context"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -776,3 +777,242 @@ identity: test identity
 		t.Errorf("Name = %q, want %q", p.Name, "test")
 	}
 }
+
+// MockRepoFetcher is a mock implementation of RepoContentFetcher for testing.
+type MockRepoFetcher struct {
+	Contents map[string][]ContentEntry // path -> entries
+	Files    map[string]string         // path -> content
+}
+
+func (m *MockRepoFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
+	key := fmt.Sprintf("%s/%s/%s", owner, repo, path)
+	entries, ok := m.Contents[key]
+	if !ok {
+		return nil, fmt.Errorf("path not found: %s", path)
+	}
+	return entries, nil
+}
+
+func (m *MockRepoFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	key := fmt.Sprintf("%s/%s/%s", owner, repo, filepath)
+	content, ok := m.Files[key]
+	if !ok {
+		return "", fmt.Errorf("file not found: %s", filepath)
+	}
+	return content, nil
+}
+
+func TestLoadRepoPersonas(t *testing.T) {
+	validPersona := `name: custom-security
+display_name: Custom Security
+identity: |
+  You are a custom security reviewer.
+focus:
+  - SQL injection
+  - XSS attacks
+ignore:
+  - Code style
+severity:
+  major: Critical vulnerabilities
+  minor: Potential issues
+  nit: Suggestions
+`
+
+	tests := []struct {
+		name      string
+		fetcher   *MockRepoFetcher
+		wantCount int
+		wantNames []string
+	}{
+		{
+			name: "no personas directory",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{},
+				Files:    map[string]string{},
+			},
+			wantCount: 0,
+		},
+		{
+			name: "empty personas directory",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{
+					"owner/repo/.review-bot/personas": {},
+				},
+				Files: map[string]string{},
+			},
+			wantCount: 0,
+		},
+		{
+			name: "one valid persona",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{
+					"owner/repo/.review-bot/personas": {
+						{Name: "custom-security.yaml", Path: ".review-bot/personas/custom-security.yaml", Type: "file"},
+					},
+				},
+				Files: map[string]string{
+					"owner/repo/.review-bot/personas/custom-security.yaml": validPersona,
+				},
+			},
+			wantCount: 1,
+			wantNames: []string{"custom-security"},
+		},
+		{
+			name: "skip non-yaml files",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{
+					"owner/repo/.review-bot/personas": {
+						{Name: "custom-security.yaml", Path: ".review-bot/personas/custom-security.yaml", Type: "file"},
+						{Name: "readme.md", Path: ".review-bot/personas/readme.md", Type: "file"},
+						{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
+					},
+				},
+				Files: map[string]string{
+					"owner/repo/.review-bot/personas/custom-security.yaml": validPersona,
+				},
+			},
+			wantCount: 1,
+			wantNames: []string{"custom-security"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			personas, err := LoadRepoPersonas(ctx, tt.fetcher, "owner", "repo")
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if len(personas) != tt.wantCount {
+				t.Errorf("got %d personas, want %d", len(personas), tt.wantCount)
+			}
+			for _, name := range tt.wantNames {
+				if _, ok := personas[name]; !ok {
+					t.Errorf("missing expected persona %q", name)
+				}
+			}
+		})
+	}
+}
+
+func TestLoadPersonaWithFallback(t *testing.T) {
+	customSecurity := `name: security
+display_name: Custom Security Override
+identity: |
+  Custom security reviewer for this repo.
+focus:
+  - Repo-specific security
+ignore:
+  - General stuff
+severity:
+  major: Critical
+  minor: Warning
+  nit: Info
+`
+
+	tests := []struct {
+		name            string
+		fetcher         *MockRepoFetcher
+		personaName     string
+		wantDisplayName string
+		wantErr         bool
+	}{
+		{
+			name: "repo persona overrides builtin",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{
+					"owner/repo/.review-bot/personas": {
+						{Name: "security.yaml", Path: ".review-bot/personas/security.yaml", Type: "file"},
+					},
+				},
+				Files: map[string]string{
+					"owner/repo/.review-bot/personas/security.yaml": customSecurity,
+				},
+			},
+			personaName:     "security",
+			wantDisplayName: "Custom Security Override",
+		},
+		{
+			name: "fallback to builtin when repo has no override",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{},
+				Files:    map[string]string{},
+			},
+			personaName:     "security",
+			wantDisplayName: "Security Specialist",
+		},
+		{
+			name: "unknown persona",
+			fetcher: &MockRepoFetcher{
+				Contents: map[string][]ContentEntry{},
+				Files:    map[string]string{},
+			},
+			personaName: "nonexistent",
+			wantErr:     true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+			p, err := LoadPersonaWithFallback(ctx, tt.fetcher, "owner", "repo", tt.personaName)
+			if tt.wantErr {
+				if err == nil {
+					t.Error("expected error, got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if p.DisplayName != tt.wantDisplayName {
+				t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplayName)
+			}
+		})
+	}
+}
+
+func TestListAllPersonas(t *testing.T) {
+	customPersona := `name: repo-specific
+display_name: Repo Specific
+identity: A repo-specific reviewer.
+focus: []
+ignore: []
+severity:
+  major: Major
+  minor: Minor
+  nit: Nit
+`
+
+	fetcher := &MockRepoFetcher{
+		Contents: map[string][]ContentEntry{
+			"owner/repo/.review-bot/personas": {
+				{Name: "repo-specific.yaml", Path: ".review-bot/personas/repo-specific.yaml", Type: "file"},
+			},
+		},
+		Files: map[string]string{
+			"owner/repo/.review-bot/personas/repo-specific.yaml": customPersona,
+		},
+	}
+
+	ctx := context.Background()
+	names := ListAllPersonas(ctx, fetcher, "owner", "repo")
+
+	// Should include both built-ins and repo-specific
+	builtins := ListBuiltinPersonas()
+	if len(names) <= len(builtins) {
+		t.Error("expected more personas than just built-ins")
+	}
+
+	// Check repo-specific is included
+	found := false
+	for _, name := range names {
+		if name == "repo-specific" {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Error("repo-specific persona not found in list")
+	}
+}

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