[MINOR] Duplicate doc comment block: getDeploymentURL has its doc comment written twice in succession (lines 157-169 of aicore.go). The first copy should be removed.
[MINOR] The comment says 'displayName is not HTML-escaped as Gitea sanitizes rendered Markdown' and 'Persona display names are controlled by repo owners (trusted input)'. However, displayName can come from a --persona-file whose path is validated for workspace containment but whose contents (the display_name field) are not sanitized. A persona file checked into a repo could contain something like display_name: "] Review\n\n<script> in a pathological case. Since Gitea does sanitize Markdown-rendered HTML, this is low risk, but the trust assertion in the comment ('controlled by repo owners') is slightly inaccurate — it could also come from any file a developer adds to the repo. The comment should be more precise or dropped.
[MINOR] The design document shows a YAML persona file format as the primary example (with # .review/personas/security.yaml header and YAML syntax like name: security) but the implementation uses JSON. The design revision note at the bottom explains this, but the main body of the document is misleading — it still shows YAML syntax in the code block under '2. Persona File Format'. This will confuse future contributors reading the design doc.
[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.
[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.
[NIT] BuildSystemPromptWithPersona is exported but not used in main.go — main.go calls BuildPersonaSystemPrompt directly and then appends patterns/conventions via the budget system. This exported function implements a different (non-budget-aware) path. It's only tested, not used in production. Either document clearly that it's an alternative API for callers who don't need the budget system, or make it unexported if it's just for testing convenience.
[NIT] The TestValidateWorkspacePath test creates a symlink to /etc/passwd and doesn't have OS-specific guards. On Windows, os.Symlink to /etc/passwd would fail or behave differently. The existing tests in the repo don't appear to target Windows, so this is likely acceptable, but it's worth noting for cross-platform CI.
[MINOR] The comment says displayName is not HTML-escaped as Gitea sanitizes rendered Markdown. Persona display names are controlled by repo owners (trusted input). While display names are set by repo owners (trusted), they come from YAML files that could be committed by contributors. If a malicious contributor adds a display_name containing --> or similar, it could break the sentinel comment or inject content into the review body. The sentinel is protected (uses sentinelName from reviewerName which is validated), but the header # <displayName> Review is unescaped. This is low severity given Gitea sanitizes Markdown, but the trust model statement in the comment is slightly overstated.
[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.
[NIT] BuildSystemPromptWithPersona is exported and documented as a convenience wrapper, but it's not used anywhere in the codebase (main.go assembles the prompt manually). This creates a parallel code path that could diverge. Either use it in main.go or make it unexported/remove it to avoid maintenance burden.
[MAJOR] The project's stated convention is 'Go standard library only — no external dependencies,' but this PR adds github.com/goccy/go-yaml v1.19.2. The design doc mentions gopkg.in/yaml.v3 but the implementation uses a different third-party library. YAML parsing can be done with the stdlib by requiring JSON format for all persona files, or by writing a minimal YAML-subset parser. Alternatively, the project convention must be explicitly updated before adding dependencies.
[MINOR] The parsePersona function uses filepath.Ext(source) to decide the format, but for built-in personas the source is "builtin:security" which has no file extension — it correctly falls through to the YAML path. However, for user-provided files without a .json extension (e.g., .yml), it also falls through to YAML, which is correct behavior but is undocumented. More importantly, the comment says 'YAML is a superset of JSON' as justification for the fallback, but this is only approximately true and the actual behavior depends on the library. The logic would be clearer as: if JSON extension → JSON unmarshal, else → YAML unmarshal (which currently is what happens, but the comment is misleading).
[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.
[NIT] Trailing whitespace after the var p Persona declaration (blank line with spaces before the comment). Run gofmt.
[NIT] Trailing whitespace after the closing brace of the else block. Run gofmt.
[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.