From e23cbcaaf6d9297b6a3f1f02f776ed7b9aa364d1 Mon Sep 17 00:00:00 2001 From: jimmyfraiture Date: Wed, 1 Oct 2025 12:37:08 +0100 Subject: [PATCH] Fix regression 1 --- codex-rs/core/src/codex.rs | 10 ++++++---- codex-rs/core/src/executor/runner.rs | 4 ++-- codex-rs/core/src/tools/context.rs | 18 +++++++++++------- .../core/src/tools/handlers/apply_patch.rs | 2 +- .../core/src/tools/handlers/exec_stream.rs | 2 +- codex-rs/core/src/tools/handlers/mcp.rs | 7 ++----- codex-rs/core/src/tools/handlers/plan.rs | 2 +- codex-rs/core/src/tools/handlers/read_file.rs | 2 +- codex-rs/core/src/tools/handlers/shell.rs | 4 ++-- .../core/src/tools/handlers/unified_exec.rs | 2 +- codex-rs/core/src/tools/handlers/view_image.rs | 2 +- codex-rs/core/src/tools/registry.rs | 3 ++- codex-rs/otel/src/otel_event_manager.rs | 12 +++++++----- 13 files changed, 38 insertions(+), 32 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index d3e28eb599..5884c6d4b6 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -51,7 +51,9 @@ use crate::exec::StreamOutput; use crate::exec_command::ExecCommandParams; use crate::exec_command::ExecSessionManager; use crate::exec_command::WriteStdinParams; -use crate::executor::{normalize_exec_result, Executor, ExecutorConfig}; +use crate::executor::Executor; +use crate::executor::ExecutorConfig; +use crate::executor::normalize_exec_result; use crate::mcp_connection_manager::McpConnectionManager; use crate::model_family::find_family_for_model; use crate::openai_model_info::get_model_info; @@ -2353,10 +2355,10 @@ pub(crate) async fn exit_review_mode( .await; } -use crate::tools::context::ApplyPatchCommandContext; -use crate::tools::context::ExecCommandContext; use crate::executor::errors::ExecError; use crate::executor::linkers::PreparedExec; +use crate::tools::context::ApplyPatchCommandContext; +use crate::tools::context::ExecCommandContext; #[cfg(test)] pub(crate) use tests::make_session_and_context; @@ -2372,12 +2374,12 @@ mod tests { use crate::state::TaskKind; use crate::tasks::SessionTask; use crate::tasks::SessionTaskContext; - use codex_app_server_protocol::AuthMode; use crate::tools::MODEL_FORMAT_HEAD_LINES; use crate::tools::MODEL_FORMAT_MAX_BYTES; use crate::tools::MODEL_FORMAT_MAX_LINES; use crate::tools::MODEL_FORMAT_TAIL_LINES; use crate::tools::handle_container_exec_with_params; + use codex_app_server_protocol::AuthMode; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; diff --git a/codex-rs/core/src/executor/runner.rs b/codex-rs/core/src/executor/runner.rs index 0ff0df2390..75c4d9b4f9 100644 --- a/codex-rs/core/src/executor/runner.rs +++ b/codex-rs/core/src/executor/runner.rs @@ -6,7 +6,7 @@ use std::time::Duration; use super::backends::ExecutionMode; use super::backends::backend_for_mode; use super::cache::ApprovalCache; -use crate::codex::{Session}; +use crate::codex::Session; use crate::error::CodexErr; use crate::error::SandboxErr; use crate::error::get_error_message_ui; @@ -23,8 +23,8 @@ use crate::protocol::AskForApproval; use crate::protocol::ReviewDecision; use crate::protocol::SandboxPolicy; use crate::shell; -use codex_otel::otel_event_manager::ToolDecisionSource; use crate::tools::context::ExecCommandContext; +use codex_otel::otel_event_manager::ToolDecisionSource; #[derive(Clone, Debug)] pub(crate) struct ExecutorConfig { diff --git a/codex-rs/core/src/tools/context.rs b/codex-rs/core/src/tools/context.rs index a8d6cc2a9e..43d25aa1f5 100644 --- a/codex-rs/core/src/tools/context.rs +++ b/codex-rs/core/src/tools/context.rs @@ -59,7 +59,7 @@ impl ToolPayload { pub enum ToolOutput { Function { content: String, - success: bool, + success: Option, }, Mcp { result: Result, @@ -74,6 +74,13 @@ impl ToolOutput { } } + pub fn success_for_logging(&self) -> bool { + match self { + ToolOutput::Function { success, .. } => success.unwrap_or(true), + ToolOutput::Mcp { result } => result.is_ok(), + } + } + pub fn into_response(self, call_id: &str, payload: &ToolPayload) -> ResponseInputItem { match self { ToolOutput::Function { content, success } => { @@ -85,10 +92,7 @@ impl ToolOutput { } else { ResponseInputItem::FunctionCallOutput { call_id: call_id.to_string(), - output: FunctionCallOutputPayload { - content, - success: Some(success), - }, + output: FunctionCallOutputPayload { content, success }, } } } @@ -112,7 +116,7 @@ mod tests { }; let response = ToolOutput::Function { content: "patched".to_string(), - success: true, + success: Some(true), } .into_response("call-42", &payload); @@ -132,7 +136,7 @@ mod tests { }; let response = ToolOutput::Function { content: "ok".to_string(), - success: true, + success: Some(true), } .into_response("fn-1", &payload); diff --git a/codex-rs/core/src/tools/handlers/apply_patch.rs b/codex-rs/core/src/tools/handlers/apply_patch.rs index 02852edfb7..857baf0de1 100644 --- a/codex-rs/core/src/tools/handlers/apply_patch.rs +++ b/codex-rs/core/src/tools/handlers/apply_patch.rs @@ -79,7 +79,7 @@ impl ToolHandler for ApplyPatchHandler { Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/handlers/exec_stream.rs b/codex-rs/core/src/tools/handlers/exec_stream.rs index d931d3ddbd..db9d4b0b2b 100644 --- a/codex-rs/core/src/tools/handlers/exec_stream.rs +++ b/codex-rs/core/src/tools/handlers/exec_stream.rs @@ -65,7 +65,7 @@ impl ToolHandler for ExecStreamHandler { Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 8379915aaa..7c533fbbd7 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -60,11 +60,8 @@ impl ToolHandler for McpHandler { Ok(ToolOutput::Mcp { result }) } codex_protocol::models::ResponseInputItem::FunctionCallOutput { output, .. } => { - let success = output.success.unwrap_or(false); - Ok(ToolOutput::Function { - content: output.content, - success, - }) + let codex_protocol::models::FunctionCallOutputPayload { content, success } = output; + Ok(ToolOutput::Function { content, success }) } _ => Err(FunctionCallError::RespondToModel( "mcp handler received unexpected response variant".to_string(), diff --git a/codex-rs/core/src/tools/handlers/plan.rs b/codex-rs/core/src/tools/handlers/plan.rs index f1dc584177..3ef89ea548 100644 --- a/codex-rs/core/src/tools/handlers/plan.rs +++ b/codex-rs/core/src/tools/handlers/plan.rs @@ -41,7 +41,7 @@ impl ToolHandler for PlanHandler { Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/handlers/read_file.rs b/codex-rs/core/src/tools/handlers/read_file.rs index c14aed840a..3589a2b793 100644 --- a/codex-rs/core/src/tools/handlers/read_file.rs +++ b/codex-rs/core/src/tools/handlers/read_file.rs @@ -91,7 +91,7 @@ impl ToolHandler for ReadFileHandler { let collected = read_file_slice(&path, offset, limit).await?; Ok(ToolOutput::Function { content: collected.join("\n"), - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 907f193fe1..fbcb493e24 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -75,7 +75,7 @@ impl ToolHandler for ShellHandler { .await?; Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } ToolPayload::LocalShell { params } => { @@ -92,7 +92,7 @@ impl ToolHandler for ShellHandler { .await?; Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } _ => Err(FunctionCallError::RespondToModel(format!( diff --git a/codex-rs/core/src/tools/handlers/unified_exec.rs b/codex-rs/core/src/tools/handlers/unified_exec.rs index 4afd7d3b83..7175afb99d 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec.rs @@ -106,7 +106,7 @@ impl ToolHandler for UnifiedExecHandler { Ok(ToolOutput::Function { content, - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index b525004a1f..b7846db961 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -58,7 +58,7 @@ impl ToolHandler for ViewImageHandler { Ok(ToolOutput::Function { content: "attached local image path".to_string(), - success: true, + success: Some(true), }) } } diff --git a/codex-rs/core/src/tools/registry.rs b/codex-rs/core/src/tools/registry.rs index dc8e21d23b..8ec4125495 100644 --- a/codex-rs/core/src/tools/registry.rs +++ b/codex-rs/core/src/tools/registry.rs @@ -108,11 +108,12 @@ impl ToolRegistry { match handler.handle(invocation).await { Ok(output) => { let preview = output.log_preview(); + let success = output.success_for_logging(); let mut guard = output_cell .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); *guard = Some(output); - Ok(preview) + Ok((preview, success)) } Err(err) => Err(err), } diff --git a/codex-rs/otel/src/otel_event_manager.rs b/codex-rs/otel/src/otel_event_manager.rs index 3e2ffeb7af..3baf7c86e7 100644 --- a/codex-rs/otel/src/otel_event_manager.rs +++ b/codex-rs/otel/src/otel_event_manager.rs @@ -366,10 +366,10 @@ impl OtelEventManager { call_id: &str, arguments: &str, f: F, - ) -> Result + ) -> Result<(String, bool), E> where F: FnOnce() -> Fut, - Fut: Future>, + Fut: Future>, E: Display, { let start = Instant::now(); @@ -377,10 +377,12 @@ impl OtelEventManager { let duration = start.elapsed(); let (output, success) = match &result { - Ok(content) => (content, true), - Err(error) => (&error.to_string(), false), + Ok((preview, success)) => (preview.clone(), *success), + Err(error) => (error.to_string(), false), }; + let success_str = if success { "true" } else { "false" }; + tracing::event!( tracing::Level::INFO, event.name = "codex.tool_result", @@ -396,7 +398,7 @@ impl OtelEventManager { call_id = %call_id, arguments = %arguments, duration_ms = %duration.as_millis(), - success = %success, + success = %success_str, output = %output, );