feat(persona): add role-based review personas #55
@@ -79,7 +79,7 @@ inputs:
|
||||
required: false
|
||||
default: ''
|
||||
persona-file:
|
||||
description: 'Path to persona JSON file with custom review focus'
|
||||
description: 'Path to custom persona JSON file'
|
||||
required: false
|
||||
default: ''
|
||||
|
||||
|
||||
@@ -380,38 +380,32 @@ Each persona posts independently with its own sentinel, so reviews don't interfe
|
||||
|
||||
### Custom Personas
|
||||
|
||||
Create a YAML file with your domain-specific review focus:
|
||||
Create a JSON 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
|
||||
```json
|
||||
// .review/personas/trading.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:
|
||||
@@ -420,18 +414,17 @@ Use it in CI:
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: trading
|
||||
persona-file: .review/personas/trading.yaml
|
||||
persona-file: .review/personas/trading.json
|
||||
...
|
||||
```
|
||||
|
||||
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) |
|
||||
| Structured format | Yes (JSON) | No (freeform) |
|
||||
| Focus/ignore lists | Yes | Manual |
|
||||
| Severity calibration | Yes | Manual |
|
||||
| Header display name | Yes | No |
|
||||
|
||||
@@ -103,11 +103,13 @@ func TestValidateWorkspacePath(t *testing.T) {
|
||||
errMatch: "resolves outside workspace",
|
||||
},
|
||||
{
|
||||
name: "absolute path gets normalized to relative",
|
||||
name: "absolute path normalized to workspace-relative",
|
||||
workspace: tmpDir,
|
||||
path: "/etc/passwd",
|
||||
wantErr: true,
|
||||
errMatch: "failed to resolve", // filepath.Join strips leading / making it <workspace>/etc/passwd which doesn't exist
|
||||
// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
|
||||
// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
|
||||
errMatch: "failed to resolve",
|
||||
},
|
||||
{
|
||||
name: "nonexistent file",
|
||||
@@ -152,7 +154,6 @@ func TestValidateWorkspacePath(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
|
||||
|
sonnet-review-bot
commented
[NIT] Double blank line after the closing brace of TestValidateWorkspacePath before the makeReview helper. Minor style inconsistency. **[NIT]** Double blank line after the closing brace of TestValidateWorkspacePath before the makeReview helper. Minor style inconsistency.
|
||||
r := gitea.Review{
|
||||
|
sonnet-review-bot
commented
[NIT] There is a double blank line between **[NIT]** There is a double blank line between `TestValidateWorkspacePath` and `makeReview`. Consistent single blank lines between test functions would match the rest of the file.
|
||||
ID: id,
|
||||
@@ -164,7 +165,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
|
||||
return r
|
||||
}
|
||||
|
||||
|
||||
func TestBuildSupersededBody(t *testing.T) {
|
||||
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
||||
sentinel := "<!-- review-bot:sonnet -->"
|
||||
@@ -734,8 +734,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) {
|
||||
{"<!-- review-bot:sonnet --> rest", "sonnet"},
|
||||
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
|
||||
{"no sentinel here", "unknown"},
|
||||
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
|
||||
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
|
||||
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
|
||||
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
# Design: Role-based Review Personas (Issue #51)
|
||||
|
gpt-review-bot
commented
[NIT] The design doc initially describes YAML personas before noting a switch to JSON at the end. This could confuse readers; consider moving the JSON revision note to the top or removing YAML snippets. **[NIT]** The design doc initially describes YAML personas before noting a switch to JSON at the end. This could confuse readers; consider moving the JSON revision note to the top or removing YAML snippets.
sonnet-review-bot
commented
[NIT] The design doc still references YAML throughout (phase checklist, finding #2, struct tags use **[NIT]** The design doc still references YAML throughout (phase checklist, finding #2, struct tags use `yaml:` annotations) even after the revision section at the bottom switched to JSON. The checklist items reference `security.yaml`, `architect.yaml`, `docs.yaml`. These inconsistencies are confusing for future readers. The doc should be updated to consistently reflect the JSON decision.
sonnet-review-bot
commented
[NIT] The design doc still references YAML in several places (e.g. the Persona struct uses **[NIT]** The design doc still references YAML in several places (e.g. the Persona struct uses `yaml:` tags, the design says 'personas/*.yaml', the completion checklist asks 'Persona struct matches YAML schema exactly?') even though the design was revised to JSON. The doc is internally inconsistent. Since this is a docs file it doesn't affect correctness, but it will confuse future readers.
sonnet-review-bot
commented
[NIT] The design doc references YAML throughout (personas/*.yaml, gopkg.in/yaml.v3) before the revision section at the bottom switches to JSON. The doc would be cleaner if the YAML references in the early sections were updated to reflect the final JSON decision, rather than leaving the revision as an addendum. As a static doc this is low priority. **[NIT]** The design doc references YAML throughout (personas/*.yaml, gopkg.in/yaml.v3) before the revision section at the bottom switches to JSON. The doc would be cleaner if the YAML references in the early sections were updated to reflect the final JSON decision, rather than leaving the revision as an addendum. As a static doc this is low priority.
sonnet-review-bot
commented
[NIT] The design doc still references YAML in several places (Persona struct with **[NIT]** The design doc still references YAML in several places (Persona struct with `yaml:` tags, `.review/personas/trading.yaml`, `security.yaml` phase items) after the JSON revision was made. These are internal doc inconsistencies but worth cleaning up for accuracy.
sonnet-review-bot
commented
[NIT] The design document still references YAML as the persona format in multiple places (e.g., the **[NIT]** The design document still references YAML as the persona format in multiple places (e.g., the `Persona struct` Go code block uses `yaml:` struct tags, the persona file format sections show YAML examples). The design revision section at the bottom explains the switch to JSON, but the earlier sections weren't updated. This creates confusion for anyone reading the doc top-to-bottom. Consider updating or striking through the YAML sections, or noting "see Design Revision below" at the first YAML mention.
sonnet-review-bot
commented
[NIT] The design document mentions **[NIT]** The design document mentions `gopkg.in/yaml.v3` but the implementation uses `github.com/goccy/go-yaml`. The design doc's 'Design Revision' section should be updated to reflect the actual library chosen, or the implementation should use gopkg.in/yaml.v3 as designed.
|
||||
|
||||
> **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)
|
||||
```
|
||||
|
||||
|
||||
@@ -1,5 +1,3 @@
|
||||
module gitea.weiker.me/rodin/review-bot
|
||||
|
||||
go 1.26.2
|
||||
|
||||
require github.com/goccy/go-yaml v1.19.2
|
||||
|
||||
@@ -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=
|
||||
@@ -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))
|
||||
}
|
||||
|
||||
|
||||
@@ -5,37 +5,34 @@ import (
|
||||
"encoding/json"
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `path/filepath` is imported but only used for `filepath.Join("personas", filename)` in `LoadBuiltinPersona`. Since the path separator for embedded FS is always `/` (regardless of OS), `embed.FS` paths must use forward slashes per the `io/fs` spec. Using `filepath.Join` here works on Linux/macOS but would produce `personas\security` on Windows if that ever becomes a build target. Using `"personas/" + filename` directly would be safer and removes the need for the `path/filepath` import.
|
||||
"fmt"
|
||||
"os"
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `path/filepath` is imported and used in `LoadBuiltinPersona` to construct `filepath.Join("personas", filename)`. On Windows this would produce `personas\filename.json`, but `embed.FS` always uses forward slashes. This should use `"personas/" + filename` directly (or `path.Join` from the `path` package, not `filepath`). This is a correctness bug on non-Unix platforms, though it won't affect Linux CI runners.
|
||||
"path/filepath"
|
||||
"strings"
|
||||
|
||||
"github.com/goccy/go-yaml"
|
||||
"unicode/utf8"
|
||||
)
|
||||
|
||||
//go:embed personas/*.yaml
|
||||
//go:embed personas/*.json
|
||||
|
sonnet-review-bot
commented
[MAJOR] Imports gopkg.in/yaml.v3 in violation of the zero-external-dependencies convention. The parsePersona function should use only encoding/json. Built-in personas stored as .yaml files should either be converted to .json or parsed with a minimal hand-rolled YAML subset — though the simplest fix consistent with the design document's own resolution is to store built-in personas as .json and remove the yaml.v3 import entirely. **[MAJOR]** Imports gopkg.in/yaml.v3 in violation of the zero-external-dependencies convention. The parsePersona function should use only encoding/json. Built-in personas stored as .yaml files should either be converted to .json or parsed with a minimal hand-rolled YAML subset — though the simplest fix consistent with the design document's own resolution is to store built-in personas as .json and remove the yaml.v3 import entirely.
|
||||
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"`
|
||||
|
gpt-review-bot
commented
[MAJOR] Imports gopkg.in/yaml.v3 and parses YAML personas, conflicting with the project’s zero-dependency requirement. Either remove YAML support or replace with JSON-only parsing. **[MAJOR]** Imports gopkg.in/yaml.v3 and parses YAML personas, conflicting with the project’s zero-dependency requirement. Either remove YAML support or replace with JSON-only parsing.
|
||||
DisplayName string `json:"display_name"`
|
||||
|
gpt-review-bot
commented
[MINOR] Persona.ModelPref is parsed but not used anywhere. Consider documenting intended usage or removing until needed to reduce API surface. **[MINOR]** Persona.ModelPref is parsed but not used anywhere. Consider documenting intended usage or removing until needed to reduce API surface.
|
||||
ModelPref string `json:"model_preference,omitempty"`
|
||||
|
gpt-review-bot
commented
[NIT] Persona.ModelPref (model_preference) is defined but not used anywhere. Consider either wiring it into model selection or removing it to avoid confusion. **[NIT]** Persona.ModelPref (model_preference) is defined but not used anywhere. Consider either wiring it into model selection or removing it to avoid confusion.
|
||||
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"`
|
||||
}
|
||||
|
[MAJOR] LoadPersona reads an arbitrary file path from the local filesystem without any path normalization, workspace boundary checks, or symlink resolution. In CI, a misconfigured workflow or malicious change could cause the bot to read files outside the repository workspace and include their contents (via persona fields) in the LLM system prompt, risking data exfiltration. Mirror the strict workspace + symlink checks used for system-prompt-file. **[MAJOR]** LoadPersona reads an arbitrary file path from the local filesystem without any path normalization, workspace boundary checks, or symlink resolution. In CI, a misconfigured workflow or malicious change could cause the bot to read files outside the repository workspace and include their contents (via persona fields) in the LLM system prompt, risking data exfiltration. Mirror the strict workspace + symlink checks used for system-prompt-file.
[MINOR] Persona files from the repository are read and unmarshaled without an explicit size limit. A very large YAML/JSON persona file could cause elevated memory/CPU usage during os.ReadFile and yaml/json unmarshaling, enabling a potential CI resource exhaustion vector. Consider enforcing a maximum file size before reading/unmarshaling and rejecting oversized files. **[MINOR]** Persona files from the repository are read and unmarshaled without an explicit size limit. A very large YAML/JSON persona file could cause elevated memory/CPU usage during os.ReadFile and yaml/json unmarshaling, enabling a potential CI resource exhaustion vector. Consider enforcing a maximum file size before reading/unmarshaling and rejecting oversized files.
|
||||
|
||||
// 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
|
||||
|
gpt-review-bot
commented
[NIT] ListBuiltinPersonas returns nil on error, which may require nil checks at call sites. Returning an empty slice is often more ergonomic and safer. **[NIT]** ListBuiltinPersonas returns nil on error, which may require nil checks at call sites. Returning an empty slice is often more ergonomic and safer.
|
||||
if err != nil {
|
||||
available := ListBuiltinPersonas()
|
||||
@@ -57,10 +54,11 @@ func LoadBuiltinPersona(name string) (*Persona, error) {
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `LoadBuiltinPersona` calls `ListBuiltinPersonas()` to build an error message only on failure. This means if `embeddedPersonas.ReadFile` fails AND `embeddedPersonas.ReadDir` (inside `ListBuiltinPersonas`) also fails, the error message will be `unknown built-in persona "x" (available: )` with an empty list — which is confusing. Consider hardcoding the known persona names in the error message or handling the ReadDir error in ListBuiltinPersonas more gracefully.
|
||||
// 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")
|
||||
|
sonnet-review-bot
commented
[MINOR] The inline comment on **[MINOR]** The inline comment on `embeddedPersonas.ReadFile("personas/" + filename)` says 'embed.FS paths use forward slashes per io/fs spec'. While correct and useful, this comment is placed on the wrong line — it belongs on the `ReadFile` call line (line 59), which is where it appears. This is fine. However, the function silently falls through to listing available personas when the embedded file is not found, which means a typo in the name produces an error message that lists alternatives. This is good UX, but the `ListBuiltinPersonas()` call inside `LoadBuiltinPersona` adds a small O(n) directory scan on every failed lookup — not a real concern at this scale but worth noting.
|
||||
if err != nil {
|
||||
|
sonnet-review-bot
commented
[MINOR] **[MINOR]** `LoadBuiltinPersona` calls `ListBuiltinPersonas()` to build an error message when the persona isn't found. However, `ListBuiltinPersonas` reads the directory from the embedded FS — if that somehow fails, it returns `nil` and the error message becomes 'unknown built-in persona "foo" (available: )'. This is a minor usability issue, but it would be better to hardcode the list of known personas or at least handle the empty-list case.
|
||||
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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] ListBuiltinPersonas silently returns nil when ReadDir fails. The caller (LoadBuiltinPersona error message) would then say 'available: ' with no names, which is confusing. Consider logging or returning an error, or at minimum ensuring the error path doesn't produce a misleading 'available: ' suffix in the error message. **[MINOR]** ListBuiltinPersonas silently returns nil when ReadDir fails. The caller (LoadBuiltinPersona error message) would then say 'available: ' with no names, which is confusing. Consider logging or returning an error, or at minimum ensuring the error path doesn't produce a misleading 'available: ' suffix in the error message.
|
||||
return nil, fmt.Errorf("parse persona %s: %w", source, err)
|
||||
|
sonnet-review-bot
commented
[NIT] Trailing whitespace after the **[NIT]** Trailing whitespace after the `var p Persona` declaration (blank line with spaces before the comment). Run `gofmt`.
|
||||
}
|
||||
|
||||
if err := validatePersona(&p, source); err != nil {
|
||||
return nil, err
|
||||
|
sonnet-review-bot
commented
[NIT] There is a trailing whitespace after **[NIT]** There is a trailing whitespace after `var p Persona` before the blank line. Minor style issue.
|
||||
}
|
||||
@@ -112,3 +97,16 @@ func validatePersona(p *Persona, source string) error {
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `CapitalizeFirst` is placed in `persona.go` but is used by `formatter.go`. It's a general string utility that doesn't have a conceptual relationship with persona loading. It would be more discoverable in `formatter.go` or a shared `strings.go`/`util.go` file within the `review` package.
|
||||
return nil
|
||||
}
|
||||
|
||||
|
[MINOR] Persona YAML is parsed without limits using a third‑party YAML library. If a workflow references a persona file from the PR branch, a malicious PR could supply a very large or adversarial YAML (e.g., anchor expansion) to cause excessive CPU/memory consumption during unmarshalling. Consider enforcing a maximum file size before reading/parsing and/or preferring JSON (stdlib) for untrusted inputs. **[MINOR]** Persona YAML is parsed without limits using a third‑party YAML library. If a workflow references a persona file from the PR branch, a malicious PR could supply a very large or adversarial YAML (e.g., anchor expansion) to cause excessive CPU/memory consumption during unmarshalling. Consider enforcing a maximum file size before reading/parsing and/or preferring JSON (stdlib) for untrusted inputs.
|
||||
// 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:]
|
||||
}
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `contains` / `containsHelper` functions in persona_test.go are reinventing `strings.Contains`. The standard library already provides this. The custom implementation is error-prone (the `contains` wrapper has a subtle redundancy: it checks `len(s) >= len(substr)` and `s == substr` before calling `containsHelper`, which already handles the equality case in its loop). Just use `strings.Contains` directly.
|
||||
|
||||
func TestLoadPersonaInvalidYAML(t *testing.T) {
|
||||
func TestLoadPersonaInvalidJSON(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `contains` / `containsHelper` helpers duplicate functionality from `strings.Contains`. The test file is in the `review` package (not `review_test`), so it could just use `strings.Contains` directly — or at minimum import `strings`. Adding custom substring matching helpers in tests is unnecessary complexity and a maintenance burden.
|
||||
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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `contains` and `containsHelper` helpers re-implement `strings.Contains` from scratch. This is unnecessary — `strings.Contains` is already in the standard library and is used everywhere else in the test files. This also violates the principle of using standard library functions.
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `contains` and `containsHelper` helpers are reimplementing `strings.Contains`, which is already available in the standard library. The helpers add no value and the custom implementation has a subtle bug: `containsHelper` is only called when `len(s) > 0`, but the outer `contains` check `len(s) >= len(substr)` already handles the empty-substr case. Just use `strings.Contains` directly — it handles all edge cases correctly and is idiomatic Go.
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `contains` and `containsHelper` helpers are reinventing `strings.Contains`. The project already imports `strings` elsewhere and the stdlib `strings.Contains` is available in test files. Using `strings.Contains` directly would be clearer and eliminate ~10 lines of custom code.
|
||||
}
|
||||
|
||||
_, 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)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `TestLoadPersonaInvalidYAML` uses `"not: valid: yaml: here"` as invalid YAML, but this is actually valid YAML (it's a key `not` with value `valid: yaml: here` as a string). The test may pass only because the YAML parses to a struct with empty required fields, triggering the validation error rather than a parse error. The test description says 'invalid YAML' but it's really testing 'YAML missing required fields'. The test still provides coverage but the intent is misleading.
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
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")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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"
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
@@ -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"
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
@@ -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"
|
||||
}
|
||||
}
|
||||
@@ -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"
|
||||
[NIT] The README custom persona example shows YAML format and says 'JSON format is also supported for backwards compatibility' — but this is a new feature with no prior format, so there's no backwards compatibility concern. After removing the yaml.v3 dependency and going JSON-only per the design document's own resolution, this sentence should be removed or changed to accurately describe which format is canonical.