Skip to content
Merged
108 changes: 102 additions & 6 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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];
} = {},
Expand Down Expand Up @@ -227,7 +228,7 @@ function transcript(
offset: opts.range[0],
limit: opts.range[1],
}
: { file_path: DIFF },
: { file_path: opts.toolPath ?? DIFF },
},
},
],
Expand Down Expand Up @@ -920,7 +921,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 the review is partial —
// any "Reviewed…" opener above "Not reviewed:" read as the body
// contradicting itself (#8811).
expect(r.body).toContain(
'Partially reviewed — gaps disclosed. Suggestions are inline.',
);
expect(r.body).not.toContain('no blockers');
});
});
Expand Down Expand Up @@ -1089,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('Reviewed.');
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:');
Expand Down Expand Up @@ -1300,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');
});
});

Expand Down Expand Up @@ -2089,16 +2097,18 @@ describe('coverage is recomputed, never accepted', () => {
expect(r.body).not.toContain('Reviewed.');
});

it('keeps the "Reviewed." opener while any chunk is certified', () => {
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.
// 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.');
expect(r.body).toContain('Partially reviewed — gaps disclosed.');
expect(r.body).not.toContain('could not certify');
});

Expand Down Expand Up @@ -2157,6 +2167,84 @@ 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.
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="${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: p,
env: ENV,
modelId: MODEL,
});
expect(r.body).toContain(
'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',
`Verify the daemon marker rename does not break macos-build behavior\n${DIFF}`,
);
transcript(
'p2',
`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('macos-build…');
expect(r.body).toContain('linux-build…');
});

it('renders prompt-derived labels as inert Markdown', () => {
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 the \\"daemon marker\\" regression for @owner from #123"`',
);
});

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
Expand Down Expand Up @@ -2934,6 +3022,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 partial opener, in both halves (#8811).
expect(r.body).toContain('Partially reviewed — gaps disclosed.');
expect(r.body).toContain('仅完成部分审查,审查缺口已披露。');
});

it('quotes untranslatable caller text as-is in both halves', () => {
Expand Down Expand Up @@ -3983,6 +4074,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');
Expand Down Expand Up @@ -4052,6 +4146,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');
});

Expand Down
66 changes: 60 additions & 6 deletions packages/cli/src/commands/review/compose-review.ts
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
verificationGaps,
TranscriptsUnavailableError,
} from './lib/coverage.js';
import { compressSummary } from './findings.js';
import {
BUDGET_STOP_PHRASE,
ROUND_CAP_PHRASE,
Expand Down Expand Up @@ -584,6 +585,7 @@ function composeReviewBody(
for (const label of cov.idleAgents) {
coverageEntries.push({
subject: label,
publicSubject: publicAgentSubject(label),
reason: 'the agent made no tool call: it read nothing',
reasonZh: '该 agent 未发起任何工具调用:它什么都没读',
});
Expand All @@ -605,6 +607,7 @@ function composeReviewBody(
for (const label of cov.blindAgents) {
coverageEntries.push({
subject: label,
publicSubject: publicAgentSubject(label),
reason:
'launched with a prompt that never named the diff file, so it ' +
'could not have read it',
Expand All @@ -626,11 +629,13 @@ function composeReviewBody(
for (const label of cov.unopenedAgents) {
coverageEntries.push({
subject: label,
publicSubject: publicAgentSubject(label),
reason:
'pointed at diff lines it never opened: it made tool calls, but ' +
'none of them read the diff',
reasonZh:
'它被指向 diff 的行却从未打开:有工具调用,但没有一次读取 diff',
'启动 prompt 为它指定了 diff 中的行,但它从未打开:有工具调用,' +
'却没有一次读取 diff',
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
});
}
if (cov.unopenedAgents.length > 0) {
Expand Down Expand Up @@ -1117,11 +1122,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)}`,
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
)
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
.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}。`,
Expand Down Expand Up @@ -1405,6 +1418,16 @@ 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;
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
// 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
? {
Expand All @@ -1413,7 +1436,22 @@ function composeReviewBody(
}
: canCertify
? { en: 'Reviewed — no blockers.', zh: '已审查——无阻断问题。' }
: { en: 'Reviewed.', zh: '已审查。' },
: hasCoverageGaps
? {
en: 'Partially reviewed — gaps disclosed.',
zh: '仅完成部分审查,审查缺口已披露。',
}
: findingsFileUnreadable
? {
en: 'Review incomplete — findings unavailable.',
zh: '审查未完成——发现不可用。',
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
}
: findingsUnverifiedAtCompose
? {
en: 'Review incomplete — unverified findings disclosed.',
zh: '审查未完成——未验证的发现已披露。',
}
: { en: 'Reviewed.', zh: '已审查。' },
);
}

Expand Down Expand Up @@ -1483,6 +1521,22 @@ 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
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
* 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 publicAgentSubject(label: string): string | undefined {
return /^chunk \d+$/.test(label)
? undefined
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
: mdField(JSON.stringify(compressSummary(label.replace(/[`\r\n]+/g, ' '))));
}

/**
* A set of unreviewed chunk ids, said in the PR author's units.
*
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/lib/coverage.ts
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
Original file line number Diff line number Diff line change
Expand Up @@ -359,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 first.length > 60 ? `${first.slice(0, 57)}...` : first;
if (first) return first.replace(/\s+/g, ' ');
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
return rec.agentName || rec.agentId;
}

Expand Down
Loading