[NIT] The subprocess side of TestMainSubprocess_InvalidDocMapPath calls main() and returns, but GITHUB_WORKSPACE is set by the outer test via the environment string "GITHUB_WORKSPACE="+t.TempDir(). The subprocess process inherits that env var, so the temp dir created in the outer test process is passed as a string to the child — this is correct. Just worth noting that the outer t.TempDir() is evaluated in the outer process before cmd.Env is built, so it is a real, existing directory. No bug, just confirming the intent.
Sonnet Review
[NIT] The flag description for --doc-map-trusted-ref is a long inline string literal. Style-wise this is consistent with other flags in the file so no change needed, but it's worth noting that very long flag descriptions can be hard to read in --help output.
[NIT] The doc comment for ParseDocMapConfigContent says 'e.g. "main:main@"' but the actual format used in main.go is 'owner/repo@ref:path'. Minor doc inconsistency, no functional impact.
[NIT] repoDir := os.TempDir() in the subprocess branch uses the system temp dir directly rather than creating a unique temp dir. Since nonexistent-doc-map.yml is unlikely to exist there in practice this works, but it's slightly fragile — if some other test or process creates that file in /tmp, the test would unexpectedly pass EvalSymlinks and fail the confinement check instead of the stat check. Using t.TempDir() would be safer, but t is not accessible in subprocess mode. A comment explaining the constraint would help.
[MINOR] The Lstat + ModeSymlink check after EvalSymlinks is acknowledged as 'defense-in-depth' in the comment, but filepath.EvalSymlinks already guarantees the returned path is fully resolved with no symlinks. The comment correctly explains this, but the dead check (ModeSymlink can never be set on a fully-resolved path) adds noise. It's not incorrect, just permanently unreachable code that could mislead future readers into thinking the check provides real protection here.
[NIT] The updated TestValidateDocmapPath_Symlink test no longer covers the case of an in-repo symlink (one whose target is within the repo root), which is now the allowed case per the semantic change. A brief comment or a second sub-case showing that an in-repo symlink is accepted would make the changed behavior explicit and prevent a future developer from re-hardening against it unnecessarily.
[NIT] Both new subprocess tests pass the same set of required flags verbatim (copy-paste). If the required-flag set changes, both tests need updating. This is a minor maintainability concern, not a correctness issue — a shared helper (like the existing cleanEnv pattern) could be used to build the base args slice, but it's low priority given the established pattern in the file.
[NIT] The early validation at line 176 calls validateWorkspacePath and discards the resolved path (using _). Then at the later doc-map processing block (Step 6c), validateWorkspacePath is called again on the same path. This is a double call to EvalSymlinks on the happy path. Consider storing the resolved path in a variable (e.g., a map or a local var) to avoid the redundant syscall, or restructure so the early check and the later usage share the resolved result.
Sonnet Review
[NIT] The architecture diagram in SKILL.md says the dispatch script 'exits after emitting one SPAWN line', but the spec (docs/dev-loop-spec.md §4) says HANDOFF lines continue without exiting, and the diagram itself also says 'emits HANDOFF for each qualifying PR (does not exit after HANDOFF)'. The 'exits' phrasing is slightly misleading — it only exits early for SPAWN, not HANDOFF. This is not wrong but could be clarified to 'exits after emitting one SPAWN line (but continues for HANDOFF)' or similar.
[NIT] The dispatch rules table in SKILL.md says Rule 10 condition is 'All checks pass' but the corresponding rule in docs/dev-loop-spec.md §5 (Rule 10) also requires verifying all bot reviews are current. The SKILL.md table is a summary so this simplification is acceptable, but a note like 'All checks pass + bot reviews current' would be more precise.
[NIT] Rule 10 says 'If already assigned to aweiker: skip (assume handoff was already performed; continue to next PR without emitting another HANDOFF).' This hardcodes the reviewer username. Consider referencing a config field (e.g., reviewer in the project config) rather than a literal name, both for documentation accuracy and maintainability.
[NIT] SKILL.md states the dispatch script 'exits after one SPAWN action per pass', but docs/dev-loop-spec.md (Rule 10) clarifies that multiple HANDOFFs can be emitted in one pass and the script does NOT exit after HANDOFF. The architecture diagram in SKILL.md is slightly misleading — it implies the script always exits after one action, when in reality it continues processing PRs after HANDOFF emissions.
[NIT] Rule 10 states 'If already assigned to aweiker: skip.' — it's unclear whether this means skip the HANDOFF emission only, or skip applying the ready label too. The spec would benefit from a clarifying sentence, e.g. 'If the PR is already assigned to aweiker, assume handoff was already performed and continue to the next PR without emitting another HANDOFF.'