fix: de-dupe nested required and bound a registered MCP tool schema

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

backend/windmill-mcp/src/common/schema.rs

@@ -261,6 +261,19 @@ pub fn make_schema_compatible(schema: &mut Value) {
         obj.insert("type".to_string(), Value::String("string".to_string()));
     }
 
+    // 2c. `required` is `uniqueItems` at every subschema, not just the root, so a
+    // repeat nested inside a property makes a strict validator reject the whole tool
+    // just as a root-level one does -- and the tool then vanishes from the client's
+    // list rather than failing loudly. `transform_property_keys` covers only the root,
+    // where it also has to follow key renames.
+    if let Some(Value::Array(required)) = obj.get_mut("required") {
+        let mut seen = HashSet::new();
+        required.retain(|name| match name.as_str() {
+            Some(name) => seen.insert(name.to_string()),
+            None => false,
+        });
+    }
+
     // 3. Fix contradictory type: if `properties` is present, type must be "object"
     if obj.contains_key("properties") {
         match obj.get("type").and_then(|v| v.as_str()) {
@@ -851,6 +864,28 @@ mod tests {
         assert!(desc.contains("$res:f/platform/aws_dev"));
     }
 
+    #[test]
+    fn dedupes_required_at_every_depth() {
+        // A repeat anywhere makes a strict validator reject the whole tool, and the
+        // root-level pass in `transform_property_keys` never sees a nested one.
+        let mut schema = json!({
+            "type": "object",
+            "properties": {
+                "user": {
+                    "type": "object",
+                    "properties": { "id": { "type": "string" } },
+                    "required": ["id", "id"]
+                }
+            },
+            "required": ["user", "user"]
+        });
+
+        make_schema_compatible(&mut schema);
+
+        assert_eq!(schema["required"], json!(["user"]));
+        assert_eq!(schema["properties"]["user"]["required"], json!(["id"]));
+    }
+
     #[test]
     fn enriched_resource_stays_a_string_through_make_schema_compatible() {
         // A resource param is stored with the resource's own object shape. Enrichment

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

@@ -7,24 +7,15 @@ const { getMcpToolsMock, callMcpToolMock, listResourceMock, session } = vi.hoist
 	session: { email: 'first@windmill.dev' }
 }))
 
