mirror of
https://github.com/openai/codex.git
synced 2026-10-21 12:10:06 +00:00
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
This commit is contained in:
@@ -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:
|
||||
|
||||
@@ -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![
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user