Add plugin lock-file sync - #6316
Conversation
github-actions
Bot
added
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## plugins-lock/03-install-hooks #6316 +/- ##
================================================================
Coverage ? 72.95%
================================================================
Files ? 747
Lines ? 78910
Branches ? 0
================================================================
Hits ? 57571
Misses ? 17263
Partials ? 4076 ☔ 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 one state-consistency bug plus missing CLI E2E coverage and a thin-wrapper boundary violation. Please address the inline findings before merge.
eb93233 to
735241d
Compare
github-actions
Bot
added
size/XL
735241d to
68fb269
Compare
github-actions
Bot
added
size/XL
68fb269 to
b820980
Compare
github-actions
Bot
added
size/XL
JAORMX
left a comment
JAORMX
left a comment
There was a problem hiding this comment.
The adoption rollback and CLI E2E findings were addressed. The follow-up panel found a concurrency hole in adoption compensation and incomplete Claude Code registration health checks.
| if err := recordLockEntry(pl.ProjectRoot, lockEntryInput{ | ||
| Name: pl.Metadata.Name, | ||
| Version: pl.Metadata.Version, | ||
| Source: source, | ||
| ResolvedReference: resolved, | ||
| Digest: pl.Digest, | ||
| ContentDigest: contentDigest, | ||
| }); err != nil { | ||
| return fmt.Errorf("writing lock entry: %w", errors.Join(errLockWrite, err)) | ||
| } | ||
| pl.Managed = true | ||
| if err := s.store.Update(ctx, pl); err != nil { | ||
| remErr := removeLockEntry(plugins.UninstallOptions{ | ||
| Name: pl.Metadata.Name, Scope: plugins.ScopeProject, ProjectRoot: pl.ProjectRoot, | ||
| }) |
There was a problem hiding this comment.
High — failed adoption can remove a concurrent operation’s valid lock entry. Adoption does not take the existing per-plugin mutex. If install/sync upserts the same entry after recordLockEntry but before this DB update fails, the unconditional removeLockEntry deletes the other operation’s pin while its DB row can remain managed. Please serialize adoption with s.locks, re-read state under that lock, and compensate by restoring the entry observed before adoption rather than blindly removing by name.
There was a problem hiding this comment.
Fixed. adoptPlugin now holds the per-plugin lock, re-reads the row, and snapshots the existing lock entry. If Update(Managed=true) fails, we restore that snapshot (or remove the name only when none existed). A test covers restoring a pre-existing pin. 2ebd4a847.
| mp, err := readClaudeMarketplace(claudeMarketplaceFilePath(filepath.Dir(dir))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| found := false | ||
| for _, p := range mp.Plugins { | ||
| if p.Name == req.Name { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| return fmt.Errorf("plugin %q is missing from marketplace.json", req.Name) | ||
| } | ||
|
|
||
| settingsPath := a.settingsPath(req.Scope, req.ProjectRoot) | ||
| content, err := os.ReadFile(settingsPath) // #nosec G304 -- path is a known tool config file location | ||
| if err != nil { | ||
| return fmt.Errorf("reading settings.json: %w", err) | ||
| } | ||
| root, err := parseSettings(content, settingsPath) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| enabled, ok := root["enabledPlugins"].(map[string]any) | ||
| if !ok { | ||
| return fmt.Errorf("plugin %q is not enabled in settings.json", req.Name) | ||
| } | ||
| val, ok := enabled[pluginKey(req.Name)] | ||
| if !ok { | ||
| return fmt.Errorf("plugin %q is not enabled in settings.json", req.Name) | ||
| } | ||
| if on, _ := val.(bool); !on { | ||
| return fmt.Errorf("plugin %q is disabled in settings.json", req.Name) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Medium — health can report a Claude plugin current while Claude cannot discover it. This verifies enabledPlugins and only the marketplace entry’s name. It does not validate the extraKnownMarketplaces.toolhive directory registration written by enablePluginInSettings, nor that the matching marketplace entry has the expected local source path. Removing/corrupting either leaves the plugin unavailable but sync --check passes. Please validate the complete registration tuple (marketplace source/path plus plugin name/source), not just presence by name.
There was a problem hiding this comment.
Fixed. Claude Health now requires marketplace source ./<name> plus extraKnownMarketplaces.toolhive as a directory source whose path is the marketplace root. 2ebd4a847.
Restore project plugins from the lock file via thv ai-plugin sync and POST /plugins/sync, gated by TOOLHIVE_PLUGINS_LOCK_ENABLED.
A failed DB update after writing the lock entry left the plugin untracked; remove the entry so the next sync can retry. Cover thv ai-plugin sync exit codes the same way skills lock does.
Sync must not report a plugin current when a requested client is missing or marketplace registration is gone, and must refuse to adopt a local tag that cannot be restored later.
Adoption must hold the per-plugin lock and put back the pre-existing entry if marking Managed fails.
b820980 to
2ebd4a8
Compare
github-actions
Bot
added
size/XL
2 participants
Summary
plugins:.thv ai-plugin syncandPOST /plugins/syncrestore project plugins fromtoolhive.lock.yaml(--check/--adopt/--prune, confirmation gate, exit codes 0/2/3/4).buildPinnedReference— never re-resolved from source.SyncRestorebypasses the same-digest no-op so on-disk drift is repaired.--checkhashes every client's canonical plugin directory (contentDigest), notmarketplace.json.--adoptrecords unmanaged project-scope installs without requiring--allow-unsigned(plugin Sigstore is Stack 2).TOOLHIVE_PLUGINS_LOCK_ENABLED(403 when off). Upgrade is a 501 stub until the next stack PR.Part of #6300. Stack 4/5 — schema → lock-service → install-hooks → sync → upgrade.
Type of change
Test plan
./pkg/plugins/pluginsvcsync/pin tests and./pkg/api/v1sync endpoint tests, with the Taskfile race/ldflagsflags)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,thv ai-plugin syncrestores project plugins fromtoolhive.lock.yaml.Special notes for reviewers
pin.gois copied fromskillsvcrather than extracted — agreed: reuse, don't extract.--allow-unsigned. Until Stack 2, lock validation allows an entry with neither provenance norunsigned. Requiring the flag here would make every plugin adopt fail.Upgradeon*serviceis a 501 stub soPluginLockServicetype-asserts for/sync. The next PR replaces it.cmd/thv/app/skill_confirm.goandexitcode.go.