fix(cli): detect overlapping review comment ranges - #9801
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, not theoretical. Linked #9219 documents a concrete occurrence (a manual Direction: aligned — this is the project's own review-tooling dedup gate, fixing the first bullet of an open, reproduced P2 bug. CHANGELOG has no direct reference (internal tooling), but the area is squarely relevant. Doesn't touch auth/sandbox/model-selection/telemetry or any public contract. Size: not applicable — Approach: focused and minimal — carry Risk: no elevated risk signals — neither changed file matches the revert-correlated high-risk paths. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 是已观测到的 bug，不是理论性问题。关联的 #9219 记录了具体场景（对 PR 9204、commit 方向： 对齐——这是项目自身 review 工具的去重门，修复一个已复现的 P2 bug 的第一条。CHANGELOG 无直接引用（内部工具），但该领域明显相关。不触及 auth/沙箱/模型选择/遥测或任何公共契约。 规模： 不适用—— 方案： 聚焦且最小——把 风险： 无升级风险信号——两个改动文件均未命中与回滚相关的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewRead against The command-side change is correct and careful. The range math normalizes missing/reversed ends with The open question is the seam with the report's consumer — the bundled
So, @bluefateludi — was the skill-side half (findings-file format + report shape + drop-rule wording) meant as a follow-up, or could it ride along? The classification change stands on its own as a necessary first step; asking whether the plan for the second half exists before this merges, since landing just this half produces a report state the documented consumer contract can't consume deterministically. @wenshao — you own this subsystem and merged the #9212/#9629 presubmit changes, so this is ultimately your call. Test evidenceThis is an unattended CI run — PR code is never executed here by policy, so the evidence is the PR's own CI read through the API. On the reviewed commit, the PR's CI has not run yet: both Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Sandboxed verification would settle the behavioural claim: 中文说明代码审查针对 命令侧的改动正确且细致。 区间计算用 未决问题在于与报告消费方——内置
所以，@bluefateludi——skill 侧那一半（findings 文件格式 + 报告形状 + 删除规则措辞）是计划作为后续跟进，还是可以随本 PR 一起？分类改动本身作为必要的第一步是成立的；想问的是在本 PR 合入之前，第二半的计划是否已经存在——因为只落这一半会产生一种文档化消费契约无法确定性消费的报告状态。@wenshao——这个子系统由你维护，#9212/#9629 的 presubmit 改动也是你合入的，最终由你定夺。 测试证据这是无人值守的 CI 运行——按策略此处从不执行 PR 代码，证据是通过 API 读取的 PR 自身 CI。在被审查的 commit 上，PR 的 CI 尚未运行： 沙箱验证可以坐实行为性声明： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — correct, conservative, and well-tested inside its stated scope; but the consumer seam means #9219's motivating scenario is not caught end-to-end yet, and no CI has run on this commit at all. Stepping back: the command-side half of this is exactly the shape the fix should take — range carried on both ends, normalized interval intersection, the exact-anchor re-post exemption preserved, single-line behavior provably unchanged, and not a drive-by edit in the diff. If I were maintaining ⏸️ Deferring to @wenshao — needs a human call on two things:
No changes requested — nothing in the diff needs reworking. This is a completeness call, not a defect report. @bluefateludi, nice, disciplined change within its scope; the above is the one seam worth settling before it lands. 中文说明信心度：3/5 —— 在其声明范围内正确、保守且测试充分；但消费方接缝意味着 #9219 的动机场景端到端仍未被拦截，而且该 commit 上根本还没有 CI 运行过。 退一步看：命令侧这一半正是这个修复应有的形状——两端携带范围、归一化的区间相交、精确定锚的 re-post 豁免得以保留、单行行为可证明地不变，diff 里没有一处顺手改动。如果六个月后由我维护 ⏸️ 转交 @wenshao —— 有两件事需要人来定夺：
不请求修改——diff 中没有需要重写的东西。这是完整性判断，不是缺陷报告。@bluefateludi，在其范围内这是一次有章法的改动；合入前值得敲定的就是上面那个接缝。 — Qwen Code · qwen3.8-max Reviewed at |
|
Thanks for the review. This PR intentionally focuses on command-side range support. I can address the /review skill integration in a follow-up, or include it here if maintainers prefer. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 1a": run packages/cli presubmit.test.ts under vitest (blocked: no node_modules in worktree; full install not attempted); "agent 1a": npm run typecheck over the changed files (blocked: same missing-dependency condition).
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 1a"：run packages/cli presubmit.test.ts under vitest (blocked: no node_modules in worktree; full install not attempted)；"agent 1a"：npm run typecheck over the changed files (blocked: same missing-dependency condition)。
— qwen3.8-max via Qwen Code /review (v0.22.0)
| out.push({ | ||
| path: e.path, | ||
| line: typeof e.line === 'number' ? e.line : 0, | ||
| ...(typeof e.start_line === 'number' ? { startLine: e.start_line } : {}), |
There was a problem hiding this comment.
[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 reportsoverlap. - Out-of-domain widens it:
start_line: -3yields[-3,18], flipping an unrelated same-file comment at line 5 tooverlapandblockOnExistingCommentsto true — a wrong premise handed to the deterministic drop rule; fractional2.5misses dedup at line 2. Pre-diffstart_linewas 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)
| const commentLine = c.line ?? 0; | ||
| const commentStartLine = c.start_line ?? commentLine; | ||
| const commentRangeStart = Math.min(commentStartLine, commentLine); |
There was a problem hiding this comment.
[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 overlapsNewFinding = newFindings.some((finding) => { | ||
| if (finding.path !== (c.path ?? '')) return false; |
There was a problem hiding this comment.
[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)
Local real-environment verification reportI built the merge of this PR with current Verdict — the code is correct and safe, but the loop is not closed. The behaviour change is real and it fires on ~1 in 5 of our actual review comments. Nothing regresses: exact-line matching, the carried-id re-post exemption, the same-file guard and disjoint ranges are all unchanged, and no finding that used to be dropped stops being dropped. What I could not confirm is that the change produces the outcome the description claims: the half of it the description leads with is unreachable in production today, and the half that does fire raises Environment
1. Static checks — all green
2. Same-tree A/B on a real GitHub PRTwo bundles from one tree —
The real multi-line comment the fix keys on — GitHub really does return
P1 is the fix working, on a real comment, through the real Reachability is real, not theoretical. Across 8 recent PRs, 97 of 455 Qwen
Their spans: median 3 lines, p90 10, max 141 (this one, 3. Gap A — the direction the description leads with cannot happen in productionThe description says the bug is
My probe P3 only flips because I hand-wrote Two of the three new tests ( 4. Gap B — the overlap fires, but Step 7's drop rule has no target
Under range matching they no longer coincide. In P1 the report hands back posting.md is explicit that this decision point must stay deterministic ("this has been improvised as an interactive question, which stalls a headless run forever"). A block with an empty drop set is exactly the state that invites improvisation. 5. Gap C — a new false-positive class, from posting.md's own instructionsposting.md tells the orchestrator to include body-only Criticals in the findings file with a placeholder line:
P5 does exactly that — 6. Gap D — the production-reachable half is untestedMutants applied to
M4 is the important one. Deleting the comment-side RecommendationThe change is safe to merge and I found no regression. But as it stands it broadens a detector without moving the consumer contract, so the outcome it advertises — a duplicate that stops being posted — does not actually happen. Three small follow-ups would close it, and they belong in this PR rather than a new one:
Happy to be talked out of any of this — everything above is reproducible from 中文版本本地真实环境验证报告我把本 PR 与当前 结论 —— 代码正确且安全，但链路没有闭合。 行为变更是真实的，在我们实际的 review 评论里约 1/5 会触发。没有任何回归：精确行匹配、carried-id re-post 豁免、同文件守卫、不相交区间全部不变，原本会被丢弃的 finding 也不会变成不丢弃。我没能确认的是它能否产出描述里声称的结果：描述主打的那半边在当前生产环境根本走不到，而真正会触发的那半边会把 环境
1. 静态检查 —— 全绿
2. 真实 GitHub PR 上的同树 A/B同一棵树出两个 bundle ——
修复所依赖的那条真实多行评论 —— GitHub 确实返回了
P1 就是修复在真实评论、真实 可达性是实测的，不是推测。 最近 8 个 PR 里，455 条 Qwen
跨度：中位数 3 行，p90 10 行，最大 141 行（这一条， 3. 缺口 A —— 描述主打的那个方向在生产环境走不到描述说 bug 是「既有评论位于新起草的多行 finding 内部」时报
我的探针 P3 之所以能翻转，只是因为我手工往 findings 文件里写了 3 个新测试里有 2 个（ 4. 缺口 B —— overlap 触发了，但 Step 7 的丢弃规则找不到目标
改成区间匹配后两者不再相等。P1 里报告回传的是 posting.md 明确要求这个决策点必须保持确定性（"this has been improvised as an interactive question, which stalls a headless run forever"）。「阻塞但没有可丢弃项」恰好就是最容易诱发即兴发挥的状态。 5. 缺口 C —— 一个新的误报类别，来自 posting.md 自己的指示posting.md 要求 orchestrator 把 body-only Critical 也写进 findings 文件，用占位行号：
P5 就照着做了 —— 6. 缺口 D —— 生产环境唯一会跑到的那半边没有测试在
M4 是关键。 把评论侧读取 建议改动可以安全合并，我没有发现回归。但就目前状态而言，它只拓宽了检测器，没有同步消费方契约，所以它宣称的效果 —— 重复评论不再被发出 —— 实际上并没有发生。三个小的后续可以把它闭合，且应该放在本 PR 里而不是新开一个：
以上任何一条都欢迎反驳 —— 全部结论都可以从 |
3 participants
What this PR does
Preserves the start of multi-line review findings and classifies existing inline comments by same-file closed-interval intersection instead of comparing only their ending lines. Exact single-line behavior remains unchanged, and carried-finding re-post detection remains tied to the exact original anchor.
Why it's needed
/review presubmitcould reportnoConflictwhen an existing comment sat inside a newly drafted multi-line finding. That allowed a duplicate finding to pass the deterministic overlap gate and forced reviewers to detect it manually.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/commands/review/presubmit.test.ts.overlap, while disjoint ranges remainnoConflict.Evidence (Before & After)
Before: the focused regression produced two failures because a comment at line 15 did not match a new finding spanning lines 12–18, and partially intersecting ranges were also reported as
noConflict.After: the complete presubmit test file passes 109/109, including point-inside-range, partial intersection, disjoint control, parsing, exact-line, and carried-id/re-post coverage. An independent test-engineer run passed the 19 parsing/range-focused tests and
git diff --check.Tested on
Environment (optional)
Windows 11, Node.js 22.19.0, npm 10.x, Vitest 3.2.7. Repository-wide build and typecheck passed; focused Prettier and ESLint checks passed.
Risk & Scope
Linked Issues
Partially addresses #9219.
中文说明
本 PR 做了什么
保留多行 review finding 的起始行，并将既有行内评论的冲突判定从“仅比较结束行”改为“同一文件内闭区间相交”。单行行为保持不变，carried finding 的 re-post 识别仍严格绑定原始精确锚点。
为什么需要它
当既有评论位于新起草的多行 finding 范围内部时，
/review presubmit可能错误报告noConflict。这会让重复 finding 穿过确定性 overlap gate，只能依赖 reviewer 手工发现。Reviewer Test Plan
如何验证
cd packages/cli && npx vitest run src/commands/review/presubmit.test.ts。overlap，同时不相交区间仍归类为noConflict。修改前后证据
修改前：定向回归产生两个失败；位于第 15 行的评论无法匹配第 12–18 行的新 finding，部分相交的区间也被报告为
noConflict。修改后：完整 presubmit 测试文件 109/109 通过，覆盖范围内点、部分相交、不相交对照、解析、精确单行以及 carried-id/re-post。独立 test-engineer 运行的 19 个解析/范围相关测试与
git diff --check也通过。测试平台
环境
Windows 11、Node.js 22.19.0、npm 10.x、Vitest 3.2.7。仓库级 build 与 typecheck 通过；定向 Prettier 与 ESLint 检查通过。
风险与范围
关联 Issue
部分解决 #9219，不自动关闭该跟踪 Issue。