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

Merged
TO
3mo ago
+175-41

What & why

I reviewed the automated-review (cubic) findings on the stage→prod deploy PR (#3506) — 40 findings. One is genuinely harmful to users, and it's fixed here. The rest are already-fixed or documented-deferred (details below).

The issue (P1) — fixed

A scheduled browser automation flipped any non-done/non-failed task to done or failed — including tasks a person deliberately marked not_relevant (which carries a human justification). That silently overwrote the compliance decision and orphaned its justification.

The fix

Guard both the done and failed transitions so an automated run never overwrites a not_relevant task — matching the existing codebase norm (cloud-security already "skips not_relevant (user intent)"). Added a small exported helper isTaskStatusProtectedFromAutomation + unit tests.

Scope: apps/api/src/trigger/browser-automation/run-browser-automation.ts (+ spec). 8 tests pass, typecheck clean.

The other 39 findings — why they're not here

  • Already fixed on the feature branch. The deploy diff re-includes the original code, so cubic re-surfaces issues that are already addressed: the API-error .error checks, task:* vs integration:* permission gating, empty-state connect gating, evidence best-effort capture, session cleanup on callback throw, markVerified freshness, the roll-up pass verdict, the MFA-instruction cache, etc.
  • Documented-deferred edges (recorded, none prod-blocking): orphaned 1Password item on credential re-save (H-2), session cleanup on a mid-connect unmount, the sibling-automation task-status race, 1Password resolve-error classification, and the DNS-blind IsSafeUrl (mitigated — the browser runs in Browserbase's isolated egress, not our network).
  • Low / not-an-issue: the vendor-logo route is a favicon proxy to a fixed upstream host (not an open SSRF); the remainder are accessibility / test-coverage nits.

Only the not_relevant overwrite would actively harm a user's data — hence a focused PR.

Summary by cubic

Scheduled browser runs no longer overwrite tasks marked not_relevant; human decisions and justifications are preserved. Done/failed transitions are now guarded atomically and only apply when the task isn’t already done/failed.

  • Bug Fixes
    • Use atomic updateMany with a WHERE that excludes done, failed, and protected statuses (isTaskStatusProtectedFromAutomation) to prevent race conditions; derive statusChangedToFailed from affected row count.
    • Add unit tests for isTaskStatusProtectedFromAutomation and worker-level tests that assert the atomic updateMany filters and zero-count handling; compute reviewDate from frequency without pre-checking current status.