diff --git a/README.md b/README.md index 2da99d9..97d2dd5 100644 --- a/README.md +++ b/README.md @@ -363,8 +363,19 @@ craft auto-detects your AI agent and installs skills to the correct directory: When both agents are detected, craft prompts you to choose. Use `--target ` to override auto-detection. -**Global installs** (`craft get`, `craft install -g`) write to agent skill directories. -**Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored). +**Global installs** (`craft get`, `craft install -g`) write to agent skill directories using a flat naming scheme so agents can discover skills. Each skill becomes a direct child of the skills root: + +``` +~/.claude/skills/ +├── github.com--acme--company-standards--coding-style/ +│ └── SKILL.md +└── github.com--acme--company-standards--review-checklist/ + └── SKILL.md +``` + +The flat directory name is derived from the composite key: slashes become `--`, dots and casing are preserved (e.g., `github.com/acme/company-standards/coding-style` → `github.com--acme--company-standards--coding-style`). The encoding is injective — distinct composite keys always produce distinct flat keys. + +**Project installs** (`craft install`) vendor to `forge/` in the project root (gitignored), using nested composite-key paths. ## Known Limitations diff --git a/internal/cli/get.go b/internal/cli/get.go index 4d73aa4..5cc6626 100644 --- a/internal/cli/get.go +++ b/internal/cli/get.go @@ -238,7 +238,7 @@ func runGet(cmd *cobra.Command, args []string) error { // Install to agent directories progress.Update("Installing skills...") for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w\n note: dependencies were added to the global manifest but installation could not complete\n hint: run 'craft install -g' to retry, or 'craft remove -g ' to undo", err) } diff --git a/internal/cli/install.go b/internal/cli/install.go index 595a8c4..4f80404 100644 --- a/internal/cli/install.go +++ b/internal/cli/install.go @@ -121,7 +121,7 @@ func runInstallGlobal(cmd *cobra.Command) error { progress.Update("Installing skills...") for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w", err) } diff --git a/internal/cli/list.go b/internal/cli/list.go index a5843e6..6e780ff 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -6,6 +6,7 @@ import ( "strings" "text/tabwriter" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/resolve" "github.com/spf13/cobra" ) @@ -84,7 +85,11 @@ func runList(cmd *cobra.Command, args []string) error { for _, d := range deps { cmd.Printf("%s %s %s\n", sanitize(d.alias), d.version, sanitize(d.url)) if len(d.skills) > 0 { - cmd.Printf(" skills: %s\n", sanitize(strings.Join(d.skills, ", "))) + displaySkills := d.skills + if globalFlag { + displaySkills = installlib.QualifySkillNames(d.url, d.skills) + } + cmd.Printf(" skills: %s\n", sanitize(strings.Join(displaySkills, ", "))) } else { cmd.Printf(" skills: (none)\n") } diff --git a/internal/cli/remove.go b/internal/cli/remove.go index 2df197a..63ae7e6 100644 --- a/internal/cli/remove.go +++ b/internal/cli/remove.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/manifest" "github.com/erdemtuna/craft/internal/pinfile" "github.com/erdemtuna/craft/internal/resolve" @@ -138,7 +139,12 @@ func runRemove(cmd *cobra.Command, args []string) error { for _, skillName := range orphaned { removedFromAny := false for _, tp := range targetPath { - skillDir := filepath.Join(tp, nsPrefix, skillName) + var skillDir string + if globalFlag { + skillDir = filepath.Join(tp, installlib.FlatKey(installlib.CompositeKey(nsPrefix, skillName))) + } else { + skillDir = filepath.Join(tp, nsPrefix, skillName) + } // Path traversal protection absSkillDir, err := filepath.Abs(skillDir) if err != nil { @@ -158,7 +164,9 @@ func runRemove(cmd *cobra.Command, args []string) error { cmd.PrintErrf(" warning: could not remove %s: %v\n", skillDir, err) } else { removedFromAny = true - cleanEmptyParents(tp, filepath.Dir(skillDir)) + if !globalFlag { + cleanEmptyParents(tp, filepath.Dir(skillDir)) + } } } } diff --git a/internal/cli/remove_test.go b/internal/cli/remove_test.go index 9c1bd66..6bd1d6d 100644 --- a/internal/cli/remove_test.go +++ b/internal/cli/remove_test.go @@ -362,3 +362,70 @@ func TestCleanEmptyParents_StopsAtRoot(t *testing.T) { t.Error("root should not be deleted") } } + +func TestRunRemoveGlobal_FlatCleanup(t *testing.T) { + // Set up isolated HOME for global manifest/pinfile + fakeHome := t.TempDir() + origHome := os.Getenv("HOME") + t.Setenv("HOME", fakeHome) + defer func() { _ = os.Setenv("HOME", origHome) }() + + // Reset globalFlag after test (cobra persistent flags leak between tests) + defer func() { globalFlag = false }() + + craftDir := filepath.Join(fakeHome, ".craft") + _ = os.MkdirAll(craftDir, 0755) + + manifestContent := `schema_version: 1 +name: global-pkg +dependencies: + my-dep: github.com/org/repo@v1.0.0 +` + _ = os.WriteFile(filepath.Join(craftDir, "craft.yaml"), []byte(manifestContent), 0644) + + pinContent := `pin_version: 1 +resolved: + github.com/org/repo@v1.0.0: + commit: abc123 + integrity: sha256-test + skills: + - my-skill +` + _ = os.WriteFile(filepath.Join(craftDir, "craft.pin.yaml"), []byte(pinContent), 0644) + + // Create flat skill directory (as InstallFlat would produce) + targetDir := filepath.Join(fakeHome, "agent-skills") + flatSkillDir := filepath.Join(targetDir, "github.com--org--repo--my-skill") + _ = os.MkdirAll(flatSkillDir, 0755) + _ = os.WriteFile(filepath.Join(flatSkillDir, "SKILL.md"), []byte("skill"), 0644) + + var buf bytes.Buffer + rootCmd.SetOut(&buf) + rootCmd.SetErr(&buf) + rootCmd.SetArgs([]string{"remove", "-g", "--target", targetDir, "my-dep"}) + err := rootCmd.Execute() + + if err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, buf.String()) + } + + output := buf.String() + if !strings.Contains(output, "Removed") { + t.Errorf("expected 'Removed' message, got: %s", output) + } + + // Verify flat skill directory was removed + if _, err := os.Stat(flatSkillDir); err == nil { + t.Error("flat skill directory should have been removed") + } + + // Verify no nested parent directories were created + if _, err := os.Stat(filepath.Join(targetDir, "github.com")); err == nil { + t.Error("no nested parent directories should exist") + } + + // Verify target dir itself still exists + if _, err := os.Stat(targetDir); err != nil { + t.Error("target directory should still exist") + } +} diff --git a/internal/cli/tree.go b/internal/cli/tree.go index 48da460..a987428 100644 --- a/internal/cli/tree.go +++ b/internal/cli/tree.go @@ -3,6 +3,7 @@ package cli import ( "strings" + installlib "github.com/erdemtuna/craft/internal/install" "github.com/erdemtuna/craft/internal/resolve" "github.com/erdemtuna/craft/internal/ui" "github.com/spf13/cobra" @@ -30,7 +31,7 @@ func runTree(cmd *cobra.Command, args []string) error { var localSkills []string for _, s := range m.Skills { parts := strings.Split(strings.TrimRight(s, "/"), "/") - localSkills = append(localSkills, parts[len(parts)-1]) + localSkills = append(localSkills, sanitize(parts[len(parts)-1])) } // Build alias lookup from manifest @@ -57,10 +58,21 @@ func runTree(cmd *cobra.Command, args []string) error { alias = parsed.Repo } + skills := entry.Skills + if globalFlag { + skills = installlib.QualifySkillNames(parsed.PackageIdentity(), entry.Skills) + } + + // Sanitize all user-derived strings before rendering + sanitizedSkills := make([]string, len(skills)) + for i, s := range skills { + sanitizedSkills[i] = sanitize(s) + } + deps = append(deps, ui.DepNode{ - Alias: alias, - URL: key, - Skills: entry.Skills, + Alias: sanitize(alias), + URL: sanitize(key), + Skills: sanitizedSkills, }) } diff --git a/internal/cli/update.go b/internal/cli/update.go index 0f29248..5858de7 100644 --- a/internal/cli/update.go +++ b/internal/cli/update.go @@ -224,7 +224,7 @@ func runUpdate(cmd *cobra.Command, args []string) error { } for _, targetPath := range targetPaths { - if err := installlib.Install(targetPath, skillFiles); err != nil { + if err := installlib.InstallFlat(targetPath, skillFiles); err != nil { progress.Fail("Installation failed") return fmt.Errorf("installation failed: %w", err) } diff --git a/internal/install/installer.go b/internal/install/installer.go index ef89969..1efcff4 100644 --- a/internal/install/installer.go +++ b/internal/install/installer.go @@ -86,3 +86,50 @@ func Install(target string, skills map[string]map[string][]byte) error { return nil } + +// FlatKey converts a composite skill key (host/owner/repo/skill) into a flat +// directory name suitable for agent skill discovery. Slashes become "--". +// Dots and casing are preserved. The encoding is injective — distinct +// composite keys always produce distinct flat keys. +// +// Example: "github.com/org/repo/my-skill" → "github.com--org--repo--my-skill" +func FlatKey(compositeKey string) string { + return strings.ReplaceAll(compositeKey, "/", "--") +} + +// InstallFlat installs skills using flat directory names so that each skill +// is a direct child of the target directory. This is used for global installs +// where AI agents need to discover skills by scanning immediate children. +// It transforms composite keys via FlatKey then delegates to Install. +func InstallFlat(target string, skills map[string]map[string][]byte) error { + flat := make(map[string]map[string][]byte, len(skills)) + for k, v := range skills { + fk := FlatKey(k) + if fk == "" { + return fmt.Errorf("empty composite key produces empty flat key") + } + flat[fk] = v + } + return Install(target, flat) +} + +// CompositeKey builds a composite skill key from a package identity and skill +// name. This is the canonical way to construct the key used by both Install +// and remove operations, ensuring format consistency. +func CompositeKey(packageIdentity, skillName string) string { + return packageIdentity + "/" + skillName +} + +// QualifySkillNames prefixes each skill name with its package identity to form +// composite key display names. Used by list and tree commands for global scope. +// Empty skill names are skipped to avoid trailing-slash display artifacts. +func QualifySkillNames(packageIdentity string, skills []string) []string { + qualified := make([]string, 0, len(skills)) + for _, s := range skills { + if s == "" { + continue + } + qualified = append(qualified, CompositeKey(packageIdentity, s)) + } + return qualified +} diff --git a/internal/install/installer_test.go b/internal/install/installer_test.go index a026db6..b91a0ae 100644 --- a/internal/install/installer_test.go +++ b/internal/install/installer_test.go @@ -208,3 +208,206 @@ func TestInstallCompositeKeys(t *testing.T) { t.Errorf("Unexpected content for other/tools: %q", content2) } } + +func TestFlatKey(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"standard composite key", "github.com/org/repo/my-skill", "github.com--org--repo--my-skill"}, + {"real PAW key", "github.com/lossyrob/phased-agent-workflow/paw-implement", "github.com--lossyrob--phased-agent-workflow--paw-implement"}, + {"anthropic skills", "github.com/anthropics/skills/skill-creator", "github.com--anthropics--skills--skill-creator"}, + {"simple skill name", "simple-skill", "simple-skill"}, + {"custom host with dots", "host.name/owner/repo/skill", "host.name--owner--repo--skill"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FlatKey(tt.input) + if got != tt.want { + t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestFlatKeyEdgeCases(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + {"dots in org", "github.com/my.org/repo/skill", "github.com--my.org--repo--skill"}, + {"dots in repo", "github.com/org/my.repo/skill", "github.com--org--my.repo--skill"}, + {"dots in skill", "github.com/org/repo/my.skill", "github.com--org--repo--my.skill"}, + {"dots everywhere", "git.hub.com/my.org/my.repo/my.skill", "git.hub.com--my.org--my.repo--my.skill"}, + {"mixed casing", "GitHub.com/MyOrg/Repo/Skill", "GitHub.com--MyOrg--Repo--Skill"}, + {"many components", "a/b/c/d/e", "a--b--c--d--e"}, + {"no slashes no dots", "plain-name", "plain-name"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := FlatKey(tt.input) + if got != tt.want { + t.Errorf("FlatKey(%q) = %q, want %q", tt.input, got, tt.want) + } + }) + } +} + +func TestInstallFlatCreatesStructure(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": { + "SKILL.md": []byte("flat skill"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Should be flat, not nested + flatDir := filepath.Join(target, "github.com--org--repo--my-skill") + content, err := os.ReadFile(filepath.Join(flatDir, "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile error: %v", err) + } + if string(content) != "flat skill" { + t.Errorf("Unexpected content: %q", content) + } + + // Nested path should NOT exist + nestedDir := filepath.Join(target, "github.com", "org", "repo", "my-skill") + if _, err := os.Stat(nestedDir); err == nil { + t.Error("nested directory should not exist after InstallFlat") + } +} + +func TestInstallFlatMultiPackage(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/skill-a": { + "SKILL.md": []byte("a"), + }, + "github.com/other/tools/skill-b": { + "SKILL.md": []byte("b"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Both should be direct children of target + entries, err := os.ReadDir(target) + if err != nil { + t.Fatalf("ReadDir error: %v", err) + } + if len(entries) != 2 { + t.Fatalf("expected 2 entries, got %d", len(entries)) + } + + names := make(map[string]bool) + for _, e := range entries { + names[e.Name()] = true + } + if !names["github.com--org--repo--skill-a"] { + t.Error("missing github.com--org--repo--skill-a") + } + if !names["github.com--other--tools--skill-b"] { + t.Error("missing github.com--other--tools--skill-b") + } +} + +func TestInstallFlatSameName(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": { + "SKILL.md": []byte("from org"), + }, + "github.com/other/tools/my-skill": { + "SKILL.md": []byte("from other"), + }, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + // Same leaf name, different flat keys — no collision + c1, err := os.ReadFile(filepath.Join(target, "github.com--org--repo--my-skill", "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile org: %v", err) + } + if string(c1) != "from org" { + t.Errorf("org content: %q", c1) + } + + c2, err := os.ReadFile(filepath.Join(target, "github.com--other--tools--my-skill", "SKILL.md")) + if err != nil { + t.Fatalf("ReadFile other: %v", err) + } + if string(c2) != "from other" { + t.Errorf("other content: %q", c2) + } +} + +func TestInstallFlatOverwrites(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + + skills1 := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": {"SKILL.md": []byte("v1")}, + } + if err := InstallFlat(target, skills1); err != nil { + t.Fatalf("first InstallFlat: %v", err) + } + + skills2 := map[string]map[string][]byte{ + "github.com/org/repo/my-skill": {"SKILL.md": []byte("v2")}, + } + if err := InstallFlat(target, skills2); err != nil { + t.Fatalf("second InstallFlat: %v", err) + } + + content, _ := os.ReadFile(filepath.Join(target, "github.com--org--repo--my-skill", "SKILL.md")) + if string(content) != "v2" { + t.Errorf("expected v2, got %q", content) + } +} + +func TestInstallFlatDistinguishesDotFromDash(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + // Dots and hyphens are distinct — no collision with injective encoding + skills := map[string]map[string][]byte{ + "github.com/org/my.repo/skill": {"SKILL.md": []byte("dot")}, + "github.com/org/my-repo/skill": {"SKILL.md": []byte("dash")}, + } + + if err := InstallFlat(target, skills); err != nil { + t.Fatalf("InstallFlat error: %v", err) + } + + c1, _ := os.ReadFile(filepath.Join(target, "github.com--org--my.repo--skill", "SKILL.md")) + if string(c1) != "dot" { + t.Errorf("dot repo content: %q", c1) + } + c2, _ := os.ReadFile(filepath.Join(target, "github.com--org--my-repo--skill", "SKILL.md")) + if string(c2) != "dash" { + t.Errorf("dash repo content: %q", c2) + } +} + +func TestInstallFlatRejectsEmptyKey(t *testing.T) { + target := filepath.Join(t.TempDir(), "skills") + skills := map[string]map[string][]byte{ + "": {"SKILL.md": []byte("bad")}, + } + err := InstallFlat(target, skills) + if err == nil { + t.Fatal("expected error for empty composite key") + } + if !strings.Contains(err.Error(), "empty") { + t.Errorf("expected 'empty' in error, got: %v", err) + } +}