fix(review): clear the deferred-suggestion backlog from #9175's review rounds

packages/cli/src/commands/review/agent-prompt.test.ts

@@ -2638,8 +2638,18 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
     // The command runs install + builds + tests in one process; the agent's default
     // 120s shell timeout would kill it — the very failure this command prevents, one
     // level up. So the block tells the agent to pass the tool's max, 600000ms.
+    //
+    // Pinned PER SITE, not per prompt: three sites supply the directive (the
+    // first call, the resume paragraph's "Same …", the efficacy probe's
+    // "… too"), so a whole-prompt `toContain` stayed green with any one of
+    // them deleted — and the deleted first-call directive is exactly the
+    // 120s mid-install kill this assertion's own comment names.
     const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' });
-    expect(p).toContain(`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}`);
+    expect(p).toContain(
+      `Invoke it with \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`:`,
+    );
+    expect(p).toContain(`Same \`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\``);
+    expect(p).toContain(`\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\` too`);
   });
 
   it('tells Agent 7 how to CONTINUE a run one call could not finish', () => {
@@ -2649,8 +2659,14 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
     // call teaches the agent to report a truncated dimension as a finished
     // one — which is what three live reviews did.
     const p = buildRoleBrief(PR_PLAN, '7', { planPath: '/abs/tmp/plan.json' });
-    expect(p).toContain('testScope.notRun');
-    expect(p).toContain('"clamped": true');
+    // Anchored to the continuation PARAGRAPH's own sentence: the bare
+    // literals are also supplied verbatim by the role-7 base brief
+    // (agent-briefs), so `toContain('testScope.notRun')` stayed green with
+    // the whole paragraph deleted.
+    expect(p).toContain(
+      'Work is left when `testScope.notRun` is non-empty, or when any ' +
+        '`test[]` entry has `"clamped": true`',
+    );
 
     // Asserted on the CONTINUATION BLOCK ALONE, which is the whole point. The
     // first cut of this test searched the entire prompt: `--resume` matched the
@@ -2667,11 +2683,45 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => {
     // `resolve` — not spelled as POSIX literals: on Windows the prompt carries
     // `C:\\abs\\tmp\\plan.json`, and a hardcoded expectation fails there for a
     // reason that has nothing to do with the continuation block.
-    expect(resumeBlock[0]).toContain('review build-test');
+    // The FULL wrapper, on THIS block: the whole-prompt pins are satisfied
+    // by the first invocation block and vice versa, so a wrapper deleted
+    // from either one shipped green — and a resume block without it execs
+    // bare PATH `qwen`, an old global that lacks `build-test` entirely.
+    expect(resumeBlock[0]).toContain(
+      '"${QWEN_CODE_CLI:-qwen}" review build-test',
+    );
     expect(resumeBlock[0]).toContain(`--plan ${resolve('/abs/tmp/plan.json')}`);
+    // The tree too: a continuation against a different tree measures a
+    // different run. Never asserted before — a dropped `--worktree` line
+    // shipped green.
+    expect(resumeBlock[0]).toContain(
+      `--worktree ${resolve('.qwen/tmp/review-pr-6766')}`,
+    );
     expect(resumeBlock[0]).toContain(
       `--out ${join(resolve('/abs/tmp'), 'qwen-review-pr-6766-build-test.json')}`,
     );
+    expect(resumeBlock[0]).toContain('--resume');
+
+    // And the FIRST invocation block carries its own wrapper and tree — the
+    // same two elements, scoped to the block that must supply them.
+    const firstBlock = fences.find(
+      (f) => f.includes('review build-test') && !f.includes('--resume'),
+    );
+    expect(firstBlock).toBeDefined();
+    expect(firstBlock).toContain('"${QWEN_CODE_CLI:-qwen}" review build-test');
+    expect(firstBlock).toContain(
+      `--worktree ${resolve('.qwen/tmp/review-pr-6766')}`,
+    );
+
+    // The third-shape sentence, at BOTH prose sites — the role-7 base brief
+    // and the welded resume paragraph each carry it, so a single toContain
+    // is satisfied by either and a one-site deletion ships green. Counted,
+    // not just matched: deleting the sentence anywhere drops the count, and
+    // an agent missing it treats the endedBeforeTests shape as continuable,
+    // spending a MAX_RESUME_CALLS slot on a --resume that can only answer
+    // "ended before its test phase".
+    expect(p.split('"endedBeforeTests": true').length - 1).toBe(2);
+    expect(p.split('do not spend a continuation on it').length - 1).toBe(2);
   });
 
   it('welds the PR into Agent 0 — an unqualified number judges the wrong issue', () => {

packages/cli/src/commands/review/agent-prompt.ts

@@ -1429,13 +1429,14 @@ export function buildRoleBrief(
           'at 106s and `packages/cli` at 401s, before the rest). Work is left when ' +
           '`testScope.notRun` is non-empty, or when any `test[]` entry has ' +
           '`"clamped": true` — a suite the budget started too late and killed, which ' +
-          'says nothing about the suite. A third shape carries no field at all: a ' +
+          'says nothing about the suite. A third shape ends before any suite: a ' +
           'single-package repo whose budget ran out before its one suite has an ' +
-          'empty `test[]` and no `testScope`, and only its `note` says so — read ' +
-          'the note before calling the dimension finished. That shape cannot be ' +
-          'continued (a continuation has no recorded scope to read, and answers ' +
-          '"ended before its test phase" without running anything): report the ' +
-          'dimension UNFINISHED and do not spend a continuation on it. A resumed ' +
+          'empty `test[]`, no `testScope`, and `"endedBeforeTests": true` — the ' +
+          "report's own stamp — with the note naming the unrun suite. That shape " +
+          'cannot be continued (a continuation has no recorded scope to read; a ' +
+          '`--resume` on it answers "ended before its test phase" and points at a ' +
+          'fresh run): report the dimension UNFINISHED and do not spend a ' +
+          'continuation on it. A resumed ' +
           'call skips install and build and ' +
           'runs only what is left, merging into the SAME report file. Same ' +
           `\`timeout: ${SHELL_TOOL_MAX_TIMEOUT_MS}\`, and at most ` +

packages/cli/src/commands/review/build-test.test.ts

@@ -449,6 +449,30 @@ describe('runBuildTest', () => {
     expect(rep.testScope?.caveat).toBeUndefined();
     expect(rep.note).toContain('no package to build');
     expect(rep.note).toContain('complete answer');
+    // Not a probe: the stamp must not appear on an ordinary zero-affected run.
+    expect(rep.buildOnly).toBeUndefined();
+
+    // The probe stamp rides the zero-affected return too — this producer
+    // path branches on buildOnly for its note but used to drop the stamp,
+    // so a resumed probe report lost its probe answer.
+    const probe = runBuildTest({
+      plan: planPath,
+      worktree: root,
+      timeout: 5,
+      install: false,
+      buildOnly: true,
+      exec: (command) => {
+        calls.push(command);
+        return {
+          command,
+          exitCode: 0,
+          seconds: 1,
+          timedOut: false,
+          output: '',
+        };
+      },
+    });
+    expect(probe.buildOnly).toBe(true);
   });
 
   it('runs nothing but discloses the caveat for out-of-workspace files that are not inert', () => {
@@ -931,6 +955,12 @@ describe('runBuildTest', () => {
     // And the note must not claim tests it did not run.
     expect(buildOnly.note).toContain('build-only');
     expect(buildOnly.note).not.toContain('ran the tests');
+    // The WRITE half of the probe stamp, on the main results-initializer
+    // path: a probe report without it read as a completed zero-suite run on
+    // --resume — the probe/completed misclassification the split exists to
+    // prevent. The non-probe sibling must not carry it.
+    expect(buildOnly.buildOnly).toBe(true);
+    expect(withTests.buildOnly).toBeUndefined();
   });
 
   it('scopes build AND tests to the changed workspace and its dependents', () => {
@@ -1577,6 +1607,11 @@ describe('runBuildTest', () => {
     expect(rep.note).not.toContain('defines no test script');
     expect(rep.note).toContain('not run: .');
     expect(rep.ok).toBe(true);
+    // The structural stamp: build green, no probe, no scope — without it the
+    // resume split read this exact report as COMPLETED with no suite to run,
+    // certified the one existing suite as finished, and dropped the fresh
+    // re-run advice that is the only path to ever running it.
+    expect(rep.endedBeforeTests).toBe(true);
   });
 
   it('runs the AFFECTED workspace first, so the budget trims dependents, never the changed suite', () => {
@@ -3204,6 +3239,64 @@ describe('runBuildTest', () => {
       expect(rep.note).not.toContain('reached every suite');
     });
 
+    it('a COMPLETED zero-suite run is not "ended before its test phase"', () => {
+      // A single-root package with no test script finishes a fresh run
+      // completely: test [], no scope, ok true. Calling that "ended before
+      // its test phase" was self-contradictory beside the report's own note
+      // ("defines no test script, so no tests ran"), and its re-run advice
+      // re-derived the same zero-suite answer at the price of a full fresh
+      // install+build. The split is structural — ok and the buildOnly stamp
+      // — never the note's prose.
+      threePackages();
+      const outPath = join(root, 'report.json');
+      const base = {
+        toolchain: 'npm',
+        run: runId(),
+        affected: ['packages/core'],
+        buildSet: ['packages/core'],
+        widenedWith: [],
+        install: okResult('npm ci --no-audit --no-fund'),
+        build: [okResult('npm run build --workspace="packages/core"')],
+        test: [],
+        ok: true,
+        timedOut: [],
+        note: 'the package defines no test script, so no tests ran',
+      };
+      writeFileSync(outPath, JSON.stringify(base));
+      const attempt = () =>
+        runBuildTest({
+          plan: planPath,
+          worktree: root,
+          out: outPath,
+          timeout: 60,
+          install: true,
+          resume: true,
+          exec: okResult,
+        });
+      const completed = attempt();
+      expect(completed.note).toContain('completed with no suite to run');
+      expect(completed.note).not.toContain('ended before its test phase');
+      expect(completed.note).not.toContain('Re-run build-test');
+
+      // The deliberate probe keeps the early-end message: its report has no
+      // tests and no scope BY CHOICE, and only the stamp tells it apart.
+      writeFileSync(outPath, JSON.stringify({ ...base, buildOnly: true }));
+      expect(attempt().note).toContain('ended before its test phase');
+
+      // And the budget-floor shape keeps it too — build green, ok true, no
+      // scope, but the fresh path stamped that the test phase ran nothing.
+      // This is the shape whose real suite the false-completion answer
+      // certified as finished.
+      writeFileSync(
+        outPath,
+        JSON.stringify({ ...base, endedBeforeTests: true }),
+      );
+      const floored = attempt();
+      expect(floored.note).toContain('ended before its test phase');
+      expect(floored.note).toContain('Re-run build-test without --resume');
+      expect(floored.note).not.toContain('completed with no suite to run');
+    });
+
     it('keeps reporting work left when a retry is killed AGAIN by the budget', () => {
       // The ordinary outcome when an expensive suite is admitted late: it is
       // re-clamped rather than finished. Reporting that as a completed run
@@ -3337,6 +3430,85 @@ describe('runBuildTest', () => {
 
       expect(rep.note).not.toContain('Every suite in scope has now run');
       expect(rep.note).toContain('npm test --workspace="packages/a"');
+      // Named ONCE. The unattempted retry rides both lists — its command is
+      // still-to-run, its stale clamped entry survives in the merged test[]
+      // — and two additive-looking clauses both naming it made one command
+      // read as two against the continuation budget, with the provisional
+      // clause claiming it was "killed on a deadline the budget shortened"
+      // this call, false for a retry never started. The still-to-run clause
+      // fully describes it; the provisional clause must not repeat it.
+      expect(rep.note).toContain('still to run');
+      expect(rep.note).not.toContain('still provisional');
+    });
+
+    it('the caveat names BOTH halves when work is unattempted AND re-clamped', () => {
+      // An else-if kept the provisional half out of the caveat whenever
+      // outstanding work existed — and the brief quotes the caveat as the
+      // live limitation, so a reader of it alone under-counted what is
+      // left. The two segments stay disjoint: a re-clamped suite killed
+      // again THIS call is provisional; an unreached workspace is still to
+      // run.
+      threePackages();
+      const outPath = join(root, 'report.json');
+      writeFileSync(
+        outPath,
+        JSON.stringify({
+          toolchain: 'npm',
+          run: runId(),
+          affected: ['packages/core'],
+          buildSet: ['packages/core', 'packages/b'],
+          widenedWith: [],
+          install: null,
+          build: [okResult('npm run build --workspace="packages/core"')],
+          test: [
+            {
+              command: 'npm test --workspace="packages/core"',
+              exitCode: null,
+              seconds: 100,
+              timedOut: true,
+              output: '',
+              deadlineMs: 100_000,
+              clamped: true,
+            },
+          ],
+          ok: false,
+          timedOut: [],
+          note: 'one clamped, one unreached',
+          testScope: {
+            workspaces: ['packages/core'],
+            notRun: ['packages/b'],
+          },
+        }),
+      );
+
+      // The retry is admitted with a budget-shortened deadline and killed
+      // again (exec burns ~6s and reports the timeout); what remains is
+      // below the attempt floor, so packages/b is never started.
+      const rep = runBuildTest({
+        plan: planPath,
+        worktree: root,
+        out: outPath,
+        timeout: 60,
+        budget: 20,
+        install: true,
+        resume: true,
+        exec: (command) => {
+          Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, 6000);
+          return {
+            command,
+            exitCode: null,
+            seconds: 6,
+            timedOut: true,
+            output: '',
+          };
+        },
+      });
+
+      const caveat = rep.testScope?.caveat ?? '';
+      expect(caveat).toContain('still to run');
+      expect(caveat).toContain('packages/b');
+      expect(caveat).toContain('provisional');
+      expect(caveat).toContain('npm test --workspace="packages/core"');
     });
 
     it("runs the AFFECTED pending suite first — the fresh path's invariant", () => {
@@ -3713,6 +3885,49 @@ describe('runBuildTest', () => {
         { toolchain: 'npm', test: [] },
         { toolchain: 'npm', test: [], build: [] },
         { toolchain: 'npm', test: [], timedOut: [] },
+        // The `test` clause needs its own UNMASKED witness: a fixture that
+        // also omitted `affected` and `ok` was refused by their clauses
+        // whatever happened to the `test` clause, so deleting
+        // `!commandsOk(shape.test)` kept every refusal green while a report
+        // truncated of only its `test` key cleared the mutated gate and
+        // died at `previous.test.filter` — the raw crash the gate exists to
+        // replace. Every other walked field is present and valid here, so
+        // the refusal rides on the `test` clause alone.
+        {
+          toolchain: 'npm',
+          affected: ['packages/core'],
+          ok: true,
+          build: [],
+          timedOut: [],
+        },
+        // The two newest clauses need their own witnesses too: a corrupted
+        // stamp must refuse here, not steer the nothing-to-resume message
+        // off a non-boolean truthiness ('"buildOnly": "yes"' fails
+        // `=== true` and would read as a completed zero-suite run).
+        {
+          toolchain: 'npm',
+          affected: ['packages/core'],
+          ok: true,
+          buildOnly: 'yes',
+          test: [],
+          build: [],
+          timedOut: [],
+        },
+        {
+          toolchain: 'npm',
+          affected: ['packages/core'],
+          ok: 'false',
+          test: [],
+          build: [],
+          timedOut: [],
+        },
+        {
+          toolchain: 'npm',
+          affected: ['packages/core'],
+          test: [],
+          build: [],
+          timedOut: [],
+        },
       ]) {
         writeFileSync(outPath, JSON.stringify(partial));
         expect(() =>