Compare commits
1 Commits
issue-146
...
c1c8e61e9d
| Author | SHA1 | Date | |
|---|---|---|---|
| c1c8e61e9d |
@@ -2,10 +2,12 @@ package review
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
|
"context"
|
||||||
"embed"
|
"embed"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
|
"path/filepath"
|
||||||
"sort"
|
"sort"
|
||||||
"strings"
|
"strings"
|
||||||
"unicode/utf8"
|
"unicode/utf8"
|
||||||
@@ -28,6 +30,9 @@ const MaxYAMLDepth = 20
|
|||||||
// This prevents DoS via wide-but-shallow structures that bypass depth limits.
|
// This prevents DoS via wide-but-shallow structures that bypass depth limits.
|
||||||
const MaxYAMLNodes = 1000
|
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.
|
// Persona defines a specialized review role with focused expertise.
|
||||||
type Persona struct {
|
type Persona struct {
|
||||||
Name string `json:"name" yaml:"name"`
|
Name string `json:"name" yaml:"name"`
|
||||||
@@ -48,6 +53,20 @@ type Severity struct {
|
|||||||
Nit string `json:"nit" yaml:"nit"`
|
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.
|
// 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.
|
// Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON.
|
||||||
// Files larger than MaxPersonaFileSize are rejected.
|
// Files larger than MaxPersonaFileSize are rejected.
|
||||||
@@ -130,6 +149,92 @@ func ListBuiltinPersonas() []string {
|
|||||||
return names
|
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.
|
// parsePersona parses persona data from JSON or YAML format.
|
||||||
// Format is detected by the source file extension.
|
// Format is detected by the source file extension.
|
||||||
func parsePersona(data []byte, source string) (*Persona, error) {
|
func parsePersona(data []byte, source string) (*Persona, error) {
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
package review
|
package review
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"context"
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
@@ -776,3 +777,242 @@ identity: test identity
|
|||||||
t.Errorf("Name = %q, want %q", p.Name, "test")
|
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