[NIT] The comment // t.TempDir() is evaluated here in the outer process, producing a real directory / that is passed as the GITHUB_WORKSPACE env var string to the subprocess. is duplicated verbatim in both new test functions. It's accurate and useful, but could be a shared helper (e.g. workspaceEnv(t)) to avoid repeating the same comment+pattern four times total (two existing tests already use similar patterns). Minor — not worth blocking.
Sonnet Review
[NIT] The resolvedDocMapFile variable is declared at a wide scope and left as the zero value (empty string) when *docMapTrustedRef != "". It is then used later in the else branch of the trusted-ref check. This is correct — the empty string is only used in the else branch — but it is slightly fragile. A future refactor could accidentally use resolvedDocMapFile in the trusted-ref path. A comment noting that resolvedDocMapFile is intentionally empty when docMapTrustedRef is set would improve clarity, or the variable could be moved into the else block. No bug here.
[NIT] The ParseDocMapConfigContent function converts a string to []byte with []byte(content) before passing to parseDocMapBytes. This is a trivial allocation but the function signature could alternatively accept []byte directly. This is a minor style point — the current string signature is ergonomic for callers that have a string from GetFileContentRef, so this is acceptable as-is.
🤖 AI Review — PR #153 (Config B: Opus investigates, GPT-5 judges)
Multi-Model AI Review — PR #153 (Config B: Opus investigates, GPT-5 judges)
Review — Config B (Opus investigates, GPT-5 judges)
[MINOR] The mutation loop for InvalidPRNumber searches for the string "1" to replace with "notanumber". This is fragile: if baseSubprocessArgs() ever gains another argument whose value is "1" (e.g. a count flag), the wrong argument would be mutated. A more robust approach would be to search for the flag name "--pr" and replace the element at i+1, as is done in InvalidRepo (which looks for "owner/repo" — a unique value). The same concern applies to the InvalidPRNumber test. Low risk today but worth noting.
[NIT] The doc comment for ParseDocMapConfigContent uses "main:main@<ref>" as the example source string, but the actual usage in main.go formats it as "owner/repo@ref:path". The example is slightly misleading — minor documentation inconsistency.
[NIT] The early validation of --doc-map path now happens before client initialization (good for fail-fast), but when --doc-map-trusted-ref is set, the resolvedDocMapFile variable is never used (the trusted-ref path fetches from VCS, ignoring the local file). This is correct behavior but the variable name resolvedDocMapFile implies a local file will be used. A comment clarifying that this is only used in the local-workspace fallback path would improve readability.
[NIT] The os.Lstat + ModeSymlink check after filepath.EvalSymlinks is documented as 'defense-in-depth', but filepath.EvalSymlinks guarantees the returned path contains no symlinks — the ModeSymlink bit can never be set on the result. The check is harmless but the accompanying comment calling it 'defense-in-depth' is misleading because it can never fire. Consider either removing the dead check or replacing the comment with a more accurate note like '// EvalSymlinks guarantees this is unreachable; kept for belt-and-suspenders'.