fix: guard registered MCP tool schemas and keep the emitted tool order stable

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JgzuxyafKNF2uaEeL35XQw

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

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}` }])

frontend/src/lib/components/copilot/chat/global/mcpTools.ts

@@ -91,12 +91,17 @@ export function clearMcpToolsCache() {
  */
 const loadedTools = new Map<string, Tool<{}>>()
 
-/** 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<string, number>()
+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<string, unknown> {
+	const empty = { type: 'object', properties: {} }
+	if (typeof schema !== 'object' || schema === null || Array.isArray(schema)) return empty
+	const { $schema: _dropped, ...rest } = schema as Record<string, unknown>
+	const properties =
+		typeof rest.properties === 'object' &&
+		rest.properties !== null &&
+		!Array.isArray(rest.properties)
+			? (rest.properties as Record<string, unknown>)
+			: {}
+	// `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,

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<string, Record<string, Entry>> {
 	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