Add thv skill upgrade: re-resolve pinned skills to newer content

[samuv]

Summary

• Why: RFC THV-0080 needs a controlled way to pull newer content for skills pinned by source (a registry name, an OCI tag, a git branch), without silently drifting the lock file on every plain re-install. This ships the full upgrade vertical slice in one PR, matching PR4's precedent (the CLI command can't exist without its API endpoint).
• What:
  • Upgrade re-resolves each targeted entry's Source — via the same git/OCI/registry dispatch order Install uses (resolveLatestState), stopping short of extraction/DB/lock writes — and installs newer content only when the digest actually changed. Source itself is never rewritten.
  • Entries pinned to an immutable reference (an OCI digest or a full git commit hash — isImmutableSource, added in PR4's pin.go) report not-upgradable: there's nothing newer to resolve to.
  • --preview reports what would change without persisting it (still fetches to compare digests — an OCI pull has no lighter "digest only" primitive, matching the RFC's own "preview is not side-effect-free" note; git sources resolve without touching disk). --allow-ref-change permits the resolved reference itself changing. --fail-on-changes gives CI a freshness gate.
  • var _ skills.SkillLockService = (*service)(nil) lands here now that both Sync (PR4) and Upgrade exist; SkillsRouter widens from PR4's narrow skillSyncer interface to the full skills.SkillLockService.
  • thv skill upgrade [name...] CLI.

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

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 cover: up-to-date (source unchanged), installs newer content and preserves Source, preview reports without persisting, not-upgradable for an immutable source, unknown name returns 404, --fail-on-changes stops at the first change. API handler tests including the 501 path and a --fail-on-changes 409 propagation. New E2E spec: build+push v1 → install → republish newer content at the same OCI reference → upgrade --preview reports the change without touching the lock file → upgrade applies it and the lock file records the new digest with Source unchanged.

Does this introduce a user-facing change?

Not yet — same TOOLHIVE_SKILLS_LOCK_ENABLED gate as the rest of the stack. Once the full stack lands: thv skill upgrade re-resolves and updates a project's pinned skills.

Special notes for reviewers

• resolveLatestState/resolveOCILatest/resolveGitLatest intentionally duplicate a slice of Install's dispatch logic rather than refactoring Install to expose a resolve-only phase — a deeper split felt like more surgery than this PR should carry; flagging for a second opinion on whether that tradeoff is right.
• The API layer discards partial UpgradeResult outcomes when --fail-on-changes trips (only the error is returned) — a caller doesn't currently see which earlier entries already succeeded before the stop. Noting as a known simplification, not fixed here.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 64.00000% with 63 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.79%. Comparing base (f8fe482) to head (f764fed).

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/upgrade.go	63.76%	37 Missing and 13 partials ⚠️
pkg/skills/client/client.go	0.00%	12 Missing ⚠️
pkg/skills/skillsvc/pin.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                   Coverage Diff                   @@
##           skills-lock/04-sync    #5896      +/-   ##
=======================================================
- Coverage                71.80%   71.79%   -0.02%     
=======================================================
  Files                      710      711       +1     
  Lines                    72574    72746     +172     
=======================================================
+ Hits                     52113    52228     +115     
- Misses                   16727    16776      +49     
- Partials                  3734     3742       +8     

☔ 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.

[samuv]

Tracked in #5899 (RFC THV-0080 stack).

[JAORMX]

🤖 Automated panel review — PR 5/6 (thv skill upgrade)

Multi-agent review, each finding adversarially verified against the diff. Advisory. Whole stack gated behind TOOLHIVE_SKILLS_LOCK_ENABLED (off by default). Headline finding: the OCI upgrade path is broken and untested — the two bugs below ship green precisely because coverage is git-only.

🟠 Major — OCI upgrade is broken

• resolveOCILatest returns the un-qualified ref.String() (upgrade.go:~262) while install stores qualifiedOCIRef(ref) in ResolvedReference (via #5894's recordLockState). go-containerregistry's ref.String() omits the implicit :latest; qualifiedOCIRef appends it (oci.go:22-36). So for a tag-less source like ghcr.io/org/skill, the lock pins …:latest but upgrade computes … (no tag) → the guard newRef != entry.ResolvedReference && !AllowRefChange is always true whenever the digest moved → the skill is reported RefChangeBlocked and refused. That's the single most common upgrade case (implicit :latest re-pushed). Fix: return qualifiedOCIRef(ref) to match the install side.
• resolveLatestState's OCI branch omits the registry fallback Install has (upgrade.go:~216). Install treats a parsed OCI ref as isOCI, tries installFromOCI, and on failure for an ambiguous namespace/name falls back to resolveFromRegistry. resolveLatestState reproduces only the first half — so a skill installed via the (real, tested) registry-fallback path parses as isOCI, goes straight to registry.Pull, fails exactly as it did at install, and reports Failed. A skill that installs cleanly can never be upgraded. This is the "faithful duplication dropped a branch" risk you flagged for a second opinion — the panel confirms it dropped a branch.
• The entire OCI resolve/compare/pin path + RefChangeBlocked/--allow-ref-change logic is untested (upgrade_test.go). Every upgrade test uses git fixtures where ResolvedReference == Source, so the ref-change branch never fires — which is why the two bugs above weren't caught. A table test that installs an OCI/registry-resolved skill, republishes a new manifest digest, and asserts Upgraded (plus a repoint case for RefChangeBlocked with/without --allow-ref-change) would close all three.

🟡 Minor

• --preview isn't read-only for OCI (skill_upgrade.go:53 help text). It's documented "without installing anything," but resolveOCILatest does a live registry.Pull into the local blob store (no digest-only primitive). The PR body acknowledges this; the user-facing help doesn't — disk-fill / cache-poison / metadata-leak surface for a flag promising nothing installs. Say it still fetches to compare digests.
• isImmutableSource only recognizes lowercase commit hashes (pin.go:34) — [0-9a-f] while the sibling git resolver accepts [0-9a-fA-F]. An uppercase 40-hex commit source is classified mutable and needlessly re-cloned on upgrade, defeating "immutable pins are never re-resolved."
• The 409 handler discards the partial UpgradeResult (skills.go:~431, author-noted). On --fail-on-changes the API returns only the error, so neither API clients nor the CLI can see which skills are stale — degrades to "N skill(s) would change" with no names, and is inconsistent with the sync path (which returns its result before the exit-code error). Confirming this is a real gap, not acceptable. (This also feeds the exit-code precedence bug — see #5897.)
• resolveLatestState duplicates Install's git/OCI/registry dispatch with no shared source of truth (upgrade.go:~205), so the two can silently drift — which is exactly what produced the two majors above. Worth extracting a shared resolve-only phase rather than maintaining two copies.

🟢 Verified praise

• The plan-all-then-apply split makes --fail-on-changes a genuine read-only gate (evaluated before any mutation), and content-addressed pinning defeats plan-vs-apply TOCTOU. Solid supply-chain hygiene, with a dedicated multi-entry regression test.

[samuv]

The panel's headline was correct — the OCI path was broken and shipping green only because coverage was git-only. All fixed in the latest push:

• Unqualified ref.String() vs qualifiedOCIRef (major): resolveOCILatest now returns qualifiedOCIRef(ref), matching what install records in resolvedReference — a moved digest on a tag-less source is an upgrade again, not a phantom ref-change block.
• Dropped registry fallback (major): resolveLatestState now mirrors Install's branch — on a failed direct pull of an ambiguous namespace/name (per isUnambiguousOCIRef), it falls back to the registry catalogue. A skill installed through that fallback is now upgradable through it too.
• Zero OCI coverage (major): new test matrix in upgrade_oci_test.go — tag-less source upgrades (fails without the qualification fix), tagged up-to-date, ref-change blocked without / permitted with --allow-ref-change, and a registry-fallback source upgrading end-to-end.
• --fail-on-changes 409 discarding the result: redesigned to mirror sync's check mode — the gate evaluates the plan, never installs, and returns the full outcome set with a 200. API clients see exactly which skills are stale, a genuine resolution failure stays distinguishable from "would change", and exit codes derive client-side from the outcomes (Add typed exit codes and confirmation gate to skill sync/upgrade #5897). The 409 is gone from the service and the swagger contract.
• Minors: isFullCommitHash accepts uppercase hex (with test); --preview help text now says OCI sources are still fetched to compare digests; Outcomes always marshals as an array.
• Deferred: extracting a shared resolve-only dispatch between resolveLatestState and Install — agreed it's the structural fix for the drift that caused these bugs, but it's a refactor of the shared install path and belongs in its own PR. Tracked as a follow-up in Track: Skills lock file + Sigstore signing (RFC THV-0080) #5899.

[JAORMX]

🤖 Automated panel re-review on 7c01f04 → approving

All three majors are fixed and each was independently re-verified end-to-end against the real code:

• ✅ Tag-less OCI spurious RefChangeBlocked: resolveOCILatest now returns qualifiedOCIRef(ref), matching what install records (recordLockState ← qualifiedOCIRef via install_oci.go), so a moved digest on a tag-less source reports Upgraded. TestPlanUpgrade_OCITagLessSourceUpgrades fails on the old code and passes on the fix.
• ✅ Missing registry fallback: resolveLatestState now mirrors Install's isUnambiguousOCIRef → resolveFromRegistry → resolveRegistryLatest dispatch, so an ambiguous namespace/name install can upgrade. Traced identical to install.go; TestPlanUpgrade_RegistryFallbackSourceUpgrades covers it.
• ✅ Untested OCI path: new upgrade_oci_test.go (mocked RegistryClient + real store) exercises tag-less upgrade, up-to-date, ref-change blocked/allowed, and registry fallback — the branches git fixtures never touched.

Minors fixed: --preview help now discloses the OCI fetch-to-compare; isFullCommitHash accepts uppercase hex; the --fail-on-changes 409 was replaced by a 200 returning the full outcome set. Dispatch-duplication refactor reasonably deferred to #5899 (now covered by tests).

Important cross-stack note (why this is still approvable): removing the 409 means --fail-on-changes is inert in this PR in isolation — it returns outcomes with no error, so thv skill upgrade --fail-on-changes would exit 0. The non-zero exit is delivered by the CLI's upgradeExitError in stacked PR #5897, which I've re-reviewed and verified is present and correct (failed > wouldChange precedence, inversion unit test, exit-3/exit-2 E2E). So the CI gate is functional in the merged stack; it just doesn't live in this PR. Flagging so it isn't lost: the flag must not be relied on until #5897 is in the branch (and the whole feature is gated off by default regardless). LGTM.