From 3e82cab0f7349fdcb6982001210d499a8a83c638 Mon Sep 17 00:00:00 2001 From: Leonardo Faoro Date: Wed, 15 Jul 2026 15:27:38 +0300 Subject: [PATCH 1/4] feat(skills): discover shared ~/.agents/skills with multi-root skill loading Add ~/.agents/skills as a read-only global discovery root after the primary Zero skills dir and before plugin roots. Unify runtime and CLI discovery on one multi-root path (DiscoveryRoots / LoadFromRoots / GlobalRoots) and always register the multi-root skill tool so bare runs and plugin runs share the same load surface. Install/remove/lock still target only the primary Zero skills directory. Fixes #610 --- CHANGELOG.md | 3 + docs/EXTENDING.md | 18 ++- docs/HOW_ZERO_WORKS.md | 7 +- internal/cli/distribution.go | 3 +- internal/cli/plugin_activate.go | 30 ++-- internal/cli/plugin_activate_test.go | 41 ++++++ internal/cli/skills.go | 43 +++--- internal/cli/skills_test.go | 92 +++++++++++- internal/plugins/activate.go | 83 +++++------ internal/plugins/activate_test.go | 89 +++++++++++ internal/skills/install.go | 46 ++++++ internal/skills/skills.go | 135 ++++++++++++++++- internal/skills/skills_test.go | 213 +++++++++++++++++++++++++++ 13 files changed, 703 insertions(+), 100 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3755047e9..83d61946b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -127,6 +127,9 @@ tagged. Until then, source builds report the version `dev`. ## [Unreleased] ### Added +- Shared multi-agent skills discovery: when present, `~/.agents/skills` is searched after the primary + Zero skills dir (and before plugin skill roots). `zero skills list` / `info` and the runtime `skill` + tool share one multi-root discovery path; install/remove still target only the Zero skills directory. - `SECURITY.md` with a private vulnerability-reporting path, `CODE_OF_CONDUCT.md`, this changelog, and GitHub issue/PR templates. - Interactive `/theme` picker: bare `/theme` opens a popup that live-previews each palette as you move diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index 5c76d1cc6..6cb1ece36 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -102,14 +102,24 @@ The full format spec (frontmatter fields, tool scopes, prompt conventions) is in Skills are markdown instruction packs the agent can pull in on demand. Each skill is a directory containing a `SKILL.md`. Standalone project skill directories are not supported in this version (shared project-wide skills must go in `AGENTS.md` or as a hook). However, project plugins (section 6) may bundle skills, which are merged into the active run. -Discovery root: `$ZERO_SKILLS_DIR` → `$XDG_DATA_HOME/zero/skills` → `~/.local/share/zero/skills/`. A missing directory is fine — Zero just reports "no skills". +Discovery roots (earlier wins on name collisions): + +1. **Primary Zero dir** — `$ZERO_SKILLS_DIR` if set, else `$XDG_DATA_HOME/zero/skills`, else `~/.local/share/zero/skills/` +2. **Shared multi-agent dir** — `~/.agents/skills/` when present (read-only discovery; never an install target) +3. **Plugin skill roots** — skills bundled by active plugins (section 6) + +A missing directory is fine — Zero just omits it. Management commands (`zero skills add` / `remove`) always write to the primary Zero dir only; `list` / `info` search primary + `~/.agents/skills`. ```text -~/.local/share/zero/skills/ +~/.local/share/zero/skills/ # primary (install/remove target) run-benchmarks/ SKILL.md write-changelog/ SKILL.md + +~/.agents/skills/ # optional shared multi-agent root + shared-review/ + SKILL.md ``` `SKILL.md` format: @@ -126,9 +136,9 @@ description: Run the project's benchmark suite and summarize the deltas. 3. Report any regression > 5% with the function name and the previous value. ``` -Only `name` and `description` are recognized in the frontmatter today. The `name` defaults to the directory name. Within a single skills root, duplicate names are resolved by lexicographic directory order. During an agent run, Zero loads the default skills directory before plugin skill roots; earlier roots win name collisions silently. Plugin-declared skills (section 6) are merged into the active agent run at plugin activation time, so bundled skills appear in the available skills list and can be loaded with the `skill` tool. +Only `name` and `description` are recognized in the frontmatter today. The `name` defaults to the directory name. Within a single skills root, duplicate names are resolved by lexicographic directory order. Across roots, Zero loads the primary directory first, then `~/.agents/skills`, then plugin skill roots; earlier roots win name collisions. Plugin-declared skills (section 6) are merged into the active agent run at plugin activation time, so bundled skills appear in the available skills list and can be loaded with the `skill` tool. -The `skill` core tool lets the agent load any discovered skill by name. +The `skill` tool lets the agent load any discovered skill by name (primary, agents, or plugin). ## 4. Hooks diff --git a/docs/HOW_ZERO_WORKS.md b/docs/HOW_ZERO_WORKS.md index 4c84667ef..7481114e1 100644 --- a/docs/HOW_ZERO_WORKS.md +++ b/docs/HOW_ZERO_WORKS.md @@ -733,9 +733,10 @@ flowchart TD - **Skills** are instruction packs the model can load on demand. - **Specialists** are sub-agents callable through the `Task` tool. - **MCP servers** contribute external tools. -- **Plugins** can add tools, hooks, and skill roots. Bootstrap merges plugin skill - roots into the runtime skill tool and prompt-visible skill list; the base - `internal/skills` scanner still scans one root at a time. +- **Plugins** can add tools, hooks, and skill roots. Bootstrap always registers a + multi-root skill tool: primary Zero skills dir, optional `~/.agents/skills`, + then plugin skill roots (earlier wins). `internal/skills` owns that merge via + `LoadFromRoots` / `DiscoveryRoots`; single-root `Load` remains for install/write. - **Hooks** can observe or block tool lifecycle events. ## End-to-End Data Flow diff --git a/internal/cli/distribution.go b/internal/cli/distribution.go index 8cf2d640d..5cd1c310c 100644 --- a/internal/cli/distribution.go +++ b/internal/cli/distribution.go @@ -118,7 +118,8 @@ func runSkillInfo(args []string, dir string, stdout io.Writer, stderr io.Writer) if name == "" { return writeExecUsageError(stderr, "usage: zero skill info [--json]") } - info, ok := skills.Info(dir, name) + // Resolve across primary + ~/.agents/skills; lock metadata only from primary. + info, ok := skills.InfoFromRoots(dir, skills.GlobalRoots(dir), name) if !ok { return writeAppError(stderr, fmt.Sprintf("skill %q not found", name), exitUsage) } diff --git a/internal/cli/plugin_activate.go b/internal/cli/plugin_activate.go index 2215af4e6..64ff72758 100644 --- a/internal/cli/plugin_activate.go +++ b/internal/cli/plugin_activate.go @@ -15,11 +15,11 @@ import ( // pluginActivation holds what plugin activation contributed to a bootstrap so the // later dispatcher + skill wiring can consume it: the plugin hook definitions and -// the plugin skill search roots. The zero value (no plugins) is inert — the -// dispatcher gets no extra hooks and the skill tool keeps only the default dir. -// The embedded trustSkip reports whether the project plugin layer was dropped for -// an untrusted workspace, so the caller can fold it into the single combined -// trust notice. +// the plugin skill search roots. The zero value (no plugins) still overlays the +// multi-root skill tool (primary + ~/.agents/skills) so discovery is identical +// with or without plugins. The embedded trustSkip reports whether the project +// plugin layer was dropped for an untrusted workspace, so the caller can fold it +// into the single combined trust notice. type pluginActivation struct { hooks []hooks.Definition skillRoots []string @@ -52,6 +52,9 @@ func activatePlugins(workspaceRoot string, registry *tools.Registry, deps appDep loaded, err := deps.loadPlugins(plugins.LoadOptions{Cwd: workspaceRoot, ExcludeProject: excludeProject}) if err != nil { writePluginActivationWarning(stderr, "failed to load plugins: "+err.Error()) + // Still overlay multi-root discovery (primary + agents) so a plugin load + // failure cannot hide shared skills. + registry.Register(plugins.NewSkillTool(deps.skillsDir(), nil)) return pluginActivation{trustSkip: skip} } @@ -67,14 +70,11 @@ func activatePlugins(workspaceRoot string, registry *tools.Registry, deps appDep writePluginActivationWarning(stderr, warning) } - // Re-register the skill tool to also resolve plugin-declared skills. The core - // skill tool only reads the default skills dir; the plugin-aware replacement - // merges the default dir with the plugin skill roots (default dir wins a name - // clash), so plugin skills appear in the agent's skill list. With no plugin - // skill roots this is byte-equivalent to the default skills surface. - if len(result.SkillRoots) > 0 { - registry.Register(plugins.NewSkillTool(deps.skillsDir(), result.SkillRoots)) - } + // Always overlay the multi-root skill tool so discovery is identical with or + // without plugins: primary DefaultDir + optional ~/.agents/skills + plugin + // roots (earlier wins). With empty SkillRoots and no agents dir this is + // byte-equivalent to the core single-dir skill surface. + registry.Register(plugins.NewSkillTool(deps.skillsDir(), result.SkillRoots)) return pluginActivation{hooks: result.Hooks, skillRoots: result.SkillRoots, trustSkip: skip} } @@ -91,8 +91,8 @@ func projectPluginsDirExists(workspaceRoot string) bool { } // skillInfos resolves the reusable skills the model can load via the skill tool — -// the default skills dir merged with any plugin skill roots, the same set the -// plugin-aware skill tool resolves against — as plain data for the agent's system +// primary dir + optional ~/.agents/skills + plugin skill roots, the same set the +// multi-root skill tool resolves against — as plain data for the agent's system // prompt. It returns nil when no skills are installed, so a skill-less run leaves // the prompt byte-identical. func (a pluginActivation) skillInfos(defaultDir string) []agent.SkillInfo { diff --git a/internal/cli/plugin_activate_test.go b/internal/cli/plugin_activate_test.go index ff2e30abb..32c73799e 100644 --- a/internal/cli/plugin_activate_test.go +++ b/internal/cli/plugin_activate_test.go @@ -69,6 +69,11 @@ func TestActivatePluginsRegistersToolAndCollectsHooks(t *testing.T) { } func TestActivatePluginsRegistersPluginSkillTool(t *testing.T) { + // Isolate host ~/.agents/skills so it cannot interfere with this test. + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + pluginDir := t.TempDir() skillDir := filepath.Join(pluginDir, "skills", "demo-skill") if err := os.MkdirAll(skillDir, 0o755); err != nil { @@ -108,6 +113,42 @@ func TestActivatePluginsRegistersPluginSkillTool(t *testing.T) { } } +func TestActivatePluginsAlwaysRegistersMultiRootSkillTool(t *testing.T) { + // Even with zero plugins / zero plugin skill roots, activation must overlay + // the multi-root skill tool so ~/.agents/skills is loadable on bare runs. + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + skillDir := filepath.Join(agents, "agents-skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("---\nname: agents-skill\n---\nagents body"), 0o644); err != nil { + t.Fatal(err) + } + + registry := tools.NewRegistry() + registry.Register(tools.NewSkillTool(t.TempDir())) // core single-dir tool + var stderr bytes.Buffer + workspace := t.TempDir() + activation := activatePlugins(workspace, registry, fakePluginDeps(t, nil), &stderr, workspace) + if len(activation.skillRoots) != 0 { + t.Fatalf("expected no plugin skill roots, got %#v", activation.skillRoots) + } + skillTool, ok := registry.Get("skill") + if !ok { + t.Fatal("skill tool missing after activation") + } + res := skillTool.Run(context.Background(), map[string]any{"name": "agents-skill"}) + if res.Status != tools.StatusOK || !bytes.Contains([]byte(res.Output), []byte("agents body")) { + t.Fatalf("agents skill must load with zero plugin roots: %q (%s)", res.Status, res.Output) + } +} + func TestActivatePluginsSurfacesLoadDiagnostics(t *testing.T) { // Load succeeds (no top-level error) but reports a per-plugin diagnostic for a // plugin it had to skip. activatePlugins must forward that diagnostic to stderr diff --git a/internal/cli/skills.go b/internal/cli/skills.go index 5bff33951..04650084c 100644 --- a/internal/cli/skills.go +++ b/internal/cli/skills.go @@ -59,25 +59,21 @@ func runSkills(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) } func runSkillsList(dir string, options skillListOptions, stdout io.Writer, stderr io.Writer) int { - discovered, err := skills.List(dir) + // Management CLI lists global roots only (primary + ~/.agents/skills); plugin + // skills stay out of `zero skills list` and surface via the agent skill tool. + roots := skills.GlobalRoots(dir) + discovered, dups, err := skills.ListFromRoots(roots) if err != nil { return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) } - // Surface name collisions that List silently resolved (first directory wins), - // so a shadowed same-named skill is reported instead of just disappearing. - // Warnings go to stderr, keeping stdout (including --json) clean. - if dups, derr := skills.Duplicates(dir); derr == nil { - for _, dup := range dups { - fmt.Fprintf(stderr, "warning: duplicate skill %q: using %s, ignoring %s\n", - redaction.RedactString(dup.Name, redaction.Options{}), - redaction.RedactString(dup.Winner, redaction.Options{}), - redaction.RedactString(dup.Loser, redaction.Options{})) - } - } else { - // Don't silently swallow a scan failure: "no warnings" would then be - // ambiguous (no duplicates vs. detection broke). Surface it on stderr. - fmt.Fprintf(stderr, "warning: could not check for duplicate skills: %s\n", - redaction.ErrorMessage(derr, redaction.Options{})) + // Surface name collisions that ListFromRoots silently resolved (earlier root + // wins), so a shadowed same-named skill is reported instead of just + // disappearing. Warnings go to stderr, keeping stdout (including --json) clean. + for _, dup := range dups { + fmt.Fprintf(stderr, "warning: duplicate skill %q: using %s, ignoring %s\n", + redaction.RedactString(dup.Name, redaction.Options{}), + redaction.RedactString(dup.Winner, redaction.Options{}), + redaction.RedactString(dup.Loser, redaction.Options{})) } if options.json { payload := struct { @@ -88,16 +84,16 @@ func runSkillsList(dir string, options skillListOptions, stdout io.Writer, stder } return exitSuccess } - output := redaction.RedactString(formatSkillList(discovered, dir), redaction.Options{}) + output := redaction.RedactString(formatSkillList(discovered), redaction.Options{}) if _, err := fmt.Fprintln(stdout, output); err != nil { return exitCrash } return exitSuccess } -func formatSkillList(discovered []skills.Skill, dir string) string { +func formatSkillList(discovered []skills.Skill) string { if len(discovered) == 0 { - return fmt.Sprintf("No Zero skills found in %s.", dir) + return "No skills found." } lines := []string{"Zero Skills:"} for _, skill := range discovered { @@ -131,10 +127,13 @@ func writeSkillsHelp(w io.Writer) error { zero skills Commands: - list List discovered Zero skills - add Install a skill (checksum-pinned in skills.lock) + list List discovered skills (Zero dir + ~/.agents/skills) + add Install a skill into the Zero skills dir (checksum-pinned in skills.lock) info Show a skill's frontmatter, source, and pinned hash - remove Remove an installed skill and its lockfile entry + remove Remove an installed skill and its lockfile entry from the Zero skills dir + +list and info also search ~/.agents/skills when present (read-only). +add and remove always target the Zero-specific skills directory. `) return err } diff --git a/internal/cli/skills_test.go b/internal/cli/skills_test.go index fbc207795..63dc608e0 100644 --- a/internal/cli/skills_test.go +++ b/internal/cli/skills_test.go @@ -23,7 +23,15 @@ func writeSkillFixture(t *testing.T, dir string, name string, content string) { } } +func isolateCLIAgentsHome(t *testing.T) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) +} + func TestRunSkillsListText(t *testing.T) { + isolateCLIAgentsHome(t) dir := t.TempDir() writeSkillFixture(t, dir, "confirmation-policy", "---\nname: confirmation-policy\ndescription: Ask before risky actions.\n---\nbody") @@ -43,6 +51,7 @@ func TestRunSkillsListText(t *testing.T) { } func TestRunSkillsListWarnsOnDuplicateNames(t *testing.T) { + isolateCLIAgentsHome(t) dir := t.TempDir() // Two directories declare the same frontmatter name; List keeps one and the // other is shadowed. The command must warn instead of silently dropping it. @@ -62,6 +71,7 @@ func TestRunSkillsListWarnsOnDuplicateNames(t *testing.T) { } func TestRunSkillsDefaultsToList(t *testing.T) { + isolateCLIAgentsHome(t) dir := t.TempDir() writeSkillFixture(t, dir, "demo", "body") @@ -78,6 +88,7 @@ func TestRunSkillsDefaultsToList(t *testing.T) { } func TestRunSkillsListJSON(t *testing.T) { + isolateCLIAgentsHome(t) dir := t.TempDir() writeSkillFixture(t, dir, "demo", "---\nname: demo\ndescription: a demo\n---\nbody") @@ -111,6 +122,7 @@ func TestRunSkillsListJSON(t *testing.T) { } func TestRunSkillsEmptyDir(t *testing.T) { + isolateCLIAgentsHome(t) var stdout, stderr bytes.Buffer exit := runWithDeps([]string{"skills", "list"}, &stdout, &stderr, appDeps{ skillsDir: func() string { return filepath.Join(t.TempDir(), "missing") }, @@ -118,11 +130,89 @@ func TestRunSkillsEmptyDir(t *testing.T) { if exit != 0 { t.Fatalf("exit = %d, stderr = %s", exit, stderr.String()) } - if !strings.Contains(strings.ToLower(stdout.String()), "no") { + if !strings.Contains(strings.ToLower(stdout.String()), "no skills found") { t.Fatalf("expected a no-skills message, got:\n%s", stdout.String()) } } +func TestRunSkillsListIncludesAgentsOnlySkill(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + writeSkillFixture(t, agents, "agents-only", "---\nname: agents-only\ndescription: Shared multi-agent skill.\n---\nbody") + + primary := t.TempDir() + var stdout, stderr bytes.Buffer + exit := runWithDeps([]string{"skills", "list"}, &stdout, &stderr, appDeps{ + skillsDir: func() string { return primary }, + }) + if exit != 0 { + t.Fatalf("exit = %d, stderr = %s", exit, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "agents-only") { + t.Fatalf("list should include agents-only skill:\n%s", out) + } + if !strings.Contains(out, agents) { + t.Fatalf("list should show agents path:\n%s", out) + } +} + +func TestRunSkillsListPrimaryShadowsAgents(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + primary := t.TempDir() + writeSkillFixture(t, primary, "shared", "---\nname: shared\ndescription: From primary.\n---\nbody") + writeSkillFixture(t, agents, "shared", "---\nname: shared\ndescription: From agents.\n---\nbody") + + var stdout, stderr bytes.Buffer + exit := runWithDeps([]string{"skills", "list"}, &stdout, &stderr, appDeps{ + skillsDir: func() string { return primary }, + }) + if exit != 0 { + t.Fatalf("exit = %d, stderr = %s", exit, stderr.String()) + } + if !strings.Contains(stdout.String(), "From primary") { + t.Fatalf("primary should win list description:\n%s", stdout.String()) + } + if !strings.Contains(stdout.String(), primary) { + t.Fatalf("primary path should be shown:\n%s", stdout.String()) + } + if !strings.Contains(stderr.String(), `duplicate skill "shared"`) { + t.Fatalf("expected cross-root duplicate warning, got: %q", stderr.String()) + } +} + +func TestRunSkillInfoResolvesAgentsOnly(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + writeSkillFixture(t, agents, "agents-info", "---\nname: agents-info\ndescription: Agents info.\n---\nbody") + + primary := t.TempDir() + var stdout, stderr bytes.Buffer + exit := runWithDeps([]string{"skill", "info", "agents-info"}, &stdout, &stderr, appDeps{ + skillsDir: func() string { return primary }, + }) + if exit != 0 { + t.Fatalf("exit = %d, stderr = %s", exit, stderr.String()) + } + out := stdout.String() + if !strings.Contains(out, "agents-info") || !strings.Contains(out, "Agents info.") { + t.Fatalf("info missing agents skill:\n%s", out) + } + if strings.Contains(out, "source:") || strings.Contains(out, "hash:") { + t.Fatalf("agents-only info must not invent lock metadata:\n%s", out) + } + if !strings.Contains(out, agents) { + t.Fatalf("info should show agents path:\n%s", out) + } +} + func TestRunSkillsDoesNotLaunchTUI(t *testing.T) { var stdout, stderr bytes.Buffer launchCalled := false diff --git a/internal/plugins/activate.go b/internal/plugins/activate.go index 6a0bd7fb3..443e74bde 100644 --- a/internal/plugins/activate.go +++ b/internal/plugins/activate.go @@ -271,72 +271,55 @@ func skillSearchRoot(path string) string { // recognize a manifest path that points at the file rather than the directory. const skillFileName = "SKILL.md" -// MergedSkills loads the default skills directory plus the supplied plugin skill -// roots and returns one merged, name-deduplicated list (Content stripped, like -// skills.List) alongside the duplicate-name collisions across all roots. Earlier -// roots win a name clash, matching skills.Load's first-wins rule; the default dir -// is always considered first so a user skill shadows a same-named plugin skill. -// A bad root simply yields no skills rather than failing the merge. +// MergedSkills loads the default skills directory, the shared ~/.agents/skills +// root when present, plus the supplied plugin skill roots and returns one +// merged, name-deduplicated list (Content stripped, like skills.List) alongside +// the duplicate-name collisions across all roots. Earlier roots win a name +// clash, matching skills.Load's first-wins rule; the default dir is always +// considered first so a user skill shadows agents and plugins. A bad root simply +// yields no skills rather than failing the merge. func MergedSkills(defaultDir string, pluginRoots []string) ([]skills.Skill, []skills.DuplicateName) { return mergeSkills(defaultDir, pluginRoots, false) } -// mergeSkills merges the default skills dir and plugin roots into one sorted, -// name-deduplicated list. keepContent retains each skill's body (skills.Load) -// versus stripping it (skills.List). Roots are considered in order with the -// default dir first, so an earlier root wins a name clash; collisions are recorded -// rather than crashing, preserving the skills package's Duplicates behaviour. +// mergeSkills merges the primary skills dir, optional ~/.agents/skills, and +// plugin roots into one sorted, name-deduplicated list via skills.LoadFromRoots / +// ListFromRoots. keepContent retains each skill's body versus stripping it. +// Roots are considered in order with the default dir first, so an earlier root +// wins a name clash; collisions are recorded rather than crashing. func mergeSkills(defaultDir string, pluginRoots []string, keepContent bool) ([]skills.Skill, []skills.DuplicateName) { - merged := []skills.Skill{} - dups := []skills.DuplicateName{} - byName := map[string]skills.Skill{} - - roots := append([]string{defaultDir}, pluginRoots...) - for _, root := range roots { - if strings.TrimSpace(root) == "" { - continue - } - var loaded []skills.Skill - var err error - if keepContent { - loaded, err = skills.Load(root) - } else { - loaded, err = skills.List(root) - } - if err != nil { - continue - } - for _, skill := range loaded { - if winner, clash := byName[skill.Name]; clash { - dups = append(dups, skills.DuplicateName{Name: skill.Name, Winner: winner.Path, Loser: skill.Path}) - continue - } - byName[skill.Name] = skill - merged = append(merged, skill) + // Keep defaultDir injectable for tests rather than always calling DefaultDir. + roots := skills.GlobalRoots(defaultDir) + // GlobalRoots already includes AgentsDir; append plugin roots only. + for _, root := range pluginRoots { + if root = strings.TrimSpace(root); root != "" { + roots = append(roots, root) } } - - sort.Slice(merged, func(left int, right int) bool { - return merged[left].Name < merged[right].Name - }) + if keepContent { + merged, dups, _ := skills.LoadFromRoots(roots) + return merged, dups + } + merged, dups, _ := skills.ListFromRoots(roots) return merged, dups } // skillTool is a drop-in replacement for the core skill tool that resolves a -// named skill across the default skills directory PLUS the plugin skill roots, so -// plugin-declared skills surface in the agent's skill list. It keeps the core -// tool's name, schema, and read-only/allow safety so registering it simply -// overlays plugin skills onto the existing surface. +// named skill across the default skills directory, the shared ~/.agents/skills +// root, and plugin skill roots, so agents- and plugin-declared skills surface in +// the agent's skill list. It keeps the core tool's name, schema, and +// read-only/allow safety so registering it simply overlays multi-root discovery +// onto the existing surface. type skillTool struct { defaultDir string pluginRoots []string } -// NewSkillTool builds the plugin-aware skill tool. defaultDir is the standard +// NewSkillTool builds the multi-root skill tool. defaultDir is the standard // skills directory (skills.DefaultDir); pluginRoots are the plugin skill search -// roots from an ActivationResult. The returned tool merges both, deterministically -// deduplicating by name (default dir wins a clash) and listing all available -// skills when an unknown name is requested. +// roots from an ActivationResult. The returned tool merges primary + agents + +// plugins, deterministically deduplicating by name (default dir wins a clash) +// and listing all available skills when an unknown name is requested. func NewSkillTool(defaultDir string, pluginRoots []string) tools.Tool { return skillTool{defaultDir: defaultDir, pluginRoots: append([]string{}, pluginRoots...)} } @@ -345,7 +328,7 @@ func (tool skillTool) Name() string { return "skill" } func (tool skillTool) Description() string { return "Load a named Zero skill and return its instructions as the tool output. " + - "Skills are reusable, on-demand instruction sets (including any contributed by plugins). " + + "Skills are reusable, on-demand instruction sets (including shared ~/.agents/skills and any contributed by plugins). " + "Call this when a relevant skill exists; an unknown name returns the list of available skills." } diff --git a/internal/plugins/activate_test.go b/internal/plugins/activate_test.go index 937ea2658..63f4097fb 100644 --- a/internal/plugins/activate_test.go +++ b/internal/plugins/activate_test.go @@ -350,6 +350,7 @@ func TestActivateResolvesManifestRelativeSkillRoot(t *testing.T) { } func TestNewSkillToolSurfacesPluginSkillInList(t *testing.T) { + isolateAgentsHome(t) defaultDir := t.TempDir() writeTestSkill(t, defaultDir, "core-skill", "core body") pluginRoot := t.TempDir() @@ -384,6 +385,7 @@ func TestNewSkillToolSurfacesPluginSkillInList(t *testing.T) { } func TestMergeSkillRootsListsPluginSkillInAgentList(t *testing.T) { + isolateAgentsHome(t) defaultDir := t.TempDir() writeTestSkill(t, defaultDir, "core-skill", "core") @@ -404,6 +406,9 @@ func TestMergeSkillRootsListsPluginSkillInAgentList(t *testing.T) { } func TestMergedSkillsRecordsDuplicatesWithoutCrashing(t *testing.T) { + // Isolate AgentsDir so a host ~/.agents/skills cannot add extra collisions. + isolateAgentsHome(t) + defaultDir := t.TempDir() writeTestSkill(t, defaultDir, "shared", "default copy") @@ -426,6 +431,90 @@ func TestMergedSkillsRecordsDuplicatesWithoutCrashing(t *testing.T) { } } +// isolateAgentsHome points HOME/USERPROFILE at an empty temp home so AgentsDir +// does not pick up the developer's real ~/.agents/skills during unit tests. +func isolateAgentsHome(t *testing.T) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) +} + +func TestMergedSkillsIncludesAgentsWithoutPlugins(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + writeTestSkill(t, agents, "agents-skill", "from agents") + + defaultDir := t.TempDir() + listed, dups := MergedSkills(defaultDir, nil) + if len(dups) != 0 { + t.Fatalf("unexpected dups: %#v", dups) + } + if len(listed) != 1 || listed[0].Name != "agents-skill" { + t.Fatalf("agents skill should surface with no plugins, got %#v", listed) + } +} + +func TestMergedSkillsPriorityPrimaryAgentsPlugins(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + + primary := t.TempDir() + pluginRoot := t.TempDir() + writeTestSkill(t, primary, "shared", "primary body") + writeTestSkill(t, agents, "shared", "agents body") + writeTestSkill(t, agents, "agents-plugin", "agents body") + writeTestSkill(t, pluginRoot, "shared", "plugin body") + writeTestSkill(t, pluginRoot, "agents-plugin", "plugin body") + writeTestSkill(t, pluginRoot, "plugin-only", "plugin only") + + listed, dups := MergedSkills(primary, []string{pluginRoot}) + byName := map[string]string{} + for _, skill := range listed { + // Content is stripped by MergedSkills; use path origin instead. + byName[skill.Name] = skill.Path + } + if !strings.Contains(byName["shared"], primary) { + t.Fatalf("primary should win shared, path=%q", byName["shared"]) + } + if !strings.Contains(byName["agents-plugin"], agents) { + t.Fatalf("agents should win over plugin for agents-plugin, path=%q", byName["agents-plugin"]) + } + if !strings.Contains(byName["plugin-only"], pluginRoot) { + t.Fatalf("plugin-only missing, path=%q", byName["plugin-only"]) + } + if len(dups) < 2 { + t.Fatalf("expected cross-layer dups, got %#v", dups) + } +} + +func TestNewSkillToolLoadsAgentsSkillWithoutPluginRoots(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + writeTestSkill(t, agents, "agents-skill", "agents body") + + tool := NewSkillTool(t.TempDir(), nil) + got := tool.Run(context.Background(), map[string]any{"name": "agents-skill"}) + if got.Status != tools.StatusOK || !strings.Contains(got.Output, "agents body") { + t.Fatalf("expected agents skill body with zero plugin roots, got %q (%s)", got.Status, got.Output) + } +} + func TestActivateSkipsMalformedPluginAndContinues(t *testing.T) { registry := tools.NewRegistry() good := toolPlugin(t.TempDir(), ToolExtension{Name: "good", Command: "good", Permission: PermissionPrompt}) diff --git a/internal/skills/install.go b/internal/skills/install.go index c47d6b3cb..1ac8c17f7 100644 --- a/internal/skills/install.go +++ b/internal/skills/install.go @@ -221,6 +221,52 @@ func Info(dir string, name string) (SkillInfo, bool) { return info, true } +// InfoFromRoots resolves the named skill across discovery roots (earlier roots +// win). Lock source/hash are attached only when the winning skill lives under +// primaryDir and that dir's lockfile has an entry — agents-only skills return +// frontmatter + path with empty Source/Hash. primaryDir is typically the Zero +// write root (DefaultDir / skillsDir). +func InfoFromRoots(primaryDir string, roots []string, name string) (SkillInfo, bool) { + loaded, _, err := LoadFromRoots(roots) + if err != nil { + return SkillInfo{}, false + } + target := strings.TrimSpace(name) + var skill Skill + found := false + for _, candidate := range loaded { + if candidate.Name == target { + skill = candidate + found = true + break + } + } + if !found { + return SkillInfo{}, false + } + info := SkillInfo{Skill: skill} + primaryDir = strings.TrimSpace(primaryDir) + if primaryDir == "" { + return info, true + } + // Only attach lock metadata when the winner is from the primary write root. + // Compare path prefixes after cleaning so agents-only skills never pick up a + // Zero lock entry by name coincidence. + skillPath := filepath.Clean(skill.Path) + primaryRoot := filepath.Clean(primaryDir) + rel, err := filepath.Rel(primaryRoot, skillPath) + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return info, true + } + if lock, err := ReadLock(primaryDir); err == nil { + if entry, found := lock[skill.Name]; found { + info.Source = entry.Source + info.Hash = entry.Hash + } + } + return info, true +} + // ReadLock loads the lockfile from dir. A missing lockfile yields an empty map // with no error so callers can treat "no lockfile" as "nothing installed". func ReadLock(dir string) (map[string]LockEntry, error) { diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 16a411bed..1583c45b6 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -32,6 +32,9 @@ const skillFileName = "SKILL.md" // explicit ZERO_SKILLS_DIR override wins; otherwise it is // $XDG_DATA_HOME/zero/skills or ~/.local/share/zero/skills. The directory is // NOT created — a missing directory simply yields no skills. +// +// DefaultDir is the primary write root for install/remove/lock. Runtime discovery +// also considers AgentsDir and plugin skill roots via DiscoveryRoots / LoadFromRoots. func DefaultDir(env map[string]string) string { if override := strings.TrimSpace(envValue(env, "ZERO_SKILLS_DIR")); override != "" { return override @@ -56,6 +59,66 @@ func DefaultDir(env map[string]string) string { return filepath.Join(base, "zero", "skills") } +// AgentsDir returns ~/.agents/skills when that path exists and is a directory. +// It is a shared, read-only multi-agent skills root (Zero, Hermes, Claude Code, +// etc.) and is never the target of install/remove/lock. Missing, non-directory, +// or unresolvable home yields "" with no error and no directory creation. +// +// Home resolution matches other packages: HOME, then USERPROFILE, then +// os.UserHomeDir(). ZERO_SKILLS_DIR is intentionally ignored — agents is a +// pure convention path, not a Zero-specific override. +func AgentsDir(env map[string]string) string { + home := strings.TrimSpace(firstNonEmpty( + envValue(env, "HOME"), + envValue(env, "USERPROFILE"), + )) + if home == "" { + userHome, err := os.UserHomeDir() + if err != nil || strings.TrimSpace(userHome) == "" { + return "" + } + home = userHome + } + dir := filepath.Join(home, ".agents", "skills") + info, err := os.Stat(dir) + if err != nil || !info.IsDir() { + return "" + } + return dir +} + +// DiscoveryRoots returns ordered skill roots for runtime discovery: primary +// DefaultDir, optional AgentsDir when present, then pluginRoots. Empty strings +// are omitted. Earlier entries win on name clashes. +func DiscoveryRoots(env map[string]string, pluginRoots []string) []string { + return collectRoots(DefaultDir(env), AgentsDir(env), pluginRoots) +} + +// collectRoots assembles ordered non-empty skill roots. primary is typically +// DefaultDir (or an injected test dir); agents is typically AgentsDir's result. +func collectRoots(primary string, agents string, pluginRoots []string) []string { + roots := make([]string, 0, 2+len(pluginRoots)) + if primary = strings.TrimSpace(primary); primary != "" { + roots = append(roots, primary) + } + if agents = strings.TrimSpace(agents); agents != "" { + roots = append(roots, agents) + } + for _, root := range pluginRoots { + if root = strings.TrimSpace(root); root != "" { + roots = append(roots, root) + } + } + return roots +} + +// GlobalRoots returns discovery roots for management CLI list/info: an explicit +// primary write/root dir (usually skillsDir / DefaultDir) plus AgentsDir when +// present. Plugin roots are excluded from management UX. +func GlobalRoots(primary string) []string { + return collectRoots(primary, AgentsDir(nil), nil) +} + // DuplicateName records two skills that resolved to the same frontmatter name. // Winner is the SKILL.md path of the skill that was kept (the one in the // lexicographically-first directory); Loser is the path that was dropped. @@ -77,14 +140,69 @@ type DuplicateName struct { // winner regardless of sort stability. Use Duplicates to surface a warning about // any such collisions. // -// NOTE: Load scans one root. Agent startup merges plugin-declared skill roots -// separately during plugin activation, so the runtime skill surface can include -// both the default directory and skills bundled by active plugins. +// NOTE: Load scans one root. Runtime discovery uses LoadFromRoots / +// DiscoveryRoots (primary DefaultDir, optional ~/.agents/skills, then plugin +// skill roots). Prefer those multi-root helpers for agent/CLI discovery; keep +// Load for single-dir install/write call sites. func Load(dir string) ([]Skill, error) { skills, _, err := load(dir) return skills, err } +// LoadFromRoots loads and merges skills from the provided directories (earlier +// entries win on name clashes). Missing and empty roots are skipped. Intra-root +// and cross-root collisions are reported as DuplicateName. A root that fails to +// load for a non-missing reason is skipped rather than failing the whole merge, +// matching the previous plugin-merge behaviour. +func LoadFromRoots(dirs []string) ([]Skill, []DuplicateName, error) { + merged := make([]Skill, 0) + duplicates := []DuplicateName{} + byName := map[string]int{} + + for _, dir := range dirs { + if strings.TrimSpace(dir) == "" { + continue + } + loaded, rootDups, err := load(dir) + if err != nil { + // Fail open per root: one bad directory must not hide the rest. + continue + } + duplicates = append(duplicates, rootDups...) + for _, skill := range loaded { + if winnerIdx, clash := byName[skill.Name]; clash { + duplicates = append(duplicates, DuplicateName{ + Name: skill.Name, + Winner: merged[winnerIdx].Path, + Loser: skill.Path, + }) + continue + } + byName[skill.Name] = len(merged) + merged = append(merged, skill) + } + } + + sort.Slice(merged, func(left int, right int) bool { + return merged[left].Name < merged[right].Name + }) + return merged, duplicates, nil +} + +// ListFromRoots is like LoadFromRoots but strips Content (like List). +func ListFromRoots(dirs []string) ([]Skill, []DuplicateName, error) { + loaded, dups, err := LoadFromRoots(dirs) + if err != nil { + return nil, dups, err + } + listed := make([]Skill, 0, len(loaded)) + for _, skill := range loaded { + skill.Content = "" + listed = append(listed, skill) + } + return listed, dups, nil +} + // Duplicates returns the duplicate-name collisions Load resolved by the // first-directory-wins rule, so a caller can warn the user that a shadowed skill // was dropped. A missing directory yields no duplicates and no error. @@ -281,7 +399,7 @@ func splitFrontmatter(normalized string) (string, string, bool) { // Matching is case-insensitive on the key; the first occurrence wins. func frontmatterValue(frontmatter string, key string) string { prefix := strings.ToLower(key) + ":" - for _, line := range strings.Split(frontmatter, "\n") { + for line := range strings.SplitSeq(frontmatter, "\n") { trimmed := strings.TrimSpace(line) if strings.HasPrefix(strings.ToLower(trimmed), prefix) { value := strings.TrimSpace(trimmed[len(prefix):]) @@ -297,3 +415,12 @@ func envValue(env map[string]string, key string) string { } return os.Getenv(key) } + +func firstNonEmpty(values ...string) string { + for _, value := range values { + if strings.TrimSpace(value) != "" { + return value + } + } + return "" +} diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 31f8f6d0f..7be63f64a 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -292,3 +292,216 @@ func TestDefaultDirFallsBackToHome(t *testing.T) { t.Fatalf("DefaultDir = %q, want %q", got, want) } } + +func TestAgentsDirReturnsExistingDirectory(t *testing.T) { + home := t.TempDir() + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + got := AgentsDir(map[string]string{"HOME": home}) + if got != agents { + t.Fatalf("AgentsDir = %q, want %q", got, agents) + } +} + +func TestAgentsDirMissingIsEmpty(t *testing.T) { + home := t.TempDir() + got := AgentsDir(map[string]string{"HOME": home}) + if got != "" { + t.Fatalf("AgentsDir for missing path = %q, want empty", got) + } +} + +func TestAgentsDirFileNotDirIsEmpty(t *testing.T) { + home := t.TempDir() + agentsParent := filepath.Join(home, ".agents") + if err := os.MkdirAll(agentsParent, 0o755); err != nil { + t.Fatal(err) + } + // skills is a file, not a directory + if err := os.WriteFile(filepath.Join(agentsParent, "skills"), []byte("not a dir"), 0o644); err != nil { + t.Fatal(err) + } + got := AgentsDir(map[string]string{"HOME": home}) + if got != "" { + t.Fatalf("AgentsDir for file = %q, want empty", got) + } +} + +func TestAgentsDirHonorsUserProfile(t *testing.T) { + home := t.TempDir() + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + got := AgentsDir(map[string]string{"USERPROFILE": home}) + if got != agents { + t.Fatalf("AgentsDir via USERPROFILE = %q, want %q", got, agents) + } +} + +func TestAgentsDirIgnoresZeroSkillsDir(t *testing.T) { + home := t.TempDir() + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + // ZERO_SKILLS_DIR must not redirect or suppress AgentsDir. + got := AgentsDir(map[string]string{ + "HOME": home, + "ZERO_SKILLS_DIR": filepath.Join(home, "zero-only"), + }) + if got != agents { + t.Fatalf("AgentsDir with ZERO_SKILLS_DIR set = %q, want %q", got, agents) + } +} + +func TestAgentsDirUnresolvableHomeIsEmpty(t *testing.T) { + // Empty env map values + no real UserHomeDir fallback is hard to force, but + // empty HOME/USERPROFILE with an empty home should still not panic. + // When UserHomeDir works this may return a host path; only assert no panic + // and that a deliberately empty-looking override path is not invented from ZERO. + _ = AgentsDir(map[string]string{"HOME": "", "USERPROFILE": ""}) +} + +func TestDiscoveryRootsOrderAndOmission(t *testing.T) { + home := t.TempDir() + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + primary := filepath.Join(home, "zero-skills") + env := map[string]string{ + "HOME": home, + "ZERO_SKILLS_DIR": primary, + } + roots := DiscoveryRoots(env, []string{"", " /plugin/a ", "plugin/b"}) + want := []string{primary, agents, "/plugin/a", "plugin/b"} + if len(roots) != len(want) { + t.Fatalf("DiscoveryRoots = %#v, want %#v", roots, want) + } + for i := range want { + if roots[i] != want[i] { + t.Fatalf("DiscoveryRoots[%d] = %q, want %q (full %#v)", i, roots[i], want[i], roots) + } + } +} + +func TestDiscoveryRootsOmitsMissingAgents(t *testing.T) { + home := t.TempDir() + primary := filepath.Join(home, "zero-skills") + env := map[string]string{ + "HOME": home, + "ZERO_SKILLS_DIR": primary, + } + roots := DiscoveryRoots(env, nil) + if len(roots) != 1 || roots[0] != primary { + t.Fatalf("DiscoveryRoots without agents = %#v, want only primary", roots) + } +} + +func TestLoadFromRootsPrimaryWinsOverAgents(t *testing.T) { + primary := t.TempDir() + agents := t.TempDir() + writeSkill(t, primary, "shared", "---\nname: shared\n---\nprimary body\n") + writeSkill(t, agents, "shared", "---\nname: shared\n---\nagents body\n") + writeSkill(t, agents, "agents-only", "---\nname: agents-only\n---\nagents only\n") + + loaded, dups, err := LoadFromRoots([]string{primary, agents}) + if err != nil { + t.Fatalf("LoadFromRoots: %v", err) + } + byName := map[string]Skill{} + for _, skill := range loaded { + byName[skill.Name] = skill + } + if byName["shared"].Content != "primary body" { + t.Fatalf("primary should win shared, got %q", byName["shared"].Content) + } + if byName["agents-only"].Content != "agents only" { + t.Fatalf("agents-only missing: %#v", loaded) + } + if len(dups) != 1 || dups[0].Name != "shared" { + t.Fatalf("expected one shared duplicate, got %#v", dups) + } +} + +func TestLoadFromRootsSkipsEmptyAndMissing(t *testing.T) { + primary := t.TempDir() + writeSkill(t, primary, "solo", "---\nname: solo\n---\nbody\n") + loaded, dups, err := LoadFromRoots([]string{"", filepath.Join(t.TempDir(), "missing"), primary}) + if err != nil { + t.Fatalf("LoadFromRoots: %v", err) + } + if len(loaded) != 1 || loaded[0].Name != "solo" { + t.Fatalf("expected only solo, got %#v", loaded) + } + if len(dups) != 0 { + t.Fatalf("unexpected dups: %#v", dups) + } +} + +func TestListFromRootsStripsContent(t *testing.T) { + dir := t.TempDir() + writeSkill(t, dir, "demo", "---\nname: demo\n---\nbody content\n") + listed, _, err := ListFromRoots([]string{dir}) + if err != nil { + t.Fatalf("ListFromRoots: %v", err) + } + if len(listed) != 1 || listed[0].Name != "demo" { + t.Fatalf("unexpected listed: %#v", listed) + } + if listed[0].Content != "" { + t.Fatalf("ListFromRoots must strip Content, got %q", listed[0].Content) + } +} + +func TestGlobalRootsIncludesAgents(t *testing.T) { + home := t.TempDir() + agents := filepath.Join(home, ".agents", "skills") + if err := os.MkdirAll(agents, 0o755); err != nil { + t.Fatal(err) + } + // Point HOME so AgentsDir finds the temp agents root. + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + primary := filepath.Join(home, "primary") + roots := GlobalRoots(primary) + if len(roots) != 2 || roots[0] != primary || roots[1] != agents { + t.Fatalf("GlobalRoots = %#v, want [primary, agents]", roots) + } +} + +func TestInfoFromRootsAgentsOnlyHasNoLock(t *testing.T) { + primary := t.TempDir() + agents := t.TempDir() + writeSkill(t, agents, "shared-agents", "---\nname: agents-skill\ndescription: from agents\n---\nbody\n") + info, ok := InfoFromRoots(primary, []string{primary, agents}, "agents-skill") + if !ok { + t.Fatal("expected agents skill to resolve") + } + if info.Skill.Name != "agents-skill" || info.Skill.Description != "from agents" { + t.Fatalf("unexpected skill: %#v", info.Skill) + } + if info.Source != "" || info.Hash != "" { + t.Fatalf("agents-only skill must not carry lock metadata, got source=%q hash=%q", info.Source, info.Hash) + } +} + +func TestInfoFromRootsPrimaryLockMetadata(t *testing.T) { + primary := t.TempDir() + writeSkill(t, primary, "demo", "---\nname: demo\n---\nbody\n") + // Write a lockfile entry the way install would. + lockPath := filepath.Join(primary, LockFileName) + if err := os.WriteFile(lockPath, []byte(`{"demo":{"source":"file:///src","hash":"sha256:abc"}}`), 0o644); err != nil { + t.Fatal(err) + } + info, ok := InfoFromRoots(primary, []string{primary}, "demo") + if !ok { + t.Fatal("expected primary skill") + } + if info.Source != "file:///src" || info.Hash != "sha256:abc" { + t.Fatalf("lock metadata missing: %#v", info) + } +} From ee1d14add0886b52bef2e25af879453f84025dd1 Mon Sep 17 00:00:00 2001 From: Leonardo Faoro Date: Wed, 15 Jul 2026 16:03:34 +0300 Subject: [PATCH 2/4] docs(skills): note lock stays on the primary write root Document that install/remove/lock target only the primary Zero skills directory, matching multi-root discovery write-root isolation. --- CHANGELOG.md | 2 +- docs/EXTENDING.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 83d61946b..a9361e7fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,7 +129,7 @@ tagged. Until then, source builds report the version `dev`. ### Added - Shared multi-agent skills discovery: when present, `~/.agents/skills` is searched after the primary Zero skills dir (and before plugin skill roots). `zero skills list` / `info` and the runtime `skill` - tool share one multi-root discovery path; install/remove still target only the Zero skills directory. + tool share one multi-root discovery path; install/remove/lock still target only the Zero skills directory. - `SECURITY.md` with a private vulnerability-reporting path, `CODE_OF_CONDUCT.md`, this changelog, and GitHub issue/PR templates. - Interactive `/theme` picker: bare `/theme` opens a popup that live-previews each palette as you move diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index 6cb1ece36..a748d95f1 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -108,10 +108,10 @@ Discovery roots (earlier wins on name collisions): 2. **Shared multi-agent dir** — `~/.agents/skills/` when present (read-only discovery; never an install target) 3. **Plugin skill roots** — skills bundled by active plugins (section 6) -A missing directory is fine — Zero just omits it. Management commands (`zero skills add` / `remove`) always write to the primary Zero dir only; `list` / `info` search primary + `~/.agents/skills`. +A missing directory is fine — Zero just omits it. Management commands (`zero skills add` / `remove` / `lock`) always write to the primary Zero dir only; `list` / `info` search primary + `~/.agents/skills`. ```text -~/.local/share/zero/skills/ # primary (install/remove target) +~/.local/share/zero/skills/ # primary (install/remove/lock target) run-benchmarks/ SKILL.md write-changelog/ From 4be858eff9a6f9fd4a828beeefd30063af477fe3 Mon Sep 17 00:00:00 2001 From: Leonardo Faoro Date: Wed, 15 Jul 2026 16:03:40 +0300 Subject: [PATCH 3/4] fix(skills): bubble primary root load errors from LoadFromRoots Return non-missing failures from the first non-empty skills root so a broken primary dir is not reported as an empty skill list. Keep fail-open behavior for optional roots (~/.agents/skills, plugins). --- internal/skills/skills.go | 22 +++++++++++---- internal/skills/skills_test.go | 51 ++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 1583c45b6..6eb3fe8e9 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -150,14 +150,20 @@ func Load(dir string) ([]Skill, error) { } // LoadFromRoots loads and merges skills from the provided directories (earlier -// entries win on name clashes). Missing and empty roots are skipped. Intra-root -// and cross-root collisions are reported as DuplicateName. A root that fails to -// load for a non-missing reason is skipped rather than failing the whole merge, -// matching the previous plugin-merge behaviour. +// entries win on name clashes). Empty roots are skipped. Missing directories are +// treated as empty (same as Load). Intra-root and cross-root collisions are +// reported as DuplicateName. +// +// The first non-empty root is the required primary: non-missing load failures +// (permission, I/O, not a directory, etc.) are returned so callers do not +// confuse a broken primary skills dir with "no skills". Later optional roots +// (e.g. ~/.agents/skills, plugin roots) fail open so one bad optional directory +// does not hide the rest. func LoadFromRoots(dirs []string) ([]Skill, []DuplicateName, error) { merged := make([]Skill, 0) duplicates := []DuplicateName{} byName := map[string]int{} + primary := true for _, dir := range dirs { if strings.TrimSpace(dir) == "" { @@ -165,9 +171,15 @@ func LoadFromRoots(dirs []string) ([]Skill, []DuplicateName, error) { } loaded, rootDups, err := load(dir) if err != nil { - // Fail open per root: one bad directory must not hide the rest. + if primary { + // Required primary root: surface real I/O failures instead of + // collapsing to an empty skill list. + return nil, nil, err + } + // Optional roots fail open: one bad directory must not hide the rest. continue } + primary = false duplicates = append(duplicates, rootDups...) for _, skill := range loaded { if winnerIdx, clash := byName[skill.Name]; clash { diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 7be63f64a..4dd8e6a46 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -442,6 +442,57 @@ func TestLoadFromRootsSkipsEmptyAndMissing(t *testing.T) { } } +func TestLoadFromRootsBubblesPrimaryError(t *testing.T) { + // A regular file is not a directory; ReadDir returns a non-ErrNotExist error. + // That must bubble from the first non-empty root instead of looking like "no skills". + notDir := filepath.Join(t.TempDir(), "not-a-dir") + if err := os.WriteFile(notDir, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + optional := t.TempDir() + writeSkill(t, optional, "fallback", "---\nname: fallback\n---\nbody\n") + + loaded, dups, err := LoadFromRoots([]string{notDir, optional}) + if err == nil { + t.Fatalf("expected primary root error, got loaded=%#v dups=%#v", loaded, dups) + } + if loaded != nil { + t.Fatalf("expected nil skills on primary error, got %#v", loaded) + } + if dups != nil { + t.Fatalf("expected nil dups on primary error, got %#v", dups) + } +} + +func TestLoadFromRootsOptionalRootFailOpen(t *testing.T) { + primary := t.TempDir() + writeSkill(t, primary, "keep", "---\nname: keep\n---\nbody\n") + notDir := filepath.Join(t.TempDir(), "not-a-dir") + if err := os.WriteFile(notDir, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + agents := t.TempDir() + writeSkill(t, agents, "agents-only", "---\nname: agents-only\n---\nbody\n") + + loaded, dups, err := LoadFromRoots([]string{primary, notDir, agents}) + if err != nil { + t.Fatalf("optional root failure must not fail merge: %v", err) + } + byName := map[string]Skill{} + for _, skill := range loaded { + byName[skill.Name] = skill + } + if byName["keep"].Name != "keep" { + t.Fatalf("primary skill missing: %#v", loaded) + } + if byName["agents-only"].Name != "agents-only" { + t.Fatalf("later optional skill should still load: %#v", loaded) + } + if len(dups) != 0 { + t.Fatalf("unexpected dups: %#v", dups) + } +} + func TestListFromRootsStripsContent(t *testing.T) { dir := t.TempDir() writeSkill(t, dir, "demo", "---\nname: demo\n---\nbody content\n") From 7fad1ecef138e373cf6ec04d62fab89513bfc978 Mon Sep 17 00:00:00 2001 From: Leonardo Faoro Date: Wed, 15 Jul 2026 16:30:26 +0300 Subject: [PATCH 4/4] fix(skills): treat non-directory skills root as error on Windows Windows maps ENOTDIR to ErrNotExist, so ReadDir on a regular file looked like a missing skills dir. Reclassify existing non-directories and assert the portable load behavior in tests. --- internal/skills/skills.go | 15 +++++++++++++++ internal/skills/skills_test.go | 30 ++++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 2 deletions(-) diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 6eb3fe8e9..6a7550566 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -28,6 +28,11 @@ type Skill struct { const skillFileName = "SKILL.md" +// errNotDirectory is returned when a skills root path exists but is not a +// directory. On Windows, os.ReadDir reports that case as ErrNotExist +// (ENOTDIR aliases ERROR_PATH_NOT_FOUND), so load reclassifies it explicitly. +var errNotDirectory = errors.New("not a directory") + // DefaultDir resolves the skills directory, mirroring sessions.DefaultRoot. An // explicit ZERO_SKILLS_DIR override wins; otherwise it is // $XDG_DATA_HOME/zero/skills or ~/.local/share/zero/skills. The directory is @@ -261,6 +266,16 @@ func load(dir string) ([]Skill, []DuplicateName, error) { entries, err := os.ReadDir(dir) if err != nil { if errors.Is(err, os.ErrNotExist) { + // Windows maps ENOTDIR to ERROR_PATH_NOT_FOUND, which is also + // ErrNotExist. A primary skills path that exists but is not a + // directory must still surface as a load error, not "no skills". + if info, statErr := os.Stat(dir); statErr == nil && !info.IsDir() { + return nil, nil, &os.PathError{ + Op: "readdir", + Path: dir, + Err: errNotDirectory, + } + } return []Skill{}, nil, nil } return nil, nil, err diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 4dd8e6a46..da392cbf5 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -1,6 +1,7 @@ package skills import ( + "errors" "os" "path/filepath" "strings" @@ -147,6 +148,25 @@ func TestLoadMissingDirYieldsEmpty(t *testing.T) { } } +func TestLoadNotDirectoryErrors(t *testing.T) { + // Portable across Unix and Windows: a regular file is not a skills root. + // Windows reports ReadDir(file) as ErrNotExist; load must reclassify it. + notDir := filepath.Join(t.TempDir(), "not-a-dir") + if err := os.WriteFile(notDir, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + loaded, err := Load(notDir) + if err == nil { + t.Fatalf("expected error for non-directory skills root, got loaded=%#v", loaded) + } + if errors.Is(err, os.ErrNotExist) && !errors.Is(err, errNotDirectory) { + t.Fatalf("non-directory skills root must not look missing: %v", err) + } + if loaded != nil { + t.Fatalf("expected nil skills on non-directory root, got %#v", loaded) + } +} + func TestLoadSortsByName(t *testing.T) { dir := t.TempDir() writeSkill(t, dir, "zeta", "body") @@ -443,8 +463,9 @@ func TestLoadFromRootsSkipsEmptyAndMissing(t *testing.T) { } func TestLoadFromRootsBubblesPrimaryError(t *testing.T) { - // A regular file is not a directory; ReadDir returns a non-ErrNotExist error. - // That must bubble from the first non-empty root instead of looking like "no skills". + // A regular file is not a directory. On Unix ReadDir fails with ENOTDIR; on + // Windows that case is aliased to ErrNotExist, so load must reclassify an + // existing non-directory and bubble it from the first non-empty root. notDir := filepath.Join(t.TempDir(), "not-a-dir") if err := os.WriteFile(notDir, []byte("x"), 0o644); err != nil { t.Fatal(err) @@ -456,6 +477,11 @@ func TestLoadFromRootsBubblesPrimaryError(t *testing.T) { if err == nil { t.Fatalf("expected primary root error, got loaded=%#v dups=%#v", loaded, dups) } + // Unix returns ENOTDIR; Windows reclassifies via errNotDirectory because + // ENOTDIR aliases ErrNotExist there. Either way it must not look missing. + if errors.Is(err, os.ErrNotExist) && !errors.Is(err, errNotDirectory) { + t.Fatalf("primary non-directory must not look missing: %v", err) + } if loaded != nil { t.Fatalf("expected nil skills on primary error, got %#v", loaded) }