feat: native SAP AI Core support

Add native SAP AI Core provider that handles OAuth token management and
deployment discovery automatically. This eliminates the need for the
external LLM proxy when running in SAP environments.

Changes:
- Add AICoreClient with OAuth token caching and deployment URL discovery
- Support both Anthropic and OpenAI models via AI Core deployments
- Update CI to use native AI Core provider
- Update action inputs to accept AI Core credentials
- Update README with AI Core configuration examples

Model names must match AI Core deployment names (e.g. anthropic--claude-4.6-sonnet, gpt-5).

.gitea/actions/review/action.yml

@@ -96,14 +96,6 @@ inputs:
     description: 'Local file with additional system prompt instructions (e.g. security review focus)'
     required: false
     default: ''
-  persona:
-    description: 'Built-in persona name (security, architect, docs)'
-    required: false
-    default: ''
-  persona-file:
-    description: 'Path to custom persona JSON file'
-    required: false
-    default: ''
 
 runs:
   using: 'composite'
@@ -185,8 +177,6 @@ runs:
         LLM_PROVIDER: ${{ inputs.llm-provider }}
         UPDATE_EXISTING: ${{ inputs.update-existing }}
         SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
-        PERSONA: ${{ inputs.persona }}
-        PERSONA_FILE: ${{ inputs.persona-file }}
         AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
         AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
         AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}

.gitea/workflows/pr-ready-gate.yml

@@ -1,38 +0,0 @@
-name: PR Ready Gate
-
-on:
-  pull_request:
-    types: [synchronize]
-
-jobs:
-  clear-labels:
-    runs-on: ubuntu-24.04
-    # Always run - curl commands are safe if labels don't exist
-    steps:
-      - name: Remove ready and self-reviewed labels, reassign to author
-        env:
-          GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}
-        run: |
-          PR_NUMBER=${{ github.event.pull_request.number }}
-          AUTHOR=${{ github.event.pull_request.user.login }}
-          READY_LABEL_ID=38
-          SELF_REVIEWED_LABEL_ID=37
-          
-          # Remove ready label if present
-          curl -sS -X DELETE \
-            -H "Authorization: token $GITEA_TOKEN" \
-            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${READY_LABEL_ID}" || true
-          
-          # Remove self-reviewed label if present
-          curl -sS -X DELETE \
-            -H "Authorization: token $GITEA_TOKEN" \
-            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${SELF_REVIEWED_LABEL_ID}" || true
-          
-          # Reassign to author
-          curl -sS -X PATCH \
-            -H "Authorization: token $GITEA_TOKEN" \
-            -H "Content-Type: application/json" \
-            -d "{\"assignees\": [\"${AUTHOR}\"]}" \
-            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/pulls/${PR_NUMBER}"
-          
-          echo "Cleared ready/self-reviewed labels and reassigned PR #${PR_NUMBER} to ${AUTHOR}"

CONVENTIONS.md

@@ -2,26 +2,8 @@
 
 ## Language & Dependencies
 
+- Go standard library only — no external dependencies.
 - Target the latest stable Go release.
-- **STRICT ALLOWLIST:** Only packages listed below may be imported. No exceptions.
-
-### Approved Third-Party Packages
-
-| Package | Use Case | Scope |
-|---------|----------|-------|
-| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
-| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
-
-**Any import not in this table or the Go standard library is forbidden.**
-
-Transitive dependencies of approved packages are automatically allowed.
-
-To request a new dependency:
-1. Open a PR that ONLY updates this table
-2. Requires explicit approval from Aaron
-3. After merge, a separate PR may use the package
-
-*Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
 
 ## Error Handling
 

Makefile

@@ -1,4 +1,4 @@
-.PHONY: build test test-integration lint clean coverage check-deps precommit
+.PHONY: build test test-integration lint clean coverage
 
 build:
 	go build -o review-bot ./cmd/review-bot/
@@ -12,15 +12,9 @@ test-integration:
 lint:
 	go vet ./...
 
-check-deps:
-	@./scripts/check-deps.sh
-
 clean:
 	rm -f review-bot
 
 coverage:
 	go test -coverprofile=coverage.out ./...
 	go tool cover -func=coverage.out
-
-# Precommit runs all checks required before pushing
-precommit: check-deps lint test

README.md

@@ -9,7 +9,7 @@ AI-powered code review bot for Gitea pull requests. Fetches diff + context, send
 - **Smart budget**: Automatically trims context to fit model token limits
 - **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
 - **Custom prompts**: Load additional instructions from a file (e.g. security-focused review)
-- **Minimal dependencies**: Go stdlib + `gopkg.in/yaml.v3` only
+- **Zero dependencies**: Go stdlib only
 
 ## Quick Start: Composite Action
 
