Log MCPServer handler exceptions by kind and keep crash details off the wire (#3314)
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
This commit is contained in:
parent
f1c40b062f
commit
56af44727b
7 changed files with 1427 additions and 310 deletions
|
|
@ -85,7 +85,7 @@ async def test_call_tool_result_has_three_things_to_read() -> None:
|
|||
|
||||
|
||||
async def test_a_raising_tool_is_a_result_not_an_exception() -> None:
|
||||
"""tutorial003 `!!! check`: the exception's message comes back in content with is_error=True."""
|
||||
"""tutorial003 `!!! check`: the ToolError's message comes back in content with is_error=True."""
|
||||
async with Client(tutorial003.mcp) as client:
|
||||
result = await client.call_tool("lookup_book", {"title": "Solaris"})
|
||||
assert result.is_error
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ order on a modern connection, the `ping` removal, and both `filterwarnings` reci
|
|||
so the prose cannot drift away from what the SDK does.
|
||||
"""
|
||||
|
||||
import logging
|
||||
import warnings
|
||||
|
||||
import pytest
|
||||
|
|
@ -117,20 +118,25 @@ def test_mcp_deprecation_warning_is_a_user_warning() -> None:
|
|||
|
||||
|
||||
@pytest.mark.filterwarnings("error::mcp.MCPDeprecationWarning")
|
||||
async def test_error_filter_turns_the_deprecated_call_into_the_documented_tool_error() -> None:
|
||||
async def test_error_filter_turns_the_deprecated_call_into_the_documented_tool_error(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""The `!!! check`: `"error::mcp.MCPDeprecationWarning"` makes `old_log` fail.
|
||||
|
||||
Under the error filter the warning becomes the raised exception, the tool manager
|
||||
wraps it, and the result is exactly the tool error the page quotes.
|
||||
Under the error filter the warning becomes the raised exception, the tool wrapper treats it as a
|
||||
crash, and the result plus the logged warning are exactly what the page quotes.
|
||||
"""
|
||||
caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server")
|
||||
async with Client(mcp) as client:
|
||||
result = await client.call_tool("old_log", {})
|
||||
assert result.is_error
|
||||
[content] = result.content
|
||||
assert isinstance(content, TextContent)
|
||||
assert content.text == (
|
||||
"Error executing tool old_log: The logging capability is deprecated as of 2026-07-28 (SEP-2577)."
|
||||
)
|
||||
assert content.text == "Error executing tool old_log"
|
||||
(record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert record.exc_info is not None and isinstance(record.exc_info[1], BaseException)
|
||||
assert str(record.exc_info[1].__cause__) == "The logging capability is deprecated as of 2026-07-28 (SEP-2577)."
|
||||
assert type(record.exc_info[1].__cause__).__name__ == "MCPDeprecationWarning"
|
||||
|
||||
|
||||
async def test_filterwarnings_ignore_silences_the_whole_category() -> None:
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
"""`docs/handlers/elicitation.md`: every claim the page makes, proved against the real SDK."""
|
||||
|
||||
import logging
|
||||
from typing import Literal
|
||||
|
||||
import pytest
|
||||
|
|
@ -123,8 +124,7 @@ async def test_an_answer_that_does_not_match_the_schema_never_reaches_the_tool_c
|
|||
async with Client(tutorial001.mcp, mode="legacy", elicitation_callback=on_elicit) as client:
|
||||
result = await client.call_tool("book_table", {"date": "2025-12-25", "party_size": 2})
|
||||
assert result.is_error
|
||||
assert isinstance(result.content[0], TextContent)
|
||||
assert "does not match the requested schema" in result.content[0].text
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool book_table")]
|
||||
|
||||
|
||||
class Address(BaseModel):
|
||||
|
|
@ -158,15 +158,21 @@ async def choose_seating(ctx: Context) -> str:
|
|||
return result.data.area
|
||||
|
||||
|
||||
async def test_a_nested_model_is_rejected_before_anything_is_sent() -> None:
|
||||
"""`!!! warning`: a non-primitive field raises `TypeError` inside `ctx.elicit`, with this exact message."""
|
||||
async def test_a_nested_model_is_rejected_before_anything_is_sent(caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""`!!! warning`: a non-primitive field raises `TypeError` inside `ctx.elicit` with this exact message,
|
||||
which fails the call and lands in the server log rather than on the wire."""
|
||||
caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server")
|
||||
async with Client(schema_gate_server, mode="legacy") as client:
|
||||
result = await client.call_tool("sign_up", {})
|
||||
assert result.is_error
|
||||
assert isinstance(result.content[0], TextContent)
|
||||
assert result.content[0].text == (
|
||||
"Error executing tool sign_up: Elicitation schema field 'address' rendered as "
|
||||
"{'$ref': '#/$defs/Address'}, which is not a valid PrimitiveSchemaDefinition"
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool sign_up")]
|
||||
(record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert record.exc_info is not None and record.exc_info[1] is not None
|
||||
cause = record.exc_info[1].__cause__
|
||||
assert isinstance(cause, TypeError)
|
||||
assert str(cause) == (
|
||||
"Elicitation schema field 'address' rendered as {'$ref': '#/$defs/Address'}, "
|
||||
"which is not a valid PrimitiveSchemaDefinition"
|
||||
)
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -1,17 +1,19 @@
|
|||
"""`docs/servers/handling-errors.md`: every claim the page makes, proved against the real SDK."""
|
||||
|
||||
import logging
|
||||
|
||||
import pytest
|
||||
from mcp_types import INVALID_PARAMS, ErrorData, TextContent, TextResourceContents
|
||||
|
||||
from docs_src.handling_errors import tutorial001, tutorial002, tutorial003
|
||||
from docs_src.handling_errors import tutorial001, tutorial002, tutorial003, tutorial004
|
||||
from mcp import Client, MCPError
|
||||
|
||||
# See test_index.py for why this is a per-module mark and not a conftest hook.
|
||||
pytestmark = [pytest.mark.anyio, pytest.mark.filterwarnings("error::mcp.MCPDeprecationWarning")]
|
||||
|
||||
|
||||
async def test_a_plain_exception_becomes_a_tool_error_the_model_reads() -> None:
|
||||
"""tutorial001: any non-`MCPError` exception comes back as `is_error=True` with the message in `content`."""
|
||||
async def test_tool_error_becomes_a_tool_error_the_model_reads() -> None:
|
||||
"""tutorial001: `ToolError` comes back as `is_error=True` with the message in `content`."""
|
||||
async with Client(tutorial001.mcp) as client:
|
||||
result = await client.call_tool("get_author", {"title": "Nothing"})
|
||||
assert result.is_error
|
||||
|
|
@ -21,6 +23,16 @@ async def test_a_plain_exception_becomes_a_tool_error_the_model_reads() -> None:
|
|||
assert result.structured_content is None
|
||||
|
||||
|
||||
async def test_tool_error_is_one_info_line(caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""tutorial001: on the server a `ToolError` is one INFO record with no traceback."""
|
||||
caplog.set_level(logging.INFO)
|
||||
async with Client(tutorial001.mcp) as client:
|
||||
await client.call_tool("get_author", {"title": "Nothing"})
|
||||
records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)]
|
||||
assert not [r for r in caplog.records if r.levelno >= logging.WARNING]
|
||||
|
||||
|
||||
async def test_a_title_the_catalog_knows_is_an_ordinary_result() -> None:
|
||||
"""tutorial001: the non-raising path is a plain `is_error=False` result."""
|
||||
async with Client(tutorial001.mcp) as client:
|
||||
|
|
@ -55,6 +67,21 @@ async def test_mcp_error_only_fires_on_the_raising_path() -> None:
|
|||
assert result.structured_content == {"result": "Frank Herbert"}
|
||||
|
||||
|
||||
async def test_any_other_exception_is_a_crash_the_model_sees_generically(caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""tutorial004, "Any other exception": the `KeyError` text stays on the server; the model gets only the
|
||||
generic line, and the log gets one ERROR record with the traceback under the documented message."""
|
||||
caplog.set_level(logging.INFO)
|
||||
async with Client(tutorial004.mcp) as client:
|
||||
result = await client.call_tool("get_author", {"title": "Nothing"})
|
||||
assert result.is_error
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool get_author")]
|
||||
(record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert (record.levelno, record.getMessage()) == (logging.ERROR, "Tool 'get_author' raised an unexpected exception")
|
||||
assert record.exc_info is not None
|
||||
logged = record.exc_info[1]
|
||||
assert logged is not None and isinstance(logged.__cause__, KeyError)
|
||||
|
||||
|
||||
async def test_resource_not_found_error_maps_to_invalid_params() -> None:
|
||||
"""tutorial003: `ResourceNotFoundError` from a template handler is `-32602` with the URI in `data`."""
|
||||
async with Client(tutorial003.mcp) as client:
|
||||
|
|
@ -69,12 +96,10 @@ async def test_resource_not_found_error_maps_to_invalid_params() -> None:
|
|||
|
||||
async def test_raise_exceptions_does_not_turn_a_tool_error_into_a_traceback() -> None:
|
||||
"""The closing `!!! info`: even `raise_exceptions=True` leaves a failing tool as the `is_error=True` result."""
|
||||
async with Client(tutorial001.mcp, raise_exceptions=True) as client:
|
||||
async with Client(tutorial004.mcp, raise_exceptions=True) as client:
|
||||
result = await client.call_tool("get_author", {"title": "Nothing"})
|
||||
assert result.is_error
|
||||
assert result.content == [
|
||||
TextContent(type="text", text="Error executing tool get_author: No book titled 'Nothing' in the catalog.")
|
||||
]
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool get_author")]
|
||||
|
||||
|
||||
async def test_a_title_the_template_knows_reads_normally() -> None:
|
||||
|
|
@ -84,3 +109,14 @@ async def test_a_title_the_template_knows_reads_normally() -> None:
|
|||
(contents,) = result.contents
|
||||
assert isinstance(contents, TextResourceContents)
|
||||
assert contents.text == "Dune by Frank Herbert"
|
||||
|
||||
|
||||
async def test_a_bad_argument_is_an_info_line_not_a_crash(caplog: pytest.LogCaptureFixture) -> None:
|
||||
""" "Errors you never raise": schema rejection of the arguments is logged at INFO with no traceback."""
|
||||
caplog.set_level(logging.INFO)
|
||||
async with Client(tutorial001.mcp) as client:
|
||||
result = await client.call_tool("get_author", {"title": 42})
|
||||
assert result.is_error
|
||||
records = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert [(r.levelno, r.exc_info) for r in records] == [(logging.INFO, None)]
|
||||
assert not [r for r in caplog.records if r.levelno >= logging.WARNING]
|
||||
|
|
|
|||
|
|
@ -1,5 +1,7 @@
|
|||
"""`docs/servers/structured-output.md`: every claim the page makes, proved against the real SDK."""
|
||||
|
||||
import logging
|
||||
|
||||
import pytest
|
||||
from inline_snapshot import snapshot
|
||||
from mcp_types import EmbeddedResource, ImageContent, TextContent, TextResourceContents
|
||||
|
|
@ -151,15 +153,19 @@ async def test_dict_str_return_is_not_wrapped() -> None:
|
|||
assert result.structured_content == {"London": 16.2, "Reykjavik": 4.4}
|
||||
|
||||
|
||||
async def test_return_value_is_validated_against_the_schema() -> None:
|
||||
"""tutorial007: a return value that does not match the output schema is a tool error, not a result."""
|
||||
async def test_return_value_is_validated_against_the_schema(caplog: pytest.LogCaptureFixture) -> None:
|
||||
"""tutorial007: a return value that does not match the output schema is a tool error, not a result;
|
||||
the field name goes to the server log, not the client."""
|
||||
caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server")
|
||||
async with Client(tutorial007.mcp) as client:
|
||||
result = await client.call_tool("get_weather", {"city": "London"})
|
||||
assert result.is_error
|
||||
assert result.structured_content is None
|
||||
assert isinstance(result.content[0], TextContent)
|
||||
assert result.content[0].text.startswith("Error executing tool get_weather: 1 validation error for WeatherData")
|
||||
assert "humidity\n Field required" in result.content[0].text
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool get_weather")]
|
||||
(record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert record.getMessage() == "Tool 'get_weather' raised an unexpected exception"
|
||||
assert record.exc_info is not None and record.exc_info[1] is not None
|
||||
assert "1 validation error for WeatherData\nhumidity\n Field required" in str(record.exc_info[1].__cause__)
|
||||
|
||||
|
||||
async def test_structured_output_false_opts_out() -> None:
|
||||
|
|
|
|||
|
|
@ -83,6 +83,28 @@ async def test_a_failing_tool_returns_is_error_true_instead_of_raising() -> None
|
|||
]
|
||||
|
||||
|
||||
async def test_a_crashing_tool_is_the_bare_form_with_its_traceback_in_the_server_log(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""The bare `Error executing tool <name>`: the tool raised something other than ToolError, its text
|
||||
is withheld, and the server log carries the exact ERROR message the page names."""
|
||||
mcp = MCPServer("Weather")
|
||||
|
||||
@mcp.tool()
|
||||
def forecast(city: str) -> str:
|
||||
"""Today's forecast for one city. Crashes: the upstream table is missing the key."""
|
||||
forecasts: dict[str, str] = {}
|
||||
return forecasts[city]
|
||||
|
||||
caplog.set_level(logging.ERROR, logger="mcp.server.mcpserver.server")
|
||||
async with Client(mcp) as client:
|
||||
result = await client.call_tool("forecast", {"city": "Atlantis"})
|
||||
assert result.content == [TextContent(type="text", text="Error executing tool forecast")]
|
||||
(record,) = [r for r in caplog.records if r.name == "mcp.server.mcpserver.server"]
|
||||
assert record.getMessage() == "Tool 'forecast' raised an unexpected exception"
|
||||
assert record.exc_info is not None
|
||||
|
||||
|
||||
async def test_an_unknown_tool_is_the_same_kind_of_result() -> None:
|
||||
"""`Unknown tool: <name>` travels the same `is_error=True` path as a failing tool."""
|
||||
async with Client(tutorial001.mcp) as client:
|
||||
|
|
|
|||
|
|
@ -139,6 +139,19 @@ async def test_safe_join_serves_a_file_inside_the_base_directory(
|
|||
assert content.text == "# Printer setup"
|
||||
|
||||
|
||||
async def test_a_missing_manual_is_resource_not_found(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None:
|
||||
"""tutorial002 and the closing tip: a path with no file behind it is `-32602` with the handler's message."""
|
||||
monkeypatch.setattr(tutorial002, "DOCS_ROOT", tmp_path)
|
||||
async with Client(tutorial002.mcp) as client:
|
||||
with pytest.raises(MCPError) as exc:
|
||||
await client.read_resource("manuals://printing/missing.md")
|
||||
assert exc.value.error == ErrorData(
|
||||
code=INVALID_PARAMS,
|
||||
message="No manual at 'printing/missing.md'.",
|
||||
data={"uri": "manuals://printing/missing.md"},
|
||||
)
|
||||
|
||||
|
||||
def test_safe_join_raises_when_the_resolved_path_escapes_the_base(tmp_path: Path) -> None:
|
||||
"""tutorial002: a path that climbs out of `DOCS_ROOT` raises `PathEscapeError`."""
|
||||
with pytest.raises(PathEscapeError):
|
||||
|
|
|
|||
Loading…
Reference in a new issue