Repository navigation
Conversation
|
Updated 7:37 PM PT - Jul 21st, 2026
❌ @robobun, your commit ceadce4 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34200That installs a local version of the PR into your bun-34200 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
WalkthroughChangesThe glob component parser now excludes trailing separators when classifying syntax and wildcard length, while tests cover recursive directory scanning with Globstar parsing and scanning
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Re the find-issues suggestion: #21300 is unrelated. That report uses |
There was a problem hiding this comment.
Beyond the inline notes: checked that dropping saturating_sub(1) on last_idx is safe (the len == 0 early return already guards it), and that a bare-/ component (effective_len == 0) still classifies as Literal exactly as before.
Extended reasoning...
The two inline findings already cover the interesting collateral behavior (adjacent trailing-sep file admission is pre-existing; .//../ reclassification is a consistency nit). I additionally traced the moved block for underflow — the old code used saturating_sub(1) and the new code uses plain - 1, but make_component returns None on len == 0 before reaching it, so no underflow. I also traced the degenerate effective_len == 0 case (a component that is only /): comp_slice becomes empty, check_special_syntax is false, so it classifies as Literal — identical to the pre-PR path where comp_slice == b"/" also fell through to Literal. The WildcardFilepath branch still keys on component.len rather than effective_len, but for every trailing-slash input it reaches the same hint as before.
There was a problem hiding this comment.
LGTM — 95401e4 addresses both prior notes exactly as suggested, and the new excludes files when the final component carries a trailing separator block covers the matrix.
Checked the follow-up: !next.trailing_sep in the !is_last peek-ahead and the S::ISDIR gate on the statat fast path are both correct and minimal; next_pattern.unwrap() remains safe since !is_last implies idx+1 < len. Re-traced make_component for the effective_len == 0 case (bare /) — still Literal + trailing_sep, unchanged. */ reclassifying None→Single has no observable effect (same match_pattern_impl result, NT filter still matches everything).
Extended reasoning...
Overview
Three focused edits to src/glob/GlobWalker.rs: (1) make_component now detects trailing_sep up front and classifies on effective_len so **/ is SyntaxHint::Double; (2) match_pattern_file's !is_last peek-ahead adds && !next.trailing_sep; (3) the Literal-tail statat fast path gates on S::ISDIR(mode) when the component carries a trailing separator. Nine tests added to the existing trailing directory separator describe block in scan.test.ts.
Security risks
None. This is pattern-component classification and match gating inside the glob walker; no auth, crypto, or privilege boundaries. The change tightens behavior (a trailing / now excludes files on paths where it previously leaked them), which if anything reduces surprise.
Level of scrutiny
Moderate — glob traversal feeds the bundler, test runner, and install, so a misclassification could hide or over-match entries. That's why I traced every reclassified shape (**/, */, ./, ../, bare /) through both the walker and the Windows NT-filter path in the first pass, and re-checked the two follow-up hunks against the exact shapes I'd flagged. All check out; the ../ → DotBack side effect is confirmed intentional and consistency-restoring.
Other factors
I reviewed the first commit in detail and left two findings (one heads-up, one pre-existing same-class bug); both threads are now resolved and the author's fix in 95401e4 is exactly the two local edits I proposed, plus tests for each shape (**/foo/, **/*/, foo/, sub/foo/, */foo/, and the positive directory case). Tests use tempDir/using, are cross-platform via path.sep, and assert exact sorted arrays. The bug-hunting system found nothing on the follow-up commit. No CODEOWNERS entry for src/glob/.
|
CI on ceadce4 (build 77295, post-rebase): the glob suites pass on every lane. Remaining red is unrelated to this diff:
This diff touches only |
|
@robobun fix conflicts and push |
make_component() classified the syntax hint before detecting the
trailing separator, so the pattern-final component "**/" (len 3) missed
the len == 2 => SyntaxHint::Double arm and was left as a generic
one-segment pattern. The walker only recurses on SyntaxHint::Double, so
scan("**/") and scan("prefix/**/") stopped at a single directory level
while match() on the same Glob object (which consults pattern_slice(),
already stripping the separator) accepted arbitrarily deep paths.
Detect trailing_sep first and classify on the same slice pattern_slice()
returns. "**/" now recurses like "**" while still filtering out files
via the existing trailing_sep check in match_pattern_file().
Widen the trailing-separator fix to the two sibling sites that still admitted files: - match_pattern_file's `**/X` peek-ahead checked pattern.trailing_sep (the globstar's) but not next_pattern.trailing_sep, so "**/foo/" and "**/*/" yielded regular files. - The Literal-tail statat() fast path accepted S::ISREG without consulting trailing_sep, so "foo/" and "sub/foo/" yielded a file.
95401e4 to
ceadce4
Compare
|
Rebased onto main (85ddc95). The only conflict was with #35002's dead-code removal of the |
|
Do not merge this PR yet. It changes a form that works on main. A pattern whose last segment is
The cause is in The PR body and my earlier review reply called the I will change the PR so that these five patterns keep the result they have on main, and I will add them as tests. |
Problem
A trailing
/on a glob pattern is the standard idiom for "directories only" (bash globstar, fast-glob,node:fs.globSync).Bun.Glob().scanSync()violates this in three places:Cause
build_pattern_componentskeeps the trailing separator insidecomponent.lenfor the pattern-final component, andmake_componentclassified the syntax hint on that rawlenbefore detectingtrailing_sep. So"**/"(len 3) missed thelen == 2 => SyntaxHint::Doublearm and stayedSyntaxHint::None; the walker only recurses onDouble.Component::pattern_slice()already subtractstrailing_sep, which is whymatch()still sees bare**and accepts deep paths.Two adjacent paths carried the same defect:
match_pattern_file's**/Xpeek-ahead checkedpattern.trailing_sep(the globstar's, always false) but notnext_pattern.trailing_sep, so"**/foo/"and"**/*/"yielded regular files.statat()fast path intransition_to_dir_iter_stateacceptedS::ISREGwithout consultingtrailing_sep, so"foo/"and"sub/foo/"yielded a regular file.Fix
make_componentnow detectstrailing_sepfirst and classifies the syntax hint on the same slicepattern_slice()returns (len - trailing_sep)."**/"classifies asSyntaxHint::Doubleand recurses like"**", whilematch_pattern_file's existingtrailing_sepguard continues to exclude files.match_pattern_file's!is_lastbranch additionally requires!next.trailing_sep.statat()fast path requiresS::ISDIR(mode)when the component has a trailing separator.As a consequence of classifying on the stripped slice, a bare
"../"now agrees with".."(bothDotBack, both yield[]); previously"../"classified asLiteraland yielded[".."]via the statat fast path.Verification
Added nine cases to the
trailing directory separatordescribe block intest/js/bun/glob/scan.test.ts:**/,prefix/**/, scan/match agreement, async scan parity,**/foo/on files and on directories,**/*/,foo/,sub/foo/, and*/foo/. Eight fail withUSE_SYSTEM_BUN=1and all pass with this change; the ninth (**/foo/on a directory) is a regression guard. Fullscan.test.ts(203),match.test.ts(26), andnode/fs/glob.test.ts(27) suites pass.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/glob/scan.test.ts