Skip to content

sourcemap: accept a source map whose sourcesContent is absent or null - #36582

Open
robobun wants to merge 9 commits into
mainfrom
farm/d05b985c/sourcemap-optional-sourcescontent
Open

robobun wants to merge 9 commits into
mainfrom
farm/d05b985c/sourcemap-optional-sourcescontent

Conversation

@robobun

@robobun robobun commented Jul 31, 2026 •

Copy link
Copy Markdown
Collaborator

Related to #15644 and #32070

Problem

  • A // @bun file whose source map has no sourcesContent keeps its stack unmapped and prints warn: Could not decode sourcemap in '<file>': InvalidSourceMap.
  • parse_json (src/sourcemap/lib.rs:909-917) requires a sourcesContent list. ECMA-426 makes it optional, and projects strip it from bun build maps (Notes).

Fix

  • An absent or null sourcesContent now means no source text, as in the parse_json change of Runtime plugins: positions go through the inline source map of what onLoad returned #46148.
  • Lookup::get_source_code reads no file for an empty source name. In a blob: module that read aborts (panic: slice index starts at 1 but ends at 0), and these maps now reach it.
  • Verified: 9 new tests in test/js/bun/sourcemap/negative-vlq-mapping.test.ts and test/js/bun/util/inspect-error.test.js fail on main and pass here.
  • Self-reviewed: 42 concerns raised on a larger shape. The review recommended this reduction (Notes).

Background

  • Bun runs a // @bun file (bun build output) as it is. The map serves stacks, and sourcesContent only code frames.
  • Bun finds <file>.map by the file's name alone.
  • Considered: accepting every shape, and a rule for which <file>.map belongs to a file. Both need a maintainer ruling and left this PR.

