feat: add .skillignore support for filtering internal skills#375
feat: add .skillignore support for filtering internal skills#375anthony-maio wants to merge 6 commits intovercel-labs:mainfrom
Conversation
|
@anthony-maio is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a77a0b9bfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR adds .skillignore support to allow skill repository maintainers to filter out internal or experimental skills from public discovery. The feature reads a .skillignore file from the repository root containing patterns (exact names or trailing wildcards) that determine which skills should be hidden.
Changes:
- Added new
src/skillignore.tsmodule with parsing and pattern matching logic - Integrated
.skillignorefiltering into thediscoverSkills()function insrc/skills.ts - Added comprehensive unit tests in
tests/skillignore.test.tscovering parsing, pattern matching, and file loading
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| src/skillignore.ts | New module implementing .skillignore file parsing, pattern matching (exact and trailing wildcard), and file loading with graceful error handling |
| src/skills.ts | Integrated .skillignore filtering at the end of discoverSkills(), loading patterns from basePath and filtering discovered skills |
| tests/skillignore.test.ts | Unit tests for parseSkillIgnore (17 test cases), isSkillIgnored, and loadSkillIgnorePatterns functions with various edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function isSkillIgnored(skillName: string, patterns: string[]): boolean { | ||
| return patterns.some((pattern) => { | ||
| if (pattern.endsWith('*')) { | ||
| return skillName.startsWith(pattern.slice(0, -1)); | ||
| } | ||
| return skillName === pattern; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The wildcard pattern matching is safe but limited - it only supports trailing wildcards (e.g., "test-*"). However, the current implementation doesn't validate or sanitize patterns before using them. While the simple startsWith/equals check is safe, consider documenting the supported pattern syntax clearly or adding validation to reject unsupported patterns (like leading wildcards, middle wildcards, or regex patterns) to avoid user confusion.
| describe('loadSkillIgnorePatterns', () => { | ||
| let testDir: string; | ||
|
|
||
| beforeEach(() => { | ||
| testDir = join(tmpdir(), `skillignore-test-${Date.now()}`); | ||
| mkdirSync(testDir, { recursive: true }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| rmSync(testDir, { recursive: true, force: true }); | ||
| }); | ||
|
|
||
| it('reads and parses .skillignore file', async () => { | ||
| writeFileSync(join(testDir, '.skillignore'), '# Internal tools\ninternal-*\ntest-runner\n'); | ||
| const patterns = await loadSkillIgnorePatterns(testDir); | ||
| expect(patterns).toEqual(['internal-*', 'test-runner']); | ||
| }); | ||
|
|
||
| it('returns empty array when file does not exist', async () => { | ||
| const patterns = await loadSkillIgnorePatterns(testDir); | ||
| expect(patterns).toEqual([]); | ||
| }); | ||
|
|
||
| it('returns empty array for empty file', async () => { | ||
| writeFileSync(join(testDir, '.skillignore'), ''); | ||
| const patterns = await loadSkillIgnorePatterns(testDir); | ||
| expect(patterns).toEqual([]); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test suite only includes unit tests for the skillignore parsing and matching functions, but lacks integration tests that verify .skillignore works correctly with the discoverSkills function. Consider adding integration tests that create a test directory structure with skills and a .skillignore file, then verify that discoverSkills correctly filters out the ignored skills. This would help catch issues like the early return path bypassing the filtering logic.
src/skillignore.ts
Outdated
| * internal-tool # exact match | ||
| * test-* # trailing wildcard |
There was a problem hiding this comment.
The documentation comment at line 9 shows an inline comment after a pattern ("internal-tool # exact match"), but the parseSkillIgnore function does not strip inline comments. This creates a discrepancy between the documented format and the actual implementation. Either update the documentation to show that inline comments are not supported, or update the implementation to handle them.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot open a new pull request to apply changes based on the comments in this thread |
Co-authored-by: vercel[bot] <35613825+vercel[bot]@users.noreply.github.com>
- Fix early return bypass: .skillignore now filters even when discoverSkills() takes the direct SKILL.md early return path - Load ignore patterns once and reuse across both code paths - Clarify doc comment to document inline comment support - Add 4 integration tests verifying discoverSkills() respects .skillignore (pattern filtering, early return, includeInternal bypass)
Added .skillignore support plus test coverage (17 tests)
How it works:
Example .skillignore: