Repository navigation
feat(core): accept a host-supplied sandboxAdapter - #96
frontegg-david merged 1 commit into
Conversation
|
Warning Review limit reachedYour organization has reached its usage spending cap. Adjust your spending cap in the billing tab. Next included review available in 17 minutes. View limit detailsLimit details: You’ve used all 3 included reviews currently available. Your 43 included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (7)
Comment |
fc52096 to
e47b0dc
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation, documentation, validation, ownership semantics, and regression coverage are consistent with the stated requirements.
Review effort: Balanced
Findings: None
What changed in this PR
Adds host-supplied sandbox adapters to core while improving behavior when node:vm is unavailable.
Changes:
- Adds validated
sandboxAdaptersupport with caller-owned lifecycle. - Introduces a descriptive
node:vmavailability error. - Documents adapter precedence and adds regression tests.
| File | Description |
|---|---|
libs/core/src/types.ts |
Defines and documents sandboxAdapter. |
libs/core/src/node-vm.ts |
Checks node:vm availability. |
libs/core/src/enclave.ts |
Validates and selects host adapters. |
libs/core/src/__tests__/enclave.sandbox-adapter.spec.ts |
Tests custom adapter behavior. |
libs/core/src/__tests__/enclave.missing-node-vm.spec.ts |
Tests missing-VM handling. |
docs/enclave/core-libraries/enclave-vm/configuration.mdx |
Documents configuration and examples. |
docs/enclave/api-reference/enclave-vm.mdx |
Updates the API options table. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Enclave could only run scripts with node:vm. Without it (core bundled for a browser with vm mapped to an empty module) run() failed with "vm.createContext is not a function", and there was no supported way to run scripts another way: the exported InterpreterAdapter could not be selected, and the adapter option is ignored while the double VM is on (the default). New sandboxAdapter option: an object with execute(code, context) and dispose() that runs the validated, transformed script in place of the double VM and the adapter option. The enclave does not dispose it, so one adapter can serve several enclaves. The constructor throws a TypeError for a value without those methods. Without node:vm, run() now fails with "@enclave-vm/core needs node:vm, which this runtime does not provide. In a browser, use @enclave-vm/browser; elsewhere, pass a sandboxAdapter". The docs now say that adapter takes effect only with the double VM off (the worker_threads example did not disable it) and document sandboxAdapter. Fixes #91 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e47b0dc to
09a9518
Compare
Stack 5/10:
main← #92 ← #93 ← #94 ← #95 ← this ← #98 ← #99 ← #100 ← #101 ← #102Fixes #91.
Validation
All three points reproduce:
vmmapped to an empty module,run()fails withTypeError: vm.createContext is not a function(DOUBLE_VM_EXECUTION_ERROR).getAdapter()returns the double VM before it readsconfig.adapter. So{ adapter: 'isolated-vm' }runs fine, and the documented{ adapter: 'worker_threads', workerPoolConfig }example actually runs in the double VM.InterpreterAdapteris exported, butnew Enclave({ adapter: 'interpreter', doubleVm: { enabled: false } })fails withUnknown adapter: interpreter.Fix
New
sandboxAdapteroption. It takes any object withexecute(code, context)anddispose(), and runs the script in place of the double VM and theadapteroption.execute.ExecutionContext(limits, tool handler, stats, abort signal).Enclave.dispose()doesn't dispose it, so one adapter can serve several enclaves.TypeErrorat construction.Missing
node:vm. Before the double VM or thevmadapter is built,run()now fails with:@enclave-vm/core needs node:vm, which this runtime does not provide. In a browser, use @enclave-vm/browser; elsewhere, pass a sandboxAdapter (for example new InterpreterAdapter()).Docs.
adapter(JSDoc, configuration page, API reference) now says it takes effect only withdoubleVm: { enabled: false }, and theworker_threadsexample sets that. There's a new "Custom Sandbox Adapter" section.With this, FrontMCP's playground can pass its forwarding adapter as
sandboxAdapterinstead of patchinggetAdapter()in the published bundle.Not changed (follow-ups)
adapteris still ignored while the double VM is on; now it's documented. Honoring it, or throwing, would change behavior for hosts that setadapter: 'worker_threads'today and actually run on the double VM.InterpreterAdapterstill only providesMath,JSONand__safe_callTool, so scripts usingDate,parallelor custom globals fail on it.docs/enclave/api-reference/enclave-vm.mdxdocumentscreateWorkerPoolAdapter()/createDoubleVmAdapter()factories that don't exist. Only the option rows are fixed here.Red → green
New specs
enclave.sandbox-adapter.spec.ts(8 tests) andenclave.missing-node-vm.spec.ts(3 tests, withjest.mock('vm', () => ({}))). Before: 10 failed. The one that passed is a guard: validation runs before the adapter. After: 11 passed.Verification
jest -c libs/core/jest.config.ts --testPathIgnorePatterns /perf/: 66 suites, 2354 passed, 1 skipped; perf suite: 48 passedyarn lint,yarn format:check,yarn build: pass🤖 Generated with Claude Code