Repository navigation
feat: choose which VGV plugins each harness is installed from - #97
Conversation
Narrow an install to the pinned plugins it names: resolve only those, translate from them, and enable only them for Claude Code. The manifest keeps the version it recorded for a plugin a run skipped, and doctor notes a pinned plugin that was never installed instead of calling it stale.
Credit each plugin for what it contributes, so the Claude translator reads the list off the assets it already gets. The other translators keep their signatures instead of taking a plugin list none of them uses.
doctor resolved every pinned plugin, so after an install that named some of them it planned the rest as content that was missing and exited 70. It now resolves what the manifest records, and a plugin that was never installed is a note rather than an install to run. Also drop the PluginAssets copy that merge can do itself, record a distinct version in the skipped-plugin test so it can fail, and fix the install doc comment and the README example.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@vgvbot /review-pr |
|
Code review submitted. |
![unicoderbot[bot]](https://avatars.githubusercontent.com/in/3144111?s=60&v=4){kind=small}
There was a problem hiding this comment.
The --plugin selection is threaded through cleanly. PluginAssets.plugins replaces ResolvedAssets.plugins, the resolver filters with only, and Claude's enabledPlugins follows the assets. I checked the reconciler. A partial install after a full one keeps the skipped plugin's enabledPlugins key and its manifest ownership, because _deepMerge merges tables key by key and Contribution.of keeps owning the untouched table. So the README is right that uninstall still removes it. _pluginVersions also keeps the version already recorded for a skipped plugin, as intended.
Two problems remain in doctor, and both come from the plugin selection being recorded once per home:
- Its fix suggestion undoes the selection.
FixCommandnever passes--plugin. If a home was installed with--plugin vgv-wingspan, any problemdoctorfinds is fixed with plainvgv_ai install --only claude, which installs every pinned plugin. That brings back the plugins the user chose to leave out. - The recorded plugins are not per harness. Say a user does a full install for Claude, then runs
install --only codex --plugin vgv-wingspan. The manifest still records both plugins, sodoctorresolves both for Codex and reports the other plugin's Codex files as missing (stale, exit 70). That is the false alarm this PR set out to remove, in a different form.
A smaller point: a version 1 manifest that installed everything now reports each plugin as <name> <version> not installed, which isn't true for that home. The PR intends that behaviour, but the wording could be more careful.
install --plugin can leave a plugin out of one harness while another is installed from it, so a manifest that records one set for the whole home makes doctor plan both for both. It now records them per harness, plans each harness against the plugins it was installed from, and reports the versions in that harness's own section. The fix doctor prints names those plugins too, so following it cannot install one that was deliberately left out, and no combined fix is printed when the harnesses need different plugins. Manifest version 3; an older one reads as empty, as a malformed one does.
ryzizub
changed the title
|
Both review comments are addressed in c705bee. Recorded plugins are not per harness. The manifest now records plugin versions per harness (
|
|
@vgvbot /review |
|
Unknown slash command: |
|
@vgvbot /review-pr |
|
Code review submitted. |
There was a problem hiding this comment.
Both points from the last review are fixed.
- The manifest now records plugin versions per harness.
doctorgroups harnesses by the plugins they were installed from and plans each group separately. FixCommandpasses--pluginwhen a harness has a strict subset of the pinned plugins, andcombinereturns null when the fixes need different plugins.- The auditor drops the recorded versions for a harness once it has no entries left.
- There are tests where two harnesses record different plugins.
The point about how a version 1 manifest was worded no longer applies, because older manifests are no longer read.
Two problems remain:
- Upgrading loses track of every installed file (high).
decodenow matches onlyversion: 3. A version 1 or 2 manifest falls through toInstallManifest(), which drops all its entries as well as its plugins. After upgrading, the nextinstalltreats every file VGV wrote earlier as the user's. It keeps those files and reports conflicts unless--forceis passed.uninstallremoves nothing, anddoctorsays nothing is installed. The new test asserts this behaviour (expect(manifest.entries, isEmpty)). Keep reading the entries from older manifests. For the plugins, either copy the home-wide map to each harness that owns entries, or record none for them. - The Shared section's fix ignores
--plugin(medium). That section is built withoutplugins:, so itsFixCommandinstalls every pinned plugin for each harness inreadsShared. That brings back the plugins the user left out, which is the problem this PR set out to fix.
The format never shipped, so it stays version 1 with its plugin versions per harness: a plugins map that is not per harness is dropped and the entries are read as before. The Shared section now names the plugins its readers were installed from, and prints no fix at all when they differ, since one install cannot fix the skills they share without adding a plugin one of them was left without.
|
Both points from the second review are addressed in cfc0257. Upgrading loses track of every installed file. Nothing has shipped, so instead of migrating the format, the manifest is version 1 from this PR on: the plugin versions are recorded per harness, a The Shared section's fix ignores the plugin selection. |
|
@vgvbot /review-pr |
|
Code review submitted. |
There was a problem hiding this comment.
Both open points from the last review are fixed.
- Upgrade path:
decodestays at version 1 and keeps the entries. Apluginsmap that is not per harness is now dropped instead of the whole manifest. Those harnesses fall back to being checked against every pinned plugin, which matches what an earlierinstallwrote. The new test checks that the entries survive. - Shared fix:
_sharedpasses the plugin set the reader harnesses have in common. When their sets differ,ReportSection.fixreturns null, so the Shared section no longer suggests a command that would add back a plugin the user left out. Tests cover the case where the readers share a set and the case where they differ.
One new, smaller issue comes from that change. FixCommand.combine still drops null fixes with nonNulls. A null section fix used to mean "nothing to fix". Now it can also mean "no single command fixes this". As a result, the closing line "Run … to fix everything install can" can appear even though a Shared problem is not covered by it. See the inline comment.
Neither the flat document nor a plugins map that is not per harness is a format this CLI ever wrote, so the tests say what decoding does with a document it cannot read instead of naming an older format.
|
@vgvbot /review-pr |
|
Code review submitted — PR approved. |
There was a problem hiding this comment.
The only changes since my last review rename two tests in install_manifest_test.dart. The new names (a document with no version is not a manifest and keeps the entries when it cannot read the plugin versions) match what those tests check. I found nothing new.
Still open from the last review: FixCommand.combine still drops null fixes with nonNulls, and doctor_command.dart:96 still prints the result as the command that will "fix everything install can". A Shared section that needs install has a null fix when its reader harnesses were installed from different plugins. In that case, the closing command can leave the Shared problem unfixed. The fix and the test I suggested in my earlier inline comment on fix_command.dart:25 still apply.
The rest of the PR still looks correct:
- Plugin versions are recorded per harness.
doctorplans each group of harnesses that share a plugin set separately.enabledPluginslists only the installed plugins.uninstallforgets the versions of a harness it empties.
A section's fix is null both when install changes nothing it found and when its harnesses were installed from different plugins, so the closing line could promise to fix everything while leaving a shared skill out of the command. A section that install can fix but that no one command covers now leaves the report without that line.
ryzizub
changed the title
Grouping harnesses by a joined string made the plugin names part of a key format, and splitting it back assumed none of them holds a space. An EqualityMap and an EqualitySet compare the selections directly, so nothing is joined, split, or taken as the one to compare the rest with. A harness the manifest records no plugins for now carries every pinned name rather than null, which the resolver already treats the same way.
Description
vgv_ai install --plugin <name>(repeatable) narrows an install to the pinned plugins you name. Without it, every pinned plugin is installed as before.PluginAssetsnames the plugins it came from, so Claude Code'senabledPluginslists only what was installed.--plugincan leave a plugin out of one harness while another is installed from it. The format never shipped, so it stays at version 1: a plugins map that is not per harness is dropped and the entries are read as before.doctorplans each harness against the plugins it was installed from, reports their versions in that harness's own section, and notes a pinned plugin a harness wasn't installed from rather than calling it stale.installcommanddoctorprints names those plugins, so following it cannot add one that was left out. Where harnesses were installed from different plugins, that section has no fix and the report has no closing command either.Type of Change
🤖 Generated with Claude Code