error printer: remap frames when the original source is unavailable, and not twice after error.stack - #38296
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
Status: ready for review. The diff is green on every lane that ran; the remaining red on build 95894 is CI infrastructure, not this change. Build 95894 (head 1893679, rebased onto main): 176 of 179 jobs passed, including all Linux lanes (glibc, musl, ASAN), Windows 2019 x64, and 7 of 8 Windows 11 aarch64 shards. The three non-passing jobs never ran: the two darwin 14 aarch64 shards expired after about five hours in the queue (the The change itself is platform independent (two lines of control flow in Reproduced (release 1.3.14, 1.4.0 canary and a main debug build) with the Verification: Review threads: the two comment-cop threads were addressed by shortening both comments; the code-frame nit (generated excerpt under a mapped frame when no original text exists) is pre-existing behavior shared with virtual modules and is called out in the description rather than changed here. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the two-line control-flow fix is a strict improvement and the remapped skip now matches remap_stack_frame_positions. The inline nit about the caret/gutter coordinate mismatch on the new fallback path is cosmetic and can be a follow-up.
Extended reasoning...
Overview
Two surgical changes to remap_zig_exception in src/jsc/VirtualMachine.rs:
return→break 'code EMPTYwhenfetch_without_on_load_pluginsfails, so the top frame's mapped position is written and the remaining-frames loop runs even when the original source can't be read.- Skip
frames[i].remappedin the per-frame loop, matching whatremap_stack_frame_positionsat line 5411 already does, so frames parsed back out of an already-remappederror.stackstring aren't pushed through the map a second time.
The rest of the diff is test coverage in inspect-error.test.js (two new subprocess tests plus mechanical inline-snapshot shifts from the added import line) and a fix to normalizeError so debug-only builtin frames without a filename are still stripped.
Security risks
None. This is diagnostic output formatting — the change affects what line:column numbers appear in printed stack traces. No parsing of untrusted input, no auth/crypto/permissions surface.
Level of scrutiny
Medium-low. VirtualMachine.rs is core, but the touched function is the error-printer remap path, not execution semantics. Both changes are one-liners whose correctness is locally verifiable: the first reuses the existing empty-code fallback (same as the top_frame_is_builtin and no-preview branches directly above it), and the second mirrors the sibling function's guard. *must_reset_parser_arena_later was not set on the old return path either, so no behavior change there.
Other factors
- Tests are thorough: hand-built VLQ mappings with
sourcesContent: [null]and no source on disk; a transpiled module deleted after import;.stack-read-first for both present and deleted sources;Bun.inspect,err.stack, and uncaught stderr all cross-checked. Follows harness conventions (tempDir,bunEnv/bunExe, concurrent pipe draining,test.concurrent, exit code asserted last). - The one finding is explicitly a cosmetic nit (mapped column drawn against a generated code-frame line on the new fallback) and is a strict improvement over the prior behavior of showing nothing at all; the same shape already exists on the
top_frame_is_builtinbranch. - The comment-cop feedback was addressed in cc77d40 (comments shortened to one line each).
- The
normalizeErrorregex change was checked for over-matching:\(:?\d+:\d+\)$cannot match a frame that has a filename before the position, so only the intended debug-only builtin frames are stripped.
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 1893679 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38296That installs a local version of the PR into your bun-38296 --bun |
…ead, and never twice remap_zig_exception returned early when the source text for the code frame could not be fetched (a .map without sourcesContent whose source is not on disk, or a module deleted after it was loaded). By then the top frame's file name had already been swapped for the mapped one while its line:column were still the generated ones, and the remaining frames were never touched, so Bun.inspect(err) and the uncaught exception output disagreed with err.stack. Fall through with an empty code frame instead. The per-frame loop also re-applied the source map to frames that came out of an already formatted err.stack string (remapped = true), so console.error(err) after any err.stack access printed wrong positions for every frame but the first. Skip those frames, as remap_stack_frame_positions already does. The normalizeError helper in inspect-error.test.js looked for the old " (:" shape of the debug-only builtin frame, which now prints as "at require (51:24)"; match on the missing file instead.
cc77d40 to
1893679
Compare
There was a problem hiding this comment.
LGTM — the two-line runtime change brings remap_zig_exception in line with its siblings (the top_frame_is_builtin arm already does break 'code EMPTY, and remap_stack_frame_positions already skips .remapped frames), and the tests cover the full variant matrix. My earlier nit about the mixed-coordinate code frame stands as a follow-up; it's pre-existing behavior and out of scope here as discussed.
What was reviewed:
- Confirmed
must_reset_parser_arena_lateris correctly not set on the new failure path (nothing was parsed) and no resource is leaked by thebreak—EMPTYis static, and theoriginal_sourcederefs only apply on the success arm. - Verified the
.remappedskip matchesremap_stack_frame_positionsat VirtualMachine.rs:5425 exactly. - Checked the new
normalizeErrorregex is anchored and only strips frames with no file path, so it can't drop legitimate frames. - Test structure:
tempDir/using, concurrent pipe drain, exit-code-last,test.concurrent, hermetic (no network),String(dir)captured before disposal.
Extended reasoning...
Overview
Two-line fix in remap_zig_exception (src/jsc/VirtualMachine.rs): (1) when fetch_without_on_load_plugins fails to read the mapped source for the code-frame preview, break 'code EMPTY instead of returning, so the top frame's position is written and the remaining-frames loop runs; (2) skip frames whose .remapped flag is already set in that loop, so frames parsed back from a materialized error.stack string aren't pushed through the source map a second time. The rest of the diff is test coverage and mechanical snapshot shifts from the added harness import.
Security risks
None. This is error-formatting output only — no parsing of untrusted data, no allocation sizing from external input, no auth/crypto/permissions surface. The failure path now falls through to the same code the top_frame_is_builtin and !enable_source_code_preview arms already exercise.
Level of scrutiny
Low-to-medium. The runtime change is three lines and each line has a direct sibling in the same function or in remap_stack_frame_positions (line 5425) that already does the identical thing, so this is convergence rather than new behavior. The one behavioral question — whether skipping must_reset_parser_arena_later = true on the failure path leaks anything — is answered by the fact that the fetch failed before touching the arena, same as the pre-change return. The empty ZigStringSlice is static, and get_lines_in_text on it returns None, leaving the JSC-collected source lines in place (the pre-existing fallback my earlier nit describes).
Other factors
- Tests are thorough for a fix this size: external-map-with-null-sourcesContent and deleted-transpiled-module cover the first change; present/deleted × inspect/stack-then-inspect/uncaught-after-stack cover the second, all cross-checked against
err.stackas ground truth. They follow harness conventions (tempDirwithusing,bunEnv/bunExe,test.concurrent, concurrent pipe drain, exit code asserted last). - The
normalizeErrorregex tightening is a net safety improvement over the old substring check — it's anchored end-to-end and only matches frames with a bareline:col(no path segment), so it cannot accidentally strip real frames. - All prior review threads are resolved: the two comment-cop notes were addressed by shortening the comments; my earlier cosmetic nit about the caret/gutter coordinate mix is acknowledged as pre-existing (shared with virtual modules) and deliberately deferred, which is called out in the PR body.
- Fixes #15859 with a linked test; the related sourcemap/console/reportError suites were re-run per the PR body and CI is building green.
2 participants
Problem
Bun.inspect(err),console.error(err)and the uncaught exception output print the mapped file name with the generated line:column when the mapped source cannot be read:at thrower (orig.ts:2:27)whereerr.stacksaysat thrower (orig.ts:4:9). The frames below it are not remapped at all (at host.js:5:8)..mapwhosesourcesContententry isnull(or missing) and whose source is not on disk (a deployedbun build --target=bun --sourcemapartifact with the sources stripped), or any module bun transpiled itself that was deleted or moved after it was loaded.remap_zig_exception(src/jsc/VirtualMachine.rs:5704on main)returned whenfetch_without_on_load_pluginsfor the code frame failed. At that point the top frame'ssource_urlhad already been replaced by the mapped name (line 5679) butpositionwas still the generated one (set at 5731, after the return), and the loop remapping the other frames (5778) never ran. Samecatch returnexisted in the Zig version.err.stackhas been read,toZigExceptionrebuilds the frames by parsing that string and marks themremapped(ZigException.cpp:615). The top frame honors that, but the loop at 5778 pushed the other frames through the source map a second time, soerr.stack; console.error(err)printedat outer (m.js:1:39)/at top (m.js:1:39)for frames whose real positions are2:20and3:18. Any error reporter that touches.stackbefore the error is logged or goes uncaught hits this.let x = error.stack; throw errorprintingf1at the wrong line:13:5before,8:5with this change) and the.stack-access half of Unsupported proper logging of AggregateError, Error.cause, modified/accessed Error.stack #1352; the AggregateError and assigned-.stackparts of Unsupported proper logging of AggregateError, Error.cause, modified/accessed Error.stack #1352 are not touched here..stackhas been read, the stack-string parser still stops at the first frame without parentheses (the global-code frame, usually last), so that frame stays missing from the printed output. That is a separate bug inV8StackTraceIterator(ZigException.cpp) and is tracked separately.Fix
breakout of thecodeblock with an empty slice instead of returning. The rest of the function then behaves as for every other case with no original text (same fallback the plugin virtual module case already uses): the generated line is shown as the code frame, the top frame gets the mapped position, and the other frames are remapped. That code frame therefore carries generated line numbers under a mapped frame line; dropping the excerpt or the caret in that situation would be a separate change that also affects the virtual module output, so it is not part of this fix.remappedset in the per-frame loop, which is whatremap_stack_frame_positionsalready does. Without this the first change would also regress the ".stackread first, source missing" case, which today is only correct because the early return skipped the loop.data:/blob:modules,new Function, plugin virtual modules,bun testfailures and--compilebinaries is unchanged (compared against the release build); those all fetch their source fine.test/js/bun/util/inspect-error.test.js,source map remapping of the printed stack. An external map withsourcesContent: [null]and no source on disk checksBun.inspect,err.stackand the uncaught output against fixed expected positions; a second test covers a transpiled module deleted after import, andBun.inspect/ uncaught output aftererr.stackwas read, for a present and a deleted module, all compared againsterr.stack. Both fail on the release build and on main'ssrc/(the deleted-module test also fails with only the first change applied), pass with the fix.normalizeErrorstripped debug-only builtin frames by looking for(:, but that frame now prints asat require (51:24), so the two minified-file tests failed on debug builds; it now matches on the missing file name. The other inline snapshots only moved because of the added import.test/js/bun/sourcemap/,test/js/node/module/sourcemap.test.js,test/regression/issue/23022-stack-trace-iterator.test.ts, the compile sourcemap tests and the console/reportError tests: green.Background
err.stackis formatted in C++ (FormatStackTraceForJS.cpp), which callsremap_stack_frame_positionsfor all frames at once. The error printer (Bun.inspect,console.error, uncaught exceptions) instead builds aZigExceptionand callsremap_zig_exception, which remaps the top frame, fetches its original source to print the code frame, then remaps the remaining frames. The bug is only in the second path, which is whyerr.stackwas right while the printed output was wrong.Lookup::display_source_url_if_neededreturns the original file name when the map is external (a.mapfile or a--compilebinary); for bun's own runtime transpilation the file name does not change, only line:column do. That is why the external case showed a wrong file/position mix and the deleted-module case showed generated positions under the right name.ZigStackFrame.remappedmeans the frame's position is already an original position. It is set by the remap paths and by theerror.stackstring parser, since that string was itself produced by the remapping formatter.ErrorInstanceonceerr.stackhas been materialized, which is why reading.stackchanges which input the printer works from.Repro output, before and after
host.js(// @bunpragma, sidecar map withsourcesContent: [null],orig.tsnot on disk):m.js,.stackread before the error is printed:Fixes #15859
[review] gate passed · iteration 1 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 1
evidence per changed file