Visitar URL original
fix discovery support in nested dirs by tommaso-moro · Pull Request #13459 · cli/cli · GitHub
Skip to content

fix discovery support in nested dirs #13459

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
babakks merged 5 commits into from
Jun 2, 2026
Merged

fix discovery support in nested dirs #13459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
00fc8c9
fix discovery support in nested dirs
tommaso-moro May 19, 2026
fb748cb
add logic to preview too
tommaso-moro May 19, 2026
ad99832
Merge branch 'trunk' into tommy/nested-dirs-bug-fix
tommaso-moro May 22, 2026
994024f
address copilot comments
tommaso-moro May 22, 2026
678660b
address review comments
tommaso-moro Jun 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 55 additions & 22 deletions internal/skills/discovery/discovery.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,28 @@ func MatchSkillPath(filePath string) (name, namespace string) {
return m.name, m.namespace
}

// IsSkillPath reports whether a skill selector looks like a repo-relative path
// rather than a simple skill name.
func IsSkillPath(name string) bool {
name = strings.TrimSuffix(name, "/")
if name == "" {
return false
}
if strings.HasSuffix(name, "/SKILL.md") {
return true
}
if strings.HasPrefix(name, "skills/") || strings.HasPrefix(name, "plugins/") {
return true
}
if strings.Contains(name, "/skills/") || strings.Contains(name, "/plugins/") {
return true
}
if strings.Count(name, "/") >= 2 {
return true
}
return false
}

// matchSkillConventions checks if a blob path matches any known skill convention.
func matchSkillConventions(entry treeEntry) *skillMatch {
if path.Base(entry.Path) != "SKILL.md" {
Expand Down Expand Up @@ -451,48 +473,46 @@ func matchSkillConventions(entry treeEntry) *skillMatch {
}

// matchHiddenDirConventions checks if a blob path matches a skill convention
// under a hidden (dot-prefixed) root directory. These patterns mirror the
// standard skills/ conventions but rooted under .{host}/skills/:
// under a path that contains a hidden (dot-prefixed) directory. These patterns
// mirror the standard skills/ conventions, but only when a hidden segment
// appears anywhere in the ancestor path:
//
// - .{host}/skills/*/SKILL.md -> "hidden-dir"
// - .{host}/skills/{scope}/*/SKILL.md -> "hidden-dir-namespaced"
// - {prefix}/.{host}/{suffix}/skills/*/SKILL.md -> "hidden-dir"
// - {prefix}/.{host}/{suffix}/skills/{scope}/*/SKILL.md -> "hidden-dir-namespaced"
func matchHiddenDirConventions(entry treeEntry) *skillMatch {
if path.Base(entry.Path) != "SKILL.md" {
return nil
}
if !hasHiddenSegment(entry.Path) {
return nil
}

// .{host}/skills/*
// .{host}/skills/{scope}/*
// {prefix}/.{host}/{suffix}/skills/*
// {prefix}/.{host}/{suffix}/skills/{scope}/*
dir := path.Dir(entry.Path)
skillName := path.Base(dir)

if !validateName(skillName) {
return nil
}

// .{host}/skills
// .{host}/skills/{scope}
// {prefix}/.{host}/{suffix}/skills
// {prefix}/.{host}/{suffix}/skills/{scope}
parentDir := path.Dir(dir)

// .{host}/skills/*/SKILL.md
// {prefix}/.{host}/{suffix}/skills/*/SKILL.md
if path.Base(parentDir) == "skills" {
hiddenRoot := path.Dir(parentDir)
if path.Dir(hiddenRoot) == "." && strings.HasPrefix(hiddenRoot, ".") {
return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "hidden-dir"}
}
return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "hidden-dir"}
}

// .{host}/skills/{scope}/*/SKILL.md
// {prefix}/.{host}/{suffix}/skills/{scope}/*/SKILL.md
grandparentDir := path.Dir(parentDir)
if path.Base(grandparentDir) == "skills" {
hiddenRoot := path.Dir(grandparentDir)
if path.Dir(hiddenRoot) == "." && strings.HasPrefix(hiddenRoot, ".") {
namespace := path.Base(parentDir)
if !validateName(namespace) {
return nil
}
return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "hidden-dir-namespaced"}
namespace := path.Base(parentDir)
if !validateName(namespace) {
return nil
}
return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "hidden-dir-namespaced"}
}

return nil
Expand Down Expand Up @@ -655,8 +675,19 @@ func FetchDescriptionsConcurrent(client *api.Client, host, owner, repo string, s
wg.Wait()
}

// DiscoverSkillByPathOptions controls optional behavior for DiscoverSkillByPathWithOptions.
type DiscoverSkillByPathOptions struct {
SkipDescription bool
}

// DiscoverSkillByPath looks up a single skill by its exact path in the repository.
func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skillPath string) (*Skill, error) {
return DiscoverSkillByPathWithOptions(client, host, owner, repo, commitSHA, skillPath, DiscoverSkillByPathOptions{})
}

