Skip to content

fix: ignored-parent semantics#658

Open
robertolopezlopez wants to merge 1 commit into
mainfrom
fix/CLI-1526
Open

fix: ignored-parent semantics#658
robertolopezlopez wants to merge 1 commit into
mainfrom
fix/CLI-1526

Conversation

@robertolopezlopez

@robertolopezlopez robertolopezlopez commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Description

Prevents ignore files inside an already ignored directory from re-including descendant files.

GAF collected rules from every nested .gitignore, .dcignore, and .snyk file before filtering paths.

Example

.gitignore:                         node_modules
node_modules/pkg/.gitignore:        !index.js
node_modules/pkg/index.js

The nested negation incorrectly re-included index.js. Git does not process ignore files beneath an ignored parent directory.

Solution

While building ignore rules, ignore files whose path has been already ignored.

Testing

  • Unit tests: new and modified existing.
  • Verified code-client-go and snyk-ls consumers locally.
  • make format, make generate, make lint, make test

Related task

CLI-1526

Checklist

  • Tests added and all succeed (make test)
  • Regenerated mocks, etc. (make generate)
  • Linted (make lint)
  • Test your changes work for the CLI
    1. Clone / pull the latest CLI main.
    2. Run go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMIT in the cliv2 directory.
      • Tip: for local testing, you can uncomment the line near the bottom of the CLI's go.mod to point to your local GAF code.
    3. Run go mod tidy in the cliv2 directory.
    4. Run the CLI tests and do any required manual testing.
    5. Open a PR in the CLI repo now with the go.mod and go.sum changes.
    • Once this PR is merged, repeat these steps, but pointing to the latest GAF commit on main and update your CLI PR.

Note

Medium Risk
Changes which files are scanned for Snyk/CLI uploads by altering ignore semantics; behavior shifts for repos that relied on nested negations under excluded trees, but matches Git and reduces incorrect inclusions.

Overview
Aligns ignore-rule collection with Git by not reading .gitignore, .dcignore, or .snyk files whose path is already covered by rules gathered from earlier ignore files.

In buildGlobs, a matcher is built incrementally from accumulated globs. Each candidate ignore file is skipped when that matcher already matches its path; otherwise its rules are merged and the matcher is refreshed. That stops nested negations (e.g. node_modules at the root plus !index.js under node_modules/pkg/) from re-including files Git would still exclude.

Tests add node_modules / .snyk parent cases and tighten expectations for ignored folders with negation rules so descendants under an ignored parent stay excluded.

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

@snyk-io

snyk-io Bot commented Jul 16, 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 16, 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.

@robertolopezlopez
robertolopezlopez marked this pull request as ready for review July 16, 2026 08:24
@robertolopezlopez
robertolopezlopez requested review from a team as code owners July 16, 2026 08:24
@snyk-pr-review-bot

This comment has been minimized.

Comment thread pkg/utils/file_filter.go
parsedRules := parseIgnoreFile(content, filepath.Dir(ignoreFile))
globs = append(globs, parsedRules...)
}
globPatternMatcher = gitignore.CompileIgnoreLines(globs...)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Issue: This is a proper performance issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compiling is a very CPU cost intensive operation

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@PeterSchafer it seems it is a real issue as nested .gitignore are treated as a flat list. It diverges from the .gitignore expected behavior

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably this current approach is not good as you said, but we might wanna check what is being ignored and ignore nested ones

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe a strategy building a "tree" where we can prune the branches is the best approach

Comment on lines +609 to +621
{
name: "ignore file below ignored parent cannot reinclude files",
files: map[string]string{
".gitignore": "node_modules\n",
"node_modules/pkg/.gitignore": "!index.js\n",
"node_modules/pkg/index.js": "x",
"node_modules/pkg/other.js": "x",
"src/index.js": "x",
},
ruleFiles: []string{".gitignore"},
excluded: []string{"node_modules/pkg/index.js", "node_modules/pkg/other.js"},
kept: []string{"src/index.js"},
},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it seems for .gitignore inside node_modules is not something common / expected to have. But this reveals a potential issue regarding nested directories / experimental repos inside real repos, etc.

Nice finding!

@PeterSchafer

Copy link
Copy Markdown
Contributor

Suggestion: Let's understand the problem better and the possible solutions. This is a crucial logic that needs to be carefully handled.

@robertolopezlopez
robertolopezlopez deleted the fix/CLI-1526 branch July 20, 2026 13:35
@robertolopezlopez
robertolopezlopez restored the fix/CLI-1526 branch July 20, 2026 13:46
@robertolopezlopez

Copy link
Copy Markdown
Contributor Author

closed by mistake, reopening

@snyk-pr-review-bot

Copy link
Copy Markdown

PR Reviewer Guide 🔍

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

Performance Regression 🟡 [minor]

The buildGlobs function now recompiles the entire ignore pattern matcher inside a loop for every ignore file found. For large repositories with many nested ignore files (e.g., a monorepo or a project with many node_modules sub-packages), this results in O(N^2) complexity for rule compilation. Since this is a framework (GAF) used by CLI and IDE plugins, this could lead to noticeable lag during file scanning. Consider optimizing by only recompiling when the directory depth changes or using a more incremental approach if supported by the underlying library.

for _, ignoreFile := range ignoreFiles {
	if globPatternMatcher.MatchesPath(ignoreFile) {
		continue
	}

	var content []byte
	content, err := os.ReadFile(ignoreFile)
	if err != nil {
		return nil, err
	}

	if filepath.Base(ignoreFile) == ".snyk" { // .snyk files are yaml files and should be parsed differently
		parsedRules := fw.parseDotSnykFile(content, filepath.Dir(ignoreFile))
		globs = append(globs, parsedRules...)
	} else { // .gitignore, .dcignore, etc. are just a list of ignore rules
		parsedRules := parseIgnoreFile(content, filepath.Dir(ignoreFile))
		globs = append(globs, parsedRules...)
	}
	globPatternMatcher = gitignore.CompileIgnoreLines(globs...)
}
📚 Repository Context Analyzed

This review considered 2 relevant code sections from 1 files (average relevance: 0.60)

🤖 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 fe47ad7. Configure here.

Comment thread pkg/utils/file_filter.go
globPatternMatcher := gitignore.CompileIgnoreLines()
for _, ignoreFile := range ignoreFiles {
if globPatternMatcher.MatchesPath(ignoreFile) {
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Skipped root policy rule files

Medium Severity

buildGlobs skips reading a rule file when MatchesPath is true for that file’s path. That also drops later .snyk or .dcignore files at the repo root when an earlier .gitignore ignores them, even though their parent directory is not excluded. Those policy excludes no longer enter globs, so scanning can miss paths that used to be filtered.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fe47ad7. Configure here.

Comment thread pkg/utils/file_filter.go
parsedRules := parseIgnoreFile(content, filepath.Dir(ignoreFile))
globs = append(globs, parsedRules...)
}
globPatternMatcher = gitignore.CompileIgnoreLines(globs...)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Recompiles matcher every ignore file

Medium Severity

Each loop iteration in buildGlobs calls gitignore.CompileIgnoreLines on the entire growing globs slice. CompileIgnoreLines is expensive, so repos with many nested ignore files pay roughly quadratic CPU during rule collection compared to the previous single-pass append.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit fe47ad7. Configure here.

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.

3 participants