From 95f5ed1738ce0adb3b93c1a46cba967f54744671 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:22:18 +0000 Subject: [PATCH 1/2] fix(dedup): Group findings by semantic bug identity Honor model-selected duplicate groups without an exact-text veto, and compare findings across distant source and test locations. Keep instructions conservative for independent bugs and cover the repeated-output-gate regression. Co-Authored-By: Greg Pstrucha --- packages/warden/src/output/dedup.test.ts | 102 +++++++++++++++++++---- packages/warden/src/output/dedup.ts | 79 +++--------------- packages/warden/src/sdk/extract.test.ts | 41 +++++++-- packages/warden/src/sdk/extract.ts | 30 ++----- 4 files changed, 140 insertions(+), 112 deletions(-) diff --git a/packages/warden/src/output/dedup.test.ts b/packages/warden/src/output/dedup.test.ts index 430e1836..015f8ed4 100644 --- a/packages/warden/src/output/dedup.test.ts +++ b/packages/warden/src/output/dedup.test.ts @@ -19,6 +19,7 @@ import { } from './dedup.js'; import type { Finding } from '../types/index.js'; import type { ExistingComment } from './dedup.js'; +import * as runtimes from '../sdk/runtimes/index.js'; describe('generateContentHash', () => { it('generates consistent 8-char hex hash', () => { @@ -322,6 +323,10 @@ Use \`Number.isFinite\` before saving [the value](https://example.com).`, }); describe('deduplicateFindings', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + const baseFinding: Finding = { id: 'f1', severity: 'high', @@ -466,6 +471,39 @@ describe('deduplicateFindings', () => { expect(result.duplicateActions[1]!.type).toBe('react_external'); }); + it('matches a paraphrase anchored in a test to an existing implementation comment', async () => { + const existing: ExistingComment = { + id: 1, path: 'src/typesafe.py', line: 294, + title: 'Output answers bypass the collection gate', + description: 'Both system_one wrappers record answers with gen_ai.outputs disabled.', + contentHash: 'existing-hash', isWarden: true, findingId: 'OUT-GATE', + }; + const duplicate: Finding = { + id: 'test-output', severity: 'high', + title: 'Evaluation answers ignore the output opt-out', + description: 'Gate output messages in the sync and async wrappers on collection policy.', + location: { path: 'tests/test_typesafe.py', startLine: 639 }, + }; + const runAuxiliary = vi.fn().mockResolvedValue({ + success: true, data: [{ findingIndex: 1, existingIndex: 1 }], + }); + vi.spyOn(runtimes, 'getRuntime').mockReturnValue({ + name: 'pi', runAuxiliary, + } as unknown as ReturnType); + + const result = await deduplicateFindings([duplicate, baseFinding], [existing], { runtime: 'pi' }); + + expect(result.newFindings).toEqual([baseFinding]); + expect(result.duplicateActions).toEqual([expect.objectContaining({ + type: 'update_warden', matchType: 'semantic', originalFindingId: duplicate.id, + existingComment: existing, + finding: { ...duplicate, id: 'OUT-GATE', reportedId: 'OUT-GATE' }, + })]); + expect(runAuxiliary).toHaveBeenCalledWith(expect.objectContaining({ + prompt: expect.stringContaining('even if wording, evidence, or the reported file and line differ'), + })); + }); + it('works without API key (hash-only mode)', async () => { const findings = [baseFinding]; const existingComments: ExistingComment[] = []; @@ -803,7 +841,7 @@ describe('consolidateBatchFindings', () => { expect(result.removedCount).toBe(0); }); - it('does not group findings more than 5 lines apart', async () => { + it('keeps findings more than 5 lines apart in hashOnly mode', async () => { const finding1: Finding = { id: 'f1', severity: 'high', @@ -826,7 +864,7 @@ describe('consolidateBatchFindings', () => { expect(result.removedCount).toBe(0); }); - it('does not group findings in different files', async () => { + it('keeps findings in different files in hashOnly mode', async () => { const finding1: Finding = { id: 'f1', severity: 'high', @@ -848,32 +886,68 @@ describe('consolidateBatchFindings', () => { expect(result.removedCount).toBe(0); }); - it('skips LLM when no proximity clusters exist', async () => { + it('merges semantic duplicates across distant lines and files but retains unrelated findings', async () => { const finding1: Finding = { id: 'f1', severity: 'high', - title: 'Bug A', - description: 'Description A', + title: 'Outputs bypass the privacy gate', + description: 'Both wrappers serialize answers even when output collection is disabled.', location: { path: 'src/file.ts', startLine: 10 }, }; const finding2: Finding = { id: 'f2', severity: 'medium', - title: 'Bug B', - description: 'Description B', + title: 'Output collection opt-out is ignored', + description: 'Check the output setting before writing answers in both wrappers.', location: { path: 'src/file.ts', startLine: 100 }, }; - // Even with an API key, no LLM call should be made since findings are far apart - const result = await consolidateBatchFindings([finding1, finding2], { apiKey: 'test-key' }); - expect(result.findings).toHaveLength(2); - expect(result.removedCount).toBe(0); - expect(result.usage).toBeUndefined(); + const finding3: Finding = { + ...finding2, id: 'f3', title: 'Evaluation answers recorded without consent', + location: { path: 'tests/file.test.ts', startLine: 200 }, + }; + const unrelated: Finding = { + ...finding1, id: 'f4', title: 'Score probabilities and legend can misalign', + description: 'Independent dictionary iteration can pair probabilities with the wrong labels.', + }; + const runAuxiliary = vi.fn().mockResolvedValue({ + success: true, data: [[1, 2, 3]], + usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, + }); + vi.spyOn(runtimes, 'getRuntime').mockReturnValue({ + name: 'pi', runAuxiliary, + } as unknown as ReturnType); + + const result = await consolidateBatchFindings([finding1, finding2, finding3, unrelated], { runtime: 'pi' }); + expect(result.findings).toEqual([ + { ...finding1, additionalLocations: [finding2.location, finding3.location] }, + unrelated, + ]); + expect(result.removedCount).toBe(2); + expect(result.removedFindings).toEqual([finding2, finding3]); + expect(runAuxiliary).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ + prompt: expect.stringContaining('tests/file.test.ts:200'), + })); + }); + + it('keeps findings when semantic consolidation fails', async () => { + const findings: Finding[] = [ + { id: 'f1', severity: 'high', title: 'Output gate missing', description: 'Outputs recorded without consent' }, + { id: 'f2', severity: 'medium', title: 'Output opt-out ignored', description: 'Gate output recording' }, + ]; + vi.spyOn(runtimes, 'getRuntime').mockReturnValue({ + name: 'pi', runAuxiliary: vi.fn().mockResolvedValue({ success: false, error: 'provider_error' }), + } as unknown as ReturnType); + + const result = await consolidateBatchFindings(findings, { runtime: 'pi' }); + + expect(result.findings).toEqual(findings); + expect(result.removedFindings).toEqual([]); + expect(console.warn).toHaveBeenCalledWith('LLM batch consolidation failed, keeping all findings: provider_error'); }); - it('groups findings within 5 lines of each other for proximity check', async () => { - // This tests the proximity grouping logic (without LLM since no API key) + it('keeps nearby findings when runtime authentication is unavailable', async () => { const finding1: Finding = { id: 'f1', severity: 'high', diff --git a/packages/warden/src/output/dedup.ts b/packages/warden/src/output/dedup.ts index cfe351a8..670a3c7b 100644 --- a/packages/warden/src/output/dedup.ts +++ b/packages/warden/src/output/dedup.ts @@ -521,8 +521,9 @@ ${findingsList} `, ` Return a JSON array of objects identifying which findings are DUPLICATES of which existing comments. -Only mark as duplicate if they describe the SAME issue at the SAME location (within a few lines). -Different issues at the same location are NOT duplicates. +Mark as duplicate when the finding describes the SAME underlying bug as an existing comment, even if wording, evidence, or the reported file and line differ. +A report anchored in a caller or test can duplicate one anchored in the implementation. Compare the actual defect and affected behavior, not just the location or issue category. +Independent bugs requiring separate fixes are NOT duplicates, even at the same location. Keep uncertain matches and findings with additional independent bugs separate. `, buildJsonOutputSection(`[{"findingIndex": 1, "existingIndex": 2}] where findingIndex is the 1-based index of the new finding and existingIndex is the 1-based index of the matching existing comment. @@ -694,8 +695,6 @@ export function findingToExistingComment(finding: Finding, skill?: string): Exis // Intra-batch consolidation // ----------------------------------------------------------------------------- -const PROXIMITY_THRESHOLD = 5; - /** * Result from consolidating findings within a single batch. */ @@ -711,62 +710,12 @@ const ConsolidationGroupsSchema = z.array( z.array(z.number().int()) ); -/** - * Group findings by file path, then identify clusters where findings are within - * PROXIMITY_THRESHOLD lines of each other. Returns only clusters with 2+ findings. - */ -function findProximityClusters(findings: Finding[]): Finding[][] { - // Group by file path - const byPath = new Map(); - for (const f of findings) { - const path = f.location?.path ?? ''; - const existing = byPath.get(path); - if (existing) { - existing.push(f); - } else { - byPath.set(path, [f]); - } - } - - const clusters: Finding[][] = []; - - for (const group of byPath.values()) { - if (group.length < 2) continue; - - // Sort by line number - const sorted = [...group].sort((a, b) => findingLine(a) - findingLine(b)); - - // Single-linkage clustering: consecutive findings within PROXIMITY_THRESHOLD - // lines of each other are grouped together. - const first = sorted[0]; - if (!first) continue; - let current: Finding[] = [first]; - - for (let i = 1; i < sorted.length; i++) { - const prev = sorted[i - 1]; - const curr = sorted[i]; - if (!prev || !curr) continue; - - if (findingLine(curr) - findingLine(prev) <= PROXIMITY_THRESHOLD) { - current.push(curr); - } else { - if (current.length >= 2) clusters.push(current); - current = [curr]; - } - } - if (current.length >= 2) clusters.push(current); - } - - return clusters; -} - /** * Consolidate findings within a single batch to remove duplicates that describe - * the same root cause. Three-phase approach: + * the same root cause, regardless of wording or location. * * 1. Hash dedup: remove exact duplicates (same path:line:contentHash) - * 2. Proximity grouping: identify clusters of findings within 5 lines of each other - * 3. LLM consolidation: ask the auxiliary runtime to group findings by root cause (only when proximity matches exist) + * 2. LLM consolidation: group remaining findings by semantic issue identity * * For each group, keeps the highest-severity finding. */ @@ -803,19 +752,14 @@ export async function consolidateBatchFindings( console.log(`Consolidate: ${hashRemovedCount} exact duplicate findings removed within batch`); } - // Phase 2: Proximity grouping - const clusters = findProximityClusters(hashDeduped); - - // If no proximity clusters, hash-only mode, or no runtime auth, return hash-deduped results. - if (clusters.length === 0 || options.hashOnly || !canUseRuntimeAuth(options)) { + if (hashDeduped.length <= 1 || options.hashOnly || !canUseRuntimeAuth(options)) { return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings }; } - // Phase 3: LLM consolidation for proximity clusters - // Only send clustered findings to the LLM (deduplicated across clusters) - const clusteredList = [...new Set(clusters.flat())]; - const findingsList = formatIndexedFindingsForPrompt(clusteredList, { + // Compare the full batch: the same defect may be anchored in distant hunks or tests. + const findingsList = formatIndexedFindingsForPrompt(hashDeduped, { includeSeverity: true, + includeVerification: true, }); const prompt = joinPromptSections([ @@ -827,7 +771,8 @@ ${findingsList} `, ` Return a JSON array of arrays, where each inner array contains the 1-based indices of findings that describe the same root cause. -Only group findings that are truly about the same underlying issue. Findings about different issues should NOT be grouped even if they're nearby. +Group by semantic meaning, not exact wording of titles, descriptions, or evidence. Reports of the same defect may point to different lines, files, callers, or tests. +Sharing an issue category is not enough: independent bugs requiring separate fixes are separate findings, even if nearby. Keep uncertain matches and findings with additional independent bugs separate. Singletons (findings with no duplicates) should not appear in any group. `, buildJsonOutputSection('Return the JSON array. Return [] if no findings share a root cause.'), @@ -850,7 +795,7 @@ Singletons (findings with no duplicates) should not appear in any group. return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings, usage: result.usage }; } - const { absorbed, replacements } = applyMergeGroups(clusteredList, result.data); + const { absorbed, replacements } = applyMergeGroups(hashDeduped, result.data); if (absorbed.size === 0) { return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings, usage: result.usage }; diff --git a/packages/warden/src/sdk/extract.test.ts b/packages/warden/src/sdk/extract.test.ts index 8c88fe8a..64986768 100644 --- a/packages/warden/src/sdk/extract.test.ts +++ b/packages/warden/src/sdk/extract.test.ts @@ -250,7 +250,7 @@ describe('mergeCrossLocationFindings', () => { })); }); - it('preserves independent authorization bugs even when synthesis groups them together', async () => { + it('preserves independent authorization bugs left ungrouped by synthesis', async () => { // Sanitized from the Sentry corpus: one broad authorization category, three fixes. const findings = [ makeFinding({ @@ -276,7 +276,7 @@ describe('mergeCrossLocationFindings', () => { }), ]; mockCallHaiku.mockResolvedValue({ - success: true, data: [[1, 2, 3]], + success: true, data: [], usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, }); const onFindingProcessing = vi.fn(); @@ -288,22 +288,47 @@ describe('mergeCrossLocationFindings', () => { expect(onFindingProcessing).not.toHaveBeenCalled(); }); - it('keeps distinct evidence while merging identical claims within an overbroad group', async () => { - const first = makeFinding({ id: 'first', verification: 'The create handler omits the tenant guard.', location: { path: 'src/a.ts', startLine: 1 } }); - const duplicate = { ...first, id: 'duplicate', location: { path: 'src/b.ts', startLine: 2 } }; - const distinct = { ...first, id: 'distinct', verification: 'The delete handler checks the wrong tenant.', location: { path: 'src/a.ts', startLine: 5 } }; + it('merges paraphrases of the same output privacy bug across source and tests', async () => { + // Based on sentry-python#7720: different hunks rediscovered the same missing gate. + const first = makeFinding({ + id: 'output-gate', severity: 'high', + title: 'GenAI output answers bypass the output-collection gate', + description: 'Both system_one wrappers record answers even when output collection is disabled.', + verification: 'The sync and async wrappers serialize response.answers without checking gen_ai.outputs.', + location: { path: 'src/typesafe.py', startLine: 294 }, + }); + const duplicate = makeFinding({ + id: 'output-opt-out', + title: 'TypeSafe outputs ignore GenAI output-collection settings', + description: 'Gate GEN_AI_OUTPUT_MESSAGES in both wrappers on the output setting or legacy PII policy.', + verification: 'The test asserts recorded evaluation answers without opting in to output collection.', + location: { path: 'tests/test_typesafe.py', startLine: 639 }, + }); + const distinct = makeFinding({ + id: 'score-order', + title: 'Score probabilities and legend can misalign', + description: 'Independent dictionary iteration can pair probabilities with the wrong labels.', + location: { path: 'src/typesafe.py', startLine: 212 }, + }); mockCallHaiku.mockResolvedValue({ - success: true, data: [[1, 2, 3]], + success: true, data: [[1, 2]], usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, }); + const onFindingProcessing = vi.fn(); - const result = await mergeCrossLocationFindings([first, duplicate, distinct], { apiKey: 'test-key', repoPath: tempDir }); + const result = await mergeCrossLocationFindings([first, duplicate, distinct], { + apiKey: 'test-key', repoPath: tempDir, onFindingProcessing, + }); expect(result.findings).toEqual([ { ...first, additionalLocations: [duplicate.location] }, distinct, ]); expect(result.mergedCount).toBe(1); + expect(onFindingProcessing).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ + stage: 'merge', action: 'merged', finding: duplicate, + replacement: result.findings[0], + })); }); it('still attributes the absorbed finding to its winner when both share the exact same location', async () => { diff --git a/packages/warden/src/sdk/extract.ts b/packages/warden/src/sdk/extract.ts index 8e28374b..571887b1 100644 --- a/packages/warden/src/sdk/extract.ts +++ b/packages/warden/src/sdk/extract.ts @@ -405,27 +405,12 @@ interface ApplyGroupsResult { absorbedToWinner: Map; } -function splitMergeGroupByClaim(findings: Finding[], indices: number[]): number[][] { - const claims = new Map(); - for (const index of indices) { - const finding = findings[index - 1]; - if (!finding || !finding.title.trim() || !finding.description.trim()) continue; - // Location-only merging cannot preserve a different title, claim, or - // evidence trace. Keep those reports independent even if synthesis groups them. - const key = JSON.stringify([finding.title.trim(), finding.description.trim(), finding.verification?.trim() ?? '']); - const group = claims.get(key) ?? []; - group.push(index); - claims.set(key, group); - } - return [...claims.values()]; -} - /** * Apply LLM-returned merge groups to a list of findings. * * For each group, the highest-priority finding becomes the winner, and all - * other findings' locations are folded into its additionalLocations only when - * their titles, descriptions, and evidence agree. Distinct claims survive. + * other findings' locations are folded into its additionalLocations. The model + * decides semantic equivalence; paraphrased titles or evidence do not veto a group. * Handles overlapping groups by substituting prior replacements and tracking * absorbed findings by their original identity. * @@ -440,7 +425,7 @@ export function applyMergeGroups( const replacements = new Map(); const absorbedToWinner = new Map(); - for (const group of groups.flatMap((indices) => splitMergeGroupByClaim(indexedFindings, indices))) { + for (const group of groups) { const uniqueIndices = [...new Set(group)]; if (uniqueIndices.length < 2) continue; @@ -538,10 +523,8 @@ function readSnippet(repoPath: string, filePath: string, startLine: number, cont * Merge findings that describe the same issue across different code locations. * * Uses the configured auxiliary runtime to identify groups of findings about - * the same root cause at different locations. Only findings with identical - * trimmed titles, descriptions, and verification text are merged: the + * the same root cause at different locations, regardless of wording. The * highest-priority finding keeps the other locations in `additionalLocations`. - * Differing claims or evidence remain independent. * * Skips entirely (no LLM call) when: * - Fewer than 2 findings have locations @@ -574,8 +557,9 @@ export async function mergeCrossLocationFindings( const prompt = joinPromptSections([ ` -Identify which of these code review findings describe the SAME underlying issue appearing at different locations. Group them by shared root cause. -Sharing a category such as missing authorization is not enough: independent endpoints requiring separate fixes are separate findings. Keep uncertain matches separate. +Identify which of these code review findings describe the SAME underlying bug. Group by semantic meaning, not exact wording of titles, descriptions, or evidence. +Reports of the same defect may point to different lines, files, callers, or tests. For example, repeated reports that both sync and async wrappers omit the same output-collection guard are one finding, even when anchored in different tests. +Sharing a category such as missing authorization is not enough: independent endpoints requiring separate fixes are separate findings. Keep uncertain matches separate, and keep a broader finding separate if it contains an additional independent bug. `, ` ${findingDescriptions} From 30ef763eea2e56e58c96788409f7a150d416ad90 Mon Sep 17 00:00:00 2001 From: "sentry-junior[bot]" <264270552+sentry-junior[bot]@users.noreply.github.com> Date: Mon, 28 Sep 2026 18:46:37 +0000 Subject: [PATCH 2/2] ref(dedup): Trim redundant regression coverage Keep paraphrase and cross-file consolidation checks, reuse the existing merge test, and exclude locationless findings from inline consolidation. Co-Authored-By: Greg Pstrucha --- packages/warden/src/output/dedup.test.ts | 60 +--------------- packages/warden/src/output/dedup.ts | 10 +-- packages/warden/src/sdk/extract.test.ts | 91 ++---------------------- 3 files changed, 16 insertions(+), 145 deletions(-) diff --git a/packages/warden/src/output/dedup.test.ts b/packages/warden/src/output/dedup.test.ts index 015f8ed4..63fbb793 100644 --- a/packages/warden/src/output/dedup.test.ts +++ b/packages/warden/src/output/dedup.test.ts @@ -323,10 +323,6 @@ Use \`Number.isFinite\` before saving [the value](https://example.com).`, }); describe('deduplicateFindings', () => { - afterEach(() => { - vi.restoreAllMocks(); - }); - const baseFinding: Finding = { id: 'f1', severity: 'high', @@ -471,39 +467,6 @@ describe('deduplicateFindings', () => { expect(result.duplicateActions[1]!.type).toBe('react_external'); }); - it('matches a paraphrase anchored in a test to an existing implementation comment', async () => { - const existing: ExistingComment = { - id: 1, path: 'src/typesafe.py', line: 294, - title: 'Output answers bypass the collection gate', - description: 'Both system_one wrappers record answers with gen_ai.outputs disabled.', - contentHash: 'existing-hash', isWarden: true, findingId: 'OUT-GATE', - }; - const duplicate: Finding = { - id: 'test-output', severity: 'high', - title: 'Evaluation answers ignore the output opt-out', - description: 'Gate output messages in the sync and async wrappers on collection policy.', - location: { path: 'tests/test_typesafe.py', startLine: 639 }, - }; - const runAuxiliary = vi.fn().mockResolvedValue({ - success: true, data: [{ findingIndex: 1, existingIndex: 1 }], - }); - vi.spyOn(runtimes, 'getRuntime').mockReturnValue({ - name: 'pi', runAuxiliary, - } as unknown as ReturnType); - - const result = await deduplicateFindings([duplicate, baseFinding], [existing], { runtime: 'pi' }); - - expect(result.newFindings).toEqual([baseFinding]); - expect(result.duplicateActions).toEqual([expect.objectContaining({ - type: 'update_warden', matchType: 'semantic', originalFindingId: duplicate.id, - existingComment: existing, - finding: { ...duplicate, id: 'OUT-GATE', reportedId: 'OUT-GATE' }, - })]); - expect(runAuxiliary).toHaveBeenCalledWith(expect.objectContaining({ - prompt: expect.stringContaining('even if wording, evidence, or the reported file and line differ'), - })); - }); - it('works without API key (hash-only mode)', async () => { const findings = [baseFinding]; const existingComments: ExistingComment[] = []; @@ -911,6 +874,7 @@ describe('consolidateBatchFindings', () => { ...finding1, id: 'f4', title: 'Score probabilities and legend can misalign', description: 'Independent dictionary iteration can pair probabilities with the wrong labels.', }; + const locationless: Finding = { ...finding1, id: 'f5', location: undefined }; const runAuxiliary = vi.fn().mockResolvedValue({ success: true, data: [[1, 2, 3]], usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, @@ -919,32 +883,14 @@ describe('consolidateBatchFindings', () => { name: 'pi', runAuxiliary, } as unknown as ReturnType); - const result = await consolidateBatchFindings([finding1, finding2, finding3, unrelated], { runtime: 'pi' }); + const result = await consolidateBatchFindings([locationless, finding1, finding2, finding3, unrelated], { runtime: 'pi' }); expect(result.findings).toEqual([ + locationless, { ...finding1, additionalLocations: [finding2.location, finding3.location] }, unrelated, ]); expect(result.removedCount).toBe(2); expect(result.removedFindings).toEqual([finding2, finding3]); - expect(runAuxiliary).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ - prompt: expect.stringContaining('tests/file.test.ts:200'), - })); - }); - - it('keeps findings when semantic consolidation fails', async () => { - const findings: Finding[] = [ - { id: 'f1', severity: 'high', title: 'Output gate missing', description: 'Outputs recorded without consent' }, - { id: 'f2', severity: 'medium', title: 'Output opt-out ignored', description: 'Gate output recording' }, - ]; - vi.spyOn(runtimes, 'getRuntime').mockReturnValue({ - name: 'pi', runAuxiliary: vi.fn().mockResolvedValue({ success: false, error: 'provider_error' }), - } as unknown as ReturnType); - - const result = await consolidateBatchFindings(findings, { runtime: 'pi' }); - - expect(result.findings).toEqual(findings); - expect(result.removedFindings).toEqual([]); - expect(console.warn).toHaveBeenCalledWith('LLM batch consolidation failed, keeping all findings: provider_error'); }); it('keeps nearby findings when runtime authentication is unavailable', async () => { diff --git a/packages/warden/src/output/dedup.ts b/packages/warden/src/output/dedup.ts index 670a3c7b..051bc1a1 100644 --- a/packages/warden/src/output/dedup.ts +++ b/packages/warden/src/output/dedup.ts @@ -752,12 +752,14 @@ export async function consolidateBatchFindings( console.log(`Consolidate: ${hashRemovedCount} exact duplicate findings removed within batch`); } - if (hashDeduped.length <= 1 || options.hashOnly || !canUseRuntimeAuth(options)) { + // A locationless winner would suppress an otherwise postable inline comment. + const withLocations = hashDeduped.filter((finding) => finding.location); + if (withLocations.length <= 1 || options.hashOnly || !canUseRuntimeAuth(options)) { return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings }; } - // Compare the full batch: the same defect may be anchored in distant hunks or tests. - const findingsList = formatIndexedFindingsForPrompt(hashDeduped, { + // Compare across files: the same defect may be anchored in distant hunks or tests. + const findingsList = formatIndexedFindingsForPrompt(withLocations, { includeSeverity: true, includeVerification: true, }); @@ -795,7 +797,7 @@ Singletons (findings with no duplicates) should not appear in any group. return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings, usage: result.usage }; } - const { absorbed, replacements } = applyMergeGroups(hashDeduped, result.data); + const { absorbed, replacements } = applyMergeGroups(withLocations, result.data); if (absorbed.size === 0) { return { findings: hashDeduped, removedCount: hashRemovedCount, removedFindings: hashRemovedFindings, usage: result.usage }; diff --git a/packages/warden/src/sdk/extract.test.ts b/packages/warden/src/sdk/extract.test.ts index 64986768..dd599b73 100644 --- a/packages/warden/src/sdk/extract.test.ts +++ b/packages/warden/src/sdk/extract.test.ts @@ -206,18 +206,22 @@ describe('mergeCrossLocationFindings', () => { } }); - it('merges two findings into one with additionalLocations', async () => { + it('merges paraphrased findings and preserves their locations', async () => { const findings = [ makeFinding({ id: 'f1', severity: 'high', - title: 'Missing null check', + title: 'Outputs bypass the collection gate', + description: 'Both wrappers record answers when output collection is disabled.', + verification: 'Neither output write checks gen_ai.outputs.', location: { path: 'src/a.ts', startLine: 3 }, }), makeFinding({ id: 'f2', severity: 'medium', - title: 'Missing null check', + title: 'Output opt-out is ignored', + description: 'Gate answer serialization in the sync and async wrappers.', + verification: 'The test records outputs without opting in.', location: { path: 'src/b.ts', startLine: 2 }, }), ]; @@ -250,87 +254,6 @@ describe('mergeCrossLocationFindings', () => { })); }); - it('preserves independent authorization bugs left ungrouped by synthesis', async () => { - // Sanitized from the Sentry corpus: one broad authorization category, three fixes. - const findings = [ - makeFinding({ - id: 'thresholds', - title: 'Empty project filter exposes release thresholds', - description: 'An empty project selection falls back to thresholds from every tenant.', - verification: 'The empty branch omits the project predicate.', - location: { path: 'src/thresholds.py', startLine: 40 }, - }), - makeFinding({ - id: 'plugins', - title: 'Plugin endpoint skips project access', - description: 'A caller-supplied project ID is loaded without checking membership.', - verification: 'The plugin handler never checks access to the loaded project.', - location: { path: 'src/plugins.py', startLine: 25 }, - }), - makeFinding({ - id: 'detectors', - title: 'Detector endpoint skips project access', - description: 'Detector lookup accepts project IDs outside the caller organization.', - verification: 'The detector query does not intersect the accessible projects.', - location: { path: 'src/detectors.py', startLine: 30 }, - }), - ]; - mockCallHaiku.mockResolvedValue({ - success: true, data: [], - usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, - }); - const onFindingProcessing = vi.fn(); - - const result = await mergeCrossLocationFindings(findings, { apiKey: 'test-key', repoPath: tempDir, onFindingProcessing }); - - expect(result.findings).toEqual(findings); - expect(result.mergedCount).toBe(0); - expect(onFindingProcessing).not.toHaveBeenCalled(); - }); - - it('merges paraphrases of the same output privacy bug across source and tests', async () => { - // Based on sentry-python#7720: different hunks rediscovered the same missing gate. - const first = makeFinding({ - id: 'output-gate', severity: 'high', - title: 'GenAI output answers bypass the output-collection gate', - description: 'Both system_one wrappers record answers even when output collection is disabled.', - verification: 'The sync and async wrappers serialize response.answers without checking gen_ai.outputs.', - location: { path: 'src/typesafe.py', startLine: 294 }, - }); - const duplicate = makeFinding({ - id: 'output-opt-out', - title: 'TypeSafe outputs ignore GenAI output-collection settings', - description: 'Gate GEN_AI_OUTPUT_MESSAGES in both wrappers on the output setting or legacy PII policy.', - verification: 'The test asserts recorded evaluation answers without opting in to output collection.', - location: { path: 'tests/test_typesafe.py', startLine: 639 }, - }); - const distinct = makeFinding({ - id: 'score-order', - title: 'Score probabilities and legend can misalign', - description: 'Independent dictionary iteration can pair probabilities with the wrong labels.', - location: { path: 'src/typesafe.py', startLine: 212 }, - }); - mockCallHaiku.mockResolvedValue({ - success: true, data: [[1, 2]], - usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, - }); - const onFindingProcessing = vi.fn(); - - const result = await mergeCrossLocationFindings([first, duplicate, distinct], { - apiKey: 'test-key', repoPath: tempDir, onFindingProcessing, - }); - - expect(result.findings).toEqual([ - { ...first, additionalLocations: [duplicate.location] }, - distinct, - ]); - expect(result.mergedCount).toBe(1); - expect(onFindingProcessing).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ - stage: 'merge', action: 'merged', finding: duplicate, - replacement: result.findings[0], - })); - }); - it('still attributes the absorbed finding to its winner when both share the exact same location', async () => { // Regression: mergeGroupLocations seeds `seen` with the winner's own // location, so a loser at that same location is filtered out of