Repository navigation
Add first-class codemode scheduler - #157
Merged
Merged
Conversation
createEnv omitted SCHEDULER_DO after EnvSchema required it, so getEnv() threw and resolveSessionEmail swallowed the error as a null session. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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:
📝 WalkthroughWalkthroughThis pull request implements a first-class scheduler system that allows users to schedule codemode code execution on recurring (cron) or one-shot (ISO 8601) schedules. It adds a Durable Object ( Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/MCP Client
participant Cap as Scheduler Capability
participant Client as Scheduler Client
participant DO as SchedulerDO
participant Storage as DO Storage
participant Pipeline as Codemode Pipeline
User->>Cap: scheduler_upsert (name, code, schedule)
Cap->>Cap: requireSchedulerUser()
Cap->>Client: schedulerCreate/Update()
Client->>DO: POST /jobs (callerContext, job data)
DO->>DO: Parse & normalize job (schedule, timezone)
DO->>DO: Compute nextRunAt
DO->>Storage: Save job:<jobId> & job-context:<jobId>
DO->>DO: syncAlarm() - set to earliest nextRunAt
DO-->>Client: Return ScheduledJobView
Client-->>Cap: Return ScheduledJobView
Cap-->>User: Return ScheduledJobView
Note over DO: [Time passes, alarm fires]
DO->>Storage: Load all jobs
DO->>DO: Filter jobs where enabled && nextRunAt <= now
DO->>DO: processDueJobs(jobs, executeJob callback)
loop For each due job
DO->>Pipeline: runCodemodeWithRegistry (code, params, callerContext)
Pipeline-->>DO: result or error
DO->>DO: Record lastRunAt, lastRunStatus, lastRunError
alt Cron schedule
DO->>DO: computeNextRunAt()
DO->>Storage: Save updated job
else Once schedule
DO->>Storage: Delete job:<jobId> & job-context:<jobId>
end
end
DO->>DO: syncAlarm() - set to next earliest nextRunAt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
April 12, 2026 19:42
Contributor
|
🔎 Preview deployed: https://kody-pr-157.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/worker/src/scheduler/process-due-jobs.node.test.ts (1)
44-54: Prefer order-agnostic assertions forsaveJobs.These assertions currently depend on array order. If processing order changes, this can fail despite correct behavior.
Proposed refactor
- expect(result.saveJobs[0]).toMatchObject({ + const firstSaved = result.saveJobs.find((job) => job.id === 'job-1') + expect(firstSaved).toMatchObject({ id: 'job-1', lastRunStatus: 'error', lastRunError: 'boom', lastRunAt: now.toISOString(), }) - expect(result.saveJobs[1]).toMatchObject({ + const secondSaved = result.saveJobs.find((job) => job.id === 'job-2') + expect(secondSaved).toMatchObject({ id: 'job-2', lastRunStatus: 'success', lastRunAt: now.toISOString(), })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/scheduler/process-due-jobs.node.test.ts` around lines 44 - 54, The test currently asserts result.saveJobs by index which is order-dependent; update it to be order-agnostic by asserting the array contains the expected job objects regardless of order—e.g., replace the two index-based expects on result.saveJobs[...] with a single assertion using expect.arrayContaining and expect.objectContaining for the two job shapes (id 'job-1' with lastRunStatus 'error' and lastRunError 'boom', and id 'job-2' with lastRunStatus 'success'), or alternatively map result.saveJobs by id and assert each mapped entry matches the expected properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 233-260: The test hard-codes a future timestamp causing flakiness
when that date passes; instead generate a dynamic future runAt (e.g., Date.now()
+ some offset) before calling mcpClient.client.callTool and pass that ISO
timestamp into the codemode.scheduler_update arguments (refer to jobId and
codemode.scheduler_update in the call), then assert against that generated ISO
string (use the same value for both expected schedule.runAt and nextRunAt when
inspecting updateStructured) so the test always uses a valid future time.
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 23-24: The current key callerContextStorageKey
('scheduler:caller-context') is global to the Durable Object and causes caller
context (baseUrl, connector refs, storageContext) to be overwritten across jobs;
change all uses to persist caller context per-job by composing the storage key
from jobStorageKeyPrefix and the job id (e.g., jobStorageKeyPrefix + jobId +
':caller-context') so each job stores/reads its own context. Update every
read/write that references callerContextStorageKey (including the block around
lines 287-297) to use the per-job key and ensure functions that load or apply
caller context accept/derive jobId when constructing the storage key.
- Around line 172-199: The current update always calls computeNextRunAt and
overwrites existing.nextRunAt; change this to preserve existing.nextRunAt unless
the schedule or timezone actually changed or the job is being re-enabled:
compare the new schedule (from normalizeScheduledJobSchedule) and new timezone
(from normalizeSchedulerTimezone) to existing.schedule/timezone, and only call
computeNextRunAt({schedule, timezone}) when they differ or when
payload.body.enabled transitions from false to true; otherwise set
updated.nextRunAt = existing.nextRunAt. Use the existing symbols
(normalizeScheduledJobSchedule, normalizeSchedulerTimezone, computeNextRunAt,
payload.body.enabled, existing) to locate where to add the conditional.
---
Nitpick comments:
In `@packages/worker/src/scheduler/process-due-jobs.node.test.ts`:
- Around line 44-54: The test currently asserts result.saveJobs by index which
is order-dependent; update it to be order-agnostic by asserting the array
contains the expected job objects regardless of order—e.g., replace the two
index-based expects on result.saveJobs[...] with a single assertion using
expect.arrayContaining and expect.objectContaining for the two job shapes (id
'job-1' with lastRunStatus 'error' and lastRunError 'boom', and id 'job-2' with
lastRunStatus 'success'), or alternatively map result.saveJobs by id and assert
each mapped entry matches the expected properties.
🪄 Autofix (Beta)
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
Run ID: 45491ff8-6081-4270-8081-b85dc07990c8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
docs/use/execute.mdpackage.jsonpackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/scheduler/domain.tspackages/worker/src/mcp/capabilities/scheduler/index.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-create.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-delete.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-get.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-list.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-run-now.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-update.tspackages/worker/src/mcp/capabilities/scheduler/shared.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/scheduler/client.tspackages/worker/src/scheduler/process-due-jobs.node.test.tspackages/worker/src/scheduler/process-due-jobs.tspackages/worker/src/scheduler/schedule.node.test.tspackages/worker/src/scheduler/schedule.tspackages/worker/src/scheduler/scheduler-do.tspackages/worker/src/scheduler/types.tspackages/worker/wrangler.jsonc
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/worker/src/scheduler/scheduler-do.ts (2)
24-25:⚠️ Potential issue | 🟠 MajorPersist caller context per job, not per Durable Object.
This still shares one execution context across every job in the user's DO, so a later create/update/run-now can overwrite
baseUrl, connector refs, orstorageContextfor older jobs. Store caller context under a job-specific key and load it per job duringalarm(); use a separate namespace such asjob-context:${jobId}so thejob:prefix scan inlistStoredJobs()keeps returning onlyScheduledJobrecords.Also applies to: 122-126, 299-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 24 - 25, The callerContextStorageKey currently stores caller context on the DO level causing later operations to overwrite context for other jobs; change to persist caller context per job by using a job-scoped key pattern (e.g. job-context:${jobId}) instead of callerContextStorageKey, update storage writes where callerContextStorageKey is set (e.g. in create/update/run-now flows) to write under job-context:${jobId}, and update alarm() and listStoredJobs() readers to load the job-specific context by reading job-context:${jobId} when handling a ScheduledJob; ensure the jobStorageKeyPrefix ('job:') remains reserved for ScheduledJob records so listStoredJobs() scanning logic continues to return only ScheduledJob entries.
182-209:⚠️ Potential issue | 🟠 MajorPreserve
nextRunAton metadata-only updates.Line 206 still recomputes
nextRunAtfor every PATCH. Renaming a job or editingcode/paramscan silently push an already pending run out to a later time. Keepexisting.nextRunAtunless the normalized schedule/timezone changed orenabledis transitioning fromfalsetotrue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 182 - 209, The code always recomputes nextRunAt (via computeNextRunAt) when building the updated ScheduledJob, which can move a pending run; change the logic in the update block that constructs updated (around schedule, timezone, enabled) so that nextRunAt is set to existing.nextRunAt unless one of three conditions is true: the normalized schedule changed (compare new schedule vs existing.schedule), the normalized timezone changed (compare timezone vs existing.timezone), or enabled is transitioning from false to true (existing.enabled === false && (payload.body.enabled ?? existing.enabled) === true); only in those cases call computeNextRunAt({ schedule, timezone }) to assign nextRunAt. Reference symbols: normalizeScheduledJobSchedule, normalizeSchedulerTimezone, payload.body.enabled, existing.nextRunAt, computeNextRunAt, and the updated: ScheduledJob construction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 235-242: The call to executeJob in the run-now path can throw
(e.g., runCodemodeWithRegistry throws) and currently bypasses the normal error
shape; wrap the executeJob(...) invocation in a try/catch and convert any thrown
exception into a unified failure result object (e.g., { ok: false, error: string
| Error, ... }) so the subsequent code can always read execution.ok and
execution.error; make the same change for the other direct executeJob call noted
(lines ~261-279) so both paths persist lastRunStatus/lastRunError consistently.
---
Duplicate comments:
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 24-25: The callerContextStorageKey currently stores caller context
on the DO level causing later operations to overwrite context for other jobs;
change to persist caller context per job by using a job-scoped key pattern (e.g.
job-context:${jobId}) instead of callerContextStorageKey, update storage writes
where callerContextStorageKey is set (e.g. in create/update/run-now flows) to
write under job-context:${jobId}, and update alarm() and listStoredJobs()
readers to load the job-specific context by reading job-context:${jobId} when
handling a ScheduledJob; ensure the jobStorageKeyPrefix ('job:') remains
reserved for ScheduledJob records so listStoredJobs() scanning logic continues
to return only ScheduledJob entries.
- Around line 182-209: The code always recomputes nextRunAt (via
computeNextRunAt) when building the updated ScheduledJob, which can move a
pending run; change the logic in the update block that constructs updated
(around schedule, timezone, enabled) so that nextRunAt is set to
existing.nextRunAt unless one of three conditions is true: the normalized
schedule changed (compare new schedule vs existing.schedule), the normalized
timezone changed (compare timezone vs existing.timezone), or enabled is
transitioning from false to true (existing.enabled === false &&
(payload.body.enabled ?? existing.enabled) === true); only in those cases call
computeNextRunAt({ schedule, timezone }) to assign nextRunAt. Reference symbols:
normalizeScheduledJobSchedule, normalizeSchedulerTimezone, payload.body.enabled,
existing.nextRunAt, computeNextRunAt, and the updated: ScheduledJob
construction.
🪄 Autofix (Beta)
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
Run ID: 5e366617-5a37-4001-a079-1f3fa4f79dd7
📒 Files selected for processing (1)
packages/worker/src/scheduler/scheduler-do.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
|
||
| export function requireSchedulerUser(ctx: CapabilityContext) { | ||
| return requireMcpUser(ctx.callerContext) | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent auth helper usage across scheduler capabilities
Low Severity
requireSchedulerUser in shared.ts is a trivial wrapper around requireMcpUser(ctx.callerContext), but only scheduler-list and scheduler-update use it. The other four capabilities (scheduler-create, scheduler-delete, scheduler-get, scheduler-run-now) call requireMcpUser(ctx.callerContext) directly. This split makes it unclear whether the wrapper exists for a reason or is leftover from a refactor, increasing maintenance confusion.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 393e02b. Configure here.
Interpolating memory_id into generated codemode source triggered a workerd SWC internal error; using execute's params keeps user code stable. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/worker/src/scheduler/scheduler-do.ts (2)
241-242:⚠️ Potential issue | 🟠 MajorNormalize thrown codemode execution failures in
executeJob.If
runCodemodeWithRegistrythrows,handleRunNowcurrently bubbles into the top-levelfetchcatch and skipslastRunStatus/lastRunErrorpersistence.Suggested fix
- const execution = await runCodemodeWithRegistry( - this.env, - callerContext, - job.code, - job.params, - workerExports, - ) + let execution + try { + execution = await runCodemodeWithRegistry( + this.env, + callerContext, + job.code, + job.params, + workerExports, + ) + } catch (error) { + return { + ok: false, + error: formatSchedulerError(error), + logs: [], + } + }Also applies to: 272-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 241 - 242, executeJob currently lets exceptions from runCodemodeWithRegistry bubble up (causing handleRunNow to skip persisting lastRunStatus/lastRunError); update executeJob to catch errors from runCodemodeWithRegistry (and any codemode-related calls), normalize them into a structured result (e.g., { success: false, error: <message|stack|code> }) and return that instead of throwing so handleRunNow can always write lastRunStatus and lastRunError; specifically modify executeJob to wrap the runCodemodeWithRegistry call in a try/catch, populate a failure result, and ensure handleRunNow uses that result to update ScheduledJob.lastRunStatus and ScheduledJob.lastRunError (also apply the same pattern to the similar block around lines 272-279).
24-25:⚠️ Potential issue | 🟠 MajorPersist caller context per job, not per Durable Object.
A single
scheduler:caller-contextkey is still shared across all jobs in the user DO, so newer mutations overwrite execution context for older jobs.Suggested fix
-const callerContextStorageKey = 'scheduler:caller-context' const jobStorageKeyPrefix = 'job:' function getJobStorageKey(jobId: string) { return `${jobStorageKeyPrefix}${jobId}` } + +function getCallerContextStorageKey(jobId: string) { + return `${getJobStorageKey(jobId)}:caller-context` +} @@ - const callerContext = await this.getPersistedCallerContext() const result = await processDueJobs({ jobs: dueJobs, now, - executeJob: async (job) => this.executeJob(job, callerContext), + executeJob: async (job) => + this.executeJob(job, await this.getPersistedCallerContext(job.id)), }) @@ - await this.persistCallerContext(payload.callerContext) + await this.persistCallerContext(job.id, payload.callerContext) @@ - await this.persistCallerContext(payload.callerContext) + await this.persistCallerContext(jobId, payload.callerContext) @@ - await this.persistCallerContext(payload.callerContext) + await this.persistCallerContext(jobId, payload.callerContext) @@ - private async persistCallerContext( + private async persistCallerContext( + jobId: string, callerContext: PersistedSchedulerCallerContext, ) { - await this.ctx.storage.put(callerContextStorageKey, callerContext) + await this.ctx.storage.put(getCallerContextStorageKey(jobId), callerContext) } - private async getPersistedCallerContext() { + private async getPersistedCallerContext(jobId: string) { return ( (await this.ctx.storage.get<PersistedSchedulerCallerContext>( - callerContextStorageKey, + getCallerContextStorageKey(jobId), )) ?? null ) }Also applies to: 122-123, 310-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 24 - 25, The code currently stores caller context under a single key (callerContextStorageKey) so newer jobs overwrite older ones; change the storage scheme to persist context per job by composing the key from jobStorageKeyPrefix + jobId (e.g., `${jobStorageKeyPrefix}${jobId}:caller-context`) and update all places that read/write callerContextStorageKey to use this per-job key; ensure all usages (reads, writes, deletes) that reference callerContextStorageKey are replaced so each job uses its own storage key derived from jobId.packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts (1)
233-260:⚠️ Potential issue | 🟠 MajorAvoid hard-coded future
runAtin scheduler update E2E.This timestamp will become stale and eventually break the test when
once.runAtmust be in the future.Suggested fix
+ const futureRunAt = new Date(Date.now() + 24 * 60 * 60 * 1000).toISOString() + const updateResult = await mcpClient.client.callTool({ name: 'execute', arguments: { code: `async () => { return await codemode.scheduler_update({ id: ${JSON.stringify(jobId)}, enabled: false, - schedule: { type: 'once', runAt: '2026-04-18T15:00:00Z' }, + schedule: { type: 'once', runAt: ${JSON.stringify(futureRunAt)} }, }) }`, }, }) @@ expect(updateStructured?.result?.schedule).toEqual({ type: 'once', - runAt: '2026-04-18T15:00:00.000Z', + runAt: futureRunAt, }) - expect(updateStructured?.result?.nextRunAt).toBe('2026-04-18T15:00:00.000Z') + expect(updateStructured?.result?.nextRunAt).toBe(futureRunAt)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts` around lines 233 - 260, The test uses a hard-coded future timestamp for codemode.scheduler_update which will become stale; instead compute a dynamic future ISO timestamp when building the callTool code (e.g., now + some minutes) and inject that value into the code string passed to mcpClient.client.callTool, then update the assertions that reference updateStructured?.result?.schedule.runAt and nextRunAt to expect the computed ISO (with millisecond precision) rather than the fixed literal; locate the call in the test where callTool is invoked and the subsequent expectations for updateStructured to make these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 182-186: The type-narrowing for payload.body.schedule fails
because the boolean flag hasScheduleUpdate prevents TypeScript from tracking the
narrowed type; replace that pattern by extracting payload.body.schedule into a
local variable (e.g., const newSchedule = payload.body.schedule), use a direct
check on that variable (newSchedule !== undefined) and call
normalizeScheduledJobSchedule(newSchedule) when present, otherwise fall back to
existing.schedule; update the code that defines schedule to reference this local
variable and normalizeScheduledJobSchedule to satisfy typechecking.
---
Duplicate comments:
In `@packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts`:
- Around line 233-260: The test uses a hard-coded future timestamp for
codemode.scheduler_update which will become stale; instead compute a dynamic
future ISO timestamp when building the callTool code (e.g., now + some minutes)
and inject that value into the code string passed to mcpClient.client.callTool,
then update the assertions that reference
updateStructured?.result?.schedule.runAt and nextRunAt to expect the computed
ISO (with millisecond precision) rather than the fixed literal; locate the call
in the test where callTool is invoked and the subsequent expectations for
updateStructured to make these changes.
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 241-242: executeJob currently lets exceptions from
runCodemodeWithRegistry bubble up (causing handleRunNow to skip persisting
lastRunStatus/lastRunError); update executeJob to catch errors from
runCodemodeWithRegistry (and any codemode-related calls), normalize them into a
structured result (e.g., { success: false, error: <message|stack|code> }) and
return that instead of throwing so handleRunNow can always write lastRunStatus
and lastRunError; specifically modify executeJob to wrap the
runCodemodeWithRegistry call in a try/catch, populate a failure result, and
ensure handleRunNow uses that result to update ScheduledJob.lastRunStatus and
ScheduledJob.lastRunError (also apply the same pattern to the similar block
around lines 272-279).
- Around line 24-25: The code currently stores caller context under a single key
(callerContextStorageKey) so newer jobs overwrite older ones; change the storage
scheme to persist context per job by composing the key from jobStorageKeyPrefix
+ jobId (e.g., `${jobStorageKeyPrefix}${jobId}:caller-context`) and update all
places that read/write callerContextStorageKey to use this per-job key; ensure
all usages (reads, writes, deletes) that reference callerContextStorageKey are
replaced so each job uses its own storage key derived from jobId.
🪄 Autofix (Beta)
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
Run ID: 546b4d15-14e5-4ca7-a6ff-31ff7a609ca9
📒 Files selected for processing (2)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/scheduler/scheduler-do.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/scheduler/scheduler-do.ts`:
- Around line 66-69: The normalizeOptionalParams function currently returns any
JSON value; update it to enforce that params must be a plain JSON object per the
ScheduledJob.params contract: if params is null/undefined return undefined,
otherwise validate that typeof params === "object" and Array.isArray(params) ===
false (and not null) and throw a TypeError (or reject) when the value is an
array or a non-object primitive so invalid inputs are rejected up front;
reference the function name normalizeOptionalParams and ensure the thrown error
message clearly states it expects an object for ScheduledJob.params.
- Around line 111-112: The current catch in the scheduler handler
indiscriminately maps all exceptions to a 400 using formatSchedulerError; change
it so only explicit client/request validation errors (the specific error
type/class that formatSchedulerError expects) are returned as new
Response(formatSchedulerError(error), { status: 400 }), and all other exceptions
are rethrown (or allowed to bubble) so they surface as 5xx and are captured by
the outer Sentry wrapper around the handler (see Sentry wrapper near lines
~397-400); locate the catch in scheduler-do.ts and use type/instance checks
against the client-error type or a predicate to decide whether to return 400 vs
rethrow.
🪄 Autofix (Beta)
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
Run ID: d109ad26-fb42-4604-9d07-79b9f8be4b8f
📒 Files selected for processing (3)
packages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/scheduler/process-due-jobs.node.test.tspackages/worker/src/scheduler/scheduler-do.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/scheduler/process-due-jobs.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts
Comment on lines
+66
to
+69
| function normalizeOptionalParams( | ||
| params: Record<string, unknown> | null | undefined, | ||
| ): Record<string, unknown> | undefined { | ||
| return params === null || params === undefined ? undefined : params |
There was a problem hiding this comment.
Validate params as a JSON object before persisting it.
request.json() is untyped at runtime, so this currently accepts arrays, strings, numbers, etc. That violates the ScheduledJob.params contract and pushes bad input into scheduled execution instead of rejecting it up front.
Suggested fix
-function normalizeOptionalParams(
- params: Record<string, unknown> | null | undefined,
-): Record<string, unknown> | undefined {
- return params === null || params === undefined ? undefined : params
+function normalizeOptionalParams(
+ params: unknown,
+): Record<string, unknown> | undefined {
+ if (params === null || params === undefined) {
+ return undefined
+ }
+ if (typeof params !== 'object' || Array.isArray(params)) {
+ throw new Error('Scheduled job params must be a JSON object.')
+ }
+ return params as Record<string, unknown>
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function normalizeOptionalParams( | |
| params: Record<string, unknown> | null | undefined, | |
| ): Record<string, unknown> | undefined { | |
| return params === null || params === undefined ? undefined : params | |
| function normalizeOptionalParams( | |
| params: unknown, | |
| ): Record<string, unknown> | undefined { | |
| if (params === null || params === undefined) { | |
| return undefined | |
| } | |
| if (typeof params !== 'object' || Array.isArray(params)) { | |
| throw new Error('Scheduled job params must be a JSON object.') | |
| } | |
| return params as Record<string, unknown> | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 66 - 69, The
normalizeOptionalParams function currently returns any JSON value; update it to
enforce that params must be a plain JSON object per the ScheduledJob.params
contract: if params is null/undefined return undefined, otherwise validate that
typeof params === "object" and Array.isArray(params) === false (and not null)
and throw a TypeError (or reject) when the value is an array or a non-object
primitive so invalid inputs are rejected up front; reference the function name
normalizeOptionalParams and ensure the thrown error message clearly states it
expects an object for ScheduledJob.params.
Comment on lines
+111
to
+112
| } catch (error) { | ||
| return new Response(formatSchedulerError(error), { status: 400 }) |
There was a problem hiding this comment.
Don't collapse server failures into HTTP 400s.
This catch turns everything—including storage faults and unexpected bugs—into a client error. It also swallows exceptions before the Sentry wrapper at Lines 397-400 can see them. Restrict 4xx responses to explicit request errors and let unexpected failures surface as 500s.
Suggested direction
- } catch (error) {
- return new Response(formatSchedulerError(error), { status: 400 })
+ } catch (error) {
+ if (error instanceof SchedulerNotFoundError) {
+ return new Response(error.message, { status: 404 })
+ }
+ if (error instanceof SchedulerValidationError) {
+ return new Response(error.message, { status: 400 })
+ }
+ throw error
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/scheduler/scheduler-do.ts` around lines 111 - 112, The
current catch in the scheduler handler indiscriminately maps all exceptions to a
400 using formatSchedulerError; change it so only explicit client/request
validation errors (the specific error type/class that formatSchedulerError
expects) are returned as new Response(formatSchedulerError(error), { status: 400
}), and all other exceptions are rethrown (or allowed to bubble) so they surface
as 5xx and are captured by the outer Sentry wrapper around the handler (see
Sentry wrapper near lines ~397-400); locate the catch in scheduler-do.ts and use
type/instance checks against the client-error type or a predicate to decide
whether to return 400 vs rethrow.
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/scheduler/scheduler-upsert.ts (1)
29-35: Avoid defaulting required create fields to empty strings.Line 29 and Line 30 silently coerce missing create inputs into
''. It’s safer to fail fast and preserve the required-field contract.Proposed fix
async handler(args, ctx: CapabilityContext) { const user = requireSchedulerUser(ctx) if (args.id === undefined) { + if ( + args.name === undefined || + args.code === undefined || + args.schedule === undefined + ) { + throw new Error( + 'scheduler_upsert create requires name, code, and schedule.', + ) + } return schedulerCreate(ctx.env, user.userId, { callerContext: ctx.callerContext, body: { - name: args.name ?? '', - code: args.code ?? '', + name: args.name, + code: args.code, ...(args.params !== undefined && args.params !== null ? { params: args.params } : {}), - schedule: args.schedule!, + schedule: args.schedule,Based on learnings: Follow MCP capabilities specifications as documented in docs/contributing/adding-capabilities.md and mcp-apps-spec-notes.md
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/scheduler/scheduler-upsert.ts` around lines 29 - 35, The code in scheduler-upsert.ts is masking missing required create fields by defaulting args.name and args.code to empty strings; instead, preserve the required-field contract by removing the "?? ''" defaults and enforce presence (e.g., throw or validate) before building the payload. Update the builder that uses args.name and args.code to either (a) perform an explicit null/undefined check and throw a clear error referencing "name" or "code", or (b) use the non-null asserted values expected by the create flow so missing inputs fail early; keep the existing conditional handling for params and timezone and leave schedule as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/scheduler/shared.ts`:
- Around line 22-25: The runAt schema currently only checks non-empty strings,
so update the runAt Zod validator (the runAt field in the schema in shared.ts)
to validate an ISO 8601 UTC timestamp: replace .min(1) with a refinement that
asserts the string matches an ISO8601 UTC pattern (for example using a regex
like /\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z/ or use
z.string().datetime() if your Zod version supports it) and provide a clear error
message (e.g. "must be an ISO 8601 UTC timestamp like 2023-01-01T00:00:00Z") so
malformed timestamps are rejected at the schema boundary.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/scheduler/scheduler-upsert.ts`:
- Around line 29-35: The code in scheduler-upsert.ts is masking missing required
create fields by defaulting args.name and args.code to empty strings; instead,
preserve the required-field contract by removing the "?? ''" defaults and
enforce presence (e.g., throw or validate) before building the payload. Update
the builder that uses args.name and args.code to either (a) perform an explicit
null/undefined check and throw a clear error referencing "name" or "code", or
(b) use the non-null asserted values expected by the create flow so missing
inputs fail early; keep the existing conditional handling for params and
timezone and leave schedule as-is.
🪄 Autofix (Beta)
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
Run ID: 08236ecc-a61d-4de0-a740-e2be1ffe6096
📒 Files selected for processing (10)
docs/use/execute.mdpackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/mcp/capabilities/scheduler/domain.tspackages/worker/src/mcp/capabilities/scheduler/scheduler-upsert.tspackages/worker/src/mcp/capabilities/scheduler/shared.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/scheduler/types.ts
✅ Files skipped from review due to trivial changes (7)
- packages/worker/src/mcp/tools/search.ts
- packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts
- packages/worker/src/mcp/tools/execute.ts
- packages/worker/src/mcp/server-instructions.ts
- docs/use/execute.md
- packages/worker/src/mcp/capabilities/scheduler/domain.ts
- packages/worker/src/scheduler/types.ts
Comment on lines
+22
to
+25
| runAt: z | ||
| .string() | ||
| .min(1) | ||
| .describe('ISO 8601 UTC timestamp for a one-shot run.'), |
There was a problem hiding this comment.
Validate once.runAt at the schema boundary.
Line 22 only enforces non-empty text, so malformed timestamps can pass capability validation and fail deeper in scheduler execution.
Proposed fix
z.object({
type: z.literal('once'),
runAt: z
.string()
.min(1)
+ .refine(
+ (value) => !Number.isNaN(Date.parse(value)) && value.endsWith('Z'),
+ 'runAt must be an ISO 8601 UTC timestamp ending with "Z".',
+ )
.describe('ISO 8601 UTC timestamp for a one-shot run.'),
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| runAt: z | |
| .string() | |
| .min(1) | |
| .describe('ISO 8601 UTC timestamp for a one-shot run.'), | |
| runAt: z | |
| .string() | |
| .min(1) | |
| .refine( | |
| (value) => !Number.isNaN(Date.parse(value)) && value.endsWith('Z'), | |
| 'runAt must be an ISO 8601 UTC timestamp ending with "Z".', | |
| ) | |
| .describe('ISO 8601 UTC timestamp for a one-shot run.'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/scheduler/shared.ts` around lines 22 -
25, The runAt schema currently only checks non-empty strings, so update the
runAt Zod validator (the runAt field in the schema in shared.ts) to validate an
ISO 8601 UTC timestamp: replace .min(1) with a refinement that asserts the
string matches an ISO8601 UTC pattern (for example using a regex like
/\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d+)?Z/ or use z.string().datetime() if
your Zod version supports it) and provide a clear error message (e.g. "must be
an ISO 8601 UTC timestamp like 2023-01-01T00:00:00Z") so malformed timestamps
are rejected at the schema boundary.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit b43e833. Configure here.
| } else { | ||
| await this.ctx.storage.put(getJobStorageKey(jobId), updated) | ||
| await this.persistCallerContext(jobId, payload.callerContext) | ||
| } |
Contributor
There was a problem hiding this comment.
Missing syncAlarm in handleRunNow recurring branch
Low Severity
The handleRunNow method's else branch (recurring jobs) writes to storage but omits the syncAlarm() call that every other storage-mutating handler includes (handleCreateJob, handleUpdateJob, handleDeleteJob, the once-type branch of handleRunNow, and alarm()). While handleRunNow doesn't currently change nextRunAt or enabled, the missing call breaks the self-healing pattern and means a previously-lost alarm won't be re-established by this code path.
Reviewed by Cursor Bugbot for commit b43e833. Configure here.
This was referenced Apr 14, 2026
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
2 participants
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.
Closes #156
Summary
SchedulerDOwith Durable Object storage, alarm scheduling, and codemode execution reusescheduler_upsert,scheduler_list,scheduler_get,scheduler_delete, andscheduler_run_nowcodemode capabilitiesnextRunAt, and normalized execution failuresexecuteparams and using a dynamic futurerunAttimestampTesting
npm run typechecknpx vitest run --project node-unit packages/worker/src/scheduler/schedule.node.test.ts packages/worker/src/scheduler/process-due-jobs.node.test.tsnpx vitest run --project workers-unit packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts packages/worker/src/oauth-handlers.workers.test.tsnpx vitest run --project mcp-e2e packages/worker/src/mcp/mcp-server.mcp-e2e.test.ts -t "mcp server manages scheduled codemode jobs"Summary by CodeRabbit
New Features
Documentation