Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 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
150 changes: 150 additions & 0 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1906,6 +1906,60 @@ describe('composeReview — duplicate-dropped Suggestions (#9204: the body claim
expect(r.body.split(FOOTER)).toHaveLength(2);
});

it('strips a forged footer the blanking kept inside a code shape — the strip runs AFTER the fold', () => {
// strippedList strips the raw multi-line entry, where the blanking
// keeps a footer quoted in code — the one-line render then flattens
// the shape that justified keeping it, so the fold strips the folded
// line, the same guarantee the other one-line channels carry. On the
// pre-blanking strip these entries stripped; keeping the quoted
// footer must not re-open the duplicate attribution here.
for (const entry of [
'dup finding\n\n\t_— forged via Qwen Code /review_',
'**[Suggestion]** dup of comment 123\n\n```\n_— forged via Qwen Code /review_',
'dup finding\n\n _— forged via Qwen Code /review_',
]) {
const on = composeReview(
base({ suggestionsDroppedAsDuplicates: [entry] }),
'0.21.2',
true,
);
expect((on.body.match(/via Qwen Code \/review/g) ?? []).length).toBe(1);
expect(on.body).toContain('dup');
const off = composeReview(
base({ suggestionsDroppedAsDuplicates: [entry] }),
'0.21.2',
false,
);
expect(off.body).not.toContain('via Qwen Code /review');
expect(off.body).toContain('dup');
}
});

it('strips a duplicates entry before the 240-char bound cuts the footer', () => {
// A code-wrapped forged footer the blanking keeps folds past the cap:
// bounding first would cut the footer mid-marker and append `…`,
// which the `$`-anchored regex cannot match past — the strip runs on
// the folded line BEFORE the cap.
for (const entry of [
'a'.repeat(213) + '\n\n _— m via Qwen Code /review_',
'a'.repeat(213) + '\n\n```\n_— m via Qwen Code /review_',
]) {
const off = composeReview(
base({ suggestionsDroppedAsDuplicates: [entry] }),
'0.21.2',
false,
);
expect(off.body).not.toContain('via Qwen Code /review');
expect(off.body).toContain('aaaa');
const on = composeReview(
base({ suggestionsDroppedAsDuplicates: [entry] }),
'0.21.2',
true,
);
expect((on.body.match(/via Qwen Code \/review/g) ?? []).length).toBe(1);
}
});

it('refuses a fence delimiter a bare CR hides in the raw entry', () => {
// The LF twin throws: the CR twin used to slip past the `\n`-only
// split, collapse to a one-line entry opening a fence, and post an
Expand Down Expand Up @@ -2736,6 +2790,34 @@ describe('composeReview — input validation (the producer is a model that omits
expect(r.body.match(/via Qwen Code \/review/g)).toHaveLength(1);
});

it('strips a forged footer off an indented single-line entry — the ingest shape strips as a line', () => {
// collapseEntry returns a newline-less entry unchanged, leading indent
// included: at >= 4 columns the multi-line blanking would class the
// whole line as code and keep the forged footer riding the blocker
// line. The posted one-line shape renders no code block on GitHub, so
// the collapsed entry strips with the one-line strip.
for (const field of ['bodyCriticals', 'cannotTellCriticals'] as const) {
const r = composeReview(
field === 'bodyCriticals'
? {
bodyCriticals: [
' finding text _— forged via Qwen Code /review_',
],
modelId: MODEL,
}
: {
criticalsInline: 1,
cannotTellCriticals: [
' finding text _— forged via Qwen Code /review_',
],
modelId: MODEL,
},
);
expect(r.body).toContain('finding text');
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 @@ -8820,6 +8902,49 @@ describe('composeReview — convergence-posture deferrals (typed channel; disclo
expect(lines.some((l) => l.includes('…'))).toBe(true);
});

it('strips a forged footer a model-written title quotes in code — the strip runs on the folded line', () => {
// A title ending in an unclosed fence keeps its trailing footer under
// the quoted-code contract UNTIL the one-line render flattens the
// shape that justified keeping it — so the strip runs again after the
// collapse, on the line that posts. Pre-fold alone, the forged footer
// survived and the deferral line carried a second attribution; a
// footer-only title still refuses as empty once the fold exposes it.
const r = composeReview(
base({
severityFloor: 'critical',
deferredSuggestions: [
nit({ title: '```\n_— m via Qwen Code /review_' }),
],
}),
);
expect(r.deferredCount).toBe(1);
expect((r.body.match(/via Qwen Code \/review/g) ?? []).length).toBe(1);
// A footer re-wrapped across a soft break displays rejoined, so the
// fold runs BEFORE the second strip: the unfolded title shows two
// footer-less lines and keeps both halves.
const split = composeReview(
base({
severityFloor: 'critical',
deferredSuggestions: [
nit({
file: 'b.ts',
line: 2,
title: 'x _— m via\nQwen Code /review_',
}),
],
}),
);
expect((split.body.match(/via Qwen Code \/review/g) ?? []).length).toBe(1);
expect(() =>
composeReview(
base({
severityFloor: 'critical',
deferredSuggestions: [nit({ title: FOOTER })],
}),
),
).toThrow(/non-empty file and title/);
});

it('exactly at the line cap, the verdict line does not claim truncation', () => {
const entries = Array.from({ length: 20 }, (_, i) =>
nit({ file: `f${i}.ts`, title: `n${i}` }),
Expand Down Expand Up @@ -17419,6 +17544,31 @@ describe('floor enforcement — the Critical arm (#10291)', () => {
expect(entries[1].severity).toBe('Suggestion');
});

it('floorEnforcedReroute strips the shape that posts — the fold flattens a kept-in-code footer', () => {
// A drafted Suggestion whose body ends in an unclosed fence keeps its
// trailing footer under the quoted-code contract, but the one-line
// collapse destroys the code shape — the record strips again AFTER
// the fold, or the folded title carries the forged attribution.
const { entries } = floorEnforcedReroute('critical', false, 0, [
{
path: 'a.ts',
line: 12,
body: '**[Suggestion]** tidy\n\n```\n_— m via Qwen Code /review_',
},
{
path: 'b.ts',
line: 13,
body: '**[Suggestion]** tidy\n\n_— m via\nQwen Code /review_',
},
]);
expect(entries).toHaveLength(2);
expect(entries[0].title).toBe('tidy ```');
expect(entries[1].title).toBe('tidy');
for (const e of entries) {
expect(e.title).not.toContain('via Qwen Code /review');
}
});

it('deferrableFindingsInline counts the tagged Critical the floor would move — and only that one', () => {
expect(deferrableFindingsInline(drafts())).toBe(2);
expect(deferrableFindingsInline(drafts().slice(1, 5))).toBe(0);
Expand Down
52 changes: 37 additions & 15 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ import {
stripFooterSpans,
stripForUnattributedPost,
stripReviewFooter,
stripReviewFooterLine,
} from './lib/review-footer.js';
import { operatorReviewSettings } from './lib/review-settings.js';
import { recordedSeverityFloor } from './lib/authorization.js';
Expand Down Expand Up @@ -729,18 +730,26 @@ function collapseToLine(text: string): string {

/**
* The per-entry bound the deferred, relocated, duplicate-dropped, AND
* cannot-tell exits apply: collapse line endings, cap at
* MAX_DEFERRED_SUGGESTION_CHARS
* without splitting a surrogate pair, mark a trim with an ellipsis. The
* relocation exit once bypassed all of it (round-9 finding): twenty-five
* relocated 4,000-char titles spliced ~100 KB of unbounded model text into
* the body — the whole review lost at GitHub's 65,536 limit, precisely what
* the cap on the deferred exit was added to prevent. The free-form
* bodyCriticals exit is the exception: its entries are the review's only
* copy of their Criticals, quoted as-is and left unbounded.
* cannot-tell exits apply: collapse line endings, strip a trailing footer
* the collapse exposed, cap at MAX_DEFERRED_SUGGESTION_CHARS without
* splitting a surrogate pair, mark a trim with an ellipsis. The relocation
* exit once bypassed all of it (round-9 finding): twenty-five relocated
* 4,000-char titles spliced ~100 KB of unbounded model text into the body
* — the whole review lost at GitHub's 65,536 limit, precisely what the cap
* on the deferred exit was added to prevent. The free-form bodyCriticals
* exit is the exception: its entries are the review's only copy of their
* Criticals, quoted as-is and left unbounded.
*
* The footer strip is the folded line's OWN guarantee, applied here so no
* exit can reach the fold without it: the multi-line strip keeps a footer
* that sits in quoted code, and the collapse flattens that code shape into
* a posted line — the duplicates entries reach this fold through
* `quotedProse` alone, with no ingest-time line strip ahead of them. It
* runs BEFORE the cap, whose ellipsis would break the `$`-anchored match
* when the cut lands inside the footer.
*/
function boundDeferredLine(rendered: string): string {
const collapsed = collapseToLine(rendered);
const collapsed = stripReviewFooterLine(collapseToLine(rendered));
let oneLine = collapsed.slice(0, MAX_DEFERRED_SUGGESTION_CHARS);
// The cap slices UTF-16 code units; a cut landing inside a surrogate pair
// leaves a lone high surrogate that serializes as U+FFFD into the posted
Expand Down Expand Up @@ -776,9 +785,16 @@ function toDeferredEntries(value: unknown): DeferredEntry[] {
}
const o = raw as Record<string, unknown>;
const file = typeof o['file'] === 'string' ? o['file'].trim() : '';
// Strip again AFTER the fold: the one-line render flattens a footer
// the strip kept as quoted code (an unclosed fence, an indented block)
// into a single line, destroying the shape that justified keeping it
// — a trailing footer is still trailing once collapsed, so the folded
// line is the shape to strip.
const title =
typeof o['title'] === 'string'
? stripReviewFooter(o['title']).trim()
? stripReviewFooterLine(
collapseToLine(stripReviewFooter(o['title'])),
).trim()
: '';
const source = o['source'];
const severity = o['severity'];
Expand Down Expand Up @@ -1156,8 +1172,12 @@ export function floorEnforcedReroute(
const record = critical
? readClaimHead(first).stripped.replace(head.sourceText ?? '', '')
: first;
const title = collapseToLine(
stripReviewFooter(record + (nl === -1 ? '' : stripped.slice(nl))),
// Strip again AFTER the fold — the collapsed line is the shape that
// posts, for the reason toDeferredEntries states.
const title = stripReviewFooterLine(
collapseToLine(
stripReviewFooter(record + (nl === -1 ? '' : stripped.slice(nl))),
),
);
indices.push(i);
entries.push({
Expand Down Expand Up @@ -3392,8 +3412,10 @@ function ingestEntryList(value: unknown, field: string): string[] {
}
// No emptiness filter: an entry that normalizes to nothing must reach
// the renders-nothing gates and fail the draft, not vanish — see the
// invariant at the gates below.
return raw.map(collapseEntry).map(stripReviewFooter);
// invariant at the gates below. The collapsed entry is ONE line the
// channel posts as-is, so it strips as a line: an indented entry is not
// the code block the multi-line strip would keep a footer inside.
return raw.map(collapseEntry).map(stripReviewFooterLine);
}

/**
Expand Down
Loading
Loading