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
Code review found 2 important issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 2 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | src/mcp/client/session.py:531-537 |
Custom client->server notifications are silently discarded on the default Client(server) modern in-process path |
| 🔴 Important | src/mcp/server/runner.py:167-173 |
Warn-once dedup for dropped notifications is ineffective on the modern stream path (fresh ServerRunner per notification) |
| 🟡 Nit | src/mcp/server/session.py:103-119 |
Server-emitted vendor notifications drop with no log on modern HTTP with json_response=True (and debug-only without rela |
| 🟡 Nit | docs/client/callbacks.md:138 |
callbacks.md overclaims: notifications/cancelled (and owned subscription acks) never reach message_handler |
Annotations
Check failure on line 537 in src/mcp/client/session.py
claude / Claude Code Review
Custom client->server notifications are silently discarded on the default Client(server) modern in-process path
Custom client->server notifications sent via the newly-widened `ClientSession.send_notification` are silently discarded on the default `Client(server)` in-process (modern) path: the server side of the dispatcher pair uses the no-op `_no_inbound_client_notifications` (src/mcp/client/client.py:117), so a handler registered with `Server.add_notification_handler` never fires and no log line is emitted at any level — contradicting this PR's new docstring and the low-level-server.md "dropped with a wa
Check failure on line 173 in src/mcp/server/runner.py
claude / Claude Code Review
Warn-once dedup for dropped notifications is ineffective on the modern stream path (fresh ServerRunner per notification)
The once-per-method warn dedup for dropped notifications never engages on the modern (2026-07-28) stream path: `_serve_modern_stream.on_notify` builds a fresh `ServerRunner` (and `Connection.from_envelope`) for every inbound notification, so `_warned_notification_drops` is always empty and every repeat of the same unhandled custom method (or version-gated spec method) logs at WARNING — exactly the log flood this mechanism was added to prevent, and noisier than the pre-PR debug-only behavior on t
Check warning on line 119 in src/mcp/server/session.py
claude / Claude Code Review
Server-emitted vendor notifications drop with no log on modern HTTP with json_response=True (and debug-only without related_request_id)
The server-to-client vendor-notification flow this PR documents and type-legalizes can still drop silently on the modern streamable-HTTP entry: with `json_response=True`, `handle_modern_request` never sets `dctx.sink`, so `ctx.session.send_notification(..., related_request_id=ctx.request_id)` — the exact pattern the new docs prescribe — is discarded with no log at any level; and without `related_request_id`, `_NoChannelOutbound.notify` drops at debug only, while this PR upgraded the receive-side
Check warning on line 138 in docs/client/callbacks.md
claude / Claude Code Review
callbacks.md overclaims: notifications/cancelled (and owned subscription acks) never reach message_handler
The rewritten sentence overclaims: "every notification the negotiated protocol version defines reaches it" is contradicted by the SDK itself — `notifications/cancelled` (defined at every negotiated version) is deliberately swallowed by `ClientSession._on_notify` before the `message_handler` tee, and a well-formed `notifications/subscriptions/acknowledged` owned by a live `listen()` route is consumed as driver state and never tees either. Since this PR re-scoped the sentence to be precise about `