Skip to content

Commit 6a072fb

Browse files
benbrandtbenbrandt
committed
Refine params parsing
1 parent ebd50c4 commit 6a072fb

16 files changed

Lines changed: 299 additions & 58 deletions

File tree

‎src/agent-client-protocol-core/src/jsonrpc.rs‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ use crate::jsonrpc::task_actor::{Task, TaskTx};
3535
use crate::mcp_server::McpServer;
3636
use crate::role::HasPeer;
3737
use crate::role::Role;
38-
use crate::util::json_cast;
38+
use crate::util::json_cast_params;
3939
use crate::{Agent, Client, ConnectTo, RoleId};
4040

4141
/// Handlers process incoming JSON-RPC messages on a connection.
@@ -2198,8 +2198,16 @@ pub trait JsonRpcMessage: 'static + Debug + Sized + Send + Clone {
21982198

21992199
/// Parse this type from a method name and parameters.
22002200
///
2201-
/// Returns an error if the method doesn't match or deserialization fails.
2202-
/// Callers should use `matches_method` first to check if this type handles the method.
2201+
/// Return `crate::Error::method_not_found()` only when `method` is not handled by this
2202+
/// type. When the method matches, any other error is treated as terminal for that dispatch:
2203+
/// the message will be rejected rather than falling through to later handlers for the same
2204+
/// method.
2205+
///
2206+
/// For incoming request/notification params, prefer `crate::util::json_cast_params()` so
2207+
/// malformed payloads become `crate::Error::invalid_params()`.
2208+
///
2209+
/// For backward compatibility, the incoming dispatch matchers still normalize legacy
2210+
/// `crate::ErrorCode::ParseError` values from older custom parsers into `Invalid params`.
22032211
fn parse_message(method: &str, params: &impl Serialize) -> Result<Self, crate::Error>;
22042212
}
22052213

@@ -2460,8 +2468,14 @@ impl<Req: JsonRpcRequest, Notif: JsonRpcMessage> Dispatch<Req, Notif> {
24602468
}
24612469
}
24622470

