Add PluginLockService and managed install flag - #6311
Conversation
Sync and upgrade operate on the whole lock file, not one plugin, and prune needs a durable marker to tell lock-managed installs apart from ones a user manages by hand.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6311 +/- ##
=======================================
Coverage 72.85% 72.85%
=======================================
Files 742 742
Lines 77804 77816 +12
=======================================
+ Hits 56683 56694 +11
- Misses 17145 17148 +3
+ Partials 3976 3974 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The floating # v4 tag moved off the pinned commit, so zizmor fails every PR with a ref-version-mismatch.
samuv
left a comment
There was a problem hiding this comment.
Summary: The lock-service separation and managed persistence largely mirror the established skills design cleanly. I found one API-facade gap: the aliased enum types do not expose their constants through the plugins package. Checklist — Tests: comprehensive storage/migration coverage and all CI checks pass; Docs: generated Swagger updated; Registry impact: none; Security: no regression found; Backwards compatibility: additive schema/API change, with the facade gap noted inline.
Type aliases do not carry constants, so plugin consumers still had to import pkg/skills to name those values.
Summary
plugins:lock key, not a single named plugin — a different shape thanPluginService. Lock-managed installs also need a durable marker sosync --prunecan tell lock-managed plugins apart from ones a user installed by hand.PluginLockServiceinterface (Sync,Upgrade) with generated mock, kept separate fromPluginService(mirrorsSkillLockService).SyncOptions/SyncResult/UpgradeOptions/…) so plugin callers do not importpkg/skillsfor an identical shape.InstalledPlugin.Managed+ migration 005 (installed_plugins.managed), wired through the SQLite store. Create/Update rejectManagedon user-scoped installs.Part of #6300. Stack 2/5 — schema → lock-service → install-hooks → sync → upgrade. Based on #6303 (PR1); no new consumers yet —
pluginsvcwiring lands in PR3.Type of change
Test plan
task test—./pkg/storage/sqliteand./pkg/pluginswith race detector; fulltask teststill has pre-existingpluginsvcSSRF/git-ref failures onmain, none in these packages)task lint-fix)Added: migration test proving 005 defaults existing rows to unmanaged and its Down cleanly drops the column without disturbing 002–004 tables; store-level round-trip test for
Managed(create, then flip viaUpdate); project-scope invariant test.Does this introduce a user-facing change?
Yes —
InstalledPlugingains amanagedfield, visible in the existing GET/list plugins API responses (swagger updated accordingly). No new endpoints yet. The field is alwaysfalseuntil PR3 starts recording lock-managed installs (still gated).Implementation plan
Approved implementation plan (PR2 slice)
PluginLockServicemirrorsSkillLockService. Reuse skills lock option/result types via aliases. SQLitemanagedflag oninstalled_plugins, default 0. Only ever true for project-scoped installs. No sync/upgrade implementation in this PR.Special notes for reviewers
Sync/Upgradeare not implemented onpluginsvcyet — this PR only adds the interface, types, and storage. Callers land in PR3–PR5.encodePluginJSONFieldsso adding the managed-scope check does not trip gocyclo onCreate.