Skip to content

fs: apply function exclude to glob results#63049

Open
Kelpy2004 wants to merge 1 commit intonodejs:mainfrom
Kelpy2004:codex/fix-glob-function-exclude-results
Open

fs: apply function exclude to glob results#63049
Kelpy2004 wants to merge 1 commit intonodejs:mainfrom
Kelpy2004:codex/fix-glob-function-exclude-results

Conversation

@Kelpy2004
Copy link
Copy Markdown

Function-form exclude callbacks were applied while queuing subpatterns, but some terminal result paths were added directly to the result set. That meant files matched by terminal ** or terminal child patterns could bypass an exclude(path) callback that matched the full relative path.

For example, globSync('**', { cwd, exclude: (path) => path === 'a/skip.txt' }) could still include a/skip.txt.

This routes result additions through a shared helper that applies function-form excludes before recording sync and async glob results, while preserving the existing array-exclude filtering in ResultSet.

Added coverage for:

  • fs.globSync()
  • callback fs.glob()
  • fs.promises.glob()
  • terminal ** results
  • terminal pattern results

Validation performed locally:

  • node --check lib/internal/fs/glob.js
  • node --check test/parallel/test-fs-glob.mjs
  • standalone reproduction against installed Node v24.11.1 confirmed the pre-fix behavior still returns an excluded nested file

I could not run the patched Node test binary because this sparse checkout does not include a built local node executable.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 30, 2026
@Kelpy2004 Kelpy2004 force-pushed the codex/fix-glob-function-exclude-results branch from 4882cfd to de11752 Compare May 1, 2026 13:20
@Kelpy2004
Copy link
Copy Markdown
Author

I pushed an update that adds coverage for withFileTypes: true as well, so the regression tests now cover string and Dirent result paths across fs.globSync(), callback fs.glob(), and fs.promises.glob().

The currently visible GitHub Actions runs are all action_required, which means they are waiting for maintainer approval to run on this forked PR rather than failing due to test output.

Local validation:

  • node --check lib/internal/fs/glob.js
  • node --check test/parallel/test-fs-glob.mjs
  • standalone reproduction against installed Node v24.11.1 still shows the pre-fix bug (exclude does not remove a nested terminal ** result)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants