LGTM — telemetry-only additions following the MCP OTel semconv spec, well covered by new tests; the inline docstring nit is worth a quick follow-up but doesn't block.

1 issue found across 2 files

<sub>Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic</sub>

Additional findings (outside current diff — PR may have been updated during review):

• 🔴 src/mcp/server/_otel.py:72-77 — The new {"is_error": True} match arm marks the span as a tool error (error.type="tool_error", StatusCode.ERROR), but the runner's result coercion (serialize_server_result with by_name=False, extra="ignore") silently drops the snake_case is_error key, so the client actually receives a successful result with no isError field — the telemetry contradicts the wire response. Consider dropping the snake_case arm (and test_tool_error_snake_case_dict_result_sets_error_type), or fixing the runner coercion to accept by_name so the snake_case shape really does reach the client as an error.

  Extended reasoning...

  What the bug is

  The post-result match in OpenTelemetryMiddleware treats three shapes as a tool error: CallToolResult(is_error=True), {"isError": True}, and {"is_error": True} (src/mcp/server/_otel.py:74). The PR description justifies the dict arms with "result coercion happens after the middleware runs", i.e. the assumption that the runner will later coerce these dict shapes into an isError: true result on the wire. That assumption holds for the model and the camelCase dict, but not for the snake_case shape — its error flag never reaches the client, so the span reports a tool failure for a request whose wire response is a success.

  The code path

  After the middleware chain returns, ServerRunner._on_request passes the raw dict through _dump_result (returns dicts unchanged, src/mcp/server/runner.py:148-158) and then serialize_server_result(method, version, result) (src/mcp/server/runner.py:295). That function validates against the per-version wire model with adapter.validate_python(data, by_name=False) and dumps with by_alias=True, exclude_none=True (src/mcp/types/methods.py:615-617). The wire CallToolResult declares is_error only under the alias isError with extra="ignore" (src/mcp/types/v2025_11_25/init.py:3144-3155). Although WireModel sets populate_by_name=True, the explicit by_name=False at the call site overrides it, so the snake_case key is_error is treated as an extra and silently discarded.

  Step-by-step proof (reproduced under the repo's pydantic 2.12.5):

  1. A tools/call handler returns {"content": [], "is_error": True}.
  2. The middleware's match arm {"is_error": True} fires → span gets error.type="tool_error" and StatusCode.ERROR.
  3. The runner calls serialize_server_result("tools/call", "2025-11-25", {"content": [], "is_error": True}).
  4. validate_python(..., by_name=False) does not recognize is_error as the field (alias-only matching), and extra="ignore" drops it → is_error=None on the model.
  5. dump_python(by_alias=True, exclude_none=True) produces {"content": []} — no isError key, which the spec defines as success.
  6. The client sees a successful tool call; the exported span reports tool_error / ERROR.

  Why existing tests don't catch it

  The PR's own test_tool_error_snake_case_dict_result_sets_error_type only asserts the span attributes and never inspects the response body, so it passes while encoding the wrong assumption. By contrast, the camelCase test's sibling (test_emits_server_span_with_method_and_target) does assert the wire body for the success case.

  Impact

  Operators see false-positive tool failures (error spans, error rates, alerts) for handlers that return snake_case dicts — failures that never happened on the wire. (Viewed the other way, the deeper pre-existing issue is that the runner silently drops a handler's snake_case error flag; either way, the new arm bakes in an assumption about coercion that does not hold.)

  How to fix

  Either drop the {"is_error": True} arm and its test — only the CallToolResult model and the camelCase isError dict survive coercion as wire errors — or change the runner's result coercion to validate with by_name=True so the snake_case shape genuinely reaches the client as isError: true (in which case the arm and the telemetry would be correct).