From 14d8fc18524ec22b55c38d937d3d989b167d230e Mon Sep 17 00:00:00 2001 From: norvalbv Date: Thu, 6 Aug 2026 22:44:12 +0100 Subject: [PATCH 1/2] perf(review): overlap the completeness judge with the fleet, make its PASS intent-sticky, cap 6 Three latency/cost changes to the ship gate chain, driven by telemetry (~/.devkit/telemetry/gate-events.jsonl): 1. Review concurrency default 3 -> 6. The correctness lens split schedules ~8 judge tasks on a backend commit; at 6 they run in at most two waves, so the fleet makespan approaches the slowest single lens instead of a 3-slot packing. The bound is subscription slots, not CPU/memory. 2. Parallel completeness prewarm (ship path). The sc-1442 message file already reaches pre-commit, so the review hook fragment now launches guard-review completeness alongside the fleet, applies the commit-msg fragment's exact exit contract, and lets the commit-msg gate re-judge from cache. The fleet blocking the commit kills the judge instead of waiting. Interactive commits (no message file) and review mode (same env, different meaning) are excluded. The verdict store merges under a lock, so the two concurrent writers cannot clobber each other. 3. Intent-sticky completeness PASS (cost ruling). The gate judges the MESSAGE's claims, so a retry whose diff was reshaped to satisfy another reviewer -- same branch, same message -- is not re-judged: a confident PASS is saved under a branch+message+brief key (version-salted) beside the exact-bytes key. FAILs are never sticky. normalizeCommitMessage() converges the ship temp file with git's cleanup=whitespace output so the two hooks compute identical keys. Net: opus completeness is paid once per branch+message across a retry chain, and a passing attempt's judged path drops from fleet+completeness serial to max(fleet-at-6, completeness). Co-Authored-By: Claude Opus 5 (1M context) --- .husky/pre-commit | 38 ++++++++++ cli/__tests__/husky-block-exec.test.mts | 56 ++++++++++++++- cli/lib/husky/husky-block.mts | 38 ++++++++++ dist/cli/lib/husky/husky-block.mjs | 38 ++++++++++ dist/gate-engine/review/completeness.mjs | 60 +++++++++++++--- dist/gate-engine/review/run-review.mjs | 4 +- dist/gate-engine/review/telemetry/timing.mjs | 9 ++- .../review/__tests__/run-review.test.mts | 69 +++++++++++++++++-- gate-engine/review/completeness.mts | 66 +++++++++++++++--- gate-engine/review/run-review.mts | 4 +- gate-engine/review/telemetry/timing.mts | 9 ++- 11 files changed, 352 insertions(+), 39 deletions(-) diff --git a/.husky/pre-commit b/.husky/pre-commit index 4bb354ba..a2102734 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -144,8 +144,31 @@ fi if __dk_gate_selected review; then # devkit:guard-review echo "๐Ÿ” Reviewer gate (headless domain judges)..." +# Ship path only (sc-1442 message file present): start the completeness judge NOW, in parallel +# with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the +# shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of +# opus overlaps the fleet instead of following it. Interactive commits (no message yet) are +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” +# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports +# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, +# not part of a range review. +comp_pid="" +if [ "${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "${DEVKIT_COMMIT_MSG_FILE:-}" ]; then + echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." + __dk_no_git_env node gate-engine/review/cli.mts completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! +fi rrc=0 __dk_no_git_env node gate-engine/review/cli.mts --gate || rrc=$? +crc=0 +if [ -n "$comp_pid" ]; then + if [ "$rrc" -eq 0 ] || [ "$rrc" -eq 2 ]; then + wait "$comp_pid" || crc=$? + else + # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is + # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + kill "$comp_pid" 2>/dev/null || true + fi +fi if [ "$rrc" -eq 1 ]; then echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -157,7 +180,22 @@ elif [ "$rrc" -ne 0 ] && [ "$rrc" -ne 2 ]; then echo " guard-review: unexpected exit $rrc โ€” blocking the commit." exit 1 fi +# Same exit contract as the commit-msg fragment (commit-msg-block.mts) โ€” a completeness verdict +# means the same thing regardless of WHERE it was judged, just earlier here. +if [ "$crc" -eq 1 ]; then + echo " Confirmed completeness gap (hard-by-default; findings above)." + echo " Fix the gap, or โ€” with the user's explicit OK โ€” GUARD_NO_COMPLETENESS=1 git commit ..." + exit 1 +elif [ "$crc" -eq 4 ]; then + echo " NOT a gate rejection โ€” no defect was named; the staged content itself is unreadable." + exit 1 +elif [ "$crc" -eq 3 ]; then + echo " guard-review completeness: judge unavailable โ€” strict ship mode failed closed." + echo " Check \`claude\` CLI auth/quota, then re-run devkit ship (cleared judgements are cached)." + exit 1 +fi # rrc 0 = pass/cached/nothing-to-do, rrc 2 = inconclusive (non-strict fail-open) โ†’ continue. +# crc 0 = pass (now cached for the commit-msg gate) / skipped, crc 2 = fail-open โ†’ continue. # /devkit:guard-review fi diff --git a/cli/__tests__/husky-block-exec.test.mts b/cli/__tests__/husky-block-exec.test.mts index 30bc8b8c..8c25b263 100644 --- a/cli/__tests__/husky-block-exec.test.mts +++ b/cli/__tests__/husky-block-exec.test.mts @@ -35,10 +35,17 @@ const hasDash = existsSync('/bin/dash'); function runHook( env = {}, selection = { biome: false, guards: ALL_GUARDS }, - { shell = 'sh', dirPrefix = 'dk-hook-exec-' } = {}, + { shell = 'sh', dirPrefix = 'dk-hook-exec-', shipMsg = false } = {}, ) { const home = mkdtempSync(join(tmpdir(), dirPrefix)); homes.push(home); + if (shipMsg) { + // The sc-1442 composed-message temp file a ship exports โ€” its presence arms the parallel + // completeness prewarm in the review fragment. + const msgf = join(home, 'ship-msg.txt'); + writeFileSync(msgf, 'feat: thing\n\nbody\n'); + env = { DEVKIT_COMMIT_MSG_FILE: msgf, ...env }; + } const bin = join(home, '.bun', 'bin'); mkdirSync(bin, { recursive: true }); writeFileSync( @@ -49,7 +56,7 @@ echo "$tool $*" >> "$HOME/calls.log" case "$tool" in guard-deterministic) exit \${DET_RC:-0};; guard-decisions) exit \${DEC_RC:-0};; - guard-review) exit \${REVIEW_RC:-0};; + guard-review) case "$1" in completeness) exit \${COMP_RC:-0};; *) exit \${REVIEW_RC:-0};; esac;; *) exit 0;; esac `, @@ -147,6 +154,51 @@ describe('assembled hook execution (stubbed bunx, sh -e)', () => { }); }); +describe('parallel completeness prewarm (ship message file present)', () => { + it('no DEVKIT_COMMIT_MSG_FILE โ†’ completeness never launched (interactive commits unchanged)', () => { + const r = runHook(); + expect(r.status).toBe(0); + expect(r.calls).toContain('guard-review --gate'); + expect(r.calls).not.toContain('guard-review completeness'); + }); + + it('with the ship message file, completeness runs alongside the fleet and a clean pair passes', () => { + const r = runHook({}, undefined, { shipMsg: true }); + expect(r.status).toBe(0); + expect(r.calls).toContain('guard-review completeness --gate'); + expect(r.calls).toContain('guard-review --gate'); + }); + + it('a confident completeness FAIL (exit 1) blocks the commit at pre-commit', () => { + const r = runHook({ COMP_RC: '1' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); + expect(r.stdout).toContain('Confirmed completeness gap'); + }); + + it('completeness exit 3 (strict outage) fails closed with the remedy banner', () => { + const r = runHook({ COMP_RC: '3' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); + expect(r.stdout).toContain('strict ship mode failed closed'); + }); + + it('completeness exit 4 (unreadable staged content) blocks and names the cause', () => { + const r = runHook({ COMP_RC: '4' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); + expect(r.stdout).toContain('NOT a gate rejection'); + }); + + it('completeness exit 2 fails open', () => { + expect(runHook({ COMP_RC: '2' }, undefined, { shipMsg: true }).status).toBe(0); + }); + + it('a fleet FAIL blocks as the fleet, never as the parallel completeness verdict', () => { + const r = runHook({ REVIEW_RC: '1', COMP_RC: '1' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); + expect(r.stdout).toContain('opus-confirmed'); + expect(r.stdout).not.toContain('Confirmed completeness gap'); + }); +}); + describe('biome-format re-stage step (real git)', () => { // The re-stage step runs `git add` on files it just re-read from `git diff --cached` โ€” for a // release commit that force-added a gitignored `dist/` (`git add -f dist`), a plain `git add` diff --git a/cli/lib/husky/husky-block.mts b/cli/lib/husky/husky-block.mts index b3e38676..54334269 100644 --- a/cli/lib/husky/husky-block.mts +++ b/cli/lib/husky/husky-block.mts @@ -73,8 +73,31 @@ fi # /devkit:guard-decisions`, review: `# devkit:guard-review echo "๐Ÿ” Reviewer gate (headless domain judges)..." +# Ship path only (sc-1442 message file present): start the completeness judge NOW, in parallel +# with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the +# shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of +# opus overlaps the fleet instead of following it. Interactive commits (no message yet) are +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” +# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports +# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, +# not part of a range review. +comp_pid="" +if [ "\${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "\${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "\${DEVKIT_COMMIT_MSG_FILE:-}" ]; then + echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." + __dk_no_git_env bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! +fi rrc=0 __dk_no_git_env bunx guard-review --gate || rrc=$? +crc=0 +if [ -n "$comp_pid" ]; then + if [ "$rrc" -eq 0 ] || [ "$rrc" -eq 2 ]; then + wait "$comp_pid" || crc=$? + else + # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is + # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + kill "$comp_pid" 2>/dev/null || true + fi +fi if [ "$rrc" -eq 1 ]; then echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -86,7 +109,22 @@ elif [ "$rrc" -ne 0 ] && [ "$rrc" -ne 2 ]; then echo " guard-review: unexpected exit $rrc โ€” blocking the commit." exit 1 fi +# Same exit contract as the commit-msg fragment (commit-msg-block.mts) โ€” a completeness verdict +# means the same thing regardless of WHERE it was judged, just earlier here. +if [ "$crc" -eq 1 ]; then + echo " Confirmed completeness gap (hard-by-default; findings above)." + echo " Fix the gap, or โ€” with the user's explicit OK โ€” GUARD_NO_COMPLETENESS=1 git commit ..." + exit 1 +elif [ "$crc" -eq 4 ]; then + echo " NOT a gate rejection โ€” no defect was named; the staged content itself is unreadable." + exit 1 +elif [ "$crc" -eq 3 ]; then + echo " guard-review completeness: judge unavailable โ€” strict ship mode failed closed." + echo " Check \\\`claude\\\` CLI auth/quota, then re-run devkit ship (cleared judgements are cached)." + exit 1 +fi # rrc 0 = pass/cached/nothing-to-do, rrc 2 = inconclusive (non-strict fail-open) โ†’ continue. +# crc 0 = pass (now cached for the commit-msg gate) / skipped, crc 2 = fail-open โ†’ continue. # /devkit:guard-review`, }; diff --git a/dist/cli/lib/husky/husky-block.mjs b/dist/cli/lib/husky/husky-block.mjs index 63208a0c..c9b27d5a 100644 --- a/dist/cli/lib/husky/husky-block.mjs +++ b/dist/cli/lib/husky/husky-block.mjs @@ -46,8 +46,31 @@ fi # /devkit:guard-decisions`, review: `# devkit:guard-review echo "๐Ÿ” Reviewer gate (headless domain judges)..." +# Ship path only (sc-1442 message file present): start the completeness judge NOW, in parallel +# with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the +# shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of +# opus overlaps the fleet instead of following it. Interactive commits (no message yet) are +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” +# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports +# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, +# not part of a range review. +comp_pid="" +if [ "\${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "\${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "\${DEVKIT_COMMIT_MSG_FILE:-}" ]; then + echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." + __dk_no_git_env bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! +fi rrc=0 __dk_no_git_env bunx guard-review --gate || rrc=$? +crc=0 +if [ -n "$comp_pid" ]; then + if [ "$rrc" -eq 0 ] || [ "$rrc" -eq 2 ]; then + wait "$comp_pid" || crc=$? + else + # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is + # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + kill "$comp_pid" 2>/dev/null || true + fi +fi if [ "$rrc" -eq 1 ]; then echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -59,7 +82,22 @@ elif [ "$rrc" -ne 0 ] && [ "$rrc" -ne 2 ]; then echo " guard-review: unexpected exit $rrc โ€” blocking the commit." exit 1 fi +# Same exit contract as the commit-msg fragment (commit-msg-block.mts) โ€” a completeness verdict +# means the same thing regardless of WHERE it was judged, just earlier here. +if [ "$crc" -eq 1 ]; then + echo " Confirmed completeness gap (hard-by-default; findings above)." + echo " Fix the gap, or โ€” with the user's explicit OK โ€” GUARD_NO_COMPLETENESS=1 git commit ..." + exit 1 +elif [ "$crc" -eq 4 ]; then + echo " NOT a gate rejection โ€” no defect was named; the staged content itself is unreadable." + exit 1 +elif [ "$crc" -eq 3 ]; then + echo " guard-review completeness: judge unavailable โ€” strict ship mode failed closed." + echo " Check \\\`claude\\\` CLI auth/quota, then re-run devkit ship (cleared judgements are cached)." + exit 1 +fi # rrc 0 = pass/cached/nothing-to-do, rrc 2 = inconclusive (non-strict fail-open) โ†’ continue. +# crc 0 = pass (now cached for the commit-msg gate) / skipped, crc 2 = fail-open โ†’ continue. # /devkit:guard-review`, }; // Guard run order: the deterministic orchestrator first (one aggregated report), AI gates last so diff --git a/dist/gate-engine/review/completeness.mjs b/dist/gate-engine/review/completeness.mjs index 99053714..627e817e 100644 --- a/dist/gate-engine/review/completeness.mjs +++ b/dist/gate-engine/review/completeness.mjs @@ -45,6 +45,29 @@ import { buildCappedDiffEvidence } from "./diff-evidence.mjs"; import { cacheKey, parseReviewVerdict, stripFrontmatter } from "./reviewers.mjs"; const AGENT_NAME = 'feature-completeness-reviewer'; const TOOLS = 'Read,Grep,Glob,Bash(git diff:*),Bash(git log:*),Bash(git status:*)'; +// Trailing whitespace + blank-run normalisation, mirroring git's `--cleanup=whitespace` (the mode +// a `-m`/`-F` commit gets). The gate is now judged from TWO message sources that must produce the +// SAME cache key: the sc-1442 ship temp file at pre-commit (raw composed message) and git's +// cleaned COMMIT_EDITMSG at commit-msg. Without this, a message with a trailing space or a double +// blank line keys differently per hook and the pre-commit prewarm's cached PASS silently misses โ€” +// re-paying the full opus judgement the prewarm existed to avoid. +const TRAILING_WS_RE = /[ \t]+$/gm; +const BLANK_RUN_RE = /\n{3,}/g; +export function normalizeCommitMessage(raw) { + return raw.replace(TRAILING_WS_RE, '').replace(BLANK_RUN_RE, '\n\n').trim(); +} +/** The branch a sticky verdict is scoped to: the ship's exported branch, else the checkout's. */ +function verdictBranch(cwd) { + const exported = process.env.DEVKIT_SHIP_BRANCH; + if (exported) + return exported; + try { + return execSync('git rev-parse --abbrev-ref HEAD', { cwd, encoding: 'utf8' }).trim(); + } + catch { + return ''; + } +} // The capped, omission-accounted stdin-evidence builder (sc-1060) now lives in diff-evidence.mts // so gate-engine/review/claude-md.mts's CLAUDE.md renderer can reuse the same capping shape for // conventions-reviewer (which, having no Bash, needs the identical pre-rendered-evidence pattern @@ -82,11 +105,12 @@ export async function runCompleteness(msgFile, cwd = process.cwd(), { exec = exe return finish(0); let prompt; let diff; + let stickyKey = ''; try { const cfg = resolveGuardConfig(cwd); if (cfg.noLlm) return finish(0); - const message = readFileSync(path.isAbsolute(msgFile) ? msgFile : path.resolve(cwd, msgFile), 'utf8'); + const message = normalizeCommitMessage(readFileSync(path.isAbsolute(msgFile) ? msgFile : path.resolve(cwd, msgFile), 'utf8')); const files = execSync('git diff --cached --name-only', { cwd, encoding: 'utf8' }) .split('\n') .map((s) => s.trim()) @@ -102,6 +126,22 @@ export async function runCompleteness(msgFile, cwd = process.cwd(), { exec = exe console.error(`guard-review: ${AGENT_NAME}.md not found under ${dir} โ€” completeness skipped`); return finish(0); } + // Intent-scoped sticky PASS (cost ruling, 2026-08-06): this gate judges the MESSAGE's claims + // against the delivered change, so a retry whose diff was reshaped to satisfy ANOTHER + // reviewer โ€” same branch, same message โ€” has not changed what is claimed and is not + // re-judged. What re-opens the gate is a new claim or a new judge: an amended message, a + // different branch, a changed reviewer brief, or a devkit upgrade (cacheKey's versionSalt). + // A FAIL is never sticky (only the confident-PASS save below writes this key), so a found gap + // must genuinely be re-judged closed. Checked before scopedTargets/diff assembly โ€” a sticky + // hit skips the retrieval work too, not just the judge. + stickyKey = cacheKey('completeness-intent', `${verdictBranch(cwd)}\u0000${message}`, body); + const sticky = loadCache(cwd)[stickyKey]; + if (sticky) { + console.error('guard-review: completeness โ€” cached PASS (same branch + message; a retry-reshaped diff is not re-judged)'); + const stickyDuration = typeof sticky.duration_ms === 'number' ? sticky.duration_ms : undefined; + emitCacheHit('review:completeness', sticky.model, stickyDuration); + return finish(0, 'full', stickyDuration); + } const targets = await scopedTargets(files, message.split('\n')[0] ?? '', 6, cwd).catch(() => []); // The FULL --stat rides uncapped ahead of the evidence: on a branch-sized commit the caps // drop whole files, but the judge must at least SEE the complete file/churn map of what it @@ -171,14 +211,16 @@ export async function runCompleteness(msgFile, cwd = process.cwd(), { exec = exe // Only a CONFIDENT PASS is cached โ€” never a FAIL (the author fixes, the evidence changes), never // an unparseable verdict, and never the GUARD_COMPLETENESS_HARD=0 soften below (it exits 0 on a // FAIL the judge did make; caching it would make one softened run silence every later re-run). - if (verdict === 'PASS') - savePasses(cwd, { - [key]: { - at: new Date().toISOString(), - model: 'opus', - duration_ms: Date.now() - startedAt, - }, - }); + if (verdict === 'PASS') { + const meta = { + at: new Date().toISOString(), + model: 'opus', + duration_ms: Date.now() - startedAt, + }; + // Both identities: the exact byte key (any caller, any order) and the branch+message sticky + // key that lets a ship retry with a reshaped diff skip this judge (see the lookup above). + savePasses(cwd, stickyKey ? { [key]: meta, [stickyKey]: meta } : { [key]: meta }); + } if (verdict !== 'FAIL') return finish(0); console.error(`guard-review: completeness finding โ€” ${reason || 'see transcript'}`); diff --git a/dist/gate-engine/review/run-review.mjs b/dist/gate-engine/review/run-review.mjs index b428860a..15222ed5 100644 --- a/dist/gate-engine/review/run-review.mjs +++ b/dist/gate-engine/review/run-review.mjs @@ -21,7 +21,7 @@ * reviewer-eval bench validated the domain reviewers at haiku, 6/6 block/6/6 clean; a FAIL still * escalates to opus, so opus stays the block authority) ยท * GUARD_REVIEW_SKIP comma-list of reviewer names to disable individually ยท - * GUARD_REVIEW_CONCURRENCY max judge cascades in flight (default 2, floor 1) ยท + * GUARD_REVIEW_CONCURRENCY max judge cascades in flight (default 6, floor 1) ยท * GUARD_AI_STRICT=1 ship mode (first-pass retry once, then fail closed) ยท cfg.noLlm skip. * FRINK_* aliases honoured. Judges are isolated (JUDGE_ISOLATION) with an airtight read-only * allowlist โ€” a gate judge can never write, stage, or commit. @@ -260,7 +260,7 @@ async function cascadeVerdict({ reviewer, files }, { cwd, cfg, exec = execJudgeA } /** * The gate โ†’ exit code (see module contract). Selected reviewers run concurrently but BOUNDED to - * `reviewConcurrency()` cascades in flight (GUARD_REVIEW_CONCURRENCY, default 2) โ€” so under machine + * `reviewConcurrency()` cascades in flight (GUARD_REVIEW_CONCURRENCY, default 6) โ€” so under machine * load each judge keeps enough CPU + subscription slots to finish under its timeout. Wall-clock is * ceil(N/K) waves of the slowest cascade rather than the single slowest, a deliberate trade. */ diff --git a/dist/gate-engine/review/telemetry/timing.mjs b/dist/gate-engine/review/telemetry/timing.mjs index ae07c0ca..9d25aa5b 100644 --- a/dist/gate-engine/review/telemetry/timing.mjs +++ b/dist/gate-engine/review/telemetry/timing.mjs @@ -1,7 +1,10 @@ import { emitGateTiming } from "../../judge/gate-events.mjs"; -// 3, not 2: the correctness lens split turns one reviewer into up to four pool tasks, and at 2 they -// queue behind each other for no reason โ€” the bound that matters is subscription slots, not CPU. -const DEFAULT_REVIEW_CONCURRENCY = 3; +// 6, not 3: the correctness lens split turns one reviewer into up to four pool tasks, so a +// backend commit schedules ~8 (4 lenses + 4 domain reviewers). At 6 the lens wave and most of the +// fleet run in one wave and the review makespan approaches the slowest single judge instead of +// packing eight tasks into three slots. The bound that matters is subscription slots, not CPU or +// memory โ€” watch judge timeout rates, not RSS, if this ever needs lowering. +const DEFAULT_REVIEW_CONCURRENCY = 6; /** Max judge cascades in flight; invalid settings preserve the default. */ export function reviewConcurrency() { const n = Number.parseInt(process.env.GUARD_REVIEW_CONCURRENCY ?? process.env.FRINK_REVIEW_CONCURRENCY ?? '', 10); diff --git a/gate-engine/review/__tests__/run-review.test.mts b/gate-engine/review/__tests__/run-review.test.mts index 90bafb58..2b704e16 100644 --- a/gate-engine/review/__tests__/run-review.test.mts +++ b/gate-engine/review/__tests__/run-review.test.mts @@ -14,7 +14,12 @@ import { fileURLToPath } from 'node:url'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { DEEP_JUDGE_TIMEOUT_MS } from '../../judge/run-judge.mts'; import { loadCache } from '../cache.mts'; -import { buildCompletenessEvidence, runCompleteness, wrapCompleteness } from '../completeness.mts'; +import { + buildCompletenessEvidence, + normalizeCommitMessage, + runCompleteness, + wrapCompleteness, +} from '../completeness.mts'; import { CORRECTNESS_LENSES, FOUR_WAY_LENS_GROUPS, lensGroupId } from '../lens/split.mts'; import { readProgress, unfinishedReviewers, writeProgress } from '../progress.mts'; import { REVIEWERS } from '../reviewers.mts'; @@ -602,7 +607,7 @@ describe('runReviewGate โ€” cascade + exit contract', () => { expect(events.find((e) => e.type === 'gate_timing')).toMatchObject({ gate: 'review', cache_state: 'full', - parallelism: 3, + parallelism: 6, }); // A synthetic pass row would inflate review_result's fail-rate denominator and flatten the // duration percentiles that any judgement-cache change has to be sized against. @@ -1639,12 +1644,12 @@ describe('runReviewGate โ€” per-completion checkpoints', () => { describe('runReviewGate โ€” bounded judge concurrency (sc-1050)', () => { // consumerRepo({backend, frontend}) stages one file per domain โ†’ all 7 reviewers selected // (backend pair, frontend pair, commit-guard, correctness, conventions). - it('default cap 3: at most 3 judge cascades run at once, all still complete + cache', async () => { + it('default cap 6: at most 6 judge cascades run at once, all still complete + cache', async () => { const repo = consumerRepo({ backend: true, frontend: true }); const probe = concurrencyProbe(repo); expect(await runReviewGate(repo, { exec: probe.exec })).toBe(0); expect(probe.exec).toHaveBeenCalledTimes(7); - expect(probe.maxInflight()).toBe(3); + expect(probe.maxInflight()).toBe(6); expect(Object.keys(loadCache(repo)).length).toBe(7); }); @@ -1671,16 +1676,16 @@ describe('runReviewGate โ€” bounded judge concurrency (sc-1050)', () => { const probe = concurrencyProbe(repo, { failFirst: true }); // first attempt null โ†’ one strict retry expect(await runReviewGate(repo, { exec: probe.exec })).toBe(0); expect(probe.exec).toHaveBeenCalledTimes(14); // 7 reviewers ร— (attempt + retry), sequential in-slot - expect(probe.maxInflight()).toBeLessThanOrEqual(3); + expect(probe.maxInflight()).toBeLessThanOrEqual(6); }); - it('a garbage / out-of-range cap falls back to the default of 3', async () => { + it('a garbage / out-of-range cap falls back to the default of 6', async () => { for (const bad of ['', '0', '-3', 'abc']) { const repo = consumerRepo({ backend: true, frontend: true }); process.env.GUARD_REVIEW_CONCURRENCY = bad; const probe = concurrencyProbe(repo); expect(await runReviewGate(repo, { exec: probe.exec })).toBe(0); - expect(probe.maxInflight()).toBe(3); + expect(probe.maxInflight()).toBe(6); } }); @@ -1943,6 +1948,45 @@ describe('runCompleteness โ€” hard-by-default commit-msg gate', () => { expect(captured.args).toContain('opus'); // straight opus, no cascade }); + // The cost ruling (2026-08-06): completeness judges the message's CLAIMS, so a ship retry whose + // diff was reshaped to satisfy another reviewer โ€” same branch, same message โ€” is not re-judged. + it('a PASS is intent-sticky: a reshaped diff on the same branch + message skips the judge', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(1); + // The retry: a correctness fix reshapes the staged diff; the claim (message) is unchanged. + writeFileSync(join(repo, 'src', 'main', 'db.ts'), 'export const q = 2;\n'); + execSync('git add .', { cwd: repo }); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(1); // sticky hit โ€” the opus judgement is not re-paid + }); + + it('an amended message is a NEW claim โ€” the sticky pass does not cover it', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(await runCompleteness(msg(repo, 'feat: add db layer AND retries'), repo, { exec })).toBe( + 0, + ); + expect(exec).toHaveBeenCalledTimes(2); + }); + + it('a FAIL is never sticky โ€” the retry with a fixed diff re-judges', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + let verdict = 'VERDICT: FAIL โ€” half-shipped'; + const exec = mkExec(async () => verdict); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(1); + verdict = 'VERDICT: PASS'; + writeFileSync(join(repo, 'src', 'main', 'db.ts'), 'export const q = 3;\n'); + execSync('git add .', { cwd: repo }); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(2); + }); + it('GUARD_NO_COMPLETENESS=1 skips before any spawn', async () => { const repo = consumerRepo({ backend: true }); process.env.GUARD_NO_COMPLETENESS = '1'; @@ -2135,6 +2179,17 @@ describe('buildCompletenessEvidence โ€” per-file caps + omission accounting (sc- expect(prompt).toContain('OMITTED'); expect(prompt).toContain('investigate EVERY OMITTED/TRUNCATED entry'); }); + + // The gate is judged from TWO message sources that must produce the SAME cache key: the ship's + // raw composed temp file at pre-commit (the parallel prewarm) and git's cleaned COMMIT_EDITMSG + // at commit-msg. Whitespace-only differences between them must normalise away, or the prewarm's + // cached PASS silently misses and the opus judgement is re-paid. + it('normalizeCommitMessage converges the ship temp file and git cleanup=whitespace output', () => { + const composed = 'feat: x \n\n\n\nbody line \n\n'; + const gitCleaned = 'feat: x\n\nbody line\n'; + expect(normalizeCommitMessage(composed)).toBe(normalizeCommitMessage(gitCleaned)); + expect(normalizeCommitMessage(composed)).toBe('feat: x\n\nbody line'); + }); }); // The SHIPPED default (GUARD_CORRECTNESS_SPLIT unset) fans correctness out to one judge per lens. diff --git a/gate-engine/review/completeness.mts b/gate-engine/review/completeness.mts index 50b84bca..1ebc28e4 100644 --- a/gate-engine/review/completeness.mts +++ b/gate-engine/review/completeness.mts @@ -50,6 +50,29 @@ import { cacheKey, parseReviewVerdict, stripFrontmatter } from './reviewers.mts' const AGENT_NAME = 'feature-completeness-reviewer'; const TOOLS = 'Read,Grep,Glob,Bash(git diff:*),Bash(git log:*),Bash(git status:*)'; +// Trailing whitespace + blank-run normalisation, mirroring git's `--cleanup=whitespace` (the mode +// a `-m`/`-F` commit gets). The gate is now judged from TWO message sources that must produce the +// SAME cache key: the sc-1442 ship temp file at pre-commit (raw composed message) and git's +// cleaned COMMIT_EDITMSG at commit-msg. Without this, a message with a trailing space or a double +// blank line keys differently per hook and the pre-commit prewarm's cached PASS silently misses โ€” +// re-paying the full opus judgement the prewarm existed to avoid. +const TRAILING_WS_RE = /[ \t]+$/gm; +const BLANK_RUN_RE = /\n{3,}/g; +export function normalizeCommitMessage(raw: string): string { + return raw.replace(TRAILING_WS_RE, '').replace(BLANK_RUN_RE, '\n\n').trim(); +} + +/** The branch a sticky verdict is scoped to: the ship's exported branch, else the checkout's. */ +function verdictBranch(cwd: string): string { + const exported = process.env.DEVKIT_SHIP_BRANCH; + if (exported) return exported; + try { + return execSync('git rev-parse --abbrev-ref HEAD', { cwd, encoding: 'utf8' }).trim(); + } catch { + return ''; + } +} + // The capped, omission-accounted stdin-evidence builder (sc-1060) now lives in diff-evidence.mts // so gate-engine/review/claude-md.mts's CLAUDE.md renderer can reuse the same capping shape for // conventions-reviewer (which, having no Bash, needs the identical pre-rendered-evidence pattern @@ -100,12 +123,12 @@ export async function runCompleteness( if (envFlag('NO_COMPLETENESS')) return finish(0); let prompt: string; let diff: string; + let stickyKey = ''; try { const cfg = resolveGuardConfig(cwd); if (cfg.noLlm) return finish(0); - const message = readFileSync( - path.isAbsolute(msgFile) ? msgFile : path.resolve(cwd, msgFile), - 'utf8', + const message = normalizeCommitMessage( + readFileSync(path.isAbsolute(msgFile) ? msgFile : path.resolve(cwd, msgFile), 'utf8'), ); const files = execSync('git diff --cached --name-only', { cwd, encoding: 'utf8' }) .split('\n') @@ -123,6 +146,25 @@ export async function runCompleteness( console.error(`guard-review: ${AGENT_NAME}.md not found under ${dir} โ€” completeness skipped`); return finish(0); } + // Intent-scoped sticky PASS (cost ruling, 2026-08-06): this gate judges the MESSAGE's claims + // against the delivered change, so a retry whose diff was reshaped to satisfy ANOTHER + // reviewer โ€” same branch, same message โ€” has not changed what is claimed and is not + // re-judged. What re-opens the gate is a new claim or a new judge: an amended message, a + // different branch, a changed reviewer brief, or a devkit upgrade (cacheKey's versionSalt). + // A FAIL is never sticky (only the confident-PASS save below writes this key), so a found gap + // must genuinely be re-judged closed. Checked before scopedTargets/diff assembly โ€” a sticky + // hit skips the retrieval work too, not just the judge. + stickyKey = cacheKey('completeness-intent', `${verdictBranch(cwd)}\u0000${message}`, body); + const sticky = loadCache(cwd)[stickyKey]; + if (sticky) { + console.error( + 'guard-review: completeness โ€” cached PASS (same branch + message; a retry-reshaped diff is not re-judged)', + ); + const stickyDuration = + typeof sticky.duration_ms === 'number' ? sticky.duration_ms : undefined; + emitCacheHit('review:completeness', sticky.model, stickyDuration); + return finish(0, 'full', stickyDuration); + } const targets = await scopedTargets(files, message.split('\n')[0] ?? '', 6, cwd).catch( () => [], ); @@ -202,14 +244,16 @@ export async function runCompleteness( // Only a CONFIDENT PASS is cached โ€” never a FAIL (the author fixes, the evidence changes), never // an unparseable verdict, and never the GUARD_COMPLETENESS_HARD=0 soften below (it exits 0 on a // FAIL the judge did make; caching it would make one softened run silence every later re-run). - if (verdict === 'PASS') - savePasses(cwd, { - [key]: { - at: new Date().toISOString(), - model: 'opus', - duration_ms: Date.now() - startedAt, - }, - }); + if (verdict === 'PASS') { + const meta = { + at: new Date().toISOString(), + model: 'opus', + duration_ms: Date.now() - startedAt, + }; + // Both identities: the exact byte key (any caller, any order) and the branch+message sticky + // key that lets a ship retry with a reshaped diff skip this judge (see the lookup above). + savePasses(cwd, stickyKey ? { [key]: meta, [stickyKey]: meta } : { [key]: meta }); + } if (verdict !== 'FAIL') return finish(0); console.error(`guard-review: completeness finding โ€” ${reason || 'see transcript'}`); console.error(raw.trim()); diff --git a/gate-engine/review/run-review.mts b/gate-engine/review/run-review.mts index 8ff26821..adff4dbd 100644 --- a/gate-engine/review/run-review.mts +++ b/gate-engine/review/run-review.mts @@ -21,7 +21,7 @@ * reviewer-eval bench validated the domain reviewers at haiku, 6/6 block/6/6 clean; a FAIL still * escalates to opus, so opus stays the block authority) ยท * GUARD_REVIEW_SKIP comma-list of reviewer names to disable individually ยท - * GUARD_REVIEW_CONCURRENCY max judge cascades in flight (default 2, floor 1) ยท + * GUARD_REVIEW_CONCURRENCY max judge cascades in flight (default 6, floor 1) ยท * GUARD_AI_STRICT=1 ship mode (first-pass retry once, then fail closed) ยท cfg.noLlm skip. * FRINK_* aliases honoured. Judges are isolated (JUDGE_ISOLATION) with an airtight read-only * allowlist โ€” a gate judge can never write, stage, or commit. @@ -335,7 +335,7 @@ async function cascadeVerdict( /** * The gate โ†’ exit code (see module contract). Selected reviewers run concurrently but BOUNDED to - * `reviewConcurrency()` cascades in flight (GUARD_REVIEW_CONCURRENCY, default 2) โ€” so under machine + * `reviewConcurrency()` cascades in flight (GUARD_REVIEW_CONCURRENCY, default 6) โ€” so under machine * load each judge keeps enough CPU + subscription slots to finish under its timeout. Wall-clock is * ceil(N/K) waves of the slowest cascade rather than the single slowest, a deliberate trade. */ diff --git a/gate-engine/review/telemetry/timing.mts b/gate-engine/review/telemetry/timing.mts index 63510c36..3675e40e 100644 --- a/gate-engine/review/telemetry/timing.mts +++ b/gate-engine/review/telemetry/timing.mts @@ -1,8 +1,11 @@ import { emitGateTiming } from '../../judge/gate-events.mts'; -// 3, not 2: the correctness lens split turns one reviewer into up to four pool tasks, and at 2 they -// queue behind each other for no reason โ€” the bound that matters is subscription slots, not CPU. -const DEFAULT_REVIEW_CONCURRENCY = 3; +// 6, not 3: the correctness lens split turns one reviewer into up to four pool tasks, so a +// backend commit schedules ~8 (4 lenses + 4 domain reviewers). At 6 the lens wave and most of the +// fleet run in one wave and the review makespan approaches the slowest single judge instead of +// packing eight tasks into three slots. The bound that matters is subscription slots, not CPU or +// memory โ€” watch judge timeout rates, not RSS, if this ever needs lowering. +const DEFAULT_REVIEW_CONCURRENCY = 6; /** Max judge cascades in flight; invalid settings preserve the default. */ export function reviewConcurrency(): number { From 91b0e7d1d5a11641d86397ef13e42e39e7d6c451 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Fri, 7 Aug 2026 09:24:41 +0100 Subject: [PATCH 2/2] fix(review): the cancelled completeness judge must actually die, not just be signalled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged the kill path as signal-without-reap. Investigating it found the deeper cause: `__dk_no_git_env` is a shell FUNCTION, so backgrounding it forked a subshell โ€” $! was that wrapper, and the judge ran on as an orphaned grandchild. Signalling the wrapper killed nothing that mattered, and `wait` alone would have reaped the wrapper just as harmlessly. Proven with a probe: after kill+wait the judge was still in `pgrep`. Backgrounding a SIMPLE command instead (DK_NO_GIT_ENV_INLINE, one env -u prefix built from the same GIT_ENV_VARS as the function) makes the shell fork-and-exec directly, so $! IS the judge; kill then reaches it and wait reaps it before the hook returns. That matters beyond tidiness: the judge inherits git's stdout/stderr, and a survivor holds the ship capture pipeline's read end open โ€” commit-with-gate-capture.sh's R3 hang โ€” while still spending opus on a verdict nobody will read. Test coverage this PR was thin on, now added: - the reap contract itself, with a stub that releases the inherited pipe first (so the assertion measures the HOOK's return, not the pipe drain) and delays inside its TERM handler (so 'returned' and 'reaped' are separable). Verified failing against the unfixed hook. - the same for a fleet exit 3, not just exit 1 โ€” every block path. - review mode does not prewarm; a missing message-file path arms nothing. - sticky-key components proven to re-open the gate one at a time: branch (isolated with a moving diff, since the exact-bytes key is legitimately branch-independent) and reviewer brief. - the prewarm handoff end to end: ship temp file then git-cleaned message is ONE judgement, which is what normalizeCommitMessage exists for. - a sticky hit emits cache_hit + a fully-cached gate_timing, never a silent skip. - DEVKIT_SHIP_BRANCH added to the suite's env-hygiene list (the sticky key reads it, so a suite run during a ship would otherwise be steered). Co-Authored-By: Claude Opus 5 (1M context) --- .husky/pre-commit | 22 +++-- cli/__tests__/husky-block-exec.test.mts | 57 +++++++++++- cli/lib/husky/husky-block.mts | 23 +++-- cli/lib/husky/review-fragments.mts | 15 ++++ dist/cli/lib/husky/husky-block.mjs | 24 +++-- dist/cli/lib/husky/review-fragments.mjs | 14 +++ .../review/__tests__/run-review.test.mts | 88 +++++++++++++++++++ 7 files changed, 225 insertions(+), 18 deletions(-) diff --git a/.husky/pre-commit b/.husky/pre-commit index a2102734..a4d9a235 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -148,14 +148,18 @@ echo "๐Ÿ” Reviewer gate (headless domain judges)..." # with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the # shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of # opus overlaps the fleet instead of following it. Interactive commits (no message yet) are -# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” -# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports -# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, -# not part of a range review. +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed AND reaped +# below โ€” nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it +# exports the SAME env as its reviewer intent file (review-target.sh), but completeness is a +# commit gate, not part of a range review. +# +# DK_NO_GIT_ENV_INLINE, not the __dk_no_git_env function: backgrounding a function forks a +# subshell, which would make $! a wrapper whose death leaves the judge orphaned and running. See +# review-fragments.mts. comp_pid="" if [ "${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "${DEVKIT_COMMIT_MSG_FILE:-}" ]; then echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." - __dk_no_git_env node gate-engine/review/cli.mts completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! + env -u GIT_ALTERNATE_OBJECT_DIRECTORIES -u GIT_CONFIG -u GIT_CONFIG_PARAMETERS -u GIT_CONFIG_COUNT -u GIT_OBJECT_DIRECTORY -u GIT_DIR -u GIT_WORK_TREE -u GIT_IMPLICIT_WORK_TREE -u GIT_GRAFT_FILE -u GIT_INDEX_FILE -u GIT_NO_REPLACE_OBJECTS -u GIT_REPLACE_REF_BASE -u GIT_PREFIX -u GIT_SHALLOW_FILE -u GIT_COMMON_DIR -u GIT_GLOB_PATHSPECS -u GIT_NOGLOB_PATHSPECS -u GIT_LITERAL_PATHSPECS -u GIT_ICASE_PATHSPECS node gate-engine/review/cli.mts completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! fi rrc=0 __dk_no_git_env node gate-engine/review/cli.mts --gate || rrc=$? @@ -166,7 +170,15 @@ if [ -n "$comp_pid" ]; then else # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + # SIGNAL THEN REAP, never signal alone: the judge inherited git's stdout/stderr, so the + # ship capture pipeline only unblocks once every copy of that write-end is closed. A hook + # that returns while a signalled child is still winding down leaves the reader waiting on + # a pipe nobody will write to again โ€” the exact hang commit-with-gate-capture.sh's R3 + # supervisor exists to bound. Both lines are status-tested (|| true) because a reaped + # job's status IS the signal (143), and under sh -e an untested non-zero would abort the + # hook here, before it reports its own verdict below. kill "$comp_pid" 2>/dev/null || true + wait "$comp_pid" 2>/dev/null || true fi fi if [ "$rrc" -eq 1 ]; then diff --git a/cli/__tests__/husky-block-exec.test.mts b/cli/__tests__/husky-block-exec.test.mts index 8c25b263..961ffc99 100644 --- a/cli/__tests__/husky-block-exec.test.mts +++ b/cli/__tests__/husky-block-exec.test.mts @@ -56,7 +56,25 @@ echo "$tool $*" >> "$HOME/calls.log" case "$tool" in guard-deterministic) exit \${DET_RC:-0};; guard-decisions) exit \${DEC_RC:-0};; - guard-review) case "$1" in completeness) exit \${COMP_RC:-0};; *) exit \${REVIEW_RC:-0};; esac;; + guard-review) + case "$1" in + completeness) + # COMP_SLOW_TERM: a judge that does not die the instant it is signalled. It releases the + # inherited stdout/stderr FIRST (\`exec >/dev/null\`) so this harness measures the HOOK's + # own return, not the pipe drain โ€” otherwise spawnSync would block on the pipe regardless + # and a hook that never reaps would still look correct. The trap then delays before + # recording that it finished winding down, so "hook returned" and "child was reaped" are + # separable events. + if [ -n "\${COMP_SLOW_TERM:-}" ]; then + exec >/dev/null 2>&1 + trap 'sleep 1; echo reaped > "$HOME/comp-reaped"; exit 143' TERM + echo running > "$HOME/comp-running" + sleep 30 & + wait $! + fi + exit \${COMP_RC:-0};; + *) exit \${REVIEW_RC:-0};; + esac;; *) exit 0;; esac `, @@ -82,7 +100,8 @@ esac } catch { // hook never reached the stub } - return { status, stdout, calls }; + // `home` rides along so a test can assert on markers the stubs dropped there (the reap probe). + return { status, stdout, calls, home }; } describe('assembled hook execution (stubbed bunx, sh -e)', () => { @@ -197,6 +216,40 @@ describe('parallel completeness prewarm (ship message file present)', () => { expect(r.stdout).toContain('opus-confirmed'); expect(r.stdout).not.toContain('Confirmed completeness gap'); }); + + it('review mode does NOT prewarm โ€” it exports the same env for its reviewer intent file', () => { + const r = runHook({ DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_GUARDS: 'review' }, undefined, { + shipMsg: true, + }); + expect(r.status).toBe(0); + expect(r.calls).toContain('guard-review --gate'); + expect(r.calls).not.toContain('guard-review completeness'); + }); + + it('a message-file path that does not exist arms nothing (the -f guard, not just -n)', () => { + const r = runHook({ DEVKIT_COMMIT_MSG_FILE: '/nonexistent/dk-msg.txt' }); + expect(r.status).toBe(0); + expect(r.calls).not.toContain('guard-review completeness'); + }); + + // The reap contract: the judge inherits git's stdout/stderr, so a hook that returns while a + // signalled child is still winding down leaves the ship's capture reader on a pipe nobody will + // close โ€” commit-with-gate-capture.sh's R3 hang. Signalling alone is not enough; the harness + // stub releases the pipe first so this asserts the HOOK waited, not that the pipe drained. + it('a killed completeness judge is REAPED before the hook returns, not merely signalled', () => { + const r = runHook({ REVIEW_RC: '1', COMP_SLOW_TERM: '1' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); // still the fleet's verdict + expect(existsSync(join(r.home, 'comp-running'))).toBe(true); // the judge really did start + // Written only by the TERM handler, after a delay: present iff the hook waited for it. + expect(existsSync(join(r.home, 'comp-reaped'))).toBe(true); + }); + + it('the fleet failing CLOSED (exit 3) also kills and reaps โ€” every block path, not just exit 1', () => { + const r = runHook({ REVIEW_RC: '3', COMP_SLOW_TERM: '1' }, undefined, { shipMsg: true }); + expect(r.status).toBe(1); + expect(r.stdout).toContain('strict ship mode failed closed'); + expect(existsSync(join(r.home, 'comp-reaped'))).toBe(true); + }); }); describe('biome-format re-stage step (real git)', () => { diff --git a/cli/lib/husky/husky-block.mts b/cli/lib/husky/husky-block.mts index 54334269..fd5f91fa 100644 --- a/cli/lib/husky/husky-block.mts +++ b/cli/lib/husky/husky-block.mts @@ -14,6 +14,7 @@ import { buildCommitTerminalFragment } from './commit-terminal.mts'; import { markEnd, markStart } from './husky.mts'; import { DK_HOOK_HELPERS, + DK_NO_GIT_ENV_INLINE, DK_REVIEW_BASELINE_HELPER, selectedFragment, } from './review-fragments.mts'; @@ -77,14 +78,18 @@ echo "๐Ÿ” Reviewer gate (headless domain judges)..." # with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the # shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of # opus overlaps the fleet instead of following it. Interactive commits (no message yet) are -# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” -# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports -# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, -# not part of a range review. +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed AND reaped +# below โ€” nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it +# exports the SAME env as its reviewer intent file (review-target.sh), but completeness is a +# commit gate, not part of a range review. +# +# DK_NO_GIT_ENV_INLINE, not the __dk_no_git_env function: backgrounding a function forks a +# subshell, which would make $! a wrapper whose death leaves the judge orphaned and running. See +# review-fragments.mts. comp_pid="" if [ "\${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "\${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "\${DEVKIT_COMMIT_MSG_FILE:-}" ]; then echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." - __dk_no_git_env bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! + ${DK_NO_GIT_ENV_INLINE} bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! fi rrc=0 __dk_no_git_env bunx guard-review --gate || rrc=$? @@ -95,7 +100,15 @@ if [ -n "$comp_pid" ]; then else # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + # SIGNAL THEN REAP, never signal alone: the judge inherited git's stdout/stderr, so the + # ship capture pipeline only unblocks once every copy of that write-end is closed. A hook + # that returns while a signalled child is still winding down leaves the reader waiting on + # a pipe nobody will write to again โ€” the exact hang commit-with-gate-capture.sh's R3 + # supervisor exists to bound. Both lines are status-tested (|| true) because a reaped + # job's status IS the signal (143), and under sh -e an untested non-zero would abort the + # hook here, before it reports its own verdict below. kill "$comp_pid" 2>/dev/null || true + wait "$comp_pid" 2>/dev/null || true fi fi if [ "$rrc" -eq 1 ]; then diff --git a/cli/lib/husky/review-fragments.mts b/cli/lib/husky/review-fragments.mts index 7fd8486f..346c9a3c 100644 --- a/cli/lib/husky/review-fragments.mts +++ b/cli/lib/husky/review-fragments.mts @@ -16,6 +16,21 @@ export const DK_NO_GIT_ENV_HELPER = `__dk_no_git_env() { env ${GIT_ENV_VARS.map((name) => `-u ${name}`).join(' \\\n ')} "$@" }`; +/** + * The same scrub as a PREFIX for one simple command, for the single case the function form cannot + * serve: a BACKGROUNDED gate whose pid the hook must later signal. + * + * `__dk_no_git_env cmd &` backgrounds a shell FUNCTION, which forks a subshell โ€” so `$!` is that + * subshell and the gate itself is a grandchild. Signalling the subshell then kills a wrapper while + * the real judge runs on, orphaned, still holding the stdout/stderr it inherited from git (and + * still spending model budget on a verdict nobody will read). Backgrounding a simple command + * instead makes the shell fork-and-exec directly, so `$!` IS the gate and one kill reaches it. + * + * Single line, no continuations: it has to sit inline ahead of a command in a background job. + * Same GIT_ENV_VARS source as the function above, so the two can never scrub different sets. + */ +export const DK_NO_GIT_ENV_INLINE = `env ${GIT_ENV_VARS.map((name) => `-u ${name}`).join(' ')}`; + // Review mode has its own positive guard allowlist. Normal commit/ship runs select everything in // the generated hook; review runs only ids named by DEVKIT_REVIEW_GUARDS. export const DK_GATE_SELECTED_HELPER = `__dk_gate_selected() { diff --git a/dist/cli/lib/husky/husky-block.mjs b/dist/cli/lib/husky/husky-block.mjs index c9b27d5a..d6b09e65 100644 --- a/dist/cli/lib/husky/husky-block.mjs +++ b/dist/cli/lib/husky/husky-block.mjs @@ -11,7 +11,7 @@ */ import { buildCommitTerminalFragment } from "./commit-terminal.mjs"; import { markEnd, markStart } from "./husky.mjs"; -import { DK_HOOK_HELPERS, DK_REVIEW_BASELINE_HELPER, selectedFragment, } from "./review-fragments.mjs"; +import { DK_HOOK_HELPERS, DK_NO_GIT_ENV_INLINE, DK_REVIEW_BASELINE_HELPER, selectedFragment, } from "./review-fragments.mjs"; // The ONE deterministic line: `guard-deterministic` (gate-engine/deterministic/run.mjs) owns the // prefix-cache check/record, runs the selected guards (.devkit/config.json components.guards), // applies the rc trichotomy per gate, and aggregates every failure into one report + one exit @@ -50,14 +50,18 @@ echo "๐Ÿ” Reviewer gate (headless domain judges)..." # with the reviewer fleet, instead of serially at commit-msg. Its confident PASS lands in the # shared verdict store, so the commit-msg gate re-judges it as a cache hit โ€” the serial ~4min of # opus overlaps the fleet instead of following it. Interactive commits (no message yet) are -# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed below โ€” -# nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it exports -# the SAME env as its reviewer intent file (review-target.sh), but completeness is a commit gate, -# not part of a range review. +# unchanged. Lifetime is scoped to this hook: the judge is either wait'ed on or killed AND reaped +# below โ€” nothing outlives the hook to hold git's output pipe open. Review mode is excluded โ€” it +# exports the SAME env as its reviewer intent file (review-target.sh), but completeness is a +# commit gate, not part of a range review. +# +# DK_NO_GIT_ENV_INLINE, not the __dk_no_git_env function: backgrounding a function forks a +# subshell, which would make $! a wrapper whose death leaves the judge orphaned and running. See +# review-fragments.mts. comp_pid="" if [ "\${DEVKIT_RUN_MODE:-}" != "review" ] && [ -n "\${DEVKIT_COMMIT_MSG_FILE:-}" ] && [ -f "\${DEVKIT_COMMIT_MSG_FILE:-}" ]; then echo "๐Ÿงฉ Completeness judge started in parallel (ship message known)..." - __dk_no_git_env bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! + ${DK_NO_GIT_ENV_INLINE} bunx guard-review completeness --gate "$DEVKIT_COMMIT_MSG_FILE" & comp_pid=$! fi rrc=0 __dk_no_git_env bunx guard-review --gate || rrc=$? @@ -68,7 +72,15 @@ if [ -n "$comp_pid" ]; then else # The fleet already blocked this commit โ€” stop paying for a judgement of a diff that is # about to change. (The verdict would be keyed to THIS diff; the fix invalidates it.) + # SIGNAL THEN REAP, never signal alone: the judge inherited git's stdout/stderr, so the + # ship capture pipeline only unblocks once every copy of that write-end is closed. A hook + # that returns while a signalled child is still winding down leaves the reader waiting on + # a pipe nobody will write to again โ€” the exact hang commit-with-gate-capture.sh's R3 + # supervisor exists to bound. Both lines are status-tested (|| true) because a reaped + # job's status IS the signal (143), and under sh -e an untested non-zero would abort the + # hook here, before it reports its own verdict below. kill "$comp_pid" 2>/dev/null || true + wait "$comp_pid" 2>/dev/null || true fi fi if [ "$rrc" -eq 1 ]; then diff --git a/dist/cli/lib/husky/review-fragments.mjs b/dist/cli/lib/husky/review-fragments.mjs index 1a9501f1..31647259 100644 --- a/dist/cli/lib/husky/review-fragments.mjs +++ b/dist/cli/lib/husky/review-fragments.mjs @@ -13,6 +13,20 @@ import { GIT_ENV_VARS } from "../../../gate-engine/judge/judge-isolation.mjs"; export const DK_NO_GIT_ENV_HELPER = `__dk_no_git_env() { env ${GIT_ENV_VARS.map((name) => `-u ${name}`).join(' \\\n ')} "$@" }`; +/** + * The same scrub as a PREFIX for one simple command, for the single case the function form cannot + * serve: a BACKGROUNDED gate whose pid the hook must later signal. + * + * `__dk_no_git_env cmd &` backgrounds a shell FUNCTION, which forks a subshell โ€” so `$!` is that + * subshell and the gate itself is a grandchild. Signalling the subshell then kills a wrapper while + * the real judge runs on, orphaned, still holding the stdout/stderr it inherited from git (and + * still spending model budget on a verdict nobody will read). Backgrounding a simple command + * instead makes the shell fork-and-exec directly, so `$!` IS the gate and one kill reaches it. + * + * Single line, no continuations: it has to sit inline ahead of a command in a background job. + * Same GIT_ENV_VARS source as the function above, so the two can never scrub different sets. + */ +export const DK_NO_GIT_ENV_INLINE = `env ${GIT_ENV_VARS.map((name) => `-u ${name}`).join(' ')}`; // Review mode has its own positive guard allowlist. Normal commit/ship runs select everything in // the generated hook; review runs only ids named by DEVKIT_REVIEW_GUARDS. export const DK_GATE_SELECTED_HELPER = `__dk_gate_selected() { diff --git a/gate-engine/review/__tests__/run-review.test.mts b/gate-engine/review/__tests__/run-review.test.mts index 2b704e16..b31e0a6e 100644 --- a/gate-engine/review/__tests__/run-review.test.mts +++ b/gate-engine/review/__tests__/run-review.test.mts @@ -67,6 +67,9 @@ const ENV_KEYS = [ // semantic Target retrieval, which must stay deterministic/off in tests). 'DEVKIT_COMMIT_MSG_FILE', 'DECISIONS_NO_EMBED', + // The completeness sticky key is scoped to the shipping branch, so a developer running the suite + // DURING a ship (which exports this) would otherwise key verdicts to that ship's branch. + 'DEVKIT_SHIP_BRANCH', ]; const saved = {}; const COMMIT_GUARD_INIT_SCRIPT = ` @@ -1987,6 +1990,91 @@ describe('runCompleteness โ€” hard-by-default commit-msg gate', () => { expect(exec).toHaveBeenCalledTimes(2); }); + // The sticky key's three inputs, each proven to re-open the gate on its own. A component that + // silently stopped keying would replay a stale PASS across branches or briefs โ€” the failure mode + // that makes a sticky verdict dangerous rather than cheap. + // Isolating the BRANCH component needs a moving diff: with the diff held still the exact-bytes + // key hits on its own (identical inputs, identical judgement โ€” branch-independent, and true + // before this change), which would mask whether the sticky key is branch-scoped at all. + it('a different branch is a different claim โ€” the sticky pass does not cross branches', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + const reshape = (n: number) => { + writeFileSync(join(repo, 'src', 'main', 'db.ts'), `export const q = ${n};\n`); + execSync('git add .', { cwd: repo }); + }; + process.env.DEVKIT_SHIP_BRANCH = 'feat/one'; + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(1); + reshape(2); // same branch, same claim, reshaped diff โ†’ sticky hit + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(1); + reshape(3); // a DIFFERENT branch: neither key covers it + process.env.DEVKIT_SHIP_BRANCH = 'feat/two'; + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(2); + }); + + it('an edited reviewer brief re-judges โ€” a new judge is not covered by the old verdict', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + writeFileSync( + join(repo, '.claude', 'agents', 'feature-completeness-reviewer.md'), + '---\nname: feature-completeness-reviewer\n---\nBrief for feature-completeness-reviewer. Also check migrations.', + ); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + expect(exec).toHaveBeenCalledTimes(2); + }); + + // The actual prewarm handoff: pre-commit judges the ship's RAW composed temp file, commit-msg + // re-judges git's cleaned COMMIT_EDITMSG. Same claim, so the second must be a cache hit โ€” this is + // the contract normalizeCommitMessage exists for, asserted end to end rather than as a string fn. + it('the ship temp file and git-cleaned message are ONE judgement across the two hooks', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + // pre-commit: ship's composed message (trailing spaces, extra blank runs, no cleanup applied). + expect( + await runCompleteness(msg(repo, 'feat: add db layer \n\n\n\nships the pool. \n\n'), repo, { + exec, + }), + ).toBe(0); + // commit-msg: the same message after git's --cleanup=whitespace. + expect( + await runCompleteness(msg(repo, 'feat: add db layer\n\nships the pool.\n'), repo, { exec }), + ).toBe(0); + expect(exec).toHaveBeenCalledTimes(1); // one opus judgement, not two + }); + + it('a sticky hit reports itself as a cache_hit and a fully-cached gate, never a silent skip', async () => { + const repo = consumerRepo({ backend: true }); + vi.spyOn(console, 'error').mockImplementation(() => {}); + const exec = mkExec(async () => 'VERDICT: PASS'); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + const sink = join(repo, 'events.jsonl'); + process.env.DEVKIT_GATE_EVENTS = sink; + process.env.DEVKIT_SHIP_ID = 'ship-sticky'; + writeFileSync(join(repo, 'src', 'main', 'db.ts'), 'export const q = 9;\n'); + execSync('git add .', { cwd: repo }); + expect(await runCompleteness(msg(repo, 'feat: add db layer'), repo, { exec })).toBe(0); + const events = readFileSync(sink, 'utf8') + .trim() + .split('\n') + .map((l) => JSON.parse(l)); + // Labelled exactly as judge_exec labels it, so hit rate stays a group-by with no join. + expect(events.find((e) => e.type === 'cache_hit')).toMatchObject({ + judge: 'review:completeness', + model: 'opus', + }); + expect(events.find((e) => e.type === 'gate_timing')).toMatchObject({ + gate: 'completeness', + cache_state: 'full', + }); + }); + it('GUARD_NO_COMPLETENESS=1 skips before any spawn', async () => { const repo = consumerRepo({ backend: true }); process.env.GUARD_NO_COMPLETENESS = '1';