fix(api): don't let scheduled browser runs overwrite not_relevant tasks

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);
+  });
+});

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(