fix(plugins): keep the toggle on the pinned plugin directory throughout

The previous fix pinned the manifest read and publish to a handle on the
plugin directory but still hashed the tree for plugins.lock by path, and
reported success without checking that the path still named the pinned
directory. If the directory was replaced after it was opened, the edit
landed in the moved-away original while the lockfile check and refresh
read the replacement, and the command reported a state the installed
plugin did not have.

Hash the tree before and after the edit through the pinned handle, and
confirm that the plugin's directory entry is still the pinned directory
before refreshing the lockfile or reporting any result, including a
no-op. hashTree now delegates to the same rooted walker, so path and
handle callers hash identical bytes.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

internal/plugins/enabled.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"strings"
@@ -118,18 +119,28 @@ func SetEnabledByID(options LoadOptions, id string, enabled bool) (SetEnabledRes
 // holds the installtxn lock for the plugin's install root.
 //
 // The lock only serializes cooperating installers; it does not stop the plugin
-// directory from being swapped for a link after discovery found it. So the
-// manifest is read and published through a handle pinned to that directory
-// (openPluginDir), never by re-resolving its path.
+// directory from being swapped after discovery found it. So the whole update
+// works on one directory, pinned by openPluginDir: the manifest is read and
+// published, and the tree hashed for the lockfile, through that handle, never
+// by re-resolving the path.
 func setEnabledLocked(plugin LoadedPlugin, enabled bool) (bool, error) {
-	dir, err := openPluginDir(plugin)
+	pinned, err := openPluginDir(plugin)
 	if err != nil {
 		return false, err
 	}
-	defer dir.Close()
+	defer pinned.Close()
+	return setEnabledPinned(plugin, pinned, enabled)
+}
 
+// setEnabledPinned is setEnabledLocked on an already pinned plugin directory.
+//
+// The edit lands in the pinned directory even if the path now names another
+// one, so before refreshing the lockfile or reporting any result it confirms
+// that the directory entry still is the pinned directory. Otherwise the
+// command would report a state that the installed plugin does not have.
+func setEnabledPinned(plugin LoadedPlugin, pinned *pinnedPluginDir, enabled bool) (bool, error) {
 	manifestName := filepath.Base(plugin.ManifestPath)
-	raw, perm, err := readManifest(dir, manifestName)
+	raw, perm, err := readManifest(pinned.dir, manifestName)
 	if err != nil {
 		return false, err
 	}
@@ -138,24 +149,33 @@ func setEnabledLocked(plugin LoadedPlugin, enabled bool) (bool, error) {
 		return false, err
 	}
 	if !changed {
+		if err := pinned.verifyLive(); err != nil {
+			return false, err
+		}
 		return false, nil
 	}
 
 	// 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)
+	lock, entry, hashClean := cleanLockEntry(plugin, pinned.dir)
 
-	if err := publishManifest(dir, manifestName, data, perm); err != nil {
+	if err := publishManifest(pinned.dir, manifestName, data, perm); err != nil {
 		return false, err
 	}
 	if !hashClean {
+		if err := pinned.verifyLive(); err != nil {
+			return false, err
+		}
 		return true, nil
 	}
-	after, err := hashTree(plugin.PluginDir)
+	after, err := hashRootedTree(pinned.dir)
 	if err != nil {
 		return true, fmt.Errorf("plugin manifest updated but %s hash refresh failed: %w", LockFileName, err)
 	}
+	if err := pinned.verifyLive(); err != nil {
+		return false, err
+	}
 	entry.Hash = after
 	lock[plugin.ID] = entry
 	if err := writeLock(plugin.Root, lock); err != nil {
@@ -164,45 +184,80 @@ func setEnabledLocked(plugin LoadedPlugin, enabled bool) (bool, error) {
 	return true, nil
 }
 
-// openPluginDir returns a handle on the plugin directory discovery found,
-// confined to the plugin's install root.
+// errPluginDirReplaced reports that the plugin's directory entry no longer
+// names the directory the toggle pinned and edited.
+var errPluginDirReplaced = errors.New("plugin directory was replaced while it was being updated; the plugin installed there now was not changed")
+
+// pinnedPluginDir is the plugin directory discovery found, held open for the
+// length of one toggle.
+type pinnedPluginDir struct {
+	root *os.Root // the plugin's install root
+	name string   // the plugin directory's entry name in root
+	path string   // the plugin directory's path, for messages
+	dir  *os.Root // the plugin directory itself
+}
+
+// Close releases both handles.
+func (p *pinnedPluginDir) Close() {
+	_ = p.dir.Close()
+	_ = p.root.Close()
+}
+
+// verifyLive confirms that the plugin's entry in its install root is still a
+// real directory, not a symlink, junction, or other reparse point, and is the
+// very directory p.dir holds open.
+func (p *pinnedPluginDir) verifyLive() error {
+	opened, err := p.dir.Stat(".")
+	if err != nil {
+		return fmt.Errorf("inspect plugin directory: %w", err)
+	}
+	entry, err := p.root.Lstat(p.name)
+	if errors.Is(err, fs.ErrNotExist) {
+		return fmt.Errorf("%w: %s", errPluginDirReplaced, p.path)
+	}
+	if err != nil {
+		return fmt.Errorf("inspect plugin directory: %w", err)
+	}
+	if entry.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 {
+		return fmt.Errorf("%w: plugin directory %s", pathjail.ErrReparse, p.path)
+	}
+	if !os.SameFile(entry, opened) {
+		return fmt.Errorf("%w: %s", errPluginDirReplaced, p.path)
+	}
+	return nil
+}
+
+// openPluginDir pins the plugin directory discovery found, confined to the
+// plugin's install root.
 //
-// The directory entry must be a real directory, not a symlink, junction, or
-// other reparse point, and it must be the very directory the handle opened: a
-// link swapped in before the open is refused here, and one swapped in after it
-// cannot redirect anything, because every later read and write is relative to
-// the returned handle rather than to a path.
-func openPluginDir(plugin LoadedPlugin) (*os.Root, error) {
+// A link swapped in before the open is refused here. One swapped in after it
+// cannot redirect anything, because every later read, write, and hash is
+// relative to the returned handle rather than to a path; verifyLive then
+// detects that the path no longer names the edited directory.
+func openPluginDir(plugin LoadedPlugin) (*pinnedPluginDir, error) {
 	root, name, err := pathjail.Open(plugin.Root, plugin.PluginDir)
 	if err != nil {
 		return nil, fmt.Errorf("open plugin directory: %w", err)
 	}
-	defer root.Close()
 	if name == "." || filepath.Base(name) != name {
+		_ = root.Close()
 		return nil, fmt.Errorf("%w: plugin directory %s is not an entry of %s", pathjail.ErrEscapes, plugin.PluginDir, plugin.Root)
 	}
 	if err := pathjail.RefuseReparse(root, name); err != nil {
+		_ = root.Close()
 		return nil, fmt.Errorf("open plugin directory: %w", err)
 	}
 	dir, err := root.OpenRoot(name)
 	if err != nil {
+		_ = root.Close()
 		return nil, fmt.Errorf("open plugin directory: %w", err)
 	}
-	opened, err := dir.Stat(".")
-	if err != nil {
-		_ = dir.Close()
-		return nil, fmt.Errorf("inspect plugin directory: %w", err)
+	pinned := &pinnedPluginDir{root: root, name: name, path: plugin.PluginDir, dir: dir}
+	if err := pinned.verifyLive(); err != nil {
+		pinned.Close()
+		return nil, err
 	}
-	entry, err := root.Lstat(name)
-	if err != nil {
-		_ = dir.Close()
-		return nil, fmt.Errorf("inspect plugin directory: %w", err)
-	}
-	if entry.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 || !os.SameFile(entry, opened) {
-		_ = dir.Close()
-		return nil, fmt.Errorf("%w: plugin directory %s changed while it was being opened", pathjail.ErrReparse, plugin.PluginDir)
-	}
-	return dir, nil
+	return pinned, nil
 }
 
 // readManifest reads name from dir and returns its bytes and permission bits.
@@ -254,9 +309,9 @@ func publishManifest(dir *os.Root, name string, data []byte, perm os.FileMode) e
 	return nil
 }
 
-// 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) {
+// cleanLockEntry reports whether the pinned plugin directory dir still matches
+// the lockfile entry recorded for the plugin and returns both for a refresh.
+func cleanLockEntry(plugin LoadedPlugin, dir *os.Root) (map[string]LockEntry, LockEntry, bool) {
 	lock, err := ReadLock(plugin.Root)
 	if err != nil {
 		return nil, LockEntry{}, false
@@ -265,7 +320,7 @@ func cleanLockEntry(plugin LoadedPlugin) (map[string]LockEntry, LockEntry, bool)
 	if !ok {
 		return nil, LockEntry{}, false
 	}
-	current, err := hashTree(plugin.PluginDir)
+	current, err := hashRootedTree(dir)
 	if err != nil || current != entry.Hash {
 		return nil, LockEntry{}, false
 	}

internal/plugins/enabled_test.go

@@ -425,6 +425,79 @@ func TestSetEnabledRefusesASwappedPluginDirectory(t *testing.T) {
 	}
 }
 
+// TestSetEnabledDetectsAPluginDirectoryReplacedAfterOpening replaces the live
+// plugin directory after the toggle pinned it. The edit then lands in the
+// moved-away directory, so the toggle must not refresh the lockfile from either
+// tree and must not report success: the plugin installed at the path now is
+// unchanged.
+func TestSetEnabledDetectsAPluginDirectoryReplacedAfterOpening(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		pinned  bool // the pinned manifest's enabled value
+		changes bool // whether disabling it is a real edit
+	}{
+		{"edit", true, true},
+		{"no-op", false, false},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			root := t.TempDir()
+			manifestPath := writeRawManifest(t, root, "demo", demoManifest(tc.pinned))
+			pluginDir := filepath.Dir(manifestPath)
+			hash, err := hashTree(pluginDir)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if err := writeLock(root, map[string]LockEntry{"zero.demo": {Source: "./demo", Hash: hash}}); err != nil {
+				t.Fatal(err)
+			}
+			lockPath := filepath.Join(root, LockFileName)
+			lockBefore, err := os.ReadFile(lockPath)
+			if err != nil {
+				t.Fatal(err)
+			}
+			plugin, ok, err := FindByID(enableTestOptions(root), "zero.demo")
+			if err != nil || !ok {
+				t.Fatalf("FindByID: ok=%v err=%v", ok, err)
+			}
+			pinned, err := openPluginDir(plugin)
+			if err != nil {
+				t.Fatalf("openPluginDir: %v", err)
+			}
+			defer pinned.Close()
+
+			moved := filepath.Join(t.TempDir(), "moved-away")
+			if err := os.Rename(pluginDir, moved); err != nil {
+				t.Fatal(err)
+			}
+			replacement := strings.Replace(demoManifest(true), `"version": "1.0.0"`, `"version": "2.0.0"`, 1)
+			writeRawManifest(t, root, "demo", replacement)
+
+			// The lockfile check hashes the pinned tree, which still matches the
+			// recorded hash, not the replacement now at the path.
+			if _, _, clean := cleanLockEntry(plugin, pinned.dir); !clean {
+				t.Error("cleanLockEntry hashed the replacement at the path instead of the pinned directory")
+			}
+
+			if _, err := setEnabledPinned(plugin, pinned, false); !errors.Is(err, errPluginDirReplaced) {
+				t.Fatalf("setEnabledPinned after the directory was replaced: err = %v, want %v", err, errPluginDirReplaced)
+			}
+			if after, err := os.ReadFile(manifestPath); err != nil || string(after) != replacement {
+				t.Fatalf("the replacement manifest changed (err %v): %q", err, after)
+			}
+			if after, err := os.ReadFile(lockPath); err != nil || string(after) != string(lockBefore) {
+				t.Fatalf("the lockfile changed (err %v):\n got %s\nwant %s", err, after, lockBefore)
+			}
+			movedRaw, err := os.ReadFile(filepath.Join(moved, "plugin.json"))
+			if err != nil {
+				t.Fatal(err)
+			}
+			if tc.changes != (string(movedRaw) != demoManifest(tc.pinned)) {
+				t.Fatalf("pinned manifest edit = %v, want %v: %q", !tc.changes, tc.changes, movedRaw)
+			}
+		})
+	}
+}
+
 func TestSetEnabledByIDRespectsUserOnlyFilter(t *testing.T) {
 	dir := t.TempDir()
 	userRoot := filepath.Join(dir, "user")

internal/plugins/install.go

@@ -14,8 +14,10 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"os/exec"
+	"path"
 	"path/filepath"
 	"sort"
 	"strings"
@@ -567,15 +569,29 @@ func validInstallID(id string) bool {
 // sorted order. Each file contributes its plugin-relative path, executable bit,
 // and bytes, so renames, mode flips, and content edits all change the hash.
 func hashTree(root string) (string, error) {
+	handle, err := os.OpenRoot(root)
+	if err != nil {
+		return "", err
+	}
+	defer handle.Close()
+	return hashRootedTree(handle)
+}
+
+// hashRootedTree is hashTree over a directory that is already open. Walking
+// relative to the handle hashes the directory it names even if its path is
+// replaced meanwhile, and keeps the walk inside it.
+func hashRootedTree(handle *os.Root) (string, error) {
 	hasher := sha256.New()
-	if err := hashTreeInto(hasher, root, root); err != nil {
+	if err := hashTreeInto(hasher, handle.FS(), "."); err != nil {
 		return "", err
 	}
 	return "sha256:" + hex.EncodeToString(hasher.Sum(nil)), nil
 }
 
-func hashTreeInto(hasher io.Writer, root string, dir string) error {
-	entries, err := os.ReadDir(dir)
+// hashTreeInto hashes dir, a slash-separated path relative to fsys, into
+// hasher.
+func hashTreeInto(hasher io.Writer, fsys fs.FS, dir string) error {
+	entries, err := fs.ReadDir(fsys, dir)
 	if err != nil {
 		return err
 	}
@@ -588,8 +604,8 @@ func hashTreeInto(hasher io.Writer, root string, dir string) error {
 		if name == ".git" {
 			continue
 		}
-		path := filepath.Join(dir, name)
-		info, err := os.Lstat(path)
+		rel := path.Join(dir, name)
+		info, err := fs.Lstat(fsys, rel)
 		if err != nil {
 			return err
 		}
@@ -598,25 +614,21 @@ func hashTreeInto(hasher io.Writer, root string, dir string) error {
 			// Skipped by copyTree, so excluded from the hash too.
 			continue
 		case info.IsDir():
-			if err := hashTreeInto(hasher, root, path); err != nil {
+			if err := hashTreeInto(hasher, fsys, rel); err != nil {
 				return err
 			}
 		case info.Mode().IsRegular():
-			rel, err := filepath.Rel(root, path)
-			if err != nil {
-				return err
-			}
 			executable := 0
 			if info.Mode().Perm()&0o111 != 0 {
 				executable = 1
 			}
 			// Null-delimited header keeps file boundaries unambiguous (paths cannot
 			// contain null bytes) so two trees cannot collide by shifting bytes.
-			header := fmt.Sprintf("%s\x00%d\x00", filepath.ToSlash(rel), executable)
+			header := fmt.Sprintf("%s\x00%d\x00", rel, executable)
 			if _, err := io.WriteString(hasher, header); err != nil {
 				return err
 			}
-			file, err := os.Open(path)
+			file, err := fsys.Open(rel)
 			if err != nil {
 				return err
 			}