[ryzizub]

Description

vgv_ai update only updated the binary, so installed hooks and plugins stayed on the old pins until the user reran install.

• After a successful pub update, update now runs dart pub global run vgv_ai_cli:vgv_ai install. That runs the newly activated version, which has the new pins.
• It runs only when a manifest exists, so it never installs on a machine that has not been set up with install.
• The child runs install --refresh, a new hidden flag: it reinstalls the harnesses and plugins the manifest records at the new pins, without detecting or prompting. A plugin the new version no longer pins is skipped with a warning.
• Homebrew and install-script users get a hint to run vgv_ai install after upgrading.

Type of Change

• ✨ New feature (non-breaking change which adds functionality)
• 🛠️ Bug fix (non-breaking change which fixes an issue)
• ❌ Breaking change (fix or feature that would cause existing functionality to change)
• 🧹 Code refactor
• ✅ Build configuration change
• 📝 Documentation
• 🗑️ Chore
• 🧪 Test

[marcossevilla]

/unicoder /review-pr

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

Running install from the newly activated version after a successful pub update is a sound way to move plugins to the new pins. Gating it on the manifest keeps it off machines that were never set up. Using inheritStdio lets the picker show what is already installed. Error handling and exit-code propagation are tidy, and the tests cover the main branches well (no manifest, no home, the start failure, the child's exit code, and no reinstall after a failed pub update or when already on the latest version).

One correctness problem: the child runs a bare install, so it does not reuse the choices the user made in their earlier install.

1. Harness selection is lost. A bare install runs detection again. A user who ran install --only claude or install --no-codex gets every detected harness configured, including ones they opted out of. The manifest already records plugins per harness (manifest.plugins.keys), so update could pass --only <recorded harnesses>.
2. Plugin selection is lost when stdin is not a terminal. InstallCommand._selectPlugins installs every pinned plugin when _hasTerminal is false. So vgv_ai update run from a script, in CI, or with stdin piped quietly installs plugins the user had left out. Passing --plugin for each recorded plugin when stdin is not a terminal would keep their choice. In a terminal the picker already defaults to what was recorded.

Smaller point: root flags such as --verbose are not forwarded to the child, so a verbose update runs a non-verbose install.

I suggest building the child arguments from the loaded manifest and adding tests for those arguments, for example a manifest recording only one harness.

[ryzizub]

/unicoder /review-pr

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

Thanks for working through the earlier review. Most of it is fixed:

• The reinstall now passes --only for each recorded harness, so it no longer runs detection again.
• It passes --plugin for the recorded plugins, so a run outside a terminal no longer installs every pinned plugin.
• --verbose is forwarded to the child.
• An unreadable manifest is now handled: update skips the reinstall instead of failing.
• The tests now write a real manifest and check the arguments the child gets.

The new argument building has some problems of its own:

1. The merged plugin list gives harnesses plugins they never had. Each harness records its own plugins, but --plugin applies to every harness --only names. If claude recorded only vgv-wingspan and codex recorded vgv-wingspan and vgv-ffca-plugin, the reinstall adds vgv-ffca-plugin to claude, and _pluginVersions then records it there. The test reinstalls just the recorded harnesses and plugins asserts exactly this behaviour. That contradicts the doc comment, which says the reinstall never widens the user's choices.
2. A plugin the new version no longer pins breaks the reinstall. --plugin uses allowed: vgvPlugins from the new version. If a recorded plugin was dropped or renamed, the child exits with a usage error and nothing is reinstalled.
3. A manifest with no plugin record still runs a bare install. This covers a manifest that is malformed (it decodes as empty) or one that has no plugins field. Detection runs again and, outside a terminal, every pinned plugin is installed. That is the same widening the earlier review raised. The reinstalls with the updated version after an install test expects this behaviour.

[ryzizub]

/unicoder /review-pr

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

Thanks for this revision. Moving the selection into a hidden install --refresh flag is a cleaner design than having the old binary build the arguments. The new version now decides what to reinstall against its own pin list, which fixes two of the three earlier points:

• Fixed: dropped or renamed plugins. _recordedPlugins intersects the recorded names with vgvPlugins and warns about the rest, so the reinstall no longer fails with a usage error. The skips a recorded plugin this version no longer pins test covers this.
• Fixed: no plugin record. An empty or malformed manifest now stops with Nothing to refresh… instead of running a bare install. That means no new detection and no install-everything outside a terminal. The installs nothing without a recorded install test covers this.
• New: passing --refresh together with --only, --plugin or a harness flag is now a usage error, and has a test.

Still open: each harness can still end up with plugins it never had. The union logic moved from update into _recordedPlugins, but it behaves the same. Details are in the inline comment. The new reinstalls the recorded harnesses and plugins test gives both harnesses the same plugin, so it does not catch this.

[unicoderbot]

This is still the union-widening problem from the last review; it has moved here from update. _recordedPlugins merges every selected harness's plugins into one set, and translate and _pluginVersions then apply that set to every harness in selected.

Example: the manifest records claude: {vgv-wingspan} and codex: {vgv-wingspan, vgv-ffca-plugin}. update then installs vgv-ffca-plugin for claude and records it under claude, so each later refresh keeps it there.

Now that --refresh runs inside the new version, a fix doesn't need multiple child processes. Two options:

• Group the harnesses by their recorded, still-pinned plugin set, then run resolve, translate and reconcile once per group.
• Translate the full union, then keep only each harness's own plugins when writing the plan and calling _pluginVersions.

It's also worth giving the two harnesses different plugin sets in reinstalls the recorded harnesses and plugins, so the test fails on the current behaviour.

[ryzizub]

Intentional. install applies one plugin selection across every harness it configures, and --refresh keeps that model. Per-harness plugin sets only arise from separate install --only runs, and we're not changing that in this PR.

[ryzizub]

/unicoder /review-pr

[unicoderbot]

I'll review the code and share my feedback shortly.

• Preparing environment
  • Cloning repository
  • Setting up dev environment
• Reviewing code
• Posting review

Code review submitted.

[unicoderbot]

Re-checked against the current diff. The two fixes from last time still hold, and the new parts look correct:

• Flag rejection: passing --refresh with --only, --plugin or a harness flag is rejected, and the test covers it.
• Manifest check: update only reinstalls when a manifest file exists. ManifestStore.save deletes the file once it records nothing, so a full uninstall stops update from reinstalling afterwards.
• Child process: it runs from the newly activated version via Platform.resolvedExecutable, which is dart under pub global. It forwards --verbose, inherits stdio, and update returns the child's exit code.
• Test isolation: command_runner_test now gives UpdateCommand an empty environment, so it never reinstalls into the real home directory.

Still open: the union-widening problem from my last review. _recordedPlugins still merges every recorded harness's plugins into one set, and translate and _pluginVersions then apply that set to every harness in selected. The reinstalls the recorded harnesses and plugins test still gives both harnesses the same plugin, so it does not catch this. See the earlier inline comment; I am not repeating it here.

New, minor: when every recorded plugin has been dropped from the pins, --refresh falls through to the interactive-install error message. It tells the user to pass --plugin, which --refresh rejects. Details are in the inline comment.