feat(skills): discover shared ~/.agents/skills with multi-root skill loading#696
Conversation
…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 Gitlawb#610
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughZero discovers skills from the primary Zero directory, optional ChangesMulti-root skills discovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant activatePlugins
participant internalSkills
participant skillTool
Agent->>activatePlugins: start activation
activatePlugins->>internalSkills: resolve primary, agents, and plugin roots
internalSkills-->>activatePlugins: merged skills and duplicate names
activatePlugins->>skillTool: register multi-root skill tool
skillTool-->>Agent: resolve requested skill
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
internal/cli/skills.go (1)
94-107: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNon-empty header still says "Zero Skills:" even though the list can now include ~/.agents/skills-only entries.
The empty-case message was generalized to "No skills found." in this same diff (line 96), but the header for a non-empty result (line 98) still hardcodes "Zero Skills:" — which is misleading once a listed skill actually came from the shared, non-Zero-specific
~/.agents/skillsroot rather than the Zero-specific directory.✏️ Suggested tweak
- lines := []string{"Zero Skills:"} + lines := []string{"Skills:"}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/skills.go` around lines 94 - 107, Update the non-empty header in formatSkillList to use a generic skills label instead of "Zero Skills:". Keep the existing empty-case message and per-skill formatting unchanged.internal/skills/skills.go (2)
97-113: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
collectRootsdoesn't dedupe identical root paths.If
primary,agents, or an entry inpluginRootsresolve to the same directory (e.g.ZERO_SKILLS_DIRpointed at~/.agents/skills, or a plugin declaring a skill root that happens to equalAgentsDir()), that directory is scanned twice byLoadFromRoots/load(), and every skill inside it will be reported as a false cross-rootDuplicateNameagainst itself.🛠️ Proposed fix: dedupe by cleaned path
func collectRoots(primary string, agents string, pluginRoots []string) []string { roots := make([]string, 0, 2+len(pluginRoots)) + seen := make(map[string]bool, 2+len(pluginRoots)) + add := func(root string) { + root = strings.TrimSpace(root) + if root == "" { + return + } + key := filepath.Clean(root) + if seen[key] { + return + } + seen[key] = true + roots = append(roots, root) + } - 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) - } - } + add(primary) + add(agents) + for _, root := range pluginRoots { + add(root) + } return roots }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/skills.go` around lines 97 - 113, Update collectRoots to deduplicate roots by their cleaned path while preserving the existing primary, agents, and plugin ordering and whitespace filtering. Track already-added cleaned paths and skip subsequent entries that resolve to the same directory, so LoadFromRoots scans each root only once.
90-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
DiscoveryRootsis dead outside tests
mergeSkillsstill rebuilds the same root list by hand. Either threadenvthrough and callDiscoveryRootshere, or remove the helper if the env-driven variant isn’t needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/skills/skills.go` around lines 90 - 95, Update mergeSkills to use DiscoveryRoots with the existing environment and plugin roots instead of rebuilding the root list manually, threading env through its callers as needed. Preserve the documented ordering and empty-root filtering provided by DiscoveryRoots, and remove any redundant manual root assembly.internal/cli/skills_test.go (1)
138-214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
isolateCLIAgentsHomeinstead of re-inlining the HOME/USERPROFILE setup.These three new tests each duplicate the exact
home := t.TempDir(); t.Setenv("HOME", home); t.Setenv("USERPROFILE", home)sequence thatisolateCLIAgentsHome(26-31) was just introduced to replace — the helper is used in every other test in this file except these three.♻️ Example fix for one of the three (same pattern applies to the other two)
func TestRunSkillsListIncludesAgentsOnlySkill(t *testing.T) { - home := t.TempDir() - t.Setenv("HOME", home) - t.Setenv("USERPROFILE", home) + isolateCLIAgentsHome(t) + home := os.Getenv("HOME") agents := filepath.Join(home, ".agents", "skills")Note: this requires exposing
homeback out of the helper (e.g. have it return the temp dir) rather than re-readingos.Getenv, if you want to avoid the extra indirection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/skills_test.go` around lines 138 - 214, Replace the duplicated HOME and USERPROFILE setup in TestRunSkillsListIncludesAgentsOnlySkill, TestRunSkillsListPrimaryShadowsAgents, and TestRunSkillInfoResolvesAgentsOnly with isolateCLIAgentsHome. Update that helper to return the created temporary home directory, and use the returned value where these tests need to construct the .agents/skills path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/EXTENDING.md`:
- Around line 105-122: Update the management-command description in
docs/EXTENDING.md lines 105-122 to state that lock, alongside add and remove,
writes only to the primary Zero skills directory; update CHANGELOG.md lines
130-132 to mention install/remove/lock isolation, preserving the existing
documentation scope.
In `@internal/skills/skills.go`:
- Around line 152-190: The LoadFromRoots function currently discards errors from
every root, including the required primary skills directory. Preserve fail-open
behavior for optional roots, but return the loading error when the first
configured root fails, while retaining the existing merge and duplicate handling
for successfully loaded roots.
---
Nitpick comments:
In `@internal/cli/skills_test.go`:
- Around line 138-214: Replace the duplicated HOME and USERPROFILE setup in
TestRunSkillsListIncludesAgentsOnlySkill, TestRunSkillsListPrimaryShadowsAgents,
and TestRunSkillInfoResolvesAgentsOnly with isolateCLIAgentsHome. Update that
helper to return the created temporary home directory, and use the returned
value where these tests need to construct the .agents/skills path.
In `@internal/cli/skills.go`:
- Around line 94-107: Update the non-empty header in formatSkillList to use a
generic skills label instead of "Zero Skills:". Keep the existing empty-case
message and per-skill formatting unchanged.
In `@internal/skills/skills.go`:
- Around line 97-113: Update collectRoots to deduplicate roots by their cleaned
path while preserving the existing primary, agents, and plugin ordering and
whitespace filtering. Track already-added cleaned paths and skip subsequent
entries that resolve to the same directory, so LoadFromRoots scans each root
only once.
- Around line 90-95: Update mergeSkills to use DiscoveryRoots with the existing
environment and plugin roots instead of rebuilding the root list manually,
threading env through its callers as needed. Preserve the documented ordering
and empty-root filtering provided by DiscoveryRoots, and remove any redundant
manual root assembly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8e9e035-c19e-4997-9130-3447afe2855e
📒 Files selected for processing (13)
CHANGELOG.mddocs/EXTENDING.mddocs/HOW_ZERO_WORKS.mdinternal/cli/distribution.gointernal/cli/plugin_activate.gointernal/cli/plugin_activate_test.gointernal/cli/skills.gointernal/cli/skills_test.gointernal/plugins/activate.gointernal/plugins/activate_test.gointernal/skills/install.gointernal/skills/skills.gointernal/skills/skills_test.go
Document that install/remove/lock target only the primary Zero skills directory, matching multi-root discovery write-root isolation.
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).
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.
There was a problem hiding this comment.
Verified on the PR branch: build/vet/gofmt clean and the new skills/plugins/cli skills tests pass; the only cli failures are the pre-existing "provider anthropic requires model" ones, which reproduce identically on clean origin/main. The merge is sound per-root symlink confinement keeps a hostile ~/.agents/skills entry from escaping its root or picking up a Zero lock entry (filepath.Rel guard in InfoFromRoots), optional roots fail open, and primary non-directory errors now bubble consistently across Windows and Unix. Minor heads-up: ~/.agents/skills is auto-discovered by the permission-allow skill tool with no prompt, so skills placed there by other tools load without asking that's the intended feature and it's read-only/confined, just calling out the trust-surface expansion. Approving.
3 participants
Summary
Add
~/.agents/skillsas a read-only global skills discovery root and unify CLI and runtime skill loading on one multi-root path.Discovery (first wins by name):
ZERO_SKILLS_DIR| XDG/home)~/.agents/skills(if present)Write root unchanged:
zero skills install/remove/ lock still target only the primary Zero skills directory.This also collapses the previous half-wiring where the multi-root skill tool was
only registered when plugins were active. Bare runs and plugin runs now share the same multi-root
skilltool.Linked issue
Fixes #610
Why
Shared
*/SKILL.mdpacks are emerging as a multi-agent convention. Without this, users must duplicate skills into Zero’s own directory to use them alongside other agents (Hermes, Claude Code, etc.).Zero-specific skills still win on name conflicts.
What changed
internal/skills:AgentsDir,DiscoveryRoots,GlobalRoots,LoadFromRoots,ListFromRoots,InfoFromRootsinternal/plugins:mergeSkillsbecomes a thin wrapper over the skills multi-root API
CLI
skills list/infouse the same global discovery roots as runtime (minus plugins)Always register the multi-root skill tool at activation (even with 0 plugins)
Tests for agents-dir discovery, first-wins priority, and write-root isolation
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).gg
Summary by CodeRabbit
~/.agents/skillsdirectory.zero skills listandzero skills infosearch across the primary directory plus~/.agents/skills(and plugin roots), with first-root wins for name collisions and winner/loser warnings.