From 7af029013607a0e7047087e039fef64c7552e984 Mon Sep 17 00:00:00 2001 From: Shreyansh Jain Date: Tue, 3 Nov 2026 17:47:07 +0530 Subject: [PATCH] fix: keep tool input from overriding the MCP handler's tool --- s14_mcp_plugin/README.ja.md | 7 ++----- s14_mcp_plugin/README.md | 7 ++----- s14_mcp_plugin/README.zh.md | 7 ++----- s14_mcp_plugin/code.py | 10 ++++++---- s15_integrated_harness/code.py | 10 ++++++---- tests/test_agent_teams_runtime.py | 15 +++++++++++++++ 6 files changed, 33 insertions(+), 23 deletions(-) diff --git a/s14_mcp_plugin/README.ja.md b/s14_mcp_plugin/README.ja.md index 109a351ec5..e7f1be1dde 100644 --- a/s14_mcp_plugin/README.ja.md +++ b/s14_mcp_plugin/README.ja.md @@ -119,13 +119,10 @@ tools.append({ "description": tool_def.get("description", ""), "input_schema": schema, }) -handlers[prefixed] = ( - lambda *, client=server, tool=raw_name, **kwargs: - client.call_tool(tool, kwargs) -) +handlers[prefixed] = make_mcp_handler(server, raw_name) ``` -model は prefix 付きの名前を見る。handler は server の元の tool name で `MCPClient` を呼ぶ。default argument が現在の client と tool を保持するため、loop 内の lambda がすべて最後の tool を参照することはない。 +model は prefix 付きの名前を見る。handler は server の元の tool name で `MCPClient` を呼ぶ。`make_mcp_handler()` が現在の client と tool を closure に保持するため、loop 内の lambda がすべて最後の tool を参照することはない。default argument でも同じことはできるが、それは通常の keyword parameter のままであり、tool input の `tool` key が default を上書きし、許可済みの呼び出しを別の server tool に差し替えてしまう。 ### 6. permission は host が決める diff --git a/s14_mcp_plugin/README.md b/s14_mcp_plugin/README.md index d2e87a0881..b79cb9fc9c 100644 --- a/s14_mcp_plugin/README.md +++ b/s14_mcp_plugin/README.md @@ -119,13 +119,10 @@ tools.append({ "description": tool_def.get("description", ""), "input_schema": schema, }) -handlers[prefixed] = ( - lambda *, client=server, tool=raw_name, **kwargs: - client.call_tool(tool, kwargs) -) +handlers[prefixed] = make_mcp_handler(server, raw_name) ``` -The model sees the prefixed name. The handler calls `MCPClient` with the server's original tool name. Default arguments capture the current client and tool so every lambda does not point to the last item in the loop. +The model sees the prefixed name. The handler calls `MCPClient` with the server's original tool name. `make_mcp_handler()` captures the current client and tool in a closure so every lambda does not point to the last item in the loop. Default arguments fix that too, but they stay ordinary keyword parameters: a `tool` key in the tool input would replace them and send an approved call to a different server tool. ### 6. The host decides permissions diff --git a/s14_mcp_plugin/README.zh.md b/s14_mcp_plugin/README.zh.md index e12edaa510..99e2b8527f 100644 --- a/s14_mcp_plugin/README.zh.md +++ b/s14_mcp_plugin/README.zh.md @@ -119,13 +119,10 @@ tools.append({ "description": tool_def.get("description", ""), "input_schema": schema, }) -handlers[prefixed] = ( - lambda *, client=server, tool=raw_name, **kwargs: - client.call_tool(tool, kwargs) -) +handlers[prefixed] = make_mcp_handler(server, raw_name) ``` -模型看到带前缀的名字；handler 仍使用 server 原始工具名调用 `MCPClient`。默认参数保存当前 client 和 tool，避免循环里的 lambda 全部指向最后一个工具。 +模型看到带前缀的名字；handler 仍使用 server 原始工具名调用 `MCPClient`。`make_mcp_handler()` 用闭包保存当前 client 和 tool，避免循环里的 lambda 全部指向最后一个工具。默认参数也能做到这一点，但它们仍是普通关键字参数：工具输入里的 `tool` 键会覆盖默认值，把已放行的调用改派到另一个 server 工具。 ### 6. 权限由宿主配置决定 diff --git a/s14_mcp_plugin/code.py b/s14_mcp_plugin/code.py index dee196cbe9..e97ca996b1 100644 --- a/s14_mcp_plugin/code.py +++ b/s14_mcp_plugin/code.py @@ -320,6 +320,11 @@ def run_connect_mcp(name: str) -> str: BUILTIN_HANDLERS = {**BASE_HANDLERS, "connect_mcp": run_connect_mcp} +def make_mcp_handler(client: MCPClient, tool: str): + """Bind client and tool in a closure so tool input cannot replace them.""" + return lambda **kwargs: client.call_tool(tool, kwargs) + + def assemble_tool_pool() -> tuple[list[dict], dict[str, callable]]: """Combine built-in tools with every connected server tool.""" global mcp_tool_policies @@ -354,10 +359,7 @@ def assemble_tool_pool() -> tuple[list[dict], dict[str, callable]]: "description": tool_def.get("description", ""), "input_schema": schema, }) - handlers[prefixed] = ( - lambda *, client=server, tool=raw_name, **kwargs: - client.call_tool(tool, kwargs) - ) + handlers[prefixed] = make_mcp_handler(server, raw_name) policies[prefixed] = MCP_HOST_POLICY.get( (server_name, raw_name), "confirm" ) diff --git a/s15_integrated_harness/code.py b/s15_integrated_harness/code.py index d59d0e4216..3c1cebbe59 100644 --- a/s15_integrated_harness/code.py +++ b/s15_integrated_harness/code.py @@ -2751,6 +2751,11 @@ def connect_mcp(name: str) -> str: f"Discovered {len(mcp_client.tools)} tools: {', '.join(tool_names)}") +def make_mcp_handler(client: MCPClient, tool: str): + """Bind client and tool in a closure so tool input cannot replace them.""" + return lambda **kwargs: client.call_tool(tool, kwargs) + + def assemble_tool_pool() -> tuple[list[dict], dict]: """Merge builtin tools + all MCP tools into one pool.""" global mcp_tool_policies @@ -2784,10 +2789,7 @@ def assemble_tool_pool() -> tuple[list[dict], dict]: "description": tool_def.get("description", ""), "input_schema": schema, }) - handlers[prefixed] = ( - lambda *, client=mcp_client, tool=raw_name, **kwargs: - client.call_tool(tool, kwargs) - ) + handlers[prefixed] = make_mcp_handler(mcp_client, raw_name) policies[prefixed] = MCP_HOST_POLICY.get( (server_name, raw_name), "confirm" ) diff --git a/tests/test_agent_teams_runtime.py b/tests/test_agent_teams_runtime.py index 0fe5060bd2..47df2e80ad 100644 --- a/tests/test_agent_teams_runtime.py +++ b/tests/test_agent_teams_runtime.py @@ -648,6 +648,21 @@ def test_mcp_permission_uses_host_policy(self): "Permission denied by user", ) + def test_mcp_handlers_ignore_a_tool_key_in_the_tool_input(self): + for lesson_path in MCP_LESSONS: + with self.subTest(lesson=lesson_path.parent.name): + with tempfile.TemporaryDirectory() as tmp: + lesson = load_lesson(Path(tmp), lesson_path) + lesson.connect_mcp("deploy") + _tools, handlers = lesson.assemble_tool_pool() + + output = handlers["mcp__deploy__status"]( + service="web", tool="trigger" + ) + + self.assertNotIn("Triggered", output) + self.assertIn("unexpected keyword argument 'tool'", output) + def test_integrated_permission_requires_approval_for_every_shell_command(self): with tempfile.TemporaryDirectory() as tmp: root = Path(tmp)