diff --git a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts index 41570610d2..b9ffb2498b 100644 --- a/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts +++ b/frontend/src/lib/components/copilot/chat/AIChatManager.svelte.ts @@ -2698,8 +2698,12 @@ export class AIChatManager { }, get tools() { // Re-read every iteration by `chatLoop`, so a remote MCP tool registered - // during one iteration is callable on the next. - return [...self.tools, ...self.planMode.tools, ...loadedMcpTools()] + // during one iteration is callable on the next. GLOBAL only: it is the + // only mode that installs the MCP tools or reconciles the loaded set, so + // anywhere else these would be schemas a mode never opted into and + // nothing would ever drop them. + const mcpTools = self.mode === AIMode.GLOBAL ? loadedMcpTools() : [] + return [...self.tools, ...self.planMode.tools, ...mcpTools] }, get helpers() { return self.helpers diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts index bb7c502e89..f0ee8da1f2 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts @@ -183,6 +183,55 @@ describe('loaded remote tools', () => { }) }) + // A registered tool's schema goes into the request, where a provider rejects the + // whole completion over one bad tool — and the tool stays registered, so every + // later send fails too. These are shapes Windmill's own MCP server has emitted. + it('makes a request-breaking remote schema safe before registering it', () => { + registerMcpTools(server, [ + { + name: 'broken', + description: 'x', + inputSchema: { + $schema: 'https://json-schema.org/draft/2020-12/schema', + type: 'string', + properties: { a: { type: 'string' } }, + required: ['a', 'a', 'ghost'] + } + } as any + ]) + + const params = loadedMcpTools()[0].def.function.parameters as any + expect(params.type).toBe('object') + expect(params.required).toEqual(['a']) + expect(params.$schema).toBeUndefined() + }) + + it('falls back to an empty object schema when the remote sends no usable one', () => { + registerMcpTools(server, [{ name: 'nada', description: 'x', inputSchema: null } as any]) + + expect(loadedMcpTools()[0].def.function.parameters).toEqual({ type: 'object', properties: {} }) + }) + + // The emitted list carries Anthropic's cache_control breakpoint on its last entry, + // so calling or re-registering a tool must not move it: a reorder re-processes the + // whole cached prefix on the next iteration. + it('keeps the emitted tool order stable across calls and re-registration', async () => { + registerMcpTools(server, [TOOLS[0], TOOLS[1]]) + const before = loadedMcpTools().map((t) => t.def.function.name) + + // Call the first-registered one, then re-register the pair. + await loadedMcpTools()[0].fn({ + args: {}, + workspace: 'test-ws', + helpers: {}, + toolCallbacks: createToolCallbacks(), + toolId: 'tool-1' + }) + registerMcpTools(server, [TOOLS[0], TOOLS[1]]) + + expect(loadedMcpTools().map((t) => t.def.function.name)).toEqual(before) + }) + it('bounds the loaded set, evicting the least recently registered', () => { for (let i = 0; i < 30; i++) { registerMcpTools(server, [{ ...TOOLS[0], name: `tool_${i}` }]) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index db6b236601..14655c0a9b 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts @@ -91,12 +91,17 @@ export function clearMcpToolsCache() { */ const loadedTools = new Map>() -/** Insertion order is call order, so the first entry is the least recently used. */ +/** + * Recency for eviction, kept beside the map rather than as its order. The emitted + * tool list carries Anthropic's `cache_control` breakpoint on its last entry, so + * reordering it on every call would invalidate the cached prefix — system prompt, + * skills and transcript — for the rest of the turn. + */ +const lastUsed = new Map() +let useCounter = 0 + function touchLoadedTool(key: string) { - const tool = loadedTools.get(key) - if (!tool) return - loadedTools.delete(key) - loadedTools.set(key, tool) + if (loadedTools.has(key)) lastUsed.set(key, ++useCounter) } export function loadedMcpTools(): Tool<{}>[] { @@ -124,11 +129,15 @@ export function mcpServerForToolName(toolName: string): string | undefined { export function forgetLoadedMcpTools(serverPath?: string) { if (serverPath === undefined) { loadedTools.clear() + lastUsed.clear() return } const prefix = `${serverPath}::` for (const key of [...loadedTools.keys()]) { - if (key.startsWith(prefix)) loadedTools.delete(key) + if (key.startsWith(prefix)) { + loadedTools.delete(key) + lastUsed.delete(key) + } } } @@ -486,11 +495,60 @@ function registeredToolName(serverPath: string, toolName: string): string { return `${full.slice(0, MAX_TOOL_NAME_CHARS - 8)}_${shortHash(full)}` } +/** + * A remote server's `inputSchema`, made safe to send as a provider tool definition. + * + * Until now a remote schema only ever reached the model as tool-result text; a + * registered tool puts it in the request itself, where a provider rejects the whole + * completion rather than the one bad tool — and the chat would keep failing, because + * the tool stays registered. Windmill's own MCP server has shipped both of the shapes + * guarded here, so "the server is trusted" is not an argument. + * + * Deliberately shallow: this is a guard against a request-breaking schema, not a + * validator. Anything it cannot make sense of collapses to "accepts any object", + * which costs the model its argument names but keeps the chat alive. + */ +function safeInputSchema(schema: unknown): Record { + const empty = { type: 'object', properties: {} } + if (typeof schema !== 'object' || schema === null || Array.isArray(schema)) return empty + const { $schema: _dropped, ...rest } = schema as Record + const properties = + typeof rest.properties === 'object' && + rest.properties !== null && + !Array.isArray(rest.properties) + ? (rest.properties as Record) + : {} + // `required` is `uniqueItems`, and must name properties that exist: a strict + // validator rejects a repeat, and a name with no property reads as a parameter + // the model can never satisfy. + const required = Array.isArray(rest.required) + ? [ + ...new Set( + rest.required.filter( + (name): name is string => typeof name === 'string' && name in properties + ) + ) + ] + : [] + return { ...rest, type: 'object', properties, required } +} + function evictLoadedTools() { while (loadedTools.size > MAX_LOADED_TOOLS) { - const oldest = loadedTools.keys().next() - if (oldest.done) return - loadedTools.delete(oldest.value) + let victim: string | undefined + let victimRank = Infinity + for (const key of loadedTools.keys()) { + // Never used since registering ranks by registration order, which is what + // `lastUsed` misses on purpose: an unused tool goes before a used one. + const rank = lastUsed.get(key) ?? -1 + if (rank < victimRank) { + victimRank = rank + victim = key + } + } + if (victim === undefined) return + loadedTools.delete(victim) + lastUsed.delete(victim) } } @@ -507,15 +565,16 @@ export function registerMcpTools(server: McpServer, tools: McpToolDef[]): string const key = `${server.path}::${tool.name}` const name = registeredToolName(server.path, tool.name) const readOnly = isReadOnly(tool) - // Re-registering refreshes the schema and counts as a use. - loadedTools.delete(key) + // Set without deleting first: re-registering refreshes the schema in place, + // and Map.set keeps an existing key's position, so the emitted tool list — and + // the cache breakpoint on its last entry — does not move. loadedTools.set(key, { def: { type: 'function', function: { name, description: `${tool.description ?? tool.name}\n(MCP server ${server.path})`, - parameters: tool.inputSchema ?? { type: 'object', properties: {} } + parameters: safeInputSchema(tool.inputSchema) } }, showDetails: true, diff --git a/frontend/src/lib/components/mcp/iconCache.ts b/frontend/src/lib/components/mcp/iconCache.ts index 04f3b99991..120106c37b 100644 --- a/frontend/src/lib/components/mcp/iconCache.ts +++ b/frontend/src/lib/components/mcp/iconCache.ts @@ -14,7 +14,11 @@ import { providerKey } from './providerIcon' // value, which the list endpoint strips. type Entry = { key: string | null; editedAt?: string; host?: string } -const STORE_KEY = 'mcp_provider_icons' +// Versioned: entries written before `host` existed hold a key (often `null`, the +// no-shipped-icon case the favicon fallback is for) and read as a cache hit, so they +// would never learn a host and never draw a favicon. Bumping starts them over, at one +// resource read each. +const STORE_KEY = 'mcp_provider_icons_v2' function read(): Record> { try { @@ -49,14 +53,36 @@ export function cachedProviderMark( return entry ? { key: entry.key, host: entry.host } : undefined } -/** The url's host, when it has one worth drawing a favicon for. */ +/** Suffixes that only ever name something inside a network, so a favicon lookup for + * one would disclose an internal endpoint and could not succeed anyway. */ +const PRIVATE_HOST_SUFFIXES = [ + '.local', + '.localhost', + '.internal', + '.intranet', + '.lan', + '.corp', + '.home', + '.localdomain' +] + +/** + * The url's host, when it has one worth drawing a favicon for. + * + * Fetching a favicon tells the favicon service which host was asked about, so this + * withholds what it can recognise as private. It cannot recognise all of it: a public + * domain used internally (`mcp.internal.example.com`) is indistinguishable from any + * other, so a self-hosted instance still discloses that hostname when its server has + * no shipped icon. + */ export function providerHost(url: unknown): string | undefined { if (typeof url !== 'string') return undefined try { - const { hostname } = new URL(url) + const hostname = new URL(url).hostname.toLowerCase() // A loopback, a bare address, or an intranet single-label name has no favicon // to fetch, and asking would disclose it to the favicon service for nothing. if (!hostname.includes('.') || /^[\d.]+$/.test(hostname)) return undefined + if (PRIVATE_HOST_SUFFIXES.some((suffix) => hostname.endsWith(suffix))) return undefined return hostname } catch { return undefined