mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Revert to 4428986e (tests) by reverting subsequent commits
This commit is contained in:
Ahmed Ibrahim
@@ -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<ResponseItem> = 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::<Vec<_>>();
|
||||
|
||||
// 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::<Vec<_>>();
|
||||
|
||||
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::<Vec<_>>()
|
||||
};
|
||||
self.record_items(missing_calls, task_kind);
|
||||
}
|
||||
|
||||
pub(crate) fn prompt(&self, task_kind: TaskKind) -> Vec<ResponseItem> {
|
||||
@@ -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.
|
||||
}
|
||||
|
||||
@@ -60,12 +60,10 @@ impl SessionState {
|
||||
task_kind: TaskKind,
|
||||
pending_input: Vec<ResponseItem>,
|
||||
) -> Vec<ResponseItem> {
|
||||
// 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)
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user