From 160d6a38eb3b423aad875791930ab9cc8853483e Mon Sep 17 00:00:00 2001 From: Andre Derjagin Date: Fri, 14 Aug 2026 09:48:30 +0200 Subject: [PATCH 1/3] feat(issue-545): cache tools/list per ttlMs/cacheScope, emit CacheableResult (#545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Client (McpManager.listTools): TTL cache keyed like the connection pool — public scope under the bare server id, private/unknown/absent under the pool key (server id + token hash). Server TTLs clamped to 15 min; absent ttlMs gets a 60s default (deliberate spec deviation, ADR-0009; OMADIA_MCP_TOOLLIST_TTL_MS=0 restores spec-strict). Purged on close()/ closeAll() and immediately on notifications/tools/list_changed; lazy expiry on read. Discovery and the security rescan bypass via fresh: true. The bare-id probe only accepts entries filed as public (sharedPublic): a token-less caller's private list shares that key and must never cross auth contexts. Cached descriptors are deep-copied on write and read so caller mutation cannot poison the cache. Own servers now emit CacheableResult: loopback ttlMs 300000/public (frozen per-turn list), public server ttlMs 60000/private (per-API-key filtered list; tools/call keeps checking bindings live). --- ...9-default-ttl-for-mcp-tool-list-caching.md | 9 + docs/middleware-agent-handoff.md | 50 +++ middleware/.env.example | 8 + .../harness-orchestrator/src/index.ts | 6 + .../src/loopbackMcpServer.ts | 12 + .../harness-orchestrator/src/mcp/mcpClient.ts | 183 +++++++- middleware/src/mcp/publicMcpServer.ts | 17 + middleware/src/routes/agentBuilder.ts | 4 +- middleware/src/services/mcpRescan.ts | 4 +- .../test/cliBridge/loopbackMcpServer.test.ts | 10 +- middleware/test/fixtures/stdioMcpServer.mjs | 17 + middleware/test/mcpToolListCache.test.ts | 410 ++++++++++++++++++ .../publicMcp/publicMcpEndpoint.e2e.test.ts | 7 + 13 files changed, 730 insertions(+), 7 deletions(-) create mode 100644 docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md create mode 100644 middleware/test/mcpToolListCache.test.ts diff --git a/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md b/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md new file mode 100644 index 000000000..86fadc96e --- /dev/null +++ b/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md @@ -0,0 +1,9 @@ +# Default TTL for MCP tool-list caching when servers omit ttlMs + +MCP 2026-07-28 says a `tools/list` result without `ttlMs` must be treated as `ttlMs: 0` — i.e. not cached. Almost all remote servers we connect to today speak protocol ≤ 2025-11-25 and emit no `ttlMs`, so a spec-strict client cache would be a no-op and the cost/latency win of #545 (fewer discovery round-trips, stable prompt-cache tool blocks) would not materialize. We deliberately deviate on the client side: `McpManager` applies a configurable default TTL (60s, `OMADIA_MCP_TOOLLIST_TTL_MS`, opt-out via `0`) to list results that carry no `ttlMs`, and clamps server-provided TTLs to 15 minutes. + +The spec binds servers' emission, not clients' caching policy, so this is a policy choice, not a protocol violation. Safety valves: Discovery and Rescan always bypass the cache, `notifications/tools/list_changed` purges it immediately, entries are keyed like the connection pool (token-hash for `private`/unknown scope, server id for `public`), and tool *calls* are never gated on a cached list. + +Considered and rejected: spec-strict no-cache-without-ttlMs (defeats the purpose of the issue until the external ecosystem catches up to 2026-07-28). + +See also ADR-0008 (MCP connection lifetime) for the pool this cache is keyed against. diff --git a/docs/middleware-agent-handoff.md b/docs/middleware-agent-handoff.md index fe3d83e51..5c4ae2618 100644 --- a/docs/middleware-agent-handoff.md +++ b/docs/middleware-agent-handoff.md @@ -1065,6 +1065,42 @@ Verdikt ohne `code`), `test/adminProvidersRoute.test.ts` (DTO trägt `test/adminSettingsRoute.test.ts` (abgelehnter Wert → `settings.invalid_values`, unbekannter Key bzw. nicht installiertes Ziel-Plugin → `settings.no_valid_changes`). +### MCP Tool-List-Cache via `ttlMs`/`cacheScope` (issue #545) + +MCP 2026-07-28 macht `tools/list`-Results cachebar (`CacheableResult`: +`ttlMs` + `cacheScope`). Umgesetzt auf SDK 1.30.0 — **kein** v2-Bump nötig, +die Felder überleben das loose Result-Parsing (gleiches Muster wie +`resultType`, #544). + +- **Client** (`McpManager.listTools`, `packages/harness-orchestrator/src/mcp/ + mcpClient.ts`): TTL-Cache, Key via `mcpToolListCacheKey` — `public` ⇒ bare + Server-ID, `private`/unbekannt/fehlend ⇒ Pool-Key (Server-ID + Token-Hash, + Token-Rotation = Cache-Miss). Der Bare-Id-Probe akzeptiert nur als `public` + abgelegte Einträge (`sharedPublic`-Flag): die private Liste eines token-losen + Callers hat denselben Key (Pool-Key ohne Token = Server-ID) und darf nie + über Auth-Kontexte geteilt werden. Rückgaben sind Deep-Copies in beide + Richtungen — Caller-Mutation (Plugins!) erreicht den Cache nicht. + Server-`ttlMs` geclampt auf 15 min + (`MCP_TOOLLIST_MAX_TTL_MS`); fehlt `ttlMs`, greift ein Default von 60 s — + **bewusste Spec-Abweichung** (Spec: fehlend ⇒ nicht cachen), Begründung in + ADR-0009; `OMADIA_MCP_TOOLLIST_TTL_MS=0` stellt spec-strikt zurück. +- **Invalidierung:** `notifications/tools/list_changed` purgt sofort (Handler + wird vor `connect` registriert); `close()`/`closeAll()` purgen mit; Expiry + lazy beim Read (kein Timer, wie `evictIdle`). +- **Bypass:** Discovery (Builder-Route) und der Security-Rescan listen immer + frisch (`fresh: true`) — ein Scan über eine gecachte Liste scannt nichts. + Cache-Nutznießer ist der Plugin-Accessor `ctx.mcp.listTools()`. +- **Eigene Server emittieren:** Loopback `ttlMs: 300000` / `public` (Liste ist + pro Turn-Instanz eingefroren, nicht caller-abhängig); Public-Server + `ttlMs: 60000` / `private` (Liste ist per API-Key gefiltert — `private` ist + Pflicht, sonst leaken fremde Tool-Sets; `tools/call` prüft Bindings weiter + live, Revoke bleibt sofort wirksam). `list_changed`-*Emission* aus eigenen + Servern ist bewusst Folge-Issue. + +Tests: `test/mcpToolListCache.test.ts` (pure Regeln + Stdio-/HTTP-Fixtures), +Emission-Asserts in `test/cliBridge/loopbackMcpServer.test.ts` und +`test/publicMcp/publicMcpEndpoint.e2e.test.ts`. + --- ## 4. Migration Managed Agents → Lokal @@ -1454,6 +1490,20 @@ PORT=3979 `.env.example` ist gepflegt. Leere Strings parsed zod als `""`, nicht `undefined` — daher muss der Fallback `||` sein, nicht `??`. +### Package-lokale Env-Variablen (`OMADIA_*`, ohne zod-Schema) + +Das `harness-orchestrator`-Package importiert `config.ts` **nicht**; seine +Optionen laufen als `OMADIA_*`-Env mit Modul-Konstante als Default und werden +pro Aufruf aufgelöst (Änderung greift ohne Restart): + +``` +OMADIA_TOOL_DISPATCH_TIMEOUT_MS=240000 # äußere Dispatch-Deadline (W3-A) +OMADIA_MCP_CALL_TIMEOUT_MS=60000 # Idle-Budget pro MCP-Request (W0-2) +OMADIA_MCP_CALL_MAX_TOTAL_TIMEOUT_MS=180000 # absolute Decke inkl. Retry (W0-2) +OMADIA_MCP_TOOLLIST_TTL_MS=60000 # Default-TTL Tool-List-Cache (#545, + # ADR-0009); 0 = spec-strikt aus +``` + ### Wichtige Gotchas 1. **`??` vs `||`** bei Env-Fallbacks — haben wir einmal gefangen, steht diff --git a/middleware/.env.example b/middleware/.env.example index 7ce64fa36..0d5a5f026 100644 --- a/middleware/.env.example +++ b/middleware/.env.example @@ -127,6 +127,14 @@ DIAGRAM_PUBLIC_BASE_URL=http://localhost:8080 # once PUBLIC_BASE_URL is the middleware's own public host. # MCP_OAUTH_REDIRECT_URI=http://localhost:3000/bot-api/v1/operator/mcp-oauth/callback +# --- MCP tool-list caching (issue #545, ADR-0009) ---------------------------- +# TTL (ms) applied to a cached `tools/list` result when the remote server sends +# no `ttlMs` of its own (MCP 2026-07-28 CacheableResult). Server-provided TTLs +# are honoured but clamped to 15 minutes. `0` = spec-strict: never cache a +# list without an explicit server `ttlMs`. Read per listing — a change applies +# without a restart. Discovery and the security rescan always bypass the cache. +# OMADIA_MCP_TOOLLIST_TTL_MS=60000 + # --- In-app "Create Issue" button (operator GitHub device flow) ------------- # Lets an operator connect their OWN GitHub account and file issues to the # public repo (byte5ai/omadia) as themselves. Uses GitHub's DEVICE FLOW, so diff --git a/middleware/packages/harness-orchestrator/src/index.ts b/middleware/packages/harness-orchestrator/src/index.ts index 63ba3d1ee..2f4e6da8e 100644 --- a/middleware/packages/harness-orchestrator/src/index.ts +++ b/middleware/packages/harness-orchestrator/src/index.ts @@ -139,6 +139,12 @@ export { // Exported so the no-collateral-invalidation rule of `McpManager.close(id)` // can be unit tested and reused by ops tooling (issue #563). mcpPoolScopeMatches, + // #545 — the tool-list cache rules (TTL normalisation + scope keying) are + // the contract, unit-tested as pure functions like `mcpPoolScopeMatches`. + mcpToolListTtlMs, + mcpToolListCacheKey, + MCP_TOOLLIST_DEFAULT_TTL_MS, + MCP_TOOLLIST_MAX_TTL_MS, mcpToolToLocalSubAgentTool, mcpToolToNativeSpec, renderToolResult, diff --git a/middleware/packages/harness-orchestrator/src/loopbackMcpServer.ts b/middleware/packages/harness-orchestrator/src/loopbackMcpServer.ts index 7ef7f0e1e..c8ef9d860 100644 --- a/middleware/packages/harness-orchestrator/src/loopbackMcpServer.ts +++ b/middleware/packages/harness-orchestrator/src/loopbackMcpServer.ts @@ -31,6 +31,11 @@ import { sortByToolName } from './toolOrdering.js'; const MAX_REQUEST_BYTES = 8 * 1024 * 1024; +/** #545 — `ttlMs` advertised on `tools/list` (5 minutes). Any value is true + * for as long as this per-turn server exists; 5 minutes comfortably covers a + * turn without pretending the list is immortal across turns. */ +const LOOPBACK_TOOLLIST_TTL_MS = 300_000; + class PayloadTooLargeError extends Error {} export interface LoopbackMcpServerDeps { @@ -149,6 +154,13 @@ export class LoopbackMcpServer { description: tool.description, inputSchema: tool.input_schema, })), + // #545 — MCP 2026-07-28 `CacheableResult`. Both fields are honest by + // construction: `deps.tools` is readonly and this server lives exactly + // one turn, so the list cannot change while anyone holds it (`ttlMs` can + // afford to be generous), and it is identical for every caller — there + // is one bearer and no per-principal filtering (`public`). + ttlMs: LOOPBACK_TOOLLIST_TTL_MS, + cacheScope: 'public', })); mcp.setRequestHandler(CallToolRequestSchema, async (request) => { diff --git a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts index c119711f1..cd7a96487 100644 --- a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts +++ b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts @@ -43,7 +43,10 @@ import { } from '@modelcontextprotocol/sdk/client/stdio.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; -import { CallToolResultSchema } from '@modelcontextprotocol/sdk/types.js'; +import { + CallToolResultSchema, + ToolListChangedNotificationSchema, +} from '@modelcontextprotocol/sdk/types.js'; import { z } from 'zod'; import type { LocalSubAgentTool, @@ -386,11 +389,69 @@ export interface McpManagerOptions { * child process) would live as long as the process. `0` or negative * disables eviction. Defaults to `MCP_POOL_IDLE_TTL_MS`. */ readonly idleTtlMs?: number; + /** Issue #545 — TTL applied to a cached tool list whose server sent no + * `ttlMs` (ADR-0009). Same optional-dependency shape as `idleTtlMs`: wins + * over the `OMADIA_MCP_TOOLLIST_TTL_MS` env override, defaults to + * `MCP_TOOLLIST_DEFAULT_TTL_MS`. `0` (or negative) restores the + * spec-strict reading — absent `ttlMs` means no caching. */ + readonly toolListTtlMs?: number; } /** Default idle lifetime of a pooled MCP connection (5 minutes). */ export const MCP_POOL_IDLE_TTL_MS = 300_000; +/** + * Default TTL for a cached tool list when the server sent no `ttlMs` (#545). + * + * MCP 2026-07-28 says an absent `ttlMs` MUST be read as 0 — no caching. The + * ecosystem we connect to today speaks ≤ 2025-11-25 and never sends the field, + * so the spec-strict reading would make the cache a no-op and forfeit the + * discovery round-trips the cache exists to save. Deliberate client-side + * deviation, recorded in ADR-0009; `OMADIA_MCP_TOOLLIST_TTL_MS=0` (or + * `toolListTtlMs: 0`) opts back into spec-strict behaviour. + */ +export const MCP_TOOLLIST_DEFAULT_TTL_MS = 60_000; + +/** + * Ceiling on any tool-list TTL, server-provided or defaulted (ADR-0009). + * `notifications/tools/list_changed` invalidates cooperative servers + * immediately; this clamp bounds the staleness an uncooperative server can + * buy itself with an extravagant `ttlMs`. + */ +export const MCP_TOOLLIST_MAX_TTL_MS = 900_000; + +/** + * Effective TTL for one `tools/list` result. Exported for the same reason as + * `mcpPoolScopeMatches`: the rules are the contract, so they are unit-tested + * as a pure function rather than through timing-sensitive integration paths. + * + * - server sent a finite number → clamped to [0, MCP_TOOLLIST_MAX_TTL_MS] + * (spec: negative reads as 0; 0 means "do not cache") + * - absent or malformed → `defaultTtlMs` (ADR-0009), same clamp + */ +export function mcpToolListTtlMs(ttlMs: unknown, defaultTtlMs: number): number { + const fromServer = + typeof ttlMs === 'number' && Number.isFinite(ttlMs) ? Math.max(0, ttlMs) : undefined; + return Math.min(fromServer ?? Math.max(0, defaultTtlMs), MCP_TOOLLIST_MAX_TTL_MS); +} + +/** + * Cache key for one `tools/list` result (#545). + * + * `cacheScope: "public"` promises the list is identical across auth contexts, + * so it may be shared under the bare server id. Everything else — `"private"`, + * absent, or a value this client does not recognise — stays under the caller's + * pool key (server id + token hash), because a mis-shared tool list crosses + * auth contexts and a missed share only costs one extra round-trip. + */ +export function mcpToolListCacheKey( + poolKey: string, + serverId: string, + cacheScope: unknown, +): string { + return cacheScope === 'public' ? serverId : poolKey; +} + /** * True when `key` is a pool key belonging to server `id`. * @@ -509,6 +570,20 @@ function envMs(name: string, fallback: number): number { return parsed; } +/** `OMADIA_MCP_TOOLLIST_TTL_MS`, resolved per Listing (an operator change + * applies without a restart, like the dispatch timeout). NOT `envMs`: there + * `<= 0` is a config mistake, here `0` is the deliberate spec-strict opt-out + * (ADR-0009) and must survive parsing. A negative value reads as 0 too — the + * same rule `mcpToolListTtlMs` applies to the `toolListTtlMs` option, so the + * two config paths cannot disagree about what a negative default means. */ +function envToolListTtlMs(): number { + const raw = process.env['OMADIA_MCP_TOOLLIST_TTL_MS']; + if (raw === undefined || raw.trim() === '') return MCP_TOOLLIST_DEFAULT_TTL_MS; + const parsed = Number(raw); + if (!Number.isFinite(parsed)) return MCP_TOOLLIST_DEFAULT_TTL_MS; + return Math.max(0, parsed); +} + /** * The MCP request policy as it would be applied to the NEXT `callTool` — the * same resolution `callTool` performs, exposed so the timeout-hierarchy @@ -546,6 +621,24 @@ export class McpManager { * here and attached to the sidecar. A miss just omits the schema. */ private readonly outputSchemas = new Map>(); + /** Issue #545 — cached `tools/list` results. Keyed via `mcpToolListCacheKey` + * (bare server id for `public` scope, pool key otherwise), so the same + * `mcpPoolScopeMatches` rule that scopes `close()` also scopes cache + * invalidation. Expiry is lazy, like `evictIdle`: checked on read, never by + * a timer. */ + private readonly toolLists = new Map< + string, + { + readonly descriptors: readonly McpToolDescriptor[]; + readonly expiresAt: number; + /** Filed under the bare server id BECAUSE the server said `public`. A + * token-less caller's `private` list lands under the bare id too (its + * pool key IS the id), and must never be served across auth contexts — + * this flag is what tells the two apart on the shared-slot probe. */ + readonly sharedPublic: boolean; + } + >(); + /** Optional audit observer + dispatch guard (issues #462/#454). Existing * `new McpManager()` call sites keep working unchanged. */ constructor(private readonly options?: McpManagerOptions) {} @@ -732,9 +825,22 @@ export class McpManager { } } - /** Discover the tool list a server exposes. Throws on connection failure so - * the operator-facing `/discover` endpoint can report it. */ - async listTools(cfg: McpServerConfig): Promise { + /** + * List the tools a server exposes. Throws on connection failure so the + * operator-facing `/discover` endpoint can report it. + * + * Issue #545 — results are cached per `ttlMs`/`cacheScope` (MCP 2026-07-28 + * `CacheableResult`; ADR-0009 for the default applied when a server sends no + * `ttlMs`). Callers for whom a cached list would defeat the purpose pass + * `fresh: true`: Discovery (the Builder route persists what it fetches) and + * the security Rescan (a scan of a cached list scans nothing). The runtime + * Listing path — the plugin `ctx.mcp.listTools()` accessor — is the cache's + * beneficiary and uses the default. + */ + async listTools( + cfg: McpServerConfig, + opts?: { readonly fresh?: boolean }, + ): Promise { // Attach the OAuth token (issue #459 W9): some servers (e.g. Figma) require // authorization even to `initialize`/`tools/list`, so discovery must use the // caller's token exactly like a tool call — otherwise every OAuth-protected @@ -747,6 +853,18 @@ export class McpManager { /* token resolution must not break discovery */ } } + // The private key is derivable before the fetch; the public one is just the + // server id — so a read probes private-then-public and finds a hit wherever + // the last write's `cacheScope` filed it. The second probe accepts only + // entries filed as `public`: a token-less caller's `private` list sits + // under the bare id as well (its pool key IS the id) and serving it to a + // tokened caller would cross auth contexts. + const privateKey = this.poolKey(cfg, token); + if (!opts?.fresh) { + const hit = + this.cachedToolList(privateKey) ?? this.cachedToolList(cfg.id, { publicOnly: true }); + if (hit) return [...structuredClone(hit)]; + } const { client } = await this.getOrConnect(await this.withResolvedConfig(cfg), token); const res = await client.listTools(); const tools = Array.isArray(res?.tools) ? res.tools : []; @@ -765,9 +883,53 @@ export class McpManager { : {}), })); for (const d of descriptors) this.rememberToolSchema(cfg.id, d); + // #545 — file the result under the scope the server declared. `ttlMs` and + // `cacheScope` survive the SDK's parse because `ResultSchema` is loose + // (same shipped-SDK reading as `resultType`, #544); read defensively all + // the same, since both come off the wire. + const cacheable = res as { ttlMs?: unknown; cacheScope?: unknown }; + const ttl = mcpToolListTtlMs( + cacheable.ttlMs, + this.options?.toolListTtlMs ?? envToolListTtlMs(), + ); + if (ttl > 0) { + this.toolLists.set(mcpToolListCacheKey(privateKey, cfg.id, cacheable.cacheScope), { + // Deep-copied on write (and again on read): callers — plugins via + // `ctx.mcp.listTools()` — must not be able to mutate the cached + // descriptors that later callers receive. + descriptors: structuredClone(descriptors), + expiresAt: Date.now() + ttl, + sharedPublic: cacheable.cacheScope === 'public', + }); + } return descriptors; } + /** A still-fresh cached tool list under `key`, expiring lazily on read. + * `publicOnly` is set on the bare-server-id probe, so a token-less caller's + * `private` entry (also keyed by the bare id) never crosses auth contexts. */ + private cachedToolList( + key: string, + opts?: { readonly publicOnly?: boolean }, + ): readonly McpToolDescriptor[] | undefined { + const entry = this.toolLists.get(key); + if (!entry) return undefined; + if (entry.expiresAt <= Date.now()) { + this.toolLists.delete(key); + return undefined; + } + if (opts?.publicOnly && !entry.sharedPublic) return undefined; + return entry.descriptors; + } + + /** Drop every cached tool list belonging to server `id` (or to one exact + * pool key) — the same scoping rule `close()` applies to connections. */ + private purgeToolLists(id: string): void { + for (const key of this.toolLists.keys()) { + if (mcpPoolScopeMatches(key, id)) this.toolLists.delete(key); + } + } + /** Invoke a tool. Never throws — returns an `Error: …` string on failure so * the orchestrator turn keeps going. */ async callTool( @@ -1033,11 +1195,16 @@ export class McpManager { * fully invalidated. It never crosses server ids; see `mcpPoolScopeMatches`. */ async close(id: string): Promise { + // #545 — a closed server is an invalidated server: its cached tool list + // must not outlive its connections (deleted/reconfigured/re-authorized + // servers all arrive here). + this.purgeToolLists(id); const keys = [...this.entries.keys()].filter((key) => mcpPoolScopeMatches(key, id)); await Promise.all(keys.map((key) => this.dropEntry(key))); } async closeAll(): Promise { + this.toolLists.clear(); await Promise.all([...this.entries.keys()].map((key) => this.dropEntry(key))); } @@ -1096,6 +1263,14 @@ export class McpManager { private async connect(cfg: McpServerConfig, token: string | null): Promise { const transport = this.makeTransport(cfg, token); const client = new Client(CLIENT_INFO); + // #545 — `notifications/tools/list_changed` invalidates a cached tool list + // IMMEDIATELY, even mid-TTL (the spec's two freshness mechanisms are + // complementary: the TTL bounds staleness, the notification reports it). + // All scopes of this server are dropped — the server said "changed", not + // for whom. Registered before `connect` so no notification can race past. + client.setNotificationHandler(ToolListChangedNotificationSchema, () => { + this.purgeToolLists(cfg.id); + }); await client.connect(transport); return { client }; } diff --git a/middleware/src/mcp/publicMcpServer.ts b/middleware/src/mcp/publicMcpServer.ts index 7f83c8c31..e7fe68d1f 100644 --- a/middleware/src/mcp/publicMcpServer.ts +++ b/middleware/src/mcp/publicMcpServer.ts @@ -84,6 +84,13 @@ export const MAX_REQUEST_BYTES = 8 * 1024 * 1024; * denial-of-service budget rather than a protection. */ export const DEFAULT_TOOL_TIMEOUT_MS = 30_000; +/** #545 — `ttlMs` advertised on `tools/list` (60s). Short on purpose: a + * revoked/reconfigured binding may keep appearing in a client's cached list + * for up to this long (authorization is separate — `tools/call` checks + * bindings live). Also the only relief for the free-polling asymmetry noted + * at the batch gate: each `tools/list` costs a Postgres `bindings.get`. */ +export const PUBLIC_TOOLLIST_TTL_MS = 60_000; + /** Process-wide ceiling on tool calls in flight from this endpoint. Tools reach * Odoo/M365/Confluence and the LLM providers; an unbounded public fan-in * starves the operator-facing chat path that shares those pools. */ @@ -466,6 +473,16 @@ export class PublicMcpServer { // per-call `_meta` twin rides only the `tools/call` result below. this.sanitized('tools/list', async () => ({ tools: await this.listToolsFor(principal), + // #545 — MCP 2026-07-28 `CacheableResult`. `private` is mandatory, not + // a choice: the list is filtered per API key (`callableToolNames`), so + // sharing it across auth contexts would leak which tools OTHER keys + // may call — exactly the enumeration `listToolsFor` exists to prevent. + // The TTL is short because a revoked binding stays invisible in a + // cached LIST for up to this long; authorization itself is unaffected + // (`tools/call` checks bindings live on every call, see + // `publicMcpKeyBindings`' no-positive-cache rule). + ttlMs: PUBLIC_TOOLLIST_TTL_MS, + cacheScope: 'private', })), ); diff --git a/middleware/src/routes/agentBuilder.ts b/middleware/src/routes/agentBuilder.ts index 370f4e238..56ae59875 100644 --- a/middleware/src/routes/agentBuilder.ts +++ b/middleware/src/routes/agentBuilder.ts @@ -1242,7 +1242,9 @@ export function createAgentBuilderRouter( }); return; } - const tools = await mcp.listTools(toMcpConfig(row)); + // #545 — Discovery bypasses the tool-list cache: what gets scanned and + // persisted below must be what the server exposes NOW, not a cached view. + const tools = await mcp.listTools(toMcpConfig(row), { fresh: true }); // Scan gate (epic #459 W1, issue #454): every discovered tool is scanned // and its verdict persisted BEFORE the tool list itself is stored, so no // unscanned tool ever becomes visible or grantable. diff --git a/middleware/src/services/mcpRescan.ts b/middleware/src/services/mcpRescan.ts index 9ea5d48fe..eb67d2ca7 100644 --- a/middleware/src/services/mcpRescan.ts +++ b/middleware/src/services/mcpRescan.ts @@ -35,7 +35,9 @@ export async function rescanAllMcpServers( for (const server of servers) { if (server.status !== 'enabled') continue; try { - const tools = await manager.listTools(mcpRowToConfig(server)); + // #545 — the security rescan bypasses the tool-list cache: re-scanning + // a cached list would re-scan what was already scanned. + const tools = await manager.listTools(mcpRowToConfig(server), { fresh: true }); const verdicts = scanDiscoveredTools(server.id, tools); for (const verdict of verdicts) { await graph.upsertMcpToolVerdict(verdict); diff --git a/middleware/test/cliBridge/loopbackMcpServer.test.ts b/middleware/test/cliBridge/loopbackMcpServer.test.ts index ffa237d5e..69409c234 100644 --- a/middleware/test/cliBridge/loopbackMcpServer.test.ts +++ b/middleware/test/cliBridge/loopbackMcpServer.test.ts @@ -146,11 +146,19 @@ describe('LoopbackMcpServer', () => { }); assert.equal(listResponse.status, 200); const listPayload = parseMcpJson(await listResponse.text()) as { - result?: { tools?: Array<{ name: string }> }; + result?: { + tools?: Array<{ name: string }>; + ttlMs?: unknown; + cacheScope?: unknown; + }; }; assert.ok( listPayload.result?.tools?.some((tool) => tool.name === 'ping'), ); + // #545 — MCP 2026-07-28 CacheableResult: frozen per-turn list, one + // bearer, no per-principal filtering ⇒ generous TTL, public scope. + assert.equal(listPayload.result?.ttlMs, 300_000); + assert.equal(listPayload.result?.cacheScope, 'public'); const callResponse = await fetch(handle.url, { method: 'POST', diff --git a/middleware/test/fixtures/stdioMcpServer.mjs b/middleware/test/fixtures/stdioMcpServer.mjs index 2678f794d..70d9465b2 100644 --- a/middleware/test/fixtures/stdioMcpServer.mjs +++ b/middleware/test/fixtures/stdioMcpServer.mjs @@ -45,6 +45,11 @@ function handle(request) { return; } if (method === 'tools/list') { + // `list ` lines are the list-call counter for the tool-list cache + // tests (#545), the same way `start ` counts spawns for #563. + if (marker) appendFileSync(marker, `list ${process.pid}\n`); + const ttlRaw = process.env['MCP_FIXTURE_LIST_TTL_MS']; + const scope = process.env['MCP_FIXTURE_LIST_SCOPE']; send({ jsonrpc: '2.0', id, @@ -56,8 +61,20 @@ function handle(request) { inputSchema: { type: 'object', properties: {}, required: [] }, }, ], + // MCP 2026-07-28 CacheableResult fields, emitted only when the test + // asks for them — the default fixture stays a ≤2025-11-25 server + // that sends neither. + ...(ttlRaw !== undefined ? { ttlMs: Number(ttlRaw) } : {}), + ...(scope ? { cacheScope: scope } : {}), }, }); + // One `notifications/tools/list_changed` right after the first list, when + // asked: lets a test observe the immediate mid-TTL invalidation without + // needing a side channel into the child. + if (process.env['MCP_FIXTURE_LIST_CHANGED'] === '1' && !globalThis.__sentListChanged) { + globalThis.__sentListChanged = true; + send({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' }); + } return; } if (method === 'tools/call') { diff --git a/middleware/test/mcpToolListCache.test.ts b/middleware/test/mcpToolListCache.test.ts new file mode 100644 index 000000000..cadae38f7 --- /dev/null +++ b/middleware/test/mcpToolListCache.test.ts @@ -0,0 +1,410 @@ +import { describe, it, after } from 'node:test'; +import { strict as assert } from 'node:assert'; +import { mkdtemp, readFile, rm } from 'node:fs/promises'; +import { createServer } from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import { + McpManager, + mcpToolListCacheKey, + mcpToolListTtlMs, + MCP_TOOLLIST_DEFAULT_TTL_MS, + MCP_TOOLLIST_MAX_TTL_MS, + type McpAuthProvider, + type McpServerConfig, +} from '@omadia/orchestrator'; + +/** + * Tool-list caching rules (#545, MCP 2026-07-28 `CacheableResult`; ADR-0009). + * + * The pure rules — TTL normalisation, scope keying — are pinned as unit tests. + * The stateful behaviour (hit, expiry, `fresh` bypass, `close()` purge, + * `list_changed` purge) runs against the same stdio fixture the pool tests use + * (`test/fixtures/stdioMcpServer.mjs`): its `list ` marker lines are the + * ground truth for "did the wire get asked again", so a cache hit is asserted + * on server-observed behaviour rather than on internal maps. + * + * FOOTPRINT: spawns stdio children — suites run at `concurrency: 1`, like + * `mcpPool.test.ts` and for the same reason. + */ + +const FIXTURE = join(dirname(fileURLToPath(import.meta.url)), 'fixtures', 'stdioMcpServer.mjs'); + +function stdioServer( + id: string, + marker: string, + env: Record = {}, +): McpServerConfig { + return { + id, + name: `fixture-${id}`, + transport: 'stdio', + endpoint: `"${process.execPath}" "${FIXTURE}"`, + env: { MCP_FIXTURE_MARKER: marker, MCP_FIXTURE_MODE: 'ok', ...env }, + }; +} + +/** The `list ` lines the fixture appended — one per `tools/list` served. */ +async function listCalls(marker: string): Promise { + try { + const text = await readFile(marker, 'utf8'); + return text.split('\n').filter((line) => line.startsWith('list ')).length; + } catch { + return 0; + } +} + +async function waitFor( + what: string, + predicate: () => boolean | Promise, + deadlineMs: number, +): Promise { + const deadline = Date.now() + deadlineMs; + for (;;) { + if (await predicate()) return; + if (Date.now() >= deadline) throw new Error(`timed out waiting for ${what}`); + await new Promise((resolve) => setTimeout(resolve, 10)); + } +} + +describe('mcpToolListTtlMs (pure rules)', () => { + it('honours a server ttlMs and clamps it to the ceiling', () => { + assert.equal(mcpToolListTtlMs(5_000, 60_000), 5_000); + assert.equal(mcpToolListTtlMs(86_400_000, 60_000), MCP_TOOLLIST_MAX_TTL_MS); + }); + + it('reads ttlMs: 0 as "do not cache" — the default must not resurrect it', () => { + assert.equal(mcpToolListTtlMs(0, 60_000), 0); + }); + + it('reads a negative ttlMs as 0 (spec)', () => { + assert.equal(mcpToolListTtlMs(-1, 60_000), 0); + }); + + it('falls back to the default when ttlMs is absent or malformed (ADR-0009)', () => { + assert.equal(mcpToolListTtlMs(undefined, 60_000), 60_000); + assert.equal(mcpToolListTtlMs('60000', 60_000), 60_000); + assert.equal(mcpToolListTtlMs(Number.NaN, 60_000), 60_000); + }); + + it('a defaultTtlMs of 0 restores the spec-strict reading', () => { + assert.equal(mcpToolListTtlMs(undefined, 0), 0); + }); + + it('clamps the default too — no path escapes the ceiling', () => { + assert.equal(mcpToolListTtlMs(undefined, 86_400_000), MCP_TOOLLIST_MAX_TTL_MS); + }); +}); + +describe('mcpToolListCacheKey (pure rules)', () => { + it('shares a public list under the bare server id', () => { + assert.equal(mcpToolListCacheKey('srv#abc123', 'srv', 'public'), 'srv'); + }); + + it('keeps private, absent, and unrecognised scopes under the pool key', () => { + assert.equal(mcpToolListCacheKey('srv#abc123', 'srv', 'private'), 'srv#abc123'); + assert.equal(mcpToolListCacheKey('srv#abc123', 'srv', undefined), 'srv#abc123'); + assert.equal(mcpToolListCacheKey('srv#abc123', 'srv', 'org'), 'srv#abc123'); + }); +}); + +describe('McpManager tool-list cache (stdio fixture)', { concurrency: 1 }, () => { + const dirs: string[] = []; + const managers: McpManager[] = []; + + async function bench(): Promise<(name: string) => string> { + const dir = await mkdtemp(join(tmpdir(), 'omadia-mcp-toollist-')); + dirs.push(dir); + return (name: string) => join(dir, `${name}.marker`); + } + + function manager(options?: ConstructorParameters[0]): McpManager { + const m = new McpManager(options); + managers.push(m); + return m; + } + + after(async () => { + await Promise.all(managers.map((m) => m.closeAll())); + await Promise.all(dirs.map((dir) => rm(dir, { recursive: true, force: true }))); + }); + + it('serves the second listing from cache when the server sends a ttlMs', async () => { + const marker = (await bench())('ttl'); + const cfg = stdioServer('cache-hit', marker, { MCP_FIXTURE_LIST_TTL_MS: '60000' }); + const m = manager(); + const first = await m.listTools(cfg); + assert.equal(first.length, 1); + const second = await m.listTools(cfg); + assert.deepEqual(second, first); + assert.equal(await listCalls(marker), 1); + }); + + it('applies the ADR-0009 default when the server sends no ttlMs', async () => { + const marker = (await bench())('default'); + const cfg = stdioServer('cache-default', marker); + const m = manager(); // no toolListTtlMs option → MCP_TOOLLIST_DEFAULT_TTL_MS + assert.equal(MCP_TOOLLIST_DEFAULT_TTL_MS > 0, true); + await m.listTools(cfg); + await m.listTools(cfg); + assert.equal(await listCalls(marker), 1); + }); + + it('toolListTtlMs: 0 is spec-strict — no ttlMs, no cache', async () => { + const marker = (await bench())('strict'); + const cfg = stdioServer('cache-strict', marker); + const m = manager({ toolListTtlMs: 0 }); + await m.listTools(cfg); + await m.listTools(cfg); + assert.equal(await listCalls(marker), 2); + }); + + it('OMADIA_MCP_TOOLLIST_TTL_MS=0 opts the env path into spec-strict too', async () => { + const marker = (await bench())('env-strict'); + const cfg = stdioServer('cache-env-strict', marker); + const m = manager(); // no option → the env var decides + process.env['OMADIA_MCP_TOOLLIST_TTL_MS'] = '0'; + try { + await m.listTools(cfg); + await m.listTools(cfg); + } finally { + delete process.env['OMADIA_MCP_TOOLLIST_TTL_MS']; + } + assert.equal(await listCalls(marker), 2); + }); + + it('a server ttlMs of 0 is never cached, even with a default configured', async () => { + const marker = (await bench())('zero'); + const cfg = stdioServer('cache-zero', marker, { MCP_FIXTURE_LIST_TTL_MS: '0' }); + const m = manager({ toolListTtlMs: 60_000 }); + await m.listTools(cfg); + await m.listTools(cfg); + assert.equal(await listCalls(marker), 2); + }); + + it('fresh: true bypasses a still-fresh cache (Discovery/Rescan path)', async () => { + const marker = (await bench())('fresh'); + const cfg = stdioServer('cache-fresh', marker, { MCP_FIXTURE_LIST_TTL_MS: '60000' }); + const m = manager(); + await m.listTools(cfg); + await m.listTools(cfg, { fresh: true }); + assert.equal(await listCalls(marker), 2); + // …and the fresh result re-primes the cache. + await m.listTools(cfg); + assert.equal(await listCalls(marker), 2); + }); + + it('an expired entry is re-fetched', async () => { + const marker = (await bench())('expiry'); + const cfg = stdioServer('cache-expiry', marker, { MCP_FIXTURE_LIST_TTL_MS: '20' }); + const m = manager(); + await m.listTools(cfg); + await new Promise((resolve) => setTimeout(resolve, 30)); + await m.listTools(cfg); + assert.equal(await listCalls(marker), 2); + }); + + it('close(serverId) purges the cached list along with the connections', async () => { + const marker = (await bench())('close'); + const cfg = stdioServer('cache-close', marker, { MCP_FIXTURE_LIST_TTL_MS: '60000' }); + const m = manager(); + await m.listTools(cfg); + await m.close(cfg.id); + await m.listTools(cfg); + assert.equal(await listCalls(marker), 2); + }); + + it('OMADIA_MCP_TOOLLIST_TTL_MS with a negative value reads as 0 — same rule as the option path', async () => { + const marker = (await bench())('env-negative'); + const cfg = stdioServer('cache-env-negative', marker); + const m = manager(); + process.env['OMADIA_MCP_TOOLLIST_TTL_MS'] = '-5'; + try { + await m.listTools(cfg); + await m.listTools(cfg); + } finally { + delete process.env['OMADIA_MCP_TOOLLIST_TTL_MS']; + } + assert.equal(await listCalls(marker), 2); + }); + + it('a caller mutating the returned descriptors cannot poison the cache', async () => { + const marker = (await bench())('mutate'); + const cfg = stdioServer('cache-mutate', marker, { MCP_FIXTURE_LIST_TTL_MS: '60000' }); + const m = manager(); + const first = await m.listTools(cfg); + // Plugins get these objects via `ctx.mcp.listTools()`; `readonly` only + // protects at the type level, so simulate a misbehaving JS caller. + (first[0] as { name: string }).name = 'poisoned'; + first.pop(); + const second = await m.listTools(cfg); + assert.equal(await listCalls(marker), 1); // still served from cache … + assert.equal(second[0]?.name, 'ping'); // … and unpoisoned + (second[0] as { name: string }).name = 'poisoned-again'; // hit path too + const third = await m.listTools(cfg); + assert.equal(third[0]?.name, 'ping'); + }); + + it('notifications/tools/list_changed purges a still-fresh entry', async () => { + const marker = (await bench())('changed'); + const cfg = stdioServer('cache-changed', marker, { + MCP_FIXTURE_LIST_TTL_MS: '60000', + MCP_FIXTURE_LIST_CHANGED: '1', + }); + const m = manager(); + await m.listTools(cfg); + // The notification is written by the child right after its first list + // response; give the client loop a beat to dispatch it. + await waitFor( + 'list_changed to invalidate the cache', + async () => { + await m.listTools(cfg); + return (await listCalls(marker)) >= 2; + }, + 2_000, + ); + }); +}); + +/** + * Scope keying end-to-end: the stdio fixture cannot exercise it, because + * `poolKey()` deliberately ignores the token for stdio (the child never sees + * it). This hand-rolled streamable-HTTP fixture records the `Authorization` + * header of every `tools/list` it serves — so "token rotation ⇒ cache miss" + * (private) and "token rotation ⇒ shared hit" (public) are asserted on what + * the server actually saw. + */ +describe('McpManager tool-list cache scope keying (HTTP fixture)', { concurrency: 1 }, () => { + const managers: McpManager[] = []; + const closers: Array<() => Promise> = []; + + after(async () => { + await Promise.all(managers.map((m) => m.closeAll())); + await Promise.all(closers.map((close) => close())); + }); + + /** Minimal stateless streamable-HTTP MCP server: JSON replies, no session + * ids — the same mode the SDK's stateless example (and our own loopback) + * uses. Requests with an id get a JSON-RPC result; notifications get 202. */ + async function httpFixture(list: { + ttlMs: number; + cacheScope: string; + }): Promise<{ cfg: McpServerConfig; listAuths: string[] }> { + const listAuths: string[] = []; + const server = createServer((req, res) => { + // POST-only, like the loopback server: the spec makes the standalone GET + // SSE stream optional and blesses 405, and the SDK client tolerates it. + if (req.method !== 'POST') { + res.writeHead(405, { Allow: 'POST' }).end(); + return; + } + let body = ''; + req.setEncoding('utf8'); + req.on('data', (chunk: string) => (body += chunk)); + req.on('end', () => { + const msg = JSON.parse(body) as { + id?: number | string | null; + method?: string; + params?: { protocolVersion?: string }; + }; + if (msg.id === undefined || msg.id === null) { + res.writeHead(202).end(); + return; + } + const reply = (result: unknown): void => { + res + .writeHead(200, { 'content-type': 'application/json' }) + .end(JSON.stringify({ jsonrpc: '2.0', id: msg.id, result })); + }; + if (msg.method === 'initialize') { + reply({ + protocolVersion: msg.params?.protocolVersion ?? '2025-06-18', + capabilities: { tools: {} }, + serverInfo: { name: 'http-fixture', version: '0.0.1' }, + }); + return; + } + if (msg.method === 'tools/list') { + listAuths.push(String(req.headers.authorization ?? '')); + reply({ + tools: [ + { + name: 'ping', + description: 'Replies with pong.', + inputSchema: { type: 'object', properties: {}, required: [] }, + }, + ], + ttlMs: list.ttlMs, + cacheScope: list.cacheScope, + }); + return; + } + reply({}); + }); + }); + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + closers.push( + () => new Promise((resolve) => server.close(() => resolve())), + ); + const { port } = server.address() as AddressInfo; + const cfg: McpServerConfig = { + id: `http-${port}`, + name: `http-fixture-${port}`, + transport: 'http', + endpoint: `http://127.0.0.1:${port}/mcp`, + }; + return { cfg, listAuths }; + } + + /** Hands out the given tokens in order; the last one repeats. */ + function authWithTokens(...values: readonly (string | null)[]): McpAuthProvider { + let next = 0; + return { + getToken: (): Promise => { + const value = values[Math.min(next, values.length - 1)] ?? null; + next += 1; + return Promise.resolve(value); + }, + onAuthFailure: (): Promise => Promise.resolve(null), + }; + } + + function manager(auth: McpAuthProvider): McpManager { + const m = new McpManager({ auth }); + managers.push(m); + return m; + } + + it('private scope: a rotated token is a new cache context — miss, then hit under the new token', async () => { + const { cfg, listAuths } = await httpFixture({ ttlMs: 60_000, cacheScope: 'private' }); + const m = manager(authWithTokens('tok-a', 'tok-b')); + await m.listTools(cfg); // tok-a → fetch + await m.listTools(cfg); // tok-b → new pool key → fetch + await m.listTools(cfg); // tok-b again → hit + assert.deepEqual(listAuths, ['Bearer tok-a', 'Bearer tok-b']); + }); + + it('public scope: the list is shared across tokens under the bare server id', async () => { + const { cfg, listAuths } = await httpFixture({ ttlMs: 60_000, cacheScope: 'public' }); + const m = manager(authWithTokens('tok-a', 'tok-b')); + await m.listTools(cfg); // tok-a → fetch, filed under the server id + await m.listTools(cfg); // tok-b → different pool key, same public entry → hit + assert.deepEqual(listAuths, ['Bearer tok-a']); + }); + + it('a token-less caller’s private list is never shared with a tokened caller', async () => { + // The token-less pool key IS the bare server id, so this private entry + // lands in the same slot the public probe reads — it must stay invisible + // there. (Regression: the probe used to accept it and served the anonymous + // list to the tokened caller.) + const { cfg, listAuths } = await httpFixture({ ttlMs: 60_000, cacheScope: 'private' }); + const m = manager(authWithTokens(null, 'tok-b')); + await m.listTools(cfg); // no token → filed under the bare server id + await m.listTools(cfg); // tok-b → other auth context → must hit the wire + await m.listTools(cfg); // tok-b again → hit under its own pool key + assert.deepEqual(listAuths, ['', 'Bearer tok-b']); + }); +}); diff --git a/middleware/test/publicMcp/publicMcpEndpoint.e2e.test.ts b/middleware/test/publicMcp/publicMcpEndpoint.e2e.test.ts index 99ea7fb07..43d179d46 100644 --- a/middleware/test/publicMcp/publicMcpEndpoint.e2e.test.ts +++ b/middleware/test/publicMcp/publicMcpEndpoint.e2e.test.ts @@ -177,6 +177,13 @@ describe('public MCP endpoint', () => { const { status, payload } = await h.rpc(listToolsRequest(), { token: KEY_TOKEN }); assert.equal(status, 200); assert.deepEqual(toolNames(payload), SORTED_TOOLS); + // #545 — MCP 2026-07-28 CacheableResult: the list is filtered per API key, + // so `private` is mandatory (sharing it across auth contexts would leak + // other keys' tool sets), and the TTL stays short so a revoked binding + // does not linger in cached lists. + const result = (payload as { result?: { ttlMs?: unknown; cacheScope?: unknown } }).result; + assert.equal(result?.ttlMs, 60_000); + assert.equal(result?.cacheScope, 'private'); }); // ── statelessness ───────────────────────────────────────────────────────── From 2a24f075d3c5b2233e11d35d4b79ce7cd93d3f7a Mon Sep 17 00:00:00 2001 From: Andre Derjagin Date: Fri, 14 Aug 2026 10:40:31 +0200 Subject: [PATCH 2/3] fix(issue-545): tool-list purge lost when list_changed races the in-flight fetch (#545) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SDK dispatches notifications via Promise.resolve().then(), so a list_changed arriving in the same stdio chunk as the tools/list response could purge the cache BEFORE listTools primed it — the invalidation was lost and the stale-marked list stayed cached for its full TTL. Flaked in CI where pipe buffering coalesces the frames. Guard the cache write with a purge-generation counter: snapshot before the fetch, skip the write when any purge advanced it (purgeToolLists, closeAll). The stdio fixture now writes response + notification in one stdout.write, so the existing test exercises the coalesced delivery deterministically (red without the guard, green with it). --- .../harness-orchestrator/src/mcp/mcpClient.ts | 19 ++++++++++++++++++- middleware/test/fixtures/stdioMcpServer.mjs | 14 ++++++++++---- 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts index 7eea8c196..934fd77ae 100644 --- a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts +++ b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts @@ -849,6 +849,17 @@ export class McpManager { } >(); + /** #545 — purge generation for the tool-list cache. A purge that lands while + * a `tools/list` fetch is in flight must win over that fetch's cache write: + * the SDK dispatches a `list_changed` notification as a microtask, so it can + * run between the response resolving and `listTools` priming the cache — + * the purge would hit an empty map and the stale-marked list would then be + * cached for its full TTL. `listTools` snapshots this counter before the + * fetch and skips the write when any purge advanced it. Global rather than + * per-server: the false positive (an unrelated purge skipping one write) is + * just one extra fetch, and a purge is rare. */ + private toolListPurgeGen = 0; + /** Optional audit observer + dispatch guard (issues #462/#454). Existing * `new McpManager()` call sites keep working unchanged. */ constructor(private readonly options?: McpManagerOptions) {} @@ -1091,6 +1102,10 @@ export class McpManager { this.cachedToolList(privateKey) ?? this.cachedToolList(cfg.id, { publicOnly: true }); if (hit) return [...structuredClone(hit)]; } + // Snapshot BEFORE the fetch: a purge (list_changed, close) arriving while + // the request is in flight advances the generation, and the write below + // then steps aside instead of resurrecting an invalidated entry. + const purgeGen = this.toolListPurgeGen; const { client } = await this.getOrConnect(await this.withResolvedConfig(cfg), token); const res = await client.listTools(); const tools = Array.isArray(res?.tools) ? res.tools : []; @@ -1118,7 +1133,7 @@ export class McpManager { cacheable.ttlMs, this.options?.toolListTtlMs ?? envToolListTtlMs(), ); - if (ttl > 0) { + if (ttl > 0 && purgeGen === this.toolListPurgeGen) { this.toolLists.set(mcpToolListCacheKey(privateKey, cfg.id, cacheable.cacheScope), { // Deep-copied on write (and again on read): callers — plugins via // `ctx.mcp.listTools()` — must not be able to mutate the cached @@ -1151,6 +1166,7 @@ export class McpManager { /** Drop every cached tool list belonging to server `id` (or to one exact * pool key) — the same scoping rule `close()` applies to connections. */ private purgeToolLists(id: string): void { + this.toolListPurgeGen += 1; for (const key of this.toolLists.keys()) { if (mcpPoolScopeMatches(key, id)) this.toolLists.delete(key); } @@ -1437,6 +1453,7 @@ export class McpManager { } async closeAll(): Promise { + this.toolListPurgeGen += 1; this.toolLists.clear(); await Promise.all([...this.entries.keys()].map((key) => this.dropEntry(key))); } diff --git a/middleware/test/fixtures/stdioMcpServer.mjs b/middleware/test/fixtures/stdioMcpServer.mjs index 70d9465b2..a17acd5e9 100644 --- a/middleware/test/fixtures/stdioMcpServer.mjs +++ b/middleware/test/fixtures/stdioMcpServer.mjs @@ -50,7 +50,7 @@ function handle(request) { if (marker) appendFileSync(marker, `list ${process.pid}\n`); const ttlRaw = process.env['MCP_FIXTURE_LIST_TTL_MS']; const scope = process.env['MCP_FIXTURE_LIST_SCOPE']; - send({ + const response = { jsonrpc: '2.0', id, result: { @@ -67,14 +67,20 @@ function handle(request) { ...(ttlRaw !== undefined ? { ttlMs: Number(ttlRaw) } : {}), ...(scope ? { cacheScope: scope } : {}), }, - }); + }; // One `notifications/tools/list_changed` right after the first list, when // asked: lets a test observe the immediate mid-TTL invalidation without - // needing a side channel into the child. + // needing a side channel into the child. Written in the SAME + // `stdout.write` as the response — one pipe chunk — so the client sees + // the coalesced delivery CI produces under load (the #545 purge-vs-prime + // race) on every run, not just when the pipe happens to batch. if (process.env['MCP_FIXTURE_LIST_CHANGED'] === '1' && !globalThis.__sentListChanged) { globalThis.__sentListChanged = true; - send({ jsonrpc: '2.0', method: 'notifications/tools/list_changed' }); + const changed = { jsonrpc: '2.0', method: 'notifications/tools/list_changed' }; + process.stdout.write(`${JSON.stringify(response)}\n${JSON.stringify(changed)}\n`); + return; } + send(response); return; } if (method === 'tools/call') { From ead3779844ddc388ba3fb99cd74edd47b1930275 Mon Sep 17 00:00:00 2001 From: Marcel Wege Date: Sat, 15 Aug 2026 06:54:31 +0200 Subject: [PATCH 3/3] fix(#545): emit the cache hints on the era that can actually read them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #700 split the public endpoint's `tools/list` handler in two, one per protocol era. The `ttlMs`/`cacheScope` this branch added landed on the 2025 leg alone — which is the one era where the fields are not vocabulary and no client can read them, while the 2026-07-28 leg (the only one that can) emitted none. Both legs now carry the same hints, with the same reasoning: `private` is mandatory because the list is filtered per API key, and the TTL is short because a revoked binding lingers in a cached list for that long. `McpListToolsResult` now declares `ttlMs`/`cacheScope` instead of the read site casting to reach them. The cast made "the server sent nothing" and "this type forgot the fields" indistinguishable, so a change that really did stop them arriving would have compiled in silence and the cache would have fallen back to its default TTL forever. The comment justifying that read is corrected too. Loose passthrough is why the fields survive on the v1 legs; on the v2 leg they are modelled 2026-07-28 fields that arrive verbatim. Measured, because there was reason to doubt it — the v2 `listTools` runs with `cacheMode: 'bypass'`, and bypass turns out to skip only the SDK's own response cache, not the hints. Not collapsed into "the SDK is lenient": that is exactly the reasoning which proved false for `resultType` on a legacy-era v2 decode (#562 phase 2). ADR-0009 gains the structural half of its own premise: for `stdio` and `sse` peers the ecosystem never catches up, because `server/discover` is answered at the HTTP edge only. The default TTL is the permanent path there, not a transitional one. Mutation check: dropping the hints from the modern leg turns the new guard red, and only that one. Refs #545 --- ...9-default-ttl-for-mcp-tool-list-caching.md | 2 + .../harness-orchestrator/src/mcp/mcpClient.ts | 44 +++++++++++++++---- middleware/src/mcp/publicMcpServer.ts | 15 +++++++ .../publicMcp/publicMcpModernMrtr.test.ts | 20 +++++++++ 4 files changed, 73 insertions(+), 8 deletions(-) diff --git a/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md b/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md index 86fadc96e..627d1e925 100644 --- a/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md +++ b/docs/adr/0009-default-ttl-for-mcp-tool-list-caching.md @@ -6,4 +6,6 @@ The spec binds servers' emission, not clients' caching policy, so this is a poli Considered and rejected: spec-strict no-cache-without-ttlMs (defeats the purpose of the issue until the external ecosystem catches up to 2026-07-28). +One correction to "until the ecosystem catches up": for `stdio` and `sse` peers it never will. #562 measured that `server/discover` is answered at the HTTP edge only — a non-HTTP peer returns `-32601` regardless of its advertised versions, and pinning fails with `ERA_NEGOTIATION_FAILED` — so those transports stay legacy-era by construction and can never carry `ttlMs`. The default TTL is therefore the permanent path for them, not a transitional one. `http` peers are the only ones from which a server-declared TTL can arrive, and it does: verified end to end that `ttlMs`/`cacheScope` reach the client verbatim even though `listTools` runs with the SDK's `cacheMode: 'bypass'` (that bypass skips the SDK's own response cache, not the hints). + See also ADR-0008 (MCP connection lifetime) for the pool this cache is keyed against. diff --git a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts index 934fd77ae..1734683df 100644 --- a/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts +++ b/middleware/packages/harness-orchestrator/src/mcp/mcpClient.ts @@ -563,6 +563,24 @@ interface McpToolListEntry { interface McpListToolsResult { readonly tools?: readonly McpToolListEntry[]; + /** + * MCP 2026-07-28 `CacheableResult` (#545). Declared rather than cast at the + * read site, for the reason `LENIENT_CALL_TOOL_RESULT_SCHEMA` gives about + * `resultType`: a cast makes "the field is absent" and "this type forgot + * about the field" indistinguishable, so a change that really did stop + * these arriving would compile in silence and the tool-list cache would + * quietly fall back to its default TTL forever. + * + * Both are `unknown` on purpose. They come off the wire, `mcpToolListTtlMs` + * and `mcpToolListCacheKey` are the validators, and a second opinion here + * about what a well-formed value looks like is a second place to disagree. + * + * Measured, because the v2 path had reason to lose them: `listTools` runs + * with `cacheMode: 'bypass'`, and bypass only skips the SDK's own response + * cache — the hints still arrive verbatim on a modern-era connection. + */ + readonly ttlMs?: unknown; + readonly cacheScope?: unknown; } /** @@ -1124,23 +1142,33 @@ export class McpManager { : {}), })); for (const d of descriptors) this.rememberToolSchema(cfg.id, d); - // #545 — file the result under the scope the server declared. `ttlMs` and - // `cacheScope` survive the SDK's parse because `ResultSchema` is loose - // (same shipped-SDK reading as `resultType`, #544); read defensively all - // the same, since both come off the wire. - const cacheable = res as { ttlMs?: unknown; cacheScope?: unknown }; + // #545 — file the result under the scope the server declared. + // + // The two fields reach here by DIFFERENT routes, and only one of them is + // the loose-passthrough argument #544 used for `resultType`: + // - v1 legs (stdio, sse): `ResultSchema` is loose, so a server that + // sends them survives parsing. 2025-era servers do not send them, so + // in practice this is the ADR-0009 default-TTL path. + // - the v2 leg (http): they are modelled 2026-07-28 fields and arrive + // verbatim. MEASURED, because there was reason to doubt it — + // `listTools` runs with `cacheMode: 'bypass'` (#562 phase 2), and + // bypass skips only the SDK's own response cache, not the hints. + // Do not collapse that into "the SDK is lenient": on the v2 family that + // reasoning is what proved false for `resultType` on a legacy-era decode. + // + // Read defensively regardless — both come off the wire. const ttl = mcpToolListTtlMs( - cacheable.ttlMs, + res.ttlMs, this.options?.toolListTtlMs ?? envToolListTtlMs(), ); if (ttl > 0 && purgeGen === this.toolListPurgeGen) { - this.toolLists.set(mcpToolListCacheKey(privateKey, cfg.id, cacheable.cacheScope), { + this.toolLists.set(mcpToolListCacheKey(privateKey, cfg.id, res.cacheScope), { // Deep-copied on write (and again on read): callers — plugins via // `ctx.mcp.listTools()` — must not be able to mutate the cached // descriptors that later callers receive. descriptors: structuredClone(descriptors), expiresAt: Date.now() + ttl, - sharedPublic: cacheable.cacheScope === 'public', + sharedPublic: res.cacheScope === 'public', }); } return descriptors; diff --git a/middleware/src/mcp/publicMcpServer.ts b/middleware/src/mcp/publicMcpServer.ts index 41fc6d3fc..836d99463 100644 --- a/middleware/src/mcp/publicMcpServer.ts +++ b/middleware/src/mcp/publicMcpServer.ts @@ -783,6 +783,21 @@ export class PublicMcpServer { // carries it as `unknown`. The runtime value is the identical object // the v1 leg puts on the wire — nothing here reshapes it. tools: (await this.listToolsFor(principal)) as unknown as ModernTool[], + // #545 — the SAME `CacheableResult` hints the legacy leg emits, and + // this is the leg that can actually be read: `ttlMs`/`cacheScope` are + // 2026-07-28 vocabulary, and a client only reaches that revision here. + // Emitting them on the 2025 leg alone (which is where they landed + // before #700 split this handler in two) advertises a cache policy to + // exactly the callers who cannot see it. + // + // `private` is mandatory rather than chosen: the list is filtered per + // API key by `listToolsFor`, so a shared entry would leak which tools + // OTHER keys may call — the enumeration that filtering exists to + // prevent. The short TTL bounds how long a revoked binding can linger + // in a client's cached LIST; authorization is unaffected, because + // `tools/call` re-checks bindings live on every call. + ttlMs: PUBLIC_TOOLLIST_TTL_MS, + cacheScope: 'private' as const, })), ); diff --git a/middleware/test/publicMcp/publicMcpModernMrtr.test.ts b/middleware/test/publicMcp/publicMcpModernMrtr.test.ts index 2fb006953..49dbae7b1 100644 --- a/middleware/test/publicMcp/publicMcpModernMrtr.test.ts +++ b/middleware/test/publicMcp/publicMcpModernMrtr.test.ts @@ -177,6 +177,26 @@ describe('#700 — the public endpoint serves MRTR to a 2026-07-28 client', () = assert.equal(client.getProtocolEra(), 'modern'); }); + it('MUTATION CHECK: emits the CacheableResult hints on the era that can read them (#545)', async (t) => { + // `publicMcpEndpoint.e2e.test.ts` pins the same two fields on the 2025 + // leg. This is the half that matters: `ttlMs` / `cacheScope` are + // 2026-07-28 vocabulary, and #700 split this handler in two, so hints + // added to the legacy leg alone would advertise a cache policy to exactly + // the callers who cannot see it. + const h = await start(options(bookRoom), t); + if (!h) return; + const client = await connect(h); + + const listed = (await client.listTools(undefined, { cacheMode: 'bypass' })) as unknown as { + ttlMs?: unknown; + cacheScope?: unknown; + }; + assert.equal(listed.ttlMs, 60_000); + // `private` is mandatory, not chosen: the list is filtered per API key, so + // a shared entry would leak which tools OTHER keys may call. + assert.equal(listed.cacheScope, 'private'); + }); + it('asks with a spec-shaped embedded elicitation and an opaque requestState', async (t) => { const h = await start(options(bookRoom), t); if (!h) return;