diff --git a/review/persona.go b/review/persona.go index 0f7f141..d697927 100644 --- a/review/persona.go +++ b/review/persona.go @@ -2,10 +2,12 @@ package review import ( "bytes" + "context" "embed" "encoding/json" "fmt" "os" + "path/filepath" "sort" "strings" "unicode/utf8" @@ -28,6 +30,9 @@ const MaxYAMLDepth = 20 // This prevents DoS via wide-but-shallow structures that bypass depth limits. const MaxYAMLNodes = 1000 +// RepoPersonasPath is the path within a repository where custom personas are stored. +const RepoPersonasPath = ".review-bot/personas" + // Persona defines a specialized review role with focused expertise. type Persona struct { Name string `json:"name" yaml:"name"` @@ -48,6 +53,20 @@ type Severity struct { Nit string `json:"nit" yaml:"nit"` } +// RepoContentFetcher is an interface for fetching file content from a repository. +// This allows LoadRepoPersonas to work with any client that can list and fetch files. +type RepoContentFetcher interface { + ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) + GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) +} + +// ContentEntry represents a file or directory entry (mirrors gitea.ContentEntry). +type ContentEntry struct { + Name string `json:"name"` + Path string `json:"path"` + Type string `json:"type"` // "file" or "dir" +} + // LoadPersona loads a persona from a JSON or YAML file path. // Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON. // Files larger than MaxPersonaFileSize are rejected. @@ -130,6 +149,92 @@ func ListBuiltinPersonas() []string { return names } +// LoadRepoPersonas loads custom personas from a repository's .review-bot/personas/ directory. +// Returns an empty map if the directory doesn't exist or is empty. +// Repo personas take precedence over built-in personas with the same name. +func LoadRepoPersonas(ctx context.Context, client RepoContentFetcher, owner, repo string) (map[string]*Persona, error) { + personas := make(map[string]*Persona) + + entries, err := client.ListContents(ctx, owner, repo, RepoPersonasPath) + if err != nil { + // Directory doesn't exist — not an error, just no custom personas + return personas, nil + } + + for _, entry := range entries { + if entry.Type != "file" { + continue + } + // Only load YAML files + ext := strings.ToLower(filepath.Ext(entry.Name)) + if ext != ".yaml" && ext != ".yml" { + continue + } + + content, err := client.GetFileContent(ctx, owner, repo, entry.Path) + if err != nil { + // Log but don't fail — one bad persona shouldn't break the whole review + continue + } + + // Validate file size + if len(content) > MaxPersonaFileSize { + continue + } + + persona, err := parsePersona([]byte(content), "repo:"+entry.Path) + if err != nil { + // Log but don't fail + continue + } + + personas[persona.Name] = persona + } + + return personas, nil +} + +// LoadPersonaWithFallback loads a persona by name, checking the repo first, then built-ins. +// This is the primary entry point for loading personas during a review. +func LoadPersonaWithFallback(ctx context.Context, client RepoContentFetcher, owner, repo, name string) (*Persona, error) { + // Try repo personas first + repoPersonas, err := LoadRepoPersonas(ctx, client, owner, repo) + if err == nil { + if p, ok := repoPersonas[name]; ok { + return p, nil + } + } + + // Fall back to built-in + return LoadBuiltinPersona(name) +} + +// ListAllPersonas returns a merged list of available personas (repo + built-in). +// Repo personas take precedence over built-ins with the same name. +func ListAllPersonas(ctx context.Context, client RepoContentFetcher, owner, repo string) []string { + seen := make(map[string]bool) + + // Built-ins first + for _, name := range ListBuiltinPersonas() { + seen[name] = true + } + + // Repo personas override + repoPersonas, err := LoadRepoPersonas(ctx, client, owner, repo) + if err == nil { + for name := range repoPersonas { + seen[name] = true + } + } + + names := make([]string, 0, len(seen)) + for name := range seen { + names = append(names, name) + } + sort.Strings(names) + return names +} + // parsePersona parses persona data from JSON or YAML format. // Format is detected by the source file extension. func parsePersona(data []byte, source string) (*Persona, error) { diff --git a/review/persona_test.go b/review/persona_test.go index b90eae7..5958ea4 100644 --- a/review/persona_test.go +++ b/review/persona_test.go @@ -1,6 +1,7 @@ package review import ( + "context" "fmt" "os" "path/filepath" @@ -776,3 +777,242 @@ identity: test identity t.Errorf("Name = %q, want %q", p.Name, "test") } } + +// MockRepoFetcher is a mock implementation of RepoContentFetcher for testing. +type MockRepoFetcher struct { + Contents map[string][]ContentEntry // path -> entries + Files map[string]string // path -> content +} + +func (m *MockRepoFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { + key := fmt.Sprintf("%s/%s/%s", owner, repo, path) + entries, ok := m.Contents[key] + if !ok { + return nil, fmt.Errorf("path not found: %s", path) + } + return entries, nil +} + +func (m *MockRepoFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { + key := fmt.Sprintf("%s/%s/%s", owner, repo, filepath) + content, ok := m.Files[key] + if !ok { + return "", fmt.Errorf("file not found: %s", filepath) + } + return content, nil +} + +func TestLoadRepoPersonas(t *testing.T) { + validPersona := `name: custom-security +display_name: Custom Security +identity: | + You are a custom security reviewer. +focus: + - SQL injection + - XSS attacks +ignore: + - Code style +severity: + major: Critical vulnerabilities + minor: Potential issues + nit: Suggestions +` + + tests := []struct { + name string + fetcher *MockRepoFetcher + wantCount int + wantNames []string + }{ + { + name: "no personas directory", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{}, + Files: map[string]string{}, + }, + wantCount: 0, + }, + { + name: "empty personas directory", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{ + "owner/repo/.review-bot/personas": {}, + }, + Files: map[string]string{}, + }, + wantCount: 0, + }, + { + name: "one valid persona", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{ + "owner/repo/.review-bot/personas": { + {Name: "custom-security.yaml", Path: ".review-bot/personas/custom-security.yaml", Type: "file"}, + }, + }, + Files: map[string]string{ + "owner/repo/.review-bot/personas/custom-security.yaml": validPersona, + }, + }, + wantCount: 1, + wantNames: []string{"custom-security"}, + }, + { + name: "skip non-yaml files", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{ + "owner/repo/.review-bot/personas": { + {Name: "custom-security.yaml", Path: ".review-bot/personas/custom-security.yaml", Type: "file"}, + {Name: "readme.md", Path: ".review-bot/personas/readme.md", Type: "file"}, + {Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"}, + }, + }, + Files: map[string]string{ + "owner/repo/.review-bot/personas/custom-security.yaml": validPersona, + }, + }, + wantCount: 1, + wantNames: []string{"custom-security"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + personas, err := LoadRepoPersonas(ctx, tt.fetcher, "owner", "repo") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(personas) != tt.wantCount { + t.Errorf("got %d personas, want %d", len(personas), tt.wantCount) + } + for _, name := range tt.wantNames { + if _, ok := personas[name]; !ok { + t.Errorf("missing expected persona %q", name) + } + } + }) + } +} + +func TestLoadPersonaWithFallback(t *testing.T) { + customSecurity := `name: security +display_name: Custom Security Override +identity: | + Custom security reviewer for this repo. +focus: + - Repo-specific security +ignore: + - General stuff +severity: + major: Critical + minor: Warning + nit: Info +` + + tests := []struct { + name string + fetcher *MockRepoFetcher + personaName string + wantDisplayName string + wantErr bool + }{ + { + name: "repo persona overrides builtin", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{ + "owner/repo/.review-bot/personas": { + {Name: "security.yaml", Path: ".review-bot/personas/security.yaml", Type: "file"}, + }, + }, + Files: map[string]string{ + "owner/repo/.review-bot/personas/security.yaml": customSecurity, + }, + }, + personaName: "security", + wantDisplayName: "Custom Security Override", + }, + { + name: "fallback to builtin when repo has no override", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{}, + Files: map[string]string{}, + }, + personaName: "security", + wantDisplayName: "Security Specialist", + }, + { + name: "unknown persona", + fetcher: &MockRepoFetcher{ + Contents: map[string][]ContentEntry{}, + Files: map[string]string{}, + }, + personaName: "nonexistent", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + p, err := LoadPersonaWithFallback(ctx, tt.fetcher, "owner", "repo", tt.personaName) + if tt.wantErr { + if err == nil { + t.Error("expected error, got nil") + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if p.DisplayName != tt.wantDisplayName { + t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplayName) + } + }) + } +} + +func TestListAllPersonas(t *testing.T) { + customPersona := `name: repo-specific +display_name: Repo Specific +identity: A repo-specific reviewer. +focus: [] +ignore: [] +severity: + major: Major + minor: Minor + nit: Nit +` + + fetcher := &MockRepoFetcher{ + Contents: map[string][]ContentEntry{ + "owner/repo/.review-bot/personas": { + {Name: "repo-specific.yaml", Path: ".review-bot/personas/repo-specific.yaml", Type: "file"}, + }, + }, + Files: map[string]string{ + "owner/repo/.review-bot/personas/repo-specific.yaml": customPersona, + }, + } + + ctx := context.Background() + names := ListAllPersonas(ctx, fetcher, "owner", "repo") + + // Should include both built-ins and repo-specific + builtins := ListBuiltinPersonas() + if len(names) <= len(builtins) { + t.Error("expected more personas than just built-ins") + } + + // Check repo-specific is included + found := false + for _, name := range names { + if name == "repo-specific" { + found = true + break + } + } + if !found { + t.Error("repo-specific persona not found in list") + } +}