fix(mcp): a tool's refusal reaches the agent with its reason (#4794)
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 59s
CI & Build / Python tests (push) Successful in 1m52s
CI & Build / Build & push image (push) Successful in 27s
CI & Build / Python lint (push) Successful in 3s
CI & Build / Plugin hooks (push) Successful in 14s
CI & Build / TypeScript typecheck (push) Successful in 53s
CI & Build / integration (push) Successful in 59s
CI & Build / Python tests (push) Successful in 1m52s
CI & Build / Build & push image (push) Successful in 27s
SDK 2.x passes on only a ToolError's text. Any other exception becomes
UnexpectedToolError("Error executing tool X") and its message stays on the
server. ValueError is how every Scribe tool refuses — what was refused, why,
what to do instead — so every refusal reached the agent bare, and it could
only retry blind. Seen live: tune_retrieval(actor="operator") and
judge_menu(verdicts=[]) both answered "Error executing tool …" and nothing
else.
StrictArgsMCPServer.call_tool re-raises an UnexpectedToolError caused by a
ValueError as a ToolError carrying the message, in the SDK's own
"Error executing tool X: <reason>" shape. Other exceptions are crashes and
stay masked. The stale comment claiming the SDK returns ValueError text is
corrected.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -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`
|
||||
|
||||
Reference in New Issue
Block a user