Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions .changeset/fix-exception-duplicate-notification.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"layne": patch
---

Fix duplicate notifications being sent on every scan when exception approvals are active
16 changes: 14 additions & 2 deletions src/__tests__/worker.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1045,17 +1045,29 @@ describe('processJob()', () => {
}));
});

it('always notifies when exception is approved, even if finding count did not increase', async () => {
it('notifies when exception is approved and the job was triggered by the approval comment', async () => {
const exceptions = new Map([['LAYNE-a3f29c81', { approver: 'alice', reason: 'ok', timestamp: '' }]]);
(loadExceptions as ReturnType<typeof vi.fn>).mockResolvedValueOnce(exceptions);
(buildExceptionSummary as ReturnType<typeof vi.fn>).mockReturnValueOnce({ conclusion: 'success', summary: 'Excepted.' });
(redis.get as ReturnType<typeof vi.fn>).mockResolvedValueOnce('1'); // same count as current finding

await processJob(baseJob);
const exceptionTriggeredJob = { ...baseJob, data: { ...baseJob.data, triggeredByException: true } };
await processJob(exceptionTriggeredJob);

expect(notify).toHaveBeenCalledOnce();
});

it('does not notify on exception approval when the scan was triggered by a new commit push', async () => {
const exceptions = new Map([['LAYNE-a3f29c81', { approver: 'alice', reason: 'ok', timestamp: '' }]]);
(loadExceptions as ReturnType<typeof vi.fn>).mockResolvedValueOnce(exceptions);
(buildExceptionSummary as ReturnType<typeof vi.fn>).mockReturnValueOnce({ conclusion: 'success', summary: 'Excepted.' });
(redis.get as ReturnType<typeof vi.fn>).mockResolvedValueOnce('1'); // same count as current finding — no increase

await processJob(baseJob); // triggeredByException is absent

expect(notify).not.toHaveBeenCalled();
});

it('passes exceptionApproval: null to notify when no exception approvers are configured', async () => {
(loadScanConfig as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
semgrep: { enabled: true, extraArgs: [] }, trufflehog: { enabled: true, extraArgs: [] },
Expand Down
5 changes: 4 additions & 1 deletion src/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ async function handleIssueComment(payload: Record<string, unknown>): Promise<{ s
installation,
jobId,
action: 'issue_comment',
triggeredByException: true,
}).catch(err => console.error(`[server] Failed to enqueue scan: ${(err as Error).message}`));
}

Expand Down Expand Up @@ -513,7 +514,7 @@ async function isJobActive(jobId: string): Promise<boolean> {
return false;
}

async function enqueueScan({ pull_request, repository, installation, jobId, action }: {
async function enqueueScan({ pull_request, repository, installation, jobId, action, triggeredByException }: {
pull_request: {
number: number;
head: { sha: string; ref: string };
Expand All @@ -524,6 +525,7 @@ async function enqueueScan({ pull_request, repository, installation, jobId, acti
installation: Record<string, unknown>;
jobId: string;
action: string;
triggeredByException?: boolean;
}): Promise<{ status: number; body: string }> {
if (await isJobActive(jobId)) {
debug('server', `duplicate webhook ignored: job already active for ${jobId}`);
Expand Down Expand Up @@ -566,6 +568,7 @@ async function enqueueScan({ pull_request, repository, installation, jobId, acti
prNumber: pull_request.number,
labels: pull_request.labels?.map(l => l.name) ?? [],
checkRunId,
...(triggeredByException ? { triggeredByException: true } : {}),
}, {
jobId,
});
Expand Down
1 change: 1 addition & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ export interface JobData {
prNumber: number;
labels: string[];
checkRunId: number;
triggeredByException?: boolean;
}

export interface PRCacheData {
Expand Down
6 changes: 4 additions & 2 deletions src/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,8 +289,10 @@ async function runScan(job: Job<JobData>, scanConfig: Awaited<ReturnType<typeof
const prevCount = await getNotifyCount(owner, repo, prNumber);
await setNotifyCount(owner, repo, prNumber, actionableFindings.length);

// Always notify on exception approval, otherwise only on new findings
if (exceptionApproval?.approved || actionableFindings.length > prevCount) {
// Notify on exception approval (only when this scan was triggered by the approval comment),
// or when the finding count has increased since the last scan.
const isExceptionApprovalScan = job.data.triggeredByException === true;
if ((isExceptionApprovalScan && exceptionApproval?.approved) || actionableFindings.length > prevCount) {
await notify({ findings: actionableFindings, owner, repo, prNumber, notificationConfig: scanConfig.notifications, exceptionApproval })
.catch(err => console.error('[worker] notification dispatch error:', (err as Error).message));
}
Expand Down