2471+
/// Normalize legacy parse errors from matched incoming request/notification params.
2472+
///
2473+
/// `MethodNotFound` still means "this type does not handle the method" and therefore falls
2474+
/// through to later handlers. The SDK's own request/notification parsers now return
2475+
/// `InvalidParams` directly; this rewrite remains only as a compatibility backstop for older
2476+
/// custom parsers that still emit `ParseError`.
24632477
fn normalize_incoming_message_parse_error(err: crate::Error) -> Option<crate::Error> {
2464-
match err.code {
2478+
match &err.code {
24652479
crate::ErrorCode::MethodNotFound => None,
24662480
crate::ErrorCode::ParseError => {
24672481
let mut invalid_params = crate::Error::invalid_params();
@@ -2522,6 +2536,9 @@ pub enum TypedDispatchMatch<Req: JsonRpcRequest, Notif: JsonRpcNotification> {
25222536

25232537
impl Dispatch {
25242538
/// Match this dispatch against a request type without using `Err` for parse failures.
2539+
///
2540+
/// Once `Req::matches_method` is true, any parse error is terminal for that method.
2541+
/// The request is rejected rather than offered to later handlers for the same method.
25252542
#[must_use]
25262543
pub fn match_request<Req: JsonRpcRequest>(self) -> RequestMatch<Req> {
25272544
match self {
@@ -2548,6 +2565,9 @@ impl Dispatch {
25482565
}
25492566

25502567
/// Match this dispatch against a notification type without using `Err` for parse failures.
2568+
///
2569+
/// Once `Notif::matches_method` is true, any parse error is terminal for that method.
2570+
/// The notification is rejected rather than offered to later handlers for the same method.
25512571
#[must_use]
25522572
pub fn match_notification<Notif: JsonRpcNotification>(self) -> NotificationMatch<Notif> {
25532573
match self {
@@ -2575,6 +2595,10 @@ impl Dispatch {
25752595

25762596
/// Match this dispatch against typed request and notification types without using `Err`
25772597
/// for parse failures.
2598+
///
2599+
/// Once the request/notification side matches by method, any parse error is terminal for
2600+
/// that method and yields [`TypedDispatchMatch::Rejected`]. Likewise, a matched response
2601+
/// whose result cannot be decoded is rejected instead of bubbling up as a fatal `Err`.
25782602
pub fn match_typed_dispatch<Req: JsonRpcRequest, Notif: JsonRpcNotification>(
25792603
self,
25802604
) -> TypedDispatchMatch<Req, Notif> {
@@ -2659,7 +2683,7 @@ impl Dispatch {
26592683
let Some(value) = message.params().get("sessionId") else {
26602684
return Ok(None);
26612685
};
2662-
let session_id = serde_json::from_value(value.clone())?;
2686+
let session_id = json_cast_params(value)?;
26632687
Ok(Some(session_id))
26642688
}
26652689
}
@@ -2735,7 +2759,11 @@ impl UntypedMessage {
27352759
id: Option<jsonrpcmsg::Id>,
27362760
) -> Result<jsonrpcmsg::Request, crate::Error> {
27372761
let Self { method, params } = self;
2738-
Ok(jsonrpcmsg::Request::new_v2(method, json_cast(params)?, id))
2762+
Ok(jsonrpcmsg::Request::new_v2(
2763+
method,
2764+
crate::util::json_cast_params(params)?,
2765+
id,
2766+
))
27392767
}
27402768
}
27412769

‎src/agent-client-protocol-core/src/role.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ use std::{any::TypeId, fmt::Debug, future::Future, hash::Hash};
99
use serde::{Deserialize, Serialize};
1010

1111
use crate::schema::{METHOD_SUCCESSOR_MESSAGE, SuccessorMessage};
12-
use crate::util::json_cast;
1312
use crate::{Builder, ConnectionTo, Dispatch, Handled, JsonRpcMessage, UntypedMessage};
1413

1514
/// Roles for the ACP protocol.
@@ -230,7 +229,14 @@ where
230229
"Response variant cannot be unwrapped as SuccessorMessage",
231230
)
232231
})?;
233-
let SuccessorMessage { message, meta } = json_cast(untyped_message.params())?;
232+
let SuccessorMessage { message, meta } =
233+
match crate::util::json_cast_params(untyped_message.params()) {
234+
Ok(message) => message,
235+
Err(error) => {
236+
dispatch.respond_with_error(error, connection.clone())?;
237+
return Ok(Handled::Yes);
238+
}
239+
};
234240
let successor_dispatch = dispatch.try_map_message(|_| Ok(message))?;
235241
tracing::trace!(
236242
unwrapped_method = %successor_dispatch.method(),

‎src/agent-client-protocol-core/src/role/acp.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,14 @@ where
279279
MatchDispatchFrom::new(message, &connection)
280280
.if_message_from(Agent, async |message| {
281281
// If this is for our session-id, proxy it to the client.
282-
if let Some(session_id) = message.get_session_id()?
282+
let session_id = match message.get_session_id() {
283+
Ok(session_id) => session_id,
284+
Err(error) => {
285+
message.respond_with_error(error, connection.clone())?;
286+
return Ok(Handled::Yes);
287+
}
288+
};
289+
if let Some(session_id) = session_id
283290
&& session_id == self.session_id
284291
{
285292
connection.send_proxied_message_to(Client, message)?;

‎src/agent-client-protocol-core/src/schema/mod.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ macro_rules! impl_jsonrpc_request {
3636
if method != $method {
3737
return Err($crate::Error::method_not_found());
3838
}
39-
$crate::util::json_cast(params)
39+
$crate::util::json_cast_params(params)
4040
}
4141
}
4242

@@ -84,7 +84,7 @@ macro_rules! impl_jsonrpc_notification {
8484
if method != $method {
8585
return Err($crate::Error::method_not_found());
8686
}
87-
$crate::util::json_cast(params)
87+
$crate::util::json_cast_params(params)
8888
}
8989
}
9090

@@ -133,10 +133,10 @@ macro_rules! impl_jsonrpc_request_enum {
133133
params: &impl serde::Serialize,
134134
) -> Result<Self, $crate::Error> {
135135
match method {
136-
$( $(#[$meta])* $method => $crate::util::json_cast(params).map(Self::$variant), )*
136+
$( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )*
137137
_ => {
138138
if let Some(custom_method) = method.strip_prefix('_') {
139-
$crate::util::json_cast(params).map(
139+
$crate::util::json_cast_params(params).map(
140140
|ext_req: $crate::schema::ExtRequest| {
141141
Self::$ext_variant($crate::schema::ExtRequest::new(
142142
custom_method.to_string(),
@@ -196,10 +196,10 @@ macro_rules! impl_jsonrpc_notification_enum {
196196
params: &impl serde::Serialize,
197197
) -> Result<Self, $crate::Error> {
198198
match method {
199-
$( $(#[$meta])* $method => $crate::util::json_cast(params).map(Self::$variant), )*
199+
$( $(#[$meta])* $method => $crate::util::json_cast_params(params).map(Self::$variant), )*
200200
_ => {
201201
if let Some(custom_method) = method.strip_prefix('_') {
202-
$crate::util::json_cast(params).map(
202+
$crate::util::json_cast_params(params).map(
203203
|ext_notif: $crate::schema::ExtNotification| {
204204
Self::$ext_variant($crate::schema::ExtNotification::new(
205205
custom_method.to_string(),

‎src/agent-client-protocol-core/src/schema/proxy_protocol.rs‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,8 @@ impl<M: JsonRpcMessage> JsonRpcMessage for SuccessorMessage<M> {
5050
if method != METHOD_SUCCESSOR_MESSAGE {
5151
return Err(crate::Error::method_not_found());
5252
}
53-
let outer = crate::util::json_cast::<_, SuccessorMessage<UntypedMessage>>(params)?;
53+
let outer =
54+
crate::util::json_cast_params::<_, SuccessorMessage<UntypedMessage>>(params)?;
5455
if !M::matches_method(&outer.message.method) {
5556
return Err(crate::Error::method_not_found());
5657
}
@@ -161,7 +162,8 @@ impl<M: JsonRpcMessage> JsonRpcMessage for McpOverAcpMessage<M> {
161162
if method != METHOD_MCP_MESSAGE {
162163
return Err(crate::Error::method_not_found());
163164
}
164-
let outer = crate::util::json_cast::<_, McpOverAcpMessage<UntypedMessage>>(params)?;
165+
let outer =
166+
crate::util::json_cast_params::<_, McpOverAcpMessage<UntypedMessage>>(params)?;
165167
if !M::matches_method(&outer.message.method) {
166168
return Err(crate::Error::method_not_found());
167169
}

‎src/agent-client-protocol-core/src/session.rs‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -741,7 +741,14 @@ where
741741
);
742742
MatchDispatchFrom::new(message, &cx)
743743
.if_message_from(Agent, async |message| {
744-
if let Some(session_id) = message.get_session_id()? {
744+
let session_id = match message.get_session_id() {
745+
Ok(session_id) => session_id,
746+
Err(error) => {
747+
message.respond_with_error(error, cx.clone())?;
748+
return Ok(Handled::Yes);
749+
}
750+
};
751+
if let Some(session_id) = session_id {
745752
tracing::trace!(
746753
message_session_id = ?session_id,
747754
handler_session_id = ?self.session_id,

‎src/agent-client-protocol-core/src/typed.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
use jsonrpcmsg::Params;
33

44
use crate::{
5-
ConnectionTo, Responder, JsonRpcNotification, JsonRpcRequest, UntypedMessage,
5+
ConnectionTo, JsonRpcNotification, JsonRpcRequest, Responder, UntypedMessage,
66
util::json_cast,
77
};
88

‎src/agent-client-protocol-core/src/util.rs‎

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,24 +8,64 @@ use futures::{
88
mod typed;
99
pub use typed::{MatchDispatch, MatchDispatchFrom, TypeNotification};
1010

11-
/// Cast from `N` to `M` by serializing/deserialization to/from JSON.
11+
fn serde_conversion_error(
12+
kind: impl FnOnce() -> crate::Error,
13+
error: impl ToString,
14+
json: Option<serde_json::Value>,
15+
phase: &'static str,
16+
) -> crate::Error {
17+
let mut data = serde_json::json!({
18+
"error": error.to_string(),
19+
"phase": phase,
20+
});
21+
if let Some(json) = json {
22+
data["json"] = json;
23+
}
24+
kind().data(data)
25+
}
26+
27+
/// Cast between JSON and typed values for local/internal conversions.
28+
///
29+
/// This is appropriate for response decoding, outbound JSON-RPC conversion, and other
30+
/// framework-internal serde transformations where `InvalidParams` would be misleading.
1231
pub fn json_cast<N, M>(params: N) -> Result<M, crate::Error>
1332
where
1433
N: serde::Serialize,
1534
M: serde::de::DeserializeOwned,
1635
{
1736
let json = serde_json::to_value(params).map_err(|e| {
18-
crate::Error::parse_error().data(serde_json::json!({
19-
"error": e.to_string(),
20-
"phase": "serialization"
21-
}))
37+
serde_conversion_error(crate::Error::internal_error, e, None, "serialization")
38+
})?;
39+
let m = serde_json::from_value(json.clone()).map_err(|e| {
40+
serde_conversion_error(
41+
crate::Error::internal_error,
42+
e,
43+
Some(json),
44+
"deserialization",
45+
)
46+
})?;
47+
Ok(m)
48+
}
49+
50+
/// Cast incoming request/notification params into a typed payload.
51+
///
52+
/// Deserialization failures become `InvalidParams`, while serialization failures are
53+
/// treated as local/internal bugs.
54+
pub fn json_cast_params<N, M>(params: N) -> Result<M, crate::Error>
55+
where
56+
N: serde::Serialize,
57+
M: serde::de::DeserializeOwned,
58+
{
59+
let json = serde_json::to_value(params).map_err(|e| {
60+
serde_conversion_error(crate::Error::internal_error, e, None, "serialization")
2261
})?;
2362
let m = serde_json::from_value(json.clone()).map_err(|e| {
24-
crate::Error::parse_error().data(serde_json::json!({
25-
"error": e.to_string(),
26-
"json": json,
27-
"phase": "deserialization"
28-
}))
63+
serde_conversion_error(
64+
crate::Error::invalid_params,
65+
e,
66+
Some(json),
67+
"deserialization",
68+
)
2969
})?;
3070
Ok(m)
3171
}

‎src/agent-client-protocol-core/tests/jsonrpc_advanced.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ impl JsonRpcMessage for PingRequest {
7575
if !Self::matches_method(method) {
7676
return Err(agent_client_protocol_core::Error::method_not_found());
7777
}
78-
agent_client_protocol_core::util::json_cast(params)
78+
agent_client_protocol_core::util::json_cast_params(params)
7979
}
8080
}
8181

@@ -132,7 +132,7 @@ impl JsonRpcMessage for SlowRequest {
132132
if !Self::matches_method(method) {
133133
return Err(agent_client_protocol_core::Error::method_not_found());
134134
}
135-
agent_client_protocol_core::util::json_cast(params)
135+
agent_client_protocol_core::util::json_cast_params(params)
136136
}
137137
}
138138

‎src/agent-client-protocol-core/tests/jsonrpc_connection_builder.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ impl JsonRpcMessage for FooRequest {
6060
if !Self::matches_method(method) {
6161
return Err(agent_client_protocol_core::Error::method_not_found());
6262
}
63-
agent_client_protocol_core::util::json_cast(params)
63+
agent_client_protocol_core::util::json_cast_params(params)
6464
}
6565
}
6666

@@ -116,7 +116,7 @@ impl JsonRpcMessage for BarRequest {
116116
if !Self::matches_method(method) {
117117
return Err(agent_client_protocol_core::Error::method_not_found());
118118
}
119-
agent_client_protocol_core::util::json_cast(params)
119+
agent_client_protocol_core::util::json_cast_params(params)
120120
}
121121
}
122122

@@ -269,7 +269,7 @@ impl JsonRpcMessage for TrackRequest {
269269
if !Self::matches_method(method) {
270270
return Err(agent_client_protocol_core::Error::method_not_found());
271271
}
272-
agent_client_protocol_core::util::json_cast(params)
272+
agent_client_protocol_core::util::json_cast_params(params)
273273
}
274274
}
275275

@@ -397,7 +397,7 @@ impl JsonRpcMessage for Method1Request {
397397
if !Self::matches_method(method) {
398398
return Err(agent_client_protocol_core::Error::method_not_found());
399399
}
400-
agent_client_protocol_core::util::json_cast(params)
400+
agent_client_protocol_core::util::json_cast_params(params)
401401
}
402402
}
403403

@@ -432,7 +432,7 @@ impl JsonRpcMessage for Method2Request {
432432
if !Self::matches_method(method) {
433433
return Err(agent_client_protocol_core::Error::method_not_found());
434434
}
435-
agent_client_protocol_core::util::json_cast(params)
435+
agent_client_protocol_core::util::json_cast_params(params)
436436
}
437437
}
438438

@@ -626,7 +626,7 @@ impl JsonRpcMessage for EventNotification {
626626
if !Self::matches_method(method) {
627627
return Err(agent_client_protocol_core::Error::method_not_found());
628628
}
629-
agent_client_protocol_core::util::json_cast(params)
629+
agent_client_protocol_core::util::json_cast_params(params)
630630
}
631631
}
632632

0 commit comments

Comments
 (0)