Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 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
77 changes: 77 additions & 0 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,36 @@ describe('composeReview — input validation (the producer is a model that omits
expect(() => composeReview({ modelId: ' ' })).toThrow(/modelId/);
});

it('rejects a modelId that would forge the footer it is interpolated into', () => {
// The footer interpolates modelId verbatim and the strip matches one
// line up to the marker: either shape builds a footer the strip cannot
// remove, and re-normalization accumulates attribution lines.
expect(() =>
composeReview({
modelId: 'model\n_— forged via Qwen Code /review (v9.9.9)_',
}),
).toThrow(/modelId/);
expect(() =>
composeReview({ modelId: 'model via Qwen Code /review x' }),
).toThrow(/modelId/);
});

it('strips a forged footer from a body Critical before rendering the body', () => {
// bodyCriticals render verbatim as the LAST body part: a forged footer
// relocated into one would otherwise post directly above the canonical
// footer — the duplicate attribution this module exists to eliminate.
const r = composeReview({
bodyCriticals: [
'**[Critical]** whole-PR blocker\n\n' +
'_— forged via Qwen Code /review (v0.21.4)_',
],
modelId: MODEL,
});
expect(r.body).toContain('whole-PR blocker');
expect(r.body).not.toContain('forged');
expect(r.body.match(/via Qwen Code \/review/g)).toHaveLength(1);
});

