Skip to content

feat(action): Improve duplicate comment UX #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dcramer merged 2 commits into from
Jan 31, 2026
Merged

feat(action): Improve duplicate comment UX #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd187e9
feat(action): Improve duplicate comment UX
dcramer Jan 31, 2026
6b33a95
fix(dedup): Address review feedback for duplicate comment handling
dcramer Jan 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 47 additions & 13 deletions src/action/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,12 @@
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';
Expand Down Expand Up @@ -232,72 +233,72 @@
async function postReviewToGitHub(
octokit: Octokit,
context: EventContext,
result: RenderResult
): Promise<void> {
if (!context.pullRequest) {
return;
}

// Only post PR reviews with inline comments - skip standalone summary comments
// as they add noise without providing actionable inline feedback
if (!result.review) {
return;
}

const { owner, name: repo } = context.repository;
const pullNumber = context.pullRequest.number;
const commitId = context.pullRequest.headSha;

const reviewComments = result.review.comments
.filter((c): c is typeof c & { path: string; line: number } => Boolean(c.path && c.line))
.map((c) => ({
path: c.path,
line: c.line,
side: c.side ?? ('RIGHT' as const),
body: c.body,
start_line: c.start_line,
start_side: c.start_line ? c.start_side ?? ('RIGHT' as const) : undefined,
}));

await octokit.pulls.createReview({
owner,
repo,
pull_number: pullNumber,
commit_id: commitId,
event: result.review.event,
body: result.review.body,
comments: reviewComments,
});
}

/**
* Get the default branch for a repository from the GitHub API.
*/
async function getDefaultBranchFromAPI(
octokit: Octokit,
owner: string,
repo: string
): Promise<string> {
const { data } = await octokit.repos.get({ owner, repo });
return data.default_branch;
}

/**
* Handle scheduled analysis events.
*/
async function runScheduledAnalysis(
octokit: Octokit,
inputs: ActionInputs,
repoPath: string
): Promise<void> {
logGroup('Loading configuration');
console.log(`Config path: ${inputs.configPath}`);
logGroupEnd();

const configFullPath = join(repoPath, inputs.configPath);
const config = loadWardenConfig(dirname(configFullPath));

// Find schedule triggers

Check warning on line 301 in src/action/main.ts

View check run for this annotation

@sentry/warden / warden: testing-guidelines

Missing test coverage for new exported function fetchExistingComments 

The new `fetchExistingComments` function is exported but has no tests. Per testing guidelines, every user entry point (including exported functions) should have at least one basic test with mocked external services.
const scheduleTriggers = config.triggers.filter((t) => t.event === 'schedule');
if (scheduleTriggers.length === 0) {
console.log('No schedule triggers configured');
Expand Down Expand Up @@ -419,85 +420,85 @@
failureReasons.push(`${trigger.name}: Found ${count} ${failOn}+ severity issues`);
}

logGroupEnd();
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
triggerErrors.push(`${trigger.name}: ${errorMessage}`);
console.error(`::warning::Trigger ${trigger.name} failed: ${error}`);
logGroupEnd();
}
}

handleTriggerErrors(triggerErrors, scheduleTriggers.length);

// Set outputs
const criticalCount = countSeverity(allReports, 'critical');
const highCount = countSeverity(allReports, 'high');

Check warning on line 436 in src/action/main.ts

View check run for this annotation

@sentry/warden / warden: testing-guidelines

Missing test coverage for new exported function updateWardenComment 

The new `updateWardenComment` function is exported but has no tests. Per testing guidelines, every user entry point (including exported functions) should have at least one basic test with mocked external services.

setOutput('findings-count', totalFindings);
setOutput('critical-count', criticalCount);
setOutput('high-count', highCount);
setOutput('summary', allReports.map((r) => r.summary).join('\n') || 'Scheduled analysis complete');

if (shouldFailAction) {
setFailed(failureReasons.join('; '));
}

console.log(`\nScheduled analysis complete: ${totalFindings} total findings`);
}

