diff --git a/src/__tests__/orchestrate.test.ts b/src/__tests__/orchestrate.test.ts index 5b717df..928ed50 100644 --- a/src/__tests__/orchestrate.test.ts +++ b/src/__tests__/orchestrate.test.ts @@ -2098,6 +2098,57 @@ describe("orchestrate", () => { ]) }) + it("dedups via content tier when title matches but category differs", async () => { + const findings = fixtureReviewResponse.findings + const targetFinding = findings[0] + if (!targetFinding) { + throw new Error("expected at least one fixture finding") + } + const shiftedAnchor = computeAnchorKey({ + ...targetFinding, + category: "subtle_bugs", + line: targetFinding.line + 10, + }) + + const stubs = makeOrchestrateDeps({ + githubClient: { + fetchBotReviewComments: async () => [ + existingComment( + `**${targetFinding.title}**\nHigh severity · subtle_bugs · high confidence\n\nSome description.\n\n`, + ), + ], + }, + }) + const logger = createTestLogger() + + const result = await orchestrate(stubs.deps, logger) + + expect(result.findingsCount).toBe(findings.length - 1) + expect(logger.messages).toContainEqual({ + level: "info", + message: "content-tier dedup suppressed finding", + data: { + file: targetFinding.file, + line: targetFinding.line, + category: targetFinding.category, + title: targetFinding.title, + }, + }) + expect(logger.messages).toContainEqual({ + level: "info", + message: "cross-run dedup against prior bot comments", + data: { + statusCommentFound: false, + existingAnchorCount: 1, + priorBotCommentCount: 1, + findingsAfterFilter: findings.length, + findingsSurvivedDedup: findings.length - 1, + droppedByPositional: 0, + droppedByContent: 1, + }, + }) + }) + it("legacy title-hash anchors don't dedup", async () => { const stubs = makeOrchestrateDeps({ githubClient: { diff --git a/src/orchestrate.ts b/src/orchestrate.ts index fb171a9..e93ae61 100644 --- a/src/orchestrate.ts +++ b/src/orchestrate.ts @@ -23,7 +23,7 @@ import { buildStatusComment, coalesceAnchors, extractAnchors, - isDuplicateFinding, + classifyDuplicate, mapFindingsToReview, renderStandaloneFinding, REVIEW_MARKER, @@ -668,9 +668,24 @@ const runReviewPipeline = async ( // inline comments (live positions) and its beyond-diff issue comments // (anchor lines). Runs before the cap so duplicates don't consume slots. const existingAnchors = [...inlineState.anchors, ...issueState.anchors] - const newFindings = realFindings.filter( - (finding) => !isDuplicateFinding(finding, existingAnchors), - ) + const newFindings: Finding[] = [] + const dedupCounts = { positional: 0, content: 0 } + for (const finding of realFindings) { + const tier = classifyDuplicate(finding, existingAnchors) + if (tier) { + dedupCounts[tier]++ + if (tier === "content") { + logger.info("content-tier dedup suppressed finding", { + file: finding.file, + line: finding.line, + category: finding.category, + title: finding.title, + }) + } + } else { + newFindings.push(finding) + } + } logger.info("cross-run dedup against prior bot comments", { statusCommentFound: issueState.statusCommentExists, @@ -678,6 +693,8 @@ const runReviewPipeline = async ( priorBotCommentCount: priorBotComments.length, findingsAfterFilter: realFindings.length, findingsSurvivedDedup: newFindings.length, + droppedByPositional: dedupCounts.positional, + droppedByContent: dedupCounts.content, }) const { diff --git a/src/review/__tests__/comment-mapping.test.ts b/src/review/__tests__/comment-mapping.test.ts index 480939c..5527797 100644 --- a/src/review/__tests__/comment-mapping.test.ts +++ b/src/review/__tests__/comment-mapping.test.ts @@ -1009,7 +1009,7 @@ describe("coalesceAnchors", () => { expect(coalesceAnchors(anchors)).toEqual(anchors) }) - it("coalesces content-similar anchors across different categories", () => { + it("keeps content-similar anchors across different categories as distinct findings", () => { const anchors = [ { file: "src/a.ts", @@ -1025,14 +1025,7 @@ describe("coalesceAnchors", () => { }, ] - expect(coalesceAnchors(anchors)).toEqual([ - { - file: "src/a.ts", - category: "correctness", - line: 50, - title: "Missing null check on user.email", - }, - ]) + expect(coalesceAnchors(anchors)).toEqual(anchors) }) it("returns an empty array for no anchors", () => { diff --git a/src/review/__tests__/title-similarity.test.ts b/src/review/__tests__/title-similarity.test.ts index ccaf920..34db01e 100644 --- a/src/review/__tests__/title-similarity.test.ts +++ b/src/review/__tests__/title-similarity.test.ts @@ -90,4 +90,24 @@ describe("titleSimilarity", () => { }), ).toBe(0.5) }) + + it("deduplicates left tokens so duplicates do not inflate the ratio", () => { + // Without dedup: filter counts "check" twice + "email" → 3, union Set → 2, ratio 1.5 + // With dedup: both sides are {check, email} → ratio 1.0 + expect( + titleSimilarity({ + leftTokens: ["check", "check", "email"], + rightTokens: ["check", "email"], + }), + ).toBe(1) + }) + + it("deduplicates right tokens so duplicates do not inflate the ratio", () => { + expect( + titleSimilarity({ + leftTokens: ["alpha"], + rightTokens: ["alpha", "alpha", "beta"], + }), + ).toBe(0.5) + }) }) diff --git a/src/review/comment-mapping.ts b/src/review/comment-mapping.ts index 33894a1..f64fa60 100644 --- a/src/review/comment-mapping.ts +++ b/src/review/comment-mapping.ts @@ -150,24 +150,37 @@ const isContentDuplicate = ({ ) } -export const isDuplicateFinding = ( +export type DuplicateTier = "positional" | "content" + +/** Returns which dedup tier matched, or null if the finding is new. */ +export const classifyDuplicate = ( finding: AnchorEntry, anchors: AnchorEntry[], -): boolean => { - return anchors.some((anchor) => { - return ( - isPositionalDuplicate({ finding, anchor }) || - isContentDuplicate({ finding, anchor }) - ) - }) +): DuplicateTier | null => { + for (const anchor of anchors) { + if (isPositionalDuplicate({ finding, anchor })) return "positional" + } + for (const anchor of anchors) { + if (isContentDuplicate({ finding, anchor })) return "content" + } + return null } -/** Collapses anchors the dedup rules would treat as one finding. A - * fail-open fetch can repost an already-anchored finding, leaving two - * anchors for it — the status comment counts findings, not anchors. */ +export const isDuplicateFinding = ( + finding: AnchorEntry, + anchors: AnchorEntry[], +): boolean => classifyDuplicate(finding, anchors) !== null + +/** Collapses positionally overlapping anchors for the tracked-findings + * count. Uses positional dedup only — content similarity must not + * collapse genuinely distinct prior findings that happen to share + * title vocabulary. */ export const coalesceAnchors = (anchors: AnchorEntry[]): AnchorEntry[] => { return anchors.reduce((kept, anchor) => { - if (isDuplicateFinding(anchor, kept)) return kept + const positionalMatch = kept.some((existing) => { + return isPositionalDuplicate({ finding: anchor, anchor: existing }) + }) + if (positionalMatch) return kept return [...kept, anchor] }, []) } diff --git a/src/review/title-similarity.ts b/src/review/title-similarity.ts index 8913ed1..54a3be5 100644 --- a/src/review/title-similarity.ts +++ b/src/review/title-similarity.ts @@ -55,7 +55,8 @@ export const normalizeTitle = (title: string): string[] => { return [...new Set(tokens)].toSorted() } -/** Jaccard similarity: |A ∩ B| / |A ∪ B|. 0 = disjoint, 1 = identical. */ +/** Jaccard similarity: |A ∩ B| / |A ∪ B|. 0 = disjoint, 1 = identical. + * Deduplicates internally so the result stays in [0, 1] for arbitrary input. */ export const titleSimilarity = ({ leftTokens, rightTokens, @@ -63,9 +64,12 @@ export const titleSimilarity = ({ leftTokens: string[] rightTokens: string[] }): number => { - if (leftTokens.length === 0 || rightTokens.length === 0) return 0 + const leftSet = new Set(leftTokens) const rightSet = new Set(rightTokens) - const intersection = leftTokens.filter((token) => rightSet.has(token)).length - const union = new Set([...leftTokens, ...rightTokens]).size + if (leftSet.size === 0 || rightSet.size === 0) return 0 + const intersection = [...leftSet].filter((token) => { + return rightSet.has(token) + }).length + const union = new Set([...leftSet, ...rightSet]).size return intersection / union }