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

README.md

@@ -377,33 +377,41 @@ jobs:
 
 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:
+Create a YAML 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"
-  }
-}
+```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:
@@ -412,16 +420,18 @@ Use it in CI:
 - uses: rodin/review-bot/.gitea/actions/review@v1
   with:
     reviewer-name: trading
-    persona-file: .review/personas/trading.json
+    persona-file: .review/personas/trading.yaml
     ...
 ```
 
+JSON format is also supported for backwards compatibility.
+
 ### Persona vs system-prompt-file
 
 | Feature | `persona` / `persona-file` | `system-prompt-file` |
 |---------|---------------------------|----------------------|
 | Replaces base prompt | Yes | No (appends) |
-| Structured format | Yes (JSON) | No (freeform) |
+| Structured format | Yes (YAML/JSON) | No (freeform) |
 | Focus/ignore lists | Yes | Manual |
 | Severity calibration | Yes | Manual |
 | Header display name | Yes | No |

go.mod

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

go.sum

@@ -0,0 +1,3 @@
+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/persona.go

@@ -7,32 +7,35 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+
+	"gopkg.in/yaml.v3"
 )
 
-//go:embed personas/*.json
+//go:embed personas/*.yaml
 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"`
+	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"`
-	Minor string `json:"minor"`
-	Nit   string `json:"nit"`
+	Major string `json:"major" yaml:"major"`
+	Minor string `json:"minor" yaml:"minor"`
+	Nit   string `json:"nit" yaml:"nit"`
 }
 
 // LoadPersona loads a persona from a file path.
+// Supports both YAML (.yaml, .yml) and JSON (.json) formats.
 func LoadPersona(path string) (*Persona, error) {
 	data, err := os.ReadFile(path)
 	if err != nil {
@@ -44,7 +47,7 @@ func LoadPersona(path string) (*Persona, error) {
 // 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"
+	filename := name + ".yaml"
 	data, err := embeddedPersonas.ReadFile(filepath.Join("personas", filename))
 	if err != nil {
 		available := ListBuiltinPersonas()
@@ -65,8 +68,10 @@ func ListBuiltinPersonas() []string {
 			continue
 		}
 		name := e.Name()
-		if strings.HasSuffix(name, ".json") {
-			names = append(names, strings.TrimSuffix(name, ".json"))
+		if strings.HasSuffix(name, ".yaml") {
+			names = append(names, strings.TrimSuffix(name, ".yaml"))
+		} else if strings.HasSuffix(name, ".yml") {
+			names = append(names, strings.TrimSuffix(name, ".yml"))
 		}
 	}
 	return names
@@ -74,9 +79,20 @@ func ListBuiltinPersonas() []string {
 
 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)
+	
+	// Determine format by extension or try YAML first (it's a superset of JSON)
+	ext := strings.ToLower(filepath.Ext(source))
+	if ext == ".json" {
+		if err := json.Unmarshal(data, &p); err != nil {
+			return nil, fmt.Errorf("parse persona %s: %w", source, err)
+		}
+	} else {
+		// YAML (also handles .yaml, .yml, and builtin: prefix)
+		if err := yaml.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
 	}

review/persona_test.go

@@ -3,6 +3,7 @@ package review
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 )
 
@@ -23,7 +24,7 @@ func TestLoadBuiltinPersona(t *testing.T) {
 			name:        "architect persona",
 			personaName: "architect",
 			wantErr:     false,
-			wantDisplay: "Architecture Reviewer",
+			wantDisplay: "Software Architect",
 		},
 		{
 			name:        "docs persona",
@@ -86,8 +87,51 @@ func TestListBuiltinPersonas(t *testing.T) {
 	}
 }
 
-func TestLoadPersonaFromFile(t *testing.T) {
-	// Create a temp persona file
+func TestLoadPersonaFromYAMLFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.yaml")
+
+	content := `
+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 TestLoadPersonaFromJSONFile(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "test.json")
 
@@ -124,22 +168,22 @@ func TestLoadPersonaFromFile(t *testing.T) {
 func TestLoadPersonaValidation(t *testing.T) {
 	tests := []struct {
 		name    string
-		json    string
+		yaml    string
 		wantErr string
 	}{
 		{
 			name:    "missing name",
-			json:    `{"identity": "test"}`,
+			yaml:    "identity: test",
 			wantErr: "name is required",
 		},
 		{
 			name:    "missing identity",
-			json:    `{"name": "test"}`,
+			yaml:    "name: test",
 			wantErr: "identity is required",
 		},
 		{
 			name: "display_name defaults to name",
-			json: `{"name": "test", "identity": "test identity"}`,
+			yaml: "name: test\nidentity: test identity",
 			// No error expected - should succeed
 		},
 	}
@@ -147,8 +191,8 @@ func TestLoadPersonaValidation(t *testing.T) {
 	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 {
+			path := filepath.Join(dir, "test.yaml")
+			if err := os.WriteFile(path, []byte(tt.yaml), 0644); err != nil {
 				t.Fatalf("failed to write test file: %v", err)
 			}
 
@@ -158,7 +202,7 @@ func TestLoadPersonaValidation(t *testing.T) {
 					t.Errorf("expected error containing %q, got nil", tt.wantErr)
 					return
 				}
-				if !contains(err.Error(), tt.wantErr) {
+				if !strings.Contains(err.Error(), tt.wantErr) {
 					t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
 				}
 				return
@@ -178,34 +222,21 @@ func TestLoadPersonaValidation(t *testing.T) {
 }
 
 func TestLoadPersonaFileNotFound(t *testing.T) {
-	_, err := LoadPersona("/nonexistent/path/persona.json")
+	_, err := LoadPersona("/nonexistent/path/persona.yaml")
 	if err == nil {
 		t.Error("expected error for nonexistent file")
 	}
 }
 
-func TestLoadPersonaInvalidJSON(t *testing.T) {
+func TestLoadPersonaInvalidYAML(t *testing.T) {
 	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.json")
-	if err := os.WriteFile(path, []byte("not json"), 0644); err != nil {
+	path := filepath.Join(dir, "invalid.yaml")
+	if err := os.WriteFile(path, []byte("not: valid: yaml: here"), 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")
+		t.Error("expected error for invalid YAML")
 	}
 }
-
-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

@@ -1,25 +0,0 @@
-{
-  "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/architect.yaml

@@ -0,0 +1,34 @@
+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.json

@@ -1,24 +0,0 @@
-{
-  "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/docs.yaml

@@ -0,0 +1,33 @@
+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.json

@@ -1,26 +0,0 @@
-{
-  "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"
-  }
-}

review/personas/security.yaml

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