Skip to content

fix(plugins): refuse a concurrent install instead of reporting success - #153

Merged
kipavy merged 1 commit into
devfrom
fix/plugin-install-silent-noop
Aug 19, 2026
Merged

fix(plugins): refuse a concurrent install instead of reporting success#153
kipavy merged 1 commit into
devfrom
fix/plugin-install-silent-noop

Conversation

@kipavy

@kipavy kipavy commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Follow-up from the #150/#152 review: the only case in the deep-link confirm flow where the UI actively lies.

The bug

marketplaceStore.installPlugin returned silently when the requested id was already in installing:

if (installing.has(plugin.id)) return;

Every caller reads that resolve as a completed install. Accept a plugin-install deep-link sheet while the Settings tab is installing the same plugin and the sheet resolves, dismisses, and tells the user it worked — having written nothing.

The fix

The second request throws PluginInstallInProgressError (new, in src/plugins/installErrors.ts), which pluginInstallErrorMessage names for the user via a new settings.plugins.install.alreadyInstalling key in all four locales. Both the Settings toast and the confirm sheet already route through that helper, so this adds no second instanceof ladder.

Refused rather than joined to the in-flight install, deliberately. The sheet and the Settings tab can hold different reviewed manifest texts for the same id (reviewedManifestText is what closes the fetch→consent→load TOCTOU). Joining would install the other caller's reviewed bundle while telling this caller its consent was honoured — trading a false success for a consent bypass.

Callers after the change:

  • deep-link confirm sheet — shows "already being installed", stays honest.
  • PluginsSection — unreachable in practice (the button is disabled while busy), same toast path if it ever is.
  • plugins.ts domain layer (MCP install_plugin) — propagates the message instead of the misleading produced no loaded plugin.
  • restoreMissingPlugins — sequential and already catching; unaffected.

Tests

Three added to marketplaceStore.installUpdate.test.ts, sharing one gatedFetch() helper that parks the first install inside plugin_fetch_url:

  • a second install of an id already installing is refused
  • a refused concurrent install does not clear the running install's busy state
  • an id is installable again once its previous install has settled

All three fail on dev, pass here.

Verification

  • Full CI=true vitest run: 479/480 files, 3718/3719 tests. Sole failure is tests/pluginBundleBuild.test.ts (39237ms — the known contention flake); re-run alone with the edited file it is 17/17 green.
  • Real typecheck node ./node_modules/typescript/bin/tsc --noEmit -p tsconfig.json: exit 0, no output.

installPlugin returned silently when the same id was already installing, so
every caller read the resolve as a completed install. Accepting a plugin-install
deep-link sheet while the Settings tab was installing the same plugin closed the
sheet reporting success having written nothing.

The second request now throws PluginInstallInProgressError, which
pluginInstallErrorMessage names for the user. It is refused rather than joined to
the running install: the two callers may have reviewed different manifests for
the same id, and the running install cannot stand in for the one this caller
consented to.
@kipavy
kipavy merged commit 79d60a5 into dev Aug 19, 2026
4 checks passed
@kipavy
kipavy deleted the fix/plugin-install-silent-noop branch August 19, 2026 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant