Repository navigation
Conversation
Bun.Glob.scan() returned an empty result for brace patterns where an
alternative contains a path separator (e.g. `svc/{src/env.ts,env.ts}`),
even though match() handled the same pattern correctly.
The scanner models a pattern as one component per directory level and
split the pattern on every separator, including separators inside a
brace group. `svc/{src/env.ts,env.ts}` was therefore cut into the bogus
components `svc`, `{src` and `env.ts,env.ts}` and matched nothing. A
brace alternative containing a separator spans multiple levels, which
the one-component-per-level model cannot represent.
Expand such patterns into separate brace-free patterns (the same set of
strings match() evaluates the braces against) and walk each in turn,
rebuilding the components per expansion and deduping results through the
existing matched_paths set. Expansion honors backslash escapes, `[...]`
bracket classes and nested braces, and is bounded so adversarial
patterns cannot blow up. Patterns with no separator inside braces keep
the existing single-pass behavior.
The expansion drives the shared walker Iterator, so scan, scanSync and
the other scan entry points (shell globbing, workspaces, --filter) all
benefit.
|
Updated 3:45 PM PT - Jun 22nd, 2026
❌ @robobun, your commit 7ee1584 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32599That installs a local version of the PR into your bun-32599 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough
ChangesGlobWalker brace expansion for path-separator alternatives
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 2
🤖 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/glob/GlobWalker.rs`:
- Around line 2225-2248: In the brace_group_spans_separator function, the
backslash escape handling at the match arm for b'\\' is being processed before
checking if the backslash is a native path separator. On Windows, backslash is a
native separator and should not be unconditionally treated as an escape
character. Modify the escape-skipping logic to either only apply on non-Windows
platforms or check if the backslash is a native separator first before
incrementing i by 2 to skip it. Ensure that on Windows, a backslash can still be
classified as a separator through the bun_core::path_sep::is_sep_native check.
- Around line 585-606: The absolute-literal fast path in the init() method can
set IterState::Matched(path) without inserting the path into matched_paths,
causing duplicate paths to be emitted when processing overlapping brace
expansions like /tmp/{a/b,a/b}. In the iteration logic where IterState::Matched
paths are returned to consumers (likely in the walk() method or similar
iteration handler), add a check against matched_paths.contains_key() before
returning the path. If the path is not already recorded in matched_paths, insert
it first, ensuring that all matched paths are properly deduplicated across
expansion boundaries regardless of whether they came from the fast path or
regular path in init().
🪄 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: dcc8fd6c-05cf-4c94-a31a-cf5807cb7007
📒 Files selected for processing (2)
src/glob/GlobWalker.rstest/js/bun/glob/scan.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…as a separator
Two fixes from review of the brace-expansion change:
- The no-special-syntax fast path in `Iterator::init` set `IterState::Matched`
without recording the path in `matched_paths`. Because the iterator now
re-runs `init` per brace expansion, an absolute pattern whose alternatives
overlap (e.g. `/x/{a,a}` or `/x/{a,*}`) emitted the same file twice, and
`walk` (which reads `matched_paths`) dropped fast-path matches entirely. Route
the fast path through a new `record_full_match` helper that checks and inserts
`matched_paths`, mirroring `prepare_matched_path`'s NUL handling so dedupe is
exact in both SENTINEL modes. Absolute literals now surface from `scan` too.
- `brace_group_spans_separator` and `find_first_brace_group` consumed `\` as an
escape on every platform, but `build_pattern_components` treats `\` as a path
separator on Windows (`is_sep_native`). A pattern like `svc/{src\env.ts,b}`
therefore skipped expansion yet still got split on the `\`. Gate the escape
handling to non-Windows so a `\` inside a brace group triggers expansion on
Windows, matching the splitter.
Adds scan/scanSync coverage for single-alternative wildcard groups spanning a
separator (the `{*/*}` shape) and for absolute-literal expansion surfacing/dedupe.
|
Notes on the two automated findings above: #24000 ( #25789 takes the same approach (expand brace groups containing a separator before walking), but it targets |
The earlier dedupe change (record_full_match) routed Iterator::init's
no-special-syntax fast path through matched_paths. That surfaced absolute
literal matches scan/scanSync had always dropped, including directories that
onlyFiles excludes: the `../.` matrix case started returning `[""]` instead of
`[]`, diverging from the fast-glob oracle. Revert it and restore the original
fast path. The double-emit it guarded against only reached next() consumers for
absolute brace patterns, which shell (its own brace expansion) and
workspace/--filter (relative patterns) do not produce.
Separately, walking each brace expansion re-runs init with that expansion's own
root, so an absolute alternative with a missing prefix (e.g.
`{/missing/*.ts,ok/*.ts}`) threw ENOENT and aborted the whole scan, dependent on
alternative order. Brace alternatives are an unordered set, so tolerate
ENOENT/ENOTDIR from the per-expansion root open (yield nothing for that
alternative) when brace_expansions is non-empty; single patterns keep the
original error so a bad cwd still surfaces.
Keeps the Windows backslash-in-braces fix. Adds a scan test for the missing-root
case and drops the now-invalid absolute-surfacing test.
…pans
`brace_group_spans_separator` gated its separator check on `!in_brackets`, but
`build_pattern_components` splits on a path separator regardless of bracket
state. For a degenerate pattern like `{x,a[/]b}` the detector returned false, so
expansion was skipped and the splitter cut the group into `{x,a[` / `]b}`,
losing the valid `x` alternative. Drop the `!in_brackets` guard from the
separator arm (keeping it on the brace/comma arms) so the detector mirrors the
splitter and the group's other alternatives expand instead of being mis-split.
…caped separators
Two follow-ups from review of the brace-expansion change:
- The ENOENT/ENOTDIR tolerance for a missing per-expansion root also fired for
relative brace patterns, whose root is the shared user `cwd`, so
`{a/b,c/d}` with a non-existent `cwd` silently returned `[]` instead of
throwing. Gate it on `was_absolute` so only an absolute alternative's own
literal prefix is tolerated; a bad `cwd` (and single patterns) still surface
the error.
- `brace_group_spans_separator` consumed a backslash-escaped separator as an
escape, but `build_pattern_components` splits on a separator regardless of a
preceding backslash, so `svc/{src\/env.ts,env.ts}` skipped expansion and got
mis-split. A new `escape_advance` helper stops the backslash arm from
consuming a following separator (detector and both `find_first_brace_group`
loops), so the detector mirrors the splitter and sibling alternatives expand.
Adds a scan test that a missing cwd still throws for a relative brace pattern,
and unit tests for the escaped-separator case.
When a brace group expands to exactly one pattern (a single-alternative group
such as `{src/env.ts}`, or an unclosed group pushed verbatim), it is equivalent
to the brace-free pattern, so set it as `pattern` and fall through to the
single-pattern path instead of the per-expansion machinery. That makes `{X}`
byte-for-byte transparent to `X`: it shares the single-pattern fast path,
`onlyFiles` filtering, and error handling. Previously the 1-element
`brace_expansions` vec tripped the missing-root tolerance, so a single absolute
alternative with a missing root returned `[]` while the bare pattern threw.
`brace_expansions` is now only populated for genuine multi-alternative groups.
Adds a scan test that a single-alternative group throws on a missing absolute
root exactly like its bare form.
expand_braces filtered empty results at every recursion level and only
re-injected the empty case when a sub-expansion was entirely empty. An empty
alternative (`{X,}`) filling a whole sub-slot therefore lost its empty branch
even when an outer prefix/suffix would have made it a valid non-empty pattern:
`{a/b,c}{d,}` dropped `a/b` and `c`, and `x{a/b,{c,}}` dropped `x`, both of
which match() accepts.
Keep empty pieces through the recursion (an empty input now yields one empty
string, so the per-level empty-vec fixups are gone) and drop patterns that stay
empty once, in expand_braces. Adds unit tests and a scan case for a trailing
optional group.
There was a problem hiding this comment.
All my earlier findings have been addressed and I have nothing further, but this reworks the glob walker's iterator state machine and root-open error semantics across scan/shell/workspaces/--filter, so it's worth a human pass before merge.
Extended reasoning...
Overview
This PR adds brace-expansion support to src/glob/GlobWalker.rs for patterns where a brace alternative contains a path separator (e.g. svc/{src/env.ts,env.ts}). It introduces ~200 lines of new parsing/expansion logic (brace_group_spans_separator, find_first_brace_group, expand_braces), two new walker fields (brace_expansions, expansion_cursor), and modifies Iterator::init, Iterator::next, and walk() to re-run init per expansion and advance through expansions on work-stack exhaustion. It also changes root-open error handling to tolerate ENOENT/ENOTDIR for absolute brace alternatives. Tests added in both Rust unit tests and test/js/bun/glob/scan.test.ts.
Security risks
None identified. The new parser is bounded (10k expansions, depth 32) so adversarial patterns can't blow up memory or stack. No new filesystem operations beyond what the existing walker already does; no auth/crypto/permissions surface.
Level of scrutiny
High. This is core path-handling code shared by Bun.Glob.scan/scanSync, shell globbing, workspace resolution, and --filter. The change modifies the iterator state machine (re-running init() per expansion, which was previously single-shot) and alters error semantics (ENOENT/ENOTDIR now swallowed for absolute brace alternatives). Over the course of review I and CodeRabbit flagged seven distinct correctness issues across five fix-up commits — Windows \ handling, absolute-literal fast-path dedup (later reverted due to a snapshot regression), missing-root error propagation, the was_absolute gate, single-expansion transparency, escaped-separator detection, and empty-branch propagation. All have been addressed or consciously deferred (the unclosed-[ edge case), but the density of subtle interactions argues for a human reviewer to confirm the final shape.
Other factors
- All prior inline comments from me are resolved; the bug-hunting pass on the current head found nothing new.
- One nit (unclosed
[inside a brace alternative losing siblings) was intentionally left as-is with a reasonable justification — malformed input, heavier fix, documented behavior. - The visible CI status comment references an older commit (fb3cb0f) with failures; status for the current head (7ee1584) isn't shown in the thread.
- The dedup-via-
matched_pathsfix for the absolute-literal fast path was reverted after it regressed an existing snapshot, leaving a known (narrow, documented) double-emit case for absolute brace patterns vianext()consumers — a deliberate trade-off a human should be aware of.
|
CI red on 7ee1584 is unrelated flake, not this diff. The glob tests (
None touch the glob walker. For non-brace patterns (what workspace resolution and |
|
Independently reproduced and landed on the same fix on current main (332f744): |
|
Triage note: #32596 was closed by the reporter after working around it downstream (archgate/cli#475), not by a change in Bun. The bug still reproduces on current main (f426a8e): |
|
This bug was reported again with a brace group at the start of the pattern. The pattern The branch no longer merges with main.
A rebase is needed before this can land. |
This branch has not been deployed
1 participant
Problem
Bun.Glob.scan()/scanSync()return an empty result for brace patterns where an alternative contains a path separator, e.g.svc/{src/env.ts,env.ts}.match()handles the same pattern correctly, and so do Node'sfs.globand fast-glob, so the scanner is inconsistent with both.Fixes #32596.
Cause
The scanner (
src/glob/GlobWalker.rs) models a pattern as one component per directory level and splits the pattern on every path separator, including separators inside a brace group.svc/{src/env.ts,env.ts}is therefore cut into the componentssvc,{srcandenv.ts,env.ts}, which match no real directory layout, so the walk yields nothing. A brace alternative containing a separator spans multiple levels with different depths (src/env.tsis two levels,env.tsis one), which the one-component-per-level model cannot represent.match()is unaffected because it evaluates braces inline over the whole string (src/glob/matcher.rs).Fix
Expand a pattern whose braces contain a path separator into separate brace-free patterns, the same set of strings
match()evaluates the braces against, and walk each in turn. The walker rebuilds its components per expansion and dedupes results through the existingmatched_pathsset, so overlapping alternatives collapse to one result. Expansion honors backslash escapes,[...]bracket classes and nested braces, and is bounded (10k patterns, depth 32) so adversarial patterns cannot blow up. Patterns with no separator inside braces ({a,b}/c,src/{x,y}.ts) keep the existing single-pass behavior unchanged.The expansion drives the shared walker
Iterator, soscan,scanSync, and the other scan entry points (shell globbing, workspaces,--filter) all get the fix.Verification
Added regression coverage in
test/js/bun/glob/scan.test.ts(bothscanandscanSync): the reported pattern, alternatives of differing depth, a globstar inside a brace alternative, dedup of overlapping alternatives, and a scan/match agreement check. Unit tests for the expansion and detection helpers live insrc/glob/GlobWalker.rs.All 11 fail on
main(each returns[]) and pass with this change.Note
#25789 took the same approach earlier, but it patches
src/glob/GlobWalker.zig, the original implementation that has since been ported to Rust and is no longer compiled, and it currently has merge conflicts. This change implements the same idea against the live Rust walker and covers the shared iterator that every scan path uses.