feat(persona): add role-based review personas

Add persona system for specialized review roles. Each persona defines:
- A specific review focus (security, architecture, documentation)
- Custom system prompt additions
- Personality/tone adjustments

Built-in personas: security, architect, docs
Custom personas: load from JSON via persona-file flag

Includes workspace validation to prevent path traversal attacks.

Closes #51

.gitea/actions/review/action.yml

@@ -74,6 +74,14 @@ 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 persona JSON file with custom review focus'
+    required: false
+    default: ''
 
 runs:
   using: 'composite'
@@ -155,6 +163,8 @@ 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 }}
       run: |
         ARGS=""
         if [ "${{ inputs.dry-run }}" = "true" ]; then

.gitea/workflows/ci.yml

@@ -19,6 +19,7 @@ jobs:
       - run: go build -o review-bot ./cmd/review-bot
 
   # Self-review: builds from source since we're pre-release
+  # Models configured to match SAP AI Core deployments
   review:
     runs-on: ubuntu-24.04
     if: github.event_name == 'pull_request'
@@ -30,27 +31,12 @@ jobs:
             token_secret: SONNET_REVIEW_TOKEN
             provider: anthropic
             llm_path: /anthropic/v1
-            model: claude-sonnet-4-6
+            model: anthropic--claude-4.6-sonnet
           - name: gpt
             token_secret: GPT_REVIEW_TOKEN
             provider: openai
             llm_path: /openai/v1
             model: gpt-5
-          - name: gpt41
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-4.1
-          - name: gpt5-mini
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-5-mini
-          - name: gpt41-mini
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-4.1-mini
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
             provider: openai

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

@@ -0,0 +1,32 @@
+name: PR Ready Gate
+
+on:
+  pull_request:
+    types: [synchronize]
+
+jobs:
+  clear-labels:
+    runs-on: ubuntu-24.04
+    if: contains(github.event.pull_request.labels.*.name, 'self-reviewed')
+    steps:
+      - name: Remove self-reviewed label, reassign to author
+        env:
+          GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}
+        run: |
+          PR_NUMBER=${{ github.event.pull_request.number }}
+          AUTHOR=${{ github.event.pull_request.user.login }}
+          SELF_REVIEWED_LABEL_ID=37
+          
+          # 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 self-reviewed label and reassigned PR #${PR_NUMBER} to ${AUTHOR}"

README.md

@@ -182,6 +182,8 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
 | `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 JSON file 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 |
@@ -329,3 +331,100 @@ 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 JSON file with your domain-specific review focus:
+
+```json
+{
+  "name": "trading",
+  "display_name": "Trading Domain Expert",
+  "identity": "You are a trading systems expert reviewing code for correctness.\n\nYour expertise:\n- Order lifecycle and state machines\n- Fill handling and partial fills\n- Position tracking and P&L calculations\n- 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.json
+    ...
+```
+
+### Persona vs system-prompt-file
+
+| Feature | `persona` / `persona-file` | `system-prompt-file` |
+|---------|---------------------------|----------------------|
+| Replaces base prompt | Yes | No (appends) |
+| Structured format | Yes (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,6 +70,8 @@ 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 or anthropic")
+	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")
 
 	flag.Parse()
 
@@ -91,6 +93,36 @@ 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)
+	}
+
+	// Load persona if specified
+	var persona *review.Persona
+	if *personaName != "" {
+		var err error
+		persona, err = review.LoadBuiltinPersona(*personaName)
+		if err != nil {
+			slog.Error("failed to load persona", "persona", *personaName, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
+	} else if *personaFile != "" {
+		resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
+		if err != nil {
+			slog.Error("invalid persona-file path", "error", err)
+			os.Exit(1)
+		}
+		persona, err = review.LoadPersona(resolvedPath)
+		if err != nil {
+			slog.Error("failed to load persona file", "file", *personaFile, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
+	}
+
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
 		slog.Error("invalid reviewer name", "error", err)
@@ -201,34 +233,14 @@ func main() {
 	// Step 6b: Load additional system prompt if specified
 	additionalPrompt := ""
 	if *systemPromptFile != "" {
-		workspace := os.Getenv("GITHUB_WORKSPACE")
-		if workspace == "" {
-			workspace, _ = os.Getwd()
-		}
-		absWorkspace, err := filepath.Abs(workspace)
+		resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file")
 		if err != nil {
-			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)
+			slog.Error("invalid system-prompt-file path", "error", err)
 			os.Exit(1)
 		}
 		data, err := os.ReadFile(resolvedPath)
 		if err != nil {
-			slog.Error("failed to read system prompt file", "path", promptPath, "error", err)
+			slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err)
 			os.Exit(1)
 		}
 		additionalPrompt = string(data)
@@ -236,7 +248,13 @@ func main() {
 	}
 
 	// Step 7: Budget-aware prompt assembly
-	systemBase := review.BuildSystemBase()
+	var systemBase string
+	if persona != nil {
+		systemBase = review.BuildPersonaSystemPrompt(persona)
+		slog.Debug("using persona system prompt", "persona", persona.Name)
+	} else {
+		systemBase = review.BuildSystemBase()
+	}
 	if additionalPrompt != "" {
 		systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
 	}
@@ -254,30 +272,51 @@ func main() {
 		slog.Warn("context trimmed to fit budget", "trimmed", budgetResult.Trimmed)
 	}
 
-	// Step 8: Call LLM
+	// Step 8: Call LLM (with retry on parse failure)
 	slog.Info("sending request to LLM", "model", *llmModel)
 	messages := []llm.Message{
 		{Role: "system", Content: budgetResult.SystemPrompt},
 		{Role: "user", Content: budgetResult.UserPrompt},
 	}
 
-	response, err := llmClient.Complete(ctx, messages)
-	if err != nil {
-		slog.Error("LLM request failed", "model", *llmModel, "error", err)
-		os.Exit(1)
-	}
-	slog.Info("LLM response received", "bytes", len(response))
+	var response string
+	var result *review.ReviewResult
+	for attempt := 1; attempt <= 2; attempt++ {
+		if attempt > 1 {
+			slog.Warn("retrying LLM request after parse failure", "attempt", attempt)
+			time.Sleep(time.Second)
+		}
 
-	// Step 9: Parse response
-	result, err := review.ParseResponse(response)
-	if err != nil {
-		slog.Error("failed to parse LLM response", "error", err)
-		os.Exit(1)
+		response, err = llmClient.Complete(ctx, messages)
+		if err != nil {
+			slog.Error("LLM request failed", "model", *llmModel, "error", err, "attempt", attempt)
+			if attempt == 2 {
+				os.Exit(1)
+			}
+			continue
+		}
+		slog.Info("LLM response received", "bytes", len(response), "attempt", attempt)
+
+		// Step 9: Parse response
+		result, err = review.ParseResponse(response)
+		if err != nil {
+			slog.Error("failed to parse LLM response", "error", err, "attempt", attempt)
+			if attempt == 2 {
+				os.Exit(1)
+			}
+			continue
+		}
+		break
 	}
 	slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
 
 	// Step 10: Format and post review
-	reviewBody := review.FormatMarkdown(result, *reviewerName)
+	var reviewBody string
+	if persona != nil && persona.DisplayName != "" {
+		reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName)
+	} else {
+		reviewBody = review.FormatMarkdown(result, *reviewerName)
+	}
 
 	// Add commit footer so readers know which commit was evaluated
 	if pr.Head.Sha != "" {
@@ -299,6 +338,24 @@ func main() {
 
 	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
+	// Stale check: verify HEAD hasn't moved since we started
+	evaluatedSHA := pr.Head.Sha
+	var currentSHA string
+	currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
+	if err != nil {
+		slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
+		// currentSHA stays empty — shouldSkipStaleReview will return false
+	} else {
+		currentSHA = currentPR.Head.Sha
+	}
+	if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
+		slog.Warn("HEAD moved during review — skipping stale review",
+			"evaluated", evaluatedSHA,
+			"current", currentSHA,
+			"pr", prNumber)
+		return
+	}
+
 	// Map findings to inline comments for lines present in the diff
 	diffRanges := gitea.ParseDiffNewLines(diff)
 	var inlineComments []gitea.ReviewComment
@@ -553,6 +610,36 @@ 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
+	if !strings.HasPrefix(fullPath, absWorkspace+string(filepath.Separator)) && fullPath != absWorkspace {
+		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)
+	}
+	if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
+		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 {
@@ -650,3 +737,16 @@ func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
 	}
 	return result
 }
+
+// shouldSkipStaleReview reports whether to skip posting because HEAD moved.
+// Returns true (skip) if evaluatedSHA differs from currentSHA.
+// Returns false (don't skip) if:
+//   - SHAs match (no movement)
+//   - currentSHA is empty (re-fetch failed; prefer posting stale over failing)
+func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
+	if currentSHA == "" {
+		// Re-fetch failed; better to post potentially stale than fail
+		return false
+	}
+	return evaluatedSHA != currentSHA
+}

cmd/review-bot/main_test.go

@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/exec"
 	"strings"
+	"path/filepath"
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/gitea"
@@ -45,6 +46,113 @@ 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 gets normalized to relative",
+			workspace: tmpDir,
+			path:      "/etc/passwd",
+			wantErr:   true,
+			errMatch:  "failed to resolve", // filepath.Join strips leading / making it <workspace>/etc/passwd which doesn't exist
+		},
+		{
+			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,
@@ -862,3 +970,53 @@ func TestFindAllOwnReviews(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldSkipStaleReview(t *testing.T) {
+	tests := []struct {
+		name         string
+		evaluatedSHA string
+		currentSHA   string
+		wantSkip     bool
+	}{
+		{
+			name:         "matching SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "abc123def456",
+			wantSkip:     false,
+		},
+		{
+			name:         "different SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "xyz789abc123",
+			wantSkip:     true,
+		},
+		{
+			name:         "empty current SHA (re-fetch failed)",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "both empty (edge case)",
+			evaluatedSHA: "",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "only current empty",
+			evaluatedSHA: "abc123",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := shouldSkipStaleReview(tc.evaluatedSHA, tc.currentSHA)
+			if got != tc.wantSkip {
+				t.Errorf("shouldSkipStaleReview(%q, %q) = %v, want %v",
+					tc.evaluatedSHA, tc.currentSHA, got, tc.wantSkip)
+			}
+		})
+	}
+}

docs/DESIGN-51-personas.md

@@ -0,0 +1,353 @@
+# Design: Role-based Review Personas (Issue #51)
+
+## 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 YAML 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
+
+### 2. Persona File Format
+
+```yaml
+# .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 YAML 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: JSON Instead of YAML
+
+**Reason:** Project convention is "Go standard library only — no external dependencies."
+
+YAML requires `gopkg.in/yaml.v3` or similar. To maintain zero dependencies, persona files will use JSON instead.
+
+### Updated Persona File Format
+
+```json
+{
+  "name": "security",
+  "display_name": "Security Specialist",
+  "model_preference": "opus",
+  "identity": "You are a security specialist reviewing code for vulnerabilities.\nYour expertise: OWASP Top 10, injection attacks, auth/authz, secrets management.",
+  "focus": [
+    "Injection attacks (SQL, command, path traversal, template)",
+    "Authentication and authorization gaps",
+    "Secrets exposure (hardcoded credentials, tokens in logs)"
+  ],
+  "ignore": [
+    "Code style and naming conventions",
+    "Performance (unless security-related)",
+    "Documentation"
+  ],
+  "severity": {
+    "major": "Privilege escalation, information disclosure, DoS",
+    "minor": "Missing rate limiting, verbose errors",
+    "nit": "Theoretical risk with low exploitability"
+  }
+}
+```
+
+This maintains all the same fields but uses JSON encoding, which Go handles natively via `encoding/json`.

llm/client.go

@@ -75,12 +75,52 @@ type Message struct {
 // Complete sends a chat completion request and returns the assistant's response content.
 // The first message with role "system" is treated as the system prompt.
 func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
-	switch c.provider {
-	case ProviderAnthropic:
-		return c.completeAnthropic(ctx, messages)
-	default:
-		return c.completeOpenAI(ctx, messages)
+	var result string
+	var err error
+
+	for attempt := 0; attempt < 2; attempt++ {
+		switch c.provider {
+		case ProviderAnthropic:
+			result, err = c.completeAnthropic(ctx, messages)
+		default:
+			result, err = c.completeOpenAI(ctx, messages)
+		}
+
+		if err == nil {
+			return result, nil
+		}
+
+		// Only retry on response body read errors (transient network issues).
+		// Do not retry on context cancellation, status errors, or parse errors
+		// that indicate a structural API problem.
+		if !isRetryableError(err) {
+			return "", err
+		}
+
+		if attempt == 0 && ctx.Err() == nil {
+			// Brief pause before retry to allow transient issues to resolve.
+			time.Sleep(500 * time.Millisecond)
+		}
 	}
+
+	return "", err
+}
+
+// isRetryableError returns true for transient errors worth retrying.
+func isRetryableError(err error) bool {
+	if err == nil {
+		return false
+	}
+	s := err.Error()
+	// Body read failures (connection reset, truncation)
+	if strings.Contains(s, "read response") {
+		return true
+	}
+	// Unexpected body length (our content-length validation)
+	if strings.Contains(s, "body length mismatch") {
+		return true
+	}
+	return false
 }
 
 // --- OpenAI-compatible implementation ---
@@ -231,6 +271,12 @@ func (c *Client) doRequest(req *http.Request, parse func([]byte) (string, error)
 		return "", fmt.Errorf("read response: %w", err)
 	}
 
+	// Validate body length against Content-Length header when present.
+	// A mismatch indicates the response was truncated in transit.
+	if cl := resp.ContentLength; cl > 0 && int64(len(body)) < cl {
+		return "", fmt.Errorf("body length mismatch: Content-Length=%d, received=%d", cl, len(body))
+	}
+
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		return "", fmt.Errorf("LLM API error (status %d): %s", resp.StatusCode, string(body))
 	}

llm/client_test.go

@@ -3,6 +3,7 @@ package llm
 import (
 	"context"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -295,3 +296,131 @@ func TestWithProvider(t *testing.T) {
 		t.Errorf("expected provider anthropic, got %s", client.provider)
 	}
 }
+
+func TestComplete_RetryOnBodyReadError(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		if attempts == 1 {
+			// First attempt: send headers then close connection abruptly
+			// Simulate by writing partial response and flushing with wrong Content-Length
+			w.Header().Set("Content-Length", "1000")
+			w.WriteHeader(http.StatusOK)
+			w.Write([]byte(`{"choices":[{"message":{"con`))
+			// The test HTTP server will close the connection after handler returns,
+			// but Content-Length mismatch means client gets fewer bytes than expected
+			return
+		}
+		// Second attempt: succeed
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(ChatResponse{
+			Choices: []struct {
+				Message struct {
+					Content string `json:"content"`
+				} `json:"message"`
+			}{{Message: struct {
+				Content string `json:"content"`
+			}{Content: "success"}}},
+		})
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "key", "model")
+	got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
+	if err != nil {
+		t.Fatalf("expected retry to succeed, got error: %v", err)
+	}
+	if got != "success" {
+		t.Errorf("expected %q, got %q", "success", got)
+	}
+	if attempts != 2 {
+		t.Errorf("expected 2 attempts, got %d", attempts)
+	}
+}
+
+func TestComplete_ContentLengthMismatch(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		if attempts == 1 {
+			// Claim Content-Length is larger than actual body
+			w.Header().Set("Content-Length", "500")
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			// Write less than 500 bytes
+			w.Write([]byte(`{"choices":[{"message":{"content":"partial"}}]}`))
+			return
+		}
+		// Second attempt succeeds
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(ChatResponse{
+			Choices: []struct {
+				Message struct {
+					Content string `json:"content"`
+				} `json:"message"`
+			}{{Message: struct {
+				Content string `json:"content"`
+			}{Content: "complete"}}},
+		})
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "key", "model")
+	got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
+	if err != nil {
+		t.Fatalf("expected retry to succeed on content-length mismatch, got: %v", err)
+	}
+	if got != "complete" {
+		t.Errorf("expected %q, got %q", "complete", got)
+	}
+}
+
+func TestComplete_NoRetryOnAPIError(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusBadRequest)
+		w.Write([]byte(`{"error":"bad request"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "key", "model")
+	_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
+	if err == nil {
+		t.Fatal("expected error for 400, got nil")
+	}
+	if attempts != 1 {
+		t.Errorf("should not retry on API errors, got %d attempts", attempts)
+	}
+}
+
+func TestIsRetryableError(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      string
+		expected bool
+	}{
+		{"nil formatted", "", false},
+		{"read response error", "read response: unexpected EOF", true},
+		{"body length mismatch", "body length mismatch: Content-Length=1000, received=500", true},
+		{"API error", "LLM API error (status 400): bad request", false},
+		{"parse error", "parse response: unexpected end of JSON input", false},
+		{"request error", "LLM request: connection refused", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.err == "" {
+				if isRetryableError(nil) {
+					t.Error("nil error should not be retryable")
+				}
+				return
+			}
+			err := fmt.Errorf("%s", tt.err)
+			got := isRetryableError(err)
+			if got != tt.expected {
+				t.Errorf("isRetryableError(%q) = %v, want %v", tt.err, got, tt.expected)
+			}
+		})
+	}
+}

review/formatter.go

@@ -53,3 +53,48 @@ func GiteaEvent(verdict string) string {
 		return "COMMENT"
 	}
 }
+
+// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
+// 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 := strings.ToUpper(headerName[:1]) + headerName[1:]
+		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
+	}
+
+	sb.WriteString("## Summary\n\n")
+	sb.WriteString(result.Summary)
+	sb.WriteString("\n\n")
+
+	if len(result.Findings) > 0 {
+		sb.WriteString("## Findings\n\n")
+		sb.WriteString("| # | Severity | File | Line | Finding |\n")
+		sb.WriteString("|---|----------|------|------|--------|\n")
+
+		for i, f := range result.Findings {
+			sb.WriteString(fmt.Sprintf("| %d | [%s] | `%s` | %d | %s |\n",
+				i+1, f.Severity, f.File, f.Line, f.Finding))
+		}
+		sb.WriteString("\n")
+	}
+
+	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))
+		// Hidden sentinel for identifying this bot's reviews during cleanup
+		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", sentinelName))
+	}
+
+	return sb.String()
+}

review/formatter_test.go

@@ -159,3 +159,58 @@ 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/parser.go

@@ -29,7 +29,19 @@ func ParseResponse(response string) (*ReviewResult, error) {
 
 	var result ReviewResult
 	if err := json.Unmarshal([]byte(cleaned), &result); err != nil {
-		return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
+		// LLMs sometimes produce JSON with unescaped quotes inside string values.
+		// Try to repair before giving up.
+		repaired := repairJSON(cleaned)
+		if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {
+			// Include diagnostic info: lengths help identify truncation
+			rawLen := len(response)
+			cleanedLen := len(cleaned)
+			preview := cleaned
+			if len(preview) > 200 {
+				preview = preview[:100] + "..." + preview[len(preview)-100:]
+			}
+			return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw length: %d, cleaned length: %d\nCleaned preview: %s", err, rawLen, cleanedLen, preview)
+		}
 	}
 
 	// Validate verdict
@@ -74,3 +86,230 @@ func extractJSON(s string) string {
 	s = strings.TrimSpace(s)
 	return s
 }
+
+// repairJSON attempts to fix common LLM JSON issues:
+// - Unescaped double quotes inside string values
+//
+// Strategy: walk the JSON structurally. Object keys are parsed normally (LLMs
+// get those right). For string VALUES, we find all candidate closing quotes and
+// pick the LAST one that leaves valid JSON structure afterward — maximizing
+// string content, which is the correct bias for the "LLM put unescaped quotes
+// in a string value" failure mode.
+func repairJSON(s string) string {
+	runes := []rune(s)
+	var out strings.Builder
+	out.Grow(len(s) + 64)
+
+	i := 0
+	for i < len(runes) {
+		c := runes[i]
+
+		if c != '"' {
+			out.WriteRune(c)
+			i++
+			continue
+		}
+
+		// We hit an opening quote. Determine if this is a key or a value.
+		// Keys: the standard JSON parser in LLMs gets keys right, so we parse
+		// them normally (first unescaped quote closes).
+		// Values: may contain unescaped quotes — use the repair heuristic.
+		isValue := isValuePosition(runes, i)
+
+		if !isValue {
+			// Parse key/simple string normally
+			out.WriteRune('"')
+			i++
+			for i < len(runes) {
+				ch := runes[i]
+				if ch == '\\' && i+1 < len(runes) {
+					out.WriteRune(ch)
+					i++
+					out.WriteRune(runes[i])
+					i++
+					continue
+				}
+				if ch == '"' {
+					out.WriteRune('"')
+					i++
+					break
+				}
+				out.WriteRune(ch)
+				i++
+			}
+			continue
+		}
+
+		// Value string — find the correct close using last-valid-candidate heuristic
+		out.WriteRune('"')
+		i++
+
+		closeIdx := findClosingQuote(runes, i)
+
+		// Write everything between open and close, escaping interior quotes
+		for j := i; j < closeIdx; j++ {
+			ch := runes[j]
+			if ch == '\\' && j+1 < closeIdx {
+				// Already-escaped sequence — pass through
+				out.WriteRune(ch)
+				j++
+				out.WriteRune(runes[j])
+			} else if ch == '"' {
+				out.WriteRune('\\')
+				out.WriteRune('"')
+			} else {
+				out.WriteRune(ch)
+			}
+		}
+
+		// Write the closing quote
+		out.WriteRune('"')
+		i = closeIdx + 1
+	}
+
+	return out.String()
+}
+
+// isValuePosition determines if the quote at position i is opening a JSON value
+// string (as opposed to an object key). We only apply repair to values that
+// follow ':' since those are the free-text fields where LLMs produce unescaped
+// quotes. Array elements and keys are left alone (parsed normally).
+func isValuePosition(runes []rune, i int) bool {
+	// Look backward, skipping whitespace, for the preceding structural char
+	j := i - 1
+	for j >= 0 && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
+		j--
+	}
+	if j < 0 {
+		return false
+	}
+	// After ':' → definitely a value
+	return runes[j] == ':'
+}
+
+// findClosingQuote finds the index of the true closing quote for a JSON string
+// value starting at position start (the character after the opening quote).
+// It collects all unescaped quote candidates and returns the FIRST one that
+// produces valid JSON continuation (deeper lookahead verifies the next token).
+func findClosingQuote(runes []rune, start int) int {
+	// Collect all candidate positions for the closing quote.
+	var candidates []int
+	for j := start; j < len(runes); j++ {
+		if runes[j] == '\\' {
+			j++ // skip escaped character
+			continue
+		}
+		if runes[j] == '"' {
+			candidates = append(candidates, j)
+		}
+	}
+
+	if len(candidates) == 0 {
+		return len(runes)
+	}
+
+	if len(candidates) == 1 {
+		return candidates[0]
+	}
+
+	// Try candidates from FIRST to LAST. The correct closing quote is the
+	// earliest one that produces valid JSON structure after it (verified by
+	// deeper lookahead that checks the next token is a valid JSON start).
+	for _, idx := range candidates {
+		if isValidJSONAfterClose(runes, idx+1) {
+			return idx
+		}
+	}
+
+	// Fallback: return the last candidate
+	return candidates[len(candidates)-1]
+}
+
+// isValidJSONAfterClose checks whether the runes after a candidate closing quote
+// look like valid JSON continuation for a VALUE string. Since we only use this
+// for value positions, ':' is NOT a valid continuation (values are never keys).
+// Checks deeper structure to avoid being fooled by JSON-like content in strings.
+func isValidJSONAfterClose(runes []rune, pos int) bool {
+	j := pos
+	for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
+		j++
+	}
+
+	if j >= len(runes) {
+		return true
+	}
+
+	next := runes[j]
+	if next == '}' || next == ']' {
+		// Closing a container. Verify what follows the close is also valid:
+		// another structural char, comma, or EOF.
+		return isValidAfterContainerClose(runes, j+1)
+	}
+	if next == ',' {
+		// After comma, must be followed by a valid JSON token
+		j++
+		for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
+			j++
+		}
+		if j >= len(runes) {
+			return false // trailing comma with nothing after — invalid
+		}
+		return isJSONTokenStart(runes, j)
+	}
+	// ':' is NOT valid here — we're in a value position, not a key.
+	// Any other character is also invalid.
+	return false
+}
+
+// isValidAfterContainerClose checks that after a } or ], the continuation is
+// structurally valid: more closes, comma+token, or EOF.
+func isValidAfterContainerClose(runes []rune, pos int) bool {
+	j := pos
+	for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
+		j++
+	}
+	if j >= len(runes) {
+		return true
+	}
+	next := runes[j]
+	if next == '}' || next == ']' {
+		return isValidAfterContainerClose(runes, j+1)
+	}
+	if next == ',' {
+		j++
+		for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
+			j++
+		}
+		if j >= len(runes) {
+			return false
+		}
+		return isJSONTokenStart(runes, j)
+	}
+	return false
+}
+
+// isJSONTokenStart returns true if the rune could begin a JSON value or key.
+// For keywords (true/false/null), verifies the full keyword is present.
+func isJSONTokenStart(runes []rune, pos int) bool {
+	if pos >= len(runes) {
+		return false
+	}
+	r := runes[pos]
+	switch {
+	case r == '"': // string
+		return true
+	case r == '{' || r == '[': // object or array
+		return true
+	case r == 't': // true
+		return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "true"
+	case r == 'f': // false
+		return pos+5 <= len(runes) && string(runes[pos:pos+5]) == "false"
+	case r == 'n': // null
+		return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "null"
+	case r >= '0' && r <= '9': // number
+		return true
+	case r == '-': // negative number
+		return true
+	}
+	return false
+}

review/parser_test.go

@@ -1,6 +1,7 @@
 package review
 
 import (
+	"encoding/json"
 	"testing"
 )
 
@@ -112,3 +113,112 @@ func TestParseResponse_MarkdownFencesNoLang(t *testing.T) {
 		t.Errorf("expected APPROVE, got %q", result.Verdict)
 	}
 }
+
+func TestParseResponse_UnescapedQuotesInStrings(t *testing.T) {
+	// Real failure from CI: Sonnet puts unescaped quotes like (e.g. "28") in findings
+	input := `{"verdict": "APPROVE", "summary": "Clean PR", "findings": [{"severity": "NIT", "file": "ci/Dockerfile", "line": 14, "finding": "The comment says OTP_VERSION is the major version (e.g. \"28\") but it actually contains unescaped quotes like (e.g. "28") which breaks JSON"}], "recommendation": "Ship it"}`
+
+	result, err := ParseResponse(input)
+	if err != nil {
+		t.Fatalf("expected repair to handle unescaped quotes, got error: %v", err)
+	}
+	if result.Verdict != "APPROVE" {
+		t.Errorf("expected APPROVE, got %q", result.Verdict)
+	}
+	if len(result.Findings) != 1 {
+		t.Fatalf("expected 1 finding, got %d", len(result.Findings))
+	}
+}
+
+func TestRepairJSON_NoOpOnValid(t *testing.T) {
+	valid := `{"key": "value", "num": 42}`
+	result := repairJSON(valid)
+	if result != valid {
+		t.Errorf("repairJSON should not modify valid JSON\n  got:  %s\n  want: %s", result, valid)
+	}
+}
+
+func TestRepairJSON_FixesUnescapedQuotes(t *testing.T) {
+	// Interior quote followed by non-structural character
+	input := `{"msg": "use "foo" here"}`
+	result := repairJSON(input)
+
+	// Should be parseable now
+	var m map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &m); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+}
+
+func TestRepairJSON_InteriorQuoteBeforeComma(t *testing.T) {
+	// Bug reported by reviewer: interior quoted word immediately before a comma
+	input := `{"msg": "say "yes", and go"}`
+	result := repairJSON(input)
+
+	var m map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &m); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+	// The full string content should be preserved
+	msg, ok := m["msg"].(string)
+	if !ok {
+		t.Fatal("msg field missing or not a string")
+	}
+	if msg != `say "yes", and go` {
+		t.Errorf("unexpected msg content: %q", msg)
+	}
+}
+
+func TestRepairJSON_InteriorQuoteBeforeCloseBrace(t *testing.T) {
+	// Bug reported by reviewer: JSON-shaped syntax inside string values
+	input := `{"msg": "input map {"key": "val"} caused error"}`
+	result := repairJSON(input)
+
+	var m map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &m); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+}
+
+func TestRepairJSON_MultipleFields(t *testing.T) {
+	// Multiple string fields with unescaped quotes in different positions
+	input := `{"a": "hello "world"", "b": "foo"}`
+	result := repairJSON(input)
+
+	var m map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &m); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+	if _, ok := m["b"]; !ok {
+		t.Error("expected 'b' field to be preserved")
+	}
+}
+
+func TestRepairJSON_PreservesEscapedQuotes(t *testing.T) {
+	// Already-escaped quotes should not be double-escaped
+	input := `{"msg": "already \"escaped\" here"}`
+	result := repairJSON(input)
+
+	if result != input {
+		t.Errorf("repairJSON should not modify already-escaped quotes\n  got:  %s\n  want: %s", result, input)
+	}
+
+	var m map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &m); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+}
+
+func TestRepairJSON_ComplexNestedContent(t *testing.T) {
+	// Combines both reviewer bugs: quoted words before commas AND JSON-like content
+	input := `{"verdict": "APPROVE", "findings": [{"finding": "The map {"key": "val"} and (e.g. "28") and say "yes", then stop"}]}`
+	result := repairJSON(input)
+
+	var parsed map[string]interface{}
+	if err := json.Unmarshal([]byte(result), &parsed); err != nil {
+		t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
+	}
+	if parsed["verdict"] != "APPROVE" {
+		t.Errorf("expected verdict APPROVE, got %v", parsed["verdict"])
+	}
+}

review/persona.go

@@ -0,0 +1,98 @@
+package review
+
+import (
+	"embed"
+	"encoding/json"
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+)
+
+//go:embed personas/*.json
+var embeddedPersonas embed.FS
+
+// Persona defines a specialized review role with focused expertise.
+type Persona struct {
+	Name         string   `json:"name"`
+	DisplayName  string   `json:"display_name"`
+	ModelPref    string   `json:"model_preference,omitempty"`
+	Identity     string   `json:"identity"`
+	Focus        []string `json:"focus"`
+	Ignore       []string `json:"ignore"`
+	Severity     Severity `json:"severity"`
+	OutputFormat string   `json:"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"`
+	Minor string `json:"minor"`
+	Nit   string `json:"nit"`
+}
+
+// LoadPersona loads a persona from a file path.
+func LoadPersona(path string) (*Persona, error) {
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return nil, fmt.Errorf("read persona file %s: %w", path, err)
+	}
+	return parsePersona(data, path)
+}
+
+// LoadBuiltinPersona loads a built-in persona by name.
+// Returns an error if the persona doesn't exist.
+func LoadBuiltinPersona(name string) (*Persona, error) {
+	filename := name + ".json"
+	data, err := embeddedPersonas.ReadFile(filepath.Join("personas", filename))
+	if err != nil {
+		available := ListBuiltinPersonas()
+		return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
+	}
+	return parsePersona(data, "builtin:"+name)
+}
+
+// ListBuiltinPersonas returns the names of all built-in personas.
+func ListBuiltinPersonas() []string {
+	entries, err := embeddedPersonas.ReadDir("personas")
+	if err != nil {
+		return nil
+	}
+	var names []string
+	for _, e := range entries {
+		if e.IsDir() {
+			continue
+		}
+		name := e.Name()
+		if strings.HasSuffix(name, ".json") {
+			names = append(names, strings.TrimSuffix(name, ".json"))
+		}
+	}
+	return names
+}
+
+func parsePersona(data []byte, source string) (*Persona, error) {
+	var p Persona
+	if err := json.Unmarshal(data, &p); 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
+}
+
+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
+}

review/persona_prompt.go

@@ -0,0 +1,118 @@
+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 (same as base, but with persona context)
+	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("{\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("- 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

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

@@ -0,0 +1,211 @@
+package review
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+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: "Architecture Reviewer",
+		},
+		{
+			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 TestLoadPersonaFromFile(t *testing.T) {
+	// Create a temp persona file
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.json")
+
+	content := `{
+		"name": "test",
+		"display_name": "Test Persona",
+		"identity": "You are a test persona.",
+		"focus": ["testing"],
+		"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")
+	}
+}
+
+func TestLoadPersonaValidation(t *testing.T) {
+	tests := []struct {
+		name    string
+		json    string
+		wantErr string
+	}{
+		{
+			name:    "missing name",
+			json:    `{"identity": "test"}`,
+			wantErr: "name is required",
+		},
+		{
+			name:    "missing identity",
+			json:    `{"name": "test"}`,
+			wantErr: "identity is required",
+		},
+		{
+			name: "display_name defaults to name",
+			json: `{"name": "test", "identity": "test identity"}`,
+			// 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.json")
+			if err := os.WriteFile(path, []byte(tt.json), 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 !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.json")
+	if err == nil {
+		t.Error("expected error for nonexistent file")
+	}
+}
+
+func TestLoadPersonaInvalidJSON(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "invalid.json")
+	if err := os.WriteFile(path, []byte("not 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 contains(s, substr string) bool {
+	return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsHelper(s, substr))
+}
+
+func containsHelper(s, substr string) bool {
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}

review/personas/architect.json

@@ -0,0 +1,25 @@
+{
+  "name": "architect",
+  "display_name": "Architecture Reviewer",
+  "identity": "You are an architecture reviewer focused on design patterns, code organization, and maintainability.\n\nYour expertise:\n- Design patterns and their appropriate application\n- Code organization and module boundaries\n- API design and contracts\n- Error handling patterns\n- Concurrency patterns and safety\n- Testing patterns and testability",
+  "focus": [
+    "Design pattern violations or misapplications",
+    "Module boundary violations and improper coupling",
+    "API contract clarity and consistency",
+    "Error handling completeness and patterns",
+    "Concurrency safety and patterns",
+    "Testability and dependency injection",
+    "Separation of concerns"
+  ],
+  "ignore": [
+    "Security vulnerabilities (handled by security persona)",
+    "Performance micro-optimizations",
+    "Minor style preferences",
+    "Documentation formatting"
+  ],
+  "severity": {
+    "major": "Design issues that will cause maintenance burden or bugs: tight coupling, missing abstractions, broken contracts",
+    "minor": "Suboptimal patterns that could be improved: redundant code, unclear boundaries",
+    "nit": "Style suggestions that improve consistency but don't affect correctness"
+  }
+}

review/personas/docs.json

@@ -0,0 +1,24 @@
+{
+  "name": "docs",
+  "display_name": "Documentation Reviewer",
+  "identity": "You are a documentation reviewer focused on API clarity, code comments, and user-facing documentation.\n\nYour expertise:\n- API documentation completeness\n- Code comment quality and accuracy\n- README and user guide clarity\n- Example code correctness\n- Error message helpfulness",
+  "focus": [
+    "Missing or outdated API documentation",
+    "Misleading or incorrect code comments",
+    "Unclear error messages",
+    "Missing or incorrect examples",
+    "README accuracy and completeness",
+    "Public API ergonomics and naming"
+  ],
+  "ignore": [
+    "Implementation details (unless they affect the public API)",
+    "Performance",
+    "Security (handled by security persona)",
+    "Internal code organization"
+  ],
+  "severity": {
+    "major": "Misleading documentation that will cause users to make mistakes",
+    "minor": "Missing documentation for public APIs",
+    "nit": "Minor wording improvements or formatting"
+  }
+}

review/personas/security.json

@@ -0,0 +1,26 @@
+{
+  "name": "security",
+  "display_name": "Security Specialist",
+  "identity": "You are a security specialist reviewing code for vulnerabilities.\n\nYour expertise:\n- OWASP Top 10 vulnerabilities\n- Injection attacks (SQL, command, path traversal, template)\n- Authentication and authorization patterns\n- Secrets management and exposure risks\n- Race conditions with security implications\n- 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"
+  }
+}