fix(time): stop discarding McpError in the call_tool handler

[tounsils]

Description

get_zoneinfo raises a typed error so a caller can tell a bad argument from a server fault:

raise McpError(ErrorData(code=INVALID_PARAMS, message=f"Invalid timezone: {str(e)}"))

The call_tool handler then catches it with a blanket except Exception and re-raises it as a plain ValueError (src/time/src/mcp_server_time/server.py:215). The code is discarded and the message is prefixed a second time:

get_current_time(timezone="Invalid/Zone")

before: "Error processing mcp-server-time query: Invalid timezone: 'No time zone found with key Invalid/Zone'"
after:  "Invalid timezone: 'No time zone found with key Invalid/Zone'"

The fetch server, the other Python server in this repo, already lets McpError propagate untouched.

Changes

• Re-raise McpError ahead of the blanket handler. Every other exception keeps the existing prefix, so nothing else changes on the wire.
• Extract build_server() from serve(). The handler previously had no test coverage because it was only reachable through serve(), which binds stdio; the extraction lets a test drive it over an in-memory session. serve() is otherwise unchanged and still does exactly what it did.

Testing

uv run pyright src    # 0 errors, 0 warnings
uv run pytest test/   # 41 passed (38 before), 0 skipped

Three tests added against the handler: the McpError path, the generic-error path, and a success path. The first was confirmed red with only the except McpError: raise removed:

FAILED test/time_server_test.py::test_call_tool_preserves_mcp_error_for_invalid_timezone
  assert "Error processing mcp-server-time query: Invalid timezone: ...".startswith("Invalid timezone:")

The new tests pin anyio_backend to asyncio via a fixture, since trio is not a dev dependency here and the anyio plugin would otherwise parametrize over both.

Motivation and Context

The typed error exists so a client can distinguish a correctable argument from a server fault. Flattening it to ValueError removes that distinction at the only point where it reaches the caller.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

[Copilot]

🟡 Changes recommended

MCP SDK 1.29.0 still converts the re-raised error into an untyped tool result, so INVALID_PARAMS is not preserved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes time-server error handling and makes the server testable in memory.

Changes:

• Re-raises McpError before generic exception wrapping.
• Extracts build_server().
• Adds handler-level tests.

File summaries

File	Description
src/time/src/mcp_server_time/server.py	Refactors server creation and error handling.
src/time/test/time_server_test.py	Tests success and error paths through an in-memory session.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

MCP 1.29 still converts the re-raised error into an untyped tool-error result.

Review details

Suppressed comments (1)

src/time/src/mcp_server_time/server.py:218

• Re-raising here still does not preserve INVALID_PARAMS on the wire. With the locked MCP 1.29 SDK, the wrapper installed by @server.call_tool() catches every Exception from this handler and converts it to CallToolResult(isError=True), so the outer JSON-RPC handler never receives this McpError. The new test's assert result.isError confirms that the typed protocol error is still flattened; only the duplicate prefix is removed. Please route this through a request handler/API that can emit the original ErrorData (or first fix/use an SDK API that lets McpError escape), and assert that session.call_tool(...) raises an McpError whose code is INVALID_PARAMS.

        except McpError:
            # Already carries a specific code and message (e.g. INVALID_PARAMS
            # from get_zoneinfo); re-wrapping would discard both.
            raise

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Balanced