Repository navigation
Add one-off job scheduling capability - #237
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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 49 minutes and 13 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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces standalone job scheduling via MCP capabilities ( Changes
Sequence DiagramsequenceDiagram
participant Client as Client/Agent
participant MCP as MCP Handler
participant JobService as Jobs Service
participant JobManager as Job Manager
participant RepoSession as Repo Session
participant Storage as Storage
Client->>MCP: job_schedule(name, code, schedule, params)
activate MCP
MCP->>JobService: createScheduledJobFromArgs(args)
activate JobService
JobService->>JobManager: createJob(JobCreateInput)
activate JobManager
JobManager->>Storage: Write job record with schedule
JobManager-->>JobService: JobView
deactivate JobManager
JobService->>JobManager: syncJobManagerAlarm(userId)
activate JobManager
JobManager-->>JobService: (alarm synced)
deactivate JobManager
JobService-->>MCP: buildJobScheduleOutput(JobView)
deactivate JobService
MCP-->>Client: { job_id, name, schedule, next_run_at }
deactivate MCP
Note over Client,Storage: At scheduled time...
Client->>JobService: executeJobOnce(jobId)
activate JobService
JobService->>RepoSession: readFile(kody.json)
activate RepoSession
RepoSession-->>JobService: manifest (kind, entrypoint)
deactivate RepoSession
JobService->>JobService: detectStandaloneJob(manifest)
JobService->>JobService: executeBundledJobModule(repoContext, entrypoint)
activate JobService
JobService->>Storage: Read/write via storageId=job:jobId
Storage-->>JobService: (storage bound)
JobService-->>JobService: ExecuteResult
deactivate JobService
JobService-->>Client: { ok: true, result, logs }
deactivate JobService
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
🔎 Preview deployed: https://kody-pr-237.kentcdodds.workers.dev Worker: Mocks:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 385f880. Configure here.
| onceScheduleSchema, | ||
| intervalScheduleSchema, | ||
| cronScheduleSchema, | ||
| ]) |
There was a problem hiding this comment.
Redundant identical schema creates divergence risk
Low Severity
scheduledJobSummarySchema is defined as an identical copy of scheduledJobScheduleSchema — both are z.discriminatedUnion('type', [...]) with the exact same three variants. The summary schema is only used once in jobScheduleOutputSchema. Having two separate-but-identical schema objects risks them silently diverging if a future change updates one but not the other.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 385f880. Configure here.
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: 1
🧹 Nitpick comments (4)
packages/worker/src/mcp/server-instructions.ts (1)
50-50: Minor: run-on sentence.Consider splitting for readability — the clause "Package jobs are owned by packages, ad hoc jobs can be scheduled with
job_schedule, and package apps are optional package surfaces" packs three unrelated facts into one comma-joined run-on.✏️ Suggested wording
-- Cross-package imports use specifiers such as \`kody:`@my-package/export-name`\`. Package jobs are owned by packages, ad hoc jobs can be scheduled with \`job_schedule\`, and package apps are optional package surfaces. +- Cross-package imports use specifiers such as \`kody:`@my-package/export-name`\`. Package jobs are owned by packages; ad hoc jobs can be scheduled with \`job_schedule\`. Package apps are optional package surfaces.🤖 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 50, The sentence mentioning cross-package imports and package jobs is a run-on; split it into clearer sentences and separate unrelated facts: keep the cross-package import explanation using the specifier `kody:`@my-package/export-name`` as one sentence, then create a second sentence stating that package jobs are owned by packages and that ad hoc jobs can be scheduled with `job_schedule`, and optionally a third sentence calling out that package apps are optional package surfaces; update the string in server-instructions text accordingly to improve readability.docs/use/execute.md (1)
77-81: Storage ownership phrasing: minor awkwardness."bound storage is execute-, app-, package-, or job-owned durable state" is a bit dense. Consider rephrasing to something like "bound storage is durable state owned by an execute call, an app, a package, or a job." Non-blocking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/use/execute.md` around lines 77 - 81, Rewrite the awkward sentence "bound storage is execute-, app-, package-, or job-owned durable state" to a clearer phrasing; replace that exact phrase in the docs with something like "bound storage is durable state owned by an execute call, an app, a package, or a job" (or a similar concise wording) so the meaning is clearer and easier to read.packages/worker/src/mcp/capabilities/jobs/shared.ts (1)
65-75: Duplicate discriminated union definitions.
scheduledJobScheduleSchemaandscheduledJobSummarySchemaare identical. Consider aliasing the second to the first to reduce duplication and keep them in sync.♻️ Suggested cleanup
export const scheduledJobScheduleSchema = z.discriminatedUnion('type', [ onceScheduleSchema, intervalScheduleSchema, cronScheduleSchema, ]) -const scheduledJobSummarySchema = z.discriminatedUnion('type', [ - onceScheduleSchema, - intervalScheduleSchema, - cronScheduleSchema, -]) +const scheduledJobSummarySchema = scheduledJobScheduleSchema🤖 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 65 - 75, scheduledJobScheduleSchema and scheduledJobSummarySchema are duplicated discriminated unions; replace the duplicate definition by aliasing scheduledJobSummarySchema to scheduledJobScheduleSchema so they stay in sync—locate the two constants (scheduledJobScheduleSchema, scheduledJobSummarySchema) and change the latter to a direct reference to the former instead of redefining the union.packages/worker/src/jobs/service.ts (1)
758-802: Misleading variable nameentrypointrefers to the manifest file, not the entrypoint.In the standalone branch,
entrypointon line 758 holds the contents ofkody.json(the manifest), whilemoduleFileon line 791 holds the actual entrypoint. This mirrors awkward naming that already existed in the package branch but is worth tightening up in the new code. Consider renaming tomanifestFilefor clarity — it also makes the error message on line 765 read more naturally.♻️ Suggested rename
- const entrypoint = await sessionClient.readFile({ + const manifestFile = await sessionClient.readFile({ sessionId: session.id, userId: input.callerContext.user.userId, path: manifestPath, }) - if (!entrypoint.content) { + if (!manifestFile.content) { return { error: `Job manifest "${manifestPath}" was not found in repo session.`, result: null, logs: bypassLogs, } } let manifest: ReturnType<typeof parseRepoManifest> try { manifest = parseRepoManifest({ - content: entrypoint.content, + content: manifestFile.content, manifestPath, })🤖 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 758 - 802, Rename the local variable entrypoint to manifestFile to accurately reflect that it contains the manifest contents; update all usages (the sessionClient.readFile call result, the content checks, and the parseRepoManifest invocation) to use manifestFile, and adjust the related error message that currently says `Job manifest "${manifestPath}" was not found in repo session.` if needed to read naturally with manifestFile; ensure subsequent logic that calls parseRepoManifest, getManifestEntrypointPath, and the later sessionClient.readFile for the actual module file (moduleFile) remains unchanged.
🤖 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/service.node.test.ts`:
- Around line 754-812: The test calls createJob but never sets up the repo
mocks, causing repoMockModule.ensureEntitySource / syncArtifactSourceSnapshot to
be undefined when run in isolation; add a call to mockRepoPersistence() before
createJob in this test so the repoMockModule implementations (ensureEntitySource
returning an ensuredSource with an id) are registered; target the test block
using createJob and ensure mockRepoPersistence() is invoked earlier in that test
so createJob's access to ensuredSource.id won't throw.
---
Nitpick comments:
In `@docs/use/execute.md`:
- Around line 77-81: Rewrite the awkward sentence "bound storage is execute-,
app-, package-, or job-owned durable state" to a clearer phrasing; replace that
exact phrase in the docs with something like "bound storage is durable state
owned by an execute call, an app, a package, or a job" (or a similar concise
wording) so the meaning is clearer and easier to read.
In `@packages/worker/src/jobs/service.ts`:
- Around line 758-802: Rename the local variable entrypoint to manifestFile to
accurately reflect that it contains the manifest contents; update all usages
(the sessionClient.readFile call result, the content checks, and the
parseRepoManifest invocation) to use manifestFile, and adjust the related error
message that currently says `Job manifest "${manifestPath}" was not found in
repo session.` if needed to read naturally with manifestFile; ensure subsequent
logic that calls parseRepoManifest, getManifestEntrypointPath, and the later
sessionClient.readFile for the actual module file (moduleFile) remains
unchanged.
In `@packages/worker/src/mcp/capabilities/jobs/shared.ts`:
- Around line 65-75: scheduledJobScheduleSchema and scheduledJobSummarySchema
are duplicated discriminated unions; replace the duplicate definition by
aliasing scheduledJobSummarySchema to scheduledJobScheduleSchema so they stay in
sync—locate the two constants (scheduledJobScheduleSchema,
scheduledJobSummarySchema) and change the latter to a direct reference to the
former instead of redefining the union.
In `@packages/worker/src/mcp/server-instructions.ts`:
- Line 50: The sentence mentioning cross-package imports and package jobs is a
run-on; split it into clearer sentences and separate unrelated facts: keep the
cross-package import explanation using the specifier
`kody:`@my-package/export-name`` as one sentence, then create a second sentence
stating that package jobs are owned by packages and that ad hoc jobs can be
scheduled with `job_schedule`, and optionally a third sentence calling out that
package apps are optional package surfaces; update the string in
server-instructions text accordingly to improve readability.
🪄 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 Plus
Run ID: 6baa2953-1662-427a-9d7e-c058da47ad80
📒 Files selected for processing (14)
docs/use/execute.mddocs/use/first-steps.mddocs/use/packages.mdpackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/mcp/capabilities/builtin-domains.tspackages/worker/src/mcp/capabilities/jobs/domain.tspackages/worker/src/mcp/capabilities/jobs/job-schedule-once.tspackages/worker/src/mcp/capabilities/jobs/job-schedule.node.test.tspackages/worker/src/mcp/capabilities/jobs/job-schedule.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.node.test.tspackages/worker/src/mcp/capabilities/meta/meta-list-capabilities.tspackages/worker/src/mcp/server-instructions.ts
| test('executeJobOnce runs repo-backed one-off jobs from kody.json manifests', async () => { | ||
| const db = createDatabase() | ||
| const env = { | ||
| APP_DB: db, | ||
| LOADER: {} as WorkerLoader, | ||
| REPO_SESSION: {} as DurableObjectNamespace, | ||
| STORAGE_RUNNER: { | ||
| idFromName(name: string) { | ||
| return name as unknown as DurableObjectId | ||
| }, | ||
| get() { | ||
| return { | ||
| getValue: async () => ({ key: 'count', value: 2 }), | ||
| setValue: async () => ({ ok: true, key: 'count' }), | ||
| deleteValue: async () => ({ ok: true, key: 'count', deleted: true }), | ||
| clearStorage: async () => ({ ok: true }), | ||
| listValues: async () => ({ | ||
| entries: [], | ||
| estimatedBytes: 0, | ||
| truncated: false, | ||
| nextStartAfter: null, | ||
| pageSize: 250, | ||
| }), | ||
| exportStorage: async () => ({ | ||
| entries: [], | ||
| estimatedBytes: 0, | ||
| truncated: false, | ||
| nextStartAfter: null, | ||
| pageSize: 250, | ||
| }), | ||
| sqlQuery: async () => ({ | ||
| columns: ['value'], | ||
| rows: [{ value: 2 }], | ||
| rowCount: 1, | ||
| rowsRead: 1, | ||
| rowsWritten: 0, | ||
| }), | ||
| } | ||
| }, | ||
| }, | ||
| } as unknown as Env | ||
| mockRepoPersistence() | ||
| const callerContext = createBaseCallerContext() | ||
|
|
||
| const jobView = await createJob({ | ||
| env, | ||
| callerContext, | ||
| body: { | ||
| name: 'Capability-created one-off job', | ||
| code: 'export default async () => ({ ok: true, adHoc: true })', | ||
| params: { | ||
| step: 'lights-off', | ||
| }, | ||
| schedule: { | ||
| type: 'once', | ||
| runAt: '2026-04-17T15:00:00Z', | ||
| }, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Missing mockRepoPersistence() before createJob — test relies on state leaked from the previous test.
Unlike the adjacent test on line 594 (which calls mockRepoPersistence() at line 635 before createJob), this new test invokes createJob at line 798 without first configuring repoMockModule.ensureEntitySource / syncArtifactSourceSnapshot. Currently it only works because the mockImplementation set by an earlier test run is still in place (vi.restoreAllMocks() in afterEach does not reset implementations on non-spy vi.fn() mocks). Running this test in isolation, or reordering tests, will break it because ensureEntitySource would return undefined and ensuredSource.id in createJob would throw.
🛠️ Suggested fix
} as unknown as Env
+ mockRepoPersistence()
const callerContext = createBaseCallerContext()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/jobs/service.node.test.ts` around lines 754 - 812, The
test calls createJob but never sets up the repo mocks, causing
repoMockModule.ensureEntitySource / syncArtifactSourceSnapshot to be undefined
when run in isolation; add a call to mockRepoPersistence() before createJob in
this test so the repoMockModule implementations (ensureEntitySource returning an
ensuredSource with an id) are registered; target the test block using createJob
and ensure mockRepoPersistence() is invoked earlier in that test so createJob's
access to ensuredSource.id won't throw.
2 participants
Summary
job_schedulecapability for scheduling standalone repo-backed jobs without creating a saved package first, including one-off, interval, and cron schedulesjob_schedule_onceas a compatibility wrapper for one-off schedules while routing scheduling logic through shared helperskody.jsonjob sources run correctly while package-owned jobs keep using package manifests, and deduplicate the shared bundle/execute pathTesting
npm run test -- packages/worker/src/mcp/capabilities/jobs/job-schedule.node.test.ts packages/worker/src/jobs/service.node.test.tsnpx oxlint packages/worker/src/mcp/capabilities/jobs/shared.ts packages/worker/src/mcp/capabilities/jobs/job-schedule.ts packages/worker/src/mcp/capabilities/jobs/job-schedule-once.ts packages/worker/src/mcp/capabilities/jobs/job-schedule.node.test.ts packages/worker/src/jobs/service.ts✅ Validateand🔎 Previewruns forcursor/capability-one-off-jobs-4bb0now passSummary by CodeRabbit
Release Notes
New Features
job_scheduleandjob_schedule_oncecapabilities to schedule repo-backed jobs directly without creating saved packages.Documentation