Skip notification in initialization handshake#421
Skip notification in initialization handshake#4214t145 merged 2 commits intomodelcontextprotocol:mainfrom
Conversation
github-actions
bot
added
T-core
|
@vorporeal Could you help review this pr? |
crates/rmcp/src/service/client.rs
Outdated
| } | ||
| ServerJsonRpcMessage::Notification(notification) => { | ||
| let notification = serde_json::to_string(¬ification).unwrap_or_default(); | ||
| tracing::debug!( |
There was a problem hiding this comment.
should we use handler's implementation to handle notifications?
There was a problem hiding this comment.
Could you point out how should we do that?
Sorry I am not familiar with the code base enough so I try to keep it simple in this PR. Moreover, the type of MCP doing this doesn't follow the specs (EDIT: specs' recommendations). The connection is not consider established so we only debug log for courtesy. The handler assumes the connection is established right?
There was a problem hiding this comment.
The connection is not consider established so we only debug log for courtesy.
The spec allow both server and client send log and ping notification before initialization. But I am not sure about that should the peer side handle those notifications. I assume that some one may use a customized logger in handler's on_log_notification method, and that person would miss the log before initialization.
Could you point out how should we do that?
We can pass the handler into this function and let the handler handle ping and log notifications. (If all of us think it's reasonable).
There was a problem hiding this comment.
Done! Please review them again.
We handle the initialization process more robustly. - Allow logging and ping - For other messages, we simply ignore it instead of rejecting right away
* fix: handle logging and ping in handshake We handle the initialization process more robustly. - Allow logging and ping - For other messages, we simply ignore it instead of rejecting right away * fix: inject context to notification handler
2 participants
This allows supporting nonconforming MCP servers by skipping notification in initialization handshake.
Fix #412
Motivation and Context
How Has This Been Tested?
Tested with DesktopCommanderMCP. All standard MCP servers continue to work normally.
Breaking Changes
None
Types of changes
Checklist
Additional context