From c2c9a051282ef6709887231e342c194bc5bc4332 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 10 Aug 2026 17:15:08 +0800 Subject: [PATCH 1/6] fix(cli): word review gap disclosures so they cannot read as contradictions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A posted review body on #8811 opened "Reviewed. Not reviewed: This PR narrows the daemon-marker check from a truthy tes..." and readers could not tell whether the PR had been reviewed at all. The opener certified a part of the diff while the disclosure named an agent that never opened it — both true, but the pair read as a self-contradiction, and the disclosure subject (a launch-prompt fragment truncated mid-word) looked like a claim about the whole PR. - When "Not reviewed:" clauses follow, the opener now says "Reviewed, with gaps disclosed below." The certifying and zero-certified openers keep their exact wording. - Agent-derived subjects (the truncated prompt line used when an agent is tied to no chunk) render quoted — they are names, not sentences. - That truncation stops at a word boundary instead of mid-word. Verdicts, caps, dedup and the certification check are untouched: the internal subject stays the unquoted label and every structural check keys on it. --- .../commands/review/compose-review.test.ts | 46 +++++++++++++++++-- .../cli/src/commands/review/compose-review.ts | 28 ++++++++++- .../cli/src/commands/review/lib/coverage.ts | 9 +++- 3 files changed, 76 insertions(+), 7 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 5497a0314dd..9fcf7e75412 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -920,7 +920,12 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve base({ suggestionsInline: 1, unreviewedDimensions: ['security'] }), ); expect(r.event).toBe('COMMENT'); - expect(r.body).toContain('Reviewed. Suggestions are inline.'); + // The gap disclosure follows, so the opener says so — a bare "Reviewed." + // two words above "Not reviewed:" read as the body contradicting itself + // (#8811). + expect(r.body).toContain( + 'Reviewed, with gaps disclosed below. Suggestions are inline.', + ); expect(r.body).not.toContain('no blockers'); }); }); @@ -1089,7 +1094,7 @@ describe('composeReview — stacked states compose, none erased', () => { // downgradeApprove did not fire (base event was COMMENT), so no sentence… expect(r.body).not.toContain('Downgraded'); // …but every disclosure is present exactly once, and nothing certifies. - expect(r.body).toContain('Reviewed.'); + expect(r.body).toContain('Reviewed, with gaps disclosed below.'); expect(r.body).toContain('Suggestions are inline.'); expect(r.body).toContain('1 Suggestion-level finding(s)'); expect(r.body).toContain('Unresolved, please confirm:'); @@ -2089,16 +2094,18 @@ describe('coverage is recomputed, never accepted', () => { expect(r.body).not.toContain('Reviewed.'); }); - it('keeps the "Reviewed." opener while any chunk is certified', () => { + it('keeps a certifying opener while any chunk is certified — and names the gaps it carries', () => { // chunk 1 built and never launched; chunk 2 reviewed properly. A partial - // gap is a disclosure, not a zero-certification. + // gap is a disclosure, not a zero-certification — and the opener says the + // disclosures follow, so "Reviewed." never sits beside "Not reviewed:" + // (#8811). const p = plan(); recordBuilt(p, 1); recordBuilt(p, 2); recordMatrix(p); transcript('a2', goodPrompt(2), { toolCalls: 2 }); const r = composeReview({ planPath: p, env: ENV, modelId: MODEL }); - expect(r.body).toContain('Reviewed.'); + expect(r.body).toContain('Reviewed, with gaps disclosed below.'); expect(r.body).not.toContain('could not certify'); }); @@ -2157,6 +2164,32 @@ describe('coverage is recomputed, never accepted', () => { ); }); + it('quotes a prose agent label — it is the agent’s name, not a claim about the PR', () => { + // #8811: a whole-diff agent (no `chunk N of M` in its prompt) was + // disclosed by the truncated first line of its launch prompt, rendered + // bare — "Not reviewed: This PR narrows the daemon-marker check from a + // truthy tes..." read as a sentence about the whole PR, not the name of + // the one agent that failed. Quotes say which it is, and the truncation + // stops at a word boundary instead of mid-word. + transcript( + 'p1', + 'This PR narrows the daemon-marker check from a truthy test to an exact one\n' + + `read_file(file_path="${DIFF}")`, + { toolCalls: 0 }, + ); + const r = composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: plan(), + env: ENV, + modelId: MODEL, + }); + expect(r.body).toContain( + 'Not reviewed: "This PR narrows the daemon-marker check from a truthy..."', + ); + expect(r.body).not.toContain('truthy tes...'); + }); + 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 @@ -2934,6 +2967,9 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)' expect(r.body).toContain('未审查:全 diff 测试覆盖检查——'); // The zh sentence carries the translated reason, not the English one. expect(r.body).toContain('没有记录表明它的 brief 到达过任何 agent'); + // The gap opener, in both halves (#8811). + expect(r.body).toContain('Reviewed, with gaps disclosed below.'); + expect(r.body).toContain('已审查,未覆盖部分见下方披露。'); }); it('quotes untranslatable caller text as-is in both halves', () => { diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 7ec573d4047..96ecfca040d 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -584,6 +584,7 @@ function composeReviewBody( for (const label of cov.idleAgents) { coverageEntries.push({ subject: label, + ...quotedAgentSubject(label), reason: 'the agent made no tool call: it read nothing', reasonZh: '该 agent 未发起任何工具调用:它什么都没读', }); @@ -605,6 +606,7 @@ function composeReviewBody( for (const label of cov.blindAgents) { coverageEntries.push({ subject: label, + ...quotedAgentSubject(label), reason: 'launched with a prompt that never named the diff file, so it ' + 'could not have read it', @@ -626,6 +628,7 @@ function composeReviewBody( for (const label of cov.unopenedAgents) { coverageEntries.push({ subject: label, + ...quotedAgentSubject(label), reason: 'pointed at diff lines it never opened: it made tool calls, but ' + 'none of them read the diff', @@ -1405,6 +1408,10 @@ function composeReviewBody( coverageEntries.some((e) => e.subject === 'coverage') || (plannedChunks.length > 0 && coveredChunks.every((id) => disclosedChunkIds.has(id))); + // A bare "Reviewed." two words before "Not reviewed:" reads as the body + // contradicting itself — the shape that confused readers on #8811. When + // disclosures follow, the opener says so; the certifying and the + // zero-certified openers above keep their exact wording. clauses.push( nothingCertified ? { @@ -1413,7 +1420,12 @@ function composeReviewBody( } : canCertify ? { en: 'Reviewed — no blockers.', zh: '已审查——无阻断问题。' } - : { en: 'Reviewed.', zh: '已审查。' }, + : notReviewedParts.length > 0 + ? { + en: 'Reviewed, with gaps disclosed below.', + zh: '已审查,未覆盖部分见下方披露。', + } + : { en: 'Reviewed.', zh: '已审查。' }, ); } @@ -1483,6 +1495,20 @@ function composeReviewBody( }; } +/** + * The public subject for an agent-derived disclosure label. A `chunk N` + * label stays bare — the chunk collapse translates it into the author's + * units. Any other label is the truncated first line of a launch prompt: + * prose. Rendered bare it reads as a claim about the PR itself — + * #8811's posted body carried "Not reviewed: This PR narrows the + * daemon-marker check from a truthy tes..." — not as the name of the agent + * that failed. Quoted, it reads as a name. The INTERNAL subject stays the + * unquoted label: the dedup and certification checks key on it. + */ +function quotedAgentSubject(label: string): { publicSubject?: string } { + return /^chunk \d+$/.test(label) ? {} : { publicSubject: `"${label}"` }; +} + /** * A set of unreviewed chunk ids, said in the PR author's units. * diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 57a51dba8ee..663203751a1 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -359,7 +359,14 @@ function publicRoleLabelZh(req: RequiredAgent): string | undefined { function label(rec: AgentRecord, chunk: number | null): string { if (chunk !== null) return `chunk ${chunk}`; const first = rec.launchPrompt.split('\n')[0]?.trim() ?? ''; - if (first) return first.length > 60 ? `${first.slice(0, 57)}...` : first; + if (first) { + if (first.length <= 60) return first; + // Truncate at a word boundary: a mid-word cut ("from a truthy tes...") + // made the label read like a garbled sentence of the body it lands in. + const cut = first.slice(0, 57); + const lastSpace = cut.lastIndexOf(' '); + return `${lastSpace > 0 ? cut.slice(0, lastSpace) : cut}...`; + } return rec.agentName || rec.agentId; } From 5896624f6880a9bc3236597a362876df5865053f Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 10 Aug 2026 17:44:43 +0800 Subject: [PATCH 2/6] fix(cli): open gap-carrying review bodies as partial, and de-translate one reason MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Feedback on the first wording: "Reviewed, with gaps disclosed below." still starts with the word the "Not reviewed:" clauses take back, and readers had to reconcile the pair anyway. The opener now says the review is PARTIAL instead — "Partially reviewed — gaps disclosed below." — so opener and disclosures read in one direction (Chinese half: 仅完成部分审查,未覆盖部分见下方披露。). Also replaces the translationese Chinese rendering of the unopened-agent reason ("它被指向 diff 的行却从未打开") with natural phrasing. --- .../commands/review/compose-review.test.ts | 26 +++++++++---------- .../cli/src/commands/review/compose-review.ts | 17 +++++++----- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 9fcf7e75412..4f82e0096d5 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -920,11 +920,11 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve base({ suggestionsInline: 1, unreviewedDimensions: ['security'] }), ); expect(r.event).toBe('COMMENT'); - // The gap disclosure follows, so the opener says so — a bare "Reviewed." - // two words above "Not reviewed:" read as the body contradicting itself - // (#8811). + // The gap disclosure follows, so the opener says the review is partial — + // any "Reviewed…" opener above "Not reviewed:" read as the body + // contradicting itself (#8811). expect(r.body).toContain( - 'Reviewed, with gaps disclosed below. Suggestions are inline.', + 'Partially reviewed — gaps disclosed below. Suggestions are inline.', ); expect(r.body).not.toContain('no blockers'); }); @@ -1094,7 +1094,7 @@ describe('composeReview — stacked states compose, none erased', () => { // downgradeApprove did not fire (base event was COMMENT), so no sentence… expect(r.body).not.toContain('Downgraded'); // …but every disclosure is present exactly once, and nothing certifies. - expect(r.body).toContain('Reviewed, with gaps disclosed below.'); + expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); expect(r.body).toContain('Suggestions are inline.'); expect(r.body).toContain('1 Suggestion-level finding(s)'); expect(r.body).toContain('Unresolved, please confirm:'); @@ -2094,18 +2094,18 @@ describe('coverage is recomputed, never accepted', () => { expect(r.body).not.toContain('Reviewed.'); }); - it('keeps a certifying opener while any chunk is certified — and names the gaps it carries', () => { + it('opens partial, not zero-certified, while any chunk is certified — and names the gaps it carries', () => { // chunk 1 built and never launched; chunk 2 reviewed properly. A partial - // gap is a disclosure, not a zero-certification — and the opener says the - // disclosures follow, so "Reviewed." never sits beside "Not reviewed:" - // (#8811). + // gap is a disclosure, not a zero-certification — and the opener says + // the review is partial, so no "Reviewed…" opener ever sits beside + // "Not reviewed:" (#8811). const p = plan(); recordBuilt(p, 1); recordBuilt(p, 2); recordMatrix(p); transcript('a2', goodPrompt(2), { toolCalls: 2 }); const r = composeReview({ planPath: p, env: ENV, modelId: MODEL }); - expect(r.body).toContain('Reviewed, with gaps disclosed below.'); + expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); expect(r.body).not.toContain('could not certify'); }); @@ -2967,9 +2967,9 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)' expect(r.body).toContain('未审查:全 diff 测试覆盖检查——'); // The zh sentence carries the translated reason, not the English one. expect(r.body).toContain('没有记录表明它的 brief 到达过任何 agent'); - // The gap opener, in both halves (#8811). - expect(r.body).toContain('Reviewed, with gaps disclosed below.'); - expect(r.body).toContain('已审查,未覆盖部分见下方披露。'); + // The partial opener, in both halves (#8811). + expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); + expect(r.body).toContain('仅完成部分审查,未覆盖部分见下方披露。'); }); it('quotes untranslatable caller text as-is in both halves', () => { diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 96ecfca040d..921906a66da 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -633,7 +633,8 @@ function composeReviewBody( 'pointed at diff lines it never opened: it made tool calls, but ' + 'none of them read the diff', reasonZh: - '它被指向 diff 的行却从未打开:有工具调用,但没有一次读取 diff', + '启动 prompt 为它指定了 diff 中的行,但它从未打开:有工具调用,' + + '却没有一次读取 diff', }); } if (cov.unopenedAgents.length > 0) { @@ -1408,10 +1409,12 @@ function composeReviewBody( coverageEntries.some((e) => e.subject === 'coverage') || (plannedChunks.length > 0 && coveredChunks.every((id) => disclosedChunkIds.has(id))); - // A bare "Reviewed." two words before "Not reviewed:" reads as the body - // contradicting itself — the shape that confused readers on #8811. When - // disclosures follow, the opener says so; the certifying and the - // zero-certified openers above keep their exact wording. + // Any opener starting with "Reviewed" reads as contradicting the + // "Not reviewed:" clauses below it — announcing the gaps does not fix + // it, as the first cut of this wording showed (#8811). When disclosures + // follow, the opener says the review is PARTIAL instead, so the pair + // reads in one direction; the certifying and the zero-certified openers + // above keep their exact wording. clauses.push( nothingCertified ? { @@ -1422,8 +1425,8 @@ function composeReviewBody( ? { en: 'Reviewed — no blockers.', zh: '已审查——无阻断问题。' } : notReviewedParts.length > 0 ? { - en: 'Reviewed, with gaps disclosed below.', - zh: '已审查,未覆盖部分见下方披露。', + en: 'Partially reviewed — gaps disclosed below.', + zh: '仅完成部分审查,未覆盖部分见下方披露。', } : { en: 'Reviewed.', zh: '已审查。' }, ); From fdb5fa3b4d81adf89fb4af70d639719433471571 Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Mon, 10 Aug 2026 20:34:06 +0800 Subject: [PATCH 3/6] fix(cli): keep review gap disclosures unambiguous --- .../commands/review/compose-review.test.ts | 69 +++++++++++++++---- .../cli/src/commands/review/compose-review.ts | 43 ++++++++---- .../cli/src/commands/review/lib/coverage.ts | 10 +-- 3 files changed, 88 insertions(+), 34 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 4f82e0096d5..639bf5ffbed 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -193,6 +193,7 @@ function transcript( toolCalls?: number; text?: string; opens?: string[]; + toolPath?: string; /** `[offset, limit]` making the diff reads ranged, as a compliant agent's are. */ range?: [number, number]; } = {}, @@ -227,7 +228,7 @@ function transcript( offset: opts.range[0], limit: opts.range[1], } - : { file_path: DIFF }, + : { file_path: opts.toolPath ?? DIFF }, }, }, ], @@ -924,7 +925,7 @@ describe('composeReview — event caps (round-7 Critical #2: caps must reach eve // any "Reviewed…" opener above "Not reviewed:" read as the body // contradicting itself (#8811). expect(r.body).toContain( - 'Partially reviewed — gaps disclosed below. Suggestions are inline.', + 'Partially reviewed — gaps disclosed. Suggestions are inline.', ); expect(r.body).not.toContain('no blockers'); }); @@ -1094,7 +1095,7 @@ describe('composeReview — stacked states compose, none erased', () => { // downgradeApprove did not fire (base event was COMMENT), so no sentence… expect(r.body).not.toContain('Downgraded'); // …but every disclosure is present exactly once, and nothing certifies. - expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); + expect(r.body).toContain('Partially reviewed — gaps disclosed.'); expect(r.body).toContain('Suggestions are inline.'); expect(r.body).toContain('1 Suggestion-level finding(s)'); expect(r.body).toContain('Unresolved, please confirm:'); @@ -1305,6 +1306,8 @@ describe('composeReview — budget-gap disclosures (a channel, never a cap)', () }); expect(r.body).toContain('Not explored to full depth'); expect(r.body).not.toContain('no blockers'); + expect(r.body).toContain('Reviewed.'); + expect(r.body).not.toContain('Partially reviewed'); }); }); @@ -2105,7 +2108,7 @@ describe('coverage is recomputed, never accepted', () => { recordMatrix(p); transcript('a2', goodPrompt(2), { toolCalls: 2 }); const r = composeReview({ planPath: p, env: ENV, modelId: MODEL }); - expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); + expect(r.body).toContain('Partially reviewed — gaps disclosed.'); expect(r.body).not.toContain('could not certify'); }); @@ -2171,23 +2174,60 @@ describe('coverage is recomputed, never accepted', () => { // truthy tes..." read as a sentence about the whole PR, not the name of // the one agent that failed. Quotes say which it is, and the truncation // stops at a word boundary instead of mid-word. - transcript( - 'p1', + const p = plan({ han: true }); + transcript('a2', goodPrompt(2), { toolCalls: 2 }); + recordBuilt(p, 2); + recordMatrix(p); + const brief = briefPath(p, 'chunk-1'); + writeFileSync(brief, 'The chunk-1 brief.'); + const launch = 'This PR narrows the daemon-marker check from a truthy test to an exact one\n' + - `read_file(file_path="${DIFF}")`, - { toolCalls: 0 }, - ); + `read_file(file_path="${brief}")\n` + + `read_file(file_path="${DIFF}", offset=0, limit=100)`; + writeFileSync(join(promptRecordDir(p), 'chunk-1.txt'), launch); + transcript('p1', launch, { + toolCalls: 1, + toolPath: join(dir, 'other.ts'), + opens: [brief], + }); const r = composeReview({ criticalsInline: 0, suggestionsInline: 0, - planPath: plan(), + planPath: p, env: ENV, modelId: MODEL, }); expect(r.body).toContain( - 'Not reviewed: "This PR narrows the daemon-marker check from a truthy..."', + 'Not reviewed: `"This PR narrows the daemon-marker check from a truthy test…"`', ); expect(r.body).not.toContain('truthy tes...'); + expect(r.body).toContain( + '启动 prompt 为它指定了 diff 中的行,但它从未打开', + ); + }); + + it('keeps long agent labels distinct when their first word matches', () => { + transcript( + 'p1', + `Angle-A: packages/core/src/services/session-lifecycle-manager.ts\n${DIFF}`, + ); + transcript( + 'p2', + `Angle-A: packages/core/src/services/session-lifecycle-reaper.ts\n${DIFF}`, + ); + const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); + + expect(r.body).toContain('session-lifecycle-manag…'); + expect(r.body).toContain('session-lifecycle-reape…'); + }); + + it('renders prompt-derived labels as inert Markdown', () => { + transcript('p1', `Fix @owner from #123 in handling\n${DIFF}`); + const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); + + expect(r.body).toContain( + 'Not reviewed: `"Fix @owner from #123 in handling"`', + ); }); it('names a blind launch as itself, not as a whiff', () => { @@ -2968,8 +3008,8 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)' // The zh sentence carries the translated reason, not the English one. expect(r.body).toContain('没有记录表明它的 brief 到达过任何 agent'); // The partial opener, in both halves (#8811). - expect(r.body).toContain('Partially reviewed — gaps disclosed below.'); - expect(r.body).toContain('仅完成部分审查,未覆盖部分见下方披露。'); + expect(r.body).toContain('Partially reviewed — gaps disclosed.'); + expect(r.body).toContain('仅完成部分审查,审查缺口已披露。'); }); it('quotes untranslatable caller text as-is in both halves', () => { @@ -4019,6 +4059,9 @@ describe('composeReview — the findings file tag check', () => { '1 finding(s) still carried the `— [unverified]` tag when the loop ' + 'ended', ); + expect(r.body).toContain( + 'Review incomplete — unverified findings disclosed.', + ); // The opener may not certify over a loop that ended mid-verification. expect(r.body).not.toContain('no blockers'); expect(r.remediation.join(' ')).toContain('--role verify'); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 921906a66da..98b3d17cdf5 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -584,7 +584,7 @@ function composeReviewBody( for (const label of cov.idleAgents) { coverageEntries.push({ subject: label, - ...quotedAgentSubject(label), + publicSubject: publicAgentSubject(label), reason: 'the agent made no tool call: it read nothing', reasonZh: '该 agent 未发起任何工具调用:它什么都没读', }); @@ -606,7 +606,7 @@ function composeReviewBody( for (const label of cov.blindAgents) { coverageEntries.push({ subject: label, - ...quotedAgentSubject(label), + publicSubject: publicAgentSubject(label), reason: 'launched with a prompt that never named the diff file, so it ' + 'could not have read it', @@ -628,7 +628,7 @@ function composeReviewBody( for (const label of cov.unopenedAgents) { coverageEntries.push({ subject: label, - ...quotedAgentSubject(label), + publicSubject: publicAgentSubject(label), reason: 'pointed at diff lines it never opened: it made tool calls, but ' + 'none of them read the diff', @@ -1121,11 +1121,19 @@ function composeReviewBody( const shown = keptBudgetGaps.slice(0, MAX_BUDGET_GAP_LINES); const more = keptBudgetGaps.length - shown.length; const enList = - shown.map((it) => `${it.agent}: ${mdField(it.gap)}`).join('; ') + - (more > 0 ? `, and ${more} more` : ''); + shown + .map( + (it) => + `${publicAgentSubject(it.agent) ?? it.agent}: ${mdField(it.gap)}`, + ) + .join('; ') + (more > 0 ? `, and ${more} more` : ''); const zhList = - shown.map((it) => `${it.agent}:${mdField(it.gap)}`).join(';') + - (more > 0 ? `,另有 ${more} 条` : ''); + shown + .map( + (it) => + `${publicAgentSubject(it.agent) ?? it.agent}:${mdField(it.gap)}`, + ) + .join(';') + (more > 0 ? `,另有 ${more} 条` : ''); notReviewedParts.push({ en: `Not explored to full depth (tool budget reached): ${enList}.`, zh: `未探索到全部深度(达到工具调用预算):${zhList}。`, @@ -1409,6 +1417,10 @@ function composeReviewBody( coverageEntries.some((e) => e.subject === 'coverage') || (plannedChunks.length > 0 && coveredChunks.every((id) => disclosedChunkIds.has(id))); + const hasCoverageGaps = + unreviewed.length + coverageEntries.length > 0 || + missingReceipts.length > 0 || + uncoverable.length > 0; // Any opener starting with "Reviewed" reads as contradicting the // "Not reviewed:" clauses below it — announcing the gaps does not fix // it, as the first cut of this wording showed (#8811). When disclosures @@ -1423,12 +1435,17 @@ function composeReviewBody( } : canCertify ? { en: 'Reviewed — no blockers.', zh: '已审查——无阻断问题。' } - : notReviewedParts.length > 0 + : hasCoverageGaps ? { - en: 'Partially reviewed — gaps disclosed below.', - zh: '仅完成部分审查,未覆盖部分见下方披露。', + en: 'Partially reviewed — gaps disclosed.', + zh: '仅完成部分审查,审查缺口已披露。', } - : { en: 'Reviewed.', zh: '已审查。' }, + : findingsUnverifiedAtCompose + ? { + en: 'Review incomplete — unverified findings disclosed.', + zh: '审查未完成——未验证的发现已披露。', + } + : { en: 'Reviewed.', zh: '已审查。' }, ); } @@ -1508,8 +1525,8 @@ function composeReviewBody( * that failed. Quoted, it reads as a name. The INTERNAL subject stays the * unquoted label: the dedup and certification checks key on it. */ -function quotedAgentSubject(label: string): { publicSubject?: string } { - return /^chunk \d+$/.test(label) ? {} : { publicSubject: `"${label}"` }; +function publicAgentSubject(label: string): string | undefined { + return /^chunk \d+$/.test(label) ? undefined : mdField(`"${label}"`); } /** diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index 663203751a1..f489b7c8f09 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -75,6 +75,7 @@ import { chunkIdsProblem } from './diff-plan.js'; import { readBudgetStop } from './deadline.js'; import { budgetGapDisclosures } from './budget.js'; import { shellQuotePath } from './shell-quote.js'; +import { compressSummary } from '../findings.js'; export interface CoverageFromTranscripts { /** True only when every chunk was reviewed by an agent that could and did. */ @@ -359,14 +360,7 @@ function publicRoleLabelZh(req: RequiredAgent): string | undefined { function label(rec: AgentRecord, chunk: number | null): string { if (chunk !== null) return `chunk ${chunk}`; const first = rec.launchPrompt.split('\n')[0]?.trim() ?? ''; - if (first) { - if (first.length <= 60) return first; - // Truncate at a word boundary: a mid-word cut ("from a truthy tes...") - // made the label read like a garbled sentence of the body it lands in. - const cut = first.slice(0, 57); - const lastSpace = cut.lastIndexOf(' '); - return `${lastSpace > 0 ? cut.slice(0, lastSpace) : cut}...`; - } + if (first) return compressSummary(first); return rec.agentName || rec.agentId; } From f2d807632d23ef42e106b546266339517fe635fd Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 11 Aug 2026 00:48:09 +0800 Subject: [PATCH 4/6] fix(review): preserve distinct agent labels --- .../src/commands/review/compose-review.test.ts | 17 +++++++++++------ .../cli/src/commands/review/compose-review.ts | 18 +++++++++++++----- .../cli/src/commands/review/lib/coverage.ts | 3 +-- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 639bf5ffbed..6bdd66e44a3 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -2209,24 +2209,27 @@ describe('coverage is recomputed, never accepted', () => { it('keeps long agent labels distinct when their first word matches', () => { transcript( 'p1', - `Angle-A: packages/core/src/services/session-lifecycle-manager.ts\n${DIFF}`, + `Verify the daemon marker rename does not break macos-build behavior\n${DIFF}`, ); transcript( 'p2', - `Angle-A: packages/core/src/services/session-lifecycle-reaper.ts\n${DIFF}`, + `Verify the daemon marker rename does not break linux-build behavior\n${DIFF}`, ); const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); - expect(r.body).toContain('session-lifecycle-manag…'); - expect(r.body).toContain('session-lifecycle-reape…'); + expect(r.body).toContain('macos-build…'); + expect(r.body).toContain('linux-build…'); }); it('renders prompt-derived labels as inert Markdown', () => { - transcript('p1', `Fix @owner from #123 in handling\n${DIFF}`); + transcript( + 'p1', + `Fix the "daemon marker" regression for @owner from #123\n${DIFF}`, + ); const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); expect(r.body).toContain( - 'Not reviewed: `"Fix @owner from #123 in handling"`', + 'Not reviewed: `"Fix the \\"daemon marker\\" regression for @owner from #123"`', ); }); @@ -4131,6 +4134,8 @@ describe('composeReview — the findings file tag check', () => { expect(r.event).toBe('COMMENT'); expect(r.cappedBy).toContain('findings-unverified-at-compose'); expect(r.body).toContain('findings file could not be read at compose time'); + expect(r.body).toContain('Review incomplete — findings unavailable.'); + expect(r.body).not.toContain('unverified findings disclosed'); expect(r.remediation.join(' ')).toContain('findingsPath'); }); diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 98b3d17cdf5..7722f361f52 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -30,6 +30,7 @@ import { verificationGaps, TranscriptsUnavailableError, } from './lib/coverage.js'; +import { compressSummary } from './findings.js'; import { BUDGET_STOP_PHRASE, ROUND_CAP_PHRASE, @@ -1440,12 +1441,17 @@ function composeReviewBody( en: 'Partially reviewed — gaps disclosed.', zh: '仅完成部分审查,审查缺口已披露。', } - : findingsUnverifiedAtCompose + : findingsFileUnreadable ? { - en: 'Review incomplete — unverified findings disclosed.', - zh: '审查未完成——未验证的发现已披露。', + en: 'Review incomplete — findings unavailable.', + zh: '审查未完成——发现不可用。', } - : { en: 'Reviewed.', zh: '已审查。' }, + : findingsUnverifiedAtCompose + ? { + en: 'Review incomplete — unverified findings disclosed.', + zh: '审查未完成——未验证的发现已披露。', + } + : { en: 'Reviewed.', zh: '已审查。' }, ); } @@ -1526,7 +1532,9 @@ function composeReviewBody( * unquoted label: the dedup and certification checks key on it. */ function publicAgentSubject(label: string): string | undefined { - return /^chunk \d+$/.test(label) ? undefined : mdField(`"${label}"`); + return /^chunk \d+$/.test(label) + ? undefined + : mdField(JSON.stringify(compressSummary(label))); } /** diff --git a/packages/cli/src/commands/review/lib/coverage.ts b/packages/cli/src/commands/review/lib/coverage.ts index f489b7c8f09..12d0c7d6b00 100644 --- a/packages/cli/src/commands/review/lib/coverage.ts +++ b/packages/cli/src/commands/review/lib/coverage.ts @@ -75,7 +75,6 @@ import { chunkIdsProblem } from './diff-plan.js'; import { readBudgetStop } from './deadline.js'; import { budgetGapDisclosures } from './budget.js'; import { shellQuotePath } from './shell-quote.js'; -import { compressSummary } from '../findings.js'; export interface CoverageFromTranscripts { /** True only when every chunk was reviewed by an agent that could and did. */ @@ -360,7 +359,7 @@ function publicRoleLabelZh(req: RequiredAgent): string | undefined { function label(rec: AgentRecord, chunk: number | null): string { if (chunk !== null) return `chunk ${chunk}`; const first = rec.launchPrompt.split('\n')[0]?.trim() ?? ''; - if (first) return compressSummary(first); + if (first) return first.replace(/\s+/g, ' '); return rec.agentName || rec.agentId; } From a4ebac2f52ee6d36b5c3740836aecd590a2e163f Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 11 Aug 2026 10:57:59 +0800 Subject: [PATCH 5/6] fix(review): normalize agent label spacing --- .../cli/src/commands/review/compose-review.test.ts | 12 ++++++++++++ packages/cli/src/commands/review/compose-review.ts | 2 +- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 6bdd66e44a3..dc41878c430 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -2233,6 +2233,18 @@ describe('coverage is recomputed, never accepted', () => { ); }); + it('collapses spaces after removing backticks from agent labels', () => { + transcript( + 'p1', + `You are review agent \`security\` — inspect auth\n${DIFF}`, + ); + const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); + + expect(r.body).toContain( + 'Not reviewed: `"You are review agent security — inspect auth"`', + ); + }); + 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 diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 7722f361f52..491b34d4ea3 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1534,7 +1534,7 @@ function composeReviewBody( function publicAgentSubject(label: string): string | undefined { return /^chunk \d+$/.test(label) ? undefined - : mdField(JSON.stringify(compressSummary(label))); + : mdField(JSON.stringify(compressSummary(label.replace(/[`\r\n]+/g, ' ')))); } /** From 664d10c8a55460d4e09875c71dbfd509a4aeb47e Mon Sep 17 00:00:00 2001 From: yiliang114 Date: Tue, 11 Aug 2026 16:35:29 +0800 Subject: [PATCH 6/6] fix(review): keep incomplete review openers honest --- .../commands/review/compose-review.test.ts | 27 ++++- .../cli/src/commands/review/compose-review.ts | 101 ++++++++++-------- 2 files changed, 85 insertions(+), 43 deletions(-) diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 76d6a40dd38..fdb35113a06 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -978,6 +978,15 @@ describe('composeReview — context-unavailable (clause 2)', () => { expect(r.body).not.toMatch(/Reviewed\.\s/); }); + it('discloses coverage gaps before the diff-only warning', () => { + const r = composeReview( + base({ contextUnavailable: true, unreviewedDimensions: ['security'] }), + ); + expect(r.body.indexOf('Partially reviewed')).toBeLessThan( + r.body.indexOf('Reviewed diff-only'), + ); + }); + it('does not soften a REQUEST_CHANGES', () => { const r = composeReview( base({ criticalsInline: 1, contextUnavailable: true }), @@ -1179,9 +1188,16 @@ describe('composeReview — stacked states compose, none erased', () => { describe('composeReview — RC carries every applicable disclosure (no clause squeezed out)', () => { it('RC + context-unavailable keeps the diff-only trust warning in the body', () => { const r = composeReview( - base({ criticalsInline: 1, contextUnavailable: true }), + base({ + criticalsInline: 1, + contextUnavailable: true, + unreviewedDimensions: ['security'], + }), ); expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.body.indexOf('Partially reviewed')).toBeLessThan( + r.body.indexOf('Reviewed diff-only'), + ); expect(r.body).toContain('Reviewed diff-only'); }); @@ -2249,6 +2265,15 @@ describe('coverage is recomputed, never accepted', () => { expect(r.body).toContain('linux-build…'); }); + it('counts agent labels that truncate to the same public subject', () => { + const prefix = `Verify ${'the same long scope '.repeat(5)}`; + transcript('p1', `${prefix}macos behavior\n${DIFF}`); + transcript('p2', `${prefix}linux behavior\n${DIFF}`); + const r = composeReview({ planPath: plan(), env: ENV, modelId: MODEL }); + + expect(r.body).toContain('(×2)'); + }); + it('renders prompt-derived labels as inert Markdown', () => { transcript( 'p1', diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index e7707b39b49..5fdfa3ed606 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -1381,20 +1381,35 @@ function composeReviewBody( // selector — and the partition below keys on the INTERNAL subject, so a // public phrase can never shadow a chunk id out of the chunk collapse. const chunkIds: number[] = []; - const named: string[] = []; - const namedZh: string[] = []; + const named = new Map(); for (const e of entries) { const m = /^chunk (\d+)$/.exec(e.subject); if (m) chunkIds.push(Number(m[1])); else { - named.push(e.publicSubject ?? e.subject); - namedZh.push(e.subjectZh ?? e.publicSubject ?? e.subject); + const subject = e.publicSubject ?? e.subject; + const existing = named.get(subject); + if (existing) existing.count++; + else + named.set(subject, { + zh: e.subjectZh ?? subject, + count: 1, + }); } } const gap = chunkIds.length > 0 ? describeChunkGap(chunkIds, plannedChunks) : null; - const shown = [...(gap ? [gap.phrase] : []), ...named]; - const shownZh = [...(gap ? [gap.phraseZh] : []), ...namedZh]; + const shown = [ + ...(gap ? [gap.phrase] : []), + ...[...named].map(([subject, { count }]) => + count > 1 ? `${subject} (×${count})` : subject, + ), + ]; + const shownZh = [ + ...(gap ? [gap.phraseZh] : []), + ...[...named.values()].map(({ zh, count }) => + count > 1 ? `${zh}(×${count})` : zh, + ), + ]; const reasonZh = reasonZhOf.get(reason) ?? reason; notReviewedParts.push({ en: reason @@ -1424,6 +1439,31 @@ function composeReviewBody( zh: '仅审查了 diff——无法获取 PR 已有的讨论,因此这不构成批准,也不构成"无阻断问题"的结论。', }; + const disclosedChunkIds = new Set(); + for (const e of coverageEntries) { + const m = /^chunk (\d+)$/.exec(e.subject); + if (m) disclosedChunkIds.add(Number(m[1])); + } + const nothingCertified = + coverageEntries.some((e) => e.subject === 'coverage') || + (plannedChunks.length > 0 && + coveredChunks.every((id) => disclosedChunkIds.has(id))); + const hasCoverageGaps = + unreviewed.length + coverageEntries.length > 0 || + missingReceipts.length > 0 || + uncoverable.length > 0; + const coverageOpener: Bi | undefined = nothingCertified + ? { + en: '⚠️ This run could not certify that any of this diff was reviewed.', + zh: '⚠️ 本次运行无法证明这个 diff 的任何部分经过了审查。', + } + : hasCoverageGaps + ? { + en: 'Partially reviewed — gaps disclosed.', + zh: '仅完成部分审查,审查缺口已披露。', + } + : undefined; + // A deferred checker (actionlint's embedded shell): disclosed on EVERY verdict — // including Approve — so the reader knows a workflow's shell was not linted, but // it does not cap the verdict (it is a tool limitation, not a finding or an @@ -1484,6 +1524,7 @@ function composeReviewBody( // trust warning (clause 2), an undecided existing Critical (clause 5), // or the unread-scope disclosure (clause 6). const parts = [ + ...(coverageOpener ? [coverageOpener] : []), ...(contextUnavailable ? [contextUnavailableClause] : []), ...cannotTellBlock, ...notReviewedParts, @@ -1552,9 +1593,9 @@ function composeReviewBody( }); } - // 2. Context-unavailable clause — when present, it opens the body and no - // clause may certify "no blockers". + // 2. Context-unavailable clause — no later clause may certify "no blockers". if (contextUnavailable) { + if (coverageOpener) clauses.push(coverageOpener); clauses.push(contextUnavailableClause); } else { // 3. Opener — certifying only when the review can actually certify it. @@ -1569,13 +1610,11 @@ function composeReviewBody( !downgradeRequestChanges && c === 0 && cannotTell.length === 0 && - uncoverable.length === 0 && - unreviewed.length + coverageEntries.length === 0 && + !hasCoverageGaps && // A missing receipt caps the event but was left out of certification, so a // body could open "Reviewed — no blockers." two lines above "nobody read // them." Nothing nobody read can be certified blocker-free — and neither // can a loop that ended with findings no verifier ever ruled on. - missingReceipts.length === 0 && // A disclosed budget gap is not a blocker, but "Reviewed — no // blockers." two lines above "Not explored to full depth" is the // opener certifying what the disclosure takes back — the exact @@ -1593,19 +1632,6 @@ function composeReviewBody( // `coverage` subject is the no-plan/unreadable-transcripts family — there // is no chunk universe to count, and what cannot be counted cannot be // certified. - const disclosedChunkIds = new Set(); - for (const e of coverageEntries) { - const m = /^chunk (\d+)$/.exec(e.subject); - if (m) disclosedChunkIds.add(Number(m[1])); - } - const nothingCertified = - coverageEntries.some((e) => e.subject === 'coverage') || - (plannedChunks.length > 0 && - coveredChunks.every((id) => disclosedChunkIds.has(id))); - const hasCoverageGaps = - unreviewed.length + coverageEntries.length > 0 || - missingReceipts.length > 0 || - uncoverable.length > 0; // Any opener starting with "Reviewed" reads as contradicting the // "Not reviewed:" clauses below it — announcing the gaps does not fix // it, as the first cut of this wording showed (#8811). When disclosures @@ -1613,29 +1639,20 @@ function composeReviewBody( // reads in one direction; the certifying and the zero-certified openers // above keep their exact wording. clauses.push( - nothingCertified - ? { - en: '⚠️ This run could not certify that any of this diff was reviewed.', - zh: '⚠️ 本次运行无法证明这个 diff 的任何部分经过了审查。', - } - : canCertify + coverageOpener ?? + (canCertify ? { en: 'Reviewed — no blockers.', zh: '已审查——无阻断问题。' } - : hasCoverageGaps + : findingsFileUnreadable ? { - en: 'Partially reviewed — gaps disclosed.', - zh: '仅完成部分审查,审查缺口已披露。', + en: 'Review incomplete — findings unavailable.', + zh: '审查未完成——发现不可用。', } - : findingsFileUnreadable + : findingsUnverifiedAtCompose ? { - en: 'Review incomplete — findings unavailable.', - zh: '审查未完成——发现不可用。', + en: 'Review incomplete — unverified findings disclosed.', + zh: '审查未完成——未验证的发现已披露。', } - : findingsUnverifiedAtCompose - ? { - en: 'Review incomplete — unverified findings disclosed.', - zh: '审查未完成——未验证的发现已披露。', - } - : { en: 'Reviewed.', zh: '已审查。' }, + : { en: 'Reviewed.', zh: '已审查。' }), ); }