Record project-scope skill installs in the lock file

[samuv]

Summary

• Why: RFC THV-0080 requires every project-scoped skill install to be pinned in toolhive.lock.yaml — including transitively materialized toolhive.requires dependencies — with lock-write failure hard-failing the install (never a silently-unpinned skill). This is the largest PR in the stack; flagging it up front for extra review time.
• What:
  • Install hooks into the single existing choke point all install paths already funnel through (installAndRegister): compute contentDigest, upsert the lock entry, roll back the DB record (matching the existing group-registration rollback pattern) if the lock write fails.
  • toolhive.requires dependencies materialize recursively — a Visited set guards cycles, skills.MaxDependencies bounds the whole tree (not just one skill's declared list) — with RequiredBy merged (not overwritten) when a dependency is shared by multiple parents.
  • Uninstall cascades to orphaned, non-explicit dependencies via Lockfile.RemoveParentFromRequiredBy, itself cycle-safe.
  • The whole feature stays inert on main behind TOOLHIVE_SKILLS_LOCK_ENABLED until the full RFC v1 (this stack + the later Sigstore stack) lands — following the existing TOOLHIVE_DEV precedent (pkg/skills/gitresolver/reference.go) for staged rollouts. Every PR in the stack stays independently mergeable without exposing partial behavior to users.

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

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 coverage (86.3% for pkg/skills/skillsvc) targets the risk areas directly: contentDigest/source recorded correctly on install, dependency materialization, a dependency shared by two parents merging RequiredBy, a requires cycle terminating, the MaxDependencies tree-wide cap, lock-write failure rolling back the DB record, cascade uninstall (orphaned dependency removed, explicit dependency never cascaded, shared dependency survives one parent's removal, cascade cycle terminating). Two new E2E specs exercise the real HTTP API + thv serve subprocess + a real OCI push/install round-trip, confirming the lock file is written/removed correctly outside the unit-test harness.

Does this introduce a user-facing change?

Not yet — gated behind TOOLHIVE_SKILLS_LOCK_ENABLED, off by default. Once the full stack lands: project-scope thv skill install will write/update toolhive.lock.yaml and materialize toolhive.requires dependencies; thv skill uninstall will cascade-remove orphaned dependencies.

Special notes for reviewers

• toolhive.requires is read back from the extracted SKILL.md on disk (not from the OCI/git resolver's own parse) — this works uniformly across OCI and git sources without needing to plumb requires through every install path's return type.
• test/e2e/api_helpers.go's ServerConfig gains an ExtraEnv []string field (empty/no-op for every existing E2E test) so this PR's tests can opt into the feature flag on their own thv serve subprocess without turning it on for the whole E2E suite.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 81.93833% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.87%. Comparing base (a72e34a) to head (78b4c7d).

Files with missing lines	Patch %	Lines
pkg/skills/skillsvc/content_digest.go	46.66%	8 Missing and 8 partials ⚠️
pkg/skills/skillsvc/uninstall.go	76.19%	9 Missing and 6 partials ⚠️
pkg/skills/skillsvc/lock.go	86.84%	5 Missing and 5 partials ⚠️

Additional details and impacted files

@@                       Coverage Diff                       @@
##           skills-lock/02-lock-service    #5894      +/-   ##
===============================================================
+ Coverage                        71.78%   71.87%   +0.08%     
===============================================================
  Files                              705      708       +3     
  Lines                            72176    72375     +199     
===============================================================
+ Hits                             51815    52017     +202     
+ Misses                           16657    16623      -34     
- Partials                          3704     3735      +31     

☔ 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 3/6 (install hooks + toolhive.requires materialization)

Multi-agent review, each finding adversarially verified against the diff. Advisory. Whole stack gated behind TOOLHIVE_SKILLS_LOCK_ENABLED (off by default). This is the PR you flagged for extra review time — the panel agrees the rollback path is where the risk concentrates.

🟠 Major — dependency-tree rollback is not transactional

• A later dependency failure leaves already-installed siblings orphaned (lock.go:~504 → install.go:~230/353). materializeDependencies installs each toolhive.requires dep via a full recursive s.Install that fully commits DB record + files + its own lock entry before advancing. When a later dep fails, installAndRegister's rollback() deletes only the top-level skill's record + entry. Any earlier-succeeding sibling stays installed on disk, Managed in the DB, and pinned with RequiredBy=[parent] pointing at a parent that no longer exists — violating the RFC's "every non-explicit entry has a live parent" invariant and contradicting the hard-fail philosophy. Scenario: parent requires [depA (ok), depB (missing)] → depA leaks orphaned. The only rollback test uses a single first-failing dep, so this partial-success case is uncovered.
• Destructive rollback on --force / pre-existing entries (install.go:~353). rollback() unconditionally store.Delete + lockfile.RemoveEntry, with no guard distinguishing a record newly created by this call from a pre-existing one. On a --force re-install of an already-locked skill that just gained a requires edge, a transient dep-resolution error deletes the skill's DB record and its previously-valid lock entry — including RequiredBy relationships other parents merged onto it. A working install is destroyed by a transient network error. Consider deleting only records this call created, or a tree-wide compensation/transaction.

🟠 Major — cross-PR (uninstall side)

• Install/uninstall disagree on whether a lock-write failure is fatal → resurrection (uninstall.go:~62). Install hard-fails + rolls back; uninstall appends the removeLockEntryAndCascade error to cleanupErrs and returns after DB+files are already gone. Result: a stale lock entry for a skill absent from DB/disk → the next thv skill sync sees entry present + dbOK=false and reinstalls the "uninstalled" skill, reintroducing AI-executed instructions the user thought they removed.

🟡 Minor

• Materialized deps drop the parent's --group (lock.go:~496). depOpts omits Group, so every transitively materialized dep lands in groups.DefaultGroup even when the parent was installed into a non-default group — splitting a parent and its deps across groups.
• contentDigest is computed from only Clients[0]'s directory (content_digest.go:~20). Tampering with any other client's materialized copy (e.g. .cursor/ vs .claude/) leaves the pin matching, and which dir is protected is install-order-dependent.

🟢 Verified praise

• Cascade-uninstall edge cases are covered well (orphan cascades, shared dep survives one parent, explicit never cascaded, install+uninstall cycles proven to terminate), and the cascade is gated on the persisted Managed flag as well as the env var, so behavior stays inert and self-consistent across toggles.

(Note: ~marginally over the 400-LOC guideline but cohesive and disclosed — not a concern.)

[samuv]

The panel was right that the rollback path is where the risk concentrates — all three majors were real and are fixed in the latest push:

• Non-transactional dependency rollback: rollback of a newly-created install now reuses the uninstall cascade — the parent's entry is removed, the parent is stripped from every dependency's RequiredBy, and freshly-materialized orphans (required only by the rolled-back parent, not explicit) are fully uninstalled (DB, files, lock entry), while pre-existing dependencies with other parents or explicit installs survive. Covered by TestInstallProjectScope_SiblingDependencyRolledBackWithParent (depA succeeds, depB fails → depA fully cleaned).
• Destructive rollback of pre-existing records: install paths now thread the pre-install store record out via an internal InstallResult.PreExisting, and installAndRegister snapshots the prior lock entry before any write. Rollback restores both when the record pre-existed instead of deleting them — a --force reinstall that fails on a new dependency leaves the original digest and merged RequiredBy intact (TestInstallProjectScope_RollbackRestoresPreExistingState). Known accepted limitation: a pre-existing dep whose digest was upgraded mid-tree before a later sibling failed is not reverted to its old digest — the lock stays consistent and sync reconciles it.
• Uninstall resurrection: uninstall now removes the lock entry first and aborts with everything intact if that fails. A failure after that point leaves the opposite, safe inconsistency (installed-but-unlocked → reported as removed-from-lock, prunable) — never a stale entry that the next sync silently reinstalls (TestUninstall_LockWriteFailureAbortsBeforeDestruction).
• Deps drop --group: materialized dependencies now join the parent's group (TestInstallProjectScope_DependencyJoinsParentGroup).
• contentDigest from Clients[0] only: fixed on the sync PR (Add thv skill sync: restore project skills from the lock file #5895), where verification lives — entryMatchesInstalled now checks every client directory; the record-side comment explains why first-client is fine at install time only.
• Lock-state errors wrapped as 500: partially addressed on Add thv skill sync: restore project skills from the lock file #5895 — lock-write failures are now identified by a sentinel instead of the blanket 500 mapping, so unrelated 500s (e.g. missing resolver) no longer masquerade as lock-write-failed. Preserving arbitrary inner status codes through the wrap is left as-is for now: the sentinel fix removes the misclassification that mattered for automation.

[JAORMX]

🤖 Automated panel re-review on 1c30244 → approving

All three majors are genuinely fixed — each traced end-to-end in code and independently re-verified by an adversarial check:

• ✅ Non-transactional rollback: a fresh install that fails on a later dep now runs removeLockEntry + cascadeUninstall (the same path uninstall uses), so orphaned siblings are removed from lock + DB + disk. Covered by TestInstallProjectScope_SiblingDependencyRolledBackWithParent.
• ✅ Destructive --force rollback: InstallResult.PreExisting + a prevEntry snapshot mean rollback now restores (not deletes) a pre-existing DB record and its lock entry, preserving other parents' RequiredBy. Covered by TestInstallProjectScope_RollbackRestoresPreExistingState.
• ✅ Uninstall resurrection: uninstallOne removes the lock entry first and aborts intact on failure, before any DB/disk destruction — the resurrection window is gone. Covered by TestUninstall_LockWriteFailureAbortsBeforeDestruction.

Minors: materialized deps now join the parent's group (✅ tested); the single-Clients[0] contentDigest is reasonably deferred to sync-side verification (#5895 hashes every client dir).

One new minor to consider (non-blocking): in rollbackInstall, the pre-existing-managed-parent branch (prevEntry != nil) restores the parent and returns without cascading — so if a --force reinstall newly declared several deps and an earlier sibling succeeded before a later one failed, that sibling is left installed/pinned. This does not violate the RFC "live parent" invariant (the restored parent still exists) and sync --prune reconciles it, so it's a soft leak — worth an optional follow-up. LGTM.