Narrow message_handler's parameter to notifications and exceptions - #3168
Narrow message_handler's parameter to notifications and exceptions#3168maxisbey merged 3 commits into
Conversation
Sorry, something went wrong.
The client's message_handler is only ever called with a validated ServerNotification or a transport-level Exception; server-initiated requests are answered by the typed callbacks. The RequestResponder arm of the MessageHandlerFnT union was a v1 leftover with a typing-only stub behind it, so every handler had to annotate a case the session never delivers. Type the parameter as an exported IncomingMessage alias (ServerNotification | Exception) in mcp.client, delete the dead RequestResponder stub, and drop the now-empty mcp.shared.session compatibility module (ProgressFnT already lives in mcp.shared.dispatcher).
📚 Documentation preview
|
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 26 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
There was a problem hiding this comment.
Beyond the two inline doc nits, I also checked for dangling importers of the deleted mcp.shared.session module and the removed RequestResponder stub — no references remain anywhere in src/, tests/, examples/, or docs/ outside the migration guide's own removal notes, so the deletion is complete. Also confirmed the _evicting_message_handler wrapper in client.py and the default handler were both updated to the narrowed IncomingMessage signature.
Extended reasoning...The substantive risk in this PR is the removal of a public compat module (mcp.shared.session) and the narrowing of MessageHandlerFnT's parameter. I grepped the whole repo for shared.session and RequestResponder: the only hits are in docs/migration.md, which documents the removal, so no code path still imports the deleted names. The narrowing itself is safe by contravariance (handlers annotated with a wider type, e.g. object, still type-check), and runtime behavior is unchanged — _on_notify/_on_stream_exception only ever passed ServerNotification | Exception. The two inline findings are documentation nits (docstring over-claim, stale interaction-suite README line) and don't affect correctness.
Sorry, something went wrong.
IncomingMessage's docstring and the callbacks page said every server notification reaches message_handler, but notifications/cancelled is applied by the dispatcher and a subscription ack for a live listen() stream is consumed by that stream, so neither is delivered. Say so. Also drop the "shared type aliases" mention from the interaction suite's layout guide now that _helpers.py holds only the recording transport. No-Verification-Needed: comment- and doc-only change
There was a problem hiding this comment.
Beyond the inline finding, I verified the core semantic claim behind the narrowing: _message_handler is only ever invoked with a parsed ServerNotification (_on_notify, after the CancelledNotification guard) or a transport-level Exception (_deliver_stream_exception) — no server-request path reaches it, so the removed RequestResponder arm was genuinely dead. Also checked for stragglers: no imports of mcp.shared.session or RequestResponder remain anywhere in src/tests/examples (only migration.md prose), and the previous review's two findings (the "every server notification" over-claim and the interaction-suite README line) are both addressed in the current revision.
Extended reasoning...One nit was filed inline (whats-new.md's removed-import-paths list), so this note only records what else was examined and ruled out this run: the runtime delivery path confirms the narrowed IncomingMessage union matches actual behavior, no stale imports of the deleted module survive, and the prior run's docstring/README feedback was resolved by commit 9012574. This is informational context for the author and any later review pass — not an approval.
Sorry, something went wrong.
…sion removal The class docstring claimed transport exceptions reach message_handler only when the session builds its own dispatcher, but a caller-supplied JSONRPCDispatcher with no stream-exception hook is wired up the same way (the standard Client path), so exceptions arrive on any stream-backed dispatcher. Say that instead. Also add mcp.shared.session and its RequestResponder stub to the removed-import-paths bullet in whats-new. No-Verification-Needed: docstring- and doc-only change
There was a problem hiding this comment.
LGTM — the typing-only narrowing is sound and all feedback from my earlier passes has been addressed.
What was reviewed this run:
- Confirmed no dangling references to mcp.shared.session or RequestResponder remain anywhere in src/, tests/, or examples/ — the only mentions left are the migration/whats-new docs describing the removal.
- Checked src/mcp/shared/ on disk: session.py is gone and nothing in mcp/__init__.py re-exported from it.
- Verified the ProgressFnT import swap in session_group.py and test_progress.py points at its real home, mcp.shared.dispatcher.
- Re-checked the delivery path in ClientSession._on_notify/_intercept_notification: only validated ServerNotifications and stream Exceptions reach message_handler, so the narrowed IncomingMessage union matches what actually happens at runtime.
Overview
This PR narrows MessageHandlerFnT's parameter from the v1 union RequestResponder[ServerRequest, ClientResult] | ServerNotification | Exception to a new exported alias mcp.client.IncomingMessage = ServerNotification | Exception, deletes the runtime-dead RequestResponder typing stub, and removes the mcp.shared.session compatibility module whose only remaining content was a ProgressFnT re-export. The other 20+ changed files are mechanical annotation swaps in tests, examples, and docs, plus migration-guide and whats-new entries for the breaking change.
Security risks
None. This is a typing and public-API-surface change with no new runtime logic: the RequestResponder arm being deleted was a stub the SDK never instantiated, and the dispatch code in ClientSession (_on_notify, _on_request, _deliver_stream_exception) is untouched apart from a docstring correction. No auth, crypto, parsing, or transport behavior changes.
Level of scrutiny
Medium, applied across four review passes total. The runtime risk is near zero — I traced the delivery path and confirmed message_handler only ever receives a parsed ServerNotification or a transport Exception, so the narrowed union cannot exclude a value that actually arrives, and the parameter position is contravariant so downstream handlers with wider annotations still type-check. The real risk surface was API/docs consistency for a breaking change, which is where the three earlier findings landed (over-claiming "every server notification" in the docstring and callbacks page, a stale tests/interaction/README.md layout line, and the missing mcp.shared.session entry in whats-new's removed-import-paths bullet). All three were fixed in 9012574 and ebb173c, and I verified the fixes are in the current diff. A repo-wide grep confirms no code still imports the deleted module or class.
Other factors
Breaking changes are expected policy on this branch (main is the v2 rework), and the migration guide documents the removal with the exact replacement spelling, satisfying the repo's own bar. The new export in mcp.client.__all__ is exactly what the test suite's own TODO in tests/interaction/_helpers.py asked for, so the API addition is a deliberate resolution of known friction rather than a drive-by re-export. The author states the full suite passes at 100% coverage with end-to-end verification of both union arms, and every prior reviewer thread (including cubic's) is resolved. Nothing outstanding remains, so approving rather than posting another deferral.
Sorry, something went wrong.
ClientSession only ever calls message_handler with a validated ServerNotification or a transport-level Exception — server-initiated requests are answered by the typed callbacks (sampling_callback, elicitation_callback, list_roots_callback) and never reach it. Yet MessageHandlerFnT still typed the parameter as RequestResponder[ServerRequest, ClientResult] | ServerNotification | Exception, a v1 leftover backed by a typing-only stub the SDK never instantiates. Every handler had to annotate a dead arm, and because the union had no exported name, writing a correctly typed handler meant importing that stub from mcp.shared.session and assembling the union by hand (the tests had a TODO complaining about exactly this).
This narrows the contract to what actually happens:
The parameter is contravariant, so a handler annotated more loosely (e.g. message: object) still type-checks; only the removed RequestResponder spelling stops importing, which the migration guide covers.
Motivation and Context
The RequestResponder arm was unreachable at runtime and existed solely so v1-shaped annotations kept importing. Making the illegal state unrepresentable removes a phantom case from every message handler.
How Has This Been Tested?
Existing tests updated to the new alias; full suite passes at 100% coverage. Verified end-to-end by running the standalone_get (stdio + HTTP) and stickynotes story examples with IncomingMessage-typed handlers, plus small public-API probes confirming both arms are delivered — a LoggingMessageNotification via MCPServer's ctx.info() and a ConnectionError injected on the client read stream — and that import mcp.shared.session now fails cleanly.
Breaking Changes
Yes, documented in docs/migration.md:
Types of changes
Checklist
AI Disclaimer