Repository navigation
Conversation
A brace group only expands in bash, picomatch, and minimatch when it
contains at least one unescaped top-level comma. Bun's matcher expanded
every {...} regardless, so {a} matched "a" instead of "{a}", {} matched
the empty string, and {1..3} matched "1..3".
Fix by scanning the pattern once in r#match and escaping each { / }
belonging to a group with no top-level comma (or no closing brace at
all) to \\{ / \\}; the matching loop then handles them as ordinary
literals with no changes. Bun.Glob runs the scan once at construction
and stores the result so match() stays on the fast path.
|
Updated 4:14 AM PT - Jun 28th, 2026
❌ @robobun, your commit b952eaf has 4 failures in
🧪 To try this PR locally: bunx bun-pr 32902That installs a local version of the PR into your bun-32902 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
WalkthroughAdds ChangesGlob literal brace preprocessing
Possibly related PRs
🚥 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: 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/matcher.rs`:
- Around line 116-118: Clippy is flagging the early-return in the glob matcher
because the result of strings::index_of_char is only used as a presence check
and the index is discarded. Update the logic in the glob matching path around
the index_of_char call to use the ? operator for the None case instead of an
explicit is_none() return, since the later parsing step rescans the string
anyway. Keep the change localized to the matcher function that handles brace
detection.
In `@test/js/bun/glob/match.test.ts`:
- Around line 336-434: The new brace-group tests only verify literal braces, not
that the entire no-comma group is treated as plain text. Add at least one case
in match.test.ts using a metacharacter inside a no-comma group, such as
Glob("{*}") or Glob("{[ab]}"), and assert it matches the exact literal string
and does not act like a glob. Keep the coverage alongside the existing
brace-group tests in the Glob.match suite.
🪄 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: 1968c629-7baa-46a5-9e82-6060d4374dd6
📒 Files selected for processing (4)
src/glob/lib.rssrc/glob/matcher.rssrc/runtime/api/glob.rstest/js/bun/glob/match.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The previous commit stored the brace-escaped pattern in Glob.pattern,
which GlobWalker::build_pattern_components also reads. On Windows it
treats backslash as a path separator, so the inserted escapes would
split the pattern into bogus components. Keep Glob.pattern raw and
cache the escaped form in a separate match_pattern field used only by
match(); the walker's per-component r#match applies the escaping itself.
The shell neutralized a component-leading interpolated ! with the
one-branch group {!}, which the matcher now treats as a literal.
Emit {!,!} so the group has a top-level comma and still expands.
Also: clippy question_mark fix, rebuild the deeply-nested-brace fixture
with a comma at every level so it still reaches the >255 depth counters,
and cover glob syntax inside a literal group ({*}, {?}, {[ab]}).
|
Re the duplicate flag: #32894 and this PR are not the same fix. #32894 only handles an unclosed Also worth calling out for reviewers: the first push of this PR broke the Bun shell's glob neutralizer. |
…es fixture test The 40,000-brace fixture from 8c87871 guarded match_brace's pre-scan counter against being narrowed back to i16. With comma-less groups now escaped to literals before matching, that unclosed pattern never reaches match_brace, so add a comma-bearing 40,000-level variant alongside it (verified by narrowing the counter to i16, which aborts with a checked overflow on the new fixture and passes without it). The braces test ran its 10-pattern x 7894-path vscode fixture matrix as one test, which takes ~4-5.3s under a debug+ASAN build against a 5s per-test budget and was timing out roughly one run in four on this PR. Split the matrix into one test per pattern with identical assertions so each has its own budget; the remaining direct assertions stay in the original braces test. No coverage is removed.
There was a problem hiding this comment.
All four issues I raised earlier are now addressed (the walker keeps the raw pattern, the shell emits {!,!}, and both the >255 and >32767 depth fixtures are restored), and I didn't find anything new — but this is a deliberate semantic change to bun_glob::r#match that fans out to every caller (walker components, --filter, pm pack, path.matchesGlob, the shell), so it's worth a human sign-off on the behavior change itself.
Extended reasoning...
Overview
This PR changes Bun.Glob.match() (and every other bun_glob::r#match caller) to treat {...} groups with no top-level unescaped comma as literal braces rather than one-branch expansions, aligning with bash/picomatch/minimatch. It adds escape_literal_braces() preprocessing in src/glob/matcher.rs, a match_preprocessed() fast path, caches the escaped pattern in Glob (src/runtime/api/glob.rs) separately from the raw pattern the walker uses, updates the shell's ! neutralizer in Expansion.rs to emit {!,!}, and adds/reworks ~150 lines of test assertions in match.test.ts.
Security risks
None identified. No auth/crypto/permissions surface; the change is pure pattern-matching semantics on byte slices. The preprocessing allocates at most glob.len() + literal.len() bytes and uses SmallVec for the scan stack, so no new unbounded-growth vector.
Level of scrutiny
Medium-high. The implementation itself is compact and well-tested, but r#match is called from many places — the walker's per-component slow path, the test scanner, pm pack, workspace --filter, path.matchesGlob, and the shell glob — and this PR intentionally changes its result for inputs like {a}, {}, {1..3}. The first push broke bunshell.test.ts on every CI lane (the {!} wrapper) and would have regressed Windows scan() (escaped \ treated as a path separator); both are now fixed, but they illustrate the blast radius. A maintainer should confirm the semantic change is wanted across all those call sites, not just Bun.Glob.match().
Other factors
- All four of my earlier inline comments are resolved in the current diff:
match_patternis stored separately so the walker still sees the raw pattern;Expansion.rsemitsb"{!,!}"; the 300-level fixtures use"{a,"soskip_branch's counter is exercised; and 9eebde1 adds a 40,000-level comma-bearing fixture to keepmatch_brace'si32pre-scan covered. - The bug-hunting pass on the latest revision found nothing.
- There is a known textual conflict with #32894 in
matcher.rs(this PR subsumes its unclosed-{handling). - Test coverage for the new behavior is thorough (literal groups, nested literal-outer/expanding-inner, escaped commas, commas inside
[...], glob syntax inside literal groups, negation, wildcard interaction, already-escaped braces). - The braces-vs-filelist test was also refactored from one ~79k-match test into
test.eachto avoid the 5s ASAN budget — a reasonable de-flake but worth a quick glance since it changes test structure unrelated to the headline fix.
Build 65889 produced no signal: every build-cpp and build-rust job expired waiting for a Buildkite agent and the 252 downstream test jobs cascaded to waiting_failed, so nothing compiled or ran.
escape_literal_braces set in_brackets on every [ and never cleared it
when the class was unclosed, so in a pattern like {a,[} the closing }
was swallowed and the end-of-input drain escaped the opening { even
though the group has a top-level comma. That made the group a dead
literal and regressed new Glob("{a,[}").match("a") from true to false.
bash, picomatch, and minimatch all expand {a,[} to a and [.
Only enter bracket mode when an unescaped ] exists after the [, which
is also how picomatch treats an unclosed [. The match_brace scanners
are unchanged: they fire each branch at its comma and never needed the
structural close, so the pre-existing handling of an unclosed [ before
a comma is out of scope here.
bracket_has_closing walks from the [ to the end of the pattern, and the
escape_literal_braces loop called it for every [ that was not already
inside a class. A run of unclosed [ after a { therefore rescanned the
remaining pattern once per [, making the scan quadratic where it used
to be linear. The lookahead and the loop use the same escape-aware
walk, so once it finds no ] after one [ there is none after any later
[ either; a single flag ends the probing and restores the linear scan.
Measured 2.6s down to 71ms on a { followed by 40,000 unclosed [ under
a debug ASAN build, with identical results, so no regression test is
added (it cannot be asserted without coupling the test to a timeout).
Also narrow the bracket_has_closing doc and the matching test comment:
the references expand both branches of {a,[}, but this scan only keeps
the group an expansion; whether the [ branch can match is still up to
match_brace, whose unclosed-[ handling is a separate pre-existing gap.
There was a problem hiding this comment.
All my earlier feedback is addressed (b952eaf handles the last two — the O(n²) unclosed-[ rescan and the overstated {a,[} comments). No further issues found, but this is a user-visible semantic change to the glob matcher that ripples through the walker, the shell's metachar neutralizer, pm pack, --filter, and path.matchesGlob, so it warrants a human sign-off.
Extended reasoning...
Overview
This PR changes Bun.Glob.match() (and every other caller of bun_glob::r#match) so that a {...} group with no unescaped top-level comma is treated as literal braces rather than a one-branch expansion, matching bash/picomatch/minimatch. It does so by adding a preprocessing pass (escape_literal_braces) that rewrites such groups to \\{...\\}, plus a match_preprocessed entrypoint so Bun.Glob can cache the rewrite. The escaped form is deliberately kept out of GlobWalker::build_pattern_components (which treats \\ as a path separator on Windows) by storing it in a separate match_pattern field while the walker keeps reading the raw pattern. The shell's neutralize_glob_metachars is updated from {!} to {!,!} so its component-leading-! wrapper survives the new preprocessing. ~120 lines of new Rust plus extensive test additions and rewrites in match.test.ts.
Security risks
None identified. Glob patterns at every call site are developer-authored (CLI flags, package.json fields, test config, shell literals), not remote-attacker-controlled. The one algorithmic-complexity concern I raised (O(n²) rescans on long runs of unclosed [) was fixed in b952eaf with the no_closing_bracket latch, restoring linear behaviour.
Level of scrutiny
High. This is a deliberate user-visible semantic change to a core matching primitive that fans out to Bun.Glob.match/scan, path.matchesGlob, the shell glob path, bun pm pack files[], --filter, and the test scanner. The implementation has subtle interactions (Windows \\-as-separator in the walker, bracket-class vs. brace nesting, unclosed-[ handling, depth-counter test coverage) and went through six rounds of fixes during review — each of which the author handled well, but the breadth of impact and the interaction with two sibling PRs (#32894, #32895) make this one a human should sign off on.
Other factors
All seven of my prior inline findings are now addressed in code: the Windows walker regression (raw pattern kept for the walker), the shell {!} wrapper (now {!,!}), the >255 and >32767 depth-counter test coverage (rebuilt with comma-bearing fixtures, verified by re-narrowing the counter), the unclosed-[-hides-} regression (bracket_has_closing lookahead), the O(n²) rescan (latched), and the overstated doc/test comments (narrowed). The bug-hunting pass on the current head found nothing further. The robobun CI comment still shows ❌ against 17de434 (the second-to-last commit); the head is b952eaf, so CI status on the final commit is worth confirming before merge.
CI status at head
|
| Lane | What failed | Related to this PR? |
|---|---|---|
| darwin 26 aarch64 test-bun (x2) | buildkite-agent artifact download timed out after 120s for step 'darwin-aarch64-build-bun'. The jobs never received a binary; zero tests ran. |
No (Buildkite infra) |
| darwin 14 aarch64 test-bun (timed out) | test/js/bun/http/fetch-file-upload.test.ts uploads roundtrip with sendfile() (10s timeout, 4 retries) and test/js/bun/http/bun-serve-file.test.ts aborting a streaming file response mid-transfer ... (90s timeout). |
No (HTTP / sendfile) |
| debian 13 aarch64 test-bun | test/js/bun/net/socket.test.ts expectMaxObjectTypeCount leak assertions (Expected: <= 50, Received: 58) plus Unexpected open(). |
No (TCP object-count flake) |
| alpine 3.23 x64 test-bun | test/js/sql/sql-mysql.auth.test.ts: the mysql_native_password Docker sidecar never passed its health check (application not healthy after 1m0s), plus an http-proxy tunnel timeout. The same MySQL sidecar failure hit this PR's earlier build 65783. |
No (Docker service infra) |
There is also one warning-level annotation for test/js/bun/http/serve.test.ts on Windows 2019 x64 that passed on retry.
None of those files or subsystems (TCP sockets, Docker MySQL, HTTP file serving, sendfile) is touched here. This PR changes src/glob/matcher.rs, src/glob/lib.rs, src/runtime/api/glob.rs, src/runtime/shell/states/Expansion.rs, and test/js/bun/glob/match.test.ts.
More useful than the absence of glob failures is their presence as passes in the red lanes themselves:
- the darwin 14 aarch64 shard ran
test/js/bun/glob/match.test.tsandtest/js/bun/shell/bunshell.test.ts; neither appears in its failures. - the alpine 3.23 x64 shard also ran
test/js/bun/glob/match.test.ts; it is not in its failures either.
So the changed test file ran and passed on both a failing macOS aarch64 lane and a failing Linux x64 musl lane, on top of 281 fully green jobs and zero glob, shell, Bun.Glob, path.matchesGlob, or fs.glob failures anywhere in the build.
For the build history on this branch: build 65889 produced no signal at all (every build-cpp / build-rust job expired waiting for a Buildkite agent and the 252 downstream test jobs cascaded to waiting_failed), so I re-ran it once with an empty commit. Build 66133 is that re-run. I am not going to keep pushing empty commits to re-roll network and Docker flake, so this is where it stands.
Local verification on this exact head under bun bd (debug + ASAN): test/js/bun/glob/match.test.ts 37/37, test/js/bun/glob/scan.test.ts 172/172, test/js/bun/shell/bunshell.test.ts -t glob 14/14, and the new tests fail on an unfixed build. All review threads (9) are resolved.
This is ready for a maintainer: the remaining red is infrastructure and unrelated flake that a retrigger will not reliably clear.
|
The Bun shell shows this matcher bug directly. With files The shell cases are in One related note from that attempt: the walker does not follow a symlink for a segment it classifies as a pattern. |
This branch has not been deployed
1 participant
Problem
Bun.Glob.match()expands every{...}as a brace group, including groups that contain no comma:bash only performs brace expansion when the group contains at least one unescaped comma (or a
..sequence expression):touch '{a}.ts'; echo {a}.tsprints{a}.ts. picomatch and minimatch agree:This bites framework filename conventions that use literal braces in paths, and means
{1..3}(which Bun does not implement as a range) matches the wrong thing.Cause
match_braceinsrc/glob/matcher.rstries each comma-separated segment of a{...}group as a branch; when there is no comma it still tries the single segment between{and}, so{a}becomes a one-branch expansion matchingaand{}matches the empty string.Fix
Scan the pattern once up front and, for every
{...}group that has no unescaped top-level comma (or no closing}at all), escape the braces to\{/\}. The matching loop then sees them as ordinary literal bytes with no changes to its state machine. The scan uses the same depth /[...]/\rules asmatch_brace, so a comma nested inside an inner group or inside a bracket class does not count, and only the braces themselves become literal: glob syntax inside the group still applies ({*}matches{abc},{a{b,c}}matches{ab}or{ac}, same as bash and picomatch).One deliberate divergence from
match_brace: the scan only enters bracket mode at a[when an unescaped]follows, so an unclosed[is a literal byte and cannot hide the group's}or,. Without this,{a,[}would be misclassified as an unclosed group and go literal, but bash, picomatch, and minimatch all keep it an expansion, andmatch_bracealready matched theabranch (it fires each branch at its comma and does not need the structural close). This only decides whether the group survives escaping; the[branch itself is still unreachable becausematch_brace's own unclosed-[handling is unchanged, which is the separate pre-existing gap #32895 addresses. The firstfalsefrom the lookahead is sticky (no]after one[means none after any later[), so a long run of unclosed[stays linear.The escaped bytes stay inside
r#matchand never reachGlobWalker::build_pattern_components(which treats\as a path separator on Windows and would split the pattern at the inserted escapes).Bun.Globstill caches the escaped form in a dedicated field somatch()does not rescan on every call, but the walker keeps reading the raw pattern and its per-componentr#matchapplies the escaping itself. Every other caller ofbun_glob::r#match(test scanner,pm pack,--filter,path.matchesGlob, etc.) gets the scan inline.The shell neutralized a component-leading interpolated
!by emitting the one-branch group{!}, which this change correctly turns into a literal. It now emits{!,!}, a real two-branch expansion whose branches are both the literal!, so interpolated!stays inert.Verification
New
brace groups without a comma are literalblock intest/js/bun/glob/match.test.tscovers the reported cases plus{1..3}, unclosed{, escaped\,, comma inside[...], glob syntax inside a literal group ({*},{?},{[ab]}), an unclosed[inside a comma-bearing group ({a,[}still expands), literal-outer/expanding-inner nesting, wildcard and negation interaction, and already-escaped\{. A few existing assertions in thenested bracesblock were asserting the expansion behaviour for comma-less inner groups and are updated to the bash/picomatch result.The deeply-nested-brace fixtures from 8c87871 were written against patterns that this change now treats as literals, so they no longer reached the depth counters they guard. The 300-level fixtures now carry a comma at every level so
skip_branchstill scans past the >255 boundary, and a comma-bearing 40,000-level fixture sits alongside the original unclosed one somatch_brace's pre-scan still climbs pasti16::MAX. Verified by temporarily narrowing that counter back toi16: the new fixture aborts with a checked-overflow panic and passes again ati32.The
bracestest ran its 10-pattern x 7894-path vscode fixture matrix as a single test, which takes 4 to 5.3 seconds underbun bd(debug + ASAN) against the 5 second per-test budget and was timing out about one run in four, before and after this change. It is split into one test per pattern with identical assertions so each has its own budget.scan()/scanSync()pick up the fix via the walker's per-componentr#match(new Bun.Glob("{a}.ts").scanSync()now yields{a}.ts, nota.ts), andpath.matchesGlobinherits it throughBun.Glob.Related: #32894 handles the unclosed-
{case specifically; this PR's preprocessing covers that case as well (an unclosed{has no top-level comma before a matching}, so it is escaped), so whichever lands second will need a small rebase inmatcher.rs.