From d7d5151a1fddd9e84d0f0e9dc9eaef48a656ddb8 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 02:49:21 -0700 Subject: [PATCH] feat(persona): add role-based review personas (#51) Implement role-based review personas that provide specialized review focus: - Security: vulnerabilities, auth, secrets, injection attacks - Architect: design patterns, code organization, API contracts - Docs: documentation quality, API clarity, error messages Changes: - Add persona loading from JSON files and embedded built-ins - Add --persona and --persona-file CLI flags (mutually exclusive) - Add BuildPersonaSystemPrompt for persona-specific prompts - Add FormatMarkdownWithDisplay for persona display names - Update action.yml with persona and persona-file inputs - Add comprehensive tests for all new functionality - Document personas in README with examples The persona system replaces the generic 'You are an expert code reviewer' prompt with domain-specific identity, focus areas, ignore list, and severity calibration. This reduces redundancy between multiple reviewers and catches domain-specific issues that generic reviewers miss. Closes #51 --- .gitea/actions/review/action.yml | 10 + README.md | 99 +++++++++ cmd/review-bot/main.go | 44 +++- docs/DESIGN-51-personas.md | 353 +++++++++++++++++++++++++++++++ review/formatter.go | 45 ++++ review/formatter_test.go | 55 +++++ review/persona.go | 98 +++++++++ review/persona_prompt.go | 116 ++++++++++ review/persona_prompt_test.go | 157 ++++++++++++++ review/persona_test.go | 211 ++++++++++++++++++ review/personas/architect.json | 25 +++ review/personas/docs.json | 24 +++ review/personas/security.json | 26 +++ 13 files changed, 1261 insertions(+), 2 deletions(-) create mode 100644 docs/DESIGN-51-personas.md 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.json create mode 100644 review/personas/docs.json create mode 100644 review/personas/security.json diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 78f1005..0cb44b1 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -71,6 +71,14 @@ inputs: required: false default: 'true' system-prompt-file: + 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: '' description: 'Local file with additional system prompt instructions (e.g. security review focus)' required: false default: '' @@ -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..184fa35 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,100 @@ 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 JSON file with your domain-specific review focus: + +```json +{ + "name": "trading", + "display_name": "Trading Domain Expert", + "identity": "You are a trading systems expert reviewing code for correctness.\n\nYour expertise:\n- Order lifecycle and state machines\n- Fill handling and partial fills\n- Position tracking and P&L calculations\n- 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.json + ... +``` + +### Persona vs system-prompt-file + +| Feature | `persona` / `persona-file` | `system-prompt-file` | +|---------|---------------------------|----------------------| +| Replaces base prompt | Yes | No (appends) | +| Structured format | Yes (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 021ccc3..5cd4536 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,33 @@ 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 != "" { + var err error + persona, err = review.LoadPersona(*personaFile) + 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) @@ -236,7 +265,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 +328,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 != "" { diff --git a/docs/DESIGN-51-personas.md b/docs/DESIGN-51-personas.md new file mode 100644 index 0000000..3abeb3e --- /dev/null +++ b/docs/DESIGN-51-personas.md @@ -0,0 +1,353 @@ +# 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: JSON Instead of YAML + +**Reason:** Project convention is "Go standard library only — no external dependencies." + +YAML requires `gopkg.in/yaml.v3` or similar. To maintain zero dependencies, persona files will use JSON instead. + +### Updated Persona File Format + +```json +{ + "name": "security", + "display_name": "Security Specialist", + "model_preference": "opus", + "identity": "You are a security specialist reviewing code for vulnerabilities.\nYour expertise: OWASP Top 10, injection attacks, auth/authz, secrets management.", + "focus": [ + "Injection attacks (SQL, command, path traversal, template)", + "Authentication and authorization gaps", + "Secrets exposure (hardcoded credentials, tokens in logs)" + ], + "ignore": [ + "Code style and naming conventions", + "Performance (unless security-related)", + "Documentation" + ], + "severity": { + "major": "Privilege escalation, information disclosure, DoS", + "minor": "Missing rate limiting, verbose errors", + "nit": "Theoretical risk with low exploitability" + } +} +``` + +This maintains all the same fields but uses JSON encoding, which Go handles natively via `encoding/json`. diff --git a/review/formatter.go b/review/formatter.go index 3e54823..eda5996 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -53,3 +53,48 @@ func GiteaEvent(verdict string) string { return "COMMENT" } } + +// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name. +// 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 + + // 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)) + } + + sb.WriteString("## Summary\n\n") + sb.WriteString(result.Summary) + sb.WriteString("\n\n") + + if len(result.Findings) > 0 { + sb.WriteString("## Findings\n\n") + sb.WriteString("| # | Severity | File | Line | Finding |\n") + sb.WriteString("|---|----------|------|------|--------|\n") + + for i, f := range result.Findings { + sb.WriteString(fmt.Sprintf("| %d | [%s] | `%s` | %d | %s |\n", + i+1, f.Severity, f.File, f.Line, f.Finding)) + } + sb.WriteString("\n") + } + + sb.WriteString("## Recommendation\n\n") + sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation)) + + 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", sentinelName)) + } + + return sb.String() +} 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, "