Narrow message_handler's parameter to notifications and exceptions - #3168
Merged
Claude / Claude Code Review
completed
Jul 25, 2026 in 25m 9s
Code review found 2 potential issues
Found 2 candidates, confirmed 2. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/mcp/client/session.py:174-175 |
IncomingMessage docstring over-claims: cancellations and consumed listen acks never reach message_handler |
| 🟡 Nit | tests/interaction/_helpers.py:1-6 |
Stale interaction-suite README: _helpers.py no longer holds type aliases |
Annotations
Check warning on line 175 in src/mcp/client/session.py
claude / Claude Code Review
IncomingMessage docstring over-claims: cancellations and consumed listen acks never reach message_handler
The new `IncomingMessage` docstring (and the edited paragraph in `docs/client/callbacks.md`) says `message_handler` receives "every server notification", but two arms of the `ServerNotification` union are deliberately never delivered: `CancelledNotification` (`_on_notify` early-returns on it) and `subscriptions/acknowledged` acks for a registered listen route (consumed by `_intercept_notification`). Consider softening the wording to something like "server notifications the session surfaces (canc
Check warning on line 6 in tests/interaction/_helpers.py
claude / Claude Code Review
Stale interaction-suite README: _helpers.py no longer holds type aliases
The layout guide in tests/interaction/README.md (line 43) still describes `_helpers.py` as 'shared type aliases + the wire-recording transport', but this PR removes the module's last type alias (`IncomingMessage`, now exported from `mcp.client`), leaving only the recording transport. Update that README line to match — e.g. '`_helpers.py` — the wire-recording transport'.
Loading