feat(ai-isolate): add native QuickJS Code Mode isolate driver for Bun#750
feat(ai-isolate): add native QuickJS Code Mode isolate driver for Bun#750lithdew wants to merge 2 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Bun-native QuickJS isolate driver package, wires it into the example app and Code Mode docs, and includes validation, benchmarking, and release metadata updates. ChangesBun QuickJS Isolate Driver
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
examples/ts-code-mode-web/src/components/ToolSidebar.tsx (1)
106-111: 💤 Low valueConsider runtime detection for better UX.
The option is marked
available: trueunconditionally, so users on Node.js can select it but will encounter a runtime error when the driver attempts to create a context. While the description warns "requires running the server with Bun," detecting the runtime at page load and conditionally settingavailable: falseon Node would prevent confusing errors.🎨 Optional runtime detection pattern
{ id: 'quickjs-bun', name: 'QuickJS Bun', description: 'Native QuickJS engine (requires running the server with Bun)', - available: true, + available: typeof Bun !== 'undefined', },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@examples/ts-code-mode-web/src/components/ToolSidebar.tsx` around lines 106 - 111, The quickjs-bun item in ToolSidebar.tsx currently hard-codes available: true, which allows Node clients to select it and hit runtime errors; change it to compute availability at load time (e.g., derive a boolean like isBunRuntime from a server-provided endpoint or a prop/initial state and set available: isBunRuntime) so the entry for id 'quickjs-bun' is disabled when the server/runtime is not Bun; update the ToolSidebar component to fetch or accept the runtime indicator (or feature flag) and use that to set the available property for the 'quickjs-bun' item and the UI disabled state.packages/ai-isolate-quickjs-bun/src/isolate-driver.ts (1)
141-142: 💤 Low valueConsider caching the module import alongside the library.
importQuickJSBun()is called on everycreateContext()invocation. While JavaScript runtimes cache dynamic imports, you could align this with thelibraryPromisepattern for consistency and to make the caching explicit.♻️ Suggested refactor
-let libraryPromise: Promise<QuickJS> | undefined +let modulePromise: Promise<QuickJSBunModule> | undefined +let libraryPromise: Promise<QuickJS> | undefined +async function loadQuickJSModule(): Promise<QuickJSBunModule> { + modulePromise ??= importQuickJSBun() + try { + return await modulePromise + } catch (error) { + modulePromise = undefined + throw error + } +} async function loadQuickJSLibrary(): Promise<QuickJS> { - libraryPromise ??= importQuickJSBun().then((mod) => new mod.QuickJS()) + libraryPromise ??= loadQuickJSModule().then((mod) => new mod.QuickJS()) // ... }Then in
createContext:- const quickjs = await importQuickJSBun() + const quickjs = await loadQuickJSModule() const library = await loadQuickJSLibrary()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ai-isolate-quickjs-bun/src/isolate-driver.ts` around lines 141 - 142, importQuickJSBun() is being invoked on every createContext() call; make this explicit and consistent with the existing libraryPromise pattern by caching the dynamic import result. Add a top-level promise (e.g., quickjsModulePromise) that stores importQuickJSBun(), use that cached promise inside createContext() instead of calling importQuickJSBun() directly, and ensure you still await loadQuickJSLibrary() (libraryPromise) as before so both the module and library are only loaded once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/code-mode/code-mode-isolates.md`:
- Around line 115-120: The markdown link in the QuickJS Bun Driver paragraph
uses backticks around the link text
(`[quickjs-bun](https://github.com/superpowerdotcom/quickjs-bun)`) which
prevents it rendering as a proper clickable link; update the text in the QuickJS
Bun Driver section (the line containing
`[quickjs-bun](https://github.com/superpowerdotcom/quickjs-bun)`) to either use
monospace label with a linked URL like via the
[`quickjs-bun`](https://github.com/superpowerdotcom/quickjs-bun) package or a
normal link via the
[quickjs-bun](https://github.com/superpowerdotcom/quickjs-bun) package so the
link renders correctly.
---
Nitpick comments:
In `@examples/ts-code-mode-web/src/components/ToolSidebar.tsx`:
- Around line 106-111: The quickjs-bun item in ToolSidebar.tsx currently
hard-codes available: true, which allows Node clients to select it and hit
runtime errors; change it to compute availability at load time (e.g., derive a
boolean like isBunRuntime from a server-provided endpoint or a prop/initial
state and set available: isBunRuntime) so the entry for id 'quickjs-bun' is
disabled when the server/runtime is not Bun; update the ToolSidebar component to
fetch or accept the runtime indicator (or feature flag) and use that to set the
available property for the 'quickjs-bun' item and the UI disabled state.
In `@packages/ai-isolate-quickjs-bun/src/isolate-driver.ts`:
- Around line 141-142: importQuickJSBun() is being invoked on every
createContext() call; make this explicit and consistent with the existing
libraryPromise pattern by caching the dynamic import result. Add a top-level
promise (e.g., quickjsModulePromise) that stores importQuickJSBun(), use that
cached promise inside createContext() instead of calling importQuickJSBun()
directly, and ensure you still await loadQuickJSLibrary() (libraryPromise) as
before so both the module and library are only loaded once.
🪄 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: efa0ea2b-d53c-4c49-ac81-f8007e2d5a1d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.changeset/quickjs-bun-isolate-driver.mddocs/code-mode/code-mode-isolates.mddocs/code-mode/code-mode.mddocs/comparison/vercel-ai-sdk.mddocs/config.jsonexamples/ts-code-mode-web/package.jsonexamples/ts-code-mode-web/src/components/ToolSidebar.tsxexamples/ts-code-mode-web/src/lib/create-isolate-driver.tsexamples/ts-code-mode-web/vite.config.tsknip.jsonpackages/ai-code-mode/README.mdpackages/ai-code-mode/skills/ai-code-mode/SKILL.mdpackages/ai-isolate-quickjs-bun/README.mdpackages/ai-isolate-quickjs-bun/benchmarks/compare-with-wasm.tspackages/ai-isolate-quickjs-bun/package.jsonpackages/ai-isolate-quickjs-bun/src/error-normalizer.tspackages/ai-isolate-quickjs-bun/src/index.tspackages/ai-isolate-quickjs-bun/src/isolate-context.tspackages/ai-isolate-quickjs-bun/src/isolate-driver.tspackages/ai-isolate-quickjs-bun/tests/escape-attempts.test.tspackages/ai-isolate-quickjs-bun/tests/isolate-driver.test.tspackages/ai-isolate-quickjs-bun/tsconfig.jsonpackages/ai-isolate-quickjs-bun/vite.config.ts
lithdew
force-pushed
the
quickjs-bun-code-mode
branch
from
cb7abd2 to
ab73851
Compare
|
Rebased to the latest origin/main and fixed merge conflicts. |
lithdew
force-pushed
the
quickjs-bun-code-mode
branch
3 times, most recently
from
9b80eb7 to
8634ac7
Compare
|
View your CI Pipeline Execution ↗ for commit ec8c161
☁️ Nx Cloud last updated this comment at |
|
View your CI Pipeline Execution ↗ for commit 8634ac7
☁️ Nx Cloud last updated this comment at |
@tanstack/ai
@tanstack/ai-acp
@tanstack/ai-angular
@tanstack/ai-anthropic
@tanstack/ai-bedrock
@tanstack/ai-claude-code
@tanstack/ai-client
@tanstack/ai-code-mode
@tanstack/ai-code-mode-skills
@tanstack/ai-codex
@tanstack/ai-devtools-core
@tanstack/ai-elevenlabs
@tanstack/ai-event-client
@tanstack/ai-fal
@tanstack/ai-gemini
@tanstack/ai-grok
@tanstack/ai-grok-build
@tanstack/ai-groq
@tanstack/ai-isolate-cloudflare
@tanstack/ai-isolate-node
@tanstack/ai-isolate-quickjs
@tanstack/ai-isolate-quickjs-bun
@tanstack/ai-mcp
@tanstack/ai-mistral
@tanstack/ai-ollama
@tanstack/ai-openai
@tanstack/ai-opencode
@tanstack/ai-openrouter
@tanstack/ai-preact
@tanstack/ai-react
@tanstack/ai-react-ui
@tanstack/ai-sandbox
@tanstack/ai-sandbox-cloudflare
@tanstack/ai-sandbox-daytona
@tanstack/ai-sandbox-docker
@tanstack/ai-sandbox-local-process
@tanstack/ai-sandbox-sprites
@tanstack/ai-sandbox-vercel
@tanstack/ai-solid
@tanstack/ai-solid-ui
@tanstack/ai-svelte
@tanstack/ai-utils
@tanstack/ai-vue
@tanstack/ai-vue-ui
@tanstack/openai-base
@tanstack/preact-ai-devtools
@tanstack/react-ai-devtools
@tanstack/solid-ai-devtools
commit: |
Adds @tanstack/ai-isolate-quickjs-bun, a Code Mode IsolateDriver that runs QuickJS natively on the Bun runtime via bun:ffi (through quickjs-bun) instead of WebAssembly. It is a drop-in replacement for @tanstack/ai-isolate-quickjs on Bun servers. - Per-context native QuickJS runtime with its own memory/stack/timeout limits; contexts execute independently (the WASM driver serializes all executions through one asyncified module). - Same JSON tool-call protocol, console prefixes, and normalized MemoryLimit/StackOverflowError/DisposedError contract as the other drivers, plus a normalized TimeoutError for deadline expiry. - maxToolCalls (default 1000) and conosle log caps to bound stack and memory growth from untrusted snadbox code. - Requires Bun >= 1.3.14; throws an error on Node.js. Unit tests mirror the WASM/Node suites and run under `bun test`; the Node-side rejection test runs in normal CI. Docs, the ai-code-mode README + skill, and the code-mode example have been updated.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
lithdew
force-pushed
the
quickjs-bun-code-mode
branch
from
8634ac7 to
ec8c161
Compare
tombeckenham
left a comment
tombeckenham
left a comment
There was a problem hiding this comment.
It's good work generally. Thee's a few issues raised through the AI review you should address.
I've got 2 main concerns. I'm not sure about how we add the bun tests to the suite cleanly, and I couldn't properly get the example app to run in bun. Let me know how you'd address that.
| - sandbox | ||
| - secure execution | ||
| --- | ||
|
|
There was a problem hiding this comment.
[bug] The YAML frontmatter opening --- (line 1) is never closed before the body starts at line 19. Every other Code Mode doc (code-mode.md, client-integration.md, etc.) closes frontmatter with --- on its own line. The diff removed the closing delimiter. Parsers (e.g. typedoc-plugin-frontmatter) will treat lines 2–34—including the comparison table and headings—as frontmatter or mis-parse the page.
Suggestion: Restore the closing --- immediately after the keywords block (after line 17), matching the other docs in docs/code-mode/.
| QuickJS WASM uses an asyncified execution model — the WASM module can pause while awaiting host async functions (your tools). Executions are serialized through a global queue to prevent concurrent WASM calls, which the asyncify model does not support. Fatal errors (memory exhaustion, stack overflow) are detected, the VM is disposed, and a structured error is returned. Console output is captured and returned with the result. | ||
|
|
||
| > **Performance note:** QuickJS interprets JavaScript rather than JIT-compiling it, so compute-heavy scripts run slower than with the Node driver. For typical LLM-generated scripts that are mostly waiting on `external_*` tool calls, this difference is not significant. | ||
| > **Performance note:** QuickJS interprets JavaScript rather than JIT-compiling it, so compute-heavy scripts run slower than with the Node driver. For typical LLM-generated scripts that are mostly waiting on `external_`* tool calls, this difference is not significant. |
There was a problem hiding this comment.
[nit] Broken inline-code formatting: `external_`* tool calls — the backtick closes before _, leaving a stray * and incorrect rendering.
Suggestion: Use `external_*` (or "external_* tool calls" without partial backticks).
| | -------------- | -------- | -------- | ------------------------------------------------------------------------------------------------------------------------------------ | | ||
| | `memoryLimit` | `number` | `128` | Maximum heap memory for the QuickJS runtime, in megabytes. | | ||
| | `timeout` | `number` | `30000` | Maximum wall-clock time per execution, in milliseconds. | | ||
| | `maxStackSize` | `number` | `524288` | Maximum call stack size in bytes (default: 512 KiB). Increase for deeply recursive code; decrease to catch runaway recursion sooner. | |
There was a problem hiding this comment.
[suggestion] The QuickJS Bun "Options" table documents memoryLimit, timeout, and maxStackSize but omits maxToolCalls, which is a first-class driver option (default 1000) documented in the package README, changeset, and QuickJSBunIsolateDriverConfig. Users reading the canonical isolate-driver doc will not discover this safety limit.
Suggestion: Add a maxToolCalls row to the Options table and mention it briefly in "How it works", consistent with packages/ai-isolate-quickjs-bun/README.md.
| "clean": "premove ./build ./dist", | ||
| "lint:fix": "eslint ./src --fix", | ||
| "test:build": "publint --strict", | ||
| "test:bun": "bun test ./tests", |
There was a problem hiding this comment.
[suggestion] The substantive test suite (tests/*.test.ts, ~880 lines) is gated with describe.skipIf(typeof Bun === 'undefined') and only runnable via test:bun. Standard CI targets (test:lib / test:pr) use Vitest on Node and therefore exercise only the single "rejects createContext on Node.js" test. Escape-attempt, timeout, memory-limit, maxToolCalls, and concurrency behavior are unverified in CI.
I'm unsure whether we should add in bun to the main test suite. I'm thinking that you'll need a second CI workflow to run the bun tests separately. We'll need core approval to add that in.
| id: 'quickjs-bun', | ||
| name: 'QuickJS Bun', | ||
| description: 'Native QuickJS engine (requires running the server with Bun)', | ||
| available: true, |
There was a problem hiding this comment.
[suggestion] The new quickjs-bun VM option is marked available: true unconditionally. When the example server runs under Node (the default pnpm workflow), selecting it fails at createContext with a runtime error. The description warns about Bun, but the UI still presents it as available—unlike the Node driver path in create-isolate-driver.ts, which falls back to QuickJS on failure.
Suggestion: Gate available on server runtime (e.g. env flag or server-side probe), or add a quickjs-bun fallback/error message in create-isolate-driver.ts similar to the Node driver pattern.
| * | ||
| * This driver runs QuickJS natively through `bun:ffi` (via `quickjs-bun`) | ||
| * instead of WebAssembly. Each context gets its own QuickJS runtime with | ||
| * dedicated memory and stack limits, so sandboxes are fully isolated from |
There was a problem hiding this comment.
[suggestion] "fully isolated ... from the host" overstates the boundary. Per-context heaps and scope isolation hold and are well-tested, but this driver runs QuickJS compiled by TinyCC and called through bun:ffi — directly in the host process address space, with no sandbox boundary. The memory "limit" is QuickJS's internal accounting (JS_SetMemoryLimit), not a hardware/OS/VM wall. Unlike Node isolated-vm (separate V8 isolate) or the WASM driver (WebAssembly linear-memory sandbox), a memory-safety bug in QuickJS or a mishandled FFI handle here is a host-process compromise, not a contained fault.
Suggestion: keep the accurate per-context-independence framing, but reword to state the trade-off honestly (native FFI trades WASM's/V8's containment boundary for speed — fits trusted-ish or otherwise-contained deployments, weaker fit where the sandbox wall itself is the security requirement). Same applies to the README's "with the same sandboxing guarantees" line — the guarantees are not the same, only the engine and scope surface are.
| // the handle dumps to something else. | ||
| let argsJson = '{}' | ||
| try { | ||
| const dumped = vm.dump(argsHandle ?? vm.undefined) |
There was a problem hiding this comment.
[suggestion] When vm.dump throws here (sandbox heap exhausted while materializing the args string), the catch swallows the JSException and the code falls through with argsJson = '{}' — and then the async IIFE below still calls binding.execute({}). So a host tool runs with empty args instead of the caller's real args (a real side effect with the wrong inputs for something like readFile/deleteRecords), and the underlying OOM is never surfaced or classified.
The "dump returned a non-string" case degrading to {} is fine (the wrapper guarantees a JSON string). But the throw case should not invoke the binding — record it via hostSettleError ??= this.toNormalizedError(error) (or settle the sandbox promise with the classified error) so the execution loop surfaces it and releases the VM if it was fatal.
| return this.disposedResult() | ||
| } | ||
|
|
||
| this.logs.length = 0 |
There was a problem hiding this comment.
[suggestion] Reused-context state bleed: execute() resets host-side counters here but nothing drains the QuickJS job queue between runs. After a timeout/abort, the previous program's un-run promise reactions stay queued; on the next execute() the loop's executePendingJob() runs those stale jobs first — attributing their wall-clock, tool-call budget, and console.* output to the new execution.
Latent today (the main consumer creates a fresh context per call and disposes it), but this class explicitly supports multi-execute — that's the whole point of the execQueue serialization + per-run reset. Either drain/reset the VM job queue on entry, or document that a context is single-execute.
| * dropped for the rest of the execution. | ||
| */ | ||
| const MAX_LOG_ENTRIES = 10_000 | ||
| const MAX_LOG_BYTES = 1_000_000 |
There was a problem hiding this comment.
[nit] MAX_LOG_BYTES is compared against msg.length in pushLog, but String.length is UTF-16 code units, not bytes — while the constant name and the doc comment say "bytes". Worst-case multi-byte content lets the host log buffer reach ~3–4 MB before truncation. Still bounded, so low risk. Rename to MAX_LOG_CHARS, or measure real byte length, to match the stated intent.
| driver = createQuickJSIsolateDriver() | ||
| break | ||
| } | ||
| case 'quickjs-bun': { |
There was a problem hiding this comment.
[suggestion] The quickjs-bun driver case added here is effectively unreachable end-to-end, which makes the new sidebar option impossible to actually exercise:
- The server never passes the selected VM through. All nine server call sites hardcode
createIsolateDriver('node')(api.execute-prompt.ts:31,api.product-codemode.ts:63,api.database-demo.ts,api.banking-demo.ts,api.reports.ts,api.report-event.ts,api.structured-output.ts,reports/*.ts). Selecting "QuickJS Bun" (or anything) inToolSidebarchanges nothing — the server always builds the Nodeisolated-vmdriver. - There's no way to run the server under Bun.
devisvite dev(Node), there's nobunscript, and the README has no Bun instructions (its whole troubleshooting section is aboutisolated-vmon Node). But this driver requires the server process itself to be Bun.
So as shipped, the option is cosmetic. To make it usable: thread the selected IsolateVM from the request into getDriver() (keyed cache per VM), add a "dev:bun" script that runs the server under Bun, and document the Bun run path in the README. Otherwise the sidebar entry should be removed until the example can actually run it.
2 participants
🎯 Changes
Adds a new Code Mode isolate driver,
@tanstack/ai-isolate-quickjs-bun, that runs QuickJS natively on Bun viabun:ffi(throughquickjs-bun) rather than WebAssembly.It implements the existing
IsolateDrivercontract from@tanstack/ai-code-mode, so it's a drop-in replacement for@tanstack/ai-isolate-quickjson Bun servers:Why
On Bun, the existing WASM driver (
quickjs-emscripten) is both slower and, in my testing, unreliable for async host tool calls. Its asyncify bridge crashes the shared WASM module (memory access out of bounds) and hangs once a single execution makes ≥ 4 sequential awaited tool calls (reproduced on Node 22 and Bun 1.3.14).quickjs-bunmaps the QuickJS C API directly throughbun:ffi, giving each context its own native runtime with no asyncify and no shared-VM serialization.Benchmarks
packages/ai-isolate-quickjs-bun/benchmarks/compare-with-wasm.ts, both drivers driven through the publicIsolateDriverinterface (Apple M-series, darwin/arm64; Bun 1.3.14 for the native driver, Node 22 for the WASM driver as its native habitat):return 1 + 1fib(20))return 1 + 1(reused context)¹ The WASM driver's asyncified host tool calls repeatedly crash the shared WASM module and hang subsequent executions (≥ 4 sequential awaited host calls per process), on both Node 22 and Bun 1.3.14. Sync-only workloads are unaffected. WASM wins cold start (one-time WASM instantiate vs TinyCC compile of the QuickJS sources); the native driver wins steady-state per-execution by ~14× on the trivial case.
Notes
The suites are gated with
describe.skipIf(typeof Bun === 'undefined'), mirroring how@tanstack/ai-isolate-nodeskips when its native addon is unavailable — would like to know if it is fine to add aoven-sh/setup-bunjob (pnpm --filter @tanstack/ai-isolate-quickjs-bun test:bun) so that the full test suite is ran in CI.✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
@tanstack/ai-isolate-quickjs-bun, drop-in for Bun) usingbun:ffi, with configurabletimeout,memoryLimit,maxStackSize, andmaxToolCalls(default 1000) plus normalized limit/timeout error behavior.quickjs-bundriver option.