diff --git a/src/scribe/mcp/server.py b/src/scribe/mcp/server.py index 79ef5416..dc1ce5ca 100644 --- a/src/scribe/mcp/server.py +++ b/src/scribe/mcp/server.py @@ -4,7 +4,7 @@ from __future__ import annotations import difflib from mcp.server.mcpserver import MCPServer -from mcp.server.mcpserver.exceptions import ToolError +from mcp.server.mcpserver.exceptions import ToolError, UnexpectedToolError from mcp.server.transport_security import TransportSecuritySettings from quart import Quart @@ -282,9 +282,10 @@ class StrictArgsMCPServer(MCPServer): declared cannot have been meant to be dropped. """ - # ToolError, not ValueError: the SDK returns either one's text to the - # caller as an error result, but logs anything that is not a ToolError as - # an unexpected crash. A misnamed argument is the caller's mistake. + # ToolError, not ValueError: since SDK 2.x only a ToolError's text reaches + # the caller. Anything else is wrapped as UnexpectedToolError("Error + # executing tool X") and its message stays on the server. A misnamed + # argument is the caller's mistake, and the caller needs to read why. async def call_tool(self, name, arguments, context=None): try: tool = self._tool_manager.get_tool(name) @@ -304,7 +305,17 @@ class StrictArgsMCPServer(MCPServer): f"It accepts: {', '.join(sorted(declared))}. Nothing was " "created or changed — retry with the declared names." ) - return await super().call_tool(name, arguments, context) + try: + return await super().call_tool(name, arguments, context) + except UnexpectedToolError as exc: + # ValueError is how every Scribe tool refuses (#4794): what was + # refused, why, and what to do instead, written for the agent. + # SDK 2.x masks it like a crash, so the agent got a failure with + # no reason and could only retry blind. Other exceptions ARE + # crashes; their text can carry internals and stays masked. + if isinstance(exc.__cause__, ValueError): + raise ToolError(f"{exc}: {exc.__cause__}") from exc.__cause__ + raise def build_mcp_server() -> MCPServer: diff --git a/tests/test_mcp_strict_args.py b/tests/test_mcp_strict_args.py index f68e86c1..cff86143 100644 --- a/tests/test_mcp_strict_args.py +++ b/tests/test_mcp_strict_args.py @@ -12,7 +12,7 @@ call carrying arguments the tool does not declare, with a did-you-mean when one is close. An error the caller sees once beats data half-written forever. """ import pytest -from mcp.server.mcpserver.exceptions import ToolError +from mcp.server.mcpserver.exceptions import ToolError, UnexpectedToolError from scribe.mcp.server import StrictArgsMCPServer @@ -59,6 +59,58 @@ async def test_unknown_tool_keeps_the_upstream_error(): assert "no_such_tool" in str(exc.value) +# --- a refusal's text reaches the agent (#4794) ------------------------------- +# +# SDK 2.x passes on only a ToolError's text; anything else is wrapped as +# "Error executing tool X" and its message is kept on the server. ValueError is +# how every Scribe tool refuses, so for weeks every refusal arrived bare. + + +def _raising_server() -> StrictArgsMCPServer: + mcp = StrictArgsMCPServer("refusal-test") + + @mcp.tool() + def refuse() -> str: + raise ValueError("record 7 is not yours to change — open your own copy") + + @mcp.tool() + def crash() -> str: + raise RuntimeError("password=hunter2 at db.internal:5432") + + return mcp + + +async def test_a_refusal_reaches_the_caller_with_its_reason(): + mcp = _raising_server() + with pytest.raises(ToolError) as exc: + await mcp.call_tool("refuse", {}) + msg = str(exc.value) + assert "Error executing tool refuse" in msg + assert "record 7 is not yours to change — open your own copy" in msg + assert not isinstance(exc.value, UnexpectedToolError), ( + "still typed as a crash, so the SDK's handler would log it as one") + + +async def test_a_crash_stays_masked(): + """Only the refusal convention is unmasked: a crash's text can carry + internals, and the SDK hides it for that reason.""" + mcp = _raising_server() + with pytest.raises(UnexpectedToolError) as exc: + await mcp.call_tool("crash", {}) + assert "hunter2" not in str(exc.value) + + +@pytest.mark.usefixtures("_bind_user") +async def test_a_real_tools_refusal_reaches_the_caller(): + """Against the real server, on a refusal that needs no database: the + message judge_menu writes for an empty verdict list.""" + from scribe.mcp.server import build_mcp_server + + with pytest.raises(ToolError) as exc: + await build_mcp_server().call_tool("judge_menu", {"log_id": 1, "verdicts": []}) + assert "at least one verdict" in str(exc.value) + + async def test_the_original_regression_create_note_with_content(): """Pin #2709 itself against the REAL server: `content=` on create_note must raise before dispatch — naming the bad argument and listing `body`