diff --git a/docs/agents/agent-skills.mdx b/docs/agents/agent-skills.mdx index aa9008bea1..fcbe4b15b2 100644 --- a/docs/agents/agent-skills.mdx +++ b/docs/agents/agent-skills.mdx @@ -67,7 +67,9 @@ Plugin skills have the lowest precedence within their scope and are read-only. A Global plugins can be installed from git via **Settings → Plugins** (paste a git URL or `owner/repo[@ref]`). The preview lists the full package before anything is written. Select which skills and MCP servers to import; both groups start with all current items selected and support **Select all** and **Clear**. You can install with neither group selected. Agents, workflows, slash commands, and hooks are unaffected by this choice; review their disclosures too. Imported MCP servers remain disabled until you enable them per workspace. -Installed rows show imported-of-available counts. For a managed, present plugin, choose **Add components** (or **Add Plugin Components…** in the command palette) to import more from its locked installed version without fetching the remote. Previously imported components are checked and read-only. Select additions and confirm, or cancel without changing anything. If the installed version changes during review, the inventory refreshes and you must select again. Removing imports is not supported by this flow. +Installed rows show imported-of-available counts. For a managed, present plugin, choose **Manage components** (or **Manage Plugin Components…** in the command palette) to change which skills and MCP servers are imported from its locked installed version without fetching the remote. Deselect imported components, select new ones, or use **Select all** and **Clear** for a whole group. Review the add/remove counts and choose **Save changes**, or **Cancel** to discard your draft. Saving an empty selection keeps the plugin installed with no skills or MCP servers imported; uninstall is a separate action. If the installed files or saved selection change during review, the inventory refreshes and you must review and save again. + +Removing imports does not delete source files, plugin data, or workspace MCP settings (including enablement and tool allowlists). New MCP imports remain disabled until enabled per workspace; re-adding a server honors its saved workspace enablement. Agents, hooks, workflows, and slash commands are not changed by this selection. Legacy installs continue to import all components until you save a different selection. Once a selection is explicit, updates preserve it and do not automatically import newly added components. Updates preserve explicit selections: newly available skills and MCP servers stay unimported until you add them. Updates still require consent for full-package capability changes, including unimported components. Older managed installs without a saved selection continue importing everything, including new components on update. Unmanaged and project-local plugins are unchanged. diff --git a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx index 024638e110..8904d03e68 100644 --- a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx +++ b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx @@ -88,12 +88,8 @@ const ComponentChooser: React.FC<{ detail: `${server.transport} · ${server.summary}`, })); const imported = props.imported?.[group] ?? []; - const selectable = entries - .map((entry) => entry.name) - .filter((name) => !imported.includes(name)); - const count = entries.filter( - ({ name }) => imported.includes(name) || props.selected[group].includes(name) - ).length; + const selectable = entries.map((entry) => entry.name); + const count = entries.filter(({ name }) => props.selected[group].includes(name)).length; const change = (names: string[]) => props.onChange({ ...props.selected, [group]: names }); return (
change( checked === true @@ -154,8 +150,9 @@ const ComponentChooser: React.FC<{ ); })}

- Importing MCP servers only makes them available; they stay disabled until enabled per - workspace. + {props.imported + ? "New MCP imports need workspace opt-in. Re-adding a server honors its saved workspace enablement." + : "Importing MCP servers only makes them available; they stay disabled until enabled per workspace."}

