Skip to content

fix(ai-isolate-cloudflare): port worker from unsafe_eval to worker_loader#523

Open
Sriketk wants to merge 3 commits intoTanStack:mainfrom
Sriketk:fix/ai-isolate-cloudflare-worker-loader
Open

fix(ai-isolate-cloudflare): port worker from unsafe_eval to worker_loader#523
Sriketk wants to merge 3 commits intoTanStack:mainfrom
Sriketk:fix/ai-isolate-cloudflare-worker-loader

Conversation

@Sriketk
Copy link
Copy Markdown

@Sriketk Sriketk commented May 3, 2026

🎯 Changes

Port @tanstack/ai-isolate-cloudflare worker from the unsafe_eval binding to worker_loader (Dynamic Workers).

Closes #522.

Why

unsafe_eval is workerd-internal and gated by Cloudflare for all customer accounts — there is no public entitlement and no path to enable it. The current driver is unusable in production and broken in wrangler dev on Wrangler 4.x:

Attempt Result
wrangler deploy binding UNSAFE_EVAL has an unknown type eval [code: 10021]
wrangler dev binding mode remote, runtime returns UnsafeEvalNotAvailable
wrangler dev --local (3.114) Fatal: setsocketopt TCP_NODELAY crash

Cloudflare's supported replacement is the worker_loader (Dynamic Workers) binding, GA-beta'd 2026-03-24.

What changed

  • src/worker/index.ts: swap env.UNSAFE_EVAL.eval(wrappedCode) for env.LOADER.load({modules}).getEntrypoint().fetch(...). Wraps the existing IIFE-returning string in an ES module that exposes a fetch handler returning the structured result as JSON.
  • wrangler.toml: [[unsafe.bindings]][[worker_loaders]] binding = "LOADER". Compat date bumped to 2026-05-01.
  • Error renamed: UnsafeEvalNotAvailableWorkerLoaderNotAvailable.
  • Tests updated to mock the new binding shape.
  • dev-server.mjs removed: wrangler dev binds worker_loader natively, so the custom Miniflare bootstrap is no longer needed. dev:worker script now runs wrangler dev.
  • README updated for the new plan / binding requirements.

The HTTP tool-callback protocol, driver code, and public API are unchanged. ~120 LOC change.

Validation

  • pnpm test:lib — 36/36 pass
  • pnpm test:types — clean
  • pnpm test:eslint — clean
  • Validated end-to-end on a real CF deployment (a bun patch of the published 0.1.8 with the same swap, deployed at cobalt-sandbox.<account>.workers.dev, e2e calling tool-callback round-trip against a Drizzle-backed driver).

Plan / cost

worker_loader is Workers Paid plan only. The CF API rejects deploys and --remote on Free with code: 10195"In order to use Dynamic Workers, you must switch to a paid plan." Local wrangler dev accepts the binding on the Free plan, so inner-loop iteration stays free.

This is a tier requirement Cloudflare enforces, not a regression in this PR — unsafe_eval would have required Paid in any account that ever managed to enable it. The README now states the plan requirement explicitly.

Breaking

Yes — minor bump. Consumers must update wrangler.toml:

# before
[[unsafe.bindings]]
name = "UNSAFE_EVAL"
type = "unsafe_eval"

# after
[[worker_loaders]]
binding = "LOADER"

✅ Checklist

  • I have followed the steps in the Contributing guide.
  • I have tested this code locally with pnpm run test:pr (package-scoped tests pass).

🚀 Release Impact

  • This change affects published code, and I have generated a changeset.
  • This change is docs/CI/dev-only (no release).

Summary by CodeRabbit

  • Breaking Changes

    • Worker deployment now requires a LOADER (worker_loader) binding instead of the previous unsafe-eval binding.
  • Documentation

    • Setup and deployment guidance updated for the wrangler dev / worker_loader workflow, with Free vs Paid plan considerations and failure-mode guidance.
  • Chores

    • Local development now uses wrangler dev.
    • Cloudflare compatibility date updated to 2026-05-01.

…ader

Cloudflare gates the `unsafe_eval` binding for all customer prod
accounts (no public entitlement); the previous driver was unusable in
production and broken in `wrangler dev` on current Wrangler 4.x.

