diff --git a/internal/skills/discovery/discovery.go b/internal/skills/discovery/discovery.go index b2c8baaed93..1b0c7f0075a 100644 --- a/internal/skills/discovery/discovery.go +++ b/internal/skills/discovery/discovery.go @@ -405,6 +405,24 @@ func matchSkillConventions(entry treeEntry) *skillMatch { return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "plugins"} } + // Deeply nested skills/ directory: /skills//SKILL.md + // Matches skills/ at any depth, not just at the repository root. + // Exclude paths with dot-prefixed segments (handled by + // matchHiddenDirConventions) and paths under a plugins/ directory + // (handled by the plugins convention above). + if path.Base(parentDir) == "skills" && !hasHiddenSegment(entry.Path) && !hasPluginsAncestor(entry.Path) { + return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "skills"} + } + + // Deeply nested namespaced: /skills///SKILL.md + if path.Base(grandparentDir) == "skills" && !hasHiddenSegment(entry.Path) && !hasPluginsAncestor(entry.Path) { + namespace := path.Base(parentDir) + if !validateName(namespace) { + return nil + } + return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "skills-namespaced"} + } + if parentDir == "." && skillName != "skills" && skillName != "plugins" && !strings.HasPrefix(skillName, ".") { return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "root"} } @@ -534,6 +552,7 @@ func DiscoverSkillsWithOptions(client *api.Client, host, owner, repo, commitSHA return nil, fmt.Errorf( "no skills found in %s/%s\n"+ " Expected skills in skills/*/SKILL.md, skills/{scope}/*/SKILL.md,\n"+ + " {prefix}/skills/*/SKILL.md, {prefix}/skills/{scope}/*/SKILL.md,\n"+ " */SKILL.md, or plugins/*/skills/*/SKILL.md\n"+ " This repository may be a curated list rather than a skills publisher", owner, repo, @@ -667,18 +686,35 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill return nil, fmt.Errorf("no SKILL.md found in %s", skillPath) } - var namespace string + var namespace, convention string parts := strings.Split(skillPath, "/") - if len(parts) >= 3 && parts[0] == "skills" { - namespace = parts[1] + for i, p := range parts { + if p != "skills" { + continue + } + + // Plugin convention: .../plugins//skills/ + if i >= 2 && parts[i-2] == "plugins" { + namespace = parts[i-1] + convention = "plugins" + break + } + + // Namespaced skill convention: .../skills// + afterSkills := parts[i+1:] + if len(afterSkills) >= 2 { + namespace = afterSkills[0] + } + break } skill := &Skill{ - Name: skillName, - Namespace: namespace, - Path: skillPath, - BlobSHA: blobSHA, - TreeSHA: treeSHA, + Name: skillName, + Namespace: namespace, + Convention: convention, + Path: skillPath, + BlobSHA: blobSHA, + TreeSHA: treeSHA, } skill.Description = fetchDescription(client, host, owner, repo, skill) @@ -907,7 +943,9 @@ func DiscoverLocalSkillsWithOptions(dir string, opts DiscoverOptions) ([]Skill, 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, */SKILL.md, or plugins/*/skills/*/SKILL.md", + " skills/{scope}/*/SKILL.md, {prefix}/skills/*/SKILL.md,\n"+ + " {prefix}/skills/{scope}/*/SKILL.md, */SKILL.md, or\n"+ + " plugins/*/skills/*/SKILL.md", dir, ) } @@ -955,6 +993,26 @@ func validateName(name string) bool { return safeNamePattern.MatchString(name) } +// hasHiddenSegment reports whether any path component starts with a dot. +func hasHiddenSegment(p string) bool { + for _, seg := range strings.Split(p, "/") { + if strings.HasPrefix(seg, ".") { + return true + } + } + return false +} + +// hasPluginsAncestor reports whether any path component is "plugins". +func hasPluginsAncestor(p string) bool { + for _, seg := range strings.Split(p, "/") { + if seg == "plugins" { + return true + } + } + return false +} + // IsSpecCompliant checks if a skill name matches the strict agentskills.io spec. func IsSpecCompliant(name string) bool { if len(name) == 0 || len(name) > 64 { diff --git a/internal/skills/discovery/discovery_test.go b/internal/skills/discovery/discovery_test.go index 41600f21c72..bc3ce979b54 100644 --- a/internal/skills/discovery/discovery_test.go +++ b/internal/skills/discovery/discovery_test.go @@ -100,6 +100,52 @@ func TestMatchSkillConventions(t *testing.T) { path: ".hidden/SKILL.md", wantNil: true, }, + { + name: "nested skills directory", + path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md", + wantName: "terraform-style-guide", + wantConvention: "skills", + }, + { + name: "deeply nested skills directory", + path: "a/b/c/skills/my-skill/SKILL.md", + wantName: "my-skill", + wantConvention: "skills", + }, + { + name: "nested namespaced skills directory", + path: "terraform/code-generation/skills/hashicorp/terraform-style-guide/SKILL.md", + wantName: "terraform-style-guide", + wantNamespace: "hashicorp", + wantConvention: "skills-namespaced", + }, + { + name: "single prefix before skills directory", + path: "packer/skills/packer-builder/SKILL.md", + wantName: "packer-builder", + wantConvention: "skills", + }, + { + name: "root-level skills still has priority", + path: "skills/code-review/SKILL.md", + wantName: "code-review", + wantConvention: "skills", + }, + { + name: "nested skills dir itself is not a skill", + path: "terraform/skills/SKILL.md", + wantNil: true, + }, + { + name: "nested skills under hidden dir excluded", + path: ".claude/skills/code-review/SKILL.md", + wantNil: true, + }, + { + name: "nested plugins skills not matched as plain skills", + path: "vendor/plugins/hubot/skills/pr-summary/SKILL.md", + wantNil: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -865,6 +911,41 @@ func TestDiscoverSkills(t *testing.T) { }, wantSkills: []string{"code-review"}, }, + { + name: "discovers skills in nested skills directory", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/abc123"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "abc123", "truncated": false, + "tree": []map[string]interface{}{ + {"path": "terraform/code-generation/skills/terraform-style-guide", "type": "tree", "sha": "tree-sha-1"}, + {"path": "terraform/code-generation/skills/terraform-style-guide/SKILL.md", "type": "blob", "sha": "blob-1"}, + {"path": "terraform/code-generation/skills/terraform-test", "type": "tree", "sha": "tree-sha-2"}, + {"path": "terraform/code-generation/skills/terraform-test/SKILL.md", "type": "blob", "sha": "blob-2"}, + {"path": "README.md", "type": "blob", "sha": "readme"}, + }, + })) + }, + wantSkills: []string{"terraform-style-guide", "terraform-test"}, + }, + { + name: "discovers mixed root-level and nested skills", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/abc123"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "abc123", "truncated": false, + "tree": []map[string]interface{}{ + {"path": "skills/code-review", "type": "tree", "sha": "tree-sha-1"}, + {"path": "skills/code-review/SKILL.md", "type": "blob", "sha": "blob-1"}, + {"path": "terraform/skills/tf-lint", "type": "tree", "sha": "tree-sha-2"}, + {"path": "terraform/skills/tf-lint/SKILL.md", "type": "blob", "sha": "blob-2"}, + }, + })) + }, + wantSkills: []string{"code-review", "tf-lint"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -967,12 +1048,13 @@ func TestDiscoverSkillsWithOptions(t *testing.T) { func TestDiscoverSkillByPath(t *testing.T) { tests := []struct { - name string - skillPath string - stubs func(*httpmock.Registry) - wantName string - wantNS string - wantErr string + name string + skillPath string + stubs func(*httpmock.Registry) + wantName string + wantNS string + wantConvention string + wantErr string }{ { name: "discovers skill by path", @@ -1112,6 +1194,84 @@ func TestDiscoverSkillByPath(t *testing.T) { }, wantErr: "no SKILL.md found", }, + { + name: "deeply nested path discovers skill", + skillPath: "terraform/code-generation/skills/terraform-style-guide", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/terraform%2Fcode-generation%2Fskills"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "terraform-style-guide", "path": "terraform/code-generation/skills/terraform-style-guide", "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"}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==", + })) + }, + wantName: "terraform-style-guide", + }, + { + name: "deeply nested namespaced path sets namespace", + skillPath: "terraform/code-generation/skills/hashicorp/terraform-style-guide", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/terraform%2Fcode-generation%2Fskills%2Fhashicorp"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "terraform-style-guide", "path": "terraform/code-generation/skills/hashicorp/terraform-style-guide", "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"}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==", + })) + }, + wantName: "terraform-style-guide", + wantNS: "hashicorp", + }, + { + name: "plugins path sets namespace and convention", + skillPath: "plugins/hubot/skills/pr-summary", + stubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/plugins%2Fhubot%2Fskills"), + httpmock.JSONResponse([]map[string]interface{}{ + {"name": "pr-summary", "path": "plugins/hubot/skills/pr-summary", "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"}, + }, + })) + reg.Register( + httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"), + httpmock.JSONResponse(map[string]interface{}{ + "sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==", + })) + }, + wantName: "pr-summary", + wantNS: "hubot", + wantConvention: "plugins", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1131,6 +1291,9 @@ func TestDiscoverSkillByPath(t *testing.T) { require.NoError(t, err) assert.Equal(t, tt.wantName, skill.Name) assert.Equal(t, tt.wantNS, skill.Namespace) + if tt.wantConvention != "" { + assert.Equal(t, tt.wantConvention, skill.Convention) + } }) } } @@ -1184,6 +1347,19 @@ func TestDiscoverLocalSkills(t *testing.T) { setup: func(t *testing.T, dir string) {}, wantErr: "could not access", }, + { + name: "discovers skills in nested skills/ directory", + createDir: true, + setup: func(t *testing.T, dir string) { + t.Helper() + for _, name := range []string{"terraform-style-guide", "terraform-test"} { + skillDir := filepath.Join(dir, "terraform", "code-generation", "skills", name) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("# "+name), 0o644)) + } + }, + wantSkills: []string{"terraform-style-guide", "terraform-test"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1278,6 +1454,7 @@ func TestMatchesSkillPath(t *testing.T) { {name: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary"}, {name: "non-skill file", path: "README.md", wantName: ""}, {name: "non-SKILL.md in skill dir", path: "skills/code-review/prompt.txt", wantName: ""}, + {name: "nested skills convention", path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1300,6 +1477,8 @@ func TestMatchSkillPath(t *testing.T) { {name: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"}, {name: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"}, {name: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""}, + {name: "nested skills convention", path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide", wantNamespace: ""}, + {name: "nested namespaced convention", path: "terraform/code-generation/skills/hashicorp/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide", wantNamespace: "hashicorp"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index 88cd2673163..d792501fd27 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -107,7 +107,9 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru tracking metadata injected into frontmatter. Skills are discovered automatically using the %[1]sskills/*/SKILL.md%[1]s convention - defined by the Agent Skills specification. For more information on the specification, + defined by the Agent Skills specification, including when the %[1]sskills/%[1]s + directory is nested under a prefix (e.g. %[1]sterraform/code-generation/skills/...%[1]s). + For more information on the specification, see: https://agentskills.io/specification The skill argument can be a name, a namespaced name (%[1]sauthor/skill%[1]s), @@ -504,6 +506,9 @@ func isSkillPath(name string) bool { if strings.HasPrefix(name, "skills/") || strings.HasPrefix(name, "plugins/") { return true } + if strings.Contains(name, "/skills/") || strings.Contains(name, "/plugins/") { + return true + } return false } diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index c557c93e7fa..e4bd074bd5f 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "fmt" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -231,7 +232,7 @@ func stubSkillByPath(reg *httpmock.Registry, owner, repo, sha, skillPath, skillN parentPath = skillPath[:idx] } reg.Register( - httpmock.REST("GET", fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, parentPath)), + httpmock.REST("GET", fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, url.PathEscape(parentPath))), httpmock.StringResponse(fmt.Sprintf(`[{"name": %q, "path": %q, "sha": %q, "type": "dir"}]`, skillName, skillPath, treeSHA)), ) } @@ -759,6 +760,34 @@ func TestInstallRun(t *testing.T) { }, wantStdout: "Installed git-commit", }, + { + name: "remote install by nested skill path skips full discovery", + isTTY: true, + stubs: func(reg *httpmock.Registry) { + stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123") + stubSkillByPath(reg, "monalisa", "skills-repo", "abc123", + "terraform/code-generation/skills/terraform-style-guide", "terraform-style-guide", "treeSHA") + // DiscoverSkillByPath: tree + blob (for fetchDescription) + stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) + // installer.Install: tree + blob (again, for writing files) + stubInstallFiles(reg, "monalisa", "skills-repo", "treeSHA", "blobSHA", gitCommitContent) + }, + opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { + t.Helper() + return &InstallOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, + GitClient: &git.Client{RepoDir: t.TempDir()}, + SkillSource: "monalisa/skills-repo", + SkillName: "terraform/code-generation/skills/terraform-style-guide", + Agent: "github-copilot", + Scope: "project", + ScopeChanged: true, + Dir: t.TempDir(), + } + }, + wantStdout: "Installed terraform-style-guide", + }, { name: "remote install with URL repo argument", isTTY: true, @@ -2075,6 +2104,31 @@ func TestRunLocalInstall(t *testing.T) { } } +func Test_isSkillPath(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: "SKILL.md at root", path: "SKILL.md", want: true}, + {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: "name containing skills substring", path: "myskills", 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 Test_printReviewHint(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index e9f1e0442ce..1c9d4d91329 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -395,6 +395,13 @@ func selectSkill(opts *PreviewOptions, skills []discovery.Skill) (discovery.Skil return s, nil } } + // Fall back to InstallName so that namespaced identifiers produced + // by the post-install hint (e.g. "namespace/skill") are accepted. + for _, s := range skills { + if s.InstallName() == opts.SkillName { + return s, nil + } + } return discovery.Skill{}, fmt.Errorf("skill %q not found in %s", opts.SkillName, ghrepo.FullName(opts.repo)) } diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index a5d5554ffe9..be3c861170e 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -206,6 +206,51 @@ func TestPreviewRun(t *testing.T) { }, wantStdout: "My Skill", }, + { + name: "preview plugins skill matched by install name", + tty: true, + opts: &PreviewOptions{ + repo: ghrepo.New("owner", "repo"), + SkillName: "aws-common/aws-mcp-setup", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/owner/repo/releases/latest"), + httpmock.StringResponse(`{"tag_name": "v1.0.0"}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/ref/tags/v1.0.0"), + httpmock.StringResponse(`{"object": {"sha": "abc123", "type": "commit"}}`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/abc123"), + httpmock.StringResponse(`{ + "sha": "abc123", + "truncated": false, + "tree": [ + {"path": "plugins", "type": "tree", "sha": "tree-plugins"}, + {"path": "plugins/aws-common", "type": "tree", "sha": "tree-awscommon"}, + {"path": "plugins/aws-common/skills", "type": "tree", "sha": "tree-awsskills"}, + {"path": "plugins/aws-common/skills/aws-mcp-setup", "type": "tree", "sha": "treeSHA3"}, + {"path": "plugins/aws-common/skills/aws-mcp-setup/SKILL.md", "type": "blob", "sha": "blob789"} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/trees/treeSHA3"), + httpmock.StringResponse(`{ + "tree": [ + {"path": "SKILL.md", "type": "blob", "sha": "blob789", "size": 50} + ] + }`), + ) + reg.Register( + httpmock.REST("GET", "repos/owner/repo/git/blobs/blob789"), + httpmock.StringResponse(`{"sha": "blob789", "content": "`+encodedContent+`", "encoding": "base64"}`), + ) + }, + wantStdout: "My Skill", + }, { name: "skill not found", tty: true,