async function run(): Promise<void> {
const inputs = getInputs();

Check warning on line 451 in src/action/main.ts

View check run for this annotation

@sentry/warden / warden: testing-guidelines

Missing test coverage for new exported function addReactionToComment 

The new `addReactionToComment` function is exported but has no tests. Per testing guidelines, every user entry point (including exported functions) should have at least one basic test with mocked external services.

if (!inputs.githubToken) {
setFailed('GitHub token is required');
}

const eventName = process.env['GITHUB_EVENT_NAME'];
const eventPath = process.env['GITHUB_EVENT_PATH'];
const repoPath = process.env['GITHUB_WORKSPACE'];

if (!eventName || !eventPath || !repoPath) {
setFailed('This action must be run in a GitHub Actions environment');
}

// Set both env vars so code using either will work
process.env['WARDEN_ANTHROPIC_API_KEY'] = inputs.anthropicApiKey;
process.env['ANTHROPIC_API_KEY'] = inputs.anthropicApiKey;

const octokit = new Octokit({ auth: inputs.githubToken });

// Route schedule events to dedicated handler
if (eventName === 'schedule' || eventName === 'workflow_dispatch') {
return runScheduledAnalysis(octokit, inputs, repoPath);
}

let eventPayload: unknown;
try {
eventPayload = JSON.parse(readFileSync(eventPath, 'utf-8'));
} catch (error) {
setFailed(`Failed to read event payload: ${error}`);
}

logGroup('Building event context');
console.log(`Event: ${eventName}`);
console.log(`Workspace: ${repoPath}`);
logGroupEnd();

let context: EventContext;
try {
context = await buildEventContext(eventName, eventPayload, repoPath, octokit);
} catch (error) {
setFailed(`Failed to build event context: ${error}`);
}

logGroup('Loading configuration');
console.log(`Config path: ${inputs.configPath}`);
logGroupEnd();

const configFullPath = join(repoPath, inputs.configPath);
const config = loadWardenConfig(dirname(configFullPath));

Check warning on line 501 in src/action/main.ts

View check run for this annotation

@sentry/warden / warden: testing-guidelines

Missing test coverage for new exported function processDuplicateActions 

The new `processDuplicateActions` function is exported but has no tests. Per testing guidelines, every user entry point (including exported functions) should have at least one basic test with mocked external services.
// Resolve triggers with defaults and match
const resolvedTriggers = config.triggers.map((t) => resolveTrigger(t, config));
const matchedTriggers = resolvedTriggers.filter((t) => matchTrigger(t, context));
Expand Down Expand Up @@ -643,21 +644,25 @@

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,
context.pullRequest.number
);
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}`);
Expand All @@ -681,13 +686,40 @@
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`);
Copy link

@cursor cursor bot Jan 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Skipped duplicate actions not logged, reducing user visibility

Medium Severity

The processDuplicateActions function returns a skipped count, but this value is never logged in main.ts. When multiple triggers in the same run detect the same issue, duplicate actions are created but always skipped because findingToExistingComment creates in-memory comments without the body property needed for updates. Users see "Found N duplicate findings" in logs but get no indication that skill attribution updates were skipped. This contradicts the PR's goal of "providing better visibility into which skills detected each issue."

Additional Locations (1)

Fix in Cursor Fix in Web

}
}

Expand Down Expand Up @@ -715,7 +747,7 @@
? 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);
}
Expand Down Expand Up @@ -748,12 +780,14 @@
// 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);
Expand All @@ -764,7 +798,7 @@
} 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');
}

Expand Down
101 changes: 91 additions & 10 deletions src/output/dedup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 () => {
Expand All @@ -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,
Expand All @@ -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 () => {
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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,
Expand All @@ -243,22 +253,73 @@ 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 () => {
const findings = [baseFinding];
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---\n<sub>warden: security-review</sub>`;
expect(parseWardenSkills(body)).toEqual(['security-review']);
});

it('parses multiple skills', () => {
const body = `**:warning: Issue**\n\nDescription\n\n---\n<sub>warden: security-review, code-quality, performance</sub>`;
expect(parseWardenSkills(body)).toEqual(['security-review', 'code-quality', 'performance']);
});

it('handles extra whitespace', () => {
const body = `<sub>warden: skill1 , skill2 </sub>`;
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---\n<sub>warden: skill1</sub>`;
const result = updateWardenCommentBody(body, 'skill2');
expect(result).toContain('<sub>warden: skill1, skill2</sub>');
});

it('returns null if skill already listed', () => {
const body = `<sub>warden: skill1, skill2</sub>`;
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---\n<sub>warden: security-review</sub>\n<!-- warden:v1:file.ts:10:abc123 -->`;
const result = updateWardenCommentBody(body, 'code-quality');
expect(result).toContain('**:warning: SQL Injection**');
expect(result).toContain('User input passed to query');
expect(result).toContain('<sub>warden: security-review, code-quality</sub>');
expect(result).toContain('<!-- warden:v1:file.ts:10:abc123 -->');
});
});

Expand All @@ -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',
Expand Down
Loading
Loading