mirror of
https://github.com/openai/codex.git
synced 2026-10-20 08:19:50 +00:00
Preserve content kinds during image preparation (#40281)
## Why Image preparation can rewrite message content, including replacing images that cannot be processed with model-visible error text. The positional content-kind metadata must stay aligned with those rewrites. ## What changed - Process message images together with their content-kind annotations. - Preserve existing kinds for unchanged and resized content. - Classify failed-image placeholders as `images.preparation_error`. ## Testing - Cover metadata preservation across successful, resized, and failed images. - Verify Responses Lite sends `user.image` and `images.preparation_error` kinds. GitOrigin-RevId: 1be7bcbe47014251019bfff0d826c9dfe3239dec
This commit is contained in:
@@ -1511,9 +1511,23 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() {
|
||||
);
|
||||
let history = child_thread.session.clone_history().await;
|
||||
let history_items = history.raw_items().cloned().collect::<Vec<_>>();
|
||||
let mut expected_final_answer =
|
||||
assistant_message("parent final answer", Some(MessagePhase::FinalAnswer));
|
||||
expected_final_answer.set_turn_id_if_missing(&turn_context.sub_id);
|
||||
let expected_final_answer = parent_thread
|
||||
.session
|
||||
.clone_history()
|
||||
.await
|
||||
.raw_items()
|
||||
.find(|item| {
|
||||
matches!(
|
||||
item,
|
||||
ResponseItem::Message {
|
||||
role,
|
||||
phase: Some(MessagePhase::FinalAnswer),
|
||||
..
|
||||
} if role == "assistant"
|
||||
)
|
||||
})
|
||||
.cloned()
|
||||
.expect("parent final answer should be recorded");
|
||||
let mut expected_developer_message = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "developer".to_string(),
|
||||
|
||||
@@ -6,8 +6,12 @@ use crate::context::ResizedImage;
|
||||
use crate::original_image_detail::can_request_original_image_detail;
|
||||
use codex_analytics::ImageDetailSetting;
|
||||
use codex_analytics::ImagePreparationMetadata;
|
||||
use codex_context_fragments::AnnotatedContent;
|
||||
use codex_context_fragments::set_annotated_content;
|
||||
use codex_context_fragments::to_annotated_content;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::ContentItemKind;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ImageDetail;
|
||||
@@ -121,15 +125,19 @@ pub(crate) fn prepare_response_items(
|
||||
})
|
||||
};
|
||||
for mut item in std::mem::take(items) {
|
||||
let mut annotated_content = to_annotated_content(&mut item);
|
||||
let resize_notice = match &mut item {
|
||||
ResponseItem::Message { role, content, .. } => {
|
||||
ResponseItem::Message { role, .. } => {
|
||||
let Some(mut content) = annotated_content.take() else {
|
||||
continue;
|
||||
};
|
||||
let resized_images = prepare_message_content(
|
||||
content,
|
||||
&mut content,
|
||||
ImageOrigin {
|
||||
message_role: Some(role),
|
||||
message_role: Some(role.as_str()),
|
||||
item_id: None,
|
||||
},
|
||||
if role == "user" {
|
||||
if role.as_str() == "user" {
|
||||
resize_notice_mode
|
||||
} else {
|
||||
ImageResizeNoticeMode::Disabled
|
||||
@@ -137,6 +145,7 @@ pub(crate) fn prepare_response_items(
|
||||
&mut metadata,
|
||||
mode,
|
||||
);
|
||||
let _ = set_annotated_content(&mut item, content);
|
||||
(!resized_images.is_empty()).then(|| {
|
||||
ImageResizeNotice::new(ImageResizeNoticeSource::UserMessage, resized_images)
|
||||
})
|
||||
@@ -172,7 +181,7 @@ pub(crate) fn prepare_response_items(
|
||||
}
|
||||
|
||||
fn prepare_message_content(
|
||||
items: &mut [ContentItem],
|
||||
items: &mut [AnnotatedContent],
|
||||
origin: ImageOrigin<'_>,
|
||||
resize_notice_mode: ImageResizeNoticeMode,
|
||||
metadata: &mut Vec<ImagePreparationMetadata>,
|
||||
@@ -180,12 +189,12 @@ fn prepare_message_content(
|
||||
) -> Vec<ResizedImage> {
|
||||
let image_count = items
|
||||
.iter()
|
||||
.filter(|item| matches!(item, ContentItem::InputImage { .. }))
|
||||
.filter(|item| matches!(item.content(), ContentItem::InputImage { .. }))
|
||||
.count();
|
||||
let mut image_number = 0;
|
||||
let mut resized_images = Vec::new();
|
||||
for item in items {
|
||||
if let ContentItem::InputImage { image_url, detail } = item {
|
||||
if let ContentItem::InputImage { image_url, detail } = item.content_mut() {
|
||||
image_number += 1;
|
||||
match prepare_image(image_url, detail, origin, metadata, mode) {
|
||||
Ok(Some(resize)) if resize_notice_mode == ImageResizeNoticeMode::Enabled => {
|
||||
@@ -201,9 +210,10 @@ fn prepare_message_content(
|
||||
Ok(_) => {}
|
||||
Err(error) => {
|
||||
warn!(%error, "failed to prepare message image");
|
||||
*item = ContentItem::InputText {
|
||||
text: error.placeholder().to_string(),
|
||||
};
|
||||
*item = AnnotatedContent::input_text(
|
||||
error.placeholder(),
|
||||
ContentItemKind("images.preparation_error".to_string()),
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -52,6 +52,16 @@ fn preparation_preserves_small_image_bytes_and_replaces_remote_urls() {
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
}];
|
||||
items.push(ResponseItem::Message {
|
||||
id: None,
|
||||
role: "developer".to_string(),
|
||||
content: vec![ContentItem::InputImage {
|
||||
image_url: "https://example.com/developer-image.png".to_string(),
|
||||
detail: Some(ImageDetail::High),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
});
|
||||
|
||||
prepare_response_items(
|
||||
&mut items,
|
||||
@@ -71,6 +81,25 @@ fn preparation_preserves_small_image_bytes_and_replaces_remote_urls() {
|
||||
};
|
||||
assert_eq!(decoded_image(image_url).0, original_bytes);
|
||||
assert_eq!(text, REMOTE_IMAGE_URL_PLACEHOLDER);
|
||||
assert_eq!(
|
||||
&items[1],
|
||||
&ResponseItem::Message {
|
||||
id: None,
|
||||
role: "developer".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: REMOTE_IMAGE_URL_PLACEHOLDER.to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: Some(
|
||||
InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![ContentItemKind(
|
||||
"images.preparation_error".to_string()
|
||||
)]),
|
||||
..Default::default()
|
||||
}
|
||||
),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -199,7 +228,16 @@ fn resize_notices_preserve_original_image_positions_and_skip_failed_images() {
|
||||
},
|
||||
],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
internal_chat_message_metadata_passthrough: Some(
|
||||
InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![
|
||||
ContentItemKind("user.image".to_string()),
|
||||
ContentItemKind("user.image".to_string()),
|
||||
ContentItemKind("user.image".to_string()),
|
||||
]),
|
||||
..Default::default()
|
||||
},
|
||||
),
|
||||
},
|
||||
ResponseItem::FunctionCallOutput {
|
||||
id: None,
|
||||
@@ -231,9 +269,25 @@ fn resize_notices_preserve_original_image_positions_and_skip_failed_images() {
|
||||
"</image_resize_notice>"
|
||||
);
|
||||
|
||||
let ResponseItem::Message { content, .. } = &items[0] else {
|
||||
let ResponseItem::Message {
|
||||
content,
|
||||
internal_chat_message_metadata_passthrough,
|
||||
..
|
||||
} = &items[0]
|
||||
else {
|
||||
panic!("expected message");
|
||||
};
|
||||
assert_eq!(
|
||||
internal_chat_message_metadata_passthrough,
|
||||
&Some(InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![
|
||||
ContentItemKind("user.image".to_string()),
|
||||
ContentItemKind("images.preparation_error".to_string()),
|
||||
ContentItemKind("user.image".to_string()),
|
||||
]),
|
||||
..Default::default()
|
||||
}),
|
||||
);
|
||||
let [
|
||||
ContentItem::InputImage {
|
||||
image_url: small_message_image_url,
|
||||
|
||||
@@ -19,6 +19,7 @@ use codex_protocol::protocol::SessionMeta;
|
||||
use codex_protocol::protocol::SessionMetaLine;
|
||||
use codex_protocol::protocol::WorldStateItem;
|
||||
use codex_protocol::security_risk::SecurityRiskScore;
|
||||
use core_test_support::responses::strip_metadata_from_items;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::collections::BTreeMap;
|
||||
@@ -171,7 +172,9 @@ async fn record_initial_history_ignores_security_risk_scores() {
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
raw_history_items(&session.state.lock().await.clone_history()),
|
||||
strip_metadata_from_items(&raw_history_items(
|
||||
&session.state.lock().await.clone_history()
|
||||
)),
|
||||
vec![user_item]
|
||||
);
|
||||
}
|
||||
|
||||
@@ -259,7 +259,10 @@ fn user_message(text: &str) -> ResponseItem {
|
||||
text: text.to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
internal_chat_message_metadata_passthrough: Some(InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![ContentItemKind("unknown".to_string())]),
|
||||
..Default::default()
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -322,7 +325,10 @@ fn assistant_message(text: &str) -> ResponseItem {
|
||||
text: text.to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
internal_chat_message_metadata_passthrough: Some(InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![ContentItemKind("unknown".to_string())]),
|
||||
..Default::default()
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2504,7 +2510,16 @@ async fn prepares_resumed_history_before_installing_it() {
|
||||
},
|
||||
],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
internal_chat_message_metadata_passthrough: Some(
|
||||
InternalChatMessageMetadataPassthrough {
|
||||
content_item_kinds: Some(vec![
|
||||
ContentItemKind("images.preparation_error".to_string()),
|
||||
ContentItemKind("images.preparation_error".to_string()),
|
||||
ContentItemKind("unknown".to_string()),
|
||||
]),
|
||||
..Default::default()
|
||||
},
|
||||
),
|
||||
}]
|
||||
);
|
||||
assert_eq!(
|
||||
@@ -11184,30 +11199,14 @@ async fn sample_rollout(
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
);
|
||||
|
||||
let user1 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "first user".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let user1 = user_message("first user");
|
||||
live_history.record_items(
|
||||
std::iter::once(&user1),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
);
|
||||
rollout_items.push(RolloutItem::ResponseItem(user1.clone().into()));
|
||||
|
||||
let assistant1 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: "assistant reply one".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let assistant1 = assistant_message("assistant reply one");
|
||||
live_history.record_items(
|
||||
std::iter::once(&assistant1),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
@@ -11230,30 +11229,14 @@ async fn sample_rollout(
|
||||
window_id: Some(window_ids.window_id.to_string()),
|
||||
}));
|
||||
|
||||
let user2 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "second user".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let user2 = user_message("second user");
|
||||
live_history.record_items(
|
||||
std::iter::once(&user2),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
);
|
||||
rollout_items.push(RolloutItem::ResponseItem(user2.clone().into()));
|
||||
|
||||
let assistant2 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: "assistant reply two".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let assistant2 = assistant_message("assistant reply two");
|
||||
live_history.record_items(
|
||||
std::iter::once(&assistant2),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
@@ -11276,30 +11259,14 @@ async fn sample_rollout(
|
||||
window_id: Some(window_ids.window_id.to_string()),
|
||||
}));
|
||||
|
||||
let user3 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "user".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: "third user".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let user3 = user_message("third user");
|
||||
live_history.record_items(
|
||||
std::iter::once(&user3),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
);
|
||||
rollout_items.push(RolloutItem::ResponseItem(user3.into()));
|
||||
|
||||
let assistant3 = ResponseItem::Message {
|
||||
id: None,
|
||||
role: "assistant".to_string(),
|
||||
content: vec![ContentItem::OutputText {
|
||||
text: "assistant reply three".to_string(),
|
||||
}],
|
||||
phase: None,
|
||||
internal_chat_message_metadata_passthrough: None,
|
||||
};
|
||||
let assistant3 = assistant_message("assistant reply three");
|
||||
live_history.record_items(
|
||||
std::iter::once(&assistant3),
|
||||
reconstruction_turn.model_info.truncation_policy.into(),
|
||||
|
||||
@@ -266,6 +266,7 @@ async fn responses_lite_prepares_images() -> Result<()> {
|
||||
.await;
|
||||
|
||||
let request = response_mock.single_request();
|
||||
assert!(request.has_content_kinds(&["user.image", "images.preparation_error"]));
|
||||
let user_content = request
|
||||
.input()
|
||||
.into_iter()
|
||||
|
||||
Reference in New Issue
Block a user