diff --git a/README.md b/README.md index fcd5271..a895319 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 content is read-only; enable/disable preferences live in Antares config. +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/cmd/antares/main.go b/cmd/antares/main.go index 4468e1c..eeb91c0 100644 --- a/cmd/antares/main.go +++ b/cmd/antares/main.go @@ -195,16 +195,20 @@ 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 + skillsCancel context.CancelFunc + skillsDone chan struct{} } func bootstrap(ctx context.Context) (*runtimeServices, error) { @@ -215,6 +219,10 @@ func bootstrap(ctx context.Context) (*runtimeServices, error) { if err != nil { return nil, err } + cfg, err = migrateSkillState(cfg) + if err != nil { + return nil, err + } if err := logx.Setup(cfg.Logging.Level, cfg.Logging.File, cfg.Logging.JSON); err != nil { return nil, fmt.Errorf("setting up logging: %w", err) } @@ -274,9 +282,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 +319,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) @@ -328,6 +348,7 @@ func bootstrap(ctx context.Context) (*runtimeServices, error) { } } + rt.startSkillRefresh(ctx) return rt, nil } @@ -697,6 +718,10 @@ func (rt *runtimeServices) reload() error { if err != nil { return err } + cfg, err = migrateSkillState(cfg) + if err != nil { + return err + } cfg, _ = config.Effective(rt.cfg, cfg) previous := rt.cfg if err := cfg.Server.ValidateListen(); err != nil { @@ -719,13 +744,12 @@ 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}) - if err := rt.skills.Reload(); err != nil { + if err := rt.skills.Reconfigure(skills.Options{ + Dirs: expandAll(cfg.Skills.Dirs), PackDirs: []string{packDir}, + UserHome: rt.skillsHome, ProjectDir: rt.skillsProjectDir, + }); 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)) @@ -756,7 +780,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.go b/cmd/antares/skills.go new file mode 100644 index 0000000..bd72dce --- /dev/null +++ b/cmd/antares/skills.go @@ -0,0 +1,24 @@ +package main + +import ( + "fmt" + + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/skills" +) + +// migrateSkillState imports only the selected configured sources, once per profile. +func migrateSkillState(cfg *config.Config) (*config.Config, error) { + if cfg.Skills.FrontmatterMigrated { + return cfg, nil + } + manager := skills.NewManager(skills.Options{Dirs: expandAll(cfg.Skills.Dirs)}) + if err := manager.Reload(); err != nil { + return nil, fmt.Errorf("migrate skill preferences: %w", err) + } + fresh, err := config.MigrateSkillState(manager.LegacyDisabled()) + if err != nil { + return nil, fmt.Errorf("migrate skill preferences: %w", err) + } + return fresh, nil +} diff --git a/cmd/antares/skills_migration_test.go b/cmd/antares/skills_migration_test.go new file mode 100644 index 0000000..f7c1808 --- /dev/null +++ b/cmd/antares/skills_migration_test.go @@ -0,0 +1,275 @@ +package main + +import ( + "bytes" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/skills" +) + +func skillMigrationConfig(t *testing.T) *config.Config { + t.Helper() + home := t.TempDir() + t.Setenv("ANTARES_HOME", home) + t.Setenv("ANTARES_CONFIG", filepath.Join(home, "config.yaml")) + t.Setenv("ANTARES_PROFILE", "default") + cfg := config.Default() + cfg.Skills.Dirs = []string{filepath.Join(home, "skills")} + if err := os.MkdirAll(cfg.Skills.Dirs[0], 0o700); err != nil { + t.Fatal(err) + } + if err := config.Save(cfg); err != nil { + t.Fatal(err) + } + return cfg +} + +func migrationSource(t *testing.T, dir, file, name, enabled string) string { + t.Helper() + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, file) + if err := os.WriteFile(path, []byte("---\nname: "+name+"\ndescription: fixture\nenabled: "+enabled+"\n---\nBODY\n"), 0o600); err != nil { + t.Fatal(err) + } + return path +} + +func TestMigrateSkillStateOnceAndSelectedSources(t *testing.T) { + cfg := skillMigrationConfig(t) + cfg.Skills.Disabled = []string{"missing"} + second := t.TempDir() + cfg.Skills.Dirs = append(cfg.Skills.Dirs, second) + path := migrationSource(t, cfg.Skills.Dirs[0], "legacy.md", "legacy", "false") + migrationSource(t, cfg.Skills.Dirs[0], "duplicate.md", "duplicate", "false") + migrationSource(t, second, "duplicate.md", "duplicate", "true") + migrationSource(t, config.Path("security-skills"), "pack.md", "automatic-pack", "false") + before, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if err := config.Save(cfg); err != nil { + t.Fatal(err) + } + cfg, err = migrateSkillState(cfg) + if err != nil { + t.Fatal(err) + } + if !cfg.Skills.FrontmatterMigrated || !reflect.DeepEqual(cfg.Skills.Disabled, []string{"legacy", "missing"}) { + t.Fatalf("migration = %+v", cfg.Skills) + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Fatal("migration rewrote legacy source") + } + cfg, err = config.SetSkillEnabled("legacy", true) + if err != nil { + t.Fatal(err) + } + migrationSource(t, cfg.Skills.Dirs[0], "later.md", "later", "false") + cfg, err = config.Reload() + if err != nil { + t.Fatal(err) + } + cfg, err = migrateSkillState(cfg) + if err != nil { + t.Fatal(err) + } + m := skills.NewManager(skills.Options{Dirs: expandAll(cfg.Skills.Dirs)}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + m.SetDisabled(cfg.Skills.Disabled) + for _, name := range []string{"legacy", "later"} { + s, ok := m.Get(name) + if !ok || !s.Enabled { + t.Fatalf("%s disabled by stale header after migration: %+v", name, s) + } + } + // The completed marker bypasses even a now-malformed configured source. + if err := os.WriteFile(path, []byte("---\nenabled: [\n---\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := migrateSkillState(cfg); err != nil { + t.Fatalf("completed migration rescanned: %v", err) + } +} + +func TestMigrateSkillStateEmptyAndExplicitPack(t *testing.T) { + cfg := skillMigrationConfig(t) + cfg.Skills.Dirs = append(cfg.Skills.Dirs, filepath.Join(t.TempDir(), "missing")) + if err := config.Save(cfg); err != nil { + t.Fatal(err) + } + fresh, err := migrateSkillState(cfg) + if err != nil { + t.Fatal(err) + } + if !fresh.Skills.FrontmatterMigrated || len(fresh.Skills.Disabled) != 0 { + t.Fatalf("empty migration = %+v", fresh.Skills) + } + // Explicitly configured bundled paths are configured sources, not excluded by location. + cfg.Skills.Dirs = []string{config.Path("security-skills")} + migrationSource(t, cfg.Skills.Dirs[0], "explicit.md", "explicit", "false") + if err := config.Save(cfg); err != nil { + t.Fatal(err) + } + fresh, err = migrateSkillState(cfg) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(fresh.Skills.Disabled, []string{"explicit"}) { + t.Fatalf("explicit import = %v", fresh.Skills.Disabled) + } +} + +func TestMigrateSkillStateIncompleteRetry(t *testing.T) { + cfg := skillMigrationConfig(t) + migrationSource(t, cfg.Skills.Dirs[0], "good.md", "good", "false") + broken := filepath.Join(cfg.Skills.Dirs[0], "broken.md") + if err := os.WriteFile(broken, []byte("---\nenabled: [\n---\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := migrateSkillState(cfg); err == nil || !strings.Contains(err.Error(), "migrate skill preferences:") { + t.Fatalf("incomplete migration error = %v", err) + } + fresh, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if fresh.Skills.FrontmatterMigrated { + t.Fatal("partial migration marked complete") + } + migrationSource(t, cfg.Skills.Dirs[0], "broken.md", "repaired", "false") + fresh, err = migrateSkillState(fresh) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(fresh.Skills.Disabled, []string{"good", "repaired"}) { + t.Fatalf("retry import = %v", fresh.Skills.Disabled) + } +} + +func TestMigrateSkillStateSaveFailureLeavesMarkerUnset(t *testing.T) { + cfg := skillMigrationConfig(t) + migrationSource(t, cfg.Skills.Dirs[0], "legacy.md", "legacy", "false") + // The file remains readable, but atomic replacement needs directory write permission. + original, err := os.ReadFile(config.ConfigFile()) + if err != nil { + t.Fatal(err) + } + if err := os.Chmod(filepath.Dir(config.ConfigFile()), 0o500); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(filepath.Dir(config.ConfigFile()), 0o700) }) + if os.Geteuid() == 0 { + t.Skip("permission failure requires an unprivileged process; config package covers deterministic write failure") + } + if _, err := migrateSkillState(cfg); err == nil { + t.Fatal("migration succeeded despite unwritable config directory") + } + after, err := os.ReadFile(config.ConfigFile()) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(original, after) || cfg.Skills.FrontmatterMigrated { + t.Fatal("failed migration changed persisted marker") + } +} + +func TestMigrateSkillStateUnreadableSourceRetry(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("permission semantics require an unprivileged process") + } + for _, rootUnreadable := range []bool{false, true} { + t.Run(map[bool]string{false: "file", true: "root"}[rootUnreadable], func(t *testing.T) { + cfg := skillMigrationConfig(t) + p := migrationSource(t, cfg.Skills.Dirs[0], "legacy.md", "legacy", "false") + if rootUnreadable { + p = cfg.Skills.Dirs[0] + } + if err := os.Chmod(p, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(p, 0o700) }) + if _, err := migrateSkillState(cfg); err == nil { + t.Fatal("unreadable source completed migration") + } + fresh, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if fresh.Skills.FrontmatterMigrated { + t.Fatal("incomplete import marked complete") + } + if err := os.Chmod(p, 0o700); err != nil { + t.Fatal(err) + } + fresh, err = migrateSkillState(fresh) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(fresh.Skills.Disabled, []string{"legacy"}) { + t.Fatalf("retry lost opt-out: %v", fresh.Skills.Disabled) + } + }) + } +} + +func TestRuntimeReloadAbortsIncompleteSkillMigration(t *testing.T) { + cfg := skillMigrationConfig(t) + if err := os.WriteFile(filepath.Join(cfg.Skills.Dirs[0], "bad.md"), []byte("---\nenabled: [\n---\n"), 0o600); err != nil { + t.Fatal(err) + } + rt := &runtimeServices{cfg: cfg.Clone()} + rt.cfg.Skills.FrontmatterMigrated = true + if err := rt.reload(); err == nil || !strings.Contains(err.Error(), "migrate skill preferences:") { + t.Fatalf("reload migration error = %v", err) + } + if !rt.cfg.Skills.FrontmatterMigrated { + t.Fatal("failed migration published replacement config") + } +} + +func TestMigrationDoesNotImportAutomaticBorrowedOptOut(t *testing.T) { + cfg := skillMigrationConfig(t) + home := t.TempDir() + t.Setenv("HOME", home) + borrowed := migrationSource(t, filepath.Join(home, ".agent", "skills", "borrowed"), "SKILL.md", "borrowed", "false") + before, err := os.ReadFile(borrowed) + if err != nil { + t.Fatal(err) + } + cfg, err = migrateSkillState(cfg) + if err != nil { + t.Fatal(err) + } + if len(cfg.Skills.Disabled) != 0 { + t.Fatalf("imported automatic opt-out: %v", cfg.Skills.Disabled) + } + m := skills.NewManager(skills.Options{Dirs: expandAll(cfg.Skills.Dirs), UserHome: home}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + m.SetDisabled(cfg.Skills.Disabled) + s, ok := m.Get("borrowed") + if !ok || !s.Enabled || !s.ReadOnly { + t.Fatalf("borrowed control: %+v", s) + } + after, err := os.ReadFile(borrowed) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(before, after) { + t.Fatal("borrowed source rewritten") + } +} 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/api.md b/docs/api.md index 7b79e42..9ce1c3a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -130,6 +130,14 @@ 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 and toggled; toggles persist exact names in `skills.disabled`, not source +files. Save and delete return HTTP 403 without changing borrowed content or +creating a configured override. Toggle persistence/reload failures return HTTP 500; +success follows both persistence and live publication. 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/configuration.md b/docs/configuration.md index aff74cd..a855969 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -234,6 +234,7 @@ which would leave the model reading a call with no answer. skills: enabled: true dirs: [~/.antares/skills] + disabled: [] auto_create: true creation_nudge_interval: 20 ``` @@ -241,6 +242,11 @@ skills: `dirs` is searched in order and later directories win, so a personal copy can override a shared one. See [Skills](skills.md). +`disabled` contains exact, case-sensitive names turned off for this profile, +including names whose files are temporarily absent. Dashboard switches update +this list without changing source files. Legacy configured opt-outs are imported +once; see [Managing skills](skills.md#managing-them). + ## Server ```yaml diff --git a/docs/skills.md b/docs/skills.md index 3afddb5..4ab6bf1 100644 --- a/docs/skills.md +++ b/docs/skills.md @@ -36,7 +36,6 @@ Port 8787 already in use usually means the old process did not exit. Check | `description` | **The most important line.** How the agent decides whether this is relevant | | `tags` | For your own browsing | | `triggers` | Words that make it more likely to surface | -| `enabled` | `false` keeps it on disk but out of the prompt | The description does the work. "Deployment stuff" will not get picked; "Deploy this project to the home server. Use when asked to deploy, ship, or release." @@ -47,6 +46,11 @@ will. Only names and descriptions go into the system prompt — the catalogue. Bodies are fetched on demand with the `skill` tool. + +Disabled names are omitted from new prompts and from the skill tool's list, +search, read, and chain results. Re-enabling restores access. Already-sent model +context cannot be retracted, and this preference does not restrict generic +filesystem tools. Saving skill content does not enable a disabled name. Twenty skills therefore cost a few hundred tokens per turn rather than tens of thousands, and adding more does not degrade the conversation. @@ -58,9 +62,47 @@ 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 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 +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 skill content is read-only through skill management: +save and delete refuse to modify or shadow it. The toggle API changes only Antares +configuration, including for these borrowed skills. Edit the original file to +change its content. +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. + +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 @@ -97,8 +139,26 @@ learned it says so and writes nothing. /skills deploy filter ``` -The dashboard's Skills page lists them with a switch each, shows the body -inline, and has a Browse button for the hub. +`/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 shows the startup catalog and polls every five seconds +while visible. Imported cards show a Read-only content badge and open a viewer +with the source path and procedure. Their switches update Antares configuration; +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. + +Switches save exact, case-sensitive skill names in `skills.disabled` in the active +profile's configuration; they never rewrite skill files. Preferences remain when +a file is removed or reinstalled. `skills.enabled` is the separate global gate. + +On the first startup with this setting, Antares imports `enabled: false` from +selected files in configured skill directories. It records completion in +`skills.frontmatter_migrated`. Later header changes do not affect enablement. +An unreadable or malformed configured source aborts that initial import; repair +the source and restart to retry. The dashboard and `/skills` still show off entries. ```yaml skills: diff --git a/internal/agent/agent.go b/internal/agent/agent.go index 00f7025..25236ae 100644 --- a/internal/agent/agent.go +++ b/internal/agent/agent.go @@ -248,8 +248,13 @@ func (a *Agent) SetConfig(cfg *config.Config) { if cfg == nil { return } + a.servicesMu.Lock() + if a.skills != nil { + a.skills.SetDisabled(cfg.Skills.Disabled) + } prev := a.cfg.Load() a.cfg.Store(cfg) + a.servicesMu.Unlock() // A raised MaxConcurrentSessions makes room for parked RunQueued // waiters immediately; without a wake here they would sit on the old // channel until an unrelated turn ended. @@ -274,6 +279,9 @@ func (a *Agent) SetRAG(p tools.RAGProvider) { // SetSkills attaches the skill library. Publishes under servicesMu. func (a *Agent) SetSkills(m *skills.Manager) { a.servicesMu.Lock() + if cfg := a.cfg.Load(); m != nil && cfg != nil { + m.SetDisabled(cfg.Skills.Disabled) + } a.skills = m a.servicesMu.Unlock() } 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/skill_state_test.go b/internal/agent/skill_state_test.go new file mode 100644 index 0000000..213204c --- /dev/null +++ b/internal/agent/skill_state_test.go @@ -0,0 +1,290 @@ +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 skillStateAgent(t *testing.T) (*Agent, *skills.Manager, string) { + t.Helper() + home := t.TempDir() + t.Setenv("ANTARES_HOME", home) + t.Setenv("ANTARES_CONFIG", filepath.Join(home, "config.yaml")) + t.Setenv("ANTARES_PROFILE", "default") + cfg := config.Default() + cfg.Skills.Enabled = true + cfg.Skills.FrontmatterMigrated = true + cfg.Memory.Enabled = false + cfg.Memory.UserProfileEnabled = false + cfg.RAG.Enabled = false + cfg.Agent.Workspace = home + cfg.Tools.ApprovalMode = "auto" + m := skills.NewManager(skills.Options{Dirs: []string{home}}) + a := agentWithConfig(cfg) + a.SetSkills(m) + return a, m, home +} + +func agentSkillSource(t *testing.T, dir, name, description, extra, body string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name+".md"), []byte("---\nname: "+name+"\ndescription: "+description+"\n"+extra+"---\n"+body+"\n"), 0o600); err != nil { + t.Fatal(err) + } +} + +func runStateSkill(t *testing.T, a *Agent, args map[string]any) (string, bool) { + t.Helper() + if len(args) <= 2 { + body, err := executeSessionSkill(a, &store.Session{ID: "state-session", Workspace: a.Config().Agent.Workspace}, args["action"].(string), fmt.Sprint(args["name"])) + if err != nil { + return err.Error(), true + } + return body, false + } + tool, ok := tools.Default().Get("skill") + if !ok { + t.Fatal("skill tool unavailable") + } + raw, err := json.Marshal(args) + if err != nil { + t.Fatal(err) + } + out := a.executeTools(context.Background(), []llm.ToolCall{{ID: "state", Name: "skill", Arguments: string(raw)}}, map[string]tools.Tool{"skill": tool}, Request{Platform: "web"}, &store.Session{ID: "state-session", Workspace: a.Config().Agent.Workspace}, func(Event) error { return nil }) + if len(out) != 1 { + t.Fatalf("tool outcomes = %d", len(out)) + } + return out[0].message.Content, out[0].isError +} + +func TestDisabledSkillReadAndUsage(t *testing.T) { + a, m, dir := skillStateAgent(t) + agentSkillSource(t, dir, "secret", "SECRET_DESCRIPTION", "", "SECRET_BODY") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + cfg := a.Config().Clone() + cfg.Skills.Disabled = []string{"secret"} + a.SetConfig(cfg) + body, isError := runStateSkill(t, a, map[string]any{"action": "read", "name": "secret"}) + if !isError || strings.Contains(body, "SECRET_BODY") || !strings.Contains(body, "not found") { + t.Fatalf("disabled read leaked or succeeded: error=%v body=%q", isError, body) + } + if s, _ := m.Get("secret"); s.UsageCount != 0 { + t.Fatalf("disabled read incremented usage to %d", s.UsageCount) + } + cfg = cfg.Clone() + cfg.Skills.Disabled = nil + a.SetConfig(cfg) + body, isError = runStateSkill(t, a, map[string]any{"action": "read", "name": "secret"}) + if isError || !strings.Contains(body, "SECRET_BODY") { + t.Fatalf("re-enabled read failed: error=%v body=%q", isError, body) + } + if s, _ := m.Get("secret"); s.UsageCount != 1 { + t.Fatalf("enabled read usage = %d", s.UsageCount) + } +} + +func TestDisabledSkillSearchBeforeLimit(t *testing.T) { + a, m, dir := skillStateAgent(t) + cfg := a.Config().Clone() + for i := range 35 { + name := fmt.Sprintf("needle-%02d", i) + agentSkillSource(t, dir, name, "DISABLED_DESCRIPTION", "tech_stack: [web]\ncwe_ids: [CWE-89]\n", "DISABLED_BODY") + cfg.Skills.Disabled = append(cfg.Skills.Disabled, name) + } + agentSkillSource(t, dir, "available", "needle ENABLED_DESCRIPTION", "tech_stack: [web]\ncwe_ids: [CWE-89]\n", "ENABLED_BODY") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + a.SetConfig(cfg) + for _, args := range []map[string]any{{"action": "search", "name": "needle"}, {"action": "search", "name": "needle", "tech": "web", "cwe": "89"}, {"action": "list"}} { + body, isError := runStateSkill(t, a, args) + if isError || strings.Contains(body, "DISABLED_DESCRIPTION") || !strings.Contains(body, "ENABLED_DESCRIPTION") { + t.Fatalf("disabled search/list displaced enabled match: args=%v error=%v body=%q", args, isError, body) + } + } + handle := a.skillLibrary(nil) + hits := handle.Search("needle", 30) + if len(hits) != 1 || hits[0].Name != "available" { + t.Fatalf("adapter search failed enabled-only limit: %+v", hits) + } + if hits := m.Search("needle", 100); len(hits) != 36 { + t.Fatalf("administrative search omitted disabled entries: %d", len(hits)) + } + if prompt := m.PromptBlock(0); strings.Contains(prompt, "DISABLED_DESCRIPTION") || !strings.Contains(prompt, "ENABLED_DESCRIPTION") { + t.Fatalf("prompt exposed disabled names: %s", prompt) + } +} + +func TestDisabledSkillChainsAndPackAccess(t *testing.T) { + a, m, dir := skillStateAgent(t) + packDir := t.TempDir() + m = skills.NewManager(skills.Options{Dirs: []string{dir}, PackDirs: []string{packDir}}) + a.SetSkills(m) + agentSkillSource(t, dir, "origin", "ORIGIN_DESCRIPTION", "chains_with: [off, on, pack]\n", "ORIGIN_BODY") + agentSkillSource(t, dir, "off", "OFF_DESCRIPTION", "", "OFF_BODY") + agentSkillSource(t, dir, "on", "ON_DESCRIPTION", "", "ON_BODY") + agentSkillSource(t, packDir, "pack", "PACK_DESCRIPTION", "", "PACK_BODY") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + cfg := a.Config().Clone() + cfg.Skills.Disabled = []string{"off"} + a.SetConfig(cfg) + body, isError := runStateSkill(t, a, map[string]any{"action": "chains", "name": "origin"}) + if isError || strings.Contains(body, "OFF_DESCRIPTION") || !strings.Contains(body, "ON_DESCRIPTION") || !strings.Contains(body, "PACK_DESCRIPTION") { + t.Fatalf("chain target filtering: error=%v body=%q", isError, body) + } + cfg = cfg.Clone() + cfg.Skills.Disabled = append(cfg.Skills.Disabled, "origin") + a.SetConfig(cfg) + body, _ = runStateSkill(t, a, map[string]any{"action": "chains", "name": "origin"}) + if strings.Contains(body, "ON_DESCRIPTION") || strings.Contains(body, "PACK_DESCRIPTION") { + t.Fatalf("disabled origin exposed targets: %q", body) + } + if len(m.Chains("origin")) != 3 { + t.Fatal("administrative chain targets were filtered") + } + for _, action := range []string{"read", "search"} { + body, isError = runStateSkill(t, a, map[string]any{"action": action, "name": "pack"}) + if isError || !strings.Contains(body, "PACK_") { + t.Fatalf("enabled pack %s failed: %q", action, body) + } + } + body, _ = runStateSkill(t, a, map[string]any{"action": "list"}) + if strings.Contains(body, "PACK_DESCRIPTION") { + t.Fatalf("pack leaked into everyday list: %q", body) + } +} + +func TestSkillHandleObservesConfigAndReplacement(t *testing.T) { + a, m, dir := skillStateAgent(t) + agentSkillSource(t, dir, "same", "DESCRIPTION", "", "ORIGINAL_BODY") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + handle := a.skillLibrary(nil) + cfg := a.Config().Clone() + cfg.Skills.Disabled = []string{"same"} + a.SetConfig(cfg) + if _, body, ok := handle.Read("same"); ok || body != "" { + t.Fatalf("retained handle missed config: ok=%v body=%q", ok, body) + } + cfg = cfg.Clone() + cfg.Skills.Disabled = nil + a.SetConfig(cfg) + if _, body, ok := handle.Read("same"); !ok || body != "ORIGINAL_BODY" { + t.Fatalf("retained handle did not re-enable: ok=%v body=%q", ok, body) + } + replacementDir := t.TempDir() + agentSkillSource(t, replacementDir, "same", "DESCRIPTION", "", "REPLACEMENT_BODY") + replacement := skills.NewManager(skills.Options{Dirs: []string{replacementDir}}) + if err := replacement.Reload(); err != nil { + t.Fatal(err) + } + a.SetSkills(replacement) + body, isError := runStateSkill(t, a, map[string]any{"action": "read", "name": "same"}) + if isError || !strings.Contains(body, "REPLACEMENT_BODY") { + t.Fatalf("new tool retained retired manager: %q", body) + } + cfg = cfg.Clone() + cfg.Skills.Disabled = []string{"same"} + a.SetConfig(cfg) + body, isError = runStateSkill(t, a, map[string]any{"action": "save", "name": "same", "description": "edited", "body": "EDITED_BODY with enough procedure details to be accepted by the real tool."}) + if isError { + t.Fatalf("content editing disabled skill failed: %q", body) + } + if s, _ := replacement.Get("same"); s.Enabled || !strings.Contains(s.Body, "EDITED_BODY") { + t.Fatalf("save reset preference or lost edit: %+v", s) + } +} + +func TestConcurrentSkillReadsAndConfigPublication(t *testing.T) { + a, m, dir := skillStateAgent(t) + agentSkillSource(t, dir, "same", "DESCRIPTION", "", "BODY") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + handle := a.skillLibrary(nil) + on := a.Config().Clone() + off := on.Clone() + off.Skills.Disabled = []string{"same"} + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + for range 200 { + a.SetConfig(off) + a.SetConfig(on) + } + }() + go func() { + defer wg.Done() + for range 200 { + handle.List() + handle.Read("same") + handle.Search("same", 1) + handle.Chains("same") + } + }() + wg.Wait() + a.SetConfig(off) + if _, _, ok := handle.Read("same"); ok { + t.Fatal("final disabled publication invisible to retained handle") + } +} + +func TestScopedSkillHandlesObserveDisabledReconfigure(t *testing.T) { + a, _, configured := skillStateAgent(t) + projectA, projectB := t.TempDir(), t.TempDir() + writeSessionSkill(t, projectA, "collision", "A_DESCRIPTION", "A_BODY") + writeSessionSkill(t, projectB, "collision", "B_DESCRIPTION", "B_BODY") + m := skills.NewManager(skills.Options{Dirs: []string{configured}}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + a.SetSkills(m) + sessions := []*store.Session{ + {ID: "a", Workspace: projectA, Meta: store.Meta{"project_dir": projectA}}, + {ID: "b", Workspace: projectB, Meta: store.Meta{"project_dir": projectB}}, + } + handles := []tools.SkillLibrary{a.skillLibrary(sessions[0]), a.skillLibrary(sessions[1])} + cfg := a.Config().Clone() + cfg.Skills.Disabled = []string{"collision"} + a.SetConfig(cfg) + if err := m.Reconfigure(skills.Options{Dirs: []string{configured}}); err != nil { + t.Fatal(err) + } + for i, sess := range sessions { + if _, body, ok := handles[i].Read("collision"); ok || body != "" { + t.Fatalf("bound handle leaked disabled body: %q", body) + } + if body, err := executeSessionSkill(a, sess, "read", "collision"); err == nil || strings.Contains(body, "_BODY") { + t.Fatalf("scoped disabled read: %q %v", body, err) + } + } + cfg = cfg.Clone() + cfg.Skills.Disabled = nil + a.SetConfig(cfg) + for i, sess := range sessions { + want := []string{"A_BODY", "B_BODY"}[i] + if _, body, ok := handles[i].Read("collision"); !ok || body != want { + t.Fatalf("retained scoped handle = %q want %q", body, want) + } + if body, err := executeSessionSkill(a, sess, "read", "collision"); err != nil || !strings.Contains(body, want) { + t.Fatalf("scoped enabled read: %q %v", body, err) + } + } +} diff --git a/internal/agent/skills.go b/internal/agent/skills.go index 8963a30..e07514b 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" ) @@ -27,20 +31,31 @@ func (a skillAdapter) List() []tools.SkillInfo { } func (a skillAdapter) Search(query string, limit int) []tools.SkillInfo { - return infos(a.m.Search(query, limit)) + return infos(a.m.SearchFiltered(query, skills.Filter{EnabledOnly: true}, limit)) } func (a skillAdapter) SearchFiltered(query, cwe, tech, category string, limit int) []tools.SkillInfo { - return infos(a.m.SearchFiltered(query, skills.Filter{CWE: cwe, Tech: tech, Category: category}, limit)) + return infos(a.m.SearchFiltered(query, skills.Filter{CWE: cwe, Tech: tech, Category: category, EnabledOnly: true}, limit)) } func (a skillAdapter) Chains(name string) []tools.SkillInfo { - return infos(a.m.Chains(name)) + origin, ok := a.m.Get(name) + if !ok || !origin.Enabled { + return nil + } + items := a.m.Chains(name) + out := make([]tools.SkillInfo, 0, len(items)) + for _, s := range items { + if s.Enabled { + out = append(out, toInfo(s)) + } + } + return out } func (a skillAdapter) Read(name string) (tools.SkillInfo, string, bool) { s, ok := a.m.Get(name) - if !ok { + if !ok || !s.Enabled { return tools.SkillInfo{}, "", false } return toInfo(*s), s.Body, true @@ -71,12 +86,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/config/config.go b/internal/config/config.go index e2e4555..5c04d98 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -643,6 +643,8 @@ type Plugins struct { type Skills struct { Enabled bool `yaml:"enabled" json:"enabled"` Dirs []string `yaml:"dirs" json:"dirs"` + Disabled []string `yaml:"disabled" json:"disabled"` + FrontmatterMigrated bool `yaml:"frontmatter_migrated" json:"frontmatter_migrated"` CreationNudgeInterval int `yaml:"creation_nudge_interval" json:"creation_nudge_interval"` AutoCreate bool `yaml:"auto_create" json:"auto_create"` HubSources []string `yaml:"hub_sources" json:"hub_sources"` diff --git a/internal/config/schema.go b/internal/config/schema.go index a815675..9e6ff7f 100644 --- a/internal/config/schema.go +++ b/internal/config/schema.go @@ -105,6 +105,7 @@ func tierFor(path string) string { // never match a login (a non-bcrypt string fails every comparison). var hidden = map[string]bool{ "server.dashboard_password_hash": true, + "skills.frontmatter_migrated": true, } var enums = map[string][]string{ diff --git a/internal/config/skills.go b/internal/config/skills.go new file mode 100644 index 0000000..be0c42d --- /dev/null +++ b/internal/config/skills.go @@ -0,0 +1,105 @@ +package config + +import ( + "errors" + "fmt" + "os" + "sort" + "strings" + "sync" + + "gopkg.in/yaml.v3" +) + +var skillStateMu sync.Mutex + +// MigrateSkillState imports legacy frontmatter opt-outs exactly once. Imported +// names are merged with preferences already present in the active profile. +func MigrateSkillState(disabled []string) (*Config, error) { + skillStateMu.Lock() + defer skillStateMu.Unlock() + + cfg, err := readPersistedConfig() + if err != nil { + return nil, err + } + if cfg.Skills.FrontmatterMigrated { + return Reload() + } + + cfg.Skills.Disabled = sortedUniqueNames(append(cfg.Skills.Disabled, disabled...)) + cfg.Skills.FrontmatterMigrated = true + return persistSkillState(cfg) +} + +// SetSkillEnabled updates one exact skill name in the active profile. Skill +// state must be migrated before requests can alter it, so a later migration +// cannot unexpectedly reapply stale frontmatter state. +func SetSkillEnabled(name string, enabled bool) (*Config, error) { + if strings.TrimSpace(name) == "" { + return nil, errors.New("skill name is required") + } + + skillStateMu.Lock() + defer skillStateMu.Unlock() + + cfg, err := readPersistedConfig() + if err != nil { + return nil, err + } + if !cfg.Skills.FrontmatterMigrated { + return nil, errors.New("skill preferences have not been migrated") + } + + if enabled { + kept := cfg.Skills.Disabled[:0] + for _, disabled := range cfg.Skills.Disabled { + if disabled != name { + kept = append(kept, disabled) + } + } + cfg.Skills.Disabled = kept + } else { + cfg.Skills.Disabled = append(cfg.Skills.Disabled, name) + } + cfg.Skills.Disabled = sortedUniqueNames(cfg.Skills.Disabled) + return persistSkillState(cfg) +} + +// readPersistedConfig deliberately bypasses Load and Reload: their result has +// environment overrides and normalized paths that must never be written back. +func readPersistedConfig() (*Config, error) { + path := ConfigFile() + raw, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("read %s: %w", path, err) + } + cfg := Default() + if err := yaml.Unmarshal(raw, cfg); err != nil { + return nil, fmt.Errorf("parse %s: %w", path, err) + } + return cfg, nil +} + +func persistSkillState(cfg *Config) (*Config, error) { + if err := writeFile(ConfigFile(), cfg); err != nil { + return nil, err + } + return Reload() +} + +func sortedUniqueNames(names []string) []string { + if len(names) == 0 { + return nil + } + sort.Strings(names) + write := 1 + for _, name := range names[1:] { + if name == names[write-1] { + continue + } + names[write] = name + write++ + } + return names[:write] +} diff --git a/internal/config/skills_test.go b/internal/config/skills_test.go new file mode 100644 index 0000000..c2a9c9d --- /dev/null +++ b/internal/config/skills_test.go @@ -0,0 +1,314 @@ +package config + +import ( + "errors" + "os" + "path/filepath" + "reflect" + "strings" + "sync" + "testing" + + "gopkg.in/yaml.v3" +) + +func TestMigrateSkillStateUnionsOnceWithoutPersistingRuntimeValues(t *testing.T) { + home := isolateConfigHome(t) + path := filepath.Join(home, "config.yaml") + writeSkillConfigFixture(t, path, `server: + host: 127.0.0.1 +agent: + workspace: $SKILL_WORKSPACE +skills: + enabled: false + disabled: [zeta, existing, zeta] + frontmatter_migrated: false +`) + t.Setenv("ANTARES_HOST", "0.0.0.0") + t.Setenv("SKILL_WORKSPACE", filepath.Join(home, "runtime-workspace")) + + cfg, err := MigrateSkillState([]string{"legacy", "existing", " Alpha ", "legacy"}) + if err != nil { + t.Fatalf("MigrateSkillState: %v", err) + } + wantDisabled := []string{" Alpha ", "existing", "legacy", "zeta"} + if !reflect.DeepEqual(cfg.Skills.Disabled, wantDisabled) { + t.Fatalf("runtime disabled = %#v, want %#v", cfg.Skills.Disabled, wantDisabled) + } + if !cfg.Skills.FrontmatterMigrated { + t.Fatal("runtime migration marker is false") + } + if cfg.Skills.Enabled { + t.Fatal("migration changed the independent global skills.enabled gate") + } + if cfg.Server.Host != "0.0.0.0" { + t.Fatalf("runtime server.host = %q, want environment override", cfg.Server.Host) + } + if cfg.Agent.Workspace != filepath.Join(home, "runtime-workspace") { + t.Fatalf("runtime agent.workspace = %q, want expanded environment path", cfg.Agent.Workspace) + } + + disk := readSkillConfigFixture(t, path) + if disk.Server.Host != "127.0.0.1" { + t.Fatalf("persisted server.host = %q, want operator value", disk.Server.Host) + } + if disk.Agent.Workspace != "$SKILL_WORKSPACE" { + t.Fatalf("persisted agent.workspace = %q, want unnormalized input", disk.Agent.Workspace) + } + if disk.Skills.Enabled { + t.Fatal("persisted migration changed skills.enabled") + } + if !disk.Skills.FrontmatterMigrated { + t.Fatal("persisted migration marker is false") + } + if !reflect.DeepEqual(disk.Skills.Disabled, wantDisabled) { + t.Fatalf("persisted disabled = %#v, want %#v", disk.Skills.Disabled, wantDisabled) + } + + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read migrated config: %v", err) + } + if err := os.Chmod(home, 0o500); err != nil { + t.Fatalf("make config directory read-only: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) + + cfg, err = MigrateSkillState([]string{"later"}) + if err != nil { + t.Fatalf("second MigrateSkillState should only reload: %v", err) + } + if containsExact(cfg.Skills.Disabled, "later") { + t.Fatal("one-time migration imported a name after the marker was set") + } + after, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config after second migration: %v", err) + } + if !reflect.DeepEqual(after, before) { + t.Fatal("already-completed migration rewrote config") + } +} + +func TestSetSkillEnabledPreservesExactAndStaleNames(t *testing.T) { + home := isolateConfigHome(t) + path := filepath.Join(home, "config.yaml") + writeSkillConfigFixture(t, path, `server: + host: 127.0.0.1 +skills: + enabled: false + disabled: [stale, Skill, " Skill ", Skill] + frontmatter_migrated: true +`) + t.Setenv("ANTARES_HOST", "0.0.0.0") + + cfg, err := SetSkillEnabled("Skill", true) + if err != nil { + t.Fatalf("enable Skill: %v", err) + } + want := []string{" Skill ", "stale"} + if !reflect.DeepEqual(cfg.Skills.Disabled, want) { + t.Fatalf("disabled after exact enable = %#v, want %#v", cfg.Skills.Disabled, want) + } + + cfg, err = SetSkillEnabled(" exact name ", false) + if err != nil { + t.Fatalf("disable exact spaced name: %v", err) + } + want = []string{" exact name ", " Skill ", "stale"} + if !reflect.DeepEqual(cfg.Skills.Disabled, want) { + t.Fatalf("disabled after exact disable = %#v, want %#v", cfg.Skills.Disabled, want) + } + if cfg.Skills.Enabled { + t.Fatal("per-skill update changed the independent global gate") + } + + cfg, err = SetSkillEnabled("stale", false) + if err != nil { + t.Fatalf("disable stale name again: %v", err) + } + if !reflect.DeepEqual(cfg.Skills.Disabled, want) { + t.Fatalf("duplicate disable changed set = %#v, want %#v", cfg.Skills.Disabled, want) + } + + disk := readSkillConfigFixture(t, path) + if !reflect.DeepEqual(disk.Skills.Disabled, want) { + t.Fatalf("persisted disabled = %#v, want %#v", disk.Skills.Disabled, want) + } + if disk.Skills.Enabled { + t.Fatal("persisted per-skill update changed skills.enabled") + } + if disk.Server.Host != "127.0.0.1" { + t.Fatalf("persisted server.host = %q, want operator value", disk.Server.Host) + } +} + +func TestSetSkillEnabledSerializesConcurrentUpdates(t *testing.T) { + home := isolateConfigHome(t) + path := filepath.Join(home, "config.yaml") + writeSkillConfigFixture(t, path, `skills: + frontmatter_migrated: true +`) + + start := make(chan struct{}) + errCh := make(chan error, 2) + var wg sync.WaitGroup + for _, name := range []string{"bravo", "alpha"} { + name := name + wg.Add(1) + go func() { + defer wg.Done() + <-start + _, err := SetSkillEnabled(name, false) + errCh <- err + }() + } + close(start) + wg.Wait() + close(errCh) + for err := range errCh { + if err != nil { + t.Fatalf("concurrent SetSkillEnabled: %v", err) + } + } + + disk := readSkillConfigFixture(t, path) + want := []string{"alpha", "bravo"} + if !reflect.DeepEqual(disk.Skills.Disabled, want) { + t.Fatalf("concurrent disabled updates = %#v, want %#v", disk.Skills.Disabled, want) + } +} + +func TestSkillStateMutationErrors(t *testing.T) { + t.Run("missing config", func(t *testing.T) { + isolateConfigHome(t) + if _, err := MigrateSkillState(nil); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("MigrateSkillState error = %v, want os.ErrNotExist", err) + } + if _, err := SetSkillEnabled("known", false); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("SetSkillEnabled error = %v, want os.ErrNotExist", err) + } + }) + + t.Run("malformed config", func(t *testing.T) { + home := isolateConfigHome(t) + path := filepath.Join(home, "config.yaml") + writeSkillConfigFixture(t, path, "skills: [not: valid\n") + if _, err := MigrateSkillState(nil); err == nil || !strings.Contains(err.Error(), "parse "+path) { + t.Fatalf("MigrateSkillState error = %v, want parse error for path", err) + } + if _, err := SetSkillEnabled("known", false); err == nil || !strings.Contains(err.Error(), "parse "+path) { + t.Fatalf("SetSkillEnabled error = %v, want parse error for path", err) + } + }) + + t.Run("unmigrated", func(t *testing.T) { + home := isolateConfigHome(t) + writeSkillConfigFixture(t, filepath.Join(home, "config.yaml"), `skills: + frontmatter_migrated: false +`) + if _, err := SetSkillEnabled("known", false); err == nil || err.Error() != "skill preferences have not been migrated" { + t.Fatalf("SetSkillEnabled error = %v, want unmigrated error", err) + } + }) + + t.Run("blank name", func(t *testing.T) { + isolateConfigHome(t) + if _, err := SetSkillEnabled(" \t\n ", false); err == nil || err.Error() != "skill name is required" { + t.Fatalf("SetSkillEnabled error = %v, want required-name error", err) + } + }) +} + +func TestMigrateSkillStateAtomicSaveFailureLeavesMarkerUnset(t *testing.T) { + home := isolateConfigHome(t) + path := filepath.Join(home, "config.yaml") + writeSkillConfigFixture(t, path, `skills: + disabled: [existing] + frontmatter_migrated: false +`) + if err := os.Chmod(home, 0o500); err != nil { + t.Fatalf("make config directory read-only: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(home, 0o700) }) + + if _, err := MigrateSkillState([]string{"legacy"}); err == nil { + t.Skip("filesystem bypasses directory write permissions") + } + + disk := readSkillConfigFixture(t, path) + if disk.Skills.FrontmatterMigrated { + t.Fatal("failed atomic save persisted the migration marker") + } + if !reflect.DeepEqual(disk.Skills.Disabled, []string{"existing"}) { + t.Fatalf("failed atomic save changed disabled names to %#v", disk.Skills.Disabled) + } +} + +func TestSkillsCloneAndSchema(t *testing.T) { + cfg := Default() + cfg.Skills.Disabled = []string{"one", "two"} + cfg.Skills.FrontmatterMigrated = true + clone := cfg.Clone() + clone.Skills.Disabled[0] = "changed" + clone.Skills.FrontmatterMigrated = false + if !reflect.DeepEqual(cfg.Skills.Disabled, []string{"one", "two"}) { + t.Fatalf("Clone shares skills.disabled backing storage: %#v", cfg.Skills.Disabled) + } + if !cfg.Skills.FrontmatterMigrated { + t.Fatal("changing clone migration marker changed original") + } + + paths := make(map[string]bool) + for _, field := range Schema() { + paths[field.Path] = true + } + if !paths["skills.disabled"] { + t.Fatal("skills.disabled is absent from editable schema") + } + if paths["skills.frontmatter_migrated"] { + t.Fatal("migration bookkeeping marker is exposed in editable schema") + } +} + +type skillConfigFixture struct { + Server struct { + Host string `yaml:"host"` + } `yaml:"server"` + Agent struct { + Workspace string `yaml:"workspace"` + } `yaml:"agent"` + Skills Skills `yaml:"skills"` +} + +func writeSkillConfigFixture(t *testing.T, path, body string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatalf("create config directory: %v", err) + } + if err := os.WriteFile(path, []byte(body), 0o600); err != nil { + t.Fatalf("write config fixture: %v", err) + } +} + +func readSkillConfigFixture(t *testing.T, path string) skillConfigFixture { + t.Helper() + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read config fixture: %v", err) + } + var cfg skillConfigFixture + if err := yaml.Unmarshal(raw, &cfg); err != nil { + t.Fatalf("parse config fixture: %v", err) + } + return cfg +} + +func containsExact(names []string, want string) bool { + for _, name := range names { + if name == want { + return true + } + } + return false +} 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..6403c23 100644 --- a/internal/server/handlers_config.go +++ b/internal/server/handlers_config.go @@ -121,9 +121,17 @@ func (s *Server) handleSaveRawConfig(w http.ResponseWriter, r *http.Request) { // applyReload rebuilds services that depend on configuration. func (s *Server) applyReload() error { if s.reloadFn == nil { - cfg := config.Get() + cfg, err := config.Reload() + if err != nil { + return err + } s.SetConfig(cfg) - s.agent.SetConfig(cfg) + if s.agent != nil { + s.agent.SetConfig(cfg) + } + if manager := s.currentSkills(); manager != nil { + manager.SetDisabled(cfg.Skills.Disabled) + } if s.gateway != nil { s.gateway.SetConfig(cfg) } @@ -132,10 +140,10 @@ func (s *Server) applyReload() error { if err := s.reloadFn(); err != nil { 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 + cfg := config.Get() + s.SetConfig(cfg) + if manager := s.currentSkills(); manager != nil { + manager.SetDisabled(cfg.Skills.Disabled) } 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..e70b4f7 100644 --- a/internal/server/handlers_subsystems.go +++ b/internal/server/handlers_subsystems.go @@ -3,6 +3,7 @@ package server import ( "context" "errors" + "fmt" "net/http" "strings" "time" @@ -22,7 +23,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,23 +38,20 @@ 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 { - writeError(w, http.StatusServiceUnavailable, errSkillsOff) - return - } + var body struct { Name string `json:"name"` Enabled bool `json:"enabled"` @@ -61,19 +60,37 @@ 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 { - writeError(w, http.StatusBadRequest, err) + + s.skillsConfigMu.Lock() + defer s.skillsConfigMu.Unlock() + + manager := s.currentSkills() + if manager == nil { + writeError(w, http.StatusServiceUnavailable, errSkillsOff) + return + } + if _, ok := manager.Get(body.Name); !ok { + writeError(w, http.StatusBadRequest, fmt.Errorf("skill %q not found", body.Name)) + return + } + if _, err := config.SetSkillEnabled(body.Name, body.Enabled); err != nil { + writeError(w, http.StatusInternalServerError, err) + return + } + if err := s.applyReload(); err != nil { + writeError(w, http.StatusInternalServerError, err) return } writeJSON(w, http.StatusOK, map[string]bool{"ok": true}) } 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 +99,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,21 +114,30 @@ 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) + status := http.StatusBadRequest + if errors.Is(err, skills.ErrReadOnly) { + status = http.StatusForbidden + } + writeError(w, status, err) return } writeJSON(w, http.StatusOK, sk) } 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 { - writeError(w, http.StatusBadRequest, err) + if err := mgr.Delete(r.PathValue("name")); err != nil { + 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}) @@ -501,19 +528,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/server.go b/internal/server/server.go index 226965f..c925b56 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -55,6 +55,9 @@ type Server struct { mu sync.RWMutex reloadFn func() error + // skillsConfigMu serializes skill preference writes through live publication, + // so concurrent toggles cannot overwrite one another or publish out of order. + skillsConfigMu sync.Mutex // dashSessions holds active dashboard login session tokens (cookie value → // expiry). Guarded by its own mutex; cleared when the password changes. @@ -107,6 +110,12 @@ func New(o Options) *Server { dashSessions: map[string]time.Time{}, } + // An embedded server may have no agent-owned manager. Seed its fallback + // manager from the supplied config so administrative reads are correct from + // the first request rather than only after a reload. + if s.skills != nil && (s.agent == nil || s.agent.Skills() == nil) && s.cfg != nil { + s.skills.SetDisabled(s.cfg.Skills.Disabled) + } // Restore dashboard logins so a daemon restart does not break EventSource // reattach (/api/chat/attach) for browsers that still hold a valid cookie. s.loadDashSessions() diff --git a/internal/server/skill_toggle_test.go b/internal/server/skill_toggle_test.go new file mode 100644 index 0000000..0552d35 --- /dev/null +++ b/internal/server/skill_toggle_test.go @@ -0,0 +1,456 @@ +package server + +import ( + "bytes" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "reflect" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/enowdev/antares/internal/agent" + "github.com/enowdev/antares/internal/config" + "github.com/enowdev/antares/internal/skills" +) + +func TestSkillToggleDoesNotMutateWritableSource(t *testing.T) { + s, manager, _, sources := newSkillToggleServer(t, []string{"writable"}, nil) + path := sources["writable"] + before := snapshotSkillSource(t, path) + + if rr := postSkillToggle(s, "writable", false); rr.Code != http.StatusOK { + t.Fatalf("disable status = %d, want 200 (body=%s)", rr.Code, rr.Body.String()) + } + if sk, ok := manager.Get("writable"); !ok || sk.Enabled { + t.Fatalf("live skill after disable = %#v, found=%v", sk, ok) + } + assertSkillSourceUnchanged(t, path, before) +} + +func TestSkillTogglePersistsWithoutMutatingReadOnlySourceAndRestarts(t *testing.T) { + s, manager, cfgPath, sources := newSkillToggleServer(t, []string{"toggle-me"}, nil) + path := sources["toggle-me"] + if err := os.Chmod(path, 0o444); err != nil { + t.Fatal(err) + } + if err := os.Chmod(filepath.Dir(path), 0o555); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + _ = os.Chmod(filepath.Dir(path), 0o755) + _ = os.Chmod(path, 0o644) + }) + before := snapshotSkillSource(t, path) + + if rr := postSkillToggle(s, "toggle-me", false); rr.Code != http.StatusOK { + t.Fatalf("disable status = %d, want 200 (body=%s)", rr.Code, rr.Body.String()) + } + if sk, ok := manager.Get("toggle-me"); !ok || sk.Enabled { + t.Fatalf("live skill after disable = %#v, found=%v", sk, ok) + } + persisted, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(persisted.Skills.Disabled, []string{"toggle-me"}) { + t.Fatalf("persisted disabled = %#v, want [toggle-me]", persisted.Skills.Disabled) + } + assertSkillSourceUnchanged(t, path, before) + + restartedManager := skills.NewManager(skills.Options{Dirs: persisted.Skills.Dirs}) + if err := restartedManager.Reload(); err != nil { + t.Fatal(err) + } + restarted := New(Options{Config: persisted, Skills: restartedManager}) + if sk, ok := restarted.currentSkills().Get("toggle-me"); !ok || sk.Enabled { + t.Fatalf("restarted skill = %#v, found=%v; want disabled", sk, ok) + } + if rr := postSkillToggle(restarted, "toggle-me", true); rr.Code != http.StatusOK { + t.Fatalf("re-enable status = %d, want 200 (body=%s)", rr.Code, rr.Body.String()) + } + if sk, ok := restarted.currentSkills().Get("toggle-me"); !ok || !sk.Enabled { + t.Fatalf("live skill after re-enable = %#v, found=%v", sk, ok) + } + persisted, err = config.Reload() + if err != nil { + t.Fatal(err) + } + if len(persisted.Skills.Disabled) != 0 { + t.Fatalf("persisted disabled after re-enable = %#v, want empty", persisted.Skills.Disabled) + } + if cfgPath != config.ConfigFile() { + t.Fatalf("fixture config path changed from %q to %q", cfgPath, config.ConfigFile()) + } + assertSkillSourceUnchanged(t, path, before) +} + +func TestSkillToggleConcurrentDifferentNamesPersistsUnion(t *testing.T) { + s, manager, _, _ := newSkillToggleServer(t, []string{"alpha", "bravo"}, nil) + + var wg sync.WaitGroup + results := make(chan *httptest.ResponseRecorder, 2) + for _, name := range []string{"alpha", "bravo"} { + name := name + wg.Add(1) + go func() { + defer wg.Done() + results <- postSkillToggle(s, name, false) + }() + } + wg.Wait() + close(results) + for rr := range results { + if rr.Code != http.StatusOK { + t.Fatalf("concurrent toggle status = %d, want 200 (body=%s)", rr.Code, rr.Body.String()) + } + } + persisted, err := config.Reload() + if err != nil { + t.Fatal(err) + } + want := []string{"alpha", "bravo"} + if !reflect.DeepEqual(persisted.Skills.Disabled, want) { + t.Fatalf("persisted disabled = %#v, want %#v", persisted.Skills.Disabled, want) + } + for _, name := range want { + if sk, ok := manager.Get(name); !ok || sk.Enabled { + t.Fatalf("live %s = %#v, found=%v; want disabled", name, sk, ok) + } + } +} + +func TestSkillToggleRejectsUnknownAndUnavailableManager(t *testing.T) { + s, _, _, _ := newSkillToggleServer(t, []string{"known"}, []string{"existing"}) + if rr := postSkillToggle(s, "missing", false); rr.Code != http.StatusBadRequest { + t.Fatalf("unknown skill status = %d, want 400 (body=%s)", rr.Code, rr.Body.String()) + } + persisted, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(persisted.Skills.Disabled, []string{"existing"}) { + t.Fatalf("unknown skill created preference: %#v", persisted.Skills.Disabled) + } + + nilServer := &Server{} + if rr := postSkillToggle(nilServer, "known", false); rr.Code != http.StatusServiceUnavailable { + t.Fatalf("nil manager status = %d, want 503 (body=%s)", rr.Code, rr.Body.String()) + } +} + +func TestSkillToggleSaveFailureDoesNotPublish(t *testing.T) { + _, manager, cfgPath, _ := newSkillToggleServer(t, []string{"known"}, nil) + before, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatal(err) + } + file, err := os.Open(cfgPath) + if err != nil { + t.Fatal(err) + } + defer file.Close() + procPath := "/proc/self/fd/" + strconv.Itoa(int(file.Fd())) + if _, err := os.Stat(procPath); err != nil { + t.Skipf("proc fd paths unavailable: %v", err) + } + t.Setenv("ANTARES_CONFIG", procPath) + cfg := config.Get() + s := New(Options{Config: cfg, Skills: manager}) + + rr := postSkillToggle(s, "known", false) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("save failure status = %d, want 500 (body=%s)", rr.Code, rr.Body.String()) + } + if sk, ok := manager.Get("known"); !ok || !sk.Enabled { + t.Fatalf("failed save published disabled state: %#v, found=%v", sk, ok) + } + after, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(after, before) { + t.Fatal("failed config save changed the persisted file") + } +} + +func TestSkillToggleReloadFailurePersistsWithoutPublishing(t *testing.T) { + _, manager, _, _ := newSkillToggleServer(t, []string{"known"}, nil) + cfg := config.Get() + s := New(Options{ + Config: cfg, + Skills: manager, + Reload: func() error { return errors.New("reload failed") }, + }) + + rr := postSkillToggle(s, "known", false) + if rr.Code != http.StatusInternalServerError { + t.Fatalf("reload failure status = %d, want 500 (body=%s)", rr.Code, rr.Body.String()) + } + persisted, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(persisted.Skills.Disabled, []string{"known"}) { + t.Fatalf("saved preference after reload failure = %#v, want [known]", persisted.Skills.Disabled) + } + if len(s.config().Skills.Disabled) != 0 { + t.Fatalf("reload failure published server config: %#v", s.config().Skills.Disabled) + } + if sk, ok := manager.Get("known"); !ok || !sk.Enabled { + t.Fatalf("reload failure published state despite callback error: %#v, found=%v", sk, ok) + } +} + +func TestApplyReloadWithoutCallbackReturnsErrorsAndToleratesNilAgent(t *testing.T) { + s, manager, cfgPath, _ := newSkillToggleServer(t, []string{"known"}, nil) + if err := os.WriteFile(cfgPath, []byte("skills: [unterminated\n"), 0o600); err != nil { + t.Fatal(err) + } + before := s.config() + if err := s.applyReload(); err == nil { + t.Fatal("applyReload succeeded with malformed persisted configuration") + } + if s.config() != before { + t.Fatal("failed applyReload replaced the server config") + } + if sk, ok := manager.Get("known"); !ok || !sk.Enabled { + t.Fatalf("failed applyReload changed manager state: %#v, found=%v", sk, ok) + } +} + +func TestNewInitializesFallbackSkillPreferences(t *testing.T) { + s, manager, _, _ := newSkillToggleServer(t, []string{"known"}, []string{"known"}) + if s.agent != nil { + t.Fatal("fixture unexpectedly has an agent") + } + if sk, ok := manager.Get("known"); !ok || sk.Enabled { + t.Fatalf("fallback skill = %#v, found=%v; want disabled at construction", sk, ok) + } +} + +func TestSkillHandlersObserveInPlaceReconfigure(t *testing.T) { + home := t.TempDir() + t.Setenv("ANTARES_HOME", home) + t.Setenv("ANTARES_PROFILE", "default") + t.Setenv("ANTARES_CONFIG", filepath.Join(home, "config.yaml")) + oldDir := filepath.Join(home, "old-skills") + newDir := filepath.Join(home, "new-skills") + packDir := filepath.Join(home, "pack-skills") + writeToggleServerSkill(t, oldDir, "replace-me") + writeToggleServerSkill(t, oldDir, "old-only") + writeToggleServerSkill(t, newDir, "replace-me") + writeToggleServerSkill(t, newDir, "replacement-only") + writeToggleServerSkill(t, packDir, "code-review") + + cfg := config.Default() + cfg.Skills.Enabled = true + cfg.Skills.FrontmatterMigrated = true + cfg.Skills.Dirs = []string{oldDir} + if err := config.SaveAt(config.ConfigFile(), cfg); err != nil { + t.Fatal(err) + } + liveManager := skills.NewManager(skills.Options{Dirs: []string{oldDir}, PackDirs: []string{packDir}}) + if err := liveManager.Reload(); err != nil { + t.Fatal(err) + } + a := &agent.Agent{} + a.SetConfig(cfg) + a.SetSkills(liveManager) + boundHandle, err := liveManager.ForProject(t.TempDir()) + if err != nil { + t.Fatal(err) + } + s := New(Options{ + Config: cfg, + Agent: a, + Skills: liveManager, + Reload: func() error { + reloaded, err := config.Reload() + if err != nil { + return err + } + if err := liveManager.Reconfigure(skills.Options{Dirs: []string{newDir}, PackDirs: []string{packDir}}); err != nil { + return err + } + a.SetConfig(reloaded) + return nil + }, + }) + + if rr := postSkillToggle(s, "replace-me", false); rr.Code != http.StatusOK { + t.Fatalf("toggle status = %d, want 200 (body=%s)", rr.Code, rr.Body.String()) + } + if s.currentSkills() != liveManager || a.Skills() != liveManager { + t.Fatal("in-place reload detached the shared skill manager") + } + if s.skills != liveManager { + t.Fatal("applyReload replaced the fallback manager") + } + if s.commandDeps().Skills != liveManager { + t.Fatal("command dependencies detached from the shared skill manager") + } + if sk, ok := liveManager.Get("replace-me"); !ok || sk.Enabled { + t.Fatalf("reconfigured manager preference = %#v, found=%v; want disabled", sk, ok) + } + if boundHandle == liveManager { + t.Fatal("project scope did not return a bound handle") + } + if sk, ok := boundHandle.Get("replace-me"); !ok || sk.Enabled { + t.Fatalf("bound handle preference = %#v, found=%v; want disabled", sk, ok) + } + + getReq := httptest.NewRequest(http.MethodGet, "/api/skills/replacement-only", nil) + getReq.SetPathValue("name", "replacement-only") + getRR := httptest.NewRecorder() + s.handleGetSkill(getRR, getReq) + if getRR.Code != http.StatusOK { + t.Fatalf("replacement get status = %d, want 200 (body=%s)", getRR.Code, getRR.Body.String()) + } + + listRR := httptest.NewRecorder() + s.handleListSkills(listRR, httptest.NewRequest(http.MethodGet, "/api/skills", nil)) + if !strings.Contains(listRR.Body.String(), "replacement-only") || strings.Contains(listRR.Body.String(), "old-only") { + t.Fatalf("list did not use replacement manager: %s", listRR.Body.String()) + } + + hubRR := httptest.NewRecorder() + s.handleHubSkills(hubRR, httptest.NewRequest(http.MethodGet, "/api/hub/skills", nil)) + var hubBody struct { + Skills []struct { + Name string `json:"name"` + Installed bool `json:"installed"` + } `json:"skills"` + } + if err := json.Unmarshal(hubRR.Body.Bytes(), &hubBody); err != nil { + t.Fatalf("decode hub response: %v (body=%s)", err, hubRR.Body.String()) + } + hubInstalled := false + for _, entry := range hubBody.Skills { + if entry.Name == "code-review" { + hubInstalled = entry.Installed + break + } + } + if !hubInstalled { + t.Fatalf("hub did not mark replacement-manager skill installed: %s", hubRR.Body.String()) + } + + saveRR := httptest.NewRecorder() + s.handleSaveSkill(saveRR, httptest.NewRequest(http.MethodPost, "/api/skills", strings.NewReader(`{"name":"saved-on-replacement","description":"new","body":"body"}`))) + if saveRR.Code != http.StatusOK { + t.Fatalf("replacement save status = %d, want 200 (body=%s)", saveRR.Code, saveRR.Body.String()) + } + if _, err := os.Stat(filepath.Join(newDir, "saved-on-replacement.md")); err != nil { + t.Fatalf("replacement manager did not receive save: %v", err) + } + if _, err := os.Stat(filepath.Join(oldDir, "saved-on-replacement.md")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("stale manager received save: %v", err) + } + + libraryRR := httptest.NewRecorder() + s.handleSkillLibrary(libraryRR, httptest.NewRequest(http.MethodGet, "/api/skills/library", nil)) + if !strings.Contains(libraryRR.Body.String(), "code-review") || strings.Contains(libraryRR.Body.String(), "old-only") { + t.Fatalf("library did not use replacement manager: %s", libraryRR.Body.String()) + } + + deleteReq := httptest.NewRequest(http.MethodDelete, "/api/skills/replacement-only", nil) + deleteReq.SetPathValue("name", "replacement-only") + deleteRR := httptest.NewRecorder() + s.handleDeleteSkill(deleteRR, deleteReq) + if deleteRR.Code != http.StatusOK { + t.Fatalf("replacement delete status = %d, want 200 (body=%s)", deleteRR.Code, deleteRR.Body.String()) + } + if _, err := os.Stat(filepath.Join(newDir, "replacement-only.md")); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("replacement manager did not delete its source: %v", err) + } +} + +func newSkillToggleServer(t *testing.T, names, disabled []string) (*Server, *skills.Manager, string, map[string]string) { + t.Helper() + home := t.TempDir() + t.Setenv("ANTARES_HOME", home) + t.Setenv("ANTARES_PROFILE", "default") + cfgPath := filepath.Join(home, "config.yaml") + t.Setenv("ANTARES_CONFIG", cfgPath) + dir := filepath.Join(home, "skills") + sources := make(map[string]string, len(names)) + for _, name := range names { + sources[name] = writeToggleServerSkill(t, dir, name) + } + cfg := config.Default() + cfg.Skills.Enabled = true + cfg.Skills.Dirs = []string{dir} + cfg.Skills.Disabled = append([]string(nil), disabled...) + cfg.Skills.FrontmatterMigrated = true + if err := config.SaveAt(cfgPath, cfg); err != nil { + t.Fatalf("seed config: %v", err) + } + manager := skills.NewManager(skills.Options{Dirs: []string{dir}}) + if err := manager.Reload(); err != nil { + t.Fatalf("load skills: %v", err) + } + return New(Options{Config: cfg, Skills: manager}), manager, cfgPath, sources +} + +func writeToggleServerSkill(t *testing.T, dir, name string) string { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + path := filepath.Join(dir, name+".md") + body := "---\nname: " + name + "\ndescription: " + name + " description\n---\n\n" + name + " body\n" + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + fixed := time.Unix(1_700_000_000, 0) + if err := os.Chtimes(path, fixed, fixed); err != nil { + t.Fatal(err) + } + return path +} + +func postSkillToggle(s *Server, name string, enabled bool) *httptest.ResponseRecorder { + body := `{"name":"` + name + `","enabled":` + strconv.FormatBool(enabled) + `}` + rr := httptest.NewRecorder() + s.handleToggleSkill(rr, httptest.NewRequest(http.MethodPost, "/api/skills/toggle", strings.NewReader(body))) + return rr +} + +type skillSourceSnapshot struct { + body []byte + mode os.FileMode + size int64 + modTime time.Time +} + +func snapshotSkillSource(t *testing.T, path string) skillSourceSnapshot { + t.Helper() + body, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + return skillSourceSnapshot{body: body, mode: info.Mode(), size: info.Size(), modTime: info.ModTime()} +} + +func assertSkillSourceUnchanged(t *testing.T, path string, before skillSourceSnapshot) { + t.Helper() + after := snapshotSkillSource(t, path) + if !bytes.Equal(after.body, before.body) { + t.Fatalf("skill source bytes changed during config toggle:\nbefore: %q\nafter: %q", before.body, after.body) + } + if after.mode != before.mode || after.size != before.size || !after.modTime.Equal(before.modTime) { + t.Fatalf("skill source stat changed: before={mode:%v size:%d mtime:%s} after={mode:%v size:%d mtime:%s}", + before.mode, before.size, before.modTime, after.mode, after.size, after.modTime) + } +} 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..bf742f8 --- /dev/null +++ b/internal/server/skills_test.go @@ -0,0 +1,303 @@ +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 TestSkillHTTPReadOnly(t *testing.T) { + home := t.TempDir() + state := t.TempDir() + t.Setenv("ANTARES_HOME", state) + t.Setenv("ANTARES_PROFILE", "default") + t.Setenv("ANTARES_CONFIG", filepath.Join(state, "config.yaml")) + configured := t.TempDir() + borrowedDir := filepath.Join(home, ".agent", "skills", "borrowed") + if err := os.MkdirAll(borrowedDir, 0o755); err != nil { + t.Fatal(err) + } + borrowedPath := filepath.Join(borrowedDir, "SKILL.md") + borrowedBytes := []byte("---\nname: borrowed\ndescription: Borrowed description\nenabled: true\n---\nBORROWED_BODY\n") + if err := os.WriteFile(borrowedPath, borrowedBytes, 0o644); err != nil { + t.Fatal(err) + } + borrowedBefore := snapshotSkillSource(t, borrowedPath) + mgr := skills.NewManager(skills.Options{Dirs: []string{configured}, UserHome: home}) + if err := mgr.Reload(); err != nil { + t.Fatal(err) + } + cfg := config.Default() + cfg.Skills.Enabled = true + cfg.Skills.Dirs = []string{configured} + cfg.Skills.FrontmatterMigrated = true + if err := config.SaveAt(config.ConfigFile(), cfg); err != nil { + t.Fatal(err) + } + s := New(Options{Config: cfg, 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 != "borrowed" || !listed.Skills[0].ReadOnly { + t.Fatalf("list did not expose borrowed 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("borrowed") + if get.Code != http.StatusOK { + t.Fatalf("get borrowed status = %d, want 200; body=%q", get.Code, get.Body.String()) + } + if fetched.Skill.Name != "borrowed" || !fetched.Skill.ReadOnly || fetched.Body != "BORROWED_BODY" { + t.Fatalf("get did not return borrowed 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":"borrowed","description":"changed","body":"CHANGED","tags":[]}`)) + s.handleSaveSkill(rr, req) + }, + }, + { + name: "delete", + run: func(rr *httptest.ResponseRecorder) { + req := httptest.NewRequest(http.MethodDelete, "/api/skills/borrowed", nil) + req.SetPathValue("name", "borrowed") + s.handleDeleteSkill(rr, req) + }, + }, + } + for _, mutation := range readonlyRequests { + rr := httptest.NewRecorder() + mutation.run(rr) + if rr.Code != http.StatusForbidden { + t.Fatalf("%s borrowed status = %d, want 403; body=%q", mutation.name, rr.Code, rr.Body.String()) + } + } + + toggle := httptest.NewRecorder() + s.handleToggleSkill(toggle, httptest.NewRequest(http.MethodPost, "/api/skills/toggle", strings.NewReader(`{"name":"borrowed","enabled":false}`))) + if toggle.Code != http.StatusOK { + t.Fatalf("toggle borrowed status = %d, want 200; body=%q", toggle.Code, toggle.Body.String()) + } + toggled, toggledBody := getSkill("borrowed") + if toggled.Code != http.StatusOK || toggledBody.Skill.Enabled || !toggledBody.Skill.ReadOnly { + t.Fatalf("toggle borrowed did not update effective state: status=%d skill=%+v", toggled.Code, toggledBody.Skill) + } + persisted, err := config.Reload() + if err != nil { + t.Fatal(err) + } + if len(persisted.Skills.Disabled) != 1 || persisted.Skills.Disabled[0] != "borrowed" { + t.Fatalf("persisted disabled = %#v, want [borrowed]", persisted.Skills.Disabled) + } + assertSkillSourceUnchanged(t, borrowedPath, borrowedBefore) + 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) + } + + writableToggle := httptest.NewRecorder() + s.handleToggleSkill(writableToggle, httptest.NewRequest(http.MethodPost, "/api/skills/toggle", strings.NewReader(`{"name":"writable","enabled":false}`))) + if writableToggle.Code != http.StatusOK { + t.Fatalf("toggle writable status = %d, want 200; body=%q", writableToggle.Code, writableToggle.Body.String()) + } + writableToggled, toggledBody := getSkill("writable") + if writableToggled.Code != http.StatusOK || toggledBody.Skill.Enabled { + t.Fatalf("toggle writable did not persist: status=%d skill=%+v", writableToggled.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()) + + 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 new file mode 100644 index 0000000..1bdd97d --- /dev/null +++ b/internal/skills/discovery.go @@ -0,0 +1,318 @@ +package skills + +import ( + "context" + "errors" + "io/fs" + "os" + "path/filepath" + "strings" +) + +type sourceKind uint8 + +const ( + sourcePack sourceKind = iota + sourceUser + sourceProject + sourceConfigured +) + +type sourceRoot struct { + path string + 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"}, + {".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"}, +} + +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(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) != "" { + for _, parts := range userSkillRoots { + userRoots = append(userRoots, filepath.Join(append([]string{opts.UserHome}, parts...)...)) + } + } + user, err = discoverRoots(scan, rootsForPaths(userRoots, sourceUser)) + if firstErr == nil { + firstErr = err + } + configured, err = discoverRoots(scan, 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(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(scan, rootsForPaths(paths, sourceProject)) +} + +func rootsForPaths(paths []string, kind sourceKind) []sourceRoot { + return appendSourceRoots(nil, paths, kind) +} + +func discoverRoots(scan *discoveryScan, roots []sourceRoot) (map[string]*Skill, error) { + found := make(map[string]*Skill) + var firstErr error + for _, root := range roots { + 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 + } + } + return found, firstErr +} + +// 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(scan *discoveryScan, root sourceRoot, publish func(*Skill)) error { + var firstErr error + ancestors := make(map[string]struct{}) + 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 nil + } + if info.IsDir() { + canonical, err := canonicalPath(logical) + if err != nil { + if !errors.Is(err, fs.ErrNotExist) && firstErr == nil { + firstErr = err + } + return nil + } + if _, cycle := ancestors[canonical]; cycle { + return nil + } + 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 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 + } + if err := walk(filepath.Join(logical, entry.Name())); err != nil { + delete(ancestors, canonical) + return err + } + } + delete(ancestors, canonical) + return nil + } + if !info.Mode().IsRegular() { + return nil + } + base := filepath.Base(logical) + if root.kind == sourceUser || root.kind == sourceProject { + if !strings.EqualFold(base, "SKILL.md") { + return nil + } + } else if !strings.EqualFold(filepath.Ext(base), ".md") { + return nil + } + + parsed, err := scan.parse(logical, info) + if err != 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 nil + } + skill := *parsed + 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) + return nil + } + if err := walk(root.path); err != nil { + return err + } + 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/discovery_test.go b/internal/skills/discovery_test.go new file mode 100644 index 0000000..7b4cea0 --- /dev/null +++ b/internal/skills/discovery_test.go @@ -0,0 +1,456 @@ +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 disabled := requireSkill(t, m, "disabled-native"); !disabled.Enabled { + t.Fatalf("legacy enabled header affected runtime state: %+v", disabled) + } + if got := m.LegacyDisabled(); len(got) != 1 || got[0] != "disabled-native" { + t.Fatalf("LegacyDisabled() = %v, want configured legacy input", got) + } + m.SetDisabled([]string{"disabled-native"}) + if strings.Contains(m.PromptBlock(0), "disabled-native") { + t.Fatal("config-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") + m.SetDisabled([]string{"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 || got.Enabled { + t.Fatalf("after removing %q winner = body %q usage %d enabled %v, want %q usage 1 disabled", fixtures[i].body, got.Body, got.UsageCount, got.Enabled, 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) + packOriginal, err := os.ReadFile(packMutablePath) + if err != nil { + t.Fatal(err) + } + m.SetDisabled([]string{"pack-mutable"}) + if requireSkill(t, m, "pack-mutable").Enabled { + t.Fatal("pack preference did not update the effective skill") + } + if raw, err := os.ReadFile(packMutablePath); err != nil || string(raw) != string(packOriginal) { + t.Fatalf("SetDisabled changed pack source: err=%v bytes=%q", err, raw) + } + 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 }}, + {"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) + } + } + m.SetDisabled([]string{"imported"}) + assertSkillDisabled(t, m, "imported") + 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") + } + override.SetDisabled([]string{"imported"}) + assertSkillDisabled(t, override, "imported") + + 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/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..69acb12 --- /dev/null +++ b/internal/skills/refresh_test.go @@ -0,0 +1,390 @@ +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") + } + manager.SetDisabled([]string{"live"}) + assertSkillDisabled(t, manager, "live") + + writeFile(t, livePath, skillDocument("live", "changed description", "EDITED_BODY_LONGER", false)) + eventually(t, func() bool { + updated, ok := manager.Get("live") + return ok && updated.Body == "EDITED_BODY_LONGER" && !updated.Enabled && + len(manager.Search("changed description", 5)) == 1 && !strings.Contains(manager.PromptBlock(0), "live") + }, "metadata/body edit to refresh while config-disabled state survives") + + atomicTemp := filepath.Join(filepath.Dir(livePath), "replacement.tmp") + writeFile(t, atomicTemp, skillDocument("live", "atomic description", "ATOMIC_BODY", true)) + if err := os.Rename(atomicTemp, livePath); err != nil { + t.Fatal(err) + } + eventually(t, func() bool { + atomic, ok := manager.Get("live") + return ok && atomic.Body == "ATOMIC_BODY" && !atomic.Enabled && !strings.Contains(manager.PromptBlock(0), "live") + }, "atomic replacement preserving disabled state") + + 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) + } + for label, scoped := range map[string]*Manager{"root": manager, "A": a, "B": b} { + if requireSkill(t, scoped, "live").Enabled { + t.Fatalf("%s reconfigure cleared shared disabled state", label) + } + } + 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("live") + return !ok && len(manager.Search("atomic description", 5)) == 0 && !strings.Contains(manager.PromptBlock(0), "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 new file mode 100644 index 0000000..5e4be29 --- /dev/null +++ b/internal/skills/scope.go @@ -0,0 +1,258 @@ +package skills + +import ( + "context" + "fmt" + "path/filepath" + "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 + disabled map[string]struct{} + cache map[string]cachedSkillFile + root *Manager + sharedOnly *Manager + + // 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 current default. A bound handle keeps its logical project path. +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) + 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{}, + disabled: map[string]struct{}{}, + cache: map[string]cachedSkillFile{}, + startupBase: defaultProject, + defaultProject: defaultProject, + 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 +} + +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 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 + } + 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.defaultProject != "" { + err = m.state.projectErrs[m.state.defaultProject] + } + m.state.mu.RUnlock() + return root, err + } + + logical, err := m.normalizeProject(projectDir) + if err != nil { + return m.state.sharedOnly, err + } + + // 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 + if registeredErr == nil { + registeredErr = m.state.projectErrs[logical] + } + m.state.mu.RUnlock() + if registered { + 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} + 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 { + 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() + startupBase := m.state.startupBase + m.state.mu.RUnlock() + if startupBase == "" { + return "", fmt.Errorf("normalize relative project directory %q: startup project directory is unavailable", projectDir) + } + return filepath.Clean(filepath.Join(startupBase, projectDir)), nil +} + +func (m *Manager) selectedProjectLocked() map[string]*Skill { + if m.sharedOnly { + return nil + } + projectDir := m.projectDir + if projectDir == "" { + projectDir = m.state.defaultProject + } + 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, m.cloneEffectiveSkillLocked(skill)) + } + return out +} + +func (m *Manager) cloneEffectiveSkillLocked(skill *Skill) Skill { + clone := cloneSkill(skill) + clone.UsageCount = m.state.usage[skill.Name] + _, disabled := m.state.disabled[skill.Name] + clone.Enabled = !disabled + return clone +} diff --git a/internal/skills/scope_test.go b/internal/skills/scope_test.go new file mode 100644 index 0000000..758112d --- /dev/null +++ b/internal/skills/scope_test.go @@ -0,0 +1,289 @@ +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) + } + manager.SetDisabled([]string{"collision"}) + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b, "relative-a": relative} { + if requireSkill(t, scoped, "collision").Enabled { + t.Fatalf("%s did not observe shared disabled state", label) + } + } + 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())-1 { + t.Fatalf("A enabled count = %d, list length = %d with one disabled", 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 || got.Enabled { + t.Fatalf("%s configured disabled winner = %+v", label, got) + } + } + if err := os.Remove(filepath.Join(configured, "collision.md")); err != nil { + t.Fatal(err) + } + mustReload(t, b) + for label, scoped := range map[string]*Manager{"startup": manager, "a": a, "b": b} { + got := requireSkill(t, scoped, "collision") + want := map[string]string{"startup": "STARTUP_COLLISION", "a": "A_COLLISION", "b": "B_COLLISION"}[label] + if got.Body != want || got.Enabled { + t.Fatalf("%s fallback after configured removal = %+v, want body %q still disabled", label, got, want) + } + } + + 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) + } + } + b.SetDisabled([]string{"collision", "shared-saved"}) + 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 SetDisabled", 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/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..50f91b0 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -5,12 +5,10 @@ package skills import ( "errors" "fmt" - "io/fs" "os" "path/filepath" "sort" "strings" - "sync" "time" "gopkg.in/yaml.v3" @@ -39,13 +37,17 @@ 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"` + legacyDisabled bool } // frontMatter is the YAML header of a skill file. type frontMatter struct { Name string `yaml:"name"` Description string `yaml:"description"` - Enabled *bool `yaml:"enabled"` + Enabled *bool `yaml:"enabled,omitempty"` Source string `yaml:"source"` Category string `yaml:"category"` Tags []string `yaml:"tags"` @@ -56,90 +58,21 @@ 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{}} -} - -// 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() -} - -// 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 -} - -// Reload rescans every configured directory. -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 - } - } +// ErrReadOnly is returned when a mutation targets an imported skill. +var ErrReadOnly = errors.New("automatically discovered skills are read-only") - 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 +80,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,19 +91,15 @@ 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 - } - if fm.Enabled != nil { - s.Enabled = *fm.Enabled + s.Source = fm.Source + if fm.Enabled != nil && !*fm.Enabled { + s.legacyDisabled = true } } } @@ -211,86 +131,99 @@ 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, *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 } - cp := *s - return &cp, true + clone := m.cloneEffectiveSkillLocked(skill) + return &clone, true } -// SetEnabled toggles a 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) +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 +} + +// SetDisabled replaces the profile-wide set of disabled logical skill names. +// Names are matched exactly after source precedence has selected a winner. +func (m *Manager) SetDisabled(names []string) { + if m == nil { + return } - raw, err := os.ReadFile(s.Path) - if err != nil { - return err + disabled := make(map[string]struct{}, len(names)) + for _, name := range names { + disabled[name] = struct{}{} } - text := strings.ReplaceAll(string(raw), "\r\n", "\n") + m.state.mu.Lock() + m.state.disabled = disabled + m.state.mu.Unlock() +} - value := "false" - if enabled { - value = "true" - } - switch { - case strings.HasPrefix(text, "---\n"): - end := strings.Index(text[4:], "\n---") - if end < 0 { - return errors.New("front matter is not terminated") - } - header := text[4 : 4+end] - 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:") { - lines[i] = "enabled: " + value - } - } - header = strings.Join(lines, "\n") - } else { - header += "\nenabled: " + value - } - text = "---\n" + header + rest - default: - text = "---\nname: " + s.Name + "\nenabled: " + value + "\n---\n\n" + text +// LegacyDisabled returns selected configured-source names carrying the retired +// enabled: false header. The header is migration input only and never affects +// the manager's effective state. +func (m *Manager) LegacyDisabled() []string { + if m == nil { + return nil } - - if err := os.WriteFile(s.Path, []byte(text), 0o644); err != nil { - return err + m.state.mu.RLock() + defer m.state.mu.RUnlock() + out := make([]string, 0) + for name, skill := range m.state.configured { + if skill.legacyDisabled { + out = append(out, name) + } } - return m.Reload() + sort.Strings(out) + return out } -// 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 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) } 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,38 +234,56 @@ 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 } - if err := m.Reload(); err != nil { + if err := m.reloadLocked(); err != nil { return nil, err } s, _ := m.Get(name) return s, nil } -// Delete removes a skill file. +func (m *Manager) writeDir() string { + m.state.mu.RLock() + defer m.state.mu.RUnlock() + for _, dir := range m.state.opts.Dirs { + if strings.TrimSpace(dir) != "" { + return dir + } + } + return "" +} + +// 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) } + if s.ReadOnly { + return fmt.Errorf("%w: %q", ErrReadOnly, name) + } 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 @@ -367,6 +318,8 @@ type Filter struct { Tech string // Category matches the skill category exactly. Category string + // EnabledOnly excludes disabled names before ranking and limiting results. + EnabledOnly bool } // Search finds skills by keyword, ranked by relevance. It matches across the @@ -392,7 +345,7 @@ func (m *Manager) SearchFiltered(query string, f Filter, limit int) []Skill { } var hits []scored for _, s := range list { - if !passesFilter(s, f) { + if (f.EnabledOnly && !s.Enabled) || !passesFilter(s, f) { continue } hay := strings.ToLower(strings.Join([]string{ @@ -491,14 +444,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, m.cloneEffectiveSkillLocked(chained)) } } return out @@ -525,18 +483,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 } @@ -547,18 +508,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, m.cloneEffectiveSkillLocked(skill)) } - 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) @@ -574,11 +538,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++ } } @@ -587,11 +554,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 _, disabled := m.state.disabled[skill.Name]; !disabled { n++ } } diff --git a/internal/skills/state_test.go b/internal/skills/state_test.go new file mode 100644 index 0000000..8e58647 --- /dev/null +++ b/internal/skills/state_test.go @@ -0,0 +1,251 @@ +package skills + +import ( + "bytes" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" +) + +func writeStateSkill(t *testing.T, dir, file, name, header, body string) string { + t.Helper() + path := filepath.Join(dir, file) + content := "---\nname: " + name + "\n" + header + "---\n\n" + body + "\n" + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + return path +} + +func TestDisabledStateIsEffectiveWithoutMutatingSources(t *testing.T) { + base := t.TempDir() + dir := filepath.Join(base, "configured") + packDir := filepath.Join(base, "pack") + home := filepath.Join(base, "home") + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(packDir, 0o755); err != nil { + t.Fatal(err) + } + legacyPath := writeStateSkill(t, dir, "legacy.md", "legacy", "description: legacy description\nenabled: false\n", "legacy body") + writeStateSkill(t, dir, "active.md", "active", "description: active description\n", "active body") + packPath := writeStateSkill(t, packDir, "pack.md", "pack", "description: pack description\ncategory: web\nenabled: false\n", "pack body") + importedDir := filepath.Join(home, ".agent", "skills", "imported") + if err := os.MkdirAll(importedDir, 0o755); err != nil { + t.Fatal(err) + } + importedPath := writeStateSkill(t, importedDir, "SKILL.md", "imported", "description: imported description\nenabled: false\n", "imported body") + + fixedTime := time.Unix(1_700_000_000, 0) + for _, path := range []string{legacyPath, packPath, importedPath} { + if err := os.Chtimes(path, fixedTime, fixedTime); err != nil { + t.Fatal(err) + } + } + type sourceSnapshot struct { + bytes []byte + info os.FileInfo + } + before := make(map[string]sourceSnapshot) + for _, path := range []string{legacyPath, packPath, importedPath} { + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + before[path] = sourceSnapshot{bytes: raw, info: info} + } + + m := NewManager(Options{Dirs: []string{dir}, PackDirs: []string{packDir}, UserHome: home}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + if got := m.LegacyDisabled(); !reflect.DeepEqual(got, []string{"legacy"}) { + t.Fatalf("LegacyDisabled() = %v, want configured-only [legacy]", got) + } + for _, name := range []string{"legacy", "pack", "imported"} { + if skill, ok := m.Get(name); !ok || !skill.Enabled { + t.Fatalf("%s enabled state before config overlay = (%+v, %v), want enabled", name, skill, ok) + } + } + + m.SetDisabled([]string{"legacy", "pack", "imported"}) + + listed := m.List() + if got := enabledByName(listed); !reflect.DeepEqual(got, map[string]bool{"active": true, "imported": false, "legacy": false, "pack": false}) { + t.Fatalf("List enabled state = %v", got) + } + for _, name := range []string{"legacy", "imported"} { + assertSkillDisabled(t, m, name) + } + library, total := m.Library("web", 0, 10) + if total != 1 || len(library) != 1 || library[0].Name != "pack" || library[0].Enabled { + t.Fatalf("Library(web) = (%+v, %d), want disabled pack", library, total) + } + if got := m.Count(); got != 1 { + t.Fatalf("Count() = %d, want only active enabled", got) + } + prompt := m.PromptBlock(0) + if !strings.Contains(prompt, "active: active description") || strings.Contains(prompt, "legacy") || strings.Contains(prompt, "imported") || strings.Contains(prompt, "pack") { + t.Fatalf("PromptBlock() did not reflect effective state: %q", prompt) + } + + for path, snapshot := range before { + afterBytes, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + afterInfo, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(afterBytes, snapshot.bytes) { + t.Fatalf("SetDisabled changed source bytes for %q:\n%s", path, afterBytes) + } + if afterInfo.Mode() != snapshot.info.Mode() || afterInfo.Size() != snapshot.info.Size() || !afterInfo.ModTime().Equal(snapshot.info.ModTime()) { + t.Fatalf("SetDisabled changed source stat for %q: before=%+v after=%+v", path, snapshot.info, afterInfo) + } + } +} + +func TestLegacyDisabledUsesConfiguredWinnerAndSortsNames(t *testing.T) { + base := t.TempDir() + first := filepath.Join(base, "first") + second := filepath.Join(base, "second") + pack := filepath.Join(base, "pack") + for _, dir := range []string{first, second, pack} { + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatal(err) + } + } + writeStateSkill(t, first, "duplicate.md", "duplicate", "enabled: false\n", "losing legacy body") + writeStateSkill(t, second, "duplicate.md", "duplicate", "enabled: true\n", "winning body") + writeStateSkill(t, second, "z.md", "z-name", "enabled: false\n", "z body") + writeStateSkill(t, second, "a.md", "a-name", "enabled: false\n", "a body") + writeStateSkill(t, pack, "pack.md", "pack-legacy", "enabled: false\n", "pack body") + + m := NewManager(Options{Dirs: []string{first, second}, PackDirs: []string{pack}}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + if got := m.LegacyDisabled(); !reflect.DeepEqual(got, []string{"a-name", "z-name"}) { + t.Fatalf("LegacyDisabled() = %v, want sorted configured winners only", got) + } + winner, ok := m.Get("duplicate") + if !ok || winner.Body != "winning body" || !winner.Enabled { + t.Fatalf("selected duplicate = (%+v, %v), want enabled later-directory winner", winner, ok) + } + if packSkill := requireSkill(t, m, "pack-legacy"); !packSkill.Enabled { + t.Fatalf("legacy pack header affected runtime state: %+v", packSkill) + } +} + +func TestDisabledPreferenceSurvivesReloadSaveDeleteAndRecreate(t *testing.T) { + dir := t.TempDir() + path := writeStateSkill(t, dir, "persistent.md", "persistent", "description: old\nenabled: false\n", "old body") + m := NewManager(Options{Dirs: []string{dir}}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + m.SetDisabled([]string{"persistent"}) + + if err := m.Reload(); err != nil { + t.Fatal(err) + } + assertSkillDisabled(t, m, "persistent") + + saved, err := m.Save("persistent", "saved", "saved body", []string{"state"}) + if err != nil { + t.Fatal(err) + } + if saved.Enabled { + t.Fatalf("Save returned enabled skill despite retained preference: %+v", saved) + } + raw, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(raw), "enabled:") { + t.Fatalf("Save persisted retired enabled header:\n%s", raw) + } + + if err := m.Delete("persistent"); err != nil { + t.Fatal(err) + } + if _, ok := m.Get("persistent"); ok { + t.Fatal("deleted skill remains present") + } + writeStateSkill(t, dir, "persistent.md", "persistent", "description: recreated\n", "recreated body") + if err := m.Reload(); err != nil { + t.Fatal(err) + } + assertSkillDisabled(t, m, "persistent") +} + +func TestSetDisabledClonesInputAndReplacesState(t *testing.T) { + dir := t.TempDir() + writeStateSkill(t, dir, "one.md", "one", "", "one body") + writeStateSkill(t, dir, "two.md", "two", "", "two body") + m := NewManager(Options{Dirs: []string{dir}}) + if err := m.Reload(); err != nil { + t.Fatal(err) + } + + names := []string{"one"} + m.SetDisabled(names) + names[0] = "two" + assertSkillDisabled(t, m, "one") + if two, _ := m.Get("two"); !two.Enabled { + t.Fatal("mutating SetDisabled input changed manager state") + } + + m.SetDisabled(nil) + if m.Count() != 2 { + t.Fatalf("SetDisabled(nil) did not clear state: count=%d", m.Count()) + } +} + +func TestReloadReportsErrorAndPublishesReadableSkills(t *testing.T) { + configured := t.TempDir() + writeStateSkill(t, configured, "good.md", "good", "description: readable\n", "good body") + writeStateSkill(t, configured, "malformed.md", "malformed", "tags: [unterminated\n", "bad body") + + m := NewManager(Options{Dirs: []string{configured}}) + err := m.Reload() + if err == nil { + t.Fatal("Reload() error = nil, want configured parse error") + } + if !strings.Contains(err.Error(), "invalid front matter") { + t.Fatalf("Reload() error = %q, want invalid front matter", err) + } + good, ok := m.Get("good") + if !ok || good.Body != "good body" { + t.Fatalf("readable skill was not published with partial scan: (%+v, %v)", good, ok) + } + if _, ok := m.Get("malformed"); ok { + t.Fatal("malformed skill was published") + } +} + +func enabledByName(skills []Skill) map[string]bool { + out := make(map[string]bool, len(skills)) + for _, skill := range skills { + out[skill.Name] = skill.Enabled + } + return out +} + +func assertSkillDisabled(t *testing.T, m *Manager, name string) { + t.Helper() + skill, ok := m.Get(name) + if !ok || skill.Enabled { + t.Fatalf("Get(%q) = (%+v, %v), want disabled", name, skill, ok) + } +} diff --git a/internal/tools/skill.go b/internal/tools/skill.go index bfd5963..fc89a6c 100644 --- a/internal/tools/skill.go +++ b/internal/tools/skill.go @@ -138,7 +138,7 @@ func (skillTool) Execute(_ context.Context, in Input) Result { if err := lib.Write(name, args.Description, body, args.Tags); err != nil { return Errorf("save failed: %v", err) } - return Text(fmt.Sprintf("Saved skill %q. It will appear in your catalogue on the next turn.", name)) + return Text(fmt.Sprintf("Saved skill %q.", name)) default: return Errorf("unknown action %q (want list, search, read, chains, or save)", args.Action) diff --git a/web/src/lib/i18n.tsx b/web/src/lib/i18n.tsx index 19893a8..818d02f 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 content', + 'skills.discoveredReadOnly': + 'Discovered from another skill directory. Enable or disable it in Antares; edit the source file to change its content.', '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..b4de536 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,10 +57,9 @@ 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, setData } = usePoll<{ skills: Skill[]; library?: number }>(endpoint, 5000) const [busy, setBusy] = useState('') + const [toggleError, setToggleError] = useState('') const [browsing, setBrowsing] = useState(false) const [editing, setEditing] = useState(null) const [creating, setCreating] = useState(false) @@ -86,8 +86,13 @@ export default function SkillsPage() { const toggle = async (name: string, enabled: boolean) => { setBusy(name) + setToggleError('') try { await post('/skills/toggle', { name, enabled }) + if (data) setData({ ...data, skills: data.skills.map((s) => s.name === name ? { ...s, enabled } : s) }) + reload() + } catch (e) { + setToggleError(`${name}: ${(e as Error).message}`) reload() } finally { setBusy('') @@ -124,6 +129,7 @@ export default function SkillsPage() { /> ) : null} + {toggleError ?

{toggleError}

: null} ) @@ -179,11 +185,14 @@ export default function SkillsPage() { + {!s.read_only ? ( + + ) : null} ))} @@ -231,27 +242,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 +300,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')} - /> -
-
-
- -