Typed custom server notifications; warn on unbound client-side drops - #3173
Typed custom server notifications; warn on unbound client-side drops#3173maxisbey wants to merge 2 commits into
Conversation
Widen ClientSession.send_notification and ServerSession.send_notification to accept any Notification subclass, mirroring send_request's open Request arm, so extensions can emit their own methods without casts. Raise dropped-notification logging from debug to a once-per-method warning that names the remedy: on the client when no NotificationBinding observes the method, on the server when a spec notification is not defined at the negotiated version or a custom method has no registered handler (an unserved spec notification stays at debug; repeat drops of a method log at debug so a stream cannot flood the log). Document the message_handler routing change and the binding channel in the migration guide, fix the callbacks page's catch-all claim, and cover the vendor-notification round trip with an interaction test, un-deferring its requirement.
📚 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 13 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 inline findings, one other candidate was examined and ruled out this run: unbounded growth of the client-side _warned_notification_drops dedup set under peer-controlled unique method names (src/mcp/client/session.py) — the set is session-scoped and holds only small method strings, so its lifetime and cost are bounded by the session itself.
Extended reasoning...
Bugs were found and are posted as inline comments (the modern in-process notification delivery gap, the per-message ServerRunner dedup-scope issue, plus two doc/observability nits), so the inline comments already signal that human review is needed. This note only records the one additional candidate that finder agents raised and verifiers refuted — the client-side warn-dedup set growing under peer-controlled method names — so a later pass does not re-explore it. I confirmed the set at src/mcp/client/session.py:366 is per-ClientSession, making the growth bounded by session lifetime; a peer flooding unique methods costs more in notification processing than in set entries.
…debug Drop the once-per-method dedup and its per-runner state: the client now warns on every unbound notification (registering a binding, even a no-op one, is the silence), and server-side inbound drops return to debug since the peer is an arbitrary client and warning on its input floods the log. Revert the ClientSession.send_notification widening: the 2026-07-28 transports do not agree on client-to-server custom notifications yet, so the closed union stays the accurate type. Carve notifications/cancelled and listen-owned acks out of the message_handler catch-all claim, add the imports the notification example was missing, and note that a json_response answer has no stream to carry request-scoped notifications.
maxisbey
changed the title
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
LGTM — the revision since my last review resolves all four findings from that pass: ClientSession.send_notification stays closed (avoiding the silently-dropped in-process client→server path), the server-side warn-dedup that couldn't survive the per-message modern runner is gone in favor of keeping server drops at debug, the docs now caveat the json_response=True outbound drop, and the message_handler sentence carves out notifications/cancelled and consumed listen acks.
What was reviewed this run:
- The per-event client WARNING as a peer-controlled log surface — examined and ruled out: it matches the existing per-event
Failed to validate notificationwarning in the same path, the peer is a server the client chose, and the choice is deliberate and pinned by test. - The
ServerSession.send_notificationwidening — annotation-only; the runtime dump-and-send path is unchanged and mirrorsClientSession.send_request's openRequest[Any, Any]arm. - Doc/migration examples —
NotificationBindingis exported frommcp.client,TaskStatusNotificationParamsexists inmcp_types, and the low-level-server example now includes its imports.
Extended reasoning...
Overview
The runtime delta is two small changes: ServerSession.send_notification widens its parameter annotation to types.ServerNotification | types.Notification[Any, Any] (the body — dump and channel.notify — is untouched, so this legalizes existing behaviour), and the client's unbound-notification drop in ClientSession._on_notify moves from logger.debug to logger.warning with a message naming the remedy. The rest is docs (extensions, low-level server, callbacks, migration, whats-new), test updates pinning the new log level, and a new interaction test that drives the full server→client vendor-notification loop, un-deferring the extensions:client:notification-binding-delivery requirement (note= is a valid Requirement field).
Security risks
None identified. No auth, crypto, or permission code is touched. The one attack-surface question — a peer flooding the client log with WARNING lines via unique unbound methods — was raised by a finder and refuted by verification this run: the same code path already warns per-event on malformed notifications, the client's peer is a server the user chose to connect to, the message contains only the method name and version (no payload), and the per-event choice is deliberate (commit "Warn per event on unbound client notifications; keep server drops at debug") with a test asserting [WARNING, WARNING] for repeats. Server-side inbound drops stay at debug (runner.py:258), so the untrusted-client direction gains no new log surface.
Level of scrutiny
This deserved a careful pass because the previous revision had two real 🔴 findings (a silently-dropped in-process client→server path behind a widened ClientSession.send_notification, and a warn-dedup set scoped to a per-message ServerRunner). The current revision resolves both structurally rather than patching them: the client-side type stays closed (verified at src/mcp/client/session.py:531), and the dedup mechanism no longer exists on either side. The two 🟡 doc overclaims are also fixed — callbacks.md now carves out notifications/cancelled and consumed listen acks, and low-level-server.md states the json_response=True drop caveat. Cubic's points (stale first-drop-warns dedup wording, missing example imports) are moot or fixed in this revision.
Other factors
Test coverage is strong and behaviour-pinning: the session-tier tests assert the exact log message and level for both the version-gated and vendor-method drop branches, and the new interaction test exercises the documented emit pattern (ctx.session.send_notification(..., related_request_id=ctx.request_id)) end-to-end through a ClientExtension binding. The migration guide documents the one v1 delivery difference (notifications/tasks/status), satisfying the repo's breaking-change documentation rule, and the referenced symbols in the doc examples all exist (NotificationBinding exported from mcp.client, TaskStatusNotificationParams in mcp_types). Remaining outbound-drop observability gaps are pre-existing, now documented, and explicitly scoped to follow-ups in the PR description.
1 participant
A server can now send a custom (non-spec) notification through the typed surface, and a client that drops one leaves a warning naming the fix.
Motivation and Context
Feedback from a downstream framework: a server-to-client notification whose method isn't in the negotiated version's core tables only reaches a registered
NotificationBinding, and otherwise is dropped atlogger.debug— quiet enough that the fix on their side was found by trial and error. Separately, emitting such a notification from the server needed acast()or# type: ignore, becauseServerSession.send_notificationis typed to the closed spec union even though the runtime already sends whatever it's given.Two changes:
ServerSession.send_notificationaccepts anymcp_types.Notificationsubclass, matching the openRequest[Any, Any]armsend_requestalready has. Runtime is unchanged; this legalizes the existing behaviour.debugto awarningthat names the remedy (register aNotificationBinding; a no-op binding is the "I know, ignore it").Server-side inbound drops stay at
debug: the peer there is an arbitrary client, and warning per untrusted input is a log-flooding surface. Nothing is teed tomessage_handlereither — its contract is the typed spec union, and v1 dropped unknown methods before it too. The one v1 delivery difference isnotifications/tasks/status, which was a member of v1's union; the migration guide now says where it went and how to observe it.ClientSession.send_notificationis intentionally left closed. The 2026-07-28 transports don't agree on client-to-server custom notifications yet (the modern stdio loop dispatches them, streamable HTTP answers a notification POST with an error, the in-process path receives none), so the closed type is the accurate one until that's decided.How Has This Been Tested?
Unit tests pin the client drop as a per-event
WARNING. A new interaction test drives the full loop: anMCPServertool emits vendor notifications throughctx.session.send_notification(..., related_request_id=ctx.request_id)and the declaringClientExtension's binding receives them in order, un-deferring the previously deferredextensions:client:notification-binding-deliveryrequirement. Also exercised end-to-end against a running streamable HTTP server at2026-07-28.Breaking Changes
None. The widened annotation only accepts more; the log-level change is observability.
Types of changes
Checklist
Additional context
Deliberately out of scope, happy to file follow-ups: a catch-all handler for unobserved notifications, a
Client(notification_bindings=...)kwarg (a one-off binding on the high-levelClientcurrently needs aClientExtension, which also advertises its identifier), anotifications()contribution on the serverExtension, and observability for the outbound drop sites (ajson_response=Trueresponse has no stream, so a notification sent during it is discarded).AI Disclaimer