diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index 480b96f7c..51167dd3f 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -58,6 +58,7 @@ use codex_app_server_protocol::AppInfo; use codex_otel::TelemetryAuthMode; use codex_protocol::models::BaseInstructions; use codex_protocol::models::ContentItem; +use codex_protocol::models::McpToolOutput; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ModelsResponse; @@ -1607,7 +1608,7 @@ fn prefers_structured_content_when_present() { meta: None, }; - let got = FunctionCallOutputPayload::from(&ctr); + let got = McpToolOutput::from(&ctr).into_function_call_output_payload(); let expected = FunctionCallOutputPayload { body: FunctionCallOutputBody::Text( serde_json::to_string(&json!({ @@ -1689,7 +1690,7 @@ fn falls_back_to_content_when_structured_is_null() { meta: None, }; - let got = FunctionCallOutputPayload::from(&ctr); + let got = McpToolOutput::from(&ctr).into_function_call_output_payload(); let expected = FunctionCallOutputPayload { body: FunctionCallOutputBody::Text( serde_json::to_string(&vec![text_block("hello"), text_block("world")]).unwrap(), @@ -1709,7 +1710,7 @@ fn success_flag_reflects_is_error_true() { meta: None, }; - let got = FunctionCallOutputPayload::from(&ctr); + let got = McpToolOutput::from(&ctr).into_function_call_output_payload(); let expected = FunctionCallOutputPayload { body: FunctionCallOutputBody::Text( serde_json::to_string(&json!({ "message": "bad" })).unwrap(), @@ -1729,7 +1730,7 @@ fn success_flag_true_with_no_error_and_content_used() { meta: None, }; - let got = FunctionCallOutputPayload::from(&ctr); + let got = McpToolOutput::from(&ctr).into_function_call_output_payload(); let expected = FunctionCallOutputPayload { body: FunctionCallOutputBody::Text( serde_json::to_string(&vec![text_block("alpha")]).unwrap(), diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 29d1de5b6..6bce8c093 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -29,9 +29,7 @@ use crate::protocol::McpToolCallBeginEvent; use crate::protocol::McpToolCallEndEvent; use crate::state_db; use codex_protocol::mcp::CallToolResult; -use codex_protocol::models::FunctionCallOutputBody; -use codex_protocol::models::FunctionCallOutputPayload; -use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::McpToolOutput; use codex_protocol::openai_models::InputModality; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::ReviewDecision; @@ -58,7 +56,7 @@ pub(crate) async fn handle_mcp_tool_call( server: String, tool_name: String, arguments: String, -) -> ResponseInputItem { +) -> McpToolOutput { // Parse the `arguments` as JSON. An empty string is OK, but invalid JSON // is not. let arguments_value = if arguments.trim().is_empty() { @@ -68,13 +66,7 @@ pub(crate) async fn handle_mcp_tool_call( Ok(value) => Some(value), Err(e) => { error!("failed to parse tool call arguments: {e}"); - return ResponseInputItem::FunctionCallOutput { - call_id: call_id.clone(), - output: FunctionCallOutputPayload { - body: FunctionCallOutputBody::Text(format!("err: {e}")), - success: Some(false), - }, - }; + return McpToolOutput::from_error_text(format!("err: {e}")); } } }; @@ -118,7 +110,7 @@ pub(crate) async fn handle_mcp_tool_call( turn_context .session_telemetry .counter("codex.mcp.call", 1, &[("status", status)]); - return ResponseInputItem::McpToolCallOutput { call_id, result }; + return McpToolOutput::from_result(result); } if let Some(decision) = maybe_request_mcp_tool_approval( @@ -212,7 +204,7 @@ pub(crate) async fn handle_mcp_tool_call( .session_telemetry .counter("codex.mcp.call", 1, &[("status", status)]); - return ResponseInputItem::McpToolCallOutput { call_id, result }; + return McpToolOutput::from_result(result); } let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { @@ -258,7 +250,7 @@ pub(crate) async fn handle_mcp_tool_call( .session_telemetry .counter("codex.mcp.call", 1, &[("status", status)]); - ResponseInputItem::McpToolCallOutput { call_id, result } + McpToolOutput::from_result(result) } async fn maybe_mark_thread_memory_mode_polluted(sess: &Session, turn_context: &TurnContext) { diff --git a/codex-rs/core/src/stream_events_utils.rs b/codex-rs/core/src/stream_events_utils.rs index 8f77a80e3..afd600c94 100644 --- a/codex-rs/core/src/stream_events_utils.rs +++ b/codex-rs/core/src/stream_events_utils.rs @@ -359,14 +359,8 @@ pub(crate) fn response_input_to_response_item(input: &ResponseInputItem) -> Opti output: output.clone(), }) } - ResponseInputItem::McpToolCallOutput { call_id, result } => { - let output = match result { - Ok(call_tool_result) => FunctionCallOutputPayload::from(call_tool_result), - Err(err) => FunctionCallOutputPayload { - body: FunctionCallOutputBody::Text(err.clone()), - success: Some(false), - }, - }; + ResponseInputItem::McpToolCallOutput { call_id, output } => { + let output = output.as_function_call_output_payload(); Some(ResponseItem::FunctionCallOutput { call_id: call_id.clone(), output, diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index b5e799566..ce6f8ea53 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -7,10 +7,10 @@ use crate::truncate::TruncationPolicy; use crate::truncate::formatted_truncate_text; use crate::turn_diff_tracker::TurnDiffTracker; use crate::unified_exec::resolve_max_tokens; -use codex_protocol::mcp::CallToolResult; use codex_protocol::models::FunctionCallOutputBody; use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::models::FunctionCallOutputPayload; +use codex_protocol::models::McpToolOutput; use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ShellToolCallParams; use codex_protocol::models::function_call_output_content_items_to_text; @@ -82,23 +82,21 @@ pub trait ToolOutput: Send { } } -pub struct McpToolOutput { - pub result: Result, -} - impl ToolOutput for McpToolOutput { fn log_preview(&self) -> String { - format!("{:?}", self.result) + let output = self.as_function_call_output_payload(); + let preview = output.body.to_text().unwrap_or_else(|| output.to_string()); + telemetry_preview(&preview) } fn success_for_logging(&self) -> bool { - self.result.is_ok() + self.success } fn to_response_item(&self, call_id: &str, _payload: &ToolPayload) -> ResponseInputItem { ResponseInputItem::McpToolCallOutput { call_id: call_id.to_string(), - result: self.result.clone(), + output: self.clone(), } } } @@ -273,15 +271,14 @@ fn response_input_to_code_mode_result(response: ResponseInputItem) -> JsonValue content_items_to_code_mode_result(&items) } }, - ResponseInputItem::McpToolCallOutput { result, .. } => match result { - Ok(result) => match FunctionCallOutputPayload::from(&result).body { + ResponseInputItem::McpToolCallOutput { output, .. } => { + match output.as_function_call_output_payload().body { FunctionCallOutputBody::Text(text) => JsonValue::String(text), FunctionCallOutputBody::ContentItems(items) => { content_items_to_code_mode_result(&items) } - }, - Err(error) => JsonValue::String(error), - }, + } + } } } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index fc921be96..14b6926e8 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -3,47 +3,16 @@ use std::sync::Arc; use crate::function_tool::FunctionCallError; use crate::mcp_tool_call::handle_mcp_tool_call; -use crate::tools::context::FunctionToolOutput; -use crate::tools::context::McpToolOutput; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolPayload; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; -use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::McpToolOutput; pub struct McpHandler; - -pub enum McpHandlerOutput { - Mcp(McpToolOutput), - Function(FunctionToolOutput), -} - -impl crate::tools::context::ToolOutput for McpHandlerOutput { - fn log_preview(&self) -> String { - match self { - Self::Mcp(output) => output.log_preview(), - Self::Function(output) => output.log_preview(), - } - } - - fn success_for_logging(&self) -> bool { - match self { - Self::Mcp(output) => output.success_for_logging(), - Self::Function(output) => output.success_for_logging(), - } - } - - fn to_response_item(&self, call_id: &str, payload: &ToolPayload) -> ResponseInputItem { - match self { - Self::Mcp(output) => output.to_response_item(call_id, payload), - Self::Function(output) => output.to_response_item(call_id, payload), - } - } -} - #[async_trait] impl ToolHandler for McpHandler { - type Output = McpHandlerOutput; + type Output = McpToolOutput; fn kind(&self) -> ToolKind { ToolKind::Mcp @@ -74,7 +43,7 @@ impl ToolHandler for McpHandler { let (server, tool, raw_arguments) = payload; let arguments_str = raw_arguments; - let response = handle_mcp_tool_call( + let output = handle_mcp_tool_call( Arc::clone(&session), &turn, call_id.clone(), @@ -84,26 +53,6 @@ impl ToolHandler for McpHandler { ) .await; - match response { - ResponseInputItem::McpToolCallOutput { result, .. } => { - Ok(McpHandlerOutput::Mcp(McpToolOutput { result })) - } - ResponseInputItem::FunctionCallOutput { output, .. } => { - let success = output.success; - match output.body { - codex_protocol::models::FunctionCallOutputBody::Text(text) => Ok( - McpHandlerOutput::Function(FunctionToolOutput::from_text(text, success)), - ), - codex_protocol::models::FunctionCallOutputBody::ContentItems(content) => { - Ok(McpHandlerOutput::Function( - FunctionToolOutput::from_content(content, success), - )) - } - } - } - _ => Err(FunctionCallError::RespondToModel( - "mcp handler received unexpected response variant".to_string(), - )), - } + Ok(output) } } diff --git a/codex-rs/core/src/tools/js_repl/mod.rs b/codex-rs/core/src/tools/js_repl/mod.rs index bc2a5342c..4ffb92518 100644 --- a/codex-rs/core/src/tools/js_repl/mod.rs +++ b/codex-rs/core/src/tools/js_repl/mod.rs @@ -620,29 +620,23 @@ impl JsReplManager { output, ) } - ResponseInputItem::McpToolCallOutput { result, .. } => match result { - Ok(result) => { - let output = FunctionCallOutputPayload::from(result); - let mut summary = Self::summarize_function_output_payload( - "mcp_tool_call_output", - JsReplToolCallPayloadKind::McpResult, - &output, - ); - summary.payload_item_count = Some(result.content.len()); - summary.structured_content_present = Some(result.structured_content.is_some()); - summary.result_is_error = Some(result.is_error.unwrap_or(false)); - summary - } - Err(error) => { - let mut summary = Self::summarize_text_payload( - Some("mcp_tool_call_output"), - JsReplToolCallPayloadKind::McpErrorResult, - error, - ); - summary.result_is_error = Some(true); - summary - } - }, + ResponseInputItem::McpToolCallOutput { output, .. } => { + let function_output = output.as_function_call_output_payload(); + let payload_kind = if output.success { + JsReplToolCallPayloadKind::McpResult + } else { + JsReplToolCallPayloadKind::McpErrorResult + }; + let mut summary = Self::summarize_function_output_payload( + "mcp_tool_call_output", + payload_kind, + &function_output, + ); + summary.payload_item_count = Some(output.content.len()); + summary.structured_content_present = Some(output.structured_content.is_some()); + summary.result_is_error = Some(!output.success); + summary + } } } diff --git a/codex-rs/core/src/tools/parallel.rs b/codex-rs/core/src/tools/parallel.rs index a37c93db9..e64597675 100644 --- a/codex-rs/core/src/tools/parallel.rs +++ b/codex-rs/core/src/tools/parallel.rs @@ -124,7 +124,9 @@ impl ToolCallRuntime { }, ToolPayload::Mcp { .. } => ResponseInputItem::McpToolCallOutput { call_id: call.call_id.clone(), - result: Err(Self::abort_message(call, secs)), + output: codex_protocol::models::McpToolOutput::from_error_text( + Self::abort_message(call, secs), + ), }, _ => ResponseInputItem::FunctionCallOutput { call_id: call.call_id.clone(), diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index f8948a772..ca15ea951 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -206,7 +206,7 @@ pub enum ResponseInputItem { }, McpToolCallOutput { call_id: String, - result: Result, + output: McpToolOutput, }, CustomToolCallOutput { call_id: String, @@ -843,14 +843,8 @@ impl From for ResponseItem { ResponseInputItem::FunctionCallOutput { call_id, output } => { Self::FunctionCallOutput { call_id, output } } - ResponseInputItem::McpToolCallOutput { call_id, result } => { - let output = match result { - Ok(result) => FunctionCallOutputPayload::from(&result), - Err(tool_call_err) => FunctionCallOutputPayload { - body: FunctionCallOutputBody::Text(format!("err: {tool_call_err:?}")), - success: Some(false), - }, - }; + ResponseInputItem::McpToolCallOutput { call_id, output } => { + let output = output.into_function_call_output_payload(); Self::FunctionCallOutput { call_id, output } } ResponseInputItem::CustomToolCallOutput { call_id, output } => { @@ -1190,25 +1184,59 @@ impl<'de> Deserialize<'de> for FunctionCallOutputPayload { } } -impl From<&CallToolResult> for FunctionCallOutputPayload { - fn from(call_tool_result: &CallToolResult) -> Self { - let CallToolResult { +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema, TS)] +#[serde(rename_all = "camelCase")] +pub struct McpToolOutput { + pub content: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub structured_content: Option, + pub success: bool, +} + +impl McpToolOutput { + pub fn from_result(result: Result) -> Self { + match result { + Ok(result) => Self::from(&result), + Err(error) => Self::from_error_text(error), + } + } + + pub fn from_error_text(text: String) -> Self { + Self { + content: vec![serde_json::json!({ + "type": "text", + "text": text, + })], + structured_content: None, + success: false, + } + } + + pub fn into_call_tool_result(self) -> CallToolResult { + let Self { content, structured_content, - is_error, - meta: _, - } = call_tool_result; + success, + } = self; - let is_success = is_error != &Some(true); + CallToolResult { + content, + structured_content, + is_error: Some(!success), + meta: None, + } + } - if let Some(structured_content) = structured_content + pub fn as_function_call_output_payload(&self) -> FunctionCallOutputPayload { + if let Some(structured_content) = &self.structured_content && !structured_content.is_null() { match serde_json::to_string(structured_content) { Ok(serialized_structured_content) => { return FunctionCallOutputPayload { body: FunctionCallOutputBody::Text(serialized_structured_content), - success: Some(is_success), + success: Some(self.success), }; } Err(err) => { @@ -1220,7 +1248,7 @@ impl From<&CallToolResult> for FunctionCallOutputPayload { } } - let serialized_content = match serde_json::to_string(content) { + let serialized_content = match serde_json::to_string(&self.content) { Ok(serialized_content) => serialized_content, Err(err) => { return FunctionCallOutputPayload { @@ -1230,7 +1258,7 @@ impl From<&CallToolResult> for FunctionCallOutputPayload { } }; - let content_items = convert_mcp_content_to_items(content); + let content_items = convert_mcp_content_to_items(&self.content); let body = match content_items { Some(content_items) => FunctionCallOutputBody::ContentItems(content_items), @@ -1239,7 +1267,28 @@ impl From<&CallToolResult> for FunctionCallOutputPayload { FunctionCallOutputPayload { body, - success: Some(is_success), + success: Some(self.success), + } + } + + pub fn into_function_call_output_payload(self) -> FunctionCallOutputPayload { + self.as_function_call_output_payload() + } +} + +impl From<&CallToolResult> for McpToolOutput { + fn from(call_tool_result: &CallToolResult) -> Self { + let CallToolResult { + content, + structured_content, + is_error, + meta: _, + } = call_tool_result; + + Self { + content: content.clone(), + structured_content: structured_content.clone(), + success: is_error != &Some(true), } } } @@ -1833,7 +1882,7 @@ mod tests { meta: None, }; - let payload = FunctionCallOutputPayload::from(&call_tool_result); + let payload = McpToolOutput::from(&call_tool_result).into_function_call_output_payload(); assert_eq!(payload.success, Some(true)); let Some(items) = payload.content_items() else { panic!("expected content items"); @@ -1900,7 +1949,7 @@ mod tests { meta: None, }; - let payload = FunctionCallOutputPayload::from(&call_tool_result); + let payload = McpToolOutput::from(&call_tool_result).into_function_call_output_payload(); let Some(items) = payload.content_items() else { panic!("expected content items"); };