Repository navigation
Conversation
The `{` arm of the `<tag>` rewriter copied everything up to the next `}`
verbatim, so a template containing a `{{` brace escape shielded all of the
markup between it and the next `}` from the rewrite. Those tags were then
neither converted to ANSI nor stripped, which is how `bun install` printed
`<green>"bun"<r>: <green>"latest"<r>` in the "expects a map of specifiers"
error. Treat `{{` as the literal brace it is in the proc-macro, in
`pretty_fmt_runtime`, and in clap's copy of the same state machine.
|
Status: reproduced on the released binary ( |
WalkthroughEscaped double opening braces now remain literal in compile-time, runtime, and CLI formatting paths. Markup processing continues after escaped braces. Regression tests cover placeholders, format specifications, color handling, macro/runtime parity, and install diagnostics. ChangesEscaped brace formatting
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: 1
🤖 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 `@test/cli/install/bun-install.test.ts`:
- Around line 4991-5002: Remove the stdout content assertion following the
concurrent Promise.all in the install test, while retaining proc.stdout.text()
in that Promise.all to drain stdout. Keep the normalized stderr snapshot and
exitCode assertions unchanged.
🪄 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: 5aad6f5b-bafd-446c-b663-408c9d9ba61c
⛔ Files ignored due to path filters (1)
test/cli/install/__snapshots__/bun-install.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
src/bun_core/output.rssrc/bun_core_macros/lib.rssrc/clap_macros/lib.rstest/cli/install/bun-install.test.ts
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(` | ||
| "1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}} | ||
| ^ | ||
| error: dependencies expects a map of specifiers, e.g. | ||
| "dependencies": { | ||
| "bun": "latest" | ||
| } | ||
| at <dir>/package.json:1:55" | ||
| `); | ||
| expect(stdout).toContain("bun install v1."); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the version-prefix assertion.
The bun install version header is not part of this regression and can change independently. Keep proc.stdout.text() in the concurrent Promise.all, but do not assert its contents. The normalized stderr snapshot and exitCode already verify the invalid-specifier path.
Suggested change
- const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+ const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(`
...
- expect(stdout).toContain("bun install v1.");
expect(exitCode).toBe(1);As per coding guidelines: “Subprocess tests must drain stdout, stderr, and process exit concurrently.”
Based on learnings: “Do not assert stdout for commands like bun install whose output can vary between runs.”
📝 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.
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(` | |
| "1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}} | |
| ^ | |
| error: dependencies expects a map of specifiers, e.g. | |
| "dependencies": { | |
| "bun": "latest" | |
| } | |
| at <dir>/package.json:1:55" | |
| `); | |
| expect(stdout).toContain("bun install v1."); | |
| const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | |
| expect(normalizeBunSnapshot(stderr, String(dir))).toMatchInlineSnapshot(` | |
| "1 | {"name":"foo","version":"0.0.1","dependencies":{"bun":1}} | |
| ^ | |
| error: dependencies expects a map of specifiers, e.g. | |
| "dependencies": { | |
| "bun": "latest" | |
| } | |
| at <dir>/package.json:1:55" | |
| `); |
🤖 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 `@test/cli/install/bun-install.test.ts` around lines 4991 - 5002, Remove the
stdout content assertion following the concurrent Promise.all in the install
test, while retaining proc.stdout.text() in that Promise.all to drain stdout.
Keep the normalized stderr snapshot and exitCode assertions unchanged.
Sources: Coding guidelines, Learnings
|
Taking the stdout suggestion: the new test will only read stderr and the exit code (stdout ignored, same as the FORCE_COLOR test). It goes out with the next push, which I am holding until a review pass on the diff finishes so there is a single CI run. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix applied consistently across all three rewriter copies.
What was reviewed:
- Traced the
{{handling through the state machine in all three sites; verified{{{{,{{{spec}}}, and{{...{:<4}...}}cases behave correctly (all covered by new unit tests). - Confirmed
}}needs no special case since lone}already falls to the default arm. - Checked the new install tests follow harness conventions (
tempDir,{...env, FORCE_COLOR}, concurrent pipe drain, exitCode last); snapshot updates were previously certifying the bug.
Extended reasoning...
Overview
Fixes a bug in the pretty_fmt! markup rewriter (and its two documented copies in pretty_fmt_runtime and clap_macros::pretty_rewrite) where the { arm — which exists to shield format specs like {:<16} from the <tag> parser — did not recognize {{ as format_args!'s literal-brace escape. It scanned from {{ to the next }, copying the whole span verbatim and leaving any <green>...<r> markup inside it unrewritten. The user-visible symptom was raw <green>"bun"<r> tags leaking into three bun install error messages from Package.rs.
The fix is a 5-line early-out in each of the three state machines: on {{, emit {{ and continue so the text after it is still subject to markup rewriting; only an undoubled { starts the verbatim spec copy. Tests are added at the unit level (proc-macro output, runtime rewriter output, macro/runtime parity, pretty_fmt_args end-to-end) and at the integration level (updated snapshots + a new FORCE_COLOR test asserting the actual ANSI escapes).
Security risks
None. This is a compile-time / display-time string formatter for Bun's own error-message templates; no untrusted input reaches these paths, and the change only affects how {{ is tokenized relative to the existing <tag> rewriter.
Level of scrutiny
Low-to-medium. The change is mechanical and identical across the three sibling implementations, which are already documented as intentional near-copies. The author enumerated the input space carefully (escape abutting a spec, quadruple braces, spec-with-< after an escape), ran the old vs new algorithm over all 97 {{ literals under src/ to confirm only the three intended templates change output, and noted the workspace still compiles — ruling out any template that had been relying on the old behavior to hide a literal <...>. The snapshots being updated were asserting the buggy output, which is exactly the case where a snapshot update is warranted.
Other factors
- All three copies of the state machine are updated in the same PR (fixing the whole class, per repo review guidance).
- New tests follow harness conventions:
tempDir,bunEnvspread,Promise.allpipe draining,normalizeBunSnapshot, exitCode asserted last. - Author verified the tests fail with
USE_SYSTEM_BUN=1and pass withbun bd test. - The
macro_matches_runtimeparity test now covers brace escapes, guarding against future divergence between the proc-macro and runtime paths.
|
Nothing to change from this review. The only pending edit is the stdout assertion removal from the thread above; it goes out in the next push. |
|
Updated 1:05 PM PT - Aug 12th, 2026
❌ @robobun, your commit b3f516f has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37855That installs a local version of the PR into your bun-37855 --bun |
Problem
bun installprints raw markup in its "expects a map of specifiers" error, with colors on or off: the example line comes out as<green>"bun"<r>: <green>"latest"<r>.optionalDependenciesand for a non-string specifier; the install snapshot was asserting the leaked text.{to the next}verbatim, and it treated the{{escape as the start of a spec, so the example body up to its first}was copied through unrewritten.Fix
{{the rewriter emits it unchanged and keeps rewriting what follows; only a single{starts the verbatim copy. A lone}was already passed through, so}}needs nothing. Applied to all three copies of the rewriter.format_args!'s own left to right reading of the template ({{is a literal brace, what follows is ordinary text), so the emitted template means what the source template says.src/containing{{, only the threePackage.rstemplates change their expansion; the rest expand byte for byte as before, and the workspace still compiles.FORCE_COLOR=1asserting real ANSI codes, that fail against the released binary and pass with the fix.Background
pretty_fmt!templates:<green>...<r>style tags are rewritten into ANSI codes (colors on) or dropped (colors off), and the result becomes aformat_args!template.format_args!template{...}is a placeholder and{{/}}are escapes for literal braces. The rewriter must leave placeholders such as{:<16}alone, since a<inside one is not a tag.Original description
What
bun installprints raw markup tags in the "expects a map of specifiers" error, with colors on or off:Same output for a non-object value (
"optionalDependencies": "bar") and for an object with a non-string specifier ("dependencies": {"bun": 1}); all three share the template insrc/install/lockfile/Package.rs. The tags on the first line of the example (<r><green>"{0}"<r>) are rewritten, only the ones after the{{survive.test/cli/install/__snapshots__/bun-install.test.ts.snapand theoptionalDependenciestest were asserting the leaked output.Why
The templates go through
pretty_fmt!(viaadd_error_pretty!), whose output is aformat_args!template. The rewriter's{arm exists to shield format specs such as{:<16}from the tag parser, and it does that by copying everything from a{up to the next}verbatim. It did not know aboutformat_args!'s{{/}}escapes, so forthe scan starting at
{{runs to the first}of the closing}}and emits the whole example body untouched.{{is a literal brace in the template language the macro emits (and in the onepretty_fmt_args/substitute_templateconsume at runtime, which already unescape{{), so the text after it is ordinary text and has to be rewritten like any other text. Matchingformat_args!'s own left to right pairing ({{is an escape, any other{opens a spec) is the only interpretation under which the emitted template means the same thing as the source template. A lone}was already copied through, so}}needs no special case.Fix
src/bun_core_macros/lib.rs(rewrite): on{{, emit{{and continue rewriting; only an undoubled{starts the verbatim spec copy.src/bun_core/output.rs(pretty_fmt_runtime): same change, so the runtime rewriter and the proc-macro keep producing identical output (themacro_matches_runtimetest now covers brace escapes too).src/clap_macros/lib.rs(pretty_rewrite): same change. It is documented as a 1:1 copy ofpretty_fmt_runtimeand produces the plain form of a help description whilepretty_fmt_runtimeproduces the colored form of the same string, so the two have to agree.I scanned every string literal under
src/containing{{(97 of them) through the old and new algorithm; the only ones whose expansion changes are the threePackage.rstemplates. The other{{templates that reach a pretty macro (Timings.rs,unlink_command.rs,S3File.rs,S3Client.rs,JSBundler.rs) have no markup after the escape and expand byte for byte as before. The workspace also still compiles, which rules out any template that had been using{{ ... }}to hide a literal<...>from the tag parser (that would now be a compile error).Tests
src/bun_core/output.rs:markup_after_escaped_brace_is_rewrittenandruntime_markup_after_escaped_brace_is_rewritten(escape followed by tags, escape abutting a spec, spec with<after an escape,pretty_fmt_argsend to end), plus two brace templates in the macro/runtime parity check.cargo miri test -p bun_core -- pretty_fmt: 11 pass (thebun_coretest binary does not link on its own, so miri is how these run).test/cli/install/bun-install.test.ts: updated thedependenciessnapshot and theoptionalDependenciesexpectation, added a test for the third path (object with a non-string specifier) and one withFORCE_COLOR=1asserting the example is rendered as\x1b[32m"bun"\x1b[0m: \x1b[32m"latest"\x1b[0m. The four affected tests fail against the released binary (USE_SYSTEM_BUN=1) and pass withbun bd test.