From 8ab406300cdc89fb7650cdae05b3c053e3a8c07c Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Thu, 16 Oct 2025 08:07:14 -0700 Subject: [PATCH] Revert to 4428986e (tests) by reverting subsequent commits --- codex-rs/core/src/conversation_history.rs | 154 ++++++---------------- codex-rs/core/src/state/session.rs | 4 +- 2 files changed, 38 insertions(+), 120 deletions(-) diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index baee62aa26..3d45006997 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -1,10 +1,5 @@ use codex_protocol::models::ResponseInputItem; use codex_protocol::models::ResponseItem; -use std::sync::atomic::AtomicU64; -use std::sync::atomic::Ordering; -use tracing::warn; - -static SYNTHETIC_TOOL_OUTPUTS: AtomicU64 = AtomicU64::new(0); use crate::state::TaskKind; @@ -84,77 +79,50 @@ impl ConversationHistory { } pub(crate) fn handle_missing_tool_call_output(&mut self, task_kind: TaskKind) { - // Select the appropriate thread's history vector. - let thread_vec: &mut Vec = match task_kind { - TaskKind::Regular | TaskKind::Compact => &mut self.items, - TaskKind::Review => &mut self.review_thread_history, + // call_ids that are part of this response. + let content = match task_kind { + TaskKind::Regular => self.contents(), + TaskKind::Review => self.review_thread_contents(), + TaskKind::Compact => self.contents(), }; - - // Determine which calls are already completed. - let completed_call_ids = thread_vec + let completed_call_ids = content .iter() .filter_map(|ri| match ri { - ResponseItem::FunctionCallOutput { call_id, .. } => Some(call_id.clone()), - ResponseItem::CustomToolCallOutput { call_id, .. } => Some(call_id.clone()), + ResponseItem::FunctionCallOutput { call_id, .. } => Some(call_id), + ResponseItem::CustomToolCallOutput { call_id, .. } => Some(call_id), _ => None, }) .collect::>(); - // Find tool calls that are missing outputs and insert synthetic outputs - // immediately after the corresponding call so the call appears resolved - // before any subsequent user input is processed. - // We re-scan the vector for each missing call id so insertion indices stay valid. - let pending_call_ids = thread_vec - .iter() - .filter_map(|ri| match ri { - ResponseItem::FunctionCall { call_id, .. } => Some(call_id.clone()), - ResponseItem::LocalShellCall { - call_id: Some(call_id), - .. - } => Some(call_id.clone()), - ResponseItem::CustomToolCall { call_id, .. } => Some(call_id.clone()), - _ => None, - }) - .collect::>(); - - for call_id in pending_call_ids { - if completed_call_ids.iter().any(|c| c == &call_id) { - continue; - } - - // Find the last index of the corresponding call in the current vector. - if let Some(idx) = thread_vec.iter().rposition(|ri| match ri { - ResponseItem::FunctionCall { call_id: id, .. } => id == &call_id, - ResponseItem::LocalShellCall { - call_id: Some(id), .. - } => id == &call_id, - ResponseItem::CustomToolCall { call_id: id, .. } => id == &call_id, - _ => false, - }) { - // Optionally fail fast in CI/dev to surface original issues. - if std::env::var("CODEX_FAIL_ON_SYNTHETIC_TOOL_OUTPUT") - .map_or(false, |v| v == "1" || v.eq_ignore_ascii_case("true")) - { - panic!("Synthetic tool output inserted for unresolved call_id={call_id}"); - } - - // Log a warning to help developers discover where/when this happens. - warn!( - %call_id, - thread = ?task_kind, - "Inserting synthetic tool output for unresolved tool call" - ); - - SYNTHETIC_TOOL_OUTPUTS.fetch_add(1, Ordering::Relaxed); - thread_vec.insert( - idx + 1, - ResponseItem::CustomToolCallOutput { - call_id: call_id.clone(), - output: "aborted".to_string(), - }, - ); - } - } + // call_ids that were pending but are not part of this response. + // This usually happens because the user interrupted the model before we responded to one of its tool calls + // and then the user sent a follow-up message. + let missing_calls = { + content + .iter() + .filter_map(|ri| match ri { + ResponseItem::FunctionCall { call_id, .. } => Some(call_id), + ResponseItem::LocalShellCall { + call_id: Some(call_id), + .. + } => Some(call_id), + ResponseItem::CustomToolCall { call_id, .. } => Some(call_id), + _ => None, + }) + .filter_map(|call_id| { + if completed_call_ids.contains(&call_id) { + None + } else { + Some(call_id.clone()) + } + }) + .map(|call_id| ResponseItem::CustomToolCallOutput { + call_id, + output: "aborted".to_string(), + }) + .collect::>() + }; + self.record_items(missing_calls, task_kind); } pub(crate) fn prompt(&self, task_kind: TaskKind) -> Vec { @@ -185,7 +153,6 @@ fn is_api_message(message: &ResponseItem) -> bool { mod tests { use super::*; use codex_protocol::models::ContentItem; - use pretty_assertions::assert_eq; fn assistant_msg(text: &str) -> ResponseItem { ResponseItem::Message { @@ -246,51 +213,4 @@ mod tests { ], ); } - - #[test] - fn inserts_synthetic_tool_output_next_to_call_before_user_input() { - let mut h = ConversationHistory::default(); - - // Record a pending tool call without an output. - let call_id = "call-1".to_string(); - let tool_call = ResponseItem::FunctionCall { - id: None, - name: "shell".to_string(), - arguments: "{\"command\":[\"echo\",\"hi\"]}".to_string(), - call_id: call_id.clone(), - }; - h.record_items([tool_call], TaskKind::Regular); - - // Ensure missing outputs are handled first. - h.handle_missing_tool_call_output(TaskKind::Regular); - - // Then record a new user message. - let user = user_msg("follow up"); - h.add_pending_input(vec![user.clone()], TaskKind::Regular); - - // Expect the synthetic output to be inserted immediately after the call - // and before the user message. - let items = h.contents(); - assert_eq!(items.len(), 3); - match (&items[0], &items[1], &items[2]) { - ( - ResponseItem::FunctionCall { call_id: id0, .. }, - ResponseItem::CustomToolCallOutput { - call_id: id1, - output, - }, - ResponseItem::Message { role, .. }, - ) => { - assert_eq!(id0, &call_id); - assert_eq!(id1, &call_id); - assert_eq!(output, "aborted"); - assert_eq!(role, "user"); - } - _ => panic!("unexpected item ordering: {items:?}"), - } - } - - // Intentionally not testing the env-based panic path here: mutating - // environment variables is unsafe in this edition and risks test - // interference. The behavior is exercised indirectly by integration tests. } diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index f9f7eb8c58..ff3b5fe094 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -60,12 +60,10 @@ impl SessionState { task_kind: TaskKind, pending_input: Vec, ) -> Vec { - // Ensure any missing tool-call outputs are recorded before - // accepting new pending input to preserve correct ordering. - self.history.handle_missing_tool_call_output(task_kind); if !pending_input.is_empty() { self.history.add_pending_input(pending_input, task_kind); } + self.history.handle_missing_tool_call_output(task_kind); self.history.prompt(task_kind) }