@@ -207,8 +207,6 @@ AI Core handles OAuth token management and deployment discovery automatically. M
 | `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
 | `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
 | `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
-| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
-| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
 | `temperature` | No | `0` | LLM temperature (0 = server default) |
 | `timeout` | No | `300` | LLM request timeout in seconds |
 | `dry-run` | No | `false` | Print review to stdout instead of posting |
@@ -359,116 +357,3 @@ budget/             Token estimation + context trimming
 ## License
 
 MIT
-
-## Review Personas
-
-Personas provide role-based review specialization. Instead of generic code review, each persona focuses on a specific domain (security, architecture, documentation) with tailored prompts and severity calibration.
-
-### Built-in Personas
-
-| Persona | Focus |
-|---------|-------|
-| `security` | Vulnerabilities, auth bypass, secrets exposure, injection attacks |
-| `architect` | Design patterns, code organization, API contracts, testability |
-| `docs` | Documentation quality, API clarity, error messages |
-
-### Using Built-in Personas
-
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: security
-    persona: security
-    llm-model: claude-opus-4-20250514  # Security benefits from strong reasoning
-    ...
-```
-
-### Multiple Personas in Parallel
-
-```yaml
-jobs:
-  review:
-    strategy:
-      matrix:
-        include:
-          - name: security
-            persona: security
-          - name: architect
-            persona: architect
-    steps:
-      - uses: rodin/review-bot/.gitea/actions/review@v1
-        with:
-          reviewer-name: ${{ matrix.name }}
-          persona: ${{ matrix.persona }}
-          ...
-```
-
-Each persona posts independently with its own sentinel, so reviews don't interfere.
-
-
-### Custom Personas
-
-Create a YAML file with your domain-specific review focus:
-
-```yaml
-# .review/personas/trading.yaml
-name: trading
-display_name: Trading Domain Expert
-
-identity: |
-  You are a trading systems expert reviewing code for correctness.
-
-  Your expertise:
-  - Order lifecycle and state machines
-  - Fill handling and partial fills
-  - Position tracking and P&L calculations
-  - Event sourcing invariants
-
-focus:
-  - Order state machine correctness
-  - Fill handling edge cases (partial, overfill)
-  - Position and P&L calculation accuracy
-  - Event replay determinism
-  - Decimal precision for money
-
-ignore:
-  - Code style
-  - General performance
-  - Documentation formatting
-
-severity:
-  major: "Bugs that cause incorrect positions, fills, or money calculations"
-  minor: "Edge cases that could cause issues under unusual conditions"
-  nit: "Clarity improvements for domain logic"
-```
-
-Use it in CI:
-
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: trading
-    persona-file: .review/personas/trading.yaml
-    ...
-```
-
-YAML is the recommended format for personas because it supports:
-- Multi-line strings with `|` blocks (cleaner identity definitions)
-- Comments for documentation
-- More readable arrays and nested structures
-
-JSON is also supported for backwards compatibility—just use `.json` extension.
-
-
-### Persona vs system-prompt-file
-
-| Feature | `persona` / `persona-file` | `system-prompt-file` |
-|---------|---------------------------|----------------------|
-| Replaces base prompt | Yes | No (appends) |
-| Structured format | Yes (YAML/JSON) | No (freeform) |
-| Focus/ignore lists | Yes | Manual |
-| Severity calibration | Yes | Manual |
-| Header display name | Yes | No |
-| Built-in options | Yes | No |
-
-Use personas for domain-specialized reviews. Use `system-prompt-file` for minor tweaks to the generic review.

cmd/review-bot/main.go

@@ -70,8 +70,6 @@ func main() {
 	llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
 	llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
 	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai, anthropic, or aicore")
-	personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
-	personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
 	// AI Core specific flags (only used when provider=aicore)
 	aicoreClientID := flag.String("aicore-client-id", envOrDefault("AICORE_CLIENT_ID", ""), "SAP AI Core client ID (for provider=aicore)")
 	aicoreClientSecret := flag.String("aicore-client-secret", envOrDefault("AICORE_CLIENT_SECRET", ""), "SAP AI Core client secret (for provider=aicore)")
@@ -109,14 +107,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Validate persona flags are mutually exclusive
-	if *personaName != "" && *personaFile != "" {
-		slog.Error("--persona and --persona-file are mutually exclusive")
-		os.Exit(1)
-	}
-
-	// 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 {
 		slog.Error("invalid reviewer name", "error", err)
@@ -173,43 +163,6 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
 	defer cancel()
 
-	// Load persona if specified (after Gitea client init to support repo personas)
-	var persona *review.Persona
-	if *personaName != "" {
-		// 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 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 := repoPersonas[*personaName]; ok {
-			persona = p
-			slog.Info("loaded repo persona", "persona", persona.Name, "display", persona.DisplayName, "repo", owner+"/"+repoName)
-		} else {
-			// Fall back to built-in
-			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)
-	}
-
 	slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
 
 	// Step 1: Fetch PR metadata
@@ -273,14 +226,34 @@ func main() {
 	// Step 6b: Load additional system prompt if specified
 	additionalPrompt := ""
 	if *systemPromptFile != "" {
-		resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file")
+		workspace := os.Getenv("GITHUB_WORKSPACE")
+		if workspace == "" {
+			workspace, _ = os.Getwd()
+		}
+		absWorkspace, err := filepath.Abs(workspace)
 		if err != nil {
-			slog.Error("invalid system-prompt-file path", "error", err)
+			slog.Error("failed to resolve workspace path", "error", err)
+			os.Exit(1)
+		}
+		promptPath := filepath.Join(absWorkspace, *systemPromptFile)
+		promptPath = filepath.Clean(promptPath)
+		if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace {
+			slog.Error("system-prompt-file resolves outside workspace", "path", promptPath, "workspace", absWorkspace)
+			os.Exit(1)
+		}
+		// Resolve symlinks and re-validate to prevent symlink traversal
+		resolvedPath, err := filepath.EvalSymlinks(promptPath)
+		if err != nil {
+			slog.Error("failed to resolve system prompt file", "path", promptPath, "error", err)
+			os.Exit(1)
+		}
+		if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
+			slog.Error("system-prompt-file symlink resolves outside workspace", "resolved", resolvedPath, "workspace", absWorkspace)
 			os.Exit(1)
 		}
 		data, err := os.ReadFile(resolvedPath)
 		if err != nil {
-			slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err)
+			slog.Error("failed to read system prompt file", "path", promptPath, "error", err)
 			os.Exit(1)
 		}
 		additionalPrompt = string(data)
@@ -288,13 +261,7 @@ func main() {
 	}
 
 	// Step 7: Budget-aware prompt assembly
-	var systemBase string
-	if persona != nil {
-		systemBase = review.BuildPersonaSystemPrompt(persona)
-		slog.Debug("using persona system prompt", "persona", persona.Name)
-	} else {
-		systemBase = review.BuildSystemBase()
-	}
+	systemBase := review.BuildSystemBase()
 	if additionalPrompt != "" {
 		systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
 	}
@@ -351,12 +318,7 @@ func main() {
 	slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
 
 	// Step 10: Format and post review
-	var reviewBody string
-	if persona != nil && persona.DisplayName != "" {
-		reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName)
-	} else {
-		reviewBody = review.FormatMarkdown(result, *reviewerName)
-	}
+	reviewBody := review.FormatMarkdown(result, *reviewerName)
 
 	// Add commit footer so readers know which commit was evaluated
 	if pr.Head.Sha != "" {
@@ -544,9 +506,6 @@ 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 == "" {
@@ -562,22 +521,11 @@ 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()
 }
@@ -664,43 +612,6 @@ func validateReviewerName(name string) error {
 	return nil
 }
 
-// validateWorkspacePath ensures a file path is within the workspace and resolves
-// symlinks to prevent traversal attacks. Returns the resolved absolute path or
-// an error if the path is outside the workspace.
-func validateWorkspacePath(path, pathName string) (string, error) {
-	workspace := os.Getenv("GITHUB_WORKSPACE")
-	if workspace == "" {
-		workspace, _ = os.Getwd()
-	}
-	absWorkspace, err := filepath.Abs(workspace)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve workspace path: %w", err)
-	}
-
-	// Join and clean the path
-	fullPath := filepath.Join(absWorkspace, path)
-	fullPath = filepath.Clean(fullPath)
-
-	// Check path is within workspace using filepath.Rel (more robust than HasPrefix)
-	rel, err := filepath.Rel(absWorkspace, fullPath)
-	if err != nil || strings.HasPrefix(rel, "..") {
-		return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace)
-	}
-
-	// Resolve symlinks and re-validate to prevent symlink traversal
-	resolvedPath, err := filepath.EvalSymlinks(fullPath)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve %s: %w", pathName, err)
-	}
-
-	relResolved, err := filepath.Rel(absWorkspace, resolvedPath)
-	if err != nil || strings.HasPrefix(relResolved, "..") {
-		return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace)
-	}
-
-	return resolvedPath, nil
-}
-
 // buildSupersededBody creates the body for a superseded review: struck-through banner
 // with collapsed original content and the commit it was evaluated against.
 func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
@@ -811,32 +722,3 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
 	}
 	return evaluatedSHA != currentSHA
 }
-
-// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface.
-type giteaClientAdapter struct {
-	client *gitea.Client
-}
-
-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
-	}
-	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 (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return a.client.GetFileContent(ctx, owner, repo, filepath)
-}

cmd/review-bot/main_test.go

@@ -6,7 +6,6 @@ import (
 	"log/slog"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
 	"testing"
 
@@ -46,114 +45,6 @@ func TestValidateReviewerName(t *testing.T) {
 	}
 }
 
-func TestValidateWorkspacePath(t *testing.T) {
-	// Create a temp directory as our workspace
-	tmpDir := t.TempDir()
-
-	// Create a valid file inside the workspace
-	validFile := filepath.Join(tmpDir, "valid.json")
-	if err := os.WriteFile(validFile, []byte("{}"), 0644); err != nil {
-		t.Fatalf("failed to create test file: %v", err)
-	}
-
-	// Create a subdirectory with a file
-	subDir := filepath.Join(tmpDir, "subdir")
-	if err := os.MkdirAll(subDir, 0755); err != nil {
-		t.Fatalf("failed to create subdir: %v", err)
-	}
-	nestedFile := filepath.Join(subDir, "nested.json")
-	if err := os.WriteFile(nestedFile, []byte("{}"), 0644); err != nil {
-		t.Fatalf("failed to create nested file: %v", err)
-	}
-
-	// Create a symlink pointing outside the workspace
-	symlinkPath := filepath.Join(tmpDir, "evil-symlink.json")
-	if err := os.Symlink("/etc/passwd", symlinkPath); err != nil {
-		t.Fatalf("failed to create symlink: %v", err)
-	}
-
-	// Save and restore GITHUB_WORKSPACE
-	origWorkspace := os.Getenv("GITHUB_WORKSPACE")
-	defer os.Setenv("GITHUB_WORKSPACE", origWorkspace)
-
-	tests := []struct {
-		name      string
-		workspace string
-		path      string
-		wantErr   bool
-		errMatch  string
-	}{
-		{
-			name:      "valid relative path",
-			workspace: tmpDir,
-			path:      "valid.json",
-			wantErr:   false,
-		},
-		{
-			name:      "valid nested path",
-			workspace: tmpDir,
-			path:      "subdir/nested.json",
-			wantErr:   false,
-		},
-		{
-			name:      "path traversal attempt",
-			workspace: tmpDir,
-			path:      "../../../etc/passwd",
-			wantErr:   true,
-			errMatch:  "resolves outside workspace",
-		},
-		{
-			name:      "absolute path normalized to workspace-relative",
-			workspace: tmpDir,
-			path:      "/etc/passwd",
-			wantErr:   true,
-			// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
-			// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
-			errMatch: "failed to resolve",
-		},
-		{
-			name:      "nonexistent file",
-			workspace: tmpDir,
-			path:      "nonexistent.json",
-			wantErr:   true,
-			errMatch:  "failed to resolve",
-		},
-		{
-			name:      "symlink escaping workspace",
-			workspace: tmpDir,
-			path:      "evil-symlink.json",
-			wantErr:   true,
-			errMatch:  "symlink resolves outside workspace",
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			os.Setenv("GITHUB_WORKSPACE", tc.workspace)
-			resolved, err := validateWorkspacePath(tc.path, "test-file")
-
-			if tc.wantErr {
-				if err == nil {
-					t.Errorf("expected error for %q, got nil", tc.path)
-				} else if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) {
-					t.Errorf("error %q should contain %q", err.Error(), tc.errMatch)
-				}
-			} else {
-				if err != nil {
-					t.Errorf("expected no error for %q, got %v", tc.path, err)
-				}
-				if resolved == "" {
-					t.Error("expected non-empty resolved path")
-				}
-				// Verify resolved path is within workspace
-				if !strings.HasPrefix(resolved, tc.workspace) {
-					t.Errorf("resolved path %q not within workspace %q", resolved, tc.workspace)
-				}
-			}
-		})
-	}
-}
-
 func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
 	r := gitea.Review{
 		ID:    id,
@@ -165,6 +56,7 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
 	return r
 }
 
+
 func TestBuildSupersededBody(t *testing.T) {
 	original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
 	sentinel := "<!-- review-bot:sonnet -->"

docs/DESIGN-51-personas.md

@@ -1,334 +0,0 @@
-# Design: Role-based Review Personas (Issue #51)
-
-> **Note:** This design was revised during implementation to use JSON instead of YAML
-> to maintain the repository's zero-external-dependencies convention. All persona
-> files use JSON format. See "Design Revision" section at the end for details.
-
-## Problem
-
-Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to:
-
-1. **Redundancy** — Two reviewers (e.g., GPT + Claude twins) often flag identical issues
-2. **Gaps** — Generic reviewers miss specialized concerns (security, domain logic, architecture)
-3. **Noise** — NITs about style mixed with critical security findings
-4. **No ownership** — Findings lack clear domain attribution
-
-## Constraints
-
-- Must work with existing CLI flags and CI workflow patterns
-- Must not break backwards compatibility (existing configs still work)
-- Must integrate cleanly with the budget system (personas add to context)
-- Multiple personas running in parallel must not interfere with each other
-- Each persona must have clear scope boundaries (no duplication)
-
-## Proposed Approach
-
-### 1. Persona Definition
-
-A persona is a named review role with:
-- **Identity** — Who am I? What's my expertise?
-- **Focus** — What do I look for?
-- **Scope boundaries** — What do I explicitly NOT comment on?
-- **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain?
-
-Personas are defined in JSON files that can live:
-1. In the pattern repos (shared across projects)
-2. In the target repo (project-specific personas)
-3. Inline via a new `--persona-file` flag (JSON format)
-
-### 2. Persona File Format
-
-```json
-# .review/personas/security.yaml
-name: security
-display_name: Security Specialist
-model_preference: opus  # optional hint for expensive analysis
-
-identity: |
-  You are a security specialist reviewing code for vulnerabilities.
-  Your expertise: OWASP Top 10, injection attacks, auth/authz, secrets management,
-  event sourcing security (replay attacks, event injection).
-
-focus:
-  - Injection attacks (SQL, command, path traversal, template)
-  - Authentication and authorization gaps
-  - Secrets exposure (hardcoded credentials, tokens in logs)
-  - Input validation (unsanitized input, unsafe deserialization)
-  - Race conditions with security implications
-  - Event sourcing attack vectors
-
-ignore:
-  - Code style and naming conventions
-  - Performance (unless security-related)
-  - Documentation
-  - General code quality
-  - Test coverage
-
-severity:
-  critical: "Remote code execution, auth bypass, data exfiltration"
-  major: "Privilege escalation, information disclosure, DoS"
-  minor: "Missing rate limiting, verbose errors"
-  nit: "Theoretical risk with low exploitability"
-
-output_format: |
-  For each finding:
-  - Severity: [CRITICAL|MAJOR|MINOR|NIT]
-  - Attack vector: How could this be exploited?
-  - Evidence: Code snippet showing the vulnerability
-  - Recommendation: Specific fix
-```
-
-### 3. New CLI Flags
-
-```
---persona-file PATH      Path to persona JSON file (local or in repo)
---persona NAME           Built-in persona name (security, architect, domain)
-```
-
-Either flag sets the persona. If neither is provided, behavior is unchanged (generic review).
-
-### 4. Prompt Assembly
-
-Current flow:
-```
-SystemBase → Patterns → Conventions → [LLM]
-```
-
-New flow with persona:
-```
-PersonaPrompt (from YAML) → Patterns (filtered?) → Conventions → [LLM]
-```
-
-The persona's identity/focus/ignore/severity sections become the system prompt, replacing the generic "You are an expert code reviewer" base.
-
-### 5. Built-in Personas
-
-Ship with these built-in personas (loadable via `--persona NAME`):
-
-| Name | Focus |
-|------|-------|
-| `security` | Vulnerabilities, auth, secrets |
-| `architect` | Patterns, consistency, design |
-| `domain` | Business logic (requires repo-specific config) |
-| `docs` | Documentation, API clarity |
-
-Built-in personas live in `review/personas/` as embedded Go assets or YAML shipped with the binary.
-
-### 6. CI Workflow Integration
-
-Single persona:
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: security
-    persona: security
-    ...
-```
-
-Multiple personas (parallel jobs):
-```yaml
-jobs:
-  review:
-    strategy:
-      matrix:
-        include:
-          - name: security
-            persona: security
-          - name: architect
-            persona: architect
-    steps:
-      - uses: rodin/review-bot/.gitea/actions/review@v1
-        with:
-          reviewer-name: ${{ matrix.name }}
-          persona: ${{ matrix.persona }}
-```
-
-Custom persona from repo:
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: trading
-    persona-file: .review/personas/trading.yaml
-```
-
-### 7. Persona + Patterns Interaction
-
-Some personas benefit from filtered patterns:
-- Security → only security-related patterns
-- Architect → all patterns (structural focus)
-- Domain → domain docs, not language patterns
-
-For v1, keep it simple: all patterns are included regardless of persona. Future enhancement could add `patterns_filter` to persona YAML.
-
-### 8. Output Format Changes
-
-Persona name appears in the review header:
-```markdown
-# Security Review
-
-## Summary
-No critical vulnerabilities found in this change.
-
-## Findings
-| # | Severity | File | Line | Finding |
-...
-
-## Recommendation
-**APPROVE** — No security-relevant issues detected.
-
----
-*Review by security*
-<!-- review-bot:security -->
-```
-
-## State/Data Model
-
-### Persona struct
-
-```go
-// review/persona.go
-type Persona struct {
-    Name          string   `yaml:"name"`
-    DisplayName   string   `yaml:"display_name"`
-    ModelPref     string   `yaml:"model_preference,omitempty"`
-    Identity      string   `yaml:"identity"`
-    Focus         []string `yaml:"focus"`
-    Ignore        []string `yaml:"ignore"`
-    Severity      Severity `yaml:"severity"`
-    OutputFormat  string   `yaml:"output_format,omitempty"`
-}
-
-type Severity struct {
-    Critical string `yaml:"critical"`
-    Major    string `yaml:"major"`
-    Minor    string `yaml:"minor"`
-    Nit      string `yaml:"nit"`
-}
-```
-
-### Loading precedence
-
-1. `--persona-file PATH` → load from local file system
-2. `--persona NAME` → load from embedded built-ins
-3. Neither → use generic system prompt (current behavior)
-
-## Error Cases
-
-| Error | Handling |
-|-------|----------|
-| Persona file not found | Fatal exit with clear message |
-| Invalid YAML in persona file | Fatal exit with parse error |
-| Both `--persona` and `--persona-file` specified | Fatal exit: mutually exclusive |
-| Unknown built-in persona name | Fatal exit with list of valid names |
-| Empty identity in persona | Warning, fall back to generic prompt |
-
-## Edge Cases
-
-- **Empty focus list**: Valid — persona relies on identity alone
-- **Empty ignore list**: Valid — no explicit scope exclusions
-- **No severity section**: Use default MAJOR/MINOR/NIT definitions
-- **Model preference set but budget insufficient**: Ignore preference, log warning
-- **Persona file in pattern repo**: Fetch like other pattern files
-
-## Testing Strategy
-
-### Unit tests
-- `persona_test.go`: Parse valid/invalid YAML, validate required fields
-- `prompt_test.go`: Verify persona prompt assembly
-- Integration with budget: persona prompts count toward token limit
-
-### Integration tests
-- End-to-end with `--persona security` (built-in)
-- End-to-end with `--persona-file custom.yaml`
-- Backwards compatibility: no flags = generic behavior
-
-### Manual verification
-- Run security persona on a PR with obvious vulnerability
-- Verify security persona ignores style issues
-- Verify non-security persona doesn't flag security issues
-
-## Implementation Phases
-
-### Phase 1: Persona types and loading
-- [ ] `review/persona.go`: Persona struct + YAML parsing
-- [ ] `review/persona_test.go`: Unit tests
-- [ ] Embed built-in personas in binary
-- [ ] Compiles clean, tests pass
-
-### Phase 2: Prompt generation
-- [ ] `review/prompt.go`: `BuildPersonaPrompt(p Persona) string`
-- [ ] Modify `BuildSystemBase()` to accept optional persona
-- [ ] Integrate persona prompt with budget system
-- [ ] Tests for prompt assembly
-
-### Phase 3: CLI integration
-- [ ] Add `--persona` and `--persona-file` flags
-- [ ] Flag validation (mutually exclusive, valid names)
-- [ ] Load persona based on flags
-- [ ] Pass persona to prompt builder
-
-### Phase 4: Action integration
-- [ ] Add `persona` and `persona-file` inputs to action.yml
-- [ ] Update README with persona examples
-- [ ] End-to-end CI test
-
-### Phase 5: Built-in personas
-- [ ] `security.yaml` built-in
-- [ ] `architect.yaml` built-in
-- [ ] `docs.yaml` built-in
-- [ ] Document each persona's focus
-
-## Open Questions
-
-1. **Persona file location in repo**: Should we support `--persona-file .review/security.yaml` where the file is fetched from the PR's repo (like conventions)? This adds complexity but enables project-specific personas without action changes.
-
-2. **Model preference enforcement**: If persona specifies `model_preference: opus` but the action uses a different model, should we warn? Override? Ignore? Current thinking: log warning, use the specified model (user controls model via action input).
-
-3. **Severity override output**: If persona defines custom severity levels (CRITICAL), should the JSON output include them, or map back to standard MAJOR/MINOR/NIT? Current thinking: keep standard output format, use severity calibration only for prompt guidance.
-
-## Completion Checklist
-
-1. Persona struct matches YAML schema exactly?
-2. Built-in personas embedded in binary (not external files)?
-3. `--persona` and `--persona-file` are mutually exclusive?
-4. Unknown persona name produces clear error with valid options?
-5. Empty persona file fields have sensible defaults?
-6. Persona prompt integrates with budget system (token counting)?
-7. Backwards compatibility: no flags = current behavior?
-8. Review header shows persona display name?
-9. Sentinel still uses reviewer-name (not persona name)?
-10. Unit tests cover parse errors, missing fields, valid YAML?
-
-## Design Review Findings (Self-Review)
-
-### Finding 1: Severity Mapping
-The persona YAML allows `critical` severity, but the LLM output parser (`review/parser.go`) only accepts MAJOR/MINOR/NIT. 
-
-**Resolution:** Keep standard output format. Persona severity section is ONLY for calibrating the LLM's judgment (prompt guidance). Output must still use MAJOR/MINOR/NIT. Document this clearly in persona format docs.
-
-### Finding 2: Embedding Built-in Personas
-Go doesn't natively embed YAML. Must use `//go:embed` directive (Go 1.16+).
-
-**Resolution:** Create `review/personas/` directory with YAML files and use:
-```go
-//go:embed personas/*.yaml
-var embeddedPersonas embed.FS
-```
-
-### Finding 3: display_name vs reviewer-name
-Design says header shows "persona display name" but sentinel uses "reviewer-name". This is correct - they serve different purposes:
-- `display_name` → human-readable header ("Security Specialist Review")
-- `reviewer-name` → machine sentinel for cleanup (`<!-- review-bot:security -->`)
-
-When persona is used, `display_name` takes precedence for the header title, but `reviewer-name` (CLI flag) is still used for the sentinel.
-
-## Design Revision: YAML with gopkg.in/yaml.v3
-
-**Decision:** Add `gopkg.in/yaml.v3` as a dependency.
-
-YAML is preferred over JSON for persona files because:
-- Multi-line strings are cleaner (no escaping quotes in identity/focus text)
-- Comments are supported for documentation
-- More human-readable for complex persona definitions
-
-The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas.

