diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 92a64500415..7c1e513ec4a 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -3445,6 +3445,10 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(out).toContain('2 auditors required this round'); expect(out).toContain('chunk 13 — retired: dry in rounds 1 and 2'); expect(out).toContain('certificate final'); + // Pin the spelled cap number, not just the branch: a hardcoded `5-round + // cap leaves` in the note wording would otherwise ship silently and tell + // the orchestrator a false cap on exactly the huge-diff runs this targets. + expect(out).toContain('3-round cap leaves'); expect(out).not.toContain('next cold check round 4'); }); @@ -3472,6 +3476,19 @@ describe('per-chunk retirement — cold territories stop costing a round', () => .join('\n'); expect(msg).toContain('ROUND CAP'); expect(msg).toContain('round cap is 3'); + // The load-bearing tail rules — the same verify-only / compose-floor + // contract the budget message's test pins and SKILL.md's round-cap + // bullet mirrors; a reword that drops any of these silently loosens + // the termination contract, so pin each. + expect(msg).toContain('agent-prompt --role verify'); + expect(msg).toContain('never a hand-rolled agent'); + expect(msg).toContain('compose floor'); + expect(msg).toContain('Do NOT re-verify findings already'); + // The wait-bound and no-fresh-pass clauses too — the budget message's + // test pins the same two for the sibling refusal; one bounded-tail + // protocol, both pin both. + expect(msg).toContain('stop waiting on any verifier batch still out'); + expect(msg).toContain('invent a fresh re-verification pass'); // The marker is on disk so compose-review caps without the relay. expect(readBudgetStop(plan)?.cause).toBe('round-cap'); expect(readBudgetStop(plan)?.cap).toBe(3); @@ -3592,6 +3609,165 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(readRoundStamps(plan)).toHaveLength(stampsBefore); }); + it('huge cap: a converged past-cap round exits 5, not the cap — convergence outranks it', () => { + // The ordering the PR documents four times (the convergence check runs + // BEFORE the round-cap gate) with no test pin: hoisting the cap check + // above it survives the whole suite. Round 5 is past the cap of 3, but + // its schedule has converged (every chunk twice-dry, odd round → all + // skipped), so it must exit 5 CONVERGED with NO marker — not exit 4 at + // the cap. History that lands convergence on an odd past-cap round: 13/14 + // dry in rounds 1-2 (retire at 3), 15 whiffs round 1 then goes dry in + // 2-3, so round 3 (odd) builds only 15 and nothing converges before 5. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + runRound(1); + auditorTranscript(recordOf(1, 13), DRY); + auditorTranscript(recordOf(1, 14), DRY); + auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(3, { 15: DRY }); // 13,14 retired (odd → skipped); only 15 built + expect(keysOf(3)).toHaveLength(1); + + const out = runRound(5); // 5 > cap 3, but the schedule has converged + expect(process.exitCode).toBe(5); + expect(out).toBe(''); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + // Convergence outranks the cap: no round-cap refusal, no marker written. + expect(readBudgetStop(plan)).toBeNull(); + }); + + it('huge cap: a CONVERGED exit clears a stale same-run round-cap marker', () => { + // Retry-after-refusal: round 4 (even) is refused at the cap — every + // retired chunk is DUE a cold check, so the schedule is not converged and + // 4 > 3 refuses, writing the marker. The orchestrator then asks for round + // 5, which converges. Nothing else unlinks budget-stop.json, so without + // the converged-branch clear the stale marker caps a verdict that + // legitimately converged. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + runRound(1); + auditorTranscript(recordOf(1, 13), DRY); + auditorTranscript(recordOf(1, 14), DRY); + auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(3, { 15: DRY }); + + runRound(4); // even → retired chunks due cold checks → not converged → cap refuses + expect(process.exitCode).toBe(4); + expect(readBudgetStop(plan)?.cause).toBe('round-cap'); + + process.exitCode = undefined; + const out = runRound(5); // odd → all skipped → converged + expect(process.exitCode).toBe(5); + expect(out).toBe(''); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + // The marker channel is closed AND the relay channel is recalled: the + // refusal instructed the orchestrator to add the stop entry to + // unreviewedDimensions, and nothing but this sentence removes it once + // the marker (and with it compose-review's dedup splice) is gone. + expect(msg).toContain('remove it now — this convergence supersedes'); + expect(readBudgetStop(plan)).toBeNull(); // the stale marker is cleared + }); + + it('huge cap: a CONVERGED exit clears a stale same-run time-budget marker too', () => { + // The clear is cause-blind, but both sibling clear tests produce their + // marker via the round-cap gate — a cause-conditional clear + // (`if (readBudgetStop(p)?.cause === 'round-cap') clearBudgetStop(p)`) + // passes them both and leaves a time-budget marker capping a verdict + // the audit legitimately converged. Cap 5 so even round 4 reaches the + // TIME gate instead of the cap gate: cold checks due → not converged → + // admitted at the cap, refused at the near deadline. Round 5 then + // converges and must clear the time-budget marker the same way. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 5 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + runRound(1); + auditorTranscript(recordOf(1, 13), DRY); + auditorTranscript(recordOf(1, 14), DRY); + auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(3, { 15: DRY }); + + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + runRound(4); // even → not converged → 4 <= cap 5 → refused at the time gate + expect(process.exitCode).toBe(4); + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 4 by the review time budget', + ); + + process.exitCode = undefined; + const out = runRound(5); // odd → all skipped → converged, before any gate + expect(process.exitCode).toBe(5); + expect(out).toBe(''); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + expect(readBudgetStop(plan)).toBeNull(); // the stale time-budget marker is cleared + }); + + it('huge cap: a converged --chunk retry clears the stale cap marker too', () => { + // The --chunk gate threads the same convergence-first path with its own + // `args.plan`, but only the --all-chunks site's marker clear is pinned + // above: a converged per-chunk retry after a cap refusal must exit 5 + // CONVERGED and clear the stale marker exactly like it, not exit 4 at + // the cap (the ordering) and not leave the marker capping a verdict the + // audit legitimately converged (the clear). Same retry-after-refusal + // history as the --all-chunks test. + writeFileSync( + plan, + JSON.stringify({ ...PLAN, budget: { reverseAuditRounds: 3 } }), + ); + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + runRound(1); + auditorTranscript(recordOf(1, 13), DRY); + auditorTranscript(recordOf(1, 14), DRY); + auditorTranscript(recordOf(1, 15), WHIFF, { calls: 0 }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(3, { 15: DRY }); + + runRound(4); // even → retired chunks due cold checks → cap refuses + expect(process.exitCode).toBe(4); + expect(readBudgetStop(plan)?.cause).toBe('round-cap'); + + process.exitCode = undefined; + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + chunk: 13, + round: 5, + }); + expect(process.exitCode).toBe(5); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(keysOf(5)).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + expect(readBudgetStop(plan)).toBeNull(); // the stale marker is cleared + }); + it('a cold-check-only round is still built, admitted and stamped', () => { answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 75ad5f14b91..e6744a39110 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -44,6 +44,7 @@ import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; import { + clearBudgetStop, expectedRoundSeconds, readRoundStamps, reverseAuditBudgetExhausted, @@ -1885,8 +1886,14 @@ function admitReverseAuditRound( `not rebuild or retry. A marker has been recorded and compose-review ` + `will cap the verdict; still add \`${roundCapStopEntry(cap)}\` to ` + `unreviewedDimensions so the terminal report agrees. If the cap ` + - `round reported findings whose verdicts have not landed, launch ` + - `their verifiers alone and wait, then proceed to Step 6.`, + `round reported findings whose verdicts have not landed, verify them ` + + `ONLY through \`agent-prompt --role verify\` (never a hand-rolled ` + + `agent) — it is gated on the compose floor and will refuse once too ` + + `little time remains; when the deadline is within that floor, stop ` + + `waiting on any verifier batch still out and compose with the tags ` + + `in hand. Do NOT re-verify findings already confirmed in earlier ` + + `rounds, and do NOT invent a fresh re-verification pass. Then ` + + `proceed to Step 6.`, ); process.exitCode = 4; return false; @@ -1909,13 +1916,27 @@ function admitReverseAuditRound( * twice over, so another round would audit nothing the history has not * already answered. Not an error and not a gap — no record, no stamp, no * disclosure owed; a round that builds nothing was never admitted. + * + * Clears any same-run stop marker first: a converged exit can follow an + * over-cap round the gate already refused (round 4 refused under cap 3, + * then round 5's schedule is converged — the convergence check runs before + * the cap gate), and that stale round-cap marker would otherwise cap a + * verdict the audit legitimately converged. The message also recalls the + * relay channel: the earlier refusal told the orchestrator to add its stop + * entry to unreviewedDimensions, and once the marker is gone the + * compose-review splice that dedups it no longer runs — only this + * instruction removes it. */ -function refuseConverged(): void { +function refuseConverged(planPath: string): void { + clearBudgetStop(planPath); writeStderrLine( 'CONVERGED: every chunk holds two consecutive substantive dry audits; ' + 'the reverse audit has converged — stop the loop and proceed to ' + 'Step 6. This is a clean convergence, not a gap: no ' + - 'unreviewedDimensions entry is owed.', + 'unreviewedDimensions entry is owed. If an earlier round-cap or ' + + 'budget refusal told you to add its stop entry to ' + + 'unreviewedDimensions, remove it now — this convergence supersedes ' + + 'it.', ); process.exitCode = 5; } @@ -1968,7 +1989,7 @@ function runAllChunks( } if (schedule !== null && schedule.converged) { - refuseConverged(); + refuseConverged(planPath); return; } @@ -2470,7 +2491,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { schedule = null; } if (schedule !== null && schedule.converged) { - refuseConverged(); + refuseConverged(args.plan); return; } } diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 5497a0314dd..9e380d4c4ba 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -826,6 +826,24 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve expect(r.remediation.join(' ')).not.toContain('reverse audit:'); }); + it('a round-cap stop does NOT suppress the not-built gap — its rebuild is admitted', () => { + // R4-9: the reverseByDesign exemption is time-budget-ONLY. A round-cap + // marker with zero reverse-audit records must not suppress the not-built + // gap the way a time-budget stop does: the cap gate refuses only + // `round > cap`, so the gap's FIX (rebuild `--round 1`) is admitted, and + // a local run has no deadline to refuse it at all. Reading the marker + // cause-blind would silently drop both the gap and its rebuild + // remediation for a run that audited nothing. + const plan = coveredPlan([]); // no reverse-audit ran — the not-built shape + writeRoundCapStop(plan, 3, 4); + const r = composeReview({ planPath: plan, env: ENV, modelId: MODEL }); + // The round-cap marker still discloses and caps the verdict… + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain('reverse-audit round cap of 3'); + // …but the not-built gap and its rebuild remediation are still owed. + expect(r.remediation.join(' ')).toContain('reverse audit:'); + }); + it('renders the budget stop bilingually on a Han-description PR', () => { // Every sibling structural disclosure carries a zh pair; the budget stop // used to ride the caller-prose path and posted English into both halves. diff --git a/packages/cli/src/commands/review/lib/budget.ts b/packages/cli/src/commands/review/lib/budget.ts index 2106c034077..b8f87aa0c82 100644 --- a/packages/cli/src/commands/review/lib/budget.ts +++ b/packages/cli/src/commands/review/lib/budget.ts @@ -130,7 +130,7 @@ const SWEEP_FLOOR = 25; /** * The reverse-audit loop's full round cap (SKILL.md Step 5's "stop at the * plan's `reverseAuditRounds` cap"). The normal value; a huge diff gets - * `HUGE_REVERSE_AUDIT_ROUNDS` instead. `retirement.ts` re-exports it. + * `HUGE_REVERSE_AUDIT_ROUNDS` instead. `compose-review` imports it directly. */ export const MAX_REVERSE_AUDIT_ROUNDS = 5; @@ -232,10 +232,13 @@ export function reviewBudget(input: BudgetInput): ReviewBudget { /** * The reverse-audit round cap a plan's budget carries, for every reader - * that enforces or narrates it (the admission gate, the retirement - * scheduler, the cold-check note). A plan without the field — an older - * CLI — or a garbled value reads as the full cap: an old plan errs toward - * more auditing, never less, exactly like every other budget fallback. + * that enforces or narrates it (the admission gate and the cold-check + * note, both in `agent-prompt`; the retirement scheduler deliberately + * ignores the cap — whether a scheduled cold check is allowed is the note + * composer's question, not the schedule's). A plan without the field — an + * older CLI — or a garbled value reads as the full cap: an old plan errs + * toward more auditing, never less, exactly like every other budget + * fallback. * * The accepted range is floored at `HUGE_REVERSE_AUDIT_ROUNDS`, the * smallest cap the CLI ever writes. A value of one or two is out of band diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 57a51dba8ee..883de9cc898 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -1428,7 +1428,7 @@ export function verificationGaps( (k) => k === 'reverse-audit' || k.startsWith('reverse-audit--'), ); const reverse = bestDelivery(reverseKeys); - // A budget-stop marker means the round builder itself refused the reverse + // A TIME-budget stop marker means the round builder refused the reverse // audit on the run's time budget. Exactly ONE gap shape is then by design: // `not-built` — the refusal writes no record, so an audit with no records // is the audit the gate stopped, and the gap's FIX (rebuild the round) @@ -1441,7 +1441,15 @@ export function verificationGaps( // hand-written round-1 launch is exactly as undelivered when round 3 later // hits the budget, and suppressing it would let "stopped before round 3" // imply the rounds that did run were faithful. - const budgetStopped = readBudgetStop(planPath) !== null; + // + // Only the time-budget cause earns this exemption. A ROUND-CAP stop does + // NOT: the cap gate refuses only `round > cap`, so the not-built gap's FIX + // (rebuild `--round 1`) is admitted, and a local run has no deadline to + // refuse it at all — the monotone-refusal premise fails twice. So a + // round-cap marker leaves the not-built gap and its rebuild remediation + // owed, exactly as if no marker were present. + const stop = readBudgetStop(planPath); + const budgetStopped = stop !== null && stop.cause !== 'round-cap'; const reverseByDesign = budgetStopped && reverse === 'not-built'; // A repairable reverse-audit gap only at high: medium is complete without it. const reverseGap = !balancedMedium && !reverseByDesign && reverse !== 'ok'; diff --git a/packages/cli/src/commands/review/lib/deadline.test.ts b/packages/cli/src/commands/review/lib/deadline.test.ts index 8777ca869ea..0ca849f088d 100644 --- a/packages/cli/src/commands/review/lib/deadline.test.ts +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -25,6 +25,7 @@ import { DEFAULT_COMPOSE_FLOOR_SECONDS, budgetStopEntry, budgetStopEntryZh, + clearBudgetStop, expectedRoundSeconds, readBudgetStop, readRoundStamps, @@ -40,6 +41,7 @@ import { verifyBudgetMessage, writeBudgetStop, } from './deadline.js'; +import { promptRecordDir } from './prompt-record.js'; const NOW_MS = 1_754_000_000_000; const NOW_S = NOW_MS / 1000; @@ -347,6 +349,72 @@ describe('the budget-stop marker — the deterministic half of the disclosure', expect(readBudgetStop(p)?.round).toBe(3); }); + it('first refusal wins — a later cap write does not flip a same-run budget marker', () => { + // The retry-after-refusal misbehavior class: the time gate refuses round + // 3, the orchestrator asks for round 4 anyway, the cap gate fires first + // (4 > 3) and — without the guard — overwrites the marker. compose-review + // would then splice out the wrong relayed entry and post two contradictory + // stop disclosures. First-write-wins keeps the marker the audit actually + // stopped on. + const p = stopPlan(); + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 3, + NOW_MS, + ); + writeRoundCapStop(p, 3, 4, NOW_MS); + const stop = readBudgetStop(p); + expect(stop?.cause).toBeUndefined(); // still the time-budget marker + expect(stop?.entry).toBe( + 'reverse audit — stopped before round 3 by the review time budget', + ); + }); + + it('first refusal wins the other way — a later budget write does not flip a cap marker', () => { + const p = stopPlan(); + writeRoundCapStop(p, 3, 4, NOW_MS); + writeBudgetStop( + p, + { + remainingSeconds: 900, + reserveSeconds: 3600, + expectedRoundSeconds: 1800, + }, + 5, + NOW_MS, + ); + const stop = readBudgetStop(p); + expect(stop?.cause).toBe('round-cap'); + expect(stop?.cap).toBe(3); + }); + + it('clearBudgetStop removes a same-run marker — and never throws', () => { + // The CONVERGED-exit tests in agent-prompt.test.ts pin the call SITE; + // this pins the function itself, so a refactor that moves the clear out + // of refuseConverged (or unlinks a different file) fails here directly, + // not only through the loop-level tests. + const p = stopPlan(); + writeRoundCapStop(p, 3, 4, NOW_MS); + expect(readBudgetStop(p)?.cause).toBe('round-cap'); + clearBudgetStop(p); + expect(readBudgetStop(p)).toBeNull(); + // A repeat clear (file already gone), a run with no record dir at all, + // and an unlink that fails (record dir blocked by a regular file) are + // all no-ops, not throws: the file is the thing to be rid of, and a + // clear that cannot run still only leaves a cap on, never corrupts a + // verdict. + expect(() => clearBudgetStop(p)).not.toThrow(); + const fresh = stopPlan(); + expect(() => clearBudgetStop(fresh)).not.toThrow(); + writeFileSync(promptRecordDir(fresh), 'a file where the record dir goes'); + expect(() => clearBudgetStop(fresh)).not.toThrow(); + }); + it('the dedup phrase travels with the entry it identifies', () => { // compose-review dedups the orchestrator's relayed copy by this phrase; // a reword of the entry that left the phrase behind would post the @@ -431,6 +499,11 @@ describe('reverseAuditBudgetMessage', () => { expect(msg).toContain('never a hand-rolled agent'); expect(msg).toContain('compose floor'); expect(msg).toContain('Do NOT re-verify findings already'); + // The wait-bound and no-fresh-pass clauses the round-cap refusal's + // tail carries (and SKILL.md's budget-stop bullet documents) — the + // two refusals share one bounded-tail protocol, so both pin both. + expect(msg).toContain('stop waiting on any verifier batch still out'); + expect(msg).toContain('invent a fresh re-verification pass'); }); it('says "the next round" when no round number was passed', () => { @@ -464,6 +537,30 @@ describe('writeRoundCapStop — the round-cap marker', () => { } }); + it('a cap marker from before the plan capture is a previous run — the guard still writes', () => { + // Mirror of the budget-stop fence test for the round-cap writer: run 1 + // stops at the cap and is killed before cleanup; run 2 re-captures the + // plan and runs past the cap again. The first-refusal-wins guard must + // read through the stale file via the run-epoch fence — a raw + // existsSync check would make run 2's writeRoundCapStop a no-op, and + // compose-review would neither cap the verdict nor print the stop + // disclosure for an audit that stopped at the cap. + const dir = mkdtempSync(join(tmpdir(), 'rc-stop-')); + try { + const plan = join(dir, 'plan.json'); + writeFileSync(plan, '{}'); + backdatePlan(plan); + writeRoundCapStop(plan, 3, 4, PLAN_CAPTURED_MS - 28_800_000); // 8h before capture + expect(readBudgetStop(plan)).toBeNull(); // fenced out as a previous run + writeRoundCapStop(plan, 3, 4, NOW_MS); + const stop = readBudgetStop(plan); + expect(stop?.cause).toBe('round-cap'); + expect(stop?.cap).toBe(3); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('disclosure names the cap in both languages', () => { expect(roundCapStopDisclosure(3).reason).toContain(ROUND_CAP_PHRASE); expect(roundCapStopDisclosure(3).reason).toContain('of 3'); diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index c2ab90b4063..8bb1ee5c794 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -39,7 +39,13 @@ // still bounds the run, and a broken environment variable must degrade to // today's behaviour, not wedge every budgeted review at round 1. -import { mkdirSync, readFileSync, statSync, writeFileSync } from 'node:fs'; +import { + mkdirSync, + readFileSync, + rmSync, + statSync, + writeFileSync, +} from 'node:fs'; import { join } from 'node:path'; import { promptRecordDir } from './prompt-record.js'; @@ -504,7 +510,8 @@ export function roundCapStopEntryZh(cap: number): string { * without converging — without depending on the orchestrator to relay the * entry. Same marker file and same swallow-on-write-error discipline as * `writeBudgetStop`; only one stop fires per run, whichever refusal comes - * first. + * first — the same-run guard below enforces it, so a retry-past-cap after a + * time-budget stop cannot flip the recorded cause. */ export function writeRoundCapStop( planPath: string, @@ -513,6 +520,11 @@ export function writeRoundCapStop( nowMs: number = Date.now(), ): void { try { + // First refusal wins: a same-run marker already on disk (its run-epoch + // fence in `readBudgetStop` excludes previous runs') is left untouched, + // so a time-budget stop followed by a retry that the cap then refuses + // does not post two contradictory stop disclosures. + if (readBudgetStop(planPath) !== null) return; const dir = promptRecordDir(planPath); mkdirSync(dir, { recursive: true }); const stop: BudgetStop = { @@ -536,7 +548,8 @@ export function writeRoundCapStop( * reads it back and synthesizes the verdict-capping disclosure without * depending on the orchestrator to relay a sentence. Write errors are * swallowed: the stderr instruction still carries the entry, and a gate - * that cannot write must still refuse. + * that cannot write must still refuse. First refusal wins here too — a + * same-run marker already on disk is left untouched. */ export function writeBudgetStop( planPath: string, @@ -545,6 +558,7 @@ export function writeBudgetStop( nowMs: number = Date.now(), ): void { try { + if (readBudgetStop(planPath) !== null) return; const dir = promptRecordDir(planPath); mkdirSync(dir, { recursive: true }); const stop: BudgetStop = { @@ -591,6 +605,23 @@ export function readBudgetStop(planPath: string): BudgetStop | null { } } +/** + * Remove any stop marker beside the prompt records. Called when the loop + * reaches a clean end that outranks an earlier same-run refusal — a + * CONVERGED exit after an over-cap round was refused: the marker would + * otherwise survive (nothing else unlinks it) and cap a verdict the audit + * legitimately converged. Missing file and unlink errors are swallowed — + * the file was the thing to be rid of. + */ +export function clearBudgetStop(planPath: string): void { + try { + rmSync(join(promptRecordDir(planPath), STOP_FILE), { force: true }); + } catch { + // Best-effort: a marker we could not remove still only caps a verdict, + // never corrupts one, and the converged stderr is the load-bearing half. + } +} + /** * The refusal, spelled as the termination rule it is. Printed to stderr by * `agent-prompt` alongside exit code 4; the disclosure sentence matches the @@ -620,9 +651,11 @@ export function reverseAuditBudgetMessage( `\`agent-prompt --role verify\` (never a hand-rolled agent) — it is gated ` + `on the compose floor and will refuse once too little time remains, ` + `leaving any still-\`[unverified]\` findings tagged for compose-review to ` + - `cap — then compose and submit. Do NOT re-verify findings already ` + - `confirmed in earlier rounds. A review that stops here still reports ` + - `everything it proved; a review that runs past its deadline is killed ` + - `holding all of it.` + `cap; when the deadline is within that floor, stop waiting on any ` + + `verifier batch still out and compose with the tags in hand. Do NOT ` + + `re-verify findings already confirmed in earlier rounds, and do NOT ` + + `invent a fresh re-verification pass. Then compose and submit — a ` + + `review that stops here still reports everything it proved; a review ` + + `that runs past its deadline is killed holding all of it.` ); } diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 262fed58f17..c6127602f0b 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -591,10 +591,10 @@ The countermeasure is cheap and needs no new machinery: before Step 4, sanity-ch | ----------------------- | ------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Review agents | 14 (+0-2) | issue fidelity + 3 procedural correctness walks (1a/1b/1c) + security + 3 quality slices (3a/3b/3c) + perf/tests + 3 undirected personas + build&test, plus 0-2 diff-specialized finders; cross-repo skips Agents 7 and 1c (12), non-PR skips Agent 0 (13) | | Sharded verification | `ceil(F/8)` | F = findings; typically 1-2; keeps each verifier's job small on high-finding reviews | -| Iterative reverse audit | 2-5 | loop ends after two consecutive dry rounds; 5-round hard cap | +| Iterative reverse audit | 2-5 (2-3 huge) | loop ends after two consecutive dry rounds; 5-round hard cap — 3 for a huge diff (effective ≥ 3000 lines) | | **Total** | **~17-23 (~15-22)** | Row maxima do not co-occur on typical runs (~17-19 is common), but the honest sum of ranges is 17-23 same-repo, 15-22 cross-repo/local. **Low effort: 0 subagent calls** — the angle rotation runs in the orchestrator's own context; medium launches its reduced fan-out | -**Large diffs (> 500 source lines OR > 3200 total diff lines, Step 3B, high effort) — `ceil(diffLines / 400)` chunk agents + `5..7` whole-diff agents + `3H` invariant agents (H = heavy files) + `ceil(F/8)` verify (F = findings) + `rounds × chunks` reverse audit.** The reverse audit dominates: it fans out one auditor per chunk per round, and the stop rule needs two consecutive dry rounds (hard cap 5). PR #6457 (5801 diff lines, 19 chunks, 1 heavy file) costs ~27-29 first-wave calls, then `19 × (2..5) = 38-95` reverse auditors — ~66-126 calls total depending on how long the audit keeps finding; ~70 is the clean-run floor, and the count scales with chunks and findings, not a fixed ceiling. +**Large diffs (> 500 source lines OR > 3200 total diff lines, Step 3B, high effort) — `ceil(diffLines / 400)` chunk agents + `5..7` whole-diff agents + `3H` invariant agents (H = heavy files) + `ceil(F/8)` verify (F = findings) + `rounds × chunks` reverse audit.** The reverse audit dominates: it fans out one auditor per chunk per round, and the stop rule needs two consecutive dry rounds (hard cap 5 — **3 for a huge diff, effective ≥ 3000 lines**, which narrows the per-chunk multiplier to `2..3`). PR #6457 (5801 diff lines, 19 chunks, 1 heavy file) costs ~27-29 first-wave calls, then `19 × (2..5) = 38-95` reverse auditors — ~66-126 calls total depending on how long the audit keeps finding (a huge-diff cap-3 run of the same shape narrows this to `19 × (2..3) = 38-57`); ~70 is the clean-run floor, and the count scales with chunks and findings, not a fixed ceiling. That is roughly 4x the small-diff budget, and it buys the thing the small-diff topology cannot deliver at that size: coverage. Ten dimension agents (the roster of the day; fourteen now) on a 5801-line diff each read the same truncated 14% window (see "Why the diff is a file, not a command"), so nine of the ten calls were redundant reads of the same hunks. Nineteen chunk agents each read a distinct ~390-line territory, and every line of the diff has exactly one accountable owner. The comparison to make is not ~70 calls vs ~17: PR #6457 took **eight** review rounds at 12-14 calls each — over 100 calls — and was still surfacing Criticals in code that had been in the diff since the first commit. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index b824baabbda..df6b39ee42f 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -667,8 +667,8 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - A round is **dry** only when _every_ agent in it returned zero new findings **with** the evidence-bearing receipt (`No issues found — `). A round containing a twice-whiffed agent is **not dry** — silence is not convergence evidence — so the loop continues (the hard cap below still bounds it). - **When the loop ends with any scope still outstanding** (by cap, or by dry rounds elsewhere), terminal prose is not enough: add one self-explained entry per scope to `unreviewedDimensions` — e.g. `reverse audit of chunk 3 — the auditor returned nothing substantive twice` — so compose-review serializes it and caps a would-be Approve at `COMMENT`. The primary Step 3 pass did read that scope (its receipt stands), but this run's contract includes the reverse audit, and a verdict must not silently claim an audit that never ran. - Stop after **two consecutive dry rounds** (the 3A criterion — one auditor, so round-dry and territory-dry are the same thing). One dry round is not evidence of convergence: on PR #6457 the review returned "no blockers" twice and the very next round surfaced five Criticals, three of them in code that had been in the diff since the first commit. A single lazy agent must not be able to end the loop. A dry convergence pair satisfies this rule in one launch — its two members are exactly the two independent audits the rule demands; what the pair removes is the wall clock between them, not either audit. When the loop ends on this rule, the last reporting round's verifiers are already in flight (they launched with the next round's auditors) — wait for their verdicts and apply them in the final merge before Step 6. -- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a convergence at the cap round (round 5 normally, round 3 under the huge-diff cap). (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) -- Stop at the plan's **`reverseAuditRounds` cap** — 5, or 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so launch them alone before Step 6 and wait for their verdicts; the tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. +- **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). If an earlier round-cap or budget refusal told you to add its stop entry to `unreviewedDimensions`, remove it now — this convergence supersedes that stop (the marker on disk is cleared the same way). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a convergence at the cap round (round 5 normally, round 3 under the huge-diff cap). (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) +- Stop at the plan's **`reverseAuditRounds` cap** — 5, or 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. The tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. - Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the plan's round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is hashed into the record key and copied to the digest-named list file each block points at — so a launch that drops the pointer matches no record, and the delivery floor counts the agent's read of that file exactly as it counts the brief's. - **A reporting round whose every finding the verifier rejected is retroactively dry.** The merge already removes a rejected entry from the cumulative list; from the merge that applies the last of a round's rejections, the round also stops counting as a reporting round, and the two-consecutive-dry rule reads rounds' **effective** status. Rejected means rejected — an entry confirmed at low confidence keeps its round a reporting round. Under the pipelined loop a round's verdicts land while the next round runs, so the upgrade usually arrives one round late, and that is still one round saved: a measured run held round 2 dry, watched round 3's sole finding be rejected, and then ran rounds 4 **and 5** — round 4's dry return plus the rejection already in hand was the two-dry evidence, and the fifth round audited nothing the loop had not already answered (measured; DESIGN.md — The rounds a rejected finding bought (PR #8353)). The rule leans on the rejection bar the verifier's brief already enforces — a rejection claims direct counter-evidence, never mere unverifiability — so a round retired by rejections is retired on evidence, not on doubt. **It pairs forward only, and is consulted when a round returns**: on round _k_'s dry return, first apply every verdict that has landed (the unconditional merge — the retirement takes effect at this application, not at some earlier moment), then end the loop if round _k−1_ was dry or is now retired. Round _k−1_ counts **launches, not labels**: the convergence pair is one round here — a pair member is never round _k−1_ on its own (the pair bullet's not-carried-forward rule stands), and a reporting pair retires only when every finding from **both** members is rejected. The upgrade never ends the loop by itself — a preceding dry round plus a freshly-retired round stops nothing while the next round is already in flight: that round was launched, and its return is taken whatever it says, because a launched auditor can be carrying a real Critical. This is the measured shape (round 4's return is where the loop closes under this rule — the measured run, which predates it, ran a fifth round; a cap-5 shape — under the 3-round huge-diff tier the upgrade can only ever retire rounds 1–2, since the cap round's verdicts land during its solo verification, after the loop has already ended) and the only pairing licensed here. It softens nothing else: a whiffed scope stays not-audited whatever the verdicts say, and on 3B the retirement ledger's per-chunk certificates are untouched — this rule reads at the level the round counter reads. - **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. (Step 4's initial verification is the k=0 case of the same rule: its shards ride with the first reverse-audit launch — the convergence pair on 3A, round 1's fan-out on 3B.) The serial shape (audit → wait for verification → next round) spent 5-8 minutes per round waiting for verifiers whose results the next round's auditors never needed. Two orderings still hold: the **last** round's verification must complete before Step 6 (that ordering is what keeps unverified entries out of the report and the PR, backed by the tag backstop at the end of this step — which `compose-review` machine-checks from `findingsPath`, Step 6), and a rejected finding leaves the cumulative list at the next merge. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 20767227316..fd7392bcf5f 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -81,6 +81,25 @@ describe('bundled review skill', () => { expect(body).toContain('`agent-prompt --roster` after the rules load'); }); + it('pins the bounded-tail protocol on the round-cap bullet', () => { + // The ROUND CAP refusal message carries the same verify-only / + // compose-floor contract; a revert of the bullet's protocol hunk must + // fail a test, not slip through. + const body = skillBody(); + expect(body).toContain('`agent-prompt --role verify` **only**'); + expect(body).toContain('no fresh re-verification pass'); + }); + + it('pins the relay-entry removal on the CONVERGED bullet', () => { + // The CONVERGED clear removes the marker on disk, but the entry an + // earlier stop refusal told the orchestrator to relay is orchestrator + // state — compose-review's dedup splice stops running once the marker + // is gone, so only this instruction recalls it. A revert of the + // sentence must fail a test, not slip through. + const body = skillBody(); + expect(body).toContain('remove it now — this convergence supersedes'); + }); + it('routes both remote-resolution paths through match-remote', () => { // The pr-url path (Step 1) and the bare-PR-number path both resolve the // remote via the deterministic matcher. A later edit reverting either