Skip to content

Commit a67f4f7

Browse files
SamMorrowDrumsSamMorrowDrums
authored
Merge pull request cli#13235 from cli/sammorrowdrums/fix-skill-install-discovery
Make skill discovery less strict: support nested `skills/` directories
2 parents 49d4747 + 9a368f4 commit a67f4f7

4 files changed

Lines changed: 313 additions & 17 deletions

File tree

‎internal/skills/discovery/discovery.go‎

Lines changed: 67 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,24 @@ func matchSkillConventions(entry treeEntry) *skillMatch {
405405
return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "plugins"}
406406
}
407407

408+
// Deeply nested skills/ directory: <prefix>/skills/<name>/SKILL.md
409+
// Matches skills/ at any depth, not just at the repository root.
410+
// Exclude paths with dot-prefixed segments (handled by
411+
// matchHiddenDirConventions) and paths under a plugins/ directory
412+
// (handled by the plugins convention above).
413+
if path.Base(parentDir) == "skills" && !hasHiddenSegment(entry.Path) && !hasPluginsAncestor(entry.Path) {
414+
return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "skills"}
415+
}
416+
417+
// Deeply nested namespaced: <prefix>/skills/<namespace>/<name>/SKILL.md
418+
if path.Base(grandparentDir) == "skills" && !hasHiddenSegment(entry.Path) && !hasPluginsAncestor(entry.Path) {
419+
namespace := path.Base(parentDir)
420+
if !validateName(namespace) {
421+
return nil
422+
}
423+
return &skillMatch{entry: entry, name: skillName, namespace: namespace, skillDir: dir, convention: "skills-namespaced"}
424+
}
425+
408426
if parentDir == "." && skillName != "skills" && skillName != "plugins" && !strings.HasPrefix(skillName, ".") {
409427
return &skillMatch{entry: entry, name: skillName, skillDir: dir, convention: "root"}
410428
}
@@ -534,6 +552,7 @@ func DiscoverSkillsWithOptions(client *api.Client, host, owner, repo, commitSHA
534552
return nil, fmt.Errorf(
535553
"no skills found in %s/%s\n"+
536554
" Expected skills in skills/*/SKILL.md, skills/{scope}/*/SKILL.md,\n"+
555+
" {prefix}/skills/*/SKILL.md, {prefix}/skills/{scope}/*/SKILL.md,\n"+
537556
" */SKILL.md, or plugins/*/skills/*/SKILL.md\n"+
538557
" This repository may be a curated list rather than a skills publisher",
539558
owner, repo,
@@ -667,18 +686,35 @@ func DiscoverSkillByPath(client *api.Client, host, owner, repo, commitSHA, skill
667686
return nil, fmt.Errorf("no SKILL.md found in %s", skillPath)
668687
}
669688

670-
var namespace string
689+
var namespace, convention string
671690
parts := strings.Split(skillPath, "/")
672-
if len(parts) >= 3 && parts[0] == "skills" {
673-
namespace = parts[1]
691+
for i, p := range parts {
692+
if p != "skills" {
693+
continue
694+
}
695+
696+
// Plugin convention: .../plugins/<ns>/skills/<name>
697+
if i >= 2 && parts[i-2] == "plugins" {
698+
namespace = parts[i-1]
699+
convention = "plugins"
700+
break
701+
}
702+
703+
// Namespaced skill convention: .../skills/<ns>/<name>
704+
afterSkills := parts[i+1:]
705+
if len(afterSkills) >= 2 {
706+
namespace = afterSkills[0]
707+
}
708+
break
674709
}
675710

676711
skill := &Skill{
677-
Name: skillName,
678-
Namespace: namespace,
679-
Path: skillPath,
680-
BlobSHA: blobSHA,
681-
TreeSHA: treeSHA,
712+
Name: skillName,
713+
Namespace: namespace,
714+
Convention: convention,
715+
Path: skillPath,
716+
BlobSHA: blobSHA,
717+
TreeSHA: treeSHA,
682718
}
683719

684720
skill.Description = fetchDescription(client, host, owner, repo, skill)
@@ -907,7 +943,9 @@ func DiscoverLocalSkillsWithOptions(dir string, opts DiscoverOptions) ([]Skill,
907943
return nil, fmt.Errorf(
908944
"no skills found in %s\n"+
909945
" Expected SKILL.md in the directory, or skills in skills/*/SKILL.md,\n"+
910-
" skills/{scope}/*/SKILL.md, */SKILL.md, or plugins/*/skills/*/SKILL.md",
946+
" skills/{scope}/*/SKILL.md, {prefix}/skills/*/SKILL.md,\n"+
947+
" {prefix}/skills/{scope}/*/SKILL.md, */SKILL.md, or\n"+
948+
" plugins/*/skills/*/SKILL.md",
911949
dir,
912950
)
913951
}
@@ -955,6 +993,26 @@ func validateName(name string) bool {
955993
return safeNamePattern.MatchString(name)
956994
}
957995

996+
// hasHiddenSegment reports whether any path component starts with a dot.
997+
func hasHiddenSegment(p string) bool {
998+
for _, seg := range strings.Split(p, "/") {
999+
if strings.HasPrefix(seg, ".") {
1000+
return true
1001+
}
1002+
}
1003+
return false
1004+
}
1005+
1006+
// hasPluginsAncestor reports whether any path component is "plugins".
1007+
func hasPluginsAncestor(p string) bool {
1008+
for _, seg := range strings.Split(p, "/") {
1009+
if seg == "plugins" {
1010+
return true
1011+
}
1012+
}
1013+
return false
1014+
}
1015+
9581016
// IsSpecCompliant checks if a skill name matches the strict agentskills.io spec.
9591017
func IsSpecCompliant(name string) bool {
9601018
if len(name) == 0 || len(name) > 64 {

‎internal/skills/discovery/discovery_test.go‎

Lines changed: 185 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,52 @@ func TestMatchSkillConventions(t *testing.T) {
100100
path: ".hidden/SKILL.md",
101101
wantNil: true,
102102
},
103+
{
104+
name: "nested skills directory",
105+
path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md",
106+
wantName: "terraform-style-guide",
107+
wantConvention: "skills",
108+
},
109+
{
110+
name: "deeply nested skills directory",
111+
path: "a/b/c/skills/my-skill/SKILL.md",
112+
wantName: "my-skill",
113+
wantConvention: "skills",
114+
},
115+
{
116+
name: "nested namespaced skills directory",
117+
path: "terraform/code-generation/skills/hashicorp/terraform-style-guide/SKILL.md",
118+
wantName: "terraform-style-guide",
119+
wantNamespace: "hashicorp",
120+
wantConvention: "skills-namespaced",
121+
},
122+
{
123+
name: "single prefix before skills directory",
124+
path: "packer/skills/packer-builder/SKILL.md",
125+
wantName: "packer-builder",
126+
wantConvention: "skills",
127+
},
128+
{
129+
name: "root-level skills still has priority",
130+
path: "skills/code-review/SKILL.md",
131+
wantName: "code-review",
132+
wantConvention: "skills",
133+
},
134+
{
135+
name: "nested skills dir itself is not a skill",
136+
path: "terraform/skills/SKILL.md",
137+
wantNil: true,
138+
},
139+
{
140+
name: "nested skills under hidden dir excluded",
141+
path: ".claude/skills/code-review/SKILL.md",
142+
wantNil: true,
143+
},
144+
{
145+
name: "nested plugins skills not matched as plain skills",
146+
path: "vendor/plugins/hubot/skills/pr-summary/SKILL.md",
147+
wantNil: true,
148+
},
103149
}
104150
for _, tt := range tests {
105151
t.Run(tt.name, func(t *testing.T) {
@@ -865,6 +911,41 @@ func TestDiscoverSkills(t *testing.T) {
865911
},
866912
wantSkills: []string{"code-review"},
867913
},
914+
{
915+
name: "discovers skills in nested skills directory",
916+
stubs: func(reg *httpmock.Registry) {
917+
reg.Register(
918+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/abc123"),
919+
httpmock.JSONResponse(map[string]interface{}{
920+
"sha": "abc123", "truncated": false,
921+
"tree": []map[string]interface{}{
922+
{"path": "terraform/code-generation/skills/terraform-style-guide", "type": "tree", "sha": "tree-sha-1"},
923+
{"path": "terraform/code-generation/skills/terraform-style-guide/SKILL.md", "type": "blob", "sha": "blob-1"},
924+
{"path": "terraform/code-generation/skills/terraform-test", "type": "tree", "sha": "tree-sha-2"},
925+
{"path": "terraform/code-generation/skills/terraform-test/SKILL.md", "type": "blob", "sha": "blob-2"},
926+
{"path": "README.md", "type": "blob", "sha": "readme"},
927+
},
928+
}))
929+
},
930+
wantSkills: []string{"terraform-style-guide", "terraform-test"},
931+
},
932+
{
933+
name: "discovers mixed root-level and nested skills",
934+
stubs: func(reg *httpmock.Registry) {
935+
reg.Register(
936+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/abc123"),
937+
httpmock.JSONResponse(map[string]interface{}{
938+
"sha": "abc123", "truncated": false,
939+
"tree": []map[string]interface{}{
940+
{"path": "skills/code-review", "type": "tree", "sha": "tree-sha-1"},
941+
{"path": "skills/code-review/SKILL.md", "type": "blob", "sha": "blob-1"},
942+
{"path": "terraform/skills/tf-lint", "type": "tree", "sha": "tree-sha-2"},
943+
{"path": "terraform/skills/tf-lint/SKILL.md", "type": "blob", "sha": "blob-2"},
944+
},
945+
}))
946+
},
947+
wantSkills: []string{"code-review", "tf-lint"},
948+
},
868949
}
869950
for _, tt := range tests {
870951
t.Run(tt.name, func(t *testing.T) {
@@ -967,12 +1048,13 @@ func TestDiscoverSkillsWithOptions(t *testing.T) {
9671048

9681049
func TestDiscoverSkillByPath(t *testing.T) {
9691050
tests := []struct {
970-
name string
971-
skillPath string
972-
stubs func(*httpmock.Registry)
973-
wantName string
974-
wantNS string
975-
wantErr string
1051+
name string
1052+
skillPath string
1053+
stubs func(*httpmock.Registry)
1054+
wantName string
1055+
wantNS string
1056+
wantConvention string
1057+
wantErr string
9761058
}{
9771059
{
9781060
name: "discovers skill by path",
@@ -1112,6 +1194,84 @@ func TestDiscoverSkillByPath(t *testing.T) {
11121194
},
11131195
wantErr: "no SKILL.md found",
11141196
},
1197+
{
1198+
name: "deeply nested path discovers skill",
1199+
skillPath: "terraform/code-generation/skills/terraform-style-guide",
1200+
stubs: func(reg *httpmock.Registry) {
1201+
reg.Register(
1202+
httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/terraform%2Fcode-generation%2Fskills"),
1203+
httpmock.JSONResponse([]map[string]interface{}{
1204+
{"name": "terraform-style-guide", "path": "terraform/code-generation/skills/terraform-style-guide", "sha": "tree-sha", "type": "dir"},
1205+
}))
1206+
reg.Register(
1207+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree-sha"),
1208+
httpmock.JSONResponse(map[string]interface{}{
1209+
"sha": "tree-sha", "truncated": false,
1210+
"tree": []map[string]interface{}{
1211+
{"path": "SKILL.md", "type": "blob", "sha": "blob-sha"},
1212+
},
1213+
}))
1214+
reg.Register(
1215+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"),
1216+
httpmock.JSONResponse(map[string]interface{}{
1217+
"sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==",
1218+
}))
1219+
},
1220+
wantName: "terraform-style-guide",
1221+
},
1222+
{
1223+
name: "deeply nested namespaced path sets namespace",
1224+
skillPath: "terraform/code-generation/skills/hashicorp/terraform-style-guide",
1225+
stubs: func(reg *httpmock.Registry) {
1226+
reg.Register(
1227+
httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/terraform%2Fcode-generation%2Fskills%2Fhashicorp"),
1228+
httpmock.JSONResponse([]map[string]interface{}{
1229+
{"name": "terraform-style-guide", "path": "terraform/code-generation/skills/hashicorp/terraform-style-guide", "sha": "tree-sha", "type": "dir"},
1230+
}))
1231+
reg.Register(
1232+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree-sha"),
1233+
httpmock.JSONResponse(map[string]interface{}{
1234+
"sha": "tree-sha", "truncated": false,
1235+
"tree": []map[string]interface{}{
1236+
{"path": "SKILL.md", "type": "blob", "sha": "blob-sha"},
1237+
},
1238+
}))
1239+
reg.Register(
1240+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"),
1241+
httpmock.JSONResponse(map[string]interface{}{
1242+
"sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==",
1243+
}))
1244+
},
1245+
wantName: "terraform-style-guide",
1246+
wantNS: "hashicorp",
1247+
},
1248+
{
1249+
name: "plugins path sets namespace and convention",
1250+
skillPath: "plugins/hubot/skills/pr-summary",
1251+
stubs: func(reg *httpmock.Registry) {
1252+
reg.Register(
1253+
httpmock.REST("GET", "repos/monalisa/octocat-skills/contents/plugins%2Fhubot%2Fskills"),
1254+
httpmock.JSONResponse([]map[string]interface{}{
1255+
{"name": "pr-summary", "path": "plugins/hubot/skills/pr-summary", "sha": "tree-sha", "type": "dir"},
1256+
}))
1257+
reg.Register(
1258+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/trees/tree-sha"),
1259+
httpmock.JSONResponse(map[string]interface{}{
1260+
"sha": "tree-sha", "truncated": false,
1261+
"tree": []map[string]interface{}{
1262+
{"path": "SKILL.md", "type": "blob", "sha": "blob-sha"},
1263+
},
1264+
}))
1265+
reg.Register(
1266+
httpmock.REST("GET", "repos/monalisa/octocat-skills/git/blobs/blob-sha"),
1267+
httpmock.JSONResponse(map[string]interface{}{
1268+
"sha": "blob-sha", "encoding": "base64", "content": "IyBTa2lsbA==",
1269+
}))
1270+
},
1271+
wantName: "pr-summary",
1272+
wantNS: "hubot",
1273+
wantConvention: "plugins",
1274+
},
11151275
}
11161276
for _, tt := range tests {
11171277
t.Run(tt.name, func(t *testing.T) {
@@ -1131,6 +1291,9 @@ func TestDiscoverSkillByPath(t *testing.T) {
11311291
require.NoError(t, err)
11321292
assert.Equal(t, tt.wantName, skill.Name)
11331293
assert.Equal(t, tt.wantNS, skill.Namespace)
1294+
if tt.wantConvention != "" {
1295+
assert.Equal(t, tt.wantConvention, skill.Convention)
1296+
}
11341297
})
11351298
}
11361299
}
@@ -1184,6 +1347,19 @@ func TestDiscoverLocalSkills(t *testing.T) {
11841347
setup: func(t *testing.T, dir string) {},
11851348
wantErr: "could not access",
11861349
},
1350+
{
1351+
name: "discovers skills in nested skills/ directory",
1352+
createDir: true,
1353+
setup: func(t *testing.T, dir string) {
1354+
t.Helper()
1355+
for _, name := range []string{"terraform-style-guide", "terraform-test"} {
1356+
skillDir := filepath.Join(dir, "terraform", "code-generation", "skills", name)
1357+
require.NoError(t, os.MkdirAll(skillDir, 0o755))
1358+
require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("# "+name), 0o644))
1359+
}
1360+
},
1361+
wantSkills: []string{"terraform-style-guide", "terraform-test"},
1362+
},
11871363
}
11881364
for _, tt := range tests {
11891365
t.Run(tt.name, func(t *testing.T) {
@@ -1278,6 +1454,7 @@ func TestMatchesSkillPath(t *testing.T) {
12781454
{name: "plugins convention", path: "plugins/hubot/skills/pr-summary/SKILL.md", wantName: "pr-summary"},
12791455
{name: "non-skill file", path: "README.md", wantName: ""},
12801456
{name: "non-SKILL.md in skill dir", path: "skills/code-review/prompt.txt", wantName: ""},
1457+
{name: "nested skills convention", path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide"},
12811458
}
12821459
for _, tt := range tests {
12831460
t.Run(tt.name, func(t *testing.T) {
@@ -1300,6 +1477,8 @@ func TestMatchSkillPath(t *testing.T) {
13001477
{name: "same name different namespace 1", path: "skills/kynan/commit/SKILL.md", wantName: "commit", wantNamespace: "kynan"},
13011478
{name: "same name different namespace 2", path: "skills/will/commit/SKILL.md", wantName: "commit", wantNamespace: "will"},
13021479
{name: "root convention", path: "my-skill/SKILL.md", wantName: "my-skill", wantNamespace: ""},
1480+
{name: "nested skills convention", path: "terraform/code-generation/skills/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide", wantNamespace: ""},
1481+
{name: "nested namespaced convention", path: "terraform/code-generation/skills/hashicorp/terraform-style-guide/SKILL.md", wantName: "terraform-style-guide", wantNamespace: "hashicorp"},
13031482
}
13041483
for _, tt := range tests {
13051484
t.Run(tt.name, func(t *testing.T) {

‎pkg/cmd/skills/install/install.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,9 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru
107107
tracking metadata injected into frontmatter.
108108
109109
Skills are discovered automatically using the %[1]sskills/*/SKILL.md%[1]s convention
110-
defined by the Agent Skills specification. For more information on the specification,
110+
defined by the Agent Skills specification, including when the %[1]sskills/%[1]s
111+
directory is nested under a prefix (e.g. %[1]sterraform/code-generation/skills/...%[1]s).
112+
For more information on the specification,
111113
see: https://agentskills.io/specification
112114
113115
The skill argument can be a name, a namespaced name (%[1]sauthor/skill%[1]s),
@@ -504,6 +506,9 @@ func isSkillPath(name string) bool {
504506
if strings.HasPrefix(name, "skills/") || strings.HasPrefix(name, "plugins/") {
505507
return true
506508
}
509+
if strings.Contains(name, "/skills/") || strings.Contains(name, "/plugins/") {
510+
return true
511+
}
507512
return false
508513
}
509514

0 commit comments

Comments
 (0)