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

[samuv]

Summary

• Why: 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 everywhere else. 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. This is the last PR of Stack 1.
• What:
  • cmd/thv/app/exitcode.go: exitCodeError lets a RunE carry a specific exit code back through cobra; 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 (matching the RFC's stated rationale, not the workload-upgrade command's precedent of silently skipping the prompt when non-interactive — skill content is higher-stakes).
  • sync/upgrade gain --yes, skipped automatically by --check/--preview (which never install anything). Exit-code mapping prioritizes partial failure over drift/ref-change-blocked (a real operational failure is a stronger signal than a guard doing its job).
  • docs/arch/12-skills-system.md: new "Project Lock File" section covering the rollout gate, schema, install/uninstall hooks, sync, upgrade, and the exit-code contract — plus updates to the existing storage/API/CLI tables.
  • CLI E2E coverage for exit codes 0, 2, and 4 (--check clean/drifted, non-interactive refusal, --yes bypass).

Part of the production stack for #5715 (RFC THV-0080). Stack: 6/6 (final PR of Stack 1) — lockfile → lock-service → install-hooks → sync → upgrade → cli-exitcodes. Based on #5896 (PR5).

Type of change

• New feature

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e, scoped: ginkgo --label-filter=skills-lock)
• Linting (task lint-fix)

Unit tests for ExitCodeFromError/withExitCode (including error wrapping), syncExitError/upgradeExitError precedence rules, requireConfirmation's --yes and non-interactive-refusal paths (the interactive TTY-prompt branch is untestable in a normal go test process and is left uncovered, matching the existing confirmUpgrade helper for workload upgrades — this codebase's established precedent for interactive-only code). New CLI E2E spec file drives the real thv binary against a real server process and asserts actual process exit codes via *exec.ExitError.

Also verified against a clean origin/main worktree that a batch of 24 unrelated skills E2E failures on this dev machine (extracting skill: target path validation: symlink found at "/var") are a pre-existing macOS-local environment quirk (/var → /private/var), not something this stack introduced.

Does this introduce a user-facing change?

Not yet — same TOOLHIVE_SKILLS_LOCK_ENABLED gate as the rest of the stack. Once the full RFC v1 (this stack + the later Sigstore stack) lands: thv skill sync/upgrade will prompt for confirmation before installing and use the documented exit codes for scripting.

Special notes for reviewers

• This PR intentionally does not flip the TOOLHIVE_SKILLS_LOCK_ENABLED default — per the plan agreed for this stack, the feature stays gated until the Sigstore stack (built on top of this one) also lands, since the RFC frames "v1" as lock-file and signing together.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.84%. Comparing base (9817f7e) to head (ec82025).

Additional details and impacted files

@@                    Coverage Diff                     @@
##           skills-lock/05-upgrade    #5897      +/-   ##
==========================================================
+ Coverage                   71.79%   71.84%   +0.04%     
==========================================================
  Files                         711      711              
  Lines                       72746    72746              
==========================================================
+ Hits                        52228    52261      +33     
+ Misses                      16775    16720      -55     
- Partials                     3743     3765      +22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[JAORMX]

🤖 Automated panel review — PR 6/6 (typed exit codes + confirmation gate)

Multi-agent review, each finding adversarially verified against the diff. Advisory. Whole stack gated behind TOOLHIVE_SKILLS_LOCK_ENABLED (off by default).

🟠 Major — the exit-code contract this PR exists to implement is violated on one path

• Partial failure (exit 3) is silently downgraded to check failure (exit 2) under --fail-on-changes (skill_upgrade.go:~93). The error path maps any 409 unconditionally: if httperr.Code(err) == http.StatusConflict { return withExitCode(wrapped, ExitCodeCheckFailure) }. But in #5896 a resolve failure also becomes a 409 (the FailOnChanges loop returns 409 for any status that isn't UpToDate/NotUpgradable, which includes Failed), and the partial result is discarded. So when --fail-on-changes detects a change and a skill genuinely failed (e.g. transient registry outage), CI sees exit 2 ("lock is stale, auto-open upgrade PR") instead of the RFC-mandated exit 3 ("something broke"). Your upgradeExitError precedence (failed > refBlocked) is correct — it's just bypassed by the 409 shortcut. Root cause spans #5896 (server conflating Failed with "would change" + discarding the result), but the observable inversion lives in this mapping.

🟡 Minor

• The confirmation prompt is blind (skill_confirm.go:~35). requireConfirmation prints only "Sync skills for <root>? [y/N]" — no skill names, source, resolvedReference, or digest, and it runs before the client call so nothing is shown first. The comment sells it as a security control against AI-executed instructions, and the docs equate the lock to go.sum — but with Sigstore deferred, sync installs whatever digest is committed with no verification, and the lock is attacker-editable. This is strictly less informative than the precedent it's modeled on (thv upgrade calls printUpgradeDetail() before confirmUpgrade()). A lock-diff that swaps a digest/resolvedReference (leaving source untouched for an easy line-diff wave-through) sails past the prompt. Print the change set before prompting.
• projectRoot is echoed verbatim into the TTY prompt via fmt.Printf (skill_confirm.go:~35). CWE-22 validation targets traversal, not printable-char safety; a checkout dir name containing ANSI/OSC escapes can repaint/spoof the very prompt that's meant to be the human gate. Strip/escape non-graphic runes before printing.
• No test covers exit 3 (test/e2e/cli_skills_lock_test.go). E2E asserts 0/2/4 but not partial failure — the branch with the most precedence logic and the PR's core reason to exist (and the branch broken by the major above).

🟢 Verified praise

• exitCodeError via errors.As correctly survives fmt.Errorf("…: %w") wrapping upstream — good choice.
• Refusing a non-interactive run without --yes (exit 4) instead of silently proceeding is the correct fail-closed divergence from the confirmUpgrade precedent, and it's well-tested.

---

🧭 Stack-level notes (posting here as the top of the stack)

• Gating consistency: sync/upgrade are registered in init() and ship in --help + generated docs with no "experimental / off by default" marker, while the service hard-gates them (403). Because requireConfirmation runs before the API call, a disabled-feature run exits 4 without --yes but hits the 403 path with --yes — the failure code depends on --yes. The prompt also goes to stdout, polluting --format json.
• Sigstore deferral (the strategic call): RFC THV-0080's "v1" is lock-file and signing together — precisely because "a committed digest alone is an unauthenticated trust root" was the finding that motivated adding signing. Shipping the digest half first is a reasonable delivery split, but across the stack the code/docs currently present it as delivering integrity/trust when it delivers drift detection over an attacker-editable file. Recommend landing the stack (it's gated off) but making the trust language honest until Sigstore lands, and fixing the schema forward-compat hazard (see #5892) now so the signing fields slot in without a version-skew break.
• Sequencing praise: the skillSyncer → SkillLockService interface widening, the managed-flag introduce/consume split, and the default-off posture are a genuinely clean staged rollout.

[samuv]

All findings addressed, including the stack-level notes. In the latest push:

• Exit 3 downgraded to exit 2 under --fail-on-changes (major): with Add thv skill upgrade: re-resolve pinned skills to newer content #5896's redesign (outcomes instead of a 409), the CLI's unconditional 409→exit-2 mapping is gone. upgradeExitError derives everything from outcomes with the correct precedence — any failed → exit 3 (never masked), else --fail-on-changes + would-change → exit 2, else non-preview ref-block → exit 4. The inversion case (gate run with one stale skill and one genuine failure → exit 3) has a dedicated unit test.
• Blind confirmation prompt: sync/upgrade now print the lock entries being acted on (name, source, short digest) before prompting — the human gate has something concrete to judge. Honest caveat added to the docs: until Sigstore lands this is a speed bump, not a security boundary.
• ANSI/OSC injection via projectRoot: everything echoed into the prompt (and the entry summary) is stripped of non-graphic runes.
• Prompt polluting --format json: prompt and summary go to stderr; stdout stays machine-parseable.
• No experimental marker: both commands' help now says "(experimental, requires TOOLHIVE_SKILLS_LOCK_ENABLED=true)"; CLI docs regenerated. This addresses the discoverability half of the --yes-dependent 403-vs-exit-4 note; the code-path asymmetry itself is inherent to server-side gating and goes away with the gate.
• No exit-3 E2E: added — a pinned skill whose registry vanished fails the repair and exits 3; plus a fresh-clone --check E2E (exit 2, closing the loop on Add thv skill sync: restore project skills from the lock file #5895's blocker).
• Trust-language honesty (stack-level): docs/arch/12-skills-system.md now states the pre-Sigstore trust model plainly — reproducibility and drift detection over a repository-editable file, not verified integrity; a committed digest is an unauthenticated trust root and lock-file diffs deserve the same review weight as the skill content itself.

[JAORMX]

🤖 Automated panel re-review on 61ff016 → approving

The major exit-code inversion is fixed and adversarially confirmed against the real code:

• ✅ Exit-3 no longer masked as exit-2: with #5896 now returning outcomes instead of a 409, upgradeExitError derives the exit code from result.Outcomes with failed (3) > wouldChange (2) > refBlocked precedence — checked strictly in that order, and UpgradeStatusFailed only increments failed. A genuine resolve failure during --fail-on-changes now exits 3, not 2. Covered by the explicit inversion unit test ({Upgraded, Failed} + failOnChanges → exit 3) and a wired exit-3 E2E.

Minors fixed:

• ✅ Confirmation prompt now prints per-entry name + source + short-digest (printLockEntriesSummary) before prompting.
• ✅ sanitizeTerminal strips non-graphic runes from the action string, projectRoot, and every echoed entry field — the ANSI/OSC prompt-spoofing vector is closed.
• ✅ Prompt + summary moved to stderr, so --format json stdout stays parseable; both commands' help now marks them experimental (docs regenerated).
• ✅ Exit-3 (partial failure) and fresh-clone --check exit-2 now have E2E coverage.

Nits acknowledged as by-design/inherent: the confirmation gate is CLI-only (the API server has no TTY); the disabled-feature exit code differs by --yes (a consequence of server-side gating, gone once the gate is removed). One new cosmetic nit: upgrade [name...] shows all lock entries in the confirmation summary even when args restrict the target set — consider scoping it to args. Non-blocking. LGTM.

---

That closes out the whole Stack 1 review: with 611f480/1c30244/a954095/7c01f04/61ff016 (and #5892 0a45fce), every blocker and major from the original panel is fixed and independently verified. 🎉