From c56521b4a43d03300ef5dc29ff8fbf0fce18d820 Mon Sep 17 00:00:00 2001 From: Renzo Beux Date: Fri, 28 Aug 2026 13:20:08 -0300 Subject: [PATCH] feat(server): process-level plugin MCP tool source Sibling of plugin-event-sink.ts: the plugins layer registers a source, and src/mcp/tools.ts reads it as a defaulted argument. mcp-transport/ is not touched at all, so it never learns that plugins exist and the tool-call audit row for a plugin tool comes out of code that did not change. Pull rather than push, unlike the event sink, because the answer depends on the session's ctx: there is nothing a push could carry ahead of time, and reading it per attach means activating or uninstalling a plugin takes effect on the next session with nothing to invalidate. Same argument channel-registry.ts makes for setPluginChannelSource. An unset source means "no plugin tools" rather than an error, the right answer for any app built without the plugins runtime. MCPSINK-003 pins that case, which none of the sibling sinks covers today. Not try/caught here: attach() already contains a throwing source and logs which one failed, and catching again would swallow that. The harness takes dynamicTools explicitly so suites exercise the path without installing a process-level source. MCPSINK-001 to MCPSINK-005. --- server/src/mcp/tools.ts | 10 ++- server/src/plugin-mcp-tools.ts | 36 +++++++++ server/tests/helpers/mcp-harness.ts | 12 ++- .../tests/unit/plugins/mcp-tool-sink.test.ts | 77 +++++++++++++++++++ 4 files changed, 131 insertions(+), 4 deletions(-) create mode 100644 server/src/plugin-mcp-tools.ts create mode 100644 server/tests/unit/plugins/mcp-tool-sink.test.ts diff --git a/server/src/mcp/tools.ts b/server/src/mcp/tools.ts index 0fc5ce0fd..73b008848 100644 --- a/server/src/mcp/tools.ts +++ b/server/src/mcp/tools.ts @@ -1,5 +1,6 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; import type { McpAttachOptions, McpRegistry } from '../nest-mcp'; +import { getPluginMcpToolSource } from '../plugin-mcp-tools'; /** * Attaches the MCP surface to a session's server. @@ -11,7 +12,12 @@ import type { McpAttachOptions, McpRegistry } from '../nest-mcp'; * (injected into the transport service); the no-Nest harness hands in * createTestRegistry()'s build. A null registry (direct callers without either) * skips the attach. + * + * `dynamicTools` defaults to the process-level plugin source rather than being + * passed down from the transport, so mcp-transport/ never learns that plugins + * exist and an app built without the plugins runtime resolves it to undefined. + * Tests pass it explicitly instead of reaching for the global. */ -export function registerTools(registry: McpRegistry | null, server: McpServer, userId: number, scopes: string[] | null, isStaticToken = false, getDeprecationNotice: () => string | null = () => null, onInvoke?: McpAttachOptions['onInvoke']): void { - if (registry) registry.attach(server, { userId, scopes, isStaticToken, getDeprecationNotice }, { onInvoke }); +export function registerTools(registry: McpRegistry | null, server: McpServer, userId: number, scopes: string[] | null, isStaticToken = false, getDeprecationNotice: () => string | null = () => null, onInvoke?: McpAttachOptions['onInvoke'], dynamicTools: McpAttachOptions['dynamicTools'] = getPluginMcpToolSource() ?? undefined): void { + if (registry) registry.attach(server, { userId, scopes, isStaticToken, getDeprecationNotice }, { onInvoke, dynamicTools }); } diff --git a/server/src/plugin-mcp-tools.ts b/server/src/plugin-mcp-tools.ts new file mode 100644 index 000000000..df3fb4bc3 --- /dev/null +++ b/server/src/plugin-mcp-tools.ts @@ -0,0 +1,36 @@ +// The seam that lets a per-session MCP server carry tools contributed by installed +// plugins, without src/nest/mcp-transport/ gaining an edge to the plugin runtime — +// the same module cycle trip-warnings.mcp.ts documents. Sibling of +// plugin-event-sink.ts and plugin-user-lifecycle.ts; the plugins layer registers +// the source at bootstrap and clears it on shutdown. + +import type { McpContext, McpDynamicTool } from './nest-mcp'; + +/** + * PULL rather than push, unlike setPluginEventSink: the answer depends on the + * session's ctx, so there is nothing a push could carry ahead of time, and + * reading it on every attach means activating or uninstalling a plugin takes + * effect on the next session with nothing to invalidate. That is the argument + * channel-registry.ts already makes for setPluginChannelSource. + * + * Unlike every sink beside it this one returns a value on the request path. An + * unset source is not an error: it means "no plugin tools", which is the right + * answer for any app built without the plugins runtime, and for every test that + * never registers one. + * + * Deliberately NOT wrapped in try/catch here. nest-mcp's attach() contains a + * throwing source per its own contract and logs which one failed; catching it + * again on the way out would swallow that diagnostic and report the same + * degraded surface less usefully. + */ +export type PluginMcpToolSource = (ctx: McpContext) => readonly McpDynamicTool[]; + +let source: PluginMcpToolSource | null = null; + +export function setPluginMcpToolSource(fn: PluginMcpToolSource | null): void { + source = fn; +} + +export function getPluginMcpToolSource(): PluginMcpToolSource | null { + return source; +} diff --git a/server/tests/helpers/mcp-harness.ts b/server/tests/helpers/mcp-harness.ts index 8bc6a4ac8..22388b706 100644 --- a/server/tests/helpers/mcp-harness.ts +++ b/server/tests/helpers/mcp-harness.ts @@ -14,6 +14,7 @@ import { McpServer } from '@modelcontextprotocol/sdk/server/mcp'; import { Client } from '@modelcontextprotocol/sdk/client/index'; import { InMemoryTransport } from '@modelcontextprotocol/sdk/inMemory'; import { registerTools } from '../../src/mcp/tools'; +import type { McpAttachOptions } from '../../src/nest-mcp'; import { createMcpTestRegistry } from './mcp-test-controllers'; export interface McpHarness { @@ -39,10 +40,17 @@ export interface McpHarnessOptions { isStaticToken?: boolean; /** Fire-once deprecation-notice closure (default: registerTools' () => null) */ getDeprecationNotice?: () => string | null; + /** + * Host-contributed tools for this session. Passed explicitly so a suite never + * has to install the process-level plugin source to exercise the path; left + * undefined, registerTools resolves it to that source, which is unset in + * every suite that does not register one. + */ + dynamicTools?: McpAttachOptions['dynamicTools']; } export async function createMcpHarness(options: McpHarnessOptions): Promise { - const { userId, withTools = true, scopes = null, isStaticToken = false, getDeprecationNotice } = options; + const { userId, withTools = true, scopes = null, isStaticToken = false, getDeprecationNotice, dynamicTools } = options; const server = new McpServer({ name: 'trek-test', version: '1.0.0' }); @@ -51,7 +59,7 @@ export async function createMcpHarness(options: McpHarnessOptions): Promise ({ content: [{ type: 'text', text: 'dynamic' }] }), +}; + +/** Records what attach() was handed, without needing a real registry. */ +function spyRegistry() { + const attach = vi.fn(); + return { attach, registry: { attach } as unknown as Parameters[0] }; +} + +afterEach(() => { + setPluginMcpToolSource(null); +}); + +describe('setPluginMcpToolSource', () => { + it('MCPSINK-001: is unset by default', () => { + expect(getPluginMcpToolSource()).toBeNull(); + }); + + it('MCPSINK-002: round-trips a source and clears back to null', () => { + const source = () => [echo]; + setPluginMcpToolSource(source); + expect(getPluginMcpToolSource()).toBe(source); + + setPluginMcpToolSource(null); + expect(getPluginMcpToolSource()).toBeNull(); + }); +}); + +describe('registerTools wiring', () => { + it('MCPSINK-003: passes no dynamic source when none is registered', () => { + const { attach, registry } = spyRegistry(); + + registerTools(registry, new McpServer({ name: 't', version: '1' }), 1, null); + + expect(attach.mock.calls[0][2]).toEqual({ onInvoke: undefined, dynamicTools: undefined }); + }); + + it('MCPSINK-004: passes the registered source through to attach', () => { + const source = (_ctx: McpContext) => [echo]; + setPluginMcpToolSource(source); + const { attach, registry } = spyRegistry(); + + registerTools(registry, new McpServer({ name: 't', version: '1' }), 1, null); + + expect(attach.mock.calls[0][2].dynamicTools).toBe(source); + }); + + it('MCPSINK-005: an explicit argument wins over the process-level source', () => { + setPluginMcpToolSource(() => [echo]); + const explicit = () => []; + const { attach, registry } = spyRegistry(); + + registerTools(registry, new McpServer({ name: 't', version: '1' }), 1, null, false, undefined, undefined, explicit); + + expect(attach.mock.calls[0][2].dynamicTools).toBe(explicit); + }); +});