Repository navigation
Add Durable Object facets support for scheduled jobs - #163
kentcdodds wants to merge 15 commits into
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 35 minutes and 55 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds an alarm-driven Durable Object job system: DB migration, schedule utilities (cron/interval), JobRunner Durable Object with facet bridge and exec worker, D1 repo/service layers, MCP job capabilities and schemas, facet runtime bridging, runtime typings/bindings, tests, and documentation/examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as MCP Client
participant Handler as job_create Handler
participant Service as Job Service
participant DB as D1
participant Runner as JobRunner DO
participant RPC as jobRunnerRpc
Client->>Handler: job_create request
Handler->>Service: createJobRecord(...)
Service->>DB: INSERT job row
DB-->>Service: JobRecord
Service-->>Handler: JobRecord
Handler->>RPC: syncJobRunnerFromDb(userId, jobId)
RPC->>Runner: configure(config)
Runner->>Runner: computeNextRunAt & set alarm
Runner-->>RPC: ok
RPC-->>Handler: runner info
Handler-->>Client: job details + nextRunAt
sequenceDiagram
participant Alarm as System Alarm
participant Runner as JobRunner DO
participant Facet as Job Facet
participant Storage as SQLite
Alarm->>Runner: alarm fires
Runner->>Runner: load config & check enabled/kill-switch
alt skipped
Runner->>Storage: record skipped run or resync alarm
else execute
Runner->>Facet: getFacetStub() then facet.run()
Facet-->>Runner: result / error
Runner->>Storage: insert run_history row
Runner->>Storage: update runner_status counters/timestamps
Runner->>Runner: computeNextRunAt & set alarm
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
7d4749f to
19aef9a
Compare
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/capabilities/domain-metadata.ts (1)
6-10:⚠️ Potential issue | 🟡 MinorRemove
schedulerfromcapabilityDomainNames— it's been replaced byjobsin the built-in registry.
jobswas added tobuiltinDomains, butschedulerremains in the canonical enum even thoughschedulerDomainis no longer exported or registered. This creates a stale domain ID inBuiltinCapabilityDomain. Remove theschedulerentry unless there's a specific backward-compatibility requirement for external consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/domain-metadata.ts` around lines 6 - 10, Remove the stale "scheduler" entry from the canonical domain list and related enum: delete the 'scheduler' item from capabilityDomainNames (and any corresponding string in BuiltinCapabilityDomain) so it matches builtinDomains which now uses 'jobs', and ensure any references to schedulerDomain export/registration are removed or replaced with jobs; update BuiltinCapabilityDomain, capabilityDomainNames, and any export named schedulerDomain to avoid the stale domain ID.
🧹 Nitpick comments (8)
packages/worker/src/mcp/facet-runtime.ts (1)
124-129: Remove unusedserverCodeargument from wrapper factory input.
createFacetWrapperModuleacceptsserverCodebut never uses it, which makes the API misleading.Cleanup diff
function createFacetWrapperModule(input: { baseClassName: string expectedExportDescription: string facetName: string - serverCode: string }) { @@ 'facet-entry.js': createFacetWrapperModule({ baseClassName: input.baseClassName, expectedExportDescription: input.expectedExportDescription, facetName: input.facetName, - serverCode: input.serverCode, }),Also applies to: 181-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/facet-runtime.ts` around lines 124 - 129, The createFacetWrapperModule factory's input includes an unused serverCode property; remove serverCode from the input type and the function signature of createFacetWrapperModule (and any local parameter destructuring) and update all call sites that pass serverCode (including the other occurrence noted) to stop supplying that argument; also update any related types or JSDoc referencing serverCode so the API no longer advertises this unused parameter.packages/worker/src/mcp/server-instructions.ts (1)
25-25: Consider mentioningjob_deletein the same guidance bullet.The new line covers create/update/run/control/history well; adding delete keeps the quick-reference list complete for lifecycle operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/server-instructions.ts` at line 25, Update the guidance bullet to mention job_delete alongside job_create and job_update so the lifecycle operations are complete: when describing `job_create` / `job_update` include `job_delete` and keep the rest of the list (`job_list` / `job_get`, `job_run_now`, `job_history`, and `job_enable` / `job_disable`) as-is for inspecting, triggering, viewing history, and controlling alarm scheduling.packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts (1)
79-80: UsecapabilityDomainNames.jobsinstead of raw'jobs'literals.This keeps the test coupled to canonical domain ids and avoids string drift.
Suggested test tweak
- expect(builtinDomains.some((domain) => domain.name === 'jobs')).toBe(true) - const jobsDomain = builtinDomains.find((domain) => domain.name === 'jobs') + expect( + builtinDomains.some( + (domain) => domain.name === capabilityDomainNames.jobs, + ), + ).toBe(true) + const jobsDomain = builtinDomains.find( + (domain) => domain.name === capabilityDomainNames.jobs, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts` around lines 79 - 80, The test currently uses raw string literals 'jobs' when asserting and finding the jobs domain; replace those literals with the canonical identifier capabilityDomainNames.jobs and ensure capabilityDomainNames is imported into the test file so the assertions become expect(builtinDomains.some(domain => domain.name === capabilityDomainNames.jobs)).toBe(true) and const jobsDomain = builtinDomains.find(domain => domain.name === capabilityDomainNames.jobs).packages/worker/migrations/0018-jobs.sql (1)
7-10: Add table-level integrity checks for schedule and enabled fields.Line 7 and Line 9 currently accept malformed values if data is written outside repo helpers. Consider constraining
schedule_jsonandenabledat schema level to prevent corrupt job rows.Proposed migration tweak
CREATE TABLE jobs ( id TEXT PRIMARY KEY NOT NULL, user_id TEXT NOT NULL, name TEXT NOT NULL, server_code TEXT NOT NULL, server_code_id TEXT NOT NULL, - schedule_json TEXT NOT NULL, + schedule_json TEXT NOT NULL CHECK (json_valid(schedule_json)), timezone TEXT NOT NULL, - enabled INTEGER NOT NULL DEFAULT 1, + enabled INTEGER NOT NULL DEFAULT 1 CHECK (enabled IN (0, 1)), created_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP), updated_at TEXT NOT NULL DEFAULT (CURRENT_TIMESTAMP) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0018-jobs.sql` around lines 7 - 10, Add table-level CHECK constraints to the jobs table so malformed values cannot be inserted: enforce schedule_json is valid JSON and non-empty (e.g., use json_valid(schedule_json) or a DB-specific JSON cast/parse check on create) and enforce enabled can only be 0 or 1 (CHECK(enabled IN (0,1))). Update the CREATE TABLE statement that defines schedule_json and enabled to include these CHECK constraints so they apply at the schema level.packages/worker/src/mcp/capabilities/jobs/domain.ts (1)
135-137: SimplifyPromise.allSettledwith single promise.Using
Promise.allSettledfor a single promise is verbose. If the intent is to ignore failures, usecatchor a plaintry/catch.♻️ Simplified version
await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) - await Promise.allSettled([ - jobRunnerRpc(ctx.env, args.job_id).deleteRunner(), - ]) + await jobRunnerRpc(ctx.env, args.job_id).deleteRunner().catch(() => { + // Ignore errors - the job record is already deleted + }) return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/jobs/domain.ts` around lines 135 - 137, Replace the unnecessary Promise.allSettled wrapper around the single call to jobRunnerRpc(ctx.env, args.job_id).deleteRunner(): either await the promise directly and ignore errors with .catch(() => {}) or wrap the await in a try/catch and handle or log the error; ensure you reference the same call expression jobRunnerRpc(ctx.env, args.job_id).deleteRunner() so behavior stays the same while removing Promise.allSettled.packages/worker/src/jobs/schedule.node.test.ts (1)
65-67: Consider clarifying the test name.The comment "Kent timezone" is informal. A clearer name like
normalizeJobTimezone defaults to America/Denverwould be more descriptive without relying on contextual knowledge.✏️ Suggested rename
-test('normalizeJobTimezone defaults to Kent timezone', () => { +test('normalizeJobTimezone defaults to America/Denver', () => { expect(normalizeJobTimezone(null)).toBe('America/Denver') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/schedule.node.test.ts` around lines 65 - 67, Rename the test that currently reads "normalizeJobTimezone defaults to Kent timezone" to a clearer, descriptive name such as "normalizeJobTimezone defaults to America/Denver" so the behavior is explicit; update the test declaration (the test(...) call wrapping normalizeJobTimezone(null) expectation) to use the new string while leaving the assertion and function reference (normalizeJobTimezone) unchanged.packages/worker/src/jobs/service.ts (1)
33-38: Consider trimming server code for consistency.
normalizeJobServerCodechecks if the trimmed string is empty but returns the original untrimmed string. This differs fromnormalizeJobNamewhich returns the trimmed value. Consider whether leading/trailing whitespace in server code should be preserved or trimmed for consistency.♻️ Optional: Trim server code for consistency
export function normalizeJobServerCode(serverCode: string) { - if (!serverCode.trim()) { + const trimmed = serverCode.trim() + if (!trimmed) { throw new Error('Jobs require non-empty server code.') } - return serverCode + return trimmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/service.ts` around lines 33 - 38, normalizeJobServerCode currently validates using serverCode.trim() but returns the original untrimmed value, causing inconsistency with normalizeJobName; update normalizeJobServerCode to trim the input before returning (e.g., compute trimmed = serverCode.trim(); throw if trimmed is empty; return trimmed) so leading/trailing whitespace is removed and behavior matches normalizeJobName.packages/worker/src/jobs/job-runner.ts (1)
146-163: Cache the job record to avoid redundant DB calls.
getDetails()callsthis.requireJob()three times (lines 152, 158, 159), each triggering a D1 database query. Cache the result in a local variable.♻️ Cache requireJob result
async getDetails(): Promise<JobDetails> { const config = await this.readConfig(this.ctx.id.toString()) const status = await this.readRunnerStatus() + const job = await this.requireJob() return { id: config.jobId, userId: config.userId, - name: (await this.requireJob()).name, + name: job.name, serverCode: config.serverCode, serverCodeId: config.serverCodeId, schedule: config.schedule, timezone: config.timezone, enabled: config.enabled, - createdAt: (await this.requireJob()).createdAt, - updatedAt: (await this.requireJob()).updatedAt, + createdAt: job.createdAt, + updatedAt: job.updatedAt, ...status, scheduleSummary: await this.getScheduleSummary(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/job-runner.ts` around lines 146 - 163, getDetails() calls this.requireJob() multiple times causing redundant D1 queries; fix by calling await this.requireJob() once at the start of getDetails(), store the Job record in a local variable (e.g., const job = await this.requireJob()), and then use job.name, job.createdAt, and job.updatedAt when building the returned JobDetails instead of invoking requireJob() repeatedly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/use/examples/job-feed-checker.md`:
- Around line 10-20: The SELECT against feed_state runs before the table is
created in the Job.run serverCode; move the CREATE TABLE IF NOT EXISTS call
(this.ctx.storage.sql.exec(... 'CREATE TABLE IF NOT EXISTS feed_state ...')) to
run before querying (the const latestSeen = this.ctx.storage.sql.exec('SELECT
item_id FROM feed_state ...').one()...) so the table exists on first run and the
SELECT won't fail.
In `@docs/use/jobs.md`:
- Around line 141-142: Update the broken example link in the jobs documentation
by replacing the reference to ./examples/job-feed-watcher.md with the actual
example filename ./examples/job-feed-checker.md in the line that reads "See
[Feed watcher example](./examples/job-feed-watcher.md)" so the link targets the
correct file.
In `@packages/worker/src/jobs/jobs-capabilities.workers.test.ts`:
- Around line 42-54: The test helper createJobCode builds a Durable Object class
that wrongly calls this.ctx.storage.kv.get()/put(); change it to use the
standard Durable Object storage API this.ctx.storage.get() and
this.ctx.storage.put() inside the Job.run() method, ensure you await
asynchronous storage calls and coerce the possibly undefined get() result to a
number (e.g. Number(...) ?? 0) before incrementing, and persist the updated
value with await this.ctx.storage.put('count', count) so the test runtime
matches the rest of the codebase.
In `@packages/worker/src/mcp/facet-runtime.ts`:
- Around line 62-68: normalizeBridgeScope currently coerces any non-'user' scope
to 'app', which can mask typos and cause wrong data access; update the function
(normalizeBridgeScope, parameters scope and defaultScope, types FacetBridgeScope
and FacetBridgeProps['scopeLabel']) to validate resolvedScope explicitly: if
resolvedScope is 'user' return 'user', if it's 'app' return 'app', otherwise
throw a clear error (e.g., InvalidArgument or RangeError) mentioning the invalid
scope value so callers will fail-fast instead of silently using 'app'.
In `@packages/worker/wrangler.jsonc`:
- Around line 5-9: The "compatibility_flags" array currently contains a
malformed "experimental" entry; locate the "compatibility_flags" key and either
remove the bare "experimental" string or replace it with a properly namespaced
experimental flag (e.g., use the exact Cloudflare format
"experimental:<feature_name>" such as "experimental:nodejs_compat_v2" if that
was intended). Ensure the final array only contains valid flags like
"nodejs_compat", "global_fetch_strictly_public" and any correctly prefixed
experimental flag or none at all so Wrangler will accept deployment.
---
Outside diff comments:
In `@packages/worker/src/mcp/capabilities/domain-metadata.ts`:
- Around line 6-10: Remove the stale "scheduler" entry from the canonical domain
list and related enum: delete the 'scheduler' item from capabilityDomainNames
(and any corresponding string in BuiltinCapabilityDomain) so it matches
builtinDomains which now uses 'jobs', and ensure any references to
schedulerDomain export/registration are removed or replaced with jobs; update
BuiltinCapabilityDomain, capabilityDomainNames, and any export named
schedulerDomain to avoid the stale domain ID.
---
Nitpick comments:
In `@packages/worker/migrations/0018-jobs.sql`:
- Around line 7-10: Add table-level CHECK constraints to the jobs table so
malformed values cannot be inserted: enforce schedule_json is valid JSON and
non-empty (e.g., use json_valid(schedule_json) or a DB-specific JSON cast/parse
check on create) and enforce enabled can only be 0 or 1 (CHECK(enabled IN
(0,1))). Update the CREATE TABLE statement that defines schedule_json and
enabled to include these CHECK constraints so they apply at the schema level.
In `@packages/worker/src/jobs/job-runner.ts`:
- Around line 146-163: getDetails() calls this.requireJob() multiple times
causing redundant D1 queries; fix by calling await this.requireJob() once at the
start of getDetails(), store the Job record in a local variable (e.g., const job
= await this.requireJob()), and then use job.name, job.createdAt, and
job.updatedAt when building the returned JobDetails instead of invoking
requireJob() repeatedly.
In `@packages/worker/src/jobs/schedule.node.test.ts`:
- Around line 65-67: Rename the test that currently reads "normalizeJobTimezone
defaults to Kent timezone" to a clearer, descriptive name such as
"normalizeJobTimezone defaults to America/Denver" so the behavior is explicit;
update the test declaration (the test(...) call wrapping
normalizeJobTimezone(null) expectation) to use the new string while leaving the
assertion and function reference (normalizeJobTimezone) unchanged.
In `@packages/worker/src/jobs/service.ts`:
- Around line 33-38: normalizeJobServerCode currently validates using
serverCode.trim() but returns the original untrimmed value, causing
inconsistency with normalizeJobName; update normalizeJobServerCode to trim the
input before returning (e.g., compute trimmed = serverCode.trim(); throw if
trimmed is empty; return trimmed) so leading/trailing whitespace is removed and
behavior matches normalizeJobName.
In
`@packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts`:
- Around line 79-80: The test currently uses raw string literals 'jobs' when
asserting and finding the jobs domain; replace those literals with the canonical
identifier capabilityDomainNames.jobs and ensure capabilityDomainNames is
imported into the test file so the assertions become
expect(builtinDomains.some(domain => domain.name ===
capabilityDomainNames.jobs)).toBe(true) and const jobsDomain =
builtinDomains.find(domain => domain.name === capabilityDomainNames.jobs).
In `@packages/worker/src/mcp/capabilities/jobs/domain.ts`:
- Around line 135-137: Replace the unnecessary Promise.allSettled wrapper around
the single call to jobRunnerRpc(ctx.env, args.job_id).deleteRunner(): either
await the promise directly and ignore errors with .catch(() => {}) or wrap the
await in a try/catch and handle or log the error; ensure you reference the same
call expression jobRunnerRpc(ctx.env, args.job_id).deleteRunner() so behavior
stays the same while removing Promise.allSettled.
In `@packages/worker/src/mcp/facet-runtime.ts`:
- Around line 124-129: The createFacetWrapperModule factory's input includes an
unused serverCode property; remove serverCode from the input type and the
function signature of createFacetWrapperModule (and any local parameter
destructuring) and update all call sites that pass serverCode (including the
other occurrence noted) to stop supplying that argument; also update any related
types or JSDoc referencing serverCode so the API no longer advertises this
unused parameter.
In `@packages/worker/src/mcp/server-instructions.ts`:
- Line 25: Update the guidance bullet to mention job_delete alongside job_create
and job_update so the lifecycle operations are complete: when describing
`job_create` / `job_update` include `job_delete` and keep the rest of the list
(`job_list` / `job_get`, `job_run_now`, `job_history`, and `job_enable` /
`job_disable`) as-is for inspecting, triggering, viewing history, and
controlling alarm scheduling.
🪄 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: 3cb89e9a-b2c2-482d-865e-e66675311337
📒 Files selected for processing (24)
docs/use/examples/job-feed-checker.mddocs/use/execute.mddocs/use/index.mddocs/use/jobs.mdpackages/worker/migrations/0018-jobs.sqlpackages/worker/src/index.tspackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/jobs-capabilities.workers.test.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/schedule.node.test.tspackages/worker/src/jobs/schedule.tspackages/worker/src/jobs/service.tspackages/worker/src/jobs/types.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/jobs/domain.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/facet-runtime.tspackages/worker/src/mcp/server-instructions.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/mcp/tools/search.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonc
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
🔎 Preview deployed: https://kody-pr-163.kentcdodds.workers.dev Worker: Mocks:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/jobs/exec-worker.ts`:
- Around line 19-22: The generated entrypoint leaves params undefined while
assigning a fallback to paramsValue, so code that uses params (e.g.,
job_server_exec snippets referencing params.foo) can throw; update the async run
declaration in exec-worker.ts (the run method) to default the parameter (async
run(params = {})) or immediately normalize it via params = params ?? {} and then
use params throughout (remove or sync paramsValue) so any snippet that accesses
params will never see undefined.
In `@packages/worker/src/jobs/schedule.ts`:
- Line 2: The import line currently pulls in an unused type JobRecord which
causes the typecheck failure; update the import in schedule.ts to remove
JobRecord so only JobSchedule is imported (i.e., change "import { type
JobRecord, type JobSchedule } from './types.ts'" to import just JobSchedule),
ensuring no other references to JobRecord remain in the file.
In `@packages/worker/src/mcp/capabilities/jobs/domain.ts`:
- Around line 58-76: The handler in domain.ts currently persists via
createJobRecord and then calls syncJobRunnerFromDb which can throw after the DB
write; modify the handler to perform compensation: after createJobRecord(...) if
syncJobRunnerFromDb(...) throws, call the inverse delete (e.g., deleteJobRecord
or equivalent cleanup) to remove the newly created job, and for update-like
handlers that use an updateResult with a before snapshot revert the DB to
updateResult.before on failure; alternatively you can run the validation/sync
(syncJobRunnerFromDb) before committing the D1 write and only call
createJobRecord/update after sync succeeds—apply the same pattern to the other
handler blocks referenced (lines 89-118, 236-255, 270-288) and ensure errors
from sync are caught and trigger the proper compensation or rollback.
- Around line 133-137: The current flow removes the DB row before awaiting
runner cleanup and uses Promise.allSettled which hides failures; change the
order to call and await jobRunnerRpc(ctx.env, args.job_id).deleteRunner() first
(letting any thrown error propagate), and only after successful deletion call
removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id); remove the
Promise.allSettled wrapper so cleanup errors are not swallowed (requireJobRecord
can remain before cleanup to validate the row).
In `@packages/worker/src/mcp/capabilities/jobs/shared.ts`:
- Around line 10-26: The jobScheduleSchema currently uses two z.object branches
that will silently strip unknown keys (e.g., a payload with both cron and
intervalMs) and accept the first match; update each branch in jobScheduleSchema
(the two z.object(...) entries) to be strict by chaining .strict() so any extra
keys cause validation to fail rather than be dropped, ensuring ambiguous
schedules are rejected instead of silently choosing a branch.
🪄 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: 9133eefb-7fdb-4e9d-a8b2-f15a9c4c572b
📒 Files selected for processing (11)
docs/use/examples/job-feed-checker.mddocs/use/jobs.mdpackages/worker/src/index.tspackages/worker/src/jobs/exec-worker.tspackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/jobs-capabilities.workers.test.tspackages/worker/src/jobs/schedule.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/jobs/domain.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/wrangler.jsonc
✅ Files skipped from review due to trivial changes (4)
- docs/use/jobs.md
- packages/worker/src/jobs/jobs-capabilities.workers.test.ts
- docs/use/examples/job-feed-checker.md
- packages/worker/src/jobs/job-runner.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/worker/src/index.ts
- packages/worker/wrangler.jsonc
| await requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) | ||
| await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) | ||
| await Promise.allSettled([ | ||
| jobRunnerRpc(ctx.env, args.job_id).deleteRunner(), | ||
| ]) |
There was a problem hiding this comment.
Delete the runner before deleting the source-of-truth row, and don't swallow cleanup failures.
deleteRunner() in packages/worker/src/jobs/job-runner.ts (Lines 255-259) already handles the missing-runner case, so Promise.allSettled(...) only hides real cleanup failures here. With the current order, a rejected cleanup still returns success after the D1 row is gone, leaving orphaned runner state that the jobs table can no longer repair.
Suggested fix
const user = requireJobsUser(ctx)
await requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id)
- await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id)
- await Promise.allSettled([
- jobRunnerRpc(ctx.env, args.job_id).deleteRunner(),
- ])
+ await jobRunnerRpc(ctx.env, args.job_id).deleteRunner()
+ await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id)
return {
job_id: args.job_id,
deleted: true as const,📝 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.
| await requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) | |
| await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) | |
| await Promise.allSettled([ | |
| jobRunnerRpc(ctx.env, args.job_id).deleteRunner(), | |
| ]) | |
| await requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) | |
| await jobRunnerRpc(ctx.env, args.job_id).deleteRunner() | |
| await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/jobs/domain.ts` around lines 133 - 137,
The current flow removes the DB row before awaiting runner cleanup and uses
Promise.allSettled which hides failures; change the order to call and await
jobRunnerRpc(ctx.env, args.job_id).deleteRunner() first (letting any thrown
error propagate), and only after successful deletion call
removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id); remove the
Promise.allSettled wrapper so cleanup errors are not swallowed (requireJobRecord
can remain before cleanup to validate the row).
* Refine saved app integration guidance Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Harden saved app docs and validation Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Fix saved app update server code validation Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Restore strict saved app validation Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> * Remove low-signal saved app test Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
| }) | ||
| await this.syncAlarm() | ||
| return await this.getDetails() | ||
| } |
There was a problem hiding this comment.
Unused setEnabled and setKillSwitch methods on JobRunner
Low Severity
setEnabled and setKillSwitch are defined on JobRunnerBase and declared in the jobRunnerRpc type, but no capability handler or any other code ever calls them. All enable/disable/kill-switch changes go through updateJobRecord → syncJobRunnerFromDb → configure. These methods are dead code that increases the maintenance surface and may mislead future contributors into thinking they are part of an active code path.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e908ea. Configure here.
| existing.timezone !== nextRecord.timezone || | ||
| existing.enabled !== nextRecord.enabled, | ||
| } | ||
| } |
There was a problem hiding this comment.
updateJobRecord silently ignores killSwitchEnabled and historyLimit fields
Low Severity
updateJobRecord accepts JobUpdatePatch which includes killSwitchEnabled and historyLimit, but the function body only persists D1-column fields (name, serverCode, schedule, etc.) and silently drops those two fields. The capability handler in domain.ts compensates by passing them separately to syncJobRunnerFromDb, but the type signature of updateJobRecord is misleading — a future caller could reasonably assume those fields are persisted when they are not.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2e908ea. Configure here.
| return facetName === 'main' | ||
| ? baseClassName | ||
| : `${baseClassName}_${facetName.replaceAll(/[^a-zA-Z0-9_]/g, '_')}` | ||
| } |
There was a problem hiding this comment.
Exported buildFacetClassExportName from facet-runtime never imported externally
Low Severity
buildFacetClassExportName is exported from facet-runtime.ts but is never imported by any other module — it's only used internally by createFacetStartup. Meanwhile, a different function with the same name but a different signature (one argument vs two) exists in app-runner-facet-names.ts. The unnecessary export with a clashing name risks confusion and accidental misuse.
Reviewed by Cursor Bugbot for commit 2e908ea. Configure here.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/worker/src/mcp/capabilities/jobs/domain.ts (1)
182-186:⚠️ Potential issue | 🟠 MajorDelete the runner before deleting the D1 row.
This still removes the source-of-truth row first and hides
deleteRunner()failures withPromise.allSettled(). If cleanup fails, the capability returns success while leaving orphaned runner state that the jobs table can no longer repair.Suggested fix
const user = requireJobsUser(ctx) await requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) - await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) - await Promise.allSettled([ - jobRunnerRpc(ctx.env, args.job_id).deleteRunner(), - ]) + await jobRunnerRpc(ctx.env, args.job_id).deleteRunner() + await removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id) return { job_id: args.job_id, deleted: true as const,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/jobs/domain.ts` around lines 182 - 186, Currently the code removes the DB row before attempting runner cleanup and hides failures via Promise.allSettled; change the order so requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) still verifies the row, then await jobRunnerRpc(ctx.env, args.job_id).deleteRunner() (do not use Promise.allSettled) and let errors propagate or handle them explicitly so failures surface, and only after successful deletion call removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id); update the code paths around jobRunnerRpc(...).deleteRunner() to throw/return an error on failure instead of swallowing it.
🧹 Nitpick comments (1)
packages/worker/src/mcp/capabilities/apps/ui-save-app.ts (1)
89-134: Consider extracting duplicated serverCode validation in superRefine.The serverCode pattern check (lines 91-100 and 124-133) is identical in both branches. This could be consolidated after the conditional checks to reduce duplication.
♻️ Suggested consolidation
.superRefine((value, ctx) => { if (value.app_id !== undefined) { - if ( - value.serverCode != null && - !appServerCodeExportPattern.test(value.serverCode) - ) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: ['serverCode'], - message: 'serverCode must export class App extends DurableObject', - }) - } - return + // Update path: no required fields + } else { + // Create path: require title, description, clientCode + if (value.title === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['title'], + message: 'title is required when creating a saved app.', + }) + } + if (value.description === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['description'], + message: 'description is required when creating a saved app.', + }) + } + if (value.clientCode === undefined) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: ['clientCode'], + message: 'clientCode is required when creating a saved app.', + }) + } } - if (value.title === undefined) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: ['title'], - message: 'title is required when creating a saved app.', - }) - } - if (value.description === undefined) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: ['description'], - message: 'description is required when creating a saved app.', - }) - } - if (value.clientCode === undefined) { - ctx.addIssue({ - code: z.ZodIssueCode.custom, - path: ['clientCode'], - message: 'clientCode is required when creating a saved app.', - }) - } + // Common: validate serverCode pattern for both paths if ( value.serverCode != null && !appServerCodeExportPattern.test(value.serverCode) ) { ctx.addIssue({ code: z.ZodIssueCode.custom, path: ['serverCode'], message: 'serverCode must export class App extends DurableObject', }) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/apps/ui-save-app.ts` around lines 89 - 134, In superRefine, the serverCode pattern check is duplicated inside both the app_id branch and the create-branch; remove the duplicate by performing the app_id conditional (the early return) and the create-field presence checks (title, description, clientCode) first, then add a single serverCode validation that uses appServerCodeExportPattern and calls ctx.addIssue with the same code/path/message when serverCode is non-null and fails the test; update references to serverCode, appServerCodeExportPattern, ctx.addIssue, and the superRefine block accordingly.
🤖 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/jobs/schedule.ts`:
- Around line 20-29: assertValidJobCronExpression only checks the field count
and allows invalid values; update it to instantiate the Cron parser to validate
syntax early so bad expressions are rejected before persistence. In the
assertValidJobCronExpression function, after trimming and checking fields,
construct a new Cron(trimmed, { paused: true }) (or equivalent parser used by
computeNextJobRunAt) to trigger parser validation and throw on invalid values,
then return the normalized fields; reference assertValidJobCronExpression and
the Cron parser (as used by computeNextJobRunAt) when making the change.
In `@packages/worker/src/jobs/service.ts`:
- Around line 115-122: The update flow currently ignores the boolean result from
updateJob and treats a false (no row updated) as success; modify the callers (in
service functions that call getJobById and then updateJob) to check the return
value of updateJob(jobDb, userId, jobId, ...) and if it returns false throw or
return a failure (e.g., throw NotFoundError or Abort) instead of proceeding to
publish/commit; apply the same check and error path for the other occurrence
that updates and then publishes/rolls back (the block around the second
updateJob call) so concurrent deletes produce a proper error and rollback rather
than a false-success.
In `@packages/worker/src/mcp/capabilities/jobs/shared.ts`:
- Around line 87-112: Update the Zod object schemas to reject unknown keys by
chaining .strict() onto each schema definition: add .strict() to
jobCreateInputSchema, jobUpdatePatchSchema, jobUpdateInputSchema, and
jobServerExecInputSchema (i.e., convert the current z.object({...}) instances
for those symbols into z.object({...}).strict()) so that requests with unknown
or misspelled fields fail-fast instead of being silently dropped.
---
Duplicate comments:
In `@packages/worker/src/mcp/capabilities/jobs/domain.ts`:
- Around line 182-186: Currently the code removes the DB row before attempting
runner cleanup and hides failures via Promise.allSettled; change the order so
requireJobRecord(ctx.env.APP_DB, user.userId, args.job_id) still verifies the
row, then await jobRunnerRpc(ctx.env, args.job_id).deleteRunner() (do not use
Promise.allSettled) and let errors propagate or handle them explicitly so
failures surface, and only after successful deletion call
removeJobRecord(ctx.env.APP_DB, user.userId, args.job_id); update the code paths
around jobRunnerRpc(...).deleteRunner() to throw/return an error on failure
instead of swallowing it.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/apps/ui-save-app.ts`:
- Around line 89-134: In superRefine, the serverCode pattern check is duplicated
inside both the app_id branch and the create-branch; remove the duplicate by
performing the app_id conditional (the early return) and the create-field
presence checks (title, description, clientCode) first, then add a single
serverCode validation that uses appServerCodeExportPattern and calls
ctx.addIssue with the same code/path/message when serverCode is non-null and
fails the test; update references to serverCode, appServerCodeExportPattern,
ctx.addIssue, and the superRefine block accordingly.
🪄 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: 6a992aa2-16a3-4ec5-b4ec-10922ce2c0cd
📒 Files selected for processing (25)
docs/guides/README.mddocs/guides/generated-ui-oauth.mddocs/guides/integration-backed-app-happy-path.mddocs/guides/integration-bootstrap.mddocs/guides/oauth.mddocs/use/examples/saved-app-counter.mddocs/use/jobs.mddocs/use/saved-app-backends.mddocs/use/skills-and-apps.mdpackages/worker/src/jobs/exec-worker.tspackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/jobs-capabilities.workers.test.tspackages/worker/src/jobs/schedule.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/app-runner.tspackages/worker/src/mcp/capabilities/apps/app-server-exec.tspackages/worker/src/mcp/capabilities/apps/ui-save-app.tspackages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tspackages/worker/src/mcp/capabilities/coding/kody-official-guide.tspackages/worker/src/mcp/capabilities/jobs/domain.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/facet-runtime.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/observability.workers.test.tspackages/worker/wrangler.jsonc
✅ Files skipped from review due to trivial changes (3)
- docs/guides/README.md
- docs/guides/integration-backed-app-happy-path.md
- packages/worker/src/mcp/capabilities/apps/app-server-exec.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.ts
- packages/worker/src/jobs/exec-worker.ts
- docs/use/jobs.md
- packages/worker/src/jobs/job-runner.ts
| export function assertValidJobCronExpression(expression: string) { | ||
| const trimmed = expression.trim() | ||
| const fields = trimmed.split(cronFieldPattern).filter(Boolean) | ||
| if (fields.length !== 5) { | ||
| throw new Error( | ||
| 'Cron schedules must use standard 5-field syntax: minute hour day-of-month month day-of-week.', | ||
| ) | ||
| } | ||
| return fields.join(' ') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/worker/src/jobs/schedule.ts | head -50Repository: kentcdodds/kody
Length of output: 1791
🏁 Script executed:
rg "assertValidJobCronExpression" packages/worker/src/jobs/ -B 2 -A 2Repository: kentcdodds/kody
Length of output: 1498
🏁 Script executed:
rg "croner\|cron\|Cron" packages/worker/src/jobs/schedule.ts -B 1 -A 1 | head -40Repository: kentcdodds/kody
Length of output: 41
🏁 Script executed:
cat packages/worker/package.json | grep -A 5 -B 5 "cron"Repository: kentcdodds/kody
Length of output: 368
🏁 Script executed:
rg "assertValidJobCronExpression|computeNextJobRunAt" packages/worker/src/ -B 3 -A 3Repository: kentcdodds/kody
Length of output: 5855
🏁 Script executed:
rg "normalizeJobSchedule" packages/worker/src/ -B 3 -A 3Repository: kentcdodds/kody
Length of output: 3966
🏁 Script executed:
rg "writeConfig\|writeRunnerStatus" packages/worker/src/jobs/job-runner.ts -B 5 -A 2Repository: kentcdodds/kody
Length of output: 41
🏁 Script executed:
cat -n packages/worker/src/jobs/schedule.node.test.tsRepository: kentcdodds/kody
Length of output: 2013
🏁 Script executed:
rg "assertValidJobCronExpression|computeNextJobRunAt" packages/worker/src/jobs/schedule.node.test.ts -A 5 -B 2Repository: kentcdodds/kody
Length of output: 639
🏁 Script executed:
rg "createJob|updateJob" packages/worker/src/jobs/service.ts -A 20 | head -80Repository: kentcdodds/kody
Length of output: 2229
🏁 Script executed:
cat -n packages/worker/src/jobs/schedule.ts | tail -30Repository: kentcdodds/kody
Length of output: 1148
🏁 Script executed:
cd packages/worker && npm list cronerRepository: kentcdodds/kody
Length of output: 101
🏁 Script executed:
cat > /tmp/test_cron.mjs << 'EOF'
import { Cron } from 'croner';
// Test invalid expressions
const testCases = [
'61 * * * *', // minute out of range
'foo * * * *', // invalid token
'0 0 0 0 0', // valid syntax but weird values
];
for (const expr of testCases) {
try {
new Cron(expr, { paused: true });
console.log(`✓ PASSED: "${expr}"`);
} catch (e) {
console.log(`✗ FAILED: "${expr}" - ${e.message}`);
}
}
EOF
cd packages/worker && node /tmp/test_cron.mjsRepository: kentcdodds/kody
Length of output: 970
🏁 Script executed:
cat -n packages/worker/src/jobs/schedule.ts | sed -n '51,90p'Repository: kentcdodds/kody
Length of output: 1636
🏁 Script executed:
cat > /tmp/check_croner.js << 'EOF'
// Check croner documentation/library behavior
// We'll examine what the library does with invalid expressions
const invalidCases = [
'61 * * * *', // minute out of range
'foo * * * *', // non-numeric
'* * * * * *', // 6 fields instead of 5
];
console.log('Testing invalid cron expressions with croner pattern:');
console.log('If these follow the 5-field pattern, they will pass assertValidJobCronExpression:');
for (const expr of invalidCases) {
const cronFieldPattern = /\s+/;
const fields = expr.trim().split(cronFieldPattern).filter(Boolean);
const passes = fields.length === 5;
console.log(` "${expr}" -> ${fields.length} fields -> ${passes ? 'PASS' : 'FAIL'}`);
}
EOF
node /tmp/check_croner.jsRepository: kentcdodds/kody
Length of output: 301
assertValidJobCronExpression only validates field count, not field values.
Invalid expressions like 61 * * * * or foo * * * * will pass this helper and be persisted to the database. The actual validation deferred to the Cron parser instantiation in computeNextJobRunAt() happens after the schedule is already stored. Validate cron syntax earlier by instantiating the Cron parser within assertValidJobCronExpression to fail before persistence:
export function assertValidJobCronExpression(expression: string) {
const trimmed = expression.trim()
const fields = trimmed.split(cronFieldPattern).filter(Boolean)
if (fields.length !== 5) {
throw new Error(
'Cron schedules must use standard 5-field syntax: minute hour day-of-month month day-of-week.',
)
}
// Validate syntax early by instantiating the parser
new Cron(trimmed, { paused: true })
return fields.join(' ')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/jobs/schedule.ts` around lines 20 - 29,
assertValidJobCronExpression only checks the field count and allows invalid
values; update it to instantiate the Cron parser to validate syntax early so bad
expressions are rejected before persistence. In the assertValidJobCronExpression
function, after trimming and checking fields, construct a new Cron(trimmed, {
paused: true }) (or equivalent parser used by computeNextJobRunAt) to trigger
parser validation and throw on invalid values, then return the normalized
fields; reference assertValidJobCronExpression and the Cron parser (as used by
computeNextJobRunAt) when making the change.
| await updateJob(input.db, input.userId, input.jobId, { | ||
| name: nextRecord.name, | ||
| serverCode: nextRecord.serverCode, | ||
| serverCodeId: nextRecord.serverCodeId, | ||
| schedule: nextRecord.schedule, | ||
| timezone: nextRecord.timezone, | ||
| enabled: nextRecord.enabled, | ||
| }) |
There was a problem hiding this comment.
Check updateJob(...) before reporting success.
packages/worker/src/jobs/repo.ts returns false when the row was not updated, but both helpers ignore that and still act as if the mutation succeeded. A concurrent delete between getJobById() and updateJob() becomes a false-success update/rollback.
Suggested fix
- await updateJob(input.db, input.userId, input.jobId, {
+ const updated = await updateJob(input.db, input.userId, input.jobId, {
name: nextRecord.name,
serverCode: nextRecord.serverCode,
serverCodeId: nextRecord.serverCodeId,
schedule: nextRecord.schedule,
timezone: nextRecord.timezone,
enabled: nextRecord.enabled,
})
+ if (!updated) {
+ throw new Error(`Job "${input.jobId}" was not updated.`)
+ }
@@
-export async function restoreJobRecord(db: D1Database, record: JobRecord) {
- await updateJob(db, record.userId, record.id, {
+export async function restoreJobRecord(db: D1Database, record: JobRecord) {
+ const restored = await updateJob(db, record.userId, record.id, {
name: record.name,
serverCode: record.serverCode,
serverCodeId: record.serverCodeId,
schedule: record.schedule,
timezone: record.timezone,
enabled: record.enabled,
})
+ if (!restored) {
+ throw new Error(`Job "${record.id}" could not be restored.`)
+ }
}Also applies to: 159-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/jobs/service.ts` around lines 115 - 122, The update flow
currently ignores the boolean result from updateJob and treats a false (no row
updated) as success; modify the callers (in service functions that call
getJobById and then updateJob) to check the return value of updateJob(jobDb,
userId, jobId, ...) and if it returns false throw or return a failure (e.g.,
throw NotFoundError or Abort) instead of proceeding to publish/commit; apply the
same check and error path for the other occurrence that updates and then
publishes/rolls back (the block around the second updateJob call) so concurrent
deletes produce a proper error and rollback rather than a false-success.
| export const jobCreateInputSchema = z.object({ | ||
| name: z.string().min(1), | ||
| serverCode: z | ||
| .string() | ||
| .min(1) | ||
| .describe( | ||
| 'Durable Object source that must export `class Job extends DurableObject` with an async `run()` method.', | ||
| ), | ||
| schedule: jobScheduleSchema, | ||
| timezone: z | ||
| .string() | ||
| .nullable() | ||
| .optional() | ||
| .describe('Optional IANA timezone. Defaults to America/Denver.'), | ||
| enabled: z.boolean().optional(), | ||
| }) | ||
|
|
||
| export const jobUpdatePatchSchema = z.object({ | ||
| name: z.string().min(1).optional(), | ||
| serverCode: z.string().min(1).optional(), | ||
| schedule: jobScheduleSchema.optional(), | ||
| timezone: z.string().nullable().optional(), | ||
| enabled: z.boolean().optional(), | ||
| kill_switch_enabled: z.boolean().optional(), | ||
| history_limit: z.number().int().min(1).max(500).optional(), | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the current state of the file to confirm the schemas don't use .strict()
cat -n packages/worker/src/mcp/capabilities/jobs/shared.ts | head -150Repository: kentcdodds/kody
Length of output: 4781
🏁 Script executed:
# Let's also search for any usage of .strict() in this file and the project
rg "\.strict\(\)" packages/worker/src/mcp/capabilities/jobs/Repository: kentcdodds/kody
Length of output: 188
🏁 Script executed:
# Check if there are Zod usage patterns in other similar files
fd -e ts -path "*/mcp/capabilities/*" | head -10Repository: kentcdodds/kody
Length of output: 227
🌐 Web query:
Zod object schema default behavior unknown keys strip
💡 Result:
By default, Zod object schemas (created with z.object) strip unknown keys from the parsed output. They accept input objects with extra keys (validating only the defined ones) but remove any unrecognized keys from the result. Example from official docs: const Dog = z.object({ name: z.string, age: z.number.optional, }); Dog.parse({ name: "Yeller", extraKey: true }); // => { name: "Yeller" } Alternatives: - z.strictObject: Throws on unknown keys. - z.object({...}).passthrough or .catchall(z.unknown): Passes through unknown keys. - z.looseObject: Allows and preserves unknown keys (with catchall). This behavior has been default since Zod 2 (2020) and remains in Zod 4 (stable as of 2026).
Citations:
- 1: https://zod.dev/api?id=keyof
- 2: https://v4.zod.dev/api?id=omit
- 3: https://zod.dev/api
- 4: https://www.zod.dev/api?id=objects
🏁 Script executed:
# Let's check if these schemas are used in actual request processing
rg "jobCreateInputSchema|jobUpdatePatchSchema|jobUpdateInputSchema|jobServerExecInputSchema" packages/worker/src/mcp/capabilities/jobs/ -A 3Repository: kentcdodds/kody
Length of output: 3182
Add .strict() to capability request schemas to prevent silent field drops.
Zod silently strips unknown keys from objects by default. With the current schemas, typos like historyLimit instead of history_limit or other unrecognized fields are silently dropped, causing capability calls to succeed without applying the intended changes. This is especially problematic for jobUpdatePatchSchema, where a caller won't know if their requested field update was ignored due to a typo. Add .strict() to jobCreateInputSchema, jobUpdatePatchSchema, jobUpdateInputSchema, and jobServerExecInputSchema to fail fast on unknown fields instead.
Note: jobScheduleSchema already uses .strict() correctly on its union members.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/jobs/shared.ts` around lines 87 - 112,
Update the Zod object schemas to reject unknown keys by chaining .strict() onto
each schema definition: add .strict() to jobCreateInputSchema,
jobUpdatePatchSchema, jobUpdateInputSchema, and jobServerExecInputSchema (i.e.,
convert the current z.object({...}) instances for those symbols into
z.object({...}).strict()) so that requests with unknown or misspelled fields
fail-fast instead of being silently dropped.
| async __kody_exportStorage() { | ||
| const entries = [] | ||
| for (const [key, value] of await this.ctx.storage.list()) { | ||
| entries.push({ key, value }) | ||
| } | ||
| return { | ||
| entries, | ||
| estimatedBytes: this.ctx.storage.sql.databaseSize, | ||
| } | ||
| } |
There was a problem hiding this comment.
Storage export should stay paginated.
__kody_exportStorage() now loads the entire DO store into memory. Because this runtime is shared by saved apps and jobs, one large facet can turn export into a timeout or OOM. packages/worker/src/mcp/app-runner.ts:99-170 already had bounded pagination; please keep that shape here instead of returning the whole store.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
There are 6 total unresolved issues (including 3 from previous reviews).
Reviewed by Cursor Bugbot for commit f8b012f. Configure here.
|
|
||
| export function readJson<TPayload>(request: Request): Promise<TPayload | null> { | ||
| return request.json().catch(() => null) as Promise<TPayload | null> | ||
| } |
There was a problem hiding this comment.
Exported readJson from facet-runtime is never imported
Low Severity
readJson is exported from facet-runtime.ts but never imported by any other module. The only import from this file is in job-runner.ts, which does not include readJson. Meanwhile, app-runner.ts defines its own local readJson with near-identical logic rather than importing the shared one. This is a dead export that the PR description's "remove dead shared facet-runtime exec/error helper exports" follow-up intended to clean up.
Reviewed by Cursor Bugbot for commit f8b012f. Configure here.
| 'Content-Type': 'application/json; charset=utf-8', | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Duplicated jsonResponse helper across app-runner and facet-runtime
Low Severity
jsonResponse is identically defined in both app-runner.ts (private) and facet-runtime.ts (exported). The PR description states the saved-app and job runners share facet-runtime.ts for "common error formatting" and shared helpers, yet app-runner.ts still uses its own copy rather than importing the shared one. A future fix to one copy risks being missed in the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f8b012f. Configure here.
| return this.env.APP.callAppRpc(methodName, args) | ||
| }, | ||
| } | ||
| ${input.code} |
There was a problem hiding this comment.
Exec worker missing implicit return for user code
Medium Severity
The createSavedAppExecWorkerModule injects user code into the run method body as raw statements. If the user's snippet contains only an expression (e.g. await app.call('readState')) without an explicit return, the result is silently discarded and undefined is returned. The job exec worker has the same pattern. The docs show examples with explicit return, but the code field description says "function body," which many callers may interpret as returning the last expression's value.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f8b012f. Configure here.
2 participants
Summary
JobRunnersupervisor Durable Object that loadsclass Job extends DurableObjectfacets via the dynamic worker loaderjob_*capabilities, docs, and targeted tests for cron/interval scheduling, run-now, enable/disable, kill switch, history, and server code rotationjob_server_execcontract with the merged saved-app fix from Fix saved app facet exec and partial update semantics #165 by compiling snippets in a throwaway Dynamic Worker and exposing an explicitjob.call(methodName, ...args)bridgeRebase note
main, which now includes both Fix saved app facet exec and partial update semantics #165 and Kody app backend patterns #166.Design notes
Architecture
This change follows the same three-layer facet pattern introduced for saved apps:
JobRunner) keyed byjob_idclass Job extends DurableObjectglobalOutbound: nullJobRunnerowns the control plane and observability state in its own SQLite database, while the dynamic job facet gets its own isolated SQLite storage for incremental state. The supervisor injects a scopedKODYbridge so job code can access values, secrets, connectors, skills, and host-gateway fetch without receiving unrestricted outbound access.Shared primitives reused from the apps work
The saved-app and job runners share
packages/worker/src/mcp/facet-runtime.tsfor:Scheduling model
Jobs are persisted in D1 (
jobstable) and mirrored into oneJobRunnerDO per job. Supported schedules are:{ cron: "0 8 * * *" }{ intervalMs: 3600000 }The supervisor stores
next_run_atin its own SQLite DB and re-arms the alarm after each run.job_run_nowexecutes immediately but preserves the normal alarm cadence instead of shifting the stored schedule. Metadata-only edits also preserve the pending scheduled fire time, while schedule/timezone/enable transitions recompute it.Observability and control
JobRunnertracks:The MCP surface exposes
job_history,job_storage_reset,job_server_exec,job_enable,job_disable, andjob_run_nowin addition to CRUD.job_server_execcontractjob_server_execmirrors the saved-app-side fix from #165: it does not eval source inside the running facet. Instead, Kody compiles the provided snippet into a throwaway Dynamic Worker and binds an explicit RPC bridge for the scheduled job facet.Inside the snippet body you can access:
job— explicit bridge withjob.call(methodName, ...args)for invoking public RPC methods on the saved jobJobclassparams— optional JSON input passed through the capabilityThat keeps the boundary explicit:
new Function(...)inside the job facetJobclass actually exposesjob_storage_exportFollow-up fixes kept after review
executeafterui_save_app__kody_*method names in the job exec bridgejob_deleteresilient in the current workers-unit runtime instead of hard-failing on facet cleanup internalsTradeoffs
cronerwith timezone-aware next-run computation.run_nowcalls intentionally do not rewrite the stored cadence.Validation
npm installnpm run typechecknpx vitest run --project node-unit packages/worker/src/jobs/schedule.node.test.tsnpx vitest run --project workers-unit packages/worker/src/jobs/jobs-capabilities.workers.test.ts packages/worker/src/mcp/capabilities/build-capability-registry.workers.test.tsnpm run validatenpm run testandnpm run test:e2e:runSummary by CodeRabbit