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.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Merge readiness
- [P1] Rebase onto current
mainand re-run the affected checks
docs/EXTENDING.md:290
The PR head is based ond9b882e, while livemainis now1b5db17(656 files and two releases ahead) and changes bothdocs/EXTENDING.mdandinternal/cli/extensions_test.go. The tree merge is mechanically clean, but the repository's current contribution guidance makes a stale base a hard blocker. Please rebase, retain the current-target changes, and revalidate the resolved diff.
Findings
-
[P1] Serialize and durably publish the manifest mutation
internal/plugins/enabled.go:43
SetEnabledByIDresolves and reads a manifest, thenSetEnabledpublishes it without theinstalltxn.Lockused byInstallandRemove. The operation therefore does not serialize its read-modify-write cycle with the other commands that own the same user-plugin directory andplugins.lock. For example: a toggle reads the old manifest; an install acquires the lock, swaps in a newly staged plugin and writes its new hash; then the toggle writes its already-read JSON to the now-replaced path. The new plugin's manifest is overwritten by the old one while the lock entry still describes the new tree. The hand-rolled temporary-file path also omits theSyncand Windows write-through behavior ininstalltxn.WriteFileAtomically, so an interruption can publish incomplete manifest bytes and make the plugin fail JSON discovery.Address the root cause by making enablement participate in the existing transaction boundary: derive the selected plugin's root, acquire its
installtxn.Lockbefore the final lookup/read, and publish viainstalltxn.WriteFileAtomicallywhile that lock is held. Keep the lock scope limited to the established plugin root and retain the current precedence behavior. Add a deterministic concurrency test that pauses an install between staging and commit, then proves a simultaneous toggle cannot overwrite the installed manifest or desynchronize its lock entry; also cover a writer failure/interruption path. -
[P1] Do not corrupt extension-defined integer values while toggling
internal/plugins/enabled.go:57
The loader deliberately accepts unknown manifest fields, but this command decodes the entire document intomap[string]any. Go's default JSON decoder represents each number there asfloat64; when the map is re-marshaled, integers outside IEEE-754's exact range are rounded. A valid extension value such as9007199254740993is consequently written back as9007199254740992when the user only asks to disable the plugin. This permanently changes configuration the command neither owns nor validates.The root cause is a lossy whole-document round trip. Preserve numeric tokens for every untouched field—at minimum decode with
json.Decoder.UseNumberbefore changingenabled, rather thanjson.Unmarshalintoany; a targeted representation-preserving edit is also acceptable if it preserves the manifest's existing schema behavior. Add a regression test containing an unknown nested field with9007199254740993, toggle both directions, and assert that the value remains exactly the same after each write.
4 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).