diff --git a/backend/windmill-mcp/src/common/schema.rs b/backend/windmill-mcp/src/common/schema.rs index 2684287ad3..005b4bde29 100644 --- a/backend/windmill-mcp/src/common/schema.rs +++ b/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 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 83e88bf7ed..2a71e149d2 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.test.ts +++ b/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]) diff --git a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts b/frontend/src/lib/components/copilot/chat/global/mcpTools.ts index 4843810588..6324fdb187 100644 --- a/frontend/src/lib/components/copilot/chat/global/mcpTools.ts +++ b/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))), diff --git a/frontend/src/lib/components/copilot/chat/shared.ts b/frontend/src/lib/components/copilot/chat/shared.ts index 2ae24a1b76..5fd22d9ad1 100644 --- a/frontend/src/lib/components/copilot/chat/shared.ts +++ b/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 | 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 diff --git a/frontend/src/lib/components/copilot/chat/toolSchema.ts b/frontend/src/lib/components/copilot/chat/toolSchema.ts new file mode 100644 index 0000000000..d6a41945f3 --- /dev/null +++ b/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 | 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) + } + } + } +}