From 434e9c7913aa8cd1d2e50b8c690c3c67dcec6afb Mon Sep 17 00:00:00 2001 From: verify Date: Tue, 11 Aug 2026 11:08:04 +0800 Subject: [PATCH 1/5] perf(review): extend the convergence pair to 3B (chunked) reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reverse-audit loop is 77-80% of the wall clock on long CI reviews (measured on two v0.21.9 runs: 291min with 223min in the RA loop, 252min with ~205min), and on 3B its rounds ran serially because the convergence pair — rounds 1 and 2 launched together — was 3A-only. The pair's arithmetic is per-territory, not whole-diff: a chunk dry in round 1 leaves its slice of the cumulative findings list unchanged, so that chunk's round-2 auditor re-runs substantively the same audit. Launching `--all-chunks --round 1` and `--all-chunks --round 2` in one response runs each chunk's two establishing audits concurrently, saving one round's wall (~30-56min) off every chunked review — at the same one-round suppression window the 3A pair and the pipelined loop already accept. Orchestration-only: the CLI already builds round 2 before round 1's transcripts exist (round 2 always fans out to every chunk; the retirement schedule only reads history from round 3), and the deadline gate prices the paired round-2 admission on its 600s floor exactly as the 3A pair relies on. A new agent-prompt test pins that mechanism; SKILL.md carries the per-chunk pair, and DESIGN.md the measurement. --- .../src/commands/review/agent-prompt.test.ts | 24 +++++++++++++++++++ .../core/src/skills/bundled/review/DESIGN.md | 4 ++++ .../core/src/skills/bundled/review/SKILL.md | 10 ++++---- 3 files changed, 33 insertions(+), 5 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 92a64500415..916006bb1b2 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -3310,6 +3310,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 records are empty 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/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 262fed58f17..3e0fee1e37d 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 still packs tighter than two serial rounds ever could. + ### 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 b824baabbda..3a70400209c 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,17 @@ 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. -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); same deadline-gate degradation (if the round-2 `--all-chunks` build is refused, exit 4, launch round 1 alone and take the stop). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** @@ -676,7 +676,7 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c **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. From e3c274dbc43e3d85d2ba17c75d98cef171727638 Mon Sep 17 00:00:00 2001 From: verify Date: Tue, 11 Aug 2026 11:55:11 +0800 Subject: [PATCH 2/5] docs(review): fix the stale 3B parenthetical in the pipelined-loop bullet The k=0 launch-coupling note still described 3B's first reverse-audit launch as "round 1's fan-out"; with the convergence pair now applying to 3B it is rounds 1 and 2 per chunk, matching the same fix already made at Step 4's verifier-coupling paragraph. --- packages/core/src/skills/bundled/review/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 3a70400209c..5c5acfa6178 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -671,7 +671,7 @@ 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 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 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 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. From 1da92642ae45f7a62eace7c9a4645c868831d0e9 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 07:26:06 +0000 Subject: [PATCH 3/5] fix(review): price the 3B pair's wall at the gate and define its reporting transition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deadline gate priced the concurrent 3B pair's round-2 build off the seconds-old round-1 stamp — clamped to the observation floor — so both members were committed at roughly one round's price even though their two per-chunk fan-outs share the tool-concurrency pool and can take up to two rounds' wall. Admissions whose predecessor is still in flight now pay both members' wall in waves of the pool (expectedAdmissionSeconds): one round's price when the pool holds both fan-outs at once, up to the two-round bound when it serializes them, and the refusal degrades to round 1 alone as the skill's budget-stop rule says. SKILL.md's 3B pair also defined only the dry outcome; its reporting transition now spells out waiting for both fan-outs, deduping across rounds and chunks, one `--round 2` verifier batch riding round 3's build, and the pair's exemption from the pipelined k/k+1 launch rule. DESIGN.md's "packs tighter than two serial rounds ever could" claim is replaced with the provable bound, and the gate's wave pricing is recorded beside it. Tests pin the pair price (deadline.ts and the builder's refusal/admission shapes) and the skill's same-response pair launch. --- .../src/commands/review/agent-prompt.test.ts | 57 ++++++++- .../cli/src/commands/review/agent-prompt.ts | 27 +++-- .../src/commands/review/lib/deadline.test.ts | 114 ++++++++++++++++++ .../cli/src/commands/review/lib/deadline.ts | 89 ++++++++++++-- .../core/src/skills/bundled/review/DESIGN.md | 2 +- .../core/src/skills/bundled/review/SKILL.md | 9 +- .../src/skills/bundled/review/SKILL.test.ts | 17 +++ 7 files changed, 294 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 916006bb1b2..bf08ecdf07a 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', () => { @@ -3320,7 +3375,7 @@ describe('per-chunk retirement — cold territories stop costing a round', () => // 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 records are empty at this point + 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); diff --git a/packages/cli/src/commands/review/agent-prompt.ts b/packages/cli/src/commands/review/agent-prompt.ts index 75ad5f14b91..cfd9eaab3c5 100644 --- a/packages/cli/src/commands/review/agent-prompt.ts +++ b/packages/cli/src/commands/review/agent-prompt.ts @@ -44,7 +44,7 @@ import { dirname, join, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { launchToolBudget, reverseAuditRoundCap } from './lib/budget.js'; import { - expectedRoundSeconds, + expectedAdmissionSeconds, readRoundStamps, reverseAuditBudgetExhausted, reverseAuditBudgetMessage, @@ -1861,12 +1861,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 @@ -1893,7 +1898,7 @@ function admitReverseAuditRound( } const spent = reverseAuditBudgetExhausted( process.env, - expectedRoundSeconds(planPath, round), + expectedAdmissionSeconds(planPath, round, fanOutWidth, process.env), ); if (spent !== null) { writeBudgetStop(planPath, spent, round); @@ -1988,6 +1993,7 @@ function runAllChunks( planPath, round, reverseAuditRoundCap(report.budget), + chunks.length, ) ) { return; @@ -2382,7 +2388,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, @@ -2400,6 +2408,7 @@ function runAgentPrompt(args: AgentPromptArgs): void { args.plan, args.round, reverseAuditRoundCap(report.budget), + 1, ) ) { return; @@ -2450,14 +2459,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' @@ -2479,6 +2491,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 8777ca869ea..6a5dcd4094f 100644 --- a/packages/cli/src/commands/review/lib/deadline.test.ts +++ b/packages/cli/src/commands/review/lib/deadline.test.ts @@ -23,8 +23,11 @@ import { DEFAULT_RESERVE_SECONDS, DEFAULT_ROUND_SECONDS, DEFAULT_COMPOSE_FLOOR_SECONDS, + DEFAULT_TOOL_CONCURRENCY, + TOOL_CONCURRENCY_ENV, budgetStopEntry, budgetStopEntryZh, + expectedAdmissionSeconds, expectedRoundSeconds, readBudgetStop, readRoundStamps, @@ -280,6 +283,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 c2ab90b4063..63715ffca1c 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -41,6 +41,7 @@ import { mkdirSync, readFileSync, statSync, 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. */ @@ -132,6 +133,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; @@ -220,6 +231,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 @@ -243,16 +275,55 @@ 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. + */ +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 costliestSpanSeconds(stamps, nowMs) ?? DEFAULT_ROUND_SECONDS; } - 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 3e0fee1e37d..b96c8de6e90 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -923,7 +923,7 @@ Measured on the CI reviews of #8619 and #8607: both audits converged at the mini ### 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 still packs tighter than two serial rounds ever could. +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 rounds a rejected finding bought (PR #8353) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 5c5acfa6178..7946333bbe8 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -630,9 +630,12 @@ After deduplication, run reverse audit **iteratively** — the first launch ride - **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 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: 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 this cannot currently fire — the rule exists so a future pricing change degrades to the serial shape instead of to a guess. -**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); same deadline-gate degradation (if the round-2 `--all-chunks` build is refused, exit 4, launch round 1 alone and take the stop). The launch coupling holds too: both members ride with the Step 4 verifier shards (Step 4 names this). +**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 round-2 `--all-chunks` build is refused by the deadline gate (exit 4), launch round 1 alone and take the stop. The gate prices that build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so the refusal fires exactly when the pair plus the reserve does not fit, and round 1 alone keeps the serial shape. **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** @@ -671,7 +674,7 @@ 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 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 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, whole-diff on 3A and per-chunk rounds 1 and 2 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. diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 20767227316..2962db72521 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -81,6 +81,23 @@ 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'); + }); + it('routes both remote-resolution paths through match-remote', () => { // The pr-url path (Step 1) and the bare-PR-number path both resolve the // remote via the deterministic matcher. A later edit reverting either From 876d315208a3a964c9baa307e2b463b206e52ce5 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 12:40:45 +0000 Subject: [PATCH 4/5] fix(review): cover the both-refused pair shape and document the gate's pricing bounds - Delegate expectedAdmissionSeconds' solo branch to expectedRoundSeconds, restoring one production round-cost estimator (R2-3). - State that the pair price covers the auditor fan-outs only; the co-launched Step 4 verifier shards' extra wave is the reserve's to carry (R2-1). - Document the pair-shaped span ledger's solo over-price as accepted conservatism, in the estimator doc and DESIGN.md (R2-6). - Mirror the 3A pair annotation in the Step 5 3B copyable command block (R2-7). - Make both pair refusal bullets orientation-symmetric and cover the both-builds-refused shape, where nothing launches and the first refusal's marker is the stop (R2-8). --- .../cli/src/commands/review/lib/deadline.ts | 17 ++++++++++++++++- .../core/src/skills/bundled/review/DESIGN.md | 2 +- .../core/src/skills/bundled/review/SKILL.md | 7 +++++-- 3 files changed, 22 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/review/lib/deadline.ts b/packages/cli/src/commands/review/lib/deadline.ts index e9558e49dc8..8215f65a297 100644 --- a/packages/cli/src/commands/review/lib/deadline.ts +++ b/packages/cli/src/commands/review/lib/deadline.ts @@ -302,6 +302,21 @@ export function expectedRoundSeconds( * 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, @@ -317,7 +332,7 @@ export function expectedAdmissionSeconds( const predecessorInFlight = last !== undefined && nowMs - last.atMs < MIN_OBSERVED_ROUND_SECONDS * 1000; if (!predecessorInFlight) { - return costliestSpanSeconds(stamps, nowMs) ?? DEFAULT_ROUND_SECONDS; + return expectedRoundSeconds(planPath, round, nowMs); } const single = costliestSpanSeconds(stamps.slice(0, -1), last.atMs) ?? diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 182f7e1d257..46ed460d9c5 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -923,7 +923,7 @@ Measured on the CI reviews of #8619 and #8607: both audits converged at the mini ### 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. +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) diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 73d34387ab6..ed9b1193ccc 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -630,12 +630,12 @@ After deduplication, run reverse audit **iteratively** — the first launch ride - **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: 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 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. **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 round-2 `--all-chunks` build is refused by the deadline gate (exit 4), launch round 1 alone and take the stop. The gate prices that build as the pair's wall — both fan-outs in waves of the runtime's tool-concurrency pool — so the refusal fires exactly when the pair plus the reserve does not fit, and round 1 alone keeps the serial shape. +- 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:** @@ -651,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 \ From 3e231e51f0877c810c4d7429848764136d489e90 Mon Sep 17 00:00:00 2001 From: qwen-code-dev-bot Date: Tue, 11 Aug 2026 14:58:02 +0000 Subject: [PATCH 5/5] test(review): pin the 3B pair's reporting transition in the skill test --- packages/core/src/skills/bundled/review/SKILL.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index b1af52af46f..181aaeb33de 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -96,6 +96,10 @@ describe('bundled review skill', () => { 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', () => {