From edb825851087c0492d6d4033b9e0fdbf5f13a9af Mon Sep 17 00:00:00 2001 From: Reidho Satria Date: Wed, 16 Sep 2026 15:36:46 +0700 Subject: [PATCH 1/9] feat(skills): discover conventional user and project skill directories --- cmd/antares/main.go | 47 ++-- docs/skills.md | 34 ++- internal/skills/discovery.go | 204 ++++++++++++++ internal/skills/discovery_test.go | 443 ++++++++++++++++++++++++++++++ internal/skills/search_test.go | 2 +- internal/skills/skills.go | 178 ++++++------ 6 files changed, 794 insertions(+), 114 deletions(-) create mode 100644 internal/skills/discovery.go create mode 100644 internal/skills/discovery_test.go diff --git a/cmd/antares/main.go b/cmd/antares/main.go index 4468e1c..0c004bf 100644 --- a/cmd/antares/main.go +++ b/cmd/antares/main.go @@ -195,16 +195,18 @@ func cmdTUI() error { // runtimeServices bundles everything a running server needs, so a config reload // can rebuild the pieces that depend on configuration. type runtimeServices struct { - mu sync.Mutex - cfg *config.Config - db store.Store - shell *tools.ShellManager - agent *agent.Agent - skills *skills.Manager - cron *cron.Runner - gateway *gateway.Manager - mcp *mcp.Manager - social *socialbrowser.Manager + mu sync.Mutex + cfg *config.Config + db store.Store + shell *tools.ShellManager + agent *agent.Agent + skills *skills.Manager + cron *cron.Runner + gateway *gateway.Manager + mcp *mcp.Manager + social *socialbrowser.Manager + skillsHome string + skillsProjectDir string } func bootstrap(ctx context.Context) (*runtimeServices, error) { @@ -274,9 +276,20 @@ func bootstrap(ctx context.Context) (*runtimeServices, error) { slog.Info("unpacked the security skill library", "count", n) } - skillDirs := append(append([]string{}, cfg.Skills.Dirs...), "~/.antares/security-skills") - skillMgr := skills.NewManager(expandAll(skillDirs)) - skillMgr.SetPackDirs([]string{packDir}) + skillsHome, err := os.UserHomeDir() + if err != nil || strings.TrimSpace(skillsHome) == "" { + slog.Warn("automatic user skills unavailable", "error", err) + skillsHome = "" + } + skillsProjectDir, err := os.Getwd() + if err != nil { + slog.Warn("automatic project skills unavailable", "error", err) + skillsProjectDir = "" + } + skillMgr := skills.NewManager(skills.Options{ + Dirs: expandAll(cfg.Skills.Dirs), PackDirs: []string{packDir}, + UserHome: skillsHome, ProjectDir: skillsProjectDir, + }) if err := skillMgr.Reload(); err != nil { slog.Warn("some skills failed to load", "error", err) } @@ -300,6 +313,7 @@ func bootstrap(ctx context.Context) (*runtimeServices, error) { ag.SetRoles(roleReg) rt := &runtimeServices{cfg: cfg, db: db, shell: shell, agent: ag, skills: skillMgr} + rt.skillsHome, rt.skillsProjectDir = skillsHome, skillsProjectDir rt.social = socialbrowser.New() ag.SetSocialBrowser(rt.social) @@ -719,9 +733,10 @@ func (rt *runtimeServices) reload() error { rt.agent.SetRAG(ragProvider) packDir := config.Path("security-skills") - skillDirs := append(append([]string{}, cfg.Skills.Dirs...), "~/.antares/security-skills") - rt.skills = skills.NewManager(expandAll(skillDirs)) - rt.skills.SetPackDirs([]string{packDir}) + rt.skills = skills.NewManager(skills.Options{ + Dirs: expandAll(cfg.Skills.Dirs), PackDirs: []string{packDir}, + UserHome: rt.skillsHome, ProjectDir: rt.skillsProjectDir, + }) if err := rt.skills.Reload(); err != nil { slog.Warn("some skills failed to load", "error", err) } diff --git a/docs/skills.md b/docs/skills.md index 3afddb5..4823e6f 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -58,9 +58,37 @@ skills: dirs: [~/.antares/skills] ``` -Several directories are searched in order and later ones win, so a personal copy -overrides a shared one — useful for a team directory in a repository plus your -own adjustments. +Configured `dirs` remain writable; new skills save to the first nonblank directory. +Native `~/.antares` paths follow `ANTARES_HOME`. Flat `.md` files still work there. + +Antares also discovers these directories automatically, in the order shown: + +| Under the OS user home | Under the selected project | +|---|---| +| `.agent/skills` | `.agent/skills` | +| `.agents/skills` | `.agents/skills` | +| `.claude/skills` | `.claude/skills` | +| `.codex/skills` | `.codex/skills` | +| `.config/opencode/skills` | `.opencode/skills` | +| `.omp/agent/managed-skills` | `.github/skills` | + +Automatic roots use the OS home independently of `ANTARES_HOME`; the OpenCode +home path does not follow `XDG_CONFIG_HOME`. Project roots are beneath the startup +directory, without searching parent directories. + +Automatic sources accept only `SKILL.md` (case-insensitive), recursively. Supporting +Markdown and hidden descendants are ignored. A missing name uses the logical parent +folder name. Symlinks are followed with cycle detection; missing roots are not created. + +For duplicate names, priority from lowest to highest is bundled security pack, +automatic user roots, automatic project roots, then configured `dirs`. Later roots +within each group win; files within a root are visited in lexical order. + +Automatically discovered skills are read-only through skill management: save, +toggle, and delete refuse to modify or shadow them. Edit the original file instead. +An explicitly configured copy wins and remains writable, including when its directory +is also an automatic root. Hub installs and `/learn` still write an Antares copy +to their configured/native destination. ## Getting them diff --git a/internal/skills/discovery.go b/internal/skills/discovery.go new file mode 100644 index 0000000..12a136a --- /dev/null +++ b/internal/skills/discovery.go @@ -0,0 +1,204 @@ +package skills + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + "strings" +) + +type sourceKind uint8 + +const ( + sourcePack sourceKind = iota + sourceUser + sourceProject + sourceConfigured +) + +type sourceRoot struct { + path string + kind sourceKind +} + +var userSkillRoots = [][]string{ + {".agent", "skills"}, + {".agents", "skills"}, + {".claude", "skills"}, + {".codex", "skills"}, + {".config", "opencode", "skills"}, + {".omp", "agent", "managed-skills"}, +} + +var projectSkillRoots = [][]string{ + {".agent", "skills"}, + {".agents", "skills"}, + {".claude", "skills"}, + {".codex", "skills"}, + {".opencode", "skills"}, + {".github", "skills"}, +} + +// discover scans roots from low to high priority. Later occurrences of a name +// replace earlier ones, while malformed entries leave the rest of the freshly +// discovered catalogue available. +func discover(opts Options) (map[string]*Skill, error) { + found := make(map[string]*Skill) + var firstErr error + for _, root := range discoveryRoots(opts) { + if err := scanRoot(root, func(skill *Skill) { + found[skill.Name] = skill + }); err != nil && firstErr == nil { + firstErr = err + } + } + return found, firstErr +} + +func discoveryRoots(opts Options) []sourceRoot { + roots := make([]sourceRoot, 0, len(opts.PackDirs)+len(opts.Dirs)+12) + roots = appendSourceRoots(roots, opts.PackDirs, sourcePack) + if strings.TrimSpace(opts.UserHome) != "" { + paths := make([]string, 0, len(userSkillRoots)) + for _, parts := range userSkillRoots { + paths = append(paths, filepath.Join(append([]string{opts.UserHome}, parts...)...)) + } + roots = appendSourceRoots(roots, paths, sourceUser) + } + if strings.TrimSpace(opts.ProjectDir) != "" { + paths := make([]string, 0, len(projectSkillRoots)) + for _, parts := range projectSkillRoots { + paths = append(paths, filepath.Join(append([]string{opts.ProjectDir}, parts...)...)) + } + roots = appendSourceRoots(roots, paths, sourceProject) + } + return appendSourceRoots(roots, opts.Dirs, sourceConfigured) +} + +// appendSourceRoots drops equivalent roots only within one source kind. Walking +// backwards preserves the last, highest-priority spelling of each root. +func appendSourceRoots(dst []sourceRoot, paths []string, kind sourceKind) []sourceRoot { + kept := make([]string, 0, len(paths)) + for i := len(paths) - 1; i >= 0; i-- { + path := paths[i] + if strings.TrimSpace(path) == "" { + continue + } + logical, err := filepath.Abs(path) + if err != nil { + logical = filepath.Clean(path) + } + duplicate := false + for _, prior := range kept { + if sameRoot(logical, prior) { + duplicate = true + break + } + } + if duplicate { + continue + } + kept = append(kept, logical) + } + for i := len(kept) - 1; i >= 0; i-- { + dst = append(dst, sourceRoot{path: kept[i], kind: kind}) + } + return dst +} + +func sameRoot(left, right string) bool { + leftInfo, leftErr := os.Stat(left) + rightInfo, rightErr := os.Stat(right) + if leftErr == nil && rightErr == nil { + return os.SameFile(leftInfo, rightInfo) + } + return left == right +} + +func scanRoot(root sourceRoot, publish func(*Skill)) error { + var firstErr error + ancestors := make(map[string]struct{}) + var walk func(string) + walk = func(logical string) { + info, err := os.Stat(logical) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { + firstErr = err + } + return + } + if info.IsDir() { + canonical, err := filepath.EvalSymlinks(logical) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { + firstErr = err + } + return + } + canonical, err = filepath.Abs(canonical) + if err != nil { + if firstErr == nil { + firstErr = err + } + return + } + if _, cycle := ancestors[canonical]; cycle { + return + } + ancestors[canonical] = struct{}{} + entries, err := os.ReadDir(logical) + if err != nil { + delete(ancestors, canonical) + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { + firstErr = err + } + return + } + for _, entry := range entries { // os.ReadDir returns lexical order. + if strings.HasPrefix(entry.Name(), ".") { + continue + } + walk(filepath.Join(logical, entry.Name())) + } + delete(ancestors, canonical) + return + } + if !info.Mode().IsRegular() { + return + } + base := filepath.Base(logical) + if root.kind == sourceUser || root.kind == sourceProject { + if !strings.EqualFold(base, "SKILL.md") { + return + } + } else if !strings.EqualFold(filepath.Ext(base), ".md") { + return + } + + skill, err := parseFile(logical) + if err != nil { + if firstErr == nil { + firstErr = err + } + return + } + if strings.TrimSpace(skill.Name) == "" { + if strings.EqualFold(base, "SKILL.md") { + skill.Name = filepath.Base(filepath.Dir(logical)) + } else { + skill.Name = strings.TrimSuffix(base, filepath.Ext(base)) + } + } + if skill.Source == "" { + skill.Source = "local" + } + skill.Path = logical + skill.UpdatedAt = info.ModTime() + skill.Pack = root.kind == sourcePack + skill.ReadOnly = root.kind == sourceUser || root.kind == sourceProject + publish(skill) + } + walk(root.path) + return firstErr +} diff --git a/internal/skills/discovery_test.go b/internal/skills/discovery_test.go new file mode 100644 index 0000000..7b7395c --- /dev/null +++ b/internal/skills/discovery_test.go @@ -0,0 +1,443 @@ +package skills + +import ( + "errors" + "os" + "os/exec" + "path/filepath" + "runtime" + "strings" + "testing" +) + +var expectedUserSkillRoots = [][]string{ + {".agent", "skills"}, + {".agents", "skills"}, + {".claude", "skills"}, + {".codex", "skills"}, + {".config", "opencode", "skills"}, + {".omp", "agent", "managed-skills"}, +} + +var expectedProjectSkillRoots = [][]string{ + {".agent", "skills"}, + {".agents", "skills"}, + {".claude", "skills"}, + {".codex", "skills"}, + {".opencode", "skills"}, + {".github", "skills"}, +} + +func makeDir(t *testing.T, path string) string { + t.Helper() + if err := os.MkdirAll(path, 0o755); err != nil { + t.Fatal(err) + } + return path +} +func writeFile(t *testing.T, path, content string) { + t.Helper() + makeDir(t, filepath.Dir(path)) + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func skillDocument(name, description, body string, enabled bool) string { + enabledText := "true" + if !enabled { + enabledText = "false" + } + header := "---\n" + if name != "" { + header += "name: " + name + "\n" + } + return header + "description: " + description + "\nenabled: " + enabledText + "\nsource: fixture\n" + + "tags: [discovery]\ntriggers: [on demand]\ncategory: testing\n" + + "tech_stack: [go]\ncwe_ids: [CWE-1]\nowasp_id: A01\nchains_with: [next]\n---\n\n" + body + "\n" +} + +func mustReload(t *testing.T, m *Manager) { + t.Helper() + if err := m.Reload(); err != nil { + t.Fatal(err) + } +} + +func requireSkill(t *testing.T, m *Manager, name string) *Skill { + t.Helper() + skill, ok := m.Get(name) + if !ok { + t.Fatalf("skill %q was not discovered; got %v", name, names(m.List())) + } + return skill +} + +func TestDiscoveryRootsAndFormats(t *testing.T) { + home := t.TempDir() + project := t.TempDir() + configured := makeDir(t, filepath.Join(t.TempDir(), "native")) + missing := filepath.Join(t.TempDir(), "must-stay-missing") + missingHome := filepath.Join(t.TempDir(), "missing-home") + missingAutomatic := filepath.Join(missingHome, ".agent", "skills") + empty := NewManager(Options{UserHome: missingHome}) + mustReload(t, empty) + if _, err := os.Stat(missingAutomatic); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing automatic root was created or returned an unexpected error: %v", err) + } + + for i, parts := range expectedUserSkillRoots { + name := "user-root-" + string(rune('1'+i)) + path := filepath.Join(append([]string{home}, parts...)...) + writeFile(t, filepath.Join(path, name, "SKILL.md"), skillDocument(name, name+" description", name+" body", true)) + } + for i, parts := range expectedProjectSkillRoots { + name := "project-root-" + string(rune('1'+i)) + path := filepath.Join(append([]string{project}, parts...)...) + writeFile(t, filepath.Join(path, name, "sKiLl.Md"), skillDocument(name, name+" description", name+" body", true)) + } + + fallbackPath := filepath.Join(home, ".agent", "skills", "logical-fallback", "nested", "SKILL.md") + writeFile(t, fallbackPath, skillDocument("", "fallback description", "fallback body", true)) + supportRoot := filepath.Join(home, ".agent", "skills", "support") + writeFile(t, filepath.Join(supportRoot, "README.md"), skillDocument("support-readme", "ignored", "ignored", true)) + writeFile(t, filepath.Join(supportRoot, "DESIGN.md"), skillDocument("support-design", "ignored", "ignored", true)) + writeFile(t, filepath.Join(supportRoot, "references", "help.md"), skillDocument("support-help", "ignored", "ignored", true)) + writeFile(t, filepath.Join(supportRoot, ".hidden", "SKILL.md"), skillDocument("hidden-skill", "ignored", "ignored", true)) + writeFile(t, filepath.Join(configured, "flat.md"), skillDocument("native-flat", "native description", "native body", true)) + writeFile(t, filepath.Join(configured, "disabled.md"), skillDocument("disabled-native", "disabled description", "disabled body", false)) + writeFile(t, filepath.Join(configured, "broken.md"), "---\nname: [not valid\n---\nbroken") + writeFile(t, filepath.Join(configured, "valid.md"), skillDocument("valid-beside-broken", "valid", "valid body", true)) + + configuredFallbackPath := filepath.Join(configured, "configured-parent", "SKILL.md") + writeFile(t, configuredFallbackPath, skillDocument("", "configured fallback", "configured fallback body", true)) + hiddenConfiguredRoot := makeDir(t, filepath.Join(t.TempDir(), ".explicit-hidden-root")) + writeFile(t, filepath.Join(hiddenConfiguredRoot, "visible.md"), skillDocument("hidden-root-visible", "visible", "VISIBLE", true)) + m := NewManager(Options{Dirs: []string{missing, configured, hiddenConfiguredRoot}, UserHome: home, ProjectDir: project}) + err := m.Reload() + if err == nil || !strings.Contains(err.Error(), "invalid front matter") { + t.Fatalf("Reload error = %v, want malformed front matter error", err) + } + for i := 1; i <= 6; i++ { + requireSkill(t, m, "user-root-"+string(rune('0'+i))) + requireSkill(t, m, "project-root-"+string(rune('0'+i))) + } + fallback := requireSkill(t, m, "nested") + if fallback.Path != fallbackPath || !fallback.ReadOnly || fallback.Description != "fallback description" { + t.Fatalf("fallback skill = %+v, want logical parent name/path, metadata, and read-only provenance", fallback) + } + metadata := requireSkill(t, m, "user-root-1") + if !metadata.ReadOnly || metadata.Pack || metadata.Source != "fixture" || metadata.UpdatedAt.IsZero() || metadata.Category != "testing" || metadata.OWASPID != "A01" || len(metadata.Tags) != 1 || len(metadata.Triggers) != 1 || len(metadata.TechStack) != 1 || len(metadata.CWEIDs) != 1 || len(metadata.ChainsWith) != 1 { + t.Fatalf("front matter metadata or automatic provenance was not preserved: %+v", metadata) + } + metadata.Tags[0] = "mutated" + if got := requireSkill(t, m, "user-root-1"); got.Tags[0] != "discovery" { + t.Fatalf("caller mutation leaked into manager state: %+v", got.Tags) + } + for _, absent := range []string{"support-readme", "support-design", "support-help", "hidden-skill"} { + if _, ok := m.Get(absent); ok { + t.Fatalf("support/hidden document %q was loaded", absent) + } + } + configuredFallback := requireSkill(t, m, "configured-parent") + if configuredFallback.Path != configuredFallbackPath || configuredFallback.ReadOnly { + t.Fatalf("configured SKILL.md fallback = %+v, want writable parent fallback", configuredFallback) + } + if requireSkill(t, m, "native-flat").ReadOnly { + t.Fatal("configured flat Markdown must remain writable") + } + if strings.Contains(m.PromptBlock(0), "disabled-native") { + t.Fatal("disabled skill appeared in PromptBlock") + } + requireSkill(t, m, "valid-beside-broken") + requireSkill(t, m, "hidden-root-visible") + if _, err := os.Stat(missing); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("missing root was created or returned an unexpected error: %v", err) + } +} + +func TestDiscoveryPrecedenceAndReadOnly(t *testing.T) { + base := t.TempDir() + home, project := filepath.Join(base, "home"), filepath.Join(base, "project") + pack := makeDir(t, filepath.Join(base, "security-skills")) + packSibling := makeDir(t, filepath.Join(base, "security-skills-copy")) + configured1 := makeDir(t, filepath.Join(base, "configured-1")) + configured2 := makeDir(t, filepath.Join(base, "configured-2")) + user1 := filepath.Join(home, ".agent", "skills") + user2 := filepath.Join(home, ".agents", "skills") + project1 := filepath.Join(project, ".agent", "skills") + project2 := filepath.Join(project, ".agents", "skills") + // Within one root lexical traversal is the final tie-breaker. + writeFile(t, filepath.Join(configured2, "a-collision.md"), skillDocument("lexical-winner", "a", "LEXICAL_A", true)) + writeFile(t, filepath.Join(configured2, "z-collision.md"), skillDocument("lexical-winner", "z", "LEXICAL_Z", true)) + + type fixture struct { + path string + body string + } + fixtures := []fixture{ + {filepath.Join(pack, "winner.md"), "pack"}, + {filepath.Join(user1, "winner", "SKILL.md"), "user-1"}, + {filepath.Join(user2, "winner", "SKILL.md"), "user-2"}, + {filepath.Join(project1, "winner", "SKILL.md"), "project-1"}, + {filepath.Join(project2, "winner", "SKILL.md"), "project-2"}, + {filepath.Join(configured1, "winner.md"), "configured-1"}, + {filepath.Join(configured2, "winner.md"), "configured-2"}, + } + for _, fixture := range fixtures { + writeFile(t, fixture.path, skillDocument("winner", fixture.body, fixture.body, true)) + } + writeFile(t, filepath.Join(packSibling, "sibling.md"), skillDocument("pack-sibling", "sibling", "sibling", true)) + + dirs := []string{" ", configured1, configured2, configured2, packSibling} + packs := []string{pack} + m := NewManager(Options{Dirs: dirs, PackDirs: packs, UserHome: home, ProjectDir: project}) + dirs[1] = filepath.Join(base, "mutated-caller") + packs[0] = packSibling + mustReload(t, m) + winner := requireSkill(t, m, "winner") + if winner.Body != "configured-2" || winner.ReadOnly || winner.Pack { + t.Fatalf("initial winner = %+v, want second configured root", winner) + } + if sibling := requireSkill(t, m, "pack-sibling"); sibling.Pack || sibling.ReadOnly { + t.Fatalf("configured sibling was mislabeled as pack/imported: %+v", sibling) + } + if got := requireSkill(t, m, "lexical-winner"); got.Body != "LEXICAL_Z" { + t.Fatalf("within-root winner body = %q, want lexically later file", got.Body) + } + + // Equivalent roots are retained only once per kind, using the last logical + // spelling, and nonblank directory bytes are not trimmed. + dedupeTarget := makeDir(t, filepath.Join(base, "dedupe-target")) + writeFile(t, filepath.Join(dedupeTarget, "deduped.md"), skillDocument("deduped", "deduped", "DEDUPED", true)) + alias1, alias2 := filepath.Join(base, "alias-1"), filepath.Join(base, "alias-2") + if runtime.GOOS == "windows" { + t.Log("skipping equivalent-symlink root assertion on Windows") + } else { + if err := os.Symlink(dedupeTarget, alias1); err != nil { + t.Fatalf("directory symlinks unavailable after discovery assertions: %v", err) + } + if err := os.Symlink(dedupeTarget, alias2); err != nil { + t.Fatal(err) + } + } + spacedRoot := makeDir(t, filepath.Join(base, " spaced root ")) + writeFile(t, filepath.Join(spacedRoot, "spaced.md"), skillDocument("spaced-root", "spaced", "SPACED", true)) + crossKind := makeDir(t, filepath.Join(base, "cross-kind")) + writeFile(t, filepath.Join(crossKind, "shared.md"), skillDocument("cross-kind", "cross", "CROSS", true)) + dedupeDirs := []string{spacedRoot, crossKind} + if runtime.GOOS != "windows" { + dedupeDirs = []string{alias1, spacedRoot, alias2, crossKind} + } + dedupeManager := NewManager(Options{Dirs: dedupeDirs, PackDirs: []string{crossKind}}) + mustReload(t, dedupeManager) + if runtime.GOOS != "windows" { + if got := requireSkill(t, dedupeManager, "deduped"); got.Path != filepath.Join(alias2, "deduped.md") { + t.Fatalf("deduplicated root path = %q, want last logical alias", got.Path) + } + } + if got := requireSkill(t, dedupeManager, "spaced-root"); got.Path != filepath.Join(spacedRoot, "spaced.md") { + t.Fatalf("spaced configured root path = %q, want bytes preserved", got.Path) + } + if got := requireSkill(t, dedupeManager, "cross-kind"); got.Pack || got.ReadOnly { + t.Fatalf("explicit occurrence was suppressed by equivalent pack root: %+v", got) + } + m.MarkUsed("winner") + for i := len(fixtures) - 1; i > 0; i-- { + if err := os.Remove(fixtures[i].path); err != nil { + t.Fatal(err) + } + mustReload(t, m) + got := requireSkill(t, m, "winner") + want := fixtures[i-1].body + if got.Body != want || got.UsageCount != 1 { + t.Fatalf("after removing %q winner = body %q usage %d, want %q usage 1", fixtures[i].body, got.Body, got.UsageCount, want) + } + wantPack := i-1 == 0 + wantReadOnly := i-1 >= 1 && i-1 <= 4 + if got.Pack != wantPack || got.ReadOnly != wantReadOnly { + t.Fatalf("winner provenance after removing %q = pack %v readonly %v, want %v/%v", fixtures[i].body, got.Pack, got.ReadOnly, wantPack, wantReadOnly) + } + } + if got := requireSkill(t, m, "winner"); !got.Pack || got.ReadOnly { + t.Fatalf("pack winner provenance = %+v, want pack and mutable", got) + } + if err := os.Remove(fixtures[0].path); err != nil { + t.Fatal(err) + } + mustReload(t, m) + if _, ok := m.Get("winner"); ok { + t.Fatal("winner remained after every source was removed") + } + + packMutablePath := filepath.Join(pack, "pack-mutable.md") + writeFile(t, packMutablePath, skillDocument("pack-mutable", "pack", "PACK_MUTABLE", true)) + mustReload(t, m) + if err := m.SetEnabled("pack-mutable", false); err != nil { + t.Fatalf("pack skill must retain existing mutability: %v", err) + } + if requireSkill(t, m, "pack-mutable").Enabled { + t.Fatal("pack toggle did not update the effective skill") + } + importedPath := filepath.Join(user1, "imported", "SKILL.md") + original := skillDocument("imported", "imported", "ORIGINAL", true) + writeFile(t, importedPath, original) + mixedPath := filepath.Join(user1, "mixed", "SKILL.md") + mixedOriginal := skillDocument("Mixed Name", "mixed", "MIXED_ORIGINAL", true) + normalizedPath := filepath.Join(user1, "normalized", "SKILL.md") + normalizedOriginal := skillDocument("normalized-name", "normalized", "NORMALIZED_ORIGINAL", true) + writeFile(t, normalizedPath, normalizedOriginal) + writeFile(t, mixedPath, mixedOriginal) + mustReload(t, m) + operations := []struct { + name string + run func() error + }{ + {"save", func() error { _, err := m.Save("imported", "changed", "changed", nil); return err }}, + {"toggle", func() error { return m.SetEnabled("imported", false) }}, + {"delete", func() error { return m.Delete("imported") }}, + } + for _, operation := range operations { + if err := operation.run(); !errors.Is(err, ErrReadOnly) { + t.Fatalf("%s error = %v, want ErrReadOnly", operation.name, err) + } + } + if _, err := m.Save("Mixed Name", "changed", "changed", nil); !errors.Is(err, ErrReadOnly) { + t.Fatalf("Save using unsanitized imported metadata name error = %v, want ErrReadOnly", err) + } + if _, err := m.Save("Normalized Name", "changed", "changed", nil); !errors.Is(err, ErrReadOnly) { + t.Fatalf("Save using a name that normalizes to an imported name error = %v, want ErrReadOnly", err) + } + if raw, err := os.ReadFile(normalizedPath); err != nil || string(raw) != normalizedOriginal { + t.Fatalf("normalized-name read-only source changed: err=%v bytes=%q", err, raw) + } + if raw, err := os.ReadFile(mixedPath); err != nil || string(raw) != mixedOriginal { + t.Fatalf("mixed-name read-only source changed: err=%v bytes=%q", err, raw) + } + if raw, err := os.ReadFile(importedPath); err != nil || string(raw) != original { + t.Fatalf("read-only source changed: err=%v bytes=%q", err, raw) + } + if _, err := os.Stat(filepath.Join(configured1, "imported.md")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("read-only Save created a configured shadow: %v", err) + } + saved, err := m.Save("new skill", "new", "new body", nil) + if err != nil { + t.Fatal(err) + } + if saved.Path != filepath.Join(configured1, "new-skill.md") { + t.Fatalf("new skill path = %q, want first nonempty configured root", saved.Path) + } + + override := NewManager(Options{Dirs: []string{user1}, UserHome: home}) + mustReload(t, override) + if requireSkill(t, override, "imported").ReadOnly { + t.Fatal("explicit configuration of an automatic root must make its winner writable") + } + if err := override.SetEnabled("imported", false); err != nil { + t.Fatalf("explicit override toggle failed: %v", err) + } + + noWritable := NewManager(Options{Dirs: []string{"", " "}, UserHome: home}) + mustReload(t, noWritable) + if _, err := noWritable.Save("new", "", "", nil); err == nil || err.Error() != "no skills directory configured" { + t.Fatalf("Save without a configured directory error = %v", err) + } +} + +func TestDiscoverySymlinks(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink and FIFO matrix is exercised on Unix") + } + base := t.TempDir() + home := filepath.Join(base, "home") + root := filepath.Join(home, ".agent", "skills") + targetA := makeDir(t, filepath.Join(base, "target-a")) + targetB := makeDir(t, filepath.Join(base, "target-b")) + writeFile(t, filepath.Join(targetA, "SKILL.md"), skillDocument("linked-root", "target a", "A_BODY", true)) + writeFile(t, filepath.Join(targetB, "SKILL.md"), skillDocument("linked-root", "target b", "B_BODY", true)) + makeDir(t, filepath.Dir(root)) + if err := os.Symlink(targetA, root); err != nil { + t.Skipf("directory symlinks unavailable: %v", err) + } + + folderTarget := makeDir(t, filepath.Join(base, "folder-target")) + writeFile(t, filepath.Join(folderTarget, "SKILL.md"), skillDocument("", "folder alias", "FOLDER_BODY", true)) + if err := os.Symlink(folderTarget, filepath.Join(targetA, "logical-folder")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(folderTarget, filepath.Join(targetA, "other-folder")); err != nil { + t.Fatal(err) + } + fileTarget := filepath.Join(base, "file-target.md") + writeFile(t, fileTarget, skillDocument("linked-file", "file alias", "FILE_BODY", true)) + if err := os.Symlink(fileTarget, filepath.Join(targetA, "SKILL-LINK.md")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(fileTarget, filepath.Join(targetA, "SKILL.md.link")); err != nil { + t.Fatal(err) + } + fileAliasDir := makeDir(t, filepath.Join(targetA, "file-alias")) + if err := os.Symlink(fileTarget, filepath.Join(fileAliasDir, "SKILL.md")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(base, "absent"), filepath.Join(targetA, "broken")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetA, filepath.Join(targetA, "cycle")); err != nil { + t.Fatal(err) + } + writeFile(t, filepath.Join(targetA, "README.md"), skillDocument("support-link", "ignored", "ignored", true)) + if err := os.Symlink(filepath.Join(targetA, "README.md"), filepath.Join(targetA, "support-copy.md")); err != nil { + t.Fatal(err) + } + fifoDir := makeDir(t, filepath.Join(targetA, "fifo")) + fifo := filepath.Join(fifoDir, "SKILL.md") + if _, err := exec.LookPath("mkfifo"); err != nil { + t.Skipf("FIFO creation tool unavailable: %v", err) + } + if output, err := exec.Command("mkfifo", "-m", "600", fifo).CombinedOutput(); err != nil { + t.Fatalf("FIFO creation failed: %v: %s", err, output) + } + + m := NewManager(Options{UserHome: home}) + mustReload(t, m) + linked := requireSkill(t, m, "linked-root") + if linked.Path != filepath.Join(root, "SKILL.md") || linked.Body != "A_BODY" { + t.Fatalf("linked root = %+v, want logical path and target A body", linked) + } + otherFolder := requireSkill(t, m, "other-folder") + if otherFolder.Path != filepath.Join(root, "other-folder", "SKILL.md") || otherFolder.Body != "FOLDER_BODY" { + t.Fatalf("second logical alias fallback/path = %+v, want independent logical name/path", otherFolder) + } + folder := requireSkill(t, m, "logical-folder") + if folder.Path != filepath.Join(root, "logical-folder", "SKILL.md") { + t.Fatalf("symlinked folder fallback/path = %+v, want logical alias", folder) + } + file := requireSkill(t, m, "linked-file") + if file.Path != filepath.Join(root, "file-alias", "SKILL.md") { + t.Fatalf("symlinked SKILL.md path = %q, want logical path", file.Path) + } + for _, absent := range []string{"support-link", "fifo"} { + if _, ok := m.Get(absent); ok { + t.Fatalf("non-procedure %q was loaded", absent) + } + } + + if err := os.Remove(root); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetB, root); err != nil { + t.Fatal(err) + } + mustReload(t, m) + if got := requireSkill(t, m, "linked-root"); got.Body != "B_BODY" || got.Path != filepath.Join(root, "SKILL.md") { + t.Fatalf("retargeted root = %+v, want target B through same logical path", got) + } + if err := os.Remove(root); err != nil { + t.Fatal(err) + } + mustReload(t, m) + if _, ok := m.Get("linked-root"); ok { + t.Fatal("removed symlink root remained in the catalog") + } +} diff --git a/internal/skills/search_test.go b/internal/skills/search_test.go index 720319b..3b31c30 100644 --- a/internal/skills/search_test.go +++ b/internal/skills/search_test.go @@ -20,7 +20,7 @@ func loadManager(t *testing.T) *Manager { writeSkill(t, dir, "attack-sqli", "description: \"SQL injection\"\ncategory: web-application\ntags: [sqli, database]\ntech_stack: [web]\ncwe_ids: [CWE-89]\nchains_with: [attack-idor]\n") writeSkill(t, dir, "attack-idor", "description: \"IDOR\"\ncategory: web-application\ntags: [idor, authz]\ntech_stack: [web, api]\ncwe_ids: [CWE-639]\n") writeSkill(t, dir, "attack-jwt", "description: \"JWT attacks and token forgery\"\ncategory: web-application\ntags: [jwt, auth]\ntech_stack: [web]\ncwe_ids: [CWE-287, CWE-345]\n") - m := NewManager([]string{dir}) + m := NewManager(Options{Dirs: []string{dir}}) if err := m.Reload(); err != nil { t.Fatal(err) } diff --git a/internal/skills/skills.go b/internal/skills/skills.go index f29e755..10c77d8 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -5,7 +5,6 @@ package skills import ( "errors" "fmt" - "io/fs" "os" "path/filepath" "sort" @@ -39,6 +38,9 @@ type Skill struct { // loadable, but kept out of the prompt catalogue so thousands of them do // not bury the conversation. Pack bool `json:"pack,omitempty"` + // ReadOnly marks automatically discovered user/project skills. Pack roots + // preserve their existing management behavior. + ReadOnly bool `json:"read_only"` } // frontMatter is the YAML header of a skill file. @@ -56,90 +58,58 @@ type frontMatter struct { ChainsWith []string `yaml:"chains_with"` } -// Manager loads and caches skills from the configured directories. -type Manager struct { - mu sync.RWMutex - dirs []string - packDirs []string - skills map[string]*Skill - usage map[string]int +// Options describes the independent skill sources. Configured and pack +// directories retain existing mutation behavior; conventional roots are read-only. +type Options struct { + Dirs []string + PackDirs []string + UserHome string + ProjectDir string } -// NewManager builds a manager over the given directories. -func NewManager(dirs []string) *Manager { - return &Manager{dirs: dirs, skills: map[string]*Skill{}, usage: map[string]int{}} +// Manager loads and caches skills from configured and discovered directories. +type Manager struct { + mu sync.RWMutex + opts Options + skills map[string]*Skill + usage map[string]int } -// SetPackDirs marks directories whose skills are the bundled security library: -// searchable but not in the prompt catalogue. -func (m *Manager) SetPackDirs(dirs []string) { - m.mu.Lock() - m.packDirs = dirs - m.mu.Unlock() +// ErrReadOnly is returned when a mutation targets an imported skill. +var ErrReadOnly = errors.New("automatically discovered skills are read-only") + +// NewManager builds a manager over independent writable, bundled, user, and +// project sources. Options are cloned so caller mutations cannot reconfigure it. +func NewManager(opts Options) *Manager { + return &Manager{opts: cloneOptions(opts), skills: map[string]*Skill{}, usage: map[string]int{}} } -// isPack reports whether a path is under a pack directory. -func (m *Manager) isPack(path string) bool { - for _, d := range m.packDirs { - if d != "" && strings.HasPrefix(path, d) { - return true - } - } - return false +func cloneOptions(opts Options) Options { + opts.Dirs = append([]string(nil), opts.Dirs...) + opts.PackDirs = append([]string(nil), opts.PackDirs...) + return opts } -// Reload rescans every configured directory. +// Reload rescans every source and atomically publishes all successfully parsed +// entries. A malformed entry does not hide valid entries from the same scan. func (m *Manager) Reload() error { - found := map[string]*Skill{} - var firstErr error - - for _, dir := range m.dirs { - if strings.TrimSpace(dir) == "" { - continue - } - if err := os.MkdirAll(dir, 0o755); err != nil && firstErr == nil { - firstErr = err - } - err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { - if err != nil { - return nil - } - if d.IsDir() { - if strings.HasPrefix(d.Name(), ".") && d.Name() != "." { - return filepath.SkipDir - } - return nil - } - if !strings.EqualFold(filepath.Ext(path), ".md") { - return nil - } - s, err := parseFile(path) - if err != nil { - if firstErr == nil { - firstErr = err - } - return nil - } - // Later directories win, letting a user copy override a bundled skill. - found[s.Name] = s - return nil - }) - if err != nil && firstErr == nil { - firstErr = err - } - } + m.mu.RLock() + opts := cloneOptions(m.opts) + m.mu.RUnlock() + found, firstErr := discover(opts) m.mu.Lock() for name, sk := range found { sk.UsageCount = m.usage[name] - sk.Pack = m.isPack(sk.Path) } m.skills = found m.mu.Unlock() return firstErr } -// parseFile reads one skill file, tolerating a missing front matter block. +// parseFile reads one skill file, tolerating a missing front matter block. Its +// result is source-neutral: the scanner attaches logical path, fallback name, +// modification time, and provenance for each occurrence. func parseFile(path string) (*Skill, error) { raw, err := os.ReadFile(path) if err != nil { @@ -147,16 +117,7 @@ func parseFile(path string) (*Skill, error) { } text := strings.ReplaceAll(string(raw), "\r\n", "\n") - s := &Skill{ - Path: path, - Enabled: true, - Source: "local", - Name: strings.TrimSuffix(filepath.Base(path), filepath.Ext(path)), - } - if fi, err := os.Stat(path); err == nil { - s.UpdatedAt = fi.ModTime() - } - + s := &Skill{Enabled: true} body := text if strings.HasPrefix(text, "---\n") { if end := strings.Index(text[4:], "\n---"); end >= 0 { @@ -167,17 +128,13 @@ func parseFile(path string) (*Skill, error) { if err := yaml.Unmarshal([]byte(header), &fm); err != nil { return nil, fmt.Errorf("%s: invalid front matter: %w", path, err) } - if fm.Name != "" { - s.Name = fm.Name - } + s.Name = fm.Name s.Description = fm.Description s.Tags, s.Triggers = fm.Tags, fm.Triggers s.Category = fm.Category s.TechStack, s.CWEIDs, s.ChainsWith = fm.TechStack, fm.CWEIDs, fm.ChainsWith s.OWASPID = fm.OWASPID - if fm.Source != "" { - s.Source = fm.Source - } + s.Source = fm.Source if fm.Enabled != nil { s.Enabled = *fm.Enabled } @@ -217,7 +174,7 @@ func (m *Manager) List() []Skill { defer m.mu.RUnlock() out := make([]Skill, 0, len(m.skills)) for _, s := range m.skills { - out = append(out, *s) + out = append(out, cloneSkill(s)) } sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) return out @@ -231,16 +188,29 @@ func (m *Manager) Get(name string) (*Skill, bool) { if !ok { return nil, false } - cp := *s - return &cp, true + clone := cloneSkill(s) + return &clone, true } -// SetEnabled toggles a skill by rewriting its front matter. +func cloneSkill(s *Skill) Skill { + clone := *s + clone.Tags = append([]string(nil), s.Tags...) + clone.Triggers = append([]string(nil), s.Triggers...) + clone.TechStack = append([]string(nil), s.TechStack...) + clone.CWEIDs = append([]string(nil), s.CWEIDs...) + clone.ChainsWith = append([]string(nil), s.ChainsWith...) + return clone +} + +// SetEnabled toggles a writable skill by rewriting its front matter. func (m *Manager) SetEnabled(name string, enabled bool) error { s, ok := m.Get(name) if !ok { return fmt.Errorf("skill %q not found", name) } + if s.ReadOnly { + return fmt.Errorf("%w: %q", ErrReadOnly, name) + } raw, err := os.ReadFile(s.Path) if err != nil { return err @@ -261,8 +231,8 @@ func (m *Manager) SetEnabled(name string, enabled bool) error { rest := text[4+end:] if strings.Contains(header, "enabled:") { lines := strings.Split(header, "\n") - for i, l := range lines { - if strings.HasPrefix(strings.TrimSpace(l), "enabled:") { + for i, line := range lines { + if strings.HasPrefix(strings.TrimSpace(line), "enabled:") { lines[i] = "enabled: " + value } } @@ -281,16 +251,23 @@ func (m *Manager) SetEnabled(name string, enabled bool) error { return m.Reload() } -// Save writes (or overwrites) a skill file in the first configured directory. +// Save writes (or overwrites) a skill file in the first nonempty configured +// directory. Imported effective names cannot be shadowed through this API. func (m *Manager) Save(name, description, body string, tags []string) (*Skill, error) { - if len(m.dirs) == 0 { - return nil, errors.New("no skills directory configured") + if existing, ok := m.Get(name); ok && existing.ReadOnly { + return nil, fmt.Errorf("%w: %q", ErrReadOnly, name) } name = sanitizeName(name) if name == "" { return nil, errors.New("skill name is required") } - dir := m.dirs[0] + if existing, ok := m.Get(name); ok && existing.ReadOnly { + return nil, fmt.Errorf("%w: %q", ErrReadOnly, name) + } + dir := m.writeDir() + if dir == "" { + return nil, errors.New("no skills directory configured") + } if err := os.MkdirAll(dir, 0o755); err != nil { return nil, err } @@ -301,7 +278,6 @@ func (m *Manager) Save(name, description, body string, tags []string) (*Skill, e return nil, err } content := "---\n" + string(headerYAML) + "---\n\n" + strings.TrimSpace(body) + "\n" - path := filepath.Join(dir, name+".md") if err := os.WriteFile(path, []byte(content), 0o644); err != nil { return nil, err @@ -313,12 +289,26 @@ func (m *Manager) Save(name, description, body string, tags []string) (*Skill, e return s, nil } -// Delete removes a skill file. +func (m *Manager) writeDir() string { + m.mu.RLock() + defer m.mu.RUnlock() + for _, dir := range m.opts.Dirs { + if strings.TrimSpace(dir) != "" { + return dir + } + } + return "" +} + +// Delete removes a writable skill file. func (m *Manager) Delete(name string) error { s, ok := m.Get(name) if !ok { return fmt.Errorf("skill %q not found", name) } + if s.ReadOnly { + return fmt.Errorf("%w: %q", ErrReadOnly, name) + } if err := os.Remove(s.Path); err != nil { return err } From 3b41c230d083f8f7a2e4509b8e167c72308c8118 Mon Sep 17 00:00:00 2001 From: Reidho Satria Date: Wed, 16 Sep 2026 16:02:08 +0700 Subject: [PATCH 2/9] feat(skills): isolate project catalogs by session --- docs/skills.md | 10 +- internal/agent/prompt.go | 19 ++- internal/agent/skills.go | 39 ++++- internal/agent/skills_test.go | 269 +++++++++++++++++++++++++++++ internal/agent/tool_execution.go | 2 +- internal/commands/handlers.go | 24 +++ internal/commands/skills_test.go | 130 ++++++++++++++ internal/skills/discovery.go | 63 ++++--- internal/skills/scope.go | 284 +++++++++++++++++++++++++++++++ internal/skills/scope_test.go | 281 ++++++++++++++++++++++++++++++ internal/skills/skills.go | 171 +++++++++---------- 11 files changed, 1163 insertions(+), 129 deletions(-) create mode 100644 internal/agent/skills_test.go create mode 100644 internal/commands/skills_test.go create mode 100644 internal/skills/scope.go create mode 100644 internal/skills/scope_test.go diff --git a/docs/skills.md b/docs/skills.md index 4823e6f..fe30c12 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -73,8 +73,10 @@ Antares also discovers these directories automatically, in the order shown: | `.omp/agent/managed-skills` | `.github/skills` | Automatic roots use the OS home independently of `ANTARES_HOME`; the OpenCode -home path does not follow `XDG_CONFIG_HOME`. Project roots are beneath the startup -directory, without searching parent directories. +home path does not follow `XDG_CONFIG_HOME`. Project roots are beneath the chat's +persisted project folder, without searching parent directories. Resumed chats keep +that binding. The dashboard and chats without a project use the startup directory; +relative project paths resolve against that startup directory. Automatic sources accept only `SKILL.md` (case-insensitive), recursively. Supporting Markdown and hidden descendants are ignored. A missing name uses the logical parent @@ -125,6 +127,10 @@ learned it says so and writes nothing. /skills deploy filter ``` +`/skills` uses the current session's project catalog, including for hub-installed +checks. A project session sees shared user/configured skills and its own project +skills, not the startup project's or another chat project's procedures. + The dashboard's Skills page lists them with a switch each, shows the body inline, and has a Browse button for the hub. diff --git a/internal/agent/prompt.go b/internal/agent/prompt.go index 3501d71..5e7ce3d 100644 --- a/internal/agent/prompt.go +++ b/internal/agent/prompt.go @@ -21,11 +21,10 @@ import ( // buildSystemPrompt assembles identity, environment, memory, and tool guidance. func (a *Agent) buildSystemPrompt(ctx context.Context, req Request, sess *store.Session, active []tools.Tool) string { cfg := a.config() - // Snapshot the live-replaceable services once, so a mid-prompt reload - // (SetRAG / SetSkills) cannot leave the nil check disagreeing with the - // use below. + // Snapshot the live-replaceable RAG service once, so a mid-prompt reload + // cannot leave the nil check disagreeing with the use below. The skills + // manager is likewise snapshotted through skillsForSession when enabled. ragProvider := a.RAG() - skillsMgr := a.Skills() var b strings.Builder b.WriteString("You are ") @@ -178,11 +177,13 @@ You are running as a worker for another agent. Nobody is watching your stream. } } - if skillsMgr != nil && cfg.Skills.Enabled { - if catalogue := skillsMgr.PromptBlock(60); catalogue != "" { - b.WriteString("\n## Your skills\n\n") - b.WriteString("Procedures you have learned. Read one with the skill tool before following it.\n\n") - b.WriteString(catalogue) + if cfg.Skills.Enabled { + if skillsMgr := a.skillsForSession(sess); skillsMgr != nil { + if catalogue := skillsMgr.PromptBlock(60); catalogue != "" { + b.WriteString("\n## Your skills\n\n") + b.WriteString("Procedures you have learned. Read one with the skill tool before following it.\n\n") + b.WriteString(catalogue) + } } } diff --git a/internal/agent/skills.go b/internal/agent/skills.go index 8963a30..ed6ba8a 100644 --- a/internal/agent/skills.go +++ b/internal/agent/skills.go @@ -1,7 +1,11 @@ package agent import ( + "log/slog" + "strings" + "github.com/enowdev/antares/internal/skills" + "github.com/enowdev/antares/internal/store" "github.com/enowdev/antares/internal/tools" ) @@ -71,12 +75,37 @@ func (a skillAdapter) Write(name, description, body string, tags []string) error func (a skillAdapter) MarkUsed(name string) { a.m.MarkUsed(name) } -// skillLibrary exposes the manager to tools, or nil when skills are off. -// The manager is snapshotted once so a concurrent SetSkills cannot leave the -// returned adapter pointing at a stale (or nil) library. -func (a *Agent) skillLibrary() tools.SkillLibrary { +// skillsForSession snapshots the live manager and binds its catalogue to the +// persisted project selection. Scope failures are nonfatal: ForProject returns +// the partial scope, or a shared-only view when the path cannot be normalized. +func (a *Agent) skillsForSession(sess *store.Session) *skills.Manager { m := a.Skills() - if m == nil || !a.config().Skills.Enabled { + if m == nil { + return nil + } + + var projectDir string + if sess != nil && sess.Meta != nil { + projectDir, _ = sess.Meta["project_dir"].(string) + } + if strings.TrimSpace(projectDir) == "" { + projectDir = "" + } + scoped, err := m.ForProject(projectDir) + if err != nil { + slog.Warn("some skills failed to load", "error", err) + } + return scoped +} + +// skillLibrary exposes the session's scoped manager to tools, or nil when +// skills are off. +func (a *Agent) skillLibrary(sess *store.Session) tools.SkillLibrary { + if !a.config().Skills.Enabled { + return nil + } + m := a.skillsForSession(sess) + if m == nil { return nil } return skillAdapter{m: m} diff --git a/internal/agent/skills_test.go b/internal/agent/skills_test.go new file mode 100644 index 0000000..af75749 --- /dev/null +++ b/internal/agent/skills_test.go @@ -0,0 +1,269 @@ +package agent + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/llm" + "github.com/enowdev/antares/internal/skills" + "github.com/enowdev/antares/internal/store" + "github.com/enowdev/antares/internal/tools" +) + +func writeSessionSkill(t *testing.T, root, name, description, body string, chains ...string) { + t.Helper() + dir := filepath.Join(root, ".agent", "skills", name) + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + doc := "---\nname: " + name + "\ndescription: " + description + "\nenabled: true\n" + if len(chains) > 0 { + doc += "chains_with:\n" + for _, chain := range chains { + doc += " - " + chain + "\n" + } + } + doc += "---\n\n" + body + "\n" + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte(doc), 0o644); err != nil { + t.Fatal(err) + } +} + +func executeSessionSkill(a *Agent, sess *store.Session, action, name string) (string, error) { + t, ok := tools.Default().Get("skill") + if !ok { + return "", fmt.Errorf("registered skill tool is missing") + } + args, err := json.Marshal(map[string]string{"action": action, "name": name}) + if err != nil { + return "", err + } + outcomes := a.executeTools( + context.Background(), + []llm.ToolCall{{ID: "scope-probe-" + sess.ID, Name: "skill", Arguments: string(args)}}, + map[string]tools.Tool{"skill": t}, + Request{Platform: "web"}, + sess, + func(Event) error { return nil }, + ) + if len(outcomes) != 1 { + return "", fmt.Errorf("skill tool returned %d outcomes", len(outcomes)) + } + if outcomes[0].isError { + return "", fmt.Errorf("skill tool failed: %s", outcomes[0].message.Content) + } + return outcomes[0].message.Content, nil +} + +func requireScopedText(got string, wants, rejects []string) error { + for _, want := range wants { + if !strings.Contains(got, want) { + return fmt.Errorf("missing %q in:\n%s", want, got) + } + } + for _, reject := range rejects { + if strings.Contains(got, reject) { + return fmt.Errorf("unexpected %q in:\n%s", reject, got) + } + } + return nil +} + +func TestSkillSessionIsolation(t *testing.T) { + home := t.TempDir() + t.Setenv("ANTARES_HOME", t.TempDir()) + startup := t.TempDir() + projectA := t.TempDir() + projectB := t.TempDir() + + writeSessionSkill(t, home, "session-shared", "SHARED_DESCRIPTION", "SHARED_BODY") + writeSessionSkill(t, startup, "session-startup-only", "STARTUP_ONLY_DESCRIPTION", "STARTUP_ONLY_BODY") + writeSessionSkill(t, startup, "session-collision", "STARTUP_COLLISION_DESCRIPTION", "STARTUP_COLLISION_BODY", "session-startup-follow") + writeSessionSkill(t, startup, "session-startup-follow", "STARTUP_FOLLOW_DESCRIPTION", "STARTUP_FOLLOW_BODY") + writeSessionSkill(t, projectA, "session-a-only", "A_ONLY_DESCRIPTION", "A_ONLY_BODY") + writeSessionSkill(t, projectA, "session-collision", "A_COLLISION_DESCRIPTION", "A_COLLISION_BODY", "session-a-follow") + writeSessionSkill(t, projectA, "session-a-follow", "A_FOLLOW_DESCRIPTION", "A_FOLLOW_BODY") + writeSessionSkill(t, projectB, "session-b-only", "B_ONLY_DESCRIPTION", "B_ONLY_BODY") + writeSessionSkill(t, projectB, "session-collision", "B_COLLISION_DESCRIPTION", "B_COLLISION_BODY", "session-b-follow") + writeSessionSkill(t, projectB, "session-b-follow", "B_FOLLOW_DESCRIPTION", "B_FOLLOW_BODY") + + manager := skills.NewManager(skills.Options{UserHome: home, ProjectDir: startup}) + if err := manager.Reload(); err != nil { + t.Fatalf("reload skills: %v", err) + } + + a, startupSession := errorAgent(t) + cfg := config.Default() + cfg.Memory.Enabled = false + cfg.Skills.Enabled = true + a.SetConfig(cfg) + a.SetSkills(manager) + a.reg = tools.Default() + + ctx := context.Background() + sessionA := &store.Session{ID: "session-a", Platform: "web", Workspace: projectA, Meta: store.Meta{"project_dir": projectA}} + sessionB := &store.Session{ID: "session-b", Platform: "web", Workspace: projectB, Meta: store.Meta{"project_dir": projectB}} + for _, sess := range []*store.Session{sessionA, sessionB} { + if err := a.db.CreateSession(ctx, sess); err != nil { + t.Fatalf("create %s: %v", sess.ID, err) + } + } + + startupPrompt := a.buildSystemPrompt(ctx, Request{}, startupSession, nil) + if err := requireScopedText(startupPrompt, + []string{"SHARED_DESCRIPTION", "STARTUP_ONLY_DESCRIPTION", "STARTUP_COLLISION_DESCRIPTION"}, + []string{"A_ONLY_DESCRIPTION", "A_COLLISION_DESCRIPTION", "B_ONLY_DESCRIPTION", "B_COLLISION_DESCRIPTION"}, + ); err != nil { + t.Fatalf("startup prompt: %v", err) + } + + resumeReq := Request{SessionID: sessionA.ID, Message: "resume without a project_dir request"} + resumedA, err := a.resolveSession(ctx, &resumeReq) + if err != nil { + t.Fatalf("resume A: %v", err) + } + + promptCases := []struct { + name string + sess *store.Session + want []string + rejects []string + }{ + { + name: "A resumed from persisted metadata", sess: resumedA, + want: []string{"SHARED_DESCRIPTION", "A_ONLY_DESCRIPTION", "A_COLLISION_DESCRIPTION"}, + rejects: []string{"STARTUP_ONLY_DESCRIPTION", "STARTUP_COLLISION_DESCRIPTION", "STARTUP_FOLLOW_DESCRIPTION", "B_ONLY_DESCRIPTION", "B_COLLISION_DESCRIPTION", "B_FOLLOW_DESCRIPTION"}, + }, + { + name: "B", sess: sessionB, + want: []string{"SHARED_DESCRIPTION", "B_ONLY_DESCRIPTION", "B_COLLISION_DESCRIPTION"}, + rejects: []string{"STARTUP_ONLY_DESCRIPTION", "STARTUP_COLLISION_DESCRIPTION", "STARTUP_FOLLOW_DESCRIPTION", "A_ONLY_DESCRIPTION", "A_COLLISION_DESCRIPTION", "A_FOLLOW_DESCRIPTION"}, + }, + } + for _, tc := range promptCases { + t.Run("prompt "+tc.name, func(t *testing.T) { + prompt := a.buildSystemPrompt(ctx, Request{}, tc.sess, nil) + if err := requireScopedText(prompt, tc.want, tc.rejects); err != nil { + t.Fatal(err) + } + }) + } + + toolCases := []struct { + name string + sess *store.Session + onlyDescription string + collisionDesc string + collisionBody string + followName string + foreignFragments []string + }{ + { + name: "A", sess: resumedA, onlyDescription: "A_ONLY_DESCRIPTION", + collisionDesc: "A_COLLISION_DESCRIPTION", collisionBody: "A_COLLISION_BODY", followName: "session-a-follow", + foreignFragments: []string{"STARTUP_ONLY_DESCRIPTION", "STARTUP_COLLISION_DESCRIPTION", "STARTUP_COLLISION_BODY", "session-startup-follow", "B_ONLY_DESCRIPTION", "B_COLLISION_DESCRIPTION", "B_COLLISION_BODY", "session-b-follow"}, + }, + { + name: "B", sess: sessionB, onlyDescription: "B_ONLY_DESCRIPTION", + collisionDesc: "B_COLLISION_DESCRIPTION", collisionBody: "B_COLLISION_BODY", followName: "session-b-follow", + foreignFragments: []string{"STARTUP_ONLY_DESCRIPTION", "STARTUP_COLLISION_DESCRIPTION", "STARTUP_COLLISION_BODY", "session-startup-follow", "A_ONLY_DESCRIPTION", "A_COLLISION_DESCRIPTION", "A_COLLISION_BODY", "session-a-follow"}, + }, + } + for _, tc := range toolCases { + t.Run("tool "+tc.name, func(t *testing.T) { + checks := []struct { + action string + name string + wants []string + }{ + {action: "list", wants: []string{"SHARED_DESCRIPTION", tc.onlyDescription, tc.collisionDesc}}, + {action: "search", name: "session-collision", wants: []string{tc.collisionDesc}}, + {action: "read", name: "session-collision", wants: []string{tc.collisionDesc, tc.collisionBody}}, + {action: "read", name: "session-shared", wants: []string{"SHARED_DESCRIPTION", "SHARED_BODY"}}, + {action: "chains", name: "session-collision", wants: []string{tc.followName}}, + } + for _, check := range checks { + got, err := executeSessionSkill(a, tc.sess, check.action, check.name) + if err != nil { + t.Fatalf("%s: %v", check.action, err) + } + if err := requireScopedText(got, check.wants, tc.foreignFragments); err != nil { + t.Fatalf("%s: %v", check.action, err) + } + } + }) + } + + for i, sess := range []*store.Session{ + nil, + {Meta: nil}, + {Meta: store.Meta{}}, + {Meta: store.Meta{"project_dir": 42}}, + {Meta: store.Meta{"project_dir": " \t "}}, + } { + scoped := a.skillsForSession(sess) + if scoped == nil { + t.Fatalf("default scope case %d returned nil", i) + } + if _, ok := scoped.Get("session-startup-only"); !ok { + t.Fatalf("default scope case %d did not select startup catalogue", i) + } + } + + invalid := &store.Session{Meta: store.Meta{"project_dir": "invalid\x00project"}} + partial := a.skillsForSession(invalid) + if partial == nil { + t.Fatal("invalid project binding returned nil instead of a partial safe view") + } + if _, ok := partial.Get("session-shared"); !ok { + t.Fatal("invalid project binding hid the shared user catalogue") + } + if _, ok := partial.Get("session-startup-only"); ok { + t.Fatal("invalid project binding leaked the startup project catalogue") + } + + start := make(chan struct{}) + errCh := make(chan error, 32) + var wg sync.WaitGroup + for i := 0; i < cap(errCh); i++ { + sess := resumedA + bodyWant, bodyReject := "A_COLLISION_BODY", "B_COLLISION_BODY" + descriptionWant, descriptionReject := "A_COLLISION_DESCRIPTION", "B_COLLISION_DESCRIPTION" + if i%2 == 1 { + sess = sessionB + bodyWant, bodyReject = "B_COLLISION_BODY", "A_COLLISION_BODY" + descriptionWant, descriptionReject = "B_COLLISION_DESCRIPTION", "A_COLLISION_DESCRIPTION" + } + wg.Add(1) + go func(sess *store.Session, bodyWant, bodyReject, descriptionWant, descriptionReject string) { + defer wg.Done() + <-start + prompt := a.buildSystemPrompt(ctx, Request{}, sess, nil) + if err := requireScopedText(prompt, []string{descriptionWant}, []string{descriptionReject}); err != nil { + errCh <- fmt.Errorf("concurrent prompt: %w", err) + return + } + got, err := executeSessionSkill(a, sess, "read", "session-collision") + if err != nil { + errCh <- err + return + } + if err := requireScopedText(got, []string{bodyWant}, []string{bodyReject}); err != nil { + errCh <- fmt.Errorf("concurrent read: %w", err) + } + }(sess, bodyWant, bodyReject, descriptionWant, descriptionReject) + } + close(start) + wg.Wait() + close(errCh) + for err := range errCh { + t.Error(err) + } +} diff --git a/internal/agent/tool_execution.go b/internal/agent/tool_execution.go index 90311ff..8d89d98 100644 --- a/internal/agent/tool_execution.go +++ b/internal/agent/tool_execution.go @@ -202,7 +202,7 @@ func (a *Agent) executeTools( AskUser: a.askBridge(sess.ID, safeEmit), Deps: &tools.Deps{ Config: a.config(), Store: a.db, RAG: ragProvider, Shell: a.shell, - Sub: a.subAgentFor(req), Tasks: a.backgroundFor(req), Skills: a.skillLibrary(), + Sub: a.subAgentFor(req), Tasks: a.backgroundFor(req), Skills: a.skillLibrary(sess), SocialBrowser: a.socialBrowser, Checkpoint: func(sessionID, path, tool string) { a.saveCheckpoint(sessionID, path, tool, req.turnMarker) diff --git a/internal/commands/handlers.go b/internal/commands/handlers.go index e8b1fc6..7904930 100644 --- a/internal/commands/handlers.go +++ b/internal/commands/handlers.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "sort" "strconv" "strings" @@ -225,9 +226,32 @@ func cmdToolset(_ context.Context, d Deps, in Input) (Result, error) { } func cmdSkills(ctx context.Context, d Deps, in Input) (Result, error) { + if d.Agent != nil { + if mgr := d.Agent.Skills(); mgr != nil { + d.Skills = mgr + } + } + var projectDir string + if in.SessionID != "" { + if d.Store == nil { + return Result{}, errNoStore + } + sess, err := d.Store.GetSession(ctx, in.SessionID) + if err != nil { + return Result{}, err + } + if sess != nil { + projectDir, _ = sess.Meta["project_dir"].(string) + } + } if d.Skills == nil { return Result{}, errNoSkills } + var err error + d.Skills, err = d.Skills.ForProject(projectDir) + if err != nil { + slog.Warn("some skills failed to load", "error", err) + } // "/skills search foo" and "/skills install id" reach the hub; anything // else lists what is already installed. if verb, rest, _ := strings.Cut(in.Args, " "); verb == "search" || verb == "browse" { diff --git a/internal/commands/skills_test.go b/internal/commands/skills_test.go new file mode 100644 index 0000000..960b79e --- /dev/null +++ b/internal/commands/skills_test.go @@ -0,0 +1,130 @@ +package commands + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/enowdev/antares/internal/agent" + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/skills" + "github.com/enowdev/antares/internal/store" +) + +func TestSkillCommandSessionIsolation(t *testing.T) { + ctx := context.Background() + base := t.TempDir() + home, startup := filepath.Join(base, "home"), filepath.Join(base, "startup") + projects := map[string]string{"S": startup, "A": filepath.Join(base, "a"), "B": filepath.Join(base, "b")} + write := func(root, name, description string) { + t.Helper() + path := filepath.Join(root, ".agent", "skills", name, "SKILL.md") + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("---\nname: "+name+"\ndescription: "+description+"\n---\nbody\n"), 0o644); err != nil { + t.Fatal(err) + } + } + write(home, "scope-global", "GLOBAL_MARKER") + for id, root := range projects { + write(root, "scope-"+strings.ToLower(id)+"-only", id+"_ONLY_MARKER") + write(root, "scope-collision", id+"_COLLISION_MARKER") + } + mgr := skills.NewManager(skills.Options{UserHome: home, ProjectDir: startup}) + if err := mgr.Reload(); err != nil { + t.Fatal(err) + } + db, err := store.Open(ctx, "memory", "", 1, 5000, false) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { db.Close() }) + a := agent.New(config.Default(), db, nil, nil, nil) + a.SetSkills(mgr) + // A stale fallback must not override the live agent manager. + fallback := skills.NewManager(skills.Options{}) + deps := Deps{Agent: a, Skills: fallback, Store: db} + for id, root := range projects { + if err := db.CreateSession(ctx, &store.Session{ID: id, Meta: store.Meta{"project_dir": root}}); err != nil { + t.Fatal(err) + } + } + for id, value := range map[string]any{"blank": " ", "nonstring": 42, "missing": nil} { + if err := db.CreateSession(ctx, &store.Session{ID: id, Meta: store.Meta{"project_dir": value}}); err != nil { + t.Fatal(err) + } + } + check := func(sessionID, wanted string) { + t.Helper() + result, err := Run(ctx, deps, Input{Name: "skills", Args: "scope-", SessionID: sessionID, Surface: "web"}) + if err != nil { + t.Errorf("session %s: %v", sessionID, err) + return + } + for _, marker := range []string{"GLOBAL_MARKER", wanted + "_ONLY_MARKER", wanted + "_COLLISION_MARKER"} { + if !strings.Contains(result.Output, marker) { + t.Errorf("session %s missing %s: %s", sessionID, marker, result.Output) + } + } + for _, other := range []string{"S", "A", "B"} { + if other != wanted && (strings.Contains(result.Output, other+"_ONLY_MARKER") || strings.Contains(result.Output, other+"_COLLISION_MARKER")) { + t.Errorf("session %s leaked %s catalog: %s", sessionID, other, result.Output) + } + } + } + for _, id := range []string{"", "S", "blank", "nonstring", "missing"} { + check(id, "S") + } + check("A", "A") + check("B", "B") + check("", "S") + var wg sync.WaitGroup + for _, id := range []string{"A", "B"} { + wg.Go(func() { + for range 10 { + check(id, id) + } + }) + } + wg.Wait() + if _, err := Run(ctx, Deps{Skills: mgr}, Input{Name: "skills", SessionID: "A"}); !errors.Is(err, errNoStore) { + t.Fatalf("missing store = %v, want errNoStore", err) + } + if _, err := Run(ctx, deps, Input{Name: "skills", SessionID: "absent"}); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("missing session = %v, want ErrNotFound", err) + } + // Malformed project content must not leak the startup catalog or hide valid entries. + write(projects["A"], "scope-partial", "A_PARTIAL_MARKER") + bad := filepath.Join(projects["A"], ".agent", "skills", "bad", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(bad), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(bad, []byte("---\nname: [\n---\nbad"), 0o644); err != nil { + t.Fatal(err) + } + if err := mgr.Reload(); err == nil { + t.Fatal("malformed skill did not report error") + } + check("A", "A") + result, err := Run(ctx, deps, Input{Name: "skills", Args: "scope-partial", SessionID: "A"}) + if err != nil || !strings.Contains(result.Output, "A_PARTIAL_MARKER") { + t.Fatalf("partial catalog: %+v, %v", result, err) + } + if err := db.CreateSession(ctx, &store.Session{ID: "invalid", Meta: store.Meta{"project_dir": "bad\x00path"}}); err != nil { + t.Fatal(err) + } + result, err = Run(ctx, deps, Input{Name: "skills", Args: "scope-", SessionID: "invalid"}) + if err != nil || !strings.Contains(result.Output, "GLOBAL_MARKER") { + t.Fatalf("shared-only catalog: %+v, %v", result, err) + } + for _, id := range []string{"S", "A", "B"} { + if strings.Contains(result.Output, id+"_ONLY_MARKER") || strings.Contains(result.Output, id+"_COLLISION_MARKER") { + t.Fatalf("normalization failure leaked project %s: %s", id, result.Output) + } + } +} diff --git a/internal/skills/discovery.go b/internal/skills/discovery.go index 12a136a..04282ec 100644 --- a/internal/skills/discovery.go +++ b/internal/skills/discovery.go @@ -40,13 +40,46 @@ var projectSkillRoots = [][]string{ {".github", "skills"}, } -// discover scans roots from low to high priority. Later occurrences of a name -// replace earlier ones, while malformed entries leave the rest of the freshly -// discovered catalogue available. -func discover(opts Options) (map[string]*Skill, error) { +// discoverShared scans each shared source kind independently. Lower-priority +// entries remain in their layer so removing an override reveals them later. +func discoverShared(opts Options) (bundled, user, configured map[string]*Skill, firstErr error) { + bundled, err := discoverRoots(rootsForPaths(opts.PackDirs, sourcePack)) + firstErr = err + userRoots := make([]string, 0, len(userSkillRoots)) + if strings.TrimSpace(opts.UserHome) != "" { + for _, parts := range userSkillRoots { + userRoots = append(userRoots, filepath.Join(append([]string{opts.UserHome}, parts...)...)) + } + } + user, err = discoverRoots(rootsForPaths(userRoots, sourceUser)) + if firstErr == nil { + firstErr = err + } + configured, err = discoverRoots(rootsForPaths(opts.Dirs, sourceConfigured)) + if firstErr == nil { + firstErr = err + } + return bundled, user, configured, firstErr +} + +// discoverProject scans only the conventional roots beneath one normalized +// logical project directory. +func discoverProject(projectDir string) (map[string]*Skill, error) { + paths := make([]string, 0, len(projectSkillRoots)) + for _, parts := range projectSkillRoots { + paths = append(paths, filepath.Join(append([]string{projectDir}, parts...)...)) + } + return discoverRoots(rootsForPaths(paths, sourceProject)) +} + +func rootsForPaths(paths []string, kind sourceKind) []sourceRoot { + return appendSourceRoots(nil, paths, kind) +} + +func discoverRoots(roots []sourceRoot) (map[string]*Skill, error) { found := make(map[string]*Skill) var firstErr error - for _, root := range discoveryRoots(opts) { + for _, root := range roots { if err := scanRoot(root, func(skill *Skill) { found[skill.Name] = skill }); err != nil && firstErr == nil { @@ -56,26 +89,6 @@ func discover(opts Options) (map[string]*Skill, error) { return found, firstErr } -func discoveryRoots(opts Options) []sourceRoot { - roots := make([]sourceRoot, 0, len(opts.PackDirs)+len(opts.Dirs)+12) - roots = appendSourceRoots(roots, opts.PackDirs, sourcePack) - if strings.TrimSpace(opts.UserHome) != "" { - paths := make([]string, 0, len(userSkillRoots)) - for _, parts := range userSkillRoots { - paths = append(paths, filepath.Join(append([]string{opts.UserHome}, parts...)...)) - } - roots = appendSourceRoots(roots, paths, sourceUser) - } - if strings.TrimSpace(opts.ProjectDir) != "" { - paths := make([]string, 0, len(projectSkillRoots)) - for _, parts := range projectSkillRoots { - paths = append(paths, filepath.Join(append([]string{opts.ProjectDir}, parts...)...)) - } - roots = appendSourceRoots(roots, paths, sourceProject) - } - return appendSourceRoots(roots, opts.Dirs, sourceConfigured) -} - // appendSourceRoots drops equivalent roots only within one source kind. Walking // backwards preserves the last, highest-priority spelling of each root. func appendSourceRoots(dst []sourceRoot, paths []string, kind sourceKind) []sourceRoot { diff --git a/internal/skills/scope.go b/internal/skills/scope.go new file mode 100644 index 0000000..9dc83c3 --- /dev/null +++ b/internal/skills/scope.go @@ -0,0 +1,284 @@ +package skills + +import ( + "fmt" + "path/filepath" + "sort" + "strings" + "sync" +) + +// managerState owns the immutable source snapshots shared by every scoped +// Manager handle. Project maps contain only project-source entries; effective +// catalogues are resolved while holding mu rather than copied per scope. +type managerState struct { + mu sync.RWMutex + scanMu sync.Mutex + + opts Options + + bundled map[string]*Skill + user map[string]*Skill + configured map[string]*Skill + projects map[string]map[string]*Skill + projectErrs map[string]error + scopes map[string]*Manager + usage map[string]int + root *Manager + sharedOnly *Manager + + // startupDir is the normalized logical startup project directory used both + // by the root handle and as the base for relative session project paths. + startupDir string + defaultErr error + sharedErr error +} + +// Manager is a lightweight view over shared skill source snapshots. An empty +// projectDir denotes the root handle, whose selected project follows the +// state's default. A bound handle keeps its logical project path for life. +type Manager struct { + state *managerState + projectDir string + sharedOnly bool +} + +// NewManager builds a manager over independent writable, bundled, user, and +// project sources. Options are cloned so caller mutations cannot reconfigure it. +func NewManager(opts Options) *Manager { + opts = cloneOptions(opts) + startupDir, defaultErr := normalizeStartupProject(opts.ProjectDir) + opts.ProjectDir = startupDir + state := &managerState{ + opts: opts, + bundled: map[string]*Skill{}, + user: map[string]*Skill{}, + configured: map[string]*Skill{}, + projects: map[string]map[string]*Skill{}, + projectErrs: map[string]error{}, + scopes: map[string]*Manager{}, + usage: map[string]int{}, + startupDir: startupDir, + defaultErr: defaultErr, + } + state.root = &Manager{state: state} + state.sharedOnly = &Manager{state: state, sharedOnly: true} + return state.root +} + +func cloneOptions(opts Options) Options { + opts.Dirs = append([]string(nil), opts.Dirs...) + opts.PackDirs = append([]string(nil), opts.PackDirs...) + return opts +} + +func normalizeStartupProject(projectDir string) (string, error) { + if strings.TrimSpace(projectDir) == "" { + return "", nil + } + if strings.IndexByte(projectDir, 0) >= 0 { + return "", fmt.Errorf("normalize startup project directory: path contains NUL") + } + logical, err := filepath.Abs(projectDir) + if err != nil { + return "", fmt.Errorf("normalize startup project directory %q: %w", projectDir, err) + } + return filepath.Clean(logical), nil +} + +// ForProject returns a lightweight catalogue view bound to projectDir. Relative +// paths resolve against the captured startup project. A normalization failure +// returns a shared-only view so callers never accidentally observe the startup +// or another project's skills. +func (m *Manager) ForProject(projectDir string) (*Manager, error) { + if m == nil { + return nil, nil + } + if strings.TrimSpace(projectDir) == "" { + m.state.mu.RLock() + root := m.state.root + err := m.state.defaultErr + if err == nil { + err = m.state.sharedErr + } + if err == nil && m.state.startupDir != "" { + err = m.state.projectErrs[m.state.startupDir] + } + m.state.mu.RUnlock() + return root, err + } + + logical, err := m.normalizeProject(projectDir) + if err != nil { + return m.state.sharedOnly, err + } + + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() + m.state.mu.RLock() + view, registered := m.state.scopes[logical] + registeredErr := m.state.sharedErr + if registeredErr == nil { + registeredErr = m.state.projectErrs[logical] + } + m.state.mu.RUnlock() + if registered { + return view, registeredErr + } + + view = &Manager{state: m.state, projectDir: logical} + found, scanErr := discoverProject(logical) + m.state.mu.Lock() + m.state.projects[logical] = found + m.state.projectErrs[logical] = scanErr + m.state.scopes[logical] = view + sharedErr := m.state.sharedErr + m.state.mu.Unlock() + if sharedErr != nil { + return view, sharedErr + } + return view, scanErr +} + +func (m *Manager) normalizeProject(projectDir string) (string, error) { + if strings.IndexByte(projectDir, 0) >= 0 { + return "", fmt.Errorf("normalize project directory: path contains NUL") + } + if filepath.IsAbs(projectDir) { + return filepath.Clean(projectDir), nil + } + m.state.mu.RLock() + startupDir := m.state.startupDir + m.state.mu.RUnlock() + if startupDir == "" { + return "", fmt.Errorf("normalize relative project directory %q: startup project directory is unavailable", projectDir) + } + return filepath.Clean(filepath.Join(startupDir, projectDir)), nil +} + +// Reload rescans shared sources once and every registered logical project. All +// successful partial snapshots are published atomically; usage counters remain +// shared and name-keyed. +func (m *Manager) Reload() error { + if m == nil { + return nil + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() + return m.reloadLocked() +} + +// reloadLocked requires scanMu. Mutation methods use it after writing so they +// do not recursively acquire the scan lock. +func (m *Manager) reloadLocked() error { + state := m.state + state.mu.RLock() + opts := cloneOptions(state.opts) + projectDirs := make([]string, 0, len(state.projects)) + for projectDir := range state.projects { + projectDirs = append(projectDirs, projectDir) + } + startupDir := state.startupDir + defaultErr := state.defaultErr + state.mu.RUnlock() + if startupDir != "" { + registered := false + for _, projectDir := range projectDirs { + if projectDir == startupDir { + registered = true + break + } + } + if !registered { + projectDirs = append(projectDirs, startupDir) + } + } + sort.Strings(projectDirs) + + bundled, user, configured, sharedErr := discoverShared(opts) + projects := make(map[string]map[string]*Skill, len(projectDirs)) + projectErrs := make(map[string]error, len(projectDirs)) + firstErr := defaultErr + if firstErr == nil { + firstErr = sharedErr + } + for _, projectDir := range projectDirs { + found, err := discoverProject(projectDir) + projects[projectDir] = found + projectErrs[projectDir] = err + if firstErr == nil && err != nil { + firstErr = err + } + } + + state.mu.Lock() + state.bundled = bundled + state.user = user + state.configured = configured + state.projects = projects + state.projectErrs = projectErrs + state.sharedErr = sharedErr + state.mu.Unlock() + return firstErr +} + +func (m *Manager) selectedProjectLocked() map[string]*Skill { + if m.sharedOnly { + return nil + } + projectDir := m.projectDir + if projectDir == "" { + projectDir = m.state.startupDir + } + if projectDir == "" { + return nil + } + return m.state.projects[projectDir] +} + +func (m *Manager) effectiveSkillLocked(name string) (*Skill, bool) { + if skill, ok := m.state.configured[name]; ok { + return skill, true + } + if skill, ok := m.selectedProjectLocked()[name]; ok { + return skill, true + } + if skill, ok := m.state.user[name]; ok { + return skill, true + } + skill, ok := m.state.bundled[name] + return skill, ok +} + +func (m *Manager) effectiveSkillsLocked(yield func(*Skill) bool) { + layers := [4]map[string]*Skill{m.state.configured, m.selectedProjectLocked(), m.state.user, m.state.bundled} + for i, layer := range layers { + for name, skill := range layer { + shadowed := false + for _, higher := range layers[:i] { + if _, exists := higher[name]; exists { + shadowed = true + break + } + } + if !shadowed && !yield(skill) { + return + } + } + } +} + +func (m *Manager) effectiveListLocked() []Skill { + capacity := len(m.state.configured) + len(m.state.user) + len(m.state.bundled) + len(m.selectedProjectLocked()) + out := make([]Skill, 0, capacity) + for skill := range m.effectiveSkillsLocked { + out = append(out, cloneSkillWithUsage(skill, m.state.usage[skill.Name])) + } + return out +} + +func cloneSkillWithUsage(skill *Skill, usage int) Skill { + clone := cloneSkill(skill) + clone.UsageCount = usage + return clone +} diff --git a/internal/skills/scope_test.go b/internal/skills/scope_test.go new file mode 100644 index 0000000..0985ce7 --- /dev/null +++ b/internal/skills/scope_test.go @@ -0,0 +1,281 @@ +package skills + +import ( + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" +) + +func writeProjectSkill(t *testing.T, projectDir, name, body string) string { + t.Helper() + path := filepath.Join(projectDir, ".agent", "skills", name, "SKILL.md") + writeFile(t, path, skillDocument(name, name+" description", body, true)) + return path +} + +func TestProjectScopeIsolationAndPrecedence(t *testing.T) { + base := t.TempDir() + home := filepath.Join(base, "home") + startup := filepath.Join(base, "startup") + projectA := filepath.Join(base, "project-a") + projectB := filepath.Join(base, "project-b") + configured := filepath.Join(base, "configured") + pack := filepath.Join(base, "pack") + + writeFile(t, filepath.Join(home, ".agent", "skills", "global", "SKILL.md"), skillDocument("global", "global", "GLOBAL", true)) + writeFile(t, filepath.Join(pack, "pack-only.md"), skillDocument("pack-only", "pack", "PACK", true)) + writeFile(t, filepath.Join(pack, "collision.md"), skillDocument("collision", "pack", "PACK_COLLISION", true)) + writeProjectSkill(t, startup, "startup-only", "STARTUP") + writeProjectSkill(t, startup, "collision", "STARTUP_COLLISION") + writeProjectSkill(t, projectA, "a-only", "A_ONLY") + writeProjectSkill(t, projectA, "collision", "A_COLLISION") + writeProjectSkill(t, projectB, "b-only", "B_ONLY") + writeProjectSkill(t, projectB, "collision", "B_COLLISION") + + manager := NewManager(Options{Dirs: []string{configured}, PackDirs: []string{pack}, UserHome: home, ProjectDir: startup}) + mustReload(t, manager) + a, err := manager.ForProject(projectA) + if err != nil { + t.Fatal(err) + } + b, err := manager.ForProject(projectB) + if err != nil { + t.Fatal(err) + } + relative, err := manager.ForProject("../project-a") + if err != nil { + t.Fatal(err) + } + + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b, "relative-a": relative} { + if got := requireSkill(t, scoped, "global"); got.Body != "GLOBAL" { + t.Fatalf("%s global body = %q", label, got.Body) + } + if got := requireSkill(t, scoped, "pack-only"); got.Body != "PACK" || !got.Pack { + t.Fatalf("%s pack skill = %+v", label, got) + } + } + if got := requireSkill(t, manager, "collision"); got.Body != "STARTUP_COLLISION" { + t.Fatalf("startup collision = %q", got.Body) + } + if got := requireSkill(t, a, "collision"); got.Body != "A_COLLISION" { + t.Fatalf("A collision = %q", got.Body) + } + if got := requireSkill(t, b, "collision"); got.Body != "B_COLLISION" { + t.Fatalf("B collision = %q", got.Body) + } + if got := requireSkill(t, relative, "collision"); got.Body != "A_COLLISION" { + t.Fatalf("relative A collision = %q", got.Body) + } + search := a.Search("collision", 1) + if len(search) != 1 || search[0].Body != "A_COLLISION" { + t.Fatalf("A search winner = %+v", search) + } + if got := a.Count(); got != len(a.List()) { + t.Fatalf("A enabled count = %d, list length = %d", got, len(a.List())) + } + var readers sync.WaitGroup + for range 8 { + readers.Add(2) + go func() { + defer readers.Done() + got, ok := a.Get("collision") + if !ok || got.Body != "A_COLLISION" { + t.Errorf("concurrent A collision = %+v, present=%v", got, ok) + } + }() + go func() { + defer readers.Done() + got, ok := b.Get("collision") + if !ok || got.Body != "B_COLLISION" { + t.Errorf("concurrent B collision = %+v, present=%v", got, ok) + } + }() + } + readers.Wait() + for _, check := range []struct { + scope *Manager + absent string + }{{manager, "a-only"}, {manager, "b-only"}, {a, "startup-only"}, {a, "b-only"}, {b, "startup-only"}, {b, "a-only"}} { + if _, ok := check.scope.Get(check.absent); ok { + t.Fatalf("scope unexpectedly exposed %q", check.absent) + } + } + + writeFile(t, filepath.Join(configured, "collision.md"), skillDocument("collision", "configured", "CONFIGURED_COLLISION", true)) + mustReload(t, a) + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b} { + if got := requireSkill(t, scoped, "collision"); got.Body != "CONFIGURED_COLLISION" || got.ReadOnly || got.Pack { + t.Fatalf("%s configured winner = %+v", label, got) + } + } + if err := os.Remove(filepath.Join(configured, "collision.md")); err != nil { + t.Fatal(err) + } + mustReload(t, b) + if requireSkill(t, a, "collision").Body != "A_COLLISION" || requireSkill(t, b, "collision").Body != "B_COLLISION" || requireSkill(t, manager, "collision").Body != "STARTUP_COLLISION" { + t.Fatal("removing configured override did not reveal each scope's project layer") + } + + saved, err := a.Save("shared saved", "saved", "SAVED_BODY", []string{"saved"}) + if err != nil { + t.Fatal(err) + } + if saved.Path != filepath.Join(configured, "shared-saved.md") { + t.Fatalf("scoped Save path = %q", saved.Path) + } + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b} { + if got := requireSkill(t, scoped, "shared-saved"); got.Body != "SAVED_BODY" { + t.Fatalf("%s did not observe scoped Save: %+v", label, got) + } + } + if err := b.SetEnabled("shared-saved", false); err != nil { + t.Fatal(err) + } + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b} { + if requireSkill(t, scoped, "shared-saved").Enabled { + t.Fatalf("%s did not observe scoped toggle", label) + } + } + if err := manager.Delete("shared-saved"); err != nil { + t.Fatal(err) + } + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b} { + if _, ok := scoped.Get("shared-saved"); ok { + t.Fatalf("%s retained scoped deletion", label) + } + } +} + +func TestProjectScopeErrorsPartialAndSharedOnly(t *testing.T) { + base := t.TempDir() + home := filepath.Join(base, "home") + configured := filepath.Join(base, "configured") + project := filepath.Join(base, "partial") + writeFile(t, filepath.Join(home, ".agent", "skills", "global", "SKILL.md"), skillDocument("global", "global", "GLOBAL", true)) + writeFile(t, filepath.Join(configured, "configured.md"), skillDocument("configured", "configured", "CONFIGURED", true)) + writeProjectSkill(t, project, "valid", "VALID") + writeFile(t, filepath.Join(project, ".agent", "skills", "broken", "SKILL.md"), "---\nname: [broken\n---\nbody") + + manager := NewManager(Options{Dirs: []string{configured}, UserHome: home}) + mustReload(t, manager) + partial, err := manager.ForProject(project) + if err == nil || !strings.Contains(err.Error(), "invalid front matter") { + t.Fatalf("partial scope error = %v", err) + } + if requireSkill(t, partial, "valid").Body != "VALID" { + t.Fatal("valid project skill was hidden by malformed neighbor") + } + for _, name := range []string{"global", "configured"} { + requireSkill(t, partial, name) + } + + sharedOnly, err := manager.ForProject("relative-project") + if err == nil || !strings.Contains(err.Error(), "startup project directory is unavailable") { + t.Fatalf("relative scope error = %v", err) + } + for _, name := range []string{"global", "configured"} { + requireSkill(t, sharedOnly, name) + } + if _, ok := sharedOnly.Get("valid"); ok { + t.Fatal("normalization fallback leaked another project's skill") + } + malformed, err := manager.ForProject("bad\x00path") + if err == nil { + t.Fatal("NUL project path unexpectedly normalized") + } + if _, ok := malformed.Get("valid"); ok { + t.Fatal("malformed path fallback leaked registered project skills") + } + + var nilManager *Manager + if got, err := nilManager.ForProject(project); got != nil || err != nil { + t.Fatalf("nil manager ForProject = %#v, %v", got, err) + } +} + +func TestProjectScopeReloadAliasUsageChainsAndCopies(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink retarget coverage is exercised on Unix") + } + base := t.TempDir() + startup := filepath.Join(base, "startup") + alias := filepath.Join(base, "alias") + targetA := filepath.Join(base, "target-a") + targetB := filepath.Join(base, "target-b") + pack := filepath.Join(base, "pack") + + writeProjectSkill(t, targetA, "collision", "ALIAS_A") + writeProjectSkill(t, targetB, "collision", "ALIAS_B") + writeProjectSkill(t, targetA, "chain-start", "CHAIN_A") + chainPath := filepath.Join(targetA, ".agent", "skills", "chain-start", "SKILL.md") + writeFile(t, chainPath, strings.Replace(skillDocument("chain-start", "chain", "CHAIN_A", true), "chains_with: [next]", "chains_with: [collision]", 1)) + writeFile(t, filepath.Join(pack, "collision.md"), skillDocument("collision", "pack", "PACK_COLLISION", true)) + if err := os.Symlink(targetA, alias); err != nil { + t.Skipf("directory symlinks unavailable: %v", err) + } + + manager := NewManager(Options{PackDirs: []string{pack}, ProjectDir: startup}) + mustReload(t, manager) + scoped, err := manager.ForProject(alias) + if err != nil { + t.Fatal(err) + } + if categories := scoped.Categories(); len(categories) != 0 { + t.Fatalf("project collision should hide bundled categories: %+v", categories) + } + manager.MarkUsed("collision") + got := requireSkill(t, scoped, "collision") + if got.Body != "ALIAS_A" || got.UsageCount != 1 || got.Pack { + t.Fatalf("alias A effective skill = %+v", got) + } + if got.Path != filepath.Join(alias, ".agent", "skills", "collision", "SKILL.md") { + t.Fatalf("alias scope path = %q, want logical alias path", got.Path) + } + chains := scoped.Chains("chain-start") + if len(chains) != 1 || chains[0].Body != "ALIAS_A" || chains[0].UsageCount != 1 { + t.Fatalf("scoped chain resolution = %+v", chains) + } + if scoped.PackCount() != 0 { + t.Fatalf("project collision should hide effective pack count, got %d", scoped.PackCount()) + } + library, total := scoped.Library("", 0, 10) + if total != 0 || len(library) != 0 { + t.Fatalf("project collision should hide pack browsing entry: total=%d items=%+v", total, library) + } + + listed := scoped.List() + for i := range listed { + if listed[i].Name == "collision" { + listed[i].Tags[0] = "caller-mutated" + listed[i].ChainsWith = append(listed[i].ChainsWith, "caller-mutated") + } + } + if got := requireSkill(t, scoped, "collision"); got.Tags[0] != "discovery" || len(got.ChainsWith) != 1 || got.ChainsWith[0] != "next" { + t.Fatalf("List slice mutation leaked into shared snapshot: %+v", got) + } + + if err := os.Remove(alias); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetB, alias); err != nil { + t.Fatal(err) + } + mustReload(t, manager) + if got := requireSkill(t, scoped, "collision"); got.Body != "ALIAS_B" || got.UsageCount != 1 { + t.Fatalf("retargeted bound scope = %+v", got) + } + if _, ok := scoped.Get("chain-start"); ok { + t.Fatal("bound alias retained a skill from its previous target") + } + root, err := manager.ForProject("") + if err != nil { + t.Fatal(err) + } + if got := requireSkill(t, root, "collision"); got.Body != "PACK_COLLISION" { + t.Fatalf("startup collision = %q, want bundled fallback", got.Body) + } +} diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 10c77d8..3d22fef 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -9,7 +9,6 @@ import ( "path/filepath" "sort" "strings" - "sync" "time" "gopkg.in/yaml.v3" @@ -67,46 +66,9 @@ type Options struct { ProjectDir string } -// Manager loads and caches skills from configured and discovered directories. -type Manager struct { - mu sync.RWMutex - opts Options - skills map[string]*Skill - usage map[string]int -} - // ErrReadOnly is returned when a mutation targets an imported skill. var ErrReadOnly = errors.New("automatically discovered skills are read-only") -// NewManager builds a manager over independent writable, bundled, user, and -// project sources. Options are cloned so caller mutations cannot reconfigure it. -func NewManager(opts Options) *Manager { - return &Manager{opts: cloneOptions(opts), skills: map[string]*Skill{}, usage: map[string]int{}} -} - -func cloneOptions(opts Options) Options { - opts.Dirs = append([]string(nil), opts.Dirs...) - opts.PackDirs = append([]string(nil), opts.PackDirs...) - return opts -} - -// Reload rescans every source and atomically publishes all successfully parsed -// entries. A malformed entry does not hide valid entries from the same scan. -func (m *Manager) Reload() error { - m.mu.RLock() - opts := cloneOptions(m.opts) - m.mu.RUnlock() - - found, firstErr := discover(opts) - m.mu.Lock() - for name, sk := range found { - sk.UsageCount = m.usage[name] - } - m.skills = found - m.mu.Unlock() - return firstErr -} - // parseFile reads one skill file, tolerating a missing front matter block. Its // result is source-neutral: the scanner attaches logical path, fallback name, // modification time, and provenance for each occurrence. @@ -168,27 +130,30 @@ func (m *Manager) Everyday() []Skill { return kept } -// List returns all known skills, sorted by name. +// List returns all effective skills for this scope, sorted by name. func (m *Manager) List() []Skill { - m.mu.RLock() - defer m.mu.RUnlock() - out := make([]Skill, 0, len(m.skills)) - for _, s := range m.skills { - out = append(out, cloneSkill(s)) + if m == nil { + return nil } + m.state.mu.RLock() + defer m.state.mu.RUnlock() + out := m.effectiveListLocked() sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) return out } -// Get returns one skill by name. +// Get returns one effective skill by name. func (m *Manager) Get(name string) (*Skill, bool) { - m.mu.RLock() - defer m.mu.RUnlock() - s, ok := m.skills[name] + if m == nil { + return nil, false + } + m.state.mu.RLock() + defer m.state.mu.RUnlock() + skill, ok := m.effectiveSkillLocked(name) if !ok { return nil, false } - clone := cloneSkill(s) + clone := cloneSkillWithUsage(skill, m.state.usage[name]) return &clone, true } @@ -204,6 +169,11 @@ func cloneSkill(s *Skill) Skill { // SetEnabled toggles a writable skill by rewriting its front matter. func (m *Manager) SetEnabled(name string, enabled bool) error { + if m == nil { + return errors.New("skills manager is unavailable") + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() s, ok := m.Get(name) if !ok { return fmt.Errorf("skill %q not found", name) @@ -248,12 +218,17 @@ func (m *Manager) SetEnabled(name string, enabled bool) error { if err := os.WriteFile(s.Path, []byte(text), 0o644); err != nil { return err } - return m.Reload() + return m.reloadLocked() } // Save writes (or overwrites) a skill file in the first nonempty configured // directory. Imported effective names cannot be shadowed through this API. func (m *Manager) Save(name, description, body string, tags []string) (*Skill, error) { + if m == nil { + return nil, errors.New("skills manager is unavailable") + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() if existing, ok := m.Get(name); ok && existing.ReadOnly { return nil, fmt.Errorf("%w: %q", ErrReadOnly, name) } @@ -282,7 +257,7 @@ func (m *Manager) Save(name, description, body string, tags []string) (*Skill, e if err := os.WriteFile(path, []byte(content), 0o644); err != nil { return nil, err } - if err := m.Reload(); err != nil { + if err := m.reloadLocked(); err != nil { return nil, err } s, _ := m.Get(name) @@ -290,9 +265,9 @@ func (m *Manager) Save(name, description, body string, tags []string) (*Skill, e } func (m *Manager) writeDir() string { - m.mu.RLock() - defer m.mu.RUnlock() - for _, dir := range m.opts.Dirs { + m.state.mu.RLock() + defer m.state.mu.RUnlock() + for _, dir := range m.state.opts.Dirs { if strings.TrimSpace(dir) != "" { return dir } @@ -302,6 +277,11 @@ func (m *Manager) writeDir() string { // Delete removes a writable skill file. func (m *Manager) Delete(name string) error { + if m == nil { + return errors.New("skills manager is unavailable") + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() s, ok := m.Get(name) if !ok { return fmt.Errorf("skill %q not found", name) @@ -312,17 +292,17 @@ func (m *Manager) Delete(name string) error { if err := os.Remove(s.Path); err != nil { return err } - return m.Reload() + return m.reloadLocked() } // MarkUsed increments the in-memory usage counter shown in the dashboard. func (m *Manager) MarkUsed(name string) { - m.mu.Lock() - defer m.mu.Unlock() - m.usage[name]++ - if s, ok := m.skills[name]; ok { - s.UsageCount = m.usage[name] + if m == nil { + return } + m.state.mu.Lock() + defer m.state.mu.Unlock() + m.state.usage[name]++ } // PromptBlock renders the enabled skills as a compact catalogue for the system @@ -481,14 +461,19 @@ func scoreSkill(s Skill, words []string) int { // Chains resolves a skill's chains_with entries to the skills that exist, so // the agent can see which follow-on techniques compound with this one. func (m *Manager) Chains(name string) []Skill { - s, ok := m.Get(name) + if m == nil { + return nil + } + m.state.mu.RLock() + defer m.state.mu.RUnlock() + skill, ok := m.effectiveSkillLocked(name) if !ok { return nil } - out := make([]Skill, 0, len(s.ChainsWith)) - for _, next := range s.ChainsWith { - if ns, ok := m.Get(next); ok { - out = append(out, *ns) + out := make([]Skill, 0, len(skill.ChainsWith)) + for _, next := range skill.ChainsWith { + if chained, ok := m.effectiveSkillLocked(next); ok { + out = append(out, cloneSkillWithUsage(chained, m.state.usage[next])) } } return out @@ -515,18 +500,21 @@ func matchesAll(hay, query string) bool { // Categories lists the pack skill categories with their counts, for browsing. func (m *Manager) Categories() map[string]int { - m.mu.RLock() - defer m.mu.RUnlock() out := map[string]int{} - for _, s := range m.skills { - if !s.Pack { + if m == nil { + return out + } + m.state.mu.RLock() + defer m.state.mu.RUnlock() + for skill := range m.effectiveSkillsLocked { + if !skill.Pack { continue } - cat := s.Category - if cat == "" { - cat = "uncategorised" + category := skill.Category + if category == "" { + category = "uncategorised" } - out[cat]++ + out[category]++ } return out } @@ -537,18 +525,21 @@ func (m *Manager) Library(category string, offset, limit int) ([]Skill, int) { if limit <= 0 { limit = 50 } - m.mu.RLock() + if m == nil { + return nil, 0 + } + m.state.mu.RLock() all := make([]Skill, 0) - for _, s := range m.skills { - if !s.Pack { + for skill := range m.effectiveSkillsLocked { + if !skill.Pack { continue } - if category != "" && !strings.EqualFold(s.Category, category) { + if category != "" && !strings.EqualFold(skill.Category, category) { continue } - all = append(all, *s) + all = append(all, cloneSkillWithUsage(skill, m.state.usage[skill.Name])) } - m.mu.RUnlock() + m.state.mu.RUnlock() sort.Slice(all, func(i, j int) bool { return all[i].Name < all[j].Name }) total := len(all) @@ -564,11 +555,14 @@ func (m *Manager) Library(category string, offset, limit int) ([]Skill, int) { // PackCount reports how many skills came from the bundled library. func (m *Manager) PackCount() int { - m.mu.RLock() - defer m.mu.RUnlock() + if m == nil { + return 0 + } + m.state.mu.RLock() + defer m.state.mu.RUnlock() n := 0 - for _, s := range m.skills { - if s.Pack { + for skill := range m.effectiveSkillsLocked { + if skill.Pack { n++ } } @@ -577,11 +571,14 @@ func (m *Manager) PackCount() int { // Count reports how many skills are enabled. func (m *Manager) Count() int { - m.mu.RLock() - defer m.mu.RUnlock() + if m == nil { + return 0 + } + m.state.mu.RLock() + defer m.state.mu.RUnlock() n := 0 - for _, s := range m.skills { - if s.Enabled { + for skill := range m.effectiveSkillsLocked { + if skill.Enabled { n++ } } From c1d51918ba7f5bbb431d955fd66850810ffa2980 Mon Sep 17 00:00:00 2001 From: Reidho Satria Date: Wed, 16 Sep 2026 16:23:45 +0700 Subject: [PATCH 3/9] feat(skills): refresh live catalogs without restarting --- cmd/antares/main.go | 45 ++- cmd/antares/skills_test.go | 66 +++++ docs/skills.md | 6 + internal/server/handlers_commands.go | 3 +- internal/server/handlers_config.go | 4 - internal/server/handlers_hub.go | 10 +- internal/server/handlers_subsystems.go | 38 ++- internal/server/skills_helper.go | 7 +- internal/server/skills_test.go | 137 +++++++++ internal/skills/discovery.go | 167 ++++++++--- internal/skills/refresh.go | 139 +++++++++ internal/skills/refresh_test.go | 385 +++++++++++++++++++++++++ internal/skills/scope.go | 164 +++++------ 13 files changed, 1008 insertions(+), 163 deletions(-) create mode 100644 cmd/antares/skills_test.go create mode 100644 internal/server/skills_test.go create mode 100644 internal/skills/refresh.go create mode 100644 internal/skills/refresh_test.go diff --git a/cmd/antares/main.go b/cmd/antares/main.go index 0c004bf..9ad534b 100644 --- a/cmd/antares/main.go +++ b/cmd/antares/main.go @@ -207,6 +207,8 @@ type runtimeServices struct { social *socialbrowser.Manager skillsHome string skillsProjectDir string + skillsCancel context.CancelFunc + skillsDone chan struct{} } func bootstrap(ctx context.Context) (*runtimeServices, error) { @@ -342,6 +344,7 @@ func bootstrap(ctx context.Context) (*runtimeServices, error) { } } + rt.startSkillRefresh(ctx) return rt, nil } @@ -733,14 +736,12 @@ func (rt *runtimeServices) reload() error { rt.agent.SetRAG(ragProvider) packDir := config.Path("security-skills") - rt.skills = skills.NewManager(skills.Options{ + if err := rt.skills.Reconfigure(skills.Options{ Dirs: expandAll(cfg.Skills.Dirs), PackDirs: []string{packDir}, UserHome: rt.skillsHome, ProjectDir: rt.skillsProjectDir, - }) - if err := rt.skills.Reload(); err != nil { + }); err != nil { slog.Warn("some skills failed to load", "error", err) } - rt.agent.SetSkills(rt.skills) if cfg.Plugins.Enabled { pluginMgr := plugin.NewManager(expandAll(cfg.Plugins.Dirs)) @@ -771,7 +772,43 @@ func (rt *runtimeServices) reload() error { return nil } +// startSkillRefresh owns the one catalog worker for this runtime, including when +// skills are loaded for the dashboard but disabled for agent prompts and tools. +func (rt *runtimeServices) startSkillRefresh(ctx context.Context) { + if rt == nil { + return + } + rt.mu.Lock() + defer rt.mu.Unlock() + if rt.skills == nil || rt.skillsDone != nil { + return + } + ctx, rt.skillsCancel = context.WithCancel(ctx) + done := make(chan struct{}) + rt.skillsDone = done + mgr := rt.skills + go func() { + defer close(done) + mgr.Watch(ctx, 5*time.Second) + }() +} + +func (rt *runtimeServices) stopSkillRefresh() { + if rt == nil { + return + } + rt.mu.Lock() + cancel, done := rt.skillsCancel, rt.skillsDone + rt.mu.Unlock() + if cancel == nil { + return + } + cancel() + <-done +} + func (rt *runtimeServices) close() { + rt.stopSkillRefresh() if rt.mcp != nil { rt.mcp.Close() } diff --git a/cmd/antares/skills_test.go b/cmd/antares/skills_test.go new file mode 100644 index 0000000..942430d --- /dev/null +++ b/cmd/antares/skills_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" + + "github.com/enowdev/antares/internal/skills" +) + +func TestSkillRefreshStopsWithLiveParent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "lifecycle.md") + write := func(body string) { + t.Helper() + if err := os.WriteFile(path, []byte("---\nname: lifecycle\n---\n"+body), 0o644); err != nil { + t.Fatal(err) + } + } + write("before stop") + mgr := skills.NewManager(skills.Options{Dirs: []string{dir}}) + if err := mgr.Reload(); err != nil { + t.Fatal(err) + } + parent, cancel := context.WithCancel(context.Background()) + defer cancel() + rt := &runtimeServices{skills: mgr} + rt.startSkillRefresh(parent) + done := rt.skillsDone + t.Cleanup(rt.stopSkillRefresh) + stopped := make(chan struct{}) + go func() { rt.stopSkillRefresh(); close(stopped) }() + select { + case <-stopped: + case <-time.After(2 * time.Second): + t.Fatal("stop did not join refresh while parent remained live") + } + select { + case <-done: + default: + t.Fatal("stop returned before refresh completed") + } + if parent.Err() != nil { + t.Fatal("stopping refresh canceled its parent") + } + rt.stopSkillRefresh() + (*runtimeServices)(nil).stopSkillRefresh() + write("after stop") + deadline := time.NewTimer(5500 * time.Millisecond) + defer deadline.Stop() + probe := time.NewTicker(20 * time.Millisecond) + defer probe.Stop() + for { + select { + case <-deadline.C: + return + case <-probe.C: + got, ok := mgr.Get("lifecycle") + if !ok || got.Body != "before stop" { + t.Fatalf("catalog refreshed after joined stop: %+v", got) + } + } + } +} diff --git a/docs/skills.md b/docs/skills.md index fe30c12..c7d234b 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -92,6 +92,12 @@ An explicitly configured copy wins and remains writable, including when its dire is also an automatic root. Hub installs and `/learn` still write an Antares copy to their configured/native destination. +The running manager rescans every five seconds, including when skills are disabled +for the agent. Additions, normal edits, removals, and symlink retargets are visible +on the next scan without restarting. Edits preserving file identity, size, and +mtime are reparsed every twelve ticks (about one minute). A new prompt uses the +current catalog; a prompt already sent to a model is not rewritten. + ## Getting them **Bundled.** Eight are written on first run. See [the hub](hub.md). diff --git a/internal/server/handlers_commands.go b/internal/server/handlers_commands.go index dd795a2..5f4f3cf 100644 --- a/internal/server/handlers_commands.go +++ b/internal/server/handlers_commands.go @@ -10,11 +10,12 @@ import ( // commandDeps hands the shared command layer everything the server has wired. func (s *Server) commandDeps() commands.Deps { + mgr := s.currentSkills() return commands.Deps{ Config: s.config, Agent: s.agent, Store: s.db, - Skills: s.skills, + Skills: mgr, MCP: s.mcp, Reload: s.applyReload, Version: version.Version, diff --git a/internal/server/handlers_config.go b/internal/server/handlers_config.go index aa4e10d..3a4ebd6 100644 --- a/internal/server/handlers_config.go +++ b/internal/server/handlers_config.go @@ -133,10 +133,6 @@ func (s *Server) applyReload() error { return err } s.SetConfig(config.Get()) - // The agent owns the rebuilt skill library after a reload. - if m := s.agent.Skills(); m != nil { - s.skills = m - } return nil } diff --git a/internal/server/handlers_hub.go b/internal/server/handlers_hub.go index 77abc39..cc411ba 100644 --- a/internal/server/handlers_hub.go +++ b/internal/server/handlers_hub.go @@ -21,6 +21,7 @@ func (s *Server) skillDir() string { // handleHubSkills browses the skill catalogue. A query naming a repository or // a URL reaches out; anything else searches what ships in the binary. func (s *Server) handleHubSkills(w http.ResponseWriter, r *http.Request) { + mgr := s.currentSkills() query := r.URL.Query().Get("q") found, err := hub.SearchSkills(r.Context(), query) if err != nil { @@ -31,8 +32,8 @@ func (s *Server) handleHubSkills(w http.ResponseWriter, r *http.Request) { // Mark what is already on disk so the UI can offer the right action. installed := map[string]bool{} - if s.skills != nil { - for _, sk := range s.skills.List() { + if mgr != nil { + for _, sk := range mgr.List() { installed[sk.Name] = true } } @@ -45,6 +46,7 @@ func (s *Server) handleHubSkills(w http.ResponseWriter, r *http.Request) { // handleHubInstallSkill fetches a skill and writes it into the skills // directory, then reloads so it is usable on the next turn. func (s *Server) handleHubInstallSkill(w http.ResponseWriter, r *http.Request) { + mgr := s.currentSkills() var body struct { ID string `json:"id"` } @@ -57,8 +59,8 @@ func (s *Server) handleHubInstallSkill(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, map[string]any{"ok": false, "error": err.Error()}) return } - if s.skills != nil { - _ = s.skills.Reload() + if mgr != nil { + _ = mgr.Reload() } writeJSON(w, http.StatusOK, map[string]any{ "ok": true, "name": entry.Name, "path": path, "summary": entry.Summary, diff --git a/internal/server/handlers_subsystems.go b/internal/server/handlers_subsystems.go index 65aa03c..a39f420 100644 --- a/internal/server/handlers_subsystems.go +++ b/internal/server/handlers_subsystems.go @@ -22,7 +22,8 @@ var ( // ---- skills ----------------------------------------------------------------- func (s *Server) handleListSkills(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeJSON(w, http.StatusOK, map[string]any{"skills": []any{}}) return } @@ -36,20 +37,21 @@ func (s *Server) handleListSkills(w http.ResponseWriter, r *http.Request) { category := strings.TrimSpace(r.URL.Query().Get("category")) if q != "" || cwe != "" || tech != "" || category != "" { writeJSON(w, http.StatusOK, map[string]any{ - "skills": s.skills.SearchFiltered(q, skills.Filter{CWE: cwe, Tech: tech, Category: category}, 100), + "skills": mgr.SearchFiltered(q, skills.Filter{CWE: cwe, Tech: tech, Category: category}, 100), "searching": true, - "library": s.skills.PackCount(), + "library": mgr.PackCount(), }) return } writeJSON(w, http.StatusOK, map[string]any{ - "skills": s.skills.Everyday(), - "library": s.skills.PackCount(), + "skills": mgr.Everyday(), + "library": mgr.PackCount(), }) } func (s *Server) handleToggleSkill(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeError(w, http.StatusServiceUnavailable, errSkillsOff) return } @@ -61,7 +63,7 @@ func (s *Server) handleToggleSkill(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, err) return } - if err := s.skills.SetEnabled(body.Name, body.Enabled); err != nil { + if err := mgr.SetEnabled(body.Name, body.Enabled); err != nil { writeError(w, http.StatusBadRequest, err) return } @@ -69,11 +71,12 @@ func (s *Server) handleToggleSkill(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleGetSkill(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeError(w, http.StatusServiceUnavailable, errSkillsOff) return } - sk, ok := s.skills.Get(r.PathValue("name")) + sk, ok := mgr.Get(r.PathValue("name")) if !ok { writeError(w, http.StatusNotFound, errNotFound) return @@ -82,7 +85,8 @@ func (s *Server) handleGetSkill(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleSaveSkill(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeError(w, http.StatusServiceUnavailable, errSkillsOff) return } @@ -96,7 +100,7 @@ func (s *Server) handleSaveSkill(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, err) return } - sk, err := s.skills.Save(body.Name, body.Description, body.Body, body.Tags) + sk, err := mgr.Save(body.Name, body.Description, body.Body, body.Tags) if err != nil { writeError(w, http.StatusBadRequest, err) return @@ -105,11 +109,12 @@ func (s *Server) handleSaveSkill(w http.ResponseWriter, r *http.Request) { } func (s *Server) handleDeleteSkill(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeError(w, http.StatusServiceUnavailable, errSkillsOff) return } - if err := s.skills.Delete(r.PathValue("name")); err != nil { + if err := mgr.Delete(r.PathValue("name")); err != nil { writeError(w, http.StatusBadRequest, err) return } @@ -501,19 +506,20 @@ func (s *Server) refreshMCP(w http.ResponseWriter, r *http.Request, refresher mc // handleSkillLibrary browses the bundled security skill library — paged, by // category — so thousands of skills are explorable without searching blind. func (s *Server) handleSkillLibrary(w http.ResponseWriter, r *http.Request) { - if s.skills == nil { + mgr := s.currentSkills() + if mgr == nil { writeJSON(w, http.StatusOK, map[string]any{"skills": []any{}, "categories": map[string]int{}, "total": 0}) return } category := r.URL.Query().Get("category") offset := queryInt(r, "offset", 0) limit := queryInt(r, "limit", 50) - page, total := s.skills.Library(category, offset, limit) + page, total := mgr.Library(category, offset, limit) writeJSON(w, http.StatusOK, map[string]any{ "skills": page, "total": total, "offset": offset, "limit": limit, - "categories": s.skills.Categories(), + "categories": mgr.Categories(), }) } diff --git a/internal/server/skills_helper.go b/internal/server/skills_helper.go index 4950322..5ac1e9f 100644 --- a/internal/server/skills_helper.go +++ b/internal/server/skills_helper.go @@ -2,10 +2,9 @@ package server import "github.com/enowdev/antares/internal/skills" -// currentSkills returns the live skill manager: the agent owns it after a -// reload (rt.reload rebuilds it), so a per-operation snapshot from the agent -// is authoritative. Tests that construct a Server with no agent still get the -// seeded Options.Skills. +// currentSkills snapshots the live skill manager once per operation. The agent +// is authoritative when present; its stable manager pointer is reconfigured in +// place. Tests that construct a Server with no agent still use Options.Skills. func (s *Server) currentSkills() *skills.Manager { if s.agent != nil { if m := s.agent.Skills(); m != nil { diff --git a/internal/server/skills_test.go b/internal/server/skills_test.go new file mode 100644 index 0000000..c7c3acb --- /dev/null +++ b/internal/server/skills_test.go @@ -0,0 +1,137 @@ +package server + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/enowdev/antares/internal/agent" + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/skills" +) + +func writeServerSkill(t *testing.T, dir, name, category, body string) { + t.Helper() + content := fmt.Sprintf("---\nname: %s\ndescription: %s description\nenabled: true\ncategory: %s\n---\n%s\n", name, name, category, body) + if err := os.WriteFile(filepath.Join(dir, name+".md"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } +} + +func decodeServerJSON(t *testing.T, rr *httptest.ResponseRecorder, dst any) { + t.Helper() + if err := json.NewDecoder(rr.Body).Decode(dst); err != nil { + t.Fatalf("decode response status %d: %v; body=%q", rr.Code, err, rr.Body.String()) + } +} + +func TestSkillConsumersSeeInPlaceReconfigure(t *testing.T) { + t.Setenv("ANTARES_HOME", t.TempDir()) + + oldDir, oldPack := t.TempDir(), t.TempDir() + newDir, newPack := t.TempDir(), t.TempDir() + fallbackDir := t.TempDir() + writeServerSkill(t, oldDir, "old-catalog", "everyday", "OLD_BODY") + writeServerSkill(t, oldPack, "old-library", "old-category", "OLD_LIBRARY_BODY") + writeServerSkill(t, newDir, "new-catalog", "everyday", "NEW_BODY") + writeServerSkill(t, newPack, "new-library", "new-category", "NEW_LIBRARY_BODY") + writeServerSkill(t, fallbackDir, "fallback-only", "everyday", "FALLBACK_BODY") + + live := skills.NewManager(skills.Options{Dirs: []string{oldDir}, PackDirs: []string{oldPack}}) + if err := live.Reload(); err != nil { + t.Fatal(err) + } + fallback := skills.NewManager(skills.Options{Dirs: []string{fallbackDir}}) + if err := fallback.Reload(); err != nil { + t.Fatal(err) + } + + cfg := config.Default() + ag := &agent.Agent{} + ag.SetConfig(cfg) + ag.SetSkills(live) + s := &Server{ + cfg: cfg, + agent: ag, + skills: fallback, + reloadFn: func() error { + return live.Reconfigure(skills.Options{Dirs: []string{newDir}, PackDirs: []string{newPack}}) + }, + } + + if err := s.applyReload(); err != nil { + t.Fatal(err) + } + + list := httptest.NewRecorder() + s.handleListSkills(list, httptest.NewRequest(http.MethodGet, "/api/skills", nil)) + if list.Code != http.StatusOK { + t.Fatalf("list status = %d, want 200; body=%q", list.Code, list.Body.String()) + } + var listed struct { + Skills []skills.Skill `json:"skills"` + } + decodeServerJSON(t, list, &listed) + if len(listed.Skills) != 1 || listed.Skills[0].Name != "new-catalog" { + t.Fatalf("list did not expose only the reconfigured everyday catalog: %+v", listed.Skills) + } + + get := httptest.NewRecorder() + getReq := httptest.NewRequest(http.MethodGet, "/api/skills/new-catalog", nil) + getReq.SetPathValue("name", "new-catalog") + s.handleGetSkill(get, getReq) + if get.Code != http.StatusOK { + t.Fatalf("get status = %d, want 200; body=%q", get.Code, get.Body.String()) + } + var fetched struct { + Skill skills.Skill `json:"skill"` + Body string `json:"body"` + } + decodeServerJSON(t, get, &fetched) + if fetched.Skill.Name != "new-catalog" || !strings.Contains(fetched.Body, "NEW_BODY") { + t.Fatalf("get returned stale skill: %+v body=%q", fetched.Skill, fetched.Body) + } + + oldGet := httptest.NewRecorder() + oldGetReq := httptest.NewRequest(http.MethodGet, "/api/skills/old-catalog", nil) + oldGetReq.SetPathValue("name", "old-catalog") + s.handleGetSkill(oldGet, oldGetReq) + if oldGet.Code != http.StatusNotFound { + t.Fatalf("removed skill status = %d, want 404; body=%q", oldGet.Code, oldGet.Body.String()) + } + + library := httptest.NewRecorder() + s.handleSkillLibrary(library, httptest.NewRequest(http.MethodGet, "/api/skills/library", nil)) + if library.Code != http.StatusOK { + t.Fatalf("library status = %d, want 200; body=%q", library.Code, library.Body.String()) + } + var libraryBody struct { + Skills []skills.Skill `json:"skills"` + Total int `json:"total"` + } + decodeServerJSON(t, library, &libraryBody) + if libraryBody.Total != 1 || len(libraryBody.Skills) != 1 || libraryBody.Skills[0].Name != "new-library" { + t.Fatalf("library did not expose only the reconfigured pack: %+v (total %d)", libraryBody.Skills, libraryBody.Total) + } + + command := httptest.NewRecorder() + commandReq := httptest.NewRequest(http.MethodPost, "/api/commands/run", strings.NewReader(`{"input":"/skills","surface":"web"}`)) + s.handleCommandRun(command, commandReq) + if command.Code != http.StatusOK { + t.Fatalf("command status = %d, want 200; body=%q", command.Code, command.Body.String()) + } + var commandBody struct { + OK bool `json:"ok"` + Output string `json:"output"` + } + decodeServerJSON(t, command, &commandBody) + if !commandBody.OK || !strings.Contains(commandBody.Output, "new-catalog") || !strings.Contains(commandBody.Output, "new-library") || + strings.Contains(commandBody.Output, "old-catalog") || strings.Contains(commandBody.Output, "old-library") || strings.Contains(commandBody.Output, "fallback-only") { + t.Fatalf("command did not use the reconfigured live catalog: %+v", commandBody) + } +} diff --git a/internal/skills/discovery.go b/internal/skills/discovery.go index 04282ec..1bdd97d 100644 --- a/internal/skills/discovery.go +++ b/internal/skills/discovery.go @@ -1,6 +1,7 @@ package skills import ( + "context" "errors" "io/fs" "os" @@ -22,6 +23,22 @@ type sourceRoot struct { kind sourceKind } +// cachedSkillFile holds only parser output and filesystem identity. Logical +// path, fallback name, modification time, and provenance belong to each source +// occurrence and are attached after a cache hit. +type cachedSkillFile struct { + info fs.FileInfo + parsed *Skill +} +type discoveryScan struct { + ctx context.Context + force bool + previous map[string]cachedSkillFile + next map[string]cachedSkillFile + failed map[string]struct{} + preservePrevious bool +} + var userSkillRoots = [][]string{ {".agent", "skills"}, {".agents", "skills"}, @@ -40,10 +57,40 @@ var projectSkillRoots = [][]string{ {".github", "skills"}, } +func newDiscoveryScan(ctx context.Context, force bool, previous map[string]cachedSkillFile, preservePrevious bool) *discoveryScan { + if ctx == nil { + ctx = context.Background() + } + capacity := 0 + if !preservePrevious { + capacity = len(previous) + } + return &discoveryScan{ + ctx: ctx, force: force, previous: previous, + next: make(map[string]cachedSkillFile, capacity), + failed: make(map[string]struct{}), preservePrevious: preservePrevious, + } +} + +func (scan *discoveryScan) cache() map[string]cachedSkillFile { + if !scan.preservePrevious { + return scan.next + } + // First-project registration only adds its own cache entries. The caller + // holds scanMu and the state write lock; parsed values remain immutable. + for path := range scan.failed { + delete(scan.previous, path) + } + for path, cached := range scan.next { + scan.previous[path] = cached + } + return scan.previous +} + // discoverShared scans each shared source kind independently. Lower-priority // entries remain in their layer so removing an override reveals them later. -func discoverShared(opts Options) (bundled, user, configured map[string]*Skill, firstErr error) { - bundled, err := discoverRoots(rootsForPaths(opts.PackDirs, sourcePack)) +func discoverShared(scan *discoveryScan, opts Options) (bundled, user, configured map[string]*Skill, firstErr error) { + bundled, err := discoverRoots(scan, rootsForPaths(opts.PackDirs, sourcePack)) firstErr = err userRoots := make([]string, 0, len(userSkillRoots)) if strings.TrimSpace(opts.UserHome) != "" { @@ -51,11 +98,11 @@ func discoverShared(opts Options) (bundled, user, configured map[string]*Skill, userRoots = append(userRoots, filepath.Join(append([]string{opts.UserHome}, parts...)...)) } } - user, err = discoverRoots(rootsForPaths(userRoots, sourceUser)) + user, err = discoverRoots(scan, rootsForPaths(userRoots, sourceUser)) if firstErr == nil { firstErr = err } - configured, err = discoverRoots(rootsForPaths(opts.Dirs, sourceConfigured)) + configured, err = discoverRoots(scan, rootsForPaths(opts.Dirs, sourceConfigured)) if firstErr == nil { firstErr = err } @@ -64,23 +111,26 @@ func discoverShared(opts Options) (bundled, user, configured map[string]*Skill, // discoverProject scans only the conventional roots beneath one normalized // logical project directory. -func discoverProject(projectDir string) (map[string]*Skill, error) { +func discoverProject(scan *discoveryScan, projectDir string) (map[string]*Skill, error) { paths := make([]string, 0, len(projectSkillRoots)) for _, parts := range projectSkillRoots { paths = append(paths, filepath.Join(append([]string{projectDir}, parts...)...)) } - return discoverRoots(rootsForPaths(paths, sourceProject)) + return discoverRoots(scan, rootsForPaths(paths, sourceProject)) } func rootsForPaths(paths []string, kind sourceKind) []sourceRoot { return appendSourceRoots(nil, paths, kind) } -func discoverRoots(roots []sourceRoot) (map[string]*Skill, error) { +func discoverRoots(scan *discoveryScan, roots []sourceRoot) (map[string]*Skill, error) { found := make(map[string]*Skill) var firstErr error for _, root := range roots { - if err := scanRoot(root, func(skill *Skill) { + if err := scan.ctx.Err(); err != nil { + return found, err + } + if err := scanRoot(scan, root, func(skill *Skill) { found[skill.Name] = skill }); err != nil && firstErr == nil { firstErr = err @@ -129,35 +179,31 @@ func sameRoot(left, right string) bool { return left == right } -func scanRoot(root sourceRoot, publish func(*Skill)) error { +func scanRoot(scan *discoveryScan, root sourceRoot, publish func(*Skill)) error { var firstErr error ancestors := make(map[string]struct{}) - var walk func(string) - walk = func(logical string) { + var walk func(string) error + walk = func(logical string) error { + if err := scan.ctx.Err(); err != nil { + return err + } info, err := os.Stat(logical) if err != nil { if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { firstErr = err } - return + return nil } if info.IsDir() { - canonical, err := filepath.EvalSymlinks(logical) + canonical, err := canonicalPath(logical) if err != nil { if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { firstErr = err } - return - } - canonical, err = filepath.Abs(canonical) - if err != nil { - if firstErr == nil { - firstErr = err - } - return + return nil } if _, cycle := ancestors[canonical]; cycle { - return + return nil } ancestors[canonical] = struct{}{} entries, err := os.ReadDir(logical) @@ -166,36 +212,47 @@ func scanRoot(root sourceRoot, publish func(*Skill)) error { if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { firstErr = err } - return + return nil } for _, entry := range entries { // os.ReadDir returns lexical order. + if err := scan.ctx.Err(); err != nil { + delete(ancestors, canonical) + return err + } if strings.HasPrefix(entry.Name(), ".") { continue } - walk(filepath.Join(logical, entry.Name())) + if err := walk(filepath.Join(logical, entry.Name())); err != nil { + delete(ancestors, canonical) + return err + } } delete(ancestors, canonical) - return + return nil } if !info.Mode().IsRegular() { - return + return nil } base := filepath.Base(logical) if root.kind == sourceUser || root.kind == sourceProject { if !strings.EqualFold(base, "SKILL.md") { - return + return nil } } else if !strings.EqualFold(filepath.Ext(base), ".md") { - return + return nil } - skill, err := parseFile(logical) + parsed, err := scan.parse(logical, info) if err != nil { - if firstErr == nil { + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return err + } + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { firstErr = err } - return + return nil } + skill := *parsed if strings.TrimSpace(skill.Name) == "" { if strings.EqualFold(base, "SKILL.md") { skill.Name = filepath.Base(filepath.Dir(logical)) @@ -210,8 +267,52 @@ func scanRoot(root sourceRoot, publish func(*Skill)) error { skill.UpdatedAt = info.ModTime() skill.Pack = root.kind == sourcePack skill.ReadOnly = root.kind == sourceUser || root.kind == sourceProject - publish(skill) + publish(&skill) + return nil + } + if err := walk(root.path); err != nil { + return err } - walk(root.path) return firstErr } + +func (scan *discoveryScan) parse(logical string, info fs.FileInfo) (*Skill, error) { + if err := scan.ctx.Err(); err != nil { + return nil, err + } + canonical, err := canonicalPath(logical) + if err != nil { + return nil, err + } + if cached, ok := scan.next[canonical]; ok && sameCachedFile(cached, info) { + return cached.parsed, nil + } + if !scan.force { + if cached, ok := scan.previous[canonical]; ok && sameCachedFile(cached, info) { + scan.next[canonical] = cached + return cached.parsed, nil + } + } + parsed, err := parseFile(logical) + if err != nil { + delete(scan.next, canonical) + scan.failed[canonical] = struct{}{} + return nil, err + } + scan.next[canonical] = cachedSkillFile{info: info, parsed: parsed} + delete(scan.failed, canonical) + return parsed, nil +} + +func canonicalPath(path string) (string, error) { + canonical, err := filepath.EvalSymlinks(path) + if err != nil { + return "", err + } + return filepath.Abs(canonical) +} + +func sameCachedFile(cached cachedSkillFile, info fs.FileInfo) bool { + return cached.parsed != nil && cached.info != nil && os.SameFile(cached.info, info) && + cached.info.Size() == info.Size() && cached.info.ModTime().Equal(info.ModTime()) +} diff --git a/internal/skills/refresh.go b/internal/skills/refresh.go new file mode 100644 index 0000000..655b661 --- /dev/null +++ b/internal/skills/refresh.go @@ -0,0 +1,139 @@ +package skills + +import ( + "context" + "errors" + "log/slog" + "sort" + "time" +) + +const ( + defaultRefreshInterval = 5 * time.Second + forcedRefreshTicks = 12 +) + +// Watch blocks in the caller's goroutine until cancellation. A forced parse every +// twelve ticks bounds detection latency for metadata-preserving edits. +func (m *Manager) Watch(ctx context.Context, interval time.Duration) { + if m == nil { + return + } + if interval <= 0 { + interval = defaultRefreshInterval + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + ticks := 0 + for { + select { + case <-ctx.Done(): + return + case <-ticker.C: + ticks++ + m.state.scanMu.Lock() + err := ctx.Err() + if err == nil { + err = m.refreshLocked(ctx, ticks%forcedRefreshTicks == 0) + } + m.state.scanMu.Unlock() + if err != nil && !isContextError(err) { + slog.Warn("some skills failed to load", "error", err) + } + } + } +} + +// Reload synchronously reparses all shared sources and registered projects. +func (m *Manager) Reload() error { + if m == nil { + return nil + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() + return m.reloadLocked() +} + +// reloadLocked is used after mutations already holding scanMu. +func (m *Manager) reloadLocked() error { + return m.refreshLocked(context.Background(), true) +} + +func (m *Manager) refreshLocked(ctx context.Context, force bool) error { + m.state.mu.RLock() + opts, defaultErr := m.state.opts, m.state.defaultErr + m.state.mu.RUnlock() + return m.scanAndPublishLocked(ctx, opts, defaultErr, force) +} + +// Reconfigure publishes cloned options and their newly scanned layers together. +// Existing bound handles retain their logical project paths; only the root view +// follows the new default project. Relative paths keep the original startup base. +func (m *Manager) Reconfigure(opts Options) error { + if m == nil { + return nil + } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() + opts = cloneOptions(opts) + project, err := normalizeReconfiguredProject(opts.ProjectDir, m.state.startupBase) + opts.ProjectDir = project + return m.scanAndPublishLocked(context.Background(), opts, err, true) +} + +// scanAndPublishLocked requires scanMu, never holding the state write lock while +// walking. Non-cancellation errors publish successful entries; canceled scans +// leave the prior options, layers and cache untouched. +func (m *Manager) scanAndPublishLocked(ctx context.Context, opts Options, defaultErr error, force bool) error { + state := m.state + state.mu.RLock() + projectsToScan := make([]string, 0, len(state.scopes)+1) + for path := range state.scopes { + projectsToScan = append(projectsToScan, path) + } + if opts.ProjectDir != "" { + if _, registered := state.scopes[opts.ProjectDir]; !registered { + projectsToScan = append(projectsToScan, opts.ProjectDir) + } + } + previous := state.cache + state.mu.RUnlock() + sort.Strings(projectsToScan) + + scan := newDiscoveryScan(ctx, force, previous, false) + bundled, user, configured, sharedErr := discoverShared(scan, opts) + projects := make(map[string]map[string]*Skill, len(projectsToScan)) + projectErrs := make(map[string]error, len(projectsToScan)) + firstErr := defaultErr + if firstErr == nil { + firstErr = sharedErr + } + for _, path := range projectsToScan { + if err := ctx.Err(); err != nil { + return err + } + found, err := discoverProject(scan, path) + projects[path], projectErrs[path] = found, err + if firstErr == nil { + firstErr = err + } + } + if err := ctx.Err(); err != nil { + return err + } + state.mu.Lock() + defer state.mu.Unlock() + if err := ctx.Err(); err != nil { + return err + } + state.opts = opts + state.defaultProject, state.defaultErr = opts.ProjectDir, defaultErr + state.bundled, state.user, state.configured = bundled, user, configured + state.projects, state.projectErrs, state.sharedErr = projects, projectErrs, sharedErr + state.cache = scan.cache() + return firstErr +} + +func isContextError(err error) bool { + return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) +} diff --git a/internal/skills/refresh_test.go b/internal/skills/refresh_test.go new file mode 100644 index 0000000..4364d99 --- /dev/null +++ b/internal/skills/refresh_test.go @@ -0,0 +1,385 @@ +package skills + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "strings" + "sync" + "testing" + "time" +) + +const refreshTestInterval = 15 * time.Millisecond + +func startTestWatch(t *testing.T, manager *Manager) (context.CancelFunc, <-chan struct{}) { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + done := make(chan struct{}) + go func() { + defer close(done) + manager.Watch(ctx, refreshTestInterval) + }() + return cancel, done +} + +func stopTestWatch(t *testing.T, cancel context.CancelFunc, done <-chan struct{}) { + t.Helper() + cancel() + select { + case <-done: + case <-time.After(time.Second): + t.Fatal("Watch did not stop after context cancellation") + } +} + +func eventually(t *testing.T, condition func() bool, detail string) { + t.Helper() + deadline := time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + if condition() { + return + } + runtime.Gosched() + time.Sleep(time.Millisecond) + } + t.Fatalf("deadline waiting for %s", detail) +} + +func eventuallySkill(t *testing.T, manager *Manager, name string, condition func(*Skill) bool) { + t.Helper() + eventually(t, func() bool { + skill, ok := manager.Get(name) + return ok && condition(skill) + }, "skill "+name) +} + +func TestWatchRefreshTransitions(t *testing.T) { + base := t.TempDir() + home := filepath.Join(base, "home") + startup := filepath.Join(base, "startup") + projectA := filepath.Join(base, "project-a") + projectB := filepath.Join(base, "project-b") + configuredA := filepath.Join(base, "configured-a") + configuredB := filepath.Join(base, "configured-b") + manager := NewManager(Options{Dirs: []string{configuredA}, UserHome: home, ProjectDir: startup}) + mustReload(t, manager) + a, err := manager.ForProject(projectA) + if err != nil { + t.Fatal(err) + } + b, err := manager.ForProject(projectB) + if err != nil { + t.Fatal(err) + } + writeProjectSkill(t, projectB, "b-only", "B_STABLE") + mustReload(t, manager) + + cancel, done := startTestWatch(t, manager) + defer func() { stopTestWatch(t, cancel, done) }() + + // The automatic root is absent when the watcher starts and is enumerated on + // every tick, so creating the root and its first procedure needs no Reload. + livePath := filepath.Join(home, ".agent", "skills", "live", "SKILL.md") + writeFile(t, livePath, skillDocument("live", "initial description", "INITIAL_BODY", true)) + eventuallySkill(t, manager, "live", func(skill *Skill) bool { + return skill.Body == "INITIAL_BODY" && skill.ReadOnly + }) + if got := manager.Search("initial description", 5); len(got) != 1 || got[0].Name != "live" { + t.Fatalf("Search after automatic addition = %+v", got) + } + if !strings.Contains(manager.PromptBlock(0), "live") { + t.Fatal("PromptBlock did not expose enabled watched skill") + } + + writeFile(t, livePath, skillDocument("renamed-live", "changed description", "EDITED_BODY_LONGER", false)) + eventually(t, func() bool { + _, old := manager.Get("live") + renamed, ok := manager.Get("renamed-live") + return !old && ok && renamed.Body == "EDITED_BODY_LONGER" && !renamed.Enabled && + len(manager.Search("changed description", 5)) == 1 && !strings.Contains(manager.PromptBlock(0), "renamed-live") + }, "metadata/body edit to update every query surface") + + atomicTemp := filepath.Join(filepath.Dir(livePath), "replacement.tmp") + writeFile(t, atomicTemp, skillDocument("atomic-live", "atomic description", "ATOMIC_BODY", true)) + if err := os.Rename(atomicTemp, livePath); err != nil { + t.Fatal(err) + } + eventually(t, func() bool { + _, old := manager.Get("renamed-live") + atomic, ok := manager.Get("atomic-live") + return !old && ok && atomic.Body == "ATOMIC_BODY" && strings.Contains(manager.PromptBlock(0), "atomic-live") + }, "atomic replacement") + + projectAPath := writeProjectSkill(t, projectA, "a-only", "A_INITIAL") + eventuallySkill(t, a, "a-only", func(skill *Skill) bool { return skill.Body == "A_INITIAL" }) + writeFile(t, projectAPath, skillDocument("a-only", "a changed", "A_CHANGED_LONGER", true)) + eventuallySkill(t, a, "a-only", func(skill *Skill) bool { return skill.Body == "A_CHANGED_LONGER" }) + if got := requireSkill(t, b, "b-only"); got.Body != "B_STABLE" { + t.Fatalf("A refresh changed B scope: %+v", got) + } + if _, ok := b.Get("a-only"); ok { + t.Fatal("A watched skill leaked into B") + } + + writeFile(t, filepath.Join(configuredA, "old.md"), skillDocument("old-configured", "old", "OLD", true)) + eventuallySkill(t, manager, "old-configured", func(skill *Skill) bool { return skill.Body == "OLD" }) + newStartup := filepath.Join(base, "elsewhere", "new-startup") + writeProjectSkill(t, newStartup, "new-startup-only", "NEW_STARTUP") + writeFile(t, filepath.Join(configuredB, "new.md"), skillDocument("new-configured", "new", "NEW", true)) + newDirs := []string{configuredB} + if err := manager.Reconfigure(Options{Dirs: newDirs, UserHome: home, ProjectDir: newStartup}); err != nil { + t.Fatal(err) + } + newDirs[0] = configuredA + if _, ok := manager.Get("old-configured"); ok { + t.Fatal("root handle retained removed configured source after Reconfigure") + } + for label, scoped := range map[string]*Manager{"root": manager, "A": a, "B": b} { + if got := requireSkill(t, scoped, "new-configured"); got.Body != "NEW" { + t.Fatalf("%s existing handle missed reconfigured source: %+v", label, got) + } + } + if got := requireSkill(t, manager, "new-startup-only"); got.Body != "NEW_STARTUP" { + t.Fatalf("root handle did not follow reconfigured default project: %+v", got) + } + writeFile(t, filepath.Join(configuredB, "after.md"), skillDocument("after-reconfigure", "after", "AFTER", true)) + eventuallySkill(t, a, "after-reconfigure", func(skill *Skill) bool { return skill.Body == "AFTER" }) + relative, err := manager.ForProject("../project-a") + if err != nil { + t.Fatal(err) + } + if got := requireSkill(t, relative, "a-only"); got.Body != "A_CHANGED_LONGER" { + t.Fatalf("relative binding no longer uses original startup directory: %+v", got) + } + + if err := os.Remove(livePath); err != nil { + t.Fatal(err) + } + eventually(t, func() bool { + _, ok := manager.Get("atomic-live") + return !ok && len(manager.Search("atomic description", 5)) == 0 && !strings.Contains(manager.PromptBlock(0), "atomic-live") + }, "watched removal") + var readers sync.WaitGroup + for range 12 { + readers.Add(1) + go func() { + defer readers.Done() + for range 30 { + _ = manager.List() + _, _ = a.Get("a-only") + _ = b.Search("B_STABLE", 2) + } + }() + } + if _, err := manager.Save("concurrent", "concurrent", "CONCURRENT", nil); err != nil { + t.Fatal(err) + } + readers.Wait() +} + +func TestWatchDetectsPreservedMetadata(t *testing.T) { + configured := t.TempDir() + path := filepath.Join(configured, "stable.md") + oldDoc := skillDocument("stable", "same", "AAAA", true) + newDoc := skillDocument("stable", "same", "BBBB", true) + if len(oldDoc) != len(newDoc) { + t.Fatal("fixture documents must be the same size") + } + writeFile(t, path, oldDoc) + manager := NewManager(Options{Dirs: []string{configured}}) + mustReload(t, manager) + originalInfo, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + + writeFile(t, path, newDoc) + if err := os.Chtimes(path, originalInfo.ModTime(), originalInfo.ModTime()); err != nil { + t.Fatal(err) + } + cancel, done := startTestWatch(t, manager) + defer func() { stopTestWatch(t, cancel, done) }() + // Cached ticks may preserve the old bytes; the twelfth tick must not. + eventuallySkill(t, manager, "stable", func(skill *Skill) bool { return skill.Body == "BBBB" }) +} + +func TestReloadBypassesParseCache(t *testing.T) { + configured := t.TempDir() + path := filepath.Join(configured, "stable.md") + oldDoc := skillDocument("stable", "same", "AAAA", true) + newDoc := skillDocument("stable", "same", "BBBB", true) + if len(oldDoc) != len(newDoc) { + t.Fatal("fixture documents must be the same size") + } + writeFile(t, path, oldDoc) + manager := NewManager(Options{Dirs: []string{configured}}) + mustReload(t, manager) + originalInfo, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + writeFile(t, path, newDoc) + if err := os.Chtimes(path, originalInfo.ModTime(), originalInfo.ModTime()); err != nil { + t.Fatal(err) + } + mustReload(t, manager) + if got := requireSkill(t, manager, "stable"); got.Body != "BBBB" { + t.Fatalf("forced Reload reused cached parser output: %+v", got) + } +} + +func TestWatchCancellationStopsRefresh(t *testing.T) { + configured := t.TempDir() + path := filepath.Join(configured, "skill.md") + writeFile(t, path, skillDocument("skill", "initial", "INITIAL", true)) + manager := NewManager(Options{Dirs: []string{configured}}) + mustReload(t, manager) + cancel, done := startTestWatch(t, manager) + stopTestWatch(t, cancel, done) + + writeFile(t, path, skillDocument("skill", "changed", "CHANGED_AFTER_CANCEL", true)) + deadline := time.Now().Add(4 * refreshTestInterval) + for time.Now().Before(deadline) { + if got := requireSkill(t, manager, "skill"); got.Body != "INITIAL" { + t.Fatalf("catalog changed after Watch stopped: %+v", got) + } + runtime.Gosched() + } +} + +func TestCanceledRefreshDoesNotPublishPartialScan(t *testing.T) { + configured := t.TempDir() + writeFile(t, filepath.Join(configured, "first.md"), skillDocument("first", "first", "FIRST", true)) + writeFile(t, filepath.Join(configured, "second.md"), skillDocument("second", "second", "SECOND", true)) + manager := NewManager(Options{Dirs: []string{configured}}) + mustReload(t, manager) + + writeFile(t, filepath.Join(configured, "first.md"), skillDocument("first-new", "changed", "CHANGED", true)) + // Cancellation occurs after the first replacement was parsed but while later + // directory entries remain, proving publication is all-or-nothing mid-scan. + ctx := &cancelAfterChecks{Context: context.Background(), remaining: 6} + manager.state.scanMu.Lock() + err := manager.refreshLocked(ctx, true) + manager.state.scanMu.Unlock() + if !errors.Is(err, context.Canceled) { + t.Fatalf("canceled refresh error = %v", err) + } + if got := requireSkill(t, manager, "first"); got.Body != "FIRST" { + t.Fatalf("canceled refresh published partial replacement: %+v", got) + } + if _, ok := manager.Get("first-new"); ok { + t.Fatal("canceled refresh published newly parsed entry") + } + requireSkill(t, manager, "second") +} + +type cancelAfterChecks struct { + context.Context + remaining int +} + +func (ctx *cancelAfterChecks) Err() error { + ctx.remaining-- + if ctx.remaining <= 0 { + return context.Canceled + } + return nil +} + +func TestWatchRetriesParseErrorsAndPrunesCache(t *testing.T) { + configured := t.TempDir() + path := filepath.Join(configured, "retry.md") + neighbor := filepath.Join(configured, "neighbor.md") + writeFile(t, path, skillDocument("retry", "valid", "VALID", true)) + writeFile(t, neighbor, skillDocument("neighbor", "initial", "NEIGHBOR_INITIAL", true)) + manager := NewManager(Options{Dirs: []string{configured}}) + mustReload(t, manager) + cancel, done := startTestWatch(t, manager) + defer func() { stopTestWatch(t, cancel, done) }() + + writeFile(t, path, "---\nname: [broken\n---\nBROKEN") + writeFile(t, neighbor, skillDocument("neighbor", "changed", "NEIGHBOR_CHANGED_LONGER", true)) + eventually(t, func() bool { + _, ok := manager.Get("retry") + updated, neighborOK := manager.Get("neighbor") + return !ok && neighborOK && updated.Body == "NEIGHBOR_CHANGED_LONGER" + }, "failed parse removal with successful neighbor publication") + writeFile(t, path, skillDocument("retry", "recovered", "RECOVERED_LONGER", true)) + eventuallySkill(t, manager, "retry", func(skill *Skill) bool { return skill.Body == "RECOVERED_LONGER" }) + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + eventually(t, func() bool { + _, ok := manager.Get("retry") + return !ok + }, "removed cached file to disappear") +} +func TestWatchCacheSourceNeutralAcrossAliases(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink cache coverage is exercised on Unix") + } + base := t.TempDir() + home := filepath.Join(base, "home") + target := filepath.Join(base, "shared-skill.md") + writeFile(t, target, skillDocument("", "shared cache", "SHARED", true)) + for _, alias := range []string{ + filepath.Join(home, ".agent", "skills", "alpha", "SKILL.md"), + filepath.Join(home, ".agents", "skills", "beta", "SKILL.md"), + } { + makeDir(t, filepath.Dir(alias)) + if err := os.Symlink(target, alias); err != nil { + t.Skipf("file symlinks unavailable: %v", err) + } + } + manager := NewManager(Options{UserHome: home}) + mustReload(t, manager) + cancel, done := startTestWatch(t, manager) + defer func() { stopTestWatch(t, cancel, done) }() + writeFile(t, filepath.Join(home, ".agent", "skills", "tick-marker", "SKILL.md"), skillDocument("tick-marker", "tick", "TICK", true)) + eventuallySkill(t, manager, "tick-marker", func(skill *Skill) bool { return skill.Body == "TICK" }) + for name, wantSuffix := range map[string]string{ + "alpha": filepath.Join("alpha", "SKILL.md"), + "beta": filepath.Join("beta", "SKILL.md"), + } { + got := requireSkill(t, manager, name) + if got.Body != "SHARED" || !strings.HasSuffix(got.Path, wantSuffix) { + t.Fatalf("cached alias %q materialization = %+v", name, got) + } + } +} + +func TestWatchRetargetsBoundAlias(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("symlink retarget coverage is exercised on Unix") + } + base := t.TempDir() + startup := filepath.Join(base, "startup") + targetA := filepath.Join(base, "target-a") + targetB := filepath.Join(base, "target-b") + alias := filepath.Join(base, "alias") + writeProjectSkill(t, targetA, "collision", "ALIAS_A") + writeProjectSkill(t, targetB, "collision", "ALIAS_B") + if err := os.Symlink(targetA, alias); err != nil { + t.Skipf("directory symlinks unavailable: %v", err) + } + manager := NewManager(Options{ProjectDir: startup}) + mustReload(t, manager) + scoped, err := manager.ForProject(alias) + if err != nil { + t.Fatal(err) + } + cancel, done := startTestWatch(t, manager) + defer func() { stopTestWatch(t, cancel, done) }() + if err := os.Remove(alias); err != nil { + t.Fatal(err) + } + if err := os.Symlink(targetB, alias); err != nil { + t.Fatal(err) + } + eventuallySkill(t, scoped, "collision", func(skill *Skill) bool { + return skill.Body == "ALIAS_B" && strings.HasPrefix(skill.Path, alias) + }) +} diff --git a/internal/skills/scope.go b/internal/skills/scope.go index 9dc83c3..233b766 100644 --- a/internal/skills/scope.go +++ b/internal/skills/scope.go @@ -1,9 +1,9 @@ package skills import ( + "context" "fmt" "path/filepath" - "sort" "strings" "sync" ) @@ -24,19 +24,21 @@ type managerState struct { projectErrs map[string]error scopes map[string]*Manager usage map[string]int + cache map[string]cachedSkillFile root *Manager sharedOnly *Manager - // startupDir is the normalized logical startup project directory used both - // by the root handle and as the base for relative session project paths. - startupDir string - defaultErr error - sharedErr error + // startupBase never changes: relative session bindings remain anchored to + // the process's originally captured startup directory after reconfiguration. + startupBase string + defaultProject string + defaultErr error + sharedErr error } // Manager is a lightweight view over shared skill source snapshots. An empty // projectDir denotes the root handle, whose selected project follows the -// state's default. A bound handle keeps its logical project path for life. +// state's current default. A bound handle keeps its logical project path. type Manager struct { state *managerState projectDir string @@ -47,19 +49,21 @@ type Manager struct { // project sources. Options are cloned so caller mutations cannot reconfigure it. func NewManager(opts Options) *Manager { opts = cloneOptions(opts) - startupDir, defaultErr := normalizeStartupProject(opts.ProjectDir) - opts.ProjectDir = startupDir + defaultProject, defaultErr := normalizeStartupProject(opts.ProjectDir) + opts.ProjectDir = defaultProject state := &managerState{ - opts: opts, - bundled: map[string]*Skill{}, - user: map[string]*Skill{}, - configured: map[string]*Skill{}, - projects: map[string]map[string]*Skill{}, - projectErrs: map[string]error{}, - scopes: map[string]*Manager{}, - usage: map[string]int{}, - startupDir: startupDir, - defaultErr: defaultErr, + opts: opts, + bundled: map[string]*Skill{}, + user: map[string]*Skill{}, + configured: map[string]*Skill{}, + projects: map[string]map[string]*Skill{}, + projectErrs: map[string]error{}, + scopes: map[string]*Manager{}, + usage: map[string]int{}, + cache: map[string]cachedSkillFile{}, + startupBase: defaultProject, + defaultProject: defaultProject, + defaultErr: defaultErr, } state.root = &Manager{state: state} state.sharedOnly = &Manager{state: state, sharedOnly: true} @@ -86,10 +90,25 @@ func normalizeStartupProject(projectDir string) (string, error) { return filepath.Clean(logical), nil } +func normalizeReconfiguredProject(projectDir, startupBase string) (string, error) { + if strings.TrimSpace(projectDir) == "" { + return "", nil + } + if strings.IndexByte(projectDir, 0) >= 0 { + return "", fmt.Errorf("normalize project directory: path contains NUL") + } + if filepath.IsAbs(projectDir) { + return filepath.Clean(projectDir), nil + } + if startupBase == "" { + return "", fmt.Errorf("normalize relative project directory %q: startup project directory is unavailable", projectDir) + } + return filepath.Clean(filepath.Join(startupBase, projectDir)), nil +} + // ForProject returns a lightweight catalogue view bound to projectDir. Relative -// paths resolve against the captured startup project. A normalization failure -// returns a shared-only view so callers never accidentally observe the startup -// or another project's skills. +// paths resolve against the originally captured startup project. A normalization +// failure returns a shared-only view rather than another project's catalogue. func (m *Manager) ForProject(projectDir string) (*Manager, error) { if m == nil { return nil, nil @@ -101,8 +120,8 @@ func (m *Manager) ForProject(projectDir string) (*Manager, error) { if err == nil { err = m.state.sharedErr } - if err == nil && m.state.startupDir != "" { - err = m.state.projectErrs[m.state.startupDir] + if err == nil && m.state.defaultProject != "" { + err = m.state.projectErrs[m.state.defaultProject] } m.state.mu.RUnlock() return root, err @@ -113,8 +132,8 @@ func (m *Manager) ForProject(projectDir string) (*Manager, error) { return m.state.sharedOnly, err } - m.state.scanMu.Lock() - defer m.state.scanMu.Unlock() + // Registered scopes are the common path. Avoid queueing behind an active + // filesystem scan when their immutable snapshot can be returned immediately. m.state.mu.RLock() view, registered := m.state.scopes[logical] registeredErr := m.state.sharedErr @@ -126,12 +145,29 @@ func (m *Manager) ForProject(projectDir string) (*Manager, error) { return view, registeredErr } + m.state.scanMu.Lock() + defer m.state.scanMu.Unlock() + // Another first-use registration may have completed while this caller waited. + m.state.mu.RLock() + view, registered = m.state.scopes[logical] + registeredErr = m.state.sharedErr + if registeredErr == nil { + registeredErr = m.state.projectErrs[logical] + } + previous := m.state.cache + m.state.mu.RUnlock() + if registered { + return view, registeredErr + } + view = &Manager{state: m.state, projectDir: logical} - found, scanErr := discoverProject(logical) + scan := newDiscoveryScan(context.Background(), false, previous, true) + found, scanErr := discoverProject(scan, logical) m.state.mu.Lock() m.state.projects[logical] = found m.state.projectErrs[logical] = scanErr m.state.scopes[logical] = view + m.state.cache = scan.cache() sharedErr := m.state.sharedErr m.state.mu.Unlock() if sharedErr != nil { @@ -148,78 +184,12 @@ func (m *Manager) normalizeProject(projectDir string) (string, error) { return filepath.Clean(projectDir), nil } m.state.mu.RLock() - startupDir := m.state.startupDir + startupBase := m.state.startupBase m.state.mu.RUnlock() - if startupDir == "" { + if startupBase == "" { return "", fmt.Errorf("normalize relative project directory %q: startup project directory is unavailable", projectDir) } - return filepath.Clean(filepath.Join(startupDir, projectDir)), nil -} - -// Reload rescans shared sources once and every registered logical project. All -// successful partial snapshots are published atomically; usage counters remain -// shared and name-keyed. -func (m *Manager) Reload() error { - if m == nil { - return nil - } - m.state.scanMu.Lock() - defer m.state.scanMu.Unlock() - return m.reloadLocked() -} - -// reloadLocked requires scanMu. Mutation methods use it after writing so they -// do not recursively acquire the scan lock. -func (m *Manager) reloadLocked() error { - state := m.state - state.mu.RLock() - opts := cloneOptions(state.opts) - projectDirs := make([]string, 0, len(state.projects)) - for projectDir := range state.projects { - projectDirs = append(projectDirs, projectDir) - } - startupDir := state.startupDir - defaultErr := state.defaultErr - state.mu.RUnlock() - if startupDir != "" { - registered := false - for _, projectDir := range projectDirs { - if projectDir == startupDir { - registered = true - break - } - } - if !registered { - projectDirs = append(projectDirs, startupDir) - } - } - sort.Strings(projectDirs) - - bundled, user, configured, sharedErr := discoverShared(opts) - projects := make(map[string]map[string]*Skill, len(projectDirs)) - projectErrs := make(map[string]error, len(projectDirs)) - firstErr := defaultErr - if firstErr == nil { - firstErr = sharedErr - } - for _, projectDir := range projectDirs { - found, err := discoverProject(projectDir) - projects[projectDir] = found - projectErrs[projectDir] = err - if firstErr == nil && err != nil { - firstErr = err - } - } - - state.mu.Lock() - state.bundled = bundled - state.user = user - state.configured = configured - state.projects = projects - state.projectErrs = projectErrs - state.sharedErr = sharedErr - state.mu.Unlock() - return firstErr + return filepath.Clean(filepath.Join(startupBase, projectDir)), nil } func (m *Manager) selectedProjectLocked() map[string]*Skill { @@ -228,7 +198,7 @@ func (m *Manager) selectedProjectLocked() map[string]*Skill { } projectDir := m.projectDir if projectDir == "" { - projectDir = m.state.startupDir + projectDir = m.state.defaultProject } if projectDir == "" { return nil From 3dbcab05e82d56245ad91ec154d7b6158e98851e Mon Sep 17 00:00:00 2001 From: Reidho Satria Date: Wed, 16 Sep 2026 16:41:14 +0700 Subject: [PATCH 4/9] feat(web): show live discovered skills as read-only --- README.md | 9 +- docs/api.md | 6 + docs/skills.md | 7 +- internal/server/handlers_subsystems.go | 18 ++- internal/server/skills_test.go | 152 ++++++++++++++++++++ web/src/lib/i18n.tsx | 3 + web/src/pages/SkillsPage.tsx | 192 ++++++++++++++++--------- 7 files changed, 310 insertions(+), 77 deletions(-) diff --git a/README.md b/README.md index fcd5271..20ef3ff 100644 --- a/README.md +++ b/README.md @@ -279,10 +279,11 @@ dimensions and larger corpora are where the graph earns its keep. With knowledge into every turn; a project session can index its whole folder and keep it fresh as files change. -**Skills.** Markdown files with YAML front matter in `~/.antares/skills`. The -agent writes its own after solving something non-obvious; the catalogue (names -and descriptions only) goes in the prompt, and full bodies are fetched on demand -so the context stays small. +**Skills.** Markdown procedures in configured Antares directories and twelve +conventional user/project locations, discovered and refreshed automatically. +Imported sources are read-only through skill management; project chats use their +own catalog. Names and descriptions go in the prompt; full bodies are fetched on +demand. See [docs/skills.md](docs/skills.md) for paths, precedence, and refresh timing. **Scheduling.** A five-field cron parser plus `@daily`/`@every 90m` shorthands. Jobs are natural-language prompts that run unattended and can deliver their diff --git a/docs/api.md b/docs/api.md index 7b79e42..7db4d9c 100644 --- a/docs/api.md +++ b/docs/api.md @@ -130,6 +130,12 @@ not a transport error. | `POST /api/rag/search` | Query | | `DELETE /api/rag/collections/{name}` | Drop a collection | +Skill list/get responses include `read_only`. Automatically discovered skills can +be read, but save, toggle, and delete return HTTP 403 without changing the source +or creating a configured override. These endpoints use the startup catalog; +`POST /api/commands/run` with `/skills` and a `session_id` uses that session's +persisted project binding. See [skill sources and precedence](skills.md#where-they-live). + ## Scheduling and channels | | | diff --git a/docs/skills.md b/docs/skills.md index c7d234b..b42701e 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -137,8 +137,11 @@ learned it says so and writes nothing. checks. A project session sees shared user/configured skills and its own project skills, not the startup project's or another chat project's procedures. -The dashboard's Skills page lists them with a switch each, shows the body -inline, and has a Browse button for the hub. +The dashboard's Skills page shows the startup catalog and polls every five seconds +while visible. Imported cards show a Read-only badge and open a viewer with the +source path and procedure; their toggle is disabled and editing/deletion controls +are omitted. Close and reopen the viewer to read a refreshed body. Polling does +not replace an unsaved draft in a writable skill editor. Browse opens the hub. ```yaml skills: diff --git a/internal/server/handlers_subsystems.go b/internal/server/handlers_subsystems.go index a39f420..90901e5 100644 --- a/internal/server/handlers_subsystems.go +++ b/internal/server/handlers_subsystems.go @@ -64,7 +64,11 @@ func (s *Server) handleToggleSkill(w http.ResponseWriter, r *http.Request) { return } if err := mgr.SetEnabled(body.Name, body.Enabled); err != nil { - writeError(w, http.StatusBadRequest, err) + status := http.StatusBadRequest + if errors.Is(err, skills.ErrReadOnly) { + status = http.StatusForbidden + } + writeError(w, status, err) return } writeJSON(w, http.StatusOK, map[string]bool{"ok": true}) @@ -102,7 +106,11 @@ func (s *Server) handleSaveSkill(w http.ResponseWriter, r *http.Request) { } sk, err := mgr.Save(body.Name, body.Description, body.Body, body.Tags) if err != nil { - writeError(w, http.StatusBadRequest, err) + status := http.StatusBadRequest + if errors.Is(err, skills.ErrReadOnly) { + status = http.StatusForbidden + } + writeError(w, status, err) return } writeJSON(w, http.StatusOK, sk) @@ -115,7 +123,11 @@ func (s *Server) handleDeleteSkill(w http.ResponseWriter, r *http.Request) { return } if err := mgr.Delete(r.PathValue("name")); err != nil { - writeError(w, http.StatusBadRequest, err) + status := http.StatusBadRequest + if errors.Is(err, skills.ErrReadOnly) { + status = http.StatusForbidden + } + writeError(w, status, err) return } writeJSON(w, http.StatusOK, map[string]bool{"deleted": true}) diff --git a/internal/server/skills_test.go b/internal/server/skills_test.go index c7c3acb..b0b01fc 100644 --- a/internal/server/skills_test.go +++ b/internal/server/skills_test.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "encoding/json" "fmt" "net/http" @@ -30,6 +31,157 @@ func decodeServerJSON(t *testing.T, rr *httptest.ResponseRecorder, dst any) { } } +func TestSkillHTTPReadOnly(t *testing.T) { + home := t.TempDir() + configured := t.TempDir() + importedDir := filepath.Join(home, ".agent", "skills", "imported") + if err := os.MkdirAll(importedDir, 0o755); err != nil { + t.Fatal(err) + } + importedPath := filepath.Join(importedDir, "SKILL.md") + importedBytes := []byte("---\nname: imported\ndescription: Imported description\nenabled: true\n---\nIMPORTED_BODY\n") + if err := os.WriteFile(importedPath, importedBytes, 0o644); err != nil { + t.Fatal(err) + } + + mgr := skills.NewManager(skills.Options{Dirs: []string{configured}, UserHome: home}) + if err := mgr.Reload(); err != nil { + t.Fatal(err) + } + s := &Server{skills: mgr} + + list := httptest.NewRecorder() + s.handleListSkills(list, httptest.NewRequest(http.MethodGet, "/api/skills", nil)) + if list.Code != http.StatusOK { + t.Fatalf("list status = %d, want 200; body=%q", list.Code, list.Body.String()) + } + var listed struct { + Skills []skills.Skill `json:"skills"` + } + decodeServerJSON(t, list, &listed) + if len(listed.Skills) != 1 || listed.Skills[0].Name != "imported" || !listed.Skills[0].ReadOnly { + t.Fatalf("list did not expose imported skill as read-only: %+v", listed.Skills) + } + + getSkill := func(name string) (*httptest.ResponseRecorder, struct { + Skill skills.Skill `json:"skill"` + Body string `json:"body"` + }) { + t.Helper() + rr := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/skills/"+name, nil) + req.SetPathValue("name", name) + s.handleGetSkill(rr, req) + var response struct { + Skill skills.Skill `json:"skill"` + Body string `json:"body"` + } + if rr.Code == http.StatusOK { + decodeServerJSON(t, rr, &response) + } + return rr, response + } + + get, fetched := getSkill("imported") + if get.Code != http.StatusOK { + t.Fatalf("get imported status = %d, want 200; body=%q", get.Code, get.Body.String()) + } + if fetched.Skill.Name != "imported" || !fetched.Skill.ReadOnly || fetched.Body != "IMPORTED_BODY" { + t.Fatalf("get did not return imported body and read-only metadata: %+v body=%q", fetched.Skill, fetched.Body) + } + + readonlyRequests := []struct { + name string + run func(*httptest.ResponseRecorder) + }{ + { + name: "save", + run: func(rr *httptest.ResponseRecorder) { + req := httptest.NewRequest(http.MethodPost, "/api/skills", strings.NewReader(`{"name":"imported","description":"changed","body":"CHANGED","tags":[]}`)) + s.handleSaveSkill(rr, req) + }, + }, + { + name: "toggle", + run: func(rr *httptest.ResponseRecorder) { + req := httptest.NewRequest(http.MethodPost, "/api/skills/toggle", strings.NewReader(`{"name":"imported","enabled":false}`)) + s.handleToggleSkill(rr, req) + }, + }, + { + name: "delete", + run: func(rr *httptest.ResponseRecorder) { + req := httptest.NewRequest(http.MethodDelete, "/api/skills/imported", nil) + req.SetPathValue("name", "imported") + s.handleDeleteSkill(rr, req) + }, + }, + } + for _, mutation := range readonlyRequests { + rr := httptest.NewRecorder() + mutation.run(rr) + if rr.Code != http.StatusForbidden { + t.Fatalf("%s imported status = %d, want 403; body=%q", mutation.name, rr.Code, rr.Body.String()) + } + } + after, err := os.ReadFile(importedPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, importedBytes) { + t.Fatalf("read-only mutations changed imported bytes: got %q", after) + } + configuredEntries, err := os.ReadDir(configured) + if err != nil { + t.Fatal(err) + } + if len(configuredEntries) != 0 { + t.Fatalf("read-only save created a configured shadow: %+v", configuredEntries) + } + + save := httptest.NewRecorder() + s.handleSaveSkill(save, httptest.NewRequest(http.MethodPost, "/api/skills", strings.NewReader(`{"name":"writable","description":"Writable description","body":"WRITABLE_BODY","tags":["local"]}`))) + if save.Code != http.StatusOK { + t.Fatalf("create writable status = %d, want 200; body=%q", save.Code, save.Body.String()) + } + created, createdBody := getSkill("writable") + if created.Code != http.StatusOK || createdBody.Skill.ReadOnly || createdBody.Body != "WRITABLE_BODY" { + t.Fatalf("created writable skill is not editable: status=%d skill=%+v body=%q", created.Code, createdBody.Skill, createdBody.Body) + } + + edit := httptest.NewRecorder() + s.handleSaveSkill(edit, httptest.NewRequest(http.MethodPost, "/api/skills", strings.NewReader(`{"name":"writable","description":"Edited description","body":"EDITED_BODY","tags":[]}`))) + if edit.Code != http.StatusOK { + t.Fatalf("edit writable status = %d, want 200; body=%q", edit.Code, edit.Body.String()) + } + edited, editedBody := getSkill("writable") + if edited.Code != http.StatusOK || editedBody.Body != "EDITED_BODY" { + t.Fatalf("edit writable did not persist: status=%d body=%q", edited.Code, editedBody.Body) + } + + toggle := httptest.NewRecorder() + s.handleToggleSkill(toggle, httptest.NewRequest(http.MethodPost, "/api/skills/toggle", strings.NewReader(`{"name":"writable","enabled":false}`))) + if toggle.Code != http.StatusOK { + t.Fatalf("toggle writable status = %d, want 200; body=%q", toggle.Code, toggle.Body.String()) + } + toggled, toggledBody := getSkill("writable") + if toggled.Code != http.StatusOK || toggledBody.Skill.Enabled { + t.Fatalf("toggle writable did not persist: status=%d skill=%+v", toggled.Code, toggledBody.Skill) + } + + deleted := httptest.NewRecorder() + deleteReq := httptest.NewRequest(http.MethodDelete, "/api/skills/writable", nil) + deleteReq.SetPathValue("name", "writable") + s.handleDeleteSkill(deleted, deleteReq) + if deleted.Code != http.StatusOK { + t.Fatalf("delete writable status = %d, want 200; body=%q", deleted.Code, deleted.Body.String()) + } + missing, _ := getSkill("writable") + if missing.Code != http.StatusNotFound { + t.Fatalf("deleted writable status = %d, want 404; body=%q", missing.Code, missing.Body.String()) + } +} + func TestSkillConsumersSeeInPlaceReconfigure(t *testing.T) { t.Setenv("ANTARES_HOME", t.TempDir()) diff --git a/web/src/lib/i18n.tsx b/web/src/lib/i18n.tsx index 19893a8..051b710 100644 --- a/web/src/lib/i18n.tsx +++ b/web/src/lib/i18n.tsx @@ -662,6 +662,9 @@ const en = { 'skills.compose': 'Write a skill', 'skills.on': 'On', 'skills.off': 'Off', + 'skills.readOnly': 'Read-only', + 'skills.discoveredReadOnly': + 'Discovered from another skill directory. Edit the source file to change this skill.', 'skills.nameLocked': 'The name is the file id and cannot be changed.', 'skills.composeDesc': 'A reusable procedure. Be concrete — exact commands, paths, and pitfalls.', 'skills.name': 'Name', diff --git a/web/src/pages/SkillsPage.tsx b/web/src/pages/SkillsPage.tsx index e80074e..16faa53 100644 --- a/web/src/pages/SkillsPage.tsx +++ b/web/src/pages/SkillsPage.tsx @@ -9,7 +9,7 @@ import { TrashSimple, } from '@phosphor-icons/react' import { del, get, post } from '@/lib/api' -import { useApi } from '@/lib/hooks' +import { usePoll } from '@/lib/hooks' import { useI18n } from '@/lib/i18n' import { cn } from '@/lib/utils' import { PageLayout } from '@/components/layout/PageLayout' @@ -45,6 +45,7 @@ interface Skill { path: string enabled: boolean source: string + read_only: boolean tags?: string[] triggers?: string[] updated_at: string @@ -56,9 +57,7 @@ export default function SkillsPage() { const [filter, setFilter] = useState('') const [query, setQuery] = useState('') const endpoint = query ? `/skills?q=${encodeURIComponent(query)}` : '/skills' - const { data, loading, reload } = useApi<{ skills: Skill[]; library?: number }>(endpoint, [ - endpoint, - ]) + const { data, loading, reload } = usePoll<{ skills: Skill[]; library?: number }>(endpoint, 5000) const [busy, setBusy] = useState('') const [browsing, setBrowsing] = useState(false) const [editing, setEditing] = useState(null) @@ -179,11 +178,14 @@ export default function SkillsPage() { + {!s.read_only ? ( + + ) : null} ))} @@ -231,27 +235,42 @@ function SkillEditor({ }) { const { t } = useI18n() const isNew = !skill + const readOnly = !!skill?.read_only const [draft, setDraft] = useState({ name: skill?.name ?? '', description: skill?.description ?? '', body: '', }) + const [fullSkill, setFullSkill] = useState(skill) const [saving, setSaving] = useState(false) const [error, setError] = useState() - // Load the body when editing (the list omits it). + // The list omits the body. Imported skills also use the fetched metadata so + // the viewer reflects the source as it existed when it was opened. useEffect(() => { if (!skill) return let cancelled = false - get<{ body: string }>(`/skills/${encodeURIComponent(skill.name)}`) + get<{ skill: Skill; body: string }>(`/skills/${encodeURIComponent(skill.name)}`) .then((r) => { - if (!cancelled) setDraft((d) => ({ ...d, body: r.body })) + if (cancelled) return + if (readOnly) { + setFullSkill(r.skill) + setDraft({ + name: r.skill.name, + description: r.skill.description, + body: r.body, + }) + } else { + setDraft((d) => ({ ...d, body: r.body })) + } + }) + .catch((e: Error) => { + if (!cancelled) setError(e.message) }) - .catch(() => {}) return () => { cancelled = true } - }, [skill]) + }, [readOnly, skill]) const save = async () => { if (!draft.name.trim() || !draft.body.trim()) return @@ -274,44 +293,82 @@ function SkillEditor({ -
-
- - setDraft((d) => ({ ...d, name: e.target.value }))} - placeholder="deploy-homeserver" - /> - {!isNew ?

{t('skills.nameLocked')}

: null} -
-
- - setDraft((d) => ({ ...d, description: e.target.value }))} - placeholder={t('skills.whenToUsePlaceholder')} - /> -
-
-
- -