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 0b655b801b..81b0452281 100644 --- a/pkg/groups/plugins.go +++ b/pkg/groups/plugins.go @@ -11,49 +11,47 @@ 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 fmt.Errorf("updating group %q: %w", groupName, err) + return false, fmt.Errorf("updating group %q: %w", groupName, err) } - return nil + return true, 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) +// 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("listing groups: %w", err) + return fmt.Errorf("getting group %q: %w", groupName, 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) - } - } + 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) } return nil } diff --git a/pkg/groups/plugins_test.go b/pkg/groups/plugins_test.go index 206ec29da1..15d0fa04d7 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,87 +87,65 @@ 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) } }) } } -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 matching group", + name: "removes plugin from group", + groupName: "mygroup", 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().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 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", + name: "no-op when plugin is not a member", + groupName: "mygroup", + pluginName: "absent", 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) + m.EXPECT().Get(gomock.Any(), "mygroup"). + Return(&Group{Name: "mygroup", Plugins: []string{"other"}}, nil) }, }, { - name: "returns error when List fails", + name: "no-op when group name is empty", + groupName: "", pluginName: "my-plugin", - setupMock: func(m *groupmocks.MockManager) { - m.EXPECT().List(gomock.Any()).Return(nil, errors.New("store error")) - }, - wantErr: "listing groups", + setupMock: func(_ *groupmocks.MockManager) {}, }, { - name: "returns error when Update fails", + name: "returns error when group not found", + groupName: "nonexistent", 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")) + m.EXPECT().Get(gomock.Any(), "nonexistent"). + Return(nil, errors.New("group not found")) }, - wantErr: "updating group", + wantErr: "getting group", }, } @@ -177,7 +157,7 @@ func TestRemovePluginFromAllGroups(t *testing.T) { mgr := groupmocks.NewMockManager(ctrl) tt.setupMock(mgr) - err := RemovePluginFromAllGroups(context.Background(), mgr, tt.pluginName) + err := RemovePluginFromGroup(context.Background(), mgr, tt.groupName, tt.pluginName) if tt.wantErr != "" { require.Error(t, err) diff --git a/pkg/plugins/adapter.go b/pkg/plugins/adapter.go index 47a8fc0d44..e7c3720f31 100644 --- a/pkg/plugins/adapter.go +++ b/pkg/plugins/adapter.go @@ -103,6 +103,17 @@ 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 + // 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 546ee5ec06..04ba62fbc7 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,110 @@ 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) +} + +// 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 { + 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..8452294902 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,45 @@ 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) +} + +// 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) + 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/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/mocks/mock_adapter.go b/pkg/plugins/mocks/mock_adapter.go index 3a201e74a4..3477e5e7e7 100644 --- a/pkg/plugins/mocks/mock_adapter.go +++ b/pkg/plugins/mocks/mock_adapter.go @@ -55,6 +55,34 @@ 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) +} + +// 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/options.go b/pkg/plugins/options.go index 0d9938d41b..190f5aa72f 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). @@ -44,12 +48,39 @@ 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:"-"` + // RestoreFiles undoes this install's on-disk writes: dematerialize a + // fresh install, or restore the previous tree after a failed upgrade. + // 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/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..5e03eeaee8 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" @@ -14,13 +15,17 @@ 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 // (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 { @@ -28,15 +33,15 @@ 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. + // 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, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) + return s.installFromGit(ctx, opts, scope) } // Splice opts.Version as the tag for tag-less OCI-like references. @@ -54,13 +59,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, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) - } - // 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. @@ -73,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, @@ -110,7 +112,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 @@ -226,11 +228,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, result, opts.Group, result.Plugin.Metadata.Name, scope, opts.ProjectRoot) + return s.installFromOCI(ctx, opts, scope, ref) } // selectOCIPluginPackage selects the first OCI package from a registry entry's @@ -283,10 +281,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 @@ -294,23 +293,151 @@ 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. +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, 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, 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) - return nil, fmt.Errorf("registering plugin in group: %w", err) + 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. + // 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 { + root, rootErr := lockfile.OpenRoot(opts.ProjectRoot) + if rootErr != nil { + return nil, errors.Join( + fmt.Errorf("opening lock file root: %w", rootErr), + 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, result, rollbackParams{}), + ) + } + if e, ok := lf.GetPlugin(pluginName); ok { + prevEntry = &e + } + } + + var addedToGroup bool + groupName := resolvedGroupName(opts.Group) + rollback := func() error { + return s.rollbackInstall(ctx, result, rollbackParams{ + lockScoped: lockScoped, + prevEntry: prevEntry, + addedToGroup: addedToGroup, + groupName: groupName, + }) + } + + added, err := s.registerPluginInGroup(ctx, opts.Group, pluginName) + if err != nil { + 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 { + return nil, httperr.WithCode( + errors.Join(fmt.Errorf("recording plugin in project lock file: %w", err), rollback()), + http.StatusInternalServerError, + ) + } + result.Plugin = updated } + 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, + result *plugins.InstallResult, + 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, projectRoot); err != nil { + errs = append(errs, fmt.Errorf("deleting rolled-back DB record: %w", err)) + } + + if result.RestoreFiles != nil { + if err := result.RestoreFiles(ctx); err != nil { + errs = append(errs, err) + } + } + + 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 !params.lockScoped { + return errors.Join(errs...) + } + 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, *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: 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 0e73395dc6..106b9d5b51 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -7,13 +7,17 @@ import ( "context" "errors" "fmt" + "io/fs" "net/http" + "os" + "path/filepath" "slices" "strings" "time" "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" ) @@ -40,6 +44,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 +89,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 @@ -69,6 +103,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, @@ -80,21 +117,15 @@ 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) - if err := s.store.Update(ctx, pl); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - return nil, err - } - return &plugins.InstallResult{Plugin: pl}, 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. 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, @@ -103,56 +134,107 @@ 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 { - return nil, err - } - pl := buildInstalledPlugin(opts, scope, allClients, nil) - if err := s.store.Update(ctx, pl); err != nil { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) - return nil, err - } - return &plugins.InstallResult{Plugin: pl}, nil + return s.materializeAndPersist(ctx, opts, scope, allClients, allClients, nil, existing.Managed, false) } // 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]clientTreeBackup + if useSnapshot { + var snapErr error + 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) + } + } + + 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 { - s.dematerializeAll(ctx, materialized, opts.Name, scope, opts.ProjectRoot) + + 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}, 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. +// 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 { 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, ) + 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, @@ -161,33 +243,270 @@ 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) + 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) + return nil, errors.Join(wrapped, s.dematerializeAll(ctx, failed, opts.Name, scope, opts.ProjectRoot)) } materialized = append(materialized, ct) } return materialized, nil } -// 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. +// 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 +} + +// 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 { + 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. +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, 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( + 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) + if err != nil { + return nil, fmt.Errorf("resolving %s install path of %q: %w", ct, name, err) + } + tree, 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 + } + 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{tree: tree, registered: registered} + } + if len(errs) > 0 { + return backups, errors.Join(errs...) + } + if len(backups) == 0 { + return nil, nil + } + return backups, nil +} + +// 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]clientTreeBackup, + allClients []string, +) error { + var errs []error + restored := make(map[string]struct{}, len(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 + } + 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)) + } + } + restoreErr := restoreDir(dir, backup.tree) + if restoreErr != nil { + errs = append(errs, fmt.Errorf("restoring %s plugin tree: %w", ct, restoreErr)) + } + // 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, + ProjectRoot: projectRoot, + }); err != nil { + errs = append(errs, fmt.Errorf("restoring %s registration: %w", ct, err)) + } + } + } + var extra []string + for _, ct := range allClients { + if _, ok := restored[ct]; !ok { + extra = append(extra, ct) + } + } + if err := s.dematerializeAll(ctx, extra, name, scope, projectRoot); err != nil { + errs = append(errs, err) + } + return errors.Join(errs...) +} + +// 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) (treeSnapshot, error) { + if _, err := os.Stat(dir); err != nil { + return treeSnapshot{}, err + } + 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() + if infoErr != nil { + return infoErr + } + if !info.Mode().IsRegular() { + return nil + } + data, readErr := os.ReadFile(path) //nolint:gosec // path is under a GetPluginPath-validated directory + if readErr != nil { + return readErr + } + snap.files[rel] = fileSnapshot{data: data, mode: sanitizeFileMode(info.Mode())} + return nil + }) + if err != nil { + return treeSnapshot{}, err + } + return snap, nil +} + +// 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 := 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.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) + } + } + return errors.Join(errs...) +} + +// 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 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 @@ -354,3 +673,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/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..292962d951 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -19,10 +19,11 @@ 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. 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( ctx context.Context, opts plugins.InstallOptions, @@ -110,7 +111,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..a4ccb82c48 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" @@ -28,10 +29,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) @@ -167,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, @@ -262,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, @@ -414,10 +422,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, @@ -427,6 +436,36 @@ func TestInstallWithExtraction(t *testing.T) { require.NoError(t, err) assert.ElementsMatch(t, []string{"claude-code", "codex"}, result.Plugin.Clients) }) + + // 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) + 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) + 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})) + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: "my-plugin", + LayerData: layerData, + Digest: "sha256:new", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "db update error") + }) } func TestInstallRoundTrip(t *testing.T) { @@ -548,6 +587,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.go b/pkg/plugins/pluginsvc/lock.go new file mode 100644 index 0000000000..84c7a15080 --- /dev/null +++ b/pkg/plugins/pluginsvc/lock.go @@ -0,0 +1,111 @@ +// 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) { + // 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 == "" { + return pl, fmt.Errorf("recording lock state for %q: content digest was not computed", pl.Metadata.Name) + } + source := opts.LockSource + 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..dccb1c8ec7 --- /dev/null +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -0,0 +1,986 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "errors" + "fmt" + "io/fs" + "net/http" + "os" + "path/filepath" + "testing" + + "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" + "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) EnsureRegistered(context.Context, plugins.DematerializeRequest) error { + 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} +} + +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}), + WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + ) + 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") + + _, err = os.Stat(filepath.Join(projectRoot, ".claude", "plugins", "my-plugin")) + 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) + + 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) + + // 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", + LayerData: makePluginLayerDataWithBody(t, "my-plugin", "# hello v2"), + Digest: validLockDigestAlt(), + Scope: plugins.ScopeProject, + ProjectRoot: projectRoot, + Clients: []string{"claude-code"}, + }) + 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") + + 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) + + 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) +} + +// 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) + 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) +} + +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 { + if s.beforeDelete != nil { + if err := s.beforeDelete(); err != nil { + return err + } + } + 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 + after func() +} + +func (a *failingDematerializeAdapter) Dematerialize(_ context.Context, _ plugins.DematerializeRequest) error { + if a.after != nil { + a.after() + } + 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) +} + +//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() + + 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)) + + // 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), 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, tree)) + + 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()) + assert.DirExists(t, filepath.Join(dest, "skills", "empty"), + "empty directories must be recreated on restore") +} + +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) Health(context.Context, plugins.DematerializeRequest) error { + return nil +} + +func (*failingMaterializeAdapter) SupportedComponents() []plugins.ComponentType { + return nil +} + +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) + 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...) + 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) + + inner := svc.(*service) //nolint:forcetypeassert + inner.groupManager = gm + + _, 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") +} + +//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) +} + +// 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) + + // 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 1ea8609fa3..33716f1b9a 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -8,19 +8,27 @@ import ( "errors" "fmt" "net/http" + "slices" "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 after snapshotting every client +// 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) @@ -44,8 +52,204 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) } return err } + return s.uninstallExisting(ctx, opts, scope, existing) +} + +// 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 + } + } + + restoreLock, err := removeManagedLockEntry(opts, existing, scope) + if err != nil { + 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 && s.clientManager != nil { + var snapErr error + 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), + restoreLock(), + ) + } + } + + cleanupErrs := s.dematerializeClients(ctx, existing, scope, opts.ProjectRoot) + if len(cleanupErrs) > 0 && restoreLock != nil { + return errors.Join(append(cleanupErrs, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + ))...) + } + + restoreGroups, groupErr := s.removePluginGroups(ctx, opts.Name) + if groupErr != nil { + if restoreLock != nil { + return errors.Join(groupErr, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + )) + } + return errors.Join(append(cleanupErrs, groupErr)...) + } - // Dematerialize for each client — best-effort. + 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 { + restoreErrs = append(restoreErrs, s.compensateManagedUninstall( + ctx, restoreLock, opts.Name, scope, opts.ProjectRoot, backups, existing.Clients, + )) + } + return errors.Join(restoreErrs...) + } + return errors.Join(cleanupErrs...) +} + +// 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, +) (restore func(context.Context) error, err error) { + if s.groupManager == nil { + return nil, nil + } + all, err := s.groupManager.List(ctx) + if err != nil { + return nil, fmt.Errorf("removing plugin from groups: listing groups: %w", err) + } + 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 func(ctx context.Context) error { return restoreUpTo(ctx, len(members)) }, nil +} + +// 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 nil +} + +// compensateManagedUninstall restores the lock pin and every snapshotted +// 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, + name string, + scope plugins.Scope, + projectRoot string, + 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), + ) +} + +// 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() error, err error) { + if scope != plugins.ScopeProject || !existing.Managed { + return nil, nil + } + + 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 !hasPrev { + return func() error { return nil }, nil + } + 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 +} + +// dematerializeClients best-effort removes on-disk copies for each client the +// plugin was installed into. Missing adapters are skipped (unmanaged path). +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] @@ -53,23 +257,12 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) continue } if dmErr := adapter.Dematerialize(ctx, plugins.DematerializeRequest{ - Name: opts.Name, + Name: existing.Metadata.Name, Scope: scope, - ProjectRoot: opts.ProjectRoot, + ProjectRoot: projectRoot, }); dmErr != nil { cleanupErrs = append(cleanupErrs, fmt.Errorf("dematerializing plugin for client %q: %w", clientType, dmErr)) } } - - if err := s.store.Delete(ctx, opts.Name, scope, opts.ProjectRoot); err != nil { - 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...) + return cleanupErrs } diff --git a/pkg/plugins/pluginsvc/uninstall_test.go b/pkg/plugins/pluginsvc/uninstall_test.go index f2706b4baf..31912d22c2 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" @@ -96,9 +98,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) { + // 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) { t.Parallel() ctrl := gomock.NewController(t) store := storemocks.NewMockPluginStore(ctrl) @@ -111,8 +114,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 +125,97 @@ 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 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. t.Run("missing materializer for stored client is skipped", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -190,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), @@ -201,4 +294,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)) + }) }