Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/decisions/review-gate-in-chain.md
Original file line number Diff line number Diff line change
Expand Up @@ -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/<run-id>.log, and review_phase() streams a phase=<name> t=<elapsed>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.
Loading