Skip to content

perf(glob): prune ignored directories during traversal, not just post-filter #6123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 7 commits into from
Jul 4, 2026
Merged

perf(glob): prune ignored directories during traversal, not just post-filter #6123

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5ffa0af
fix(glob): apply ignore patterns before traversal, not just post-filter
minmax Jul 1, 2026
d9bdcbe
Merge branch 'main' into fix/glob-ignore-before-traversal
wenshao Jul 1, 2026
e4dee3d
fix(glob): address review feedback on traversal pruning
Jul 2, 2026
94642a4
Merge branch 'main' into fix/glob-ignore-before-traversal
wenshao Jul 2, 2026
2494009
fix(glob): mirror trailing-slash fix in QwenIgnoreParser + document c…
Jul 2, 2026
7552894
fix(glob): address remaining review follow-ups
minmax Jul 2, 2026
38a0e46
fix(glob): append trailing '/' for directory-only pattern matching in…
Jul 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/core/src/services/fileDiscoveryService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,13 @@ export class FileDiscoveryService {
}

/**
* Unified method to check if a file should be ignored based on filtering options
* Unified method to check if a file should be ignored based on filtering options.
*
* Convention: append a trailing `/` to `filePath` to signal that the path
* refers to a directory. This allows directory-only ignore patterns (e.g.
* `node_modules/`) to match correctly during traversal pruning. Both the
* GitIgnoreParser and QwenIgnoreParser preserve the trailing slash through
* their internal path normalization.
*/
shouldIgnoreFile(
filePath: string,
Expand Down
93 changes: 93 additions & 0 deletions packages/core/src/tools/glob.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,6 +644,99 @@ describe('GlobTool', () => {
expect(result.llmContent).toContain('visible.txt');
expect(result.llmContent).not.toContain('hidden.secret');
});

it('does not over-ignore nested dirs for a root-anchored gitignore pattern', async () => {
// Regression: `/dist` is anchored to the repo root and must NOT exclude

@wenshao wenshao Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Tests don't verify that the ignore option is actually passed to glob.

All 3 new tests assert only on result.llmContent (post-filter output). The post-filter (filterFilesWithReport) would produce identical results without the ignore option — removing the ignore: { ignored, childrenIgnored } block from the glob call would not cause any test to fail.

The core change of this PR has no test coverage.

Suggested fix — add at least one assertion verifying the glob spy received the ignore callbacks:

const lastCall = vi.mocked(glob.glob).mock.calls.at(-1);
const globOptions = lastCall?.[1];
expect(globOptions?.ignore).toBeDefined();
expect(globOptions?.ignore?.childrenIgnored).toBeTypeOf('function');

— qwen3.7-max via Qwen Code /review

// a nested `src/dist`. Traversal pruning delegates to the real gitignore
// logic, so anchoring is preserved (a lossy `/dist` -> `**/dist/**`
// conversion would wrongly prune src/dist while walking).
await fs.writeFile(path.join(tempRootDir, '.gitignore'), '/dist\n');
await fs.mkdir(path.join(tempRootDir, 'dist'));
await fs.writeFile(path.join(tempRootDir, 'dist', 'root.keep'), 'x');
await fs.mkdir(path.join(tempRootDir, 'src', 'dist'), {
recursive: true,
});
await fs.writeFile(
path.join(tempRootDir, 'src', 'dist', 'nested.keep'),
'x',
);

const invocation = new GlobTool(mockConfig).build({
pattern: '**/*.keep',
});
const result = await invocation.execute(abortSignal);

expect(result.llmContent).toContain('nested.keep');
expect(result.llmContent).not.toContain('root.keep');
});

it('prunes a gitignored directory (e.g. node_modules) during traversal', async () => {
await fs.writeFile(
path.join(tempRootDir, '.gitignore'),
'node_modules/\n',
);
await fs.mkdir(path.join(tempRootDir, 'node_modules', 'pkg'), {
recursive: true,
});
await fs.writeFile(
path.join(tempRootDir, 'node_modules', 'pkg', 'dep.keep'),
'x',
);
await fs.mkdir(path.join(tempRootDir, 'app'));
await fs.writeFile(path.join(tempRootDir, 'app', 'main.keep'), 'x');

const invocation = new GlobTool(mockConfig).build({
pattern: '**/*.keep',
});
const result = await invocation.execute(abortSignal);

expect(result.llmContent).toContain('main.keep');
expect(result.llmContent).not.toContain('dep.keep');
});

it('passes ignore callbacks to glob for traversal pruning', async () => {
await fs.writeFile(
path.join(tempRootDir, '.gitignore'),
'node_modules/\n',
);
await fs.mkdir(path.join(tempRootDir, 'node_modules'));

vi.mocked(glob.glob).mockClear();

const invocation = new GlobTool(mockConfig).build({
pattern: '**/*.keep',
});
await invocation.execute(abortSignal);

const lastCall = vi.mocked(glob.glob).mock.calls.at(-1);
const globOptions = lastCall?.[1] as
| { ignore?: { ignored?: unknown; childrenIgnored?: unknown } }
| undefined;
expect(globOptions?.ignore).toBeDefined();
expect(globOptions?.ignore?.ignored).toBeTypeOf('function');
expect(globOptions?.ignore?.childrenIgnored).toBeTypeOf('function');
});

it('honors gitignore negation re-inclusion during traversal', async () => {
// `!build/keep.keep` re-includes a file under an otherwise-ignored path.
// Dropping negations (as a pattern conversion must) would wrongly prune
// it; delegating to the real ignore logic preserves re-inclusion.
await fs.writeFile(
path.join(tempRootDir, '.gitignore'),
'build/**\n!build/keep.keep\n',
);
await fs.mkdir(path.join(tempRootDir, 'build'));
await fs.writeFile(path.join(tempRootDir, 'build', 'keep.keep'), 'x');
await fs.writeFile(path.join(tempRootDir, 'build', 'skip.keep'), 'x');

const invocation = new GlobTool(mockConfig).build({
pattern: '**/*.keep',
});
const result = await invocation.execute(abortSignal);

expect(result.llmContent).toContain('keep.keep');
expect(result.llmContent).not.toContain('skip.keep');
});
});

