From be918411f17dfaedf9c692a0ca33aa06af6f0504 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 10:30:08 +0000 Subject: [PATCH 01/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20enforce=20installed?= =?UTF-8?q?=20plugin=20MCP=20component=20removals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use canonical plugins.json selection snapshots at discovery, startup, and call authorization boundaries. Preserve retained server connections and retire only removed components, deferring leased clients until release. Remember managed installation provenance so missing registry rows cannot reclassify hidden sibling components as unmanaged. Validation: 583 unit tests passed (1 Windows-only skip); typecheck, touched ESLint, Prettier, and git diff --check passed. No UI or public API changes. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$4.69`_ --- src/common/orpc/schemas/mcp.ts | 1 + src/common/types/mcp.ts | 2 + src/node/services/agentPlugins/discovery.ts | 5 + .../services/agentPlugins/mcpConfig.test.ts | 61 ++- src/node/services/agentPlugins/mcpConfig.ts | 14 + .../services/agentPlugins/registry.test.ts | 58 ++- src/node/services/agentPlugins/registry.ts | 60 +++ src/node/services/di/layers/core.ts | 6 + src/node/services/mcpServerManager.test.ts | 428 +++++++++++++++++- src/node/services/mcpServerManager.ts | 192 +++++++- 10 files changed, 814 insertions(+), 13 deletions(-) diff --git a/src/common/orpc/schemas/mcp.ts b/src/common/orpc/schemas/mcp.ts index ad286f383f2..e40e32c5f90 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 552f6d8800c..781fabfc355 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/services/agentPlugins/discovery.ts b/src/node/services/agentPlugins/discovery.ts index 8ab12099da8..b5fdcfe1f11 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/mcpConfig.test.ts b/src/node/services/agentPlugins/mcpConfig.test.ts index 0c8aedf2b68..767abd926f1 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 32597ddd05d..d7baa54e4e2 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 126279be817..906fe772359 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 c46f5e33b49..7b52b633a97 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/di/layers/core.ts b/src/node/services/di/layers/core.ts index 6abe54f1dec..b182813eac9 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"; @@ -373,6 +377,8 @@ export const MCPServerManagerLive = Layer.effect( telemetryService: opts.telemetryService, pluginInvalidation: { keyPrefix: PLUGIN_SERVER_KEY_PREFIX, + readComponentPolicy: () => + readPluginMcpPolicy(path.join(mcpConfig.rootDir, PLUGIN_REGISTRY_FILE_NAME)), 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 22f1ed042e6..95e30d46804 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -10,6 +10,9 @@ import { MCP_PROMPT_TRUNCATION_MARKER, } from "@/common/constants/toolLimits"; import { 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, @@ -103,7 +106,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 +234,429 @@ 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[]) => { + await fs.writeFile( + `${registryPath}.tmp`, + JSON.stringify({ + plugins: [createTestPluginInstallEntry("demo", { skills: [], mcpServers: names })], + }) + ); + await fs.rename(`${registryPath}.tmp`, registryPath); + }; + await write(["remove", "keep"]); + const configs: Record = { ordinary: stdioConfig("ordinary") }; + for (const name of ["remove", "keep", "added"]) { + configs[`plugin:instance:${name}`] = { + ...stdioConfig(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 instance = new MCPServerManager(configService as unknown as MCPConfigService, { + pluginInvalidation: { + keyPrefix: "plugin:", + readToken: () => Promise.resolve(undefined), + readComponentPolicy: read, + }, + }); + 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 }; + }; + 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("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("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([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 diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index 34cd7cb0c42..26abcb6cac9 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,8 @@ export interface MCPServerManagerOptions { pluginInvalidation?: { keyPrefix: string; readToken: () => Promise; + /** Atomic plugins.json content, independent of tree replacement and override epochs. */ + readComponentPolicy?: () => Promise; /** * Disk-authoritative workspace override read. A sibling's uninstall also * pruned plugin keys from workspace override FILES; the sweep uses this @@ -1311,6 +1316,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 +1363,86 @@ 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(); + } + + 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 refreshComponentPolicy(): Promise { + 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; + delete entry.retiredPluginInstances; + for (const instance of retired ?? []) { + try { + await instance.close(); + } catch (error) { + log.warn("Failed to close removed plugin component", { name: instance.name, error }); + } + } + } + } + /** * Retire cached plugin instances when a SIBLING process mutated a plugin * (see MCPServerManagerOptions.pluginInvalidation). Runs before every @@ -1372,6 +1464,7 @@ export class MCPServerManager { // published token and proceed; a failed sweep leaves the token // unpublished so the next serve retries it. const run = async (): Promise => { + if (invalidation.readComponentPolicy !== undefined) await this.refreshComponentPolicy(); const [token, overridesEpoch] = await Promise.all([ invalidation.readToken(), invalidation.readOverridesEpoch?.(), @@ -1858,6 +1951,14 @@ export class MCPServerManager { if (current === 1) { this.workspaceLeases.delete(workspaceId); + if (this.pluginInvalidation?.readComponentPolicy !== undefined) { + this.reconcilePluginComponents().catch((error: unknown) => { + log.warn("Failed to reconcile plugin components after lease release", { + workspaceId, + error, + }); + }); + } return; } @@ -1961,7 +2062,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 +2099,8 @@ export class MCPServerManager { trusted = false, agentPlugins?: AgentPluginsMcpContext | null ): Promise { + if (this.pluginInvalidation?.readComponentPolicy !== undefined) + await this.reconcilePluginComponents(); const allServers = await this.getAllServers(projectPath, trusted, agentPlugins); const enabled = this.applyServerOverrides(allServers, overrides); return this.filterServersByPolicy(enabled); @@ -2127,6 +2244,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 +2262,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) @@ -2668,7 +2793,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. @@ -2922,7 +3047,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 @@ -3304,21 +3429,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,6 +3930,12 @@ export class MCPServerManager { const readOverridesEpoch = this.pluginInvalidation?.readOverridesEpoch; let pending: ReturnType | "retry"; if (acquireOverridesLock === undefined || readOverridesEpoch === undefined) { + const policy = + this.pluginInvalidation?.readComponentPolicy !== undefined + ? await this.readComponentPolicy() + : undefined; + if (!this.componentAllowed(serverName, undefined, policy)) + throw new Error(`MCP server '${serverName}' is disabled`); pending = dispatch(); } else { // ONE deadline for acquisition and the fenced epoch read: the outer @@ -3829,6 +3966,12 @@ export class MCPServerManager { ) { return { epochMoved: true } as const; } + const policy = + this.pluginInvalidation?.readComponentPolicy !== undefined + ? await this.readComponentPolicy() + : undefined; + if (!this.componentAllowed(serverName, undefined, policy)) + throw new Error(`MCP server '${serverName}' is disabled`); pending = dispatch(); } finally { await release(); @@ -4028,7 +4171,24 @@ export class MCPServerManager { publish: (invalidatedKeys: string[]) => void ): Promise { const invalidatedKeys: string[] = []; + const removedComponents: string[] = []; for (;;) { + if (this.pluginInvalidation?.readComponentPolicy !== undefined) + await this.reconcilePluginComponents(); + 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) { + log.warn("Failed to close removed plugin startup", { name, error }); + } + } + if (removedComponents.length > 0) await this.reconcilePluginComponents(); const clockBeforeScan = this.prefixInvalidationClock; invalidatedKeys.push( ...(await this.closeInvalidatedInstances(instances, startedAtEpoch, workspaceId)) @@ -4036,7 +4196,9 @@ 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)) + ); return; } } @@ -4062,7 +4224,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) { @@ -4682,6 +4844,12 @@ export class MCPServerManager { } return "dispatch"; }; + const componentPolicy = + this.pluginInvalidation?.readComponentPolicy !== undefined + ? await bounded(this.readComponentPolicy()) + : undefined; + if (!this.componentAllowed(serverName, servedInfo, componentPolicy)) + throw revoked(`server '${serverName}'`); if (decide() === "retry") { continue; } @@ -4733,6 +4901,12 @@ export class MCPServerManager { // iteration's preflight evicts and re-derives from disk. continue; } + const componentPolicy = + this.pluginInvalidation?.readComponentPolicy !== undefined + ? await bounded(this.readComponentPolicy()) + : undefined; + if (!this.componentAllowed(serverName, servedInfo, componentPolicy)) + throw revoked(`server '${serverName}'`); // Process-local state may have moved during the two awaits above. if (decide() === "retry") { continue; From 408bcd4517931e9a1a2e2f7418e52dfe093286a2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 10:53:26 +0000 Subject: [PATCH 02/12] =?UTF-8?q?=F0=9F=A4=96=20feat:=20manage=20installed?= =?UTF-8?q?=20plugin=20component=20selections?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace additive imports with full, consent-bound selections for installed plugins. Preserve plugin trees, data, identities and workspace MCP preferences while reporting post-save cleanup failures as warnings. Add reversible inline management, conflict/lost-response recovery, full-App stories and documentation. Validation: 591 unit/sibling tests passed (1 platform skip), 10 IPC/UI tests, 6 Storybook interactions, make static-check and make static-check-full passed. Recorded desktop/mobile sandbox UAT remains with the integrating parent. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh`_ --- docs/agents/agent-skills.mdx | 4 +- .../Sections/PluginsSettingsSection.tsx | 228 +++++++----- .../Sections/pluginsSectionIntents.ts | 2 +- .../stories/App.pluginImports.stories.tsx | 91 ++++- src/browser/stories/mocks/orpc.ts | 10 + src/browser/utils/commandIds.ts | 2 +- src/browser/utils/commands/sources.test.ts | 13 +- src/browser/utils/commands/sources.ts | 24 +- src/common/orpc/schemas/api.test.ts | 19 + src/common/orpc/schemas/api.ts | 15 +- src/node/orpc/router.ts | 8 +- .../agentPlugins/installService.test.ts | 336 +++++++++++++----- .../services/agentPlugins/installService.ts | 63 ++-- .../builtInSkillContent.generated.ts | 4 +- tests/ipc/agentPlugins.test.ts | 58 +-- tests/ui/config/pluginImports.test.ts | 142 ++++++-- 16 files changed, 748 insertions(+), 271 deletions(-) diff --git a/docs/agents/agent-skills.mdx b/docs/agents/agent-skills.mdx index aa9008bea18..fcbe4b15b27 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 024638e1102..bcec8aaa9dd 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,42 @@ const ComponentChooser: React.FC<{ ); -const AddComponentsPanel: React.FC<{ +function effectiveImports(inventory: AgentPluginComponents): AgentPluginImportedComponents { + 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 +204,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 +213,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 +231,96 @@ 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 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; + setSaved(true); 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, selected)) + ) { + setInventory(current.data); + setSelected(effectiveImports(current.data)); + setSaved(true); + setError(warning ?? null); + if (!confirmed) publishAgentPluginsMutated(); + await props.onSaved(); + } else if ( + !sameTree || + !sameImports(current.data.importedComponents ?? null, inventory.importedComponents ?? null) + ) { + setInventory(current.data); + setSelected(effectiveImports(current.data)); + setError( + "The installed plugin or selection changed. Inventory refreshed; review your choices and save again." + ); } + } 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.` + ); } 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 +328,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 +818,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 +907,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 +1179,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 +1225,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 afd2ff1f5ae..92be0285490 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 8a56f71c739..544ed22b067 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 7ceb461107f..ce8958cd4f0 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 5e17db10fde..4a154336ded 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 dd7f8bf6354..2a4aa19d937 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 346072915cd..1246ceb8ff0 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 abee162fbf3..f0be10232e1 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 2ba5c6e594b..6f13bba02e2 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/node/orpc/router.ts b/src/node/orpc/router.ts index b9430d9263f..b20ae5559a9 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/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index b4b015ff384..ee0e8acad81 100644 --- a/src/node/services/agentPlugins/installService.test.ts +++ b/src/node/services/agentPlugins/installService.test.ts @@ -10,6 +10,7 @@ import { MCPConfigService } from "@/node/services/mcpConfigService"; import { LocalRuntime } from "@/node/runtime/LocalRuntime"; import { 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 +281,15 @@ describe("AgentPluginInstallService", () => { pluginInvalidation: { keyPrefix: "plugin:", readToken: () => readMutationEpochToken(stagingDir()), + readComponentPolicy: () => readPluginMcpPolicy(registryFile()), 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 +335,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 +350,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 +366,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 +475,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 +536,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 +546,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"], }); @@ -612,12 +650,128 @@ 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("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("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 () => { + // This read acquires the same lock; it must see the saved policy, not deadlock. + 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 +781,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 +878,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 +938,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 +971,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 +990,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 +1026,43 @@ 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([]); }); - 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 99f90eeaee9..394d3ee3b73 100644 --- a/src/node/services/agentPlugins/installService.ts +++ b/src/node/services/agentPlugins/installService.ts @@ -1774,8 +1774,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() { @@ -2722,15 +2723,22 @@ export class AgentPluginInstallService { }); } - 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 = 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 +2746,26 @@ 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 entry; await this.writeRegistry( envelope, rawEntries.map((raw) => { @@ -2769,10 +2780,18 @@ 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 }; }); + // 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/agentSkills/builtInSkillContent.generated.ts b/src/node/services/agentSkills/builtInSkillContent.generated.ts index ba18e328df5..863706b06aa 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/tests/ipc/agentPlugins.test.ts b/tests/ipc/agentPlugins.test.ts index 87d3c62c710..5dadb347af9 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 a867592bcab..649b5b06c64 100644 --- a/tests/ui/config/pluginImports.test.ts +++ b/tests/ui/config/pluginImports.test.ts @@ -89,7 +89,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 +121,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 +159,120 @@ 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 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(["button", "keyboard", "palette"] as const)( "reopening installation via %s clears prior success through the next failed attempt", async (entryPoint) => { @@ -221,7 +323,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 +335,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 +350,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 +370,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 +383,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 +398,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); From 73961248bcf1222ef36afe29a232aa1571b45556 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 11:10:45 +0000 Subject: [PATCH 03/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20preserve=20componen?= =?UTF-8?q?t=20no-ops=20and=20retry=20failed=20MCP=20cleanup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Skip runtime reconciliation when component selection is effectively unchanged. Keep failed retired clients owned for retry, report cleanup failures from explicit saves, and leave automatic MCP boundaries non-disruptive. Validation: 589 tests passed (1 Windows-only skip), typecheck, touched ESLint, formatting, and diff checks passed. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$24.47`_ --- .../agentPlugins/installService.test.ts | 59 +++++++++++++++++ .../services/agentPlugins/installService.ts | 8 ++- src/node/services/mcpServerManager.test.ts | 63 +++++++++++++++++++ src/node/services/mcpServerManager.ts | 30 ++++++--- 4 files changed, 148 insertions(+), 12 deletions(-) diff --git a/src/node/services/agentPlugins/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index ee0e8acad81..8d1d2b84a8a 100644 --- a/src/node/services/agentPlugins/installService.test.ts +++ b/src/node/services/agentPlugins/installService.test.ts @@ -703,6 +703,65 @@ describe("AgentPluginInstallService", () => { ).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 () => { + // Reading takes the same mutation lock, proving it was released before cleanup. + 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"] }; diff --git a/src/node/services/agentPlugins/installService.ts b/src/node/services/agentPlugins/installService.ts index 394d3ee3b73..e78f7e03adf 100644 --- a/src/node/services/agentPlugins/installService.ts +++ b/src/node/services/agentPlugins/installService.ts @@ -2738,7 +2738,7 @@ export class AgentPluginInstallService { [...new Set(selection.skills)].sort(), [...new Set(selection.mcpServers)].sort(), ]); - const data = await this.runExclusive(async () => { + 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 @@ -2765,7 +2765,8 @@ export class AgentPluginInstallService { skills: inventory.skills.map((skill) => skill.name), mcpServers: inventory.mcpServers.map((server) => server.serverName), }; - if (selectionKey(importedComponents) === selectionKey(previous)) return entry; + if (selectionKey(importedComponents) === selectionKey(previous)) + return { data: entry, changed: false }; await this.writeRegistry( envelope, rawEntries.map((raw) => { @@ -2780,8 +2781,9 @@ export class AgentPluginInstallService { }; }) ); - 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 { diff --git a/src/node/services/mcpServerManager.test.ts b/src/node/services/mcpServerManager.test.ts index 95e30d46804..3e766dc675c 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -341,6 +341,69 @@ describe("MCPServerManager", () => { } ); + 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("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); diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index 26abcb6cac9..e1a00a054dc 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -1365,7 +1365,7 @@ export class MCPServerManager { /** Call after the atomic selection write and after releasing the install lock. */ async reconcilePluginComponents(): Promise { - await this.retireCrossProcessPluginInstances(); + await this.retireCrossProcessPluginInstances(true); } private componentAllowed( @@ -1389,7 +1389,8 @@ export class MCPServerManager { return { registryPath: this.componentPolicy?.registryPath ?? "", imports: null }; } - private async refreshComponentPolicy(): Promise { + 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)) { @@ -1432,15 +1433,18 @@ export class MCPServerManager { // their retired client. Only active instances participate in retention. if (this.getLeaseCount(workspaceId) > 0) continue; const retired = entry.retiredPluginInstances; - delete 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; } /** @@ -1451,7 +1455,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; @@ -1463,8 +1467,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) await this.refreshComponentPolicy(); + if (invalidation.readComponentPolicy !== undefined) + cleanupError = await this.refreshComponentPolicy(); const [token, overridesEpoch] = await Promise.all([ invalidation.readToken(), invalidation.readOverridesEpoch?.(), @@ -1520,6 +1526,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; } @@ -1952,7 +1964,7 @@ export class MCPServerManager { if (current === 1) { this.workspaceLeases.delete(workspaceId); if (this.pluginInvalidation?.readComponentPolicy !== undefined) { - this.reconcilePluginComponents().catch((error: unknown) => { + this.retireCrossProcessPluginInstances().catch((error: unknown) => { log.warn("Failed to reconcile plugin components after lease release", { workspaceId, error, @@ -2100,7 +2112,7 @@ export class MCPServerManager { agentPlugins?: AgentPluginsMcpContext | null ): Promise { if (this.pluginInvalidation?.readComponentPolicy !== undefined) - await this.reconcilePluginComponents(); + await this.retireCrossProcessPluginInstances(); const allServers = await this.getAllServers(projectPath, trusted, agentPlugins); const enabled = this.applyServerOverrides(allServers, overrides); return this.filterServersByPolicy(enabled); @@ -4174,7 +4186,7 @@ export class MCPServerManager { const removedComponents: string[] = []; for (;;) { if (this.pluginInvalidation?.readComponentPolicy !== undefined) - await this.reconcilePluginComponents(); + await this.retireCrossProcessPluginInstances(); for (const [name, instance] of instances) { if (this.componentAllowed(name)) continue; instances.delete(name); @@ -4188,7 +4200,7 @@ export class MCPServerManager { log.warn("Failed to close removed plugin startup", { name, error }); } } - if (removedComponents.length > 0) await this.reconcilePluginComponents(); + if (removedComponents.length > 0) await this.retireCrossProcessPluginInstances(); const clockBeforeScan = this.prefixInvalidationClock; invalidatedKeys.push( ...(await this.closeInvalidatedInstances(instances, startedAtEpoch, workspaceId)) From 6deceb6213b95318327f71afe53ddd76eee932bb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 11:55:13 +0000 Subject: [PATCH 04/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20refresh=20plugin=20?= =?UTF-8?q?card=20counts=20after=20component=20conflicts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keep card counts in sync with the refreshed editor after a stale selection is rejected. Cover both skill and MCP counts with the real-IPC UI regression. Validation: 716 unit tests (1 platform skip), 10 IPC/UI tests, 6 warmed Storybook interactions, static-check, and static-check-full passed. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$76.33`_ --- .../features/Settings/Sections/PluginsSettingsSection.tsx | 2 ++ tests/ui/config/pluginImports.test.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx index bcec8aaa9dd..51a1f4e44bd 100644 --- a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx +++ b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx @@ -297,6 +297,8 @@ const ManageComponentsPanel: React.FC<{ 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( diff --git a/tests/ui/config/pluginImports.test.ts b/tests/ui/config/pluginImports.test.ts index 649b5b06c64..d5ae0d177f9 100644 --- a/tests/ui/config/pluginImports.test.ts +++ b/tests/ui/config/pluginImports.test.ts @@ -236,6 +236,8 @@ describeIntegration("Selective plugin imports", () => { ); 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") From dc497b55e26ac2099ac4f6273ea300a1aff45e13 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 12:34:05 +0000 Subject: [PATCH 05/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20invalidate=20plugin?= =?UTF-8?q?=20consumers=20after=20uncertain=20saves?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh availability consumers and outer plugin counts when the selection write loses its response and the confirming inventory read also fails. Retain the uncertain status and pending draft, and do not invalidate definite failed saves. Add real-IPC full-App regressions for persisted removals and definite failures, including availability, counts, reopen behavior and no automatic resubmission. Validation: 11 plugin UI tests passed; make typecheck, scoped ESLint, Prettier and git diff --check passed. Full integration/static gates remain parent-owned. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$24.35`_ --- .../Sections/PluginsSettingsSection.tsx | 5 ++ tests/ui/config/pluginImports.test.ts | 83 +++++++++++++++++++ 2 files changed, 88 insertions(+) diff --git a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx index 51a1f4e44bd..86073c7dff8 100644 --- a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx +++ b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx @@ -306,6 +306,11 @@ const ManageComponentsPanel: React.FC<{ ? `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 (responseLost) { + // The write may have committed even when both responses are unavailable. + publishAgentPluginsMutated(); + await props.onSaved(); + } } finally { setBusy(false); } diff --git a/tests/ui/config/pluginImports.test.ts b/tests/ui/config/pluginImports.test.ts index d5ae0d177f9..e915f25b165 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"; @@ -275,6 +276,88 @@ describeIntegration("Selective plugin imports", () => { 120000 ); + test.each(["lost response", "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); + 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: "Cancel" }).hasAttribute("disabled")).toBe( + false + ) + ); + await consumerRefresh; + expect(available.some((skill) => skill.name === "review")).toBe( + failure === "rejected save" + ); + expect(refreshCount).toBe(failure === "lost response" ? 1 : 0); + expect( + canvas.getByText( + failure === "lost response" ? /0 of 2 skills imported/ : /1 of 2 skills imported/ + ) + ).toBeDefined(); + expect(canvas.getByRole("alert").textContent).toMatch(/could not confirm/i); + 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 === "lost response" ? /0 of 2 skills imported/ : /1 of 2 skills imported/ + ) + ).toBeDefined(); + } finally { + unsubscribe(); + await consumerRefresh; + } + }, + 120000 + ); + test.each(["button", "keyboard", "palette"] as const)( "reopening installation via %s clears prior success through the next failed attempt", async (entryPoint) => { From 8c8ed41a06f19ac695c3ec1f88e8a1a5da276e4e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 12:39:30 +0000 Subject: [PATCH 06/12] =?UTF-8?q?=F0=9F=A4=96=20tests:=20distinguish=20plu?= =?UTF-8?q?gin=20updates=20from=20explicit=20component=20replacement?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the installed-plugin regression to prove that updates preserve raw selections for absent components, unavailable desired names remain invalid, and explicit management replaces those selections. A later update restoring the component must not silently re-import it. Document the Clear/empty-consent rationale beside effectiveImports without changing selection semantics. Validation: 134 installer tests and 11 full-App plugin UI tests passed; make typecheck, scoped ESLint, Prettier and git diff --check passed. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$36.19`_ --- .../Sections/PluginsSettingsSection.tsx | 1 + .../agentPlugins/installService.test.ts | 39 ++++++++++++++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx index 86073c7dff8..46909b3c992 100644 --- a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx +++ b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx @@ -161,6 +161,7 @@ const ComponentChooser: 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) diff --git a/src/node/services/agentPlugins/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index 8d1d2b84a8a..64e06601046 100644 --- a/src/node/services/agentPlugins/installService.test.ts +++ b/src/node/services/agentPlugins/installService.test.ts @@ -1094,7 +1094,44 @@ describe("AgentPluginInstallService", () => { }) ).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("unchanged legacy selection remains import-all; changing it becomes explicit", async () => { From a563307c5a04359be8af964a469c69ee8ded4561 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 12:37:07 +0000 Subject: [PATCH 07/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20close=20MCP=20compo?= =?UTF-8?q?nent=20cleanup=20and=20test=20admission=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop retired leased clients during matching plugin tree invalidations without adding deselected restart candidates. Use existing idle ticks to retry failed retirement before evicting its ownership. Recheck managed component policy after named-test preparation and before admitting the connection test. Validation: manager and plugin/config/DI sibling suites, repository typecheck, touched ESLint, formatting, and diff checks passed. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$37.78`_ --- src/node/services/mcpServerManager.test.ts | 150 +++++++++++++++++++++ src/node/services/mcpServerManager.ts | 65 ++++++--- 2 files changed, 199 insertions(+), 16 deletions(-) diff --git a/src/node/services/mcpServerManager.test.ts b/src/node/services/mcpServerManager.test.ts index 3e766dc675c..519fb5fb1d0 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -27,6 +27,7 @@ import { MCPConfigService } from "./mcpConfigService"; import { Config } from "@/node/config"; 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"; @@ -404,6 +405,155 @@ describe("MCPServerManager", () => { } ); + 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("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); diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index e1a00a054dc..6ca17333ede 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -1987,8 +1987,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) { @@ -1997,6 +1998,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), @@ -2004,6 +2011,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( @@ -4074,6 +4086,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)) { @@ -4372,13 +4396,26 @@ 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. + const plugin = this.managedPluginServers.get(trimmedName) ?? server.plugin; + if ( + plugin?.componentPolicy !== undefined && + !this.componentAllowed(trimmedName, server, await this.readComponentPolicy()) + ) { + return { + success: false, + error: `MCP server '${trimmedName}' is disabled by component policy`, + }; + } + return runServerTest(launch, projectPath, `server "${trimmedName}"`); + }; if (server.transport === "stdio") { const launch = await prepareStdioLaunch(server); - return runServerTest( - { transport: "stdio", ...launch }, - projectPath, - `server "${trimmedName}"` - ); + return testNamedServer({ transport: "stdio", ...launch }); } try { @@ -4389,16 +4426,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 }; From f5cc72a292ad2f485b9cf1340487002ed04ff781 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 13:06:10 +0000 Subject: [PATCH 08/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20fence=20MCP=20compo?= =?UTF-8?q?nent=20admission=20with=20the=20plugin=20writer=20lock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hold the shared plugin mutation lock from a fresh policy read through synchronous tool, prompt, and named-test dispatch. Try it under the override fence so a plugin writer can still prune overrides; deny managed admission when the fence is missing or contended. Cover real old-inode reads against component setters, override-prune progress, and exactly-once cleanup after read failure, timeout, abort, and late lock acquisition. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$73.96`_ --- .../agentPlugins/installService.test.ts | 155 +++++++- .../services/agentPlugins/installService.ts | 23 +- src/node/services/agentPlugins/journals.ts | 20 + src/node/services/di/layers/core.ts | 8 +- src/node/services/mcpServerManager.test.ts | 353 +++++++++++++++++- src/node/services/mcpServerManager.ts | 150 +++++--- 6 files changed, 626 insertions(+), 83 deletions(-) diff --git a/src/node/services/agentPlugins/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index 64e06601046..10463f9f08c 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,8 +7,11 @@ 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"; @@ -282,6 +285,8 @@ describe("AgentPluginInstallService", () => { 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, }, @@ -650,6 +655,152 @@ describe("AgentPluginInstallService", () => { expect(await pathExists(path.join(pluginsDir(), "demo-plugin"))).toBe(false); }); + 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"] }; diff --git a/src/node/services/agentPlugins/installService.ts b/src/node/services/agentPlugins/installService.ts index e78f7e03adf..27e80db7791 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(); diff --git a/src/node/services/agentPlugins/journals.ts b/src/node/services/agentPlugins/journals.ts index 948f1fa48f2..40813534fd4 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/di/layers/core.ts b/src/node/services/di/layers/core.ts index b182813eac9..561e6b2a7d3 100644 --- a/src/node/services/di/layers/core.ts +++ b/src/node/services/di/layers/core.ts @@ -10,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, @@ -379,6 +383,8 @@ export const MCPServerManagerLive = Layer.effect( 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 519fb5fb1d0..151de164b9a 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -9,7 +9,10 @@ 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"; @@ -22,9 +25,11 @@ 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"; @@ -239,19 +244,25 @@ describe("MCPServerManager", () => { await fs.mkdir(path.join(home, "plugins"), { recursive: true }); const registryPath = path.join(home, "plugins.json"); const write = async (names: string[]) => { - await fs.writeFile( - `${registryPath}.tmp`, - JSON.stringify({ - plugins: [createTestPluginInstallEntry("demo", { skills: [], mcpServers: names })], - }) - ); - await fs.rename(`${registryPath}.tmp`, registryPath); + 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, @@ -264,12 +275,15 @@ describe("MCPServerManager", () => { 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: { - keyPrefix: "plugin:", - readToken: () => Promise.resolve(undefined), - readComponentPolicy: read, - }, + pluginInvalidation: invalidation, }); const internals = instance as unknown as MCPServerManagerTestAccess; const started: Array> = []; @@ -286,7 +300,7 @@ describe("MCPServerManager", () => { started.push(client); return Promise.resolve(client); }); - return { instance, internals, started }; + return { instance, internals, started, invalidation }; }; manager.dispose(); const local = makeManager(); @@ -699,6 +713,317 @@ describe("MCPServerManager", () => { ).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); diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index 6ca17333ede..8de911d69c6 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -1198,6 +1198,11 @@ export interface MCPServerManagerOptions { 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 @@ -1389,6 +1394,70 @@ export class MCPServerManager { 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 }> { + // 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 { pending: dispatch() }; + 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. + // Hold the writer lock BEFORE opening policy, through synchronous dispatch. + checkActive(); + const policy = await bounded(read()); + checkActive(); + if (!this.componentAllowed(name, info, policy)) + throw new Error(`MCP server '${name}' is disabled by component policy`); + const pending = dispatch(); + // Dispatch may reject before the filesystem releases finish. Observe it now, + // but return the original result only after both admission locks are released. + Promise.resolve(pending).catch(() => undefined); + return { pending }; + } finally { + await release(); + } + } + private async refreshComponentPolicy(): Promise { let cleanupError: Error | undefined; const previous = this.componentPolicy; @@ -3954,13 +4023,12 @@ export class MCPServerManager { const readOverridesEpoch = this.pluginInvalidation?.readOverridesEpoch; let pending: ReturnType | "retry"; if (acquireOverridesLock === undefined || readOverridesEpoch === undefined) { - const policy = - this.pluginInvalidation?.readComponentPolicy !== undefined - ? await this.readComponentPolicy() - : undefined; - if (!this.componentAllowed(serverName, undefined, policy)) - throw new Error(`MCP server '${serverName}' is disabled`); - 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 @@ -3990,13 +4058,10 @@ export class MCPServerManager { ) { return { epochMoved: true } as const; } - const policy = - this.pluginInvalidation?.readComponentPolicy !== undefined - ? await this.readComponentPolicy() - : undefined; - if (!this.componentAllowed(serverName, undefined, policy)) - throw new Error(`MCP server '${serverName}' is disabled`); - pending = dispatch(); + ({ pending } = await this.withComponentPolicyFence(serverName, undefined, dispatch, { + signal: options?.signal, + timeoutMs: Math.max(0, fenceDeadlineAt - Date.now()), + })); } finally { await release(); } @@ -4401,17 +4466,14 @@ export class MCPServerManager { ): Promise => { // Admit the named test after disk/OAuth preparation, before its connection // deadline starts. Ad-hoc drafts never carry managed plugin provenance. - const plugin = this.managedPluginServers.get(trimmedName) ?? server.plugin; - if ( - plugin?.componentPolicy !== undefined && - !this.componentAllowed(trimmedName, server, await this.readComponentPolicy()) - ) { - return { - success: false, - error: `MCP server '${trimmedName}' is disabled by component policy`, - }; + try { + const { pending } = await this.withComponentPolicyFence(trimmedName, server, () => + runServerTest(launch, projectPath, `server "${trimmedName}"`) + ); + return await pending; + } catch (error) { + return { success: false, error: getErrorMessage(error) }; } - return runServerTest(launch, projectPath, `server "${trimmedName}"`); }; if (server.transport === "stdio") { const launch = await prepareStdioLaunch(server); @@ -4889,22 +4951,23 @@ export class MCPServerManager { } return "dispatch"; }; - const componentPolicy = - this.pluginInvalidation?.readComponentPolicy !== undefined - ? await bounded(this.readComponentPolicy()) - : undefined; - if (!this.componentAllowed(serverName, servedInfo, componentPolicy)) - throw revoked(`server '${serverName}'`); 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 @@ -4934,7 +4997,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 ( @@ -4946,21 +5009,14 @@ export class MCPServerManager { // iteration's preflight evicts and re-derives from disk. continue; } - const componentPolicy = - this.pluginInvalidation?.readComponentPolicy !== undefined - ? await bounded(this.readComponentPolicy()) - : undefined; - if (!this.componentAllowed(serverName, servedInfo, componentPolicy)) - throw revoked(`server '${serverName}'`); - // 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( From 27678a0746e25be5bc09863e888e88d0cd5e3530 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 15:02:08 +0000 Subject: [PATCH 09/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20recheck=20plugin=20?= =?UTF-8?q?components=20at=20every=20MCP=20cold-start=20boundary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse the component writer try-lock after the override fence so queued stdio and remote startups cannot launch a removed managed component. Retain the bounded stdio abort and remote initiation lock lifetimes. Validation: 674 tests passed, one Windows-only skip; typecheck, targeted ESLint, Prettier and diff checks passed. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh`_ --- src/node/services/mcpServerManager.test.ts | 240 +++++++++++++++++---- src/node/services/mcpServerManager.ts | 117 ++++++---- 2 files changed, 278 insertions(+), 79 deletions(-) diff --git a/src/node/services/mcpServerManager.test.ts b/src/node/services/mcpServerManager.test.ts index 151de164b9a..356b639ce17 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -568,6 +568,148 @@ describe("MCPServerManager", () => { } ); + 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); @@ -2401,28 +2543,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 } @@ -2431,8 +2578,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, @@ -2440,7 +2589,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"); }); @@ -2448,28 +2599,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 } } @@ -2478,9 +2634,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")), { @@ -2496,15 +2654,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 8de911d69c6..0efbf67be08 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -1400,10 +1400,26 @@ export class MCPServerManager { 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 { pending: dispatch() }; + if (plugin?.componentPolicy === undefined) return () => Promise.resolve(); const read = this.pluginInvalidation?.readComponentPolicy; const acquire = this.pluginInvalidation?.tryAcquireComponentPolicyLock; const unavailable = () => @@ -1442,19 +1458,16 @@ export class MCPServerManager { } try { // Atomic rename alone is insufficient: readFile may still own the old inode. - // Hold the writer lock BEFORE opening policy, through synchronous dispatch. + // 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`); - const pending = dispatch(); - // Dispatch may reject before the filesystem releases finish. Observe it now, - // but return the original result only after both admission locks are released. - Promise.resolve(pending).catch(() => undefined); - return { pending }; - } finally { + return release; + } catch (error) { await release(); + throw error; } } @@ -5403,10 +5416,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, @@ -5434,19 +5450,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) { @@ -5455,26 +5472,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(); @@ -5516,7 +5543,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; } @@ -5539,6 +5570,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, @@ -5824,6 +5857,8 @@ export class MCPServerManager { // client but cannot undo traffic or credentials already sent. const tryHttp = () => this.launchUnderOverrideFence( + name, + info, () => createMCPClient({ transport: { @@ -5839,6 +5874,8 @@ export class MCPServerManager { const trySse = () => this.launchUnderOverrideFence( + name, + info, () => createMCPClient({ transport: { From 1b802837f2bbabfd0dec14336a88f5d76b18c649 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 15:24:40 +0000 Subject: [PATCH 10/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20keep=20plugin=20inv?= =?UTF-8?q?entory=20reads=20off=20the=20MCP=20admission=20writer=20lock?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bracket component inventory with the existing discovery journal/epoch gate instead of holding the plugin writer lock across full-tree hashes. Reject installer transactions overlapping the scan, including complete reinstall with an unchanged final content receipt. Keep component updates under the existing SHA, content-hash and selection compare-and-swap lock. Validation: 678 tests passed, one Windows-only skip; final make static-check passed with one ESLint worker. Regression tests reproduce both contention and concurrent-installer receipt invalidation. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$103.63`_ --- .../agentPlugins/installService.test.ts | 127 +++++++++++++++++- .../services/agentPlugins/installService.ts | 19 ++- 2 files changed, 139 insertions(+), 7 deletions(-) diff --git a/src/node/services/agentPlugins/installService.test.ts b/src/node/services/agentPlugins/installService.test.ts index 10463f9f08c..e82c81cb591 100644 --- a/src/node/services/agentPlugins/installService.test.ts +++ b/src/node/services/agentPlugins/installService.test.ts @@ -573,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({ @@ -873,7 +992,9 @@ describe("AgentPluginInstallService", () => { const before = await fsPromises.readFile(registryFile(), "utf8"); const empty = { skills: [], mcpServers: [] }; const reconcile = spyOn(manager, "reconcilePluginComponents").mockImplementation(async () => { - // Reading takes the same mutation lock, proving it was released before cleanup. + // 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); @@ -937,7 +1058,9 @@ describe("AgentPluginInstallService", () => { }; const reconcile = spyOn(manager, "reconcilePluginComponents").mockImplementationOnce( async () => { - // This read acquires the same lock; it must see the saved policy, not deadlock. + // 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); diff --git a/src/node/services/agentPlugins/installService.ts b/src/node/services/agentPlugins/installService.ts index 27e80db7791..5c250940cdb 100644 --- a/src/node/services/agentPlugins/installService.ts +++ b/src/node/services/agentPlugins/installService.ts @@ -2701,11 +2701,20 @@ 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 setComponents(args: { From 6680946720aeb752a51777f31c8da95d126d458b Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 14:45:26 +0000 Subject: [PATCH 11/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20reconcile=20acknowl?= =?UTF-8?q?edged=20plugin=20component=20saves?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refresh outer plugin counts when an acknowledged selection write cannot be confirmed by the inventory read. Confirm the current tree and raw persisted selection before showing success; if another writer supersedes the save, show its current selection and counts without automatically resubmitting. Add deterministic full-App regressions for failed confirmation and subsequent writers, including restoration of the original baseline and explicit retry. Validation: 15 plugin UI/IPC tests passed; make typecheck, scoped ESLint, Prettier and git diff --check passed. Both defects were reproduced test-first. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$50.51`_ --- .../Sections/PluginsSettingsSection.tsx | 16 ++-- tests/ui/config/pluginImports.test.ts | 91 +++++++++++++++++-- 2 files changed, 93 insertions(+), 14 deletions(-) diff --git a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx index 46909b3c992..8904d03e682 100644 --- a/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx +++ b/src/browser/features/Settings/Sections/PluginsSettingsSection.tsx @@ -250,6 +250,7 @@ const ManageComponentsPanel: React.FC<{ setSaved(false); let confirmed = false; let responseLost = false; + let savedImports: AgentPluginImportedComponents | null = selected; let warning: string | undefined; try { const result = await api.agentPlugins @@ -267,7 +268,7 @@ const ManageComponentsPanel: React.FC<{ if (!result.success) throw new Error(result.error); confirmed = true; warning = result.cleanupWarning; - setSaved(true); + savedImports = result.data.importedComponents ?? null; publishAgentPluginsMutated(); } catch (err) { setError(getErrorMessage(err)); @@ -280,8 +281,9 @@ const ManageComponentsPanel: React.FC<{ current.data.lockedSha === inventory.lockedSha && current.data.contentHash === inventory.contentHash; if ( - confirmed || - (responseLost && sameTree && sameImports(current.data.importedComponents ?? null, selected)) + (confirmed || responseLost) && + sameTree && + sameImports(current.data.importedComponents ?? null, savedImports) ) { setInventory(current.data); setSelected(effectiveImports(current.data)); @@ -290,9 +292,11 @@ const ManageComponentsPanel: React.FC<{ 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( @@ -307,9 +311,9 @@ const ManageComponentsPanel: React.FC<{ ? `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 (responseLost) { - // The write may have committed even when both responses are unavailable. - publishAgentPluginsMutated(); + if (confirmed || responseLost) { + // Refresh counts for acknowledged or possible writes even without an inventory receipt. + if (!confirmed) publishAgentPluginsMutated(); await props.onSaved(); } } finally { diff --git a/tests/ui/config/pluginImports.test.ts b/tests/ui/config/pluginImports.test.ts index e915f25b165..81c972ab070 100644 --- a/tests/ui/config/pluginImports.test.ts +++ b/tests/ui/config/pluginImports.test.ts @@ -276,7 +276,7 @@ describeIntegration("Selective plugin imports", () => { 120000 ); - test.each(["lost response", "rejected save"] as const)( + 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); @@ -307,6 +307,7 @@ describeIntegration("Selective plugin imports", () => { 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 @@ -315,21 +316,21 @@ describeIntegration("Selective plugin imports", () => { try { await user.click(canvas.getByRole("button", { name: "Save changes" })); await waitFor(() => - expect(canvas.getByRole("button", { name: "Cancel" }).hasAttribute("disabled")).toBe( - false - ) + 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 === "lost response" ? 1 : 0); + expect(refreshCount).toBe(failure === "rejected save" ? 0 : 1); expect( canvas.getByText( - failure === "lost response" ? /0 of 2 skills imported/ : /1 of 2 skills imported/ + failure === "rejected save" ? /1 of 2 skills imported/ : /0 of 2 skills imported/ ) ).toBeDefined(); - expect(canvas.getByRole("alert").textContent).toMatch(/could not confirm/i); + expect(canvas.getByRole("alert")).toBeDefined(); expect(canvas.queryByRole("button", { name: "Done" })).toBeNull(); expect(canvas.getByRole("checkbox", { name: "review" }).getAttribute("aria-checked")).toBe( "false" @@ -347,7 +348,7 @@ describeIntegration("Selective plugin imports", () => { ).toBe(failure === "rejected save" ? "true" : "false"); expect( canvas.getByText( - failure === "lost response" ? /0 of 2 skills imported/ : /1 of 2 skills imported/ + failure === "rejected save" ? /1 of 2 skills imported/ : /0 of 2 skills imported/ ) ).toBeDefined(); } finally { @@ -358,6 +359,80 @@ describeIntegration("Selective plugin imports", () => { 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) => { From f2609e183f50f3a11af1215ec4342f9b45a74667 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 16:29:16 +0000 Subject: [PATCH 12/12] =?UTF-8?q?=F0=9F=A4=96=20fix:=20retain=20failed=20c?= =?UTF-8?q?omponent=20startup=20closes=20for=20cleanup=20retries?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Transfer deselected startup clients whose close fails into the published workspace entry's existing retired-client set. Cover initial, additive, timed-out retry and leased restart publication without exposing retired clients or reusing them when a component is re-added. Validation: six deterministic regressions failed before the fix; 684 manager/sibling tests passed with one Windows-only skip. Sequential make typecheck and make static-check passed. --- _Generated with `xum` • Model: `coder:openai/gpt-6-astra` • Thinking: `xhigh` • Cost: `$119.29`_ --- src/node/services/mcpServerManager.test.ts | 118 +++++++++++++++++++++ src/node/services/mcpServerManager.ts | 24 +++-- 2 files changed, 136 insertions(+), 6 deletions(-) diff --git a/src/node/services/mcpServerManager.test.ts b/src/node/services/mcpServerManager.test.ts index 356b639ce17..491cba6da0e 100644 --- a/src/node/services/mcpServerManager.test.ts +++ b/src/node/services/mcpServerManager.test.ts @@ -1297,6 +1297,124 @@ describe("MCPServerManager", () => { } ); + 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) => { diff --git a/src/node/services/mcpServerManager.ts b/src/node/services/mcpServerManager.ts index 0efbf67be08..53ddc2a7491 100644 --- a/src/node/services/mcpServerManager.ts +++ b/src/node/services/mcpServerManager.ts @@ -2788,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 @@ -2799,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); } @@ -2973,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. @@ -2982,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); } @@ -3230,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 @@ -3241,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; @@ -3259,6 +3265,7 @@ export class MCPServerManager { entry = { configSignature: signature, instances, + ...(failedRetirements.size > 0 ? { retiredPluginInstances: failedRetirements } : {}), enabledServerNames, enabledServers, enabledServersGeneration: configGenerationUsed, @@ -4276,16 +4283,19 @@ 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(); @@ -4299,6 +4309,7 @@ export class MCPServerManager { try { await instance.close(); } catch (error) { + failedRetirements.add(instance); log.warn("Failed to close removed plugin startup", { name, error }); } } @@ -4311,7 +4322,8 @@ export class MCPServerManager { // calls, which are finite user-driven plugin update/uninstall events. if (this.prefixInvalidationClock === clockBeforeScan) { publish( - [...invalidatedKeys, ...removedComponents].filter((name) => this.componentAllowed(name)) + [...invalidatedKeys, ...removedComponents].filter((name) => this.componentAllowed(name)), + failedRetirements ); return; }