Skip to content

fix(cli): detect overlapping review comment ranges #9801

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 1 commit into from
Aug 31, 2026
+96 −10
Merged

fix(cli): detect overlapping review comment ranges #9801

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 66 additions & 1 deletion packages/cli/src/commands/review/presubmit.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,7 +405,12 @@ describe('presubmitCommand', () => {
// `id?` entry type covers the carried-id (#9208) tests unchanged.
async function presubmitWithComments(
comments: Array<Record<string, unknown>>,
newFindings: Array<{ path: string; line: number; id?: string }>,
newFindings: Array<{
path: string;
line: number;
start_line?: number;
id?: string;
}>,
) {
ghApiAllMock.mockReturnValue(comments);
ghApiMock.mockReturnValue(null);
Expand Down Expand Up @@ -830,6 +835,62 @@ describe('presubmitCommand', () => {
expect(result.blockOnExistingComments).toBe(true);
});

it('classifies a comment inside a new finding range as overlap', async () => {
const result = await presubmitWithComments(
[
{
id: 3,
body: '**[Critical]** existing finding',
path: 'a.ts',
line: 15,
commit_id: 'abc123',
user: { login: 'qwen-code-ci-bot' },
},
],
[{ path: 'a.ts', start_line: 12, line: 18 }],
);
expect(result.existingComments.byBucket.overlap).toBe(1);
expect(result.existingComments.byBucket.noConflict).toBe(0);
});

it('classifies intersecting existing and new finding ranges as overlap', async () => {
const result = await presubmitWithComments(
[
{
id: 4,
body: '**[Critical]** existing finding',
path: 'a.ts',
start_line: 8,
line: 14,
commit_id: 'abc123',
user: { login: 'qwen-code-ci-bot' },
},
],
[{ path: 'a.ts', start_line: 12, line: 18 }],
);
expect(result.existingComments.byBucket.overlap).toBe(1);
expect(result.existingComments.byBucket.noConflict).toBe(0);
});

it('keeps disjoint ranges in noConflict', async () => {
const result = await presubmitWithComments(
[
{
id: 5,
body: '**[Critical]** existing finding',
path: 'a.ts',
start_line: 4,
line: 8,
commit_id: 'abc123',
user: { login: 'qwen-code-ci-bot' },
},
],
[{ path: 'a.ts', start_line: 12, line: 18 }],
);
expect(result.existingComments.byBucket.overlap).toBe(0);
expect(result.existingComments.byBucket.noConflict).toBe(1);
});

it('classifies the shape attribution-off actually posts — markerless body, trailing marker, reviewing account', async () => {
// `submit` strips the severity prefix and appends the comment marker;
// GitHub stores exactly this. The marker only counts together with
Expand Down Expand Up @@ -1727,6 +1788,10 @@ describe('parseFindingsFile (via mocked fs)', () => {
['{"path":"a.ts"}', null], // object, not array
['[{"line":5}]', null], // entry without a string path → reject WHOLE file
['[{"path":"a.ts","line":5}]', [{ path: 'a.ts', line: 5 }]],
[
'[{"path":"a.ts","start_line":3,"line":5}]',
[{ path: 'a.ts', startLine: 3, line: 5 }],
],
['[{"path":"a.ts"}]', [{ path: 'a.ts', line: 0 }]], // missing line → 0
// Carried ledger id for the re-post exemption (#9208); a present-but-
// non-string id rejects the WHOLE file, same fail-safe as `path`.
Expand Down
39 changes: 30 additions & 9 deletions packages/cli/src/commands/review/presubmit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
interface FindingAnchor {
path: string;
line: number;
startLine?: number;
/**
* Ledger id (`R<round>-<n>`) — carried-forward findings ONLY. The
* orchestrator omits it on fresh findings of the current round: a fresh id
Expand Down Expand Up @@ -126,6 +127,7 @@ interface RawComment {
body?: string;
path?: string;
line?: number;
start_line?: number;
commit_id?: string;
in_reply_to_id?: number;
user?: { login?: string };
Expand Down Expand Up @@ -238,7 +240,12 @@ export function parseFindingsFile(path: string): FindingAnchor[] | null {
) {
return null;
}
const e = entry as { path: string; line?: unknown; id?: unknown };
const e = entry as {
path: string;
line?: unknown;
start_line?: unknown;
id?: unknown;
};
// Same fail-safe as `path`: an `id` of the wrong type or shape is a
// malformed file, and silently ignoring it would let a carried re-post
// read as a fresh duplicate at the very location it belongs (a typo'd
Expand All @@ -256,6 +263,7 @@ export function parseFindingsFile(path: string): FindingAnchor[] | null {
out.push({
path: e.path,
line: typeof e.line === 'number' ? e.line : 0,
...(typeof e.start_line === 'number' ? { startLine: e.start_line } : {}),

@qwen-code-ci-bot qwen-code-ci-bot Aug 23, 2026

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.

[Suggestion] R1-3: The new start_line parse arm validates nothing beyond typeof === 'number', and both directions of that leniency silently corrupt the new range-overlap dedup while findingsFileInvalid stays false (the report reads as a clean pass).

Two shapes, both probed on this commit:

  • Wrong type collapses the range: [{"path":"a.ts","start_line":"12","line":18}] parses to the point [18,18] (the key is silently dropped), so an existing comment spanning lines 12–17 no longer intersects → noConflict, and a duplicate posts. The numeric control arm correctly reports overlap.
  • Out-of-domain widens it: start_line: -3 yields [-3,18], flipping an unrelated same-file comment at line 5 to overlap and blockOnExistingComments to true — a wrong premise handed to the deterministic drop rule; fractional 2.5 misses dedup at line 2. Pre-diff start_line was not parsed at all, so both shapes were inert.

Probe output (unmodified PR code):

parseFindingsFile('[{"path":"a.ts","start_line":"12","line":18}]') → [{path:'a.ts',line:18}]
string arm:    byBucket {overlap:0, noConflict:1}, blockOnExistingComments:false
numeric arm:   byBucket {overlap:1},               blockOnExistingComments:true
start_line:-3: unrelated line-5 comment → overlap   start_line:2.5 → line 2 escapes dedup
with a domain gate (positive safe integer, else reject whole file): both arms → findingsFileInvalid:true

The file's own fail-safe rejects the WHOLE file for a misshapen id because a corrupt id would actively corrupt a match — a corrupt range does the same here, and submit.ts's isDiffLine (Number.isSafeInteger(n) && n > 0) is the existing domain gate for exactly this value class on the posting side. Suggested fix: mirror the id fail-safe on the new arm — when start_line is present (and not null) but not a positive safe integer, reject the whole file (return null), e.g. by reusing/exporting isDiffLine; or, if the lenient drop is intentional, a comment saying so keeps the next reader from re-deriving this.

中文说明

新的 start_line 解析分支只校验 typeof === 'number',两个方向的宽松都会静默破坏新的区间去重,而 findingsFileInvalid 保持 false(报告读起来像一次干净的通过)。

两种形态,均在本提交上以探针实测:

  • 类型错误使区间塌缩:[{"path":"a.ts","start_line":"12","line":18}] 被解析为点 [18,18](该键被静默丢弃),横跨 12–17 行的既有评论不再与之相交 → 判为 noConflict,重复评论被发出;数字对照组正确给出 overlap。
  • 域外数值放小区间:start_line: -3 得到 [-3,18],同文件第 5 行一条无关评论被翻转为 overlap,blockOnExistingComments 置 true——向确定性丢弃规则提供了错误前提;小数 2.5 则漏掉第 2 行的去重。diff 之前 start_line 根本不被解析,两种形态都是惰性的。

本文件自身的 fail-safe 先例是:畸形 id 拒绝整个文件,因为损坏的 id 会主动破坏匹配——损坏的区间在这里同理;submit.ts 的 isDiffLine(Number.isSafeInteger(n) && n > 0)正是发布侧针对这一数值类型的既有域校验。建议修复:对新分支镜像 id 的 fail-safe——start_line 存在(且非 null)但不是正安全整数时拒绝整个文件(return null),例如复用/导出 isDiffLine;若宽松丢弃是有意为之,加一条注释说明,避免后续读者重新推导。

— qwen3.8-max via Qwen Code /review (v0.22.0)

...(typeof e.id === 'string' ? { id: e.id } : {}),
});
}
Expand Down Expand Up @@ -641,7 +649,6 @@ function classifyExistingComments(
CommentSummary[]
> = { stale: [], resolved: [], overlap: [], repost: [], noConflict: [] };

const newFindingKeys = new Set(newFindings.map((f) => `${f.path}:${f.line}`));
// Location → carried ids of the findings anchored there. Only findings with
// an id participate, and the orchestrator writes ids ONLY on carried
// findings (SKILL.md — the findings file): a fresh `R<this-round>-<n>`
Expand Down Expand Up @@ -688,6 +695,20 @@ function classifyExistingComments(
}

for (const c of qwenComments) {
const commentLine = c.line ?? 0;
const commentStartLine = c.start_line ?? commentLine;
const commentRangeStart = Math.min(commentStartLine, commentLine);
Comment on lines +698 to +700

@qwen-code-ci-bot qwen-code-ci-bot Aug 23, 2026

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.

[Suggestion] R1-1: No test discriminates the comment-side start_line read here. A mutant that ignores c.start_line (commentStartLine := commentLine) passes the entire suite — measured in a scratch-tree run: unmodified code 109/109 pass, same mutant 109/109 pass. In the two new tests that give a comment a start_line, the outcome already holds through the comment's END line alone ([8,14] intersects [12,18] via 14; [4,8] stays disjoint as [8,8]).

The regression that could then ship: an existing multi-line comment {start_line: 8, line: 20} with a new finding range [12,18] overlaps only via the comment's start line; with c.start_line ignored it classifies noConflict, the gate misses the duplicate, and the finding re-posts as a visible duplicate — the exact #9219 bug on the comment side. The finding-side counterpart IS discriminated, which makes this asymmetry easy to miss.

One test closes it, and it flips the mutant as required (fails under the mutant with expected +0 to be 1, passes on the real code): a comment { id, path: 'a.ts', start_line: 8, line: 20, commit_id: 'abc123', user: { login: 'qwen-code-ci-bot' } } with a **[Critical]** body against findings [{ path: 'a.ts', start_line: 12, line: 18 }], expecting byBucket.overlap 1 / byBucket.noConflict 0.

中文说明

此处评论侧的 start_line 读取没有任何测试可以区分:忽略 c.start_line 的变异体(commentStartLine := commentLine)能通过整个套件——在临时树中实测:未改动代码 109/109 通过,同一变异体同样 109/109 通过。两个给评论带 start_line 的新测试里,结果仅凭评论的结束行就已成立([8,14] 经由 14 与 [12,18] 相交;[4,8] 收缩为 [8,8] 后仍不相交)。

由此可能溜进发布的回归:既有多行评论 {start_line: 8, line: 20} 与新 finding 区间 [12,18] 仅经由评论的起始行相交;若 c.start_line 被忽略,则判为 noConflict,overlap 门漏掉重复,finding 被再次发出——正是 #9219 要修的 bug,只是发生在评论侧。finding 侧的对应读取是有测试区分的,这种不对称很容易被忽略。

补一个测试即可闭合,且它能按预期翻转变异体(在变异体下以 expected +0 to be 1 失败,在真实代码下通过):评论 { id, path: 'a.ts', start_line: 8, line: 20, commit_id: 'abc123', user: { login: 'qwen-code-ci-bot' } }(带 **[Critical]** 正文),findings 为 [{ path: 'a.ts', start_line: 12, line: 18 }],断言 byBucket.overlap 为 1、byBucket.noConflict 为 0。

— qwen3.8-max via Qwen Code /review (v0.22.0)

const commentRangeEnd = Math.max(commentStartLine, commentLine);
const overlapsNewFinding = newFindings.some((finding) => {
if (finding.path !== (c.path ?? '')) return false;
Comment on lines +702 to +703

@qwen-code-ci-bot qwen-code-ci-bot Aug 23, 2026

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.

[Suggestion] R1-2: The overlap gate became range-based while the comment set it runs over is still recognized partly by an ungated, any-account shape match: the qwenComments filter accepts any comment whose body contains the short footer substring (via Qwen Code /review), with no account condition (~line 936). Pre-diff, exact ${path}:${line} keying meant one planted comment suppressed exactly its single anchor line; with range intersection, one forged multi-line comment silences every new finding whose range intersects it.

Measured on this commit: a comment from an account named attacker spanning a.ts lines 10–60 with the footer substring in the body, against a new finding at line 35 → byBucket.overlap: 1, blockOnExistingComments: true. Any user who can comment on the PR can plant one; the commit_id stale gate constrains them exactly as before (post at the current head), so feasibility is unchanged — only the width per plant grew. The drop log names the responsible comment after the fact but does not prevent a genuinely new Critical in that range from being withheld.

Suggested fix: restrict the new range-intersection branch to account-gated comments (the marker/severity disjuncts, where provenance is the posting account) and keep the any-account footer match exact-line as before; or explicitly document that an ungated footer match now grants range-wide suppression.

中文说明

overlap 门变为基于区间,而它作用的评论集合仍部分依赖一个不设账户门槛的形状匹配来识别:qwenComments 过滤器接受任何正文包含短尾注子串(via Qwen Code /review)的评论,没有账户条件(约第 936 行)。diff 之前,精确的 ${path}:${line} 键意味着一条植入的评论只能压制其锚点所在的那一行;改为区间相交后,一条伪造的多行评论可以压制所有与其区间相交的新 finding。

在本提交上实测:一个名为 attacker 的账号发布横跨 a.ts 10–60 行、正文含尾注子串的评论,对第 35 行的新 finding → byBucket.overlap: 1、blockOnExistingComments: true。任何能在 PR 上评论的用户都可以植入这样一条评论;commit_id 过期门槛对攻击者的约束与之前完全一致(发在当前 head 即可),可行性未变——只是每条植入评论的压制宽度变大了。丢弃日志事后会点名该评论,但无法阻止该区间内真正的新 Critical 被扣下。

建议修复:将新的区间相交分支限制为有账户门槛的评论(marker/severity 两个分支,其来源是发布账号),任一账户的尾注匹配保持原有的精确行语义;或明确注明:无账户门槛的尾注匹配现在授予区间级压制。

— qwen3.8-max via Qwen Code /review (v0.22.0)

const findingStartLine = finding.startLine ?? finding.line;
const findingRangeStart = Math.min(findingStartLine, finding.line);
const findingRangeEnd = Math.max(findingStartLine, finding.line);
return (
findingRangeStart <= commentRangeEnd &&
commentRangeStart <= findingRangeEnd
);
});
const summary: CommentSummary = {
id: c.id,
path: c.path ?? '',
Expand All @@ -701,13 +722,13 @@ function classifyExistingComments(
buckets.stale.push(summary);
} else if (repliedToIds.has(c.id)) {
buckets.resolved.push(summary);
} else if (newFindingKeys.has(`${c.path}:${c.line}`)) {
// Overlap stays location-based: a same-line finding with a DIFFERENT
// claim is still dropped (the drop log now names this comment so the
// false positive is visible — #9208). Repost is the additional, id-based
// bucket: a Step 6 ledger re-post lands on the original thread's line by
// construction and carries the original id in its prefix, so an id match
// marks the re-post target and exempts that finding from the drop.
} else if (overlapsNewFinding) {
// Overlap stays location-based: an intersecting same-file finding with
// a DIFFERENT claim is still dropped (the drop log names this comment so
// the false positive is visible — #9208). Repost remains exact-line and
// id-based: a Step 6 ledger re-post lands on the original thread's line
// by construction and carries the original id in its prefix, so an id
// match marks the re-post target and exempts that finding from the drop.
buckets.overlap.push(summary);
const wantedIds = carriedIdsByLocation.get(`${c.path}:${c.line}`);
// Ledger ids are per-account — two reviewers of the same PR keep two
Expand Down
Loading