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