docs/DESIGN-57-yaml-persona.md

@@ -1,108 +0,0 @@
-# Design: YAML Support for Persona Files (#57)
-
-## Problem
-
-JSON is awkward for persona files that contain multi-line text (identity, severity descriptions). YAML supports cleaner multi-line strings and comments, improving readability and maintainability.
-
-## Constraints
-
-- Backwards compatibility: existing JSON personas must continue to work
-- Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486)
-- Consistency: use `.yaml` extension (not `.yml`)
-- Library: use `gopkg.in/yaml.v3` (approved in CONVENTIONS.md) with explicit depth limiting
-
-## Proposed Approach
-
-1. **Update `parsePersona`** to detect format from file extension
-2. **Add YAML parsing** with explicit depth limit (defense in depth)
-3. **Keep JSON as fallback** for files without `.yaml`/`.yml` extension
-4. **Convert built-in personas** to YAML format
-5. **Update embed directive** to include both formats
-
-### File Extension Detection
-
-```go
-func parsePersona(data []byte, source string) (*Persona, error) {
-    isYAML := strings.HasSuffix(source, ".yaml") || strings.HasSuffix(source, ".yml")
-    if isYAML {
-        return parseYAML(data, source)
-    }
-    return parseJSON(data, source)
-}
-```
-
-### YAML Parsing with Depth Protection
-
-```go
-func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
-    var node yaml.Node
-    dec := yaml.NewDecoder(bytes.NewReader(data))
-    if err := dec.Decode(&node); err != nil {
-        return err
-    }
-    if err := checkYAMLDepth(&node, 0, maxDepth); err != nil {
-        return err
-    }
-    return node.Decode(out)
-}
-
-func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
-    if depth > maxDepth {
-        return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
-    }
-    // Handle alias nodes by following the Alias pointer
-    if node.Kind == yaml.AliasNode && node.Alias != nil {
-        return checkYAMLDepth(node.Alias, depth, maxDepth)
-    }
-    for _, child := range node.Content {
-        if err := checkYAMLDepth(child, depth+1, maxDepth); err != nil {
-            return err
-        }
-    }
-    return nil
-}
-```
-
-The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct.
-
-## State/Data Model
-
-No new state. Same `Persona` struct, just different parsing.
-
-## Error Cases
-
-| Error | Handling |
-|-------|----------|
-| Invalid YAML syntax | Return parse error with source file |
-| Deeply nested YAML | Library rejects (v1.16.0+ fix) |
-| Unknown extension | Fall back to JSON parsing |
-| Missing required fields | Validation rejects after parse |
-
-## Edge Cases
-
-- File with `.json` extension but YAML content → JSON parse fails, user sees error
-- File with no extension → defaults to JSON
-- Embedded persona reference like `builtin:security` → detect by embed path (`personas/X.yaml`)
-
-## Testing Strategy
-
-1. Unit tests for YAML parsing (valid, invalid, deeply nested)
-2. Unit tests for extension detection
-3. Integration test for built-in personas (now YAML)
-4. Backwards compat test: verify JSON still works for external files
-
-## Completion Checklist
-
-1. [ ] `go-yaml` dependency added at v1.16.0+
-2. [ ] Extension detection uses case-insensitive comparison
-3. [ ] YAML parse errors include source file name
-4. [ ] JSON parsing still works for `.json` files
-5. [ ] Built-in personas converted to YAML with readable multi-line strings
-6. [ ] Embed directive updated to include `*.yaml`
-7. [ ] Test for deeply nested YAML rejection
-8. [ ] All existing tests pass
-
-## Open Questions
-
-- Should we support both `.yaml` AND `.yml`? Issue says `.yaml` only for consistency, but some users expect `.yml`. **Decision:** Support both for reading, recommend `.yaml` in docs.
-- Should we add a "format" field to detect mismatched extension/content? **Decision:** No, keep it simple. Extension determines format.

