shell: glob brace-expanded patterns instead of dropping the glob - #30999
Conversation
|
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 (3)
WalkthroughShell expansion state machine updated to compose brace expansion with glob expansion. The ChangesShell brace-glob expansion composition
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 8:23 PM PT - May 18th, 2026
✅ @robobun, your commit 97f5e8288d187b2028b96b44fd93b796a28d1850 passed in 🧪 To try this PR locally: bunx bun-pr 30999That installs a local version of the PR into your bun-30999 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@robobun fix review comments |
|
✅ Review comments addressed:
CI on 97f5e82 (#55939): every test lane that ran is green — all three Windows lanes, |
There was a problem hiding this comment.
LGTM — both earlier review comments addressed in 45bd924 (Windows separator normalization + word-level assertions).
Extended reasoning...
Overview
This PR fixes a dead-code bug in the Rust shell port's word-expansion state machine (src/runtime/shell/states/Expansion.rs). Previously do_brace_expand ended with if has_glob_expansion() { Done } else { Done } — both arms identical — so a word combining brace and glob syntax (src/*.{ts,tsx}, {src,lib}/*.ts) was brace-expanded but never globbed. The fix: when has_glob_expansion(), keep current_out (the original pattern) and set state to Glob; the BraceExpand arm in next() then dispatches transition_to_glob_state — the exact same call the non-brace glob path already makes a few lines below. Two regression tests added in test/js/bun/shell/brace.test.ts using temp fixtures.
Security risks
None. This is pure shell-word expansion bringing the Rust port to parity with the existing Zig reference (which already behaves this way in released Bun). No new input surface, no auth/crypto/permissions, no untrusted-data handling beyond what the glob walker already does.
Level of scrutiny
Moderate — shell expansion affects how Bun.$ / bun run build argv. But the change is ~20 lines, mechanically replaces an admitted-TODO no-op branch with the documented Zig behavior, and reuses the existing transition_to_glob_state entry point unchanged. The state-machine flow is straightforward: do_brace_expand sets state = Glob, caller checks for that and returns transition_to_glob_state(...) (which itself returns Yield::suspended() or, on walker-init failure, sets state = Err and returns Yield::Next — both handled correctly by the existing trampoline). current_out.clear() correctly moves to the non-glob branch only, since the glob path needs the original pattern.
Other factors
I previously left two inline comments on this PR (Windows backslash separators in test assertions; vacuous substring toContain). Both were addressed in 45bd924 — tests now .replaceAll("\\\\", "/") and assert against out.split(" ") — and both threads are marked resolved. The bug-hunting system found no new issues on the updated revision. No CODEOWNERS cover these paths. Output semantics (literal brace variants followed by glob matches) match the Zig reference per the PR description, so no new design decision is being introduced.
The libuv-derived helper called assert() without including <cassert>. This compiled by accident via transitive includes in some unified-source groupings but fails under the current debug build. Switch to WTF's ASSERT (already available via root.h), matching every other assertion in this directory.
There was a problem hiding this comment.
Both earlier review comments are addressed (path-separator normalization + word-level assertions). The state-machine change looks correct and mirrors the Zig spec, but I'll leave this for a human since it changes user-visible shell expansion semantics and Build #55885 is currently red.
Extended reasoning...
Overview
This PR fixes brace+glob composition in the Rust shell interpreter port. do_brace_expand previously had a dead if has_glob_expansion() { Done } else { Done } branch and unconditionally cleared current_out; the fix keeps current_out and transitions to ExpansionState::Glob so the BraceExpand arm in next() can dispatch transition_to_glob_state — exactly the same call the plain-glob path makes a few lines below. There is also a cosmetic assert → ASSERT swap in wtf-bindings.cpp and two new regression tests in brace.test.ts.
Prior feedback
I had flagged (1) Windows path-separator breakage in the new tests and (2) a vacuous substring assertion. Both were fixed in 45bd924 via .replaceAll("\\\\", "/") + out.split(" ") with array toContain, and the inline threads are resolved. No new issues found on re-review.
Security risks
None. No untrusted input handling, auth, or crypto is touched; the glob walker and brace expander are pre-existing and only the state transition between them changed.
Level of scrutiny
Moderate. The diff is small and the control flow is easy to trace (the Err path from MAX_BRACE_EXPANSIONS still falls through to continue → break correctly), but this is a user-visible semantics change in Bun.$ / --shell=bun / Windows bun run. A human familiar with the Zig→Rust shell port should confirm the composed output (literal brace variants plus glob matches) is the intended released-Bun behavior to preserve.
Other factors
Buildkite #55885 on 45bd924 is currently reporting failures, and the head commit e36ec3a (the ASSERT swap) hasn't been confirmed green in the thread. Worth checking whether those failures are related before merge.
There was a problem hiding this comment.
Both of my earlier comments are addressed and I didn't find anything new — the state-machine fix looks correct and matches the Zig spec, but since this changes user-facing shell expansion semantics it's worth a quick human sanity check before merge.
Extended reasoning...
Overview
Three files: (1) src/runtime/shell/states/Expansion.rs — the core fix: do_brace_expand previously ended in a dead if has_glob_expansion() { Done } else { Done } and unconditionally cleared current_out; it now sets state = Glob and preserves current_out when the atom also has a glob, and the BraceExpand arm in next() dispatches transition_to_glob_state (mirroring the normal glob path). (2) test/js/bun/shell/brace.test.ts — two new regression tests with temp fixtures. (3) src/jsc/bindings/wtf-bindings.cpp — trivial assert → WTF ASSERT swap.
Security risks
None identified. No new untrusted-input parsing, no auth/crypto/permissions. The glob walk itself is unchanged — this just re-enters the existing glob path that was already reachable for non-brace patterns.
Level of scrutiny
Moderate. The diff is small (~15 logic lines) and replaces obviously-dead code with the documented Zig behavior, but it alters the shell interpreter's expansion state machine and changes user-visible Bun.$ output for brace+glob words (now emits literal brace variants and glob matches, which the PR states is byte-identical to released/Zig Bun). That semantic — and the interaction with on_glob_walk_done's no-match error path now that out.buf is pre-populated with brace variants — is the kind of thing a human familiar with the Zig spec should glance at.
Other factors
I previously flagged two test issues (Windows path separators, vacuous substring assertion); both were fixed in 45bd924 and the inline threads are resolved. The bug-hunting pass on the latest revision found nothing. A maintainer (alii) is already engaged on the PR. No CODEOWNERS cover these paths. I traced the new control flow: Err from the brace-cap still breaks correctly, Done still breaks, and Glob correctly bypasses the suspended-arm by dispatching directly — so the change reads as correct to me, just not trivial enough to rubber-stamp.
3 participants
A shell word combining brace + glob (
src/*.{ts,tsx},{src,lib}/*.ts,rm -rf {dist,build}/*.js) was brace-expanded but the resulting*patterns were never globbed. Silent, exit 0. AffectsBun.$,--shell=bun, bunfigshell="bun", and Windowsbun run.do_brace_expandended withme.state = if has_glob_expansion() { Done } else { Done }— both armsDone, the glob check a dead no-op (an in-source TODO admitted it). It also clearedcurrent_out, which Zig keeps. Zig re-enters the.globstate after brace expansion, so the original pattern (e.g.src/*.{ts,tsx}) is globbed (the glob walker brace-expands and globs it) and its matches are appended after the literal brace variants.When
has_glob_expansion(), keepcurrent_outand set state toGlob; theBraceExpandarm then dispatchestransition_to_glob_state(the same call the normal glob path makes). Output is now byte-identical to released Bun:echo src/*.{ts,tsx}→src/*.ts src/*.tsx src/app.ts src/util.tsx. Regression test inbrace.test.tswith a temp fixture.