Record plugin installs in the project lock file

[samuv]

Summary

• Why: RFC THV-0080's plugins: key is only useful if project-scope installs actually pin into toolhive.lock.yaml, and a failed lock write must not leave an install silently unpinned.
• What:
  • Gate the whole rollout behind TOOLHIVE_PLUGINS_LOCK_ENABLED so plugin entries stay out of the live skills trust document until verification is wired.
  • Project-scope Install upserts a plugins: entry (source, resolvedReference, digest, contentDigest) and marks the store record managed. Lock-write failure rolls back the DB record (restore pre-existing, otherwise delete).
  • Uninstall of a managed plugin removes the lock entry first; a lock-write failure aborts while the install is still intact.
  • contentDigest is the frozen skills dirhash over the canonical plugin tree (in-memory ExtractPlugin file set), not marketplace.json / settings.json.
  • Plugin requires is not materialized; requiredBy is unused in this v1.

Part of #6300. Stack 3/5 — schema → lock-service → install-hooks → sync → upgrade.

Type of change

• New feature

Test plan

• Unit tests (./pkg/plugins and ./pkg/plugins/pluginsvc with the Taskfile race/ldflags flags; lock tests cover record, gate-off, user-scope, skills-key isolation, lock-write rollback, pre-existing restore, uninstall, and uninstall abort-before-destroy)
• Linting (task lint-fix)

Does this introduce a user-facing change?

Mostly gated, partly live:

• Gated behind TOOLHIVE_PLUGINS_LOCK_ENABLED=true: lock-entry recording and the content digest. With the gate on, project-scope thv ai-plugin install / uninstall write and remove plugins: entries in toolhive.lock.yaml.
• Live by default: the snapshot/restore rollback machinery (keyed off a configured ClientManager, not the gate), group-membership compensation, and the uninstall reordering — group removal now runs before the DB delete, so a group-backend failure aborts uninstall retryably where it previously succeeded with leaked memberships.
• Breaking for out-of-tree adapters: MaterializationAdapter gains two required methods, EnsureRegistered and Health (everything in-tree is updated). Should be called out in release notes.

Implementation plan

Approved implementation plan (PR3 slice)

