Repository navigation
Unify jobs around codemode runtime - #171
Merged
Merged
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 6 minutes and 49 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 Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughJobs now run solely via codemode and use a durable per-job storage bucket identified by a stable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Service as Job Service
participant RunCM as runCodemodeWithRegistry
participant StorageDO as StorageRunner DO
participant Executor as Codemode Executor
User->>Service: request run (jobId, params)
Service->>Service: resolve job (code, storageId)
Service->>RunCM: runCodemodeWithRegistry(code, params, options { executorExports, storageTools? })
RunCM->>Executor: initialize executor with merged tools & helperPrelude
alt storageTools present
Executor->>StorageDO: storage RPC (get/set/list/sql) via storageTools
StorageDO-->>Executor: storage/sql result
end
Executor-->>RunCM: execution result
RunCM-->>Service: outcome
Service-->>User: final response
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 |
kentcdodds
marked this pull request as ready for review
April 14, 2026 20:07
Contributor
|
🔎 Preview deployed: https://kody-pr-171.kentcdodds.workers.dev Worker: Mocks:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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/jobs/service.ts (1)
499-505:⚠️ Potential issue | 🟠 MajorLet
executeJobOnce()own the missing-context failure path.
executeJobOnce()already convertsnullcaller context into a structured execution error, but these new pre-sync calls happen first. For facet jobs whosecaller_context_jsonis missing or unparseable,runJobNow()/runDueJobsForUser()now throw before that failure can be recorded.💡 Guard the syncs and keep the existing structured failure behavior
- if (hasFacetState(row.record)) { + if (hasFacetState(row.record) && activeCallerContext !== null) { await syncRunnerForJob({ env: input.env, job: row.record, callerContext: activeCallerContext, }) }- if (hasFacetState(job)) { + if (hasFacetState(job) && callerContext !== null) { await syncRunnerForJob({ env: input.env, job, callerContext, }) }Also applies to: 562-568
🤖 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 499 - 505, The pre-sync call to syncRunnerForJob should be guarded so missing/unparseable caller context doesn't cause runJobNow()/runDueJobsForUser to throw before executeJobOnce can record the structured execution error; update the calls that use hasFacetState(row.record) and syncRunnerForJob(...) to first verify the activeCallerContext (the parsed caller context used elsewhere) is non-null/valid and only then invoke syncRunnerForJob, otherwise skip the pre-sync and allow executeJobOnce (the existing job execution path) to handle and surface the execution error; apply the same guard to the other similar block that triggers syncRunnerForJob in runDueJobsForUser/runJobNow.
🧹 Nitpick comments (1)
packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts (1)
10-143: Close the in-memory SQLite handle explicitly in this test.
DatabaseSyncis created at Line 10 but never closed. Wrapping the body intry/finallywithdb.close()will prevent potential open-handle leaks in test runs.♻️ Suggested update
test('0020 jobs migration rewrites legacy rows into the final codemode-first shape', async () => { const db = new DatabaseSync(':memory:') - db.exec(await readMigration('0018-jobs.sql')) - db.exec(await readMigration('0019-jobs-constraints.sql')) + try { + db.exec(await readMigration('0018-jobs.sql')) + db.exec(await readMigration('0019-jobs-constraints.sql')) - const insertJob = db.prepare(` + const insertJob = db.prepare(` ... - `) + `) - insertJob.run(...) - insertJob.run(...) + insertJob.run(...) + insertJob.run(...) - db.exec(await readMigration('0020-jobs-codemode-only.sql')) + db.exec(await readMigration('0020-jobs-codemode-only.sql')) - const columns = db + const columns = db ... - expect(rows).toEqual([...]) + expect(rows).toEqual([...]) + } finally { + db.close() + } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts` around lines 10 - 143, The test creates a DatabaseSync instance named db but never closes it; wrap the test body that runs migrations, inserts, and assertions in a try/finally (or use a finally equivalent) and call db.close() in the finally block to ensure DatabaseSync is always closed; target the block around the existing use of DatabaseSync (the insertJob.prepare/run, db.exec(readMigration(...)), and the final expects) so db.close() executes regardless of test failures.
🤖 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/migrations/0020-jobs-codemode-only.sql`:
- Around line 60-66: The CASE that builds code uses
REPLACE(COALESCE(NULLIF(TRIM(method_name), ''), 'run'), '''', '''''') to escape
single quotes but still allows literal newlines to break the JavaScript string;
replace this manual escaping with a JSON-safe quoted string by passing the
method_name through json_quote() (or
json_quote(COALESCE(NULLIF(TRIM(method_name), ''), 'run'))) when concatenating
the generated 'async (params) => await job.call(...)' expression so quotes and
newlines are properly escaped; update the branch that builds code from
server_code/method_name (the WHEN server_code ... THEN 'async (params) => await
job.call(' ... ) to use json_quote for method_name and remove the custom
REPLACE-based escaping.
In `@packages/worker/src/mcp/capabilities/jobs/shared.ts`:
- Around line 98-103: The schema in shared.ts currently allows null/absent
shapes that should be rejected up-front: remove .nullable() from the
codemode/code fields (the z.string().min(1).nullable().optional() entries) so
update requests with code: null fail validation, and add a schema-level
refinement (superRefine) to the upsert/create schema to reject create payloads
that have methodName present but no serverCode; update or add this validation
where the shared upsert schema is composed (refer to the schema definitions in
packages/worker/src/mcp/capabilities/jobs/shared.ts and the upsert handling in
packages/worker/src/mcp/capabilities/jobs/job-upsert.ts) so
resolveCreateShape()/resolveUpdatedShape() in
packages/worker/src/jobs/service.ts no longer see these as runtime errors.
---
Outside diff comments:
In `@packages/worker/src/jobs/service.ts`:
- Around line 499-505: The pre-sync call to syncRunnerForJob should be guarded
so missing/unparseable caller context doesn't cause
runJobNow()/runDueJobsForUser to throw before executeJobOnce can record the
structured execution error; update the calls that use hasFacetState(row.record)
and syncRunnerForJob(...) to first verify the activeCallerContext (the parsed
caller context used elsewhere) is non-null/valid and only then invoke
syncRunnerForJob, otherwise skip the pre-sync and allow executeJobOnce (the
existing job execution path) to handle and surface the execution error; apply
the same guard to the other similar block that triggers syncRunnerForJob in
runDueJobsForUser/runJobNow.
---
Nitpick comments:
In `@packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts`:
- Around line 10-143: The test creates a DatabaseSync instance named db but
never closes it; wrap the test body that runs migrations, inserts, and
assertions in a try/finally (or use a finally equivalent) and call db.close() in
the finally block to ensure DatabaseSync is always closed; target the block
around the existing use of DatabaseSync (the insertJob.prepare/run,
db.exec(readMigration(...)), and the final expects) so db.close() executes
regardless of test failures.
🪄 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: ac264a4e-610b-48bb-aac8-7955beb95404
📒 Files selected for processing (18)
docs/contributing/architecture/data-storage.mddocs/use/execute.mdpackages/worker/migrations/0020-jobs-codemode-only.sqlpackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/job-runner.workers.test.tspackages/worker/src/jobs/jobs-codemode-only-migration.node.test.tspackages/worker/src/jobs/process-due-jobs.node.test.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/jobs/types.tspackages/worker/src/mcp/capabilities/jobs/domain.tspackages/worker/src/mcp/capabilities/jobs/job-upsert.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/tools/execute.ts
💤 Files with no reviewable changes (2)
- packages/worker/src/jobs/process-due-jobs.node.test.ts
- packages/worker/src/jobs/job-runner.ts
Comment on lines
+60
to
+66
| CASE | ||
| WHEN code IS NOT NULL AND TRIM(code) != '' THEN code | ||
| WHEN server_code IS NOT NULL AND TRIM(server_code) != '' THEN | ||
| 'async (params) => await job.call(''' || | ||
| REPLACE(COALESCE(NULLIF(TRIM(method_name), ''), 'run'), '''', '''''') || | ||
| ''', params)' | ||
| END AS code, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pip install --quiet esprima
python - <<'PY'
import sqlite3
import esprima
conn = sqlite3.connect(":memory:")
expr = """
SELECT
'async (params) => await job.call(''' ||
REPLACE(?, '''', '''''') ||
''', params)'
"""
for method_name in ["run", "it's", "line\nbreak"]:
(generated_code,) = conn.execute(expr, (method_name,)).fetchone()
print(f"\nmethod_name={method_name!r}")
print(f"generated_code={generated_code!r}")
try:
esprima.parseScript(f"({generated_code})")
print("parse=ok")
except Exception as exc:
print(f"parse=FAIL -> {exc}")
PYRepository: kentcdodds/kody
Length of output: 654
🏁 Script executed:
fd -type f -name "0020-jobs-codemode-only.sql"Repository: kentcdodds/kody
Length of output: 229
🏁 Script executed:
cat -n packages/worker/migrations/0020-jobs-codemode-only.sql | sed -n '55,70p'Repository: kentcdodds/kody
Length of output: 592
🏁 Script executed:
# Check for SQLite version or other migration setup files
fd -type f -name "*.sql" packages/worker/migrations/ | head -20Repository: kentcdodds/kody
Length of output: 229
🏁 Script executed:
# Check if json_quote is used elsewhere in the codebase
rg "json_quote" --type sqlRepository: kentcdodds/kody
Length of output: 41
🏁 Script executed:
pip install --quiet esprima
python - <<'PY'
import sqlite3
import esprima
conn = sqlite3.connect(":memory:")
# Test with the ACTUAL SQL from the migration file using CHAR(92)
expr = """
SELECT
'async (params) => await job.call(''' ||
REPLACE(?, '''', CHAR(92) || '''') ||
''', params)'
"""
for method_name in ["run", "it's", "line\nbreak"]:
(generated_code,) = conn.execute(expr, (method_name,)).fetchone()
print(f"\nmethod_name={method_name!r}")
print(f"generated_code={generated_code!r}")
try:
esprima.parseScript(f"({generated_code})")
print("parse=ok")
except Exception as exc:
print(f"parse=FAIL -> {exc}")
PYRepository: kentcdodds/kody
Length of output: 395
🏁 Script executed:
# Check the broader context of the migration to understand schema constraints
cat -n packages/worker/migrations/0020-jobs-codemode-only.sql | head -55Repository: kentcdodds/kody
Length of output: 1622
🏁 Script executed:
# Test if json_quote() would properly escape for JavaScript
pip install --quiet esprima
python - <<'PY'
import sqlite3
import esprima
conn = sqlite3.connect(":memory:")
# Test with json_quote approach suggested in review
expr = """
SELECT
'async (params) => await job.call(' ||
json_quote(?) ||
', params)'
"""
for method_name in ["run", "it's", "line\nbreak"]:
try:
(generated_code,) = conn.execute(expr, (method_name,)).fetchone()
print(f"\nmethod_name={method_name!r}")
print(f"generated_code={generated_code!r}")
try:
esprima.parseScript(f"({generated_code})")
print("parse=ok")
except Exception as exc:
print(f"parse=FAIL -> {exc}")
except Exception as e:
print(f"Error with method_name={method_name!r}: {e}")
PYRepository: kentcdodds/kody
Length of output: 357
The migration partially escapes method_name for JavaScript, but literal newlines will still break.
The code uses CHAR(92) (backslash) to escape single quotes, which fixes the it's case. However, if method_name contains a literal newline, the generated code will have an unescaped newline inside the string literal, causing a parse error. Use json_quote() instead to properly escape both quotes and newlines for JavaScript:
Recommended fix
CASE
WHEN code IS NOT NULL AND TRIM(code) != '' THEN code
WHEN server_code IS NOT NULL AND TRIM(server_code) != '' THEN
- 'async (params) => await job.call(''' ||
- REPLACE(COALESCE(NULLIF(TRIM(method_name), ''), 'run'), '''', CHAR(92) || '''') ||
- ''', params)'
+ 'async (params) => await job.call(' ||
+ json_quote(COALESCE(NULLIF(TRIM(method_name), ''), 'run')) ||
+ ', params)'
END AS code,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/migrations/0020-jobs-codemode-only.sql` around lines 60 - 66,
The CASE that builds code uses REPLACE(COALESCE(NULLIF(TRIM(method_name), ''),
'run'), '''', '''''') to escape single quotes but still allows literal newlines
to break the JavaScript string; replace this manual escaping with a JSON-safe
quoted string by passing the method_name through json_quote() (or
json_quote(COALESCE(NULLIF(TRIM(method_name), ''), 'run'))) when concatenating
the generated 'async (params) => await job.call(...)' expression so quotes and
newlines are properly escaped; update the branch that builds code from
server_code/method_name (the WHEN server_code ... THEN 'async (params) => await
job.call(' ... ) to use json_quote for method_name and remove the custom
REPLACE-based escaping.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Migration escaping misses backslashes in method names
- Escaped backslashes before single quotes in the generated JS string so method names with backslashes are preserved safely.
You can send follow-ups to the cloud agent here.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/migrations/0020-jobs-codemode-only.sql (1)
60-65:⚠️ Potential issue | 🟠 MajorUse
json_quoteformethod_nameserialization in generated JS wrapper.Line 63-Line 65 still builds a single-quoted JS string via manual
REPLACE(...), which does not escape literal newlines. A newline inmethod_namewill produce invalid JavaScript after migration.Proposed fix
CASE WHEN code IS NOT NULL AND TRIM(code) != '' THEN code WHEN server_code IS NOT NULL AND TRIM(server_code) != '' THEN - 'async (params) => await job.call(''' || - REPLACE(COALESCE(NULLIF(TRIM(method_name), ''), 'run'), '''', CHAR(92) || '''') || - ''', params)' + 'async (params) => await job.call(' || + json_quote(COALESCE(NULLIF(TRIM(method_name), ''), 'run')) || + ', params)' ELSE 'async () => { throw new Error("migration: missing code") }' END AS code,#!/bin/bash set -euo pipefail python - <<'PY' import sqlite3 conn = sqlite3.connect(":memory:") expr = """ SELECT 'async (params) => await job.call(''' || REPLACE(COALESCE(NULLIF(TRIM(?), ''), 'run'), '''', CHAR(92) || '''') || ''', params)' """ for method_name in ["run", "it's", "line\nbreak"]: (generated,) = conn.execute(expr, (method_name,)).fetchone() print(f"method_name={method_name!r}") print(f"generated={generated!r}") print("contains_literal_newline=", "\\n" in generated) print("-" * 40) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/migrations/0020-jobs-codemode-only.sql` around lines 60 - 65, The CASE expression that builds the JS wrapper currently uses REPLACE(...) to escape single quotes for method_name (in the branch that returns 'async (params) => await job.call(...)'), which fails to escape literal newlines and other characters; replace the manual REPLACE-based serialization with sqlite's json_quote on the COALESCE(NULLIF(TRIM(method_name), ''), 'run') value so the generated JS string is safely escaped (keep the surrounding concatenation that builds 'async (params) => await job.call(' || json_quote(...) || ', params)'). Ensure the symbol references are the CASE branch that constructs the JS wrapper and the TRIM(method_name)/COALESCE expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/migrations/0020-jobs-codemode-only.sql`:
- Around line 60-65: The CASE expression that builds the JS wrapper currently
uses REPLACE(...) to escape single quotes for method_name (in the branch that
returns 'async (params) => await job.call(...)'), which fails to escape literal
newlines and other characters; replace the manual REPLACE-based serialization
with sqlite's json_quote on the COALESCE(NULLIF(TRIM(method_name), ''), 'run')
value so the generated JS string is safely escaped (keep the surrounding
concatenation that builds 'async (params) => await job.call(' || json_quote(...)
|| ', params)'). Ensure the symbol references are the CASE branch that
constructs the JS wrapper and the TRIM(method_name)/COALESCE expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e0ee097-ae84-4add-a92d-bf1a413d93a4
📒 Files selected for processing (1)
packages/worker/migrations/0020-jobs-codemode-only.sql
| userId: callerContext.user?.userId ?? '', | ||
| storageId: storageTools.storageId, | ||
| writable: storageTools.writable, | ||
| }) |
Contributor
There was a problem hiding this comment.
storageTools.userId silently ignored in buildCodemodeFns
Medium Severity
buildCodemodeFns accepts storageTools with a userId field via StorageToolOptions, but ignores it — instead using callerContext.user?.userId ?? '' when creating storage codemode tools. If a caller ever passes a storageTools.userId that differs from the caller context user (e.g., for cross-user storage inspection), the wrong Durable Object would be addressed. The userId field on StorageToolOptions is effectively dead, creating a misleading API contract.
Reviewed by Cursor Bugbot for commit 752fec3. Configure here.
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
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/jobs/types.ts (1)
101-120:⚠️ Potential issue | 🟠 MajorDon't let update/upsert clear a required
codefield.Line 104 and Line 115 still allow
code: null, butJobRecord.codeandJobCreateInput.codeare now required. That leaves the public contract able to express a persisted state the codemode-only runtime no longer supports.🔧 Proposed fix
export type JobUpdateInput = { id: string name?: string - code?: string | null + code?: string params?: Record<string, unknown> | null schedule?: JobSchedule timezone?: string | null enabled?: boolean killSwitchEnabled?: boolean } export type JobUpsertInput = { id?: string name?: string - code?: string | null + code?: string params?: Record<string, unknown> | null schedule?: JobSchedule timezone?: string | null enabled?: boolean killSwitchEnabled?: boolean }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/types.ts` around lines 101 - 120, JobUpdateInput and JobUpsertInput currently allow code?: string | null which can clear a required code field; update these types (JobUpdateInput and JobUpsertInput) to forbid null for code (make it either code?: string or code: string when appropriate) so callers cannot set code to null and violate the runtime expectation expressed by JobRecord.code and JobCreateInput.code; ensure the change preserves optionality for omitted fields but disallows explicit null, and update any usage sites or validators that rely on null semantics to use absence instead.
🧹 Nitpick comments (4)
packages/worker/src/mcp/storage-bindings.ts (1)
24-29: Normalize returned binding keys after trim checks.The code validates trimmed identifiers but returns raw strings. Returning canonical (trimmed) keys avoids whitespace-driven bucket key drift.
♻️ Proposed refactor
if (scope === 'app') { - return storageContext?.appId?.trim() - ? storageContext.appId - : storageContext?.storageId?.trim() - ? storageContext.storageId - : null + const appId = storageContext?.appId?.trim() + if (appId) return appId + const storageId = storageContext?.storageId?.trim() + return storageId || null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/storage-bindings.ts` around lines 24 - 29, The block in storage-bindings.ts that checks scope === 'app' validates trimmed identifiers but returns raw values; change the returns to the canonical trimmed versions so whitespace is removed from binding keys: when returning storageContext.appId or storageContext.storageId use their .trim() results (guarding for null/undefined) instead of the original strings; update the conditional expressions around storageContext?.appId and storageContext?.storageId to both check and return the trimmed value to prevent bucket key drift.packages/worker/src/storage-runner.workers.test.ts (1)
131-151: Consider usingexpect().rejectsfor cleaner error assertion.The test correctly verifies that mutating SQL is blocked when
writable: false, but could be simplified using Vitest's built-in rejection matcher.♻️ Suggested refactor
test('storage runner blocks mutating SQL when writable is false', async () => { const storageId = createExecuteStorageId() const runner = storageRunnerRpc({ env, userId: 'user-123', storageId, }) - try { - await runner.sqlQuery({ - query: 'delete from counters', - writable: false, - }) - throw new Error('Expected read-only SQL mutation to fail.') - } catch (error) { - expect(error).toBeInstanceOf(Error) - expect((error as Error).message).toBe( - 'Read-only storage.sql only allows SELECT, EXPLAIN, and schema PRAGMA queries. Pass writable: true to allow mutations.', - ) - } + await expect( + runner.sqlQuery({ + query: 'delete from counters', + writable: false, + }), + ).rejects.toThrow( + 'Read-only storage.sql only allows SELECT, EXPLAIN, and schema PRAGMA queries. Pass writable: true to allow mutations.', + ) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/storage-runner.workers.test.ts` around lines 131 - 151, The test "storage runner blocks mutating SQL when writable is false" uses try/catch for error assertions; replace it with Vitest's promise rejection matcher by calling storageRunnerRpc(...).sqlQuery(...) and asserting with expect(promise).rejects.toThrow(...) (or .rejects.toEqual(...) if you want exact message) to assert the error message, targeting the runner.sqlQuery call returned promise from storageRunnerRpc to make the test concise and idiomatic.packages/worker/src/mcp/capabilities/storage/storage-query.ts (1)
5-7: Consider usingrequireStorageUserfor consistency withstorage-export.ts.This file imports
requireMcpUserdirectly from#mcp/capabilities/meta/require-user.tsand calls it asrequireMcpUser(ctx.callerContext)on line 46, whereasstorage-export.tsuses the shared helperrequireStorageUser(ctx)from./shared.ts. Both do the same thing, but using the shared helper would be more consistent across storage capabilities.♻️ Suggested change for consistency
import { z } from 'zod' import { defineDomainCapability } from '#mcp/capabilities/define-domain-capability.ts' import { capabilityDomainNames } from '#mcp/capabilities/domain-metadata.ts' import { type CapabilityContext } from '#mcp/capabilities/types.ts' -import { requireMcpUser } from '#mcp/capabilities/meta/require-user.ts' import { storageRunnerRpc } from '#worker/storage-runner.ts' -import { storageIdSchema } from './shared.ts' +import { storageIdSchema, requireStorageUser } from './shared.ts'And on line 46:
- const user = requireMcpUser(ctx.callerContext) + const user = requireStorageUser(ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/storage/storage-query.ts` around lines 5 - 7, Replace the direct use of requireMcpUser(ctx.callerContext) in storage-query.ts with the shared helper requireStorageUser(ctx) used by storage-export.ts: remove the import of requireMcpUser from '#mcp/capabilities/meta/require-user.ts', import requireStorageUser from './shared.ts' alongside storageIdSchema, and call requireStorageUser(ctx) wherever requireMcpUser(ctx.callerContext) is currently used (e.g., in the function that currently invokes requireMcpUser) to keep storage capability auth logic consistent.packages/worker/src/jobs/types.ts (1)
59-63: Avoid a second source of truth for job storage IDs.This duplicates
createJobStorageIdinpackages/worker/src/storage-runner.ts(Lines 56-58). Since creation, migration, and lookup all depend on the exact same string format, two builders here are an easy drift point. Prefer one shared implementation and re-export it where needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/jobs/types.ts` around lines 59 - 63, The job storage ID builder here duplicates the logic in createJobStorageId (and jobStorageIdPrefix) in storage-runner.ts; remove buildJobStorageId and jobStorageIdPrefix from packages/worker/src/jobs/types.ts and instead import and re-export the single source of truth createJobStorageId (or export the prefix if needed) so all callers use createJobStorageId from storage-runner.ts to avoid drift between implementations. Ensure any references to buildJobStorageId are updated to import createJobStorageId and keep the exported API the same.
🤖 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`:
- Line 6: The test imports updateJob but never uses it; remove updateJob from
the import list in the import statement that currently reads import { createJob,
executeJobOnce, updateJob } from './service.ts' so it becomes only import {
createJob, executeJobOnce } from './service.ts' to eliminate the unused import
warning.
In `@packages/worker/src/mcp/capabilities/storage/shared.ts`:
- Around line 9-16: There is an unused duplicate schema: remove dead code by
deleting storageSqlOutputSchema, or preferably refactor storage-query.ts to
import storageSqlOutputSchema and replace its local outputSchema with
storageSqlOutputSchema.extend({ ok: z.boolean(), query: z.string(), writable:
z.boolean() }) so the shared base fields live in storageSqlOutputSchema and you
avoid duplication; update any imports/exports accordingly (symbols:
storageSqlOutputSchema, outputSchema, storage-query.ts).
In `@packages/worker/src/storage-runner.ts`:
- Around line 48-50: The durable-object name builder buildStorageRunnerName
currently concatenates raw userId and storageId with ':' causing ambiguous
collisions when either component contains ':'; update buildStorageRunnerName to
encode both components (e.g., via encodeURIComponent or a stable base64url
encoder) before joining with ':' so the resulting name is unambiguous and
reversible; ensure you apply the same decoding/lookup logic wherever
buildStorageRunnerName is consumed if needed.
- Around line 94-118: The assertSqlAllowed function currently only checks the
leading prefix (allowedReadOnlyPrefixes) allowing multi-statement payloads like
"SELECT 1; DELETE ..."; update assertSqlAllowed to reject any multi-statement
queries when writable is falsy by detecting semicolons or multiple
statements—e.g., after trimming ensure the query contains no ';' and/or that
splitting on ';' yields exactly one non-empty statement and that the single
statement matches the allowed read-only prefixes; keep the existing writable
branch intact so writable: true still permits mutations.
- Around line 120-130: The cursorToSqlResult function currently materializes the
entire cursor via cursor.toArray(), which can OOM for large SELECTs called by
sqlQuery; update sqlQuery (and cursorToSqlResult) to enforce a safe max row
limit (reusing the same pagination bounds as listValues, e.g., 250–1000) or
stream results by iterating the SqlStorageCursor with cursor.next()/for…of and
stop after the limit, then build StorageSqlResult.columns, rows (truncated to
the enforced limit), rowCount (actual returned rows), and preserve
rowsRead/rowsWritten; ensure the limit is applied before materializing and that
sqlQuery documents/returns a truncated result rather than loading the full
result set.
---
Outside diff comments:
In `@packages/worker/src/jobs/types.ts`:
- Around line 101-120: JobUpdateInput and JobUpsertInput currently allow code?:
string | null which can clear a required code field; update these types
(JobUpdateInput and JobUpsertInput) to forbid null for code (make it either
code?: string or code: string when appropriate) so callers cannot set code to
null and violate the runtime expectation expressed by JobRecord.code and
JobCreateInput.code; ensure the change preserves optionality for omitted fields
but disallows explicit null, and update any usage sites or validators that rely
on null semantics to use absence instead.
---
Nitpick comments:
In `@packages/worker/src/jobs/types.ts`:
- Around line 59-63: The job storage ID builder here duplicates the logic in
createJobStorageId (and jobStorageIdPrefix) in storage-runner.ts; remove
buildJobStorageId and jobStorageIdPrefix from packages/worker/src/jobs/types.ts
and instead import and re-export the single source of truth createJobStorageId
(or export the prefix if needed) so all callers use createJobStorageId from
storage-runner.ts to avoid drift between implementations. Ensure any references
to buildJobStorageId are updated to import createJobStorageId and keep the
exported API the same.
In `@packages/worker/src/mcp/capabilities/storage/storage-query.ts`:
- Around line 5-7: Replace the direct use of requireMcpUser(ctx.callerContext)
in storage-query.ts with the shared helper requireStorageUser(ctx) used by
storage-export.ts: remove the import of requireMcpUser from
'#mcp/capabilities/meta/require-user.ts', import requireStorageUser from
'./shared.ts' alongside storageIdSchema, and call requireStorageUser(ctx)
wherever requireMcpUser(ctx.callerContext) is currently used (e.g., in the
function that currently invokes requireMcpUser) to keep storage capability auth
logic consistent.
In `@packages/worker/src/mcp/storage-bindings.ts`:
- Around line 24-29: The block in storage-bindings.ts that checks scope ===
'app' validates trimmed identifiers but returns raw values; change the returns
to the canonical trimmed versions so whitespace is removed from binding keys:
when returning storageContext.appId or storageContext.storageId use their
.trim() results (guarding for null/undefined) instead of the original strings;
update the conditional expressions around storageContext?.appId and
storageContext?.storageId to both check and return the trimmed value to prevent
bucket key drift.
In `@packages/worker/src/storage-runner.workers.test.ts`:
- Around line 131-151: The test "storage runner blocks mutating SQL when
writable is false" uses try/catch for error assertions; replace it with Vitest's
promise rejection matcher by calling storageRunnerRpc(...).sqlQuery(...) and
asserting with expect(promise).rejects.toThrow(...) (or .rejects.toEqual(...) if
you want exact message) to assert the error message, targeting the
runner.sqlQuery call returned promise from storageRunnerRpc to make the test
concise and idiomatic.
🪄 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: af5a1e50-9b89-458b-b7be-7bb4badd9d5b
📒 Files selected for processing (36)
docs/contributing/architecture/data-storage.mddocs/use/execute.mdpackages/shared/src/chat.tspackages/worker/migrations/0020-jobs-codemode-only.sqlpackages/worker/migrations/0021-jobs-storage-id.sqlpackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/job-runner.workers.test.tspackages/worker/src/jobs/jobs-codemode-only-migration.node.test.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/jobs/types.tspackages/worker/src/mcp/app-runner.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/job-upsert.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/capabilities/storage/domain.tspackages/worker/src/mcp/capabilities/storage/shared.tspackages/worker/src/mcp/capabilities/storage/storage-export.tspackages/worker/src/mcp/capabilities/storage/storage-query.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/storage-bindings.tspackages/worker/src/mcp/storage.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/storage-runner.tspackages/worker/src/storage-runner.workers.test.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonc
💤 Files with no reviewable changes (2)
- packages/worker/src/jobs/job-runner.ts
- packages/worker/src/jobs/job-runner.workers.test.ts
✅ Files skipped from review due to trivial changes (5)
- packages/worker/src/mcp/storage.ts
- packages/worker/src/mcp/capabilities/jobs/domain.ts
- packages/worker/src/mcp/capabilities/builtin-domains.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- docs/use/execute.md
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/worker/src/mcp/generated-ui-api.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts
- packages/worker/src/mcp/tools/execute.ts
- packages/worker/migrations/0020-jobs-codemode-only.sql
- packages/worker/src/mcp/capabilities/jobs/job-upsert.ts
- packages/worker/src/jobs/repo.ts
- packages/worker/src/mcp/run-codemode-registry.ts
| import { saveSecret } from '#mcp/secrets/service.ts' | ||
| import { saveValue } from '#mcp/values/service.ts' | ||
| import { createJob, executeJobOnce } from './service.ts' | ||
| import { createJob, executeJobOnce, updateJob } from './service.ts' |
There was a problem hiding this comment.
Remove unused import updateJob.
Static analysis indicates updateJob is imported but never used in this test file.
🧹 Suggested fix
-import { createJob, executeJobOnce, updateJob } from './service.ts'
+import { createJob, executeJobOnce } from './service.ts'📝 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
| import { createJob, executeJobOnce, updateJob } from './service.ts' | |
| import { createJob, executeJobOnce } from './service.ts' |
🧰 Tools
🪛 GitHub Check: 🧹 Lint
[warning] 6-6: eslint(no-unused-vars)
Identifier 'updateJob' is imported but never used.
🤖 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` at line 6, The test imports
updateJob but never uses it; remove updateJob from the import list in the import
statement that currently reads import { createJob, executeJobOnce, updateJob }
from './service.ts' so it becomes only import { createJob, executeJobOnce } from
'./service.ts' to eliminate the unused import warning.
Comment on lines
+9
to
+16
| export const storageSqlOutputSchema = z.object({ | ||
| storage_id: z.string(), | ||
| columns: z.array(z.string()), | ||
| rows: z.array(z.record(z.string(), z.union([z.string(), z.number(), z.null()]))), | ||
| row_count: z.number(), | ||
| rows_read: z.number(), | ||
| rows_written: z.number(), | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of storageSqlOutputSchema
rg -n 'storageSqlOutputSchema' --type=tsRepository: kentcdodds/kody
Length of output: 164
🏁 Script executed:
fd -t f "storage-query" --type fRepository: kentcdodds/kody
Length of output: 120
🏁 Script executed:
cat -n packages/worker/src/mcp/capabilities/storage/shared.tsRepository: kentcdodds/kody
Length of output: 1286
🏁 Script executed:
rg -n "import.*shared" packages/worker/src/mcp/capabilities/storage/ --type=tsRepository: kentcdodds/kody
Length of output: 326
🏁 Script executed:
cat -n packages/worker/src/mcp/capabilities/storage/storage-query.tsRepository: kentcdodds/kody
Length of output: 2748
🏁 Script executed:
rg -n "outputSchema" packages/worker/src/mcp/capabilities/storage/ --type=ts -A 3Repository: kentcdodds/kody
Length of output: 1277
🏁 Script executed:
rg -n "storageSql" packages/worker/src/mcp/ --type=tsRepository: kentcdodds/kody
Length of output: 164
🏁 Script executed:
rg -n "storageExportOutputSchema|storageIdSchema" packages/worker/src/mcp/capabilities/storage/ --type=tsRepository: kentcdodds/kody
Length of output: 855
Remove dead code storageSqlOutputSchema or refactor storage-query.ts to use it.
The schema is never imported or used anywhere in the codebase. Meanwhile, storage-query.ts defines a local outputSchema that duplicates the base fields from storageSqlOutputSchema plus additional fields (ok, query, writable). Either remove the unused schema, or refactor storage-query.ts to use storageSqlOutputSchema.extend({...}) to avoid field duplication and potential drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/storage/shared.ts` around lines 9 - 16,
There is an unused duplicate schema: remove dead code by deleting
storageSqlOutputSchema, or preferably refactor storage-query.ts to import
storageSqlOutputSchema and replace its local outputSchema with
storageSqlOutputSchema.extend({ ok: z.boolean(), query: z.string(), writable:
z.boolean() }) so the shared base fields live in storageSqlOutputSchema and you
avoid duplication; update any imports/exports accordingly (symbols:
storageSqlOutputSchema, outputSchema, storage-query.ts).
Comment on lines
+120
to
+130
| function cursorToSqlResult( | ||
| cursor: SqlStorageCursor<Record<string, StorageSqlValue>>, | ||
| ): StorageSqlResult { | ||
| const rows = cursor.toArray() | ||
| return { | ||
| columns: [...cursor.columnNames], | ||
| rows, | ||
| rowCount: rows.length, | ||
| rowsRead: cursor.rowsRead, | ||
| rowsWritten: cursor.rowsWritten, | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What APIs does Cloudflare Durable Object SqlStorageCursorexpose for incremental iteration or limiting rows withouttoArray(), and are there documented response-size limits for Durable Object RPC payloads?
💡 Result:
Cloudflare Durable Objects SqlStorageCursor exposes the following APIs for incremental iteration without toArray: next, raw (returns Iterator over raw column arrays), and for...of iteration (Iterable/Iterator protocol). It also has one for single-row results. There are no documented methods like limit or skip on the cursor itself; pagination must use SQL LIMIT/OFFSET in the query. Properties include columnNames (string[]), rowsRead (number of rows iterated so far), and rowsWritten. No explicit documented response-size limits for Durable Object RPC payloads were found in official docs. Durable Objects are Workers with general Workers limits applying: response header size 128KB total, response body no enforced limit (CDN cache limits 512MB Free/Pro/Business), memory 128MB per isolate. SQLite row size limit is 2MB, SQL statement 100KB. RPC via stubs uses standard Worker fetch with these constraints; large responses risk memory exhaustion or practical limits from serialization/transfer.
Citations:
- 1: https://developers.cloudflare.com/durable-objects/api/sqlite-storage-api/
- 2: https://developers.cloudflare.com/durable-objects/platform/limits/
- 3: https://developers.cloudflare.com/durable-objects/best-practices/access-durable-objects-storage/
- 4: https://developers.cloudflare.com/workers/platform/limits/
🏁 Script executed:
cat -n packages/worker/src/storage-runner.ts | sed -n '100,230p'Repository: kentcdodds/kody
Length of output: 4261
🏁 Script executed:
rg -n "sqlQuery|rowCount|MAX.*ROW|LIMIT" packages/worker/src/storage-runner.tsRepository: kentcdodds/kody
Length of output: 203
🏁 Script executed:
rg -n "normalizePageSize|pageSize" packages/worker/src/storage-runner.tsRepository: kentcdodds/kody
Length of output: 633
🏁 Script executed:
cat -n packages/worker/src/storage-runner.ts | sed -n '68,85p'Repository: kentcdodds/kody
Length of output: 709
🏁 Script executed:
cat -n packages/worker/src/storage-runner.ts | sed -n '1,70p'Repository: kentcdodds/kody
Length of output: 1990
Add row limits to SQL query results.
The sqlQuery method (line 206) executes arbitrary SELECT queries without enforcing a maximum row count. Line 123 materializes the entire result with toArray(), which can exhaust memory on broad queries. Unlike listValues, which applies pagination bounds (250–1,000 rows), sqlQuery has no safeguards. Add a SQL LIMIT clause or use the cursor's incremental iteration APIs (next() or for…of) to stream results instead of materializing them.
This also applies to the query execution pattern at lines 206–216.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/storage-runner.ts` around lines 120 - 130, The
cursorToSqlResult function currently materializes the entire cursor via
cursor.toArray(), which can OOM for large SELECTs called by sqlQuery; update
sqlQuery (and cursorToSqlResult) to enforce a safe max row limit (reusing the
same pagination bounds as listValues, e.g., 250–1000) or stream results by
iterating the SqlStorageCursor with cursor.next()/for…of and stop after the
limit, then build StorageSqlResult.columns, rows (truncated to the enforced
limit), rowCount (actual returned rows), and preserve rowsRead/rowsWritten;
ensure the limit is applied before materializing and that sqlQuery
documents/returns a truncated result rather than loading the full result set.
Contributor
|
Bugbot Autofix prepared fixes for all 3 issues found in the latest run.
Preview (33eb0fc173)diff --git a/docs/contributing/architecture/data-storage.md b/docs/contributing/architecture/data-storage.md
--- a/docs/contributing/architecture/data-storage.md
+++ b/docs/contributing/architecture/data-storage.md
@@ -53,23 +53,24 @@
- The Worker routes same-origin browser chat traffic to the agent using the
existing app session cookie rather than the public MCP OAuth flow
-## Durable Objects (`JobManager` and `JobRunner`)
+## Durable Objects (`JobManager` and `StorageRunner`)
-Unified jobs use two Durable Object roles:
+Jobs use two Durable Object roles:
- `JobManager`: one object per user, responsible only for alarm scheduling and
dispatching due jobs from D1-backed metadata
-- `JobRunner`: one object per facet-backed job, responsible for dynamic-worker
- loading, explicit RPC bridging, and isolated facet SQLite state
+- `StorageRunner`: one object per durable storage id, responsible for isolated
+ SQLite state that can be bound to execute calls, jobs, and dedicated storage
+ inspection capabilities
Storage split:
- D1 `jobs` table: job metadata, persisted caller context, schedule, run
- counters, last error, last duration, and run history
+ counters, last error, last duration, run history, codemode source, and
+ stable `storage_id`
- `JobManager` SQLite: only alarm bookkeeping needed to wake the right user's
due jobs
-- `JobRunner` parent SQLite: facet runner config and observability for one job
-- Facet SQLite: the user job's isolated Durable Object state
+- `StorageRunner` SQLite: isolated durable state addressed by `storageId`
## Configuration reference
@@ -81,5 +82,5 @@
- `MCP_OBJECT` (Durable Objects)
- `ChatAgent` (Durable Objects)
- `JOB_MANAGER` (Durable Objects)
-- `JOB_RUNNER` (Durable Objects)
+- `STORAGE_RUNNER` (Durable Objects)
- `ASSETS` (static assets bucket)
diff --git a/docs/use/execute.md b/docs/use/execute.md
--- a/docs/use/execute.md
+++ b/docs/use/execute.md
@@ -45,13 +45,16 @@
- **`job_run_now`** — trigger a job immediately without changing its normal
schedule
-Jobs support two execution kinds:
+Jobs have:
-- **`codemode`** jobs store async arrow function source and run through the same
+- **`code`** stores the async arrow function source and runs through the same
execute/capability runtime as normal `execute`
-- **`facet`** jobs store `serverCode` that exports
- **`class Job extends DurableObject`** and run through an isolated per-job
- facet Durable Object with its own SQLite state
+- Each job has a stable **`storageId`** that identifies its durable storage
+ bucket
+- Scheduled jobs run with writable storage access
+- Ad hoc execute calls can bind to a storage bucket with **`storageId`** and use
+ **`storage.get(...)`**, **`storage.set(...)`**, **`storage.list(...)`**, and
+ **`storage.sql(query, params?)`**
Schedules support:
@@ -60,6 +63,12 @@
- **interval** — fixed durations such as **`15m`**, **`1h`**, or **`1d`**
- **once** — an ISO 8601 UTC timestamp such as **`2026-04-17T15:00:00Z`**
+For dedicated inspection, use:
+
+- **`storage_export`** — export one storage bucket as JSON
+- **`storage_query`** — run SQL against one storage bucket (read-only by
+ default, opt into writes explicitly)
+
## Long-term memory
Kody can surface a small number of relevant long-term memories when you pass a
diff --git a/packages/shared/src/chat.ts b/packages/shared/src/chat.ts
--- a/packages/shared/src/chat.ts
+++ b/packages/shared/src/chat.ts
@@ -44,6 +44,7 @@
export const mcpStorageContextSchema = object({
sessionId: optional(nullable(string())),
appId: optional(nullable(string())),
+ storageId: optional(nullable(string())),
})
const remoteConnectorRefSchema = object({
diff --git a/packages/worker/migrations/0020-jobs-codemode-only.sql b/packages/worker/migrations/0020-jobs-codemode-only.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0020-jobs-codemode-only.sql
@@ -1,0 +1,104 @@
+PRAGMA defer_foreign_keys = ON;
+
+CREATE TABLE jobs_v3 (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ name TEXT NOT NULL,
+ code TEXT NOT NULL,
+ server_code TEXT,
+ server_code_id TEXT,
+ method_name TEXT,
+ params_json TEXT,
+ schedule_json TEXT NOT NULL,
+ timezone TEXT NOT NULL,
+ enabled INTEGER NOT NULL DEFAULT 1 CHECK (enabled IN (0, 1)),
+ kill_switch_enabled INTEGER NOT NULL DEFAULT 0 CHECK (kill_switch_enabled IN (0, 1)),
+ caller_context_json TEXT NOT NULL,
+ created_at TEXT NOT NULL,
+ updated_at TEXT NOT NULL,
+ last_run_at TEXT,
+ last_run_status TEXT,
+ last_run_error TEXT,
+ last_duration_ms INTEGER,
+ next_run_at TEXT NOT NULL,
+ run_count INTEGER NOT NULL DEFAULT 0,
+ success_count INTEGER NOT NULL DEFAULT 0,
+ error_count INTEGER NOT NULL DEFAULT 0,
+ run_history_json TEXT NOT NULL DEFAULT '[]'
+);
+
+INSERT INTO jobs_v3 (
+ id,
+ user_id,
+ name,
+ code,
+ server_code,
+ server_code_id,
+ method_name,
+ params_json,
+ schedule_json,
+ timezone,
+ enabled,
+ kill_switch_enabled,
+ caller_context_json,
+ created_at,
+ updated_at,
+ last_run_at,
+ last_run_status,
+ last_run_error,
+ last_duration_ms,
+ next_run_at,
+ run_count,
+ success_count,
+ error_count,
+ run_history_json
+)
+SELECT
+ id,
+ user_id,
+ name,
+ CASE
+ WHEN code IS NOT NULL AND TRIM(code) != '' THEN code
+ WHEN server_code IS NOT NULL AND TRIM(server_code) != '' THEN
+ 'async (params) => await job.call(''' ||
+ REPLACE(REPLACE(COALESCE(NULLIF(TRIM(method_name), ''), 'run'), CHAR(92), CHAR(92) || CHAR(92)), '''', CHAR(92) || '''') ||
+ ''', params)'
+ ELSE 'async () => { throw new Error("migration: missing code") }'
+ END AS code,
+ server_code,
+ server_code_id,
+ CASE
+ WHEN server_code IS NOT NULL AND TRIM(server_code) != '' THEN
+ COALESCE(NULLIF(TRIM(method_name), ''), 'run')
+ ELSE NULL
+ END AS method_name,
+ params_json,
+ schedule_json,
+ timezone,
+ enabled,
+ kill_switch_enabled,
+ caller_context_json,
+ created_at,
+ updated_at,
+ last_run_at,
+ last_run_status,
+ last_run_error,
+ last_duration_ms,
+ next_run_at,
+ run_count,
+ success_count,
+ error_count,
+ run_history_json
+FROM jobs;
+
+DROP TABLE jobs;
+
+ALTER TABLE jobs_v3 RENAME TO jobs;
+
+CREATE INDEX idx_jobs_user_next_run_at
+ ON jobs(user_id, enabled, kill_switch_enabled, next_run_at);
+
+CREATE INDEX idx_jobs_user_name
+ ON jobs(user_id, name);
+
+PRAGMA defer_foreign_keys = OFF;
diff --git a/packages/worker/migrations/0021-jobs-storage-id.sql b/packages/worker/migrations/0021-jobs-storage-id.sql
new file mode 100644
--- /dev/null
+++ b/packages/worker/migrations/0021-jobs-storage-id.sql
@@ -1,0 +1,87 @@
+PRAGMA defer_foreign_keys = ON;
+
+CREATE TABLE jobs_v4 (
+ id TEXT PRIMARY KEY NOT NULL,
+ user_id TEXT NOT NULL,
+ name TEXT NOT NULL,
+ code TEXT NOT NULL,
+ storage_id TEXT NOT NULL,
+ params_json TEXT,
+ schedule_json TEXT NOT NULL,
+ timezone TEXT NOT NULL,
+ enabled INTEGER NOT NULL DEFAULT 1 CHECK (enabled IN (0, 1)),
+ kill_switch_enabled INTEGER NOT NULL DEFAULT 0 CHECK (kill_switch_enabled IN (0, 1)),
+ caller_context_json TEXT NOT NULL,
+ created_at TEXT NOT NULL,
+ updated_at TEXT NOT NULL,
+ last_run_at TEXT,
+ last_run_status TEXT,
+ last_run_error TEXT,
+ last_duration_ms INTEGER,
+ next_run_at TEXT NOT NULL,
+ run_count INTEGER NOT NULL DEFAULT 0,
+ success_count INTEGER NOT NULL DEFAULT 0,
+ error_count INTEGER NOT NULL DEFAULT 0,
+ run_history_json TEXT NOT NULL DEFAULT '[]'
+);
+
+INSERT INTO jobs_v4 (
+ id,
+ user_id,
+ name,
+ code,
+ storage_id,
+ params_json,
+ schedule_json,
+ timezone,
+ enabled,
+ kill_switch_enabled,
+ caller_context_json,
+ created_at,
+ updated_at,
+ last_run_at,
+ last_run_status,
+ last_run_error,
+ last_duration_ms,
+ next_run_at,
+ run_count,
+ success_count,
+ error_count,
+ run_history_json
+)
+SELECT
+ id,
+ user_id,
+ name,
+ code,
+ 'job:' || id AS storage_id,
+ params_json,
+ schedule_json,
+ timezone,
+ enabled,
+ kill_switch_enabled,
+ caller_context_json,
+ created_at,
+ updated_at,
+ last_run_at,
+ last_run_status,
+ last_run_error,
+ last_duration_ms,
+ next_run_at,
+ run_count,
+ success_count,
+ error_count,
+ run_history_json
+FROM jobs;
+
+DROP TABLE jobs;
+
+ALTER TABLE jobs_v4 RENAME TO jobs;
+
+CREATE INDEX idx_jobs_user_next_run_at
+ ON jobs(user_id, enabled, kill_switch_enabled, next_run_at);
+
+CREATE INDEX idx_jobs_user_name
+ ON jobs(user_id, name);
+
+PRAGMA defer_foreign_keys = OFF;
diff --git a/packages/worker/src/env-schema.ts b/packages/worker/src/env-schema.ts
--- a/packages/worker/src/env-schema.ts
+++ b/packages/worker/src/env-schema.ts
@@ -176,12 +176,15 @@
)
},
),
- JOB_RUNNER: createSchema<unknown, DurableObjectNamespace>(
+ STORAGE_RUNNER: createSchema<unknown, DurableObjectNamespace>(
(value, context) => {
if (value) {
return { value: value as DurableObjectNamespace }
}
- return fail('Missing JOB_RUNNER binding for facet jobs.', context.path)
+ return fail(
+ 'Missing STORAGE_RUNNER binding for durable execute and job storage.',
+ context.path,
+ )
},
),
APP_BASE_URL: optionalUrlStringSchema,
diff --git a/packages/worker/src/index.ts b/packages/worker/src/index.ts
--- a/packages/worker/src/index.ts
+++ b/packages/worker/src/index.ts
@@ -6,7 +6,7 @@
import { MCP } from './mcp/index.ts'
import { AppFacetBridge, AppRunner } from './mcp/app-runner.ts'
import { JobManager } from './jobs/manager-do.ts'
-import { JobFacetBridge, JobRunner } from './jobs/job-runner.ts'
+import { StorageRunner } from './storage-runner.ts'
import { chatAgentBasePath } from '@kody-internal/shared/chat-routes.ts'
import { getWorkerSentryOptions } from './sentry-options.ts'
import { handleRequest } from '#app/handler.ts'
@@ -50,9 +50,8 @@
MCP,
AppFacetBridge,
AppRunner,
- JobFacetBridge,
JobManager,
- JobRunner,
+ StorageRunner,
}
const claudeWidgetDomainSuffix = '.claudemcpcontent.com'
diff --git a/packages/worker/src/jobs/job-runner.ts b/packages/worker/src/jobs/job-runner.ts
deleted file mode 100644
--- a/packages/worker/src/jobs/job-runner.ts
+++ /dev/null
@@ -1,698 +1,0 @@
-import * as Sentry from '@sentry/cloudflare'
-import { DurableObject, WorkerEntrypoint } from 'cloudflare:workers'
-import { buildSentryOptions } from '#worker/sentry-options.ts'
-import { expandSecretPlaceholders } from '#mcp/fetch-gateway.ts'
-import { getValue, listValues, saveValue } from '#mcp/values/service.ts'
-import { resolveSecret } from '#mcp/secrets/service.ts'
-import {
- buildConnectorValueName,
- parseConnectorConfig,
- parseConnectorJson,
-} from '#mcp/capabilities/values/connector-shared.ts'
-
-const defaultJobFacetName = 'main'
-const defaultStorageExportPageSize = 250
-const maxStorageExportPageSize = 1_000
-const jobFacetClassExportName = 'JobFacet'
-const jobExecEntrypointName = 'JobExecWorker'
-
-type JobRunnerConfig = {
- jobId: string
- userId: string
- baseUrl: string
- storageContext: {
- sessionId: string | null
- appId: string | null
- }
- facetNames: Array<string>
- serverCode: string
- serverCodeId: string
- methodName: string
- killSwitchEnabled: boolean
- lastError: string | null
-}
-
-type JobBridgeProps = {
- jobId: string
- userId: string
- baseUrl: string
- storageContext: {
- sessionId: string | null
- appId: string | null
- }
- facetName: string
-}
-
-type JobStorageExport = {
- entries: Array<{
- key: string
- value: unknown
- }>
- estimatedBytes: number
- truncated: boolean
- nextStartAfter: string | null
- pageSize: number
-}
-
-const configStorageKey = 'config'
-
-function createFacetWrapperModule() {
- return `
-import * as userModule from './user-job.js'
-
-const BaseJob = userModule.Job
-const reservedMethodNames = new Set([
- 'fetch',
- 'alarm',
- 'webSocketMessage',
- 'webSocketClose',
- 'webSocketError',
- '__kody_resetStorage',
- '__kody_exportStorage',
- '__kody_invokeUserMethod',
-])
-
-if (typeof BaseJob !== 'function') {
- throw new Error('Facet job server code must export class Job extends DurableObject.')
-}
-
-export class ${jobFacetClassExportName} extends BaseJob {
- async __kody_resetStorage() {
- await this.ctx.storage.deleteAll()
- return { ok: true }
- }
-
- async __kody_exportStorage(options) {
- const requestedPageSize =
- typeof options?.pageSize === 'number' && Number.isFinite(options.pageSize)
- ? Math.trunc(options.pageSize)
- : ${defaultStorageExportPageSize}
- const pageSize = Math.min(
- Math.max(requestedPageSize, 1),
- ${maxStorageExportPageSize},
- )
- const startAfter =
- typeof options?.startAfter === 'string' && options.startAfter
- ? options.startAfter
- : undefined
- const entries = []
- let nextStartAfter = null
- let truncated = false
- const listedEntries = await this.ctx.storage.list({
- ...(startAfter ? { startAfter } : {}),
- limit: pageSize + 1,
- })
- for (const [key, value] of listedEntries) {
- if (entries.length === pageSize) {
- truncated = true
- break
- }
- entries.push({ key, value })
- nextStartAfter = key
- }
- return {
- entries,
- estimatedBytes: this.ctx.storage.sql.databaseSize,
- truncated,
- nextStartAfter: truncated ? nextStartAfter : null,
- pageSize,
- }
- }
-
- async __kody_invokeUserMethod(methodName, args) {
- if (typeof methodName !== 'string' || !methodName.trim()) {
- throw new Error('Job RPC method name must be a non-empty string.')
- }
- const normalizedMethodName = methodName.trim()
- if (
- reservedMethodNames.has(normalizedMethodName) ||
- normalizedMethodName.startsWith('__kody_')
- ) {
- throw new Error(\`Job RPC method "\${normalizedMethodName}" is not allowed.\`)
- }
- const method = Reflect.get(this, normalizedMethodName)
- if (typeof method !== 'function') {
- throw new Error(\`Job RPC method "\${normalizedMethodName}" was not found.\`)
- }
- return await Reflect.apply(
- method,
- this,
- Array.isArray(args) ? args : [],
- )
- }
-}
- `.trim()
-}
-
-function createJobExecWorkerModule(input: { code: string }) {
- return `
-import { WorkerEntrypoint } from 'cloudflare:workers'
-
-export class ${jobExecEntrypointName} extends WorkerEntrypoint {
- async run(params) {
- const job = {
- call: (methodName, ...args) => {
- return this.env.JOB.callJobRpc(methodName, args)
- },
- }
- const __kodyUserCode = (${input.code})
- return await __kodyUserCode(params)
- }
-}
- `.trim()
-}
-
-function normalizeStorageContext(
- storageContext:
- | {
- sessionId?: string | null
- appId?: string | null
- }
- | null
- | undefined,
-) {
- return {
- sessionId: storageContext?.sessionId ?? null,
- appId: storageContext?.appId ?? null,
- }
-}
-
-function defaultConfig(jobId: string): JobRunnerConfig {
- return {
- jobId,
- userId: '',
- baseUrl: '',
- storageContext: {
- sessionId: null,
- appId: null,
- },
- facetNames: [defaultJobFacetName],
- serverCode: '',
- serverCodeId: crypto.randomUUID(),
- methodName: 'run',
- killSwitchEnabled: false,
- lastError: null,
- }
-}
-
-export class JobFacetBridge extends WorkerEntrypoint<Env, JobBridgeProps> {
- async callJobRpc(methodName: string, args: Array<unknown> = []) {
- return await jobRunnerRpc(this.env, this.ctx.props.jobId).callJobRpc({
- jobId: this.ctx.props.jobId,
- facetName: this.ctx.props.facetName,
- methodName,
- args,
- })
- }
-
- async fetchWithResolvedSecrets(input: {
- url: string
- method?: string
- headers?: Record<string, string>
- body?: string
- }) {
- const request = new Request(input.url, {
- method: input.method ?? 'GET',
- headers: input.headers,
- body: input.body,
- })
- const resolved = await expandSecretPlaceholders({
- request,
- props: {
- baseUrl: this.ctx.props.baseUrl,
- userId: this.ctx.props.userId,
- storageContext: this.ctx.props.storageContext,
- },
- env: this.env,
- })
- return {
- url: resolved.url,
- method: resolved.method,
- headers: Object.fromEntries(resolved.headers.entries()),
- body:
- resolved.method === 'GET' || resolved.method === 'HEAD'
- ? undefined
- : await resolved.text(),
- }
- }
-
- async connectorGet(args: Record<string, unknown>) {
- const name = typeof args['name'] === 'string' ? args['name'].trim() : ''
- if (!name) {
- throw new Error('connector_get requires a connector name.')
- }
- const value = await getValue({
- env: this.env,
- userId: this.ctx.props.userId,
- name: buildConnectorValueName(name),
- scope: 'user',
- storageContext: this.ctx.props.storageContext,
- })
- if (!value) {
- return { connector: null }
- }
- return {
- connector: parseConnectorConfig(parseConnectorJson(value.value), name),
- }
- }
-
- async connectorList() {
- const values = await listValues({
- env: this.env,
- userId: this.ctx.props.userId,
- scope: 'user',
- storageContext: this.ctx.props.storageContext,
- })
- const connectors = values
- .map((value) => {
- const name = value.name.startsWith('_connector:')
- ? value.name.slice('_connector:'.length).trim()
- : null
- if (!name) return null
- return parseConnectorConfig(parseConnectorJson(value.value), name)
- })
- .filter((value): value is NonNullable<typeof value> => value != null)
- return { connectors }
- }
-
- async valueGet(name: string, scope: 'app' | 'user' = 'app') {
- return await getValue({
- env: this.env,
- userId: this.ctx.props.userId,
- name,
- scope,
- storageContext: this.ctx.props.storageContext,
- })
- }
-
- async valueSet(input: {
- name: string
- value: string
- description?: string
- scope?: 'app' | 'user'
- }) {
- return await saveValue({
- env: this.env,
- userId: this.ctx.props.userId,
- name: input.name,
- value: input.value,
- description: input.description ?? '',
- scope: input.scope ?? 'app',
- storageContext: this.ctx.props.storageContext,
- })
- }
-
- async secretPlaceholder(name: string, scope: 'app' | 'user' = 'app') {
- const resolved = await resolveSecret({
- env: this.env,
- userId: this.ctx.props.userId,
- name,
- scope,
- storageContext: this.ctx.props.storageContext,
- })
- if (!resolved.found) {
- throw new Error(`Secret "${name}" was not found.`)
- }
- return `{{secret:${name}|scope=${scope}}}`
- }
-
- async metaRunSkill(
- name: string,
- params: Record<string, unknown> | undefined,
- ) {
- const { metaRunSkillCapability } =
- await import('#mcp/capabilities/meta/meta-run-skill.ts')
- const { createMcpCallerContext } = await import('#mcp/context.ts')
- return await metaRunSkillCapability.handler(
- { name, params },
- {
- env: this.env,
- callerContext: createMcpCallerContext({
- baseUrl: this.ctx.props.baseUrl,
- user: {
- userId: this.ctx.props.userId,
- email: '',
- displayName: `job:${this.ctx.props.jobId}`,
- },
- storageContext: this.ctx.props.storageContext,
- }),
- },
- )
- }
-}
-
-class JobRunnerBase extends DurableObject<Env> {
- async configure(input: {
- jobId: string
- userId: string
- baseUrl?: string
- storageContext?: {
- sessionId?: string | null
- appId?: string | null
- } | null
- serverCode: string
- serverCodeId: string
- methodName?: string | null
- killSwitchEnabled?: boolean
- }) {
- const existing = await this.readConfig(input.jobId)
- const nextConfig: JobRunnerConfig = {
- ...existing,
- jobId: input.jobId,
- userId: input.userId,
- baseUrl: input.baseUrl ?? existing.baseUrl,
- storageContext:
- input.storageContext === undefined
- ? existing.storageContext
- : normalizeStorageContext(input.storageContext),
- serverCode: input.serverCode,
- serverCodeId: input.serverCodeId,
- methodName: input.methodName?.trim() || existing.methodName || 'run',
- killSwitchEnabled: input.killSwitchEnabled ?? existing.killSwitchEnabled,
- }
- await this.writeConfig(nextConfig)
- const codeChanged =
- existing.serverCodeId !== nextConfig.serverCodeId ||
- existing.serverCode !== nextConfig.serverCode
- const bridgeChanged =
- existing.userId !== nextConfig.userId ||
- existing.baseUrl !== nextConfig.baseUrl ||
- JSON.stringify(existing.storageContext) !==
- JSON.stringify(nextConfig.storageContext)
- if (codeChanged || bridgeChanged) {
- for (const facetName of nextConfig.facetNames) {
- this.ctx.facets.abort(
- facetName,
- new Error(
- codeChanged ? 'Job code updated.' : 'Job bridge context updated.',
- ),
- )
- }
- }
- return nextConfig
- }
-
- async resetStorage(input: { jobId: string; facetName?: string | null }) {
- const facetName = input.facetName?.trim() || defaultJobFacetName
- const facet = await this.getFacetStub(input.jobId, facetName)
- await (
- facet as unknown as { __kody_resetStorage: () => Promise<unknown> }
- ).__kody_resetStorage()
- this.ctx.facets.abort(facetName, new Error('Facet storage reset.'))
- return {
- ok: true,
- jobId: input.jobId,
- facetName,
- }
- }
-
- async exportStorage(input: {
- jobId: string
- facetName?: string | null
- pageSize?: number
- startAfter?: string | null
- }) {
- const facetName = input.facetName?.trim() || defaultJobFacetName
- const facet = await this.getFacetStub(input.jobId, facetName)
- const result = await (
- facet as unknown as {
- __kody_exportStorage: (payload?: {
- pageSize?: number
- startAfter?: string | null
- }) => Promise<JobStorageExport>
- }
- ).__kody_exportStorage({
- pageSize: input.pageSize,
- startAfter: input.startAfter ?? null,
- })
- return {
- ok: true,
- jobId: input.jobId,
- facetName,
- export: result,
- }
- }
-
- async runStoredJob(input: {
- jobId: string
- facetName?: string | null
- methodName?: string | null
- params?: Record<string, unknown>
- }) {
- const config = await this.readConfig(input.jobId)
- const methodName = input.methodName?.trim() || config.methodName || 'run'
- const result = await this.callJobRpc({
- jobId: input.jobId,
- facetName: input.facetName ?? defaultJobFacetName,
- methodName,
- args: input.params === undefined ? [] : [input.params],
- })
- return {
- ok: true,
- jobId: input.jobId,
... diff truncated: showing 800 of 3898 linesYou can send follow-ups to the cloud agent here. |
Co-authored-by: me <me@kentcdodds.com>
cursor
Bot
force-pushed
the
cursor/unify-job-runtime-f161
branch
from
April 14, 2026 23:29
33eb0fc to
ed8dfda
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/worker/src/jobs/service.node.test.ts (1)
6-6:⚠️ Potential issue | 🟡 MinorRemove unused import
updateJob.Static analysis indicates
updateJobis imported but never used in this test file.🧹 Suggested fix
-import { createJob, executeJobOnce, updateJob } from './service.ts' +import { createJob, executeJobOnce } from './service.ts'🤖 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` at line 6, The test imports an unused symbol updateJob from './service.ts'; remove updateJob from the import list in the test (leave createJob and executeJobOnce) so the import statement only imports used functions and eliminates the unused-import lint error; update the import line that currently reads import { createJob, executeJobOnce, updateJob } from './service.ts' to exclude updateJob.packages/worker/src/storage-runner.ts (3)
94-118:⚠️ Potential issue | 🔴 CriticalUnresolved: Read-only SQL guard is bypassable with multi-statement queries.
The
assertSqlAllowedfunction only validates the leading prefix, so a query likeSELECT 1; DROP TABLE kv;passes the read-only check. Since Cloudflare Durable Objects'ctx.storage.sql.exec()executes all semicolon-separated statements, this allows write operations in read-only mode.🛡️ Suggested fix
function assertSqlAllowed(query: string, writable: boolean | undefined) { const trimmed = query.trim() if (!trimmed) { throw new Error('storage.sql requires a non-empty query.') } if (writable) return trimmed + // Reject multi-statement queries in read-only mode + if (trimmed.includes(';')) { + throw new Error( + 'Read-only storage.sql does not allow multi-statement queries. Remove semicolons or pass writable: true.', + ) + } const normalized = trimmed.toLowerCase() const allowedReadOnlyPrefixes = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/storage-runner.ts` around lines 94 - 118, The read-only guard in assertSqlAllowed is bypassable via multi-statement SQL (e.g., "SELECT 1; DROP TABLE ...") because it only checks the leading prefix; update assertSqlAllowed (used before calling ctx.storage.sql.exec()) to either reject queries containing multiple statements (detect semicolons outside quotes/comments) when writable is false, or split the SQL into individual statements and validate each one against the allowedReadOnlyPrefixes; ensure the writable parameter still permits multi-statement mutations and that the new check handles whitespace/comments and quoted semicolons safely so only truly single or all-read-only statements are allowed in read-only mode.
120-131:⚠️ Potential issue | 🟠 MajorUnresolved: SQL query results are unbounded and can exhaust memory.
The
cursorToSqlResultfunction callscursor.toArray()which materializes the entire result set. UnlikelistValueswhich enforces pagination (max 1,000 rows),sqlQueryhas no safeguards against queries returning millions of rows.🛡️ Suggested fix — enforce a row limit
+const maxSqlResultRows = 1_000 + function cursorToSqlResult( cursor: SqlStorageCursor<Record<string, StorageSqlValue>>, ): StorageSqlResult { - const rows = cursor.toArray() + const rows: Array<Record<string, StorageSqlValue>> = [] + for (const row of cursor) { + if (rows.length >= maxSqlResultRows) break + rows.push(row) + } return { columns: [...cursor.columnNames], rows, rowCount: rows.length, rowsRead: cursor.rowsRead, rowsWritten: cursor.rowsWritten, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/storage-runner.ts` around lines 120 - 131, cursorToSqlResult currently calls cursor.toArray() and can materialize an unbounded result set from sqlQuery; change cursorToSqlResult to enforce a hard row limit (e.g. reuse the listValues pagination cap like MAX_LIST_ROWS or introduce MAX_SQL_ROWS = 1000), iterate the cursor and collect at most that many rows rather than calling toArray(), set rowCount to the collected size, preserve rowsRead/rowsWritten, and if the result was truncated either attach a flag (e.g. truncated: true) or log/warn so callers know the output was limited; update any callers of sqlQuery/sqlCursor expectations accordingly (refer to cursorToSqlResult and sqlQuery).
48-50:⚠️ Potential issue | 🟠 MajorUnresolved: Durable object name collision risk.
The
buildStorageRunnerNamefunction still concatenatesuserIdandstorageIdwith:, butstorageIdalready contains:(e.g.,job:uuid). This makes the DO name ambiguous:(userId = "a:b", storageId = "c")produces the same name as(userId = "a", storageId = "b:c").🔧 Suggested fix
function buildStorageRunnerName(userId: string, storageId: string) { - return `${userId}:${storageId}` + return JSON.stringify([userId, storageId]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/storage-runner.ts` around lines 48 - 50, buildStorageRunnerName currently concatenates userId and storageId with ":" which allows collisions when either part contains ":"; update buildStorageRunnerName to produce an unambiguous name by encoding each component (e.g., base64url encode userId and storageId) and then join them (e.g., `${b64(userId)}:${b64(storageId)}`) or alternatively include explicit lengths (e.g., `${userId.length}:${userId}:${storageId.length}:${storageId}`); change the implementation in the buildStorageRunnerName function to use one of these approaches so DO names cannot collide.
🧹 Nitpick comments (2)
packages/worker/src/storage-runner.workers.test.ts (1)
131-151: Consider usingexpect().rejectsfor cleaner error assertion.The try/catch pattern works but
expect().rejectsprovides a more declarative assertion style.♻️ Optional refactor using expect().rejects
-test('storage runner blocks mutating SQL when writable is false', async () => { - const storageId = createExecuteStorageId() - const runner = storageRunnerRpc({ - env, - userId: 'user-123', - storageId, - }) - - try { - await runner.sqlQuery({ - query: 'delete from counters', - writable: false, - }) - throw new Error('Expected read-only SQL mutation to fail.') - } catch (error) { - expect(error).toBeInstanceOf(Error) - expect((error as Error).message).toBe( - 'Read-only storage.sql only allows SELECT, EXPLAIN, and schema PRAGMA queries. Pass writable: true to allow mutations.', - ) - } -}) +test('storage runner blocks mutating SQL when writable is false', async () => { + const storageId = createExecuteStorageId() + const runner = storageRunnerRpc({ + env, + userId: 'user-123', + storageId, + }) + + await expect( + runner.sqlQuery({ + query: 'delete from counters', + writable: false, + }), + ).rejects.toThrow( + 'Read-only storage.sql only allows SELECT, EXPLAIN, and schema PRAGMA queries. Pass writable: true to allow mutations.', + ) +})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/storage-runner.workers.test.ts` around lines 131 - 151, Replace the try/catch assertion in the "storage runner blocks mutating SQL when writable is false" test with a declarative promise rejection assertion: call runner.sqlQuery with the same payload and use Jest's expect(...).rejects matcher to assert it rejects with an Error and the exact message; locate the test by its name and the call to runner.sqlQuery({ query: 'delete from counters', writable: false }) and substitute the try/catch block with an await expect(runner.sqlQuery(...)).rejects.toThrow( /* expected message */ ) (or equivalent rejects.toEqual/error-message assertion).packages/worker/src/mcp/run-codemode-registry.ts (1)
452-562: DuplicatedcreateStorageCodemodeToolsimplementations across files.This file defines
createStorageCodemodeToolslocally (lines 452–562), whilepackages/worker/src/storage-runner.tsalso exports acreateStorageCodemodeToolsfunction (lines 262–345). The implementations differ subtly in how they handle the writable flag and tool inclusion:
- Here: Always includes all storage tools but guards mutations with
ensureWritable()at runtime.- storage-runner.ts: Conditionally includes mutation tools only when
writable: true.Consider consolidating into a single implementation in
storage-runner.tsand importing it here to avoid divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/run-codemode-registry.ts` around lines 452 - 562, The PR duplicates createStorageCodemodeTools: this file defines a local createStorageCodemodeTools (with ensureWritable guards and always-exported mutation tools) while storage-runner.ts already exports a differing implementation; consolidate by removing the local implementation and importing the canonical createStorageCodemodeTools from storage-runner.ts, update callers here to use the imported function, and ensure the exported tool set and writable behavior match the storage-runner.ts version (verify ensureWritable, storage_get/storage_set/storage_delete/storage_list/storage_clear/storage_sql symbols remain supported).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/jobs/service.node.test.ts`:
- Line 6: The test imports an unused symbol updateJob from './service.ts';
remove updateJob from the import list in the test (leave createJob and
executeJobOnce) so the import statement only imports used functions and
eliminates the unused-import lint error; update the import line that currently
reads import { createJob, executeJobOnce, updateJob } from './service.ts' to
exclude updateJob.
In `@packages/worker/src/storage-runner.ts`:
- Around line 94-118: The read-only guard in assertSqlAllowed is bypassable via
multi-statement SQL (e.g., "SELECT 1; DROP TABLE ...") because it only checks
the leading prefix; update assertSqlAllowed (used before calling
ctx.storage.sql.exec()) to either reject queries containing multiple statements
(detect semicolons outside quotes/comments) when writable is false, or split the
SQL into individual statements and validate each one against the
allowedReadOnlyPrefixes; ensure the writable parameter still permits
multi-statement mutations and that the new check handles whitespace/comments and
quoted semicolons safely so only truly single or all-read-only statements are
allowed in read-only mode.
- Around line 120-131: cursorToSqlResult currently calls cursor.toArray() and
can materialize an unbounded result set from sqlQuery; change cursorToSqlResult
to enforce a hard row limit (e.g. reuse the listValues pagination cap like
MAX_LIST_ROWS or introduce MAX_SQL_ROWS = 1000), iterate the cursor and collect
at most that many rows rather than calling toArray(), set rowCount to the
collected size, preserve rowsRead/rowsWritten, and if the result was truncated
either attach a flag (e.g. truncated: true) or log/warn so callers know the
output was limited; update any callers of sqlQuery/sqlCursor expectations
accordingly (refer to cursorToSqlResult and sqlQuery).
- Around line 48-50: buildStorageRunnerName currently concatenates userId and
storageId with ":" which allows collisions when either part contains ":"; update
buildStorageRunnerName to produce an unambiguous name by encoding each component
(e.g., base64url encode userId and storageId) and then join them (e.g.,
`${b64(userId)}:${b64(storageId)}`) or alternatively include explicit lengths
(e.g., `${userId.length}:${userId}:${storageId.length}:${storageId}`); change
the implementation in the buildStorageRunnerName function to use one of these
approaches so DO names cannot collide.
---
Nitpick comments:
In `@packages/worker/src/mcp/run-codemode-registry.ts`:
- Around line 452-562: The PR duplicates createStorageCodemodeTools: this file
defines a local createStorageCodemodeTools (with ensureWritable guards and
always-exported mutation tools) while storage-runner.ts already exports a
differing implementation; consolidate by removing the local implementation and
importing the canonical createStorageCodemodeTools from storage-runner.ts,
update callers here to use the imported function, and ensure the exported tool
set and writable behavior match the storage-runner.ts version (verify
ensureWritable,
storage_get/storage_set/storage_delete/storage_list/storage_clear/storage_sql
symbols remain supported).
In `@packages/worker/src/storage-runner.workers.test.ts`:
- Around line 131-151: Replace the try/catch assertion in the "storage runner
blocks mutating SQL when writable is false" test with a declarative promise
rejection assertion: call runner.sqlQuery with the same payload and use Jest's
expect(...).rejects matcher to assert it rejects with an Error and the exact
message; locate the test by its name and the call to runner.sqlQuery({ query:
'delete from counters', writable: false }) and substitute the try/catch block
with an await expect(runner.sqlQuery(...)).rejects.toThrow( /* expected message
*/ ) (or equivalent rejects.toEqual/error-message assertion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: da271e34-b943-4927-990d-7d6594dde638
📒 Files selected for processing (48)
docs/contributing/architecture/data-storage.mddocs/use/execute.mdpackages/shared/src/chat.tspackages/worker/migrations/0020-jobs-codemode-only.sqlpackages/worker/migrations/0021-jobs-storage-id.sqlpackages/worker/src/env-schema.tspackages/worker/src/index.tspackages/worker/src/jobs/job-runner.tspackages/worker/src/jobs/job-runner.workers.test.tspackages/worker/src/jobs/jobs-codemode-only-migration.node.test.tspackages/worker/src/jobs/process-due-jobs.node.test.tspackages/worker/src/jobs/repo.tspackages/worker/src/jobs/service.node.test.tspackages/worker/src/jobs/service.tspackages/worker/src/jobs/types.tspackages/worker/src/mcp/app-runner.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/job-upsert.tspackages/worker/src/mcp/capabilities/jobs/shared.tspackages/worker/src/mcp/capabilities/meta/meta-run-skill.tspackages/worker/src/mcp/capabilities/secrets/secret-delete.tspackages/worker/src/mcp/capabilities/secrets/secret-list.tspackages/worker/src/mcp/capabilities/secrets/secret-set.tspackages/worker/src/mcp/capabilities/storage/domain.tspackages/worker/src/mcp/capabilities/storage/shared.tspackages/worker/src/mcp/capabilities/storage/storage-export.tspackages/worker/src/mcp/capabilities/storage/storage-query.tspackages/worker/src/mcp/capabilities/values/connector-get.tspackages/worker/src/mcp/capabilities/values/connector-list.tspackages/worker/src/mcp/capabilities/values/value-delete.tspackages/worker/src/mcp/capabilities/values/value-get.tspackages/worker/src/mcp/capabilities/values/value-list.tspackages/worker/src/mcp/capabilities/values/value-set.tspackages/worker/src/mcp/generated-ui-api.tspackages/worker/src/mcp/mcp-server.mcp-e2e.test.tspackages/worker/src/mcp/observability.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/mcp/storage-bindings.tspackages/worker/src/mcp/storage.tspackages/worker/src/mcp/tools/execute.node.test.tspackages/worker/src/mcp/tools/execute.tspackages/worker/src/oauth-handlers.workers.test.tspackages/worker/src/storage-runner.tspackages/worker/src/storage-runner.workers.test.tspackages/worker/worker-configuration.d.tspackages/worker/wrangler.jsonc
💤 Files with no reviewable changes (3)
- packages/worker/src/jobs/process-due-jobs.node.test.ts
- packages/worker/src/jobs/job-runner.workers.test.ts
- packages/worker/src/jobs/job-runner.ts
✅ Files skipped from review due to trivial changes (7)
- packages/worker/src/mcp/app-runner.ts
- packages/worker/src/mcp/capabilities/jobs/domain.ts
- packages/worker/src/mcp/capabilities/domain-metadata.ts
- packages/worker/src/mcp/capabilities/builtin-domains.ts
- packages/worker/src/jobs/repo.ts
- packages/worker/migrations/0021-jobs-storage-id.sql
- packages/worker/src/mcp/capabilities/storage/shared.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- packages/worker/src/mcp/storage.ts
- packages/worker/src/mcp/capabilities/meta/meta-run-skill.ts
- packages/shared/src/chat.ts
- packages/worker/src/mcp/capabilities/storage/domain.ts
- packages/worker/src/mcp/observability.ts
- packages/worker/src/index.ts
- packages/worker/src/mcp/capabilities/jobs/job-upsert.ts
- docs/use/execute.md
- packages/worker/src/mcp/capabilities/storage/storage-query.ts
- packages/worker/src/env-schema.ts
- packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts
- packages/worker/migrations/0020-jobs-codemode-only.sql
- packages/worker/src/mcp/tools/execute.ts
- docs/contributing/architecture/data-storage.md
- packages/worker/src/mcp/capabilities/storage/storage-export.ts
- packages/worker/src/mcp/storage-bindings.ts
- packages/worker/worker-configuration.d.ts
- packages/worker/src/jobs/types.ts
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Co-authored-by: me <me@kentcdodds.com>
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 3 total unresolved issues (including 1 from previous review).
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Reviewed by Cursor Bugbot for commit fd1af51. Configure here.
| writable: storageTools.writable, | ||
| }) | ||
| : {}), | ||
| ...additionalTools, |
Contributor
There was a problem hiding this comment.
Storage tools silently override capabilities without collision check
Medium Severity
buildCodemodeFns checks additionalTools for name collisions against capabilityMap but applies no such check for the storage codemode tools (storage_get, storage_list, storage_sql, etc.). These tools are spread after capabilities, silently overriding any capability with a matching name. Today the names don't collide, but the storageDomain already registers capabilities in the same storage_* namespace (storage_export, storage_query), making a future collision likely if new storage capabilities or tools are added.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit fd1af51. Configure here.
| if (trimmed.includes(';')) { | ||
| throw new Error( | ||
| 'Read-only storage.sql only allows a single SELECT, EXPLAIN, or schema PRAGMA statement. Pass writable: true to allow multi-statement or mutating queries.', | ||
| ) |
Contributor
There was a problem hiding this comment.
Read-only SQL check blocks semicolons inside string literals
Low Severity
assertSqlAllowed rejects any read-only query whose text contains a semicolon via trimmed.includes(';'), which also matches semicolons inside SQL string literals (e.g., SELECT 'a;b' AS val). Valid single-statement read-only queries that happen to include a literal semicolon character will be incorrectly rejected, forcing users to switch to writable: true even though the query is genuinely read-only.
Reviewed by Cursor Bugbot for commit fd1af51. Configure here.
This was referenced Apr 15, 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
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.
Summary
storageIds and a sharedStorageRunnerexecutewithstorageId/writableplus built-instorage.*andstorage.sql(...)storage_queryandstorage_exportcapabilities for read-only inspection by defaultstorage_idand remove the oldJobRunnerpath from the worker surfaceTesting
npm run test -- --run packages/worker/src/mcp/run-codemode-registry.node.test.ts packages/worker/src/mcp/tools/execute.node.test.ts packages/worker/src/jobs/service.node.test.ts packages/worker/src/jobs/jobs-codemode-only-migration.node.test.ts packages/worker/src/storage-runner.workers.test.tsnpm run test:mcp -- packages/worker/src/mcp/mcp-server.mcp-e2e.test.tsnpm run typechecknpm run buildSummary by CodeRabbit
New Features
Changes
Documentation
Tests