// DiscoverSkillByPathWithOptions looks up a single skill by its exact path in
// the repository, applying the given options.
func DiscoverSkillByPathWithOptions(client *api.Client, host, owner, repo, commitSHA, skillPath string, opts DiscoverSkillByPathOptions) (*Skill, error) {
skillPath = strings.TrimSuffix(skillPath, "/SKILL.md")
skillPath = strings.TrimSuffix(skillPath, "/")

Expand Down Expand Up @@ -737,7 +768,9 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill
TreeSHA: treeSHA,
}

skill.Description = fetchDescription(client, host, owner, repo, skill)
if !opts.SkipDescription {
skill.Description = fetchDescription(client, host, owner, repo, skill)
}

return skill, nil
}
Expand Down
110 changes: 107 additions & 3 deletions internal/skills/discovery/discovery_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,19 @@ func TestMatchHiddenDirConventions(t *testing.T) {
wantNamespace: "monalisa",
wantConvention: "hidden-dir-namespaced",
},
{
name: "nested hidden dir skills directory",
path: "foo/bar/.claude/skills/code-review/SKILL.md",
wantName: "code-review",
wantConvention: "hidden-dir",
},
{
name: "nested hidden dir namespaced skill",
path: "foo/bar/.claude/skills/monalisa/code-review/SKILL.md",
wantName: "code-review",
wantNamespace: "monalisa",
wantConvention: "hidden-dir-namespaced",
},
{
name: "not a SKILL.md file",
path: ".claude/skills/code-review/README.md",
Expand All @@ -228,9 +241,17 @@ func TestMatchHiddenDirConventions(t *testing.T) {
wantNil: true,
},
{
name: "too deeply nested hidden dir",
path: ".claude/nested/skills/code-review/SKILL.md",
wantNil: true,
name: "hidden dir with nested skills directory",
path: ".claude/nested/skills/code-review/SKILL.md",
wantName: "code-review",
wantConvention: "hidden-dir",
},
Comment thread
babakks marked this conversation as resolved.
Resolved
{
name: "hidden dir with nested namespaced skills directory",
path: ".claude/nested/skills/monalisa/code-review/SKILL.md",
wantName: "code-review",
wantNamespace: "monalisa",
wantConvention: "hidden-dir-namespaced",
},
{
name: "invalid skill name",
Expand Down Expand Up @@ -992,6 +1013,16 @@ func TestDiscoverSkillsWithOptions(t *testing.T) {
},
}

nestedHiddenTree := map[string]interface{}{
"sha": "abc123", "truncated": false,
"tree": []map[string]interface{}{
{"path": "foo/bar/.claude/skills/hidden-skill", "type": "tree", "sha": "tree-sha-1"},
{"path": "foo/bar/.claude/skills/hidden-skill/SKILL.md", "type": "blob", "sha": "blob-1"},
{"path": "foo/bar/.claude/nested/skills/deep-hidden-skill", "type": "tree", "sha": "tree-sha-2"},
{"path": "foo/bar/.claude/nested/skills/deep-hidden-skill/SKILL.md", "type": "blob", "sha": "blob-2"},
},
Comment thread
babakks marked this conversation as resolved.
Resolved
}

emptyTree := map[string]interface{}{
"sha": "abc123", "truncated": false,
"tree": []map[string]interface{}{
Expand All @@ -1015,6 +1046,11 @@ func TestDiscoverSkillsWithOptions(t *testing.T) {
tree: mixedTree,
wantSkills: []string{"hidden-skill", "standard-skill"},
},
{
name: "nested hidden-dir tree returns hidden skill",
tree: nestedHiddenTree,
wantSkills: []string{"deep-hidden-skill", "hidden-skill"},
},
{
name: "no skills at all",
tree: emptyTree,
Expand Down Expand Up @@ -1298,6 +1334,32 @@ func TestDiscoverSkillByPath(t *testing.T) {
}
}

func TestDiscoverSkillByPathWithOptionsSkipsDescription(t *testing.T) {
reg := &httpmock.Registry{}
defer reg.Verify(t)

reg.Register(
httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/skills"),
httpmock.JSONResponse([]map[string]interface{}{
{"name": "code-review", "path": "skills/code-review", "sha": "tree-sha", "type": "dir"},
}))
reg.Register(
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree-sha"),
httpmock.JSONResponse(map[string]interface{}{
"sha": "tree-sha", "truncated": false,
"tree": []map[string]interface{}{
{"path": "SKILL.md", "type": "blob", "sha": "blob-sha"},
},
}))

client := api.NewClientFromHTTP(&http.Client{Transport: reg})
skill, err := DiscoverSkillByPathWithOptions(client, "github.com", "monalisa", "octocat-skills", "abc123", "skills/code-review", DiscoverSkillByPathOptions{SkipDescription: true})

require.NoError(t, err)
assert.Equal(t, "code-review", skill.Name)
assert.Empty(t, skill.Description)
}

func TestDiscoverLocalSkills(t *testing.T) {
tests := []struct {
name string
Expand Down Expand Up @@ -1415,6 +1477,20 @@ func TestDiscoverLocalSkillsWithOptions(t *testing.T) {
},
wantSkills: []string{"standard", "hidden"},
},
{
name: "nested hidden dir returns skill",
setup: func(t *testing.T, dir string) {
t.Helper()
skillDir := filepath.Join(dir, "foo", "bar", ".claude", "skills", "hidden")
require.NoError(t, os.MkdirAll(skillDir, 0o755))
require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("# hidden"), 0o644))

deepDir := filepath.Join(dir, "foo", "bar", ".claude", "nested", "skills", "deep-hidden")
require.NoError(t, os.MkdirAll(deepDir, 0o755))
require.NoError(t, os.WriteFile(filepath.Join(deepDir, "SKILL.md"), []byte("# deep-hidden"), 0o644))
},
wantSkills: []string{"deep-hidden", "hidden"},
},
Comment thread
babakks marked this conversation as resolved.
Resolved
{
name: "no skills at all",
setup: func(t *testing.T, _ string) { t.Helper() },
Expand Down Expand Up @@ -1489,6 +1565,34 @@ func TestMatchSkillPath(t *testing.T) {
}
}

func TestIsSkillPath(t *testing.T) {
tests := []struct {
name string
path string
want bool
}{
{name: "empty string", path: "", want: false},
{name: "plain skill name", path: "git-commit", want: false},
{name: "bare SKILL.md", path: "SKILL.md", want: false},
{name: "SKILL.md suffix", path: "skills/code-review/SKILL.md", want: true},
{name: "starts with skills/", path: "skills/code-review", want: true},
{name: "starts with plugins/", path: "plugins/hubot/skills/pr-summary", want: true},
{name: "nested skills/ path", path: "terraform/code-generation/skills/terraform-style-guide", want: true},
{name: "deeply nested skills/ path", path: "a/b/c/skills/my-skill", want: true},
{name: "nested plugins/ path", path: "vendor/plugins/hubot/skills/pr-summary", want: true},
{name: "arbitrary nested skill path", path: "packages/agent-skills/netsuite-ai-connector-instructions", want: true},
{name: "arbitrary nested skill path with trailing slash", path: "skills-catalog/matlab-core/matlab-debugging/", want: true},
{name: "name containing skills substring", path: "myskills", want: false},
{name: "namespaced skill name", path: "monalisa/code-review", want: false},
{name: "namespaced path", path: "skills/monalisa/issue-triage", want: true},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.want, IsSkillPath(tt.path))
})
}
}

func TestDiscoverSkillFiles(t *testing.T) {
tests := []struct {
name string
Expand Down
29 changes: 8 additions & 21 deletions pkg/cmd/skills/install/install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,10 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru
see: https://agentskills.io/specification

The skill argument can be a name, a namespaced name (%[1]sauthor/skill%[1]s),
or an exact path within the repository (%[1]sskills/author/skill%[1]s or
%[1]sskills/author/skill/SKILL.md%[1]s).
or an exact path within the repository (%[1]sskills/author/skill%[1]s,
%[1]spackages/agent-skills/code-review%[1]s, or any %[1]s.../SKILL.md%[1]s path).
Namespaced names with one slash are matched by name. Use a %[1]sSKILL.md%[1]s
suffix to force a one-directory path outside the standard conventions.

Performance tip: when installing from a large repository with many
skills, providing an exact path instead of a skill name avoids a
Expand Down Expand Up @@ -155,6 +157,9 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru
# Install from a large namespaced repo by path (efficient, skips full discovery)
$ gh skill install github/awesome-copilot skills/monalisa/code-review

# Install from a non-standard nested path (efficient, skips full discovery)
$ gh skill install monalisa/skills-repo packages/agent-skills/code-review

Comment thread
babakks marked this conversation as resolved.
Resolved
# Install from a local directory
$ gh skill install ./my-skills-repo --from-local

Expand Down Expand Up @@ -276,7 +281,7 @@ func installRun(opts *InstallOptions) error {

var selectedSkills []discovery.Skill

if isSkillPath(opts.SkillName) {
if discovery.IsSkillPath(opts.SkillName) {
opts.IO.StartProgressIndicatorWithLabel("Looking up skill")
skill, err := discovery.DiscoverSkillByPath(apiClient, hostname, opts.repo.RepoOwner(), opts.repo.RepoName(), resolved.SHA, opts.SkillName)
opts.IO.StopProgressIndicator()
Expand Down Expand Up @@ -543,24 +548,6 @@ func runLocalInstall(opts *InstallOptions) error {
return nil
}

// isSkillPath returns true if the argument looks like a repo-relative path
// rather than a simple skill name.
func isSkillPath(name string) bool {
if name == "" {
return false
}
if name == "SKILL.md" || strings.HasSuffix(name, "/SKILL.md") {
return true
}
if strings.HasPrefix(name, "skills/") || strings.HasPrefix(name, "plugins/") {
return true
}
if strings.Contains(name, "/skills/") || strings.Contains(name, "/plugins/") {
return true
}
return false
}

func resolveRepoArg(skillSource string, canPrompt bool, p prompter.Prompter) (ghrepo.Interface, string, error) {
if skillSource == "" {
if !canPrompt {
Expand Down
Loading
Loading