feat(filter-diff): support glob patterns in exclude-paths#224
feat(filter-diff): support glob patterns in exclude-paths#224jedp-docker wants to merge 1 commit into
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding in the newly added glob-matching heuristic.
d541578 to
93c6222
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
| function toGlob(pattern: string): string { | ||
| if (/[*?[]/.test(pattern)) return pattern; | ||
| if (pattern.endsWith('/')) return `${pattern}**`; | ||
| return pattern; |
There was a problem hiding this comment.
[MEDIUM] Bare directory entry without trailing slash silently stops matching subdirectory paths
Under the old startsWith implementation, a user who configured vendor (no trailing slash) as an exclude-paths entry would have all paths beginning with the string "vendor" excluded — including vendor/foo.go, vendor/a/b/c.go, etc. Under the new matchesGlob-based implementation, toGlob('vendor') returns 'vendor' unchanged (no metacharacters, no trailing slash), so matchesGlob('vendor/foo.go', 'vendor') returns false. Only a path literally equal to "vendor" would now match.
This is a silent behavioral regression: no error is raised, files simply stop being excluded. The correct pattern for directory exclusion is vendor/ (with trailing slash), which the PR correctly expands to vendor/**, but users who omit the trailing slash will experience a silent change in behavior.
The PR description notes that no known docker org consumer uses this pattern, but external consumers of cagent-action could be affected without any warning.
Suggested mitigation: Emit a warning to stderr (similar to the existing process.stderr.write calls in applyFilter) when a pattern contains no metacharacters, no trailing slash, and no path separator — indicating it may be intended as a directory prefix and suggesting vendor/ instead. Alternatively, document this breaking change prominently in the action's changelog.
There was a problem hiding this comment.
Thanks for adding the warning! The check on line 169 looks good — it correctly identifies patterns that:
- Contain no glob metacharacters
- Have a last segment with no file extension
This should catch the common case of bare directory names like vendor while avoiding false positives on files like package-lock.json. The message is clear and actionable.
One edge case: patterns like docs/internal (subdirectory without trailing slash) won't trigger the warning because they contain /. That's probably fine — the warning focuses on the most likely mistake (top-level directory names), and the message itself explains the general rule.
93c6222 to
70b6ae8
Compare
Plain prefix matching required listing every package-lock.json path explicitly. With glob support, a single `**/package-lock.json` entry covers all lock files at any depth. Uses `path.matchesGlob` (added in Node v22.5.0/v20.17.0, available in the node24 runtime — no new dependencies). Patterns containing `*` are matched as globs; plain strings continue to use `startsWith` for backward compatibility. Adds a `declare module` augmentation for `matchesGlob` since the @types/node version pinned in pnpm-lock.yaml predates its type definition. The lockfile cannot be updated until a pre-existing exotic subdep in @actions/artifact@6.2.1 is resolved upstream. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
70b6ae8 to
a729d75
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The glob-pattern implementation is correct and the backward-compatibility analysis is sound. Two low-severity observations about the warning heuristic and the prefix-to-exact-match semantic shift are noted as inline comments.
| const lastSegment = p.split('/').at(-1) ?? p; | ||
| if (!/[*?[]/.test(p) && !lastSegment.includes('.')) { | ||
| process.stderr.write( | ||
| `⚠️ exclude-paths entry "${p}" looks like a bare directory name — did you mean "${p}/"? Without a trailing slash only an exact path match is performed.\n`, |
There was a problem hiding this comment.
[LOW] Warning heuristic false-positives on dotless exact filenames (Makefile, LICENSE, Dockerfile, etc.)
The heuristic in applyFilter (lines 172–177) triggers a warning for any pattern that has no glob metacharacters and whose last path segment contains no dot. This correctly catches accidental bare directory names like src or vendor, but it also fires for perfectly valid exact-file exclusions such as Makefile, LICENSE, Dockerfile, Gemfile, Rakefile, and CODEOWNERS.
When a user writes exclude-paths: Makefile, parseExcludePrefixes returns 'Makefile' (no dot, no glob chars), and the warning emits:
⚠️ exclude-paths entry "Makefile" looks like a bare directory name — did you mean "Makefile/"? Without a trailing slash only an exact path match is performed.
Appending / to a filename is nonsensical and the resulting pattern Makefile/ would never match anything. This doesn't affect filtering correctness, but it produces misleading Actions log output.
A simple fix would be to also exclude single-segment patterns from the warning (if the pattern contains no /, it cannot be a bare directory):
if (!/[*?[]/.test(p) && !lastSegment.includes('.') && p.includes('/')) {|
|
||
| const isExcluded = (filePath: string): boolean => | ||
| prefixes.some((prefix) => filePath.startsWith(prefix)); | ||
| prefixes.some((pattern) => matchesGlob(filePath, pattern)); |
There was a problem hiding this comment.
[LOW] Silent behavior change from prefix-match to exact/glob match may break existing consumers
The old filePath.startsWith(prefix) performed a string-prefix match. The new matchesGlob(filePath, pattern) performs exact or glob matching. For any exclude-paths entry that contains no glob metacharacters and no trailing slash (treated as an exact-file pattern by toGlob), the semantics change:
| Pattern | Old (startsWith) |
New (matchesGlob) |
|---|---|---|
src/foo.ts |
Matches src/foo.ts, src/foo.ts.bak, src/foo.tsx |
Matches only src/foo.ts |
backend/gen |
Matches backend/gen/anything |
Matches only backend/gen exactly |
The PR description notes a search of Docker org consumers shows no one relies on the prefix behavior, and a runtime warning now nudges users to add trailing slashes for directory entries. However, external consumers or any user with a pattern like backend/gen (without trailing slash, no file extension) will silently stop having their files excluded after upgrading — they will see no error, just unexpected files appearing in reviews.
No code change is required if the scope is intentionally Docker-org-only, but a CHANGELOG / migration note and a more prominent warning (perhaps also logged during filterDiff for "exact" patterns that look like directories) would help external users notice the change.
Summary
exclude-pathsnow accepts glob patterns in addition to plain path prefixes**/package-lock.jsonentry covers lock files at any depth, instead of listing every path explicitlyvendor/) are automatically expanded tovendor/**; all other plain entries continue to work as exact path matches — fully backward compatibleImplementation
Uses
path.matchesGlob(added in Node v22.5.0/v20.17.0, available in thenode24runtime — no new dependencies). All patterns now route throughmatchesGlobafter normalization inparseExcludePrefixes:*,?,[)**/package-lock.json/vendor/vendor/**package-lock.jsonThis unifies all matching through a single code path, removing the previous
startsWithbranch while preserving all existing behavior.Backward compatibility
The trailing-slash expansion (
vendor/→vendor/**) preserves existing directory-prefix semantics underpath.matchesGlob. A search across the Docker org confirms the two real consumers are both covered:vendor/→ expands tovendor/**✓package-lock.json,alertz/tools/package-lock.json→ exact match viamatchesGlob✓No repo uses a bare directory path without a trailing slash (e.g.
backend/geninstead ofbackend/gen/), so that edge case is not a concern.Type augmentation
path.matchesGlobis not typed in@types/node@22.0.0(the version pinned inpnpm-lock.yaml). Normally this would be fixed by bumping@types/node, but that requires regenerating the lockfile — which currently fails due to a pre-existing issue:@actions/artifact@6.2.1declares"@protobuf-ts/plugin": "^2.2.3-alpha.1"as a dependency. Under pnpm'sblockExoticSubdeps: true+resolutionMode: time-based(both required by docker-security/safe-defaults), any freshpnpm installfails withERR_PNPM_EXOTIC_SUBDEP. The frozen lockfile used in CI works fine because--frozen-lockfileskips resolution entirely and the check never fires.The fix (
@protobuf-ts/plugindropped from deps) is already merged in theactions/toolkitsource as@actions/artifact@6.2.2, but that version has not been published to npm yet. Once it is, a normalpnpm installwill work and we can bump@types/nodecleanly.In the meantime,
matchesGlobis declared via a typed cast infilter-diff.tswith a comment explaining the blocker.Motivation
Without glob support, callers had to enumerate every lock file path explicitly. In docker/internal-agents there are 7
package-lock.jsonfiles — with this change that list collapses to one line:Test plan
**/nameglob matching at root and nested depth,?and[...]metacharacters, directory expansion (vendor/→vendor/**), exact file path matching, and coexistence of multiple pattern types🤖 Generated with Claude Code