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) <noreply@anthropic.com>

Author: wesm

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

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