mirror of
https://github.com/google-gemini/gemini-cli.git
synced 2026-03-16 00:51:25 -07:00
fix(core): resolve MCP tool FQN validation, schema export, and wildcards in subagents (#22069)
This commit is contained in:
@@ -107,7 +107,9 @@ const localAgentSchema = z
|
|||||||
display_name: z.string().optional(),
|
display_name: z.string().optional(),
|
||||||
tools: z
|
tools: z
|
||||||
.array(
|
.array(
|
||||||
z.string().refine((val) => isValidToolName(val), {
|
z
|
||||||
|
.string()
|
||||||
|
.refine((val) => isValidToolName(val, { allowWildcards: true }), {
|
||||||
message: 'Invalid tool name',
|
message: 'Invalid tool name',
|
||||||
}),
|
}),
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -17,7 +17,13 @@ import {
|
|||||||
type Schema,
|
type Schema,
|
||||||
} from '@google/genai';
|
} from '@google/genai';
|
||||||
import { ToolRegistry } from '../tools/tool-registry.js';
|
import { ToolRegistry } from '../tools/tool-registry.js';
|
||||||
import { DiscoveredMCPTool } from '../tools/mcp-tool.js';
|
import { type AnyDeclarativeTool } from '../tools/tools.js';
|
||||||
|
import {
|
||||||
|
DiscoveredMCPTool,
|
||||||
|
isMcpToolName,
|
||||||
|
parseMcpToolName,
|
||||||
|
MCP_TOOL_PREFIX,
|
||||||
|
} from '../tools/mcp-tool.js';
|
||||||
import { CompressionStatus } from '../core/turn.js';
|
import { CompressionStatus } from '../core/turn.js';
|
||||||
import { type ToolCallRequestInfo } from '../scheduler/types.js';
|
import { type ToolCallRequestInfo } from '../scheduler/types.js';
|
||||||
import { type Message } from '../confirmation-bus/types.js';
|
import { type Message } from '../confirmation-bus/types.js';
|
||||||
@@ -146,28 +152,55 @@ export class LocalAgentExecutor<TOutput extends z.ZodTypeAny> {
|
|||||||
context.config.getAgentRegistry().getAllAgentNames(),
|
context.config.getAgentRegistry().getAllAgentNames(),
|
||||||
);
|
);
|
||||||
|
|
||||||
const registerToolByName = (toolName: string) => {
|
const registerToolInstance = (tool: AnyDeclarativeTool) => {
|
||||||
// Check if the tool is a subagent to prevent recursion.
|
// Check if the tool is a subagent to prevent recursion.
|
||||||
// We do not allow agents to call other agents.
|
// We do not allow agents to call other agents.
|
||||||
if (allAgentNames.has(toolName)) {
|
if (allAgentNames.has(tool.name)) {
|
||||||
debugLogger.warn(
|
debugLogger.warn(
|
||||||
`[LocalAgentExecutor] Skipping subagent tool '${toolName}' for agent '${definition.name}' to prevent recursion.`,
|
`[LocalAgentExecutor] Skipping subagent tool '${tool.name}' for agent '${definition.name}' to prevent recursion.`,
|
||||||
);
|
);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
agentToolRegistry.registerTool(tool);
|
||||||
|
};
|
||||||
|
|
||||||
|
const registerToolByName = (toolName: string) => {
|
||||||
|
// Handle global wildcard
|
||||||
|
if (toolName === '*') {
|
||||||
|
for (const tool of parentToolRegistry.getAllTools()) {
|
||||||
|
registerToolInstance(tool);
|
||||||
|
}
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Handle MCP wildcards
|
||||||
|
if (isMcpToolName(toolName)) {
|
||||||
|
if (toolName === `${MCP_TOOL_PREFIX}*`) {
|
||||||
|
for (const tool of parentToolRegistry.getAllTools()) {
|
||||||
|
if (tool instanceof DiscoveredMCPTool) {
|
||||||
|
registerToolInstance(tool);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
const parsed = parseMcpToolName(toolName);
|
||||||
|
if (parsed.serverName && parsed.toolName === '*') {
|
||||||
|
for (const tool of parentToolRegistry.getToolsByServer(
|
||||||
|
parsed.serverName,
|
||||||
|
)) {
|
||||||
|
registerToolInstance(tool);
|
||||||
|
}
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// If the tool is referenced by name, retrieve it from the parent
|
// If the tool is referenced by name, retrieve it from the parent
|
||||||
// registry and register it with the agent's isolated registry.
|
// registry and register it with the agent's isolated registry.
|
||||||
const tool = parentToolRegistry.getTool(toolName);
|
const tool = parentToolRegistry.getTool(toolName);
|
||||||
if (tool) {
|
if (tool) {
|
||||||
if (tool instanceof DiscoveredMCPTool) {
|
registerToolInstance(tool);
|
||||||
// Subagents MUST use fully qualified names for MCP tools to ensure
|
|
||||||
// unambiguous tool calls and to comply with policy requirements.
|
|
||||||
// We automatically "upgrade" any MCP tool to its qualified version.
|
|
||||||
agentToolRegistry.registerTool(tool.asFullyQualifiedTool());
|
|
||||||
} else {
|
|
||||||
agentToolRegistry.registerTool(tool);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -1175,10 +1208,9 @@ export class LocalAgentExecutor<TOutput extends z.ZodTypeAny> {
|
|||||||
const { toolConfig, outputConfig } = this.definition;
|
const { toolConfig, outputConfig } = this.definition;
|
||||||
|
|
||||||
if (toolConfig) {
|
if (toolConfig) {
|
||||||
const toolNamesToLoad: string[] = [];
|
|
||||||
for (const toolRef of toolConfig.tools) {
|
for (const toolRef of toolConfig.tools) {
|
||||||
if (typeof toolRef === 'string') {
|
if (typeof toolRef === 'string') {
|
||||||
toolNamesToLoad.push(toolRef);
|
// The names were already expanded and loaded into the registry during creation.
|
||||||
} else if (typeof toolRef === 'object' && 'schema' in toolRef) {
|
} else if (typeof toolRef === 'object' && 'schema' in toolRef) {
|
||||||
// Tool instance with an explicit schema property.
|
// Tool instance with an explicit schema property.
|
||||||
toolsList.push(toolRef.schema);
|
toolsList.push(toolRef.schema);
|
||||||
@@ -1187,10 +1219,8 @@ export class LocalAgentExecutor<TOutput extends z.ZodTypeAny> {
|
|||||||
toolsList.push(toolRef);
|
toolsList.push(toolRef);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
// Add schemas from tools that were registered by name.
|
// Add schemas from tools that were explicitly registered by name or wildcard.
|
||||||
toolsList.push(
|
toolsList.push(...this.toolRegistry.getFunctionDeclarations());
|
||||||
...this.toolRegistry.getFunctionDeclarationsFiltered(toolNamesToLoad),
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// Always inject complete_task.
|
// Always inject complete_task.
|
||||||
|
|||||||
@@ -58,6 +58,7 @@ export function parseMcpToolName(name: string): {
|
|||||||
// Remove the prefix
|
// Remove the prefix
|
||||||
const withoutPrefix = name.slice(MCP_TOOL_PREFIX.length);
|
const withoutPrefix = name.slice(MCP_TOOL_PREFIX.length);
|
||||||
// The first segment is the server name, the rest is the tool name
|
// The first segment is the server name, the rest is the tool name
|
||||||
|
// Must be strictly `server_tool` where neither are empty
|
||||||
const match = withoutPrefix.match(/^([^_]+)_(.+)$/);
|
const match = withoutPrefix.match(/^([^_]+)_(.+)$/);
|
||||||
if (match) {
|
if (match) {
|
||||||
return {
|
return {
|
||||||
@@ -390,25 +391,6 @@ export class DiscoveredMCPTool extends BaseDeclarativeTool<
|
|||||||
`${this.serverName}${MCP_QUALIFIED_NAME_SEPARATOR}${this.serverToolName}`,
|
`${this.serverName}${MCP_QUALIFIED_NAME_SEPARATOR}${this.serverToolName}`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
asFullyQualifiedTool(): DiscoveredMCPTool {
|
|
||||||
return new DiscoveredMCPTool(
|
|
||||||
this.mcpTool,
|
|
||||||
this.serverName,
|
|
||||||
this.serverToolName,
|
|
||||||
this.description,
|
|
||||||
this.parameterSchema,
|
|
||||||
this.messageBus,
|
|
||||||
this.trust,
|
|
||||||
this.isReadOnly,
|
|
||||||
this.getFullyQualifiedName(),
|
|
||||||
this.cliConfig,
|
|
||||||
this.extensionName,
|
|
||||||
this.extensionId,
|
|
||||||
this._toolAnnotations,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
protected createInvocation(
|
protected createInvocation(
|
||||||
params: ToolParams,
|
params: ToolParams,
|
||||||
messageBus: MessageBus,
|
messageBus: MessageBus,
|
||||||
|
|||||||
@@ -25,7 +25,8 @@ vi.mock('./tool-names.js', async (importOriginal) => {
|
|||||||
...actual,
|
...actual,
|
||||||
TOOL_LEGACY_ALIASES: mockedAliases,
|
TOOL_LEGACY_ALIASES: mockedAliases,
|
||||||
isValidToolName: vi.fn().mockImplementation((name: string, options) => {
|
isValidToolName: vi.fn().mockImplementation((name: string, options) => {
|
||||||
if (mockedAliases[name]) return true;
|
if (Object.prototype.hasOwnProperty.call(mockedAliases, name))
|
||||||
|
return true;
|
||||||
return actual.isValidToolName(name, options);
|
return actual.isValidToolName(name, options);
|
||||||
}),
|
}),
|
||||||
getToolAliases: vi.fn().mockImplementation((name: string) => {
|
getToolAliases: vi.fn().mockImplementation((name: string) => {
|
||||||
@@ -55,11 +56,9 @@ describe('tool-names', () => {
|
|||||||
expect(isValidToolName(`${DISCOVERED_TOOL_PREFIX}my_tool`)).toBe(true);
|
expect(isValidToolName(`${DISCOVERED_TOOL_PREFIX}my_tool`)).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should validate MCP tool names (server__tool)', () => {
|
it('should validate modern MCP FQNs (mcp_server_tool)', () => {
|
||||||
expect(isValidToolName('server__tool')).toBe(true);
|
expect(isValidToolName('mcp_server_tool')).toBe(true);
|
||||||
expect(isValidToolName('my-server__my-tool')).toBe(true);
|
expect(isValidToolName('mcp_my-server_my-tool')).toBe(true);
|
||||||
expect(isValidToolName('my.server__my:tool')).toBe(true);
|
|
||||||
expect(isValidToolName('my-server...truncated__tool')).toBe(true);
|
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should validate legacy tool aliases', async () => {
|
it('should validate legacy tool aliases', async () => {
|
||||||
@@ -69,28 +68,33 @@ describe('tool-names', () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should reject invalid tool names', () => {
|
it('should return false for invalid tool names', () => {
|
||||||
expect(isValidToolName('')).toBe(false);
|
expect(isValidToolName('invalid-tool-name')).toBe(false);
|
||||||
expect(isValidToolName('invalid-name')).toBe(false);
|
expect(isValidToolName('mcp_server')).toBe(false);
|
||||||
expect(isValidToolName('server__')).toBe(false);
|
expect(isValidToolName('mcp__tool')).toBe(false);
|
||||||
expect(isValidToolName('__tool')).toBe(false);
|
expect(isValidToolName('mcp_invalid server_tool')).toBe(false);
|
||||||
expect(isValidToolName('server__tool__extra')).toBe(false);
|
expect(isValidToolName('mcp_server_invalid tool')).toBe(false);
|
||||||
|
expect(isValidToolName('mcp_server_')).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
it('should handle wildcards when allowed', () => {
|
it('should handle wildcards when allowed', () => {
|
||||||
// Default: not allowed
|
// Default: not allowed
|
||||||
expect(isValidToolName('*')).toBe(false);
|
expect(isValidToolName('*')).toBe(false);
|
||||||
expect(isValidToolName('server__*')).toBe(false);
|
expect(isValidToolName('mcp_*')).toBe(false);
|
||||||
|
expect(isValidToolName('mcp_server_*')).toBe(false);
|
||||||
|
|
||||||
// Explicitly allowed
|
// Explicitly allowed
|
||||||
expect(isValidToolName('*', { allowWildcards: true })).toBe(true);
|
expect(isValidToolName('*', { allowWildcards: true })).toBe(true);
|
||||||
expect(isValidToolName('server__*', { allowWildcards: true })).toBe(true);
|
expect(isValidToolName('mcp_*', { allowWildcards: true })).toBe(true);
|
||||||
|
expect(isValidToolName('mcp_server_*', { allowWildcards: true })).toBe(
|
||||||
|
true,
|
||||||
|
);
|
||||||
|
|
||||||
// Invalid wildcards
|
// Invalid wildcards
|
||||||
expect(isValidToolName('__*', { allowWildcards: true })).toBe(false);
|
expect(isValidToolName('mcp__*', { allowWildcards: true })).toBe(false);
|
||||||
expect(isValidToolName('server__tool*', { allowWildcards: true })).toBe(
|
expect(
|
||||||
false,
|
isValidToolName('mcp_server_tool*', { allowWildcards: true }),
|
||||||
);
|
).toBe(false);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
|||||||
@@ -221,6 +221,12 @@ export const DISCOVERED_TOOL_PREFIX = 'discovered_tool_';
|
|||||||
/**
|
/**
|
||||||
* List of all built-in tool names.
|
* List of all built-in tool names.
|
||||||
*/
|
*/
|
||||||
|
import {
|
||||||
|
isMcpToolName,
|
||||||
|
parseMcpToolName,
|
||||||
|
MCP_TOOL_PREFIX,
|
||||||
|
} from './mcp-tool.js';
|
||||||
|
|
||||||
export const ALL_BUILTIN_TOOL_NAMES = [
|
export const ALL_BUILTIN_TOOL_NAMES = [
|
||||||
GLOB_TOOL_NAME,
|
GLOB_TOOL_NAME,
|
||||||
WRITE_TODOS_TOOL_NAME,
|
WRITE_TODOS_TOOL_NAME,
|
||||||
@@ -290,25 +296,44 @@ export function isValidToolName(
|
|||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
// MCP tools (format: server__tool)
|
// Handle standard MCP FQNs (mcp_server_tool or wildcards mcp_*, mcp_server_*)
|
||||||
if (name.includes('__')) {
|
if (isMcpToolName(name)) {
|
||||||
const parts = name.split('__');
|
// Global wildcard: mcp_*
|
||||||
if (parts.length !== 2 || parts[0].length === 0 || parts[1].length === 0) {
|
if (name === `${MCP_TOOL_PREFIX}*` && options.allowWildcards) {
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Explicitly reject names with empty server component (e.g. mcp__tool)
|
||||||
|
if (name.startsWith(`${MCP_TOOL_PREFIX}_`)) {
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
const server = parts[0];
|
const parsed = parseMcpToolName(name);
|
||||||
const tool = parts[1];
|
// Ensure that both components are populated. parseMcpToolName splits at the second _,
|
||||||
|
// so `mcp__tool` has serverName="", toolName="tool"
|
||||||
if (tool === '*') {
|
if (parsed.serverName && parsed.toolName) {
|
||||||
return !!options.allowWildcards;
|
|
||||||
}
|
|
||||||
|
|
||||||
// Basic slug validation for server and tool names.
|
// Basic slug validation for server and tool names.
|
||||||
// We allow dots (.) and colons (:) as they are valid in function names and
|
// We allow dots (.) and colons (:) as they are valid in function names and
|
||||||
// used for truncation markers.
|
// used for truncation markers.
|
||||||
const slugRegex = /^[a-z0-9_.:-]+$/i;
|
const slugRegex = /^[a-z0-9_.:-]+$/i;
|
||||||
return slugRegex.test(server) && slugRegex.test(tool);
|
|
||||||
|
if (!slugRegex.test(parsed.serverName)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (parsed.toolName === '*') {
|
||||||
|
return options.allowWildcards === true;
|
||||||
|
}
|
||||||
|
|
||||||
|
// A tool name consisting only of underscores is invalid.
|
||||||
|
if (/^_*$/.test(parsed.toolName)) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
return slugRegex.test(parsed.toolName);
|
||||||
|
}
|
||||||
|
|
||||||
|
return false;
|
||||||
}
|
}
|
||||||
|
|
||||||
return false;
|
return false;
|
||||||
|
|||||||
@@ -310,13 +310,13 @@ describe('ToolRegistry', () => {
|
|||||||
excludedTools: ['tool-a'],
|
excludedTools: ['tool-a'],
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: 'should match simple MCP tool names, when qualified or unqualified',
|
name: 'should match simple MCP tool names',
|
||||||
tools: [mcpTool, mcpTool.asFullyQualifiedTool()],
|
tools: [mcpTool],
|
||||||
excludedTools: [mcpTool.name],
|
excludedTools: [mcpTool.name],
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: 'should match qualified MCP tool names when qualified or unqualified',
|
name: 'should match qualified MCP tool names',
|
||||||
tools: [mcpTool, mcpTool.asFullyQualifiedTool()],
|
tools: [mcpTool],
|
||||||
excludedTools: [mcpTool.name],
|
excludedTools: [mcpTool.name],
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
@@ -414,9 +414,9 @@ describe('ToolRegistry', () => {
|
|||||||
const toolName = 'my-tool';
|
const toolName = 'my-tool';
|
||||||
const mcpTool = createMCPTool(serverName, toolName, 'desc');
|
const mcpTool = createMCPTool(serverName, toolName, 'desc');
|
||||||
|
|
||||||
// Register same MCP tool twice (one as alias, one as qualified)
|
// Register same MCP tool twice
|
||||||
|
toolRegistry.registerTool(mcpTool);
|
||||||
toolRegistry.registerTool(mcpTool);
|
toolRegistry.registerTool(mcpTool);
|
||||||
toolRegistry.registerTool(mcpTool.asFullyQualifiedTool());
|
|
||||||
|
|
||||||
const toolNames = toolRegistry.getAllToolNames();
|
const toolNames = toolRegistry.getAllToolNames();
|
||||||
expect(toolNames).toEqual([`mcp_${serverName}_${toolName}`]);
|
expect(toolNames).toEqual([`mcp_${serverName}_${toolName}`]);
|
||||||
@@ -698,9 +698,8 @@ describe('ToolRegistry', () => {
|
|||||||
const toolName = 'my-tool';
|
const toolName = 'my-tool';
|
||||||
const mcpTool = createMCPTool(serverName, toolName, 'description');
|
const mcpTool = createMCPTool(serverName, toolName, 'description');
|
||||||
|
|
||||||
// Register both alias and qualified
|
|
||||||
toolRegistry.registerTool(mcpTool);
|
toolRegistry.registerTool(mcpTool);
|
||||||
toolRegistry.registerTool(mcpTool.asFullyQualifiedTool());
|
toolRegistry.registerTool(mcpTool);
|
||||||
|
|
||||||
const declarations = toolRegistry.getFunctionDeclarations();
|
const declarations = toolRegistry.getFunctionDeclarations();
|
||||||
expect(declarations).toHaveLength(1);
|
expect(declarations).toHaveLength(1);
|
||||||
|
|||||||
@@ -222,15 +222,11 @@ export class ToolRegistry {
|
|||||||
*/
|
*/
|
||||||
registerTool(tool: AnyDeclarativeTool): void {
|
registerTool(tool: AnyDeclarativeTool): void {
|
||||||
if (this.allKnownTools.has(tool.name)) {
|
if (this.allKnownTools.has(tool.name)) {
|
||||||
if (tool instanceof DiscoveredMCPTool) {
|
|
||||||
tool = tool.asFullyQualifiedTool();
|
|
||||||
} else {
|
|
||||||
// Decide on behavior: throw error, log warning, or allow overwrite
|
// Decide on behavior: throw error, log warning, or allow overwrite
|
||||||
debugLogger.warn(
|
debugLogger.warn(
|
||||||
`Tool with name "${tool.name}" is already registered. Overwriting.`,
|
`Tool with name "${tool.name}" is already registered. Overwriting.`,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
|
||||||
this.allKnownTools.set(tool.name, tool);
|
this.allKnownTools.set(tool.name, tool);
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -594,7 +590,17 @@ export class ToolRegistry {
|
|||||||
for (const name of toolNames) {
|
for (const name of toolNames) {
|
||||||
const tool = this.getTool(name);
|
const tool = this.getTool(name);
|
||||||
if (tool) {
|
if (tool) {
|
||||||
declarations.push(tool.getSchema(modelId));
|
let schema = tool.getSchema(modelId);
|
||||||
|
|
||||||
|
// Ensure the schema name matches the qualified name for MCP tools
|
||||||
|
if (tool instanceof DiscoveredMCPTool) {
|
||||||
|
schema = {
|
||||||
|
...schema,
|
||||||
|
name: tool.getFullyQualifiedName(),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
|
||||||
|
declarations.push(schema);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return declarations;
|
return declarations;
|
||||||
@@ -670,17 +676,6 @@ export class ToolRegistry {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!tool && name.includes('__')) {
|
|
||||||
for (const t of this.allKnownTools.values()) {
|
|
||||||
if (t instanceof DiscoveredMCPTool) {
|
|
||||||
if (t.getFullyQualifiedName() === name) {
|
|
||||||
tool = t;
|
|
||||||
break;
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
if (tool && this.isActiveTool(tool)) {
|
if (tool && this.isActiveTool(tool)) {
|
||||||
return tool;
|
return tool;
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user