Repository navigation
shell: keep trailing empty variant when parsing nested brace groups - #34865
Conversation
Parser::parse_expansion's outer loop guard consumed the closing brace
after a trailing comma, so the pending empty variant was never pushed.
calculate_expanded_amount still counted it, leaving the last output
slot blank: {x,a{,}b} expanded to ["x","ab",""] instead of
["x","ab","ab"]. The non-nested expand_flat path was already
correct.
Make the inner loop the sole consumer of Close/Eof so every variant,
including the trailing empty, is pushed before the outer loop exits.
|
Warning Review limit reached
Next review available in: 22 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)
Comment |
|
Reproduced with Self-review surfaced one concern: the loop restructure also changes nested comma-free |
|
Updated 8:26 PM PT - Jul 20th, 2026
✅ @robobun, your commit 0261e126ac14838c748d5335e8b38936c35ab915 passed in 🧪 To try this PR locally: bunx bun-pr 34865That installs a local version of the PR into your bun-34865 --bun |
![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 control-flow fix in parse_expansion with thorough coverage.
What was reviewed:
- Traced the new loop against
{a,},{{d,e}{g,h}},{123,{456,789},abc}, and the deeply-nested test — all produce the same variant lists as before except the trailing-empty case, which now agrees withcalculate_expanded_amount. - Checked the
None => break truearm:match_any([Close, Eof])runs first, soparse_atomcannot see Eof here — the arm is defensive only, no behavior change vs. the oldNone => break. - Checked
{}inside a nested group: now yields one empty variant instead of zero;expand_nestedhandlesMany([])via thestart >= many_lenbubble-up, and the slot count fromcalculate_expanded_amountis unchanged. - Verified each new test vector against bash's expansion; robobun confirmed 8/11 fail on the released binary.
Extended reasoning...
Overview
The PR fixes a bug in src/shell_parser/braces.rs::Parser::parse_expansion where a nested brace group ending in a trailing comma ({a,}) dropped the final empty variant. The old outer while !self.match_any(&[Close, Eof]) guard consumed the Close token immediately after a Comma broke the inner loop, so the pending empty variant was never pushed. Because calculate_expanded_amount already counted that variant, the output array was oversized and the last slot surfaced as an empty string. The fix moves Close/Eof consumption exclusively into the inner loop and makes the outer loop unconditional, so every iteration pushes exactly one variant before checking close. A test.each block with 11 cases (trailing/leading/middle empties, sibling products, prefix/suffix text) is added to test/js/bun/shell/brace.test.ts.
Security risks
None. This is a pure control-flow restructure inside a bump-arena parser for shell brace-expansion syntax. No new allocations, no pointer arithmetic changes, no untrusted-length handling. The existing MAX_BRACE_GROUPS guard and check_brace_group_count still apply unchanged.
Level of scrutiny
Medium — parser logic, but the change is a ~15-line loop restructure whose invariant ("each outer iteration pushes one variant, terminates on Close/Eof") is easy to verify by hand. I traced the new loop against every existing test in the file (nested, nested 2, nested sibling product ×4, very deeply nested) and confirmed the variant sequences are identical. The only semantic delta beyond the fix is that a bare {} inside a nested group now parses to one empty variant instead of zero; expand_nested already handles GroupAtoms::Many with length 0 via the start >= many_len bubble-up path, and calculate_expanded_amount already counted it as 1, so this is a strict improvement in parser/counter agreement rather than a regression.
Other factors
- The
None => break truearm (formerlyNone => breakwithcloseleft false) is unreachable in practice:parse_atomreturnsNoneonly onEof, and the inner loop'smatch_any(&[Close, Eof])check runs first. Treating it asclose = trueis the more defensive choice and avoids a spurious extra outer iteration if it ever were reached. - Test vectors were spot-checked against bash's actual expansion; all 11 match.
- robobun confirmed
USE_SYSTEM_BUN=1fails 8 of the 11 new cases (the 3 that pass are the leading/middle-empty controls the PR says already worked) and the debug build passes all — the tests are load-bearing. - Tests are added to the existing
brace.test.tsfile per repo convention, usetest.eachfor the matrix, and assert exact arrays withtoEqual.
There was a problem hiding this comment.
LGTM — small, well-scoped control-flow fix that brings parse_expansion into agreement with calculate_expanded_amount.
What was reviewed
- Traced the new
loop { let close = loop { ... } }structure against{a,},{},{{a,},x}, and{{a,}{b,}}— each now pushes exactly one variant per outer iteration, including the trailing empty. - Checked that
{}(which previously produced 0 variants, now 1 empty variant) is only reachable on the nested path and yields identical output throughexpand_nested— no observable behavior change there. - Confirmed the
parse_atom()? => Nonearm is effectively unreachable (Eof is consumed bymatch_anyfirst) andbreak truematches the old net effect. - Test coverage hits trailing/leading/middle empties, sibling products, and prefix/suffix text; robobun confirmed 8/11 fail on system Bun and all pass on the debug build.
Extended reasoning...
Overview
Two files: a ~15-line control-flow reshape in src/shell_parser/braces.rs::Parser::parse_expansion, and an 11-case test.each block added to test/js/bun/shell/brace.test.ts. The bug was that the outer while !self.match_any(&[Close, Eof]) guard consumed Close before the trailing empty variant after a comma could be pushed, so nested {a,} produced one variant while calculate_expanded_amount sized the output array for two. The fix removes the outer guard and makes the inner loop the sole consumer of Close/Eof, returning a close: bool from a labeled-break loop so every outer iteration unconditionally pushes exactly one variant.
Security risks
None. This is pure in-memory string expansion over already-tokenized input. The existing MAX_BRACE_GROUPS / MAX_NESTED_BRACES DoS bounds are untouched, and the change cannot cause the parser to consume fewer tokens than before (every path still advances via match_any/r#match/parse_atom), so no new infinite-loop surface.
Level of scrutiny
Medium — it's a parser correctness fix with recursive downstream consumers (expand_nested), so I hand-traced several inputs including the new edge case where an immediately-closing group ({} in nested position) now yields one empty Many([]) variant instead of zero variants. That variant is a zero-length Many, which expand_nested handles via the start >= many_len bubble-up path, producing identical output to before. The None => break true arm (previously None => break with close=false) is unreachable in practice because match_any([Close, Eof]) runs first, and even if reached the net behavior is identical (old code would push the variant then exit on the next outer-loop guard).
Other factors
Tests are placed in the existing brace.test.ts alongside related coverage, use test.each per house style, and cover the full variant matrix (trailing/leading/middle empties × leading/trailing/interior nesting × with/without surrounding text × sibling products). robobun independently confirmed the tests fail on the released binary and pass with the fix. No prior human review comments to address. The comment above the inner loop is concise and states the invariant. This is exactly the kind of localized bugfix that doesn't need a human gate.
The loop restructure also makes a nested comma-free {} parse to one
empty variant instead of zero, so expand_nested no longer drops the
text after it. Pin that with tests.
There was a problem hiding this comment.
LGTM — focused parser fix that makes the nested-brace path agree with calculate_expanded_amount and bash.
What was reviewed
- Traced
parse_expansionold vs new for{a,},{},{a,,b}— inner loop is now the sole consumer ofClose/Eof, so every outer iteration pushes exactly one variant; no infinite-loop risk (Eof/Close/Comma/parse_atom→Noneall terminate). - Confirmed the
{}→1-empty-variant change is safe inexpand_nested(emptyManyhits thestart >= many_lenbubble-up path, so trailing text is no longer dropped). - Verified
calculate_expanded_amountalready counted the trailing empty and nested{}as 1, so out-array sizing now matches what the expander writes. - Test matrix covers trailing/leading/middle empties, sibling products of trailing-empty groups, and nested comma-free
{}; robobun confirmed 8/11 fail on system bun and all pass on the debug build.
Extended reasoning...
Overview
Rewrites the outer/inner loop structure of Parser::parse_expansion in src/shell_parser/braces.rs so that only the inner loop consumes Close/Eof. Previously the outer while !self.match_any(&[Close, Eof]) guard would eat the Close immediately after a trailing comma, dropping the pending empty variant. The new shape guarantees each outer iteration pushes exactly one variant before checking close. A secondary effect: a nested comma-free {} now yields one empty variant instead of zero, which fixes expand_nested returning early and dropping text after it. 14 new test.each cases in test/js/bun/shell/brace.test.ts pin the behavior.
Security risks
None. Pure parser control-flow reshaping over already-lexed tokens; no new allocation sites, no unsafe blocks touched, no user-controlled sizes introduced. The existing MAX_BRACE_GROUPS bound and check_brace_group_count still gate recursion depth.
Level of scrutiny
Medium — this is a recursive-descent parser feeding expand_nested (which uses raw-pointer backrefs), so I traced termination and downstream handling of the new empty-Many variant shape. Termination: every inner-loop arm either consumes a token (match_any/r#match/parse_atom→advance) or breaks; Eof is idempotent under advance and yields break true. Downstream: an empty Many([]) group in expand_nested takes the start >= many_len branch and bubbles up correctly, so no new 0-length-slice hazards. calculate_expanded_amount was already counting these variants (Close contributes acc + segment_product with segment_product initialized to 1), so the out-array size and the number of writes now agree — the original symptom (last slot left empty) is eliminated at the root.
Other factors
- The flat (non-nested) path via
build_expansion_tablealready handled trailing empties and is untouched. parse_expansionand theasttypes have no consumers outsidebraces.rs.- robobun confirmed the fails-before/passes-after contract (
USE_SYSTEM_BUN=1fails 8 of the new cases). - Tests use
test.eachwith exacttoEqualassertions against bash-matching outputs, placed alongside existing$.bracescoverage.
…-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 -->
2 participants
Problem
When a nested brace group ends with an empty alternative (
{a,}), the nested-expansion parser drops that variant.calculate_expanded_amountstill 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.Only the nested (
contains_nested) path is affected; the flat path handles trailing empties correctly viabuild_expansion_table. Leading ({,a}) and middle ({a,,b}) empties already work.Cause
In
Parser::parse_expansion, after the inner loop breaks onCommaand pushes a variant, the outerwhile !self.match_any(&[Close, Eof])peeksClose, 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 withcalculate_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 variantsexpand_nestedreturns early without bubbling up, so any text after the{}in that branch was dropped:After this change a nested
{}parses to one empty variant, which matches whatcalculate_expanded_amountalready computed and whatexpand_flatalready does fora{}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.eachblock intest/js/bun/shell/brace.test.tscovering 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, andlexsuites remain green.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
root cause · written by the author bot
The outer loop guard in
parse_expansionconsumed theClosetoken immediately after a trailing comma in a nested brace group, so the pending empty variant was never pushed even thoughcalculate_expanded_amounthad already counted it, leaving the last output slot unwritten. The fix restructures the loop so that only the inner loop consumesCloseorEof, 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 correctsexpand_nesteddropping text that fo…