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
47 changes: 26 additions & 21 deletions packages/cli/src/commands/review/check-coverage.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1348,6 +1348,11 @@ describe('an agent that paged its chunk still read it', () => {
});
});

/** The old rendered shape, for the regex assertions: structural gaps, joined. */
const gapText = (r: {
gaps: Array<{ subject: string; reason: string }>;
}): string => r.gaps.map((g) => `${g.subject} — ${g.reason}`).join(' ');

describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', () => {
// A Step 4/5 agent as a real run leaves it: the CLI's record of the prompt it
// built (`agent-prompt --role <role>`), the brief that prompt points at, and the
Expand Down Expand Up @@ -1427,7 +1432,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()

const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.ok).toBe(false);
expect(r.gaps.join(' ')).toMatch(/verification — /);
expect(gapText(r)).toMatch(/verification — /);

// The compliant launch — the full printed prompt — clears it.
transcript('v-full', full, { calls: 2, opens: [brief] });
Expand Down Expand Up @@ -1468,7 +1473,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
const p = plan(); // no reverse-audit fixture: the step was skipped
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.ok).toBe(false);
const gap = r.gaps.join(' ');
const gap = gapText(r);
expect(gap).toMatch(
/reverse audit — no auditor was launched with a prompt this skill builds/,
);
Expand All @@ -1490,7 +1495,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'reverse-audit', { rewritten: true });
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.ok).toBe(false);
const gap = r.gaps.join(' ');
const gap = gapText(r);
// It says what happened — the auditor ran AND opened its brief (that is how
// this shape is even detected, and a text denying it publishes a false
// mechanism) …
Expand Down Expand Up @@ -1527,7 +1532,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'reverse-audit');
step45(p, 'verify', { rewritten: true });
const r = verificationGaps(p, { postsFindings: true }, ENV);
const gap = r.gaps.join(' ');
const gap = gapText(r);
expect(gap).toMatch(/a verifier ran and opened its brief/);
expect(gap).toMatch(/no agent was launched with the prompt the CLI built/);
expect(gap).not.toMatch(/no verifier ran/);
Expand All @@ -1546,7 +1551,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'reverse-audit', { opensBrief: false });
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.ok).toBe(false);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/reverse audit — it was launched with the built prompt but never opened its brief/,
);
});
Expand All @@ -1556,7 +1561,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'reverse-audit', { launch: false });
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.ok).toBe(false);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/reverse audit — its prompt was built, but no agent was launched with it/,
);
});
Expand All @@ -1565,32 +1570,30 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
const p = plan();
step45(p, 'reverse-audit--chunk-1');
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.gaps.join(' ')).not.toMatch(/reverse audit/);
expect(gapText(r)).not.toMatch(/reverse audit/);
});

it('requires a verifier when the review posts findings', () => {
const p = plan();
step45(p, 'reverse-audit'); // isolate the verify gap
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.ok).toBe(false);
expect(r.gaps.join(' ')).toMatch(
/verification — the review posts findings/,
);
expect(gapText(r)).toMatch(/verification — the review posts findings/);
});

it('does not require a verifier when the review confirmed nothing', () => {
const p = plan();
step45(p, 'reverse-audit');
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.gaps.join(' ')).not.toMatch(/verification/);
expect(gapText(r)).not.toMatch(/verification/);
});

it('flags a verifier built but whose agent never opened its brief', () => {
const p = plan();
step45(p, 'reverse-audit');
step45(p, 'verify', { opensBrief: false });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/verification — it was launched with the built prompt but never opened its brief/,
);
});
Expand All @@ -1603,7 +1606,7 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'reverse-audit');
step45(p, 'verify', { launch: false });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/verification — its prompt was built, but no agent was launched with it/,
);
});
Expand All @@ -1620,10 +1623,12 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
expect(r.ok).toBe(false);
expect(r.gaps).toHaveLength(1);
const gap = r.gaps[0];
expect(gap).toMatch(/^verification and reverse audit — /);
expect(gap).toMatch(/each ran and opened its brief/);
expect(gap).toMatch(/written by hand/);
expect(gap).toMatch(/cannot be counted as verified/);
expect(gap.subject).toBe('verification and reverse audit');
expect(gap.subjectZh).toBe('验证与反向审计');
expect(gap.reasonZh).toContain('手写');
expect(gap.reason).toMatch(/each ran and opened its brief/);
expect(gap.reason).toMatch(/written by hand/);
expect(gap.reason).toMatch(/cannot be counted as verified/);
// The remediation stays per-role: the two rebuild commands differ.
const fix = r.remediation.join(' ');
expect(fix).toContain('--role reverse-audit');
Expand All @@ -1639,13 +1644,13 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
step45(p, 'verify', { launch: false });
const r = verificationGaps(p, { postsFindings: true }, ENV);
expect(r.gaps).toHaveLength(2);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/reverse audit — an auditor ran and opened its brief/,
);
expect(r.gaps.join(' ')).toMatch(
expect(gapText(r)).toMatch(
/verification — its prompt was built, but no agent was launched with it/,
);
expect(r.gaps.join(' ')).not.toMatch(/verification and reverse audit/);
expect(gapText(r)).not.toMatch(/verification and reverse audit/);
});

it('does not merge when the review posts no findings — verify was never owed', () => {
Expand All @@ -1654,6 +1659,6 @@ describe('verificationGaps — Step 4 and Step 5 ran, and read their briefs', ()
const p = plan(); // neither step on record
const r = verificationGaps(p, { postsFindings: false }, ENV);
expect(r.gaps).toHaveLength(1);
expect(r.gaps[0]).toMatch(/^reverse audit — /);
expect(r.gaps[0].subject).toBe('reverse audit');
});
});
89 changes: 87 additions & 2 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,15 @@ const DIFF = '/abs/diff.txt';
* satisfies that one. A plan that requires nothing is not a plan any capture
* command writes, and coverage now reads the roster out of it.
*/
function plan(opts: { step45?: boolean } = {}): string {
function plan(opts: { step45?: boolean; han?: boolean } = {}): string {
const p = join(dir, 'plan.json');
writeFileSync(
p,
JSON.stringify({
diffPathAbsolute: DIFF,
// What fetch-pr records when the PR description contains Han
// characters — the deterministic bilingual-body switch.
...(opts.han ? { prDescriptionHasHan: true } : {}),
srcDiffLines: 5000,
diffLines: 5000,
files: [{ path: 'a.ts', kind: 'source', removedLines: 0, heavy: false }],
Expand Down Expand Up @@ -279,10 +282,11 @@ function blindPrompt(chunk: number): string {
*/
function coveredPlan(
step45Keys: string[] = ['verify', 'reverse-audit'],
planOpts: { han?: boolean } = {},
): string {
transcript('a1', goodPrompt(1), { toolCalls: 3 });
transcript('a2', goodPrompt(2), { toolCalls: 2 });
const p = plan({ step45: false });
const p = plan({ step45: false, ...planOpts });
recordBuilt(p, 1);
recordBuilt(p, 2);
recordMatrix(p);
Expand Down Expand Up @@ -1983,17 +1987,20 @@ describe('describeChunkGap — chunk ids leave in the author units', () => {
it('every planned chunk collapses to the diff itself', () => {
expect(describeChunkGap([2, 1, 3], planned)).toEqual({
phrase: 'the entire diff',
phraseZh: '整个 diff',
plural: false,
});
});

it('names the files of a narrow gap — sorted by id, deduped', () => {
expect(describeChunkGap([2], planned)).toEqual({
phrase: 'the diff section covering src/b.ts, src/c.ts',
phraseZh: '涉及 src/b.ts、src/c.ts 的 diff 片段',
plural: false,
});
expect(describeChunkGap([3, 1], planned)).toEqual({
phrase: 'the diff sections covering src/a.ts, src/d.ts',
phraseZh: '涉及 src/a.ts、src/d.ts 的 diff 片段',
plural: true,
});
// A subject disclosed twice is one gap.
Expand All @@ -2008,6 +2015,7 @@ describe('describeChunkGap — chunk ids leave in the author units', () => {
];
expect(describeChunkGap([1, 2], wide)).toEqual({
phrase: "2 of the diff's 3 sections",
phraseZh: 'diff 3 个片段中的 2 个',
plural: true,
});
});
Expand All @@ -2020,18 +2028,95 @@ describe('describeChunkGap — chunk ids leave in the author units', () => {
];
expect(describeChunkGap([1, 2], partial)).toEqual({
phrase: "2 of the diff's 3 sections",
phraseZh: 'diff 3 个片段中的 2 个',
plural: true,
});
});

it('still says something with no plan to count against', () => {
expect(describeChunkGap([7], [])).toEqual({
phrase: '1 section of the diff',
phraseZh: 'diff 中的 1 个片段',
plural: false,
});
expect(describeChunkGap([9, 7], [])).toEqual({
phrase: '2 sections of the diff',
phraseZh: 'diff 中的 2 个片段',
plural: true,
});
});
});

describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)', () => {
it('folds the complete Chinese version under the English body, footer outside the fold', () => {
// Not base(): its planPath default runs coveredPlan() again on the same
// path and would overwrite the han-stamped plan.
const r = composeReview({
suggestionsInline: 1,
planPath: coveredPlan(undefined, { han: true }),
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('COMMENT');
// English leads, untouched.
expect(
r.body.startsWith('Reviewed — no blockers. Suggestions are inline.'),
).toBe(true);
// The complete Chinese version rides collapsed.
expect(r.body).toContain('<details>\n<summary>中文说明</summary>');
expect(r.body).toContain('已审查——无阻断问题。 建议见行内评论。');
// One footer, after the fold — never inside it.
expect(r.body.endsWith(FOOTER)).toBe(true);
expect(r.body.split(FOOTER)).toHaveLength(2);
expect(r.body.indexOf('</details>')).toBeLessThan(r.body.indexOf(FOOTER));
});

it('stays English-only without the plan flag', () => {
const r = composeReview(base({ suggestionsInline: 1 }));
expect(r.body).not.toContain('<details>');
expect(r.body).not.toContain('中文');
});

it('translates the LGTM body', () => {
const r = composeReview({
planPath: coveredPlan(undefined, { han: true }),
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('APPROVE');
expect(r.body).toContain('No issues found. LGTM! ✅');
expect(r.body).toContain('未发现问题。LGTM!✅');
});

it('translates the disclosures — role phrase and Not-reviewed frame', () => {
// test-matrix required and never built → one role gap, both languages.
const p = plan({ han: true });
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 });
expect(r.body).toContain(
'Not reviewed: the whole-diff test-coverage check',
);
expect(r.body).toContain('未审查:全 diff 测试覆盖检查——');
// The zh sentence carries the translated reason, not the English one.
expect(r.body).toContain('没有记录表明它的 brief 到达过任何 agent');
});

it('quotes untranslatable caller text as-is in both halves', () => {
const r = composeReview({
suggestionsInline: 1,
cannotTellCriticals: ['old blocker at a.ts:1 — still reachable?'],
planPath: coveredPlan(undefined, { han: true }),
env: ENV,
modelId: MODEL,
});
expect(r.body).toContain('Unresolved, please confirm:');
expect(r.body).toContain('未决,请确认:');
// The caller's text, once per half.
expect(
r.body.match(/old blocker at a\.ts:1 — still reachable\?/g) ?? [],
).toHaveLength(2);
});
});
Loading
Loading