fix(api): don't let scheduled browser runs overwrite not_relevant tasks#3507
Merged
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
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.
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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-failedtask todoneorfailed— including tasks a person deliberately markednot_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_relevanttask — matching the existing codebase norm (cloud-security already "skips not_relevant (user intent)"). Added a small exported helperisTaskStatusProtectedFromAutomation+ 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
.errorchecks,task:*vsintegration:*permission gating, empty-state connect gating, evidence best-effort capture, session cleanup on callback throw,markVerifiedfreshness, the roll-uppassverdict, the MFA-instruction cache, etc.IsSafeUrl(mitigated — the browser runs in Browserbase's isolated egress, not our network).vendor-logoroute is a favicon proxy to a fixed upstream host (not an open SSRF); the remainder are accessibility / test-coverage nits.Only the
not_relevantoverwrite 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 alreadydone/failed.updateManywith a WHERE that excludesdone,failed, and protected statuses (isTaskStatusProtectedFromAutomation) to prevent race conditions; derivestatusChangedToFailedfrom affected row count.isTaskStatusProtectedFromAutomationand worker-level tests that assert the atomicupdateManyfilters and zero-count handling; computereviewDatefromfrequencywithout pre-checking current status.Written for commit 2ea3283. Summary will update on new commits.