[MINOR] Duplicate doc comment on getDeploymentURL. The function has two consecutive doc comment blocks — one starting 'getDeploymentURL returns the deployment URL...' appears twice. The second block is the authoritative one (it includes the token-return note and the concurrency caveat), but the first redundant line should be removed.
[NIT] The isAICore variable is computed at line 94 before flag parsing conceptually completes (flag.Parse() is at line 82, so it's fine), but the comment '// Validate required fields' block is now split in two places — the early check and then the switch below. The isAICore variable is also used implicitly for provider routing. Consider consolidating or at least adding a comment noting that isAICore is derived from the already-parsed flag.
[NIT] In TestAICoreClient_DeploymentFetch, when two RUNNING deployments exist for gpt-5, the test asserts the second one (deploy-789) is returned. The current implementation happens to return deploy-789 because it overwrites deploy-456 during iteration, but this is iteration-order dependent on the JSON response. The test comment 'Should find running gpt-5, not stopped one' is accurate, but the assertion on the exact URL (deploy-789 vs deploy-123 for the other case) implicitly depends on the map-overwrite behavior. This is fine given the deterministic JSON array order from the test server, but worth a comment.
[NIT] TestAICoreClient_TokenExpiry sets expires_in: 1 in the server response but never actually waits for the token to expire naturally — it directly manipulates client.tokenExpiry. The server-side expires_in: 1 is misleading; it could just be any value since the test bypasses natural expiry. Consider using a comment or a more clearly intentional value (e.g., 3600) to avoid confusion.
[MINOR] When the deployment cache lookup succeeds on the RLock path, getToken is called again (acquiring locks again), even though a fresh token was already obtained if this was just populated. This is a minor inefficiency — not a correctness issue — but the fast-path could return the already-fetched token from the cache population path. The current structure is correct but slightly suboptimal.
[MINOR] BuildPersonaSystemPrompt builds the JSON output format spec using ~20 individual sb.WriteString calls on string literals. This duplicates logic that already exists in BuildSystemBase() (or the base prompt builder). A shared constant or helper for the JSON output spec would reduce the risk of the persona and generic prompts diverging silently.
[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.
[NIT] FormatMarkdownWithDisplay duplicates nearly all of FormatMarkdown — the only differences are the header/footer name split. Consider refactoring FormatMarkdown to call FormatMarkdownWithDisplay(result, reviewerName, reviewerName) to keep the two in sync, or extract the shared table-building logic.
[NIT] The path/filepath import is not in alphabetical order relative to the other imports (os, os/exec, strings). goimports would place it before os. Minor style issue.
[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.
[MINOR] There is a blank line immediately after the os.Exit(1) block (the required-fields validation) and before the persona-flags validation comment. This is a minor style issue (double blank line) that gofmt would not flag but deviates from the existing code style where single blank lines separate logical sections.
[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.
[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.
[MINOR] The YAML for the system-prompt-file input is malformed — the persona and persona-file inputs are inserted between the system-prompt-file key and its description/required/default fields. This means system-prompt-file loses its description/metadata in the action definition. The two new inputs should be placed after the complete system-prompt-file block, not in the middle of it.
[NIT] BuildSystemPromptWithPersona exists as a helper but is never called from main.go — the main function manually assembles systemBase then appends additionalPrompt, patterns, and conventions separately via the budget system. This exported function is unused dead code in practice. Either wire it into the main flow or unexport/remove it.
[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.
[NIT] FormatMarkdownWithDisplay is nearly identical to FormatMarkdown — the only difference is the two-name parameter split. Consider refactoring FormatMarkdown to delegate to FormatMarkdownWithDisplay(result, reviewerName, reviewerName) to avoid duplicating the findings table and recommendation rendering logic.