diff --git a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts index 58561ea2..d5c39c6e 100644 --- a/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts +++ b/apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts @@ -718,7 +718,11 @@ export class AiChatToolsService { if (spec.mcpOnly) continue; if (spec.inlineBothHosts) continue; const run = spec.inAppExecute ?? spec.execute; - if (!run) continue; // defensive: a shared spec always carries one of them. + // Guaranteed present by assertEverySpecIsRegisterable() (#494), which runs + // at tool-specs module load and throws if a non-inline spec the in-app host + // registers carries neither inAppExecute nor execute — so this can no longer + // silently drop a mis-declared tool. Kept as a type-narrowing guard. + if (!run) continue; tools[spec.inAppKey] = sharedTool( spec, (async (args) => diff --git a/packages/mcp/src/index.ts b/packages/mcp/src/index.ts index 298c88d5..745c68a8 100644 --- a/packages/mcp/src/index.ts +++ b/packages/mcp/src/index.ts @@ -304,7 +304,11 @@ export function createDocmostMcpServer(config: DocmostMcpConfig): McpServer { content: { type: "text"; text: string }[]; }; } - // Canonical execute returns raw data; wrap it as JSON text content. + // Canonical execute returns raw data; wrap it as JSON text content. The `!` + // is backed by assertEverySpecIsRegisterable() (#494), which runs at + // tool-specs module load and throws if a non-inline, non-inAppOnly spec + // reaches this loop without an execute/mcpExecute — so this can no longer be + // a call-time TypeError in production. const raw = await spec.execute!(docmostClient, args); return jsonContent(raw); }; diff --git a/packages/mcp/src/tool-specs.ts b/packages/mcp/src/tool-specs.ts index 0bc9fd24..106602fa 100644 --- a/packages/mcp/src/tool-specs.ts +++ b/packages/mcp/src/tool-specs.ts @@ -2433,3 +2433,48 @@ export function assertEverySpecDeclaresWriteClass(): void { // Enforce at module load (registration time) on both hosts. assertEverySpecDeclaresWriteClass(); + +/** + * Registration-time assert (#494): every spec that a host registers through the + * shared-registry loop MUST carry a callable execute path for THAT host. On the + * MCP host (index.ts) the loop runs `mcpExecute` else `spec.execute!` — a spec + * with neither throws a TypeError at CALL time (fires only once the model picks + * that tool, in production, on the MCP host). On the in-app host + * (ai-chat-tools.service.ts) the loop does `inAppExecute ?? execute`, then + * `if (!run) continue` — a spec with neither is SILENTLY dropped, so the tool + * simply vanishes from the agent with no error at all. A `// mirror this` comment + * is not a guard; this closes the mirror with a real structural check that fires + * at module load on BOTH hosts (both import this file), turning a latent runtime + * TypeError / silent-drop into a loud startup failure. + * + * `inlineBothHosts` specs are exempt: both hosts register them inline with a + * hand-wired handler and they deliberately carry no execute (their backing helper + * cannot cross into this zod-agnostic file). A spec that is `inAppOnly` need not + * satisfy the MCP-host arm (that host skips it) and vice-versa for `mcpOnly`. + */ +export function assertEverySpecIsRegisterable( + specs: Record = SHARED_TOOL_SPECS, +): void { + for (const [key, spec] of Object.entries(specs)) { + if (spec.inlineBothHosts) continue; + // MCP host registers the spec unless it is inAppOnly; its handler calls + // `mcpExecute` when present, otherwise `execute!`. + if (!spec.inAppOnly && !spec.execute && !spec.mcpExecute) { + throw new Error( + `tool-specs: spec "${key}" is registered on the MCP host but carries ` + + `neither execute nor mcpExecute`, + ); + } + // In-app host registers it unless it is mcpOnly; its loop runs + // `inAppExecute ?? execute`. + if (!spec.mcpOnly && !spec.execute && !spec.inAppExecute) { + throw new Error( + `tool-specs: spec "${key}" is registered on the in-app host but carries ` + + `neither execute nor inAppExecute`, + ); + } + } +} + +// Enforce at module load (registration time) on both hosts. +assertEverySpecIsRegisterable(); diff --git a/packages/mcp/test/unit/tool-specs.test.mjs b/packages/mcp/test/unit/tool-specs.test.mjs index aad5354a..7a4e57e1 100644 --- a/packages/mcp/test/unit/tool-specs.test.mjs +++ b/packages/mcp/test/unit/tool-specs.test.mjs @@ -7,6 +7,7 @@ import { SHARED_TOOL_WRITE_CLASS, isRetryableWriteClass, assertEverySpecDeclaresWriteClass, + assertEverySpecIsRegisterable, } from "../../build/tool-specs.js"; // The shared registry is consumed by BOTH the zod-v3 MCP server and the zod-v4 @@ -83,6 +84,99 @@ test("#489: representative reads are readOnly and representative writes are writ } }); +// #494 — every non-inline spec MUST carry a callable execute path for each host +// that registers it, or the MCP host throws a call-time TypeError (`execute!`) +// and the in-app host silently drops the tool. A registration-time assert closes +// this mirror. These tests REDDEN if the guard is weakened/removed. +test("#494: assertEverySpecIsRegisterable does not throw for the shipped registry", () => { + assert.doesNotThrow(() => assertEverySpecIsRegisterable()); + // Sanity: the shipped registry really does satisfy the invariant per-spec. + for (const [key, spec] of Object.entries(SHARED_TOOL_SPECS)) { + if (spec.inlineBothHosts) continue; + if (!spec.inAppOnly) { + assert.ok( + spec.execute || spec.mcpExecute, + `${key}: MCP-host spec missing execute/mcpExecute`, + ); + } + if (!spec.mcpOnly) { + assert.ok( + spec.execute || spec.inAppExecute, + `${key}: in-app-host spec missing execute/inAppExecute`, + ); + } + } +}); + +test("#494: a non-inline spec with no execute/mcpExecute is rejected (MCP-host arm)", () => { + // A shared spec (registered on BOTH hosts) with no execute at all. + const bad = { + lonely: { + mcpName: "lonely", + inAppKey: "lonely", + writeClass: "readOnly", + description: "no execute anywhere", + tier: "core", + catalogLine: "lonely — nothing", + }, + }; + assert.throws( + () => assertEverySpecIsRegisterable(bad), + /lonely.*neither execute nor mcpExecute/, + ); +}); + +test("#494: an inAppOnly spec with no execute/inAppExecute is rejected (in-app-host arm)", () => { + const bad = { + lonely: { + mcpName: "lonely", + inAppKey: "lonely", + writeClass: "readOnly", + description: "in-app only, but no runner", + tier: "core", + catalogLine: "lonely — nothing", + inAppOnly: true, + }, + }; + assert.throws( + () => assertEverySpecIsRegisterable(bad), + /lonely.*neither execute nor inAppExecute/, + ); +}); + +test("#494: an mcpExecute-only spec passes the MCP arm; an inAppExecute-only spec passes the in-app arm", () => { + // mcpOnly spec with only mcpExecute — the MCP arm is satisfied, the in-app arm + // is skipped (mcpOnly), so it must NOT throw. + const mcpOnly = { + t: { + mcpName: "t", inAppKey: "t", writeClass: "readOnly", + description: "x", tier: "core", catalogLine: "t — x", + mcpOnly: true, mcpExecute: async () => ({}), + }, + }; + assert.doesNotThrow(() => assertEverySpecIsRegisterable(mcpOnly)); + // inAppOnly spec with only inAppExecute — symmetric. + const inAppOnly = { + t: { + mcpName: "t", inAppKey: "t", writeClass: "readOnly", + description: "x", tier: "core", catalogLine: "t — x", + inAppOnly: true, inAppExecute: async () => ({}), + }, + }; + assert.doesNotThrow(() => assertEverySpecIsRegisterable(inAppOnly)); +}); + +test("#494: an inlineBothHosts spec is exempt from the execute requirement", () => { + const inline = { + t: { + mcpName: "t", inAppKey: "t", writeClass: "readOnly", + description: "x", tier: "core", catalogLine: "t — x", + inlineBothHosts: true, // no execute — registered inline by both hosts + }, + }; + assert.doesNotThrow(() => assertEverySpecIsRegisterable(inline)); +}); + test("buildShape (when present) returns a usable ZodRawShape with a real zod", () => { for (const [key, spec] of Object.entries(SHARED_TOOL_SPECS)) { if (!spec.buildShape) continue;