glob: honour backslash escapes of non-special characters in scan() - #32876
glob: honour backslash escapes of non-special characters in scan()#32876robobun wants to merge 5 commits into
Conversation
Bun.Glob.scan() returned no results for patterns like 'sp\ ace.txt' even though Bun.Glob.match() accepted them, because check_special_syntax did not flag '\' and the component was routed through the Literal fast path (raw memcmp / statat with the backslash still in the bytes). The same applied to the WildcardFilepath '*.ext' fast path. Add '\' to SYNTAX_TOKENS and to the WildcardFilepath bailout set so any component containing a backslash falls through to the matcher, which already unescapes '\x' to 'x' for any 'x'. On Windows '\' is a path separator and never appears inside a component slice, so this is a no-op there.
|
Warning Review limit reached
More reviews will be available in 15 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughOn non-Windows, Backslash escape in glob
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:38 PM PT - Jun 27th, 2026
❌ @robobun, your commit 1357bb5 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 32876That installs a local version of the PR into your bun-32876 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
On Windows \ is a path separator, not an escape. A trailing \ on the last component reaches check_special_syntax as part of comp_slice, so including \ in SYNTAX_TOKENS there changed classification of patterns like 'foo\' from Literal to None. Gate the \ additions on !IS_WINDOWS so Windows behaviour is unchanged by this PR.
![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 `@test/js/bun/glob/scan.test.ts`:
- Around line 624-630: The shared temp fixture created in the glob scan test is
never cleaned up, leaving a real directory behind for the process lifetime. Add
teardown in the same test scope that sets up cwd with tempDirWithFiles(), using
an afterAll() cleanup or a disposable fixture pattern, so the glob scan tests
remain hermetic across repeated runs.
- Around line 633-647: The negative-control case in the Glob scan/match table is
not asserting the expected false outcome, so the suite never verifies
`glob.match()` for the escaped backslash pattern. Update the test in the
`test.each` block around `Glob`, `scanSync`, `scan`, and `match` so the `"sp\\\\
ace.txt"` case explicitly checks that `glob.match(...)` is false for the
non-matching candidate, while keeping the existing positive checks for the other
cases.
🪄 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: 70a094ee-ba7f-4167-92ac-d0334b5c56f1
📒 Files selected for processing (2)
src/glob/GlobWalker.rstest/js/bun/glob/scan.test.ts
There was a problem hiding this comment.
LGTM — the !IS_WINDOWS gate in 7c97cc2 cleanly addresses the trailing-separator concern, and the POSIX fix is minimal and well-tested.
Extended reasoning...
Overview
This PR fixes a scan()/match() inconsistency in Bun.Glob: patterns with backslash-escaped non-special characters (e.g. sp\ ace.txt) matched via match() but returned nothing from scan(). The fix is 3 effective lines in src/glob/GlobWalker.rs: add \ to SYNTAX_TOKENS and to the WildcardFilepath bailout set, both gated on !IS_WINDOWS. This routes backslash-containing components through the full matcher (which already unescapes correctly) instead of the raw-byte-compare fast paths. A new describe block in scan.test.ts covers literal, *.ext, directory, **, absolute, and \\ negative-control cases.
Security risks
None. The change only affects which internal code path (fast byte compare vs. full glob matcher) handles a pattern component; it does not touch filesystem permissions, path resolution boundaries, or any auth/crypto surface. The fix strictly widens the set of components that go through the already-correct slow matcher — it can never cause a previously-slow-matched component to take a fast path, so the worst case is a tiny perf cost on \-containing patterns, not a correctness or traversal regression.
Level of scrutiny
Low–medium. This is a narrowly-scoped bug fix in pattern-component classification with a clear root-cause writeup and a conservative remedy. After commit 7c97cc2 gated both additions on !IS_WINDOWS, the Windows code path is byte-for-byte identical to main (SYNTAX_TOKENS is the same literal and the new match arm is dead), eliminating the trailing-\ edge case I flagged earlier. On POSIX the only behavioral change is for patterns that contain a backslash, which were already broken.
Other factors
Both of my prior inline comments are resolved: the pre-existing perf nit (loop over-scan) was acknowledged and reasonably deferred to a follow-up since it predates this PR and is correctness-neutral; the Windows trailing-separator concern was directly fixed by the IS_WINDOWS gate. The bug-hunting system found nothing on the latest revision. No CODEOWNERS entry covers src/glob/. Test coverage is solid (6 new cases exercising every classification path the fix touches, plus the existing 178 scan.test.ts and 26 match.test.ts tests reported green).
Remove the temp dir in afterAll and add an explicit match() assertion for the escaped-backslash pattern so the negative control case actually exercises match() instead of only the (empty) scan result.
There was a problem hiding this comment.
LGTM — the !IS_WINDOWS gating addresses the trailing-separator concern and all other feedback is resolved.
Extended reasoning...
Overview
Two-line production change in src/glob/GlobWalker.rs: add \ to SYNTAX_TOKENS and to the WildcardFilepath bailout set, both gated on !IS_WINDOWS. This routes any pattern component containing a backslash through the full glob::match matcher (which already unescapes correctly) instead of the byte-equality Literal / WildcardFilepath fast paths, fixing the scan()/match() divergence for patterns like sp\ ace.txt. Accompanied by a 7-case test block in scan.test.ts covering literal, *.ext, directory, **, absolute-path, and a \\ negative control.
Security risks
None. No auth, crypto, permissions, or untrusted-input parsing is touched. The change is strictly conservative on the classifier side: it can only move components from a fast path to the slow path, never the reverse, so it cannot introduce a new way for a pattern to match something it shouldn't.
Level of scrutiny
Low-to-moderate. The diff is tiny and the mechanism is simple — widening a token set so more components fall through to the canonical matcher. On Windows the change is a literal no-op (both additions are if !IS_WINDOWS), which was the fix applied in 7c97cc2 after my earlier note about the trailing-separator edge case. On POSIX the only behavioural change is that backslash-containing components now match via glob::match instead of memcmp; correctness improves and the only cost is losing the fast path for the rare \-in-pattern case.
Other factors
- All prior review threads (my two inline notes and the two CodeRabbit nits) are resolved; 7c97cc2 added the Windows gating and 1357bb5 added the
afterAllcleanup plus the explicit negative-controlmatch()assertion. - The bug-hunting system found nothing on the latest revision.
- The two CI failures on 7c97cc2 (
sql-mysql.auth.test.ts,v8-heap-snapshot.test.tsSIGKILL) are unrelated to glob. - No CODEOWNERS entry covers
src/glob/or the test file. - The new tests are POSIX-only (
describe.skipIf(isWindows)), which is correct since\is a path separator on Windows and the production change is gated out there.
|
Status: the change is complete and all review threads are resolved. The remaining CI red is infrastructure, not this diff. On the latest run (build 65765, head
The two earlier runs failed on different but equally unrelated lanes: a Local verification on Linux x64 (debug, ASAN): I've already pushed one empty retrigger for an earlier flake, so I'll stop re-rolling here to avoid spamming CI. Happy to rebase or re-push if a maintainer wants another run. |
1 participant
Problem
Bun.Glob.scan()/scanSync()return nothing for a pattern with a backslash-escaped non-special character, whileBun.Glob.match()on the same pattern says it matches:POSIX, bash and fast-glob all treat
\xas quotingxwhether or notxis a glob metacharacter, and Bun's own matcher already does the same. Only the scanner disagrees, soscanandmatchare inconsistent with each other.Cause
GlobWalker::check_special_syntaxinsrc/glob/GlobWalker.rsonly looks for*[{?!when classifying a pattern component. A component likesp\ ace.txtcontains none of those, so it is taggedSyntaxHint::Literaland routed through the literal fast paths:match_wildcard_literal(raw byte equality) and thestatat()shortcut, both of which compare / open the filename with the backslash still in it. Escaping a special character (st\*ar.txt) happens to work because the*tripscheck_special_syntax, the component getsSyntaxHint::None, and matching falls through to the real matcher which unescapes correctly.The
*.extfast path has the same blind spot: its bailout set is[{?*, so*.t\ sis taggedWildcardFilepathand compared as raw bytes.Fix
Add
\toSYNTAX_TOKENSand to theWildcardFilepathbailout set, so any component that contains a backslash is classified asSyntaxHint::Noneand matched via the full matcher (glob::match), which already strips\before any following character. On Windows\is a path separator and is consumed before component classification, so this change is a no-op there.Verification
Added
describe("backslash escapes non-special characters")totest/js/bun/glob/scan.test.tscovering a literal component, a*.extcomponent, an escaped directory component, a**/recursion, an absolute path, and a\\negative control. Onmain5 of the 6 cases fail (empty result); with this change all pass. Fullscan.test.ts(178 tests) andmatch.test.ts(26 tests) remain green.#32853 noted this divergence in its "not in this PR" section; this change addresses it.