From 2346759c6724c9999dd3759cb953b27ba150df3b Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Sun, 26 Jul 2026 23:41:44 -0400 Subject: [PATCH 1/3] fix(api): scheduled browser runs must not overwrite a not_relevant task MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A scheduled browser automation flipped any non-done/non-failed task to done or failed — including tasks a human deliberately marked not_relevant (with a justification). That silently destroyed the compliance decision. Guard both the done and failed transitions with isTaskStatusProtectedFromAutomation, matching the codebase norm (cloud-security skips not_relevant as user intent). +tests. --- .../run-browser-automation.spec.ts | 14 +++++++++ .../run-browser-automation.ts | 29 +++++++++++++++---- 2 files changed, 38 insertions(+), 5 deletions(-) diff --git a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts index 907d4b0788..e2f1dfa509 100644 --- a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts +++ b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts @@ -23,6 +23,7 @@ jest.mock('@trycompai/email', () => ({ })); import { + isTaskStatusProtectedFromAutomation, shouldMarkTaskDoneAfterBrowserRun, shouldMarkTaskFailedAfterBrowserRun, } from './run-browser-automation'; @@ -75,3 +76,16 @@ describe('shouldMarkTaskFailedAfterBrowserRun', () => { expect(shouldMarkTaskFailedAfterBrowserRun({ evaluationStatus: 'pass' })).toBe(false); }); }); + +describe('isTaskStatusProtectedFromAutomation', () => { + it('protects a deliberate human not_relevant decision', () => { + // A scheduled run must never overwrite this (it has a human justification). + expect(isTaskStatusProtectedFromAutomation('not_relevant')).toBe(true); + }); + + it('does not protect the normal automatable statuses', () => { + for (const status of ['todo', 'in_progress', 'in_review', 'done', 'failed']) { + expect(isTaskStatusProtectedFromAutomation(status)).toBe(false); + } + }); +}); diff --git a/apps/api/src/trigger/browser-automation/run-browser-automation.ts b/apps/api/src/trigger/browser-automation/run-browser-automation.ts index 9ae79d9f51..952296dfce 100644 --- a/apps/api/src/trigger/browser-automation/run-browser-automation.ts +++ b/apps/api/src/trigger/browser-automation/run-browser-automation.ts @@ -37,6 +37,17 @@ export function shouldMarkTaskFailedAfterBrowserRun(input: { return input.evaluationStatus === 'fail' || input.needsReauth === true; } +/** + * Task statuses a scheduled run must NOT overwrite — a deliberate human decision. + * `not_relevant` is set by a person (with a justification) to exclude the task + * from compliance; an automation flipping it to done/failed would silently + * destroy that decision. Mirrors the codebase norm (cloud-security skips + * `not_relevant` "user intent"). Exported for unit testing. + */ +export function isTaskStatusProtectedFromAutomation(status: string): boolean { + return status === 'not_relevant'; +} + /** * Worker task that runs a single browser automation. * @@ -123,7 +134,11 @@ export const runBrowserAutomation = task({ select: { status: true, frequency: true }, }); - if (currentTask && currentTask.status !== 'done') { + if ( + currentTask && + currentTask.status !== 'done' && + !isTaskStatusProtectedFromAutomation(currentTask.status) + ) { let reviewDate: Date | undefined; if (currentTask.frequency) { reviewDate = new Date(); @@ -177,10 +192,14 @@ export const runBrowserAutomation = task({ const oldStatus = taskBeforeUpdate?.status ?? 'todo'; // Transition only: don't re-flip / re-report a task that's already - // failed. The per-org runner (run-org-browser-automations) collects - // these transitions and sends one bundled failure email — covering both - // `needs_reauth` and control-regressed (`evaluation fail`) cases. - if (oldStatus !== 'failed') { + // failed. Also never overwrite a deliberate human decision like + // `not_relevant`. The per-org runner (run-org-browser-automations) + // collects these transitions and sends one bundled failure email — + // covering both `needs_reauth` and control-regressed (`evaluation fail`). + if ( + oldStatus !== 'failed' && + !isTaskStatusProtectedFromAutomation(oldStatus) + ) { await db.task.update({ where: { id: taskId }, data: { status: 'failed' }, From cab68611ca139c803026675182c796bbaba84f0e Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Sun, 26 Jul 2026 23:59:26 -0400 Subject: [PATCH 2/3] fix(api): make the not_relevant guard an atomic conditional update The status check and the update were separate queries, so a human marking a task not_relevant between them could still be overwritten. Move the guard into the WHERE clause of an atomic updateMany (excluding done/failed + not_relevant), and derive statusChangedToFailed from whether a row actually changed. --- .../run-browser-automation.spec.ts | 2 +- .../run-browser-automation.ts | 103 +++++++++--------- 2 files changed, 50 insertions(+), 55 deletions(-) diff --git a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts index e2f1dfa509..70b9b9dcd2 100644 --- a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts +++ b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts @@ -2,7 +2,7 @@ jest.mock('@db', () => ({ db: { browserAutomation: { findUnique: jest.fn(), update: jest.fn() }, browserAutomationRun: { create: jest.fn() }, - task: { findUnique: jest.fn(), update: jest.fn() }, + task: { findUnique: jest.fn(), update: jest.fn(), updateMany: jest.fn() }, organization: { findUnique: jest.fn() }, member: { findMany: jest.fn() }, }, diff --git a/apps/api/src/trigger/browser-automation/run-browser-automation.ts b/apps/api/src/trigger/browser-automation/run-browser-automation.ts index 952296dfce..9ea65b38bd 100644 --- a/apps/api/src/trigger/browser-automation/run-browser-automation.ts +++ b/apps/api/src/trigger/browser-automation/run-browser-automation.ts @@ -38,14 +38,19 @@ export function shouldMarkTaskFailedAfterBrowserRun(input: { } /** - * Task statuses a scheduled run must NOT overwrite — a deliberate human decision. - * `not_relevant` is set by a person (with a justification) to exclude the task - * from compliance; an automation flipping it to done/failed would silently - * destroy that decision. Mirrors the codebase norm (cloud-security skips - * `not_relevant` "user intent"). Exported for unit testing. + * Task statuses a scheduled run must NEVER overwrite — deliberate human + * decisions. `not_relevant` is set by a person (with a justification) to exclude + * the task from compliance; an automation flipping it to done/failed would + * silently destroy that decision. Mirrors the codebase norm (cloud-security + * skips `not_relevant` as "user intent"). Single source of truth for both the + * exported guard and the atomic status-update WHERE clauses below. */ +export const AUTOMATION_PROTECTED_TASK_STATUSES = ['not_relevant'] as const; + export function isTaskStatusProtectedFromAutomation(status: string): boolean { - return status === 'not_relevant'; + return (AUTOMATION_PROTECTED_TASK_STATUSES as readonly string[]).includes( + status, + ); } /** @@ -131,40 +136,36 @@ export const runBrowserAutomation = task({ ) { const currentTask = await db.task.findUnique({ where: { id: taskId }, - select: { status: true, frequency: true }, + select: { frequency: true }, }); - if ( - currentTask && - currentTask.status !== 'done' && - !isTaskStatusProtectedFromAutomation(currentTask.status) - ) { - let reviewDate: Date | undefined; - if (currentTask.frequency) { - reviewDate = new Date(); - switch (currentTask.frequency) { - case 'monthly': - reviewDate.setMonth(reviewDate.getMonth() + 1); - break; - case 'quarterly': - reviewDate.setMonth(reviewDate.getMonth() + 3); - break; - case 'yearly': - reviewDate.setFullYear(reviewDate.getFullYear() + 1); - break; - } + let reviewDate: Date | undefined; + if (currentTask?.frequency) { + reviewDate = new Date(); + switch (currentTask.frequency) { + case 'monthly': + reviewDate.setMonth(reviewDate.getMonth() + 1); + break; + case 'quarterly': + reviewDate.setMonth(reviewDate.getMonth() + 3); + break; + case 'yearly': + reviewDate.setFullYear(reviewDate.getFullYear() + 1); + break; } - - await db.task.update({ - where: { id: taskId }, - data: { - status: 'done', - ...(reviewDate ? { reviewDate } : {}), - }, - }); - - logger.info(`Task ${taskId} marked as done`); } + + // Atomic: flip to done only if it isn't already done and isn't a + // protected human status — the guard is in the WHERE, so a concurrent + // `not_relevant` action can't be clobbered between a read and the write. + const doneUpdate = await db.task.updateMany({ + where: { + id: taskId, + status: { notIn: ['done', ...AUTOMATION_PROTECTED_TASK_STATUSES] }, + }, + data: { status: 'done', ...(reviewDate ? { reviewDate } : {}) }, + }); + if (doneUpdate.count > 0) logger.info(`Task ${taskId} marked as done`); } } else { logger.error(`Automation ${automationId} failed`, { @@ -185,25 +186,19 @@ export const runBrowserAutomation = task({ needsReauth: result.needsReauth, }) ) { - const taskBeforeUpdate = await db.task.findUnique({ - where: { id: taskId }, - select: { status: true }, + // Atomic transition: flip to failed only if it isn't already failed and + // isn't a protected human status (e.g. `not_relevant`). The guard lives + // in the WHERE so a concurrent human action can't be overwritten between + // a read and the write; count > 0 means THIS run caused the transition + // (which drives the per-org bundled failure email). + const failedUpdate = await db.task.updateMany({ + where: { + id: taskId, + status: { notIn: ['failed', ...AUTOMATION_PROTECTED_TASK_STATUSES] }, + }, + data: { status: 'failed' }, }); - const oldStatus = taskBeforeUpdate?.status ?? 'todo'; - - // Transition only: don't re-flip / re-report a task that's already - // failed. Also never overwrite a deliberate human decision like - // `not_relevant`. The per-org runner (run-org-browser-automations) - // collects these transitions and sends one bundled failure email — - // covering both `needs_reauth` and control-regressed (`evaluation fail`). - if ( - oldStatus !== 'failed' && - !isTaskStatusProtectedFromAutomation(oldStatus) - ) { - await db.task.update({ - where: { id: taskId }, - data: { status: 'failed' }, - }); + if (failedUpdate.count > 0) { statusChangedToFailed = true; } else { logger.info( From 2ea3283766640f377a51af3390908cbfec9a8912 Mon Sep 17 00:00:00 2001 From: Tofik Hasanov Date: Mon, 27 Jul 2026 00:13:40 -0400 Subject: [PATCH 3/3] test(api): worker-level coverage for the atomic not_relevant guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Assert the worker's done and failed transitions both go through updateMany with a status filter that excludes not_relevant (and done/failed), and that a zero-count result is treated as no transition — so a regression in either atomic filter is caught instead of silently letting not_relevant be overwritten. --- .../run-browser-automation.spec.ts | 106 ++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts index 70b9b9dcd2..759dce010e 100644 --- a/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts +++ b/apps/api/src/trigger/browser-automation/run-browser-automation.spec.ts @@ -22,12 +22,32 @@ jest.mock('@trycompai/email', () => ({ isUserUnsubscribed: jest.fn().mockResolvedValue(false), })); +// Control what the actual browser run returns so we can drive the task-status +// transitions in the worker body. +const mockRunBrowserAutomation = jest.fn(); +jest.mock('../../browserbase/browserbase.service', () => ({ + BrowserbaseService: jest.fn().mockImplementation(() => ({ + runBrowserAutomation: mockRunBrowserAutomation, + })), +})); + +import { db } from '@db'; import { isTaskStatusProtectedFromAutomation, + runBrowserAutomation, shouldMarkTaskDoneAfterBrowserRun, shouldMarkTaskFailedAfterBrowserRun, } from './run-browser-automation'; +type WorkerTask = { + run: (payload: { + automationId: string; + automationName: string; + organizationId: string; + taskId: string; + }) => Promise<{ statusChangedToFailed: boolean }>; +}; + describe('shouldMarkTaskDoneAfterBrowserRun', () => { it('allows screenshot-only automations to complete tasks', () => { expect( @@ -89,3 +109,89 @@ describe('isTaskStatusProtectedFromAutomation', () => { } }); }); + +describe('runBrowserAutomation worker — task-status transitions are atomic + guarded', () => { + const worker = runBrowserAutomation as unknown as WorkerTask; + const payload = { + automationId: 'bau_1', + automationName: 'GitHub MFA', + organizationId: 'org_1', + taskId: 'tsk_1', + }; + + beforeEach(() => { + jest.clearAllMocks(); + (db.browserAutomation.findUnique as jest.Mock).mockResolvedValue({ + id: 'bau_1', + isEnabled: true, + evaluationCriteria: 'MFA is enforced', + }); + // findUnique serves both the title lookup and the frequency lookup. + (db.task.findUnique as jest.Mock).mockResolvedValue({ + title: 'Enforce MFA', + frequency: 'monthly', + }); + (db.task.updateMany as jest.Mock).mockResolvedValue({ count: 1 }); + (db.browserAutomation.update as jest.Mock).mockResolvedValue({}); + }); + + it('flips a passing run to done via an updateMany that excludes not_relevant', async () => { + mockRunBrowserAutomation.mockResolvedValue({ + success: true, + runId: 'bar_1', + screenshotUrl: 'https://s3/shot.jpg', + evaluationStatus: 'pass', + }); + + await worker.run(payload); + + expect(db.task.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { + id: 'tsk_1', + status: { notIn: expect.arrayContaining(['done', 'not_relevant']) }, + }, + data: expect.objectContaining({ status: 'done' }), + }), + ); + // The task is never updated by primary key alone (which would ignore the guard). + expect(db.task.update).not.toHaveBeenCalled(); + }); + + it('flips a regressed run to failed via an updateMany that excludes not_relevant', async () => { + mockRunBrowserAutomation.mockResolvedValue({ + success: false, + runId: 'bar_1', + evaluationStatus: 'fail', + }); + + const result = await worker.run(payload); + + expect(db.task.updateMany).toHaveBeenCalledWith( + expect.objectContaining({ + where: { + id: 'tsk_1', + status: { notIn: expect.arrayContaining(['failed', 'not_relevant']) }, + }, + data: { status: 'failed' }, + }), + ); + expect(result.statusChangedToFailed).toBe(true); + expect(db.task.update).not.toHaveBeenCalled(); + }); + + it('treats a zero-count update as no transition (protected/already-failed task)', async () => { + mockRunBrowserAutomation.mockResolvedValue({ + success: false, + runId: 'bar_1', + evaluationStatus: 'fail', + }); + // The atomic guard matched nothing — e.g. the task was concurrently marked + // not_relevant, or was already failed. + (db.task.updateMany as jest.Mock).mockResolvedValue({ count: 0 }); + + const result = await worker.run(payload); + + expect(result.statusChangedToFailed).toBe(false); + }); +});