Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for multipart tools within the Model Context Protocol (MCP) plugin, enabling tools to natively return rich media such as images. Key changes include updating the MCP SDK to version 1.29.0, refactoring tool creation logic to support both standard and multipart responses, and adding comprehensive tests and a dedicated test application for these new capabilities. Additionally, the PR includes bug fixes for Gemini tool response conversion and error regex matching. Review feedback identifies a bug in the client.callTool invocation where the schema is incorrectly passed as the second argument and suggests enhancing error handling by pushing messages to the history queue to allow the LLM to attempt corrections.
| ): ToolAction | MultipartToolAction { | ||
| const config = { | ||
| name: `${params.serverName}/${toolDef.name}`, | ||
| description: toolDef.description || '', | ||
| inputJsonSchema: toolDef.inputSchema as JSONSchema7, | ||
| outputSchema: z.any(), | ||
| metadata: { mcp: { _meta: toolDef._meta || {} } }, | ||
| ...(params.multipart ? { multipart: true } : {}), | ||
| }; | ||
|
|
||
| if (params.multipart) { | ||
| const t = tool(config, async (args, { context }) => { | ||
| logger.debug( | ||
| `[MCP] calling tool '${params.serverName}/${tool.name}' in host '${params.name}'` | ||
| `[MCP] calling tool '${params.serverName}/${toolDef.name}' in host '${params.name}'` | ||
| ); | ||
| const result = await client.callTool({ | ||
| name: tool.name, | ||
| const rawResult = await client.callTool( | ||
| { | ||
| name: toolDef.name, | ||
| arguments: args, | ||
| _meta: context?.mcp?._meta, | ||
| }, | ||
| CallToolResultSchema | ||
| ); | ||
| const result: unknown = rawResult; | ||
|
|
||
| if (isCompatibilityCallToolResult(result)) { | ||
| if (params.rawToolResponses) return { output: result }; | ||
| return { output: result.toolResult }; | ||
| } | ||
|
|
||
| if (isCallToolResult(result)) { | ||
| if (params.rawToolResponses) return { output: result }; | ||
| if (result.isError) { | ||
| return { output: { error: toText(result.content) } }; | ||
| } | ||
| return { | ||
| content: result.content.map((p) => fromMcpPart(p)), | ||
| output: processResult(result), | ||
| }; | ||
| } | ||
|
|
||
| return { output: rawResult }; | ||
| }); | ||
| (t as any).attach = (_: any) => t; // Backwards compatibility | ||
| return t; | ||
| } | ||
|
|
||
| const t = tool(config, async (args, { context }) => { | ||
| logger.debug( | ||
| `[MCP] calling tool '${params.serverName}/${toolDef.name}' in host '${params.name}'` | ||
| ); | ||
| const rawResult = await client.callTool( | ||
| { | ||
| name: toolDef.name, | ||
| arguments: args, | ||
| _meta: context?.mcp?._meta, | ||
| }); | ||
| }, | ||
| CallToolResultSchema | ||
| ); | ||
| const result: unknown = rawResult; | ||
|
|
||
| if (isCompatibilityCallToolResult(result)) { | ||
| if (params.rawToolResponses) return result; | ||
| return processResult(result as CallToolResult); | ||
| return result.toolResult; | ||
| } | ||
| ); | ||
|
|
||
| if (isCallToolResult(result)) { | ||
| if (params.rawToolResponses) return result; | ||
| return processResult(result); | ||
| } | ||
|
|
||
| return rawResult; | ||
| }); | ||
| (t as any).attach = (_: any) => t; // Backwards compatibility | ||
| return t; | ||
| } |
There was a problem hiding this comment.
The implementation of createDynamicTool contains a bug in the client.callTool invocation and violates repository rules regarding error handling and code structure.
- Bug: client.callTool is being passed CallToolResultSchema as its second argument. In the MCP SDK, the second argument is for RequestOptions. The parameters should be part of the first argument object.
- Error Handling: Per repository rules, when a tool fails (result.isError), instead of returning an error object, a message should be added to the history queue to inform the LLM of the error and return control, allowing the LLM to attempt a correction.
- Code Structure: While refactoring duplication is good, ensure that merging the logic for ToolAction and MultipartToolAction does not violate the rule against merging functions that handle different types expected to evolve separately.
The suggested code fixes the SDK call and implements the required error handling flow.
): ToolAction | MultipartToolAction {
const config = {
name: params.serverName + "/" + toolDef.name,
description: toolDef.description || "",
inputJsonSchema: toolDef.inputSchema as JSONSchema7,
outputSchema: z.any(),
metadata: { mcp: { _meta: toolDef._meta || {} } },
...(params.multipart ? { multipart: true } : {}),
};
const t = tool(config, async (args, { context }) => {
logger.debug(
"[MCP] calling tool " + params.serverName + "/" + toolDef.name
);
const result: unknown = await client.callTool({
name: toolDef.name,
arguments: args,
_meta: context?.mcp?._meta,
});
if (isCompatibilityCallToolResult(result)) {
const output = params.rawToolResponses ? result : result.toolResult;
return params.multipart ? { output } : output;
}
if (isCallToolResult(result)) {
if (params.rawToolResponses) {
return params.multipart ? { output: result } : result;
}
if (result.isError) {
context.history.push({
role: "user",
content: [{ type: "text", text: "Tool error: " + toText(result.content) }]
});
return;
}
const output = processResult(result);
if (params.multipart) {
return {
content: result.content.map((p) => fromMcpPart(p)),
output,
};
}
return output;
}
return params.multipart ? { output: result } : result;
});
(t as any).attach = (_: any) => t;
return t;
}References
- When a tool fails, instead of throwing an error or returning a ToolResponse, add a message to the history queue to inform the LLM of the error and return control, allowing the LLM to attempt a correction.
- Do not merge functions with identical implementations if they handle different types that are expected to evolve separately in the future.
|
decided to do it a different way. |
Checklist (if applicable):