From 2e93afc272cf2c74f06ff4e4f40bbb59b41f7dde Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 01:26:31 +0200 Subject: [PATCH] Install skills flat by Name, not namespaced InstallName Most agent clients (Claude Code, Copilot, etc.) only discover immediate subdirectories of their skills folder. When a skill repository used namespaced paths like skills/author/my-skill/, the installer created nested directories (e.g. .claude/skills/author/my-skill/) that clients could not find. This separates the skill's identity (InstallName, used for lockfile keys, search, filtering, display) from the filesystem path (Name, used for the install directory). Skills are now always installed flat: .claude/skills/my-skill/SKILL.md (not .claude/skills/author/my-skill/) Changes: - installer: use skill.Name for directory paths instead of InstallName - install.go: use skill.Name for overwrite checks and prompts - collisions: detect conflicts by Name since flat install means two skills with the same Name but different Namespace values will collide - update: clean up old namespaced directories when migrating to flat Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- internal/skills/discovery/collisions.go | 16 +++++---- internal/skills/installer/installer.go | 8 +++-- pkg/cmd/skills/install/install.go | 4 +-- pkg/cmd/skills/install/install_test.go | 45 ++++++++++--------------- pkg/cmd/skills/update/update.go | 18 ++++++++++ pkg/cmd/skills/update/update_test.go | 6 +++- 6 files changed, 57 insertions(+), 40 deletions(-) diff --git a/internal/skills/discovery/collisions.go b/internal/skills/discovery/collisions.go index 38bf9b26b..6aae3c7b7 100644 --- a/internal/skills/discovery/collisions.go +++ b/internal/skills/discovery/collisions.go @@ -6,20 +6,22 @@ import ( "strings" ) -// NameCollision represents a group of skills that share the same InstallName -// and would overwrite each other when installed to the same directory. +// NameCollision represents a group of skills that share the same install +// directory name and would overwrite each other when installed. type NameCollision struct { - Name string // the conflicting install name (may include namespace prefix) + Name string // the conflicting skill name (directory name) DisplayNames []string // display names of each conflicting skill } -// FindNameCollisions detects skills that share the same InstallName and returns a -// sorted slice of collisions. Callers decide how to present the conflict to -// the user (different flows need different error messages). +// FindNameCollisions detects skills whose Name fields collide (meaning they +// would be installed to the same directory) and returns a sorted slice of +// collisions. Skills are installed flat by Name, so two skills with the same +// Name but different Namespace values still conflict. Callers decide how to +// present the conflict to the user. func FindNameCollisions(skills []Skill) []NameCollision { byName := make(map[string][]Skill) for _, s := range skills { - byName[s.InstallName()] = append(byName[s.InstallName()], s) + byName[s.Name] = append(byName[s.Name], s) } var collisions []NameCollision diff --git a/internal/skills/installer/installer.go b/internal/skills/installer/installer.go index e27d35f5b..7a355093d 100644 --- a/internal/skills/installer/installer.go +++ b/internal/skills/installer/installer.go @@ -178,7 +178,10 @@ func InstallLocal(opts *LocalOptions) (*Result, error) { } func installLocalSkill(sourceRoot string, skill discovery.Skill, baseDir string) error { - skillDir := filepath.Join(baseDir, filepath.FromSlash(skill.InstallName())) + // Use skill.Name (not InstallName) so skills are always installed flat. + // Most agent clients only discover immediate subdirectories of their + // skills folder and do not find skills nested under namespace directories. + skillDir := filepath.Join(baseDir, skill.Name) if err := os.MkdirAll(skillDir, 0o755); err != nil { return fmt.Errorf("could not create directory %s: %w", skillDir, err) } @@ -246,7 +249,8 @@ func installLocalSkill(sourceRoot string, skill discovery.Skill, baseDir string) } func installSkill(opts *Options, skill discovery.Skill, baseDir string) error { - skillDir := filepath.Join(baseDir, filepath.FromSlash(skill.InstallName())) + // Use skill.Name (not InstallName) for a flat directory layout. + skillDir := filepath.Join(baseDir, skill.Name) if err := os.MkdirAll(skillDir, 0o755); err != nil { return fmt.Errorf("could not create directory %s: %w", skillDir, err) } diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index a22249225..6c0988531 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -971,7 +971,7 @@ func truncateDescription(s string, maxWidth int) string { func checkOverwrite(opts *InstallOptions, skills []discovery.Skill, targetDir string, canPrompt bool) ([]discovery.Skill, error) { var existing, fresh []discovery.Skill for _, s := range skills { - dir := filepath.Join(targetDir, filepath.FromSlash(s.InstallName())) + dir := filepath.Join(targetDir, s.Name) if _, err := os.Stat(dir); err == nil { existing = append(existing, s) } else { @@ -1013,7 +1013,7 @@ func checkOverwrite(opts *InstallOptions, skills []discovery.Skill, targetDir st } func existingSkillPrompt(targetDir string, incoming discovery.Skill) string { - skillFile := filepath.Join(targetDir, filepath.FromSlash(incoming.InstallName()), "SKILL.md") + skillFile := filepath.Join(targetDir, incoming.Name, "SKILL.md") data, err := os.ReadFile(skillFile) if err != nil { return fmt.Sprintf("Skill %q already exists. Overwrite?", incoming.DisplayName()) diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index b7aa956c5..200332592 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -857,7 +857,7 @@ func TestInstallRun(t *testing.T) { wantErr: "conflicting names", }, { - name: "remote install all with namespaced skills avoids collisions", + name: "remote install all with namespaced skills detects collisions", isTTY: true, stubs: func(reg *httpmock.Registry) { stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123") @@ -868,7 +868,7 @@ func TestInstallRun(t *testing.T) { `{"path": "skills/bob/xlsx-pro", "type": "tree", "sha": "treeB"}, ` + `{"path": "skills/bob/xlsx-pro/SKILL.md", "type": "blob", "sha": "blobB"}` stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123", treeJSON) - // Extra blob stubs consumed by FetchDescriptionsConcurrent during interactive selection. + // Blob stubs consumed by FetchDescriptionsConcurrent during interactive selection. contentA := base64.StdEncoding.EncodeToString([]byte("---\nname: xlsx-pro\ndescription: Alice\n---\n# A\n")) contentB := base64.StdEncoding.EncodeToString([]byte("---\nname: xlsx-pro\ndescription: Bob\n---\n# B\n")) reg.Register( @@ -877,10 +877,6 @@ func TestInstallRun(t *testing.T) { reg.Register( httpmock.REST("GET", "repos/monalisa/skills-repo/git/blobs/blobB"), httpmock.StringResponse(fmt.Sprintf(`{"sha": "blobB", "content": %q, "encoding": "base64"}`, contentB))) - stubInstallFiles(reg, "monalisa", "skills-repo", "treeA", "blobA", - "---\nname: xlsx-pro\ndescription: Alice\n---\n# A\n") - stubInstallFiles(reg, "monalisa", "skills-repo", "treeB", "blobB", - "---\nname: xlsx-pro\ndescription: Bob\n---\n# B\n") }, opts: func(ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions { t.Helper() @@ -901,7 +897,7 @@ func TestInstallRun(t *testing.T) { Dir: t.TempDir(), } }, - wantStdout: "Installed", + wantErr: "conflicting names", }, { name: "remote install friendlyDir shows tilde for home paths", @@ -1670,7 +1666,7 @@ func TestRunLocalInstall(t *testing.T) { wantStdout: "Installed direct-skill", }, { - name: "namespaced skills install to separate directories", + name: "namespaced skills with same name collide in flat install", isTTY: true, setup: func(t *testing.T, sourceDir, _ string) { t.Helper() @@ -1699,38 +1695,25 @@ func TestRunLocalInstall(t *testing.T) { GitClient: &git.Client{RepoDir: t.TempDir()}, } }, - verify: func(t *testing.T, targetDir string) { - t.Helper() - _, err := os.Stat(filepath.Join(targetDir, "alice", "xlsx-pro", "SKILL.md")) - assert.NoError(t, err, "alice/xlsx-pro should be installed") - _, err = os.Stat(filepath.Join(targetDir, "bob", "xlsx-pro", "SKILL.md")) - assert.NoError(t, err, "bob/xlsx-pro should be installed") - }, - wantStdout: "Installed alice/xlsx-pro", + wantErr: "conflicting names", }, { - name: "local install with --force overwrites namespaced skill", + name: "local install with --force overwrites namespaced skill flat", isTTY: true, setup: func(t *testing.T, sourceDir, targetDir string) { t.Helper() - for _, ns := range []string{"alice", "bob"} { - writeLocalTestSkill(t, sourceDir, filepath.Join("skills", ns, "xlsx-pro"), - fmt.Sprintf("---\nname: xlsx-pro\ndescription: %s xlsx-pro\n---\n# Test\n", ns)) - } - require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "alice", "xlsx-pro"), 0o755)) + writeLocalTestSkill(t, sourceDir, filepath.Join("skills", "alice", "xlsx-pro"), + "---\nname: xlsx-pro\ndescription: alice xlsx-pro\n---\n# Test\n") + require.NoError(t, os.MkdirAll(filepath.Join(targetDir, "xlsx-pro"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(targetDir, "xlsx-pro", "SKILL.md"), []byte("old"), 0o644)) }, opts: func(ios *iostreams.IOStreams, sourceDir, targetDir string) *InstallOptions { t.Helper() - pm := &prompter.PrompterMock{ - MultiSelectWithSearchFunc: func(_, _ string, _, _ []string, _ func(string) prompter.MultiSelectSearchResult) ([]string, error) { - return []string{allSkillsKey}, nil - }, - } return &InstallOptions{ IO: ios, SkillSource: sourceDir, localPath: sourceDir, - Prompter: pm, + SkillName: "xlsx-pro", Force: true, Agent: "github-copilot", Scope: "project", @@ -1739,6 +1722,12 @@ func TestRunLocalInstall(t *testing.T) { GitClient: &git.Client{RepoDir: t.TempDir()}, } }, + verify: func(t *testing.T, targetDir string) { + t.Helper() + content, err := os.ReadFile(filepath.Join(targetDir, "xlsx-pro", "SKILL.md")) + require.NoError(t, err) + assert.Contains(t, string(content), "alice xlsx-pro") + }, wantStdout: "Installed", }, { diff --git a/pkg/cmd/skills/update/update.go b/pkg/cmd/skills/update/update.go index 7923a6bde..8b0d831e0 100644 --- a/pkg/cmd/skills/update/update.go +++ b/pkg/cmd/skills/update/update.go @@ -414,6 +414,24 @@ func updateRun(opts *UpdateOptions) error { failed = true continue } + + // When the install location has changed (e.g. migrating from a + // namespaced layout to flat), remove the old directory so that the + // stale copy does not shadow the freshly installed one. + newDir := filepath.Join(installOpts.Dir, u.skill.Name) + if installOpts.Dir == "" && u.local.host != nil { + if d, err := u.local.host.InstallDir(u.local.scope, gitRoot, homeDir); err == nil { + newDir = filepath.Join(d, u.skill.Name) + } + } + if newDir != "" && u.local.dir != "" && filepath.Clean(newDir) != filepath.Clean(u.local.dir) { + _ = os.RemoveAll(u.local.dir) + // Remove the parent if it is now empty (leftover namespace directory). + parent := filepath.Dir(u.local.dir) + if entries, readErr := os.ReadDir(parent); readErr == nil && len(entries) == 0 { + _ = os.Remove(parent) + } + } if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.Out, "%s Updated %s\n", cs.SuccessIcon(), u.local.name) } else { diff --git a/pkg/cmd/skills/update/update_test.go b/pkg/cmd/skills/update/update_test.go index 86fdcaa80..752e652b9 100644 --- a/pkg/cmd/skills/update/update_test.go +++ b/pkg/cmd/skills/update/update_test.go @@ -726,10 +726,14 @@ func TestUpdateRun(t *testing.T) { }, verify: func(t *testing.T, dir string) { t.Helper() - content, err := os.ReadFile(filepath.Join(dir, "monalisa", "code-review", "SKILL.md")) + // After update, skill should be installed flat (not namespaced). + content, err := os.ReadFile(filepath.Join(dir, "code-review", "SKILL.md")) require.NoError(t, err) assert.Contains(t, string(content), "github-repo: https://github.com/monalisa/octocat-skills") assert.NotContains(t, string(content), "Old namespaced content") + // Old namespaced directory should be cleaned up. + _, err = os.Stat(filepath.Join(dir, "monalisa", "code-review")) + assert.True(t, os.IsNotExist(err), "old namespaced directory should be removed") }, wantStdout: "Updated monalisa/code-review", },