MediaMetz/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-03-10 22:21:22 -07:00
Code Issues Packages Projects Releases Wiki Activity

Stop checking MCP tool schemas for type definitions (#9574)

Browse Source 
Co-authored-by: Gal Zahavi <38544478+galz10@users.noreply.github.com>
Co-authored-by: anthony bushong <agmsb@users.noreply.github.com>
Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com>
Co-authored-by: Sandy Tao <sandytao520@icloud.com>
Co-authored-by: Jacob MacDonald <jakemac@google.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: christine betts <chrstn@uw.edu>
Co-authored-by: Adam Weidman <65992621+adamfweidman@users.noreply.github.com>
Co-authored-by: matt korwel <matt.korwel@gmail.com>
Co-authored-by: Shreya Keshive <skeshive@gmail.com>
Co-authored-by: HugoMurillo <mhugorodrigo@google.com>
Co-authored-by: Shreya Keshive <shreyakeshive@google.com>
Co-authored-by: Miguel Solorio <miguelsolorio@google.com>
Co-authored-by: Christie Warwick (Wilson) <bobcatfish@gmail.com>
Co-authored-by: shrutip90 <shruti.p90@gmail.com>
This commit is contained in:
geoffdowns
2025-09-25 21:42:21 -07:00
committed by GitHub
parent 53434d860a
commit 11c995e9fa
3 changed files with 51 additions and 272 deletions
Download Patch File Download Diff File Expand all files Collapse all files

39
integration-tests/mcp_server_cyclic_schema.test.ts
View File

@@ -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');
});
});

196
packages/core/src/tools/mcp-client.test.ts
View File

@@ -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' };

88
packages/core/src/tools/mcp-client.ts
View File

@@ -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<string, unknown>;
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,