From 835b7f0f9e6b6cdeba890492d5ad806078d2fe5b Mon Sep 17 00:00:00 2001 From: Hesham Salman Date: Mon, 27 Apr 2026 16:35:11 -0400 Subject: [PATCH] fix: honor negation patterns under default predicate quantifier A filter rule that mixed positive and bare negation patterns under the default ('some') predicate quantifier matched nearly every file in a PR. Each pattern was compiled into its own picomatch matcher and combined via Array.prototype.some, so a standalone '!**/*.md' (true for any non-markdown file) flipped the whole rule into a near-universal match. Group bare string patterns into a single matcher with gitignore-style semantics: a file matches when it matches at least one positive pattern and does not match any negation pattern. The 'every' quantifier path is unchanged, since per-pattern matching under .every() already produces correct subtractive semantics with negations. The '!(extglob)' single- string form is preserved by detecting only '!' not followed by '('. Apply the same gitignore-style grouping to status-tagged array patterns so 'added: ["src/**", "!**/*.md"]' behaves correctly. Reject rules made up entirely of negation patterns (no positive include) so the failure is loud rather than a silent permanent no-match. Closes dorny/paths-filter#260 --- __tests__/filter.test.ts | 104 ++++++++++++++++++++++++++++++++++ dist/index.js | 111 ++++++++++++++++++++++++++++++++++--- src/filter.ts | 117 ++++++++++++++++++++++++++++++++++++--- 3 files changed, 316 insertions(+), 16 deletions(-) diff --git a/__tests__/filter.test.ts b/__tests__/filter.test.ts index 7d7da94..10f15b7 100644 --- a/__tests__/filter.test.ts +++ b/__tests__/filter.test.ts @@ -17,6 +17,24 @@ describe('yaml filter parsing tests', () => { const t = () => new Filter(yaml) expect(t).toThrow(/^Invalid filter.*/) }) + + test('throws when a rule contains only negation patterns', () => { + const yaml = ` + excludes: + - '!**/*.md' + - '!**/*.txt' + ` + expect(() => new Filter(yaml)).toThrow(/at least one positive pattern/) + }) + + test('throws when a status-tagged rule contains only negation patterns', () => { + const yaml = ` + docs: + - modified: + - '!**/*.md' + ` + expect(() => new Filter(yaml)).toThrow(/at least one positive pattern/) + }) }) describe('matching tests', () => { @@ -148,6 +166,92 @@ describe('matching tests', () => { expect(otherPkgJpegMatch.backend).toEqual([]) }) + test('negation patterns under default quantifier exclude files instead of matching everything (issue #260)', () => { + const yaml = ` + mobile: + - 'mobile/**' + - '!mobile/**/*.md' + - '!mobile/.config/**' + - '.github/workflows/test_mobile.yml' + ` + const filter = new Filter(yaml) + + // Files outside the included path must NOT match purely because they are + // not mobile markdown files. This was the original bug: a standalone + // '!mobile/**/*.md' picomatch returned true for any non-markdown path, + // and the default 'some' quantifier flipped the rule into a near-universal match. + const unrelated = modified(['web/src/foo.tsx', 'docs/README.md', 'server/main.go']) + expect(filter.match(unrelated).mobile).toEqual([]) + + // Mobile sources should still match. + const mobileSrc = modified(['mobile/src/app.ts', 'mobile/lib/index.ts']) + expect(filter.match(mobileSrc).mobile).toEqual(mobileSrc) + + // Negated paths inside the include set must be excluded. + const mobileExcluded = modified(['mobile/README.md', 'mobile/.config/eslint.json']) + expect(filter.match(mobileExcluded).mobile).toEqual([]) + + // The standalone workflow path must still match. + const workflow = modified(['.github/workflows/test_mobile.yml']) + expect(filter.match(workflow).mobile).toEqual(workflow) + }) + + test('negation across YAML anchors is honored under default quantifier', () => { + const yaml = ` + shared: &shared + - 'common/**' + - '!**/*.md' + - '!**/*.txt' + src: + - 'src/**' + - *shared + ` + const filter = new Filter(yaml) + + // Anchor-inherited positives still match. + expect(filter.match(modified(['common/util.ts'])).src).toEqual(modified(['common/util.ts'])) + // The rule's own positive still matches. + expect(filter.match(modified(['src/app.ts'])).src).toEqual(modified(['src/app.ts'])) + // Anchor-inherited negations exclude files even when a sibling positive matches. + expect(filter.match(modified(['src/README.md'])).src).toEqual([]) + expect(filter.match(modified(['common/notes.txt'])).src).toEqual([]) + // Files outside every positive pattern do not match. + expect(filter.match(modified(['other/file.ts'])).src).toEqual([]) + }) + + test('status-tagged array honors negation patterns (issue #260, status form)', () => { + const yaml = ` + src: + - modified: + - 'src/**' + - '!src/**/*.md' + ` + const filter = new Filter(yaml) + const tsFile = modified(['src/app.ts']) + const mdFile = modified(['src/README.md']) + const unrelated = modified(['docs/intro.md']) + expect(filter.match(tsFile).src).toEqual(tsFile) + expect(filter.match(mdFile).src).toEqual([]) + expect(filter.match(unrelated).src).toEqual([]) + }) + + test('mixing string patterns and status-tagged patterns still matches both forms', () => { + const yaml = ` + backend: + - 'src/**' + - '!src/**/*.md' + - added: 'migrations/**' + ` + const filter = new Filter(yaml) + + expect(filter.match(modified(['src/server.ts'])).backend).toEqual(modified(['src/server.ts'])) + expect(filter.match(modified(['src/README.md'])).backend).toEqual([]) + const addedMigration: File[] = [{status: ChangeStatus.Added, filename: 'migrations/0001.sql'}] + expect(filter.match(addedMigration).backend).toEqual(addedMigration) + const modifiedMigration = modified(['migrations/0001.sql']) + expect(filter.match(modifiedMigration).backend).toEqual([]) + }) + test('matches path based on rules included using YAML anchor', () => { const yaml = ` shared: &shared diff --git a/dist/index.js b/dist/index.js index 3365cd3..0c5b50c 100644 --- a/dist/index.js +++ b/dist/index.js @@ -138,7 +138,33 @@ class Filter { } } parseFilterItemYaml(item) { + var _a; if (Array.isArray(item)) { + // Under the default 'some' quantifier, group all (recursively flattened) + // bare string patterns into a single matcher with gitignore-style + // semantics: a file matches the rule when it matches at least one + // positive pattern AND does not match any negation pattern. + // + // Without this grouping, each '!pattern' is compiled into its own + // picomatch matcher that returns true for every file *not* matching the + // pattern. The default 'some' quantifier then OR's those predicates + // together, so a standalone '!**/*.md' makes the whole rule match + // nearly any path. The 'every' quantifier already produces correct + // subtractive semantics under per-pattern matching, so it keeps the + // legacy parsing path unchanged. + if (((_a = this.filterConfig) === null || _a === void 0 ? void 0 : _a.predicateQuantifier) !== PredicateQuantifier.EVERY) { + const { stringPatterns, otherItems } = this.collectArrayItems(item); + const grouped = this.groupedStringMatcher(stringPatterns); + if (grouped === undefined && otherItems.length === 0) { + this.throwInvalidFormatError('Filter rule must contain at least one positive pattern; got only negation patterns or an empty pattern list'); + } + const result = []; + if (grouped !== undefined) { + result.push({ status: undefined, isMatch: grouped }); + } + result.push(...otherItems); + return result; + } return flat(item.map(i => this.parseFilterItemYaml(i))); } if (typeof item === 'string') { @@ -149,18 +175,87 @@ class Filter { if (typeof key !== 'string' || (typeof pattern !== 'string' && !Array.isArray(pattern))) { this.throwInvalidFormatError(`Expected [key:string]= pattern:string | string[], but [${key}:${typeof key}]= ${pattern}:${typeof pattern} found`); } - return { - status: key - .split('|') - .map(x => x.trim()) - .filter(x => x.length > 0) - .map(x => x.toLowerCase()), - isMatch: (0, picomatch_1.default)(pattern, MatchOptions) - }; + const status = key + .split('|') + .map(x => x.trim()) + .filter(x => x.length > 0) + .map(x => x.toLowerCase()); + return { status, isMatch: this.compileStatusPattern(pattern, key) }; }); } this.throwInvalidFormatError(`Unexpected element type '${typeof item}'`); } + // Recursively walk a YAML array (which may contain nested arrays from YAML + // anchors) and partition its leaves into raw string patterns vs. fully + // parsed FilterRuleItems for status-tagged objects. + collectArrayItems(item) { + if (Array.isArray(item)) { + const stringPatterns = []; + const otherItems = []; + for (const i of item) { + const sub = this.collectArrayItems(i); + stringPatterns.push(...sub.stringPatterns); + otherItems.push(...sub.otherItems); + } + return { stringPatterns, otherItems }; + } + if (typeof item === 'string') { + return { stringPatterns: [item], otherItems: [] }; + } + return { stringPatterns: [], otherItems: this.parseFilterItemYaml(item) }; + } + // Compiles the right-hand side of a status-tagged YAML entry (e.g. + // `added: 'src/**'` or `added: ['src/**', '!src/**/*.md']`) into a single + // matcher. String-array forms are routed through groupedStringMatcher so + // they get the same gitignore-style negation semantics as bare-array rules + // - otherwise the same #260 bug shape would still bite under a status tag. + // A single string pattern keeps the legacy single-picomatch compilation, + // which preserves existing behavior for `!(extglob)` and plain literals. + compileStatusPattern(pattern, key) { + if (typeof pattern === 'string') { + return (0, picomatch_1.default)(pattern, MatchOptions); + } + const matcher = this.groupedStringMatcher(pattern); + if (matcher === undefined) { + this.throwInvalidFormatError(`Status-tagged filter '${key}' must contain at least one positive pattern; got only negation patterns or an empty list`); + } + return matcher; + } + // Builds a single matcher with gitignore-style semantics over a list of + // string patterns: a file matches when at least one positive pattern matches + // and no negation pattern matches. + // + // A pattern is treated as a gitignore-style negation only when it begins + // with '!' followed by anything other than '('. The '!(...)' form is an + // extglob expression that picomatch parses as a single pattern, so it must + // not be split into a positive/negative pair. + // + // Returns undefined when the list contains no positive patterns - in that + // case there is nothing to include, so the rule cannot match any file. + groupedStringMatcher(patterns) { + const positives = []; + const negatives = []; + for (const p of patterns) { + if (this.isNegationPrefix(p)) { + negatives.push(p.slice(1)); + } + else { + positives.push(p); + } + } + if (positives.length === 0) { + return undefined; + } + const positiveMatcher = (0, picomatch_1.default)(positives, MatchOptions); + if (negatives.length === 0) { + return positiveMatcher; + } + const negativeMatcher = (0, picomatch_1.default)(negatives, MatchOptions); + return (str) => positiveMatcher(str) && !negativeMatcher(str); + } + isNegationPrefix(pattern) { + return pattern.length > 1 && pattern.startsWith('!') && !pattern.startsWith('!('); + } throwInvalidFormatError(message) { throw new Error(`Invalid filter YAML format: ${message}.`); } diff --git a/src/filter.ts b/src/filter.ts index 1947ef8..1597f2a 100644 --- a/src/filter.ts +++ b/src/filter.ts @@ -116,6 +116,33 @@ export class Filter { private parseFilterItemYaml(item: FilterItemYaml): FilterRuleItem[] { if (Array.isArray(item)) { + // Under the default 'some' quantifier, group all (recursively flattened) + // bare string patterns into a single matcher with gitignore-style + // semantics: a file matches the rule when it matches at least one + // positive pattern AND does not match any negation pattern. + // + // Without this grouping, each '!pattern' is compiled into its own + // picomatch matcher that returns true for every file *not* matching the + // pattern. The default 'some' quantifier then OR's those predicates + // together, so a standalone '!**/*.md' makes the whole rule match + // nearly any path. The 'every' quantifier already produces correct + // subtractive semantics under per-pattern matching, so it keeps the + // legacy parsing path unchanged. + if (this.filterConfig?.predicateQuantifier !== PredicateQuantifier.EVERY) { + const {stringPatterns, otherItems} = this.collectArrayItems(item) + const grouped = this.groupedStringMatcher(stringPatterns) + if (grouped === undefined && otherItems.length === 0) { + this.throwInvalidFormatError( + 'Filter rule must contain at least one positive pattern; got only negation patterns or an empty pattern list' + ) + } + const result: FilterRuleItem[] = [] + if (grouped !== undefined) { + result.push({status: undefined, isMatch: grouped}) + } + result.push(...otherItems) + return result + } return flat(item.map(i => this.parseFilterItemYaml(i))) } @@ -130,20 +157,94 @@ export class Filter { `Expected [key:string]= pattern:string | string[], but [${key}:${typeof key}]= ${pattern}:${typeof pattern} found` ) } - return { - status: key - .split('|') - .map(x => x.trim()) - .filter(x => x.length > 0) - .map(x => x.toLowerCase()) as ChangeStatus[], - isMatch: picomatch(pattern, MatchOptions) - } + const status = key + .split('|') + .map(x => x.trim()) + .filter(x => x.length > 0) + .map(x => x.toLowerCase()) as ChangeStatus[] + return {status, isMatch: this.compileStatusPattern(pattern, key)} }) } this.throwInvalidFormatError(`Unexpected element type '${typeof item}'`) } + // Recursively walk a YAML array (which may contain nested arrays from YAML + // anchors) and partition its leaves into raw string patterns vs. fully + // parsed FilterRuleItems for status-tagged objects. + private collectArrayItems(item: FilterItemYaml): {stringPatterns: string[]; otherItems: FilterRuleItem[]} { + if (Array.isArray(item)) { + const stringPatterns: string[] = [] + const otherItems: FilterRuleItem[] = [] + for (const i of item) { + const sub = this.collectArrayItems(i) + stringPatterns.push(...sub.stringPatterns) + otherItems.push(...sub.otherItems) + } + return {stringPatterns, otherItems} + } + if (typeof item === 'string') { + return {stringPatterns: [item], otherItems: []} + } + return {stringPatterns: [], otherItems: this.parseFilterItemYaml(item)} + } + + // Compiles the right-hand side of a status-tagged YAML entry (e.g. + // `added: 'src/**'` or `added: ['src/**', '!src/**/*.md']`) into a single + // matcher. String-array forms are routed through groupedStringMatcher so + // they get the same gitignore-style negation semantics as bare-array rules + // - otherwise the same #260 bug shape would still bite under a status tag. + // A single string pattern keeps the legacy single-picomatch compilation, + // which preserves existing behavior for `!(extglob)` and plain literals. + private compileStatusPattern(pattern: string | string[], key: string): (str: string) => boolean { + if (typeof pattern === 'string') { + return picomatch(pattern, MatchOptions) + } + const matcher = this.groupedStringMatcher(pattern) + if (matcher === undefined) { + this.throwInvalidFormatError( + `Status-tagged filter '${key}' must contain at least one positive pattern; got only negation patterns or an empty list` + ) + } + return matcher + } + + // Builds a single matcher with gitignore-style semantics over a list of + // string patterns: a file matches when at least one positive pattern matches + // and no negation pattern matches. + // + // A pattern is treated as a gitignore-style negation only when it begins + // with '!' followed by anything other than '('. The '!(...)' form is an + // extglob expression that picomatch parses as a single pattern, so it must + // not be split into a positive/negative pair. + // + // Returns undefined when the list contains no positive patterns - in that + // case there is nothing to include, so the rule cannot match any file. + private groupedStringMatcher(patterns: string[]): ((str: string) => boolean) | undefined { + const positives: string[] = [] + const negatives: string[] = [] + for (const p of patterns) { + if (this.isNegationPrefix(p)) { + negatives.push(p.slice(1)) + } else { + positives.push(p) + } + } + if (positives.length === 0) { + return undefined + } + const positiveMatcher = picomatch(positives, MatchOptions) + if (negatives.length === 0) { + return positiveMatcher + } + const negativeMatcher = picomatch(negatives, MatchOptions) + return (str: string) => positiveMatcher(str) && !negativeMatcher(str) + } + + private isNegationPrefix(pattern: string): boolean { + return pattern.length > 1 && pattern.startsWith('!') && !pattern.startsWith('!(') + } + private throwInvalidFormatError(message: string): never { throw new Error(`Invalid filter YAML format: ${message}.`) }