diff --git a/packages/cli/src/commands/review/presubmit.test.ts b/packages/cli/src/commands/review/presubmit.test.ts index 328c3385024..7c6d5d6c034 100644 --- a/packages/cli/src/commands/review/presubmit.test.ts +++ b/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>, - 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`. diff --git a/packages/cli/src/commands/review/presubmit.ts b/packages/cli/src/commands/review/presubmit.ts index da606814759..705991bb6d3 100644 --- a/packages/cli/src/commands/review/presubmit.ts +++ b/packages/cli/src/commands/review/presubmit.ts @@ -50,6 +50,7 @@ import { interface FindingAnchor { path: string; line: number; + startLine?: number; /** * Ledger id (`R-`) — 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-` @@ -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