feat: load personas from target repo .review-bot/personas/
CI / test (pull_request) Successful in 9m30s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 9m50s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 10m19s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m36s
CI / test (pull_request) Successful in 9m30s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 9m50s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 10m19s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 10m36s
- Add RepoContentFetcher interface for fetching repo files - Add LoadRepoPersonas() to load custom personas from repo - Add LoadPersonaWithFallback() to check repo then built-in - Add ListAllPersonas() to merge repo + built-in persona names - Repo personas take precedence over built-ins with same name Closes #60
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user