diff --git a/.github/workflows/qwen-code-pr-review.yml b/.github/workflows/qwen-code-pr-review.yml index 6985b028056..e9d3bcfc3cd 100644 --- a/.github/workflows/qwen-code-pr-review.yml +++ b/.github/workflows/qwen-code-pr-review.yml @@ -828,18 +828,22 @@ jobs: # The budget itself is chosen outside this file — repository # variable, workflow input, or a /review --timeout=N comment — so # the reserve scales with whatever arrived instead of assuming a - # size: a quarter of the attempt, floored at 10 minutes (a tiny + # size: a third of the attempt, floored at 10 minutes (a tiny # explicit budget degrades to skipping the audit loop, correctly) - # and capped at 60 (the verify+compose+post tail does not grow - # just because the budget did). The 3600 cap mirrors + # and capped at 80 (the verify+compose+post tail does not grow + # just because the budget did). Under the pipelined loop the + # reserve is the terminal round's ONLY cover, and the only tail + # ever measured (#8368) was past 30 minutes and still running + # when the kill arrived — so the third is insurance until + # pipelined runs measure their tails. The 4800 cap mirrors # DEFAULT_RESERVE_SECONDS in packages/cli/src/commands/review/ # lib/deadline.ts (the CLI's fallback when this var is absent) — # keep the two in sync. - QWEN_REVIEW_DEADLINE_RESERVE_SECONDS="$(( attempt_timeout / 4 ))" + QWEN_REVIEW_DEADLINE_RESERVE_SECONDS="$(( attempt_timeout / 3 ))" if [ "$QWEN_REVIEW_DEADLINE_RESERVE_SECONDS" -lt 600 ]; then QWEN_REVIEW_DEADLINE_RESERVE_SECONDS=600 - elif [ "$QWEN_REVIEW_DEADLINE_RESERVE_SECONDS" -gt 3600 ]; then - QWEN_REVIEW_DEADLINE_RESERVE_SECONDS=3600 + elif [ "$QWEN_REVIEW_DEADLINE_RESERVE_SECONDS" -gt 4800 ]; then + QWEN_REVIEW_DEADLINE_RESERVE_SECONDS=4800 fi export QWEN_REVIEW_DEADLINE_RESERVE_SECONDS set +e diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 44ba8babeb6..33285c42e94 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -2472,17 +2472,24 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); }); - function call(role: string, extra: Record = {}): string { - const dir = mkdtempSync(join(tmpdir(), 'ap-budget-')); - dirs.push(dir); - const plan = join(dir, 'plan.json'); - writeFileSync(plan, JSON.stringify(PLAN)); - const findings = join(dir, 'findings.md'); - writeFileSync(findings, ''); + /** Run the handler; `planPath` reuses an earlier call's plan and findings. */ + function call( + role: string, + extra: Record = {}, + planPath?: string, + ): string { + let plan = planPath; + if (plan === undefined) { + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-')); + dirs.push(dir); + plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); + writeFileSync(join(dir, 'findings.md'), ''); + } (agentPromptCommand.handler as (a: unknown) => void)({ plan, role, - findings, + findings: join(dirname(plan), 'findings.md'), ...extra, }); return plan; @@ -2605,6 +2612,134 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); }); + it('exempts a --chunk repair of an ADMITTED round — even past the deadline', () => { + // A --chunk call on a STAMPED round rebuilds one auditor of a round + // already admitted (a truncated delivery, repaired per chunk); its cost + // was counted when the round was admitted. Refusing it leaves the + // truncation unrepairable under a disclosure naming the wrong round — + // so the stamp, and only the stamp, buys the exemption. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const plan = call('reverse-audit', { 'all-chunks': true, round: 3 }); + expect(process.exitCode).toBeUndefined(); + expect(readRoundStamps(plan).some((s) => s.round === 3)).toBe(true); + + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) - 600); + call('reverse-audit', { chunk: 13, round: 3 }, plan); + + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + expect((writeStderrLine as unknown as Mock).mock.calls).toHaveLength(0); + // The repair stamps nothing new: the round it repairs carries the + // admission, and a rebuild's clock must not measure as the round's cost. + expect(readRoundStamps(plan).filter((s) => s.round === 3)).toHaveLength(1); + // And the expired-deadline repair leg writes no budget-stop marker: + // the round was admitted, so no truncation disclosure is owed. + expect(readBudgetStop(plan)).toBeNull(); + }); + + it('gates a --chunk build of a round never admitted — no stamp, no exemption', () => { + // The probe that found the bypass: an expired deadline refuses + // `--all-chunks --round 4` and writes the "stopped before round 4" + // marker — and then N per-chunk builds of round 4 each exited 0, running + // the round past the deadline while the disclosure said it never + // started. Without a round-4 stamp there is no admitted round to + // repair, so the --chunk build answers to the same gate. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) - 600); + const plan = call('reverse-audit', { 'all-chunks': true, round: 4 }); + expect(process.exitCode).toBe(4); + + process.exitCode = undefined; + (writeStdoutLine as unknown as Mock).mockClear(); + call('reverse-audit', { chunk: 13, round: 4 }, plan); + + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(readRecordedPrompts(plan).size).toBe(0); + expect(readRoundStamps(plan)).toHaveLength(0); + // The refusal is the round's, not a path of its own: one marker, one + // disclosure, whichever flag asked. + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 4 by the review time budget', + ); + }); + + it('the exemption keys on the stamp, not the record — a half-built round stays refused', () => { + // Reachable state: an --all-chunks build whose second chunk has an + // unusable line range passes requireAuditableChunks (which validates + // ids only), records the first chunk's prompt inside the block map, + // then throws before the stamp is written. A record without a stamp + // is NOT an admitted round: keying the exemption on the recorded + // prompts would let every later --chunk build of it past an expired + // deadline — the #8368-class bypass this gate closes. + const dir = mkdtempSync(join(tmpdir(), 'ap-budget-half-')); + dirs.push(dir); + const planPath = join(dir, 'plan.json'); + const halfBroken = { + ...PLAN, + chunks: [ + PLAN.chunks[0], + { ...PLAN.chunks[1], startLine: null }, + PLAN.chunks[2], + ], + }; + writeFileSync(planPath, JSON.stringify(halfBroken)); + const findingsPath = join(dir, 'findings.md'); + writeFileSync(findingsPath, ''); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + expect(() => + (agentPromptCommand.handler as (a: unknown) => void)({ + plan: planPath, + role: 'reverse-audit', + findings: findingsPath, + 'all-chunks': true, + round: 3, + }), + ).toThrow(/no usable line range/); + // One record (chunk 13), zero stamps — exactly the state the probe needs. + expect( + [...readRecordedPrompts(planPath).keys()].some((k) => + k.includes('--chunk-13--round-3--'), + ), + ).toBe(true); + expect(readRoundStamps(planPath)).toHaveLength(0); + + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) - 600); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan: planPath, + role: 'reverse-audit', + findings: findingsPath, + chunk: 13, + round: 3, + }); + + // Refused — the record buys no exemption — with the round's marker. + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(readBudgetStop(planPath)?.entry).toBe( + 'reverse audit — stopped before round 3 by the review time budget', + ); + }); + + it('the first --chunk build of an unadmitted round IS its admission', () => { + // An orchestrator building a round per chunk from the start pays the + // gate once: the first build stamps the round, the next round's estimate + // measures from it, and the later chunk builds are repairs of it. + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7200); + const plan = call('reverse-audit', { chunk: 13, round: 3 }); + + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + expect(readRoundStamps(plan).some((s) => s.round === 3)).toBe(true); + // An admission leaves no budget-stop marker: both consumers key on + // presence alone, and a defensive write here would cap every admitted + // run's verdict with a false truncation disclosure. + expect(readBudgetStop(plan)).toBeNull(); + }); + it('a broken plan still throws when the budget is exhausted — reads beat the gate', () => { // The gate needs only the plan's PATH, but it must not speak first: a // refusal would record a budget stop against a plan that cannot even @@ -2722,9 +2857,9 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () writeFileSync(plan, JSON.stringify(PLAN)); // this run's capture, after const findings = join(dir, 'findings.md'); writeFileSync(findings, ''); - // 5500s remaining fits reserve + the 1800s CONSTANT (5400) — admitted — + // 7000s remaining fits reserve + the 1800s CONSTANT (6600) — admitted — // while the stale ~28800s measurement would refuse. - process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 5500); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7000); (agentPromptCommand.handler as (a: unknown) => void)({ plan, role: 'reverse-audit', @@ -2738,7 +2873,7 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () it("measures the previous round's cost at the gate, not the constant", () => { // Round 1 admitted with a far deadline (it stamps); backdate the stamp // 3000s. The second deadline leaves room for reserve + the CONSTANT - // round estimate (3600 + 1800 fits in 5500) but not for reserve + the + // round estimate (4800 + 1800 fits in 7000) but not for reserve + the // MEASURED 3000s — so only a gate that measures refuses. The unsafe // direction is under-estimation: admitting a terminal round that does // not fit, the killed-mid-verification outcome this gate exists to @@ -2754,7 +2889,7 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () // THIS run, and the previous-run fence keys on the plan's mtime. const captured = (Date.now() - 4_000_000) / 1000; utimesSync(plan, captured, captured); - process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 5500); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 7000); (agentPromptCommand.handler as (a: unknown) => void)({ plan, role: 'reverse-audit', @@ -2773,3 +2908,559 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () expect(readRoundStamps(plan)).toHaveLength(1); }); }); + +describe('per-chunk retirement — cold territories stop costing a round', () => { + // Measured on a real 3B run (6 chunks × 5 rounds = 30 auditors, ~95 + // minutes): chunks 3 and 6 were dry in ALL five rounds; chunks 1, 2 and 4 + // yielded in most. The round-global convergence rule made one hot chunk + // keep every cold one under audit for the whole run. These tests drive the + // real handler round by round, writing transcripts the way the harness + // does, and assert the schedule that falls out of that history. + const dirs: string[] = []; + let dir: string; + let plan: string; + let findings: string; + let seq = 0; + const SAVED: Record = {}; + const DIFF = PLAN.diffPathAbsolute; + + // Substantive receipts and returns, in the shapes the classifier reads: + // DRY clears both the no-issues phrase and the ~120-char substance floor; + // WHIFF is the bare stock sentence the floor exists to reject; YIELD files + // a finding block against a real file. + const DRY = + 'No new issues found — re-walked the whole territory, the retry cap and ' + + "both changed exports' call sites; every gap I checked was already in " + + 'the confirmed list.'; + const WHIFF = 'No issues found.'; + const YIELD = + 'Found one gap the prior rounds missed.\n\n' + + '- **File:** packages/cli/src/commands/review/x.test.ts:12\n' + + '- **Anchor:** const a = 1\n' + + '- **Issue:** off-by-one in the retry cap\n' + + '- **Severity:** Suggestion\n'; + + beforeEach(() => { + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + dir = mkdtempSync(join(tmpdir(), 'ap-retire-')); + dirs.push(dir); + plan = join(dir, 'plan.json'); + writeFileSync(plan, JSON.stringify(PLAN)); // chunks 13, 14, 15 + // Backdate the plan so every transcript this test writes counts as newer + // — the same mtime fence coverage uses against a previous review's agents. + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + findings = join(dir, 'findings.md'); + writeFileSync(findings, ''); + for (const k of ['QWEN_CODE_PROJECT_DIR', 'QWEN_CODE_SESSION_ID']) { + SAVED[k] = process.env[k]; + } + process.env['QWEN_CODE_PROJECT_DIR'] = dir; + process.env['QWEN_CODE_SESSION_ID'] = 'S1'; + mkdirSync(join(dir, 'subagents', 'S1'), { recursive: true }); + }); + afterEach(() => { + process.exitCode = undefined; + delete process.env[DEADLINE_ENV]; + for (const [k, v] of Object.entries(SAVED)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); + }); + + /** Run one --all-chunks round through the real handler; return its stdout. */ + function runRound(round: number): string { + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + findings, + 'all-chunks': true, + round, + }); + const calls = (writeStdoutLine as unknown as Mock).mock.calls; + return calls.length > 0 ? (calls[0][0] as string) : ''; + } + + /** The record the round's build wrote for one chunk — the launch text. */ + function recordOf(round: number, chunk: number): string { + for (const [key, prompt] of readRecordedPrompts(plan)) { + if (key.startsWith(`reverse-audit--chunk-${chunk}--round-${round}--`)) { + return prompt; + } + } + throw new Error(`no record for chunk ${chunk} round ${round}`); + } + + /** Round-`round` record keys, one string per chunk they were built for. */ + function keysOf(round: number): string[] { + return [...readRecordedPrompts(plan).keys()].filter((k) => + k.includes(`--round-${round}--`), + ); + } + + /** + * Write a transcript the way the harness writes one: the launch prompt as + * the first record, then `calls` successful reads of the diff, then the + * final text. `calls: 0` is the whiff shape — prose and nothing else. + */ + function auditorTranscript( + launchPrompt: string, + finalText: string, + opts: { calls?: number } = {}, + ): void { + const id = `aud-${++seq}`; + const base = { agentId: id, agentName: 'general-purpose', sessionId: 'S1' }; + // Read the territory the launch bakes, the way a verbatim delivery + // does: the dry bar compares the lines a transcript read against the + // record's own baked read, so a synthetic auditor must open the same + // window its prompt names. + const baked = /offset=(\d+), limit=(\d+)/.exec(launchPrompt); + const readOffset = baked ? Number(baked[1]) : 0; + const readLimit = baked ? Number(baked[2]) : 100; + const lines = [ + JSON.stringify({ + ...base, + type: 'user', + message: { role: 'user', parts: [{ text: launchPrompt }] }, + }), + ]; + for (let i = 0; i < (opts.calls ?? 1); i++) { + lines.push( + JSON.stringify({ + ...base, + type: 'assistant', + message: { + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { + file_path: DIFF, + offset: readOffset, + limit: readLimit, + }, + }, + }, + ], + }, + }), + JSON.stringify({ + ...base, + type: 'tool_result', + message: { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: 'diff bytes' }, + }, + }, + ], + }, + }), + ); + } + lines.push( + JSON.stringify({ + ...base, + type: 'assistant', + message: { role: 'model', parts: [{ text: finalText }] }, + }), + ); + writeFileSync( + join(dir, 'subagents', 'S1', `agent-${id}.jsonl`), + lines.join('\n') + '\n', + ); + } + + /** + * Run a round and answer each built chunk with the given final text — the + * transcript's launch prompt is the record itself, exactly what a verbatim + * delivery looks like. `null` answers with no transcript at all. + */ + function answerRound( + round: number, + texts: Record, + ): string { + const out = runRound(round); + for (const [chunk, text] of Object.entries(texts)) { + if (text === null) continue; + auditorTranscript(recordOf(round, Number(chunk)), text); + } + return out; + } + + it('rounds 1 and 2 always fan out to every chunk — they establish the record', () => { + const r1 = answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + expect(r1).toContain('3 auditors required this round — one per chunk.'); + // Even on a round-1 history that is already all-dry, round 2 is full: + // one dry audit is not a certificate, and the rule only reads at k >= 3. + const r2 = runRound(2); + expect(r2).toContain('3 auditors required this round — one per chunk.'); + expect(r2).not.toContain('retirement:'); + expect(keysOf(2)).toHaveLength(3); + }); + + it('round 3 skips a chunk dry in rounds 1 and 2, and the note names it', () => { + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + const out = runRound(3); + + expect(process.exitCode).toBeUndefined(); + expect(out).toContain('2 auditors required this round'); + expect(out).toContain('— chunk 14 ─'); + expect(out).toContain('— chunk 15 ─'); + expect(out).not.toContain('— chunk 13 ─'); + expect(out).toContain('───── end of round — 2 auditors ─────'); + // The certificate, after the end-of-round line, exactly relayable. + expect(out).toContain( + 'chunk 13 — retired: dry in rounds 1 and 2, next cold check round 4', + ); + expect(out.indexOf('retirement:')).toBeGreaterThan( + out.indexOf('end of round'), + ); + // The skipped chunk leaves no record — nothing downstream is owed a + // launch for it (check-coverage's roster never contains reverse-audit + // keys, and verificationGaps reads only keys that exist). + const keys = keysOf(3); + expect(keys).toHaveLength(2); + expect(keys.some((k) => k.includes('--chunk-13--'))).toBe(false); + // A partial round is still an admission: the stamp is written. + expect(readRoundStamps(plan).some((s) => s.round === 3)).toBe(true); + }); + + it('findings quoting a read window cannot widen a territory', () => { + // The record is the FOLDED prompt — the cumulative findings list rides + // inside it verbatim. Prose quoting a read window used to inject the + // range into the baked territory, and any-overlap-passes meant an + // auditor that read only the diff's head could retire a chunk whose + // territory is thousands of lines below — the hole the territory check + // closed, reopened by honest findings. Chunk 13's territory is + // 3808-4024; the injected `offset=0, limit=50` is exactly the window + // the auditors below read. + writeFileSync( + findings, + '- **File:** packages/cli/src/x.ts:12 — the earlier read used ' + + 'offset=0, limit=50 — **Severity:** Suggestion\n', + ); + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + + const out = runRound(3); + expect(out).toContain('3 auditors required this round'); + expect(out).not.toContain('retirement:'); + }); + + it('a round-5 skip names the certificate final — the cap forbids round 6', () => { + // 13 yields in rounds 1,2 (hot), then goes dry in 3,4 — retiring at + // round 5, whose next cold check would be round 6: past the 5-round + // hard cap. The note is the orchestrator's only word about the chunk; + // it must not promise an audit the cap forbids. + answerRound(1, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: YIELD, 14: YIELD, 15: YIELD }); + answerRound(3, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(4, { 13: DRY, 14: YIELD, 15: YIELD }); + + const out = runRound(5); + expect(out).toContain('2 auditors required this round'); + expect(out).toContain('chunk 13 — retired: dry in rounds 3 and 4'); + expect(out).toContain('certificate final'); + expect(out).not.toContain('next cold check round 6'); + }); + + it('the cold check comes due on parity — the retired chunk is built again', () => { + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(3, { 14: YIELD, 15: YIELD }); // 13 skipped, odd offset + const out = runRound(4); + + // (4 - 2) is even: the cold check is due, and the round is whole again. + expect(out).toContain('3 auditors required this round — one per chunk.'); + expect(out).toContain('— chunk 13 (cold check) ─'); + expect(out).not.toContain('retirement:'); + expect(keysOf(4)).toHaveLength(3); + }); + + it('a cold check that yields returns the chunk to every-round auditing', () => { + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(3, { 14: YIELD, 15: YIELD }); + answerRound(4, { 13: YIELD, 14: YIELD, 15: YIELD }); // the cold check yields + const out = runRound(5); + + // Its two most recent audits are now [dry, yielded]: hot, due, untagged. + expect(out).toContain('3 auditors required this round — one per chunk.'); + expect(out).toContain('— chunk 13 ─'); + expect(out).not.toContain('(cold check)'); + expect(out).not.toContain('retired'); + }); + + it('a whiffed or missing receipt keeps the chunk hot', () => { + answerRound(1, { 13: DRY, 14: DRY, 15: YIELD }); + // 13's round-2 receipt is the bare stock sentence (under the substance + // floor, zero tool calls); 14's round-2 auditor left no transcript at + // all. Neither is a dry audit, so neither chunk may retire. + const r2 = runRound(2); + auditorTranscript(recordOf(2, 13), WHIFF, { calls: 0 }); + auditorTranscript(recordOf(2, 15), YIELD); + expect(r2).toContain('3 auditors required'); + const out = runRound(3); + expect(out).toContain('3 auditors required this round — one per chunk.'); + expect(out).not.toContain('retirement:'); + }); + + it('all retired and none due: exit 5, CONVERGED, nothing built, nothing stamped', () => { + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + const recordsBefore = readRecordedPrompts(plan).size; + const stampsBefore = readRoundStamps(plan).length; + const out = runRound(3); + + expect(process.exitCode).toBe(5); + expect(out).toBe(''); // no stdout blocks at all + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + expect(msg).toContain('stop the loop and proceed to Step 6'); + expect(msg).toContain('no unreviewedDimensions entry is owed'); + // No new records, and no admission stamp — a round that builds nothing + // was never admitted, and must not skew the next admission's estimate. + expect(readRecordedPrompts(plan).size).toBe(recordsBefore); + expect(readRoundStamps(plan)).toHaveLength(stampsBefore); + }); + + 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 }); + // Round 4 directly: (4 - 2) is even for every chunk, so the whole round + // is cold checks — built, and stamped like any admission. + const out = runRound(4); + + expect(process.exitCode).toBeUndefined(); + expect(out).toContain('3 auditors required this round'); + expect(out).toContain('— chunk 13 (cold check) ─'); + expect(out).toContain('— chunk 15 (cold check) ─'); + expect(keysOf(4)).toHaveLength(3); + expect(readRoundStamps(plan).some((s) => s.round === 4)).toBe(true); + }); + + it('a --chunk rebuild of an admitted round bypasses retirement — a repair is not scheduling', () => { + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + runRound(3); // admits round 3; 13 is retired and not built + // 13 is retired and NOT due at round 3 — but round 3 is stamped, so the + // rebuild path is the orchestrator repairing a delivery, and it must + // never be refused one. + (writeStdoutLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + chunk: 13, + findings, + round: 3, + }); + expect(process.exitCode).toBeUndefined(); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + expect( + keysOf(3).some((k) => k.startsWith('reverse-audit--chunk-13--')), + ).toBe(true); + }); + + it('a converged round cannot be rebuilt one auditor at a time', () => { + // The converged builder exits 5 and stamps nothing — so a --chunk build + // of that round is NOT a repair, and letting it through would reopen a + // loop the history has closed, one auditor per call. Same exit, same + // instruction: the audit is done. + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + chunk: 13, + findings, + round: 3, + }); + expect(process.exitCode).toBe(5); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(keysOf(3)).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + expect(readRoundStamps(plan).some((s) => s.round === 3)).toBe(false); + }); + + it('transcripts unavailable: full fan-out, never fewer', () => { + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + // The history says "converged" — but without the harness's records it is + // unreadable, and an unreadable history must degrade to today's + // behaviour: every territory audited. + delete process.env['QWEN_CODE_PROJECT_DIR']; + const out = runRound(3); + expect(process.exitCode).toBeUndefined(); + expect(out).toContain('3 auditors required this round — one per chunk.'); + expect(keysOf(3)).toHaveLength(3); + }); + + it('a converged --chunk build exits 5 under deadline pressure — convergence outranks the budget', () => { + // The --chunk gate must rule on convergence BEFORE the budget: with a + // deadline close enough to refuse, a budget-first ordering would exit + // 4 and write a budget-stop marker over an audit that had already + // converged, capping the verdict with a false truncation disclosure. + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + (writeStdoutLine as unknown as Mock).mockClear(); + (writeStderrLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + chunk: 13, + findings, + round: 3, + }); + + expect(process.exitCode).toBe(5); // CONVERGED, not BUDGET + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + expect(msg).not.toContain('BUDGET:'); + expect(readBudgetStop(plan)).toBeNull(); + }); + + it('transcripts unavailable: the --chunk gate builds too, degrade before convergence', () => { + // The history says "converged" — but without the harness's records it + // is unreadable, and the --chunk gate must degrade exactly like the + // round builder: build the auditor, never refuse one, and never exit + // 5 on a history it cannot read. + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + delete process.env['QWEN_CODE_PROJECT_DIR']; + (writeStdoutLine as unknown as Mock).mockClear(); + (agentPromptCommand.handler as (a: unknown) => void)({ + plan, + role: 'reverse-audit', + chunk: 13, + findings, + round: 3, + }); + + expect(process.exitCode).toBeUndefined(); // built, not exit 5 CONVERGED + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(1); + expect( + keysOf(3).some((k) => k.startsWith('reverse-audit--chunk-13--')), + ).toBe(true); + }); + + it('a converged round outranks the budget gate — done is not truncated', () => { + // A converged audit owes no round, no disclosure and no cap. Refusing + // it on the budget would write a truncation entry for a run that + // stopped because it FINISHED — so the convergence check runs first, + // and the gate only ever sees a round that is still due. + answerRound(1, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const out = runRound(3); + 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(msg).not.toContain('BUDGET:'); + expect(readBudgetStop(plan)).toBeNull(); + }); + + it('the budget gate still refuses a round that is due: exit 4, not 5', () => { + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: YIELD, 15: YIELD }); + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 60); + const out = runRound(3); + expect(process.exitCode).toBe(4); + expect(out).toBe(''); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('BUDGET:'); + expect(msg).not.toContain('CONVERGED'); + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 3 by the review time budget', + ); + expect(keysOf(3)).toHaveLength(0); + }); + + it('a shortcut launch matching every record retires nothing', () => { + // Build rounds 1 and 2 with NO transcripts, then hand the history ONE + // agent launched with every recorded prompt concatenated — the + // one-agent-many-blocks shortcut `verificationGaps` refuses to certify + // the roster with, in the shape it actually takes: a single launch. + // `wasDeliveredVerbatim` allows additions, so the attack transcript + // matches all six records — per record it is a UNIQUE match, which is + // exactly why counting transcripts per record would credit every chunk + // one dry receipt and let a single agent retire the whole round. The + // guard counts records per transcript instead: matching several + // records, it certifies none. + answerRound(1, { 13: null, 14: null, 15: null }); + answerRound(2, { 13: null, 14: null, 15: null }); + const concatenated = [1, 2] + .flatMap((r) => [13, 14, 15].map((c) => recordOf(r, c))) + .join('\n\n'); + auditorTranscript(concatenated, DRY); + + const out = runRound(3); + expect(process.exitCode).toBeUndefined(); + expect(out).toContain('3 auditors required this round — one per chunk.'); + expect(out).not.toContain('retirement:'); + expect(keysOf(3)).toHaveLength(3); + + // A second identical launch retires nothing either — two ambiguous + // transcripts certify as little as one. + auditorTranscript(concatenated, DRY); + const again = runRound(3); + expect(process.exitCode).toBeUndefined(); + expect(again).toContain('3 auditors required this round — one per chunk.'); + expect(again).not.toContain('retirement:'); + }); + + it('staggered certificates re-align — mixed parities still converge', () => { + // 13 retires off rounds 1,2 (certificate parity even); 14 and 15 earn + // theirs a round later, off 2,3 (odd). Per-chunk parity anchors would + // cold-check the two groups on opposite rounds forever — the loop would + // converge in fact and still report the hard cap. One global parity + // pulls them back onto the same rounds. + answerRound(1, { 13: DRY, 14: YIELD, 15: YIELD }); + answerRound(2, { 13: DRY, 14: DRY, 15: DRY }); + answerRound(3, { 14: DRY, 15: DRY }); // 13 skipped + + // Round 4 cold-checks EVERY retired chunk despite the stagger. + const r4 = runRound(4); + expect(r4).toContain('— chunk 13 (cold check) ─'); + expect(r4).toContain('— chunk 14 (cold check) ─'); + expect(r4).toContain('— chunk 15 (cold check) ─'); + auditorTranscript(recordOf(4, 13), DRY); + auditorTranscript(recordOf(4, 14), DRY); + auditorTranscript(recordOf(4, 15), DRY); + + // All three retired, none due: the clean exit the stagger used to make + // unreachable. + const r5 = runRound(5); + expect(process.exitCode).toBe(5); + expect(r5).toBe(''); + const msg = (writeStderrLine as unknown as Mock).mock.calls + .map((c) => c[0]) + .join('\n'); + expect(msg).toContain('CONVERGED'); + }); +}); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index ef9a58a52ea..81780932119 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 { expectedRoundSeconds, + readRoundStamps, reverseAuditBudgetExhausted, reverseAuditBudgetMessage, stampRound, @@ -55,6 +56,11 @@ import { type DiffChunk, } from './lib/diff-plan.js'; import { recordPrompt, writeBrief } from './lib/prompt-record.js'; +import { + REVERSE_AUDIT_MAX_ROUNDS, + scheduleReverseAuditRound, + type RoundSchedule, +} from './lib/retirement.js'; import { BRIEFS, type RoleId } from './lib/agent-briefs.js'; import { pathRulesFor } from './lib/path-rules.js'; import { @@ -1387,10 +1393,11 @@ function runRoster(report: PlanReport, planPath: string, rules?: string): void { * round with one territory silently gone, and a duplicated id resolved both * blocks to the first matching chunk and keyed them to one record — the * second territory never audited, under an end marker that says the round - * is whole. Also called ahead of the budget gate: a plan no round could - * ever be built from must get this diagnosis whatever the clock says — - * refusing it as a budget stop would write a marker over a corrupt plan, - * say "proceed to Step 6", and preempt the one actionable repair. + * is whole. Called ahead of the retirement schedule and the budget gate: a + * plan no round could ever be built from must get this diagnosis whatever + * the clock says — refusing it as a budget stop would write a marker over + * a corrupt plan, say "proceed to Step 6", and preempt the one actionable + * repair. */ function requireAuditableChunks(report: PlanReport): DiffChunk[] { if (!Array.isArray(report.chunks) || report.chunks.length === 0) { @@ -1408,6 +1415,50 @@ function requireAuditableChunks(report: PlanReport): DiffChunk[] { return chunks; } +/** + * Gate one reverse-audit admission. Every part of the refusal — the marker + * `compose-review` synthesizes the verdict-capping disclosure from, the + * stderr termination rule, exit 4 — happens here, so the three admission + * paths (the chunkless single build, an --all-chunks round, a --chunk build + * of a round never admitted) cannot drift apart. Returns false when the + * round was refused: the caller builds nothing. The admission STAMP is not + * written here — it lands after the build succeeds, in each build path: the + * stamp is what the next round's gate measures cost from, and a build that + * throws must not leave one behind. + */ +function admitReverseAuditRound( + planPath: string, + round: number | undefined, +): boolean { + const spent = reverseAuditBudgetExhausted( + process.env, + expectedRoundSeconds(planPath, round), + ); + if (spent !== null) { + writeBudgetStop(planPath, spent, round); + writeStderrLine(reverseAuditBudgetMessage(spent, round)); + process.exitCode = 4; + return false; + } + return true; +} + +/** + * The loop's OTHER termination rule: every territory has proven itself cold + * 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. + */ +function refuseConverged(): void { + 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.', + ); + process.exitCode = 5; +} + function runAllChunks( report: PlanReport, planPath: string, @@ -1417,9 +1468,65 @@ function runAllChunks( round?: number, ): void { const chunks = requireAuditableChunks(report); + + // Which chunks this round actually owes an auditor. Rounds 1 and 2 always + // fan out to every chunk — they establish each chunk's record — and from + // round 3 the schedule reads the audit history (the CLI's own prompt + // records against the harness's transcripts, the same pair every delivery + // check trusts) and retires the territories that have twice in a row + // returned a substantive all-clear. Measured on a real 3B run (6 chunks × + // 5 rounds, ~95 minutes), two chunks were dry in all five rounds while + // three yielded in most: the loop earns its keep in the hot territories, + // and the cold ones were a third of its bill. + let schedule: RoundSchedule | null = null; + if (role === 'reverse-audit' && round !== undefined && round >= 3) { + try { + schedule = scheduleReverseAuditRound( + planPath, + chunks.map((c) => c.id), + round, + process.env, + typeof report.diffPathAbsolute === 'string' + ? report.diffPathAbsolute + : undefined, + ); + } catch { + // Transcripts unavailable, an unreadable plan stat, anything: the + // schedule is an optimization, and a broken optimizer must degrade to + // today's behaviour — every territory audited — never to fewer + // auditors. `null` below means "everything is due". + schedule = null; + } + } + + if (schedule !== null && schedule.converged) { + refuseConverged(); + return; + } + + // The budget gate, deferred here from the single-build path for + // --all-chunks rounds so the convergence check above runs FIRST: a + // converged audit is done — it owes no round, and refusing it would cap a + // clean verdict with a truncation it never earned. A round still due is + // refused exactly as the single-build path refuses one — nothing built, + // nothing recorded, exit 4 with the marker. The admission stamp lands + // AFTER the build below succeeds, never here: a cold-check-only round + // that builds is an admission and stamps like any other; the converged + // round above built nothing and stamps nothing; a build that throws + // leaves no stamp for the next round's gate to misprice. + if (role === 'reverse-audit' && !admitReverseAuditRound(planPath, round)) { + return; + } + + const dueSet = schedule === null ? null : new Set(schedule.due); + const dueChunks = + dueSet === null ? chunks : chunks.filter((c) => dueSet.has(c.id)); + const coldSet = new Set(schedule?.coldChecks ?? []); + const skipped = schedule?.skipped ?? []; + const digest = findingsDigest(findingsContent, rules); const roundPart = round !== undefined ? `--round-${round}` : ''; - const blocks = chunks.map((c, i) => { + const blocks = dueChunks.map((c, i) => { const key = `${role}--chunk-${c.id}${roundPart}--${digest}`; const { prompt } = buildLaunch( report, @@ -1429,27 +1536,69 @@ function runAllChunks( ); const printed = foldFindings(role, findingsContent, prompt); recordPrompt(planPath, key, printed); + // The cold-check tag lives in the SEPARATOR label, never in the prompt: + // separators are display, and the delivery check compares prompts. + const cold = coldSet.has(c.id) ? ' (cold check)' : ''; return ( - `───── auditor ${i + 1} of ${chunks.length} — chunk ${c.id} ─────\n\n` + + `───── auditor ${i + 1} of ${dueChunks.length} — chunk ${c.id}${cold} ─────\n\n` + printed ); }); + // The scope clause names the retirement when there is one, so the reader + // learns the round shrank from the header and not from a diff of block + // counts; when nothing is retired the sentence is byte-identical to what + // it always said. + const scope = + skipped.length === 0 + ? 'one per chunk' + : `one per chunk still under audit (${skipped.length} retired ` + + `chunk(s) skipped; the retirement note after the end-of-round line ` + + `says which — relay it to the terminal)`; + const retirementNote = + skipped.length === 0 + ? [] + : [ + `retirement: a chunk whose two most recent audits are substantive ` + + `dry receipts is cold-checked on alternating rounds instead of ` + + `audited on every one; a cold check that yields returns it to ` + + `every-round auditing. Skipped this round:\n` + + skipped + .map( + (s) => + `chunk ${s.chunkId} — retired: dry in rounds ` + + `${s.dryRounds[0]} and ${s.dryRounds[1]}, ` + + (s.nextColdCheck > REVERSE_AUDIT_MAX_ROUNDS + ? `certificate final — the ` + + `${REVERSE_AUDIT_MAX_ROUNDS}-round hard cap leaves ` + + `the loop no round for a cold check` + : `next cold check round ${s.nextColdCheck}`), + ) + .join('\n'), + ]; writeStdoutLine( [ - `${chunks.length} auditors required this round — one per chunk. Launch ` + + `${dueChunks.length} auditors required this round — ${scope}. Launch ` + `one agent per block below, passing its block VERBATIM — copy, do not ` + `retype, and NEVER sample this output (no \`| head\`): the text IS the ` + `deliverable, and a launch reconstructed from a sample matches no ` + - `record. Blocks are numbered \`auditor k of ${chunks.length}\` and the ` + - `output ends with an end-of-round line — if either is missing, the ` + - `output was truncated in transit; rebuild just the missing chunks with ` + - `--chunk . Write each Agent call's \`description\` (the task ` + + `record. Blocks are numbered \`auditor k of ${dueChunks.length}\`, and ` + + `the output ends with an end-of-round line — followed by the ` + + `retirement note, when there is one. If either the numbering or the ` + + `end-of-round line is missing, the output was truncated in transit; ` + + `rebuild just the missing chunks with --chunk . Write each ` + + `Agent call's \`description\` (the task ` + `name the user watches) in your output language, translating the ` + `separator label — display only; the prompt stays the block VERBATIM.`, ...blocks, - `───── end of round — ${chunks.length} auditors ─────`, + `───── end of round — ${dueChunks.length} auditors ─────`, + ...retirementNote, ].join('\n\n'), ); + // Admitted AND built: stamp now, so the next round's gate can measure + // this one — see the gate comment above for why never at admission. + if (role === 'reverse-audit') { + stampRound(planPath, round); + } } function runAgentPrompt(args: AgentPromptArgs): void { @@ -1735,16 +1884,6 @@ function runAgentPrompt(args: AgentPromptArgs): void { } } - // The all-chunks dispatch below builds from `chunks[]`; validate that - // structure BEFORE the budget gate, so a plan no round could ever be - // built from gets its structural error whatever the clock says — refused - // as a budget stop it would write a marker over a corrupt plan and tell - // the orchestrator to proceed to Step 6 when the actionable repair is a - // Step 1 re-capture. - if (args.allChunks && args.role && findingsContent !== undefined) { - requireAuditableChunks(report); - } - // The budget gate — after every validation and every file read, because a // malformed call or a broken plan deserves its own error (refusing here // would record a budget stop against a plan that cannot even parse) — @@ -1761,17 +1900,70 @@ function runAgentPrompt(args: AgentPromptArgs): void { // and a build that throws must not leave one behind — priced from a // failed build, the next round would be floored to the 600s minimum, // widening admission in exactly the unsafe direction. - if (args.role === 'reverse-audit') { - const spent = reverseAuditBudgetExhausted( - process.env, - expectedRoundSeconds(args.plan, args.round), - ); - if (spent !== null) { - writeBudgetStop(args.plan, spent, args.round); - writeStderrLine(reverseAuditBudgetMessage(spent, args.round)); - process.exitCode = 4; - return; + // This is the chunkless single build's gate (the 3A round). An + // --all-chunks round gates inside the round builder instead, AFTER its + // convergence check — a converged audit is done and owes no round, and + // refusing it here would cap a clean verdict with a truncation it never + // earned. A --chunk build is ruled on below, keyed on the round's stamp. + if ( + args.role === 'reverse-audit' && + !hasChunk && + !args.allChunks && + !admitReverseAuditRound(args.plan, args.round) + ) { + return; + } + + // The reverse-audit gate for a --chunk build, placed after the plan read + // because its convergence half reads the plan's chunk list. A round + // holding an admission stamp is being REPAIRED — a truncated delivery, + // rebuilt per chunk — and bypasses everything: its cost and its schedule + // were ruled on when the round was admitted, and refusing the repair + // leaves the truncation unrepairable (the auditor never launched, + // nothing writing the unreviewedDimensions entry for it) under a + // disclosure that names the wrong round. A round with NO stamp reached + // with --chunk is not a repair; it is the round being built one auditor + // at a time — measured: an expired deadline refused `--all-chunks + // --round 4` and wrote the "stopped before round 4" marker, then N + // per-chunk builds of round 4 each exited 0, and the round ran past the + // deadline while the disclosure said it never started. So an unadmitted + // round answers to the same sequence --all-chunks answers to: convergence + // first (a done audit is not truncated), then the budget — the first + // chunk build IS the round's admission (its stamp lands after the build + // below), and the ones after it are repairs of it. A chunk merely + // retired inside a live round is still buildable: refusing it could only + // spare an audit, and sparing audits is never this file's failure + // direction. + if ( + args.role === 'reverse-audit' && + hasChunk && + !readRoundStamps(args.plan).some((s) => s.round === (args.round ?? null)) + ) { + if (args.round !== undefined) { + let schedule: RoundSchedule | null = null; + try { + schedule = scheduleReverseAuditRound( + args.plan, + (Array.isArray(report.chunks) ? (report.chunks as DiffChunk[]) : []) + .map((c) => c?.id) + .filter((id): id is number => typeof id === 'number'), + args.round, + process.env, + typeof report.diffPathAbsolute === 'string' + ? report.diffPathAbsolute + : undefined, + ); + } catch { + // Same degradation as the round builder: an unreadable history must + // fall back to building the auditor, never to refusing it. + schedule = null; + } + if (schedule !== null && schedule.converged) { + refuseConverged(); + return; + } } + if (!admitReverseAuditRound(args.plan, args.round)) return; } if (args.allChunks && args.role && findingsContent !== undefined) { @@ -1783,11 +1975,6 @@ function runAgentPrompt(args: AgentPromptArgs): void { rules, args.round, ); - // Admitted AND built: stamp now, so the next round's gate can measure - // this one — see the gate comment above for why not earlier. - if (args.role === 'reverse-audit') { - stampRound(args.plan, args.round); - } return; } @@ -1845,9 +2032,11 @@ function runAgentPrompt(args: AgentPromptArgs): void { : prompt; recordPrompt(args.plan, key, printed); writeStdoutLine(printed); - // Admitted AND built — the single-build twin of the all-chunks stamp - // above (a `--chunk ` rebuild lands here; the one-per-round guard in - // `stampRound` keeps it from shrinking its own round's observed cost). + // Admitted AND built — the single-build twin of the all-chunks stamp in + // `runAllChunks`. A `--chunk ` build lands here too: the first chunk + // build of an unadmitted round writes its admission stamp, and the + // rebuilds after it are repairs the one-per-round guard in `stampRound` + // keeps from shrinking the round's observed cost. if (args.role === 'reverse-audit') { stampRound(args.plan, args.round); } @@ -1860,7 +2049,10 @@ export const agentPromptCommand: CommandModule = { "ranges and the agent's own brief are welded in, not left to the caller to " + 'remember). Exit codes: 0 built; 4 the review time budget refused another ' + 'reverse-audit round (a termination rule, not an error — see the BUDGET line ' + - 'on stderr); anything else is a bad call or a broken plan.', + 'on stderr); 5 the reverse audit CONVERGED — every chunk holds two ' + + 'consecutive substantive dry audits and none is due a cold check, so stop ' + + 'the loop and proceed to Step 6 (also a termination rule, and a clean one: ' + + 'no disclosure is owed); anything else is a bad call or a broken plan.', builder: (yargs) => yargs .option('plan', { @@ -1879,7 +2071,12 @@ export const agentPromptCommand: CommandModule = { }) .option('chunk', { type: 'number', - describe: 'Which chunk id this agent owns (a Step 3B territory agent)', + describe: + 'Which chunk id this agent owns (a Step 3B territory agent). With ' + + '--role reverse-audit --round N it rebuilds ONE auditor of that ' + + 'round: a round already admitted is repaired without gates; a ' + + 'round never admitted answers to the same convergence and budget ' + + 'gates as --all-chunks.', }) .option('file', { type: 'string', @@ -1892,7 +2089,12 @@ export const agentPromptCommand: CommandModule = { describe: 'With --role reverse-audit --findings: build one block per chunk ' + 'in one call, labelled and separated (Step 5, 3B). Never sample ' + - 'the output; each block is pasted verbatim to its own agent.', + 'the output; each block is pasted verbatim to its own agent. From ' + + '--round 3 on, a chunk whose two most recent audits were both ' + + 'substantive dry receipts is retired to alternating-round cold ' + + 'checks (the retirement note after the end-of-round line says ' + + 'which); a round where every chunk is retired and none is due ' + + 'exits 5 — the audit has converged.', }) .option('roster', { type: 'boolean', diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 0c5a2bc8e12..249994f84cd 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -3501,3 +3501,115 @@ describe('the ledger marker reaches the POSTED body', () => { expect(r.body).not.toContain('qwen-review-ledger'); }); }); + +describe('composeReview — the findings file tag check', () => { + // The pipelined loop's invariant, machine-read. Under the serial loop the + // last round's verification completing before Step 6 was structural; the + // pipelined loop replaced the structure with a tag the orchestrator adds, + // removes, and reads by hand. The delivery floor cannot see the miss — one + // delivered verify launch anywhere in the run satisfies it, keyed per + // round's findings digest — so compose-review reads the cumulative + // findings file itself and caps on any surviving tag. + + function findingsFile(content: string): string { + const f = join(dir, 'qwen-review-findings.md'); + writeFileSync(f, content); + return f; + } + + const TAGGED = + '- **File:** src/pay.ts:42\n' + + '- **Issue:** off-by-one in the retry cap\n' + + '- **Severity:** Critical — [unverified]\n'; + const CLEAN = + '- **File:** src/pay.ts:42\n' + + '- **Issue:** off-by-one in the retry cap\n' + + '- **Severity:** Critical\n'; + + it('caps a clean Approve at Comment and discloses the surviving tag', () => { + const r = composeReview(base({ findingsPath: findingsFile(TAGGED) })); + expect(r.baseEvent).toBe('APPROVE'); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('findings-unverified-at-compose'); + expect(r.body).toContain( + '1 finding(s) still carried the `— [unverified]` tag when the loop ' + + 'ended', + ); + // The opener may not certify over a loop that ended mid-verification. + expect(r.body).not.toContain('no blockers'); + expect(r.remediation.join(' ')).toContain('--role verify'); + expect(verdictLine(r)).toBe( + 'Verdict: Comment — an Approve was NOT available: findings were ' + + 'still unverified when the loop ended', + ); + }); + + it('counts every surviving tag', () => { + const two = `${TAGGED}\n- **File:** src/other.ts:7 — race in the retry queue — [unverified]\n`; + const r = composeReview(base({ findingsPath: findingsFile(two) })); + expect(r.event).toBe('COMMENT'); + expect(r.body).toContain('2 finding(s) still carried the'); + }); + + it('a tag-free findings file caps nothing', () => { + const r = composeReview(base({ findingsPath: findingsFile(CLEAN) })); + expect(r.event).toBe('APPROVE'); + expect(r.cappedBy).not.toContain('findings-unverified-at-compose'); + }); + + it('a missing findingsPath disables the check — every non-high run', () => { + const r = composeReview(base({})); + expect(r.event).toBe('APPROVE'); + expect(r.cappedBy).not.toContain('findings-unverified-at-compose'); + }); + + it('softens a Request changes whose blockers are non-deterministic', () => { + // The verifier's delivery is clean here (coveredPlan records it), so the + // softening is the tag flag alone: a review posting non-deterministic + // Criticals cannot prove they are not the still-tagged entries. + const r = composeReview( + base({ criticalsInline: 1, findingsPath: findingsFile(TAGGED) }), + ); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('findings-unverified-at-compose'); + expect(r.cappedBy).not.toContain('criticals-unverified'); + expect(verdictLine(r)).toBe( + 'Verdict: Comment — a Request changes was NOT available: findings ' + + 'were still unverified when the loop ended (they are posted, ' + + 'disclosed)', + ); + }); + + it('a deterministic-only Request changes stands despite the tag', () => { + // A [build] blocker is pre-confirmed; nothing posted owed a verifier, so + // a tag on an entry the review did not confirm un-blocks nothing — but + // the disclosure still rides the body. + const r = composeReview( + base({ + bodyCriticals: ['[build] tsc fails on the merge commit'], + findingsPath: findingsFile(TAGGED), + }), + ); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).toContain('findings-unverified-at-compose'); + expect(r.body).toContain('still carried the `— [unverified]` tag'); + }); + + it('fails CLOSED on a findingsPath that does not read', () => { + const r = composeReview( + base({ findingsPath: join(dir, 'no-such-findings.md') }), + ); + expect(r.baseEvent).toBe('APPROVE'); + expect(r.event).toBe('COMMENT'); + expect(r.cappedBy).toContain('findings-unverified-at-compose'); + expect(r.body).toContain('findings file could not be read at compose time'); + expect(r.remediation.join(' ')).toContain('findingsPath'); + }); + + it('refuses a present findingsPath of the wrong shape', () => { + expect(() => + composeReview(base({ findingsPath: 42 as unknown as string })), + ).toThrow(/findingsPath must be a non-empty string/); + }); +}); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index e33a6fcd4be..2379a04d29b 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -129,6 +129,18 @@ export interface ComposeReviewInput { * whether the diff was read. */ planPath?: string; + /** + * The cumulative reverse-audit findings file at loop end — the same file + * every round's `agent-prompt --findings` received, after the final merge. + * compose-review reads it itself for the one fact Step 6's confirmed-only + * read is otherwise a model's word on: whether any entry still carries the + * `— [unverified]` tag. A surviving tag means no verifier ever ruled on + * that entry, and the verdict is capped whether or not the report excluded + * it. A path that does not read fails closed — "could not show" and "was + * not" read the same to the person the verdict posts at. Omitted, the + * check is off: every non-high review, which runs no Step 5. + */ + findingsPath?: string; /** * Where to look for the harness's records. Defaults to the environment the CLI * exported. A test seam only — production never passes it, and a model cannot: @@ -207,6 +219,15 @@ export interface ComposeReviewResult { lowSignal: { agents: number; srcDiffLines: number } | null; } +/** + * The Step 5 tag, exactly as the loop's merge writes and removes it: an + * entry not yet through verification carries it, a confirmed verdict removes + * it, and a tag that survives to compose time is an entry no verifier ever + * ruled on. Whitespace-tolerant only — the tag is prose the orchestrator + * copies, and a re-wrap must not hide it. + */ +const UNVERIFIED_FINDING_TAG_RE = /—\s*\[unverified\]/gi; + function withMarker(line: string): string { return line.startsWith(CRITICAL_PREFIX) ? line : `${CRITICAL_PREFIX} ${line}`; } @@ -679,6 +700,56 @@ function composeReviewBody( criticalsUnverified = criticalsNeedingVerify >= 1; } } + + // The pipelined loop's invariant, machine-checked. "The last round's + // verification completes before Step 6" used to be STRUCTURAL — the serial + // loop could not build round k+1 before round k's verdicts merged — and + // pipelining replaced the structure with a tag the orchestrator adds, + // removes, and reads by hand. The delivery floor above cannot see the miss: + // it asks for ONE clean verify delivery across the whole key family, and + // each round's verifier is keyed by that round's findings digest, so round + // 1's launch clears the floor while round 5's findings go out unverified. + // So the findings file itself is read here: a surviving tag is an entry no + // verifier ruled on, and it caps the verdict whether or not Step 6's read + // excluded it. The path is a caller-written input like `planPath`; the + // check fails CLOSED when it does not read, and fails OPEN when it is + // omitted — a medium review runs no Step 5 and has no findings file. + let findingsUnverifiedAtCompose = false; + let findingsFileUnreadable = false; + let unverifiedTagCount = 0; + const findingsPath: unknown = input.findingsPath; + if (findingsPath !== undefined && findingsPath !== null) { + if (typeof findingsPath !== 'string' || findingsPath.trim() === '') { + throw new TypeError( + `compose-review: findingsPath must be a non-empty string, got ${JSON.stringify(findingsPath)}`, + ); + } + try { + const findingsContent = readFileSync(findingsPath, 'utf8'); + unverifiedTagCount = ( + findingsContent.match(UNVERIFIED_FINDING_TAG_RE) ?? [] + ).length; + findingsUnverifiedAtCompose = unverifiedTagCount > 0; + if (findingsUnverifiedAtCompose) { + remediation.push( + 'findings still tagged `— [unverified]`: relaunch the verifier ' + + 'for each tagged entry (Step 4, `--role verify` with that ' + + 'entry), apply its verdict in the cumulative findings file, ' + + 'and run compose-review again with the updated file', + ); + } + } catch { + findingsFileUnreadable = true; + findingsUnverifiedAtCompose = true; + remediation.push( + 'findings file not readable: pass the cumulative reverse-audit ' + + "findings file — the one every round's `--findings` received — " + + 'as `findingsPath` in the state JSON, and run compose-review ' + + 'again', + ); + } + } + const contextUnavailable = toBool( input.contextUnavailable, 'contextUnavailable', @@ -732,32 +803,41 @@ function composeReviewBody( } if (contextUnavailable) cappedBy.push('context-unavailable'); if (criticalsUnverified) cappedBy.push('criticals-unverified'); + if (findingsUnverifiedAtCompose) { + cappedBy.push('findings-unverified-at-compose'); + } let event: ReviewEvent = baseEvent; if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT'; - // The ONE cap that reaches a Request changes — because it removes the - // premise the never-soften rule stands on. "A REQUEST_CHANGES earned by a - // confirmed Critical is never softened" presumes CONFIRMED, and this flag - // is precisely the statement that no verifier ever ruled on the blockers. - // The header's own principle — an unverified finding must not become a - // public blocker (the false "leaks tokens" Critical is the exact harm) — - // was mechanics for the Approve row only, and a real bot review shipped - // through the gap: a CHANGES_REQUESTED on an external contributor's PR - // (#7166) whose one Critical the body itself disclosed as unverified. - // The findings still post, disclosed; the review just may not BLOCK on a - // claim nobody confirmed. Manipulation check: a run that wants an Approve - // gains nothing here (the same flag caps Approve via `unreviewed`), and a - // run that wants to block without verifying now cannot. + // The caps that reach a Request changes — because they remove the premise + // the never-soften rule stands on. "A REQUEST_CHANGES earned by a + // confirmed Critical is never softened" presumes CONFIRMED, and these + // flags are precisely the statement that the confirmation is missing: + // `criticalsUnverified` says no verifier ever ruled (the delivery floor), + // `findingsUnverifiedAtCompose` says the findings file itself still + // carries `— [unverified]` tags at compose time. The header's own + // principle — an unverified finding must not become a public blocker (the + // false "leaks tokens" Critical is the exact harm) — was mechanics for + // the Approve row only, and a real bot review shipped through the gap: a + // CHANGES_REQUESTED on an external contributor's PR (#7166) whose one + // Critical the body itself disclosed as unverified. The findings still + // post, disclosed; the review just may not BLOCK on a claim nobody + // confirmed. Manipulation check: a run that wants an Approve gains + // nothing here (the same flags cap Approve), and a run that wants to + // block without verifying now cannot. // …unless a DETERMINISTIC Critical also rides the review: a `[build]`/ // `[test]` finding is pre-confirmed, its Request changes is earned with or // without a verifier, and softening it alongside its unverified sibling // would un-block a confirmed build failure. The unverified ones stay - // disclosed either way. + // disclosed either way. The tag flag also needs a non-deterministic + // Critical in the payload before it softens: when nothing posted owed a + // verifier, a tag on an entry the report did not confirm blocks nothing. const deterministicBodyCriticals = bodyCriticals.length - nonDeterministicBodyCriticals; if ( event === 'REQUEST_CHANGES' && - criticalsUnverified && + (criticalsUnverified || + (findingsUnverifiedAtCompose && criticalsNeedingVerify >= 1)) && deterministicBodyCriticals === 0 ) { event = 'COMMENT'; @@ -1071,6 +1151,26 @@ function composeReviewBody( ] : []; + // The findings file's own evidence that the loop ended with verification + // outstanding — rendered on every event the cap binds, RC included (a + // deterministic blocker beside a tagged entry keeps its Request changes + // but not the silence about the tag). + const unverifiedTagsBlock: Bi[] = !findingsUnverifiedAtCompose + ? [] + : findingsFileUnreadable + ? [ + { + en: '⚠️ The reverse-audit findings file could not be read at compose time, so this run cannot show its findings were verified.', + zh: '⚠️ 组合评审时无法读取反向审计发现文件,本次运行无法证明其发现已经过验证。', + }, + ] + : [ + { + en: `⚠️ ${unverifiedTagCount} finding(s) still carried the \`— [unverified]\` tag when the loop ended — the verifier never ruled on them, and they are not confirmed.`, + zh: `⚠️ 循环结束时仍有 ${unverifiedTagCount} 条发现带着 \`— [unverified]\` 标记——验证者从未对它们作出裁决,它们不算已确认。`, + }, + ]; + if (event === 'REQUEST_CHANGES') { // Empty body, except the disclosures: every clause whose state holds // appears on every event — a confirmed blocker must not squeeze out the @@ -1080,6 +1180,7 @@ function composeReviewBody( ...(contextUnavailable ? [contextUnavailableClause] : []), ...cannotTellBlock, ...notReviewedParts, + ...unverifiedTagsBlock, ...deferredBlock, ...testPlanBlock, ...bodyCriticalBlock, @@ -1151,8 +1252,10 @@ function composeReviewBody( unreviewed.length + coverageEntries.length === 0 && // A missing receipt caps the event but was left out of certification, so a // body could open "Reviewed — no blockers." two lines above "nobody read - // them." Nothing nobody read can be certified blocker-free. - missingReceipts.length === 0; + // them." Nothing nobody read can be certified blocker-free — and neither + // can a loop that ended with findings no verifier ever ruled on. + missingReceipts.length === 0 && + !findingsUnverifiedAtCompose; // The opener may not say "Reviewed." over a disclosure set that denies it. // #7268's posted body opened exactly that way — "Reviewed. Suggestions are // inline." above two sentences disclosing all 49 chunks — and the author's @@ -1214,6 +1317,10 @@ function composeReviewBody( // 6. Not-reviewed disclosure. clauses.push(...notReviewedParts); + // 6a. Verification outstanding at loop end — the findings file's surviving + // `— [unverified]` tags, machine-read. + clauses.push(...unverifiedTagsBlock); + // 6b. Deferred-checker disclosure (non-capping) — a workflow whose embedded // shell actionlint would lint but we do not yet trust. clauses.push(...deferredBlock); @@ -1913,6 +2020,8 @@ export function verdictLine(r: ComposeReviewResult): string { 'uncoverable-chunk': 'part of the diff cannot be read at all', 'unreviewed-dimension': 'a dimension nobody reviewed', 'context-unavailable': "the PR's existing discussion could not be read", + 'findings-unverified-at-compose': + 'findings were still unverified when the loop ended', }; let line = `Verdict: ${label[r.event]}`; // Why an Approve was not available — but only when one would otherwise have been. @@ -1926,19 +2035,24 @@ export function verdictLine(r: ComposeReviewResult): string { // A coverage cap never softens a Request changes — a confirmed blocker earned // that, and naming a constraint that did not bind would send the reader // looking for an effect that is not there — so the Approve clause is gated on - // the base having been an Approve at all. The unverified-blockers cap is the - // one exception, because it says the confirmation never happened, and its - // sentence must name what the reader would otherwise chase: a Comment posted - // over visible **[Critical]** comments reads as a contradiction until the - // line says why. + // the base having been an Approve at all. The unverified family is the + // exception — the delivery floor and the findings file's surviving tags both + // say the confirmation never happened — and the sentence must name what the + // reader would otherwise chase: a Comment posted over visible **[Critical]** + // comments reads as a contradiction until the line says why. if ( r.baseEvent === 'REQUEST_CHANGES' && r.event === 'COMMENT' && - r.cappedBy.includes('criticals-unverified') + (r.cappedBy.includes('criticals-unverified') || + r.cappedBy.includes('findings-unverified-at-compose')) ) { line += - ' — a Request changes was NOT available: its blockers were never ' + - 'verified (they are posted, disclosed as unverified)'; + ' — a Request changes was NOT available: ' + + (r.cappedBy.includes('criticals-unverified') + ? 'its blockers were never verified (they are posted, disclosed as ' + + 'unverified)' + : 'findings were still unverified when the loop ended (they are ' + + 'posted, disclosed)'); } else if (r.baseEvent === 'APPROVE' && r.event !== 'APPROVE') { const reasons = r.cappedBy.map((c) => why[c] ?? c); if (r.downgraded) reasons.push('a presubmit check failed'); diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 6b7b7b60c89..e525190bb8d 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -134,6 +134,15 @@ export interface Brief { brief: string; } +/** + * The model receipt the reverse-audit brief hands every auditor as its + * example. Exported so the retirement classifier can refuse a clause that + * parrots it — measured: agents repeat what they are handed, and a receipt + * the prompt wrote is not evidence of a walk. + */ +export const REVERSE_AUDIT_EXAMPLE_RECEIPT = + "No issues found — re-walked the reconnect state machine and the two changed exports' call sites; every gap I checked was already in the list"; + export const BRIEFS: Record = { '0': { label: 'Agent 0: Issue fidelity & root-cause ownership', @@ -644,7 +653,7 @@ The asymmetry cuts both ways: confirming also requires the trace, and a finding - **Report only Critical or Suggestion.** Do not report Nice to have. - A found gap uses the standard finding format (with \`Source: [review]\`), including its failure scenario — your findings go through the same verification as any other, so they must carry the evidence a verifier can trace. -If you find no new gap in your scope, say so **and name what you re-examined** — \`No issues found — re-walked the reconnect state machine and the two changed exports' call sites; every gap I checked was already in the list\`. A bare "No issues found." is indistinguishable from an agent that did nothing, and it is treated as one: it ends nothing, and it earns your scope a relaunch.`, +If you find no new gap in your scope, say so **and name what you re-examined** — \`${REVERSE_AUDIT_EXAMPLE_RECEIPT}\`. A bare "No issues found." is indistinguishable from an agent that did nothing, and it is treated as one: it ends nothing, and it earns your scope a relaunch.`, }, }; diff --git a/packages/cli/src/commands/review/lib/deadline.test.ts b/packages/cli/src/commands/review/lib/deadline.test.ts index d8acc12bff3..9ac06f6ffb1 100644 --- a/packages/cli/src/commands/review/lib/deadline.test.ts +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -112,7 +112,7 @@ describe('reverseAuditBudgetExhausted — the round must fit, and its tail', () it('honours the reserve-0 escape hatch — only the round itself must fit', () => { // `r >= 0` (not `> 0`) is the documented escape hatch: reserve 0 keeps // only the refusal of a round that cannot finish before the deadline. - // An edit to `> 0` would silently fall back to the 3600s default and + // An edit to `> 0` would silently fall back to the 4800s default and // refuse the next round a full hour before the operator's deadline. const env = { [DEADLINE_ENV]: String(NOW_S + DEFAULT_ROUND_SECONDS + 60), @@ -190,13 +190,26 @@ describe('the round-cost estimate — measured when it can be', () => { expect(expectedRoundSeconds(p, 2, NOW_MS)).toBe(2400); }); - it('measures the NEWEST previous round when several are on file', () => { + it('prices from the COSTLIEST measured round, not the newest', () => { const p = plan(); stampRound(p, 1, NOW_MS - 3_000_000); // round 1 admitted 50 min ago stampRound(p, 2, NOW_MS - 1_200_000); // round 2 admitted 20 min ago - // Scanning oldest-first would report 3000; the loop's cost trend is the - // newest admission's. - expect(expectedRoundSeconds(p, 3, NOW_MS)).toBe(1200); + // Round 1's span (admission to admission) is 30 minutes; round 2's + // still-open span is 20. The reserve is the terminal round's only + // cover, so the gate holds the worst case the run has proved — a + // repair relaunch makes one round the expensive one, and a newest-only + // estimate nets it away the round after it lands. + expect(expectedRoundSeconds(p, 3, NOW_MS)).toBe(1800); + }); + + it('the costliest span can be a MIDDLE round', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 4_000_000); + stampRound(p, 2, NOW_MS - 2_000_000); // round 1's span: 33 min + stampRound(p, 3, NOW_MS - 1_500_000); // round 2's span: 8 min + // Round 3's open span is 25 min; the max is round 1's 33-minute span — + // neither the first nor the newest span measured from `now` alone. + expect(expectedRoundSeconds(p, 4, NOW_MS)).toBe(2000); }); it('ignores a stamp of the SAME round — a rebuild is not a round', () => { @@ -377,7 +390,7 @@ describe('the CI wiring contract', () => { expect(workflow).toContain( `${DEADLINE_ENV}="$(( $(date +%s) + attempt_timeout ))"`, ); - expect(workflow).toContain(`${RESERVE_ENV}="$(( attempt_timeout / 4 ))"`); + expect(workflow).toContain(`${RESERVE_ENV}="$(( attempt_timeout / 3 ))"`); // The workflow's reserve cap documents itself as mirroring // DEFAULT_RESERVE_SECONDS ("keep the two in sync") — enforce the mirror, // so a one-sided bump diverges a test instead of the CI tail. diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index ffc6d590e3d..76749737306 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -53,26 +53,42 @@ export const RESERVE_ENV = 'QWEN_REVIEW_DEADLINE_RESERVE_SECONDS'; * verification of that round's findings, compose-review, anchor resolution * and the submission itself. * - * The measured round estimate ALSO contains one verification pass — a round - * is admitted only after the previous round's findings were verified and - * merged (SKILL.md Step 5), so an admission-to-admission span includes the - * verification between them — which means the gate holds back roughly one - * verification more than the terminal round strictly needs. That overlap is - * deliberate margin, not double-entry bookkeeping that slipped: round costs - * trend UP (each round re-reads the diff against a longer findings list, and - * repair relaunches land mid-loop), so the previous round's measurement - * under-predicts the next in exactly the runs that end near the boundary — - * and the two error directions are not symmetric. Over-reserving ends the - * loop at most one round early, disclosed as a budget stop; under-reserving - * is #8368 — killed mid-verification, holding every confirmed finding. + * Under the pipelined loop (SKILL.md Step 5), a round's verification + * launches WITH the next round's auditors instead of sitting between + * admissions — so the admission-to-admission span the gate measures + * contains no verification pass, and the terminal round's verification has + * exactly one cover: this reserve. That makes the reserve's sizing the + * whole margin, not a top-up on an overlap the measurement already + * carried — so the estimate refuses to be optimistic too: it prices the + * round from the COSTLIEST span the run has measured (see + * `expectedRoundSeconds`), because round costs do not climb smoothly — + * each round re-reads the diff against a longer findings list, and a + * repair relaunch lands mid-loop and makes one round the expensive one — + * and the newest span alone under-predicts the next in exactly the runs + * that end near the boundary. Over-reserving ends the loop at most one + * round early, disclosed as a budget stop; under-reserving is #8368 — + * killed mid-verification, holding every confirmed finding. + * + * Sized from the only tail measurement the record holds (#8368, +1699 + * lines): the loop ended with half an hour left and the outer kill found + * round 5's verification STILL RUNNING — the tail had consumed more than 30 + * minutes and was nowhere through (compose, anchor resolution and + * submission never started). No upper bound was ever measured, so the size + * is insurance, not arithmetic: pipelining made this reserve the terminal + * round's ONLY cover, and until pipelined runs measure their tails, the + * reserve buys the unknown, not the known. Over-reserving ends the loop at + * most one round early, disclosed as a budget stop; under-reserving is + * #8368. * * This is only the fallback: the budget itself is * chosen outside the CLI (a repository variable, a workflow input, a * `/review --timeout=N` comment), so the review workflow passes a reserve * scaled to the budget it resolved rather than trusting this constant to fit - * an arbitrary one. A local run has no deadline and no reserve at all. + * an arbitrary one. The workflow caps that scaled reserve at this same + * number (`.github/workflows/qwen-code-pr-review.yml`) — keep the two in + * sync. A local run has no deadline and no reserve at all. */ -export const DEFAULT_RESERVE_SECONDS = 3600; +export const DEFAULT_RESERVE_SECONDS = 4800; /** * The admission estimate for a round nothing has measured yet — round 1, or @@ -176,27 +192,37 @@ export function stampRound( /** * What the round about to be admitted is expected to cost, in seconds: the - * observed cost of the previous round (admission-to-admission — its agents, - * their verification, the orchestration between) when a stamp exists, else - * the conservative constant. The span deliberately overlaps the tail - * reserve by one verification pass — see `DEFAULT_RESERVE_SECONDS` for why - * that margin is kept rather than netted out. A stamp of the SAME round is - * ignored — that is a rebuild, and measuring it would report a round as - * cheap because its prompts were built twice quickly. + * COSTLIEST round the run has measured (admission-to-admission — its audit + * fan-out and the orchestration around it; under the pipelined loop a + * round's verification overlaps the NEXT round instead of sitting between + * admissions, so it is not in this measure, and the terminal round's + * verification is exactly what the deadline's reserve covers) when a stamp + * exists, else the conservative constant. The costliest, not the newest: + * the reserve is the terminal round's only cover, and the run's own worst + * span is the evidence of what a round can cost — a newest-only estimate + * nets a mid-loop repair relaunch away the round after it lands, in + * exactly the runs that end near the boundary. A stamp of the SAME round + * is ignored — that is a rebuild, and measuring it would report a round + * as cheap because its prompts were built twice quickly. */ export function expectedRoundSeconds( planPath: string, round: number | undefined, nowMs: number = Date.now(), ): number { - const stamps = readRoundStamps(planPath); - for (let i = stamps.length - 1; i >= 0; i--) { - const s = stamps[i]; - if (round !== undefined && s.round === round) continue; - const observed = Math.round((nowMs - s.atMs) / 1000); - return Math.max(MIN_OBSERVED_ROUND_SECONDS, observed); + const stamps = readRoundStamps(planPath).filter( + (s) => round === undefined || s.round !== round, + ); + if (stamps.length === 0) return DEFAULT_ROUND_SECONDS; + let maxSeconds = 0; + for (let i = 0; i < stamps.length; i++) { + const end = i + 1 < stamps.length ? stamps[i + 1].atMs : nowMs; + maxSeconds = Math.max( + maxSeconds, + Math.round((end - stamps[i].atMs) / 1000), + ); } - return DEFAULT_ROUND_SECONDS; + return Math.max(MIN_OBSERVED_ROUND_SECONDS, maxSeconds); } export interface BudgetExhausted { diff --git a/packages/cli/src/commands/review/lib/prompt-record.ts b/packages/cli/src/commands/review/lib/prompt-record.ts index bf783607840..ebf611212ca 100644 --- a/packages/cli/src/commands/review/lib/prompt-record.ts +++ b/packages/cli/src/commands/review/lib/prompt-record.ts @@ -27,7 +27,13 @@ // agent's actual launch prompt. The two artifacts have different authors, and // neither is the orchestrator. -import { mkdirSync, readFileSync, readdirSync, writeFileSync } from 'node:fs'; +import { + mkdirSync, + readFileSync, + readdirSync, + statSync, + writeFileSync, +} from 'node:fs'; import { dirname, join, basename, resolve } from 'node:path'; /** @@ -128,8 +134,24 @@ export function recordPrompt( } } -/** Every prompt this plan's builder emitted, keyed as it was recorded. */ -export function readRecordedPrompts(planPath: string): Map { +/** + * Every prompt this plan's builder emitted, keyed as it was recorded. + * + * `sinceMs` is the same fence every other reader of this directory applies — + * the plan's mtime. Nothing clears the record dir, and a run that dies + * mid-review leaves its records beside the retry's (the CI retry re-runs the + * review at the SAME plan path, under a freshly-captured plan): a record file + * older than the plan belongs to that dead attempt. A caller that reads + * records as OBLIGATIONS (coverage: "an agent was owed for this key") passes + * nothing — an obligation is not less owed for being stale, and dropping one + * would excuse the agent it demands. A caller that reads them as HISTORY + * (retirement) must pass the fence, or the dead attempt's records shadow the + * live ones. + */ +export function readRecordedPrompts( + planPath: string, + sinceMs?: number, +): Map { const out = new Map(); const dir = promptRecordDir(planPath); let names: string[]; @@ -150,7 +172,9 @@ export function readRecordedPrompts(planPath: string): Map { } catch { continue; // Not a name this module wrote. } - out.set(key, readFileSync(join(dir, name), 'utf8')); + const file = join(dir, name); + if (sinceMs !== undefined && statSync(file).mtimeMs < sinceMs) continue; + out.set(key, readFileSync(file, 'utf8')); } catch { /* raced with a cleanup */ } @@ -187,32 +211,66 @@ export function wasDeliveredVerbatim( launchPrompt: string, built: string, ): boolean { - // A zero-byte record is not a prompt, and the loop below would be vacuously true - // for it — the check would pass every agent, and the roster would credit a role - // to whichever transcript it happened to look at first. `recordPrompt` swallows - // its write errors by design (a read-only tmp dir must not stop a review being - // *built*), so an empty file is exactly what a partial write leaves behind. It is - // the one input that must fail closed. + return deliveredVerbatim(flattenPrompt(launchPrompt), built); +} + +/** + * `wasDeliveredVerbatim` with the launch prompt already put through + * `flattenPrompt`. The family exists for the one caller that pairs MANY + * records against MANY transcripts (the retirement scheduler): flattening is + * the expensive half of the check, and a caller that flattens each launch + * once pays it per transcript instead of per (record, transcript) pair — a + * few thousand full-prompt passes on the run the scheduler was built for, + * all before the round is admitted. Same contract, same failure modes. + */ +export function deliveredVerbatim( + flattenedLaunch: string, + built: string, +): boolean { if (built.trim().length === 0) return false; - const delivered = flatten(launchPrompt); + return deliveredVerbatimLines(flattenedLaunch, promptLines(built)); +} + +/** + * `deliveredVerbatim` with BOTH halves pre-flattened: the launch through + * `flattenPrompt`, the built prompt through `promptLines`. The scheduler + * hoists each record's `promptLines` alongside each transcript's flatten, so + * the pairing walk re-splits neither side — on the 6-chunk x 4-prior-round + * shape the old per-pair record flatten re-split every record's whole folded + * prompt (cumulative findings list included) once per candidate. + */ +export function deliveredVerbatimLines( + flattenedLaunch: string, + builtLines: string[], +): boolean { + // A zero-byte record is not a prompt, and the loop below would be vacuously + // true for it (no lines) — the check would pass every agent, and the roster + // would credit a role to whichever transcript it happened to look at first. + // `recordPrompt` swallows its write errors by design (a read-only tmp dir + // must not stop a review being *built*), so an empty file is exactly what a + // partial write leaves behind. It is the one input that must fail closed. + if (builtLines.length === 0) return false; let at = 0; - for (const line of lines(built)) { - const i = delivered.indexOf(line, at); + for (const line of builtLines) { + const i = flattenedLaunch.indexOf(line, at); if (i === -1) return false; at = i + line.length; } return true; } -/** Whitespace collapsed to single spaces: a re-wrap is not an edit. */ -function flatten(s: string): string { +/** + * Whitespace collapsed to single spaces: a re-wrap is not an edit. What + * `deliveredVerbatim` expects its launch side to have been put through. + */ +export function flattenPrompt(s: string): string { return s.replace(/\s+/g, ' ').trim(); } /** The built prompt's lines, whitespace-normalized, blanks dropped. */ -function lines(built: string): string[] { +export function promptLines(built: string): string[] { return built .split('\n') - .map((l) => flatten(l)) + .map((l) => flattenPrompt(l)) .filter((l) => l.length > 0); } diff --git a/packages/cli/src/commands/review/lib/retirement.test.ts b/packages/cli/src/commands/review/lib/retirement.test.ts new file mode 100644 index 00000000000..2143effa719 --- /dev/null +++ b/packages/cli/src/commands/review/lib/retirement.test.ts @@ -0,0 +1,790 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { + mkdtempSync, + mkdirSync, + readdirSync, + rmSync, + utimesSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { scheduleReverseAuditRound } from './retirement.js'; +import { promptRecordDir, recordPrompt } from './prompt-record.js'; +import { REVERSE_AUDIT_EXAMPLE_RECEIPT } from './agent-briefs.js'; + +// Direct unit coverage for the scheduler's own rules — the classifier's +// thresholds, the outcome merge, the injective guard and the parity rules — +// driving `scheduleReverseAuditRound` over synthetic histories instead of +// the full command handler. The handler tests exercise the same module end +// to end; this file is where a guard wired in the wrong direction fails +// loudly at the level it lives at. + +const DRY = + 'No new issues found — re-walked the whole territory, the retry cap and ' + + "both changed exports' call sites; every gap I checked was already in " + + 'the confirmed list.'; +const WHIFF = 'No issues found.'; +const YIELD = + 'Found one gap the prior rounds missed.\n\n' + + '- **File:** packages/cli/src/commands/review/x.test.ts:12\n' + + '- **Anchor:** const a = 1\n' + + '- **Issue:** off-by-one in the retry cap\n' + + '- **Severity:** Suggestion\n'; + +describe('scheduleReverseAuditRound — the scheduler on its own', () => { + let dir: string; + let plan: string; + let diff: string; + let seq = 0; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'retirement-')); + plan = join(dir, 'plan.json'); + writeFileSync(plan, '{}'); + // Backdate the plan so every transcript this test writes counts as + // newer — the same mtime fence the scheduler applies against a previous + // review's agents in the same session. + const old = new Date(2020, 0, 1); + utimesSync(plan, old, old); + diff = join(dir, 'diff.txt'); + process.env['QWEN_CODE_PROJECT_DIR'] = dir; + process.env['QWEN_CODE_SESSION_ID'] = 'S1'; + mkdirSync(join(dir, 'subagents', 'S1'), { recursive: true }); + }); + + afterEach(() => { + delete process.env['QWEN_CODE_PROJECT_DIR']; + delete process.env['QWEN_CODE_SESSION_ID']; + rmSync(dir, { recursive: true, force: true }); + }); + + /** + * Record a built (chunk, round) prompt the way the builder does. The role + * id is stapled onto the body because the real builder always emits it — + * the identity line and the brief path both carry `reverse-audit` — and + * the scheduler's cheap transcript pre-filter keys on it: a synthetic + * launch without it is dropped before the pairing walk. + */ + function record( + round: number, + chunk: number, + body: string, + digest = 'abc123', + ): string { + const prompt = `reverse-audit ${body}`; + recordPrompt( + plan, + `reverse-audit--chunk-${chunk}--round-${round}--${digest}`, + prompt, + ); + return prompt; + } + + /** Where `record` put a key's file — for tests that backdate one. */ + function recordFile(round: number, chunk: number, digest: string): string { + return join( + promptRecordDir(plan), + `${encodeURIComponent(`reverse-audit--chunk-${chunk}--round-${round}--${digest}`)}.txt`, + ); + } + + /** + * Write a transcript the way the harness writes one: launch prompt first, + * then `calls` successful reads of `filePath` (the diff unless told + * otherwise), then the final text. `calls: 0` is the whiff shape — prose + * and nothing else. + */ + function transcript( + launchPrompt: string, + finalText: string, + calls = 1, + filePath: string = diff, + offset = 0, + limit = 100, + ): void { + const id = `aud-${++seq}`; + const base = { + agentId: id, + agentName: 'general-purpose', + sessionId: 'S1', + }; + const lines = [ + JSON.stringify({ + ...base, + type: 'user', + message: { role: 'user', parts: [{ text: launchPrompt }] }, + }), + ]; + for (let i = 0; i < calls; i++) { + lines.push( + JSON.stringify({ + ...base, + type: 'assistant', + message: { + role: 'model', + parts: [ + { + functionCall: { + name: 'read_file', + args: { file_path: filePath, offset, limit }, + }, + }, + ], + }, + }), + JSON.stringify({ + ...base, + type: 'tool_result', + message: { + role: 'user', + parts: [ + { + functionResponse: { + name: 'read_file', + response: { output: 'diff bytes' }, + }, + }, + ], + }, + }), + ); + } + lines.push( + JSON.stringify({ + ...base, + type: 'assistant', + message: { role: 'model', parts: [{ text: finalText }] }, + }), + ); + writeFileSync( + join(dir, 'subagents', 'S1', `agent-${id}.jsonl`), + lines.join('\n') + '\n', + ); + } + + function schedule(round: number, chunks = [13, 14, 15]) { + return scheduleReverseAuditRound(plan, chunks, round, process.env, diff); + } + + /** Two dry rounds answered honestly, one transcript per record. */ + function dryTwice(chunks: number[]): void { + for (const r of [1, 2]) { + for (const c of chunks) { + transcript(record(r, c, `chunk ${c} round ${r} territory walk`), DRY); + } + } + } + + it('rounds 1 and 2 fan out to every chunk, reading no history', () => { + expect(schedule(1)).toEqual({ + due: [13, 14, 15], + coldChecks: [], + skipped: [], + converged: false, + }); + expect(schedule(2).due).toEqual([13, 14, 15]); + }); + + it('a chunk twice dry retires on the odd round and cold-checks on the even one', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY); + // 14 and 15 stay hot: records with no transcript certify nothing. + record(1, 14, 'chunk 14 round 1 territory walk'); + record(2, 14, 'chunk 14 round 2 territory walk'); + record(1, 15, 'chunk 15 round 1 territory walk'); + record(2, 15, 'chunk 15 round 2 territory walk'); + + const r3 = schedule(3); + expect(r3.due).toEqual([14, 15]); + expect(r3.coldChecks).toEqual([]); + expect(r3.converged).toBe(false); + expect(r3.skipped).toEqual([ + { chunkId: 13, dryRounds: [1, 2], nextColdCheck: 4 }, + ]); + + const r4 = schedule(4); + expect(r4.due).toEqual([13, 14, 15]); + expect(r4.coldChecks).toEqual([13]); + expect(r4.skipped).toEqual([]); + }); + + it('a bare receipt is not dry — the substance floor rejects it', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + // The stock sixteen-character sentence, with the tool calls to look + // believable: the floor still reads it as `unknown`, not `dry`. + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), WHIFF); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([13]); + expect(r3.skipped).toEqual([]); + }); + + it('a return that never opened the diff is not dry, however substantive it sounds', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY, 0); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('successful calls that never touched the diff are not dry — the two guards are independent', () => { + // Every other transcript here reads the diff, so `successfulToolCalls` + // and `diffToolCalls` move in lockstep and the classifier's two guards + // are exercised only together. An auditor that reads only its own brief + // clears the first guard but not the second: the receipt must still + // read `unknown`, so the chunk stays under audit. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + DRY, + 1, + join(dir, 'brief.md'), + ); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([13]); + expect(r3.skipped).toEqual([]); + }); + + it('a finding outranks a dry receipt — yielded history keeps the chunk hot', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), YIELD); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('one launch matching several records certifies none — the guard is records per transcript', () => { + // The shortcut's real shape is ONE agent handed the whole round's + // blocks. Its single transcript verbatim-contains every record, so it + // is each record's unique match — counting transcripts per record + // would credit every chunk the same receipt and retire the round + // whole. Matching several records, it must certify none. + const r1 = [13, 14].map((c) => record(1, c, `chunk ${c} round 1 walk`)); + const r2 = [13, 14].map((c) => record(2, c, `chunk ${c} round 2 walk`)); + transcript(r1.join('\n\n'), DRY); + transcript(r2.join('\n\n'), DRY); + + const r3 = schedule(3, [13, 14]); + expect(r3.due).toEqual([13, 14]); + expect(r3.skipped).toEqual([]); + expect(r3.converged).toBe(false); + }); + + it('several honest transcripts for ONE record all certify it — the relaunch merge', () => { + // SKILL mandates relaunching a whiffing auditor once within the round, + // with the same block verbatim: two transcripts, one record. Both must + // count — the whiff reads `unknown`, the substantive receipt `dry`, + // and the merge takes the dry. + const p1 = record(1, 13, 'chunk 13 round 1 territory walk'); + transcript(p1, WHIFF, 0); + transcript(p1, DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a yield in ANY matching transcript outranks the merge', () => { + const p1 = record(1, 13, 'chunk 13 round 1 territory walk'); + transcript(p1, YIELD); + transcript(p1, DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('staggered certificates share one parity — both cold-check on the even round', () => { + // 13 earns its certificate off rounds 1,2; 14 a round later, off 2,3. + // Per-chunk parity anchors would cold-check them on opposite rounds + // forever; one global parity lines them up. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), DRY); + transcript(record(1, 14, 'chunk 14 round 1 territory walk'), YIELD); + transcript(record(2, 14, 'chunk 14 round 2 territory walk'), DRY); + transcript(record(3, 14, 'chunk 14 round 3 territory walk'), DRY); + + // Round 3: 13 retired (odd round → skipped); 14's certificate only + // completes once its round-2 and round-3 audits are both in history. + expect(schedule(3, [13, 14]).due).toEqual([14]); + // Round 4: both retired, both cold-checked together. + const r4 = schedule(4, [13, 14]); + expect(r4.due).toEqual([13, 14]); + expect(r4.coldChecks).toEqual([13, 14]); + }); + + it('all retired and none due is convergence', () => { + dryTwice([13, 14]); + const r3 = schedule(3, [13, 14]); + expect(r3.due).toEqual([]); + expect(r3.coldChecks).toEqual([]); + expect(r3.converged).toBe(true); + expect(r3.skipped.map((s) => s.chunkId)).toEqual([13, 14]); + }); + + it('a yielding cold check puts the chunk back on the every-round schedule', () => { + dryTwice([13]); + // Round 3 skipped; round 4 is the cold check — and it yields. + transcript(record(4, 13, 'chunk 13 round 4 territory walk'), YIELD); + + const r5 = schedule(5, [13]); + expect(r5.due).toEqual([13]); + expect(r5.coldChecks).toEqual([]); + expect(r5.converged).toBe(false); + }); + + it('the records of the round being built are not history', () => { + dryTwice([13]); + // A rebuild of round 3 (a repaired delivery) writes a round-3 record + // before the schedule is asked; it must not count as evidence. + record(3, 13, 'chunk 13 round 3 territory walk'); + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.skipped.map((s) => s.chunkId)).toEqual([13]); + }); + + it('transcripts older than the plan do not count', () => { + const p1 = record(1, 13, 'chunk 13 round 1 territory walk'); + const p2 = record(2, 13, 'chunk 13 round 2 territory walk'); + transcript(p1, DRY); + transcript(p2, DRY); + // Age every transcript this test wrote to a fixed past (a previous + // review in the same session), then move the fence past it. The + // records keep their real mtimes and stay fresh, so only the + // transcripts age out — unfenced, they would verbatim-match the + // records and retire the chunk. Advancing the plan to a FUTURE + // instant instead would fence the records out too, and `due` would + // pass with zero records regardless of transcripts. + const old = new Date(2021, 0, 1); + for (const name of readdirSync(join(dir, 'subagents', 'S1'))) { + utimesSync(join(dir, 'subagents', 'S1', name), old, old); + } + const fence = new Date(2022, 0, 1); + utimesSync(plan, fence, fence); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it("records older than the plan are a dead attempt's — the retry still retires", () => { + // The CI retry re-runs the review at the SAME plan path and nothing + // clears the record dir. The dead attempt's findings list is a prefix of + // the retry's, so the retry's honest launch verbatim-contains BOTH + // records for a (chunk, round) — unfenced, the injectivity guard counts + // two records for one transcript and certifies neither, and the retry + // never retires a chunk. Fenced by file mtime against the plan — the + // same fence the transcripts and the budget files take — the dead + // records read as absent and the honest pair certifies. + const fresh: string[] = []; + for (const r of [1, 2]) { + record(r, 13, `chunk 13 round ${r} territory walk`, 'dead01'); + fresh.push( + record( + r, + 13, + `chunk 13 round ${r} territory walk\nwith the retry's grown findings list`, + ), + ); + } + transcript(fresh[0], DRY); + transcript(fresh[1], DRY); + // Both attempts' records fresh: ambiguous, so nothing certifies — the + // exact shape the probe measured (`two attempts, twice dry → due: [13]`). + expect(schedule(3, [13]).due).toEqual([13]); + + // Backdate the dead attempt's records past the plan's mtime: fenced out, + // the retry's own pair is each transcript's unique match, and it retires. + const dead = new Date(2019, 0, 1); + utimesSync(recordFile(1, 13, 'dead01'), dead, dead); + utimesSync(recordFile(2, 13, 'dead01'), dead, dead); + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('an honest short English receipt is dry — structure, not a length floor', () => { + // 78 characters, the probe that stayed hot under the old 120-char floor: + // the phrase, the dash, and a clause naming what was re-walked. + const receipt = + 'No issues found — re-walked the retry cap and both changed ' + + "exports' call sites."; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a receipt whose phrase is bolded is dry — emphasis is not a sentence break', () => { + // Auditors bold the phrase in the same **File:** / **Severity:** idiom + // the pipeline writes in; the old separator class refused the closing + // marks, so the most idiomatic shape never retired — on the unfixed + // class this receipt reads `unknown` and the chunk stays due. + const receipt = + '**No issues found** — re-walked the retry cap and both changed ' + + "exports' call sites."; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a bolded Chinese phrase is dry, exactly like the English one', () => { + const receipt = + '**未发现新问题** —— 重新走查了重连状态机与两个已改导出的全部调用点,' + + '每个疑点都已在确认清单中。'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a parenthesised scope between phrase and separator is dry', () => { + // The filler admits parentheses beside words: a scope label is not a + // sentence break, and the clause after the separator still names the + // territory. + const receipt = + 'No new issues found (chunk 13) — re-walked the retry cap and both ' + + "changed exports' call sites."; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a Chinese receipt with a named territory is dry', () => { + // The other probe: auditors narrate in the review's output language, and + // the old English-only phrase left a zh receipt `unknown` at any length. + const receipt = + '未发现新问题——重新走查了重连状态机与两个已改导出的全部调用点,' + + '每个疑点都已在确认清单中。'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('the bare zh stock sentence is not dry, exactly like the English one', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + '未发现问题。', + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('a receipt whose clause names nothing is not dry', () => { + // The structure is phrase, separator, then a clause that NAMES what was + // examined — "all good." clears a separator but names no territory. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + 'No new issues found — all good.', + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('a launch without the builder’s role marker certifies nothing', () => { + // The real builder never emits a reverse-audit launch without the role + // id in it, and the scheduler drops marker-less transcripts before the + // pairing walk. A hand-built record whose body lacks it can only lose + // matches — and a lost match fails toward auditing. + for (const r of [1, 2]) { + const bare = `chunk 13 round ${r} bare body`; + recordPrompt(plan, `reverse-audit--chunk-13--round-${r}--abc123`, bare); + transcript(bare, DRY); + } + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('an echoed file line without the finding block is not a yield', () => { + // The cumulative list rides in the launch prompt, and an auditor + // explaining "already covered" can quote an entry's **File:** line into + // its return. A quotation is not a report: a filed finding carries the + // full block, severity included, and only the pair reads as `yielded`. + // The echo still ends in a substantive receipt, so the chunk retires. + const echo = + 'The cumulative list already covers **File:** src/pay.ts:42 — not ' + + 're-reporting it.\n\n' + + DRY; + for (const r of [1, 2]) { + const built = record(r, 13, `chunk 13 round ${r} territory`); + transcript(built, echo); + } + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.skipped.map((s) => s.chunkId)).toEqual([13]); + }); + + it('a Chinese receipt separated by a full-width colon is dry', () => { + // U+FF1A is the standard zh separator; the receipt's separator class + // admits a colon in either width. Probed on the unfixed class: the + // byte-identical receipt with an ASCII colon retired while this one + // read `unknown` and re-audited every round. + const receipt = + '未发现问题:重新走查了重连状态机与两个已改导出的全部调用点,' + + '每个疑点都已在确认清单中。'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it("parroting the brief's own example receipt is not dry", () => { + // Every reverse auditor is handed this exact sentence as the model + // answer; a clause that echoes it names nothing the agent examined + // itself, whatever its length. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + `${REVERSE_AUDIT_EXAMPLE_RECEIPT}.`, + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('a stray backtick is not a named object — only an enclosed span is', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + 'No new issues found — all good. `', + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('an enclosed code span still names an object', () => { + // Short enough that ONLY the span shortcut can clear it. + const receipt = 'No issues found — `retry-cap`.'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('the conjunction "and/or" is not a path', () => { + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk'), + 'No issues found — and/or cases.', + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('a real path still names an object — dotted extension, one slash', () => { + // Short enough that ONLY the path shortcut can clear it. + const receipt = 'No issues found — checked src/pay.ts.'; + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), receipt); + transcript(record(2, 13, 'chunk 13 round 2 territory walk'), receipt); + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('a diff read outside the baked territory is not dry', () => { + // The record bakes the chunk's read; the transcript's only diff read + // is elsewhere in the file. The receipt reads `unknown` whatever it + // says — an auditor that never opened the territory has no claim on + // it, and no other stage re-asks the question. + for (const r of [1, 2]) { + const built = record( + r, + 13, + `chunk 13 round ${r} walk — ` + + `read_file(file_path="${diff}", offset=1000, limit=200)`, + ); + transcript(built, DRY, 1, diff, 0, 50); + } + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('an overlapping read of the baked territory still retires', () => { + // Overlap is the bar, not containment: the second audit pages the + // territory, and its half-read still lands inside. + for (const r of [1, 2]) { + const built = record( + r, + 13, + `chunk 13 round ${r} walk — ` + + `read_file(file_path="${diff}", offset=1000, limit=200)`, + ); + transcript( + built, + DRY, + 1, + diff, + r === 1 ? 1000 : 1100, + r === 1 ? 200 : 50, + ); + } + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('findings prose quoting a read window cannot widen the territory', () => { + // The record is the FOLDED launch prompt — the cumulative findings + // list rides inside it, verbatim. Prose quoting ANY `offset=N, + // limit=M` pair (a read_file call under discussion; this PR's own + // review threads do) used to inject the range into the territory, and + // any-overlap-with-any-range passes: an auditor whose only diff read + // was lines 1-50 retired a chunk whose territory is 1001-1200 the + // moment a finding quoted `offset=0, limit=50`. Only the read aimed + // at the diff is territory. + for (const r of [1, 2]) { + const built = record( + r, + 13, + `chunk 13 round ${r} walk\n` + + '## Already confirmed — do not re-report these\n' + + 'the earlier read used read_file(offset=0, limit=50)\n' + + `read_file(file_path="${diff}", offset=1000, limit=200)`, + ); + transcript(built, DRY, 1, diff, 0, 50); + } + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('the real baked read still retires beside noisy findings', () => { + // The positive control for the bound scan: the same findings noise, + // and the auditor opens the territory itself. + for (const r of [1, 2]) { + const built = record( + r, + 13, + `chunk 13 round ${r} walk\n` + + '## Already confirmed — do not re-report these\n' + + 'the earlier read used read_file(offset=0, limit=50)\n' + + `read_file(file_path="${diff}", offset=1000, limit=200)`, + ); + transcript(built, DRY, 1, diff, 1000, 200); + } + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(true); + }); + + it('quoting a WHOLE cumulative-list entry is not a yield', () => { + // The cumulative list rides in the launch prompt as full blocks — + // File AND Severity — and an auditor justifying "already covered" + // can quote one whole. A file line appearing verbatim in its own + // launch prompt marks the quotation; the honest receipt underneath + // still retires the chunk. + const quoted = + 'The list already carries this entry, so it is not re-reported:\n' + + '- **File:** src/pay.ts:42\n' + + '- **Severity:** Suggestion\n\n' + + DRY; + for (const r of [1, 2]) { + const built = record( + r, + 13, + `chunk 13 round ${r} territory\n**File:** src/pay.ts:42`, + ); + transcript(built, quoted); + } + + const r3 = schedule(3, [13]); + expect(r3.due).toEqual([]); + expect(r3.skipped.map((s) => s.chunkId)).toEqual([13]); + }); + + it('a cold check nobody certified puts the chunk back on the every-round schedule', () => { + dryTwice([13]); + // Round 4 is the cold check — built, but the launch left no certified + // transcript. The round still belongs to the history with an empty + // outcome set, so the two-most-recent-dry rule breaks and the chunk + // is hot again — a refactor skipping empty rounds would retire it + // forever over a cold check that produced no evidence. + record(4, 13, 'chunk 13 round 4 territory walk'); + + const r5 = schedule(5, [13]); + expect(r5.due).toEqual([13]); + expect(r5.coldChecks).toEqual([]); + expect(r5.converged).toBe(false); + }); + + it('a chunk with no audit history stays due when its neighbour retires', () => { + dryTwice([13]); + // 16 entered the loop mid-capture (or its records were lost): no + // history at all. Retirement needs TWO certificates, and nothing is + // not one — the chunk stays hot while 13 skips. + const r3 = schedule(3, [13, 16]); + expect(r3.due).toEqual([16]); + expect(r3.skipped.map((s) => s.chunkId)).toEqual([13]); + expect(r3.converged).toBe(false); + }); + + it('two live records for ONE (chunk, round): any yield keeps it hot', () => { + // A --chunk repair re-records the same (chunk, round) under a new + // findings digest; both records stay live. The bodies are disjoint so + // each transcript certifies exactly its own record, and the merge + // must carry BOTH outcomes — one yield proves the territory hot + // whichever order the filesystem returns the records in. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk', 'aaa111'), + YIELD, + ); + transcript( + record(2, 13, 'chunk 13 round 2 rules-corrected rebuild walk', 'fff999'), + DRY, + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('the same-round pair still outranks retirement with the digest order flipped', () => { + // The twin of the test above with the digests swapped: the two + // arrangements flip the filesystem's record order, so a + // last-record-wins overwrite cannot pass both. + transcript(record(1, 13, 'chunk 13 round 1 territory walk'), DRY); + transcript( + record(2, 13, 'chunk 13 round 2 territory walk', 'fff999'), + YIELD, + ); + transcript( + record(2, 13, 'chunk 13 round 2 rules-corrected rebuild walk', 'aaa111'), + DRY, + ); + + expect(schedule(3, [13]).due).toEqual([13]); + }); + + it('an empty chunk list is not convergence', () => { + // Unreachable through the command (`runAllChunks` refuses a chunkless + // plan first), but the function is exported and convergence is an exit-5 + // termination rule: it must not be reachable from nothing. + const r3 = schedule(3, []); + expect(r3.due).toEqual([]); + expect(r3.converged).toBe(false); + }); +}); diff --git a/packages/cli/src/commands/review/lib/retirement.ts b/packages/cli/src/commands/review/lib/retirement.ts new file mode 100644 index 00000000000..b386b538404 --- /dev/null +++ b/packages/cli/src/commands/review/lib/retirement.ts @@ -0,0 +1,513 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// Per-chunk retirement for the Step 5 reverse-audit loop. +// +// On a 3B plan the loop launches one auditor PER CHUNK PER ROUND, up to five +// rounds. Measured on a real run (6 chunks × 5 rounds = 30 auditors, ~95 +// minutes): chunks 3 and 6 came back dry in ALL five rounds, while chunks 1, +// 2 and 4 yielded in most of them. The loop's convergence rule is +// round-global — two consecutive dry ROUNDS — so one hot territory keeps +// every cold one under audit for the whole run: auditor after auditor +// re-walking code that has twice produced a substantive all-clear, on +// exactly the large reviews where the rounds they pad push the loop into the +// budget gate. +// +// So from round 3 on the schedule becomes per-chunk. A chunk whose two most +// recent audits are both substantive dry receipts is RETIRED: instead of an +// auditor every round it gets a cold check on alternating rounds, and a cold +// check that yields puts it straight back on the every-round schedule. +// Rounds 1 and 2 always fan out to every chunk — they are what establishes +// each chunk's record. +// +// The history this is read from is the same pair of artifacts every delivery +// check trusts: the prompts this CLI recorded itself building (keyed +// `reverse-audit--chunk---round---`) and the harness's own +// transcripts of the agents launched with them. Nothing the orchestrator +// writes is consulted — a schedule the subject of the checks could edit is a +// schedule that retires whatever chunk is inconvenient to audit. +// +// Everything here fails toward auditing, chunk by chunk: no transcripts, no +// matching transcript, a whiffed receipt, an unclassifiable return — each +// reads as "not dry", and a chunk that cannot prove itself cold stays hot. +// The failure mode of a bug in this file is the old behaviour (audit every +// territory every round), never a skipped one. + +import { statSync } from 'node:fs'; +import { readTranscripts, type AgentRecord } from './transcripts.js'; +import { REVERSE_AUDIT_EXAMPLE_RECEIPT } from './agent-briefs.js'; +import { + deliveredVerbatimLines, + flattenPrompt, + promptLines, + readRecordedPrompts, +} from './prompt-record.js'; + +/** What one prior audit of one chunk provably produced. */ +export type AuditOutcome = 'yielded' | 'dry' | 'unknown'; + +/** A retired chunk skipped this round, with the receipts that earned it. */ +export interface RetiredChunk { + chunkId: number; + /** The two most recent audit rounds — both substantive dry receipts. */ + dryRounds: [number, number]; + /** The next round whose parity puts the chunk back under audit. */ + nextColdCheck: number; +} + +export interface RoundSchedule { + /** Chunk ids to build this round, in the order the caller gave them. */ + due: number[]; + /** The subset of `due` that is a retired chunk's alternating cold check. */ + coldChecks: number[]; + /** Retired chunks NOT due this round — the retirement note names these. */ + skipped: RetiredChunk[]; + /** Every chunk is retired and none is due: the audit has converged. */ + converged: boolean; +} + +/** + * The loop's hard cap, mirroring SKILL.md's Step 5 ("Stop after 5 rounds + * regardless"). Enforcing it is the orchestrator's — the builder will build + * a sixth round if asked — but the retirement note is the orchestrator's + * only word about a skipped chunk, and it must not promise a cold check the + * cap has already forbidden. + */ +export const REVERSE_AUDIT_MAX_ROUNDS = 5; + +/** + * The round part of a per-chunk reverse-audit record key, as `runAllChunks` + * and the single-chunk rebuild path both spell it. The digest tail is matched + * loosely on purpose: its width is the digest function's business, and a key + * this regex misses is merely history this module cannot see — fail-open. + */ +const RECORD_KEY_RE = /^reverse-audit--chunk-(\d+)--round-(\d+)--[0-9a-f]+$/; + +/** + * Every launch the builder emits for this loop carries the literal role id — + * the identity line and the brief path both spell it, whitespace-free, so no + * re-wrap can hide it — and each record's own lines carry it too, so a + * transcript that could verbatim-match any record must contain it. That makes + * it a sound cheap cut over the transcripts before the pairing walk. + */ +const REVERSE_AUDIT_MARKER = 'reverse-audit'; + +/** + * The diff lines a record's prompt points its chunk at, 1-based and + * inclusive. Every per-chunk launch this CLI builds bakes exactly one + * `read_file(file_path="…", offset=N, limit=M)` aimed at the diff; the dry + * bar compares what the transcript actually read against it. Empty when the + * prompt bakes no read, where the bar falls back to "opened the diff at + * all" — a shape this module's own records never have. + * + * The scan is bound to the diff's own path because the record is the FOLDED + * launch prompt — the cumulative findings list rides inside it, verbatim, + * above the builder's own text — and findings prose quoting ANY + * `offset=N, limit=M` pair (a read_file call under discussion, this very + * file in a diff) would otherwise inject its range into the territory. + * `openedTheTerritory` passes on ANY overlap with ANY range, so an injected + * range can only WIDEN the bar: an auditor whose only diff read was lines + * 1-50 would retire a chunk whose territory is 1001-1200 the moment a + * finding quoted `offset=0, limit=50` — the same range-blind hole the + * territory check exists to close, reopened by honest findings. Only a read + * aimed at the diff is territory. An unknown diff path reads as no + * territory: the transcripts side then marks no call a diff read, every + * transcript classifies `unknown`, and no chunk retires — the territory is + * never consulted. + */ +function bakedRanges( + prompt: string, + diffPath: string | undefined, +): Array<[number, number]> { + const out: Array<[number, number]> = []; + if (diffPath === undefined) return out; + for (const m of prompt.matchAll( + /read_file\(\s*file_path="([^"]*)",\s*offset=(\d+),\s*limit=(\d+)/gi, + )) { + if (m[1] !== diffPath) continue; + const offset = Number(m[2]); + const limit = Number(m[3]); + if (limit > 0) out.push([offset + 1, offset + limit]); + } + return out; +} + +/** A finding's file line — the shape `FINDING_FORMAT` asks every role for. */ +const FILE_LINE_RE = /\*\*File:\*\*\s*([^\n]*)/g; + +/** + * The other half of a filed finding. A `**File:**` line alone is not proof + * the auditor FILED anything: the cumulative list is folded into its launch + * prompt, and an auditor explaining "already covered, not re-reporting" can + * echo an entry's file line into its return. Every finding actually filed + * carries the full block the format mandates — severity included — so the + * pair is what distinguishes a report from a bare file-line echo; a + * quotation of a WHOLE entry is caught in `classifyReturn`, where the + * launch prompt carrying the cumulative list is on hand. Misreading an echo + * as `yielded` is cost, not corruption (the chunk just stays hot), but it is + * exactly the cost this module exists to stop paying. + */ +const SEVERITY_LINE_RE = /\*\*Severity:\*\*/; + +/** + * The no-issues receipt, read by its STRUCTURE: the phrase, a dash or colon, + * then the clause naming what was re-examined. + * + * The reverse-audit brief demands a receipt that "names what it re-examined", + * and its own model answer has exactly this shape — "No issues found — + * re-walked the reconnect state machine and the two changed exports' call + * sites; …". A bare "No issues found." — the text 23 real whiffing agents + * returned — has the phrase and nothing after it, and specific-sounding + * brevity is not evidence of a walk. The first cut enforced a 120-character + * floor on the whole return instead of this structure, and both of its + * failure modes pointed the same silent way: an honest 78-character English + * receipt and a Chinese receipt of ANY length both read `unknown`, and the + * chunk never retired — on exactly the budgeted runs the optimization exists + * for. Auditors narrate in the review's output language (`未发现问题` is the + * phrasing compose-review itself ships), so the phrase accepts the zh forms + * beside the English ones. + * + * The separator admits an em/en dash anywhere (`——` doubled included), a + * colon in either width, and an ASCII hyphen only when it stands alone — + * space-led or doubled — so the dash inside `retry-cap` never opens a clause + * mid-word. Closing emphasis and quotation may sit between the phrase and + * the separator — auditors bold the phrase (`**No issues found** — …`, + * `**未发现新问题** —— …`) in the same `**File:**` / `**Severity:**` idiom + * the rest of the pipeline writes in, and a receipt refused on a bold mark + * reads `unknown` and never retires, on exactly the budgeted runs the + * optimization exists for. The filler between phrase and separator (`were + * found`, `were detected`, a parenthesised scope) is capped and word-only + * apart from parentheses: a period or other markdown in between is a new + * sentence, not this receipt. + */ +const DRY_RECEIPT_RE = new RegExp( + '(?:\\bno (?:new )?(?:issues?|findings?|gaps?)[ \\w()]{0,32}?' + + '|未发现(?:新的?)?(?:问题|发现)' + + '|无新的?(?:问题|发现)' + + '|没有(?:发现)?(?:新的?)?问题)' + + '\\s*[*_)\\]"”’]*\\s*(?:[—–]+|[::]|--+|-+\\s)\\s*' + + '([\\s\\S]*)', + 'i', +); + +/** CJK ideographs — a zh clause packs its substance into far fewer chars. */ +const CJK_RE = /[一-鿿]/g; + +/** + * The clause the brief's own example receipt leaves AFTER its separator — + * extracted with the same regex that parses receipts, so the two cannot + * drift. Empty when the example ever stops matching its own parser, which + * disables the parrot refusal rather than refuse every clause. + */ +const EXAMPLE_RECEIPT_CLAUSE = ( + DRY_RECEIPT_RE.exec(REVERSE_AUDIT_EXAMPLE_RECEIPT)?.[1] ?? '' +).trim(); + +/** + * Does the clause after the receipt's separator name anything? An ENCLOSED + * code span or a real path is a named object at any length — a stray + * backtick is prose punctuation, not a quotation, and "N/A" is not a path + * (one character on the slash's left), neither is the conjunction "and/or": + * a path has a second slash or a dotted extension. Otherwise ~20 flattened + * characters, or a handful of ideographs, is the least that can name a + * territory; "all good." can not. The brief's own example receipt is + * refused outright, but only VERBATIM: a clause containing the example's + * whole clause reads as the parrot it is, while real parroting is partial + * — the shape and a phrase or two — and a partial echo passes this check; + * what catches that is the rest of the dry bar (the territory read, the + * substance floor). This refusal closes the cheapest path: the exact + * sentence every auditor is handed. Misjudging here fails the way + * everything in this module fails — the receipt reads `unknown` and the + * chunk stays under audit. + */ +function substantiveClause(clause: string): boolean { + const c = clause.replace(/\s+/g, ' ').trim(); + if (c.length === 0) return false; + if (EXAMPLE_RECEIPT_CLAUSE.length > 0 && c.includes(EXAMPLE_RECEIPT_CLAUSE)) { + return false; + } + if (/`[^`]+`/.test(c)) return true; + if (/\w[\w.-]+\/[\w.$-]+\/\w/.test(c)) return true; + if (/\w[\w.-]+\/[\w$-]+\.\w+/.test(c)) return true; + if ((c.match(CJK_RE) ?? []).length >= 4) return true; + return c.length >= 20; +} + +/** + * Classify one auditor's return. + * + * `yielded` outranks everything: a return that files a finding against a + * real file proves the territory hot, whatever else it says. `dry` requires + * all of a structurally substantive no-issues receipt AND the tool calls + * that make it believable — an agent that never opened the diff has an + * opinion about lines it did not read, which is the whiff wearing a costume + * (measured: 80 of 129 real transcripts made no tool call, and every one + * still returned confident, specific-sounding prose) — AND the read must + * land in the chunk's territory: a successful read of the diff's first + * screenful proves nothing about lines a thousand down. Anything else is + * `unknown`, which the scheduler treats as NOT dry. + * + * `territory` is the diff lines the record's own prompt bakes for this + * chunk (1-based, inclusive); empty when it bakes no read, where the old + * bar — any successful diff read — stands. + */ +function classifyReturn( + rec: AgentRecord, + territory: Array<[number, number]>, +): AuditOutcome { + const text = rec.finalText.trim(); + if (SEVERITY_LINE_RE.test(text)) { + for (const m of text.matchAll(FILE_LINE_RE)) { + const file = (m[1] ?? '').trim(); + if (file === '' || /^N\/A\b/i.test(file)) continue; + // The cumulative list rides in this agent's own launch prompt, + // folded verbatim, and every entry in it is a full block — File + // AND Severity. An auditor explaining "already covered, not + // re-reporting" can quote one whole, and the quotation must not + // read as a filing: an entry whose exact file line is already on + // the list cannot be a new finding against it. Skipping costs an + // audit at most; counting a quotation re-opens the never-retire + // direction on the loop's most common honest return. + if (rec.launchPrompt.includes(`**File:** ${file}`)) continue; + return 'yielded'; + } + } + const receipt = DRY_RECEIPT_RE.exec(text); + if ( + rec.successfulToolCalls > 0 && + rec.diffToolCalls > 0 && + openedTheTerritory(rec.diffReads, territory) && + receipt !== null && + substantiveClause(receipt[1] ?? '') + ) { + return 'dry'; + } + return 'unknown'; +} + +/** + * Whether any of the transcript's reads lands in the chunk's baked + * territory. Overlap is the bar, not containment: an honest auditor pages + * an oversized chunk, and each page overlaps the territory even though no + * single read holds it all. A read with no line range (a `read_file` with + * no limit) proves no lines at all and overlaps nothing. + */ +function openedTheTerritory( + diffReads: Array<[number, number]>, + territory: Array<[number, number]>, +): boolean { + if (territory.length === 0) return true; + return diffReads.some(([s, e]) => + territory.some(([ts, te]) => s <= te && ts <= e), + ); +} + +/** + * One outcome for one (chunk, round), from every record and transcript that + * spoke to it. A round can legitimately have several of both — a same-round + * rebuild with corrected rules is a second record; a relaunch is a second + * transcript — and the merge fails toward auditing: any yield proves the + * territory hot, a dry needs at least one substantive receipt and no yield, + * and an empty set proves nothing. + */ +function mergeOutcomes(outcomes: AuditOutcome[]): AuditOutcome { + if (outcomes.includes('yielded')) return 'yielded'; + if (outcomes.includes('dry')) return 'dry'; + return 'unknown'; +} + +/** + * Which chunks round `round` owes an auditor, from the audit history the + * harness and the prompt records agree on. + * + * Retirement: a chunk whose two most recent audits are both `dry` is due + * only on even rounds — one round skipped, one round cold-checked, + * alternating on a SINGLE global parity every retired chunk shares, so + * staggered certificates re-align and the all-retired convergence stays + * reachable (the loop below says why the anchor is not the chunk's own + * parity). A retired chunk whose cold check yields simply stops satisfying + * the two-most-recent-dry rule and is due every round again; no state is + * kept anywhere, the history IS the state. + * + * Throws whatever the transcript or record readers throw + * (`TranscriptsUnavailableError` included): the CALLER owns the fail-open, + * because the right degradation — build every chunk — is a build decision, + * not a schedule. + */ +export function scheduleReverseAuditRound( + planPath: string, + chunkIds: number[], + round: number, + env: NodeJS.ProcessEnv = process.env, + diffPath?: string, +): RoundSchedule { + // Rounds 1 and 2 establish the record; there is nothing to retire on. + if (round < 3) { + return { + due: [...chunkIds], + coldChecks: [], + skipped: [], + converged: false, + }; + } + + // Transcripts older than the plan belong to a previous review in the same + // session — the same collision `coverageFromTranscripts` guards against. + // The records take the SAME fence: nothing clears the record dir, and the + // CI retry of a dead attempt re-runs the review at the same plan path with + // the dead attempt's records still on disk. The retry's honest launch — + // its findings list a superset of the dead attempt's, in the same order — + // verbatim-contains BOTH records for a (chunk, round), so unfenced records + // trip the injectivity guard below (two records, one transcript, neither + // certified): fail-safe, but retirement silently off on exactly the + // retries with the least time left. A record older than the plan is the + // dead attempt's, and reads as absent. + const since = statSync(planPath).mtimeMs; + const transcripts = readTranscripts(since, env, diffPath); + const built = readRecordedPrompts(planPath, since); + + // The prior-round records: one per (chunk, round) prompt this CLI built. + // Only PRIOR rounds are history — a record of the round being built is a + // rebuild of it (a repaired delivery), not evidence about the territory. + const records: Array<{ + chunkId: number; + round: number; + lines: string[]; + territory: Array<[number, number]>; + }> = []; + for (const [key, prompt] of built) { + const m = RECORD_KEY_RE.exec(key); + if (!m) continue; + const r = Number(m[2]); + if (r >= round) continue; + records.push({ + chunkId: Number(m[1]), + round: r, + // Flattened ONCE per record, beside the once-per-transcript flatten + // below: the pairing walk pays neither half per (record, transcript) + // pair. + lines: promptLines(prompt), + territory: bakedRanges(prompt, diffPath), + }); + } + + // Which transcripts certify which record — injectively, in the direction + // that actually bounds the shortcut: how many RECORDS each transcript + // matches. `wasDeliveredVerbatim` allows additions, so a launch prompt + // that verbatim-contains SEVERAL recorded prompts matches every one of + // them: one agent handed several blocks (or a whole round concatenated) + // is the shortcut this module exists to catch. That shortcut is ONE + // launch, and counting transcripts per record cannot see it — the single + // transcript is each record's unique match, so every chunk would be + // credited the same dry receipt and the round would retire whole. So + // invert the relation: a transcript that matches several records names + // no territory specifically and certifies none — the failure lands where + // every failure here lands, on the audit side. Honest launches are + // untouched either way: the round number and each chunk's territory are + // baked into the prompt, so one matches exactly one record, its own — + // and several honest transcripts for one record (the mandated whiff + // relaunch) each certify it, the multi-transcript merge `mergeOutcomes` + // promises. + // + // The pairing walk is O(records × transcripts) over multi-KB prompts, on + // the critical path before the round is admitted — so cut the transcript + // side down to the launches that carry the role marker first (a launch + // that could match any record contains it; see REVERSE_AUDIT_MARKER), and + // flatten each survivor ONCE instead of once per pair (the record side is + // already flattened once per record, above). A transcript the cut drops + // fails the way everything here fails: it certifies nothing, and its + // chunk stays under audit. + const candidates = transcripts + .filter((t) => t.launchPrompt.includes(REVERSE_AUDIT_MARKER)) + .map((t) => ({ transcript: t, flat: flattenPrompt(t.launchPrompt) })); + const matchesByRecord = records.map((rec) => + candidates + .filter((c) => deliveredVerbatimLines(c.flat, rec.lines)) + .map((c) => c.transcript), + ); + const recordsPerTranscript = new Map(); + for (const matches of matchesByRecord) { + for (const t of matches) { + recordsPerTranscript.set(t, (recordsPerTranscript.get(t) ?? 0) + 1); + } + } + const outcomesByRecord = matchesByRecord.map((matches, i) => + matches + .filter((t) => recordsPerTranscript.get(t) === 1) + .map((t) => classifyReturn(t, records[i].territory)), + ); + + // chunk id → prior round → every outcome that round's records produced. A + // record no transcript certifies (a blank partial write, an undelivered + // build, an ambiguous launch) contributes nothing, and its round + // classifies `unknown`: the round was scheduled for this chunk, and + // nothing proves it dry. + const history = new Map>(); + records.forEach((rec, i) => { + let byRound = history.get(rec.chunkId); + if (!byRound) { + byRound = new Map(); + history.set(rec.chunkId, byRound); + } + byRound.set(rec.round, [ + ...(byRound.get(rec.round) ?? []), + ...outcomesByRecord[i], + ]); + }); + + const due: number[] = []; + const coldChecks: number[] = []; + const skipped: RetiredChunk[] = []; + for (const chunkId of chunkIds) { + const audits = [...(history.get(chunkId)?.entries() ?? [])] + .map(([r, outcomes]) => ({ round: r, outcome: mergeOutcomes(outcomes) })) + .sort((a, b) => a.round - b.round); + const lastTwo = audits.slice(-2); + const retired = + lastTwo.length === 2 && lastTwo.every((a) => a.outcome === 'dry'); + if (!retired) { + // Hot — including a chunk with no history at all, one whose latest + // receipt was a whiff, and one whose cold check yielded. + due.push(chunkId); + continue; + } + // Cold checks land on ONE global parity — the even rounds — not on the + // chunk's own certificate parity. Per-chunk anchors never re-align: a + // chunk dry in rounds 2,3 (last dry round odd) beside one dry in 1,2 + // (even) cold-checks on opposite rounds forever, the all-retired + // CONVERGED exit can never fire, and the loop always runs to the cap — + // on exactly the staggered large-PR shape retirement exists for. A + // certificate that completes on an odd last-dry round simply takes its + // first cold check one round sooner; after that every retired chunk + // skips and cold-checks together, and convergence is reachable again. + if (round % 2 === 0) { + due.push(chunkId); + coldChecks.push(chunkId); + } else { + skipped.push({ + chunkId, + dryRounds: [lastTwo[0].round, lastTwo[1].round], + // The next even round — this branch only runs on odd rounds, so + // that is always round + 1. Whether the cap allows it is the note + // composer's question, not the schedule's (see + // REVERSE_AUDIT_MAX_ROUNDS). + nextColdCheck: round + 1, + }); + } + } + + return { + due, + coldChecks, + skipped, + // An empty `chunkIds` empties `due` vacuously — nothing was ever under + // audit, so nothing has proven itself cold. `runAllChunks` refuses a + // chunkless plan long before scheduling, but this function is exported, + // and convergence is an exit-5 termination rule: it must not be + // reachable from nothing. + converged: chunkIds.length > 0 && due.length === 0, + }; +} diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index c548f29346b..9f5c0f13a70 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -577,7 +577,7 @@ All confirmed findings (aggregated or standalone) proceed to Step 5. **Medium skips this step.** A balanced (medium) review stops after Step 4: it goes straight to Step 6, composes the report and verdict from the verified findings, and does not run the reverse audit — which is why `compose-review` caps a clean medium review at `Comment` (Step 6) and why medium never writes the incremental cache or posts (`--comment` forces high). Everything below is high effort only. -After aggregation, run reverse audit **iteratively**. Each round receives the cumulative confirmed findings from all prior rounds, so successive rounds focus on whatever the previous round missed. +After aggregation, run reverse audit **iteratively**. Each round receives the cumulative reported findings from all prior rounds, so successive rounds focus on whatever the previous round missed. **Why iterative**: A single pass leaves whatever the reverse audit agent itself missed. Each round narrows what's left to discover, until diminishing returns terminate the loop. @@ -585,10 +585,11 @@ After aggregation, run reverse audit **iteratively**. Each round receives the cu - **Small diffs (Step 3A path):** one reverse audit agent per round, reading the whole diff. - **Large diffs (Step 3B path):** one reverse audit agent **per chunk** per round, launched together in a single response. A single agent asked to re-read a 5 800-line diff with a growing finding list appended is the most context-starved agent in the pipeline — precisely on the PRs where the reverse audit matters most. Each per-chunk auditor gets the same territory as its Step 3B counterpart, plus the cumulative finding list for the **whole** diff (so it knows what is already covered elsewhere). +- **The builder schedules the 3B fan-out; you do not.** Rounds 1 and 2 audit every chunk — they are what establishes each territory's record. From round 3 on, `--all-chunks` reads the harness transcripts and **retires** any chunk whose own last two audits were substantively dry (the receipt named what it examined AND the transcript shows the diff was opened): a retired chunk is cold-checked on alternating rounds instead of every round, and a cold check that yields anything returns it to every-round auditing. The savings land on the odd rounds — every retired chunk cold-checks together on the even ones, so an even round's fan-out is unchanged; expect rounds 3 and 5 to shrink, not round 4. The blocks it prints are the round; the `retirement:` note after the `end of round` line names each skipped chunk and its certificate — relay that note in your narration, and do not hand-build an auditor for a chunk the builder skipped. Why, measured: on a real 6-chunk run, two chunks were dry in **all five rounds** — a third of the loop's auditors re-certifying territories that had already converged, while the three hot chunks were where every finding came from. Attention follows evidence; the certificate a retired chunk holds (two consecutive substantive dry audits) is exactly the one the whole loop used to end on. **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** -Write **the cumulative list of every confirmed finding so far** (Steps 3-4 plus all prior rounds) to a file, so the auditor hunts what is not already on it. An early round on a clean review may have nothing confirmed yet — pass the file anyway (empty is fine; the command tells the auditor so). Then: +Write **the cumulative list of every finding reported so far** (Steps 3-4 plus all prior rounds — verified or still under verification; entries a verifier rejected are removed) to a file, so the auditor hunts what is not already on it. **Every entry not yet through Step 4 carries a trailing `— [unverified]` tag** — added at the merge that admits it, removed by the merge after its verdict lands. An early round on a clean review may have nothing confirmed yet — pass the file anyway (empty is fine; the command tells the auditor so). Then: ```bash # Step 3A (small diff): one auditor per round, the whole diff. @@ -607,7 +608,7 @@ Write **the cumulative list of every confirmed finding so far** (Steps 3-4 plus Redirect and `read_file` it paged, exactly as with `--roster`: one labelled block per chunk, numbered `auditor k of N`, closed by an `end of round` line — launch one agent per block, verbatim. **Never sample the builder's output** (`| head`, `| tail`, a truncated read): the text IS the deliverable, and sampling it has cost a full repair round (measured; DESIGN.md — The head-sampled roster). To rebuild a single auditor after a gap: `--chunk ` in place of `--all-chunks`, keeping the same `--findings`, `--rules` and `--round` — a rebuild that drops one of them is keyed as a different launch and matches no requirement. -**`--findings` is required for this role — the command refuses without it** (an early round with nothing confirmed yet passes an empty file; the command tells the auditor so). **Pass the round as `--round `** — the CLI bakes it into the identity line and the record key, so two rounds are two receipts even when the findings list has not changed between them. **Paste what it prints verbatim — the whole block. Do not write a round label yourself**: hand-written labels and hand-written launches have each cost a repair round or a capped verdict (measured; DESIGN.md — The hand-written reverse-audit launches). The command records the exact block it prints — findings included, keyed per round's findings digest — so a launch that drops the confirmed list matches no record. It also gives each auditor its diff reads — the whole plan in 3A, one chunk's range in 3B (a Step 3B auditor handed the whole 5 800-line diff is the most context-starved agent in the pipeline, on exactly the PRs where the reverse audit matters most). In worktree mode its `working_dir` is the PR worktree. +**`--findings` is required for this role — the command refuses without it** (an early round with nothing confirmed yet passes an empty file; the command tells the auditor so). **Pass the round as `--round `** — the CLI bakes it into the identity line and the record key, so two rounds are two receipts even when the findings list has not changed between them. **Paste what it prints verbatim — the whole block. Do not write a round label yourself**: hand-written labels and hand-written launches have each cost a repair round or a capped verdict (measured; DESIGN.md — The hand-written reverse-audit launches). The command records the exact block it prints — findings included, keyed per round's findings digest — so a launch that drops the cumulative list matches no record. It also gives each auditor its diff reads — the whole plan in 3A, one chunk's range in 3B (a Step 3B auditor handed the whole 5 800-line diff is the most context-starved agent in the pipeline, on exactly the PRs where the reverse audit matters most). In worktree mode its `working_dir` is the PR worktree. The brief holds what the auditor is for: hunt only the **gaps** no prior agent caught, report only Critical or Suggestion, apply the Exclusion Criteria, and end with a substantive receipt (`No issues found — `) — a bare "No issues found." fails the substantive-return check below and triggers the one relaunch. @@ -616,16 +617,18 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - **The substantive-return check applies to every round** — the same rule as Step 3's, enforced here, after each round returns: a bare `No issues found.` with no evidence of what the agent re-examined is a whiff, not a clean bill. Relaunch that agent once, within the round. If the relaunch is also bare, do not spin — take it, but its scope counts as **not audited**: track it in an outstanding-whiffed-scopes list, and clear it only when a later round's agent for that scope returns substantively. - 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**. 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. -- Stop after **5 rounds** regardless (hard cap), and say so in the output rather than implying convergence. -- New findings from each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. -- **The round builder is also the loop's clock.** In a time-budgeted run (CI exports `QWEN_REVIEW_DEADLINE_EPOCH`; a local run normally has no deadline and is untouched), `agent-prompt --role reverse-audit` refuses to build a round that no longer fits: the remaining time must cover **the round itself** (estimated from the previous round's measured cost — the builder stamps each admission — or a conservative constant for round 1) **plus** the reserve kept for its verification, compose-review and submission. On refusal it prints a `BUDGET:` line to stderr and exits **4**. That refusal is a termination rule, not an error — do not rebuild the round, do not relaunch auditors, and do not retry the command. The builder also records a budget-stop marker that `compose-review` reads directly, so the verdict is capped whether or not you relay anything; still add the exact entry the message names (`reverse audit — stopped before round by the review time budget`) to `unreviewedDimensions` so the terminal report and the body agree, and proceed to Step 6 with the findings already confirmed — spending what remains only on verifying findings already in hand, composing, and submitting. Why this exists, measured: a +1699-line PR's CI review ran the audit loop to the 5-round cap, spent 3.5 of its 4 budgeted hours there, and was killed by the outer CI timeout while round 5's findings were still being verified — every confirmed finding died with it. A review that stops on the budget still reports everything it proved; one that runs past it reports nothing. +- 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. 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 round-5 convergence. (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 after **5 rounds** regardless (hard cap), and say so in the output rather than implying convergence. If round 5 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. +- 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 5-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 5-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 folded into the launch prompt and hashed into the record key, so no delivery check changes. +- **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. 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. +- **The round builder is also the loop's clock.** In a time-budgeted run (CI exports `QWEN_REVIEW_DEADLINE_EPOCH`; a local run normally has no deadline and is untouched), `agent-prompt --role reverse-audit` refuses to build a round that no longer fits: the remaining time must cover **the round itself** (estimated from the costliest round's measured cost so far — a repair relaunch can make one round the expensive one, and the gate prices the worst case the run has proved, not the newest dip — or a conservative constant for round 1) **plus** the reserve kept for its verification, compose-review and submission. On refusal it prints a `BUDGET:` line to stderr and exits **4**. That refusal is a termination rule, not an error — do not rebuild the round, do not relaunch auditors, and do not retry the command. The builder also records a budget-stop marker that `compose-review` reads directly, so the verdict is capped whether or not you relay anything; still add the exact entry the message names (`reverse audit — stopped before round by the review time budget`) to `unreviewedDimensions` so the terminal report and the body agree, and proceed to Step 6 with the findings already confirmed — spending what remains only on verifying findings already in hand, composing, and submitting. Why this exists, measured: a +1699-line PR's CI review ran the audit loop to the 5-round cap, spent 3.5 of its 4 budgeted hours there, and was killed by the outer CI timeout while round 5's findings were still being verified — every confirmed finding died with it. A review that stops on the budget still reports everything it proved; one that runs past it reports nothing. **Reverse audit findings go through Step 4 verification like any other finding.** They used to skip it on the theory that the auditor "already has full context." That premise fails exactly when the diff is large — the auditor with the least room to think was the one whose output nobody checked. If the very first round finds nothing, that is a good sign — but run the second round anyway before believing it. -All confirmed findings (from aggregation + all reverse audit rounds) proceed to Step 6. +All confirmed findings (from aggregation + all reverse audit rounds) proceed to Step 6. An entry still tagged `— [unverified]` when the loop ends is not among them: the final merge before Step 6 applies every verdict that landed, so a tag that survives means the verifier never ruled on that entry — relaunch it once, and if the tag still survives, add `reverse audit finding — the verifier never ruled on it` to `unreviewedDimensions` (which caps a would-be Approve at COMMENT) and treat that entry as low-confidence (terminal-only, "Needs Human Review"), never as confirmed. This is also machine-checked: Step 6 passes this file to `compose-review` as `findingsPath`, and any tag still in it there caps the verdict at Comment and says so in the body — a tag you forgot to exclude cannot ride an Approve or a Request changes out the door. ## Step 6: Present findings @@ -774,7 +777,7 @@ Each entry carries `id` (unique — outcomes and resolved anchors both join on i # description to pick the body language, and that gh call must hit the PR's host. ``` -It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every verified review — **high and medium** — whether or not you are going to post. The state file is the same one Step 7 uses (see there for every field): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the presubmit flags, the model id. It does **not** take the coverage or the inline counts, and it **refuses** a state JSON carrying `criticalsInline`/`suggestionsInline`. It derives coverage from the harness's transcripts, and it **counts** the inline findings from `--comments`: write the drafted inline comments to that file first — the same `[{path, line, body, …}]` array the Step 7 payload will carry, each body opening with its `**[Critical]**`/`**[Suggestion]**` marker; a review with nothing anchored inline passes a file containing `[]`. A report-only run has read Approve over a blocker its own report listed (measured; DESIGN.md — The Approve over a relocated Critical); counted from the draft, that finding cannot fall out of the computation. **If the comment set changes after composing** — an anchor fails to resolve, a finding relocates to the body, a comment is dropped — update the comments file (and the state), and run `compose-review` again: the verdict must be computed from the set you actually post, and Step 7's `submit` recounts from the payload to hold you to it. +It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every verified review — **high and medium** — whether or not you are going to post. The state file is the same one Step 7 uses (see there for every field): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the `findingsPath` (high effort — the cumulative reverse-audit findings file, for the `— [unverified]` check), the presubmit flags, the model id. It does **not** take the coverage or the inline counts, and it **refuses** a state JSON carrying `criticalsInline`/`suggestionsInline`. It derives coverage from the harness's transcripts, and it **counts** the inline findings from `--comments`: write the drafted inline comments to that file first — the same `[{path, line, body, …}]` array the Step 7 payload will carry, each body opening with its `**[Critical]**`/`**[Suggestion]**` marker; a review with nothing anchored inline passes a file containing `[]`. A report-only run has read Approve over a blocker its own report listed (measured; DESIGN.md — The Approve over a relocated Critical); counted from the draft, that finding cannot fall out of the computation. **If the comment set changes after composing** — an anchor fails to resolve, a finding relocates to the body, a comment is dropped — update the comments file (and the state), and run `compose-review` again: the verdict must be computed from the set you actually post, and Step 7's `submit` recounts from the payload to hold you to it. **It also proves Step 4 and Step 5 ran — the way `check-coverage` proves Step 3.** `check-coverage` runs at Step 3D, before verify and reverse audit exist, so its roster cannot reach them; and their count is not in the plan (verify shards on the finding count, the reverse audit loops until it goes dry), so there is no exact roster to check. What there is is a floor, and `compose-review` — which runs at **high and medium** effort — checks it from the same transcripts: at least one **verifier** ran and opened its brief (whenever the review posts findings), and, **at high effort**, at least one **reverse auditor** did. A **medium** review runs no reverse audit by design, so that floor is legitimately unmet and `compose-review` caps a would-be Approve to **Comment** — the honest ceiling for a balanced pass that never looked twice for what Step 3 missed; a verified Critical still yields **Request changes**, so medium flags real blockers, it just never certifies Approve (only high does). At high effort a reverse audit **skipped wholesale**, or run with agents that never opened their brief, is named in `unreviewedDimensions` and caps the verdict, exactly like a dimension nobody reviewed. You do not pass a flag for this and cannot turn it off: the proof is the intersection of the prompt the CLI recorded building (`--role verify` / `--role reverse-audit`) and the harness's transcript of an agent that ran it. So a run cannot approve a diff by skipping the pass that looks for what Step 3 missed — the highest-value catch here is a clean, zero-finding review that never ran its reverse audit. @@ -1041,6 +1044,7 @@ Then reference each finding's `assets` URLs in its inline comment body as `![evi - `suggestionsDiscarded` — Suggestions whose anchors failed offline validation or the 422 recovery. They still count toward `S`: dropping every anchor must never upgrade the verdict. - `cannotTellCriticals` — one line per existing PR Critical whose Step 6 re-check landed on `cannot tell` (location + what could not be determined). - `planPath` — the plan report from Step 1. **Coverage is not an input.** `submit` recomputes it from the harness's transcripts, because a `coverage` object you typed is a document you write — and the last time this skill trusted one, it was fabricated. +- `findingsPath` — the cumulative reverse-audit findings file at loop end (high effort only): the same file every round's `--findings` received, after the final merge. `compose-review` reads it for surviving `— [unverified]` tags — a tag at compose time is an entry no verifier ruled on, and it caps the verdict at Comment, disclosed in the body. Omit at medium and low; they run no Step 5. - `uncoverableChunks` / `unreviewedDimensions` — any _additional_ not-reviewed scope from Step 3 (e.g. `"chunk 5 (src/big.min.js)"`, `"security"`). A bare dimension name gets the standard whiffed-agent explanation; an entry carrying its own reason after an em-dash (`"issue-fidelity — linked issue #123 could not be fetched"`) is rendered verbatim. - `contextUnavailable` — the Step 1 state. - `presubmit` — `downgradeApprove` / `downgradeRequestChanges` / `downgradeReasons` from the presubmit report. Do not apply a downgrade by hand; hand it over and let `submit` own the semantics (a Suggestion-only review is already `COMMENT`, so nothing is downgraded and no "downgraded from Approve" sentence is emitted).