Repository navigation
Conversation
getNodeVMContextOptions() rejected an explicitly undefined strings or wasm key in codeGeneration/contextCodeGeneration with ERR_INVALID_ARG_TYPE. Node treats an undefined key the same as an omitted one (the default, true), and its own getContextOptions() builds the codeGeneration object with undefined holes, so skip the boolean check for undefined the way the sibling name/origin/microtaskMode reads already do. Covers createContext, vm.runInNewContext and Script#runInNewContext, which share this function.
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: fix pushed, self-review done (no findings), waiting on CI. Reproduced on bun 1.4.0 and main (b555e06) with: const vm = require("node:vm");
vm.createContext({}, { codeGeneration: { wasm: undefined } });
// TypeError [ERR_INVALID_ARG_TYPE]: The "options.codeGeneration.wasm" property must be of type boolean. Received undefinedSame for Test: Related but separate: #38326 (runInNewContext context reuse) changes different lines and still needs this change for undefined keys. The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — two-line Node compat fix bringing codeGeneration.strings/.wasm in line with the sibling option reads.
What was reviewed:
- The two
&& !value.isUndefined()guards match the pattern already used forname/origin/microtaskMode/codeGenerationa few lines above;allowStrings/allowWasmdefault totrueinNodeVMContextOptionsso skipping the assignment is correct. - Confirmed
undefined→ default (true) is not a security loosening — it's identical to the key being absent, which was already accepted. - Tests cover all three entry points sharing this helper, verify
undefinednext to explicitfalsekeeps thefalse, and confirmnull/0/1/"false"are still rejected withERR_INVALID_ARG_TYPE.
Extended reasoning...
Overview
Two-character-class change in src/jsc/bindings/NodeVM.cpp (getNodeVMContextOptions): the if (allowStringsValue) and if (allowWasmValue) guards each gain && !value.isUndefined(), so a present-but-undefined codeGeneration.strings/.wasm falls through to the struct default (true) instead of failing the isBoolean() check. A new nested describe block in test/js/node/vm/vm.test.ts exercises the behaviour across createContext, vm.runInNewContext, and Script#runInNewContext.
Security risks
None. The only new accepted input is undefined, which now behaves exactly like an absent key (already accepted). The default is true (code generation allowed), so there is no accidental tightening or loosening relative to the pre-existing absent-key path. null, numbers, and strings still throw ERR_INVALID_ARG_TYPE, and the tests assert that.
Level of scrutiny
Low. This is a mechanical Node.js compat fix — the sibling reads for name, origin, microtaskMode, and the outer codeGeneration object in the same function already skip undefined with the exact same idiom; this just brings the two nested keys in line. NodeVMContextOptions::allowStrings/allowWasm are initialized to true (verified in NodeVM.h:80-81), so the fall-through is correct. No memory safety, GC, or exception-scope concerns: isUndefined() is a non-throwing tag check on a value already validated by RETURN_IF_EXCEPTION.
Other factors
- CODEOWNERS does not cover either file.
- Tests are well-structured: added to the existing
codeGeneration optionsdescribe in the right file, usedescribe.each/test.eachper harness conventions, cover the full entry-point matrix (all three callers ofgetNodeVMContextOptions), assert both the positive contract (undefined → default, mixed with explicitfalse) and the negative contract (non-boolean non-undefined still rejected with the specific error code). - PR description confirms
USE_SYSTEM_BUN=1failure and Node v26.3.0 parity, and that the fullvm.test.tsand 95test-vm-*.jsparallel tests still pass.
|
Updated 8:05 PM PT - Aug 13th, 2026
❌ @robobun, your commit 89da7ed has some failures in 🧪 To try this PR locally: bunx bun-pr 38314That installs a local version of the PR into your bun-38314 --bun |
|
Updated 12:53 AM PT - Aug 14th, 2026
✅ @robobun, your commit 89da7ede33dc64c26b5509e3ff08005f0cc93527 passed in 🧪 To try this PR locally: bunx bun-pr 38314That installs a local version of the PR into your bun-38314 --bun |
This branch has not been deployed
1 participant
Problem
vm.createContext({}, { codeGeneration: { wasm: undefined } })throwsTypeError [ERR_INVALID_ARG_TYPE]: The "options.codeGeneration.wasm" property must be of type boolean. Received undefined. Same forstrings, and forcontextCodeGeneration.{strings,wasm}throughvm.runInNewContext()andScript#runInNewContext(). Node accepts all of these.getNodeVMContextOptions()insrc/jsc/bindings/NodeVM.cpp(lines 724 and 736) reads the two keys withgetIfPropertyExists(), which returnsjsUndefined()(a non-empty value) for a key that is present but set toundefined, and then only checksisBoolean(). Thename,origin,microtaskModeandcodeGenerationreads a few lines above already skipundefined; these two did not.lib/vm.jsdoes:getContextOptions()passescodeGeneration: { strings, wasm }along with whichever of the two the caller left out stillundefined, and user code that spreads optional settings produces the same shape.Fix
undefined, so the key falls through to the default (allowStrings/allowWasmare initialized totrueinNodeVMContextOptions).null, numbers and strings are still rejected withERR_INVALID_ARG_TYPE.createContext()destructures{ strings = true, wasm = true } = codeGenerationbeforevalidateBoolean, andgetContextOptions()only validates the two keys when they are!== undefined. Node rejectsnullfor both, which this keeps.getNodeVMContextOptions()(createContextreadscodeGeneration,vm.runInNewContextandScript#runInNewContextreadcontextCodeGeneration), so one change covers them.Script#runInNewContextstill validatescontextCodeGenerationthroughgetNodeVMContextOptions(), so the undefined keys need this change with or without it. The two PRs change different lines ofNodeVM.cpp.test/js/node/vm/vm.test.ts,codeGeneration options > strings/wasm set to undefined. For each of the three entry points it checks that an undefined key is accepted and behaves as the default (eval andWebAssembly.Moduleboth work), that an undefined key next to an explicitfalseleaves thefalsein effect, and thatnull/0/1/"false"are still rejected. The six acceptance tests fail on the current release (USE_SYSTEM_BUN=1) and pass with this change; the expected values were checked against node v26.3.0.test/js/node/vm/vm.test.ts(232 pass, 0 fail) and all 95test/js/node/test/parallel/test-vm-*.jsfiles (includingtest-vm-codegen.jsandtest-vm-basic.js) against the debug build, all passing.Background
codeGeneration.strings/.wasmare per-context switches for whether code running in avmcontext may compile code from strings (eval,new Function) and compile WebAssembly. Both default totrue.JSObject::getIfPropertyExists()returns an emptyJSValuewhen the property is absent and the property's value (which may bejsUndefined()) when it exists, soif (value)alone distinguishes missing from present, not missing fromundefined.[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
26 concerns were raised and did not survive verification.