From 57e62a345fdcb828a1b2b88b96083fa0e3c456d5 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 08:43:21 -0700 Subject: [PATCH 1/2] 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 --- .gitea/actions/review/action.yml | 10 + README.md | 109 ++++++++++ cmd/review-bot/main.go | 110 ++++++++--- cmd/review-bot/main_test.go | 108 ++++++++++ docs/DESIGN-51-personas.md | 330 +++++++++++++++++++++++++++++++ go.mod | 2 + go.sum | 2 + review/formatter.go | 49 +++-- review/formatter_test.go | 55 ++++++ review/persona.go | 114 +++++++++++ review/persona_prompt.go | 104 ++++++++++ review/persona_prompt_test.go | 157 +++++++++++++++ review/persona_test.go | 242 +++++++++++++++++++++++ review/personas/architect.yaml | 34 ++++ review/personas/docs.yaml | 33 ++++ review/personas/security.yaml | 34 ++++ review/prompt.go | 44 +++-- 17 files changed, 1477 insertions(+), 60 deletions(-) create mode 100644 docs/DESIGN-51-personas.md create mode 100644 go.sum create mode 100644 review/persona.go create mode 100644 review/persona_prompt.go create mode 100644 review/persona_prompt_test.go create mode 100644 review/persona_test.go create mode 100644 review/personas/architect.yaml create mode 100644 review/personas/docs.yaml create mode 100644 review/personas/security.yaml diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 78f1005..f4ebb80 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -74,6 +74,14 @@ inputs: description: 'Local file with additional system prompt instructions (e.g. security review focus)' required: false default: '' + persona: + description: 'Built-in persona name (security, architect, docs)' + required: false + default: '' + persona-file: + description: 'Path to persona JSON file with custom review focus' + required: false + default: '' runs: using: 'composite' @@ -155,6 +163,8 @@ runs: LLM_PROVIDER: ${{ inputs.llm-provider }} UPDATE_EXISTING: ${{ inputs.update-existing }} SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }} + PERSONA: ${{ inputs.persona }} + PERSONA_FILE: ${{ inputs.persona-file }} run: | ARGS="" if [ "${{ inputs.dry-run }}" = "true" ]; then diff --git a/README.md b/README.md index 0060da8..402182a 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,8 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp | `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) | | `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos | | `system-prompt-file` | No | `""` | Local file with additional system prompt instructions | +| `persona` | No | `""` | Built-in persona name (security, architect, docs) | +| `persona-file` | No | `""` | Path to persona JSON file with custom review focus | | `temperature` | No | `0` | LLM temperature (0 = server default) | | `timeout` | No | `300` | LLM request timeout in seconds | | `dry-run` | No | `false` | Print review to stdout instead of posting | @@ -329,3 +331,110 @@ budget/ Token estimation + context trimming ## License MIT + +## Review Personas + +Personas provide role-based review specialization. Instead of generic code review, each persona focuses on a specific domain (security, architecture, documentation) with tailored prompts and severity calibration. + +### Built-in Personas + +| Persona | Focus | +|---------|-------| +| `security` | Vulnerabilities, auth bypass, secrets exposure, injection attacks | +| `architect` | Design patterns, code organization, API contracts, testability | +| `docs` | Documentation quality, API clarity, error messages | + +### Using Built-in Personas + +```yaml +- uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: security + persona: security + llm-model: claude-opus-4-20250514 # Security benefits from strong reasoning + ... +``` + +### Multiple Personas in Parallel + +```yaml +jobs: + review: + strategy: + matrix: + include: + - name: security + persona: security + - name: architect + persona: architect + steps: + - uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: ${{ matrix.name }} + persona: ${{ matrix.persona }} + ... +``` + +Each persona posts independently with its own sentinel, so reviews don't interfere. + + +### Custom Personas + +Create a YAML file with your domain-specific review focus: + +```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: + +```yaml +- uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: trading + 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 (YAML/JSON) | No (freeform) | +| Focus/ignore lists | Yes | Manual | +| Severity calibration | Yes | Manual | +| Header display name | Yes | No | +| Built-in options | Yes | No | + +Use personas for domain-specialized reviews. Use `system-prompt-file` for minor tweaks to the generic review. diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 33da475..f136e6c 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -70,6 +70,8 @@ func main() { llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)") llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)") llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic") + personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)") + personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file") flag.Parse() @@ -91,6 +93,36 @@ func main() { os.Exit(1) } + // Validate persona flags are mutually exclusive + if *personaName != "" && *personaFile != "" { + slog.Error("--persona and --persona-file are mutually exclusive") + os.Exit(1) + } + + // Load persona if specified + var persona *review.Persona + if *personaName != "" { + var err error + persona, err = review.LoadBuiltinPersona(*personaName) + if err != nil { + slog.Error("failed to load persona", "persona", *personaName, "error", err) + os.Exit(1) + } + slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName) + } else if *personaFile != "" { + resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file") + if err != nil { + slog.Error("invalid persona-file path", "error", err) + os.Exit(1) + } + persona, err = review.LoadPersona(resolvedPath) + if err != nil { + slog.Error("failed to load persona file", "file", *personaFile, "error", err) + os.Exit(1) + } + slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name) + } + // Validate reviewer-name: only safe characters allowed in sentinel if err := validateReviewerName(*reviewerName); err != nil { slog.Error("invalid reviewer name", "error", err) @@ -201,34 +233,14 @@ func main() { // Step 6b: Load additional system prompt if specified additionalPrompt := "" if *systemPromptFile != "" { - workspace := os.Getenv("GITHUB_WORKSPACE") - if workspace == "" { - workspace, _ = os.Getwd() - } - absWorkspace, err := filepath.Abs(workspace) + resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file") if err != nil { - slog.Error("failed to resolve workspace path", "error", err) - os.Exit(1) - } - promptPath := filepath.Join(absWorkspace, *systemPromptFile) - promptPath = filepath.Clean(promptPath) - if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace { - slog.Error("system-prompt-file resolves outside workspace", "path", promptPath, "workspace", absWorkspace) - os.Exit(1) - } - // Resolve symlinks and re-validate to prevent symlink traversal - resolvedPath, err := filepath.EvalSymlinks(promptPath) - if err != nil { - slog.Error("failed to resolve system prompt file", "path", promptPath, "error", err) - os.Exit(1) - } - if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace { - slog.Error("system-prompt-file symlink resolves outside workspace", "resolved", resolvedPath, "workspace", absWorkspace) + slog.Error("invalid system-prompt-file path", "error", err) os.Exit(1) } data, err := os.ReadFile(resolvedPath) if err != nil { - slog.Error("failed to read system prompt file", "path", promptPath, "error", err) + slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err) os.Exit(1) } additionalPrompt = string(data) @@ -236,7 +248,13 @@ func main() { } // Step 7: Budget-aware prompt assembly - systemBase := review.BuildSystemBase() + var systemBase string + if persona != nil { + systemBase = review.BuildPersonaSystemPrompt(persona) + slog.Debug("using persona system prompt", "persona", persona.Name) + } else { + systemBase = review.BuildSystemBase() + } if additionalPrompt != "" { systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt } @@ -293,7 +311,12 @@ func main() { slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings)) // Step 10: Format and post review - reviewBody := review.FormatMarkdown(result, *reviewerName) + var reviewBody string + if persona != nil && persona.DisplayName != "" { + reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName) + } else { + reviewBody = review.FormatMarkdown(result, *reviewerName) + } // Add commit footer so readers know which commit was evaluated if pr.Head.Sha != "" { @@ -587,6 +610,43 @@ func validateReviewerName(name string) error { return nil } +// validateWorkspacePath ensures a file path is within the workspace and resolves +// symlinks to prevent traversal attacks. Returns the resolved absolute path or +// an error if the path is outside the workspace. +func validateWorkspacePath(path, pathName string) (string, error) { + workspace := os.Getenv("GITHUB_WORKSPACE") + if workspace == "" { + workspace, _ = os.Getwd() + } + absWorkspace, err := filepath.Abs(workspace) + if err != nil { + return "", fmt.Errorf("failed to resolve workspace path: %w", err) + } + + // Join and clean the path + fullPath := filepath.Join(absWorkspace, path) + fullPath = filepath.Clean(fullPath) + + // Check path is within workspace using filepath.Rel (more robust than HasPrefix) + rel, err := filepath.Rel(absWorkspace, fullPath) + if err != nil || strings.HasPrefix(rel, "..") { + return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace) + } + + // Resolve symlinks and re-validate to prevent symlink traversal + resolvedPath, err := filepath.EvalSymlinks(fullPath) + if err != nil { + return "", fmt.Errorf("failed to resolve %s: %w", pathName, err) + } + + relResolved, err := filepath.Rel(absWorkspace, resolvedPath) + if err != nil || strings.HasPrefix(relResolved, "..") { + return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace) + } + + return resolvedPath, nil +} + // buildSupersededBody creates the body for a superseded review: struck-through banner // with collapsed original content and the commit it was evaluated against. func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string { diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 4c55d02..b6b9de2 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -6,6 +6,7 @@ import ( "log/slog" "os" "os/exec" + "path/filepath" "strings" "testing" @@ -45,6 +46,113 @@ func TestValidateReviewerName(t *testing.T) { } } +func TestValidateWorkspacePath(t *testing.T) { + // Create a temp directory as our workspace + tmpDir := t.TempDir() + + // Create a valid file inside the workspace + validFile := filepath.Join(tmpDir, "valid.json") + if err := os.WriteFile(validFile, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + // Create a subdirectory with a file + subDir := filepath.Join(tmpDir, "subdir") + if err := os.MkdirAll(subDir, 0755); err != nil { + t.Fatalf("failed to create subdir: %v", err) + } + nestedFile := filepath.Join(subDir, "nested.json") + if err := os.WriteFile(nestedFile, []byte("{}"), 0644); err != nil { + t.Fatalf("failed to create nested file: %v", err) + } + + // Create a symlink pointing outside the workspace + symlinkPath := filepath.Join(tmpDir, "evil-symlink.json") + if err := os.Symlink("/etc/passwd", symlinkPath); err != nil { + t.Fatalf("failed to create symlink: %v", err) + } + + // Save and restore GITHUB_WORKSPACE + origWorkspace := os.Getenv("GITHUB_WORKSPACE") + defer os.Setenv("GITHUB_WORKSPACE", origWorkspace) + + tests := []struct { + name string + workspace string + path string + wantErr bool + errMatch string + }{ + { + name: "valid relative path", + workspace: tmpDir, + path: "valid.json", + wantErr: false, + }, + { + name: "valid nested path", + workspace: tmpDir, + path: "subdir/nested.json", + wantErr: false, + }, + { + name: "path traversal attempt", + workspace: tmpDir, + path: "../../../etc/passwd", + wantErr: true, + errMatch: "resolves outside workspace", + }, + { + name: "absolute path gets normalized to relative", + workspace: tmpDir, + path: "/etc/passwd", + wantErr: true, + errMatch: "failed to resolve", // filepath.Join strips leading / making it /etc/passwd which doesn't exist + }, + { + name: "nonexistent file", + workspace: tmpDir, + path: "nonexistent.json", + wantErr: true, + errMatch: "failed to resolve", + }, + { + name: "symlink escaping workspace", + workspace: tmpDir, + path: "evil-symlink.json", + wantErr: true, + errMatch: "symlink resolves outside workspace", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + os.Setenv("GITHUB_WORKSPACE", tc.workspace) + resolved, err := validateWorkspacePath(tc.path, "test-file") + + if tc.wantErr { + if err == nil { + t.Errorf("expected error for %q, got nil", tc.path) + } else if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) { + t.Errorf("error %q should contain %q", err.Error(), tc.errMatch) + } + } else { + if err != nil { + t.Errorf("expected no error for %q, got %v", tc.path, err) + } + if resolved == "" { + t.Error("expected non-empty resolved path") + } + // Verify resolved path is within workspace + if !strings.HasPrefix(resolved, tc.workspace) { + t.Errorf("resolved path %q not within workspace %q", resolved, tc.workspace) + } + } + }) + } +} + + func makeReview(id int64, login, state string, stale bool, body string) gitea.Review { r := gitea.Review{ ID: id, diff --git a/docs/DESIGN-51-personas.md b/docs/DESIGN-51-personas.md new file mode 100644 index 0000000..26f244d --- /dev/null +++ b/docs/DESIGN-51-personas.md @@ -0,0 +1,330 @@ +# Design: Role-based Review Personas (Issue #51) + +## Problem + +Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to: + +1. **Redundancy** — Two reviewers (e.g., GPT + Claude twins) often flag identical issues +2. **Gaps** — Generic reviewers miss specialized concerns (security, domain logic, architecture) +3. **Noise** — NITs about style mixed with critical security findings +4. **No ownership** — Findings lack clear domain attribution + +## Constraints + +- Must work with existing CLI flags and CI workflow patterns +- Must not break backwards compatibility (existing configs still work) +- Must integrate cleanly with the budget system (personas add to context) +- Multiple personas running in parallel must not interfere with each other +- Each persona must have clear scope boundaries (no duplication) + +## Proposed Approach + +### 1. Persona Definition + +A persona is a named review role with: +- **Identity** — Who am I? What's my expertise? +- **Focus** — What do I look for? +- **Scope boundaries** — What do I explicitly NOT comment on? +- **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain? + +Personas are defined in YAML files that can live: +1. In the pattern repos (shared across projects) +2. In the target repo (project-specific personas) +3. Inline via a new `--persona-file` flag + +### 2. Persona File Format + +```yaml +# .review/personas/security.yaml +name: security +display_name: Security Specialist +model_preference: opus # optional hint for expensive analysis + +identity: | + You are a security specialist reviewing code for vulnerabilities. + Your expertise: OWASP Top 10, injection attacks, auth/authz, secrets management, + event sourcing security (replay attacks, event injection). + +focus: + - Injection attacks (SQL, command, path traversal, template) + - Authentication and authorization gaps + - Secrets exposure (hardcoded credentials, tokens in logs) + - Input validation (unsanitized input, unsafe deserialization) + - Race conditions with security implications + - Event sourcing attack vectors + +ignore: + - Code style and naming conventions + - Performance (unless security-related) + - Documentation + - General code quality + - Test coverage + +severity: + critical: "Remote code execution, auth bypass, data exfiltration" + major: "Privilege escalation, information disclosure, DoS" + minor: "Missing rate limiting, verbose errors" + nit: "Theoretical risk with low exploitability" + +output_format: | + For each finding: + - Severity: [CRITICAL|MAJOR|MINOR|NIT] + - Attack vector: How could this be exploited? + - Evidence: Code snippet showing the vulnerability + - Recommendation: Specific fix +``` + +### 3. New CLI Flags + +``` +--persona-file PATH Path to persona YAML file (local or in repo) +--persona NAME Built-in persona name (security, architect, domain) +``` + +Either flag sets the persona. If neither is provided, behavior is unchanged (generic review). + +### 4. Prompt Assembly + +Current flow: +``` +SystemBase → Patterns → Conventions → [LLM] +``` + +New flow with persona: +``` +PersonaPrompt (from YAML) → Patterns (filtered?) → Conventions → [LLM] +``` + +The persona's identity/focus/ignore/severity sections become the system prompt, replacing the generic "You are an expert code reviewer" base. + +### 5. Built-in Personas + +Ship with these built-in personas (loadable via `--persona NAME`): + +| Name | Focus | +|------|-------| +| `security` | Vulnerabilities, auth, secrets | +| `architect` | Patterns, consistency, design | +| `domain` | Business logic (requires repo-specific config) | +| `docs` | Documentation, API clarity | + +Built-in personas live in `review/personas/` as embedded Go assets or YAML shipped with the binary. + +### 6. CI Workflow Integration + +Single persona: +```yaml +- uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: security + persona: security + ... +``` + +Multiple personas (parallel jobs): +```yaml +jobs: + review: + strategy: + matrix: + include: + - name: security + persona: security + - name: architect + persona: architect + steps: + - uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: ${{ matrix.name }} + persona: ${{ matrix.persona }} +``` + +Custom persona from repo: +```yaml +- uses: rodin/review-bot/.gitea/actions/review@v1 + with: + reviewer-name: trading + persona-file: .review/personas/trading.yaml +``` + +### 7. Persona + Patterns Interaction + +Some personas benefit from filtered patterns: +- Security → only security-related patterns +- Architect → all patterns (structural focus) +- Domain → domain docs, not language patterns + +For v1, keep it simple: all patterns are included regardless of persona. Future enhancement could add `patterns_filter` to persona YAML. + +### 8. Output Format Changes + +Persona name appears in the review header: +```markdown +# Security Review + +## Summary +No critical vulnerabilities found in this change. + +## Findings +| # | Severity | File | Line | Finding | +... + +## Recommendation +**APPROVE** — No security-relevant issues detected. + +--- +*Review by security* + +``` + +## State/Data Model + +### Persona struct + +```go +// review/persona.go +type Persona struct { + Name string `yaml:"name"` + DisplayName string `yaml:"display_name"` + ModelPref string `yaml:"model_preference,omitempty"` + Identity string `yaml:"identity"` + Focus []string `yaml:"focus"` + Ignore []string `yaml:"ignore"` + Severity Severity `yaml:"severity"` + OutputFormat string `yaml:"output_format,omitempty"` +} + +type Severity struct { + Critical string `yaml:"critical"` + Major string `yaml:"major"` + Minor string `yaml:"minor"` + Nit string `yaml:"nit"` +} +``` + +### Loading precedence + +1. `--persona-file PATH` → load from local file system +2. `--persona NAME` → load from embedded built-ins +3. Neither → use generic system prompt (current behavior) + +## Error Cases + +| Error | Handling | +|-------|----------| +| Persona file not found | Fatal exit with clear message | +| Invalid YAML in persona file | Fatal exit with parse error | +| Both `--persona` and `--persona-file` specified | Fatal exit: mutually exclusive | +| Unknown built-in persona name | Fatal exit with list of valid names | +| Empty identity in persona | Warning, fall back to generic prompt | + +## Edge Cases + +- **Empty focus list**: Valid — persona relies on identity alone +- **Empty ignore list**: Valid — no explicit scope exclusions +- **No severity section**: Use default MAJOR/MINOR/NIT definitions +- **Model preference set but budget insufficient**: Ignore preference, log warning +- **Persona file in pattern repo**: Fetch like other pattern files + +## Testing Strategy + +### Unit tests +- `persona_test.go`: Parse valid/invalid YAML, validate required fields +- `prompt_test.go`: Verify persona prompt assembly +- Integration with budget: persona prompts count toward token limit + +### Integration tests +- End-to-end with `--persona security` (built-in) +- End-to-end with `--persona-file custom.yaml` +- Backwards compatibility: no flags = generic behavior + +### Manual verification +- Run security persona on a PR with obvious vulnerability +- Verify security persona ignores style issues +- Verify non-security persona doesn't flag security issues + +## Implementation Phases + +### Phase 1: Persona types and loading +- [ ] `review/persona.go`: Persona struct + YAML parsing +- [ ] `review/persona_test.go`: Unit tests +- [ ] Embed built-in personas in binary +- [ ] Compiles clean, tests pass + +### Phase 2: Prompt generation +- [ ] `review/prompt.go`: `BuildPersonaPrompt(p Persona) string` +- [ ] Modify `BuildSystemBase()` to accept optional persona +- [ ] Integrate persona prompt with budget system +- [ ] Tests for prompt assembly + +### Phase 3: CLI integration +- [ ] Add `--persona` and `--persona-file` flags +- [ ] Flag validation (mutually exclusive, valid names) +- [ ] Load persona based on flags +- [ ] Pass persona to prompt builder + +### Phase 4: Action integration +- [ ] Add `persona` and `persona-file` inputs to action.yml +- [ ] Update README with persona examples +- [ ] End-to-end CI test + +### Phase 5: Built-in personas +- [ ] `security.yaml` built-in +- [ ] `architect.yaml` built-in +- [ ] `docs.yaml` built-in +- [ ] Document each persona's focus + +## Open Questions + +1. **Persona file location in repo**: Should we support `--persona-file .review/security.yaml` where the file is fetched from the PR's repo (like conventions)? This adds complexity but enables project-specific personas without action changes. + +2. **Model preference enforcement**: If persona specifies `model_preference: opus` but the action uses a different model, should we warn? Override? Ignore? Current thinking: log warning, use the specified model (user controls model via action input). + +3. **Severity override output**: If persona defines custom severity levels (CRITICAL), should the JSON output include them, or map back to standard MAJOR/MINOR/NIT? Current thinking: keep standard output format, use severity calibration only for prompt guidance. + +## Completion Checklist + +1. Persona struct matches YAML schema exactly? +2. Built-in personas embedded in binary (not external files)? +3. `--persona` and `--persona-file` are mutually exclusive? +4. Unknown persona name produces clear error with valid options? +5. Empty persona file fields have sensible defaults? +6. Persona prompt integrates with budget system (token counting)? +7. Backwards compatibility: no flags = current behavior? +8. Review header shows persona display name? +9. Sentinel still uses reviewer-name (not persona name)? +10. Unit tests cover parse errors, missing fields, valid YAML? + +## Design Review Findings (Self-Review) + +### Finding 1: Severity Mapping +The persona YAML allows `critical` severity, but the LLM output parser (`review/parser.go`) only accepts MAJOR/MINOR/NIT. + +**Resolution:** Keep standard output format. Persona severity section is ONLY for calibrating the LLM's judgment (prompt guidance). Output must still use MAJOR/MINOR/NIT. Document this clearly in persona format docs. + +### Finding 2: Embedding Built-in Personas +Go doesn't natively embed YAML. Must use `//go:embed` directive (Go 1.16+). + +**Resolution:** Create `review/personas/` directory with YAML files and use: +```go +//go:embed personas/*.yaml +var embeddedPersonas embed.FS +``` + +### Finding 3: display_name vs reviewer-name +Design says header shows "persona display name" but sentinel uses "reviewer-name". This is correct - they serve different purposes: +- `display_name` → human-readable header ("Security Specialist Review") +- `reviewer-name` → machine sentinel for cleanup (``) + +When persona is used, `display_name` takes precedence for the header title, but `reviewer-name` (CLI flag) is still used for the sentinel. + +## Design Revision: YAML with gopkg.in/yaml.v3 + +**Decision:** Add `gopkg.in/yaml.v3` as a dependency. + +YAML is preferred over JSON for persona files because: +- Multi-line strings are cleaner (no escaping quotes in identity/focus text) +- Comments are supported for documentation +- More human-readable for complex persona definitions + +The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas. diff --git a/go.mod b/go.mod index 9b2e8d2..5348662 100644 --- a/go.mod +++ b/go.mod @@ -1,3 +1,5 @@ module gitea.weiker.me/rodin/review-bot go 1.26.2 + +require github.com/goccy/go-yaml v1.19.2 diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..bd88ba6 --- /dev/null +++ b/go.sum @@ -0,0 +1,2 @@ +github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= +github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= diff --git a/review/formatter.go b/review/formatter.go index 3e54823..1eaab7c 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -7,10 +7,37 @@ import ( // FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review. func FormatMarkdown(result *ReviewResult, reviewerName string) string { + return FormatMarkdownWithDisplay(result, reviewerName, reviewerName) +} + +// GiteaEvent converts the verdict to the Gitea API event string. +func GiteaEvent(verdict string) string { + switch verdict { + case "APPROVE": + return "APPROVED" + case "REQUEST_CHANGES": + return "REQUEST_CHANGES" + default: + return "COMMENT" + } +} + +// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name. +// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown. +// Persona display names are controlled by repo owners (trusted input). +// displayName is used for the header title, sentinelName is used for the cleanup sentinel. +// If displayName is empty, sentinelName is used for both. +func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string { var sb strings.Builder - if reviewerName != "" { - title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:] + // Use display name for header, or fall back to sentinel name + headerName := displayName + if headerName == "" { + headerName = sentinelName + } + + if headerName != "" { + title := strings.ToUpper(headerName[:1]) + headerName[1:] sb.WriteString(fmt.Sprintf("# %s Review\n\n", title)) } @@ -33,23 +60,11 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string { sb.WriteString("## Recommendation\n\n") sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation)) - if reviewerName != "" { - sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName)) + if sentinelName != "" { + sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", headerName)) // Hidden sentinel for identifying this bot's reviews during cleanup - sb.WriteString(fmt.Sprintf("\n\n", reviewerName)) + sb.WriteString(fmt.Sprintf("\n\n", sentinelName)) } return sb.String() } - -// GiteaEvent converts the verdict to the Gitea API event string. -func GiteaEvent(verdict string) string { - switch verdict { - case "APPROVE": - return "APPROVED" - case "REQUEST_CHANGES": - return "REQUEST_CHANGES" - default: - return "COMMENT" - } -} diff --git a/review/formatter_test.go b/review/formatter_test.go index aa28846..7684e88 100644 --- a/review/formatter_test.go +++ b/review/formatter_test.go @@ -159,3 +159,58 @@ func TestFormatMarkdown_RoleTitle(t *testing.T) { t.Error("should not contain role title header when reviewer name is empty") } } + +func TestFormatMarkdownWithDisplay(t *testing.T) { + result := &ReviewResult{ + Verdict: "APPROVE", + Summary: "Test summary", + Findings: nil, + Recommendation: "Test recommendation", + } + + t.Run("with display name", func(t *testing.T) { + body := FormatMarkdownWithDisplay(result, "Security Specialist", "security") + + // Header should use display name + if !strings.Contains(body, "# Security Specialist Review") { + t.Error("header should use display name") + } + + // Sentinel should use sentinel name + if !strings.Contains(body, "") { + t.Error("sentinel should use sentinel name") + } + + // Footer "Review by" should use display name + if !strings.Contains(body, "*Review by Security Specialist*") { + t.Error("footer should use display name") + } + }) + + t.Run("without display name", func(t *testing.T) { + body := FormatMarkdownWithDisplay(result, "", "reviewer") + + // Should fall back to sentinel name for header + if !strings.Contains(body, "# Reviewer Review") { + t.Error("header should fall back to sentinel name") + } + + if !strings.Contains(body, "") { + t.Error("sentinel should use sentinel name") + } + }) + + t.Run("empty both names", func(t *testing.T) { + body := FormatMarkdownWithDisplay(result, "", "") + + // Should not have header + if strings.Contains(body, "# ") && strings.Contains(body, " Review") { + t.Error("should not have header when both names empty") + } + + // Should not have sentinel + if strings.Contains(body, "" sentinel := "" @@ -734,8 +734,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) { {" rest", "sonnet"}, {" rest", "gpt-review"}, {"no sentinel here", "unknown"}, - {" end", "abc"}, // embedded in text + {" end", "abc"}, // embedded in text } for _, tc := range tests { diff --git a/docs/DESIGN-51-personas.md b/docs/DESIGN-51-personas.md index 26f244d..b865dcf 100644 --- a/docs/DESIGN-51-personas.md +++ b/docs/DESIGN-51-personas.md @@ -1,5 +1,9 @@ # Design: Role-based Review Personas (Issue #51) +> **Note:** This design was revised during implementation to use JSON instead of YAML +> to maintain the repository's zero-external-dependencies convention. All persona +> files use JSON format. See "Design Revision" section at the end for details. + ## Problem Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to: @@ -27,14 +31,14 @@ A persona is a named review role with: - **Scope boundaries** — What do I explicitly NOT comment on? - **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain? -Personas are defined in YAML files that can live: +Personas are defined in JSON files that can live: 1. In the pattern repos (shared across projects) 2. In the target repo (project-specific personas) -3. Inline via a new `--persona-file` flag +3. Inline via a new `--persona-file` flag (JSON format) ### 2. Persona File Format -```yaml +```json # .review/personas/security.yaml name: security display_name: Security Specialist @@ -77,7 +81,7 @@ output_format: | ### 3. New CLI Flags ``` ---persona-file PATH Path to persona YAML file (local or in repo) +--persona-file PATH Path to persona JSON file (local or in repo) --persona NAME Built-in persona name (security, architect, domain) ``` diff --git a/go.mod b/go.mod index 5348662..9b2e8d2 100644 --- a/go.mod +++ b/go.mod @@ -1,5 +1,3 @@ module gitea.weiker.me/rodin/review-bot go 1.26.2 - -require github.com/goccy/go-yaml v1.19.2 diff --git a/go.sum b/go.sum deleted file mode 100644 index bd88ba6..0000000 --- a/go.sum +++ /dev/null @@ -1,2 +0,0 @@ -github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM= -github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA= diff --git a/review/formatter.go b/review/formatter.go index 1eaab7c..d2e109f 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -37,7 +37,7 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s } if headerName != "" { - title := strings.ToUpper(headerName[:1]) + headerName[1:] + title := CapitalizeFirst(headerName) sb.WriteString(fmt.Sprintf("# %s Review\n\n", title)) } diff --git a/review/persona.go b/review/persona.go index 82dbb01..c8d676c 100644 --- a/review/persona.go +++ b/review/persona.go @@ -5,37 +5,34 @@ import ( "encoding/json" "fmt" "os" - "path/filepath" "strings" - - "github.com/goccy/go-yaml" + "unicode/utf8" ) -//go:embed personas/*.yaml +//go:embed personas/*.json var embeddedPersonas embed.FS // Persona defines a specialized review role with focused expertise. type Persona struct { - 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"` + 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"` } // 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" yaml:"major"` - Minor string `json:"minor" yaml:"minor"` - Nit string `json:"nit" yaml:"nit"` + Major string `json:"major"` + Minor string `json:"minor"` + Nit string `json:"nit"` } -// LoadPersona loads a persona from a file path. -// Supports both YAML (.yaml, .yml) and JSON (.json) formats. +// LoadPersona loads a persona from a JSON file path. func LoadPersona(path string) (*Persona, error) { data, err := os.ReadFile(path) if err != nil { @@ -47,7 +44,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 + ".yaml" + filename := name + ".json" data, err := embeddedPersonas.ReadFile("personas/" + filename) // embed.FS paths use forward slashes per io/fs spec if err != nil { available := ListBuiltinPersonas() @@ -57,10 +54,11 @@ func LoadBuiltinPersona(name string) (*Persona, error) { } // ListBuiltinPersonas returns the names of all built-in personas. +// Returns an empty slice if the embedded directory cannot be read. func ListBuiltinPersonas() []string { entries, err := embeddedPersonas.ReadDir("personas") if err != nil { - return nil + return []string{} } var names []string for _, e := range entries { @@ -68,10 +66,8 @@ func ListBuiltinPersonas() []string { continue } name := e.Name() - 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")) + if strings.HasSuffix(name, ".json") { + names = append(names, strings.TrimSuffix(name, ".json")) } } return names @@ -79,20 +75,9 @@ func ListBuiltinPersonas() []string { func parsePersona(data []byte, source string) (*Persona, error) { var p Persona - - // 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 := json.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 } @@ -112,3 +97,16 @@ func validatePersona(p *Persona, source string) error { } return nil } + +// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way. +// Returns the original string if it's empty. +func CapitalizeFirst(s string) string { + if s == "" { + return s + } + r, size := utf8.DecodeRuneInString(s) + if r == utf8.RuneError { + return s + } + return strings.ToUpper(string(r)) + s[size:] +} diff --git a/review/persona_test.go b/review/persona_test.go index 7dbf8ef..f749ef9 100644 --- a/review/persona_test.go +++ b/review/persona_test.go @@ -87,26 +87,22 @@ func TestListBuiltinPersonas(t *testing.T) { } } -func TestLoadPersonaFromYAMLFile(t *testing.T) { +func TestLoadPersonaFromJSONFile(t *testing.T) { dir := t.TempDir() - path := filepath.Join(dir, "test.yaml") + path := filepath.Join(dir, "test.json") - 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 -` + content := `{ + "name": "test", + "display_name": "Test Persona", + "identity": "You are a test persona.\nMulti-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) @@ -131,59 +127,25 @@ severity: } } -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 + json string wantErr string }{ { name: "missing name", - yaml: "identity: test", + json: `{"identity": "test"}`, wantErr: "name is required", }, { name: "missing identity", - yaml: "name: test", + json: `{"name": "test"}`, wantErr: "identity is required", }, { name: "display_name defaults to name", - yaml: "name: test\nidentity: test identity", + json: `{"name": "test", "identity": "test identity"}`, // No error expected - should succeed }, } @@ -191,8 +153,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.yaml") - if err := os.WriteFile(path, []byte(tt.yaml), 0644); err != nil { + path := filepath.Join(dir, "test.json") + if err := os.WriteFile(path, []byte(tt.json), 0644); err != nil { t.Fatalf("failed to write test file: %v", err) } @@ -222,21 +184,56 @@ func TestLoadPersonaValidation(t *testing.T) { } func TestLoadPersonaFileNotFound(t *testing.T) { - _, err := LoadPersona("/nonexistent/path/persona.yaml") + _, err := LoadPersona("/nonexistent/path/persona.json") if err == nil { t.Error("expected error for nonexistent file") } } -func TestLoadPersonaInvalidYAML(t *testing.T) { +func TestLoadPersonaInvalidJSON(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 { + path := filepath.Join(dir, "invalid.json") + if err := os.WriteFile(path, []byte("not valid json {"), 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") + t.Error("expected error for invalid JSON") + } +} + +func TestCapitalizeFirst(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"hello", "Hello"}, + {"Hello", "Hello"}, + {"HELLO", "HELLO"}, + {"a", "A"}, + {"", ""}, + {"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case + {"über", "Über"}, // German umlaut + {"élève", "Élève"}, // French accent + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + got := CapitalizeFirst(tt.input) + if got != tt.want { + t.Errorf("CapitalizeFirst(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestListBuiltinPersonasReturnsEmptySlice(t *testing.T) { + // ListBuiltinPersonas should return an empty slice (not nil) on error. + // We can't easily test the error case, but we can verify the success case + // returns a proper slice. + names := ListBuiltinPersonas() + if names == nil { + t.Error("ListBuiltinPersonas should return empty slice, not nil") } } diff --git a/review/personas/architect.json b/review/personas/architect.json new file mode 100644 index 0000000..86541c1 --- /dev/null +++ b/review/personas/architect.json @@ -0,0 +1,26 @@ +{ + "name": "architect", + "display_name": "Software Architect", + "identity": "You are a software architect reviewing code for design quality.\n\nYour expertise:\n- Design patterns and anti-patterns\n- Code organization and module boundaries\n- API design and contracts\n- Testability and dependency injection\n- Consistency with existing architecture\n- 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" + } +} diff --git a/review/personas/architect.yaml b/review/personas/architect.yaml deleted file mode 100644 index d63e50e..0000000 --- a/review/personas/architect.yaml +++ /dev/null @@ -1,34 +0,0 @@ -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" diff --git a/review/personas/docs.json b/review/personas/docs.json new file mode 100644 index 0000000..1829667 --- /dev/null +++ b/review/personas/docs.json @@ -0,0 +1,26 @@ +{ + "name": "docs", + "display_name": "Documentation Reviewer", + "identity": "You are a documentation specialist reviewing code for clarity and documentation quality.\n\nYour expertise:\n- API documentation and examples\n- Code comments and their accuracy\n- Error message clarity\n- README and guide quality\n- 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" + } +} diff --git a/review/personas/docs.yaml b/review/personas/docs.yaml deleted file mode 100644 index 95610e8..0000000 --- a/review/personas/docs.yaml +++ /dev/null @@ -1,33 +0,0 @@ -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" diff --git a/review/personas/security.json b/review/personas/security.json new file mode 100644 index 0000000..fd159f7 --- /dev/null +++ b/review/personas/security.json @@ -0,0 +1,26 @@ +{ + "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" + } +} diff --git a/review/personas/security.yaml b/review/personas/security.yaml deleted file mode 100644 index 6dab313..0000000 --- a/review/personas/security.yaml +++ /dev/null @@ -1,34 +0,0 @@ -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" -- 2.47.3