Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 25 additions & 4 deletions crates/rmcp/src/handler/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,31 @@ impl<H: ServerHandler> Service<RoleServer> for H {
.list_tools(request.params, context)
.await
.map(ServerResult::ListToolsResult),
ClientRequest::CustomRequest(request) => self
.on_custom_request(request, context)
.await
.map(ServerResult::CustomResult),
ClientRequest::CustomRequest(request) => {

@connor4312 connor4312 Oct 17, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this is identifying a valid problem but this seems like not the right approach. Having to maintain the method lists and mapping in multiple places is fragile and likely to drift over time. Fixing this however requires some rework up to the deserialization step where the distinction between unknown methods and incorrect params is lost.

Instead I would suggest reworking deserialization a little bit into something like this (rough example) where the method is identified in order to deserialize (or fail on) the specific request params

impl<'de> Deserialize<'de> for IncomingRequest {
    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
    where
        D: Deserializer<'de>,
    {
        let raw = RawRequest::deserialize(deserializer)?;

        Ok(match raw.method.as_str() {
            "tools/call" => {
                Self::CallTool(serde_json::from_value(raw.params))
            }
// ...
            _ => Self::Custom(raw),
        })
    }
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Alternative design idea from Gemini, leaning on macros:


  1. Derive the method name from the request type itself (no manual 18-entry table):
    Every typed request is already parameterized by M: ConstString (Request<M, P>, RequestOptionalParam<M, P>, RequestNoParam<M>). Add a trait:
    pub(crate) trait RequestMethod {
        const METHOD: &'static str;
    }
    impl<M: ConstString, P> RequestMethod for Request<M, P> {
        const METHOD: &'static str = M::VALUE;
    }
    impl<M: ConstString, P> RequestMethod for RequestOptionalParam<M, P> {
        const METHOD: &'static str = M::VALUE;
    }
    impl<M: ConstString> RequestMethod for RequestNoParam<M> {
        const METHOD: &'static str = M::VALUE;
    }
  2. In ts_union!, dispatch Deserialize by "method" instead of #[serde(untagged)]:
    • Peek at "method" in the buffered map/value.
    • Match "method" against $(<$V as RequestMethod>::METHOD => ...)*:
      • Deserialize only $V (which is also $O(1)$ instead of trying up to 18 untagged variants sequentially).
      • If $V::deserialize fails, capture the serde error right there (plus _meta into extensions) into a non-public variant on the #[non_exhaustive] enum (e.g., InvalidParams { method: &'static str, error: String, extensions: Extensions }) instead of falling through to CustomRequest and re-parsing later.
    • Only if "method" does not match any <$V as RequestMethod>::METHOD does it deserialize as CustomRequest.
  3. Move lifecycle/capability gating in handle_request into a single pre-check on request.method() (not duplicated in both the match arms and a not_served slice):
    Before match request, run one validate_method_served(self, request.method(), legacy_request, &context)? check that handles:
    • !legacy_request: reject initialize, ping, logging/setLevel, resources/subscribe, resources/unsubscribe with -32601 (fixing the bug where SetLevelRequest and InitializeRequest with valid params currently bypass the !legacy_request check).
    • legacy_request: reject subscriptions/listen with -32601.
    • tasks/get | tasks/update | tasks/cancel: run validate_tasks_capability.
      Then in match request, the individual arms don't need if !legacy_request / validate_tasks_capability at all, and ClientRequest::InvalidParams { method, error, .. } simply returns Err(McpError::invalid_params(format!("invalid params for {method}: {error}"), None)).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, agreed — the second list was the weak part. I'll rework it along those lines in this PR:

  1. a RequestMethod trait so each request type carries its method name;
  2. a Deserialize for ClientRequest that dispatches on method, deserializes only that variant, and keeps its error in a #[doc(hidden)] InvalidParams variant (the enum is already #[non_exhaustive]), with CustomRequest only for unknown methods;
  3. one validate_method_served pre-check, which also closes the bypass you spotted for initialize and logging/setLevel with valid params on 2026-07-28.

Should ServerRequest (sampling and elicitation on the client side) get the same treatment here, or in a follow-up?

AI disclosure: drafted with Claude Code; I reviewed it.

// A known method only lands here when its params failed to
// deserialize; that is the client's mistake, not an unknown method.
// Methods this lifecycle does not serve stay "method not found",
// as the typed arms above answer them (SEP-2575).
let not_served: &[&str] = if legacy_request {
&[SubscriptionsListenRequestMethod::VALUE]
} else {
&[
InitializeResultMethod::VALUE,
PingRequestMethod::VALUE,
SetLevelRequestMethod::VALUE,
SubscribeRequestMethod::VALUE,
UnsubscribeRequestMethod::VALUE,
]
};
if !not_served.contains(&request.method.as_str())
&& let Some(reason) = ClientRequest::known_method_params_error(&request)
{
return Err(McpError::invalid_params(reason, None));
}
self.on_custom_request(request, context)
.await
.map(ServerResult::CustomResult)
}
ClientRequest::GetTaskRequest(request) => {
validate_tasks_capability::<GetTaskMethod, _>(self, &context)?;
self.get_task(request.params, context)
Expand Down
54 changes: 54 additions & 0 deletions crates/rmcp/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4815,6 +4815,60 @@ ts_union!(
| CustomRequest;
);

impl ClientRequest {
/// Why the params of `request` do not fit the method it names, when that
/// method is one this SDK defines.
///
/// `ClientRequest` is untagged, so a request for a known method whose params
/// fail to deserialize lands in [`ClientRequest::CustomRequest`] instead of
/// its typed variant. Reading the request again as that method's type
/// recovers the error, so the server can answer `-32602` rather than treat
/// a known method as unknown. Returns `None` for a method the SDK does not
/// define.
#[cfg(feature = "server")]
pub(crate) fn known_method_params_error(request: &CustomRequest) -> Option<String> {
let mut message = serde_json::Map::new();
message.insert("method".into(), Value::String(request.method.clone()));
if let Some(params) = &request.params {
message.insert("params".into(), params.clone());
}
let message = Value::Object(message);
macro_rules! check {
($($method:ty => $request:ty),* $(,)?) => {
$(
if request.method == <$method as ConstString>::VALUE {
let error = serde_json::from_value::<$request>(message)
.err()
.map_or_else(|| "params do not match the method".to_owned(), |e| e.to_string());
return Some(format!("invalid params for {}: {error}", request.method));
}
)*
};
}
check!(
PingRequestMethod => PingRequest,
InitializeResultMethod => InitializeRequest,
DiscoverRequestMethod => DiscoverRequest,
CompleteRequestMethod => CompleteRequest,
SetLevelRequestMethod => SetLevelRequest,
GetPromptRequestMethod => GetPromptRequest,
ListPromptsRequestMethod => ListPromptsRequest,
ListResourcesRequestMethod => ListResourcesRequest,
ListResourceTemplatesRequestMethod => ListResourceTemplatesRequest,
ReadResourceRequestMethod => ReadResourceRequest,
SubscriptionsListenRequestMethod => SubscriptionsListenRequest,
SubscribeRequestMethod => SubscribeRequest,
UnsubscribeRequestMethod => UnsubscribeRequest,
CallToolRequestMethod => CallToolRequest,
ListToolsRequestMethod => ListToolsRequest,
GetTaskMethod => GetTaskRequest,
UpdateTaskMethod => UpdateTaskRequest,
CancelTaskMethod => CancelTaskRequest,
);
None
}
}

impl ClientRequest {
pub fn method(&self) -> &str {
match &self {
Expand Down
63 changes: 63 additions & 0 deletions crates/rmcp/tests/test_custom_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,3 +188,66 @@ async fn test_custom_server_request_reaches_client() -> anyhow::Result<()> {
client.cancel().await?;
Ok(())
}

/// A request that names a method the SDK defines but whose params do not fit
/// it is answered with `-32602` Invalid params, not `-32601` Method not found,
/// and never reaches `on_custom_request` (#1347).
#[tokio::test]
async fn test_known_method_with_invalid_params_is_invalid_params() -> anyhow::Result<()> {
let (server_transport, client_transport) = tokio::io::duplex(4096);
let payload = Arc::new(Mutex::new(None));
{
let payload = payload.clone();
tokio::spawn(async move {
let server = CustomRequestServer {
receive_signal: Arc::new(Notify::new()),
payload,
}
.serve(server_transport)
.await?;
server.waiting().await?;
anyhow::Ok(())
});
}
let client = ().serve(client_transport).await?;

for (method, params) in [
("tools/call", Some(json!({ "name": 7, "arguments": {} }))),
(
"tools/call",
Some(json!({ "name": "ok", "arguments": "x" })),
),
("tools/call", None),
("prompts/get", Some(json!({ "name": 7 }))),
("resources/read", Some(json!({ "uri": 7 }))),
] {
let result = client
.send_request(ClientRequest::CustomRequest(CustomRequest::new(
method,
params.clone(),
)))
.await;
match result {
Err(rmcp::ServiceError::McpError(error)) => {
assert_eq!(
error.code,
rmcp::model::ErrorCode::INVALID_PARAMS,
"{method} {params:?}: {error:?}"
);
assert!(
error.message.contains(method),
"{method} {params:?}: message {:?} does not name the method",
error.message
);
}
other => panic!("{method} {params:?}: expected Invalid params, got {other:?}"),
}
}
assert!(
payload.lock().await.is_none(),
"a known method with invalid params reached on_custom_request"
);

client.cancel().await?;
Ok(())
}
Loading