Merge pull request cli#13266 from cli/sammorrowdrums/fix-skill-install-flat-path

Install skills flat by Name, not namespaced InstallName

Author: SamMorrowDrums

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

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)
 	}

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())

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",
 		},
 		{

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 {

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",
 		},