Skip to content

JUnit report and GitHub Actions annotation: print a frame source that is not a file path as-is - #39828

Merged
Jarred-Sumner merged 1 commit into
mainfrom
farm/8baaf117/stack-frame-source-url-not-a-path
Aug 21, 2026
Merged

Jarred-Sumner merged 1 commit into
mainfrom
farm/8baaf117/stack-frame-source-url-not-a-path

Conversation

@robobun

@robobun robobun commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • A frame in a long data: URL module aborts the JUnit reporter and the GITHUB_ACTIONS annotation: panic: range end index 6103 out of range for slice of length 4096. A long //# sourceURL= name does the same.
  • Cause: record_failure (src/runtime/cli/test_command.rs:386) and print_github_annotation (src/jsc/VirtualMachine.rs:6621, :6683) call resolve_path::relative on the source URL of every frame. relative writes into fixed path buffers with no length check. A source URL is not always a path.
  • The same call mangles a short URL: webpack://app/./x.ts prints as webpack:/app/x.ts.

Fix

Background

  • The source_url of a frame is what JSC recorded for its code. For a file module it is the absolute path. For a data: module it is the whole URL. For eval'd code it is the //# sourceURL= name.
  • resolve_path::relative(from, to) normalizes both arguments into thread-local PathBuffers and builds the ../ form in a third. It first joins a to that is not absolute onto the cwd.
  • Both reports run from the error printer for every error bun prints, so plain bun test in GitHub Actions hits this too.
