fix(cli): detect overlapping review comment ranges

packages/cli/src/commands/review/presubmit.test.ts

@@ -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);
@@ -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
@@ -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`.

packages/cli/src/commands/review/presubmit.ts

@@ -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
@@ -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 };
@@ -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
@@ -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 } : {}),
       ...(typeof e.id === 'string' ? { id: e.id } : {}),
     });
   }
@@ -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>`
@@ -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);
+    const commentRangeEnd = Math.max(commentStartLine, commentLine);
+    const overlapsNewFinding = newFindings.some((finding) => {
+      if (finding.path !== (c.path ?? '')) return false;
+      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 ?? '',
@@ -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