From 284a3cd47e8b7645b92eedfe63e714940d52c924 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Tue, 14 Jul 2026 11:58:43 -0700 Subject: [PATCH] fix(mcp): use real wire name in ResourceLink marker + surface resource text in isError path Follow-ups on top of #64061's salvage: - ResourceLink markers now point at mcp____read_resource (the actual registered tool name via mcp_prefixed_tool_name) instead of a nonexistent _read_resource the agent could hallucinate-call. - The isError path now surfaces EmbeddedResource .resource.text blocks instead of dropping them, so error payloads carried in resources no longer collapse to a bare 'MCP tool returned an error'. (Same-class fix flagged in #64061 and independently addressed in #63576 by @alauer.) - 3 new error-path tests + updated ResourceLink wire-name assertion. --- tests/tools/test_mcp_resource_content.py | 73 +++++++++++++++++++++++- tools/mcp_tool.py | 15 ++++- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/tests/tools/test_mcp_resource_content.py b/tests/tools/test_mcp_resource_content.py index eeb15ce99d35..3111c2933020 100644 --- a/tests/tools/test_mcp_resource_content.py +++ b/tests/tools/test_mcp_resource_content.py @@ -70,7 +70,9 @@ class TestRenderResourceBlock: ) out = _render_mcp_resource_block(link, "slack") assert "slack://files/F123" in out - assert "slack_read_resource" in out + # Must be the real wire name (mcp____read_resource), not a + # made-up "_read_resource" the agent can't actually call. + assert "mcp__slack__read_resource" in out assert "report.pdf" in out def test_oversized_blob_fails_explicitly_without_writing(self, doc_cache, monkeypatch): @@ -208,3 +210,72 @@ class TestToolResultLoopOrdering: mimeType="application/pdf", ) assert _cache_mcp_image_block(block) == "" + + +class TestErrorPathResourceText: + """isError payloads must surface EmbeddedResource text, not drop it.""" + + @pytest.fixture() + def _handler(self, monkeypatch): + import asyncio + from unittest.mock import AsyncMock, MagicMock, patch as mock_patch + + from tools import mcp_tool + + fake_session = MagicMock() + fake_server = SimpleNamespace(session=fake_session, _rpc_lock=None) + + def _fake_run_on_mcp_loop(coro_or_factory, timeout=30): + coro = coro_or_factory() if callable(coro_or_factory) else coro_or_factory + loop = asyncio.new_event_loop() + try: + async def _install_lock_and_run(): + for srv in list(mcp_tool._servers.values()): + if getattr(srv, "_rpc_lock", None) is None: + srv._rpc_lock = asyncio.Lock() + return await coro + + return loop.run_until_complete(_install_lock_and_run()) + finally: + loop.close() + + with mock_patch.dict(mcp_tool._servers, {"test-server": fake_server}), \ + mock_patch("tools.mcp_tool._run_on_mcp_loop", side_effect=_fake_run_on_mcp_loop): + fake_session.call_tool = AsyncMock() + yield fake_session, mcp_tool._make_tool_handler("test-server", "my-tool", 30.0) + + def test_error_embedded_resource_text_surfaced(self, _handler): + from unittest.mock import AsyncMock + + session, handler = _handler + res = SimpleNamespace(uri="mem://err", mimeType="text/plain", + text="quota exceeded for workspace W1", blob=None) + session.call_tool = AsyncMock(return_value=SimpleNamespace( + content=[_embedded(res)], isError=True, structuredContent=None, + )) + data = json.loads(handler({})) + assert "quota exceeded for workspace W1" in data["error"] + + def test_error_mixed_text_and_resource(self, _handler): + from unittest.mock import AsyncMock + + session, handler = _handler + res = SimpleNamespace(uri="mem://err", mimeType="text/plain", + text=" — details in resource", blob=None) + session.call_tool = AsyncMock(return_value=SimpleNamespace( + content=[SimpleNamespace(type="text", text="tool failed"), _embedded(res)], + isError=True, structuredContent=None, + )) + data = json.loads(handler({})) + assert "tool failed" in data["error"] + assert "details in resource" in data["error"] + + def test_error_with_no_text_blocks_falls_back(self, _handler): + from unittest.mock import AsyncMock + + session, handler = _handler + session.call_tool = AsyncMock(return_value=SimpleNamespace( + content=[], isError=True, structuredContent=None, + )) + data = json.loads(handler({})) + assert data["error"] == "MCP tool returned an error" diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 942884dc5e44..4438a715718a 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -832,7 +832,11 @@ def _render_mcp_resource_block(block, server_name: str = "") -> str: details += f", name={name}" if mime: details += f", mimeType={mime}" - reader = f"{server_name}_read_resource" if server_name else "the MCP server's read_resource tool" + reader = ( + mcp_prefixed_tool_name(server_name, "read_resource") + if server_name + else "the MCP server's read_resource tool" + ) return f"[MCP resource link: {details} — fetch it with {reader}]" resource = getattr(block, "resource", None) @@ -4107,8 +4111,15 @@ def _make_tool_handler(server_name: str, tool_name: str, tool_timeout: float): if result.isError: error_text = "" for block in (result.content or []): - if hasattr(block, "text"): + if getattr(block, "text", None): error_text += block.text + continue + # EmbeddedResource blocks inside error payloads carry + # their text under .resource.text — previously dropped, + # leaving a bare "MCP tool returned an error". + res_text = getattr(getattr(block, "resource", None), "text", None) + if res_text: + error_text += str(res_text) return json.dumps({ "error": _sanitize_error( error_text or "MCP tool returned an error"