From 7d428b6b0d3b4c001cebb19909233dd4c1fde4ee Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 17:19:17 +0200 Subject: [PATCH 1/3] Add typed exit codes and confirmation gate to skill sync/upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC THV-0080 gives CI a scriptable contract for sync/upgrade: exit 2 for check/freshness failures, 3 for partial failures, 4 for policy rejections, distinct from the generic 1 cobra already uses. It also requires a pre-install confirmation gate — skill content is a set of AI-followed instructions, so a non-interactive run must refuse outright without --yes rather than silently proceeding. - cmd/thv/app/exitcode.go: exitCodeError carries a specific exit code through a RunE return; main.go maps it via ExitCodeFromError instead of a hardcoded os.Exit(1) - cmd/thv/app/skill_confirm.go: requireConfirmation prompts on a TTY, refuses with ExitCodePolicyRejection off one - sync/upgrade gain --yes, skipped automatically by --check/ --preview (which never install anything); their exit-code mapping prioritizes partial failure over drift/ref-change-blocked - docs/arch/12-skills-system.md: new Project Lock File section covering rollout gating, schema, install/uninstall hooks, sync, upgrade, and the exit-code contract - CLI E2E coverage for all four exit codes (0/2/4, plus --yes) This is the last PR of Stack 1 (lock file + sync + upgrade) for RFC THV-0080. Stack 2 (Sigstore signing/verification) builds on top once this stack is fully merged; TOOLHIVE_SKILLS_LOCK_ENABLED stays off by default until that full v1 lands. Part of the production stack for #5715 (RFC THV-0080). Stack: 6/6. --- cmd/thv/app/exitcode.go | 60 ++++++++++++++++ cmd/thv/app/exitcode_test.go | 53 ++++++++++++++ cmd/thv/app/skill_confirm.go | 42 +++++++++++ cmd/thv/app/skill_confirm_test.go | 31 +++++++++ cmd/thv/app/skill_sync.go | 39 ++++++++++- cmd/thv/app/skill_sync_test.go | 98 ++++++++++++++++++++++++++ cmd/thv/app/skill_upgrade.go | 63 ++++++++++++++++- cmd/thv/app/skill_upgrade_test.go | 91 ++++++++++++++++++++++++ cmd/thv/main.go | 2 +- docs/arch/12-skills-system.md | 96 ++++++++++++++++++++++++- docs/cli/thv_skill_sync.md | 5 ++ docs/cli/thv_skill_upgrade.md | 5 ++ test/e2e/cli_skills_lock_test.go | 112 ++++++++++++++++++++++++++++++ 13 files changed, 689 insertions(+), 8 deletions(-) create mode 100644 cmd/thv/app/exitcode.go create mode 100644 cmd/thv/app/exitcode_test.go create mode 100644 cmd/thv/app/skill_confirm.go create mode 100644 cmd/thv/app/skill_confirm_test.go create mode 100644 cmd/thv/app/skill_sync_test.go create mode 100644 cmd/thv/app/skill_upgrade_test.go create mode 100644 test/e2e/cli_skills_lock_test.go diff --git a/cmd/thv/app/exitcode.go b/cmd/thv/app/exitcode.go new file mode 100644 index 0000000000..210152f65d --- /dev/null +++ b/cmd/thv/app/exitcode.go @@ -0,0 +1,60 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import "errors" + +// Exit codes for thv skill sync/upgrade, per RFC THV-0080's CI/scripting +// contract. 0 (success) and 1 (generic/unclassified error) are Go and +// cobra's own defaults and are not represented here. +const ( + // ExitCodeCheckFailure means sync --check or upgrade --fail-on-changes + // found drift/available changes: the project does not match its lock + // file, but nothing was installed, written, or removed. + ExitCodeCheckFailure = 2 + // ExitCodePartialFailure means some, but not all, of the targeted + // skills failed during sync or upgrade; check the reported outcomes for + // which ones. + ExitCodePartialFailure = 3 + // ExitCodePolicyRejection means the operation was refused by policy + // rather than attempted and failed: a non-interactive sync/upgrade + // declined the pre-install confirmation gate without --yes, or a + // ref/signer-change guard blocked without its override flag. + ExitCodePolicyRejection = 4 +) + +// exitCodeError pairs an error with the process exit code main() should use +// for it, so business logic in a RunE can request a specific exit code +// without main() needing to know the semantics of every command's errors. +type exitCodeError struct { + err error + code int +} + +// withExitCode wraps err so ExitCodeFromError reports code for it. Returns +// nil if err is nil, so callers can write `return withExitCode(err, ...)` +// unconditionally after an operation that may or may not have failed. +func withExitCode(err error, code int) error { + if err == nil { + return nil + } + return &exitCodeError{err: err, code: code} +} + +func (e *exitCodeError) Error() string { return e.err.Error() } +func (e *exitCodeError) Unwrap() error { return e.err } + +// ExitCodeFromError returns the process exit code for err: 0 for nil, the +// code carried by an exitCodeError (see withExitCode) if err wraps one, +// otherwise 1 — the generic failure code cobra callers already expect. +func ExitCodeFromError(err error) int { + if err == nil { + return 0 + } + var ece *exitCodeError + if errors.As(err, &ece) { + return ece.code + } + return 1 +} diff --git a/cmd/thv/app/exitcode_test.go b/cmd/thv/app/exitcode_test.go new file mode 100644 index 0000000000..71bb076f51 --- /dev/null +++ b/cmd/thv/app/exitcode_test.go @@ -0,0 +1,53 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "errors" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestExitCodeFromError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want int + }{ + {name: "nil error", err: nil, want: 0}, + {name: "generic error", err: errors.New("boom"), want: 1}, + {name: "check failure", err: withExitCode(errors.New("drift"), ExitCodeCheckFailure), want: 2}, + {name: "partial failure", err: withExitCode(errors.New("partial"), ExitCodePartialFailure), want: 3}, + {name: "policy rejection", err: withExitCode(errors.New("refused"), ExitCodePolicyRejection), want: 4}, + { + name: "wrapped exit code error is still detected", + err: fmt.Errorf("context: %w", withExitCode(errors.New("drift"), ExitCodeCheckFailure)), + want: 2, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, ExitCodeFromError(tt.err)) + }) + } +} + +func TestWithExitCodeNilIsNil(t *testing.T) { + t.Parallel() + assert.NoError(t, withExitCode(nil, ExitCodeCheckFailure)) +} + +func TestExitCodeErrorUnwraps(t *testing.T) { + t.Parallel() + inner := errors.New("boom") + err := withExitCode(inner, ExitCodePartialFailure) + assert.ErrorIs(t, err, inner) + assert.Equal(t, inner.Error(), err.Error()) +} diff --git a/cmd/thv/app/skill_confirm.go b/cmd/thv/app/skill_confirm.go new file mode 100644 index 0000000000..a0f92a2cc7 --- /dev/null +++ b/cmd/thv/app/skill_confirm.go @@ -0,0 +1,42 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "bufio" + "fmt" + "os" + "strings" + + "golang.org/x/term" +) + +// requireConfirmation enforces RFC THV-0080's pre-install confirmation gate +// for sync/upgrade. With yes set, it returns immediately without prompting. +// On an interactive terminal, it prompts and reports whether the user +// confirmed. In a non-interactive context, it refuses outright with a +// policy-rejection exit code rather than silently proceeding: skill content +// is a set of AI-executed instructions, so unattended execution without an +// explicit --yes is not an acceptable default the way it might be for a +// lower-stakes operation. +func requireConfirmation(action string, yes bool) (confirmed bool, err error) { + if yes { + return true, nil + } + if !term.IsTerminal(int(os.Stdin.Fd())) { //nolint:gosec // uintptr fits int on all supported platforms + return false, withExitCode( + fmt.Errorf("%s requires confirmation; pass --yes to run non-interactively", action), + ExitCodePolicyRejection, + ) + } + + fmt.Printf("%s? [y/N]: ", action) + reader := bufio.NewReader(os.Stdin) + response, readErr := reader.ReadString('\n') + if readErr != nil { + return false, fmt.Errorf("failed to read user input: %w", readErr) + } + response = strings.TrimSpace(strings.ToLower(response)) + return response == "y" || response == "yes", nil +} diff --git a/cmd/thv/app/skill_confirm_test.go b/cmd/thv/app/skill_confirm_test.go new file mode 100644 index 0000000000..e2bec9ae06 --- /dev/null +++ b/cmd/thv/app/skill_confirm_test.go @@ -0,0 +1,31 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRequireConfirmationYesSkipsPrompt(t *testing.T) { + t.Parallel() + confirmed, err := requireConfirmation("do the thing", true) + require.NoError(t, err) + assert.True(t, confirmed) +} + +// TestRequireConfirmationNonInteractiveWithoutYesRefuses exercises the +// non-interactive path: in this test process, os.Stdin is not a terminal +// (it's whatever `go test` wires up), so this reaches the policy-rejection +// branch without needing to fake TTY detection. +func TestRequireConfirmationNonInteractiveWithoutYesRefuses(t *testing.T) { + t.Parallel() + confirmed, err := requireConfirmation("do the thing", false) + require.Error(t, err) + assert.False(t, confirmed) + assert.Equal(t, ExitCodePolicyRejection, ExitCodeFromError(err)) + assert.Contains(t, err.Error(), "--yes") +} diff --git a/cmd/thv/app/skill_sync.go b/cmd/thv/app/skill_sync.go index 300b2634ea..26e4d4ff75 100644 --- a/cmd/thv/app/skill_sync.go +++ b/cmd/thv/app/skill_sync.go @@ -18,6 +18,7 @@ var ( skillSyncCheck bool skillSyncAdopt bool skillSyncPrune bool + skillSyncYes bool skillSyncFormat string ) @@ -29,7 +30,11 @@ var skillSyncCmd = &cobra.Command{ Missing or drifted skills are reinstalled at their pinned digest. Use --check to report drift without installing anything (suitable for CI). Use --adopt to record lock entries for existing unmanaged installs, and ---prune to remove installs no longer present in the lock file.`, +--prune to remove installs no longer present in the lock file. + +Unless --check is set, sync prompts for confirmation before installing — +skill content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI).`, PreRunE: chainPreRunE( ValidateFormat(&skillSyncFormat), ), @@ -49,6 +54,8 @@ func init() { "Write lock entries for existing unmanaged project-scope installs") skillSyncCmd.Flags().BoolVar(&skillSyncPrune, "prune", false, "Remove installs no longer present in the lock file") + skillSyncCmd.Flags().BoolVar(&skillSyncYes, "yes", false, + "Skip the confirmation prompt (required when not running interactively)") AddFormatFlag(skillSyncCmd, &skillSyncFormat) } @@ -58,6 +65,17 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { return err } + if !skillSyncCheck { + confirmed, confirmErr := requireConfirmation("Sync skills for "+projectRoot, skillSyncYes) + if confirmErr != nil { + return confirmErr + } + if !confirmed { + fmt.Println("Sync cancelled.") + return nil + } + } + c := newSkillClient(cmd.Context()) result, err := c.Sync(cmd.Context(), skills.SyncOptions{ ProjectRoot: projectRoot, @@ -70,7 +88,24 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { return formatSkillError("sync skills", err) } - return printSyncResult(result, skillSyncFormat) + if err := printSyncResult(result, skillSyncFormat); err != nil { + return err + } + return syncExitError(result, skillSyncCheck) +} + +// syncExitError maps a SyncResult to RFC THV-0080's exit-code contract. +// Partial failure takes precedence over check-mode drift: something +// actually going wrong is a stronger signal than the project merely not +// matching its lock file. +func syncExitError(result *skills.SyncResult, check bool) error { + if len(result.Failed) > 0 { + return withExitCode(fmt.Errorf("sync failed for %d skill(s)", len(result.Failed)), ExitCodePartialFailure) + } + if check && len(result.Drifted) > 0 { + return withExitCode(fmt.Errorf("%d skill(s) drifted from the lock file", len(result.Drifted)), ExitCodeCheckFailure) + } + return nil } func printSyncResult(result *skills.SyncResult, format string) error { diff --git a/cmd/thv/app/skill_sync_test.go b/cmd/thv/app/skill_sync_test.go new file mode 100644 index 0000000000..9ae9e507f1 --- /dev/null +++ b/cmd/thv/app/skill_sync_test.go @@ -0,0 +1,98 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills" +) + +func TestSyncExitError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + result *skills.SyncResult + check bool + wantCode int + }{ + {name: "clean sync", result: &skills.SyncResult{AlreadyCurrent: []string{"a"}}, wantCode: 0}, + { + name: "check finds drift", + result: &skills.SyncResult{Drifted: []string{"a"}}, + check: true, + wantCode: ExitCodeCheckFailure, + }, + { + name: "non-check drift is not a failure (already reinstalled)", + result: &skills.SyncResult{Drifted: []string{"a"}, Installed: []string{"a"}}, + check: false, + wantCode: 0, + }, + { + name: "any failure is a partial failure", + result: &skills.SyncResult{Failed: []skills.SyncFailure{{Name: "a", Error: "boom"}}}, + wantCode: ExitCodePartialFailure, + }, + { + name: "failure takes precedence over check drift", + result: &skills.SyncResult{ + Drifted: []string{"a"}, + Failed: []skills.SyncFailure{{Name: "b", Error: "boom"}}, + }, + check: true, + wantCode: ExitCodePartialFailure, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := syncExitError(tt.result, tt.check) + assert.Equal(t, tt.wantCode, ExitCodeFromError(err)) + }) + } +} + +func TestIsSyncResultEmpty(t *testing.T) { + t.Parallel() + + assert.True(t, isSyncResultEmpty(&skills.SyncResult{})) + assert.False(t, isSyncResultEmpty(&skills.SyncResult{Installed: []string{"a"}})) + assert.False(t, isSyncResultEmpty(&skills.SyncResult{Failed: []skills.SyncFailure{{Name: "a"}}})) +} + +func TestPrintSyncResultJSON(t *testing.T) { + t.Parallel() + err := printSyncResult(&skills.SyncResult{Installed: []string{"my-skill"}}, FormatJSON) + require.NoError(t, err) +} + +func TestPrintSyncResultText(t *testing.T) { + t.Parallel() + + t.Run("every category populated", func(t *testing.T) { + t.Parallel() + err := printSyncResult(&skills.SyncResult{ + Installed: []string{"installed-skill"}, + Drifted: []string{"drifted-skill"}, + AlreadyCurrent: []string{"current-skill"}, + NeverManaged: []string{"unmanaged-skill"}, + RemovedFromLock: []string{"removed-skill"}, + Pruned: []string{"pruned-skill"}, + Failed: []skills.SyncFailure{{Name: "failed-skill", Reason: skills.FailureReasonUnknown, Error: "boom"}}, + }, FormatText) + require.NoError(t, err) + }) + + t.Run("empty result prints nothing-to-sync", func(t *testing.T) { + t.Parallel() + err := printSyncResult(&skills.SyncResult{}, FormatText) + require.NoError(t, err) + }) +} diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go index 9fed7e2fae..c1fd61fa54 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -6,9 +6,11 @@ package app import ( "encoding/json" "fmt" + "net/http" "github.com/spf13/cobra" + "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/skills" ) @@ -18,6 +20,7 @@ var ( skillUpgradePreview bool skillUpgradeFailOnChanges bool skillUpgradeAllowRefChange bool + skillUpgradeYes bool skillUpgradeFormat string ) @@ -33,7 +36,11 @@ 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.`, +freshness gate. + +Unless --preview is set, upgrade prompts for confirmation before installing — +skill content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI).`, PreRunE: chainPreRunE( ValidateFormat(&skillUpgradeFormat), ), @@ -53,6 +60,8 @@ func init() { "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") + skillUpgradeCmd.Flags().BoolVar(&skillUpgradeYes, "yes", false, + "Skip the confirmation prompt (required when not running interactively)") AddFormatFlag(skillUpgradeCmd, &skillUpgradeFormat) } @@ -62,6 +71,17 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { return err } + if !skillUpgradePreview { + confirmed, confirmErr := requireConfirmation("Upgrade skills for "+projectRoot, skillUpgradeYes) + if confirmErr != nil { + return confirmErr + } + if !confirmed { + fmt.Println("Upgrade cancelled.") + return nil + } + } + c := newSkillClient(cmd.Context()) result, err := c.Upgrade(cmd.Context(), skills.UpgradeOptions{ ProjectRoot: projectRoot, @@ -72,10 +92,47 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { AllowRefChange: skillUpgradeAllowRefChange, }) if err != nil { - return formatSkillError("upgrade skills", err) + wrapped := formatSkillError("upgrade skills", err) + if httperr.Code(err) == http.StatusConflict { + // --fail-on-changes tripped: a CI freshness gate, not an + // operational failure. + return withExitCode(wrapped, ExitCodeCheckFailure) + } + return wrapped } - return printUpgradeResult(result, skillUpgradeFormat) + if err := printUpgradeResult(result, skillUpgradeFormat); err != nil { + return err + } + return upgradeExitError(result) +} + +// upgradeExitError maps an UpgradeResult to RFC THV-0080's exit-code +// contract. A failed outcome takes precedence over a ref-change block: +// something actually going wrong is a stronger signal than a guard doing +// its job. +func upgradeExitError(result *skills.UpgradeResult) error { + var failed, refBlocked int + for _, o := range result.Outcomes { + switch o.Status { + case skills.UpgradeStatusFailed: + failed++ + case skills.UpgradeStatusRefChangeBlocked: + refBlocked++ + case skills.UpgradeStatusUpgraded, skills.UpgradeStatusUpToDate, skills.UpgradeStatusNotUpgradable: + // No exit-code impact. + } + } + if failed > 0 { + return withExitCode(fmt.Errorf("upgrade failed for %d skill(s)", failed), ExitCodePartialFailure) + } + if refBlocked > 0 { + return withExitCode( + fmt.Errorf("%d skill(s) blocked by a reference change; use --allow-ref-change", refBlocked), + ExitCodePolicyRejection, + ) + } + return nil } func printUpgradeResult(result *skills.UpgradeResult, format string) error { diff --git a/cmd/thv/app/skill_upgrade_test.go b/cmd/thv/app/skill_upgrade_test.go new file mode 100644 index 0000000000..9acfe8ae39 --- /dev/null +++ b/cmd/thv/app/skill_upgrade_test.go @@ -0,0 +1,91 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/skills" +) + +func TestUpgradeExitError(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + outcomes []skills.UpgradeOutcome + wantCode int + }{ + { + name: "all up to date", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusUpToDate}}, + wantCode: 0, + }, + { + name: "upgraded is not a failure", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusUpgraded}}, + wantCode: 0, + }, + { + name: "not upgradable is not a failure", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusNotUpgradable}}, + wantCode: 0, + }, + { + name: "ref change blocked is a policy rejection", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusRefChangeBlocked}}, + wantCode: ExitCodePolicyRejection, + }, + { + name: "failed outcome is a partial failure", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusFailed}}, + wantCode: ExitCodePartialFailure, + }, + { + name: "failure takes precedence over ref-change-blocked", + outcomes: []skills.UpgradeOutcome{ + {Name: "a", Status: skills.UpgradeStatusRefChangeBlocked}, + {Name: "b", Status: skills.UpgradeStatusFailed}, + }, + wantCode: ExitCodePartialFailure, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := upgradeExitError(&skills.UpgradeResult{Outcomes: tt.outcomes}) + assert.Equal(t, tt.wantCode, ExitCodeFromError(err)) + }) + } +} + +func TestPrintUpgradeResultJSON(t *testing.T) { + t.Parallel() + err := printUpgradeResult(&skills.UpgradeResult{ + Outcomes: []skills.UpgradeOutcome{{Name: "my-skill", Status: skills.UpgradeStatusUpgraded}}, + }, FormatJSON) + require.NoError(t, err) +} + +func TestPrintUpgradeResultTextNoOutcomes(t *testing.T) { + t.Parallel() + err := printUpgradeResult(&skills.UpgradeResult{}, FormatText) + require.NoError(t, err) +} + +func TestPrintUpgradeResultTextEveryStatus(t *testing.T) { + t.Parallel() + err := printUpgradeResult(&skills.UpgradeResult{Outcomes: []skills.UpgradeOutcome{ + {Name: "upgraded-skill", Status: skills.UpgradeStatusUpgraded, OldDigest: "old", NewDigest: "new"}, + {Name: "current-skill", Status: skills.UpgradeStatusUpToDate}, + {Name: "pinned-skill", Status: skills.UpgradeStatusNotUpgradable}, + {Name: "blocked-skill", Status: skills.UpgradeStatusRefChangeBlocked, NewResolvedReference: "new-ref"}, + {Name: "failed-skill", Status: skills.UpgradeStatusFailed, Reason: skills.FailureReasonUnknown, Error: "boom"}, + }}, FormatText) + require.NoError(t, err) +} diff --git a/cmd/thv/main.go b/cmd/thv/main.go index fb03b8282e..0f66538d2e 100644 --- a/cmd/thv/main.go +++ b/cmd/thv/main.go @@ -81,7 +81,7 @@ func main() { if err := cmd.ExecuteContext(ctx); err != nil { // Clean up any remaining lock files on error exit lockfile.CleanupAllLocks() - os.Exit(1) + os.Exit(app.ExitCodeFromError(err)) } // Clean up lock files on normal exit diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index 83e16b8409..b857695e41 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -308,6 +308,7 @@ installed_skills table ├── client_apps (BLOB, JSONB-encoded []string) ├── status (installed | pending | failed) ├── installed_at (TEXT, ISO 8601) +├── managed (INTEGER, 0/1 — tracked in the project's toolhive.lock.yaml; see below) └── UNIQUE(entry_id, scope, project_root) skill_dependencies table @@ -322,7 +323,86 @@ oci_tags table (reserved; not currently populated) └── digest (TEXT NOT NULL — content digest) ``` -**Implementation:** `pkg/storage/sqlite/skill_store.go`, `pkg/storage/interfaces.go` (SkillStore), `pkg/storage/sqlite/migrations/001_create_entries_and_skills.sql` +**Implementation:** `pkg/storage/sqlite/skill_store.go`, `pkg/storage/interfaces.go` (SkillStore), `pkg/storage/sqlite/migrations/001_create_entries_and_skills.sql`, `003_add_managed_flag.sql` + +## Project Lock File + +RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080-skills-lock-file.md) adds a project-level `toolhive.lock.yaml`, committed at the project root, that pins the exact content of every project-scoped skill install — the same guarantee `package-lock.json`, `Cargo.lock`, and `go.sum` provide elsewhere. Two teammates (or a CI runner) cloning the same repo restore identical skill content via `thv skill sync`, rather than whatever the source currently resolves to. + +### Rollout + +The feature is gated behind the `TOOLHIVE_SKILLS_LOCK_ENABLED` environment variable (`skills.LockFileFeatureEnabled()`) while it lands across a stack of PRs, following the existing `TOOLHIVE_DEV` precedent for staged rollouts (`pkg/skills/gitresolver/reference.go`). With the flag unset, project-scope installs behave exactly as they did before this RFC — no lock file is written, no `toolhive.requires` materialization happens, `sync`/`upgrade` refuse with a clear "experimental" error. + +### Schema + +```yaml +version: 1 +skills: + - name: code-review + version: "1.0.0" + source: code-review # exactly what the user/registry resolver requested; never rewritten + resolvedReference: ghcr.io/org/code-review:1.0.0 + digest: sha256:9f2b1e... # the pin: OCI manifest digest or git commit hash + contentDigest: sha256:a1b2c3d4... # deterministic dirhash of the materialized files, for on-disk integrity + requiredBy: [parent-skill] # present only for transitively materialized toolhive.requires deps + explicit: true # false for a dependency that was never directly installed by name +``` + +Entries are sorted by name for stable diffs. `source` is never rewritten by `sync` or `upgrade` — only `upgrade` re-resolves it, and only to decide whether newer content exists. + +**Implementation:** `pkg/skills/lockfile/` (schema, load/save, atomic writes via `os.Root`, `contentDigest` dirhash algorithm) + +### Install and Uninstall Hooks + +For project-scope installs (with the feature enabled), `skillsvc.Install`'s single existing choke point (`installAndRegister` — every dispatch path, OCI or git, direct or registry-resolved, converges there) additionally: + +1. Computes `contentDigest` from the extracted files. +2. Materializes `toolhive.requires` dependencies recursively — reading `SKILL.md` back from disk (not from the resolver's own parse, so this works uniformly across OCI and git sources), with a `Visited` set guarding cycles and `skills.MaxDependencies` bounding the whole tree. +3. Upserts the lock entry, merging `requiredBy` when a dependency is shared by multiple parents. + +**A lock-write failure fails the entire install** (rolling back the DB record, matching the existing group-registration failure/rollback pattern) — RFC THV-0080 treats "installed but unpinned" as worse than "not installed." + +`Uninstall` mirrors this: for a lock-managed skill, it removes the skill's own lock entry and cascades to any dependency that consequently loses its last requiring parent (and is not itself `explicit`), via `Lockfile.RemoveParentFromRequiredBy` — itself cycle-safe. + +**Implementation:** `pkg/skills/skillsvc/lock.go`, `pkg/skills/skillsvc/install.go`, `pkg/skills/skillsvc/uninstall.go` + +### Sync + +`thv skill sync` reconciles installed skills against the lock file: + +| Lock file vs. installed state | Outcome | +|---|---| +| Digest and contentDigest both match | Reported as up to date | +| DB record exists but digest or contentDigest differs | Drifted — reinstalled at the pinned reference (`--check`: reported only, nothing written) | +| No DB record for a locked entry | Missing — installed fresh at the pinned reference | +| Installed, `managed`, but no lock entry | Removed from lock — reported (`--prune`: uninstalled) | +| Installed, not `managed`, no lock entry | Never managed — reported (`--adopt`: lock entry written from current state) | + +Reinstalling *at the pinned reference* (never re-resolving `source`) uses `buildPinnedReference` (`pkg/skills/skillsvc/pin.go`) to rewrite the entry's `resolvedReference` with its pinned digest substituted in (an OCI digest reference, or a git reference with the commit hash spliced in as the ref). A subtlety this surfaced: reinstalling at an *unchanged* digest — the normal case when repairing on-disk drift — would otherwise hit the install path's "same digest means content is already correct" fast path and silently skip re-extraction. An internal `InstallOptions.SyncRestore` flag bypasses that fast path specifically for this case. + +**Implementation:** `pkg/skills/skillsvc/sync.go`, `pkg/skills/skillsvc/pin.go` + +### Upgrade + +`thv skill upgrade [name...]` 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 resolved digest changed. Entries pinned to an immutable reference (an OCI digest, or a git reference already pinned to a full commit hash — `isImmutableSource`) are reported not-upgradable. `--preview` reports what would change without persisting it (an OCI preview still pulls the artifact into the local store — there's no lighter "digest only" primitive — so it is not fully side-effect-free, matching the RFC's own caveat). `--allow-ref-change` permits the resolved reference itself changing; `--fail-on-changes` gives CI a freshness gate. + +**Implementation:** `pkg/skills/skillsvc/upgrade.go` + +### CLI Confirmation and Exit Codes + +Because skill content is a set of AI-followed instructions, `sync` and `upgrade` gate real installs behind a confirmation prompt (skipped by `--check`/`--preview`, which never install anything). On a non-interactive terminal without `--yes`, the command refuses outright rather than silently proceeding. + +Exit codes follow a CI-oriented contract distinct from the generic `1` used elsewhere in the CLI: + +| Code | Meaning | +|---|---| +| `0` | Success | +| `1` | Generic/unclassified error (cobra's default) | +| `2` | Check/freshness failure — `sync --check` found drift, or `upgrade --fail-on-changes` found available changes | +| `3` | Partial failure — some, but not all, targeted skills failed | +| `4` | Policy rejection — the confirmation gate declined a non-interactive run, or `--allow-ref-change` blocked a reference change | + +**Implementation:** `cmd/thv/app/exitcode.go`, `cmd/thv/app/skill_confirm.go`, `cmd/thv/app/skill_sync.go`, `cmd/thv/app/skill_upgrade.go` ## API @@ -342,6 +422,10 @@ oci_tags table (reserved; not currently populated) | `GET` | `/builds` | List local builds | | `DELETE` | `/builds/{tag}` | Delete a local build | | `GET` | `/content` | Get a skill's SKILL.md body and file listing for a reference | +| `POST` | `/sync` | Restore project skills to match the lock file ([Project Lock File](#project-lock-file), RFC THV-0080) | +| `POST` | `/upgrade` | Re-resolve project skills and install newer pinned content ([Project Lock File](#project-lock-file), RFC THV-0080) | + +`/sync` and `/upgrade` are served only when the configured `SkillService` also implements `skills.SkillLockService` (as `skillsvc.New`'s does) — otherwise they return `501 Not Implemented`. **Implementation:** `pkg/api/v1/skills.go` @@ -366,7 +450,9 @@ thv skill ├── build [path] Build skill to OCI artifact ├── push [reference] Push built skill to registry ├── builds List locally-built OCI artifacts -└── builds remove [tag] Delete a locally-built artifact +├── builds remove [tag] Delete a locally-built artifact +├── sync Restore project skills to match the lock file +└── upgrade [name...] Re-resolve project skills and install newer pinned content ``` **Implementation:** `cmd/thv/app/skill*.go` @@ -455,6 +541,12 @@ ToolHive owns the installation lifecycle, scoping model, CLI/API interfaces, and | HTTP client | `pkg/skills/client/` | | CLI commands | `cmd/thv/app/skill*.go` | | Group integration | `pkg/groups/skills.go` | +| Lock file schema | `pkg/skills/lockfile/` | +| Lock file rollout gate | `pkg/skills/feature_gate.go` | +| Install/uninstall lock hooks | `pkg/skills/skillsvc/lock.go` | +| Sync | `pkg/skills/skillsvc/sync.go`, `pkg/skills/skillsvc/pin.go` | +| Upgrade | `pkg/skills/skillsvc/upgrade.go` | +| CLI exit codes / confirmation | `cmd/thv/app/exitcode.go`, `cmd/thv/app/skill_confirm.go` | ## Related Documentation diff --git a/docs/cli/thv_skill_sync.md b/docs/cli/thv_skill_sync.md index e4bef4a9d9..3acf4c7208 100644 --- a/docs/cli/thv_skill_sync.md +++ b/docs/cli/thv_skill_sync.md @@ -22,6 +22,10 @@ Missing or drifted skills are reinstalled at their pinned digest. Use Use --adopt to record lock entries for existing unmanaged installs, and --prune to remove installs no longer present in the lock file. +Unless --check is set, sync prompts for confirmation before installing — +skill content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI). + ``` thv skill sync [flags] ``` @@ -36,6 +40,7 @@ thv skill sync [flags] -h, --help help for sync --project-root string Project root path (default: auto-detected from the current directory) --prune Remove installs no longer present in the lock file + --yes Skip the confirmation prompt (required when not running interactively) ``` ### Options inherited from parent commands diff --git a/docs/cli/thv_skill_upgrade.md b/docs/cli/thv_skill_upgrade.md index edb5873bd3..7564cd79d6 100644 --- a/docs/cli/thv_skill_upgrade.md +++ b/docs/cli/thv_skill_upgrade.md @@ -26,6 +26,10 @@ and --allow-ref-change to permit the resolved reference itself changing --fail-on-changes evaluates the same plan and never installs: it is a CI freshness gate. +Unless --preview is set, upgrade prompts for confirmation before installing — +skill content is a set of AI-followed instructions. Pass --yes to skip the +prompt (required in non-interactive contexts such as CI). + ``` thv skill upgrade [skill-name...] [flags] ``` @@ -40,6 +44,7 @@ thv skill upgrade [skill-name...] [flags] -h, --help help for upgrade --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) + --yes Skip the confirmation prompt (required when not running interactively) ``` ### Options inherited from parent commands diff --git a/test/e2e/cli_skills_lock_test.go b/test/e2e/cli_skills_lock_test.go new file mode 100644 index 0000000000..34d78a1995 --- /dev/null +++ b/test/e2e/cli_skills_lock_test.go @@ -0,0 +1,112 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package e2e_test + +import ( + "errors" + "net/http/httptest" + "os" + "os/exec" + "path/filepath" + + "github.com/google/go-containerregistry/pkg/registry" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/stacklok/toolhive/test/e2e" +) + +// This RFC THV-0080 feature is gated behind TOOLHIVE_SKILLS_LOCK_ENABLED +// while it lands across a stack of PRs (see skills.LockFileFeatureEnabled), +// so this Describe block runs its own server with the gate turned on rather +// than sharing the default-off server other CLI skills tests use. +var _ = Describe("Skills CLI lock file exit codes (RFC THV-0080)", Label("api", "cli", "skills", "skills-lock", "e2e"), func() { + var ( + config *e2e.ServerConfig + apiServer *e2e.Server + thvConfig *e2e.TestConfig + ) + + BeforeEach(func() { + config = e2e.NewServerConfig() + config.ExtraEnv = []string{"TOOLHIVE_SKILLS_LOCK_ENABLED=true"} + apiServer = e2e.StartServer(config) + thvConfig = e2e.NewTestConfig() + }) + + thvSkillCmd := func(args ...string) *e2e.THVCommand { + fullArgs := append([]string{"skill"}, args...) + return e2e.NewTHVCommand(thvConfig, fullArgs...). + WithEnv("TOOLHIVE_API_URL=" + apiServer.BaseURL()) + } + + exitCodeOf := func(err error) int { + var exitErr *exec.ExitError + ExpectWithOffset(1, errors.As(err, &exitErr)).To(BeTrue(), "expected an *exec.ExitError, got %T: %v", err, err) + return exitErr.ExitCode() + } + + Describe("thv skill sync --check", func() { + It("exits 0 when the project matches its lock file", func() { + projectRoot := makeE2EProjectRoot() + skillName := "cli-lock-clean-skill" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A clean skill for CLI exit code testing") + + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(201)) + + stdout, _ := thvSkillCmd("sync", "--check", "--project-root", projectRoot).ExpectSuccess() + Expect(stdout).To(ContainSubstring("Up to date")) + Expect(stdout).To(ContainSubstring(skillName)) + }) + + It("exits 2 when the project has drifted from its lock file", func() { + projectRoot := makeE2EProjectRoot() + skillName := "cli-lock-drifted-skill" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A drifted skill for CLI exit code testing") + + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(201)) + + By("Deleting the installed files so the project drifts from the lock file") + skillDir := filepath.Join(projectRoot, ".claude", "skills", skillName) + Expect(os.RemoveAll(skillDir)).To(Succeed()) + + _, _, err := thvSkillCmd("sync", "--check", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred()) + Expect(exitCodeOf(err)).To(Equal(2)) + }) + }) + + Describe("thv skill sync without --yes", func() { + It("exits 4 when running non-interactively without --yes", func() { + projectRoot := makeE2EProjectRoot() + + _, _, err := thvSkillCmd("sync", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred(), "a non-interactive sync without --yes must refuse rather than proceed silently") + Expect(exitCodeOf(err)).To(Equal(4)) + }) + }) + + Describe("thv skill sync --yes", func() { + It("exits 0 and proceeds without prompting", func() { + projectRoot := makeE2EProjectRoot() + + stdout, _ := thvSkillCmd("sync", "--yes", "--project-root", projectRoot).ExpectSuccess() + Expect(stdout).To(ContainSubstring("Nothing to sync")) + }) + }) +}) From d6257076658d84077b708584dc354a30da8625d2 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Tue, 21 Jul 2026 19:28:19 +0200 Subject: [PATCH 2/3] Gate upgrade's ref-change exit code behind --preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit upgradeExitError mapped a ref-change-blocked outcome to exit 4 unconditionally, even during --preview — where nothing was actually blocked, only reported. Thread preview through, mirroring how syncExitError already gates Drifted behind --check. Also reconcile a docs contradiction about --preview's side effects, drop a stale signer-change reference in the exit-code doc comment (that guard doesn't exist until Stack 2), and add upgrade exit-code E2E coverage. --- cmd/thv/app/exitcode.go | 5 ++-- cmd/thv/app/skill_upgrade.go | 11 +++++--- cmd/thv/app/skill_upgrade_test.go | 17 +++++++++++- docs/arch/12-skills-system.md | 2 +- test/e2e/cli_skills_lock_test.go | 46 +++++++++++++++++++++++++++++++ 5 files changed, 73 insertions(+), 8 deletions(-) diff --git a/cmd/thv/app/exitcode.go b/cmd/thv/app/exitcode.go index 210152f65d..a8bfbbb236 100644 --- a/cmd/thv/app/exitcode.go +++ b/cmd/thv/app/exitcode.go @@ -19,8 +19,9 @@ const ( ExitCodePartialFailure = 3 // ExitCodePolicyRejection means the operation was refused by policy // rather than attempted and failed: a non-interactive sync/upgrade - // declined the pre-install confirmation gate without --yes, or a - // ref/signer-change guard blocked without its override flag. + // declined the pre-install confirmation gate without --yes, or the + // ref-change guard blocked without --allow-ref-change. More policy + // gates (e.g. a signer-change guard) may map here in a future stack. ExitCodePolicyRejection = 4 ) diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go index c1fd61fa54..17ba3308b8 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -104,14 +104,17 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { if err := printUpgradeResult(result, skillUpgradeFormat); err != nil { return err } - return upgradeExitError(result) + return upgradeExitError(result, skillUpgradePreview) } // upgradeExitError maps an UpgradeResult to RFC THV-0080's exit-code // contract. A failed outcome takes precedence over a ref-change block: // something actually going wrong is a stronger signal than a guard doing -// its job. -func upgradeExitError(result *skills.UpgradeResult) error { +// its job. preview gates the ref-change-blocked exit code the same way +// syncExitError gates Drifted behind --check: during --preview nothing was +// actually blocked, only reported, so a block must not exit as a policy +// rejection. +func upgradeExitError(result *skills.UpgradeResult, preview bool) error { var failed, refBlocked int for _, o := range result.Outcomes { switch o.Status { @@ -126,7 +129,7 @@ func upgradeExitError(result *skills.UpgradeResult) error { if failed > 0 { return withExitCode(fmt.Errorf("upgrade failed for %d skill(s)", failed), ExitCodePartialFailure) } - if refBlocked > 0 { + if !preview && refBlocked > 0 { return withExitCode( fmt.Errorf("%d skill(s) blocked by a reference change; use --allow-ref-change", refBlocked), ExitCodePolicyRejection, diff --git a/cmd/thv/app/skill_upgrade_test.go b/cmd/thv/app/skill_upgrade_test.go index 9acfe8ae39..e799686635 100644 --- a/cmd/thv/app/skill_upgrade_test.go +++ b/cmd/thv/app/skill_upgrade_test.go @@ -18,6 +18,7 @@ func TestUpgradeExitError(t *testing.T) { tests := []struct { name string outcomes []skills.UpgradeOutcome + preview bool wantCode int }{ { @@ -40,6 +41,12 @@ func TestUpgradeExitError(t *testing.T) { outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusRefChangeBlocked}}, wantCode: ExitCodePolicyRejection, }, + { + name: "ref change blocked during preview is not a policy rejection", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusRefChangeBlocked}}, + preview: true, + wantCode: 0, + }, { name: "failed outcome is a partial failure", outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusFailed}}, @@ -53,12 +60,20 @@ func TestUpgradeExitError(t *testing.T) { }, wantCode: ExitCodePartialFailure, }, + { + name: "failure still fails even during preview", + outcomes: []skills.UpgradeOutcome{ + {Name: "a", Status: skills.UpgradeStatusFailed}, + }, + preview: true, + wantCode: ExitCodePartialFailure, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := upgradeExitError(&skills.UpgradeResult{Outcomes: tt.outcomes}) + err := upgradeExitError(&skills.UpgradeResult{Outcomes: tt.outcomes}, tt.preview) assert.Equal(t, tt.wantCode, ExitCodeFromError(err)) }) } diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index b857695e41..01e494a4db 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -390,7 +390,7 @@ Reinstalling *at the pinned reference* (never re-resolving `source`) uses `build ### CLI Confirmation and Exit Codes -Because skill content is a set of AI-followed instructions, `sync` and `upgrade` gate real installs behind a confirmation prompt (skipped by `--check`/`--preview`, which never install anything). On a non-interactive terminal without `--yes`, the command refuses outright rather than silently proceeding. +Because skill content is a set of AI-followed instructions, `sync` and `upgrade` gate real installs behind a confirmation prompt (skipped by `--check`/`--preview`, which never write to the lock file or extracted skill directories — an OCI `--preview` still pulls the artifact to compare digests, but persists nothing). On a non-interactive terminal without `--yes`, the command refuses outright rather than silently proceeding. Exit codes follow a CI-oriented contract distinct from the generic `1` used elsewhere in the CLI: diff --git a/test/e2e/cli_skills_lock_test.go b/test/e2e/cli_skills_lock_test.go index 34d78a1995..96c1fd0dd0 100644 --- a/test/e2e/cli_skills_lock_test.go +++ b/test/e2e/cli_skills_lock_test.go @@ -14,6 +14,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/stacklok/toolhive/pkg/skills/lockfile" "github.com/stacklok/toolhive/test/e2e" ) @@ -109,4 +110,49 @@ var _ = Describe("Skills CLI lock file exit codes (RFC THV-0080)", Label("api", Expect(stdout).To(ContainSubstring("Nothing to sync")) }) }) + + Describe("thv skill upgrade --fail-on-changes", func() { + It("exits 2 when a skill would change, without installing it", func() { + projectRoot := makeE2EProjectRoot() + skillName := "cli-lock-upgrade-fail-on-changes-skill" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "The original description") + + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(201)) + + 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(200)) + repushResp := pushSkill(apiServer, ociRef) + defer repushResp.Body.Close() + Expect(repushResp.StatusCode).To(Equal(204)) + + root, err := lockfile.OpenRoot(projectRoot) + Expect(err).ToNot(HaveOccurred()) + before, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + beforeEntry, ok := before.Get(skillName) + Expect(ok).To(BeTrue()) + + _, _, err = thvSkillCmd("upgrade", "--yes", "--fail-on-changes", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred()) + Expect(exitCodeOf(err)).To(Equal(2)) + + By("Verifying nothing was actually installed before the conflict was reported") + after, err := lockfile.Load(root) + Expect(err).ToNot(HaveOccurred()) + afterEntry, ok := after.Get(skillName) + Expect(ok).To(BeTrue()) + Expect(afterEntry.Digest).To(Equal(beforeEntry.Digest), + "--fail-on-changes must not install a changed skill before reporting the conflict") + }) + }) }) From 624c7203a1a2c596814454395a5bf41e20a1b754 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Thu, 23 Jul 2026 11:27:47 +0200 Subject: [PATCH 3/3] Honor the exit-code contract under fail-on-changes and harden prompts The panel review's major: the CLI mapped any 409 unconditionally to exit 2, silently downgrading a genuine resolution failure during the CI gate from "something broke" (exit 3) to "lock is stale" (exit 2) and discarding the partial result. With the server now returning outcomes instead of a 409, exit codes derive entirely from outcomes with the correct precedence (failed > would-change > ref-blocked), sync --check counts missing installs toward exit 2, and plan-only runs print "would upgrade" instead of claiming an install happened. The confirmation prompt also gets teeth and armor: it now prints the lock entries being acted on (a bare "proceed?" gave the human gate nothing to judge), goes to stderr so --format json stays parseable, and strips non-graphic runes so a hostile directory or entry name cannot repaint the prompt with terminal escapes. Both commands are marked experimental in their help text, the architecture doc states the pre-Sigstore trust model plainly (drift detection over a repository-editable file, not verified integrity), and E2E coverage adds the missing exit-3 path and the fresh-clone exit-2 gate. --- cmd/thv/app/skill_confirm.go | 55 +++++++++++++++++++++++++- cmd/thv/app/skill_sync.go | 19 +++++++-- cmd/thv/app/skill_sync_test.go | 14 +++++++ cmd/thv/app/skill_upgrade.go | 65 +++++++++++++++++++------------ cmd/thv/app/skill_upgrade_test.go | 47 ++++++++++++++++++---- docs/arch/12-skills-system.md | 8 ++-- docs/cli/thv_skill.md | 4 +- docs/cli/thv_skill_sync.md | 5 ++- docs/cli/thv_skill_upgrade.md | 5 ++- test/e2e/cli_skills_lock_test.go | 55 ++++++++++++++++++++++++++ 10 files changed, 231 insertions(+), 46 deletions(-) diff --git a/cmd/thv/app/skill_confirm.go b/cmd/thv/app/skill_confirm.go index a0f92a2cc7..f760beabf4 100644 --- a/cmd/thv/app/skill_confirm.go +++ b/cmd/thv/app/skill_confirm.go @@ -8,8 +8,11 @@ import ( "fmt" "os" "strings" + "unicode" "golang.org/x/term" + + "github.com/stacklok/toolhive/pkg/skills/lockfile" ) // requireConfirmation enforces RFC THV-0080's pre-install confirmation gate @@ -20,18 +23,23 @@ import ( // is a set of AI-executed instructions, so unattended execution without an // explicit --yes is not an acceptable default the way it might be for a // lower-stakes operation. +// +// The prompt goes to stderr — stdout is reserved for the command's result +// (e.g. --format json output must stay machine-parseable) — and everything +// echoed into it is sanitized: a directory name carrying ANSI/OSC escapes +// must not be able to repaint the very prompt acting as the human gate. func requireConfirmation(action string, yes bool) (confirmed bool, err error) { if yes { return true, nil } if !term.IsTerminal(int(os.Stdin.Fd())) { //nolint:gosec // uintptr fits int on all supported platforms return false, withExitCode( - fmt.Errorf("%s requires confirmation; pass --yes to run non-interactively", action), + fmt.Errorf("%s requires confirmation; pass --yes to run non-interactively", sanitizeTerminal(action)), ExitCodePolicyRejection, ) } - fmt.Printf("%s? [y/N]: ", action) + fmt.Fprintf(os.Stderr, "%s? [y/N]: ", sanitizeTerminal(action)) reader := bufio.NewReader(os.Stdin) response, readErr := reader.ReadString('\n') if readErr != nil { @@ -40,3 +48,46 @@ func requireConfirmation(action string, yes bool) (confirmed bool, err error) { response = strings.TrimSpace(strings.ToLower(response)) return response == "y" || response == "yes", nil } + +// printLockEntriesSummary shows what the confirmation is actually about: +// the lock entries sync/upgrade will act on (name, source, pinned digest). +// A bare "proceed? [y/N]" tells the user nothing — a lock-file diff that +// swapped a digest or resolvedReference would sail past it unseen. Printed +// to stderr alongside the prompt; best-effort (an unreadable lock file is +// reported by the command itself, not here). +func printLockEntriesSummary(projectRoot string) { + root, err := lockfile.OpenRoot(projectRoot) + if err != nil { + return + } + lf, err := lockfile.Load(root) + if err != nil || len(lf.Skills) == 0 { + return + } + fmt.Fprintf(os.Stderr, "Lock file entries for %s:\n", sanitizeTerminal(projectRoot)) + for _, e := range lf.Skills { + fmt.Fprintf(os.Stderr, " %s %s %s\n", + sanitizeTerminal(e.Name), sanitizeTerminal(e.Source), sanitizeTerminal(shortDigest(e.Digest))) + } +} + +// shortDigest truncates a pin for display: enough hex to eyeball-compare, +// not enough to dominate the line. +func shortDigest(d string) string { + const keep = 19 // "sha256:" + 12 hex, or 19 chars of a commit hash + if len(d) <= keep { + return d + } + return d[:keep] + "…" +} + +// sanitizeTerminal strips non-graphic runes (control characters, ANSI/OSC +// escape introducers) from a string echoed to the terminal, keeping spaces. +func sanitizeTerminal(s string) string { + return strings.Map(func(r rune) rune { + if r == ' ' || unicode.IsGraphic(r) { + return r + } + return -1 + }, s) +} diff --git a/cmd/thv/app/skill_sync.go b/cmd/thv/app/skill_sync.go index 26e4d4ff75..baac271d5d 100644 --- a/cmd/thv/app/skill_sync.go +++ b/cmd/thv/app/skill_sync.go @@ -24,9 +24,12 @@ var ( var skillSyncCmd = &cobra.Command{ Use: "sync", - Short: "Restore project skills to match the lock file", + Short: "Restore project skills to match the lock file (experimental)", Long: `Restore a project's installed skills to match toolhive.lock.yaml. +Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive +server while the lock file feature rolls out. + Missing or drifted skills are reinstalled at their pinned digest. Use --check to report drift without installing anything (suitable for CI). Use --adopt to record lock entries for existing unmanaged installs, and @@ -66,6 +69,9 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { } if !skillSyncCheck { + if !skillSyncYes { + printLockEntriesSummary(projectRoot) + } confirmed, confirmErr := requireConfirmation("Sync skills for "+projectRoot, skillSyncYes) if confirmErr != nil { return confirmErr @@ -97,13 +103,18 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { // syncExitError maps a SyncResult to RFC THV-0080's exit-code contract. // Partial failure takes precedence over check-mode drift: something // actually going wrong is a stronger signal than the project merely not -// matching its lock file. +// matching its lock file. Missing counts toward the check failure exactly +// like drift — on a fresh clone every locked skill is missing, and that is +// the canonical state the CI gate exists to catch. func syncExitError(result *skills.SyncResult, check bool) error { if len(result.Failed) > 0 { return withExitCode(fmt.Errorf("sync failed for %d skill(s)", len(result.Failed)), ExitCodePartialFailure) } - if check && len(result.Drifted) > 0 { - return withExitCode(fmt.Errorf("%d skill(s) drifted from the lock file", len(result.Drifted)), ExitCodeCheckFailure) + if outOfSync := len(result.Drifted) + len(result.Missing); check && outOfSync > 0 { + return withExitCode( + fmt.Errorf("%d skill(s) drifted from or are missing against the lock file", outOfSync), + ExitCodeCheckFailure, + ) } return nil } diff --git a/cmd/thv/app/skill_sync_test.go b/cmd/thv/app/skill_sync_test.go index 9ae9e507f1..b600d1c013 100644 --- a/cmd/thv/app/skill_sync_test.go +++ b/cmd/thv/app/skill_sync_test.go @@ -28,12 +28,26 @@ func TestSyncExitError(t *testing.T) { check: true, wantCode: ExitCodeCheckFailure, }, + { + // The fresh-clone CI gate: a lock entry with no install record + // at all must fail --check exactly like drift does. + name: "check finds missing installs", + result: &skills.SyncResult{Missing: []string{"a"}}, + check: true, + wantCode: ExitCodeCheckFailure, + }, { name: "non-check drift is not a failure (already reinstalled)", result: &skills.SyncResult{Drifted: []string{"a"}, Installed: []string{"a"}}, check: false, wantCode: 0, }, + { + name: "non-check missing is not a failure (already installed)", + result: &skills.SyncResult{Missing: []string{"a"}, Installed: []string{"a"}}, + check: false, + wantCode: 0, + }, { name: "any failure is a partial failure", result: &skills.SyncResult{Failed: []skills.SyncFailure{{Name: "a", Error: "boom"}}}, diff --git a/cmd/thv/app/skill_upgrade.go b/cmd/thv/app/skill_upgrade.go index 17ba3308b8..8d1b1b7ae6 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -6,11 +6,9 @@ package app import ( "encoding/json" "fmt" - "net/http" "github.com/spf13/cobra" - "github.com/stacklok/toolhive-core/httperr" "github.com/stacklok/toolhive/pkg/skills" ) @@ -26,9 +24,12 @@ var ( var skillUpgradeCmd = &cobra.Command{ Use: "upgrade [skill-name...]", - Short: "Upgrade project skills to newer pinned content", + Short: "Upgrade project skills to newer pinned content (experimental)", Long: `Re-resolve a project's lock entries and install newer content where available. +Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive +server while the lock file feature rolls out. + 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 persisting anything (OCI @@ -71,7 +72,10 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { return err } - if !skillUpgradePreview { + if !skillUpgradePreview && !skillUpgradeFailOnChanges { + if !skillUpgradeYes { + printLockEntriesSummary(projectRoot) + } confirmed, confirmErr := requireConfirmation("Upgrade skills for "+projectRoot, skillUpgradeYes) if confirmErr != nil { return confirmErr @@ -92,44 +96,48 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { AllowRefChange: skillUpgradeAllowRefChange, }) if err != nil { - wrapped := formatSkillError("upgrade skills", err) - if httperr.Code(err) == http.StatusConflict { - // --fail-on-changes tripped: a CI freshness gate, not an - // operational failure. - return withExitCode(wrapped, ExitCodeCheckFailure) - } - return wrapped + return formatSkillError("upgrade skills", err) } - if err := printUpgradeResult(result, skillUpgradeFormat); err != nil { + if err := printUpgradeResult(result, skillUpgradeFormat, skillUpgradePreview || skillUpgradeFailOnChanges); err != nil { return err } - return upgradeExitError(result, skillUpgradePreview) + return upgradeExitError(result, skillUpgradePreview, skillUpgradeFailOnChanges) } // upgradeExitError maps an UpgradeResult to RFC THV-0080's exit-code -// contract. A failed outcome takes precedence over a ref-change block: -// something actually going wrong is a stronger signal than a guard doing -// its job. preview gates the ref-change-blocked exit code the same way -// syncExitError gates Drifted behind --check: during --preview nothing was -// actually blocked, only reported, so a block must not exit as a policy -// rejection. -func upgradeExitError(result *skills.UpgradeResult, preview bool) error { - var failed, refBlocked int +// contract, entirely from the reported outcomes. Precedence: a failed +// outcome (exit 3) beats everything — a genuine failure must never be +// masked as "lock is stale" (exit 2) or a guard doing its job (exit 4). +// With failOnChanges, any would-change outcome is the CI freshness signal +// (exit 2). A ref-change block maps to a policy rejection (exit 4) only +// when the run wasn't a preview/gate evaluation — during those nothing was +// actually blocked, only reported. +func upgradeExitError(result *skills.UpgradeResult, preview, failOnChanges bool) error { + var failed, refBlocked, wouldChange int for _, o := range result.Outcomes { switch o.Status { case skills.UpgradeStatusFailed: failed++ case skills.UpgradeStatusRefChangeBlocked: refBlocked++ - case skills.UpgradeStatusUpgraded, skills.UpgradeStatusUpToDate, skills.UpgradeStatusNotUpgradable: + wouldChange++ + case skills.UpgradeStatusUpgraded: + wouldChange++ + case skills.UpgradeStatusUpToDate, skills.UpgradeStatusNotUpgradable: // No exit-code impact. } } if failed > 0 { return withExitCode(fmt.Errorf("upgrade failed for %d skill(s)", failed), ExitCodePartialFailure) } - if !preview && refBlocked > 0 { + if failOnChanges && wouldChange > 0 { + return withExitCode( + fmt.Errorf("%d skill(s) would change; the lock file is stale", wouldChange), + ExitCodeCheckFailure, + ) + } + if !preview && !failOnChanges && refBlocked > 0 { return withExitCode( fmt.Errorf("%d skill(s) blocked by a reference change; use --allow-ref-change", refBlocked), ExitCodePolicyRejection, @@ -138,7 +146,10 @@ func upgradeExitError(result *skills.UpgradeResult, preview bool) error { return nil } -func printUpgradeResult(result *skills.UpgradeResult, format string) error { +// printUpgradeResult renders the outcomes. planOnly (a --preview or +// --fail-on-changes run) switches "upgraded" to "would upgrade": nothing +// was installed in those modes, and saying otherwise misreads the gate. +func printUpgradeResult(result *skills.UpgradeResult, format string, planOnly bool) error { if format == FormatJSON { data, err := json.MarshalIndent(result, "", " ") if err != nil { @@ -152,10 +163,14 @@ func printUpgradeResult(result *skills.UpgradeResult, format string) error { fmt.Println("No skills in the project's lock file") return nil } + upgradedVerb := "upgraded" + if planOnly { + upgradedVerb = "would upgrade" + } for _, o := range result.Outcomes { switch o.Status { case skills.UpgradeStatusUpgraded: - fmt.Printf("%s: upgraded %s -> %s\n", o.Name, o.OldDigest, o.NewDigest) + fmt.Printf("%s: %s %s -> %s\n", o.Name, upgradedVerb, o.OldDigest, o.NewDigest) case skills.UpgradeStatusUpToDate: fmt.Printf("%s: up to date\n", o.Name) case skills.UpgradeStatusNotUpgradable: diff --git a/cmd/thv/app/skill_upgrade_test.go b/cmd/thv/app/skill_upgrade_test.go index e799686635..22356e48f2 100644 --- a/cmd/thv/app/skill_upgrade_test.go +++ b/cmd/thv/app/skill_upgrade_test.go @@ -16,10 +16,11 @@ func TestUpgradeExitError(t *testing.T) { t.Parallel() tests := []struct { - name string - outcomes []skills.UpgradeOutcome - preview bool - wantCode int + name string + outcomes []skills.UpgradeOutcome + preview bool + failOnChanges bool + wantCode int }{ { name: "all up to date", @@ -68,12 +69,42 @@ func TestUpgradeExitError(t *testing.T) { preview: true, wantCode: ExitCodePartialFailure, }, + { + name: "fail-on-changes with pending upgrade is a check failure", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusUpgraded}}, + failOnChanges: true, + wantCode: ExitCodeCheckFailure, + }, + { + name: "fail-on-changes with a blocked ref change is a check failure", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusRefChangeBlocked}}, + failOnChanges: true, + wantCode: ExitCodeCheckFailure, + }, + { + name: "fail-on-changes with everything current exits clean", + outcomes: []skills.UpgradeOutcome{{Name: "a", Status: skills.UpgradeStatusUpToDate}}, + failOnChanges: true, + wantCode: 0, + }, + { + // The exit-code inversion from the panel review: a genuine + // resolution failure during the CI gate must exit 3, not be + // silently downgraded to "lock is stale" (exit 2). + name: "fail-on-changes with a genuine failure is a partial failure, not a check failure", + outcomes: []skills.UpgradeOutcome{ + {Name: "a", Status: skills.UpgradeStatusUpgraded}, + {Name: "b", Status: skills.UpgradeStatusFailed}, + }, + failOnChanges: true, + wantCode: ExitCodePartialFailure, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := upgradeExitError(&skills.UpgradeResult{Outcomes: tt.outcomes}, tt.preview) + err := upgradeExitError(&skills.UpgradeResult{Outcomes: tt.outcomes}, tt.preview, tt.failOnChanges) assert.Equal(t, tt.wantCode, ExitCodeFromError(err)) }) } @@ -83,13 +114,13 @@ func TestPrintUpgradeResultJSON(t *testing.T) { t.Parallel() err := printUpgradeResult(&skills.UpgradeResult{ Outcomes: []skills.UpgradeOutcome{{Name: "my-skill", Status: skills.UpgradeStatusUpgraded}}, - }, FormatJSON) + }, FormatJSON, false) require.NoError(t, err) } func TestPrintUpgradeResultTextNoOutcomes(t *testing.T) { t.Parallel() - err := printUpgradeResult(&skills.UpgradeResult{}, FormatText) + err := printUpgradeResult(&skills.UpgradeResult{}, FormatText, false) require.NoError(t, err) } @@ -101,6 +132,6 @@ func TestPrintUpgradeResultTextEveryStatus(t *testing.T) { {Name: "pinned-skill", Status: skills.UpgradeStatusNotUpgradable}, {Name: "blocked-skill", Status: skills.UpgradeStatusRefChangeBlocked, NewResolvedReference: "new-ref"}, {Name: "failed-skill", Status: skills.UpgradeStatusFailed, Reason: skills.FailureReasonUnknown, Error: "boom"}, - }}, FormatText) + }}, FormatText, true) require.NoError(t, err) } diff --git a/docs/arch/12-skills-system.md b/docs/arch/12-skills-system.md index 01e494a4db..9e6786e4ee 100644 --- a/docs/arch/12-skills-system.md +++ b/docs/arch/12-skills-system.md @@ -329,6 +329,8 @@ oci_tags table (reserved; not currently populated) RFC [THV-0080](https://github.com/stacklok/toolhive-rfcs/blob/main/rfcs/THV-0080-skills-lock-file.md) adds a project-level `toolhive.lock.yaml`, committed at the project root, that pins the exact content of every project-scoped skill install — the same guarantee `package-lock.json`, `Cargo.lock`, and `go.sum` provide elsewhere. Two teammates (or a CI runner) cloning the same repo restore identical skill content via `thv skill sync`, rather than whatever the source currently resolves to. +**Trust model, stated plainly:** until the RFC's Sigstore signing/verification stack lands, the lock file provides *reproducibility and drift detection over a repository-editable file* — not verified integrity. A committed digest is an unauthenticated trust root: anyone who can change `toolhive.lock.yaml` in the repository controls what `sync` installs. Reviewing lock-file diffs (especially `digest` and `resolvedReference` changes) carries the same weight as reviewing the AI-executed skill content itself. Signature verification on consume is what upgrades this from drift detection to integrity, and is why the whole feature ships gated until that half exists. + ### Rollout The feature is gated behind the `TOOLHIVE_SKILLS_LOCK_ENABLED` environment variable (`skills.LockFileFeatureEnabled()`) while it lands across a stack of PRs, following the existing `TOOLHIVE_DEV` precedent for staged rollouts (`pkg/skills/gitresolver/reference.go`). With the flag unset, project-scope installs behave exactly as they did before this RFC — no lock file is written, no `toolhive.requires` materialization happens, `sync`/`upgrade` refuse with a clear "experimental" error. @@ -384,13 +386,13 @@ Reinstalling *at the pinned reference* (never re-resolving `source`) uses `build ### Upgrade -`thv skill upgrade [name...]` 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 resolved digest changed. Entries pinned to an immutable reference (an OCI digest, or a git reference already pinned to a full commit hash — `isImmutableSource`) are reported not-upgradable. `--preview` reports what would change without persisting it (an OCI preview still pulls the artifact into the local store — there's no lighter "digest only" primitive — so it is not fully side-effect-free, matching the RFC's own caveat). `--allow-ref-change` permits the resolved reference itself changing; `--fail-on-changes` gives CI a freshness gate. +`thv skill upgrade [name...]` 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 resolved digest changed. Entries pinned to an immutable reference (an OCI digest, or a git reference already pinned to a full commit hash — `isImmutableSource`) are reported not-upgradable. `--preview` reports what would change without persisting it (an OCI preview still pulls the artifact into the local store — there's no lighter "digest only" primitive — so it is not fully side-effect-free, matching the RFC's own caveat). `--allow-ref-change` permits the resolved reference itself changing; `--fail-on-changes` gives CI a freshness gate — it evaluates the same plan, never installs, and returns the full outcome set (exit codes are derived client-side from the outcomes, so a genuine resolution failure still surfaces as a partial failure rather than "lock is stale"). **Implementation:** `pkg/skills/skillsvc/upgrade.go` ### CLI Confirmation and Exit Codes -Because skill content is a set of AI-followed instructions, `sync` and `upgrade` gate real installs behind a confirmation prompt (skipped by `--check`/`--preview`, which never write to the lock file or extracted skill directories — an OCI `--preview` still pulls the artifact to compare digests, but persists nothing). On a non-interactive terminal without `--yes`, the command refuses outright rather than silently proceeding. +Because skill content is a set of AI-followed instructions, `sync` and `upgrade` gate real installs behind a confirmation prompt (skipped by `--check`/`--preview`/`--fail-on-changes`, which never write to the lock file or extracted skill directories — an OCI `--preview` still pulls the artifact to compare digests, but persists nothing). The prompt is printed to stderr together with a summary of the lock entries being acted on (name, source, short digest) so the human gate has something concrete to judge, and everything echoed into it is stripped of non-graphic characters so a hostile directory or entry name cannot repaint the prompt with terminal escapes. On a non-interactive terminal without `--yes`, the command refuses outright rather than silently proceeding. Until Sigstore verification lands, this prompt is a speed bump, not a security boundary — see the trust-model note above. Exit codes follow a CI-oriented contract distinct from the generic `1` used elsewhere in the CLI: @@ -398,7 +400,7 @@ Exit codes follow a CI-oriented contract distinct from the generic `1` used else |---|---| | `0` | Success | | `1` | Generic/unclassified error (cobra's default) | -| `2` | Check/freshness failure — `sync --check` found drift, or `upgrade --fail-on-changes` found available changes | +| `2` | Check/freshness failure — `sync --check` found drifted or missing installs, or `upgrade --fail-on-changes` found available changes | | `3` | Partial failure — some, but not all, targeted skills failed | | `4` | Policy rejection — the confirmation gate declined a non-interactive run, or `--allow-ref-change` blocked a reference change | diff --git a/docs/cli/thv_skill.md b/docs/cli/thv_skill.md index a97336a789..2717cddc63 100644 --- a/docs/cli/thv_skill.md +++ b/docs/cli/thv_skill.md @@ -38,8 +38,8 @@ The skill command provides subcommands to manage skills. * [thv skill install](thv_skill_install.md) - Install a skill * [thv skill list](thv_skill_list.md) - List installed skills * [thv skill push](thv_skill_push.md) - Push a built skill -* [thv skill sync](thv_skill_sync.md) - Restore project skills to match the lock file +* [thv skill sync](thv_skill_sync.md) - Restore project skills to match the lock file (experimental) * [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 upgrade](thv_skill_upgrade.md) - Upgrade project skills to newer pinned content (experimental) * [thv skill validate](thv_skill_validate.md) - Validate a skill definition diff --git a/docs/cli/thv_skill_sync.md b/docs/cli/thv_skill_sync.md index 3acf4c7208..f7a78f99cd 100644 --- a/docs/cli/thv_skill_sync.md +++ b/docs/cli/thv_skill_sync.md @@ -11,12 +11,15 @@ mdx: ## thv skill sync -Restore project skills to match the lock file +Restore project skills to match the lock file (experimental) ### Synopsis Restore a project's installed skills to match toolhive.lock.yaml. +Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive +server while the lock file feature rolls out. + Missing or drifted skills are reinstalled at their pinned digest. Use --check to report drift without installing anything (suitable for CI). Use --adopt to record lock entries for existing unmanaged installs, and diff --git a/docs/cli/thv_skill_upgrade.md b/docs/cli/thv_skill_upgrade.md index 7564cd79d6..c8ef974e8d 100644 --- a/docs/cli/thv_skill_upgrade.md +++ b/docs/cli/thv_skill_upgrade.md @@ -11,12 +11,15 @@ mdx: ## thv skill upgrade -Upgrade project skills to newer pinned content +Upgrade project skills to newer pinned content (experimental) ### Synopsis Re-resolve a project's lock entries and install newer content where available. +Experimental: requires TOOLHIVE_SKILLS_LOCK_ENABLED=true on the ToolHive +server while the lock file feature rolls out. + 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 persisting anything (OCI diff --git a/test/e2e/cli_skills_lock_test.go b/test/e2e/cli_skills_lock_test.go index 96c1fd0dd0..66ca0983f2 100644 --- a/test/e2e/cli_skills_lock_test.go +++ b/test/e2e/cli_skills_lock_test.go @@ -111,6 +111,61 @@ var _ = Describe("Skills CLI lock file exit codes (RFC THV-0080)", Label("api", }) }) + Describe("thv skill sync partial failure", func() { + It("exits 3 when a pinned skill cannot be reinstalled", func() { + projectRoot := makeE2EProjectRoot() + skillName := "cli-lock-exit3-skill" + + ociRegistry := httptest.NewServer(registry.New()) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A skill whose registry will vanish") + + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(201)) + + By("Tampering with the installed files and killing the registry, so the repair reinstall must fail") + skillMD := filepath.Join(projectRoot, ".claude", "skills", skillName, "SKILL.md") + Expect(os.WriteFile(skillMD, []byte("tampered"), 0o644)).To(Succeed()) + ociRegistry.Close() + + _, _, err := thvSkillCmd("sync", "--yes", "--project-root", projectRoot).Run() + Expect(err).To(HaveOccurred(), "a failed repair must not exit 0") + Expect(exitCodeOf(err)).To(Equal(3), "an operational failure is exit 3, not a freshness signal") + }) + }) + + Describe("thv skill sync --check on a fresh clone", func() { + It("exits 2 when the lock file has entries but nothing is installed", func() { + projectRoot := makeE2EProjectRoot() + skillName := "cli-lock-freshclone-skill" + + ociRegistry := httptest.NewServer(registry.New()) + DeferCleanup(ociRegistry.Close) + ociRef := buildAndPushSkill(apiServer, ociRegistry, skillName, "A skill for the fresh-clone gate") + + installResp := installSkill(apiServer, installSkillRequest{ + Name: ociRef, Scope: "project", ProjectRoot: projectRoot, + }) + defer installResp.Body.Close() + Expect(installResp.StatusCode).To(Equal(201)) + + By("Simulating a fresh clone: the committed lock file survives, local install state does not") + lockPath := filepath.Join(projectRoot, "toolhive.lock.yaml") + lockBytes, err := os.ReadFile(lockPath) //nolint:gosec // fixed test path + Expect(err).ToNot(HaveOccurred()) + uninstallResp := uninstallScopedSkill(apiServer, skillName, projectRoot) + defer uninstallResp.Body.Close() + Expect(uninstallResp.StatusCode).To(Equal(204)) + Expect(os.WriteFile(lockPath, lockBytes, 0o644)).To(Succeed()) + + _, _, cmdErr := thvSkillCmd("sync", "--check", "--project-root", projectRoot).Run() + Expect(cmdErr).To(HaveOccurred(), "the CI gate must not green-light a checkout with nothing installed") + Expect(exitCodeOf(cmdErr)).To(Equal(2)) + }) + }) + Describe("thv skill upgrade --fail-on-changes", func() { It("exits 2 when a skill would change, without installing it", func() { projectRoot := makeE2EProjectRoot()