diff --git a/.husky/pre-commit b/.husky/pre-commit index 4bb354ba..a4d9a235 100755 --- a/.husky/pre-commit +++ b/.husky/pre-commit @@ -144,8 +144,43 @@ 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 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)..." + 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=$? +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.) + # 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 echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -157,7 +192,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..961ffc99 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,25 @@ 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) + # 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 `, @@ -75,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)', () => { @@ -147,6 +173,85 @@ 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'); + }); + + 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)', () => { // 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..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'; @@ -73,8 +74,43 @@ 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 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_INLINE} 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.) + # 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 echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -86,7 +122,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/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 63208a0c..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 @@ -46,8 +46,43 @@ 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 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_INLINE} 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.) + # 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 echo " A reviewer FAILED (opus-confirmed). Fix the findings above, then re-run." exit 1 @@ -59,7 +94,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/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/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..b31e0a6e 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'; @@ -62,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 = ` @@ -602,7 +610,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 +1647,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 +1679,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 +1951,130 @@ 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); + }); + + // 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'; @@ -2135,6 +2267,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 {