Repository navigation
Conversation
The `**` arm of glob_match_impl only promoted `**` to a globstar when the
byte after it was `/` or the end of the pattern. When the `**` is the last
thing in a brace branch that byte is the group's `,` or `}`, so the `**`
fell back to single-segment `*` semantics: `a/{**,b}` matched `a/x` but
not `a/x/y`, and `test/{foo/**,bar}/baz` rejected `test/foo/x/y/baz`.
Look at where the pattern actually continues instead: past the `}` of the
group (and of any enclosing groups the `**` also ends), which is exactly
where skip_branch would resume. If that position is `/` or the end of the
pattern, the `**` is a whole segment, and matching continues from there,
so `a/{**,b}/c` now means the same as `a/**/c` or `a/b/c`, including the
zero-segment `a/c`. A group followed by anything else (`a/{**,b}x`, i.e.
`a/**x`) still demotes the `**` to `*`, and a `,`/`}` outside any group is
still a literal. The trailing `/*` special case is evaluated at the same
continuation point so `{**,b}/*` agrees with `**/*`.
|
Warning Review limit reached
Next review available in: 6 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 |
|
Status: fix and tests pushed, waiting on CI.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is well-reasoned and the test coverage is thorough, but since it changes globstar/brace-group interaction in a matcher used by bun test filters, --filter, and the bundler — and overlaps the same arm as #39004/#38996 — a human look to confirm the intended semantics would still be worthwhile.
What was reviewed:
- Traced
globstar_continuationthrough trailing, nested, and non-segment-terminating groups; confirmedbrace_depth == 0short-circuits so brace-free patterns are byte-identical to before. - Checked that
state.wildcardis captured before the group is exited, so backtracking re-enters the**at the in-branch depth and replays the same skip. - Verified the segment-initial check still reads the pre-skip
state.glob_index(what precedes**) while only the follow check usesrest. - Confirmed the relocated
**/*-vs-empty-final-segment special case andskip_branchare read-only onbrace_stack(mutates only the copiedrest).
Extended reasoning...
Overview
This PR fixes Bun.Glob.match() so that a ** ending a brace branch (e.g. a/{**,b}/c) is treated as a globstar rather than being demoted to a single-segment *. The change is ~15 lines in the * arm of glob_match_impl plus an 8-line helper globstar_continuation in src/glob/matcher.rs, a doc-comment update, and a 24-pattern / 125-assertion test table in test/js/bun/glob/match.test.ts.
The mechanism: before deciding whether the ** is a whole segment, walk past any enclosing ,/} via the existing skip_branch (on a copy of state) and evaluate the /-or-end-of-pattern test at that position. When the test passes, commit the copy's glob_index/brace_depth so matching resumes after the group; the saved wildcard still points at the ** with the inside-branch depth, so backtracking is unchanged.
Security risks
None. Input is untrusted glob text but the change only reads bytes via glob.get() / bounds-checked indexing that were already reachable, and skip_branch is bounded by brace_stack (max depth 10). The brace_depth > 0 guard means the new loop iterates at most 10 times. No allocation, no new arithmetic on lengths.
Level of scrutiny
Medium-high. The diff is small and the fix is at the correct layer, but the glob matcher is on the path of bun test filters, workspace --filter, and bundler allowUnresolved, and the interaction of backtracking state (wildcard/globstar), brace_depth, and the brace stack is subtle enough that a second pair of eyes on the semantics is warranted. The PR description also flags two open PRs (#39004, #38996) touching the same arm that will need a mechanical rebase — a maintainer should coordinate landing order.
Other factors
- The PR description includes differential fuzzing (39k pattern/path pairs) showing disagreements outside the targeted construct are byte-identical before and after, and an A/B perf run showing the change is noise for brace-free patterns.
- Test coverage is strong: trailing/mid-pattern groups, nested groups where
**closes multiple levels, sequential groups that backtrack into the globstar, negation, the**/*empty-final-segment edge, and the negative boundaries ({**,b}x,{x**,b}, literal,outside a group) that must stay demoted. - I verified the helper mutates only a copy of
State(*stateisCopy) and readsbrace_stackimmutably, so a failed globstar check leavesstateuntouched and the,/}arm handles the branch exit as before. - No prior human review on the timeline; only a rate-limited CodeRabbit stub.
This branch has not been deployed
1 participant
Problem
**that is the last thing in a brace branch behaves like a single-segment*inGlob.match()(and in the other users ofbun_glob::match:bun testpath filters,--filterworkspace globs, bundlerallowUnresolvedpatterns):*arm ofglob_match_impl(src/glob/matcher.rs:240on main) promotes**to a globstar only when the byte right after it is/or the end of the whole pattern. When the**ends a branch that byte is the group's,or}, so it takes the plain*path even though, per the rule documented onmatchand per what brace expansion of the pattern gives (a/**ora/b), it is a whole segment.Fix
**(globstar_continuation): while the next byte is the,/}of a group we are inside, step past that group's}with the existingskip_branch, i.e. land exactly where matching would resume anyway. The// end-of-pattern test, the existing trailing-/*special case, and the globstar bookkeeping (skip_to_separator) are then all applied at that position, and matching continues from there.a/{**,b}/chas to match the union ofa/**/canda/b/c. For the**branch the text that follows the**in the expansion is whatever follows the group, which is the position this change looks at. Usingskip_branchfor the lookahead means the decision agrees with how the,/}would have been consumed one iteration later (literal,/}outside a group, unterminated groups, nested and sequential groups all behave as they do today); the only new behaviour is that the**keeps globstar semantics when that position is/or the end of the pattern. Backtracking is unchanged: the saved wildcard still points at the**with the inside-the-branchbrace_depth, so re-entering it exits the group again the same way.a/{**,b}xisa/**x, not a whole segment) still demotes the**to*; patterns without braces never enter the new loop (brace_depthis 0), so they are byte-for-byte the same decision as before.scan()is unaffected as well: it matches per path component, so a component like{**,b}never spans a/(glob: expand brace groups that span path separators when scanning #32599 is what makesscan()expand such groups; this change is what makesmatch()agree with it).`**` that ends a brace branch is a globstarblock intest/js/bun/glob/match.test.ts, a table of 24 patterns / 125 assertions each annotated with the brace-free patterns it must be equivalent to: trailing and non-trailing groups, the**in the first / last / an empty branch, nested groups (a/{c,{**,d}}/e), a following group that backtracks into the globstar (a/{**,b}/{c,d}), two such groups in one pattern, a**before the group, negation, plus the boundaries that must stay as they are (a/{**,b}x,a/{x**,b}, literal,outside a group,a/{**,b}/*vsa/x/). Every expectation was cross-checked against the union of the pattern's brace expansions evaluated by the released bun. 18 of the 24 rows fail on bun 1.4.0 (the test also fails on a debug build of main), all pass with this change.test/js/bun/glob/{match,scan,stress,proto,path-length}.test.tspass with the debug build (the only failures seen were the fixturebracestest and thescantests that walktest/node_modules, which time out identically on an unmodified debug build on this loaded box;braceshas been reported separately). Interleaved A/B of the fixture workload (13 patterns x 7895 paths) on debug builds with and without the change: 3832 ms vs 3872 ms best-of-6, i.e. noise; with no brace group open the helper stops at itsbrace_depthcheck.**by its own surroundings, not by the brace branch being matched #39004 and glob: stop a non-segment**from swallowing the**segments after it in match() #38996 both change the other half of the whole-segment rule (what may precede the**) in the same arm and leave the/-or-end lookahead as is, so this is independent of both; whichever lands later needs a mechanical rebase of this hunk.Background
glob_match_implwalks pattern and path together. On{it callsmatch_brace, which tries each branch by recursing intoglob_match_implat the branch's start with aBraceframe (open index, branch index, close index) pushed onbrace_stack; when a branch's text is exhausted the,/}arm callsskip_branch, which jumps past the enclosing group's}and decrementsbrace_depth, so the rest of the pattern after the group is matched by the same recursive call.wildcard, pointing at the**) and callsskip_to_separator, which makes each later backtrack hand one more path segment to the**.is_end_invalidis the existing name for "there is more pattern after this**": it selects between the**/restform (skip the/, keep matchingrest) and the trailing-**form (only a fully consumed path is a match). With this change both forms are driven by the position after the group rather than the byte after the**.glob.len() - idx == 2 && "/*"block above the promotion is a pre-existing special case that keeps**/*from matching a path whose last segment is empty ("","x/"); it is moved to the same position so{**,b}/*behaves like**/*for those paths (covered by thea/{**,b}/*row).Differential check against brace expansion (not part of the PR)
Generated 39,412 distinct (braced pattern, path) pairs from the alphabet
a b c * ** a* *b ? / { , }with up to three levels of nesting and comparedmatch()of the braced pattern with the union ofmatch()over its brace-free expansions on the same binary (expansions contain no braces, so they are unaffected by this change).{preceded by/,,,{or the pattern start;}followed by/,,,}or the end): 1043 disagreements on the released build, 33 with this change. The remaining 33 are all instances of the pre-existing**/*-vs-empty-last-segment special case applied inside a group (**/{*,x}against""), which disagree the same way on the released build.**directly before,/}are the same 297 before and after (byte-identical list), i.e. nothing outside the targeted construct changed.a{**,b},{a,b}{**,c}): the**opens a brace branch and is therefore treated as segment-initial by the existing (and, in glob: judge a**by its own surroundings, not by the brace branch being matched #39004/glob: stop a non-segment**from swallowing the**segments after it in match() #38996, retained) rule, so with this change it is a globstar there too. That matches how bun already treatsa{**/y,b}and how picomatch compilesa{**,b}; textual expansion (a**) would say otherwise. Not asserted either way in the tests.