Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 52 additions & 3 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1238,6 +1238,52 @@ describe('coverage is recomputed, never accepted', () => {
).toHaveLength(1);
});

it('a chunk whose launch failure is already disclosed leaves the nobody-read sentence — cause, not consequence twice', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] No test covers the partial-overlap case — some missing receipts disclosed, some not — which is the only scenario where unexplainedReceipts differs from both [] and missingReceipts. A future edit to the sentence template accidentally rendering missingReceipts instead of unexplainedReceipts would pass both existing tests unchanged: in the "all disclosed" test the guard prevents the push, and in the "none disclosed" test both lists are identical. Concrete cost: the exact #7166 double-disclosure this PR fixes could regress silently.

Consider adding a test with three chunks: chunk 1 built-but-not-launched (disclosed), chunk 2 reviewed, chunk 3 neither disclosed nor reviewed. Assert r.body contains chunk 3 under the "nobody read them" sentence but does NOT list chunk 1 in that sentence.

— qwen3.8-max-preview via Qwen Code /review

// #7166's first post-grouping body carried seventeen chunks in BOTH the
// "nobody read them" sentence and the not-launched roster sentence: the
// consequence restated beside its cause. The cap and remediation keep the
// full list; only the posted sentence dedupes.
const p = plan();
recordBuilt(p, 1);
recordBuilt(p, 2);
// chunk 2 reviewed properly; chunk 1 built and never launched — its
// territory therefore unread, and its cause on record.
transcript('a2', goodPrompt(2), { toolCalls: 2 });
const r = composeReview({ planPath: p, env: ENV, modelId: MODEL });
expect(r.cappedBy).toContain('chunk-nobody-read'); // the cap keeps the fact
expect(r.remediation.join(' ')).toContain('chunks nobody read');
expect(r.body).toContain('chunk 1');
// …but only under its cause: no second sentence restating the consequence.
expect(r.body).not.toContain('nobody read them');
});

it('keeps the nobody-read sentence for a chunk with no disclosed cause', () => {
// The 3A shape: chunks are not roster requirements, so an unread chunk has
// no launch-side disclosure to explain it — the receipt sentence is the
// only place the author learns those lines went unread.
const p = join(dir, 'plan-3a.json');
writeFileSync(
p,
JSON.stringify({
diffPathAbsolute: DIFF,
srcDiffLines: 100,
diffLines: 200,
files: [
{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false },
],
chunks: [
{ id: 1, startLine: 1, endLine: 100 },
{ id: 2, startLine: 101, endLine: 200 },
],
}),
);
const old = new Date(2020, 0, 1);
utimesSync(p, old, old);
const r = composeReview({ planPath: p, env: ENV, modelId: MODEL });
expect(r.body).toContain('nobody read them');
expect(r.body).toMatch(/chunk 1, chunk 2 — no agent reported covering/);
});

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
Expand Down Expand Up @@ -1311,9 +1357,12 @@ describe('coverage is recomputed, never accepted', () => {
);
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');
// nobody read — the CAP and the repair ride along, while the posted body
// says it once, under the cause: the blind sentence already explains the
// unread territory, and restating it as "nobody read them" beside it was
// the #7166 double-disclosure.
expect(r.cappedBy).toContain('chunk-nobody-read');
expect(r.body).not.toContain('no agent reported covering');
expect(r.remediation.join(' ')).toMatch(
/chunks nobody read: build each with/,
);
Expand Down
23 changes: 19 additions & 4 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -576,11 +576,26 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult {
// as a line too long to read, which is true of an *uncoverable* chunk and a
// fabrication about one nobody receipted — the author would be told the diff
// defeated the reader, when in fact no reader turned up.
notReviewedParts.push(
`Not reviewed: ${missingReceipts
.map((id) => `chunk ${id}`)
.join(', ')} — no agent reported covering these; nobody read them.`,
//
// But a chunk whose disclosure entry already says WHY it went unread — its
// launch never happened, or happened on a rewritten prompt — is one fact,
// not two: "nobody read chunk 2" beside "chunk 2 — its prompt was built,
// but no agent on record was launched with it" restates the consequence
// next to its cause, and #7166's first post-grouping body carried
// seventeen chunks twice exactly this way. The cap and the remediation
// above keep the FULL list — only the posted sentence dedupes, and only
// for subjects another sentence already explains.
const disclosedSubjects = new Set(coverageEntries.map((e) => e.subject));
const unexplainedReceipts = missingReceipts.filter(
(id) => !disclosedSubjects.has(`chunk ${id}`),
);
if (unexplainedReceipts.length > 0) {
notReviewedParts.push(
`Not reviewed: ${unexplainedReceipts
.map((id) => `chunk ${id}`)
.join(', ')} — no agent reported covering these; nobody read them.`,
);
}
}
if (uncoverable.length > 0) {
notReviewedParts.push(
Expand Down
Loading