fix(patch): cherry-pick 931e668 to release/v0.33.0-preview.4-pr-21425 [CONFLICTS] (#21478)

Co-authored-by: Abhi <43648792+abhipatel12@users.noreply.github.com>
Co-authored-by: Abhi <abhipatel@google.com>
This commit is contained in:
gemini-cli-robot
2026-03-06 19:29:28 -05:00
committed by GitHub
parent c316fc6c51
commit a6256cdabf
20 changed files with 704 additions and 514 deletions
+51 -18
View File
@@ -8,9 +8,12 @@
import type { Mocked } from 'vitest';
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import { safeJsonStringify } from '../utils/safeJsonStringify.js';
import { DiscoveredMCPTool, generateValidName } from './mcp-tool.js'; // Added getStringifiedResultForDisplay
import type { ToolResult } from './tools.js';
import { ToolConfirmationOutcome } from './tools.js'; // Added ToolConfirmationOutcome
import {
DiscoveredMCPTool,
generateValidName,
formatMcpToolName,
} from './mcp-tool.js'; // Added getStringifiedResultForDisplay
import { ToolConfirmationOutcome, type ToolResult } from './tools.js';
import type { CallableTool, Part } from '@google/genai';
import { ToolErrorType } from './tool-error.js';
import {
@@ -43,23 +46,23 @@ const createSdkResponse = (
describe('generateValidName', () => {
it('should return a valid name for a simple function', () => {
expect(generateValidName('myFunction')).toBe('myFunction');
expect(generateValidName('myFunction')).toBe('mcp_myFunction');
});
it('should replace invalid characters with underscores', () => {
expect(generateValidName('invalid-name with spaces')).toBe(
'invalid-name_with_spaces',
'mcp_invalid-name_with_spaces',
);
});
it('should truncate long names', () => {
expect(generateValidName('x'.repeat(80))).toBe(
'xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx...xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
'mcp_xxxxxxxxxxxxxxxxxxxxxxxxxx...xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx',
);
});
it('should handle names with only invalid characters', () => {
expect(generateValidName('!@#$%^&*()')).toBe('__________');
expect(generateValidName('!@#$%^&*()')).toBe('mcp___________');
});
it.each([
@@ -74,6 +77,30 @@ describe('generateValidName', () => {
);
});
describe('formatMcpToolName', () => {
it('should format a fully qualified name', () => {
expect(formatMcpToolName('github', 'list_repos')).toBe(
'mcp_github_list_repos',
);
});
it('should handle global wildcards', () => {
expect(formatMcpToolName('*')).toBe('mcp_*');
});
it('should handle tool-level wildcards', () => {
expect(formatMcpToolName('github', '*')).toBe('mcp_github_*');
});
it('should handle undefined toolName as a tool-level wildcard', () => {
expect(formatMcpToolName('github')).toBe('mcp_github_*');
});
it('should format explicitly global wildcard with specific tool', () => {
expect(formatMcpToolName('*', 'list_repos')).toBe('mcp_*_list_repos');
});
});
describe('DiscoveredMCPTool', () => {
const serverName = 'mock-mcp-server';
const serverToolName = 'actual-server-tool-name';
@@ -110,8 +137,10 @@ describe('DiscoveredMCPTool', () => {
describe('constructor', () => {
it('should set properties correctly', () => {
expect(tool.name).toBe(serverToolName);
expect(tool.schema.name).toBe(serverToolName);
expect(tool.name).toBe('mcp_mock-mcp-server_actual-server-tool-name');
expect(tool.schema.name).toBe(
'mcp_mock-mcp-server_actual-server-tool-name',
);
expect(tool.schema.description).toBe(baseDescription);
expect(tool.schema.parameters).toBeUndefined();
expect(tool.schema.parametersJsonSchema).toEqual(inputSchema);
@@ -937,31 +966,35 @@ describe('DiscoveredMCPTool', () => {
describe('MCP Tool Naming Regression Fixes', () => {
describe('generateValidName', () => {
it('should replace spaces with underscores', () => {
expect(generateValidName('My Tool')).toBe('My_Tool');
expect(generateValidName('My Tool')).toBe('mcp_My_Tool');
});
it('should allow colons', () => {
expect(generateValidName('namespace:tool')).toBe('namespace:tool');
expect(generateValidName('namespace:tool')).toBe('mcp_namespace:tool');
});
it('should ensure name starts with a letter or underscore', () => {
expect(generateValidName('123-tool')).toBe('_123-tool');
expect(generateValidName('-tool')).toBe('_-tool');
expect(generateValidName('.tool')).toBe('_.tool');
expect(generateValidName('valid_tool_name')).toBe('mcp_valid_tool_name');
expect(generateValidName('alsoValid-123.name')).toBe(
'mcp_alsoValid-123.name',
);
expect(generateValidName('another:valid:name')).toBe(
'mcp_another:valid:name',
);
});
it('should handle very long names by truncating in the middle', () => {
const longName = 'a'.repeat(40) + '__' + 'b'.repeat(40);
const result = generateValidName(longName);
expect(result.length).toBeLessThanOrEqual(63);
expect(result).toMatch(/^a{30}\.\.\.b{30}$/);
expect(result).toMatch(/^mcp_a{26}\.\.\.b{30}$/);
});
it('should handle very long names starting with a digit', () => {
const longName = '1' + 'a'.repeat(80);
const result = generateValidName(longName);
expect(result.length).toBeLessThanOrEqual(63);
expect(result.startsWith('_1')).toBe(true);
expect(result.startsWith('mcp_1')).toBe(true);
});
});
@@ -977,7 +1010,7 @@ describe('MCP Tool Naming Regression Fixes', () => {
);
const qn = tool.getFullyQualifiedName();
expect(qn).toBe('My_Server__my-tool');
expect(qn).toBe('mcp_My_Server_my-tool');
});
it('should handle long server and tool names in qualified name', () => {
@@ -1008,7 +1041,7 @@ describe('MCP Tool Naming Regression Fixes', () => {
);
const qn = tool.getFullyQualifiedName();
expect(qn).toBe('_123-server__tool');
expect(qn).toBe('mcp_123-server_tool');
});
});
});
+98 -11
View File
@@ -5,6 +5,7 @@
*/
import { safeJsonStringify } from '../utils/safeJsonStringify.js';
import { debugLogger } from '../utils/debugLogger.js';
import type {
ToolCallConfirmationDetails,
ToolInvocation,
@@ -25,18 +26,92 @@ import type { McpContext } from './mcp-client.js';
/**
* The separator used to qualify MCP tool names with their server prefix.
* e.g. "server_name__tool_name"
* e.g. "mcp_server_name_tool_name"
*/
export const MCP_QUALIFIED_NAME_SEPARATOR = '__';
export const MCP_QUALIFIED_NAME_SEPARATOR = '_';
/**
* Returns true if `name` matches the MCP qualified name format: "server__tool",
* i.e. exactly two non-empty parts separated by the MCP_QUALIFIED_NAME_SEPARATOR.
* The strict prefix that all MCP tools must start with.
*/
export const MCP_TOOL_PREFIX = 'mcp_';
/**
* Returns true if `name` matches the MCP qualified name format: "mcp_server_tool",
* i.e. starts with the "mcp_" prefix.
*/
export function isMcpToolName(name: string): boolean {
if (!name.includes(MCP_QUALIFIED_NAME_SEPARATOR)) return false;
const parts = name.split(MCP_QUALIFIED_NAME_SEPARATOR);
return parts.length === 2 && parts[0].length > 0 && parts[1].length > 0;
return name.startsWith(MCP_TOOL_PREFIX);
}
/**
* Extracts the server name and tool name from a fully qualified MCP tool name.
* Expected format: `mcp_{server_name}_{tool_name}`
* @param name The fully qualified tool name.
* @returns An object containing the extracted `serverName` and `toolName`, or
* `undefined` properties if the name doesn't match the expected format.
*/
export function parseMcpToolName(name: string): {
serverName?: string;
toolName?: string;
} {
if (!isMcpToolName(name)) {
return {};
}
// Remove the prefix
const withoutPrefix = name.slice(MCP_TOOL_PREFIX.length);
// The first segment is the server name, the rest is the tool name
const match = withoutPrefix.match(/^([^_]+)_(.+)$/);
if (match) {
return {
serverName: match[1],
toolName: match[2],
};
}
return {};
}
/**
* Assembles a fully qualified MCP tool name (or wildcard pattern) from its server and tool components.
*
* @param serverName The backend MCP server name (can be '*' for global wildcards).
* @param toolName The name of the tool (can be undefined or '*' for tool-level wildcards).
* @returns The fully qualified name (e.g., `mcp_server_tool`, `mcp_*`, `mcp_server_*`).
*/
export function formatMcpToolName(
serverName: string,
toolName?: string,
): string {
if (serverName === '*' && !toolName) {
return `${MCP_TOOL_PREFIX}*`;
} else if (serverName === '*') {
return `${MCP_TOOL_PREFIX}*_${toolName}`;
} else if (!toolName) {
return `${MCP_TOOL_PREFIX}${serverName}_*`;
} else {
return `${MCP_TOOL_PREFIX}${serverName}_${toolName}`;
}
}
/**
* Interface representing metadata annotations specific to an MCP tool.
* Ensures strongly-typed access to server-level properties.
*/
export interface McpToolAnnotation extends Record<string, unknown> {
_serverName: string;
}
/**
* Type guard to check if tool annotations implement McpToolAnnotation.
*/
export function isMcpToolAnnotation(
annotation: unknown,
): annotation is McpToolAnnotation {
return (
typeof annotation === 'object' &&
annotation !== null &&
// eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion
typeof (annotation as Record<string, unknown>)['_serverName'] === 'string'
);
}
type ToolParams = Record<string, unknown>;
@@ -276,7 +351,10 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
private readonly _toolAnnotations?: Record<string, unknown>,
) {
super(
generateValidName(nameOverride ?? serverToolName),
nameOverride ??
generateValidName(
`${serverName}${MCP_QUALIFIED_NAME_SEPARATOR}${serverToolName}`,
),
`${serverToolName} (${serverName} MCP Server)`,
description,
Kind.Other,
@@ -304,7 +382,9 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
}
getFullyQualifiedPrefix(): string {
return `${this.serverName}${MCP_QUALIFIED_NAME_SEPARATOR}`;
return generateValidName(
`${this.serverName}${MCP_QUALIFIED_NAME_SEPARATOR}`,
);
}
getFullyQualifiedName(): string {
@@ -495,8 +575,12 @@ const MAX_FUNCTION_NAME_LENGTH = 64;
/** Visible for testing */
export function generateValidName(name: string) {
// Replace invalid characters (based on 400 error message from Gemini API) with underscores
let validToolname = name.replace(/[^a-zA-Z0-9_.:-]/g, '_');
// Enforce the mcp_ prefix for all generated MCP tool names
let validToolname = name.startsWith('mcp_') ? name : `mcp_${name}`;
// Replace invalid characters with underscores to conform to Gemini API:
// ^[a-zA-Z_][a-zA-Z0-9_\-.:]{0,63}$
validToolname = validToolname.replace(/[^a-zA-Z0-9_\-.:]/g, '_');
// Ensure it starts with a letter or underscore
if (/^[^a-zA-Z_]/.test(validToolname)) {
@@ -507,6 +591,9 @@ export function generateValidName(name: string) {
// Note: We use 63 instead of 64 to be safe, as some environments have off-by-one behaviors.
const safeLimit = MAX_FUNCTION_NAME_LENGTH - 1;
if (validToolname.length > safeLimit) {
debugLogger.warn(
`Truncating MCP tool name "${validToolname}" to fit within the 64 character limit. This tool may require user approval.`,
);
validToolname =
validToolname.slice(0, 30) + '...' + validToolname.slice(-30);
}
+31 -33
View File
@@ -13,9 +13,12 @@ import { ApprovalMode } from '../policy/types.js';
import { ToolRegistry, DiscoveredTool } from './tool-registry.js';
import { DISCOVERED_TOOL_PREFIX } from './tool-names.js';
import { DiscoveredMCPTool, MCP_QUALIFIED_NAME_SEPARATOR } from './mcp-tool.js';
import type { FunctionDeclaration, CallableTool } from '@google/genai';
import { mcpToTool } from '@google/genai';
import { DiscoveredMCPTool } from './mcp-tool.js';
import {
mcpToTool,
type FunctionDeclaration,
type CallableTool,
} from '@google/genai';
import { spawn } from 'node:child_process';
import fs from 'node:fs';
@@ -307,7 +310,7 @@ describe('ToolRegistry', () => {
{
name: 'should match qualified MCP tool names when qualified or unqualified',
tools: [mcpTool, mcpTool.asFullyQualifiedTool()],
excludedTools: [`${mcpTool.getFullyQualifiedPrefix()}${mcpTool.name}`],
excludedTools: [mcpTool.name],
},
{
name: 'should match class names',
@@ -395,7 +398,7 @@ describe('ToolRegistry', () => {
// Assert that the returned array contains all tool names, with MCP qualified
expect(toolNames).toContain('c-tool');
expect(toolNames).toContain('a-tool');
expect(toolNames).toContain('my-server__my-tool');
expect(toolNames).toContain('mcp_my-server_my-tool');
expect(toolNames).toHaveLength(3);
});
@@ -409,7 +412,7 @@ describe('ToolRegistry', () => {
toolRegistry.registerTool(mcpTool.asFullyQualifiedTool());
const toolNames = toolRegistry.getAllToolNames();
expect(toolNames).toEqual([`${serverName}__${toolName}`]);
expect(toolNames).toEqual([`mcp_${serverName}_${toolName}`]);
});
});
@@ -439,7 +442,11 @@ describe('ToolRegistry', () => {
// Assert that the array has the correct tools and is sorted by name
expect(toolsFromServer1).toHaveLength(3);
expect(toolNames).toEqual(['apple-tool', 'banana-tool', 'zebra-tool']);
expect(toolNames).toEqual([
'mcp_mcp-server-uno_apple-tool',
'mcp_mcp-server-uno_banana-tool',
'mcp_mcp-server-uno_zebra-tool',
]);
// Assert that all returned tools are indeed from the correct server
for (const tool of toolsFromServer1) {
@@ -481,8 +488,8 @@ describe('ToolRegistry', () => {
'builtin-1',
'builtin-2',
DISCOVERED_TOOL_PREFIX + 'discovered-1',
'apple-server__mcp-apple',
'zebra-server__mcp-zebra',
'mcp_apple-server_mcp-apple',
'mcp_zebra-server_mcp-zebra',
]);
});
});
@@ -598,25 +605,20 @@ describe('ToolRegistry', () => {
});
describe('getTool', () => {
it('should retrieve an MCP tool by its fully qualified name even if registered with simple name', () => {
it('should retrieve an MCP tool by its fully qualified name', () => {
const serverName = 'my-server';
const toolName = 'my-tool';
const mcpTool = createMCPTool(serverName, toolName, 'description');
// Register tool (will be registered as 'my-tool' since no conflict)
// Register tool
toolRegistry.registerTool(mcpTool);
// Verify it is available as 'my-tool'
expect(toolRegistry.getTool('my-tool')).toBeDefined();
expect(toolRegistry.getTool('my-tool')?.name).toBe('my-tool');
// Verify it is available as 'my-server__my-tool'
const fullyQualifiedName = `${serverName}__${toolName}`;
// Verify it is available as 'mcp_my-server_my-tool'
const fullyQualifiedName = `mcp_${serverName}_${toolName}`;
const retrievedTool = toolRegistry.getTool(fullyQualifiedName);
expect(retrievedTool).toBeDefined();
// The returned tool object is the same, so its name property is still 'my-tool'
expect(retrievedTool?.name).toBe('my-tool');
expect(retrievedTool?.name).toBe(fullyQualifiedName);
});
it('should retrieve an MCP tool by its fully qualified name when tool name has special characters', () => {
@@ -626,19 +628,15 @@ describe('ToolRegistry', () => {
const validToolName = 'my_tool';
const mcpTool = createMCPTool(serverName, toolName, 'description');
// Register tool (will be registered as sanitized name)
// Register tool
toolRegistry.registerTool(mcpTool);
// Verify it is available as sanitized name
expect(toolRegistry.getTool(validToolName)).toBeDefined();
expect(toolRegistry.getTool(validToolName)?.name).toBe(validToolName);
// Verify it is available as 'my-server__my_tool'
const fullyQualifiedName = `${serverName}__${validToolName}`;
// Verify it is available as 'mcp_my-server_my_tool'
const fullyQualifiedName = `mcp_${serverName}_${validToolName}`;
const retrievedTool = toolRegistry.getTool(fullyQualifiedName);
expect(retrievedTool).toBeDefined();
expect(retrievedTool?.name).toBe(validToolName);
expect(retrievedTool?.name).toBe(fullyQualifiedName);
});
it('should resolve qualified names in getFunctionDeclarationsFiltered', () => {
@@ -648,13 +646,13 @@ describe('ToolRegistry', () => {
toolRegistry.registerTool(mcpTool);
const fullyQualifiedName = `${serverName}${MCP_QUALIFIED_NAME_SEPARATOR}${toolName}`;
const fullyQualifiedName = `mcp_${serverName}_${toolName}`;
const declarations = toolRegistry.getFunctionDeclarationsFiltered([
fullyQualifiedName,
]);
expect(declarations).toHaveLength(1);
expect(declarations[0].name).toBe(toolName);
expect(declarations[0].name).toBe(fullyQualifiedName);
});
it('should retrieve a tool using its legacy alias', async () => {
@@ -685,7 +683,7 @@ describe('ToolRegistry', () => {
const declarations = toolRegistry.getFunctionDeclarations();
expect(declarations).toHaveLength(1);
expect(declarations[0].name).toBe(`${serverName}__${toolName}`);
expect(declarations[0].name).toBe(`mcp_${serverName}_${toolName}`);
});
it('should deduplicate MCP tools in declarations', () => {
@@ -699,7 +697,7 @@ describe('ToolRegistry', () => {
const declarations = toolRegistry.getFunctionDeclarations();
expect(declarations).toHaveLength(1);
expect(declarations[0].name).toBe(`${serverName}__${toolName}`);
expect(declarations[0].name).toBe(`mcp_${serverName}_${toolName}`);
});
});
@@ -752,7 +750,7 @@ describe('ToolRegistry', () => {
const allTools = toolRegistry.getAllTools();
const toolNames = allTools.map((t) => t.name);
expect(toolNames).toContain('read-only-tool');
expect(toolNames).toContain('mcp_test-server_read-only-tool');
});
it('should exclude non-read-only MCP tools when denied by policy in plan mode', () => {
@@ -769,7 +767,7 @@ describe('ToolRegistry', () => {
const allTools = toolRegistry.getAllTools();
const toolNames = allTools.map((t) => t.name);
expect(toolNames).not.toContain('write-mcp-tool');
expect(toolNames).not.toContain('mcp_test-server_write-mcp-tool');
});
});
+9 -7
View File
@@ -443,13 +443,15 @@ export class ToolRegistry {
private buildToolMetadata(): Map<string, Record<string, unknown>> {
const toolMetadata = new Map<string, Record<string, unknown>>();
for (const [name, tool] of this.allKnownTools) {
if (tool.toolAnnotations) {
const metadata: Record<string, unknown> = { ...tool.toolAnnotations };
// Include server name so the policy engine can resolve composite
// wildcard patterns (e.g. "*__*") against unqualified tool names.
if (tool instanceof DiscoveredMCPTool) {
metadata['_serverName'] = tool.serverName;
}
const metadata: Record<string, unknown> = tool.toolAnnotations
? { ...tool.toolAnnotations }
: {};
// Include server name so the policy engine can resolve composite
// wildcard patterns (e.g. "*__*") against unqualified tool names.
if (tool instanceof DiscoveredMCPTool) {
metadata['_serverName'] = tool.serverName;
}
if (Object.keys(metadata).length > 0) {
toolMetadata.set(name, metadata);
}
}