From 79f4124533e2d57d39a1c4e0f50d184d7a96c72b Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim Date: Wed, 15 Oct 2025 15:59:34 -0700 Subject: [PATCH] feedback --- codex-rs/core/src/conversation_history.rs | 130 ++++++++++++++++------ codex-rs/core/src/state/session.rs | 4 +- 2 files changed, 96 insertions(+), 38 deletions(-) diff --git a/codex-rs/core/src/conversation_history.rs b/codex-rs/core/src/conversation_history.rs index 3d45006997..9c2c03275e 100644 --- a/codex-rs/core/src/conversation_history.rs +++ b/codex-rs/core/src/conversation_history.rs @@ -79,50 +79,62 @@ impl ConversationHistory { } pub(crate) fn handle_missing_tool_call_output(&mut self, task_kind: TaskKind) { - // 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(), + // 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, }; - let completed_call_ids = content + + // Determine which calls are already completed. + let completed_call_ids = thread_vec .iter() .filter_map(|ri| match ri { - ResponseItem::FunctionCallOutput { call_id, .. } => Some(call_id), - ResponseItem::CustomToolCallOutput { call_id, .. } => Some(call_id), + ResponseItem::FunctionCallOutput { call_id, .. } => Some(call_id.clone()), + ResponseItem::CustomToolCallOutput { call_id, .. } => Some(call_id.clone()), _ => None, }) .collect::>(); - // 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); + // 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, + }) { + thread_vec.insert( + idx + 1, + ResponseItem::CustomToolCallOutput { + call_id: call_id.clone(), + output: "aborted".to_string(), + }, + ); + } + } } pub(crate) fn prompt(&self, task_kind: TaskKind) -> Vec { @@ -153,6 +165,7 @@ 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 { @@ -213,4 +226,47 @@ 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:?}"), + } + } } diff --git a/codex-rs/core/src/state/session.rs b/codex-rs/core/src/state/session.rs index ff3b5fe094..f9f7eb8c58 100644 --- a/codex-rs/core/src/state/session.rs +++ b/codex-rs/core/src/state/session.rs @@ -60,10 +60,12 @@ 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) }