Snapshot tests for trace parsing/symbolization + Sentry view_url#21
Merged
dylan-conway merged 5 commits intomainfrom Apr 11, 2026
Merged
Snapshot tests for trace parsing/symbolization + Sentry view_url#21dylan-conway merged 5 commits intomainfrom
dylan-conway merged 5 commits intomainfrom
Conversation
…in Sentry
- Extract the post-symbolizer pipeline (address adjustment, function-name
cleaning, path normalization, frame filtering) from remapUncached into
backend/symbolize.ts so it can run without spawning a process or fetching
debug files. remap.ts re-exports the helpers for backward compat.
- Fixture-driven snapshot tests:
test/parse.test.ts URL string -> Parse (24 fixtures, all 9
platforms x v1/v2, plus js/unknown/foreign
frames, compressed panic, oom, error, url prefix)
test/symbolize.test.ts recorded symbolizer stdout -> Address[]
(6 fixtures: macOS offset, Windows backslash
paths, jscSignalHandler/panic/assert stripping,
interleaved js/foreign frames, all-?? revert)
Snapshots lock current main behavior; intentional decoder changes show up
as reviewable diffs and are accepted with --update-snapshots.
- scripts/capture-fixture.ts: turn a real bun.report URL into a parse
fixture (and, with --symbolize, a symbolize fixture with real recorded
symbolizer stdout).
- scripts/seed-parse-fixtures.ts + test/helpers/encode.ts: regenerate the
synthetic platform-matrix fixtures.
- .github/workflows/test.yml: run tsc --noEmit + bun test on push/PR.
- Sentry: thread the raw trace string through sendToSentry and attach
https://bun.report/<trace>/view as tags.view_url (when <=200 chars) and
always as extra.view_url, so crashes link back to the report site.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughAdds snapshot-driven parse/symbolize tests and many fixtures; extracts symbolization post-processing into a new module; threads the original trace string into Sentry payloads; removes Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/remap.ts (1)
115-118:⚠️ Potential issue | 🔴 CriticalCritical: Error created but not thrown.
The error is constructed but never thrown when the symbolizer subprocess fails. This silently ignores symbolizer failures and proceeds with an empty
stdout, causing incorrect or missing symbolization.🐛 Proposed fix
if ((await subproc.exited) !== 0) { const e: any = new Error("pdb-addr2line failed: " + (await Bun.readableStreamToText(subproc.stderr))); e.code = "PdbAddr2LineFailed"; + throw e; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/remap.ts` around lines 115 - 118, The code constructs an Error when the symbolizer subprocess (subproc) exits with a non-zero code but never throws it, so modify the error-handling branch in remap.ts (the block that checks (await subproc.exited) !== 0) to throw the constructed error after setting e.code; ensure you still await Bun.readableStreamToText(subproc.stderr) to include stderr text in the message and then throw the Error (keeping the error code "PdbAddr2LineFailed") instead of silently continuing, so symbolization failures are propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yml:
- Around line 18-20: Update the GitHub Actions step that uses
oven-sh/setup-bun@v2 to stop using bun-version: latest and instead pin a stable
version or range (for example bun-version: 1.2.x or a specific patch like 1.2.3)
to ensure reproducible CI; locate the step with uses: oven-sh/setup-bun@v2 and
change the bun-version input accordingly so CI does not automatically float to
new, potentially breaking Bun releases.
In `@backend/sentry.ts`:
- Around line 10-12: In remapToPayload, the constructed view_url uses trace_str
raw; update the view_url creation (the view_url variable inside remapToPayload)
to pass trace_str through encodeURI() so the path segment is properly escaped
(e.g., interpolate encodeURI(trace_str) into the URL) to match other
constructors and avoid unsafe characters.
In `@backend/symbolize.ts`:
- Around line 72-99: filterAddresses mutates the input array via shift/pop;
clone the input at entry (e.g., const work = addrs.slice()) and perform all
removals on that cloned array (use index-based slicing or mutating methods on
the clone) while keeping the original addrs untouched; ensure the final
emptiness check and fallback returns the original input (addrs) if all entries
are removed, and return the filtered clone instead of the original variable.
- Around line 150-153: The current extraction for line uses
Math.floor(Number(...)) which allows non-integer or non-finite numeric forms;
change to strict integer validation: read the substring (using
str.slice(second_colon + 1, last_colon)) into a raw string, ensure it matches an
integer pattern (e.g., /^\-?\d+$/ for signed integers or /^\d+$/ if only
positives), convert to a Number and validate Number.isFinite and
Number.isInteger before accepting it; if any check fails return null. Apply this
to the code around the variables line, str, second_colon and last_colon so only
finite integers are accepted as line numbers.
In `@scripts/capture-fixture.ts`:
- Line 76: The Bun.spawn call that creates const subproc in
scripts/capture-fixture.ts is missing a timeout, so the symbolizer can hang
indefinitely; update the Bun.spawn invocation for subproc to include a timeout
option (e.g., timeout: 5000) so Bun will auto-terminate hung processes,
mirroring the behavior used in backend/remap.ts; ensure the timeout value is
passed in the same options object where cmd and stdio are set.
In `@test/helpers/encode.ts`:
- Around line 31-56: Add missing parser reason kinds to the ReasonSpec union and
handle them in encodeReason: extend ReasonSpec with { kind:
"illegal_instruction" }, { kind: "bus_error" }, { kind: "float_exception" }, and
{ kind: "unaligned" }, and add corresponding switch branches in encodeReason
that return the correct single-digit prefix for each (e.g., "3" for
illegal_instruction, "4" for bus_error, "5" for float_exception, "6" for
unaligned) so the helper can emit the same reason-code prefixes the parser
expects; update any fixtures/tests that rely on these encoded reasons to use the
new kinds.
- Around line 70-78: In buildTraceString, ensure the command field is exactly
one character to preserve parser field alignment: validate opts.command is a
single char (or explicitly truncate to one char) before appending; if invalid,
throw an Error indicating "command must be a single character". Update the logic
in buildTraceString (the function that currently appends opts.command before
trace_version and commitish) so only a single-character command is appended,
preventing shifts of trace_version/commitish offsets.
- Around line 80-87: When iterating opts.addresses and handling a.object ===
"bun" before calling encodeVlq(a.address), validate that a.address is not 0 or 1
(those are reserved: 0 = address-list terminator, 1 = named-object marker); if
it is, throw a clear Error mentioning the offending a.address and context (e.g.,
"reserved bun address 0/1 in opts.addresses") so malformed fixtures are detected
early; update the loop around opts.addresses (the branch that calls encodeVlq)
to perform this guard/check and raise an informative error instead of encoding
the reserved values.
In `@test/parse.test.ts`:
- Around line 18-22: The test asserts result is not null but then passes
possibly-null-typed result into stable(), leaving stable's null-branch dead;
update the test to either use a non-null assertion when calling stable (i.e.,
pass result! to stable) or change stable's signature/implementation to accept
only non-null input and remove null handling; locate the parse call in the test
(variable result) and the stable function definition to apply the chosen fix so
TypeScript's types match the runtime assertion.
---
Outside diff comments:
In `@backend/remap.ts`:
- Around line 115-118: The code constructs an Error when the symbolizer
subprocess (subproc) exits with a non-zero code but never throws it, so modify
the error-handling branch in remap.ts (the block that checks (await
subproc.exited) !== 0) to throw the constructed error after setting e.code;
ensure you still await Bun.readableStreamToText(subproc.stderr) to include
stderr text in the message and then throw the Error (keeping the error code
"PdbAddr2LineFailed") instead of silently continuing, so symbolization failures
are propagated.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96bb57e8-e1e1-46a3-a775-4b0d3c7b6a39
⛔ Files ignored due to path filters (2)
test/__snapshots__/parse.test.ts.snapis excluded by!**/*.snaptest/__snapshots__/symbolize.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (42)
.github/workflows/test.ymlbackend/index.tsbackend/remap.tsbackend/sentry.tsbackend/symbolize.tspackage.jsonscripts/capture-fixture.tsscripts/seed-parse-fixtures.tstest/README.mdtest/fixtures/parse/v1-linux-aarch64-segfault.jsontest/fixtures/parse/v1-linux-x86_64-foreign-object.jsontest/fixtures/parse/v1-linux-x86_64-segfault.jsontest/fixtures/parse/v1-linux-x86_64-with-js-and-unknown-frames.jsontest/fixtures/parse/v1-linux-x86_64_baseline-segfault.jsontest/fixtures/parse/v1-macos-aarch64-oom.jsontest/fixtures/parse/v1-macos-aarch64-panic-compressed.jsontest/fixtures/parse/v1-macos-aarch64-segfault.jsontest/fixtures/parse/v1-macos-aarch64-with-url-prefix.jsontest/fixtures/parse/v1-macos-x86_64-segfault.jsontest/fixtures/parse/v1-macos-x86_64_baseline-segfault.jsontest/fixtures/parse/v1-windows-aarch64-segfault.jsontest/fixtures/parse/v1-windows-x86_64-segfault.jsontest/fixtures/parse/v1-windows-x86_64_baseline-segfault.jsontest/fixtures/parse/v2-linux-aarch64-segfault.jsontest/fixtures/parse/v2-linux-x86_64-segfault.jsontest/fixtures/parse/v2-linux-x86_64_baseline-segfault.jsontest/fixtures/parse/v2-macos-aarch64-segfault.jsontest/fixtures/parse/v2-macos-x86_64-segfault.jsontest/fixtures/parse/v2-macos-x86_64_baseline-segfault.jsontest/fixtures/parse/v2-windows-aarch64-segfault.jsontest/fixtures/parse/v2-windows-x86_64-error-reason.jsontest/fixtures/parse/v2-windows-x86_64-segfault.jsontest/fixtures/parse/v2-windows-x86_64_baseline-segfault.jsontest/fixtures/symbolize/all-unknown-reverts.jsontest/fixtures/symbolize/linux-jsc-signal-handler.jsontest/fixtures/symbolize/macos-typical.jsontest/fixtures/symbolize/mixed-js-and-foreign-frames.jsontest/fixtures/symbolize/panic-prefix-stripped.jsontest/fixtures/symbolize/windows-backslash-paths.jsontest/helpers/encode.tstest/parse.test.tstest/symbolize.test.ts
Comment on lines
+18
to
+20
| - uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: latest |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider pinning Bun version for CI stability.
Using bun-version: latest may cause unexpected CI failures when a new Bun release introduces breaking changes. Consider pinning to a specific version (e.g., 1.2.x) for reproducible builds, or at minimum a major version constraint.
♻️ Suggested fix
- uses: oven-sh/setup-bun@v2
with:
- bun-version: latest
+ bun-version: "1.2"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: latest | |
| - uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: "1.2" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yml around lines 18 - 20, Update the GitHub Actions
step that uses oven-sh/setup-bun@v2 to stop using bun-version: latest and
instead pin a stable version or range (for example bun-version: 1.2.x or a
specific patch like 1.2.3) to ensure reproducible CI; locate the step with uses:
oven-sh/setup-bun@v2 and change the bun-version input accordingly so CI does not
automatically float to new, potentially breaking Bun releases.
Comment on lines
+10
to
+12
| async function remapToPayload(parse: Parse, remap: Remap, trace_str: string): Promise<Sentry.Payload> { | ||
| const event_id = MD5.hash(parse.cache_key!, "hex"); | ||
| const view_url = `https://bun.report/${trace_str}/view`; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how trace_str is passed to sendToSentry and whether it's already sanitized
rg -n -A5 'sendToSentry\(' --type=tsRepository: oven-sh/bun.report
Length of output: 990
🏁 Script executed:
#!/bin/bash
# Check where 'str' and 'parsed_str' come from in backend/index.ts
head -n 185 backend/index.ts | tail -n 10 && echo "---" && head -n 315 backend/index.ts | tail -n 10Repository: oven-sh/bun.report
Length of output: 602
🏁 Script executed:
#!/bin/bash
# Search for any URL encoding or sanitization utilities in the codebase
rg -n 'encodeURI|encodeURIComponent|sanitize|escape' --type=tsRepository: oven-sh/bun.report
Length of output: 1735
🏁 Script executed:
#!/bin/bash
# Look for where 'str' is defined/assigned around line 182
sed -n '150,185p' backend/index.ts | head -n 40Repository: oven-sh/bun.report
Length of output: 1119
🏁 Script executed:
#!/bin/bash
# Look for where 'parsed_str' is defined/assigned around line 308
sed -n '270,310p' backend/index.ts | head -n 50Repository: oven-sh/bun.report
Length of output: 1240
🏁 Script executed:
#!/bin/bash
# Find where remapAndRedirect is called to understand parsed_str origin
rg -n 'remapAndRedirect' backend/index.tsRepository: oven-sh/bun.report
Length of output: 242
🏁 Script executed:
#!/bin/bash
# Get more context around line 308 to see where parsed_str is derived
sed -n '260,315p' backend/index.tsRepository: oven-sh/bun.report
Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Check where str is first assigned/defined before line 200
sed -n '1,200p' backend/index.ts | tail -n 60Repository: oven-sh/bun.report
Length of output: 1971
Use encodeURI() when constructing the view_url.
The trace_str parameter is derived from the request URL pathname and interpolated directly into the view_url without encoding. While URL pathnames have inherent restrictions, this approach is inconsistent with similar URL construction patterns in backend/index.ts (lines 119, 143), which properly use encodeURI(). To ensure the constructed URL is safe and consistent, wrap trace_str with encodeURI():
Example fix
const view_url = `https://bun.report/${encodeURI(trace_str)}/view`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/sentry.ts` around lines 10 - 12, In remapToPayload, the constructed
view_url uses trace_str raw; update the view_url creation (the view_url variable
inside remapToPayload) to pass trace_str through encodeURI() so the path segment
is properly escaped (e.g., interpolate encodeURI(trace_str) into the URL) to
match other constructors and avoid unsafe characters.
Comment on lines
+72
to
+99
| export function filterAddresses(addrs: Address[]): Address[] { | ||
| const old = addrs.slice(); | ||
|
|
||
| while ( | ||
| addrs[0]?.function?.includes?.("WTF::jscSignalHandler") || | ||
| addrs[0]?.function?.includes?.("assertionFailure") || | ||
| addrs[0]?.function?.includes?.("panic") || | ||
| addrs[0]?.function?.endsWith?.("assert") | ||
| ) { | ||
| addrs.shift(); | ||
|
|
||
| // remove additional `??` lines | ||
| while (addrs.length > 0 && (!addrs[0].remapped || addrs[0].function === "??")) { | ||
| addrs.shift(); | ||
| } | ||
| } | ||
|
|
||
| // remove trailing ?? lines | ||
| while (addrs.length > 0 && (!addrs[addrs.length - 1].remapped || addrs[addrs.length - 1].function === "??")) { | ||
| addrs.pop(); | ||
| } | ||
|
|
||
| // if this operation somehow removes all addresses, revert | ||
| if (addrs.length === 0) { | ||
| return old; | ||
| } | ||
|
|
||
| return addrs; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid mutating filterAddresses input in-place.
filterAddresses() currently mutates the caller-provided array (shift/pop). Since this module is framed as a pure pipeline, cloning at entry avoids side effects for external callers.
♻️ Proposed refactor
-export function filterAddresses(addrs: Address[]): Address[] {
- const old = addrs.slice();
+export function filterAddresses(input: Address[]): Address[] {
+ const addrs = input.slice();
+ const old = addrs.slice();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function filterAddresses(addrs: Address[]): Address[] { | |
| const old = addrs.slice(); | |
| while ( | |
| addrs[0]?.function?.includes?.("WTF::jscSignalHandler") || | |
| addrs[0]?.function?.includes?.("assertionFailure") || | |
| addrs[0]?.function?.includes?.("panic") || | |
| addrs[0]?.function?.endsWith?.("assert") | |
| ) { | |
| addrs.shift(); | |
| // remove additional `??` lines | |
| while (addrs.length > 0 && (!addrs[0].remapped || addrs[0].function === "??")) { | |
| addrs.shift(); | |
| } | |
| } | |
| // remove trailing ?? lines | |
| while (addrs.length > 0 && (!addrs[addrs.length - 1].remapped || addrs[addrs.length - 1].function === "??")) { | |
| addrs.pop(); | |
| } | |
| // if this operation somehow removes all addresses, revert | |
| if (addrs.length === 0) { | |
| return old; | |
| } | |
| return addrs; | |
| export function filterAddresses(input: Address[]): Address[] { | |
| const addrs = input.slice(); | |
| const old = addrs.slice(); | |
| while ( | |
| addrs[0]?.function?.includes?.("WTF::jscSignalHandler") || | |
| addrs[0]?.function?.includes?.("assertionFailure") || | |
| addrs[0]?.function?.includes?.("panic") || | |
| addrs[0]?.function?.endsWith?.("assert") | |
| ) { | |
| addrs.shift(); | |
| // remove additional `??` lines | |
| while (addrs.length > 0 && (!addrs[0].remapped || addrs[0].function === "??")) { | |
| addrs.shift(); | |
| } | |
| } | |
| // remove trailing ?? lines | |
| while (addrs.length > 0 && (!addrs[addrs.length - 1].remapped || addrs[addrs.length - 1].function === "??")) { | |
| addrs.pop(); | |
| } | |
| // if this operation somehow removes all addresses, revert | |
| if (addrs.length === 0) { | |
| return old; | |
| } | |
| return addrs; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/symbolize.ts` around lines 72 - 99, filterAddresses mutates the input
array via shift/pop; clone the input at entry (e.g., const work = addrs.slice())
and perform all removals on that cloned array (use index-based slicing or
mutating methods on the clone) while keeping the original addrs untouched;
ensure the final emptiness check and fallback returns the original input (addrs)
if all entries are removed, and return the filtered clone instead of the
original variable.
Comment on lines
+150
to
+153
| const line = Math.floor(Number(str.slice(second_colon + 1, last_colon))); | ||
| if (isNaN(line)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Tighten source-line number validation.
Line 150 currently accepts non-integer/non-finite numeric forms after coercion. Rejecting anything except finite integers prevents invalid line metadata from leaking downstream.
🛠️ Proposed fix
- const line = Math.floor(Number(str.slice(second_colon + 1, last_colon)));
- if (isNaN(line)) {
+ const rawLine = Number(str.slice(second_colon + 1, last_colon));
+ if (!Number.isInteger(rawLine) || !Number.isFinite(rawLine) || rawLine < 0) {
return null;
}
+ const line = rawLine;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/symbolize.ts` around lines 150 - 153, The current extraction for line
uses Math.floor(Number(...)) which allows non-integer or non-finite numeric
forms; change to strict integer validation: read the substring (using
str.slice(second_colon + 1, last_colon)) into a raw string, ensure it matches an
integer pattern (e.g., /^\-?\d+$/ for signed integers or /^\d+$/ if only
positives), convert to a Number and validate Number.isFinite and
Number.isInteger before accepting it; if any check fails return null. Apply this
to the code around the variables line, str, second_colon and last_colon so only
finite integers are accepted as line numbers.
| "-f", | ||
| ...bun_addrs, | ||
| ]; | ||
| const subproc = Bun.spawn({ cmd, stdio: ["ignore", "pipe", "pipe"] }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing timeout for subprocess.
Unlike backend/remap.ts (line 112) which sets timeout: 5000, this script spawns the symbolizer without a timeout. Bun.spawn supports "timeout: 5000, // 5 seconds in milliseconds" to auto-terminate hung processes. A symbolizer could hang indefinitely on malformed input.
♻️ Suggested fix
-const subproc = Bun.spawn({ cmd, stdio: ["ignore", "pipe", "pipe"] });
+const subproc = Bun.spawn({ cmd, stdio: ["ignore", "pipe", "pipe"], timeout: 30000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const subproc = Bun.spawn({ cmd, stdio: ["ignore", "pipe", "pipe"] }); | |
| const subproc = Bun.spawn({ cmd, stdio: ["ignore", "pipe", "pipe"], timeout: 30000 }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/capture-fixture.ts` at line 76, The Bun.spawn call that creates const
subproc in scripts/capture-fixture.ts is missing a timeout, so the symbolizer
can hang indefinitely; update the Bun.spawn invocation for subproc to include a
timeout option (e.g., timeout: 5000) so Bun will auto-terminate hung processes,
mirroring the behavior used in backend/remap.ts; ensure the timeout value is
passed in the same options object where cmd and stdio are set.
Comment on lines
+31
to
+56
| export type ReasonSpec = | ||
| | { kind: "panic"; message: string } | ||
| | { kind: "unreachable" } | ||
| | { kind: "segfault"; addr_hi: number; addr_lo: number } | ||
| | { kind: "stack_overflow" } | ||
| | { kind: "error"; message: string } | ||
| | { kind: "oom" }; | ||
|
|
||
| function encodeReason(r: ReasonSpec): string { | ||
| switch (r.kind) { | ||
| case "panic": { | ||
| const compressed = deflateSync(Buffer.from(r.message)); | ||
| return "0" + compressed.toString("base64url"); | ||
| } | ||
| case "unreachable": | ||
| return "1"; | ||
| case "segfault": | ||
| return "2" + encodeVlq(r.addr_hi) + encodeVlq(r.addr_lo); | ||
| case "stack_overflow": | ||
| return "7"; | ||
| case "error": | ||
| return "8" + r.message; | ||
| case "oom": | ||
| return "9"; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Expand ReasonSpec/encodeReason to cover all parser reason codes.
The parser supports additional reason codes (illegal_instruction, bus_error, float_exception, unaligned) but this helper can’t generate them yet, which limits fixture branch coverage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/encode.ts` around lines 31 - 56, Add missing parser reason kinds
to the ReasonSpec union and handle them in encodeReason: extend ReasonSpec with
{ kind: "illegal_instruction" }, { kind: "bus_error" }, { kind:
"float_exception" }, and { kind: "unaligned" }, and add corresponding switch
branches in encodeReason that return the correct single-digit prefix for each
(e.g., "3" for illegal_instruction, "4" for bus_error, "5" for float_exception,
"6" for unaligned) so the helper can emit the same reason-code prefixes the
parser expects; update any fixtures/tests that rely on these encoded reasons to
use the new kinds.
Comment on lines
+70
to
+78
| export function buildTraceString(opts: BuildTraceOpts): string { | ||
| if (opts.commitish.length !== 7) throw new Error("commitish must be 7 chars"); | ||
| const [f0, f1] = opts.features ?? [0, 0]; | ||
| let s = ""; | ||
| s += opts.version + "/"; | ||
| s += platform_char[`${opts.os}-${opts.arch}`]; | ||
| s += opts.command; | ||
| s += opts.trace_version; | ||
| s += opts.commitish; |
There was a problem hiding this comment.
Enforce single-character command to preserve parser field alignment.
Line 76 appends opts.command verbatim, but the parser reads command at a fixed single-character position. Multi-char input will shift trace_version/commitish offsets and produce invalid traces.
🛠️ Proposed fix
export function buildTraceString(opts: BuildTraceOpts): string {
if (opts.commitish.length !== 7) throw new Error("commitish must be 7 chars");
+ if (opts.command.length !== 1) throw new Error("command must be exactly 1 char");
const [f0, f1] = opts.features ?? [0, 0];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function buildTraceString(opts: BuildTraceOpts): string { | |
| if (opts.commitish.length !== 7) throw new Error("commitish must be 7 chars"); | |
| const [f0, f1] = opts.features ?? [0, 0]; | |
| let s = ""; | |
| s += opts.version + "/"; | |
| s += platform_char[`${opts.os}-${opts.arch}`]; | |
| s += opts.command; | |
| s += opts.trace_version; | |
| s += opts.commitish; | |
| export function buildTraceString(opts: BuildTraceOpts): string { | |
| if (opts.commitish.length !== 7) throw new Error("commitish must be 7 chars"); | |
| if (opts.command.length !== 1) throw new Error("command must be exactly 1 char"); | |
| const [f0, f1] = opts.features ?? [0, 0]; | |
| let s = ""; | |
| s += opts.version + "/"; | |
| s += platform_char[`${opts.os}-${opts.arch}`]; | |
| s += opts.command; | |
| s += opts.trace_version; | |
| s += opts.commitish; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/encode.ts` around lines 70 - 78, In buildTraceString, ensure the
command field is exactly one character to preserve parser field alignment:
validate opts.command is a single char (or explicitly truncate to one char)
before appending; if invalid, throw an Error indicating "command must be a
single character". Update the logic in buildTraceString (the function that
currently appends opts.command before trace_version and commitish) so only a
single-character command is appended, preventing shifts of
trace_version/commitish offsets.
Comment on lines
+80
to
+87
| for (const a of opts.addresses) { | ||
| if (a.object === "js") { | ||
| s += "="; | ||
| } else if (a.object === "?") { | ||
| s += "_"; | ||
| } else if (a.object === "bun") { | ||
| s += encodeVlq(a.address); | ||
| } else { |
There was a problem hiding this comment.
Guard reserved Bun address values (0 and 1).
For object === "bun", encoding 0 collides with the address-list terminator, and 1 collides with named-object marker semantics in the parser. This can silently generate malformed fixtures.
🛠️ Proposed fix
} else if (a.object === "bun") {
+ if (a.address === 0 || a.address === 1) {
+ throw new Error("bun address values 0 and 1 are reserved by trace encoding");
+ }
s += encodeVlq(a.address);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const a of opts.addresses) { | |
| if (a.object === "js") { | |
| s += "="; | |
| } else if (a.object === "?") { | |
| s += "_"; | |
| } else if (a.object === "bun") { | |
| s += encodeVlq(a.address); | |
| } else { | |
| for (const a of opts.addresses) { | |
| if (a.object === "js") { | |
| s += "="; | |
| } else if (a.object === "?") { | |
| s += "_"; | |
| } else if (a.object === "bun") { | |
| if (a.address === 0 || a.address === 1) { | |
| throw new Error("bun address values 0 and 1 are reserved by trace encoding"); | |
| } | |
| s += encodeVlq(a.address); | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/helpers/encode.ts` around lines 80 - 87, When iterating opts.addresses
and handling a.object === "bun" before calling encodeVlq(a.address), validate
that a.address is not 0 or 1 (those are reserved: 0 = address-list terminator, 1
= named-object marker); if it is, throw a clear Error mentioning the offending
a.address and context (e.g., "reserved bun address 0/1 in opts.addresses") so
malformed fixtures are detected early; update the loop around opts.addresses
(the branch that calls encodeVlq) to perform this guard/check and raise an
informative error instead of encoding the reserved values.
Comment on lines
+18
to
+22
| test(fixture.description, async () => { | ||
| const result = await parse(fixture.input); | ||
| expect(result).not.toBeNull(); | ||
| expect(stable(result)).toMatchSnapshot(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using result! or restructuring to avoid potential confusion.
Line 20 asserts result is not null, but line 21 passes result (which TypeScript still types as possibly null) to stable(). While the runtime behavior is correct, the stable() function's null handling (line 28) becomes dead code. Consider either:
- Using non-null assertion:
stable(result!) - Simplifying
stable()to only accept non-null input
This is minor since the test logic is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/parse.test.ts` around lines 18 - 22, The test asserts result is not null
but then passes possibly-null-typed result into stable(), leaving stable's
null-branch dead; update the test to either use a non-null assertion when
calling stable (i.e., pass result! to stable) or change stable's
signature/implementation to accept only non-null input and remove null handling;
locate the parse call in the test (variable result) and the stable function
definition to apply the chosen fix so TypeScript's types match the runtime
assertion.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/roundtrip.test.ts`:
- Around line 7-12: The test loop only covers positive VLQ values; add negative
fixtures to exercise signed decoding in decodePart and ensure encodeVlq
roundtrips negatives too—update the array in the test (the values iterated for
encodeVlq/decodePart) to include representative negative cases such as -1, -32,
-33 and a larger negative like -0x1234 (and any sentinel boundary your decoder
expects) so the same assertions (dec === v and consumed === encodeVlq(v).length)
run for negative inputs as well.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d958e858-f964-411a-a725-0d0d69bd0dc5
📒 Files selected for processing (1)
test/roundtrip.test.ts
Comment on lines
+7
to
+12
| for (const v of [0, 1, 2, 31, 32, 0x1234, 0x10ab34, 0x7fffffff]) { | ||
| test(`0x${v.toString(16)}`, () => { | ||
| const [dec, consumed] = decodePart(encodeVlq(v)); | ||
| expect(dec).toBe(v); | ||
| expect(consumed).toBe(encodeVlq(v).length); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Expand VLQ roundtrip cases to cover negative decode paths.
This set misses signed edge cases handled by decodePart (e.g., negative values and sentinel-style boundaries). Add a few negative fixtures so regressions in signed decoding are caught early.
Suggested test diff
- for (const v of [0, 1, 2, 31, 32, 0x1234, 0x10ab34, 0x7fffffff]) {
+ for (const v of [0, 1, 2, 31, 32, -1, -32, 0x1234, 0x10ab34, 0x7fffffff]) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/roundtrip.test.ts` around lines 7 - 12, The test loop only covers
positive VLQ values; add negative fixtures to exercise signed decoding in
decodePart and ensure encodeVlq roundtrips negatives too—update the array in the
test (the values iterated for encodeVlq/decodePart) to include representative
negative cases such as -1, -32, -33 and a larger negative like -0x1234 (and any
sentinel boundary your decoder expects) so the same assertions (dec === v and
consumed === encodeVlq(v).length) run for negative inputs as well.
Comment on lines
+83
to
+85
| if (c.reason.kind === "panic") expect(p.message).toBe(`panic: ${c.reason.message}`); | ||
| if (c.reason.kind === "unreachable") expect(p.message).toContain("unreachable"); | ||
| if (c.reason.kind === "segfault") expect(p.message).toContain(c.reason.addr_lo.toString(16).toUpperCase()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer exact reason-message assertions for non-panic cases.
toContain(...) is a bit loose for snapshot-locking behavior; exact expected strings would better catch formatting regressions in reason rendering.
Suggested assertion tightening
- if (c.reason.kind === "unreachable") expect(p.message).toContain("unreachable");
- if (c.reason.kind === "segfault") expect(p.message).toContain(c.reason.addr_lo.toString(16).toUpperCase());
+ if (c.reason.kind === "unreachable") {
+ expect(p.message).toBe("panic: reached unreachable code");
+ }
+ if (c.reason.kind === "segfault") {
+ expect(p.message).toBe(`Segmentation fault at address 0x${c.reason.addr_lo.toString(16).toUpperCase().padStart(8, "0")}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (c.reason.kind === "panic") expect(p.message).toBe(`panic: ${c.reason.message}`); | |
| if (c.reason.kind === "unreachable") expect(p.message).toContain("unreachable"); | |
| if (c.reason.kind === "segfault") expect(p.message).toContain(c.reason.addr_lo.toString(16).toUpperCase()); | |
| if (c.reason.kind === "panic") expect(p.message).toBe(`panic: ${c.reason.message}`); | |
| if (c.reason.kind === "unreachable") { | |
| expect(p.message).toBe("panic: reached unreachable code"); | |
| } | |
| if (c.reason.kind === "segfault") { | |
| expect(p.message).toBe(`Segmentation fault at address 0x${c.reason.addr_lo.toString(16).toUpperCase().padStart(8, "0")}`); | |
| } |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/remap.ts (1)
115-118:⚠️ Potential issue | 🔴 CriticalCritical: Error created but never thrown.
When
pdb-addr2linefails (non-zero exit), the error objecteis created and itscodeproperty is set, but the error is never thrown. The function silently continues with empty/partialstdout, leading to incorrect symbolization results.🐛 Proposed fix
if ((await subproc.exited) !== 0) { const e: any = new Error("pdb-addr2line failed: " + (await Bun.readableStreamToText(subproc.stderr))); e.code = "PdbAddr2LineFailed"; + throw e; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/remap.ts` around lines 115 - 118, The code creates an Error object when pdb-addr2line exits non-zero but never throws it; update the block in remap.ts (the section that checks if ((await subproc.exited) !== 0)) to read stderr into a variable, construct the Error (set e.code = "PdbAddr2LineFailed"), and then throw that error so the caller sees the failure (e.g., replace the current creation-only behavior with a throw after awaiting Bun.readableStreamToText(subproc.stderr)); ensure you await the stderr read before throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@backend/remap.ts`:
- Around line 115-118: The code creates an Error object when pdb-addr2line exits
non-zero but never throws it; update the block in remap.ts (the section that
checks if ((await subproc.exited) !== 0)) to read stderr into a variable,
construct the Error (set e.code = "PdbAddr2LineFailed"), and then throw that
error so the caller sees the failure (e.g., replace the current creation-only
behavior with a throw after awaiting Bun.readableStreamToText(subproc.stderr));
ensure you await the stderr read before throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d3c545c-d5c0-411b-951c-7a00a06ea0d0
📒 Files selected for processing (4)
backend/feature.tsbackend/remap.tsbackend/sentry.tslib/parser.ts
💤 Files with no reviewable changes (2)
- backend/feature.ts
- lib/parser.ts
Evenly split (33 macos / 34 linux / 33 windows), all bun >= 1.3.0, selected for diversity across reason (segv/panic/ill/fpe/oom/so), trace size (xs..lg), and presence of unknown/js and foreign-object frames. Each fixture records its source issue number. scripts/harvest-github.ts pages through GitHub issue search and extracts every bun.report URL; scripts/select-fixtures.ts greedily picks one trace per (os, reason, size, has-unknown, has-foreign, arch) signature then tops up to the per-os target. scripts/ingest-urls.ts does the same for a pasted/file list.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
backend/symbolize.ts(no behavior change) and add fixture-driven snapshot tests forparse()and the symbolizer-output →Address[]path. 24 parse fixtures (all platforms × v1/v2) + 6 symbolize fixtures lock current behavior; decoder improvements will show up as snapshot diffs.scripts/capture-fixture.tsrecords a real crash URL (and optionally realllvm-symbolizer/pdb-addr2linestdout) as a fixture;scripts/seed-parse-fixtures.tsregenerates the synthetic matrix. Seetest/README.md..github/workflows/test.ymlto run typecheck + tests in CI.view_url(https://bun.report/<trace>/view) on Sentry events as a tag (≤200 chars) and always underextra, so issues link back to the report site.v3 is intentionally not covered — only v1/v2 fixtures.
Test plan
bun x tsc --noEmitbun test— 30 pass / 30 snapshotsview_urlin tags/extrabun scripts/capture-fixture.ts <name> '<url>' --symbolizeand review snapshots (follow-up)