diff --git a/cmd/thv/app/skill_helpers.go b/cmd/thv/app/skill_helpers.go index 06f0bdc6f4..071917704f 100644 --- a/cmd/thv/app/skill_helpers.go +++ b/cmd/thv/app/skill_helpers.go @@ -10,6 +10,7 @@ import ( "github.com/spf13/cobra" + tclient "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/skills" skillclient "github.com/stacklok/toolhive/pkg/skills/client" ) @@ -65,3 +66,18 @@ func validateProjectRootForScope(scopeVar, projectRootVar *string) func(*cobra.C return nil } } + +// resolveProjectRoot returns explicit if set, otherwise auto-detects the +// project root by walking up from the current directory looking for .git — +// used by commands (sync, upgrade) that operate on "the project you're in" +// rather than requiring --project-root on every invocation. +func resolveProjectRoot(explicit string) (string, error) { + if explicit != "" { + return explicit, nil + } + root, err := tclient.DetectProjectRoot("") + if err != nil { + return "", fmt.Errorf("detecting project root: %w (use --project-root to specify it explicitly)", err) + } + return root, nil +} diff --git a/cmd/thv/app/skill_sync.go b/cmd/thv/app/skill_sync.go new file mode 100644 index 0000000000..300b2634ea --- /dev/null +++ b/cmd/thv/app/skill_sync.go @@ -0,0 +1,124 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "encoding/json" + "fmt" + + "github.com/spf13/cobra" + + "github.com/stacklok/toolhive/pkg/skills" +) + +var ( + skillSyncProjectRoot string + skillSyncClientsRaw string + skillSyncCheck bool + skillSyncAdopt bool + skillSyncPrune bool + skillSyncFormat string +) + +var skillSyncCmd = &cobra.Command{ + Use: "sync", + Short: "Restore project skills to match the lock file", + Long: `Restore a project's installed skills to match toolhive.lock.yaml. + +Missing or drifted skills 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.`, + PreRunE: chainPreRunE( + ValidateFormat(&skillSyncFormat), + ), + RunE: skillSyncCmdFunc, +} + +func init() { + skillCmd.AddCommand(skillSyncCmd) + + skillSyncCmd.Flags().StringVar(&skillSyncProjectRoot, "project-root", "", + "Project root path (default: auto-detected from the current directory)") + skillSyncCmd.Flags().StringVar(&skillSyncClientsRaw, "clients", "", + `Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client`) + skillSyncCmd.Flags().BoolVar(&skillSyncCheck, "check", false, + "Report drift without installing, writing, or removing anything") + skillSyncCmd.Flags().BoolVar(&skillSyncAdopt, "adopt", false, + "Write lock entries for existing unmanaged project-scope installs") + skillSyncCmd.Flags().BoolVar(&skillSyncPrune, "prune", false, + "Remove installs no longer present in the lock file") + AddFormatFlag(skillSyncCmd, &skillSyncFormat) +} + +func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { + projectRoot, err := resolveProjectRoot(skillSyncProjectRoot) + if err != nil { + return err + } + + c := newSkillClient(cmd.Context()) + result, err := c.Sync(cmd.Context(), skills.SyncOptions{ + ProjectRoot: projectRoot, + Clients: parseSkillInstallClients(skillSyncClientsRaw), + Check: skillSyncCheck, + Adopt: skillSyncAdopt, + Prune: skillSyncPrune, + }) + if err != nil { + return formatSkillError("sync skills", err) + } + + return printSyncResult(result, skillSyncFormat) +} + +func printSyncResult(result *skills.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 printSkillNameGroup(label string, names []string) { + if len(names) == 0 { + return + } + fmt.Printf("%s:\n", label) + for _, name := range names { + fmt.Printf(" %s\n", name) + } +} + +func isSyncResultEmpty(result *skills.SyncResult) bool { + return len(result.Installed) == 0 && + len(result.Drifted) == 0 && + len(result.Missing) == 0 && + len(result.AlreadyCurrent) == 0 && + len(result.NeverManaged) == 0 && + len(result.RemovedFromLock) == 0 && + len(result.Pruned) == 0 && + len(result.Failed) == 0 +} diff --git a/docs/cli/thv_skill.md b/docs/cli/thv_skill.md index 3a93a62f77..9466733538 100644 --- a/docs/cli/thv_skill.md +++ b/docs/cli/thv_skill.md @@ -38,6 +38,7 @@ The skill command provides subcommands to manage skills. * [thv skill install](thv_skill_install.md) - Install a skill * [thv skill list](thv_skill_list.md) - List installed skills * [thv skill push](thv_skill_push.md) - Push a built skill +* [thv skill sync](thv_skill_sync.md) - Restore project skills to match the lock file * [thv skill uninstall](thv_skill_uninstall.md) - Uninstall a skill * [thv skill validate](thv_skill_validate.md) - Validate a skill definition diff --git a/docs/cli/thv_skill_sync.md b/docs/cli/thv_skill_sync.md new file mode 100644 index 0000000000..e4bef4a9d9 --- /dev/null +++ b/docs/cli/thv_skill_sync.md @@ -0,0 +1,50 @@ +--- +title: thv skill sync +hide_title: true +description: Reference for ToolHive CLI command `thv skill sync` +last_update: + author: autogenerated +slug: thv_skill_sync +mdx: + format: md +--- + +## thv skill sync + +Restore project skills to match the lock file + +### Synopsis + +Restore a project's installed skills to match toolhive.lock.yaml. + +Missing or drifted skills 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. + +``` +thv skill 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 +``` + +### Options inherited from parent commands + +``` + --debug Enable debug mode +``` + +### SEE ALSO + +* [thv skill](thv_skill.md) - Manage skills + diff --git a/docs/server/docs.go b/docs/server/docs.go index 24270dbbea..7bedb9c7ca 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1650,6 +1650,24 @@ const docTemplate = `{ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.FailureReason": { + "description": "Reason is a typed failure reason for CI and automation.", + "enum": [ + "registry-unreachable", + "digest-missing", + "validation-rejected", + "lock-write-failed", + "unknown" + ], + "type": "string", + "x-enum-varnames": [ + "FailureReasonRegistryUnreachable", + "FailureReasonDigestMissing", + "FailureReasonValidationRejected", + "FailureReasonLockWriteFailed", + "FailureReasonUnknown" + ] + }, "github_com_stacklok_toolhive_pkg_skills.InstallStatus": { "description": "Status is the current installation status.", "enum": [ @@ -1843,6 +1861,91 @@ const docTemplate = `{ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.SyncFailure": { + "properties": { + "error": { + "description": "Error is a human-readable description of the failure.", + "type": "string" + }, + "name": { + "description": "Name is the skill name that failed.", + "type": "string" + }, + "reason": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason" + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.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_skills.ValidationResult": { "properties": { "errors": { @@ -3440,6 +3543,36 @@ const docTemplate = `{ }, "type": "object" }, + "pkg_api_v1.syncSkillsRequest": { + "description": "Request to restore a project's installed skills 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\nskill-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 skills installed but not present in the lock file", + "type": "boolean" + } + }, + "type": "object" + }, "pkg_api_v1.toolOverride": { "description": "Tool override", "properties": { @@ -6248,6 +6381,87 @@ const docTemplate = `{ ] } }, + "/api/v1beta/skills/sync": { + "post": { + "description": "Restore a project's installed skills to match toolhive.lock.yaml", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.syncSkillsRequest", + "summary": "request", + "description": "Sync request" + } + ] + } + } + }, + "description": "Sync request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.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 skills from the lock file", + "tags": [ + "skills" + ] + } + }, "/api/v1beta/skills/validate": { "post": { "description": "Validate a skill definition", diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 322fbb8507..f6b04a0b23 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1643,6 +1643,24 @@ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.FailureReason": { + "description": "Reason is a typed failure reason for CI and automation.", + "enum": [ + "registry-unreachable", + "digest-missing", + "validation-rejected", + "lock-write-failed", + "unknown" + ], + "type": "string", + "x-enum-varnames": [ + "FailureReasonRegistryUnreachable", + "FailureReasonDigestMissing", + "FailureReasonValidationRejected", + "FailureReasonLockWriteFailed", + "FailureReasonUnknown" + ] + }, "github_com_stacklok_toolhive_pkg_skills.InstallStatus": { "description": "Status is the current installation status.", "enum": [ @@ -1836,6 +1854,91 @@ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.SyncFailure": { + "properties": { + "error": { + "description": "Error is a human-readable description of the failure.", + "type": "string" + }, + "name": { + "description": "Name is the skill name that failed.", + "type": "string" + }, + "reason": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason" + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.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_skills.ValidationResult": { "properties": { "errors": { @@ -3433,6 +3536,36 @@ }, "type": "object" }, + "pkg_api_v1.syncSkillsRequest": { + "description": "Request to restore a project's installed skills 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\nskill-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 skills installed but not present in the lock file", + "type": "boolean" + } + }, + "type": "object" + }, "pkg_api_v1.toolOverride": { "description": "Tool override", "properties": { @@ -6241,6 +6374,87 @@ ] } }, + "/api/v1beta/skills/sync": { + "post": { + "description": "Restore a project's installed skills to match toolhive.lock.yaml", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.syncSkillsRequest", + "summary": "request", + "description": "Sync request" + } + ] + } + } + }, + "description": "Sync request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.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 skills from the lock file", + "tags": [ + "skills" + ] + } + }, "/api/v1beta/skills/validate": { "post": { "description": "Validate a skill definition", diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 39a14d1aab..ab09c2f486 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1698,6 +1698,21 @@ components: description: Reference is the OCI reference for the dependency. type: string type: object + github_com_stacklok_toolhive_pkg_skills.FailureReason: + description: Reason is a typed failure reason for CI and automation. + enum: + - registry-unreachable + - digest-missing + - validation-rejected + - lock-write-failed + - unknown + type: string + x-enum-varnames: + - FailureReasonRegistryUnreachable + - FailureReasonDigestMissing + - FailureReasonValidationRejected + - FailureReasonLockWriteFailed + - FailureReasonUnknown github_com_stacklok_toolhive_pkg_skills.InstallStatus: description: Status is the current installation status. enum: @@ -1848,6 +1863,81 @@ components: description: Version is the semantic version of the skill. type: string type: object + github_com_stacklok_toolhive_pkg_skills.SyncFailure: + properties: + error: + description: Error is a human-readable description of the failure. + type: string + name: + description: Name is the skill name that failed. + type: string + reason: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason' + type: object + github_com_stacklok_toolhive_pkg_skills.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_skills.ValidationResult: properties: errors: @@ -3152,6 +3242,35 @@ components: type: array uniqueItems: false type: object + pkg_api_v1.syncSkillsRequest: + description: Request to restore a project's installed skills 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 + skill-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 skills installed but not present + in the lock file + type: boolean + type: object pkg_api_v1.toolOverride: description: Tool override properties: @@ -5212,6 +5331,54 @@ paths: summary: Push a skill tags: - skills + /api/v1beta/skills/sync: + post: + description: Restore a project's installed skills to match toolhive.lock.yaml + requestBody: + content: + application/json: + schema: + oneOf: + - type: object + - $ref: '#/components/schemas/pkg_api_v1.syncSkillsRequest' + description: Sync request + summary: request + description: Sync request + required: true + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.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 skills from the lock file + tags: + - skills /api/v1beta/skills/validate: post: description: Validate a skill definition diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index b0f58c9001..ac932651a9 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -4,7 +4,9 @@ package v1 import ( + "context" "encoding/json" + "errors" "fmt" "net/http" @@ -15,16 +17,30 @@ import ( "github.com/stacklok/toolhive/pkg/skills" ) +// skillSyncer is the narrow slice of skills.SkillLockService the sync +// endpoint needs. Kept separate from the full interface so this PR does not +// need to ship an Upgrade stub; SkillsRouter widens to skills.SkillLockService +// once Upgrade lands alongside its own endpoint. +type skillSyncer interface { + Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) +} + // SkillsRoutes defines the routes for skill management. type SkillsRoutes struct { skillService skills.SkillService + lockService skillSyncer } -// SkillsRouter creates a new router for skill management endpoints. +// SkillsRouter creates a new router for skill management endpoints. If +// skillService's concrete implementation also satisfies skillSyncer (as +// skillsvc.New's does), /sync is served; otherwise it returns 501. func SkillsRouter(skillService skills.SkillService) http.Handler { routes := SkillsRoutes{ skillService: skillService, } + if syncer, ok := skillService.(skillSyncer); ok { + routes.lockService = syncer + } r := chi.NewRouter() r.Get("/", apierrors.ErrorHandler(routes.listSkills)) @@ -37,6 +53,7 @@ func SkillsRouter(skillService skills.SkillService) http.Handler { r.Get("/builds", apierrors.ErrorHandler(routes.listBuilds)) r.Delete("/builds/{tag}", apierrors.ErrorHandler(routes.deleteBuild)) r.Get("/content", apierrors.ErrorHandler(routes.getSkillContent)) + r.Post("/sync", apierrors.ErrorHandler(routes.syncSkills)) return r } @@ -365,3 +382,45 @@ func (s *SkillsRoutes) getSkillContent(w http.ResponseWriter, r *http.Request) e w.Header().Set("Content-Type", "application/json") return json.NewEncoder(w).Encode(content) } + +// syncSkills restores a project's installed skills to match its lock file. +// +// @Summary Sync project skills from the lock file +// @Description Restore a project's installed skills to match toolhive.lock.yaml +// @Tags skills +// @Accept json +// @Produce json +// @Param request body syncSkillsRequest true "Sync request" +// @Success 200 {object} skills.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/skills/sync [post] +func (s *SkillsRoutes) syncSkills(w http.ResponseWriter, r *http.Request) error { + if s.lockService == nil { + return httperr.WithCode(errors.New("skill sync is not supported by this server"), http.StatusNotImplemented) + } + + var req syncSkillsRequest + 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(), skills.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/skills_sync_test.go b/pkg/api/v1/skills_sync_test.go new file mode 100644 index 0000000000..6f4e04ff4b --- /dev/null +++ b/pkg/api/v1/skills_sync_test.go @@ -0,0 +1,126 @@ +// 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/skills" + skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" +) + +// skillServiceWithSync wraps a mocked SkillService and adds a Sync method, so +// SkillsRouter's opportunistic skillSyncer type assertion succeeds — the same +// shape skillsvc.New's concrete service has once it implements both. +type skillServiceWithSync struct { + skills.SkillService + syncFn func(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) +} + +func (s *skillServiceWithSync) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { + return s.syncFn(ctx, opts) +} + +func TestSyncSkillsEndpoint(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + service skills.SkillService + body string + wantStatus int + wantContains string + }{ + { + name: "successful sync returns 200 with result", + service: &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + syncFn: func(_ context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { + assert.Equal(t, "/tmp/proj", opts.ProjectRoot) + assert.True(t, opts.Check) + return &skills.SyncResult{AlreadyCurrent: []string{"my-skill"}}, nil + }, + }, + body: `{"project_root":"/tmp/proj","check":true}`, + wantStatus: http.StatusOK, + wantContains: `"my-skill"`, + }, + { + name: "service without Sync support returns 501", + service: skillsmocks.NewMockSkillService(gomock.NewController(t)), + body: `{"project_root":"/tmp/proj"}`, + wantStatus: http.StatusNotImplemented, + }, + { + name: "invalid JSON body returns 400", + service: &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + syncFn: func(context.Context, skills.SyncOptions) (*skills.SyncResult, error) { + t.Fatal("Sync must not be called for an invalid body") + return nil, nil + }, + }, + body: `not json`, + wantStatus: http.StatusBadRequest, + }, + { + name: "sync error propagates with its status code", + service: &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + syncFn: func(context.Context, skills.SyncOptions) (*skills.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() + + router := SkillsRouter(tt.service) + req := httptest.NewRequest(http.MethodPost, "/sync", bytes.NewBufferString(tt.body)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assert.Equal(t, tt.wantStatus, rec.Code) + if tt.wantContains != "" { + assert.Contains(t, rec.Body.String(), tt.wantContains) + } + }) + } +} + +func TestSyncSkillsResponseIsValidJSON(t *testing.T) { + t.Parallel() + + svc := &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + syncFn: func(context.Context, skills.SyncOptions) (*skills.SyncResult, error) { + return &skills.SyncResult{Installed: []string{"a"}, Drifted: []string{"b"}}, nil + }, + } + router := SkillsRouter(svc) + req := httptest.NewRequest(http.MethodPost, "/sync", bytes.NewBufferString(`{"project_root":"/tmp/proj"}`)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var result skills.SyncResult + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &result)) + assert.Equal(t, []string{"a"}, result.Installed) + assert.Equal(t, []string{"b"}, result.Drifted) +} diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 833a9b0aa5..9514c9a702 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -69,6 +69,23 @@ type pushSkillRequest struct { Reference string `json:"reference"` } +// syncSkillsRequest represents the request to sync a project's skills. +// +// @Description Request to restore a project's installed skills to match its lock file +type syncSkillsRequest 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 + // skill-supporting client detected on this host. + Clients []string `json:"clients,omitempty"` + // Prune removes project-scoped skills 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"` +} + // buildListResponse represents the response for listing locally-built OCI skill artifacts. // // @Description Response containing a list of locally-built OCI skill artifacts diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index 4c5c69a606..191b910aab 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -267,6 +267,23 @@ func (c *Client) GetContent(ctx context.Context, opts skills.ContentOptions) (*s return &content, nil } +// Sync restores a project's installed skills to match its lock file. +func (c *Client) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { + body := syncRequest{ + ProjectRoot: opts.ProjectRoot, + Clients: opts.Clients, + Prune: opts.Prune, + Check: opts.Check, + Adopt: opts.Adopt, + } + + var result skills.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/skills/client/dto.go b/pkg/skills/client/dto.go index 104f7e2fad..74b480d6eb 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -41,3 +41,11 @@ type installResponse struct { type listBuildsResponse struct { Builds []skills.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/skills/options.go b/pkg/skills/options.go index d228fec6dc..5f58e533fd 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -43,6 +43,15 @@ type InstallOptions struct { // Name that must not overwrite the entry's original Source. Internal use // only — NOT exposed via HTTP API. LockSource string `json:"-"` + // LockResolvedReference overrides the value recorded as the lock entry's + // ResolvedReference. When empty, the entry's ResolvedReference is + // whatever this install actually resolved to. Set by Sync when + // reinstalling at a pinned reference: without this override, a drift + // repair would overwrite ResolvedReference with the internal pinned + // form (e.g. a digest or commit hash spliced into the reference) + // instead of preserving what Source originally resolved to. Internal + // use only — NOT exposed via HTTP API. + LockResolvedReference string `json:"-"` // RequiredByParent is set when this install is a transitively materialized // dependency (toolhive.requires) of another skill, naming that parent. // Empty means the user explicitly requested this install. Internal use @@ -54,6 +63,13 @@ type InstallOptions struct { // through recursive dependency installs. Internal use only — NOT exposed // via HTTP API. Visited map[string]struct{} `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. @@ -215,6 +231,11 @@ type SyncResult struct { // file. Normally these are reinstalled to match it; when Check is set, // nothing is written and this field reports the drift only. Drifted []string `json:"drifted,omitempty"` + // 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. + Missing []string `json:"missing,omitempty"` // AlreadyCurrent lists skills that already matched the lock file. AlreadyCurrent []string `json:"already_current,omitempty"` // NeverManaged lists project-scoped skills never recorded as lock-managed. diff --git a/pkg/skills/skillsvc/content_digest.go b/pkg/skills/skillsvc/content_digest.go index 6f9364cf49..839b7552be 100644 --- a/pkg/skills/skillsvc/content_digest.go +++ b/pkg/skills/skillsvc/content_digest.go @@ -16,9 +16,11 @@ import ( const skillMDFileName = "SKILL.md" // computeContentDigest hashes the on-disk file set for an installed skill, -// for lock file integrity verification. Every client directory a skill is -// installed into is written from the same source, so any one of them is -// representative; the first client in the skill's Clients list is used. +// for recording in the lock file at install time. Every client directory is +// written from the same source in the same operation, so the first client's +// copy is representative *at record time*; verification (sync --check) must +// not make that assumption and checks every client directory — see +// entryMatchesInstalled. func computeContentDigest(pathResolver skills.PathResolver, sk skills.InstalledSkill) (string, error) { dir, err := installedSkillDir(pathResolver, sk) if err != nil { diff --git a/pkg/skills/skillsvc/install_extraction.go b/pkg/skills/skillsvc/install_extraction.go index 4a9f680c10..7093b08baa 100644 --- a/pkg/skills/skillsvc/install_extraction.go +++ b/pkg/skills/skillsvc/install_extraction.go @@ -54,12 +54,12 @@ func (s *service) dispatchExtraction( clientTypes []string, clientDirs map[string]string, ) (*skills.InstallResult, error) { - if isExtractionNoOp(existing, storeErr, opts, clientTypes) { + if !opts.SyncRestore && isExtractionNoOp(existing, storeErr, opts, clientTypes) { return &skills.InstallResult{Skill: existing}, nil } digestMatches := storeErr == nil && existing.Digest == opts.Digest - if digestMatches && storeErr == nil { + if digestMatches && storeErr == nil && !opts.SyncRestore { return s.installExtractionSameDigestNewClients(ctx, opts, scope, existing, clientTypes, clientDirs) } diff --git a/pkg/skills/skillsvc/install_git.go b/pkg/skills/skillsvc/install_git.go index 36b8c84c52..4c64d56e0c 100644 --- a/pkg/skills/skillsvc/install_git.go +++ b/pkg/skills/skillsvc/install_git.go @@ -124,7 +124,11 @@ func (s *service) applyGitInstallExisting( clientDirs map[string]string, files []gitresolver.FileEntry, ) (*skills.InstallResult, error) { - if existing.Digest != opts.Digest { + // SyncRestore forces the same full re-extraction path as a digest + // change: sync repairs on-disk drift that happened without the pinned + // digest changing, so the "same digest means content is already + // correct" no-op/skip branches below must not apply. + if existing.Digest != opts.Digest || opts.SyncRestore { allClients, allDirs, err := s.expandToExistingClients( existing.Clients, clientTypes, clientDirs, opts.Name, scope, opts.ProjectRoot) if err != nil { diff --git a/pkg/skills/skillsvc/lock.go b/pkg/skills/skillsvc/lock.go index ec9aa4d568..b589dd4c75 100644 --- a/pkg/skills/skillsvc/lock.go +++ b/pkg/skills/skillsvc/lock.go @@ -5,6 +5,7 @@ package skillsvc import ( "context" + "errors" "fmt" "slices" @@ -12,6 +13,11 @@ import ( "github.com/stacklok/toolhive/pkg/skills/lockfile" ) +// errLockWrite marks failures to write the project lock file, so +// classifySyncFailure can map exactly these — and not every HTTP 500 — to +// the lock-write-failed reason automation keys on. +var errLockWrite = errors.New("lock file write failed") + // recordLockState updates opts.ProjectRoot's lock file to reflect a // just-completed project-scope install: an entry for sk, plus recursively // materialized entries for any toolhive.requires dependencies declared in @@ -34,16 +40,21 @@ func (s *service) recordLockState( if source == "" { source = originalName } + resolvedReference := opts.LockResolvedReference + if resolvedReference == "" { + resolvedReference = sk.Reference + } if err := recordLockEntry(sk.ProjectRoot, lockEntryInput{ Name: sk.Metadata.Name, Version: sk.Metadata.Version, Source: source, - ResolvedReference: sk.Reference, + ResolvedReference: resolvedReference, Digest: sk.Digest, ContentDigest: contentDigest, RequiredByParent: opts.RequiredByParent, + PreserveExplicit: opts.SyncRestore, }); err != nil { - return sk, fmt.Errorf("writing lock entry: %w", err) + return sk, fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) } if !sk.Managed { @@ -53,8 +64,15 @@ func (s *service) recordLockState( } } - if err := s.materializeDependencies(ctx, opts, source, sk); err != nil { - return sk, fmt.Errorf("materializing dependencies: %w", err) + // A sync restore reinstalls exactly what the lock file already pins: + // every dependency has its own entry that the sync loop restores + // directly, so re-materializing toolhive.requires here would re-resolve + // each dependency from its mutable source string — silently upgrading + // and re-pinning it, the opposite of a deterministic restore. + if !opts.SyncRestore { + if err := s.materializeDependencies(ctx, opts, source, sk); err != nil { + return sk, fmt.Errorf("materializing dependencies: %w", err) + } } return sk, nil } @@ -132,6 +150,12 @@ type lockEntryInput struct { // transitively materialized dependency. Empty means the entry is // explicit (a direct, user-requested install). RequiredByParent string + // PreserveExplicit keeps the existing entry's Explicit flag verbatim + // instead of deriving it from RequiredByParent. Set by sync restores: a + // restore is not a user install, so it must not promote a non-explicit + // dependency to explicit — that would permanently exempt it from + // cascade removal. + PreserveExplicit bool } // recordLockEntry upserts a single entry into projectRoot's lock file. When @@ -153,10 +177,14 @@ func recordLockEntry(projectRoot string, in lockEntryInput) error { ContentDigest: in.ContentDigest, Explicit: in.RequiredByParent == "", } - if existing, ok := lf.Get(in.Name); ok { + existing, exists := lf.Get(in.Name) + if exists { entry.RequiredBy = existing.RequiredBy entry.Explicit = entry.Explicit || existing.Explicit } + if in.PreserveExplicit { + entry.Explicit = exists && existing.Explicit + } if in.RequiredByParent != "" { entry.RequiredBy = appendUnique(entry.RequiredBy, in.RequiredByParent) } diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index 16b633f19c..355e1ae08b 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -49,11 +49,25 @@ func newLockTestService(t *testing.T, gr *gitmocks.MockResolver) (skills.SkillSe DoAndReturn(func(_, skillName string, _ skills.Scope, _ string) (string, error) { return filepath.Join(installBase, skillName), nil }) + // Only reached when a caller omits --clients with no prior DB record to + // fall back on (e.g. sync restoring an entry missing from a fresh + // clone) — the RFC's client-agnostic design expands that to every + // skill-supporting client detected on the host. + pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code"}) svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) return svc, projectRoot } +// gitSkillVersion builds SKILL.md content for a git-resolved test fixture +// with a distinct version ("2.0.0"), for tests simulating newer content +// published at the same source (e.g. an upgrade scenario). It never +// declares requires. +func gitSkillVersion(name string) []byte { + fm := fmt.Sprintf("---\nname: %s\ndescription: test skill\nversion: 2.0.0\n---\n# %s\n", name, name) + return []byte(fm) +} + // gitSkill builds SKILL.md content for a git-resolved test fixture, // optionally declaring toolhive.requires dependencies. func gitSkill(name string, requires ...string) []byte { @@ -92,14 +106,27 @@ type gitFixture struct { // fakeGitFixtures dispatches a MockResolver's Resolve calls by matching the // reference URL against a fixed set of registered skills, each getting its -// own commit hash so digests differ. +// own commit hash so digests differ. Like a real repository, it keeps every +// registered revision: resolving a reference pinned to a full commit hash +// returns that revision's content, while an unpinned reference returns the +// latest — so sync's pinned restores behave the way real git checkouts do. type fakeGitFixtures struct { - skills map[string]gitFixture // keyed by URL + skills map[string]gitFixture // latest content, keyed by URL + history map[string]map[string]gitFixture // URL -> commit hash -> content +} + +// fixtureCommitHash derives the deterministic per-revision commit hash the +// fake resolver reports for a registered fixture. +func fixtureCommitHash(url string, content []byte) string { + return fmt.Sprintf("%040x", len(content)+len(url)) } func newGitResolverMock(t *testing.T) (*gitmocks.MockResolver, *fakeGitFixtures) { t.Helper() - fx := &fakeGitFixtures{skills: make(map[string]gitFixture)} + fx := &fakeGitFixtures{ + skills: make(map[string]gitFixture), + history: make(map[string]map[string]gitFixture), + } gr := gitmocks.NewMockResolver(gomock.NewController(t)) gr.EXPECT().Resolve(gomock.Any(), gomock.Any()).AnyTimes(). DoAndReturn(func(_ context.Context, ref *gitresolver.GitReference) (*gitresolver.ResolveResult, error) { @@ -107,10 +134,13 @@ func newGitResolverMock(t *testing.T) (*gitmocks.MockResolver, *fakeGitFixtures) if !ok { return nil, fmt.Errorf("no fixture registered for %q", ref.URL) } + if pinned, isPinned := fx.history[ref.URL][ref.Ref]; isPinned { + fixture = pinned + } return &gitresolver.ResolveResult{ SkillConfig: &skills.ParseResult{Name: fixture.name}, Files: []gitresolver.FileEntry{{Path: "SKILL.md", Content: fixture.content, Mode: 0644}}, - CommitHash: fmt.Sprintf("%040x", len(fixture.content)+len(ref.URL)), // deterministic, distinct per fixture + CommitHash: fixtureCommitHash(ref.URL, fixture.content), }, nil }) return gr, fx @@ -118,14 +148,24 @@ func newGitResolverMock(t *testing.T) (*gitmocks.MockResolver, *fakeGitFixtures) func (f *fakeGitFixtures) register(name string, content []byte) { _, url := gitRef(name) - f.skills[url] = gitFixture{name: name, content: content} + fixture := gitFixture{name: name, content: content} + f.skills[url] = fixture + if f.history[url] == nil { + f.history[url] = make(map[string]gitFixture) + } + f.history[url][fixtureCommitHash(url, content)] = fixture } -func readLockfile(t *testing.T, projectRoot string) *lockfile.Lockfile { +func mustOpenRoot(t *testing.T, projectRoot string) lockfile.Root { t.Helper() root, err := lockfile.OpenRoot(projectRoot) require.NoError(t, err) - lf, err := lockfile.Load(root) + return root +} + +func readLockfile(t *testing.T, projectRoot string) *lockfile.Lockfile { + t.Helper() + lf, err := lockfile.Load(mustOpenRoot(t, projectRoot)) require.NoError(t, err) return lf } diff --git a/pkg/skills/skillsvc/pin.go b/pkg/skills/skillsvc/pin.go new file mode 100644 index 0000000000..b84d7dd4a3 --- /dev/null +++ b/pkg/skills/skillsvc/pin.go @@ -0,0 +1,75 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +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 +} + +func isFullCommitHash(ref string) bool { + if len(ref) != 40 { + return false + } + for _, c := range ref { + if (c < '0' || c > '9') && (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/skills/skillsvc/pin_test.go b/pkg/skills/skillsvc/pin_test.go new file mode 100644 index 0000000000..df1ad97910 --- /dev/null +++ b/pkg/skills/skillsvc/pin_test.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +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/skills@main#testing-conventions", + Digest: testCommitHash, + }, + want: "git://github.com/org/skills@" + testCommitHash + "#testing-conventions", + }, + { + name: "git reference without a subdir", + entry: lockfile.Entry{ + ResolvedReference: "git://github.com/org/skills", + Digest: testCommitHash, + }, + want: "git://github.com/org/skills@" + 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/skill@sha256:" + hexDigestForTest()}, + want: true, + }, + { + name: "OCI tag source is mutable", + entry: lockfile.Entry{Source: "ghcr.io/org/skill:1.0.0"}, + want: false, + }, + { + name: "git full commit hash source is immutable", + entry: lockfile.Entry{Source: "git://github.com/org/skill@" + testCommitHash}, + want: true, + }, + { + name: "git branch source is mutable", + entry: lockfile.Entry{Source: "git://github.com/org/skill@main"}, + want: false, + }, + { + name: "git source with no ref is mutable", + entry: lockfile.Entry{Source: "git://github.com/org/skill"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, isImmutableSource(tt.entry)) + }) + } +} + +// hexDigestForTest returns a fixed, valid 64-char hex string for OCI digest fixtures. +func hexDigestForTest() string { + const s = "abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789" + return s[:64] +} diff --git a/pkg/skills/skillsvc/sync.go b/pkg/skills/skillsvc/sync.go new file mode 100644 index 0000000000..3d68c2df0a --- /dev/null +++ b/pkg/skills/skillsvc/sync.go @@ -0,0 +1,244 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + "github.com/stacklok/toolhive/pkg/storage" +) + +// Sync restores a project's installed skills 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 skills.SyncOptions) (*skills.SyncResult, error) { + if !skills.LockFileFeatureEnabled() { + return nil, errExperimentalLockFeature + } + + _, projectRoot, err := normalizeProjectRoot(skills.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: skills.ScopeProject, ProjectRoot: projectRoot}) + if err != nil { + return nil, fmt.Errorf("listing installed skills: %w", err) + } + installedByName := make(map[string]skills.InstalledSkill, len(installed)) + for _, sk := range installed { + installedByName[sk.Metadata.Name] = sk + } + + result := &skills.SyncResult{} + for _, entry := range lf.Skills { + sk, dbOK := installedByName[entry.Name] + s.syncLockedEntry(ctx, opts, entry, sk, dbOK, result) + } + for _, sk := range installed { + if _, ok := lf.Get(sk.Metadata.Name); ok { + continue // handled by the loop above + } + s.syncUnlockedInstall(ctx, opts, sk, result) + } + + return result, nil +} + +// errExperimentalLockFeature is returned by Sync/Upgrade while the lock file +// feature is behind its rollout gate (skills.LockFileFeatureEnabled). +var errExperimentalLockFeature = httperr.WithCode( + fmt.Errorf("skills lock file support is experimental; set %s=true to use it", skills.LockFileEnvVar), + http.StatusForbidden, +) + +// 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. Recording +// Missing before the Check return is what makes --check a real gate on a +// fresh clone or CI runner, where every entry has a lock entry but no +// install record. +func (s *service) syncLockedEntry( + ctx context.Context, + opts skills.SyncOptions, + entry lockfile.Entry, + sk skills.InstalledSkill, + dbOK bool, + result *skills.SyncResult, +) { + if dbOK && entryMatchesInstalled(s.pathResolver, entry, sk) { + 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, sk, dbOK); err != nil { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: entry.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + result.Installed = append(result.Installed, entry.Name) +} + +// entryMatchesInstalled reports whether the installed skill'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 entryMatchesInstalled(pathResolver skills.PathResolver, entry lockfile.Entry, sk skills.InstalledSkill) bool { + if sk.Digest != entry.Digest { + return false + } + if len(sk.Clients) == 0 { + return false + } + for _, client := range sk.Clients { + dir, err := pathResolver.GetSkillPath(client, sk.Metadata.Name, sk.Scope, sk.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 skills.SyncOptions, entry lockfile.Entry, existing skills.InstalledSkill, 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, skills.InstallOptions{ + Name: pinnedRef, + Scope: skills.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 skills.SyncOptions, sk skills.InstalledSkill, result *skills.SyncResult, +) { + if !sk.Managed { + result.NeverManaged = append(result.NeverManaged, sk.Metadata.Name) + if opts.Adopt && !opts.Check { + if err := s.adoptSkill(ctx, sk); err != nil { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: sk.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + } + } + return + } + + result.RemovedFromLock = append(result.RemovedFromLock, sk.Metadata.Name) + if opts.Prune && !opts.Check { + if err := s.Uninstall(ctx, skills.UninstallOptions{ + Name: sk.Metadata.Name, Scope: skills.ScopeProject, ProjectRoot: opts.ProjectRoot, + }); err != nil { + result.Failed = append(result.Failed, skills.SyncFailure{ + Name: sk.Metadata.Name, Reason: classifySyncFailure(err), Error: err.Error(), + }) + return + } + result.Pruned = append(result.Pruned, sk.Metadata.Name) + } +} + +// adoptSkill 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. +func (s *service) adoptSkill(ctx context.Context, sk skills.InstalledSkill) error { + contentDigest, err := computeContentDigest(s.pathResolver, sk) + if err != nil { + return fmt.Errorf("computing content digest: %w", err) + } + if err := recordLockEntry(sk.ProjectRoot, lockEntryInput{ + Name: sk.Metadata.Name, + Version: sk.Metadata.Version, + Source: sk.Reference, + ResolvedReference: sk.Reference, + Digest: sk.Digest, + ContentDigest: contentDigest, + }); err != nil { + return fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) + } + sk.Managed = true + if err := s.store.Update(ctx, sk); err != nil { + return fmt.Errorf("marking skill as lock-managed: %w", err) + } + return nil +} + +// 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. Lock-write failures are +// identified by the sentinel specifically: mapping every HTTP 500 to +// lock-write-failed would mislabel unrelated internal errors (e.g. a +// missing resolver) for the automation that keys on this reason. +func classifySyncFailure(err error) skills.FailureReason { + if errors.Is(err, errLockWrite) { + return skills.FailureReasonLockWriteFailed + } + switch httperr.Code(err) { + case http.StatusNotFound: + return skills.FailureReasonDigestMissing + case http.StatusBadGateway, http.StatusGatewayTimeout, http.StatusTooManyRequests: + return skills.FailureReasonRegistryUnreachable + case http.StatusBadRequest, http.StatusUnprocessableEntity, http.StatusConflict: + return skills.FailureReasonValidationRejected + default: + return skills.FailureReasonUnknown + } +} diff --git a/pkg/skills/skillsvc/sync_test.go b/pkg/skills/skillsvc/sync_test.go new file mode 100644 index 0000000000..f479823845 --- /dev/null +++ b/pkg/skills/skillsvc/sync_test.go @@ -0,0 +1,402 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "net/http" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + skillsmocks "github.com/stacklok/toolhive/pkg/skills/mocks" + "github.com/stacklok/toolhive/pkg/storage/sqlite" +) + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_FeatureDisabledReturnsForbidden(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + t.Setenv(skills.LockFileEnvVar, "false") + + _, err := svc.(*service).Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.Error(t, err) + assert.Equal(t, http.StatusForbidden, httperr.Code(err)) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_ReportsUpToDateWhenNothingChanged(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.AlreadyCurrent) + assert.Empty(t, result.Installed) + assert.Empty(t, result.Drifted) + assert.Empty(t, result.Failed) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_ReinstallsDriftedContent(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // Tamper with the installed file. + skillMD := filepath.Join(projectRoot, ".claude", "skills", "my-skill", "SKILL.md") + require.NoError(t, os.WriteFile(skillMD, []byte("tampered content"), 0o644)) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Drifted) + assert.Equal(t, []string{"my-skill"}, result.Installed) + + restored, err := os.ReadFile(skillMD) //nolint:gosec // fixed test path + require.NoError(t, err) + assert.Contains(t, string(restored), "name: my-skill") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_ReinstallPreservesResolvedReference(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before, ok := readLockfile(t, projectRoot).Get("my-skill") + require.True(t, ok) + require.Equal(t, ref, before.ResolvedReference) + + // Tamper with the installed file so sync has to reinstall at the pinned + // reference — the drift-repair path that must not overwrite + // ResolvedReference with that internal pinned form. + skillMD := filepath.Join(projectRoot, ".claude", "skills", "my-skill", "SKILL.md") + require.NoError(t, os.WriteFile(skillMD, []byte("tampered content"), 0o644)) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + require.Equal(t, []string{"my-skill"}, result.Installed) + + after, ok := readLockfile(t, projectRoot).Get("my-skill") + 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 newLockTestService, incompatible with t.Parallel +func TestSync_CheckReportsDriftWithoutWriting(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + skillMD := filepath.Join(projectRoot, ".claude", "skills", "my-skill", "SKILL.md") + require.NoError(t, os.WriteFile(skillMD, []byte("tampered content"), 0o644)) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Drifted) + assert.Empty(t, result.Installed, "check must not install/write anything") + + stillTampered, err := os.ReadFile(skillMD) //nolint:gosec // fixed test path + require.NoError(t, err) + assert.Equal(t, "tampered content", string(stillTampered)) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_MissingInstallIsRestored(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + require.NoError(t, os.RemoveAll(filepath.Join(projectRoot, ".claude", "skills", "my-skill"))) + require.NoError(t, svc.(*service).store.Delete(t.Context(), "my-skill", skills.ScopeProject, projectRoot)) //nolint:forcetypeassert + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Missing, "an entry with no install record is Missing") + assert.Equal(t, []string{"my-skill"}, result.Installed) + assert.Empty(t, result.Drifted, "a missing (not previously-DB-tracked) install is Missing, not Drifted") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) +} + +// TestSync_CheckReportsMissingInstalls guards the fresh-clone CI gate: a lock +// entry with no install record at all (the state of every entry on a fresh +// checkout or CI runner, where SQLite state is per-machine) must be reported +// by --check, not silently skipped. Before the fix, such entries landed in +// no result bucket and the gate exited 0 with nothing installed. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_CheckReportsMissingInstalls(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // Simulate the fresh clone: lock file committed, no local install state. + require.NoError(t, os.RemoveAll(filepath.Join(projectRoot, ".claude", "skills", "my-skill"))) + require.NoError(t, svc.(*service).store.Delete(t.Context(), "my-skill", skills.ScopeProject, projectRoot)) //nolint:forcetypeassert + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Missing, "--check must report the missing install") + assert.Empty(t, result.Installed, "check must not install anything") + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "check must not have installed anything") +} + +// TestSync_RestoreDoesNotReResolveDependencies covers the deterministic- +// restore guarantee: restoring a parent whose SKILL.md declares +// toolhive.requires must NOT re-materialize those dependencies from their +// mutable source strings. Each dependency has its own pinned lock entry the +// sync loop restores directly; re-resolving would silently upgrade and +// re-pin a dependency whose upstream tag moved. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_RestoreDoesNotReResolveDependencies(t *testing.T) { + gr, fx := newGitResolverMock(t) + // The dependency name sorts BEFORE the parent's: the sync loop restores + // it first (correctly, at its pin), and the parent's restore afterwards + // is what would re-resolve and re-pin it if requires recursion ran — + // with nothing after it in the loop to repair the damage. + depRef, _ := gitRef("a-pinned-dep") + fx.register("a-pinned-dep", gitSkill("a-pinned-dep")) + fx.register("z-parent-skill", gitSkill("z-parent-skill", depRef)) + svc, projectRoot := newLockTestService(t, gr) + + parentRef, _ := gitRef("z-parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: parentRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + depBefore, ok := readLockfile(t, projectRoot).Get("a-pinned-dep") + require.True(t, ok) + + // Move the dependency's upstream: same source string, newer content. + fx.register("a-pinned-dep", gitSkillVersion("a-pinned-dep")) + + // Wipe local state so sync must restore both entries from the lock file. + svcImpl := svc.(*service) //nolint:forcetypeassert + require.NoError(t, os.RemoveAll(filepath.Join(projectRoot, ".claude", "skills"))) + require.NoError(t, svcImpl.store.Delete(t.Context(), "z-parent-skill", skills.ScopeProject, projectRoot)) + require.NoError(t, svcImpl.store.Delete(t.Context(), "a-pinned-dep", skills.ScopeProject, projectRoot)) + + result, err := svcImpl.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Empty(t, result.Failed) + + depAfter, ok := readLockfile(t, projectRoot).Get("a-pinned-dep") + require.True(t, ok) + assert.Equal(t, depBefore.Digest, depAfter.Digest, + "restore must reinstall the dependency at its pinned digest, not re-resolve the moved source") + assert.Equal(t, depBefore.ResolvedReference, depAfter.ResolvedReference) +} + +// TestSync_RestorePreservesExplicitFlag: a restore is not a user install, +// so it must not promote a non-explicit dependency's lock entry to +// Explicit: true — that would permanently exempt it from cascade removal +// when its last requiring parent is uninstalled. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_RestorePreservesExplicitFlag(t *testing.T) { + gr, fx := newGitResolverMock(t) + depRef, _ := gitRef("dep-skill") + fx.register("dep-skill", gitSkill("dep-skill")) + fx.register("parent-skill", gitSkill("parent-skill", depRef)) + svc, projectRoot := newLockTestService(t, gr) + + parentRef, _ := gitRef("parent-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: parentRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before, ok := readLockfile(t, projectRoot).Get("dep-skill") + require.True(t, ok) + require.False(t, before.Explicit, "a materialized dependency starts non-explicit") + + // Tamper with the dependency so sync restores it. + depMD := filepath.Join(projectRoot, ".claude", "skills", "dep-skill", "SKILL.md") + require.NoError(t, os.WriteFile(depMD, []byte("tampered"), 0o644)) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Contains(t, result.Installed, "dep-skill") + + after, ok := readLockfile(t, projectRoot).Get("dep-skill") + require.True(t, ok) + assert.False(t, after.Explicit, "a restore must not promote a dependency to explicit") + assert.Equal(t, before.RequiredBy, after.RequiredBy) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_AdoptsUnmanagedInstall(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("unmanaged-skill", gitSkill("unmanaged-skill")) + svc, projectRoot := newLockTestService(t, gr) + + // Disable the feature for the initial install so it lands unmanaged + // (no lock entry, Managed=false) — simulating a pre-existing install + // from before the lock feature was ever enabled. + t.Setenv(skills.LockFileEnvVar, "false") + ref, _ := gitRef("unmanaged-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + t.Setenv(skills.LockFileEnvVar, "true") + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"unmanaged-skill"}, result.NeverManaged) + + result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Adopt: true}) + require.NoError(t, err) + assert.Equal(t, []string{"unmanaged-skill"}, result.NeverManaged) + + lf := readLockfile(t, projectRoot) + entry, ok := lf.Get("unmanaged-skill") + require.True(t, ok, "adopt must write a lock entry for the unmanaged install") + assert.NotEmpty(t, entry.ContentDigest) + + sk, err := svc.Info(t.Context(), skills.InfoOptions{Name: "unmanaged-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) + require.NotNil(t, sk.InstalledSkill) + assert.True(t, sk.InstalledSkill.Managed, "adopt must mark the DB record as managed") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestSync_PrunesRemovedFromLock(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("my-skill", gitSkill("my-skill")) + svc, projectRoot := newLockTestService(t, gr) + + ref, _ := gitRef("my-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // Simulate the entry being deleted from the lock file by hand (or by a + // teammate's commit) while the DB record and files remain. + require.NoError(t, lockfile.RemoveEntry(mustOpenRoot(t, projectRoot), "my-skill")) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.RemovedFromLock) + + result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Prune: true}) + require.NoError(t, err) + assert.Equal(t, []string{"my-skill"}, result.Pruned) + + _, err = svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.Error(t, err, "prune must uninstall the skill") +} + +// TestSync_CheckDetectsTamperInAnyClientDir: verification must cover every +// client directory a skill is installed into. Checking only the first +// 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. +// +//nolint:paralleltest // uses t.Setenv, incompatible with t.Parallel +func TestSync_CheckDetectsTamperInAnyClientDir(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("multi-skill", gitSkill("multi-skill")) + + t.Setenv(skills.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() }) + store := sqlite.NewSkillStore(db) + + projectRoot := makeProjectRoot(t) + // Unlike newLockTestService's resolver, this one gives every client its + // own directory so the per-client verification actually has two copies + // to distinguish. + ctrl := gomock.NewController(t) + pr := skillsmocks.NewMockPathResolver(ctrl) + pr.EXPECT().GetSkillPath(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes(). + DoAndReturn(func(client, skillName string, _ skills.Scope, _ string) (string, error) { + return filepath.Join(projectRoot, "."+client, "skills", skillName), nil + }) + pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) + svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) + + ref, _ := gitRef("multi-skill") + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: ref, Scope: skills.ScopeProject, ProjectRoot: projectRoot, + Clients: []string{"claude-code", "cursor"}, + }) + require.NoError(t, err) + + syncer := svc.(*service) //nolint:forcetypeassert + result, err := syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + require.Equal(t, []string{"multi-skill"}, result.AlreadyCurrent, "untampered install must verify clean") + + // Tamper with the SECOND client's copy only. + cursorMD := filepath.Join(projectRoot, ".cursor", "skills", "multi-skill", "SKILL.md") + require.NoError(t, os.WriteFile(cursorMD, []byte("tampered"), 0o644)) + + result, err = syncer.Sync(t.Context(), skills.SyncOptions{ProjectRoot: projectRoot, Check: true}) + require.NoError(t, err) + assert.Equal(t, []string{"multi-skill"}, result.Drifted, + "tampering with a non-first client's copy must be reported as drift") +} diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 7aa25c7ac8..2dc240f725 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -1173,4 +1173,83 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " _, ok = lf.Get(skillName) Expect(ok).To(BeFalse(), "expected the lock entry to be removed after uninstall") }) + + It("restores a deleted skill's files via sync", func() { + projectRoot := makeE2EProjectRoot() + skillName := "lock-e2e-sync-skill" + + By("Starting an in-process OCI registry and pushing a test skill") + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A skill for sync E2E testing") + + By("Installing the skill into the project") + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(http.StatusCreated)) + + By("Deleting the installed files without touching the lock file or DB record") + skillDir := filepath.Join(projectRoot, ".claude", "skills", skillName) + Expect(os.RemoveAll(skillDir)).To(Succeed()) + _, statErr := os.Stat(filepath.Join(skillDir, "SKILL.md")) + Expect(os.IsNotExist(statErr)).To(BeTrue()) + + By("Running sync --check — it must report drift without restoring anything") + checkResp := syncSkills(apiServer, syncSkillsRequest{ProjectRoot: projectRoot, Check: true}) + defer checkResp.Body.Close() + Expect(checkResp.StatusCode).To(Equal(http.StatusOK)) + var checkResult syncResultResponse + Expect(json.NewDecoder(checkResp.Body).Decode(&checkResult)).To(Succeed()) + Expect(checkResult.Drifted).To(ContainElement(skillName)) + _, statErr = os.Stat(filepath.Join(skillDir, "SKILL.md")) + Expect(os.IsNotExist(statErr)).To(BeTrue(), "--check must not restore files") + + By("Running sync — it must restore the missing files") + syncResp := syncSkills(apiServer, syncSkillsRequest{ProjectRoot: projectRoot}) + defer syncResp.Body.Close() + Expect(syncResp.StatusCode).To(Equal(http.StatusOK)) + var syncResult syncResultResponse + Expect(json.NewDecoder(syncResp.Body).Decode(&syncResult)).To(Succeed()) + Expect(syncResult.Installed).To(ContainElement(skillName)) + + content, readErr := os.ReadFile(filepath.Join(skillDir, "SKILL.md")) + Expect(readErr).ToNot(HaveOccurred()) + Expect(string(content)).To(ContainSubstring(skillName)) + + By("Cleaning up") + cleanupResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + defer cleanupResp.Body.Close() + }) }) + +type syncSkillsRequest 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"` +} + +type syncResultResponse struct { + Installed []string `json:"installed,omitempty"` + Drifted []string `json:"drifted,omitempty"` + AlreadyCurrent []string `json:"already_current,omitempty"` + NeverManaged []string `json:"never_managed,omitempty"` + RemovedFromLock []string `json:"removed_from_lock,omitempty"` + Pruned []string `json:"pruned,omitempty"` +} + +func syncSkills(server *e2e.Server, req syncSkillsRequest) *http.Response { + jsonData, err := json.Marshal(req) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + resp, err := http.Post( + server.BaseURL()+"/api/v1beta/skills/sync", + "application/json", + bytes.NewBuffer(jsonData), + ) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +}