perf(glob): prune ignored directories during traversal, not just post-filter - #6123
Conversation
The glob tool enumerated ALL files (including node_modules, target, .git) before applying .gitignore/.qwenignore post-filtering. On large workspaces this traverses millions of files and can hang the session indefinitely. Now the glob call receives ignore patterns from .gitignore, .qwenignore, and universal exclusions (node_modules, .git) via the `ignore` option, so these directories are skipped during traversal entirely. Adds getRootPatterns() to GitIgnoreParser and getTraversalIgnorePatterns() to FileDiscoveryService to expose ignore patterns for pre-traversal use.
| return false; | ||
| } | ||
| return this.fileService.shouldIgnoreFile( | ||
| relativePath, |
There was a problem hiding this comment.
[Critical] Directory-only gitignore patterns (node_modules/, dist/, build/) are never pruned during traversal.
The ignore npm library requires a trailing / on the path to match directory-only patterns. Since path.relative() returns paths without trailing slashes, ig.ignores('node_modules') returns false for pattern node_modules/. The childrenIgnored callback never prunes the top-level ignored directory — glob still descends into it and enumerates all children.
Verified empirically: ig.ignores('node_modules') → false, ig.ignores('node_modules/') → true for pattern node_modules/.
The post-filter catches descendant files (multi-segment paths like node_modules/pkg/file.js do match), so functional correctness is preserved — but the performance optimization (the stated purpose of this PR) is defeated for the most common gitignore pattern forms.
Suggested fix: append / when the entry is a directory. The glob Path type exposes isDirectory():
| relativePath, | |
| 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 | |
| // dir); ignore rules are only defined relative to the root. | |
| if ( | |
| !relativePath || | |
| relativePath === '..' || | |
| relativePath.startsWith(`..${path.sep}`) || | |
| path.isAbsolute(relativePath) | |
| ) { | |
| return false; | |
| } | |
| const ignorePath = entry.isDirectory() ? relativePath + '/' : relativePath; | |
| return this.fileService.shouldIgnoreFile( | |
| ignorePath, | |
| fileFilteringOptions, | |
| ); | |
| }; |
Note: also verify that shouldIgnoreFile → GitIgnoreParser.isIgnored preserves the trailing / before passing to ig.ignores(). Currently path.resolve() inside isIgnored strips it, so the fix may need to go deeper into the ignore parser.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| 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 |
There was a problem hiding this comment.
[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
| if ( | ||
| !relativePath || | ||
| relativePath.startsWith('..') || | ||
| path.isAbsolute(relativePath) |
There was a problem hiding this comment.
[Suggestion] relativePath.startsWith('..') is overly broad — a directory named ..foo is misclassified as outside the project root.
path.relative('/project', '/project/..foo') returns "..foo", which passes startsWith('..') but is a valid path inside the project root.
The existing isPathWithinRoot utility (workspaceContext.ts:324) handles this correctly:
| path.isAbsolute(relativePath) | |
| relativePath === '..' || | |
| relativePath.startsWith(`..${path.sep}`) || |
— qwen3.7-max via Qwen Code /review
| // gitignore→glob pattern conversion cannot reproduce these correctly. | ||
| const isTraversalIgnored = (entry: { fullpath(): string }): boolean => { | ||
| const relativePath = path.relative(projectRoot, entry.fullpath()); | ||
| // Never prune paths outside the project root (e.g. an external search |
There was a problem hiding this comment.
[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
E2E Tmux Test Report -- PR #6123Test Environment
Note: Full CLI build ( Test 1: Unit Tests -- glob.test.ts (51/51 passed)All existing tests pass, plus the 3 new tests added by this PR:
Test 2: FileDiscoveryService tests (15/15 passed)All FileDiscoveryService tests pass, confirming the new Test 3: GitIgnoreParser tests (21/21 passed)All GitIgnoreParser tests pass, confirming the new Test 4: E2E Integration -- direct glob traversal test (3/3 passed)A standalone script tested the glob VerdictAll tests pass. The optimization is correct and safe:
Total test count: 87 tests passed, 0 failed across glob (51), FileDiscoveryService (15), GitIgnoreParser (21). Chinese TranslationE2E Tmux 测试报告 -- PR #6123测试环境
注意: 完整 CLI 构建 ( 测试结果
总计: 87 项测试全部通过, 0 项失败 PR 新增的 3 项测试
结论所有测试通过。 优化正确且安全:
|
- Append trailing '/' for directory entries so ignore library matches
directory-only patterns like `node_modules/`
- Preserve trailing '/' through path.resolve/relative round-trip in
GitIgnoreParser.isIgnored
- Replace overly broad startsWith('..') with isPathWithinRoot to avoid
misclassifying directories like `..foo`
- Add test verifying ignore callbacks are actually passed to glob
|
@qwen-code /review |
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
The traversal-pruning approach is sound: delegating to FileDiscoveryService.shouldIgnoreFile correctly preserves gitignore semantics (anchoring, negation, nested ignore files) that a hand-rolled pattern conversion would lose. The HEAD commit addresses all of @wenshao's earlier feedback (trailing / for directory-only patterns, isPathWithinRoot replacing the overly-broad startsWith('..'), verifying the callbacks are passed to glob). Tests cover the key scenarios.
One performance observation inline.
| const ignorePath = entry.isDirectory() | ||
| ? relativePath + '/' | ||
| : relativePath; | ||
| return this.fileService.shouldIgnoreFile( |
There was a problem hiding this comment.
[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.
| follow: false, | ||
| signal, | ||
| ignore: { | ||
| ignored: isTraversalIgnored, |
There was a problem hiding this comment.
[Suggestion] GitIgnoreParser.isIgnored creates a fresh ignore() instance on every call (re-adding all patterns and compiling Minimatch regexes). Pre-PR this was only called for post-filter results (tens of files). Now both ignored and childrenIgnored invoke it for every entry during traversal — potentially tens of thousands of entries in a large repo, with directories evaluated twice.
On a repo with 50k files at average depth 5, this creates ~50k ignore() instances and ~250k ig.add() + ig.ignores() calls. The pattern arrays are cached in this.cache, but the compiled ignore() instances are not.
Consider either:
- Caching the
ignore()result per-entry (e.g., aMap<string, boolean>keyed byentry.fullpath()) to eliminate the double evaluation fromignored+childrenIgnored. - Building a single long-lived
ignore()instance with all root-level patterns pre-loaded, reused across calls. - Extracting ignore patterns as glob-compatible strings and passing them as glob's native
ignorearray, avoiding the callback entirely.
— qwen3.7-max via Qwen Code /review
| // 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: { |
There was a problem hiding this comment.
[Suggestion] The traversal callback has no debug logging and no error boundary. If isTraversalIgnored over-prunes (e.g., a broad .gitignore pattern), glob silently returns fewer or zero results with no diagnostic trail — indistinguishable from a legitimate empty result. If any call in the chain throws unexpectedly, the entire glob operation fails.
Consider adding:
- A
try/catchreturningfalse(fail-open, matchingGitIgnoreParser.isIgnored's own pattern). - A debug log on
return trueand a summary count after glob completes, so over-pruning is diagnosable.
let traversalPrunedCount = 0;
const isTraversalIgnored = (entry: {
fullpath(): string;
isDirectory(): boolean;
}): boolean => {
try {
// ... existing logic ...
const ignored = this.fileService.shouldIgnoreFile(ignorePath, fileFilteringOptions);
if (ignored) {
traversalPrunedCount++;
debugLogger.debug(`Traversal pruned: ${ignorePath}`);
}
return ignored;
} catch (e) {
debugLogger.warn('Traversal ignore check failed:', e);
return false;
}
};— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| try { | ||
| const isDir = filePath.endsWith('/'); |
There was a problem hiding this comment.
[Suggestion] The trailing-/ preservation added here for GitIgnoreParser is not mirrored in QwenIgnoreParser.normalizePathForIgnore. That method also runs path.resolve() + path.relative() which strip the trailing /, but never re-adds it.
Since isTraversalIgnored calls shouldIgnoreFile which dispatches to both parsers, directory-only patterns (e.g., node_modules/) in .qwenignore files won't match during traversal pruning. The post-filter maintains output correctness, but the performance benefit this PR delivers for .gitignore is silently lost for directories ignored only via .qwenignore.
The shouldIgnoreFile / isIgnored interface also doesn't document that a trailing / on filePath signals "this is a directory" — the PR author fixed one parser but not the other precisely because nothing at the interface level signals this convention.
Suggested fix: apply the same isDir capture + re-append pattern to QwenIgnoreParser.normalizePathForIgnore, and add a JSDoc note on shouldIgnoreFile documenting the trailing-/ convention.
— qwen3.7-max via Qwen Code /review
| expect(result.llmContent).not.toContain('hidden.secret'); | ||
| }); | ||
|
|
||
| it('does not over-ignore nested dirs for a root-anchored gitignore pattern', async () => { |
There was a problem hiding this comment.
[Suggestion] The new tests cover gitignore anchoring, directory pruning, callback wiring, and negation re-inclusion — all good. Two additional scenarios would strengthen coverage:
-
respectGitIgnore: falsewith traversal: no test verifies that traversal pruning is disabled whenrespectGitIgnoreisfalse. The code passesfileFilteringOptionstoshouldIgnoreFile, so this should work, but a regression would silently over-prune results with no test to catch it. -
External directory +
.gitignore: theisPathWithinRootguard prevents pruning paths outside the project root, but no test combines an external search directory with a.gitignoreto verify the guard works correctly.
— qwen3.7-max via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Previous critical traversal-pruning issues are resolved at this head. I found no new critical blocker.
…onvention - Apply same isDir capture + re-append pattern to QwenIgnoreParser.normalizePathForIgnore so directory-only patterns (e.g. `node_modules/`) in .qwenignore also match during traversal - Add JSDoc on shouldIgnoreFile documenting the trailing `/` convention
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The traversal-pruning approach is sound — delegating to FileDiscoveryService.shouldIgnoreFile correctly preserves gitignore semantics (anchoring, negation, nested ignore files) that a hand-rolled pattern conversion would lose. The trailing-slash convention is consistently propagated through all three layers (glob callback, GitIgnoreParser, QwenIgnoreParser). Tests adequately cover the key scenarios: root-anchored patterns, directory pruning, callback wiring, and negation re-inclusion. Build and all 52 glob tests pass.
— qwen3.7-max via Qwen Code /review
- Memoize the compiled ignore matcher per directory in GitIgnoreParser so traversal pruning no longer rebuilds a fresh ignore() (with full pattern recompilation) for every entry. Behavior-preserving; cuts the hot-path cost from O(entries) instance builds to O(directories). - Fail open in the glob traversal ignore callback: on an unexpected error, log at debug and do not prune (the post-filter stays the source of truth), avoiding a silent empty result or a crashed glob. - Tests: respectGitIgnore=false leaves gitignored dirs unpruned; an external search dir is never pruned by the project root's ignore rules.
✅ Local end-to-end verification — merge referenceI built the PR at head Verdict: correct and safe to merge. The change is behaviorally equivalent to the old post-filter-only path, delivers a real traversal-pruning speedup, and cannot over-prune. One optional test-coverage suggestion below (non-blocking). Existing tests (real code, vitest aliased to
|
| Suite | Result |
|---|---|
glob.test.ts (incl. 4 new cases) |
52 / 52 pass |
gitIgnoreParser + qwenIgnoreParser + fileDiscoveryService |
47 / 47 pass |
tsc --noEmit on the 4 changed files |
0 errors (23 unrelated provider/model errors are pre-existing env split-brain — identical count at merge-base, PR adds none) |
Correctness — behavioral equivalence & no over-pruning
I ran real glob with vs without the ignore callbacks and applied the real post-filter to both, across 6 tricky scenarios (root-anchored /dist vs nested src/dist; node_modules/; negation under a glob parent build/** + !build/keep; negation under a directory parent build/ + !build/keep; a nested .gitignore trying to re-include inside an ignored dir; a nested .gitignore in a non-ignored dir). Final result sets were identical in every case — pruning only removes what the post-filter would have removed anyway.
The one real risk with directory pruning is over-pruning (dropping a file that !negation re-includes). This cannot happen here because both paths call the same ignore.ignores(), which enforces git's rule "a file cannot be re-included if a parent directory is excluded." I brute-forced the invariant ignores(dir/) === true ⟹ every descendant ignored over 280 (dir, file) pairs × 21 exotic pattern sets (**, ?, [0-9], mid-path wildcards, nested negations): 0 violations. Anchoring (/dist ≠ src/dist), negation, nested ignore files, and the .git dir all behave correctly.
Performance — pruning is real and load-bearing
childrenIgnored prunes during the walk, so ignored subtrees are never enumerated. Proven two ways:
readdircounter (customfsinjected into glob): with pruning,node_modulesis never opened; without it, it is fully read.- Real
GlobToolInvocation(spiedshouldIgnoreFile):node_modules/is queried once bychildrenIgnoredand returnstrue, so no deeper path is ever queried (no descent).
Synthetic tree with 1500 ignored files (node_modules/, dist/) + 20 source files:
| with pruning | without | |
|---|---|---|
readdir calls |
22 | 383 (17.4× fewer) |
| raw entries glob returns | 21 | 1521 |
| wall time | 5.6 ms | 44.7 ms (~8× faster) |
The trailing-slash fix in both parsers is what makes this work: ignores('node_modules') is false but ignores('node_modules/') is true, so directory-only patterns only prune when the dir is signalled with a trailing /. Mutation-reverting that fix disables all pruning (glob re-enumerates node_modules) while keeping results correct — confirming it is load-bearing for performance, not correctness.
Mutation matrix (proves each change carries weight)
| Mutation | Author tests | Descent/perf tests |
|---|---|---|
| Revert parser trailing-slash | ✅ still pass | ❌ pruning gone (node_modules re-walked) |
Remove childrenIgnored |
❌ passes ignore callbacks fails |
❌ real GlobTool descends |
Minor suggestion (non-blocking)
The 4 new tests assert correctness (already guaranteed by the pre-existing post-filter) and wiring (ignore callbacks are functions), but none would fail if traversal pruning silently regressed — e.g. reverting the trailing-slash fix, or childrenIgnored: () => false, keeps all current tests green while losing the entire speedup. Consider adding one descent-observing test (inject a readdir-counting fs, or spy shouldIgnoreFile and assert no path under the pruned dir is queried) so the performance guarantee has a regression guard. Optional — the change itself is correct as-is.
🇨🇳 中文版（完整对应）
✅ 本地端到端验证 —— 合并参考
我在隔离 worktree 里检出 PR head 2494009 并构建，驱动的是真实的 glob 遍历 + 真实的 FileDiscoveryService/GitIgnoreParser/QwenIgnoreParser（非 mock）。相对 origin/main 的净 diff 恰好就是这 5 个文件。依赖版本：glob@10.5.0、path-scurry@1.11.1、ignore@5.3.2。
结论：正确、可安全合并。 该改动与旧的「仅后置过滤」路径行为完全等价，带来真实的遍历剪枝提速，且不可能过度剪枝。文末有一条可选的测试覆盖建议（不阻塞合并）。
既有测试（真实代码，vitest alias 到 src）
| 套件 | 结果 |
|---|---|
glob.test.ts（含 4 个新用例） |
52 / 52 通过 |
gitIgnoreParser + qwenIgnoreParser + fileDiscoveryService |
47 / 47 通过 |
对 4 个改动文件跑 tsc --noEmit |
0 错误（另有 23 个 provider/model 无关报错，是环境 split-brain 遗留 —— 在 merge-base 上数量完全一致，本 PR 一个都没新增） |
正确性 —— 行为等价 & 不过度剪枝
我用真实 glob带 vs 不带 ignore 回调各跑一遍，并对两者都套用真实后置过滤，覆盖 6 个刁钻场景（根锚定 /dist vs 嵌套 src/dist；node_modules/；glob 父目录下的取反 build/** + !build/keep；目录父级下的取反 build/ + !build/keep；被忽略目录内用嵌套 .gitignore 尝试重新包含；未被忽略目录内的嵌套 .gitignore）。每个场景最终结果集都完全一致 —— 剪枝只会移除后置过滤本来也会移除的东西。
目录剪枝唯一的真实风险是过度剪枝（丢掉被 !取反 重新包含的文件）。这里不会发生，因为两条路径都调用同一个 ignore.ignores()，它实现了 git 规则 —— 「父目录被排除时，文件无法被重新包含」。我对不变式 ignores(dir/) === true ⟹ 其下所有后代都被忽略 做了暴力枚举：280 个 (目录, 文件) 组合 × 21 组刁钻模式（**、?、[0-9]、路径中段通配、嵌套取反），0 个违例。锚定（/dist ≠ src/dist）、取反、嵌套 ignore 文件、.git 目录均表现正确。
性能 —— 剪枝真实生效且承重
childrenIgnored 在遍历过程中剪枝，被忽略的子树根本不会被枚举。两种方式证明：
readdir计数器（向 glob 注入自定义fs）：开启剪枝时node_modules从不被打开；关闭时则被完整读取。- 真实
GlobToolInvocation（对shouldIgnoreFile打桩）：node_modules/被childrenIgnored查询一次并返回true，因此更深的路径一次都没被查询（未下钻）。
含 1500 个被忽略文件（node_modules/、dist/）+ 20 个源码文件的合成目录树：
| 开启剪枝 | 关闭 | |
|---|---|---|
readdir 调用 |
22 | 383（少 17.4 倍） |
| glob 返回的原始条目 | 21 | 1521 |
| 墙钟耗时 | 5.6 ms | 44.7 ms（约快 8 倍） |
两个 parser 里的「保留结尾斜杠」修复正是关键：ignores('node_modules') 是 false，而 ignores('node_modules/') 是 true，所以仅目录模式只有在用结尾 / 标记目录时才会剪枝。变异回退该修复后，剪枝全部失效（glob 重新枚举 node_modules）但结果仍正确 —— 证明它对性能承重、对正确性不承重。
变异矩阵（证明每处改动都承重）
| 变异 | 作者的测试 | 下钻/性能测试 |
|---|---|---|
| 回退 parser 结尾斜杠 | ✅ 仍通过 | ❌ 剪枝消失（node_modules 被重新遍历） |
移除 childrenIgnored |
❌ passes ignore callbacks 挂 |
❌ 真实 GlobTool 会下钻 |
小建议（不阻塞）
4 个新测试断言的是正确性（既有后置过滤本就保证）和接线（ignore 回调是函数），但没有一个会在遍历剪枝悄悄失效时挂掉 —— 例如回退结尾斜杠修复、或把 childrenIgnored: () => false，所有现有测试仍全绿，而整个提速已丢失。建议补一个观测下钻的测试（注入计数 readdir 的 fs，或对 shouldIgnoreFile 打桩、断言被剪枝目录之下的路径一次都没被查询），给性能保证加一道回归护栏。可选 —— 改动本身已正确。
Verified locally against real glob@10.5.0 traversal at head 2494009; scenarios, mutation matrix, invariant sweep, and perf numbers reproduced from an isolated build. This is a verification report, not an approval gate.
✅ Verification report — real-runtime E2EVerdict: looks good to merge. The performance claim reproduces cleanly (ignored subtrees are pruned during traversal), correctness is byte-identical to Verified head MethodI checked the PR head out into a worktree and drove the real 1) Performance — the core claim ✔Real tree with 10,000 gitignored files under
The 2) Correctness — zero regression ✔Because the post-filter is retained as the source of truth, output is identical between base and PR for every scenario — the pruning only has to avoid over-pruning. Ran the real tool on real FS (base vs PR, both correct):
A hand-rolled gitignore→glob pattern conversion (the alternative the PR explicitly avoids) would break the anchored and negation cases; delegating to the real ignore engine does not. 3) Why the parser trailing-slash change is needed, and safe ✔Raw Decomposition (isolated by reverting one file at a time): 4) A regression I found on the earlier revision — now fixed ✔On the revision I first tested ( The mid-review commit
Pruning win is unaffected (still 0 stats / ~32 ms on the 10k tree). The same commit also wraps the traversal check in a fail-open Test-suite notes
Minor, non-blocking
🇨🇳 中文版（点击展开）✅ 验证报告 — 真实运行时 E2E结论：可以合并。 性能优化（在遍历过程中剪掉被忽略的目录，而不是遍历完再过滤）可稳定复现；在我尝试的所有场景中，输出与 验证 head 方法将 PR head 检出到独立 worktree，驱动真实的 1）性能 — 核心诉求 ✔真实目录树：
2）正确性 — 零回归 ✔由于后置过滤（post-filter）仍作为最终依据，base 与 PR 的输出完全一致——剪枝只需保证不会过度剪枝。在真实文件系统上运行真实工具（base vs PR，均正确）：
若采用手写的 gitignore→glob 模式转换（PR 明确避免的方案），锚定与反选场景会出错；委托给真实 ignore 引擎则不会。 3）为什么需要「保留结尾斜杠」这个 parser 改动，且它是安全的 ✔原生 分解（逐个文件回退隔离）： 4）我在较早版本上发现的一个回归 — 现已修复 ✔在我最初测试的版本（ 评审期间的提交
剪枝收益不受影响（10k 树上仍为 0 次 stat / ~32 ms）。同一提交还给遍历检查加了 fail-open 的 测试套件
次要、不阻塞
Reproduction harness (real |
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running. The traversal-pruning approach is sound — delegating to FileDiscoveryService correctly preserves gitignore semantics, the ignorerCache memoization is a justified performance tradeoff, and the test suite provides solid coverage including callback wiring verification. LGTM. ✅
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Well-engineered performance optimization for glob traversal. The approach of delegating to the real .gitignore/.qwenignore ignore logic (rather than hand-rolling pattern conversion) correctly preserves anchoring, negation/re-inclusion, and nested ignore files. The getIgnorerForDir memoization is a clean improvement over per-call recompilation. Defensive error handling (fail-open in traversal callbacks) and external-dir exclusion are both correct. Tests cover the important edge cases including root-anchored patterns, negation re-inclusion, and the respectGitIgnore: false path.
Downgraded from Approve to Comment: CI still running (review-pr check is pending).
— qwen3-coder via Qwen Code /review
| for (const dir of dirsToVisit) { | ||
| const relativeDir = path.relative(this.projectRoot, dir); | ||
| if (relativeDir) { | ||
| const normalizedRelativeDir = relativeDir.replace(/\\/g, '/'); |
There was a problem hiding this comment.
[Suggestion] The intermediate directory check in the for loop tests ig.ignores(normalizedRelativeDir) without appending a trailing /. Every entry in dirsToVisit is a directory, but the ignore library treats directory-only patterns differently: for a .gitignore pattern logs/, ig.ignores('logs') returns false while ig.ignores('logs/') returns true.
This means the ancestor-ignored break on the next line does not fire for directory-only patterns — the code descends into ignored directories and loads their .gitignore files (unnecessary I/O, and pollutes ignorerCache with entries built from inside ignored trees).
| const normalizedRelativeDir = relativeDir.replace(/\\/g, '/'); | |
| const normalizedRelativeDir = relativeDir.replace(/\\/g, '/') + '/'; |
— qwen3.7-max via Qwen Code /review
| isDirectory(): boolean; | ||
| }): boolean => { | ||
| try { | ||
| const relativePath = path.relative(projectRoot, entry.fullpath()); |
There was a problem hiding this comment.
[Suggestion] Redundant path round-trip on every traversal entry. path.relative(projectRoot, entry.fullpath()) computes a relative path here, then shouldIgnoreFile → GitIgnoreParser.isIgnored internally calls path.resolve(this.projectRoot, filePath) (undoing it) followed by path.relative(this.projectRoot, resolved) (redoing it). Every entry during traversal pays for path.relative × 2 + path.resolve × 1.
Consider either (a) passing the already-computed absolute path into shouldIgnoreFile via an optional parameter, or (b) inlining the getIgnorerForDir + ig.ignores() call directly since isTraversalIgnored already has both the absolute and relative paths.
— qwen3.7-max via Qwen Code /review
| normalizedPath += '/'; | ||
| } | ||
|
|
||
| if (normalizedPath.startsWith('/') || normalizedPath === '') { |
There was a problem hiding this comment.
[Suggestion] The trailing-slash preservation logic added here (and mirrored in qwenIgnoreParser.ts:184-188) has no direct unit test coverage. No test in gitIgnoreParser.test.ts or qwenIgnoreParser.test.ts calls isIgnored('node_modules/') against a .gitignore containing node_modules/. The new code path is only exercised indirectly through glob integration tests.
A regression in the parser-level directory-pattern matching would be masked by glob's post-filter fallback, making it hard to diagnose. Consider adding direct unit tests:
expect(parser.isIgnored('node_modules/')).toBe(true)with patternnode_modules/expect(parser.isIgnored('node_modules')).toBe(false)for the same pattern (verifying trailing/is what triggers the match)
— qwen3.7-max via Qwen Code /review
… ancestor check
The ignore library requires trailing '/' on the path to match directory-only
patterns like 'logs/'. Without it, ig.ignores('logs') returns false for
pattern 'logs/', causing unnecessary descent into ignored directories.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
The traversal-pruning approach is sound — delegating to FileDiscoveryService.shouldIgnoreFile correctly preserves gitignore semantics (anchoring, negation, nested ignore files). The ignorerCache memoization is a justified per-directory optimization, and the trailing-slash convention for directory-only patterns is implemented consistently across both parsers. Build and all 86 tests in the affected files pass.
— qwen3.7-max via Qwen Code /review
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Previous critical traversal-pruning issues remain resolved at the current head. I found no new critical blocker.
|
@qwen-code /triage |
|
Thanks for the PR @minmax! Template: Clear "What" and "Why" sections with solid technical detail. Missing the "Reviewer Test Plan" and "Risk & Scope" sections from the PR template — the "Testing" section covers the intent but doesn't match the template headings. Not blocking since a maintainer has already engaged with the substance. Problem: Real and well-documented. The glob tool fully enumerates ignored trees ( Direction: Performance optimization of a core tool by delegating to the existing ignore engine — squarely within scope. Avoids the lossy gitignore→glob pattern conversion problem (anchoring, negation, nested ignore files). No CHANGELOG precedent needed for internal tool performance. Approach: Scope is tight and minimal — traversal pruning hooks plus ignorer memoization. No unnecessary abstractions or scope creep. Post-filter stays as the source of truth, so the change is purely additive. Moving to code review. 🔍 中文说明感谢 @minmax 的 PR！ 模板：PR 有清晰的"What"和"Why"部分，技术细节扎实。缺少 PR 模板 中的"Reviewer Test Plan"和"Risk & Scope"部分——"Testing"部分覆盖了测试意图但标题不匹配。由于 maintainer 已就实质内容进行了审查，不作为阻断项。 问题：问题真实且有据可查。glob 工具在后过滤之前会完整枚举被忽略的目录树（ 方向：通过委托给现有忽略引擎来优化核心工具的性能——完全在范围内。避免了有损的 gitignore→glob 模式转换问题（锚定、取反、嵌套忽略文件）。内部工具性能优化不需要 CHANGELOG 先例。 方案：范围紧凑且最小化——遍历修剪钩子加忽略器缓存。没有不必要的抽象或范围蔓延。后过滤保持为最终权威，因此变更纯粹是增量性的。进入代码审查。🔍 — Qwen Code · qwen3.7-max |
Code ReviewMy independent approach for this problem: pass The PR's implementation matches this approach closely and adds good details I would have missed:
Tests — 6 new tests, all meaningful: root-anchored patterns, directory pruning, callback wiring verification (addresses the "tests don't verify One minor note: TestingThis is a pure performance optimization — glob's output is identical before and after (post-filter remains the source of truth). The relevant verification is unit tests, not TUI before/after: Build compiles cleanly (no new warnings from the PR). 中文说明代码审查我对此问题的独立方案：向 glob 库传递 PR 的实现与此方案高度一致，并添加了一些我可能遗漏的细节：
测试 — 6 个新测试全部有意义：根锚定模式、目录修剪、回调传递验证、 一个小建议： 测试这是纯性能优化——glob 的输出在优化前后完全相同（后过滤器保持为最终权威）。相关验证是单元测试，而非 TUI before/after：全部 101 个测试通过，构建编译干净。 — Qwen Code · qwen3.7-max |
ReflectionThis is a clean, well-scoped performance optimization. The approach — delegate traversal pruning to the real ignore engine rather than hand-rolling a pattern conversion — is exactly right. A lossy conversion would silently break anchored patterns and drop negations; the PR avoids that trap entirely by reusing The memoization in The tests are thorough and the right kind of thorough: they verify not just "correct output" but also "the optimization is actually wired up" (the callback-passing test). The external-directory safety test is a good catch — ignore rules should not apply outside the project root. 101 tests pass, build is clean, no behavioral regression. Status: qqqys approved after confirming prior critical issues were resolved. wenshao's CHANGES_REQUESTED appears stale — both critical findings (trailing Approving — this is ready to ship once the stale CHANGES_REQUESTED is cleared. ✅ 中文说明反思这是一个干净、范围合理的性能优化。方案——将遍历修剪委托给真正的忽略引擎而不是手工编写模式转换——完全正确。有损转换会默默破坏锚定模式和取反；PR 通过复用
测试全面且方式正确：不仅验证"正确输出"，还验证"优化确实已连接"（回调传递测试）。外部目录安全测试也是好的发现——忽略规则不应在项目根目录之外生效。 101 个测试通过，构建干净，无行为回归。 状态：qqqys 在确认之前的关键问题已解决后批准了。wenshao 的 CHANGES_REQUESTED 看起来已过时——两个关键发现（目录专用模式的尾部 批准——一旦过时的 CHANGES_REQUESTED 被清除即可合并。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
5 participants
What this PR does
Applies
.gitignore/.qwenignorerules during glob traversal instead ofonly post-filtering the collected results. The
globtool now passes anignoreobject (childrenIgnored/ignored) that delegates toFileDiscoveryService.shouldIgnoreFile, so ignored directories are prunedwhile walking.
Why
node_modules, build output) are nolonger fully enumerated just to be discarded.
(anchoring like
/dist, negation / re-inclusion!keep, nested ignorefiles) are preserved. A lossy gitignore→glob pattern conversion would
over-ignore anchored patterns and drop negations.
Notes
reporting); it rarely has anything left to remove after pruning.
respectGitIgnore: falsestill searches everything..gitremains handled byGitIgnoreParser.Testing
packages/core/src/tools/glob.test.ts— added cases: root-anchored/distdoes not over-ignore nested
src/dist; a gitignorednode_modulesis prunedduring traversal; negation re-inclusion is honored. Full glob suite green.
Closes #6121