fix(review): report what the transcripts prove; build the roster in one call

packages/cli/src/commands/review/check-coverage.ts

@@ -39,6 +39,8 @@ import {
   coverageFromTranscripts,
   TranscriptsUnavailableError,
 } from './lib/coverage.js';
+import { promptRecordDir } from './lib/prompt-record.js';
+import { shellQuotePath } from './lib/shell-quote.js';
 
 interface CheckCoverageArgs {
   plan: string;
@@ -110,8 +112,8 @@ function runCheckCoverage(args: CheckCoverageArgs): void {
         `that never named the diff file — ${report.blindAgents.join(', ')}. They ` +
         `could not have read the diff, whatever they returned. Do NOT relaunch ` +
         `them as they are: a second blind agent reads no more than the first. ` +
-        `Build each prompt with \`qwen review agent-prompt --plan <plan> ` +
-        `--chunk <id>\` and pass it verbatim.`,
+        `Build each prompt with \`"\${QWEN_CODE_CLI:-qwen}" review agent-prompt ` +
+        `--plan ${shellQuotePath(args.plan)} --chunk <id>\` and pass it verbatim.`,
     );
   }
   // The prompt was built in code and then edited on the way to the agent. Nothing
@@ -124,19 +126,52 @@ function runCheckCoverage(args: CheckCoverageArgs): void {
         `\`agent-prompt\` prints a prompt to be passed VERBATIM; a summary of it is ` +
         `not it. The last run to paraphrase one dropped the rule against reciting a ` +
         `stock sentence and replaced the project's review rules with three sentences ` +
-        `of its own. Re-run \`qwen review agent-prompt\` and pass its output ` +
+        `of its own. Re-run \`"\${QWEN_CODE_CLI:-qwen}" review agent-prompt\` and ` +
+        `pass its output ` +
         `unedited — copy it, do not retype it.`,
     );
   }
   // The one failure no other check in this file can see. Every other question is
