From 6fe3920a4d836b97c838fd39874cc6a5d07d1d40 Mon Sep 17 00:00:00 2001 From: mattn Date: Tue, 24 Feb 2026 08:07:09 +0900 Subject: [PATCH] perf: refactoring collecting skills (#688) * perf: refactoring collecting skills * Fix order to store dir.Name() * Add tests --- pkg/skills/loader.go | 140 +++++++++++--------------------------- pkg/skills/loader_test.go | 131 +++++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 101 deletions(-) diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go index eb0d5f322..f4f55a698 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -71,112 +71,50 @@ func NewSkillsLoader(workspace string, globalSkills string, builtinSkills string func (sl *SkillsLoader) ListSkills() []SkillInfo { skills := make([]SkillInfo, 0) + seen := make(map[string]bool) - if sl.workspaceSkills != "" { - if dirs, err := os.ReadDir(sl.workspaceSkills); err == nil { - for _, dir := range dirs { - if dir.IsDir() { - skillFile := filepath.Join(sl.workspaceSkills, dir.Name(), "SKILL.md") - if _, err := os.Stat(skillFile); err == nil { - info := SkillInfo{ - Name: dir.Name(), - Path: skillFile, - Source: "workspace", - } - metadata := sl.getSkillMetadata(skillFile) - if metadata != nil { - info.Description = metadata.Description - info.Name = metadata.Name - } - if err := info.validate(); err != nil { - slog.Warn("invalid skill from workspace", "name", info.Name, "error", err) - continue - } - skills = append(skills, info) - } - } + addSkills := func(dir, source string) { + if dir == "" { + return + } + dirs, err := os.ReadDir(dir) + if err != nil { + return + } + for _, d := range dirs { + if !d.IsDir() { + continue } + skillFile := filepath.Join(dir, d.Name(), "SKILL.md") + if _, err := os.Stat(skillFile); err != nil { + continue + } + info := SkillInfo{ + Name: d.Name(), + Path: skillFile, + Source: source, + } + metadata := sl.getSkillMetadata(skillFile) + if metadata != nil { + info.Description = metadata.Description + info.Name = metadata.Name + } + if err := info.validate(); err != nil { + slog.Warn("invalid skill from "+source, "name", info.Name, "error", err) + continue + } + if seen[info.Name] { + continue + } + seen[info.Name] = true + skills = append(skills, info) } } - // 全局 skills (~/.picoclaw/skills) - 被 workspace skills 覆盖 - if sl.globalSkills != "" { - if dirs, err := os.ReadDir(sl.globalSkills); err == nil { - for _, dir := range dirs { - if dir.IsDir() { - skillFile := filepath.Join(sl.globalSkills, dir.Name(), "SKILL.md") - if _, err := os.Stat(skillFile); err == nil { - // 检查是否已被 workspace skills 覆盖 - exists := false - for _, s := range skills { - if s.Name == dir.Name() && s.Source == "workspace" { - exists = true - break - } - } - if exists { - continue - } - - info := SkillInfo{ - Name: dir.Name(), - Path: skillFile, - Source: "global", - } - metadata := sl.getSkillMetadata(skillFile) - if metadata != nil { - info.Description = metadata.Description - info.Name = metadata.Name - } - if err := info.validate(); err != nil { - slog.Warn("invalid skill from global", "name", info.Name, "error", err) - continue - } - skills = append(skills, info) - } - } - } - } - } - - if sl.builtinSkills != "" { - if dirs, err := os.ReadDir(sl.builtinSkills); err == nil { - for _, dir := range dirs { - if dir.IsDir() { - skillFile := filepath.Join(sl.builtinSkills, dir.Name(), "SKILL.md") - if _, err := os.Stat(skillFile); err == nil { - // 检查是否已被 workspace 或 global skills 覆盖 - exists := false - for _, s := range skills { - if s.Name == dir.Name() && (s.Source == "workspace" || s.Source == "global") { - exists = true - break - } - } - if exists { - continue - } - - info := SkillInfo{ - Name: dir.Name(), - Path: skillFile, - Source: "builtin", - } - metadata := sl.getSkillMetadata(skillFile) - if metadata != nil { - info.Description = metadata.Description - info.Name = metadata.Name - } - if err := info.validate(); err != nil { - slog.Warn("invalid skill from builtin", "name", info.Name, "error", err) - continue - } - skills = append(skills, info) - } - } - } - } - } + // Priority: workspace > global > builtin + addSkills(sl.workspaceSkills, "workspace") + addSkills(sl.globalSkills, "global") + addSkills(sl.builtinSkills, "builtin") return skills } diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index aca901d33..9428bea62 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -1,9 +1,12 @@ package skills import ( + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSkillsInfoValidate(t *testing.T) { @@ -135,6 +138,134 @@ func TestExtractFrontmatter(t *testing.T) { } } +// createSkillDir creates a skill directory with a SKILL.md file containing the given frontmatter. +func createSkillDir(t *testing.T, base, dirName, name, description string) { + t.Helper() + dir := filepath.Join(base, dirName) + require.NoError(t, os.MkdirAll(dir, 0o755)) + content := "---\nname: " + name + "\ndescription: " + description + "\n---\n\n# " + name + require.NoError(t, os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(content), 0o644)) +} + +func TestListSkillsWorkspaceOverridesGlobal(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + + createSkillDir(t, filepath.Join(ws, "skills"), "my-skill", "my-skill", "workspace version") + createSkillDir(t, global, "my-skill", "my-skill", "global version") + + sl := NewSkillsLoader(ws, global, "") + skills := sl.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "workspace", skills[0].Source) + assert.Equal(t, "workspace version", skills[0].Description) +} + +func TestListSkillsGlobalOverridesBuiltin(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + builtin := filepath.Join(tmp, "builtin") + + createSkillDir(t, global, "my-skill", "my-skill", "global version") + createSkillDir(t, builtin, "my-skill", "my-skill", "builtin version") + + sl := NewSkillsLoader(ws, global, builtin) + skills := sl.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "global", skills[0].Source) + assert.Equal(t, "global version", skills[0].Description) +} + +func TestListSkillsMetadataNameDedup(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + + // Different directory names but same metadata name + createSkillDir(t, filepath.Join(ws, "skills"), "dir-a", "shared-name", "workspace version") + createSkillDir(t, global, "dir-b", "shared-name", "global version") + + sl := NewSkillsLoader(ws, global, "") + skills := sl.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "shared-name", skills[0].Name) + assert.Equal(t, "workspace", skills[0].Source) +} + +func TestListSkillsMultipleDistinctSkills(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + builtin := filepath.Join(tmp, "builtin") + + createSkillDir(t, filepath.Join(ws, "skills"), "skill-a", "skill-a", "desc a") + createSkillDir(t, global, "skill-b", "skill-b", "desc b") + createSkillDir(t, builtin, "skill-c", "skill-c", "desc c") + + sl := NewSkillsLoader(ws, global, builtin) + skills := sl.ListSkills() + + assert.Len(t, skills, 3) + names := map[string]string{} + for _, s := range skills { + names[s.Name] = s.Source + } + assert.Equal(t, "workspace", names["skill-a"]) + assert.Equal(t, "global", names["skill-b"]) + assert.Equal(t, "builtin", names["skill-c"]) +} + +func TestListSkillsInvalidSkillSkipped(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + + // Invalid name (underscore) + createSkillDir(t, filepath.Join(ws, "skills"), "bad_skill", "bad_skill", "desc") + // Valid skill + createSkillDir(t, global, "good-skill", "good-skill", "desc") + + sl := NewSkillsLoader(ws, global, "") + skills := sl.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "good-skill", skills[0].Name) +} + +func TestListSkillsEmptyAndNonexistentDirs(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + emptyDir := filepath.Join(tmp, "empty") + require.NoError(t, os.MkdirAll(emptyDir, 0o755)) + + sl := NewSkillsLoader(ws, emptyDir, filepath.Join(tmp, "nonexistent")) + skills := sl.ListSkills() + + assert.Empty(t, skills) +} + +func TestListSkillsDirWithoutSkillMD(t *testing.T) { + tmp := t.TempDir() + ws := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + + // Directory exists but has no SKILL.md + require.NoError(t, os.MkdirAll(filepath.Join(global, "no-skillmd"), 0o755)) + // Valid skill alongside + createSkillDir(t, global, "real-skill", "real-skill", "desc") + + sl := NewSkillsLoader(ws, global, "") + skills := sl.ListSkills() + + assert.Len(t, skills, 1) + assert.Equal(t, "real-skill", skills[0].Name) +} + func TestStripFrontmatter(t *testing.T) { sl := &SkillsLoader{}