serve: JSON payload for the development error page, restoring stack traces - #37081
Conversation
…tack traces
`Bun.serve({ development: true })` answers a throwing request with an HTML
error page rendered by `packages/bun-error`. The page's payload was still the
old peechy binary encoding (a base64 blob decoded in the browser by
`src/fallback.ts` + the checked-in `src/api/schema.js`), and the encoder had
lost the exception stack traces, source lines and build/resolve messages
along the way, so the page showed an error title with nothing under it, and
a syntax error in an imported file showed an empty box.
- New `DevErrorPage` (src/runtime/server/DevErrorPage.rs) writes the payload
as JSON into `<script type="application/json">` in the (moved) template
`src/runtime/server/dev-error-page.html`, including frames with 1-based
positions, trimmed source lines, and the request's build/resolve messages.
- `bun_jsc::schema_api` becomes `bun_jsc::exception_list`: plain owned
snapshots of a `ZigException` (`ZigStackTrace::snapshot`), now carrying the
source line text. `SourceURLFormatter` no longer prints a trailing `:` when
line/column are excluded, and origin remapping follows the same rule as
the terminal printer.
- Deleted: the binary `Writer` and fallback types in `bun_options_types`,
`bun_js_parser::parser::Runtime::Fallback`, `bun_ast::api::Log`,
`src/fallback.ts` + its codegen step, `src/api/schema.{js,d.ts}`, the
`peechy` npm dependency, and bun-error's websocket-era `renderBuildFailure`.
bun-error gets a small local `schema.ts` describing the JSON.
- `bun_core::fmt::substitute_named` is the `{[name]s}` templater shared by
`bun init` and the error page; `src/codegen/replacements.ts` inlines the
loader table it used to import from schema.js.
- Test: dev error page payload for a thrown error, a syntax error and a
failed resolution (test/js/bun/http/serve.test.ts).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe PR replaces the binary fallback error-page protocol with native exception snapshots and server-rendered development error pages. It removes fallback decoding, obsolete schemas, related dependencies, and legacy rendering paths. Tests validate embedded JSON error responses. Development error page migration
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bun_core/fmt.rs`:
- Around line 433-455: Add regression tests for substitute_named covering exact
named replacements, adjacent placeholders, unmatched placeholders remaining
unchanged, and malformed markers being copied through without corruption.
Exercise both empty and non-empty surrounding text as needed, preserving the
helper’s documented behavior used by DevErrorPage HTML and generated project
files.
In `@src/codegen/replacements.ts`:
- Around line 91-117: Add “json5” and “md” to the Loader array in enums, in that
exact order after “yaml” so they occupy schema IDs 20 and 21. Keep the generated
LoaderIdToLabel and LoaderLabelToId mappings aligned with these entries.
In `@src/runtime/server/dev-error-page.html`:
- Around line 2-6: Update the root html element in the error page to include
lang="en", and add a stable title element within head alongside the existing
metadata so assistive technology can identify the page.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94a7f353-5940-497d-98c1-ad9eb7d0eccc
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockbun.lockis excluded by!**/*.locktest/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.gitattributes.gitignorepackage.jsonpackages/bun-error/index.tsxpackages/bun-error/markdown.tspackages/bun-error/runtime-error.tspackages/bun-error/schema.tspackages/bun-error/stack-trace-parser.tsscripts/build/codegen.tsscripts/glob-sources.tssrc/api/schema.d.tssrc/api/schema.jssrc/ast/lib.rssrc/bun_core/fmt.rssrc/codegen/replacements.tssrc/fallback-backend.htmlsrc/fallback.tssrc/js_parser/Cargo.tomlsrc/js_parser/parser.rssrc/jsc/VirtualMachine.rssrc/jsc/ZigException.rssrc/jsc/ZigStackFrame.rssrc/jsc/ZigStackTrace.rssrc/jsc/lib.rssrc/options_types/schema.rssrc/runtime/bake/bake_body.rssrc/runtime/cli/init_command.rssrc/runtime/error.rssrc/runtime/server/DevErrorPage.rssrc/runtime/server/RequestContext.rssrc/runtime/server/dev-error-page.htmlsrc/runtime/server/mod.rssrc/runtime/server/server_body.rstest/bake/dev/react-response.test.tstest/internal/source-lints/dead-symbols-pub-exports-sweep.test.tstest/js/bun/http/serve.test.tstest/package.json
💤 Files with no reviewable changes (12)
- .gitattributes
- src/fallback-backend.html
- src/js_parser/Cargo.toml
- src/runtime/error.rs
- test/package.json
- .gitignore
- scripts/build/codegen.ts
- package.json
- src/fallback.ts
- src/js_parser/parser.rs
- src/ast/lib.rs
- src/api/schema.d.ts
main updated src/api/schema.{js,d.ts} (json5/md/xml loader ids) and made
schema.js an explicit input of the JS-modules codegen edge; this branch
deletes those files. Keep the deletion, add the three loader names to the
inlined table in src/codegen/replacements.ts (generated defines are
identical to main's), and drop the schema.js input from
scripts/build/codegen.ts.
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 (2)
src/js_parser/parser.rs (1)
760-761: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for hyphenated JSX tags.
Existing tests cover namespaced tags and member expressions. Add a case such as
<Hello-Button />.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js_parser/parser.rs` around lines 760 - 761, Add a regression test covering a hyphenated JSX tag such as <Hello-Button /> alongside the existing namespaced-tag and member-expression tests, verifying it parses successfully and preserves the expected tag name.Source: Coding guidelines
src/runtime/cli/init_command.rs (1)
1920-1920: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd
bun initentry-point regression tests.Cover
../entry,..\\entry,/absolute/entry,C:\\absolute\\entry, and a safe nested path. Assert that unsafe paths do not create files and the nested path does.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/cli/init_command.rs` at line 1920, Add regression tests for the bun init entry-point validation around is_absolute_loose and split_any, covering ../entry, ..\entry, /absolute/entry, C:\absolute\entry, and a safe nested path. Assert that each unsafe path creates no files, while the safe nested path creates the expected files.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/js_parser/parser.rs`:
- Around line 760-761: Add a regression test covering a hyphenated JSX tag such
as <Hello-Button /> alongside the existing namespaced-tag and member-expression
tests, verifying it parses successfully and preserves the expected tag name.
In `@src/runtime/cli/init_command.rs`:
- Line 1920: Add regression tests for the bun init entry-point validation around
is_absolute_loose and split_any, covering ../entry, ..\entry, /absolute/entry,
C:\absolute\entry, and a safe nested path. Assert that each unsafe path creates
no files, while the safe nested path creates the expected files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c9728aa7-4372-4728-9d57-7e4dd5c66c11
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitattributessrc/bun_core/fmt.rssrc/codegen/replacements.tssrc/js_parser/parser.rssrc/options_types/schema.rssrc/runtime/cli/init_command.rssrc/runtime/server/RequestContext.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
bun-error hides a frame's line/column when they are -1; an invalid Ordinal was being serialized as one_based() == 0 and rendered as ':0:0'.
There was a problem hiding this comment.
Thanks for the fix in b06b256 — one_based_or_missing now emits -1 for frames without a source position, matching bun-error's sentinel. No further issues found, but given the scope (new hand-written JSON serializer embedded in HTML, changes across RequestContext/ZigException/VirtualMachine error paths, ~4.5k lines of legacy encoding removed) this should get a human look.
What was reviewed:
escape_for_script_element—<→\u003cprevents</script>/<!--breakout; only occurs inside JSON strings so the escape is valid.substitute_namedmove tobun_core::fmt— same semantics as the removedinit_command::substitute; covered by init tests and the newunfilledPlaceholderassertion.- Inlined
Loadertable inreplacements.ts— ids 1–22 matchapi::Loaderdiscriminants; generated$LoaderIdToLabel/$LoaderLabelToIdare byte-identical to the deleted schema.js output. originnow gated onis_from_devserverinadd_to_error_listcallers — only affects the URL prefix in dev-error-page frame paths.
Extended reasoning...
Overview
This PR replaces the legacy peechy binary encoding for Bun.serve({ development: true })'s error page with a JSON payload embedded in <script type="application/json">, restoring stack traces, source lines, and build/resolve messages that the old encoder had lost. It adds src/runtime/server/DevErrorPage.rs (hand-written JSON serializer + HTML template substitution), renames bun_jsc::schema_api → exception_list with owned snapshots that now carry source-line text, consolidates the {[name]s} templater into bun_core::fmt::substitute_named, and deletes ~4.5k lines of dead peechy scaffolding (src/api/schema.{js,d.ts}, src/fallback.ts, the Writer/fallback types in bun_options_types, Runtime::Fallback, the peechy npm dep). packages/bun-error gets a local schema.ts and drops the websocket-era renderBuildFailure path.
Security risks
The main surface is user-controlled error content (message, stack, file paths, source lines) serialized as JSON and embedded in a <script> element. escape_for_script_element replaces every < with \u003c, which defeats both </script> and <!-- parser-state changes; since < can only appear inside JSON string literals (all other output is structural or numeric), the escape is always inside a quoted string where \u003c is equivalent. format_json_string_utf8 handles quote/backslash/control escaping. This is dev-mode-only (development: true), which bounds the blast radius. I did not find an injection path.
Level of scrutiny
High. 38 files across Rust core (jsc bindings, VirtualMachine, RequestContext), the bun-error preact frontend, codegen (replacements.ts loader table), and build scripts. The JSON serializer is hand-rolled rather than serde-derived, so field ordering and escaping are manual. Two behavioral tweaks beyond the headline change: SourceURLFormatter now suppresses the trailing : when exclude_line_column is set (fixes a latent formatting bug), and origin is now only passed to add_to_error_list when is_from_devserver is true (previously unconditional). Neither looks wrong, but both are the kind of thing a maintainer should confirm was intentional.
Other factors
My one prior finding (invalid Ordinal positions serializing as 0 instead of -1) was addressed in b06b256 with one_based_or_missing. All CodeRabbit threads are resolved. The new serve.test.ts case is thorough — it asserts the exact JSON shape for a thrown TypeError (frame file/line/column, source lines), a syntax error (build message with line_text), and a resolve error (specifier in on.resolve), plus that no template placeholder survives and the renderer bundle is present. react-response.test.ts was updated to read the new format. Given the breadth and the number of subsystems touched, a maintainer sign-off is appropriate even with clean automated review.
2 participants
What does this PR do?
Bun.serve({ development: true })renders an HTML error page (thepackages/bun-erroroverlay) when a handler throws. Its payload was still the legacy peechy binary encoding — a base64 blob decoded in the browser bysrc/fallback.tsplus the checked-in generatedsrc/api/schema.js— and the encoder had lost stack traces, source lines and build messages, so the page showed only an error title (and an empty box for a syntax error in an imported file).DevErrorPage(src/runtime/server/DevErrorPage.rs) embeds the payload as JSON in<script type="application/json">(with<escaped) insidesrc/runtime/server/dev-error-page.html. Frames (1-based line/column, matching the terminal), source lines and build/resolve messages are back.bun_jsc::schema_api→bun_jsc::exception_list: owned snapshots of aZigExceptionthat now keep source line text;SourceURLFormatterstops emitting a trailing:when line/column are excluded.Writer/fallback types inbun_options_types,Runtime::Fallbackinbun_js_parser,bun_ast::api::Log,src/fallback.tsand its codegen step,src/api/schema.{js,d.ts}(~4.5k lines), thepeechynpm dep, and bun-error's dead websocket-erarenderBuildFailure. bun-error gets a localschema.tsfor the JSON shape.bun_core::fmt::substitute_namedis now the one{[name]s}templater (shared withbun init);src/codegen/replacements.tsinlines the loader table it imported fromschema.js(byte-identical defines).Follow-ups, not in here: bun-error's unused client-side
renderRuntimeError/sourcemap path, and collapsing theschema::apimirror enums (which also owns the loader-tablejson5/mdgap noted inreplacements.ts).How did you verify your code works?
serve.test.tscase spawns a dev server and asserts the JSON payload for a thrownTypeError(exact frame incl. file/line/column, source line text), a syntax error in an imported module (message, location,line_text) and a failed package resolution; it fails on current canary and passes on this build.test/bake/dev/react-response.test.ts(reads the same payload),test/js/bun/runtime-error.test.tsandtest/cli/init/init.test.tspass.file:line:col, highlighted source lines and the stack (before: title only / empty for build errors). Also drove HEAD (no body), a thrown string, and a message containing</script><script>(stays inside the JSON).cargo clippy --workspace, Windows cross-check ofbun_jsc/bun_js_parser.