From d0dad148ebeb7d5707420122d0d6ce3c18996090 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 17:01:43 +0200 Subject: [PATCH 1/4] Add thv skill upgrade: re-resolve pinned skills to newer content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC THV-0080 needs a controlled way to pull newer content for skills pinned by source, without silently drifting the lock file on every install. This adds the full upgrade vertical slice: service logic, HTTP endpoint, client, and CLI command. - Upgrade re-resolves each targeted entry's Source (via the same git/OCI/registry dispatch order Install uses, stopping short of extraction — resolveLatestState) and installs newer content when the digest changed; Source itself is never rewritten - Entries pinned to an immutable reference (an OCI digest or full git commit hash) report not-upgradable — there's nothing newer to resolve to - --preview reports what would change without persisting it; --allow-ref-change permits the resolved reference itself changing; --fail-on-changes gives CI a freshness gate - var _ skills.SkillLockService = (*service)(nil) lands here now that both Sync and Upgrade exist; SkillsRouter widens from the narrow skillSyncer interface (PR4) to the full interface - `thv skill upgrade [name...]` CLI Part of the production stack for #5715 (RFC THV-0080). Stack: 5/6. --- cmd/thv/app/skill_upgrade.go | 107 ++++++++++++++ docs/cli/thv_skill.md | 1 + docs/cli/thv_skill_upgrade.md | 51 +++++++ docs/server/docs.go | 203 +++++++++++++++++++++++++- docs/server/swagger.json | 203 +++++++++++++++++++++++++- docs/server/swagger.yaml | 148 ++++++++++++++++++- pkg/api/v1/skills.go | 67 +++++++-- pkg/api/v1/skills_sync_test.go | 16 ++- pkg/api/v1/skills_types.go | 19 +++ pkg/api/v1/skills_upgrade_test.go | 119 ++++++++++++++++ pkg/skills/client/client.go | 20 +++ pkg/skills/client/dto.go | 9 ++ pkg/skills/skillsvc/upgrade.go | 211 ++++++++++++++++++++++++++++ pkg/skills/skillsvc/upgrade_test.go | 167 ++++++++++++++++++++++ test/e2e/api_skills_test.go | 116 ++++++++++++++- 15 files changed, 1430 insertions(+), 27 deletions(-) create mode 100644 cmd/thv/app/skill_upgrade.go create mode 100644 docs/cli/thv_skill_upgrade.md create mode 100644 pkg/api/v1/skills_upgrade_test.go create mode 100644 pkg/skills/skillsvc/upgrade.go create mode 100644 pkg/skills/skillsvc/upgrade_test.go diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go new file mode 100644 index 0000000000..863af1ee92 --- /dev/null +++ b/cmd/thv/app/skill_upgrade.go @@ -0,0 +1,107 @@ +// 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 ( + skillUpgradeProjectRoot string + skillUpgradeClientsRaw string + skillUpgradePreview bool + skillUpgradeFailOnChanges bool + skillUpgradeAllowRefChange bool + skillUpgradeFormat string +) + +var skillUpgradeCmd = &cobra.Command{ + Use: "upgrade [skill-name...]", + Short: "Upgrade project skills to newer pinned content", + Long: `Re-resolve a project's lock entries and install newer content where available. + +Skills pinned to an immutable reference (an OCI digest or a full git commit +hash) are reported not-upgradable — there is nothing newer to resolve to. +Use --preview to see what would change without installing, and +--allow-ref-change to permit the resolved reference itself changing (e.g. a +registry entry repointed at a different repository).`, + PreRunE: chainPreRunE( + ValidateFormat(&skillUpgradeFormat), + ), + RunE: skillUpgradeCmdFunc, +} + +func init() { + skillCmd.AddCommand(skillUpgradeCmd) + + skillUpgradeCmd.Flags().StringVar(&skillUpgradeProjectRoot, "project-root", "", + "Project root path (default: auto-detected from the current directory)") + skillUpgradeCmd.Flags().StringVar(&skillUpgradeClientsRaw, "clients", "", + `Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client`) + skillUpgradeCmd.Flags().BoolVar(&skillUpgradePreview, "preview", false, + "Report what would change without installing anything") + skillUpgradeCmd.Flags().BoolVar(&skillUpgradeFailOnChanges, "fail-on-changes", false, + "Exit with an error if any skill would change (a CI freshness gate)") + skillUpgradeCmd.Flags().BoolVar(&skillUpgradeAllowRefChange, "allow-ref-change", false, + "Permit the resolved reference itself to change during upgrade") + AddFormatFlag(skillUpgradeCmd, &skillUpgradeFormat) +} + +func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { + projectRoot, err := resolveProjectRoot(skillUpgradeProjectRoot) + if err != nil { + return err + } + + c := newSkillClient(cmd.Context()) + result, err := c.Upgrade(cmd.Context(), skills.UpgradeOptions{ + ProjectRoot: projectRoot, + Names: args, + Clients: parseSkillInstallClients(skillUpgradeClientsRaw), + Preview: skillUpgradePreview, + FailOnChanges: skillUpgradeFailOnChanges, + AllowRefChange: skillUpgradeAllowRefChange, + }) + if err != nil { + return formatSkillError("upgrade skills", err) + } + + return printUpgradeResult(result, skillUpgradeFormat) +} + +func printUpgradeResult(result *skills.UpgradeResult, 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 + } + + if len(result.Outcomes) == 0 { + fmt.Println("No skills in the project's lock file") + return nil + } + for _, o := range result.Outcomes { + switch o.Status { + case skills.UpgradeStatusUpgraded: + fmt.Printf("%s: upgraded %s -> %s\n", o.Name, o.OldDigest, o.NewDigest) + case skills.UpgradeStatusUpToDate: + fmt.Printf("%s: up to date\n", o.Name) + case skills.UpgradeStatusNotUpgradable: + fmt.Printf("%s: not upgradable (pinned to an immutable reference)\n", o.Name) + case skills.UpgradeStatusRefChangeBlocked: + fmt.Printf("%s: reference change blocked (would move to %s; use --allow-ref-change)\n", o.Name, o.NewResolvedReference) + case skills.UpgradeStatusFailed: + fmt.Printf("%s: failed [%s]: %s\n", o.Name, o.Reason, o.Error) + } + } + return nil +} diff --git a/docs/cli/thv_skill.md b/docs/cli/thv_skill.md index 9466733538..a97336a789 100644 --- a/docs/cli/thv_skill.md +++ b/docs/cli/thv_skill.md @@ -40,5 +40,6 @@ The skill command provides subcommands to manage 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 upgrade](thv_skill_upgrade.md) - Upgrade project skills to newer pinned content * [thv skill validate](thv_skill_validate.md) - Validate a skill definition diff --git a/docs/cli/thv_skill_upgrade.md b/docs/cli/thv_skill_upgrade.md new file mode 100644 index 0000000000..b585871ded --- /dev/null +++ b/docs/cli/thv_skill_upgrade.md @@ -0,0 +1,51 @@ +--- +title: thv skill upgrade +hide_title: true +description: Reference for ToolHive CLI command `thv skill upgrade` +last_update: + author: autogenerated +slug: thv_skill_upgrade +mdx: + format: md +--- + +## thv skill upgrade + +Upgrade project skills to newer pinned content + +### Synopsis + +Re-resolve a project's lock entries and install newer content where available. + +Skills pinned to an immutable reference (an OCI digest or a full git commit +hash) are reported not-upgradable — there is nothing newer to resolve to. +Use --preview to see what would change without installing, and +--allow-ref-change to permit the resolved reference itself changing (e.g. a +registry entry repointed at a different repository). + +``` +thv skill upgrade [skill-name...] [flags] +``` + +### Options + +``` + --allow-ref-change Permit the resolved reference itself to change during upgrade + --clients string Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client + --fail-on-changes Exit with an error if any skill would change (a CI freshness gate) + --format string Output format (json, text) (default "text") + -h, --help help for upgrade + --preview Report what would change without installing anything + --project-root string Project root path (default: auto-detected from the current directory) +``` + +### 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 7bedb9c7ca..764c6b65c8 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -1651,7 +1651,7 @@ const docTemplate = `{ "type": "object" }, "github_com_stacklok_toolhive_pkg_skills.FailureReason": { - "description": "Reason is a typed failure reason for CI and automation.", + "description": "Reason is a typed failure reason when Status is UpgradeStatusFailed.", "enum": [ "registry-unreachable", "digest-missing", @@ -1946,6 +1946,68 @@ const docTemplate = `{ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome": { + "properties": { + "error": { + "description": "Error is a human-readable description of the failure, set only when Status is UpgradeStatusFailed.", + "type": "string" + }, + "name": { + "description": "Name is the skill name.", + "type": "string" + }, + "new_digest": { + "description": "NewDigest is the digest the source currently resolves to. Equal to\nOldDigest when Status is UpgradeStatusUpToDate.", + "type": "string" + }, + "new_resolved_reference": { + "description": "NewResolvedReference is the new resolvedReference when it changed.", + "type": "string" + }, + "old_digest": { + "description": "OldDigest is the digest pinned in the lock file before this operation.", + "type": "string" + }, + "reason": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason" + }, + "status": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeStatus" + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeResult": { + "properties": { + "outcomes": { + "description": "Outcomes contains one entry per skill considered for upgrade.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome" + }, + "type": "array", + "uniqueItems": false + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeStatus": { + "description": "Status is the outcome of the upgrade attempt.", + "enum": [ + "upgraded", + "up-to-date", + "not-upgradable", + "ref-change-blocked", + "failed" + ], + "type": "string", + "x-enum-varnames": [ + "UpgradeStatusUpgraded", + "UpgradeStatusUpToDate", + "UpgradeStatusNotUpgradable", + "UpgradeStatusRefChangeBlocked", + "UpgradeStatusFailed" + ] + }, "github_com_stacklok_toolhive_pkg_skills.ValidationResult": { "properties": { "errors": { @@ -3777,6 +3839,44 @@ const docTemplate = `{ }, "type": "object" }, + "pkg_api_v1.upgradeSkillsRequest": { + "description": "Request to re-resolve a project's lock entries and install newer content", + "properties": { + "allow_ref_change": { + "description": "AllowRefChange permits resolvedReference changes during upgrade", + "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 + }, + "fail_on_changes": { + "description": "FailOnChanges exits with an error when any mutable source would upgrade", + "type": "boolean" + }, + "names": { + "description": "Names restricts the upgrade to specific skill names. Empty means every entry.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "preview": { + "description": "Preview reports what would change without installing (still fetches to compare digests)", + "type": "boolean" + }, + "project_root": { + "description": "ProjectRoot is the project root path whose lock file should be upgraded", + "type": "string" + } + }, + "type": "object" + }, "pkg_api_v1.validateSkillRequest": { "description": "Request to validate a skill definition", "properties": { @@ -6462,6 +6562,107 @@ const docTemplate = `{ ] } }, + "/api/v1beta/skills/upgrade": { + "post": { + "description": "Re-resolve a project's lock entries and install newer content where available", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.upgradeSkillsRequest", + "summary": "request", + "description": "Upgrade request" + } + ] + } + } + }, + "description": "Upgrade request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeResult" + } + } + }, + "description": "OK" + }, + "400": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Bad Request" + }, + "403": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Forbidden (feature not enabled)" + }, + "404": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Found (a requested name is not in the lock file)" + }, + "409": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Conflict (--fail-on-changes tripped)" + }, + "500": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Internal Server Error" + }, + "501": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Implemented" + } + }, + "summary": "Upgrade project skills", + "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 f6b04a0b23..7091748b5a 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -1644,7 +1644,7 @@ "type": "object" }, "github_com_stacklok_toolhive_pkg_skills.FailureReason": { - "description": "Reason is a typed failure reason for CI and automation.", + "description": "Reason is a typed failure reason when Status is UpgradeStatusFailed.", "enum": [ "registry-unreachable", "digest-missing", @@ -1939,6 +1939,68 @@ }, "type": "object" }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome": { + "properties": { + "error": { + "description": "Error is a human-readable description of the failure, set only when Status is UpgradeStatusFailed.", + "type": "string" + }, + "name": { + "description": "Name is the skill name.", + "type": "string" + }, + "new_digest": { + "description": "NewDigest is the digest the source currently resolves to. Equal to\nOldDigest when Status is UpgradeStatusUpToDate.", + "type": "string" + }, + "new_resolved_reference": { + "description": "NewResolvedReference is the new resolvedReference when it changed.", + "type": "string" + }, + "old_digest": { + "description": "OldDigest is the digest pinned in the lock file before this operation.", + "type": "string" + }, + "reason": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason" + }, + "status": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeStatus" + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeResult": { + "properties": { + "outcomes": { + "description": "Outcomes contains one entry per skill considered for upgrade.", + "items": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome" + }, + "type": "array", + "uniqueItems": false + } + }, + "type": "object" + }, + "github_com_stacklok_toolhive_pkg_skills.UpgradeStatus": { + "description": "Status is the outcome of the upgrade attempt.", + "enum": [ + "upgraded", + "up-to-date", + "not-upgradable", + "ref-change-blocked", + "failed" + ], + "type": "string", + "x-enum-varnames": [ + "UpgradeStatusUpgraded", + "UpgradeStatusUpToDate", + "UpgradeStatusNotUpgradable", + "UpgradeStatusRefChangeBlocked", + "UpgradeStatusFailed" + ] + }, "github_com_stacklok_toolhive_pkg_skills.ValidationResult": { "properties": { "errors": { @@ -3770,6 +3832,44 @@ }, "type": "object" }, + "pkg_api_v1.upgradeSkillsRequest": { + "description": "Request to re-resolve a project's lock entries and install newer content", + "properties": { + "allow_ref_change": { + "description": "AllowRefChange permits resolvedReference changes during upgrade", + "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 + }, + "fail_on_changes": { + "description": "FailOnChanges exits with an error when any mutable source would upgrade", + "type": "boolean" + }, + "names": { + "description": "Names restricts the upgrade to specific skill names. Empty means every entry.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false + }, + "preview": { + "description": "Preview reports what would change without installing (still fetches to compare digests)", + "type": "boolean" + }, + "project_root": { + "description": "ProjectRoot is the project root path whose lock file should be upgraded", + "type": "string" + } + }, + "type": "object" + }, "pkg_api_v1.validateSkillRequest": { "description": "Request to validate a skill definition", "properties": { @@ -6455,6 +6555,107 @@ ] } }, + "/api/v1beta/skills/upgrade": { + "post": { + "description": "Re-resolve a project's lock entries and install newer content where available", + "requestBody": { + "content": { + "application/json": { + "schema": { + "oneOf": [ + { + "type": "object" + }, + { + "$ref": "#/components/schemas/pkg_api_v1.upgradeSkillsRequest", + "summary": "request", + "description": "Upgrade request" + } + ] + } + } + }, + "description": "Upgrade request", + "required": true + }, + "responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeResult" + } + } + }, + "description": "OK" + }, + "400": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Bad Request" + }, + "403": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Forbidden (feature not enabled)" + }, + "404": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Found (a requested name is not in the lock file)" + }, + "409": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Conflict (--fail-on-changes tripped)" + }, + "500": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Internal Server Error" + }, + "501": { + "content": { + "application/json": { + "schema": { + "type": "string" + } + } + }, + "description": "Not Implemented" + } + }, + "summary": "Upgrade project skills", + "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 ab09c2f486..f719c0f7f8 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -1699,7 +1699,7 @@ components: type: string type: object github_com_stacklok_toolhive_pkg_skills.FailureReason: - description: Reason is a typed failure reason for CI and automation. + description: Reason is a typed failure reason when Status is UpgradeStatusFailed. enum: - registry-unreachable - digest-missing @@ -1938,6 +1938,56 @@ components: type: array uniqueItems: false type: object + github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome: + properties: + error: + description: Error is a human-readable description of the failure, set only + when Status is UpgradeStatusFailed. + type: string + name: + description: Name is the skill name. + type: string + new_digest: + description: |- + NewDigest is the digest the source currently resolves to. Equal to + OldDigest when Status is UpgradeStatusUpToDate. + type: string + new_resolved_reference: + description: NewResolvedReference is the new resolvedReference when it changed. + type: string + old_digest: + description: OldDigest is the digest pinned in the lock file before this + operation. + type: string + reason: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.FailureReason' + status: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeStatus' + type: object + github_com_stacklok_toolhive_pkg_skills.UpgradeResult: + properties: + outcomes: + description: Outcomes contains one entry per skill considered for upgrade. + items: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeOutcome' + type: array + uniqueItems: false + type: object + github_com_stacklok_toolhive_pkg_skills.UpgradeStatus: + description: Status is the outcome of the upgrade attempt. + enum: + - upgraded + - up-to-date + - not-upgradable + - ref-change-blocked + - failed + type: string + x-enum-varnames: + - UpgradeStatusUpgraded + - UpgradeStatusUpToDate + - UpgradeStatusNotUpgradable + - UpgradeStatusRefChangeBlocked + - UpgradeStatusFailed github_com_stacklok_toolhive_pkg_skills.ValidationResult: properties: errors: @@ -3438,6 +3488,41 @@ components: type: array uniqueItems: false type: object + pkg_api_v1.upgradeSkillsRequest: + description: Request to re-resolve a project's lock entries and install newer + content + properties: + allow_ref_change: + description: AllowRefChange permits resolvedReference changes during upgrade + 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 + fail_on_changes: + description: FailOnChanges exits with an error when any mutable source would + upgrade + type: boolean + names: + description: Names restricts the upgrade to specific skill names. Empty + means every entry. + items: + type: string + type: array + uniqueItems: false + preview: + description: Preview reports what would change without installing (still + fetches to compare digests) + type: boolean + project_root: + description: ProjectRoot is the project root path whose lock file should + be upgraded + type: string + type: object pkg_api_v1.validateSkillRequest: description: Request to validate a skill definition properties: @@ -5379,6 +5464,67 @@ paths: summary: Sync project skills from the lock file tags: - skills + /api/v1beta/skills/upgrade: + post: + description: Re-resolve a project's lock entries and install newer content where + available + requestBody: + content: + application/json: + schema: + oneOf: + - type: object + - $ref: '#/components/schemas/pkg_api_v1.upgradeSkillsRequest' + description: Upgrade request + summary: request + description: Upgrade request + required: true + responses: + "200": + content: + application/json: + schema: + $ref: '#/components/schemas/github_com_stacklok_toolhive_pkg_skills.UpgradeResult' + description: OK + "400": + content: + application/json: + schema: + type: string + description: Bad Request + "403": + content: + application/json: + schema: + type: string + description: Forbidden (feature not enabled) + "404": + content: + application/json: + schema: + type: string + description: Not Found (a requested name is not in the lock file) + "409": + content: + application/json: + schema: + type: string + description: Conflict (--fail-on-changes tripped) + "500": + content: + application/json: + schema: + type: string + description: Internal Server Error + "501": + content: + application/json: + schema: + type: string + description: Not Implemented + summary: Upgrade project skills + 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 ac932651a9..f3937d9a2d 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -4,7 +4,6 @@ package v1 import ( - "context" "encoding/json" "errors" "fmt" @@ -17,29 +16,22 @@ 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 + lockService skills.SkillLockService } // 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. +// skillService's concrete implementation also satisfies skills.SkillLockService +// (as skillsvc.New's does), /sync and /upgrade are served; otherwise both +// return 501. func SkillsRouter(skillService skills.SkillService) http.Handler { routes := SkillsRoutes{ skillService: skillService, } - if syncer, ok := skillService.(skillSyncer); ok { - routes.lockService = syncer + if lockSvc, ok := skillService.(skills.SkillLockService); ok { + routes.lockService = lockSvc } r := chi.NewRouter() @@ -54,6 +46,7 @@ func SkillsRouter(skillService skills.SkillService) http.Handler { r.Delete("/builds/{tag}", apierrors.ErrorHandler(routes.deleteBuild)) r.Get("/content", apierrors.ErrorHandler(routes.getSkillContent)) r.Post("/sync", apierrors.ErrorHandler(routes.syncSkills)) + r.Post("/upgrade", apierrors.ErrorHandler(routes.upgradeSkills)) return r } @@ -424,3 +417,49 @@ func (s *SkillsRoutes) syncSkills(w http.ResponseWriter, r *http.Request) error w.Header().Set("Content-Type", "application/json") return json.NewEncoder(w).Encode(result) } + +// upgradeSkills re-resolves a project's lock entries and installs newer +// content where available. +// +// @Summary Upgrade project skills +// @Description Re-resolve a project's lock entries and install newer content where available +// @Tags skills +// @Accept json +// @Produce json +// @Param request body upgradeSkillsRequest true "Upgrade request" +// @Success 200 {object} skills.UpgradeResult +// @Failure 400 {string} string "Bad Request" +// @Failure 403 {string} string "Forbidden (feature not enabled)" +// @Failure 404 {string} string "Not Found (a requested name is not in the lock file)" +// @Failure 409 {string} string "Conflict (--fail-on-changes tripped)" +// @Failure 500 {string} string "Internal Server Error" +// @Failure 501 {string} string "Not Implemented" +// @Router /api/v1beta/skills/upgrade [post] +func (s *SkillsRoutes) upgradeSkills(w http.ResponseWriter, r *http.Request) error { + if s.lockService == nil { + return httperr.WithCode(errors.New("skill upgrade is not supported by this server"), http.StatusNotImplemented) + } + + var req upgradeSkillsRequest + 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.Upgrade(r.Context(), skills.UpgradeOptions{ + ProjectRoot: req.ProjectRoot, + Names: req.Names, + Preview: req.Preview, + FailOnChanges: req.FailOnChanges, + AllowRefChange: req.AllowRefChange, + Clients: req.Clients, + }) + 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 index 6f4e04ff4b..6923a9c1c7 100644 --- a/pkg/api/v1/skills_sync_test.go +++ b/pkg/api/v1/skills_sync_test.go @@ -20,18 +20,26 @@ import ( 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. +// skillServiceWithSync wraps a mocked SkillService and adds Sync and Upgrade +// methods, so SkillsRouter's opportunistic skills.SkillLockService type +// assertion succeeds — the same shape skillsvc.New's concrete service has. type skillServiceWithSync struct { skills.SkillService - syncFn func(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) + syncFn func(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) + upgradeFn func(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) } func (s *skillServiceWithSync) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.SyncResult, error) { return s.syncFn(ctx, opts) } +func (s *skillServiceWithSync) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) { + if s.upgradeFn == nil { + return &skills.UpgradeResult{}, nil + } + return s.upgradeFn(ctx, opts) +} + func TestSyncSkillsEndpoint(t *testing.T) { t.Parallel() diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index 9514c9a702..072cfb3ed8 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -86,6 +86,25 @@ type syncSkillsRequest struct { Adopt bool `json:"adopt,omitempty"` } +// upgradeSkillsRequest represents the request to upgrade a project's skills. +// +// @Description Request to re-resolve a project's lock entries and install newer content +type upgradeSkillsRequest struct { + // ProjectRoot is the project root path whose lock file should be upgraded + ProjectRoot string `json:"project_root"` + // Names restricts the upgrade to specific skill names. Empty means every entry. + Names []string `json:"names,omitempty"` + // Preview reports what would change without installing (still fetches to compare digests) + Preview bool `json:"preview,omitempty"` + // FailOnChanges exits with an error when any mutable source would upgrade + FailOnChanges bool `json:"fail_on_changes,omitempty"` + // AllowRefChange permits resolvedReference changes during upgrade + AllowRefChange bool `json:"allow_ref_change,omitempty"` + // Clients lists target client identifiers. Empty means every + // skill-supporting client detected on this host. + Clients []string `json:"clients,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/api/v1/skills_upgrade_test.go b/pkg/api/v1/skills_upgrade_test.go new file mode 100644 index 0000000000..e9f5d6192c --- /dev/null +++ b/pkg/api/v1/skills_upgrade_test.go @@ -0,0 +1,119 @@ +// 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" +) + +func TestUpgradeSkillsEndpoint(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + service skills.SkillService + body string + wantStatus int + wantContains string + }{ + { + name: "successful upgrade returns 200 with outcomes", + service: &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + upgradeFn: func(_ context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) { + assert.Equal(t, "/tmp/proj", opts.ProjectRoot) + assert.Equal(t, []string{"my-skill"}, opts.Names) + assert.True(t, opts.AllowRefChange) + return &skills.UpgradeResult{Outcomes: []skills.UpgradeOutcome{ + {Name: "my-skill", Status: skills.UpgradeStatusUpgraded}, + }}, nil + }, + }, + body: `{"project_root":"/tmp/proj","names":["my-skill"],"allow_ref_change":true}`, + wantStatus: http.StatusOK, + wantContains: `"upgraded"`, + }, + { + name: "service without Upgrade 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)), + upgradeFn: func(context.Context, skills.UpgradeOptions) (*skills.UpgradeResult, error) { + t.Fatal("Upgrade must not be called for an invalid body") + return nil, nil + }, + }, + body: `not json`, + wantStatus: http.StatusBadRequest, + }, + { + name: "fail-on-changes conflict propagates as 409", + service: &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + upgradeFn: func(context.Context, skills.UpgradeOptions) (*skills.UpgradeResult, error) { + return nil, httperr.WithCode(assert.AnError, http.StatusConflict) + }, + }, + body: `{"project_root":"/tmp/proj","fail_on_changes":true}`, + wantStatus: http.StatusConflict, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + router := SkillsRouter(tt.service) + req := httptest.NewRequest(http.MethodPost, "/upgrade", 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 TestUpgradeSkillsResponseIsValidJSON(t *testing.T) { + t.Parallel() + + svc := &skillServiceWithSync{ + SkillService: skillsmocks.NewMockSkillService(gomock.NewController(t)), + upgradeFn: func(context.Context, skills.UpgradeOptions) (*skills.UpgradeResult, error) { + return &skills.UpgradeResult{Outcomes: []skills.UpgradeOutcome{ + {Name: "a", Status: skills.UpgradeStatusUpToDate}, + }}, nil + }, + } + router := SkillsRouter(svc) + req := httptest.NewRequest(http.MethodPost, "/upgrade", bytes.NewBufferString(`{"project_root":"/tmp/proj"}`)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + require.Equal(t, http.StatusOK, rec.Code) + var result skills.UpgradeResult + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &result)) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, "a", result.Outcomes[0].Name) +} diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index 191b910aab..efa83dec9e 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -39,6 +39,7 @@ var ErrServerUnreachable = errors.New("could not reach ToolHive API server — i // Compile-time interface check. var _ skills.SkillService = (*Client)(nil) +var _ skills.SkillLockService = (*Client)(nil) // Client is an HTTP client for the ToolHive Skills API. type Client struct { @@ -284,6 +285,25 @@ func (c *Client) Sync(ctx context.Context, opts skills.SyncOptions) (*skills.Syn return &result, nil } +// Upgrade re-resolves a project's lock entries and installs newer content +// where available. +func (c *Client) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, error) { + body := upgradeRequest{ + ProjectRoot: opts.ProjectRoot, + Names: opts.Names, + Preview: opts.Preview, + FailOnChanges: opts.FailOnChanges, + AllowRefChange: opts.AllowRefChange, + Clients: opts.Clients, + } + + var result skills.UpgradeResult + if err := c.doJSONRequest(ctx, http.MethodPost, "/upgrade", 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 74b480d6eb..400f17465a 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -49,3 +49,12 @@ type syncRequest struct { Check bool `json:"check,omitempty"` Adopt bool `json:"adopt,omitempty"` } + +type upgradeRequest struct { + ProjectRoot string `json:"project_root"` + Names []string `json:"names,omitempty"` + Preview bool `json:"preview,omitempty"` + FailOnChanges bool `json:"fail_on_changes,omitempty"` + AllowRefChange bool `json:"allow_ref_change,omitempty"` + Clients []string `json:"clients,omitempty"` +} diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go new file mode 100644 index 0000000000..42db8e089e --- /dev/null +++ b/pkg/skills/skillsvc/upgrade.go @@ -0,0 +1,211 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "errors" + "fmt" + "net/http" + + nameref "github.com/google/go-containerregistry/pkg/name" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/gitresolver" + "github.com/stacklok/toolhive/pkg/skills/lockfile" +) + +// var _ ensures *service continues to satisfy the full lock service surface +// now that both Sync (PR4) and Upgrade exist. +var _ skills.SkillLockService = (*service)(nil) + +// Upgrade re-resolves each targeted lock entry's Source and, when the +// resolved digest has changed, installs the newer content and rewrites the +// entry (Source itself is never rewritten — see RFC THV-0080). Entries +// pinned to an immutable reference (an OCI digest or a full git commit hash) +// are reported not-upgradable: there is nothing newer to resolve to. +func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*skills.UpgradeResult, 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 + } + + targets, err := selectUpgradeTargets(lf, opts.Names) + if err != nil { + return nil, err + } + + result := &skills.UpgradeResult{} + for _, entry := range targets { + outcome := s.upgradeEntry(ctx, opts, entry) + result.Outcomes = append(result.Outcomes, outcome) + if opts.FailOnChanges && outcome.Status != skills.UpgradeStatusUpToDate && outcome.Status != skills.UpgradeStatusNotUpgradable { + return result, httperr.WithCode( + fmt.Errorf("skill %q would change (%s); failing due to --fail-on-changes", outcome.Name, outcome.Status), + http.StatusConflict, + ) + } + } + return result, nil +} + +// selectUpgradeTargets returns the lock entries to upgrade: every entry when +// names is empty, or the named subset in the order requested. An unknown +// name is an error — it is almost always a typo, and silently skipping it +// would make a scripted "upgrade these specific skills" call falsely report +// success. +func selectUpgradeTargets(lf *lockfile.Lockfile, names []string) ([]lockfile.Entry, error) { + if len(names) == 0 { + return lf.Skills, nil + } + targets := make([]lockfile.Entry, 0, len(names)) + for _, name := range names { + entry, ok := lf.Get(name) + if !ok { + return nil, httperr.WithCode( + fmt.Errorf("skill %q is not present in the lock file", name), + http.StatusNotFound, + ) + } + targets = append(targets, entry) + } + return targets, nil +} + +// upgradeEntry resolves entry's current state and, if warranted, applies the +// upgrade. It never returns an error: every outcome (including a resolution +// or install failure) is reported as an UpgradeOutcome so a multi-skill +// upgrade can report partial results instead of aborting on the first error. +func (s *service) upgradeEntry(ctx context.Context, opts skills.UpgradeOptions, entry lockfile.Entry) skills.UpgradeOutcome { + outcome := skills.UpgradeOutcome{Name: entry.Name, OldDigest: entry.Digest} + + if isImmutableSource(entry) { + outcome.Status = skills.UpgradeStatusNotUpgradable + return outcome + } + + newRef, newDigest, err := s.resolveLatestState(ctx, entry.Source) + if err != nil { + outcome.Status = skills.UpgradeStatusFailed + outcome.Reason = classifySyncFailure(err) + outcome.Error = err.Error() + return outcome + } + outcome.NewDigest = newDigest + + if newDigest == entry.Digest { + outcome.Status = skills.UpgradeStatusUpToDate + return outcome + } + + if newRef != entry.ResolvedReference && !opts.AllowRefChange { + outcome.Status = skills.UpgradeStatusRefChangeBlocked + outcome.NewResolvedReference = newRef + return outcome + } + if newRef != entry.ResolvedReference { + outcome.NewResolvedReference = newRef + } + + if opts.Preview { + outcome.Status = skills.UpgradeStatusUpgraded + return outcome + } + + if _, err := s.Install(ctx, skills.InstallOptions{ + Name: entry.Source, + Scope: skills.ScopeProject, + ProjectRoot: opts.ProjectRoot, + Clients: opts.Clients, + LockSource: entry.Source, + }); err != nil { + outcome.Status = skills.UpgradeStatusFailed + outcome.Reason = classifySyncFailure(err) + outcome.Error = err.Error() + return outcome + } + + outcome.Status = skills.UpgradeStatusUpgraded + return outcome +} + +// resolveLatestState re-resolves source (a lock entry's original Source +// value) to its current resolvedReference and digest, using the same +// dispatch order as Install (git, direct OCI, registry name), but stopping +// short of extraction or any DB/lock write. For OCI sources this still pulls +// the artifact into the local store — there is no lighter "digest only" +// primitive in RegistryClient — matching the RFC's "preview is not +// side-effect-free" note; git sources resolve without touching disk. +func (s *service) resolveLatestState(ctx context.Context, source string) (resolvedRef, digestStr string, err error) { + if gitresolver.IsGitReference(source) { + return s.resolveGitLatest(ctx, source) + } + + ref, isOCI, err := parseOCIReference(source) + if err != nil { + return "", "", httperr.WithCode(fmt.Errorf("invalid OCI reference %q: %w", source, err), http.StatusBadRequest) + } + if isOCI { + return s.resolveOCILatest(ctx, ref) + } + + resolved, regErr := s.resolveFromRegistry(source) + if regErr != nil { + return "", "", regErr + } + if resolved == nil { + return "", "", httperr.WithCode(fmt.Errorf("skill %q not found in registry", source), http.StatusNotFound) + } + switch { + case resolved.OCIRef != nil: + return s.resolveOCILatest(ctx, resolved.OCIRef) + case resolved.GitURL != "": + return s.resolveGitLatest(ctx, resolved.GitURL) + } + return "", "", httperr.WithCode( + fmt.Errorf("skill %q resolved from registry but has no installable package", source), + http.StatusUnprocessableEntity, + ) +} + +func (s *service) resolveGitLatest(ctx context.Context, gitURL string) (string, string, error) { + if s.gitResolver == nil { + return "", "", httperr.WithCode(errors.New("git resolver is not configured"), http.StatusInternalServerError) + } + gitRef, err := gitresolver.ParseGitReference(gitURL) + if err != nil { + return "", "", httperr.WithCode(fmt.Errorf("invalid git reference: %w", err), http.StatusBadRequest) + } + resolved, err := s.gitResolver.Resolve(ctx, gitRef) + if err != nil { + return "", "", httperr.WithCode(fmt.Errorf("resolving git skill: %w", err), http.StatusBadGateway) + } + return gitURL, resolved.CommitHash, nil +} + +func (s *service) resolveOCILatest(ctx context.Context, ref nameref.Reference) (string, string, error) { + if s.registry == nil || s.ociStore == nil { + return "", "", httperr.WithCode(errors.New("OCI registry is not configured"), http.StatusInternalServerError) + } + d, err := s.registry.Pull(ctx, s.ociStore, ref.String()) + if err != nil { + return "", "", httperr.WithCode(fmt.Errorf("pulling %q: %w", ref.String(), err), classifyPullError(err)) + } + return ref.String(), d.String(), nil +} diff --git a/pkg/skills/skillsvc/upgrade_test.go b/pkg/skills/skillsvc/upgrade_test.go new file mode 100644 index 0000000000..5996ac47b2 --- /dev/null +++ b/pkg/skills/skillsvc/upgrade_test.go @@ -0,0 +1,167 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/httperr" + "github.com/stacklok/toolhive/pkg/skills" +) + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_FeatureDisabledReturnsForbidden(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + t.Setenv(skills.LockFileEnvVar, "false") + + _, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{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 TestUpgrade_ReportsUpToDateWhenSourceUnchanged(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) + + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusUpToDate, result.Outcomes[0].Status) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_InstallsNewerContent(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 := readLockfile(t, projectRoot) + beforeEntry, _ := before.Get("my-skill") + + // Republish newer content at the same fixture URL — same source, new commit. + fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + outcome := result.Outcomes[0] + assert.Equal(t, skills.UpgradeStatusUpgraded, outcome.Status) + assert.Equal(t, beforeEntry.Digest, outcome.OldDigest) + assert.NotEqual(t, outcome.OldDigest, outcome.NewDigest) + + after := readLockfile(t, projectRoot) + afterEntry, ok := after.Get("my-skill") + require.True(t, ok) + assert.Equal(t, outcome.NewDigest, afterEntry.Digest) + assert.Equal(t, beforeEntry.Source, afterEntry.Source, "Source must never be rewritten by upgrade") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_PreviewDoesNotInstall(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 := readLockfile(t, projectRoot) + beforeEntry, _ := before.Get("my-skill") + + fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Preview: true, + }) + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusUpgraded, result.Outcomes[0].Status, "preview still reports what would happen") + + after := readLockfile(t, projectRoot) + afterEntry, ok := after.Get("my-skill") + require.True(t, ok) + assert.Equal(t, beforeEntry.Digest, afterEntry.Digest, "preview must not rewrite the lock file") +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_NotUpgradableForImmutableSource(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) + + // Hand-edit the lock entry to pin an immutable (full commit hash) source, + // as if the user had originally installed with an explicit @. + lf := readLockfile(t, projectRoot) + entry, ok := lf.Get("my-skill") + require.True(t, ok) + entry.Source = ref + "@" + entry.Digest + lf.Upsert(entry) + require.NoError(t, lf.Save(mustOpenRoot(t, projectRoot))) + + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusNotUpgradable, result.Outcomes[0].Status) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_UnknownNameReturnsNotFound(t *testing.T) { + gr, _ := newGitResolverMock(t) + svc, projectRoot := newLockTestService(t, gr) + + _, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Names: []string{"does-not-exist"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusNotFound, httperr.Code(err)) +} + +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_FailOnChangesStopsAtFirstChange(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) + + fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + + _, err = svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, Preview: true, FailOnChanges: true, + }) + require.Error(t, err) + assert.Equal(t, http.StatusConflict, httperr.Code(err)) +} diff --git a/test/e2e/api_skills_test.go b/test/e2e/api_skills_test.go index 2dc240f725..9dc93a52ad 100644 --- a/test/e2e/api_skills_test.go +++ b/test/e2e/api_skills_test.go @@ -1061,9 +1061,9 @@ func makeE2EProjectRoot() string { return resolved } -func uninstallScopedSkill(server *e2e.Server, name, scope, projectRoot string) *http.Response { - u := fmt.Sprintf("%s/api/v1beta/skills/%s?scope=%s&project_root=%s", - server.BaseURL(), name, scope, url.QueryEscape(projectRoot)) +func uninstallScopedSkill(server *e2e.Server, name, projectRoot string) *http.Response { + u := fmt.Sprintf("%s/api/v1beta/skills/%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) @@ -1135,7 +1135,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " Expect(entry.Explicit).To(BeTrue()) By("Cleaning up") - cleanupResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + cleanupResp := uninstallScopedSkill(apiServer, skillName, projectRoot) defer cleanupResp.Body.Close() }) @@ -1163,7 +1163,7 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " Expect(ok).To(BeTrue(), "expected a lock entry before uninstall") By("Uninstalling the skill") - uninstallResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + uninstallResp := uninstallScopedSkill(apiServer, skillName, projectRoot) defer uninstallResp.Body.Close() Expect(uninstallResp.StatusCode).To(Equal(http.StatusNoContent)) @@ -1219,11 +1219,115 @@ var _ = Describe("Project-scope skills lock file (RFC THV-0080)", Label("api", " Expect(string(content)).To(ContainSubstring(skillName)) By("Cleaning up") - cleanupResp := uninstallScopedSkill(apiServer, skillName, "project", projectRoot) + cleanupResp := uninstallScopedSkill(apiServer, skillName, projectRoot) + defer cleanupResp.Body.Close() + }) + + It("upgrades a skill when newer content is republished at the same reference", func() { + projectRoot := makeE2EProjectRoot() + skillName := "lock-e2e-upgrade-skill" + + By("Starting an in-process OCI registry and pushing the initial version") + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "The original description") + + 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)) + + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + lf, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + before, ok := lf.Get(skillName) + Expect(ok).To(BeTrue()) + + By("Republishing newer content at the same OCI reference") + newSkillDir := createTestSkillDir(skillName, "The updated description") + rebuildResp := buildSkill(apiServer, newSkillDir, ociRef) + defer rebuildResp.Body.Close() + Expect(rebuildResp.StatusCode).To(Equal(http.StatusOK)) + repushResp := pushSkill(apiServer, ociRef) + defer repushResp.Body.Close() + Expect(repushResp.StatusCode).To(Equal(http.StatusNoContent)) + + By("Previewing the upgrade — it must report the change without installing it") + previewResp := upgradeSkills(apiServer, upgradeSkillsRequest{ProjectRoot: projectRoot, Preview: true}) + defer previewResp.Body.Close() + Expect(previewResp.StatusCode).To(Equal(http.StatusOK)) + var previewResult upgradeResultResponse + Expect(json.NewDecoder(previewResp.Body).Decode(&previewResult)).To(Succeed()) + Expect(previewResult.Outcomes).To(HaveLen(1)) + Expect(previewResult.Outcomes[0].Status).To(Equal("upgraded")) + + lf, err = lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + afterPreview, ok := lf.Get(skillName) + Expect(ok).To(BeTrue()) + Expect(afterPreview.Digest).To(Equal(before.Digest), "preview must not rewrite the lock file") + + By("Applying the upgrade") + applyResp := upgradeSkills(apiServer, upgradeSkillsRequest{ProjectRoot: projectRoot}) + defer applyResp.Body.Close() + Expect(applyResp.StatusCode).To(Equal(http.StatusOK)) + var applyResult upgradeResultResponse + Expect(json.NewDecoder(applyResp.Body).Decode(&applyResult)).To(Succeed()) + Expect(applyResult.Outcomes).To(HaveLen(1)) + Expect(applyResult.Outcomes[0].Status).To(Equal("upgraded")) + + lf, err = lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + after, ok := lf.Get(skillName) + Expect(ok).To(BeTrue()) + Expect(after.Digest).ToNot(Equal(before.Digest), "the lock file must record the new digest") + Expect(after.Source).To(Equal(before.Source), "Source must never be rewritten by upgrade") + + By("Cleaning up") + cleanupResp := uninstallScopedSkill(apiServer, skillName, projectRoot) defer cleanupResp.Body.Close() }) }) +type upgradeSkillsRequest struct { + ProjectRoot string `json:"project_root"` + Names []string `json:"names,omitempty"` + Preview bool `json:"preview,omitempty"` + FailOnChanges bool `json:"fail_on_changes,omitempty"` + AllowRefChange bool `json:"allow_ref_change,omitempty"` + Clients []string `json:"clients,omitempty"` +} + +type upgradeOutcomeResponse struct { + Name string `json:"name"` + Status string `json:"status"` + OldDigest string `json:"old_digest,omitempty"` + NewDigest string `json:"new_digest,omitempty"` + NewResolvedReference string `json:"new_resolved_reference,omitempty"` + Reason string `json:"reason,omitempty"` + Error string `json:"error,omitempty"` +} + +type upgradeResultResponse struct { + Outcomes []upgradeOutcomeResponse `json:"outcomes"` +} + +func upgradeSkills(server *e2e.Server, req upgradeSkillsRequest) *http.Response { + jsonData, err := json.Marshal(req) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + + resp, err := http.Post( + server.BaseURL()+"/api/v1beta/skills/upgrade", + "application/json", + bytes.NewBuffer(jsonData), + ) + ExpectWithOffset(1, err).ToNot(HaveOccurred()) + return resp +} + type syncSkillsRequest struct { ProjectRoot string `json:"project_root"` Clients []string `json:"clients,omitempty"` From 4e3eeb90b1283d287909b9457fa2696b9a893622 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 19:24:18 +0200 Subject: [PATCH 2/4] Make --fail-on-changes a true no-mutate CI gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upgrade installed each target in sequence and only checked FailOnChanges after each one returned, so a multi-skill upgrade would install the first changed skill for real before reporting the conflict — a partial mutation a read-only CI gate must never produce. Split into a resolve-only planning pass (also reused to avoid re-resolving Source a second time when applying) and an apply pass that only runs once every target has been checked. Upgrade also now preserves a skill's existing clients instead of expanding to every detected client, matching Sync's behavior. --- pkg/skills/skillsvc/lock_test.go | 7 +- pkg/skills/skillsvc/upgrade.go | 108 ++++++++++++++++++++-------- pkg/skills/skillsvc/upgrade_test.go | 90 +++++++++++++++++++++-- 3 files changed, 171 insertions(+), 34 deletions(-) diff --git a/pkg/skills/skillsvc/lock_test.go b/pkg/skills/skillsvc/lock_test.go index 355e1ae08b..e46da33153 100644 --- a/pkg/skills/skillsvc/lock_test.go +++ b/pkg/skills/skillsvc/lock_test.go @@ -52,8 +52,11 @@ func newLockTestService(t *testing.T, gr *gitmocks.MockResolver) (skills.SkillSe // 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"}) + // skill-supporting client detected on the host. Deliberately returns + // more than one client so tests can distinguish "preserved the skill's + // existing (narrower) client list" from "fell through to every detected + // client" (see TestUpgrade_PreservesExistingClients). + pr.EXPECT().ListSkillSupportingClients().AnyTimes().Return([]string{"claude-code", "cursor"}) svc := New(store, WithPathResolver(pr), WithGitResolver(gr)) return svc, projectRoot diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go index 42db8e089e..9621894479 100644 --- a/pkg/skills/skillsvc/upgrade.go +++ b/pkg/skills/skillsvc/upgrade.go @@ -51,17 +51,33 @@ func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*ski return nil, err } - result := &skills.UpgradeResult{} - for _, entry := range targets { - outcome := s.upgradeEntry(ctx, opts, entry) - result.Outcomes = append(result.Outcomes, outcome) - if opts.FailOnChanges && outcome.Status != skills.UpgradeStatusUpToDate && outcome.Status != skills.UpgradeStatusNotUpgradable { - return result, httperr.WithCode( - fmt.Errorf("skill %q would change (%s); failing due to --fail-on-changes", outcome.Name, outcome.Status), - http.StatusConflict, - ) + // Resolve every target's latest state first, without installing + // anything. This lets --fail-on-changes be checked against the whole + // batch before any of it is applied — a mutating call must not upgrade + // some skills for real and then report a conflict, leaving a partially + // upgraded project with no clear signal of what happened. + plans := make([]upgradePlan, len(targets)) + for i, entry := range targets { + plans[i] = s.planUpgrade(ctx, opts, entry) + } + + if opts.FailOnChanges { + result := &skills.UpgradeResult{} + for _, p := range plans { + result.Outcomes = append(result.Outcomes, p.outcome) + if p.outcome.Status != skills.UpgradeStatusUpToDate && p.outcome.Status != skills.UpgradeStatusNotUpgradable { + return result, httperr.WithCode( + fmt.Errorf("skill %q would change (%s); failing due to --fail-on-changes", p.outcome.Name, p.outcome.Status), + http.StatusConflict, + ) + } } } + + result := &skills.UpgradeResult{} + for _, p := range plans { + result.Outcomes = append(result.Outcomes, s.applyUpgrade(ctx, opts, p)) + } return result, nil } @@ -88,16 +104,30 @@ func selectUpgradeTargets(lf *lockfile.Lockfile, names []string) ([]lockfile.Ent return targets, nil } -// upgradeEntry resolves entry's current state and, if warranted, applies the -// upgrade. It never returns an error: every outcome (including a resolution -// or install failure) is reported as an UpgradeOutcome so a multi-skill -// upgrade can report partial results instead of aborting on the first error. -func (s *service) upgradeEntry(ctx context.Context, opts skills.UpgradeOptions, entry lockfile.Entry) skills.UpgradeOutcome { +// upgradePlan is entry's resolved outcome before any install happens: either +// a terminal status (not-upgradable, up-to-date, ref-change-blocked, or a +// resolution failure) that needs no further action, or the pinned reference +// to install when the upgrade is applied. +type upgradePlan struct { + entry lockfile.Entry + outcome skills.UpgradeOutcome + pinnedRef string // set only when the upgrade needs installing + resolvedRef string // the resolved reference to record as ResolvedReference; set alongside pinnedRef +} + +// planUpgrade resolves entry's current state and determines its outcome, +// without installing anything — this lets Upgrade check --fail-on-changes +// against every target before any of them are applied. When an upgrade is +// warranted, the outcome's digest is pinned into pinnedRef so applyUpgrade +// installs exactly what was resolved here, rather than re-resolving +// entry.Source from scratch (which could pick up a different digest if a +// mutable ref moved between planning and applying). +func (s *service) planUpgrade(ctx context.Context, opts skills.UpgradeOptions, entry lockfile.Entry) upgradePlan { outcome := skills.UpgradeOutcome{Name: entry.Name, OldDigest: entry.Digest} if isImmutableSource(entry) { outcome.Status = skills.UpgradeStatusNotUpgradable - return outcome + return upgradePlan{entry: entry, outcome: outcome} } newRef, newDigest, err := s.resolveLatestState(ctx, entry.Source) @@ -105,44 +135,66 @@ func (s *service) upgradeEntry(ctx context.Context, opts skills.UpgradeOptions, outcome.Status = skills.UpgradeStatusFailed outcome.Reason = classifySyncFailure(err) outcome.Error = err.Error() - return outcome + return upgradePlan{entry: entry, outcome: outcome} } outcome.NewDigest = newDigest if newDigest == entry.Digest { outcome.Status = skills.UpgradeStatusUpToDate - return outcome + return upgradePlan{entry: entry, outcome: outcome} } if newRef != entry.ResolvedReference && !opts.AllowRefChange { outcome.Status = skills.UpgradeStatusRefChangeBlocked outcome.NewResolvedReference = newRef - return outcome + return upgradePlan{entry: entry, outcome: outcome} } if newRef != entry.ResolvedReference { outcome.NewResolvedReference = newRef } - if opts.Preview { - outcome.Status = skills.UpgradeStatusUpgraded - return outcome + pinnedRef, err := buildPinnedReference(lockfile.Entry{ResolvedReference: newRef, Digest: newDigest}) + if err != nil { + outcome.Status = skills.UpgradeStatusFailed + outcome.Reason = skills.FailureReasonUnknown + outcome.Error = fmt.Errorf("pinning resolved reference: %w", err).Error() + return upgradePlan{entry: entry, outcome: outcome} + } + + outcome.Status = skills.UpgradeStatusUpgraded + return upgradePlan{entry: entry, outcome: outcome, pinnedRef: pinnedRef, resolvedRef: newRef} +} + +// applyUpgrade installs plan's pinned content when the plan calls for it. +// Preview mode reports the plan's outcome without installing anything. +func (s *service) applyUpgrade(ctx context.Context, opts skills.UpgradeOptions, plan upgradePlan) skills.UpgradeOutcome { + if plan.pinnedRef == "" || opts.Preview { + return plan.outcome + } + + clients := opts.Clients + if len(clients) == 0 { + if existing, err := s.store.Get(ctx, plan.entry.Name, skills.ScopeProject, opts.ProjectRoot); err == nil { + clients = existing.Clients + } } if _, err := s.Install(ctx, skills.InstallOptions{ - Name: entry.Source, - Scope: skills.ScopeProject, - ProjectRoot: opts.ProjectRoot, - Clients: opts.Clients, - LockSource: entry.Source, + Name: plan.pinnedRef, + Scope: skills.ScopeProject, + ProjectRoot: opts.ProjectRoot, + Clients: clients, + LockSource: plan.entry.Source, + LockResolvedReference: plan.resolvedRef, }); err != nil { + outcome := plan.outcome outcome.Status = skills.UpgradeStatusFailed outcome.Reason = classifySyncFailure(err) outcome.Error = err.Error() return outcome } - outcome.Status = skills.UpgradeStatusUpgraded - return outcome + return plan.outcome } // resolveLatestState re-resolves source (a lock entry's original Source diff --git a/pkg/skills/skillsvc/upgrade_test.go b/pkg/skills/skillsvc/upgrade_test.go index 5996ac47b2..3a1c6761e0 100644 --- a/pkg/skills/skillsvc/upgrade_test.go +++ b/pkg/skills/skillsvc/upgrade_test.go @@ -59,7 +59,7 @@ func TestUpgrade_InstallsNewerContent(t *testing.T) { beforeEntry, _ := before.Get("my-skill") // Republish newer content at the same fixture URL — same source, new commit. - fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + fx.register("my-skill", gitSkillVersion("my-skill")) result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert require.NoError(t, err) @@ -91,7 +91,7 @@ func TestUpgrade_PreviewDoesNotInstall(t *testing.T) { before := readLockfile(t, projectRoot) beforeEntry, _ := before.Get("my-skill") - fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + fx.register("my-skill", gitSkillVersion("my-skill")) result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert ProjectRoot: projectRoot, Preview: true, @@ -106,6 +106,39 @@ func TestUpgrade_PreviewDoesNotInstall(t *testing.T) { assert.Equal(t, beforeEntry.Digest, afterEntry.Digest, "preview must not rewrite the lock file") } +// TestUpgrade_PreservesExistingClients guards against Upgrade silently +// expanding a skill to every detected client. Install without an explicit +// InstallOptions.Clients falls back to whatever the caller passed (here, +// a single client); Upgrade must default to that skill's already-installed +// Clients when opts.Clients is empty, matching Sync's reinstallPinned — not +// fall through to Install's own "no clients means every detected client" +// default. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_PreservesExistingClients(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) + + fx.register("my-skill", gitSkillVersion("my-skill")) + + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ProjectRoot: projectRoot}) //nolint:forcetypeassert + require.NoError(t, err) + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusUpgraded, result.Outcomes[0].Status) + + info, err := svc.Info(t.Context(), skills.InfoOptions{Name: "my-skill", Scope: skills.ScopeProject, ProjectRoot: projectRoot}) + require.NoError(t, err) + assert.Equal(t, []string{"claude-code"}, info.InstalledSkill.Clients, + "upgrade must preserve the skill's existing clients, not expand to every detected client") +} + //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel func TestUpgrade_NotUpgradableForImmutableSource(t *testing.T) { gr, fx := newGitResolverMock(t) @@ -146,7 +179,7 @@ func TestUpgrade_UnknownNameReturnsNotFound(t *testing.T) { } //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestUpgrade_FailOnChangesStopsAtFirstChange(t *testing.T) { +func TestUpgrade_FailOnChangesWithPreviewReportsConflictWithoutWriting(t *testing.T) { gr, fx := newGitResolverMock(t) fx.register("my-skill", gitSkill("my-skill")) svc, projectRoot := newLockTestService(t, gr) @@ -157,7 +190,7 @@ func TestUpgrade_FailOnChangesStopsAtFirstChange(t *testing.T) { }) require.NoError(t, err) - fx.register("my-skill", gitSkillVersion("my-skill", "2.0.0")) + fx.register("my-skill", gitSkillVersion("my-skill")) _, err = svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert ProjectRoot: projectRoot, Preview: true, FailOnChanges: true, @@ -165,3 +198,52 @@ func TestUpgrade_FailOnChangesStopsAtFirstChange(t *testing.T) { require.Error(t, err) assert.Equal(t, http.StatusConflict, httperr.Code(err)) } + +// TestUpgrade_FailOnChangesWithoutPreviewDoesNotMutateAnyEntry covers the +// core fix: --fail-on-changes must behave as a read-only CI gate even +// without --preview. Before the fix, Upgrade installed each target in +// sequence and only checked FailOnChanges after each one, so a multi-entry +// lock file with one changed skill would install that skill for real before +// reporting the conflict — a partial mutation the gate is supposed to +// prevent entirely. +// +//nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel +func TestUpgrade_FailOnChangesWithoutPreviewDoesNotMutateAnyEntry(t *testing.T) { + gr, fx := newGitResolverMock(t) + fx.register("changed-skill", gitSkill("changed-skill")) + fx.register("stable-skill", gitSkill("stable-skill")) + svc, projectRoot := newLockTestService(t, gr) + + changedRef, _ := gitRef("changed-skill") + stableRef, _ := gitRef("stable-skill") + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: changedRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: stableRef, Scope: skills.ScopeProject, ProjectRoot: projectRoot, Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + before := readLockfile(t, projectRoot) + changedBefore, _ := before.Get("changed-skill") + stableBefore, _ := before.Get("stable-skill") + + // Republish newer content for only one of the two skills. + fx.register("changed-skill", gitSkillVersion("changed-skill")) + + _, err = svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, FailOnChanges: true, + }) + require.Error(t, err) + assert.Equal(t, http.StatusConflict, httperr.Code(err)) + + after := readLockfile(t, projectRoot) + changedAfter, ok := after.Get("changed-skill") + require.True(t, ok) + assert.Equal(t, changedBefore.Digest, changedAfter.Digest, + "the changed skill must not be installed for real before --fail-on-changes reports the conflict") + stableAfter, ok := after.Get("stable-skill") + require.True(t, ok) + assert.Equal(t, stableBefore.Digest, stableAfter.Digest) +} From 5dca4712f7e0d890bcc120b4ef7dac9ee4a64a57 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Jul 2026 11:14:31 +0200 Subject: [PATCH 3/4] Fix broken OCI upgrade path and make fail-on-changes report-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two panel-review majors left the OCI upgrade path broken (and invisible, since coverage was git-only): resolveOCILatest returned the unqualified ref.String() while install records qualifiedOCIRef, so every digest change on a tag-less source tripped the ref-change guard instead of upgrading — the single most common upgrade case; and resolveLatestState dropped Install's registry-catalogue fallback for ambiguous names, so a skill installed through that fallback could never be upgraded. Fix both and add an OCI test matrix (tag-less upgrade, tagged up-to-date, ref-change blocked/allowed, registry-fallback source). fail_on_changes previously returned a 409 that discarded the partial result — hiding which skills were stale and conflating a genuine resolution failure with "would change". It now evaluates the plan and returns the full outcomes with a 200, mirroring sync's check mode; exit-code mapping moves entirely to the CLI. Also accept uppercase hex in isFullCommitHash, state honestly in the help text that OCI previews still fetch artifacts, and return a non-nil Outcomes slice. --- cmd/thv/app/skill_upgrade.go | 13 +- docs/cli/thv_skill_upgrade.md | 13 +- docs/server/docs.go | 10 -- docs/server/swagger.json | 10 -- docs/server/swagger.yaml | 6 - pkg/api/v1/skills.go | 5 +- pkg/skills/skillsvc/pin.go | 5 +- pkg/skills/skillsvc/pin_test.go | 8 ++ pkg/skills/skillsvc/upgrade.go | 72 ++++++---- pkg/skills/skillsvc/upgrade_oci_test.go | 182 ++++++++++++++++++++++++ pkg/skills/skillsvc/upgrade_test.go | 24 ++-- 11 files changed, 277 insertions(+), 71 deletions(-) create mode 100644 pkg/skills/skillsvc/upgrade_oci_test.go diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go index 863af1ee92..9fed7e2fae 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -28,9 +28,12 @@ var skillUpgradeCmd = &cobra.Command{ Skills pinned to an immutable reference (an OCI digest or a full git commit hash) are reported not-upgradable — there is nothing newer to resolve to. -Use --preview to see what would change without installing, and ---allow-ref-change to permit the resolved reference itself changing (e.g. a -registry entry repointed at a different repository).`, +Use --preview to see what would change without persisting anything (OCI +sources are still fetched into the local artifact store to compare digests), +and --allow-ref-change to permit the resolved reference itself changing +(e.g. a registry entry repointed at a different repository). +--fail-on-changes evaluates the same plan and never installs: it is a CI +freshness gate.`, PreRunE: chainPreRunE( ValidateFormat(&skillUpgradeFormat), ), @@ -45,9 +48,9 @@ func init() { skillUpgradeCmd.Flags().StringVar(&skillUpgradeClientsRaw, "clients", "", `Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client`) skillUpgradeCmd.Flags().BoolVar(&skillUpgradePreview, "preview", false, - "Report what would change without installing anything") + "Report what would change without persisting anything (OCI sources are still fetched to compare digests)") skillUpgradeCmd.Flags().BoolVar(&skillUpgradeFailOnChanges, "fail-on-changes", false, - "Exit with an error if any skill would change (a CI freshness gate)") + "Report what would change without installing anything; a CI freshness gate") skillUpgradeCmd.Flags().BoolVar(&skillUpgradeAllowRefChange, "allow-ref-change", false, "Permit the resolved reference itself to change during upgrade") AddFormatFlag(skillUpgradeCmd, &skillUpgradeFormat) diff --git a/docs/cli/thv_skill_upgrade.md b/docs/cli/thv_skill_upgrade.md index b585871ded..edb5873bd3 100644 --- a/docs/cli/thv_skill_upgrade.md +++ b/docs/cli/thv_skill_upgrade.md @@ -19,9 +19,12 @@ Re-resolve a project's lock entries and install newer content where available. Skills pinned to an immutable reference (an OCI digest or a full git commit hash) are reported not-upgradable — there is nothing newer to resolve to. -Use --preview to see what would change without installing, and ---allow-ref-change to permit the resolved reference itself changing (e.g. a -registry entry repointed at a different repository). +Use --preview to see what would change without persisting anything (OCI +sources are still fetched into the local artifact store to compare digests), +and --allow-ref-change to permit the resolved reference itself changing +(e.g. a registry entry repointed at a different repository). +--fail-on-changes evaluates the same plan and never installs: it is a CI +freshness gate. ``` thv skill upgrade [skill-name...] [flags] @@ -32,10 +35,10 @@ thv skill upgrade [skill-name...] [flags] ``` --allow-ref-change Permit the resolved reference itself to change during upgrade --clients string Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client - --fail-on-changes Exit with an error if any skill would change (a CI freshness gate) + --fail-on-changes Report what would change without installing anything; a CI freshness gate --format string Output format (json, text) (default "text") -h, --help help for upgrade - --preview Report what would change without installing anything + --preview Report what would change without persisting anything (OCI sources are still fetched to compare digests) --project-root string Project root path (default: auto-detected from the current directory) ``` diff --git a/docs/server/docs.go b/docs/server/docs.go index 764c6b65c8..1b2c0d523a 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -6626,16 +6626,6 @@ const docTemplate = `{ }, "description": "Not Found (a requested name is not in the lock file)" }, - "409": { - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - }, - "description": "Conflict (--fail-on-changes tripped)" - }, "500": { "content": { "application/json": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index 7091748b5a..82ca8995b3 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -6619,16 +6619,6 @@ }, "description": "Not Found (a requested name is not in the lock file)" }, - "409": { - "content": { - "application/json": { - "schema": { - "type": "string" - } - } - }, - "description": "Conflict (--fail-on-changes tripped)" - }, "500": { "content": { "application/json": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index f719c0f7f8..01c9f39276 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -5504,12 +5504,6 @@ paths: schema: type: string description: Not Found (a requested name is not in the lock file) - "409": - content: - application/json: - schema: - type: string - description: Conflict (--fail-on-changes tripped) "500": content: application/json: diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index f3937d9a2d..4f0105f68c 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -419,7 +419,9 @@ func (s *SkillsRoutes) syncSkills(w http.ResponseWriter, r *http.Request) error } // upgradeSkills re-resolves a project's lock entries and installs newer -// content where available. +// content where available. With fail_on_changes set, it evaluates the plan +// and returns the outcomes without installing anything — clients derive the +// freshness verdict from the outcome statuses, mirroring sync's check mode. // // @Summary Upgrade project skills // @Description Re-resolve a project's lock entries and install newer content where available @@ -431,7 +433,6 @@ func (s *SkillsRoutes) syncSkills(w http.ResponseWriter, r *http.Request) error // @Failure 400 {string} string "Bad Request" // @Failure 403 {string} string "Forbidden (feature not enabled)" // @Failure 404 {string} string "Not Found (a requested name is not in the lock file)" -// @Failure 409 {string} string "Conflict (--fail-on-changes tripped)" // @Failure 500 {string} string "Internal Server Error" // @Failure 501 {string} string "Not Implemented" // @Router /api/v1beta/skills/upgrade [post] diff --git a/pkg/skills/skillsvc/pin.go b/pkg/skills/skillsvc/pin.go index b84d7dd4a3..c422147c3b 100644 --- a/pkg/skills/skillsvc/pin.go +++ b/pkg/skills/skillsvc/pin.go @@ -30,12 +30,15 @@ func isImmutableSource(entry lockfile.Entry) bool { return isDigest } +// 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') { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') && (c < 'A' || c > 'F') { return false } } diff --git a/pkg/skills/skillsvc/pin_test.go b/pkg/skills/skillsvc/pin_test.go index df1ad97910..ea984a6d95 100644 --- a/pkg/skills/skillsvc/pin_test.go +++ b/pkg/skills/skillsvc/pin_test.go @@ -90,6 +90,14 @@ func TestIsImmutableSource(t *testing.T) { entry: lockfile.Entry{Source: "git://github.com/org/skill@main"}, want: false, }, + { + // The sibling git resolver accepts uppercase hex; classifying + // an uppercase pin as mutable would needlessly re-clone it on + // every upgrade despite the pin being immutable. + name: "git uppercase full commit hash source is immutable", + entry: lockfile.Entry{Source: "git://github.com/org/skill@ABCDEF0123456789ABCDEF0123456789ABCDEF01"}, + want: true, + }, { name: "git source with no ref is mutable", entry: lockfile.Entry{Source: "git://github.com/org/skill"}, diff --git a/pkg/skills/skillsvc/upgrade.go b/pkg/skills/skillsvc/upgrade.go index 9621894479..0ef8900601 100644 --- a/pkg/skills/skillsvc/upgrade.go +++ b/pkg/skills/skillsvc/upgrade.go @@ -52,30 +52,23 @@ func (s *service) Upgrade(ctx context.Context, opts skills.UpgradeOptions) (*ski } // Resolve every target's latest state first, without installing - // anything. This lets --fail-on-changes be checked against the whole - // batch before any of it is applied — a mutating call must not upgrade - // some skills for real and then report a conflict, leaving a partially - // upgraded project with no clear signal of what happened. + // anything. FailOnChanges is a CI freshness gate: it reports the full + // planned outcome set and never runs the apply pass at all — returning + // the outcomes (rather than an error that discards them) lets callers + // see exactly which skills are stale and distinguish "would change" + // from a genuine resolution failure. Exit-code mapping happens in the + // CLI from these outcomes, mirroring how sync --check works. plans := make([]upgradePlan, len(targets)) for i, entry := range targets { plans[i] = s.planUpgrade(ctx, opts, entry) } - if opts.FailOnChanges { - result := &skills.UpgradeResult{} - for _, p := range plans { + result := &skills.UpgradeResult{Outcomes: make([]skills.UpgradeOutcome, 0, len(plans))} + for _, p := range plans { + if opts.FailOnChanges { result.Outcomes = append(result.Outcomes, p.outcome) - if p.outcome.Status != skills.UpgradeStatusUpToDate && p.outcome.Status != skills.UpgradeStatusNotUpgradable { - return result, httperr.WithCode( - fmt.Errorf("skill %q would change (%s); failing due to --fail-on-changes", p.outcome.Name, p.outcome.Status), - http.StatusConflict, - ) - } + continue } - } - - result := &skills.UpgradeResult{} - for _, p := range plans { result.Outcomes = append(result.Outcomes, s.applyUpgrade(ctx, opts, p)) } return result, nil @@ -199,11 +192,12 @@ func (s *service) applyUpgrade(ctx context.Context, opts skills.UpgradeOptions, // resolveLatestState re-resolves source (a lock entry's original Source // value) to its current resolvedReference and digest, using the same -// dispatch order as Install (git, direct OCI, registry name), but stopping -// short of extraction or any DB/lock write. For OCI sources this still pulls -// the artifact into the local store — there is no lighter "digest only" -// primitive in RegistryClient — matching the RFC's "preview is not -// side-effect-free" note; git sources resolve without touching disk. +// dispatch order as Install (git, direct OCI with registry fallback, +// registry name), but stopping short of extraction or any DB/lock write. +// For OCI sources this still pulls the artifact into the local store — +// there is no lighter "digest only" primitive in RegistryClient — matching +// the RFC's "preview is not side-effect-free" note; git sources resolve +// without touching disk. func (s *service) resolveLatestState(ctx context.Context, source string) (resolvedRef, digestStr string, err error) { if gitresolver.IsGitReference(source) { return s.resolveGitLatest(ctx, source) @@ -214,7 +208,24 @@ func (s *service) resolveLatestState(ctx context.Context, source string) (resolv return "", "", httperr.WithCode(fmt.Errorf("invalid OCI reference %q: %w", source, err), http.StatusBadRequest) } if isOCI { - return s.resolveOCILatest(ctx, ref) + newRef, newDigest, ociErr := s.resolveOCILatest(ctx, ref) + if ociErr == nil { + return newRef, newDigest, nil + } + // Mirror Install's fallback: an ambiguous "namespace/name" that + // fails as a direct OCI pull may be a registry catalogue name — the + // path the skill was originally installed through. Without this + // branch, a skill that installs cleanly via the registry fallback + // could never be upgraded (its source fails the direct pull exactly + // as it did at install time). + if isUnambiguousOCIRef(source, ref) { + return "", "", ociErr + } + resolved, regErr := s.resolveFromRegistry(source) + if regErr != nil || resolved == nil { + return "", "", ociErr + } + return s.resolveRegistryLatest(ctx, source, resolved) } resolved, regErr := s.resolveFromRegistry(source) @@ -224,6 +235,14 @@ func (s *service) resolveLatestState(ctx context.Context, source string) (resolv if resolved == nil { return "", "", httperr.WithCode(fmt.Errorf("skill %q not found in registry", source), http.StatusNotFound) } + return s.resolveRegistryLatest(ctx, source, resolved) +} + +// resolveRegistryLatest resolves the latest state of a registry catalogue +// result, dispatching to the OCI or git resolver it points at. +func (s *service) resolveRegistryLatest( + ctx context.Context, source string, resolved *registryResolveResult, +) (string, string, error) { switch { case resolved.OCIRef != nil: return s.resolveOCILatest(ctx, resolved.OCIRef) @@ -259,5 +278,10 @@ func (s *service) resolveOCILatest(ctx context.Context, ref nameref.Reference) ( if err != nil { return "", "", httperr.WithCode(fmt.Errorf("pulling %q: %w", ref.String(), err), classifyPullError(err)) } - return ref.String(), d.String(), nil + // qualifiedOCIRef, not ref.String(): install records the qualified form + // (implicit ":latest" made explicit) in ResolvedReference, and this value + // is compared against it for the ref-change guard. The unqualified form + // would misreport every digest change on a tag-less source as a blocked + // reference change. + return qualifiedOCIRef(ref), d.String(), nil } diff --git a/pkg/skills/skillsvc/upgrade_oci_test.go b/pkg/skills/skillsvc/upgrade_oci_test.go new file mode 100644 index 0000000000..3a65dcbff4 --- /dev/null +++ b/pkg/skills/skillsvc/upgrade_oci_test.go @@ -0,0 +1,182 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "fmt" + "testing" + + godigest "github.com/opencontainers/go-digest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + ociskills "github.com/stacklok/toolhive-core/oci/skills" + ocimocks "github.com/stacklok/toolhive-core/oci/skills/mocks" + regtypes "github.com/stacklok/toolhive-core/registry/types" + regmocks "github.com/stacklok/toolhive/pkg/registry/mocks" + "github.com/stacklok/toolhive/pkg/skills" + "github.com/stacklok/toolhive/pkg/skills/lockfile" + storemocks "github.com/stacklok/toolhive/pkg/storage/mocks" +) + +// newOCIUpgradeService builds a service with a mocked OCI registry client, a +// real (temp) OCI store, and an optional registry lookup — the dependencies +// planUpgrade's OCI resolution path touches. The skill store is a mock with +// no expectations: planning never writes. +func newOCIUpgradeService( + t *testing.T, reg ociskills.RegistryClient, lookup *regmocks.MockProvider, +) *service { + t.Helper() + ctrl := gomock.NewController(t) + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + opts := []Option{WithRegistryClient(reg), WithOCIStore(ociStore)} + if lookup != nil { + opts = append(opts, WithSkillLookup(lookup)) + } + svc := New(storemocks.NewMockSkillStore(ctrl), opts...) + return svc.(*service) //nolint:forcetypeassert // white-box test in the same package +} + +func ociTestDigest(seed byte) string { + const alphabet = "0123456789abcdef" + b := make([]byte, 64) + for i := range b { + b[i] = alphabet[(i+int(seed))%len(alphabet)] + } + return "sha256:" + string(b) +} + +// TestPlanUpgrade_OCITagLessSourceUpgrades pins the resolvedReference +// qualification contract: install records qualifiedOCIRef (implicit +// ":latest" made explicit), so upgrade's resolution must produce the same +// form. Before the fix, resolveOCILatest returned ref.String() — the +// unqualified form — so every digest change on a tag-less source was +// misreported as a blocked reference change instead of an upgrade. +func TestPlanUpgrade_OCITagLessSourceUpgrades(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "ghcr.io/org/skill"). + Return(godigest.Digest(ociTestDigest(2)), nil) + svc := newOCIUpgradeService(t, reg, nil) + + entry := lockfile.Entry{ + Name: "my-skill", + Source: "ghcr.io/org/skill", // tag-less: the most common upgrade case + ResolvedReference: "ghcr.io/org/skill:latest", + Digest: ociTestDigest(1), + } + plan := svc.planUpgrade(t.Context(), skills.UpgradeOptions{}, entry) + + assert.Equal(t, skills.UpgradeStatusUpgraded, plan.outcome.Status, + "a moved digest on a tag-less source is an upgrade, not a blocked ref change") + assert.Equal(t, ociTestDigest(2), plan.outcome.NewDigest) + assert.Equal(t, "ghcr.io/org/skill:latest", plan.resolvedRef, + "upgrade must resolve to the same qualified form install records") +} + +func TestPlanUpgrade_OCITaggedSourceUpToDate(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "ghcr.io/org/skill:v1"). + Return(godigest.Digest(ociTestDigest(1)), nil) + svc := newOCIUpgradeService(t, reg, nil) + + entry := lockfile.Entry{ + Name: "my-skill", + Source: "ghcr.io/org/skill:v1", + ResolvedReference: "ghcr.io/org/skill:v1", + Digest: ociTestDigest(1), + } + plan := svc.planUpgrade(t.Context(), skills.UpgradeOptions{}, entry) + assert.Equal(t, skills.UpgradeStatusUpToDate, plan.outcome.Status) +} + +// TestPlanUpgrade_OCIRefChangeGuard exercises the guard on a genuine +// reference change (the recorded resolvedReference no longer matches what +// the source resolves to): blocked by default, permitted with +// --allow-ref-change. +func TestPlanUpgrade_OCIRefChangeGuard(t *testing.T) { + t.Parallel() + + entry := lockfile.Entry{ + Name: "my-skill", + Source: "ghcr.io/org/skill", + ResolvedReference: "ghcr.io/org/old-location:latest", // recorded before the repoint + Digest: ociTestDigest(1), + } + + t.Run("blocked without allow-ref-change", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "ghcr.io/org/skill"). + Return(godigest.Digest(ociTestDigest(2)), nil) + svc := newOCIUpgradeService(t, reg, nil) + + plan := svc.planUpgrade(t.Context(), skills.UpgradeOptions{}, entry) + assert.Equal(t, skills.UpgradeStatusRefChangeBlocked, plan.outcome.Status) + assert.Equal(t, "ghcr.io/org/skill:latest", plan.outcome.NewResolvedReference) + assert.Empty(t, plan.pinnedRef, "a blocked plan must not carry anything to install") + }) + + t.Run("permitted with allow-ref-change", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "ghcr.io/org/skill"). + Return(godigest.Digest(ociTestDigest(2)), nil) + svc := newOCIUpgradeService(t, reg, nil) + + plan := svc.planUpgrade(t.Context(), skills.UpgradeOptions{AllowRefChange: true}, entry) + assert.Equal(t, skills.UpgradeStatusUpgraded, plan.outcome.Status) + assert.Equal(t, "ghcr.io/org/skill:latest", plan.outcome.NewResolvedReference) + assert.NotEmpty(t, plan.pinnedRef) + }) +} + +// TestPlanUpgrade_RegistryFallbackSourceUpgrades covers the dropped branch: +// a skill originally installed through Install's OCI-pull -> registry- +// catalogue fallback has an ambiguous "namespace/name" source that fails a +// direct pull. Without mirroring the fallback, such a skill installs +// cleanly but can never be upgraded — its resolution fails exactly as the +// direct pull did at install time. +func TestPlanUpgrade_RegistryFallbackSourceUpgrades(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + reg := ocimocks.NewMockRegistryClient(ctrl) + // The direct pull of the ambiguous source fails, as it did at install. + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "io.github.test/my-skill"). + Return(godigest.Digest(""), fmt.Errorf("name unknown")) + // The registry catalogue resolves it to a concrete OCI reference. + reg.EXPECT().Pull(gomock.Any(), gomock.Any(), "ghcr.io/test/my-skill:v2"). + Return(godigest.Digest(ociTestDigest(3)), nil) + lookup := regmocks.NewMockProvider(ctrl) + lookup.EXPECT().SearchSkills("my-skill").Return([]regtypes.Skill{ + { + Namespace: "io.github.test", + Name: "my-skill", + Packages: []regtypes.SkillPackage{ + {RegistryType: "oci", Identifier: "ghcr.io/test/my-skill:v2"}, + }, + }, + }, nil) + svc := newOCIUpgradeService(t, reg, lookup) + + entry := lockfile.Entry{ + Name: "my-skill", + Source: "io.github.test/my-skill", // registry catalogue name, ambiguous OCI shape + ResolvedReference: "ghcr.io/test/my-skill:v2", + Digest: ociTestDigest(1), + } + plan := svc.planUpgrade(t.Context(), skills.UpgradeOptions{}, entry) + + assert.Equal(t, skills.UpgradeStatusUpgraded, plan.outcome.Status, + "a registry-fallback-installed skill must be upgradable through the same fallback") + assert.Equal(t, ociTestDigest(3), plan.outcome.NewDigest) +} diff --git a/pkg/skills/skillsvc/upgrade_test.go b/pkg/skills/skillsvc/upgrade_test.go index 3a1c6761e0..90f7146624 100644 --- a/pkg/skills/skillsvc/upgrade_test.go +++ b/pkg/skills/skillsvc/upgrade_test.go @@ -178,8 +178,14 @@ func TestUpgrade_UnknownNameReturnsNotFound(t *testing.T) { assert.Equal(t, http.StatusNotFound, httperr.Code(err)) } +// TestUpgrade_FailOnChangesReportsOutcomesWithoutError: the gate returns +// the full outcome set with a 200 rather than an error — callers (and the +// CLI's exit-code mapping) derive the freshness verdict from the outcomes, +// which also distinguishes "would change" from a genuine resolution +// failure and names the stale skills. +// //nolint:paralleltest // uses t.Setenv via newLockTestService, incompatible with t.Parallel -func TestUpgrade_FailOnChangesWithPreviewReportsConflictWithoutWriting(t *testing.T) { +func TestUpgrade_FailOnChangesReportsOutcomesWithoutError(t *testing.T) { gr, fx := newGitResolverMock(t) fx.register("my-skill", gitSkill("my-skill")) svc, projectRoot := newLockTestService(t, gr) @@ -192,11 +198,13 @@ func TestUpgrade_FailOnChangesWithPreviewReportsConflictWithoutWriting(t *testin fx.register("my-skill", gitSkillVersion("my-skill")) - _, err = svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert - ProjectRoot: projectRoot, Preview: true, FailOnChanges: true, + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + ProjectRoot: projectRoot, FailOnChanges: true, }) - require.Error(t, err) - assert.Equal(t, http.StatusConflict, httperr.Code(err)) + require.NoError(t, err, "fail-on-changes reports outcomes, it does not error") + require.Len(t, result.Outcomes, 1) + assert.Equal(t, skills.UpgradeStatusUpgraded, result.Outcomes[0].Status, + "the outcome reports what would change so callers can name the stale skill") } // TestUpgrade_FailOnChangesWithoutPreviewDoesNotMutateAnyEntry covers the @@ -232,11 +240,11 @@ func TestUpgrade_FailOnChangesWithoutPreviewDoesNotMutateAnyEntry(t *testing.T) // Republish newer content for only one of the two skills. fx.register("changed-skill", gitSkillVersion("changed-skill")) - _, err = svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert + result, err := svc.(*service).Upgrade(t.Context(), skills.UpgradeOptions{ //nolint:forcetypeassert ProjectRoot: projectRoot, FailOnChanges: true, }) - require.Error(t, err) - assert.Equal(t, http.StatusConflict, httperr.Code(err)) + require.NoError(t, err) + require.Len(t, result.Outcomes, 2) after := readLockfile(t, projectRoot) changedAfter, ok := after.Get("changed-skill") From 01bebac37186858f8788748ba95dad7c57d1a359 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 24 Jul 2026 10:02:28 +0000 Subject: [PATCH 4/4] Fix forwarding notification context cancellation race --- pkg/vmcp/client/forwarding.go | 9 +++++++-- pkg/vmcp/client/forwarding_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/pkg/vmcp/client/forwarding.go b/pkg/vmcp/client/forwarding.go index d61c324add..b6e94427d5 100644 --- a/pkg/vmcp/client/forwarding.go +++ b/pkg/vmcp/client/forwarding.go @@ -214,11 +214,16 @@ func newSamplingForwarder(callCtx context.Context, req vmcp.SamplingRequester) c // never surfaced to the backend. Other notification methods are ignored (the // go-sdk server re-emits list-changed notifications automatically). func newNotificationForwarder(callCtx context.Context, notifier vmcp.ClientNotifier) func(mcp.JSONRPCNotification) { + // Backend notifications are delivered asynchronously and can arrive just after + // the tool call context is cancelled; keep the captured downstream-session + // values but ignore cancellation so best-effort forwarding still runs. + forwardCtx := context.WithoutCancel(callCtx) + return func(n mcp.JSONRPCNotification) { fields := n.Params.AdditionalFields switch n.Method { case vmcp.MethodProgressNotification: - err := notifier.NotifyProgress(callCtx, vmcp.ProgressNotification{ + err := notifier.NotifyProgress(forwardCtx, vmcp.ProgressNotification{ ProgressToken: fields["progressToken"], Progress: toFloat(fields["progress"]), Total: toFloat(fields["total"]), @@ -228,7 +233,7 @@ func newNotificationForwarder(callCtx context.Context, notifier vmcp.ClientNotif slog.Debug("failed to forward progress notification", "error", err) } case vmcp.MethodLogNotification: - err := notifier.NotifyLog(callCtx, vmcp.LogMessage{ + err := notifier.NotifyLog(forwardCtx, vmcp.LogMessage{ Level: toString(fields["level"]), Logger: toString(fields["logger"]), Data: fields["data"], diff --git a/pkg/vmcp/client/forwarding_test.go b/pkg/vmcp/client/forwarding_test.go index 9476dcc4f1..b4c142222c 100644 --- a/pkg/vmcp/client/forwarding_test.go +++ b/pkg/vmcp/client/forwarding_test.go @@ -175,6 +175,34 @@ func TestNewNotificationForwarder_ForwardsLog(t *testing.T) { }) } +func TestNewNotificationForwarder_UsesWithoutCancelContext(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + notifier := mocks.NewMockClientNotifier(ctrl) + + baseCtx, cancel := context.WithCancel(context.WithValue(t.Context(), ctxKey{}, "downstream-session")) + cancel() + + notifier.EXPECT(). + NotifyLog(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, _ vmcp.LogMessage) error { + assert.Equal(t, "downstream-session", ctx.Value(ctxKey{})) + assert.NoError(t, ctx.Err(), "notification forwarding should ignore cancellation on callCtx") + return nil + }) + + handler := newNotificationForwarder(baseCtx, notifier) + handler(mcp.JSONRPCNotification{ + Notification: mcp.Notification{ + Method: vmcp.MethodLogNotification, + Params: mcp.NotificationParams{ + AdditionalFields: map[string]any{"level": "info", "data": "hello"}, + }, + }, + }) +} + // TestNewNotificationForwarder_IgnoresUnknownMethod verifies that a notification // method the forwarder does not relay does not reach the notifier. func TestNewNotificationForwarder_IgnoresUnknownMethod(t *testing.T) {