Mirror skills PR3 (#5894) at the installAndRegister choke point:

• Capture LockSource as the caller's original name before internal resolution.
• Compute contentDigest from the layer file set (same files ExtractPlugin writes) before recording.
• Fail the entire install on lock-write error; rollback restores PreExisting or deletes the new record.
• Uninstall removes the plugins: entry first (no requires cascade).
• Do not add SyncRestore yet — that bypass of isExtractionNoOp lands with sync (PR4).

Special notes for reviewers

• No requires materialization and no uninstall cascade — agreed v1: lock only explicit plugin installs.
• isExtractionNoOp has a comment that PR4's SyncRestore will need to bypass it for on-disk drift repair at the same digest.
• LockSource / LockResolvedReference are on InstallOptions now so sync/upgrade can preserve the original source without a follow-up options change.

[codecov]

Codecov Report

❌ Patch coverage is 67.09559% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.05%. Comparing base (7af27d3) to head (9788c10).

Files with missing lines	Patch %	Lines
pkg/plugins/pluginsvc/install_extraction.go	65.36%	37 Missing and 25 partials ⚠️
pkg/plugins/adapters/claudecode.go	39.39%	20 Missing and 20 partials ⚠️
pkg/plugins/pluginsvc/uninstall.go	77.88%	14 Missing and 9 partials ⚠️
pkg/plugins/adapters/codex.go	21.73%	16 Missing and 2 partials ⚠️
pkg/plugins/pluginsvc/install.go	81.48%	9 Missing and 6 partials ⚠️
pkg/plugins/pluginsvc/content_digest.go	52.63%	5 Missing and 4 partials ⚠️
pkg/plugins/pluginsvc/lock.go	86.66%	3 Missing and 3 partials ⚠️
pkg/plugins/pluginsvc/install_git.go	0.00%	4 Missing ⚠️
pkg/plugins/pluginsvc/install_oci.go	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6314      +/-   ##
==========================================
- Coverage   73.08%   73.05%   -0.04%     
==========================================
  Files         745      748       +3     
  Lines       78804    79264     +460     
==========================================
+ Hits        57597    57909     +312     
- Misses      17177    17267      +90     
- Partials     4030     4088      +58     

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

Panel review found three transaction-boundary issues that can leave filesystem, DB, group, and lock state inconsistent. Please address the inline findings before merge.

[JAORMX]

The original three findings were addressed, but the follow-up panel found two remaining rollback hazards: unresolved client paths can be mistaken for absent installs, and compensation failures are still discarded after destructive replacement.

[JAORMX]

Two merge-blocking rollback gaps remain in the current head.

[JAORMX]

Re-reviewed commit 098d44a against the current PR diff. The compensation fixes are directionally correct, but the new unconditional snapshot failure path regresses unmanaged uninstall and is failing the unit suite. Requesting changes for the inline blocker.

[samuv]

Update

a48dffed6 closes the remaining install/uninstall transaction gaps:

• Unmanaged uninstall skips unused tree snapshots (fixes client-manager regression)
• Managed uninstall requires a materializer for every stored client
• Fresh installs snapshot+restore pre-existing trees on rollback
• Prior lock-entry snapshot is fail-closed; group cleanup before DB delete

env -u TOOLHIVE_DEV go test ./pkg/plugins/pluginsvc/ -count=1 passes.

[samuv]

Rebased onto current main (8343851e8). All previously unanswered review threads have replies and are marked resolved — please take another look when convenient.

[JAORMX]

Re-reviewed commits after 098d44a against the current main-based diff. Two rollback gaps remain.

[jhrozek]

Went through this one carefully. Overall the transactional design is a real improvement over both what's on main and what skillsvc does today, and the commit-by-commit progression is easy to follow. Build, vet and tests are green here.

Leaving this as a comment rather than blocking, since nothing I found will bite a user in normal operation. Two things I'd like to see addressed before merge though, and both are cheap:

1. restoreClientTrees can re-register a plugin whose file restore just failed (inline below, with a suggestion).
2. TestInstallProjectScope_RollbackRestoresPreExistingState doesn't actually test the thing it's named for. The chmod prevents the lock write from ever happening, so the restore assertion passes trivially.

Three broader notes that don't fit on a line:

The PR description says the change is inert without TOOLHIVE_PLUGINS_LOCK_ENABLED, and that isn't quite right. The gate only covers lock-entry recording and the content digest (install.go:316, install_extraction.go:673). The snapshot/restore machinery, group-membership rollback, the uninstall reordering and requireMaterializers are all live by default, since materializeAndPersist keys snapshotting off s.clientManager != nil with no feature check. Worth correcting in the description, because it changes how much of this needs scrutiny now versus when the gate flips.

docs/arch/14-plugins-system.md needs an update. Three spots are stale now: the adapter box at lines 28-29 doesn't list EnsureRegistered/Health; section 3 still says only "Failure rolls back already-materialized clients"; and section 4 describes the old unconditional dematerialize / delete record / remove from groups flow, which is exactly what this replaces for managed installs. #6135 and #5782 both updated that doc, so there's precedent.

On divergence from skillsvc: the lock-recording halves match closely, but the failure semantics are now opposite designs. Skills discards rollback errors and fails forward on uninstall; plugins joins them and can abort. I think the plugins model is the better one, but the "Mirror of skillsvc.X" comments in install.go, install_oci.go, install_git.go and install_extraction.go are misleading now. Either qualify them with where and why they diverge, or file a follow-up to converge skills. Related: skills has an errLockWrite sentinel that classifySyncFailure keys on, and since sync is PR4 of this stack, adding it now is cheaper than re-threading it later.

One last small thing. The prevEntry snapshot in installAndRegister calls lockfile.Load outside the file lock, then recordLockState takes it via Update. Same pattern in removeManagedLockEntry. The in-process pluginLock doesn't cover a second thv process, so there's a narrow cross-process TOCTOU. Probably fine given how plugins actually get installed, but noting it.

[samuv]

@jhrozek thanks for the careful pass — everything is addressed in ab70cbfcc:

• Both pre-merge asks: EnsureRegistered is skipped when the tree restore failed, and the pre-existing-state test now injects the failure after the lock write (drift state + beforeUpdate store hook), asserting the entry was genuinely overwritten before restore.
• Snapshot policy: unified on "degrade gracefully without a ClientManager" across install, digest upgrade, and managed uninstall; WithClientManager stays optional as documented.
• restoreDir: directories (including empty ones) are recorded and recreated, files go through fileutils.WriteContainedFile, and the no-symlinks assumption is documented on treeSnapshot.
• Cleanups: unreachable lock.go fallbacks removed (digest absence now fails loudly), RemovePluginFromAllGroups deleted, rollbackInstall slimmed to (ctx, result, rollbackParams).
• New coverage: rollback compensation error joined with the trigger, and addedToGroup == false leaving pre-existing memberships alone.
• Description: corrected the feature-gate claim (snapshot/rollback machinery, group compensation, and the uninstall reordering are live by default), and flagged the MaterializationAdapter interface widening for release notes.
• Arch doc: docs/arch/14-plugins-system.md updated — adapter interface box, install compensation, and the managed/unmanaged uninstall flows.
• skillsvc divergence: the "Mirror of skillsvc" comments now qualify that failure semantics deliberately diverge. Converging skills onto the joined-compensation model is a good follow-up; note the plugins errLockWrite sentinel already lands with sync in Add plugin lock-file sync #6316, threaded through classifySyncFailure there.
• Cross-process TOCTOU: agreed it exists and is acceptable for how plugins are installed today; the in-process lock covers the API server, and a file-level lock would be a separate change.

[jhrozek]

Re-checked everything from my earlier pass against 9788c10. All ten inline findings are addressed, plus the three from the summary — description, arch doc, and the mirror-of-skillsvc comments. CI is fully green (47/47) including E2E.

A few of the fixes came out better than what I suggested:

• Rather than duplicating the ClientManager guard, installExtractionUpgradeDigest now delegates to materializeAndPersist, which drops ~25 lines of duplicated snapshot/persist logic. I checked the semantics survive the move (resultClients/existingClients/managed/create line up with the old buildInstalledPlugin + Managed + Update).
• The unreachable contentDigest fallback became an explicit invariant error instead of just being deleted, so if the invariant ever breaks you get a clear failure rather than a silently recomputed digest.
• restoreDir picked up directory capture in walk order, a filepath.IsLocal guard, and WriteContainedFile, and the symlink assumption is now documented on treeSnapshot rather than left implicit.

On the test in particular, this is the bit that makes it real:

assert.Equal(t, validLockDigestAlt(), digestAtFailure,
    "precondition: the lock entry must have been overwritten before the injected failure")

Injecting on the second store.Update so the entry is already rewritten is exactly right, and the added RollbackCompensationErrorIsJoined and RollbackKeepsPreExistingGroupMembership cover the two gaps I'd only mentioned in passing.

Two things I raised are deliberately not in here, both fine by me: the errLockWrite sentinel (worth picking up in PR4 where sync actually needs it) and the cross-process TOCTOU on the lockfile.Load outside the file lock at install.go:330, which I still think is acceptable given how plugins get installed.

Note this doesn't clear @JAORMX's outstanding changes-requested — that needs their re-review.