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.
|
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 (2)
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 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.
2 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 commandsTest plan
go test ./internal/plugins/ ./internal/cli/ -count=1go vet ./internal/plugins/ ./internal/cli/golangci-lint(unused/ineffassign/staticcheck) on./internal/plugins/— 0 issuesgovulncheck ./...— no vulnerabilitieszero plugins disable <id>, confirmlistshows disabled and tools are not activated; thenenableagainSummary by CodeRabbit
zero plugins enable <id>andzero plugins disable <id>commands.--user.enabledflag (without removing the plugin).plugin.jsonfile permissions when toggling enablement.