+// `../shared` is stubbed because it reaches the chat's stores. The schema normalizer
+// that sanitizes a remote `inputSchema` on its way into a provider request is
+// deliberately NOT here — it lives in `../toolSchema`, a leaf module with no such
+// imports, so these tests exercise the real one rather than a copy that could drift.
 vi.mock('../shared', () => ({
 	createToolDef: (_schema: unknown, name: string, description: string) => ({
 		type: 'function',
 		function: { name, description, parameters: {} }
-	}),
-	// Mirrors the real normalizer closely enough to keep assertions about what
-	// reaches a provider honest: it strips empty/null `format`, recursively.
-	normalizeToolParameterSchema: function strip(schema: any): void {
-		if (!schema || typeof schema !== 'object') return
-		if (schema.format === null || schema.format === '') delete schema.format
-		for (const child of Object.values(schema.properties ?? {})) strip(child)
-		if (Array.isArray(schema.items)) schema.items.forEach(strip)
-		else strip(schema.items)
-		for (const kw of ['allOf', 'anyOf', 'oneOf']) {
-			if (Array.isArray(schema[kw])) schema[kw].forEach(strip)
-		}
-		if (typeof schema.additionalProperties === 'object') strip(schema.additionalProperties)
-	}
+	})
 }))
 
 vi.mock('$lib/gen', () => ({
@@ -222,6 +213,23 @@ describe('loaded remote tools', () => {
 		expect(params.properties.a.format).toBeUndefined()
 	})
 
+	// A registered schema rides in every request for the rest of the conversation, so
+	// an outsized one is left to the wrapper — where it costs a tool result once —
+	// rather than truncated into something the model cannot tell is incomplete.
+	it('does not register a tool whose schema is too large to carry', () => {
+		const huge = {
+			name: 'huge',
+			description: 'x',
+			inputSchema: {
+				type: 'object',
+				properties: { a: { type: 'string', description: 'x'.repeat(9000) } }
+			}
+		} as any
+
+		expect(registerMcpTools(server, [huge])).toEqual([undefined])
+		expect(loadedMcpTools()).toEqual([])
+	})
+
 	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])
 

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

@@ -1,6 +1,7 @@
 import { z } from 'zod'
 import { ResourceService, type GetMcpToolsResponse } from '$lib/gen'
-import { createToolDef, normalizeToolParameterSchema, type Tool } from '../shared'
+import { createToolDef, type Tool } from '../shared'
+import { normalizeToolParameterSchema } from '../toolSchema'
 import { enabledMcpPaths } from '$lib/components/mcp/enabledServers'
 
 /**
@@ -28,6 +29,11 @@ const MAX_DESCRIPTION_CHARS = 200
 const MAX_LOADED_TOOLS = 25
 // Both providers cap tool names; OpenAI's 64 is the lower of the two.
 const MAX_TOOL_NAME_CHARS = 64
+// A registered schema is server-controlled text that rides in every request for the
+// rest of the conversation, so it is bounded like every other payload a server
+// controls. Past this, the tool is left unregistered and the model reaches it through
+// `call_mcp_*` instead — the schema still arrives, but only when a call fails.
+const MAX_TOOL_SCHEMA_CHARS = 8_000
 const MAX_RESULT_CHARS = 20_000
 // A server writes its own error text, and every enabled server can contribute
 // one, so search results are capped the same way call results are.
@@ -567,11 +573,17 @@ function evictLoadedTools() {
  * Each tool carries its own `readOnlyHint`, so the confirmation gate is per tool
  * and the read/write wrappers' "you used the wrong one" failure cannot arise.
  */
-export function registerMcpTools(server: McpServer, tools: McpToolDef[]): string[] {
+export function registerMcpTools(server: McpServer, tools: McpToolDef[]): (string | undefined)[] {
 	const names = tools.map((tool) => {
 		const key = `${server.path}::${tool.name}`
 		const name = registeredToolName(server.path, tool.name)
 		const readOnly = isReadOnly(tool)
+		const parameters = safeInputSchema(tool.inputSchema)
+		if (JSON.stringify(parameters).length > MAX_TOOL_SCHEMA_CHARS) {
+			// Left to the wrapper rather than truncated: half a schema would be worse
+			// than none, since the model cannot tell which half it is missing.
+			return undefined
+		}
 		// 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.
@@ -581,7 +593,7 @@ export function registerMcpTools(server: McpServer, tools: McpToolDef[]): string
 				function: {
 					name,
 					description: `${tool.description ?? tool.name}\n(MCP server ${server.path})`,
-					parameters: safeInputSchema(tool.inputSchema)
+					parameters
 				}
 			},
 			showDetails: true,
@@ -685,7 +697,12 @@ export function createMcpTools(servers: McpServer[]): Tool<{}>[] {
 				}
 				for (const { server, tools } of perServerMatches.values()) {
 					const registered = registerMcpTools(server, tools)
-					tools.forEach((tool, i) => callNames.set(tool, registered[i]))
+					// A tool whose schema was too large to register has no `call` name, and
+					// the summary then advertises the wrapper for it instead.
+					tools.forEach((tool, i) => {
+						const name = registered[i]
+						if (name !== undefined) callNames.set(tool, name)
+					})
 				}
 				const result = boundedSearch({
 					matches: top.map((s) => summarizeTool(s.server, s.tool, callNames.get(s.tool))),

frontend/src/lib/components/copilot/chat/shared.ts

@@ -7,6 +7,7 @@ import type { UserDraftItemKind } from '$lib/gen'
 // The gate's two refusals, from a module that holds prose and one size limit: under the
 // shallow-import rule below, the rest of plan mode is not reachable from here.
 import { PLAN_MODE_MESSAGES } from './planModeMessages'
+import { normalizeToolParameterSchema } from './toolSchema'
 // Import-free leaf, so it satisfies the shallow-import rule below.
 import {
 	openItemPreviewAction,
@@ -1395,52 +1396,6 @@ export const createSearchHubScriptsTool = (withContent: boolean = false) => ({
 	}
 })
 
-/**
- * Recursively normalizes JSON Schema quirks that specific providers reject.
- */
-export function normalizeToolParameterSchema(schema: Record<string, any> | undefined): void {
-	if (!schema || typeof schema !== 'object') {
-		return
-	}
-
-	// Remove format if it's null or empty string
-	if (schema.format === null || schema.format === '') {
-		delete schema.format
-	}
-
-	// Recurse into properties
-	if (schema.properties && typeof schema.properties === 'object') {
-		for (const key of Object.keys(schema.properties)) {
-			normalizeToolParameterSchema(schema.properties[key])
-		}
-	}
-
-	// Recurse into items (for arrays)
-	if (schema.items) {
-		if (Array.isArray(schema.items)) {
-			for (const item of schema.items) {
-				normalizeToolParameterSchema(item)
-			}
-		} else {
-			normalizeToolParameterSchema(schema.items)
-		}
-	}
-
-	// Recurse into additionalProperties if it's an object schema
-	if (schema.additionalProperties && typeof schema.additionalProperties === 'object') {
-		normalizeToolParameterSchema(schema.additionalProperties)
-	}
-
-	// Recurse into allOf, anyOf, oneOf
-	for (const key of ['allOf', 'anyOf', 'oneOf']) {
-		if (Array.isArray(schema[key])) {
-			for (const subSchema of schema[key]) {
-				normalizeToolParameterSchema(subSchema)
-			}
-		}
-	}
-}
-
 export async function buildSchemaForTool(
 	toolDef: ChatCompletionFunctionTool,
 	schemaBuilder: () => Promise<FunctionParameters>

frontend/src/lib/components/copilot/chat/toolSchema.ts

@@ -0,0 +1,56 @@
+/**
+ * Recursively normalizes JSON Schema quirks that specific providers reject.
+ *
+ * Its own module rather than part of `shared`: a tool schema can come from a third
+ * party (an MCP server's `inputSchema`), so this runs on paths that have no business
+ * pulling in the chat's stores, and its tests have no business mocking them.
+ */
+export function normalizeToolParameterSchema(schema: Record<string, any> | undefined): void {
+	if (!schema || typeof schema !== 'object') {
+		return
+	}
+
+	// Remove format if it's null or empty string
+	if (schema.format === null || schema.format === '') {
+		delete schema.format
+	}
+
+	// `required` is `uniqueItems` at every subschema, and a provider rejects the whole
+	// request over a repeat — which for a tool built from a third party's schema means
+	// every send fails until that tool goes away.
+	if (Array.isArray(schema.required)) {
+		schema.required = [...new Set(schema.required.filter((n: unknown) => typeof n === 'string'))]
+	}
+
+	// Recurse into properties
+	if (schema.properties && typeof schema.properties === 'object') {
+		for (const key of Object.keys(schema.properties)) {
+			normalizeToolParameterSchema(schema.properties[key])
+		}
+	}
+
+	// Recurse into items (for arrays)
+	if (schema.items) {
+		if (Array.isArray(schema.items)) {
+			for (const item of schema.items) {
+				normalizeToolParameterSchema(item)
+			}
+		} else {
+			normalizeToolParameterSchema(schema.items)
+		}
+	}
+
+	// Recurse into additionalProperties if it's an object schema
+	if (schema.additionalProperties && typeof schema.additionalProperties === 'object') {
+		normalizeToolParameterSchema(schema.additionalProperties)
+	}
+
+	// Recurse into allOf, anyOf, oneOf
+	for (const key of ['allOf', 'anyOf', 'oneOf']) {
+		if (Array.isArray(schema[key])) {
+			for (const subSchema of schema[key]) {
+				normalizeToolParameterSchema(subSchema)
+			}
+		}
+	}
+}