From b3cfaeb6d30101262dc2b7350f5a349cd0417386 Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Wed, 6 Aug 2025 13:19:15 -0700 Subject: [PATCH] Add detection of tools with bad schemas and automatically omit them with a warning (#5694) --- packages/core/src/tools/mcp-client.test.ts | 336 +++++++++++++++++++++ packages/core/src/tools/mcp-client.ts | 68 +++++ 2 files changed, 404 insertions(+) diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index 9997d60e..1ccba76a 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -12,6 +12,7 @@ import { isEnabled, discoverTools, discoverPrompts, + hasValidTypes, } from './mcp-client.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; import * as SdkClientStdioLib from '@modelcontextprotocol/sdk/client/stdio.js'; @@ -97,6 +98,182 @@ describe('mcp-client', () => { `Error discovering tool: 'invalid tool name' from MCP server 'test-server': ${testError.message}`, ); }); + + it('should skip tools if a parameter is missing a type', async () => { + const mockedClient = {} as unknown as ClientLib.Client; + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}); + vi.mocked(GenAiLib.mcpToTool).mockReturnValue({ + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'validTool', + parametersJsonSchema: { + type: 'object', + properties: { + param1: { type: 'string' }, + }, + }, + }, + { + name: 'invalidTool', + parametersJsonSchema: { + type: 'object', + properties: { + param1: { description: 'a param with no type' }, + }, + }, + }, + ], + }), + } as unknown as GenAiLib.CallableTool); + + const tools = await discoverTools('test-server', {}, mockedClient); + + expect(tools.length).toBe(1); + expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool'); + expect(consoleWarnSpy).toHaveBeenCalledOnce(); + expect(consoleWarnSpy).toHaveBeenCalledWith( + `Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` + + `missing types in its parameter schema. Please file an issue with the owner of the MCP server.`, + ); + consoleWarnSpy.mockRestore(); + }); + + it('should skip tools if a nested parameter is missing a type', async () => { + const mockedClient = {} as unknown as ClientLib.Client; + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}); + vi.mocked(GenAiLib.mcpToTool).mockReturnValue({ + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'invalidTool', + parametersJsonSchema: { + type: 'object', + properties: { + param1: { + type: 'object', + properties: { + nestedParam: { + description: 'a nested param with no type', + }, + }, + }, + }, + }, + }, + ], + }), + } as unknown as GenAiLib.CallableTool); + + const tools = await discoverTools('test-server', {}, mockedClient); + + expect(tools.length).toBe(0); + expect(consoleWarnSpy).toHaveBeenCalledOnce(); + expect(consoleWarnSpy).toHaveBeenCalledWith( + `Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` + + `missing types in its parameter schema. Please file an issue with the owner of the MCP server.`, + ); + consoleWarnSpy.mockRestore(); + }); + + it('should skip tool if an array item is missing a type', async () => { + const mockedClient = {} as unknown as ClientLib.Client; + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}); + vi.mocked(GenAiLib.mcpToTool).mockReturnValue({ + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'invalidTool', + parametersJsonSchema: { + type: 'object', + properties: { + param1: { + type: 'array', + items: { + description: 'an array item with no type', + }, + }, + }, + }, + }, + ], + }), + } as unknown as GenAiLib.CallableTool); + + const tools = await discoverTools('test-server', {}, mockedClient); + + expect(tools.length).toBe(0); + expect(consoleWarnSpy).toHaveBeenCalledOnce(); + expect(consoleWarnSpy).toHaveBeenCalledWith( + `Skipping tool 'invalidTool' from MCP server 'test-server' because it has ` + + `missing types in its parameter schema. Please file an issue with the owner of the MCP server.`, + ); + consoleWarnSpy.mockRestore(); + }); + + it('should discover tool with no properties in schema', async () => { + const mockedClient = {} as unknown as ClientLib.Client; + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}); + vi.mocked(GenAiLib.mcpToTool).mockReturnValue({ + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'validTool', + parametersJsonSchema: { + type: 'object', + }, + }, + ], + }), + } as unknown as GenAiLib.CallableTool); + + const tools = await discoverTools('test-server', {}, mockedClient); + + expect(tools.length).toBe(1); + expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool'); + expect(consoleWarnSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + }); + + it('should discover tool with empty properties object in schema', async () => { + const mockedClient = {} as unknown as ClientLib.Client; + const consoleWarnSpy = vi + .spyOn(console, 'warn') + .mockImplementation(() => {}); + vi.mocked(GenAiLib.mcpToTool).mockReturnValue({ + tool: () => + Promise.resolve({ + functionDeclarations: [ + { + name: 'validTool', + parametersJsonSchema: { + type: 'object', + properties: {}, + }, + }, + ], + }), + } as unknown as GenAiLib.CallableTool); + + const tools = await discoverTools('test-server', {}, mockedClient); + + expect(tools.length).toBe(1); + expect(vi.mocked(DiscoveredMCPTool).mock.calls[0][2]).toBe('validTool'); + expect(consoleWarnSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + }); }); describe('discoverPrompts', () => { @@ -433,4 +610,163 @@ describe('mcp-client', () => { ); }); }); + + describe('hasValidTypes', () => { + it('should return true for a valid schema with anyOf', () => { + const schema = { + anyOf: [{ type: 'string' }, { type: 'number' }], + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false for an invalid schema with anyOf', () => { + const schema = { + anyOf: [{ type: 'string' }, { description: 'no type' }], + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a valid schema with allOf', () => { + const schema = { + allOf: [ + { type: 'string' }, + { type: 'object', properties: { foo: { type: 'string' } } }, + ], + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false for an invalid schema with allOf', () => { + const schema = { + allOf: [{ type: 'string' }, { description: 'no type' }], + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a valid schema with oneOf', () => { + const schema = { + oneOf: [{ type: 'string' }, { type: 'number' }], + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false for an invalid schema with oneOf', () => { + const schema = { + oneOf: [{ type: 'string' }, { description: 'no type' }], + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a valid schema with nested subschemas', () => { + const schema = { + anyOf: [ + { type: 'string' }, + { + allOf: [ + { type: 'object', properties: { a: { type: 'string' } } }, + { type: 'object', properties: { b: { type: 'number' } } }, + ], + }, + ], + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false for an invalid schema with nested subschemas', () => { + const schema = { + anyOf: [ + { type: 'string' }, + { + allOf: [ + { type: 'object', properties: { a: { type: 'string' } } }, + { description: 'no type' }, + ], + }, + ], + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a schema with a type and subschemas', () => { + const schema = { + type: 'string', + anyOf: [{ minLength: 1 }, { maxLength: 5 }], + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false for a schema with no type and no subschemas', () => { + const schema = { + description: 'a schema with no type', + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a valid schema', () => { + const schema = { + type: 'object', + properties: { + param1: { type: 'string' }, + }, + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return false if a parameter is missing a type', () => { + const schema = { + type: 'object', + properties: { + param1: { description: 'a param with no type' }, + }, + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return false if a nested parameter is missing a type', () => { + const schema = { + type: 'object', + properties: { + param1: { + type: 'object', + properties: { + nestedParam: { + description: 'a nested param with no type', + }, + }, + }, + }, + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return false if an array item is missing a type', () => { + const schema = { + type: 'object', + properties: { + param1: { + type: 'array', + items: { + description: 'an array item with no type', + }, + }, + }, + }; + expect(hasValidTypes(schema)).toBe(false); + }); + + it('should return true for a schema with no properties', () => { + const schema = { + type: 'object', + }; + expect(hasValidTypes(schema)).toBe(true); + }); + + it('should return true for a schema with an empty properties object', () => { + const schema = { + type: 'object', + properties: {}, + }; + expect(hasValidTypes(schema)).toBe(true); + }); + }); }); diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 26244d9e..9a35b84e 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -416,6 +416,65 @@ export async function connectAndDiscover( } } +/** + * Recursively validates that a JSON schema and all its nested properties and + * items have a `type` defined. + * + * @param schema The JSON schema to validate. + * @returns `true` if the schema is valid, `false` otherwise. + * + * @visiblefortesting + */ +export function hasValidTypes(schema: unknown): boolean { + if (typeof schema !== 'object' || schema === null) { + // Not a schema object we can validate, or not a schema at all. + // Treat as valid as it has no properties to be invalid. + return true; + } + + const s = schema as Record; + + if (!s.type) { + // These keywords contain an array of schemas that should be validated. + // + // If no top level type was given, then they must each have a type. + let hasSubSchema = false; + const schemaArrayKeywords = ['anyOf', 'allOf', 'oneOf']; + for (const keyword of schemaArrayKeywords) { + const subSchemas = s[keyword]; + if (Array.isArray(subSchemas)) { + hasSubSchema = true; + for (const subSchema of subSchemas) { + if (!hasValidTypes(subSchema)) { + return false; + } + } + } + } + + // If the node itself is missing a type and had no subschemas, then it isn't valid. + if (!hasSubSchema) return false; + } + + if (s.type === 'object' && s.properties) { + if (typeof s.properties === 'object' && s.properties !== null) { + for (const prop of Object.values(s.properties)) { + if (!hasValidTypes(prop)) { + return false; + } + } + } + } + + if (s.type === 'array' && s.items) { + if (!hasValidTypes(s.items)) { + return false; + } + } + + return true; +} + /** * Discovers and sanitizes tools from a connected MCP client. * It retrieves function declarations from the client, filters out disabled tools, @@ -448,6 +507,15 @@ export async function discoverTools( continue; } + if (!hasValidTypes(funcDecl.parametersJsonSchema)) { + console.warn( + `Skipping tool '${funcDecl.name}' from MCP server '${mcpServerName}' ` + + `because it has missing types in its parameter schema. Please file an ` + + `issue with the owner of the MCP server.`, + ); + continue; + } + discoveredTools.push( new DiscoveredMCPTool( mcpCallableTool,