GitHub Actions annotation and code frame caret: use the first frame that has a file, not a builtin frame on top - #38335
GitHub Actions annotation and code frame caret: use the first frame that has a file, not a builtin frame on top#38335robobun wants to merge 1 commit into
Conversation
…irst frame that has a file print_github_annotation took frames[0] unconditionally, so an error thrown from inside a JS builtin (`[].reduce(...)`) was annotated as `::error file=,line=1,col=11` with the builtin's internal position, and `file=native` / `file=unknown` / `file=node:events` once the top frame was a placeholder or one of bun's own modules. The caret under the code frame used the same frame and so pointed at the builtin's column as well. remap_zig_exception already skipped these frames when choosing which file to show; that list is now ZigStackFrame::has_user_source(), and the annotation and the caret use it to pick the same frame. With no such frame the annotation is emitted without a location.
|
Reproduced on bun 1.4.0 and on main (e697804): |
|
Warning Review limit reached
Next review available in: 14 minutes 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 (4)
Comment |
There was a problem hiding this comment.
LGTM — extracts the existing frame-skip list into ZigStackFrame::has_user_source() and applies it to the two other consumers so all three agree on which frame the error is attributed to.
What was reviewed:
has_user_source()is an exact extraction of the 7-case skip list fromremap_zig_exception; that call site is behavior-identical.- Caret loop now skips empty/
native/unknown/[source:in addition tobun:/node:, aligning it with the frame whose source lines are printed; annotation now.find()s the first user frame instead of unconditionally takingframes[0]. top_frame.is_some()→!frames.is_empty()is equivalent (oldtop_framewasframes.first()), so the annotation body still lists every frame.- Tests cover all four top-frame shapes (empty /
native/node:/unknown) with exact computed line/col, the no-location fallback, and the caret column against the source line.
Extended reasoning...
Overview
Touches error-printing diagnostics only: src/jsc/VirtualMachine.rs (three sites in remap_zig_exception, print_error_instance_body's caret loop, and print_github_annotation) plus a new has_user_source() predicate on ZigStackFrame. Tests added in test/cli/test/bun-test.test.ts (GitHub Actions annotation) and test/js/bun/test/stack.test.ts (caret placement).
Security risks
None. This is formatting of diagnostic output — which frame's file/line/col is printed in the code-frame caret and the ::error file=... GHA workflow command. No new parsing of untrusted input, no allocation, no FFI; has_user_source() is a pure predicate over an already-held &BunString.
Level of scrutiny
Low-to-medium. The change is small and mechanical: it lifts an existing 7-case skip list verbatim into a helper and reuses it at two sites that were using strict subsets of that list, so the three consumers (source-line lookup, caret placement, annotation location) now agree. The only observable effect is on diagnostic text, and the worst-case regression would be a mis-placed caret or annotation — not a runtime behavior change. The remap_zig_exception site is behavior-identical after the extraction; the caret site widens its skip set to match; the annotation site switches from "first frame with a position" to "first frame with a user file and a position", falling back to the same location-less ::error title= it already emitted for a positionless top frame. The annotation deliberately isn't gated on hide_bun_stackframes (an empty file= is wrong regardless of that opt-in), which the PR description calls out.
Other factors
Test coverage is thorough: a test.each matrix over the four source_url shapes (empty builtin, native after .stack read, node:events, unknown after .stack read) each asserting the exact file=,line=,col= computed from the fixture's own source; a no-user-frame case asserting the location-less form still prints the frame list; and two caret-column tests asserting ^ lands on reduce in the printed source line. The PR reports all new tests fail on 1.4.0 and pass with the change, and lists the adjacent suites re-run green. The top_frame.is_some() → !frames.is_empty() rewrite is a no-op given the old top_frame was frames.first(). No prior review comments to address; the PR notes the small overlap with #38328 in the same block and how they compose.
|
Heads-up on the caret half of this: #38349 makes the code frame's caret column a property of the excerpt ( |
|
Updated 1:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 2ae77aa has some failures in 🧪 To try this PR locally: bunx bun-pr 38335That installs a local version of the PR into your bun-38335 --bun |
Problem
GITHUB_ACTIONS=true, an error thrown from inside a JS builtin is annotated with an empty file and the builtin's internal position.[].reduce((a, b) => a)inx.jsprints::error file=,line=1,col=11,title=TypeError: reduce of empty array with no initial value::(release;line=12,col=9on a debug build) instead offile=x.js,line=1,col=4.file=native,line=1,col=11onceerror.stackhas been read (the frames are then parsed back out of the string, where the builtin is printed asnative),file=unknown,line=1,col=1for an error created below a native frame aftererror.stackwas read, andfile=node%3Aevents,line=59,col=31for an error created inside one of bun's own modules (emitter.emit("error")with no listener). None of these name a file in the repository, so GitHub shows the annotation in the run summary only.[].reduce((a, b) => a)the source line shown is the user's, but the^is placed at the builtin's column. Same for thenative/unknowncases;node:frames were already skipped there.print_github_annotation(src/jsc/VirtualMachine.rs) takesframes[0]and only checks that it has a position, and the caret code inprint_error_instance_bodyskips onlybun:/node:frames. The code that picks which file to show in the code frame (remap_zig_exception) already skips empty,native,unknown,[unknown],[source:...],bun:andnode:frames, so the three consumers disagreed about which frame the error is attributed to. Reproduces on 1.4.0 and on main.Fix
remap_zig_exceptionbecomesZigStackFrame::has_user_source();remap_zig_exceptionuses it unchanged, and the annotation and the caret now use it too, so all three point at the same frame: the first one whosesource_urlnames a source the user can open. The annotation additionally requires a position, as before; if no frame qualifies it is emitted as::error title=...without a location, which is what it already did for a positionless top frame.remap_zig_exceptionpicks, so the caret has to be placed by that frame's column or it points at nothing in particular; andfile=is only useful to GitHub for a file that exists in the checkout, which none of the skipped source URLs are. The frames below the skipped ones are already source-mapped (the per-frame loop at the end ofremap_zig_exception), which is why the TypeScript test files below report their original columns. The annotation is not gated onBUN_SHOW_BUN_STACKFRAMESlike the code frame is: that flag is about listing bun's internal frames, and an emptyfile=is wrong with or without it.test/cli/test/bun-test.test.ts, "support for Github Actions": four rows for a builtin /node:module top frame, each fresh and aftererror.stackwas read (covering the empty,native,node:andunknownsource URLs), asserting the exactfile=<test file>,line=,col=of the call in the test file; plus a stack with no file in any frame, asserting::error title=and that the frame list is still printed.test/js/bun/test/stack.test.ts:Bun.inspectof areduceTypeError, fresh and after.stackwas read, asserting the caret is underreduceon the user's line.USE_SYSTEM_BUN=1 bun teston 1.4.0: all 7 new tests fail (file=/file=native/file=node%3Aevents/file=unknown, caret at the builtin's column).bun bd testwithsrc/stashed: the same failures, with the debug build's builtin position (line=12,col=9). With the change:bun-test.test.ts86 pass / 6 todo,stack.test.ts8 pass / 1 todo.test/js/bun/util/inspect.test.js,reportError.test.ts,test/js/node/v8/capture-stack-trace.test.js,test/regression/issue/23022-stack-trace-iterator.test.ts,test/js/bun/sourcemap/,test/js/bun/console/,test/js/node/vm/vm.test.ts,vm-sourceUrl.test.ts,test/js/bun/test/bun_test.test.ts. (inspect-error.test.jshas two minified-file snapshots that already fail on main under a debug build because of the debug-onlyat require (51:24)frame; unchanged by this PR, error printer: keep parsing error.stack past frames without a function name #38308 and error printer: don't invent a 1:1 position for frames parsed out of error.stack without one #38328 fix the helper.)print_github_annotationblock to key the location on the line being valid and printcol=only when the column is; it fixes frames parsed without any position, while this PR is about frames that have a position but no file (reduce (native:1:11)still picks the builtin frame there). The two compose (thefindhere would use its line check); whichever lands second has a few-line rebase in that block. Bun.inspect: label builtin stack frames (native) instead of bare line:col #35177 only changes how the builtin frame is rendered in the list, not which frame the location comes from.Background
::error file=F,line=L,col=C,title=T::bodyon a job's output. The runner attaches it to fileFin the checkout when it exists; otherwise it is listed without a file. Bun prints one per uncaught error / failed test whenGITHUB_ACTIONSis set, fromprint_github_annotation, which runs at the end of the error printer (print_error_instance_body) on the same remappedZigExceptionthe terminal output was printed from.ZigStackFrame.source_urlfor the frames of an error: an absolute path or URL for user code; empty for JSC's JS-implemented builtins (Array.prototype.reduceand friends are JS inside JSC, so unlike C++ natives they have a line:column, into the builtin's own source, but no URL);node:<name>/bun:<name>for bun's built-in modules;native/unknownwhen the frames were rebuilt by parsingerror.stack(that string prints builtins asnativeand positionless native frames asunknown, and the parser gives the latter position 1:1);[unknown]/[source:N]are placeholders from theprepareStackTracecall-site path.name: messageprinted above the stack.remap_zig_exceptionpicks the frame whose file to read (skipping the URLs above), source-maps it and reads the lines;print_error_instance_bodylater prints them and places the caret from a frame it picks itself, which is the second of the two loops unified here.error.stackis first read, then formats the string and drops them. After that, bun's printer rebuilds frames by parsing the string (fromErrorInstanceinZigException.cpp), which is why "aftererror.stackwas read" is a separate shape in the tests.Before / after on the probes
Other top frames, annotation location only, before (1.4.0) -> after; the call is on line 2 of each probe file:
The first row is a CommonJS probe; in an ES module the top-level frame is printed without parentheses and the stack-string parser currently stops there (#38308), so that variant has no frame with a file and gets no location.