From efef7cf5207945a58c25cda8469554f73aa79476 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 13 Aug 2026 18:35:30 +0200 Subject: [PATCH 1/9] Add plugin lock-file sync Restore project plugins from the lock file via thv ai-plugin sync and POST /plugins/sync, gated by TOOLHIVE_PLUGINS_LOCK_ENABLED. Signed-off-by: Samuele Verzi --- cmd/thv/app/ai_plugin_sync.go | 160 ++++++++++ docs/cli/thv_ai-plugin.md | 1 + docs/cli/thv_ai-plugin_sync.md | 57 ++++ docs/server/docs.go | 180 ++++++++++++ docs/server/swagger.json | 180 ++++++++++++ docs/server/swagger.yaml | 141 +++++++++ pkg/api/v1/plugins.go | 53 +++- pkg/api/v1/plugins_sync_test.go | 117 ++++++++ pkg/api/v1/plugins_types.go | 17 ++ pkg/plugins/client/client.go | 17 ++ pkg/plugins/client/dto.go | 8 + pkg/plugins/options.go | 7 + pkg/plugins/pluginsvc/install_extraction.go | 9 +- pkg/plugins/pluginsvc/lock.go | 8 +- pkg/plugins/pluginsvc/pin.go | 117 ++++++++ pkg/plugins/pluginsvc/pin_test.go | 192 ++++++++++++ pkg/plugins/pluginsvc/sync.go | 307 ++++++++++++++++++++ pkg/plugins/pluginsvc/sync_test.go | 270 +++++++++++++++++ 18 files changed, 1835 insertions(+), 6 deletions(-) create mode 100644 cmd/thv/app/ai_plugin_sync.go create mode 100644 docs/cli/thv_ai-plugin_sync.md create mode 100644 pkg/api/v1/plugins_sync_test.go create mode 100644 pkg/plugins/pluginsvc/pin.go create mode 100644 pkg/plugins/pluginsvc/pin_test.go create mode 100644 pkg/plugins/pluginsvc/sync.go create mode 100644 pkg/plugins/pluginsvc/sync_test.go diff --git a/cmd/thv/app/ai_plugin_sync.go b/cmd/thv/app/ai_plugin_sync.go new file mode 100644 index 0000000000..d5809b7802 --- /dev/null +++ b/cmd/thv/app/ai_plugin_sync.go @@ -0,0 +1,160 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "encoding/json" + "fmt" + "os" + + "github.com/spf13/cobra" + + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +var ( + aiPluginSyncProjectRoot string + aiPluginSyncClientsRaw string + aiPluginSyncCheck bool + aiPluginSyncAdopt bool + aiPluginSyncPrune bool + aiPluginSyncYes bool + aiPluginSyncFormat string +) + +var aiPluginSyncCmd = &cobra.Command{ + Use: "sync", + Short: "Restore project plugins to match the lock file", + Long: `Restore a project's installed plugins to match toolhive.lock.yaml. + +Missing or drifted plugins are reinstalled at their pinned digest. Use +--check to report drift without installing anything (suitable for CI). +Use --adopt to record lock entries for existing unmanaged installs, and +--prune to remove installs no longer present in the lock file. + +Unless --check is set, sync prompts for confirmation before installing — +plugin content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI). + +Requires TOOLHIVE_PLUGINS_LOCK_ENABLED=true.`, + PreRunE: chainPreRunE( + ValidateFormat(&aiPluginSyncFormat), + ), + RunE: aiPluginSyncCmdFunc, +} + +func init() { + aiPluginCmd.AddCommand(aiPluginSyncCmd) + + aiPluginSyncCmd.Flags().StringVar(&aiPluginSyncProjectRoot, "project-root", "", + "Project root path (default: auto-detected from the current directory)") + aiPluginSyncCmd.Flags().StringVar(&aiPluginSyncClientsRaw, "clients", "", + `Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client`) + aiPluginSyncCmd.Flags().BoolVar(&aiPluginSyncCheck, "check", false, + "Report drift without installing, writing, or removing anything") + aiPluginSyncCmd.Flags().BoolVar(&aiPluginSyncAdopt, "adopt", false, + "Write lock entries for existing unmanaged project-scope installs") + aiPluginSyncCmd.Flags().BoolVar(&aiPluginSyncPrune, "prune", false, + "Remove installs no longer present in the lock file") + aiPluginSyncCmd.Flags().BoolVar(&aiPluginSyncYes, "yes", false, + "Skip the confirmation prompt (required when not running interactively)") + AddFormatFlag(aiPluginSyncCmd, &aiPluginSyncFormat) +} + +func aiPluginSyncCmdFunc(cmd *cobra.Command, _ []string) error { + projectRoot, err := resolveProjectRoot(aiPluginSyncProjectRoot) + if err != nil { + return err + } + + if !aiPluginSyncCheck { + if !aiPluginSyncYes { + printPluginLockEntriesSummary(projectRoot) + } + confirmed, confirmErr := requireConfirmation("Sync plugins for "+projectRoot, aiPluginSyncYes) + if confirmErr != nil { + return confirmErr + } + if !confirmed { + fmt.Println("Sync cancelled.") + return nil + } + } + + c := newAIPluginClient(cmd.Context()) + result, err := c.Sync(cmd.Context(), plugins.SyncOptions{ + ProjectRoot: projectRoot, + Clients: parseSkillInstallClients(aiPluginSyncClientsRaw), + Check: aiPluginSyncCheck, + Adopt: aiPluginSyncAdopt, + Prune: aiPluginSyncPrune, + }) + if err != nil { + return formatAIPluginError("sync plugins", err) + } + + if err := printPluginSyncResult(result, aiPluginSyncFormat); err != nil { + return err + } + return pluginSyncExitError(result, aiPluginSyncCheck) +} + +func pluginSyncExitError(result *plugins.SyncResult, check bool) error { + if len(result.Failed) > 0 { + return withExitCode(fmt.Errorf("sync failed for %d plugin(s)", len(result.Failed)), ExitCodePartialFailure) + } + if outOfSync := len(result.Drifted) + len(result.Missing); check && outOfSync > 0 { + return withExitCode( + fmt.Errorf("%d plugin(s) drifted from or are missing against the lock file", outOfSync), + ExitCodeCheckFailure, + ) + } + return nil +} + +func printPluginSyncResult(result *plugins.SyncResult, format string) error { + if format == FormatJSON { + data, err := json.MarshalIndent(result, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal JSON: %w", err) + } + fmt.Println(string(data)) + return nil + } + + printSkillNameGroup("Installed", result.Installed) + printSkillNameGroup("Drifted", result.Drifted) + printSkillNameGroup("Missing (not installed)", result.Missing) + printSkillNameGroup("Up to date", result.AlreadyCurrent) + printSkillNameGroup("Never managed (use --adopt to record)", result.NeverManaged) + printSkillNameGroup("Removed from lock (use --prune to remove)", result.RemovedFromLock) + printSkillNameGroup("Pruned", result.Pruned) + if len(result.Failed) > 0 { + fmt.Println("Failed:") + for _, f := range result.Failed { + fmt.Printf(" %s [%s]: %s\n", f.Name, f.Reason, f.Error) + } + } + if isSyncResultEmpty(result) { + fmt.Println("Nothing to sync — the project matches its lock file") + } + return nil +} + +func printPluginLockEntriesSummary(projectRoot string) { + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return + } + lf, err := lockfile.Load(root) + if err != nil || len(lf.Plugins) == 0 { + return + } + fmt.Fprintf(os.Stderr, "Lock file plugin entries for %s:\n", sanitizeTerminal(projectRoot)) + for _, e := range lf.Plugins { + fmt.Fprintf(os.Stderr, " %s %s %s\n", + sanitizeTerminal(e.Name), sanitizeTerminal(e.Source), sanitizeTerminal(shortDigest(e.Digest))) + } +} diff --git a/docs/cli/thv_ai-plugin.md b/docs/cli/thv_ai-plugin.md index 1df12af56f..e2cdb2495f 100644 --- a/docs/cli/thv_ai-plugin.md +++ b/docs/cli/thv_ai-plugin.md @@ -39,6 +39,7 @@ The ai-plugin command provides subcommands to manage plugins for AI tools * [thv ai-plugin install](thv_ai-plugin_install.md) - Install an AI-tool plugin * [thv ai-plugin list](thv_ai-plugin_list.md) - List installed AI-tool plugins * [thv ai-plugin push](thv_ai-plugin_push.md) - Push a built AI-tool plugin to an OCI registry +* [thv ai-plugin sync](thv_ai-plugin_sync.md) - Restore project plugins to match the lock file * [thv ai-plugin uninstall](thv_ai-plugin_uninstall.md) - Uninstall an AI-tool plugin * [thv ai-plugin validate](thv_ai-plugin_validate.md) - Validate an AI-tool plugin directory diff --git a/docs/cli/thv_ai-plugin_sync.md b/docs/cli/thv_ai-plugin_sync.md new file mode 100644 index 0000000000..55b39fa529 --- /dev/null +++ b/docs/cli/thv_ai-plugin_sync.md @@ -0,0 +1,57 @@ +--- +title: thv ai-plugin sync +hide_title: true +description: Reference for ToolHive CLI command `thv ai-plugin sync` +last_update: + author: autogenerated +slug: thv_ai-plugin_sync +mdx: + format: md +--- + +## thv ai-plugin sync + +Restore project plugins to match the lock file + +### Synopsis + +Restore a project's installed plugins to match toolhive.lock.yaml. + +Missing or drifted plugins are reinstalled at their pinned digest. Use +--check to report drift without installing anything (suitable for CI). +Use --adopt to record lock entries for existing unmanaged installs, and +--prune to remove installs no longer present in the lock file. + +Unless --check is set, sync prompts for confirmation before installing — +plugin content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI). + +Requires TOOLHIVE_PLUGINS_LOCK_ENABLED=true. + +``` +thv ai-plugin sync [flags] +``` + +### Options + +``` + --adopt Write lock entries for existing unmanaged project-scope installs + --check Report drift without installing, writing, or removing anything + --clients string Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client + --format string Output format (json, text) (default "text") + -h, --help help for sync + --project-root string Project root path (default: auto-detected from the current directory) + --prune Remove installs no longer present in the lock file + --yes Skip the confirmation prompt (required when not running interactively) +``` + +### Options inherited from parent commands + +``` + --debug Enable debug mode +``` + +### SEE ALSO + +* [thv ai-plugin](thv_ai-plugin.md) - Manage AI-tool plugins + diff --git a/docs/server/docs.go b/docs/server/docs.go index b9f55820b4..d326dc2aa3 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1253,6 +1253,75 @@ const docTemplate = `{ "ScopeProject" ] }, + "github_com_stacklok_toolhive_pkg_plugins.SyncResult": { + "properties": { + "already_current": { + "description": "AlreadyCurrent lists skills that already matched the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "drifted": { + "description": "Drifted lists skills whose on-disk contentDigest differed from the lock\nfile. Normally these are reinstalled to match it; when Check is set,\nnothing is written and this field reports the drift only.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "failed": { + "description": "Failed lists skills that could not be synced, with the reason for each.\nDrift alone is never reported here — see Drifted.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SyncFailure" + }, + "type": "array", + "uniqueItems": false + }, + "installed": { + "description": "Installed lists skills that were installed or reinstalled to match the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "missing": { + "description": "Missing lists lock entries with no corresponding install record at all\n— the fresh-clone state. Normally these are installed at their pinned\nreference; when Check is set, nothing is written and this field\nreports the gap only.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "never_managed": { + "description": "NeverManaged lists project-scoped skills never recorded as lock-managed.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "pruned": { + "description": "Pruned lists removed-from-lock skills that were uninstalled because Prune was set.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "removed_from_lock": { + "description": "RemovedFromLock lists previously managed skills absent from the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + } + }, + "type": "object" + }, "github_com_stacklok_toolhive_pkg_plugins.ValidationResult": { "properties": { "errors": { @@ -3748,6 +3817,36 @@ const docTemplate = `{ }, "type": "object" }, + "pkg_api_v1.syncPluginsRequest": { + "description": "Request to restore a project's installed plugins to match its lock file", + "properties": { + "adopt": { + "description": "Adopt writes lock entries for existing unmanaged project-scope installs", + "type": "boolean" + }, + "check": { + "description": "Check verifies on-disk content against the lock file without installing or writing anything", + "type": "boolean" + }, + "clients": { + "description": "Clients lists target client identifiers. Empty means every\nplugin-supporting client detected on this host.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "project_root": { + "description": "ProjectRoot is the project root path whose lock file should be synced", + "type": "string" + }, + "prune": { + "description": "Prune removes project-scoped plugins installed but not present in the lock file", + "type": "boolean" + } + }, + "type": "object" + }, "pkg_api_v1.syncSkillsRequest": { "description": "Request to restore a project's installed skills to match its lock file", "properties": { @@ -6259,6 +6358,87 @@ const docTemplate = `{ ] } }, + "/api/v1beta/plugins/sync": { + "post": { + "description": "Restore a project's installed plugins to match toolhive.lock.yaml", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.syncPluginsRequest", + "summary": "request", + "description": "Sync request" + } + ] + } + } + }, + "description": "Sync request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_plugins.SyncResult" + } + } + }, + "description": "OK" + }, + "400": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Bad Request" + }, + "403": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Forbidden (feature not enabled)" + }, + "500": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Internal Server Error" + }, + "501": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Implemented" + } + }, + "summary": "Sync project plugins from the lock file", + "tags": [ + "plugins" + ] + } + }, "/api/v1beta/plugins/validate": { "post": { "description": "Validate a plugin definition", diff --git a/docs/server/swagger.json b/docs/server/swagger.json index ee9a53ea29..30550982c0 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1246,6 +1246,75 @@ "ScopeProject" ] }, + "github_com_stacklok_toolhive_pkg_plugins.SyncResult": { + "properties": { + "already_current": { + "description": "AlreadyCurrent lists skills that already matched the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "drifted": { + "description": "Drifted lists skills whose on-disk contentDigest differed from the lock\nfile. Normally these are reinstalled to match it; when Check is set,\nnothing is written and this field reports the drift only.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "failed": { + "description": "Failed lists skills that could not be synced, with the reason for each.\nDrift alone is never reported here — see Drifted.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SyncFailure" + }, + "type": "array", + "uniqueItems": false + }, + "installed": { + "description": "Installed lists skills that were installed or reinstalled to match the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "missing": { + "description": "Missing lists lock entries with no corresponding install record at all\n— the fresh-clone state. Normally these are installed at their pinned\nreference; when Check is set, nothing is written and this field\nreports the gap only.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "never_managed": { + "description": "NeverManaged lists project-scoped skills never recorded as lock-managed.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "pruned": { + "description": "Pruned lists removed-from-lock skills that were uninstalled because Prune was set.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "removed_from_lock": { + "description": "RemovedFromLock lists previously managed skills absent from the lock file.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + } + }, + "type": "object" + }, "github_com_stacklok_toolhive_pkg_plugins.ValidationResult": { "properties": { "errors": { @@ -3741,6 +3810,36 @@ }, "type": "object" }, + "pkg_api_v1.syncPluginsRequest": { + "description": "Request to restore a project's installed plugins to match its lock file", + "properties": { + "adopt": { + "description": "Adopt writes lock entries for existing unmanaged project-scope installs", + "type": "boolean" + }, + "check": { + "description": "Check verifies on-disk content against the lock file without installing or writing anything", + "type": "boolean" + }, + "clients": { + "description": "Clients lists target client identifiers. Empty means every\nplugin-supporting client detected on this host.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "project_root": { + "description": "ProjectRoot is the project root path whose lock file should be synced", + "type": "string" + }, + "prune": { + "description": "Prune removes project-scoped plugins installed but not present in the lock file", + "type": "boolean" + } + }, + "type": "object" + }, "pkg_api_v1.syncSkillsRequest": { "description": "Request to restore a project's installed skills to match its lock file", "properties": { @@ -6252,6 +6351,87 @@ ] } }, + "/api/v1beta/plugins/sync": { + "post": { + "description": "Restore a project's installed plugins to match toolhive.lock.yaml", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.syncPluginsRequest", + "summary": "request", + "description": "Sync request" + } + ] + } + } + }, + "description": "Sync request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_plugins.SyncResult" + } + } + }, + "description": "OK" + }, + "400": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Bad Request" + }, + "403": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Forbidden (feature not enabled)" + }, + "500": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Internal Server Error" + }, + "501": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Implemented" + } + }, + "summary": "Sync project plugins from the lock file", + "tags": [ + "plugins" + ] + } + }, "/api/v1beta/plugins/validate": { "post": { "description": "Validate a plugin definition", diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 1d420d016f..a275399ac0 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1396,6 +1396,70 @@ components: x-enum-varnames: - ScopeUser - ScopeProject + github_com_stacklok_toolhive_pkg_plugins.SyncResult: + properties: + already_current: + description: AlreadyCurrent lists skills that already matched the lock file. + items: + type: string + type: array + uniqueItems: false + drifted: + description: |- + Drifted lists skills whose on-disk contentDigest differed from the lock + file. Normally these are reinstalled to match it; when Check is set, + nothing is written and this field reports the drift only. + items: + type: string + type: array + uniqueItems: false + failed: + description: |- + Failed lists skills that could not be synced, with the reason for each. + Drift alone is never reported here — see Drifted. + items: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.SyncFailure' + type: array + uniqueItems: false + installed: + description: Installed lists skills that were installed or reinstalled to + match the lock file. + items: + type: string + type: array + uniqueItems: false + missing: + description: |- + Missing lists lock entries with no corresponding install record at all + — the fresh-clone state. Normally these are installed at their pinned + reference; when Check is set, nothing is written and this field + reports the gap only. + items: + type: string + type: array + uniqueItems: false + never_managed: + description: NeverManaged lists project-scoped skills never recorded as + lock-managed. + items: + type: string + type: array + uniqueItems: false + pruned: + description: Pruned lists removed-from-lock skills that were uninstalled + because Prune was set. + items: + type: string + type: array + uniqueItems: false + removed_from_lock: + description: RemovedFromLock lists previously managed skills absent from + the lock file. + items: + type: string + type: array + uniqueItems: false + type: object github_com_stacklok_toolhive_pkg_plugins.ValidationResult: properties: errors: @@ -3464,6 +3528,35 @@ components: type: array uniqueItems: false type: object + pkg_api_v1.syncPluginsRequest: + description: Request to restore a project's installed plugins to match its lock + file + properties: + adopt: + description: Adopt writes lock entries for existing unmanaged project-scope + installs + type: boolean + check: + description: Check verifies on-disk content against the lock file without + installing or writing anything + type: boolean + clients: + description: |- + Clients lists target client identifiers. Empty means every + plugin-supporting client detected on this host. + items: + type: string + type: array + uniqueItems: false + project_root: + description: ProjectRoot is the project root path whose lock file should + be synced + type: string + prune: + description: Prune removes project-scoped plugins installed but not present + in the lock file + type: boolean + type: object pkg_api_v1.syncSkillsRequest: description: Request to restore a project's installed skills to match its lock file @@ -5585,6 +5678,54 @@ paths: summary: Push a plugin tags: - plugins + /api/v1beta/plugins/sync: + post: + description: Restore a project's installed plugins to match toolhive.lock.yaml + requestBody: + content: + application/json: + schema: + oneOf: + - type: object + - $ref: '#/components/schemas/pkg_api_v1.syncPluginsRequest' + description: Sync request + summary: request + description: Sync request + required: true + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_plugins.SyncResult' + description: OK + "400": + content: + application/json: + schema: + type: string + description: Bad Request + "403": + content: + application/json: + schema: + type: string + description: Forbidden (feature not enabled) + "500": + content: + application/json: + schema: + type: string + description: Internal Server Error + "501": + content: + application/json: + schema: + type: string + description: Not Implemented + summary: Sync project plugins from the lock file + tags: + - plugins /api/v1beta/plugins/validate: post: description: Validate a plugin definition diff --git a/pkg/api/v1/plugins.go b/pkg/api/v1/plugins.go index 2ff6aee30e..a05c8e8236 100644 --- a/pkg/api/v1/plugins.go +++ b/pkg/api/v1/plugins.go @@ -5,6 +5,7 @@ package v1 import ( "encoding/json" + "errors" "fmt" "net/http" @@ -19,13 +20,20 @@ import ( // PluginsRoutes defines the routes for plugin management. type PluginsRoutes struct { pluginService plugins.PluginService + lockService plugins.PluginLockService } -// PluginsRouter creates a new router for plugin management endpoints. +// PluginsRouter creates a new router for plugin management endpoints. If +// pluginService's concrete implementation also satisfies plugins.PluginLockService +// (as pluginsvc.New's does once Sync exists), /sync is served; otherwise it +// returns 501. func PluginsRouter(pluginService plugins.PluginService) http.Handler { routes := PluginsRoutes{ pluginService: pluginService, } + if lockSvc, ok := pluginService.(plugins.PluginLockService); ok { + routes.lockService = lockSvc + } // Mirrors WorkloadRouter and SkillsRouter: routes that move OCI artifacts // get a timeout sized for the transfer, everything else keeps the short @@ -45,6 +53,7 @@ func PluginsRouter(pluginService plugins.PluginService) http.Handler { r.With(stdTimeout).Get("/builds", apierrors.ErrorHandler(routes.listBuilds)) r.With(stdTimeout).Delete("/builds/{tag}", apierrors.ErrorHandler(routes.deleteBuild)) r.With(stdTimeout).Get("/content", apierrors.ErrorHandler(routes.getPluginContent)) + r.With(longTimeout).Post("/sync", apierrors.ErrorHandler(routes.syncPlugins)) return r } @@ -398,3 +407,45 @@ func (s *PluginsRoutes) getPluginContent(w http.ResponseWriter, r *http.Request) w.Header().Set("Content-Type", "application/json") return json.NewEncoder(w).Encode(content) } + +// syncPlugins restores a project's installed plugins to match its lock file. +// +// @Summary Sync project plugins from the lock file +// @Description Restore a project's installed plugins to match toolhive.lock.yaml +// @Tags plugins +// @Accept json +// @Produce json +// @Param request body syncPluginsRequest true "Sync request" +// @Success 200 {object} plugins.SyncResult +// @Failure 400 {string} string "Bad Request" +// @Failure 403 {string} string "Forbidden (feature not enabled)" +// @Failure 500 {string} string "Internal Server Error" +// @Failure 501 {string} string "Not Implemented" +// @Router /api/v1beta/plugins/sync [post] +func (s *PluginsRoutes) syncPlugins(w http.ResponseWriter, r *http.Request) error { + if s.lockService == nil { + return httperr.WithCode(errors.New("plugin sync is not supported by this server"), http.StatusNotImplemented) + } + + var req syncPluginsRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { + return httperr.WithCode( + fmt.Errorf("invalid request body: %w", err), + http.StatusBadRequest, + ) + } + + result, err := s.lockService.Sync(r.Context(), plugins.SyncOptions{ + ProjectRoot: req.ProjectRoot, + Clients: req.Clients, + Prune: req.Prune, + Check: req.Check, + Adopt: req.Adopt, + }) + if err != nil { + return err + } + + w.Header().Set("Content-Type", "application/json") + return json.NewEncoder(w).Encode(result) +} diff --git a/pkg/api/v1/plugins_sync_test.go b/pkg/api/v1/plugins_sync_test.go new file mode 100644 index 0000000000..7f9ff45767 --- /dev/null +++ b/pkg/api/v1/plugins_sync_test.go @@ -0,0 +1,117 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package v1 + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "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/plugins" + plugmocks "github.com/stacklok/toolhive/pkg/plugins/mocks" +) + +// pluginServiceWithSync wraps a mocked PluginService and adds Sync and Upgrade +// methods, so PluginsRouter's opportunistic PluginLockService type assertion +// succeeds — the same shape pluginsvc.New's concrete service has. +type pluginServiceWithSync struct { + plugins.PluginService + syncFn func(ctx context.Context, opts plugins.SyncOptions) (*plugins.SyncResult, error) + upgradeFn func(ctx context.Context, opts plugins.UpgradeOptions) (*plugins.UpgradeResult, error) +} + +func (s *pluginServiceWithSync) Sync(ctx context.Context, opts plugins.SyncOptions) (*plugins.SyncResult, error) { + return s.syncFn(ctx, opts) +} + +func (s *pluginServiceWithSync) Upgrade(ctx context.Context, opts plugins.UpgradeOptions) (*plugins.UpgradeResult, error) { + if s.upgradeFn == nil { + return &plugins.UpgradeResult{}, nil + } + return s.upgradeFn(ctx, opts) +} + +func TestSyncPluginsEndpoint(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + service plugins.PluginService + body string + wantStatus int + wantContains string + }{ + { + name: "successful sync returns 200 with result", + service: &pluginServiceWithSync{ + PluginService: plugmocks.NewMockPluginService(gomock.NewController(t)), + syncFn: func(_ context.Context, opts plugins.SyncOptions) (*plugins.SyncResult, error) { + assert.Equal(t, "/tmp/proj", opts.ProjectRoot) + assert.True(t, opts.Check) + return &plugins.SyncResult{AlreadyCurrent: []string{"my-plugin"}}, nil + }, + }, + body: `{"project_root":"/tmp/proj","check":true}`, + wantStatus: http.StatusOK, + wantContains: `"my-plugin"`, + }, + { + name: "service without Sync support returns 501", + service: plugmocks.NewMockPluginService(gomock.NewController(t)), + body: `{"project_root":"/tmp/proj"}`, + wantStatus: http.StatusNotImplemented, + }, + { + name: "invalid JSON body returns 400", + service: &pluginServiceWithSync{ + PluginService: plugmocks.NewMockPluginService(gomock.NewController(t)), + syncFn: func(context.Context, plugins.SyncOptions) (*plugins.SyncResult, error) { + t.Fatal("Sync must not be called for an invalid body") + return nil, nil + }, + }, + body: `{`, + wantStatus: http.StatusBadRequest, + }, + { + name: "sync error is forwarded", + service: &pluginServiceWithSync{ + PluginService: plugmocks.NewMockPluginService(gomock.NewController(t)), + syncFn: func(context.Context, plugins.SyncOptions) (*plugins.SyncResult, error) { + return nil, httperr.WithCode(assert.AnError, http.StatusForbidden) + }, + }, + body: `{"project_root":"/tmp/proj"}`, + wantStatus: http.StatusForbidden, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := httptest.NewRequest(http.MethodPost, "/sync", bytes.NewBufferString(tt.body)) + req.Header.Set("Content-Type", "application/json") + rec := httptest.NewRecorder() + PluginsRouter(tt.service).ServeHTTP(rec, req) + + assert.Equal(t, tt.wantStatus, rec.Code) + if tt.wantContains != "" { + assert.Contains(t, rec.Body.String(), tt.wantContains) + } + if rec.Code == http.StatusOK { + var result plugins.SyncResult + require.NoError(t, json.NewDecoder(rec.Body).Decode(&result)) + } + }) + } +} diff --git a/pkg/api/v1/plugins_types.go b/pkg/api/v1/plugins_types.go index e32aac0b8d..c822e13faa 100644 --- a/pkg/api/v1/plugins_types.go +++ b/pkg/api/v1/plugins_types.go @@ -76,3 +76,20 @@ type pluginBuildListResponse struct { // List of locally-built OCI plugin artifacts Builds []plugins.LocalBuild `json:"builds"` } + +// syncPluginsRequest represents the request to sync a project's plugins. +// +// @Description Request to restore a project's installed plugins to match its lock file +type syncPluginsRequest struct { + // ProjectRoot is the project root path whose lock file should be synced + ProjectRoot string `json:"project_root"` + // Clients lists target client identifiers. Empty means every + // plugin-supporting client detected on this host. + Clients []string `json:"clients,omitempty"` + // Prune removes project-scoped plugins installed but not present in the lock file + Prune bool `json:"prune,omitempty"` + // Check verifies on-disk content against the lock file without installing or writing anything + Check bool `json:"check,omitempty"` + // Adopt writes lock entries for existing unmanaged project-scope installs + Adopt bool `json:"adopt,omitempty"` +} diff --git a/pkg/plugins/client/client.go b/pkg/plugins/client/client.go index cb75e1c04c..5eacd67a8e 100644 --- a/pkg/plugins/client/client.go +++ b/pkg/plugins/client/client.go @@ -310,6 +310,23 @@ func (c *Client) GetContent(ctx context.Context, opts plugins.ContentOptions) (* return &content, nil } +// Sync restores a project's installed plugins to match its lock file. +func (c *Client) Sync(ctx context.Context, opts plugins.SyncOptions) (*plugins.SyncResult, error) { + body := syncRequest{ + ProjectRoot: opts.ProjectRoot, + Clients: opts.Clients, + Prune: opts.Prune, + Check: opts.Check, + Adopt: opts.Adopt, + } + + var result plugins.SyncResult + if err := c.doJSONRequest(ctx, http.MethodPost, "/sync", nil, body, &result); err != nil { + return nil, err + } + return &result, nil +} + // --- internal helpers --- func (c *Client) buildURL(path string, query url.Values) string { diff --git a/pkg/plugins/client/dto.go b/pkg/plugins/client/dto.go index 00f0f29e67..1f5b87d9c1 100644 --- a/pkg/plugins/client/dto.go +++ b/pkg/plugins/client/dto.go @@ -41,3 +41,11 @@ type installResponse struct { type listBuildsResponse struct { Builds []plugins.LocalBuild `json:"builds"` } + +type syncRequest struct { + ProjectRoot string `json:"project_root"` + Clients []string `json:"clients,omitempty"` + Prune bool `json:"prune,omitempty"` + Check bool `json:"check,omitempty"` + Adopt bool `json:"adopt,omitempty"` +} diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index 190f5aa72f..383f8dc6d3 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -60,6 +60,13 @@ type InstallOptions struct { // reinstalling at a pinned reference. Internal use only — NOT exposed // via HTTP API. LockResolvedReference string `json:"-"` + // SyncRestore forces re-extraction to every existing client even when + // Digest matches the currently-installed digest. Set by Sync when + // reinstalling at a pinned reference: the whole point is repairing + // on-disk drift that happened without the pinned digest changing, so the + // normal "same digest means content is already correct" fast path must + // not apply. Internal use only — NOT exposed via HTTP API. + SyncRestore bool `json:"-"` } // InstallResult contains the outcome of an Install operation. diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 106b9d5b51..7b0d0b357a 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -73,12 +73,12 @@ func (s *service) dispatchExtraction( storeErr error, clientTypes []string, ) (*plugins.InstallResult, error) { - if isExtractionNoOp(existing, storeErr, opts, clientTypes) { + if !opts.SyncRestore && isExtractionNoOp(existing, storeErr, opts, clientTypes) { return &plugins.InstallResult{Plugin: existing}, nil } digestMatches := storeErr == nil && existing.Digest == opts.Digest - if digestMatches { + if digestMatches && !opts.SyncRestore { return s.installExtractionSameDigestNewClients(ctx, opts, scope, existing, clientTypes) } if storeErr == nil { @@ -89,8 +89,9 @@ func (s *service) dispatchExtraction( // isExtractionNoOp reports whether the install can be short-circuited because // the same digest and all requested clients are already present. Mirror of -// 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. +// skillsvc.isExtractionNoOp. Callers must also check SyncRestore: a lock-driven +// reinstall repairs on-disk drift at the same digest, so the no-op path must +// not apply. func isExtractionNoOp(existing plugins.InstalledPlugin, storeErr error, opts plugins.InstallOptions, clientTypes []string) bool { if storeErr != nil || existing.Digest != opts.Digest { return false diff --git a/pkg/plugins/pluginsvc/lock.go b/pkg/plugins/pluginsvc/lock.go index 84c7a15080..4eebb22748 100644 --- a/pkg/plugins/pluginsvc/lock.go +++ b/pkg/plugins/pluginsvc/lock.go @@ -5,12 +5,18 @@ package pluginsvc import ( "context" + "errors" "fmt" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills/lockfile" ) +// errLockWrite marks failures to write the project lock file, so +// classifySyncFailure can map them to FailureReasonLockWriteFailed without +// matching on error text or treating every HTTP 500 as a lock-write failure. +var errLockWrite = errors.New("lock file write failed") + // 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 @@ -48,7 +54,7 @@ func (s *service) recordLockState( Digest: pl.Digest, ContentDigest: contentDigest, }); err != nil { - return pl, fmt.Errorf("writing lock entry: %w", err) + return pl, fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) } if !pl.Managed { diff --git a/pkg/plugins/pluginsvc/pin.go b/pkg/plugins/pluginsvc/pin.go new file mode 100644 index 0000000000..8e94f62211 --- /dev/null +++ b/pkg/plugins/pluginsvc/pin.go @@ -0,0 +1,117 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "fmt" + "strings" + + nameref "github.com/google/go-containerregistry/pkg/name" + + "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// isImmutableSource reports whether a lock entry's source can never produce +// newer content: an OCI digest reference, or a git reference already pinned +// to a full commit hash. Upgrade reports these as not-upgradable rather than +// attempting to re-resolve them. +func isImmutableSource(entry lockfile.Entry) bool { + if gitresolver.IsGitReference(entry.Source) { + ref, err := gitresolver.ParseGitReference(entry.Source) + return err == nil && isFullCommitHash(ref.Ref) + } + ref, err := nameref.ParseReference(entry.Source) + if err != nil { + return false + } + _, isDigest := ref.(nameref.Digest) + return isDigest +} + +// repositoryMoved reports whether two resolved references point at different +// repositories, as opposed to differing only by tag. A tag move within one +// repository is the normal way a mutable source advances; a repository move +// means the artifact now comes from somewhere else, which is the case the +// ref-change guard exists to catch. +// +// Both the registry and the repository path are compared, so ghcr.io -> +// another registry, or a different org or path on the same registry, all +// count as a move. +func repositoryMoved(oldRef, newRef string) bool { + if oldRef == newRef { + return false + } + oldRepo, ok := referenceRepository(oldRef) + if !ok { + return true + } + newRepo, ok := referenceRepository(newRef) + if !ok { + return true + } + return oldRepo != newRepo +} + +// referenceRepository returns the registry and repository portion of an OCI +// reference, dropping any tag or digest. It reports false for git references +// and for anything it cannot parse, so callers fall back to treating the +// reference as moved rather than silently equating two unlike sources. +func referenceRepository(ref string) (string, bool) { + if gitresolver.IsGitReference(ref) { + return "", false + } + parsed, err := nameref.ParseReference(ref) + if err != nil { + return "", false + } + return parsed.Context().String(), true +} + +// isFullCommitHash accepts both hex cases: the sibling git resolver does +// too, and classifying an uppercase-pinned source as mutable would +// needlessly re-clone it on every upgrade despite the pin being immutable. +func isFullCommitHash(ref string) bool { + if len(ref) != 40 { + return false + } + for _, c := range ref { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') && (c < 'A' || c > 'F') { + return false + } + } + return true +} + +// buildPinnedReference returns the exact reference sync must install: entry's +// resolvedReference re-pointed at its pinned digest, never re-resolved from +// source. This is what makes sync a restore operation rather than an upgrade +// — installing this reference always yields entry's exact pinned content. +func buildPinnedReference(entry lockfile.Entry) (string, error) { + if gitresolver.IsGitReference(entry.ResolvedReference) { + return pinGitReference(entry) + } + return pinOCIReference(entry) +} + +func pinOCIReference(entry lockfile.Entry) (string, error) { + ref, err := nameref.ParseReference(entry.ResolvedReference) + if err != nil { + return "", fmt.Errorf("parsing resolvedReference %q: %w", entry.ResolvedReference, err) + } + return ref.Context().String() + "@" + entry.Digest, nil +} + +func pinGitReference(entry lockfile.Entry) (string, error) { + gitRef, err := gitresolver.ParseGitReference(entry.ResolvedReference) + if err != nil { + return "", fmt.Errorf("parsing resolvedReference %q: %w", entry.ResolvedReference, err) + } + hostAndPath := strings.TrimPrefix(strings.TrimPrefix(gitRef.URL, "https://"), "http://") + pinned := "git://" + hostAndPath + "@" + entry.Digest + if gitRef.Path != "" { + pinned += "#" + gitRef.Path + } + return pinned, nil +} diff --git a/pkg/plugins/pluginsvc/pin_test.go b/pkg/plugins/pluginsvc/pin_test.go new file mode 100644 index 0000000000..34e68382eb --- /dev/null +++ b/pkg/plugins/pluginsvc/pin_test.go @@ -0,0 +1,192 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +const testCommitHash = "abcdef1234567890abcdef1234567890abcdef12" + +func TestBuildPinnedReference(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entry lockfile.Entry + want string + }{ + { + name: "OCI reference pins to digest", + entry: lockfile.Entry{ + ResolvedReference: "ghcr.io/org/code-review:1.0.0", + Digest: "sha256:" + hexDigestForTest(), + }, + want: "ghcr.io/org/code-review@sha256:" + hexDigestForTest(), + }, + { + name: "git reference pins to commit hash, dropping any tag/branch ref", + entry: lockfile.Entry{ + ResolvedReference: "git://github.com/org/plugins@main#testing-conventions", + Digest: testCommitHash, + }, + want: "git://github.com/org/plugins@" + testCommitHash + "#testing-conventions", + }, + { + name: "git reference without a subdir", + entry: lockfile.Entry{ + ResolvedReference: "git://github.com/org/plugins", + Digest: testCommitHash, + }, + want: "git://github.com/org/plugins@" + testCommitHash, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got, err := buildPinnedReference(tt.entry) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestBuildPinnedReferenceRejectsUnparsable(t *testing.T) { + t.Parallel() + _, err := buildPinnedReference(lockfile.Entry{ResolvedReference: "not a valid reference!!", Digest: "sha256:abc"}) + require.Error(t, err) +} + +func TestIsImmutableSource(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + entry lockfile.Entry + want bool + }{ + { + name: "OCI digest source is immutable", + entry: lockfile.Entry{Source: "ghcr.io/org/plugin@sha256:" + hexDigestForTest()}, + want: true, + }, + { + name: "OCI tag source is mutable", + entry: lockfile.Entry{Source: "ghcr.io/org/plugin:1.0.0"}, + want: false, + }, + { + name: "git full commit hash source is immutable", + entry: lockfile.Entry{Source: "git://github.com/org/plugin@" + testCommitHash}, + want: true, + }, + { + name: "git branch source is mutable", + entry: lockfile.Entry{Source: "git://github.com/org/plugin@main"}, + want: false, + }, + { + name: "git uppercase full commit hash source is immutable", + entry: lockfile.Entry{Source: "git://github.com/org/plugin@ABCDEF0123456789ABCDEF0123456789ABCDEF01"}, + want: true, + }, + { + name: "git source with no ref is mutable", + entry: lockfile.Entry{Source: "git://github.com/org/plugin"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, isImmutableSource(tt.entry)) + }) + } +} + +func TestRepositoryMoved(t *testing.T) { + t.Parallel() + + digest := "sha256:" + hexDigestForTest() + tests := []struct { + name string + oldRef string + newRef string + want bool + }{ + { + name: "identical references have not moved", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "ghcr.io/org/plugin:v1", + }, + { + name: "a version bump is not a move", + oldRef: "ghcr.io/org/plugin:0.1.0", + newRef: "ghcr.io/org/plugin:0.2.0", + }, + { + name: "moving to a digest in the same repository is not a move", + oldRef: "ghcr.io/org/plugin:0.1.0", + newRef: "ghcr.io/org/plugin@" + digest, + }, + { + name: "an implicit latest tag matches an explicit one", + oldRef: "ghcr.io/org/plugin", + newRef: "ghcr.io/org/plugin:latest", + }, + { + name: "a different repository path is a move", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "ghcr.io/org/other-plugin:v1", + want: true, + }, + { + name: "a different org on the same registry is a move", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "ghcr.io/attacker/plugin:v1", + want: true, + }, + { + name: "a different registry is a move", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "elsewhere.io/org/plugin:v1", + want: true, + }, + { + name: "git references fall back to exact comparison", + oldRef: "git://github.com/org/repo#plugins/a", + newRef: "git://github.com/org/repo#plugins/b", + want: true, + }, + { + name: "an OCI reference replaced by a git one is a move", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "git://github.com/org/repo", + want: true, + }, + { + name: "unparsable input fails closed", + oldRef: "ghcr.io/org/plugin:v1", + newRef: "not a valid reference at all", + want: true, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.want, repositoryMoved(tc.oldRef, tc.newRef)) + }) + } +} + +func hexDigestForTest() string { + return "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789" +} diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go new file mode 100644 index 0000000000..b19fb20bc2 --- /dev/null +++ b/pkg/plugins/pluginsvc/sync.go @@ -0,0 +1,307 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/client" + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage" +) + +// var _ ensures *service satisfies the lock service surface. Upgrade is a +// stub until the next PR in this stack lands the real implementation; the +// compile-time check still requires both methods so PluginsRouter's type +// assert succeeds and /sync can be served. +var _ plugins.PluginLockService = (*service)(nil) + +// Sync restores a project's installed plugins to match its lock file: missing +// or drifted entries are reinstalled at their pinned digest (never +// re-resolved from source — see buildPinnedReference), unmanaged installs are +// reported (or adopted with Adopt), and lock-managed installs no longer in +// the lock file are reported (or removed with Prune). Check performs the +// same reconciliation read-only: nothing is installed, written, or removed. +func (s *service) Sync(ctx context.Context, opts plugins.SyncOptions) (*plugins.SyncResult, error) { + if !plugins.LockFileFeatureEnabled() { + return nil, httperr.WithCode( + fmt.Errorf("plugin lock file is not enabled; set %s=true", plugins.LockFileEnvVar), + http.StatusForbidden, + ) + } + + _, projectRoot, err := normalizeProjectRoot(plugins.ScopeProject, opts.ProjectRoot) + if err != nil { + return nil, err + } + opts.ProjectRoot = projectRoot + + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return nil, err + } + lf, err := lockfile.Load(root) + if err != nil { + return nil, err + } + + installed, err := s.store.List(ctx, storage.ListFilter{Scope: plugins.ScopeProject, ProjectRoot: projectRoot}) + if err != nil { + return nil, fmt.Errorf("listing installed plugins: %w", err) + } + installedByName := make(map[string]plugins.InstalledPlugin, len(installed)) + for _, pl := range installed { + installedByName[pl.Metadata.Name] = pl + } + + result := &plugins.SyncResult{} + for _, entry := range lf.Plugins { + pl, dbOK := installedByName[entry.Name] + s.syncLockedEntry(ctx, opts, entry, pl, dbOK, result) + } + for _, pl := range installed { + if _, ok := lf.GetPlugin(pl.Metadata.Name); ok { + continue // handled by the loop above + } + s.syncUnlockedInstall(ctx, opts, pl, result) + } + + return result, nil +} + +// Upgrade is implemented in the next PR of this stack. The stub exists so +// *service satisfies PluginLockService (and /sync can be type-asserted) +// without exposing a half-built upgrade path. +func (*service) Upgrade(_ context.Context, _ plugins.UpgradeOptions) (*plugins.UpgradeResult, error) { + return nil, httperr.WithCode(errors.New("plugin upgrade is not implemented"), http.StatusNotImplemented) +} + +// syncLockedEntry reconciles one lock file entry against installed state, +// appending its outcome to result. Missing (dbOK false) and drifted (digest +// or contentDigest mismatch) entries are reinstalled at the pinned reference +// unless opts.Check is set, in which case nothing is written — both states +// are still reported (Missing/Drifted), never as failures. +func (s *service) syncLockedEntry( + ctx context.Context, + opts plugins.SyncOptions, + entry lockfile.Entry, + pl plugins.InstalledPlugin, + dbOK bool, + result *plugins.SyncResult, +) { + if dbOK && s.entryMatchesInstalled(entry, pl) { + result.AlreadyCurrent = append(result.AlreadyCurrent, entry.Name) + return + } + if dbOK { + result.Drifted = append(result.Drifted, entry.Name) + } else { + result.Missing = append(result.Missing, entry.Name) + } + if opts.Check { + return + } + if err := s.reinstallPinned(ctx, opts, entry, pl, dbOK); err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: entry.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + result.Installed = append(result.Installed, entry.Name) +} + +// entryMatchesInstalled reports whether the installed plugin's pinned digest +// still matches the lock entry and EVERY client directory's on-disk +// contentDigest does too. Checking only one client's copy would leave +// tampering with any other client's materialized files invisible to +// --check — and which directory got checked would depend on install order. +func (s *service) entryMatchesInstalled(entry lockfile.Entry, pl plugins.InstalledPlugin) bool { + if pl.Digest != entry.Digest { + return false + } + if len(pl.Clients) == 0 { + return false + } + for _, clientType := range pl.Clients { + dir, err := s.pluginInstallPath(clientType, pl.Metadata.Name, pl.Scope, pl.ProjectRoot) + if err != nil { + return false + } + contentDigest, err := lockfile.ContentDigestFromDir(dir) + if err != nil || contentDigest != entry.ContentDigest { + return false + } + } + return true +} + +// reinstallPinned reinstalls entry at its pinned reference, preserving its +// recorded Source (never re-resolving) and the clients it was previously +// installed for unless the caller overrides them. +func (s *service) reinstallPinned( + ctx context.Context, opts plugins.SyncOptions, entry lockfile.Entry, existing plugins.InstalledPlugin, dbOK bool, +) error { + pinnedRef, err := buildPinnedReference(entry) + if err != nil { + return fmt.Errorf("pinning %q: %w", entry.Name, err) + } + clients := opts.Clients + if len(clients) == 0 && dbOK { + clients = existing.Clients + } + _, err = s.Install(ctx, plugins.InstallOptions{ + Name: pinnedRef, + Scope: plugins.ScopeProject, + ProjectRoot: opts.ProjectRoot, + Clients: clients, + Force: true, // sync restores exactly the pinned content over any drifted files + LockSource: entry.Source, + LockResolvedReference: entry.ResolvedReference, // preserve — pinnedRef is a restore form + SyncRestore: true, // reinstall despite unchanged Digest — drift is on disk, not the pin + }) + return err +} + +// syncUnlockedInstall classifies a project-scope install that has no lock +// entry: NeverManaged (optionally adopted) or RemovedFromLock (optionally +// pruned), appending the outcome to result. +func (s *service) syncUnlockedInstall( + ctx context.Context, opts plugins.SyncOptions, pl plugins.InstalledPlugin, result *plugins.SyncResult, +) { + if !pl.Managed { + result.NeverManaged = append(result.NeverManaged, pl.Metadata.Name) + if opts.Adopt && !opts.Check { + if err := s.adoptPlugin(ctx, pl); err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: pl.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + } + } + return + } + + result.RemovedFromLock = append(result.RemovedFromLock, pl.Metadata.Name) + if opts.Prune && !opts.Check { + if err := s.Uninstall(ctx, plugins.UninstallOptions{ + Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: opts.ProjectRoot, + }); err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: pl.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + result.Pruned = append(result.Pruned, pl.Metadata.Name) + } +} + +// adoptPlugin writes a lock entry for an existing, unmanaged project-scope +// install, pinning its current on-disk state. The install's own Reference is +// used as Source: an adopted install predates (or never went through) lock +// tracking, so the original user-typed request is not recoverable — the +// concrete resolved reference is the closest available fact to pin against. +// +// Trust state is left unset (no provenance, not unsigned). Plugin Sigstore +// verification lands in a later PR; requiring --allow-unsigned here would +// make every adopt fail until then. Lock validation permits an entry with +// neither provenance nor unsigned. +func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) error { + contentDigest, err := s.computeInstalledContentDigest(pl) + if err != nil { + return fmt.Errorf("computing content digest: %w", err) + } + source := pl.Reference + if source == "" { + source = pl.Metadata.Name + } + if err := recordLockEntry(pl.ProjectRoot, lockEntryInput{ + Name: pl.Metadata.Name, + Version: pl.Metadata.Version, + Source: source, + ResolvedReference: lockableResolvedReference(pl.Reference), + Digest: pl.Digest, + ContentDigest: contentDigest, + }); err != nil { + return fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) + } + pl.Managed = true + if err := s.store.Update(ctx, pl); err != nil { + return fmt.Errorf("marking plugin as lock-managed: %w", err) + } + return nil +} + +// computeInstalledContentDigest hashes every client directory the plugin is +// installed into. All copies must agree — a mismatch is drift, not a pin. +func (s *service) computeInstalledContentDigest(pl plugins.InstalledPlugin) (string, error) { + if len(pl.Clients) == 0 { + return "", fmt.Errorf("plugin %q has no clients", pl.Metadata.Name) + } + var last string + for _, clientType := range pl.Clients { + dir, err := s.pluginInstallPath(clientType, pl.Metadata.Name, pl.Scope, pl.ProjectRoot) + if err != nil { + return "", err + } + digest, err := lockfile.ContentDigestFromDir(dir) + if err != nil { + return "", fmt.Errorf("hashing %s copy of %q: %w", clientType, pl.Metadata.Name, err) + } + if last != "" && digest != last { + return "", fmt.Errorf("content digest mismatch across clients for %q", pl.Metadata.Name) + } + last = digest + } + return last, nil +} + +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) +} + +// lockableResolvedReference returns ref when it is a valid git:// or OCI +// reference suitable for a lock entry's resolvedReference, and empty otherwise. +// Adopted installs from a LayerData/plain-name path may only have the plugin +// name recorded as Reference, which lock validation would reject. +func lockableResolvedReference(ref string) string { + if gitresolver.IsGitReference(ref) { + if _, err := gitresolver.ParseGitReference(ref); err == nil { + return ref + } + return "" + } + parsed, isOCI, err := parseOCIReference(ref) + if err != nil || !isOCI || parsed == nil { + return "" + } + return ref +} + +// classifySyncFailure maps an error from the install/uninstall path to an +// RFC THV-0080 typed failure reason using structured signals those paths +// already attach — the errLockWrite sentinel and httperr status codes — +// rather than matching on error message text. +func classifySyncFailure(err error) plugins.FailureReason { + if errors.Is(err, errLockWrite) { + return plugins.FailureReasonLockWriteFailed + } + switch httperr.Code(err) { + case http.StatusNotFound: + return plugins.FailureReasonDigestMissing + case http.StatusBadGateway, http.StatusGatewayTimeout, http.StatusTooManyRequests: + return plugins.FailureReasonRegistryUnreachable + case http.StatusBadRequest, http.StatusUnprocessableEntity, http.StatusConflict: + return plugins.FailureReasonValidationRejected + default: + return plugins.FailureReasonUnknown + } +} diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go new file mode 100644 index 0000000000..fbd738b16c --- /dev/null +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -0,0 +1,270 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pluginsvc + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/client" + "github.com/stacklok/toolhive/pkg/git" + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage/sqlite" +) + +const gitPluginRef = "git://github.com/org/my-plugin" + +// redirectGitClient clones a local fixture repo regardless of the requested +// URL, so Install can use a github.com git:// reference (ParseGitReference +// rejects file:// and localhost) while still exercising the real clone path. +type redirectGitClient struct { + dir string + inner git.Client +} + +func (c *redirectGitClient) Clone(ctx context.Context, config *git.CloneConfig) (*git.RepositoryInfo, error) { + cloned := *config + cloned.URL = c.dir + return c.inner.Clone(ctx, &cloned) +} + +func (c *redirectGitClient) GetFileContent(repoInfo *git.RepositoryInfo, path string) ([]byte, error) { + return c.inner.GetFileContent(repoInfo, path) +} + +func (c *redirectGitClient) HeadCommit(repoInfo *git.RepositoryInfo) (git.HeadCommit, error) { + return c.inner.HeadCommit(repoInfo) +} + +func (c *redirectGitClient) Cleanup(ctx context.Context, repoInfo *git.RepositoryInfo) error { + return c.inner.Cleanup(ctx, repoInfo) +} + +func newGitLockTestService(t *testing.T, repoDir string) (plugins.PluginService, string) { + t.Helper() + 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) + 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())), + WithGitClient(&redirectGitClient{dir: repoDir, inner: git.NewDefaultGitClient()}), + ) + return svc, projectRoot +} + +func pluginOnDiskPath(projectRoot, name string) string { + return filepath.Join(projectRoot, ".claude", "plugins", name) +} + +func tamperPluginFile(t *testing.T, projectRoot, name string) string { + t.Helper() + path := filepath.Join(pluginOnDiskPath(projectRoot, name), "commands", "hello.md") + require.NoError(t, os.WriteFile(path, []byte("tampered content"), 0o644)) + return path +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_ReportsUpToDateWhenNothingChanged(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.AlreadyCurrent) + assert.Empty(t, result.Installed) + assert.Empty(t, result.Drifted) + assert.Empty(t, result.Failed) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_CheckReportsDriftWithoutWriting(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + path := tamperPluginFile(t, projectRoot, "my-plugin") + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) //nolint:forcetypeassert + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Drifted) + assert.Empty(t, result.Installed, "check must not install/write anything") + + stillTampered, err := os.ReadFile(path) //nolint:gosec // fixed test path + require.NoError(t, err) + assert.Equal(t, "tampered content", string(stillTampered)) +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestSync_ReinstallsDriftedContent(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + svc, projectRoot := newGitLockTestService(t, repoDir) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, Scope: plugins.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + path := tamperPluginFile(t, projectRoot, "my-plugin") + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Drifted) + assert.Equal(t, []string{"my-plugin"}, result.Installed) + + restored, err := os.ReadFile(path) //nolint:gosec // fixed test path + require.NoError(t, err) + assert.Contains(t, string(restored), "# hello") +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestSync_ReinstallPreservesResolvedReference(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + svc, projectRoot := newGitLockTestService(t, repoDir) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, Scope: plugins.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + require.True(t, ok) + require.Equal(t, gitPluginRef, before.ResolvedReference) + + tamperPluginFile(t, projectRoot, "my-plugin") + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + require.Equal(t, []string{"my-plugin"}, result.Installed) + + after, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + require.True(t, ok) + assert.Equal(t, before.ResolvedReference, after.ResolvedReference, + "a drift-repair reinstall must preserve ResolvedReference, not overwrite it with the pinned restore form") +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestSync_MissingInstallIsRestored(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + svc, projectRoot := newGitLockTestService(t, repoDir) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, Scope: plugins.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + require.NoError(t, os.RemoveAll(pluginOnDiskPath(projectRoot, "my-plugin"))) + require.NoError(t, svc.(*service).store.Delete(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)) //nolint:forcetypeassert + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Missing) + assert.Equal(t, []string{"my-plugin"}, result.Installed) + assert.Empty(t, result.Drifted) + + _, err = svc.Info(t.Context(), plugins.InfoOptions{Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestSync_CheckReportsMissingInstalls(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + svc, projectRoot := newGitLockTestService(t, repoDir) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, Scope: plugins.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + require.NoError(t, os.RemoveAll(pluginOnDiskPath(projectRoot, "my-plugin"))) + require.NoError(t, svc.(*service).store.Delete(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot)) //nolint:forcetypeassert + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) //nolint:forcetypeassert + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Missing) + assert.Empty(t, result.Installed) + + _, err = svc.Info(t.Context(), plugins.InfoOptions{Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "check must not have installed anything") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_AdoptsUnmanagedInstall(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin")) + syncSvc := svc.(*service) //nolint:forcetypeassert + legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + legacy.Managed = false + require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) + + result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.NeverManaged) + + result, err = syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Adopt: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.NeverManaged) + assert.Empty(t, result.Failed) + + lf := readLockfile(t, projectRoot) + entry, ok := lf.GetPlugin("my-plugin") + require.True(t, ok, "adopt must write a lock entry for the unmanaged install") + assert.NotEmpty(t, entry.ContentDigest) + assert.False(t, entry.Unsigned) + assert.Nil(t, entry.Provenance) + + 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) + assert.True(t, info.InstalledPlugin.Managed) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_PrunesRemovedFromLock(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin")) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.RemovedFromLock) + + result, err = syncer.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Prune: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Pruned) + + _, err = svc.Info(t.Context(), plugins.InfoOptions{Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "prune must uninstall the plugin") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_DisabledGateReturnsForbidden(t *testing.T) { + svc, projectRoot := newLockTestService(t, false) + + _, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.Error(t, err) + assert.Equal(t, 403, httperr.Code(err)) +} From 2add44fa7e090345680ede4c794b3dd8392587cc Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 10:06:49 +0200 Subject: [PATCH 2/9] Fix adopt rollback and add plugin sync e2e A failed DB update after writing the lock entry left the plugin untracked; remove the entry so the next sync can retry. Cover thv ai-plugin sync exit codes the same way skills lock does. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/lock_test.go | 12 +- pkg/plugins/pluginsvc/sync.go | 11 +- pkg/plugins/pluginsvc/sync_test.go | 44 +++++ test/e2e/cli_plugins_lock_test.go | 274 +++++++++++++++++++++++++++++ 4 files changed, 327 insertions(+), 14 deletions(-) create mode 100644 test/e2e/cli_plugins_lock_test.go diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index dccb1c8ec7..ad267c6d48 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -255,7 +255,7 @@ func TestInstallProjectScope_RollbackRestoresPreExistingState(t *testing.T) { var digestAtFailure string inner.store = &hookPluginStore{ PluginStore: inner.store, - beforeUpdate: func(call int) error { + beforeUpdate: func(call int, _ plugins.InstalledPlugin) 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. @@ -315,7 +315,7 @@ func TestInstallProjectScope_RollbackCompensationErrorIsJoined(t *testing.T) { // pre-existing record restore that follows it. inner.store = &hookPluginStore{ PluginStore: inner.store, - beforeUpdate: func(call int) error { + beforeUpdate: func(call int, _ plugins.InstalledPlugin) error { if call >= 2 { return errors.New("db update unavailable") } @@ -441,9 +441,9 @@ func TestUninstall_DoesNotTouchSkillsKey(t *testing.T) { type hookPluginStore struct { storage.PluginStore beforeDelete func() error - // beforeUpdate runs before each Update with the 1-based call count; - // returning an error fails that Update. - beforeUpdate func(call int) error + // beforeUpdate runs before each Update with the 1-based call count and + // the record being written; returning an error fails that Update. + beforeUpdate func(call int, pl plugins.InstalledPlugin) error updateCalls int } @@ -459,7 +459,7 @@ func (s *hookPluginStore) Delete(ctx context.Context, name string, scope plugins 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 { + if err := s.beforeUpdate(s.updateCalls, pl); err != nil { return err } } diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index b19fb20bc2..f15957fa53 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -10,7 +10,6 @@ import ( "net/http" "github.com/stacklok/toolhive-core/httperr" - "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills/gitresolver" "github.com/stacklok/toolhive/pkg/skills/lockfile" @@ -232,6 +231,9 @@ func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) e } pl.Managed = true if err := s.store.Update(ctx, pl); err != nil { + _ = removeLockEntry(plugins.UninstallOptions{ + Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: pl.ProjectRoot, + }) return fmt.Errorf("marking plugin as lock-managed: %w", err) } return nil @@ -261,13 +263,6 @@ func (s *service) computeInstalledContentDigest(pl plugins.InstalledPlugin) (str return last, nil } -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) -} - // lockableResolvedReference returns ref when it is a valid git:// or OCI // reference suitable for a lock entry's resolvedReference, and empty otherwise. // Adopted installs from a LayerData/plain-name path may only have the plugin diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index fbd738b16c..75da97d8fa 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -5,6 +5,7 @@ package pluginsvc import ( "context" + "errors" "os" "path/filepath" "testing" @@ -268,3 +269,46 @@ func TestSync_DisabledGateReturnsForbidden(t *testing.T) { require.Error(t, err) assert.Equal(t, 403, httperr.Code(err)) } + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_AdoptUpdateFailureRemovesLockEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin")) + syncSvc := svc.(*service) //nolint:forcetypeassert + legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + legacy.Managed = false + require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) + + syncSvc.store = &hookPluginStore{ + PluginStore: syncSvc.store, + beforeUpdate: func(_ int, p plugins.InstalledPlugin) error { + if p.Managed { + return errors.New("db locked") + } + return nil + }, + } + + result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Adopt: true}) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + assert.Equal(t, "my-plugin", result.Failed[0].Name) + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.False(t, ok, "a failed adopt must not leave a lock entry without Managed=true") + + 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) + assert.False(t, info.InstalledPlugin.Managed) + + check, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, check.NeverManaged) + assert.Empty(t, check.AlreadyCurrent, "a split adopt must not look up-to-date") +} diff --git a/test/e2e/cli_plugins_lock_test.go b/test/e2e/cli_plugins_lock_test.go new file mode 100644 index 0000000000..d5b9803c85 --- /dev/null +++ b/test/e2e/cli_plugins_lock_test.go @@ -0,0 +1,274 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e_test + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "os" + "os/exec" + "path/filepath" + + "github.com/google/go-containerregistry/pkg/registry" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/stacklok/toolhive/pkg/plugins" + "github.com/stacklok/toolhive/test/e2e" +) + +var _ = Describe("Plugins CLI lock file exit codes (RFC THV-0080)", Label("api", "cli", "plugins", "plugins-lock", "e2e"), func() { + var ( + config *e2e.ServerConfig + apiServer *e2e.Server + thvConfig *e2e.TestConfig + ) + + BeforeEach(func() { + config = e2e.NewServerConfig() + config.ExtraEnv = []string{"TOOLHIVE_PLUGINS_LOCK_ENABLED=true"} + apiServer = e2e.StartServer(config) + thvConfig = e2e.NewTestConfig() + }) + + thvPluginCmd := func(args ...string) *e2e.THVCommand { + fullArgs := append([]string{"ai-plugin"}, args...) + return e2e.NewTHVCommand(thvConfig, fullArgs...). + WithEnv("TOOLHIVE_API_URL=" + apiServer.BaseURL()) + } + + exitCodeOf := func(err error) int { + var exitErr *exec.ExitError + ExpectWithOffset(1, errors.As(err, &exitErr)).To(BeTrue(), "expected an *exec.ExitError, got %T: %v", err, err) + return exitErr.ExitCode() + } + + Describe("thv ai-plugin sync --check", func() { + It("exits 0 when the project matches its lock file", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-clean-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A clean plugin for CLI exit code testing") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + stdout, _ := thvPluginCmd("sync", "--check", "--project-root", projectRoot).ExpectSuccess() + Expect(stdout).To(ContainSubstring("Up to date")) + Expect(stdout).To(ContainSubstring(pluginName)) + }) + + It("exits 2 when the project has drifted from its lock file", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-drifted-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A drifted plugin for CLI exit code testing") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + By("Deleting the installed files so the project drifts from the lock file") + pluginDir := filepath.Join(projectRoot, ".claude", "plugins", pluginName) + Expect(os.RemoveAll(pluginDir)).To(Succeed()) + + _, _, err := thvPluginCmd("sync", "--check", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred()) + Expect(exitCodeOf(err)).To(Equal(2)) + }) + }) + + Describe("thv ai-plugin sync without --yes", func() { + It("exits 4 when running non-interactively without --yes", func() { + projectRoot := makeE2EProjectRoot() + + _, _, err := thvPluginCmd("sync", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred(), "a non-interactive sync without --yes must refuse rather than proceed silently") + Expect(exitCodeOf(err)).To(Equal(4)) + }) + }) + + Describe("thv ai-plugin sync --yes", func() { + It("exits 0 and proceeds without prompting", func() { + projectRoot := makeE2EProjectRoot() + + stdout, _ := thvPluginCmd("sync", "--yes", "--project-root", projectRoot).ExpectSuccess() + Expect(stdout).To(ContainSubstring("Nothing to sync")) + }) + }) + + Describe("thv ai-plugin sync partial failure", func() { + It("exits 3 when a pinned plugin cannot be reinstalled", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-exit3-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A plugin whose registry will vanish") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + By("Tampering with the installed files and killing the registry, so the repair reinstall must fail") + hello := filepath.Join(projectRoot, ".claude", "plugins", pluginName, "commands", "hello.md") + Expect(os.WriteFile(hello, []byte("tampered"), 0o644)).To(Succeed()) + ociRegistry.Close() + + _, _, err := thvPluginCmd("sync", "--yes", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred(), "a failed repair must not exit 0") + Expect(exitCodeOf(err)).To(Equal(3), "an operational failure is exit 3, not a freshness signal") + }) + }) + + Describe("thv ai-plugin sync --check on a fresh clone", func() { + It("exits 2 when the lock file has entries but nothing is installed", func() { + projectRoot := makeE2EProjectRoot() + pluginName := "cli-lock-freshclone-plugin" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushPlugin(apiServer, ociRegistry, pluginName, "A plugin for the fresh-clone gate") + + installResp := installPlugin(apiServer, installPluginE2ERequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + By("Simulating a fresh clone: the committed lock file survives, local install state does not") + lockPath := filepath.Join(projectRoot, "toolhive.lock.yaml") + lockBytes, err := os.ReadFile(lockPath) //nolint:gosec // fixed test path + Expect(err).ToNot(HaveOccurred()) + uninstallResp := uninstallScopedPlugin(apiServer, pluginName, projectRoot) + defer uninstallResp.Body.Close() + Expect(uninstallResp.StatusCode).To(Equal(http.StatusNoContent)) + Expect(os.WriteFile(lockPath, lockBytes, 0o644)).To(Succeed()) + + _, _, cmdErr := thvPluginCmd("sync", "--check", "--project-root", projectRoot).Run() + Expect(cmdErr).To(HaveOccurred(), "the CI gate must not green-light a checkout with nothing installed") + Expect(exitCodeOf(cmdErr)).To(Equal(2)) + }) + }) +}) + +type installPluginE2ERequest struct { + Name string `json:"name"` + Scope string `json:"scope,omitempty"` + ProjectRoot string `json:"project_root,omitempty"` + Clients []string `json:"clients,omitempty"` +} + +func installPlugin(server *e2e.Server, req installPluginE2ERequest) *http.Response { + jsonData, err := json.Marshal(req) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + resp, err := http.Post( + server.BaseURL()+"/api/v1beta/plugins", + "application/json", + bytes.NewBuffer(jsonData), + ) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + +func uninstallScopedPlugin(server *e2e.Server, name, projectRoot string) *http.Response { + u := fmt.Sprintf("%s/api/v1beta/plugins/%s?scope=project&project_root=%s", + server.BaseURL(), name, url.QueryEscape(projectRoot)) + req, err := http.NewRequest(http.MethodDelete, u, nil) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + resp, err := http.DefaultClient.Do(req) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + +func buildPlugin(server *e2e.Server, path, tag string) *http.Response { + reqBody := struct { + Path string `json:"path"` + Tag string `json:"tag,omitempty"` + }{Path: path, Tag: tag} + jsonData, err := json.Marshal(reqBody) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + resp, err := http.Post( + server.BaseURL()+"/api/v1beta/plugins/build", + "application/json", + bytes.NewBuffer(jsonData), + ) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + +func pushPlugin(server *e2e.Server, reference string) *http.Response { + reqBody := struct { + Reference string `json:"reference"` + }{Reference: reference} + jsonData, err := json.Marshal(reqBody) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + resp, err := http.Post( + server.BaseURL()+"/api/v1beta/plugins/push", + "application/json", + bytes.NewBuffer(jsonData), + ) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + +func createTestPluginDir(pluginName, description string) string { + parentDir := GinkgoT().TempDir() + pluginDir := filepath.Join(parentDir, pluginName) + ExpectWithOffset(1, os.MkdirAll(filepath.Join(pluginDir, ".claude-plugin"), 0o755)).To(Succeed()) + ExpectWithOffset(1, os.MkdirAll(filepath.Join(pluginDir, "commands"), 0o755)).To(Succeed()) + + manifest := fmt.Sprintf(`{ + "name": %q, + "description": %q, + "version": "0.1.0", + "license": "Apache-2.0", + "keywords": ["test"] +}`, pluginName, description) + ExpectWithOffset(1, os.WriteFile( + filepath.Join(pluginDir, plugins.ManifestPath), + []byte(manifest), + 0o644, + )).To(Succeed()) + ExpectWithOffset(1, os.WriteFile( + filepath.Join(pluginDir, "commands", "hello.md"), + []byte("# hello\n"), + 0o644, + )).To(Succeed()) + + return pluginDir +} + +func buildAndPushPlugin(server *e2e.Server, ociRegistry *httptest.Server, pluginName, description string) string { + ociRef := fmt.Sprintf("%s/e2e-test/%s:v0.1.0", ociRegistry.Listener.Addr().String(), pluginName) + + pluginDir := createTestPluginDir(pluginName, description) + buildResp := buildPlugin(server, pluginDir, ociRef) + defer buildResp.Body.Close() + ExpectWithOffset(1, buildResp.StatusCode).To(Equal(http.StatusOK)) + + pushResp := pushPlugin(server, ociRef) + defer pushResp.Body.Close() + ExpectWithOffset(1, pushResp.StatusCode).To(Equal(http.StatusNoContent)) + + return ociRef +} From 5bf6319d87742265e7de8aa8d4808ca234c7c769 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 10:44:21 +0200 Subject: [PATCH 3/9] Require restorable pins and client health on sync Sync must not report a plugin current when a requested client is missing or marketplace registration is gone, and must refuse to adopt a local tag that cannot be restored later. Signed-off-by: Samuele Verzi --- pkg/plugins/adapters/claudecode_test.go | 25 ++++++++++ pkg/plugins/adapters/codex_test.go | 20 ++++++++ pkg/plugins/pluginsvc/sync.go | 56 +++++++++++++++++---- pkg/plugins/pluginsvc/sync_test.go | 65 +++++++++++++++++++++++++ 4 files changed, 157 insertions(+), 9 deletions(-) diff --git a/pkg/plugins/adapters/claudecode_test.go b/pkg/plugins/adapters/claudecode_test.go index 5c29cd9fc1..3f0739baad 100644 --- a/pkg/plugins/adapters/claudecode_test.go +++ b/pkg/plugins/adapters/claudecode_test.go @@ -595,3 +595,28 @@ func TestClaudeCodeAdapter_EnsureRegisteredRestoresSettings(t *testing.T) { mp := readClaudeMarketplaceManifest(t, filepath.Join(tempHome, ".claude", "plugins")) requireMarketplacePlugin(t, mp, "my-plugin") } + +func TestClaudeCodeAdapter_HealthDetectsMissingRegistration(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) + + req := plugins.DematerializeRequest{Name: "my-plugin", Scope: plugins.ScopeUser} + require.NoError(t, a.Health(context.Background(), req)) + + require.NoError(t, removeClaudeMarketplace(filepath.Join(tempHome, ".claude", "plugins"), "my-plugin")) + err = a.Health(context.Background(), req) + require.Error(t, err) + assert.Contains(t, err.Error(), "marketplace.json") +} diff --git a/pkg/plugins/adapters/codex_test.go b/pkg/plugins/adapters/codex_test.go index 3bc90d71ea..2e82b50e0b 100644 --- a/pkg/plugins/adapters/codex_test.go +++ b/pkg/plugins/adapters/codex_test.go @@ -326,6 +326,26 @@ func TestCodexAdapter_EnsureRegisteredRestoresMarketplace(t *testing.T) { assert.Equal(t, "./toolhive/foo", p.Source.Path) } +func TestCodexAdapter_HealthDetectsMissingRegistration(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)) + + req := plugins.DematerializeRequest{Name: "foo", Scope: plugins.ScopeUser} + require.NoError(t, a.Health(context.Background(), req)) + + require.NoError(t, removeCodexMarketplace(codexUserMarketplaceFile(tempHome), "foo")) + err := a.Health(context.Background(), req) + require.Error(t, err) + assert.Contains(t, err.Error(), "marketplace.json") +} + // 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/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index f15957fa53..aeb7735b84 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -95,7 +95,7 @@ func (s *service) syncLockedEntry( dbOK bool, result *plugins.SyncResult, ) { - if dbOK && s.entryMatchesInstalled(entry, pl) { + if dbOK && pl.Managed && s.entryMatchesInstalled(ctx, entry, pl, opts.Clients) { result.AlreadyCurrent = append(result.AlreadyCurrent, entry.Name) return } @@ -116,18 +116,32 @@ func (s *service) syncLockedEntry( result.Installed = append(result.Installed, entry.Name) } -// entryMatchesInstalled reports whether the installed plugin's pinned digest -// still matches the lock entry and EVERY client directory's on-disk -// contentDigest does too. Checking only one client's copy would leave -// tampering with any other client's materialized files invisible to -// --check — and which directory got checked would depend on install order. -func (s *service) entryMatchesInstalled(entry lockfile.Entry, pl plugins.InstalledPlugin) bool { +// entryMatchesInstalled reports whether the installed plugin is lock-managed, +// its pinned digest still matches the lock entry, every requested client is +// present, EVERY client directory's on-disk contentDigest matches, and each +// adapter reports the plugin as healthy (marketplace/settings present). +// Checking only one client's copy would leave tampering with any other +// client's materialized files invisible to --check — and which directory got +// checked would depend on install order. Shared registration files are +// validated via adapter Health, not folded into contentDigest. +func (s *service) entryMatchesInstalled( + ctx context.Context, + entry lockfile.Entry, + pl plugins.InstalledPlugin, + requestedClients []string, +) bool { + if !pl.Managed { + return false + } if pl.Digest != entry.Digest { return false } if len(pl.Clients) == 0 { return false } + if len(requestedClients) > 0 && !clientsContainAll(pl.Clients, requestedClients) { + return false + } for _, clientType := range pl.Clients { dir, err := s.pluginInstallPath(clientType, pl.Metadata.Name, pl.Scope, pl.ProjectRoot) if err != nil { @@ -137,6 +151,17 @@ func (s *service) entryMatchesInstalled(entry lockfile.Entry, pl plugins.Install if err != nil || contentDigest != entry.ContentDigest { return false } + adapter, ok := s.materializers[clientType] + if !ok { + return false + } + if err := adapter.Health(ctx, plugins.DematerializeRequest{ + Name: pl.Metadata.Name, + Scope: pl.Scope, + ProjectRoot: pl.ProjectRoot, + }); err != nil { + return false + } } return true } @@ -205,6 +230,8 @@ func (s *service) syncUnlockedInstall( // used as Source: an adopted install predates (or never went through) lock // tracking, so the original user-typed request is not recoverable — the // concrete resolved reference is the closest available fact to pin against. +// Adoption is rejected when that reference is not a restorable git:// or OCI +// pin (a bare local-store tag cannot be re-fetched later). // // Trust state is left unset (no provenance, not unsigned). Plugin Sigstore // verification lands in a later PR; requiring --allow-unsigned here would @@ -219,11 +246,19 @@ func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) e if source == "" { source = pl.Metadata.Name } + resolved := lockableResolvedReference(pl.Reference) + if resolved == "" { + return httperr.WithCode( + fmt.Errorf("cannot adopt %q: reference %q is not a restorable git or OCI pin", + pl.Metadata.Name, pl.Reference), + http.StatusUnprocessableEntity, + ) + } if err := recordLockEntry(pl.ProjectRoot, lockEntryInput{ Name: pl.Metadata.Name, Version: pl.Metadata.Version, Source: source, - ResolvedReference: lockableResolvedReference(pl.Reference), + ResolvedReference: resolved, Digest: pl.Digest, ContentDigest: contentDigest, }); err != nil { @@ -231,9 +266,12 @@ func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) e } pl.Managed = true if err := s.store.Update(ctx, pl); err != nil { - _ = removeLockEntry(plugins.UninstallOptions{ + remErr := removeLockEntry(plugins.UninstallOptions{ Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: pl.ProjectRoot, }) + if remErr != nil { + return fmt.Errorf("marking plugin as lock-managed: %w", errors.Join(err, remErr)) + } return fmt.Errorf("marking plugin as lock-managed: %w", err) } return nil diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 75da97d8fa..c3ea06969f 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -217,6 +217,7 @@ func TestSync_AdoptsUnmanagedInstall(t *testing.T) { legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) require.NoError(t, err) legacy.Managed = false + legacy.Reference = "ghcr.io/org/my-plugin:v1" require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) @@ -280,6 +281,7 @@ func TestSync_AdoptUpdateFailureRemovesLockEntry(t *testing.T) { legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) require.NoError(t, err) legacy.Managed = false + legacy.Reference = "ghcr.io/org/my-plugin:v1" require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) syncSvc.store = &hookPluginStore{ @@ -312,3 +314,66 @@ func TestSync_AdoptUpdateFailureRemovesLockEntry(t *testing.T) { assert.Equal(t, []string{"my-plugin"}, check.NeverManaged) assert.Empty(t, check.AlreadyCurrent, "a split adopt must not look up-to-date") } + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_AdoptRejectsUnrestorableLocalPin(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin")) + syncSvc := svc.(*service) //nolint:forcetypeassert + legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + legacy.Managed = false + legacy.Reference = "my-plugin" + require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) + + result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Adopt: true}) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + assert.Contains(t, result.Failed[0].Error, "not a restorable git or OCI pin") + assert.Equal(t, plugins.FailureReasonValidationRejected, result.Failed[0].Reason) + + _, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + assert.False(t, ok, "adoption of a bare local tag must not write a lock entry") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_RequestedClientIsNotAlreadyCurrent(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Check: true, Clients: []string{"codex"}, + }) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Drifted) + assert.Empty(t, result.AlreadyCurrent, "a plugin current in one client must not skip a requested extra client") +} + +type unhealthyAdapter struct { + extractingAdapter +} + +func (*unhealthyAdapter) Health(context.Context, plugins.DematerializeRequest) error { + return errors.New("marketplace entry missing") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_UnhealthyRegistrationIsNotAlreadyCurrent(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["claude-code"] = &unhealthyAdapter{ + extractingAdapter: extractingAdapter{ + base: filepath.Join(projectRoot, ".claude", "plugins"), + installer: skills.NewInstaller(), + }, + } + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Drifted) + assert.Empty(t, result.AlreadyCurrent, "missing marketplace/settings registration is drift, not current") +} From 0ef0c4bee2dfd2c837be57ab72163549d2016a6f Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Fri, 14 Aug 2026 14:50:23 +0200 Subject: [PATCH 4/9] Serialize adopt and restore the prior lock pin Adoption must hold the per-plugin lock and put back the pre-existing entry if marking Managed fails. Signed-off-by: Samuele Verzi --- pkg/plugins/adapters/claudecode_test.go | 51 +++++++++++++++++++++++++ pkg/plugins/pluginsvc/sync.go | 48 ++++++++++++++++++++--- pkg/plugins/pluginsvc/sync_test.go | 43 +++++++++++++++++++++ 3 files changed, 137 insertions(+), 5 deletions(-) diff --git a/pkg/plugins/adapters/claudecode_test.go b/pkg/plugins/adapters/claudecode_test.go index 3f0739baad..82a3d95fea 100644 --- a/pkg/plugins/adapters/claudecode_test.go +++ b/pkg/plugins/adapters/claudecode_test.go @@ -620,3 +620,54 @@ func TestClaudeCodeAdapter_HealthDetectsMissingRegistration(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "marketplace.json") } + +func TestClaudeCodeAdapter_HealthRequiresFullRegistration(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) + + req := plugins.DematerializeRequest{Name: "my-plugin", Scope: plugins.ScopeUser} + require.NoError(t, a.Health(context.Background(), req)) + + marketplaceRoot := filepath.Join(tempHome, ".claude", "plugins") + mpPath := claudeMarketplaceFilePath(marketplaceRoot) + mp, err := readClaudeMarketplace(mpPath) + require.NoError(t, err) + require.NotEmpty(t, mp.Plugins) + mp.Plugins[0].Source = "./other-plugin" + require.NoError(t, writeClaudeMarketplace(mpPath, mp)) + err = a.Health(context.Background(), req) + require.Error(t, err) + assert.Contains(t, err.Error(), "marketplace source") + + mp.Plugins[0].Source = "./my-plugin" + require.NoError(t, writeClaudeMarketplace(mpPath, mp)) + require.NoError(t, a.Health(context.Background(), req)) + + settingsPath := a.settingsPath(plugins.ScopeUser, "") + content, err := os.ReadFile(settingsPath) + require.NoError(t, err) + root, err := parseSettings(content, settingsPath) + require.NoError(t, err) + delete(root, "extraKnownMarketplaces") + require.NoError(t, writeSettings(root, settingsPath)) + err = a.Health(context.Background(), req) + require.Error(t, err) + assert.Contains(t, err.Error(), "extraKnownMarketplaces.toolhive") + + require.NoError(t, enablePluginInSettings(settingsPath, "my-plugin", marketplaceRoot+"/wrong")) + err = a.Health(context.Background(), req) + require.Error(t, err) + assert.Contains(t, err.Error(), "extraKnownMarketplaces.toolhive path") +} diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index aeb7735b84..6986d5152d 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -238,6 +238,18 @@ func (s *service) syncUnlockedInstall( // make every adopt fail until then. Lock validation permits an entry with // neither provenance nor unsigned. func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) error { + unlock := s.locks.lock(pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) + defer unlock() + + current, err := s.store.Get(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) + if err != nil { + return fmt.Errorf("re-reading plugin before adopt: %w", err) + } + if current.Managed { + return nil + } + pl = current + contentDigest, err := s.computeInstalledContentDigest(pl) if err != nil { return fmt.Errorf("computing content digest: %w", err) @@ -254,6 +266,20 @@ func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) e http.StatusUnprocessableEntity, ) } + + root, err := lockfile.OpenRoot(pl.ProjectRoot) + if err != nil { + return fmt.Errorf("opening lock file root: %w", err) + } + lf, err := lockfile.Load(root) + if err != nil { + return fmt.Errorf("loading lock file: %w", err) + } + var prevEntry *lockfile.Entry + if e, ok := lf.GetPlugin(pl.Metadata.Name); ok { + prevEntry = &e + } + if err := recordLockEntry(pl.ProjectRoot, lockEntryInput{ Name: pl.Metadata.Name, Version: pl.Metadata.Version, @@ -266,17 +292,29 @@ func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) e } pl.Managed = true if err := s.store.Update(ctx, pl); err != nil { - remErr := removeLockEntry(plugins.UninstallOptions{ - Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: pl.ProjectRoot, - }) - if remErr != nil { - return fmt.Errorf("marking plugin as lock-managed: %w", errors.Join(err, remErr)) + if restoreErr := restoreAdoptedLockEntry(pl, prevEntry); restoreErr != nil { + return fmt.Errorf("marking plugin as lock-managed: %w", errors.Join(err, restoreErr)) } return fmt.Errorf("marking plugin as lock-managed: %w", err) } return nil } +// restoreAdoptedLockEntry undoes adoptPlugin's lock write: reinstates the +// entry observed before adoption, or removes the name if none existed. +func restoreAdoptedLockEntry(pl plugins.InstalledPlugin, prevEntry *lockfile.Entry) error { + if prevEntry != nil { + root, err := lockfile.OpenRoot(pl.ProjectRoot) + if err != nil { + return err + } + return lockfile.UpsertPluginEntry(root, *prevEntry) + } + return removeLockEntry(plugins.UninstallOptions{ + Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: pl.ProjectRoot, + }) +} + // computeInstalledContentDigest hashes every client directory the plugin is // installed into. All copies must agree — a mismatch is drift, not a pin. func (s *service) computeInstalledContentDigest(pl plugins.InstalledPlugin) (string, error) { diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index c3ea06969f..446319337e 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -315,6 +315,49 @@ func TestSync_AdoptUpdateFailureRemovesLockEntry(t *testing.T) { assert.Empty(t, check.AlreadyCurrent, "a split adopt must not look up-to-date") } +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_AdoptUpdateFailureRestoresExistingLockEntry(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + prev := lockfile.Entry{ + Name: "my-plugin", + Source: "ghcr.io/org/other:v0", + ResolvedReference: "ghcr.io/org/other@" + validLockDigestAlt(), + Digest: validLockDigestAlt(), + ContentDigest: "sha256:" + "1111111111111111111111111111111111111111111111111111111111111111", + Explicit: true, + } + require.NoError(t, lockfile.UpsertPluginEntry(mustOpenRoot(t, projectRoot), prev)) + + syncSvc := svc.(*service) //nolint:forcetypeassert + legacy, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + legacy.Managed = false + legacy.Reference = "ghcr.io/org/my-plugin:v1" + require.NoError(t, syncSvc.store.Update(t.Context(), legacy)) + + syncSvc.store = &hookPluginStore{ + PluginStore: syncSvc.store, + beforeUpdate: func(p plugins.InstalledPlugin) error { + if p.Managed { + return errors.New("db locked") + } + return nil + }, + } + + result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Adopt: true}) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + + got, ok := readLockfile(t, projectRoot).GetPlugin("my-plugin") + require.True(t, ok, "a failed adopt must restore the pre-existing lock pin") + assert.Equal(t, prev.Source, got.Source) + assert.Equal(t, prev.Digest, got.Digest) + assert.Equal(t, prev.ResolvedReference, got.ResolvedReference) +} + //nolint:paralleltest // uses t.Setenv via newLockTestService func TestSync_AdoptRejectsUnrestorableLocalPin(t *testing.T) { svc, projectRoot := newLockTestService(t, true) From dcfb623627417b887ac090eb681886de757fed4b Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 17:17:31 +0200 Subject: [PATCH 5/9] Reconcile each plugin under its lock on sync A stale lock/DB snapshot can resurrect uninstalls; default sync must also expand to newly detected clients. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/install.go | 14 +--- pkg/plugins/pluginsvc/install_git.go | 2 +- pkg/plugins/pluginsvc/install_oci.go | 2 +- pkg/plugins/pluginsvc/service.go | 29 ++++++++ pkg/plugins/pluginsvc/sync.go | 102 +++++++++++++++++++-------- pkg/plugins/pluginsvc/sync_test.go | 58 +++++++++++++++ pkg/plugins/pluginsvc/uninstall.go | 2 +- 7 files changed, 166 insertions(+), 43 deletions(-) diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index 5e03eeaee8..e17eae3902 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -81,13 +81,8 @@ func (s *service) installByName( opts plugins.InstallOptions, scope plugins.Scope, ) (*plugins.InstallResult, error) { - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) - locked := true - defer func() { - if locked { - unlock() - } - }() + ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) + defer unlock() if len(opts.LayerData) == 0 { resolved := false @@ -99,11 +94,6 @@ func (s *service) installByName( } } if !resolved { - // Release the lock before registry lookup — installFromOCI - // acquires its own lock on the plugin name, which could be the - // same key, causing deadlock since sync.Mutex is not re-entrant. - unlock() - locked = false return s.installFromRegistryLookup(ctx, opts, scope) } } diff --git a/pkg/plugins/pluginsvc/install_git.go b/pkg/plugins/pluginsvc/install_git.go index 2a56b5e96b..8e90f34218 100644 --- a/pkg/plugins/pluginsvc/install_git.go +++ b/pkg/plugins/pluginsvc/install_git.go @@ -91,7 +91,7 @@ func (s *service) installFromGit( opts.Version = manifest.Version } - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) defer unlock() result, err := s.installWithExtraction(ctx, opts, scope) diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index 292962d951..059e38f5db 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -108,7 +108,7 @@ func (s *service) installFromOCI( opts.Version = pluginConfig.Version } - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) defer unlock() result, err := s.installWithExtraction(ctx, opts, scope) diff --git a/pkg/plugins/pluginsvc/service.go b/pkg/plugins/pluginsvc/service.go index ec97f72d9e..c95afba407 100644 --- a/pkg/plugins/pluginsvc/service.go +++ b/pkg/plugins/pluginsvc/service.go @@ -161,6 +161,35 @@ func (pl *pluginLock) lock(name string, scope plugins.Scope, projectRoot string) return m.Unlock } +type heldPluginLockKey struct{} + +type heldPluginLock struct { + name string + scope plugins.Scope + projectRoot string +} + +// lockPlugin acquires the per-plugin mutex unless ctx already holds it for +// this key, so Sync/Upgrade can serialize read+mutate and then call +// Install/Uninstall without deadlocking on a non-reentrant mutex. +func (s *service) lockPlugin( + ctx context.Context, + name string, + scope plugins.Scope, + projectRoot string, +) (context.Context, func()) { + if held, ok := ctx.Value(heldPluginLockKey{}).(heldPluginLock); ok { + if held.name == name && held.scope == scope && held.projectRoot == projectRoot { + return ctx, func() {} + } + } + unlock := s.locks.lock(name, scope, projectRoot) + ctx = context.WithValue(ctx, heldPluginLockKey{}, heldPluginLock{ + name: name, scope: scope, projectRoot: projectRoot, + }) + return ctx, unlock +} + // service is the default implementation of plugins.PluginService. It implements // the build/validate/push/content surface (Phase 2) and the install/uninstall/ // list/info surface (Phase 3), the latter driving per-client materialization diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 6986d5152d..65a95e5030 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -55,21 +55,23 @@ func (s *service) Sync(ctx context.Context, opts plugins.SyncOptions) (*plugins. if err != nil { return nil, fmt.Errorf("listing installed plugins: %w", err) } - installedByName := make(map[string]plugins.InstalledPlugin, len(installed)) - for _, pl := range installed { - installedByName[pl.Metadata.Name] = pl - } - result := &plugins.SyncResult{} + names := make([]string, 0, len(lf.Plugins)+len(installed)) + seen := make(map[string]struct{}, len(lf.Plugins)+len(installed)) for _, entry := range lf.Plugins { - pl, dbOK := installedByName[entry.Name] - s.syncLockedEntry(ctx, opts, entry, pl, dbOK, result) + names = append(names, entry.Name) + seen[entry.Name] = struct{}{} } for _, pl := range installed { - if _, ok := lf.GetPlugin(pl.Metadata.Name); ok { - continue // handled by the loop above + if _, ok := seen[pl.Metadata.Name]; ok { + continue } - s.syncUnlockedInstall(ctx, opts, pl, result) + names = append(names, pl.Metadata.Name) + } + + result := &plugins.SyncResult{} + for _, name := range names { + s.syncOne(ctx, opts, name, result) } return result, nil @@ -82,6 +84,51 @@ func (*service) Upgrade(_ context.Context, _ plugins.UpgradeOptions) (*plugins.U return nil, httperr.WithCode(errors.New("plugin upgrade is not implemented"), http.StatusNotImplemented) } +// syncOne re-reads the lock entry and DB row under the per-plugin lock, then +// reconciles that fresh state. The initial Sync snapshot is only used to +// discover names; mutation from a stale view would resurrect an uninstall or +// prune a concurrent install. +func (s *service) syncOne( + ctx context.Context, opts plugins.SyncOptions, name string, result *plugins.SyncResult, +) { + ctx, unlock := s.lockPlugin(ctx, name, plugins.ScopeProject, opts.ProjectRoot) + defer unlock() + + root, err := lockfile.OpenRoot(opts.ProjectRoot) + if err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + lf, err := lockfile.Load(root) + if err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + entry, hasEntry := lf.GetPlugin(name) + + pl, err := s.store.Get(ctx, name, plugins.ScopeProject, opts.ProjectRoot) + dbOK := err == nil + if err != nil && !errors.Is(err, storage.ErrNotFound) { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + + if hasEntry { + s.syncLockedEntry(ctx, opts, entry, pl, dbOK, result) + return + } + if !dbOK { + return + } + s.syncUnlockedInstall(ctx, opts, pl, result) +} + // syncLockedEntry reconciles one lock file entry against installed state, // appending its outcome to result. Missing (dbOK false) and drifted (digest // or contentDigest mismatch) entries are reinstalled at the pinned reference @@ -107,7 +154,7 @@ func (s *service) syncLockedEntry( if opts.Check { return } - if err := s.reinstallPinned(ctx, opts, entry, pl, dbOK); err != nil { + if err := s.reinstallPinned(ctx, opts, entry); err != nil { result.Failed = append(result.Failed, plugins.SyncFailure{ Name: entry.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -117,12 +164,11 @@ func (s *service) syncLockedEntry( } // entryMatchesInstalled reports whether the installed plugin is lock-managed, -// its pinned digest still matches the lock entry, every requested client is -// present, EVERY client directory's on-disk contentDigest matches, and each -// adapter reports the plugin as healthy (marketplace/settings present). -// Checking only one client's copy would leave tampering with any other -// client's materialized files invisible to --check — and which directory got -// checked would depend on install order. Shared registration files are +// its pinned digest still matches the lock entry, every expected client is +// present, every checked client directory's on-disk contentDigest matches, +// and each adapter reports the plugin as healthy. With no --clients override, +// expected clients are every detected plugin-supporting client so a newly +// installed client is not treated as current. Shared registration files are // validated via adapter Health, not folded into contentDigest. func (s *service) entryMatchesInstalled( ctx context.Context, @@ -139,10 +185,14 @@ func (s *service) entryMatchesInstalled( if len(pl.Clients) == 0 { return false } - if len(requestedClients) > 0 && !clientsContainAll(pl.Clients, requestedClients) { + expected := requestedClients + if len(expected) == 0 { + expected = s.availableMaterializerClients() + } + if len(expected) == 0 || !clientsContainAll(pl.Clients, expected) { return false } - for _, clientType := range pl.Clients { + for _, clientType := range mergeClientLists(pl.Clients, expected) { dir, err := s.pluginInstallPath(clientType, pl.Metadata.Name, pl.Scope, pl.ProjectRoot) if err != nil { return false @@ -167,24 +217,20 @@ func (s *service) entryMatchesInstalled( } // reinstallPinned reinstalls entry at its pinned reference, preserving its -// recorded Source (never re-resolving) and the clients it was previously -// installed for unless the caller overrides them. +// recorded Source (never re-resolving). Empty opts.Clients keeps Install's +// all-detected default so a newly detected client is materialized. func (s *service) reinstallPinned( - ctx context.Context, opts plugins.SyncOptions, entry lockfile.Entry, existing plugins.InstalledPlugin, dbOK bool, + ctx context.Context, opts plugins.SyncOptions, entry lockfile.Entry, ) error { pinnedRef, err := buildPinnedReference(entry) if err != nil { return fmt.Errorf("pinning %q: %w", entry.Name, err) } - clients := opts.Clients - if len(clients) == 0 && dbOK { - clients = existing.Clients - } _, err = s.Install(ctx, plugins.InstallOptions{ Name: pinnedRef, Scope: plugins.ScopeProject, ProjectRoot: opts.ProjectRoot, - Clients: clients, + Clients: opts.Clients, Force: true, // sync restores exactly the pinned content over any drifted files LockSource: entry.Source, LockResolvedReference: entry.ResolvedReference, // preserve — pinnedRef is a restore form @@ -238,7 +284,7 @@ func (s *service) syncUnlockedInstall( // make every adopt fail until then. Lock validation permits an entry with // neither provenance nor unsigned. func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) error { - unlock := s.locks.lock(pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) + ctx, unlock := s.lockPlugin(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) defer unlock() current, err := s.store.Get(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 446319337e..501c84aea7 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -19,6 +19,7 @@ import ( "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage" "github.com/stacklok/toolhive/pkg/storage/sqlite" ) @@ -394,6 +395,63 @@ func TestSync_RequestedClientIsNotAlreadyCurrent(t *testing.T) { assert.Empty(t, result.AlreadyCurrent, "a plugin current in one client must not skip a requested extra client") } +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_DefaultExpandsToNewlyDetectedClients(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["codex"] = &extractingAdapter{ + base: filepath.Join(projectRoot, ".agents", "plugins", "toolhive"), + installer: skills.NewInstaller(), + } + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.Drifted) + assert.Empty(t, result.AlreadyCurrent, "default sync must not treat a missing detected client as current") +} + +type staleListStore struct { + storage.PluginStore + listed []plugins.InstalledPlugin +} + +func (s *staleListStore) List(context.Context, storage.ListFilter) ([]plugins.InstalledPlugin, error) { + out := make([]plugins.InstalledPlugin, len(s.listed)) + copy(out, s.listed) + return out, nil +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_StaleListDoesNotResurrectUninstalledPlugin(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + stale, err := inner.store.List(t.Context(), storage.ListFilter{ + Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.NoError(t, err) + require.NotEmpty(t, stale) + + require.NoError(t, svc.Uninstall(t.Context(), plugins.UninstallOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + })) + + inner.store = &staleListStore{PluginStore: inner.store, listed: stale} + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Empty(t, result.Installed) + assert.Empty(t, result.NeverManaged) + + _, err = svc.Info(t.Context(), plugins.InfoOptions{ + Name: "my-plugin", Scope: plugins.ScopeProject, ProjectRoot: projectRoot, + }) + require.Error(t, err, "a stale List snapshot must not resurrect an uninstalled plugin") +} + type unhealthyAdapter struct { extractingAdapter } diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index 33716f1b9a..a0f4e13e53 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -41,7 +41,7 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) scope = defaultScope(scope) opts.ProjectRoot = projectRoot - unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) + ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) defer unlock() existing, err := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) From 856cbf429c25081859797b80183dc7359bd711a4 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 19:13:51 +0200 Subject: [PATCH 6/9] Make plugin sync use detected clients and locked helpers Default sync targets installed plugin clients, validates pinned canonical names before mutate, and drops context lock reentrancy. Signed-off-by: Samuele Verzi --- pkg/plugins/options.go | 4 + pkg/plugins/pluginsvc/install.go | 56 +++++++++--- pkg/plugins/pluginsvc/install_extraction.go | 29 +++++-- pkg/plugins/pluginsvc/install_git.go | 12 ++- pkg/plugins/pluginsvc/install_oci.go | 12 ++- pkg/plugins/pluginsvc/install_test.go | 4 +- pkg/plugins/pluginsvc/lock_test.go | 5 +- pkg/plugins/pluginsvc/service.go | 25 +----- pkg/plugins/pluginsvc/sync.go | 94 ++++++++++++++++----- pkg/plugins/pluginsvc/sync_test.go | 87 ++++++++++++++++++- pkg/plugins/pluginsvc/uninstall.go | 10 ++- 11 files changed, 270 insertions(+), 68 deletions(-) diff --git a/pkg/plugins/options.go b/pkg/plugins/options.go index 383f8dc6d3..b3118da912 100644 --- a/pkg/plugins/options.go +++ b/pkg/plugins/options.go @@ -67,6 +67,10 @@ type InstallOptions struct { // normal "same digest means content is already correct" fast path must // not apply. Internal use only — NOT exposed via HTTP API. SyncRestore bool `json:"-"` + // ExpectedCanonicalName, when set, requires the resolved plugin/manifest name + // to equal this value before any install mutation. Used by Sync/Upgrade so a + // lock entry cannot be repaired under a different canonical identity. + ExpectedCanonicalName string `json:"-"` } // InstallResult contains the outcome of an Install operation. diff --git a/pkg/plugins/pluginsvc/install.go b/pkg/plugins/pluginsvc/install.go index e17eae3902..0833208b47 100644 --- a/pkg/plugins/pluginsvc/install.go +++ b/pkg/plugins/pluginsvc/install.go @@ -27,6 +27,17 @@ import ( // 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) { + return s.install(ctx, opts, false) +} + +// installAlreadyLocked is for sync/upgrade while the per-plugin lock is held. +func (s *service) installAlreadyLocked(ctx context.Context, opts plugins.InstallOptions) (*plugins.InstallResult, error) { + return s.install(ctx, opts, true) +} + +func (s *service) install( + ctx context.Context, opts plugins.InstallOptions, alreadyLocked bool, +) (*plugins.InstallResult, error) { scope, projectRoot, err := normalizeProjectRoot(opts.Scope, opts.ProjectRoot) if err != nil { return nil, err @@ -39,9 +50,10 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl // Git references are dispatched first; the prefix is unambiguous and // cannot collide with OCI references. installFromGit holds the per-plugin - // lock across extraction, DB, group, lock-file, and rollback. + // lock across extraction, DB, group, lock-file, and rollback unless the + // caller already holds it (alreadyLocked). if gitresolver.IsGitReference(opts.Name) { - return s.installFromGit(ctx, opts, scope) + return s.installFromGit(ctx, opts, scope, alreadyLocked) } // Splice opts.Version as the tag for tag-less OCI-like references. @@ -60,8 +72,8 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl } if isOCI { // installFromOCI holds the per-plugin lock across extraction, DB, - // group, lock-file, and rollback. - return s.installFromOCI(ctx, opts, scope, ref) + // group, lock-file, and rollback unless the caller already holds it. + return s.installFromOCI(ctx, opts, scope, ref, alreadyLocked) } // Plain plugin name. @@ -69,7 +81,22 @@ func (s *service) Install(ctx context.Context, opts plugins.InstallOptions) (*pl return nil, httperr.WithCode(err, http.StatusBadRequest) } - return s.installByName(ctx, opts, scope) + return s.installByName(ctx, opts, scope, alreadyLocked) +} + +// validateExpectedCanonicalName rejects an install whose resolved plugin name +// differs from the lock entry identity Sync/Upgrade is repairing. +func validateExpectedCanonicalName(opts plugins.InstallOptions) error { + if opts.ExpectedCanonicalName == "" || opts.Name == opts.ExpectedCanonicalName { + return nil + } + return httperr.WithCode( + fmt.Errorf( + "plugin name %q does not match lock entry name %q", + opts.Name, opts.ExpectedCanonicalName, + ), + http.StatusUnprocessableEntity, + ) } // installByName handles installation for a validated plain plugin name. It @@ -80,9 +107,16 @@ func (s *service) installByName( ctx context.Context, opts plugins.InstallOptions, scope plugins.Scope, + alreadyLocked bool, ) (*plugins.InstallResult, error) { - ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) - defer unlock() + if !alreadyLocked { + var unlock func() + ctx, unlock = s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) + defer unlock() + } + // Lock is held from here (by us or the caller). Nested OCI/registry + // backends must not re-acquire. + const lockHeld = true if len(opts.LayerData) == 0 { resolved := false @@ -94,7 +128,7 @@ func (s *service) installByName( } } if !resolved { - return s.installFromRegistryLookup(ctx, opts, scope) + return s.installFromRegistryLookup(ctx, opts, scope, lockHeld) } } @@ -126,6 +160,7 @@ func (s *service) installFromRegistryLookup( ctx context.Context, opts plugins.InstallOptions, scope plugins.Scope, + alreadyLocked bool, ) (*plugins.InstallResult, error) { if s.pluginLookup != nil { // Use the last path segment as the search query (matching @@ -164,7 +199,7 @@ func (s *service) installFromRegistryLookup( } if len(matches) == 1 { - return s.installFromRegistryHit(ctx, opts, scope, matches[0]) + return s.installFromRegistryHit(ctx, opts, scope, matches[0], alreadyLocked) } if len(matches) > 1 { @@ -198,6 +233,7 @@ func (s *service) installFromRegistryHit( opts plugins.InstallOptions, scope plugins.Scope, hit PluginSearchHit, + alreadyLocked bool, ) (*plugins.InstallResult, error) { pkg, pkgErr := selectOCIPluginPackage(opts.Name, hit.Packages) if pkgErr != nil { @@ -218,7 +254,7 @@ func (s *service) installFromRegistryHit( http.StatusUnprocessableEntity, ) } - return s.installFromOCI(ctx, opts, scope, ref) + return s.installFromOCI(ctx, opts, scope, ref, alreadyLocked) } // selectOCIPluginPackage selects the first OCI package from a registry entry's diff --git a/pkg/plugins/pluginsvc/install_extraction.go b/pkg/plugins/pluginsvc/install_extraction.go index 7b0d0b357a..20170e11ab 100644 --- a/pkg/plugins/pluginsvc/install_extraction.go +++ b/pkg/plugins/pluginsvc/install_extraction.go @@ -127,6 +127,11 @@ func (s *service) installExtractionSameDigestNewClients( // configured; without one (embedded/test services, WithClientManager is // optional) compensation degrades to dematerialize-only, matching the fresh // and same-digest paths. +// +// SyncRestore is different: sync must materialize exactly the requested target +// clients (the sync client set), not re-merge historical Clients from the DB — +// otherwise an old client that is no longer detected/requested would stay in +// the persisted list forever. func (s *service) installExtractionUpgradeDigest( ctx context.Context, opts plugins.InstallOptions, @@ -134,7 +139,10 @@ func (s *service) installExtractionUpgradeDigest( existing plugins.InstalledPlugin, clientTypes []string, ) (*plugins.InstallResult, error) { - allClients := mergeClientLists(existing.Clients, clientTypes) + allClients := clientTypes + if !opts.SyncRestore { + allClients = mergeClientLists(existing.Clients, clientTypes) + } return s.materializeAndPersist(ctx, opts, scope, allClients, allClients, nil, existing.Managed, false) } @@ -512,9 +520,9 @@ func (s *service) dematerializeAll( // resolveAndValidateClients returns the deduplicated client list to target for // this install. Empty opts.Clients (or the sentinel value "all") expands to -// every client present in s.materializers (additionally filtered by -// cm.SupportsPlugins when a client manager is configured). Explicit client -// names are validated to be present in s.materializers. +// every client from availableMaterializerClients (materializer present, and +// when a client manager is set: SupportsPlugins + IsClientInstalled). Explicit +// client names are validated to be present in s.materializers. // // Unlike skillsvc.resolveAndValidateClients, this does NOT resolve filesystem // paths — the MaterializationAdapter owns path resolution, so the caller @@ -575,13 +583,18 @@ func (s *service) resolveAndValidateClients( } // availableMaterializerClients returns the sorted list of client types that -// have a configured materializer and (when a client manager is set) are -// considered plugin-supporting by it. +// have a configured materializer. When a client manager is set, only clients +// that both support plugins and appear installed on the system are included. +// When the client manager is nil, every materializer key is returned (tests +// and embedded services without detection). func (s *service) availableMaterializerClients() []string { var out []string for ct := range s.materializers { - if s.clientManager != nil && !s.clientManager.SupportsPlugins(client.ClientApp(ct)) { - continue + if s.clientManager != nil { + app := client.ClientApp(ct) + if !s.clientManager.SupportsPlugins(app) || !s.clientManager.IsClientInstalled(app) { + continue + } } out = append(out, ct) } diff --git a/pkg/plugins/pluginsvc/install_git.go b/pkg/plugins/pluginsvc/install_git.go index 8e90f34218..4d6d01ba9c 100644 --- a/pkg/plugins/pluginsvc/install_git.go +++ b/pkg/plugins/pluginsvc/install_git.go @@ -31,6 +31,7 @@ func (s *service) installFromGit( ctx context.Context, opts plugins.InstallOptions, scope plugins.Scope, + alreadyLocked bool, ) (*plugins.InstallResult, error) { if len(s.materializers) == 0 { return nil, httperr.WithCode( @@ -91,8 +92,15 @@ func (s *service) installFromGit( opts.Version = manifest.Version } - ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) - defer unlock() + if err := validateExpectedCanonicalName(opts); err != nil { + return nil, err + } + + if !alreadyLocked { + var unlock func() + ctx, unlock = s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) + defer unlock() + } result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index 059e38f5db..396218c8f4 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -29,6 +29,7 @@ func (s *service) installFromOCI( opts plugins.InstallOptions, scope plugins.Scope, ref nameref.Reference, + alreadyLocked bool, ) (*plugins.InstallResult, error) { if s.registry == nil || s.ociStore == nil { return nil, httperr.WithCode( @@ -108,8 +109,15 @@ func (s *service) installFromOCI( opts.Version = pluginConfig.Version } - ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) - defer unlock() + if err := validateExpectedCanonicalName(opts); err != nil { + return nil, err + } + + if !alreadyLocked { + var unlock func() + ctx, unlock = s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) + defer unlock() + } result, err := s.installWithExtraction(ctx, opts, scope) if err != nil { diff --git a/pkg/plugins/pluginsvc/install_test.go b/pkg/plugins/pluginsvc/install_test.go index a4ccb82c48..6849c60af8 100644 --- a/pkg/plugins/pluginsvc/install_test.go +++ b/pkg/plugins/pluginsvc/install_test.go @@ -173,7 +173,9 @@ func TestInstallWithExtraction(t *testing.T) { return nil }) - svc := newTestService(WithStore(store), WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + home := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(home, ".claude.json"), []byte("{}"), 0o644)) + svc := newTestService(WithStore(store), WithClientManager(client.NewTestClientManagerWithHome(home)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter})) result, err := svc.Install(t.Context(), plugins.InstallOptions{ Name: "my-plugin", diff --git a/pkg/plugins/pluginsvc/lock_test.go b/pkg/plugins/pluginsvc/lock_test.go index ad267c6d48..a9cbb40273 100644 --- a/pkg/plugins/pluginsvc/lock_test.go +++ b/pkg/plugins/pluginsvc/lock_test.go @@ -88,10 +88,13 @@ func newLockTestService(t *testing.T, enableGate bool) (plugins.PluginService, s base: filepath.Join(projectRoot, ".claude", "plugins"), installer: skills.NewInstaller(), } + home := t.TempDir() + // Claude Code RelPath is empty; IsClientInstalled checks ~/.claude.json. + require.NoError(t, os.WriteFile(filepath.Join(home, ".claude.json"), []byte("{}"), 0o644)) svc := New( WithStore(sqlite.NewPluginStore(db)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), - WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + WithClientManager(client.NewTestClientManagerWithHome(home)), ) return svc, projectRoot } diff --git a/pkg/plugins/pluginsvc/service.go b/pkg/plugins/pluginsvc/service.go index c95afba407..9cca4a87a7 100644 --- a/pkg/plugins/pluginsvc/service.go +++ b/pkg/plugins/pluginsvc/service.go @@ -161,33 +161,16 @@ func (pl *pluginLock) lock(name string, scope plugins.Scope, projectRoot string) return m.Unlock } -type heldPluginLockKey struct{} - -type heldPluginLock struct { - name string - scope plugins.Scope - projectRoot string -} - -// lockPlugin acquires the per-plugin mutex unless ctx already holds it for -// this key, so Sync/Upgrade can serialize read+mutate and then call -// Install/Uninstall without deadlocking on a non-reentrant mutex. +// lockPlugin acquires the per-plugin mutex. Callers that already hold the lock +// (Sync/Upgrade) must use the *AlreadyLocked / *Locked helpers instead of +// re-entering through public Install/Uninstall/adoptPlugin. func (s *service) lockPlugin( ctx context.Context, name string, scope plugins.Scope, projectRoot string, ) (context.Context, func()) { - if held, ok := ctx.Value(heldPluginLockKey{}).(heldPluginLock); ok { - if held.name == name && held.scope == scope && held.projectRoot == projectRoot { - return ctx, func() {} - } - } - unlock := s.locks.lock(name, scope, projectRoot) - ctx = context.WithValue(ctx, heldPluginLockKey{}, heldPluginLock{ - name: name, scope: scope, projectRoot: projectRoot, - }) - return ctx, unlock + return ctx, s.locks.lock(name, scope, projectRoot) } // service is the default implementation of plugins.PluginService. It implements diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 65a95e5030..255b980e13 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -8,8 +8,10 @@ import ( "errors" "fmt" "net/http" + "strings" "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/plugins" "github.com/stacklok/toolhive/pkg/skills/gitresolver" "github.com/stacklok/toolhive/pkg/skills/lockfile" @@ -91,9 +93,17 @@ func (*service) Upgrade(_ context.Context, _ plugins.UpgradeOptions) (*plugins.U func (s *service) syncOne( ctx context.Context, opts plugins.SyncOptions, name string, result *plugins.SyncResult, ) { - ctx, unlock := s.lockPlugin(ctx, name, plugins.ScopeProject, opts.ProjectRoot) + _, unlock := s.lockPlugin(ctx, name, plugins.ScopeProject, opts.ProjectRoot) defer unlock() + targetClients, err := s.resolveSyncTargetClients(opts.Clients) + if err != nil { + result.Failed = append(result.Failed, plugins.SyncFailure{ + Name: name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + root, err := lockfile.OpenRoot(opts.ProjectRoot) if err != nil { result.Failed = append(result.Failed, plugins.SyncFailure{ @@ -120,7 +130,7 @@ func (s *service) syncOne( } if hasEntry { - s.syncLockedEntry(ctx, opts, entry, pl, dbOK, result) + s.syncLockedEntry(ctx, opts, entry, pl, dbOK, targetClients, result) return } if !dbOK { @@ -140,9 +150,10 @@ func (s *service) syncLockedEntry( entry lockfile.Entry, pl plugins.InstalledPlugin, dbOK bool, + targetClients []string, result *plugins.SyncResult, ) { - if dbOK && pl.Managed && s.entryMatchesInstalled(ctx, entry, pl, opts.Clients) { + if dbOK && pl.Managed && s.entryMatchesInstalled(ctx, entry, pl, targetClients) { result.AlreadyCurrent = append(result.AlreadyCurrent, entry.Name) return } @@ -154,7 +165,7 @@ func (s *service) syncLockedEntry( if opts.Check { return } - if err := s.reinstallPinned(ctx, opts, entry); err != nil { + if err := s.reinstallPinned(ctx, opts, entry, targetClients); err != nil { result.Failed = append(result.Failed, plugins.SyncFailure{ Name: entry.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -163,9 +174,50 @@ func (s *service) syncLockedEntry( result.Installed = append(result.Installed, entry.Name) } +// resolveSyncTargetClients returns the client set Sync should check and +// restore. Empty requested expands to availableMaterializerClients (detected +// + plugin-supporting). Explicit names are validated like Install (materializer +// + SupportsPlugins) but do not require IsClientInstalled — the caller asked +// for them. +func (s *service) resolveSyncTargetClients(requested []string) ([]string, error) { + if len(requested) == 0 { + return s.availableMaterializerClients(), nil + } + for _, c := range requested { + if c == "" { + return nil, httperr.WithCode( + errors.New("clients entries must be non-empty strings"), + http.StatusBadRequest, + ) + } + if strings.EqualFold(c, clientsAllSentinel) { + return nil, httperr.WithCode( + fmt.Errorf("%q cannot be combined with other client names", clientsAllSentinel), + http.StatusBadRequest, + ) + } + } + requested = dedupeStringsPreserveOrder(requested) + for _, ct := range requested { + if _, ok := s.materializers[ct]; !ok { + return nil, httperr.WithCode( + fmt.Errorf("invalid client %q: no materializer configured", ct), + http.StatusBadRequest, + ) + } + if s.clientManager != nil && !s.clientManager.SupportsPlugins(client.ClientApp(ct)) { + return nil, httperr.WithCode( + fmt.Errorf("invalid client %q: %w", ct, client.ErrPluginsNotSupported), + http.StatusBadRequest, + ) + } + } + return requested, nil +} + // entryMatchesInstalled reports whether the installed plugin is lock-managed, // its pinned digest still matches the lock entry, every expected client is -// present, every checked client directory's on-disk contentDigest matches, +// present, every expected client directory's on-disk contentDigest matches, // and each adapter reports the plugin as healthy. With no --clients override, // expected clients are every detected plugin-supporting client so a newly // installed client is not treated as current. Shared registration files are @@ -174,7 +226,7 @@ func (s *service) entryMatchesInstalled( ctx context.Context, entry lockfile.Entry, pl plugins.InstalledPlugin, - requestedClients []string, + expected []string, ) bool { if !pl.Managed { return false @@ -185,14 +237,10 @@ func (s *service) entryMatchesInstalled( if len(pl.Clients) == 0 { return false } - expected := requestedClients - if len(expected) == 0 { - expected = s.availableMaterializerClients() - } if len(expected) == 0 || !clientsContainAll(pl.Clients, expected) { return false } - for _, clientType := range mergeClientLists(pl.Clients, expected) { + for _, clientType := range expected { dir, err := s.pluginInstallPath(clientType, pl.Metadata.Name, pl.Scope, pl.ProjectRoot) if err != nil { return false @@ -217,24 +265,26 @@ func (s *service) entryMatchesInstalled( } // reinstallPinned reinstalls entry at its pinned reference, preserving its -// recorded Source (never re-resolving). Empty opts.Clients keeps Install's -// all-detected default so a newly detected client is materialized. +// recorded Source (never re-resolving). targetClients is the resolved sync +// client set (empty opts.Clients → detected clients). Callers must hold the +// per-plugin lock. func (s *service) reinstallPinned( - ctx context.Context, opts plugins.SyncOptions, entry lockfile.Entry, + ctx context.Context, opts plugins.SyncOptions, entry lockfile.Entry, targetClients []string, ) error { pinnedRef, err := buildPinnedReference(entry) if err != nil { return fmt.Errorf("pinning %q: %w", entry.Name, err) } - _, err = s.Install(ctx, plugins.InstallOptions{ + _, err = s.installAlreadyLocked(ctx, plugins.InstallOptions{ Name: pinnedRef, Scope: plugins.ScopeProject, ProjectRoot: opts.ProjectRoot, - Clients: opts.Clients, + Clients: targetClients, Force: true, // sync restores exactly the pinned content over any drifted files LockSource: entry.Source, LockResolvedReference: entry.ResolvedReference, // preserve — pinnedRef is a restore form SyncRestore: true, // reinstall despite unchanged Digest — drift is on disk, not the pin + ExpectedCanonicalName: entry.Name, }) return err } @@ -248,7 +298,7 @@ func (s *service) syncUnlockedInstall( if !pl.Managed { result.NeverManaged = append(result.NeverManaged, pl.Metadata.Name) if opts.Adopt && !opts.Check { - if err := s.adoptPlugin(ctx, pl); err != nil { + if err := s.adoptLocked(ctx, pl); err != nil { result.Failed = append(result.Failed, plugins.SyncFailure{ Name: pl.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -259,9 +309,9 @@ func (s *service) syncUnlockedInstall( result.RemovedFromLock = append(result.RemovedFromLock, pl.Metadata.Name) if opts.Prune && !opts.Check { - if err := s.Uninstall(ctx, plugins.UninstallOptions{ + if err := s.uninstallLocked(ctx, plugins.UninstallOptions{ Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: opts.ProjectRoot, - }); err != nil { + }, plugins.ScopeProject); err != nil { result.Failed = append(result.Failed, plugins.SyncFailure{ Name: pl.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), }) @@ -284,9 +334,13 @@ func (s *service) syncUnlockedInstall( // make every adopt fail until then. Lock validation permits an entry with // neither provenance nor unsigned. func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) error { - ctx, unlock := s.lockPlugin(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) + _, unlock := s.lockPlugin(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) defer unlock() + return s.adoptLocked(ctx, pl) +} +// adoptLocked performs adoptPlugin assuming the per-plugin lock is already held. +func (s *service) adoptLocked(ctx context.Context, pl plugins.InstalledPlugin) error { current, err := s.store.Get(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) if err != nil { return fmt.Errorf("re-reading plugin before adopt: %w", err) diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 501c84aea7..2835d41f19 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -65,10 +65,13 @@ func newGitLockTestService(t *testing.T, repoDir string) (plugins.PluginService, base: filepath.Join(projectRoot, ".claude", "plugins"), installer: skills.NewInstaller(), } + home := t.TempDir() + // Claude Code RelPath is empty; IsClientInstalled checks ~/.claude.json. + require.NoError(t, os.WriteFile(filepath.Join(home, ".claude.json"), []byte("{}"), 0o644)) svc := New( WithStore(sqlite.NewPluginStore(db)), WithMaterializers(map[string]plugins.MaterializationAdapter{"claude-code": adapter}), - WithClientManager(client.NewTestClientManagerWithHome(t.TempDir())), + WithClientManager(client.NewTestClientManagerWithHome(home)), WithGitClient(&redirectGitClient{dir: repoDir, inner: git.NewDefaultGitClient()}), ) return svc, projectRoot @@ -387,12 +390,21 @@ func TestSync_RequestedClientIsNotAlreadyCurrent(t *testing.T) { svc, projectRoot := newLockTestService(t, true) installTestPlugin(t, svc, projectRoot, validLockDigest()) - result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ //nolint:forcetypeassert + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["codex"] = &extractingAdapter{ + base: filepath.Join(projectRoot, ".agents", "plugins", "toolhive"), + installer: skills.NewInstaller(), + } + // Codex is supported but not installed (no ~/.codex). Explicit --clients + // must still be allowed and must not report AlreadyCurrent. + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ ProjectRoot: projectRoot, Check: true, Clients: []string{"codex"}, }) require.NoError(t, err) assert.Equal(t, []string{"my-plugin"}, result.Drifted) assert.Empty(t, result.AlreadyCurrent, "a plugin current in one client must not skip a requested extra client") + assert.Empty(t, result.Failed) } //nolint:paralleltest // uses t.Setenv via newLockTestService @@ -405,6 +417,7 @@ func TestSync_DefaultExpandsToNewlyDetectedClients(t *testing.T) { base: filepath.Join(projectRoot, ".agents", "plugins", "toolhive"), installer: skills.NewInstaller(), } + require.NoError(t, os.MkdirAll(filepath.Join(inner.clientManager.HomeDir(), ".codex"), 0o755)) result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) require.NoError(t, err) @@ -412,6 +425,76 @@ func TestSync_DefaultExpandsToNewlyDetectedClients(t *testing.T) { assert.Empty(t, result.AlreadyCurrent, "default sync must not treat a missing detected client as current") } +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_DefaultIgnoresSupportedButAbsentClients(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + inner := svc.(*service) //nolint:forcetypeassert + inner.materializers["codex"] = &extractingAdapter{ + base: filepath.Join(projectRoot, ".agents", "plugins", "toolhive"), + installer: skills.NewInstaller(), + } + // No ~/.codex — Codex supports plugins but is not installed, so default + // sync must not require it. + + result, err := inner.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.AlreadyCurrent) + assert.Empty(t, result.Drifted) +} + +//nolint:paralleltest // uses t.Setenv via newGitLockTestService +func TestSync_CanonicalNameMismatchFailsBeforeMutate(t *testing.T) { + repoDir := createPluginTestRepo(t, "") + svc, projectRoot := newGitLockTestService(t, repoDir) + + _, err := svc.Install(t.Context(), plugins.InstallOptions{ + Name: gitPluginRef, Scope: plugins.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before := readLockfile(t, projectRoot) + entry, ok := before.GetPlugin("my-plugin") + require.True(t, ok) + require.NoError(t, lockfile.RemovePluginEntry(mustOpenRoot(t, projectRoot), "my-plugin")) + + mismatched := entry + mismatched.Name = "other-plugin" + require.NoError(t, lockfile.UpsertPluginEntry(mustOpenRoot(t, projectRoot), mismatched)) + + syncSvc := svc.(*service) //nolint:forcetypeassert + beforeDB, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + beforeDigest := beforeDB.Digest + path := filepath.Join(pluginOnDiskPath(projectRoot, "my-plugin"), "commands", "hello.md") + beforeBytes, err := os.ReadFile(path) //nolint:gosec // fixed test path + require.NoError(t, err) + + result, err := syncSvc.Sync(t.Context(), plugins.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + assert.Equal(t, "other-plugin", result.Failed[0].Name) + assert.Equal(t, plugins.FailureReasonValidationRejected, result.Failed[0].Reason) + assert.Contains(t, result.Failed[0].Error, "does not match lock entry name") + + after, ok := readLockfile(t, projectRoot).GetPlugin("other-plugin") + require.True(t, ok) + assert.Equal(t, mismatched.Digest, after.Digest) + assert.Equal(t, mismatched.ContentDigest, after.ContentDigest) + + _, err = syncSvc.store.Get(t.Context(), "other-plugin", plugins.ScopeProject, projectRoot) + require.Error(t, err, "canonical mismatch must not create a DB row under the lock name") + + still, err := syncSvc.store.Get(t.Context(), "my-plugin", plugins.ScopeProject, projectRoot) + require.NoError(t, err) + assert.Equal(t, beforeDigest, still.Digest) + + afterBytes, err := os.ReadFile(path) //nolint:gosec // fixed test path + require.NoError(t, err) + assert.Equal(t, beforeBytes, afterBytes, "on-disk tree must be unchanged after rejected sync") +} + type staleListStore struct { storage.PluginStore listed []plugins.InstalledPlugin diff --git a/pkg/plugins/pluginsvc/uninstall.go b/pkg/plugins/pluginsvc/uninstall.go index a0f4e13e53..e6893d81e4 100644 --- a/pkg/plugins/pluginsvc/uninstall.go +++ b/pkg/plugins/pluginsvc/uninstall.go @@ -41,9 +41,17 @@ func (s *service) Uninstall(ctx context.Context, opts plugins.UninstallOptions) scope = defaultScope(scope) opts.ProjectRoot = projectRoot - ctx, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) + _, unlock := s.lockPlugin(ctx, opts.Name, scope, opts.ProjectRoot) defer unlock() + return s.uninstallLocked(ctx, opts, scope) +} + +// uninstallLocked performs Uninstall assuming the per-plugin lock is already +// held (e.g. by Sync prune). opts must already be normalized. +func (s *service) uninstallLocked( + ctx context.Context, opts plugins.UninstallOptions, scope plugins.Scope, +) error { existing, err := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) if err != nil { // Idempotent: a missing record is not an error. From 094f91877a733ee96db4a6eb43f41cd21b5e1914 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 17 Aug 2026 19:29:57 +0200 Subject: [PATCH 7/9] Drop unused adoptPlugin wrapper after locked sync Sync already calls adoptLocked under the held mutex; remove the dead public wrapper and quiet installFromOCI gocyclo. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/install_oci.go | 2 ++ pkg/plugins/pluginsvc/sync.go | 9 ++------- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/pkg/plugins/pluginsvc/install_oci.go b/pkg/plugins/pluginsvc/install_oci.go index 396218c8f4..6a4c549bda 100644 --- a/pkg/plugins/pluginsvc/install_oci.go +++ b/pkg/plugins/pluginsvc/install_oci.go @@ -24,6 +24,8 @@ import ( // (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. +// +//nolint:gocyclo // pull, validate, hydrate, and locked install are one transactional path func (s *service) installFromOCI( ctx context.Context, opts plugins.InstallOptions, diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 255b980e13..7e13719580 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -333,13 +333,8 @@ func (s *service) syncUnlockedInstall( // verification lands in a later PR; requiring --allow-unsigned here would // make every adopt fail until then. Lock validation permits an entry with // neither provenance nor unsigned. -func (s *service) adoptPlugin(ctx context.Context, pl plugins.InstalledPlugin) error { - _, unlock := s.lockPlugin(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) - defer unlock() - return s.adoptLocked(ctx, pl) -} - -// adoptLocked performs adoptPlugin assuming the per-plugin lock is already held. +// adoptLocked writes a lock entry for an unmanaged install assuming the +// per-plugin lock is already held. func (s *service) adoptLocked(ctx context.Context, pl plugins.InstalledPlugin) error { current, err := s.store.Get(ctx, pl.Metadata.Name, plugins.ScopeProject, pl.ProjectRoot) if err != nil { From 25ce93b980a5e9da320d4bc504781ba7a7cc4f98 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 18 Aug 2026 10:04:41 +0200 Subject: [PATCH 8/9] Expand sole clients all sentinel on plugin sync A lone --clients all now matches the documented detected-client default instead of failing validation on every sync. Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/sync.go | 12 +++++++----- pkg/plugins/pluginsvc/sync_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/pkg/plugins/pluginsvc/sync.go b/pkg/plugins/pluginsvc/sync.go index 7e13719580..ba85fdf481 100644 --- a/pkg/plugins/pluginsvc/sync.go +++ b/pkg/plugins/pluginsvc/sync.go @@ -175,12 +175,14 @@ func (s *service) syncLockedEntry( } // resolveSyncTargetClients returns the client set Sync should check and -// restore. Empty requested expands to availableMaterializerClients (detected -// + plugin-supporting). Explicit names are validated like Install (materializer -// + SupportsPlugins) but do not require IsClientInstalled — the caller asked -// for them. +// restore. Empty requested — or the sole sentinel value "all", matching +// Install and the CLI help text — expands to availableMaterializerClients +// (detected + plugin-supporting). Explicit names are validated like Install +// (materializer + SupportsPlugins) but do not require IsClientInstalled — +// the caller asked for them. func (s *service) resolveSyncTargetClients(requested []string) ([]string, error) { - if len(requested) == 0 { + if len(requested) == 0 || + (len(requested) == 1 && strings.EqualFold(requested[0], clientsAllSentinel)) { return s.availableMaterializerClients(), nil } for _, c := range requested { diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 2835d41f19..33668b2ce0 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -101,6 +101,36 @@ func TestSync_ReportsUpToDateWhenNothingChanged(t *testing.T) { assert.Empty(t, result.Failed) } +// The documented `--clients all` mode must behave exactly like the empty +// default: expand to every detected plugin-supporting client, not fail +// validation. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_ClientsAllSentinelMatchesDefault(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Clients: []string{"All"}, + }) + require.NoError(t, err) + assert.Equal(t, []string{"my-plugin"}, result.AlreadyCurrent) + assert.Empty(t, result.Failed, "--clients all must not produce a validation failure") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService +func TestSync_ClientsAllSentinelRejectsCombination(t *testing.T) { + svc, projectRoot := newLockTestService(t, true) + installTestPlugin(t, svc, projectRoot, validLockDigest()) + + result, err := svc.(*service).Sync(t.Context(), plugins.SyncOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Clients: []string{"all", "claude-code"}, + }) + require.NoError(t, err) + require.Len(t, result.Failed, 1) + assert.Contains(t, result.Failed[0].Error, "cannot be combined") +} + //nolint:paralleltest // uses t.Setenv via newLockTestService func TestSync_CheckReportsDriftWithoutWriting(t *testing.T) { svc, projectRoot := newLockTestService(t, true) From a649e0abf8b375f3ce00f2d0d1e488e86b4135d9 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Wed, 19 Aug 2026 15:35:30 +0200 Subject: [PATCH 9/9] Match unified update hook signature in adopt test Signed-off-by: Samuele Verzi --- pkg/plugins/pluginsvc/sync_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/plugins/pluginsvc/sync_test.go b/pkg/plugins/pluginsvc/sync_test.go index 33668b2ce0..0b660d3781 100644 --- a/pkg/plugins/pluginsvc/sync_test.go +++ b/pkg/plugins/pluginsvc/sync_test.go @@ -373,7 +373,7 @@ func TestSync_AdoptUpdateFailureRestoresExistingLockEntry(t *testing.T) { syncSvc.store = &hookPluginStore{ PluginStore: syncSvc.store, - beforeUpdate: func(p plugins.InstalledPlugin) error { + beforeUpdate: func(_ int, p plugins.InstalledPlugin) error { if p.Managed { return errors.New("db locked") }