From 11c995e9faa0671afdbfdaa159314e72d1fc23ae Mon Sep 17 00:00:00 2001 From: geoffdowns Date: Thu, 25 Sep 2025 21:42:21 -0700 Subject: [PATCH] Stop checking MCP tool schemas for type definitions (#9574) Co-authored-by: Gal Zahavi <38544478+galz10@users.noreply.github.com> Co-authored-by: anthony bushong Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com> Co-authored-by: Sandy Tao Co-authored-by: Jacob MacDonald Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: christine betts Co-authored-by: Adam Weidman <65992621+adamfweidman@users.noreply.github.com> Co-authored-by: matt korwel Co-authored-by: Shreya Keshive Co-authored-by: HugoMurillo Co-authored-by: Shreya Keshive Co-authored-by: Miguel Solorio Co-authored-by: Christie Warwick (Wilson) Co-authored-by: shrutip90 --- .../mcp_server_cyclic_schema.test.ts | 39 ++-- packages/core/src/tools/mcp-client.test.ts | 196 ++---------------- packages/core/src/tools/mcp-client.ts | 88 +------- 3 files changed, 51 insertions(+), 272 deletions(-) diff --git a/integration-tests/mcp_server_cyclic_schema.test.ts b/integration-tests/mcp_server_cyclic_schema.test.ts index e85d2877d4..c1ed12ce3e 100644 --- a/integration-tests/mcp_server_cyclic_schema.test.ts +++ b/integration-tests/mcp_server_cyclic_schema.test.ts @@ -5,14 +5,26 @@ */ /** - * This test verifies we can match maximum schema depth errors from Gemini - * and then detect and warn about the potential tools that caused the error. + * This test verifies we can provide MCP tools with recursive input schemas + * (in JSON, using the $ref keyword) and both the GenAI SDK and the Gemini + * API calls succeed. Note that prior to + * https://github.com/googleapis/js-genai/commit/36f6350705ecafc47eaea3f3eecbcc69512edab7#diff-fdde9372aec859322b7c5a5efe467e0ad25a57210c7229724586ee90ea4f5a30 + * the Gemini API call would fail for such tools because the schema was + * passed not as a JSON string but using the Gemini API's tool parameter + * schema object which has stricter typing and recursion restrictions. + * If this test fails, it's likely because either the GenAI SDK or Gemini API + * has become more restrictive about the type of tool parameter schemas that + * are accepted. If this occurs: Gemini CLI previously attempted to detect + * such tools and proactively remove them from the set of tools provided in + * the Gemini API call (as FunctionDeclaration objects). It may be appropriate + * to resurrect that behavior but note that it's difficult to keep the + * GCLI filters in sync with the Gemini API restrictions and behavior. */ -import { describe, it, beforeAll, expect } from 'vitest'; -import { TestRig } from './test-helper.js'; -import { join } from 'node:path'; import { writeFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { beforeAll, describe, expect, it } from 'vitest'; +import { TestRig } from './test-helper.js'; // Create a minimal MCP server that doesn't require external dependencies // This implements the MCP protocol directly using Node.js built-ins @@ -180,15 +192,14 @@ describe('mcp server with cyclic tool schema is detected', () => { } }); - it('should error and suggest disabling the cyclic tool', async () => { - // Just run any command to trigger the schema depth error. - // If this test starts failing, check `isSchemaDepthError` from - // geminiChat.ts to see if it needs to be updated. - // Or, possibly it could mean that gemini has fixed the issue. - const output = await rig.run('hello'); + it('mcp tool list should include tool with cyclic tool schema', async () => { + const tool_list_output = await rig.run('/mcp list'); + expect(tool_list_output).toContain('tool_with_cyclic_schema'); + }); - expect(output).toMatch( - /Skipping tool 'tool_with_cyclic_schema' from MCP server 'cyclic-schema-server' because it has missing types in its parameter schema/, - ); + it('gemini api call should be successful with cyclic mcp tool schema', async () => { + // Run any command and verify that we get a non-error response from + // the Gemini API. + await rig.run('hello'); }); }); diff --git a/packages/core/src/tools/mcp-client.test.ts b/packages/core/src/tools/mcp-client.test.ts index d2c87bb8e4..a8a94484cf 100644 --- a/packages/core/src/tools/mcp-client.test.ts +++ b/packages/core/src/tools/mcp-client.test.ts @@ -4,25 +4,24 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { afterEach, describe, expect, it, vi } from 'vitest'; -import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; -import { - populateMcpServerCommand, - createTransport, - isEnabled, - hasValidTypes, - McpClient, - hasNetworkTransport, -} from './mcp-client.js'; +import * as GenAiLib from '@google/genai'; +import * as ClientLib from '@modelcontextprotocol/sdk/client/index.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; import * as SdkClientStdioLib from '@modelcontextprotocol/sdk/client/stdio.js'; -import * as ClientLib from '@modelcontextprotocol/sdk/client/index.js'; -import * as GenAiLib from '@google/genai'; -import { GoogleCredentialProvider } from '../mcp/google-auth-provider.js'; +import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; +import { afterEach, describe, expect, it, vi } from 'vitest'; import { AuthProviderType, type Config } from '../config/config.js'; +import { GoogleCredentialProvider } from '../mcp/google-auth-provider.js'; import type { PromptRegistry } from '../prompts/prompt-registry.js'; -import type { ToolRegistry } from './tool-registry.js'; import type { WorkspaceContext } from '../utils/workspaceContext.js'; +import { + createTransport, + hasNetworkTransport, + isEnabled, + McpClient, + populateMcpServerCommand, +} from './mcp-client.js'; +import type { ToolRegistry } from './tool-registry.js'; vi.mock('@modelcontextprotocol/sdk/client/stdio.js'); vi.mock('@modelcontextprotocol/sdk/client/index.js'); @@ -78,7 +77,7 @@ describe('mcp-client', () => { expect(mockedMcpToTool).toHaveBeenCalledOnce(); }); - it('should skip tools if a parameter is missing a type', async () => { + it('should not skip tools even if a parameter is missing a type', async () => { const consoleWarnSpy = vi .spyOn(console, 'warn') .mockImplementation(() => {}); @@ -137,12 +136,8 @@ describe('mcp-client', () => { ); await client.connect(); await client.discover({} as Config); - expect(mockedToolRegistry.registerTool).toHaveBeenCalledOnce(); - 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.`, - ); + expect(mockedToolRegistry.registerTool).toHaveBeenCalledTimes(2); + expect(consoleWarnSpy).not.toHaveBeenCalled(); consoleWarnSpy.mockRestore(); }); @@ -409,165 +404,6 @@ 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); - }); - }); - describe('hasNetworkTransport', () => { it('should return true if only url is provided', () => { const config = { url: 'http://example.com' }; diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 468d43139f..1879948a99 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -5,19 +5,19 @@ */ import { Client } from '@modelcontextprotocol/sdk/client/index.js'; -import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; -import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import type { SSEClientTransportOptions } from '@modelcontextprotocol/sdk/client/sse.js'; import { SSEClientTransport } from '@modelcontextprotocol/sdk/client/sse.js'; +import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js'; import type { StreamableHTTPClientTransportOptions } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import { StreamableHTTPClientTransport } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; +import type { Transport } from '@modelcontextprotocol/sdk/shared/transport.js'; import type { - Prompt, GetPromptResult, + Prompt, } from '@modelcontextprotocol/sdk/types.js'; import { - ListPromptsResultSchema, GetPromptResultSchema, + ListPromptsResultSchema, ListRootsRequestSchema, } from '@modelcontextprotocol/sdk/types.js'; import { parse } from 'shell-quote'; @@ -28,18 +28,18 @@ import { DiscoveredMCPTool } from './mcp-tool.js'; import type { FunctionDeclaration } from '@google/genai'; import { mcpToTool } from '@google/genai'; -import type { ToolRegistry } from './tool-registry.js'; -import type { PromptRegistry } from '../prompts/prompt-registry.js'; -import { MCPOAuthProvider } from '../mcp/oauth-provider.js'; -import { OAuthUtils } from '../mcp/oauth-utils.js'; -import { MCPOAuthTokenStorage } from '../mcp/oauth-token-storage.js'; -import { getErrorMessage } from '../utils/errors.js'; import { basename } from 'node:path'; import { pathToFileURL } from 'node:url'; +import { MCPOAuthProvider } from '../mcp/oauth-provider.js'; +import { MCPOAuthTokenStorage } from '../mcp/oauth-token-storage.js'; +import { OAuthUtils } from '../mcp/oauth-utils.js'; +import type { PromptRegistry } from '../prompts/prompt-registry.js'; +import { getErrorMessage } from '../utils/errors.js'; import type { Unsubscribe, WorkspaceContext, } from '../utils/workspaceContext.js'; +import type { ToolRegistry } from './tool-registry.js'; export const MCP_DEFAULT_TIMEOUT_MSEC = 10 * 60 * 1000; // default to 10 minutes @@ -564,65 +564,6 @@ 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, @@ -658,15 +599,6 @@ 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,