From afeca176f4ef7d227831715b5e5a672fcf3fe58f Mon Sep 17 00:00:00 2001 From: Matt Cowger Date: Tue, 21 Oct 2025 19:30:30 -0700 Subject: [PATCH 1/2] feat: implement dynamic MCP tool name support with triple underscore separator - Add helper functions to detect and parse dynamic MCP tool names - Normalize dynamic tools to standard 'use_mcp_tool' format - Use triple underscores (___) as separator to allow underscores in tool names - Update tool name generation in mcp_server.ts - Add comprehensive test coverage for dynamic tool parsing - Extract toolInputProps and convert to JSON string for MCP server compatibility --- .changeset/green-melons-retire.md | 5 + .../experimental/native-function-calling.md | 14 +- .../AssistantMessageParser.ts | 43 +++- .../__tests__/AssistantMessageParser.spec.ts | 227 ++++++++++++++++++ .../kilocode/native-tool-call.ts | 45 ++++ src/core/prompts/sections/mcp-servers.ts | 16 +- src/core/prompts/system.ts | 7 +- .../getAllowedJSONToolsForMode.ts | 79 ++++-- .../prompts/tools/native-tools/mcp_server.ts | 105 ++++++++ src/core/task/Task.ts | 9 +- src/core/tools/useMcpToolTool.ts | 29 ++- .../src/components/chat/McpExecution.tsx | 8 +- 12 files changed, 545 insertions(+), 42 deletions(-) create mode 100644 .changeset/green-melons-retire.md create mode 100644 src/core/assistant-message/kilocode/__tests__/AssistantMessageParser.spec.ts create mode 100644 src/core/prompts/tools/native-tools/mcp_server.ts diff --git a/.changeset/green-melons-retire.md b/.changeset/green-melons-retire.md new file mode 100644 index 0000000000..65f12f4ddd --- /dev/null +++ b/.changeset/green-melons-retire.md @@ -0,0 +1,5 @@ +--- +"kilo-code": minor +--- + +Add Native MCP Support for JSON Tool Calling diff --git a/apps/kilocode-docs/docs/features/experimental/native-function-calling.md b/apps/kilocode-docs/docs/features/experimental/native-function-calling.md index 4d9b56d06d..1fa194c242 100644 --- a/apps/kilocode-docs/docs/features/experimental/native-function-calling.md +++ b/apps/kilocode-docs/docs/features/experimental/native-function-calling.md @@ -41,12 +41,16 @@ Because of these risks and considerations, this capability is experiment, and of To enable and use native function calling, consider and perform the following: -1. Ensure you are using a provider that has been enabled in Kilo Code for this experiment. As of Oct 16, 2025, they include: +1. Ensure you are using a provider that has been enabled in Kilo Code for this experiment. As of Oct 21, 2025, they include: - OpenRouter - Kilo Code - LM Studio - OpenAI Compatible +- Z.ai +- Synthetic +- X.ai +- Chutes By default, native function calling is _disabled_ for most models. Should you wish to try it, open the Advanced settings for a given provider profile that is included in the testing group. @@ -55,11 +59,13 @@ Change the Tool Calling Style to `JSON`, and save the profile. ## Caveats This feature is currently experimental and mostly intended for users interested in contributing to its development. -It is so far only supported when using OpenRouter or Kilo Code providers. There are possible issues including, but not limited to: -- Missing tools +There are possible issues including, but not limited to: + +- ~~Missing tools~~: As of Oct 21, all tools are supported - Tools calls not updating the UI until they are complete -- MCP servers not working +- ~~MCP servers not working~~: As of Oct 21, MCPs are supported - Errors specific to certain inference providers + - Not all inference providers use servers that are fully compatible with the OpenAI specification. As a result, behavior will vary, even with the same model across providers. While nearly any provider can be configured via the OpenAI Compatible profile, testers should be aware that this is enabled purely for ease of testing and should be prepared to experience unexpected responses from providers that are not prepared to handle native function calls. diff --git a/src/core/assistant-message/AssistantMessageParser.ts b/src/core/assistant-message/AssistantMessageParser.ts index 68d34f078e..400c36ccf8 100644 --- a/src/core/assistant-message/AssistantMessageParser.ts +++ b/src/core/assistant-message/AssistantMessageParser.ts @@ -1,7 +1,7 @@ import { type ToolName, toolNames } from "@roo-code/types" import { TextContent, ToolUse, ToolParamName, toolParamNames } from "../../shared/tools" import { AssistantMessageContent } from "./parseAssistantMessage" -import { NativeToolCall, parseDoubleEncodedParams } from "./kilocode/native-tool-call" +import { extractMcpToolInfo, NativeToolCall, parseDoubleEncodedParams } from "./kilocode/native-tool-call" /** * Parser for assistant messages. Maintains state between chunks @@ -118,8 +118,11 @@ export class AssistantMessageParser { if (toolCall.function?.name) { const toolName = toolCall.function.name - // Validate that this is a recognized tool name - if (!toolNames.includes(toolName as ToolName)) { + // Check if it's a dynamic MCP tool or a recognized static tool name + const mcpToolInfo = extractMcpToolInfo(toolName) + const isValidTool = mcpToolInfo !== null || toolNames.includes(toolName as ToolName) + + if (!isValidTool) { console.warn("[AssistantMessageParser] Unknown tool name in native call:", toolName) continue } @@ -175,17 +178,46 @@ export class AssistantMessageParser { // Tool call is complete - convert it to ToolUse format if (isComplete) { const toolName = accumulatedCall.function!.name + // Finalize any current text content before adding tool use if (this.currentTextContent) { this.currentTextContent.partial = false this.currentTextContent = undefined } + // Normalize dynamic MCP tool names to "use_mcp_tool" + // Dynamic tools have format: use_mcp_tool_{serverName}_{toolName} + const mcpToolInfo = extractMcpToolInfo(toolName) + let normalizedToolName: ToolName + let normalizedParams = parsedArgs + + if (mcpToolInfo) { + // Dynamic MCP tool - normalize to "use_mcp_tool" + // Tool name format: use_mcp_tool___{serverName}___{toolName} + normalizedToolName = "use_mcp_tool" + + // Extract toolInputProps and convert to JSON string for the arguments parameter + // The model provides: { server_name, tool_name, toolInputProps: {...actual args...} } + // We need: { server_name, tool_name, arguments: "{...actual args as JSON string...}" } + const toolInputProps = (parsedArgs as any).toolInputProps || {} + const argumentsJson = JSON.stringify(toolInputProps) + + // Add server_name, tool_name, and arguments to params + normalizedParams = { + server_name: parsedArgs.server_name || mcpToolInfo.serverName, + tool_name: parsedArgs.tool_name || mcpToolInfo.toolName, + arguments: argumentsJson, + } + } else { + // Standard tool + normalizedToolName = toolName as ToolName + } + // Create a ToolUse block from the native tool call const toolUse: ToolUse = { type: "tool_use", - name: toolName as ToolName, - params: parsedArgs, + name: normalizedToolName, + params: normalizedParams, partial: false, // Now complete after accumulation } @@ -198,6 +230,7 @@ export class AssistantMessageParser { } } } + // kilocode_change end /** diff --git a/src/core/assistant-message/kilocode/__tests__/AssistantMessageParser.spec.ts b/src/core/assistant-message/kilocode/__tests__/AssistantMessageParser.spec.ts new file mode 100644 index 0000000000..998817dcf1 --- /dev/null +++ b/src/core/assistant-message/kilocode/__tests__/AssistantMessageParser.spec.ts @@ -0,0 +1,227 @@ +// npx vitest src/core/assistant-message/kilocode/__tests__/AssistantMessageParser.spec.ts + +import { AssistantMessageParser } from "../../AssistantMessageParser" +import { ToolUse } from "../../../../shared/tools" + +describe("AssistantMessageParser (streaming)", () => { + let parser: AssistantMessageParser + + beforeEach(() => { + parser = new AssistantMessageParser() + }) + describe("AssistantMessageParser (native tool calls)", () => { + let parser: AssistantMessageParser + + beforeEach(() => { + parser = new AssistantMessageParser() + }) + + describe("dynamic MCP tool name handling", () => { + it("should normalize dynamic MCP tool names to use_mcp_tool", () => { + const toolCalls = [ + { + id: "call_123", + type: "function" as const, + function: { + name: "use_mcp_tool___context7___get-library-docs", + arguments: JSON.stringify({ + toolInputProps: { + context7CompatibleLibraryID: "/vercel/next.js", + topic: "routing", + }, + }), + }, + }, + ] + + parser.processNativeToolCalls(toolCalls) + const blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(1) + const toolUse = blocks[0] as ToolUse + expect(toolUse.type).toBe("tool_use") + expect(toolUse.name).toBe("use_mcp_tool") + expect(toolUse.params.server_name).toBe("context7") + expect(toolUse.params.tool_name).toBe("get-library-docs") + // Verify arguments contains the toolInputProps as a JSON string + expect(toolUse.params.arguments).toBeDefined() + const parsedArgs = JSON.parse(toolUse.params.arguments!) + expect(parsedArgs.context7CompatibleLibraryID).toBe("/vercel/next.js") + expect(parsedArgs.topic).toBe("routing") + expect(toolUse.partial).toBe(false) + }) + + it("should handle dynamic MCP tool names with underscores in tool name", () => { + const toolCalls = [ + { + id: "call_456", + type: "function" as const, + function: { + name: "use_mcp_tool___myserver___get_user_data", + arguments: JSON.stringify({ + toolInputProps: { + userId: "123", + }, + }), + }, + }, + ] + + parser.processNativeToolCalls(toolCalls) + const blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(1) + const toolUse = blocks[0] as ToolUse + expect(toolUse.name).toBe("use_mcp_tool") + expect(toolUse.params.server_name).toBe("myserver") + expect(toolUse.params.tool_name).toBe("get_user_data") + // Verify arguments contains the toolInputProps as a JSON string + const parsedArgs = JSON.parse(toolUse.params.arguments!) + expect(parsedArgs.userId).toBe("123") + }) + + it("should reject malformed dynamic MCP tool names (no triple underscore separator)", () => { + const toolCalls = [ + { + id: "call_789", + type: "function" as const, + function: { + name: "use_mcp_tool___notripleunderscoreseparator", + arguments: JSON.stringify({}), + }, + }, + ] + + // Mock console.warn to verify it's called + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => {}) + + parser.processNativeToolCalls(toolCalls) + const blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(0) + expect(warnSpy).toHaveBeenCalledWith( + "[AssistantMessageParser] Unknown tool name in native call:", + "use_mcp_tool___notripleunderscoreseparator", + ) + + warnSpy.mockRestore() + }) + + it("should handle streaming dynamic MCP tool calls", () => { + // First delta: function name + const delta1 = [ + { + id: "call_stream", + index: 0, + type: "function" as const, + function: { + name: "use_mcp_tool___weather___get_forecast", + arguments: "", + }, + }, + ] + + // Second delta: partial arguments (name can be empty string during streaming) + const delta2 = [ + { + index: 0, + type: "function" as const, + function: { + name: "", + arguments: '{"toolInputProps": {"city": "San', + }, + }, + ] + + // Third delta: complete arguments + const delta3 = [ + { + index: 0, + type: "function" as const, + function: { + name: "", + arguments: ' Francisco"}}', + }, + }, + ] + + parser.processNativeToolCalls(delta1) + let blocks = parser.getContentBlocks() + expect(blocks).toHaveLength(0) // Not complete yet + + parser.processNativeToolCalls(delta2) + blocks = parser.getContentBlocks() + expect(blocks).toHaveLength(0) // Still not complete + + parser.processNativeToolCalls(delta3) + blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(1) + const toolUse = blocks[0] as ToolUse + expect(toolUse.name).toBe("use_mcp_tool") + expect(toolUse.params.server_name).toBe("weather") + expect(toolUse.params.tool_name).toBe("get_forecast") + // Verify arguments contains the toolInputProps as a JSON string + const parsedArgs = JSON.parse(toolUse.params.arguments!) + expect(parsedArgs.city).toBe("San Francisco") + }) + + it("should preserve existing server_name and tool_name in params if present", () => { + const toolCalls = [ + { + id: "call_preserve", + type: "function" as const, + function: { + name: "use_mcp_tool___server1___tool1", + arguments: JSON.stringify({ + server_name: "custom_server", + tool_name: "custom_tool", + toolInputProps: { + data: "test", + }, + }), + }, + }, + ] + + parser.processNativeToolCalls(toolCalls) + const blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(1) + const toolUse = blocks[0] as ToolUse + expect(toolUse.name).toBe("use_mcp_tool") + // Should preserve the params from arguments, not override with extracted values + expect(toolUse.params.server_name).toBe("custom_server") + expect(toolUse.params.tool_name).toBe("custom_tool") + // Verify arguments contains the toolInputProps as a JSON string + const parsedArgs = JSON.parse(toolUse.params.arguments!) + expect(parsedArgs.data).toBe("test") + }) + }) + + describe("standard tool names", () => { + it("should handle standard tool names without modification", () => { + const toolCalls = [ + { + id: "call_standard", + type: "function" as const, + function: { + name: "read_file", + arguments: JSON.stringify({ + path: "src/file.ts", + }), + }, + }, + ] + + parser.processNativeToolCalls(toolCalls) + const blocks = parser.getContentBlocks() + + expect(blocks).toHaveLength(1) + const toolUse = blocks[0] as ToolUse + expect(toolUse.name).toBe("read_file") + expect(toolUse.params.path).toBe("src/file.ts") + }) + }) + }) +}) diff --git a/src/core/assistant-message/kilocode/native-tool-call.ts b/src/core/assistant-message/kilocode/native-tool-call.ts index 7fc844ebb9..55c426d36d 100644 --- a/src/core/assistant-message/kilocode/native-tool-call.ts +++ b/src/core/assistant-message/kilocode/native-tool-call.ts @@ -1,3 +1,6 @@ +import { ToolName } from "@roo-code/types" +import { ToolUse } from "../../../shared/tools" + /** * Represents a native tool call from OpenAI-compatible APIs */ @@ -57,3 +60,45 @@ export function parseDoubleEncodedParams(obj: any): any { // Primitive types (number, boolean, etc.) return as-is return obj } + +const NATIVE_MCP_TOOL_PREFIX = "use_mcp_tool___" +const NATIVE_MCP_TOOL_SEPARATOR = "___" + +/** + * Check if a tool name is a dynamic MCP tool (starts with "use_mcp_tool_") + */ +function isDynamicMcpTool(toolName: string): boolean { + return toolName.startsWith(NATIVE_MCP_TOOL_PREFIX) +} + +/** + * Extract server name and tool name from dynamic MCP tool names. + * Format: use_mcp_tool___{serverName}___{toolName} + * Uses triple underscores as separator to allow underscores in tool names. + * Returns null if the format is invalid. + */ +export function extractMcpToolInfo(toolName: string): { serverName: string; toolName: string } | null { + if (!isDynamicMcpTool(toolName)) { + return null + } + + // Remove the prefix + const remainder = toolName.slice(NATIVE_MCP_TOOL_PREFIX.length) + + // Find first triple underscore to split server name and tool name + + const firstSeparatorIndex = remainder.indexOf(NATIVE_MCP_TOOL_SEPARATOR) + + if (firstSeparatorIndex === -1) { + return null // Invalid format + } + + const serverName = remainder.slice(0, firstSeparatorIndex) + const extractedToolName = remainder.slice(firstSeparatorIndex + NATIVE_MCP_TOOL_SEPARATOR.length) + + if (!serverName || !extractedToolName) { + return null // Invalid format + } + + return { serverName, toolName: extractedToolName } +} diff --git a/src/core/prompts/sections/mcp-servers.ts b/src/core/prompts/sections/mcp-servers.ts index fc257b9d52..c524a6bf9f 100644 --- a/src/core/prompts/sections/mcp-servers.ts +++ b/src/core/prompts/sections/mcp-servers.ts @@ -11,6 +11,11 @@ export async function getMcpServersSection( if (!mcpHub) { return "" } + // kilocode_change start + if (toolUseStyle === "json") { + return "" + } + // kilocode_change end const connectedServers = mcpHub.getServers().length > 0 @@ -68,19 +73,14 @@ ${connectedServers}` return baseSection } - let descSection = + return ( baseSection + ` ## Creating an MCP Server -The user may ask you something along the lines of "add a tool" that does some function, in other words to create an MCP server that provides tools and resources that may connect to external APIs for example. If they do, you should obtain detailed instructions on this topic using the fetch_instructions tool, ` - // kilocode_change: toolUseStyle - if (toolUseStyle !== "json") { - descSection += `like this: +The user may ask you something along the lines of "add a tool" that does some function, in other words to create an MCP server that provides tools and resources that may connect to external APIs for example. If they do, you should obtain detailed instructions on this topic using the fetch_instructions tool, like this: create_mcp_server ` - } - - return descSection + ) } diff --git a/src/core/prompts/system.ts b/src/core/prompts/system.ts index b933f0b974..5031e3c14c 100644 --- a/src/core/prompts/system.ts +++ b/src/core/prompts/system.ts @@ -91,7 +91,12 @@ async function generatePrompt( const [modesSection, mcpServersSection] = await Promise.all([ getModesSection(context, toolUseStyle /*kilocode_change*/), shouldIncludeMcp - ? getMcpServersSection(mcpHub, effectiveDiffStrategy, enableMcpServerCreation) + ? getMcpServersSection( + mcpHub, + effectiveDiffStrategy, + enableMcpServerCreation, + toolUseStyle, // kilocode_change + ) : Promise.resolve(""), ]) diff --git a/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts b/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts index c9a20d700f..183f047d8a 100644 --- a/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts +++ b/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts @@ -7,14 +7,54 @@ import { ALWAYS_AVAILABLE_TOOLS, TOOL_GROUPS } from "../../../../shared/tools" import { isFastApplyAvailable } from "../../../tools/editFileTool" import { nativeTools } from "." import { apply_diff_multi_file, apply_diff_single_file } from "./apply_diff" +import pWaitFor from "p-wait-for" +import { McpHub } from "../../../../services/mcp/McpHub" +import { McpServerManager } from "../../../../services/mcp/McpServerManager" +import { getMcpServerTools } from "./mcp_server" +import { ClineProvider } from "../../../webview/ClineProvider" +import { ContextProxy } from "../../../config/ContextProxy" +import * as vscode from "vscode" -export function getAllowedJSONToolsForMode( +export async function getAllowedJSONToolsForMode( mode: Mode, - codeIndexManager?: CodeIndexManager, - clineProviderState?: ClineProviderState, + provider: ClineProvider | undefined, supportsImages?: boolean, -): OpenAI.Chat.ChatCompletionTool[] { - const config = getModeConfig(mode, clineProviderState?.customModes) +): Promise { + const providerState: ClineProviderState | undefined = await provider?.getState() + const config = getModeConfig(mode, providerState?.customModes) + const context = ContextProxy.instance.rawContext + + // Initialize code index managers for all workspace folders. + let codeIndexManager: CodeIndexManager | undefined = undefined + + if (vscode.workspace.workspaceFolders) { + for (const folder of vscode.workspace.workspaceFolders) { + const manager = CodeIndexManager.getInstance(context, folder.uri.fsPath) + if (manager) { + codeIndexManager = manager + } + } + } + + const { mcpEnabled } = providerState ?? {} + let mcpHub: McpHub | undefined + if (mcpEnabled) { + if (!provider) { + throw new Error("Provider reference lost during view transition") + } + + // Wait for MCP hub initialization through McpServerManager + mcpHub = await McpServerManager.getInstance(provider.context, provider) + + if (!mcpHub) { + throw new Error("Failed to get MCP hub from server manager") + } + + // Wait for MCP servers to be connected before generating system prompt + await pWaitFor(() => !mcpHub!.isConnecting, { timeout: 10_000 }).catch(() => { + console.error("MCP servers failed to connect in time") + }) + } const tools = new Set() @@ -28,10 +68,10 @@ export function getAllowedJSONToolsForMode( isToolAllowedForMode( tool as ToolName, mode, - clineProviderState?.customModes ?? [], + providerState?.customModes ?? [], undefined, undefined, - clineProviderState?.experiments ?? {}, + providerState?.experiments ?? {}, ) ) { tools.add(tool) @@ -51,8 +91,8 @@ export function getAllowedJSONToolsForMode( tools.delete("codebase_search") } - if (isFastApplyAvailable(clineProviderState)) { - // When Morph is enabled, disable traditional editing tools + if (isFastApplyAvailable(providerState)) { + // When Fast Apply is enabled, disable traditional editing tools const traditionalEditingTools = ["apply_diff", "write_to_file", "insert_content", "search_and_replace"] traditionalEditingTools.forEach((tool) => tools.delete(tool)) } else { @@ -60,21 +100,21 @@ export function getAllowedJSONToolsForMode( } // Conditionally exclude update_todo_list if disabled in settings - if (clineProviderState?.apiConfiguration?.todoListEnabled === false) { + if (providerState?.apiConfiguration?.todoListEnabled === false) { tools.delete("update_todo_list") } // Conditionally exclude generate_image if experiment is not enabled - if (!clineProviderState?.experiments?.imageGeneration) { + if (!providerState?.experiments?.imageGeneration) { tools.delete("generate_image") } // Conditionally exclude run_slash_command if experiment is not enabled - if (!clineProviderState?.experiments?.runSlashCommand) { + if (!providerState?.experiments?.runSlashCommand) { tools.delete("run_slash_command") } - if (!clineProviderState?.browserToolEnabled || !supportsImages) { + if (!providerState?.browserToolEnabled || !supportsImages) { tools.delete("browser_action") } @@ -84,8 +124,8 @@ export function getAllowedJSONToolsForMode( nativeToolsMap.set(tool.function.name, tool) }) - if (clineProviderState?.apiConfiguration.diffEnabled) { - if (clineProviderState?.experiments.multiFileApplyDiff) { + if (providerState?.apiConfiguration.diffEnabled) { + if (providerState?.experiments.multiFileApplyDiff) { nativeToolsMap.set("apply_diff", apply_diff_multi_file) } else { nativeToolsMap.set("apply_diff", apply_diff_single_file) @@ -101,5 +141,14 @@ export function getAllowedJSONToolsForMode( } }) + // Check if MCP functionality should be included + const hasMcpGroup = config.groups.some((groupEntry) => getGroupName(groupEntry) === "mcp") + if (hasMcpGroup && mcpHub) { + const mcpTools = getMcpServerTools(mcpHub) + if (mcpTools) { + allowedTools.push(...mcpTools) + } + } + return allowedTools } diff --git a/src/core/prompts/tools/native-tools/mcp_server.ts b/src/core/prompts/tools/native-tools/mcp_server.ts new file mode 100644 index 0000000000..db63745f61 --- /dev/null +++ b/src/core/prompts/tools/native-tools/mcp_server.ts @@ -0,0 +1,105 @@ +import type OpenAI from "openai" +import { McpHub } from "../../../../services/mcp/McpHub" + +/** + * Dynamically generates native tool definitions for all enabled tools across connected MCP servers. + * + * @param mcpHub The McpHub instance containing connected servers. + * @returns An array of OpenAI.Chat.ChatCompletionTool definitions. + */ +export function getMcpServerTools(mcpHub?: McpHub): OpenAI.Chat.ChatCompletionTool[] { + if (!mcpHub) { + return [] + } + + const servers = mcpHub.getServers() + const tools: OpenAI.Chat.ChatCompletionTool[] = [] + + for (const server of servers) { + if (!server.tools) { + continue + } + for (const tool of server.tools) { + // Filter tools where tool.enabledForPrompt is not explicitly false + if (tool.enabledForPrompt === false) { + continue + } + + // Ensure parameters is a valid FunctionParameters object, even if inputSchema is undefined + const parameters = { + type: "object", + properties: { + server_name: { + type: "string", + const: server.name, + }, + tool_name: { + type: "string", + const: tool.name, + }, + }, + required: ["server_name", "tool_name", "toolInputProps"], + additionalProperties: false, + } as OpenAI.FunctionParameters + + const originalSchema = tool.inputSchema as Record | undefined + const toolInputPropsRaw = originalSchema?.properties ?? {} + const toolInputRequired = (originalSchema?.required ?? []) as string[] + + // Handle reserved property names like 'type' + const sanitizedToolInputProps: Record = {} + const sanitizedRequired: string[] = [] + + for (const [propName, propValue] of Object.entries(toolInputPropsRaw)) { + // rename 'type' to 'renamed_type' because 'type' is a reserved word in JSON Schema + // for many parsers. + if (propName === "type") { + sanitizedToolInputProps[`renamed_${propName}`] = propValue + // Update required array if 'type' was required + if (toolInputRequired.includes(propName)) { + sanitizedRequired.push(`renamed_${propName}`) + } + } else { + sanitizedToolInputProps[propName] = propValue + if (toolInputRequired.includes(propName)) { + sanitizedRequired.push(propName) + } + } + } + + // Create a proper JSON Schema object for toolInputProps + const toolInputPropsSchema: Record = { + type: "object", + properties: sanitizedToolInputProps, + additionalProperties: false, + } + + // Only add required if there are required fields + if (sanitizedRequired.length > 0) { + toolInputPropsSchema.required = sanitizedRequired + } + + parameters.properties = { + toolInputProps: toolInputPropsSchema, + ...(parameters.properties as Record), //putting this second ensures it overrides anything in the tool def. + } + + //Add the server_name and tool_name properties + + // The description matches what the MCP server provides as guidance. + // Use triple underscores as separator to allow underscores in tool names + const toolDefinition: OpenAI.Chat.ChatCompletionTool = { + type: "function", + function: { + name: `use_mcp_tool___${server.name}___${tool.name}`, + description: tool.description, + parameters: parameters, + }, + } + + tools.push(toolDefinition) + } + } + + return tools +} diff --git a/src/core/task/Task.ts b/src/core/task/Task.ts index b9ad830d8b..f3db720f1b 100644 --- a/src/core/task/Task.ts +++ b/src/core/task/Task.ts @@ -2935,16 +2935,11 @@ export class Task extends EventEmitter implements TaskLike { if (getActiveToolUseStyle(apiConfiguration) === "json" && mode) { try { const provider = this.providerRef.deref() - const providerState = await provider?.getState() - - const allowedTools = getAllowedJSONToolsForMode( + metadata.allowedTools = await getAllowedJSONToolsForMode( mode, - undefined, // codeIndexManager is private, not accessible here - providerState, + provider, this.api?.getModel()?.info?.supportsImages, ) - - metadata.allowedTools = allowedTools } catch (error) { console.error("[Task] Error getting allowed tools for mode:", error) // Continue without allowedTools - will fall back to default behavior diff --git a/src/core/tools/useMcpToolTool.ts b/src/core/tools/useMcpToolTool.ts index 9a4a56edd8..464033133d 100644 --- a/src/core/tools/useMcpToolTool.ts +++ b/src/core/tools/useMcpToolTool.ts @@ -83,6 +83,30 @@ async function validateParams( } } +/** + * Reverses property renaming applied in schema generation. + * Properties named `renamed_*` are converted back to their original names. + */ +function reversePropertyRenaming(args: Record | undefined): Record | undefined { + if (!args) { + return args + } + + const reversed: Record = {} + + for (const [key, value] of Object.entries(args)) { + if (key.startsWith("renamed_")) { + // Extract original property name (e.g., "renamed_type" -> "type") + const originalKey = key.substring("renamed_".length) + reversed[originalKey] = value + } else { + reversed[key] = value + } + } + + return reversed +} + async function validateToolExists( cline: Task, serverName: string, @@ -239,7 +263,10 @@ async function executeToolAndProcessResult( toolName, }) - const toolResult = await cline.providerRef.deref()?.getMcpHub()?.callTool(serverName, toolName, parsedArguments) + // Reverse any property renaming before calling the tool + const actualArguments = reversePropertyRenaming(parsedArguments) + + const toolResult = await cline.providerRef.deref()?.getMcpHub()?.callTool(serverName, toolName, actualArguments) let toolResultPretty = "(No response)" diff --git a/webview-ui/src/components/chat/McpExecution.tsx b/webview-ui/src/components/chat/McpExecution.tsx index 90be64913f..478044316d 100644 --- a/webview-ui/src/components/chat/McpExecution.tsx +++ b/webview-ui/src/components/chat/McpExecution.tsx @@ -54,6 +54,12 @@ export const McpExecution = ({ // kilocode_change: Main collapse state for the entire MCP execution content const [isResponseExpanded, setIsResponseExpanded] = useState(initiallyExpanded) + // Remove "renamed_" prefix from property names in JSON + const removeRenamedPrefix = useCallback((text: string): string => { + if (!text) return text + return text.replace(/"renamed_([^"]+)":/g, '"$1":') + }, []) + // Try to parse JSON and return both the result and formatted text const tryParseJson = useCallback((text: string): { isJson: boolean; formatted: string } => { if (!text) return { isJson: false, formatted: "" } @@ -275,7 +281,7 @@ export const McpExecution = ({ "mt-1 pt-1": !isArguments && (useMcpServer?.type === "use_mcp_tool" || (toolName && serverName)), })}> - + )} From 312a86354dce2b178053febba83ce4763bbe35b4 Mon Sep 17 00:00:00 2001 From: Matt Cowger Date: Fri, 24 Oct 2025 12:01:00 -0700 Subject: [PATCH 2/2] Fix duplicate tools and add tests. --- .../getAllowedJSONToolsForMode.spec.ts | 332 +++++++++++++++--- .../getAllowedJSONToolsForMode.ts | 19 +- 2 files changed, 283 insertions(+), 68 deletions(-) diff --git a/src/core/prompts/tools/native-tools/__tests__/getAllowedJSONToolsForMode.spec.ts b/src/core/prompts/tools/native-tools/__tests__/getAllowedJSONToolsForMode.spec.ts index 1fbee21190..73d655b4ce 100644 --- a/src/core/prompts/tools/native-tools/__tests__/getAllowedJSONToolsForMode.spec.ts +++ b/src/core/prompts/tools/native-tools/__tests__/getAllowedJSONToolsForMode.spec.ts @@ -1,70 +1,288 @@ -import { describe, it, expect } from "vitest" +import { beforeEach, describe, expect, it, vi } from "vitest" import { getAllowedJSONToolsForMode } from "../getAllowedJSONToolsForMode" import { Mode } from "../../../../../shared/modes" -import { ClineProviderState } from "../../../../webview/ClineProvider" +import { ClineProvider, ClineProviderState } from "../../../../webview/ClineProvider" import { apply_diff_multi_file, apply_diff_single_file } from "../apply_diff" +import { CodeIndexManager } from "../../../../../services/code-index/manager" +import { McpServerManager } from "../../../../../services/mcp/McpServerManager" +import { ContextProxy } from "../../../../config/ContextProxy" +import * as vscode from "vscode" + +vi.mock("vscode") +vi.mock("../../../../../services/code-index/manager") +vi.mock("../../../../../services/mcp/McpServerManager") +vi.mock("../../../../config/ContextProxy") describe("getAllowedJSONToolsForMode", () => { - const mockCodeIndexManager = { - isFeatureEnabled: true, - isFeatureConfigured: true, - isInitialized: true, - } as any - - const baseProviderState: Partial = { - apiConfiguration: { - diffEnabled: true, - }, - experiments: {}, - } - - it("should use single file diff when multiFileApplyDiff experiment is disabled", () => { - const providerState: Partial = { - ...baseProviderState, - apiConfiguration: { - diffEnabled: true, - }, - experiments: { - multiFileApplyDiff: false, + let mockProvider: Partial + let mockContext: any + + beforeEach(() => { + vi.clearAllMocks() + + mockContext = { + globalState: { + get: vi.fn(), + update: vi.fn(), }, + subscriptions: [], + } + + mockProvider = { + context: mockContext, + getState: vi.fn(), } - const tools = getAllowedJSONToolsForMode( - "code" as Mode, - mockCodeIndexManager, - providerState as ClineProviderState, - true, - false, - ) - - const applyDiffTool = tools.find((tool) => "function" in tool && tool.function.name === "apply_diff") - expect(applyDiffTool).toBeDefined() - expect(applyDiffTool).toEqual(apply_diff_single_file) - expect(applyDiffTool).not.toEqual(apply_diff_multi_file) + // Mock ContextProxy static getter + vi.spyOn(ContextProxy, "instance", "get").mockReturnValue({ + rawContext: mockContext, + } as any) + + // Mock vscode workspace + vi.mocked(vscode.workspace).workspaceFolders = undefined + + // Mock CodeIndexManager + vi.mocked(CodeIndexManager.getInstance).mockReturnValue(undefined) + + // Mock McpServerManager + vi.mocked(McpServerManager.getInstance).mockResolvedValue(undefined as any) }) - it("should use multi file diff when multiFileApplyDiff experiment is enabled", () => { - const providerState: Partial = { - ...baseProviderState, - apiConfiguration: { - diffEnabled: true, - }, - experiments: { - multiFileApplyDiff: true, - }, - } + describe("apply_diff tool selection", () => { + it("should use single file diff when multiFileApplyDiff experiment is disabled", async () => { + const providerState: Partial = { + experiments: { + multiFileApplyDiff: false, + }, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, false) + + const applyDiffTool = tools.find((tool) => "function" in tool && tool.function.name === "apply_diff") + expect(applyDiffTool).toBeDefined() + expect(applyDiffTool).toEqual(apply_diff_single_file) + expect(applyDiffTool).not.toEqual(apply_diff_multi_file) + }) + + it("should use multi file diff when multiFileApplyDiff experiment is enabled", async () => { + const providerState: Partial = { + experiments: { + multiFileApplyDiff: true, + }, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, false) + + const applyDiffTool = tools.find((tool) => "function" in tool && tool.function.name === "apply_diff") + expect(applyDiffTool).toBeDefined() + expect(applyDiffTool).toEqual(apply_diff_multi_file) + expect(applyDiffTool).not.toEqual(apply_diff_single_file) + }) + + it("should not include apply_diff when diffEnabled is false", async () => { + const providerState: Partial = { + experiments: { + multiFileApplyDiff: true, + }, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, false, false) + + const applyDiffTool = tools.find((tool) => "function" in tool && tool.function.name === "apply_diff") + expect(applyDiffTool).toBeUndefined() + }) + }) + + describe("no duplicate tools", () => { + it("should not return duplicate tools", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + // Check for duplicate tool names + const toolNames = tools.map((tool) => ("function" in tool ? tool.function.name : "")) + const uniqueToolNames = new Set(toolNames) + + expect(toolNames.length).toBe(uniqueToolNames.size) + }) + + it("should return consistent tool count across multiple calls", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools1 = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + const tools2 = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + expect(tools1.length).toBe(tools2.length) + }) + }) + + describe("tool filtering", () => { + it("should exclude codebase_search when CodeIndexManager is not available", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + vi.mocked(CodeIndexManager.getInstance).mockReturnValue(undefined) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const codebaseSearchTool = tools.find( + (tool) => "function" in tool && tool.function.name === "codebase_search", + ) + expect(codebaseSearchTool).toBeUndefined() + }) + + it("should exclude browser_action when browserToolEnabled is false", async () => { + const providerState: Partial = { + browserToolEnabled: false, + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const browserActionTool = tools.find( + (tool) => "function" in tool && tool.function.name === "browser_action", + ) + expect(browserActionTool).toBeUndefined() + }) + + it("should exclude browser_action when supportsImages is false", async () => { + const providerState: Partial = { + browserToolEnabled: true, + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, false) + + const browserActionTool = tools.find( + (tool) => "function" in tool && tool.function.name === "browser_action", + ) + expect(browserActionTool).toBeUndefined() + }) + + it("should exclude update_todo_list when todoListEnabled is false", async () => { + const providerState: Partial = { + apiConfiguration: { + todoListEnabled: false, + }, + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const todoListTool = tools.find((tool) => "function" in tool && tool.function.name === "update_todo_list") + expect(todoListTool).toBeUndefined() + }) + + it("should exclude generate_image when imageGeneration experiment is disabled", async () => { + const providerState: Partial = { + experiments: { + imageGeneration: false, + }, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const generateImageTool = tools.find( + (tool) => "function" in tool && tool.function.name === "generate_image", + ) + expect(generateImageTool).toBeUndefined() + }) + + it("should exclude run_slash_command when runSlashCommand experiment is disabled", async () => { + const providerState: Partial = { + experiments: { + runSlashCommand: false, + }, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const runSlashCommandTool = tools.find( + (tool) => "function" in tool && tool.function.name === "run_slash_command", + ) + expect(runSlashCommandTool).toBeUndefined() + }) + }) + + describe("always available tools", () => { + it("should always include ask_followup_question", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const askTool = tools.find((tool) => "function" in tool && tool.function.name === "ask_followup_question") + expect(askTool).toBeDefined() + }) + + it("should always include attempt_completion", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const completionTool = tools.find( + (tool) => "function" in tool && tool.function.name === "attempt_completion", + ) + expect(completionTool).toBeDefined() + }) + + it("should always include switch_mode", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) + + const switchModeTool = tools.find((tool) => "function" in tool && tool.function.name === "switch_mode") + expect(switchModeTool).toBeDefined() + }) + + it("should always include new_task", async () => { + const providerState: Partial = { + experiments: {}, + } + + vi.mocked(mockProvider.getState!).mockResolvedValue(providerState as ClineProviderState) + + const tools = await getAllowedJSONToolsForMode("code" as Mode, mockProvider as ClineProvider, true, true) - const tools = getAllowedJSONToolsForMode( - "code" as Mode, - mockCodeIndexManager, - providerState as ClineProviderState, - true, - false, - ) - - const applyDiffTool = tools.find((tool) => "function" in tool && tool.function.name === "apply_diff") - expect(applyDiffTool).toBeDefined() - expect(applyDiffTool).toEqual(apply_diff_multi_file) - expect(applyDiffTool).not.toEqual(apply_diff_single_file) + const newTaskTool = tools.find((tool) => "function" in tool && tool.function.name === "new_task") + expect(newTaskTool).toBeDefined() + }) }) }) diff --git a/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts b/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts index a24ced32b4..8e294a4cbc 100644 --- a/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts +++ b/src/core/prompts/tools/native-tools/getAllowedJSONToolsForMode.ts @@ -18,6 +18,7 @@ import * as vscode from "vscode" export async function getAllowedJSONToolsForMode( mode: Mode, provider: ClineProvider | undefined, + diffEnabled: boolean = false, supportsImages?: boolean, ): Promise { const providerState: ClineProviderState | undefined = await provider?.getState() @@ -120,7 +121,12 @@ export async function getAllowedJSONToolsForMode( // Create a map of tool names to native tool definitions for quick lookup // Exclude apply_diff tools as they are handled specially below - const allowedTools: OpenAI.Chat.ChatCompletionTool[] = [] + // Create a map of tool names to native tool definitions for quick lookup + const nativeToolsMap = new Map() + nativeTools.forEach((tool) => { + nativeToolsMap.set(tool.function.name, tool) + }) + let allowedTools: OpenAI.Chat.ChatCompletionTool[] = [] let isApplyDiffToolAllowedForMode = false for (const nativeTool of nativeTools) { @@ -139,22 +145,13 @@ export async function getAllowedJSONToolsForMode( // Handle the "apply_diff" logic separately because the same tool has different // implementations depending on whether multi-file diffs are enabled, but the same name is used. if (isApplyDiffToolAllowedForMode && diffEnabled) { - if (clineProviderState?.experiments.multiFileApplyDiff) { + if (providerState?.experiments.multiFileApplyDiff) { allowedTools.push(apply_diff_multi_file) } else { allowedTools.push(apply_diff_single_file) } } - // Map allowed tools to their native definitions - const allowedTools: OpenAI.Chat.ChatCompletionTool[] = [] - tools.forEach((toolName) => { - const nativeTool = nativeToolsMap.get(toolName) - if (nativeTool) { - allowedTools.push(nativeTool) - } - }) - // Check if MCP functionality should be included const hasMcpGroup = config.groups.some((groupEntry) => getGroupName(groupEntry) === "mcp") if (hasMcpGroup && mcpHub) {