Tab to navigate · Space to select · Enter to activate buttons @@ -163,12 +160,43 @@ const ComponentChooser: React.FC<{ ); -const AddComponentsPanel: React.FC<{ +function effectiveImports(inventory: AgentPluginComponents): AgentPluginImportedComponents { + // Manage saves replace the selection; carrying unavailable names would defeat Clear/empty consent. + return { + skills: inventory.skills + .map((skill) => skill.name) + .filter( + (name) => + !inventory.importedComponents || inventory.importedComponents.skills.includes(name) + ), + mcpServers: inventory.mcpServers + .map((server) => server.serverName) + .filter( + (name) => + !inventory.importedComponents || inventory.importedComponents.mcpServers.includes(name) + ), + }; +} + +function sameImports( + a: AgentPluginImportedComponents | null, + b: AgentPluginImportedComponents | null +): boolean { + if (a === null || b === null) return a === b; + return (["skills", "mcpServers"] as const).every( + (group) => + a[group].every((name) => b[group].includes(name)) && + b[group].every((name) => a[group].includes(name)) + ); +} + +const ManageComponentsPanel: React.FC<{ name: string; - onAdded: () => Promise; + onSaved: () => Promise; onClose: () => void; }> = (props) => { const { api } = useAPI(); + // Keep the raw baseline (including legacy absence) separate from the visible selection. const [inventory, setInventory] = useState(null); const [selected, setSelected] = useState({ skills: [], @@ -177,7 +205,7 @@ const AddComponentsPanel: React.FC<{ const [error, setError] = useState(null); const [busy, setBusy] = useState(true); const [loadAttempt, setLoadAttempt] = useState(0); - const [added, setAdded] = useState(false); + const [saved, setSaved] = useState(false); const name = props.name; useEffect(() => { @@ -186,8 +214,10 @@ const AddComponentsPanel: React.FC<{ api.agentPlugins.getComponents({ name }).then( (result) => { if (ignore) return; - if (result.success) setInventory(result.data); - else setError(result.error); + if (result.success) { + setInventory(result.data); + setSelected(effectiveImports(result.data)); + } else setError(result.error); setBusy(false); }, (err: unknown) => { @@ -202,66 +232,107 @@ const AddComponentsPanel: React.FC<{ }; }, [api, name, loadAttempt]); - const handleAdd = async () => { - if (!api || !inventory || busy || selected.skills.length + selected.mcpServers.length === 0) - return; + const imported = inventory ? effectiveImports(inventory) : { skills: [], mcpServers: [] }; + const added = (["skills", "mcpServers"] as const).reduce( + (count, group) => + count + selected[group].filter((name) => !imported[group].includes(name)).length, + 0 + ); + const removed = (["skills", "mcpServers"] as const).reduce( + (count, group) => + count + imported[group].filter((name) => !selected[group].includes(name)).length, + 0 + ); + const handleSave = async () => { + if (!api || !inventory || busy || added + removed === 0) return; setBusy(true); setError(null); - setAdded(false); + setSaved(false); + let confirmed = false; + let responseLost = false; + let savedImports: AgentPluginImportedComponents | null = selected; + let warning: string | undefined; try { - const result = await api.agentPlugins.addComponents({ - name, - expectedLockedSha: inventory.lockedSha, - expectedContentHash: inventory.contentHash, - ...selected, - }); + const result = await api.agentPlugins + .setComponents({ + name, + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: inventory.importedComponents ?? null, + importedComponents: selected, + }) + .catch((err: unknown) => { + responseLost = true; + throw err; + }); if (!result.success) throw new Error(result.error); - setInventory({ ...inventory, importedComponents: result.data.importedComponents }); - setSelected({ skills: [], mcpServers: [] }); - setAdded(true); + confirmed = true; + warning = result.cleanupWarning; + savedImports = result.data.importedComponents ?? null; publishAgentPluginsMutated(); - await props.onAdded(); } catch (err) { setError(getErrorMessage(err)); - // Compare receipts rather than parsing backend error prose. Never retry choices - // against a different installed version without another explicit selection. - try { - const current = await api.agentPlugins.getComponents({ name }); - if ( - current.success && - (current.data.lockedSha !== inventory.lockedSha || - current.data.contentHash !== inventory.contentHash) - ) { - setInventory(current.data); - setSelected({ skills: [], mcpServers: [] }); - setError( - "The installed plugin changed. Inventory refreshed; select components again before confirming." - ); - } - } catch { - /* Keep the original mutation error and choices when the inventory is unreachable. */ + } + // Refetch even after a lost response: a transport failure does not mean the write failed. + try { + const current = await api.agentPlugins.getComponents({ name }); + if (!current.success) throw new Error(current.error); + const sameTree = + current.data.lockedSha === inventory.lockedSha && + current.data.contentHash === inventory.contentHash; + if ( + (confirmed || responseLost) && + sameTree && + sameImports(current.data.importedComponents ?? null, savedImports) + ) { + setInventory(current.data); + setSelected(effectiveImports(current.data)); + setSaved(true); + setError(warning ?? null); + if (!confirmed) publishAgentPluginsMutated(); + await props.onSaved(); + } else if ( + confirmed || + !sameTree || + !sameImports(current.data.importedComponents ?? null, inventory.importedComponents ?? null) + ) { + setSaved(false); + setInventory(current.data); + setSelected(effectiveImports(current.data)); + setError( + "The installed plugin or selection changed. Inventory refreshed; review your choices and save again." + ); + // A rejected stale save still discovered newer counts for the surrounding card. + await props.onSaved(); + } + } catch (err) { + setError( + confirmed + ? `Components saved, but refreshing failed: ${getErrorMessage(err)}. Reopen to read the saved selection.` + : `Could not confirm the saved selection: ${getErrorMessage(err)}. Reopen to refresh before retrying.` + ); + if (confirmed || responseLost) { + // Refresh counts for acknowledged or possible writes even without an inventory receipt. + if (!confirmed) publishAgentPluginsMutated(); + await props.onSaved(); } } finally { setBusy(false); } }; - const imported = inventory?.importedComponents ?? { - skills: inventory?.skills.map((skill) => skill.name) ?? [], - mcpServers: inventory?.mcpServers.map((server) => server.serverName) ?? [], - }; - const allImported = - inventory && - inventory.skills.every((skill) => imported.skills.includes(skill.name)) && - inventory.mcpServers.every((server) => imported.mcpServers.includes(server.serverName)); return (

- Add components from the installed version + Manage components from the installed version {inventory ? ` · ${inventory.lockedSha.slice(0, 12)}` : ""}. No remote fetch.

+

+ Removing imports keeps source files, plugin data, and workspace MCP settings. Uninstall is + separate. +

{busy && (

- {inventory ? "Adding components…" : "Loading components…"} + {inventory ? "Saving components…" : "Loading components…"}

)} {error && ( @@ -269,34 +340,41 @@ const AddComponentsPanel: React.FC<{ {error}

)} - {added && ( + {saved && (

- Components imported. + Component selection saved.

)} {inventory && ( - { - setSelected(selection); - setAdded(false); - }} - /> - )} - {allImported && ( -

All available components are already imported.

+ <> + { + setSelected(selection); + setSaved(false); + }} + /> +

+ {added} to add · {removed} to remove +

+ {selected.skills.length + selected.mcpServers.length === 0 && ( +

+ No skills or MCP servers will be imported. The plugin stays installed. +

+ )} + )}
{inventory ? ( ) : (
@@ -752,7 +830,7 @@ export const PluginsSettingsSection: React.FC = () => { initialIntent?.type === "confirm-uninstall" ? initialIntent.name : null ); const [componentsTarget, setComponentsTarget] = useState( - initialIntent?.type === "add-components" ? initialIntent.name : null + initialIntent?.type === "manage-components" ? initialIntent.name : null ); const [installSucceeded, setInstallSucceeded] = useState(false); /** Name of the plugin with an update/uninstall in flight. */ @@ -841,7 +919,7 @@ export const PluginsSettingsSection: React.FC = () => { case "open-add-panel": openAddPanel(); break; - case "add-components": + case "manage-components": setComponentsTarget(intent.name); break; case "confirm-uninstall": @@ -1113,9 +1191,9 @@ export const PluginsSettingsSection: React.FC = () => { className="h-7 px-2 text-xs" disabled={busyPlugin !== null} onClick={() => setComponentsTarget(item.name)} - aria-label={`Add components to ${item.name}`} + aria-label={`Manage components for ${item.name}`} > - Add components + Manage components )} {updateAvailable && ( @@ -1159,10 +1237,10 @@ export const PluginsSettingsSection: React.FC = () => { managed-registry name, so the managed row is the one identity-correct anchor. */} {item.managed && item.present && componentsTarget === item.name && ( - setComponentsTarget(null)} /> )} diff --git a/src/browser/features/Settings/Sections/pluginsSectionIntents.ts b/src/browser/features/Settings/Sections/pluginsSectionIntents.ts index afd2ff1f5a..92be028549 100644 --- a/src/browser/features/Settings/Sections/pluginsSectionIntents.ts +++ b/src/browser/features/Settings/Sections/pluginsSectionIntents.ts @@ -18,7 +18,7 @@ export type PluginsSectionIntent = /** Expand the Add Plugin form. */ | { type: "open-add-panel" } /** Review additional skills/MCP from a managed installed tree. */ - | { type: "add-components"; name: string } + | { type: "manage-components"; name: string } /** Open the uninstall confirmation for a managed plugin. */ | { type: "confirm-uninstall"; name: string } /** Show the in-place update review for a capability-changing update the palette previewed. */ diff --git a/src/browser/stories/App.pluginImports.stories.tsx b/src/browser/stories/App.pluginImports.stories.tsx index 8a56f71c73..544ed22b06 100644 --- a/src/browser/stories/App.pluginImports.stories.tsx +++ b/src/browser/stories/App.pluginImports.stories.tsx @@ -36,7 +36,7 @@ const preview: AgentPluginInstallPreview = { warnings: [], }; -function setupPluginSettings(installed = false) { +function setupPluginSettings(installed = false, conflict = false) { expandLeftSidebar(); const client = setupSettingsStory({ experiments: { [EXPERIMENT_IDS.AGENT_PLUGINS]: true } }); let entry: AgentPluginInstallEntry = { @@ -99,16 +99,13 @@ function setupPluginSettings(installed = false) { entry = { ...entry, importedComponents: input.importedComponents ?? undefined }; return Promise.resolve({ success: true, data: entry }); }; - client.agentPlugins.addComponents = (input) => { - entry = { - ...entry, - importedComponents: { - skills: [...new Set([...(entry.importedComponents?.skills ?? []), ...input.skills])], - mcpServers: [ - ...new Set([...(entry.importedComponents?.mcpServers ?? []), ...input.mcpServers]), - ], - }, - }; + client.agentPlugins.setComponents = (input) => { + if (conflict) { + conflict = false; + entry = { ...entry, importedComponents: { skills: ["review"], mcpServers: ["reference"] } }; + return Promise.resolve({ success: false, error: "Selection changed in another window" }); + } + entry = { ...entry, importedComponents: input.importedComponents }; return Promise.resolve({ success: true, data: entry }); }; return client; @@ -163,28 +160,84 @@ export const PreviewPhone: AppStory = { parameters: { pixel: { matrix: { themes: ["dark"], viewports: ["phone"] } } }, }; -export const AddComponentsDesktop: AppStory = { +export const ManageComponentsDesktop: AppStory = { ...PreviewDesktop, render: () => setupPluginSettings(true)} />, play: async ({ canvasElement }) => { const canvas = await openPlugins(canvasElement); await userEvent.click( - await canvas.findByRole("button", { name: "Add components to review-tools" }) + await canvas.findByRole("button", { name: "Manage components for review-tools" }) ); - await expect(await canvas.findByRole("checkbox", { name: "review" })).toBeDisabled(); - await expect(canvas.getByRole("button", { name: "Import selected" })).toBeDisabled(); + await expect(await canvas.findByRole("checkbox", { name: "review" })).toBeEnabled(); + await expect(canvas.getByRole("button", { name: "Save changes" })).toBeDisabled(); + await userEvent.click(canvas.getByRole("checkbox", { name: "review" })); await userEvent.click(canvas.getByRole("checkbox", { name: "research" })); - await expect(canvas.getByRole("button", { name: "Import selected" })).toBeEnabled(); + await userEvent.click(canvas.getByRole("button", { name: "Save changes" })); + await waitFor(() => expect(canvas.getByRole("button", { name: "Done" })).toBeEnabled()); + await expect(canvas.getByRole("checkbox", { name: "review" })).not.toBeChecked(); + await expect(canvas.getByRole("checkbox", { name: "research" })).toBeChecked(); + await userEvent.click(canvas.getByRole("button", { name: "Done" })); + await userEvent.click( + canvas.getByRole("button", { name: "Manage components for review-tools" }) + ); + await expect(await canvas.findByRole("checkbox", { name: "review" })).not.toBeChecked(); + await expect(canvas.getByRole("checkbox", { name: "research" })).toBeChecked(); + await expect(canvas.getByRole("button", { name: "Save changes" })).toBeDisabled(); await checkPhoneBounds(canvasElement); }, }; -export const AddComponentsPhone: AppStory = { - ...AddComponentsDesktop, +export const ManageComponentsPhone: AppStory = { + ...ManageComponentsDesktop, play: async (context) => { await expect(context.parameters.pixel.matrix.viewports).toContain("phone"); - await AddComponentsDesktop.play?.(context); + await ManageComponentsDesktop.play?.(context); }, globals: { viewport: { value: "mobile1", isRotated: false } }, parameters: { pixel: { matrix: { themes: ["dark"], viewports: ["phone"] } } }, }; + +export const EmptySelection: AppStory = { + ...ManageComponentsDesktop, + play: async ({ canvasElement }) => { + const canvas = await openPlugins(canvasElement); + await userEvent.click( + await canvas.findByRole("button", { name: "Manage components for review-tools" }) + ); + const group = within(await canvas.findByRole("group", { name: "Skills" })); + await userEvent.click(group.getByRole("button", { name: "Clear" })); + await expect(canvas.getByRole("button", { name: "Save changes" })).toBeEnabled(); + await userEvent.click(canvas.getByRole("button", { name: "Save changes" })); + await canvas.findByText(/0 of 2 skills imported/); + await waitFor(() => expect(canvas.getByRole("button", { name: "Done" })).toBeEnabled()); + for (const checkbox of canvas.getAllByRole("checkbox")) + await expect(checkbox).not.toBeChecked(); + await expect( + canvas.getByRole("button", { name: "Manage components for review-tools" }) + ).toBeEnabled(); + await checkPhoneBounds(canvasElement); + }, +}; + +export const SelectionConflict: AppStory = { + ...ManageComponentsDesktop, + render: () => setupPluginSettings(true, true)} />, + play: async ({ canvasElement }) => { + const canvas = await openPlugins(canvasElement); + await userEvent.click( + await canvas.findByRole("button", { name: "Manage components for review-tools" }) + ); + await userEvent.click(await canvas.findByRole("checkbox", { name: "research" })); + await userEvent.click(canvas.getByRole("button", { name: "Save changes" })); + await canvas.findByRole("alert"); + await waitFor(() => expect(canvas.getByRole("checkbox", { name: "reference" })).toBeChecked()); + await expect(canvas.getByRole("checkbox", { name: "research" })).not.toBeChecked(); + await expect(canvas.getByRole("button", { name: "Save changes" })).toBeDisabled(); + await userEvent.click(canvas.getByRole("checkbox", { name: "research" })); + await userEvent.click(canvas.getByRole("button", { name: "Save changes" })); + await canvas.findByText(/2 of 2 skills imported/); + await waitFor(() => expect(canvas.getByRole("button", { name: "Done" })).toBeEnabled()); + await expect(canvas.queryByRole("alert")).not.toBeInTheDocument(); + await checkPhoneBounds(canvasElement); + }, +}; diff --git a/src/browser/stories/mocks/orpc.ts b/src/browser/stories/mocks/orpc.ts index 7ceb461107..ce8958cd4f 100644 --- a/src/browser/stories/mocks/orpc.ts +++ b/src/browser/stories/mocks/orpc.ts @@ -1140,6 +1140,16 @@ export function createMockORPCClient(options: MockORPCClientOptions = {}): APICl agentPlugins: { list: () => Promise.resolve({ success: true, data: agentPluginsMock?.items ?? [] }), containerLocation: () => Promise.resolve("~/.mux/plugins"), + getComponents: () => + Promise.resolve({ + success: false, + error: "No component inventory configured in this story", + }), + setComponents: () => + Promise.resolve({ + success: false, + error: "No component selection configured in this story", + }), checkUpdates: () => Promise.resolve({ success: true, data: agentPluginsMock?.updateChecks ?? [] }), preview: () => diff --git a/src/browser/utils/commandIds.ts b/src/browser/utils/commandIds.ts index 5e17db10fd..4a154336de 100644 --- a/src/browser/utils/commandIds.ts +++ b/src/browser/utils/commandIds.ts @@ -99,7 +99,7 @@ export const CommandIds = { // Agent Plugin commands (agent-plugins experiment) pluginsInstall: () => "plugins:install" as const, - pluginsAddComponents: () => "plugins:add-components" as const, + pluginsManageComponents: () => "plugins:manage-components" as const, pluginsUninstall: () => "plugins:uninstall" as const, pluginsCheckUpdates: () => "plugins:check-updates" as const, pluginsUpdateAll: () => "plugins:update-all" as const, diff --git a/src/browser/utils/commands/sources.test.ts b/src/browser/utils/commands/sources.test.ts index dd7f8bf635..2a4aa19d93 100644 --- a/src/browser/utils/commands/sources.test.ts +++ b/src/browser/utils/commands/sources.test.ts @@ -1490,7 +1490,7 @@ test("plugin component action is gated and only targets present managed installs expect( mk({ onOpenSettings: openSettings }) .flatMap((source) => source()) - .find((action) => action.id === CommandIds.pluginsAddComponents()) + .find((action) => action.id === CommandIds.pluginsManageComponents()) ).toBeUndefined(); const api = createMockORPCClient({ agentPlugins: { @@ -1525,22 +1525,25 @@ test("plugin component action is gated and only targets present managed installs const mutation = mock(() => Promise.resolve({ success: false as const, error: "Must use the chooser" }) ); - api.agentPlugins.addComponents = mutation; + api.agentPlugins.setComponents = mutation; const action = mk({ api, agentPluginsEnabled: true, onOpenSettings: openSettings }) .flatMap((source) => source()) - .find((action) => action.id === CommandIds.pluginsAddComponents()); + .find((action) => action.id === CommandIds.pluginsManageComponents()); const field = action?.prompt?.fields[0]; if (field?.type !== "select" || !action?.prompt) throw new Error("Expected component plugin picker"); expect((await field.getOptions({})).map((option) => option.id)).toEqual(["managed"]); consumePendingPluginsSectionIntent(); await action.prompt.onSubmit({ pluginName: "managed" }); - expect(consumePendingPluginsSectionIntent()).toEqual({ type: "add-components", name: "managed" }); + expect(consumePendingPluginsSectionIntent()).toEqual({ + type: "manage-components", + name: "managed", + }); const received: PluginsSectionIntent[] = []; const unsubscribe = subscribePluginsSectionIntents((intent) => received.push(intent)); try { await action.prompt.onSubmit({ pluginName: "managed" }); - expect(received).toEqual([{ type: "add-components", name: "managed" }]); + expect(received).toEqual([{ type: "manage-components", name: "managed" }]); expect(consumePendingPluginsSectionIntent()).toBeNull(); expect(mutation).not.toHaveBeenCalled(); expect(openSettings).toHaveBeenCalledWith("plugins"); diff --git a/src/browser/utils/commands/sources.ts b/src/browser/utils/commands/sources.ts index 346072915c..1246ceb8ff 100644 --- a/src/browser/utils/commands/sources.ts +++ b/src/browser/utils/commands/sources.ts @@ -1742,14 +1742,23 @@ export function buildCoreSources(p: BuildSourcesParams): Array<() => CommandActi }, }, { - id: CommandIds.pluginsAddComponents(), - title: "Add Plugin Components…", - subtitle: "Import more skills or MCP servers from the installed version", + id: CommandIds.pluginsManageComponents(), + title: "Manage Plugin Components…", + subtitle: "Choose which installed skills and MCP servers are imported", section: section.settings, - keywords: ["plugin", "add", "import", "skill", "mcp", "components"], + keywords: [ + "plugin", + "manage", + "add", + "remove", + "import", + "skill", + "mcp", + "components", + ], run: () => undefined, prompt: { - title: "Add Plugin Components", + title: "Manage Plugin Components", fields: [ { type: "select", @@ -1771,7 +1780,10 @@ export function buildCoreSources(p: BuildSourcesParams): Array<() => CommandActi }, ], onSubmit: (values) => { - publishPluginsSectionIntent({ type: "add-components", name: values.pluginName }); + publishPluginsSectionIntent({ + type: "manage-components", + name: values.pluginName, + }); openSettings("plugins"); }, }, diff --git a/src/common/orpc/schemas/api.test.ts b/src/common/orpc/schemas/api.test.ts index abee162fbf..f0be10232e 100644 --- a/src/common/orpc/schemas/api.test.ts +++ b/src/common/orpc/schemas/api.test.ts @@ -4,6 +4,7 @@ import { ProviderConfigInfoSchema, ProvidersConfigMapSchema, config, + agentPlugins, workspace, } from "./api"; import type { AWSCredentialStatus, ProviderConfigInfo, ProvidersConfigMap } from "../types"; @@ -290,3 +291,21 @@ describe("config.saveConfig schema", () => { expect(result.success).toBe(true); }); }); + +describe("agentPlugins.setComponents schema", () => { + it("requires a baseline and distinguishes legacy absence from an explicit empty selection", () => { + const request = { + name: "plugin", + expectedLockedSha: "sha", + expectedContentHash: "receipt", + importedComponents: { skills: [], mcpServers: [] }, + }; + expect(agentPlugins.setComponents.input.safeParse(request).success).toBe(false); + for (const expectedImportedComponents of [null, { skills: [], mcpServers: [] }]) { + expect( + agentPlugins.setComponents.input.parse({ ...request, expectedImportedComponents }) + .expectedImportedComponents + ).toEqual(expectedImportedComponents); + } + }); +}); diff --git a/src/common/orpc/schemas/api.ts b/src/common/orpc/schemas/api.ts index 2ba5c6e594..6f13bba02e 100644 --- a/src/common/orpc/schemas/api.ts +++ b/src/common/orpc/schemas/api.ts @@ -1083,13 +1083,22 @@ export const agentPlugins = { input: z.object({ name: z.string() }), output: ResultSchema(AgentPluginComponentsSchema, z.string()), }, - addComponents: { - input: AgentPluginImportedComponentsSchema.extend({ + setComponents: { + input: z.object({ name: z.string(), expectedLockedSha: z.string(), expectedContentHash: z.string(), + expectedImportedComponents: AgentPluginImportedComponentsSchema.nullable(), + importedComponents: AgentPluginImportedComponentsSchema, }), - output: ResultSchema(AgentPluginInstallEntrySchema, z.string()), + output: z.discriminatedUnion("success", [ + z.object({ + success: z.literal(true), + data: AgentPluginInstallEntrySchema, + cleanupWarning: z.string().optional(), + }), + z.object({ success: z.literal(false), error: z.string() }), + ]), }, /** Display path of the ACTIVE managed plugin container (config-derived root; never hardcode it in UI). */ containerLocation: { diff --git a/src/common/orpc/schemas/mcp.ts b/src/common/orpc/schemas/mcp.ts index ad286f383f..e40e32c5f9 100644 --- a/src/common/orpc/schemas/mcp.ts +++ b/src/common/orpc/schemas/mcp.ts @@ -36,6 +36,7 @@ export const MCPServerPluginProvenanceSchema = z.object({ sourceScope: z.enum(["project", "global"]), /** Installation location discriminator, e.g. ".xum/plugins/demo" (same-name plugins can sit in sibling containers). */ sourceLocation: z.string(), + componentPolicy: z.object({ registryPath: z.string(), name: z.string() }).optional(), }); export const MCPServerInfoSchema = z.discriminatedUnion("transport", [ diff --git a/src/common/types/mcp.ts b/src/common/types/mcp.ts index 552f6d8800..781fabfc35 100644 --- a/src/common/types/mcp.ts +++ b/src/common/types/mcp.ts @@ -19,6 +19,8 @@ export interface MCPServerPluginProvenance { * .agents), so the UI needs this discriminator to tell instances apart. */ sourceLocation: string; + /** Canonical managed owner; retained even if its registry row later disappears. */ + componentPolicy?: { registryPath: string; name: string }; } export interface MCPServerBaseInfo { diff --git a/src/node/orpc/router.ts b/src/node/orpc/router.ts index b9430d9263..b20ae5559a 100644 --- a/src/node/orpc/router.ts +++ b/src/node/orpc/router.ts @@ -1059,11 +1059,11 @@ export const router = (authToken?: string) => { .handler(({ context, input }) => context.agentPluginInstallService.getComponentsResult(input) ), - addComponents: t - .input(schemas.agentPlugins.addComponents.input) - .output(schemas.agentPlugins.addComponents.output) + setComponents: t + .input(schemas.agentPlugins.setComponents.input) + .output(schemas.agentPlugins.setComponents.output) .handler(({ context, input }) => - context.agentPluginInstallService.addComponentsResult(input) + context.agentPluginInstallService.setComponentsResult(input) ), containerLocation: t .input(schemas.agentPlugins.containerLocation.input) diff --git a/src/node/services/agentPlugins/discovery.ts b/src/node/services/agentPlugins/discovery.ts index 8ab12099da..b5fdcfe1f1 100644 --- a/src/node/services/agentPlugins/discovery.ts +++ b/src/node/services/agentPlugins/discovery.ts @@ -161,6 +161,7 @@ export interface AgentPluginContainer { export interface AgentPluginInfo { /** Managed global imports; absent means legacy/unmanaged import-all. */ importedComponents?: AgentPluginImportedComponents; + componentPolicy?: { registryPath: string; name: string }; name: string; scope: AgentPluginScope; /** Canonical (realpath) plugin root directory. */ @@ -868,6 +869,10 @@ export async function discoverAgentPlugins( snapshot.plugins.set(entryName, plugin); if (plugin) { const imports = snapshot.imports; + const registryPath = groups.get(canonicalPath)?.registryPath; + if (registryPath !== undefined && imports?.byName.has(entryName)) { + plugin.componentPolicy = { registryPath, name: entryName }; + } plugin.importedComponents = imports === null || (imports?.hasUnidentifiedEntries && !imports.byName.has(entryName)) ? { skills: [], mcpServers: [] } diff --git a/src/node/services/agentPlugins/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index b4b015ff38..e82c81cb59 100644 --- a/src/node/services/agentPlugins/installService.test.ts +++ b/src/node/services/agentPlugins/installService.test.ts @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/await-thenable -- bun:test types `await expect(...).rejects.toThrow()` as void */ -import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; import * as fsPromises from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -7,9 +7,13 @@ import * as path from "node:path"; import { Config } from "@/node/config"; import { MCPServerManager } from "@/node/services/mcpServerManager"; import { MCPConfigService } from "@/node/services/mcpConfigService"; +import * as runtimeFactory from "@/node/runtime/runtimeFactory"; +import type { Runtime } from "@/node/runtime/Runtime"; +import * as mcpSdk from "@/node/services/mcpClient"; import { LocalRuntime } from "@/node/runtime/LocalRuntime"; -import { readMutationEpochToken } from "./journals"; +import { acquirePluginMutationLock, MUTATION_LOCK_FILE, readMutationEpochToken } from "./journals"; import * as treeHash from "./treeHash"; +import { readPluginMcpPolicy } from "./registry"; import type { WorkspaceMcpOverridesService } from "@/node/services/workspaceMcpOverridesService"; import type { WorkspaceMCPOverrides } from "@/common/types/mcp"; import { shellQuote } from "@/common/utils/shell"; @@ -280,10 +284,17 @@ describe("AgentPluginInstallService", () => { pluginInvalidation: { keyPrefix: "plugin:", readToken: () => readMutationEpochToken(stagingDir()), + readComponentPolicy: () => readPluginMcpPolicy(registryFile()), + tryAcquireComponentPolicyLock: (options) => + acquirePluginMutationLock(muxRoot, { timeoutMs: 0, ...options }), readWorkspaceOverrides: async () => JSON.parse(await fsPromises.readFile(overridesFile, "utf8")) as WorkspaceMCPOverrides, }, }); + service = new AgentPluginInstallService(config, { + isEnabled: () => true, + mcpServerManager: manager, + }); const request = { workspaceId: "imports-workspace", projectPath: muxRoot, @@ -329,12 +340,12 @@ describe("AgentPluginInstallService", () => { expect(await fsPromises.readFile(startsFile, "utf8")).toBe(starts); expect(await fsPromises.readFile(siblingStartsFile, "utf8")).toBe(siblingStarts); expect(await fsPromises.readFile(stateFile, "utf8")).toBe("preserve me"); - await service.addComponents({ + await service.setComponents({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: [], - mcpServers: ["later"], + expectedImportedComponents: { skills: [], mcpServers: ["echo"] }, + importedComponents: { skills: [], mcpServers: ["echo", "later"] }, }); const discovered = await configService.listServers(muxRoot, false); expect(discovered[key("later")]?.disabled).toBe(true); @@ -344,12 +355,12 @@ describe("AgentPluginInstallService", () => { expect(await fsPromises.readFile(siblingStartsFile, "utf8")).toBe(siblingStarts); // A saved enable override is honored only after import, without restarting either sibling. - await service.addComponents({ + await service.setComponents({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: [], - mcpServers: ["excluded"], + expectedImportedComponents: { skills: [], mcpServers: ["echo", "later"] }, + importedComponents: { skills: [], mcpServers: ["echo", "later", "excluded"] }, }); expect((await manager.getToolsForWorkspace(request)).stats.startedServerCount).toBe(3); const afterEnabledAddition = await fsPromises.readFile(startsFile, "utf8"); @@ -360,6 +371,34 @@ describe("AgentPluginInstallService", () => { expect(await fsPromises.readFile(stateFile, "utf8")).toBe("preserve me"); expect(await fsPromises.readFile(overridesFile, "utf8")).toBe(JSON.stringify(overrides)); expect(request.overrides).toEqual(overrides); + const inventory = await service.getComponents({ name: "demo-plugin" }); + const baseline = inventory.importedComponents ?? null; + const reduced = { skills: [], mcpServers: ["echo", "later"] }; + const mutation = { + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + }; + await service.setComponents({ + ...mutation, + expectedImportedComponents: baseline, + importedComponents: reduced, + }); + expect((await manager.getToolsForWorkspace(request)).stats.startedServerCount).toBe(2); + expect(await fsPromises.readFile(startsFile, "utf8")).toBe(afterEnabledAddition); + expect(await fsPromises.readFile(siblingStartsFile, "utf8")).toBe(siblingStarts); + if (!baseline) throw new Error("Expected an explicit saved selection"); + await service.setComponents({ + ...mutation, + expectedImportedComponents: reduced, + importedComponents: baseline, + }); + expect((await manager.getToolsForWorkspace(request)).stats.startedServerCount).toBe(3); + const readdedStarts = await fsPromises.readFile(startsFile, "utf8"); + expect(readdedStarts.slice(afterEnabledAddition.length)).toMatch(/^excluded /); + expect(await fsPromises.readFile(siblingStartsFile, "utf8")).toBe(siblingStarts); + expect(await fsPromises.readFile(stateFile, "utf8")).toBe("preserve me"); + expect(await fsPromises.readFile(overridesFile, "utf8")).toBe(JSON.stringify(overrides)); } finally { await manager.stopServersWithKeyPrefix("plugin:"); manager.dispose(); @@ -441,21 +480,25 @@ describe("AgentPluginInstallService", () => { name: "demo-plugin", expectedLockedSha: reviewed.lockedSha, expectedContentHash: reviewed.contentHash, - skills: ["greet"], - mcpServers: ["echo"], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: ["greet"], mcpServers: ["echo"] }, }; - expect((await service.addComponentsResult(request)).success).toBe(false); + expect((await service.setComponentsResult(request)).success).toBe(false); expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(before); const refreshed = await service.getComponents({ name: "demo-plugin" }); expect(refreshed.lockedSha).toBe(reviewed.lockedSha); expect(refreshed.contentHash).not.toBe(reviewed.contentHash); - const accepted = await service.addComponents({ + const accepted = await service.setComponents({ ...request, expectedContentHash: refreshed.contentHash, }); - expect(accepted.importedComponents).toEqual({ skills: ["greet"], mcpServers: ["echo"] }); + expect(accepted.data.importedComponents).toEqual({ skills: ["greet"], mcpServers: ["echo"] }); expect( - await service.addComponents({ ...request, expectedContentHash: refreshed.contentHash }) + await service.setComponents({ + ...request, + expectedContentHash: refreshed.contentHash, + expectedImportedComponents: accepted.data.importedComponents ?? null, + }) ).toEqual(accepted); }); @@ -498,8 +541,8 @@ describe("AgentPluginInstallService", () => { name: "demo-plugin", expectedLockedSha: reviewed.lockedSha, expectedContentHash: reviewed.contentHash, - skills: ["greet"], - mcpServers: ["echo"], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: ["greet"], mcpServers: ["echo"] }, }; if (retarget !== "stable") { await fsPromises.cp(targetA, targetB, { recursive: true }); @@ -508,14 +551,14 @@ describe("AgentPluginInstallService", () => { await fsPromises.unlink(logical); await fsPromises.symlink(targetB, logical, "dir"); const before = await fsPromises.readFile(registryFile(), "utf8"); - expect((await service.addComponentsResult(request)).success).toBe(false); + expect((await service.setComponentsResult(request)).success).toBe(false); expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(before); const refreshed = await service.getComponents({ name: "demo-plugin" }); expect(refreshed.contentHash).not.toBe(reviewed.contentHash); expect(refreshed.mcpServers).toEqual(reviewed.mcpServers); request.expectedContentHash = refreshed.contentHash; } - expect((await service.addComponents(request)).importedComponents).toEqual({ + expect((await service.setComponents(request)).data.importedComponents).toEqual({ skills: ["greet"], mcpServers: ["echo"], }); @@ -530,6 +573,125 @@ describe("AgentPluginInstallService", () => { } ); + test("component inventory does not contend with managed MCP admission", async () => { + const preview = await service.preview({ input: remoteDir }); + await service.install({ source: preview.source, expectedSha: preview.lockedSha }); + const manager = new MCPServerManager( + new MCPConfigService(config, { + agentPluginsMcpProvider: createAgentPluginsMcpProvider({ + xumHome: muxRoot, + isEnabled: () => true, + }), + }), + { + pluginInvalidation: { + keyPrefix: "plugin:", + readToken: () => Promise.resolve(undefined), + readComponentPolicy: () => readPluginMcpPolicy(registryFile()), + tryAcquireComponentPolicyLock: (options) => + acquirePluginMutationLock(muxRoot, { timeoutMs: 0, ...options }), + }, + } + ); + const entered = Promise.withResolvers(); + const resume = Promise.withResolvers(); + const hash = treeHash.hashPluginTree; + let pause = true; + const receipt = spyOn(treeHash, "hashPluginTree").mockImplementation(async (...args) => { + if (pause) { + pause = false; + entered.resolve(); + await resume.promise; + } + return hash(...args); + }); + const exec = mock(() => Promise.reject(new Error("admitted launch reached"))); + const runtime = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + exec, + } as unknown as Runtime); + const inventory = service.getComponents({ name: "demo-plugin" }); + inventory.catch(() => undefined); + try { + await entered.promise; + const key = buildPluginServerKey( + computePluginInstanceId(path.join(pluginsDir(), "demo-plugin")), + "echo" + ); + const result = await manager.test({ projectPath: muxRoot, name: key }); + expect(exec).toHaveBeenCalledTimes(1); + expect(result.success).toBe(false); + if (result.success) throw new Error("Expected the injected launch failure"); + expect(result.error).toContain("admitted launch reached"); + const release = await acquirePluginMutationLock(muxRoot, { timeoutMs: 0 }); + await release(); + resume.resolve(); + expect((await inventory).skills.map((skill) => skill.name)).toEqual(["greet"]); + } finally { + resume.resolve(); + await inventory.catch(() => undefined); + receipt.mockRestore(); + runtime.mockRestore(); + manager.dispose(); + } + }); + + test("component inventory rejects a complete reinstall even when receipt bytes match", async () => { + const preview = await service.preview({ input: remoteDir }); + const install = { source: preview.source, expectedSha: preview.lockedSha }; + await service.install(install); + const reviewed = await service.getComponents({ name: "demo-plugin" }); + const hash = treeHash.hashPluginTree; + let replace = true; + const receipt = spyOn(treeHash, "hashPluginTree").mockImplementation(async (...args) => { + const result = await hash(...args); + if (replace) { + replace = false; + // Fail promptly on the old locked reader rather than deadlocking its + // in-process queue. The real uninstall/reinstall then spans the scan. + const release = await acquirePluginMutationLock(muxRoot, { timeoutMs: 0 }); + await release(); + await service.uninstall({ name: "demo-plugin", deletePluginData: false }); + await service.install(install); + } + return result; + }); + try { + await expect(service.getComponents({ name: "demo-plugin" })).rejects.toThrow( + /changed.*review/i + ); + } finally { + receipt.mockRestore(); + } + const refreshed = await service.getComponents({ name: "demo-plugin" }); + expect(refreshed.contentHash).toBe(reviewed.contentHash); + expect(refreshed.lockedSha).toBe(reviewed.lockedSha); + expect(refreshed.skills).toEqual(reviewed.skills); + }); + + test.each(["before", "during"])( + "component inventory rejects a pending installer journal created %s the scan", + async (when) => { + const preview = await service.preview({ input: remoteDir }); + await service.install({ source: preview.source, expectedSha: preview.lockedSha }); + const journal = path.join(stagingDir(), "update-demo-plugin.json"); + const hash = treeHash.hashPluginTree; + const receipt = spyOn(treeHash, "hashPluginTree").mockImplementation(async (...args) => { + const result = await hash(...args); + if (when === "during") await fsPromises.writeFile(journal, "{}"); + return result; + }); + try { + if (when === "before") await fsPromises.writeFile(journal, "{}"); + await expect(service.getComponents({ name: "demo-plugin" })).rejects.toThrow( + /changed.*review/i + ); + } finally { + receipt.mockRestore(); + await fsPromises.rm(journal, { force: true }); + } + } + ); + test("component inventory stays pinned through an A-to-B-to-A logical-root retarget", async () => { const preview = await service.preview({ input: remoteDir }); await service.install({ @@ -612,12 +774,337 @@ describe("AgentPluginInstallService", () => { expect(await pathExists(path.join(pluginsDir(), "demo-plugin"))).toBe(false); }); - test("concurrent additions union imports across service instances, preserve unknown names and survive restart", async () => { + test.each(["stdio", "http"] as const)( + "named %s admission pins an open policy inode against the real component setter", + async (transport) => { + if (transport === "http") { + await fsPromises.writeFile( + path.join(remoteDir, "mcp.json"), + JSON.stringify({ + $schema: AGENT_PLUGIN_MCP_SCHEMA_ID_1_0_0, + mcpServers: { echo: { type: "streamable-http", url: "https://mcp.example.test/echo" } }, + }) + ); + await commitAll(remoteDir, "HTTP fixture"); + } + const preview = await service.preview({ input: remoteDir }); + const all = { skills: [], mcpServers: ["echo"] }; + await service.install({ + source: preview.source, + expectedSha: preview.lockedSha, + importedComponents: all, + }); + const inventory = await service.getComponents({ name: "demo-plugin" }); + const configService = new MCPConfigService(config, { + agentPluginsMcpProvider: createAgentPluginsMcpProvider({ + xumHome: muxRoot, + isEnabled: () => true, + }), + }); + let finalRead = false; + let lockHeld = false; + const manager = new MCPServerManager(configService, { + pluginInvalidation: { + keyPrefix: "plugin:", + readToken: () => Promise.resolve(undefined), + readComponentPolicy: () => { + finalRead = true; + return readPluginMcpPolicy(registryFile()).finally(() => { + finalRead = false; + }); + }, + tryAcquireComponentPolicyLock: async (options) => { + const release = await acquirePluginMutationLock(muxRoot, { timeoutMs: 0, ...options }); + lockHeld = true; + return async () => { + await release(); + lockHeld = false; + }; + }, + }, + }); + const opened = Promise.withResolvers(); + const resumeRead = Promise.withResolvers(); + const launched = Promise.withResolvers(); + const finishLaunch = Promise.withResolvers(); + finishLaunch.promise.catch(() => undefined); + const writerAttempted = Promise.withResolvers(); + const readFile = fsPromises.readFile; + let interceptRead = true; + const readSpy = spyOn(fsPromises, "readFile").mockImplementation((async ( + ...args: Parameters + ) => { + if (interceptRead && finalRead && args[0] === registryFile()) { + interceptRead = false; + const handle = await fsPromises.open(registryFile(), "r"); + try { + opened.resolve(); + await resumeRead.promise; + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } + } + return readFile(...args); + }) as typeof fsPromises.readFile); + let observeWriter = false; + const link = fsPromises.link; + const linkSpy = spyOn(fsPromises, "link").mockImplementation(async (from, to) => { + try { + await link(from, to); + if (observeWriter && to === path.join(stagingDir(), MUTATION_LOCK_FILE)) + writerAttempted.resolve(false); + } catch (error) { + if (observeWriter && to === path.join(stagingDir(), MUTATION_LOCK_FILE)) + writerAttempted.resolve(true); + throw error; + } + }); + let setterFinished = false; + const initiate = mock(() => { + expect(lockHeld).toBe(true); + expect(setterFinished).toBe(false); + launched.resolve(); + return finishLaunch.promise; + }); + const runtime = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + exec: initiate, + } as unknown as Runtime); + const client = spyOn(mcpSdk, "createMCPClient").mockImplementation(initiate); + const key = buildPluginServerKey( + computePluginInstanceId(path.join(pluginsDir(), "demo-plugin")), + "echo" + ); + let setter: Promise | undefined; + const pending = manager.test({ projectPath: muxRoot, name: key }); + try { + await opened.promise; + observeWriter = true; + setter = service + .setComponents({ + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: all, + importedComponents: { skills: [], mcpServers: [] }, + }) + .then((result) => { + setterFinished = true; + return result; + }); + expect(await writerAttempted.promise).toBe(true); + expect(initiate).not.toHaveBeenCalled(); + expect(setterFinished).toBe(false); + resumeRead.resolve(); + await launched.promise; + // The setter can finish while the connection is still pending: admission + // owns the lock only through the real synchronous exec/client initiation. + await setter; + expect(lockHeld).toBe(false); + expect((await service.getComponents({ name: "demo-plugin" })).importedComponents).toEqual({ + skills: [], + mcpServers: [], + }); + expect(initiate).toHaveBeenCalledTimes(1); + } finally { + resumeRead.resolve(); + finishLaunch.reject(new Error("connection probe complete")); + await pending; + await setter; + runtime.mockRestore(); + client.mockRestore(); + readSpy.mockRestore(); + linkSpy.mockRestore(); + manager.dispose(); + } + } + ); + + test("replacement selections remove, mix, empty, and reject stale baselines without rewriting identity", async () => { const preview = await service.preview({ input: remoteDir }); + const all = { skills: ["greet"], mcpServers: ["echo"] }; + const installed = await service.install({ + source: preview.source, + expectedSha: preview.lockedSha, + importedComponents: all, + }); + const inventory = await service.getComponents({ name: installed.name }); + const request = { + name: installed.name, + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: all, + }; + const epoch = await fsPromises.readFile(path.join(stagingDir(), "mutation-epoch"), "utf8"); + for (const importedComponents of [ + { skills: ["greet"], mcpServers: [] }, + { skills: [], mcpServers: ["echo"] }, + { skills: [], mcpServers: [] }, + ]) { + const result = await service.setComponentsResult({ ...request, importedComponents }); + expect(result).toEqual({ success: true, data: { ...installed, importedComponents } }); + const before = await fsPromises.readFile(registryFile(), "utf8"); + expect( + (await service.setComponentsResult({ ...request, importedComponents: all })).success + ).toBe(false); + expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(before); + request.expectedImportedComponents = importedComponents; + } + expect(await fsPromises.readFile(path.join(stagingDir(), "mutation-epoch"), "utf8")).toBe( + epoch + ); + expect( + await pathExists(path.join(pluginsDir(), installed.name, "skills", "greet", "SKILL.md")) + ).toBe(true); + expect((await service.list())[0]).toMatchObject({ + managed: true, + present: true, + importedSkillCount: 0, + importedMcpServerCount: 0, + }); + expect( + ( + await service.setComponentsResult({ + ...request, + expectedImportedComponents: null, + importedComponents: all, + }) + ).success + ).toBe(false); + }); + + test.each([false, true])( + "effective component no-op leaves disk and runtime untouched (explicit: %s)", + async (explicit) => { + const preview = await service.preview({ input: remoteDir }); + const all = { skills: ["greet"], mcpServers: ["echo"] }; + await service.install({ + source: preview.source, + expectedSha: preview.lockedSha, + ...(explicit ? { importedComponents: all } : {}), + }); + const manager = new MCPServerManager(new MCPConfigService(config)); + const withRuntime = new AgentPluginInstallService(config, { + isEnabled: () => true, + mcpServerManager: manager, + }); + const inventory = await withRuntime.getComponents({ name: "demo-plugin" }); + const before = await fsPromises.readFile(registryFile(), "utf8"); + const empty = { skills: [], mcpServers: [] }; + const reconcile = spyOn(manager, "reconcilePluginComponents").mockImplementation(async () => { + // Cleanup must run after releasing the component writer lock. + const release = await acquirePluginMutationLock(muxRoot, { timeoutMs: 0 }); + await release(); + expect( + (await withRuntime.getComponents({ name: "demo-plugin" })).importedComponents + ).toEqual(empty); + }); + const write = spyOn( + withRuntime as unknown as { writeRegistry: () => Promise }, + "writeRegistry" + ); + const request = { + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: explicit ? all : null, + }; + try { + const unchanged = await withRuntime.setComponentsResult({ + ...request, + importedComponents: { skills: ["greet", "greet"], mcpServers: ["echo", "echo"] }, + }); + expect(unchanged).toMatchObject({ success: true }); + expect(write).not.toHaveBeenCalled(); + expect(reconcile).not.toHaveBeenCalled(); + expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(before); + const changed = await withRuntime.setComponentsResult({ + ...request, + importedComponents: empty, + }); + expect(changed).toMatchObject({ success: true, data: { importedComponents: empty } }); + expect(changed).not.toHaveProperty("cleanupWarning"); + expect(write).toHaveBeenCalledTimes(1); + expect(reconcile).toHaveBeenCalledTimes(1); + } finally { + write.mockRestore(); + reconcile.mockRestore(); + manager.dispose(); + } + } + ); + + test("cleanup failures report saved selections and reconciliation runs outside the mutation lock", async () => { + const preview = await service.preview({ input: remoteDir }); + const all = { skills: ["greet"], mcpServers: ["echo"] }; await service.install({ source: preview.source, expectedSha: preview.lockedSha, - importedComponents: { skills: [], mcpServers: [] }, + importedComponents: all, + }); + const manager = new MCPServerManager(new MCPConfigService(config)); + const withRuntime = new AgentPluginInstallService(config, { + isEnabled: () => true, + mcpServerManager: manager, + }); + const inventory = await withRuntime.getComponents({ name: "demo-plugin" }); + const empty = { skills: [], mcpServers: [] }; + const request = { + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: all, + importedComponents: empty, + }; + const reconcile = spyOn(manager, "reconcilePluginComponents").mockImplementationOnce( + async () => { + // Cleanup must run after releasing the component writer lock. + const release = await acquirePluginMutationLock(muxRoot, { timeoutMs: 0 }); + await release(); + expect( + (await withRuntime.getComponents({ name: request.name })).importedComponents + ).toEqual(empty); + throw new Error("client close failed"); + } + ); + const write = spyOn( + withRuntime as unknown as { writeRegistry: () => Promise }, + "writeRegistry" + ).mockImplementationOnce(() => Promise.reject(new Error("disk full"))); + try { + expect(await withRuntime.setComponentsResult(request)).toEqual({ + success: false, + error: "disk full", + }); + expect(reconcile).not.toHaveBeenCalled(); + const saved = await withRuntime.setComponentsResult(request); + expect(saved).toMatchObject({ success: true, data: { importedComponents: empty } }); + if (!saved.success) throw new Error("Expected the selection to remain saved"); + expect(saved.cleanupWarning).toContain("client close failed"); + expect((await withRuntime.getComponents({ name: request.name })).importedComponents).toEqual( + empty + ); + const readded = await withRuntime.setComponentsResult({ + ...request, + expectedImportedComponents: empty, + importedComponents: all, + }); + expect(readded).toMatchObject({ success: true, data: { importedComponents: all } }); + expect(reconcile).toHaveBeenCalledTimes(2); + } finally { + write.mockRestore(); + reconcile.mockRestore(); + manager.dispose(); + } + }); + + test("concurrent replacements reject stale selections, normalize sets and preserve unknown registry fields", async () => { + const preview = await service.preview({ input: remoteDir }); + const initial = { skills: [], mcpServers: [] }; + await service.install({ + source: preview.source, + expectedSha: preview.lockedSha, + importedComponents: initial, }); const [entry] = (await registry()) as Array>; await fsPromises.writeFile( @@ -627,73 +1114,75 @@ describe("AgentPluginInstallService", () => { plugins: [ { ...entry, - futureField: { preserve: true }, - importedComponents: { - skills: ["removed-skill"], - mcpServers: ["removed-server"], - futureSelection: true, - }, + futureField: true, + importedComponents: { ...initial, futureSelection: true }, }, ], }) ); const other = new AgentPluginInstallService(config, { isEnabled: () => true }); + const inventory = await service.getComponents({ name: "demo-plugin" }); const args = { name: "demo-plugin", - expectedLockedSha: preview.lockedSha, - expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: initial, }; - const epoch = await fsPromises.readFile(path.join(stagingDir(), "mutation-epoch"), "utf8"); - await Promise.all([ - service.addComponents({ ...args, skills: ["greet", "greet"], mcpServers: [] }), - other.addComponents({ ...args, skills: [], mcpServers: ["echo", "echo"] }), + const results = await Promise.all([ + service.setComponentsResult({ + ...args, + importedComponents: { skills: ["greet", "greet"], mcpServers: ["echo"] }, + }), + other.setComponentsResult({ + ...args, + importedComponents: { skills: ["greet"], mcpServers: [] }, + }), ]); - const importedComponents = { - skills: ["greet", "removed-skill"], - mcpServers: ["echo", "removed-server"], - }; - expect((await service.getComponents({ name: args.name })).importedComponents).toEqual( - importedComponents - ); + expect(results.filter((result) => result.success)).toHaveLength(1); + expect(results.filter((result) => !result.success)).toHaveLength(1); + const winner = results.find((result) => result.success); + if (!winner?.success || !winner.data.importedComponents) + throw new Error("Expected one saved replacement"); + const selection = winner.data.importedComponents; const saved = await fsPromises.readFile(registryFile(), "utf8"); expect(JSON.parse(saved)).toMatchObject({ futureEnvelope: true, - plugins: [ - { - futureField: { preserve: true }, - importedComponents: { ...importedComponents, futureSelection: true }, - }, - ], + plugins: [{ futureField: true, importedComponents: { ...selection, futureSelection: true } }], }); - await service.addComponents({ ...args, skills: ["greet"], mcpServers: ["echo"] }); + expect( + ( + await service.setComponentsResult({ + ...args, + expectedImportedComponents: { ...selection, skills: ["greet", "greet"] }, + importedComponents: selection, + }) + ).success + ).toBe(true); expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(saved); - // Selection additions must not publish the destructive epoch that retires unrelated MCP servers. - expect(await fsPromises.readFile(path.join(stagingDir(), "mutation-epoch"), "utf8")).toBe( - epoch - ); const restarted = new AgentPluginInstallService(config, { isEnabled: () => true }); expect((await restarted.getComponents({ name: args.name })).importedComponents).toEqual( - importedComponents + selection ); - const stale = await restarted.addComponentsResult({ - ...args, - expectedLockedSha: "e".repeat(40), - skills: [], - mcpServers: [], - }); - expect(stale.success).toBe(false); - if (!stale.success) expect(stale.error).toMatch(/changed since/); - expect( - (await restarted.addComponentsResult({ ...args, skills: ["missing"], mcpServers: [] })) - .success - ).toBe(false); + for (const importedComponents of [ + { skills: ["missing"], mcpServers: [] }, + { skills: [], mcpServers: ["missing"] }, + ]) { + expect( + ( + await restarted.setComponentsResult({ + ...args, + expectedImportedComponents: selection, + importedComponents, + }) + ).success + ).toBe(false); + } expect( ( - await restarted.addComponentsResult({ + await restarted.setComponentsResult({ ...args, name: "not-installed", - skills: [], - mcpServers: [], + importedComponents: initial, }) ).success ).toBe(false); @@ -722,12 +1211,12 @@ describe("AgentPluginInstallService", () => { expect((await service.getComponentsResult({ name: "demo-plugin" })).success).toBe(false); expect( ( - await service.addComponentsResult({ + await service.setComponentsResult({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: contentHash, - skills: [], - mcpServers: ["echo"], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: [], mcpServers: ["echo"] }, }) ).success ).toBe(false); @@ -782,19 +1271,19 @@ describe("AgentPluginInstallService", () => { await writePluginFixture(remoteDir, { version: "2.0.0" }); const nextSha = await commitAll(remoteDir, "metadata update"); const [added, updated] = await Promise.all([ - service.addComponents({ + service.setComponents({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: ["greet"], - mcpServers: [], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: ["greet"], mcpServers: [] }, }), service.update({ name: "demo-plugin" }), ]); - expect(added.importedComponents).toEqual({ skills: ["greet"], mcpServers: [] }); + expect(added.data.importedComponents).toEqual({ skills: ["greet"], mcpServers: [] }); expect(updated.lockedSha).toBe(nextSha); expect((await service.getComponents({ name: "demo-plugin" })).importedComponents).toEqual( - added.importedComponents + added.data.importedComponents ); }); @@ -815,11 +1304,11 @@ describe("AgentPluginInstallService", () => { name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: ["greet"], - mcpServers: ["echo"], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: ["greet"], mcpServers: ["echo"] }, }; try { - expect(await service.addComponentsResult(args)).toEqual({ + expect(await service.setComponentsResult(args)).toEqual({ success: false, error: "disk full", }); @@ -834,7 +1323,7 @@ describe("AgentPluginInstallService", () => { } finally { writeSpy.mockRestore(); } - expect((await service.addComponents(args)).importedComponents).toEqual({ + expect((await service.setComponents(args)).data.importedComponents).toEqual({ skills: ["greet"], mcpServers: ["echo"], }); @@ -870,31 +1359,80 @@ describe("AgentPluginInstallService", () => { expect(updated.importedComponents).toEqual({ skills: ["greet"], mcpServers: [] }); expect( ( - await service.addComponentsResult({ + await service.setComponentsResult({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: [], - mcpServers: ["echo"], + expectedImportedComponents: { skills: [], mcpServers: [] }, + importedComponents: { skills: [], mcpServers: ["echo"] }, }) ).success ).toBe(false); - expect((await service.getComponents({ name: "demo-plugin" })).skills).toEqual([]); + const inventory = await service.getComponents({ name: "demo-plugin" }); + expect(inventory.skills).toEqual([]); + expect(inventory.importedComponents).toEqual({ skills: ["greet"], mcpServers: [] }); + const request = { + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: inventory.importedComponents ?? null, + }; + // Update preservation does not authorize importing an unavailable name on a later explicit save. + expect( + ( + await service.setComponentsResult({ + ...request, + importedComponents: { skills: ["greet"], mcpServers: ["echo"] }, + }) + ).success + ).toBe(false); + expect((await service.getComponents({ name: request.name })).importedComponents).toEqual( + inventory.importedComponents + ); + const selection = { skills: [], mcpServers: ["echo"] }; + expect( + await service.setComponentsResult({ ...request, importedComponents: selection }) + ).toMatchObject({ + success: true, + data: { importedComponents: selection }, + }); + await writePluginFixture(remoteDir, { version: "3.0.0" }); + await commitAll(remoteDir, "restore the removed skill"); + const restored = await service.previewUpdate({ name: request.name }); + await service.update({ + name: request.name, + consent: { fromSha: restored.fromSha, toSha: restored.toSha }, + }); + const after = await service.getComponents({ name: request.name }); + expect(after.skills.map((skill) => skill.name)).toEqual(["greet"]); + expect(after.importedComponents).toEqual(selection); }); - test("additions to legacy installs remain import-all on disk", async () => { + test("unchanged legacy selection remains import-all; changing it becomes explicit", async () => { const preview = await service.preview({ input: remoteDir }); await service.install({ source: preview.source, expectedSha: preview.lockedSha }); const before = await fsPromises.readFile(registryFile(), "utf8"); - const updated = await service.addComponents({ + const updated = await service.setComponents({ name: "demo-plugin", expectedLockedSha: preview.lockedSha, expectedContentHash: (await service.getComponents({ name: "demo-plugin" })).contentHash, - skills: ["greet"], - mcpServers: [], + expectedImportedComponents: null, + importedComponents: { skills: ["greet"], mcpServers: ["echo"] }, }); - expect(updated.importedComponents).toBeUndefined(); + expect(updated.data.importedComponents).toBeUndefined(); expect(await fsPromises.readFile(registryFile(), "utf8")).toBe(before); + const inventory = await service.getComponents({ name: "demo-plugin" }); + const changed = await service.setComponentsResult({ + name: "demo-plugin", + expectedLockedSha: inventory.lockedSha, + expectedContentHash: inventory.contentHash, + expectedImportedComponents: null, + importedComponents: { skills: [], mcpServers: [] }, + }); + expect(changed).toMatchObject({ + success: true, + data: { importedComponents: { skills: [], mcpServers: [] } }, + }); }); test("consent preview discloses symlinked skills and warns on escaping symlinks", async () => { diff --git a/src/node/services/agentPlugins/installService.ts b/src/node/services/agentPlugins/installService.ts index 99f90eeaee..5c250940cd 100644 --- a/src/node/services/agentPlugins/installService.ts +++ b/src/node/services/agentPlugins/installService.ts @@ -50,7 +50,6 @@ import { import { MAX_FILE_SIZE } from "@/node/services/tools/fileCommon"; import { ensurePathContained, hasErrorCode } from "@/node/services/tools/skillFileUtils"; import { raceWithAbortAndTimeout } from "@/node/utils/concurrency/withTimeout"; -import { acquireCrossProcessLock } from "@/node/utils/main/crossProcessLock"; import { shellQuote } from "@/common/utils/shell"; import { execFileAsync } from "@/node/utils/disposableExec"; import { @@ -62,7 +61,9 @@ import { type AgentPluginInfo, } from "./discovery"; import { + acquirePluginMutationLock, bumpContainerMutationEpoch, + MUTATION_LOCK_FILE, isJournalName, JOURNAL_PREFIXES, MUTATION_EPOCH_FILE, @@ -148,20 +149,8 @@ const PROMOTION_MARKER_FILE = ".mux-promotion-marker"; /** Staging dirs left behind by crashes are reclaimed after this age. */ const STALE_STAGING_MAX_AGE_MS = 60 * 60 * 1000; -/** - * Cross-process mutation lock file in the staging root. The in-process - * mutationQueue serializes one service instance, but two processes sharing - * the same rootDir (ALLOW_MULTIPLE_INSTANCES, a desktop app alongside `mux - * server`) each have their own queue: two concurrent mutations could both - * read the same plugins.json snapshot and the later atomic write would - * silently drop the earlier one's entry. Every mutation transaction - * (registry read → directory moves → registry write) holds this lock. - */ -const MUTATION_LOCK_FILE = "mutation.lock"; /** How long an acquire waits on a live holder before failing (covers a full clone). */ const MUTATION_LOCK_ACQUIRE_TIMEOUT_MS = 10 * 60 * 1000; -/** Pid-reuse guard: no plugin mutation legitimately runs this long. */ -const MUTATION_LOCK_STALE_MS = 30 * 60 * 1000; /** Bound discovery/settings waits on startup crash-recovery I/O. */ const JOURNAL_RECONCILIATION_TIMEOUT_MS = 30_000; @@ -713,12 +702,8 @@ export class AgentPluginInstallService { // process sharing rootDir must not interleave its read-modify-write of // plugins.json (or its directory moves) with ours. const locked = async (): Promise => { - const release = await acquireCrossProcessLock({ - lockPath: path.join(this.stagingRoot, MUTATION_LOCK_FILE), - acquireTimeoutMs: MUTATION_LOCK_ACQUIRE_TIMEOUT_MS, - staleMs: MUTATION_LOCK_STALE_MS, - timeoutMessage: - "Another Mux process is currently modifying plugins. Wait for it to finish and try again.", + const release = await acquirePluginMutationLock(this.config.rootDir, { + timeoutMs: MUTATION_LOCK_ACQUIRE_TIMEOUT_MS, }); try { return await fn(); @@ -1774,8 +1759,9 @@ export class AgentPluginInstallService { return this.captureResult(() => this.getComponents(args)); } - addComponentsResult(args: Parameters[0]) { - return this.captureResult(() => this.addComponents(args)); + async setComponentsResult(args: Parameters[0]) { + const result = await this.captureResult(() => this.setComponents(args)); + return result.success ? { success: true as const, ...result.data } : result; } listResult() { @@ -2715,22 +2701,38 @@ export class AgentPluginInstallService { async getComponents(args: { name: string }): Promise { this.assertEnabled(); - return this.runExclusive(async () => { - const entry = (await this.readRegistry("strict")).find((entry) => entry.name === args.name); - if (entry === undefined) throw new Error(`No managed plugin named '${args.name}'.`); - return this.readInstalledComponents(entry); - }); + // Inventory must not hold the writer lock over full-tree hashes and deny + // live MCP admission. Reuse discovery's journal/epoch bracket to reject + // overlapping installer moves, including a complete rollback or reinstall. + const gate = await journalDerivedDiscoveryGate([this.containerDir]); + const changed = () => + new Error( + "Installed plugin files changed during component review. Refresh the component inventory." + ); + if (gate.suppressed.length > 0) throw changed(); + const entry = (await this.readRegistry("strict")).find((entry) => entry.name === args.name); + if (entry === undefined) throw new Error(`No managed plugin named '${args.name}'.`); + const inventory = await this.readInstalledComponents(entry); + if ((await gate.confirm()).length > 0) throw changed(); + return inventory; } - async addComponents( - args: AgentPluginImportedComponents & { - name: string; - expectedLockedSha: string; - expectedContentHash: string; - } - ): Promise { + async setComponents(args: { + name: string; + expectedLockedSha: string; + expectedContentHash: string; + expectedImportedComponents: AgentPluginImportedComponents | null; + importedComponents: AgentPluginImportedComponents; + }): Promise<{ data: AgentPluginInstallEntry; cleanupWarning?: string }> { this.assertEnabled(); - return this.runExclusive(async () => { + const selectionKey = (selection: AgentPluginImportedComponents | null) => + selection === null + ? null + : JSON.stringify([ + [...new Set(selection.skills)].sort(), + [...new Set(selection.mcpServers)].sort(), + ]); + const { data, changed } = await this.runExclusive(async () => { const { envelope, rawEntries } = await this.readRegistryDocument("strict"); const entry = this.parseRegistryEntries(rawEntries, "strict").find( (entry) => entry.name === args.name @@ -2738,23 +2740,27 @@ export class AgentPluginInstallService { if (entry === undefined) throw new Error(`No readable managed plugin named '${args.name}'.`); if (entry.lockedSha !== args.expectedLockedSha) throw new Error("Plugin changed since component review. Refresh the component inventory."); + if ( + selectionKey(entry.importedComponents ?? null) !== + selectionKey(args.expectedImportedComponents) + ) + throw new Error( + "Plugin selection changed since component review. Refresh the component inventory." + ); const inventory = await this.readInstalledComponents(entry); if (inventory.contentHash !== args.expectedContentHash) { throw new Error( "Plugin files changed since component review. Refresh the component inventory." ); } - const added = this.validateComponentImports(args, inventory); - // Legacy installs already import everything; do not silently convert their update behavior. - if (entry.importedComponents === undefined) return entry; - const importedComponents = { - skills: [...new Set([...entry.importedComponents.skills, ...added.skills])].sort(), - mcpServers: [ - ...new Set([...entry.importedComponents.mcpServers, ...added.mcpServers]), - ].sort(), + const importedComponents = this.validateComponentImports(args.importedComponents, inventory); + // An unchanged legacy import-all selection must keep following future package inventory. + const previous = entry.importedComponents ?? { + skills: inventory.skills.map((skill) => skill.name), + mcpServers: inventory.mcpServers.map((server) => server.serverName), }; - if (JSON.stringify(importedComponents) === JSON.stringify(entry.importedComponents)) - return entry; + if (selectionKey(importedComponents) === selectionKey(previous)) + return { data: entry, changed: false }; await this.writeRegistry( envelope, rawEntries.map((raw) => { @@ -2769,10 +2775,19 @@ export class AgentPluginInstallService { }; }) ); - // Fresh discovery reads publish these additive imports. Do NOT bump the tree mutation - // epoch: it recycles every plugin server. An overlapping scan sees only a safe older subset. - return { ...entry, importedComponents }; + return { data: { ...entry, importedComponents }, changed: true }; }); + if (!changed) return { data }; + // Persist first and release the mutation lock: reconciliation rediscovers current policy. + // Never recycle the tree or prune workspace preferences for reversible selections. + try { + await this.deps.mcpServerManager?.reconcilePluginComponents(); + return { data }; + } catch (error) { + const cleanupWarning = `Components saved, but MCP cleanup needs retry: ${getErrorMessage(error)}`; + log.warn(cleanupWarning, { name: args.name }); + return { data, cleanupWarning }; + } } /** Managed registry entries merged with unmanaged plugins found by global discovery. */ diff --git a/src/node/services/agentPlugins/journals.ts b/src/node/services/agentPlugins/journals.ts index 948f1fa48f..40813534fd 100644 --- a/src/node/services/agentPlugins/journals.ts +++ b/src/node/services/agentPlugins/journals.ts @@ -16,11 +16,31 @@ import { randomUUID } from "node:crypto"; import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; +import { acquireCrossProcessLock } from "@/node/utils/main/crossProcessLock"; import { hasErrorCode } from "@/node/services/tools/skillFileUtils"; /** Staging dir name under the mux home dir — NOT under ~/.mux/plugins, which discovery scans. */ export const STAGING_DIR_NAME = "plugin-staging"; +/** Shared by installer transactions and MCP's final component-policy admission. */ +export const MUTATION_LOCK_FILE = "mutation.lock"; +/** Pid-reuse guard; the shared lock renews the lease while a holder is alive. */ +const MUTATION_LOCK_STALE_MS = 30 * 60 * 1000; + +export function acquirePluginMutationLock( + rootDir: string, + options: { timeoutMs: number; signal?: AbortSignal } +): Promise<() => Promise> { + return acquireCrossProcessLock({ + lockPath: path.join(rootDir, STAGING_DIR_NAME, MUTATION_LOCK_FILE), + acquireTimeoutMs: options.timeoutMs, + staleMs: MUTATION_LOCK_STALE_MS, + signal: options.signal, + timeoutMessage: + "Another Mux process is currently modifying plugins. Wait for it to finish and try again.", + }); +} + /** * Mutation-epoch handshake file in the staging root. The install service * rewrites it with a fresh random token immediately BEFORE deleting any diff --git a/src/node/services/agentPlugins/mcpConfig.test.ts b/src/node/services/agentPlugins/mcpConfig.test.ts index 0c8aedf2b6..767abd926f 100644 --- a/src/node/services/agentPlugins/mcpConfig.test.ts +++ b/src/node/services/agentPlugins/mcpConfig.test.ts @@ -10,6 +10,7 @@ import { DisposableTempDir } from "@/node/services/tempDir"; import { Config } from "@/node/config"; import { MCPConfigService } from "@/node/services/mcpConfigService"; import { MCPServerManager } from "@/node/services/mcpServerManager"; +import { readPluginMcpPolicy } from "./registry"; import { createTestPluginInstallEntry } from "./testFixtures"; import type { AgentPluginInfo } from "./discovery"; import { AGENT_PLUGIN_SCHEMA_ID_1_0_0 } from "./manifest"; @@ -699,7 +700,14 @@ describe("createAgentPluginsMcpProvider", () => { }) ); const manager = new MCPServerManager( - new MCPConfigService(new Config(xumHome), { agentPluginsMcpProvider: provider }) + new MCPConfigService(new Config(xumHome), { agentPluginsMcpProvider: provider }), + { + pluginInvalidation: { + keyPrefix: "plugin:", + readToken: () => Promise.resolve(undefined), + readComponentPolicy: () => readPluginMcpPolicy(registryPath), + }, + } ); try { const servers = await manager.listServers(home.path, overrides, true, context); @@ -727,6 +735,18 @@ describe("createAgentPluginsMcpProvider", () => { expect(global[globalKey]?.plugin?.serverName).toBe("allowed"); expect(loaded[keyFor("allowed")]?.plugin?.sourceScope).toBe("project"); expect(loaded[keyFor("blocked")]).toBeUndefined(); + for (const info of Object.values(loaded).filter( + (info) => info.plugin?.serverName === "allowed" + )) { + expect(info.plugin?.componentPolicy).toEqual({ + registryPath: path.join(physicalHome, "plugins.json"), + name: "managed", + }); + } + await fs.unlink(registryPath); + expect( + Object.keys(await manager.listServers(home.path, overrides, true, context)) + ).toEqual([keyFor("unmanaged")]); await fs.writeFile(registryPath, "{"); expect( Object.keys(await manager.listServers(home.path, overrides, true, context)) @@ -1020,3 +1040,42 @@ describe("resolveAgentPluginsMcpContext", () => { ).toBeNull(); }); }); + +test("MCP discovery rechecks selection after loading component files", async () => { + using tmp = new DisposableTempDir("plugin-mcp-discovery-policy"); + await withHomeDir(tmp.path, async () => { + const xumHome = path.join(tmp.path, ".xum"); + await writeDiscoverablePlugin( + path.join(xumHome, "plugins"), + "demo", + mcpDoc({ removed: STDIO_ENTRY }) + ); + const registry = path.join(xumHome, "plugins.json"); + await fs.writeFile( + registry, + JSON.stringify({ plugins: [createTestPluginInstallEntry("demo")] }) + ); + const original = fs.open; + const open = spyOn(fs, "open").mockImplementation( + async (...args: Parameters) => { + const handle = await original(...args); + if (String(args[0]).endsWith("mcp.json")) { + await fs.writeFile( + `${registry}.tmp`, + JSON.stringify({ + plugins: [createTestPluginInstallEntry("demo", { skills: [], mcpServers: [] })], + }) + ); + await fs.rename(`${registry}.tmp`, registry); + } + return handle; + } + ); + try { + const provider = createAgentPluginsMcpProvider({ xumHome, isEnabled: () => true }); + expect(await provider({ trusted: false })).toEqual({}); + } finally { + open.mockRestore(); + } + }); +}); diff --git a/src/node/services/agentPlugins/mcpConfig.ts b/src/node/services/agentPlugins/mcpConfig.ts index 32597ddd05..d7baa54e4e 100644 --- a/src/node/services/agentPlugins/mcpConfig.ts +++ b/src/node/services/agentPlugins/mcpConfig.ts @@ -1,3 +1,8 @@ +import { + isPluginMcpServerAllowed, + readPluginMcpPolicy, + PLUGIN_REGISTRY_FILE_NAME, +} from "./registry"; import { createHash } from "node:crypto"; import { constants as fsConstants } from "node:fs"; import * as fsPromises from "node:fs/promises"; @@ -668,6 +673,7 @@ export async function loadPluginMcpServers( "loadPluginMcpServers: normalized info must carry provenance" ); info.plugin.serverName = serverName; + if (plugin.componentPolicy !== undefined) info.plugin.componentPolicy = plugin.componentPolicy; servers[buildPluginServerKey(instanceId, serverName)] = info; } @@ -816,6 +822,14 @@ export function createAgentPluginsMcpProvider(ctx: { } catch (error) { log.warn(`Agent Plugins MCP discovery failed: ${getErrorMessage(error)}`); } + // Descriptor reads await plugin files after discovery's registry read. + // Revoke against current consent before exposing any managed registration. + if (Object.values(merged).some((info) => info.plugin?.componentPolicy !== undefined)) { + const policy = await readPluginMcpPolicy(path.join(ctx.xumHome, PLUGIN_REGISTRY_FILE_NAME)); + for (const [name, info] of Object.entries(merged)) { + if (!isPluginMcpServerAllowed(info.plugin, policy)) delete merged[name]; + } + } return merged; }; } diff --git a/src/node/services/agentPlugins/registry.test.ts b/src/node/services/agentPlugins/registry.test.ts index 126279be81..906fe77235 100644 --- a/src/node/services/agentPlugins/registry.test.ts +++ b/src/node/services/agentPlugins/registry.test.ts @@ -1,8 +1,8 @@ import * as fs from "node:fs/promises"; import * as path from "node:path"; -import { describe, expect, test } from "bun:test"; +import { describe, expect, spyOn, test } from "bun:test"; import { DisposableTempDir } from "@/node/services/tempDir"; -import { readPluginComponentImports } from "./registry"; +import { readPluginComponentImports, readPluginMcpPolicy } from "./registry"; import { createTestPluginInstallEntry } from "./testFixtures"; const legacy = createTestPluginInstallEntry("demo"); @@ -86,3 +86,57 @@ describe("readPluginComponentImports", () => { expect(recovered.byName.get("demo")).toEqual(selective.importedComponents); }); }); + +describe("readPluginMcpPolicy", () => { + test("canonical snapshot ignores skills, metadata, ordering, and duplicate selected names", async () => { + using tmp = new DisposableTempDir("mcp-policy-content"); + await fs.mkdir(path.join(tmp.path, "plugins")); + const registryPath = path.join(tmp.path, "plugins.json"); + await readRows(registryPath, [ + legacy, + createTestPluginInstallEntry("other", { skills: [], mcpServers: ["b", "a", "a"] }), + ]); + const before = await readPluginMcpPolicy(registryPath); + await readRows(registryPath, [ + createTestPluginInstallEntry("other", { skills: ["new"], mcpServers: ["a", "b"] }), + { ...legacy, lockedSha: "new" }, + ]); + expect(await readPluginMcpPolicy(registryPath)).toEqual(before); + await readRows(registryPath, [ + legacy, + createTestPluginInstallEntry("other", { skills: [], mcpServers: ["b"] }), + ]); + expect(await readPluginMcpPolicy(registryPath)).not.toEqual(before); + }); + + test("owner retarget during the registry read cannot authorize the old canonical owner", async () => { + using tmp = new DisposableTempDir("mcp-policy-owner"); + const a = path.join(tmp.path, "a"); + const b = path.join(tmp.path, "b"); + const alias = path.join(tmp.path, "alias"); + for (const home of [a, b]) { + await fs.mkdir(path.join(home, "plugins"), { recursive: true }); + await readRows(path.join(home, "plugins.json"), [legacy]); + } + await fs.symlink(a, alias, "dir"); + const original = fs.readFile; + const read = spyOn(fs, "readFile").mockImplementation( + // The forwarding wrapper preserves readFile's encoding-dependent overloads. + (async (...args: Parameters) => { + const value = await original(...args); + await fs.unlink(alias); + await fs.symlink(b, alias, "dir"); + return value; + }) as typeof fs.readFile + ); + try { + expect((await readPluginMcpPolicy(path.join(alias, "plugins.json"))).imports).toBeNull(); + expect(read).toHaveBeenCalledTimes(1); + } finally { + read.mockRestore(); + } + expect((await readPluginMcpPolicy(path.join(alias, "plugins.json"))).registryPath).toBe( + path.join(b, "plugins.json") + ); + }); +}); diff --git a/src/node/services/agentPlugins/registry.ts b/src/node/services/agentPlugins/registry.ts index c46f5e33b4..7b52b633a9 100644 --- a/src/node/services/agentPlugins/registry.ts +++ b/src/node/services/agentPlugins/registry.ts @@ -1,4 +1,6 @@ +import type { MCPServerPluginProvenance } from "@/common/types/mcp"; import * as fsPromises from "node:fs/promises"; +import * as path from "node:path"; import { AgentPluginInstallEntrySchema, type AgentPluginImportedComponents, @@ -127,3 +129,61 @@ export async function readPluginComponentImports(registryFile: string): Promise< return null; } } + +/** MCP-only content snapshot: skills/metadata changes must not recycle MCP clients. */ +export interface PluginMcpPolicy { + registryPath: string; + imports: Record | null; +} + +export function isPluginMcpServerAllowed( + plugin: MCPServerPluginProvenance | undefined, + policy: PluginMcpPolicy | undefined +): boolean { + if (plugin?.componentPolicy === undefined) return true; + const owner = plugin.componentPolicy; + if ( + policy?.registryPath !== owner.registryPath || + policy.imports == null || + !Object.hasOwn(policy.imports, owner.name) + ) + return false; + const selected = policy.imports[owner.name]; + return selected === null || selected.includes(plugin.serverName); +} + +export async function readPluginMcpPolicy(registryFile: string): Promise { + try { + const home = path.dirname(registryFile); + const owner = await fsPromises.realpath(home); + const container = await fsPromises.realpath(path.join(owner, "plugins")); + const registryPath = path.join(owner, path.basename(registryFile)); + const selection = await readPluginComponentImports(registryPath); + // The registry belongs to the pinned owner, not to an alias that can move + // during the read. Recheck both logical and canonical bindings. + if ( + (await fsPromises.realpath(home)) !== owner || + (await fsPromises.realpath(owner)) !== owner || + (await fsPromises.realpath(path.join(home, "plugins"))) !== container || + (await fsPromises.realpath(path.join(owner, "plugins"))) !== container + ) + throw new Error("Managed plugin owner changed during policy read"); + return { + registryPath, + imports: + selection === null + ? null + : Object.fromEntries( + [...selection.byName] + .sort(([a], [b]) => a.localeCompare(b)) + .map(([name, components]) => [ + name, + components === undefined ? null : [...new Set(components.mcpServers)].sort(), + ]) + ), + }; + } catch { + // Failure denies managed provenance only; unmanaged plugins do not consult this policy. + return { registryPath: registryFile, imports: null }; + } +} diff --git a/src/node/services/agentSkills/builtInSkillContent.generated.ts b/src/node/services/agentSkills/builtInSkillContent.generated.ts index ba18e328df..863706b06a 100644 --- a/src/node/services/agentSkills/builtInSkillContent.generated.ts +++ b/src/node/services/agentSkills/builtInSkillContent.generated.ts @@ -2013,7 +2013,9 @@ export const BUILTIN_SKILL_FILES: Record> = { "", "Global plugins can be installed from git via **Settings → Plugins** (paste a git URL or `owner/repo[@ref]`). The preview lists the full package before anything is written. Select which skills and MCP servers to import; both groups start with all current items selected and support **Select all** and **Clear**. You can install with neither group selected. Agents, workflows, slash commands, and hooks are unaffected by this choice; review their disclosures too. Imported MCP servers remain disabled until you enable them per workspace.", "", - "Installed rows show imported-of-available counts. For a managed, present plugin, choose **Add components** (or **Add Plugin Components…** in the command palette) to import more from its locked installed version without fetching the remote. Previously imported components are checked and read-only. Select additions and confirm, or cancel without changing anything. If the installed version changes during review, the inventory refreshes and you must select again. Removing imports is not supported by this flow.", + "Installed rows show imported-of-available counts. For a managed, present plugin, choose **Manage components** (or **Manage Plugin Components…** in the command palette) to change which skills and MCP servers are imported from its locked installed version without fetching the remote. Deselect imported components, select new ones, or use **Select all** and **Clear** for a whole group. Review the add/remove counts and choose **Save changes**, or **Cancel** to discard your draft. Saving an empty selection keeps the plugin installed with no skills or MCP servers imported; uninstall is a separate action. If the installed files or saved selection change during review, the inventory refreshes and you must review and save again.", + "", + "Removing imports does not delete source files, plugin data, or workspace MCP settings (including enablement and tool allowlists). New MCP imports remain disabled until enabled per workspace; re-adding a server honors its saved workspace enablement. Agents, hooks, workflows, and slash commands are not changed by this selection. Legacy installs continue to import all components until you save a different selection. Once a selection is explicit, updates preserve it and do not automatically import newly added components.", "", "Updates preserve explicit selections: newly available skills and MCP servers stay unimported until you add them. Updates still require consent for full-package capability changes, including unimported components. Older managed installs without a saved selection continue importing everything, including new components on update. Unmanaged and project-local plugins are unchanged.", "", diff --git a/src/node/services/di/layers/core.ts b/src/node/services/di/layers/core.ts index 6abe54f1de..561e6b2a7d 100644 --- a/src/node/services/di/layers/core.ts +++ b/src/node/services/di/layers/core.ts @@ -1,3 +1,7 @@ +import { + readPluginMcpPolicy, + PLUGIN_REGISTRY_FILE_NAME, +} from "@/node/services/agentPlugins/registry"; import { readPersistedExperimentEnabled } from "@/node/services/experimentsService"; import { ClaudeDesignService } from "@/node/services/claudeDesignService"; import * as os from "os"; @@ -6,7 +10,11 @@ import { Context, Effect, Layer } from "effect"; import { EXPERIMENT_IDS } from "@/common/constants/experiments"; import { secretsToRecord } from "@/common/types/secrets"; import { isMultiProject } from "@/common/utils/multiProject"; -import { STAGING_DIR_NAME, readMutationEpochToken } from "@/node/services/agentPlugins/journals"; +import { + acquirePluginMutationLock, + STAGING_DIR_NAME, + readMutationEpochToken, +} from "@/node/services/agentPlugins/journals"; import { PLUGIN_SERVER_KEY_PREFIX, createAgentPluginsMcpProvider, @@ -373,6 +381,10 @@ export const MCPServerManagerLive = Layer.effect( telemetryService: opts.telemetryService, pluginInvalidation: { keyPrefix: PLUGIN_SERVER_KEY_PREFIX, + readComponentPolicy: () => + readPluginMcpPolicy(path.join(mcpConfig.rootDir, PLUGIN_REGISTRY_FILE_NAME)), + tryAcquireComponentPolicyLock: (options) => + acquirePluginMutationLock(mcpConfig.rootDir, { timeoutMs: 0, ...options }), readToken: () => readMutationEpochToken(path.join(mcpConfig.rootDir, STAGING_DIR_NAME)), // Bounded/cancellable like the send path's own read: a distrusted or // cold serve re-reads through here, and an unreachable SSH/Docker diff --git a/src/node/services/mcpServerManager.test.ts b/src/node/services/mcpServerManager.test.ts index 22f1ed042e..491cba6da0 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -9,7 +9,13 @@ import { MCP_PROMPT_MAX_TEXT_BYTES, MCP_PROMPT_TRUNCATION_MARKER, } from "@/common/constants/toolLimits"; -import { MUTATION_EPOCH_UNREADABLE_TOKEN } from "@/node/services/agentPlugins/journals"; +import { + acquirePluginMutationLock, + MUTATION_EPOCH_UNREADABLE_TOKEN, +} from "@/node/services/agentPlugins/journals"; +import { readPluginMcpPolicy } from "./agentPlugins/registry"; +import { createTestPluginInstallEntry } from "./agentPlugins/testFixtures"; +import type { MCPServerInfo } from "@/common/types/mcp"; import * as mcpSdk from "@/node/services/mcpClient"; import { MCPServerManager, @@ -19,11 +25,14 @@ import { runMCPToolWithDeadline, wrapMCPTools, type MCPWorkspaceRequestOptions, + type MCPServerManagerOptions, } from "./mcpServerManager"; import { MCPConfigService } from "./mcpConfigService"; import { Config } from "@/node/config"; +import { WorkspaceMcpOverridesService } from "./workspaceMcpOverridesService"; import type { TelemetryService } from "./telemetryService"; import type { Runtime } from "@/node/runtime/Runtime"; +import * as runtimeFactory from "@/node/runtime/runtimeFactory"; import { DevcontainerRuntime } from "@/node/runtime/DevcontainerRuntime"; import { RemoteRuntime } from "@/node/runtime/RemoteRuntime"; import { DisposableTempDir } from "@/node/services/tempDir"; @@ -103,7 +112,7 @@ function testInstance( autoFallbackUsed: false, tools: options.tools ?? {}, prompts: options.prompts ?? [], - getPrompt: options.getPrompt ?? mock(() => Promise.resolve({ messages: [] })), + getPrompt: options.getPrompt ?? mock(() => Promise.resolve({ messages: [], context: {} })), ...(options.refreshTools !== undefined ? { refreshTools: options.refreshTools } : {}), // Prompt fixtures need a refresher because production stores catalogs // only through refreshInstancePrompts. @@ -231,6 +240,1221 @@ describe("MCPServerManager", () => { } }); + async function componentFixture(home: string) { + await fs.mkdir(path.join(home, "plugins"), { recursive: true }); + const registryPath = path.join(home, "plugins.json"); + const write = async (names: string[]) => { + const release = await acquirePluginMutationLock(home, { timeoutMs: 5000 }); + try { + await fs.writeFile( + `${registryPath}.tmp`, + JSON.stringify({ + plugins: [createTestPluginInstallEntry("demo", { skills: [], mcpServers: names })], + }) + ); + await fs.rename(`${registryPath}.tmp`, registryPath); + } finally { + await release(); + } + }; + await write(["remove", "keep"]); + const configs: Record = { ordinary: stdioConfig("ordinary") }; + for (const name of ["remove", "keep", "added"]) { + configs[`plugin:instance:${name}`] = { + ...stdioConfig(name), + env: { PLUGIN_DATA: path.join(home, "data", name) }, + plugin: { + pluginName: "demo", + serverName: name, + sourceScope: "global", + sourceLocation: "plugins/demo", + componentPolicy: { registryPath, name: "demo" }, + }, + }; + } + configService.listServers = mock(() => Promise.resolve({ ...configs })); + const read = mock(() => readPluginMcpPolicy(registryPath)); + const makeManager = () => { + const invalidation: NonNullable = { + keyPrefix: "plugin:", + readToken: () => Promise.resolve(undefined), + readComponentPolicy: read, + tryAcquireComponentPolicyLock: (options) => + acquirePluginMutationLock(home, { timeoutMs: 0, ...options }), + }; + const instance = new MCPServerManager(configService as unknown as MCPConfigService, { + pluginInvalidation: invalidation, + }); + const internals = instance as unknown as MCPServerManagerTestAccess; + const started: Array> = []; + internals.startSingleServer = mock((name: unknown) => { + const client = testInstance(String(name), { + tools: { echo: testTool() }, + prompts: [{ name: "review" }], + getPrompt: mock(() => + Promise.resolve({ + messages: [{ role: "user", content: { type: "text", text: "review" } }], + }) + ), + }); + started.push(client); + return Promise.resolve(client); + }); + return { instance, internals, started, invalidation }; + }; + manager.dispose(); + const local = makeManager(); + manager = local.instance; + access = local.internals; + return { ...local, registryPath, write, configs, read, makeManager }; + } + + test.each([false, true])( + "component removal preserves sibling identity (leased: %s)", + async (leased) => { + using tmp = new DisposableTempDir("mcp-components"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("components"); + const before = await manager.getToolsForWorkspace(request); + const removed = f.started.find((i) => i.name.endsWith(":remove"))!; + const retained = f.started.filter((i) => i !== removed); + if (leased) manager.acquireLease(request.workspaceId); + await f.write(["keep"]); + await manager.reconcilePluginComponents(); + const after = await manager.getToolsForWorkspace(request); + expect(Object.values(after.toolServerNames).sort()).toEqual([ + "ordinary", + "plugin:instance:keep", + ]); + expect(after.stats.enabledServerCount).toBe(2); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + timedOutServerNames: string[]; + enabledServerNames: Set; + }; + for (const client of retained) { + expect(entry.instances.get(client.name)).toBe(client); + expect(client.close).not.toHaveBeenCalled(); + } + expect(entry.timedOutServerNames).not.toContain(removed.name); + expect(entry.enabledServerNames.has(removed.name)).toBe(false); + const toolName = Object.keys(before.toolServerNames).find( + (key) => before.toolServerNames[key] === removed.name + )!; + expect( + before.tools[toolName].execute!({}, { toolCallId: "held", messages: [], context: {} }) + ).rejects.toThrow(/disabled|unavailable/); + expect(manager.getPrompt(request.workspaceId, removed.name, "review", {})).rejects.toThrow(); + expect(removed.tools.echo.execute).not.toHaveBeenCalled(); + if (leased) { + expect(removed.close).not.toHaveBeenCalled(); + manager.releaseLease(request.workspaceId); + await manager.reconcilePluginComponents(); + } + expect(removed.close).toHaveBeenCalledTimes(1); + expect(f.started).toHaveLength(3); + } + ); + + test.each([false, true])( + "component cleanup failures remain retryable without blocking retained clients (readd: %s)", + async (readd) => { + using tmp = new DisposableTempDir("mcp-component-cleanup-retry"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("cleanup-retry"); + const served = await manager.getToolsForWorkspace(request); + const removed = f.started.find((instance) => instance.name.endsWith(":remove"))!; + const retained = f.started.filter((instance) => instance !== removed); + let failClose = true; + const close = spyOn(removed as { close: () => Promise }, "close").mockImplementation( + () => + failClose ? Promise.reject(new Error("removed client close failed")) : Promise.resolve() + ); + try { + await f.write(["keep"]); + const error: unknown = await manager + .reconcilePluginComponents() + .catch((error: unknown) => error); + expect(error).toBeInstanceOf(Error); + expect(String(error)).toContain("removed client close failed"); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + retiredPluginInstances?: Set; + }; + expect(entry.retiredPluginInstances?.has(removed)).toBe(true); + const after = await manager.getToolsForWorkspace(request); + expect(Object.values(after.toolServerNames).sort()).toEqual([ + "ordinary", + "plugin:instance:keep", + ]); + expect(close.mock.calls.length).toBeGreaterThan(1); + const toolName = Object.keys(served.toolServerNames).find( + (key) => served.toolServerNames[key] === removed.name + )!; + const heldError: unknown = await Promise.resolve( + served.tools[toolName].execute!({}, { toolCallId: "removed", messages: [], context: {} }) + ).catch((error: unknown) => error); + expect(heldError).toBeInstanceOf(Error); + expect(removed.tools.echo.execute).not.toHaveBeenCalled(); + if (readd) { + await f.write(["keep", "remove"]); + const readded = await manager.getToolsForWorkspace(request); + expect(Object.values(readded.toolServerNames)).toContain(removed.name); + expect(entry.instances.get(removed.name)).not.toBe(removed); + } + failClose = false; + const attempts = close.mock.calls.length; + await manager.getToolsForWorkspace(request); + expect(close).toHaveBeenCalledTimes(attempts + 1); + expect(entry.retiredPluginInstances?.size ?? 0).toBe(0); + await manager.reconcilePluginComponents(); + expect(close).toHaveBeenCalledTimes(attempts + 1); + for (const instance of retained) { + expect(entry.instances.get(instance.name)).toBe(instance); + expect(instance.close).not.toHaveBeenCalled(); + } + } finally { + close.mockRestore(); + } + } + ); + + test.each([false, true])( + "prefix stops include retired leased clients without reviving removals (readd: %s)", + async (readd) => { + using tmp = new DisposableTempDir("mcp-retired-prefix"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("retired-prefix"); + const first = await manager.getToolsForWorkspace(request); + const removed = f.started.find((instance) => instance.name.endsWith(":remove"))!; + const retained = f.started.filter((instance) => instance !== removed); + manager.acquireLease(request.workspaceId); + try { + await f.write(["keep"]); + await manager.getToolsForWorkspace(request); + if (readd) { + await f.write(["keep", "remove"]); + await manager.getToolsForWorkspace(request); + } + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + retiredPluginInstances?: Set; + timedOutServerNames: string[]; + }; + expect(entry.retiredPluginInstances?.has(removed)).toBe(true); + await manager.stopServersWithKeyPrefix(removed.name); + expect(removed.close).toHaveBeenCalledTimes(1); + expect(entry.retiredPluginInstances?.has(removed) ?? false).toBe(false); + expect(entry.timedOutServerNames.includes(removed.name)).toBe(readd); + for (const instance of retained) { + expect(entry.instances.get(instance.name)).toBe(instance); + expect(instance.close).not.toHaveBeenCalled(); + } + if (!readd) { + const toolName = Object.keys(first.toolServerNames).find( + (key) => first.toolServerNames[key] === removed.name + )!; + const error: unknown = await Promise.resolve( + first.tools[toolName].execute!({}, { toolCallId: "stopped", messages: [], context: {} }) + ).catch((error: unknown) => error); + expect(error).toBeInstanceOf(Error); + expect(removed.tools.echo.execute).not.toHaveBeenCalled(); + } + } finally { + manager.releaseLease(request.workspaceId); + await manager.reconcilePluginComponents(); + } + expect(removed.close).toHaveBeenCalledTimes(1); + } + ); + + test("idle cleanup retries retired-only failures without another MCP request", async () => { + using tmp = new DisposableTempDir("mcp-retired-idle"); + const f = await componentFixture(tmp.path); + delete f.configs.ordinary; + const request = workspaceRequest("retired-idle"); + await manager.getToolsForWorkspace(request); + const removed = f.started.find((instance) => instance.name.endsWith(":remove"))!; + let fail = true; + const close = spyOn(removed as { close: () => Promise }, "close").mockImplementation( + () => (fail ? Promise.reject(new Error("close failed")) : Promise.resolve()) + ); + const sweep = spyOn( + manager as unknown as { retireCrossProcessPluginInstances: () => Promise }, + "retireCrossProcessPluginInstances" + ); + try { + await f.write([]); + await manager.reconcilePluginComponents().catch(() => undefined); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + retiredPluginInstances?: Set; + lastActivity: number; + }; + expect(entry.instances.size).toBe(0); + entry.lastActivity = Date.now() - 11 * 60_000; + for (const shouldFail of [true, false]) { + fail = shouldFail; + sweep.mockClear(); + const attempts = close.mock.calls.length; + access.cleanupIdleServers(); + expect(sweep).toHaveBeenCalledTimes(1); + await sweep.mock.results[0].value; + expect(close).toHaveBeenCalledTimes(attempts + 1); + expect(entry.retiredPluginInstances?.has(removed) ?? false).toBe(shouldFail); + if (shouldFail) expect(access.workspaceServers.get(request.workspaceId)).toBe(entry); + } + } finally { + close.mockRestore(); + sweep.mockRestore(); + } + }); + + test.each(["stdio", "http", "sse", "auto"] as const)( + "named Test connection rechecks current policy before %s launch", + async (transport) => { + using tmp = new DisposableTempDir("mcp-named-test-policy"); + const f = await componentFixture(tmp.path); + const key = "plugin:instance:remove"; + const plugin = f.configs[key].plugin; + f.configs[key] = + transport === "stdio" + ? { + ...stdioConfig("removed"), + plugin, + env: { PLUGIN_DATA: path.join(tmp.path, "data") }, + } + : { transport, url: "https://mcp.example.test/removed", disabled: false, plugin }; + configService.listServers.mockImplementation(async () => { + const snapshot = { ...f.configs }; + await f.write(["keep"]); + return snapshot; + }); + const exec = mock(() => Promise.reject(new Error("launch reached"))); + const runtime = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + exec, + } as unknown as Runtime); + const client = spyOn(mcpSdk, "createMCPClient").mockImplementation(() => + Promise.reject(new Error("connection reached")) + ); + try { + const named = await manager.test({ projectPath: tmp.path, name: key }); + expect(named.success).toBe(false); + if (named.success) throw new Error("Expected revoked named test to fail"); + expect(named.error).toMatch(/disabled|unavailable/); + expect(exec).not.toHaveBeenCalled(); + expect(client).not.toHaveBeenCalled(); + f.read.mockClear(); + // Explicit user drafts are not managed descriptors, even when their name matches. + await manager.test({ + projectPath: tmp.path, + name: key, + ...(transport === "stdio" + ? { command: "draft" } + : { transport, url: "https://mcp.example.test/draft" }), + }); + expect(transport === "stdio" ? exec : client).toHaveBeenCalledTimes(1); + expect(f.read).not.toHaveBeenCalled(); + exec.mockClear(); + client.mockClear(); + delete f.configs[key].plugin!.componentPolicy; + await manager.test({ projectPath: tmp.path, name: key }); + expect(transport === "stdio" ? exec : client).toHaveBeenCalledTimes(1); + expect(f.read).not.toHaveBeenCalled(); + } finally { + runtime.mockRestore(); + client.mockRestore(); + } + } + ); + + test.each(["stdio", "http", "sse", "auto"] as const)( + "normal %s startup rechecks components after waiting for the override fence", + async (transport) => { + using tmp = new DisposableTempDir("mcp-start-component-fence"); + const f = await componentFixture(tmp.path); + f.invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + let removeAtFence = false; + let overrideHeld = false; + f.invalidation.acquireOverridesLock = async () => { + if (removeAtFence) await f.write(["keep"]); + overrideHeld = true; + return () => { + overrideHeld = false; + return Promise.resolve(); + }; + }; + await manager.getToolsForWorkspace(workspaceRequest("startup-baseline")); + removeAtFence = true; + const key = "plugin:instance:remove"; + const info: MCPServerInfo = + transport === "stdio" + ? f.configs[key] + : { + transport, + url: "https://mcp.example.test", + disabled: false, + plugin: f.configs[key].plugin, + }; + const exec = mock(() => Promise.reject(new Error("spawn reached"))); + const client = spyOn(mcpSdk, "createMCPClient").mockImplementation(() => + Promise.reject(new Error("connection reached")) + ); + try { + const error: unknown = await access + .startSingleServerImpl( + key, + info, + { exec } as unknown as Runtime, + PROJECT_PATH, + WORKSPACE_PATH, + undefined, + () => undefined, + new AbortController().signal + ) + .catch((error: unknown) => error); + expect(exec).not.toHaveBeenCalled(); + expect(client).not.toHaveBeenCalled(); + expect(String(error)).toMatch(/disabled|unavailable/); + expect(overrideHeld).toBe(false); + } finally { + client.mockRestore(); + } + } + ); + + test.each([false, true])( + "normal startup fails closed on component writer contention (overrides tracked: %s)", + async (trackOverrides) => { + using tmp = new DisposableTempDir("mcp-start-component-contention"); + const f = await componentFixture(tmp.path); + let overrideHeld = false; + if (trackOverrides) { + f.invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + f.invalidation.acquireOverridesLock = () => { + overrideHeld = true; + return Promise.resolve(() => { + overrideHeld = false; + return Promise.resolve(); + }); + }; + } + await manager.getToolsForWorkspace(workspaceRequest("contention-baseline")); + const exec = mock(() => Promise.reject(new Error("spawn reached"))); + const start = () => + access + .startSingleServerImpl( + "plugin:instance:remove", + f.configs["plugin:instance:remove"], + { exec } as unknown as Runtime, + PROJECT_PATH, + WORKSPACE_PATH, + undefined, + () => undefined, + new AbortController().signal + ) + .catch((error: unknown) => error); + const release = await acquirePluginMutationLock(tmp.path, { timeoutMs: 0 }); + try { + expect(String(await start())).toContain("unavailable"); + expect(exec).not.toHaveBeenCalled(); + // Uninstall can now prune overrides without waiting on this startup. + expect(overrideHeld).toBe(false); + } finally { + await release(); + } + await f.write(["keep"]); + expect(String(await start())).toContain("disabled"); + expect(exec).not.toHaveBeenCalled(); + } + ); + + test("normal auto startup rechecks components before its SSE fallback", async () => { + using tmp = new DisposableTempDir("mcp-fallback-component-fence"); + const f = await componentFixture(tmp.path); + f.invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + const client = spyOn(mcpSdk, "createMCPClient").mockImplementation(() => + Promise.reject(Object.assign(new Error("HTTP not supported"), { status: 404 })) + ); + f.invalidation.acquireOverridesLock = async () => { + if (client.mock.calls.length > 0) await f.write(["keep"]); + return () => Promise.resolve(); + }; + try { + await manager.getToolsForWorkspace(workspaceRequest("fallback-baseline")); + const key = "plugin:instance:remove"; + const error: unknown = await access + .startSingleServerImpl( + key, + { + transport: "auto", + url: "https://mcp.example.test", + disabled: false, + plugin: f.configs[key].plugin, + }, + TEST_RUNTIME, + PROJECT_PATH, + WORKSPACE_PATH, + undefined, + () => undefined, + new AbortController().signal + ) + .catch((error: unknown) => error); + expect(client).toHaveBeenCalledTimes(1); + expect(String(error)).toMatch(/disabled|unavailable/); + } finally { + client.mockRestore(); + } + }); + + test("component policy rejects a held tool in a second manager without local notification", async () => { + using tmp = new DisposableTempDir("mcp-components-sibling"); + const f = await componentFixture(tmp.path); + const sibling = f.makeManager(); + try { + const request = workspaceRequest("sibling"); + const served = await sibling.instance.getToolsForWorkspace(request); + const removed = sibling.started.find((i) => i.name.endsWith(":remove"))!; + const toolName = Object.keys(served.toolServerNames).find( + (key) => served.toolServerNames[key] === removed.name + )!; + await f.write(["keep"]); + await manager.reconcilePluginComponents(); + expect(removed.close).not.toHaveBeenCalled(); + expect( + served.tools[toolName].execute!({}, { toolCallId: "held", messages: [], context: {} }) + ).rejects.toThrow(/disabled|unavailable/); + expect(removed.tools.echo.execute).not.toHaveBeenCalled(); + expect(removed.close).toHaveBeenCalledTimes(1); + for (const client of sibling.started.filter((i) => i !== removed)) + expect(client.close).not.toHaveBeenCalled(); + } finally { + sibling.instance.dispose(); + } + }); + + test("component mixed equal-count selection and rapid readd do not restart retained clients", async () => { + using tmp = new DisposableTempDir("mcp-components-mixed"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("mixed"); + await manager.getToolsForWorkspace(request); + const retained = f.started.filter((i) => !i.name.endsWith(":remove")); + await f.write(["keep", "added"]); + const after = await manager.getToolsForWorkspace(request); + expect(Object.values(after.toolServerNames).sort()).toEqual([ + "ordinary", + "plugin:instance:added", + "plugin:instance:keep", + ]); + expect(f.started).toHaveLength(4); + await f.write([]); + await f.write(["keep", "added"]); + await manager.reconcilePluginComponents(); + await manager.getToolsForWorkspace(request); + expect(f.started).toHaveLength(4); + for (const client of retained) expect(client.close).not.toHaveBeenCalled(); + }); + + test("component readd under an active lease closes only the retired client on release", async () => { + using tmp = new DisposableTempDir("mcp-components-leased-readd"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("leased-readd"); + await manager.getToolsForWorkspace(request); + manager.acquireLease(request.workspaceId); + const old = f.started.find((i) => i.name.endsWith(":remove"))!; + await f.write(["keep"]); + await manager.getToolsForWorkspace(request); + await f.write(["keep", "remove"]); + const result = await manager.getToolsForWorkspace(request); + expect(Object.values(result.toolServerNames)).toContain(old.name); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + }; + const replacement = entry.instances.get(old.name); + expect(replacement).not.toBe(old); + expect(old.close).not.toHaveBeenCalled(); + manager.releaseLease(request.workspaceId); + await manager.reconcilePluginComponents(); + expect(old.close).toHaveBeenCalledTimes(1); + for (const client of f.started.filter((i) => i !== old)) + expect(client.close).not.toHaveBeenCalled(); + expect(entry.instances.get(old.name)).toBe(replacement); + }); + + test("component cleanup permits an already admitted leased invocation to finish", async () => { + using tmp = new DisposableTempDir("mcp-components-admitted"); + const f = await componentFixture(tmp.path); + const entered = Promise.withResolvers(); + const finish = Promise.withResolvers(); + const original = access.startSingleServer; + access.startSingleServer = async (...args) => { + const client = (await original(...args)) as ReturnType; + if (args[0] === "plugin:instance:remove") + client.tools.echo = { + ...testTool(), + execute: mock(() => { + entered.resolve(); + return finish.promise; + }), + }; + return client; + }; + const request = workspaceRequest("admitted"); + const first = await manager.getToolsForWorkspace(request); + const toolName = Object.keys(first.toolServerNames).find((key) => + first.toolServerNames[key].endsWith(":remove") + )!; + manager.acquireLease(request.workspaceId); + const pending: unknown = first.tools[toolName].execute!( + {}, + { toolCallId: "admitted", messages: [], context: {} } + ); + await entered.promise; + await f.write(["keep"]); + await manager.reconcilePluginComponents(); + finish.resolve("finished"); + expect(await pending).toBe("finished"); + manager.releaseLease(request.workspaceId); + await manager.reconcilePluginComponents(); + expect(f.started.find((i) => i.name.endsWith(":remove"))!.close).toHaveBeenCalledTimes(1); + }); + + test("component authorization reads current policy after a slow override fence", async () => { + using tmp = new DisposableTempDir("mcp-components-final-gate"); + const f = await componentFixture(tmp.path); + let removeAtFence = false; + const invalidation = ( + manager as unknown as { + pluginInvalidation: { + readOverridesEpoch: () => Promise; + readWorkspaceOverrides: () => Promise>; + acquireOverridesLock: () => Promise<() => Promise>; + }; + } + ).pluginInvalidation; + invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + invalidation.acquireOverridesLock = async () => { + if (removeAtFence) await f.write(["keep"]); + return () => Promise.resolve(); + }; + const request = workspaceRequest("final-gate"); + const first = await manager.getToolsForWorkspace(request); + removeAtFence = true; + const toolName = Object.keys(first.toolServerNames).find((key) => + first.toolServerNames[key].endsWith(":remove") + )!; + expect( + first.tools[toolName].execute!({}, { toolCallId: "held", messages: [], context: {} }) + ).rejects.toThrow(/disabled|unavailable/); + expect( + f.started.find((i) => i.name.endsWith(":remove"))!.tools.echo.execute + ).not.toHaveBeenCalled(); + }); + + test.each(["tool", "prompt", "test"] as const)( + "managed %s admission fails closed without its writer fence", + async (operation) => { + using tmp = new DisposableTempDir("mcp-components-missing-fence"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("missing-fence"); + const served = await manager.getToolsForWorkspace(request); + delete f.invalidation.tryAcquireComponentPolicyLock; + const key = "plugin:instance:remove"; + const toolName = Object.keys(served.toolServerNames).find( + (name) => served.toolServerNames[name] === key + )!; + const exec = mock(() => Promise.reject(new Error("launch reached"))); + const runtime = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + exec, + } as unknown as Runtime); + try { + if (operation === "test") { + const result = await manager.test({ projectPath: tmp.path, name: key }); + expect(result.success).toBe(false); + if (result.success) throw new Error("Expected missing fence to deny the test"); + expect(result.error).toMatch(/unavailable/); + expect(exec).not.toHaveBeenCalled(); + } else { + const pending: unknown = + operation === "tool" + ? served.tools[toolName].execute!( + {}, + { toolCallId: "missing-fence", messages: [], context: {} } + ) + : manager.getPrompt(request.workspaceId, key, "review", {}); + expect(Promise.resolve(pending)).rejects.toThrow(/unavailable/); + const client = f.started.find((instance) => instance.name === key)!; + expect(client.tools.echo.execute).not.toHaveBeenCalled(); + expect(client.getPrompt).not.toHaveBeenCalled(); + } + const ordinary = Object.keys(served.toolServerNames).find( + (name) => served.toolServerNames[name] === "ordinary" + )!; + await served.tools[ordinary].execute!( + {}, + { toolCallId: "ordinary", messages: [], context: {} } + ); + expect( + f.started.find((instance) => instance.name === "ordinary")!.tools.echo.execute + ).toHaveBeenCalledTimes(1); + } finally { + runtime.mockRestore(); + } + } + ); + + test.each(["tool", "prompt"] as const)( + "%s admission holds the writer lock before opening the final policy inode", + async (operation) => { + using tmp = new DisposableTempDir("mcp-components-inode"); + const f = await componentFixture(tmp.path); + const overrides = new WorkspaceMcpOverridesService(new Config(tmp.path)); + let overrideHeld = false; + let pluginHeld = false; + f.invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + f.invalidation.acquireOverridesLock = async (options) => { + const release = await overrides.acquireExclusiveLock(options); + overrideHeld = true; + return async () => { + await release(); + overrideHeld = false; + }; + }; + f.invalidation.tryAcquireComponentPolicyLock = async (options) => { + const release = await acquirePluginMutationLock(tmp.path, { timeoutMs: 0, ...options }); + pluginHeld = true; + return async () => { + await release(); + pluginHeld = false; + }; + }; + const dispatched = Promise.withResolvers(); + const finish = Promise.withResolvers(); + const original = access.startSingleServer; + access.startSingleServer = async (...args) => { + const instance = (await original(...args)) as ReturnType; + if (args[0] === "plugin:instance:remove") { + const dispatch = () => { + expect(pluginHeld).toBe(true); + expect(overrideHeld).toBe(true); + dispatched.resolve(); + return finish.promise; + }; + instance.tools.echo = { ...testTool(), execute: () => dispatch().then(() => "ok") }; + instance.getPrompt = mock(() => + dispatch().then(() => ({ + messages: [{ role: "user", content: { type: "text", text: "ok" } }], + })) + ); + } + return instance; + }; + const request = workspaceRequest("inode"); + const served = await manager.getToolsForWorkspace(request); + const toolName = Object.keys(served.toolServerNames).find( + (name) => served.toolServerNames[name] === "plugin:instance:remove" + )!; + const opened = Promise.withResolvers(); + const resume = Promise.withResolvers(); + const readFile = fs.readFile; + let intercept = true; + const readSpy = spyOn(fs, "readFile").mockImplementation((async ( + ...args: Parameters + ) => { + if (intercept && overrideHeld && args[0] === f.registryPath) { + intercept = false; + const handle = await fs.open(f.registryPath, "r"); + try { + opened.resolve(); + await resume.promise; + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } + } + return readFile(...args); + }) as typeof fs.readFile); + const pending = Promise.resolve( + operation === "tool" + ? served.tools[toolName].execute!({}, { toolCallId: "inode", messages: [], context: {} }) + : manager.getPrompt(request.workspaceId, "plugin:instance:remove", "review", {}) + ); + const settled = pending.catch((error: unknown) => error); + let writer: Promise | undefined; + try { + await opened.promise; + expect(pluginHeld).toBe(true); + expect(acquirePluginMutationLock(tmp.path, { timeoutMs: 0 })).rejects.toThrow(); + writer = f.write(["keep"]); + resume.resolve(); + await dispatched.promise; + await writer; + const releaseOverride = await overrides.acquireExclusiveLock({ timeoutMs: 1000 }); + await releaseOverride(); + expect(pluginHeld).toBe(false); + expect(overrideHeld).toBe(false); + finish.resolve(); + if (operation === "tool") expect(await settled).toBe("ok"); + else expect(await settled).toBeInstanceOf(Error); + } finally { + resume.resolve(); + finish.resolve(); + await settled; + await writer; + readSpy.mockRestore(); + } + } + ); + + test.each(["tool", "prompt"] as const)( + "%s contention releases overrides so a plugin writer can prune", + async (operation) => { + using tmp = new DisposableTempDir("mcp-components-writer-wins"); + const f = await componentFixture(tmp.path); + const key = "plugin:0123456789abcdef:remove"; + f.configs[key] = f.configs["plugin:instance:remove"]; + delete f.configs["plugin:instance:remove"]; + const config = new Config(tmp.path); + const workspacePath = path.join(tmp.path, "checkout"); + const workspaceId = "writer-wins"; + await fs.mkdir(workspacePath); + await config.editConfig((current) => { + current.projects.set(workspacePath, { + workspaces: [ + { + path: workspacePath, + id: workspaceId, + name: workspaceId, + runtimeConfig: { type: "local" }, + }, + ], + }); + return current; + }); + const overrides = new WorkspaceMcpOverridesService(config); + await overrides.setOverridesForWorkspace(workspaceId, { + enabledServers: [key, "ordinary"], + }); + let releaseWriter: (() => Promise) | undefined; + let prune: ReturnType | undefined; + let overrideReleases = 0; + f.invalidation.readOverridesEpoch = () => Promise.resolve("stable"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + f.invalidation.acquireOverridesLock = async (options) => { + const release = await overrides.acquireExclusiveLock(options); + // The installer wins the plugin lock after preflight; pruning then waits + // for the invocation's override fence, the former O -> P -> O cycle. + releaseWriter = await acquirePluginMutationLock(tmp.path, { timeoutMs: 0 }); + prune = overrides.prunePluginOverrideKeysForWorkspaces( + [workspaceId], + "plugin:0123456789abcdef:" + ); + return async () => { + overrideReleases++; + await release(); + }; + }; + const request = workspaceRequest(workspaceId); + const served = await manager.getToolsForWorkspace(request); + const toolName = Object.keys(served.toolServerNames).find( + (name) => served.toolServerNames[name] === key + )!; + try { + const pending: unknown = + operation === "tool" + ? served.tools[toolName].execute!( + {}, + { toolCallId: "writer", messages: [], context: {} } + ) + : manager.getPrompt(workspaceId, key, "review", {}); + expect(Promise.resolve(pending)).rejects.toThrow(/unavailable.*retry/); + expect(overrideReleases).toBe(1); + expect(await prune).toEqual([]); + expect( + (await overrides.getOverridesForWorkspace(workspaceId)).overrides.enabledServers + ).toEqual(["ordinary"]); + const client = f.started.find((instance) => instance.name === key)!; + expect(client.tools.echo.execute).not.toHaveBeenCalled(); + expect(client.getPrompt).not.toHaveBeenCalled(); + } finally { + await releaseWriter?.(); + await prune; + } + } + ); + + test.each( + (["tool", "prompt"] as const).flatMap((operation) => + (["read-error", "read-abort", "read-timeout", "late-acquisition"] as const).map( + (failure) => ({ operation, failure }) + ) + ) + )( + "$operation component fence releases exactly once on $failure without late dispatch", + async ({ operation, failure }) => { + using tmp = new DisposableTempDir("mcp-components-release"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("release"); + const served = await manager.getToolsForWorkspace(request); + const key = "plugin:instance:remove"; + const toolName = Object.keys(served.toolServerNames).find( + (name) => served.toolServerNames[name] === key + )!; + const controller = new AbortController(); + const entered = Promise.withResolvers(); + const resumeAcquisition = Promise.withResolvers(); + const resumeRead = Promise.withResolvers(); + const released = Promise.withResolvers(); + let inFence = false; + let releaseCount = 0; + const now = spyOn(Date, "now"); + f.invalidation.tryAcquireComponentPolicyLock = async () => { + const release = await acquirePluginMutationLock(tmp.path, { timeoutMs: 0 }); + inFence = true; + if (failure === "late-acquisition") { + entered.resolve(); + await resumeAcquisition.promise; + } + return async () => { + releaseCount++; + await release(); + released.resolve(); + }; + }; + f.read.mockImplementation(async () => { + if (inFence) { + entered.resolve(); + if (failure === "read-error") throw new Error("final policy read failed"); + if (failure === "read-timeout") now.mockReturnValue(Date.now() + 60_000); + if (failure === "read-abort" || failure === "read-timeout") await resumeRead.promise; + } + return readPluginMcpPolicy(f.registryPath); + }); + const pending = Promise.resolve( + operation === "tool" + ? served.tools[toolName].execute!( + {}, + { toolCallId: "release", messages: [], context: {}, abortSignal: controller.signal } + ) + : manager.getPrompt(request.workspaceId, key, "review", {}, { signal: controller.signal }) + ); + const rejected = pending.catch((error: unknown) => error); + try { + await entered.promise; + if (failure === "read-abort" || failure === "late-acquisition") controller.abort(); + expect(await rejected).toBeInstanceOf(Error); + resumeAcquisition.resolve(); + await released.promise; + resumeRead.resolve(); + expect(releaseCount).toBe(1); + const release = await acquirePluginMutationLock(tmp.path, { timeoutMs: 0 }); + await release(); + const client = f.started.find((instance) => instance.name === key)!; + expect(client.tools.echo.execute).not.toHaveBeenCalled(); + expect(client.getPrompt).not.toHaveBeenCalled(); + } finally { + now.mockRestore(); + resumeAcquisition.resolve(); + resumeRead.resolve(); + await rejected; + } + } + ); + + test("unavailable component reader fails closed only for managed clients", async () => { + using tmp = new DisposableTempDir("mcp-components-reader"); + const f = await componentFixture(tmp.path); + const request = workspaceRequest("reader"); + await manager.getToolsForWorkspace(request); + f.read.mockImplementation(() => Promise.reject(new Error("home unavailable"))); + const result = await manager.getToolsForWorkspace(request); + expect(Object.values(result.toolServerNames)).toEqual(["ordinary"]); + expect(f.started.find((i) => i.name === "ordinary")!.close).not.toHaveBeenCalled(); + }); + + test("component removal drops failed startup retry candidates", async () => { + using tmp = new DisposableTempDir("mcp-components-retries"); + const f = await componentFixture(tmp.path); + const removed = "plugin:instance:remove"; + const startup = mock(async (servers: unknown) => ({ + instances: new Map( + await Promise.all( + Object.keys(servers as Record) + .filter((name) => name !== removed) + .map(async (name) => [name, await access.startSingleServer(name)] as const) + ) + ), + failedServerNames: Object.hasOwn(servers as object, removed) ? [removed] : [], + timedOutServerNames: Object.hasOwn(servers as object, removed) ? [removed] : [], + })); + access.startServers = startup; + const request = workspaceRequest("retries"); + await manager.getToolsForWorkspace(request); + await f.write(["keep"]); + await manager.reconcilePluginComponents(); + const entry = access.workspaceServers.get(request.workspaceId) as { + timedOutServerNames: string[]; + retryingTimedOutServerNames: Set; + }; + expect(entry.timedOutServerNames).toEqual([]); + expect(entry.retryingTimedOutServerNames.has(removed)).toBe(false); + const result = await manager.getToolsForWorkspace(request); + expect(result.stats.failedServerNames).not.toContain(removed); + for (const [servers] of startup.mock.calls.slice(1)) + expect(servers).not.toHaveProperty(removed); + }); + + test("component owner retarget denies old tools without affecting unrelated clients", async () => { + using tmp = new DisposableTempDir("mcp-components-owner"); + const f = await componentFixture(path.join(tmp.path, "old")); + const next = path.join(tmp.path, "new"); + await fs.mkdir(path.join(next, "plugins"), { recursive: true }); + await fs.writeFile(path.join(next, "plugins.json"), await fs.readFile(f.registryPath)); + const request = workspaceRequest("owner"); + const first = await manager.getToolsForWorkspace(request); + f.read.mockImplementation(() => readPluginMcpPolicy(path.join(next, "plugins.json"))); + const toolName = Object.keys(first.toolServerNames).find((key) => + first.toolServerNames[key].endsWith(":remove") + )!; + expect( + first.tools[toolName].execute!({}, { toolCallId: "owner", messages: [], context: {} }) + ).rejects.toThrow(/disabled|unavailable/); + expect(f.started.find((i) => i.name === "ordinary")!.close).not.toHaveBeenCalled(); + }); + + test("component policy read count is bounded independently of server count", async () => { + using tmp = new DisposableTempDir("mcp-components-read-budget"); + const f = await componentFixture(tmp.path); + const names = Array.from({ length: 32 }, (_, index) => `server${index}`); + for (const name of names) + f.configs[`plugin:many:${name}`] = { + ...stdioConfig(name), + plugin: { ...f.configs["plugin:instance:keep"].plugin!, serverName: name }, + }; + await f.write(["keep", ...names]); + const request = workspaceRequest("read-budget"); + await manager.getToolsForWorkspace(request); + f.read.mockClear(); + const served = await manager.getToolsForWorkspace(request); + expect(Object.keys(served.tools)).toHaveLength(34); + expect(f.read.mock.calls.length).toBeLessThanOrEqual(4); + f.read.mockClear(); + await served.tools.ordinary_echo.execute!( + {}, + { toolCallId: "budget", messages: [], context: {} } + ); + expect(f.read.mock.calls.length).toBeLessThanOrEqual(4); + }); + + test("component policy churn exhausts the bounded stable scan", async () => { + using tmp = new DisposableTempDir("mcp-components-churn"); + const f = await componentFixture(tmp.path); + let reads = 0; + f.read.mockImplementation(() => + Promise.resolve({ + registryPath: f.registryPath, + imports: { demo: [String(reads++)] }, + }) + ); + expect(access.runWithStablePluginEpoch(() => Promise.resolve(undefined))).rejects.toThrow( + /kept racing/ + ); + expect(reads).toBeLessThanOrEqual(18); + }); + + test.each(["missing", "unreadable"])( + "%s component policy never downgrades managed servers", + async (failure) => { + using tmp = new DisposableTempDir("mcp-components-policy"); + const f = await componentFixture(tmp.path); + f.configs.unmanaged = { + ...stdioConfig("unmanaged"), + plugin: { + pluginName: "demo", + serverName: "remove", + sourceScope: "global", + sourceLocation: ".agents/plugins/demo", + }, + }; + const request = workspaceRequest("policy"); + await manager.getToolsForWorkspace(request); + if (failure === "missing") await fs.unlink(f.registryPath); + else { + await fs.unlink(f.registryPath); + await fs.mkdir(f.registryPath); + } + // Discovery can now report the orphan as unmanaged; remembered provenance must win. + delete f.configs["plugin:instance:remove"].plugin!.componentPolicy; + const after = await manager.getToolsForWorkspace(request); + expect(Object.values(after.toolServerNames).sort()).toEqual(["ordinary", "unmanaged"]); + for (const client of f.started.filter((i) => !i.name.startsWith("plugin:"))) + expect(client.close).not.toHaveBeenCalled(); + } + ); + + test.each(["initial", "additional", "retry", "restart", "retired-only", "readd"] as const)( + "failed component startup retirement stays owned after %s publication", + async (mode) => { + using tmp = new DisposableTempDir("mcp-startup-retirement"); + const f = await componentFixture(tmp.path); + const key = "plugin:instance:remove"; + const request = workspaceRequest("startup-retirement"); + const startServers = access.startServers; + if (mode === "retired-only") { + delete f.configs.ordinary; + await f.write(["remove"]); + } else if (mode === "additional") { + await f.write(["keep"]); + await manager.getToolsForWorkspace(request); + await f.write(["keep", "remove"]); + } else if (mode === "retry") { + access.startServers = async (servers, ...args) => { + const remaining = { ...(servers as Record) }; + delete remaining[key]; + const result = await startServers.call(manager, remaining, ...args); + return { ...result, failedServerNames: [key], timedOutServerNames: [key] }; + }; + await manager.getToolsForWorkspace(request); + access.startServers = startServers; + } else if (mode === "restart") { + await manager.getToolsForWorkspace(request); + f.started.find((client) => client.name === key)!.isClosed = true; + manager.acquireLease(request.workspaceId); + } + const entered = Promise.withResolvers(); + const resume = Promise.withResolvers(); + const original = access.startSingleServer; + let failClose = true; + let failedClient: ReturnType | undefined; + access.startSingleServer = async (...args) => { + const client = (await original(...args)) as ReturnType; + if (args[0] === key && failedClient === undefined) { + failedClient = client; + client.close = mock(() => + failClose ? Promise.reject(new Error("startup close failed")) : Promise.resolve() + ); + entered.resolve(); + await resume.promise; + } + return client; + }; + const pending = manager.getToolsForWorkspace(request); + pending.catch(() => undefined); + try { + await entered.promise; + await f.write(mode === "retired-only" ? [] : ["keep"]); + resume.resolve(); + const served = await pending; + const retained = f.started.filter((client) => client.name !== key); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + retiredPluginInstances?: Set; + enabledServerNames: Set; + timedOutServerNames: string[]; + lastActivity: number; + }; + expect(failedClient).toBeDefined(); + expect(failedClient!.close.mock.calls.length).toBeGreaterThan(0); + expect(entry.retiredPluginInstances?.has(failedClient)).toBe(true); + expect(entry.instances.has(key)).toBe(false); + expect(entry.enabledServerNames.has(key)).toBe(false); + expect(entry.timedOutServerNames).not.toContain(key); + expect(Object.values(served.toolServerNames)).not.toContain(key); + expect(served.stats.startedServerCount).toBe(mode === "retired-only" ? 0 : 2); + expect(served.stats.enabledServerCount).toBe(mode === "retired-only" ? 0 : 2); + expect(served.stats.failedServerNames).not.toContain(key); + for (const client of retained) { + expect(entry.instances.get(client.name)).toBe(client); + expect(client.close).not.toHaveBeenCalled(); + } + if (mode === "readd") { + await f.write(["keep", "remove"]); + const readded = await manager.getToolsForWorkspace(request); + expect(Object.values(readded.toolServerNames)).toContain(key); + expect(entry.instances.get(key)).not.toBe(failedClient); + expect(entry.retiredPluginInstances?.has(failedClient)).toBe(true); + } + failClose = false; + const attempts = failedClient!.close.mock.calls.length; + if (mode === "retired-only") { + const sweep = spyOn( + manager as unknown as { retireCrossProcessPluginInstances: () => Promise }, + "retireCrossProcessPluginInstances" + ); + try { + entry.lastActivity = Date.now() - 11 * 60_000; + access.cleanupIdleServers(); + expect(sweep).toHaveBeenCalledTimes(1); + await sweep.mock.results[0].value; + } finally { + sweep.mockRestore(); + } + } else if (mode === "additional" || mode === "restart") { + await manager.stopServersWithKeyPrefix(key); + } else { + await manager.reconcilePluginComponents(); + } + expect(failedClient!.close).toHaveBeenCalledTimes(attempts + 1); + expect(entry.retiredPluginInstances?.has(failedClient) ?? false).toBe(false); + for (const client of retained) { + expect(entry.instances.get(client.name)).toBe(client); + expect(client.close).not.toHaveBeenCalled(); + } + } finally { + failClose = false; + resume.resolve(); + await pending.catch(() => undefined); + if (mode === "restart") manager.releaseLease(request.workspaceId); + await manager.reconcilePluginComponents(); + } + } + ); + + test.each([false, true])( + "component removal fences pending startup (readd during cleanup: %s)", + async (readd) => { + using tmp = new DisposableTempDir("mcp-components-start"); + const f = await componentFixture(tmp.path); + const entered = Promise.withResolvers(); + const finish = Promise.withResolvers(); + const original = access.startSingleServer; + access.startSingleServer = async (...args) => { + const client = await original(...args); + if (args[0] === "plugin:instance:remove") { + entered.resolve(); + await finish.promise; + if (readd) + (client as ReturnType).close = mock(async () => { + await f.write(["keep", "remove"]); + }); + } + return client; + }; + const request = workspaceRequest("startup"); + const pending = manager.getToolsForWorkspace(request); + await entered.promise; + await f.write(["keep"]); + finish.resolve(); + const after = await pending; + expect(f.started[1].close).toHaveBeenCalledTimes(1); + expect(Object.values(after.toolServerNames).includes("plugin:instance:remove")).toBe(readd); + const entry = access.workspaceServers.get(request.workspaceId) as { + instances: Map; + timedOutServerNames: string[]; + }; + expect(entry.timedOutServerNames).not.toContain("plugin:instance:remove"); + if (readd) expect(entry.instances.get("plugin:instance:remove")).not.toBe(f.started[1]); + const starts = f.started.length; + await manager.getToolsForWorkspace(request); + expect(f.started).toHaveLength(starts); + } + ); + test("cross-process plugin mutation token retires cached plugin instances before serving", async () => { // A sibling process's update/uninstall recycles only its OWN manager; // this manager must notice the bumped on-disk mutation token and retire @@ -1437,28 +2661,33 @@ describe("MCPServerManager", () => { test("a remote launch fence releases the writer's lock at its initiation deadline while the handshake is pending", async () => { // Every settings save and prune would otherwise queue behind an // endpoint-controlled handshake for the whole startup deadline. - manager.dispose(); + using tmp = new DisposableTempDir("mcp-component-launch-lifetime"); + const f = await componentFixture(tmp.path); let lockHeld = false; - manager = new MCPServerManager(configService as unknown as MCPConfigService, { - pluginInvalidation: { - keyPrefix: "plugin:", - readToken: () => Promise.resolve("plugins-1"), - readOverridesEpoch: () => Promise.resolve("epoch-1"), - readWorkspaceOverrides: () => Promise.resolve({}), - acquireOverridesLock: () => { - lockHeld = true; - return Promise.resolve(() => { - lockHeld = false; - return Promise.resolve(); - }); - }, - }, - }); - access = manager as unknown as MCPServerManagerTestAccess; - configService.listServers = mock(() => Promise.resolve({})); + let componentLockHeld = false; + const acquireComponentLock = f.invalidation.tryAcquireComponentPolicyLock!; + f.invalidation.tryAcquireComponentPolicyLock = async (options) => { + const release = await acquireComponentLock(options); + componentLockHeld = true; + return async () => { + await release(); + componentLockHeld = false; + }; + }; + f.invalidation.readOverridesEpoch = () => Promise.resolve("epoch-1"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + f.invalidation.acquireOverridesLock = () => { + lockHeld = true; + return Promise.resolve(() => { + lockHeld = false; + return Promise.resolve(); + }); + }; await manager.getToolsForWorkspace(workspaceRequest("ws-remote-fence-baseline")); const fence = access as unknown as { launchUnderOverrideFence: ( + name: string, + info: MCPServerInfo, launch: () => Promise, signal: AbortSignal, options?: { releaseAfterMs?: number } @@ -1467,8 +2696,10 @@ describe("MCPServerManager", () => { const handshake = Promise.withResolvers(); let heldAtLaunch: boolean | undefined; const launched = fence.launchUnderOverrideFence( + "plugin:instance:remove", + f.configs["plugin:instance:remove"], () => { - heldAtLaunch = lockHeld; + heldAtLaunch = lockHeld && componentLockHeld; return handshake.promise; }, new AbortController().signal, @@ -1476,7 +2707,9 @@ describe("MCPServerManager", () => { ); expect(heldAtLaunch).toBeUndefined(); await waitFor(() => !lockHeld && heldAtLaunch === true); - // The handshake is still pending; the lock is already released. + // A real component writer can commit while the admitted handshake is pending. + await f.write(["keep"]); + expect(componentLockHeld).toBe(false); handshake.resolve("connected"); expect(await launched).toBe("connected"); }); @@ -1484,28 +2717,33 @@ describe("MCPServerManager", () => { test("a stdio launch still awaiting its exec at the fence deadline is aborted, not released", async () => { // Releasing would let an SSH exec still acquiring its connection send the // repository-configured command after a sibling's revocation committed. - manager.dispose(); + using tmp = new DisposableTempDir("mcp-component-launch-lifetime"); + const f = await componentFixture(tmp.path); let lockHeld = false; - manager = new MCPServerManager(configService as unknown as MCPConfigService, { - pluginInvalidation: { - keyPrefix: "plugin:", - readToken: () => Promise.resolve("plugins-1"), - readOverridesEpoch: () => Promise.resolve("epoch-1"), - readWorkspaceOverrides: () => Promise.resolve({}), - acquireOverridesLock: () => { - lockHeld = true; - return Promise.resolve(() => { - lockHeld = false; - return Promise.resolve(); - }); - }, - }, - }); - access = manager as unknown as MCPServerManagerTestAccess; - configService.listServers = mock(() => Promise.resolve({})); + let componentLockHeld = false; + const acquireComponentLock = f.invalidation.tryAcquireComponentPolicyLock!; + f.invalidation.tryAcquireComponentPolicyLock = async (options) => { + const release = await acquireComponentLock(options); + componentLockHeld = true; + return async () => { + await release(); + componentLockHeld = false; + }; + }; + f.invalidation.readOverridesEpoch = () => Promise.resolve("epoch-1"); + f.invalidation.readWorkspaceOverrides = () => Promise.resolve({}); + f.invalidation.acquireOverridesLock = () => { + lockHeld = true; + return Promise.resolve(() => { + lockHeld = false; + return Promise.resolve(); + }); + }; await manager.getToolsForWorkspace(workspaceRequest("ws-stdio-fence-baseline")); const fence = access as unknown as { launchUnderOverrideFence: ( + name: string, + info: MCPServerInfo, launch: (launchSignal: AbortSignal) => Promise, signal: AbortSignal, options?: { abortAfterMs?: { ms: number; serverName: string } } @@ -1514,9 +2752,11 @@ describe("MCPServerManager", () => { let launchSignal: AbortSignal | undefined; let heldWhilePending: boolean | undefined; const launched = fence.launchUnderOverrideFence( + "plugin:instance:remove", + f.configs["plugin:instance:remove"], (signal) => { launchSignal = signal; - heldWhilePending = lockHeld; + heldWhilePending = lockHeld && componentLockHeld; // Mirrors RemoteRuntime.exec: settles only through the abort. return new Promise((_resolve, reject) => { signal.addEventListener("abort", () => reject(new Error("Operation aborted")), { @@ -1532,15 +2772,19 @@ describe("MCPServerManager", () => { expect(heldWhilePending).toBe(true); expect(launchSignal?.aborted).toBe(true); expect(lockHeld).toBe(false); + expect(componentLockHeld).toBe(false); // A launch that hands back its stream in time is unaffected and released. const quick = await fence.launchUnderOverrideFence( + "plugin:instance:remove", + f.configs["plugin:instance:remove"], (signal) => Promise.resolve(signal.aborted ? "aborted" : "spawned"), new AbortController().signal, { abortAfterMs: { ms: 1_000, serverName: "quick" } } ); expect(quick).toBe("spawned"); expect(lockHeld).toBe(false); + expect(componentLockHeld).toBe(false); }); test("startSingleServerImpl cleans up client that resolves after abort", async () => { diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index 34cd7cb0c4..53ddc2a749 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -1,3 +1,4 @@ +import { isPluginMcpServerAllowed, type PluginMcpPolicy } from "./agentPlugins/registry"; import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; import type { OAuthClientProvider, PriorDiscovery } from "@modelcontextprotocol/client"; @@ -1138,6 +1139,8 @@ interface MCPToolsForWorkspaceResult { enablementDerivedFrom?: MCPWorkspaceRequestOptions; } interface WorkspaceServers { + /** Removed selections are detached immediately, but leased calls keep their client alive. */ + retiredPluginInstances?: Set; configSignature: string; instances: Map; /** Filters prompts while leased restarts can leave disabled clients cached. */ @@ -1193,6 +1196,13 @@ export interface MCPServerManagerOptions { pluginInvalidation?: { keyPrefix: string; readToken: () => Promise; + /** Atomic plugins.json content, independent of tree replacement and override epochs. */ + readComponentPolicy?: () => Promise; + /** Same writer lock as plugins.json mutations. Must try once, never queue/wait. */ + tryAcquireComponentPolicyLock?: (options: { + signal?: AbortSignal; + }) => Promise<() => Promise>; + /** * Disk-authoritative workspace override read. A sibling's uninstall also * pruned plugin keys from workspace override FILES; the sweep uses this @@ -1311,6 +1321,13 @@ export class MCPServerManager { private readonly prefixInvalidations = new Map(); /** See MCPServerManagerOptions.pluginInvalidation. */ private readonly pluginInvalidation?: MCPServerManagerOptions["pluginInvalidation"]; + private componentPolicy: PluginMcpPolicy | undefined; + private componentPolicyRevision = 0; + private readonly managedPluginServers = new Map>(); + private readonly managedPluginInstances = new Map< + string, + NonNullable["componentPolicy"]> + >(); private pluginInvalidationTokenSeen = false; private lastPluginInvalidationToken: string | undefined; private lastOverridesEpochToken: string | undefined; @@ -1351,6 +1368,167 @@ export class MCPServerManager { this.pluginInvalidation = options?.pluginInvalidation; } + /** Call after the atomic selection write and after releasing the install lock. */ + async reconcilePluginComponents(): Promise { + await this.retireCrossProcessPluginInstances(true); + } + + private componentAllowed( + name: string, + info?: MCPServerInfo, + policy = this.componentPolicy + ): boolean { + if (this.pluginInvalidation?.readComponentPolicy === undefined) return true; + return isPluginMcpServerAllowed(this.managedPluginServers.get(name) ?? info?.plugin, policy); + } + + private async readComponentPolicy(): Promise { + const read = this.pluginInvalidation?.readComponentPolicy; + if (read === undefined) return undefined; + try { + const result = await raceWithAbortAndTimeout(read(), { timeoutMs: CALL_GATE_TIMEOUT_MS }); + if (result.kind === "ok") return result.value; + } catch (error) { + log.debug("MCP component policy unavailable", { error }); + } + return { registryPath: this.componentPolicy?.registryPath ?? "", imports: null }; + } + + private async withComponentPolicyFence( + name: string, + info: MCPServerInfo | undefined, + dispatch: () => T, + options: { signal?: AbortSignal; timeoutMs?: number } = {} + ): Promise<{ pending: T }> { + const release = await this.acquireComponentPolicyFence(name, info, options); + try { + const pending = dispatch(); + // Observe early rejection while admission locks are being released. + Promise.resolve(pending).catch(() => undefined); + return { pending }; + } finally { + await release(); + } + } + + private async acquireComponentPolicyFence( + name: string, + info: MCPServerInfo | undefined, + options: { signal?: AbortSignal; timeoutMs?: number } = {} + ): Promise<() => Promise> { + // Ownership comes from the served provenance, never an unfenced policy read: + // a missing registry row must not turn a managed client into a legacy one. + const plugin = this.managedPluginServers.get(name) ?? info?.plugin; + if (plugin?.componentPolicy === undefined) return () => Promise.resolve(); + const read = this.pluginInvalidation?.readComponentPolicy; + const acquire = this.pluginInvalidation?.tryAcquireComponentPolicyLock; + const unavailable = () => + new Error( + `MCP server '${name}' is unavailable while plugin components are being updated; retry` + ); + if (read === undefined || acquire === undefined) throw unavailable(); + const deadlineAt = Date.now() + (options.timeoutMs ?? CALL_GATE_TIMEOUT_MS); + const checkActive = () => { + if (options.signal?.aborted) throw new Error(`MCP request for '${name}' was aborted`); + if (Date.now() >= deadlineAt) throw unavailable(); + }; + const bounded = async (work: Promise): Promise => { + // A pre-aborted race does not subscribe to work; still observe late rejection. + work.catch(() => undefined); + const result = await raceWithAbortAndTimeout(work, { + timeoutMs: Math.max(0, deadlineAt - Date.now()), + signal: options.signal, + }); + checkActive(); + if (result.kind !== "ok") throw unavailable(); + return result.value; + }; + checkActive(); + // Overrides are locked first. Uninstall takes the plugin lock and then prunes + // overrides, so waiting here would deadlock. A contended try-lock fails closed + // and lets the outer finally release overrides; no admission retry loop. + const acquisition = acquire({ signal: options.signal }); + let release: () => Promise; + try { + release = await bounded(acquisition); + } catch { + acquisition.then((lateRelease) => lateRelease()).catch(() => undefined); + checkActive(); + throw unavailable(); + } + try { + // Atomic rename alone is insufficient: readFile may still own the old inode. + // Acquire BEFORE opening policy; the caller releases after admission. + checkActive(); + const policy = await bounded(read()); + checkActive(); + if (!this.componentAllowed(name, info, policy)) + throw new Error(`MCP server '${name}' is disabled by component policy`); + return release; + } catch (error) { + await release(); + throw error; + } + } + + private async refreshComponentPolicy(): Promise { + let cleanupError: Error | undefined; + const previous = this.componentPolicy; + const policy = await this.readComponentPolicy(); + if (JSON.stringify(policy) !== JSON.stringify(this.componentPolicy)) { + this.componentPolicy = policy; + this.componentPolicyRevision++; + } + for (const [workspaceId, entry] of this.workspaceServers) { + const readded = [...this.managedPluginServers.keys()].filter( + (name) => + !this.componentAllowed(name, undefined, previous) && + this.componentAllowed(name) && + !entry.instances.has(name) && + Object.hasOwn(JSON.parse(entry.configSignature) as object, name) + ); + this.markServersForRetry(entry, readded); + const denied = new Set( + [...entry.enabledServerNames, ...entry.instances.keys()].filter( + (name) => !this.componentAllowed(name, entry.enabledServers[name]) + ) + ); + for (const name of denied) { + entry.enabledServerNames.delete(name); + delete entry.enabledServers[name]; + entry.retryingTimedOutServerNames?.delete(name); + } + entry.timedOutServerNames = + entry.timedOutServerNames?.filter((name) => this.componentAllowed(name)) ?? []; + entry.stats = this.createWorkspaceStats( + entry.enabledServerNames.size, + new Map([...entry.instances].filter(([name]) => entry.enabledServerNames.has(name))), + entry.stats.failedServerNames.filter((name) => this.componentAllowed(name)) + ); + for (const name of denied) { + const instance = entry.instances.get(name); + if (!instance) continue; + entry.instances.delete(name); + (entry.retiredPluginInstances ??= new Set()).add(instance); + } + // Keep admitted leased calls alive without letting a readd overwrite + // their retired client. Only active instances participate in retention. + if (this.getLeaseCount(workspaceId) > 0) continue; + const retired = entry.retiredPluginInstances; + for (const instance of retired ?? []) { + try { + await instance.close(); + retired?.delete(instance); + } catch (error) { + cleanupError ??= error instanceof Error ? error : new Error(getErrorMessage(error)); + log.warn("Failed to close removed plugin component", { name: instance.name, error }); + } + } + if (retired?.size === 0) delete entry.retiredPluginInstances; + } + return cleanupError; + } + /** * Retire cached plugin instances when a SIBLING process mutated a plugin * (see MCPServerManagerOptions.pluginInvalidation). Runs before every @@ -1359,7 +1537,7 @@ export class MCPServerManager { * The first read only records the token: no plugin instance can predate it * because this method guards every serve path. */ - private async retireCrossProcessPluginInstances(): Promise { + private async retireCrossProcessPluginInstances(reportCleanupErrors = false): Promise { const invalidation = this.pluginInvalidation; if (invalidation === undefined) { return; @@ -1371,7 +1549,10 @@ export class MCPServerManager { // replaced tree. Queued serves wait for the in-flight sweep, then see the // published token and proceed; a failed sweep leaves the token // unpublished so the next serve retries it. + let cleanupError: Error | undefined; const run = async (): Promise => { + if (invalidation.readComponentPolicy !== undefined) + cleanupError = await this.refreshComponentPolicy(); const [token, overridesEpoch] = await Promise.all([ invalidation.readToken(), invalidation.readOverridesEpoch?.(), @@ -1427,6 +1608,12 @@ export class MCPServerManager { }; const next = this.pluginInvalidationQueue.then(run, run); this.pluginInvalidationQueue = next.catch(() => undefined); + // Explicit saves report cleanup failures only after publishing policy/epoch work. + // Automatic MCP boundaries keep retained clients usable and retry retired clients later. + if (reportCleanupErrors) { + await next; + if (cleanupError !== undefined) throw cleanupError; + } return next; } @@ -1858,6 +2045,14 @@ export class MCPServerManager { if (current === 1) { this.workspaceLeases.delete(workspaceId); + if (this.pluginInvalidation?.readComponentPolicy !== undefined) { + this.retireCrossProcessPluginInstances().catch((error: unknown) => { + log.warn("Failed to reconcile plugin components after lease release", { + workspaceId, + error, + }); + }); + } return; } @@ -1874,8 +2069,9 @@ export class MCPServerManager { private cleanupIdleServers(): void { const now = Date.now(); + let retryRetiredComponents = false; for (const [workspaceId, entry] of this.workspaceServers) { - if (entry.instances.size === 0) continue; + if (entry.instances.size === 0 && !entry.retiredPluginInstances?.size) continue; // Never tear down a workspace's MCP servers while a stream is running. if (this.getLeaseCount(workspaceId) > 0) { @@ -1884,6 +2080,12 @@ export class MCPServerManager { const idleMs = now - entry.lastActivity; if (idleMs >= IDLE_TIMEOUT_MS) { + // Do not evict retry ownership while retired clients still fail to close. + // Once they close, a later idle sweep resumes normal workspace eviction. + if (entry.retiredPluginInstances?.size) { + retryRetiredComponents = true; + continue; + } log.info("[MCP] Stopping idle servers", { workspaceId, idleMinutes: Math.round(idleMs / 60_000), @@ -1891,6 +2093,11 @@ export class MCPServerManager { void this.stopServers(workspaceId, { retainRestartOptions: true }); } } + if (retryRetiredComponents) { + this.retireCrossProcessPluginInstances().catch((error: unknown) => { + log.warn("Failed to retry idle plugin component cleanup", { error }); + }); + } } private createWorkspaceStats( @@ -1961,7 +2168,21 @@ export class MCPServerManager { for (const [name, command] of Object.entries(this.inlineServers)) { inlineAsInfo[name] = { transport: "stdio", command, disabled: false }; } - return { ...configServers, ...inlineAsInfo }; + const servers = { ...configServers, ...inlineAsInfo }; + for (const [name, info] of Object.entries(servers)) { + const plugin = info.plugin; + if (plugin === undefined) continue; + // The logical instance prefix includes scope/alias identity. Remember + // the installation, so a vanished row cannot expose previously hidden siblings. + const instanceKey = name.slice(0, -plugin.serverName.length); + const owner = plugin.componentPolicy ?? this.managedPluginInstances.get(instanceKey); + if (owner === undefined) continue; + this.managedPluginInstances.set(instanceKey, { ...owner }); + this.managedPluginServers.set(name, { ...plugin, componentPolicy: { ...owner } }); + } + return Object.fromEntries( + Object.entries(servers).filter(([name, info]) => this.componentAllowed(name, info)) + ); } /** @@ -1984,6 +2205,8 @@ export class MCPServerManager { trusted = false, agentPlugins?: AgentPluginsMcpContext | null ): Promise { + if (this.pluginInvalidation?.readComponentPolicy !== undefined) + await this.retireCrossProcessPluginInstances(); const allServers = await this.getAllServers(projectPath, trusted, agentPlugins); const enabled = this.applyServerOverrides(allServers, overrides); return this.filterServersByPolicy(enabled); @@ -2127,6 +2350,8 @@ export class MCPServerManager { // advance the live baseline while this operation is still in flight — // the live field would then match the fresh read and accept a result // derived from pre-eviction state. + const componentPolicyUsed = JSON.stringify(this.componentPolicy); + const componentRevisionUsed = this.componentPolicyRevision; const pluginTokenUsed = this.lastPluginInvalidationToken; const overridesEpochUsed = this.lastOverridesEpochToken; const result = await operation(); @@ -2143,9 +2368,15 @@ export class MCPServerManager { // accepted — a parallel read could capture the old epoch while the // slower token read settles, accepting a pair a sibling revocation // completed in between. + const componentPolicy = + this.pluginInvalidation.readComponentPolicy !== undefined + ? await this.readComponentPolicy() + : undefined; const token = await this.pluginInvalidation.readToken(); const overridesEpoch = await this.pluginInvalidation.readOverridesEpoch?.(); if ( + componentPolicyUsed === JSON.stringify(componentPolicy) && + componentRevisionUsed === this.componentPolicyRevision && token === pluginTokenUsed && overridesEpoch === overridesEpochUsed && !isWorkspaceOverridesEpochUnreadable(overridesEpoch) @@ -2557,7 +2788,7 @@ export class MCPServerManager { retriedInstances, startupEpoch, workspaceId, - (invalidatedRetryKeys) => { + (invalidatedRetryKeys, failedRetirements) => { // Recheck ownership INSIDE the synchronous callback: a // removal-style stopServers (or config-change replacement) // landing while the awaited invalidation scan yielded has @@ -2568,6 +2799,8 @@ export class MCPServerManager { retryOwnershipLost = true; return; } + for (const instance of failedRetirements) + (existing.retiredPluginInstances ??= new Set()).add(instance); for (const [serverName, instance] of retriedInstances) { existing.instances.set(serverName, instance); } @@ -2668,7 +2901,7 @@ export class MCPServerManager { } const additiveServerNames = existing - ? this.getAdditiveServerNames(existing, signatureEntries) + ? this.getRetainableServerAdditions(existing, signatureEntries) : undefined; // If a stream is actively running, avoid closing MCP clients out from under it. @@ -2742,7 +2975,7 @@ export class MCPServerManager { restartedInstances, startupEpoch, workspaceId, - (invalidatedRestartKeys) => { + (invalidatedRestartKeys, failedRetirements) => { // Same ownership recheck as the timed-out retry path: a removal // or replacement landing during the awaited scan must not let // this merge revive clients on a detached entry. @@ -2751,6 +2984,8 @@ export class MCPServerManager { return; } + for (const instance of failedRetirements) + (existing.retiredPluginInstances ??= new Set()).add(instance); for (const [serverName, instance] of restartedInstances) { existing.instances.set(serverName, instance); } @@ -2922,7 +3157,7 @@ export class MCPServerManager { } const addedServerNames = current - ? this.getAdditiveServerNames(current, signatureEntries) + ? this.getRetainableServerAdditions(current, signatureEntries) : undefined; if (additiveServerNames !== undefined && addedServerNames === undefined) { // A newer additive request may have won the lock. Never roll it back @@ -2999,7 +3234,7 @@ export class MCPServerManager { instances, startupEpoch, workspaceId, - (invalidatedKeys) => { + (invalidatedKeys, failedRetirements) => { // Recheck the removal-stop epoch INSIDE the synchronous publication // callback: a stopServers(workspaceId) landing while the awaited // invalidation scan yielded found no cache entry to close, so @@ -3010,6 +3245,8 @@ export class MCPServerManager { } if (retained) { if (this.workspaceServers.get(workspaceId) !== retained) return; + for (const instance of failedRetirements) + (retained.retiredPluginInstances ??= new Set()).add(instance); for (const [name, instance] of instances) retained.instances.set(name, instance); retained.configSignature = signature; retained.enabledServerNames = enabledServerNames; @@ -3028,6 +3265,7 @@ export class MCPServerManager { entry = { configSignature: signature, instances, + ...(failedRetirements.size > 0 ? { retiredPluginInstances: failedRetirements } : {}), enabledServerNames, enabledServers, enabledServersGeneration: configGenerationUsed, @@ -3304,21 +3542,27 @@ export class MCPServerManager { return descriptors; } - private getAdditiveServerNames( + private getRetainableServerAdditions( entry: WorkspaceServers, next: Record ): string[] | undefined { if ([...entry.instances.values()].some((instance) => instance.isClosed)) return undefined; // Signatures are in-process JSON of launch settings, including resolved secrets. const previous = JSON.parse(entry.configSignature) as Record; - if (Object.keys(next).length <= Object.keys(previous).length) return undefined; + const added = Object.keys(next).filter((name) => !Object.hasOwn(previous, name)); + const removed = Object.keys(previous).filter((name) => !Object.hasOwn(next, name)); + if (added.length === 0 && removed.length === 0) return undefined; + // Only selection removals are non-disruptive. Unrelated configuration + // changes retain the existing full-restart/deferred-restart behavior. + if (removed.some((name) => this.componentAllowed(name))) return undefined; if ( Object.keys(previous).some( - (name) => JSON.stringify(previous[name]) !== JSON.stringify(next[name]) + (name) => + Object.hasOwn(next, name) && JSON.stringify(previous[name]) !== JSON.stringify(next[name]) ) ) return undefined; - return Object.keys(next).filter((name) => !Object.hasOwn(previous, name)); + return added; } private async computeSignatureEntries( @@ -3799,7 +4043,12 @@ export class MCPServerManager { const readOverridesEpoch = this.pluginInvalidation?.readOverridesEpoch; let pending: ReturnType | "retry"; if (acquireOverridesLock === undefined || readOverridesEpoch === undefined) { - pending = dispatch(); + ({ pending } = await this.withComponentPolicyFence( + serverName, + undefined, + dispatch, + options + )); } else { // ONE deadline for acquisition and the fenced epoch read: the outer // abort race cannot stop this callback, so a stalled home filesystem @@ -3829,7 +4078,10 @@ export class MCPServerManager { ) { return { epochMoved: true } as const; } - pending = dispatch(); + ({ pending } = await this.withComponentPolicyFence(serverName, undefined, dispatch, { + signal: options?.signal, + timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), + })); } finally { await release(); } @@ -3919,6 +4171,18 @@ export class MCPServerManager { // on an unrelated healthy client, so tearing down the whole workspace // set here would close it underneath them. for (const [workspaceId, entry] of this.workspaceServers) { + // Tree replacement also stops deselected clients held by active leases, + // but those clients must never become restart candidates. + for (const instance of entry.retiredPluginInstances ?? []) { + if (!instance.name.startsWith(prefix)) continue; + try { + await instance.close(); + entry.retiredPluginInstances?.delete(instance); + } catch (error) { + log.warn("Failed to stop retired MCP server", { error, name: instance.name }); + } + } + if (entry.retiredPluginInstances?.size === 0) delete entry.retiredPluginInstances; const removedKeys: string[] = []; for (const [serverKey, instance] of [...entry.instances]) { if (!serverKey.startsWith(prefix)) { @@ -4019,16 +4283,37 @@ export class MCPServerManager { * after publication, so it sees the published entry and closes matches. * * `publish` MUST NOT await; it receives every key closed across all scans - * and must queue them for retry (see closeInvalidatedInstances docs). + * and must queue them for retry (see closeInvalidatedInstances docs). Failed + * component closes transfer to the published entry's retired-client set so + * they remain retryable without exposing their tools or blocking a readd. */ private async closeInvalidatedInstancesThenPublish( instances: Map, startedAtEpoch: number, workspaceId: string, - publish: (invalidatedKeys: string[]) => void + publish: (invalidatedKeys: string[], failedRetirements: Set) => void ): Promise { const invalidatedKeys: string[] = []; + const removedComponents: string[] = []; + const failedRetirements = new Set(); for (;;) { + if (this.pluginInvalidation?.readComponentPolicy !== undefined) + await this.retireCrossProcessPluginInstances(); + for (const [name, instance] of instances) { + if (this.componentAllowed(name)) continue; + instances.delete(name); + removedComponents.push(name); + // Even remove->readd during awaited cleanup requires a fresh startup; + // equality of the final content snapshot alone cannot detect that ABA. + this.componentPolicyRevision++; + try { + await instance.close(); + } catch (error) { + failedRetirements.add(instance); + log.warn("Failed to close removed plugin startup", { name, error }); + } + } + if (removedComponents.length > 0) await this.retireCrossProcessPluginInstances(); const clockBeforeScan = this.prefixInvalidationClock; invalidatedKeys.push( ...(await this.closeInvalidatedInstances(instances, startedAtEpoch, workspaceId)) @@ -4036,7 +4321,10 @@ export class MCPServerManager { // Terminates: the clock only advances on stopServersWithKeyPrefix // calls, which are finite user-driven plugin update/uninstall events. if (this.prefixInvalidationClock === clockBeforeScan) { - publish(invalidatedKeys); + publish( + [...invalidatedKeys, ...removedComponents].filter((name) => this.componentAllowed(name)), + failedRetirements + ); return; } } @@ -4062,7 +4350,7 @@ export class MCPServerManager { // client that is in the middle of closing. this.workspaceServers.delete(workspaceId); - for (const instance of entry.instances.values()) { + for (const instance of [...entry.instances.values(), ...(entry.retiredPluginInstances ?? [])]) { try { await instance.close(); } catch (error) { @@ -4198,13 +4486,23 @@ export class MCPServerManager { if (server.transport !== "stdio" && server.managed === "claude-design") { return this.configService.claudeDesign.test(); } + const testNamedServer = async ( + launch: Parameters[0] + ): Promise => { + // Admit the named test after disk/OAuth preparation, before its connection + // deadline starts. Ad-hoc drafts never carry managed plugin provenance. + try { + const { pending } = await this.withComponentPolicyFence(trimmedName, server, () => + runServerTest(launch, projectPath, `server "${trimmedName}"`) + ); + return await pending; + } catch (error) { + return { success: false, error: getErrorMessage(error) }; + } + }; if (server.transport === "stdio") { const launch = await prepareStdioLaunch(server); - return runServerTest( - { transport: "stdio", ...launch }, - projectPath, - `server "${trimmedName}"` - ); + return testNamedServer({ transport: "stdio", ...launch }); } try { @@ -4215,16 +4513,12 @@ export class MCPServerManager { serverUrl: server.url, }); - return runServerTest( - { - transport: server.transport, - url: server.url, - headers: resolved.headers, - ...(authProvider ? { authProvider } : {}), - }, - projectPath, - `server "${trimmedName}"` - ); + return testNamedServer({ + transport: server.transport, + url: server.url, + headers: resolved.headers, + ...(authProvider ? { authProvider } : {}), + }); } catch (error) { const message = getErrorMessage(error); return { success: false, error: message }; @@ -4685,13 +4979,20 @@ export class MCPServerManager { if (decide() === "retry") { continue; } + // Recheck live authorization synchronously after the fenced read. + const dispatch = (): Promise | "retry" => + decide() === "retry" ? "retry" : Promise.resolve(originalExecute(args, context)); const acquireOverridesLock = this.pluginInvalidation?.acquireOverridesLock; const readOverridesEpoch = this.pluginInvalidation?.readOverridesEpoch; if (acquireOverridesLock === undefined || readOverridesEpoch === undefined) { - // No cross-process writers to fence: the checks above ran in the - // same synchronous block as this invocation start. - const result: unknown = await Promise.resolve(originalExecute(args, context)); - return result; + const { pending } = await this.withComponentPolicyFence( + serverName, + servedInfo, + dispatch, + { signal: abortSignal, timeoutMs: remainingMs() } + ); + if (pending === "retry") continue; + return await pending; } // Cross-process fence. The bracket's postflight epoch read and this // invocation are separated by promise continuations, and a sibling @@ -4721,7 +5022,7 @@ export class MCPServerManager { acquisition.then((lateRelease) => lateRelease()).catch(() => undefined); throw error; } - let pending: Promise | undefined; + let pending: Promise | "retry"; try { const epochNow = await bounded(readOverridesEpoch()); if ( @@ -4733,15 +5034,14 @@ export class MCPServerManager { // iteration's preflight evicts and re-derives from disk. continue; } - // Process-local state may have moved during the two awaits above. - if (decide() === "retry") { - continue; - } - // Synchronous from the last check to the invocation start, under the lock. - pending = Promise.resolve(originalExecute(args, context)); + ({ pending } = await this.withComponentPolicyFence(serverName, servedInfo, dispatch, { + signal: abortSignal, + timeoutMs: remainingMs(), + })); } finally { await release(); } + if (pending === "retry") continue; return await pending; } throw new Error( @@ -5128,10 +5428,13 @@ export class MCPServerManager { * the epoch) before the read — observed here, the launch refused — or * waits until the process exists, after which the bracket's postflight * closes it. The lock is released as soon as exec returned; the MCP - * handshake never runs under it. Without cross-process tracking there is - * nothing to fence: plain launch. + * handshake never runs under it. Managed components also acquire the + * plugin writer lock after overrides, through the same launch interval. + * Untracked, unmanaged servers retain their plain launch path. */ private async launchUnderOverrideFence( + name: string, + info: MCPServerInfo, /** `launchSignal` aborts with the startup signal AND at an `abortAfterMs` deadline. */ launch: (launchSignal: AbortSignal) => Promise, signal: AbortSignal, @@ -5159,19 +5462,20 @@ export class MCPServerManager { ): Promise { const acquireOverridesLock = this.pluginInvalidation?.acquireOverridesLock; const readOverridesEpoch = this.pluginInvalidation?.readOverridesEpoch; - if ( - acquireOverridesLock === undefined || - readOverridesEpoch === undefined || - !this.pluginInvalidationTokenSeen - ) { - return launch(signal); - } - // ONE deadline for acquisition and the fenced read (see getPrompt). + const trackOverrides = + acquireOverridesLock !== undefined && + readOverridesEpoch !== undefined && + this.pluginInvalidationTokenSeen; + const plugin = this.managedPluginServers.get(name) ?? info.plugin; + if (!trackOverrides && plugin?.componentPolicy === undefined) return launch(signal); + // ONE deadline for acquisition and the fenced reads (see getPrompt). const fenceDeadlineAt = Date.now() + CALL_GATE_TIMEOUT_MS; - const release = await acquireOverridesLock({ - timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), - signal, - }); + const release = trackOverrides + ? await acquireOverridesLock({ + timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), + signal, + }) + : () => Promise.resolve(); let released = false; const releaseOnce = async () => { if (!released) { @@ -5180,26 +5484,36 @@ export class MCPServerManager { } }; let pending: Promise | undefined; + let releaseComponents: (() => Promise) | undefined; try { - const epochRead = await raceWithAbortAndTimeout(readOverridesEpoch(), { - timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), + if (trackOverrides) { + const epochRead = await raceWithAbortAndTimeout(readOverridesEpoch(), { + timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), + signal, + }); + if (epochRead.kind !== "ok") { + throw new Error( + epochRead.kind === "aborted" + ? "MCP server startup was aborted" + : "MCP server startup could not read the workspace MCP settings marker in time; retry" + ); + } + if ( + epochRead.value !== this.lastOverridesEpochToken || + isWorkspaceOverridesEpochUnreadable(epochRead.value) + ) { + throw new Error( + "Workspace MCP settings changed in another process (or their change marker is unreadable) while MCP servers were about to start; retry" + ); + } + } + // Try the plugin writer lock SECOND: uninstall holds it while pruning + // overrides. Keep it through actual exec/connection initiation, not the + // earlier discovery or semaphore wait, so removal cannot precede a spawn. + releaseComponents = await this.acquireComponentPolicyFence(name, info, { signal, + timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), }); - if (epochRead.kind !== "ok") { - throw new Error( - epochRead.kind === "aborted" - ? "MCP server startup was aborted" - : "MCP server startup could not read the workspace MCP settings marker in time; retry" - ); - } - if ( - epochRead.value !== this.lastOverridesEpochToken || - isWorkspaceOverridesEpochUnreadable(epochRead.value) - ) { - throw new Error( - "Workspace MCP settings changed in another process (or their change marker is unreadable) while MCP servers were about to start; retry" - ); - } if (options?.abortAfterMs !== undefined) { const { ms, serverName } = options.abortAfterMs; const launchAbort = new AbortController(); @@ -5241,7 +5555,11 @@ export class MCPServerManager { // Released here — BEFORE the remaining wait on a still-pending remote // handshake below: every settings save and prune would otherwise queue // behind an endpoint-controlled request for the whole startup deadline. - await releaseOnce(); + try { + await releaseComponents?.(); + } finally { + await releaseOnce(); + } } return await pending; } @@ -5264,6 +5582,8 @@ export class MCPServerManager { log.debug("[MCP] Spawning stdio server", { name }); const launch = await prepareStdioLaunch(info); const execStream = await this.launchUnderOverrideFence( + name, + info, (launchSignal) => runtime.exec(launch.command, { cwd: launch.cwd ?? workspacePath, @@ -5549,6 +5869,8 @@ export class MCPServerManager { // client but cannot undo traffic or credentials already sent. const tryHttp = () => this.launchUnderOverrideFence( + name, + info, () => createMCPClient({ transport: { @@ -5564,6 +5886,8 @@ export class MCPServerManager { const trySse = () => this.launchUnderOverrideFence( + name, + info, () => createMCPClient({ transport: { diff --git a/tests/ipc/agentPlugins.test.ts b/tests/ipc/agentPlugins.test.ts index 87d3c62c71..5dadb347af 100644 --- a/tests/ipc/agentPlugins.test.ts +++ b/tests/ipc/agentPlugins.test.ts @@ -64,7 +64,7 @@ function unwrap(result: Result): T { if (remote) await cleanupTempGitRepo(remote); }); - it("imports a subset, adds offline, and requires new consent after an update", async () => { + it("imports a subset, replaces offline, and requires new consent after an update", async () => { const preview = unwrap( await env.orpc.agentPlugins.preview({ input: pathToFileURL(remote).href }) ); @@ -108,12 +108,12 @@ function unwrap(result: Result): T { expect( ( - await env.orpc.agentPlugins.addComponents({ + await env.orpc.agentPlugins.setComponents({ name, expectedLockedSha: "outdated", expectedContentHash: inventory.contentHash, - skills: ["selective-second"], - mcpServers: ["selective-second"], + expectedImportedComponents: selection, + importedComponents: { skills: ["selective-second"], mcpServers: ["selective-second"] }, }) ).success ).toBe(false); @@ -121,7 +121,7 @@ function unwrap(result: Result): T { selection ); - // No remote is available: additions must use the installed tree, not clone or fetch. + // No remote is available: selections must use the installed tree, not clone or fetch. const offlineRemote = `${remote}-offline`; await fs.rename(remote, offlineRemote); try { @@ -129,23 +129,35 @@ function unwrap(result: Result): T { name, expectedLockedSha: inventory.lockedSha, expectedContentHash: inventory.contentHash, - skills: ["selective-second", "selective-second"], - mcpServers: ["selective-second"], + expectedImportedComponents: selection, + importedComponents: { + skills: ["selective-second", "selective-second"], + mcpServers: ["selective-second"], + }, }; - const added = unwrap(await env.orpc.agentPlugins.addComponents(addition)); - expect(added.importedComponents?.skills.sort()).toEqual([ - "selective-first", - "selective-second", - ]); - expect(unwrap(await env.orpc.agentPlugins.addComponents(addition))).toEqual(added); + const added = unwrap(await env.orpc.agentPlugins.setComponents(addition)); + expect(added.importedComponents?.skills.sort()).toEqual(["selective-second"]); + expect( + unwrap( + await env.orpc.agentPlugins.setComponents({ + ...addition, + expectedImportedComponents: added.importedComponents ?? null, + }) + ) + ).toEqual(added); } finally { await fs.rename(offlineRemote, remote); } const serversAfter = await env.orpc.projects.mcp.list({ projectPath: remote }); - for (const [key, server] of Object.entries(serversBefore)) { - expect(serversAfter[key]).toEqual(server); - } - expect(Object.values(serversAfter).filter((server) => server.plugin)).toHaveLength(2); + expect( + Object.values(serversAfter) + .filter((server) => server.plugin) + .map((server) => server.plugin?.serverName) + ).toEqual(["selective-second"]); + expect(Object.values(serversAfter).filter((server) => server.plugin)[0].disabled).toBe(true); + await expect( + env.orpc.agentSkills.get({ projectPath: remote, skillName: "selective-first" }) + ).rejects.toThrow(); expect( (await env.orpc.agentSkills.list({ projectPath: remote })).some( (skill) => skill.name === "selective-second" @@ -170,23 +182,23 @@ function unwrap(result: Result): T { ).toBe(false); expect( ( - await env.orpc.agentPlugins.addComponents({ + await env.orpc.agentPlugins.setComponents({ name, expectedLockedSha: inventory.lockedSha, expectedContentHash: inventory.contentHash, - skills: ["selective-third"], - mcpServers: [], + expectedImportedComponents: selection, + importedComponents: { skills: ["selective-third"], mcpServers: [] }, }) ).success ).toBe(false); unwrap( - await env.orpc.agentPlugins.addComponents({ + await env.orpc.agentPlugins.setComponents({ name, expectedLockedSha: updated.lockedSha, expectedContentHash: unwrap(await env.orpc.agentPlugins.getComponents({ name })) .contentHash, - skills: ["selective-third"], - mcpServers: [], + expectedImportedComponents: updated.importedComponents ?? null, + importedComponents: { skills: ["selective-third"], mcpServers: [] }, }) ); expect( diff --git a/tests/ui/config/pluginImports.test.ts b/tests/ui/config/pluginImports.test.ts index a867592bca..81c972ab07 100644 --- a/tests/ui/config/pluginImports.test.ts +++ b/tests/ui/config/pluginImports.test.ts @@ -8,6 +8,7 @@ import { preloadTestModules } from "../../ipc/setup"; import { createTempGitRepo, cleanupTempGitRepo } from "../../ipc/helpers"; import { createAppHarness, type AppHarness } from "../harness"; import { EXPERIMENT_IDS } from "@/common/constants/experiments"; +import { subscribeAgentPluginsMutated } from "@/browser/utils/agentPluginMutations"; import { AGENT_PLUGIN_SCHEMA_ID_1_0_0 } from "@/node/services/agentPlugins/manifest"; import { AGENT_PLUGIN_MCP_SCHEMA_ID_1_0_0 } from "@/node/services/agentPlugins/mcpConfig"; import { execFileAsync } from "@/node/utils/disposableExec"; @@ -89,7 +90,7 @@ describeIntegration("Selective plugin imports", () => { await cleanupTempGitRepo(remote); }); - test("empty install retains choices after failure; add is additive, cancellable, keyboard accessible and retryable", async () => { + test("empty install retains choices after failure; management is reversible, cancellable, keyboard accessible and retryable", async () => { const { canvas, user } = await openPreview(app, remote); for (const checkbox of canvas.getAllByRole("checkbox")) expect(checkbox.getAttribute("aria-checked")).toBe("true"); @@ -121,35 +122,35 @@ describeIntegration("Selective plugin imports", () => { expect(installSpy).toHaveBeenCalledTimes(2); expect((await inventory(app)).importedComponents).toEqual({ skills: [], mcpServers: [] }); - await user.click(canvas.getByRole("button", { name: "Add components to review-tools" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); await canvas.findByRole("checkbox", { name: "research" }); - expect(canvas.getByRole("button", { name: "Import selected" }).hasAttribute("disabled")).toBe( + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( true ); await user.click(canvas.getByRole("checkbox", { name: "review" })); await user.click(canvas.getByRole("button", { name: "Cancel" })); expect((await inventory(app)).importedComponents?.skills).toEqual([]); - await user.click(canvas.getByRole("button", { name: "Add components to review-tools" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); expect( (await canvas.findByRole("checkbox", { name: "review" })).getAttribute("aria-checked") ).toBe("false"); await user.click(canvas.getByRole("checkbox", { name: "review" })); jest - .spyOn(app.env.services.agentPluginInstallService, "addComponents") + .spyOn(app.env.services.agentPluginInstallService, "setComponents") .mockRejectedValueOnce(new Error("Registry busy")); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByText("Registry busy"); await waitFor(() => - expect(canvas.getByRole("button", { name: "Import selected" }).hasAttribute("disabled")).toBe( + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( false ) ); expect(canvas.getByRole("checkbox", { name: "review" }).getAttribute("aria-checked")).toBe( "true" ); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByText(/1 of 2 skills imported/); - expect(canvas.getByRole("checkbox", { name: "review" }).hasAttribute("disabled")).toBe(true); + expect(canvas.getByRole("checkbox", { name: "review" }).hasAttribute("disabled")).toBe(false); expect((await inventory(app)).importedComponents).toEqual({ skills: ["review"], mcpServers: [], @@ -159,18 +160,279 @@ describeIntegration("Selective plugin imports", () => { within(canvas.getByRole("group", { name })).getByRole("button", { name: "Select all" }) ); } - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByText(/2 of 2 skills imported/); - await canvas.findByText(/already imported/); - expect(canvas.getByRole("button", { name: "Import selected" }).hasAttribute("disabled")).toBe( + + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( true ); expect((await inventory(app)).importedComponents).toEqual({ skills: ["research", "review"], mcpServers: ["reference"], }); + // Imported rows stay editable, and clearing all keeps the package installed. + await user.click(canvas.getByRole("checkbox", { name: "review" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); + await canvas.findByText(/1 of 2 skills imported/); + await user.click(canvas.getByRole("button", { name: "Done" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + expect( + (await canvas.findByRole("checkbox", { name: "review" })).getAttribute("aria-checked") + ).toBe("false"); + expect(canvas.queryByText("Component selection saved.")).toBeNull(); + for (const name of ["Skills", "MCP servers"]) { + await user.click( + within(canvas.getByRole("group", { name })).getByRole("button", { name: "Clear" }) + ); + } + const save = canvas.getByRole("button", { name: "Save changes" }); + expect(save.hasAttribute("disabled")).toBe(false); + save.focus(); + await user.keyboard("{Enter}"); + await canvas.findByText(/0 of 2 skills imported/); + expect((await inventory(app)).importedComponents).toEqual({ skills: [], mcpServers: [] }); + expect( + canvas.getByRole("button", { name: "Manage components for review-tools" }) + ).toBeDefined(); + await user.click(canvas.getByRole("button", { name: "Done" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + for (const checkbox of await canvas.findAllByRole("checkbox")) + expect(checkbox.getAttribute("aria-checked")).toBe("false"); }, 120000); + test.each(["lost response", "cleanup warning", "selection conflict"] as const)( + "management recovers from %s using persisted selection, without automatic resubmission", + async (failure) => { + const { canvas, user } = await openPreview(app, remote); + await user.click(canvas.getByRole("checkbox", { name: "research" })); + await user.click(canvas.getByRole("checkbox", { name: "reference" })); + await user.click(canvas.getByRole("button", { name: "Install" })); + await canvas.findByText(/1 of 2 skills imported/, {}, { timeout: 10000 }); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + await user.click(await canvas.findByRole("checkbox", { name: "review" })); + await user.click(canvas.getByRole("checkbox", { name: "research" })); + const backend = app.env.services.agentPluginInstallService; + const original = backend.setComponentsResult.bind(backend); + const mutation = jest + .spyOn(backend, "setComponentsResult") + .mockImplementationOnce(async (input) => { + if (failure === "selection conflict") { + // Another settings client wins the compare-and-swap after this panel's review. + await backend.setComponents({ + ...input, + importedComponents: { skills: [], mcpServers: ["reference"] }, + }); + } + const result = await original(input); + if (failure === "lost response") throw new Error("Connection lost after persistence"); + return failure === "cleanup warning" && result.success + ? { ...result, cleanupWarning: "Components saved; MCP cleanup needs retry" } + : result; + }); + await user.click(canvas.getByRole("button", { name: "Save changes" })); + await waitFor(() => + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( + true + ) + ); + if (failure === "selection conflict") { + await canvas.findByRole("alert"); + await canvas.findByText(/0 of 2 skills imported/); + await canvas.findByText(/1 of 1 MCP servers imported/); + await waitFor(() => + expect( + canvas.getByRole("checkbox", { name: "reference" }).getAttribute("aria-checked") + ).toBe("true") + ); + expect( + canvas.getByRole("checkbox", { name: "research" }).getAttribute("aria-checked") + ).toBe("false"); + expect(mutation).toHaveBeenCalledTimes(1); + await user.click(canvas.getByRole("checkbox", { name: "research" })); + await user.click(canvas.getByRole("checkbox", { name: "reference" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); + } + await waitFor(() => + expect(canvas.getByRole("button", { name: "Done" }).hasAttribute("disabled")).toBe(false) + ); + expect((await inventory(app)).importedComponents).toEqual({ + skills: ["research"], + mcpServers: [], + }); + expect(mutation).toHaveBeenCalledTimes(failure === "selection conflict" ? 2 : 1); + expect(canvas.getByRole("checkbox", { name: "review" }).getAttribute("aria-checked")).toBe( + "false" + ); + if (failure === "cleanup warning") + expect(canvas.getByRole("alert").textContent).toContain("cleanup"); + else expect(canvas.queryByRole("alert")).toBeNull(); + await user.click(canvas.getByRole("button", { name: "Done" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + expect( + (await canvas.findByRole("checkbox", { name: "research" })).getAttribute("aria-checked") + ).toBe("true"); + expect(canvas.queryByRole("alert")).toBeNull(); + }, + 120000 + ); + + test.each(["lost response", "acknowledged save", "rejected save"] as const)( + "%s with failed confirmation invalidates availability and counts only when a commit is possible", + async (failure) => { + const { canvas, user } = await openPreview(app, remote); + await user.click(canvas.getByRole("checkbox", { name: "research" })); + await user.click(canvas.getByRole("checkbox", { name: "reference" })); + await user.click(canvas.getByRole("button", { name: "Install" })); + await canvas.findByText(/1 of 2 skills imported/, {}, { timeout: 10000 }); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + await user.click(await canvas.findByRole("checkbox", { name: "review" })); + + // Model a still-mounted availability consumer with real discovery, not an event-name assertion. + const readSkills = () => app.env.orpc.agentSkills.list({ workspaceId: app.workspaceId }); + let available = await readSkills(); + expect(available.some((skill) => skill.name === "review")).toBe(true); + let consumerRefresh = Promise.resolve(); + let refreshCount = 0; + const unsubscribe = subscribeAgentPluginsMutated(() => { + refreshCount++; + consumerRefresh = readSkills().then((skills) => { + available = skills; + }); + }); + const backend = app.env.services.agentPluginInstallService; + const original = backend.setComponentsResult.bind(backend); + const mutation = jest + .spyOn(backend, "setComponentsResult") + .mockImplementationOnce(async (input) => { + if (failure === "rejected save") return { success: false, error: "Registry unavailable" }; + const result = await original(input); + expect(result.success).toBe(true); + if (failure === "acknowledged save") return result; + throw new Error("Connection lost after persistence"); + }); + jest + .spyOn(backend, "getComponents") + .mockRejectedValueOnce(new Error("Confirmation unavailable")); + try { + await user.click(canvas.getByRole("button", { name: "Save changes" })); + await waitFor(() => + expect( + canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled") + ).toBe(false) + ); + await consumerRefresh; + expect(available.some((skill) => skill.name === "review")).toBe( + failure === "rejected save" + ); + expect(refreshCount).toBe(failure === "rejected save" ? 0 : 1); + expect( + canvas.getByText( + failure === "rejected save" ? /1 of 2 skills imported/ : /0 of 2 skills imported/ + ) + ).toBeDefined(); + expect(canvas.getByRole("alert")).toBeDefined(); + expect(canvas.queryByRole("button", { name: "Done" })).toBeNull(); + expect(canvas.getByRole("checkbox", { name: "review" }).getAttribute("aria-checked")).toBe( + "false" + ); + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( + false + ); + expect(mutation).toHaveBeenCalledTimes(1); + await user.click(canvas.getByRole("button", { name: "Cancel" })); + await user.click( + canvas.getByRole("button", { name: "Manage components for review-tools" }) + ); + expect( + (await canvas.findByRole("checkbox", { name: "review" })).getAttribute("aria-checked") + ).toBe(failure === "rejected save" ? "true" : "false"); + expect( + canvas.getByText( + failure === "rejected save" ? /1 of 2 skills imported/ : /0 of 2 skills imported/ + ) + ).toBeDefined(); + } finally { + unsubscribe(); + await consumerRefresh; + } + }, + 120000 + ); + + test.each([ + { skills: ["review"], mcpServers: [] }, + { skills: ["research"], mcpServers: ["reference"] }, + ])( + "acknowledged save superseded before confirmation shows the current selection: %j", + async (latest) => { + const { canvas, user } = await openPreview(app, remote); + await user.click(canvas.getByRole("checkbox", { name: "research" })); + await user.click(canvas.getByRole("checkbox", { name: "reference" })); + await user.click(canvas.getByRole("button", { name: "Install" })); + await canvas.findByText(/1 of 2 skills imported/, {}, { timeout: 10000 }); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); + await user.click(await canvas.findByRole("checkbox", { name: "review" })); + const backend = app.env.services.agentPluginInstallService; + const read = backend.getComponents.bind(backend); + const mutation = jest.spyOn(backend, "setComponentsResult"); + jest.spyOn(backend, "getComponents").mockImplementationOnce(async (input) => { + // This read starts only after our acknowledged write; another writer wins before it returns. + const receipt = await read(input); + expect(receipt.importedComponents).toEqual({ skills: [], mcpServers: [] }); + await backend.setComponents({ + name: input.name, + expectedLockedSha: receipt.lockedSha, + expectedContentHash: receipt.contentHash, + expectedImportedComponents: receipt.importedComponents ?? null, + importedComponents: latest, + }); + return read(input); + }); + await user.click(canvas.getByRole("button", { name: "Save changes" })); + await waitFor(() => + expect( + canvas.getByRole("checkbox", { name: latest.skills[0] }).getAttribute("aria-checked") + ).toBe("true") + ); + expect(canvas.getByRole("alert")).toBeDefined(); + expect(canvas.queryByRole("button", { name: "Done" })).toBeNull(); + await waitFor(() => + expect(canvas.getByRole("button", { name: "Cancel" }).hasAttribute("disabled")).toBe(false) + ); + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( + true + ); + expect(canvas.getByText(/1 of 2 skills imported/)).toBeDefined(); + expect( + canvas.getByText( + latest.mcpServers.length ? /1 of 1 MCP servers imported/ : /0 of 1 MCP servers imported/ + ) + ).toBeDefined(); + for (const name of ["review", "research"]) { + expect(canvas.getByRole("checkbox", { name }).getAttribute("aria-checked")).toBe( + latest.skills.includes(name) ? "true" : "false" + ); + } + expect(canvas.getByRole("checkbox", { name: "reference" }).getAttribute("aria-checked")).toBe( + latest.mcpServers.length ? "true" : "false" + ); + expect(mutation).toHaveBeenCalledTimes(1); + for (const group of ["Skills", "MCP servers"]) { + await user.click( + within(canvas.getByRole("group", { name: group })).getByRole("button", { name: "Clear" }) + ); + } + await user.click(canvas.getByRole("button", { name: "Save changes" })); + await canvas.findByText(/0 of 2 skills imported/); + await waitFor(() => + expect(canvas.getByRole("button", { name: "Done" }).hasAttribute("disabled")).toBe(false) + ); + expect(mutation).toHaveBeenCalledTimes(2); + expect(canvas.queryByRole("alert")).toBeNull(); + }, + 120000 + ); + test.each(["button", "keyboard", "palette"] as const)( "reopening installation via %s clears prior success through the next failed attempt", async (entryPoint) => { @@ -221,7 +483,7 @@ describeIntegration("Selective plugin imports", () => { await user.click(canvas.getByRole("checkbox", { name: "reference" })); await user.click(canvas.getByRole("button", { name: "Install" })); await canvas.findByText(/1 of 2 skills imported/, {}, { timeout: 10000 }); - await user.click(canvas.getByRole("button", { name: "Add components to review-tools" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); await user.click(await canvas.findByRole("checkbox", { name: "research" })); const before = await inventory(app); const skillFile = path.join( @@ -233,12 +495,12 @@ describeIntegration("Selective plugin imports", () => { "SKILL.md" ); await fs.appendFile(skillFile, "Changed local skill instructions\n"); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await waitFor(() => { expect(canvas.getByRole("checkbox", { name: "research" }).getAttribute("aria-checked")).toBe( "false" ); - expect(canvas.getByRole("button", { name: "Import selected" }).hasAttribute("disabled")).toBe( + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( true ); }); @@ -248,7 +510,7 @@ describeIntegration("Selective plugin imports", () => { expect(refreshed.contentHash).not.toBe(before.contentHash); expect(refreshed.importedComponents?.skills).toEqual(["review"]); await user.click(canvas.getByRole("checkbox", { name: "research" })); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByText(/2 of 2 skills imported/); expect((await inventory(app)).importedComponents?.skills).toEqual(["research", "review"]); }, 120000); @@ -268,7 +530,7 @@ describeIntegration("Selective plugin imports", () => { jest .spyOn(app.env.services.agentPluginInstallService, "getComponents") .mockRejectedValueOnce(new Error("Inventory unavailable")); - await user.click(canvas.getByRole("button", { name: "Add components to review-tools" })); + await user.click(canvas.getByRole("button", { name: "Manage components for review-tools" })); await canvas.findByText("Inventory unavailable"); await user.click(canvas.getByRole("button", { name: "Retry inventory" })); await user.click(await canvas.findByRole("checkbox", { name: "research" })); @@ -281,14 +543,14 @@ describeIntegration("Selective plugin imports", () => { await waitFor(async () => expect((await inventory(app)).lockedSha).not.toBe(before.lockedSha), { timeout: 10000, }); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByRole("alert"); await waitFor(() => expect(canvas.getByRole("checkbox", { name: "research" }).getAttribute("aria-checked")).toBe( "false" ) ); - expect(canvas.getByRole("button", { name: "Import selected" }).hasAttribute("disabled")).toBe( + expect(canvas.getByRole("button", { name: "Save changes" }).hasAttribute("disabled")).toBe( true ); expect((await inventory(app)).importedComponents).toEqual({ @@ -296,7 +558,7 @@ describeIntegration("Selective plugin imports", () => { mcpServers: [], }); await user.click(canvas.getByRole("checkbox", { name: "research" })); - await user.click(canvas.getByRole("button", { name: "Import selected" })); + await user.click(canvas.getByRole("button", { name: "Save changes" })); await canvas.findByText(/2 of 2 skills imported/); expect((await inventory(app)).importedComponents?.skills).toEqual(["research", "review"]); }, 120000);