diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f0c3fb..6dc66ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,14 @@ Releases before 0.7.0 are described in [GitHub Releases](https://github.com/code - `http.allowedOrigins` validates the `Origin` header on the `/mcp` endpoints, as the MCP spec requires. A request with an `Origin` outside the list gets `403`; requests without an `Origin` header are always allowed. When `cors` is on, only the listed origins get CORS access. Not set by default, so existing deployments behave as before. - **Migration:** none needed. To turn it on, pass `http: { allowedOrigins: ["https://app.example.com"] }`. +### Breaking + +- `ServerOrchestrator.isReady()` now returns `false`, and `ensureReady()` throws, when registering a startup toolset's tools fails (for example a duplicate tool name, within a toolset or across toolsets when `namespaceToolsWithSetKey` is off). It used to report ready. As a result `createMcpServer` rejects at startup for such a catalog in STATIC mode, where it used to start with the partial toolset. Fix the catalog (rename or remove the duplicate tool) before upgrading. `enableToolset` results (and the `enable_toolset` meta-tool output) gain an optional `code` on that failure. Toolsets refused before registration (exposure policy, duplicate names in `startup.toolsets`) and module loader errors do not count as failures. + +### Fixed + +- A STATIC client whose startup toolsets fail to load now gets a JSON-RPC error (`-32603`, HTTP 500) on `initialize` instead of connecting with only some of its tools. The failed bundle is dropped, so the client's next `initialize` retries the load. + ## [0.7.0] ### Breaking diff --git a/src/core/AGENTS.md b/src/core/AGENTS.md index f2d7d95..53e9bb0 100644 --- a/src/core/AGENTS.md +++ b/src/core/AGENTS.md @@ -19,6 +19,7 @@ Central orchestration layer. Manages toolset lifecycle, tool registration, and s 5. **Toolset added to activeToolsets AFTER registration** — All tools must register successfully before toolset is marked active. Partial failures leave the toolset inactive but orphaned tools remain registered (MCP limitation). 6. **`enableToolsets()` batches notifications** — Calls `enableToolset(name, skipNotification=true)` per toolset, sends one `tools/list_changed` notification at the end. 7. **JSON Schema `inputSchema` uses `registerTool` with a passthrough object** — The SDK rejects plain JSON Schema as a tool schema. Zod raw shapes and schemas are passed to `registerTool` as is. Clients see an open object schema for JSON Schema tools, not the declared one. +8. **Failed tool registration at startup makes the orchestrator not ready** — `enableToolsets()` reports failures in its result and never throws. A failure that carries a `code` (set when enabling threw, normally while registering the toolset's tools, e.g. a tool name collision) becomes `initError`, so `isReady()` is `false` and `ensureReady()` throws. Refusals made before any registration (invalid name, already enabled, exposure policy) carry no code and are ignored. Module loader errors are also ignored: `ModuleResolver` logs them and continues. ## Enable Toolset Flow diff --git a/src/core/DynamicToolManager.ts b/src/core/DynamicToolManager.ts index c1ed0d5..a4e826e 100644 --- a/src/core/DynamicToolManager.ts +++ b/src/core/DynamicToolManager.ts @@ -7,6 +7,7 @@ import type { ToolSetDefinition, ToolingErrorCode, } from "../types/index.js"; +import { ToolingError } from "../errors/ToolingError.js"; import { ModuleResolver } from "../mode/ModuleResolver.js"; import { ToolRegistry } from "./ToolRegistry.js"; import type { DynamicToolManagerOptions } from "./core.types.js"; @@ -77,12 +78,12 @@ export class DynamicToolManager { * Enables a single toolset by name. * @param toolsetName - The name of the toolset to enable * @param skipNotification - If true, skips the tool list change notification - * @returns Result object with success status and message + * @returns Result object with success status and message; `code` is set only when enabling threw, normally while registering the toolset's tools */ public async enableToolset( toolsetName: string, skipNotification = false - ): Promise<{ success: boolean; message: string }> { + ): Promise<{ success: boolean; message: string; code?: ToolingErrorCode }> { const validation = this.validateToolsetForEnable(toolsetName); if ("message" in validation) return validation; @@ -116,6 +117,8 @@ export class DynamicToolManager { message: `Failed to enable toolset '${sanitized}': ${ error instanceof Error ? error.message : "Unknown error" }`, + // Only this path sets a code; ServerOrchestrator treats it as "enabling threw" + code: error instanceof ToolingError ? error.code : "E_INTERNAL", }; } } diff --git a/src/core/ServerOrchestrator.ts b/src/core/ServerOrchestrator.ts index 9c6242c..d9aee63 100644 --- a/src/core/ServerOrchestrator.ts +++ b/src/core/ServerOrchestrator.ts @@ -83,10 +83,23 @@ export class ServerOrchestrator { initial: string[] | "ALL" | undefined ): Promise { try { - if (initial === "ALL") { - await this.manager.enableToolsets(this.resolver.getAvailableToolsets()); - } else if (Array.isArray(initial) && initial.length > 0) { - await this.manager.enableToolsets(initial); + const names = + initial === "ALL" + ? this.resolver.getAvailableToolsets() + : Array.isArray(initial) + ? initial + : []; + if (names.length > 0) { + // enableToolsets reports failures in its result instead of throwing. + // Only a failure while registering tools carries a code; refusals made + // before any registration (policy, already enabled) leave nothing partial. + const { results } = await this.manager.enableToolsets(names); + const failures = results.filter((r) => !r.success && r.code); + if (failures.length > 0) { + throw new Error( + `Failed to enable startup toolsets: ${failures.map((r) => r.message).join("; ")}` + ); + } } } catch (error) { this.initError = diff --git a/src/http/AGENTS.md b/src/http/AGENTS.md index 7f30ebc..b84f7f5 100644 --- a/src/http/AGENTS.md +++ b/src/http/AGENTS.md @@ -38,6 +38,7 @@ Provides Fastify-based HTTP transport for the MCP protocol. Handles SSE streams, → disconnectServer() — closes the bundle server's current transport (stale same-client connection) → Set transport.onclose BEFORE server.connect() (SDK 1.26+ chain requirement) → await bundle.orchestrator.isReady() — STATIC bundles preload tools async; SDK refuses capability registration after connect + → not ready → drop the bundle from the cache, respond 500 / JSON-RPC -32603; never connect with partial tools (no negative caching: each retry re-runs the module loaders) → server.connect(transport) — throws "Already connected" if above steps missed → Generate session ID, store in bundle.sessions via onsessioninitialized diff --git a/src/http/FastifyTransport.ts b/src/http/FastifyTransport.ts index 5074f3b..23648b4 100644 --- a/src/http/FastifyTransport.ts +++ b/src/http/FastifyTransport.ts @@ -256,7 +256,23 @@ export class FastifyTransport { try { // STATIC bundles preload toolsets asynchronously; the SDK refuses to // register tool capabilities once connected, so wait for them first. - await bundle.orchestrator.isReady(); + const ready = await bundle.orchestrator.isReady(); + if (!ready) { + // A failed preload is cached on the orchestrator for good, so drop + // the bundle and let the client's next initialize start a fresh one. + if (this.clientCache.get(cacheKey) === bundle) { + this.clientCache.delete(cacheKey); + } + reply.code(500); + return { + jsonrpc: "2.0", + error: { + code: -32603, + message: "Failed to load toolsets for this client.", + }, + id: null, + }; + } await bundle.server.connect(transport); } catch (error) { reply.code(500); diff --git a/src/server/AGENTS.md b/src/server/AGENTS.md index 27ee851..bc936fb 100644 --- a/src/server/AGENTS.md +++ b/src/server/AGENTS.md @@ -17,7 +17,7 @@ Factory functions for creating MCP servers. Provides standard and permission-bas 2. **Every client gets its own server, in both modes** — The MCP SDK binds one `McpServer` to one transport, so a shared STATIC server made each new client disconnect the previous one. STATIC client orchestrators preload the startup toolsets with the client's merged session context. The base server/orchestrator only serves validation at startup and the `/tools` status endpoint. 3. **Permission-based servers reject `startup` field** — Throws if `startup` provided. Toolsets come from permissions, not config. 4. **Permission-based sanitizes exposure policy** — `sanitizeExposurePolicyForPermissions` strips allowlist/denylist/maxActiveToolsets with warnings. -5. **`ensureReady()` before transport start** — STATIC mode waits for the base toolset initialization to complete (fail-fast). Per-client readiness is awaited by the transport before `connect()`. +5. **`ensureReady()` before transport start** — STATIC mode waits for the base toolset initialization to complete (fail-fast). Per-client readiness is awaited by the transport before `connect()`; a client whose preload failed gets a JSON-RPC error, not a partially loaded server. 6. **Orchestrator built via conditional builder calls** — Optional fields (`exposurePolicy`, `startup`) passed to builder only when defined. Never use `!` to coerce. ## Notifier Pattern diff --git a/tests/e2e/staticMode.e2e.test.ts b/tests/e2e/staticMode.e2e.test.ts index e27232a..c3333f7 100644 --- a/tests/e2e/staticMode.e2e.test.ts +++ b/tests/e2e/staticMode.e2e.test.ts @@ -242,6 +242,84 @@ describe("STATIC mode E2E", () => { }); }); + describe("when a client's toolsets fail to load", () => { + let server: Awaited>; + let port: number; + let flakyAttempts = 0; + + beforeAll(async () => { + port = await getAvailablePort(); + server = await createMcpServer({ + createServer: () => + new McpServer({ + name: "test-static-failing-preload", + version: "1.0.0", + }), + catalog: sessionContextCatalog, + moduleLoaders: { + // Loads fine for the startup check (no token). For "bad" it returns + // two tools with one name, so registration fails halfway through. + tenant: async (ctx: any) => { + const tools = await sessionContextModuleLoaders.tenant(ctx); + if (ctx?.API_TOKEN === "bad") return [...tools, ...tools]; + if (ctx?.API_TOKEN === "flaky" && flakyAttempts++ === 0) { + return [...tools, ...tools]; + } + return tools; + }, + }, + sessionContext: { + enabled: true, + queryParam: { + name: "config", + encoding: "base64", + allowedKeys: ["API_TOKEN"], + }, + merge: "shallow", + }, + startup: { mode: "STATIC", toolsets: "ALL" }, + http: { port }, + }); + await server.start(); + }); + + afterAll(async () => { + await server.close(); + }); + + const connect = async (clientId: string, token: string) => { + const config = Buffer.from(JSON.stringify({ API_TOKEN: token })).toString("base64"); + const transport = new StreamableHTTPClientTransport( + new URL(`http://localhost:${port}/mcp?config=${config}`), + { requestInit: { headers: { "mcp-client-id": clientId } } } + ); + const client = new Client({ name: "test", version: "1.0.0" }); + await client.connect(transport); + return client; + }; + + it("rejects the client instead of connecting with partial tools", async () => { + await expect(connect("failing-preload-client", "bad")).rejects.toThrow( + /Failed to load toolsets/ + ); + }); + + it("keeps serving clients whose toolsets load", async () => { + const client = await connect("healthy-preload-client", "good"); + expect(extractToolNames(await client.listTools())).toContain("tenant.get_context"); + await client.close(); + }); + + it("lets the same client retry after a failed preload", async () => { + await expect(connect("flaky-preload-client", "flaky")).rejects.toThrow( + /Failed to load toolsets/ + ); + const client = await connect("flaky-preload-client", "flaky"); + expect(extractToolNames(await client.listTools())).toContain("tenant.get_context"); + await client.close(); + }); + }); + describe("with ALL toolsets", () => { let server: Awaited>; let port: number; diff --git a/tests/fastifyTransport.test.ts b/tests/fastifyTransport.test.ts index 7d1ae49..d8e0565 100644 --- a/tests/fastifyTransport.test.ts +++ b/tests/fastifyTransport.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import Fastify from "fastify"; import { FastifyTransport } from "../src/http/FastifyTransport.js"; import { DynamicToolManager } from "../src/core/DynamicToolManager.js"; @@ -158,6 +158,62 @@ describe("FastifyTransport", () => { expect(res3.statusCode).toBe(404); expect(res3.json().error.message).toContain("not found"); + await transport.stop(); + }); + it("POST /mcp initialize returns a JSON-RPC error when the client's toolsets failed to load", async () => { + const connect = vi.fn(async (_t: any) => {}); + const server: any = { connect, close: async () => {} }; + const resolver = new ModuleResolver({ + catalog: { core: { name: "Core", description: "", tools: [] } } as any, + }); + const manager = new DynamicToolManager({ server, resolver }); + + const app = Fastify({ logger: false }); + const createBundle = vi.fn(() => ({ + server, + orchestrator: { isReady: async () => false } as any, + })); + const transport = new FastifyTransport(manager, createBundle, { + port: 0, + logger: false, + app, + }); + await transport.start(); + + const initialize = () => + app.inject({ + method: "POST", + url: "/mcp", + headers: { + "mcp-client-id": "failing-client", + "content-type": "application/json", + accept: "application/json, text/event-stream", + }, + payload: { + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2025-03-26", + capabilities: {}, + clientInfo: { name: "test", version: "1.0.0" }, + }, + }, + }); + + const first = await initialize(); + expect(first.statusCode).toBe(500); + expect(first.json()).toEqual({ + jsonrpc: "2.0", + error: { code: -32603, message: "Failed to load toolsets for this client." }, + id: null, + }); + expect(connect).not.toHaveBeenCalled(); + + // The failed bundle is dropped, so a retry builds a fresh one + expect((await initialize()).statusCode).toBe(500); + expect(createBundle).toHaveBeenCalledTimes(2); + await transport.stop(); }); }); diff --git a/tests/serverOrchestrator.test.ts b/tests/serverOrchestrator.test.ts index 92b2e22..2596657 100644 --- a/tests/serverOrchestrator.test.ts +++ b/tests/serverOrchestrator.test.ts @@ -102,4 +102,58 @@ describe("ServerOrchestrator", () => { }) ).toThrow(/STATIC mode requires valid toolsets or 'ALL'; none were valid/); }); + it("reports not ready when a startup toolset fails while registering tools", async () => { + const error = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const { server } = createFakeMcpServer(); + const clashing = { + core: catalog.core, + other: { name: "Other", description: "", tools: [...catalog.core.tools] }, + }; + const orch = new ServerOrchestrator({ + server, + catalog: clashing as any, + startup: { mode: "STATIC", toolsets: "ALL" }, + exposurePolicy: { namespaceToolsWithSetKey: false }, + }); + expect(await orch.isReady()).toBe(false); + await expect(orch.ensureReady()).rejects.toThrow( + /Failed to enable startup toolsets: .*other.*collision/ + ); + } finally { + error.mockRestore(); + } + }); + + it("stays ready when the exposure policy refuses a startup toolset", async () => { + const { server } = createFakeMcpServer(); + const orch = new ServerOrchestrator({ + server, + catalog: catalog as any, + startup: { mode: "STATIC", toolsets: "ALL" }, + exposurePolicy: { denylist: ["ext"] }, + }); + expect(await orch.isReady()).toBe(true); + expect(orch.getManager().getActiveToolsets()).toEqual(["core"]); + }); + + it("stays ready when startup.toolsets lists a toolset twice", async () => { + const { server } = createFakeMcpServer(); + const orch = new ServerOrchestrator({ + server, + catalog: catalog as any, + startup: { mode: "STATIC", toolsets: ["core", "core"] }, + }); + expect(await orch.isReady()).toBe(true); + }); + + it("reports ready when every startup toolset enables", async () => { + const { server } = createFakeMcpServer(); + const orch = new ServerOrchestrator({ + server, + catalog: catalog as any, + startup: { mode: "STATIC", toolsets: "ALL" }, + }); + expect(await orch.isReady()).toBe(true); + }); });