mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-20 15:33:54 +00:00
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__<server>__read_resource (the actual registered tool name via mcp_prefixed_tool_name) instead of a nonexistent <server>_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.
This commit is contained in:
parent
1d98f8dd95
commit
284a3cd47e
2 changed files with 85 additions and 3 deletions
|
|
@ -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__<server>__read_resource), not a
|
||||
# made-up "<server>_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"
|
||||
|
|
|
|||
|
|
@ -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"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue