From 8431dc590a5bba9a1185d5579a5aabfbc469e50b Mon Sep 17 00:00:00 2001 From: rka-oai Date: Mon, 20 Jul 2026 16:52:12 +0000 Subject: [PATCH] Stop retrying turns with invalid tool images (#34380) ## What changed Remove the fallback that replaced image content in the latest tool output with `Invalid image` and retried the model request. Invalid-image responses now use the existing bad-request error path without modifying conversation history. GitOrigin-RevId: f4ea782f16c5451feb261694caa73b77f24c3e4e --- codex-rs/core/src/context_manager/history.rs | 34 -------- .../core/src/context_manager/history_tests.rs | 62 -------------- codex-rs/core/src/session/turn.rs | 10 --- codex-rs/core/tests/suite/view_image.rs | 80 ------------------- 4 files changed, 186 deletions(-) diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index 464a94cbed..785fd0ac7a 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -202,40 +202,6 @@ impl ContextManager { self.world_state_baseline = None; } - /// Replace image content in the last turn if it originated from a tool output. - /// Returns true when a tool image was replaced, false otherwise. - pub(crate) fn replace_last_turn_images(&mut self, placeholder: &str) -> bool { - let Some(index) = self.items.iter().rposition(|item| { - matches!(item, ResponseItem::FunctionCallOutput { .. }) || is_user_turn_boundary(item) - }) else { - return false; - }; - - match &mut self.items[index] { - ResponseItem::FunctionCallOutput { output, .. } => { - let Some(content_items) = output.content_items_mut() else { - return false; - }; - let mut replaced = false; - let placeholder = placeholder.to_string(); - for item in content_items.iter_mut() { - if matches!(item, FunctionCallOutputContentItem::InputImage { .. }) { - *item = FunctionCallOutputContentItem::InputText { - text: placeholder.clone(), - }; - replaced = true; - } - } - if replaced { - self.history_version = self.history_version.saturating_add(1); - } - replaced - } - ResponseItem::Message { .. } => false, - _ => false, - } - } - /// Drop the last `num_turns` instruction turns from this history. /// /// Instruction turns are history messages that should behave like a new prompt boundary: diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index 69bb232b02..3c934f2775 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -815,68 +815,6 @@ fn remove_first_item_removes_matching_call_for_output() { assert_eq!(h.raw_items(), vec![]); } -#[test] -fn replace_last_turn_images_replaces_tool_output_images() { - let items = vec![ - user_input_text_msg("hi"), - ResponseItem::FunctionCallOutput { - id: None, - call_id: "call-1".to_string(), - output: FunctionCallOutputPayload { - body: FunctionCallOutputBody::ContentItems(vec![ - FunctionCallOutputContentItem::InputImage { - image_url: "data:image/png;base64,AAA".to_string(), - detail: Some(DEFAULT_IMAGE_DETAIL), - }, - ]), - success: Some(true), - }, - internal_chat_message_metadata_passthrough: None, - }, - ]; - let mut history = create_history_with_items(items); - - assert!(history.replace_last_turn_images("Invalid image")); - - assert_eq!( - history.raw_items(), - vec![ - user_input_text_msg("hi"), - ResponseItem::FunctionCallOutput { - id: None, - call_id: "call-1".to_string(), - output: FunctionCallOutputPayload { - body: FunctionCallOutputBody::ContentItems(vec![ - FunctionCallOutputContentItem::InputText { - text: "Invalid image".to_string(), - }, - ]), - success: Some(true), - }, - internal_chat_message_metadata_passthrough: None, - }, - ] - ); -} - -#[test] -fn replace_last_turn_images_does_not_touch_user_images() { - let items = vec![ResponseItem::Message { - id: None, - role: "user".to_string(), - content: vec![ContentItem::InputImage { - image_url: "data:image/png;base64,AAA".to_string(), - detail: Some(DEFAULT_IMAGE_DETAIL), - }], - phase: None, - internal_chat_message_metadata_passthrough: None, - }]; - let mut history = create_history_with_items(items.clone()); - - assert!(!history.replace_last_turn_images("Invalid image")); - assert_eq!(history.raw_items(), items); -} - #[test] fn remove_first_item_handles_local_shell_pair() { let items = vec![ diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 7da4c18693..efb3e61837 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -432,16 +432,6 @@ pub(crate) async fn run_turn( return Err(err); } Err(codex_error @ CodexErr::InvalidImageRequest()) => { - { - let mut state = sess.state.lock().await; - error_or_panic( - "Invalid image detected; sanitizing tool output to prevent poisoning", - ); - if state.history.replace_last_turn_images("Invalid image") { - continue; - } - } - sess.track_turn_codex_error(turn_context.as_ref(), &codex_error); let error = CodexErrorInfo::BadRequest; sess.emit_turn_error_lifecycle(turn_context.as_ref(), error.clone()) diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index 097237ec44..ab567f9845 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -63,10 +63,6 @@ use tempfile::TempDir; use tokio::time::Duration; use wiremock::BodyPrintLimit; use wiremock::MockServer; -#[cfg(not(debug_assertions))] -use wiremock::ResponseTemplate; -#[cfg(not(debug_assertions))] -use wiremock::matchers::body_string_contains; const VIEW_IMAGE_TURN_COMPLETE_TIMEOUT: Duration = Duration::from_secs(30); @@ -1464,79 +1460,3 @@ async fn view_image_tool_returns_unsupported_message_for_text_only_model() -> an Ok(()) } - -#[cfg(not(debug_assertions))] -#[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn replaces_invalid_local_image_after_bad_request() -> anyhow::Result<()> { - skip_if_no_network!(Ok(())); - - let server = start_mock_server().await; - - const INVALID_IMAGE_ERROR: &str = - "The image data you provided does not represent a valid image"; - - let invalid_image_mock = responses::mount_response_once_match( - &server, - body_string_contains("\"input_image\""), - ResponseTemplate::new(400) - .insert_header("content-type", "text/plain") - .set_body_string(INVALID_IMAGE_ERROR), - ) - .await; - - let success_response = sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", "done"), - ev_completed("resp-2"), - ]); - - let completion_mock = responses::mount_sse_once(&server, success_response).await; - - let mut builder = test_codex(); - let test = builder.build_with_auto_env(&server).await?; - let TestCodex { - codex, - session_configured, - .. - } = &test; - - let rel_path = "assets/poisoned.png"; - let abs_path = write_workspace_png(&test, rel_path, 1024, 512, [10u8, 20, 30, 255]).await?; - - let session_model = session_configured.model.clone(); - - codex - .submit(disabled_user_turn( - &test, - vec![UserInput::LocalImage { - path: abs_path.clone(), - detail: None, - }], - session_model, - )) - .await?; - - wait_for_event_with_timeout( - &codex, - |event| matches!(event, EventMsg::TurnComplete(_)), - VIEW_IMAGE_TURN_COMPLETE_TIMEOUT, - ) - .await; - - let first_body = invalid_image_mock.single_request().body_json(); - assert!( - find_image_message(&first_body).is_some(), - "initial request should include the uploaded image" - ); - - let second_request = completion_mock.single_request(); - let second_body = second_request.body_json(); - assert!( - find_image_message(&second_body).is_none(), - "second request should replace the invalid image" - ); - let user_texts = second_request.message_input_texts("user"); - assert!(user_texts.iter().any(|text| text == "Invalid image")); - - Ok(()) -}