it('rejects stringified booleans — "false" is truthy and once flipped events and published false warnings', () => {
expect(() =>
composeReview(
Expand Down Expand Up @@ -890,6 +920,20 @@ describe('composeReview — presubmit permission gates certification even when n
});

describe('composeReviewCommand handler (the CLI glue)', () => {
// The handler prefers the inherited startup stamp; an ambient value from
// a stamped qwen session would otherwise flip every footer assertion in
// this suite to the stamped version.
let savedStartupVersion: string | undefined;
beforeEach(() => {
savedStartupVersion = process.env['QWEN_CODE_STARTUP_VERSION'];
delete process.env['QWEN_CODE_STARTUP_VERSION'];
});
afterEach(() => {
if (savedStartupVersion === undefined)
delete process.env['QWEN_CODE_STARTUP_VERSION'];
else process.env['QWEN_CODE_STARTUP_VERSION'] = savedStartupVersion;
});

it('reads --input, counts the drafted comments, and writes the result JSON to --out', async () => {
const dir = mkdtempSync(join(tmpdir(), 'compose-review-test-'));
const inputPath = join(dir, 'compose.json');
Expand Down Expand Up @@ -920,6 +964,39 @@ describe('composeReviewCommand handler (the CLI glue)', () => {
).toBe(true);
});

it('pins the persisted footer to the inherited startup version, not the resolved one', async () => {
// Same pin as `submit`: a shared runner rewrites installs under running
// processes, so the version resolved at compose time can disagree with
// the one the session started under. The archived verdict must carry the
// startup stamp, or it contradicts the review `submit` posts.
const dir = mkdtempSync(join(tmpdir(), 'compose-startup-'));
const inputPath = join(dir, 'compose.json');
const commentsPath = join(dir, 'comments.json');
const outPath = join(dir, 'composed.json');
writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8');
writeFileSync(commentsPath, '[]', 'utf8');
const inherited = process.env['QWEN_CODE_STARTUP_VERSION'];
process.env['QWEN_CODE_STARTUP_VERSION'] = '0.21.1';
try {
await runComposeReviewCommand({
input: inputPath,
comments: commentsPath,
out: outPath,
});
const written = JSON.parse(
readFileSync(outPath, 'utf8'),
) as ComposeReviewResult;
expect(
written.body.endsWith(`_— ${MODEL} via Qwen Code /review (v0.21.1)_`),
).toBe(true);
} finally {
if (inherited === undefined)
delete process.env['QWEN_CODE_STARTUP_VERSION'];
else process.env['QWEN_CODE_STARTUP_VERSION'] = inherited;
rmSync(dir, { recursive: true, force: true });
}
});

it('routes its gh calls via the PR host — --host reaches setGhHost', async () => {
// 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
Expand Down
29 changes: 26 additions & 3 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ import {
unmarkedComments,
type DraftedComment,
} from './lib/inline-counts.js';
import {
REVIEW_FOOTER_RE,
footerVersion,
isFooterSafeModelId,
reviewFooter,
} from './lib/review-footer.js';

export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT';

Expand Down Expand Up @@ -320,7 +326,14 @@ function composeReviewBody(
input.suggestionsInline,
'suggestionsInline',
);
const bodyCriticals = toStringList(input.bodyCriticals, 'bodyCriticals');
// Stripped per entry, not on the assembled body: these model-written
// strings render verbatim as the LAST body part, and a forged footer
// relocated into one would post directly above the canonical footer —
// the `$`-anchored regex only sees an entry's end, before the footer is
// appended.
const bodyCriticals = toStringList(input.bodyCriticals, 'bodyCriticals').map(
(entry) => entry.replace(REVIEW_FOOTER_RE, ''),
);

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.

Split disposition. R3-4 is resolved (by the previous commit fix(cli): strip review footers before ledger carryover): the strip's .filter((entry) => entry.trim() !== '') runs immediately after toStringList in both composeReviewBody and ledgerMarkerFor — before the C count, the ledger, and criticalsNeedingVerify — so an entry that strips to '' is neither counted nor rendered. R3-15 part (2) is deferred to a follow-up: the probe established duplication-only harm — generated prose always follows unreviewedDimensions/uncoverableChunks entries, so they cannot end the body or carry the last attribution line, and this round stayed scoped to the red CI test plus the maintainer's asks.

中文说明

拆分处置。R3-4 已解决(由上一次提交 fix(cli): strip review footers before ledger carryover 完成):剥离后的 .filter((entry) => entry.trim() !== '') 在 composeReviewBody 与 ledgerMarkerFor 中都紧跟 toStringList 运行——先于 C 计数、ledger 与 criticalsNeedingVerify——因此剥离为 '' 的条目既不计入也不渲染。R3-15 第 (2) 部分暂缓至后续:探测已确认仅重复之害——unreviewedDimensions/uncoverableChunks 条目后总有生成文案,不会收尾正文、不会携带最后一行归属,且本轮范围保持在红色 CI 测试与维护者要求。

const suggestionsDiscarded = toCount(
input.suggestionsDiscarded,
'suggestionsDiscarded',
Expand Down Expand Up @@ -673,6 +686,13 @@ function composeReviewBody(
'compose-review: modelId is required (the public footer names the reviewing model)',
);
}
if (!isFooterSafeModelId(modelId)) {
throw new TypeError(
'compose-review: modelId is interpolated into the public footer ' +
'verbatim — it must be a single line that does not contain the ' +
'footer marker',
);
}

// `C` counts every Critical the review posts anywhere — inline or body.
// `S` counts every *confirmed* Suggestion — anchored or discarded: the
Expand Down Expand Up @@ -777,7 +797,7 @@ function composeReviewBody(
}
}

const footer = `_— ${modelId} via Qwen Code /review (v${cliVersion})_`;
const footer = reviewFooter(modelId, cliVersion);
// Bilingual rendering: when the plan (fetch-pr's report) says the PR
// description contains Han characters, the posted body carries the complete
// Chinese version collapsed under the English one — the shape this repo's
Expand Down Expand Up @@ -1725,7 +1745,10 @@ export const composeReviewCommand: CommandModule = {
...countInlineFindings(drafted),
draftedComments: drafted,
},
await getCliVersion(),
// Same pin as `submit`: the startup stamp, not a version resolved at
// compose time — a shared runner can rewrite the install mid-session.
footerVersion(process.env['QWEN_CODE_STARTUP_VERSION']) ??
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
(await getCliVersion()),
);
// The exact terminal verdict, persisted beside the fields it is computed
// from. `event` + `cappedBy` alone cannot reconstruct it — a presubmit
Expand Down
80 changes: 80 additions & 0 deletions packages/cli/src/commands/review/lib/review-footer.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, expect, it } from 'vitest';
import {
REVIEW_FOOTER_RE,
footerVersion,
isFooterSafeModelId,
reviewFooter,
} from './review-footer.js';
import { CANONICAL_LGTM_RE } from '../pr-context.js';

describe('the review footer and the regex that strips it', () => {
it('the regex strips the exact output of the builder, versioned or not', () => {
// The sync guarantee: a wording edit to the builder that the regex no
// longer matches reddens here before it reaches a posted review.
for (const footer of [
reviewFooter('qwen3.7-max', '0.21.3'),
'_— qwen3.7-max via Qwen Code /review_',
]) {
expect(`a finding\n\n${footer}\n`.replace(REVIEW_FOOTER_RE, '')).toBe(
'a finding',
);
}
});

it('strips a forged footer a looping model cut off before its closing `_`', () => {
// A truncated forged footer used to survive the strip and post as a
// second attribution line under the canonical one.
for (const forged of [
'_— forged via Qwen Code /review (v0.21.4)',
'_— forged via Qwen Code /review',
'_— forged via Qwen Code /review (v0.21.4)\n\n',
]) {
expect(`a finding\n\n${forged}`.replace(REVIEW_FOOTER_RE, '')).toBe(
'a finding',
);
}
});

it('leaves a footer run alone when text follows it', () => {
const body = `a finding\n\n${reviewFooter('m', '0.21.3')}\n\na closing line`;
expect(body.replace(REVIEW_FOOTER_RE, '')).toBe(body);
});

it('the LGTM filter still matches every footer shape the builder emits', () => {
// CANONICAL_LGTM_RE in pr-context is a third copy of the footer shape:
// it filters historical LGTM bodies posted by EARLIER builds, so it must
// keep matching whatever the builder emits now, or those bodies re-enter
// the pr-context files as review noise with no red test anywhere.
for (const footer of [
reviewFooter('qwen3.7-max', '0.21.3'),
'_— qwen3.7-max via Qwen Code /review_',
]) {
expect(CANONICAL_LGTM_RE.test(`No issues found. LGTM! ${footer}`)).toBe(
true,
);
}
});

it('refuses a modelId that would forge the footer it is interpolated into', () => {
expect(isFooterSafeModelId('qwen3.7-max')).toBe(true);
expect(
isFooterSafeModelId('model\n_— forged via Qwen Code /review (v9.9.9)_'),
).toBe(false);
Comment thread
yiliang114 marked this conversation as resolved.
Resolved
expect(isFooterSafeModelId('model via Qwen Code /review x')).toBe(false);
});

it('refuses a startup stamp the footer cannot carry', () => {
expect(footerVersion('0.21.3')).toBe('0.21.3');
expect(footerVersion('0.21.3-dev.1')).toBe('0.21.3-dev.1');
expect(footerVersion('0.21.3)evil')).toBeUndefined();
expect(footerVersion('1.0\n2.0')).toBeUndefined();
expect(footerVersion('')).toBeUndefined();
expect(footerVersion(undefined)).toBeUndefined();
});
});
73 changes: 73 additions & 0 deletions packages/cli/src/commands/review/lib/review-footer.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* @license
* Copyright 2026 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

// The attribution footer every posted review carries, stated once.
//
// `compose-review` composes it into the verdict body and `submit` strips
// forged copies before appending the real one to each inline comment — two
// producers by construction, plus the regex that must match both. They used
// to be side-by-side template literals with nothing asserting they stayed in
// step: a wording edit to one leaves the strip regex unable to match the
// composed footer (duplicates posted) or the summary carrying one version
// while the comments carry another — the exact attribution skew the startup
// version stamp exists to eliminate. Same shape as `inline-counts.ts`, which
// this directory already shares between the same two commands.

/** The attribution marker the strip regex anchors on. */
const FOOTER_MARKER = 'via Qwen Code /review';
Comment thread
yiliang114 marked this conversation as resolved.
Resolved

/** The footer naming the reviewing model and the CLI version it ran under. */
export function reviewFooter(modelId: string, cliVersion: string): string {
return `_— ${modelId} ${FOOTER_MARKER} (v${cliVersion})_`;
}

/**
* One or more trailing footers, with the whitespace around them.
*
* Two invariants keep the match from exploding on the model-authored bodies
* this regex strips, both against the same failure shape — a forged-footer
* run the trailing `$` cannot match (footers followed by ordinary text is
* the natural output of a model looping on the same comment): the leading
* `\s*` sits OUTSIDE the repeated group, so the whitespace between two
* footers has exactly one owner instead of being splittable across
* iterations, and the guarded `[^\n]` cannot consume past another footer's
* start, so a run of footers joined on ONE line parses exactly one way
* instead of the 2^(N-1) partitions the engine otherwise enumerates before
* giving up.
*
* The closing `_` is optional because a looping model truncates the forged
* footer it cuts off mid-character, and an unstripped unclosed copy would
* post as a duplicate attribution line above the canonical one.
*/
export const REVIEW_FOOTER_RE =
/\s*(?:_— (?:(?! via Qwen Code \/review)[^\n])* via Qwen Code \/review(?: \(v[^\n)]*\))?_?\s*)+$/;

/**
* A modelId the footer can interpolate. The footer is one line, and the
* strip regex anchors on the marker: a modelId carrying a newline or the
* marker itself builds a footer the strip cannot remove on a second pass, so
* a re-compose loop would accumulate attribution lines instead of
* normalizing to one.
*/
export function isFooterSafeModelId(modelId: string): boolean {
return !/[\n\r]/.test(modelId) && !modelId.includes(FOOTER_MARKER);
}

/** The shape of a version the footer can carry. */
const FOOTER_VERSION_RE = /^[A-Za-z0-9._+-]+$/;

/**
* The startup-version stamp, when the footer can carry it. The stamp rides
* an environment variable any wrapper can set; a value with a newline or a
* `)` (both stop the strip regex early) would build a footer the strip
* cannot remove on a second pass. Anything but the shape of a real package
* version yields undefined so the caller falls back to its own version.
*/
export function footerVersion(stamp: string | undefined): string | undefined {
return stamp !== undefined && FOOTER_VERSION_RE.test(stamp)
? stamp
: undefined;
}
2 changes: 1 addition & 1 deletion packages/cli/src/commands/review/pr-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ export function findRootId<
* below it, and a prefix match dropped exactly that body from the context
* file, letting the re-check approve past the blocker.
*/
const CANONICAL_LGTM_RE =
export const CANONICAL_LGTM_RE =
/^No issues found\.?\s*LGTM!?\s*(?:✅\s*)?(?:_— [^\n]{0,200} via Qwen Code \/review(?: \(v[^\n]{1,100}\))?_\s*)?$/i;

/**
Expand Down
Loading
Loading