Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe evaluation route reserves terminal occurrences and queues dispatch metadata. The dispatch route validates and processes scheduled messages. The failure route records terminal delivery failures and conditionally disables matching automations. Tests cover terminal, recurring, stale, disabled, and failure paths. ChangesAutomation terminal lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AutomationEvaluationRoute
participant QStash
participant DispatchRoute
participant RunFailedRoute
participant Database
participant AutomationDispatch
AutomationEvaluationRoute->>Database: read due automation
AutomationEvaluationRoute->>AutomationEvaluationRoute: compute and reserve occurrence
AutomationEvaluationRoute->>QStash: enqueue terminal or recurring payload
QStash->>DispatchRoute: deliver scheduled payload
DispatchRoute->>Database: validate automation state
DispatchRoute->>AutomationDispatch: dispatch valid occurrence
DispatchRoute->>Database: finalize terminal automation
QStash->>RunFailedRoute: deliver terminal failure
RunFailedRoute->>Database: record dispatch_failed run
RunFailedRoute->>Database: disable matching terminal automation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR preserves delivery of a bounded automation’s final scheduled occurrence after evaluation disables the exhausted recurrence.
Confidence Score: 3/5This PR should not merge until terminal delivery can distinguish evaluator exhaustion from a user pause made after enqueue. The new terminal bypass allows a signed queued message to start an agent after the user has disabled the automation because both disable paths preserve the same Files Needing Attention: apps/api/src/app/api/automations/dispatch/[id]/route.ts
|
| Filename | Overview |
|---|---|
| apps/api/src/app/api/automations/dispatch/[id]/route.ts | Adds terminal-message handling and stale-minute validation, but permits a queued terminal run after a user pause. |
| apps/api/src/app/api/automations/evaluate/route.ts | Computes recurrence advancement once, marks exhausted occurrences terminal, and disables them after enqueue. |
| apps/api/src/app/api/automations/dispatch/[id]/route.test.ts | Covers exhausted-terminal, disabled non-terminal, and enabled dispatch paths, but not a user pause occurring after terminal enqueue. |
| apps/api/src/app/api/automations/evaluate/route.test.ts | Verifies terminal marking and preserves the existing non-terminal advancement behavior. |
Prompt To Fix All With AI
### Issue 1
apps/api/src/app/api/automations/dispatch/[id]/route.ts:63-64
**Terminal dispatch bypasses user pause**
When a user pauses an automation after its terminal occurrence is enqueued but before delivery, `terminal` bypasses the disabled guard and the unchanged `nextRunAt` passes the stale check, causing `dispatchAutomation` to create a run and potentially start an agent despite the pause.
**How this was verified:** The pause path preserves `nextRunAt`, while this route accepts a matching terminal message and the dispatcher performs side effects without another enabled-state check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(api): preserve terminal scheduled au..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const scheduledFor = new Date(parsed.data.scheduledFor); | ||
| if (!automation.enabled && !parsed.data.terminal) { |
There was a problem hiding this comment.
When a user pauses an automation after its terminal occurrence is enqueued but before delivery, terminal bypasses the disabled guard and the unchanged nextRunAt passes the stale check, causing dispatchAutomation to create a run and potentially start an agent despite the pause.
How this was verified: The pause path preserves nextRunAt, while this route accepts a matching terminal message and the dispatcher performs side effects without another enabled-state check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/app/api/automations/dispatch/[id]/route.ts
Line: 63-64
Comment:
**Terminal dispatch bypasses user pause**
When a user pauses an automation after its terminal occurrence is enqueued but before delivery, `terminal` bypasses the disabled guard and the unchanged `nextRunAt` passes the stale check, causing `dispatchAutomation` to create a run and potentially start an agent despite the pause.
**How this was verified:** The pause path preserves `nextRunAt`, while this route accepts a matching terminal message and the dispatcher performs side effects without another enabled-state check.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app/api/automations/dispatch/`[id]/route.test.ts:
- Around line 80-133: Add a test alongside the terminal-dispatch cases that
submits a terminal payload whose scheduledFor minute differs from
automation.nextRunAt. Assert the POST response is successful with skipped:
"stale", and verify dispatchAutomation was not called.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc66d563-c347-42e8-8479-9cf524e3a6ed
📒 Files selected for processing (4)
apps/api/src/app/api/automations/dispatch/[id]/route.test.tsapps/api/src/app/api/automations/dispatch/[id]/route.tsapps/api/src/app/api/automations/evaluate/route.test.tsapps/api/src/app/api/automations/evaluate/route.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Architecture diagram
sequenceDiagram
participant Sched as Cron / Scheduler
participant Eval as Evaluate Route
participant DB as Database
participant QStash as QStash Queue
participant Dispatch as Dispatch Route
participant Dispatcher as Automation Dispatcher
note over Sched,Dispatcher: Scheduled Automation Dispatch with Terminal Handling
Sched->>Eval: POST /api/automations/evaluate (cron trigger)
Eval->>DB: Select due automations (enabled, nextRunAt <= now)
DB-->>Eval: List of due automations
loop For each due automation
Eval->>Eval: Compute next occurrence after current nextRunAt
alt Has next occurrence (non-terminal)
Eval-->>Eval: next = computedDate
else No next occurrence (terminal)
Eval-->>Eval: next = null
end
Eval->>QStash: batchJSON - enqueue dispatch message
note over Eval,QStash: body includes { terminal: (next === null) }
alt next is null (terminal)
Eval->>DB: UPDATE set { enabled: false }
else next exists (non-terminal)
Eval->>DB: UPDATE set { nextRunAt: next }
end
end
QStash-->>Dispatch: Deliver queued message (after delay)
Dispatch->>Dispatch: Verify QStash signature (signed payload)
Dispatch->>DB: SELECT automation by id
DB-->>Dispatch: Automation record
alt Automation not found
Dispatch-->>QStash: 200 { skipped: "deleted" }
else Automation disabled AND NOT terminal
Dispatch-->>QStash: 200 { skipped: "disabled" }
else Terminal AND scheduledFor minute != automation.nextRunAt minute
note over Dispatch: Schedule changed after enqueue
Dispatch-->>QStash: 200 { skipped: "stale" }
else Automation enabled OR (disabled + terminal + matching minute)
Dispatch->>Dispatcher: dispatchAutomation({ automation, scheduledFor })
alt Dispatcher succeeds
Dispatcher-->>Dispatch: { status: "dispatched", runId }
Dispatch-->>QStash: 200 { ok: true, outcome }
else Dispatcher fails (offline/failed)
Dispatcher-->>Dispatch: { status: "offline" | "dispatch_failed" }
Dispatch-->>QStash: 200 { ok: true, outcome }
end
end
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed in f272fcf. The Greptile finding was valid. A terminal occurrence now remains enabled while it is queued, so a user pause before delivery is honored. The dispatch route or QStash failure callback closes the recurrence only after an observable outcome, using a conditional occurrence match. Added coverage for user pause, stale terminal delivery, and terminal delivery failure. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/app/api/automations/dispatch/`[id]/route.ts:
- Around line 80-91: The terminal dispatch error path around
finalizeTerminalAutomation must not acknowledge the message with HTTP 200 after
finalization fails. Persist a durable, idempotent finalization retry before
returning, using the existing retry mechanism or a dedicated persisted job
rather than normal terminal evaluation, while preserving the successful dispatch
outcome and avoiding duplicate terminal processing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6096adf9-f4cf-4711-b6e0-d56b95b58616
📒 Files selected for processing (6)
apps/api/src/app/api/automations/dispatch/[id]/route.test.tsapps/api/src/app/api/automations/dispatch/[id]/route.tsapps/api/src/app/api/automations/evaluate/route.test.tsapps/api/src/app/api/automations/evaluate/route.tsapps/api/src/app/api/automations/run-failed/route.test.tsapps/api/src/app/api/automations/run-failed/route.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/src/app/api/automations/evaluate/route.ts
|
Addressed the finalization retry finding in c22b4ad. If terminal finalization fails after dispatch has recorded an outcome, the route now returns non-2xx so QStash retries the idempotent finalization. If retries exhaust, run-failed preserves an existing completed/offline run instead of rewriting it as dispatch_failed, then closes the recurrence. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/api/automations/dispatch/[id]/route.ts (1)
63-78: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winClaim terminal delivery before invoking the dispatcher.
The test at
apps/api/src/app/api/automations/dispatch/[id]/route.test.ts:158already expects that disabling the automation beforedispatchAutomationprevents terminal delivery. The current route uses the preloadedautomation.enabledvalue, so a user pause between lines 63 and 74 allows delivery of a terminal occurrence. Use a guarded, durable claim on the automation state before dispatch and only dispatch when that update succeeds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/api/automations/dispatch/`[id]/route.ts around lines 63 - 78, Update the route’s terminal-dispatch flow around the enabled and stale checks to atomically claim the terminal occurrence in durable automation state before calling dispatchAutomation. Re-read or condition the update on the automation still being enabled and matching the scheduled run, and invoke dispatchAutomation only when the guarded claim succeeds; otherwise return the appropriate skipped response and prevent delivery after a concurrent pause.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/app/api/automations/dispatch/`[id]/route.ts:
- Around line 63-78: Update the route’s terminal-dispatch flow around the
enabled and stale checks to atomically claim the terminal occurrence in durable
automation state before calling dispatchAutomation. Re-read or condition the
update on the automation still being enabled and matching the scheduled run, and
invoke dispatchAutomation only when the guarded claim succeeds; otherwise return
the appropriate skipped response and prevent delivery after a concurrent pause.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a655a98c-78c2-42bc-8d10-864d8c56b20a
📒 Files selected for processing (4)
apps/api/src/app/api/automations/dispatch/[id]/route.test.tsapps/api/src/app/api/automations/dispatch/[id]/route.tsapps/api/src/app/api/automations/run-failed/route.test.tsapps/api/src/app/api/automations/run-failed/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/api/automations/run-failed/route.test.ts
- apps/api/src/app/api/automations/run-failed/route.ts
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/app/api/automations/dispatch/[id]/route.ts (1)
63-82: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftClaim terminal delivery before
dispatchAutomation.If a user pause commits after Line 64 reads
automation.enabledbut before Line 78 callsdispatchAutomation, this route still dispatches the terminal occurrence.finalizeTerminalAutomationonly guards the post-dispatch update.Create a durable conditional delivery claim before dispatch. Match the automation ID, enabled state, and terminal reservation. Make the pause flow coordinate with that claim.
As per PR objectives, terminal delivery must preserve user pauses.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/app/api/automations/dispatch/`[id]/route.ts around lines 63 - 82, Before calling dispatchAutomation in the route handler, atomically claim the terminal delivery using the automation ID, enabled state, and terminal reservation, and return the existing skipped response when the claim fails. Update the pause flow to coordinate with this claim so a pause committed concurrently prevents terminal dispatch; retain finalizeTerminalAutomation for post-dispatch finalization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/app/api/automations/dispatch/`[id]/route.ts:
- Around line 63-82: Before calling dispatchAutomation in the route handler,
atomically claim the terminal delivery using the automation ID, enabled state,
and terminal reservation, and return the existing skipped response when the
claim fails. Update the pause flow to coordinate with this claim so a pause
committed concurrently prevents terminal dispatch; retain
finalizeTerminalAutomation for post-dispatch finalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bed4ab79-2f7c-4330-a640-6220fc4a8ff9
📒 Files selected for processing (6)
apps/api/src/app/api/automations/dispatch/[id]/route.test.tsapps/api/src/app/api/automations/dispatch/[id]/route.tsapps/api/src/app/api/automations/evaluate/route.test.tsapps/api/src/app/api/automations/evaluate/route.tsapps/api/src/app/api/automations/run-failed/route.test.tsapps/api/src/app/api/automations/run-failed/route.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/app/api/automations/run-failed/route.test.ts
- apps/api/src/app/api/automations/evaluate/route.ts
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Addressed the latest review findings in 93383c0.
Checks: focused automation tests 14/14; Biome and repository lint pass. API typecheck remains blocked by the workspace missing next/next-server type dependencies. |
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Addressed the latest Cubic findings in d9f1d96:
Validation: automation route tests 16/16, shared RRULE tests 48/48, shared/trpc typecheck, Biome, and repository lint pass. API typecheck remains blocked by missing workspace Next.js type dependencies. |
1d90272 to
12556ec
Compare
Summary
Fixes #6128.
A bounded scheduled occurrence could be accepted by QStash and then dropped because evaluation disabled the automation before delivery. The dispatch route returned 200, so no retry, failure callback, or run record followed.
Design
Checks
Summary by CodeRabbit
Bug Fixes
Tests