diff --git a/src/action/main.ts b/src/action/main.ts index 58d59d7..8b3aaf5 100644 --- a/src/action/main.ts +++ b/src/action/main.ts @@ -23,11 +23,12 @@ import { matchTrigger, shouldFail, countFindingsAtOrAbove, countSeverity } from import { resolveSkillAsync } from '../skills/loader.js'; import { filterFindingsBySeverity, SeverityThresholdSchema } from '../types/index.js'; import { - fetchExistingWardenComments, + fetchExistingComments, deduplicateFindings, findingToExistingComment, + processDuplicateActions, } from '../output/dedup.js'; -import type { ExistingComment } from '../output/dedup.js'; +import type { ExistingComment, DeduplicateResult } from '../output/dedup.js'; import { buildAnalyzedScope, findStaleComments, resolveStaleComments } from '../output/stale.js'; import type { EventContext, SkillReport, SeverityThreshold, UsageStats } from '../types/index.js'; import type { RenderResult } from '../output/types.js'; @@ -643,13 +644,13 @@ async function run(): Promise { const results = await processInBatches(matchedTriggers, runSingleTrigger, concurrency); - // Fetch existing Warden comments for deduplication (only for PRs) + // Fetch existing comments for deduplication (only for PRs) // Keep original list separate for stale detection (modified list includes newly posted comments) let fetchedComments: ExistingComment[] = []; let existingComments: ExistingComment[] = []; if (context.pullRequest) { try { - fetchedComments = await fetchExistingWardenComments( + fetchedComments = await fetchExistingComments( octokit, context.repository.owner, context.repository.name, @@ -657,7 +658,11 @@ async function run(): Promise { ); existingComments = [...fetchedComments]; if (fetchedComments.length > 0) { - console.log(`Found ${fetchedComments.length} existing Warden comments for deduplication`); + const wardenCount = fetchedComments.filter((c) => c.isWarden).length; + const externalCount = fetchedComments.length - wardenCount; + console.log( + `Found ${fetchedComments.length} existing comments for deduplication (${wardenCount} Warden, ${externalCount} external)` + ); } } catch (error) { console.warn(`::warning::Failed to fetch existing comments for deduplication: ${error}`); @@ -681,13 +686,40 @@ async function run(): Promise { try { // Deduplicate findings against existing comments let findingsToPost = filteredFindings; + let dedupResult: DeduplicateResult | undefined; + if (existingComments.length > 0 && filteredFindings.length > 0) { - findingsToPost = await deduplicateFindings(filteredFindings, existingComments, { + dedupResult = await deduplicateFindings(filteredFindings, existingComments, { apiKey: inputs.anthropicApiKey, + currentSkill: result.report.skill, }); - const dedupedCount = filteredFindings.length - findingsToPost.length; - if (dedupedCount > 0) { - console.log(`Skipping ${dedupedCount} duplicate findings for ${result.triggerName}`); + findingsToPost = dedupResult.newFindings; + + if (dedupResult.duplicateActions.length > 0) { + console.log( + `Found ${dedupResult.duplicateActions.length} duplicate findings for ${result.triggerName}` + ); + } + } + + // Process duplicate actions (update Warden comments, add reactions) + if (dedupResult && dedupResult.duplicateActions.length > 0) { + const actionCounts = await processDuplicateActions( + octokit, + context.repository.owner, + context.repository.name, + dedupResult.duplicateActions, + result.report.skill + ); + + if (actionCounts.updated > 0) { + console.log(`Updated ${actionCounts.updated} existing Warden comments with skill attribution`); + } + if (actionCounts.reacted > 0) { + console.log(`Added reactions to ${actionCounts.reacted} existing external comments`); + } + if (actionCounts.failed > 0) { + console.warn(`::warning::Failed to process ${actionCounts.failed} duplicate actions`); } } @@ -715,7 +747,7 @@ async function run(): Promise { ? findingsToPost.slice(0, result.maxFindings) : findingsToPost; for (const finding of postedFindings) { - const comment = findingToExistingComment(finding); + const comment = findingToExistingComment(finding, result.report.skill); if (comment) { existingComments.push(comment); } @@ -748,12 +780,14 @@ async function run(): Promise { // Resolve stale Warden comments (comments that no longer have matching findings) // Use fetchedComments (not existingComments) to only check comments that have threadIds // Only resolve if ALL triggers succeeded - otherwise findings may be missing due to failures + // Filter to only Warden comments - we don't resolve external comments const allTriggersSucceeded = triggerErrors.length === 0; - if (context.pullRequest && fetchedComments.length > 0 && allTriggersSucceeded) { + const wardenComments = fetchedComments.filter((c) => c.isWarden); + if (context.pullRequest && wardenComments.length > 0 && allTriggersSucceeded) { try { const allFindings = reports.flatMap((r) => r.findings); const scope = buildAnalyzedScope(context.pullRequest.files); - const staleComments = findStaleComments(fetchedComments, allFindings, scope); + const staleComments = findStaleComments(wardenComments, allFindings, scope); if (staleComments.length > 0) { const resolvedCount = await resolveStaleComments(octokit, staleComments); @@ -764,7 +798,7 @@ async function run(): Promise { } catch (error) { console.warn(`::warning::Failed to resolve stale comments: ${error}`); } - } else if (!allTriggersSucceeded && fetchedComments.length > 0) { + } else if (!allTriggersSucceeded && wardenComments.length > 0) { console.log('Skipping stale comment resolution due to trigger failures'); } diff --git a/src/output/dedup.test.ts b/src/output/dedup.test.ts index ea4b28a..7c518a6 100644 --- a/src/output/dedup.test.ts +++ b/src/output/dedup.test.ts @@ -7,6 +7,8 @@ import { isWardenComment, deduplicateFindings, findingToExistingComment, + parseWardenSkills, + updateWardenCommentBody, } from './dedup.js'; import type { Finding } from '../types/index.js'; import type { ExistingComment } from './dedup.js'; @@ -140,8 +142,9 @@ describe('deduplicateFindings', () => { it('returns all findings when no existing comments', async () => { const findings = [baseFinding]; const result = await deduplicateFindings(findings, [], { hashOnly: true }); - expect(result).toHaveLength(1); - expect(result[0]).toBe(baseFinding); + expect(result.newFindings).toHaveLength(1); + expect(result.newFindings[0]).toBe(baseFinding); + expect(result.duplicateActions).toHaveLength(0); }); it('returns all findings when findings array is empty', async () => { @@ -157,10 +160,11 @@ describe('deduplicateFindings', () => { ]; const result = await deduplicateFindings([], existingComments, { hashOnly: true }); - expect(result).toHaveLength(0); + expect(result.newFindings).toHaveLength(0); + expect(result.duplicateActions).toHaveLength(0); }); - it('filters out exact hash matches', async () => { + it('filters out exact hash matches and creates duplicate action', async () => { const existingComments: ExistingComment[] = [ { id: 1, @@ -169,11 +173,15 @@ describe('deduplicateFindings', () => { title: 'SQL Injection', description: 'User input passed to query', contentHash: generateContentHash('SQL Injection', 'User input passed to query'), + isWarden: true, }, ]; const result = await deduplicateFindings([baseFinding], existingComments, { hashOnly: true }); - expect(result).toHaveLength(0); + expect(result.newFindings).toHaveLength(0); + expect(result.duplicateActions).toHaveLength(1); + expect(result.duplicateActions[0]!.type).toBe('update_warden'); + expect(result.duplicateActions[0]!.matchType).toBe('hash'); }); it('keeps findings with different content', async () => { @@ -198,8 +206,9 @@ describe('deduplicateFindings', () => { const result = await deduplicateFindings([differentFinding], existingComments, { hashOnly: true, }); - expect(result).toHaveLength(1); - expect(result[0]!.title).toBe('XSS Vulnerability'); + expect(result.newFindings).toHaveLength(1); + expect(result.newFindings[0]!.title).toBe('XSS Vulnerability'); + expect(result.duplicateActions).toHaveLength(0); }); it('filters multiple duplicates and keeps unique findings', async () => { @@ -235,6 +244,7 @@ describe('deduplicateFindings', () => { title: 'SQL Injection', description: 'User input passed to query', contentHash: generateContentHash('SQL Injection', 'User input passed to query'), + isWarden: true, }, { id: 2, @@ -243,14 +253,20 @@ describe('deduplicateFindings', () => { title: 'Code Style', description: 'Inconsistent indentation', contentHash: generateContentHash('Code Style', 'Inconsistent indentation'), + isWarden: false, }, ]; const result = await deduplicateFindings([finding1, finding2, finding3], existingComments, { hashOnly: true, }); - expect(result).toHaveLength(1); - expect(result[0]!.id).toBe('f2'); + expect(result.newFindings).toHaveLength(1); + expect(result.newFindings[0]!.id).toBe('f2'); + expect(result.duplicateActions).toHaveLength(2); + // First should be update_warden (isWarden: true) + expect(result.duplicateActions[0]!.type).toBe('update_warden'); + // Second should be react_external (isWarden: false) + expect(result.duplicateActions[1]!.type).toBe('react_external'); }); it('works without API key (hash-only mode)', async () => { @@ -258,7 +274,52 @@ describe('deduplicateFindings', () => { const existingComments: ExistingComment[] = []; const result = await deduplicateFindings(findings, existingComments, {}); - expect(result).toHaveLength(1); + expect(result.newFindings).toHaveLength(1); + }); +}); + +describe('parseWardenSkills', () => { + it('parses single skill', () => { + const body = `**:warning: Issue**\n\nDescription\n\n---\nwarden: security-review`; + expect(parseWardenSkills(body)).toEqual(['security-review']); + }); + + it('parses multiple skills', () => { + const body = `**:warning: Issue**\n\nDescription\n\n---\nwarden: security-review, code-quality, performance`; + expect(parseWardenSkills(body)).toEqual(['security-review', 'code-quality', 'performance']); + }); + + it('handles extra whitespace', () => { + const body = `warden: skill1 , skill2 `; + expect(parseWardenSkills(body)).toEqual(['skill1', 'skill2']); + }); + + it('returns empty array for non-Warden comment', () => { + const body = 'Regular comment without attribution'; + expect(parseWardenSkills(body)).toEqual([]); + }); +}); + +describe('updateWardenCommentBody', () => { + it('adds new skill to attribution', () => { + const body = `**:warning: Issue**\n\nDescription\n\n---\nwarden: skill1`; + const result = updateWardenCommentBody(body, 'skill2'); + expect(result).toContain('warden: skill1, skill2'); + }); + + it('returns null if skill already listed', () => { + const body = `warden: skill1, skill2`; + const result = updateWardenCommentBody(body, 'skill1'); + expect(result).toBeNull(); + }); + + it('preserves rest of comment body', () => { + const body = `**:warning: SQL Injection**\n\nUser input passed to query\n\n---\nwarden: security-review\n`; + const result = updateWardenCommentBody(body, 'code-quality'); + expect(result).toContain('**:warning: SQL Injection**'); + expect(result).toContain('User input passed to query'); + expect(result).toContain('warden: security-review, code-quality'); + expect(result).toContain(''); }); }); @@ -284,9 +345,29 @@ describe('findingToExistingComment', () => { title: 'SQL Injection', description: 'User input passed to query', contentHash: generateContentHash('SQL Injection', 'User input passed to query'), + isWarden: true, + skills: [], }); }); + it('includes skill when provided', () => { + const finding: Finding = { + id: 'f1', + severity: 'high', + title: 'SQL Injection', + description: 'User input passed to query', + location: { + path: 'src/db.ts', + startLine: 42, + }, + }; + + const comment = findingToExistingComment(finding, 'security-review'); + expect(comment).not.toBeNull(); + expect(comment!.isWarden).toBe(true); + expect(comment!.skills).toEqual(['security-review']); + }); + it('uses startLine when endLine is not set', () => { const finding: Finding = { id: 'f1', diff --git a/src/output/dedup.ts b/src/output/dedup.ts index 8b325cd..0d2f1c7 100644 --- a/src/output/dedup.ts +++ b/src/output/dedup.ts @@ -4,9 +4,6 @@ import Anthropic from '@anthropic-ai/sdk'; import { z } from 'zod'; import type { Finding } from '../types/index.js'; -/** Schema for validating LLM deduplication response */ -const DuplicateIndicesSchema = z.array(z.number().int()); - /** * Parsed marker data from a Warden comment. */ @@ -17,7 +14,7 @@ export interface WardenMarker { } /** - * Existing Warden comment from GitHub. + * Existing comment from GitHub (either Warden or external). */ export interface ExistingComment { id: number; @@ -30,6 +27,40 @@ export interface ExistingComment { threadId?: string; /** Whether the thread has been resolved (resolved comments are used for dedup but not stale detection) */ isResolved?: boolean; + /** Whether this is a Warden-generated comment */ + isWarden?: boolean; + /** Skills that have already detected this issue (for Warden comments) */ + skills?: string[]; + /** The raw comment body (needed for updating Warden comments) */ + body?: string; + /** GraphQL node ID for the comment (needed for adding reactions) */ + commentNodeId?: string; +} + +/** + * Type of action to take for a duplicate finding. + */ +export type DuplicateActionType = 'update_warden' | 'react_external'; + +/** + * Action to take for a duplicate finding. + */ +export interface DuplicateAction { + type: DuplicateActionType; + finding: Finding; + existingComment: ExistingComment; + /** Whether this was a hash match or semantic match */ + matchType: 'hash' | 'semantic'; +} + +/** + * Result of deduplication with actions for duplicates. + */ +export interface DeduplicateResult { + /** Findings that are not duplicates - should be posted */ + newFindings: Finding[]; + /** Actions to take for duplicate findings */ + duplicateActions: DuplicateAction[]; } /** @@ -59,10 +90,8 @@ export function parseMarker(body: string): WardenMarker | null { return null; } - const [, path, lineStr, contentHash] = match; - if (!path || !lineStr || !contentHash) { - return null; - } + // Capture groups are guaranteed to exist when the regex matches + const [, path, lineStr, contentHash] = match as [string, string, string, string]; return { path, @@ -102,12 +131,52 @@ export function isWardenComment(body: string): boolean { return body.includes('warden:') || body.includes('