fix(review): one disclosure per subject — dedupe Not-reviewed, collapse an all-built-none-launched roster

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

@@ -818,6 +818,56 @@ describe('the roster — who should have been here', () => {
     expect(r.missingRoles[0]).not.toMatch(/--role/);
   });
 
+  it("keeps per-role entries when every prompt was built and none was launched — the collapse is compose's job", () => {
+    // The first cut collapsed this shape HERE, into one "the run stopped at
+    // the prompt builder" line — and misfired: candidatesOf is also all-empty
+    // when every agent ran on a REWRITTEN prompt, so the aggregate claimed
+    // nothing launched beside forty-three rewritten-launch disclosures that
+    // said otherwise. Coverage now reports per role, structurally
+    // (`disclosures`), and compose-review groups same-reason subjects into
+    // the one sentence — after the caller's echoes have been deduped against
+    // the very subjects a coverage-side collapse would have discarded.
+    const p = planPr();
+    for (const f of readdirSync(join(dir, 'subagents', 'S1'))) {
+      rmSync(join(dir, 'subagents', 'S1', f), { force: true });
+    }
+    transcript('stray', wholeDiff(), { calls: 8 });
+
+    const r = coverageFromTranscripts(p, ENV);
+    expect(r.ok).toBe(false);
+    const roster = requiredAgents(
+      JSON.parse(readFileSync(p, 'utf8')) as RosterPlan,
+    );
+    expect(roster.length).toBeGreaterThan(1);
+    expect(r.missingRoles).toHaveLength(roster.length);
+    expect(r.missingRoleSelectors).toHaveLength(roster.length);
+    // Structural twins, one per role, all sharing the one reason — what the
+    // compose-side grouping turns into a single sentence.
+    const notLaunched = r.disclosures.filter(
+      (d) =>
+        d.reason ===
+        'its prompt was built, but no agent on record was launched with it',
+    );
+    expect(notLaunched).toHaveLength(roster.length);
+    expect(new Set(notLaunched.map((d) => d.subject)).size).toBe(roster.length);
+  });
+
+  it('keeps the per-role not-launched text when only SOME launches are missing', () => {
+    // The collapse must not swallow the partial case: one unlaunched role
+    // beside launched siblings is that role's own line, naming it.
+    const p = planPr();
+    rmSync(join(dir, 'subagents', 'S1', 'agent-r-1c.jsonl'), { force: true });
+    transcript('sec', wholeDiff(), { calls: 8 });
+
+    const r = coverageFromTranscripts(p, ENV);
+    const gap = r.missingRoles.join(' ');
+    expect(gap).toContain('Cross-file tracer');
+    expect(gap).toContain(
+      'its prompt was built, but no agent on record was launched with it',
+    );
+    expect(gap).not.toContain('every dimension');
+  });
+
   it('tells the operator where it looked, so a wrong --plan is not a missing file', () => {
     // "The builder never ran" and "the builder ran against a different --plan" reach
     // this check as the same thing: an absent record. They are fixed differently, so

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

@@ -421,9 +421,12 @@ describe('composeReview — 422 recovery (round-7 Critical #1 & round-6: verdict
     // Before the 422: S=2. After dropping both anchors: recompose.
     const r = composeReview(base({ suggestionsDiscarded: 2 }));
     expect(r.event).toBe('COMMENT');
+    // Self-contained for the PR author — the old text said "see the terminal
+    // output", a terminal only the operator has.
     expect(r.body).toContain(
-      '2 Suggestion-level finding(s) could not be anchored to the diff; see the terminal output.',
+      '2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.',
     );
+    expect(r.body).not.toContain('terminal output');
     // Nothing is inline — the body must not claim otherwise while the
     // discarded sentence says the opposite (round-9: `s` included discarded).
     expect(r.body).not.toContain('Suggestions are inline.');
@@ -1071,6 +1074,179 @@ describe('composeReviewCommand handler (the CLI glue)', () => {
 });
 
 describe('coverage is recomputed, never accepted', () => {
+  it('does not repeat a disclosure the caller echoed back — one subject, one line', () => {
+    // #7188: the orchestrator pasted the gate's own gap sentences into
+    // `unreviewedDimensions`, coverage recomputed the same gaps, and the
+    // public body carried every disclosure twice — 22 "Not reviewed" clauses
+    // for 11 roles. The chunk list already dedupes by its `chunk <id>`
+    // prefix; the role list dedupes by label now, and when both sides name
+    // the same subject the coverage-derived text wins.
+    const p = plan();
+    transcript('a1', goodPrompt(1), { toolCalls: 3 });
+    transcript('a2', goodPrompt(2), { toolCalls: 2 });
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    // test-matrix is required by this plan's roster and never built → exactly
+    // one coverage-derived role gap.
+    const label = 'Test coverage matrix (whole-diff)';
+    const r = composeReview({
+      planPath: p,
+      env: ENV,
+      modelId: MODEL,
+      unreviewedDimensions: [
+        `${label} — the run described this gap in its own words`,
+        'a subject only the caller noticed — the auditor returned nothing twice',
+      ],
+    });
+    // One clause for the shared subject — and it is the machine's sentence,
+    // not the caller's paraphrase.
+    expect(r.body.split(label)).toHaveLength(2);
+    expect(r.body).toContain('no record shows its brief reaching an agent');
+    expect(r.body).not.toContain('described this gap in its own words');
+    // A subject the coverage recomputation cannot see survives untouched.
+    expect(r.body).toContain(
+      'a subject only the caller noticed — the auditor returned nothing twice',
+    );
+  });
+
+  it('says a shared cause once, with every subject on the one sentence', () => {
+    // #7166's posted body: ninety-nine disclosure paragraphs over FOUR causes
+    // — forty-three chunks all rewritten, fifty-five roles all unlaunched —
+    // with the six real findings buried beneath. Same cause, one sentence.
+    const p = plan();
+    // Both chunk launches rewritten: recorded prompts exist, the agents ran
+    // on hand-written prompts that DROP the brief line — an add-only wrap
+    // would rightly pass the delivery check.
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    transcript(
+      'a1',
+      `You are reviewing chunk 1 of 2.\nread_file(file_path="${DIFF}", offset=0, limit=100)`,
+      { toolCalls: 2 },
+    );
+    transcript(
+      'a2',
+      `You are reviewing chunk 2 of 2.\nread_file(file_path="${DIFF}", offset=100, limit=100)`,
+      { toolCalls: 2 },
+    );
+    const r = composeReview({ planPath: p, env: ENV, modelId: MODEL });
+    const reason = 'launched with a prompt that is not the one the CLI built';
+    // One clause for the shared cause — not one per chunk…
+    expect(r.body.split(reason)).toHaveLength(2);
+    // …and both subjects ride it.
+    expect(r.body).toMatch(
+      new RegExp(`Not reviewed: [^.]*chunk 1[^.]*chunk 2[^.]*— ${reason}\\.`),
+    );
+  });
+
+  it('an all-rewritten roster never claims nothing launched — precise cause, no contradicting aggregate', () => {
+    // The first cut collapsed all-empty verbatim matches into "the run
+    // stopped at the prompt builder" — but candidatesOf is also all-empty
+    // when every agent RAN on a rewritten prompt, and the aggregate then
+    // contradicted the rewritten-launch disclosures beside it. Reproduced
+    // and refused: both chunks rewritten, the whole-diff role unlaunched —
+    // each cause its own sentence, no "every dimension" claim anywhere.
+    const p = plan();
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    transcript(
+      'a1',
+      `You are reviewing chunk 1 of 2.\nread_file(file_path="${DIFF}", offset=0, limit=100)`,
+      { toolCalls: 2 },
+    );
+    transcript(
+      'a2',
+      `You are reviewing chunk 2 of 2.\nread_file(file_path="${DIFF}", offset=100, limit=100)`,
+      { toolCalls: 2 },
+    );
+    const r = composeReview({ planPath: p, env: ENV, modelId: MODEL });
+    expect(r.body).toMatch(
+      /Not reviewed: [^.]*chunk 1[^.]*chunk 2[^.]*— launched with a prompt that is not the one the CLI built\./,
+    );
+    expect(r.body).not.toContain('every dimension');
+    expect(r.body).not.toContain('stopped at the prompt builder');
+    // And the chunks appear under their PRECISE cause only — to the roster
+    // they are also requirements with no verbatim launch, and repeating them
+    // under that vaguer cause would claim nothing launched about agents that
+    // demonstrably ran.
+    expect(r.body).not.toContain('no agent on record was launched with it');
+  });
+
+  it('a reason carrying its own em-dash neither garbles the subject nor duplicates the line', () => {
+    // Reasons are free-form — internal failures interpolate raw error
+    // messages — so a subject/reason boundary reparsed from rendered prose
+    // regroups exactly the entries it garbles. The entries are structural
+    // now; the caller's echo of a dashed line still dedupes, by prefix
+    // against the known subject.
+    const p = plan();
+    const r = composeReview({
+      planPath: p,
+      // Transcripts unreadable: the coverage AND verification reasons both
+      // interpolate an error message — with an em-dash of their own.
+      env: {
+        QWEN_CODE_PROJECT_DIR: join(dir, 'nowhere — missing'),
+        QWEN_CODE_SESSION_ID: 'S1',
+      },
+      unreviewedDimensions: [
+        'coverage — could not read the transcripts — echoed back by the caller',
+      ],
+      modelId: MODEL,
+    });
+    // One coverage clause — the caller's dashed echo deduped by subject
+    // prefix, the machine's own text rendered once, subject intact.
+    expect(r.body.match(/Not reviewed: coverage/g)).toHaveLength(1);
+    expect(r.body).not.toContain('echoed back by the caller');
+  });
+
+  it('caller echoes of per-role gaps fold into the one grouped sentence — the #7188 shape end to end', () => {
+    // The coverage-side collapse discarded the per-role subjects before the
+    // caller's echoes could collide with them, so the body carried the
+    // caller's per-role sentences PLUS an overlapping aggregate. Per-role
+    // subjects now survive to the dedup, and the grouping makes the one
+    // sentence afterwards.
+    const p = plan();
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    // Chunks reviewed properly; the whole-diff role built but never launched.
+    transcript('a1', goodPrompt(1), { toolCalls: 3 });
+    transcript('a2', goodPrompt(2), { toolCalls: 2 });
+    const label = 'Test coverage matrix (whole-diff)';
+    const r = composeReview({
+      planPath: p,
+      env: ENV,
+      unreviewedDimensions: [
+        `${label} — its prompt was built, but no agent on record was launched with it`,
+      ],
+      modelId: MODEL,
+    });
+    expect(r.body.split(label)).toHaveLength(2);
+    expect(
+      r.body.match(/no record shows its brief reaching an agent/g) ?? [],
+    ).toHaveLength(1);
+  });
+
+  it('does not merge two invariant files under one label — the em-dash is part of the subject', () => {
+    // An invariant agent's label legitimately carries an em-dash segment
+    // (`Invariant agent A … — src/foo.ts`). A first-dash dedup key would
+    // merge two files into one subject and silently drop a disclosure.
+    const p = plan();
+    transcript('a1', goodPrompt(1), { toolCalls: 3 });
+    transcript('a2', goodPrompt(2), { toolCalls: 2 });
+    recordBuilt(p, 1);
+    recordBuilt(p, 2);
+    const r = composeReview({
+      planPath: p,
+      env: ENV,
+      modelId: MODEL,
+      unreviewedDimensions: [
+        'Invariant agent A: state, timers — src/a.ts — the agent whiffed twice',
+        'Invariant agent A: state, timers — src/b.ts — the agent whiffed twice',
+      ],
+    });
+    expect(r.body).toContain('src/a.ts');
+    expect(r.body).toContain('src/b.ts');
+  });
+
   it('caps when no plan is given — nothing can show the diff was read', () => {
     const r = composeReview({
       criticalsInline: 0,