Repository navigation
shell: treat comma-less brace groups as literal instead of truncating - #34856
Conversation
A `{...}` group with no top-level comma is not a brace expansion in
bash; it is literal. Bun's brace lexer tokenized every `{`/`}` pair as
Open/Close regardless, so a nested `{}` or `{foo}` inside an expanding
group became a zero-variant expansion and `expand_nested` stopped
without returning to the parent, silently dropping the rest of the
word:
echo x{a,{}}y -> "xay x" (bash: "xay x{}y")
echo p{q{},r}s -> "pq prs" (bash: "pq{}s prs")
Additionally, a word that tripped the brace-expansion hint but expanded
to zero groups (e.g. `echo }{,`) indexed into an empty output slice in
`do_brace_expand` and panicked.
The lexer now tracks whether each open brace has seen a comma at its
own depth; on `}` with no comma the Open token is demoted to literal
`{` and the `}` is emitted as text, so the expander never sees a
zero-variant group. `flatten_tokens` is rewritten to compact in place
(the old `Vec::remove` loop went quadratic on the long text runs the
rollback now produces), and `do_brace_expand` emits the word unchanged
when the expansion count is zero.
Existing `{{a,b}{c,d}}` assertions are updated to the bash 5.2 output
they now match.
|
Reproduced on 1.4.0: Diff is green: |
|
Warning Review limit reached
Next review available in: 8 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 (2)
WalkthroughBrace lexing now treats groups without top-level commas as literal text. Token flattening and runtime expansion handle these groups without expansion, with updated API and shell tests covering nested, deep, tail, and glob cases. ChangesBrace expansion semantics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 6:37 PM PT - Jul 20th, 2026
❌ @robobun, your commit 08addfe has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34856That installs a local version of the PR into your bun-34856 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #34223. That PR guards the The |
Also switch the subprocess brace test from external printf to the echo
builtin: on Windows printf resolves to MSYS printf.exe, whose runtime
re-expands {} in argv, so the literal {} words never reached the
assertion.
There was a problem hiding this comment.
71cd2b3 addresses both earlier findings — current_out.clone() keeps the pattern for the glob transition and the new {x},*.txt test exercises it. Beyond the stderr-drain nit, I also traced the new demote-on-} path through nested comma-less groups ({{foo}}), mixed nesting ({a{b,c}}, a{{,}}b), and unclosed-brace rollback over already-demoted tokens ({a,{b}) against the rewritten in-place flatten_tokens — each produces the bash-matching token stream with no orphaned Comma tokens.
Extended reasoning...
Re-reviewed after 71cd2b3. The mem::take → clone() change in do_brace_expand restores the current_out-survives-for-glob contract, and the added {x},*.txt test asserts the full no matches found: {x},*.txt message so the pattern is observably intact. The dropped expect(stderr).toBe("") resolves the ASAN-flake concern; the follow-on nit about the now-undrained pipe is inline. I hand-traced the lexer's new has_comma tracking through several shapes not in the test matrix — nested comma-less groups, mixed inner-expands/outer-literal, and rollback_braces running over tokens already demoted to Text — and each yields balanced Open/Close pairs with no stray Comma at a demoted depth. The flatten_tokens rewrite maintains write ≤ read so the swap/merge never aliases and contains_nested is computed on the same pass. Not approving: this is a user-visible behavior change to existing patterns ({{a,b}{c,d}} output changes) plus a full algorithm rewrite of flatten_tokens, which warrants a maintainer's sign-off.
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 `@src/runtime/shell/states/Expansion.rs`:
- Around line 322-325: Trim the new comment explaining the count == 0 fallback
near the Expansion state logic in src/runtime/shell/states/Expansion.rs lines
322-325 to three lines or fewer, preserving only the essential invariant and
rationale. Also shorten the describe-block comment in
test/js/bun/shell/brace.test.ts lines 184-188 and the subprocess-test comment at
lines 215-218 to three lines or fewer; no code behavior changes are needed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15de8c07-c7f3-4a54-adc4-8e7ac1e0509c
📒 Files selected for processing (4)
src/runtime/shell/states/Expansion.rssrc/shell_parser/braces.rstest/js/bun/shell/brace.test.tstest/js/bun/shell/bunshell.test.ts
…34865) ## Problem When a nested brace group ends with an empty alternative (`{a,}`), the nested-expansion parser drops that variant. `calculate_expanded_amount` still counts it, so the output array is sized for N variants but only N-1 are written, leaving the last slot as an empty string. ```js Bun.$.braces("{x,a{,}b}") // ["x","ab",""] bash: x ab ab Bun.$.braces("{x,{a,}}z") // ["xz","az",""] bash: xz az z Bun.$.braces("a{b,c{d,}}e") // ["abe","acde",""] bash: abe acde ace ``` Only the nested (`contains_nested`) path is affected; the flat path handles trailing empties correctly via `build_expansion_table`. Leading (`{,a}`) and middle (`{a,,b}`) empties already work. ## Cause In `Parser::parse_expansion`, after the inner loop breaks on `Comma` and pushes a variant, the outer `while !self.match_any(&[Close, Eof])` peeks `Close`, consumes it, and exits without pushing the pending empty variant. ## Fix Make the inner loop the sole consumer of `Close`/`Eof`. Every iteration of the outer loop now pushes exactly one variant and terminates only after the variant following a trailing comma has been recorded. This also brings the parser into agreement with `calculate_expanded_amount`, which already counted the trailing empty. ### Collateral: nested `{}` The same guard also caused a nested comma-free `{}` to parse to zero variants. With zero variants `expand_nested` returns early without bubbling up, so any text after the `{}` in that branch was dropped: ```js Bun.$.braces("{x,a{}b}") // was ["x","a"] now ["x","ab"] Bun.$.braces("{a,b{}}c") // was ["ac","b"] now ["ac","bc"] ``` After this change a nested `{}` parses to one empty variant, which matches what `calculate_expanded_amount` already computed and what `expand_flat` already does for `a{}b`. Bash treats a comma-free `{}` as literal text, so neither behavior is bash-exact; that broader question is the subject of #34856. This change only brings the nested parser into agreement with the slot count and the flat path. ## Verification Added a `test.each` block in `test/js/bun/shell/brace.test.ts` covering trailing, leading, and middle empty variants at various nesting positions, sibling products of trailing-empty groups, and the nested `{}` suffix case. 10 of the 14 new cases fail on the released binary and all pass with this change; existing `$.braces`, `bunshell`, `parse`, and `lex` suites remain green. <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 0 · 2 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 10 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/shell/brace.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (0261e12) test/js/bun/shell/brace.test.ts: (pass) $.braces > no-op [2.54ms] (pass) $.braces > 2 [1.83ms] (pass) $.braces > 3 [1.57ms] (pass) $.braces > nested [1.87ms] (pass) $.braces > nested 2 [1.94ms] (pass) $.braces > nested sibling product [1.53ms] (pass) $.braces > nested sibling product with surrounding text [1.80ms] (pass) $.braces > nested sibling product mixed with variants [1.70ms] (pass) $.braces > nested sibling product triple [2.14ms] 65 | // empty variant, matching calculate_expanded_amount and expand_flat. 66 | ["{x,a{}b}", ["x", "ab"]], 67 | ["{a,b{}}c", ["ac", "bc"]], 68 | ["{x,{}y}", ["x", "y"]], 69 | ])("%s", (pattern, expected) => { 70 | expect($.braces(pattern)).toEqual(expecte ... (truncated) release without fix: all passed bun test v1.4.0-canary.1 (0261e12) test/js/bun/shell/brace.test.ts: (pass) $.braces > no-op [0.05ms] (pass) $.braces > 2 [0.06ms] (pass) $.braces > 3 [0.02ms] (pass) $.braces > nested [0.05ms] (pass) $.braces > nested 2 [0.03ms] (pass) $.braces > nested sibling product [0.02ms] (pass) $.braces > nested sibling product with surrounding text [0.02ms] (pass) $.braces > nested sibling product mixed with variants [0.03ms] (pass) $.braces > nested sibling product triple [0.03ms] (pass) $.braces > nested with empty variant > {x,a{,}b} [0.02ms] (pass) $.braces > nested with empty variant > {x,{a,}}z [0.01ms] (pass) $.braces > nested with empty variant > {x,{,a}}z (pass) $.braces > nested with empty variant > {x,{,}}z (pass) $.braces > nested with empty variant > a{b,c{d,}}e (pass) $.braces > nested with empty variant > a{b,c{,d}}e (pass) $.braces > nested with empty variant > {x,{a,,b}} (pass) $.braces > nested with empty variant > {x,{a,b,}} (pass) $.braces > nested with empty variant > {{a,},x} (pass) $.braces > nested with empty variant > {{a,}{b,}} (pass) $.braces > nested with empty variant > p{q,{r,}{s,}}t [0.01ms] (pass) $.braces > nested with empty variant > {x,a ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/shell/brace.test.ts info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) bun test v1.4.0 (0261e12) test/js/bun/shell/brace.test.ts: (pass) $.braces > no-op [2.32ms] (pass) $.braces > 2 [1.84ms] (pass) $.braces > 3 [1.58ms] (pass) $.braces > nested [1.85ms] (pass) $.braces > nested 2 [1.94ms] (pass) $.braces > nested sibling product [1.52ms] (pass) $.braces > nested sibling product with surrounding text [1.83ms] (pass) $.braces > nested sibling product mixed with variants [1.70ms] (pass) $.braces > nested sibling product triple [2.22ms] (pass) $.braces > nested with empty variant > {x,a{,}b} [1.69ms] (pass) $.braces > nested with empty variant > {x,{a,}}z [0.64ms] (pass) $.braces > nested with empty variant > {x,{,a}}z [0.50ms] (pass) $.braces > nested with empty variant > {x,{,}}z [0.49ms] (pass) $.braces > nested with ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: checking for self-update (current version: 1.29.0) [configured] bun-profile → bun (stripped) in 717ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/7] gen NodeModuleModule.lut.h Generating /workspace/bun/build/release/codegen/NodeModuleModule.lut.h from /workspace/bun/src/jsc/modules/NodeModuleModule.cpp [2/7] gen cpp.rs (cppbind) [2/7] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu) info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05) info: component rust-src is up to date info: component rust-std is up to date info: checking for self-update (current version: 1.29.0) nightly-2026-05-06-x86_64-unknown-linux-gnu unchanged - rustc 1.97.0-nightly (e95e73209 2026-05-05) �[1m�[92m Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core) �[1m�[92m Compiling�[0m bu ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/shell_parser/braces.rs | 25 ++++++++++++------------- test/js/bun/shell/brace.test.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 13 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/shell_parser/braces.rs 4 1 0 test/js/bun/shell/brace.test.ts 1 2 0 ``` </details> **root cause** · written by the author bot The outer loop guard in `parse_expansion` consumed the `Close` token immediately after a trailing comma in a nested brace group, so the pending empty variant was never pushed even though `calculate_expanded_amount` had already counted it, leaving the last output slot unwritten. The fix restructures the loop so that only the inner loop consumes `Close` or `Eof`, ensuring every outer iteration pushes exactly one variant before the close check runs. As a side effect, a nested comma-free `{}` now yields one empty variant instead of zero, which also corrects `expand_nested` dropping text that fo… <!-- robobun:evidence:end -->
After #34856 demotes the outer braces to text, this input dispatches to expand_flat (brace_count never exceeds 1), not the expand_nested path the nested-empty-variant block guards. Move it to the block that tests the behaviour it actually exercises; p{q,{r,}{s,}}t in the original block still covers sibling trailing-empty groups through expand_nested.
…-5.2 literal `{}` (#34882)
Fixes `test/js/bun/shell/brace.test.ts` which is red on main (four hard
failures on every lane, e.g. [build
76627](https://buildkite.com/bun/bun/builds/76627)).
## Cause
#34856 and #34865 were developed in parallel and merged three minutes
apart. #34856 changed the brace lexer so a `{...}` group with no
top-level comma is demoted to literal text (bash 5.2 semantics) before
the parser sees it. #34865 added a `nested with empty variant`
`test.each` block whose last four entries still expect the pre-#34856
behaviour. #34865's own commit message notes #34856 was addressing the
literal-`{}` question separately; the rebase was textually clean so
neither PR's CI caught the semantic overlap.
```
$.braces > nested with empty variant > {{a,}{b,}} expected ["ab","a","b",""] got ["{ab}","{a}","{b}","{}"]
$.braces > nested with empty variant > {x,a{}b} expected ["x","ab"] got ["x","a{}b"]
$.braces > nested with empty variant > {a,b{}}c expected ["ac","bc"] got ["ac","b{}c"]
$.braces > nested with empty variant > {x,{}y} expected ["x","y"] got ["x","{}y"]
```
bash 5.2.37 agrees with the new output in every case:
```
$ bash -c 'printf "[%s] " {{a,}{b,}}' # [{ab}] [{a}] [{b}] [{}]
$ bash -c 'printf "[%s] " {x,a{}b}' # [x] [a{}b]
$ bash -c 'printf "[%s] " {a,b{}}c' # [ac] [b{}c]
$ bash -c 'printf "[%s] " {x,{}y}' # [x] [{}y]
```
## Fix
All four entries are removed from the `nested with empty variant` block:
after #34856 none of them reach the `parse_expansion`/`expand_nested`
path that block guards, because the lexer demotes their comma-less group
to text first. The remaining ten entries in that block (notably
`p{q,{r,}{s,}}t`, which still routes through `expand_nested` with
sibling trailing-empty groups) continue to cover #34865's fix.
- `{{a,}{b,}}` is moved to the `comma-less brace group is literal (bash
5.2)` block with the bash-correct expectation
`["{ab}","{a}","{b}","{}"]`. That block also runs each case through the
actual shell in a subprocess, so the pattern now gets both `$.braces()`
and end-to-end shell coverage.
- `{x,a{}b}`, `{a,b{}}c`, `{x,{}y}` are dropped together with their
now-inaccurate comment (which describes a 0-variant `{}` reaching the
parser, a state that no longer exists). Their literal-`{}` behaviour is
already asserted by `p{q{},r}s` / `{a,b{}}z` / `{a,{}}z` in that same
block.
No `src/` change: the runtime behaviour is correct (matches bash); only
the stale expectations are realigned.
## Verification
```
bun bd test test/js/bun/shell/brace.test.ts
# before: 39 pass, 4 fail
# after: 40 pass, 0 fail
```
<!-- robobun:evidence:begin -->
---
**[stamp-90s]** gate passed · iteration 0 · 1 files touched
<details><summary>passes on PR (with fix)</summary>
```console
Test-only change.
Debug/ASAN (expected pass):
$ bun bd test 'test/js/bun/shell/brace.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/js/bun/shell/brace.test.ts
info: syncing channel updates for nightly-2026-05-06-x86_64-unknown-linux-gnu
info: latest update on 2026-05-06 for version 1.97.0-nightly (e95e73209 2026-05-05)
info: component rust-src is up to date
info: checking for self-update (current version: 1.29.0)
bun test v1.4.0 (0ecd8eb)
test/js/bun/shell/brace.test.ts:
(pass) $.braces > no-op [1.89ms]
(pass) $.braces > 2 [1.81ms]
(pass) $.braces > 3 [1.77ms]
(pass) $.braces > nested [1.63ms]
(pass) $.braces > nested 2 [2.01ms]
(pass) $.braces > nested sibling product [1.71ms]
(pass) $.braces > nested sibling product with surrounding text [1.44ms]
(pass) $.braces > nested sibling product mixed with variants [1.97ms]
(pass) $.braces > nested sibling product triple [2.04ms]
(pass) $.braces > nested with empty variant > {x,a{,}b} [1.46ms]
(pass) $.braces > nested with empty variant > {x,{a,}}z [0.65ms]
(pass) $.braces > nested with empty variant > {x,{,a}}z [0.48ms]
(pass) $.braces > nested with empty variant > {x,{,}}z [0.47ms]
(pass) $.braces > nested with empty variant > a{b,c{d,}}e [0.52ms]
(pass) $.braces > nested with empty variant > a{b,c{,d}}e [0.48ms]
(pass) $.braces > nested with empty variant > {x,{a,,b}} [0.74ms]
(pass) $.braces > nested with empty variant > {x,{a,b,}} [0.47ms]
(pass) $.braces > nested with empty variant > {{a,},x} [0.49ms]
(pass) $.braces > nested with empty variant > p{q,{r,}{s,}}t [0.51ms]
(pass) $.braces > very deeply nested [2.81ms]
(pass) $.braces > empty string [2.88ms]
(pass) $.braces > unicode [2.43ms]
(pass) brace + glob composition > src/*.{ts,tsx} globs after brace expansion [62.79ms]
(pass) brace + glob composition > {src,lib}/*.ts composes a brace prefix with a glob [27.96ms]
(pass) brace + glob composition > an interpolated comma inside a brace group is one literal branch [22.66ms]
(pass) $.braces input bounds > rejects a word with an excessive number of br
... (truncated)
Exit: 0
```
</details>
<details><summary>diff hotspot</summary>
```
test/js/bun/shell/brace.test.ts | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
```
</details>
**gate history** · 1 passed · 0 rejected · iteration 0
<details><summary>evidence per changed file</summary>
```
file reads edits tests
test/js/bun/shell/brace.test.ts 2 3 0
```
</details>
<!-- robobun:evidence:end -->
What
A literal
{}(or any{...}with no comma) nested inside an expanding brace group silently truncated the word at that point, dropping the{}and everything after it:So a word like
rm -rf {old,tmp{}}/cachehanded a truncated path to the external command with exit 0 and no diagnostic.A related pre-existing bug: a word that tripped the brace-expansion hint (
{,}, and,all present) but produced zero brace groups after lexing crashed withindex out of bounds: the len is 0 but the index is 0:Cause
In bash a
{...}group is only a brace expansion if it contains an unquoted comma at its own depth; otherwise it is literal text. Bun's brace lexer tokenized every balanced{/}pair asOpen/Closeregardless. A nested{}therefore parsed as anExpansionwith zero variants, andexpand_nested'sfor j in 0..variants_lenloop did not run, returning without ever bubbling up to the parent group, so the parent's tail was never emitted.For the panic:
do_brace_expandpreallocatesouttocalculate_expanded_amount()slots and then callsexpand, which unconditionally writes toout[0]. When every brace rolled back (unbalanced) the count was 0 and the write was out of bounds.Fix
tokenize_implnow trackshas_commaper open brace. On}, if the group saw no comma at its own depth theOpentoken is demoted back to a literal{and the}is emitted as text; nested groups that did have a comma are left intact. The expander therefore never encounters a zero-variant expansion, and every output now matches bash 5.2.flatten_tokensis rewritten to compact in place instead of callingVec::removeper merge, since the rollback can now produce long runs of adjacentTexttokens (the old loop went quadratic on"{".repeat(n) + "}".repeat(n)).do_brace_expandnow emits the word unchanged when the expansion count is zero instead of indexing into an empty slice.The existing
{{a,b}{c,d}}and deeply-nested test assertions are updated to the bash output they now match (the outer group has no comma of its own, so bash keeps the literal{/}).Verification
28 pass with the fix; 15 of them fail on current main (9 truncation, 4 comma-less-is-literal, 1 updated bounds test, 1 subprocess crash).
no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts