From f8cce0fe3379dbf3ff41a63195e6a76ee3a6e8c4 Mon Sep 17 00:00:00 2001 From: sf-factory-bot Date: Sat, 10 Oct 2026 01:12:59 +0000 Subject: [PATCH 1/3] feat(generator): generate real tool handler bodies (#7) Replace the "not implemented" stub handler body with the real one: validate tool call arguments against the tool's input schema (compiled once per tool into a package-level `jsonschema::Validator`, next to the existing input-schema constant), decode them with the pbjson-generated `Deserialize`, call the tonic client, and encode the response with the pbjson-generated `Serialize`. A `None` arguments value is treated as `{}`. Schema validation failures and decode failures become tool errors without calling the backend; a `tonic::Status` becomes a tool error formatted like grpc-go's `Status.Error()` (`rpc error: code = NotFound desc = ...`). The generated server's `T` type parameter now carries the `tonic::client::GrpcService` bounds the tonic-generated client's own `unary` method needs, instead of just `Clone + Send + Sync + 'static`. Regenerate the committed example (`make generate`, protoc 29.3) and update the golden file to match. --- Cargo.lock | 57 ++++ examples/Cargo.toml | 11 + .../src/gen/examples/v1/examples.v1.mcp.rs | 277 +++++++++++++---- examples/tests/handler.rs | 274 ++++++++++++++++ protoc-gen-rust-mcp/Cargo.toml | 5 +- protoc-gen-rust-mcp/build.rs | 24 +- protoc-gen-rust-mcp/src/server.rs | 294 +++++++++++++++--- .../tests/server_integration.rs | 287 +++++++++++++++-- 8 files changed, 1109 insertions(+), 120 deletions(-) create mode 100644 examples/tests/handler.rs diff --git a/Cargo.lock b/Cargo.lock index 62b337b..f2bd66f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -69,6 +69,49 @@ version = "1.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f2032f911046de80f0a198e0901378627c33f59ea0ac00e363d481118bd70a53" +[[package]] +name = "axum" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "31b698c5f9a010f6573133b09e0de5408834d0c82f8d7475a89fc1867a71cd90" +dependencies = [ + "axum-core", + "bytes", + "futures-util", + "http", + "http-body", + "http-body-util", + "itoa", + "matchit", + "memchr", + "mime", + "percent-encoding", + "pin-project-lite", + "serde_core", + "sync_wrapper", + "tower", + "tower-layer", + "tower-service", +] + +[[package]] +name = "axum-core" +version = "0.5.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08c78f31d7b1291f7ee735c1c6780ccde7785daae9a9206026862dab7d8792d1" +dependencies = [ + "bytes", + "futures-core", + "http", + "http-body", + "http-body-util", + "mime", + "pin-project-lite", + "sync_wrapper", + "tower-layer", + "tower-service", +] + [[package]] name = "base64" version = "0.22.1" @@ -229,6 +272,7 @@ version = "0.1.0" dependencies = [ "async-trait", "futures", + "jsonschema", "pbjson", "pbjson-types", "prost", @@ -743,6 +787,12 @@ dependencies = [ "logos-codegen", ] +[[package]] +name = "matchit" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "47e1ffaa40ddd1f3ed91f717a33c8c0ee23fff369e3aa8772b9605cc1d22f4c3" + [[package]] name = "memchr" version = "2.8.3" @@ -777,6 +827,12 @@ dependencies = [ "syn 2.0.119", ] +[[package]] +name = "mime" +version = "0.3.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6877bb514081ee2a7ff5ef9de3281f14a4dd4bceac4c09388074a6b5df8a139a" + [[package]] name = "mio" version = "1.2.4" @@ -1547,6 +1603,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ac2a5518c70fa84342385732db33fb3f44bc4cc748936eb5833d2df34d6445ef" dependencies = [ "async-trait", + "axum", "base64", "bytes", "h2", diff --git a/examples/Cargo.toml b/examples/Cargo.toml index 19eee64..8ca27ee 100644 --- a/examples/Cargo.toml +++ b/examples/Cargo.toml @@ -27,6 +27,17 @@ serde_json = "1" async-trait = "0.1" rmcp = { version = "3.5", default-features = false, features = ["server"] } futures = "0.3" +# Validates tool call arguments against the generated input schema before +# decoding them (see protoc-gen-rust-mcp/src/server.rs's module doc +# comment): a runtime dependency of the generated handlers, not just this +# crate's tests, so it's a normal (not dev-)dependency. No HTTP/file +# `$ref` resolution is needed (every generated schema is self-contained), +# hence `default-features = false`. +jsonschema = { version = "0.58", default-features = false } [dev-dependencies] tokio = { version = "1", features = ["macros", "rt-multi-thread"] } +# "router" adds Server::builder()/add_service, to run a real in-process +# tonic server in tests/handler.rs ("server" and "channel" are already +# pulled in by [dependencies].tonic above). +tonic = { version = "0.14", default-features = false, features = ["router"] } diff --git a/examples/src/gen/examples/v1/examples.v1.mcp.rs b/examples/src/gen/examples/v1/examples.v1.mcp.rs index 27999bd..d97038f 100644 --- a/examples/src/gen/examples/v1/examples.v1.mcp.rs +++ b/examples/src/gen/examples/v1/examples.v1.mcp.rs @@ -14,6 +14,12 @@ static VIBE_SERVICE_SET_VIBE_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Ar ) }); +static VIBE_SERVICE_SET_VIBE_VALIDATOR: ::std::sync::LazyLock<::jsonschema::Validator> = + ::std::sync::LazyLock::new(|| { + ::jsonschema::validator_for(&::serde_json::Value::Object(VIBE_SERVICE_SET_VIBE_INPUT_SCHEMA.as_ref().clone())) + .unwrap_or_else(|e| panic!("compiling generated input schema: {e}")) + }); + static VIBE_SERVICE_GET_VIBE_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Arc<::rmcp::model::JsonObject>> = ::std::sync::LazyLock::new(|| { ::std::sync::Arc::new( @@ -25,6 +31,12 @@ static VIBE_SERVICE_GET_VIBE_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Ar ) }); +static VIBE_SERVICE_GET_VIBE_VALIDATOR: ::std::sync::LazyLock<::jsonschema::Validator> = + ::std::sync::LazyLock::new(|| { + ::jsonschema::validator_for(&::serde_json::Value::Object(VIBE_SERVICE_GET_VIBE_INPUT_SCHEMA.as_ref().clone())) + .unwrap_or_else(|e| panic!("compiling generated input schema: {e}")) + }); + static VIBE_SERVICE_SET_VIBE_DETAILS_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Arc<::rmcp::model::JsonObject>> = ::std::sync::LazyLock::new(|| { ::std::sync::Arc::new( @@ -36,6 +48,12 @@ static VIBE_SERVICE_SET_VIBE_DETAILS_INPUT_SCHEMA: ::std::sync::LazyLock<::std:: ) }); +static VIBE_SERVICE_SET_VIBE_DETAILS_VALIDATOR: ::std::sync::LazyLock<::jsonschema::Validator> = + ::std::sync::LazyLock::new(|| { + ::jsonschema::validator_for(&::serde_json::Value::Object(VIBE_SERVICE_SET_VIBE_DETAILS_INPUT_SCHEMA.as_ref().clone())) + .unwrap_or_else(|e| panic!("compiling generated input schema: {e}")) + }); + static VIBE_SERVICE_SET_VIBE_ARRAY_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Arc<::rmcp::model::JsonObject>> = ::std::sync::LazyLock::new(|| { ::std::sync::Arc::new( @@ -47,6 +65,12 @@ static VIBE_SERVICE_SET_VIBE_ARRAY_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sy ) }); +static VIBE_SERVICE_SET_VIBE_ARRAY_VALIDATOR: ::std::sync::LazyLock<::jsonschema::Validator> = + ::std::sync::LazyLock::new(|| { + ::jsonschema::validator_for(&::serde_json::Value::Object(VIBE_SERVICE_SET_VIBE_ARRAY_INPUT_SCHEMA.as_ref().clone())) + .unwrap_or_else(|e| panic!("compiling generated input schema: {e}")) + }); + static VIBE_SERVICE_SET_VIBE_OBJECTS_INPUT_SCHEMA: ::std::sync::LazyLock<::std::sync::Arc<::rmcp::model::JsonObject>> = ::std::sync::LazyLock::new(|| { ::std::sync::Arc::new( @@ -58,6 +82,12 @@ static VIBE_SERVICE_SET_VIBE_OBJECTS_INPUT_SCHEMA: ::std::sync::LazyLock<::std:: ) }); +static VIBE_SERVICE_SET_VIBE_OBJECTS_VALIDATOR: ::std::sync::LazyLock<::jsonschema::Validator> = + ::std::sync::LazyLock::new(|| { + ::jsonschema::validator_for(&::serde_json::Value::Object(VIBE_SERVICE_SET_VIBE_OBJECTS_INPUT_SCHEMA.as_ref().clone())) + .unwrap_or_else(|e| panic!("compiling generated input schema: {e}")) + }); + /// A registered tool's handler: takes the raw tool call arguments /// (already looked up by name), returns the result. Built once per /// registered tool from an `async fn(client, args) -> CallToolResult` @@ -86,7 +116,11 @@ pub struct VibeServiceMcpServer { impl VibeServiceMcpServer where - T: ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static, + T: ::tonic::client::GrpcService<::tonic::body::Body> + ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static, + T::Error: ::std::convert::Into<::tonic::codegen::StdError>, + T::ResponseBody: ::tonic::codegen::Body + ::std::marker::Send + 'static, + ::Error: ::std::convert::Into<::tonic::codegen::StdError> + ::std::marker::Send, + T::Future: ::std::marker::Send, { /// Creates a new `VibeServiceMcpServer` with no tools registered yet; call /// [`register_default_tools`](Self::register_default_tools) or @@ -111,20 +145,47 @@ where ::rmcp::model::Tool::new(r"SetVibe", r#"This is a block comment with multiple lines to test block handling "Hello World", a `backtick`, and a path like C:\vibes\new"#, VIBE_SERVICE_SET_VIBE_INPUT_SCHEMA.clone()) } - /// Stub handler for `SetVibe`: always returns a "not - /// implemented" tool error. A later generator issue (#7) replaces - /// this with argument validation, pbjson decoding, the `SetVibe` - /// tonic call, and a pbjson-encoded response. + /// Validates, decodes, calls the backend for, and encodes the + /// response of, a `SetVibe` tool call: see this module's doc + /// comment for the exact steps. async fn call_set_vibe( - _client: vibe_service_client::VibeServiceClient, - _args: ::std::option::Option<::rmcp::model::JsonObject>, + mut client: vibe_service_client::VibeServiceClient, + args: ::std::option::Option<::rmcp::model::JsonObject>, ) -> ::rmcp::model::CallToolResult { - ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( - "SetVibe is not implemented", - )]) + let args = ::serde_json::Value::Object(args.unwrap_or_default()); + let errors: ::std::vec::Vec<::std::string::String> = VIBE_SERVICE_SET_VIBE_VALIDATOR + .iter_errors(&args) + .map(|e| ::std::format!("{}: {e}", e.instance_path())) + .collect(); + if !errors.is_empty() { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("invalid arguments: {}", errors.join("; ")), + )]); + } + let request: SetVibeRequest = match ::serde_json::from_value(args) { + Ok(r) => r, + Err(e) => { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("decoding arguments: {e}"), + )]); + } + }; + match client.set_vibe(::tonic::Request::new(request)).await { + Ok(response) => match ::serde_json::to_string(response.get_ref()) { + Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]), + Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("encoding response: {e}"), + )]), + }, + // grpc-go's Status.Error() format, which Go's generated server + // returns verbatim as the tool error text. + Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("rpc error: code = {:?} desc = {}", status.code(), status.message()), + )]), + } } - /// Calls the stub handler for `SetVibe` with a clone of this + /// Calls the handler for `SetVibe` with a clone of this /// server's client. Exported so a caller can wrap it, e.g. an /// overriding tool (via [`register_tool`](Self::register_tool)) that /// pre-processes arguments and then delegates here. @@ -140,20 +201,47 @@ where ::rmcp::model::Tool::new(r"GetVibe", r"Get Vibe of the server", VIBE_SERVICE_GET_VIBE_INPUT_SCHEMA.clone()) } - /// Stub handler for `GetVibe`: always returns a "not - /// implemented" tool error. A later generator issue (#7) replaces - /// this with argument validation, pbjson decoding, the `GetVibe` - /// tonic call, and a pbjson-encoded response. + /// Validates, decodes, calls the backend for, and encodes the + /// response of, a `GetVibe` tool call: see this module's doc + /// comment for the exact steps. async fn call_get_vibe( - _client: vibe_service_client::VibeServiceClient, - _args: ::std::option::Option<::rmcp::model::JsonObject>, + mut client: vibe_service_client::VibeServiceClient, + args: ::std::option::Option<::rmcp::model::JsonObject>, ) -> ::rmcp::model::CallToolResult { - ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( - "GetVibe is not implemented", - )]) + let args = ::serde_json::Value::Object(args.unwrap_or_default()); + let errors: ::std::vec::Vec<::std::string::String> = VIBE_SERVICE_GET_VIBE_VALIDATOR + .iter_errors(&args) + .map(|e| ::std::format!("{}: {e}", e.instance_path())) + .collect(); + if !errors.is_empty() { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("invalid arguments: {}", errors.join("; ")), + )]); + } + let request: GetVibeRequest = match ::serde_json::from_value(args) { + Ok(r) => r, + Err(e) => { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("decoding arguments: {e}"), + )]); + } + }; + match client.get_vibe(::tonic::Request::new(request)).await { + Ok(response) => match ::serde_json::to_string(response.get_ref()) { + Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]), + Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("encoding response: {e}"), + )]), + }, + // grpc-go's Status.Error() format, which Go's generated server + // returns verbatim as the tool error text. + Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("rpc error: code = {:?} desc = {}", status.code(), status.message()), + )]), + } } - /// Calls the stub handler for `GetVibe` with a clone of this + /// Calls the handler for `GetVibe` with a clone of this /// server's client. Exported so a caller can wrap it, e.g. an /// overriding tool (via [`register_tool`](Self::register_tool)) that /// pre-processes arguments and then delegates here. @@ -169,20 +257,47 @@ where ::rmcp::model::Tool::new(r"SetVibeDetails", r"Set vibe details", VIBE_SERVICE_SET_VIBE_DETAILS_INPUT_SCHEMA.clone()) } - /// Stub handler for `SetVibeDetails`: always returns a "not - /// implemented" tool error. A later generator issue (#7) replaces - /// this with argument validation, pbjson decoding, the `SetVibeDetails` - /// tonic call, and a pbjson-encoded response. + /// Validates, decodes, calls the backend for, and encodes the + /// response of, a `SetVibeDetails` tool call: see this module's doc + /// comment for the exact steps. async fn call_set_vibe_details( - _client: vibe_service_client::VibeServiceClient, - _args: ::std::option::Option<::rmcp::model::JsonObject>, + mut client: vibe_service_client::VibeServiceClient, + args: ::std::option::Option<::rmcp::model::JsonObject>, ) -> ::rmcp::model::CallToolResult { - ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( - "SetVibeDetails is not implemented", - )]) + let args = ::serde_json::Value::Object(args.unwrap_or_default()); + let errors: ::std::vec::Vec<::std::string::String> = VIBE_SERVICE_SET_VIBE_DETAILS_VALIDATOR + .iter_errors(&args) + .map(|e| ::std::format!("{}: {e}", e.instance_path())) + .collect(); + if !errors.is_empty() { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("invalid arguments: {}", errors.join("; ")), + )]); + } + let request: SetVibeDetailsRequest = match ::serde_json::from_value(args) { + Ok(r) => r, + Err(e) => { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("decoding arguments: {e}"), + )]); + } + }; + match client.set_vibe_details(::tonic::Request::new(request)).await { + Ok(response) => match ::serde_json::to_string(response.get_ref()) { + Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]), + Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("encoding response: {e}"), + )]), + }, + // grpc-go's Status.Error() format, which Go's generated server + // returns verbatim as the tool error text. + Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("rpc error: code = {:?} desc = {}", status.code(), status.message()), + )]), + } } - /// Calls the stub handler for `SetVibeDetails` with a clone of this + /// Calls the handler for `SetVibeDetails` with a clone of this /// server's client. Exported so a caller can wrap it, e.g. an /// overriding tool (via [`register_tool`](Self::register_tool)) that /// pre-processes arguments and then delegates here. @@ -198,20 +313,47 @@ where ::rmcp::model::Tool::new(r"SetVibeArray", r"Set the vibe arrays", VIBE_SERVICE_SET_VIBE_ARRAY_INPUT_SCHEMA.clone()) } - /// Stub handler for `SetVibeArray`: always returns a "not - /// implemented" tool error. A later generator issue (#7) replaces - /// this with argument validation, pbjson decoding, the `SetVibeArray` - /// tonic call, and a pbjson-encoded response. + /// Validates, decodes, calls the backend for, and encodes the + /// response of, a `SetVibeArray` tool call: see this module's doc + /// comment for the exact steps. async fn call_set_vibe_array( - _client: vibe_service_client::VibeServiceClient, - _args: ::std::option::Option<::rmcp::model::JsonObject>, + mut client: vibe_service_client::VibeServiceClient, + args: ::std::option::Option<::rmcp::model::JsonObject>, ) -> ::rmcp::model::CallToolResult { - ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( - "SetVibeArray is not implemented", - )]) + let args = ::serde_json::Value::Object(args.unwrap_or_default()); + let errors: ::std::vec::Vec<::std::string::String> = VIBE_SERVICE_SET_VIBE_ARRAY_VALIDATOR + .iter_errors(&args) + .map(|e| ::std::format!("{}: {e}", e.instance_path())) + .collect(); + if !errors.is_empty() { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("invalid arguments: {}", errors.join("; ")), + )]); + } + let request: SetVibeArrayRequest = match ::serde_json::from_value(args) { + Ok(r) => r, + Err(e) => { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("decoding arguments: {e}"), + )]); + } + }; + match client.set_vibe_array(::tonic::Request::new(request)).await { + Ok(response) => match ::serde_json::to_string(response.get_ref()) { + Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]), + Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("encoding response: {e}"), + )]), + }, + // grpc-go's Status.Error() format, which Go's generated server + // returns verbatim as the tool error text. + Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("rpc error: code = {:?} desc = {}", status.code(), status.message()), + )]), + } } - /// Calls the stub handler for `SetVibeArray` with a clone of this + /// Calls the handler for `SetVibeArray` with a clone of this /// server's client. Exported so a caller can wrap it, e.g. an /// overriding tool (via [`register_tool`](Self::register_tool)) that /// pre-processes arguments and then delegates here. @@ -227,20 +369,47 @@ where ::rmcp::model::Tool::new(r"SetVibeObjects", r"Set multiple vibe objects", VIBE_SERVICE_SET_VIBE_OBJECTS_INPUT_SCHEMA.clone()) } - /// Stub handler for `SetVibeObjects`: always returns a "not - /// implemented" tool error. A later generator issue (#7) replaces - /// this with argument validation, pbjson decoding, the `SetVibeObjects` - /// tonic call, and a pbjson-encoded response. + /// Validates, decodes, calls the backend for, and encodes the + /// response of, a `SetVibeObjects` tool call: see this module's doc + /// comment for the exact steps. async fn call_set_vibe_objects( - _client: vibe_service_client::VibeServiceClient, - _args: ::std::option::Option<::rmcp::model::JsonObject>, + mut client: vibe_service_client::VibeServiceClient, + args: ::std::option::Option<::rmcp::model::JsonObject>, ) -> ::rmcp::model::CallToolResult { - ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( - "SetVibeObjects is not implemented", - )]) + let args = ::serde_json::Value::Object(args.unwrap_or_default()); + let errors: ::std::vec::Vec<::std::string::String> = VIBE_SERVICE_SET_VIBE_OBJECTS_VALIDATOR + .iter_errors(&args) + .map(|e| ::std::format!("{}: {e}", e.instance_path())) + .collect(); + if !errors.is_empty() { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("invalid arguments: {}", errors.join("; ")), + )]); + } + let request: SetVibeObjectsRequest = match ::serde_json::from_value(args) { + Ok(r) => r, + Err(e) => { + return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("decoding arguments: {e}"), + )]); + } + }; + match client.set_vibe_objects(::tonic::Request::new(request)).await { + Ok(response) => match ::serde_json::to_string(response.get_ref()) { + Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]), + Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("encoding response: {e}"), + )]), + }, + // grpc-go's Status.Error() format, which Go's generated server + // returns verbatim as the tool error text. + Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text( + ::std::format!("rpc error: code = {:?} desc = {}", status.code(), status.message()), + )]), + } } - /// Calls the stub handler for `SetVibeObjects` with a clone of this + /// Calls the handler for `SetVibeObjects` with a clone of this /// server's client. Exported so a caller can wrap it, e.g. an /// overriding tool (via [`register_tool`](Self::register_tool)) that /// pre-processes arguments and then delegates here. @@ -316,7 +485,11 @@ where impl ::rmcp::ServerHandler for VibeServiceMcpServer where - T: ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static, + T: ::tonic::client::GrpcService<::tonic::body::Body> + ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static, + T::Error: ::std::convert::Into<::tonic::codegen::StdError>, + T::ResponseBody: ::tonic::codegen::Body + ::std::marker::Send + 'static, + ::Error: ::std::convert::Into<::tonic::codegen::StdError> + ::std::marker::Send, + T::Future: ::std::marker::Send, { fn get_info(&self) -> ::rmcp::model::ServerConfig { ::rmcp::model::ServerConfig::new( diff --git a/examples/tests/handler.rs b/examples/tests/handler.rs new file mode 100644 index 0000000..d0c9590 --- /dev/null +++ b/examples/tests/handler.rs @@ -0,0 +1,274 @@ +//! End-to-end tests for the generated handler bodies (issue #7's +//! acceptance criteria), against the real example `VibeService` server +//! and a real in-process tonic backend (`FakeVibeService` below), not a +//! never-connecting client: every assertion here exercises the actual +//! validate -> decode -> call -> encode path `protoc-gen-rust-mcp` +//! generates into `examples.v1.mcp.rs`. + +use examples::v1::vibe_service_client::VibeServiceClient; +use examples::v1::vibe_service_server::{VibeService, VibeServiceServer}; +use examples::v1::*; +use rmcp::model::{CallToolRequestParams, JsonObject}; +use rmcp::{ServerHandler, ServiceExt}; +use tonic::transport::server::TcpIncoming; + +/// A `VibeService` backend: `SetVibe` with `vibe == "missing"` returns a +/// gRPC `NotFound`, exercising the handler's error-formatting path +/// against a real `tonic::Status`; every other RPC echoes enough of its +/// input to prove it actually reached the backend. +struct FakeVibeService; + +#[tonic::async_trait] +impl VibeService for FakeVibeService { + async fn set_vibe( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + let vibe = request.into_inner().vibe; + if vibe == "missing" { + return Err(tonic::Status::not_found("vibe not found")); + } + Ok(tonic::Response::new(SetVibeResponse { + previous_vibe: String::new(), + vibe, + })) + } + + async fn get_vibe( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(GetVibeResponse { + vibe: "chill".to_string(), + })) + } + + async fn set_vibe_details( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + let req = request.into_inner(); + Ok(tonic::Response::new(SetVibeResponse { + previous_vibe: String::new(), + vibe: req.vibe, + })) + } + + async fn set_vibe_array( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::unimplemented("not used by this test")) + } + + async fn set_vibe_objects( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::unimplemented("not used by this test")) + } +} + +/// Starts a real in-process tonic server for `VibeService` on an +/// OS-assigned ephemeral TCP port, and returns a connected +/// `VibeServiceClient`. +async fn serve_backend() -> VibeServiceClient { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral port"); + let addr = listener.local_addr().expect("local_addr"); + let incoming = TcpIncoming::from(listener); + tokio::spawn( + tonic::transport::Server::builder() + .add_service(VibeServiceServer::new(FakeVibeService)) + .serve_with_incoming(incoming), + ); + VibeServiceClient::connect(format!("http://{addr}")) + .await + .expect("connect to in-process server") +} + +/// A `VibeServiceClient` that never actually connects: for +/// tests that must prove the backend is never called (schema/decode +/// failures), where a real connection would mask a bug by letting an +/// unexpectedly-forwarded call fail for an unrelated reason (and where a +/// real call would just hang/error against nothing listening). +fn lazy_client() -> VibeServiceClient { + let channel = tonic::transport::Endpoint::from_static("http://127.0.0.1:1").connect_lazy(); + VibeServiceClient::new(channel) +} + +/// Serves `handler` over an in-memory duplex transport and returns a +/// connected rmcp client (same approach as +/// `protoc-gen-rust-mcp/tests/server_integration.rs`). +async fn serve(handler: H) -> rmcp::service::RunningService +where + H: ServerHandler + Send + Sync + 'static, +{ + let (server_io, client_io) = tokio::io::duplex(64 * 1024); + tokio::spawn(async move { + let running = handler.serve(server_io).await.expect("serve"); + let _ = running.waiting().await; + }); + ().serve(client_io).await.expect("client connect") +} + +fn text_of(result: &rmcp::model::CallToolResult) -> &str { + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + &text.text +} + +/// Success: the result JSON matches protojson conventions (lowerCamelCase +/// field names, zero-valued fields omitted). +#[tokio::test] +async fn success_result_matches_protojson_conventions() { + let mut server = VibeServiceMcpServer::new(serve_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("chill")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(false)); + // previousVibe is zero-valued ("") and omitted; vibe is present. + assert_eq!(text_of(&result), r#"{"vibe":"chill"}"#); +} + +/// A gRPC `NotFound` becomes `isError: true` with grpc-go's +/// `"rpc error: code = NotFound desc = ..."` text. +#[tokio::test] +async fn grpc_not_found_becomes_tool_error_in_grpc_go_format() { + let mut server = VibeServiceMcpServer::new(serve_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("missing")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + assert_eq!( + text_of(&result), + "rpc error: code = NotFound desc = vibe not found" + ); +} + +/// A wrong argument type fails schema validation before decoding; the +/// backend is never called (proven by using `lazy_client`, which would +/// hang/error on a real call attempt rather than returning this specific +/// validation message). +#[tokio::test] +async fn wrong_argument_type_is_rejected_without_calling_backend() { + let mut server = VibeServiceMcpServer::new(lazy_client()); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!(5)); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + assert_eq!( + text_of(&result), + r#"invalid arguments: /vibe: 5 is not of type "string""# + ); +} + +/// An unknown field is rejected by `additionalProperties: false` before +/// decoding, including the snake_case form of a field that exists only +/// on the *response* message (`SetVibeResponse.previous_vibe`), proving +/// this isn't accidentally accepted via the wrong message's schema. +#[tokio::test] +async fn unknown_field_including_snake_case_is_rejected_without_calling_backend() { + let mut server = VibeServiceMcpServer::new(lazy_client()); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("previous_vibe".to_string(), serde_json::json!("x")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + assert_eq!( + text_of(&result), + r#"invalid arguments: : Additional properties are not allowed ('previous_vibe' was unexpected)"# + ); +} + +/// An int64 field is accepted both as a JSON number and as a decimal +/// string (pbjson's generated `Deserialize` accepts both for a 64-bit +/// field; the schema's `type: ["integer", "string"]` reflects that), and +/// an enum is accepted by its name. `SetVibeDetails` echoes the vibe +/// string back through `SetVibeResponse`, so a successful, non-error +/// result proves the whole message (including vibeScalar) decoded. +#[tokio::test] +async fn int64_accepted_as_number_or_string_and_enum_by_name() { + let mut server = VibeServiceMcpServer::new(serve_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + for vibe_int64 in [serde_json::json!(42), serde_json::json!("42")] { + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("details")); + args.insert( + "vibeScalar".to_string(), + serde_json::json!({ + "vibeInt64": vibe_int64, + "vibeEnum": ["VIBE_GOOD"], + }), + ); + let result = client + .call_tool(CallToolRequestParams::new("SetVibeDetails").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!( + result.is_error, + Some(false), + "vibeInt64 = {vibe_int64:?} should validate and decode: {}", + text_of(&result) + ); + assert_eq!(text_of(&result), r#"{"vibe":"details"}"#); + } +} + +/// `NaN`/`Infinity`/`-Infinity` are accepted as float/double *input* +/// (both the schema and pbjson's generated deserializer special-case +/// these three strings; see `schema.rs`'s `float_schema`), going through +/// the real handler's validate-then-decode path rather than the +/// schema/deserializer in isolation. +#[tokio::test] +async fn non_finite_floats_are_accepted_as_input() { + let mut server = VibeServiceMcpServer::new(serve_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + for s in ["NaN", "Infinity", "-Infinity"] { + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("details")); + args.insert( + "vibeScalar".to_string(), + serde_json::json!({"vibeDouble": s, "vibeFloat": s}), + ); + let result = client + .call_tool(CallToolRequestParams::new("SetVibeDetails").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!( + result.is_error, + Some(false), + "{s:?} should validate and decode: {}", + text_of(&result) + ); + } +} diff --git a/protoc-gen-rust-mcp/Cargo.toml b/protoc-gen-rust-mcp/Cargo.toml index 99f4cf7..cb537c1 100644 --- a/protoc-gen-rust-mcp/Cargo.toml +++ b/protoc-gen-rust-mcp/Cargo.toml @@ -47,7 +47,10 @@ version = "0.3" [dev-dependencies.tonic] version = "0.14" default-features = false -features = ["codegen", "channel"] +# "server" + "router" run a real in-process tonic server in +# tests/server_integration.rs, so the generated handler body (#7) is +# exercised against a real backend, not just a never-connecting client. +features = ["codegen", "channel", "server", "router"] [dev-dependencies.tonic-prost] version = "0.14" diff --git a/protoc-gen-rust-mcp/build.rs b/protoc-gen-rust-mcp/build.rs index 6159f02..775bc97 100644 --- a/protoc-gen-rust-mcp/build.rs +++ b/protoc-gen-rust-mcp/build.rs @@ -72,13 +72,15 @@ fn main() -> Result<(), Box> { } /// Compiles `tests/testdata/mcpgen/fixture.proto` with real `tonic-prost- -/// build` (so `tests/server_integration.rs` exercises this generator's -/// output against a real tonic-generated client, not a hand-written -/// stand-in), then runs `server::generate_service` (this crate's own -/// generator, `#[path]`-included above) against the same descriptors and -/// writes the result as `mcpgen.mcp.rs` into `OUT_DIR`, so the test can -/// `include!` all three files the way `examples/src/gen/` does for the -/// real example. +/// build` and `pbjson-build` (so `tests/server_integration.rs` exercises +/// this generator's output against a real tonic-generated client and +/// pbjson-generated `Serialize`/`Deserialize` impls, not a hand-written +/// stand-in — the generated handler body, #7, decodes/encodes arguments +/// with those impls), then runs `server::generate_service` (this crate's +/// own generator, `#[path]`-included above) against the same descriptors +/// and writes the result as `mcpgen.mcp.rs` into `OUT_DIR`, so the test +/// can `include!` all three files the way `examples/src/gen/` does for +/// the real example. fn generate_mcpgen_fixture() -> Result<(), Box> { let proto_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/testdata/mcpgen"); println!("cargo:rerun-if-changed={}", proto_dir.display()); @@ -87,6 +89,14 @@ fn generate_mcpgen_fixture() -> Result<(), Box> { tonic_prost_build::configure().compile_fds(fds.clone())?; + let descriptor_bytes = { + use protox::prost::Message; + fds.encode_to_vec() + }; + pbjson_build::Builder::new() + .register_descriptors(&descriptor_bytes)? + .build(&[".mcpgen"])?; + use prost_reflect::DescriptorPool; let pool = DescriptorPool::from_file_descriptor_set(fds)?; diff --git a/protoc-gen-rust-mcp/src/server.rs b/protoc-gen-rust-mcp/src/server.rs index 691105b..2e19b04 100644 --- a/protoc-gen-rust-mcp/src/server.rs +++ b/protoc-gen-rust-mcp/src/server.rs @@ -1,13 +1,55 @@ //! Generates, for one gRPC service, the MCP server struct and its tool -//! definitions and registration API: the counterpart of Go's +//! definitions, handlers and registration API: the counterpart of Go's //! `generateMcpServerStruct`, `generateMcpServerService`, `generateMCPTool`, -//! `generateToolRegistration` and `generateDefaultToolsRegistration` -//! (`cmd/protoc-gen-go-mcp/mcp.go` at commit `9e072f9`). Handler *bodies* -//! are #7; `method_handler_stub` below only emits a "not implemented" tool -//! error, matching this issue's scope. `method_public_handler_fn` emits -//! the public `pub async fn _handler(&self, args)` wrapper Go also -//! exports, so a caller can delegate to the default behavior from an -//! overriding tool. +//! `generateHandler`, `generateToolRegistration` and +//! `generateDefaultToolsRegistration` (`cmd/protoc-gen-go-mcp/mcp.go` at +//! commit `9e072f9`). `method_handler_fn` (this issue, #7) emits the real +//! handler body: validate the raw arguments against the tool's input +//! schema (the validation Go's `mcp.AddTool` does, which `rmcp` itself +//! does not), decode them with the pbjson-generated `Deserialize`, call +//! the tonic client, and encode the response with the pbjson-generated +//! `Serialize`. `method_public_handler_fn` emits the public +//! `pub async fn _handler(&self, args)` wrapper Go also exports, +//! so a caller can delegate to the default behavior from an overriding +//! tool. +//! +//! # Argument validation, decoding, the tonic call, and the response +//! +//! - **Validation** happens first, against a `jsonschema::Validator` +//! compiled once per tool from the same schema embedded in the tool's +//! `inputSchema` (a package-level `static ..._VALIDATOR: LazyLock`, next +//! to the existing `..._INPUT_SCHEMA` constant). `None` arguments are +//! treated as `{}` (an empty object), matching Go's SDK (`applySchema` +//! unmarshals missing/`null` arguments into an empty map before +//! validating), since every input schema this generator emits has +//! `"type": "object"` and no required top-level properties (RPCs take +//! exactly one message argument, never scalars). On failure, every +//! failing instance path and reason is listed in the tool error text, +//! and the backend is never called. +//! - **Decoding** uses `serde_json::from_value::`, the pbjson- +//! generated counterpart of Go's `protojson.Unmarshal`. A decode error +//! (schema validation already ruled out most of these, but pbjson's +//! `Deserialize` is independent code with its own opinions, e.g. an +//! out-of-range number for a sized integer type) becomes a tool error +//! without calling the backend. +//! - **The tonic call** clones the handler's own client (already captured +//! per-registration, not per-call; see the module doc comment below) and +//! calls it with `tonic::Request::new(request)`. A `tonic::Status` +//! becomes a tool error formatted like grpc-go's `Status.Error()`: +//! `"rpc error: code = {code:?} desc = {message}"` — `tonic::Code`'s +//! `Debug` impl renders exactly grpc-go's CamelCase names (`NotFound`, +//! `InvalidArgument`, ...), confirmed against `tonic::Code` 0.14.6's +//! definition, so no separate name table is needed here. +//! - **Encoding** uses `serde_json::to_string`, the pbjson-generated +//! counterpart of Go's `protojson.Marshal`: zero-valued fields are +//! omitted, 64-bit integers are JSON strings, and enums are field +//! names, matching protojson's own conventions on every point except +//! one documented divergence: pbjson serializes a non-finite +//! float/double as JSON `null`, where protojson emits the string +//! `"NaN"`/`"Infinity"`/`"-Infinity"`; this only affects tool *results* +//! (`float_schema` in `schema.rs` already documents that inputs accept +//! all three spellings either way). Reported in this issue's PR per +//! review feedback on #18. //! //! # Shape //! @@ -43,8 +85,8 @@ use std::fmt::Write as _; -use heck::{ToShoutySnakeCase, ToSnakeCase}; -use prost_reflect::{MethodDescriptor, ServiceDescriptor}; +use heck::{ToShoutySnakeCase, ToSnakeCase, ToUpperCamelCase}; +use prost_reflect::{MessageDescriptor, MethodDescriptor, ServiceDescriptor}; use serde_json::Value; use crate::comments::{camel_to_space, process_comment_to_string}; @@ -52,10 +94,26 @@ use crate::rust_literal::rust_string_literal; use crate::schema::message_input_schema; use crate::source_info::leading_comments; +/// The bounds this generator puts on a generated server's `T` type +/// parameter: exactly what `VibeServiceClient::unary` (tonic-generated, +/// see e.g. `examples.v1.tonic.rs`) needs to be callable, plus `Clone + +/// Send + Sync + 'static` so the generated struct itself is `Send + Sync` +/// (required by `rmcp::ServerHandler`) and so a handler can clone the +/// client into a `'static` future. Verified against rmcp 3.5.1 and tonic +/// 0.14.6 by compiling and running the generated code's handler against a +/// real tonic server (see this issue's discussion). +const CLIENT_BOUNDS: &str = "\ + T: ::tonic::client::GrpcService<::tonic::body::Body> + ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static,\n\ + \u{20} T::Error: ::std::convert::Into<::tonic::codegen::StdError>,\n\ + \u{20} T::ResponseBody: ::tonic::codegen::Body + ::std::marker::Send + 'static,\n\ + \u{20} ::Error: ::std::convert::Into<::tonic::codegen::StdError> + ::std::marker::Send,\n\ + \u{20} T::Future: ::std::marker::Send,\n\ +"; + /// Renders the full generated module for one service: the input schema -/// constants, the tool handler type alias, the server struct, its `impl` -/// block (constructor, one `_tool()` + stub handler + public -/// `_handler()` wrapper per unary RPC, `register_tool`, +/// and validator constants, the tool handler type alias, the server +/// struct, its `impl` block (constructor, one `_tool()`, handler +/// and public `_handler()` wrapper per unary RPC, `register_tool`, /// `register_default_tools`, `into_tools`/`extend_tools`), and its /// `rmcp::ServerHandler` impl. Streaming RPCs are skipped, with a comment /// noting each one skipped (per the issue). @@ -92,6 +150,8 @@ pub fn generate_service(service: &ServiceDescriptor) -> String { for method in &unary_methods { out.push_str(&schema_constant(service_name, method)); out.push('\n'); + out.push_str(&validator_constant(service_name, method)); + out.push('\n'); } let _ = write!( @@ -130,7 +190,7 @@ pub fn generate_service(service: &ServiceDescriptor) -> String { out, "impl {server_name}\n\ where\n\ - \u{20} T: ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static,\n\ + \u{20} {CLIENT_BOUNDS}\ {{\n" ); let _ = write!( @@ -158,7 +218,7 @@ pub fn generate_service(service: &ServiceDescriptor) -> String { for method in &unary_methods { out.push_str(&method_tool_fn(service_name, method)); out.push('\n'); - out.push_str(&method_handler_stub(&client_path, method)); + out.push_str(&method_handler_fn(service_name, &client_path, method)); out.push('\n'); out.push_str(&method_public_handler_fn(method)); out.push('\n'); @@ -230,7 +290,7 @@ pub fn generate_service(service: &ServiceDescriptor) -> String { out, "impl ::rmcp::ServerHandler for {server_name}\n\ where\n\ - \u{20} T: ::std::clone::Clone + ::std::marker::Send + ::std::marker::Sync + 'static,\n\ + \u{20} {CLIENT_BOUNDS}\ {{\n\ \u{20} fn get_info(&self) -> ::rmcp::model::ServerConfig {{\n\ \u{20} ::rmcp::model::ServerConfig::new(\n\ @@ -337,29 +397,100 @@ fn method_description(method: &MethodDescriptor) -> String { } } -/// The stub handler for one method: a private async associated function -/// that takes an owned client and always returns a "not implemented" tool -/// error. A later generator issue (#7) replaces this with argument -/// validation, pbjson decoding, the tonic call, and a pbjson-encoded -/// response. Named `call_` (not `_handler`, which is the -/// public wrapper in [`method_public_handler_fn`]) because -/// `register_default_tools` builds its closures from this one directly, -/// cloning `self.client` once per registration rather than per call. -fn method_handler_stub(client_path: &str, method: &MethodDescriptor) -> String { +/// The name of the package-level `static ..._VALIDATOR: LazyLock` +/// constant for one method, named like [`schema_const_name`] but with a +/// `_VALIDATOR` suffix instead of `_INPUT_SCHEMA`, so the two don't +/// collide. +fn validator_const_name(service_name: &str, method: &MethodDescriptor) -> String { + format!( + "{}_{}_VALIDATOR", + service_name.to_shouty_snake_case(), + method.name().to_shouty_snake_case() + ) +} + +/// The `static __VALIDATOR: LazyLock` +/// declaration for one method, compiled once from the method's own +/// `..._INPUT_SCHEMA` constant: the validator `method_handler_fn`'s body +/// checks a tool call's raw arguments against before decoding them, the +/// validation Go's `mcp.AddTool` does and `rmcp` itself does not (this +/// issue). +fn validator_constant(service_name: &str, method: &MethodDescriptor) -> String { + let schema_const = schema_const_name(service_name, method); + let validator_const = validator_const_name(service_name, method); + format!( + "static {validator_const}: ::std::sync::LazyLock<::jsonschema::Validator> =\n \ + ::std::sync::LazyLock::new(|| {{\n \ + ::jsonschema::validator_for(&::serde_json::Value::Object({schema_const}.as_ref().clone()))\n \ + .unwrap_or_else(|e| panic!(\"compiling generated input schema: {{e}}\"))\n \ + }});\n" + ) +} + +/// The real handler for one method (this issue, #7): a private async +/// associated function that takes an owned client and the raw tool call +/// arguments (`None` treated as `{{}}`), and: +/// +/// 1. Validates them against the method's `..._VALIDATOR`, returning a +/// tool error listing every failing instance path and reason (backend +/// not called) on failure. +/// 2. Decodes them into the request message with +/// `serde_json::from_value` (the pbjson-generated `Deserialize`), +/// returning a tool error on failure (backend not called). +/// 3. Calls the tonic client, returning a tool error formatted like +/// grpc-go's `Status.Error()` on a `tonic::Status`. +/// 4. Encodes the response with `serde_json::to_string` (the +/// pbjson-generated `Serialize`) and returns it as a successful tool +/// result. +/// +/// Named `call_` (not `_handler`, which is the public +/// wrapper in [`method_public_handler_fn`]) because `register_default_tools` +/// builds its closures from this one directly, cloning `self.client` once +/// per registration rather than once per call. +fn method_handler_fn(service_name: &str, client_path: &str, method: &MethodDescriptor) -> String { let method_name = method.name(); let fn_name = method_fn_name(method); + let validator_const = validator_const_name(service_name, method); + let request_path = rust_message_path(&method.input()); format!( - " /// Stub handler for `{method_name}`: always returns a \"not\n\ - \u{20} /// implemented\" tool error. A later generator issue (#7) replaces\n\ - \u{20} /// this with argument validation, pbjson decoding, the `{method_name}`\n\ - \u{20} /// tonic call, and a pbjson-encoded response.\n\ + " /// Validates, decodes, calls the backend for, and encodes the\n\ + \u{20} /// response of, a `{method_name}` tool call: see this module's doc\n\ + \u{20} /// comment for the exact steps.\n\ \u{20} async fn call_{fn_name}(\n\ - \u{20} _client: {client_path},\n\ - \u{20} _args: ::std::option::Option<::rmcp::model::JsonObject>,\n\ + \u{20} mut client: {client_path},\n\ + \u{20} args: ::std::option::Option<::rmcp::model::JsonObject>,\n\ \u{20} ) -> ::rmcp::model::CallToolResult {{\n\ - \u{20} ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text(\n\ - \u{20} \"{method_name} is not implemented\",\n\ - \u{20} )])\n\ + \u{20} let args = ::serde_json::Value::Object(args.unwrap_or_default());\n\ + \u{20} let errors: ::std::vec::Vec<::std::string::String> = {validator_const}\n\ + \u{20} .iter_errors(&args)\n\ + \u{20} .map(|e| ::std::format!(\"{{}}: {{e}}\", e.instance_path()))\n\ + \u{20} .collect();\n\ + \u{20} if !errors.is_empty() {{\n\ + \u{20} return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text(\n\ + \u{20} ::std::format!(\"invalid arguments: {{}}\", errors.join(\"; \")),\n\ + \u{20} )]);\n\ + \u{20} }}\n\ + \u{20} let request: {request_path} = match ::serde_json::from_value(args) {{\n\ + \u{20} Ok(r) => r,\n\ + \u{20} Err(e) => {{\n\ + \u{20} return ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text(\n\ + \u{20} ::std::format!(\"decoding arguments: {{e}}\"),\n\ + \u{20} )]);\n\ + \u{20} }}\n\ + \u{20} }};\n\ + \u{20} match client.{fn_name}(::tonic::Request::new(request)).await {{\n\ + \u{20} Ok(response) => match ::serde_json::to_string(response.get_ref()) {{\n\ + \u{20} Ok(json) => ::rmcp::model::CallToolResult::success(vec![::rmcp::model::ContentBlock::text(json)]),\n\ + \u{20} Err(e) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text(\n\ + \u{20} ::std::format!(\"encoding response: {{e}}\"),\n\ + \u{20} )]),\n\ + \u{20} }},\n\ + \u{20} // grpc-go's Status.Error() format, which Go's generated server\n\ + \u{20} // returns verbatim as the tool error text.\n\ + \u{20} Err(status) => ::rmcp::model::CallToolResult::error(vec![::rmcp::model::ContentBlock::text(\n\ + \u{20} ::std::format!(\"rpc error: code = {{:?}} desc = {{}}\", status.code(), status.message()),\n\ + \u{20} )]),\n\ + \u{20} }}\n\ \u{20} }}\n" ) } @@ -369,13 +500,13 @@ fn method_handler_stub(client_path: &str, method: &MethodDescriptor) -> String { /// methods) so a caller can wrap a default handler, e.g. register an /// overriding tool that pre-processes arguments and then delegates to this /// one, which `register_default_tools` itself does not need (it calls the -/// private [`method_handler_stub`] directly with a clone of `self.client` +/// private [`method_handler_fn`] directly with a clone of `self.client` /// made once per registration, not once per call). fn method_public_handler_fn(method: &MethodDescriptor) -> String { let method_name = method.name(); let fn_name = method_fn_name(method); format!( - " /// Calls the stub handler for `{method_name}` with a clone of this\n\ + " /// Calls the handler for `{method_name}` with a clone of this\n\ \u{20} /// server's client. Exported so a caller can wrap it, e.g. an\n\ \u{20} /// overriding tool (via [`register_tool`](Self::register_tool)) that\n\ \u{20} /// pre-processes arguments and then delegates here.\n\ @@ -409,3 +540,94 @@ fn method_fn_name(method: &MethodDescriptor) -> String { pub fn client_mod_name(service: &ServiceDescriptor) -> String { format!("{}_client", service.name().to_snake_case()) } + +/// Returns the Rust path `prost-build` generates for `message`, relative to +/// the scope this generator's own output runs in (the package's module: +/// the generated `.mcp.rs` is `include!`d directly into prost's own +/// `.rs`, the same scope every top-level message in the package +/// lives in — see `generator.rs`'s module doc comment). A top-level +/// message's path is just its own `UpperCamelCase` name (e.g. +/// `SetVibeRequest`); a message nested inside another one picks up a +/// `snake_case` module segment per enclosing message (e.g. +/// `vibe_scalar::VibeEnum`'s sibling `VibeScalar::SomeNested` would be +/// `vibe_scalar::SomeNested`), matching prost-build's own nested-type +/// module naming (confirmed against `examples.v1.rs`'s committed +/// `vibe_scalar` module) and [`heck::ToUpperCamelCase`]/[`heck::ToSnakeCase`] +/// for the case conversions themselves (the same crate `prost-build` 0.14's +/// `ident::to_upper_camel`/`to_snake` are built on). RPC request/response +/// messages are never map entries (protoc rejects a map field as an RPC's +/// type), so that prost-build special case does not apply here. +fn rust_message_path(message: &MessageDescriptor) -> String { + let mut segments = Vec::new(); + segments.push(message.name().to_upper_camel_case()); + let mut parent = message.parent_message(); + while let Some(m) = parent { + segments.push(m.name().to_snake_case()); + parent = m.parent_message(); + } + segments.reverse(); + segments.join("::") +} + +#[cfg(test)] +mod tests { + use super::*; + use prost_reflect::DescriptorPool; + use prost_types::field_descriptor_proto::{Label, Type}; + use prost_types::{DescriptorProto, FieldDescriptorProto, FileDescriptorProto}; + + /// A `FileDescriptorProto` whose top-level `Outer` message has a + /// nested `Outer.Inner` message, for [`rust_message_path`]'s nested- + /// message case (not exercised by `example.proto` or + /// `tests/testdata/mcpgen/fixture.proto`, whose RPCs only use + /// top-level messages). + fn pool_with_nested_message() -> DescriptorPool { + let field = |name: &str, number: i32| FieldDescriptorProto { + name: Some(name.to_string()), + number: Some(number), + label: Some(Label::Optional as i32), + r#type: Some(Type::String as i32), + ..Default::default() + }; + let inner = DescriptorProto { + name: Some("Inner".to_string()), + field: vec![field("value", 1)], + ..Default::default() + }; + let outer = DescriptorProto { + name: Some("Outer".to_string()), + field: vec![field("value", 1)], + nested_type: vec![inner], + ..Default::default() + }; + let file = FileDescriptorProto { + name: Some("nested.proto".to_string()), + package: Some("nestedpb".to_string()), + message_type: vec![outer], + syntax: Some("proto3".to_string()), + ..Default::default() + }; + DescriptorPool::from_file_descriptor_set(prost_types::FileDescriptorSet { + file: vec![file], + }) + .expect("building descriptor pool") + } + + #[test] + fn rust_message_path_for_top_level_message() { + let pool = pool_with_nested_message(); + let outer = pool + .get_message_by_name("nestedpb.Outer") + .expect("Outer message"); + assert_eq!(rust_message_path(&outer), "Outer"); + } + + #[test] + fn rust_message_path_for_nested_message() { + let pool = pool_with_nested_message(); + let inner = pool + .get_message_by_name("nestedpb.Outer.Inner") + .expect("Outer.Inner message"); + assert_eq!(rust_message_path(&inner), "outer::Inner"); + } +} diff --git a/protoc-gen-rust-mcp/tests/server_integration.rs b/protoc-gen-rust-mcp/tests/server_integration.rs index ec6c26e..457f131 100644 --- a/protoc-gen-rust-mcp/tests/server_integration.rs +++ b/protoc-gen-rust-mcp/tests/server_integration.rs @@ -21,26 +21,145 @@ )] mod mcpgen { include!(concat!(env!("OUT_DIR"), "/mcpgen.rs")); + include!(concat!(env!("OUT_DIR"), "/mcpgen.serde.rs")); include!(concat!(env!("OUT_DIR"), "/mcpgen.mcp.rs")); } use mcpgen::other_fixture_service_client::OtherFixtureServiceClient; +use mcpgen::other_fixture_service_server::{OtherFixtureService, OtherFixtureServiceServer}; use mcpgen::vibe_fixture_service_client::VibeFixtureServiceClient; +use mcpgen::vibe_fixture_service_server::{VibeFixtureService, VibeFixtureServiceServer}; use rmcp::model::{CallToolRequestParams, JsonObject}; use rmcp::{ServerHandler, ServiceExt}; use tonic::transport::Endpoint; +use tonic::transport::server::TcpIncoming; -/// A `VibeFixtureServiceClient`/`OtherFixtureServiceClient` -/// that never actually connects (`connect_lazy`): every tool call in this -/// test is either a stub (handler bodies are #7) or an override that -/// never touches the client, so a real gRPC server is unnecessary. +/// A `VibeFixtureServiceClient` that never actually connects +/// (`connect_lazy`): for tests that only inspect registered tool +/// metadata, call an overriding tool that never touches the client, or +/// must prove the backend is never called (schema/decode failures), +/// where a real connection would mask a bug by letting an +/// unexpectedly-forwarded call fail for an unrelated reason. fn lazy_vibe_client() -> VibeFixtureServiceClient { let channel = Endpoint::from_static("http://127.0.0.1:1").connect_lazy(); VibeFixtureServiceClient::new(channel) } -fn lazy_other_client() -> OtherFixtureServiceClient { - let channel = Endpoint::from_static("http://127.0.0.1:1").connect_lazy(); +/// A `VibeFixtureService` backend for tests that exercise the real +/// handler body (this issue, #7) end to end: `SetVibe` with +/// `vibe == "missing"` returns a gRPC `NotFound`, so the error-formatting +/// path (`rpc error: code = NotFound desc = ...`) gets exercised against +/// a real `tonic::Status`, not a constructed one; every other RPC echoes +/// its input back. +struct FakeVibeBackend; + +#[tonic::async_trait] +impl VibeFixtureService for FakeVibeBackend { + async fn set_vibe( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + let vibe = request.into_inner().vibe; + if vibe == "missing" { + return Err(tonic::Status::not_found("vibe not found")); + } + Ok(tonic::Response::new(mcpgen::SetVibeResponse { vibe })) + } + + async fn get_vibe( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(mcpgen::GetVibeResponse { + vibe: "chill".to_string(), + })) + } + + type StreamVibeStream = tonic::codegen::tokio_stream::Once< + std::result::Result, + >; + + async fn stream_vibe( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::unimplemented( + "not used by this test; VibeFixtureService.StreamVibe produces no tool", + )) + } +} + +/// An `OtherFixtureService` backend: `DoOther` echoes its input, used by +/// the cross-service composition test. +struct FakeOtherBackend; + +#[tonic::async_trait] +impl OtherFixtureService for FakeOtherBackend { + async fn do_other( + &self, + request: tonic::Request, + ) -> Result, tonic::Status> { + Ok(tonic::Response::new(mcpgen::OtherResponse { + value: request.into_inner().value, + })) + } + + async fn get_vibe( + &self, + _request: tonic::Request, + ) -> Result, tonic::Status> { + Err(tonic::Status::unimplemented("not used by this test")) + } +} + +/// Starts a real in-process tonic server for `VibeFixtureService` (backed +/// by [`FakeVibeBackend`]) on an OS-assigned ephemeral TCP port, and +/// returns a `VibeFixtureServiceClient` connected to it: so a test can +/// exercise the generated handler body's tonic call and response/error +/// handling against a real backend, not a never-connecting client. +async fn serve_vibe_backend() -> VibeFixtureServiceClient { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral port"); + let addr = listener.local_addr().expect("local_addr"); + let incoming = TcpIncoming::from(listener); + tokio::spawn( + tonic::transport::Server::builder() + .add_service(VibeFixtureServiceServer::new(FakeVibeBackend)) + .serve_with_incoming(incoming), + ); + // Not `VibeFixtureServiceClient::connect`: `tonic-prost-build`'s + // generated `connect()` needs its own "transport" feature (distinct + // from `tonic`'s own), which this crate's dev-dependency on + // `tonic-prost-build` doesn't enable; building the `Channel` directly + // avoids a dependency this test doesn't otherwise need. + let channel = Endpoint::from_shared(format!("http://{addr}")) + .expect("valid endpoint") + .connect() + .await + .expect("connect to in-process server"); + VibeFixtureServiceClient::new(channel) +} + +/// Starts a real in-process tonic server for `OtherFixtureService` +/// (backed by [`FakeOtherBackend`]), the counterpart of +/// [`serve_vibe_backend`] for the cross-service composition test. +async fn serve_other_backend() -> OtherFixtureServiceClient { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind ephemeral port"); + let addr = listener.local_addr().expect("local_addr"); + let incoming = TcpIncoming::from(listener); + tokio::spawn( + tonic::transport::Server::builder() + .add_service(OtherFixtureServiceServer::new(FakeOtherBackend)) + .serve_with_incoming(incoming), + ); + let channel = Endpoint::from_shared(format!("http://{addr}")) + .expect("valid endpoint") + .connect() + .await + .expect("connect to in-process server"); OtherFixtureServiceClient::new(channel) } @@ -139,13 +258,13 @@ async fn register_tool_overrides_a_default_tool() { /// feedback on #21: "Expose the handlers publicly, as Go does"). #[tokio::test] async fn public_handler_wrapper_can_be_delegated_to_from_an_override() { - let mut server = mcpgen::VibeFixtureServiceMcpServer::new(lazy_vibe_client()); + let mut server = mcpgen::VibeFixtureServiceMcpServer::new(serve_vibe_backend().await); server.register_default_tools(); // Cloned before the override is registered, so the clone's own `tools` // map is irrelevant: `get_vibe_handler` only reads `self.client`, not // `self.tools`, so this purely exercises the public wrapper delegating - // to the same stub behavior as the default registration. + // to the same default behavior as the default registration. let delegate = server.clone(); server.register_tool( mcpgen::VibeFixtureServiceMcpServer::::get_vibe_tool(), @@ -160,14 +279,14 @@ async fn public_handler_wrapper_can_be_delegated_to_from_an_override() { .call_tool(CallToolRequestParams::new("GetVibe")) .await .expect("call_tool"); - // Same "not implemented" stub behavior as calling the default handler - // directly, proving the override actually delegated rather than - // short-circuiting. - assert_eq!(result.is_error, Some(true)); + // Same real-backend behavior as calling the default handler directly + // (FakeVibeBackend::get_vibe always returns "chill"), proving the + // override actually delegated rather than short-circuiting. + assert_eq!(result.is_error, Some(false)); let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { panic!("expected text content"); }; - assert_eq!(text.text, "GetVibe is not implemented"); + assert_eq!(text.text, r#"{"vibe":"chill"}"#); } /// `into_tools`/`extend_tools` let one MCP server serve tools from @@ -182,10 +301,10 @@ async fn public_handler_wrapper_can_be_delegated_to_from_an_override() { /// `static GET_VIBE_INPUT_SCHEMA` items, which failed to compile). #[tokio::test] async fn one_server_can_serve_tools_from_two_services() { - let mut vibe_server = mcpgen::VibeFixtureServiceMcpServer::new(lazy_vibe_client()); + let mut vibe_server = mcpgen::VibeFixtureServiceMcpServer::new(serve_vibe_backend().await); vibe_server.register_default_tools(); - let mut other_server = mcpgen::OtherFixtureServiceMcpServer::new(lazy_other_client()); + let mut other_server = mcpgen::OtherFixtureServiceMcpServer::new(serve_other_backend().await); other_server.register_default_tools(); vibe_server.extend_tools(other_server.into_tools()); @@ -207,13 +326,20 @@ async fn one_server_can_serve_tools_from_two_services() { ); // The composed-in tool is actually callable through the combined - // server, not just listed (its stub handler still returns the "not - // implemented" tool error, since handler bodies are #7). + // server, not just listed: it reaches OtherFixtureService's own + // backend (FakeOtherBackend::do_other echoes its input), not + // VibeFixtureService's. + let mut args = JsonObject::new(); + args.insert("value".to_string(), serde_json::json!("hi")); let result = client - .call_tool(CallToolRequestParams::new("DoOther")) + .call_tool(CallToolRequestParams::new("DoOther").with_arguments(args)) .await .expect("call_tool"); - assert_eq!(result.is_error, Some(true)); + assert_eq!(result.is_error, Some(false)); + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + assert_eq!(text.text, r#"{"value":"hi"}"#); } /// Each service's own `GetVibe` input schema reflects its own RPC's @@ -241,23 +367,136 @@ fn same_named_tools_across_services_have_independent_schemas() { ); } -/// Every stub handler returns an `isError: true` "not implemented" tool -/// error (handler bodies are #7), named after its RPC. +/// A successful tool call: the handler validates, decodes, calls the +/// backend, and returns the pbjson-encoded response as `isError: false` +/// text content (this issue, #7). +#[tokio::test] +async fn handler_success_returns_pbjson_encoded_response() { + let mut server = mcpgen::VibeFixtureServiceMcpServer::new(serve_vibe_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("chill")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(false)); + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + // pbjson omits the zero-valued "vibe" field... here it's non-zero, so + // it's present; this also pins the plain camelCase field name. + assert_eq!(text.text, r#"{"vibe":"chill"}"#); +} + +/// A `None` arguments value is treated as `{}` (an empty object), per the +/// issue's requirement, not rejected or passed through as JSON `null`. #[tokio::test] -async fn stub_handler_returns_not_implemented_tool_error() { +async fn handler_treats_missing_arguments_as_empty_object() { + let mut server = mcpgen::VibeFixtureServiceMcpServer::new(serve_vibe_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + // GetVibeRequest has no fields, so "{}" always validates; SetVibe's + // schema also has no required properties (an empty object validates), + // so omitting arguments there pins the same behavior for a message + // that does have fields. + let result = client + .call_tool(CallToolRequestParams::new("SetVibe")) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(false)); + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + // SetVibeRequest.vibe defaults to "", echoed back as "" by + // FakeVibeBackend; pbjson omits a zero-valued string field entirely. + assert_eq!(text.text, "{}"); +} + +/// A gRPC error becomes a tool error formatted like grpc-go's +/// `Status.Error()`: `"rpc error: code = NotFound desc = ..."` (this +/// issue, #7's explicit acceptance criterion). The backend is actually +/// called here (unlike the validation/decode failure tests below). +#[tokio::test] +async fn handler_grpc_error_becomes_tool_error_in_grpc_go_format() { + let mut server = mcpgen::VibeFixtureServiceMcpServer::new(serve_vibe_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("missing")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + assert_eq!( + text.text, + "rpc error: code = NotFound desc = vibe not found" + ); +} + +/// A wrong argument type (a number where the schema wants a string) fails +/// schema validation before decoding, and the backend is never called: +/// this issue's explicit "wrong argument type → isError, backend not +/// called" acceptance criterion. Proven by using the `lazy_*` client +/// (never connects): if the backend were called, the test would hang or +/// error on the broken connection instead of returning this message. +#[tokio::test] +async fn handler_wrong_argument_type_is_rejected_without_calling_backend() { let mut server = mcpgen::VibeFixtureServiceMcpServer::new(lazy_vibe_client()); server.register_default_tools(); let client = serve(server).await; + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!(5)); let result = client - .call_tool(CallToolRequestParams::new("GetVibe")) + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { + panic!("expected text content"); + }; + assert_eq!( + text.text, + r#"invalid arguments: /vibe: 5 is not of type "string""# + ); +} + +/// An unknown field (the schema's `additionalProperties: false`) fails +/// schema validation before decoding, and the backend is never called: +/// this issue's explicit "unknown field → isError, backend not called" +/// acceptance criterion, including the snake_case form of a field that +/// only exists on the *response* message (`SetVibeResponse.vibe`, not +/// `SetVibeRequest`), so this can't accidentally pass by decoding into +/// the wrong message. +#[tokio::test] +async fn handler_unknown_field_is_rejected_without_calling_backend() { + let mut server = mcpgen::VibeFixtureServiceMcpServer::new(lazy_vibe_client()); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("not_a_field".to_string(), serde_json::json!("x")); + let result = client + .call_tool(CallToolRequestParams::new("SetVibe").with_arguments(args)) .await .expect("call_tool"); assert_eq!(result.is_error, Some(true)); let rmcp::model::ContentBlock::Text(text) = &result.content[0] else { panic!("expected text content"); }; - assert_eq!(text.text, "GetVibe is not implemented"); + assert_eq!( + text.text, + r#"invalid arguments: : Additional properties are not allowed ('not_a_field' was unexpected)"# + ); } /// A freshly constructed server has no tools registered yet (the issue's From 3e730e30b03bd477c9f1f82fc19efb2cd9c8c66e Mon Sep 17 00:00:00 2001 From: sf-factory-bot Date: Sat, 10 Oct 2026 01:21:18 +0000 Subject: [PATCH 2/3] fix(generator): update stale stub-handler wording after self-review (#7) register_default_tools's doc comment still said "with its stub handler" after the handler bodies became real; update it, regenerate the committed example, and refresh server_integration.rs's module doc comment to also describe the #7 handler-body coverage added in this PR. --- examples/src/gen/examples/v1/examples.v1.mcp.rs | 2 +- protoc-gen-rust-mcp/src/server.rs | 2 +- protoc-gen-rust-mcp/tests/server_integration.rs | 16 +++++++++++----- 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/examples/src/gen/examples/v1/examples.v1.mcp.rs b/examples/src/gen/examples/v1/examples.v1.mcp.rs index d97038f..fcbfcf5 100644 --- a/examples/src/gen/examples/v1/examples.v1.mcp.rs +++ b/examples/src/gen/examples/v1/examples.v1.mcp.rs @@ -436,7 +436,7 @@ where self } - /// Registers every unary RPC's tool with its stub handler, the + /// Registers every unary RPC's tool with its default handler, the /// counterpart of Go's `RegisterDefaultTools`. pub fn register_default_tools(&mut self) -> &mut Self { { diff --git a/protoc-gen-rust-mcp/src/server.rs b/protoc-gen-rust-mcp/src/server.rs index 2e19b04..4c741b6 100644 --- a/protoc-gen-rust-mcp/src/server.rs +++ b/protoc-gen-rust-mcp/src/server.rs @@ -244,7 +244,7 @@ pub fn generate_service(service: &ServiceDescriptor) -> String { ); out.push_str( - " /// Registers every unary RPC's tool with its stub handler, the\n\ + " /// Registers every unary RPC's tool with its default handler, the\n\ \u{20} /// counterpart of Go's `RegisterDefaultTools`.\n\ \u{20} pub fn register_default_tools(&mut self) -> &mut Self {\n", ); diff --git a/protoc-gen-rust-mcp/tests/server_integration.rs b/protoc-gen-rust-mcp/tests/server_integration.rs index 457f131..47a3ea6 100644 --- a/protoc-gen-rust-mcp/tests/server_integration.rs +++ b/protoc-gen-rust-mcp/tests/server_integration.rs @@ -1,10 +1,16 @@ //! Integration test for `server::generate_service`'s output (issue #6's -//! acceptance criteria), exercised end-to-end over an in-memory MCP -//! transport against the real tonic-generated client code for +//! tool/registration acceptance criteria, and issue #7's handler-body +//! acceptance criteria: validation, pbjson decoding, the tonic call, and +//! pbjson encoding), exercised end-to-end over an in-memory MCP transport +//! against the real tonic-generated client code for //! `tests/testdata/mcpgen/fixture.proto` (compiled by `build.rs` with real -//! `tonic-prost-build`, and this crate's own generator run against the -//! same descriptors; see `build.rs`'s `generate_mcpgen_fixture` for how -//! `mcpgen.rs` and `mcpgen.mcp.rs` land in `OUT_DIR`). +//! `tonic-prost-build` and `pbjson-build`, and this crate's own generator +//! run against the same descriptors; see `build.rs`'s +//! `generate_mcpgen_fixture` for how `mcpgen.rs`, `mcpgen.serde.rs` and +//! `mcpgen.mcp.rs` land in `OUT_DIR`). `examples/tests/handler.rs` covers +//! the same #7 criteria again against the real, committed example +//! `VibeService` server, so a schema/pbjson regression in the real +//! generated code (not just this synthetic fixture) is also caught. //! //! Unlike `examples/`, this fixture's generated code is not committed: //! `fixture.proto` exists only to give this test a service with a From 8d6d787332f5e05b65667e706369ef0fa5230e92 Mon Sep 17 00:00:00 2001 From: sf-factory-bot Date: Sat, 10 Oct 2026 01:34:09 +0000 Subject: [PATCH 3/3] test(examples): pin NaN-result and enum-as-number review follow-ups (#7) Addresses erikist's review on PR #25: - examples/Cargo.toml: add rmcp with "client"/"transport-io" dev-deps so `cargo test -p examples` compiles standalone (it previously relied on feature unification with protoc-gen-rust-mcp's dev-deps under `cargo test --workspace`). - examples/tests/handler.rs: add non_finite_floats_in_result_are_encoded_as_json_null, pinning the pbjson-vs-protojson NaN/Infinity result divergence documented in server.rs (and implement FakeVibeService::set_vibe_array to actually echo its request, so this test has a backend to call). - examples/tests/handler.rs: add enum_as_number_is_rejected_by_schema_without_calling_backend, the one case that isolates schema validation from pbjson's own decode (which would accept an enum given as its numeric value). - protoc-gen-rust-mcp/src/server.rs: note in the module doc comment that the NaN/Infinity divergence is now pinned by a test, not just documented. --- examples/Cargo.toml | 9 +++++ examples/tests/handler.rs | 67 ++++++++++++++++++++++++++++++- protoc-gen-rust-mcp/src/server.rs | 3 +- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/examples/Cargo.toml b/examples/Cargo.toml index 8ca27ee..6eb5713 100644 --- a/examples/Cargo.toml +++ b/examples/Cargo.toml @@ -41,3 +41,12 @@ tokio = { version = "1", features = ["macros", "rt-multi-thread"] } # tonic server in tests/handler.rs ("server" and "channel" are already # pulled in by [dependencies].tonic above). tonic = { version = "0.14", default-features = false, features = ["router"] } +# tests/handler.rs drives the generated server through a real rmcp +# client ("client") over an in-memory duplex transport +# ("transport-io"), instead of calling the handler directly, so it +# exercises the same wire path a real MCP client would. Needed here (in +# addition to [dependencies]'s "server") so `cargo test -p examples` +# compiles on its own, without relying on feature unification with +# protoc-gen-rust-mcp's own dev-dependency on rmcp under +# `cargo test --workspace`. +rmcp = { version = "3.5", default-features = false, features = ["client", "transport-io"] } diff --git a/examples/tests/handler.rs b/examples/tests/handler.rs index d0c9590..9ded647 100644 --- a/examples/tests/handler.rs +++ b/examples/tests/handler.rs @@ -56,9 +56,11 @@ impl VibeService for FakeVibeService { async fn set_vibe_array( &self, - _request: tonic::Request, + request: tonic::Request, ) -> Result, tonic::Status> { - Err(tonic::Status::unimplemented("not used by this test")) + Ok(tonic::Response::new(SetVibeArrayResponse { + vibe_array: request.into_inner().vibe_array, + })) } async fn set_vibe_objects( @@ -272,3 +274,64 @@ async fn non_finite_floats_are_accepted_as_input() { ); } } + +/// Pinning the pbjson-vs-protojson difference flagged on #18's review and +/// documented in `server.rs`'s module doc comment: a non-finite +/// float/double comes back from the backend as JSON `null` in the tool +/// *result* (unlike protojson's `"NaN"`/`"Infinity"`/`"-Infinity"` +/// strings). A module doc comment alone doesn't catch a pbjson change +/// that fixes or worsens this, so this test does. +#[tokio::test] +async fn non_finite_floats_in_result_are_encoded_as_json_null() { + let mut server = VibeServiceMcpServer::new(serve_backend().await); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert( + "vibeArray".to_string(), + serde_json::json!({"vibeDoubles": ["NaN", "Infinity", "-Infinity"]}), + ); + let result = client + .call_tool(CallToolRequestParams::new("SetVibeArray").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(false)); + assert_eq!( + text_of(&result), + r#"{"vibeArray":{"vibeDoubles":[null,null,null]}}"# + ); +} + +/// An enum given as its underlying number (`1` for `VIBE_GOOD`) is +/// accepted by pbjson's generated `Deserialize` (see +/// `vibe_scalar::VibeEnum`'s `Deserialize` impl's `visit_i64`/`visit_u64`) +/// but rejected by the schema, which restricts enums to their name +/// strings (`schema.rs`'s `enum_schema`). Unlike the wrong-type and +/// unknown-field cases above, pbjson's own decode would *accept* this +/// input, so this is the one case that isolates schema validation from +/// decoding: without the validation branch, this would reach (and fail +/// only at) decode, or succeed outright. +#[tokio::test] +async fn enum_as_number_is_rejected_by_schema_without_calling_backend() { + let mut server = VibeServiceMcpServer::new(lazy_client()); + server.register_default_tools(); + let client = serve(server).await; + + let mut args = JsonObject::new(); + args.insert("vibe".to_string(), serde_json::json!("details")); + args.insert( + "vibeScalar".to_string(), + serde_json::json!({"vibeEnum": [1]}), + ); + let result = client + .call_tool(CallToolRequestParams::new("SetVibeDetails").with_arguments(args)) + .await + .expect("call_tool"); + assert_eq!(result.is_error, Some(true)); + assert_eq!( + text_of(&result), + "invalid arguments: /vibeScalar/vibeEnum/0: 1 is not of type \"string\"; \ + /vibeScalar/vibeEnum/0: 1 is not one of \"VIBE_UNSET\" or \"VIBE_GOOD\"" + ); +} diff --git a/protoc-gen-rust-mcp/src/server.rs b/protoc-gen-rust-mcp/src/server.rs index 4c741b6..7009e4c 100644 --- a/protoc-gen-rust-mcp/src/server.rs +++ b/protoc-gen-rust-mcp/src/server.rs @@ -49,7 +49,8 @@ //! `"NaN"`/`"Infinity"`/`"-Infinity"`; this only affects tool *results* //! (`float_schema` in `schema.rs` already documents that inputs accept //! all three spellings either way). Reported in this issue's PR per -//! review feedback on #18. +//! review feedback on #18, and pinned by `examples/tests/handler.rs`'s +//! `non_finite_floats_in_result_are_encoded_as_json_null`. //! //! # Shape //!