describe('file count truncation', () => {
Expand Down
37 changes: 35 additions & 2 deletions packages/core/src/tools/glob.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { ToolErrorType } from './tool-error.js';
import { getErrorMessage } from '../utils/errors.js';
import type { FileDiscoveryService } from '../services/fileDiscoveryService.js';
import { createDebugLogger } from '../utils/debugLogger.js';
import { isPathWithinRoot } from '../utils/workspaceContext.js';

const debugLogger = createDebugLogger('GLOB');

Expand Down Expand Up @@ -142,6 +143,35 @@ class GlobToolInvocation extends BaseToolInvocation<
effectivePattern = escape(effectivePattern);
}

const projectRoot = this.config.getTargetDir();
const fileFilteringOptions = this.getFileFilteringOptions();

// Prune ignored directories DURING traversal (glob's `childrenIgnored`)
// rather than only post-filtering the results. Delegating to
// FileDiscoveryService reuses the real .gitignore/.qwenignore semantics
// (anchoring, negation/re-inclusion, nested ignore files) — a hand-rolled
// gitignore→glob pattern conversion cannot reproduce these correctly.
const isTraversalIgnored = (entry: {
fullpath(): string;
isDirectory(): boolean;
}): boolean => {
const relativePath = path.relative(projectRoot, entry.fullpath());
// Never prune paths outside the project root (e.g. an external search

@wenshao wenshao Jul 1, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] Redundant path.relative → path.resolve → path.relative round-trip.

isTraversalIgnored computes path.relative(projectRoot, entry.fullpath()) here, then passes the result to shouldIgnoreFile → GitIgnoreParser.isIgnored, which does path.resolve(projectRoot, filePath) and then path.relative(projectRoot, resolved) again internally. For a large project, this means 2 extra path operations per traversed entry.

Consider a future optimization to pass the already-computed relative path through, or add a shouldIgnoreFileByRelativePath method that skips the round-trip.

— qwen3.7-max via Qwen Code /review

// dir); ignore rules are only defined relative to the root.
if (!relativePath || !isPathWithinRoot(entry.fullpath(), projectRoot)) {
return false;
}
// Append trailing '/' for directories so the ignore library matches
// directory-only patterns like `node_modules/`.
const ignorePath = entry.isDirectory()
? relativePath + '/'
: relativePath;
return this.fileService.shouldIgnoreFile(

@DragonnZhang DragonnZhang Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Performance] shouldIgnoreFile ultimately calls GitIgnoreParser.isIgnored(), which creates a fresh ignore() instance and re-adds all patterns on every invocation. With childrenIgnored called for every directory during traversal and ignored called for every file, this is O(entries * patterns) with a non-trivial constant (instance creation + pattern parsing per entry).

For projects with thousands of files and many gitignore patterns, this per-entry overhead could partially offset the traversal-pruning benefit. Consider pre-building a single ignore() instance (with all patterns loaded once) and reusing it in the callback, or exposing a cached variant of shouldIgnoreFile that skips the per-call ignore() construction. The GitIgnoreParser already caches .gitignore file contents in this.cache -- the next step would be caching the assembled ignore() instance too.

Not a blocker -- the net effect is still positive since pruning large ignored trees (e.g. node_modules) avoids walking their entire contents.

ignorePath,
fileFilteringOptions,
);
};

