From f1f0d8bbc1bec8a333c194987453fb49006b88c1 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Mon, 10 Aug 2026 09:26:20 +0100 Subject: [PATCH] docs(decisions): withdraw the wrong sc-1465 reference from PR #377's note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #377 shipped its decision note under the header `sc-1465 follow-up`. That reference is wrong and is withdrawn here. **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` enabled). That work is in flight on `fix/sc-1465-hookspath-owner` and is **not** done. Nothing in #377 touches it — the self-terminating run log, the phase stamping, and the setup/repository manifest fail-closed assertions are all unrelated to worktree hook resolution. **How it happened:** the ticket number was inferred from a local worktree directory name (`.claude/worktrees/sc-1465-hookspath-owner`) without reading the story. The real origin of that work is an autonomous P1 report claiming `devkit review` exits 0 on a drifted `core.hooksPath` — whose exit-code half did not reproduce, and whose log-opacity half did. Recorded as an append-only correction rather than an edit, per the store's own rule. Docs-only; no behaviour change. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- docs/decisions/review-gate-in-chain.md | 1 + 1 file changed, 1 insertion(+) 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.