-  // asked of an agent that ran; an agent that never ran leaves nothing to ask.
+  // asked of an agent whose transcript exists; a brief that never arrived leaves
+  // nothing to ask it of.
   if (report.missingRoles.length > 0) {
     writeStderrLine(
-      `ERROR: ${report.missingRoles.length} required agent(s) never ran — ` +
-        `${report.missingRoles.join('; ')}. The roster comes from the plan, not ` +
-        `from anything this run wrote. A dimension nobody reviewed cannot be ` +
-        `certified clean: build each prompt with the call named above and launch ` +
-        `an agent with it, verbatim.`,
+      // No count: when no role was briefed at all, `missingRoles` collapses to one
+      // line covering the whole roster, and a leading "1" would undercount it by the
+      // size of the review.
+      `ERROR: required briefs never reached their agents — ` +
+        `${report.missingRoles.join('; ')}. The roster comes from the ` +
+        `plan, not from anything this run wrote. Writing the launch yourself does ` +
+        `not substitute: the agent runs and may even find something, but the ` +
+        `severity bar, the finding format and this project's rules live in the ` +
+        `brief it was never given, and a dimension reviewed without them cannot ` +
+        `be certified clean. Build every required prompt in one call — ` +
+        `\`"\${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan ${shellQuotePath(args.plan)} --roster\` ` +
+        // No "the label above names the role" here: when no role was briefed at
+        // all, the report collapses to one line that names none of them.
+        `— and launch one agent per block it prints, verbatim. To rebuild a ` +
+        `single one: \`--role <n>\` (a per-file role takes \`--file <path>\`), ` +
+        `or \`--chunk <id>\` for a chunk agent. Pass \`--rules <rules file>\` ` +
+        `whenever Step 2 found any — a rebuild without it writes a rules-free ` +
+        `brief.\n` +
+        // The label is for humans; the selector is for the rebuild command. A
+        // label like `Test coverage matrix (whole-diff)` does not say
+        // `--role test-matrix`, and a wrong guess costs a full-roster rerun.
+        // Includes built-but-never-launched roles, whose lighter fix is
+        // relaunching the printed prompt — the clause keeps an operator from
+        // hesitating over the heavier one: a rebuild is idempotent.
+        (report.missingRoleSelectors.length > 0
+          ? `Exact selectors: ${report.missingRoleSelectors.join('; ')} ` +
+            `(rebuilding an already-built role is safe — the record is ` +
+            `overwritten with the same block)\n`
+          : '') +
+        // Where it looked, because "the builder never ran" and "the builder ran
+        // against a different --plan" are indistinguishable from a missing file and
+        // are fixed differently. The record dir hangs off the plan path as given, so
+        // a relative --plan resolves against the caller's cwd — and Steps 2-6 are
+        // run from inside the worktree. Printing the directory turns a silent
+        // disagreement about where the records live into one a reader can see.
+        `Looked for them in: ${promptRecordDir(args.plan)}`,
     );
   }
   if (report.unreadBriefs.length > 0) {
@@ -162,8 +197,8 @@ function runCheckCoverage(args: CheckCoverageArgs): void {
       'NOTE: a chunk counts as read when an agent was pointed at its lines AND ' +
         'the harness recorded that agent opening the diff. An agent handed the ' +
         'diff with no line ranges covers nothing. Build every whole-diff ' +
-        "agent's prompt with `qwen review agent-prompt --plan <plan> " +
-        '--whole-diff` and paste it verbatim ahead of its brief.',
+        'agent\'s prompt with `"${QWEN_CODE_CLI:-qwen}" review agent-prompt ' +
+        `--plan ${shellQuotePath(args.plan)} --whole-diff\` and paste it verbatim ahead of its brief.`,
     );
   }
   if (report.idleAgents.length > 0) {

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

@@ -28,6 +28,7 @@ vi.mock('../../utils/stdioHelpers.js', () => ({
   writeStdoutLine: vi.fn(),
   writeStderrLine: vi.fn(),
 }));
+import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';
 
 const MODEL = 'test-model';
 
@@ -557,6 +558,24 @@ describe('composeReview — stacked states compose, none erased', () => {
     expect(r.body).not.toContain('no blockers');
   });
 
+  it('reads as a sentence when no role was briefed at all', () => {
+    // The register this lands in matters as much as the fact. On #7012 the public
+    // CHANGES_REQUESTED body was twelve lines of the review's own plumbing, each
+    // naming an internal command (`agent-prompt --role 2`) the PR author has no way
+    // to run, while the two Criticals that needed acting on sat inline below. The
+    // author needs one thing from this: which of the review they should not trust.
+    const gap =
+      'every dimension — none of the 12 required agents was launched with a ' +
+      'prompt this skill built, so this diff was reviewed, if at all, from prompts ' +
+      'the run wrote for itself: the severity bar, the finding format and this ' +
+      "project's own rules never reached an agent";
+    const r = composeReview(base({ unreviewedDimensions: [gap] }));
+
+    expect(r.body).toContain(`Not reviewed: ${gap}.`);
+    expect(r.body).not.toMatch(/agent-prompt|--role|--chunk/);
+    expect(r.event).not.toBe('APPROVE'); // it still caps, as it always did
+  });
+
   it('RC with body Criticals plus unread scope carries both disclosures', () => {
     const r = composeReview(
       base({
@@ -901,12 +920,20 @@ describe('coverage is recomputed, never accepted', () => {
     });
     expect(r.event).not.toBe('APPROVE');
     expect(r.body).toContain('read nothing');
+    // The repair rides the remediation channel — a body disclosure whose FIX
+    // silently vanished is the exact state that channel exists to prevent, and
+    // without this line, deleting the idle push would fail no test.
+    expect(r.remediation.join(' ')).toMatch(
+      /idle agents: relaunch each with the same printed prompt/,
+    );
   });
 
   it('names a blind launch as itself, not as a whiff', () => {
     // An agent whose prompt never named the diff could not have read it, and
     // relaunching it produces another agent that cannot either. The prompt is the
-    // defect, and the body has to say so or the reader will retry forever.
+    // defect. The body says what happened — to the PR author, who cannot run
+    // `agent-prompt` — and the rebuild command rides in `remediation`, which the
+    // command prints to stderr for the orchestrator.
     const r = composeReview({
       criticalsInline: 0,
       suggestionsInline: 0,
@@ -916,7 +943,157 @@ describe('coverage is recomputed, never accepted', () => {
     });
     expect(r.event).not.toBe('APPROVE');
     expect(r.body).toContain('never named the diff file');
-    expect(r.body).toContain('agent-prompt');
+    expect(r.body).not.toContain('agent-prompt');
+    expect(r.remediation.join(' ')).toContain(
+      '"${QWEN_CODE_CLI:-qwen}" review agent-prompt',
+    );
+    expect(r.remediation.join(' ')).toMatch(/do not relaunch the old prompt/);
+    // Blind agents read nothing, so the chunks they owned are also chunks
+    // nobody read — that disclosure's repair must ride along too. Deleting the
+    // missingReceipts push used to fail no test: no fixture reached it.
+    expect(r.body).toContain('no agent reported covering');
+    expect(r.remediation.join(' ')).toMatch(
+      /chunks nobody read: build each with/,
+    );
+  });
+
+  it('a missing-roles gap has a FIX on the remediation channel', () => {
+    // The blind agents got one; the sibling categories did not, and a body
+    // disclosure with no repair command is how #7012's orchestrator ended at
+    // "the agents clearly did their job". Here the test-matrix brief was never
+    // built: the body says what cannot be certified, in the author's register,
+    // and the remediation names the roster call, in the operator's.
+    // (Blind agents are pinned in the test above; the remaining three
+    // categories in the test below — between them, every category that
+    // discloses is asserted to repair.)
+    const p = plan({ step45: false });
+    transcript('a1', goodPrompt(1), { toolCalls: 3 });
+    transcript('a2', goodPrompt(2), { toolCalls: 2 });
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    // recordMatrix(p) deliberately absent — the roster still requires it.
+    recordStep45(p);
+
+    const r = composeReview({
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      planPath: p,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).not.toBe('APPROVE');
+    expect(r.body).toContain('no record shows its brief reaching an agent');
+    expect(r.body).not.toMatch(/agent-prompt|--roster|--role/);
+    // The FIX names the run's REAL plan path — a `<plan>` placeholder pasted
+    // literally parses as a shell redirection.
+    expect(r.remediation.join(' ')).toContain(
+      `"\${QWEN_CODE_CLI:-qwen}" review agent-prompt --plan '${p}' --roster`,
+    );
+  });
+
+  it('rewritten, unread-brief and never-opened gaps each carry their FIX too', () => {
+    // The categories the missing-roles test above does not reach — without this,
+    // dropping any one of their `remediation.push` calls would fail no test, which
+    // is precisely the disclosure-without-repair state the channel exists to
+    // prevent. One plan, three defects: chunk 1's agent ran on a hand-written
+    // prompt (rewritten), chunk 2's got the built prompt and never opened its
+    // brief (unread), and a third agent got chunk 1's built prompt and never
+    // opened the diff (unopened).
+    const p = plan();
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    recordMatrix(p); // roster satisfied: these three categories, nothing else
+    transcript(
+      'a1',
+      `You are reviewing chunk 1 of 2.\n` +
+        `read_file(file_path="${DIFF}", offset=0, limit=100)`,
+      { toolCalls: 3 },
+    );
+    transcript('a2', goodPrompt(2), { toolCalls: 3, opens: [] });
+    transcript('a3', goodPrompt(1), {
+      toolCalls: 0,
+      opens: [briefPath(p, 'chunk-1')],
+    });
+
+    const r = composeReview({
+      criticalsInline: 0,
+      suggestionsInline: 0,
+      planPath: p,
+      env: ENV,
+      modelId: MODEL,
+    });
+    expect(r.event).not.toBe('APPROVE');
+    const fixes = r.remediation.join(' ');
+    expect(fixes).toMatch(/rewritten launches: re-run/);
+    expect(fixes).toMatch(/unread briefs: relaunch/);
+    expect(fixes).toMatch(/agents that never opened the diff: relaunch/);
+    // And none of the three disclosures drags a command into the body.
+    expect(r.body).not.toMatch(/agent-prompt|--roster|--chunk/);
+  });
+
+  it('the handler prints every FIX to stderr, before the verdict, never to stdout', () => {
+    // The array on the result is data; the command boundary is the interface the
+    // orchestrator actually reads. Without this, rerouting FIX lines to stdout
+    // (corrupting the JSON callers parse) or printing them after `Verdict:` (so
+    // a reader that stops at the verdict never sees them) would stay green.
+    const p = plan({ step45: false });
+    transcript('a1', goodPrompt(1), { toolCalls: 3 });
+    transcript('a2', goodPrompt(2), { toolCalls: 2 });
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    recordStep45(p); // roster misses the test matrix → one repairable gap
+    const input = join(dir, 'input.json');
+    writeFileSync(
+      input,
+      JSON.stringify({
+        criticalsInline: 0,
+        suggestionsInline: 0,
+        planPath: p,
+        modelId: MODEL,
+      }),
+    );
+
+    const prevDir = process.env['QWEN_CODE_PROJECT_DIR'];
+    const prevSession = process.env['QWEN_CODE_SESSION_ID'];
+    process.env['QWEN_CODE_PROJECT_DIR'] = ENV['QWEN_CODE_PROJECT_DIR'];
+    process.env['QWEN_CODE_SESSION_ID'] = ENV['QWEN_CODE_SESSION_ID'];
+    try {
+      vi.mocked(writeStderrLine).mockClear();
+      vi.mocked(writeStdoutLine).mockClear();
+      (composeReviewCommand.handler as (a: Record<string, unknown>) => void)({
+        input,
+      });
+
+      const stderr = vi
+        .mocked(writeStderrLine)
+        .mock.calls.map((c) => String(c[0]));
+      const fixIdx = stderr.findIndex((l) => l.startsWith('FIX: '));
+      const verdictIdx = stderr.findIndex((l) => l.startsWith('Verdict:'));
+      expect(fixIdx).toBeGreaterThanOrEqual(0);
+      expect(verdictIdx).toBeGreaterThan(fixIdx);
+      // And stdout stays parseable JSON — no FIX line in it.
+      const stdout = vi
+        .mocked(writeStdoutLine)
+        .mock.calls.map((c) => String(c[0]))
+        .join('\n');
+      expect(() => JSON.parse(stdout)).not.toThrow();
+      expect(stdout).not.toContain('FIX: ');
+      // The composed JSON persists the EXACT verdict line, so Step 8's archived
+      // report copies it instead of re-deriving a lossy one from event+cappedBy
+      // (a presubmit downgrade depends on fields that pair does not carry).
+      const parsedOut = JSON.parse(stdout) as { verdictLine?: string };
+      expect(parsedOut.verdictLine).toMatch(/^Verdict: /);
+      const printedVerdict = vi
+        .mocked(writeStderrLine)
+        .mock.calls.map((c) => String(c[0]))
+        .find((l) => l.startsWith('Verdict:'));
+      expect(parsedOut.verdictLine).toBe(printedVerdict);
+    } finally {
+      if (prevDir === undefined) delete process.env['QWEN_CODE_PROJECT_DIR'];
+      else process.env['QWEN_CODE_PROJECT_DIR'] = prevDir;
+      if (prevSession === undefined) delete process.env['QWEN_CODE_SESSION_ID'];
+      else process.env['QWEN_CODE_SESSION_ID'] = prevSession;
+    }
   });
 
   it('caps when the transcripts cannot be read at all — and says so', () => {
@@ -964,7 +1141,9 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff
     });
     expect(r.event).toBe('COMMENT');
     expect(r.cappedBy).toContain('unreviewed-dimension');
-    expect(r.body).toMatch(/reverse audit — no auditor ran/);
+    expect(r.body).toMatch(
+      /reverse audit — no auditor was launched with a prompt this skill builds/,
+    );
   });
 
   it('discloses that posted findings were not verified when Step 4 was skipped', () => {
@@ -1045,6 +1224,7 @@ describe('verdictLine — the terminal verdict, and its dangling colon', () => {
       cappedBy: [],
       downgraded: false,
       downgradedFrom: null,
+      remediation: [],
       ...over,
     });