Downsides

  • Needs a ruling: a leftover map from another tool (tsc's default has no sourcesContent) is now applied silently, as main applies one that has sourcesContent.
  • sourcemap: ignore a sourceMappingURL line inside a string or a block comment #47347 must merge first. Without it, such a map inside a bundled string replaces the bundle's own map.
  • An accepted map also changes coverage (LF:14 LH:10 to LF:8 LH:4), the JUnit line (13 to 16) and Bun.cron paths, as on main with sourcesContent: [null].
Notes

Reports.

The shapes. ECMA-426 decodes a missing sourcesContent with no error step. For null it lets a consumer report an error and continue, and rollup writes null. These three shapes stay refused with InvalidSourceMap, as on main and in #46148:

  • a sourcesContent that is not a list and not null
  • a list that is not as long as sources
  • a sources entry that is not a string

Measurements. "main" and "here" are two release builds of the same tree (main 89452d2). They differ only in src/sourcemap/lib.rs and src/sourcemap/Mapping.rs.

What main here
<file>.map cases that remap, of 56 (8 ways a file can link the map, 7 shapes of the map) 8 24
Inline data: map cases that use the inline map, of 21 3 9
tc39 valid maps that decode, of 32 (test/js/node/test/fixtures/test426) 19 25
Instructions per map lookup, err.stack, map with sourcesContent, 1 source 28,662 28,656
The same, 101 sources 577,690 577,582
Instructions per lookup for a printed error, and for the read of its code frame 14,210 and 11,876 14,181 and 11,845
Lookup::get_source_code, code frame read from disk 10,573 10,545
Instructions per remapped frame (Lookup::display_source_url_if_needed) 1,310 1,310
Syscalls and allocator calls per lookup 5 and 39 5 and 39
parse_json, bytes 2,705 2,824
Lookup::get_source_code, bytes 818 841
Text of the stripped binary, bytes 143,094,946 143,094,946
Parses of a map with sourcesContent: two printed errors, one uncaught error 2 and 1 2 and 1
Parses of a map with no sourcesContent: the same two programs 1 and 1 (refused) 3 and 2
bun test --coverage of a one-source bundle whose map has no sourcesContent LF:14 LH:10 LF:8 LH:4
JUnit line of a test in such a file 13 16
toMatchInlineSnapshot() that must write its snapshot, in such a file test passes, then Failed to update inline snapshot, exit 1 test fails: Inline snapshot matchers must be called from the test file, exit 1
blob: module, empty source name, sourcesContent: [null], printed error exit 134 error printed, exit 0
New tests that pass, of 9 0 9
  • The instruction counts are gdb single-step counts of erased_get_source_map. The 101-source row is a hardware counter (perf_event_open), three runs each: 577,690 to 577,700 on main and 577,580 to 577,590 here. valgrind, perf, strace and bloaty are not installed on the machine.
  • The 56 cases cross 8 link states (no comment, sourceMappingURL that names the map, the same after ./, the name of another map, equal debug ids, different debug ids, an id in the file only, an id in the map only) with 7 shapes (a full list, absent, null, shorter, longer, a string, a null source). Only the absent and null columns change, in all 8 link states.
  • The tc39 maps that still fail are the 4 index maps (sections), validMappingLargeVLQ (sourcemap: decode a VLQ written with more than 8 digits #47380 decodes it) and the two maps with a null entry in sources.
  • A map with no sourcesContent costs one more parse for each printed error, because the code frame read decodes the map again and finds no text.

Readers of an accepted map. A map that decodes feeds every reader of a source position. For a map with no sourcesContent each of these now gives what main gives for the same map with sourcesContent: [null]:

  • bun test --coverage counts the lines of the original source under the name of the bundle.
  • The JUnit reporter writes the original line beside the file name of the bundle.
  • CallSite.getFileName() and Error.prepareStackTrace see the original path.
  • toMatchInlineSnapshot() with no stored snapshot fails in the matcher. A stored snapshot that matches passes on both builds.
  • Bun.cron("./job.js") from such a bundle resolves the path against the original source directory. With no src/ directory deployed it throws Failed to resolve path. I measured this in October on an earlier build of this branch, which decoded these maps the same way.

The three questions that this PR leaves to a maintainer.

  1. Is a <file>.map with no sourcesContent used when nothing in the file links it? This PR says yes, as Runtime plugins: positions go through the inline source map of what onLoad returned #46148 does. Repro: bun build src/index.ts --outdir dist --target bun, then put the map that tsc writes for its own dist/index.js beside the bundle. main prints the warning and dist/index.js:4:9. This PR prints src/index.ts:2:3 and no warning. main prints the same wrong frame when that map has sourcesContent.
  2. Do two different debug ids in the file and the map refuse the map? sourcemap: ignore a <file>.map whose debugId is not the debug id of the file #47628 proposes that.
  3. Do the three shapes above keep failing? This PR says yes, as Runtime plugins: positions go through the inline source map of what onLoad returned #46148 does.

Other open PRs on these lines.

What left this PR. Earlier heads of this branch accepted every shape of sourcesContent, named a sources entry that is not a string null, and showed an empty source name as the file. A later local version used a <file>.map of a refused shape only when the file linked it by debug id or by sourceMappingURL. The self-review found that both decide questions 1 to 3 in code, against the only maintainer-written change on the same lines (#46148). The link rule is kept on the branch robobun/d05b985c/sourcemap-pairing-guard-wip for #47628.

Not in this change

The AOT list. test/aot/runner-suites/programs-3.test.ts runs inspect-error.test.js with every program compiled. A compiled // @bun program reads no source map, so test/aot/runner-suites.programs.txt now names the five new tests of that file, beside the test of #38296.

Suites run with the debug build. test/js/bun/sourcemap/ 371 pass. inspect-error.test.js 52 pass. test/js/node/module/sourcemap.test.js 14 pass. test/cli/test/coverage.test.ts 45 pass. test/bake/dev/sourcemap.test.ts 2 pass. test/bake/dev/server-sourcemap.test.ts 5 pass. compile-sourcemap-internal.test.ts 1 pass. test/regression/issue/22003.test.ts 1 pass. cargo clippy -p bun_sourcemap is clean. The two changed test files also pass on the release build: 14 and 52. test/aot/runner-suites/programs-3.test.ts passes on the release build.


[human-review] gate passed · iteration 3 · 5 files touched

fails on main (without fix)
ASAN without fix: 9 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/sourcemap/negative-vlq-mapping.test.ts test/js/bun/util/inspect-error.test.js
bun test v1.4.4 (367d939d9)

test/js/bun/sourcemap/negative-vlq-mapping.test.ts:
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative original line [1353.45ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original column [1533.40ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original line [1623.00ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative generated column [1667.93ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative generated column [1903.09ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative original column [931.68ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > new module.Sour
... (truncated)

release without fix: all passed
bun test v1.4.4-canary.1 (00b8ba741)

test/js/bun/sourcemap/negative-vlq-mapping.test.ts:
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative generated column [118.54ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original line [95.66ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative generated column [110.22ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative original column [77.46ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative original line [90.75ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original column [86.13ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > SourceMap findEntry/findOrigin with negative line/column return empty objects [59.00ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > new module.SourceMap({mappings}) with negative generated column throws [
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/sourcemap/negative-vlq-mapping.test.ts test/js/bun/util/inspect-error.test.js
bun test v1.4.4 (367d939d9)

test/js/bun/sourcemap/negative-vlq-mapping.test.ts:
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative generated column [1044.82ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative original line [924.63ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original line [968.06ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > inline data-URL map: negative generated column [1053.83ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > external .map: negative original column [1106.83ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not asserted > new module.SourceMap({mappings}) with negative generated column throws [491.49ms]
(pass) sourcemap: negative VLQ delta in mappings is rejected, not ass
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 10959ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/119] build.rs build_script_build
[2/119] rustc bun_zlib_sys 
[3/119] rustc bun_safety 
[4/119] rustc bun_output 
[5/119] rustc bun_boringssl_sys 
[6/119] rustc bun_brotli 
[7/119] rustc bun_perf 
[8/119] rustc bun_ptr 
[9/119] rustc bun_cares_sys 
[10/119] rustc bun_clap 
[11/119] rustc bun_zstd 
[12/119] rustc bun_picohttp 
[13/119] rustc bun_base64 
[14/119] rustc bun_errno 
[15/119] rustc bun_text_diff 
[16/119] rustc bun_tcc_sys 
[17/119] rustc bun_valkey 
[18/119] rustc bun_lsquic_sys 
[19/119] rustc bun_shell_parser 
[20/119] rustc bun_zlib 
[21/119] rustc bun_collections 
[22/119] rustc bun_paths 
[23/119] rustc bun_core 
[24/119] rustc bun_url 
[25/119] rustc bun_semver 
[26/119] rustc bun_png 
[27/119] rustc bun_analytics 
[28/119] rustc bun_tar 
[29/119] rustc bun_http_types 
[30/119] rustc bun_which 
[31/119] rustc bun_dns 
[32/119] rustc bun_node_path 
[33/119] rustc bun_archive 
[34/119] rustc bun_spawn_sys 
[35/119] rustc bun_sha_hmac 
[36/119] rustc bun_boringssl 
[37/119] rustc bun_thr
... (truncated)
diff hotspot
src/sourcemap/Mapping.rs                           |   4 +
 src/sourcemap/lib.rs                               |  22 ++---
 test/aot/runner-suites.programs.txt                |   5 +
 test/js/bun/sourcemap/negative-vlq-mapping.test.ts |  48 +++++++++-
 test/js/bun/util/inspect-error.test.js             | 101 +++++++++++++++++++++
 5 files changed, 167 insertions(+), 13 deletions(-)

gate history · 2 passed · 0 rejected · iteration 3

evidence per changed file
file                                                reads  edits  tests
src/sourcemap/Mapping.rs                                5      4     81
src/sourcemap/lib.rs                                    8     14     83
test/aot/runner-suites.programs.txt                     0      0     83
test/js/bun/sourcemap/negative-vlq-mapping.test.ts      1      0     48
test/js/bun/util/inspect-error.test.js                  4      6     60

The runtime source-map decoder (parse_json in src/sourcemap/lib.rs, used
to remap error.stack for // @Bun pragma files) rejected any map whose
sourcesContent key was absent or null with InvalidSourceMap, and also
rejected maps where sourcesContent.length != sources.length.

Per the source-map v3 spec, sourcesContent is optional. Node's
--enable-source-maps and most other consumers accept these maps.

Treat absent/null sourcesContent as no-content-available and drop the
strict length check; the per-index bounds check when extracting content
already handles short arrays.
@robobun

robobun commented Jul 31, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: reduced to the decode fix in 00b8ba7 (merged with main at 89452d2, head abe715c). The description has the details.

  • Reproduction: a // @bun file with an external .map that has no sourcesContent keeps its stack unmapped and prints warn: Could not decode sourcemap in '<file>': InvalidSourceMap. Bun 1.4.2 does this, and so does a release build of main at 89452d2. The 9 new tests fail on that build. They pass on a debug build and on a release build of this branch.
  • A second reproduction, a crash: a // @bun module imported from a blob: URL, whose inline map has sources: [""] and sourcesContent: [null], aborts with panic: slice index starts at 1 but ends at 0 on Bun 1.4.2 and on main when the error is printed. This branch prints the error and continues.
  • What changed: the PR now has only the parse_json lines of Runtime plugins: positions go through the inline source map of what onLoad returned #46148 (an absent or null sourcesContent), the empty-name return in Lookup::get_source_code, and tests. The wider shapes and the rule for which <file>.map belongs to a file left the PR. Both need a maintainer ruling.
  • For a reviewer: the first Downsides bullet is a yes or no question. sourcemap: ignore a sourceMappingURL line inside a string or a block comment #47347 should merge first.
  • CI: build 139984 has one red test, and this change does not touch it: test/aot/parallel/node-http/objects-closed-proxies-without-traps.ts on macOS x64 (the order of two HTTP events differs in a compiled run). The two changed test files passed on every platform, macOS x64 included: negative-vlq-mapping.test.ts 14 pass, inspect-error.test.js 52 pass. Three macOS x64 shards were still running at 07:50 UTC on 2026-11-11.

@coderabbitai

coderabbitai Bot commented Jul 31, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The source-map parser now accepts missing or null sourcesContent and skips the length check when content is unavailable. External source lookup returns no content for an empty source name. Tests cover mapped frames, code frames, and map selection.

Changes

Source map content handling

Layer / File(s) Summary
Parse maps with unavailable source content
src/sourcemap/lib.rs, test/js/bun/sourcemap/negative-vlq-mapping.test.ts, test/js/bun/util/inspect-error.test.js, test/aot/runner-suites.programs.txt
The parser accepts missing or null sourcesContent, checks lengths when content is an array, and returns no content for unavailable indexed entries. Tests cover external map selection, mapped frames, and code frames; the AOT runner suite includes the added cases.
Stop lookup for empty source names
src/sourcemap/Mapping.rs, test/js/bun/util/inspect-error.test.js
External source lookup returns no content when the selected source name is empty. Tests cover inline maps with an empty source name and unavailable content.

Possibly related issues

  • oven-sh/bun#32070 — The issue requests source maps without sourcesContent; this change allows the runtime to consume maps without that field, but does not add an emission option.

Possibly related PRs

  • oven-sh/bun#47240 — Both changes affect source-index validation and resolution during source-map parsing.
  • oven-sh/bun#47659 — Both changes modify Lookup::get_source_code and affect source resolution.

Suggested reviewers: jarred-sumner

Merge Risk: Low · up to 66bd2

Merge Risk: 🔵 Low · up to 66bd2

Certain bundled strings can cause error stacks to show unrelated source locations. This narrow debugging issue does not block merging.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: accepting source maps with absent or null sourcesContent.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description clearly explains what the PR changes and provides extensive verification details, including tests, builds, affected behavior, limitations, and open maintainer decisions. It does not us…
Full details: Description check

Explanation

The description clearly explains what the PR changes and provides extensive verification details, including tests, builds, affected behavior, limitations, and open maintainer decisions. It does not use the exact template headings, but it contains the required information.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 00b8ba7 and abe715c.

Files selected for processing: 1

  • test/aot/runner-suites.programs.txt

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — small, spec-aligned relaxation with good test coverage.

What was reviewed:

  • parse_json now treats absent/null sourcesContent as None and drops the length-equality check; the per-index bounds guard on content_slice still protects short/empty arrays.
  • Vec::with_capacity switched to sources_paths.items().len(), which is what the loop actually iterates — correct now that the two lengths are no longer forced equal.
  • Tests cover absent/null/empty-array plus a [null] control that already passed on main, use test.concurrent.each with proper pipe draining, and assert the remapped location rather than just absence-of-warning.
Extended reasoning...

Overview

Relaxes the runtime source-map JSON decoder in src/sourcemap/lib.rs to accept maps where sourcesContent is absent, null, or shorter than sources, instead of rejecting the whole map with InvalidSourceMap. Per the source-map v3 spec this field is optional, and Node/other consumers already accept these shapes. Net Rust change is ~20 lines: the field lookup becomes Option<_>, the strict length-equality check is removed, and the single downstream consumer (content_slice) matches on the Option while retaining its existing per-index bounds check. Four new parametrized subprocess tests added to test/js/node/module/sourcemap.test.js.

Security risks

None. This is a read-only decoder used to remap error.stack for // @bun-pragma files. Relaxing acceptance of an optional field does not introduce new indexing or allocation driven by untrusted lengths — the only array index into sourcesContent is still guarded by (idx as usize) < items().len(), and the with_capacity argument now derives from sources (which was already required and iterated). A longer-than-sources array is now also accepted, but extra entries are simply never indexed.

Level of scrutiny

Low-to-moderate. The change is mechanical (required → optional), touches a single function, and only widens acceptance in the direction the spec mandates. Failure mode is bounded: worst case is source_contents: None in the returned ParseUrl, which callers already handle (the pre-existing [null] / "" paths produced the same). The StandaloneModuleGraph serializer path is untouched, as noted in the PR — it consumes bun's own bundler output which always carries content.

Other factors

  • Tests follow harness conventions: test.concurrent.each for independent subprocess cases, tempDir/bunEnv/bunExe, concurrent pipe drain, exit-code asserted last.
  • The [null] variant is an explicit control that passes on main, giving confidence the test fixture (hand-rolled VLQ segment mapping generated 3:3 → original 5:3) is itself correct.
  • PR description shows USE_SYSTEM_BUN=1 fails 3/4 new cases and bun bd test passes all 18, and robobun independently reproduced.
  • No prior reviewer comments to address.

Comment thread src/sourcemap/lib.rs Resolved
@robobun

robobun commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:08 AM PT - Nov 11th, 2026

@robobun, your commit 66bd2de is building: #140077

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/sourcemap/lib.rs Resolved
Comment thread src/sourcemap/lib.rs Resolved
Comment thread test/js/node/module/sourcemap.test.js Resolved
Comment thread test/js/node/module/sourcemap.test.js Resolved
Comment thread src/sourcemap/lib.rs Resolved
@robobun
robobun marked this pull request as draft October 27, 2026 23:10
…uested index

`parse_json` returned `InvalidSourceMap` when `sourcesContent` was
absent, was not an array, or had a length other than the length of
`sources`. The field only supplies the text of a code frame. A position
needs `mappings` and `sources` only, and ECMA-426 decodes an absent
field and a length mismatch with no error.

`parse_json` no longer reads the key before it decodes the mappings. It
reads the entry for the source index that the caller asked for, and only
when that index has a `sources` entry.

A `sources` entry that is null or not a string is a source named `null`.

More maps decode now, and they reach two places that use a source name:

- `Lookup::get_source_code` joined the directory of the file with the
  name. A module that has no directory (a `blob:` URL) and an empty name
  made that join panic with "slice index starts at 1 but ends at 0",
  also on main for a map with `sourcesContent: [null]`. It now reads no
  file when the path of the module is not absolute.
- An empty source name is the file itself. `get_source_map_impl` gives
  it the name of the file when the map is decoded.

The tests are in the files that already cover the runtime decoder and
the printed error.
@robobun robobun changed the title sourcemap: accept absent/null/short sourcesContent in runtime decoder sourcemap: never reject a map over sourcesContent, read it at the requested index Oct 28, 2026
@robobun

robobun commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator Author

@dylan-conway this PR overlaps #46148. Both rewrite the sourcesContent handling in parse_json (src/sourcemap/lib.rs), and both add a block at the end of test/js/bun/sourcemap/negative-vlq-mapping.test.ts.

Two questions:

  1. Which PR do you want to land first? The second one drops its parse_json hunk and merges the two test blocks.
  2. Runtime plugins: positions go through the inline source map of what onLoad returned #46148 keeps three rejections that this PR removes: a sourcesContent that is neither an array nor null, a sourcesContent whose length differs from sources, and a sources entry that is not a string. Are they intended? If they are, I will cut this PR down to the absent and null cases.

Three results from the tests of this PR that I expect to apply to #46148 too, because it also accepts a map that has no sourcesContent. I read its diff. I did not run its build.

  • A // @bun module imported from a blob: URL, whose inline map has an empty source name, aborts with panic: slice index starts at 1 but ends at 0. main does this today when the map has sourcesContent: [null]. This PR adds a guard in Lookup::get_source_code.
  • tsc && bun build src/index.ts --outdir dist --target bun leaves tsc's dist/index.js.map beside the bundle. main rejects that map with a warning, because tsc writes no sourcesContent. When such a map decodes, it is applied with no warning and the frames are wrong.
  • An inline map inside a bundled string (\n//# sourceMappingURL=data:... in a template literal) replaces the bundle's own map when it decodes. On main that needs a map with sourcesContent and plain ;base64. In my runs the charset=utf-8;base64 lines widened it from 1 of 4 forms to 4 of 4.

The Notes in the description have the steps for the last two.

@robobun
robobun marked this pull request as ready for review October 28, 2026 16:41

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I also checked whether dropping the MappingsOnly gate on the sourcesContent lookup in parse_json changes the error.stack path — it does not, since MappingsOnly yields source_index = None at src/sourcemap/lib.rs:1015, so no content is fetched or copied there.

Extended reasoning...

The change relaxes sourcesContent and sources validation in the Rust source-map parser (src/sourcemap/lib.rs), rewrites empty source names to the module basename, and guards the on-disk source read in src/sourcemap/Mapping.rs for non-absolute base filenames; it parses untrusted map JSON but the new index lookups are bounds-checked via Option. Inline findings are posted on the parse relaxation, so a human should weigh those; this note only records the one additional path examined and ruled out.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/sourcemap/lib.rs
Comment on lines 916 to 918
let sources_paths = match json
.get(b"sources")
.ok_or(crate::Error::InvalidSourceMap)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Users running a // @ bun bundle that sits next to a stale <file>.map written by another tool (tsc's default sourceMap: true emits no sourcesContent) now get stack frames silently remapped by a map for a different generated file, where the base printed Could not decode sourcemap and kept the real frames. The sidecar lookup at src/sourcemap/lib.rs:517-538 reads <file>.map with no sourceMappingURL or debugId reference, and parse_json at lib.rs:916-924 no longer rejects it. Fix: before accepting a map that does not declare sourcesContent, require that the file references it (a sourceMappingURL/debugId comment, or a matching file field), or land the discovery fix the description names before this one.

Why this was flagged

Trigger: tsc emits dist/index.js and dist/index.js.map (no sourcesContent, tsc's default), then bun build src/index.ts --outdir dist --target bun overwrites dist/index.js with a // @ bun bundle and no --sourcemap. On the first error.stack or printed error, get_source_map_impl with load_hint None finds no inline URL and reads dist/index.js.map from disk at src/sourcemap/lib.rs:517-538 without any reference from the file. On the base branch old lib.rs:910-917 returned InvalidSourceMap for the missing sourcesContent, so lib.rs:545-552 printed Could not decode sourcemap in '<file>': InvalidSourceMap and the frames stayed on dist/index.js. After this diff lib.rs:916-924 accepts the map, SavedSourceMap.rs:321 caches it, and VirtualMachine.rs:6758-6797 rewrites the frame URL and position using tsc's mappings for tsc's own output, which do not describe Bun's bundle, so the user sees wrong src/index.ts lines with no warning. No safeguard checks that the sidecar belongs to the file.

Verification: After the diff, parse_json at lib.rs:916-924 only requires mappings and sources; sourcesContent is looked up lazily at lib.rs:1019-1030 and its absence yields content_slice = None without error, so the stale tsc map parses Ok and is adopted as the file's map (lib.rs:539). Stack frames and the uncaught-error preview are then remapped through a map generated for a different file, with no warning.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed. It is the second Downsides bullet of the description. My run: tsc && bun build src/index.ts --outdir dist --target bun leaves tsc's dist/index.js.map beside the bundle. main prints the warning and the true frames (dist/index.js:2:25). This branch prints src/index.ts:2:10 for both frames with no warning. main already does that when the leftover map has sourcesContent (tsc with --inlineSources), so the fault is in how the runtime picks a map for a file, not in parse_json.

The fix is a separate change: read <file>.map only when the trailing comments of the file name a map (//# sourceMappingURL= or //# debugId=). This PR should merge after it, and I leave this thread open until then. I did not add a check here that is keyed on sourcesContent. That check keeps the field in control of whether a map is used, and the general check also fixes the case that is on main today.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update. This finding still applies, and the PR no longer tries to remove it in code.

The PR is now only the parse_json lines of #46148: an absent or null sourcesContent decodes for every caller. So the map in this thread is applied with no warning. On two release builds of the same tree, after bun build src/index.ts --outdir dist --target bun and with the default map of tsc as dist/index.js.map: main prints the warning and at boom (dist/index.js:4:9). This branch prints at boom (src/index.ts:2:3). main prints that same frame when the leftover map has sourcesContent.

I built a rule for this (use such a map only when the file has its debug id or names it) and took it out again. It decides which <file>.map belongs to a file. No maintainer has ruled on that, and #46148 decides it the other way on the same lines. The rule is kept on the branch robobun/d05b985c/sourcemap-pairing-guard-wip.

The first Downsides bullet of the description puts the question to the reviewer. I leave this thread open for that ruling.

Comment thread src/sourcemap/lib.rs Resolved
Comment thread src/sourcemap/lib.rs Resolved
@robobun

robobun commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator Author

A note on one row of the measurements table: toMatchInlineSnapshot in such a test file, passes on main.

On a release build of main (11c3671) that run does not write the snapshot either.

printf '// @bun\nimport { test, expect } from "bun:test";\ntest("t", () => { expect(1 + 1).toMatchInlineSnapshot(); });\n//# sourceMappingURL=a.test.js.map\n' > a.test.js
echo '{"version":3,"sources":["orig.test.ts"],"names":[],"mappings":";;AA0CA"}' > a.test.js.map
CI=false bun test ./a.test.js
warn: Could not decode sourcemap in '<dir>/a.test.js': InvalidSourceMap
(pass) t
error: Failed to update inline snapshot: Could not find 'toMatchInlineSnapshot' here
    at <dir>/a.test.js:3:54

The exit code is 1 and the file is not changed. JSC reports a call in a // @bun file at the end of the matcher name, and the writer requires the start of the name. A // @bun file with no source map gets the same error.

So for this row the PR changes which error the file gets. The matcher throws Inline snapshot matchers must be called from the test file, which main does today for a map that has sourcesContent. On both builds the run exits with 1 and writes nothing.

I looked at a fix for inline snapshots in // @bun test files and do not propose one now. No user has reported the case, and #44833 rewrites the statement in Expect::inline_snapshot that finds the call. A working version is on the branch robobun/3c86ef26/inline-snapshot-executing-file.

@robobun

robobun commented Nov 1, 2026 •

Copy link
Copy Markdown
Collaborator Author

An answer to the question about a leftover <file>.map beside a // @bun bundle, which came up in the review of this PR.

  • sourcemap: ignore a <file>.map whose debugId is not the debug id of the file #47628 makes Bun ignore a <file>.map whose debugId is not the debug id of the file, with a warning. It does not depend on this PR.
  • It does not cover a bundle with no //# debugId= line beside a leftover map. Bun still applies that map. A rule for that case (Bun reads <file>.map only when the last lines of the file name it) changes a default. open-bauth@1.6.0 on npm is that case, and not a package that the rule would hurt: its build script has no --sourcemap, its dist/index.js has no id line and 20,194 lines, and the dist/index.js.map beside it maps 20,123 generated lines. (An earlier version of this comment said that this map fits. It does not.) A maintainer must decide that first, so there is no PR for it.
  • For this PR: it accepts a map with no sourcesContent, so a map that tsc left beside a // @bun file starts to apply. If that is not wanted, a small guard here can accept a map with no sourcesContent only when the file names the map.

robobun and others added 3 commits November 10, 2026 08:53
`parse_json` refused a map with no `sourcesContent`, so the stack of a
`// @bun` file stayed unmapped and `InvalidSourceMap` was printed. A
missing or `null` `sourcesContent` now means that the map has no source
text. A value that is not a list, a list of another length than
`sources`, and a `sources` entry that is not a string are refused as
before.

`Lookup::get_source_code` reads no file for a source that has no name.
In a module with no directory (a `blob:` URL) that read aborted with
`slice index starts at 1 but ends at 0`, and a map with no
`sourcesContent` now reaches it.

The `parse_json` change is the one in #46148. The earlier changes of this
branch (every shape of `sourcesContent`, sources with no name, the
basename for an empty source name) are taken out.

Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
@robobun robobun changed the title sourcemap: never reject a map over sourcesContent, read it at the requested index sourcemap: accept a source map whose sourcesContent is absent or null Nov 11, 2026

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beyond the inline findings, I checked the description's claim that the sources/sourcesContent length check and the non-string sources rejection were dropped: the current diff keeps both (src/sourcemap/lib.rs:926 and :937), so the "longer than sources" and null-named-source shapes from the description's table are still rejected in this revision and are not a concern here; the description and its table describe an earlier version of the change.

I did not build or run the new test rows, so the fail-on-main / pass-here claim for them was not verified in this review.

Extended reasoning...

The latest push narrows the change to accepting an absent or null sourcesContent in parse_json and an empty-name guard in Lookup::get_source_code, plus tests in two existing files. The PR description still presents the broader earlier revision (no length check, null source names), which the current code does not match. No security-sensitive surface is touched; the diff is small, but the sibling display-URL path and the stale-sidecar concern from an earlier round remain open inline threads, so a human look is still needed.

Still open from earlier reviews (1):

  • Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/sourcemap/Mapping.rs — nit, pre-existing: with a module whose URL has no directory and a non-empty relative source name, the code-frame read opens a wrong root path such as /rig.ts. The guard at src/sourcemap/Mapping.rs:387 only covers an empty name; dirname of a blob: URL is "" and join_abs_string_buf_checked assumes an absolute cwd, so it emits "/" plus the name with its first byte dropped. Fix: gate the disk read on bun_paths::is_absolute(base_filename) as display_source_url_if_needed already does at Mapping.rs:328, which also covers the empty name. A map without sourcesContent now reaches this path where the base rejected it.

    Why this was flagged

    Trigger: a module imported from a blob: URL whose inline map has sources: ["orig.ts"] and no sourcesContent (or [null], which the base already accepted), and an uncaught error so Lookup::get_source_code runs. src/paths/resolve_path.rs:646-653 returns "" for dirname of a string with no separator. src/sourcemap/Mapping.rs:396-398 then calls join_abs_string_buf_checked("", buf, ["orig.ts"]). In _join_abs_string_buf at src/paths/resolve_path.rs:2030-2049 the concatenated "orig.ts" has no leading separator, so leading_len is 1 and normalize runs on temp_buf[1..], giving "/rig.ts". src/sourcemap/Mapping.rs:399 reads that file; if it exists its lines are shown as the code frame for orig.ts, otherwise None. The new empty-name guard at Mapping.rs:387 does not apply because the name is not empty. On the base the no-sourcesContent map was rejected so this path was reached only for [null].

    Verification: nit, pre-existing (the base already reaches the same route when the map carries sourcesContent: [null]). Trigger: a module loaded from a blob: URL whose inline map has a non-empty relative source name and no usable sourcesContent. src/sourcemap/Mapping.rs:387-389 only returns early for name.is_empty(). Result: "/rig.ts", which Mapping.rs:399 opens.

Comment thread src/sourcemap/Mapping.rs Resolved
`test/aot/runner-suites/programs-3.test.ts` runs `inspect-error.test.js`
with every program it starts compiled. A compiled `// @bun` program does
not read a source map, so the five new tests fail there like
`external map whose original source is unavailable` does.
@robobun

robobun commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

The PR is reduced, and the title and the description now match the head.

It has the parse_json lines of #46148 (an absent or null sourcesContent), a return for an empty source name in Lookup::get_source_code, and tests. The other refused shapes stay refused. So the questions of my comment of 2026-10-28 are answered the way #46148 answers them. @dylan-conway, one question is left for a reviewer: the first Downsides bullet.

On the review of 00b8ba7:

  • The description was updated after that review started. It no longer has the table of the earlier version.
  • /rig.ts for a relative source name in a module that has no directory: confirmed, and main does it for sourcesContent: [null]. Give a relative Bun.file() blob module an absolute source URL #46842 changes that statement. A gate on the module URL would also stop the read of an absolute source name. main reads that one today: a blob: module whose map names an absolute source and has sourcesContent: [null] prints the code frame from disk. It is now under "Not in this change".
  • The thread on the leftover map stays open. It is the question for the reviewer.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no new issues

No new issues were found in this update; 1 finding from earlier reviews is still open above.

Still open from earlier reviews (1):

  • Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

Still open from earlier reviews (1):

  • Unresolved: 1 blocking on lines changed since (possibly already fixed).

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants