Conversation
new vm.Script(code).sourceMapURL returned undefined in Bun until the script was first run, while Node reports it right after construction. Bun parses the source lazily (at first run), so the sourceMappingURL directive was not yet set on the source provider when the getter read it. Parse the source the first time sourceMapURL is read (without generating bytecode) so the directive is populated on demand, and restore the directive from cached bytecode when a script is built from cachedData. This keeps construction cheap for scripts that never read the property.
|
Updated 8:42 AM PT - Jun 20th, 2026
❌ @robobun, your commit 966ccec has 2 failures in
🧪 To try this PR locally: bunx bun-pr 32534That installs a local version of the PR into your bun-32534 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough
ChangesScript#sourceMapURL directive resolution
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The duplicate detector is right: #32018 already fixes #32018 is a much larger effort (the full node:vm suite, +25 tests) and would supersede this. This PR is a focused standalone fix that closes #32530 on its own if landing the broader draft will take longer. Happy to close in favor of #32018 if you would rather keep it all in one place. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the fix is small, well-tested, and follows the existing checkSyntax pattern in NodeVM.cpp — but since it adds a parser invocation inside a JSC property getter and touches the cached-bytecode decode path, it's worth a quick human glance from someone familiar with the JSC bindings.
Extended reasoning...
Overview
This PR fixes #32530: new vm.Script(code).sourceMapURL returned undefined in Bun until the script was first run, whereas Node populates it at construction. The change touches three files:
src/jsc/bindings/NodeVMScript.h— adds abool m_directivesResolvedmember with a getter/setter pair.src/jsc/bindings/NodeVMScript.cpp— in thesourceMapURLcustom getter, lazily callsJSC::checkSyntax(vm, source, parserError)the first time the property is read so the parser populatessourceMappingURLDirective()on the provider; in thecachedDataconstruction path, copiessourceURLDirective/sourceMappingURLDirectivefrom the decodedUnlinkedProgramCodeBlockback onto the provider; marks directives resolved aftercacheBytecode()in theproduceCachedDatapath.test/js/node/vm/vm.test.ts— adds a 6-testdescribeblock covering construction, post-run, no-directive,produceCachedData, validcachedData, and rejectedcachedData.
Security risks
None identified. The change does not touch auth, crypto, permissions, or sandbox boundaries. The only new behavior is invoking JSC's syntax checker on source text the caller already supplied, and copying string directives between JSC structures. The ParserError from checkSyntax is captured locally and not re-thrown, so a syntactically-invalid script just leaves the directive empty (returns undefined), which matches existing behavior.
Level of scrutiny
This is C++ runtime code in the JSC bindings layer — not a config tweak or mechanical change. The logic itself is small (~25 net lines), well-explained, and mirrors established patterns: checkSyntax(vm, source, error) is already used identically in NodeVM.cpp:144, and restoring directives from a decoded unlinked code block onto the provider is what JSC's own CodeCache does on a cache hit. The new bool member sits between two existing scalar members on a JSDestructibleObject and needs no GC visitation. I don't see correctness problems.
That said, JSC bindings are performance- and correctness-critical, and adding a parser invocation inside a property getter (even a cheap, once-only checkSyntax) is the kind of thing a maintainer familiar with the bindings should confirm — e.g., that there's no exception/scope subtlety, and that the lazy-parse strategy is preferred over eager-parse-at-construction here.
Other factors
No CODEOWNERS apply to these paths. The bug-hunting system found no issues. Test coverage for the new behavior is thorough across all construction paths. CI is building. There are no outstanding reviewer comments. Given the change is in native bindings rather than pure JS/config, I'm deferring rather than auto-approving.
|
Thanks. For the two JSC-bindings points raised:
|
There was a problem hiding this comment.
I didn't find any issues — the lazy checkSyntax approach and cachedData directive restoration look sound and are well-tested — but since this touches JSC bindings and overlaps with the open #32018, a maintainer should decide which to land.
Extended reasoning...
Overview
This PR fixes vm.Script#sourceMapURL returning undefined until the script is first run (issue #32530). It touches three files:
src/jsc/bindings/NodeVMScript.h— adds am_directivesResolvedbool with getter/setter, following the existingm_cachedDataProduced/m_cachedDataRejectedpattern in the same class.src/jsc/bindings/NodeVMScript.cpp— three small additions: (1) thesourceMapURLgetter now lazily callsJSC::checkSyntax(vm, source, parserError)once to populate the provider's directive; (2) when decoding validcachedData, thesourceURLDirective/sourceMappingURLDirectiveare copied from theUnlinkedProgramCodeBlockback onto the source provider (mirroring JSC's ownCodeCachebehavior); (3) aftercacheBytecode()in theproduceCachedDatapath, the flag is marked resolved.test/js/node/vm/vm.test.ts— six new tests covering construction, post-run, no-directive,produceCachedData, validcachedData, and rejectedcachedData.
Security risks
None identified. The getter only triggers a syntax-only parse of source the user already supplied; the non-throwing checkSyntax(VM&, const SourceCode&, ParserError&) overload (already used in NodeVM.cpp) reports errors via the out-param rather than throwing, so no exception-scope hazard. No user JS or coercions run during the parse. The cachedData path reads two StringImpl* fields off an already-decoded UnlinkedProgramCodeBlock and stores them on the provider — no new attacker-controlled surface.
Level of scrutiny
Medium. The diff is small (~30 LOC of C++) and follows existing idioms in the file, but it lives in JSC bindings where mistakes around GC, exception scopes, or provider lifetime can be subtle. The author addressed the exception/GC-safety questions in the thread, and the checkSyntax overload used here is the same one already in NodeVM.cpp:144. I don't see anything wrong, but I'd prefer a human familiar with the JSC bindings to give it a quick pass rather than auto-approve.
Other factors
- A duplicate-detector flagged overlap with the larger draft #32018, and the author has explicitly offered to close in favor of it. Whether to land this focused fix now or fold it into #32018 is a maintainer call that shouldn't be made by a bot.
- Test coverage is thorough (6 cases covering every branch added) and the full
vm.test.tssuite reportedly stays green. - No CODEOWNERS entry covers these paths.
- The bug-hunting system found no issues.
|
CI status for whoever picks this up: the diff is green where it matters. The two red runs failed on different, unrelated tests each time, none touching
Disjoint failures across runs is the flake signature; a |
|
Closing as redundant: #32018 landed on main (commit 0672e7d) and already fixes this. The No need to force a rebase of a duplicate fix over the merged work. Thanks to @cirospaciari for folding this into the broader node:vm pass. |
1 participant
What / why
new vm.Script(code).sourceMapURLreturnedundefinedin Bun until the script was first run. Node reports it right after construction.Fixes #32530.
Cause
The
sourceMapURLgetter readsscript->source().provider()->sourceMappingURLDirective(). That directive is only set while JSC's parser walks the source. Bun parses avm.Scriptlazily (at first run), whereas Node compiles eagerly at construction, so the directive was empty until the script ran. Running the script first made the value appear, which pinned the cause.The
cachedDatapath had the same gap: the directive is stored in the serialized bytecode but was never copied back onto the source provider after decoding.Fix
sourceMapURLis read, usingcheckSyntax(no bytecode generation), and cache the result with a flag so scripts that never read the property keep construction cheap and repeated reads do not re-parse.cachedData, restore thesourceURL/sourceMappingURLdirectives recorded in the decoded bytecode onto the provider (mirrors whatCodeCachedoes on a cache hit).Verification
New tests in
test/js/node/vm/vm.test.tscover construction, post-run,produceCachedData, validcachedData, rejectedcachedData, and the no-directive case.Without the fix (
USE_SYSTEM_BUN=1), the construction, valid-cachedData, and rejected-cachedDatacases fail withundefined. The fullvm.test.tssuite stays green (212 pass).