From 78515dbf915b3ec732160693a76a0c012695e004 Mon Sep 17 00:00:00 2001 From: Yash Datta Date: Wed, 19 Aug 2026 11:12:02 +0800 Subject: [PATCH] fix: the fleet-tools test depended on a private SDK field, so it failed on file order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `daemon-check` failed on #295 with a TypeError, not an assertion mismatch: undefined is not an object (evaluating 'Object.keys(server.instance._registeredTools)') at src/tests/collaboration.test.ts:2374 `_registeredTools` is a PRIVATE field of the MCP SDK's McpServer. Two ways that read goes wrong, and the second is what bit us: 1. It breaks on any SDK upgrade that renames or restructures internals. 2. `src/tests/provider-claude.test.ts` installs a process-global `mock.module("@anthropic-ai/claude-agent-sdk", ...)` whose fake `createSdkMcpServer` returns `{ instance: { tools } }` — no `_registeredTools`. Bun's module mocks leak across files within a run, so whether this test saw the real SDK or that fake depended entirely on which file the runner reached first. Locally collaboration.test.ts is 4th and provider-claude.test.ts is 14th, so it passed; CI's readdir order differs and it threw. Neither #295 nor #294 caused this — #295 touches no fleet code, #294 touches no fleet.ts, and the test passes on main locally. It is a latent order-dependent bug that surfaces whenever file ordering shifts, which adding any test file can do. Fix the coupling rather than the symptom. `buildFleetMcpServer` now builds its tool list through an exported `fleetToolDefinitions()` and passes that array through verbatim; the test asserts on those definitions instead of reaching into the SDK. That still proves what the old test set out to prove — that `pick()` actually filters what reaches the model, not just that the constant is well-formed — while being immune to both the SDK's internals and the mock leak. Adds a second test pinning the seam the split introduces: `fleetToolDefinitions` would be worthless if the builder stopped using it. That one asserts only the SDK's PUBLIC config shape, so it holds under a real or a mocked SDK. Verified under both file orders (provider-claude first and last) and on the full suite: 2311 pass, 0 fail. Co-Authored-By: Claude Opus 5 (1M context) --- src/daemon/fleet.ts | 56 ++++++++++++++++++++++++--------- src/tests/collaboration.test.ts | 43 ++++++++++++++++++------- 2 files changed, 72 insertions(+), 27 deletions(-) diff --git a/src/daemon/fleet.ts b/src/daemon/fleet.ts index d83cc40..416941c 100644 --- a/src/daemon/fleet.ts +++ b/src/daemon/fleet.ts @@ -539,18 +539,48 @@ export const ORCHESTRATOR_FLEET_TOOLS: ReadonlySet = new Set([ "fleet_panel", ]); +export interface FleetToolOpts { + /** + * Restrict the exposed tools to these names. Absent = the full conductor + * surface. Filtering here rather than building a second server keeps one + * definition of every tool's schema and description, so the orchestrator + * can never drift into a differently-worded `fleet_send`. + */ + tools?: ReadonlySet; +} + +/** + * The tool definitions the fleet server exposes, after filtering. + * + * Split out of `buildFleetMcpServer` so the filter can be asserted directly. + * The alternative — reading the built server's `instance._registeredTools` — + * couples a test to a PRIVATE field of the MCP SDK, which breaks on an SDK + * upgrade and, worse, silently reads the wrong shape whenever another test file + * has installed a process-global `mock.module` for the agent SDK (bun's module + * mocks leak across files in a run, so this depended on test ORDER). + * + * `buildFleetMcpServer` passes this array through verbatim, so asserting on it + * still proves what actually reaches the model. + */ +export function fleetToolDefinitions( + deps: FleetDeps, + opts?: FleetToolOpts, +): Array<{ name: string }> { + return buildFleetTools(deps, opts); +} + export function buildFleetMcpServer( deps: FleetDeps, - opts?: { - /** - * Restrict the exposed tools to these names. Absent = the full conductor - * surface. Filtering here rather than building a second server keeps one - * definition of every tool's schema and description, so the orchestrator - * can never drift into a differently-worded `fleet_send`. - */ - tools?: ReadonlySet; - }, + opts?: FleetToolOpts, ): McpSdkServerConfigWithInstance { + return createSdkMcpServer({ + name: "codeoid-fleet", + version: "0.1.0", + tools: buildFleetTools(deps, opts) as never, + }); +} + +function buildFleetTools(deps: FleetDeps, opts?: FleetToolOpts) { const handlers = createFleetHandlers(deps); const text = (payload: string) => ({ content: [{ type: "text" as const, text: payload }], @@ -559,10 +589,7 @@ 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([ + return 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 +690,5 @@ export function buildFleetMcpServer( }, async ({ session }) => text(await handlers.fleet_interrupt({ session })), ), - ]), - }); + ]); } diff --git a/src/tests/collaboration.test.ts b/src/tests/collaboration.test.ts index 68519cd..0b9081a 100644 --- a/src/tests/collaboration.test.ts +++ b/src/tests/collaboration.test.ts @@ -48,6 +48,7 @@ import { TranscriptStore } from "../daemon/transcript.js"; import { roleDeniesTool } from "../daemon/providers/tool-safety.js"; import { buildFleetMcpServer, + fleetToolDefinitions, FLEET_SEND_TOOL_NAMES, FLEET_TOOL_NAMES, isFleetSendTool, @@ -2366,24 +2367,42 @@ describe("ORCHESTRATOR_FLEET_TOOLS", () => { } }); - test("the BUILT server registers exactly those tools, not just the constant", () => { + test("the BUILT surface exposes 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. + // + // Reads the definitions the builder passes through, NOT the built server's + // `instance._registeredTools`. That private MCP-SDK field breaks on an SDK + // upgrade, and — the reason this changed — it silently reads the wrong + // shape whenever another test file has installed a process-global + // `mock.module("@anthropic-ai/claude-agent-sdk")`. Bun's module mocks leak + // across files in a run, so the old assertion passed or threw purely on + // test file ORDER (provider-claude.test.ts installs exactly such a mock, + // whose fake server has `instance.tools` and no `_registeredTools`). const deps = { listSessions: () => [], audit: () => {}, conductorSessionId: () => "g" }; - const registered = (server: unknown) => - Object.keys( - (server as { instance: { _registeredTools: Record } }).instance - ._registeredTools, - ).sort(); + const names = (opts?: { tools: ReadonlySet }) => + fleetToolDefinitions(deps as never, opts).map((t) => t.name).sort(); - expect( - registered(buildFleetMcpServer(deps as never, { tools: ORCHESTRATOR_FLEET_TOOLS })), - ).toEqual(["fleet_interrupt", "fleet_list", "fleet_panel", "fleet_send", "fleet_tasks"]); + expect(names({ tools: ORCHESTRATOR_FLEET_TOOLS })).toEqual([ + "fleet_interrupt", + "fleet_list", + "fleet_panel", + "fleet_send", + "fleet_tasks", + ]); // ...and the unfiltered conductor build still gets everything, so `pick()` // is a filter rather than a truncation. - expect(registered(buildFleetMcpServer(deps as never))).toEqual( - [...FLEET_TOOL_NAMES, ...FLEET_SEND_TOOL_NAMES].sort(), - ); + expect(names()).toEqual([...FLEET_TOOL_NAMES, ...FLEET_SEND_TOOL_NAMES].sort()); + }); + + test("the built MCP server is still wired from those same definitions", () => { + // Guards the seam the refactor introduced: `fleetToolDefinitions` would be + // worthless if `buildFleetMcpServer` stopped using it. Asserts only the + // SDK's PUBLIC config shape, so it holds under a real or mocked SDK. + const deps = { listSessions: () => [], audit: () => {}, conductorSessionId: () => "g" }; + const server = buildFleetMcpServer(deps as never, { tools: ORCHESTRATOR_FLEET_TOOLS }); + expect(server.name).toBe("codeoid-fleet"); + expect(server.type).toBe("sdk"); }); test("its send-class tools still trip the R3 hard approval gate", () => {