diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index 91b43db079..329637b119 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -200,7 +200,7 @@ macro_rules! client_request_definitions { $( $(#[experimental($reason:expr)])? $(#[doc = $variant_doc:literal])* - $variant:ident $(=> $wire:literal)? { + $variant:ident => $wire:literal { params: $(#[$params_meta:meta])* $params:ty, $(inspect_params: $inspect_params:tt,)? serialization: $serialization:ident $( ( $($serialization_args:tt)* ) )?, @@ -215,7 +215,8 @@ macro_rules! client_request_definitions { pub enum ClientRequest { $( $(#[doc = $variant_doc])* - $(#[serde(rename = $wire)] #[ts(rename = $wire)])? + #[serde(rename = $wire)] + #[ts(rename = $wire)] $variant { #[serde(rename = "id")] request_id: RequestId, @@ -232,16 +233,14 @@ macro_rules! client_request_definitions { } } + pub const fn method_name(&self) -> &'static str { + match self { + $(Self::$variant { .. } => $wire,)* + } + } + pub fn method(&self) -> String { - serde_json::to_value(self) - .ok() - .and_then(|value| { - value - .get("method") - .and_then(serde_json::Value::as_str) - .map(str::to_owned) - }) - .unwrap_or_else(|| "".to_string()) + self.method_name().to_string() } pub fn serialization_scope(&self) -> Option { @@ -258,6 +257,26 @@ macro_rules! client_request_definitions { } } + impl TryFrom for ClientRequest { + type Error = serde_json::Error; + + fn try_from(request: JSONRPCRequest) -> Result { + let JSONRPCRequest { + id: request_id, + method, + params, + trace: _, + } = request; + let mut request = serde_json::Map::new(); + request.insert("id".to_string(), serde_json::to_value(request_id)?); + request.insert("method".to_string(), serde_json::Value::String(method)); + if let Some(params) = params { + request.insert("params".to_string(), params); + } + serde_json::from_value(serde_json::Value::Object(request)) + } + } + /// Typed response from the server to the client. #[derive(Serialize, Deserialize, Debug, Clone)] #[allow(clippy::large_enum_variant)] @@ -265,7 +284,7 @@ macro_rules! client_request_definitions { pub enum ClientResponse { $( $(#[doc = $variant_doc])* - $(#[serde(rename = $wire)])? + #[serde(rename = $wire)] $variant { #[serde(rename = "id")] request_id: RequestId, @@ -282,15 +301,9 @@ macro_rules! client_request_definitions { } pub fn method(&self) -> String { - serde_json::to_value(self) - .ok() - .and_then(|value| { - value - .get("method") - .and_then(serde_json::Value::as_str) - .map(str::to_owned) - }) - .unwrap_or_else(|| "".to_string()) + match self { + $(Self::$variant { .. } => $wire.to_string(),)* + } } pub fn into_jsonrpc_parts( @@ -405,7 +418,7 @@ macro_rules! client_request_definitions { pub(crate) const EXPERIMENTAL_CLIENT_METHODS: &[&str] = &[ $( - experimental_method_entry!($(#[experimental($reason)])? $(=> $wire)?), + experimental_method_entry!($(#[experimental($reason)])? => $wire), )* ]; pub(crate) const EXPERIMENTAL_CLIENT_METHOD_PARAM_TYPES: &[&str] = &[ @@ -470,7 +483,7 @@ macro_rules! client_response_payload_from_impl { } client_request_definitions! { - Initialize { + Initialize => "initialize" { params: v1::InitializeParams, serialization: None, response: v1::InitializeResponse, @@ -1177,25 +1190,25 @@ client_request_definitions! { }, /// DEPRECATED APIs below - GetConversationSummary { + GetConversationSummary => "getConversationSummary" { params: v1::GetConversationSummaryParams, serialization: None, response: v1::GetConversationSummaryResponse, }, - GitDiffToRemote { + GitDiffToRemote => "gitDiffToRemote" { params: v1::GitDiffToRemoteParams, serialization: None, response: v1::GitDiffToRemoteResponse, }, /// DEPRECATED in favor of GetAccount - GetAuthStatus { + GetAuthStatus => "getAuthStatus" { params: v1::GetAuthStatusParams, serialization: global("account-auth"), response: v1::GetAuthStatusResponse, }, // Legacy fuzzy search cancellation is intentionally concurrent: clients reuse a // cancellation token so a newer request can cancel an older in-flight search. - FuzzyFileSearch { + FuzzyFileSearch => "fuzzyFileSearch" { params: FuzzyFileSearchParams, serialization: None, response: FuzzyFileSearchResponse, @@ -1798,6 +1811,81 @@ mod tests { RequestId::Integer(REQUEST_ID) } + fn decode_client_request_through_json( + request: &JSONRPCRequest, + ) -> std::result::Result { + serde_json::to_value(request) + .and_then(serde_json::from_value) + .map_err(|err| err.to_string()) + } + + #[test] + fn jsonrpc_request_conversion_preserves_serde_enum_decoding() { + let requests = [ + JSONRPCRequest { + id: RequestId::Integer(1), + method: "thread/archive".to_string(), + params: Some(json!({"threadId": "thread-1"})), + trace: Some(codex_protocol::protocol::W3cTraceContext { + traceparent: Some("traceparent".to_string()), + tracestate: Some("tracestate".to_string()), + }), + }, + // Required params preserve distinct omitted and explicit-null errors. + JSONRPCRequest { + id: RequestId::Integer(2), + method: "thread/archive".to_string(), + params: None, + trace: None, + }, + JSONRPCRequest { + id: RequestId::Integer(3), + method: "thread/archive".to_string(), + params: Some(serde_json::Value::Null), + trace: None, + }, + // Optional unit params preserve omitted, null, and empty-object behavior. + JSONRPCRequest { + id: RequestId::Integer(4), + method: "memory/reset".to_string(), + params: None, + trace: None, + }, + JSONRPCRequest { + id: RequestId::Integer(5), + method: "memory/reset".to_string(), + params: Some(serde_json::Value::Null), + trace: None, + }, + JSONRPCRequest { + id: RequestId::Integer(6), + method: "memory/reset".to_string(), + params: Some(json!({})), + trace: None, + }, + JSONRPCRequest { + id: RequestId::Integer(7), + method: "getConversationSummary".to_string(), + params: Some(json!({ + "conversationId": "67e55044-10b1-426f-9247-bb680e5fe0c8" + })), + trace: None, + }, + JSONRPCRequest { + id: RequestId::Integer(8), + method: "unknown/method".to_string(), + params: Some(json!({})), + trace: None, + }, + ]; + + for request in requests { + let expected = decode_client_request_through_json(&request); + let actual = ClientRequest::try_from(request).map_err(|err| err.to_string()); + assert_eq!(actual, expected); + } + } + #[test] fn client_request_serialization_scope_covers_keyed_families() { let thread_id = "thread-1".to_string(); diff --git a/codex-rs/app-server-transport/src/transport/mod.rs b/codex-rs/app-server-transport/src/transport/mod.rs index 1c45a725ed..2108ce77c8 100644 --- a/codex-rs/app-server-transport/src/transport/mod.rs +++ b/codex-rs/app-server-transport/src/transport/mod.rs @@ -256,14 +256,7 @@ async fn enqueue_incoming_message( } fn serialize_outgoing_message(outgoing_message: OutgoingMessage) -> Option { - let value = match serde_json::to_value(outgoing_message) { - Ok(value) => value, - Err(err) => { - error!("Failed to convert OutgoingMessage to JSON value: {err}"); - return None; - } - }; - match serde_json::to_string(&value) { + match serde_json::to_string(&outgoing_message) { Ok(json) => Some(json), Err(err) => { error!("Failed to serialize JSONRPCMessage: {err}"); @@ -295,6 +288,32 @@ mod tests { ); } + #[test] + fn serialize_outgoing_message_preserves_wire_shape() { + let message = OutgoingMessage::AppServerNotification(ServerNotificationEnvelope { + notification: ServerNotification::ConfigWarning(ConfigWarningNotification { + summary: "summary".to_string(), + details: None, + path: None, + range: None, + }), + emitted_at_ms: Some(1_234), + }); + + let json = serialize_outgoing_message(message).expect("message should serialize"); + assert_eq!( + serde_json::from_str::(&json).expect("message should be valid JSON"), + json!({ + "method": "configWarning", + "params": { + "summary": "summary", + "details": null, + }, + "emittedAtMs": 1_234, + }) + ); + } + #[tokio::test] async fn enqueue_incoming_request_returns_overload_error_when_queue_is_full() { let connection_id = ConnectionId(42); diff --git a/codex-rs/app-server/src/app_server_tracing.rs b/codex-rs/app-server/src/app_server_tracing.rs index 6e8133740f..764634d13f 100644 --- a/codex-rs/app-server/src/app_server_tracing.rs +++ b/codex-rs/app-server/src/app_server_tracing.rs @@ -64,8 +64,8 @@ pub(crate) fn typed_request_span( connection_id: ConnectionId, session: &ConnectionSessionState, ) -> Span { - let method = request.method(); - let span = app_server_request_span_template(&method, "in-process", request.id(), connection_id); + let method = request.method_name(); + let span = app_server_request_span_template(method, "in-process", request.id(), connection_id); let client_info = initialize_client_info_from_typed_request(request); record_client_info( @@ -78,7 +78,7 @@ pub(crate) fn typed_request_span( .or(session.client_version()), ); - attach_parent_context(&span, &method, request.id(), /*parent_trace*/ None); + attach_parent_context(&span, method, request.id(), /*parent_trace*/ None); span } diff --git a/codex-rs/app-server/src/message_processor.rs b/codex-rs/app-server/src/message_processor.rs index 133c0c84fc..239dd6aed1 100644 --- a/codex-rs/app-server/src/message_processor.rs +++ b/codex-rs/app-server/src/message_processor.rs @@ -88,15 +88,9 @@ use crate::models_refresh_worker::ModelsRefreshWorker; const CONNECTION_RPC_DRAIN_TIMEOUT: Duration = Duration::from_secs(/*secs*/ 30); -fn deserialize_client_request( - request: &JSONRPCRequest, -) -> Result { - serde_json::to_value(request) +fn deserialize_client_request(request: JSONRPCRequest) -> Result { + ClientRequest::try_from(request) .map_err(|err| invalid_request(format!("Invalid request: {err}"))) - .and_then(|request_json| { - serde_json::from_value(request_json) - .map_err(|err| invalid_request(format!("Invalid request: {err}"))) - }) } pub(crate) struct MessageProcessor { @@ -543,7 +537,7 @@ impl MessageProcessor { Arc::clone(&self.outgoing), request_context.clone(), async { - let codex_request = deserialize_client_request(&request); + let codex_request = deserialize_client_request(request); let result = match codex_request { Ok(codex_request) => { // Websocket callers finalize outbound readiness in lib.rs after mirroring