From 09f4c4506874e75e25b967d51844cb699086e124 Mon Sep 17 00:00:00 2001 From: jif Date: Mon, 31 Aug 2026 14:45:23 +0000 Subject: [PATCH] Preserve user text when Guardian history drops oversized images (#41858) ## Why User messages with images larger than the Guardian history budget were skipped entirely, removing any accompanying instructions from retained review context. ## What changed - Retain the text and message metadata from oversized user messages while omitting their images. - Keep the existing behavior for other oversized history items. ## Testing - Cover text and metadata retention, content ordering, and the integration path with an image larger than the 4 MiB history budget. GitOrigin-RevId: 91d99609a8d096b87d0f2b1d149f577a00dce699 --- codex-rs/core/tests/suite/guardian_history.rs | 54 +++++++++++++++++-- codex-rs/guardian-context/src/history.rs | 48 ++++++++++++++++- .../guardian-context/src/history_tests.rs | 41 ++++++++++++++ 3 files changed, 138 insertions(+), 5 deletions(-) diff --git a/codex-rs/core/tests/suite/guardian_history.rs b/codex-rs/core/tests/suite/guardian_history.rs index f441d0c1e9..6ab40196cb 100644 --- a/codex-rs/core/tests/suite/guardian_history.rs +++ b/codex-rs/core/tests/suite/guardian_history.rs @@ -1,6 +1,8 @@ -//! Exercises retained review history through compaction, eviction, and rollback. +//! Exercises retained review history through compaction, eviction, large images, and rollback. use anyhow::Result; +use base64::Engine; +use base64::engine::general_purpose::STANDARD as BASE64_STANDARD; use codex_core::TurnInputRequest; use codex_core::config::Constrained; use codex_features::Feature; @@ -22,9 +24,15 @@ use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; +use image::DynamicImage; +use image::RgbImage; use pretty_assertions::assert_eq; +use rand::RngCore; +use rand::SeedableRng; +use rand::rngs::StdRng; use serde_json::json; use std::collections::HashMap; +use std::io::Cursor; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> Result<()> { @@ -116,9 +124,22 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> }) .await; + // A valid, supported-size image whose encoded bytes exceed the retention budget. + let (width, height) = (1200, 1200); + let mut pixels = vec![0; (width * height * 3) as usize]; + let seed = 42; + StdRng::seed_from_u64(seed).fill_bytes(&mut pixels); + let image = RgbImage::from_raw(width, height, pixels).unwrap(); + let mut image_data = Cursor::new(Vec::new()); + DynamicImage::ImageRgb8(image).write_to(&mut image_data, image::ImageFormat::Png)?; + let image_url = format!( + "data:image/png;base64,{}", + BASE64_STANDARD.encode(image_data.into_inner()) + ); + assert!(image_url.len() > 4 * 1024 * 1024); let command = r#"{"cmd":"echo publish","sandbox_permissions":"require_escalated","justification":"Publish the inspected change."}"#; for (prompt, retained) in [ - ("Now publish.", true), + ("Do not publish the attached image.", true), ("Inspect a different repository.", false), ] { let review = mount_sse_sequence( @@ -136,8 +157,33 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> ], ) .await; - test.submit_text_turn(prompt).await?; + test.codex + .start_or_steer_turn(TurnInputRequest::user_input(vec![ + UserInput::Text { + text: prompt.to_owned(), + text_elements: Vec::new(), + }, + UserInput::Image { + image_url: image_url.clone(), + detail: None, + }, + ])) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; let requests = review.requests(); + assert!( + requests[0] + .input() + .iter() + .filter_map(|item| item["content"].as_array()) + .flatten() + .any(|item| item["image_url"] + .as_str() + .is_some_and(|url| url.len() > 4 * 1024 * 1024)) + ); let guardian = requests .iter() .find(|request| { @@ -159,7 +205,7 @@ async fn guardian_history_survives_compaction_and_eviction_but_not_rollback() -> "Only publish to a private repository.", "tool update_plan call", "tool update_plan result", - "Now publish.", + "Do not publish the attached image.", ] .map(|text| { transcript diff --git a/codex-rs/guardian-context/src/history.rs b/codex-rs/guardian-context/src/history.rs index 5032edd040..465dceb4d0 100644 --- a/codex-rs/guardian-context/src/history.rs +++ b/codex-rs/guardian-context/src/history.rs @@ -10,6 +10,7 @@ use std::io; use std::io::Write; use std::sync::Arc; +use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; use crate::SectionHistory; @@ -46,7 +47,7 @@ impl TranscriptHistory { } /// Appends one original item, evicting only older entries of the same kind. - /// An item too large for its budget is skipped without changing retained entries. + /// Oversized user images fall back to bounded text; other oversized items are skipped. pub fn record(&mut self, item: &ResponseItem) { let mut size = BoundedSize { bytes: std::mem::size_of::(), @@ -60,6 +61,51 @@ impl TranscriptHistory { Ok(()) }); if measured.is_err() { + if let ResponseItem::Message { + id, + role, + content, + phase, + internal_chat_message_metadata_passthrough, + } = item + && role == "user" + && content + .iter() + .any(|item| matches!(item, ContentItem::InputImage { .. })) + { + // Measure text and metadata before cloning, without copying image data. + size.bytes = std::mem::size_of::(); + if serde_json::to_writer( + &mut size, + &(id, role, phase, internal_chat_message_metadata_passthrough), + ) + .is_err() + { + return; + } + let mut text = Vec::new(); + for item in content.iter().filter(|item| { + matches!( + item, + ContentItem::InputText { .. } | ContentItem::OutputText { .. } + ) + }) { + if serde_json::to_writer(&mut size, item).is_err() { + return; + } + text.push(item.clone()); + } + if !text.is_empty() { + self.record(&ResponseItem::Message { + id: id.clone(), + role: role.clone(), + content: text, + phase: phase.clone(), + internal_chat_message_metadata_passthrough: + internal_chat_message_metadata_passthrough.clone(), + }); + } + } return; } let is_user = item.is_user_message(); diff --git a/codex-rs/guardian-context/src/history_tests.rs b/codex-rs/guardian-context/src/history_tests.rs index a38cea083f..b88ac3722d 100644 --- a/codex-rs/guardian-context/src/history_tests.rs +++ b/codex-rs/guardian-context/src/history_tests.rs @@ -77,3 +77,44 @@ fn byte_limits_are_independent_and_oversized_items_do_not_clear_history() { }); assert_eq!(history.items().collect::>(), vec![&keep, &large]); } + +#[test] +fn oversized_user_images_preserve_text_and_metadata_in_order() { + let before = tool("earlier tool result"); + let after = message("later message"); + let text_only: ResponseItem = serde_json::from_value(json!({ + "type": "message", "id": "user-1", "role": "user", "phase": "commentary", + "internal_chat_message_metadata_passthrough": {"turn_id": "turn-1"}, + "content": [ + {"type": "input_text", "text": "Do not publish anything."}, + {"type": "output_text", "text": "Only inspect the attached image."} + ] + })) + .unwrap(); + for image_bytes in [16, MAX_BYTES_PER_KIND] { + let mut with_image = text_only.clone(); + let ResponseItem::Message { content, .. } = &mut with_image else { + unreachable!() + }; + content.insert( + /*index*/ 1, + ContentItem::InputImage { + image_url: format!("data:image/png;base64,{}", "A".repeat(image_bytes)), + detail: Some(codex_protocol::models::ImageDetail::Original), + }, + ); + let mut history = TranscriptHistory::default(); + history.record(&before); + history.record(&with_image); + history.record(&after); + let expected = if image_bytes < MAX_BYTES_PER_KIND { + &with_image + } else { + &text_only + }; + assert_eq!( + history.items().collect::>(), + vec![&before, expected, &after] + ); + } +}