const entries = (await glob(effectivePattern, {
cwd: searchDir,
withFileTypes: true,
Expand All @@ -151,20 +181,23 @@ class GlobToolInvocation extends BaseToolInvocation<
dot: true,
follow: false,
signal,
ignore: {
ignored: isTraversalIgnored,
childrenIgnored: isTraversalIgnored,
},
})) as GlobPath[];

// Filter using paths relative to the project root (the base that
// FileDiscoveryService uses for .gitignore / .qwenignore evaluation).
// Using searchDir-relative paths would cause ignore rules to be
// evaluated against incorrect paths when searchDir != projectRoot.
const projectRoot = this.config.getTargetDir();
const relativePaths = entries.map((p) =>
path.relative(projectRoot, p.fullpath()),
);

const { filteredPaths } = this.fileService.filterFilesWithReport(
relativePaths,
this.getFileFilteringOptions(),
fileFilteringOptions,
);

const normalizePathForComparison = (p: string) =>
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/utils/gitIgnoreParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export class GitIgnoreParser implements GitIgnoreFilter {
}

try {
const isDir = filePath.endsWith('/');
const resolved = path.resolve(this.projectRoot, filePath);
const relativePath = path.relative(this.projectRoot, resolved);

Expand All @@ -115,7 +116,12 @@ export class GitIgnoreParser implements GitIgnoreFilter {
}

// Even in windows, Ignore expects forward slashes.
const normalizedPath = relativePath.replace(/\\/g, '/');
let normalizedPath = relativePath.replace(/\\/g, '/');
// Preserve trailing '/' so directory-only patterns (e.g. `node_modules/`)
// are matched correctly by the ignore library.
if (isDir && !normalizedPath.endsWith('/')) {
normalizedPath += '/';
}

if (normalizedPath.startsWith('/') || normalizedPath === '') {
return false;
Expand Down
8 changes: 7 additions & 1 deletion packages/core/src/utils/qwenIgnoreParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ export class QwenIgnoreParser implements QwenIgnoreFilter {
return null;
}

const isDir = filePath.endsWith('/');
const resolved = path.resolve(this.projectRoot, filePath);
const relativePath = path.relative(this.projectRoot, resolved);

Expand All @@ -180,7 +181,12 @@ export class QwenIgnoreParser implements QwenIgnoreFilter {
}

// Even in windows, Ignore expects forward slashes.
const normalizedPath = relativePath.replace(/\\/g, '/');
let normalizedPath = relativePath.replace(/\\/g, '/');
// Preserve trailing '/' so directory-only patterns (e.g. `node_modules/`)
// are matched correctly by the ignore library.
if (isDir && !normalizedPath.endsWith('/')) {
normalizedPath += '/';
}

if (normalizedPath.startsWith('/') || normalizedPath === '') {
return null;
Expand Down
Loading