Notes
  • Repro on the released 1.4.0 build (Linux): throw.mjs with await import("data:text/javascript," + encodeURIComponent("throw new Error('boom');" + "//".padEnd(6000, "x"))). GITHUB_ACTIONS=true bun throw.mjs prints error: boom, then panics with range end index 6055, exit 134. The same module imported from a test, run with bun test --reporter=junit --reporter-outfile=out.xml, panics with range end index 6103 and writes no report. GITHUB_ACTIONS=true bun test on that test panics too. //# sourceURL=/ followed by 100000 bytes panics with range end index 100000 out of range for slice of length 4095 (the absolute branch of relative).
  • The mangling: relative normalizes the URL (// to /, . segments dropped), joins it onto the cwd and relativizes it against dir. With GITHUB_WORKSPACE outside the cwd it also gets a ../ prefix. test/js/bun/test/err-custom-fixture.js shows the shape on an error object with an http: sourceURL: the released build prints file=http%3A/example.com/test.js, this change prints file=http%3A//example.com/test.js. A remapped frame whose sourcemap sources entry is a webpack:// URL is the same case.
  • The file= property for a non-path top frame keeps the content it has today for a short URL (file=data%3Atext/javascript...). Which frame the annotation points at is GitHub Actions annotation and code frame caret: use the first frame that has a file, not a builtin frame on top #38335. That PR edits the same header block, so one of the two gets a small textual conflict. The frame list of the annotation already printed the raw URL through source_url_formatter. It uses file only for the empty check and, on the dev server, as the prefix to strip.
  • Not covered, on purpose: an absolute path a few bytes under MAX_PATH_BYTES can still overflow inside relative when the ../ chain for dir does not fit next to it. Every caller of relative has that defect (bundler: fix panic relativizing a source path close to MAX_PATH_BYTES #38696 describes it) and paths: bounds-check path normalization and spill thread-local results to the heap #39658 and paths: heap-backed relative_alloc and join_abs_string_buf_spill; use them in _nodeModulePaths and the runtime linker #38392 fix it. It needs a path within about 3 * depth(dir) bytes of the limit. The length check here only turns the unbounded input (a URL or a sourceURL name of any length) away, and becomes redundant once one of those PRs lands.
  • print_error_instance_body runs the JUnit record_failure callback and, under GITHUB_ACTIONS, print_github_annotation for every error bun prints. So bun test in GitHub Actions hits this without --reporter=junit, and so does bun run. The buffers are MAX_PATH_BYTES long: 4096 on Linux, 1024 on macOS, 98302 on Windows.
  • bun test: stop the coverage report from panicking on modules that are not files #39824 is the sibling fix for the coverage report, which makes the same call on coverage entries. It does not touch these sites.
  • Tests pad to 100000 bytes so that the crash cases also exceed the Windows buffer. The JUnit test also pins that a frame in the test file and an absolute sourceURL are still relativized. The GITHUB_WORKSPACE test pins that a test file is reported relative to the workspace and that a webpack:// URL is not. On the released build the tests fail with exit 134 and with the mangled URL.
  • Checked: cargo fmt --all -- --check, prettier on the three test files, test/internal/source-lints, bun bd test on junit.test.js (9 pass), stack.test.ts (7 pass, 1 todo) and bun-test.test.ts (97 pass, 6 todo). Clippy was not run locally.

… report and the GitHub Actions annotation

The JUnit reporter and the GitHub Actions annotation passed every
frame's source URL through resolve_path::relative. That function
normalizes both operands in fixed path buffers. A source URL that is
not a path (a data: URL, a //# sourceURL= name) can be longer than a
path buffer, and then bun aborted with
"panic: range end index N out of range for slice of length 4096"
while it reported the error. A short non-path URL was mangled instead
(webpack://app/./x.ts printed as webpack:/app/x.ts).

ZigStackFrame::relative_source_url makes only an absolute path that
fits a path buffer relative to the report directory. Every other
source URL is printed as-is, as the plain error printer prints it.
@coderabbitai

coderabbitai Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

  • Run review for free

On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file.

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 2 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 80d3a4b4-842c-4e45-806d-93a3cf94052c

📥 Commits

Reviewing files that changed from the base of the PR and between f8d486a and f2ec311.

📒 Files selected for processing (6)
  • src/jsc/VirtualMachine.rs
  • src/jsc/ZigStackFrame.rs
  • src/runtime/cli/test_command.rs
  • test/cli/test/bun-test.test.ts
  • test/js/bun/test/stack.test.ts
  • test/js/junit-reporter/junit.test.js

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

@robobun

robobun commented Aug 20, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:17 PM PT - Aug 20th, 2026

❌ @robobun, your commit f2ec311 has 2 failures in Build #101901 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 39828

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

bun-39828 --bun

@robobun

robobun commented Aug 20, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: merged. A follow-up (#39870) that resolved relative source names (a node:vm filename, a relative //# sourceURL= name) against the cwd again was closed after review: such names are labels, and the rule here (an absolute path is relativized, everything else prints as-is) matches #39824 and the plain stack trace.

How I reproduced the bug, on the released 1.4.0 build (Linux x64):

  1. throw.mjs: await import("data:text/javascript," + encodeURIComponent("throw new Error('boom');" + "//".padEnd(6000, "x")))
  2. GITHUB_ACTIONS=true bun throw.mjs: prints error: boom, then panic: range end index 6055 out of range for slice of length 4096, exit 134.
  3. The same module imported from a test, with bun test --reporter=junit --reporter-outfile=out.xml: range end index 6103, exit 134, no report file. GITHUB_ACTIONS=true bun test on that test panics the same way.

@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 — a small, well-scoped guard that stops resolve_path::relative panicking on non-path frame sources in the JUnit reporter and GitHub Actions annotation.

What was reviewed:

  • relative_source_url gates on is_absolute + MAX_PATH_BYTES; checked against POSIX and Windows is_absolute — data:, webpack://, node:fs all fall through to as-is on both.
  • Return lifetime: relative yields &'static (thread-local buffer) which coerces to &'a; each of the three call sites consumes file before the next relative call in the same loop iteration.
  • Tests cover all three sites (JUnit record_failure, both print_github_annotation paths, and bun run via stack.test.ts) plus the positive case that real absolute paths still relativize.
Extended reasoning...

Overview

Adds ZigStackFrame::relative_source_url (src/jsc/ZigStackFrame.rs) — a 5-line guard around resolve_path::relative that only relativizes when the input is an absolute path shorter than MAX_PATH_BYTES, returning the source URL unchanged otherwise. Three existing call sites in print_github_annotation (src/jsc/VirtualMachine.rs:6621, :6683) and JunitReporter::record_failure (src/runtime/cli/test_command.rs:386) switch to it. Four new tests across three files exercise the crash (long data: URL, long //# sourceURL=), the mangling (webpack:// URL), and the still-works case (absolute path).

Security risks

None. This is diagnostic output formatting for test/CI reports. No parsing of untrusted structured data, no auth, no filesystem writes beyond what the reporters already did. The change is strictly more defensive than before (turns unbounded input away from a fixed-buffer routine).

Level of scrutiny

Low-to-moderate. The fixed change is a panic in report formatting — user-visible but not runtime-semantic. The Rust change is 15 net lines with a single associated function reused at three sites, following the "fix the whole class in the same PR" guideline. relative's &'static return coerces cleanly into the helper's &'a signature; all three call sites consume the borrowed slice within the same loop iteration before any subsequent relative call, so the thread-local buffer isn't clobbered. On Windows, is_absolute correctly rejects data:, webpack://, etc. (checked against is_absolute_windows_t).

Other factors

The PR description is unusually thorough — it names the exact panic, the mechanism, what's intentionally not covered (the near-MAX_PATH_BYTES ../-chain overflow that #39658/#38392/#38696 address in resolve_path itself), and confirms the new tests fail on the released build. Tests use Buffer.alloc per repo convention, drain pipes concurrently, and pin both the crash cases and the positive contract (real file paths still relativized, GITHUB_WORKSPACE still respected). No prior human review comments to address. The one dev-server-only edge (source_url_formatter's root_path prefix-strip when origin is set) is explicitly acknowledged in the description and is not made worse than before.

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