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>
This commit is contained in:
parent
3ad29588b8
commit
2e93afc272
6 changed files with 57 additions and 40 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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())
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
},
|
||||
{
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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",
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue