perf(review): remove serial wall-clock the audit loop and tail never needed

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,17 +398,95 @@ 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,
       target: 'pr-123',
       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<typeof saveReviewArtifact>[0];
+
+    const saved = saveReviewArtifact(parsed);
+    expect(saved.path).toBe(join(root, '.qwen/reviews/review.json'));
+    expect(saved.workspacePath).toBe('.qwen/reviews/review.json');
+  });
 });

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);

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.