error printer: fix the code frame lines above errors thrown from vm/eval sources

[robobun]

Problem

• The code frame printed above an error thrown from a source bun did not transpile (vm.Script / runInThisContext, eval, new Function, module._compile) is wrong: the line directly above the error is missing and the remaining context lines are numbered one too high. Affects both the uncaught-error output and Bun.inspect(err) / console.error(err).
• Same cause, other shapes: an error on line 2 or 3 gets no context lines at all; a ReferenceError for an identifier that ends its line (JSC positions it on the terminating \n) prints the line above but not the error line or the caret; an identifier at the very end of the source makes the scan read one byte past the source string (ASAN heap-buffer-overflow in populateStackFramePosition, ZigException.cpp:166, reproducible with Malloc=1).
• Cause: populateStackFramePosition in src/jsc/bindings/ZigException.cpp. Fix runtime stack trace computation #11581 changed the line-start scan to stop on the \n that terminates the previous line (sourceString[lineStart] != '\n'), but the block collecting the lines above (previously lines 191-223) still assumed lineStart was the first character of the error line: it stepped back over one more whole line before the collection loop started, so the loop's first line was two lines up while labelled one line up. The loop also stopped at offset 0, so line 1 was never collected. The scan started by reading sourceString[byte_position] itself, which is out of range when the position is the source length, and when that byte is a \n it was taken as the start of the error line, yielding an empty error line.
• Transpiled files are not affected: their frame is rebuilt from the original file in remap_zig_exception (VirtualMachine.rs). That producer has its own, separate line-splitting bugs (no trailing newline: strings: include unterminated final line in error code-frame preview #36683; it also keeps the \r on line 1 of CRLF files, handed off separately); neither is touched here.

Fix

• Rewrite the collection in terms of line ends: scan forward from the position to the error line's \n (or the end of the source), derive each line's start by scanning back from its end (bytes[start - 1]), and for each line above use lineStart - 1, which is by construction the \n terminating it. Line 1 is collected like any other line; the loop ends when a line starts at offset 0.
• This is what removes the out-of-bounds read: the new scan never reads the byte at the position itself, only bytes[i] for i < length going forward and bytes[i - 1] for i > 0 going back. The std::min/std::max on the position is a bounds guard on top of that, so a position outside the provider's text can never index it.
• A position sitting on a \n or at length is treated as belonging to the line it ends, which is the line JSC reports for it (frame.js:2:4 for foo on line 2 in the tests), so the frame and the caret agree.
• Lines are stored without their terminator; a trailing \r is dropped too, so CRLF sources print the same as LF ones (previously every collected line of a CRLF source carried a \r into the output and into Bun.inspect strings). The only consumer that does not trim these strings before use is the inspector's LifecycleReporter.error sourceLines payload, which for these sources now carries bare lines instead of \n-wrapped ones; every printer and the dev error page go through trimmed_text() and are unaffected by the shape change.
• The contract with the Rust side is otherwise unchanged: source_lines[0] is the error line, up to source_lines_to_collect - 1 lines above it follow, numbered the same way, and the provider ref/deref is untouched, so no Rust changes. The 8-bit-only guard is unchanged (non-Latin1 vm/eval sources got no frame before and still do not; separate limitation).
• Verified: test/js/node/vm/vm.test.ts, "code frame of an error thrown from a vm script": context count and numbering, error on line 1 (nothing above it) and on line 2, the five-line window with number padding, blank lines including a blank line 1, CRLF including a blank CRLF line, position on a terminating \n, position at end of source, lineOffset, plus the uncaught printer run with Malloc=1 for the end-of-source case. 9 of the 10 fail on the released binary (the line-1 case is the negative contract and passes on both); all pass with the fix; under ASAN the spawned case additionally crashes without the fix.
• Two existing snapshots in test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap (one via vm, one via eval) had captured the broken output (only line 4 of a script whose lines 1-3 exist); they now show lines 1-4, matching the fixture text and what the transpiled-file path prints for the same source.

Background

• Code frame: the numbered source lines, caret and name: message bun prints above a stack trace. remap_zig_exception fills them from the original file when the top frame has a source map; otherwise it calls ZigException__collectSourceLines, which is the path fixed here. Both write into the same source_lines / source_line_numbers arrays (6 slots, pre-filled with -1) that print_error_instance_body renders from the highest populated index down to the error line at index 0.
• Position / divot: JSC records for each bytecode a character offset into the source provider's text marking the expression. It is not always inside the expression's line: for an identifier it is one past the identifier's last character, which can be the \n ending the line or, for the last token of the source, the source length.
• Source provider: the object holding a script's full source text. The code frame strings are non-owning views into it; the provider is ref'd here and released with the exception, unchanged by this PR.

Before / after for the repro

new (require("node:vm").Script)("'L1';\n'L2';\n'L3';\n'L4';\nthrow new Error('x');", { filename: "ex.js" })
  .runInThisContext({ displayErrors: false });

Before:

2 | 'L1';
3 | 'L2';
4 | 'L3';
5 | throw new Error('x');
          ^
error: x
      at ex.js:5:7

After:

1 | 'L1';
2 | 'L2';
3 | 'L3';
4 | 'L4';
5 | throw new Error('x');
          ^
error: x
      at ex.js:5:7

Identifier ending a line ("'L1';\nfoo\n'L3';"), before:

1 | 'L1';
ReferenceError: foo is not defined
      at ex.js:2:4

After:

1 | 'L1';
2 | foo
       ^
ReferenceError: foo is not defined
      at ex.js:2:4

displayErrors: false only keeps node:vm from replacing err.stack with node's own header (with the default, bun currently prints no frame at all for these errors, which is a different code path); eval("...") of the same text shows the same frames through the same function.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/vm/vm.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 47 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d7e4eab6-24b5-4de2-bc4f-156050344e50

📥 Commits

Reviewing files that changed from the base of the PR and between 42d698e and e80e0b5.

⛔ Files ignored due to path filters (1)

• test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap is excluded by !**/*.snap

📒 Files selected for processing (2)

• src/jsc/bindings/ZigException.cpp
• test/js/node/vm/vm.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 1:05 AM PT - Aug 14th, 2026}

❌ @robobun, your commit e80e0b5 has some failures in Build #95314 (All Failures)

---

🧪   To try this PR locally:

bunx bun-pr 38244

That installs a local version of the PR into your bun-38244 executable, so you can run:

bun-38244 --bun

[robobun]

Status: ready for review; latest push is e80e0b5 (scan unchanged since the first revision, later commits are test additions and review nits).

CI on e80e0b5 (build 95314): 177 jobs passed, including the new tests on the linux ASAN lane and both Windows lanes. The only non-passing job is the macOS 14 arm64 test lane, which expired waiting for an agent; the same job is expiring on nearly every PR build from the last several hours (it passes on main builds), so this is lane capacity, not the change. The macOS builds themselves passed, and the change has no platform-specific code. The remaining entries in the build's failure list are known-flaky files unrelated to error printing (install registry, next-pages, watch mode, cluster, napi) that passed on retry. I have not retriggered, since that would only queue another job into the same backlog; retrying the expired job in Buildkite once the lane recovers is enough.

Reproduced on the released binary (bun 1.4.0-canary) and on main at b5afcac with the script in the PR description: the line directly above the error is missing from the code frame and the remaining lines are numbered one too high. The same input through eval / new Function / module._compile shows the same frame. With Malloc=1 the end-of-source variant ("'L1';\nfoo") is also an ASAN heap-buffer-overflow at ZigException.cpp:166 on main.

With this branch the 10 cases in test/js/node/vm/vm.test.ts pass (9 fail on the released binary; the error-on-line-1 case is the negative contract), test/js/node/vm/ passes, and the two updated vm-sourceUrl snapshots show lines 1-4 of the script instead of line 4 alone.

Related open PRs in the same area, not overlapping: #38338 (CR on line 1 in the transpiled-file producer, found while reviewing this), #36683 (unterminated last line, same producer), #38344 (vm code remapped through a real file's source map; touches other lines of the same files).

[claude]

Beyond the inline nit, I traced the new populateStackFramePosition scan for bounds: divot is clamped to [0, length], the forward scan reads bytes[lineEnd] only while lineEnd < length, and both startOfLineEndingAt and lineText index bytes[x - 1] only while x > 0 with x ≤ length at every call site — no out-of-range access on any path, including divot == length and empty source. The provider ref/deref and non-owning toStringView contract are unchanged. The two updated snapshots now match what the transpiled path already prints for the same source.

Extended reasoning...

The C++ change is a self-contained rewrite of the context-line collection loop; I walked every bytes[...] dereference against the clamped divot and the start > 0 / lineEnd < length guards and found no path that can index outside [0, length). The loop-above invariant (bytes[lineStart - 1] == '\n' whenever lineStart > 0) holds by construction of startOfLineEndingAt, so aboveEnd = lineStart - 1 is always the terminator of the previous line. Leaving this as an informational note only — the change is a non-trivial rewrite of byte-scanning C++ in the error printer, so a human pass is still appropriate.

[robobun]

Pushed two follow-ups to the review feedback so far: 122eb09 drains stdout in the new subprocess test and asserts it is empty, aeb8ecb shortens the two comments in populateStackFramePosition to one line each. The scan itself is unchanged from the first revision; test/js/node/vm/ still passes locally.

[claude]

LGTM — the earlier nit and comment-cop feedback are both addressed.

What was reviewed:

• Traced the rewritten line scan for bounds safety: divot clamped to [0, length], every bytes[...] read is guarded (lineEnd < length, start > 0 before bytes[start-1], end > start before the \r check), so the previous unclamped sourceString[byte_position] OOB is gone.
• Walked the divot-on-\n, divot-at-length, blank-line-1, and line-1-collected cases against the new startOfLineEndingAt invariant; each matches the corresponding new test.
• Confirmed the output contract is unchanged (index 0 = error line, same numbering scheme, source_lines_len untouched as before) so the Rust printer needs no changes; the two updated vm-sourceUrl snapshots had captured the buggy output.

Extended reasoning...

Overview

This PR rewrites the source-line collection block in populateStackFramePosition (src/jsc/bindings/ZigException.cpp), which builds the code frame printed above errors thrown from sources bun did not transpile (vm.Script, eval, new Function, module._compile). The old code had an off-by-one that dropped the line directly above the error and mis-numbered the rest, never collected line 1, produced an empty error line when JSC positioned the divot on a terminating \n, and read sourceString[byte_position] unclamped (an ASAN heap-buffer-overflow when the divot equals the source length). The rewrite clamps the divot, scans forward to the error line's terminator, uses a single startOfLineEndingAt helper to derive each line's start, and strips a trailing \r so CRLF sources render like the transpiled path. Nine new tests in test/js/node/vm/vm.test.ts cover the variant matrix (numbering, error on line 2, five-line window, blank lines including blank line 1, CRLF, divot on \n, divot at end of source, lineOffset, and the uncaught printer under Malloc=1), and two vm-sourceUrl snapshots that had captured the buggy output are updated.

Security risks

None. This is purely diagnostic output — the numbered source lines and caret shown above a stack trace. The change reads from a source provider string that is already ref'd for the exception's lifetime; no new allocation, no user-controlled sizing, and the fix removes a one-byte-past-the-end read rather than adding any surface.

Level of scrutiny

Moderate. It's C++ touching raw byte offsets into a source string, so I traced every index: divot = min(max(byte_position, 0), length) bounds it to [0, length]; the forward scan checks lineEnd < length before bytes[lineEnd]; startOfLineEndingAt checks start > 0 before bytes[start - 1]; lineText checks end > start before bytes[end - 1]; and the lines-above loop only enters when lineStart > 0, at which point bytes[lineStart - 1] is by construction the previous line's \n, so aboveEnd = lineStart - 1 is in bounds. line_zero_based - i cannot go negative in practice because the loop exits when lineStart == 0, which happens no later than after collecting line 1 — the same arithmetic as the old code. The only user-facing effect is a correct code frame; a mistake here would show as a wrong frame, not a runtime behavior change, and the tests pin the exact rendered lines.

Other factors

All prior review threads are resolved: the stdout-drain nit was fixed in 122eb09 (stdout now read and asserted empty), and the two comment-cop notes were addressed in aeb8ecb (comments cut to one line each). The output contract with the Rust printer is unchanged — index 0 is still the error line, up to source_lines_to_collect - 1 lines above, same OrdinalNumber numbering, and source_lines_len is left alone as before (Holder pre-initializes it) — which is why only snapshot content changed on the Rust side, not any code. No CODEOWNERS entry covers these paths.

[robobun]

Self-review pass done; pushed e80e0b5: the line text helper now uses this file's existing StringView_slice, and the test table gained the two boundary cases it was missing (error on line 1 collects nothing above it; a blank line inside a CRLF source prints empty), with the spawned test asserting stdout/frame/exit code as one object. The scan logic is unchanged. The PR description now attributes the out-of-bounds fix to the scan structure (the clamp is a guard on top of it) and notes the two sibling issues in the transpiled-file producer that are intentionally left to their own PRs.

[claude]

I re-reviewed after the follow-up commits and found no issues — the earlier stdout-drain nit and the comment-length feedback are both addressed. Because this is a rewrite of a byte-scanning routine in native C++ (fixing a heap-buffer-overflow in the process), a human look would still be worthwhile before merge.

What was reviewed:

• Traced every array access in the new scan: bytes[start-1] guarded by start > 0, bytes[lineEnd] guarded by lineEnd < length, bytes[end-1] in lineText guarded by end > start; divot clamped to [0, length] — no OOB reads remain.
• Walked the loop invariant for lines-above: lineStart > 0 implies bytes[lineStart-1] == '\n', so aboveEnd is always a valid line terminator; loop terminates at line 1 so line_zero_based - i cannot go below the physical first line.
• Checked the divot-on-'\n' and divot-at-length cases against the new tests; the CRLF strip matches the transpiled-file path's behaviour.
• Confirmed the two updated vm-sourceUrl snapshots now show the correct lines 1-4 rather than certifying the old bug.

Extended reasoning...

Overview

The PR rewrites the source-line collection block inside populateStackFramePosition in src/jsc/bindings/ZigException.cpp (~50 lines of C++ replaced). This is the path that fills the code-frame lines shown above an error thrown from a source Bun did not transpile (vm.Script, eval, new Function, module._compile). The old scan mis-numbered context lines, dropped the line directly above the error, never collected line 1, and read sourceString[byte_position] unclamped — a 1-byte heap-buffer-overflow when JSC positions the divot at the source length. The rewrite reframes the scan around line ends: clamp the divot, scan forward to the error line's terminator, derive its start with a shared startOfLineEndingAt lambda, then walk backward one line at a time using lineStart - 1 as the previous line's terminator. A lineText lambda strips a trailing \r so CRLF sources render like the transpiled-file path. Nine new tests in test/js/node/vm/vm.test.ts cover numbering, line 1/2 errors, the five-line window, blank/CRLF lines, divot-on-\n, divot-at-end-of-source, lineOffset, and an uncaught-error subprocess run under Malloc=1 for ASAN. Two vm-sourceUrl snapshots that had captured the buggy output are updated.

Security risks

None identified. The input is JSC's own source-provider text and bytecode position — not directly user-controlled bytes crossing a trust boundary. The change strictly tightens bounds (clamping the divot, checking start > 0 / lineEnd < length before every index) and removes an out-of-bounds read; it does not add any new allocation, ownership transfer, or JS re-entry. The non-owning StringView slices are backed by the source provider, which is ref'd exactly as before.

Level of scrutiny

Medium-high. This is native C++ in src/jsc/bindings/, the category REVIEW.md flags as most-blocked for memory safety, and it is a logic rewrite rather than a mechanical tweak. The blast radius is every uncaught-error / Bun.inspect(err) render for non-transpiled sources. That said, the function is self-contained, the output contract to the Rust printer is unchanged (source_lines[0] = error line, indices 1..N = lines above, same numbering), and the new scan is materially simpler and easier to reason about than what it replaces. I traced every index expression against its guard and found no reachable OOB, underflow, or off-by-one.

Other factors

Test coverage is thorough and hits the exact edge cases the PR description enumerates, including the ASAN repro under Malloc=1. All prior review feedback on this PR (my stdout-drain nit, the comment-cop length flags) is addressed and resolved in the timeline. The updated snapshots are a legitimate correction of previously-certified buggy output, not a weakening. No CODEOWNERS entry covers these paths. I am deferring rather than approving only because a ~50-line native byte-scanning rewrite that fixes a heap overflow is the kind of change where a maintainer sign-off is appropriate per the approval guidelines, not because I found anything wrong with it.