feat(persona): add role-based review personas
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 9m33s
CI / review (/anthropic/v1, anthropic--claude-4.6-sonnet, sonnet, anthropic, SONNET_REVIEW_TOKEN) (pull_request) Successful in 10m0s
CI / review (/openai/v1, gpt-5, security, openai, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m47s
CI / review (/openai/v1, gpt-5, gpt, openai, GPT_REVIEW_TOKEN) (pull_request) Successful in 11m34s
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 9m33s
CI / review (/anthropic/v1, anthropic--claude-4.6-sonnet, sonnet, anthropic, SONNET_REVIEW_TOKEN) (pull_request) Successful in 10m0s
CI / review (/openai/v1, gpt-5, security, openai, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m47s
CI / review (/openai/v1, gpt-5, gpt, openai, GPT_REVIEW_TOKEN) (pull_request) Successful in 11m34s
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
This commit is contained in:
@@ -0,0 +1,242 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"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: "Software Architect",
|
||||
},
|
||||
{
|
||||
name: "docs persona",
|
||||
personaName: "docs",
|
||||
wantErr: false,
|
||||
wantDisplay: "Documentation Reviewer",
|
||||
},
|
||||
{
|
||||
name: "unknown persona",
|
||||
personaName: "nonexistent",
|
||||
wantErr: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
p, err := LoadBuiltinPersona(tt.personaName)
|
||||
if tt.wantErr {
|
||||
if err == nil {
|
||||
t.Error("expected error, got nil")
|
||||
}
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if p.Name != tt.personaName {
|
||||
t.Errorf("Name = %q, want %q", p.Name, tt.personaName)
|
||||
}
|
||||
if p.DisplayName != tt.wantDisplay {
|
||||
t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplay)
|
||||
}
|
||||
if p.Identity == "" {
|
||||
t.Error("Identity should not be empty")
|
||||
}
|
||||
if len(p.Focus) == 0 {
|
||||
t.Error("Focus should not be empty")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestListBuiltinPersonas(t *testing.T) {
|
||||
names := ListBuiltinPersonas()
|
||||
if len(names) == 0 {
|
||||
t.Fatal("expected at least one built-in persona")
|
||||
}
|
||||
|
||||
// Check for expected personas
|
||||
expected := map[string]bool{"security": false, "architect": false, "docs": false}
|
||||
for _, name := range names {
|
||||
if _, ok := expected[name]; ok {
|
||||
expected[name] = true
|
||||
}
|
||||
}
|
||||
for name, found := range expected {
|
||||
if !found {
|
||||
t.Errorf("expected built-in persona %q not found", name)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaFromYAMLFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "test.yaml")
|
||||
|
||||
content := `
|
||||
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")
|
||||
|
||||
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
|
||||
yaml string
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "missing name",
|
||||
yaml: "identity: test",
|
||||
wantErr: "name is required",
|
||||
},
|
||||
{
|
||||
name: "missing identity",
|
||||
yaml: "name: test",
|
||||
wantErr: "identity is required",
|
||||
},
|
||||
{
|
||||
name: "display_name defaults to name",
|
||||
yaml: "name: test\nidentity: 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.yaml")
|
||||
if err := os.WriteFile(path, []byte(tt.yaml), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
p, err := LoadPersona(path)
|
||||
if tt.wantErr != "" {
|
||||
if err == nil {
|
||||
t.Errorf("expected error containing %q, got nil", tt.wantErr)
|
||||
return
|
||||
}
|
||||
if !strings.Contains(err.Error(), tt.wantErr) {
|
||||
t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
|
||||
}
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
// Check display_name defaulting
|
||||
if p.DisplayName == "" {
|
||||
t.Error("DisplayName should default to Name")
|
||||
}
|
||||
if p.DisplayName != p.Name {
|
||||
t.Errorf("DisplayName should default to Name, got %q", p.DisplayName)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaFileNotFound(t *testing.T) {
|
||||
_, err := LoadPersona("/nonexistent/path/persona.yaml")
|
||||
if err == nil {
|
||||
t.Error("expected error for nonexistent file")
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaInvalidYAML(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "invalid.yaml")
|
||||
if err := os.WriteFile(path, []byte("not: valid: yaml: 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 YAML")
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user