From 8923d4d6f4fc19ecc4e1c8132e5eccc4d0f7e805 Mon Sep 17 00:00:00 2001 From: Eva Date: Fri, 10 Jul 2026 20:48:50 +0700 Subject: [PATCH 1/3] =?UTF-8?q?feat(broker):=20@boardstate/broker=20?= =?UTF-8?q?=E2=80=94=20MCP=20client=20manager=20(M5a-1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New node-only package: the MCP CLIENT manager. Connects outward to operator-declared MCP servers, discovers their tools into a namespaced ToolManifest (with a stable anti-rug-pull hash), and calls them behind a narrow API. - config.ts: operator-authored connectors config (loadConnectorsConfig + parseConnectorsConfig). env values are process-env NAMES, never literals; validated and never echoed. - names.ts: connector:tool ids + provider-safe connector__tool names, both inside a 64-char budget; collisions fail loud. - manifest.ts: ToolManifest + deterministic hash; readOnlyHint absent => mutation (fail-safe). - broker.ts: lazy connect, pooled/warm clients, capped-backoff reconnect, stdio + Streamable-HTTP (SSE fallback) transports, callTool timeout + isError normalization. - fixture/: in-repo fake MCP server (stdio child + in-process HTTP), CI needs no network. - Depends only on @modelcontextprotocol/sdk + @boardstate/server (types only). Closes #38 --- packages/broker/package.json | 46 +++ packages/broker/src/broker.test.ts | 183 ++++++++++ packages/broker/src/broker.ts | 343 ++++++++++++++++++ packages/broker/src/broker.wire.test.ts | 120 ++++++ packages/broker/src/config.test.ts | 145 ++++++++ packages/broker/src/config.ts | 205 +++++++++++ packages/broker/src/errors.ts | 74 ++++ .../broker/src/fixture/fake-mcp-server.ts | 157 ++++++++ packages/broker/src/fixture/http-harness.ts | 70 ++++ packages/broker/src/fixture/stdio-entry.ts | 20 + packages/broker/src/index.ts | 42 +++ packages/broker/src/manifest.test.ts | 109 ++++++ packages/broker/src/manifest.ts | 115 ++++++ packages/broker/src/names.test.ts | 57 +++ packages/broker/src/names.ts | 99 +++++ packages/broker/tsconfig.json | 8 + packages/broker/tsdown.config.ts | 10 + packages/broker/vitest.config.ts | 5 + 18 files changed, 1808 insertions(+) create mode 100644 packages/broker/package.json create mode 100644 packages/broker/src/broker.test.ts create mode 100644 packages/broker/src/broker.ts create mode 100644 packages/broker/src/broker.wire.test.ts create mode 100644 packages/broker/src/config.test.ts create mode 100644 packages/broker/src/config.ts create mode 100644 packages/broker/src/errors.ts create mode 100644 packages/broker/src/fixture/fake-mcp-server.ts create mode 100644 packages/broker/src/fixture/http-harness.ts create mode 100644 packages/broker/src/fixture/stdio-entry.ts create mode 100644 packages/broker/src/index.ts create mode 100644 packages/broker/src/manifest.test.ts create mode 100644 packages/broker/src/manifest.ts create mode 100644 packages/broker/src/names.test.ts create mode 100644 packages/broker/src/names.ts create mode 100644 packages/broker/tsconfig.json create mode 100644 packages/broker/tsdown.config.ts create mode 100644 packages/broker/vitest.config.ts diff --git a/packages/broker/package.json b/packages/broker/package.json new file mode 100644 index 0000000..444ff3d --- /dev/null +++ b/packages/broker/package.json @@ -0,0 +1,46 @@ +{ + "name": "@boardstate/broker", + "version": "0.1.0", + "description": "The MCP client manager: connect outward to external MCP servers, discover their tools, and call them behind a narrow, provider-safe API", + "type": "module", + "license": "MIT", + "homepage": "https://github.com/100yenadmin/boardstate#readme", + "bugs": "https://github.com/100yenadmin/boardstate/issues", + "keywords": [ + "mcp", + "model-context-protocol", + "client", + "broker", + "tools", + "boardstate" + ], + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "bin": { + "boardstate-fake-mcp": "./dist/fixture/stdio-entry.js" + }, + "files": [ + "dist" + ], + "scripts": { + "build": "tsdown", + "test": "vitest run" + }, + "dependencies": { + "@boardstate/server": "workspace:*", + "@modelcontextprotocol/sdk": "^1.29.0" + }, + "repository": { + "type": "git", + "url": "git+https://github.com/100yenadmin/boardstate.git", + "directory": "packages/broker" + }, + "publishConfig": { + "access": "public", + "provenance": true + } +} diff --git a/packages/broker/src/broker.test.ts b/packages/broker/src/broker.test.ts new file mode 100644 index 0000000..5f9f6a3 --- /dev/null +++ b/packages/broker/src/broker.test.ts @@ -0,0 +1,183 @@ +// Broker behavior over an in-memory transport (fast, deterministic): config-only +// refusal, namespaced + provider-name calling, isError normalization, hard timeout, +// backoff on a flaky connect, and warm-client reconnect after a transport drop. + +import { afterEach, describe, expect, it } from "vitest"; +import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; +import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; +import { McpBroker } from "./broker.js"; +import { parseConnectorsConfig } from "./config.js"; +import { BrokerTimeoutError, BrokerToolError, BrokerUnknownConnectorError } from "./errors.js"; +import { buildFakeMcpServer, type FakeCatalogState } from "./fixture/fake-mcp-server.js"; + +/** + * An in-memory transport factory wired to a fresh fake server per connect. Exposes the + * connect count and the latest server-side transport so tests can force a drop. + */ +function inMemoryFactory(state: FakeCatalogState = { mutated: false }) { + let connects = 0; + let lastServerTransport: InMemoryTransport | undefined; + const factory = (): Transport => { + connects += 1; + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + lastServerTransport = serverTransport; + const server = buildFakeMcpServer(state); + void server.connect(serverTransport); + return clientTransport; + }; + return { + factory, + connects: () => connects, + dropLast: async () => { + await lastServerTransport?.close(); + // Let the propagated onclose settle before the next call. + await new Promise((resolve) => setTimeout(resolve, 5)); + }, + }; +} + +function makeBroker(state?: FakeCatalogState) { + const wiring = inMemoryFactory(state); + const config = parseConnectorsConfig({ + connectors: [{ name: "office", transport: "stdio", command: "unused-in-memory" }], + }); + const broker = new McpBroker(config, { + transportFactory: wiring.factory, + initialBackoffMs: 1, + maxBackoffMs: 4, + }); + return { broker, wiring }; +} + +describe("McpBroker", () => { + let open: McpBroker | null = null; + afterEach(async () => { + await open?.close(); + open = null; + }); + + it("refuses a connector that is not in the operator config", async () => { + const { broker } = makeBroker(); + open = broker; + await expect(broker.callTool("ghost:do_thing")).rejects.toBeInstanceOf( + BrokerUnknownConnectorError, + ); + }); + + it("discovers a namespaced manifest with the fail-safe readOnly flag", async () => { + const { broker } = makeBroker(); + open = broker; + const manifest = await broker.listTools(); + const ids = manifest.tools.map((t) => t.id); + expect(ids).toContain("office:echo"); + expect(ids).toContain("office:write_note"); + expect(manifest.tools.find((t) => t.id === "office:echo")?.readOnly).toBe(true); + // write_note has no readOnlyHint → mutation. + expect(manifest.tools.find((t) => t.id === "office:write_note")?.readOnly).toBe(false); + expect(manifest.idToProvider.get("office:echo")).toBe("office__echo"); + }); + + it("calls a tool by its manifest id and by its provider-safe name", async () => { + const { broker } = makeBroker(); + open = broker; + const manifest = await broker.listTools(); + + const byId = await broker.callTool("office:add", { a: 2, b: 3 }); + expect(byId.content).toEqual([{ type: "text", text: JSON.stringify({ sum: 5 }) }]); + + const byProvider = await broker.callTool( + "office__add", + { a: 4, b: 1 }, + { providerToId: manifest.providerToId }, + ); + expect(byProvider.content).toEqual([{ type: "text", text: JSON.stringify({ sum: 5 }) }]); + }); + + it("normalizes an isError result into a BrokerToolError carrying the server text", async () => { + const { broker } = makeBroker(); + open = broker; + let thrown: unknown; + try { + await broker.callTool("office:boom"); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(BrokerToolError); + expect((thrown as BrokerToolError).toolId).toBe("office:boom"); + expect((thrown as Error).message).toContain("boom"); + }); + + it("enforces a hard timeout", async () => { + const { broker } = makeBroker(); + open = broker; + await expect( + broker.callTool("office:sleep", { ms: 500 }, { timeout: 20 }), + ).rejects.toBeInstanceOf(BrokerTimeoutError); + }); + + it("retries a flaky connect with backoff", async () => { + let attempts = 0; + const config = parseConnectorsConfig({ + connectors: [{ name: "office", transport: "stdio", command: "x" }], + }); + // A transport whose start() rejects (a transient connect failure) for the first two + // attempts, then a live in-memory transport — the broker must back off and retry. + const broken = (): Transport => ({ + async start() { + throw new Error("connect refused"); + }, + async send() {}, + async close() {}, + }); + const broker = new McpBroker(config, { + initialBackoffMs: 1, + maxBackoffMs: 2, + transportFactory: () => { + attempts += 1; + if (attempts < 3) { + return broken(); + } + const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); + void buildFakeMcpServer().connect(serverTransport); + return clientTransport; + }, + }); + open = broker; + const manifest = await broker.listTools(); + expect(manifest.tools.length).toBeGreaterThan(0); + expect(attempts).toBe(3); + }); + + it("reconnects a warm client after the transport drops", async () => { + const { broker, wiring } = makeBroker(); + open = broker; + await broker.callTool("office:echo", { text: "hi" }); + expect(wiring.connects()).toBe(1); + + await wiring.dropLast(); + // Next use must transparently reconnect (a second connect), not call a dead client. + const result = await broker.callTool("office:echo", { text: "again" }); + expect(result.content).toEqual([{ type: "text", text: JSON.stringify({ text: "again" }) }]); + expect(wiring.connects()).toBe(2); + }); + + it("pools a warm client across calls (one connect for many calls)", async () => { + const { broker, wiring } = makeBroker(); + open = broker; + await broker.callTool("office:echo", { text: "a" }); + await broker.callTool("office:echo", { text: "b" }); + await broker.listTools(); + expect(wiring.connects()).toBe(1); + }); + + it("detects a rug-pull: the manifest hash changes when the catalog mutates", async () => { + const state: FakeCatalogState = { mutated: false }; + const { broker } = makeBroker(state); + open = broker; + const before = await broker.listTools(); + state.mutated = true; + const after = await broker.listTools(); + expect(after.hash).not.toBe(before.hash); + expect(after.tools.map((t) => t.id)).toContain("office:extra"); + }); +}); diff --git a/packages/broker/src/broker.ts b/packages/broker/src/broker.ts new file mode 100644 index 0000000..9af8666 --- /dev/null +++ b/packages/broker/src/broker.ts @@ -0,0 +1,343 @@ +// `McpBroker`: the MCP CLIENT manager. It connects OUTWARD to the external MCP servers +// an operator declared (config.ts), discovers their tools into one `ToolManifest`, and +// calls those tools behind a narrow, namespaced API the host/server layers consume. +// +// Design invariants (epic #37): +// • Config-only: a connector name not in the config is never resolved — no ambient, +// doc-introduced, or model-introduced server can be reached. +// • Lazy + pooled: a connector connects on first use and stays warm; a dropped +// transport reconnects on next use with capped exponential backoff. +// • Namespaced surface: callers speak `connector:tool` (or the provider-safe +// `connector__tool`); the broker strips the namespace before hitting the server. +// • Fail-safe reads: `readOnlyHint` absent ⇒ a tool is a mutation (manifest.ts). +// • Secret hygiene: `env` values are env-var NAMES; resolved values are forwarded to +// the transport but NEVER placed in an error, a log line, or the manifest. +// +// This package depends only on `@modelcontextprotocol/sdk` and `@boardstate/server` +// (types only). It must not import core/host/lit/schema. + +import { Client } from "@modelcontextprotocol/sdk/client/index.js"; +import { StdioClientTransport } from "@modelcontextprotocol/sdk/client/stdio.js"; +import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"; +import { SSEClientTransport } from "@modelcontextprotocol/sdk/client/sse.js"; +import type { Transport } from "@modelcontextprotocol/sdk/shared/transport.js"; +import { ErrorCode, McpError } from "@modelcontextprotocol/sdk/types.js"; +import type { ConnectorConfig, ConnectorsConfig } from "./config.js"; +import { + BrokerConnectError, + BrokerError, + BrokerTimeoutError, + BrokerToolError, + BrokerUnknownConnectorError, +} from "./errors.js"; +import { buildManifest, type DiscoveredTool, type ToolManifest } from "./manifest.js"; +import { parseManifestId } from "./names.js"; + +const CLIENT_NAME = "boardstate-broker"; +const CLIENT_VERSION = "0.1.0"; + +/** Tunable connect/backoff policy; every field has a safe default. */ +export type BrokerOptions = { + /** Max connect attempts before a connector's connect rejects. Default 4. */ + maxConnectAttempts?: number; + /** First backoff delay (ms); doubles each retry up to `maxBackoffMs`. Default 100. */ + initialBackoffMs?: number; + /** Backoff ceiling (ms). Default 5000. */ + maxBackoffMs?: number; + /** Default per-call timeout (ms) when `callTool` is given none. Default 30000. */ + defaultCallTimeoutMs?: number; + /** + * Reads process env for `env`/header refs. Injectable for tests; defaults to + * `process.env`. A missing referenced var is a connect error (never the value). + */ + env?: Record; + /** + * Build the transport(s) for a connector, freshest-first. Injectable so tests can + * drive the broker over an in-memory pair; defaults to the real stdio/http transports. + * Returning more than one transport requests an ordered fallback (http → SSE): each is + * tried in turn within a single connect attempt. + */ + transportFactory?: ( + connector: ConnectorConfig, + env: Record, + ) => Transport | Transport[]; +}; + +type ConnectorRuntime = { + config: ConnectorConfig; + client?: Client; + /** In-flight connect, shared so concurrent callers don't open duplicate clients. */ + connecting?: Promise; +}; + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** Interpolate `${ENV_NAME}` refs in a header value against resolved env (fail-closed). */ +function resolveHeaderValue( + connector: string, + header: string, + raw: string, + env: Record, +): string { + return raw.replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (_match, name: string) => { + const value = env[name]; + if (value === undefined) { + throw new BrokerConnectError( + `connector "${connector}": header "${header}" references env var ${name}, which is not set`, + ); + } + return value; + }); +} + +/** Resolve an `env` ref map ({ CHILD: SOURCE }) into concrete values (never logged). */ +function resolveEnvRefs( + connector: string, + refs: Record | undefined, + env: Record, +): Record { + const out: Record = {}; + for (const [childVar, sourceName] of Object.entries(refs ?? {})) { + const value = env[sourceName]; + if (value === undefined) { + throw new BrokerConnectError( + `connector "${connector}": env["${childVar}"] references ${sourceName}, which is not set`, + ); + } + out[childVar] = value; + } + return out; +} + +/** + * The default transport builder: a stdio transport for local servers, or an ordered + * [Streamable HTTP, SSE] pair for remotes. The broker tries Streamable HTTP first and + * falls back to the legacy SSE transport when the modern endpoint won't connect + * (Pipedream/Composio-class servers that only speak SSE). + */ +function defaultTransportFactory( + connector: ConnectorConfig, + env: Record, +): Transport[] { + if (connector.transport === "stdio") { + const resolvedEnv = resolveEnvRefs(connector.name, connector.env, env); + return [ + new StdioClientTransport({ + command: connector.command as string, + args: connector.args, + // Only the operator-referenced vars are forwarded — no ambient inheritance. + env: resolvedEnv, + }), + ]; + } + const url = new URL(connector.url as string); + const headers: Record = {}; + for (const [key, value] of Object.entries(connector.headers ?? {})) { + headers[key] = resolveHeaderValue(connector.name, key, value, env); + } + const requestInit: RequestInit = Object.keys(headers).length > 0 ? { headers } : {}; + return [ + new StreamableHTTPClientTransport(url, { requestInit }), + new SSEClientTransport(url, { requestInit }), + ]; +} + +export class McpBroker { + private readonly runtimes = new Map(); + private readonly options: Required> & { + env: Record; + transportFactory: NonNullable; + }; + + constructor(config: ConnectorsConfig, options: BrokerOptions = {}) { + for (const connector of config.connectors) { + this.runtimes.set(connector.name, { config: connector }); + } + this.options = { + maxConnectAttempts: options.maxConnectAttempts ?? 4, + initialBackoffMs: options.initialBackoffMs ?? 100, + maxBackoffMs: options.maxBackoffMs ?? 5000, + defaultCallTimeoutMs: options.defaultCallTimeoutMs ?? 30000, + env: options.env ?? process.env, + transportFactory: options.transportFactory ?? defaultTransportFactory, + }; + } + + /** The operator-declared connector names, in config order. */ + connectorNames(): string[] { + return [...this.runtimes.keys()]; + } + + private runtime(name: string): ConnectorRuntime { + const runtime = this.runtimes.get(name); + if (!runtime) { + throw new BrokerUnknownConnectorError( + `connector "${name}" is not in the operator config — refusing to connect`, + ); + } + return runtime; + } + + /** + * Ensure a connector is connected, returning its warm client. Concurrent callers share + * one in-flight connect; a dropped transport (cleared on close) reconnects here. + */ + private async ensureConnected(name: string): Promise { + const runtime = this.runtime(name); + if (runtime.client) { + return runtime.client; + } + if (runtime.connecting) { + return runtime.connecting; + } + const connect = this.connectWithBackoff(runtime).then( + (client) => { + runtime.client = client; + runtime.connecting = undefined; + return client; + }, + (error) => { + runtime.connecting = undefined; + throw error; + }, + ); + runtime.connecting = connect; + return connect; + } + + private async connectWithBackoff(runtime: ConnectorRuntime): Promise { + const { maxConnectAttempts, initialBackoffMs, maxBackoffMs } = this.options; + let lastError: unknown; + for (let attempt = 0; attempt < maxConnectAttempts; attempt += 1) { + if (attempt > 0) { + await sleep(Math.min(initialBackoffMs * 2 ** (attempt - 1), maxBackoffMs)); + } + // Fresh transports each attempt — a failed/closed transport can't be reused. The + // http builder returns [streamable, sse]; we try them in order (SSE fallback). + const built = this.options.transportFactory(runtime.config, this.options.env); + const transports = Array.isArray(built) ? built : [built]; + for (const transport of transports) { + const client = new Client({ name: CLIENT_NAME, version: CLIENT_VERSION }); + // A transport drop after a successful connect evicts the warm client so the NEXT + // use reconnects (rather than calling a dead client). + client.onclose = () => { + if (runtime.client === client) { + runtime.client = undefined; + } + }; + try { + await client.connect(transport); + return client; + } catch (error) { + lastError = error; + await client.close().catch(() => {}); + } + } + } + throw new BrokerConnectError( + `connector "${runtime.config.name}" failed to connect after ${maxConnectAttempts} attempt(s): ${ + lastError instanceof Error ? lastError.message : String(lastError) + }`, + { cause: lastError }, + ); + } + + /** Discover one connector's tools (connecting lazily). */ + private async discover(name: string): Promise { + const client = await this.ensureConnected(name); + const result = await client.listTools(); + return result.tools.map((tool) => ({ + name: tool.name, + ...(tool.description !== undefined ? { description: tool.description } : {}), + inputSchema: tool.inputSchema as Record, + ...(tool.annotations ? { annotations: { readOnlyHint: tool.annotations.readOnlyHint } } : {}), + })); + } + + /** + * Discover every connector's tools into one {@link ToolManifest} (namespaced ids + + * provider-safe names + stable hash). Connects lazily; a connector that fails to + * connect propagates its {@link BrokerConnectError}. + */ + async listTools(): Promise { + const discovered = new Map(); + for (const name of this.runtimes.keys()) { + discovered.set(name, await this.discover(name)); + } + return buildManifest(discovered); + } + + /** + * Call a tool by its `connector:tool` id OR its provider-safe `connector__tool` name. + * Strips the namespace, enforces a hard timeout, and normalizes an `isError: true` + * result into a typed {@link BrokerToolError}. + */ + async callTool( + toolRef: string, + args: Record = {}, + opts: { timeout?: number; providerToId?: ReadonlyMap } = {}, + ): Promise<{ content: unknown; structuredContent?: unknown }> { + const id = opts.providerToId?.get(toolRef) ?? toolRef; + const { connector, tool } = parseManifestId(id); + const client = await this.ensureConnected(connector); + const timeout = opts.timeout ?? this.options.defaultCallTimeoutMs; + + let result: Awaited>; + try { + result = await client.callTool({ name: tool, arguments: args }, undefined, { timeout }); + } catch (error) { + if (error instanceof McpError && error.code === ErrorCode.RequestTimeout) { + throw new BrokerTimeoutError(`tool "${id}" timed out after ${timeout}ms`); + } + if (error instanceof BrokerError) { + throw error; + } + throw new BrokerToolError(id, error instanceof Error ? error.message : String(error)); + } + + if (result.isError === true) { + throw new BrokerToolError(id, extractErrorText(result.content)); + } + return { + content: result.content, + ...(result.structuredContent !== undefined + ? { structuredContent: result.structuredContent } + : {}), + }; + } + + /** Close every warm client. Idempotent; safe to call in a `finally`. */ + async close(): Promise { + const closing: Promise[] = []; + for (const runtime of this.runtimes.values()) { + const client = runtime.client; + runtime.client = undefined; + runtime.connecting = undefined; + if (client) { + closing.push(client.close().catch(() => {})); + } + } + await Promise.all(closing); + } +} + +/** Best-effort human text out of an MCP tool result's `content` blocks. */ +function extractErrorText(content: unknown): string { + if (Array.isArray(content)) { + const text = content + .filter((block): block is { type: string; text: string } => { + return ( + typeof block === "object" && + block !== null && + (block as { type?: unknown }).type === "text" && + typeof (block as { text?: unknown }).text === "string" + ); + }) + .map((block) => block.text) + .join("\n"); + if (text.length > 0) { + return text; + } + } + return "tool returned isError with no text content"; +} diff --git a/packages/broker/src/broker.wire.test.ts b/packages/broker/src/broker.wire.test.ts new file mode 100644 index 0000000..9b3c250 --- /dev/null +++ b/packages/broker/src/broker.wire.test.ts @@ -0,0 +1,120 @@ +// Wire-contract test: the broker against the REAL fake server over BOTH transports — +// a stdio child process and an in-process Streamable-HTTP server on loopback (no +// external network). Asserts the exact request/response shapes cross the wire and that +// the manifest hash is identical across transports and runs (determinism), plus a +// live rug-pull over http. +// +// The stdio leg spawns the COMPILED entry (`dist/fixture/stdio-entry.js`); CI runs +// `pnpm build` before `pnpm test`, so it is present. When the dist entry is absent +// (a local `vitest` with no prior build) the stdio leg is skipped rather than failing. + +import { existsSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { afterAll, describe, expect, it } from "vitest"; +import { McpBroker } from "./broker.js"; +import { parseConnectorsConfig } from "./config.js"; +import { BrokerToolError } from "./errors.js"; +import { startHttpFakeServer, type HttpFakeServer } from "./fixture/http-harness.js"; + +const STDIO_ENTRY = fileURLToPath(new URL("../dist/fixture/stdio-entry.js", import.meta.url)); +const stdioAvailable = existsSync(STDIO_ENTRY); + +function stdioBroker(): McpBroker { + const config = parseConnectorsConfig({ + connectors: [ + { name: "fake", transport: "stdio", command: process.execPath, args: [STDIO_ENTRY] }, + ], + }); + return new McpBroker(config); +} + +async function httpBroker(): Promise<{ broker: McpBroker; server: HttpFakeServer }> { + const server = await startHttpFakeServer(); + const config = parseConnectorsConfig({ + connectors: [{ name: "fake", transport: "http", url: server.url }], + }); + return { broker: new McpBroker(config), server }; +} + +describe.runIf(stdioAvailable)("wire contract — stdio child", () => { + it("lists the exact namespaced manifest and calls tools over stdio", async () => { + const broker = stdioBroker(); + try { + const manifest = await broker.listTools(); + const echo = manifest.tools.find((t) => t.id === "fake:echo"); + expect(echo).toMatchObject({ + id: "fake:echo", + providerName: "fake__echo", + connector: "fake", + tool: "echo", + readOnly: true, + }); + expect(echo?.inputSchema).toMatchObject({ type: "object", required: ["text"] }); + expect(manifest.tools.find((t) => t.id === "fake:write_note")?.readOnly).toBe(false); + + const added = await broker.callTool("fake:add", { a: 7, b: 8 }); + expect(added.content).toEqual([{ type: "text", text: JSON.stringify({ sum: 15 }) }]); + + await expect(broker.callTool("fake:boom")).rejects.toBeInstanceOf(BrokerToolError); + } finally { + await broker.close(); + } + }, 20000); +}); + +describe("wire contract — Streamable HTTP (in-process)", () => { + let http: HttpFakeServer | null = null; + afterAll(async () => { + await http?.close(); + }); + + it("lists the exact namespaced manifest and calls tools over http", async () => { + const { broker, server } = await httpBroker(); + http = server; + try { + const manifest = await broker.listTools(); + const echo = manifest.tools.find((t) => t.id === "fake:echo"); + expect(echo).toMatchObject({ + id: "fake:echo", + providerName: "fake__echo", + readOnly: true, + }); + + const added = await broker.callTool("fake:add", { a: 10, b: 20 }); + expect(added.content).toEqual([{ type: "text", text: JSON.stringify({ sum: 30 }) }]); + + await expect(broker.callTool("fake:boom")).rejects.toBeInstanceOf(BrokerToolError); + } finally { + await broker.close(); + } + }, 20000); + + it("detects a live rug-pull over the wire (hash moves when the server's catalog mutates)", async () => { + const { broker, server } = await httpBroker(); + try { + const before = await broker.listTools(); + server.state.mutated = true; + const after = await broker.listTools(); + expect(after.hash).not.toBe(before.hash); + expect(after.tools.map((t) => t.id)).toContain("fake:extra"); + } finally { + await broker.close(); + await server.close(); + } + }, 20000); +}); + +describe.runIf(stdioAvailable)("wire contract — cross-transport determinism", () => { + it("produces an identical manifest hash over stdio and http", async () => { + const stdio = stdioBroker(); + const { broker: http, server } = await httpBroker(); + try { + const [a, b] = await Promise.all([stdio.listTools(), http.listTools()]); + expect(a.hash).toBe(b.hash); + } finally { + await stdio.close(); + await http.close(); + await server.close(); + } + }, 20000); +}); diff --git a/packages/broker/src/config.test.ts b/packages/broker/src/config.test.ts new file mode 100644 index 0000000..8442909 --- /dev/null +++ b/packages/broker/src/config.test.ts @@ -0,0 +1,145 @@ +// Config validation is invariant #8's teeth: connectors exist ONLY here, unknown fields +// are rejected, and every `env` value must be a process-env var NAME (a reference), not +// a literal secret. A rejected value is never echoed back. + +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { loadConnectorsConfig, parseConnectorsConfig } from "./config.js"; +import { BrokerConfigError } from "./errors.js"; + +describe("parseConnectorsConfig", () => { + it("accepts a valid stdio + http config", () => { + const config = parseConnectorsConfig({ + connectors: [ + { name: "office", transport: "stdio", command: "office-cli", args: ["--mcp"] }, + { + name: "pipedream", + transport: "http", + url: "https://mcp.example.com/mcp", + headers: { Authorization: "Bearer ${PD_TOKEN}" }, + env: { PD_TOKEN: "OFFICE_PD_TOKEN" }, + }, + ], + }); + expect(config.connectors).toHaveLength(2); + expect(config.connectors[0]?.command).toBe("office-cli"); + expect(config.connectors[1]?.url).toBe("https://mcp.example.com/mcp"); + }); + + it("rejects an unknown field on a connector", () => { + expect(() => + parseConnectorsConfig({ + connectors: [{ name: "x", transport: "stdio", command: "c", secret: "oops" }], + }), + ).toThrow(/unknown field "secret"/); + }); + + it("rejects an unknown top-level field", () => { + expect(() => parseConnectorsConfig({ connectors: [], extra: 1 })).toThrow( + /unknown top-level field "extra"/, + ); + }); + + it("rejects a literal-looking secret in env (not an env-var reference)", () => { + const literal = "sk-ant-super-secret-value-123"; + let thrown: unknown; + try { + parseConnectorsConfig({ + connectors: [{ name: "x", transport: "stdio", command: "c", env: { TOKEN: literal } }], + }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(BrokerConfigError); + // The offending value must NOT be echoed back into the error message. + expect((thrown as Error).message).not.toContain(literal); + }); + + it("accepts an env value that IS a valid env-var name reference", () => { + const config = parseConnectorsConfig({ + connectors: [{ name: "x", transport: "stdio", command: "c", env: { TOKEN: "OFFICE_TOKEN" } }], + }); + expect(config.connectors[0]?.env).toEqual({ TOKEN: "OFFICE_TOKEN" }); + }); + + it("rejects stdio without a command and http without a url", () => { + expect(() => + parseConnectorsConfig({ connectors: [{ name: "x", transport: "stdio" }] }), + ).toThrow(/requires a "command"/); + expect(() => parseConnectorsConfig({ connectors: [{ name: "y", transport: "http" }] })).toThrow( + /requires a "url"/, + ); + }); + + it("rejects url on stdio and command on http (transport mismatch)", () => { + expect(() => + parseConnectorsConfig({ + connectors: [{ name: "x", transport: "stdio", command: "c", url: "https://x" }], + }), + ).toThrow(/"url" is not valid for a stdio/); + expect(() => + parseConnectorsConfig({ + connectors: [{ name: "y", transport: "http", url: "https://x", command: "c" }], + }), + ).toThrow(/"command"\/"args" are not valid for an http/); + }); + + it("rejects a bad transport, a bad name, and duplicate names", () => { + expect(() => + parseConnectorsConfig({ connectors: [{ name: "x", transport: "carrier-pigeon" }] }), + ).toThrow(/transport must be/); + expect(() => + parseConnectorsConfig({ + connectors: [{ name: "has space", transport: "stdio", command: "c" }], + }), + ).toThrow(/name must match/); + expect(() => + parseConnectorsConfig({ + connectors: [ + { name: "dup", transport: "stdio", command: "a" }, + { name: "dup", transport: "stdio", command: "b" }, + ], + }), + ).toThrow(/duplicate connector name "dup"/); + }); + + it("rejects an invalid http url", () => { + expect(() => + parseConnectorsConfig({ connectors: [{ name: "x", transport: "http", url: "not a url" }] }), + ).toThrow(/is not a valid URL/); + }); +}); + +describe("loadConnectorsConfig", () => { + let dir: string; + beforeAll(async () => { + dir = await mkdtemp(join(tmpdir(), "bs-broker-cfg-")); + }); + afterAll(async () => { + await rm(dir, { recursive: true, force: true }); + }); + + it("loads and validates a file", async () => { + const path = join(dir, "boardstate.connectors.json"); + await writeFile( + path, + JSON.stringify({ connectors: [{ name: "office", transport: "stdio", command: "c" }] }), + ); + const config = await loadConnectorsConfig(path); + expect(config.connectors[0]?.name).toBe("office"); + }); + + it("throws BrokerConfigError on a missing file", async () => { + await expect(loadConnectorsConfig(join(dir, "nope.json"))).rejects.toBeInstanceOf( + BrokerConfigError, + ); + }); + + it("throws BrokerConfigError on malformed JSON", async () => { + const path = join(dir, "bad.json"); + await writeFile(path, "{ not json"); + await expect(loadConnectorsConfig(path)).rejects.toThrow(/not valid JSON/); + }); +}); diff --git a/packages/broker/src/config.ts b/packages/broker/src/config.ts new file mode 100644 index 0000000..29d07f8 --- /dev/null +++ b/packages/broker/src/config.ts @@ -0,0 +1,205 @@ +// The operator-authored connectors config (epic invariant #8). Connectors exist ONLY +// because they are named in this object — a `boardstate.connectors.json` file an +// operator writes and controls. A connector name that appears in a doc, a prompt, or a +// model's output but NOT here is inert: `McpBroker` refuses to resolve it. +// +// ── env-ref semantics (load-bearing) ──────────────────────────────────────────────── +// `env` values are the NAMES of process env vars, never the secret values themselves: +// +// "env": { "SLACK_TOKEN": "OFFICE_SLACK_TOKEN" } +// +// means "forward the value of process.env.OFFICE_SLACK_TOKEN as the child's SLACK_TOKEN" +// (stdio) or "resolve process.env.OFFICE_SLACK_TOKEN when building this connector's HTTP +// headers". The config file therefore holds no secrets and is safe to commit. +// +// We validate that every env value is a syntactically valid env-var reference +// (`^[A-Za-z_][A-Za-z0-9_]*$`). This is a fail-safe, not a secret detector: a literal +// that merely LOOKS like an identifier can't be told apart from a real reference, so we +// document the contract and reject everything that is obviously not a reference (spaces, +// `-`, `.`, `/`, `=`, `+`, over-long strings — the shapes real tokens take). Resolution +// happens later, in the broker, and a resolved value is never echoed into an error/log. + +import { readFile } from "node:fs/promises"; +import { BrokerConfigError } from "./errors.js"; + +export type ConnectorTransport = "stdio" | "http"; + +/** One operator-declared connector. `stdio` needs `command`; `http` needs `url`. */ +export type ConnectorConfig = { + /** Short, stable namespace prefix — the `connector` half of every `connector:tool` id. */ + name: string; + transport: ConnectorTransport; + /** stdio: the executable to spawn (e.g. `npx`, an absolute path). */ + command?: string; + /** stdio: argv for `command`. */ + args?: string[]; + /** http: the MCP endpoint URL (Streamable HTTP, with SSE fallback). */ + url?: string; + /** http: static request headers. Values may embed `${ENV_NAME}` refs (resolved later). */ + headers?: Record; + /** + * env-var REFERENCES, never literals. stdio: `{ CHILD_VAR: SOURCE_ENV_NAME }` forwards + * `process.env.SOURCE_ENV_NAME` into the child as `CHILD_VAR`. http: same names are + * available to header `${…}` interpolation. + */ + env?: Record; +}; + +export type ConnectorsConfig = { + connectors: ConnectorConfig[]; +}; + +/** Fields legal on a connector entry — anything else is a typo or an injection attempt. */ +const ALLOWED_KEYS = new Set(["name", "transport", "command", "args", "url", "headers", "env"]); + +/** A syntactically valid POSIX-ish env-var name (what an `env` value must be). */ +const ENV_REF_PATTERN = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** Same short namespace charset the server's connector uses (connector.ts:87). */ +const CONNECTOR_NAME_PATTERN = /^[A-Za-z0-9._-]{1,64}$/; + +function isPlainObject(value: unknown): value is Record { + return typeof value === "object" && value !== null && !Array.isArray(value); +} + +/** Validate one env map: every value must be an env-var reference, not a literal secret. */ +function validateEnvRefs(name: string, env: unknown): Record { + if (!isPlainObject(env)) { + throw new BrokerConfigError(`connector "${name}": env must be an object of NAME->ENV_REF`); + } + const out: Record = {}; + for (const [key, value] of Object.entries(env)) { + if (typeof value !== "string" || !ENV_REF_PATTERN.test(value)) { + // Deliberately do NOT echo `value` — if it is a mispasted secret, keep it out of logs. + throw new BrokerConfigError( + `connector "${name}": env["${key}"] must be a process-env var NAME ` + + `(matching /^[A-Za-z_][A-Za-z0-9_]*$/), not a literal value`, + ); + } + out[key] = value; + } + return out; +} + +/** Validate + normalize one raw connector entry into a `ConnectorConfig`. */ +function parseConnector(raw: unknown, index: number): ConnectorConfig { + if (!isPlainObject(raw)) { + throw new BrokerConfigError(`connectors[${index}] must be an object`); + } + for (const key of Object.keys(raw)) { + if (!ALLOWED_KEYS.has(key)) { + throw new BrokerConfigError(`connectors[${index}]: unknown field "${key}"`); + } + } + + const name = raw.name; + if (typeof name !== "string" || !CONNECTOR_NAME_PATTERN.test(name)) { + throw new BrokerConfigError(`connectors[${index}]: name must match /^[A-Za-z0-9._-]{1,64}$/`); + } + + const transport = raw.transport; + if (transport !== "stdio" && transport !== "http") { + throw new BrokerConfigError(`connector "${name}": transport must be "stdio" or "http"`); + } + + const config: ConnectorConfig = { name, transport }; + + if (transport === "stdio") { + if (typeof raw.command !== "string" || raw.command.length === 0) { + throw new BrokerConfigError(`connector "${name}": stdio transport requires a "command"`); + } + config.command = raw.command; + if (raw.args !== undefined) { + if (!Array.isArray(raw.args) || raw.args.some((a) => typeof a !== "string")) { + throw new BrokerConfigError(`connector "${name}": args must be an array of strings`); + } + config.args = raw.args as string[]; + } + if (raw.url !== undefined) { + throw new BrokerConfigError(`connector "${name}": "url" is not valid for a stdio transport`); + } + } else { + if (typeof raw.url !== "string" || raw.url.length === 0) { + throw new BrokerConfigError(`connector "${name}": http transport requires a "url"`); + } + try { + // eslint-disable-next-line no-new + new URL(raw.url); + } catch { + throw new BrokerConfigError(`connector "${name}": url "${raw.url}" is not a valid URL`); + } + config.url = raw.url; + if (raw.command !== undefined || raw.args !== undefined) { + throw new BrokerConfigError( + `connector "${name}": "command"/"args" are not valid for an http transport`, + ); + } + if (raw.headers !== undefined) { + if ( + !isPlainObject(raw.headers) || + Object.values(raw.headers).some((v) => typeof v !== "string") + ) { + throw new BrokerConfigError(`connector "${name}": headers must be a string map`); + } + config.headers = raw.headers as Record; + } + } + + if (raw.env !== undefined) { + config.env = validateEnvRefs(name, raw.env); + } + + return config; +} + +/** + * Validate a raw (JSON-parsed) connectors config. Rejects unknown fields, bad + * transports, duplicate connector names, and any `env` value that is not an env-var + * reference. Returns a normalized {@link ConnectorsConfig}. + */ +export function parseConnectorsConfig(raw: unknown): ConnectorsConfig { + if (!isPlainObject(raw)) { + throw new BrokerConfigError("connectors config must be a JSON object"); + } + for (const key of Object.keys(raw)) { + if (key !== "connectors") { + throw new BrokerConfigError(`unknown top-level field "${key}" (expected only "connectors")`); + } + } + if (!Array.isArray(raw.connectors)) { + throw new BrokerConfigError('config must have a "connectors" array'); + } + const connectors = raw.connectors.map((entry, index) => parseConnector(entry, index)); + const seen = new Set(); + for (const connector of connectors) { + if (seen.has(connector.name)) { + throw new BrokerConfigError(`duplicate connector name "${connector.name}"`); + } + seen.add(connector.name); + } + return { connectors }; +} + +/** + * Load + validate an operator-authored connectors config file (`boardstate.connectors.json`). + * Throws {@link BrokerConfigError} on a missing/malformed file or any validation failure. + */ +export async function loadConnectorsConfig(path: string): Promise { + let text: string; + try { + text = await readFile(path, "utf8"); + } catch (error) { + throw new BrokerConfigError( + `cannot read connectors config at "${path}": ${(error as Error).message}`, + ); + } + let raw: unknown; + try { + raw = JSON.parse(text); + } catch (error) { + throw new BrokerConfigError( + `connectors config at "${path}" is not valid JSON: ${(error as Error).message}`, + ); + } + return parseConnectorsConfig(raw); +} diff --git a/packages/broker/src/errors.ts b/packages/broker/src/errors.ts new file mode 100644 index 0000000..843c95d --- /dev/null +++ b/packages/broker/src/errors.ts @@ -0,0 +1,74 @@ +// Typed errors for the broker. Every failure the host/server layers can act on is a +// named subclass of `BrokerError` so callers switch on the class (or `.code`) instead +// of string-matching messages. +// +// SECURITY: broker errors must NEVER carry a resolved env-var VALUE (a forwarded +// secret) — only the env-var NAME/reference. The config layer forbids literals up +// front; these error shapes keep the discipline downstream (no value fields). + +/** Base for every broker-originated error. `code` is a stable machine tag. */ +export class BrokerError extends Error { + readonly code: string; + constructor(code: string, message: string) { + super(message); + this.name = new.target.name; + this.code = code; + } +} + +/** A connectors config that failed validation (unknown field, bad transport, non-ref env). */ +export class BrokerConfigError extends BrokerError { + constructor(message: string) { + super("broker_config_invalid", message); + } +} + +/** A tool name (manifest id or provider-safe name) that overflows the 64-char budget. */ +export class BrokerBudgetError extends BrokerError { + constructor(message: string) { + super("broker_name_budget", message); + } +} + +/** Two tools collapsing onto the same provider-safe name after sanitization. */ +export class BrokerNameCollisionError extends BrokerError { + constructor(message: string) { + super("broker_name_collision", message); + } +} + +/** A connector name referenced at call time that is not in the operator config. */ +export class BrokerUnknownConnectorError extends BrokerError { + constructor(message: string) { + super("broker_unknown_connector", message); + } +} + +/** Transport / handshake failure connecting to an external MCP server. */ +export class BrokerConnectError extends BrokerError { + constructor(message: string, options?: { cause?: unknown }) { + super("broker_connect_failed", message); + if (options?.cause !== undefined) { + this.cause = options.cause; + } + } +} + +/** A `callTool` that exceeded its hard timeout. */ +export class BrokerTimeoutError extends BrokerError { + constructor(message: string) { + super("broker_tool_timeout", message); + } +} + +/** + * A tool call the server answered with `isError: true`. The normalized message is the + * server's own text payload; `toolId` is the namespaced id that was called. + */ +export class BrokerToolError extends BrokerError { + readonly toolId: string; + constructor(toolId: string, message: string) { + super("broker_tool_error", message); + this.toolId = toolId; + } +} diff --git a/packages/broker/src/fixture/fake-mcp-server.ts b/packages/broker/src/fixture/fake-mcp-server.ts new file mode 100644 index 0000000..094667c --- /dev/null +++ b/packages/broker/src/fixture/fake-mcp-server.ts @@ -0,0 +1,157 @@ +// A fake MCP SERVER used only by the broker's tests. It is deliberately tiny but +// exercises every broker code path: a read-only tool, a mutating tool with NO +// `readOnlyHint` (so the broker's fail-safe treats it as a mutation), an `isError` +// tool, and a `sleep` tool for timeout tests. It can be driven two ways with the SAME +// catalog — as a stdio child process (`stdio-entry.ts`) and in-process over HTTP +// (`startHttpFakeServer`) — so the wire-contract test hits both transports with no +// network beyond loopback. `state.mutated` flips the catalog (adds a tool AND changes a +// schema) to drive the anti-rug-pull manifest-hash tests. +// +// Built with the SDK's low-level `Server` + request-handler idiom (mirrors +// packages/mcp/src/mcp-server.ts), not the app store — this is a pure test double. + +import { Server } from "@modelcontextprotocol/sdk/server/index.js"; +import { CallToolRequestSchema, ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js"; + +export const FAKE_SERVER_NAME = "fake-mcp"; +export const FAKE_SERVER_VERSION = "0.0.0"; + +/** Runtime toggle for rug-pull tests: flip `mutated` to change the advertised catalog. */ +export type FakeCatalogState = { mutated: boolean }; + +type FakeTool = { + name: string; + description: string; + inputSchema: Record; + readOnlyHint?: boolean; +}; + +/** The advertised catalog. When `mutated`, `add` gains a `c` operand and `extra` appears. */ +function catalog(state: FakeCatalogState): FakeTool[] { + const tools: FakeTool[] = [ + { + name: "echo", + description: "Echo the input text back.", + inputSchema: { + type: "object", + additionalProperties: false, + required: ["text"], + properties: { text: { type: "string" } }, + }, + readOnlyHint: true, + }, + { + name: "add", + description: "Add two (or, when mutated, three) numbers.", + inputSchema: state.mutated + ? { + type: "object", + additionalProperties: false, + required: ["a", "b", "c"], + properties: { a: { type: "number" }, b: { type: "number" }, c: { type: "number" } }, + } + : { + type: "object", + additionalProperties: false, + required: ["a", "b"], + properties: { a: { type: "number" }, b: { type: "number" } }, + }, + readOnlyHint: true, + }, + { + // NO readOnlyHint on purpose: the broker must treat it as a mutation (fail-safe). + name: "write_note", + description: "Pretend to persist a note (mutating).", + inputSchema: { + type: "object", + additionalProperties: false, + required: ["text"], + properties: { text: { type: "string" } }, + }, + }, + { + name: "boom", + description: "Always answers with isError:true.", + inputSchema: { type: "object", additionalProperties: false, properties: {} }, + readOnlyHint: true, + }, + { + name: "sleep", + description: "Resolve after `ms` milliseconds (drives timeout tests).", + inputSchema: { + type: "object", + additionalProperties: false, + required: ["ms"], + properties: { ms: { type: "number" } }, + }, + readOnlyHint: true, + }, + ]; + if (state.mutated) { + tools.push({ + name: "extra", + description: "Only present when the catalog is mutated.", + inputSchema: { type: "object", additionalProperties: false, properties: {} }, + readOnlyHint: true, + }); + } + return tools; +} + +function textResult(details: unknown, isError = false) { + return { + content: [{ type: "text" as const, text: JSON.stringify(details) }], + ...(isError ? { isError: true } : {}), + }; +} + +const sleep = (ms: number): Promise => new Promise((resolve) => setTimeout(resolve, ms)); + +/** + * Build a fresh fake MCP `Server` over the given (possibly shared) catalog state. Not + * yet connected — hand it a transport (`StdioServerTransport` in the child, + * `StreamableHTTPServerTransport` in-process). + */ +export function buildFakeMcpServer(state: FakeCatalogState = { mutated: false }): Server { + const server = new Server( + { name: FAKE_SERVER_NAME, version: FAKE_SERVER_VERSION }, + { capabilities: { tools: {} } }, + ); + + server.setRequestHandler(ListToolsRequestSchema, async () => ({ + tools: catalog(state).map((tool) => ({ + name: tool.name, + description: tool.description, + inputSchema: tool.inputSchema, + // Only emit annotations when the hint is set, so `write_note` arrives hint-less + // and the broker's fail-safe (absent ⇒ mutation) is genuinely exercised. + ...(tool.readOnlyHint ? { annotations: { readOnlyHint: true } } : {}), + })), + })); + + server.setRequestHandler(CallToolRequestSchema, async (request) => { + const name = request.params.name; + const args = (request.params.arguments ?? {}) as Record; + switch (name) { + case "echo": + return textResult({ text: args.text }); + case "add": { + const sum = Number(args.a) + Number(args.b) + (state.mutated ? Number(args.c) : 0); + return textResult({ sum }); + } + case "write_note": + return textResult({ ok: true, saved: args.text }); + case "boom": + return textResult({ error: "boom: this tool always fails" }, true); + case "sleep": + await sleep(Number(args.ms) || 0); + return textResult({ slept: Number(args.ms) || 0 }); + case "extra": + return textResult({ ok: true }); + default: + return textResult({ error: `unknown tool: ${name}` }, true); + } + }); + + return server; +} diff --git a/packages/broker/src/fixture/http-harness.ts b/packages/broker/src/fixture/http-harness.ts new file mode 100644 index 0000000..941a9f6 --- /dev/null +++ b/packages/broker/src/fixture/http-harness.ts @@ -0,0 +1,70 @@ +// Run the fake MCP server in-process over Streamable HTTP on an ephemeral loopback port +// (no external network — CI-safe). Returns the URL to point an http connector at, the +// shared catalog `state` (flip `.mutated` to drive rug-pull tests), and a `close`. +// +// Stateless mode per the SDK's own example (examples/server/simpleStatelessStreamableHttp): +// each POST gets a FRESH server + transport built over the SHARED `state`, so a mutated +// catalog is observed on the next request; GET/DELETE answer 405 (no server-push stream). + +import { createServer, type Server as HttpServer } from "node:http"; +import type { AddressInfo } from "node:net"; +import { StreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/streamableHttp.js"; +import { buildFakeMcpServer, type FakeCatalogState } from "./fake-mcp-server.js"; + +export type HttpFakeServer = { + /** The MCP endpoint to hand an http connector (`http://127.0.0.1:/mcp`). */ + url: string; + /** Shared catalog state — flip `.mutated` between `listTools()` calls for rug-pull tests. */ + state: FakeCatalogState; + close: () => Promise; +}; + +const METHOD_NOT_ALLOWED = JSON.stringify({ + jsonrpc: "2.0", + error: { code: -32000, message: "Method not allowed." }, + id: null, +}); + +/** Start the fake server over Streamable HTTP (stateless JSON mode) on 127.0.0.1. */ +export async function startHttpFakeServer( + initial: FakeCatalogState = { mutated: false }, +): Promise { + const state = initial; + + const http: HttpServer = createServer((req, res) => { + if (req.method !== "POST") { + res.writeHead(405).end(METHOD_NOT_ALLOWED); + return; + } + // Fresh server + transport per request, over the shared catalog state. + const mcp = buildFakeMcpServer(state); + const transport = new StreamableHTTPServerTransport({ + sessionIdGenerator: undefined, + enableJsonResponse: true, + }); + res.on("close", () => { + void transport.close().catch(() => {}); + void mcp.close().catch(() => {}); + }); + void mcp + .connect(transport) + .then(() => transport.handleRequest(req, res)) + .catch(() => { + if (!res.headersSent) { + res.statusCode = 500; + res.end(); + } + }); + }); + + await new Promise((resolve) => http.listen(0, "127.0.0.1", resolve)); + const { port } = http.address() as AddressInfo; + + return { + url: `http://127.0.0.1:${port}/mcp`, + state, + close: async () => { + await new Promise((resolve) => http.close(() => resolve())); + }, + }; +} diff --git a/packages/broker/src/fixture/stdio-entry.ts b/packages/broker/src/fixture/stdio-entry.ts new file mode 100644 index 0000000..159d7a3 --- /dev/null +++ b/packages/broker/src/fixture/stdio-entry.ts @@ -0,0 +1,20 @@ +#!/usr/bin/env node +// Runnable stdio entry for the fake MCP server — the `command` a stdio connector spawns +// in the broker's wire-contract test (and a handy manual `boardstate-fake-mcp` bin). +// `FAKE_MCP_MUTATED=1` seeds the mutated catalog so a stdio child can serve the +// post-rug-pull tool set. No network; speaks MCP over stdin/stdout only. + +import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js"; +import { buildFakeMcpServer } from "./fake-mcp-server.js"; + +async function main(): Promise { + const server = buildFakeMcpServer({ mutated: process.env.FAKE_MCP_MUTATED === "1" }); + const transport = new StdioServerTransport(); + await server.connect(transport); +} + +main().catch((error) => { + // stderr only — stdout is the MCP wire and must not be polluted. + process.stderr.write(`fake-mcp failed to start: ${(error as Error).message}\n`); + process.exit(1); +}); diff --git a/packages/broker/src/index.ts b/packages/broker/src/index.ts new file mode 100644 index 0000000..64a83f2 --- /dev/null +++ b/packages/broker/src/index.ts @@ -0,0 +1,42 @@ +// @boardstate/broker — the MCP CLIENT manager (epic #37, M5a-1). +// +// Public surface: build an `McpBroker` from an operator-authored connectors config +// (file via `loadConnectorsConfig`, or object via the constructor), then `listTools()` +// to discover a namespaced `ToolManifest` and `callTool()` to invoke a tool behind its +// `connector:tool` id or provider-safe `connector__tool` name. + +export { McpBroker } from "./broker.js"; +export type { BrokerOptions } from "./broker.js"; + +export { loadConnectorsConfig, parseConnectorsConfig } from "./config.js"; +export type { ConnectorConfig, ConnectorsConfig, ConnectorTransport } from "./config.js"; + +export { buildManifest, manifestHash } from "./manifest.js"; +export type { + DiscoveredTool, + ToolAnnotations, + ToolManifest, + ToolManifestEntry, +} from "./manifest.js"; + +export { + buildProviderNameMap, + manifestId, + parseManifestId, + toProviderName, + MANIFEST_ID_SEPARATOR, + NAME_BUDGET, + PROVIDER_NAME_PATTERN, + PROVIDER_NAME_SEPARATOR, +} from "./names.js"; + +export { + BrokerBudgetError, + BrokerConfigError, + BrokerConnectError, + BrokerError, + BrokerNameCollisionError, + BrokerTimeoutError, + BrokerToolError, + BrokerUnknownConnectorError, +} from "./errors.js"; diff --git a/packages/broker/src/manifest.test.ts b/packages/broker/src/manifest.test.ts new file mode 100644 index 0000000..dcb778c --- /dev/null +++ b/packages/broker/src/manifest.test.ts @@ -0,0 +1,109 @@ +// The manifest hash is the anti-rug-pull anchor: stable across runs and key order, +// moves iff the callable surface (tool set or an input schema) changes, indifferent to +// description churn. `readOnlyHint` absent ⇒ mutation (fail-safe). + +import { describe, expect, it } from "vitest"; +import { buildManifest, manifestHash, type DiscoveredTool } from "./manifest.js"; + +function tools(overrides: Partial[] = []): Map { + const base: DiscoveredTool[] = [ + { + name: "list_files", + description: "List files.", + inputSchema: { type: "object", properties: { path: { type: "string" } } }, + annotations: { readOnlyHint: true }, + }, + { + name: "write_file", + description: "Write a file.", + inputSchema: { type: "object", properties: { path: { type: "string" } } }, + // No annotations: must be treated as a mutation. + }, + ...overrides.map((o) => ({ name: "x", inputSchema: {}, ...o }) as DiscoveredTool), + ]; + return new Map([["office", base]]); +} + +describe("buildManifest", () => { + it("namespaces every tool and honors readOnlyHint fail-safe", () => { + const manifest = buildManifest(tools()); + const byId = new Map(manifest.tools.map((t) => [t.id, t])); + expect(byId.get("office:list_files")?.readOnly).toBe(true); + // Absent readOnlyHint ⇒ mutation. + expect(byId.get("office:write_file")?.readOnly).toBe(false); + expect(byId.get("office:list_files")?.providerName).toBe("office__list_files"); + expect(manifest.providerToId.get("office__write_file")).toBe("office:write_file"); + }); + + it("is deterministic and independent of tool discovery order", () => { + const a = buildManifest(tools()); + const reversed = new Map([["office", [...tools().get("office")!].reverse()]]); + const b = buildManifest(reversed); + expect(a.hash).toBe(b.hash); + }); + + it("hash is stable across key-order differences in a schema", () => { + const s1: DiscoveredTool = { + name: "t", + inputSchema: { type: "object", required: ["a"], properties: { a: { type: "string" } } }, + }; + const s2: DiscoveredTool = { + name: "t", + inputSchema: { properties: { a: { type: "string" } }, required: ["a"], type: "object" }, + }; + expect(manifestHash(buildManifest(new Map([["c", [s1]]])).tools)).toBe( + manifestHash(buildManifest(new Map([["c", [s2]]])).tools), + ); + }); + + it("hash does NOT move on description-only changes", () => { + const before = buildManifest(tools()); + const withNewDesc = new Map([ + [ + "office", + tools() + .get("office")! + .map((t) => ({ ...t, description: `${t.description ?? ""} (edited)` })), + ], + ]); + expect(buildManifest(withNewDesc).hash).toBe(before.hash); + }); + + it("hash MOVES when a tool is added, removed, renamed, or a schema changes", () => { + const base = buildManifest(tools()); + + const added = buildManifest( + new Map([["office", [...tools().get("office")!, { name: "new_tool", inputSchema: {} }]]]), + ); + expect(added.hash).not.toBe(base.hash); + + const removed = buildManifest(new Map([["office", [tools().get("office")![0]!]]])); + expect(removed.hash).not.toBe(base.hash); + + const renamed = buildManifest( + new Map([ + [ + "office", + tools() + .get("office")! + .map((t, i) => (i === 0 ? { ...t, name: "ls" } : t)), + ], + ]), + ); + expect(renamed.hash).not.toBe(base.hash); + + const schemaChanged = buildManifest( + new Map([ + [ + "office", + tools() + .get("office")! + .map((t, i) => + i === 0 ? { ...t, inputSchema: { type: "object", properties: {} } } : t, + ), + ], + ]), + ); + expect(schemaChanged.hash).not.toBe(base.hash); + }); +}); diff --git a/packages/broker/src/manifest.ts b/packages/broker/src/manifest.ts new file mode 100644 index 0000000..da46113 --- /dev/null +++ b/packages/broker/src/manifest.ts @@ -0,0 +1,115 @@ +// The `ToolManifest`: the broker's snapshot of every tool across every connected +// connector, plus a stable hash over it. The hash is the anti-rug-pull anchor the grant +// lifecycle (M5b-2) pins a grant to — if a server later adds/removes/renames a tool or +// changes a tool's input schema, the hash changes and the grant must be re-approved. +// +// readOnlyHint honored, fail-safe: a tool with `readOnlyHint: true` is `readOnly: true`; +// ABSENT (or false) ⇒ `readOnly: false`, i.e. treated as a mutation. This mirrors the +// AgentTool convention in packages/server/src/host.ts:42-54 (`readOnly?` absent ⇒ treat +// as a mutation) so the two layers can never disagree on what is safe to run. + +import { createHash } from "node:crypto"; +import type { AgentTool } from "@boardstate/server"; +import { buildProviderNameMap, manifestId } from "./names.js"; + +/** A discovered MCP tool annotation subset the broker cares about. */ +export type ToolAnnotations = { + readOnlyHint?: boolean; +}; + +/** The raw tool shape the SDK's `listTools()` returns (the fields we consume). */ +export type DiscoveredTool = { + name: string; + description?: string; + inputSchema: Record; + annotations?: ToolAnnotations; +}; + +export type ToolManifestEntry = { + /** Internal `connector:tool` id. */ + id: string; + /** Provider-safe `connector__tool` name (LLM function-name charset). */ + providerName: string; + /** The owning connector's config name. */ + connector: string; + /** The tool's raw name on its server (namespace stripped). */ + tool: string; + description?: string; + inputSchema: Record; + /** + * True only when the server set `readOnlyHint: true`. Absent hint ⇒ false (mutation) — + * the fail-safe default. Type-aligned with {@link AgentTool.readOnly}. + */ + readOnly: AgentTool["readOnly"]; +}; + +export type ToolManifest = { + /** Every discovered tool, sorted by manifest id for determinism. */ + tools: ToolManifestEntry[]; + /** Stable sha256 over the sorted (id + canonical input schema) pairs. */ + hash: string; + /** manifest id -> provider-safe name. */ + idToProvider: Map; + /** provider-safe name -> manifest id (the reverse lookup the M5c-1 adapter needs). */ + providerToId: Map; +}; + +/** Recursively sort object keys so semantically equal schemas serialize identically. */ +function canonicalize(value: unknown): unknown { + if (Array.isArray(value)) { + return value.map(canonicalize); + } + if (value !== null && typeof value === "object") { + const out: Record = {}; + for (const key of Object.keys(value as Record).sort()) { + out[key] = canonicalize((value as Record)[key]); + } + return out; + } + return value; +} + +/** + * Hash the sorted (manifest id + canonical input schema) pairs. Deterministic across + * runs and process boundaries; changes iff a tool is added, removed, renamed, or its + * input schema changes. Description/annotation churn deliberately does NOT move the hash + * — the grant cares about the callable surface, not the prose. + */ +export function manifestHash(entries: readonly ToolManifestEntry[]): string { + const pairs = entries + .map((entry) => [entry.id, canonicalize(entry.inputSchema)] as const) + .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)); + return createHash("sha256").update(JSON.stringify(pairs)).digest("hex"); +} + +/** + * Assemble a {@link ToolManifest} from each connector's discovered tools. Enforces the + * 64-char budget on both name forms and fails loud on a provider-name collision (both + * via {@link manifestId} / {@link buildProviderNameMap}). `discovered` maps a connector + * name to the tools its server returned. + */ +export function buildManifest(discovered: Map): ToolManifest { + const entries: ToolManifestEntry[] = []; + for (const [connector, tools] of discovered) { + for (const tool of tools) { + entries.push({ + id: manifestId(connector, tool.name), + // Filled in once the id set is known (buildProviderNameMap detects collisions). + providerName: "", + connector, + tool: tool.name, + ...(tool.description !== undefined ? { description: tool.description } : {}), + inputSchema: tool.inputSchema, + readOnly: tool.annotations?.readOnlyHint === true, + }); + } + } + entries.sort((a, b) => (a.id < b.id ? -1 : a.id > b.id ? 1 : 0)); + + const { idToProvider, providerToId } = buildProviderNameMap(entries.map((entry) => entry.id)); + for (const entry of entries) { + entry.providerName = idToProvider.get(entry.id) ?? ""; + } + + return { tools: entries, hash: manifestHash(entries), idToProvider, providerToId }; +} diff --git a/packages/broker/src/names.test.ts b/packages/broker/src/names.test.ts new file mode 100644 index 0000000..7e6eabb --- /dev/null +++ b/packages/broker/src/names.test.ts @@ -0,0 +1,57 @@ +// Namespacing has two jobs: keep both name forms inside the 64-char budget, and never +// let two tools collapse onto one provider-safe name. + +import { describe, expect, it } from "vitest"; +import { BrokerBudgetError, BrokerNameCollisionError } from "./errors.js"; +import { + buildProviderNameMap, + manifestId, + parseManifestId, + PROVIDER_NAME_PATTERN, + toProviderName, +} from "./names.js"; + +describe("manifest ids + provider names", () => { + it("builds and parses connector:tool ids", () => { + const id = manifestId("office", "list_files"); + expect(id).toBe("office:list_files"); + expect(parseManifestId(id)).toEqual({ connector: "office", tool: "list_files" }); + }); + + it("splits on the FIRST colon (tool names may be plain, connector never has one)", () => { + expect(parseManifestId("office:weird:tool")).toEqual({ + connector: "office", + tool: "weird:tool", + }); + }); + + it("enforces the 64-char budget on the manifest id", () => { + const longTool = "t".repeat(70); + expect(() => manifestId("office", longTool)).toThrow(BrokerBudgetError); + }); + + it("sanitizes into the provider charset and stays legal", () => { + const name = toProviderName("slack.io", "send-message"); + expect(name).toBe("slack_io__send-message"); + expect(PROVIDER_NAME_PATTERN.test(name)).toBe(true); + }); + + it("enforces the 64-char budget on the provider name", () => { + expect(() => toProviderName("c".repeat(40), "t".repeat(40))).toThrow(BrokerBudgetError); + }); + + it("maps ids to provider names and back", () => { + const { idToProvider, providerToId } = buildProviderNameMap([ + "office:list_files", + "slack:send", + ]); + expect(idToProvider.get("office:list_files")).toBe("office__list_files"); + expect(providerToId.get("office__list_files")).toBe("office:list_files"); + expect(providerToId.get("slack__send")).toBe("slack:send"); + }); + + it("fails loud when two ids collapse onto one provider name", () => { + // `x.y` and `x_y` both sanitize to `x_y` (`.` and `_` are out-of-charset → `_`). + expect(() => buildProviderNameMap(["x.y:send", "x_y:send"])).toThrow(BrokerNameCollisionError); + }); +}); diff --git a/packages/broker/src/names.ts b/packages/broker/src/names.ts new file mode 100644 index 0000000..c0f6f3e --- /dev/null +++ b/packages/broker/src/names.ts @@ -0,0 +1,99 @@ +// Two names for every discovered tool, both budget-capped at 64 chars: +// +// manifest id `connector:tool` — the broker's internal, human-readable handle. +// `:` is deliberate: it can never collide with a +// raw tool name and marks the namespace boundary. +// provider name `connector__tool` — a name legal in an LLM provider's tool-name +// charset `^[A-Za-z0-9_-]{1,64}$` (no `:`). This +// is what the M5c-1 adapter (#42) hands the model; +// exporting it HERE stops that adapter from +// inventing its own scheme. +// +// Sanitizing `connector`/`tool` into the provider charset is lossy (a `.` and a `-` +// both become `_`), so the provider name is NOT reversible by string surgery. The +// broker instead keeps an explicit provider-name -> manifest-id map, built once per +// manifest, and fails LOUD on any collision — cf. the reverse-map discipline in +// `toAgentToolName` (packages/mcp/src/mcp-server.ts:77), inverted for the outbound side. + +import { BrokerBudgetError, BrokerNameCollisionError } from "./errors.js"; + +/** Provider tool-name charset (Anthropic/OpenAI function names): letters, digits, `_`, `-`. */ +export const PROVIDER_NAME_PATTERN = /^[A-Za-z0-9_-]{1,64}$/; + +/** Separator between namespace and tool in the internal manifest id. */ +export const MANIFEST_ID_SEPARATOR = ":"; + +/** Separator between namespace and tool in the provider-safe name. */ +export const PROVIDER_NAME_SEPARATOR = "__"; + +/** The shared 64-char budget both names are measured against. */ +export const NAME_BUDGET = 64; + +/** Build the internal `connector:tool` manifest id, enforcing the 64-char budget. */ +export function manifestId(connector: string, tool: string): string { + const id = `${connector}${MANIFEST_ID_SEPARATOR}${tool}`; + if (id.length > NAME_BUDGET) { + throw new BrokerBudgetError( + `manifest id "${id}" is ${id.length} chars (budget ${NAME_BUDGET}); shorten the connector prefix or tool name`, + ); + } + return id; +} + +/** Split a `connector:tool` id back into its parts (first `:` wins — tool names may contain none). */ +export function parseManifestId(id: string): { connector: string; tool: string } { + const idx = id.indexOf(MANIFEST_ID_SEPARATOR); + if (idx <= 0 || idx === id.length - 1) { + throw new BrokerBudgetError(`"${id}" is not a valid connector:tool manifest id`); + } + return { connector: id.slice(0, idx), tool: id.slice(idx + 1) }; +} + +/** Replace every char outside the provider charset with `_` (lossy). */ +function sanitizeSegment(segment: string): string { + return segment.replace(/[^A-Za-z0-9-]/g, "_"); +} + +/** + * Build the provider-safe `connector__tool` name for one tool, enforcing the budget. + * Lossy per-segment sanitization means callers MUST record the (name -> id) mapping and + * detect collisions themselves — {@link buildProviderNameMap} does exactly that. + */ +export function toProviderName(connector: string, tool: string): string { + const name = `${sanitizeSegment(connector)}${PROVIDER_NAME_SEPARATOR}${sanitizeSegment(tool)}`; + if (!PROVIDER_NAME_PATTERN.test(name)) { + // Only reachable via the length bound: sanitizeSegment already coerces the charset. + throw new BrokerBudgetError( + `provider name "${name}" is ${name.length} chars (budget ${NAME_BUDGET})`, + ); + } + return name; +} + +/** + * Map every manifest id to its provider-safe name and back. Throws {@link + * BrokerNameCollisionError} the moment two distinct ids sanitize to the same provider + * name (e.g. `slack.io:send` and `slack-io:send`) — the broker will not silently route + * one tool's calls to another. + */ +export function buildProviderNameMap(ids: readonly string[]): { + idToProvider: Map; + providerToId: Map; +} { + const idToProvider = new Map(); + const providerToId = new Map(); + for (const id of ids) { + const { connector, tool } = parseManifestId(id); + const provider = toProviderName(connector, tool); + const clash = providerToId.get(provider); + if (clash !== undefined && clash !== id) { + throw new BrokerNameCollisionError( + `provider name "${provider}" is claimed by both "${clash}" and "${id}"; ` + + `give the connectors distinct provider-legal prefixes`, + ); + } + idToProvider.set(id, provider); + providerToId.set(provider, id); + } + return { idToProvider, providerToId }; +} diff --git a/packages/broker/tsconfig.json b/packages/broker/tsconfig.json new file mode 100644 index 0000000..16e8730 --- /dev/null +++ b/packages/broker/tsconfig.json @@ -0,0 +1,8 @@ +{ + "extends": "../../tsconfig.base.json", + "compilerOptions": { + "types": ["node"], + "lib": ["ES2023"] + }, + "include": ["src"] +} diff --git a/packages/broker/tsdown.config.ts b/packages/broker/tsdown.config.ts new file mode 100644 index 0000000..8868027 --- /dev/null +++ b/packages/broker/tsdown.config.ts @@ -0,0 +1,10 @@ +import { defineConfig } from "tsdown"; + +export default defineConfig({ + // The public API plus the runnable fake-server stdio entry (the `boardstate-fake-mcp` + // bin, and the child the broker's stdio wire-contract test spawns). + entry: ["src/index.ts", "src/fixture/stdio-entry.ts"], + dts: true, + format: "esm", + fixedExtension: false, +}); diff --git a/packages/broker/vitest.config.ts b/packages/broker/vitest.config.ts new file mode 100644 index 0000000..2a4d257 --- /dev/null +++ b/packages/broker/vitest.config.ts @@ -0,0 +1,5 @@ +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + test: { name: "broker", environment: "node" }, +}); From c837db16df61d56180b410907019afff37cb896c Mon Sep 17 00:00:00 2001 From: Eva Date: Fri, 10 Jul 2026 20:48:55 +0700 Subject: [PATCH 2/3] chore(broker): wire @boardstate/broker into workspace (vitest project, changeset, lockfile) --- .changeset/broker-mcp-client-manager.md | 27 +++++++++++++++++++++++++ pnpm-lock.yaml | 9 +++++++++ vitest.config.ts | 2 +- 3 files changed, 37 insertions(+), 1 deletion(-) create mode 100644 .changeset/broker-mcp-client-manager.md diff --git a/.changeset/broker-mcp-client-manager.md b/.changeset/broker-mcp-client-manager.md new file mode 100644 index 0000000..9dcab69 --- /dev/null +++ b/.changeset/broker-mcp-client-manager.md @@ -0,0 +1,27 @@ +--- +"@boardstate/broker": minor +--- + +New package `@boardstate/broker` — the MCP CLIENT manager (M5a-1, epic #37). It connects +outward to the external MCP servers an operator declares in a `boardstate.connectors.json` +config, discovers their tools into a namespaced `ToolManifest`, and calls them behind a +narrow API the host/server layers consume. + +- **Config is operator-authored only**: `loadConnectorsConfig(path)` + a programmatic + `new McpBroker(config)`. A connector name not in the config is inert. `env` values are + process-env var NAMES (references), never literal secrets — validated up front and never + echoed into errors or logs. +- **Transports**: `StdioClientTransport` for local servers, `StreamableHTTPClientTransport` + with SSE fallback for remotes. Lazy connect on first use, warm/pooled clients, capped + exponential-backoff reconnect, clean close. +- **`listTools()` → `ToolManifest`**: `connector:tool` ids AND provider-safe + `connector__tool` names (both inside a 64-char budget, collisions fail loud), input + schemas, `readOnlyHint` honored (absent ⇒ treated as a mutation — fail-safe), and a + stable manifest hash over sorted (id + canonical input-schema) pairs — the anti-rug-pull + snapshot the grant lifecycle (M5b-2) pins to. +- **`callTool(id, args, { timeout })`**: resolves the client, strips the namespace, + enforces a hard timeout, and normalizes `isError: true` results into a typed + `BrokerToolError`. + +Depends only on `@modelcontextprotocol/sdk` and `@boardstate/server` (types only). Ships +an in-repo fake MCP server fixture (stdio child + in-process HTTP) so CI needs no network. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3365423..6acd6eb 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -116,6 +116,15 @@ importers: specifier: 1.3.3 version: 1.3.3 + packages/broker: + dependencies: + '@boardstate/server': + specifier: workspace:* + version: link:../server + '@modelcontextprotocol/sdk': + specifier: ^1.29.0 + version: 1.29.0(zod@4.4.3) + packages/core: dependencies: '@boardstate/schema': diff --git a/vitest.config.ts b/vitest.config.ts index 7c72760..349b1b5 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,7 +1,7 @@ import { defineConfig } from "vitest/config"; const domPackages = ["host", "lit", "react"]; -const nodePackages = ["schema", "core", "server", "mcp", "agent"]; +const nodePackages = ["schema", "core", "server", "mcp", "agent", "broker"]; export default defineConfig({ test: { From e760eb317fb8ff21e5ff04be40a07ee09b5d4e0a Mon Sep 17 00:00:00 2001 From: Eva Date: Fri, 10 Jul 2026 21:01:34 +0700 Subject: [PATCH 3/3] =?UTF-8?q?fix(broker):=20manifest=20hash=20includes?= =?UTF-8?q?=20readOnly=20=E2=80=94=20a=20read->mutating=20flip=20must=20re?= =?UTF-8?q?-pend?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial verify refuted the anti-rug-pull claim: readOnlyHint never entered the hashed tuples, so a tool silently becoming a mutation kept its grant. The classification decides direct-execute vs pending-action downstream; it is part of the callable surface. Regression test pins the flip. --- packages/broker/src/manifest.test.ts | 17 +++++++++++++++++ packages/broker/src/manifest.ts | 17 ++++++++++------- 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/packages/broker/src/manifest.test.ts b/packages/broker/src/manifest.test.ts index dcb778c..c60eed1 100644 --- a/packages/broker/src/manifest.test.ts +++ b/packages/broker/src/manifest.test.ts @@ -69,6 +69,23 @@ describe("buildManifest", () => { expect(buildManifest(withNewDesc).hash).toBe(before.hash); }); + it("hash MOVES when a tool's readOnly classification flips (read -> mutating rug-pull)", () => { + const base = buildManifest(tools()); + const flipped = new Map([ + [ + "office", + tools() + .get("office")! + .map((t) => + t.annotations?.readOnlyHint === true + ? { ...t, annotations: { ...t.annotations, readOnlyHint: undefined } } + : t, + ), + ], + ]); + expect(buildManifest(flipped).hash).not.toBe(base.hash); + }); + it("hash MOVES when a tool is added, removed, renamed, or a schema changes", () => { const base = buildManifest(tools()); diff --git a/packages/broker/src/manifest.ts b/packages/broker/src/manifest.ts index da46113..a2558df 100644 --- a/packages/broker/src/manifest.ts +++ b/packages/broker/src/manifest.ts @@ -70,16 +70,19 @@ function canonicalize(value: unknown): unknown { } /** - * Hash the sorted (manifest id + canonical input schema) pairs. Deterministic across - * runs and process boundaries; changes iff a tool is added, removed, renamed, or its - * input schema changes. Description/annotation churn deliberately does NOT move the hash - * — the grant cares about the callable surface, not the prose. + * Hash the sorted (manifest id + canonical input schema + readOnly) tuples. + * Deterministic across runs and process boundaries; changes iff a tool is added, + * removed, renamed, its input schema changes, or its readOnly classification flips. + * readOnly MUST participate: it decides direct-execute vs pending-action downstream, + * so a read tool silently becoming a mutation is exactly the rug-pull the hash + * exists to catch. Description churn deliberately does NOT move the hash — the + * grant cares about the callable surface, not the prose. */ export function manifestHash(entries: readonly ToolManifestEntry[]): string { - const pairs = entries - .map((entry) => [entry.id, canonicalize(entry.inputSchema)] as const) + const tuples = entries + .map((entry) => [entry.id, canonicalize(entry.inputSchema), entry.readOnly] as const) .sort((a, b) => (a[0] < b[0] ? -1 : a[0] > b[0] ? 1 : 0)); - return createHash("sha256").update(JSON.stringify(pairs)).digest("hex"); + return createHash("sha256").update(JSON.stringify(tuples)).digest("hex"); } /**