diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts index ed51f410cc5..4c50f9c3fd1 100644 --- a/packages/cli/src/commands/review/save-artifact.test.ts +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -11,14 +11,17 @@ import { mkdirSync, readFileSync, readdirSync, + realpathSync, rmSync, symlinkSync, writeFileSync, } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; +import yargs from 'yargs'; +import type { Argv } from 'yargs'; import { buildReport, type Finding } from './findings.js'; -import { saveReviewArtifact } from './save-artifact.js'; +import { saveArtifactCommand, saveReviewArtifact } from './save-artifact.js'; // On a case-sensitive filesystem the alias below never exists, so that test // can only run where the filesystem folds case. Probe once, at load time, so @@ -34,7 +37,6 @@ const caseInsensitiveFs = (() => { })(); let root: string; -let previousProjectDir: string | undefined; const finding: Finding = { id: 'R1-1', @@ -80,22 +82,21 @@ function fixture() { writeJson(composed, verdict); mkdirSync(join(root, '.qwen/reviews'), { recursive: true }); writeFileSync(report, '# Review\n'); - return { findings, composed, report, out }; + // The workspace root is explicit here because the test process's cwd is the + // package directory, not the temp root — the same explicit-root path the + // skill's own Step 8 invocation takes. + return { findings, composed, report, out, workspaceRoot: root }; } beforeEach(() => { - root = mkdtempSync(join(tmpdir(), 'review-artifact-')); - previousProjectDir = process.env['QWEN_CODE_PROJECT_DIR']; - process.env['QWEN_CODE_PROJECT_DIR'] = root; + // realpath, because the cwd-default test below chdirs into the root and + // compares against process.cwd(), which returns the physical path — on + // macOS the temp dir is reached through a /var → /private/var symlink. + root = realpathSync(mkdtempSync(join(tmpdir(), 'review-artifact-'))); }); afterEach(() => { rmSync(root, { recursive: true, force: true }); - if (previousProjectDir === undefined) { - delete process.env['QWEN_CODE_PROJECT_DIR']; - } else { - process.env['QWEN_CODE_PROJECT_DIR'] = previousProjectDir; - } }); describe('saveReviewArtifact', () => { @@ -384,9 +385,9 @@ describe('saveReviewArtifact', () => { expect(existsSync(paths.out)).toBe(false); }); - it('resolves relative paths against the workspace root, not cwd', () => { - // The form SKILL.md documents. beforeEach points QWEN_CODE_PROJECT_DIR at - // the temp root while cwd stays the package directory, so the two roots + it('resolves relative paths against the explicit workspace root, not cwd', () => { + // The form the skill's Step 8 block uses. --workspace-root points at the + // temp root while cwd stays the package directory, so the two roots // differ and the resolution direction is observable. fixture(); @@ -397,12 +398,12 @@ describe('saveReviewArtifact', () => { out: '.qwen/reviews/review.json', target: 'pr-123', effort: 'high', + workspaceRoot: root, }); expect(saved.path).toBe(join(root, '.qwen/reviews/review.json')); // The registration value `record_artifact` wants, printed so the skill - // copies it verbatim — including from a PR worktree run where cwd (the - // disposable review worktree) differs from the workspace root. + // copies it verbatim. expect(saved.workspacePath).toBe('.qwen/reviews/review.json'); expect(JSON.parse(readFileSync(saved.path, 'utf8'))).toMatchObject({ schemaVersion: 1, @@ -410,4 +411,82 @@ describe('saveReviewArtifact', () => { markdownReportPath: '.qwen/reviews/review.md', }); }); + + it('resolves against cwd and never QWEN_CODE_PROJECT_DIR', () => { + // The default path, with the trap armed. No `workspaceRoot` is passed — + // an embedder that omits it must land on cwd — while the env var points + // at a decoy the removed preference would have taken: the variable names + // the session-storage directory under the runtime base, never the main + // checkout, and six of six measured CI reviews fumbled on it (DESIGN.md — + // The artifact root that pointed at qwen-home). Re-introducing + // `explicit ?? env ?? cwd` fails this test: resolution lands on the + // decoy, not on cwd. + const decoy = mkdtempSync(join(tmpdir(), 'review-artifact-decoy-')); + const savedCwd = process.cwd(); + const previous = process.env['QWEN_CODE_PROJECT_DIR']; + process.env['QWEN_CODE_PROJECT_DIR'] = decoy; + try { + fixture(); + process.chdir(root); + const saved = saveReviewArtifact({ + findings: '.qwen/tmp/findings.json', + composed: '.qwen/tmp/composed.json', + report: '.qwen/reviews/review.md', + out: '.qwen/reviews/review.json', + target: 'pr-123', + effort: 'high', + }); + + expect(saved.path).toBe(join(root, '.qwen/reviews/review.json')); + expect(saved.workspacePath).toBe('.qwen/reviews/review.json'); + expect(existsSync(join(decoy, '.qwen/reviews/review.json'))).toBe(false); + } finally { + process.chdir(savedCwd); + if (previous === undefined) { + delete process.env['QWEN_CODE_PROJECT_DIR']; + } else { + process.env['QWEN_CODE_PROJECT_DIR'] = previous; + } + rmSync(decoy, { recursive: true, force: true }); + } + }); +}); + +describe('the CLI option contract', () => { + // Every test above builds its args by hand — the same shape that let a + // flag-name bug into `test-plan`: yargs camel-cases the flag, a field named + // for the flag read `undefined` on every real invocation, and the suite + // stayed green because nothing went through yargs. `--workspace-root` is + // this command's only multi-word flag AND its trust anchor (it roots the + // containment checks), so this test does not assert the parsed shape and + // stop — it feeds the yargs-parsed object straight into saveReviewArtifact + // and asserts on a write only reachable when the root actually arrived + // from the flag: cwd stays the package directory, where none of the + // fixture inputs exist. + it('parses --workspace-root into the field saveReviewArtifact actually reads', () => { + fixture(); + + const parsed = (saveArtifactCommand.builder as (y: Argv) => Argv)( + yargs([]), + ).parseSync([ + '--findings', + '.qwen/tmp/findings.json', + '--composed', + '.qwen/tmp/composed.json', + '--report', + '.qwen/reviews/review.md', + '--target', + 'pr-123', + '--effort', + 'high', + '--out', + '.qwen/reviews/review.json', + '--workspace-root', + root, + ]) as unknown as Parameters[0]; + + const saved = saveReviewArtifact(parsed); + expect(saved.path).toBe(join(root, '.qwen/reviews/review.json')); + expect(saved.workspacePath).toBe('.qwen/reviews/review.json'); + }); }); diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts index 40386e9118e..12dbe37f2e6 100644 --- a/packages/cli/src/commands/review/save-artifact.ts +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -66,15 +66,30 @@ interface SaveArtifactArgs { target: string; effort: ReviewEffort; out: string; + workspaceRoot?: string; } -// Every path resolves against the daemon workspace root, not cwd: in PR -// worktree mode cwd is the disposable review worktree, while the durable -// output and `markdownReportPath` must stay relative to the main project for -// Web Shell's `readWorkspaceFile` to find them. The skill threads that root -// through its subprocesses as QWEN_CODE_PROJECT_DIR. -function workspaceRoot(): string { - return resolve(process.env['QWEN_CODE_PROJECT_DIR'] ?? process.cwd()); +// Every path resolves against the main checkout, because the durable output +// and `markdownReportPath` must stay relative to the main project for Web +// Shell's `readWorkspaceFile` to find them. That root arrives as +// `--workspace-root`: the skill passes the main project directory explicitly +// on every run (SKILL.md Step 8), because the root anchors the containment +// checks — `isWithin` and the symlink walk below — and an ambient cwd is only +// as trustworthy as wherever the command happened to run. Cwd is the fallback +// when the flag is absent, right whenever the caller runs from the main +// checkout — in PR worktree mode the worktree-resident inputs arrive as +// absolute paths that still sit under the main project's `.qwen/tmp/`. +// +// This used to prefer `QWEN_CODE_PROJECT_DIR`, believing it named that +// checkout. It never does: the harness exports it as the session-storage +// directory under the runtime base (`Storage.getProjectDir()` — where the +// harness's transcripts live), in every environment. Every measured CI review +// resolved its containment root there, refused its own inputs, and burned +// minutes working around it (DESIGN.md — The artifact root that pointed at +// qwen-home). An ambient variable that is wrong 100% of the time it is +// consulted is not a fallback; it is a trap, so it is not consulted at all. +function workspaceRoot(explicit?: string): string { + return resolve(explicit ?? process.cwd()); } function isWithin(parent: string, child: string): boolean { @@ -259,7 +274,7 @@ function validateFindingsReport(value: unknown): FindingsReport { export function saveReviewArtifact( args: SaveArtifactArgs, ): SavedReviewArtifact { - const root = workspaceRoot(); + const root = workspaceRoot(args.workspaceRoot); const findingsPath = workspacePath(root, args.findings, 'Findings input'); const composedPath = workspacePath(root, args.composed, 'Composed input'); const reportPath = workspacePath(root, args.report, 'Markdown report'); @@ -378,6 +393,12 @@ export const saveArtifactCommand: CommandModule = { type: 'string', demandOption: true, describe: 'Output path under .qwen/reviews/', + }) + .option('workspace-root', { + type: 'string', + describe: + 'Root that containment and relative paths resolve against ' + + '(default: the working directory — run from the main checkout)', }), handler: (argv) => { const saved = saveReviewArtifact(argv as unknown as SaveArtifactArgs); diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 2a2e89bbb27..178e352f641 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -912,3 +912,23 @@ Dogfooding this skill against its own PR emitted `Review complete: pr-6771 — A ### The five already-implemented Suggestions Dogfooded against this skill's own PR, a run reported five "Suggestions" — "Enhanced Binary File Handling", "Security Improvement for Terminal Output" — each summarising a thing the PR already did, each with `Suggested fix: N/A (already implemented)`. That is not silence being better than noise; it is noise wearing silence's clothes, and the reader has to read all five to discover there was nothing to do. + +### The 22-minute serial first verification + +Two CI reviews of similar-size PRs ran the same skill on the same day (2026-08-06). The #8619 run launched its Step 4 verifier and its round-1 reverse auditor together in one response. The #8628 run launched the verifier alone at 08:12, read its verdicts at 08:34, and only then launched round 1 at 08:37 — 22 minutes of wall clock spent waiting for verdicts the auditor's launch never consumed (the findings file carries `— [unverified]` tags for exactly this state). The pipelining rule said "round _k_'s verifiers ride with round _k+1_'s auditors" and started counting at k=1, so the initial verification's coupling was orchestrator discretion, and discretion split 50/50 across the measured runs. + +### The serial convergence pair + +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 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. + +### The artifact root that pointed at qwen-home + +Every one of six measured CI reviews (2026-08-05/06) spent 1.5–3 minutes at Step 8 rediscovering the same fact: `save-artifact` resolved its containment root from `QWEN_CODE_PROJECT_DIR`, and that variable does not name the main checkout in any environment — the harness exports it as `Storage.getProjectDir()`, the session-storage directory under the runtime base where the harness's own transcripts live. The helper refused its own inputs ("must be inside the workspace"), and each orchestrator improvised a different workaround: one overrode the env var to the repo, one copied the inputs into the qwen-home mirror and copied the artifact back, others retried path shapes until one landed. The env preference was wrong 100% of the time it was consulted; the command's cwd — the main checkout, where the skill runs every subcommand — was right in every measured run. + +### The one-command-per-turn tail + +Measured across the same six CI reviews: the post-verdict bookkeeping — Markdown report, cost-ledger, save-artifact, `record_artifact`, the incremental-cache write, cleanup — ran one command per model turn, 4–6 minutes of wall clock after the review's outcome was already decided (and, on posting runs, already on the PR), stretching past 7 minutes when the qwen-home fumbling above joined it. Every command in the tail is cheap; the turns are not — the same arithmetic that batches the Step 1 setup calls, unapplied to the other end of the run. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 7a8150110bc..303015839a8 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -527,6 +527,8 @@ 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). + 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. **Do not write the verifier's prompt. Ask for it — and hand it the shard's findings so it prints the whole block:** @@ -571,28 +573,39 @@ After verification, identify **confirmed** findings that describe the **same typ 3. If the same pattern has more than 5 occurrences and severity is **not** Critical, list the first 3 locations plus "and N more locations" **in the text you show the reader**. That is a display rule, not a data rule: keep the complete `(path, anchor, line)` list internally, because Step 7 expands the aggregate into one resolver request per location and an anchor you truncated away is a comment that never gets posted. For **Critical** patterns, always list all locations in the text as well — every instance matters. -All confirmed findings (aggregated or standalone) proceed to Step 5. +All findings (aggregated or standalone) proceed to Step 5 — confirmed ones untagged, those still under verification carrying the `— [unverified]` tag Step 5's merge rules govern. ## Step 5: Iterative reverse audit (high effort only) **Medium skips this step.** A balanced (medium) review stops after Step 4: it goes straight to Step 6, composes the report and verdict from the verified findings, and does not run the reverse audit — which is why `compose-review` caps a clean medium review at `Comment` (Step 6) and why medium never writes the incremental cache or posts (`--comment` forces high). Everything below is high effort only. -After aggregation, run reverse audit **iteratively**. Each round receives the cumulative reported findings from all prior rounds, so successive rounds focus on whatever the previous round missed. +After deduplication, run reverse audit **iteratively** — the first launch rides with the Step 4 verifiers (Step 4 names this), so aggregation and the audit overlap rather than queue. Each round receives the cumulative reported findings from all prior rounds, so successive rounds focus on whatever the previous round missed. **Why iterative**: A single pass leaves whatever the reverse audit agent itself missed. Each round narrows what's left to discover, until diminishing returns terminate the loop. **Each round is a fan-out, not one agent.** -- **Small diffs (Step 3A path):** one reverse audit agent per round, reading the whole diff. +- **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). - **The builder schedules the 3B fan-out; you do not.** Rounds 1 and 2 audit every chunk — they are what establishes each territory's record. From round 3 on, `--all-chunks` reads the harness transcripts and **retires** any chunk whose own last two audits were substantively dry (the receipt named what it examined AND the transcript shows the diff was opened): a retired chunk is cold-checked on alternating rounds instead of every round, and a cold check that yields anything returns it to every-round auditing. The savings land on the odd rounds — every retired chunk cold-checks together on the even ones, so an even round's fan-out is unchanged; expect rounds 3 and 5 to shrink, not round 4. The blocks it prints are the round; the `retirement:` note after the `end of round` line names each skipped chunk and its certificate — relay that note in your narration, and do not hand-build an auditor for a chunk the builder skipped. Why, measured: on a real 6-chunk run, two chunks were dry in **all five rounds** — a third of the loop's auditors re-certifying territories that had already converged, while the three hot chunks were where every finding came from. Attention follows evidence; the certificate a retired chunk holds (two consecutive substantive dry audits) is exactly the one the whole loop used to end on. +**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. + +- **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. + **Do not write the reverse auditor's prompt. Ask for it — and hand it the findings so far so it prints the whole block:** Write **the cumulative list of every finding reported so far** (Steps 3-4 plus all prior rounds — verified or still under verification; entries a verifier rejected are removed) to a file, so the auditor hunts what is not already on it. **Every entry not yet through Step 4 carries a trailing `— [unverified]` tag** — added at the merge that admits it, removed by the merge after its verdict lands. An early round on a clean review may have nothing confirmed yet — pass the file anyway (empty is fine; the command tells the auditor so). Then: ```bash -# Step 3A (small diff): one auditor per round, the whole diff. +# Step 3A (small diff): one auditor per round, the whole diff. The convergence +# pair is two of these builds — `--round 1` and `--round 2`, same --findings — +# launched together (the CLI keys the two records apart by round). "${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan --role reverse-audit \ --findings \ --round \ @@ -617,16 +630,17 @@ The brief holds what the auditor is for: hunt only the **gaps** no prior agent c - **The substantive-return check applies to every round** — the same rule as Step 3's, enforced here, after each round returns: a bare `No issues found.` with no evidence of what the agent re-examined is a whiff, not a clean bill. Relaunch that agent once, within the round. If the relaunch is also bare, do not spin — take it, but its scope counts as **not audited**: track it in an outstanding-whiffed-scopes list, and clear it only when a later round's agent for that scope returns substantively. - A round is **dry** only when _every_ agent in it returned zero new findings **with** the evidence-bearing receipt (`No issues found — `). A round containing a twice-whiffed agent is **not dry** — silence is not convergence evidence — so the loop continues (the hard cap below still bounds it). - **When the loop ends with any scope still outstanding** (by cap, or by dry rounds elsewhere), terminal prose is not enough: add one self-explained entry per scope to `unreviewedDimensions` — e.g. `reverse audit of chunk 3 — the auditor returned nothing substantive twice` — so compose-review serializes it and caps a would-be Approve at `COMMENT`. The primary Step 3 pass did read that scope (its receipt stands), but this run's contract includes the reverse audit, and a verdict must not silently claim an audit that never ran. -- Stop after **two consecutive dry rounds** (the 3A criterion — one auditor, so round-dry and territory-dry are the same thing). One dry round is not evidence of convergence: on PR #6457 the review returned "no blockers" twice and the very next round surfaced five Criticals, three of them in code that had been in the diff since the first commit. A single lazy agent must not be able to end the loop. When the loop ends on this rule, the last reporting round's verifiers are already in flight (they launched with the next round's auditors) — wait for their verdicts and apply them in the final merge before Step 6. +- Stop after **two consecutive dry rounds** (the 3A criterion — one auditor, so round-dry and territory-dry are the same thing). One dry round is not evidence of convergence: on PR #6457 the review returned "no blockers" twice and the very next round surfaced five Criticals, three of them in code that had been in the diff since the first commit. A single lazy agent must not be able to end the loop. A dry convergence pair satisfies this rule in one launch — its two members are exactly the two independent audits the rule demands; what the pair removes is the wall clock between them, not either audit. When the loop ends on this rule, the last reporting round's verifiers are already in flight (they launched with the next round's auditors) — wait for their verdicts and apply them in the final merge before Step 6. - **On the 3B path the builder is also the convergence ledger**: when every chunk holds two consecutive substantive dry audits and none is due a cold check, `--all-chunks` builds nothing, prints a `CONVERGED` explanation to stderr and exits **5**. Stop the loop and proceed to Step 6 — this is a **clean** convergence, not a gap: no `unreviewedDimensions` entry is owed, because each chunk holds the two-dry rule's evidence chunk by chunk — two consecutive dry **audits**, though not necessarily in consecutive rounds (a chunk dry in rounds 1 and 2 skips round 3 and cold-checks dry in round 4, holding rounds 2 and 4). Exit 5 is mainly the CLI enforcing the stop the two-dry-rounds rule above used to leave to orchestrator discretion; the new savings are the odd-round skips and a round-5 convergence. (It cannot owe a verification launch: a reporting round makes its chunk hot, so every verifier launched with a later round that did run.) - Stop after **5 rounds** regardless (hard cap), and say so in the output rather than implying convergence. If round 5 reported findings, its verifiers have NOT launched — that launch rides the next round's build, which the cap forbids — so launch them alone before Step 6 and wait for their verdicts; the tag backstop below (and `compose-review`'s machine-read of it) is what catches a miss. - Findings **reported** by each round are merged into the cumulative list **before** the next round begins, so each round sees an updated baseline. **The merge runs unconditionally — before every round build and before Step 6, whether or not the previous round reported findings**: under the pipelined loop below, round _k_'s verdicts land during round _k+1_, and every termination mode (two dry rounds, CONVERGED, budget stop, the 5-round cap) can arrive with the final rounds dry — a merge keyed to "some round reported something" would never apply the last verdicts that landed. Each merge applies every Step 4 verdict that has landed: confirmed removes the tag, rejected removes the entry. Verification status does not gate the merge — the list exists so auditors do not re-report what is already filed, and an unverified entry serves that purpose exactly as well as a confirmed one. The trade, named: an entry a verifier later rejects will have suppressed one round of rediscovery in its neighbourhood — the window is one round in one location, and the 5-round cap still bounds the loop. The tag is what keeps this mechanical rather than remembered: an entry enters the list tagged `— [unverified]`; the merge after its Step 4 verdict removes the tag (confirmed) or the entry (rejected). Step 6's confirmed-only read then has something to key on — anything still tagged is left out of the confirmed set — instead of a memory of which round each entry arrived in. The tag rides inside the findings file, which is 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. -- **Verification rides alongside the next round, not ahead of it.** When round _k_ returns with new findings, one response launches BOTH round _k_'s verifiers (Step 4, `--role verify --round k` with that round's new findings) AND round _k+1_'s auditors — build the two prompt sets first, then fire every agent together, exactly as Step 3 fans out. The serial shape (audit → wait for verification → next round) spent 5-8 minutes per round waiting for verifiers whose results the next round's auditors never needed. Two orderings still hold: the **last** round's verification must complete before Step 6 (that ordering is what keeps unverified entries out of the report and the PR, backed by the tag backstop at the end of this step — which `compose-review` machine-checks from `findingsPath`, Step 6), and a rejected finding leaves the cumulative list at the next merge. +- **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) 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. - **The round builder is also the loop's clock.** In a time-budgeted run (CI exports `QWEN_REVIEW_DEADLINE_EPOCH`; a local run normally has no deadline and is untouched), `agent-prompt --role reverse-audit` refuses to build a round that no longer fits: the remaining time must cover **the round itself** (estimated from the costliest round's measured cost so far — a repair relaunch can make one round the expensive one, and the gate prices the worst case the run has proved, not the newest dip — or a conservative constant for round 1) **plus** the reserve kept for its verification, compose-review and submission. On refusal it prints a `BUDGET:` line to stderr and exits **4**. That refusal is a termination rule, not an error — do not rebuild the round, do not relaunch auditors, and do not retry the command. The builder also records a budget-stop marker that `compose-review` reads directly, so the verdict is capped whether or not you relay anything; still add the exact entry the message names (`reverse audit — stopped before round by the review time budget`) to `unreviewedDimensions` so the terminal report and the body agree, and proceed to Step 6 with the findings already confirmed — spending what remains only on verifying findings already in hand, composing, and submitting. Why this exists, measured: a +1699-line PR's CI review ran the audit loop to the 5-round cap, spent 3.5 of its 4 budgeted hours there, and was killed by the outer CI timeout while round 5's findings were still being verified — every confirmed finding died with it. A review that stops on the budget still reports everything it proved; one that runs past it reports nothing. **Reverse audit findings go through Step 4 verification like any other finding.** They used to skip it on the theory that the auditor "already has full context." That premise fails exactly when the diff is large — the auditor with the least room to think was the one whose output nobody checked. -If the very first round finds nothing, that is a good sign — but run the second round anyway before believing it. +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.) 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. @@ -1092,6 +1106,8 @@ Clean up the JSON files in Step 9. ## Step 8: Save review report and cache +**Steps 8 and 9 are four responses, not ten.** Every command in this tail is cheap; the model turns between them are not — and this stretch runs after the verdict is already computed (and, on a posting run, already posted), so every extra turn is pure latency to the reader. Measured across six CI reviews, the one-command-per-turn shape cost 4–6 minutes after submission, before the artifact-root fumbling below stretched it further (measured; DESIGN.md — The one-command-per-turn tail). The dependency chain is short, so batch to it, issuing each group as separate tool calls in one response exactly as the Step 1 setup batch does: (1) `cost-ledger` plus the `read_file` of the findings artifact — everything the report's content still needs; (2) write the Markdown report; (3) `save-artifact` and the incremental-cache write, when this run owes one — both read only files that already exist, and neither reads the other; (4) `record_artifact` (its `workspacePath` comes from save-artifact's stdout, which is why it is not in group 3) together with Step 9's `cleanup`. **Group (4) fires only after group (3) succeeded**: `cleanup` deletes the `.qwen/tmp` side files `save-artifact` reads, so a failed group (3) is resolved first — the JSON helper is fail-closed (below), and destroying its only inputs would convert a recoverable failure into a permanent one. When it **cannot** be resolved (a malformed input, a disk error — synthesizing a replacement is forbidden by the same fail-closed rule), the run still ends properly: disclose the failure, skip `record_artifact` (there is nothing valid to register), copy the findings/composed inputs beside the Markdown report so the artifact stays rebuildable, and **still run `cleanup`** — Step 9's bypass audit and the completion line are never gated behind a success that will not come. A group's remaining reads may join its response; nothing here needs a turn of its own. Lower tiers drop the commands they never owed (low saves no artifact and writes no cache), not the batching. + ### Report persistence Save the review results to a Markdown file for future reference: @@ -1122,7 +1138,7 @@ Report content should include: A run has written an Approve into its saved report minutes after reading the capped verdict (measured; DESIGN.md — The narrated-away cap). The terminal is prose and the archive is forever; this line is the one place the archive can be made to tell the truth for free. If the composed event is not the one you expected, fix the run — not the report. -After the Markdown report exists, and before cleanup, create and register the structured review artifact for **medium and high** effort (low has no canonical composed verdict and must not invent one). Use the same filename stem as the Markdown report with a `.json` extension: +After the Markdown report exists, create and register the structured review artifact for **medium and high** effort (low has no canonical composed verdict and must not invent one) — the creation is group (3) of the batching rule above; the registration rides group (4) alongside cleanup, which never touches `.qwen/reviews/`. Use the same filename stem as the Markdown report with a `.json` extension: ```bash "${QWEN_CODE_CLI:-qwen}" review save-artifact \ @@ -1131,9 +1147,12 @@ After the Markdown report exists, and before cleanup, create and register the st --report .qwen/reviews/.md \ --target \ --effort \ + --workspace-root \ --out .qwen/reviews/.json ``` +`save-artifact` resolves relative paths and its containment root against `--workspace-root` — **pass the main project directory explicitly, as the block above does**; without the flag it falls back to its own working directory. The flag is not decoration: the root anchors the containment checks (`isWithin` and the symlink walk), and an ambient-cwd root is only as trustworthy as wherever the command happened to run — from inside the untrusted PR worktree it would be the PR's own tree, the exact threat `comment-status`'s run-from-the-main-checkout rule exists to prevent. It used to prefer `QWEN_CODE_PROJECT_DIR`, which does not name the main checkout in any environment — the harness exports it as the session-storage directory under the runtime base — and every measured CI run burned minutes rediscovering that before improvising a workaround (measured; DESIGN.md — The artifact root that pointed at qwen-home). + For PR worktree mode, the findings and composed inputs were created inside `worktreePath`, while the durable report and output belong to the main project directory. Pass absolute paths for all four: resolve `--findings` and `--composed` against `worktreePath`, and resolve `--report` and `--out` against the main project directory. The worktree lives under the main project's `.qwen/tmp/`, so all four remain inside the session workspace accepted by the helper. `save-artifact` prints one JSON object on stdout — `{"path": "", "workspacePath": ""}`. Then call `record_artifact` in the current session with exactly this registration shape, copying `workspacePath` from that stdout object verbatim (do not re-derive it from the absolute path): ```json