go.mod

@@ -1,5 +1,3 @@
 module gitea.weiker.me/rodin/review-bot
 
 go 1.26.2
-
-require gopkg.in/yaml.v3 v3.0.1

go.sum

@@ -1,4 +0,0 @@
-gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
-gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
-gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
-gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

review/formatter.go

@@ -7,37 +7,10 @@ import (
 
 // FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review.
 func FormatMarkdown(result *ReviewResult, reviewerName string) string {
-	return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
-}
-
-// GiteaEvent converts the verdict to the Gitea API event string.
-func GiteaEvent(verdict string) string {
-	switch verdict {
-	case "APPROVE":
-		return "APPROVED"
-	case "REQUEST_CHANGES":
-		return "REQUEST_CHANGES"
-	default:
-		return "COMMENT"
-	}
-}
-
-// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
-// 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
 
-	// Use display name for header, or fall back to sentinel name
-	headerName := displayName
-	if headerName == "" {
-		headerName = sentinelName
-	}
-
-	if headerName != "" {
-		title := CapitalizeFirst(headerName)
+	if reviewerName != "" {
+		title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:]
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 
@@ -60,11 +33,23 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	sb.WriteString("## Recommendation\n\n")
 	sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
 
-	if sentinelName != "" {
-		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", headerName))
+	if reviewerName != "" {
+		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
 		// Hidden sentinel for identifying this bot's reviews during cleanup
-		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", sentinelName))
+		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
 	}
 
 	return sb.String()
 }
+
+// GiteaEvent converts the verdict to the Gitea API event string.
+func GiteaEvent(verdict string) string {
+	switch verdict {
+	case "APPROVE":
+		return "APPROVED"
+	case "REQUEST_CHANGES":
+		return "REQUEST_CHANGES"
+	default:
+		return "COMMENT"
+	}
+}

review/formatter_test.go

@@ -159,58 +159,3 @@ func TestFormatMarkdown_RoleTitle(t *testing.T) {
 		t.Error("should not contain role title header when reviewer name is empty")
 	}
 }
-
-func TestFormatMarkdownWithDisplay(t *testing.T) {
-	result := &ReviewResult{
-		Verdict:        "APPROVE",
-		Summary:        "Test summary",
-		Findings:       nil,
-		Recommendation: "Test recommendation",
-	}
-
-	t.Run("with display name", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "Security Specialist", "security")
-
-		// Header should use display name
-		if !strings.Contains(body, "# Security Specialist Review") {
-			t.Error("header should use display name")
-		}
-
-		// Sentinel should use sentinel name
-		if !strings.Contains(body, "<!-- review-bot:security -->") {
-			t.Error("sentinel should use sentinel name")
-		}
-
-		// Footer "Review by" should use display name
-		if !strings.Contains(body, "*Review by Security Specialist*") {
-			t.Error("footer should use display name")
-		}
-	})
-
-	t.Run("without display name", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "", "reviewer")
-
-		// Should fall back to sentinel name for header
-		if !strings.Contains(body, "# Reviewer Review") {
-			t.Error("header should fall back to sentinel name")
-		}
-
-		if !strings.Contains(body, "<!-- review-bot:reviewer -->") {
-			t.Error("sentinel should use sentinel name")
-		}
-	})
-
-	t.Run("empty both names", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "", "")
-
-		// Should not have header
-		if strings.Contains(body, "# ") && strings.Contains(body, " Review") {
-			t.Error("should not have header when both names empty")
-		}
-
-		// Should not have sentinel
-		if strings.Contains(body, "<!-- review-bot:") {
-			t.Error("should not have sentinel when sentinel name empty")
-		}
-	})
-}

review/persona.go

@@ -1,259 +0,0 @@
-package review
-
-import (
-	"bytes"
-	"embed"
-	"encoding/json"
-	"fmt"
-	"os"
-	"sort"
-	"strings"
-	"unicode/utf8"
-
-	"gopkg.in/yaml.v3"
-)
-
-//go:embed personas/*.yaml
-var embeddedPersonas embed.FS
-
-// MaxPersonaFileSize is the maximum size for persona files (64 KB).
-// This prevents denial-of-service via excessively large files.
-const MaxPersonaFileSize = 64 * 1024
-
-// MaxYAMLDepth is the maximum nesting depth allowed in YAML persona files.
-// This prevents stack exhaustion from deeply nested structures.
-const MaxYAMLDepth = 20
-
-// MaxYAMLNodes is the maximum number of YAML nodes allowed in persona files.
-// This prevents DoS via wide-but-shallow structures that bypass depth limits.
-const MaxYAMLNodes = 1000
-
-// Persona defines a specialized review role with focused expertise.
-type Persona struct {
-	Name         string   `json:"name" yaml:"name"`
-	DisplayName  string   `json:"display_name" yaml:"display_name"`
-	ModelPref    string   `json:"model_preference,omitempty" yaml:"model_preference,omitempty"`
-	Identity     string   `json:"identity" yaml:"identity"`
-	Focus        []string `json:"focus" yaml:"focus"`
-	Ignore       []string `json:"ignore" yaml:"ignore"`
-	Severity     Severity `json:"severity" yaml:"severity"`
-	OutputFormat string   `json:"output_format,omitempty" yaml:"output_format,omitempty"`
-}
-
-// Severity defines what constitutes each severity level for this persona.
-// These are prompt guidance for the LLM, not output format changes.
-type Severity struct {
-	Major string `json:"major" yaml:"major"`
-	Minor string `json:"minor" yaml:"minor"`
-	Nit   string `json:"nit" yaml:"nit"`
-}
-
-// 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.
-//
-// Symlinks are supported: os.Stat follows symlinks, so a symlink pointing to
-// a regular file will pass the IsRegular() check. Symlinks to non-regular files
-// (directories, FIFOs, devices) are still rejected.
-func LoadPersona(path string) (*Persona, error) {
-	// os.Stat follows symlinks, so symlinks to regular files are supported.
-	// The IsRegular() check operates on the target, not the symlink itself.
-	info, err := os.Stat(path)
-	if err != nil {
-		return nil, fmt.Errorf("read persona file %s: %w", path, err)
-	}
-	if !info.Mode().IsRegular() {
-		return nil, fmt.Errorf("persona file %s is not a regular file", path)
-	}
-	if info.Size() > MaxPersonaFileSize {
-		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
-	}
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return nil, fmt.Errorf("read persona file %s: %w", path, err)
-	}
-	// Re-check size after read to defend against TOCTOU races where file
-	// grows between stat and read (e.g., appending process, replaced file).
-	if len(data) > MaxPersonaFileSize {
-		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
-	}
-	return parsePersona(data, path)
-}
-
-// LoadBuiltinPersona loads a built-in persona by name.
-// Returns an error if the persona doesn't exist.
-// Built-in personas are stored in YAML format only (see embed directive).
-func LoadBuiltinPersona(name string) (*Persona, error) {
-	yamlFile := name + ".yaml"
-	data, err := embeddedPersonas.ReadFile("personas/" + yamlFile)
-	if err != nil {
-		available := ListBuiltinPersonas()
-		return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
-	}
-	return parsePersona(data, "builtin:"+yamlFile)
-}
-
-// ListBuiltinPersonas returns the names of all built-in personas in sorted order.
-// Returns an empty slice if the embedded directory cannot be read.
-func ListBuiltinPersonas() []string {
-	entries, err := embeddedPersonas.ReadDir("personas")
-	if err != nil {
-		return []string{}
-	}
-	seen := make(map[string]bool)
-	for _, e := range entries {
-		if e.IsDir() {
-			continue
-		}
-		name := e.Name()
-		// Strip extension to get persona name
-		var personaName string
-		switch {
-		case strings.HasSuffix(name, ".yaml"):
-			personaName = strings.TrimSuffix(name, ".yaml")
-		case strings.HasSuffix(name, ".yml"):
-			personaName = strings.TrimSuffix(name, ".yml")
-		case strings.HasSuffix(name, ".json"):
-			personaName = strings.TrimSuffix(name, ".json")
-		default:
-			continue
-		}
-		if !seen[personaName] {
-			seen[personaName] = 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) {
-	lowerSource := strings.ToLower(source)
-	isYAML := strings.HasSuffix(lowerSource, ".yaml") || strings.HasSuffix(lowerSource, ".yml")
-
-	var p Persona
-	var err error
-	if isYAML {
-		err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
-	} else {
-		// Use json.Decoder with DisallowUnknownFields for consistency with
-		// YAML's KnownFields(true) - both reject unknown fields to catch typos.
-		dec := json.NewDecoder(bytes.NewReader(data))
-		dec.DisallowUnknownFields()
-		err = dec.Decode(&p)
-	}
-	if err != nil {
-		return nil, fmt.Errorf("parse persona %s: %w", source, err)
-	}
-	if err := validatePersona(&p, source); err != nil {
-		return nil, err
-	}
-	return &p, nil
-}
-
-// unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting
-// and strict field checking. This protects against stack exhaustion from deeply
-// nested structures and catches typos in field names.
-// Multi-document YAML files are rejected to prevent silent data loss.
-func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
-	// First pass: decode into a yaml.Node to check depth limits and node counts.
-	// This prevents stack exhaustion before we attempt to decode into structs.
-	var node yaml.Node
-	dec := yaml.NewDecoder(bytes.NewReader(data))
-	if err := dec.Decode(&node); err != nil {
-		return err
-	}
-
-	// Reject multi-document YAML files - silently ignoring additional documents
-	// could lead to confusing behavior where users think their changes take effect.
-	var extra yaml.Node
-	if dec.Decode(&extra) == nil {
-		return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
-	}
-
-	nodeCount := 0
-	if err := checkYAMLDepth(&node, 0, maxDepth, MaxYAMLNodes, make(map[*yaml.Node]struct{}), &nodeCount); err != nil {
-		return err
-	}
-
-	// Second pass: decode with strict field checking enabled.
-	// KnownFields(true) rejects unknown keys, catching typos like "focuss" or "identiy".
-	// We must re-decode from the original data because yaml.Node.Decode() doesn't
-	// support the KnownFields option.
-	strictDec := yaml.NewDecoder(bytes.NewReader(data))
-	strictDec.KnownFields(true)
-	return strictDec.Decode(out)
-}
-
-// checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit
-// or the total node count limit. It also detects alias cycles to prevent infinite
-// recursion from crafted YAML with self-referential aliases.
-func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*yaml.Node]struct{}, nodeCount *int) error {
-	if depth > maxDepth {
-		return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
-	}
-
-	// Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
-	*nodeCount++
-	if *nodeCount > maxNodes {
-		return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
-	}
-
-	// Cycle detection: if we've seen this node before, we're in a cycle.
-	if _, ok := seen[node]; ok {
-		return nil // Already validated this subtree, skip to avoid infinite recursion.
-	}
-	seen[node] = struct{}{}
-
-	// Handle alias nodes: follow the alias to its anchor target.
-	// Increment depth when following aliases since they expand the effective structure.
-	if node.Kind == yaml.AliasNode && node.Alias != nil {
-		return checkYAMLDepth(node.Alias, depth+1, maxDepth, maxNodes, seen, nodeCount)
-	}
-
-	for _, child := range node.Content {
-		if err := checkYAMLDepth(child, depth+1, maxDepth, maxNodes, seen, nodeCount); err != nil {
-			return err
-		}
-	}
-	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)
-	}
-	if p.Identity == "" {
-		return fmt.Errorf("persona %s: identity is required", source)
-	}
-	// DisplayName defaults to Name if not set
-	if p.DisplayName == "" {
-		p.DisplayName = p.Name
-	}
-	return nil
-}
-
-// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way.
-// Returns the original string if it's empty.
-func CapitalizeFirst(s string) string {
-	if s == "" {
-		return s
-	}
-	r, size := utf8.DecodeRuneInString(s)
-	if r == utf8.RuneError {
-		return s
-	}
-	return strings.ToUpper(string(r)) + s[size:]
-}

