Skip to content

Conversation

@kylestratis
Copy link

@kylestratis kylestratis commented Nov 10, 2025

This PR expands the universe of supported server notification types that a client can handle to include all existing types defined in types.py.

Motivation and Context

In the current state, Python clients can only handle a few notifications sent by servers, essentially making those server-sent notifications useless. To empower client developers to handle any kind of notification servers may send them, I created this PR.

How Has This Been Tested?

  • Automated testing in test_custom_notifications.py and test_notification_callbacks.py
  • Runnable demo with several example scenarios

Breaking Changes

None, this approach is fully backward-compatible

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

There are several ServerNotification types that aren't handled by the client, so Python clients are unable to handle them with callbacks. This adds support for notification-specific callbacks in the client session, all except the cancellation notification which is handled natively by the BaseSession. The progress notification is also handled in BaseSession but in that case, _received_notification() is still called, so clients can use a callback to do additional handling of a progress notification if they wish.

@kylestratis
Copy link
Author

New commit does the following:

  • Expands tests
  • Removes handling for custom notifications. This is because the model validation does in the BaseSession class forces notifications to derive from ClientNotification or ServerNotification which are union types, while custom notifications would have to derive from Notification. I'm not sure this strict checking is found in other SDKs, as I am able to send and receive custom notifications to the MCP Inspector.
  • Removes the handler for unknown notifications. The model validation makes that unnecessary, however we still need a fallback in _received_notification() to keep pyright happy. Even though the logic in BaseSession also prevents CancelledNotifications from making it to _received_notification(), pyright complains if we don't handle it, so the fallthrough case itself remains.
  • Updates README and examples to reflect this change.

I'm still not hitting 100% test coverage, but I'll try to improve that in the next few days.

@cliffhall
Copy link
Member

cliffhall commented Nov 12, 2025

From the discussion in the contributor's Discord

There's a reason for the validation. The protocol defines the message types. Clients and servers are usually written by separate entities, and so a client isn't going to know your server's custom notification type or visa versa.

Rather than push a PR to a specific SDK you're using in order to allow an out-of-spec notification pattern, if you feel strongly enough about it and can make an argument for it that could pass muster at a core maintainers meeting, the right approach is to create an SEP and state your case for changing the spec. Then all the SDKs must fall in line.

@kylestratis
Copy link
Author

@cliffhall the most recent commit removes the feature, I don't feel very strongly about it - I see pros and cons on both sides and personally lean towards not worrying about it right now, notifications aren't being heavily used it seems because before this PR clients built with SDK couldn't handle most supported notifications to begin with, and I have only seen a few complaints about this issue here and there. I don't see a need for changing the protocol before there's any clear community desire for it.

I discovered the trick for custom notifications (like many actual features in the SDK) just by reading the code and playing with what seemed possible. The inspector being able to display these notifications hinted that it might be possible to support them clientside, but I didn't notice the validation until I was tightening up the initial set of tests for the feature.

So now the PR just supports client-side handlers for the expected notification types defined in the protocol.

@cliffhall
Copy link
Member

cliffhall commented Nov 12, 2025

So now the PR just supports client-side handlers for the expected notification types defined in the protocol.

@kylestratis You should probably tidy the language in the PR description to indicate that it's not adding support for custom notifications but instead supporting the protocol specified notification types that weren't supported before, i.e., removing mention of test_custom_notifications.py and the verbiage in Additional Context, etc.

@kylestratis
Copy link
Author

So now the PR just supports client-side handlers for the expected notification types defined in the protocol.

@kylestratis You should probably tidy the language in the PR description to indicate that it's not adding support for custom notifications but instead supporting the protocol specified notification types that weren't supported before, i.e., removing mention of test_custom_notifications.py and the verbiage in Additional Context, etc.

Done, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants