Add typed exit codes and confirmation gate to skill sync/upgrade

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
+}

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())
+}

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)
+}

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")
+}

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 {