wedge stage 10: distribution (skill/plugin install, tools make)#184
Merged
Conversation
Add a file-backed distribution surface so skills and plugins can be
installed from a git URL or local path, and new plugin-tools can be
scaffolded. No central registry: a source is just a git URL or path.
- internal/skills/install.go: Install/Remove/Info + skills.lock. Validates
SKILL.md frontmatter via the existing parser, copies it into the skills
dir, and records a sha256 content hash. Reinstall surfaces the hash
change; a clash with a different source is refused without --force.
- internal/plugins/install.go: same flow validated against the plugin
manifest schema, copying the whole plugin tree (minus .git) so the
plugin is activatable. plugins.lock records source + hash.
- internal/tools/scaffold.go: generate a prompt-gated plugin-tool skeleton
(manifest + runnable shell/node/python entry stub) in the toolbox dir.
- CLI: zero skill {add,info,remove}, zero plugin {add,remove}, and a new
zero tools {make,list}; wired in app.go/extensions.go/skills.go.
Safety: fetch honors the process environment (proxy/egress) and disables
git credential prompts; install never executes fetched content (skills are
markdown; plugin install scripts are never run) and installed plugins still
go through normal activation with permission gating.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds skill and plugin installers with deterministic hashing and lockfiles, a tool scaffold producing plugin manifests and entry stubs, CLI handlers for skill/plugin/tools commands (add/info/remove, make/list) with JSON/text output, tests covering lifecycle and edge cases, and app wiring for user plugin/tool directory resolution. ChangesDistribution infrastructure for skills, plugins, and tools
Sequence Diagram(s)sequenceDiagram
participant CLI
participant DistHandler
participant PluginInstaller
participant GitRunner
participant FS
CLI->>DistHandler: zero plugin add <source> (--force/--json)
DistHandler->>PluginInstaller: Install(ctx, InstallOptions{Source, Dir, Force})
PluginInstaller->>GitRunner: clone (for remote sources)
GitRunner-->>PluginInstaller: local checkout path
PluginInstaller->>FS: locate plugin.json, read files
PluginInstaller->>PluginInstaller: hashTree -> sha256:<hex>
PluginInstaller->>FS: copyTree -> install dir (skip .git, no symlinks)
PluginInstaller->>FS: write plugins.lock (canonical source + hash)
PluginInstaller-->>DistHandler: InstallResult (ManifestPath, Hash, Updated)
DistHandler-->>CLI: formatted output (JSON or human text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (1)
internal/tools/scaffold_test.go (1)
14-147: ⚡ Quick winCover the non-default runtimes too.
The suite only exercises the default shell scaffold, but
runtimeSpecForhas separate Node and Python branches with different filenames, commands, and modes. A small table-driven test overRuntimeShell,RuntimeNode, andRuntimePythonwould catch template/manifest drift in the branches users can actually select.🤖 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/tools/scaffold_test.go` around lines 14 - 147, Add a table-driven test that calls tools.Scaffold with each runtime variant (RuntimeShell, RuntimeNode, RuntimePython) to exercise the separate branches in runtimeSpecFor; for each runtime, assert Scaffold returns no error, that result.ManifestPath and result.EntryPath exist and live under the plugin dir, that plugins.ParseManifest succeeds for the generated manifest, and that the tool command/args reference the entry (reuse commandReferencesEntry). Keep assertions the same as in TestScaffoldCreatesManifestAndStub but loop over runtimes (or create a new Test like TestScaffoldCoversAllRuntimes) and set ScaffoldOptions.Runtime accordingly to catch template/manifest drift across the Node and Python branches.
🤖 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 `@internal/cli/distribution.go`:
- Around line 340-343: The "Next steps" scaffold message incorrectly suggests
"zero plugins list"; update the string literal "Run `zero plugins list` to
confirm it loads." to "Run `zero tools list` to confirm it loads." so the hint
checks the tool discovery path (especially when deps.toolsDir() !=
deps.pluginsDir()); locate and change the message in the Next steps block
generated in distribution.go.
- Around line 372-386: The toolbox listing currently calls plugins.Load and
ignores non-fatal diagnostics (result.Diagnostics), causing broken plugins to
silently disappear; update the runToolsList flow to surface those diagnostics by
checking result.Diagnostics after plugins.Load and either (a) include them in
the JSON response (extend the payload passed to writePrettyJSON to include
Diagnostics and use redaction.RedactValue on the whole payload) or (b) for text
output, print/redact result.Diagnostics to stderr (or return an error via
writeAppError) so users see loader warnings; reference plugins.Load,
result.Diagnostics, toolboxItems, writePrettyJSON, writeAppError and
formatToolboxList to locate where to add the diagnostics handling.
- Around line 148-168: parseNameCommandArgs returns a JSON flag that the remove
handlers currently ignore; update runSkillRemove (and the analogous
plugin/remove handlers referenced) to capture the returned jsonFlag (the second
return value from parseNameCommandArgs) and handle it: either reject it by
returning a clear app/usage error when jsonFlag is true (e.g.,
writeAppError(stderr, "flag --json is not supported for remove commands",
exitUsage)) or implement a JSON response path that emits a machine-readable
success/failure object instead of the human text message; apply the same change
to the other remove handlers (the plugin remove function and the other remove
blocks mentioned) so --json is either honored or explicitly disallowed
consistently.
In `@internal/plugins/install.go`:
- Around line 120-145: The installer currently hashes only the manifest via
hashContent(data) but copies the full filtered plugin tree with
copyTree(pluginDir, target), causing mismatches when non-manifest files change;
update the install flow to compute the hash over the exact same filtered tree
that copyTree installs (i.e., walk pluginDir using the same include/exclude
rules used by copyTree and feed the concatenated/ordered contents into the
hasher instead of only hashing `data`), keep using LockEntry{Source: source,
Hash: hash} but set Hash to this tree-hash, and ensure writeLock/readLock logic
remains unchanged; also add a regression test that installs a plugin, modifies a
nested file (e.g., a tool script or prompt), reinstalls and asserts the lock
hash changed and Updated=true so nested-file changes are detected.
In `@internal/skills/install.go`:
- Around line 87-129: The installers compare the raw CLI source string which
makes local paths appear different (relative vs absolute); canonicalize local
filesystem sources before storing or comparing. Detect when options.Source
refers to a local path (not a remote git URL), resolve it to an absolute,
symlink-evaluated canonical path (e.g., via filepath.Abs +
filepath.EvalSymlinks) and use that canonicalSource in place of options.Source
for fetchSource, for the hash/manifest handling, and when writing/reading lock
entries (the values compared with previous.Source). Apply the same change in
internal/skills/install.go (refs: source, fetchDir, fetchSource,
previous.Source, ReadLock) and internal/plugins/install.go so that clash
detection always uses the normalized local path.
- Around line 132-149: Current install flow in internal/skills/install.go writes
directly into target (using target, skillFileName) after RemoveAll, then updates
lock (lock, LockEntry, writeLock), risking loss if process crashes; change to a
staged-swap: create a temp sibling directory (e.g., target+".tmp" or use
os.MkdirTemp under dir), write the SKILL.md and any files into that temp dir,
fsync files and directory, update the in-memory lock entry
(lock[name]=LockEntry{...}) and write the new lockfile to a temp file as well,
fsync it, then atomically rename the temp dir to target and rename the temp lock
to the real lock file so on-disk content and lockfile swap together; ensure any
cleanup of old temp dirs on failure and use appropriate perms and error handling
around fsync/rename operations.
---
Nitpick comments:
In `@internal/tools/scaffold_test.go`:
- Around line 14-147: Add a table-driven test that calls tools.Scaffold with
each runtime variant (RuntimeShell, RuntimeNode, RuntimePython) to exercise the
separate branches in runtimeSpecFor; for each runtime, assert Scaffold returns
no error, that result.ManifestPath and result.EntryPath exist and live under the
plugin dir, that plugins.ParseManifest succeeds for the generated manifest, and
that the tool command/args reference the entry (reuse commandReferencesEntry).
Keep assertions the same as in TestScaffoldCreatesManifestAndStub but loop over
runtimes (or create a new Test like TestScaffoldCoversAllRuntimes) and set
ScaffoldOptions.Runtime accordingly to catch template/manifest drift across the
Node and Python branches.
🪄 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 Plus
Run ID: 1c041b89-80d4-4a55-97ca-9b6f366180a3
📒 Files selected for processing (11)
internal/cli/app.gointernal/cli/distribution.gointernal/cli/distribution_test.gointernal/cli/extensions.gointernal/cli/skills.gointernal/plugins/install.gointernal/plugins/install_test.gointernal/skills/install.gointernal/skills/install_test.gointernal/tools/scaffold.gointernal/tools/scaffold_test.go
The test used a whitespace-only source dir name to force the no-usable-name rejection, which is not creatable on Windows (Smoke failure). Switched to a dir base that validSkillName rejects on every platform (contains "..") while staying creatable on Windows; the blank frontmatter still falls back to the unusable dir base, exercising the same rejection.
Collaborator
Author
|
Addressed the review (verified each against current code; gate green: gofmt clean, Fixed:
Skipped:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/distribution.go (1)
449-451: 💤 Low valueSimplify loop with variadic append.
Per staticcheck S1011, the loop can be replaced with a single variadic append.
- for _, line := range formatToolboxDiagnostics(diagnostics) { - lines = append(lines, line) - } + lines = append(lines, formatToolboxDiagnostics(diagnostics)...)🤖 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/distribution.go` around lines 449 - 451, Replace the explicit loop that appends each element from formatToolboxDiagnostics(diagnostics) into lines with a variadic append to simplify the code: use lines = append(lines, formatToolboxDiagnostics(diagnostics)...), referencing the existing identifiers lines, formatToolboxDiagnostics and diagnostics to locate and update the code.Source: Linters/SAST tools
internal/plugins/install.go (1)
507-509: 💤 Low valueComment describes "length-prefixed" but implementation uses null-delimited.
The header is null-delimited (
path\x00exec\x00), not length-prefixed. The approach is still collision-resistant since filesystem paths cannot contain null bytes, but the comment is misleading.- // Length-prefixed header keeps file boundaries unambiguous so two trees - // cannot collide by shifting bytes across the path/content split. + // Null-delimited header keeps file boundaries unambiguous (paths cannot + // contain null bytes) so two trees cannot collide by shifting bytes.🤖 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/plugins/install.go` around lines 507 - 509, The comment wrongly calls the header "length-prefixed" while the code constructs a null-delimited header via header := fmt.Sprintf("%s\x00%d\x00", filepath.ToSlash(rel), executable); update the comment to accurately describe the format (null-delimited path and executable marker) and why it remains collision-resistant (filesystem paths cannot contain NUL), or alternatively change the construction to a true length-prefixed format if that was the original intent; reference the header variable, filepath.ToSlash(rel), executable, and the fmt.Sprintf call when making the change.
🤖 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.
Nitpick comments:
In `@internal/cli/distribution.go`:
- Around line 449-451: Replace the explicit loop that appends each element from
formatToolboxDiagnostics(diagnostics) into lines with a variadic append to
simplify the code: use lines = append(lines,
formatToolboxDiagnostics(diagnostics)...), referencing the existing identifiers
lines, formatToolboxDiagnostics and diagnostics to locate and update the code.
In `@internal/plugins/install.go`:
- Around line 507-509: The comment wrongly calls the header "length-prefixed"
while the code constructs a null-delimited header via header :=
fmt.Sprintf("%s\x00%d\x00", filepath.ToSlash(rel), executable); update the
comment to accurately describe the format (null-delimited path and executable
marker) and why it remains collision-resistant (filesystem paths cannot contain
NUL), or alternatively change the construction to a true length-prefixed format
if that was the original intent; reference the header variable,
filepath.ToSlash(rel), executable, and the fmt.Sprintf call when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e22b7b76-9eb5-4307-b811-4f0f89f517da
📒 Files selected for processing (7)
internal/cli/distribution.gointernal/cli/distribution_test.gointernal/plugins/install.gointernal/plugins/install_test.gointernal/skills/install.gointernal/skills/install_test.gointernal/tools/scaffold_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/cli/distribution_test.go
- internal/skills/install.go
- internal/tools/scaffold_test.go
- internal/plugins/install_test.go
- internal/skills/install_test.go
Two Windows Smoke failures from the canonical-source review fix: - TestInstallSameLocalSourceDifferentSpellingIsNotAClash (skills + plugins) used filepath.Rel(cwd, tempdir), which errors on Windows when the temp dir (C:) and repo (D:) are on different drives. Switched to a same-directory alternate spelling (redundant /./), which still exercises canonicalSource without a cross-drive relative path. - TestRunSkillAddInfoRemove asserted the raw source path, but canonicalSource resolves symlinks/8.3-short-paths, so the stored+displayed source differs on Windows. Normalize the source via EvalSymlinks up front so the assertion matches on every platform.
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Collaborator
Author
|
@CodeRabbit approvable ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check current review state and open comments on PR `#184`
gh pr view 184 --repo Gitlawb/zero --json state,reviewDecision,reviews,comments | jq '{state,reviewDecision,reviews: (.reviews // [] | map({author:.author.login, state:.state, submittedAt:.submittedAt})), commentCount: (.comments // [] | length)}'
echo "---OPEN REVIEW THREADS---"
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 184) {
reviewThreads(first: 50) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
path
line
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == false) | {path: .comments.nodes[0].path, line: .comments.nodes[0].line, author: .comments.nodes[0].author.login, body: .comments.nodes[0].body[:200]}'Length of output: 1721
Open thread: The only unresolved thread is the staged-swap reinstall concern on
I'm withdrawing that concern and approving. [approve] |
Vasanthdev2004
approved these changes
Jun 13, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Approved
Reviewed the skill/plugin distribution and tool scaffold changes. No blockers found.
What I checked:
- Plugin and skill installs validate manifests/frontmatter before writing install state.
- Local sources are canonicalized for clash detection; git/local installs never execute fetched content.
- Plugin hashing covers installed tree content and skips
.git/symlinks; skill hashing covers the installedSKILL.mdcontent. - Remove paths validate IDs/names before deleting install targets.
- Scaffolded tools default to prompt-gated permissions and generate data only.
Note: the non-staged install replacement tradeoff is acceptable for this PR after the latest discussion/fixes; it does not introduce execution of fetched content, and CI/CodeRabbit are green.
Validation run locally on the PR head:
git diff --check origin/main...HEADgo test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smoke
Collaborator
Author
|
@CodeRabbit review..if approvable then approve it |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr view 184 --repo Gitlawb/zero --json state,reviewDecision,reviews | \
jq '{state, reviewDecision, reviews: (.reviews // [] | map({author:.author.login, state:.state, submittedAt:.submittedAt}))}'
echo "---OPEN UNRESOLVED THREADS---"
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 184) {
reviewThreads(first: 50) {
nodes {
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
body
path
line
}
}
}
}
}
}
}' | jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == false)] | length as $count | {openThreadCount: $count, threads: map({path: .comments.nodes[0].path, line: .comments.nodes[0].line, author: .comments.nodes[0].author.login, body: .comments.nodes[0].body[:200]})}'Length of output: 1856 🏁 Script executed: #!/bin/bash
# Get the thread node ID so we can resolve it
gh api graphql -f query='
{
repository(owner: "Gitlawb", name: "zero") {
pullRequest(number: 184) {
reviewThreads(first: 50) {
nodes {
id
isResolved
isOutdated
comments(first: 1) {
nodes {
author { login }
path
line
}
}
}
}
}
}
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false and .isOutdated == false and .comments.nodes[0].author.login == "coderabbitai") | .id'Length of output: 687 🏁 Script executed: #!/bin/bash
gh api graphql -f query='
mutation {
resolveReviewThread(input: {threadId: "PRRT_kwDOSqaaUs6JMOPb"}) {
thread {
id
isResolved
}
}
}'Length of output: 305
(ᵔᴥᵔ) [approve] |
Collaborator
Author
|
@CodeRabbit approve |
✅ Action performedComments resolved and changes approved. |
This was referenced Jul 8, 2026
This was referenced Jul 15, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stage 10 — Distribution
ZERO had skills/plugins but no way to install them (skills were discovery-only from a local dir) and no tool scaffolder. This adds a file-backed, local distribution surface so extensibility compounds into an ecosystem. There is no central registry: a "source" is just a git URL or a local path, and every install is checksum-pinned in a lockfile.
What was built
internal/skills/install.go—Install,Remove,Info, lockfile (skills.lock).SKILL.md(root or a single subdir), and validates its frontmatter through the existing skills parser. Invalid/nameless skills are rejected and nothing is written.SKILL.mdintoskills.DefaultDir(env)/<name>/, records asha256content hash mapped to its source inskills.lock. Reinstalling surfaces the old→new hash change.--force; reinstalling from the same source is idempotent.internal/plugins/install.go—Install,Remove, lockfile (plugins.lock).ParseManifest). The whole plugin tree is copied (entry scripts, prompts, skills) so the plugin is runnable through activation —.gitand symlinks are skipped so clone metadata and link-escapes never land in the plugins dir.internal/tools/scaffold.go—Scaffold.plugin.json(schemaVersion 1, one tool, empty parameter schema, command pointing at the entry stub) plus a runnable entry stub carrying a clear TODO. Default runtime is a POSIX shell stub (no interpreter dependency);node/pythonare also available. After plugin activation the scaffolded tool is loadable like any plugin.CLI wiring
zero skill {add,info,remove}— extendedinternal/cli/skills.go.zero plugin {add,remove}— extendedinternal/cli/extensions.go.zero tools {make,list}— new command registered ininternal/cli/app.go(dispatch + help), handlers in the newinternal/cli/distribution.go.appDepsresolverspluginsDir/toolsDir(the user plugins root) added inapp.go.Safety properties
install.shin a fetched source does not run and is not copied into the skills dir.GIT_TERMINAL_PROMPT=0so a missing-credential clone fails fast instead of blocking.Files
internal/skills/install.go+internal/skills/install_test.gointernal/plugins/install.go+internal/plugins/install_test.gointernal/tools/scaffold.go+internal/tools/scaffold_test.gointernal/cli/distribution.go+internal/cli/distribution_test.gointernal/cli/app.go,internal/cli/extensions.go,internal/cli/skills.go(subcommand wiring + help)How it was tested
TDD (RED→GREEN). Per-package unit tests cover the spec's list: local install + hash recording, invalid-source rejection, reinstall hash change + lockfile update, name-clash guard (+
--force), manifest validation, scaffold manifest parses against the real schema, list/info/remove reflect installed state, and "install never executes fetched content". Added real-git integration tests (a localfile://repo exercising the default runner end to end, asserting.gitdoes not leak) and a provider-neutrality assertion on the scaffolded manifest. End-to-end smoke through the built binary confirms the full add/list/info/update/remove and make/list lifecycle.Gate:
gofmt -l .clean,go build ./...succeeds,go test ./...passes.Deferred / out of scope
Summary by CodeRabbit
New Features
Behavior
Tests