From 3f40fbc0a886ef54f494a1ff5963971bc42c036a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 4 Sep 2025 17:49:50 -0700 Subject: [PATCH] chore: improve serialization of ServerNotification (#3193) This PR introduces introduces a new `OutgoingMessage::AppServerNotification` variant that is designed to wrap a `ServerNotification`, which makes the serialization more straightforward compared to `OutgoingMessage::Notification(OutgoingNotification)`. We still use the latter for serializing an `Event` as a `JSONRPCMessage::Notification`, but I will try to get away from that in the near future. With this change, now the generated TypeScript type for `ServerNotification` is: ```typescript export type ServerNotification = | { "method": "authStatusChange", "params": AuthStatusChangeNotification } | { "method": "loginChatGptComplete", "params": LoginChatGptCompleteNotification }; ``` whereas before it was: ```typescript export type ServerNotification = | { type: "auth_status_change"; data: AuthStatusChangeNotification } | { type: "login_chat_gpt_complete"; data: LoginChatGptCompleteNotification }; ``` Once the `Event`s are migrated to the `ServerNotification` enum in Rust, it should be considerably easier to work with notifications on the TypeScript side, as it will be possible to `switch (message.method)` and check for exhaustiveness. Though we will probably need to introduce: ```typescript export type ServerMessage = ServerRequest | ServerNotification; ``` and then we still need to group all of the `ServerResponse` types together, as well. --- .../mcp-server/src/codex_message_processor.rs | 6 +- codex-rs/mcp-server/src/outgoing_message.rs | 58 ++++++++++++++++--- codex-rs/protocol/src/mcp_protocol.rs | 13 ++++- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/codex-rs/mcp-server/src/codex_message_processor.rs b/codex-rs/mcp-server/src/codex_message_processor.rs index 7de12feb..2309e01d 100644 --- a/codex-rs/mcp-server/src/codex_message_processor.rs +++ b/codex-rs/mcp-server/src/codex_message_processor.rs @@ -815,9 +815,9 @@ impl CodexMessageProcessor { }; // For now, we send a notification for every event, - // JSON-serializing the `Event` as-is, but we will move - // to creating a special enum for notifications with a - // stable wire format. + // JSON-serializing the `Event` as-is, but these should + // be migrated to be variants of `ServerNotification` + // instead. let method = format!("codex/event/{}", event.msg); let mut params = match serde_json::to_value(event.clone()) { Ok(serde_json::Value::Object(map)) => map, diff --git a/codex-rs/mcp-server/src/outgoing_message.rs b/codex-rs/mcp-server/src/outgoing_message.rs index c26c8e3d..b735f2bf 100644 --- a/codex-rs/mcp-server/src/outgoing_message.rs +++ b/codex-rs/mcp-server/src/outgoing_message.rs @@ -97,6 +97,9 @@ impl OutgoingMessageSender { } } + /// This is used with the MCP server, but not the more general JSON-RPC app + /// server. Prefer [`OutgoingMessageSender::send_server_notification`] where + /// possible. pub(crate) async fn send_event_as_notification( &self, event: &Event, @@ -123,14 +126,9 @@ impl OutgoingMessageSender { } pub(crate) async fn send_server_notification(&self, notification: ServerNotification) { - let method = format!("codex/event/{notification}"); - let params = match serde_json::to_value(¬ification) { - Ok(serde_json::Value::Object(mut map)) => map.remove("data"), - _ => None, - }; - let outgoing_message = - OutgoingMessage::Notification(OutgoingNotification { method, params }); - let _ = self.sender.send(outgoing_message); + let _ = self + .sender + .send(OutgoingMessage::AppServerNotification(notification)); } pub(crate) async fn send_notification(&self, notification: OutgoingNotification) { @@ -148,6 +146,9 @@ impl OutgoingMessageSender { pub(crate) enum OutgoingMessage { Request(OutgoingRequest), Notification(OutgoingNotification), + /// AppServerNotification is specific to the case where this is run as an + /// "app server" as opposed to an MCP server. + AppServerNotification(ServerNotification), Response(OutgoingResponse), Error(OutgoingError), } @@ -171,6 +172,21 @@ impl From for JSONRPCMessage { params, }) } + AppServerNotification(notification) => { + let method = notification.to_string(); + let params = match notification.to_params() { + Ok(params) => Some(params), + Err(err) => { + warn!("failed to serialize notification params: {err}"); + None + } + }; + JSONRPCMessage::Notification(JSONRPCNotification { + jsonrpc: JSONRPC_VERSION.into(), + method, + params, + }) + } Response(OutgoingResponse { id, result }) => { JSONRPCMessage::Response(JSONRPCResponse { jsonrpc: JSONRPC_VERSION.into(), @@ -242,6 +258,7 @@ pub(crate) struct OutgoingError { mod tests { use codex_core::protocol::EventMsg; use codex_core::protocol::SessionConfiguredEvent; + use codex_protocol::mcp_protocol::LoginChatGptCompleteNotification; use pretty_assertions::assert_eq; use serde_json::json; use uuid::Uuid; @@ -324,4 +341,29 @@ mod tests { }); assert_eq!(params.unwrap(), expected_params); } + + #[test] + fn verify_server_notification_serialization() { + let notification = + ServerNotification::LoginChatGptComplete(LoginChatGptCompleteNotification { + login_id: Uuid::nil(), + success: true, + error: None, + }); + + let jsonrpc_notification: JSONRPCMessage = + OutgoingMessage::AppServerNotification(notification).into(); + assert_eq!( + JSONRPCMessage::Notification(JSONRPCNotification { + jsonrpc: "2.0".into(), + method: "loginChatGptComplete".into(), + params: Some(json!({ + "loginId": Uuid::nil(), + "success": true, + })), + }), + jsonrpc_notification, + "ensure the strum macros serialize the method field correctly" + ); + } } diff --git a/codex-rs/protocol/src/mcp_protocol.rs b/codex-rs/protocol/src/mcp_protocol.rs index a5a646fa..3980de0a 100644 --- a/codex-rs/protocol/src/mcp_protocol.rs +++ b/codex-rs/protocol/src/mcp_protocol.rs @@ -551,8 +551,8 @@ pub struct AuthStatusChangeNotification { } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, TS, Display)] -#[serde(tag = "type", content = "data", rename_all = "snake_case")] -#[strum(serialize_all = "snake_case")] +#[serde(tag = "method", content = "params", rename_all = "camelCase")] +#[strum(serialize_all = "camelCase")] pub enum ServerNotification { /// Authentication status changed AuthStatusChange(AuthStatusChangeNotification), @@ -561,6 +561,15 @@ pub enum ServerNotification { LoginChatGptComplete(LoginChatGptCompleteNotification), } +impl ServerNotification { + pub fn to_params(self) -> Result { + match self { + ServerNotification::AuthStatusChange(params) => serde_json::to_value(params), + ServerNotification::LoginChatGptComplete(params) => serde_json::to_value(params), + } + } +} + #[cfg(test)] mod tests { use super::*;