Conversation
|
Updated 4:20 AM PT - Aug 23rd, 2026
✅ @robobun, your commit a0c5ca3a78a1168abf046534abb8aa0975629ea6 passed in 🧪 To try this PR locally: bunx bun-pr 30539That installs a local version of the PR into your bun-30539 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParses trailing inline data:application/json ChangesInline sourcemap chaining
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/bundle_v2.zig`:
- Around line 3675-3677: The assignment to
graph.input_files.items(.input_source_map)[result.source.index.get()] =
result.input_source_map leaks the previous ParsedSourceMap and can leak the
freshly parsed map on early failure; before overwriting the graph slot,
deinit/free the existing map (if any) stored in
graph.input_files.items(.input_source_map)[...], then move
result.input_source_map into that slot, and ensure you null out or mark
result.input_source_map as moved; additionally add matching cleanup in the error
path around runResolutionForParseTask() so that if result transitions from
.success to .err the newly allocated ParsedSourceMap and its sourcesContent are
deallocated (use defer or explicit deinit in the same scope where
result.input_source_map is allocated) to avoid leaks.
In `@src/bundler/ParseTask.zig`:
- Around line 1320-1325: The inline source-map parsing currently runs for
plugin/virtual sources too; update the conditional that computes
input_source_map (the if using transpiler.options.source_map,
loader.canHaveSourceMap(), and source.contents) to also require that the input
is file-backed by checking source.path.isFile() (or that source.path.namespace
== "file") so only file-backed inputs attempt
bun.SourceMap.InputSourceMap.parseFromSource; leave the other checks
(transpiler.options.source_map and loader.canHaveSourceMap and non-empty
source.contents) intact.
In `@src/sourcemap/InputSourceMap.zig`:
- Around line 37-145: The parse function currently treats allocator failures
(e.g., at allocator.alloc, allocator.dupe, and similar catch sites such as
source_paths_slice, sources_content_slice, and the dupes inside the loops) as
generic parse failures by using `catch return null`; update each such `catch` to
inspect the error and call `bun.handleOom()` for `error.OutOfMemory` and
otherwise `return null` — e.g. replace `... catch return null` with `... catch
|err| if (err == error.OutOfMemory) bun.handleOom() else return null` at every
allocation/dupe site in InputSourceMap.parse (including the other occurrences
around lines ~203-209) so OOMs trigger Bun’s fatal path while preserving null
for real decode/validation failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3d64f326-1bc4-4bfb-98df-db68570a30e3
📒 Files selected for processing (9)
src/bundler/Graph.zigsrc/bundler/LinkerContext.zigsrc/bundler/ParseTask.zigsrc/bundler/bundle_v2.zigsrc/js_printer/js_printer.zigsrc/sourcemap/Chunk.zigsrc/sourcemap/InputSourceMap.zigsrc/sourcemap/sourcemap.zigtest/bundler/bun-build-api.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/bun-build-api.test.ts`:
- Around line 1297-1299: The test decodes base64 sourcemap payloads using m![1]
without checking that the regex match succeeded; add an explicit guard before
decoding at each occurrence (the variable m in the bun-build-api.test cases) —
e.g., assert expect(m).toBeTruthy() or if (!m) fail with a clear message, then
use m[1] to Base64-decode and JSON.parse; apply the same fix to all four
locations where m![1] is used (the matches at lines matching the
sourceMappingURL extraction).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0d06802d-137b-46ff-afe3-f7d3c764486e
📒 Files selected for processing (1)
test/bundler/bun-build-api.test.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sourcemap/InputSourceMap.zig`:
- Around line 197-212: The current findSourceMappingURL searches the whole file
for "\n//# sourceMappingURL=" which can miss a trailing inline comment or match
markers inside later content; change it to first trim trailing whitespace from
source, compute the start of the final line (use std.mem.lastIndexOfScalar to
find the last '\n' or 0), then only search for the needle inside that final-line
slice (e.g. search source[lineStart..] for "//# sourceMappingURL="). If found,
compute start/end relative to the original buffer and return the trimmed URL as
before; otherwise return null. Ensure you update uses of needle, found, start,
end and keep using bun.strings.trim for trimming.
- Around line 79-81: The parser currently treats the "version" field as
optional; in InputSourceMap.zig change the logic that handles
json.get("version") so that absence returns error.InvalidSourceMap and presence
is still validated (i.e., ensure version.data is .e_number and
version.data.e_number.value == 3.0); locate the block referencing
json.get("version") and replace the optional branch with an explicit existence
check that errors when missing and otherwise performs the same type/value
validation.
In `@test/bundler/bun-build-api.test.ts`:
- Around line 1357-1361: The test currently only asserts an inline source map
exists by checking text for "sourceMappingURL=data:...base64"; update the
assertion to decode and validate the actual fallback source used in the
malformed-map case: extract the base64 payload from the data URL in the variable
text (from Bun.file(result.outputs[0].path).text()), base64-decode and
JSON.parse it, then assert that the parsed source map's sources or
sourcesContent includes the expected intermediate/fallback source (e.g., the
intermediate filename or its source text) so the mapping truly targets the
documented fallback source instead of just existing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 292ecd2b-9daf-426c-9bc5-9fd0149be05d
📒 Files selected for processing (3)
src/bundler/bundle_v2.zigsrc/sourcemap/InputSourceMap.zigtest/bundler/bun-build-api.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/sourcemap/InputSourceMap.zig (1)
214-219:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch trailing inline sourcemaps after code on the final line.
This only accepts a comment-only last line. Valid one-line outputs like
code;//# sourceMappingURL=data:...still returnnull, so chaining is skipped for minified/plugin-generated intermediates.Suggested fix
- const needle = "//# sourceMappingURL="; - if (!bun.strings.hasPrefixComptime(last_line, needle)) return null; - return bun.strings.trim(last_line[needle.len..], " \r\t"); + const needle = "//# sourceMappingURL="; + const found = std.mem.lastIndexOf(u8, last_line, needle) orelse return null; + return bun.strings.trim(last_line[found + needle.len ..], " \r\t");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sourcemap/InputSourceMap.zig` around lines 214 - 219, The current logic in InputSourceMap.zig only accepts a sourceMappingURL if the final line starts with the needle, so cases like "code;//# sourceMappingURL=..." are missed; update the check to search for needle anywhere in last_line (use a substring/index search instead of hasPrefixComptime) and, when found, slice last_line at the found index (needle.len after the index) and then trim that substring before returning; reference the existing variables last_line_start, last_line, and needle to locate and replace the prefix-only hasPrefixComptime logic with an index-based search (e.g., std.mem.indexOf or similar) and handle the not-found case by returning null.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/LinkerContext.zig`:
- Around line 744-755: The code currently only sets path.pretty for file-backed
inputs (in the branch checking path.isFile()) but always serializes path.pretty,
which loses non-file/virtual/plugin module names; update the serialization so
that when path.isFile() is false you use outer_path.text (or set path.pretty =
outer_path.text beforehand) before calling js_printer.quoteForJSON and pushing
to joiner (symbols: outer_path, path, path.isFile(), path.pretty,
js_printer.quoteForJSON, joiner, MutableString.init, leading_comma) so
plugin/virtual module source names are preserved in emitted sources[].
In `@test/bundler/bun-build-api.test.ts`:
- Around line 1250-1253: The test currently grabs the first sourceMappingURL
match from the file (via text.match and m) which can pick up in-body markers;
change the extraction to target only the final inline sourcemap line by either
using a regex anchored to the end of the string (e.g. match the sourceMappingURL
line followed by optional whitespace to string end with the /m flag) or by
finding all matches and using the last one before base64 decoding; update the
occurrences that use Bun.file(...).text(), text.match(...), m and parsed at the
noted spots (including the other listed locations) to use this trailing-line
approach so the parsed JSON always comes from the final inline sourcemap.
---
Duplicate comments:
In `@src/sourcemap/InputSourceMap.zig`:
- Around line 214-219: The current logic in InputSourceMap.zig only accepts a
sourceMappingURL if the final line starts with the needle, so cases like
"code;//# sourceMappingURL=..." are missed; update the check to search for
needle anywhere in last_line (use a substring/index search instead of
hasPrefixComptime) and, when found, slice last_line at the found index
(needle.len after the index) and then trim that substring before returning;
reference the existing variables last_line_start, last_line, and needle to
locate and replace the prefix-only hasPrefixComptime logic with an index-based
search (e.g., std.mem.indexOf or similar) and handle the not-found case by
returning null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7927bb9d-82d1-4fb6-a98a-185ceb0904d3
📒 Files selected for processing (4)
src/bundler/LinkerContext.zigsrc/bundler/ParseTask.zigsrc/sourcemap/InputSourceMap.zigtest/bundler/bun-build-api.test.ts
c4223fc to
03f82da
Compare
06c646d to
528da29
Compare
There was a problem hiding this comment.
No new issues found, but this is a substantial bundler feature (~1k lines across parse/link/print + a new sourcemap parser handling untrusted input) with non-trivial slot-index arithmetic and ownership threading, plus an unrelated build-script change — worth a human pass before merge, and CI is still red on aarch64-musl build-rust.
Extended reasoning...
Overview
This PR threads inline //# sourceMappingURL=data:... input sourcemaps through Bun.build's output map. It adds a new parser module (InputSourceMap.{zig,rs}, ~270 lines each), a new SoA column on Graph::InputFile, ownership transfer in ParseTask/bundle_v2, per-mapping remapping in Chunk::Builder, and per-source slot expansion + chunk-stitching index arithmetic in LinkerContext::generate_source_map_for_chunk. 8 new tests in bun-build-api.test.ts. There's also an unrelated scripts/build/bun.ts change adding an order-only ninja dep to serialize strip against the smoke test.
Security risks
The new parser ingests untrusted bytes (any data: URL embedded in any bundled .js, including plugin onLoad returns). Failure modes are designed to fall back to None (silent no-chain), and earlier review rounds caught and fixed the source-index-aliasing case (out-of-range VLQ indices now hit .fail via the real sources_count). I don't see injection or auth surface, but the slot-expansion arithmetic in LinkerContext (base + chunk.end_state.source_index) and the dev-server gate are exactly the kind of subtle index bookkeeping that benefits from a second pair of eyes.
Level of scrutiny
High. This is production-critical bundler output (sourcemaps affect every debugging/error-reporting workflow), the change spans 19 files in two languages, and it carries several design decisions a maintainer should sign off on: keeping the intermediate as slot 0 alongside inner sources (vs. replacing it), gating the entire feature off for the Bake DevServer path, and deferring sourceRoot / external-.map / plugin-namespace path normalization to follow-ups.
Other factors
The PR went through several automated review rounds; all prior inline findings (dead errdefers, leak on Success→err overwrite, dev-server corruption, ban-words, port-fidelity trim) are marked resolved. The Rust port landed in 30171d1 and the tests are now active (not .todo). However, the most recent CI status (Build #56048, commit 528da29) still shows build-rust failing on aarch64-musl, and the scripts/build/bun.ts change is acknowledged as out-of-scope for the headline feature.
c74b504 to
e21f069
Compare
There was a problem hiding this comment.
No issues found on this pass, but this is a substantial feature touching the bundler's sourcemap stitching across both the Zig reference and the Rust port (new InputSourceMap module, per-chunk source_index offset arithmetic in LinkerContext/Chunk, ownership threading through Graph/ParseTask/bundle_v2, plus an unsafe 'static lifetime erasure in get_source_map_builder) — worth a human pass on the index-stitching math and the Rust lifecycle before merge.
Extended reasoning...
Overview
This PR adds inline-input-sourcemap chaining to Bun.build: a new ~270-line InputSourceMap module (both .zig reference and .rs port) plus threading through Graph/ParseTask/bundle_v2/LinkerContext/Chunk/js_printer so that a trailing //# sourceMappingURL=data:... on an input file expands the output map's sources[]/sourcesContent[] and remaps VLQ mappings through the inner map. It also bundles an unrelated scripts/build/bun.ts ninja order-only-dep fix and 8 new tests in bun-build-api.test.ts.
Security risks
Low. The new parser ingests untrusted inline sourcemap payloads from input files / plugin returns, but malformed input falls through to None/null (silent fallback), OOM goes through handle_oom/bun.outOfMemory, and the out-of-range source_index aliasing concern was fixed by passing the real sources_count to Mapping.parse. No auth, crypto, network, or filesystem-write surface.
Level of scrutiny
High. This is core bundler output-correctness code: the per-chunk source_index expansion and the prev_end_state.source_index = mapping_source_index + chunk.end_state.source_index stitching in generateSourceMapForChunk are subtle — an off-by-one silently corrupts sourcemaps for every downstream user. The Rust port also includes an unsafe { &*(r as *const _) } lifetime erasure to 'static in get_source_map_builder, manual MultiArrayList column draining in deinit_without_freeing_arena, and a Stacked-Borrows-sensitive hoist in ParseTask.rs. The PR went through ~15 inline fixes over the week (dead errdefer, leak paths, DevServer gating, ban-words, E0063, UB), which speaks to its complexity.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved. Test coverage is good (8 targeted cases including malformed/edge inputs). However, several design decisions were explicitly deferred to follow-ups (DevServer slot expansion, sourceRoot, version strictness, external .map resolution, path.pretty for non-file namespaces), and the unrelated build-script change is bundled in. Given the cross-cutting scope and the index-arithmetic subtlety, a human reviewer should sign off on the stitching math and the Rust ownership/lifetime story.
8e56af7 to
f0a63f6
Compare
There was a problem hiding this comment.
No new issues found, but this is a substantial feature touching bundler-core sourcemap stitching (slot-expansion index arithmetic in LinkerContext, per-token remapping in Chunk::Builder) plus an unsafe lifetime transmute in js_printer — worth a human pass before merge.
Extended reasoning...
Overview
This PR threads inline //# sourceMappingURL=data:... comments on bundler input files through to the output sourcemap, so authored sources (.vue/.svelte/.ts) surface in the final map instead of the intermediate .js. It adds a new ~270-line InputSourceMap module (both .rs and reference .zig), plumbs an input_source_map column through Graph::InputFile / ParseTask::Success / bundle_v2, adds remapping logic to Chunk::Builder::add_source_mapping, rewrites the sources[]/sourcesContent[] emission and chunk-stitching arithmetic in LinkerContext::generate_source_map_for_chunk, and adds a new input_source_map field to js_printer::Options with an unsafe transmute to erase its lifetime to 'static. It also carries an unrelated scripts/build/bun.ts fix (order-only ninja dep so the smoke test doesn't race strip). 19 files changed; 8 new tests in bun-build-api.test.ts.
Security risks
None apparent. The new code parses untrusted inline sourcemap JSON/base64 from input files, but malformed input falls back to None (no chain) rather than erroring; out-of-range VLQ source indices are bounded by sources_count so they can't alias neighboring slots; the URL scanner is anchored to the last line so in-body markers can't hijack. No filesystem reads of external .map files (explicitly out of scope). The unsafe transmute extends a borrow lifetime, not raw memory access.
Level of scrutiny
High. This is production bundler-core code that affects every Bun.build with sourcemap != none. The slot-expansion arithmetic (base + chunk.end_state.source_index, 1 + inner.source_index, expansion counts) is delicate — the review history shows it was wrong in multiple subtle ways (DevServer corruption, out-of-range aliasing, hint-coordinate-space mismatch) before being fixed. The unsafe lifetime erasure in js_printer/lib.rs:7995-8013 deserves a human eye on whether the SAFETY comment's invariant (graph slot outlives every add_source_mapping call) actually holds across all printer entry points. The DevServer gate (self.dev_server.is_none()) is a deliberate feature scope-down that a maintainer should sign off on.
Other factors
The PR went through ~10 rounds of bug fixes during review (dead-errdefer leaks, stacked-borrows UB, E0063 build break, ban-words violation, slot-overwrite leaks, DevServer corruption gate, hint-poisoning perf regression) — all now resolved, which speaks well of the final state but also confirms this is not mechanical code. All prior inline comments (mine and CodeRabbit's) are resolved. The unrelated scripts/build/bun.ts change is small and well-commented but bundling it here is a scope question for the maintainer. Test coverage is good (8 targeted cases including malformed/edge inputs).
|
CI on |
|
CI status after the rebase (head 8020783, build #61103): every test lane fails on exactly one test, |
3e0f8bb to
8020783
Compare
…ows UB topts is a shared borrow through a raw pointer into *transpiler. get_ast reborrows the same location mutably (also through the raw pointer), which pops topts's SharedReadOnly tag under Stacked Borrows — any subsequent topts.source_map read becomes Miri-detectable UB. Copy source_map out alongside module_type, before the `let _ = topts;` tombstone the prior author left for exactly this invariant. Flagged by claude[bot].
Main's e_string().string() takes &self now; the `mut` bindings at InputSourceMap.rs:116/126 became unused, which -D unused-mut turns into a hard error on the release build-rust lanes. mappings_e_string keeps its mut (slice() still takes &mut self).
When input_source_map chaining is active, prev_state.original_line holds the remapped *authored* line — wrong coordinate space for the intermediate file's line-offset table, so the O(1) find_line_with_hint fast path missed on every token and fell to binary search. Track the un-remapped intermediate line in a dedicated prev_intermediate_line field and hint from that. No behavior change (binary search was always the sound fallback); restores the per-token fast path on the chained feature path. Chunk.zig uses plain findLine (no hint), so it's unaffected. Flagged by claude[bot].
- InputSourceMap: reject maps whose sources[i] exceeds MAX_PATH_BYTES so an adversarial inline map can't panic the fixed-size path buffers; the map falls back cleanly instead. - write_sources_for: resolve inner names via join_abs_string_buf_checked and emit the raw name when the join overflows, rather than the panicking unchecked join. - Move the inline-sourcemap chain suite to its own test file and add an oversized-name regression test.
Windows MAX_PATH_BYTES is ~96 KB, so a 5 KB name was under the cap there and surfaced instead of being rejected. Use a 128 KB name so the map is rejected on every platform.
…es through The DevServer stitcher never consumes the parsed map, so the per-reparse scan was dead work on the HMR path. URL-schemed inner sources[] entries (webpack:///...) must not be path-joined; emit them verbatim per spec.
A plugin onResolve custom-namespace path has no on-disk directory, so joining inner names against dirname(text) produced bogus labels.
2d79d79 to
1e8554f
Compare
| let content = quoted_source_map_contents[index as usize] | ||
| .as_deref() | ||
| .unwrap_or(b""), | ||
| ); | ||
| .unwrap_or(b"null"); | ||
| j.push_static(if content.is_empty() { b"null" } else { content }); |
There was a problem hiding this comment.
🟡 nit: Slot 0's sourcesContent (quoted_source_map_contents[index]) is quoted from raw source.contents, which still carries the trailing //# sourceMappingURL=data:...;base64,<payload> comment — the same bytes now also emitted decoded into slots 1..N, so the inner map appears twice in the output. Since find_source_mapping_url already knows the comment's byte offset, consider quoting only source.contents[..comment_start] for slot 0 (or emitting null) when input_source_map.is_some(). Size-only on the new-feature path, not blocking.
Extended reasoning...
What the observation is
When chaining is active for an input file, LinkerContext.rs:1116-1119 emits quoted_source_map_contents[index] as slot 0 of the output map's sourcesContent[]. That value is populated by compute_quoted_source_contents (LinkerContext.rs:1627), which reads let contents: &[u8] = &source.contents; — the raw input file bytes, including the trailing //# sourceMappingURL=data:application/json;base64,<payload> comment. The base64 payload is exactly the JSON whose sourcesContent[] array this PR now decodes and re-emits into slots 1..N (LinkerContext.rs:1123-1134). So the inner map's bytes appear twice in the output map: once base64-encoded inside slot 0's string literal, and once decoded across slots 1..N.
The specific code path
ParseTask.rs:2698-2706finds a trailing inline map onsource.contentsand stores anInputSourceMapon the input file.compute_quoted_source_contents(LinkerContext.rs:1594-1632) later JSON-quotes the fullsource.contents— it has no knowledge ofinput_source_map, so it does not truncate at the comment.- In the
sourcesContentemission loop, slot 0 pushes that quoted string verbatim (LinkerContext.rs:1116-1119), then slots 1..N iterateism.sources_contentand push each decoded inner content (LinkerContext.rs:1123-1134).
Why existing code doesn't prevent it
Nothing in the diff strips the trailing comment from slot 0. The PR description says "sourcesContent[] holds the clean authored bytes without the trailing comment" — that's true for the authored slot (slot 1+), and the tests only assert on that slot (parsed.sourcesContent[authoredIdx]). No test inspects slot 0 (the intermediate). find_source_mapping_url in InputSourceMap.rs computes the byte offset of the trailing comment during parse but doesn't expose it, and compute_quoted_source_contents runs independently against raw source.contents.
Step-by-step proof
Take a .vue-style intermediate: 2 KB of transpiled JS + a 100 KB inline base64 comment (typical — the inner map's sourcesContent dominates and base64 inflates by 4/3).
- Pre-PR: output
sourcesContent[0]= the ~102 KB intermediate string. Total for this file: ~102 KB. - Post-PR, slot 0: same ~102 KB string (unchanged —
compute_quoted_source_contentsstill reads rawsource.contents). - Post-PR, slots 1..N: the decoded
sourcesContent[]from the inner map — roughly the ~75 KB that base64-decoded from the 100 KB payload'ssourcesContentfield. - Total: ~177 KB. The chained output map is larger than the pre-PR unchained one, and ~98% of slot 0 is redundant with slots 1..N.
esbuild's equivalent (printer.go addSourceMapping) does not emit the intermediate as a source at all when an input map is present — it emits only the inner sources — so it doesn't carry this duplication.
Impact and severity
Size-only, on the new-feature path (input_source_map.is_some()). Correctness is unaffected: slot 0 is a valid string, mappings that fall back to it still resolve, DevTools just shows the raw intermediate with its trailing base64 comment. Nothing that worked before regresses. So this is a nit, not blocking.
Suggested fix
find_source_mapping_url already computes last_line_start — the byte offset of the trailing comment. Store that on InputSourceMap (e.g. comment_start: usize) and, in compute_quoted_source_contents or at the slot-0 emission site, quote only source.contents[..comment_start] when input_source_map.is_some(). Alternatively, emit null for slot 0 (matching esbuild's behavior of not surfacing the intermediate at all), since the intermediate's contents are rarely useful once the chain resolves through to the authored source.
The inner map otherwise ships twice: base64 inside the intermediate's content plus decoded in the inner slots. find_source_mapping_url already knows the comment's byte offset; store it on InputSourceMap and quote only the bytes before it for the chained file's own slot.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // For a chained file, strip the trailing comment from | ||
| // slot 0: the inner map would otherwise ship twice | ||
| // (base64 here + decoded in slots 1..N). |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Byte offset of the trailing `//# sourceMappingURL=` line in the | ||
| /// original source (`0` when parsed from raw JSON). The linker strips | ||
| /// the comment from the emitted slot-0 `sourcesContent` so the inner | ||
| /// map isn't shipped twice. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| let file_contents: &[u8] = &sources[index as usize].contents; | ||
| match input_source_maps.and_then(|m| m[index as usize].as_deref()) { | ||
| Some(ism) | ||
| if ism.comment_start > 0 | ||
| && ism.comment_start <= file_contents.len() => | ||
| { | ||
| let truncated = &file_contents[..ism.comment_start]; | ||
| let mut quote_buf = MutableString::init(truncated.len() + 2)?; | ||
| js_printer::quote_for_json(truncated, &mut quote_buf, false)?; |
There was a problem hiding this comment.
🟡 The guard ism.comment_start > 0 conflates byte offset 0 with "unset": when the file has no newline before its final line (e.g. a plugin onLoad returns only //# sourceMappingURL=data:...), find_source_mapping_url returns comment_start = 0, so this falls through to the _ arm and ships the full base64 comment as slot-0 sourcesContent — the double-shipping this block exists to prevent. Since the bundler only populates input_source_map via parse_from_source (never raw parse), comment_start is always a real offset here; drop the > 0 check (keep <= file_contents.len()) or make the sentinel Option<usize>/usize::MAX.
Extended reasoning...
What the bug is
The match guard at LinkerContext.rs:1122 tests ism.comment_start > 0 && ism.comment_start <= file_contents.len() to decide whether to emit slot-0 sourcesContent truncated at the inline-map comment (the intent of commit 1daf82f, "strip the inline-map comment from slot-0 sourcesContent"). The > 0 half is meant to distinguish "set by parse_from_source" from "unset by raw parse()" (InputSourceMap.rs:150 initializes comment_start: 0 in parse_internal, and the field's doc comment says "0 when parsed from raw JSON"). But 0 is also a valid byte offset returned by find_source_mapping_url — so the guard gates on truthiness where it should gate on presence.
The specific code path that triggers it
In find_source_mapping_url (InputSourceMap.rs:174-184):
let last_line_start = match bun_core::strings::last_index_of_char(body, b'\n') {
Some(i) => i + 1,
None => 0,
};
...
let comment_start = last_line_start;When the whitespace-trimmed body contains no \n, last_line_start = 0, so comment_start = 0. This is a legitimate byte offset meaning "the //# sourceMappingURL= comment starts at byte 0 of the file". A plugin onLoad returning { contents: "//# sourceMappingURL=data:application/json;base64,<...>", loader: "js" } (no leading code, no leading newline) — pathological but valid JS and reachable via the #6173 plugin path this PR targets — produces exactly this.
Why existing code doesn't prevent it
The only producer of input_source_map in the bundler graph is InputSourceMap::parse_from_source (ParseTask.rs:2700); ServerComponentParseTask.rs:205 sets None. Raw InputSourceMap::parse (which sets comment_start = 0 as a genuine "unset" sentinel) is never reached from the bundler, so every ism reaching this match arm has comment_start set to a real byte offset. The > 0 check therefore has no legitimate "unset" case to reject here — it only creates a false negative for the comment_start == 0 case.
None of the 11 tests in bun-build-inline-sourcemap-chain.test.ts exercise a single-line-comment-only intermediate; they all prefix the comment with at least one line of code plus a \n.
Step-by-step proof
Take a plugin onLoad returning contents = "//# sourceMappingURL=data:application/json;base64,eyJ2ZXJz..." (no leading newline, no trailing newline):
- ParseTask.rs:2700 —
InputSourceMap::parse_from_source(&source.contents)is called (gates pass:!has_dev_server,source_map != None,loader.can_have_source_map(),!contents.is_empty()). - InputSourceMap.rs:160-168 — trailing-whitespace trim: no trailing whitespace, so
body = source. - InputSourceMap.rs:174 —
last_index_of_char(body, b'\n')→None(no newline in body) →last_line_start = 0. - InputSourceMap.rs:178-184 —
last_line = &body[0..]starts with//# sourceMappingURL=✓ →comment_start = 0. The data URL parses successfully →map.comment_start = 0. - bundle_v2.rs:7142 — moved into
graph.input_files.items_input_source_map_mut()[idx]. - LinkerContext.rs:1120-1123 —
input_source_maps.and_then(...)→Some(ism)withism.comment_start == 0. Guard evaluates0 > 0→ false. - LinkerContext.rs:1130-1135 — falls through to the
_arm: emitsquoted_source_map_contents[index], which is the JSON-quoted full file contents — i.e. the entire//# sourceMappingURL=data:...;base64,<payload>string. - LinkerContext.rs:1140-1152 — slots 1..N then emit the decoded inner
sourcesContent[i].
Result: the inner map ships twice in the output sourcesContent[] — once base64-encoded in slot 0, once decoded in slots 1..N — exactly the bloat the comment_start truncation exists to prevent. Mappings and source resolution are unaffected (they don't read sourcesContent), so correctness is fine; only output size regresses on this input shape.
How to fix it
This is REVIEW.md's "Treat empty, zero, and unset as three distinct states — gate on presence, not truthiness" rule verbatim. Two options:
- Minimal: drop the
> 0half of the guard, keeping onlyism.comment_start <= file_contents.len(). Since everyismreaching this code came fromparse_from_source,comment_startis always a valid slice index (including0), and&file_contents[..0]correctly yieldsb""→ slot 0 emits"". - Structural: change
InputSourceMap.comment_starttoOption<usize>(or useusize::MAXas the sentinel inparse_internal) so "unset" and "offset 0" are representationally distinct — then the guard becomesif let Some(start) = ism.comment_startand the ambiguity disappears at the type level.
Nit-level because the trigger requires a file whose entire non-whitespace content is the sourceMappingURL comment (only a plugin onLoad returning nothing but the comment would produce it), and the impact is output-size bloat only.
A module whose whole contents are the sourceMappingURL line has the comment at byte 0; the > 0 sentinel check skipped truncation for exactly that input and shipped the inline map twice.
| /// Byte offset of the trailing `//# sourceMappingURL=` line in the | ||
| /// original source (`None` when parsed from raw JSON). The linker | ||
| /// strips the comment from the emitted slot-0 `sourcesContent` so the | ||
| /// inner map isn't shipped twice. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
There was a problem hiding this comment.
I reviewed this PR and the bug-hunting pass found no new issues; all prior findings from earlier rounds are resolved. Given the size (~1000 lines across 10 files) and the cross-file slot-layout invariant this introduces between Chunk::Builder and LinkerContext, a human look would still be worthwhile before merge.
What was reviewed
- Slot-layout contract:
Chunk::Builderemitssource_index ∈ {0, 1+inner_idx}andLinkerContextreserves1 + external_source_names.lenslots per file; verified these agree and thatprev_end_state.source_index = base + chunk.end_state.source_indexis a no-op wheninput_source_mapisNone(chunk end-state stays 0). - The refactored
sources[]/sourcesContent[]emission loops preserve pre-PR behavior when no inline map is present (leading-comma viachunk_i > 0is correct because slot 0 is always unique; theunwrap_or(b"null")change only tightens a defensive fallback). InputSourceMap::parse_from_source: last-line anchoring, adversarial length caps, out-of-range VLQ rejection, and DevServer gating at both producer and consumer sites — all covered by the 11 new tests.- Ownership: the new
Option<Box<InputSourceMap>>SoA column is drained explicitly indeinit_without_freeing_arenaand overwritten (not leaked) on incremental reparse.
Extended reasoning...
Overview
This PR threads inline //# sourceMappingURL=data:... sourcemaps carried by bundler input files through to the final output map, so authored sources (e.g. .vue/.svelte originals) surface in sources[]/sourcesContent[] instead of stopping at the intermediate .js. It touches 10 files: a new ~240-line parser (InputSourceMap.rs), the per-token remap in Chunk::Builder::add_source_mapping, a substantial refactor of LinkerContext's sources/sourcesContent/mapping-stitching emission, a new InputFile SoA column with explicit drain, plumbing through ParseTask/Options, and an 11-case test file. Net +996/-71.
Security risks
Low. The new parser ingests untrusted bytes (inline map payloads from arbitrary input files or plugin onLoad returns), but it validates before allocating: base64 decode length is bounded by the payload, JSON parse failures return None, sources[i] longer than MAX_PATH_BYTES rejects the whole map, out-of-range VLQ source_index is rejected via the real sources_count bound, and path resolution uses the checked join_abs_string_buf_checked with a verbatim fallback. All failure paths degrade to pre-PR behavior (build succeeds, no chain). No filesystem reads for external .map references.
Level of scrutiny
High. This is core bundler output-generation code, and the correctness of every Bun.build sourcemap now depends on a cross-file invariant: Chunk::Builder emits chunk-relative source_index values that LinkerContext must rebase and whose slot count must exactly match the sources[]/sourcesContent[] expansion. The prev_end_state.source_index stitching change and the sourcesContent loop rewrite both touch the non-chained path too — I verified they reduce to the old behavior when input_source_map is None, but a maintainer familiar with the linker's stitching semantics should confirm.
Other factors
The PR has been through 32 iterations and multiple review rounds; every prior finding (dead errdefer, DevServer stitcher corruption, Stacked Borrows tombstone violation, find_line_with_hint coordinate-space bug, source-lint violations, URL-schemed/virtual-namespace source names, wasted DevServer scan, comment_start == 0 edge case) has a corresponding fix commit and test, and all threads are resolved. Two nits (percent-decoding the non-base64 branch, and a monotonic cursor for find_mapping) were explicitly deferred to a coordinated follow-up alongside the pre-existing sourceRoot gap. CI on the last full build (#97866) passed 176 jobs with only darwin agent-expiry infrastructure noise; the latest push is currently building. Given the scope and the number of subtle invariants that surfaced during iteration, this warrants a human sign-off rather than bot-only approval.
|
Update on the relationship with #32473 (external The earlier note above said that #30539 can be closed if #32473 lands. That is no longer the plan. #32473 has had no push since 2026-06-27, conflicts with main in 10 files, and forked from this branch 13 commits ago. This PR is mergeable and green on build #104123. Direction: this PR lands first. #32473 will be rebased on top of it and carry only the external |
The comment linter main gained flags every multi-line justification in this diff (CLAUDE.md 13/14). Most were inherited from #30539's style. Each is cut to what a reader could not infer from the code. Also route two byte searches in InputSourceMap.rs through bun_core::strings (last_index_of_char, index_of_any) per the byte-search source lint.
This branch has not been deployed
1 participant
Closes #30536.
Also fixes #6173 — plugin
onLoadreturns with an inline sourcemap are fed through the same scanner, so the plugin case rides on the same pipeline.Summary
When
Bun.build({ sourcemap: 'inline' })bundles an input.jsfile that carries an inline//# sourceMappingURL=data:application/json;base64,…comment (e.g..vue/.sveltecompilers emitting an intermediate.jswith a chain back to the authored.ts/.vue), the final output's sourcemap chain now resolves through to the authored source. Before this change the deepestsources[]entry was the intermediate.js; after, it's the authored source, andsourcesContent[]holds the clean authored bytes without the trailing comment. Same fix coversonLoadplugin returns (#6173) because the scanner runs onsource.contentsregardless of where they came from.Repro
Before:
sources = ["../inner.js", "../entry.ts"].sourcesContent[0]is the rawinner.jsbytes including the literal//# sourceMappingURL=…comment. Chain stops at the intermediate.After:
sources = ["../inner.js", "../inner.ts", "../entry.ts"].sourcesContent[1]is the cleaninner.ts, no embedded comment. Mappings that live in the intermediate resolve through to the authored line/column.Cause
The lexer already detects
//# sourceMappingURL=but nothing reads it back — the bundler's linker emits the output map directly fromsource.contents+ the intermediate's path, discarding any chain.Fix
Thread the inline map end-to-end:
src/sourcemap/InputSourceMap.rs(new, ~220 lines): owns the parsed innerParsedSourceMap+ per-source contents.parse_from_sourcescans for a trailing//# sourceMappingURL=data:...anchored on the last line (Source Map spec — in-body markers in template literals must not hijack the lookup). Supports both;base64,and rawdata:application/json,...payloads. Malformed input →None(silent fallback); OOM propagates viahandle_oom.src/bundler/ParseTask.rs: after parsing JS, scan the source bytes. Gated onsource_map != .None && loader.can_have_source_map() && !source.contents.is_empty()so no-sourcemap builds pay nothing; external.mapreferences are out of scope. The scan runs onsource.contentsregardless of where the contents came from (file read or pluginonLoadreturn), so this also covers the plugin case from Support sourcemaps inonLoadplugins #6173.src/bundler/Graph.rs,bundle_v2.rs: newInputFile.input_source_map: Option<Box<InputSourceMap>>column; ownership moves from the parse result onto the file and drains indeinit_without_freeing_arena(MultiArrayList'sDropis slab-only).src/sourcemap/Chunk.rs+src/js_printer/lib.rs:Builder+Optionsgrow an optionalinput_source_map. Inadd_source_mapping, the(line, col)we'd have emitted against the intermediate is translated viamap.find_mapping:source_index = 1 + inner.source_index, inner(line, col)src/bundler/LinkerContext.rs: each outer source insources[]expands to[intermediate, inner_0 … inner_N-1]andsourcesContent[]matches slot-for-slot. Chunk stitching usesbase + chunk.end_state.source_indexas the absolute end so per-chunk mappings whosesource_indexvaries across their length compose correctly. Gated onself.dev_server.is_none()because Bake'sSourceMapStore::join_vlqstitcher hard-codes onesources[]slot per file and would corrupt on expansion.Malformed or unrecognized inline maps fall back cleanly — the build still succeeds with the pre-fix behavior (tested).
Verification
bun bd test test/bundler/bun-build-api.test.ts→ 53 pass / 0 fail (8 new + 45 existing). 8 new cases underdescribe("Bun.build chains inline input sourcemaps", …):data:URL — authored source surfaces in bundled mapdata:application/json,…URL — authored source surfaces.vue?script+?template) — all surfacesource_index >= sources.len— rejected, no slot aliasing//# sourceMappingURL=marker inside a template literal (but no trailing comment) — ignored, no hijack.mapfilename reference (non-data:) — unchanged behavioronLoadreturn carrying inline map — regression guard for Support sourcemaps inonLoadplugins #6173Also ran the full
bun-build-api.test.tsand adjacent bundler suites — no regressions.Build script fix
scripts/build/bun.tsgets a small fix unrelated to the bundler but triggered by this PR's larger release-mode build: addedstrippedExeas anorderOnlyInputsninja dep for the release smoke-test rule, so the test doesn't race thestrip bunstep. Without this, the gate hit an intermittentPermission deniedonbuild/release/bunduring the smoke test.Rebase notes (latest)
Rebased across ~1250 commits of main. Three substantive adaptations:
emitPostLinkcentralizes the strip-before-smoke-test invariant, crediting bundler: chain inline input sourcemaps through to output #30539), so that commit was dropped and the PR no longer touchesscripts/build/bun.ts.unsafelifetime erasure injs_printeris gone: main parameterizedChunk::Builderover a lifetime, soinput_source_mapis now a plainOption<&'a InputSourceMap>borrow with no transmute.EObjectJSON/EArrayJSONrows) and mademapping::parsereturn aResult;InputSourceMap::parse_internalnow readsversion/mappings/sources/sourcesContentthrough the tape accessors (ObjectJSON::get,JsonValue::as_str/as_array). Without this the container lookups silently failed and no chaining occurred.Verified after the rebase: all 9 chain tests pass, and the 9 sourcemap tests in
bun-build-api.test.ts(including main's new C0-control-chars and non-ASCII-columns cases) pass.[review] gate passed · iteration 32 · 10 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 32
evidence per changed file
root cause · written by the author bot
The root cause was that the bundler read input files as raw bytes and never consumed an inline //# sourceMappingURL= comment, so the output map's sources chain stopped at the intermediate file and the comment itself leaked into sourcesContent as literal text. The fix parses the trailing inline data URL into an owned InputSourceMap stored on each input file, threads it through the printer and sourcemap chunk builder so mappings are remapped through the inner map's original coordinates, and has the linker expand the emitted sources and sourcesContent to include the authored inner sources. The…