review/persona_prompt.go

@@ -1,104 +0,0 @@
-package review
-
-import (
-	"fmt"
-	"strings"
-)
-
-// BuildPersonaSystemPrompt constructs a system prompt from a persona definition.
-// This replaces BuildSystemBase when a persona is provided.
-func BuildPersonaSystemPrompt(p *Persona) string {
-	var sb strings.Builder
-
-	// Identity section
-	sb.WriteString(p.Identity)
-	sb.WriteString("\n\n")
-
-	// Focus section
-	if len(p.Focus) > 0 {
-		sb.WriteString("## Focus Areas\n\n")
-		sb.WriteString("Concentrate your review on:\n")
-		for _, f := range p.Focus {
-			sb.WriteString(fmt.Sprintf("- %s\n", f))
-		}
-		sb.WriteString("\n")
-	}
-
-	// Ignore section
-	if len(p.Ignore) > 0 {
-		sb.WriteString("## Explicitly Out of Scope\n\n")
-		sb.WriteString("Do NOT comment on:\n")
-		for _, i := range p.Ignore {
-			sb.WriteString(fmt.Sprintf("- %s\n", i))
-		}
-		sb.WriteString("\n")
-	}
-
-	// Severity calibration
-	if p.Severity.Major != "" || p.Severity.Minor != "" || p.Severity.Nit != "" {
-		sb.WriteString("## Severity Calibration\n\n")
-		sb.WriteString("Use these severity definitions for YOUR domain:\n")
-		if p.Severity.Major != "" {
-			sb.WriteString(fmt.Sprintf("- **MAJOR**: %s\n", p.Severity.Major))
-		}
-		if p.Severity.Minor != "" {
-			sb.WriteString(fmt.Sprintf("- **MINOR**: %s\n", p.Severity.Minor))
-		}
-		if p.Severity.Nit != "" {
-			sb.WriteString(fmt.Sprintf("- **NIT**: %s\n", p.Severity.Nit))
-		}
-		sb.WriteString("\n")
-	}
-
-	// Output format instructions (shared schema from prompt.go)
-	sb.WriteString("## Review Instructions\n\n")
-	sb.WriteString("CONTEXT:\n")
-	sb.WriteString("- You will receive the full content of modified files for reference, followed by the diff showing what changed.\n")
-	sb.WriteString("- The diff shows ONLY what was added/removed. The full file content provides complete context.\n")
-	sb.WriteString("- Focus your review on the CHANGES (the diff), using the full files for context.\n\n")
-	sb.WriteString("Your task:\n")
-	sb.WriteString("1. Review the diff for issues within YOUR focus areas only.\n")
-	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
-	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
-	sb.WriteString("Output format:\n")
-	sb.WriteString(outputSchemaJSON)
-	sb.WriteString("\n\n")
-	sb.WriteString(verdictRules)
-	sb.WriteString("\n- Only report findings within your focus areas. Ignore everything else.\n")
-	sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
-	sb.WriteString("- If the diff has no changes relevant to your focus areas, APPROVE with no findings.\n")
-
-	// Custom output format if provided
-	if p.OutputFormat != "" {
-		sb.WriteString("\n\n## Additional Output Guidelines\n\n")
-		sb.WriteString(p.OutputFormat)
-	}
-
-	return sb.String()
-}
-
-// BuildSystemPromptWithPersona constructs the full system prompt, using either
-// a persona or the default generic prompt. This is a convenience wrapper that
-// combines BuildPersonaSystemPrompt (or BuildSystemBase) with patterns and conventions.
-// It is exported for use by callers who want one-shot prompt assembly.
-func BuildSystemPromptWithPersona(persona *Persona, conventions, patterns string) string {
-	var base string
-	if persona != nil {
-		base = BuildPersonaSystemPrompt(persona)
-	} else {
-		base = BuildSystemBase()
-	}
-
-	var sb strings.Builder
-	sb.WriteString(base)
-
-	if patterns != "" {
-		sb.WriteString(fmt.Sprintf("\n\n## Language Patterns & Idioms\n\nUse the following patterns as review criteria. Code that violates these established patterns is a finding:\n\n%s\n", patterns))
-	}
-
-	if conventions != "" {
-		sb.WriteString(fmt.Sprintf("\n\n## Repository Conventions\n\nThe repository has the following coding conventions that must be respected:\n\n%s\n", conventions))
-	}
-
-	return sb.String()
-}

review/persona_prompt_test.go

@@ -1,157 +0,0 @@
-package review
-
-import (
-	"strings"
-	"testing"
-)
-
-func TestBuildPersonaSystemPrompt(t *testing.T) {
-	p := &Persona{
-		Name:        "security",
-		DisplayName: "Security Specialist",
-		Identity:    "You are a security specialist.",
-		Focus:       []string{"injection attacks", "auth bypass"},
-		Ignore:      []string{"code style", "performance"},
-		Severity: Severity{
-			Major: "exploitable vulnerabilities",
-			Minor: "defense in depth",
-			Nit:   "theoretical risks",
-		},
-	}
-
-	prompt := BuildPersonaSystemPrompt(p)
-
-	// Check identity is included
-	if !strings.Contains(prompt, "You are a security specialist.") {
-		t.Error("prompt should contain identity")
-	}
-
-	// Check focus areas
-	if !strings.Contains(prompt, "Focus Areas") {
-		t.Error("prompt should contain Focus Areas section")
-	}
-	if !strings.Contains(prompt, "injection attacks") {
-		t.Error("prompt should contain focus item")
-	}
-
-	// Check ignore section
-	if !strings.Contains(prompt, "Out of Scope") {
-		t.Error("prompt should contain Out of Scope section")
-	}
-	if !strings.Contains(prompt, "code style") {
-		t.Error("prompt should contain ignore item")
-	}
-
-	// Check severity calibration
-	if !strings.Contains(prompt, "Severity Calibration") {
-		t.Error("prompt should contain Severity Calibration section")
-	}
-	if !strings.Contains(prompt, "exploitable vulnerabilities") {
-		t.Error("prompt should contain major severity definition")
-	}
-
-	// Check JSON output format is included
-	if !strings.Contains(prompt, `"verdict"`) {
-		t.Error("prompt should contain JSON output format")
-	}
-	if !strings.Contains(prompt, "APPROVE") {
-		t.Error("prompt should mention APPROVE verdict")
-	}
-}
-
-func TestBuildPersonaSystemPromptMinimal(t *testing.T) {
-	// Minimal persona with only required fields
-	p := &Persona{
-		Name:     "minimal",
-		Identity: "You are a minimal reviewer.",
-	}
-
-	prompt := BuildPersonaSystemPrompt(p)
-
-	// Should still work without optional fields
-	if !strings.Contains(prompt, "You are a minimal reviewer.") {
-		t.Error("prompt should contain identity")
-	}
-
-	// Should not have empty sections
-	if strings.Contains(prompt, "Focus Areas") && !strings.Contains(prompt, "Concentrate your review on:") {
-		t.Error("should not have Focus Areas header without content")
-	}
-}
-
-func TestBuildSystemPromptWithPersona(t *testing.T) {
-	t.Run("with persona", func(t *testing.T) {
-		p := &Persona{
-			Name:     "test",
-			Identity: "Test persona identity.",
-			Focus:    []string{"testing"},
-		}
-
-		prompt := BuildSystemPromptWithPersona(p, "test conventions", "test patterns")
-
-		if !strings.Contains(prompt, "Test persona identity.") {
-			t.Error("should contain persona identity")
-		}
-		if !strings.Contains(prompt, "test conventions") {
-			t.Error("should contain conventions")
-		}
-		if !strings.Contains(prompt, "test patterns") {
-			t.Error("should contain patterns")
-		}
-	})
-
-	t.Run("without persona", func(t *testing.T) {
-		prompt := BuildSystemPromptWithPersona(nil, "test conventions", "test patterns")
-
-		// Should use default system base
-		if !strings.Contains(prompt, "expert code reviewer") {
-			t.Error("should contain default system base when no persona")
-		}
-		if !strings.Contains(prompt, "test conventions") {
-			t.Error("should contain conventions")
-		}
-	})
-
-	t.Run("empty conventions and patterns", func(t *testing.T) {
-		p := &Persona{
-			Name:     "test",
-			Identity: "Test identity.",
-		}
-
-		prompt := BuildSystemPromptWithPersona(p, "", "")
-
-		if strings.Contains(prompt, "Language Patterns") {
-			t.Error("should not contain patterns section when empty")
-		}
-		if strings.Contains(prompt, "Repository Conventions") {
-			t.Error("should not contain conventions section when empty")
-		}
-	})
-}
-
-func TestPersonaPromptContainsOutputRules(t *testing.T) {
-	p := &Persona{
-		Name:     "test",
-		Identity: "Test.",
-	}
-
-	prompt := BuildPersonaSystemPrompt(p)
-
-	// Must contain the critical output rules
-	requiredStrings := []string{
-		"APPROVE",
-		"REQUEST_CHANGES",
-		"MAJOR",
-		"MINOR",
-		"NIT",
-		"verdict",
-		"findings",
-		"CI",
-	}
-
-	for _, s := range requiredStrings {
-		if !strings.Contains(prompt, s) {
-			t.Errorf("prompt should contain %q", s)
-		}
-	}
-}

review/persona_test.go

@@ -1,778 +0,0 @@
-package review
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"gopkg.in/yaml.v3"
-)
-
-func TestLoadBuiltinPersona(t *testing.T) {
-	tests := []struct {
-		name        string
-		personaName string
-		wantErr     bool
-		wantDisplay string
-	}{
-		{
-			name:        "security persona",
-			personaName: "security",
-			wantErr:     false,
-			wantDisplay: "Security Specialist",
-		},
-		{
-			name:        "architect persona",
-			personaName: "architect",
-			wantErr:     false,
-			wantDisplay: "Software Architect",
-		},
-		{
-			name:        "docs persona",
-			personaName: "docs",
-			wantErr:     false,
-			wantDisplay: "Documentation Reviewer",
-		},
-		{
-			name:        "unknown persona",
-			personaName: "nonexistent",
-			wantErr:     true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			p, err := LoadBuiltinPersona(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.Name != tt.personaName {
-				t.Errorf("Name = %q, want %q", p.Name, tt.personaName)
-			}
-			if p.DisplayName != tt.wantDisplay {
-				t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplay)
-			}
-			if p.Identity == "" {
-				t.Error("Identity should not be empty")
-			}
-			if len(p.Focus) == 0 {
-				t.Error("Focus should not be empty")
-			}
-		})
-	}
-}
-
-func TestListBuiltinPersonas(t *testing.T) {
-	names := ListBuiltinPersonas()
-	if len(names) == 0 {
-		t.Fatal("expected at least one built-in persona")
-	}
-
-	// Check for expected personas
-	expected := map[string]bool{"security": false, "architect": false, "docs": false}
-	for _, name := range names {
-		if _, ok := expected[name]; ok {
-			expected[name] = true
-		}
-	}
-	for name, found := range expected {
-		if !found {
-			t.Errorf("expected built-in persona %q not found", name)
-		}
-	}
-}
-
-func TestLoadPersonaFromYAMLFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.yaml")
-
-	content := `# Test persona
-name: test
-display_name: Test Persona
-identity: |
-  You are a test persona.
-  Multi-line identity works.
-focus:
-  - testing
-  - validation
-ignore:
-  - nothing
-severity:
-  major: Big problems
-  minor: Small problems
-  nit: Tiny problems
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test Persona" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
-	}
-	if len(p.Focus) != 2 {
-		t.Errorf("Focus len = %d, want 2", len(p.Focus))
-	}
-	if !strings.Contains(p.Identity, "Multi-line") {
-		t.Error("Identity should contain multi-line content")
-	}
-}
-
-func TestLoadPersonaFromYMLFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.yml")
-
-	content := `name: test
-display_name: Test YML
-identity: Test identity
-focus:
-  - testing
-ignore: []
-severity:
-  major: Big
-  minor: Small
-  nit: Tiny
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test YML" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test YML")
-	}
-}
-
-func TestLoadPersonaFromJSONFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.json")
-
-	content := `{
-		"name": "test",
-		"display_name": "Test Persona",
-		"identity": "You are a test persona.\nMulti-line identity works.",
-		"focus": ["testing", "validation"],
-
-		"ignore": ["nothing"],
-		"severity": {
-			"major": "Big problems",
-			"minor": "Small problems",
-			"nit": "Tiny problems"
-		}
-	}`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test Persona" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
-	}
-	if len(p.Focus) != 2 {
-		t.Errorf("Focus len = %d, want 2", len(p.Focus))
-	}
-	if !strings.Contains(p.Identity, "Multi-line") {
-		t.Error("Identity should contain multi-line content")
-	}
-}
-
-func TestLoadPersonaValidation(t *testing.T) {
-	tests := []struct {
-		name    string
-		content string
-		ext     string
-		wantErr string
-	}{
-		{
-			name:    "missing name yaml",
-			content: "identity: test\n",
-			ext:     ".yaml",
-			wantErr: "name is required",
-		},
-		{
-			name:    "missing identity yaml",
-			content: "name: test\n",
-			ext:     ".yaml",
-			wantErr: "identity is required",
-		},
-		{
-			name:    "missing name json",
-			content: `{"identity": "test"}`,
-			ext:     ".json",
-			wantErr: "name is required",
-		},
-		{
-			name:    "missing identity json",
-			content: `{"name": "test"}`,
-			ext:     ".json",
-			wantErr: "identity is required",
-		},
-		{
-			name:    "display_name defaults to name",
-			content: "name: test\nidentity: test identity\n",
-			ext:     ".yaml",
-			// No error expected - should succeed
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "test"+tt.ext)
-			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			p, err := LoadPersona(path)
-			if tt.wantErr != "" {
-				if err == nil {
-					t.Errorf("expected error containing %q, got nil", tt.wantErr)
-					return
-				}
-				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)
-			}
-			// Check display_name defaulting
-			if p.DisplayName == "" {
-				t.Error("DisplayName should default to Name")
-			}
-			if p.DisplayName != p.Name {
-				t.Errorf("DisplayName should default to Name, got %q", p.DisplayName)
-			}
-		})
-	}
-}
-
-func TestLoadPersonaFileNotFound(t *testing.T) {
-	_, err := LoadPersona("/nonexistent/path/persona.yaml")
-	if err == nil {
-		t.Error("expected error for nonexistent file")
-	}
-}
-
-func TestLoadPersonaInvalidYAML(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.yaml")
-	if err := os.WriteFile(path, []byte("not valid yaml:\n  - [broken"), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for invalid YAML")
-	}
-}
-
-func TestLoadPersonaInvalidJSON(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.json")
-	if err := os.WriteFile(path, []byte("not valid json {"), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for invalid JSON")
-	}
-}
-
-func TestLoadPersonaCaseInsensitiveExtension(t *testing.T) {
-	tests := []struct {
-		name string
-		ext  string
-	}{
-		{"lowercase yaml", ".yaml"},
-		{"uppercase YAML", ".YAML"},
-		{"mixed case Yaml", ".Yaml"},
-		{"lowercase yml", ".yml"},
-		{"uppercase YML", ".YML"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "test"+tt.ext)
-			content := "name: test\nidentity: test identity\n"
-			if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			p, err := LoadPersona(path)
-			if err != nil {
-				t.Fatalf("LoadPersona failed for extension %s: %v", tt.ext, err)
-			}
-			if p.Name != "test" {
-				t.Errorf("Name = %q, want %q", p.Name, "test")
-			}
-		})
-	}
-}
-
-func TestCapitalizeFirst(t *testing.T) {
-	tests := []struct {
-		input string
-		want  string
-	}{
-		{"hello", "Hello"},
-		{"Hello", "Hello"},
-		{"HELLO", "HELLO"},
-		{"a", "A"},
-		{"", ""},
-		{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
-		{"über", "Über"},   // German umlaut
-		{"élève", "Élève"}, // French accent
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.input, func(t *testing.T) {
-			got := CapitalizeFirst(tt.input)
-			if got != tt.want {
-				t.Errorf("CapitalizeFirst(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}
-
-func TestListBuiltinPersonasReturnsEmptySlice(t *testing.T) {
-	// ListBuiltinPersonas should return an empty slice (not nil) on error.
-	// We can't easily test the error case, but we can verify the success case
-	// returns a proper slice.
-	names := ListBuiltinPersonas()
-	if names == nil {
-		t.Error("ListBuiltinPersonas should return empty slice, not nil")
-	}
-}
-
-func TestYAMLMultilineStrings(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "multiline.yaml")
-
-	// Test literal block scalar (|) which preserves newlines
-	content := `name: multiline
-display_name: Multiline Test
-identity: |
-  First line.
-  Second line.
-  Third line.
-focus:
-  - item one
-ignore: []
-severity:
-  major: Major issue
-  minor: Minor issue
-  nit: Nit
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	// Literal block scalar preserves newlines
-	if !strings.Contains(p.Identity, "\n") {
-		t.Error("Identity should contain newlines from literal block scalar")
-	}
-	if !strings.Contains(p.Identity, "Second line") {
-		t.Error("Identity should contain 'Second line'")
-	}
-}
-
-func TestYAMLComments(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "comments.yaml")
-
-	content := `# This is a comment
-name: commented  # inline comment
-display_name: Commented Persona
-# Another comment
-identity: Test identity
-focus:
-  - item  # comment after item
-ignore: []
-severity:
-  major: Major
-  minor: Minor
-  nit: Nit
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	// Comments should be ignored
-	if p.Name != "commented" {
-		t.Errorf("Name = %q, want %q", p.Name, "commented")
-	}
-	if p.Focus[0] != "item" {
-		t.Errorf("Focus[0] = %q, want %q", p.Focus[0], "item")
-	}
-}
-
-func TestYAMLDeeplyNestedRejection(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "deeply-nested.yaml")
-
-	// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
-	// Each level adds 2 to the depth count (key + value mapping).
-	var sb strings.Builder
-	sb.WriteString("name: test\nidentity: test\nnested:\n")
-	indent := "  "
-	for i := 0; i < 25; i++ {
-		sb.WriteString(strings.Repeat(indent, i+1))
-		sb.WriteString(fmt.Sprintf("level%d:\n", i))
-	}
-	sb.WriteString(strings.Repeat(indent, 26))
-	sb.WriteString("value: too-deep\n")
-
-	if err := os.WriteFile(path, []byte(sb.String()), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for deeply nested YAML, got nil")
-	}
-	if !strings.Contains(err.Error(), "nesting depth exceeds") {
-		t.Errorf("error = %q, want containing 'nesting depth exceeds'", err.Error())
-	}
-}
-
-func TestYAMLFileSizeLimit(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "huge.yaml")
-
-	// Create a file larger than MaxPersonaFileSize (64 KB)
-	content := "name: test\nidentity: " + strings.Repeat("x", MaxPersonaFileSize+1) + "\n"
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for oversized file, got nil")
-	}
-	if !strings.Contains(err.Error(), "exceeds maximum size") {
-		t.Errorf("error = %q, want containing 'exceeds maximum size'", err.Error())
-	}
-}
-
-func TestYAMLAliasCycleDetection(t *testing.T) {
-	// Test that our checkYAMLDepth function handles alias cycles gracefully
-	// by using the seen map to prevent infinite recursion.
-	// We test this directly because go-yaml's parser handles most cycles
-	// at parse time, but we need to ensure our checker is robust.
-
-	// Create a node structure where an alias points to a parent node,
-	// simulating what could happen with malicious input that bypasses
-	// go-yaml's cycle detection.
-	parent := &yaml.Node{
-		Kind: yaml.MappingNode,
-		Content: []*yaml.Node{
-			{Kind: yaml.ScalarNode, Value: "name"},
-			{Kind: yaml.ScalarNode, Value: "test"},
-			{Kind: yaml.ScalarNode, Value: "nested"},
-		},
-	}
-
-	// Create a child that aliases back to the parent (artificial cycle)
-	aliasToParent := &yaml.Node{
-		Kind:  yaml.AliasNode,
-		Alias: parent,
-	}
-	parent.Content = append(parent.Content, aliasToParent)
-
-	nodeCount := 0
-	seen := make(map[*yaml.Node]struct{})
-
-	// This should NOT hang or stack overflow - the seen map prevents infinite recursion
-	err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
-	if err != nil {
-		t.Errorf("unexpected error traversing cyclic structure: %v", err)
-	}
-
-	// Verify we tracked the parent in the seen map
-	if _, ok := seen[parent]; !ok {
-		t.Error("parent node not tracked in seen map")
-	}
-}
-
-func TestYAMLMultiDocumentRejection(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "multi.yaml")
-
-	// Multi-document YAML (documents separated by ---)
-	content := `name: first
-identity: first document
----
-name: second
-identity: second document
-`
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for multi-document YAML, got nil")
-	}
-	if !strings.Contains(err.Error(), "multi-document") {
-		t.Errorf("error = %q, want containing 'multi-document'", err.Error())
-	}
-}
-
-func TestYAMLNodeCountLimit(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "wide.yaml")
-
-	// Build a YAML structure that's shallow but wide - many keys at the same level
-	// to test the node count limit (should exceed MaxYAMLNodes = 1000)
-	var sb strings.Builder
-	sb.WriteString("name: test\nidentity: test\n")
-	for i := 0; i < 600; i++ {
-		sb.WriteString(fmt.Sprintf("key%d: value%d\n", i, i))
-	}
-
-	if err := os.WriteFile(path, []byte(sb.String()), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for wide YAML exceeding node count, got nil")
-	}
-	if !strings.Contains(err.Error(), "node count exceeds") {
-		t.Errorf("error = %q, want containing 'node count exceeds'", err.Error())
-	}
-}
-
-func TestCheckYAMLDepthCycleDetectionDirect(t *testing.T) {
-	// Direct test of cycle detection in checkYAMLDepth by creating
-	// a node structure with an artificial cycle.
-	// This tests the seen map logic independent of go-yaml's parsing.
-	node := &yaml.Node{
-		Kind: yaml.MappingNode,
-		Content: []*yaml.Node{
-			{Kind: yaml.ScalarNode, Value: "key"},
-			{Kind: yaml.ScalarNode, Value: "value"},
-		},
-	}
-
-	// Create a cycle by making a child reference the parent
-	cycleChild := &yaml.Node{
-		Kind:  yaml.AliasNode,
-		Alias: node, // Points back to the parent
-	}
-	node.Content = append(node.Content,
-		&yaml.Node{Kind: yaml.ScalarNode, Value: "cyclic"},
-		cycleChild,
-	)
-
-	nodeCount := 0
-	seen := make(map[*yaml.Node]struct{})
-	err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
-
-	// Should complete without infinite recursion due to cycle detection
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-	// The seen map should contain multiple entries
-	if len(seen) < 2 {
-		t.Errorf("seen map has %d entries, expected at least 2", len(seen))
-	}
-}
-
-func TestListBuiltinPersonasSortedOrder(t *testing.T) {
-	names := ListBuiltinPersonas()
-	if len(names) < 2 {
-		t.Skip("need at least 2 personas to test ordering")
-	}
-
-	// Verify the list is sorted
-	for i := 1; i < len(names); i++ {
-		if names[i-1] > names[i] {
-			t.Errorf("ListBuiltinPersonas not sorted: %q > %q", names[i-1], names[i])
-		}
-	}
-}
-
-func TestYAMLUnknownFieldsRejected(t *testing.T) {
-	tests := []struct {
-		name    string
-		content string
-		wantErr string
-	}{
-		{
-			name: "unknown top-level field",
-			content: `name: test
-identity: test identity
-unknown_field: should fail
-`,
-			wantErr: "unknown_field",
-		},
-		{
-			name: "typo in field name",
-			content: `name: test
-identiy: typo should fail
-`,
-			wantErr: "identiy",
-		},
-		{
-			name: "unknown field in severity",
-			content: `name: test
-identity: test
-severity:
-  major: Major
-  minro: typo
-`,
-			wantErr: "minro",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "unknown.yaml")
-			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			_, err := LoadPersona(path)
-			if err == nil {
-				t.Errorf("expected error for unknown field %q, got nil", tt.wantErr)
-				return
-			}
-			if !strings.Contains(err.Error(), tt.wantErr) {
-				t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
-			}
-		})
-	}
-}
-
-func TestJSONUnknownFieldsRejected(t *testing.T) {
-	tests := []struct {
-		name    string
-		content string
-		wantErr string
-	}{
-		{
-			name: "unknown top-level field",
-			content: `{
-				"name": "test",
-				"identity": "test identity",
-				"unknown_field": "should fail"
-			}`,
-			wantErr: "unknown_field",
-		},
-		{
-			name: "typo in field name",
-			content: `{
-				"name": "test",
-				"identiy": "typo should fail"
-			}`,
-			wantErr: "identiy",
-		},
-		{
-			name: "unknown field in severity",
-			content: `{
-				"name": "test",
-				"identity": "test",
-				"severity": {
-					"major": "ok",
-					"miner": "typo"
-				}
-			}`,
-			wantErr: "miner",
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "test.json")
-			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			_, err := LoadPersona(path)
-			if err == nil {
-				t.Fatal("expected error for unknown field, got nil")
-			}
-			if !strings.Contains(err.Error(), tt.wantErr) {
-				t.Errorf("error = %q, want to contain %q", err.Error(), tt.wantErr)
-			}
-		})
-	}
-}
-
-func TestLoadPersonaSymlink(t *testing.T) {
-	// Create a regular persona file
-	dir := t.TempDir()
-	realFile := filepath.Join(dir, "real.yaml")
-	content := `name: test
-identity: test identity
-`
-	if err := os.WriteFile(realFile, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	// Create a symlink to it
-	symlink := filepath.Join(dir, "link.yaml")
-	if err := os.Symlink(realFile, symlink); err != nil {
-		t.Fatalf("failed to create symlink: %v", err)
-	}
-
-	// LoadPersona should work via symlink
-	p, err := LoadPersona(symlink)
-	if err != nil {
-		t.Fatalf("LoadPersona via symlink failed: %v", err)
-	}
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-}

review/personas/architect.yaml

@@ -1,37 +0,0 @@
-# Software Architect Persona
-# Focuses on design quality, patterns, and code organization
-
-name: architect
-display_name: Software Architect
-
-identity: |
-  You are a software architect reviewing code for design quality.
-
-  Your expertise:
-  - Design patterns and anti-patterns
-  - Code organization and module boundaries
-  - API design and contracts
-  - Testability and dependency injection
-  - Consistency with existing architecture
-  - Technical debt identification
-
-focus:
-  - Design pattern violations or misuse
-  - Module boundary violations (inappropriate coupling)
-  - API design issues (unclear contracts, leaky abstractions)
-  - Testability problems (hidden dependencies, god objects)
-  - Inconsistency with existing codebase patterns
-  - Unnecessary complexity or over-engineering
-  - Missing abstractions or premature abstraction
-
-ignore:
-  - Security vulnerabilities (security persona handles these)
-  - Performance micro-optimizations
-  - Code style and formatting
-  - Documentation typos
-  - Test implementation details
-
-severity:
-  major: "Architectural violations that will cause maintenance problems or make the codebase harder to evolve"
-  minor: "Design issues that reduce clarity or testability but don't block progress"
-  nit: "Minor pattern deviations or style preferences"

review/personas/docs.yaml

@@ -1,36 +0,0 @@
-# Documentation Reviewer Persona
-# Focuses on clarity, documentation quality, and self-documenting code
-
-name: docs
-display_name: Documentation Reviewer
-
-identity: |
-  You are a documentation specialist reviewing code for clarity and documentation quality.
-
-  Your expertise:
-  - API documentation and examples
-  - Code comments and their accuracy
-  - Error message clarity
-  - README and guide quality
-  - Naming clarity and self-documenting code
-
-focus:
-  - Missing or outdated documentation
-  - Unclear or misleading comments
-  - Poor error messages (cryptic, unhelpful, missing context)
-  - Confusing naming (functions, variables, types)
-  - Missing examples for complex APIs
-  - Inconsistent terminology
-  - Documentation that contradicts the code
-
-ignore:
-  - Security vulnerabilities
-  - Performance issues
-  - Design patterns
-  - Test coverage
-  - Code style (unless it affects readability)
-
-severity:
-  major: "Documentation that actively misleads or missing docs for critical functionality"
-  minor: "Unclear documentation or poor error messages that will confuse users"
-  nit: "Minor clarity improvements or typo fixes"

review/personas/security.yaml

@@ -1,37 +0,0 @@
-# Security Specialist Persona
-# Focuses on vulnerabilities, auth issues, and security best practices
-
-name: security
-display_name: Security Specialist
-
-identity: |
-  You are a security specialist reviewing code for vulnerabilities.
-
-  Your expertise:
-  - OWASP Top 10 vulnerabilities
-  - Injection attacks (SQL, command, path traversal, template)
-  - Authentication and authorization patterns
-  - Secrets management and exposure risks
-  - Race conditions with security implications
-  - Event sourcing attack vectors (replay attacks, event injection)
-
-focus:
-  - Injection attacks (SQL, command, path traversal, template injection)
-  - Authentication and authorization gaps or bypasses
-  - Secrets exposure (hardcoded credentials, tokens in logs, config leaks)
-  - Input validation failures (unsanitized input, unsafe deserialization)
-  - Race conditions that could be exploited
-  - Cryptographic weaknesses (weak algorithms, improper key handling)
-  - Information disclosure through error messages or logs
-
-ignore:
-  - Code style and naming conventions
-  - Performance optimizations (unless security-related)
-  - Documentation quality
-  - General code quality or readability
-  - Test coverage
-
-severity:
-  major: "Exploitable vulnerabilities: auth bypass, injection, data exfiltration, privilege escalation, RCE"
-  minor: "Defense-in-depth issues: missing rate limiting, verbose errors, weak input validation"
-  nit: "Theoretical risks with low exploitability or impact"

review/prompt.go

@@ -7,28 +7,6 @@ import (
 	"strings"
 )
 
-// outputSchemaJSON is the shared JSON output format specification used by both
-// the generic reviewer and persona-based reviewers.
-const outputSchemaJSON = `{
-  "verdict": "APPROVE" or "REQUEST_CHANGES",
-  "summary": "Brief overall assessment (1-3 sentences)",
-  "findings": [
-    {
-      "severity": "MAJOR" or "MINOR" or "NIT",
-      "file": "path/to/file",
-      "line": <line number from the diff>,
-      "finding": "Description of the issue"
-    }
-  ],
-  "recommendation": "Full recommendation text explaining your verdict"
-}`
-
-// verdictRules is the shared verdict determination rules.
-const verdictRules = `Rules:
-- If there are any MAJOR findings → verdict must be REQUEST_CHANGES
-- If there are no MAJOR findings → verdict should be APPROVE
-- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure`
-
 // BuildSystemBase returns the core system prompt instructions without
 // patterns or conventions. Used by the budget package to separate
 // trimmable from non-trimmable content.
@@ -45,10 +23,24 @@ func BuildSystemBase() string {
 	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
 	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
 	sb.WriteString("Output format:\n")
-	sb.WriteString(outputSchemaJSON)
-	sb.WriteString("\n\n")
-	sb.WriteString(verdictRules)
-	sb.WriteString("\n- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
+	sb.WriteString("{\n")
+	sb.WriteString("  \"verdict\": \"APPROVE\" or \"REQUEST_CHANGES\",\n")
+	sb.WriteString("  \"summary\": \"Brief overall assessment (1-3 sentences)\",\n")
+	sb.WriteString("  \"findings\": [\n")
+	sb.WriteString("    {\n")
+	sb.WriteString("      \"severity\": \"MAJOR\" or \"MINOR\" or \"NIT\",\n")
+	sb.WriteString("      \"file\": \"path/to/file\",\n")
+	sb.WriteString("      \"line\": <line number from the diff>,\n")
+	sb.WriteString("      \"finding\": \"Description of the issue\"\n")
+	sb.WriteString("    }\n")
+	sb.WriteString("  ],\n")
+	sb.WriteString("  \"recommendation\": \"Full recommendation text explaining your verdict\"\n")
+	sb.WriteString("}\n\n")
+	sb.WriteString("Rules:\n")
+	sb.WriteString("- If there are any MAJOR findings → verdict must be REQUEST_CHANGES\n")
+	sb.WriteString("- If there are no MAJOR findings → verdict should be APPROVE\n")
+	sb.WriteString("- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure\n")
+	sb.WriteString("- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
 	sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
 	sb.WriteString("- If the diff is empty or trivial (only formatting/whitespace), APPROVE with no findings.\n")
 

review/repo_persona.go

@@ -1,150 +0,0 @@
-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

@@ -1,443 +0,0 @@
-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)
-			}
-		})
-	}
-}

scripts/check-deps.sh

@@ -1,127 +0,0 @@
-#!/usr/bin/env bash
-# check-deps.sh - Enforces the strict dependency allowlist from CONVENTIONS.md
-# Exit 1 if any unapproved import is found.
-#
-# Requires: Bash 4+ (for associative arrays), Go toolchain
-# 
-# The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth.
-# Enforces Scope column: "test only" packages cannot appear in non-test code.
-
-set -euo pipefail
-
-# Check bash version
-if ((BASH_VERSINFO[0] < 4)); then
-    echo "❌ Bash 4+ required (found ${BASH_VERSION})"
-    echo "   On macOS: brew install bash"
-    exit 1
-fi
-
-CONVENTIONS_FILE="${1:-CONVENTIONS.md}"
-
-if [ ! -f "$CONVENTIONS_FILE" ]; then
-    echo "❌ CONVENTIONS.md not found"
-    exit 1
-fi
-
-# Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible)
-# Format: | `package` | use case | scope |
-declare -A ALLOWED_PROD=()
-declare -A ALLOWED_TEST=()
-
-while IFS= read -r line; do
-    # Use awk to extract package and scope from table row
-    pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}')
-    scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}')
-    
-    if [ -n "$pkg" ] && [ "$pkg" != "Package" ] && [[ "$pkg" =~ ^[a-zA-Z] ]]; then
-        if [[ "$scope" == *"test"* ]]; then
-            ALLOWED_TEST["$pkg"]=1
-        else
-            ALLOWED_PROD["$pkg"]=1
-        fi
-    fi
-done < <(grep '| `' "$CONVENTIONS_FILE" 2>/dev/null || true)
-
-ALL_ALLOWED=("${!ALLOWED_PROD[@]}" "${!ALLOWED_TEST[@]}")
-
-if [ ${#ALL_ALLOWED[@]} -eq 0 ]; then
-    echo "⚠️  No approved packages found in $CONVENTIONS_FILE"
-    echo "   (This is fine if you want stdlib-only)"
-fi
-
-# Helper: check if import matches any package in an associative array (literal prefix, no glob)
-matches_allowlist() {
-    local import="$1"
-    shift
-    local -n allowlist=$1
-    
-    for allowed in "${!allowlist[@]}"; do
-        # Exact match
-        if [ "$import" = "$allowed" ]; then
-            return 0
-        fi
-        # Literal prefix match for subpackages: must match "pkg/" exactly
-        if [ "${import#"$allowed/"}" != "$import" ]; then
-            return 0
-        fi
-    done
-    return 1
-}
-
-# Get direct module dependencies from go.mod
-DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>&1) || {
-    echo "❌ Failed to list dependencies: $DIRECT_IMPORTS"
-    exit 1
-}
-DIRECT_IMPORTS=$(echo "$DIRECT_IMPORTS" | grep -v '^$' || true)
-
-if [ -z "$DIRECT_IMPORTS" ]; then
-    echo "✅ No external dependencies"
-    exit 0
-fi
-
-# Check ALL direct dependencies are in some allowlist
-VIOLATIONS=""
-while IFS= read -r import; do
-    [ -z "$import" ] && continue
-    
-    if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then
-        VIOLATIONS="${VIOLATIONS}  - ${import} (not in allowlist)"$'\n'
-    fi
-done <<< "$DIRECT_IMPORTS"
-
-if [ -n "$VIOLATIONS" ]; then
-    echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
-    echo ""
-    echo "The following imports are not in the allowlist:"
-    printf "%s" "$VIOLATIONS"
-    echo ""
-    echo "To add a dependency, update CONVENTIONS.md (requires Aaron's approval)"
-    exit 1
-fi
-
-# Enforce Scope: test-only packages must not appear in non-test code
-# Get imports used by non-test code only (go list -deps without -test excludes test deps)
-PROD_IMPORTS=$(go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ./... 2>/dev/null || true)
-
-TEST_ONLY_IN_PROD=""
-for test_pkg in "${!ALLOWED_TEST[@]}"; do
-    # Use word-boundary matching: exact match or followed by /
-    if echo "$PROD_IMPORTS" | grep -qE "^${test_pkg}(/|\$|$)"; then
-        TEST_ONLY_IN_PROD="${TEST_ONLY_IN_PROD}  - ${test_pkg} (marked 'test only' but used in production code)"$'\n'
-    fi
-done
-
-if [ -n "$TEST_ONLY_IN_PROD" ]; then
-    echo "❌ TEST-ONLY DEPENDENCIES IN PRODUCTION CODE"
-    echo ""
-    printf "%s" "$TEST_ONLY_IN_PROD"
-    echo ""
-    echo "These packages are marked 'test only' in CONVENTIONS.md"
-    echo "and must only be imported from *_test.go files."
-    exit 1
-fi
-
-echo "✅ All dependencies are approved"
-echo "   Direct module deps: $(echo "$DIRECT_IMPORTS" | wc -l | tr -d ' ')"
-echo "   Production allowlist: ${#ALLOWED_PROD[@]}, Test-only allowlist: ${#ALLOWED_TEST[@]}"