Swap `env.UNSAFE_EVAL.eval(code)` for the supported `worker_loader`
(Dynamic Workers) binding — load the wrapped code as an ES module into
a fresh child Worker isolate via `env.LOADER.load({...}).getEntrypoint()
.fetch(...)` and read the structured result back as JSON.

The HTTP tool-callback protocol, driver, and public API are unchanged.
~120 LOC change in worker; tests + wrangler.toml + README updated.

Workers Paid plan is required for any edge usage (deploy or
`wrangler dev --remote`); local `wrangler dev` works on the Free plan.
The custom Miniflare `dev-server.mjs` is removed since `wrangler dev`
now binds `worker_loader` natively.

Closes TanStack#522.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 3, 2026

📝 Walkthrough

Walkthrough

This PR ports the Cloudflare worker in @tanstack/ai-isolate-cloudflare from the gated unsafe_eval binding to the worker_loader (Dynamic Workers) binding, updating configuration, implementation, dev scripts, tests, and docs while keeping the public API and tool-callback protocol unchanged.

Changes

Worker Loader Migration

Layer / File(s) Summary
Configuration & Breaking Change
packages/typescript/ai-isolate-cloudflare/wrangler.toml, .changeset/worker-loader-port.md
compatibility_date pinned to 2026-05-01; removed unsafe_eval binding and added [[worker_loaders]] binding = "LOADER"; changeset documents breaking binding change and Workers plan requirements.
Public Types / Env
packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
Introduces SANDBOX_COMPAT_DATE, WorkerLoader/LoadedWorker types and Env with optional LOADER?: WorkerLoader.
Code Wrapping
packages/typescript/ai-isolate-cloudflare/src/worker/index.ts, .../wrap-code.ts
Adds wrapAsSandboxModule(wrappedCode) to emit an ES module exporting fetch; updates module comment to reference worker_loader testability.
Core Execution Flow
packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
executeCode(...) now errors with WorkerLoaderNotAvailable when env.LOADER is missing, uses env.LOADER.load(...) to create a child Worker, calls getEntrypoint().fetch, races against an AbortController timeout sentinel, maps timeouts to TimeoutError, maps child execution failures to structured done with success: false, preserves need_tools continuation behavior.
Worker Handler Wiring
packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
Fetch handler retains CORS/POST/JSON validation and response shapes; now operates with the LOADER-based Env.
Development Workflow
packages/typescript/ai-isolate-cloudflare/package.json, packages/typescript/ai-isolate-cloudflare/dev-server.mjs (removed)
Removed Miniflare-based dev-server.mjs; dev:worker script changed to wrangler dev.
Documentation & Setup Guide
packages/typescript/ai-isolate-cloudflare/README.md, .changeset/worker-loader-port.md
Replaced Miniflare/unsafe_eval instructions with wrangler dev/worker_loader guidance; documents Free-plan rejection behavior and WorkerLoaderNotAvailable failure mode; security guidance retained.
Tests & Validation
packages/typescript/ai-isolate-cloudflare/tests/*
Updated tests to mock/use env.LOADER instead of env.UNSAFE_EVAL; assertions changed to WorkerLoaderNotAvailable; added happy-path and need_tools forwarding tests; timeout test updated to assert AbortSignal behavior.

Sequence Diagram

sequenceDiagram
    actor Client
    participant MainWorker as Main Worker (ai-isolate)
    participant LoaderBinding as LOADER Binding
    participant ChildWorker as Child Worker (Sandbox)

    Client->>MainWorker: POST /execute (code + context)
    MainWorker->>MainWorker: wrapAsSandboxModule(code) -> module source
    MainWorker->>LoaderBinding: load({ mainModule, modules, compatibilityDate })
    LoaderBinding->>ChildWorker: initialize isolate with module
    MainWorker->>ChildWorker: getEntrypoint().fetch(Request, { signal })
    ChildWorker->>ChildWorker: run wrapped IIFE, produce JSON result
    ChildWorker-->>MainWorker: Response with result JSON
    MainWorker->>MainWorker: parse result, handle need_tools/done/errors/timeouts
    MainWorker-->>Client: JSON { status, value/error, toolCalls?, continuationId? }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I swapped the old Unsafe for LOADER's light,
Spawned a child to sandbox each midnight.
Fetch calls hum, timeouts softly chime,
Tests pass on the meadow—one hop at a time.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately summarizes the main change: porting the worker from unsafe_eval to worker_loader binding.
Description check ✅ Passed Description covers all required template sections with comprehensive details on changes, motivation, validation, and breaking changes.
Linked Issues check ✅ Passed All objectives from #522 are met: unsafe_eval replaced with worker_loader, HTTP protocol and API preserved, ~120 LOC change, plan requirements documented, and validation performed.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the worker_loader migration: binding updates, error renaming, code wrapping, test mocks, and documentation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value).


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.

❤️ Share
Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts (1)

191-209: ⚡ Quick win

Please add one success-path LOADER test here as well.

Right now this file only locks down the missing-binding branch. The new LOADER.load(...).getEntrypoint().fetch(...) flow is the real regression surface in this PR, and a small mocked happy-path test would catch contract drift in mainModule, modules, entrypoint resolution, and JSON handling.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts` around lines
191 - 209, Add a happy-path test that supplies a mocked LOADER binding to the
worker invocation and asserts the full load→getEntrypoint→fetch flow: create an
env object with LOADER having a load() method that returns an object whose
getEntrypoint() returns an object with an async fetch(request) that returns a
Response (e.g., JSON { status: 'ok', result: ... }), call worker.fetch(request,
env, mockExecutionContext) using the same request shape as the existing test,
then assert response.status === 200 and the parsed JSON matches the expected
success payload (e.g., json.status === 'ok' and contains the returned result);
this will exercise the LOADER.load, getEntrypoint, and entrypoint.fetch contract
used in the worker.
🤖 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/typescript/ai-isolate-cloudflare/src/worker/index.ts`:
- Around line 111-131: The Promise.race approach returns a timeout error but
leaves entrypoint.fetch running; replace the race with a cancellation-aware
flow: create an AbortController before calling
env.LOADER.load()/loaded.getEntrypoint() and pass controller.signal into
entrypoint.fetch if the loader supports AbortSignal, set timeoutId to call
controller.abort() (and reject with TIMEOUT_SENTINEL) on timeout, and clear the
timeout when fetch resolves; if the loader/entrypoint does not support
AbortSignal, instead call the loader/entrypoint termination/dispose API (if
available) when the timeout fires or else update docs to state this is a
response-time limit only—adjust handling around timeoutPromise,
TIMEOUT_SENTINEL, timeoutId, entrypoint.fetch, env.LOADER.load, and
loaded.getEntrypoint accordingly.

---

Nitpick comments:
In `@packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts`:
- Around line 191-209: Add a happy-path test that supplies a mocked LOADER
binding to the worker invocation and asserts the full load→getEntrypoint→fetch
flow: create an env object with LOADER having a load() method that returns an
object whose getEntrypoint() returns an object with an async fetch(request) that
returns a Response (e.g., JSON { status: 'ok', result: ... }), call
worker.fetch(request, env, mockExecutionContext) using the same request shape as
the existing test, then assert response.status === 200 and the parsed JSON
matches the expected success payload (e.g., json.status === 'ok' and contains
the returned result); this will exercise the LOADER.load, getEntrypoint, and
entrypoint.fetch contract used in the worker.
🪄 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: ee905ac4-2230-4b85-8d36-afc9224989c3

📥 Commits

Reviewing files that changed from the base of the PR and between ff33855 and 167d052.

📒 Files selected for processing (10)
  • .changeset/worker-loader-port.md
  • packages/typescript/ai-isolate-cloudflare/README.md
  • packages/typescript/ai-isolate-cloudflare/dev-server.mjs
  • packages/typescript/ai-isolate-cloudflare/package.json
  • packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
  • packages/typescript/ai-isolate-cloudflare/src/worker/wrap-code.ts
  • packages/typescript/ai-isolate-cloudflare/tests/escape-attempts.test.ts
  • packages/typescript/ai-isolate-cloudflare/tests/isolate-driver.test.ts
  • packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts
  • packages/typescript/ai-isolate-cloudflare/wrangler.toml
💤 Files with no reviewable changes (1)
  • packages/typescript/ai-isolate-cloudflare/dev-server.mjs

Comment thread packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
…-path tests

Address CodeRabbit review on TanStack#523:

1. Promise.race timeout left `entrypoint.fetch` running, leaking the loaded
   child Worker isolate. Add an AbortController whose signal flows into the
   Request passed to entrypoint.fetch — the timeout now actually cancels the
   in-flight request. Promise.race remains as a belt-and-suspenders guard.

2. Add three integration tests against a mocked LOADER binding:
   - happy path: full load → getEntrypoint → fetch chain, asserts the
     load() arguments (mainModule, modules, globalOutbound) and that the
     Request carries an AbortSignal
   - need_tools: forwards toolCalls + continuationId from sandbox
   - TimeoutError: AbortSignal-driven cancellation triggers the right
     error shape

Tests: 39/39 pass.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts (2)

223-228: ⚡ Quick win

Assertions inside load() mock may be silently swallowed by the outer worker's error handling.

load() is a synchronous function called inside the outer worker's implementation. If any expect(...) inside it throws (e.g., the toBeNull() on globalOutbound), the worker's try/catch will intercept that AssertionError and return a generic error response. The test then fails at a later downstream assertion (such as expect(json.status).toBe('done')) rather than at the actual failing line, making failures hard to diagnose.

Capture the options outside the mock and assert after worker.fetch() resolves:

♻️ Proposed refactor
+    let loadCalled = false
+    let capturedOptions: Parameters<NonNullable<typeof env['LOADER']>['load']>[0] | null = null
     let receivedSignal: AbortSignal | null = null
     const env = {
       LOADER: {
         load: (options: { ... }) => {
           loadCalled = true
-          // Sanity-check the load() arguments the worker passes.
-          expect(options.mainModule).toBe('main.js')
-          expect(options.modules).toHaveProperty('main.js')
-          expect(options.modules['main.js']).toContain('export default')
-          expect(options.globalOutbound).toBeNull()
+          capturedOptions = options
           return {
             getEntrypoint: () => ({
               fetch: async (req: Request) => { ... },
             }),
           }
         },
       },
     }

     // ... worker.fetch() call ...

     expect(loadCalled).toBe(true)
+    expect(capturedOptions!.mainModule).toBe('main.js')
+    expect(capturedOptions!.modules).toHaveProperty('main.js')
+    expect(capturedOptions!.modules['main.js']).toContain('export default')
+    expect(capturedOptions!.globalOutbound).toBeNull()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts` around lines
223 - 228, The assertions currently inside the synchronous load() mock can be
swallowed by the worker's try/catch; instead capture the options object and
loadCalled flag from inside the mock (e.g., set loadCalled = true and
savedOptions = options) and remove all expect(...) calls from inside load();
then after awaiting worker.fetch(...) resolve, assert on savedOptions
(expect(savedOptions.mainModule).toBe('main.js'),
expect(savedOptions.modules).toHaveProperty('main.js'),
expect(savedOptions.modules['main.js']).toContain('export default'),
expect(savedOptions.globalOutbound).toBeNull()) and on loadCalled and
json.status to surface assertion failures at their true origin.

232-232: ⚡ Quick win

expect(receivedSignal).not.toBeNull() is trivially true; the timeout test doesn't definitively exercise the AbortSignal path.

Per the WHATWG Fetch spec and Cloudflare Workers docs, Request.signal is always a non-null AbortSignal — a never-aborted one is synthesised if no signal is provided in RequestInit. So line 257 passes regardless of whether the outer worker constructs and threads an AbortController signal.

The timeout test's mock comment says "Never resolves on its own; relies on AbortSignal," but because the outer worker also has a Promise.race sentinel (per the PR description), the test passes via that sentinel even if the AbortController is never properly connected. Neither test therefore proves that the signal is the outer worker's own timed AbortController signal.

Fix: drop the weak not.toBeNull() assertion; instead, capture receivedSignal in the timeout test and assert receivedSignal?.aborted === true after worker.fetch() returns. This confirms the outer worker actually aborts the signal — not just that a signal exists.

♻️ Proposed refactor for the timeout test
  it('returns TimeoutError when entrypoint.fetch exceeds timeout', async () => {
+   let receivedSignal: AbortSignal | null = null
    const env = {
      LOADER: {
        load: () => ({
          getEntrypoint: () => ({
            fetch: (req: Request) =>
              new Promise<Response>((_resolve, reject) => {
+               receivedSignal = req.signal
                req.signal.addEventListener('abort', () => {
                  reject(new Error('aborted'))
                })
                // Never resolves on its own; relies on AbortSignal.
              }),
          }),
        }),
      },
    }
    // ...
    const response = await worker.fetch(request, env, mockExecutionContext)

+   // Confirms the outer worker aborts its own AbortController signal on timeout,
+   // not merely that Promise.race fired.
+   expect(receivedSignal?.aborted).toBe(true)

    expect(response.status).toBe(200)
    const json = await response.json()
    expect(json.status).toBe('error')
    expect(json.error.name).toBe('TimeoutError')
    expect(json.error.message).toContain('50ms')
  })

Also applies to: 257-257, 303-336

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts` at line 232,
The current timeout test only asserts expect(receivedSignal).not.toBeNull(),
which is ineffective because Request.signal is always non-null; modify the
timeout test to capture the receivedSignal from the mocked fetch handler (the
same variable currently set by receivedSignal = req.signal) and after calling
worker.fetch(...) assert that receivedSignal?.aborted === true to prove the
outer worker's AbortController actually aborted the signal; remove the weak
not.toBeNull() assertion and add the aborted check in the timeout test that
exercises the timeout-path used by the worker code (ensure the assertion runs
after the worker.fetch promise resolves).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts`:
- Around line 223-228: The assertions currently inside the synchronous load()
mock can be swallowed by the worker's try/catch; instead capture the options
object and loadCalled flag from inside the mock (e.g., set loadCalled = true and
savedOptions = options) and remove all expect(...) calls from inside load();
then after awaiting worker.fetch(...) resolve, assert on savedOptions
(expect(savedOptions.mainModule).toBe('main.js'),
expect(savedOptions.modules).toHaveProperty('main.js'),
expect(savedOptions.modules['main.js']).toContain('export default'),
expect(savedOptions.globalOutbound).toBeNull()) and on loadCalled and
json.status to surface assertion failures at their true origin.
- Line 232: The current timeout test only asserts
expect(receivedSignal).not.toBeNull(), which is ineffective because
Request.signal is always non-null; modify the timeout test to capture the
receivedSignal from the mocked fetch handler (the same variable currently set by
receivedSignal = req.signal) and after calling worker.fetch(...) assert that
receivedSignal?.aborted === true to prove the outer worker's AbortController
actually aborted the signal; remove the weak not.toBeNull() assertion and add
the aborted check in the timeout test that exercises the timeout-path used by
the worker code (ensure the assertion runs after the worker.fetch promise
resolves).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5e527ef4-c182-4ea4-a978-f32123c8d0a7

📥 Commits

Reviewing files that changed from the base of the PR and between 167d052 and 669a2aa.

📒 Files selected for processing (2)
  • packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
  • packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/typescript/ai-isolate-cloudflare/src/worker/index.ts

Address CodeRabbit second-pass review:

1. happy-path test: hoist load() argument assertions out of the synchronous
   mock. Inside load() they get swallowed by the worker's outer try/catch and
   surface as a generic 500. Capture options into a local + assert after
   worker.fetch() resolves.

2. timeout test: `expect(receivedSignal).not.toBeNull()` is trivially true
   per the Fetch spec (Request.signal is always present). Drop it from the
   happy-path test and instead assert `signal.aborted === true` in the
   timeout test, which actually proves the outer worker's AbortController
   fired.

3. worker fix: when the AbortController fires first, fetchPromise rejects
   before timeoutPromise. Detect the timeout via either TIMEOUT_SENTINEL or
   `controller.signal.aborted` so the right error surfaces regardless of
   which branch of the race wins.

Tests: 39/39 pass.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/typescript/ai-isolate-cloudflare/src/worker/index.ts`:
- Around line 140-147: The code directly assigns await response.json() to the
local result variable and then branches on its status; instead validate this
boundary payload with Zod before using it. Define a Zod schema that matches the
expected ExecuteResponse shape (fields: status, success, value, error
{name,message,stack?}, logs: string[], toolCalls?: ToolCallRequest[]), call
resultRaw = await response.json() and then parse it with schema.parse or
schema.safeParse, and handle a failed parse by throwing or returning a clear
protocol error; update the code paths that currently reference result (the
status branching and subsequent use of value/logs/error/toolCalls) to use the
validated parsed object instead.
- Around line 71-79: The child worker currently assumes the wrapped result is
JSON-serializable and lets JSON.stringify throw (losing structured error info);
update wrapAsSandboxModule (and the code path produced by wrapCode) to catch
serialization failures: after awaiting __result, attempt to JSON.stringify
inside a try/catch and, on error, construct a deterministic serializable
fallback (e.g., { ok: false, errorType: 'SerializeError', message: String(err),
valueType: typeof __result }) or attempt safe conversions (e.g., BigInt ->
string) then return that JSON in the Response instead of letting JSON.stringify
propagate; ensure the module exports the same envelope shape for success (e.g.,
{ ok: true, value: ... }) and failure so the parent can reliably parse
structured sandbox results.
🪄 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: 4f4ebfd5-c892-4c94-8625-c266567b8e89

📥 Commits

Reviewing files that changed from the base of the PR and between 669a2aa and b2ce85f.

📒 Files selected for processing (2)
  • packages/typescript/ai-isolate-cloudflare/src/worker/index.ts
  • packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/typescript/ai-isolate-cloudflare/tests/worker.test.ts

Comment on lines +71 to +79
function wrapAsSandboxModule(wrappedCode: string): string {
return `
export default {
async fetch() {
const __result = await ${wrappedCode};
return new Response(JSON.stringify(__result), {
headers: { 'Content-Type': 'application/json' },
});
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Catch serialization failures inside the child worker.

wrapCode() can produce value: unknown, but this path assumes the full result is JSON-serializable. Values like BigInt or circular objects will throw here, outside the wrapped IIFE, so the parent only sees an opaque fetch/JSON failure and loses the structured sandbox result.

Suggested fix
 export default {
   async fetch() {
     const __result = await ${wrappedCode};
-    return new Response(JSON.stringify(__result), {
-      headers: { 'Content-Type': 'application/json' },
-    });
+    try {
+      return new Response(JSON.stringify(__result), {
+        headers: { 'Content-Type': 'application/json' },
+      });
+    } catch (__error) {
+      return new Response(
+        JSON.stringify({
+          status: 'done',
+          success: false,
+          error: {
+            name: __error?.name ?? 'SerializationError',
+            message: __error?.message ?? String(__error),
+          },
+          logs: __result?.logs ?? [],
+        }),
+        {
+          headers: { 'Content-Type': 'application/json' },
+        },
+      );
+    }
   }
 };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-isolate-cloudflare/src/worker/index.ts` around lines
71 - 79, The child worker currently assumes the wrapped result is
JSON-serializable and lets JSON.stringify throw (losing structured error info);
update wrapAsSandboxModule (and the code path produced by wrapCode) to catch
serialization failures: after awaiting __result, attempt to JSON.stringify
inside a try/catch and, on error, construct a deterministic serializable
fallback (e.g., { ok: false, errorType: 'SerializeError', message: String(err),
valueType: typeof __result }) or attempt safe conversions (e.g., BigInt ->
string) then return that JSON in the Response instead of letting JSON.stringify
propagate; ensure the module exports the same envelope shape for success (e.g.,
{ ok: true, value: ... }) and failure so the parent can reliably parse
structured sandbox results.

Comment on lines +140 to +147
const result: {
status: string
success?: boolean
value?: unknown
error?: { name: string; message: string; stack?: string }
logs: Array<string>
toolCalls?: Array<ToolCallRequest>
}

clearTimeout(timeoutId)
} = await response.json()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Validate the child-worker payload with Zod at this boundary.

This now crosses a worker boundary, but response.json() is still trusted and cast directly into the expected shape. If the loaded worker returns a partial or malformed payload, lines 149-164 can silently emit an invalid ExecuteResponse instead of a clear protocol error. Please parse this with a Zod schema before branching on status.

As per coding guidelines, "packages/typescript//src//*.ts: Use Zod for schema validation and tool definition across the library".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/typescript/ai-isolate-cloudflare/src/worker/index.ts` around lines
140 - 147, The code directly assigns await response.json() to the local result
variable and then branches on its status; instead validate this boundary payload
with Zod before using it. Define a Zod schema that matches the expected
ExecuteResponse shape (fields: status, success, value, error
{name,message,stack?}, logs: string[], toolCalls?: ToolCallRequest[]), call
resultRaw = await response.json() and then parse it with schema.parse or
schema.safeParse, and handle a failed parse by throwing or returning a clear
protocol error; update the code paths that currently reference result (the
status branching and subsequent use of value/logs/error/toolCalls) to use the
validated parsed object instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ai-isolate-cloudflare: unsafe_eval is gated by Cloudflare; port to worker_loader

1 participant