Conversation
|
Warning Review limit reached
Next review available in: 2 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 (5)
Comment |
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — moves the evalmachine.<anonymous> default to the ScriptOptions constructor so it applies to every call shape, and deletes the filenameProvided / CompileFunctionOptions reset workarounds that only existed because the default lived in the shared parser.
Checked: compileFunction still gets a null default filename (both the no-options reset path and the {}-options path), RunningScriptOptions.filename is unused so its unchanged empty default is harmless, and no other call sites construct ScriptOptions/BaseVMOptions with the removed three-arg form. Also noted the removed unconditional optionsArg.toWTFString(...) in the two binding-level host functions was itself a latent missing-exception-check on an object arg's toString, so this is a net cleanup there too. Tests cover the full option-shape matrix plus the negative (filename: "" stays non-default), CallSite.getFileName(), and compile-time SyntaxError stacks; Error.prepareStackTrace is restored in finally.
Extended reasoning...
Overview
This PR fixes node:vm so that a Script (and every entry point built on it — createScript, runInContext/runInNewContext/runInThisContext) constructed without a filename names its source evalmachine.<anonymous> like Node, instead of the empty string that rendered as file:/// in stack frames. It touches four files in src/jsc/bindings/ (NodeVM.{h,cpp}, NodeVMScript.{h,cpp}) and adds tests to test/js/node/vm/vm.test.ts.
The mechanism is a small refactor: the default used to live inside BaseVMOptions::fromJS (only reached when an options object is passed and lacks filename), forcing constructScript to seed "" and track filenameProvided, and forcing CompileFunctionOptions::fromJS to undo the default afterwards. The default now lives in ScriptOptions(), fromJS only overwrites when options.filename is a string, and both workarounds plus the now-unused three-arg BaseVMOptions constructor are deleted.
Security risks
None. This changes only the default source-URL string attached to a compiled vm script for stack-trace display. No auth, crypto, permissions, memory, or lifetime code is touched.
Level of scrutiny
Moderate — it's C++ in the JSC bindings, so I traced every consumer of the changed types. BaseVMOptions/ScriptOptions are referenced only in the four edited files; ScriptOptions is now only default-constructed (three sites, all updated) and moved (implicit move ctor is still generated since only a user-declared default constructor was added). CompileFunctionOptions keeps using BaseVMOptions::BaseVMOptions and is only default-constructed, so its filename stays null — identical to what the removed reset produced. RunningScriptOptions also inherits base constructors and is only default-constructed; its filename is written but never read (the script's SourceCode is already built), so its unchanged empty default is inert. The two binding-level host functions that dropped ScriptOptions options(optionsArg.toWTFString(globalObject), …) also lose an unchecked coercion that could run a user toString; the new code only coerces after the isString() guard with a proper RETURN_IF_EXCEPTION.
Other factors
The tests are thorough per the repo's review rules: they run inside the shared testRunInContext matrix (covering all six runIn* entry points), assert the negative case (filename: "" is not replaced), check both option forms (string and object), check CallSite.getFileName(), and pin that the three no-filename shapes produce byte-identical compile-error stacks. Global mutation (Error.prepareStackTrace) is restored in finally. The PR description confirms the tests fail under USE_SYSTEM_BUN=1, pass on the fixed build (221 pass), pass under BUN_JSC_validateExceptionChecks=1, and that all vendored test-vm-* files (including test-vm-basic.js which pins compileFunction's empty-name behavior) still pass. Net src/ change is small and strictly simplifying.
new vm.Script(code), vm.createScript(code), vm.runInThisContext(code),
vm.runInContext(code, ctx) and vm.runInNewContext(code) registered their
source under an empty filename, so stack frames and CallSite.getFileName()
reported file:/// instead of evalmachine.<anonymous>. The same happened
for { filename: undefined }. Only an options object without a filename key
got the default, because it was applied inside BaseVMOptions::fromJS.
Seed ScriptOptions with Node's default instead and let fromJS only
overwrite it with a provided string. That makes the filenameProvided flag
and the compileFunction reset (which undid the fromJS default to keep its
own empty default) unnecessary, so both are removed. An explicit "" is
still kept as is, and compileFunction still defaults to an empty name.
718caaf to
8fdcca7
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 8d95f75 has some failures in 🧪 To try this PR locally: bunx bun-pr 38318That installs a local version of the PR into your bun-38318 --bun |
There was a problem hiding this comment.
LGTM — moves the evalmachine.<anonymous> default to the ScriptOptions constructor so every call shape sees it, and removes the filenameProvided tracking and CompileFunctionOptions reset that only existed to compensate for the old placement.
What was reviewed:
- Confirmed
filenameProvidedand the three-argBaseVMOptionsconstructor have no remaining callers. - Verified
CompileFunctionOptions(stillusing BaseVMOptions::BaseVMOptions) keeps its empty-string default now thatfromJSno longer overwrites it, andRunningScriptOptions.filenameis unused for the actual source. - Checked
src/js/node/vm.ts— the two editedvmModuleRunIn*host functions are indeed unreachable (all wrappers go throughnew Script). - Tests cover the three no-filename shapes, explicit
"", string-form options,CallSite.getFileName(), and compile-time SyntaxError stacks;prepareStackTraceis restored infinally.
Extended reasoning...
Overview
This PR fixes node:vm so a Script built without an options argument (or with {filename: undefined}) is named evalmachine.<anonymous> in stack frames and CallSite.getFileName(), matching Node. Previously only new Script(code, {}) got the default; other shapes fell through to an empty filename that rendered as file:///.
The fix moves the default from a branch inside BaseVMOptions::fromJS (which only ran when an options object was present and lacked the key) into the ScriptOptions default constructor, and makes fromJS leave the constructor's value alone unless options.filename is a string. That lets the PR delete the filenameProvided field, the compensating reset in CompileFunctionOptions::fromJS, and the now-unused three-arg BaseVMOptions constructor. The two vmModuleRunIn* host functions in NodeVM.cpp are updated only because they were the last users of that constructor; vm.ts builds those APIs on Script and never calls them.
Security risks
None. This changes only the default string used as a source URL for stack frames; no auth, crypto, filesystem, or user-input parsing is involved.
Level of scrutiny
Node compat in C++ JSC bindings, but the change is pure default-value plumbing: one constant moves from a parser branch to a constructor, and dead compensation code is removed. There is no memory management, no new exception paths (the removed unconditional toWTFString in the unreachable host functions actually drops one), no GC/threading. I traced each subclass of BaseVMOptions through the new flow: ScriptOptions gets the Node default in every shape; CompileFunctionOptions keeps its empty-string default via BaseVMOptions() = default; RunningScriptOptions.filename is never used for the compiled source (the Script already carries it).
Other factors
Test coverage is thorough — the shared testRunInContext matrix exercises all six run entry points, and the new standalone test pins new Script/createScript shapes, the string-options form, explicit "", getFileName(), and identical compile-error stacks. The PR body reports the tests fail on both released Bun and a debug build of main, pass with the fix, and that all vendored test-vm-* files pass under BUN_JSC_validateExceptionChecks=1. The comment-cop bot's note about a long comment was addressed in 8d95f75 (thread resolved). No CODEOWNERS cover these paths.
This branch has not been deployed
1 participant
Problem
vm.Scriptbuilt without an options argument registers its source under an empty filename, so its stack frames print asat file:///:1:10andCallSite.getFileName()returns"file:///". Node names itevalmachine.<anonymous>(lib/vm.js:filename = 'evalmachine.<anonymous>'destructuring default).new Script(code),new Script(code, { filename: undefined }),vm.createScript(code),vm.runInThisContext(code), and{ filename: undefined }passed to any of thevm.runIn*wrappers. (vm.runInContext(code, ctx)andvm.runInNewContext(code)without options stopped being affected when node:vm: reject array and function options like Node's validateObject #38381 made those two wrappers spreadoptionsinto a fresh object; bun 1.4.0 still shows them.) The compile-timeSyntaxErrorfromnew Script("%%")has the same problem in its frame (at <parse> (:1)); only its arrow header was already right.new Script(code, {})was already correct, so the two shapes disagreed.constructScriptstarted fromScriptOptions options(""_s)(src/jsc/bindings/NodeVMScript.cpp:106) and theevalmachine.<anonymous>default lived insideBaseVMOptions::fromJS(src/jsc/bindings/NodeVM.cpp:1963), which only runs that branch when an options object exists and has nofilenamekey. No options object, orfilename: undefined, never reached it, and the empty name then rendered through theSourceOriginfallback asfile:///.Fix
ScriptOptionsnow seedsfilenamewithevalmachine.<anonymous>in its constructor;BaseVMOptions::fromJSonly overwritesfilenamewhenoptions.filenameis a string. The default is therefore in effect for every call shape, and a provided filename (string argument orfilename:property,""included) still replaces it, as in Node.fromJSis a parser shared byScript,compileFunctionand the run options, and the APIs have different defaults (Node'scompileFunctiondefaults to""). Putting theScriptdefault in the parser is what forcedCompileFunctionOptions::fromJSto undo it afterwards andconstructScriptto trackfilenameProvidedfor its header. With the default onScriptOptions, both workarounds are dead and are removed, and the compile-error header just usesoptions.filenamelikecompileFunctionalready did.compileFunctionkeeps its empty default (default-constructed options, same as the reset produced before); an explicit""still renders the compile-error header as:1(existing tests invm.test.tspin both).file:///, Node prints<anonymous>), and the runtime arrow header inhandleException(NodeVM.cpp:561-567), which is built from the error's top frame and still substitutesevalmachine.<anonymous>for an empty URL (it rendersevalmachine.<anonymous>:1for an explicit""where Node renders:1, and[native code]:0when a builtin threw). That is a frame-selection problem inhandleExceptionand is left for a separate change. TheSourceTextModulecounterpart (frames named after the module identifier) is node:vm: apply SourceTextModule lineOffset/columnOffset like Node and name frames after the identifier #38235.runInNewContext/runInThisContexthost functions inNodeVM.cppare compile fixes, not behavior: those functions are unreachable (vm.tsbuilds both APIs onScriptand never calls them), and they were the only users of the three-argumentBaseVMOptionsconstructor, which is deleted here along withScriptOptions' inherited constructors. node:vm: keep lineOffset/columnOffset from overflowing JSC parser positions #38228 deletes the functions themselves; whichever of the two lands second resolves by taking that deletion.test/js/node/vm/vm.test.ts. "defaults the filename to evalmachine. like Node" runs inside the shared matrix, so it coversvm.runInContext/runInNewContext/runInThisContextand the threeScript.prototype.runIn*paths, each with no options,{ filename: undefined }, and{ filename: "" }(must stay non-default). "the source itself is named evalmachine. when no filename is given" coversnew Scriptwithundefined/{}/{ filename: undefined },createScript, the string form ("named.js"and""),CallSite.getFileName(), and the compile-timeSyntaxErrorstacks, which must be identical across the three shapes.bun(USE_SYSTEM_BUN=1) and on a debug build of current main'ssrc/(at file:///:1:10; the twovm.runInContext/runInNewContextcases fail on their{ filename: undefined }assertion), and pass with the fix (bun bd test test/js/node/vm/vm.test.ts: 257 pass). All 97 vendoredtest/js/node/test/parallel/test-vm-*files, the threesequential/test-vm-*files, and the vendored buffer/util/repl tests that usenode:vmpass on the fixed build;test-vm-basic.jsin particular still pinscompileFunction's empty-name frames. The new tests also pass underBUN_JSC_validateExceptionChecks=1.Background
BaseVMOptions(NodeVM.h) holds thefilename/lineOffset/columnOffsetcommon to the vm option bags;ScriptOptions(new Script),CompileFunctionOptions(vm.compileFunction) andRunningScriptOptions(runIn*Contexton an existing script) derive from it and callBaseVMOptions::fromJSto read those properties from the user's options object.sourceURLof the JSCSourceCodethe script is compiled from. Stack frames,CallSite.getFileName()and the<url>:<line>arrow header Bun prepends to vm errors all read it. When it is empty, Bun's stack formatter falls back to theSourceOriginURL, which for a vm script isfileURLWithFileSystemPath(filename), i.e.file:///for an empty name; that fallback is why the bug showed up asfile:///.src/js/node/vm.tsimplementsrunInContext,runInNewContext,runInThisContextandcreateScripton top ofnew Script(code, options), which is why they share the constructor's bug. Since node:vm: reject array and function options like Node's validateObject #38381 the first two copyoptionsinto a fresh object first, so for them only a present-but-undefinedfilenamestill reaches the bug.Before / after (bun 1.4.0 vs. this branch; node v26.3.0 for reference)
(Node reports column 1 where JSC reports the column of the
new Errorexpression; the tests only check the name.)[review] gate passed · iteration 1 · 5 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