From 62d3d292781be30e4612b1bdc152f41c9e1ccbf8 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sat, 17 Oct 2026 17:12:16 +0530 Subject: [PATCH 1/2] feat(plugins): add CLI enable/disable for plugin.json Add zero plugins enable|disable so installed plugins can be toggled without removing them, matching hooks/MCP parity and EXTENDING docs. --- docs/EXTENDING.md | 4 +- internal/cli/extensions.go | 98 ++++++++++++++++++++ internal/cli/extensions_test.go | 121 ++++++++++++++++++++++++ internal/plugins/enabled.go | 114 +++++++++++++++++++++++ internal/plugins/enabled_test.go | 152 +++++++++++++++++++++++++++++++ 5 files changed, 488 insertions(+), 1 deletion(-) create mode 100644 internal/plugins/enabled.go create mode 100644 internal/plugins/enabled_test.go diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index 96d18b464..d01bdb446 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -290,10 +290,12 @@ Install and manage: zero plugins add ./github-pr-review # copy into ~/.config/zero/plugins/ or ./.zero/plugins/ zero plugins list zero plugins info github-pr-review # show manifest path, enabled state, lock metadata +zero plugins disable github-pr-review # sets "enabled": false in plugin.json +zero plugins enable github-pr-review # sets "enabled": true in plugin.json zero plugins remove github-pr-review # alias: rm ``` -A plugin is enabled by being present in the plugins directory and disabled by removing it (or by the user setting `"enabled": false` in its `plugin.json`). Plugins are not enabled or disabled by a CLI subcommand today. +A plugin is active when it is present under a plugins directory and its `plugin.json` has `"enabled": true` (the default when the field is omitted). Use `zero plugins disable ` / `zero plugins enable ` to toggle that field without removing the plugin. Pass `--user` to target only user plugins and ignore `./.zero/plugins`. Plugin commands run with the plugin directory as their working directory. Use relative paths; the loader resolves them at activation time. diff --git a/internal/cli/extensions.go b/internal/cli/extensions.go index c3bb81a4e..c50f55c6f 100644 --- a/internal/cli/extensions.go +++ b/internal/cli/extensions.go @@ -2,6 +2,7 @@ package cli import ( "context" + "errors" "fmt" "io" "strings" @@ -82,11 +83,94 @@ func runPlugins(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) return runPluginInfo(args[1:], stdout, stderr, deps) case "remove", "rm": return runPluginRemove(args[1:], deps.pluginsDir(), stdout, stderr) + case "enable": + return runPluginsToggle(args[1:], stdout, stderr, deps, false) + case "disable": + return runPluginsToggle(args[1:], stdout, stderr, deps, true) default: return writeExecUsageError(stderr, fmt.Sprintf("unknown plugins subcommand %q", args[0])) } } +type pluginToggleOptions struct { + json bool + user bool +} + +func runPluginsToggle(args []string, stdout io.Writer, stderr io.Writer, deps appDeps, disabled bool) int { + commandName := "enable" + if disabled { + commandName = "disable" + } + options, positional, help, err := parsePluginToggleArgs(args, commandName) + if err != nil { + return writeExecUsageError(stderr, err.Error()) + } + if help { + if err := writePluginsToggleHelp(stdout, commandName); err != nil { + return exitCrash + } + return exitSuccess + } + if len(positional) != 1 { + return writeExecUsageError(stderr, fmt.Sprintf("usage: zero plugins %s [--user] [--json]", commandName)) + } + pluginID := positional[0] + + cwd, err := deps.getwd() + if err != nil { + return writeAppError(stderr, "failed to resolve workspace: "+err.Error(), exitCrash) + } + loadOptions := plugins.LoadOptions{Cwd: cwd, ExcludeProject: options.user} + result, err := plugins.SetEnabledByID(loadOptions, pluginID, !disabled) + if err != nil { + if errors.Is(err, plugins.ErrNotInstalled) { + return writeExecUsageError(stderr, err.Error()) + } + return writeAppError(stderr, redaction.ErrorMessage(err, redaction.Options{}), exitCrash) + } + + state := "enabled" + if disabled { + state = "disabled" + } + if options.json { + if err := writePrettyJSON(stdout, redaction.RedactValue(result, redaction.Options{})); err != nil { + return exitCrash + } + return exitSuccess + } + 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 { + return exitCrash + } + return exitSuccess +} + +func parsePluginToggleArgs(args []string, command string) (pluginToggleOptions, []string, bool, error) { + options := pluginToggleOptions{} + positional := []string{} + for _, arg := range args { + switch arg { + case "-h", "--help", "help": + return options, positional, true, nil + case "--json": + options.json = true + case "--user": + options.user = true + default: + if strings.HasPrefix(arg, "-") { + return options, positional, false, execUsageError{fmt.Sprintf("unknown plugins %s flag %q", command, arg)} + } + positional = append(positional, arg) + } + } + return options, positional, false, nil +} + func runHooks(args []string, stdout io.Writer, stderr io.Writer, deps appDeps) int { if len(args) == 0 { return writeExecUsageError(stderr, "hooks subcommand required. Use `zero hooks list`.") @@ -566,10 +650,24 @@ Commands: info Show plugin details and lockfile metadata add Install a plugin (manifest-validated, pinned in plugins.lock) remove Remove an installed plugin and its lockfile entry + enable Enable a plugin via its plugin.json + disable Disable a plugin via its plugin.json `) return err } +func writePluginsToggleHelp(w io.Writer, command string) error { + _, err := fmt.Fprintf(w, `Usage: + zero plugins %s [flags] + +Flags: + --user Target only user plugins (ignore ./.zero/plugins) + --json Print command result as JSON + -h, --help Show this help +`, command) + return err +} + func writePluginsListHelp(w io.Writer) error { _, err := fmt.Fprint(w, `Usage: zero plugins list [flags] diff --git a/internal/cli/extensions_test.go b/internal/cli/extensions_test.go index 8b613fd3e..e6e531b93 100644 --- a/internal/cli/extensions_test.go +++ b/internal/cli/extensions_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "os" "path/filepath" "strings" "testing" @@ -333,3 +334,123 @@ func TestRunMCPPermissionsHelpDoesNotOpenStore(t *testing.T) { }) } } + +func TestRunPluginsEnableDisable(t *testing.T) { + cwd := t.TempDir() + pluginDir := filepath.Join(cwd, ".zero", "plugins", "demo") + if err := os.MkdirAll(pluginDir, 0o700); err != nil { + t.Fatal(err) + } + manifestPath := filepath.Join(pluginDir, "plugin.json") + if err := os.WriteFile(manifestPath, []byte(`{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0" +} +`), 0o600); err != nil { + t.Fatal(err) + } + + deps := appDeps{getwd: func() (string, error) { return cwd, nil }} + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"plugins", "disable", "zero.demo", "--json"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("disable exit=%d stderr=%s", exitCode, stderr.String()) + } + var disablePayload plugins.SetEnabledResult + if err := json.Unmarshal(stdout.Bytes(), &disablePayload); err != nil { + t.Fatalf("decode disable JSON: %v\n%s", err, stdout.String()) + } + if disablePayload.ID != "zero.demo" || disablePayload.Enabled || !disablePayload.Changed { + t.Fatalf("unexpected disable payload: %#v", disablePayload) + } + + raw, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), `"enabled": false`) { + t.Fatalf("manifest not disabled: %s", raw) + } + + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"plugins", "disable", "zero.demo"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("noop disable exit=%d stderr=%s", exitCode, stderr.String()) + } + if !strings.Contains(stdout.String(), "was already disabled") { + t.Fatalf("noop disable output: %s", stdout.String()) + } + + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"plugins", "enable", "zero.demo"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("enable exit=%d stderr=%s", exitCode, stderr.String()) + } + if !strings.Contains(stdout.String(), "is now enabled") { + t.Fatalf("enable output: %s", stdout.String()) + } + + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"plugins", "enable", "missing-plugin"}, &stdout, &stderr, deps) + if exitCode != exitUsage { + t.Fatalf("missing plugin exit=%d want %d stderr=%s", exitCode, exitUsage, stderr.String()) + } +} + +func TestRunPluginsDisableUserIgnoresProject(t *testing.T) { + home := t.TempDir() + cwd := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("HOME", home) + + userManifest := filepath.Join(home, "zero", "plugins", "demo", "plugin.json") + projectManifest := filepath.Join(cwd, ".zero", "plugins", "demo", "plugin.json") + for _, path := range []string{userManifest, projectManifest} { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte(`{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "enabled": true +} +`), 0o600); err != nil { + t.Fatal(err) + } + } + + deps := appDeps{getwd: func() (string, error) { return cwd, nil }} + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"plugins", "disable", "zero.demo", "--user"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("disable --user exit=%d stderr=%s", exitCode, stderr.String()) + } + if !strings.Contains(stdout.String(), "is now disabled") { + t.Fatalf("disable --user output: %s", stdout.String()) + } + + userRaw, err := os.ReadFile(userManifest) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(userRaw), `"enabled": false`) { + t.Fatalf("user manifest not disabled: %s", userRaw) + } + projectRaw, err := os.ReadFile(projectManifest) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(projectRaw), `"enabled": false`) { + t.Fatalf("project manifest should stay enabled: %s", projectRaw) + } +} diff --git a/internal/plugins/enabled.go b/internal/plugins/enabled.go new file mode 100644 index 000000000..f3b4b357b --- /dev/null +++ b/internal/plugins/enabled.go @@ -0,0 +1,114 @@ +package plugins + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" +) + +// SetEnabledResult describes a successful enable/disable of one plugin manifest. +type SetEnabledResult struct { + ID string `json:"id"` + Enabled bool `json:"enabled"` + Changed bool `json:"changed"` + Source Source `json:"source"` + ManifestPath string `json:"manifestPath"` +} + +// FindByID returns the loaded plugin with the given id after normal discovery +// and precedence (project overrides user). A missing id returns ok=false. +func FindByID(options LoadOptions, id string) (LoadedPlugin, bool, error) { + id = strings.TrimSpace(id) + if id == "" { + return LoadedPlugin{}, false, errors.New("plugin id is required") + } + result, err := Load(options) + if err != nil { + return LoadedPlugin{}, false, err + } + for _, plugin := range result.Plugins { + if plugin.ID == id { + return plugin, true, nil + } + } + return LoadedPlugin{}, false, nil +} + +// SetEnabled updates the "enabled" field in plugin.json, preserving the rest of +// the manifest. It returns whether the file actually changed. +func SetEnabled(manifestPath string, enabled bool) (changed bool, err error) { + manifestPath = strings.TrimSpace(manifestPath) + if manifestPath == "" { + return false, errors.New("plugin manifest path is required") + } + resolved, err := filepath.Abs(manifestPath) + if err != nil { + return false, err + } + + raw, err := os.ReadFile(resolved) + if err != nil { + return false, err + } + var obj map[string]any + if err := json.Unmarshal(raw, &obj); err != nil { + return false, fmt.Errorf("parse plugin manifest: %w", err) + } + if obj == nil { + return false, errors.New("plugin manifest must be a JSON object") + } + + previous, err := optionalBool(obj, "enabled", true) + if err != nil { + return false, err + } + if previous == enabled { + return false, nil + } + obj["enabled"] = enabled + + data, err := json.MarshalIndent(obj, "", " ") + if err != nil { + return false, err + } + mode := os.FileMode(0o644) + if info, statErr := os.Stat(resolved); statErr == nil { + mode = info.Mode().Perm() + } + tempPath := fmt.Sprintf("%s.tmp-%d-%d", resolved, os.Getpid(), time.Now().UnixNano()) + if err := os.WriteFile(tempPath, append(data, '\n'), mode); err != nil { + return false, err + } + if err := os.Rename(tempPath, resolved); err != nil { + _ = os.Remove(tempPath) + return false, err + } + return true, nil +} + +// 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("%w: %q", ErrNotInstalled, 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 +} diff --git a/internal/plugins/enabled_test.go b/internal/plugins/enabled_test.go new file mode 100644 index 000000000..340cd549b --- /dev/null +++ b/internal/plugins/enabled_test.go @@ -0,0 +1,152 @@ +package plugins + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "runtime" + "testing" +) + +func TestSetEnabledTogglesManifestAndIsIdempotent(t *testing.T) { + root := t.TempDir() + pluginDir := filepath.Join(root, "demo") + writePluginManifest(t, pluginDir, map[string]any{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "description": "keep me", + "tools": []any{}, + }) + manifestPath := filepath.Join(pluginDir, "plugin.json") + + changed, err := SetEnabled(manifestPath, false) + if err != nil { + t.Fatalf("SetEnabled(false): %v", err) + } + if !changed { + t.Fatal("expected first disable to change the manifest") + } + + raw, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + var obj map[string]any + if err := json.Unmarshal(raw, &obj); err != nil { + t.Fatalf("re-read manifest: %v", err) + } + if obj["enabled"] != false { + t.Fatalf("enabled = %#v, want false", obj["enabled"]) + } + if obj["description"] != "keep me" { + t.Fatalf("description was not preserved: %#v", obj["description"]) + } + + changed, err = SetEnabled(manifestPath, false) + if err != nil { + t.Fatalf("SetEnabled(false) again: %v", err) + } + if changed { + t.Fatal("expected second disable to be a no-op") + } + + changed, err = SetEnabled(manifestPath, true) + if err != nil { + t.Fatalf("SetEnabled(true): %v", err) + } + if !changed { + t.Fatal("expected enable to change the manifest") + } +} + +func TestSetEnabledByIDRespectsUserOnlyFilter(t *testing.T) { + dir := t.TempDir() + userRoot := filepath.Join(dir, "user") + projectRoot := filepath.Join(dir, "project") + writePluginManifest(t, filepath.Join(userRoot, "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "User Demo", + "version": "0.1.0", + "enabled": true, + }) + writePluginManifest(t, filepath.Join(projectRoot, "demo"), map[string]any{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Project Demo", + "version": "0.2.0", + "enabled": true, + }) + + result, err := SetEnabledByID(LoadOptions{ + Roots: []Root{ + {Source: SourceUser, Path: userRoot}, + {Source: SourceProject, Path: projectRoot}, + }, + }, "zero.demo", false) + if err != nil { + t.Fatalf("SetEnabledByID: %v", err) + } + if result.Source != SourceProject { + t.Fatalf("source = %s, want project (precedence)", result.Source) + } + if !result.Changed || result.Enabled { + t.Fatalf("unexpected result: %#v", result) + } + + userOnly, err := SetEnabledByID(LoadOptions{ + Roots: []Root{ + {Source: SourceUser, Path: userRoot}, + {Source: SourceProject, Path: projectRoot}, + }, + ExcludeProject: true, + }, "zero.demo", false) + if err != nil { + t.Fatalf("SetEnabledByID user-only: %v", err) + } + if userOnly.Source != SourceUser { + t.Fatalf("user-only source = %s, want user", userOnly.Source) + } +} + +func TestSetEnabledPreservesManifestMode(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows does not preserve chmod bits in os.Stat mode") + } + root := t.TempDir() + pluginDir := filepath.Join(root, "demo") + writePluginManifest(t, pluginDir, map[string]any{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + }) + manifestPath := filepath.Join(pluginDir, "plugin.json") + if err := os.Chmod(manifestPath, 0o644); err != nil { + t.Fatal(err) + } + + if _, err := SetEnabled(manifestPath, false); err != nil { + t.Fatalf("SetEnabled: %v", err) + } + info, err := os.Stat(manifestPath) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o644 { + t.Fatalf("mode = %04o, want 0644", got) + } +} + +func TestSetEnabledByIDMissingPlugin(t *testing.T) { + _, err := SetEnabledByID(LoadOptions{Roots: []Root{{Source: SourceUser, Path: t.TempDir()}}}, "missing", false) + if err == nil { + t.Fatal("expected missing plugin error") + } + if !errors.Is(err, ErrNotInstalled) { + t.Fatalf("error = %v, want ErrNotInstalled", err) + } +} From 7841cc991339c25f8763ea8cada72cad23883d3a Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sat, 17 Oct 2026 17:12:23 +0530 Subject: [PATCH 2/2] fix(plugins): serialize toggles with installs and preserve manifest bytes Address review findings on the enable/disable CLI: - Take the same cross-process installtxn lock plugins add/remove take on the plugin's install root before the final lookup and read, and publish plugin.json with installtxn.WriteFileAtomically while holding it. - Re-resolve the plugin under the lock so a concurrent install can never be overwritten with manifest bytes read before it committed, retrying when precedence moves to the other root; a plugin removed under the lock reports ErrNotInstalled instead of being recreated. - Edit the enabled value in place (a targeted token walk) so field order, formatting, and numeric literals survive; the old decode/re-encode dropped precision (9007199254740993 became 9007199254740992) and sorted the keys. - Fail closed on manifest stat errors, refresh a clean plugins.lock hash after a toggle, and leave pre-existing drift in place instead of absorbing it into the lockfile. - Redact the text-mode output, reject an empty plugin id as a usage error, and document project-over-user precedence in the help text. --- docs/EXTENDING.md | 2 +- internal/cli/extensions.go | 18 +- internal/cli/extensions_test.go | 73 ++++ internal/plugins/enabled.go | 355 ++++++++++++++++--- internal/plugins/enabled_test.go | 574 +++++++++++++++++++++++++++++-- internal/plugins/info.go | 3 +- 6 files changed, 939 insertions(+), 86 deletions(-) diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index d01bdb446..4733ecc1a 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -295,7 +295,7 @@ zero plugins enable github-pr-review # sets "enabled": true in plugin.json zero plugins remove github-pr-review # alias: rm ``` -A plugin is active when it is present under a plugins directory and its `plugin.json` has `"enabled": true` (the default when the field is omitted). Use `zero plugins disable ` / `zero plugins enable ` to toggle that field without removing the plugin. Pass `--user` to target only user plugins and ignore `./.zero/plugins`. +A plugin is active when it is present under a plugins directory and its `plugin.json` has `"enabled": true` (the default when the field is omitted). Use `zero plugins disable ` / `zero plugins enable ` to toggle that field without removing the plugin. Pass `--user` to target only user plugins and ignore `./.zero/plugins`; without it, a project plugin with the same id takes precedence. The toggle edits the `enabled` value in place — the rest of the manifest keeps its formatting, field order, and numeric literals — and serializes with `plugins add` / `plugins remove` on the same plugins root. Plugin commands run with the plugin directory as their working directory. Use relative paths; the loader resolves them at activation time. diff --git a/internal/cli/extensions.go b/internal/cli/extensions.go index c50f55c6f..fe8da1c53 100644 --- a/internal/cli/extensions.go +++ b/internal/cli/extensions.go @@ -115,7 +115,10 @@ func runPluginsToggle(args []string, stdout io.Writer, stderr io.Writer, deps ap if len(positional) != 1 { return writeExecUsageError(stderr, fmt.Sprintf("usage: zero plugins %s [--user] [--json]", commandName)) } - pluginID := positional[0] + pluginID := strings.TrimSpace(positional[0]) + if pluginID == "" { + return writeExecUsageError(stderr, fmt.Sprintf("usage: zero plugins %s [--user] [--json]", commandName)) + } cwd, err := deps.getwd() if err != nil { @@ -140,11 +143,11 @@ func runPluginsToggle(args []string, stdout io.Writer, stderr io.Writer, deps ap } return exitSuccess } - 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.\n", result.ID, state, result.ManifestPath) + if !result.Changed { + message = fmt.Sprintf("Plugin %s was already %s in %s.\n", result.ID, state, result.ManifestPath) + } + if _, err := fmt.Fprint(stdout, redaction.RedactString(message, redaction.Options{})); err != nil { return exitCrash } return exitSuccess @@ -664,6 +667,9 @@ Flags: --user Target only user plugins (ignore ./.zero/plugins) --json Print command result as JSON -h, --help Show this help + +Without --user, a project plugin with the same id takes precedence over a user +plugin, matching the loader's discovery order. `, command) return err } diff --git a/internal/cli/extensions_test.go b/internal/cli/extensions_test.go index e6e531b93..b8b7e1703 100644 --- a/internal/cli/extensions_test.go +++ b/internal/cli/extensions_test.go @@ -337,6 +337,9 @@ func TestRunMCPPermissionsHelpDoesNotOpenStore(t *testing.T) { func TestRunPluginsEnableDisable(t *testing.T) { cwd := t.TempDir() + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("HOME", home) pluginDir := filepath.Join(cwd, ".zero", "plugins", "demo") if err := os.MkdirAll(pluginDir, 0o700); err != nil { t.Fatal(err) @@ -404,6 +407,76 @@ func TestRunPluginsEnableDisable(t *testing.T) { } } +// The text-mode toggle output runs through the redaction pipeline, so a +// manifest path that contains something secret-shaped is masked. +func TestRunPluginsToggleTextOutputIsRedacted(t *testing.T) { + cwd := t.TempDir() + home := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", home) + t.Setenv("HOME", home) + secretDir := "sk-proj-abcdefghijklmnopqrstuvwxyz" + pluginDir := filepath.Join(cwd, ".zero", "plugins", secretDir) + if err := os.MkdirAll(pluginDir, 0o700); err != nil { + t.Fatal(err) + } + manifestPath := filepath.Join(pluginDir, "plugin.json") + if err := os.WriteFile(manifestPath, []byte(`{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0" +} +`), 0o600); err != nil { + t.Fatal(err) + } + + deps := appDeps{getwd: func() (string, error) { return cwd, nil }} + + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"plugins", "disable", "zero.demo"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("disable exit=%d stderr=%s", exitCode, stderr.String()) + } + if strings.Contains(stdout.String(), secretDir) { + t.Fatalf("toggle text leaked a secret-shaped manifest path: %q", stdout.String()) + } + if !strings.Contains(stdout.String(), "[REDACTED]") { + t.Fatalf("toggle text was not redacted: %q", stdout.String()) + } + + stdout.Reset() + stderr.Reset() + exitCode = runWithDeps([]string{"plugins", "disable", "zero.demo"}, &stdout, &stderr, deps) + if exitCode != exitSuccess { + t.Fatalf("noop disable exit=%d stderr=%s", exitCode, stderr.String()) + } + if !strings.Contains(stdout.String(), "was already disabled") { + t.Fatalf("noop toggle output: %q", stdout.String()) + } + if strings.Contains(stdout.String(), secretDir) { + t.Fatalf("noop toggle text leaked a secret-shaped manifest path: %q", stdout.String()) + } +} + +func TestRunPluginsToggleRejectsEmptyID(t *testing.T) { + deps := appDeps{getwd: func() (string, error) { return t.TempDir(), nil }} + + for _, subcommand := range []string{"enable", "disable"} { + t.Run(subcommand, func(t *testing.T) { + var stdout bytes.Buffer + var stderr bytes.Buffer + exitCode := runWithDeps([]string{"plugins", subcommand, ""}, &stdout, &stderr, deps) + if exitCode != exitUsage { + t.Fatalf("exit=%d want %d stderr=%s", exitCode, exitUsage, stderr.String()) + } + if !strings.Contains(stderr.String(), "usage: zero plugins "+subcommand) { + t.Fatalf("stderr = %q", stderr.String()) + } + }) + } +} + func TestRunPluginsDisableUserIgnoresProject(t *testing.T) { home := t.TempDir() cwd := t.TempDir() diff --git a/internal/plugins/enabled.go b/internal/plugins/enabled.go index f3b4b357b..38ce5d800 100644 --- a/internal/plugins/enabled.go +++ b/internal/plugins/enabled.go @@ -1,13 +1,15 @@ package plugins import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "os" - "path/filepath" "strings" - "time" + + "github.com/Twigpine/zero/internal/installtxn" ) // SetEnabledResult describes a successful enable/disable of one plugin manifest. @@ -19,6 +21,10 @@ type SetEnabledResult struct { ManifestPath string `json:"manifestPath"` } +// maxEnabledRelockAttempts bounds how many times SetEnabledByID re-resolves the +// plugin after it moves to another install root while waiting for the lock. +const maxEnabledRelockAttempts = 4 + // FindByID returns the loaded plugin with the given id after normal discovery // and precedence (project overrides user). A missing id returns ok=false. func FindByID(options LoadOptions, id string) (LoadedPlugin, bool, error) { @@ -38,77 +44,326 @@ func FindByID(options LoadOptions, id string) (LoadedPlugin, bool, error) { return LoadedPlugin{}, false, nil } -// SetEnabled updates the "enabled" field in plugin.json, preserving the rest of -// the manifest. It returns whether the file actually changed. -func SetEnabled(manifestPath string, enabled bool) (changed bool, err error) { - manifestPath = strings.TrimSpace(manifestPath) - if manifestPath == "" { - return false, errors.New("plugin manifest path is required") - } - resolved, err := filepath.Abs(manifestPath) - if err != nil { - return false, err +// SetEnabledByID finds a plugin by id (respecting LoadOptions) and toggles the +// top-level "enabled" field of its plugin.json. +// +// The read-modify-write is serialized with plugin installs and removals: it +// takes the same cross-process installtxn lock they take on the plugin's +// install root, re-resolves the plugin under that lock, and publishes the +// manifest with an atomic rename while still holding it, so a concurrent +// install can never be interleaved between the read and the write. Only the +// bytes of the "enabled" value change, so field order, formatting, and numeric +// literals in the rest of the manifest survive the toggle. +func SetEnabledByID(options LoadOptions, id string, enabled bool) (SetEnabledResult, error) { + for attempt := 0; ; attempt++ { + plugin, ok, err := FindByID(options, id) + if err != nil { + return SetEnabledResult{}, err + } + if !ok { + return SetEnabledResult{}, fmt.Errorf("%w: %q", ErrNotInstalled, id) + } + // Take the install root lock before the final lookup and read so an + // install or remove cannot swap the tree between them. + unlock, err := installtxn.Lock(plugin.Root) + if err != nil { + return SetEnabledResult{}, err + } + final, ok, err := FindByID(options, id) + if err != nil { + unlock() + return SetEnabledResult{}, err + } + if !ok { + unlock() + return SetEnabledResult{}, fmt.Errorf("%w: %q", ErrNotInstalled, id) + } + if final.Root != plugin.Root { + // Precedence moved to another root while this lock was pending; + // retry against the root that wins now so the edit still serializes + // with that root's installs. + unlock() + if attempt >= maxEnabledRelockAttempts { + return SetEnabledResult{}, fmt.Errorf("plugin %q kept changing roots while waiting for the install lock", id) + } + continue + } + changed, err := setEnabledLocked(final, enabled) + unlock() + if err != nil { + return SetEnabledResult{}, err + } + return SetEnabledResult{ + ID: final.ID, + Enabled: enabled, + Changed: changed, + Source: final.Source, + ManifestPath: final.ManifestPath, + }, nil } +} - raw, err := os.ReadFile(resolved) +// setEnabledLocked performs the manifest read-modify-write while the caller +// holds the installtxn lock for the plugin's install root. +func setEnabledLocked(plugin LoadedPlugin, enabled bool) (bool, error) { + raw, err := os.ReadFile(plugin.ManifestPath) if err != nil { return false, err } - var obj map[string]any - if err := json.Unmarshal(raw, &obj); err != nil { - return false, fmt.Errorf("parse plugin manifest: %w", err) - } - if obj == nil { - return false, errors.New("plugin manifest must be a JSON object") - } - - previous, err := optionalBool(obj, "enabled", true) + data, changed, err := toggleEnabledManifest(raw, enabled) if err != nil { return false, err } - if previous == enabled { + if !changed { return false, nil } - obj["enabled"] = enabled - data, err := json.MarshalIndent(obj, "", " ") + // Read the lockfile before the edit so the refresh below only runs when the + // installed tree still matched its recorded hash; a toggle must never mask + // pre-existing drift. + lock, entry, hashClean := cleanLockEntry(plugin) + + // Fail closed on stat errors instead of widening the permissions. + info, err := os.Stat(plugin.ManifestPath) if err != nil { + return false, fmt.Errorf("inspect plugin manifest: %w", err) + } + if err := installtxn.WriteFileAtomically(plugin.ManifestPath, data, info.Mode().Perm()); err != nil { return false, err } - mode := os.FileMode(0o644) - if info, statErr := os.Stat(resolved); statErr == nil { - mode = info.Mode().Perm() + if !hashClean { + return true, nil } - tempPath := fmt.Sprintf("%s.tmp-%d-%d", resolved, os.Getpid(), time.Now().UnixNano()) - if err := os.WriteFile(tempPath, append(data, '\n'), mode); err != nil { - return false, err + after, err := hashTree(plugin.PluginDir) + if err != nil { + return true, fmt.Errorf("plugin manifest updated but %s hash refresh failed: %w", LockFileName, err) } - if err := os.Rename(tempPath, resolved); err != nil { - _ = os.Remove(tempPath) - return false, err + entry.Hash = after + lock[plugin.ID] = entry + if err := writeLock(plugin.Root, lock); err != nil { + return true, fmt.Errorf("plugin manifest updated but %s hash refresh failed: %w", LockFileName, err) } return true, nil } -// 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) +// cleanLockEntry reports whether the tree on disk still matches the lockfile +// entry recorded for the plugin and returns both for a refresh. +func cleanLockEntry(plugin LoadedPlugin) (map[string]LockEntry, LockEntry, bool) { + lock, err := ReadLock(plugin.Root) if err != nil { - return SetEnabledResult{}, err + return nil, LockEntry{}, false } + entry, ok := lock[plugin.ID] if !ok { - return SetEnabledResult{}, fmt.Errorf("%w: %q", ErrNotInstalled, id) + return nil, LockEntry{}, false } - changed, err := SetEnabled(plugin.ManifestPath, enabled) + current, err := hashTree(plugin.PluginDir) + if err != nil || current != entry.Hash { + return nil, LockEntry{}, false + } + return lock, entry, true +} + +// toggleEnabledManifest returns raw with the top-level "enabled" member set to +// enabled. A missing or null value counts as enabled, matching the loader's +// optionalBool fallback. changed reports whether the result differs from raw. +func toggleEnabledManifest(raw []byte, enabled bool) (out []byte, changed bool, err error) { + dec := json.NewDecoder(bytes.NewReader(raw)) + openTok, err := dec.Token() if err != nil { - return SetEnabledResult{}, err - } - return SetEnabledResult{ - ID: plugin.ID, - Enabled: enabled, - Changed: changed, - Source: plugin.Source, - ManifestPath: plugin.ManifestPath, - }, nil + return nil, false, fmt.Errorf("parse plugin manifest: %w", err) + } + if delim, ok := openTok.(json.Delim); !ok || delim != '{' { + return nil, false, errors.New("plugin manifest must be a JSON object") + } + openEnd := dec.InputOffset() + + current := true + var valueStart, valueEnd int64 = -1, -1 + for dec.More() { + keyTok, err := dec.Token() + if err != nil { + return nil, false, fmt.Errorf("parse plugin manifest: %w", err) + } + key, ok := keyTok.(string) + if !ok { + return nil, false, errors.New("plugin manifest must be a JSON object") + } + keyEnd := dec.InputOffset() + if key != "enabled" { + if err := skipManifestValue(dec); err != nil { + return nil, false, fmt.Errorf("parse plugin manifest: %w", err) + } + continue + } + valueTok, err := dec.Token() + if err != nil { + return nil, false, fmt.Errorf("parse plugin manifest: %w", err) + } + var literal string + switch value := valueTok.(type) { + case bool: + current = value + literal = "false" + if value { + literal = "true" + } + case nil: + current = true + literal = "null" + default: + return nil, false, ManifestError{FieldPath: "enabled", Message: "Expected a boolean."} + } + start, err := enabledValueOffset(raw, keyEnd) + if err != nil { + return nil, false, err + } + if start+int64(len(literal)) > int64(len(raw)) || + !bytes.Equal(raw[start:start+int64(len(literal))], []byte(literal)) { + return nil, false, errors.New("parse plugin manifest: cannot locate the enabled value") + } + // A duplicate key keeps the last occurrence, matching json.Unmarshal. + valueStart, valueEnd = start, start+int64(len(literal)) + } + closingTok, err := dec.Token() + if err != nil { + return nil, false, fmt.Errorf("parse plugin manifest: %w", err) + } + if delim, ok := closingTok.(json.Delim); !ok || delim != '}' { + return nil, false, errors.New("plugin manifest must be a JSON object") + } + closingEnd := dec.InputOffset() + if _, err := dec.Token(); err != io.EOF { + return nil, false, errors.New("parse plugin manifest: unexpected data after the top-level object") + } + + if current == enabled { + return nil, false, nil + } + newLiteral := "false" + if enabled { + newLiteral = "true" + } + if valueStart >= 0 { + out := make([]byte, 0, len(raw)-int(valueEnd-valueStart)+len(newLiteral)) + out = append(out, raw[:valueStart]...) + out = append(out, newLiteral...) + out = append(out, raw[valueEnd:]...) + return out, true, nil + } + return insertEnabledMember(raw, openEnd, closingEnd-1, newLiteral), true, nil +} + +// enabledValueOffset returns the byte offset of the value that follows the +// colon after an "enabled" key token that ended at keyEnd. +func enabledValueOffset(raw []byte, keyEnd int64) (int64, error) { + i := keyEnd + for i < int64(len(raw)) && isJSONSpace(raw[i]) { + i++ + } + if i >= int64(len(raw)) || raw[i] != ':' { + return 0, errors.New("parse plugin manifest: missing colon after key") + } + i++ + for i < int64(len(raw)) && isJSONSpace(raw[i]) { + i++ + } + return i, nil +} + +// insertEnabledMember adds a top-level "enabled" member to a manifest that has +// none, mirroring the document's own whitespace style where it can. openEnd is +// the byte offset just after the opening brace and bracePos is the byte index +// of the closing brace. +func insertEnabledMember(raw []byte, openEnd, bracePos int64, literal string) []byte { + insertPos := bracePos + for insertPos > openEnd && isJSONSpace(raw[insertPos-1]) { + insertPos-- + } + + newline := "\n" + if bytes.Contains(raw, []byte("\r\n")) { + newline = "\r\n" + } + + first := openEnd + for first < bracePos && isJSONSpace(raw[first]) { + first++ + } + empty := first >= bracePos + multiline := bytes.IndexByte(raw[openEnd:bracePos], '\n') >= 0 + + member := `"enabled": ` + literal + var insert string + switch { + case empty && !multiline: + insert = member + case empty && multiline: + insert = newline + closingLineIndent(raw, openEnd, bracePos) + " " + member + case !empty && !multiline: + insert = "," + member + default: + insert = "," + newline + memberLineIndent(raw, openEnd, first) + member + } + + out := make([]byte, 0, len(raw)+len(insert)) + out = append(out, raw[:insertPos]...) + out = append(out, insert...) + out = append(out, raw[insertPos:]...) + return out +} + +// memberLineIndent returns the indentation of the line the first member of a +// JSON object starts on. first is the byte index of that member's first token. +func memberLineIndent(raw []byte, openEnd, first int64) string { + lineStart := first + for lineStart > openEnd && raw[lineStart-1] != '\n' { + lineStart-- + } + end := lineStart + for end < first && (raw[end] == ' ' || raw[end] == '\t') { + end++ + } + return string(raw[lineStart:end]) +} + +// closingLineIndent returns the indentation before an object's closing brace. +func closingLineIndent(raw []byte, openEnd, bracePos int64) string { + start := bracePos + for start > openEnd && (raw[start-1] == ' ' || raw[start-1] == '\t') { + start-- + } + return string(raw[start:bracePos]) +} + +// skipManifestValue consumes the next JSON value from dec. +func skipManifestValue(dec *json.Decoder) error { + tok, err := dec.Token() + if err != nil { + return err + } + delim, ok := tok.(json.Delim) + if !ok || (delim != '{' && delim != '[') { + return nil + } + depth := 1 + for depth > 0 { + tok, err := dec.Token() + if err != nil { + return err + } + if inner, ok := tok.(json.Delim); ok { + switch inner { + case '{', '[': + depth++ + case '}', ']': + depth-- + } + } + } + return nil +} + +func isJSONSpace(b byte) bool { + return b == ' ' || b == '\t' || b == '\n' || b == '\r' } diff --git a/internal/plugins/enabled_test.go b/internal/plugins/enabled_test.go index 340cd549b..5e19a5b52 100644 --- a/internal/plugins/enabled_test.go +++ b/internal/plugins/enabled_test.go @@ -1,18 +1,113 @@ package plugins import ( - "encoding/json" "errors" "os" "path/filepath" "runtime" + "strings" "testing" + "time" + + "github.com/Twigpine/zero/internal/installtxn" ) -func TestSetEnabledTogglesManifestAndIsIdempotent(t *testing.T) { +func enableTestOptions(root string) LoadOptions { + return LoadOptions{Roots: []Root{{Source: SourceUser, Path: root}}} +} + +func writeRawManifest(t *testing.T, root, dir, raw string) string { + t.Helper() + pluginDir := filepath.Join(root, dir) + if err := os.MkdirAll(pluginDir, 0o700); err != nil { + t.Fatal(err) + } + manifestPath := filepath.Join(pluginDir, "plugin.json") + if err := os.WriteFile(manifestPath, []byte(raw), 0o600); err != nil { + t.Fatal(err) + } + return manifestPath +} + +func demoManifest(enabled bool) string { + state := "false" + if enabled { + state = "true" + } + return `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "enabled": ` + state + ` +} +` +} + +func TestSetEnabledByIDPreservesManifestBytes(t *testing.T) { + root := t.TempDir() + raw := `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "limits": { + "maxBytes": 9007199254740993 + }, + "description": "keep me", + "enabled": true, + "tools": [] +} +` + manifestPath := writeRawManifest(t, root, "demo", raw) + + result, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false) + if err != nil { + t.Fatalf("SetEnabledByID(false): %v", err) + } + if !result.Changed || result.Enabled { + t.Fatalf("unexpected result: %#v", result) + } + + disabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + want := strings.Replace(raw, `"enabled": true`, `"enabled": false`, 1) + if string(disabled) != want { + t.Fatalf("toggle changed more than the enabled value:\n got %q\nwant %q", disabled, want) + } + if !strings.Contains(string(disabled), "9007199254740993") { + t.Fatal("large integer literal was rounded by the toggle") + } + + plugin, ok, err := FindByID(enableTestOptions(root), "zero.demo") + if err != nil || !ok { + t.Fatalf("FindByID after disable: ok=%v err=%v", ok, err) + } + if plugin.Enabled { + t.Fatal("loader still reports the plugin enabled after disable") + } + + result, err = SetEnabledByID(enableTestOptions(root), "zero.demo", true) + if err != nil { + t.Fatalf("SetEnabledByID(true): %v", err) + } + if !result.Changed || !result.Enabled { + t.Fatalf("unexpected enable result: %#v", result) + } + enabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + if string(enabled) != raw { + t.Fatalf("round trip did not restore the original bytes:\n got %q\nwant %q", enabled, raw) + } +} + +func TestSetEnabledByIDTogglesManifestAndIsIdempotent(t *testing.T) { root := t.TempDir() - pluginDir := filepath.Join(root, "demo") - writePluginManifest(t, pluginDir, map[string]any{ + writePluginManifest(t, filepath.Join(root, "demo"), map[string]any{ "schemaVersion": 1, "id": "zero.demo", "name": "Demo", @@ -20,48 +115,213 @@ func TestSetEnabledTogglesManifestAndIsIdempotent(t *testing.T) { "description": "keep me", "tools": []any{}, }) - manifestPath := filepath.Join(pluginDir, "plugin.json") + manifestPath := filepath.Join(root, "demo", "plugin.json") - changed, err := SetEnabled(manifestPath, false) + result, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false) if err != nil { - t.Fatalf("SetEnabled(false): %v", err) + t.Fatalf("SetEnabledByID(false): %v", err) } - if !changed { + if !result.Changed { t.Fatal("expected first disable to change the manifest") } - raw, err := os.ReadFile(manifestPath) if err != nil { t.Fatal(err) } - var obj map[string]any - if err := json.Unmarshal(raw, &obj); err != nil { - t.Fatalf("re-read manifest: %v", err) - } - if obj["enabled"] != false { - t.Fatalf("enabled = %#v, want false", obj["enabled"]) + if !strings.Contains(string(raw), `"enabled": false`) { + t.Fatalf("manifest not disabled: %s", raw) } - if obj["description"] != "keep me" { - t.Fatalf("description was not preserved: %#v", obj["description"]) + if !strings.Contains(string(raw), `"keep me"`) { + t.Fatalf("description was not preserved: %s", raw) } - changed, err = SetEnabled(manifestPath, false) + result, err = SetEnabledByID(enableTestOptions(root), "zero.demo", false) if err != nil { - t.Fatalf("SetEnabled(false) again: %v", err) + t.Fatalf("SetEnabledByID(false) again: %v", err) } - if changed { + if result.Changed { t.Fatal("expected second disable to be a no-op") } - changed, err = SetEnabled(manifestPath, true) + result, err = SetEnabledByID(enableTestOptions(root), "zero.demo", true) if err != nil { - t.Fatalf("SetEnabled(true): %v", err) + t.Fatalf("SetEnabledByID(true): %v", err) } - if !changed { + if !result.Changed { t.Fatal("expected enable to change the manifest") } } +func TestSetEnabledByIDInsertsMissingField(t *testing.T) { + t.Run("pretty manifest", func(t *testing.T) { + root := t.TempDir() + raw := `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0" +} +` + manifestPath := writeRawManifest(t, root, "demo", raw) + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err != nil { + t.Fatalf("SetEnabledByID(false): %v", err) + } + disabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + wantDisabled := `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "enabled": false +} +` + if string(disabled) != wantDisabled { + t.Fatalf("after insert:\n got %q\nwant %q", disabled, wantDisabled) + } + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", true); err != nil { + t.Fatalf("SetEnabledByID(true): %v", err) + } + enabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + wantEnabled := strings.Replace(wantDisabled, `"enabled": false`, `"enabled": true`, 1) + if string(enabled) != wantEnabled { + t.Fatalf("after replace:\n got %q\nwant %q", enabled, wantEnabled) + } + }) + + t.Run("compact manifest", func(t *testing.T) { + root := t.TempDir() + raw := `{"schemaVersion":1,"id":"zero.demo","name":"Demo","version":"1.0.0"}` + manifestPath := writeRawManifest(t, root, "demo", raw) + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err != nil { + t.Fatalf("SetEnabledByID(false): %v", err) + } + disabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + want := `{"schemaVersion":1,"id":"zero.demo","name":"Demo","version":"1.0.0","enabled": false}` + if string(disabled) != want { + t.Fatalf("compact insert:\n got %q\nwant %q", disabled, want) + } + }) + + t.Run("null value", func(t *testing.T) { + root := t.TempDir() + raw := `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "1.0.0", + "enabled": null +} +` + manifestPath := writeRawManifest(t, root, "demo", raw) + + result, err := SetEnabledByID(enableTestOptions(root), "zero.demo", true) + if err != nil { + t.Fatalf("SetEnabledByID(true): %v", err) + } + if result.Changed { + t.Fatal("null counts as enabled, so enabling must be a no-op") + } + unchanged, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + if string(unchanged) != raw { + t.Fatalf("no-op toggle rewrote the manifest: %q", unchanged) + } + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err != nil { + t.Fatalf("SetEnabledByID(false): %v", err) + } + disabled, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + want := strings.Replace(raw, `"enabled": null`, `"enabled": false`, 1) + if string(disabled) != want { + t.Fatalf("null replacement:\n got %q\nwant %q", disabled, want) + } + }) +} + +func TestToggleEnabledManifestFormattingEdges(t *testing.T) { + cases := []struct { + name string + in string + enabled bool + want string + changed bool + }{ + {"empty object", `{}`, false, `{"enabled": false}`, true}, + {"empty object with space", `{ }`, false, `{"enabled": false }`, true}, + {"empty multiline", "{\n}", false, "{\n \"enabled\": false\n}", true}, + {"compact members", `{"a":1}`, false, `{"a":1,"enabled": false}`, true}, + {"crlf members", "{\r\n \"a\": 1\r\n}", false, "{\r\n \"a\": 1,\r\n \"enabled\": false\r\n}", true}, + {"nested enabled untouched", `{"nested":{"enabled":true},"a":2}`, false, `{"nested":{"enabled":true},"a":2,"enabled": false}`, true}, + {"replace with true", `{"enabled": false}`, true, `{"enabled": true}`, true}, + {"replace with false", `{"enabled":true}`, false, `{"enabled":false}`, true}, + {"null to false", `{"enabled": null}`, false, `{"enabled": false}`, true}, + {"null to true is a no-op", `{"enabled": null}`, true, `{"enabled": null}`, false}, + {"already enabled is a no-op", `{"enabled": true}`, true, `{"enabled": true}`, false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out, changed, err := toggleEnabledManifest([]byte(tc.in), tc.enabled) + if err != nil { + t.Fatalf("toggleEnabledManifest: %v", err) + } + if changed != tc.changed { + t.Fatalf("changed = %v, want %v", changed, tc.changed) + } + if !tc.changed { + if out != nil { + t.Fatalf("no-op returned bytes: %q", out) + } + return + } + if string(out) != tc.want { + t.Fatalf("got %q, want %q", out, tc.want) + } + }) + } +} + +func TestToggleEnabledManifestRejectsMalformedInput(t *testing.T) { + for _, raw := range []string{`{"enabled": "yes"}`, `{"enabled": 1}`, `{"enabled": []}`, `{"enabled": {}}`} { + t.Run("value "+raw, func(t *testing.T) { + _, _, err := toggleEnabledManifest([]byte(raw), false) + if err == nil { + t.Fatal("expected error") + } + var manifestErr ManifestError + if !errors.As(err, &manifestErr) { + t.Fatalf("error = %v, want ManifestError", err) + } + if !strings.Contains(err.Error(), "enabled") { + t.Fatalf("error = %v, want it to name the enabled field", err) + } + }) + } + for _, raw := range []string{``, `{`, `[]`, `null`, `"str"`, `{} {}`, `{}{}`} { + t.Run("document "+raw, func(t *testing.T) { + if _, _, err := toggleEnabledManifest([]byte(raw), false); err == nil { + t.Fatalf("toggleEnabledManifest(%q) succeeded, want error", raw) + } + }) + } +} + func TestSetEnabledByIDRespectsUserOnlyFilter(t *testing.T) { dir := t.TempDir() userRoot := filepath.Join(dir, "user") @@ -117,20 +377,19 @@ func TestSetEnabledPreservesManifestMode(t *testing.T) { t.Skip("Windows does not preserve chmod bits in os.Stat mode") } root := t.TempDir() - pluginDir := filepath.Join(root, "demo") - writePluginManifest(t, pluginDir, map[string]any{ + writePluginManifest(t, filepath.Join(root, "demo"), map[string]any{ "schemaVersion": 1, "id": "zero.demo", "name": "Demo", "version": "1.0.0", }) - manifestPath := filepath.Join(pluginDir, "plugin.json") + manifestPath := filepath.Join(root, "demo", "plugin.json") if err := os.Chmod(manifestPath, 0o644); err != nil { t.Fatal(err) } - if _, err := SetEnabled(manifestPath, false); err != nil { - t.Fatalf("SetEnabled: %v", err) + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err != nil { + t.Fatalf("SetEnabledByID: %v", err) } info, err := os.Stat(manifestPath) if err != nil { @@ -150,3 +409,262 @@ func TestSetEnabledByIDMissingPlugin(t *testing.T) { t.Fatalf("error = %v, want ErrNotInstalled", err) } } + +func TestSetEnabledPublishFailureLeavesManifestIntact(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows directory permissions do not block writes the way this test needs") + } + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permission checks") + } + root := t.TempDir() + raw := demoManifest(true) + manifestPath := writeRawManifest(t, root, "demo", raw) + pluginDir := filepath.Dir(manifestPath) + if err := os.Chmod(pluginDir, 0o555); err != nil { + t.Fatal(err) + } + defer func() { _ = os.Chmod(pluginDir, 0o755) }() + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err == nil { + t.Fatal("expected the publish to fail in a read-only plugin directory") + } + after, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + if string(after) != raw { + t.Fatalf("failed publish changed the manifest:\n got %q\nwant %q", after, raw) + } +} + +// TestSetEnabledByIDWaitsForInstallRootLock proves the toggle takes the same +// cross-process lock Install and Remove take on the plugin's install root: it +// must not complete while an install holds that lock. +func TestSetEnabledByIDWaitsForInstallRootLock(t *testing.T) { + root := t.TempDir() + manifestPath := writeRawManifest(t, root, "demo", demoManifest(true)) + + unlock, err := installtxn.Lock(root) + if err != nil { + t.Fatal(err) + } + + results := make(chan SetEnabledResult, 1) + failures := make(chan error, 1) + go func() { + result, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false) + if err != nil { + failures <- err + return + } + results <- result + }() + + select { + case result := <-results: + unlock() + t.Fatalf("toggle completed while the install lock was held: %#v", result) + case err := <-failures: + unlock() + t.Fatalf("toggle failed while the install lock was held: %v", err) + case <-time.After(200 * time.Millisecond): + } + + unlock() + + select { + case result := <-results: + if !result.Changed { + t.Fatalf("toggle did not report a change: %#v", result) + } + case err := <-failures: + t.Fatalf("toggle failed after the lock was released: %v", err) + case <-time.After(10 * time.Second): + t.Fatal("toggle did not finish after the install lock was released") + } + + raw, err := os.ReadFile(manifestPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(raw), `"enabled": false`) { + t.Fatalf("manifest not disabled: %s", raw) + } +} + +// TestSetEnabledByIDSynchronizesWithStagedInstallCommit pauses an install +// between staging and commit, lets the toggle try to run, then commits the +// install. The toggle must apply to the newly installed manifest instead of +// overwriting it with bytes read before the install, and the lockfile entry +// must describe the tree actually on disk. +func TestSetEnabledByIDSynchronizesWithStagedInstallCommit(t *testing.T) { + root := t.TempDir() + oldRaw := demoManifest(true) + pluginDir := filepath.Dir(writeRawManifest(t, root, "zero.demo", oldRaw)) + + // Build the replacement tree with the same staging primitives Install + // uses and leave it staged but uncommitted. + src := t.TempDir() + newRaw := `{ + "schemaVersion": 1, + "id": "zero.demo", + "name": "Demo", + "version": "2.0.0", + "description": "reinstalled", + "enabled": true +} +` + srcPlugin := filepath.Join(src, "zero.demo") + if err := os.MkdirAll(srcPlugin, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(srcPlugin, "plugin.json"), []byte(newRaw), 0o600); err != nil { + t.Fatal(err) + } + staged, cleanup, err := installtxn.StageDir(root) + if err != nil { + t.Fatal(err) + } + defer cleanup() + if err := copyTree(srcPlugin, staged); err != nil { + t.Fatal(err) + } + stagedHash, err := hashTree(staged) + if err != nil { + t.Fatal(err) + } + + // An install holds the root lock from before its commit until after it. + unlock, err := installtxn.Lock(root) + if err != nil { + t.Fatal(err) + } + + results := make(chan SetEnabledResult, 1) + failures := make(chan error, 1) + go func() { + result, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false) + if err != nil { + failures <- err + return + } + results <- result + }() + + select { + case result := <-results: + unlock() + t.Fatalf("toggle completed while the install lock was held: %#v", result) + case err := <-failures: + unlock() + t.Fatalf("toggle failed while the install lock was held: %v", err) + case <-time.After(200 * time.Millisecond): + } + + // Commit the staged install exactly like Install does: swap the tree and + // publish the lockfile entry while still holding the lock. + lock, err := ReadLock(root) + if err != nil { + unlock() + t.Fatal(err) + } + lock["zero.demo"] = LockEntry{Source: string(SourceUser), Hash: stagedHash} + if err := installtxn.CommitDir(pluginDir, staged, func() error { return writeLock(root, lock) }); err != nil { + unlock() + t.Fatal(err) + } + unlock() + + var result SetEnabledResult + select { + case result = <-results: + case err := <-failures: + t.Fatalf("toggle failed after the install committed: %v", err) + case <-time.After(10 * time.Second): + t.Fatal("toggle did not finish after the install committed") + } + if !result.Changed { + t.Fatalf("toggle result = %#v, want changed", result) + } + + // The toggle must have applied to the newly installed manifest, not to the + // bytes it first read. + final, err := os.ReadFile(filepath.Join(pluginDir, "plugin.json")) + if err != nil { + t.Fatal(err) + } + want := strings.Replace(newRaw, `"enabled": true`, `"enabled": false`, 1) + if string(final) != want { + t.Fatalf("manifest after concurrent install + disable:\n got %q\nwant %q", final, want) + } + + // The lockfile entry must describe the tree that is actually on disk. + entries, err := ReadLock(root) + if err != nil { + t.Fatal(err) + } + current, err := hashTree(pluginDir) + if err != nil { + t.Fatal(err) + } + if entries["zero.demo"].Hash != current { + t.Fatalf("plugins.lock hash = %q, want current tree hash %q", entries["zero.demo"].Hash, current) + } +} + +func TestSetEnabledByIDRefreshesCleanLockEntry(t *testing.T) { + root := t.TempDir() + manifestPath := writeRawManifest(t, root, "zero.demo", demoManifest(true)) + pluginDir := filepath.Dir(manifestPath) + + current, err := hashTree(pluginDir) + if err != nil { + t.Fatal(err) + } + if err := writeLock(root, map[string]LockEntry{"zero.demo": {Source: string(SourceUser), Hash: current}}); err != nil { + t.Fatal(err) + } + + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", false); err != nil { + t.Fatalf("SetEnabledByID: %v", err) + } + entries, err := ReadLock(root) + if err != nil { + t.Fatal(err) + } + afterToggle, err := hashTree(pluginDir) + if err != nil { + t.Fatal(err) + } + if entries["zero.demo"].Hash == current { + t.Fatal("lock hash was not refreshed for a clean install") + } + if entries["zero.demo"].Hash != afterToggle { + t.Fatalf("lock hash = %q, want refreshed tree hash %q", entries["zero.demo"].Hash, afterToggle) + } + + info, err := Info(InfoOptions{LoadOptions: enableTestOptions(root)}, "zero.demo") + if err != nil { + t.Fatal(err) + } + if info.HashDrift { + t.Fatal("toggle left hash drift behind for a clean install") + } + + // Pre-existing drift must survive the next toggle instead of being + // absorbed into the lockfile. + if err := os.WriteFile(filepath.Join(pluginDir, "extra.txt"), []byte("edited\n"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := SetEnabledByID(enableTestOptions(root), "zero.demo", true); err != nil { + t.Fatalf("SetEnabledByID(true): %v", err) + } + entries, err = ReadLock(root) + if err != nil { + t.Fatal(err) + } + if entries["zero.demo"].Hash != afterToggle { + t.Fatalf("toggle absorbed pre-existing drift: hash = %q, want %q", entries["zero.demo"].Hash, afterToggle) + } +} diff --git a/internal/plugins/info.go b/internal/plugins/info.go index 0dac22570..82b64f846 100644 --- a/internal/plugins/info.go +++ b/internal/plugins/info.go @@ -7,7 +7,8 @@ import ( "strings" ) -// ErrNotInstalled is returned when Info cannot find a plugin with the requested id. +// ErrNotInstalled is returned when a plugin id cannot be found under the given +// LoadOptions (for example by Info or SetEnabledByID). var ErrNotInstalled = errors.New("plugin is not installed") // PluginInfo combines a loaded plugin with optional lockfile metadata.