Add thv skill sync: restore project skills from the lock file

[samuv]

Summary

• Why: RFC THV-0080 needs a way to restore a project's pinned skill set on any machine — a fresh clone, CI, a teammate's laptop — and to verify on-disk content still matches the lock file without installing anything (a CI integrity gate). This ships the full sync vertical slice in one PR since the CLI command can't exist without its API endpoint.
• What:
  • Sync reconciles installed skills against toolhive.lock.yaml: missing/drifted entries reinstall at the pinned reference (never re-resolving source — see buildPinnedReference), --check reports drift read-only, --adopt records lock entries for unmanaged installs, --prune removes installs no longer in the lock.
  • Found and fixed a real bug while building this: reinstalling at an unchanged pinned digest hit both the OCI and git install paths' "same digest means content is already correct" fast path, silently skipping re-extraction — exactly the drift sync exists to repair (a tampered file doesn't change the pinned commit/digest). A new internal SyncRestore option on InstallOptions bypasses that fast path in both install_extraction.go and install_git.go.
  • POST /api/v1beta/skills/sync, wired through a narrow skillSyncer interface in pkg/api/v1 so this PR doesn't need to ship an Upgrade stub (widens to skills.SkillLockService in PR5).
  • thv skill sync CLI: --check, --adopt, --prune, --clients, --project-root (auto-detected by walking up for .git when omitted).

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

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)

Table-driven drift-matrix tests cover up-to-date / drifted-and-reinstalled / missing-and-restored / check-mode-no-write / adopt / prune / feature-disabled-403; golden tests for buildPinnedReference (OCI digest-pin, git commit-pin with and without a subdir path) and isImmutableSource; API handler tests including the 501 "not supported" path when the underlying service lacks Sync. New E2E spec: install → delete files on disk → sync --check reports drift without touching anything → sync restores the files.

Does this introduce a user-facing change?

Not yet — same TOOLHIVE_SKILLS_LOCK_ENABLED gate as the rest of the stack (see PR3). Once the full stack lands: thv skill sync restores a project's skills to match its committed lock file.

Special notes for reviewers

• The SyncRestore fix (install_extraction.go, install_git.go) is the part I'd like the most scrutiny on — it's a small diff but changes behavior on the existing install fast paths, gated entirely behind an internal-only option only Sync sets.
• pkg/skills/client.Client gains Sync but does not yet assert skills.SkillLockService conformance — that lands in PR5 once Upgrade exists too.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 68.59903% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.85%. Comparing base (78b4c7d) to head (f8fe482).

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/sync.go	60.97%	34 Missing and 14 partials ⚠️
pkg/skills/client/client.go	0.00%	11 Missing ⚠️
pkg/skills/skillsvc/pin.go	82.35%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@                       Coverage Diff                        @@
##           skills-lock/03-install-hooks    #5895      +/-   ##
================================================================
- Coverage                         71.87%   71.85%   -0.02%     
================================================================
  Files                               708      710       +2     
  Lines                             72375    72574     +199     
================================================================
+ Hits                              52017    52147     +130     
- Misses                            16623    16676      +53     
- Partials                           3735     3751      +16     

☔ 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 4/6 (thv skill sync)

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

🔴 Blocker

• sync --check silently passes when a locked skill is entirely missing (sync.go:~92). Drift is only recorded inside if dbOK { result.Drifted = … }, then the code returns early on if opts.Check. A lock entry with no install record (dbOK=false) lands in no result bucket → the CLI prints "Nothing to sync" and the drift gate exits 0. SQLite state is per-machine and uncommitted, so on a fresh clone / fresh CI runner every entry is dbOK=false — meaning the RFC's canonical npm ci-style gate green-lights a checkout that is missing every locked skill. Fix: record dbOK==false as drift/missing before the Check return. No test covers missing-entry + --check (TestSync_MissingInstallIsRestored only covers the non-check path).

🟠 Major

