Make skill discovery less strict: support nested skills/ directories#13235
Conversation
There was a problem hiding this comment.
Pull request overview
Relaxes skill discovery and path-based installation to support repositories that place skills/ directories under nested prefixes (e.g. terraform/code-generation/skills/...), and expands tests to cover the new conventions.
Changes:
- Extend discovery matching to recognize
skills/<name>/SKILL.mdandskills/<ns>/<name>/SKILL.mdat any depth. - Treat args containing
/skills/or/plugins/as path-based installs (skipping full tree discovery). - Add/adjust tests for nested discovery, nested path-based install, and URL-escaped contents lookups.
Show a summary per file
| File | Description |
|---|---|
| pkg/cmd/skills/install/install.go | Updates CLI help text and broadens isSkillPath detection to include nested skills/ and plugins/ segments. |
| pkg/cmd/skills/install/install_test.go | Fixes stubs to match URL-escaped parent paths and adds integration/unit tests for nested path installs and isSkillPath. |
| internal/skills/discovery/discovery.go | Adds nested skills/ convention matching, improves error message text, and updates namespace detection for path-based lookups. |
| internal/skills/discovery/discovery_test.go | Adds coverage for nested skills/ discovery (remote + local) and nested path-based lookup behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
internal/skills/discovery/discovery.go:804
- Similarly, the local discovery "no skills found" error message mentions
{prefix}/skills/*/SKILL.mdbut omits the nested namespaced form{prefix}/skills/{namespace}/*/SKILL.md, even though it’s now supported. Updating the message will make troubleshooting easier.
return nil, fmt.Errorf(
"no skills found in %s\n"+
" Expected SKILL.md in the directory, or skills in skills/*/SKILL.md,\n"+
" skills/{scope}/*/SKILL.md, {prefix}/skills/*/SKILL.md,\n"+
" */SKILL.md, or plugins/*/skills/*/SKILL.md",
dir,
- Files reviewed: 4/4 changed files
- Comments generated: 4
3518d85 to
e94892e
Compare
e94892e to
0647686
Compare
tommaso-moro
left a comment
There was a problem hiding this comment.
some copilot comments, but aside from those lgtm
|
👋 @SamMorrowDrums, I pushed a commit (d9f0305) with some acceptance test changes (one was failing and one new one to cover the new behavior): Details
|
BagToad
left a comment
There was a problem hiding this comment.
LGTM after copilot comments resolved
Relax skill discovery to recognize skills/ directories at any depth in the repository tree, not just at the root. This enables repos like hashicorp/agent-skills that organize skills under prefixes such as terraform/code-generation/skills/*/SKILL.md. Changes: - matchSkillConventions: add checks for <prefix>/skills/<name>/SKILL.md and <prefix>/skills/<ns>/<name>/SKILL.md at any depth - isSkillPath: also match paths containing /skills/ for direct path-based install - DiscoverSkillByPath: fix namespace detection to find the skills segment anywhere in the path - Update error messages and help text to mention nested conventions Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
d9f0305 to
9a368f4
Compare
Problem
Repositories like
hashicorp/agent-skillsorganize skills under nested prefixes:Our current discovery logic only matched
skills/at the repository root, so these were never found bygh skill install.Changes
Relaxes skill discovery to recognize
skills/directories at any depth in the tree:matchSkillConventions: Adds checks for<prefix>/skills/<name>/SKILL.mdand<prefix>/skills/<ns>/<name>/SKILL.mdat any depth, after the existing root-level and plugins checksisSkillPath: Also matches paths containing/skills/or/plugins/(not just starting with), enabling direct path-based install likegh skill install hashicorp/agent-skills terraform/code-generation/skills/terraform-style-guideDiscoverSkillByPath: Fixes namespace detection to find theskillssegment anywhere in the path rather than assumingparts[0] == "skills"skills/directoriesTests added
TestMatchSkillConventions: nested skills dir, deeply nested, nested namespaced, single prefix, root-level priority preservedTestDiscoverSkills: nested skills in tree, mixed root-level and nested skillsTestDiscoverSkillByPath: deeply nested path, deeply nested namespaced pathTestDiscoverLocalSkills: nested local skills directoryTestMatchesSkillPath/TestMatchSkillPath: nested conventionsTest_isSkillPath: new test covering all path patterns (empty, plain name, SKILL.md, root skills/, nested skills/, nested plugins/, substring)TestInstallRun: integration test for nested path-based installstubSkillByPathto properly URL-encode parent paths with slashes