feat: file filter doesn't exclude tracked files though .gitignore rules - #7085
feat: file filter doesn't exclude tracked files though .gitignore rules#7085CatalinSnyk wants to merge 1 commit into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
8774cf8 to
4869748
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
/verification results
Auto-approval check: This PR is not eligible for automatic approval — it is not a pure dependency bump. In addition to bumping github.com/snyk/go-application-framework in cliv2/go.mod / cliv2/go.sum / cliv2-private/go.mod / cliv2-private/go.sum, it also adds two new Jest acceptance test files (650 lines) and modifies test/acceptance/fake-server.ts. Per policy, this automation only auto-approves changes that touch only dependency manifests, so this run leaves the PR unapproved for human review.
Four review lenses (semantic analysis, adversarial review, security scan, independent code review) were run against the diff and the actual pinned go-application-framework commit (9d2ac0263a36) source.
Critical
- A new test asserts the wrong expected outcome for the exact dependency version this PR pins, and will fail in CI as written. See inline comment on
gitignore-tracked-files.spec.ts.
Should Fix
- The tracked-files feature-flag env var name (
INTERNAL_SNYK_GITIGNORE_RESPECT_TRACKED_FILES_ENABLED) is duplicated as an independent magic string in both new spec files with no shared source of truth — a future rename in GAF could silently make one file's override inert (Viper ignores unknown env vars rather than erroring). gitignore-flag-wiring.spec.ts: the "does not even ask the backend to evaluate the flag" test only asserts onflags, not on scan success (code/files) — a regression that crashes the scan entirely would still make this assertion vacuously pass.gitignore-tracked-files.spec.ts's "flag is a no-op" invariance test compareson.filestooff.fileswithout first asserting either scan actually completed (unlike the siblingdescribe.eachblock, which does check exit codes) — a total scan failure on both sides would trivially pass ([] === []).
Suggestions
- The
uploadedFiles()bundle-decoding helper is duplicated near-verbatim across both new spec files; consider extracting a shared test util. - In the "scanning a subdirectory" case, the root-level
control.jsandoutside.gen.jsfixture entries sit outside the scannedsrc/subdirectory and are never exercised by any assertion — likely dead fixture data. - (Informational, out of scope for this PR) One new test case explicitly documents and pins a pre-existing
.gitignore: '!.git/**'leak of.gitinternals when the flag is off — the test comment already notes this is tracked separately, flagging here only for visibility since this PR's rollout increases how often user.gitignorecontent drives scan-set inclusion.
What went well
- The dependency bump is applied consistently across both
go.mod/go.sumpairs, hashes verify against the actual module content, andgo build/go vetsucceed against the bumped dependency. - The new fixtures show a genuinely deep and largely-correct understanding of the underlying filter algorithm — most of the trickier cases (nested
.gitignore, nested subdirectories,.snyk/.dcignoreprecedence) were independently traced against the real pinned implementation and matched exactly. - No secrets, injection, or path-traversal issues found; the
fake-server.tsroute change is test-only and additive.
No action taken on approval status (not a dependency-only change) and no changes-requested review was filed.
Sent by Cursor Automation: Automatic PR verification
|
|
||
| // GAF binds config keys to upper-cased env vars, bypassing the remote flag. | ||
| const TRACKED_FILES_FLAG_ENV = | ||
| 'INTERNAL_SNYK_GITIGNORE_RESPECT_TRACKED_FILES_ENABLED'; |
There was a problem hiding this comment.
Should Fix: this env-var name is duplicated independently in gitignore-flag-wiring.spec.ts (as FLAG_ENV) with no shared source of truth. If this config key is renamed upstream in GAF (these keys have already been renamed once, per the TrackedFilesRollout flag), a future change that updates one file but misses the other leaves that file's override silently inert (Viper ignores unrecognized env vars rather than erroring) — defeating the point of a regression test. Consider hoisting into a shared test util imported by both spec files.
| previewFeatures: true, | ||
| }); | ||
|
|
||
| expect(flags).not.toContain(REMOTE_FLAG_NAME); |
There was a problem hiding this comment.
Should Fix: this only asserts flag absence, not that the scan actually completed. If a regression made code test exit before ever reaching the network layer, flags would be [] and this assertion would pass vacuously without exercising anything. Consider also asserting code/files here, as the sibling tests at lines 168-175 and 186-197 do.
| const off = await scan(testCase, false); | ||
| const on = await scan(testCase, true); | ||
|
|
||
| expect(on.files).toEqual(off.files); |
There was a problem hiding this comment.
Should Fix: this invariance check compares on.files to off.files without first confirming either scan actually completed successfully. If both scans failed before uploading a bundle, on.files and off.files would both be [] and this would pass trivially. The sibling describe.each block above (lines ~421-433) guards against exactly this with expect([EXIT_CODE_SUCCESS, EXIT_CODE_NO_SUPPORTED_FILES]).toContain(code) — consider adding the same guard here for both off and on.
4869748 to
56e7e95
Compare
There was a problem hiding this comment.
Verification results
Ran the full 4-agent verification (semantic analysis, adversarial review, security scan, independent code review) against the latest push (head 56e7e95f).
Not auto-approved. Per policy, only a PR whose change is exclusively a dependency bump (e.g. go.mod/go.sum only) is eligible for automatic approval. This PR also adds ~650 lines of new Jest acceptance tests (gitignore-flag-wiring.spec.ts, gitignore-tracked-files.spec.ts) and a route change in fake-server.ts, so it's out of scope for auto-approval — leaving this as a comment-only review instead.
Previous review resolved
The Critical finding from the prior run on this PR (cross-source .gitignore/.dcignore negation contradicting the pinned GAF behavior) is now fully addressed: the pinned go-application-framework commit moved from 9d2ac0263a36 → e9b1d30707cd, which includes upstream commit f4beeebc99c3 ("fix: preserve untracked ignore behavior") landed specifically to fix this. I verified the current pkg/utils/file_filter.go logic directly and confirmed the NEGATION: ... negated across ignore sources test case now correctly reflects actual behavior. I've resolved that thread. The other three open threads from the previous run (FLAG_ENV duplication, flag-absence-only assertion, unguarded no-op invariance check) still stand — not addressed by this push.
New findings this round
See inline comments for Should-Fix items. Also noted, non-blocking:
- The pinned GAF commit isn't yet on
go-application-framework'smain(expected — coordinated cross-repo landing per PR intent, not a blocker). gitignore-flag-wiring.spec.ts:149—'defaults to the legacy behaviour when the backend does not know the flag'actually exercises an explicit backendfalse, not a genuinely-unknown flag (fake server has no "omit flag" mode); consider renaming or adding that mode.- Both new spec files duplicate
uploadedFiles()/bundle-decoding and server-bootstrap logic; consider extracting a shared test util. gitignore-tracked-files.spec.ts:438-442'snoOpCasesfilter derives "no-op" fromtracked/untrackshape rather than an explicit field — fragile if a future case partially untracks files.gitignore-tracked-files.spec.ts:263-274'sscanSubdircase reuses the namecontrol.jsfor two different files (root-level, out of scan scope, vs.src/control.js), which is easy to misread.- The PR title has a typo ("though" → "through") that will become the squash-commit message per this repo's conventions.
No Critical issues and no security findings (SCA/secrets/injection review of the new fixtures and the dependency bump found nothing exploitable).
Sent by Cursor Automation: Automatic PR verification
| beforeEach(configureServers); | ||
|
|
||
| async function buildFixture(testCase: FilterCase): Promise<string> { | ||
| const root = fs.mkdtempSync(join(os.tmpdir(), 'snyk-gitignore-')); |
There was a problem hiding this comment.
Should Fix: temp git fixtures created here (fs.mkdtempSync) are never cleaned up. With 19 cases × 2 flag states in the main block plus the noOpCases invariance block, this leaves dozens of full git repos behind in the OS temp dir on every run, with no afterEach/afterAll removing them — a new pattern for this suite (no other acceptance spec uses mkdtempSync without cleanup).
Fix: track created roots and remove them, e.g.:
const createdRoots: string[] = [];
// in buildFixture: createdRoots.push(root);
afterEach(() => {
for (const root of createdRoots.splice(0)) {
fs.rmSync(root, { recursive: true, force: true });
}
});|
|
||
| /** A repo where tracked.js is both git-tracked and matched by .gitignore. */ | ||
| async function buildFixture(): Promise<string> { | ||
| const root = fs.mkdtempSync(join(os.tmpdir(), 'snyk-ff-')); |
There was a problem hiding this comment.
Should Fix: same temp-directory leak as gitignore-tracked-files.spec.ts — fs.mkdtempSync here is never cleaned up across the ~9 scans in this file. Same fix: collect and fs.rmSync(root, { recursive: true, force: true }) in afterEach/afterAll.
| { | ||
| // Pins current behaviour. The flag-off result is a pre-existing leak: `!.git/**` | ||
| // negates the built-in `**/.git/**` default. Tracked separately. | ||
| name: 'GIT: a .gitignore negation of .git/** leaks repository internals with the flag off', |
There was a problem hiding this comment.
Should Fix: this case deliberately pins a real leak (!.git/** negating the built-in .git exclusion, which can expose .git/config/credentials/history with the flag off) and says "Tracked separately" — but with no ticket reference, unlike the adjacent convention of citing a ticket for related flags (e.g. CLI-1733 in cliv2/pkg/core/workflows.go). Without a concrete issue link this is easy to lose track of post-merge.
Fix: add a ticket reference, e.g. // See CLI-XXXX alongside the "Tracked separately" comment.
|
|
||
| // Called before every scan, not every test, so a test that scans twice sees only its | ||
| // own requests. | ||
| function configureServers() { |
There was a problem hiding this comment.
Should Fix: this file never pins clientFileFilterGitignore_MetaCharFix, unlike its sibling gitignore-flag-wiring.spec.ts:71, which explicitly sets it and documents why ("enable it here so rule parsing matches production rather than falling back to the legacy parser"). Per cliv2/pkg/core/workflows.go:49-58, that flag is only force-enabled when INTERNAL_PREVIEW_FEATURES_ENABLED is true for the binary under test — which this file never sets or asserts either. Every CORE/NEGATION case here that depends on glob/metacharacter parsing (*.log.js, build/, *.gen.js, !gen/keep.js) is silently relying on an ambient default with nothing tying the two together; if that default ever changes, or the suite runs against a non-preview binary, these assertions start validating the legacy parser instead of the intended one with no clear failure signal.
Fix: mirror the sibling spec — explicitly set the flag (and/or pin INTERNAL_PREVIEW_FEATURES_ENABLED) in configureServers():
function configureServers() {
server.restore();
deepCodeServer.restore();
server.setOrgSetting('sast', true);
server.setFeatureFlag('clientFileFilterGitignore_MetaCharFix', true);
// ... existing setup
}| // Body: { data: { attributes: { flags: ["flag-name", ...] } } } | ||
| app.post('/hidden/orgs/:orgId/feature_flags/evaluation', (req, res) => { | ||
| // GAF derives the URL from the API URL, so the /api prefix may or may not be present. | ||
| const flagEvaluationPaths = [ |
There was a problem hiding this comment.
Suggestion: this hardcodes the /api prefix as a second literal path. Elsewhere in this file (e.g. other hidden-endpoint routes) the established convention for deriving the /api-prefixed variant is basePath.replace('v1', 'hidden') + '/...', which stays correct for whatever basePath a caller passes rather than only matching a basePath that literally starts with /api. Low priority since both new spec files use baseApi = '/api/v1' today, so this works as written — just flagging the latent fragility.
Fix (optional):
const flagEvaluationPaths = [
'/hidden/orgs/:orgId/feature_flags/evaluation',
basePath.replace('v1', 'hidden') + '/orgs/:orgId/feature_flags/evaluation',
];
56e7e95 to
51b9f47
Compare
There was a problem hiding this comment.
Verification results
Ran the full 4-agent verification (semantic analysis, adversarial review, security scan, independent code review) against the latest push (head 51b9f47f), which re-bumps go-application-framework from v0.11.0 to v0.11.1-0.20260805145855-e9b1d30707cd.
Not auto-approved. Per policy, a PR is only eligible for automatic approval when the change is exclusively a dependency bump (e.g. go.mod/go.sum only). This PR also adds ~650 lines of new Jest acceptance tests (gitignore-flag-wiring.spec.ts, gitignore-tracked-files.spec.ts) and a routing fix in fake-server.ts, so it's out of scope for auto-approval — leaving this as a comment-only review.
This push
Only cliv2/go.mod, cliv2/go.sum, cliv2-private/go.mod, cliv2-private/go.sum changed (the GAF pin moved to a newer commit). The test files and fake-server.ts are byte-for-byte unchanged from the last reviewed push, so every previously open thread from this automation still stands unaddressed — I've re-recorded that verdict on each rather than duplicating the comments here.
New finding this round
See the inline comment on gitignore-tracked-files.spec.ts — the noOpCases filter used for the invariance check silently excludes cases that are legitimate no-ops (the .snyk/.dcignore-veto cases), missing exactly the scenario most likely to regress silently.
Verified clean
go.mod/go.sumconsistency: bothcliv2andcliv2-privatepin the identical GAF pseudo-version with matching hashes; no stalev0.11.0entries; no new/changed transitive dependencies introduced by the bump (confirmed viago.sumdiff — only the GAF lines changed).- No hardcoded secrets or credentials in the diff (scanned with gitleaks; the literal test tokens match the pre-existing pattern used across dozens of other acceptance specs).
- Core filtering test matrix (
CORE/USER RULES/NEGATION/GITcases) traced against the actual pinned GAF source and confirmed correct. - No Critical issues and no exploitable security findings.
Non-blocking note: the GAF dependency is still pinned to an untagged pseudo-version rather than a released tag — acceptable for cross-repo coordinated landing per this PR's intent, but worth swapping for a tagged release before this ships in a stable CLI release.
CI: all currently-reporting checks pass (Danger, dependency check, GitGuardian, CLA, Snyk license/security/code); CircleCI's test_and_release was pending at review time.
Sent by Cursor Automation: Automatic PR verification
|
|
||
| // The feature is strictly additive: it may only ever un-ignore tracked files. | ||
| describe('the flag is a no-op when nothing relevant is tracked', () => { | ||
| const noOpCases = cases.filter( |
There was a problem hiding this comment.
Should Fix: this filter silently excludes cases that are legitimately no-ops. It's meant to select every case where the flag must be a no-op (per the comment above: "The feature is strictly additive"), but the USER RULES cases (e.g. "a .snyk exclusion still applies to a tracked, gitignored file" and "a .dcignore rule still applies to a tracked file") also have expectedOff === expectedOn — they're no-ops too, since .snyk/.dcignore exclusions override the tracked-file un-ignoring — but they carry a non-empty tracked array with no untrack, so this predicate silently drops them from the invariance check below. The main describe.each block still asserts the correct values directly for these cases, so there's no false pass/fail today, but the dedicated "no-op" invariant never exercises the scenario most likely to regress silently: a tracked file that's still correctly excluded by a higher-precedence rule.
Fix: derive "no-op" from the expected outcome rather than the tracked/untrack shape:
const noOpCases = cases.filter(
(c) => JSON.stringify(c.expectedOn.slice().sort()) === JSON.stringify(c.expectedOff.slice().sort()),
);
a18a080 to
723e40a
Compare
723e40a to
8d7972e
Compare
This comment has been minimized.
This comment has been minimized.
PR Reviewer Guide 🔍
|
1 participant
Pull Request Submission Checklist
are release-note ready, emphasizing
what was changed, not how.
What does this PR do?
Updates GAF to v0.14.0 so
.gitignorerules no longer exclude files already tracked by Git. The behavior is feature-flagged throughclientFileFilterGitignore_TrackedFilesRollout. Explicit.snykand.dcignoreexclusions continue to apply regardless of Git tracking.Where should the reviewer start?
How should this be manually tested?
What's the product update that needs to be communicated to CLI users?
N/A