• reinstallPinned re-resolves dependencies from mutable source during a restore (sync.go:~1681). It calls the full Install (SyncRestore=true) with no signal suppressing toolhive.requires recursion. Under the base recursion (#5894), restoring a parent reads its SKILL.md and re-installs each dep by its source string (a mutable tag/branch) with no SyncRestore/Digest. If the tag has moved, the dep is re-extracted to the new digest and its lock entry is rewritten — and if that dep sorted before its parent in the sync loop, nothing restores it. So a "deterministic restore" silently upgrades and re-pins a dependency against a mutable source, defeating the RFC's "restore at the pinned reference without re-resolving source." Suppress requires recursion on restore (each dep already has its own entry the loop restores directly). Parent+dependency restore is untested.

🟡 Minor

• classifySyncFailure maps every HTTP 500 → FailureReasonLockWriteFailed (sync.go:~214). Several install-path 500s aren't lock-write failures (e.g. git resolver is not configured, path resolver is required), so automation keying on reason == lock-write-failed gets a misleading typed reason.
• E2E decodes AlreadyCurrent from the wrong JSON key (test/e2e/api_skills_test.go:~2074): up_to_date vs the actual already_current — a latent silently-passing assertion.
• PR scope: ~12 non-test/non-generated source files (guideline is 10), and it folds the SyncRestore install fast-path fix (a behavior change to the shared Install path) into the sync feature PR. Per .claude/rules/pr-creation.md that incidental fix ideally lands as its own PR so it can be reviewed/reverted independently — worth at least an explicit size-exception note.

🟢 Verified praise

• The SyncRestore fast-path bypass (your most-wanted-scrutiny item) is correctly scoped: it forces full re-extraction only for sync-restore and does not regress normal installs. Verified clean in both install_extraction.go and install_git.go.

[samuv]

The blocker was real and is fixed, along with the restore-path major — plus a latent bug the panel's trace led us to. In the latest push:

• 🔴 sync --check green-lighting a fresh clone (blocker): SyncResult gains a Missing bucket, recorded for every lock entry with no install record before the check return — so the fresh-clone/CI-runner state (lock committed, per-machine SQLite empty) now fails the gate. syncExitError counts missing toward exit 2 (on Add typed exit codes and confirmation gate to skill sync/upgrade #5897), --check unit test + a fresh-clone CLI E2E (exit 2) cover it.
• Restore re-resolving dependencies from mutable source (major): recordLockState now skips toolhive.requires materialization on SyncRestore — every dependency has its own pinned entry the sync loop restores directly. The regression test orders the dependency before its parent in the lock file (the case where nothing later in the loop repairs the damage) and moves the dep's upstream: the pin survives.
• Bonus bug found while verifying that fix: a restore promoted non-explicit dependencies to explicit: true (recordLockEntry derives Explicit from RequiredByParent == "", and a restore has no parent) — permanently exempting them from cascade removal. Restores now preserve the existing entry's Explicit verbatim (TestSync_RestorePreservesExplicitFlag).
• Single-client verification: entryMatchesInstalled now verifies the contentDigest of every client directory, so tampering with a non-first client's copy is reported as drift regardless of install order (TestSync_CheckDetectsTamperInAnyClientDir).
• 500 → lock-write-failed overreach: lock-write failures now carry a sentinel error and are classified via errors.Is; unmatched 500s fall through to unknown.
• E2E decoding the stale up_to_date key: fixed — the assertion is live again.

[JAORMX]

🤖 Automated panel re-review on a954095 → approving

The blocker and the major are both fixed and adversarially confirmed against the actual code:

• ✅ 🔴 Blocker (sync --check false-pass on fresh clone): a new SyncResult.Missing bucket is recorded before the if opts.Check return, surfaced in the CLI ("Missing (not installed)") and counted by isSyncResultEmpty. A fresh clone no longer reports "Nothing to sync". The recording defect this PR owns is gone.
• ✅ Major (restore re-resolves deps from mutable source): InstallOptions.SyncRestore is set by reinstallPinned and gates materializeDependencies in recordLockState, so a restore never re-resolves a dependency's source — each dep is restored from its own pinned entry. Verified end-to-end by TestSync_RestoreDoesNotReResolveDependencies (dep ordered before parent, upstream moved, digest asserted unchanged).
• ✅ Minors fixed: errLockWrite sentinel + errors.Is (no more blanket 500→lock-write-failed); corrected E2E already_current JSON key. Two bonus latent bugs (explicit-flag promotion on restore, single-client verification) also fixed.

Two notes (non-blocking):

1. The end-to-end exit-2 CI gate for --check lands in stacked PR #5897 (syncExitError), not here — this PR correctly fixes the recording defect it owns, and I've verified #5897 delivers the exit code. So sync --check in this PR alone still exits 0; the gate is closed once #5897 is in the stack.
2. New minor: a fresh-clone (Missing) restore without --clients forwards an empty Clients to Install, whose "empty = first client" semantics contradict SyncOptions.Clients' documented "every detected client". Since the lock records no per-entry clients, this silently narrows targets — worth expanding to all detected clients or tightening the doc. LGTM.