Conversation
vm.compileFunction created the function with the global object itself as its scope unless contextExtensions were given, so script-level let, const and class bindings (which live in the global lexical environment, in front of the global object) were unresolvable from the compiled body, and sloppy assignments to them created global properties instead. Start the scope chain at parsingContext->globalScope(), as the contextExtensions path, the Function constructor and program evaluation already do.
|
Warning Review limit reached
Next review available in: 7 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 |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit ed93688 has some failures in 🧪 To try this PR locally: bunx bun-pr 38305That installs a local version of the PR into your bun-38305 --bun |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small and the reasoning checks out, but since it changes JSC scope-chain construction for vm.compileFunction (and shifts what setGlobalScopeExtension receives in the no-extensions case), a human look from someone comfortable with JSC scope semantics would be worthwhile.
What was reviewed:
- Confirmed
options.parsingContextis always non-null here (CompileFunctionOptions::fromJSinitializes it, and the fallback branch re-initializes it), so the removed ternary was dead and the new dereference is safe. - The
contextExtensionsbranch already based its with-scope chain onparsingContext->globalScope(), so the two paths now share one base — no behavior change on that branch. setGlobalScopeExtension(functionScope)now receives the global lexical environment instead of the global object in the no-extensions case; both look inert (already walked before the extension is consulted), and #38302 removes the call.- New tests use
randomPropsto avoid polluting/colliding in the caller's global lexical scope across thedescribe.eachmatrix, and pin thecontextExtensionsshadowing order.
Extended reasoning...
Overview
Two files touched: a ~5-line change in src/jsc/bindings/NodeVM.cpp inside vmModuleCompileFunction, and 8 new test cases in test/js/node/vm/vm.test.ts (a describe.each over this-context and parsingContext, each covering read, late declaration, sloppy assignment, and contextExtensions shadowing). The C++ change replaces the function's initial scope (options.parsingContext ? options.parsingContext : globalObject, i.e. the global object) with options.parsingContext->globalScope() (the global lexical environment), and reuses that as the base for the contextExtensions with-scope chain instead of recomputing it.
Security risks
None identified. This adjusts which JSC scope object sits at the base of the compiled function's scope chain; it does not widen what a compiled function can reach beyond what vm.runIn*Context already exposes for the same context. The global lexical environment's parent is the global object, so resolution still terminates in the same place.
Level of scrutiny
Medium-high. The diff is tiny and the intent is a straightforward Node compat fix, but it lives in JSC bindings and changes scope-chain construction, where mistakes surface as subtle resolution or TDZ differences rather than crashes. It also changes the argument to the persistent setGlobalScopeExtension side effect in the no-extensions path — the PR description argues (convincingly) that this is inert and #38302 removes the call, but that is exactly the kind of nuance a JSC-familiar reviewer should sanity-check.
Other factors
- Null-safety of the new unconditional dereference is established:
CompileFunctionOptions::fromJSsetsparsingContext = globalObjectbefore doing anything else, and the caller'soptions = {}; options.parsingContext = globalObject;fallback covers the false-return path. ThecontextExtensionsbranch already dereferenced it unconditionally, so no new failure mode is introduced. - The fix mirrors how JSC's own
FunctionConstructorbuilds function scopes (per the PR description), and thecontextExtensionspath was already doing the right thing — this just brings the no-extensions path in line with it. - Tests are well-structured: they use
randomProps()sorunInThisContextletdeclarations don't collide across cases (they can't be cleaned up), assert exact values, cover all four lexical declaration forms, and verify assignments hit the binding rather than creating a global property. The twocontextExtensionscases pin existing behavior against regression. - No CODEOWNERS entry covers these files. No prior human review comments to address.
|
No code changes from this review. For whoever looks at the JSC side, the two points it singles out, with the places to check:
|
With the function now created on the global lexical environment, leaving setGlobalScopeExtension in place would have made functions that are created directly on the global object (builtins and internal modules) resolve their unresolved identifiers against script-level let/const bindings after any compileFunction call. The chain is already what the function is created with, so only hand it to the function.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated after a closer look at the Changes since the first revision:
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes JSC scope-chain semantics for vm.compileFunction (removing the realm-wide setGlobalScopeExtension and rebasing the function's scope on the global lexical environment) and overlaps with #38297 and #38302 in the same function, a human look would still be worthwhile.
What was reviewed:
- Verified
options.parsingContextis always non-null before->globalScope()—CompileFunctionOptions::fromJSsets it unconditionally, and thefromJS-returns-false branch resets it toglobalObject. - Checked that dropping
setGlobalScopeExtensiondoes not lose the with-scope taint forcontextExtensions:constructAnonymousFunctionreads the taint fromglobalObject, notparsingContext, so the deleted call never set the bit it reads anyway; theJSWithScopechain on the function itself carries the dynamic-lookup behaviour. - Tests use
randomProps()for the "this context" variant so the persistent global lexical bindings created viarunInThisContextdon't collide across test runs; thedescribe.eachmatrix covers both entry points and pins the extension-leak regression.
Extended reasoning...
Overview
Two files changed. In src/jsc/bindings/NodeVM.cpp, vmModuleCompileFunction now seeds the compiled function's scope chain with options.parsingContext->globalScope() (the JSGlobalLexicalEnvironment) instead of the global object itself, reuses that same base for the contextExtensions with-scope stack, and drops the trailing setGlobalScopeExtension(functionScope) call. In test/js/node/vm/vm.test.ts, a describe.each block adds 8 tests (4 cases × {this context, parsingContext}) covering visibility of var/let/const/class, late declaration after compile+call, assignment writing back to the lexical binding rather than creating a global property, and contextExtensions shadowing without leaking to the realm.
Security risks
None introduced. The change narrows realm-wide state mutation (removing a persistent setGlobalScopeExtension) rather than adding any. The compiled function's own scope chain now matches what new Function(...) and script evaluation already get in JSC, so no new resolution surface is exposed to user code that wasn't already reachable.
Level of scrutiny
High. Although the native diff is ~5 lines, it changes identifier resolution for every vm.compileFunction result and removes a realm-wide side effect. The PR timeline shows the author's own first revision was subtly wrong (leaving setGlobalScopeExtension in place would have let builtins created directly on the global object resolve into user let/const bindings after any compileFunction() call). That kind of second-order effect is exactly what a JSC-familiar maintainer should sanity-check. The change also intentionally overlaps with #38302 (same one-line deletion) and sits adjacent to #38297 (which realm constructAnonymousFunction compiles in), so a human should confirm the intended merge order and that the combined result is what everyone expects.
Other factors
parsingContextnon-null: confirmed both code paths set it before dereference, so the removed?:was dead as claimed.- The
LexicallyScopedFeaturesread inconstructAnonymousFunctionkeys offglobalObject->globalScopeExtension(), notparsingContext's, so the deleted call was never what set that bit for the code path that reads it; theJSWithScopeobjects in the function's own chain make the lookups dynamic regardless. - Tests are hermetic via
randomProps()for therunInThisContextvariant (which necessarily mutates the real global lexical environment). The last test's trailing assertions (typeof extensionOnlyin a freshcompileFunctionand viarun) pin the exact regression thesetGlobalScopeExtensiondeletion prevents. - The comment-cop bot flagged earlier revisions for a paragraph-long code comment; the final revision removed it, so that feedback is addressed.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The scope-chain change and the setGlobalScopeExtension removal both check out against JSC's own FunctionConstructor/globalScope() usage, and the 8 new tests cover the variant matrix well. Given it changes node:vm scope-resolution semantics in the JSC bindings and overlaps with #38297/#38302 in the same function, a human look would still be worthwhile.
What was reviewed:
- Verified
options.parsingContextis always non-null at the deref (CompileFunctionOptions::fromJSsets it unconditionally, as does the fallback branch). - Confirmed the removed
setGlobalScopeExtensionwas the only writer in Bun's codebase; remaining readers only derive a taint bit forSourceCodeKey, so no dangling consumer. - Checked that
contextExtensionsstill work without the realm-wide hook — the with-scopes are on the function's own chain, and the new tests pin both shadowing and non-leakage.
Extended reasoning...
Overview
The PR changes three effective lines in vmModuleCompileFunction (src/jsc/bindings/NodeVM.cpp): the compiled function's base scope becomes parsingContext->globalScope() (the global lexical environment) instead of the global object itself, the contextExtensions with-scope chain is stacked on that same base (deduplicating an identical expression), and the trailing setGlobalScopeExtension(functionScope) call is removed. Eight new tests in test/js/node/vm/vm.test.ts exercise both the this-context and parsingContext variants across visibility, late declaration, assignment, and contextExtensions shadowing.
Security risks
node:vm is sandbox-adjacent, so scope-chain changes matter for isolation. This change does not weaken isolation: it makes the compiled function see its own context's script-level lexical bindings (which it should, per Node), and it removes a realm-wide side effect that was leaking one call's with-scope chain to every subsequent lookup in the realm. The new tests explicitly assert the negative contract — that contextExtensions are invisible to the context and to later compiles — so the security posture improves.
Level of scrutiny
This deserves careful human review despite its small size. It changes scope-resolution semantics in the JSC bindings, where the interaction between JSGlobalLexicalEnvironment, JSWithScope, the realm-wide globalScopeExtension hook, and the TaintedByWithScopeLexicallyScopedFeature bit is subtle. The PR description is thorough and the reasoning cross-references the right JSC sources (FunctionConstructor.cpp, JSScope::resolve), but a maintainer familiar with the JSC integration should confirm the claim that with-scope lookups link as Dynamic regardless of the realm-wide hook, and sanity-check the interaction with the two concurrent PRs (#38297, #38302) touching the same function.
Other factors
options.parsingContextcannot be null at the new deref:CompileFunctionOptions::fromJSsetsthis->parsingContext = globalObjectbefore doing anything else, and the caller's fallback branch does the same.- The removed
setGlobalScopeExtensionwas the only writer in Bun's codebase; the five readers inNodeVM.cpp/NodeVMScript.cpp/NodeVMSourceTextModule.cpponly derive aSourceCodeKeytaint bit and now consistently see null, which is a fix (the bit was previously leaking to unrelated compilations after anycompileFunctioncall). - Tests use
randomProps()to avoid polluting the shared this-context lexical environment across test runs, follow the existing file's conventions, and assert exact values. All bot review comments (comment-cop about a long code comment) are resolved — the comment was removed in 0958eca. - CI for the latest commit is still building.
This branch has not been deployed
1 participant
Problem
vm.compileFunction("return x")()throwsReferenceError: x is not definedwhenxwas declared withlet,constorclassby a script in that context (vm.runInThisContext, orvm.runInContextwhen compiling withparsingContext).vardeclarations and global properties resolve fine. Node returns the binding's value.contextExtensionsentry is passed, even[{}].vmModuleCompileFunction(src/jsc/bindings/NodeVM.cpp:1597 on main) creates the function with the global object itself as its scope, so its scope chain is[global object]. Script-level lexical bindings live in the global lexical environment, which ordinary scope chains visit before the global object, so the compiled body never consults it. ThecontextExtensionsbranch builds its with-scopes onparsingContext->globalScope()(the lexical environment), which is why that path works.compileFunctionwas added in Implementvm.compileFunctionand fix some node:vm tests #18285.Fix
options.parsingContext->globalScope(), giving the chain[global lexical environment -> global object], and stack thecontextExtensionswith-scopes on that same base.parsingContextis always set by this point (CompileFunctionOptions::fromJSinitializes it to the caller's global, and so does the fallback invmModuleCompileFunction), so the oldparsingContext ? parsingContext : globalObjectnever chose anything.globalObject->globalScope()(FunctionConstructor.cpp),Interpreter::executeProgramruns scripts in it, module environments chain to it, andJSScope::abstractAccesshas a dedicatedGlobalLexicalVarresolution for it. It also matches Node, where a compiled function resolves script-level bindings like any other function of the context.options.parsingContext->setGlobalScopeExtension(functionScope)call that followed. It installed the chain realm-wide, whereJSScope::resolveconsults it for every lookup that misses on the global object. On main that is inert without extensions (it installed the global object, which the lookup had just checked) and is the extension leak node:vm: stop leaking compileFunction contextExtensions into later global lookups #38302 fixes with extensions. Combined with the first change alone, it would have installed the lexical environment, so after any plaincompileFunction()call the unresolved identifiers of functions created directly on the global object (JSC builtins, Bun's internal modules) would have started resolving to the user's script-levellet/constbindings; a reference to an undeclaredbindingsin node:wasi'sgetState()made this observable (see the details below). The chain is already what the function is created with, so the call contributed nothing to the function itself. This is the same one-line deletion as node:vm: stop leaking compileFunction contextExtensions into later global lookups #38302, whose tests pass on this branch; whichever lands second rebases onto an identical deletion.contextExtensionskeep shadowing lexical bindings (the with-scopes still sit in front of the base), and lookups through a with-scope are linked asDynamicregardless of the realm-wide hook, which is why the function does not need it. Both are pinned by the tests.global lexical bindings of ...cases run the same four tests against the caller's context and against aparsingContext(visibility ofvar/let/const/class, a binding declared after the function was compiled and first called, assignment updating the binding without creating a global property, and extensions shadowing bindings while staying invisible to the context and to later compiles). All 8 fail on bun 1.4.0 and pass here. The rest of vm.test.ts, the other test/js/node/vm/ files, node:vm: stop leaking compileFunction contextExtensions into later global lookups #38302's tests, and the vendored test-vm-basic.js, test-vm-module-basic.js, test-vm-no-dynamic-import-callback.js, test-vm-function-declaration.js, test-vm-context.js, test-vm-strict-assign.js, test-vm-not-strict.js, test-vm-cached-data.js and test-vm-createcacheddata.js pass with the debug build.constructAnonymousFunctionstill takes the realm and the scope as separate parameters (node:vm: compile compileFunction sources in the parsingContext realm #38297 changes which realm is passed). Once node:vm: compile compileFunction sources in the parsingContext realm #38297, node:vm: stop leaking compileFunction contextExtensions into later global lookups #38302 and this land, the realm, the base scope and the with-scope chain can all be derived fromparsingContextin one place; that is a follow-up rather than something to fold into three concurrent one-line PRs.Background
JSGlobalLexicalEnvironment, returned byJSGlobalObject::globalScope()): the JSC object holding a realm's script-levellet/const/classbindings. Unlikevarand function declarations, these are not properties of the global object. Its parent scope is the global object, so an ordinary scope chain ends with... -> global lexical environment -> global object.JSFunction: theJSScope*it is created with. Free identifiers in the body are resolved by walking outwards from it, so a function created directly on the global object only sees global properties. JSC and Bun create their builtins and internal modules that way, since those reference no user bindings.contextExtensions: objects compileFunction places in front of the chain aswith-style scopes (JSWithScope), so their properties shadow whatever is behind them. V8 builds the same chain for Node.JSGlobalObject::setGlobalScopeExtension): a realm-wide hook thatJSScope::resolvechecks after a lookup has reached the global object and missed. JSC's own users (evaluateWithScopeExtension, the debugger) install it around a single evaluation and clear it afterwards.Repro
With this change every line matches node v26.3.0.
Why the setGlobalScopeExtension call had to go in the same change
The first revision of this PR only changed the base scope and argued the remaining
setGlobalScopeExtensioncall was unobservable because every lookup walks the lexical environment before reaching the global object. That is true for user code, but not for functions created directly on the global object. node:wasi'sWASI#getState()/setState()reference an undeclaredbindings(a separate bug), which makes the difference visible:With only the base-scope change, the second
setStatesucceeded and overwrote the user'slet bindings. Removing the call keeps the realm in the state it is in before anycompileFunctioncall, which is also what thewhich only the compiled function seesassertions in the new tests check (they fail on 1.4.0 because of the extension leak, and would fail on the first revision for the same reason).[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file
root cause · written by the author bot
vm.compileFunction without contextExtensions built the compiled function's scope chain directly on the global object, bypassing the JSGlobalLexicalEnvironment where top level let, const and class bindings declared by scripts live, so those identifiers resolved as undefined even though var bindings were found and the contextExtensions path, which already stacked its with scopes on the lexical environment, worked. The fix bases the function's scope on parsingContext->globalScope() in both paths, matching what JSC's own Function constructor does, and removes the realm-wide setGlobalScopeExtens…