diff --git a/integration-tests/cli/acp-integration.test.ts b/integration-tests/cli/acp-integration.test.ts index 753bc5afb4c..28b8632356d 100644 --- a/integration-tests/cli/acp-integration.test.ts +++ b/integration-tests/cli/acp-integration.test.ts @@ -723,20 +723,26 @@ function setupAcpTest( // Track which permission requests we've seen const planModeRequests: PermissionRequest[] = []; - const { sendRequest, cleanup, stderr, sessionUpdates, permissionRequests, agent } = - setupAcpTest(rig, { - permissionHandler: (request) => { - // Track all permission requests for later verification - // Auto-approve exit plan mode requests with "proceed_always" to trigger auto-edit mode - if (request.toolCall?.kind === 'switch_mode') { - planModeRequests.push(request); - // Return proceed_always to switch to auto-edit mode - return { optionId: 'proceed_always' }; - } - // Auto-approve all other requests - return { optionId: 'proceed_once' }; - }, - }); + const { + sendRequest, + cleanup, + stderr, + sessionUpdates, + permissionRequests, + agent, + } = setupAcpTest(rig, { + permissionHandler: (request) => { + // Track all permission requests for later verification + // Auto-approve exit plan mode requests with "proceed_always" to trigger auto-edit mode + if (request.toolCall?.kind === 'switch_mode') { + planModeRequests.push(request); + // Return proceed_always to switch to auto-edit mode + return { optionId: 'proceed_always' }; + } + // Auto-approve all other requests + return { optionId: 'proceed_once' }; + }, + }); try { // Initialize diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 7c1e513ec4a..402d1a836de 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -45,6 +45,7 @@ import { DEADLINE_ENV, RESERVE_ENV, COMPOSE_FLOOR_ENV, + TOOL_CONCURRENCY_ENV, readBudgetStop, readRoundStamps, } from './lib/deadline.js'; @@ -2658,6 +2659,7 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () afterEach(() => { delete process.env[DEADLINE_ENV]; delete process.env[RESERVE_ENV]; + delete process.env[TOOL_CONCURRENCY_ENV]; process.exitCode = undefined; for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); }); @@ -3103,6 +3105,59 @@ describe('the reverse-audit budget gate — the loop must end by reporting', () // A refusal is not an admission. expect(readRoundStamps(plan)).toHaveLength(1); }); + + it('prices the 3B pair as one admission — round 2 bears the pair wall', () => { + // Round 2's build lands seconds after round 1's stamp, so nothing has + // measured a round yet; the price is both members' wall in waves of the + // tool-concurrency pool. PLAN has three chunks; at a 2-slot pool each + // round runs two waves and the pair three, so round 2 pays 3/2 of the + // round estimate — and the gate refuses it when the reserve plus that + // does not fit, even though round 1 (one estimate) just admitted. + process.env[TOOL_CONCURRENCY_ENV] = '2'; + process.env[RESERVE_ENV] = '600'; + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3000); + const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); + expect(process.exitCode).toBeUndefined(); + expect(readRoundStamps(plan).some((st) => st.round === 1)).toBe(true); + + (writeStdoutLine as unknown as Mock).mockClear(); + call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); + // Reserve 600 + pair price 2700 = 3300 > the 3000 remaining. + expect(process.exitCode).toBe(4); + expect((writeStdoutLine as unknown as Mock).mock.calls).toHaveLength(0); + expect(readBudgetStop(plan)?.entry).toBe( + 'reverse audit — stopped before round 2 by the review time budget', + ); + expect(readRoundStamps(plan)).toHaveLength(1); + }); + + it('admits the 3B pair when the reserve plus the pair wall fits', () => { + process.env[TOOL_CONCURRENCY_ENV] = '2'; + process.env[RESERVE_ENV] = '600'; + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3400); + const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); + expect(process.exitCode).toBeUndefined(); + (writeStdoutLine as unknown as Mock).mockClear(); + call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); + expect(process.exitCode).toBeUndefined(); + expect(readRoundStamps(plan).map((st) => st.round)).toEqual([1, 2]); + expect(readBudgetStop(plan)).toBeNull(); + }); + + it('prices the pair at one round when the pool holds both fan-outs at once', () => { + // The default 10-slot pool holds all six auditors of PLAN's 3-chunk + // pair in one wave, so round 2 pays one round estimate — a flat 2x + // price would refuse this admission (reserve 600 + 3600 > 3000) and + // gut the pair's admission win near the deadline. + process.env[RESERVE_ENV] = '600'; + process.env[DEADLINE_ENV] = String(Math.floor(Date.now() / 1000) + 3000); + const plan = call('reverse-audit', { 'all-chunks': true, round: 1 }); + expect(process.exitCode).toBeUndefined(); + (writeStdoutLine as unknown as Mock).mockClear(); + call('reverse-audit', { 'all-chunks': true, round: 2 }, plan); + expect(process.exitCode).toBeUndefined(); + expect(readRoundStamps(plan).map((st) => st.round)).toEqual([1, 2]); + }); }); describe('per-chunk retirement — cold territories stop costing a round', () => { @@ -3310,6 +3365,30 @@ describe('per-chunk retirement — cold territories stop costing a round', () => expect(keysOf(2)).toHaveLength(3); }); + it('the 3B pair: round 2 builds every chunk with round 1 still in flight (no round-1 transcripts)', () => { + // The convergence pair on 3B — the latency lever: rounds 1 and 2 are + // launched together, so round 2's builder runs BEFORE round 1's auditors + // have returned any transcript. Round 2 must still fan out to every chunk + // (the retirement schedule only reads history at k >= 3, so nothing here + // depends on round 1's records existing) and stamp its own admission, so + // the two rounds' auditors run concurrently instead of one round-wall + // apart. Pins the mechanism the SKILL 3B-pair orchestration relies on. + const r1 = runRound(1); // built, but no transcripts written for it + expect(r1).toContain('3 auditors required this round — one per chunk.'); + const r2 = runRound(2); // round 1's transcripts don't exist yet at this point + expect(r2).toContain('3 auditors required this round — one per chunk.'); + expect(r2).not.toContain('retirement:'); + expect(keysOf(1)).toHaveLength(3); + expect(keysOf(2)).toHaveLength(3); + // Both admissions are stamped, so the deadline gate prices each and the + // clock advances a round per stamp. + const rounds = readRoundStamps(plan) + .map((s) => s.round) + .sort(); + expect(rounds).toContain(1); + expect(rounds).toContain(2); + }); + 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 }); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index e6744a39110..69669cf497f 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -45,7 +45,7 @@ import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; import { clearBudgetStop, - expectedRoundSeconds, + expectedAdmissionSeconds, readRoundStamps, reverseAuditBudgetExhausted, reverseAuditBudgetMessage, @@ -1862,12 +1862,17 @@ function requireAuditableChunks(report: PlanReport): DiffChunk[] { * 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. + * throws must not leave one behind. `fanOutWidth` is the auditors this + * round fans out (1 for a whole-diff round): when the previous round is + * still in flight — the convergence pair's second member — the price + * covers both members' wall in waves of the tool-concurrency pool, not + * just this round's (deadline.ts `expectedAdmissionSeconds`). */ function admitReverseAuditRound( planPath: string, round: number | undefined, cap: number, + fanOutWidth: number, ): boolean { // The plan's round cap first: deterministic, and cheaper than the // deadline arithmetic. The full cap normally; a reduced cap for a huge @@ -1900,7 +1905,7 @@ function admitReverseAuditRound( } const spent = reverseAuditBudgetExhausted( process.env, - expectedRoundSeconds(planPath, round), + expectedAdmissionSeconds(planPath, round, fanOutWidth, process.env), ); if (spent !== null) { writeBudgetStop(planPath, spent, round); @@ -2009,6 +2014,7 @@ function runAllChunks( planPath, round, reverseAuditRoundCap(report.budget), + chunks.length, ) ) { return; @@ -2403,7 +2409,9 @@ function runAgentPrompt(args: AgentPromptArgs): void { // admits on the reserve alone hands the terminal round a start right at // the boundary, which is the killed-mid-verification failure one round // wide. The round's cost is the previous round's, measured admission to - // admission. The admission is stamped AFTER the build succeeds (below), + // admission — except when this round launches with the previous one still + // in flight (the convergence pair), where it covers both. The admission is + // stamped AFTER the build succeeds (below), // never here: the stamp is what the next round's gate measures cost from, // 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, @@ -2421,6 +2429,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { args.plan, args.round, reverseAuditRoundCap(report.budget), + 1, ) ) { return; @@ -2471,14 +2480,17 @@ function runAgentPrompt(args: AgentPromptArgs): void { hasChunk && !readRoundStamps(args.plan).some((s) => s.round === (args.round ?? null)) ) { + const planChunkIds = ( + Array.isArray(report.chunks) ? (report.chunks as DiffChunk[]) : [] + ) + .map((c) => c?.id) + .filter((id): id is number => typeof id === 'number'); 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'), + planChunkIds, args.round, process.env, typeof report.diffPathAbsolute === 'string' @@ -2500,6 +2512,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { args.plan, args.round, reverseAuditRoundCap(report.budget), + planChunkIds.length, ) ) return; diff --git a/packages/cli/src/commands/review/lib/deadline.test.ts b/packages/cli/src/commands/review/lib/deadline.test.ts index 0ca849f088d..d9f9d19f76e 100644 --- a/packages/cli/src/commands/review/lib/deadline.test.ts +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -23,9 +23,12 @@ import { DEFAULT_RESERVE_SECONDS, DEFAULT_ROUND_SECONDS, DEFAULT_COMPOSE_FLOOR_SECONDS, + DEFAULT_TOOL_CONCURRENCY, + TOOL_CONCURRENCY_ENV, budgetStopEntry, budgetStopEntryZh, clearBudgetStop, + expectedAdmissionSeconds, expectedRoundSeconds, readBudgetStop, readRoundStamps, @@ -282,6 +285,117 @@ describe('the round-cost estimate — measured when it can be', () => { }); }); +describe('the pair admission price — a round launched beside an in-flight round pays for both', () => { + // The convergence pair's second member is built seconds after the first's + // stamp, so nothing has measured a round yet. Pricing it off that + // seconds-old span committed the pair at one round's price for up to two + // rounds' wall — these pin the wave-priced pair instead. + const dirs: string[] = []; + afterEach(() => { + for (const d of dirs.splice(0)) rmSync(d, { recursive: true, force: true }); + }); + function plan(): string { + const dir = mkdtempSync(join(tmpdir(), 'deadline-pair-')); + dirs.push(dir); + const p = join(dir, 'plan.json'); + writeFileSync(p, '{}'); + backdatePlan(p); + return p; + } + + it('prices a round with no in-flight predecessor like expectedRoundSeconds', () => { + const p = plan(); + expect(expectedAdmissionSeconds(p, 1, 6, {}, NOW_MS)).toBe( + DEFAULT_ROUND_SECONDS, + ); + stampRound(p, 1, NOW_MS - 2_400_000); // round 1 returned 40 min ago + expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe( + expectedRoundSeconds(p, 2, NOW_MS), + ); + expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe(2400); + }); + + it('prices the pair at both members when the pool serializes them', () => { + // Six chunks on the default 10-slot pool: one wave per round, two + // waves for the pair — the seconds-old round-1 stamp has measured + // nothing, so the price is 2x the round estimate, not the floor. + const p = plan(); + stampRound(p, 1, NOW_MS - 30_000); + expect(expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS)).toBe( + 2 * DEFAULT_ROUND_SECONDS, + ); + }); + + it('prices the pair at one round when the pool holds both members at once', () => { + // Three chunks on ten slots: both members fit in a single wave, and + // the pair's wall is one round's — the 3A shape reads the same (width + // 1 on any pool of two or more). + const p = plan(); + stampRound(p, 1, NOW_MS - 30_000); + expect(expectedAdmissionSeconds(p, 2, 3, {}, NOW_MS)).toBe( + DEFAULT_ROUND_SECONDS, + ); + expect(expectedAdmissionSeconds(p, 2, 1, {}, NOW_MS)).toBe( + DEFAULT_ROUND_SECONDS, + ); + }); + + it('reads the pool from the tool-concurrency env, like the scheduler', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 30_000); + // A 12-slot pool holds all twelve auditors of a 6-chunk pair in one + // wave. + expect( + expectedAdmissionSeconds( + p, + 2, + 6, + { [TOOL_CONCURRENCY_ENV]: '12' }, + NOW_MS, + ), + ).toBe(DEFAULT_ROUND_SECONDS); + // A 3-slot pool runs a 6-chunk round in two waves and the pair in + // four — two rounds' price again. + expect( + expectedAdmissionSeconds( + p, + 2, + 6, + { [TOOL_CONCURRENCY_ENV]: '3' }, + NOW_MS, + ), + ).toBe(2 * DEFAULT_ROUND_SECONDS); + // Malformed falls back to the default pool, never to a wedge. + expect( + expectedAdmissionSeconds( + p, + 2, + 6, + { [TOOL_CONCURRENCY_ENV]: 'soon' }, + NOW_MS, + ), + ).toBe( + Math.ceil( + (DEFAULT_ROUND_SECONDS * Math.ceil(12 / DEFAULT_TOOL_CONCURRENCY)) / + Math.ceil(6 / DEFAULT_TOOL_CONCURRENCY), + ), + ); + }); + + it('keeps the reserve on top of the pair price at the refusal boundary', () => { + const p = plan(); + stampRound(p, 1, NOW_MS - 30_000); + const price = expectedAdmissionSeconds(p, 2, 6, {}, NOW_MS); + expect(price).toBe(2 * DEFAULT_ROUND_SECONDS); + const env = { + [DEADLINE_ENV]: String(NOW_S + DEFAULT_RESERVE_SECONDS + price), + }; + expect(reverseAuditBudgetExhausted(env, price, NOW_MS)).toBeNull(); + env[DEADLINE_ENV] = String(NOW_S + DEFAULT_RESERVE_SECONDS + price - 1); + expect(reverseAuditBudgetExhausted(env, price, NOW_MS)).not.toBeNull(); + }); +}); + describe('the budget-stop marker — the deterministic half of the disclosure', () => { const dirs: string[] = []; afterEach(() => { diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index 8bb1ee5c794..8215f65a297 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -47,6 +47,7 @@ import { writeFileSync, } from 'node:fs'; import { join } from 'node:path'; +import { parsePositiveIntegerEnv } from '@qwen-code/qwen-code-core'; import { promptRecordDir } from './prompt-record.js'; /** Unix seconds at which the review process will be killed. Set by CI. */ @@ -138,6 +139,16 @@ export const DEFAULT_ROUND_SECONDS = 1800; /** Floor for an observed round cost — a quick same-round rebuild is not a round. */ const MIN_OBSERVED_ROUND_SECONDS = 600; +/** + * The runtime's concurrent-agent slots — the pool every fan-out launch + * shares. The core tool scheduler runs the orchestrator's parallel `agent` + * calls under this cap (default 10), the review workflow does not override + * it, and an `agent-prompt` subprocess inherits the orchestrator's + * environment — so the gate and the launches it gates read the same pool. + */ +export const TOOL_CONCURRENCY_ENV = 'QWEN_CODE_MAX_TOOL_CONCURRENCY'; +export const DEFAULT_TOOL_CONCURRENCY = 10; + interface RoundStamp { round: number | null; atMs: number; @@ -226,6 +237,27 @@ export function stampRound( } } +/** + * The costliest of `stamps`' admission-to-admission spans — each span ends + * at the next stamp, the last at `endMs` — floored at the observation + * floor; `null` when there are no stamps. + */ +function costliestSpanSeconds( + stamps: RoundStamp[], + endMs: number, +): number | null { + if (stamps.length === 0) return null; + let maxSeconds = 0; + for (let i = 0; i < stamps.length; i++) { + const end = i + 1 < stamps.length ? stamps[i + 1].atMs : endMs; + maxSeconds = Math.max( + maxSeconds, + Math.round((end - stamps[i].atMs) / 1000), + ); + } + return Math.max(MIN_OBSERVED_ROUND_SECONDS, maxSeconds); +} + /** * What the round about to be admitted is expected to cost, in seconds: the * COSTLIEST round the run has measured (admission-to-admission — its audit @@ -249,16 +281,70 @@ export function expectedRoundSeconds( 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 costliestSpanSeconds(stamps, nowMs) ?? DEFAULT_ROUND_SECONDS; +} + +/** + * What the ADMISSION itself commits, in seconds — `expectedRoundSeconds`, + * except when the round being admitted launches while its predecessor is + * still in flight: the convergence pair's second member, built in the same + * response as the first. The predecessor's stamp is fresher than the + * observation floor — nothing has measured the round yet, and no elapsed + * time has paid for it — so the admission must cover BOTH members' wall, + * not just its own. That wall is the pair's two fan-outs sharing the + * tool-concurrency pool: ceil(2C/N) waves against one round's ceil(C/N), + * for C auditors on a pool of N, and the first never exceeds twice the + * second — so the price is the single-round estimate scaled by exactly + * those waves: one round's price when the pool holds both members at once + * (the 3A shape, and a 3B pair whose chunks fit), more as the pool + * serializes them, and never beyond the two-round bound whatever the pool. + * Pricing the second member off the just-written first stamp instead — a + * seconds-old span clamped to the floor — committed the pair at one + * round's price for up to two rounds' wall, and near the deadline the + * pair consumed the reserve and hit the outer timeout before posting. + * + * The price deliberately covers the pair's AUDITOR fan-outs only: the pair + * launches in the same response as the Step 4 verifier shards, which share + * the same pool waves, and if they stretch the batch past the priced waves + * the extra wall is bounded by the verifier batch's own wave count — one + * wave for any normal finding set — which the reserve the gate holds ahead + * of every admission is there to carry. + * + * One ledger shape the price does not correct: the pair stamps rounds 1 + * and 2 seconds apart, so after the pair returns, the span from round 2's + * stamp to the next admission covers the pair's whole wall, and every solo + * round after it prices at up to twice its true cost. Accepted + * conservatism: an over-priced gate refuses a round near the deadline that + * would have fit — a capped verdict that still posts — never the + * killed-before-compose shape the gate exists to prevent. + */ +export function expectedAdmissionSeconds( + planPath: string, + round: number | undefined, + fanOutWidth: number, + env: NodeJS.ProcessEnv, + nowMs: number = Date.now(), +): number { + const stamps = readRoundStamps(planPath).filter( + (s) => round === undefined || s.round !== round, + ); + const last = stamps.length > 0 ? stamps[stamps.length - 1] : undefined; + const predecessorInFlight = + last !== undefined && nowMs - last.atMs < MIN_OBSERVED_ROUND_SECONDS * 1000; + if (!predecessorInFlight) { + return expectedRoundSeconds(planPath, round, nowMs); } - return Math.max(MIN_OBSERVED_ROUND_SECONDS, maxSeconds); + const single = + costliestSpanSeconds(stamps.slice(0, -1), last.atMs) ?? + DEFAULT_ROUND_SECONDS; + const pool = parsePositiveIntegerEnv( + env[TOOL_CONCURRENCY_ENV], + DEFAULT_TOOL_CONCURRENCY, + ); + const width = Math.max(1, Math.floor(fanOutWidth)); + const pairWaves = Math.ceil((2 * width) / pool); + const roundWaves = Math.ceil(width / pool); + return Math.ceil((single * pairWaves) / roundWaves); } export interface BudgetExhausted { diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index c6127602f0b..46ed460d9c5 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -921,6 +921,10 @@ Two CI reviews of similar-size PRs ran the same skill on the same day (2026-08-0 Measured on the CI reviews of #8619 and #8607: both audits converged at the minimum — round 1 dry, round 2 dry — and the rounds ran serially at 13–25 minutes each, although a dry round leaves the cumulative findings list unchanged, so round 2's launch input was substantively identical to round 1's — the same entries, at most with verification tags the unconditional merge had cleared in between: an independent rerun, paid for at the price of a dependent one. The #8501 round-5 review made the cost concrete: round 1 came back dry, the deadline gate then refused round 2 (`BUDGET:`, exit 4), and the verdict shipped capped by a budget stop — for want of a second dry audit the run had time to launch in parallel but not in series. +### The serial 3B convergence rounds + +Two v0.21.9 CI reviews of large chunked PRs spent 77–80% of their wall clock inside the reverse-audit loop, not the fan-out. A 291-minute review ran its 28-agent fan-out in 63 minutes (22%) and then three serial reverse-audit rounds in 223 (round boundaries measured at +65, +134, +190 min); a 252-minute review ran six serial rounds of ~30–37 minutes each. On 3B the rounds ran one at a time because the convergence pair — rounds 1 and 2 launched together, which the 3A path already uses to collapse two serial rounds into one wall — was 3A-only. Its arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — the independent-rerun-paid-as-dependent shape the 3A pair removes, present on every chunk. Pairing rounds 1 and 2 on 3B launches each chunk's two establishing auditors together, saving one round's wall (~30–56 minutes measured) off every chunked review, at the same one-round suppression window the 3A pair and the pipelined loop already accept. The saving is bounded by the agent pool's concurrency: where the pool holds both rounds' auditors it is a full round, and where it does not the doubled launch is still never worse than the two serial rounds it replaces — ceil(2C/N) waves against the serial shape's 2·ceil(C/N), for C chunks on an N-slot pool, and the first never exceeds the second. The deadline gate prices the pair by the same waves: a round-2 build admitted while round 1 is still in flight pays both members' wall, so near the deadline the pair is refused as one unit and degrades to round 1 alone, instead of being committed at one round's price for up to two rounds' wall. The pair leaves one conservative mark on the ledger: rounds 1 and 2 are stamped seconds apart at the pair's start, so after the pair returns, the span from round 2's stamp to the next admission covers the pair's whole wall, and every solo round after it prices at up to twice its true cost — near the deadline the loop can stop a round earlier than the serial shape would have. Accepted: an over-priced gate refuses a round that would have fit — a capped verdict that posts — never the killed-before-compose shape it exists to prevent. + ### The rounds a rejected finding bought (PR #8353) The 15th review round of #8353 (its audit rounds numbered 1–5 within that run; `R15-1` is the incremental-review ledger's naming, not an audit round): audit round 2 dry; round 3's sole finding rejected by its verifier with direct counter-evidence — the claimed compound behavior lived entirely in unchanged code. The rejection removed the entry from the cumulative list, but not the reset it had already applied to the dry counter. Under the forward pairing the rule licenses, round 4's dry return completed the two-dry evidence the moment it landed — the retired round 3 plus dry round 4 — and round 5 (~15–20 minutes) was the waste: it audited nothing the loop had not already answered. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index df6b39ee42f..ed9b1193ccc 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -563,7 +563,7 @@ Before verification, merge findings that refer to the same issue (same file, sam Launch verification agents that between them receive **all** non-pre-confirmed findings. **Up to `plan.budget.verifyShard` findings per agent** (8), so `ceil(N / verifyShard)` agents, launched together in one response. It is flat rather than size-derived on purpose: it is a fact about how much a verifier can re-trace before its quality collapses on the tail of its list, which is a property of the verifier and not of the diff. It lives in the budget so it has one home instead of being restated here and in whatever reads it. -**At high effort, the verifiers do not launch alone.** Step 5's first reverse-audit launch — the convergence pair on a 3A plan, round 1's per-chunk fan-out on 3B — goes out **in the same response** as these verifier shards, exactly as every later round's verification rides alongside the next round's auditors (Step 5's pipelined loop; this is its k=0 case). The batch is self-contained: write the shard files **and the cumulative findings file** (Step 5 defines its form — every entry **not yet through Step 4** carries the `— [unverified]` tag; a pre-confirmed `[build]`/`[test]` entry is already through it and enters untagged, exactly as the Step 4 close-out line says) first, then build both prompt sets from them, then fire every agent together. Nothing here waits on a verdict: the tagged state is exactly what Step 5's merge rules are built around. A real run has held its round-1 auditor 22 minutes behind a verifier whose verdicts that auditor never needed, while a sibling run of the same skill, the same day, launched the two together (measured; DESIGN.md — The 22-minute serial first verification). At medium there is no reverse audit, so the verifiers launch alone; a Step 4 with no shards — zero findings, or only pre-confirmed ones — has no verifiers, so the first reverse-audit launch goes out alone, on time, its findings file carrying whatever entries exist (empty is fine; the builder accepts it and tells the auditor so). +**At high effort, the verifiers do not launch alone.** Step 5's first reverse-audit launch — the convergence pair, whole-diff on a 3A plan and per-chunk (rounds 1 and 2 together) on 3B — goes out **in the same response** as these verifier shards, exactly as every later round's verification rides alongside the next round's auditors (Step 5's pipelined loop; this is its k=0 case). The batch is self-contained: write the shard files **and the cumulative findings file** (Step 5 defines its form — every entry **not yet through Step 4** carries the `— [unverified]` tag; a pre-confirmed `[build]`/`[test]` entry is already through it and enters untagged, exactly as the Step 4 close-out line says) first, then build both prompt sets from them, then fire every agent together. Nothing here waits on a verdict: the tagged state is exactly what Step 5's merge rules are built around. A real run has held its round-1 auditor 22 minutes behind a verifier whose verdicts that auditor never needed, while a sibling run of the same skill, the same day, launched the two together (measured; DESIGN.md — The 22-minute serial first verification). At medium there is no reverse audit, so the verifiers launch alone; a Step 4 with no shards — zero findings, or only pre-confirmed ones — has no verifiers, so the first reverse-audit launch goes out alone, on time, its findings file carrying whatever entries exist (empty is fine; the builder accepts it and tells the auditor so). A single verifier for every finding was cheaper, but on a large review it becomes the most context-starved agent in the pipeline: it must re-read code for each of 30-60 findings inside one context window, and its quality collapses on the tail of the list. Sharding keeps each verifier's job small; the cost is still far below one-agent-per-finding. @@ -622,17 +622,20 @@ After deduplication, run reverse audit **iteratively** — the first launch ride **Each round is a fan-out, not one agent.** - **Small diffs (Step 3A path):** one reverse audit agent per round, reading the whole diff — except rounds 1 and 2, which are **the convergence pair** and launch together (below). -- **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). +- **Large diffs (Step 3B path):** one reverse audit agent **per chunk** per round, launched together in a single response — and rounds 1 and 2 are **the convergence pair** here too, their per-chunk auditors launched together (below). 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 the odd rounds to shrink, not the even ones (under the 3-round huge-diff cap only round 3 can shrink — the cap ends the loop before round 5). 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. -**The convergence pair (3A only).** Rounds 1 and 2 launch **in one response** — together with Step 4's verifier shards (Step 4 names this) — each built by its own `agent-prompt` call: `--round 1` and `--round 2`, the **same** `--findings` file. This is not a loosened criterion; it is the serial shape's own arithmetic made concurrent: a dry round leaves the cumulative list unchanged, so round 2's launch input was already substantively identical to round 1's — the same entries, at most with verification tags the merge had cleared in between — an independent rerun that the serial shape bought with a full round of wall clock, and that one budget-gated run could no longer afford at all, shipping a capped verdict for want of a second dry audit it had time to run in parallel but not in series (measured; DESIGN.md — The serial convergence pair). What the two-consecutive-dry criterion demands is unchanged: two independent, substantively-dry audits of the whole diff. The one delta the pair does introduce is the same one-round suppression window the pipelined loop already accepts (the merge bullet in the termination rules): the round-2 member audits with entries a verifier may be rejecting mid-flight still on its do-not-re-report list. +**The convergence pair — 3A (whole-diff form).** Rounds 1 and 2 launch **in one response** — together with Step 4's verifier shards (Step 4 names this) — each built by its own `agent-prompt` call: `--round 1` and `--round 2`, the **same** `--findings` file. This is not a loosened criterion; it is the serial shape's own arithmetic made concurrent: a dry round leaves the cumulative list unchanged, so round 2's launch input was already substantively identical to round 1's — the same entries, at most with verification tags the merge had cleared in between — an independent rerun that the serial shape bought with a full round of wall clock, and that one budget-gated run could no longer afford at all, shipping a capped verdict for want of a second dry audit it had time to run in parallel but not in series (measured; DESIGN.md — The serial convergence pair). What the two-consecutive-dry criterion demands is unchanged: two independent, substantively-dry audits of the whole diff. The one delta the pair does introduce is the same one-round suppression window the pipelined loop already accepts (the merge bullet in the termination rules): the round-2 member audits with entries a verifier may be rejecting mid-flight still on its do-not-re-report list. - **Both members dry** (substantive receipts, per the termination rules): the audit has converged. Wait for the riding verifiers' verdicts, apply the final merge, and proceed to Step 6. - **Either member reports findings**: the pair is one reporting round. Its members could not see each other, so first dedup the pair against itself (same defect, same location, same root cause keeps one, at the highest severity), merge into the cumulative list, and continue serially: the pair's verifiers ride with round 3's auditor — verify builds over the **deduped union**, sharded per Step 4's `verifyShard` exactly as any reporting round's findings are, **every shard passed as `--round 2`** (the pair's later label; never one build per member — the dedup already merged cross-member findings, and a per-member split would put one entry in front of two verifiers) — and convergence now needs two consecutive dry rounds from round 3 on. A dry member of a reporting pair is **not** carried forward as half of that evidence — its dry predates the other member's findings entering the list. One exception, and it is the retroactively-dry rule below, not a third rule: if a later merge retires the pair in full — every finding from both members rejected — the pair counts as the dry predecessor, and round 3's dry return ends the loop. - The substantive-return check applies per member, relaunch-once included. A twice-whiffed member makes the pair not dry — silence is not convergence evidence — and its scope joins the outstanding-whiffed-scopes list exactly as for any round. -- If the round-2 build is refused by the deadline gate (exit 4), launch round 1 alone and treat the refusal as the budget stop it is (the termination rules below). Defensive only: under the gate's pricing a paired round 2 admits strictly cheaper than the round 1 just admitted, so this cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. +- If the deadline gate refuses one of the pair's builds (exit 4) and admits the other, launch the admitted member alone and treat the refusal as the budget stop it is (the termination rules below). If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). The single-refusal split is defensive only: while the runtime's tool-concurrency pool holds both whole-diff members at once, the gate prices the paired round 2 at one round's wall, so it admits no dearer than the round 1 just admitted and that split cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. -On 3B the pair does not apply: rounds already fan out per chunk, rounds 1 and 2 are what establishes each chunk's record, and the retirement schedule is the convergence ledger there. What 3B shares is the launch coupling: its round 1 also rides with the Step 4 verifiers. +**The convergence pair — 3B (per-chunk form).** On 3B the pair applies per chunk. Launch `--all-chunks --round 1` **and** `--all-chunks --round 2` **in the same response** — both fan out to every chunk (rounds 1 and 2 always do, and the retirement schedule only reads history from round 3, so round 2's build needs nothing round 1 has produced yet), so each chunk's two establishing audits run concurrently instead of a round-wall apart. This is the same arithmetic as 3A read per territory: a chunk dry in round 1 leaves its slice of the cumulative list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit — one round's wall the serial shape paid on every chunked review (measured; DESIGN.md — The serial 3B convergence rounds). The convergence contract is unchanged and reads per chunk through the retirement ledger: a chunk dry in both members holds its two-consecutive-dry certificate, and a pair dry on **every** chunk converges at the round-3 `--all-chunks` build (`CONVERGED`, exit 5) exactly as an all-dry pair does on 3A. Same one-round suppression window, per chunk (a round-2 auditor audits with entries a verifier may be clearing mid-flight). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). + +- **Any auditor in either member reports findings**: the pair is one reporting round, exactly as on 3A — wait for BOTH fan-outs to return in full before the dedup (every chunk has an auditor in each member, and members cannot see each other across rounds either), dedup the pair against itself across rounds **and** chunks (same defect, same location, same root cause keeps one, at the highest severity), and merge the union into the cumulative list once. The pair's verifiers ride round 3's `--all-chunks` build: one batch over the **deduped union**, sharded per Step 4's `verifyShard`, **every shard passed as `--round 2`** (the pair's later label — never one build per member). Round 2's auditors are already in flight when round 1's returns land, so the pipelined k/k+1 rule below does not launch them again; this bullet is the pair's only transition. Convergence then reads per chunk through the retirement ledger as above: a chunk that reported in either member holds no certificate and stays under every-round audit, and the pair counts as one reporting round for the retroactively-dry rule — retired only when every finding from **both** members is rejected. +- If the deadline gate refuses one member's `--all-chunks` build (exit 4) and admits the other's, launch the admitted member alone and take the stop. The gate prices the round-2 build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so this split fires exactly when the pair plus the reserve does not fit but one round still does, and the admitted round alone keeps the serial shape. If it refuses BOTH builds, nothing launches: the remaining budget cannot cover even one round plus the reserve, the first refusal's stop marker is the stop, and the two refusals each name their own round's stop entry — proceed to Step 6 and relay the MARKER's entry only (it holds the first refusal, and it is the one `compose-review` renders). **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** @@ -648,6 +651,9 @@ Write **the cumulative list of every finding reported so far** (Steps 3-4 plus a [--rules ] # Step 3B (large diff): one auditor PER CHUNK per round — ONE call builds them all. +# The convergence pair is two of these builds — --round 1 and --round 2, same +# --findings — launched together in one response, each redirected to its own +# round file (the in the redirect names them apart). "${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan --role reverse-audit --all-chunks \ --findings \ --round \ @@ -671,12 +677,12 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - Stop at the plan's **`reverseAuditRounds` cap** — 5, or 3 for a huge diff (effective ≥ 3000 lines) — and say so in the output rather than implying convergence. The builder enforces this itself: a round past the cap gets a `ROUND CAP:` refusal on stderr and exit **4**, and — like the time-budget gate — writes a marker `compose-review` caps the verdict on whether or not you relay anything; still add the entry the message names to `unreviewedDimensions` so the terminal report agrees. If the cap round reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so verify them before Step 6 through `agent-prompt --role verify` **only** (never a hand-rolled agent), under the same bounded tail as the budget stop below: that builder is gated on the compose floor and refuses once too little time remains, and when the deadline is within the floor you stop waiting on any verifier batch still out and compose with the tags in hand — no fresh re-verification pass, and nothing already confirmed re-verified. This matters most on exactly the huge diffs the cap targets: a time-budgeted CI run that stops at the cap with ~30-90 minutes left must not spend it on an unbounded tail and die before compose. The tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. - Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the plan's round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is hashed into the record key and copied to the digest-named list file each block points at — so a launch that drops the pointer matches no record, and the delivery floor counts the agent's read of that file exactly as it counts the brief's. - **A reporting round whose every finding the verifier rejected is retroactively dry.** The merge already removes a rejected entry from the cumulative list; from the merge that applies the last of a round's rejections, the round also stops counting as a reporting round, and the two-consecutive-dry rule reads rounds' **effective** status. Rejected means rejected — an entry confirmed at low confidence keeps its round a reporting round. Under the pipelined loop a round's verdicts land while the next round runs, so the upgrade usually arrives one round late, and that is still one round saved: a measured run held round 2 dry, watched round 3's sole finding be rejected, and then ran rounds 4 **and 5** — round 4's dry return plus the rejection already in hand was the two-dry evidence, and the fifth round audited nothing the loop had not already answered (measured; DESIGN.md — The rounds a rejected finding bought (PR #8353)). The rule leans on the rejection bar the verifier's brief already enforces — a rejection claims direct counter-evidence, never mere unverifiability — so a round retired by rejections is retired on evidence, not on doubt. **It pairs forward only, and is consulted when a round returns**: on round _k_'s dry return, first apply every verdict that has landed (the unconditional merge — the retirement takes effect at this application, not at some earlier moment), then end the loop if round _k−1_ was dry or is now retired. Round _k−1_ counts **launches, not labels**: the convergence pair is one round here — a pair member is never round _k−1_ on its own (the pair bullet's not-carried-forward rule stands), and a reporting pair retires only when every finding from **both** members is rejected. The upgrade never ends the loop by itself — a preceding dry round plus a freshly-retired round stops nothing while the next round is already in flight: that round was launched, and its return is taken whatever it says, because a launched auditor can be carrying a real Critical. This is the measured shape (round 4's return is where the loop closes under this rule — the measured run, which predates it, ran a fifth round; a cap-5 shape — under the 3-round huge-diff tier the upgrade can only ever retire rounds 1–2, since the cap round's verdicts land during its solo verification, after the loop has already ended) and the only pairing licensed here. It softens nothing else: a whiffed scope stays not-audited whatever the verdicts say, and on 3B the retirement ledger's per-chunk certificates are untouched — this rule reads at the level the round counter reads. -- **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. (Step 4's initial verification is the k=0 case of the same rule: its shards ride with the first reverse-audit launch — the convergence pair on 3A, round 1's fan-out on 3B.) The serial shape (audit → wait for verification → next round) spent 5-8 minutes per round waiting for verifiers whose results the next round's auditors never needed. Two orderings still hold: the **last** round's verification must complete before Step 6 (that ordering is what keeps unverified entries out of the report and the PR, backed by the tag backstop at the end of this step — which `compose-review` machine-checks from `findingsPath`, Step 6), and a rejected finding leaves the cumulative list at the next merge. +- **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. (Step 4's initial verification is the k=0 case of the same rule: its shards ride with the first reverse-audit launch — the convergence pair, whole-diff on 3A and per-chunk rounds 1 and 2 on 3B. The convergence pair is the one exception on the LAUNCH side: a pair member's return never triggers this rule per member — round 2's auditors are already in flight — and the pair bullets above define the one transition; the pair's findings still verify as the k=2 case, riding round 3.) 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. **The tail after a budget stop is bounded, and its order is load-bearing.** Verify the last round's findings — the ones whose verifiers would have ridden the round the gate just refused — **only through `agent-prompt --role verify`, never a hand-rolled `agent`**: that builder is gated on a **compose floor** and prints a `VERIFY BUDGET:` refusal (exit 4) once too little time remains, at which point you stop verifying and compose **immediately** — findings still carrying `— [unverified]` keep the tag, and `compose-review` caps the verdict on it and never treats an unverified finding as a confirmed blocker; everything earlier rounds confirmed still posts. **Bound the wait, not just the launch:** the builder gate stops a verifier from being _built_ below the floor, but a verifier admitted _above_ it can still run a real filesystem/git E2E workload past the floor while you wait on its batch — and `agent-prompt` builds prompts, it cannot cancel a running agent. So when the deadline is within the compose floor and a verifier batch has not returned, **stop waiting on it yourself**: take the findings in hand at their current tag and compose. A verifier you stopped waiting on leaves its findings `— [unverified]`, which caps the verdict exactly as a refused build would. Do **not** re-verify findings already confirmed in earlier rounds, and do **not** invent a fresh re-verification pass — that is the unbounded work a wall runs into. Compose and submit are non-negotiable; they always run. Why this exists, measured twice: a +1699-line PR's CI review ran the audit loop to the 5-round cap and was killed while round 5's findings were still being verified (#8368); and a 4,269-line cross-worktree git guard stopped the audit correctly with ~110 minutes left, then a single hand-rolled agent re-running a 15-family shell/git bypass battery with real filesystem E2E consumed all of it — the wall hit mid-verification, compose never ran, and ~20 E2E-confirmed Critical bypasses were never posted (measured; DESIGN.md — The killed-before-compose tail (PR #8687)). 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 both members of the convergence pair find nothing, the second opinion has already run — that is what the pair is for. (On 3B, a first dry round is still only half the evidence: the second round runs before believing it.) +If both members of the convergence pair find nothing, the second opinion has already run — that is what the pair is for. (On 3B this holds per chunk: rounds 1 and 2 launch together, so each chunk's two establishing audits run at once, and a chunk is believed dry only when both members are.) 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. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index fd7392bcf5f..181aaeb33de 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -81,6 +81,27 @@ describe('bundled review skill', () => { expect(body).toContain('`agent-prompt --roster` after the rules load'); }); + it('launches the 3B convergence pair in the same response', () => { + // The pair's wall-clock saving exists only while both rounds go out + // together: a later edit serializing the skill while the prompt-builder + // tests stay green (they call each round builder themselves) restores + // the extra round wall. Bounded to the 3B section so the 3A pair's + // identical phrasing cannot satisfy it. + const body = skillBody(); + const start = body.indexOf('**The convergence pair — 3B'); + const end = body.indexOf('**Do not write the reverse auditor'); + expect(start).toBeGreaterThan(-1); + expect(end).toBeGreaterThan(start); + const section = body.slice(start, end); + expect(section).toContain('`--all-chunks --round 1`'); + expect(section).toContain('`--all-chunks --round 2`'); + expect(section).toContain('in the same response'); + // The reporting transition is the fix for the round-0 blocker; a revert + // dropping it must fail here, not slip through. + expect(section).toContain('wait for BOTH fan-outs'); + expect(section).toContain('every shard passed as `--round 2`'); + }); + it('pins the bounded-tail protocol on the round-cap bullet', () => { // The ROUND CAP refusal message carries the same verify-only / // compose-floor contract; a revert of the bullet's protocol hunk must