From 65a6a46d70c51ad88137802a1f7135e22a15c837 Mon Sep 17 00:00:00 2001 From: Renzo Beux Date: Fri, 28 Aug 2026 13:50:30 -0300 Subject: [PATCH] fix(plugins): invalidate MCP sessions on lifecycle and grant changes The advertised tool surface is frozen at initialize, so anything that changes which plugin tools exist has to end the sessions built before it. tools/list_changed is not an alternative: no event store is configured, and the surface would not be re-read anyway. Unconditional, no diffing. The surface is per-session, since it depends on the caller's scopes and the plugin's grants, so there is no ctx-free set to compare against. Five routes, mirroring what admin.controller.ts already does for addons: activate, deactivate, update, retrust and uninstall. activate(id, consent), update and retrust are also the grant-change paths, so "on an mcp:tools grant change" needs nothing extra. A failed call invalidates nothing, because nothing changed. The addon cascade needed its own call. Disabling an addon can stop a plugin that publishes tools, and the addon doing the cascading is not necessarily one of MCP_RELEVANT_ADDONS, so mcpAffected does not cover it. Still gated on a plugin actually stopping, which keeps the #1414 rule that a config-only save kills nothing. Imported from sessionManager rather than the src/mcp barrel, which evaluates readEnv().mcp at module scope. MCPINV-001 to MCPINV-007. --- server/src/nest/admin/admin.controller.ts | 7 +- server/src/nest/plugins/plugins.controller.ts | 12 ++- .../plugins/mcp-session-invalidation.test.ts | 87 +++++++++++++++++++ 3 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 server/tests/unit/plugins/mcp-session-invalidation.test.ts diff --git a/server/src/nest/admin/admin.controller.ts b/server/src/nest/admin/admin.controller.ts index 5bd368f80..4639766ba 100644 --- a/server/src/nest/admin/admin.controller.ts +++ b/server/src/nest/admin/admin.controller.ts @@ -260,7 +260,12 @@ export class AdminController { // plugin that transitively depends on them) so a plugin never runs against a // disabled addon (#plugins dependencies). if (result.addon && result.addon.enabled === false && (body as { enabled?: boolean })?.enabled === false) { - await this.pluginRuntime.deactivateForDisabledAddon(id); + const stopped = await this.pluginRuntime.deactivateForDisabledAddon(id); + // A stopped plugin takes its MCP tools off the surface, and the addon that + // cascaded here is not necessarily an MCP-relevant one, so mcpAffected + // above does not cover this. Still conditional on something actually + // stopping, which keeps the #1414 rule that a no-op save kills nothing. + if (stopped.length && !result.mcpAffected) this.admin.invalidateMcpSessions(); } return { addon: result.addon }; } diff --git a/server/src/nest/plugins/plugins.controller.ts b/server/src/nest/plugins/plugins.controller.ts index 189951bad..7067d5720 100644 --- a/server/src/nest/plugins/plugins.controller.ts +++ b/server/src/nest/plugins/plugins.controller.ts @@ -14,6 +14,10 @@ import { devLinkEnabled } from './dev-link'; import { PluginActivateDto, PluginConfigDto, PluginEgressHostsDto, PluginInstallDto, PluginLinkDto, PluginRetrustDto, PluginUninstallDto, PluginUpdateDto } from './plugins.dto'; import { ManagedForbidden, isManagedBlocked, MANAGED_FORBIDDEN_ERROR } from '../common/managed'; import { RuntimeEnvService } from '../app-config/runtime-env.service'; +// Straight from sessionManager, not the src/mcp barrel: that one evaluates +// readEnv().mcp at module scope and installs the sweep interval, which a domain +// module must not drag into every test that mocks app-config partially. +import { invalidateMcpSessions } from '../../mcp/sessionManager'; /** * Flatten a registry/install failure into the error envelope — CARRYING THE CODE. @@ -188,6 +192,7 @@ export class PluginsController { } throw new HttpException({ error: e instanceof Error ? e.message : 'activation failed' }, 400); } + invalidateMcpSessions(); return { status: this.runtime.isActive(id) ? 'active' : 'error' }; } @@ -197,6 +202,7 @@ export class PluginsController { // Cascade: disabling a plugin also disables everything that depends on it (a // dependent can't run without its dependency). The client refresh reflects it. await this.runtime.deactivateWithDependents(id); + invalidateMcpSessions(); return { status: 'inactive' }; } @@ -236,6 +242,7 @@ export class PluginsController { // (any path) releases a stale hold. Only after success — a failed update // changed nothing and must not touch the flag. await this.registry.recomputeUpdateHold(id, res.version, !!body?.version); + invalidateMcpSessions(); return res; } catch (e) { throw registryFailure(e, 'update failed'); @@ -274,7 +281,9 @@ export class PluginsController { if (!body?.version) throw new HttpException({ error: 'version is required' }, 400); if (!body?.publicKey) throw new HttpException({ error: 'publicKey is required' }, 400); try { - return await this.runtime.retrust(id, body.version, body.publicKey, { userId: user?.id ?? null, ip: getClientIp(req) }); + const res = await this.runtime.retrust(id, body.version, body.publicKey, { userId: user?.id ?? null, ip: getClientIp(req) }); + invalidateMcpSessions(); + return res; } catch (e) { throw registryFailure(e, 'retrust failed'); } @@ -284,6 +293,7 @@ export class PluginsController { @HttpCode(200) async uninstall(@Param('id') id: string, @Body() body: PluginUninstallDto) { await this.runtime.uninstall(id, !!body?.deleteData); + invalidateMcpSessions(); return { status: 'uninstalled' }; } diff --git a/server/tests/unit/plugins/mcp-session-invalidation.test.ts b/server/tests/unit/plugins/mcp-session-invalidation.test.ts new file mode 100644 index 000000000..486968803 --- /dev/null +++ b/server/tests/unit/plugins/mcp-session-invalidation.test.ts @@ -0,0 +1,87 @@ +/** + * MCP session invalidation on plugin lifecycle changes — MCPINV-001 to MCPINV-007. + * + * The advertised tool surface is frozen at initialize (no event store is + * configured, and tools/list_changed would not help), so anything that changes + * which plugin tools exist has to end the sessions that were built before it. + * + * Unconditional by design: the surface is per-session, because it depends on the + * caller's scopes and the plugin's grants, so there is no ctx-free set to diff. + */ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { PluginsController } from '../../../src/nest/plugins/plugins.controller'; +import type { RuntimeEnvService } from '../../../src/nest/app-config/runtime-env.service'; + +const invalidate = vi.fn(); +vi.mock('../../../src/mcp/sessionManager', () => ({ invalidateMcpSessions: () => invalidate() })); + +/* eslint-disable @typescript-eslint/no-explicit-any */ +const registry = { recomputeUpdateHold: vi.fn(async () => {}) } as any; +const ctrl = (runtime: any) => + new PluginsController({} as any, runtime, registry, { isManaged: () => false } as unknown as RuntimeEnvService); + +const runtime = () => + ({ + activate: vi.fn(async () => {}), + isActive: vi.fn(() => true), + deactivateWithDependents: vi.fn(async () => {}), + update: vi.fn(async () => ({ version: '1.1.0' })), + retrust: vi.fn(async () => ({ ok: true })), + uninstall: vi.fn(async () => {}), + }) as any; +/* eslint-enable @typescript-eslint/no-explicit-any */ + +beforeEach(() => { + invalidate.mockClear(); + process.env.TREK_PLUGINS_ENABLED = 'true'; +}); + +describe('plugin lifecycle invalidates MCP sessions', () => { + it('MCPINV-001: activate', async () => { + await ctrl(runtime()).activate('weather', { consent: true } as never); + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('MCPINV-002: deactivate', async () => { + await ctrl(runtime()).deactivate('weather'); + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('MCPINV-003: update', async () => { + // A version bump can change the declared tools, so it is a surface change + // even though the plugin stays active. + await ctrl(runtime()).update('weather', {} as never); + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('MCPINV-004: uninstall', async () => { + await ctrl(runtime()).uninstall('weather', {} as never); + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('MCPINV-005: retrust', async () => { + // Retrust installs a version, so it is an update by another name. + await ctrl(runtime()).retrust( + 'weather', + { version: '1.1.0', publicKey: 'k' } as never, + { id: 1 }, + { headers: {}, socket: {} } as never, + ); + expect(invalidate).toHaveBeenCalledTimes(1); + }); + + it('MCPINV-006: a failed activate does not invalidate', async () => { + // Nothing changed, so nobody's session should be dropped. + const r = runtime(); + r.activate = vi.fn(async () => { throw new Error('boom'); }); + await expect(ctrl(r).activate('weather', {} as never)).rejects.toThrow(); + expect(invalidate).not.toHaveBeenCalled(); + }); + + it('MCPINV-007: a failed update does not invalidate', async () => { + const r = runtime(); + r.update = vi.fn(async () => { throw new Error('boom'); }); + await expect(ctrl(r).update('weather', {} as never)).rejects.toThrow(); + expect(invalidate).not.toHaveBeenCalled(); + }); +});