From d61b91879030fd39637b38b7c79b78f2ff6b76b5 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 19 Mar 2026 22:27:12 -0400 Subject: [PATCH 1/4] Refactor skills installer around shared multi-agent catalog - Replace duplicated Claude/Codex install and status logic with shared agent specs - Build skill metadata and installed-path checks from embedded skills dynamically - Update tests to use agent table cases and runtime-derived expected skills --- internal/skills/skills.go | 392 +++++++++++++++++---------------- internal/skills/skills_test.go | 147 +++++++------ 2 files changed, 277 insertions(+), 262 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 57c28b39a..c3935b560 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -28,6 +28,25 @@ const ( AgentCodex Agent = "codex" ) +type agentSpec struct { + agent Agent + configDirName string + embedFS fs.FS + embedDir string +} + +type embeddedSkill struct { + DirName string + Name string + Description string + Content []byte +} + +var supportedAgents = []agentSpec{ + {agent: AgentClaude, configDirName: ".claude", embedFS: claudeSkills, embedDir: "claude"}, + {agent: AgentCodex, configDirName: ".codex", embedFS: codexSkills, embedDir: "codex"}, +} + // InstallResult contains the result of a skill installation type InstallResult struct { Agent Agent @@ -39,22 +58,14 @@ type InstallResult struct { // Install installs skills for all supported agents whose config directories exist. // It is idempotent - running multiple times will update existing skills. func Install() ([]InstallResult, error) { - var results []InstallResult - - // Install Claude skills - result, err := installClaude() - if err != nil { - return nil, fmt.Errorf("claude skills: %w", err) - } - results = append(results, result) - - // Install Codex skills - result, err = installCodex() - if err != nil { - return nil, fmt.Errorf("codex skills: %w", err) + results := make([]InstallResult, 0, len(supportedAgents)) + for _, spec := range supportedAgents { + result, err := installAgent(spec) + if err != nil { + return nil, fmt.Errorf("%s skills: %w", spec.agent, err) + } + results = append(results, result) } - results = append(results, result) - return results, nil } @@ -65,16 +76,13 @@ func IsInstalled(agent Agent) bool { return false } - var checkFiles []string + spec, ok := lookupAgent(agent) + if !ok { + return false + } - switch agent { - case AgentClaude: - skillsDir := filepath.Join(home, ".claude", "skills") - checkFiles = append(currentSkillChecks(skillsDir), legacySkillChecks(skillsDir)...) - case AgentCodex: - skillsDir := filepath.Join(home, ".codex", "skills") - checkFiles = append(currentSkillChecks(skillsDir), legacySkillChecks(skillsDir)...) - default: + checkFiles, err := installedSkillFilePaths(home, spec) + if err != nil { return false } @@ -93,47 +101,111 @@ var legacySkills = []string{ "roborev-address", } -func currentSkillChecks(skillsDir string) []string { - names := []string{ - "roborev-respond", - "roborev-fix", - "roborev-design-review", - "roborev-design-review-branch", - "roborev-review", - "roborev-review-branch", +func lookupAgent(agent Agent) (agentSpec, bool) { + for _, spec := range supportedAgents { + if spec.agent == agent { + return spec, true + } } - out := make([]string, len(names)) - for i, n := range names { - out[i] = filepath.Join(skillsDir, n, "SKILL.md") + return agentSpec{}, false +} + +func agentConfigDir(home string, spec agentSpec) string { + return filepath.Join(home, spec.configDirName) +} + +func agentSkillsDir(home string, spec agentSpec) string { + return filepath.Join(agentConfigDir(home, spec), "skills") +} + +func skillInstallPath(skillsDir, skillName string) string { + return filepath.Join(skillsDir, skillName, "SKILL.md") +} + +func embeddedSkillsForAgent(spec agentSpec) ([]embeddedSkill, error) { + entries, err := fs.ReadDir(spec.embedFS, spec.embedDir) + if err != nil { + return nil, fmt.Errorf("read embedded skills: %w", err) } - return out + + skills := make([]embeddedSkill, 0, len(entries)) + for _, entry := range entries { + if !entry.IsDir() { + continue + } + + dirName := entry.Name() + content, err := fs.ReadFile(spec.embedFS, path.Join(spec.embedDir, dirName, "SKILL.md")) + if err != nil { + return nil, fmt.Errorf("read %s/SKILL.md: %w", dirName, err) + } + + name, desc := parseFrontmatter(content) + if name == "" { + name = dirName + } + skills = append(skills, embeddedSkill{ + DirName: dirName, + Name: name, + Description: desc, + Content: content, + }) + } + return skills, nil +} + +func currentInstalledSkillFilePaths(home string, spec agentSpec) ([]string, error) { + skills, err := embeddedSkillsForAgent(spec) + if err != nil { + return nil, err + } + + skillsDir := agentSkillsDir(home, spec) + paths := make([]string, 0, len(skills)) + for _, skill := range skills { + paths = append(paths, skillInstallPath(skillsDir, skill.DirName)) + } + return paths, nil } -func legacySkillChecks(skillsDir string) []string { +func legacyInstalledSkillFilePaths(skillsDir string) []string { out := make([]string, len(legacySkills)) for i, n := range legacySkills { - out[i] = filepath.Join(skillsDir, n, "SKILL.md") + out[i] = skillInstallPath(skillsDir, n) } return out } +func installedSkillFilePaths(home string, spec agentSpec) ([]string, error) { + skillsDir := agentSkillsDir(home, spec) + current, err := currentInstalledSkillFilePaths(home, spec) + if err != nil { + return nil, err + } + return append(current, legacyInstalledSkillFilePaths(skillsDir)...), nil +} + // Update updates skills for agents that already have them installed // and removes legacy skills that are no longer shipped. func Update() ([]InstallResult, error) { - var results []InstallResult + home, err := os.UserHomeDir() + if err != nil { + return nil, fmt.Errorf("get home dir: %w", err) + } - if IsInstalled(AgentClaude) { - result, err := installClaude() + var results []InstallResult + for _, spec := range supportedAgents { + installed, err := installedSkillFilePaths(home, spec) if err != nil { - return nil, fmt.Errorf("update claude skills: %w", err) + return nil, fmt.Errorf("update %s skills: %w", spec.agent, err) + } + if !anyFileExists(installed) { + continue } - results = append(results, result) - } - if IsInstalled(AgentCodex) { - result, err := installCodex() + result, err := installAgent(spec) if err != nil { - return nil, fmt.Errorf("update codex skills: %w", err) + return nil, fmt.Errorf("update %s skills: %w", spec.agent, err) } results = append(results, result) } @@ -143,22 +215,13 @@ func Update() ([]InstallResult, error) { // removeLegacySkills deletes skill directories that are no longer // embedded in the binary. -func removeLegacySkills(agent Agent) error { +func removeLegacySkills(spec agentSpec) error { home, err := os.UserHomeDir() if err != nil { return fmt.Errorf("get home dir: %w", err) } - var skillsDir string - switch agent { - case AgentClaude: - skillsDir = filepath.Join(home, ".claude", "skills") - case AgentCodex: - skillsDir = filepath.Join(home, ".codex", "skills") - default: - return nil - } - + skillsDir := agentSkillsDir(home, spec) for _, name := range legacySkills { dir := filepath.Join(skillsDir, name) if err := os.RemoveAll(dir); err != nil { @@ -168,122 +231,42 @@ func removeLegacySkills(agent Agent) error { return nil } -func installClaude() (InstallResult, error) { - result := InstallResult{Agent: AgentClaude} +func installAgent(spec agentSpec) (InstallResult, error) { + result := InstallResult{Agent: spec.agent} home, err := os.UserHomeDir() if err != nil { return result, fmt.Errorf("get home dir: %w", err) } - // Check if ~/.claude exists - claudeDir := filepath.Join(home, ".claude") - if _, err := os.Stat(claudeDir); os.IsNotExist(err) { + configDir := agentConfigDir(home, spec) + if _, err := os.Stat(configDir); os.IsNotExist(err) { result.Skipped = true return result, nil } - // Create skills directory - skillsDir := filepath.Join(claudeDir, "skills") + skillsDir := agentSkillsDir(home, spec) if err := os.MkdirAll(skillsDir, 0755); err != nil { return result, fmt.Errorf("create skills dir: %w", err) } - // Install each skill directory - entries, err := fs.ReadDir(claudeSkills, "claude") + skills, err := embeddedSkillsForAgent(spec) if err != nil { - return result, fmt.Errorf("read embedded skills: %w", err) - } - - for _, entry := range entries { - if !entry.IsDir() { - continue - } - - skillName := entry.Name() - skillDir := filepath.Join(skillsDir, skillName) - - // Create skill directory - if err := os.MkdirAll(skillDir, 0755); err != nil { - return result, fmt.Errorf("create %s dir: %w", skillName, err) - } - - // Read SKILL.md (use path.Join for embed FS - requires forward slashes) - content, err := claudeSkills.ReadFile(path.Join("claude", skillName, "SKILL.md")) - if err != nil { - return result, fmt.Errorf("read %s/SKILL.md: %w", skillName, err) - } - - destPath := filepath.Join(skillDir, "SKILL.md") - existed := fileExists(destPath) - - if err := os.WriteFile(destPath, content, 0644); err != nil { - return result, fmt.Errorf("write %s/SKILL.md: %w", skillName, err) - } - - if existed { - result.Updated = append(result.Updated, skillName) - } else { - result.Installed = append(result.Installed, skillName) - } - } - - if err := removeLegacySkills(AgentClaude); err != nil { return result, err } - return result, nil -} - -func installCodex() (InstallResult, error) { - result := InstallResult{Agent: AgentCodex} - - home, err := os.UserHomeDir() - if err != nil { - return result, fmt.Errorf("get home dir: %w", err) - } - - // Check if ~/.codex exists - codexDir := filepath.Join(home, ".codex") - if _, err := os.Stat(codexDir); os.IsNotExist(err) { - result.Skipped = true - return result, nil - } - - // Create skills directory - skillsDir := filepath.Join(codexDir, "skills") - if err := os.MkdirAll(skillsDir, 0755); err != nil { - return result, fmt.Errorf("create skills dir: %w", err) - } - - // Install each skill directory - entries, err := fs.ReadDir(codexSkills, "codex") - if err != nil { - return result, fmt.Errorf("read embedded skills: %w", err) - } - - for _, entry := range entries { - if !entry.IsDir() { - continue - } - skillName := entry.Name() + for _, skill := range skills { + skillName := skill.DirName skillDir := filepath.Join(skillsDir, skillName) - // Create skill directory if err := os.MkdirAll(skillDir, 0755); err != nil { return result, fmt.Errorf("create %s dir: %w", skillName, err) } - // Read SKILL.md (use path.Join for embed FS - requires forward slashes) - content, err := codexSkills.ReadFile(path.Join("codex", skillName, "SKILL.md")) - if err != nil { - return result, fmt.Errorf("read %s/SKILL.md: %w", skillName, err) - } - destPath := filepath.Join(skillDir, "SKILL.md") existed := fileExists(destPath) - if err := os.WriteFile(destPath, content, 0644); err != nil { + if err := os.WriteFile(destPath, skill.Content, 0644); err != nil { return result, fmt.Errorf("write %s/SKILL.md: %w", skillName, err) } @@ -294,7 +277,7 @@ func installCodex() (InstallResult, error) { } } - if err := removeLegacySkills(AgentCodex); err != nil { + if err := removeLegacySkills(spec); err != nil { return result, err } return result, nil @@ -323,27 +306,58 @@ type AgentStatus struct { Skills map[string]SkillState // keyed by dir name (e.g. "roborev-fix") } -// ListSkills returns metadata for all embedded skills (from the Claude skill set). +func embeddedSkillCatalog() (map[string]SkillInfo, error) { + catalog := make(map[string]SkillInfo) + for _, spec := range supportedAgents { + skills, err := embeddedSkillsForAgent(spec) + if err != nil { + return nil, err + } + for _, skill := range skills { + info := SkillInfo{ + DirName: skill.DirName, + Name: skill.Name, + Description: skill.Description, + } + + existing, ok := catalog[skill.DirName] + if !ok { + catalog[skill.DirName] = info + continue + } + if existing.Name == "" { + existing.Name = info.Name + } + if existing.Description == "" { + existing.Description = info.Description + } + catalog[skill.DirName] = existing + } + } + return catalog, nil +} + +// ListSkills returns metadata for all embedded skills. func ListSkills() ([]SkillInfo, error) { - entries, err := fs.ReadDir(claudeSkills, "claude") + catalog, err := embeddedSkillCatalog() if err != nil { - return nil, fmt.Errorf("read embedded skills: %w", err) + return nil, err } - var out []SkillInfo - for _, entry := range entries { - if !entry.IsDir() { - continue - } - content, err := claudeSkills.ReadFile(path.Join("claude", entry.Name(), "SKILL.md")) + out := make([]SkillInfo, 0, len(catalog)) + for _, spec := range supportedAgents { + skills, err := embeddedSkillsForAgent(spec) if err != nil { - return nil, fmt.Errorf("read %s/SKILL.md: %w", entry.Name(), err) + return nil, err } - name, desc := parseFrontmatter(content) - if name == "" { - name = entry.Name() + for _, skill := range skills { + info, ok := catalog[skill.DirName] + if !ok { + continue + } + out = append(out, info) + delete(catalog, skill.DirName) } - out = append(out, SkillInfo{DirName: entry.Name(), Name: name, Description: desc}) } return out, nil } @@ -355,61 +369,40 @@ func Status() []AgentStatus { return nil } - type agentDef struct { - agent Agent - configDir string - embedFS embed.FS - embedDir string - } - - agents := []agentDef{ - {AgentClaude, filepath.Join(home, ".claude"), claudeSkills, "claude"}, - {AgentCodex, filepath.Join(home, ".codex"), codexSkills, "codex"}, - } - var out []AgentStatus - for _, a := range agents { + for _, spec := range supportedAgents { status := AgentStatus{ - Agent: a.agent, + Agent: spec.agent, Skills: make(map[string]SkillState), } - if _, err := os.Stat(a.configDir); err != nil { + configDir := agentConfigDir(home, spec) + if _, err := os.Stat(configDir); err != nil { out = append(out, status) continue } status.Available = true - entries, err := fs.ReadDir(a.embedFS, a.embedDir) + skills, err := embeddedSkillsForAgent(spec) if err != nil { out = append(out, status) continue } - skillsDir := filepath.Join(a.configDir, "skills") - for _, entry := range entries { - if !entry.IsDir() { - continue - } - name := entry.Name() - installedPath := filepath.Join(skillsDir, name, "SKILL.md") + skillsDir := agentSkillsDir(home, spec) + for _, skill := range skills { + installedPath := skillInstallPath(skillsDir, skill.DirName) installedContent, err := os.ReadFile(installedPath) if err != nil { - status.Skills[name] = SkillMissing + status.Skills[skill.DirName] = SkillMissing continue } - embeddedContent, err := a.embedFS.ReadFile(path.Join(a.embedDir, name, "SKILL.md")) - if err != nil { - status.Skills[name] = SkillMissing - continue - } - - if bytes.Equal(installedContent, embeddedContent) { - status.Skills[name] = SkillCurrent + if bytes.Equal(installedContent, skill.Content) { + status.Skills[skill.DirName] = SkillCurrent } else { - status.Skills[name] = SkillOutdated + status.Skills[skill.DirName] = SkillOutdated } } @@ -443,3 +436,12 @@ func fileExists(path string) bool { _, err := os.Stat(path) return err == nil } + +func anyFileExists(paths []string) bool { + for _, p := range paths { + if fileExists(p) { + return true + } + } + return false +} diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 1d13ac501..d8d443ace 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -9,13 +9,16 @@ import ( "github.com/stretchr/testify/require" ) -var expectedSkills = []string{ - "roborev-design-review", - "roborev-design-review-branch", - "roborev-fix", - "roborev-respond", - "roborev-review", - "roborev-review-branch", +type agentCase struct { + agent Agent + configDir string + legacyDir string + displayName string +} + +var agentCases = []agentCase{ + {agent: AgentClaude, configDir: ".claude", legacyDir: ".claude", displayName: string(AgentClaude)}, + {agent: AgentCodex, configDir: ".codex", legacyDir: ".codex", displayName: string(AgentCodex)}, } func setupTestEnv(t *testing.T) string { @@ -37,7 +40,19 @@ func createMockSkill(t *testing.T, homeDir string, agent Agent, skill string) { require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("old"), 0644)) } -func getResultForAgent(t *testing.T, results []InstallResult, agent Agent) *InstallResult { +func expectedSkillDirNames(t *testing.T) []string { + t.Helper() + skills, err := ListSkills() + require.NoError(t, err) + + names := make([]string, 0, len(skills)) + for _, skill := range skills { + names = append(names, skill.DirName) + } + return names +} + +func findResultByAgent(t *testing.T, results []InstallResult, agent Agent) *InstallResult { t.Helper() for i := range results { if results[i].Agent == agent { @@ -48,10 +63,25 @@ func getResultForAgent(t *testing.T, results []InstallResult, agent Agent) *Inst return nil } -func assertSkillsInstalled(t *testing.T, agentDir string) { +func requireResultCount(t *testing.T, results []InstallResult, want int) { t.Helper() - skillsDir := filepath.Join(agentDir, "skills") - for _, skill := range expectedSkills { + + require.Len(t, results, want, "unexpected install result count") +} + +func resultMap(results []InstallResult) map[Agent]InstallResult { + out := make(map[Agent]InstallResult, len(results)) + for _, result := range results { + out[result.Agent] = result + } + return out +} + +func assertSkillsInstalled(t *testing.T, homeDir string, tc agentCase) { + t.Helper() + + skillsDir := filepath.Join(homeDir, tc.configDir, "skills") + for _, skill := range expectedSkillDirNames(t) { path := filepath.Join(skillsDir, skill, "SKILL.md") _, err := os.Stat(path) require.NoError(t, err, "expected %s to exist", path) @@ -64,33 +94,27 @@ func TestInstallClaudeSkipsWhenDirMissing(t *testing.T) { results, err := Install() require.NoError(t, err, "Install failed") - claudeResult := getResultForAgent(t, results, AgentClaude) + claudeResult := findResultByAgent(t, results, AgentClaude) assert.True(t, claudeResult.Skipped, "expected Claude to be skipped when ~/.claude doesn't exist") assert.Empty(t, claudeResult.Installed, "expected no installed skills") } func TestInstallWhenDirExists(t *testing.T) { - tests := []struct { - agent Agent - dirName string - }{ - {AgentClaude, ".claude"}, - {AgentCodex, ".codex"}, - } + expectedSkills := expectedSkillDirNames(t) - for _, tt := range tests { - t.Run(string(tt.agent), func(t *testing.T) { + for _, tc := range agentCases { + t.Run(tc.displayName, func(t *testing.T) { tmpHome := setupTestEnv(t) - agentDir := filepath.Join(tmpHome, tt.dirName) + agentDir := filepath.Join(tmpHome, tc.configDir) require.NoError(t, os.MkdirAll(agentDir, 0755)) results, err := Install() require.NoError(t, err, "Install failed") - res := getResultForAgent(t, results, tt.agent) + res := findResultByAgent(t, results, tc.agent) assert.False(t, res.Skipped, "expected not to be skipped") assert.Len(t, res.Installed, len(expectedSkills)) - assertSkillsInstalled(t, agentDir) + assertSkillsInstalled(t, tmpHome, tc) }) } } @@ -104,14 +128,16 @@ func TestInstallIdempotent(t *testing.T) { results1, err := Install() require.NoError(t, err, "First install failed: %v", err) - claude1 := getResultForAgent(t, results1, AgentClaude) + expectedSkills := expectedSkillDirNames(t) + + claude1 := findResultByAgent(t, results1, AgentClaude) require.Len(t, claude1.Installed, len(expectedSkills), "first install: expected %d installed, got %d", len(expectedSkills), len(claude1.Installed)) require.Empty(t, claude1.Updated, "first install: expected 0 updated, got %d", len(claude1.Updated)) results2, err := Install() require.NoError(t, err, "Second install failed: %v", err) - claude2 := getResultForAgent(t, results2, AgentClaude) + claude2 := findResultByAgent(t, results2, AgentClaude) require.Empty(t, claude2.Installed, "second install: expected 0 installed, got %d", len(claude2.Installed)) require.Len(t, claude2.Updated, len(expectedSkills), "second install: expected %d updated, got %d", len(expectedSkills), len(claude2.Updated)) } @@ -157,6 +183,7 @@ func TestIsInstalled(t *testing.T) { }, } + expectedSkills := expectedSkillDirNames(t) for _, skill := range expectedSkills { s := skill @@ -195,29 +222,21 @@ func TestIsInstalled(t *testing.T) { } func TestInstallRemovesLegacySkills(t *testing.T) { - tests := []struct { - agent Agent - dirName string - }{ - {AgentClaude, ".claude"}, - {AgentCodex, ".codex"}, - } - - for _, tt := range tests { - t.Run(string(tt.agent), func(t *testing.T) { + for _, tc := range agentCases { + t.Run(tc.displayName, func(t *testing.T) { tmpHome := setupTestEnv(t) - require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, tt.dirName), 0755)) - createMockSkill(t, tmpHome, tt.agent, "roborev-address") + require.NoError(t, os.MkdirAll(filepath.Join(tmpHome, tc.configDir), 0755)) + createMockSkill(t, tmpHome, tc.agent, "roborev-address") _, err := Install() require.NoError(t, err) - legacyDir := filepath.Join(tmpHome, tt.dirName, "skills", "roborev-address") + legacyDir := filepath.Join(tmpHome, tc.legacyDir, "skills", "roborev-address") _, err = os.Stat(legacyDir) assert.True(t, os.IsNotExist(err), "expected legacy dir to be removed after install") - assertSkillsInstalled(t, filepath.Join(tmpHome, tt.dirName)) + assertSkillsInstalled(t, tmpHome, tc) }) } } @@ -242,30 +261,24 @@ func TestUpdateRemovesLegacySkills(t *testing.T) { } func TestUpdateLegacyOnlyInstall(t *testing.T) { - tests := []struct { - agent Agent - dirName string - }{ - {AgentClaude, ".claude"}, - {AgentCodex, ".codex"}, - } + expectedSkills := expectedSkillDirNames(t) - for _, tt := range tests { - t.Run(string(tt.agent), func(t *testing.T) { + for _, tc := range agentCases { + t.Run(tc.displayName, func(t *testing.T) { tmpHome := setupTestEnv(t) // User only has the deprecated skill — no current skills - createMockSkill(t, tmpHome, tt.agent, "roborev-address") + createMockSkill(t, tmpHome, tc.agent, "roborev-address") results, err := Update() require.NoError(t, err) require.Len(t, results, 1) - res := getResultForAgent(t, results, tt.agent) + res := findResultByAgent(t, results, tc.agent) assert.Len(t, res.Installed, len(expectedSkills)) // Legacy dir should be removed - legacyDir := filepath.Join(tmpHome, tt.dirName, "skills", "roborev-address") + legacyDir := filepath.Join(tmpHome, tc.legacyDir, "skills", "roborev-address") _, err = os.Stat(legacyDir) assert.True(t, os.IsNotExist(err), "expected legacy dir to be removed") }) @@ -273,6 +286,8 @@ func TestUpdateLegacyOnlyInstall(t *testing.T) { } func TestUpdateOnlyUpdatesInstalled(t *testing.T) { + expectedSkillCount := len(expectedSkillDirNames(t)) + tests := []struct { name string setup func(t *testing.T, homeDir string) @@ -292,7 +307,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { wantResults: 1, wantAgents: []Agent{AgentClaude}, wantUpdated: 1, - wantInstalled: len(expectedSkills) - 1, + wantInstalled: expectedSkillCount - 1, }, { name: "updates Claude with respond skill only", @@ -302,7 +317,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { wantResults: 1, wantAgents: []Agent{AgentClaude}, wantUpdated: 1, - wantInstalled: len(expectedSkills) - 1, + wantInstalled: expectedSkillCount - 1, }, { name: "updates Codex with fix skill only", @@ -312,7 +327,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { wantResults: 1, wantAgents: []Agent{AgentCodex}, wantUpdated: 1, - wantInstalled: len(expectedSkills) - 1, + wantInstalled: expectedSkillCount - 1, }, { name: "updates Codex with respond skill only", @@ -322,7 +337,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { wantResults: 1, wantAgents: []Agent{AgentCodex}, wantUpdated: 1, - wantInstalled: len(expectedSkills) - 1, + wantInstalled: expectedSkillCount - 1, }, { name: "updates both agents when both have skills", @@ -333,7 +348,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { wantResults: 2, wantAgents: []Agent{AgentClaude, AgentCodex}, wantUpdated: 1, - wantInstalled: len(expectedSkills) - 1, + wantInstalled: expectedSkillCount - 1, }, { name: "skips both when neither has skills", @@ -357,22 +372,20 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { results, err := Update() require.NoError(t, err, "Update failed: %v", err) - require.NoError(t, err, "expected %d results, got %d", tt.wantResults, len(results)) + requireResultCount(t, results, tt.wantResults) if tt.wantResults > 0 { - - agentFound := make(map[Agent]bool) + resultsByAgent := resultMap(results) for _, want := range tt.wantAgents { - agentFound[want] = false - } - for _, r := range results { - agentFound[r.Agent] = true + r, ok := resultsByAgent[want] + require.True(t, ok, "expected %s in results", want) require.Len(t, r.Updated, tt.wantUpdated, "expected %d updated for %s, got %d", tt.wantUpdated, r.Agent, len(r.Updated)) require.Len(t, r.Installed, tt.wantInstalled, "expected %d installed for %s, got %d", tt.wantInstalled, r.Agent, len(r.Installed)) } - for want, found := range agentFound { - require.True(t, found, "expected %s in results", want) - } + } + + if tt.wantResults == 0 { + assert.Empty(t, results) } }) } From f420f48d5e60ab00a7084fedecf69613afe84932 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 23 Mar 2026 17:54:52 -0500 Subject: [PATCH 2/4] Decouple installation detection from embedded file reads IsInstalled and Update only need directory names to check filesystem paths, not full SKILL.md content. Add embeddedSkillDirNames() that enumerates directories without reading files, so a missing or unreadable embedded file no longer breaks installation detection. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/skills/skills.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index c3935b560..fd820625e 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -154,16 +154,34 @@ func embeddedSkillsForAgent(spec agentSpec) ([]embeddedSkill, error) { return skills, nil } +// embeddedSkillDirNames returns just the directory names of embedded skills +// without reading file contents. Use this for path-only operations like +// IsInstalled and Update where content is not needed. +func embeddedSkillDirNames(spec agentSpec) ([]string, error) { + entries, err := fs.ReadDir(spec.embedFS, spec.embedDir) + if err != nil { + return nil, fmt.Errorf("read embedded skills: %w", err) + } + + var names []string + for _, entry := range entries { + if entry.IsDir() { + names = append(names, entry.Name()) + } + } + return names, nil +} + func currentInstalledSkillFilePaths(home string, spec agentSpec) ([]string, error) { - skills, err := embeddedSkillsForAgent(spec) + dirNames, err := embeddedSkillDirNames(spec) if err != nil { return nil, err } skillsDir := agentSkillsDir(home, spec) - paths := make([]string, 0, len(skills)) - for _, skill := range skills { - paths = append(paths, skillInstallPath(skillsDir, skill.DirName)) + paths := make([]string, 0, len(dirNames)) + for _, name := range dirNames { + paths = append(paths, skillInstallPath(skillsDir, name)) } return paths, nil } From 653146830f1049b582b840ec71998b8cb13dbd81 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 23 Mar 2026 18:36:50 -0500 Subject: [PATCH 3/4] Simplify ListSkills dedup and add regression tests Replace embeddedSkillCatalog's field-by-field merge with a simpler seen-set dedup in ListSkills, making it explicit that the first agent's metadata wins when frontmatter differs across agents. Add tests covering: deduplication across agents, first-agent-wins metadata selection, and a mock-FS regression test verifying that installation detection (embeddedSkillDirNames/currentInstalledSkillFilePaths) succeeds even when embedded SKILL.md files are absent. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/skills/skills.go | 54 ++++++----------------- internal/skills/skills_test.go | 78 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 42 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index fd820625e..607a66167 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -324,57 +324,27 @@ type AgentStatus struct { Skills map[string]SkillState // keyed by dir name (e.g. "roborev-fix") } -func embeddedSkillCatalog() (map[string]SkillInfo, error) { - catalog := make(map[string]SkillInfo) - for _, spec := range supportedAgents { - skills, err := embeddedSkillsForAgent(spec) - if err != nil { - return nil, err - } - for _, skill := range skills { - info := SkillInfo{ - DirName: skill.DirName, - Name: skill.Name, - Description: skill.Description, - } - - existing, ok := catalog[skill.DirName] - if !ok { - catalog[skill.DirName] = info - continue - } - if existing.Name == "" { - existing.Name = info.Name - } - if existing.Description == "" { - existing.Description = info.Description - } - catalog[skill.DirName] = existing - } - } - return catalog, nil -} - -// ListSkills returns metadata for all embedded skills. +// ListSkills returns metadata for all embedded skills, deduplicated by +// directory name. When the same skill exists across multiple agents, the +// first agent's metadata is used. func ListSkills() ([]SkillInfo, error) { - catalog, err := embeddedSkillCatalog() - if err != nil { - return nil, err - } - - out := make([]SkillInfo, 0, len(catalog)) + seen := make(map[string]bool) + var out []SkillInfo for _, spec := range supportedAgents { skills, err := embeddedSkillsForAgent(spec) if err != nil { return nil, err } for _, skill := range skills { - info, ok := catalog[skill.DirName] - if !ok { + if seen[skill.DirName] { continue } - out = append(out, info) - delete(catalog, skill.DirName) + seen[skill.DirName] = true + out = append(out, SkillInfo{ + DirName: skill.DirName, + Name: skill.Name, + Description: skill.Description, + }) } } return out, nil diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index d8d443ace..68c7e2da7 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -3,7 +3,9 @@ package skills import ( "os" "path/filepath" + "strings" "testing" + "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -390,3 +392,79 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { }) } } + +func TestListSkillsDeduplicatesAcrossAgents(t *testing.T) { + skills, err := ListSkills() + require.NoError(t, err) + + seen := make(map[string]bool) + for _, skill := range skills { + assert.False(t, seen[skill.DirName], "duplicate skill in ListSkills output: %s", skill.DirName) + seen[skill.DirName] = true + } +} + +func TestListSkillsUsesFirstAgentMetadata(t *testing.T) { + // When frontmatter differs across agents for the same skill, + // ListSkills should return the first agent's (Claude's) metadata. + skills, err := ListSkills() + require.NoError(t, err) + + claudeSkillsByDir := make(map[string]embeddedSkill) + claudeSpec := supportedAgents[0] + require.Equal(t, AgentClaude, claudeSpec.agent, "first agent must be Claude for this test") + + embedded, err := embeddedSkillsForAgent(claudeSpec) + require.NoError(t, err) + for _, s := range embedded { + claudeSkillsByDir[s.DirName] = s + } + + for _, skill := range skills { + cs, ok := claudeSkillsByDir[skill.DirName] + if !ok { + continue + } + assert.Equal(t, cs.Name, skill.Name, + "skill %s: name should match first agent (Claude)", skill.DirName) + assert.Equal(t, cs.Description, skill.Description, + "skill %s: description should match first agent (Claude)", skill.DirName) + } +} + +func TestDirNameEnumerationDoesNotReadContent(t *testing.T) { + // embeddedSkillDirNames only enumerates directories, so it must + // succeed even when SKILL.md files are absent. This guards against + // regressions that would make IsInstalled/Update depend on file reads. + mockFS := fstest.MapFS{ + "agent/skill-a/.keep": &fstest.MapFile{Data: []byte("")}, + "agent/skill-b/.keep": &fstest.MapFile{Data: []byte("")}, + } + spec := agentSpec{ + agent: "mock", + configDirName: ".mock", + embedFS: mockFS, + embedDir: "agent", + } + + // embeddedSkillDirNames should succeed (only reads directory entries) + names, err := embeddedSkillDirNames(spec) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"skill-a", "skill-b"}, names) + + // embeddedSkillsForAgent should fail (reads SKILL.md content) + _, err = embeddedSkillsForAgent(spec) + require.Error(t, err, "embeddedSkillsForAgent should fail when SKILL.md is missing") + + // currentInstalledSkillFilePaths should succeed via embeddedSkillDirNames + home := t.TempDir() + paths, err := currentInstalledSkillFilePaths(home, spec) + require.NoError(t, err) + require.Len(t, paths, 2) + for _, p := range paths { + assert.True(t, strings.Contains(p, filepath.Join(home, ".mock", "skills")), + "path should be under agent skills dir: %s", p) + assert.True(t, strings.HasSuffix(p, "SKILL.md"), + "path should end with SKILL.md: %s", p) + } +} From 932e21daf80474f4bdbdb300917ac3c2a34c5a83 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 23 Mar 2026 18:56:49 -0500 Subject: [PATCH 4/4] Fix lint: use slices.ContainsFunc and assert.Contains Address modernize and testifylint findings from CI. Co-Authored-By: Claude Opus 4.6 (1M context) --- internal/skills/skills.go | 8 ++------ internal/skills/skills_test.go | 5 ++--- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 607a66167..e1a91a12b 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -11,6 +11,7 @@ import ( "os" "path" "path/filepath" + "slices" "strings" ) @@ -426,10 +427,5 @@ func fileExists(path string) bool { } func anyFileExists(paths []string) bool { - for _, p := range paths { - if fileExists(p) { - return true - } - } - return false + return slices.ContainsFunc(paths, fileExists) } diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 68c7e2da7..da3f158f7 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -3,7 +3,6 @@ package skills import ( "os" "path/filepath" - "strings" "testing" "testing/fstest" @@ -462,9 +461,9 @@ func TestDirNameEnumerationDoesNotReadContent(t *testing.T) { require.NoError(t, err) require.Len(t, paths, 2) for _, p := range paths { - assert.True(t, strings.Contains(p, filepath.Join(home, ".mock", "skills")), + assert.Contains(t, p, filepath.Join(home, ".mock", "skills"), "path should be under agent skills dir: %s", p) - assert.True(t, strings.HasSuffix(p, "SKILL.md"), + assert.Contains(t, p, "SKILL.md", "path should end with SKILL.md: %s", p) } }