From b5499fe0af70caec0e0c6529bc5444fce876592b Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 13 Aug 2026 18:18:56 +0200 Subject: [PATCH 1/9] Record plugin installs in the project lock file Project-scope installs must pin plugins: in toolhive.lock.yaml, and a lock-write failure must roll back the install so nothing is left silently unpinned. Gated until sync, upgrade, and Sigstore land. Signed-off-by: Samuele Verzi --- pkg/plugins/feature_gate.go | 28 ++ pkg/plugins/feature_gate_test.go | 33 ++ pkg/plugins/options.go | 21 ++ pkg/plugins/pluginsvc/content_digest.go | 47 +++ pkg/plugins/pluginsvc/content_digest_test.go | 37 +++ pkg/plugins/pluginsvc/install.go | 101 ++++++- pkg/plugins/pluginsvc/install_extraction.go | 51 +++- pkg/plugins/pluginsvc/lock.go | 114 +++++++ pkg/plugins/pluginsvc/lock_test.go | 299 +++++++++++++++++++ pkg/plugins/pluginsvc/uninstall.go | 11 + 10 files changed, 726 insertions(+), 16 deletions(-) create mode 100644 pkg/plugins/feature_gate.go create mode 100644 pkg/plugins/feature_gate_test.go create mode 100644 pkg/plugins/pluginsvc/content_digest.go create mode 100644 pkg/plugins/pluginsvc/content_digest_test.go create mode 100644 pkg/plugins/pluginsvc/lock.go create mode 100644 pkg/plugins/pluginsvc/lock_test.go diff --git a/pkg/plugins/feature_gate.go b/pkg/plugins/feature_gate.go new file mode 100644 index 0000000000..940a94f1c0 --- /dev/null +++ b/pkg/plugins/feature_gate.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package plugins + +import ( + "os" + "strings" +) + +// LockFileEnvVar gates the project-level plugins lock file feature while it +// lands across multiple PRs. The feature is inert on main until every PR in +// the stack — lock file, sync, upgrade, and Sigstore signing/verification — +// has merged; this keeps each PR mergeable on its own without exposing +// partial, unsigned-by-default plugin entries in the live skills trust +// document (toolhive.lock.yaml). +// +// This is intentionally a plain env var, not persisted config or a CLI flag: +// it exists only for the duration of the rollout and is expected to be +// removed once the feature ships, matching the TOOLHIVE_DEV precedent for +// staged/dev-only behavior. +const LockFileEnvVar = "TOOLHIVE_PLUGINS_LOCK_ENABLED" + +// LockFileFeatureEnabled reports whether the project-level plugins lock file +// feature is enabled for this process. +func LockFileFeatureEnabled() bool { + return strings.EqualFold(os.Getenv(LockFileEnvVar), "true") +} diff --git a/pkg/plugins/feature_gate_test.go b/pkg/plugins/feature_gate_test.go new file mode 100644 index 0000000000..07e28b2880 --- /dev/null +++ b/pkg/plugins/feature_gate_test.go @@ -0,0 +1,33 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package plugins + +import "testing" + +func TestLockFileFeatureEnabled(t *testing.T) { + tests := []struct { + name string + value string + want bool + }{ + {name: "unset defaults to disabled", value: "", want: false}, + {name: "true enables", value: "true", want: true}, + {name: "mixed case true enables", value: "True", want: true}, + {name: "false stays disabled", value: "false", want: false}, + {name: "arbitrary value stays disabled", value: "1", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if tt.value == "" { + t.Setenv(LockFileEnvVar, "") + } else { + t.Setenv(LockFileEnvVar, tt.value) + } + if got := LockFileFeatureEnabled(); got != tt.want { + t.Errorf("LockFileFeatureEnabled() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index 0d9938d41b..1eb61c75a9 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -44,12 +44,33 @@ type InstallOptions struct { // Description is the plugin description, hydrated from the OCI artifact // config or git manifest. Internal use only. Description string `json:"-"` + // LockSource overrides the value recorded as the lock entry's Source. When + // empty, the entry's Source is Name as given by the caller before any + // internal resolution. Set by Sync/Upgrade, which pass an already-resolved + // Name that must not overwrite the entry's original Source. Internal use + // only — NOT exposed via HTTP API. + LockSource string `json:"-"` + // LockResolvedReference overrides the value recorded as the lock entry's + // ResolvedReference. When empty, the entry's ResolvedReference is + // whatever this install actually resolved to. Set by Sync when + // reinstalling at a pinned reference. Internal use only — NOT exposed + // via HTTP API. + LockResolvedReference string `json:"-"` } // InstallResult contains the outcome of an Install operation. type InstallResult struct { // Plugin is the installed plugin. Plugin InstalledPlugin `json:"plugin"` + // PreExisting is the store record as it was before this install, or nil + // when this install created the record. Rollback uses it to restore the + // previous state instead of destructively deleting a record this call + // did not create. Internal use only — NOT exposed via HTTP API. + PreExisting *InstalledPlugin `json:"-"` + // ContentDigest is the dirhash of the canonical plugin tree (ExtractPlugin + // output), computed at install time for recording in the lock file. + // Internal use only — NOT exposed via HTTP API. + ContentDigest string `json:"-"` } // UninstallOptions configures the behavior of the Uninstall operation. Alias diff --git a/pkg/plugins/pluginsvc/content_digest.go b/pkg/plugins/pluginsvc/content_digest.go new file mode 100644 index 0000000000..d6e687675e --- /dev/null +++ b/pkg/plugins/pluginsvc/content_digest.go @@ -0,0 +1,47 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "fmt" + + ociskills "github.com/stacklok/toolhive-core/oci/skills" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// computeContentDigest hashes the canonical plugin tree (the file set +// ExtractPlugin writes), for recording in the lock file at install time. +// The hash is taken from the decompressed layer in memory rather than a +// client adapter's on-disk directory, so marketplace.json / settings.json +// mutations are not part of the pin. Sync --check hashes the ExtractPlugin +// destination, which is the same file set. +func computeContentDigest(layerData []byte) (string, error) { + if len(layerData) == 0 { + return "", fmt.Errorf("plugin layer data is empty") + } + + tarData, err := ociskills.DecompressWithLimit(layerData, skills.MaxTotalExtractSize) + if err != nil { + return "", fmt.Errorf("decompressing plugin layer: %w", err) + } + files, err := ociskills.ExtractTarWithLimit(tarData, skills.MaxFileExtractSize) + if err != nil { + return "", fmt.Errorf("extracting plugin tree: %w", err) + } + if len(files) > skills.MaxExtractFileCount { + return "", fmt.Errorf("archive contains %d files, exceeding limit of %d", + len(files), skills.MaxExtractFileCount) + } + + contentFiles := make([]lockfile.ContentFile, 0, len(files)) + for _, f := range files { + contentFiles = append(contentFiles, lockfile.ContentFile{Path: f.Path, Content: f.Content}) + } + digest, err := lockfile.ContentDigest(contentFiles) + if err != nil { + return "", fmt.Errorf("computing content digest: %w", err) + } + return digest, nil +} diff --git a/pkg/plugins/pluginsvc/content_digest_test.go b/pkg/plugins/pluginsvc/content_digest_test.go new file mode 100644 index 0000000000..fceb47ee9f --- /dev/null +++ b/pkg/plugins/pluginsvc/content_digest_test.go @@ -0,0 +1,37 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +func TestComputeContentDigest_MatchesExtractPluginTree(t *testing.T) { + t.Parallel() + + layer := makePluginLayerData(t, "digest-plugin") + got, err := computeContentDigest(layer) + require.NoError(t, err) + + dir := filepath.Join(makeProjectRoot(t), "extracted") + _, err = skills.ExtractPlugin(layer, dir, true) + require.NoError(t, err) + want, err := lockfile.ContentDigestFromDir(dir) + require.NoError(t, err) + assert.Equal(t, want, got, "in-memory layer hash must match ExtractPlugin's on-disk tree") +} + +func TestComputeContentDigest_EmptyLayer(t *testing.T) { + t.Parallel() + _, err := computeContentDigest(nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index e660060c96..6d984d56b5 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -14,6 +14,7 @@ import ( "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) // Install installs a plugin. When the Name field contains a git reference @@ -28,6 +29,9 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl } scope = defaultScope(scope) opts.ProjectRoot = projectRoot + if opts.LockSource == "" { + opts.LockSource = opts.Name + } // Git references are dispatched first; the prefix is unambiguous and // cannot collide with OCI references. @@ -36,7 +40,7 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl if err != nil { return nil, err } - return s.installAndRegister(ctx, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, result, scope) } // Splice opts.Version as the tag for tag-less OCI-like references. @@ -56,7 +60,7 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl if isOCI { result, ociErr := s.installFromOCI(ctx, opts, scope, ref) if ociErr == nil { - return s.installAndRegister(ctx, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, result, scope) } // No registry-name fallback yet (Phase-3 later wave); surface the // OCI pull error directly. @@ -110,7 +114,7 @@ func (s *service) installByName( if err != nil { return nil, err } - return s.installAndRegister(ctx, result, opts.Group, opts.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, result, scope) } // installFromRegistryLookup resolves a plain plugin name via the registry @@ -230,7 +234,7 @@ func (s *service) installFromRegistryHit( if ociErr != nil { return nil, ociErr } - return s.installAndRegister(ctx, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) + return s.installAndRegister(ctx, opts, result, scope) } // selectOCIPluginPackage selects the first OCI package from a registry entry's @@ -294,23 +298,90 @@ func (s *service) registerPluginInGroup(ctx context.Context, groupName string, p return groups.AddPluginToGroup(ctx, s.groupManager, groupName, pluginName) } -// installAndRegister registers the just-installed plugin in the target group. -// If group registration fails, the DB record is rolled back so a retry starts -// fresh. Mirror of skillsvc.installAndRegister. +// installAndRegister registers the just-installed plugin in the target group +// and, for project-scope installs with the lock file feature enabled (see +// plugins.LockFileFeatureEnabled), records it in the project's +// toolhive.lock.yaml plugins: key. If group registration or the lock write +// fails, the DB record and lock entry are rolled back to their pre-install +// state: restored when this call updated a pre-existing record (a --force +// reinstall must not be destroyed by a transient failure), deleted when this +// call created them. func (s *service) installAndRegister( ctx context.Context, + opts plugins.InstallOptions, result *plugins.InstallResult, - groupName string, - pluginName string, scope plugins.Scope, - projectRoot string, ) (*plugins.InstallResult, error) { - if err := s.registerPluginInGroup(ctx, groupName, pluginName); err != nil { - // Best-effort rollback: remove the DB record so retries start fresh. - // Materialized files are left in place; a fresh install will overwrite - // them (the adapters are idempotent under the same name/scope). - _ = s.store.Delete(ctx, pluginName, scope, projectRoot) + pluginName := result.Plugin.Metadata.Name + lockScoped := scope == plugins.ScopeProject && plugins.LockFileFeatureEnabled() + + // Snapshot the prior plugins: lock entry before anything below can write + // one, so rollback can reinstate it rather than blindly deleting it. + var prevEntry *lockfile.Entry + if lockScoped { + if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { + if lf, loadErr := lockfile.Load(root); loadErr == nil { + if e, ok := lf.GetPlugin(pluginName); ok { + prevEntry = &e + } + } + } + } + + rollback := func() { s.rollbackInstall(ctx, opts, result, pluginName, scope, lockScoped, prevEntry) } + + if err := s.registerPluginInGroup(ctx, opts.Group, pluginName); err != nil { + // Best-effort rollback. Files on disk are left in place; a fresh + // install will overwrite them (the adapters are idempotent under the + // same name/scope). + rollback() return nil, fmt.Errorf("registering plugin in group: %w", err) } + + if lockScoped { + updated, err := s.recordLockState(ctx, opts, result.Plugin, result.ContentDigest) + if err != nil { + rollback() + return nil, httperr.WithCode( + fmt.Errorf("recording plugin in project lock file: %w", err), + http.StatusInternalServerError, + ) + } + result.Plugin = updated + } + return result, nil } + +// rollbackInstall undoes installAndRegister's side effects after a failure, +// best-effort. The DB record is restored to its pre-install snapshot when +// one exists (result.PreExisting) and deleted otherwise; the lock entry is +// likewise reinstated from prevEntry or removed. +func (s *service) rollbackInstall( + ctx context.Context, + opts plugins.InstallOptions, + result *plugins.InstallResult, + pluginName string, + scope plugins.Scope, + lockScoped bool, + prevEntry *lockfile.Entry, +) { + if result.PreExisting != nil { + _ = s.store.Update(ctx, *result.PreExisting) + } else { + _ = s.store.Delete(ctx, pluginName, scope, opts.ProjectRoot) + } + + if !lockScoped { + return + } + if prevEntry != nil { + if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { + _ = lockfile.UpsertPluginEntry(root, *prevEntry) + } + return + } + _ = removeLockEntry(plugins.UninstallOptions{ + Name: pluginName, Scope: scope, ProjectRoot: opts.ProjectRoot, + }) +} diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 0e73395dc6..2fc63c67ea 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -40,6 +40,35 @@ func (s *service) installWithExtraction( return nil, fmt.Errorf("checking existing plugin: %w", storeErr) } + contentDigest, err := lockContentDigest(opts, scope) + if err != nil { + return nil, err + } + + result, err := s.dispatchExtraction(ctx, opts, scope, existing, storeErr, clientTypes) + if err != nil { + return nil, err + } + if storeErr == nil { + // Preserve the pre-install record so a later rollback (e.g. a failed + // lock write) can restore it rather than delete it. + pre := existing + result.PreExisting = &pre + } + result.ContentDigest = contentDigest + return result, nil +} + +// dispatchExtraction routes an extraction-based install to the no-op, +// same-digest, upgrade, or fresh path based on the pre-install store state. +func (s *service) dispatchExtraction( + ctx context.Context, + opts plugins.InstallOptions, + scope plugins.Scope, + existing plugins.InstalledPlugin, + storeErr error, + clientTypes []string, +) (*plugins.InstallResult, error) { if isExtractionNoOp(existing, storeErr, opts, clientTypes) { return &plugins.InstallResult{Plugin: existing}, nil } @@ -56,7 +85,8 @@ func (s *service) installWithExtraction( // isExtractionNoOp reports whether the install can be short-circuited because // the same digest and all requested clients are already present. Mirror of -// skillsvc.isExtractionNoOp. +// skillsvc.isExtractionNoOp. Sync (a later PR) will need a SyncRestore bypass +// so a lock-driven reinstall can repair on-disk drift at the same digest. func isExtractionNoOp(existing plugins.InstalledPlugin, storeErr error, opts plugins.InstallOptions, clientTypes []string) bool { if storeErr != nil || existing.Digest != opts.Digest { return false @@ -85,6 +115,7 @@ func (s *service) installExtractionSameDigestNewClients( return nil, err } pl := buildInstalledPlugin(opts, scope, clientTypes, existing.Clients) + pl.Managed = existing.Managed if err := s.store.Update(ctx, pl); err != nil { s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) return nil, err @@ -108,6 +139,7 @@ func (s *service) installExtractionUpgradeDigest( return nil, err } pl := buildInstalledPlugin(opts, scope, allClients, nil) + pl.Managed = existing.Managed if err := s.store.Update(ctx, pl); err != nil { s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) return nil, err @@ -354,3 +386,20 @@ func missingClients(existing, requested []string) []string { } return out } + +// lockContentDigest computes the canonical-tree dirhash for a project-scope +// install when the lock file feature is enabled. Empty when the install is +// not lock-scoped, so user-scope and ungated installs skip the extra extract. +func lockContentDigest(opts plugins.InstallOptions, scope plugins.Scope) (string, error) { + if scope != plugins.ScopeProject || !plugins.LockFileFeatureEnabled() { + return "", nil + } + digest, err := computeContentDigest(opts.LayerData) + if err != nil { + return "", httperr.WithCode( + fmt.Errorf("computing content digest: %w", err), + http.StatusInternalServerError, + ) + } + return digest, nil +} diff --git a/pkg/plugins/pluginsvc/lock.go b/pkg/plugins/pluginsvc/lock.go new file mode 100644 index 0000000000..1c0c417525 --- /dev/null +++ b/pkg/plugins/pluginsvc/lock.go @@ -0,0 +1,114 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "fmt" + + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// recordLockState updates opts.ProjectRoot's lock file to reflect a +// just-completed project-scope install: a plugins: entry for pl. It also +// marks pl as lock-managed in the store. Callers must only invoke this for +// project-scope installs with the lock file feature enabled (see +// plugins.LockFileFeatureEnabled) — pl is returned updated so the caller +// can reflect the Managed flag back to its own result. +// +// Plugin requires is parsed today but not materialized; requiredBy/explicit +// stay unused until that Phase-3 wave. Every recorded plugin install is +// treated as explicit. +func (s *service) recordLockState( + ctx context.Context, + opts plugins.InstallOptions, + pl plugins.InstalledPlugin, + contentDigest string, +) (plugins.InstalledPlugin, error) { + if contentDigest == "" { + var err error + contentDigest, err = computeContentDigest(opts.LayerData) + if err != nil { + return pl, fmt.Errorf("computing content digest: %w", err) + } + } + + source := opts.LockSource + if source == "" { + source = opts.Name + } + resolvedReference := opts.LockResolvedReference + if resolvedReference == "" { + resolvedReference = pl.Reference + } + if err := recordLockEntry(pl.ProjectRoot, lockEntryInput{ + Name: pl.Metadata.Name, + Version: pl.Metadata.Version, + Source: source, + ResolvedReference: resolvedReference, + Digest: pl.Digest, + ContentDigest: contentDigest, + }); err != nil { + return pl, fmt.Errorf("writing lock entry: %w", err) + } + + if !pl.Managed { + pl.Managed = true + if err := s.store.Update(ctx, pl); err != nil { + return pl, fmt.Errorf("marking plugin as lock-managed: %w", err) + } + } + return pl, nil +} + +// lockEntryInput carries the fields recordLockEntry needs to upsert a +// plugins: lock entry, decoupled from pluginsvc's own InstallOptions shape. +type lockEntryInput struct { + Name string + Version string + Source string + ResolvedReference string + Digest string + ContentDigest string +} + +// recordLockEntry upserts a single plugins: entry into projectRoot's lock +// file. When an entry for the same name already exists, Explicit is sticky +// once true so a later reinstall cannot demote it. requiredBy is preserved +// verbatim (v1 does not materialize plugin requires). +func recordLockEntry(projectRoot string, in lockEntryInput) error { + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return err + } + return lockfile.Update(root, func(lf *lockfile.Lockfile) error { + entry := lockfile.Entry{ + Name: in.Name, + Version: in.Version, + Source: in.Source, + ResolvedReference: in.ResolvedReference, + Digest: in.Digest, + ContentDigest: in.ContentDigest, + Explicit: true, + } + existing, exists := lf.GetPlugin(in.Name) + if exists { + entry.RequiredBy = existing.RequiredBy + entry.Explicit = entry.Explicit || existing.Explicit + } + lf.UpsertPlugin(entry) + return nil + }) +} + +// removeLockEntry removes opts.Name's plugins: lock entry. Unlike skills, +// plugin uninstall does not cascade: requires is not materialized in v1. +func removeLockEntry(opts plugins.UninstallOptions) error { + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return err + } + return lockfile.RemovePluginEntry(root, opts.Name) +} diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go new file mode 100644 index 0000000000..546c7747ae --- /dev/null +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -0,0 +1,299 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage/sqlite" +) + +// extractingAdapter materializes by ExtractPlugin into /, matching +// the canonical plugin tree contentDigest hashes (not marketplace.json). +type extractingAdapter struct { + base string + installer skills.Installer +} + +func (a *extractingAdapter) Materialize(_ context.Context, req plugins.MaterializeRequest) (*plugins.MaterializeResult, error) { + dir := filepath.Join(a.base, req.Name) + if _, err := a.installer.ExtractPlugin(req.LayerData, dir, true); err != nil { + return nil, err + } + return &plugins.MaterializeResult{ + InstallPath: dir, + InstalledComponents: []plugins.ComponentType{plugins.ComponentCommands}, + }, nil +} + +func (a *extractingAdapter) Dematerialize(_ context.Context, req plugins.DematerializeRequest) error { + return a.installer.Remove(filepath.Join(a.base, req.Name)) +} + +func (*extractingAdapter) SupportedComponents() []plugins.ComponentType { + return []plugins.ComponentType{plugins.ComponentCommands} +} + +func (*extractingAdapter) ScopeSupport() plugins.ScopeSupport { + return plugins.ScopeSupport{} +} + +func newLockTestService(t *testing.T, enableGate bool) (plugins.PluginService, string) { + t.Helper() + if enableGate { + t.Setenv(plugins.LockFileEnvVar, "true") + } else { + t.Setenv(plugins.LockFileEnvVar, "") + } + + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := sqlite.Open(t.Context(), dbPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + projectRoot := makeProjectRoot(t) + adapter := &extractingAdapter{ + base: filepath.Join(projectRoot, ".claude", "plugins"), + installer: skills.NewInstaller(), + } + svc := New( + WithStore(sqlite.NewPluginStore(db)), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), + ) + return svc, projectRoot +} + +func mustOpenRoot(t *testing.T, projectRoot string) lockfile.Root { + t.Helper() + root, err := lockfile.OpenRoot(projectRoot) + require.NoError(t, err) + return root +} + +func readLockfile(t *testing.T, projectRoot string) *lockfile.Lockfile { + t.Helper() + lf, err := lockfile.Load(mustOpenRoot(t, projectRoot)) + require.NoError(t, err) + return lf +} + +func validLockDigest() string { + return "sha256:" + "abababababababababababababababababababababababababababababababab" +} + +func validLockDigestAlt() string { + return "sha256:" + "cdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcdcd" +} + +func installTestPlugin(t *testing.T, svc plugins.PluginService, projectRoot, digest string) *plugins.InstallResult { + t.Helper() + const name = "my-plugin" + result, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: name, + LayerData: makePluginLayerData(t, name), + Digest: digest, + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + return result +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_RecordsExplicitEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + result := installTestPlugin(t, svc, projectRoot, validLockDigest()) + assert.True(t, result.Plugin.Managed, "project-scope install must be marked lock-managed") + assert.NotEmpty(t, result.ContentDigest) + + lf := readLockfile(t, projectRoot) + entry, ok := lf.GetPlugin("my-plugin") + require.True(t, ok, "expected a plugins: lock entry for my-plugin") + assert.Equal(t, "my-plugin", entry.Source, "source must be exactly what the caller requested") + assert.Equal(t, validLockDigest(), entry.Digest) + assert.Equal(t, result.ContentDigest, entry.ContentDigest) + assert.True(t, entry.Explicit) + assert.Empty(t, entry.RequiredBy) + assert.Empty(t, lf.Skills, "plugin install must not write a skills: entry") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_DisabledGateDoesNotWriteLock(t *testing.T) { + svc, projectRoot := newLockTestService(t, false) + + result := installTestPlugin(t, svc, projectRoot, validLockDigest()) + assert.False(t, result.Plugin.Managed) + + _, err := os.Stat(filepath.Join(projectRoot, lockfile.FileName)) + assert.True(t, os.IsNotExist(err), "lock file must not be written when the feature is disabled") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallUserScope_DoesNotWriteLock(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + result, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeUser, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + assert.False(t, result.Plugin.Managed) + + _, err = os.Stat(filepath.Join(projectRoot, lockfile.FileName)) + assert.True(t, os.IsNotExist(err), "user-scope install must not write a lock file") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_PreservesExistingSkillsKey(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + require.NoError(t, lockfile.UpsertEntry(mustOpenRoot(t, projectRoot), lockfile.Entry{ + Name: "code-review", + Source: "code-review", + Digest: validLockDigest(), + })) + + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + lf := readLockfile(t, projectRoot) + _, ok := lf.Get("code-review") + assert.True(t, ok, "a plugin install must not drop existing skills: entries") + _, ok = lf.GetPlugin("my-plugin") + assert.True(t, ok) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err, "install must fail when the lock file cannot be written") + assert.Equal(t, http.StatusInternalServerError, httperr.Code(err)) + + _, err = svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "the DB record must be rolled back so a retry starts fresh") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + first := installTestPlugin(t, svc, projectRoot, validLockDigest()) + before, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + require.True(t, ok) + + require.NoError(t, os.Chmod(projectRoot, 0o555)) + t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err, "reinstall must fail when the lock file cannot be written") + + require.NoError(t, os.Chmod(projectRoot, 0o755)) + after, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + require.True(t, ok, "a transient failure must not destroy the pre-existing lock entry") + assert.Equal(t, before.Digest, after.Digest, "the previous pin must be restored") + + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the pre-existing DB record must survive") + assert.Equal(t, first.Plugin.Digest, info.InstalledPlugin.Digest) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_RemovesPluginLockEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.False(t, ok, "uninstall must remove the plugins: entry") + + _, err = svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_LockWriteFailureAbortsBeforeDestruction(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, os.Chmod(projectRoot, 0o555)) + t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "uninstall must fail when the lock entry cannot be removed") + + require.NoError(t, os.Chmod(projectRoot, 0o755)) + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the plugin must remain fully installed") + assert.NotNil(t, info.InstalledPlugin) + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.True(t, ok, "the lock entry must be untouched") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_DoesNotTouchSkillsKey(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + require.NoError(t, lockfile.UpsertEntry(mustOpenRoot(t, projectRoot), lockfile.Entry{ + Name: "code-review", + Source: "code-review", + Digest: validLockDigest(), + })) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + })) + + lf := readLockfile(t, projectRoot) + _, ok := lf.Get("code-review") + assert.True(t, ok, "uninstalling a plugin must not drop skills: entries") + _, ok = lf.GetPlugin("my-plugin") + assert.False(t, ok) +} diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 1ea8609fa3..089e0cb872 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -45,6 +45,17 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return err } + // The lock entry is removed FIRST, and its failure aborts the uninstall + // while everything is still intact. The reverse order (files/DB first, + // lock entry best-effort) had a resurrection hazard: a lock-write + // failure after the record and files were gone left a stale entry that + // the next sync silently reinstalled. + if scope == plugins.ScopeProject && existing.Managed { + if lockErr := removeLockEntry(opts); lockErr != nil { + return fmt.Errorf("updating project lock file: %w", lockErr) + } + } + // Dematerialize for each client — best-effort. var cleanupErrs []error for _, clientType := range existing.Clients { From 4b92bbcfb7a21aa5e458d0da19852649cacd17c7 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 10:03:40 +0200 Subject: [PATCH 2/9] Roll back plugin files after lock-write failure Hold the per-plugin lock across materialize, DB, group, and lock-file writes so uninstall cannot race. Restore on-disk trees and lock entries when a later step fails, matching the AC that rollback undoes DB and dematerialization together. Signed-off-by: Samuele Verzi --- pkg/plugins/options.go | 10 +- pkg/plugins/pluginsvc/install.go | 44 +++---- pkg/plugins/pluginsvc/install_extraction.go | 130 +++++++++++++++++++- pkg/plugins/pluginsvc/install_git.go | 12 +- pkg/plugins/pluginsvc/install_oci.go | 14 ++- pkg/plugins/pluginsvc/install_test.go | 7 +- pkg/plugins/pluginsvc/lock_test.go | 95 +++++++++++++- pkg/plugins/pluginsvc/uninstall.go | 103 +++++++++++----- 8 files changed, 342 insertions(+), 73 deletions(-) diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index 1eb61c75a9..1642865eb0 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -3,7 +3,11 @@ package plugins -import "github.com/stacklok/toolhive/pkg/skills" +import ( + "context" + + "github.com/stacklok/toolhive/pkg/skills" +) // ListOptions configures the behavior of the List operation. Alias for // skills.ListOptions (identical shape: Scope, ClientApp, ProjectRoot, Group). @@ -71,6 +75,10 @@ type InstallResult struct { // output), computed at install time for recording in the lock file. // Internal use only — NOT exposed via HTTP API. ContentDigest string `json:"-"` + // RestoreFiles undoes this install's on-disk writes: dematerialize a + // fresh install, or restore the previous tree after a failed upgrade. + // Internal use only — NOT exposed via HTTP API. + RestoreFiles func(context.Context) `json:"-"` } // UninstallOptions configures the behavior of the Uninstall operation. Alias diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index 6d984d56b5..99e5c70fff 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -34,13 +34,10 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl } // Git references are dispatched first; the prefix is unambiguous and - // cannot collide with OCI references. + // cannot collide with OCI references. installFromGit holds the per-plugin + // lock across extraction, DB, group, lock-file, and rollback. if gitresolver.IsGitReference(opts.Name) { - result, err := s.installFromGit(ctx, opts, scope) - if err != nil { - return nil, err - } - return s.installAndRegister(ctx, opts, result, scope) + return s.installFromGit(ctx, opts, scope) } // Splice opts.Version as the tag for tag-less OCI-like references. @@ -58,13 +55,9 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl ) } if isOCI { - result, ociErr := s.installFromOCI(ctx, opts, scope, ref) - if ociErr == nil { - return s.installAndRegister(ctx, opts, result, scope) - } - // No registry-name fallback yet (Phase-3 later wave); surface the - // OCI pull error directly. - return nil, ociErr + // installFromOCI holds the per-plugin lock across extraction, DB, + // group, lock-file, and rollback. + return s.installFromOCI(ctx, opts, scope, ref) } // Plain plugin name. @@ -230,11 +223,7 @@ func (s *service) installFromRegistryHit( http.StatusUnprocessableEntity, ) } - result, ociErr := s.installFromOCI(ctx, opts, scope, ref) - if ociErr != nil { - return nil, ociErr - } - return s.installAndRegister(ctx, opts, result, scope) + return s.installFromOCI(ctx, opts, scope, ref) } // selectOCIPluginPackage selects the first OCI package from a registry entry's @@ -302,10 +291,11 @@ func (s *service) registerPluginInGroup(ctx context.Context, groupName string, p // and, for project-scope installs with the lock file feature enabled (see // plugins.LockFileFeatureEnabled), records it in the project's // toolhive.lock.yaml plugins: key. If group registration or the lock write -// fails, the DB record and lock entry are rolled back to their pre-install -// state: restored when this call updated a pre-existing record (a --force -// reinstall must not be destroyed by a transient failure), deleted when this -// call created them. +// fails, the DB record, on-disk files, and lock entry are rolled back to +// their pre-install state: restored when this call updated a pre-existing +// record (a --force reinstall must not be destroyed by a transient failure), +// deleted/dematerialized when this call created them. Callers must hold the +// per-plugin lock for the duration of this call. func (s *service) installAndRegister( ctx context.Context, opts plugins.InstallOptions, @@ -331,9 +321,6 @@ func (s *service) installAndRegister( rollback := func() { s.rollbackInstall(ctx, opts, result, pluginName, scope, lockScoped, prevEntry) } if err := s.registerPluginInGroup(ctx, opts.Group, pluginName); err != nil { - // Best-effort rollback. Files on disk are left in place; a fresh - // install will overwrite them (the adapters are idempotent under the - // same name/scope). rollback() return nil, fmt.Errorf("registering plugin in group: %w", err) } @@ -355,7 +342,8 @@ func (s *service) installAndRegister( // rollbackInstall undoes installAndRegister's side effects after a failure, // best-effort. The DB record is restored to its pre-install snapshot when -// one exists (result.PreExisting) and deleted otherwise; the lock entry is +// one exists (result.PreExisting) and deleted otherwise; on-disk files are +// dematerialized (fresh install) or restored (upgrade); the lock entry is // likewise reinstated from prevEntry or removed. func (s *service) rollbackInstall( ctx context.Context, @@ -372,6 +360,10 @@ func (s *service) rollbackInstall( _ = s.store.Delete(ctx, pluginName, scope, opts.ProjectRoot) } + if result.RestoreFiles != nil { + result.RestoreFiles(ctx) + } + if !lockScoped { return } diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 2fc63c67ea..02ceeba2c5 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -7,7 +7,10 @@ import ( "context" "errors" "fmt" + "io/fs" "net/http" + "os" + "path/filepath" "slices" "strings" "time" @@ -120,7 +123,12 @@ func (s *service) installExtractionSameDigestNewClients( s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) return nil, err } - return &plugins.InstallResult{Plugin: pl}, nil + return &plugins.InstallResult{ + Plugin: pl, + RestoreFiles: func(ctx context.Context) { + s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + }, + }, nil } // installExtractionUpgradeDigest re-materializes the plugin for the union of @@ -134,17 +142,23 @@ func (s *service) installExtractionUpgradeDigest( clientTypes []string, ) (*plugins.InstallResult, error) { allClients := mergeClientLists(existing.Clients, clientTypes) - materialized, err := s.materializeForClients(ctx, opts, scope, allClients) - if err != nil { + backups := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, allClients) + if _, err := s.materializeForClients(ctx, opts, scope, allClients); err != nil { + s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) return nil, err } pl := buildInstalledPlugin(opts, scope, allClients, nil) pl.Managed = existing.Managed if err := s.store.Update(ctx, pl); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) return nil, err } - return &plugins.InstallResult{Plugin: pl}, nil + return &plugins.InstallResult{ + Plugin: pl, + RestoreFiles: func(ctx context.Context) { + s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) + }, + }, nil } // installExtractionFresh materializes the plugin for all requested clients, @@ -164,7 +178,12 @@ func (s *service) installExtractionFresh( s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) return nil, err } - return &plugins.InstallResult{Plugin: pl}, nil + return &plugins.InstallResult{ + Plugin: pl, + RestoreFiles: func(ctx context.Context) { + s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + }, + }, nil } // materializeForClients calls Materialize for each requested client type, @@ -201,6 +220,105 @@ func (s *service) materializeForClients( return materialized, nil } +// snapshotClientTrees copies each client's installed plugin tree into memory +// so a later rollback can restore the previous materialization without +// leaking temp directories. Missing directories are omitted (the client was +// not yet installed). +func (s *service) snapshotClientTrees( + name string, scope plugins.Scope, projectRoot string, clientTypes []string, +) map[string]map[string][]byte { + backups := make(map[string]map[string][]byte, len(clientTypes)) + for _, ct := range clientTypes { + dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) + if err != nil { + continue + } + files, ok := snapshotDir(dir) + if !ok { + continue + } + backups[ct] = files + } + if len(backups) == 0 { + return nil + } + return backups +} + +// restoreClientTrees writes snapshotClientTrees backups back over the live +// plugin directories. Clients without a backup are dematerialized (they were +// newly added by the failed install). +func (s *service) restoreClientTrees( + ctx context.Context, + name string, + scope plugins.Scope, + projectRoot string, + backups map[string]map[string][]byte, + allClients []string, +) { + restored := make(map[string]struct{}, len(backups)) + for ct, files := range backups { + restored[ct] = struct{}{} + dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) + if err != nil { + continue + } + restoreDir(dir, files) + } + var extra []string + for _, ct := range allClients { + if _, ok := restored[ct]; !ok { + extra = append(extra, ct) + } + } + s.dematerializeAll(ctx, extra, name, scope, projectRoot) +} + +// snapshotDir reads every regular file under dir into a relative-path map. +// Returns ok=false when dir does not exist. +func snapshotDir(dir string) (map[string][]byte, bool) { + if _, err := os.Stat(dir); err != nil { + return nil, false + } + files := make(map[string][]byte) + _ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return err + } + rel, relErr := filepath.Rel(dir, path) + if relErr != nil { + return relErr + } + data, readErr := os.ReadFile(path) //nolint:gosec // path is under a GetPluginPath-validated directory + if readErr != nil { + return readErr + } + files[rel] = data + return nil + }) + return files, true +} + +// restoreDir replaces dir with the files captured by snapshotDir. +func restoreDir(dir string, files map[string][]byte) { + _ = os.RemoveAll(dir) + for rel, data := range files { + path := filepath.Join(dir, rel) + if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + continue + } + _ = os.WriteFile(path, data, 0o600) + } +} + +// pluginInstallPath resolves the on-disk plugin directory for a client. +func (s *service) pluginInstallPath(clientType, name string, scope plugins.Scope, projectRoot string) (string, error) { + if s.clientManager == nil { + return "", errors.New("client manager is not configured") + } + return s.clientManager.GetPluginPath(client.ClientApp(clientType), name, scope, projectRoot) +} + // dematerializeAll best-effort reverts materializations performed in this call. // Errors are joined so a partial rollback still surfaces; the original install // error is returned to the caller separately. diff --git a/pkg/plugins/pluginsvc/install_git.go b/pkg/plugins/pluginsvc/install_git.go index 1048c26f08..2a56b5e96b 100644 --- a/pkg/plugins/pluginsvc/install_git.go +++ b/pkg/plugins/pluginsvc/install_git.go @@ -19,9 +19,9 @@ import ( ) // installFromGit clones a git repository, reads the plugin manifest, collects -// the plugin file tree, builds an in-memory tar.gz layer, and delegates to -// installWithExtraction. The digest is the git commit hash, enabling same-commit -// no-op and upgrade detection. +// the plugin file tree, builds an in-memory tar.gz layer, then materializes +// and registers the plugin while holding the per-plugin lock. The digest is +// the git commit hash, enabling same-commit no-op and upgrade detection. // // Unlike skillsvc.installFromGit, this does NOT call gitResolver.Resolve // (which is skill-specific: it reads SKILL.md). Instead it replicates the @@ -94,7 +94,11 @@ func (s *service) installFromGit( unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() - return s.installWithExtraction(ctx, opts, scope) + result, err := s.installWithExtraction(ctx, opts, scope) + if err != nil { + return nil, err + } + return s.installAndRegister(ctx, opts, result, scope) } // cloneAndCollectPlugin clones the repo referenced by gitRef, reads the plugin diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index 98bcb81e1f..f2204a0d98 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -19,10 +19,10 @@ import ( ) // installFromOCI pulls a plugin artifact from a remote registry, extracts -// metadata and layer data, then delegates to installWithExtraction. Mirror of -// skillsvc.installFromOCI, substituting the plugin supply-chain check -// (config.Name == OCI repo last segment) and hydrating Components/Dependencies -// from the plugin OCI config. +// metadata and layer data, then materializes and registers the plugin while +// holding the per-plugin lock. Mirror of skillsvc.installFromOCI, substituting +// the plugin supply-chain check (config.Name == OCI repo last segment) and +// hydrating Components/Dependencies from the plugin OCI config. func (s *service) installFromOCI( ctx context.Context, opts plugins.InstallOptions, @@ -110,7 +110,11 @@ func (s *service) installFromOCI( unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() - return s.installWithExtraction(ctx, opts, scope) + result, err := s.installWithExtraction(ctx, opts, scope) + if err != nil { + return nil, err + } + return s.installAndRegister(ctx, opts, result, scope) } // requiresToDependencies maps a plugin's declared `requires` OCI references diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index 250ec5d018..363c8128d1 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -28,10 +28,15 @@ import ( // makePluginLayerData builds a tar.gz layer containing a minimal plugin tree // (manifest + a command file). Used by install round-trip tests. func makePluginLayerData(t *testing.T, name string) []byte { + t.Helper() + return makePluginLayerDataWithBody(t, name, "# hello") +} + +func makePluginLayerDataWithBody(t *testing.T, name, body string) []byte { t.Helper() files := []ociartifact.FileEntry{ {Path: ".claude-plugin/plugin.json", Content: []byte(fmt.Sprintf(`{"name":%q,"version":"1.0.0"}`, name)), Mode: 0644}, - {Path: "commands/hello.md", Content: []byte("# hello"), Mode: 0644}, + {Path: "commands/hello.md", Content: []byte(body), Mode: 0644}, } data, err := ociartifact.CompressTar(files, ociartifact.DefaultTarOptions(), ociartifact.DefaultGzipOptions()) require.NoError(t, err) diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index 546c7747ae..71f0c0f777 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -5,6 +5,7 @@ package pluginsvc import ( "context" + "errors" "net/http" "os" "path/filepath" @@ -14,9 +15,11 @@ import ( "github.com/stretchr/testify/require" "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -71,6 +74,7 @@ func newLockTestService(t *testing.T, enableGate bool) (plugins.PluginService, s svc := New( WithStore(sqlite.NewPluginStore(db)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), + WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), ) return svc, projectRoot } @@ -200,6 +204,9 @@ func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, }) require.Error(t, err, "the DB record must be rolled back so a retry starts fresh") + + _, err = os.Stat(filepath.Join(projectRoot, ".claude", "plugins", "my-plugin")) + assert.True(t, os.IsNotExist(err), "a failed fresh install must dematerialize on rollback") } //nolint:paralleltest // uses t.Setenv via newLockTestService @@ -209,13 +216,16 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { first := installTestPlugin(t, svc, projectRoot, validLockDigest()) before, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") require.True(t, ok) + helloPath := filepath.Join(projectRoot, ".claude", "plugins", "my-plugin", "commands", "hello.md") + beforeHello, err := os.ReadFile(helloPath) //nolint:gosec // test fixture path + require.NoError(t, err) require.NoError(t, os.Chmod(projectRoot, 0o555)) t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) - _, err := svc.Install(t.Context(), plugins.InstallOptions{ + _, err = svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", - LayerData: makePluginLayerData(t, "my-plugin"), + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), Digest: validLockDigestAlt(), Scope: plugins.ScopeProject, ProjectRoot: projectRoot, @@ -233,6 +243,10 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { }) require.NoError(t, err, "the pre-existing DB record must survive") assert.Equal(t, first.Plugin.Digest, info.InstalledPlugin.Digest) + + afterHello, err := os.ReadFile(helloPath) //nolint:gosec // test fixture path + require.NoError(t, err, "the previous materialization must be restored") + assert.Equal(t, beforeHello, afterHello) } //nolint:paralleltest // uses t.Setenv via newLockTestService @@ -297,3 +311,80 @@ func TestUninstall_DoesNotTouchSkillsKey(t *testing.T) { _, ok = lf.GetPlugin("my-plugin") assert.False(t, ok) } + +type hookPluginStore struct { + storage.PluginStore + beforeDelete func() error +} + +func (s *hookPluginStore) Delete(ctx context.Context, name string, scope plugins.Scope, projectRoot string) error { + if s.beforeDelete != nil { + if err := s.beforeDelete(); err != nil { + return err + } + } + return s.PluginStore.Delete(ctx, name, scope, projectRoot) +} + +type failingDematerializeAdapter struct { + plugins.MaterializationAdapter + err error +} + +func (a *failingDematerializeAdapter) Dematerialize(context.Context, plugins.DematerializeRequest) error { + return a.err +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_DematerializeFailureRestoresLockEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["claude-code"] = &failingDematerializeAdapter{ + MaterializationAdapter: inner.materializers["claude-code"], + err: errors.New("permission denied"), + } + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.True(t, ok, "a failed dematerialize must restore the lock entry") + + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the DB record must survive so uninstall can be retried") + assert.NotNil(t, info.InstalledPlugin) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_StoreDeleteFailureRestoresLockEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.store = &hookPluginStore{ + PluginStore: inner.store, + beforeDelete: func() error { return errors.New("db locked") }, + } + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "db locked") + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.True(t, ok, "a failed DB delete must restore the lock entry") + + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err, "the plugin must remain installed") + assert.NotNil(t, info.InstalledPlugin) +} diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 089e0cb872..e65bbc2f6b 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -12,15 +12,17 @@ import ( "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/lockfile" "github.com/stacklok/toolhive/pkg/storage" ) // Uninstall removes an installed plugin and dematerializes it for all clients. -// Dematerialization is best-effort: errors are collected via errors.Join so a -// single client failure does not abort cleanup of the others. The DB record is -// always deleted (when present), making Uninstall idempotent. Mirror of -// skillsvc.Uninstall, substituting MaterializationAdapter.Dematerialize for -// pathResolver + installer.Remove. +// Dematerialization is best-effort for unmanaged installs: errors are collected +// via errors.Join so a single client failure does not abort cleanup of the +// others, and the DB record is still deleted. For lock-managed project-scope +// installs the lock entry is removed first; a later dematerialize or DB-delete +// failure restores the pin and aborts so the plugin is not left +// installed-but-untracked. func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) error { if err := plugins.ValidatePluginName(opts.Name); err != nil { return httperr.WithCode(err, http.StatusBadRequest) @@ -45,34 +47,21 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return err } - // The lock entry is removed FIRST, and its failure aborts the uninstall - // while everything is still intact. The reverse order (files/DB first, - // lock entry best-effort) had a resurrection hazard: a lock-write - // failure after the record and files were gone left a stale entry that - // the next sync silently reinstalled. - if scope == plugins.ScopeProject && existing.Managed { - if lockErr := removeLockEntry(opts); lockErr != nil { - return fmt.Errorf("updating project lock file: %w", lockErr) - } + restoreLock, err := removeManagedLockEntry(opts, existing, scope) + if err != nil { + return err } - // Dematerialize for each client — best-effort. - var cleanupErrs []error - for _, clientType := range existing.Clients { - adapter, ok := s.materializers[clientType] - if !ok { - continue - } - if dmErr := adapter.Dematerialize(ctx, plugins.DematerializeRequest{ - Name: opts.Name, - Scope: scope, - ProjectRoot: opts.ProjectRoot, - }); dmErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("dematerializing plugin for client %q: %w", clientType, dmErr)) - } + cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) + if len(cleanupErrs) > 0 && restoreLock != nil { + restoreLock() + return errors.Join(cleanupErrs...) } if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { + if restoreLock != nil { + restoreLock() + } return err } @@ -84,3 +73,61 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return errors.Join(cleanupErrs...) } + +// removeManagedLockEntry removes the plugins: lock entry for a lock-managed +// project-scope install, returning a restore func that reinstates the +// snapshotted entry. The restore func is nil when no entry was removed. +func removeManagedLockEntry( + opts plugins.UninstallOptions, + existing plugins.InstalledPlugin, + scope plugins.Scope, +) (restore func(), err error) { + if scope != plugins.ScopeProject || !existing.Managed { + return nil, nil + } + + var prevLock *lockfile.Entry + if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { + if lf, loadErr := lockfile.Load(root); loadErr == nil { + if e, ok := lf.GetPlugin(opts.Name); ok { + prevLock = &e + } + } + } + if lockErr := removeLockEntry(opts); lockErr != nil { + return nil, fmt.Errorf("updating project lock file: %w", lockErr) + } + if prevLock == nil { + return func() {}, nil + } + return func() { + if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { + _ = lockfile.UpsertPluginEntry(root, *prevLock) + } + }, nil +} + +// dematerializeClients best-effort removes on-disk copies for each client the +// plugin was installed into. Missing adapters are skipped. +func (s *service) dematerializeClients( + ctx context.Context, + existing plugins.InstalledPlugin, + scope plugins.Scope, + projectRoot string, +) []error { + var cleanupErrs []error + for _, clientType := range existing.Clients { + adapter, ok := s.materializers[clientType] + if !ok { + continue + } + if dmErr := adapter.Dematerialize(ctx, plugins.DematerializeRequest{ + Name: existing.Metadata.Name, + Scope: scope, + ProjectRoot: projectRoot, + }); dmErr != nil { + cleanupErrs = append(cleanupErrs, fmt.Errorf("dematerializing plugin for client %q: %w", clientType, dmErr)) + } + } + return cleanupErrs +} From bd338680305d3fc8895701ad40d7aca0b2758174 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 10:41:50 +0200 Subject: [PATCH 3/9] Restore full plugin state after install rollback Group membership, executable modes, and client marketplace registration must come back with the files so a failed lock write cannot leave a half-installed plugin. Signed-off-by: Samuele Verzi --- pkg/groups/plugins.go | 34 +++- pkg/groups/plugins_test.go | 74 ++++++- pkg/plugins/adapter.go | 5 + pkg/plugins/adapters/claudecode.go | 40 ++-- pkg/plugins/adapters/claudecode_test.go | 35 ++++ pkg/plugins/adapters/codex.go | 22 +- pkg/plugins/adapters/codex_test.go | 25 +++ pkg/plugins/mocks/mock_adapter.go | 14 ++ pkg/plugins/pluginsvc/install.go | 45 ++-- pkg/plugins/pluginsvc/install_extraction.go | 129 +++++++++--- pkg/plugins/pluginsvc/install_test.go | 3 + pkg/plugins/pluginsvc/lock_test.go | 214 ++++++++++++++++++++ pkg/plugins/pluginsvc/uninstall.go | 17 +- 13 files changed, 587 insertions(+), 70 deletions(-) diff --git a/pkg/groups/plugins.go b/pkg/groups/plugins.go index 0b655b801b..063f706c82 100644 --- a/pkg/groups/plugins.go +++ b/pkg/groups/plugins.go @@ -11,21 +11,45 @@ import ( // AddPluginToGroup adds pluginName to the Plugins slice of the named group. // Groups that do not exist return an error. Duplicate plugin names are skipped. -// Empty groupName is a no-op. -func AddPluginToGroup(ctx context.Context, mgr Manager, groupName string, pluginName string) error { +// Empty groupName is a no-op. The bool reports whether this call inserted the +// name (false when it was already a member or groupName is empty), so a later +// rollback can remove it only when this operation added it. +func AddPluginToGroup(ctx context.Context, mgr Manager, groupName string, pluginName string) (added bool, err error) { if groupName == "" { - return nil + return false, nil } group, err := mgr.Get(ctx, groupName) if err != nil { - return fmt.Errorf("getting group %q: %w", groupName, err) + return false, fmt.Errorf("getting group %q: %w", groupName, err) } if slices.Contains(group.Plugins, pluginName) { - return nil + return false, nil } group.Plugins = append(group.Plugins, pluginName) + if err := mgr.Update(ctx, group); err != nil { + return false, fmt.Errorf("updating group %q: %w", groupName, err) + } + return true, nil +} + +// RemovePluginFromGroup removes pluginName from the named group's Plugins +// slice. Missing membership is a no-op. Empty groupName is a no-op. +func RemovePluginFromGroup(ctx context.Context, mgr Manager, groupName string, pluginName string) error { + if groupName == "" { + return nil + } + group, err := mgr.Get(ctx, groupName) + if err != nil { + return fmt.Errorf("getting group %q: %w", groupName, err) + } + + idx := slices.Index(group.Plugins, pluginName) + if idx < 0 { + return nil + } + group.Plugins = slices.Delete(group.Plugins, idx, idx+1) if err := mgr.Update(ctx, group); err != nil { return fmt.Errorf("updating group %q: %w", groupName, err) } diff --git a/pkg/groups/plugins_test.go b/pkg/groups/plugins_test.go index 206ec29da1..5e03ddd3b5 100644 --- a/pkg/groups/plugins_test.go +++ b/pkg/groups/plugins_test.go @@ -24,12 +24,14 @@ func TestAddPluginToGroups(t *testing.T) { groupName string pluginName string setupMock func(*groupmocks.MockManager) + wantAdded bool wantErr string }{ { name: "adds plugin to one group", groupName: "mygroup", pluginName: "my-plugin", + wantAdded: true, setupMock: func(m *groupmocks.MockManager) { m.EXPECT().Get(gomock.Any(), "mygroup"). Return(&Group{Name: "mygroup", Plugins: []string{}}, nil) @@ -85,13 +87,15 @@ func TestAddPluginToGroups(t *testing.T) { mgr := groupmocks.NewMockManager(ctrl) tt.setupMock(mgr) - err := AddPluginToGroup(context.Background(), mgr, tt.groupName, tt.pluginName) + added, err := AddPluginToGroup(context.Background(), mgr, tt.groupName, tt.pluginName) if tt.wantErr != "" { require.Error(t, err) assert.Contains(t, err.Error(), tt.wantErr) + assert.False(t, added) } else { require.NoError(t, err) + assert.Equal(t, tt.wantAdded, added) } }) } @@ -188,3 +192,71 @@ func TestRemovePluginFromAllGroups(t *testing.T) { }) } } + +func TestRemovePluginFromGroup(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + groupName string + pluginName string + setupMock func(*groupmocks.MockManager) + wantErr string + }{ + { + name: "removes plugin from group", + groupName: "mygroup", + pluginName: "my-plugin", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Plugins: []string{"my-plugin", "other"}}, nil) + m.EXPECT().Update(gomock.Any(), &Group{Name: "mygroup", Plugins: []string{"other"}}). + Return(nil) + }, + }, + { + name: "no-op when plugin is not a member", + groupName: "mygroup", + pluginName: "absent", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Plugins: []string{"other"}}, nil) + }, + }, + { + name: "no-op when group name is empty", + groupName: "", + pluginName: "my-plugin", + setupMock: func(_ *groupmocks.MockManager) {}, + }, + { + name: "returns error when group not found", + groupName: "nonexistent", + pluginName: "my-plugin", + setupMock: func(m *groupmocks.MockManager) { + m.EXPECT().Get(gomock.Any(), "nonexistent"). + Return(nil, errors.New("group not found")) + }, + wantErr: "getting group", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mgr := groupmocks.NewMockManager(ctrl) + tt.setupMock(mgr) + + err := RemovePluginFromGroup(context.Background(), mgr, tt.groupName, tt.pluginName) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + } else { + require.NoError(t, err) + } + }) + } +} diff --git a/pkg/plugins/adapter.go b/pkg/plugins/adapter.go index 47a8fc0d44..df72d84fbd 100644 --- a/pkg/plugins/adapter.go +++ b/pkg/plugins/adapter.go @@ -103,6 +103,11 @@ type MaterializationAdapter interface { // config mutations the adapter itself made. Must be idempotent: a missing // install is not an error. Dematerialize(ctx context.Context, req DematerializeRequest) error + // EnsureRegistered restores this plugin's marketplace/settings entries + // without re-extracting files. Used after restoring a plugin-tree snapshot + // so a failed upgrade does not leave the client unable to discover the + // plugin. Must be idempotent. + EnsureRegistered(ctx context.Context, req DematerializeRequest) error // SupportedComponents returns the component types this adapter loads. SupportedComponents() []ComponentType // ScopeSupport reports whether a project-scoped install degrades for this diff --git a/pkg/plugins/adapters/claudecode.go b/pkg/plugins/adapters/claudecode.go index 546ee5ec06..4064073053 100644 --- a/pkg/plugins/adapters/claudecode.go +++ b/pkg/plugins/adapters/claudecode.go @@ -74,20 +74,8 @@ func (a *ClaudeCodeAdapter) Materialize(_ context.Context, req plugins.Materiali return nil, fmt.Errorf("extracting plugin: %w", err) } - // The marketplace root is the plugins parent directory; each plugin lives in - // a "" subdirectory referenced as a "./" source. - marketplaceRoot := filepath.Dir(dir) - - // Upsert the plugin into the shared marketplace.json at the plugins root so - // Claude Code resolves it under the "toolhive" marketplace. - if err := upsertClaudeMarketplace(marketplaceRoot, req.Name); err != nil { - return nil, fmt.Errorf("writing marketplace.json: %w", err) - } - - // Patch settings.json to enable the plugin under the toolhive marketplace. - settingsPath := a.settingsPath(req.Scope, req.ProjectRoot) - if err := enablePluginInSettings(settingsPath, req.Name, marketplaceRoot); err != nil { - return nil, fmt.Errorf("enabling plugin in settings.json: %w", err) + if err := a.registerPlugin(req.Name, req.Scope, req.ProjectRoot, dir); err != nil { + return nil, err } return &plugins.MaterializeResult{ @@ -131,6 +119,30 @@ func (a *ClaudeCodeAdapter) Dematerialize(_ context.Context, req plugins.Demater return nil } +// EnsureRegistered restores the marketplace.json and settings.json entries +// for this plugin without re-extracting files. +func (a *ClaudeCodeAdapter) EnsureRegistered(_ context.Context, req plugins.DematerializeRequest) error { + dir, err := a.cm.GetPluginPath(client.ClaudeCode, req.Name, req.Scope, req.ProjectRoot) + if err != nil { + return fmt.Errorf("resolving plugin path: %w", err) + } + return a.registerPlugin(req.Name, req.Scope, req.ProjectRoot, dir) +} + +// registerPlugin upserts the shared marketplace.json entry and enables the +// plugin in settings.json. +func (a *ClaudeCodeAdapter) registerPlugin(name string, scope plugins.Scope, projectRoot, pluginDir string) error { + marketplaceRoot := filepath.Dir(pluginDir) + if err := upsertClaudeMarketplace(marketplaceRoot, name); err != nil { + return fmt.Errorf("writing marketplace.json: %w", err) + } + settingsPath := a.settingsPath(scope, projectRoot) + if err := enablePluginInSettings(settingsPath, name, marketplaceRoot); err != nil { + return fmt.Errorf("enabling plugin in settings.json: %w", err) + } + return nil +} + // SupportedComponents returns the component types Claude Code loads. func (*ClaudeCodeAdapter) SupportedComponents() []plugins.ComponentType { return claudeCodeSupported diff --git a/pkg/plugins/adapters/claudecode_test.go b/pkg/plugins/adapters/claudecode_test.go index ee16707fd4..5c29cd9fc1 100644 --- a/pkg/plugins/adapters/claudecode_test.go +++ b/pkg/plugins/adapters/claudecode_test.go @@ -560,3 +560,38 @@ func TestClaudeCodeAdapter_ProjectScopeUsesProjectSettings(t *testing.T) { _, hasEnabled := settings["enabledPlugins"] assert.False(t, hasEnabled, "project enabledPlugins removed when empty") } + +func TestClaudeCodeAdapter_EnsureRegisteredRestoresSettings(t *testing.T) { + t.Parallel() + tempHome := resolvedTempDir(t) + cm := newTestClientManager(t, tempHome) + a := NewClaudeCodeAdapter(cm) + + layer := makePluginLayer(t, []ociskills.FileEntry{ + {Path: "commands/greet.md", Content: []byte("# greet"), Mode: 0644}, + }) + _, err := a.Materialize(context.Background(), plugins.MaterializeRequest{ + Name: "my-plugin", + LayerData: layer, + Scope: plugins.ScopeUser, + }) + require.NoError(t, err) + + require.NoError(t, a.Dematerialize(context.Background(), plugins.DematerializeRequest{ + Name: "my-plugin", + Scope: plugins.ScopeUser, + })) + + require.NoError(t, a.EnsureRegistered(context.Background(), plugins.DematerializeRequest{ + Name: "my-plugin", + Scope: plugins.ScopeUser, + })) + + settings := readSettings(t, userSettingsPath(tempHome)) + enabled, ok := settings["enabledPlugins"].(map[string]any) + require.True(t, ok, "enabledPlugins present after EnsureRegistered") + assert.Equal(t, true, enabled["my-plugin@toolhive"]) + + mp := readClaudeMarketplaceManifest(t, filepath.Join(tempHome, ".claude", "plugins")) + requireMarketplacePlugin(t, mp, "my-plugin") +} diff --git a/pkg/plugins/adapters/codex.go b/pkg/plugins/adapters/codex.go index b6bf09b6bd..b39dd0a8d6 100644 --- a/pkg/plugins/adapters/codex.go +++ b/pkg/plugins/adapters/codex.go @@ -103,11 +103,8 @@ func (a *CodexAdapter) Materialize(_ context.Context, req plugins.MaterializeReq return nil, fmt.Errorf("extracting plugin: %w", err) } - // Register the plugin in the shared marketplace.json at the marketplace root - // so Codex can discover it. - root := a.codexMarketplaceRoot(req.Scope, req.ProjectRoot) - if err := upsertCodexMarketplace(codexMarketplaceFile(root), req.Name); err != nil { - return nil, fmt.Errorf("writing codex marketplace: %w", err) + if err := a.registerPlugin(req.Name, req.Scope, req.ProjectRoot); err != nil { + return nil, err } return &plugins.MaterializeResult{ @@ -148,6 +145,21 @@ func (a *CodexAdapter) Dematerialize(_ context.Context, req plugins.Dematerializ return nil } +// EnsureRegistered restores the marketplace.json entry for this plugin +// without re-extracting files. +func (a *CodexAdapter) EnsureRegistered(_ context.Context, req plugins.DematerializeRequest) error { + return a.registerPlugin(req.Name, req.Scope, req.ProjectRoot) +} + +// registerPlugin upserts the shared Codex marketplace.json entry. +func (a *CodexAdapter) registerPlugin(name string, scope plugins.Scope, projectRoot string) error { + root := a.codexMarketplaceRoot(scope, projectRoot) + if err := upsertCodexMarketplace(codexMarketplaceFile(root), name); err != nil { + return fmt.Errorf("writing codex marketplace: %w", err) + } + return nil +} + // SupportedComponents returns the component types the Codex CLI loads. func (*CodexAdapter) SupportedComponents() []plugins.ComponentType { return codexSupported diff --git a/pkg/plugins/adapters/codex_test.go b/pkg/plugins/adapters/codex_test.go index de5b3f03b0..3bc90d71ea 100644 --- a/pkg/plugins/adapters/codex_test.go +++ b/pkg/plugins/adapters/codex_test.go @@ -301,6 +301,31 @@ func TestCodexAdapter_NonLifoUninstallKeepsMarketplaceValid(t *testing.T) { assert.NoDirExists(t, betaDir, "beta directory removed") } +func TestCodexAdapter_EnsureRegisteredRestoresMarketplace(t *testing.T) { + t.Parallel() + tempHome := resolvedTempDir(t) + cm := newTestClientManager(t, tempHome) + a := NewCodexAdapter(cm) + + layer := makePluginLayer(t, []ociskills.FileEntry{ + {Path: "skills/useful/SKILL.md", Content: []byte("# useful"), Mode: 0644}, + }) + require.NoError(t, materializeCodex(a, "foo", layer)) + require.NoError(t, a.Dematerialize(context.Background(), plugins.DematerializeRequest{ + Name: "foo", + Scope: plugins.ScopeUser, + })) + + require.NoError(t, a.EnsureRegistered(context.Background(), plugins.DematerializeRequest{ + Name: "foo", + Scope: plugins.ScopeUser, + })) + + mp := readCodexMarketplaceFileAt(t, codexUserMarketplaceFile(tempHome)) + p := findCodexPlugin(t, mp, "foo") + assert.Equal(t, "./toolhive/foo", p.Source.Path) +} + // materializeCodex is a small helper to install a named user-scope plugin. func materializeCodex(a *CodexAdapter, name string, layer []byte) error { _, err := a.Materialize(context.Background(), plugins.MaterializeRequest{ diff --git a/pkg/plugins/mocks/mock_adapter.go b/pkg/plugins/mocks/mock_adapter.go index 3a201e74a4..de2baedca0 100644 --- a/pkg/plugins/mocks/mock_adapter.go +++ b/pkg/plugins/mocks/mock_adapter.go @@ -55,6 +55,20 @@ func (mr *MockMaterializationAdapterMockRecorder) Dematerialize(ctx, req any) *g return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Dematerialize", reflect.TypeOf((*MockMaterializationAdapter)(nil).Dematerialize), ctx, req) } +// EnsureRegistered mocks base method. +func (m *MockMaterializationAdapter) EnsureRegistered(ctx context.Context, req plugins.DematerializeRequest) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "EnsureRegistered", ctx, req) + ret0, _ := ret[0].(error) + return ret0 +} + +// EnsureRegistered indicates an expected call of EnsureRegistered. +func (mr *MockMaterializationAdapterMockRecorder) EnsureRegistered(ctx, req any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnsureRegistered", reflect.TypeOf((*MockMaterializationAdapter)(nil).EnsureRegistered), ctx, req) +} + // Materialize mocks base method. func (m *MockMaterializationAdapter) Materialize(ctx context.Context, req plugins.MaterializeRequest) (*plugins.MaterializeResult, error) { m.ctrl.T.Helper() diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index 99e5c70fff..ca53b70163 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -276,10 +276,11 @@ func splitQualifiedName(s string) (namespace, name string) { // registerPluginInGroup adds the plugin to the requested group when a group // manager is configured. When groupName is empty it defaults to the "default" -// group, matching workload behavior. -func (s *service) registerPluginInGroup(ctx context.Context, groupName string, pluginName string) error { +// group, matching workload behavior. The bool reports whether this call +// inserted the name, so rollback can remove it only then. +func (s *service) registerPluginInGroup(ctx context.Context, groupName string, pluginName string) (bool, error) { if s.groupManager == nil { - return nil + return false, nil } if groupName == "" { groupName = groups.DefaultGroup @@ -287,15 +288,23 @@ func (s *service) registerPluginInGroup(ctx context.Context, groupName string, p return groups.AddPluginToGroup(ctx, s.groupManager, groupName, pluginName) } +func resolvedGroupName(groupName string) string { + if groupName == "" { + return groups.DefaultGroup + } + return groupName +} + // installAndRegister registers the just-installed plugin in the target group // and, for project-scope installs with the lock file feature enabled (see // plugins.LockFileFeatureEnabled), records it in the project's // toolhive.lock.yaml plugins: key. If group registration or the lock write -// fails, the DB record, on-disk files, and lock entry are rolled back to -// their pre-install state: restored when this call updated a pre-existing -// record (a --force reinstall must not be destroyed by a transient failure), -// deleted/dematerialized when this call created them. Callers must hold the -// per-plugin lock for the duration of this call. +// fails, the DB record, on-disk files, group membership (only when this call +// added it), and lock entry are rolled back to their pre-install state: +// restored when this call updated a pre-existing record (a --force reinstall +// must not be destroyed by a transient failure), deleted/dematerialized when +// this call created them. Callers must hold the per-plugin lock for the +// duration of this call. func (s *service) installAndRegister( ctx context.Context, opts plugins.InstallOptions, @@ -318,12 +327,17 @@ func (s *service) installAndRegister( } } - rollback := func() { s.rollbackInstall(ctx, opts, result, pluginName, scope, lockScoped, prevEntry) } + var addedToGroup bool + rollback := func() { + s.rollbackInstall(ctx, opts, result, pluginName, scope, lockScoped, prevEntry, addedToGroup, resolvedGroupName(opts.Group)) + } - if err := s.registerPluginInGroup(ctx, opts.Group, pluginName); err != nil { + added, err := s.registerPluginInGroup(ctx, opts.Group, pluginName) + if err != nil { rollback() return nil, fmt.Errorf("registering plugin in group: %w", err) } + addedToGroup = added if lockScoped { updated, err := s.recordLockState(ctx, opts, result.Plugin, result.ContentDigest) @@ -343,8 +357,9 @@ func (s *service) installAndRegister( // rollbackInstall undoes installAndRegister's side effects after a failure, // best-effort. The DB record is restored to its pre-install snapshot when // one exists (result.PreExisting) and deleted otherwise; on-disk files are -// dematerialized (fresh install) or restored (upgrade); the lock entry is -// likewise reinstated from prevEntry or removed. +// dematerialized (fresh install) or restored (upgrade); group membership is +// removed only when this call added it; the lock entry is likewise +// reinstated from prevEntry or removed. func (s *service) rollbackInstall( ctx context.Context, opts plugins.InstallOptions, @@ -353,6 +368,8 @@ func (s *service) rollbackInstall( scope plugins.Scope, lockScoped bool, prevEntry *lockfile.Entry, + addedToGroup bool, + groupName string, ) { if result.PreExisting != nil { _ = s.store.Update(ctx, *result.PreExisting) @@ -364,6 +381,10 @@ func (s *service) rollbackInstall( result.RestoreFiles(ctx) } + if addedToGroup && s.groupManager != nil { + _ = groups.RemovePluginFromGroup(ctx, s.groupManager, groupName, pluginName) + } + if !lockScoped { return } diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 02ceeba2c5..a48f38c126 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -142,21 +142,28 @@ func (s *service) installExtractionUpgradeDigest( clientTypes []string, ) (*plugins.InstallResult, error) { allClients := mergeClientLists(existing.Clients, clientTypes) - backups := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, allClients) + backups, snapErr := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, allClients) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting installed plugin trees: %w", snapErr) + } if _, err := s.materializeForClients(ctx, opts, scope, allClients); err != nil { - s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) + if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients); restoreErr != nil { + return nil, errors.Join(err, restoreErr) + } return nil, err } pl := buildInstalledPlugin(opts, scope, allClients, nil) pl.Managed = existing.Managed if err := s.store.Update(ctx, pl); err != nil { - s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) + if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients); restoreErr != nil { + return nil, errors.Join(err, restoreErr) + } return nil, err } return &plugins.InstallResult{ Plugin: pl, RestoreFiles: func(ctx context.Context) { - s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) + _ = s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) }, }, nil } @@ -220,50 +227,89 @@ func (s *service) materializeForClients( return materialized, nil } +// fileSnapshot is one regular file captured by snapshotDir: contents plus a +// sanitized permission mode so executable hooks stay executable on restore. +type fileSnapshot struct { + data []byte + mode fs.FileMode +} + +// snapshotFileModeMask strips setuid/setgid/sticky and caps at 0755, matching +// skills.PluginFilePermissionMask so restored hooks keep +x without restoring +// unsafe bits. +const snapshotFileModeMask fs.FileMode = 0o755 + +func sanitizeFileMode(mode fs.FileMode) fs.FileMode { + return mode.Perm() & snapshotFileModeMask +} + // snapshotClientTrees copies each client's installed plugin tree into memory // so a later rollback can restore the previous materialization without // leaking temp directories. Missing directories are omitted (the client was -// not yet installed). +// not yet installed). A walk/read error on an existing tree is returned so +// the caller can abort before mutating. func (s *service) snapshotClientTrees( name string, scope plugins.Scope, projectRoot string, clientTypes []string, -) map[string]map[string][]byte { - backups := make(map[string]map[string][]byte, len(clientTypes)) +) (map[string]map[string]fileSnapshot, error) { + backups := make(map[string]map[string]fileSnapshot, len(clientTypes)) + var errs []error for _, ct := range clientTypes { dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) if err != nil { continue } - files, ok := snapshotDir(dir) - if !ok { + files, err := snapshotDir(dir) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + continue + } + errs = append(errs, fmt.Errorf("snapshotting %s copy of %q: %w", ct, name, err)) continue } backups[ct] = files } + if len(errs) > 0 { + return backups, errors.Join(errs...) + } if len(backups) == 0 { - return nil + return nil, nil } - return backups + return backups, nil } // restoreClientTrees writes snapshotClientTrees backups back over the live -// plugin directories. Clients without a backup are dematerialized (they were -// newly added by the failed install). +// plugin directories and re-registers each restored client so marketplace +// and settings entries match the restored tree. Clients without a backup are +// dematerialized (they were newly added by the failed install). func (s *service) restoreClientTrees( ctx context.Context, name string, scope plugins.Scope, projectRoot string, - backups map[string]map[string][]byte, + backups map[string]map[string]fileSnapshot, allClients []string, -) { +) error { + var errs []error restored := make(map[string]struct{}, len(backups)) for ct, files := range backups { restored[ct] = struct{}{} dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) if err != nil { + errs = append(errs, fmt.Errorf("resolving %s install path: %w", ct, err)) continue } - restoreDir(dir, files) + if err := restoreDir(dir, files); err != nil { + errs = append(errs, fmt.Errorf("restoring %s plugin tree: %w", ct, err)) + } + if adapter, ok := s.materializers[ct]; ok { + if err := adapter.EnsureRegistered(ctx, plugins.DematerializeRequest{ + Name: name, + Scope: scope, + ProjectRoot: projectRoot, + }); err != nil { + errs = append(errs, fmt.Errorf("restoring %s registration: %w", ct, err)) + } + } } var extra []string for _, ct := range allClients { @@ -272,19 +318,31 @@ func (s *service) restoreClientTrees( } } s.dematerializeAll(ctx, extra, name, scope, projectRoot) + return errors.Join(errs...) } -// snapshotDir reads every regular file under dir into a relative-path map. -// Returns ok=false when dir does not exist. -func snapshotDir(dir string) (map[string][]byte, bool) { +// snapshotDir reads every regular file under dir into a relative-path map, +// preserving sanitized permission bits. Returns os.ErrNotExist when dir does +// not exist. +func snapshotDir(dir string) (map[string]fileSnapshot, error) { if _, err := os.Stat(dir); err != nil { - return nil, false + return nil, err } - files := make(map[string][]byte) - _ = filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { - if err != nil || d.IsDir() { + files := make(map[string]fileSnapshot) + err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil { return err } + if d.IsDir() { + return nil + } + info, infoErr := d.Info() + if infoErr != nil { + return infoErr + } + if !info.Mode().IsRegular() { + return nil + } rel, relErr := filepath.Rel(dir, path) if relErr != nil { return relErr @@ -293,22 +351,33 @@ func snapshotDir(dir string) (map[string][]byte, bool) { if readErr != nil { return readErr } - files[rel] = data + files[rel] = fileSnapshot{data: data, mode: sanitizeFileMode(info.Mode())} return nil }) - return files, true + if err != nil { + return nil, err + } + return files, nil } -// restoreDir replaces dir with the files captured by snapshotDir. -func restoreDir(dir string, files map[string][]byte) { - _ = os.RemoveAll(dir) - for rel, data := range files { +// restoreDir replaces dir with the files captured by snapshotDir, writing +// each file with its sanitized mode. +func restoreDir(dir string, files map[string]fileSnapshot) error { + if err := os.RemoveAll(dir); err != nil { + return err + } + var errs []error + for rel, snap := range files { path := filepath.Join(dir, rel) if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { + errs = append(errs, err) continue } - _ = os.WriteFile(path, data, 0o600) + if err := os.WriteFile(path, snap.data, snap.mode); err != nil { //nolint:gosec // mode is masked to 0755 + errs = append(errs, err) + } } + return errors.Join(errs...) } // pluginInstallPath resolves the on-disk plugin directory for a client. diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index 363c8128d1..70908a1db4 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -553,6 +553,9 @@ func TestInstallAddsPluginToGroup(t *testing.T) { gm := groupmocks.NewMockManager(ctrl) adapter := plugmocks.NewMockMaterializationAdapter(ctrl) adapter.EXPECT().Materialize(gomock.Any(), gomock.Any()).Return(&plugins.MaterializeResult{}, nil).AnyTimes() + if tt.wantErr != "" { + adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil).AnyTimes() + } tt.setupStoreMock(store) tt.setupGroupMock(gm) diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index 71f0c0f777..7910def382 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -6,6 +6,7 @@ package pluginsvc import ( "context" "errors" + "io/fs" "net/http" "os" "path/filepath" @@ -13,10 +14,14 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/client" + "github.com/stacklok/toolhive/pkg/groups" + groupmocks "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/plugins/adapters" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/lockfile" "github.com/stacklok/toolhive/pkg/storage" @@ -45,6 +50,10 @@ func (a *extractingAdapter) Dematerialize(_ context.Context, req plugins.Demater return a.installer.Remove(filepath.Join(a.base, req.Name)) } +func (*extractingAdapter) EnsureRegistered(context.Context, plugins.DematerializeRequest) error { + return nil +} + func (*extractingAdapter) SupportedComponents() []plugins.ComponentType { return []plugins.ComponentType{plugins.ComponentCommands} } @@ -388,3 +397,208 @@ func TestUninstall_StoreDeleteFailureRestoresLockEntry(t *testing.T) { require.NoError(t, err, "the plugin must remain installed") assert.NotNil(t, info.InstalledPlugin) } + +func TestSnapshotRestore_PreservesExecutableMode(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + hook := filepath.Join(dir, "hooks", "preinstall.sh") + require.NoError(t, os.MkdirAll(filepath.Dir(hook), 0o750)) + require.NoError(t, os.WriteFile(hook, []byte("#!/bin/sh\necho hi\n"), 0o600)) + require.NoError(t, os.Chmod(hook, 0o755)) + md := filepath.Join(dir, "commands", "hello.md") + require.NoError(t, os.MkdirAll(filepath.Dir(md), 0o750)) + require.NoError(t, os.WriteFile(md, []byte("# hello"), 0o644)) + + files, err := snapshotDir(dir) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o755), files[filepath.Join("hooks", "preinstall.sh")].mode) + assert.Equal(t, fs.FileMode(0o644), files[filepath.Join("commands", "hello.md")].mode) + + dest := t.TempDir() + require.NoError(t, restoreDir(dest, files)) + + info, err := os.Stat(filepath.Join(dest, "hooks", "preinstall.sh")) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o755), info.Mode().Perm()) + mdInfo, err := os.Stat(filepath.Join(dest, "commands", "hello.md")) + require.NoError(t, err) + assert.Equal(t, fs.FileMode(0o644), mdInfo.Mode().Perm()) +} + +type failingMaterializeAdapter struct { + err error +} + +func (a *failingMaterializeAdapter) Materialize(context.Context, plugins.MaterializeRequest) (*plugins.MaterializeResult, error) { + return nil, a.err +} + +func (*failingMaterializeAdapter) Dematerialize(context.Context, plugins.DematerializeRequest) error { + return nil +} + +func (*failingMaterializeAdapter) EnsureRegistered(context.Context, plugins.DematerializeRequest) error { + return nil +} + +func (*failingMaterializeAdapter) SupportedComponents() []plugins.ComponentType { + return nil +} + +func (*failingMaterializeAdapter) ScopeSupport() plugins.ScopeSupport { + return plugins.ScopeSupport{} +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_LockWriteFailureRemovesGroupMembership(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + ctrl := gomock.NewController(t) + gm := groupmocks.NewMockManager(ctrl) + + var members []string + gm.EXPECT().Get(gomock.Any(), groups.DefaultGroup).DoAndReturn( + func(context.Context, string) (*groups.Group, error) { + cp := make([]string, len(members)) + copy(cp, members) + return &groups.Group{Name: groups.DefaultGroup, Plugins: cp}, nil + }, + ).AnyTimes() + gm.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, g *groups.Group) error { + members = append([]string(nil), g.Plugins...) + return nil + }, + ).Times(2) + + inner := svc.(*service) //nolint:forcetypeassert + inner.groupManager = gm + + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Empty(t, members, "a failed fresh install must not leave the plugin in the group") +} + +//nolint:paralleltest // uses t.Setenv +func TestInstallUpgrade_SecondClientFailureRestoresRegistration(t *testing.T) { + t.Setenv(plugins.LockFileEnvVar, "true") + + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := sqlite.Open(t.Context(), dbPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + projectRoot := makeProjectRoot(t) + cm := client.NewTestClientManagerWithHome(t.TempDir()) + svc := New( + WithStore(sqlite.NewPluginStore(db)), + WithMaterializers(map[string]plugins.MaterializationAdapter{ + "claude-code": adapters.NewClaudeCodeAdapter(cm), + "codex": &failingMaterializeAdapter{err: errors.New("disk full")}, + }), + WithClientManager(cm), + ) + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + settingsPath := filepath.Join(projectRoot, ".claude", "settings.json") + before, err := os.ReadFile(settingsPath) //nolint:gosec // test fixture + require.NoError(t, err) + assert.Contains(t, string(before), "my-plugin@toolhive") + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"codex"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "disk full") + + after, err := os.ReadFile(settingsPath) //nolint:gosec // test fixture + require.NoError(t, err) + assert.Contains(t, string(after), "my-plugin@toolhive", + "a failed upgrade must restore Claude Code settings registration") + + hello, err := os.ReadFile(filepath.Join(projectRoot, ".claude", "plugins", "my-plugin", "commands", "hello.md")) //nolint:gosec + require.NoError(t, err) + assert.Equal(t, "# hello", string(hello), "the previous plugin tree must be restored") +} + +//nolint:paralleltest // uses t.Setenv +func TestUninstall_PartialDematerializeRestoresAllClients(t *testing.T) { + t.Setenv(plugins.LockFileEnvVar, "true") + + dbPath := filepath.Join(t.TempDir(), "test.db") + db, err := sqlite.Open(t.Context(), dbPath) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + projectRoot := makeProjectRoot(t) + claude := &extractingAdapter{ + base: filepath.Join(projectRoot, ".claude", "plugins"), + installer: skills.NewInstaller(), + } + codex := &extractingAdapter{ + base: filepath.Join(projectRoot, ".agents", "plugins", "toolhive"), + installer: skills.NewInstaller(), + } + svc := New( + WithStore(sqlite.NewPluginStore(db)), + WithMaterializers(map[string]plugins.MaterializationAdapter{ + "claude-code": claude, + "codex": codex, + }), + WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + ) + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code", "codex"}, + }) + require.NoError(t, err) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["codex"] = &failingDematerializeAdapter{ + MaterializationAdapter: codex, + err: errors.New("permission denied"), + } + + err = svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + + _, err = os.Stat(filepath.Join(projectRoot, ".claude", "plugins", "my-plugin", "commands", "hello.md")) + require.NoError(t, err, "the successfully dematerialized client must be restored") + + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + require.NotNil(t, info.InstalledPlugin) + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.True(t, ok, "the lock entry must be restored") +} diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index e65bbc2f6b..7fa3f2c78c 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -20,9 +20,10 @@ import ( // Dematerialization is best-effort for unmanaged installs: errors are collected // via errors.Join so a single client failure does not abort cleanup of the // others, and the DB record is still deleted. For lock-managed project-scope -// installs the lock entry is removed first; a later dematerialize or DB-delete -// failure restores the pin and aborts so the plugin is not left -// installed-but-untracked. +// installs the lock entry is removed first after snapshotting every client +// tree; a later dematerialize or DB-delete failure restores the pin, the +// plugin trees, and adapter registration so the plugin is not left +// installed-but-untracked or half-removed across clients. func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) error { if err := plugins.ValidatePluginName(opts.Name); err != nil { return httperr.WithCode(err, http.StatusBadRequest) @@ -52,15 +53,25 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return err } + backups, snapErr := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, existing.Clients) + if snapErr != nil && restoreLock != nil { + restoreLock() + return fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr) + } + cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) if len(cleanupErrs) > 0 && restoreLock != nil { restoreLock() + if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients); restoreErr != nil { + cleanupErrs = append(cleanupErrs, restoreErr) + } return errors.Join(cleanupErrs...) } if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { if restoreLock != nil { restoreLock() + _ = s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients) } return err } From 6b00509d8ed32daba79d855b18c0bca733534a99 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 14:48:05 +0200 Subject: [PATCH 4/9] Surface snapshot and restore errors on rollback A missing client path must abort before mutation, and every compensation failure has to travel with the original error. Signed-off-by: Samuele Verzi --- pkg/plugins/options.go | 6 +- pkg/plugins/pluginsvc/install.go | 62 +++++++++++++-------- pkg/plugins/pluginsvc/install_extraction.go | 49 +++++++++------- pkg/plugins/pluginsvc/install_test.go | 38 +++++++++++-- pkg/plugins/pluginsvc/uninstall.go | 28 +++++++--- 5 files changed, 125 insertions(+), 58 deletions(-) diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index 1642865eb0..190f5aa72f 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -77,8 +77,10 @@ type InstallResult struct { ContentDigest string `json:"-"` // RestoreFiles undoes this install's on-disk writes: dematerialize a // fresh install, or restore the previous tree after a failed upgrade. - // Internal use only — NOT exposed via HTTP API. - RestoreFiles func(context.Context) `json:"-"` + // Callers must join a non-nil error into the failure they are + // compensating; discarding it can hide a partial restore. Internal use + // only — NOT exposed via HTTP API. + RestoreFiles func(context.Context) error `json:"-"` } // UninstallOptions configures the behavior of the Uninstall operation. Alias diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index ca53b70163..4826150a1b 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -5,6 +5,7 @@ package pluginsvc import ( "context" + "errors" "fmt" "log/slog" "net/http" @@ -328,23 +329,24 @@ func (s *service) installAndRegister( } var addedToGroup bool - rollback := func() { - s.rollbackInstall(ctx, opts, result, pluginName, scope, lockScoped, prevEntry, addedToGroup, resolvedGroupName(opts.Group)) + rollback := func() error { + return s.rollbackInstall( + ctx, opts, result, pluginName, scope, lockScoped, prevEntry, + addedToGroup, resolvedGroupName(opts.Group), + ) } added, err := s.registerPluginInGroup(ctx, opts.Group, pluginName) if err != nil { - rollback() - return nil, fmt.Errorf("registering plugin in group: %w", err) + return nil, errors.Join(fmt.Errorf("registering plugin in group: %w", err), rollback()) } addedToGroup = added if lockScoped { updated, err := s.recordLockState(ctx, opts, result.Plugin, result.ContentDigest) if err != nil { - rollback() return nil, httperr.WithCode( - fmt.Errorf("recording plugin in project lock file: %w", err), + errors.Join(fmt.Errorf("recording plugin in project lock file: %w", err), rollback()), http.StatusInternalServerError, ) } @@ -354,12 +356,10 @@ func (s *service) installAndRegister( return result, nil } -// rollbackInstall undoes installAndRegister's side effects after a failure, -// best-effort. The DB record is restored to its pre-install snapshot when -// one exists (result.PreExisting) and deleted otherwise; on-disk files are -// dematerialized (fresh install) or restored (upgrade); group membership is -// removed only when this call added it; the lock entry is likewise -// reinstated from prevEntry or removed. +// rollbackInstall undoes installAndRegister's side effects after a failure. +// Every compensation error is returned so the caller can join it with the +// original failure; discarding it can hide a partial restore after a +// destructive rewrite. func (s *service) rollbackInstall( ctx context.Context, opts plugins.InstallOptions, @@ -370,31 +370,45 @@ func (s *service) rollbackInstall( prevEntry *lockfile.Entry, addedToGroup bool, groupName string, -) { +) error { + var errs []error if result.PreExisting != nil { - _ = s.store.Update(ctx, *result.PreExisting) - } else { - _ = s.store.Delete(ctx, pluginName, scope, opts.ProjectRoot) + if err := s.store.Update(ctx, *result.PreExisting); err != nil { + errs = append(errs, fmt.Errorf("restoring pre-existing DB record: %w", err)) + } + } else if err := s.store.Delete(ctx, pluginName, scope, opts.ProjectRoot); err != nil { + errs = append(errs, fmt.Errorf("deleting rolled-back DB record: %w", err)) } if result.RestoreFiles != nil { - result.RestoreFiles(ctx) + if err := result.RestoreFiles(ctx); err != nil { + errs = append(errs, err) + } } if addedToGroup && s.groupManager != nil { - _ = groups.RemovePluginFromGroup(ctx, s.groupManager, groupName, pluginName) + if err := groups.RemovePluginFromGroup(ctx, s.groupManager, groupName, pluginName); err != nil { + errs = append(errs, fmt.Errorf("removing plugin from group: %w", err)) + } } if !lockScoped { - return + return errors.Join(errs...) } if prevEntry != nil { - if root, err := lockfile.OpenRoot(opts.ProjectRoot); err == nil { - _ = lockfile.UpsertPluginEntry(root, *prevEntry) + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return errors.Join(append(errs, fmt.Errorf("reopening lock file: %w", err))...) + } + if err := lockfile.UpsertPluginEntry(root, *prevEntry); err != nil { + errs = append(errs, fmt.Errorf("restoring lock entry: %w", err)) } - return + return errors.Join(errs...) } - _ = removeLockEntry(plugins.UninstallOptions{ + if err := removeLockEntry(plugins.UninstallOptions{ Name: pluginName, Scope: scope, ProjectRoot: opts.ProjectRoot, - }) + }); err != nil { + errs = append(errs, fmt.Errorf("removing rolled-back lock entry: %w", err)) + } + return errors.Join(errs...) } diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index a48f38c126..864454def8 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -120,13 +120,15 @@ func (s *service) installExtractionSameDigestNewClients( pl := buildInstalledPlugin(opts, scope, clientTypes, existing.Clients) pl.Managed = existing.Managed if err := s.store.Update(ctx, pl); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + if dmErr := s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot); dmErr != nil { + return nil, errors.Join(err, dmErr) + } return nil, err } return &plugins.InstallResult{ Plugin: pl, - RestoreFiles: func(ctx context.Context) { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + RestoreFiles: func(ctx context.Context) error { + return s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) }, }, nil } @@ -162,8 +164,8 @@ func (s *service) installExtractionUpgradeDigest( } return &plugins.InstallResult{ Plugin: pl, - RestoreFiles: func(ctx context.Context) { - _ = s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) + RestoreFiles: func(ctx context.Context) error { + return s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) }, }, nil } @@ -182,13 +184,15 @@ func (s *service) installExtractionFresh( } pl := buildInstalledPlugin(opts, scope, clientTypes, nil) if err := s.store.Create(ctx, pl); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + if dmErr := s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot); dmErr != nil { + return nil, errors.Join(err, dmErr) + } return nil, err } return &plugins.InstallResult{ Plugin: pl, - RestoreFiles: func(ctx context.Context) { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + RestoreFiles: func(ctx context.Context) error { + return s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) }, }, nil } @@ -206,11 +210,11 @@ func (s *service) materializeForClients( for _, ct := range clientTypes { adapter, ok := s.materializers[ct] if !ok { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - return nil, httperr.WithCode( + err := httperr.WithCode( fmt.Errorf("no materializer configured for client %q", ct), http.StatusInternalServerError, ) + return nil, errors.Join(err, s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot)) } if _, err := adapter.Materialize(ctx, plugins.MaterializeRequest{ Name: opts.Name, @@ -219,8 +223,8 @@ func (s *service) materializeForClients( ProjectRoot: opts.ProjectRoot, Components: opts.Components, }); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - return nil, fmt.Errorf("materializing plugin for client %q: %w", ct, err) + wrapped := fmt.Errorf("materializing plugin for client %q: %w", ct, err) + return nil, errors.Join(wrapped, s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot)) } materialized = append(materialized, ct) } @@ -256,7 +260,7 @@ func (s *service) snapshotClientTrees( for _, ct := range clientTypes { dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) if err != nil { - continue + return nil, fmt.Errorf("resolving %s install path of %q: %w", ct, name, err) } files, err := snapshotDir(dir) if err != nil { @@ -317,7 +321,9 @@ func (s *service) restoreClientTrees( extra = append(extra, ct) } } - s.dematerializeAll(ctx, extra, name, scope, projectRoot) + if err := s.dematerializeAll(ctx, extra, name, scope, projectRoot); err != nil { + errs = append(errs, err) + } return errors.Join(errs...) } @@ -388,25 +394,28 @@ func (s *service) pluginInstallPath(clientType, name string, scope plugins.Scope return s.clientManager.GetPluginPath(client.ClientApp(clientType), name, scope, projectRoot) } -// dematerializeAll best-effort reverts materializations performed in this call. -// Errors are joined so a partial rollback still surfaces; the original install -// error is returned to the caller separately. +// dematerializeAll reverts materializations performed in this call. +// Errors are joined so a partial rollback still surfaces. func (s *service) dematerializeAll( ctx context.Context, clientTypes []string, name string, scope plugins.Scope, projectRoot string, -) { +) error { + var errs []error for _, ct := range clientTypes { if adapter, ok := s.materializers[ct]; ok { - _ = adapter.Dematerialize(ctx, plugins.DematerializeRequest{ + if err := adapter.Dematerialize(ctx, plugins.DematerializeRequest{ Name: name, Scope: scope, ProjectRoot: projectRoot, - }) + }); err != nil { + errs = append(errs, fmt.Errorf("dematerializing plugin for client %q: %w", ct, err)) + } } } + return errors.Join(errs...) } // resolveAndValidateClients returns the deduplicated client list to target for diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index 70908a1db4..c94deabad6 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive-core/httperr" ociartifact "github.com/stacklok/toolhive-core/oci/artifact" + "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/groups" groupmocks "github.com/stacklok/toolhive/pkg/groups/mocks" "github.com/stacklok/toolhive/pkg/plugins" @@ -172,7 +173,8 @@ func TestInstallWithExtraction(t *testing.T) { return nil }) - svc := newTestService(WithStore(store), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) + svc := newTestService(WithStore(store), WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) result, err := svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", LayerData: layerData, @@ -419,10 +421,11 @@ func TestInstallWithExtraction(t *testing.T) { return nil }) - svc := newTestService(WithStore(store), WithMaterializers(map[string]plugins.MaterializationAdapter{ - "claude-code": adapterA, - "codex": adapterB, - })) + svc := newTestService(WithStore(store), WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + WithMaterializers(map[string]plugins.MaterializationAdapter{ + "claude-code": adapterA, + "codex": adapterB, + })) result, err := svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", LayerData: layerData, @@ -432,6 +435,31 @@ func TestInstallWithExtraction(t *testing.T) { require.NoError(t, err) assert.ElementsMatch(t, []string{"claude-code", "codex"}, result.Plugin.Clients) }) + + t.Run("upgrade without client manager aborts before mutation", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockPluginStore(ctrl) + adapter := plugmocks.NewMockMaterializationAdapter(ctrl) + + existing := plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Digest: "sha256:old", + Clients: []string{"claude-code"}, + } + store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) + + svc := newTestService(WithStore(store), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: layerData, + Digest: "sha256:new", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolving") + assert.Contains(t, err.Error(), "install path") + }) } func TestInstallRoundTrip(t *testing.T) { diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 7fa3f2c78c..cdf918a55d 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -61,17 +61,16 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) if len(cleanupErrs) > 0 && restoreLock != nil { - restoreLock() - if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients); restoreErr != nil { - cleanupErrs = append(cleanupErrs, restoreErr) - } - return errors.Join(cleanupErrs...) + return errors.Join(append(cleanupErrs, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + ))...) } if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { if restoreLock != nil { - restoreLock() - _ = s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients) + return errors.Join(err, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + )) } return err } @@ -85,6 +84,21 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return errors.Join(cleanupErrs...) } +// compensateManagedUninstall restores the lock pin and every snapshotted +// client tree after a failed managed uninstall step. +func (s *service) compensateManagedUninstall( + ctx context.Context, + restoreLock func(), + name string, + scope plugins.Scope, + projectRoot string, + backups map[string]map[string]fileSnapshot, + clients []string, +) error { + restoreLock() + return s.restoreClientTrees(ctx, name, scope, projectRoot, backups, clients) +} + // removeManagedLockEntry removes the plugins: lock entry for a lock-managed // project-scope install, returning a restore func that reinstates the // snapshotted entry. The restore func is nil when no entry was removed. From e4d797bd3d3d35f63e0c893df3b353e9dab1acbf Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 17:14:24 +0200 Subject: [PATCH 5/9] Join lock restore errors on uninstall rollback Compensation must surface a failed pin restore, and a Materialize that extracts then fails has to clean that client. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/install_extraction.go | 10 ++- pkg/plugins/pluginsvc/lock_test.go | 71 ++++++++++++++++++++- pkg/plugins/pluginsvc/uninstall.go | 49 ++++++++------ 3 files changed, 107 insertions(+), 23 deletions(-) diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 864454def8..cff53e03a6 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -198,7 +198,9 @@ func (s *service) installExtractionFresh( } // materializeForClients calls Materialize for each requested client type, -// rolling back (Dematerialize) any already-materialized client on failure. +// rolling back (Dematerialize) any already-materialized client on failure, +// including the client whose Materialize returned an error (extraction can +// succeed before marketplace/settings registration fails). // Returns the list of client types that were successfully materialized. func (s *service) materializeForClients( ctx context.Context, @@ -223,8 +225,12 @@ func (s *service) materializeForClients( ProjectRoot: opts.ProjectRoot, Components: opts.Components, }); err != nil { + // Materialize can extract the tree and then fail during + // marketplace/settings registration. Compensate the failing + // client too, not only the ones already appended. + failed := append(append([]string{}, materialized...), ct) wrapped := fmt.Errorf("materializing plugin for client %q: %w", ct, err) - return nil, errors.Join(wrapped, s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot)) + return nil, errors.Join(wrapped, s.dematerializeAll(ctx, failed, opts.Name, scope, opts.ProjectRoot)) } materialized = append(materialized, ct) } diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index 7910def382..e3d22e57ca 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -337,10 +337,14 @@ func (s *hookPluginStore) Delete(ctx context.Context, name string, scope plugins type failingDematerializeAdapter struct { plugins.MaterializationAdapter - err error + err error + after func() } -func (a *failingDematerializeAdapter) Dematerialize(context.Context, plugins.DematerializeRequest) error { +func (a *failingDematerializeAdapter) Dematerialize(_ context.Context, _ plugins.DematerializeRequest) error { + if a.after != nil { + a.after() + } return a.err } @@ -398,6 +402,30 @@ func TestUninstall_StoreDeleteFailureRestoresLockEntry(t *testing.T) { assert.NotNil(t, info.InstalledPlugin) } +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_LockRestoreErrorIsJoined(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["claude-code"] = &failingDematerializeAdapter{ + MaterializationAdapter: inner.materializers["claude-code"], + err: errors.New("permission denied"), + after: func() { + lockPath := filepath.Join(projectRoot, lockfile.FileName) + require.NoError(t, os.Remove(lockPath)) + require.NoError(t, os.Mkdir(lockPath, 0o755)) + }, + } + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "permission denied") + assert.Contains(t, err.Error(), "restoring lock entry") +} + func TestSnapshotRestore_PreservesExecutableMode(t *testing.T) { t.Parallel() @@ -450,6 +478,45 @@ func (*failingMaterializeAdapter) ScopeSupport() plugins.ScopeSupport { return plugins.ScopeSupport{} } +// extractThenFailAdapter extracts the plugin tree then fails, matching Claude +// Code's Materialize: ExtractPlugin succeeds, marketplace/settings write fails. +type extractThenFailAdapter struct { + extractingAdapter + err error +} + +func (a *extractThenFailAdapter) Materialize(ctx context.Context, req plugins.MaterializeRequest) (*plugins.MaterializeResult, error) { + if _, err := a.extractingAdapter.Materialize(ctx, req); err != nil { + return nil, err + } + return nil, a.err +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstall_MaterializeFailureAfterExtractRemovesTree(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + inner := svc.(*service) //nolint:forcetypeassert + base := filepath.Join(projectRoot, ".claude", "plugins") + inner.materializers["claude-code"] = &extractThenFailAdapter{ + extractingAdapter: extractingAdapter{base: base, installer: skills.NewInstaller()}, + err: errors.New("marketplace write failed"), + } + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "marketplace write failed") + + _, statErr := os.Stat(filepath.Join(base, "my-plugin")) + assert.ErrorIs(t, statErr, os.ErrNotExist, "the extracted tree must be dematerialized") +} + //nolint:paralleltest // uses t.Setenv via newLockTestService func TestInstallProjectScope_LockWriteFailureRemovesGroupMembership(t *testing.T) { svc, projectRoot := newLockTestService(t, true) diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index cdf918a55d..9cca0b013a 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -54,9 +54,12 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) } backups, snapErr := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, existing.Clients) - if snapErr != nil && restoreLock != nil { - restoreLock() - return fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr) + if snapErr != nil { + err := fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr) + if restoreLock != nil { + return errors.Join(err, restoreLock()) + } + return err } cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) @@ -88,15 +91,17 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) // client tree after a failed managed uninstall step. func (s *service) compensateManagedUninstall( ctx context.Context, - restoreLock func(), + restoreLock func() error, name string, scope plugins.Scope, projectRoot string, backups map[string]map[string]fileSnapshot, clients []string, ) error { - restoreLock() - return s.restoreClientTrees(ctx, name, scope, projectRoot, backups, clients) + return errors.Join( + restoreLock(), + s.restoreClientTrees(ctx, name, scope, projectRoot, backups, clients), + ) } // removeManagedLockEntry removes the plugins: lock entry for a lock-managed @@ -106,29 +111,35 @@ func removeManagedLockEntry( opts plugins.UninstallOptions, existing plugins.InstalledPlugin, scope plugins.Scope, -) (restore func(), err error) { +) (restore func() error, err error) { if scope != plugins.ScopeProject || !existing.Managed { return nil, nil } - var prevLock *lockfile.Entry - if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { - if lf, loadErr := lockfile.Load(root); loadErr == nil { - if e, ok := lf.GetPlugin(opts.Name); ok { - prevLock = &e - } - } + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return nil, fmt.Errorf("opening lock file root: %w", err) + } + lf, err := lockfile.Load(root) + if err != nil { + return nil, fmt.Errorf("loading lock file: %w", err) } + prev, hasPrev := lf.GetPlugin(opts.Name) if lockErr := removeLockEntry(opts); lockErr != nil { return nil, fmt.Errorf("updating project lock file: %w", lockErr) } - if prevLock == nil { - return func() {}, nil + if !hasPrev { + return func() error { return nil }, nil } - return func() { - if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { - _ = lockfile.UpsertPluginEntry(root, *prevLock) + return func() error { + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + return fmt.Errorf("restoring lock entry: %w", err) + } + if err := lockfile.UpsertPluginEntry(root, prev); err != nil { + return fmt.Errorf("restoring lock entry: %w", err) } + return nil }, nil } From b0820f6bc72b640cffc12a116ae9a4223f21b579 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 19:03:32 +0200 Subject: [PATCH 6/9] Close plugin install and uninstall transaction gaps Unmanaged uninstalls no longer snapshot without managed rollback, managed uninstalls require every adapter, and fresh installs restore pre-existing trees instead of dematerializing them on failure. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/install.go | 29 +++-- pkg/plugins/pluginsvc/install_extraction.go | 118 +++++++++++++------- pkg/plugins/pluginsvc/install_test.go | 1 + pkg/plugins/pluginsvc/lock_test.go | 94 +++++++++++++++- pkg/plugins/pluginsvc/uninstall.go | 69 +++++++++--- pkg/plugins/pluginsvc/uninstall_test.go | 67 +++++++++-- 6 files changed, 310 insertions(+), 68 deletions(-) diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index 4826150a1b..b4d62cbc1d 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -317,22 +317,37 @@ func (s *service) installAndRegister( // Snapshot the prior plugins: lock entry before anything below can write // one, so rollback can reinstate it rather than blindly deleting it. + // OpenRoot/Load failures are fatal: treating them as "no previous pin" + // would delete a pre-existing entry on compensation. Extraction has + // already mutated DB/files, so compensate those even when the lock + // snapshot itself fails. var prevEntry *lockfile.Entry if lockScoped { - if root, rootErr := lockfile.OpenRoot(opts.ProjectRoot); rootErr == nil { - if lf, loadErr := lockfile.Load(root); loadErr == nil { - if e, ok := lf.GetPlugin(pluginName); ok { - prevEntry = &e - } - } + root, rootErr := lockfile.OpenRoot(opts.ProjectRoot) + if rootErr != nil { + return nil, errors.Join( + fmt.Errorf("opening lock file root: %w", rootErr), + s.rollbackInstall(ctx, opts, result, pluginName, scope, false, nil, false, ""), + ) + } + lf, loadErr := lockfile.Load(root) + if loadErr != nil { + return nil, errors.Join( + fmt.Errorf("loading lock file: %w", loadErr), + s.rollbackInstall(ctx, opts, result, pluginName, scope, false, nil, false, ""), + ) + } + if e, ok := lf.GetPlugin(pluginName); ok { + prevEntry = &e } } var addedToGroup bool + groupName := resolvedGroupName(opts.Group) rollback := func() error { return s.rollbackInstall( ctx, opts, result, pluginName, scope, lockScoped, prevEntry, - addedToGroup, resolvedGroupName(opts.Group), + addedToGroup, groupName, ) } diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index cff53e03a6..c0fb1f1504 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -102,6 +102,9 @@ func isExtractionNoOp(existing plugins.InstalledPlugin, storeErr error, opts plu // installExtractionSameDigestNewClients materializes the plugin for clients // not already present at the same digest, then updates the DB record. +// When a ClientManager is available, pre-existing unmanaged trees on those +// clients are snapshotted so rollback can restore them; without one, +// compensation falls back to dematerialize-only (embedded/test services). func (s *service) installExtractionSameDigestNewClients( ctx context.Context, opts plugins.InstallOptions, @@ -113,29 +116,13 @@ func (s *service) installExtractionSameDigestNewClients( if len(toWrite) == 0 { return &plugins.InstallResult{Plugin: existing}, nil } - materialized, err := s.materializeForClients(ctx, opts, scope, toWrite) - if err != nil { - return nil, err - } - pl := buildInstalledPlugin(opts, scope, clientTypes, existing.Clients) - pl.Managed = existing.Managed - if err := s.store.Update(ctx, pl); err != nil { - if dmErr := s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot); dmErr != nil { - return nil, errors.Join(err, dmErr) - } - return nil, err - } - return &plugins.InstallResult{ - Plugin: pl, - RestoreFiles: func(ctx context.Context) error { - return s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - }, - }, nil + return s.materializeAndPersist(ctx, opts, scope, toWrite, clientTypes, existing.Clients, existing.Managed, false) } // installExtractionUpgradeDigest re-materializes the plugin for the union of // requested and existing clients (upgrades write to every client), then updates -// the DB record. +// the DB record. Existing trees are always snapshotted first so a failed +// upgrade can restore prior content and registration. func (s *service) installExtractionUpgradeDigest( ctx context.Context, opts plugins.InstallOptions, @@ -148,7 +135,7 @@ func (s *service) installExtractionUpgradeDigest( if snapErr != nil { return nil, fmt.Errorf("snapshotting installed plugin trees: %w", snapErr) } - if _, err := s.materializeForClients(ctx, opts, scope, allClients); err != nil { + if _, err := s.materializeForClients(ctx, opts, scope, allClients, false); err != nil { if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients); restoreErr != nil { return nil, errors.Join(err, restoreErr) } @@ -171,42 +158,90 @@ func (s *service) installExtractionUpgradeDigest( } // installExtractionFresh materializes the plugin for all requested clients, -// then creates the DB record. +// then creates the DB record. When a ClientManager is available, pre-existing +// unmanaged trees are snapshotted so Force overwrite can be rolled back. func (s *service) installExtractionFresh( ctx context.Context, opts plugins.InstallOptions, scope plugins.Scope, clientTypes []string, ) (*plugins.InstallResult, error) { - materialized, err := s.materializeForClients(ctx, opts, scope, clientTypes) + return s.materializeAndPersist(ctx, opts, scope, clientTypes, clientTypes, nil, false, true) +} + +// materializeAndPersist materializes targetClients and creates or updates the +// DB row. When a ClientManager is configured it snapshots those targets first +// and uses restoreClientTrees for compensation; otherwise it dematerializes +// only what this call wrote. +func (s *service) materializeAndPersist( + ctx context.Context, + opts plugins.InstallOptions, + scope plugins.Scope, + targetClients []string, + resultClients []string, + existingClients []string, + managed bool, + create bool, +) (*plugins.InstallResult, error) { + useSnapshot := s.clientManager != nil + var backups map[string]map[string]fileSnapshot + if useSnapshot { + var snapErr error + backups, snapErr = s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, targetClients) + if snapErr != nil { + return nil, fmt.Errorf("snapshotting plugin trees before install: %w", snapErr) + } + } + + materialized, err := s.materializeForClients(ctx, opts, scope, targetClients, !useSnapshot) if err != nil { + if useSnapshot { + if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, targetClients); restoreErr != nil { + return nil, errors.Join(err, restoreErr) + } + } return nil, err } - pl := buildInstalledPlugin(opts, scope, clientTypes, nil) - if err := s.store.Create(ctx, pl); err != nil { - if dmErr := s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot); dmErr != nil { + + pl := buildInstalledPlugin(opts, scope, resultClients, existingClients) + pl.Managed = managed + if create { + err = s.store.Create(ctx, pl) + } else { + err = s.store.Update(ctx, pl) + } + if err != nil { + if useSnapshot { + if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, targetClients); restoreErr != nil { + return nil, errors.Join(err, restoreErr) + } + } else if dmErr := s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot); dmErr != nil { return nil, errors.Join(err, dmErr) } return nil, err } - return &plugins.InstallResult{ - Plugin: pl, - RestoreFiles: func(ctx context.Context) error { - return s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - }, - }, nil + + restore := func(ctx context.Context) error { + if useSnapshot { + return s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, targetClients) + } + return s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + } + return &plugins.InstallResult{Plugin: pl, RestoreFiles: restore}, nil } -// materializeForClients calls Materialize for each requested client type, -// rolling back (Dematerialize) any already-materialized client on failure, -// including the client whose Materialize returned an error (extraction can -// succeed before marketplace/settings registration fails). +// materializeForClients calls Materialize for each requested client type. +// When compensate is true, a failure dematerializes every client that was +// written (including the failing client, whose Materialize can extract before +// marketplace/settings registration fails). When compensate is false, the +// caller is responsible for restoreClientTrees / dematerializeAll. // Returns the list of client types that were successfully materialized. func (s *service) materializeForClients( ctx context.Context, opts plugins.InstallOptions, scope plugins.Scope, clientTypes []string, + compensate bool, ) ([]string, error) { var materialized []string for _, ct := range clientTypes { @@ -216,7 +251,10 @@ func (s *service) materializeForClients( fmt.Errorf("no materializer configured for client %q", ct), http.StatusInternalServerError, ) - return nil, errors.Join(err, s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot)) + if compensate { + return nil, errors.Join(err, s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot)) + } + return nil, err } if _, err := adapter.Materialize(ctx, plugins.MaterializeRequest{ Name: opts.Name, @@ -225,11 +263,14 @@ func (s *service) materializeForClients( ProjectRoot: opts.ProjectRoot, Components: opts.Components, }); err != nil { + wrapped := fmt.Errorf("materializing plugin for client %q: %w", ct, err) + if !compensate { + return nil, wrapped + } // Materialize can extract the tree and then fail during // marketplace/settings registration. Compensate the failing // client too, not only the ones already appended. failed := append(append([]string{}, materialized...), ct) - wrapped := fmt.Errorf("materializing plugin for client %q: %w", ct, err) return nil, errors.Join(wrapped, s.dematerializeAll(ctx, failed, opts.Name, scope, opts.ProjectRoot)) } materialized = append(materialized, ct) @@ -257,7 +298,8 @@ func sanitizeFileMode(mode fs.FileMode) fs.FileMode { // so a later rollback can restore the previous materialization without // leaking temp directories. Missing directories are omitted (the client was // not yet installed). A walk/read error on an existing tree is returned so -// the caller can abort before mutating. +// the caller can abort before mutating. Path-resolution failures (including +// a missing ClientManager) abort rather than being treated as "not installed." func (s *service) snapshotClientTrees( name string, scope plugins.Scope, projectRoot string, clientTypes []string, ) (map[string]map[string]fileSnapshot, error) { diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index c94deabad6..10ea6f5400 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -269,6 +269,7 @@ func TestInstallWithExtraction(t *testing.T) { adapterA.EXPECT().Materialize(gomock.Any(), gomock.Any()).Return(&plugins.MaterializeResult{}, nil) adapterB.EXPECT().Materialize(gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("disk full")) adapterA.EXPECT().Dematerialize(gomock.Any(), plugins.DematerializeRequest{Name: "my-plugin", Scope: plugins.ScopeUser}).Return(nil) + adapterB.EXPECT().Dematerialize(gomock.Any(), plugins.DematerializeRequest{Name: "my-plugin", Scope: plugins.ScopeUser}).Return(nil) svc := newTestService(WithStore(store), WithMaterializers(map[string]plugins.MaterializationAdapter{ "claude-code": adapterA, diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index e3d22e57ca..1e7a14ab53 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -534,6 +534,13 @@ func TestInstallProjectScope_LockWriteFailureRemovesGroupMembership(t *testing.T gm.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( func(_ context.Context, g *groups.Group) error { members = append([]string(nil), g.Plugins...) + if len(g.Plugins) > 0 { + // After group registration (and the prior-lock snapshot), make + // the lock path unwritable so recordLockState fails. + lockPath := filepath.Join(projectRoot, lockfile.FileName) + _ = os.Remove(lockPath) + require.NoError(t, os.MkdirAll(lockPath, 0o755)) + } return nil }, ).Times(2) @@ -541,7 +548,6 @@ func TestInstallProjectScope_LockWriteFailureRemovesGroupMembership(t *testing.T inner := svc.(*service) //nolint:forcetypeassert inner.groupManager = gm - require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) _, err := svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", LayerData: makePluginLayerData(t, "my-plugin"), @@ -669,3 +675,89 @@ func TestUninstall_PartialDematerializeRestoresAllClients(t *testing.T) { _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") assert.True(t, ok, "the lock entry must be restored") } + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallFresh_LockWriteFailureRestoresPreexistingTree(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + cm := client.NewTestClientManagerWithHome(t.TempDir()) + inner := svc.(*service) //nolint:forcetypeassert + inner.clientManager = cm + + pluginDir, err := cm.GetPluginPath(client.ClaudeCode, "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Join(pluginDir, "commands"), 0o750)) + prior := []byte("# prior unmanaged") + require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "commands", "hello.md"), prior, 0o644)) + + // Make lock writes fail after extraction/DB create. + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, + }) + require.Error(t, err) + + got, readErr := os.ReadFile(filepath.Join(pluginDir, "commands", "hello.md")) //nolint:gosec + require.NoError(t, readErr) + assert.Equal(t, string(prior), string(got), + "a failed Force install must restore the pre-existing unmanaged tree") + + _, infoErr := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.ErrorIs(t, infoErr, storage.ErrNotFound) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallAndRegister_LockSnapshotFailureRollsBackDB(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + + // A lock path that is a directory makes Load fail after extraction. + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "loading lock file") + + info, infoErr := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.ErrorIs(t, infoErr, storage.ErrNotFound) + assert.Nil(t, info) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestUninstall_ManagedMissingMaterializerAborts(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + delete(inner.materializers, "claude-code") + + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "no materializer configured") + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.True(t, ok, "the lock pin must remain when uninstall is refused") + info, err := svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + require.NotNil(t, info.InstalledPlugin) +} diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 9cca0b013a..24f270fe01 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -21,9 +21,13 @@ import ( // via errors.Join so a single client failure does not abort cleanup of the // others, and the DB record is still deleted. For lock-managed project-scope // installs the lock entry is removed first after snapshotting every client -// tree; a later dematerialize or DB-delete failure restores the pin, the -// plugin trees, and adapter registration so the plugin is not left -// installed-but-untracked or half-removed across clients. +// tree; a later dematerialize, group-cleanup, or DB-delete failure restores +// the pin, the plugin trees, and adapter registration so the plugin is not +// left installed-but-untracked or half-removed across clients. +// +// Tree snapshots run only when managed rollback is available: unmanaged and +// user-scope uninstalls must not require a ClientManager just to take unused +// backups. func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) error { if err := plugins.ValidatePluginName(opts.Name); err != nil { return httperr.WithCode(err, http.StatusBadRequest) @@ -48,18 +52,28 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return err } + managed := scope == plugins.ScopeProject && existing.Managed + if managed { + if err := s.requireMaterializers(existing.Clients); err != nil { + return err + } + } + restoreLock, err := removeManagedLockEntry(opts, existing, scope) if err != nil { return err } - backups, snapErr := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, existing.Clients) - if snapErr != nil { - err := fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr) - if restoreLock != nil { - return errors.Join(err, restoreLock()) + var backups map[string]map[string]fileSnapshot + if restoreLock != nil { + var snapErr error + backups, snapErr = s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, existing.Clients) + if snapErr != nil { + return errors.Join( + fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr), + restoreLock(), + ) } - return err } cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) @@ -69,6 +83,21 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) ))...) } + // Remove group membership before deleting the DB row so a failed group + // cleanup remains retryable (Uninstall is a no-op once the record is gone). + if s.groupManager != nil { + if groupErr := groups.RemovePluginFromAllGroups(ctx, s.groupManager, opts.Name); groupErr != nil { + groupErr = fmt.Errorf("removing plugin from groups: %w", groupErr) + if restoreLock != nil { + return errors.Join(groupErr, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + )) + } + cleanupErrs = append(cleanupErrs, groupErr) + return errors.Join(cleanupErrs...) + } + } + if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { if restoreLock != nil { return errors.Join(err, s.compensateManagedUninstall( @@ -78,13 +107,23 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) return err } - if s.groupManager != nil { - if groupErr := groups.RemovePluginFromAllGroups(ctx, s.groupManager, opts.Name); groupErr != nil { - cleanupErrs = append(cleanupErrs, fmt.Errorf("removing plugin from groups: %w", groupErr)) + return errors.Join(cleanupErrs...) +} + +// requireMaterializers fails closed when a managed uninstall would delete the +// lock/DB while leaving an executable tree behind because no adapter can +// dematerialize a recorded client. Unmanaged uninstall keeps the historical +// skip-missing-adapter behavior. +func (s *service) requireMaterializers(clients []string) error { + for _, clientType := range clients { + if _, ok := s.materializers[clientType]; !ok { + return httperr.WithCode( + fmt.Errorf("no materializer configured for client %q; refusing managed uninstall", clientType), + http.StatusInternalServerError, + ) } } - - return errors.Join(cleanupErrs...) + return nil } // compensateManagedUninstall restores the lock pin and every snapshotted @@ -144,7 +183,7 @@ func removeManagedLockEntry( } // dematerializeClients best-effort removes on-disk copies for each client the -// plugin was installed into. Missing adapters are skipped. +// plugin was installed into. Missing adapters are skipped (unmanaged path). func (s *service) dematerializeClients( ctx context.Context, existing plugins.InstalledPlugin, diff --git a/pkg/plugins/pluginsvc/uninstall_test.go b/pkg/plugins/pluginsvc/uninstall_test.go index f2706b4baf..216b8d8960 100644 --- a/pkg/plugins/pluginsvc/uninstall_test.go +++ b/pkg/plugins/pluginsvc/uninstall_test.go @@ -96,9 +96,10 @@ func TestUninstall(t *testing.T) { assert.Contains(t, err.Error(), "db locked") }) - // RemovePluginFromAllGroups fails: the error is joined into the final - // result (store.Delete already succeeded). - t.Run("group removal failure joins into result", func(t *testing.T) { + // RemovePluginFromAllGroups fails before the DB delete so the record + // remains and uninstall can be retried; dematerialize may already have + // run (best-effort) and its errors are joined when present. + t.Run("group removal failure aborts before store delete", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) store := storemocks.NewMockPluginStore(ctrl) @@ -111,8 +112,7 @@ func TestUninstall(t *testing.T) { } store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil) - store.EXPECT().Delete(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(nil) - // RemovePluginFromAllGroups calls List then Update for each matching group. + // Delete must not run — the DB row is what makes retry possible. gm.EXPECT().List(gomock.Any()).Return(nil, errors.New("etcd unavailable")) svc := newTestService(WithStore(store), WithGroupManager(gm), @@ -123,8 +123,9 @@ func TestUninstall(t *testing.T) { assert.Contains(t, err.Error(), "etcd unavailable") }) - // A missing materializer for a stored client type is skipped (not an error); - // the remaining clients dematerialize and the record is deleted. + // A missing materializer for a stored client type is skipped (not an error) + // on unmanaged uninstall; the remaining clients dematerialize and the + // record is deleted. t.Run("missing materializer for stored client is skipped", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -201,4 +202,56 @@ func TestUninstall(t *testing.T) { err := svc.Uninstall(t.Context(), plugins.UninstallOptions{Name: "my-plugin"}) require.NoError(t, err) }) + + // Unmanaged uninstall must not require a ClientManager just to take an + // unused tree snapshot (regression: client manager is not configured). + t.Run("unmanaged uninstall without client manager dematerializes and deletes", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockPluginStore(ctrl) + adapter := plugmocks.NewMockMaterializationAdapter(ctrl) + + existing := plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Clients: []string{"claude-code"}, + } + store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) + adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().Delete(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(nil) + + svc := newTestService(WithStore(store), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{Name: "my-plugin"}) + require.NoError(t, err) + }) + + // Managed uninstall refuses to delete the pin/DB when a recorded client + // has no materializer, so executable trees are not left orphaned. + t.Run("managed uninstall refuses missing materializer", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockPluginStore(ctrl) + + projectRoot := makeProjectRoot(t) + existing := plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Clients: []string{"claude-code", "ghost-client"}, + Managed: true, + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + } + store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeProject, projectRoot). + Return(existing, nil) + + svc := newTestService(WithStore(store), + WithMaterializers(map[string]plugins.MaterializationAdapter{ + "claude-code": plugmocks.NewMockMaterializationAdapter(ctrl), + })) + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "no materializer configured for client \"ghost-client\"") + assert.Equal(t, http.StatusInternalServerError, httperr.Code(err)) + }) } From 54e2b4682440741f3933a7cbaf10a797fa2ac2e0 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 19:29:29 +0200 Subject: [PATCH 7/9] Reduce uninstall cyclomatic complexity for lint Split lock-held uninstall into helpers so managed compensation paths stay readable under the gocyclo budget. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/uninstall.go | 57 ++++++++++++++++++++++-------- 1 file changed, 42 insertions(+), 15 deletions(-) diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 24f270fe01..5d489bb3d0 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -51,9 +51,18 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) } return err } + return s.uninstallExisting(ctx, opts, scope, existing) +} - managed := scope == plugins.ScopeProject && existing.Managed - if managed { +// uninstallExisting performs uninstall for a looked-up store record under the +// per-plugin lock. +func (s *service) uninstallExisting( + ctx context.Context, + opts plugins.UninstallOptions, + scope plugins.Scope, + existing plugins.InstalledPlugin, +) error { + if scope == plugins.ScopeProject && existing.Managed { if err := s.requireMaterializers(existing.Clients); err != nil { return err } @@ -83,19 +92,11 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) ))...) } - // Remove group membership before deleting the DB row so a failed group - // cleanup remains retryable (Uninstall is a no-op once the record is gone). - if s.groupManager != nil { - if groupErr := groups.RemovePluginFromAllGroups(ctx, s.groupManager, opts.Name); groupErr != nil { - groupErr = fmt.Errorf("removing plugin from groups: %w", groupErr) - if restoreLock != nil { - return errors.Join(groupErr, s.compensateManagedUninstall( - ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, - )) - } - cleanupErrs = append(cleanupErrs, groupErr) - return errors.Join(cleanupErrs...) + if err := s.removePluginGroups(ctx, opts.Name, restoreLock, scope, opts.ProjectRoot, backups, existing.Clients); err != nil { + if restoreLock != nil { + return err } + return errors.Join(append(cleanupErrs, err)...) } if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { @@ -106,10 +107,36 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) } return err } - return errors.Join(cleanupErrs...) } +// removePluginGroups removes the plugin from all groups before DB delete so a +// failed cleanup remains retryable. Managed failures compensate lock/files. +func (s *service) removePluginGroups( + ctx context.Context, + name string, + restoreLock func() error, + scope plugins.Scope, + projectRoot string, + backups map[string]map[string]fileSnapshot, + clients []string, +) error { + if s.groupManager == nil { + return nil + } + groupErr := groups.RemovePluginFromAllGroups(ctx, s.groupManager, name) + if groupErr == nil { + return nil + } + groupErr = fmt.Errorf("removing plugin from groups: %w", groupErr) + if restoreLock != nil { + return errors.Join(groupErr, s.compensateManagedUninstall( + ctx, restoreLock, name, scope, projectRoot, backups, clients, + )) + } + return groupErr +} + // requireMaterializers fails closed when a managed uninstall would delete the // lock/DB while leaving an executable tree behind because no adapter can // dematerialize a recorded client. Unmanaged uninstall keeps the historical From d17c95252cb945dfa80ee8f18887f2ff9e878b1f Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 18 Aug 2026 10:01:39 +0200 Subject: [PATCH 8/9] Restore exact group and registration state on rollback Uninstall snapshots memberships so partial group removal or a DB delete failure re-adds them, and tree restores replay the adapter Health probe instead of registering unmanaged trees. Signed-off-by: Samuele Verzi --- pkg/plugins/adapter.go | 6 ++ pkg/plugins/adapters/claudecode.go | 80 +++++++++++++++++ pkg/plugins/adapters/codex.go | 24 ++++++ pkg/plugins/mocks/mock_adapter.go | 14 +++ pkg/plugins/pluginsvc/install_extraction.go | 73 +++++++++++----- pkg/plugins/pluginsvc/lock_test.go | 95 +++++++++++++++++++++ pkg/plugins/pluginsvc/uninstall.go | 82 ++++++++++++------ pkg/plugins/pluginsvc/uninstall_test.go | 94 +++++++++++++++++++- 8 files changed, 420 insertions(+), 48 deletions(-) diff --git a/pkg/plugins/adapter.go b/pkg/plugins/adapter.go index df72d84fbd..e7c3720f31 100644 --- a/pkg/plugins/adapter.go +++ b/pkg/plugins/adapter.go @@ -108,6 +108,12 @@ type MaterializationAdapter interface { // so a failed upgrade does not leave the client unable to discover the // plugin. Must be idempotent. EnsureRegistered(ctx context.Context, req DematerializeRequest) error + // Health reports whether this plugin is fully materialized for the client: + // the plugin tree exists and any required marketplace or settings + // registration is present. A missing directory or registration is an + // error. Health must not hash shared files into a content digest — it is + // a presence check, not a pin. + Health(ctx context.Context, req DematerializeRequest) error // SupportedComponents returns the component types this adapter loads. SupportedComponents() []ComponentType // ScopeSupport reports whether a project-scoped install degrades for this diff --git a/pkg/plugins/adapters/claudecode.go b/pkg/plugins/adapters/claudecode.go index 4064073053..04ba62fbc7 100644 --- a/pkg/plugins/adapters/claudecode.go +++ b/pkg/plugins/adapters/claudecode.go @@ -129,6 +129,86 @@ func (a *ClaudeCodeAdapter) EnsureRegistered(_ context.Context, req plugins.Dema return a.registerPlugin(req.Name, req.Scope, req.ProjectRoot, dir) } +// Health reports whether the plugin directory exists and the marketplace.json +// plus settings.json entries that make Claude Code discover it are present. +func (a *ClaudeCodeAdapter) Health(_ context.Context, req plugins.DematerializeRequest) error { + dir, err := a.cm.GetPluginPath(client.ClaudeCode, req.Name, req.Scope, req.ProjectRoot) + if err != nil { + return fmt.Errorf("resolving plugin path: %w", err) + } + if _, err := os.Stat(dir); err != nil { + return fmt.Errorf("plugin directory missing: %w", err) + } + + mp, err := readClaudeMarketplace(claudeMarketplaceFilePath(filepath.Dir(dir))) + if err != nil { + return err + } + wantSource := "./" + req.Name + found := false + for _, p := range mp.Plugins { + if p.Name != req.Name { + continue + } + if p.Source != wantSource { + return fmt.Errorf("plugin %q marketplace source is %q, want %q", req.Name, p.Source, wantSource) + } + found = true + break + } + if !found { + return fmt.Errorf("plugin %q is missing from marketplace.json", req.Name) + } + + settingsPath := a.settingsPath(req.Scope, req.ProjectRoot) + content, err := os.ReadFile(settingsPath) // #nosec G304 -- path is a known tool config file location + if err != nil { + return fmt.Errorf("reading settings.json: %w", err) + } + root, err := parseSettings(content, settingsPath) + if err != nil { + return err + } + return claudeMarketplaceRegistrationHealthy(root, req.Name, filepath.Dir(dir)) +} + +// claudeMarketplaceRegistrationHealthy reports whether settings.json has the +// enabledPlugins entry and the extraKnownMarketplaces.toolhive directory +// source that Claude Code needs to discover the plugin. +func claudeMarketplaceRegistrationHealthy(root map[string]any, pluginName, marketplaceRoot string) error { + enabled, ok := root["enabledPlugins"].(map[string]any) + if !ok { + return fmt.Errorf("plugin %q is not enabled in settings.json", pluginName) + } + val, ok := enabled[pluginKey(pluginName)] + if !ok { + return fmt.Errorf("plugin %q is not enabled in settings.json", pluginName) + } + if on, _ := val.(bool); !on { + return fmt.Errorf("plugin %q is disabled in settings.json", pluginName) + } + + marketplaces, ok := root["extraKnownMarketplaces"].(map[string]any) + if !ok { + return fmt.Errorf("extraKnownMarketplaces.toolhive is missing from settings.json") + } + tv, ok := marketplaces[marketplaceName].(map[string]any) + if !ok { + return fmt.Errorf("extraKnownMarketplaces.toolhive is missing from settings.json") + } + src, ok := tv["source"].(map[string]any) + if !ok { + return fmt.Errorf("extraKnownMarketplaces.toolhive source is missing from settings.json") + } + if src["source"] != "directory" { + return fmt.Errorf("extraKnownMarketplaces.toolhive source type is %v, want directory", src["source"]) + } + if src["path"] != marketplaceRoot { + return fmt.Errorf("extraKnownMarketplaces.toolhive path is %v, want %s", src["path"], marketplaceRoot) + } + return nil +} + // registerPlugin upserts the shared marketplace.json entry and enables the // plugin in settings.json. func (a *ClaudeCodeAdapter) registerPlugin(name string, scope plugins.Scope, projectRoot, pluginDir string) error { diff --git a/pkg/plugins/adapters/codex.go b/pkg/plugins/adapters/codex.go index b39dd0a8d6..8452294902 100644 --- a/pkg/plugins/adapters/codex.go +++ b/pkg/plugins/adapters/codex.go @@ -151,6 +151,30 @@ func (a *CodexAdapter) EnsureRegistered(_ context.Context, req plugins.Demateria return a.registerPlugin(req.Name, req.Scope, req.ProjectRoot) } +// Health reports whether the plugin directory exists and the shared Codex +// marketplace.json lists the plugin. +func (a *CodexAdapter) Health(_ context.Context, req plugins.DematerializeRequest) error { + pluginDir, err := a.cm.GetPluginPath(client.Codex, req.Name, req.Scope, req.ProjectRoot) + if err != nil { + return fmt.Errorf("resolving plugin path: %w", err) + } + if _, err := os.Stat(pluginDir); err != nil { + return fmt.Errorf("plugin directory missing: %w", err) + } + + root := a.codexMarketplaceRoot(req.Scope, req.ProjectRoot) + mp, err := readCodexMarketplace(codexMarketplaceFile(root)) + if err != nil { + return err + } + for _, p := range mp.Plugins { + if p.Name == req.Name { + return nil + } + } + return fmt.Errorf("plugin %q is missing from marketplace.json", req.Name) +} + // registerPlugin upserts the shared Codex marketplace.json entry. func (a *CodexAdapter) registerPlugin(name string, scope plugins.Scope, projectRoot string) error { root := a.codexMarketplaceRoot(scope, projectRoot) diff --git a/pkg/plugins/mocks/mock_adapter.go b/pkg/plugins/mocks/mock_adapter.go index de2baedca0..3477e5e7e7 100644 --- a/pkg/plugins/mocks/mock_adapter.go +++ b/pkg/plugins/mocks/mock_adapter.go @@ -69,6 +69,20 @@ func (mr *MockMaterializationAdapterMockRecorder) EnsureRegistered(ctx, req any) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "EnsureRegistered", reflect.TypeOf((*MockMaterializationAdapter)(nil).EnsureRegistered), ctx, req) } +// Health mocks base method. +func (m *MockMaterializationAdapter) Health(ctx context.Context, req plugins.DematerializeRequest) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Health", ctx, req) + ret0, _ := ret[0].(error) + return ret0 +} + +// Health indicates an expected call of Health. +func (mr *MockMaterializationAdapterMockRecorder) Health(ctx, req any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Health", reflect.TypeOf((*MockMaterializationAdapter)(nil).Health), ctx, req) +} + // Materialize mocks base method. func (m *MockMaterializationAdapter) Materialize(ctx context.Context, req plugins.MaterializeRequest) (*plugins.MaterializeResult, error) { m.ctrl.T.Helper() diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index c0fb1f1504..0475971e3b 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -131,7 +131,7 @@ func (s *service) installExtractionUpgradeDigest( clientTypes []string, ) (*plugins.InstallResult, error) { allClients := mergeClientLists(existing.Clients, clientTypes) - backups, snapErr := s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, allClients) + backups, snapErr := s.snapshotClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, allClients) if snapErr != nil { return nil, fmt.Errorf("snapshotting installed plugin trees: %w", snapErr) } @@ -184,10 +184,10 @@ func (s *service) materializeAndPersist( create bool, ) (*plugins.InstallResult, error) { useSnapshot := s.clientManager != nil - var backups map[string]map[string]fileSnapshot + var backups map[string]clientTreeBackup if useSnapshot { var snapErr error - backups, snapErr = s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, targetClients) + backups, snapErr = s.snapshotClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, targetClients) if snapErr != nil { return nil, fmt.Errorf("snapshotting plugin trees before install: %w", snapErr) } @@ -285,6 +285,16 @@ type fileSnapshot struct { mode fs.FileMode } +// clientTreeBackup is one client's pre-mutation state: the plugin tree files +// plus whether the adapter reported the plugin as registered at snapshot +// time. Restore must reproduce that exact state — re-registering a tree that +// was never registered would make a failed install enable a previously +// undiscoverable unmanaged plugin. +type clientTreeBackup struct { + files map[string]fileSnapshot + registered bool +} + // snapshotFileModeMask strips setuid/setgid/sticky and caps at 0755, matching // skills.PluginFilePermissionMask so restored hooks keep +x without restoring // unsafe bits. @@ -296,14 +306,16 @@ func sanitizeFileMode(mode fs.FileMode) fs.FileMode { // snapshotClientTrees copies each client's installed plugin tree into memory // so a later rollback can restore the previous materialization without -// leaking temp directories. Missing directories are omitted (the client was -// not yet installed). A walk/read error on an existing tree is returned so -// the caller can abort before mutating. Path-resolution failures (including -// a missing ClientManager) abort rather than being treated as "not installed." +// leaking temp directories, recording alongside each tree whether the +// adapter considered the plugin registered at snapshot time. Missing +// directories are omitted (the client was not yet installed). A walk/read +// error on an existing tree is returned so the caller can abort before +// mutating. Path-resolution failures (including a missing ClientManager) +// abort rather than being treated as "not installed." func (s *service) snapshotClientTrees( - name string, scope plugins.Scope, projectRoot string, clientTypes []string, -) (map[string]map[string]fileSnapshot, error) { - backups := make(map[string]map[string]fileSnapshot, len(clientTypes)) + ctx context.Context, name string, scope plugins.Scope, projectRoot string, clientTypes []string, +) (map[string]clientTreeBackup, error) { + backups := make(map[string]clientTreeBackup, len(clientTypes)) var errs []error for _, ct := range clientTypes { dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) @@ -318,7 +330,15 @@ func (s *service) snapshotClientTrees( errs = append(errs, fmt.Errorf("snapshotting %s copy of %q: %w", ct, name, err)) continue } - backups[ct] = files + registered := false + if adapter, ok := s.materializers[ct]; ok { + registered = adapter.Health(ctx, plugins.DematerializeRequest{ + Name: name, + Scope: scope, + ProjectRoot: projectRoot, + }) == nil + } + backups[ct] = clientTreeBackup{files: files, registered: registered} } if len(errs) > 0 { return backups, errors.Join(errs...) @@ -329,31 +349,46 @@ func (s *service) snapshotClientTrees( return backups, nil } -// restoreClientTrees writes snapshotClientTrees backups back over the live -// plugin directories and re-registers each restored client so marketplace -// and settings entries match the restored tree. Clients without a backup are -// dematerialized (they were newly added by the failed install). +// restoreClientTrees restores each backed-up client to its exact snapshot +// state: any registration the failed operation added is cleared via +// Dematerialize, the snapshotted tree is rewritten, and marketplace/settings +// entries are re-registered only when the snapshot found them registered. +// Clients without a backup are dematerialized (they were newly added by the +// failed install). func (s *service) restoreClientTrees( ctx context.Context, name string, scope plugins.Scope, projectRoot string, - backups map[string]map[string]fileSnapshot, + backups map[string]clientTreeBackup, allClients []string, ) error { var errs []error restored := make(map[string]struct{}, len(backups)) - for ct, files := range backups { + for ct, backup := range backups { restored[ct] = struct{}{} dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) if err != nil { errs = append(errs, fmt.Errorf("resolving %s install path: %w", ct, err)) continue } - if err := restoreDir(dir, files); err != nil { + adapter, hasAdapter := s.materializers[ct] + // Clear whatever files/registration the failed operation left behind + // before rewriting the snapshot, so a tree that was unregistered at + // snapshot time does not stay registered after rollback. + if hasAdapter { + if err := adapter.Dematerialize(ctx, plugins.DematerializeRequest{ + Name: name, + Scope: scope, + ProjectRoot: projectRoot, + }); err != nil { + errs = append(errs, fmt.Errorf("clearing %s state before restore: %w", ct, err)) + } + } + if err := restoreDir(dir, backup.files); err != nil { errs = append(errs, fmt.Errorf("restoring %s plugin tree: %w", ct, err)) } - if adapter, ok := s.materializers[ct]; ok { + if hasAdapter && backup.registered { if err := adapter.EnsureRegistered(ctx, plugins.DematerializeRequest{ Name: name, Scope: scope, diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index 1e7a14ab53..0946490ebf 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -6,6 +6,7 @@ package pluginsvc import ( "context" "errors" + "fmt" "io/fs" "net/http" "os" @@ -54,6 +55,13 @@ func (*extractingAdapter) EnsureRegistered(context.Context, plugins.Dematerializ return nil } +func (a *extractingAdapter) Health(_ context.Context, req plugins.DematerializeRequest) error { + if _, err := os.Stat(filepath.Join(a.base, req.Name)); err != nil { + return fmt.Errorf("plugin directory missing: %w", err) + } + return nil +} + func (*extractingAdapter) SupportedComponents() []plugins.ComponentType { return []plugins.ComponentType{plugins.ComponentCommands} } @@ -470,6 +478,10 @@ func (*failingMaterializeAdapter) EnsureRegistered(context.Context, plugins.Dema return nil } +func (*failingMaterializeAdapter) Health(context.Context, plugins.DematerializeRequest) error { + return nil +} + func (*failingMaterializeAdapter) SupportedComponents() []plugins.ComponentType { return nil } @@ -714,6 +726,89 @@ func TestInstallFresh_LockWriteFailureRestoresPreexistingTree(t *testing.T) { require.ErrorIs(t, infoErr, storage.ErrNotFound) } +// registrationTrackingAdapter mimics Claude Code: Materialize extracts and +// registers, Dematerialize removes and deregisters, EnsureRegistered +// re-registers, Health fails unless registered. +type registrationTrackingAdapter struct { + extractingAdapter + registered bool +} + +func (a *registrationTrackingAdapter) Materialize( + ctx context.Context, req plugins.MaterializeRequest, +) (*plugins.MaterializeResult, error) { + res, err := a.extractingAdapter.Materialize(ctx, req) + if err != nil { + return nil, err + } + a.registered = true + return res, nil +} + +func (a *registrationTrackingAdapter) Dematerialize(ctx context.Context, req plugins.DematerializeRequest) error { + a.registered = false + return a.extractingAdapter.Dematerialize(ctx, req) +} + +func (a *registrationTrackingAdapter) EnsureRegistered(context.Context, plugins.DematerializeRequest) error { + a.registered = true + return nil +} + +func (a *registrationTrackingAdapter) Health(ctx context.Context, req plugins.DematerializeRequest) error { + if !a.registered { + return errors.New("plugin is not registered") + } + return a.extractingAdapter.Health(ctx, req) +} + +// A forced install over a pre-existing unmanaged tree that was NOT registered +// must not leave the plugin registered after rollback: restore reproduces the +// exact snapshot state (files present, registration absent). +// +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallFresh_RollbackDoesNotRegisterUnmanagedTree(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + cm := client.NewTestClientManagerWithHome(t.TempDir()) + inner := svc.(*service) //nolint:forcetypeassert + inner.clientManager = cm + tracking := ®istrationTrackingAdapter{ + extractingAdapter: extractingAdapter{ + base: filepath.Join(projectRoot, ".claude", "plugins"), + installer: skills.NewInstaller(), + }, + } + inner.materializers["claude-code"] = tracking + + pluginDir, err := cm.GetPluginPath(client.ClaudeCode, "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Join(pluginDir, "commands"), 0o750)) + prior := []byte("# prior unregistered unmanaged") + require.NoError(t, os.WriteFile(filepath.Join(pluginDir, "commands", "hello.md"), prior, 0o644)) + require.False(t, tracking.registered, "precondition: the unmanaged tree is not registered") + + // Make lock writes fail after extraction/DB create. + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, lockfile.FileName), 0o755)) + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# installed"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + Force: true, + }) + require.Error(t, err) + + got, readErr := os.ReadFile(filepath.Join(pluginDir, "commands", "hello.md")) //nolint:gosec + require.NoError(t, readErr) + assert.Equal(t, string(prior), string(got), + "a failed Force install must restore the pre-existing unmanaged tree") + assert.False(t, tracking.registered, + "rollback must not register a tree that was unregistered at snapshot time") +} + //nolint:paralleltest // uses t.Setenv via newLockTestService func TestInstallAndRegister_LockSnapshotFailureRollsBackDB(t *testing.T) { svc, projectRoot := newLockTestService(t, true) diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 5d489bb3d0..0af9cda568 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net/http" + "slices" "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/groups" @@ -73,10 +74,10 @@ func (s *service) uninstallExisting( return err } - var backups map[string]map[string]fileSnapshot + var backups map[string]clientTreeBackup if restoreLock != nil { var snapErr error - backups, snapErr = s.snapshotClientTrees(opts.Name, scope, opts.ProjectRoot, existing.Clients) + backups, snapErr = s.snapshotClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, existing.Clients) if snapErr != nil { return errors.Join( fmt.Errorf("snapshotting plugin trees before uninstall: %w", snapErr), @@ -92,49 +93,74 @@ func (s *service) uninstallExisting( ))...) } - if err := s.removePluginGroups(ctx, opts.Name, restoreLock, scope, opts.ProjectRoot, backups, existing.Clients); err != nil { + restoreGroups, groupErr := s.removePluginGroups(ctx, opts.Name) + if groupErr != nil { if restoreLock != nil { - return err + return errors.Join(groupErr, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + )) } - return errors.Join(append(cleanupErrs, err)...) + return errors.Join(append(cleanupErrs, groupErr)...) } if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { + restoreErrs := []error{err} + if restoreGroups != nil { + restoreErrs = append(restoreErrs, restoreGroups(ctx)) + } if restoreLock != nil { - return errors.Join(err, s.compensateManagedUninstall( + restoreErrs = append(restoreErrs, s.compensateManagedUninstall( ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, )) } - return err + return errors.Join(restoreErrs...) } return errors.Join(cleanupErrs...) } -// removePluginGroups removes the plugin from all groups before DB delete so a -// failed cleanup remains retryable. Managed failures compensate lock/files. +// removePluginGroups removes the plugin from every group that references it, +// before the DB delete so a failed cleanup remains retryable. Group updates +// run sequentially and can fail midway, so memberships are snapshotted first: +// a mid-removal failure re-adds the memberships already removed, and the +// returned restore func lets a later DB-delete failure reinstate all of them. +// The restore func is nil when the plugin belonged to no group. func (s *service) removePluginGroups( - ctx context.Context, - name string, - restoreLock func() error, - scope plugins.Scope, - projectRoot string, - backups map[string]map[string]fileSnapshot, - clients []string, -) error { + ctx context.Context, name string, +) (restore func(context.Context) error, err error) { if s.groupManager == nil { - return nil + return nil, nil } - groupErr := groups.RemovePluginFromAllGroups(ctx, s.groupManager, name) - if groupErr == nil { - return nil + all, err := s.groupManager.List(ctx) + if err != nil { + return nil, fmt.Errorf("removing plugin from groups: listing groups: %w", err) } - groupErr = fmt.Errorf("removing plugin from groups: %w", groupErr) - if restoreLock != nil { - return errors.Join(groupErr, s.compensateManagedUninstall( - ctx, restoreLock, name, scope, projectRoot, backups, clients, - )) + var members []string + for _, g := range all { + if slices.Contains(g.Plugins, name) { + members = append(members, g.Name) + } + } + if len(members) == 0 { + return nil, nil + } + restoreUpTo := func(ctx context.Context, upTo int) error { + var errs []error + for _, groupName := range members[:upTo] { + if _, addErr := groups.AddPluginToGroup(ctx, s.groupManager, groupName, name); addErr != nil { + errs = append(errs, fmt.Errorf("restoring plugin membership in group %q: %w", groupName, addErr)) + } + } + return errors.Join(errs...) + } + for i, groupName := range members { + if removeErr := groups.RemovePluginFromGroup(ctx, s.groupManager, groupName, name); removeErr != nil { + return nil, errors.Join( + fmt.Errorf("removing plugin from groups: group %q: %w", groupName, removeErr), + restoreUpTo(ctx, i), + ) + } } - return groupErr + return func(ctx context.Context) error { return restoreUpTo(ctx, len(members)) }, nil } // requireMaterializers fails closed when a managed uninstall would delete the @@ -161,7 +187,7 @@ func (s *service) compensateManagedUninstall( name string, scope plugins.Scope, projectRoot string, - backups map[string]map[string]fileSnapshot, + backups map[string]clientTreeBackup, clients []string, ) error { return errors.Join( diff --git a/pkg/plugins/pluginsvc/uninstall_test.go b/pkg/plugins/pluginsvc/uninstall_test.go index 216b8d8960..b7699eff8f 100644 --- a/pkg/plugins/pluginsvc/uninstall_test.go +++ b/pkg/plugins/pluginsvc/uninstall_test.go @@ -4,8 +4,10 @@ package pluginsvc import ( + "context" "errors" "net/http" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -123,6 +125,94 @@ func TestUninstall(t *testing.T) { assert.Contains(t, err.Error(), "etcd unavailable") }) + // A failure on the second group update restores the membership already + // removed from the first group before returning. + t.Run("second group update failure restores first membership", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockPluginStore(ctrl) + adapter := plugmocks.NewMockMaterializationAdapter(ctrl) + gm := groupmocks.NewMockManager(ctrl) + + existing := plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Clients: []string{"claude-code"}, + } + store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) + adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil) + + groupA := map[string][]string{"alpha": {"my-plugin", "other"}, "beta": {"my-plugin"}} + gm.EXPECT().List(gomock.Any()).Return([]*groups.Group{ + {Name: "alpha", Plugins: append([]string(nil), groupA["alpha"]...)}, + {Name: "beta", Plugins: append([]string(nil), groupA["beta"]...)}, + }, nil) + gm.EXPECT().Get(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, name string) (*groups.Group, error) { + return &groups.Group{Name: name, Plugins: append([]string(nil), groupA[name]...)}, nil + }, + ).AnyTimes() + gm.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, g *groups.Group) error { + if g.Name == "beta" && !slices.Contains(g.Plugins, "my-plugin") { + return errors.New("beta update failed") + } + groupA[g.Name] = append([]string(nil), g.Plugins...) + return nil + }, + ).AnyTimes() + + svc := newTestService(WithStore(store), WithGroupManager(gm), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{Name: "my-plugin"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "beta update failed") + assert.Contains(t, groupA["alpha"], "my-plugin", + "the membership removed from alpha must be restored after beta fails") + }) + + // A DB-delete failure after successful group removal restores every + // removed membership so the still-installed plugin stays attached. + t.Run("store delete failure restores group memberships", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockPluginStore(ctrl) + adapter := plugmocks.NewMockMaterializationAdapter(ctrl) + gm := groupmocks.NewMockManager(ctrl) + + existing := plugins.InstalledPlugin{ + Metadata: plugins.PluginMetadata{Name: "my-plugin"}, + Clients: []string{"claude-code"}, + } + store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) + adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil) + store.EXPECT().Delete(gomock.Any(), "my-plugin", plugins.ScopeUser, ""). + Return(errors.New("db locked")) + + memberships := map[string][]string{"alpha": {"my-plugin"}} + gm.EXPECT().List(gomock.Any()).Return([]*groups.Group{ + {Name: "alpha", Plugins: append([]string(nil), memberships["alpha"]...)}, + }, nil) + gm.EXPECT().Get(gomock.Any(), "alpha").DoAndReturn( + func(context.Context, string) (*groups.Group, error) { + return &groups.Group{Name: "alpha", Plugins: append([]string(nil), memberships["alpha"]...)}, nil + }, + ).AnyTimes() + gm.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, g *groups.Group) error { + memberships[g.Name] = append([]string(nil), g.Plugins...) + return nil + }, + ).AnyTimes() + + svc := newTestService(WithStore(store), WithGroupManager(gm), + WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) + err := svc.Uninstall(t.Context(), plugins.UninstallOptions{Name: "my-plugin"}) + require.Error(t, err) + assert.Contains(t, err.Error(), "db locked") + assert.Contains(t, memberships["alpha"], "my-plugin", + "a failed DB delete must restore the removed group membership") + }) + // A missing materializer for a stored client type is skipped (not an error) // on unmanaged uninstall; the remaining clients dematerialize and the // record is deleted. @@ -191,10 +281,12 @@ func TestUninstall(t *testing.T) { store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) adapter.EXPECT().Dematerialize(gomock.Any(), gomock.Any()).Return(nil) store.EXPECT().Delete(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(nil) - // RemovePluginFromAllGroups lists groups, finds one containing the plugin, updates it. + // Group removal lists memberships, then removes per group (Get+Update). gm.EXPECT().List(gomock.Any()).Return([]*groups.Group{ {Name: "mygroup", Plugins: []string{"my-plugin", "other"}}, }, nil) + gm.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&groups.Group{Name: "mygroup", Plugins: []string{"my-plugin", "other"}}, nil) gm.EXPECT().Update(gomock.Any(), gomock.Any()).Return(nil) svc := newTestService(WithStore(store), WithGroupManager(gm), From 9788c105866696d1485758c52ddbb2deb4187a82 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 19 Aug 2026 15:32:44 +0200 Subject: [PATCH 9/9] Address second-review feedback on install rollback Skip re-registration after a failed tree restore, preserve empty directories through hardened contained writes, degrade gracefully without a ClientManager everywhere, drop unreachable lock fallbacks and dead group helper, and exercise the prevEntry restore for real. Signed-off-by: Samuele Verzi --- docs/arch/14-plugins-system.md | 26 +++- pkg/groups/plugins.go | 26 ---- pkg/groups/plugins_test.go | 92 ------------- pkg/plugins/pluginsvc/install.go | 60 ++++---- pkg/plugins/pluginsvc/install_extraction.go | 108 ++++++++------- pkg/plugins/pluginsvc/install_oci.go | 3 +- pkg/plugins/pluginsvc/install_test.go | 11 +- pkg/plugins/pluginsvc/lock.go | 15 +- pkg/plugins/pluginsvc/lock_test.go | 144 ++++++++++++++++++-- pkg/plugins/pluginsvc/uninstall.go | 11 +- pkg/plugins/pluginsvc/uninstall_test.go | 2 +- 11 files changed, 277 insertions(+), 221 deletions(-) diff --git a/docs/arch/14-plugins-system.md b/docs/arch/14-plugins-system.md index 5f5dcf5a74..e6958b6127 100644 --- a/docs/arch/14-plugins-system.md +++ b/docs/arch/14-plugins-system.md @@ -25,8 +25,8 @@ different component subsets. │ per-(scope,name,projectRoot) pluginLock │ └───────────────┬─────────────────────────────────────────────┘ │ MaterializationAdapter interface - │ (Materialize / Dematerialize / SupportedComponents / - │ ScopeSupport) + │ (Materialize / Dematerialize / EnsureRegistered / + │ Health / SupportedComponents / ScopeSupport) ┌────────┴────────┐ ▼ ▼ ┌─────────────┐ ┌──────────────┐ @@ -114,13 +114,27 @@ After resolving the artifact, `installWithExtraction` resolves target clients, acquires the per-plugin lock, calls each client's `MaterializationAdapter`, builds the `InstalledPlugin` record, persists it (Create/Update with upgrade-digest/same-digest-new-clients branches), and registers the plugin in -its group. Failure rolls back already-materialized clients. +its group. When a `ClientManager` is configured, target trees are snapshotted +(files plus registration state via `Health`) before materialization; any later +failure — extraction, DB persist, group registration, or lock write — restores +the snapshot exactly (files, and `EnsureRegistered` only when the tree was +registered before) and joins every compensation error with the trigger. +Without a `ClientManager` (embedded/test services), compensation degrades to +dematerializing what this call wrote. ### 4. Uninstallation -`pluginsvc.Uninstall` calls `Dematerialize` per client (best-effort, -`errors.Join`), deletes the store record, and removes the plugin from all -groups. Idempotent. +`pluginsvc.Uninstall` is scope-dependent: + +- **Unmanaged / user scope**: `Dematerialize` per client (best-effort, + `errors.Join`), remove group memberships, then delete the store record. + Group removal snapshots memberships first and restores them if an update + midway or the DB delete fails, keeping the operation retryable. Idempotent. +- **Lock-managed project scope**: fails closed. Every stored client must have + a materializer, the lock entry is removed after snapshotting client trees, + and a later dematerialize/group/DB failure restores the pin, the trees, and + adapter registration so the plugin is never left half-removed or + installed-but-untracked. ### 5. Info diff --git a/pkg/groups/plugins.go b/pkg/groups/plugins.go index 063f706c82..81b0452281 100644 --- a/pkg/groups/plugins.go +++ b/pkg/groups/plugins.go @@ -55,29 +55,3 @@ func RemovePluginFromGroup(ctx context.Context, mgr Manager, groupName string, p } return nil } - -// RemovePluginFromAllGroups removes pluginName from every group that references it. -// It is a no-op when the plugin is not found in any group. -func RemovePluginFromAllGroups(ctx context.Context, mgr Manager, pluginName string) error { - allGroups, err := mgr.List(ctx) - if err != nil { - return fmt.Errorf("listing groups: %w", err) - } - - for _, group := range allGroups { - modified := false - for i, p := range group.Plugins { - if p == pluginName { - group.Plugins = append(group.Plugins[:i], group.Plugins[i+1:]...) - modified = true - break - } - } - if modified { - if err := mgr.Update(ctx, group); err != nil { - return fmt.Errorf("updating group %q: %w", group.Name, err) - } - } - } - return nil -} diff --git a/pkg/groups/plugins_test.go b/pkg/groups/plugins_test.go index 5e03ddd3b5..15d0fa04d7 100644 --- a/pkg/groups/plugins_test.go +++ b/pkg/groups/plugins_test.go @@ -101,98 +101,6 @@ func TestAddPluginToGroups(t *testing.T) { } } -func TestRemovePluginFromAllGroups(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - pluginName string - setupMock func(*groupmocks.MockManager) - wantErr string - }{ - { - name: "removes plugin from matching group", - pluginName: "my-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return([]*Group{ - {Name: "mygroup", Plugins: []string{"my-plugin", "other"}}, - }, nil) - m.EXPECT().Update(gomock.Any(), &Group{Name: "mygroup", Plugins: []string{"other"}}). - Return(nil) - }, - }, - { - name: "no-op when plugin is not in any group", - pluginName: "absent-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return([]*Group{ - {Name: "mygroup", Plugins: []string{"some-other-plugin"}}, - }, nil) - // No Update call expected. - }, - }, - { - name: "no-op when no groups exist", - pluginName: "my-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return([]*Group{}, nil) - }, - }, - { - name: "removes plugin from multiple groups", - pluginName: "shared", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return([]*Group{ - {Name: "group-a", Plugins: []string{"shared"}}, - {Name: "group-b", Plugins: []string{"shared", "other"}}, - }, nil) - m.EXPECT().Update(gomock.Any(), &Group{Name: "group-a", Plugins: []string{}}). - Return(nil) - m.EXPECT().Update(gomock.Any(), &Group{Name: "group-b", Plugins: []string{"other"}}). - Return(nil) - }, - }, - { - name: "returns error when List fails", - pluginName: "my-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return(nil, errors.New("store error")) - }, - wantErr: "listing groups", - }, - { - name: "returns error when Update fails", - pluginName: "my-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return([]*Group{ - {Name: "mygroup", Plugins: []string{"my-plugin"}}, - }, nil) - m.EXPECT().Update(gomock.Any(), gomock.Any()).Return(errors.New("write error")) - }, - wantErr: "updating group", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - ctrl := gomock.NewController(t) - mgr := groupmocks.NewMockManager(ctrl) - tt.setupMock(mgr) - - err := RemovePluginFromAllGroups(context.Background(), mgr, tt.pluginName) - - if tt.wantErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tt.wantErr) - } else { - require.NoError(t, err) - } - }) - } -} - func TestRemovePluginFromGroup(t *testing.T) { t.Parallel() diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index b4d62cbc1d..5e03eeaee8 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -22,7 +22,10 @@ import ( // (git://...), the repo is cloned and the plugin tree is built in memory. When // it contains an OCI reference, the artifact is pulled and extracted. A plain // name is resolved against the local OCI store, then the registry lookup. -// Mirror of skillsvc.Install, substituting the plugin install backends. +// Structural mirror of skillsvc.Install, substituting the plugin install +// backends — but the failure semantics deliberately diverge: skills discards +// rollback errors and fails forward, while plugins joins every compensation +// error with the trigger and can abort (see rollbackInstall). func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*plugins.InstallResult, error) { scope, projectRoot, err := normalizeProjectRoot(opts.Scope, opts.ProjectRoot) if err != nil { @@ -71,7 +74,8 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl // installByName handles installation for a validated plain plugin name. It // checks the local OCI store, then the registry lookup, before returning an -// error. Mirror of skillsvc.installByName. +// error. Structural mirror of skillsvc.installByName (failure semantics +// diverge — see Install). func (s *service) installByName( ctx context.Context, opts plugins.InstallOptions, @@ -327,14 +331,14 @@ func (s *service) installAndRegister( if rootErr != nil { return nil, errors.Join( fmt.Errorf("opening lock file root: %w", rootErr), - s.rollbackInstall(ctx, opts, result, pluginName, scope, false, nil, false, ""), + s.rollbackInstall(ctx, result, rollbackParams{}), ) } lf, loadErr := lockfile.Load(root) if loadErr != nil { return nil, errors.Join( fmt.Errorf("loading lock file: %w", loadErr), - s.rollbackInstall(ctx, opts, result, pluginName, scope, false, nil, false, ""), + s.rollbackInstall(ctx, result, rollbackParams{}), ) } if e, ok := lf.GetPlugin(pluginName); ok { @@ -345,10 +349,12 @@ func (s *service) installAndRegister( var addedToGroup bool groupName := resolvedGroupName(opts.Group) rollback := func() error { - return s.rollbackInstall( - ctx, opts, result, pluginName, scope, lockScoped, prevEntry, - addedToGroup, groupName, - ) + return s.rollbackInstall(ctx, result, rollbackParams{ + lockScoped: lockScoped, + prevEntry: prevEntry, + addedToGroup: addedToGroup, + groupName: groupName, + }) } added, err := s.registerPluginInGroup(ctx, opts.Group, pluginName) @@ -371,27 +377,35 @@ func (s *service) installAndRegister( return result, nil } +// rollbackParams carries the compensation state rollbackInstall needs beyond +// what the install result itself provides. Name, scope, and project root are +// derived from result.Plugin. +type rollbackParams struct { + lockScoped bool + prevEntry *lockfile.Entry + addedToGroup bool + groupName string +} + // rollbackInstall undoes installAndRegister's side effects after a failure. // Every compensation error is returned so the caller can join it with the // original failure; discarding it can hide a partial restore after a // destructive rewrite. func (s *service) rollbackInstall( ctx context.Context, - opts plugins.InstallOptions, result *plugins.InstallResult, - pluginName string, - scope plugins.Scope, - lockScoped bool, - prevEntry *lockfile.Entry, - addedToGroup bool, - groupName string, + params rollbackParams, ) error { + pluginName := result.Plugin.Metadata.Name + scope := result.Plugin.Scope + projectRoot := result.Plugin.ProjectRoot + var errs []error if result.PreExisting != nil { if err := s.store.Update(ctx, *result.PreExisting); err != nil { errs = append(errs, fmt.Errorf("restoring pre-existing DB record: %w", err)) } - } else if err := s.store.Delete(ctx, pluginName, scope, opts.ProjectRoot); err != nil { + } else if err := s.store.Delete(ctx, pluginName, scope, projectRoot); err != nil { errs = append(errs, fmt.Errorf("deleting rolled-back DB record: %w", err)) } @@ -401,27 +415,27 @@ func (s *service) rollbackInstall( } } - if addedToGroup && s.groupManager != nil { - if err := groups.RemovePluginFromGroup(ctx, s.groupManager, groupName, pluginName); err != nil { + if params.addedToGroup && s.groupManager != nil { + if err := groups.RemovePluginFromGroup(ctx, s.groupManager, params.groupName, pluginName); err != nil { errs = append(errs, fmt.Errorf("removing plugin from group: %w", err)) } } - if !lockScoped { + if !params.lockScoped { return errors.Join(errs...) } - if prevEntry != nil { - root, err := lockfile.OpenRoot(opts.ProjectRoot) + if params.prevEntry != nil { + root, err := lockfile.OpenRoot(projectRoot) if err != nil { return errors.Join(append(errs, fmt.Errorf("reopening lock file: %w", err))...) } - if err := lockfile.UpsertPluginEntry(root, *prevEntry); err != nil { + if err := lockfile.UpsertPluginEntry(root, *params.prevEntry); err != nil { errs = append(errs, fmt.Errorf("restoring lock entry: %w", err)) } return errors.Join(errs...) } if err := removeLockEntry(plugins.UninstallOptions{ - Name: pluginName, Scope: scope, ProjectRoot: opts.ProjectRoot, + Name: pluginName, Scope: scope, ProjectRoot: projectRoot, }); err != nil { errs = append(errs, fmt.Errorf("removing rolled-back lock entry: %w", err)) } diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 0475971e3b..106b9d5b51 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/client" + "github.com/stacklok/toolhive/pkg/fileutils" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/storage" ) @@ -121,8 +122,10 @@ func (s *service) installExtractionSameDigestNewClients( // installExtractionUpgradeDigest re-materializes the plugin for the union of // requested and existing clients (upgrades write to every client), then updates -// the DB record. Existing trees are always snapshotted first so a failed -// upgrade can restore prior content and registration. +// the DB record. Snapshot/restore compensation applies when a ClientManager is +// configured; without one (embedded/test services, WithClientManager is +// optional) compensation degrades to dematerialize-only, matching the fresh +// and same-digest paths. func (s *service) installExtractionUpgradeDigest( ctx context.Context, opts plugins.InstallOptions, @@ -131,30 +134,7 @@ func (s *service) installExtractionUpgradeDigest( clientTypes []string, ) (*plugins.InstallResult, error) { allClients := mergeClientLists(existing.Clients, clientTypes) - backups, snapErr := s.snapshotClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, allClients) - if snapErr != nil { - return nil, fmt.Errorf("snapshotting installed plugin trees: %w", snapErr) - } - if _, err := s.materializeForClients(ctx, opts, scope, allClients, false); err != nil { - if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients); restoreErr != nil { - return nil, errors.Join(err, restoreErr) - } - return nil, err - } - pl := buildInstalledPlugin(opts, scope, allClients, nil) - pl.Managed = existing.Managed - if err := s.store.Update(ctx, pl); err != nil { - if restoreErr := s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients); restoreErr != nil { - return nil, errors.Join(err, restoreErr) - } - return nil, err - } - return &plugins.InstallResult{ - Plugin: pl, - RestoreFiles: func(ctx context.Context) error { - return s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) - }, - }, nil + return s.materializeAndPersist(ctx, opts, scope, allClients, allClients, nil, existing.Managed, false) } // installExtractionFresh materializes the plugin for all requested clients, @@ -291,10 +271,22 @@ type fileSnapshot struct { // was never registered would make a failed install enable a previously // undiscoverable unmanaged plugin. type clientTreeBackup struct { - files map[string]fileSnapshot + tree treeSnapshot registered bool } +// treeSnapshot is one client tree captured by snapshotDir: its regular files +// plus every directory, so empty directories survive a restore. Symlinks and +// other non-regular entries are intentionally not captured — ExtractPlugin +// rejects symlinks at install time, so a ToolHive-managed tree never +// contains any. +type treeSnapshot struct { + files map[string]fileSnapshot + // dirs holds relative directory paths in walk order (parents before + // children), so restore can recreate empty directories. + dirs []string +} + // snapshotFileModeMask strips setuid/setgid/sticky and caps at 0755, matching // skills.PluginFilePermissionMask so restored hooks keep +x without restoring // unsafe bits. @@ -322,7 +314,7 @@ func (s *service) snapshotClientTrees( if err != nil { return nil, fmt.Errorf("resolving %s install path of %q: %w", ct, name, err) } - files, err := snapshotDir(dir) + tree, err := snapshotDir(dir) if err != nil { if errors.Is(err, os.ErrNotExist) { continue @@ -338,7 +330,7 @@ func (s *service) snapshotClientTrees( ProjectRoot: projectRoot, }) == nil } - backups[ct] = clientTreeBackup{files: files, registered: registered} + backups[ct] = clientTreeBackup{tree: tree, registered: registered} } if len(errs) > 0 { return backups, errors.Join(errs...) @@ -385,10 +377,14 @@ func (s *service) restoreClientTrees( errs = append(errs, fmt.Errorf("clearing %s state before restore: %w", ct, err)) } } - if err := restoreDir(dir, backup.files); err != nil { - errs = append(errs, fmt.Errorf("restoring %s plugin tree: %w", ct, err)) + restoreErr := restoreDir(dir, backup.tree) + if restoreErr != nil { + errs = append(errs, fmt.Errorf("restoring %s plugin tree: %w", ct, restoreErr)) } - if hasAdapter && backup.registered { + // Never re-register a tree whose restore failed: restoreDir removes + // the live tree before rewriting it, so a partial restore followed by + // EnsureRegistered would make the client load an incomplete plugin. + if hasAdapter && backup.registered && restoreErr == nil { if err := adapter.EnsureRegistered(ctx, plugins.DematerializeRequest{ Name: name, Scope: scope, @@ -413,16 +409,23 @@ func (s *service) restoreClientTrees( // snapshotDir reads every regular file under dir into a relative-path map, // preserving sanitized permission bits. Returns os.ErrNotExist when dir does // not exist. -func snapshotDir(dir string) (map[string]fileSnapshot, error) { +func snapshotDir(dir string) (treeSnapshot, error) { if _, err := os.Stat(dir); err != nil { - return nil, err + return treeSnapshot{}, err } - files := make(map[string]fileSnapshot) + snap := treeSnapshot{files: make(map[string]fileSnapshot)} err := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } + rel, relErr := filepath.Rel(dir, path) + if relErr != nil { + return relErr + } if d.IsDir() { + if rel != "." { + snap.dirs = append(snap.dirs, rel) + } return nil } info, infoErr := d.Info() @@ -432,37 +435,42 @@ func snapshotDir(dir string) (map[string]fileSnapshot, error) { if !info.Mode().IsRegular() { return nil } - rel, relErr := filepath.Rel(dir, path) - if relErr != nil { - return relErr - } data, readErr := os.ReadFile(path) //nolint:gosec // path is under a GetPluginPath-validated directory if readErr != nil { return readErr } - files[rel] = fileSnapshot{data: data, mode: sanitizeFileMode(info.Mode())} + snap.files[rel] = fileSnapshot{data: data, mode: sanitizeFileMode(info.Mode())} return nil }) if err != nil { - return nil, err + return treeSnapshot{}, err } - return files, nil + return snap, nil } -// restoreDir replaces dir with the files captured by snapshotDir, writing -// each file with its sanitized mode. -func restoreDir(dir string, files map[string]fileSnapshot) error { +// restoreDir replaces dir with the tree captured by snapshotDir: every +// directory is recreated (including empty ones) and every file is written +// through the contained atomic write path with its sanitized mode. +func restoreDir(dir string, tree treeSnapshot) error { + dir = filepath.Clean(dir) if err := os.RemoveAll(dir); err != nil { return err } + if err := os.MkdirAll(dir, 0o750); err != nil { + return err + } var errs []error - for rel, snap := range files { - path := filepath.Join(dir, rel) - if err := os.MkdirAll(filepath.Dir(path), 0o750); err != nil { - errs = append(errs, err) + for _, rel := range tree.dirs { + if !filepath.IsLocal(rel) { + errs = append(errs, fmt.Errorf("refusing to restore non-local directory path %q", rel)) continue } - if err := os.WriteFile(path, snap.data, snap.mode); err != nil { //nolint:gosec // mode is masked to 0755 + if err := os.MkdirAll(filepath.Join(dir, rel), 0o750); err != nil { + errs = append(errs, err) + } + } + for rel, snap := range tree.files { + if err := fileutils.WriteContainedFile(dir, rel, snap.data, 0o750, snap.mode); err != nil { errs = append(errs, err) } } diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index f2204a0d98..292962d951 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -20,7 +20,8 @@ import ( // installFromOCI pulls a plugin artifact from a remote registry, extracts // metadata and layer data, then materializes and registers the plugin while -// holding the per-plugin lock. Mirror of skillsvc.installFromOCI, substituting +// holding the per-plugin lock. Structural mirror of skillsvc.installFromOCI +// (failure semantics diverge — see Install), substituting // the plugin supply-chain check (config.Name == OCI repo last segment) and // hydrating Components/Dependencies from the plugin OCI config. func (s *service) installFromOCI( diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index 10ea6f5400..a4ccb82c48 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -437,7 +437,10 @@ func TestInstallWithExtraction(t *testing.T) { assert.ElementsMatch(t, []string{"claude-code", "codex"}, result.Plugin.Clients) }) - t.Run("upgrade without client manager aborts before mutation", func(t *testing.T) { + // WithClientManager is optional: without one, upgrades degrade to + // dematerialize-only compensation instead of failing on tree snapshots, + // matching the fresh and same-digest install paths. + t.Run("upgrade without client manager degrades to dematerialize-only", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) store := storemocks.NewMockPluginStore(ctrl) @@ -449,6 +452,9 @@ func TestInstallWithExtraction(t *testing.T) { Clients: []string{"claude-code"}, } store.EXPECT().Get(gomock.Any(), "my-plugin", plugins.ScopeUser, "").Return(existing, nil) + adapter.EXPECT().Materialize(gomock.Any(), gomock.Any()).Return(&plugins.MaterializeResult{}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db update error")) + adapter.EXPECT().Dematerialize(gomock.Any(), plugins.DematerializeRequest{Name: "my-plugin", Scope: plugins.ScopeUser}).Return(nil) svc := newTestService(WithStore(store), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) @@ -458,8 +464,7 @@ func TestInstallWithExtraction(t *testing.T) { Digest: "sha256:new", }) require.Error(t, err) - assert.Contains(t, err.Error(), "resolving") - assert.Contains(t, err.Error(), "install path") + assert.Contains(t, err.Error(), "db update error") }) } diff --git a/pkg/plugins/pluginsvc/lock.go b/pkg/plugins/pluginsvc/lock.go index 1c0c417525..84c7a15080 100644 --- a/pkg/plugins/pluginsvc/lock.go +++ b/pkg/plugins/pluginsvc/lock.go @@ -27,18 +27,15 @@ func (s *service) recordLockState( pl plugins.InstalledPlugin, contentDigest string, ) (plugins.InstalledPlugin, error) { + // contentDigest is always populated by installWithExtraction + // (lockContentDigest gates on the same ScopeProject + feature-flag + // condition as installAndRegister's lockScoped), and Install sets + // LockSource before any dispatch can rewrite opts.Name — so neither + // value needs a fallback here. if contentDigest == "" { - var err error - contentDigest, err = computeContentDigest(opts.LayerData) - if err != nil { - return pl, fmt.Errorf("computing content digest: %w", err) - } + return pl, fmt.Errorf("recording lock state for %q: content digest was not computed", pl.Metadata.Name) } - source := opts.LockSource - if source == "" { - source = opts.Name - } resolvedReference := opts.LockResolvedReference if resolvedReference == "" { resolvedReference = pl.Reference diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index 0946490ebf..dccb1c8ec7 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -226,6 +226,12 @@ func TestInstallProjectScope_LockWriteFailureRollsBackInstall(t *testing.T) { assert.True(t, os.IsNotExist(err), "a failed fresh install must dematerialize on rollback") } +// TestInstallProjectScope_RollbackRestoresPreExistingState exercises the real +// prevEntry restore branch: the failure is injected AFTER the lock entry was +// overwritten (recordLockState's managed-flag Update fails), so rollback must +// reinstate the previous pin, DB record, and files — not merely observe that +// nothing was written. +// //nolint:paralleltest // uses t.Setenv via newLockTestService func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { svc, projectRoot := newLockTestService(t, true) @@ -237,8 +243,31 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { beforeHello, err := os.ReadFile(helloPath) //nolint:gosec // test fixture path require.NoError(t, err) - require.NoError(t, os.Chmod(projectRoot, 0o555)) - t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) + // Drift state: the lock entry exists but the DB record is unmanaged, so + // recordLockState writes the lock entry and THEN updates the DB record — + // giving rollback a genuinely overwritten entry to restore. + inner := svc.(*service) //nolint:forcetypeassert + drifted, err := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + drifted.Managed = false + require.NoError(t, inner.store.Update(t.Context(), drifted)) + + var digestAtFailure string + inner.store = &hookPluginStore{ + PluginStore: inner.store, + beforeUpdate: func(call int) error { + // Call 1 persists the new digest (materializeAndPersist); call 2 + // is recordLockState marking the record managed — after the lock + // entry was already rewritten. Fail there. + if call == 2 { + if e, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin"); ok { + digestAtFailure = e.Digest + } + return errors.New("db update unavailable") + } + return nil + }, + } _, err = svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", @@ -248,9 +277,11 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { ProjectRoot: projectRoot, Clients: []string{"claude-code"}, }) - require.Error(t, err, "reinstall must fail when the lock file cannot be written") + require.Error(t, err, "reinstall must fail when marking the record managed fails") + assert.Contains(t, err.Error(), "db update unavailable") + assert.Equal(t, validLockDigestAlt(), digestAtFailure, + "precondition: the lock entry must have been overwritten before the injected failure") - require.NoError(t, os.Chmod(projectRoot, 0o755)) after, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") require.True(t, ok, "a transient failure must not destroy the pre-existing lock entry") assert.Equal(t, before.Digest, after.Digest, "the previous pin must be restored") @@ -266,6 +297,84 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { assert.Equal(t, beforeHello, afterHello) } +// A rollback whose own DB compensation fails must join that error with the +// trigger instead of reporting only the original failure. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_RollbackCompensationErrorIsJoined(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + drifted, err := inner.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + drifted.Managed = false + require.NoError(t, inner.store.Update(t.Context(), drifted)) + + // Fail recordLockState's managed-flag Update AND the rollback's + // pre-existing record restore that follows it. + inner.store = &hookPluginStore{ + PluginStore: inner.store, + beforeUpdate: func(call int) error { + if call >= 2 { + return errors.New("db update unavailable") + } + return nil + }, + } + + _, err = svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "recording plugin in project lock file", + "the trigger failure must be reported") + assert.Contains(t, err.Error(), "restoring pre-existing DB record", + "the failed compensation must be joined into the returned error") +} + +// A rollback must not remove a group membership this install did not add. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestInstallProjectScope_RollbackKeepsPreExistingGroupMembership(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + ctrl := gomock.NewController(t) + gm := groupmocks.NewMockManager(ctrl) + + // The plugin is already a member, so AddPluginToGroup reports added=false + // and rollback must leave the membership alone (no Update calls at all). + gm.EXPECT().Get(gomock.Any(), groups.DefaultGroup). + Return(&groups.Group{Name: groups.DefaultGroup, Plugins: []string{"my-plugin"}}, nil) + + inner := svc.(*service) //nolint:forcetypeassert + inner.groupManager = gm + + // Lock writes fail after group registration: the project root is + // read-only, so Load succeeds but recordLockEntry's write fails. + // Extraction still works because the plugin tree lives in a + // pre-existing writable subdirectory. + require.NoError(t, os.MkdirAll(filepath.Join(projectRoot, ".claude", "plugins"), 0o755)) + require.NoError(t, os.Chmod(projectRoot, 0o555)) + t.Cleanup(func() { _ = os.Chmod(projectRoot, 0o755) }) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: makePluginLayerData(t, "my-plugin"), + Digest: validLockDigest(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + require.Error(t, err, "install must fail when the lock entry cannot be written") + // gomock verifies no gm.Update ran: rollback did not touch the + // pre-existing membership it did not create. +} + //nolint:paralleltest // uses t.Setenv via newLockTestService func TestUninstall_RemovesPluginLockEntry(t *testing.T) { svc, projectRoot := newLockTestService(t, true) @@ -332,6 +441,10 @@ func TestUninstall_DoesNotTouchSkillsKey(t *testing.T) { type hookPluginStore struct { storage.PluginStore beforeDelete func() error + // beforeUpdate runs before each Update with the 1-based call count; + // returning an error fails that Update. + beforeUpdate func(call int) error + updateCalls int } func (s *hookPluginStore) Delete(ctx context.Context, name string, scope plugins.Scope, projectRoot string) error { @@ -343,6 +456,16 @@ func (s *hookPluginStore) Delete(ctx context.Context, name string, scope plugins return s.PluginStore.Delete(ctx, name, scope, projectRoot) } +func (s *hookPluginStore) Update(ctx context.Context, pl plugins.InstalledPlugin) error { + s.updateCalls++ + if s.beforeUpdate != nil { + if err := s.beforeUpdate(s.updateCalls); err != nil { + return err + } + } + return s.PluginStore.Update(ctx, pl) +} + type failingDematerializeAdapter struct { plugins.MaterializationAdapter err error @@ -446,13 +569,16 @@ func TestSnapshotRestore_PreservesExecutableMode(t *testing.T) { require.NoError(t, os.MkdirAll(filepath.Dir(md), 0o750)) require.NoError(t, os.WriteFile(md, []byte("# hello"), 0o644)) - files, err := snapshotDir(dir) + // An empty directory must survive the snapshot/restore round trip too. + require.NoError(t, os.MkdirAll(filepath.Join(dir, "skills", "empty"), 0o750)) + + tree, err := snapshotDir(dir) require.NoError(t, err) - assert.Equal(t, fs.FileMode(0o755), files[filepath.Join("hooks", "preinstall.sh")].mode) - assert.Equal(t, fs.FileMode(0o644), files[filepath.Join("commands", "hello.md")].mode) + assert.Equal(t, fs.FileMode(0o755), tree.files[filepath.Join("hooks", "preinstall.sh")].mode) + assert.Equal(t, fs.FileMode(0o644), tree.files[filepath.Join("commands", "hello.md")].mode) dest := t.TempDir() - require.NoError(t, restoreDir(dest, files)) + require.NoError(t, restoreDir(dest, tree)) info, err := os.Stat(filepath.Join(dest, "hooks", "preinstall.sh")) require.NoError(t, err) @@ -460,6 +586,8 @@ func TestSnapshotRestore_PreservesExecutableMode(t *testing.T) { mdInfo, err := os.Stat(filepath.Join(dest, "commands", "hello.md")) require.NoError(t, err) assert.Equal(t, fs.FileMode(0o644), mdInfo.Mode().Perm()) + assert.DirExists(t, filepath.Join(dest, "skills", "empty"), + "empty directories must be recreated on restore") } type failingMaterializeAdapter struct { diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 0af9cda568..33716f1b9a 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -74,8 +74,11 @@ func (s *service) uninstallExisting( return err } + // Tree snapshots need a ClientManager for path resolution; without one + // (WithClientManager is optional) managed compensation degrades to + // restoring the lock pin only, matching materializeAndPersist's policy. var backups map[string]clientTreeBackup - if restoreLock != nil { + if restoreLock != nil && s.clientManager != nil { var snapErr error backups, snapErr = s.snapshotClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, existing.Clients) if snapErr != nil { @@ -180,7 +183,8 @@ func (s *service) requireMaterializers(clients []string) error { } // compensateManagedUninstall restores the lock pin and every snapshotted -// client tree after a failed managed uninstall step. +// client tree after a failed managed uninstall step. Without a ClientManager +// no snapshots were taken, so only the lock pin is restored. func (s *service) compensateManagedUninstall( ctx context.Context, restoreLock func() error, @@ -190,6 +194,9 @@ func (s *service) compensateManagedUninstall( backups map[string]clientTreeBackup, clients []string, ) error { + if s.clientManager == nil { + return restoreLock() + } return errors.Join( restoreLock(), s.restoreClientTrees(ctx, name, scope, projectRoot, backups, clients), diff --git a/pkg/plugins/pluginsvc/uninstall_test.go b/pkg/plugins/pluginsvc/uninstall_test.go index b7699eff8f..31912d22c2 100644 --- a/pkg/plugins/pluginsvc/uninstall_test.go +++ b/pkg/plugins/pluginsvc/uninstall_test.go @@ -98,7 +98,7 @@ func TestUninstall(t *testing.T) { assert.Contains(t, err.Error(), "db locked") }) - // RemovePluginFromAllGroups fails before the DB delete so the record + // Group removal fails before the DB delete so the record // remains and uninstall can be retried; dematerialize may already have // run (best-effort) and its errors are joined when present. t.Run("group removal failure aborts before store delete", func(t *testing.T) {