Skip to content

fix: add compatibility handling for non-standard notifications #247

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 7, 2025

Conversation

loocor
Copy link
Contributor

@loocor loocor commented Jun 5, 2025

Summary

This PR addresses the issue where the Rust SDK fails to parse messages containing non-standard notifications (like notifications/stderr from @modelcontextprotocol/server-everything ), causing connection failures and preventing proper MCP communication.

The solution implements graceful handling of non-standard notifications at the transport layer in crates/rmcp/src/transport/async_rw.rs:

  • Introduced is_standard_notification function to check for standard MCP notifications.
  • Added try_parse_with_compatibility function to handle parsing messages with compatibility for non-standard notifications.
  • Updated the decoder implementation to utilize the new compatibility handling.
  • Added unit tests for standard notification checks and compatibility function to ensure correct behavior.

Motivation and Context

Problem: When connecting to MCP servers that send non-standard notifications (such as Claude Desktop's notifications/stderr), the Rust SDK fails to deserialize these messages, causing the entire connection to fail with parsing errors.

Root Cause: The SDK's strict adherence to the MCP specification means any non-standard notification causes a deserialization failure, breaking the communication channel.

Why This Approach: Instead of hardcoding specific non-standard notification types into the core model (which would pollute the standard and require updates for each new non-standard notification), this solution:

  • Preserves the purity of the MCP specification in the core model
  • Provides universal compatibility with any non-standard notification
  • Maintains forward compatibility without code changes
  • Follows the principle of graceful degradation

How Has This Been Tested?

  • Real-world scenario: Tested against Claude Desktop MCP servers that send notifications/stderr
  • Unit tests: Added comprehensive tests for is_standard_notification and try_parse_with_compatibility functions
  • Integration testing: Verified that standard notifications continue to work correctly
  • Edge cases: Tested with malformed JSON, mixed standard/non-standard message streams
  • Logging verification: Confirmed that non-standard notifications are properly logged and ignored

Breaking Changes

None. This is a backward-compatible change that:

  • Does not modify existing APIs or data structures
  • Maintains all existing functionality for standard notifications
  • Only adds graceful handling for previously failing scenarios

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

Design Philosophy: This solution follows the principle of "be liberal in what you accept, conservative in what you send." The SDK remains strict about outgoing messages (maintaining standard compliance) while being tolerant of incoming non-standard notifications.

Alternative Approaches Considered:

  1. Hardcoding specific types (like PR stderr-notification-handler #149): Rejected due to poor scalability and standard pollution
  2. Ignoring all parsing errors: Rejected as it would mask legitimate parsing issues
  3. Configuration-based filtering: Rejected as it adds complexity without significant benefit

Implementation Notes:

  • The is_standard_notification function should be updated when new official notifications are added to the MCP specification
  • Debug logging provides visibility into ignored non-standard notifications for troubleshooting
  • The solution operates at the transport layer, keeping the application layer clean

Future Considerations:

  • Could be extended to support configurable filtering of specific non-standard notifications
  • Metrics could be added to track the frequency of non-standard notifications
  • The approach could be generalized to handle other types of non-standard messages beyond notifications

…nc_rw

- Introduced `is_standard_notification` function to check for standard MCP notifications.
- Added `try_parse_with_compatibility` function to handle parsing messages with compatibility for non-standard notifications.
- Updated the decoder implementation to utilize the new compatibility handling.
- Added unit tests for standard notification checks and compatibility function to ensure correct behavior.
Copy link
Collaborator

@jokemanfire jokemanfire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jokemanfire jokemanfire merged commit 37b4ddb into modelcontextprotocol:main Jun 7, 2025
9 of 10 checks passed
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.

2 participants