stderr-notification-handler #149
Conversation
|
Pardon me I don't find the notifications/stderr in specification, could you cite the reference? |
|
I agree, I couldn't find it in the spec either, but it's in one of the example servers. Please see: https://github.com/modelcontextprotocol/servers/blob/de1abc85a7ddbe408fffc00f783c7e9f1a69b6b3/src/everything/everything.ts#L168. This caused a problematic scenario because it's not in the spec, but an official example server started to use this message type which caused my clients to fail. Another path we could take is having a function for unknown/arbitrary message types rather than killing the stream. What do you think? |
@EItanya That would be great. I think it's the proper way. |
… into stderr-notification
|
@4t145 I was looking into this and it seems tricky to plumb through the customization semantics? I think we would need to pass this into every transport which seems pretty heavy. I can do this but it seems pretty rough... LMK if you have other suggestions else I can do it |
|
@howardjohn Maybe we can add an |
|
I encountered the same issue today and did some testing. @EItanya 's PR solved this problem, but this approach has some concerning tendencies:
My suggested solution is to implement fallback handling in the decode and decode_eof methods of Decoder in /// Check if a notification method is a standard MCP notification
/// should update this when MCP spec is updated about new notifications
fn is_standard_notification(method: &str) -> bool {
matches!(
method,
"notifications/cancelled"
| "notifications/initialized"
| "notifications/message"
| "notifications/progress"
| "notifications/prompts/list_changed"
| "notifications/resources/list_changed"
| "notifications/resources/updated"
| "notifications/roots/list_changed"
| "notifications/tools/list_changed"
)
}
/// Try to parse a message with compatibility handling for non-standard notifications
fn try_parse_with_compatibility<T: serde::de::DeserializeOwned>(
line: &[u8],
context: &str,
) -> Result<Option<T>, JsonRpcMessageCodecError> {
if let Ok(line_str) = std::str::from_utf8(line) {
match serde_json::from_slice(line) {
Ok(item) => Ok(Some(item)),
Err(e) => {
// Check if this is a non-standard notification that should be ignored
if line_str.contains("\"method\":\"notifications/") {
// Extract the method name to check if it's standard
if let Ok(json_value) = serde_json::from_str::<serde_json::Value>(line_str) {
if let Some(method) = json_value.get("method").and_then(|m| m.as_str()) {
if method.starts_with("notifications/")
&& !is_standard_notification(method)
{
tracing::debug!(
"Ignoring non-standard notification {} {}: {}",
method,
context,
line_str
);
return Ok(None); // Skip this message
}
}
}
}
tracing::debug!(
"Failed to parse message {}: {} | Error: {}",
context,
line_str,
e
);
Err(JsonRpcMessageCodecError::Serde(e))
}
}
} else {
serde_json::from_slice(line)
.map(Some)
.map_err(JsonRpcMessageCodecError::Serde)
}
}```
```rust
impl<T: DeserializeOwned> Decoder for JsonRpcMessageCodec<T> {
// ... ...
fn decode(
&mut self,
buf: &mut BytesMut,
) -> Result<Option<Self::Item>, JsonRpcMessageCodecError> {
// ... ...
(false, Some(offset)) => {
// ... ...
// Use compatibility handling function
let item = match try_parse_with_compatibility(line, "decode")? {
Some(item) => item,
None => return Ok(None), // Skip non-standard message
};
return Ok(Some(item));
}
// ... ...
}
}
}
fn decode_eof(&mut self, buf: &mut BytesMut) -> Result<Option<T>, JsonRpcMessageCodecError> {
// ... ...
// Use compatibility handling function
let item = match try_parse_with_compatibility(&line, "decode_eof")? {
Some(item) => item,
None => return Ok(None), // Skip non-standard message
};
Some(item)
}
}
})
}
}``` |
Add a new notification type for stderr notification
Motivation and Context
When I was running an example using the modelcontextprotocol
server-everythingI ran into an issue where the library couldn't parse a new notification type:How Has This Been Tested?
I ran this using my project which builds on this called
agentgatewayBreaking Changes
Types of changes
Checklist
Additional context