Record plugin installs in the project lock file - #6314
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6314 +/- ##
==========================================
+ Coverage 72.94% 72.99% +0.04%
==========================================
Files 742 745 +3
Lines 78236 78557 +321
==========================================
+ Hits 57070 57341 +271
+ Misses 17188 17182 -6
- Partials 3978 4034 +56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
Panel review found three transaction-boundary issues that can leave filesystem, DB, group, and lock state inconsistent. Please address the inline findings before merge.
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
1e86051 to
7a77833
Compare
github-actions
Bot
added
size/XL
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
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.
| dir, err := s.pluginInstallPath(ct, name, scope, projectRoot) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
High — path-resolution failures are treated as “this client was not previously installed.” WithClientManager is explicitly optional, but pluginInstallPath requires it. When it is absent (or rejects one client), this continue produces no backup. A later rollback then classifies that existing client as newly added and dematerializes it, turning a lock/group failure into data loss. Please return this error and abort before mutation; only os.ErrNotExist from a successfully resolved path should mean “no prior tree.”
There was a problem hiding this comment.
Fixed. snapshotClientTrees now returns if pluginInstallPath fails (no client manager, rejected client). Only os.ErrNotExist after a successful path resolve is treated as “no prior tree.” Upgrade tests that omit a client manager now fail before mutation. c70b9bc13.
| return &plugins.InstallResult{ | ||
| Plugin: pl, | ||
| RestoreFiles: func(ctx context.Context) { | ||
| _ = s.restoreClientTrees(ctx, opts.Name, scope, opts.ProjectRoot, backups, allClients) | ||
| }, |
There was a problem hiding this comment.
High — the new compensation path still reports success restoring metadata when file restoration failed. restoreDir removes the whole live tree before rewriting it, so a write/registration error can leave the plugin missing or partial. This closure discards that error, and rollbackInstall also ignores DB/group/lock compensation errors; the caller receives only the original lock failure while the restored DB/lock claims corrupt files are installed. Make RestoreFiles return an error and join every compensation failure into the returned install/uninstall error (the DB-delete path in uninstall.go currently discards restoreClientTrees too).
There was a problem hiding this comment.
Fixed. RestoreFiles now returns error. rollbackInstall joins compensation failures with the original error, uninstall store.Delete failure joins restoreClientTrees errors, and materializeForClients joins dematerializeAll on a later-client failure. c70b9bc13.
Project-scope installs must pin plugins: in toolhive.lock.yaml, and a lock-write failure must roll back the install so nothing is left silently unpinned. Gated until sync, upgrade, and Sigstore land.
Hold the per-plugin lock across materialize, DB, group, and lock-file writes so uninstall cannot race. Restore on-disk trees and lock entries when a later step fails, matching the AC that rollback undoes DB and dematerialization together.
Group membership, executable modes, and client marketplace registration must come back with the files so a failed lock write cannot leave a half-installed plugin.
A missing client path must abort before mutation, and every compensation failure has to travel with the original error.
7a77833 to
c70b9bc
Compare
github-actions
Bot
added
size/XL
3 participants
Summary
plugins:key is only useful if project-scope installs actually pin intotoolhive.lock.yaml, and a failed lock write must not leave an install silently unpinned.TOOLHIVE_PLUGINS_LOCK_ENABLEDso plugin entries stay out of the live skills trust document until verification is wired.Installupserts aplugins:entry (source,resolvedReference,digest,contentDigest) and marks the store recordmanaged. Lock-write failure rolls back the DB record (restore pre-existing, otherwise delete).Uninstallof a managed plugin removes the lock entry first; a lock-write failure aborts while the install is still intact.contentDigestis the frozen skills dirhash over the canonical plugin tree (in-memory ExtractPlugin file set), notmarketplace.json/settings.json.requiresis not materialized;requiredByis unused in this v1.Part of #6300. Stack 3/5 — schema → lock-service → install-hooks → sync → upgrade.
Type of change
Test plan
./pkg/pluginsand./pkg/plugins/pluginsvcwith the Taskfile race/ldflagsflags; lock tests cover record, gate-off, user-scope, skills-key isolation, lock-write rollback, pre-existing restore, uninstall, and uninstall abort-before-destroy)task lint-fix)Does this introduce a user-facing change?
No by default — the feature is inert unless
TOOLHIVE_PLUGINS_LOCK_ENABLED=true. With the gate on, project-scopethv ai-plugin install/uninstallwrite and removeplugins:entries intoolhive.lock.yaml.Implementation plan
Approved implementation plan (PR3 slice)
Mirror skills PR3 (
#5894) at theinstallAndRegisterchoke point:LockSourceas the caller's original name before internal resolution.contentDigestfrom the layer file set (same files ExtractPlugin writes) before recording.PreExistingor deletes the new record.plugins:entry first (no requires cascade).SyncRestoreyet — that bypass ofisExtractionNoOplands with sync (PR4).Special notes for reviewers
requiresmaterialization and no uninstall cascade — agreed v1: lock only explicit plugin installs.isExtractionNoOphas a comment that PR4'sSyncRestorewill need to bypass it for on-disk drift repair at the same digest.LockSource/LockResolvedReferenceare onInstallOptionsnow so sync/upgrade can preserve the original source without a follow-up options change.