diff --git a/cmd/thv/app/exitcode.go b/cmd/thv/app/exitcode.go new file mode 100644 index 0000000000..a8bfbbb236 --- /dev/null +++ b/cmd/thv/app/exitcode.go @@ -0,0 +1,61 @@ +// 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 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 +) + +// 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..f760beabf4 --- /dev/null +++ b/cmd/thv/app/skill_confirm.go @@ -0,0 +1,93 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package app + +import ( + "bufio" + "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 +// 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. +// +// 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", sanitizeTerminal(action)), + ExitCodePolicyRejection, + ) + } + + fmt.Fprintf(os.Stderr, "%s? [y/N]: ", sanitizeTerminal(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 +} + +// 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_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..baac271d5d 100644 --- a/cmd/thv/app/skill_sync.go +++ b/cmd/thv/app/skill_sync.go @@ -18,18 +18,26 @@ var ( skillSyncCheck bool skillSyncAdopt bool skillSyncPrune bool + skillSyncYes bool skillSyncFormat string ) 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 ---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 +57,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 +68,20 @@ func skillSyncCmdFunc(cmd *cobra.Command, _ []string) error { return err } + if !skillSyncCheck { + if !skillSyncYes { + printLockEntriesSummary(projectRoot) + } + 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 +94,29 @@ 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. 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 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 } 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..b600d1c013 --- /dev/null +++ b/cmd/thv/app/skill_sync_test.go @@ -0,0 +1,112 @@ +// 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, + }, + { + // 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"}}}, + 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..8d1b1b7ae6 100644 --- a/cmd/thv/app/skill_upgrade.go +++ b/cmd/thv/app/skill_upgrade.go @@ -18,14 +18,18 @@ var ( skillUpgradePreview bool skillUpgradeFailOnChanges bool skillUpgradeAllowRefChange bool + skillUpgradeYes bool skillUpgradeFormat string ) 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 @@ -33,7 +37,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 +61,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 +72,20 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { return err } + if !skillUpgradePreview && !skillUpgradeFailOnChanges { + if !skillUpgradeYes { + printLockEntriesSummary(projectRoot) + } + 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, @@ -75,10 +99,57 @@ func skillUpgradeCmdFunc(cmd *cobra.Command, args []string) error { return formatSkillError("upgrade skills", err) } - return printUpgradeResult(result, skillUpgradeFormat) + if err := printUpgradeResult(result, skillUpgradeFormat, skillUpgradePreview || skillUpgradeFailOnChanges); err != nil { + return err + } + return upgradeExitError(result, skillUpgradePreview, skillUpgradeFailOnChanges) } -func printUpgradeResult(result *skills.UpgradeResult, format string) error { +// upgradeExitError maps an UpgradeResult to RFC THV-0080's exit-code +// 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++ + 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 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, + ) + } + return nil +} + +// 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 { @@ -92,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 new file mode 100644 index 0000000000..22356e48f2 --- /dev/null +++ b/cmd/thv/app/skill_upgrade_test.go @@ -0,0 +1,137 @@ +// 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 + preview bool + failOnChanges bool + 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: "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}}, + 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, + }, + { + name: "failure still fails even during preview", + outcomes: []skills.UpgradeOutcome{ + {Name: "a", Status: skills.UpgradeStatusFailed}, + }, + 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, tt.failOnChanges) + 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, false) + require.NoError(t, err) +} + +func TestPrintUpgradeResultTextNoOutcomes(t *testing.T) { + t.Parallel() + err := printUpgradeResult(&skills.UpgradeResult{}, FormatText, false) + 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, true) + 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..9e6786e4ee 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,88 @@ 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. + +**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. + +### 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 — 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`/`--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: + +| Code | Meaning | +|---|---| +| `0` | Success | +| `1` | Generic/unclassified error (cobra's default) | +| `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 | + +**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 +424,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 +452,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 +543,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.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 e4bef4a9d9..f7a78f99cd 100644 --- a/docs/cli/thv_skill_sync.md +++ b/docs/cli/thv_skill_sync.md @@ -11,17 +11,24 @@ 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 --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 +43,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..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 @@ -26,6 +29,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 +47,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..66ca0983f2 --- /dev/null +++ b/test/e2e/cli_skills_lock_test.go @@ -0,0 +1,213 @@ +// 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/pkg/skills/lockfile" + "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")) + }) + }) + + 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() + 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") + }) + }) +})