feat(plugins): add CLI enable/disable for plugin.json - #693
Conversation
Add zero plugins enable|disable so installed plugins can be toggled without removing them, matching hooks/MCP parity and EXTENDING docs.
WalkthroughAdds ChangesPlugin enablement controls
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant zero plugins
participant plugins.SetEnabledByID
participant plugin.json
User->>zero plugins: enable or disable plugin id
zero plugins->>plugins.SetEnabledByID: resolve plugin and requested state
plugins.SetEnabledByID->>plugin.json: update enabled field
plugin.json-->>zero plugins: return toggle result
zero plugins-->>User: display text or redacted JSON status
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: 1
🧹 Nitpick comments (1)
internal/cli/extensions_test.go (1)
338-395: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid happy-path/usage-error coverage; consider adding a
--userand no-op-message case.Coverage for
--usertargeting and the "was already %s" no-op branch currently only exists at thepluginspackage level (enabled_test.go), not at the CLI layer. A quick additional case exercisingzero plugins disable <id> --useragainst dual user/project roots, and re-runningdisableon an already-disabled plugin to check the "was already disabled" message, would close that gap.🤖 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/extensions_test.go` around lines 338 - 395, The TestRunPluginsEnableDisable CLI test only covers project-scoped changes and the initial disable message. Extend it with separate user and project plugin roots, invoke the disable command with --user to verify the user manifest is updated without changing the project manifest, then repeat disable on the already-disabled plugin and assert the “was already disabled” no-op message.
🤖 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/plugins/enabled.go`:
- Around line 89-110: Introduce or reuse a sentinel error for the missing-plugin
case in SetEnabledByID and wrap it with the plugin ID context. In
internal/plugins/enabled.go:89-110, update the !ok return; in
internal/cli/extensions.go:122-128, replace strings.Contains(err.Error(), "is
not installed") with errors.Is against that sentinel, adding the required
import. Preserve distinct handling for the empty-ID validation error.
---
Nitpick comments:
In `@internal/cli/extensions_test.go`:
- Around line 338-395: The TestRunPluginsEnableDisable CLI test only covers
project-scoped changes and the initial disable message. Extend it with separate
user and project plugin roots, invoke the disable command with --user to verify
the user manifest is updated without changing the project manifest, then repeat
disable on the already-disabled plugin and assert the “was already disabled”
no-op message.
🪄 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: d03e2bd7-3ab1-4c50-914f-11de8cd3057f
📒 Files selected for processing (5)
docs/EXTENDING.mdinternal/cli/extensions.gointernal/cli/extensions_test.gointernal/plugins/enabled.gointernal/plugins/enabled_test.go
| // SetEnabledByID finds a plugin by id (respecting LoadOptions) and toggles its | ||
| // manifest enabled field. | ||
| func SetEnabledByID(options LoadOptions, id string, enabled bool) (SetEnabledResult, error) { | ||
| plugin, ok, err := FindByID(options, id) | ||
| if err != nil { | ||
| return SetEnabledResult{}, err | ||
| } | ||
| if !ok { | ||
| return SetEnabledResult{}, fmt.Errorf("plugin %q is not installed", id) | ||
| } | ||
| changed, err := SetEnabled(plugin.ManifestPath, enabled) | ||
| if err != nil { | ||
| return SetEnabledResult{}, err | ||
| } | ||
| return SetEnabledResult{ | ||
| ID: plugin.ID, | ||
| Enabled: enabled, | ||
| Changed: changed, | ||
| Source: plugin.Source, | ||
| ManifestPath: plugin.ManifestPath, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant files and nearby code paths.
sed -n '1,220p' internal/plugins/enabled.go
printf '\n--- extensions.go ---\n'
sed -n '1,220p' internal/cli/extensions.goRepository: Gitlawb/zero
Length of output: 9719
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the error strings and any sentinel/type already used elsewhere.
rg -n '"is not installed"|plugin id is required|ErrPlugin|errors.Is|Contains\(err.Error' internal -SRepository: Gitlawb/zero
Length of output: 47876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all callers and tests around plugin enable/disable so we can judge reachability.
rg -n 'SetEnabledByID\(|runPluginsToggle\(|plugins enable|plugins disable|plugin id is required|is not installed' internal -SRepository: Gitlawb/zero
Length of output: 1703
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the toggle parser and any tests that cover empty or missing IDs.
sed -n '100,170p' internal/cli/extensions.go
printf '\n--- cli tests ---\n'
rg -n 'plugins (enable|disable)|is not installed|plugin id is required|usage: zero plugins (enable|disable)' internal/cli internal/plugins -SRepository: Gitlawb/zero
Length of output: 2710
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the plugin enable tests and the install code for matching error-handling patterns.
sed -n '1,220p' internal/plugins/enabled_test.go
printf '\n--- install.go excerpt ---\n'
sed -n '160,230p' internal/plugins/install.goRepository: Gitlawb/zero
Length of output: 5170
Use a typed error for the missing-plugin path. SetEnabledByID should wrap plugin %q is not installed with a sentinel, and internal/cli/extensions.go should switch from strings.Contains(err.Error(), "is not installed") to errors.Is(...); otherwise the empty-id path (plugin id is required) still falls into the crash branch.
📍 Affects 2 files
internal/plugins/enabled.go#L89-L110(this comment)internal/cli/extensions.go#L122-L128
🤖 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/enabled.go` around lines 89 - 110, Introduce or reuse a
sentinel error for the missing-plugin case in SetEnabledByID and wrap it with
the plugin ID context. In internal/plugins/enabled.go:89-110, update the !ok
return; in internal/cli/extensions.go:122-128, replace
strings.Contains(err.Error(), "is not installed") with errors.Is against that
sentinel, adding the required import. Preserve distinct handling for the
empty-ID validation error.
Replace string matching with errors.Is for missing plugins, and extend CLI tests for --user targeting and the already-disabled no-op message.
|
@coderabbitai review the latest commits and changes |
|
Tip For best results, initiate chat on the files or code changes.
Let me know and I'll jump right in. (ノ・ヮ・)ノ |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
fd . -e go internal/plugins internal/cli | xargs echo
echo "---enabled.go---"
cat -n internal/plugins/enabled.go
echo "---extensions.go (relevant portion)---"
cat -n internal/cli/extensions.go | sed -n '1,400p'Length of output: 23976 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "---activate.go (look for similar manifest write patterns)---"
rg -n "WriteFile|os.Rename|Chmod|optionalBool" internal/plugins/*.go
echo "---optionalBool def---"
rg -n "func optionalBool" -A 20 internal/plugins/*.goLength of output: 3794
One functional concern worth fixing before merge: File permission regression in if err := os.WriteFile(tempPath, append(data, '\n'), 0o600); err != nil {This hardcodes Suggested fix: preserve the original file's mode instead of hardcoding it, e.g.: mode := os.FileMode(0o644)
if info, statErr := os.Stat(resolved); statErr == nil {
mode = info.Mode().Perm()
}
if err := os.WriteFile(tempPath, append(data, '\n'), mode); err != nil {
return false, err
}Everything else — the ✅ Action performedReview finished.
|
Keep the original manifest permissions when rewriting enabled, falling back to 0o644 to match install and avoid silently tightening to 0o600.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Nice feature — the atomic manifest rewrite, idempotency, and project/user precedence all look right, and the new CLI tests pass on my end. One blocker: TestSetEnabledPreservesManifestMode asserts a 0644 mode after os.Chmod, but on Windows os.Stat reports 0666 regardless of chmod, so it fails on windows-latest (which is in the CI matrix and runs go test ./...). Guard that test with a runtime.GOOS != "windows" skip (or drop the exact-bits assertion) and I think this is good to go.
Windows reports 0666 from os.Stat regardless of chmod, so skip the exact permission assertion on windows-latest CI.
|
Thanks for the review, @Vasanthdev2004! Addressed the Windows CI blocker in dae3ef8: The mode-preservation behavior is still covered on Unix via the existing assertion. Please take another look when you have a moment. |
Resolve EXTENDING.md docs conflict to keep both info and enable/disable examples. Deduplicate ErrNotInstalled so enable/disable and info share one sentinel after Gitlawb#773.
|
@Vasanthdev2004 Merged latest
Earlier review items from this thread:
Also opened #790 and linked it via |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/extensions.go (1)
143-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact the text output before printing the manifest path.
result.ManifestPathis emitted directly here, while the existing list and JSON paths go throughredaction. In CI logs this can expose local usernames or home-directory paths. Redact the formatted message before writing it, and add a text-mode test covering the path.Proposed fix
+ message := "" if result.Changed { - if _, err := fmt.Fprintf(stdout, "Plugin %s is now %s in %s.\n", result.ID, state, result.ManifestPath); err != nil { - return exitCrash - } - } else if _, err := fmt.Fprintf(stdout, "Plugin %s was already %s in %s.\n", result.ID, state, result.ManifestPath); err != nil { + message = fmt.Sprintf("Plugin %s is now %s in %s.", result.ID, state, result.ManifestPath) + } else { + message = fmt.Sprintf("Plugin %s was already %s in %s.", result.ID, state, result.ManifestPath) + } + if _, err := fmt.Fprintln(stdout, redaction.RedactString(message, redaction.Options{})); err != nil { return exitCrash }🤖 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/extensions.go` around lines 143 - 147, Update the text output branches in the extension command around result.Changed to redact the fully formatted message, including result.ManifestPath, before writing it to stdout. Reuse the existing redaction mechanism used by the list and JSON paths, and add a text-mode test verifying sensitive path components are removed from both changed and already-state output.
🤖 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.
Outside diff comments:
In `@internal/cli/extensions.go`:
- Around line 143-147: Update the text output branches in the extension command
around result.Changed to redact the fully formatted message, including
result.ManifestPath, before writing it to stdout. Reuse the existing redaction
mechanism used by the list and JSON paths, and add a text-mode test verifying
sensitive path components are removed from both changed and already-state
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89d5d6f3-dd8b-4c5e-aff8-5dd55af24541
📒 Files selected for processing (4)
docs/EXTENDING.mdinternal/cli/extensions.gointernal/plugins/enabled.gointernal/plugins/info.go
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/EXTENDING.md
- internal/plugins/enabled.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. The key behaviour is right: disabling a plugin actually stops its components loading rather than just hiding it from a listing, which was the thing worth verifying. The state file is written atomically and a missing or corrupt one degrades safely, and the Windows test-mode fix that was the last blocker is in and that leg is green.
A few minor notes for later rather than now: concurrent toggles from two processes are not serialized (last writer wins, which is fine for a CLI toggle but worth knowing), and disabling a plugin mid-session takes effect on the next activation rather than immediately. Neither is a correctness problem for how this is used.
Verification note: the go toolchain is blocked on my machine by Smart App Control, so this is a source review; CI is green.
Thanks for the quick turnaround on the Windows leg. LGTM.
|
cc: @kevincodex1 |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested. The command surface is well built and the feature is wanted, but the manifest write in internal/plugins/enabled.go re-implements — less safely — machinery this same package already owns and already uses.
Reviewed at e29fddc, base d9b882e (origin/main at 097c265). Parent issue #790 carries issue-approved.
What I verified
-
The tests are load-bearing. I stubbed out the publish step so
SetEnabledreturnschanged=truewithout ever writing the manifest, and two tests caught it with the un-toggled file printed in the failure:--- FAIL: TestRunPluginsEnableDisable extensions_test.go:376: manifest not disabled: { "schemaVersion": 1, "id": "zero.demo", ... } --- FAIL: TestRunPluginsDisableUserIgnoresProject extensions_test.go:447: user manifest not disabled: { ..., "enabled": true } -
go vet ./internal/plugins/ ./internal/cli/clean;go test ./internal/plugins/ -count=1passes. -
Merged
origin/maininto the branch: auto-merge clean,go build ./...succeeds on the merged tree. -
Confirmed the orphan-temp-file concern does not apply: discovery joins the exact filename at
internal/plugins/plugins.go:253(filepath.Join(pluginDir, "plugin.json")), so a strandedplugin.json.tmp-*is never mistaken for a manifest. -
Ran on darwin/arm64 from a non-
/tmpcheckout.
Blocking
-
SetEnabledtakes no lock, while every other mutator in this package does. —internal/plugins/enabled.go:41Installacquiresinstalltxn.Lock(dir)atinternal/plugins/install.go:151, andRemoveacquires it at:203. Both then mutate under it.SetEnabledperforms a read-modify-write on a file inside that same tree with no lock at all, andinstalltxn.WriteFileAtomicallyexplicitly documents that responsibility as the caller's:// WriteFileAtomically publishes data by renaming a complete sibling temporary // file over path. The caller is responsible for any surrounding transaction // lock.
installtxn.Lockis a real cross-process lock —flockon Unix (lock_unix.go),LockFileExon Windows (lock_windows.go) — which matters here because this repo runs CLI, TUI and daemon against the same state. The concrete failure:zero plugins disable Xreads the manifest,zero plugins remove Xthen takes the lock and tears the directory down, and the first process's rename recreatesplugin.jsoninside the removed plugin's directory. The result is a manifest that discovery will load with no corresponding entry inplugins.lock— precisely the inconsistency the lock exists to prevent. -
The atomic write is hand-rolled and is weaker than the helper this package already calls. —
internal/plugins/enabled.go:78-88internal/plugins/install.go:271already callsinstalltxn.WriteFileAtomically. The new code writes its own temp-file-plus-os.Renameinstead, and differs in two ways that matter:- No
Sync.installtxn.WriteFileAtomicallycallstemp.Sync()before closing. Without it, a crash between the write and the rename can publish a truncated or zero-lengthplugin.json. That file then failsjson.Unmarshal, and the plugin drops out of discovery entirely — a worse outcome than either enabled or disabled. os.Renameinstead ofreplaceFile. On Windows the package usesMoveFileEx(..., MOVEFILE_REPLACE_EXISTING|MOVEFILE_WRITE_THROUGH)(internal/installtxn/lock_windows.go:29). Go'sos.Renamedoes not passMOVEFILE_WRITE_THROUGH, so the durability guarantee the package deliberately chose is lost on the platform it was chosen for. The existence of a platform-specificreplaceFileis the signal that plainos.Renamewas already judged insufficient here.
Both resolve together: take
installtxn.Lockon the plugins root, then publish withinstalltxn.WriteFileAtomically(resolved, append(data, '\n'), mode). That also deletes theos.Getpid()/UnixNano()temp-name construction, which is only there because the helper was not used. - No
-
Toggling rewrites the manifest with its fields reordered. —
internal/plugins/enabled.go:71json.MarshalIndentover amap[string]anyemits keys in sorted order, so a singlezero plugins disablescrambles a hand-authored file. Measured against the patched build:BEFORE AFTER { { "name": "demo", "author": "a", "version": "1.0.0", "description": "z last", "description": "z last", "enabled": false, "author": "a", "name": "demo", "enabled": true "version": "1.0.0" } }plugin.jsonis a file users edit by hand and commit. Reordering it on every toggle produces diff noise unrelated to the change, and for a project plugin under./.zero/pluginsthat noise lands in the repository. Preserving order needs either an ordered decode (json.DecoderwithToken()) or a targeted textual edit of the singleenabledfield rather than a full re-marshal. If that is judged too costly for the value, say so in the PR description and indocs/EXTENDING.mdso the churn is at least documented rather than surprising — but it should be a stated decision, not a side effect.
Non-blocking
- Permission fallback widens on a stat failure. —
internal/plugins/enabled.go:75-78. Whenos.Statreturns an error the mode silently falls back to0o644. A manifest that was0o600would be republished world-readable.os.ReadFilehas already succeeded by that point so the window is narrow, but the repo's convention on security-adjacent state is to fail closed rather than pick a permissive default — returning the stat error would be more consistent. Usinginstalltxn.WriteFileAtomicallydoes not fix this by itself; the mode decision stays yours. - Symlinked manifests are replaced rather than followed. —
internal/plugins/enabled.go:47.filepath.Absdoes not resolve symlinks, so ifplugin.jsonis a symlink the rename replaces the link with a regular file. Probably out of scope, butfilepath.EvalSymlinksis a one-line guard if you want it. - Default target when a plugin exists in both scopes. —
internal/cli/extensions.go:127. Without--user,LoadOptions{ExcludeProject: false}means project precedence wins, sozero plugins disable Xwrites the project manifest when both exist. The command does printresult.ManifestPathafterwards, which mitigates it, butwritePluginsToggleHelpdescribes only what--userdoes and not what the default does. One extra line there would remove the ambiguity. - No test asserts unrelated manifest fields survive. —
internal/plugins/enabled_test.go. The four tests cover toggling, idempotence, the--userfilter, the missing-plugin error and file mode. None reads the manifest back and checks thatname,version,commandsand the permission block are byte-identical apart fromenabled. Given that the implementation round-trips the whole document through a map, that is the assertion most worth having — and it is the one that would have caught finding 3.
Not covered by this PR
- Nothing invalidates a running daemon's or TUI's in-memory plugin set after a toggle.
zero plugins disable Xupdates the manifest; whether an already-running session notices is untested and probably a separate issue rather than this PR's job — but worth confirming the expectation before merge.
Not verified
- Windows.
TestSetEnabledPreservesManifestModeis skipped there (noted in the PR description, which is the right call), but the rename path is the part that actually diverges and it has no coverage on that platform either way. I did not cross-compile or run the Windows path. - I did not exercise
zero plugins enable/disableend-to-end against a real installed plugin; the verification above is through the package and CLI tests.
The CLI layer itself I have no complaints about: flag parsing, the --json path through redaction.RedactValue, the ErrNotInstalled mapping to a usage error rather than a crash, and the already-enabled/now-enabled distinction are all handled properly. Rework the write in enabled.go around installtxn and this becomes an approve.
3 participants
Summary
zero plugins enable|disable <id>to toggle theenabledfield inplugin.json(with--user/--json)plugins.SetEnabled/SetEnabledByIDhelpers used by the CLIdocs/EXTENDING.mdso the documented gap matches the new commandsCloses #790
Test plan
go test ./internal/plugins/ ./internal/cli/ -run 'Enabled|EnableDisable|PluginInfo|Info|SetEnabled' -count=1go vet ./internal/plugins/ ./internal/cli/govulncheck ./internal/plugins/— no vulnerabilitiesTestSetEnabledPreservesManifestModeskipped onwindows(dae3ef8)main(includes#773plugins info) and deduplicatedErrNotInstalledSummary by CodeRabbit
New Features
zero plugins enable <id>andzero plugins disable <id>commands (without removing the plugin).--userto target only user-installed plugin manifests, ignoring project plugins.--jsonoutput and clearer messaging for “already enabled/disabled” vs “now enabled/disabled”.Bug Fixes
plugin.jsonpermission bits when toggling enablement.Documentation
plugin.json.enabled(defaults to enabled when omitted).