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..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 @@ -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() }, }, @@ -22,11 +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( @@ -75,3 +96,102 @@ 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); + } + }); +}); + +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); + }); +}); 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..9ea65b38bd 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,22 @@ export function shouldMarkTaskFailedAfterBrowserRun(input: { return input.evaluationStatus === 'fail' || input.needsReauth === true; } +/** + * 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 (AUTOMATION_PROTECTED_TASK_STATUSES as readonly string[]).includes( + status, + ); +} + /** * Worker task that runs a single browser automation. * @@ -120,36 +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') { - 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`, { @@ -170,21 +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. 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') { - await db.task.update({ - where: { id: taskId }, - data: { status: 'failed' }, - }); + if (failedUpdate.count > 0) { statusChangedToFailed = true; } else { logger.info(