wedge stage 09: finish plugin wiring (tools/hooks/skills)#185
Merged
Conversation
Activate resolved plugin manifests so their declared tools, hooks, and
skills take effect at runtime instead of being parsed-but-unconsumed.
- internal/plugins/activate.go: Activate() registers each plugin tool into
the tools.Registry via a command-runner adapter (JSON args on stdin,
stdout/exit -> tools.Result, ${AGENT_PLUGIN_ROOT} expansion + env),
builds hooks.Definition values for plugin hooks, and exposes plugin
skill search roots. Permission maps allow/prompt/deny -> tools.Safety;
allow only survives load-time clamping when manifest auto-approval is
enabled, so mutating plugin tools are not auto-approved by default. A
plugin-aware skill tool merges the default skills dir with plugin roots
(recording duplicate names, never crashing). Malformed plugins/extensions
are skipped with a warning; activation order is deterministic.
- internal/cli: activatePlugins() invokes activation in both bootstrap
paths (interactive TUI in app.go and one-shot exec in exec.go), after
core/specialist/MCP registration. newHookDispatcherWithExtra folds plugin
hooks into the dispatcher's hook set. Fails open: a bad plugin warns and
is skipped, never wedging startup.
- internal/plugins/plugins.go: update the stale 'not yet registered' note.
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a plugin activation layer and integrates it into CLI bootstrap: registers plugin tools, collects plugin hooks and skill roots, folds plugin hooks into the workspace hook dispatcher, re-registers a merged ChangesPlugin Activation and Integration
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant ACT as Activate
participant REG as tools.Registry
participant DISP as hooks.Dispatcher
participant PROC as Subprocess
CLI->>ACT: load workspace plugins
ACT->>REG: register plugin tools
ACT->>DISP: return hook definitions
REG->>PROC: run plugin tool (JSON stdin, env AGENT_PLUGIN_ROOT)
PROC->>REG: stdout, stderr, exit code
REG->>CLI: mapped tools.Result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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: 4
🤖 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/plugin_activate.go`:
- Around line 30-39: The loadPlugins call can return per-plugin diagnostics that
are currently ignored; after calling deps.loadPlugins(plugins.LoadOptions{Cwd:
workspaceRoot}) and before invoking plugins.Activate, iterate over the
diagnostics on the returned loaded (e.g., loaded.Diagnostics or whichever field
holds per-plugin diagnostics in the LoadResult) and call
writePluginActivationWarning(stderr, diag) for each diagnostic string; keep the
existing top-level error handling intact and then proceed to call
plugins.Activate(registry, loaded.Plugins, plugins.ActivateOptions{Cwd:
workspaceRoot}) as before so activation continues but all load diagnostics are
surfaced.
In `@internal/plugins/activate_test.go`:
- Around line 234-275: Add a regression case to
TestActivateExposesSkillDirInLoaderRoots that covers manifest-relative skill
paths: create a LoadedPlugin whose Skills contains a PathExtension with a
relative Path like "skills/ts-review/SKILL.md" (and a variant using
"${AGENT_PLUGIN_ROOT}/skills/ts-review/SKILL.md"), call Activate(registry,
[]LoadedPlugin{plugin}, ActivateOptions{}) and assert that result.SkillRoots
contains the full pluginDir-based root (filepath.Join(pluginDir,"skills"))
rather than the bare "skills"; reference LoadedPlugin, PathExtension, Activate,
ActivateOptions and TestActivateExposesSkillDirInLoaderRoots when adding the new
assertions.
In `@internal/plugins/activate.go`:
- Around line 221-229: The code passes ext.Path verbatim into skillSearchRoot
causing relative manifest paths like "skills/foo/SKILL.md" to yield incorrect
roots; resolve ext.Path against the plugin root first (e.g. compute resolved :=
filepath.Join(plugin.Root, ext.Path) or use filepath.Clean/Abs as appropriate)
and then call skillSearchRoot(resolved) so SkillRoots contains the real
filesystem directory (update references to ext.Path -> resolved when computing
root and keep the seenRoot/SkillRoots logic unchanged). Ensure you
import/path-utilize filepath and preserve deduplication via seenRoot.
- Around line 184-186: Before calling registry.Register(tool), check whether a
tool with the same name already exists and fail the activation instead of
overwriting: use the registry lookup/has function to detect an existing entry
for ext.Name and if found return an error from the activation routine (include
ext.Name and plugin.ID in the message); only call registry.Register(tool) and
append to result.Tools (ToolProvenance{ToolName: ext.Name, PluginID: plugin.ID})
when no collision is detected. Ensure you do not append to result.Tools on
collision.
🪄 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: 86dbf486-1586-43d7-be39-2c6e965bce54
📒 Files selected for processing (9)
internal/cli/app.gointernal/cli/exec.gointernal/cli/hook_dispatch.gointernal/cli/plugin_activate.gointernal/cli/plugin_activate_test.gointernal/plugins/activate.gointernal/plugins/activate_test.gointernal/plugins/activate_unix_test.gointernal/plugins/plugins.go
expandPluginRoot kept the manifest's forward slashes, so a ${AGENT_PLUGIN_ROOT}/bin/x command became a mixed-separator path on Windows (C:\...\001/bin/x), failing the path tests. Added expandPluginRootPath (filepath.FromSlash on the placeholder-expanded executable path only; args keep their slashes since they may be URLs/flags) and used it for the tool and hook command paths.
- surface per-plugin load diagnostics on stderr (not just top-level errors) - skip plugin tools whose name collides with an already-registered tool - resolve manifest-relative plugin skill paths against the plugin dir
Collaborator
Author
|
Addressed the CodeRabbit review (pushed 4321b09): Fixed
Note: the production load path ( Gate: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/plugin_activate.go`:
- Around line 63-68: formatLoadDiagnostic currently only appends
diagnostic.ManifestPath, leaving output lacking context when it's empty; update
formatLoadDiagnostic to fall back to diagnostic.PluginPath and, if that is also
empty, diagnostic.PluginID when ManifestPath is empty, so the message becomes
"[Kind] Message (path-or-id)". Use the existing symbols Diagnostic.ManifestPath,
Diagnostic.PluginPath, Diagnostic.PluginID, and the function
formatLoadDiagnostic to locate and implement the fallback chain.
🪄 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: 83b3715d-cf0b-45e1-875d-f7762b5d94f7
📒 Files selected for processing (4)
internal/cli/plugin_activate.gointernal/cli/plugin_activate_test.gointernal/plugins/activate.gointernal/plugins/activate_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/plugin_activate_test.go
- internal/plugins/activate_test.go
- internal/plugins/activate.go
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 plugin activation/runtime wiring. No blockers found.
What I checked:
- Plugin tools register through the normal tool registry and keep
tools.Safetypermission metadata; no raw execution bypass is introduced. - Tool name collisions are skipped with warnings instead of replacing core tools or earlier plugin tools.
- Plugin command output is redacted before display, commands are timeout-bounded, and
AGENT_PLUGIN_ROOTis passed explicitly. - Plugin hooks are namespaced, appended deterministically, and still respect hooks being disabled overall.
- Plugin skills merge with default skills while default/user skills win name collisions.
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
This was referenced Jul 4, 2026
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.
Summary
Finishes the previously-stubbed
internal/pluginsso loaded plugin manifests actually take effect at runtime. Before this change, plugin tools/hooks/skills were parsed for discovery only (thezero pluginslisting) and never registered — plugins loaded but did nothing. This stage turns each plugin's declared tools, hooks, and skills into live registrations.What was built
internal/plugins/activate.go(new) —Activate(registry, plugins, options):pluginTooladapter implements thetools.Toolinterface by invoking the plugin's declared command. Tool arguments are passed as JSON on stdin; stdout/exit map onto atools.Result;${AGENT_PLUGIN_ROOT}placeholders in the command/args are expanded to the plugin dir, which is also exported as theAGENT_PLUGIN_ROOTenv var. The runner is injectable for tests and defaults to a bounded subprocess. Each tool is registered viaregistry.Register.ToolPermission(allow/prompt/deny) maps ontotools.Safety(alwaysshellside-effect + the carried permission).allowonly survives manifest parsing whenAllowManifestToolAutoApprovalwas set, so mutating plugin tools are not auto-approved by default;denytools register but are never advertised or executed.internal/hooks.Definitionper hook extension (mapping the pluginHookEventto the hooksEvent, carrying command/args,${AGENT_PLUGIN_ROOT}-expanded), with plugin-namespaced ids.internal/skills.Loadscans) and provides a plugin-awareskilltool that merges the default skills dir with plugin roots, deduplicating by name and recording collisions (preservingskills.Duplicatesbehaviour, never crashing).Meta.internal/cli/plugin_activate.go(new) + bootstrap wiring —activatePlugins(...)loads the workspace's plugins and runs activation. It is invoked in both bootstrap paths:internal/cli/app.goinrunInteractiveTUIWithSetup, right after specialist + MCP tool registration (so plugin tools are part of the deferral count).internal/cli/exec.goinrunExec, after MCP registration and before--list-tools/filter validation (so plugin tools are listable and filter-validatable).newHookDispatcherWithExtra(ininternal/cli/hook_dispatch.go) folds the plugin hook definitions into the dispatcher's active hook set; the plugin-aware skill tool is registered when a plugin declares skills. Activation fails open: a load error or malformed plugin is surfaced as a[zero] WARNINGon stderr and skipped, never wedging startup.internal/plugins/plugins.go— comment-only update replacing the stale "not yet registered" note (no behavioural change).Where activation is invoked
internal/cli/app.go:activatePlugins(workspaceRoot, registry, deps, stderr)inrunInteractiveTUIWithSetup, andnewHookDispatcherWithExtra(workspaceRoot, pluginActivation.hooks)in the TUIagent.Options.internal/cli/exec.go:activatePlugins(...)inrunExec, andnewHookDispatcherWithExtra(...)in the execagent.Options.Testing
internal/plugins/activate_test.go: tool registration + command invocation + stdout→Result mapping (stubbed runner); non-zero exit → error; permission mapping (prompt requires grant, allow runs, deny never executes); hookDefinitionbuild + event mapping + dispatcherSelect; skill search-root derivation + loader discovery; merged skill list + duplicate recording; malformed-plugin skip-with-warning; disabled-plugin skip; deterministic ordering.internal/plugins/activate_unix_test.go: real-subprocess end-to-end (JSON args on stdin and$AGENT_PLUGIN_ROOTround-trip; missing binary → tool error).internal/cli/plugin_activate_test.go: bootstrap helper registers the plugin tool, collects namespaced hooks, re-registers the plugin-aware skill tool, fails open on load error, warns on malformed plugin;newHookDispatcherWithExtraruns a folded plugin hook.gofmt -lclean on changed dirs,go build ./...andgo test ./...both green.Deferred / out of scope
AllowManifestToolAutoApprovalstays off by default at the bootstrap, so pluginallowtools are clamped to prompt unless a caller opts in.tool_searchmechanism).Summary by CodeRabbit
New Features
Tests
Documentation