Skip to content

perf: add single-walk file filter that prunes ignored directories - #661

Draft
apzuk3 wants to merge 1 commit into
mainfrom
perf/single-walk-file-filter
Draft

perf: add single-walk file filter that prunes ignored directories#661
apzuk3 wants to merge 1 commit into
mainfrom
perf/single-walk-file-filter

Conversation

@apzuk3

@apzuk3 apzuk3 commented Jul 21, 2026

Copy link
Copy Markdown

Description

Adds GetFilteredFilesSingleWalk, which traverses the directory tree once (instead of the two-pass GetAllFiles + GetFilteredFiles pipeline) and prunes wholly-excluded directories such as node_modules and .git rather than globbing every file beneath them — where almost all the traversal time on real projects was being spent.

A directory is pruned only when a rule excludes the directory itself (node_modules, /node_modules/, **/dist), never when a rule excludes only its contents (src/*, obj/**) or when the directory contains its own ignore file, since those may be re-included by a ! negation. Per-file emission still uses the full rule set, so output is identical to the original pipeline.

Includes equivalence, negation-edge-case, and pruning tests plus a benchmark against the old pipeline.

This is a re-open of #657 (originally authored by @z4ce) from a branch on the snyk org so CI runs with the proper context. Related ticket: COIN-2536.

Needed by snyk/code-client-go#164, which will pin this once merged.

Checklist

  • Tests added and all succeed (make test, incl. -race)
  • Linted (make lint)
  • go build ./... clean

Note

Medium Risk
Changes which files are scanned if pruning or negation edge cases are wrong, but behavior is heavily regression-tested against the existing pipeline and real-repo edge cases.

Overview
Adds GetFilteredFilesSingleWalk as a faster alternative to the two-pass GetAllFilesGetRulesGetFilteredFiles flow: one tree walk, ignore rules folded in as .gitignore / .dcignore / .snyk are encountered, and fs.SkipDir used to skip wholly excluded dirs (e.g. node_modules, **/dist) instead of glob-matching every file under them.

buildGlobs is refactored through globsForIgnoreFile so ignore parsing is shared. Pruning uses a separate dirMatcher built only from whole-directory rules (not src/* / obj/**), skips dirs that contain their own rule files, and does not treat .snyk as a source of prune rules. Final inclusion still uses the full accumulated glob set in emitCandidates (parallelized like the old path) so results stay aligned with the legacy pipeline.

New tests assert equivalence with the old pipeline, negation / content-only-ignore edge cases, and node_modules pruning; a benchmark compares old vs single-walk on a large ignored tree.

Reviewed by Cursor Bugbot for commit a8bac65. Bugbot is set up for automated code reviews on this repo. Configure here.

Add GetFilteredFilesSingleWalk, which traverses the directory tree once
instead of twice (GetAllFiles + GetFilteredFiles) and prunes wholly-
excluded directories such as node_modules and .git rather than globbing
every file beneath them — where almost all the time on real projects was
spent.

A directory is pruned only when a rule excludes the directory itself
(node_modules, /node_modules/, **/dist), never when a rule excludes only
its contents (src/*, obj/**) or when the directory contains its own
ignore file, since those may be re-included by a ! negation. Per-file
emission still uses the full rule set, so output is identical to the
original pipeline.

Includes equivalence, negation-edge-case, and pruning tests plus a
benchmark against the old pipeline.
@apzuk3
apzuk3 requested review from a team as code owners July 21, 2026 11:09
@snyk-io

snyk-io Bot commented Jul 21, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-io

snyk-io Bot commented Jul 21, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues
Secrets 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Platform-Inconsistent Rule Detection 🟡 [minor]

The check for content-only rules (rules ending in /* or /**) in dirExclusionGlob only looks for forward slashes. If a user provides a rule using backslashes (e.g., obj\**) on Windows, the check will fail to identify it as a content-only rule. This could lead to obj being incorrectly pruned if it matches an exclusion rule, preventing nested negations from working correctly on Windows systems.

if strings.HasSuffix(r, "/*") || strings.HasSuffix(r, "/**") {
	return "", false
}
Missing Error Propagation 🟡 [minor]

In GetFilteredFilesSingleWalk, if filepath.WalkDir returns an error (other than fs.SkipDir), the error is logged but not propagated to the caller. The filteredFilesCh is closed normally, which might lead a consumer to believe the file list is complete when it was actually truncated due to a filesystem error (e.g. permission denied on a non-ignored subtree).

if err != nil {
	fw.logger.Error().Msgf("walk dir failed: %v", err)
}
📚 Repository Context Analyzed

This review considered 6 relevant code sections from 4 files (average relevance: 1.00)

🤖 Repository instructions applied (from AGENTS.md)

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a8bac65. Configure here.

Comment thread pkg/utils/file_filter.go
if path != fw.path &&
dirMatcher.MatchesPath(path) &&
!fw.dirContainsRuleFile(path, ruleFileSet) {
return fs.SkipDir

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pruning skips same-file negations

High Severity

Directory pruning via fs.SkipDir drops entire subtrees before the final per-file pass, but dirExclusionGlob still treats many ignore lines (including trailing-slash directory rules like node_modules/ and plain node_modules) as safe whole-directory exclusions. Negated re-includes in the same .gitignore only apply to paths still visited by the old pipeline, so those files can be omitted from the scan output entirely.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a8bac65. Configure here.

Comment thread pkg/utils/file_filter.go
// applies in this directory and any subdirectory
glob = filepath.ToSlash(filepath.Join(base, "**", r))
}
glob = escapeSpecialGlobChars(glob)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Dir globs omit base quoting

Medium Severity

dirExclusionGlob builds pruning globs from an unquoted baseDir, while parseIgnoreRuleToGlobs applies regexp.QuoteMeta to the ignore file’s directory before compiling file rules. On project paths containing regex metacharacters (parentheses, plus, braces, etc.), dirMatcher can disagree with the final fileMatcher and prune the wrong subtree or skip pruning incorrectly.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a8bac65. Configure here.

@apzuk3
apzuk3 marked this pull request as draft July 21, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants