diff --git a/docs/decisions/review-gate-in-chain.md b/docs/decisions/review-gate-in-chain.md index 8572eff..4681821 100644 --- a/docs/decisions/review-gate-in-chain.md +++ b/docs/decisions/review-gate-in-chain.md @@ -55,3 +55,4 @@ created: 2026-07-02 - 2026-08-05 — sc-1317: an autonomous report claiming completeness "hangs indefinitely" (90s of silence before an impatient interrupt) does NOT reproduce — that's inside the documented ~7min typical uncached-run latency, nowhere near DEEP_JUDGE_TIMEOUT_MS. But chasing it surfaced real, separate gaps in the shared exec helper. (1) execJudge/execJudgeAsync's timeout relied on execFile/execFileSync's default killSignal (SIGTERM), which a child can trap or ignore — sc-1227 single-sourced the cap's VALUE but never guaranteed its ENFORCEMENT. Both now pass killSignal: 'SIGKILL' (mirrors the existing co-occurrence/index-refresh.mts precedent); isJudgeTimeout's err.killed === true check is unaffected (Node sets it on any timeout-kill regardless of signal), so outage classification and the timeout/sync/outage remedy split are unchanged. (2) execJudgeAsync had no try/catch around its own execFile() call, unlike its sync execJudge twin — a synchronous throw (e.g. an out-of-range timeout value, reproduced with timeout: -1) escaped as a REJECTED promise, breaking the function's own documented 'never throws, always resolves null' contract for any unguarded awaiter (completeness.mts's own await exec(...) call sits outside its try/catch). Fixed by wrapping the executor body in try/catch, routed through the same outage-classification path the callback error branch already used. (3) ExecJudgeOpts.timeout flips from optional to required — Node treats an omitted/0 timeout as 'no cap enforced at all', which would silently defeat the whole SIGKILL guarantee; every real caller already supplied one, so this closes the gap at the type level with no behavior change. Also documented (test-only, no code change — no real caller is exposed): timeout: 0 disables the cap rather than killing instantly, and timeout: Number.MAX_SAFE_INTEGER silently overflows Node's 32-bit timer to ~1ms and kills almost immediately, the opposite of a caller's likely intent. Follow-up not in scope here: whether a claude -p judge's own MCP-subprocess children could survive killing just the immediate process (would need detached + process-group kill to close, unverified whether it's a real risk on this path). - 2026-08-07 — PR #360 — on the ship path completeness no longer waits for commit-msg: the pre-commit review fragment starts it in PARALLEL with the reviewer fleet (armed only by the sc-1442 message file, so interactive commits and devkit review are unchanged) and applies the identical exit contract there. commit-msg then re-judges it from cache. Its PASS scope is ruled by [[judge-verdict-cache-scope]]. The cancelled-judge path had to background a SIMPLE command, not the __dk_no_git_env function: backgrounding a function forks a subshell, so $! was a wrapper and the judge survived kill+wait as an orphan still holding git's output pipe. - 2026-08-09 — sc-1465 follow-up — devkit review's run log is now SELF-TERMINATING: the EXIT trap stamps one terminal line (result= one of passed/skipped/failed/timeout/signaled, plus exit= and phase=) into .devkit/review-runs/.log, and review_phase() streams a phase= t=s line as each step begins. The MACHINE terminal was already ruled and shipped (2026-07-18 note: one review_run_result carrying the final exit); this closes the HUMAN artifact gap, not a second telemetry channel. Motivation: a P1 report read a 152-byte header-only log as a silent false pass on an unreviewed diff. Of ~40 exit sites in review-target.sh only three wrote to the log, so every node-helper preflight failure (setup capture, repository state, verify_target_capture, setup runtime) aborted before the terminal verdict block and left a stub indistinguishable from a killed run. Emitted from the trap rather than tee'd at each exit so no future exit path can miss it; best-effort under set +e because a failed log write must never alter a decided verdict. The report's exit-code half did NOT reproduce: drift is fail-closed at exit 1 through setup-manifest fail() -> run-direct exitCode 1 -> set -e abort -> trap, and a CLI-level regression test now pins that end to end. Rejected --allow-hooks-drift: a green verdict earned against the wrong hook set proves nothing, which is strictly worse than failing loudly. Separately closed a real fail-open behind the report's shape: runDirectReviewCli returns silently (exit 0) when its realpath entrypoint guard misses, so review-target.sh now asserts the setup and repository manifests are non-empty instead of trusting a helper's exit code. +- 2026-08-10 — **Amends:** note:2026-08-09 — Correction to the sc-1465 follow-up note above: its 'sc-1465' reference is WRONG and is withdrawn. sc-1465 is an unrelated bug — core.hooksPath is shared across linked worktrees and resolves to an absolute path under the MAIN checkout, so a worktree's commits run the main checkout's pre-commit hook instead of its own committed copy; its acceptance criterion is a per-worktree override (git config --worktree core.hooksPath with extensions.worktreeConfig), still in flight on fix/sc-1465-hookspath-owner and NOT done. None of that is touched by the self-terminating run log, the phase stamping, or the setup/repository manifest fail-closed assertions the amended note describes. The number was inferred from a local worktree DIRECTORY NAME (.claude/worktrees/sc-1465-hookspath-owner) without reading the story, and PR #377 shipped carrying it. The real origin of that work is an autonomous P1 report claiming devkit review exits 0 on a drifted core.hooksPath — exit-code half did not reproduce, log-opacity half did. Reusable lesson: a worktree directory name is not a ticket — read the story before citing it.