[MINOR] The parsePersona function uses filepath.Ext(source) to detect the format, but for built-in personas the source is "builtin:security" (no file extension). The comment says 'YAML (also handles .yaml, .yml, and builtin: prefix)' but this only works by coincidence — filepath.Ext("builtin:security") returns "" which falls into the else-YAML branch. This is fragile. A more explicit check like source == ".json" or extracting the logic to handle the builtin: prefix explicitly would be clearer and more robust.
[MINOR] The comment on FormatMarkdownWithDisplay says 'displayName is not HTML-escaped as Gitea sanitizes rendered Markdown. Persona display names are controlled by repo owners (trusted input).' While the trust reasoning is sound for built-in personas, --persona-file allows the display name to come from any YAML file in the workspace. If a workspace file is somehow malicious (e.g., a compromised dependency's config), the unescaped display name could inject markdown. This is a low-risk but worth noting given the security context of this tool.
[NIT] There is a trailing whitespace after var p Persona before the blank line. Minor style issue.
[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.
[MAJOR] The repository conventions explicitly state 'Go standard library only — no external dependencies.' This PR adds github.com/goccy/go-yaml v1.19.2 as an external dependency. The design doc (docs/DESIGN-51-personas.md) even notes a 'Design Revision' acknowledging this decision, but the justification (multi-line strings, comments in YAML) does not override the stated project constraint. YAML parsing can be handled with the stdlib encoding/json package (JSON-only persona files) or by writing a minimal YAML subset parser, but bringing in an external library violates the project's zero-dependencies rule.
[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.
[MAJOR] Introduces gopkg.in/yaml.v3 as a dependency. The repository conventions explicitly state 'Go standard library only — no external dependencies.' The design document (DESIGN-51-personas.md) even documents this conflict under 'Design Revision: JSON Instead of YAML' and resolves to use JSON to maintain zero dependencies — but the implementation ignores that resolution and uses YAML anyway. The persona loading code must be changed to use only JSON, or the YAML dependency must be removed.
[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.
[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.
[NIT] Trailing whitespace on line 89 (blank line with spaces inside parsePersona before the ext detection block). Minor style issue.
[MINOR] parsePersona uses filepath.Ext(source) to detect JSON vs YAML, but for built-in personas the source is 'builtin:security' (no file extension), so it falls through to the YAML path. This works today only because yaml.v3 is a superset of JSON, but it's fragile: if YAML is replaced with JSON-only, built-in persona sources would need to be renamed (e.g., 'builtin:security.yaml') or the detection logic changed. The dispatch logic is unclear about its contract.
[MINOR] BuildSystemPromptWithPersona has the conventions and patterns arguments in the wrong order relative to BuildSystemBase's documented usage in the budget assembly (where patterns come before conventions in the prompt). More importantly, the argument order is (persona, conventions, patterns) but the caller in main.go assembles the prompt manually rather than using this function — making BuildSystemPromptWithPersona a dead export that duplicates logic without being used. Either use it in main.go or remove it.
[NIT] The JSON output format is written as a series of sb.WriteString calls with manually formatted JSON. This is the same pattern used in BuildSystemBase and is consistent, but the inline JSON template is duplicated between BuildPersonaSystemPrompt and BuildSystemBase. A shared constant or helper would reduce drift risk if the output schema ever changes.
[MINOR] The --persona-file flag description says 'Path to persona JSON file' but the implementation also accepts YAML. The description should say 'Path to persona JSON or YAML file' (or after the dependency fix, just 'JSON file').
[NIT] Double blank line after the closing brace of TestValidateWorkspacePath before the makeReview helper. Minor style inconsistency.
[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.
[NIT] The path/filepath import was added but placed out of order — the Go convention (and goimports) groups stdlib imports alphabetically. It should appear between "os/exec" and "strings", not after "strings". Minor but goimports would reorder this.
[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.
[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.