Repository navigation
Conversation
A pattern array or brace group could return fewer paths than the union of the results of its patterns. Four places in the walker dropped continuations or results when the seen-cache keys of two patterns collided: - Cache.add marked every index of a pattern and reported a repeat when one of them was seen. The caller skipped the pattern, and the indexes that were new were never walked. It now returns the new indexes and the walker continues with those. - The "**/.." branch queued its continuation only when no other pattern had queued that path in the same step. It now appends to the queue. - A trailing ".." recorded its results under the key of a literal ".." walk from the same directory, and blocked that walk (or was blocked by it). The results set already removes duplicates, so the record is gone. - A pattern that starts with "", "." or ".." is redirected before the seen cache records it, because its key is the key of another pattern's last segment. The literal-segment shortcut no longer renames the cached Dirent. The readdir cache holds the same object, so a literal ".." renamed the entry of the parent directory and a later listing did not match it. The walk uses the pattern's name and leaves the Dirent alone. Fixes #42876
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
One verified lower-impact observation (a convention, logging or cleanup point) was not posted.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/js/internal/fs/glob.ts— pre-existing: users still lose the "." result when "./" is in the same array as a pattern whose "" tail reaches "." first, e.g. fs.globSync(["/../", "./"]) in a cwd with a subdirectory omits ".". The seen key for a trailing "" is just "", but the result at src/js/internal/fs/glob.ts:667 also depends on pattern.at(0) === ".", which the key ignores, so #unseenPattern at glob.ts:646 drops "./" before it can add ".". Fix: make the cache key (or the narrowing) account for the pattern-start condition that shapes the result, e.g. keep indexlastin the narrowed pattern when at(0) === "." and path === ".", so the union claim in the header comment holds for this case too.Extended reasoning...
The base branch has the same collision, so this is not widened by the PR, but the PR header comment (glob.ts:8-10) now claims the union of single-pattern results, which this input violates.
Trace for fs.globSync(["/../", "./"], { cwd }) with cwd containing directory "d".
Step 1 at ".": pattern B ("/../") index 0 is GLOBSTAR; for entry "d" next === ".." and nextIndex 1 < last 2, so glob.ts:763-764 queues "." with B index 2 via #queueSubpattern. Then pattern C ("./") is redirected by #redirectFirst (glob.ts:631-633) and #addSubpattern pushes C index 1 onto the same "." array, giving [B2, C1].
Step 2 at ".": B2 runs first. #unseenPattern (glob.ts:646) records cacheKey(2) === "". isLast is true, at(-1) is GLOBSTAR, but path === "." and B.at(0) is GLOBSTAR, not ".", and last is 2, so the condition at glob.ts:667 is false and "." is not added.
Then C1: cacheKey(1) === "" is already in the set, Cache.add returns null, #unseenPattern returns null, glob.ts:647-648 returns. C alone would have hit glob.ts:667 with at(0) === "." and added…Verification: pre-existing (base fails the same way by the same route; the PR touches this exact path and its new header comment at src/js/internal/fs/glob.ts:8-10 now claims a union guarantee this input violates). Trigger: a pattern array where a pattern whose
**tail reaches "." via a**/..step precedes./**, e.g.fs.globSync(["**/../**", "./**"], { cwd })with cwd containing a directoryd.…
|
Updated 3:32 AM PT - Sep 16th, 2026
❌ @robobun, your commit a77b238 has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 42878That installs a local version of the PR into your bun-42878 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Its index 1 is past the end and has the key of a trailing empty segment, so it blocked a later '../' or './' pattern at the same path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
On the review findings: the |
There was a problem hiding this comment.
I re-reviewed the three commits pushed since my last pass and found no bugs; because this deliberately diverges from the upstream Node walker (seen-cache narrowing, no Dirent rename, **/.. continuation queued instead of dropped), a maintainer should still confirm that "union of single-pattern results" is the semantics Bun wants for fs.glob.
What was reviewed:
- The new
pattern.last > 0guard in#redirectFirst(src/js/internal/fs/glob.ts:622): a bare./../root pattern is no longer queued past its end, which removes the seen-key collision I noted earlier for[".", "./"]/["..", "../"]; the new "only '..'" test cases cover both. #addSubpatternsand#iterateSubpatternswere diffed side by side: exclusion → redirect →#unseenPattern→ stat ordering, thenamelocal, and the**/..branch match in both; narrowing indexes viaPattern.childkeepssymlinks/realpaths, andisLast/firstPattern/hasSeenSymlinksall derive from the narrowed set.- Checked whether a leading
.(which the cache key drops) changes results at the root:"**"already adds.via thelast === 0 && statclause and"./**/x"vs"**/x"only widens traversal, not results, so no union loss there. - Traced the
["b/**/..", "./.."]case on the base commit: the oldseen(parent)guard in the trailing-..branch drops.after the literal..walk, so the new tests fail for the stated reason.
Extended reasoning...
Overview
The PR rewrites the seen-cache handling in src/js/internal/fs/glob.ts (the port of Node's lib/internal/fs/glob.js behind fs.glob, fs.globSync, fs.promises.glob). Cache.add now returns the set of not-yet-walked indexes instead of a boolean, both walkers are restructured around #redirectFirst / #unseenPattern / #queueSubpattern, the trailing **/.. result branch stops writing into the seen cache, and the setDirentName mutation of cached Dirents is replaced by a local name. Tests add a describe.each union table run through all three APIs plus a withFileTypes Dirent-name test. Since my previous review, three commits landed: the pattern.last > 0 guard in #redirectFirst, and two comment-shortening commits.
Security risks
None specific to this change. The code only reads the filesystem under the user-supplied cwd and the exclude checks (#isExcluded, #exclude) still run before any queueing in #addSubpattern; the reorder in #addSubpatterns moves the #isExcluded(fullpath) check earlier, not later. The **/.. branch bypasses #addSubpattern's exclude checks via #queueSubpattern, but the old code wrote to #subpatterns directly in the same way, and ResultSet.add still filters excluded results.
Level of scrutiny
Medium-high. The traversal algorithm is intricate and the PR is an intentional divergence from the reference implementation, which the repository guidance treats as the spec for ported code. I traced the seen-cache keying (cacheKey is the tail of globStrings from an index), the index-narrowing path, the sameAs dedupe (ignores symlinks/realpaths, but the only call sites construct children with default symlinks and inherited realpaths, and the **/.. branch fires once per (path, globstar index) after narrowing), and the Windows drive-root branch (typeof first === "string" is checked before endsWith). I did not find a case where narrowing to unseen indexes changes a per-index decision, since the inner loop treats indexes independently and the set-dependent properties (isLast, firstPattern, hasSeenSymlinks) are computed from the narrowed pattern. I could not build or run the debug binary in this environment, so verification was static; the base-commit trace of the ["b/**/..", "./.."] case confirms the new tests exercise the removed guard.
Other factors
No CODEOWNERS entry covers src/js/internal/fs/glob.ts. My earlier inline note about "../" vs ".." spellings colliding was pre-existing and marked non-blocking; the other earlier note is addressed by the new guard. The bug hunt exited on dry_streak with no findings. The remaining reason for a human look is policy rather than a defect: whether Bun wants fs.glob to return the union of its patterns where Node does not, and whether that divergence should be documented beyond the header comment.
The key was the tail of the pattern from the index, so two patterns that end the same way shared a key at a path, and the first one to record it blocked the other. The three special cases that worked around single collisions (the redirect before the record, the trailing '..' that did not record, the bare '..' that was not queued) are gone. The key is now the pattern's position in the expanded list plus the index. Identical expansions are dropped at construction so they do not walk twice.
|
Reshaped after a self-review: the seen cache is now keyed by (pattern, segment index) instead of the pattern tail (a77b238). The three special cases from the first version are gone, and |
There was a problem hiding this comment.
I re-reviewed the latest push (the seen cache is now keyed by pattern id and segment index instead of the pattern tail) and did not find any bugs, but since this is a deliberate algorithmic divergence from the Node port a human look is still worthwhile.
What was reviewed:
Cache.addreturning the unseen index set and#unseenPatternnarrowing viapattern.child(...): sync#addSubpatternsand async#iterateSubpatternsare changed symmetrically, including thename-local replacement for the removedsetDirentNamemutation.#queueSubpattern/Pattern.sameAsreplacing the!#subpatterns.has(path)guard in both**/..branches; the#cache.seenguard is kept, so the same pattern cannot be re-queued at the same path across steps.- Per-pattern keying means overlapping patterns (e.g.
**anda/**) now walk shared subtrees once per pattern rather than once total; readdir results are cached so this is JS-side iteration cost only, andResultSetdedups the output. - Note that the PR description still describes leading
./..redirection and a trailing..that skips the seen cache; neither is in the current diff, so the description is stale relative to the code.
Extended reasoning...
Overview
The PR touches src/js/internal/fs/glob.ts (Bun's port of Node's lib/internal/fs/glob.js) and adds tests to test/js/node/fs/glob.test.ts. The latest push replaced the earlier approach entirely: Pattern now carries a numeric #id (deduplicated by the joined globParts at construction) and cacheKey(index) is ${id}:${index} rather than the joined pattern tail. Cache.add returns the set of not-yet-seen indexes (or null), #unseenPattern narrows the pattern to those indexes before walking, and #queueSubpattern with Pattern.sameAs replaces the !this.#subpatterns.has(path) guard in the **/.. branches. The setDirentName hack is removed in favour of a local name variable. The earlier #redirectFirst logic that my previous inline comments referenced no longer exists in the diff.
Security risks
None identified. The code operates on the caller's own patterns and file tree; there is no new path construction from untrusted data beyond what the existing walker already did, and the removed setDirentName actually reduces mutation of shared objects. The new code uses Set/Map methods and a private-field comparison, no user-overridable prototype machinery was introduced beyond what the surrounding file already uses.
Level of scrutiny
Moderate. The change is a self-contained algorithm tweak in a single JS module, and the sync/async twins were swept together. However, it is a deliberate divergence from the upstream reference implementation (documented in the file header), and it changes the traversal cost model: patterns sharing a tail no longer short-circuit each other, so overlapping pattern arrays iterate shared subtrees once per pattern (readdir is cached, so no extra syscalls). That tradeoff and the divergence from Node's output are judgment calls a maintainer should weigh in on rather than something an automated review should approve outright.
Other factors
The new tests cover the union cases across globSync, promises.glob and callback glob, plus a withFileTypes Dirent-name regression, and normalize separators with sep. I could not run them here (no debug build present). The PR description is stale relative to the code: it still describes leading ./.. redirection and a trailing .. that skips the seen cache, neither of which is in the current diff. The bug hunt ran to a dry streak with no findings, and no CODEOWNERS entry covers the changed files.
Problem
fs.globSync(["**/x/*", "./d/x/x/*", "d/x/*"])overd/x/x/f1andd/x/f2returns["d/x/x/f1"]. The union of the single patterns is["d/x/f2", "d/x/x", "d/x/x/f1"]. Bun 1.3.14 returned all three."a/**/../{x,z}"overa/b/,a/x,a/zdropsa/z. Node.js v26.8.2 returns the same incomplete results.src/js/internal/fs/glob.ts. Its key was the tail of a pattern from a segment index (cacheKey, line 378), so patterns that end the same way shared a key at a path, and the first one to record it blocked the other.Cache.add(line 287) also skipped a whole pattern when one of its indexes was seen, and the**/..branch (line 744) dropped its continuation when another pattern had queued the same path.Fix
Cache.addreturns the new indexes and the walker continues with those. The**/..branch appends its continuation to the queue instead of skipping it...renamed the entry of the parent directory, a later listing did not match it, andwithFileTypesreturned a Dirent named...test/js/node/fs/glob.test.ts(23 new tests, all fail on the base branch). Alsotest-fs-glob.mjs(449 pass),test-fs-glob-throw.mjs,test-path-glob.js,test/regression/issue/24007.test.ts.Background
glob.tsis a port of Node'slib/internal/fs/glob.js. It walks one directory per step. APatternis one brace-expanded pattern plus the set of segment indexes that can match the next entry.**does not walk a directory twice and**/..does not loop.returnin the children loop. Without it, repro 1 still drops paths. node:fs glob: compile plain patterns without minimatch #42874 (perf) touches the same file and rebases on top.Notes
An earlier version of this PR kept the tail key and added three special cases (a redirect of a leading
.or..before the record, a trailing..that did not record, a bare..that was not queued).["**/.", "./**/."]still returned[]with it, so the key itself was replaced. Self-reviewed: the review raised that case and the keying model.Fuzz: random arrays of 2 to 4 patterns with a shared tail, over a fixture of 15 files, compared with the union of the single-pattern results. 0 of 9,000 arrays (3 seeds) differ. On
main, 24 of 3,000 lose a path. For 1,500 random single patterns,fs.promises.globand the callbackfs.globreturn the same paths asfs.globSync, every Dirent has its real name, and no result thatmainreturns is lost. Two single patterns gain paths againstmainand Node (**/../?and**/../*/.). The new paths are correct matches:a/b/../?matchesa/x.globSync("..")andglobSync(".")return[]on Node and Bun, before and after this change. A leading..pattern and a trailing..still spell the parent as../and.., as on Node.CI: every test lane passes. The binary-size check fails for bun-freebsd-aarch64 (+1.61 MB against canary main) with the same numbers on the previous push, which changed only comments and bundled JS. The base branch is behind main, and that is where the delta comes from.