Skip to content
222 changes: 222 additions & 0 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@ import {
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { promptRecordDir, briefPath } from './lib/prompt-record.js';
import { getGhHost, setGhHost } from './lib/gh.js';
import {
composeReview,
composeReviewCommand,
describeChunkGap,
verdictLine,
type ComposeReviewInput,
type ComposeReviewResult,
type PrBodyFetcher,
} from './compose-review.js';

vi.mock('../../utils/stdioHelpers.js', () => ({
Expand All @@ -31,6 +33,15 @@ vi.mock('../../utils/stdioHelpers.js', () => ({
}));
import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js';

const ghMock = vi.hoisted(() => vi.fn((..._args: string[]) => ''));
vi.mock('./lib/gh.js', async (importOriginal) => {
const actual = await importOriginal<typeof import('./lib/gh.js')>();
return {
...actual,
gh: ghMock,
};
});

const MODEL = 'test-model';

// Coverage is read from the harness's transcripts on disk, so the fixtures build
Expand All @@ -43,6 +54,7 @@ beforeEach(() => {
dir = mkdtempSync(join(tmpdir(), 'compose-cov-'));
ENV = { QWEN_CODE_PROJECT_DIR: dir, QWEN_CODE_SESSION_ID: 'S1' };
mkdirSync(join(dir, 'subagents', 'S1'), { recursive: true });
ghMock.mockClear();
});

afterEach(() => {
Expand Down Expand Up @@ -817,6 +829,29 @@ describe('composeReviewCommand handler (the CLI glue)', () => {
expect(written.body.endsWith(FOOTER)).toBe(true);
});

it('routes its gh calls via the PR host — --host reaches setGhHost', () => {
// The bilingual body-language recovery calls `gh pr view`; on GitHub Enterprise
// that call must hit the PR's host, or the composed body's language disagrees
// with what `submit` (which routes by host) posts. Drop the `setGhHost(host)`
// and this reddens.
const dir = mkdtempSync(join(tmpdir(), 'compose-host-'));
const inputPath = join(dir, 'compose.json');
const commentsPath = join(dir, 'comments.json');
writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8');
writeFileSync(commentsPath, '[]', 'utf8');
setGhHost(undefined);
try {
(composeReviewCommand.handler as (argv: unknown) => void)({
input: inputPath,
comments: commentsPath,
host: 'github.example.com',
});
expect(getGhHost()).toBe('github.example.com');
} finally {
setGhHost(undefined);
}
});

it('a drafted inline Critical reaches the verdict line — the report-only hole', () => {
// The dogfooded failure this boundary exists for: a report-only run (no
// submit, so nothing downstream recounts) moved its one Critical from
Expand Down Expand Up @@ -2120,3 +2155,190 @@ describe('bilingual body — the PR author writes Chinese (prDescriptionHasHan)'
).toHaveLength(2);
});
});

/**
* The plan flag is the deterministic path; this is the recovery for when it is
* missing. `fetch-pr` always writes `prDescriptionHasHan`, but a `plan-diff`
* plan never does, and an orchestrator that improvises the pipeline can hand
* `compose-review` a plan that is not `fetch-pr`'s report — which is how a
* Chinese-authored PR (#7686) shipped an English-only review while the four
* bot reviews before it, off a proper plan, were bilingual. When the flag is
* absent but the plan still names the PR, the register is recovered from the
* live description, which the caller cannot forge.
*/
describe('bilingual body — recovered from the live PR when the plan omits the flag', () => {

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.

[Nit] Strong test battery for the recovery path

Seven tests covering the fallback matrix: Han present/absent in recovered body, recorded false short-circuits (no network), no PR identity short-circuits, fetch failure → English, production reader verifies exact gh args, and the handler strip guard against a model-supplied prBodyFetcher. The recordingFetcher pattern (proving it was NOT reached in the short-circuit tests) is a nice touch.

One gap worth considering: there's no test for the case where prDescriptionHasHan is present but with a non-boolean type (e.g. "true" as string, or null). The code correctly falls through to the recovery path in that case (line 1119 typeof === 'boolean' check), but a test asserting "string \"true\" triggers recovery, not silent acceptance" would guard against a future loosening of the type check.

/** A covered plan with a PR identity but no `prDescriptionHasHan`, its mtime
* kept old so its transcripts still read as newer than it. */
function namedPlanWithoutFlag(): string {
const p = coveredPlan();
const parsed = JSON.parse(readFileSync(p, 'utf8'));
delete parsed.prDescriptionHasHan;
parsed.ownerRepo = 'QwenLM/qwen-code';
parsed.prNumber = '7686';
writeFileSync(p, JSON.stringify(parsed));
const old = new Date(2020, 0, 1);
utimesSync(p, old, old);
return p;
}

/** A fetcher that records its calls, so a test can prove it was NOT reached. */
function recordingFetcher(body: string): PrBodyFetcher & { calls: number } {
const fn = ((_ownerRepo: string, _prNumber: string) => {
fn.calls++;
return body;
}) as PrBodyFetcher & { calls: number };
fn.calls = 0;
return fn;
}

it('folds in Chinese when the recovered description contains Han', () => {
const fetch = recordingFetcher('这个 PR 懒加载首次使用的依赖。');
const r = composeReview({
suggestionsInline: 1,
planPath: namedPlanWithoutFlag(),
prBodyFetcher: fetch,
env: ENV,
modelId: MODEL,
});
expect(fetch.calls).toBe(1);
// Both halves: the English rides above the fold, the Chinese inside it.
expect(r.body).toContain('<details>\n<summary>中文说明</summary>');
expect(r.body).toContain('Suggestions are inline.');
expect(r.body).toContain('建议见行内评论。');
});

it('stays English when the recovered description has no Han', () => {
const fetch = recordingFetcher(
'This PR lazy-loads first-use dependencies.',
);
const r = composeReview({
suggestionsInline: 1,
planPath: namedPlanWithoutFlag(),
prBodyFetcher: fetch,
env: ENV,
modelId: MODEL,
});
expect(fetch.calls).toBe(1);
expect(r.body).not.toContain('<details>');
expect(r.body).not.toContain('中文');
});

it('honours a recorded false without fetching — the English author is settled', () => {
// A real fetch-pr report that fetched the body and found no Han. Re-reading
// the live PR on every English review would be waste, and the recorded
// snapshot is the answer.
const p = coveredPlan();
const parsed = JSON.parse(readFileSync(p, 'utf8'));
parsed.prDescriptionHasHan = false;
parsed.ownerRepo = 'QwenLM/qwen-code';
parsed.prNumber = '7686';
writeFileSync(p, JSON.stringify(parsed));
const old = new Date(2020, 0, 1);
utimesSync(p, old, old);
const fetch = recordingFetcher('这段中文绝不该被读到。');
const r = composeReview({
suggestionsInline: 1,
planPath: p,
prBodyFetcher: fetch,
env: ENV,
modelId: MODEL,
});
expect(fetch.calls).toBe(0);
expect(r.body).not.toContain('<details>');
});

it('does not fetch when the plan carries no PR identity', () => {
const fetch = recordingFetcher('这段中文绝不该被读到。');
const r = composeReview({
suggestionsInline: 1,
planPath: coveredPlan(), // no ownerRepo/prNumber, no flag
prBodyFetcher: fetch,
env: ENV,
modelId: MODEL,
});
expect(fetch.calls).toBe(0);
expect(r.body).not.toContain('<details>');
});

it('falls back to English when the fetch throws — language never takes the review down', () => {
const boom: PrBodyFetcher = () => {
throw new Error('gh unreachable');
};
const r = composeReview({
suggestionsInline: 1,
planPath: namedPlanWithoutFlag(),
prBodyFetcher: boom,
env: ENV,
modelId: MODEL,
});
expect(r.event).toBe('COMMENT');
expect(r.body).not.toContain('<details>');
expect(r.body).not.toContain('中文');
expect(r.body).toContain('Suggestions are inline.');
});

it('the production reader calls gh pr view with the right args and parses the body', () => {
// All other tests in this block inject a fetcher, leaving fetchPrBodyViaGh —
// the only new production behaviour — unpinned. A wrong --json field, a
// dropped JSON.parse, or a body→bodyText slip would ship English-only reviews
// with CI clean. This test reddens under those mutants.
ghMock.mockReturnValue('{"body":"这个 PR 修复了双语渲染。"}');
const r = composeReview({
suggestionsInline: 1,
planPath: namedPlanWithoutFlag(),
env: ENV,
modelId: MODEL,
});
expect(ghMock).toHaveBeenCalledWith(
'pr',
'view',
'7686',
'--repo',
'QwenLM/qwen-code',
'--json',
'body',
);
expect(r.body).toContain('<details>\n<summary>中文说明</summary>');
});

it('strips a model-supplied prBodyFetcher — it cannot suppress the Chinese fold', () => {
// The handler deletes prBodyFetcher from the input JSON (the same way it
// deletes env). Without that delete, "suppress" reaches bilingualFromPlan,
// is called as a function, throws, and the catch drops the fold — the exact
// regression this PR closes, through the alternate entry point.
ghMock.mockReturnValue('{"body":"这个 PR 修复了双语渲染。"}');
const handlerDir = mkdtempSync(join(tmpdir(), 'compose-fetcher-'));
try {
const planPath = join(handlerDir, 'plan.json');
const p = namedPlanWithoutFlag();
writeFileSync(planPath, readFileSync(p, 'utf8'));
const old = new Date(2020, 0, 1);
utimesSync(planPath, old, old);
const inputPath = join(handlerDir, 'in.json');
writeFileSync(
inputPath,
JSON.stringify({
planPath,
prBodyFetcher: 'suppress',
modelId: MODEL,
}),
);
const commentsPath = join(handlerDir, 'comments.json');
writeFileSync(commentsPath, '[]', 'utf8');
const outPath = join(handlerDir, 'out.json');
(composeReviewCommand.handler as (argv: unknown) => void)({
input: inputPath,
comments: commentsPath,
out: outPath,
});
const written = JSON.parse(
readFileSync(outPath, 'utf8'),
) as ComposeReviewResult;
// If prBodyFetcher had NOT been stripped, "suppress" would throw and the
// fold would be absent. Its presence proves the handler stripped it.
expect(written.body).toContain('<details>\n<summary>中文说明</summary>');
} finally {
rmSync(handlerDir, { recursive: true, force: true });
}
});
});
Loading
Loading