Narrow message_handler's parameter to notifications and exceptions#3168
Conversation
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
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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.
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.
…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.sessionorRequestResponderremain anywhere insrc/,tests/, orexamples/— the only mentions left are the migration/whats-new docs describing the removal. - Checked
src/mcp/shared/on disk:session.pyis gone and nothing inmcp/__init__.pyre-exported from it. - Verified the
ProgressFnTimport swap insession_group.pyandtest_progress.pypoints at its real home,mcp.shared.dispatcher. - Re-checked the delivery path in
ClientSession._on_notify/_intercept_notification: only validatedServerNotifications and streamExceptions reachmessage_handler, so the narrowedIncomingMessageunion matches what actually happens at runtime.
Extended reasoning...
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.
1 participant
ClientSessiononly ever callsmessage_handlerwith a validatedServerNotificationor a transport-levelException— server-initiated requests are answered by the typed callbacks (sampling_callback,elicitation_callback,list_roots_callback) and never reach it. YetMessageHandlerFnTstill typed the parameter asRequestResponder[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 frommcp.shared.sessionand assembling the union by hand (the tests had a TODO complaining about exactly this).This narrows the contract to what actually happens:
mcp.client.IncomingMessage = ServerNotification | Exception, whichMessageHandlerFnT.__call__now takes.RequestResponderstub is deleted, and with it themcp.shared.sessioncompatibility module — its only remaining content was aProgressFnTre-export whose home is alreadymcp.shared.dispatcher.The parameter is contravariant, so a handler annotated more loosely (e.g.
message: object) still type-checks; only the removedRequestResponderspelling stops importing, which the migration guide covers.Motivation and Context
The
RequestResponderarm 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) andstickynotesstory examples withIncomingMessage-typed handlers, plus small public-API probes confirming both arms are delivered — aLoggingMessageNotificationviaMCPServer'sctx.info()and aConnectionErrorinjected on the client read stream — and thatimport mcp.shared.sessionnow fails cleanly.Breaking Changes
Yes, documented in
docs/migration.md:message_handler's parameter type isIncomingMessage(ServerNotification | Exception); replace the hand-written v1 union with it.RequestResponderand themcp.shared.sessionmodule are removed.ProgressFnTlives inmcp.shared.dispatcher;RequestIdinmcp_types.Types of changes
Checklist
AI Disclaimer