fix: address review feedback on #1621 and #1623 (#1631)

- formalizes `ExecApprovalElicitRequestParams`
- adds some defensive logic when messages fail to parse
- fixes a typo in a comment

codex-rs/mcp-server/src/codex_tool_runner.rs

@@ -2,6 +2,7 @@
 //! Tokio task. Separated from `message_processor.rs` to keep that file small
 //! and to make future feature-growth easier to manage.
 
+use std::path::PathBuf;
 use std::sync::Arc;
 
 use codex_core::Codex;
@@ -19,15 +20,19 @@ use mcp_types::CallToolResult;
 use mcp_types::ContentBlock;
 use mcp_types::ElicitRequest;
 use mcp_types::ElicitRequestParamsRequestedSchema;
+use mcp_types::JSONRPCErrorError;
 use mcp_types::ModelContextProtocolRequest;
 use mcp_types::RequestId;
 use mcp_types::TextContent;
 use serde::Deserialize;
+use serde::Serialize;
 use serde_json::json;
 use tracing::error;
 
 use crate::outgoing_message::OutgoingMessageSender;
 
+const INVALID_PARAMS_ERROR_CODE: i64 = -32602;
+
 /// Run a complete Codex session and stream events back to the client.
 ///
 /// On completion (success or error) the function sends the appropriate
@@ -97,28 +102,44 @@ pub async fn run_codex_tool_session(
                             .unwrap_or_else(|_| command.join(" "));
                         let message = format!("Allow Codex to run `{escaped_command}` in {cwd:?}?");
 
-                        let params = json!({
+                        let params = ExecApprovalElicitRequestParams {
-                            // These fields are required so that `params`
+                            message,
-                            // conforms to ElicitRequestParams.
+                            requested_schema: ElicitRequestParamsRequestedSchema {
-                            "message": message,
-                            "requestedSchema": ElicitRequestParamsRequestedSchema {
                                 r#type: "object".to_string(),
                                 properties: json!({}),
                                 required: None,
                             },
+                            codex_elicitation: "exec-approval".to_string(),
+                            codex_mcp_tool_call_id: sub_id.clone(),
+                            codex_event_id: event.id.clone(),
+                            codex_command: command,
+                            codex_cwd: cwd,
+                        };
+                        let params_json = match serde_json::to_value(&params) {
+                            Ok(value) => value,
+                            Err(err) => {
+                                let message = format!(
+                                    "Failed to serialize ExecApprovalElicitRequestParams: {err}"
+                                );
+                                tracing::error!("{message}");
+
+                                outgoing
+                                    .send_error(
+                                        id.clone(),
+                                        JSONRPCErrorError {
+                                            code: INVALID_PARAMS_ERROR_CODE,
+                                            message,
+                                            data: None,
+                                        },
+                                    )
+                                    .await;
+
+                                continue;
+                            }
+                        };
 
-                            // These are additional fields the client can use to
-                            // correlate the request with the codex tool call.
-                            "codex_elicitation": "exec-approval",
-                            "codex_mcp_tool_call_id": sub_id,
-                            "codex_event_id": event.id,
-                            "codex_command": command,
-                            // Could convert it to base64 encoded bytes if we
-                            // don't want to use to_string_lossy() here?
-                            "codex_cwd": cwd.to_string_lossy().to_string()
-                        });
                         let on_response = outgoing
-                            .send_request(ElicitRequest::METHOD, Some(params))
+                            .send_request(ElicitRequest::METHOD, Some(params_json))
                             .await;
 
                         // Listen for the response on a separate task so we do
@@ -236,7 +257,11 @@ async fn on_exec_approval_response(
         Ok(response) => response,
         Err(err) => {
             error!("failed to deserialize ExecApprovalResponse: {err}");
-            return;
+            // If we cannot deserialize the response, we deny the request to be
+            // conservative.
+            ExecApprovalResponse {
+                decision: ReviewDecision::Denied,
+            }
         }
     };
 
@@ -255,3 +280,23 @@ async fn on_exec_approval_response(
 pub struct ExecApprovalResponse {
     pub decision: ReviewDecision,
 }
+
+/// Conforms to [`mcp_types::ElicitRequestParams`] so that it can be used as the
+/// `params` field of an [`mcp_types::ElicitRequest`].
+#[derive(Debug, Serialize)]
+struct ExecApprovalElicitRequestParams {
+    // These fields are required so that `params`
+    // conforms to ElicitRequestParams.
+    message: String,
+
+    #[serde(rename = "requestedSchema")]
+    requested_schema: ElicitRequestParamsRequestedSchema,
+
+    // These are additional fields the client can use to
+    // correlate the request with the codex tool call.
+    codex_elicitation: String,
+    codex_mcp_tool_call_id: String,
+    codex_event_id: String,
+    codex_command: Vec<String>,
+    codex_cwd: PathBuf,
+}

codex-rs/mcp-server/src/message_processor.rs

@@ -365,7 +365,7 @@ impl MessageProcessor {
         // Spawn an async task to handle the Codex session so that we do not
         // block the synchronous message-processing loop.
         task::spawn(async move {
-            // Run the Codex session and stream events Fck to the client.
+            // Run the Codex session and stream events back to the client.
             crate::codex_tool_runner::run_codex_tool_session(id, initial_prompt, config, outgoing)
                 .await;
         });