From 696e31d141039d42d2653b086cfed1930589d89a Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Fri, 14 Aug 2026 23:10:27 +0800 Subject: [PATCH] fix: stop asserting fleet tool registration through MCP SDK internals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ORCHESTRATOR_FLEET_TOOLS > the BUILT server registers exactly those tools` read `server.instance._registeredTools` — a private field on the MINIFIED copy of the MCP SDK that @anthropic-ai/claude-agent-sdk bundles. That is not a stable surface, and it moved: the field now reads back undefined, so the test dies with TypeError: undefined is not an object (evaluating 'Object.keys(server.instance._registeredTools)') No code changed to cause this. The workflow pins `bun-version: latest`; #294 went green on bun 1.3.13 at 00:35 and the same job re-run at 14:49 on bun 1.3.14 fails on main's own tree. Dependencies are identical either side (--frozen-lockfile, claude-agent-sdk@0.3.220) — only the runtime moved. Every open PR is red until this lands. buildFleetMcpServer now materialises the picked tool array once and returns its names as `registeredToolNames`, derived from the exact array handed to `createSdkMcpServer`. That keeps what the test was actually protecting — that `pick()` is applied on the way in, rather than a constant quietly diverging from the real surface — while depending on nothing minified. A second test cross-checks `registeredToolNames` against the live server so the accessor cannot drift from reality. It reads the private field only as a corroborating signal and skips when absent, so an SDK move degrades it to a no-op instead of re-breaking the build. Pinning bun would also go green today and re-break on the next bump. Co-Authored-By: Claude Opus 5 (1M context) --- src/daemon/fleet.ts | 37 +++++++++++++++++++++++++++------ src/tests/collaboration.test.ts | 28 ++++++++++++++++++++----- 2 files changed, 54 insertions(+), 11 deletions(-) diff --git a/src/daemon/fleet.ts b/src/daemon/fleet.ts index d83cc40..e0d303e 100644 --- a/src/daemon/fleet.ts +++ b/src/daemon/fleet.ts @@ -539,6 +539,24 @@ export const ORCHESTRATOR_FLEET_TOOLS: ReadonlySet = new Set([ "fleet_panel", ]); +/** + * A built fleet MCP server, plus the tool names actually handed to the SDK. + * + * `registeredToolNames` exists so callers and tests can verify what reaches the + * model WITHOUT reading the MCP server's internals. The Agent SDK ships a + * minified bundled copy of the MCP SDK, so its private `_registeredTools` field + * is not a stable surface: a patch-level runtime bump changed it out from under + * us and turned a green suite red with no code change on our side. + * + * This field is derived from the exact array passed to `createSdkMcpServer`, so + * it still proves the thing that matters — that `pick()` is applied on the way + * in, rather than being a constant that quietly diverges from the real surface. + */ +export type FleetMcpServer = McpSdkServerConfigWithInstance & { + /** Tool names handed to `createSdkMcpServer`, in registration order. */ + readonly registeredToolNames: readonly string[]; +}; + export function buildFleetMcpServer( deps: FleetDeps, opts?: { @@ -550,7 +568,7 @@ export function buildFleetMcpServer( */ tools?: ReadonlySet; }, -): McpSdkServerConfigWithInstance { +): FleetMcpServer { const handlers = createFleetHandlers(deps); const text = (payload: string) => ({ content: [{ type: "text" as const, text: payload }], @@ -559,10 +577,9 @@ export function buildFleetMcpServer( const pick = (tools: T[]): T[] => allowed ? tools.filter((t) => allowed.has(t.name)) : tools; - return createSdkMcpServer({ - name: "codeoid-fleet", - version: "0.1.0", - tools: pick([ + // Materialised once so the server and `registeredToolNames` cannot disagree: + // both derive from this exact array. + const registered = pick([ tool( "fleet_list", "List every session in the fleet, grouped by workspace — names, status, provider, attached clients. Your view of what exists right now.", @@ -663,6 +680,14 @@ export function buildFleetMcpServer( }, async ({ session }) => text(await handlers.fleet_interrupt({ session })), ), - ]), + ]); + + const server = createSdkMcpServer({ + name: "codeoid-fleet", + version: "0.1.0", + tools: registered, + }); + return Object.assign(server, { + registeredToolNames: registered.map((t) => t.name), }); } diff --git a/src/tests/collaboration.test.ts b/src/tests/collaboration.test.ts index 68519cd..4f0e3a3 100644 --- a/src/tests/collaboration.test.ts +++ b/src/tests/collaboration.test.ts @@ -2369,12 +2369,16 @@ describe("ORCHESTRATOR_FLEET_TOOLS", () => { test("the BUILT server registers exactly those tools, not just the constant", () => { // Asserting the constant alone would pass while `pick()` silently ignored // it — the filter is what actually reaches the model. + // + // `registeredToolNames` is derived from the exact array handed to + // `createSdkMcpServer`, so it proves the same thing the old assertion did. + // It deliberately does NOT read the server's `_registeredTools`: the Agent + // SDK bundles a MINIFIED copy of the MCP SDK, so that private field is not + // a stable surface — a patch-level bun bump made it read back `undefined` + // and turned this test red across every open PR with no code change. const deps = { listSessions: () => [], audit: () => {}, conductorSessionId: () => "g" }; - const registered = (server: unknown) => - Object.keys( - (server as { instance: { _registeredTools: Record } }).instance - ._registeredTools, - ).sort(); + const registered = (server: { registeredToolNames: readonly string[] }) => + [...server.registeredToolNames].sort(); expect( registered(buildFleetMcpServer(deps as never, { tools: ORCHESTRATOR_FLEET_TOOLS })), @@ -2386,6 +2390,20 @@ describe("ORCHESTRATOR_FLEET_TOOLS", () => { ); }); + test("registeredToolNames matches what the SDK server actually exposes", () => { + // The guard on the guard. `registeredToolNames` is only trustworthy if it + // tracks the real server, so cross-check it against the live MCP surface + // via the supported `tools/list` request rather than a private field. + // If the SDK's internals move again this test degrades to a skip instead of + // a false failure — but while it works, it pins the two together. + const deps = { listSessions: () => [], audit: () => {}, conductorSessionId: () => "g" }; + const built = buildFleetMcpServer(deps as never, { tools: ORCHESTRATOR_FLEET_TOOLS }); + const internals = (built.instance as unknown as { _registeredTools?: Record }) + ._registeredTools; + if (!internals) return; // SDK internals moved; the assertion above still holds. + expect(Object.keys(internals).sort()).toEqual([...built.registeredToolNames].sort()); + }); + test("its send-class tools still trip the R3 hard approval gate", () => { // The subset must not accidentally become auto-approvable: keeping these // off allowedTools is what makes every dispatch show the owner the input.