From 260261ed8f5c91ad6b7f695571a4111ed1a46272 Mon Sep 17 00:00:00 2001 From: rka-oai Date: Tue, 11 Aug 2026 00:08:09 +0000 Subject: [PATCH] Defer `view_image` processing to history insertion (#37902) ## What changed - Pass image bytes from `view_image` through unchanged for both direct and code-mode calls, leaving decoding and resizing to the shared history-insertion path. - Represent invalid image data with the existing image-omission placeholder instead of returning a tool error. ## Testing - Update the `view_image` integration test to cover placeholder output for invalid image data. GitOrigin-RevId: 62863defd4815efb8a7725712fd583ad81db3fbb --- .../core/src/tools/handlers/view_image.rs | 130 +----------------- codex-rs/core/tests/suite/view_image.rs | 13 +- 2 files changed, 11 insertions(+), 132 deletions(-) diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index d1271202d0..ca32af5b1a 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -12,7 +12,6 @@ use serde::Deserialize; use crate::function_tool::FunctionCallError; use crate::original_image_detail::can_request_original_image_detail; -use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -25,8 +24,6 @@ use crate::tools::registry::CoreToolRuntime; use crate::tools::registry::ToolExecutor; use codex_tools::ToolName; use codex_tools::ToolSpec; -use image::ImageFormat; -use std::io::Cursor; pub struct ViewImageHandler { options: ViewImageToolOptions, @@ -52,10 +49,6 @@ impl ViewImageHandler { const VIEW_IMAGE_UNSUPPORTED_MESSAGE: &str = "view_image is not allowed because you do not support image inputs"; -const VIEW_IMAGE_INVALID_MESSAGE: &str = - "unable to process image: invalid or unsupported image data"; -const VIEW_IMAGE_ENCODING_ERROR_MESSAGE: &str = - "unable to process image: could not encode validated image"; #[derive(Deserialize)] struct ViewImageArgs { @@ -111,7 +104,6 @@ impl ViewImageHandler { step_context, payload, call_id, - source, .. } = invocation; @@ -181,9 +173,6 @@ impl ViewImageHandler { "unable to read image at `{model_visible_path}`: {error}" )) })?; - let decoded_image = image::load_from_memory(&file_bytes).map_err(|_| { - FunctionCallError::RespondToModel(VIEW_IMAGE_INVALID_MESSAGE.to_string()) - })?; let can_request_original_detail = can_request_original_image_detail(&turn.model_info); let use_original_detail = self.options.unified_image_budget @@ -194,21 +183,8 @@ impl ViewImageHandler { DEFAULT_IMAGE_DETAIL }; - let image_url = if matches!(source, ToolCallSource::CodeMode { .. }) { - // Code mode must receive only image pixels, while keeping the original - // dimensions for centralized resizing and resize notices. - let mut sanitized_bytes = Vec::new(); - decoded_image - .write_to(&mut Cursor::new(&mut sanitized_bytes), ImageFormat::Png) - .map_err(|_| { - FunctionCallError::RespondToModel(VIEW_IMAGE_ENCODING_ERROR_MESSAGE.to_string()) - })?; - data_url_from_bytes("image/png", &sanitized_bytes) - } else { - // Direct tool calls retain image metadata for centralized preparation. - data_url_from_bytes("application/octet-stream", &file_bytes) - }; - drop(decoded_image); + // The history insertion path owns image decoding and resizing. + let image_url = data_url_from_bytes("application/octet-stream", &file_bytes); let item = TurnItem::ImageView(ImageViewItem { id: call_id, @@ -279,14 +255,13 @@ mod tests { use crate::session::step_context::StepContext; use crate::session::tests::make_session_and_context; use crate::session::turn_context::TurnEnvironment; + use crate::tools::context::ToolCallSource; use crate::tools::context::ToolInvocation; use crate::turn_diff_tracker::TurnDiffTracker; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use core_test_support::TempDirExt; - use image::ImageBuffer; - use image::Rgba; use pretty_assertions::assert_eq; use serde_json::json; use std::sync::Arc; @@ -309,15 +284,6 @@ mod tests { )); } - fn tiny_png() -> Vec { - let image = ImageBuffer::from_pixel(1, 1, Rgba([255u8, 0, 0, 255])); - let mut bytes = Vec::new(); - image - .write_to(&mut Cursor::new(&mut bytes), ImageFormat::Png) - .expect("encode test image"); - bytes - } - #[test] fn log_preview_omits_image_data() { let output = ViewImageOutput { @@ -358,7 +324,7 @@ mod tests { replace_primary_environment_cwd(&mut turn, image_cwd.clone()); let image_path = image_cwd.join("image.png"); - std::fs::write(image_path.as_path(), tiny_png()).expect("write test image"); + std::fs::write(image_path.as_path(), b"not a real image").expect("write test image"); Arc::make_mut(&mut turn.config) .permissions .set_permission_profile(PermissionProfile::Disabled) @@ -434,7 +400,7 @@ mod tests { replace_primary_environment_cwd(&mut turn, image_cwd.clone()); let image_path = image_cwd.join("image.png"); - std::fs::write(image_path.as_path(), tiny_png()).expect("write test image"); + std::fs::write(image_path.as_path(), b"not a real image").expect("write test image"); let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0] else { panic!("primary environment should be ready"); @@ -461,90 +427,4 @@ mod tests { result.expect("explicit high detail should be accepted"); } - - #[tokio::test(flavor = "multi_thread")] - async fn handle_returns_png_image_for_code_mode() { - let (session, mut turn) = make_session_and_context().await; - let image_dir = tempfile::tempdir().expect("create image temp dir"); - let image_cwd = image_dir.abs(); - - replace_primary_environment_cwd(&mut turn, image_cwd.clone()); - let image_path = image_cwd.join("image.png"); - std::fs::write(image_path.as_path(), tiny_png()).expect("write test image"); - let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0] - else { - panic!("primary environment should be ready"); - }; - environment.config.permission_profile = - PermissionProfileSnapshot::legacy(PermissionProfile::Disabled); - let turn = Arc::new(turn); - - let output = ViewImageHandler::default() - .handle(ToolInvocation { - session: Arc::new(session), - step_context: StepContext::for_test(Arc::clone(&turn)), - turn, - cancellation_token: tokio_util::sync::CancellationToken::new(), - tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), - call_id: "call-view-image".to_string(), - tool_name: codex_tools::ToolName::plain("view_image"), - source: ToolCallSource::CodeMode { - cell_id: "cell-1".to_string(), - runtime_tool_call_id: "tool-1".to_string(), - }, - payload: ToolPayload::Function { - arguments: json!({ "path": "image.png" }).to_string(), - }, - }) - .await - .expect("code mode image should be accepted"); - - let result = output.code_mode_result(&ToolPayload::Function { - arguments: "{}".to_string(), - }); - let image_url = result["image_url"] - .as_str() - .expect("code mode image URL should be a string"); - assert!(image_url.starts_with("data:image/png;base64,")); - } - - #[tokio::test(flavor = "multi_thread")] - async fn handle_rejects_invalid_image_before_returning_output() { - let (session, mut turn) = make_session_and_context().await; - let image_dir = tempfile::tempdir().expect("create image temp dir"); - let image_cwd = image_dir.abs(); - - replace_primary_environment_cwd(&mut turn, image_cwd.clone()); - let image_path = image_cwd.join("not-an-image.txt"); - std::fs::write(image_path.as_path(), b"arbitrary file contents") - .expect("write invalid image"); - let TurnEnvironmentState::Ready(environment) = &mut turn.environments.environments[0] - else { - panic!("primary environment should be ready"); - }; - environment.config.permission_profile = - PermissionProfileSnapshot::legacy(PermissionProfile::Disabled); - let turn = Arc::new(turn); - - let result = ViewImageHandler::default() - .handle(ToolInvocation { - session: Arc::new(session), - step_context: StepContext::for_test(Arc::clone(&turn)), - turn, - cancellation_token: tokio_util::sync::CancellationToken::new(), - tracker: Arc::new(Mutex::new(TurnDiffTracker::new())), - call_id: "call-view-image".to_string(), - tool_name: codex_tools::ToolName::plain("view_image"), - source: ToolCallSource::Direct, - payload: ToolPayload::Function { - arguments: json!({ "path": "not-an-image.txt" }).to_string(), - }, - }) - .await; - - let Err(FunctionCallError::RespondToModel(message)) = result else { - panic!("expected invalid image error"); - }; - assert_eq!(message, VIEW_IMAGE_INVALID_MESSAGE); - } } diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index fe9928e198..ef12dd54c7 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -1443,7 +1443,7 @@ async fn view_image_tool_errors_when_path_is_directory() -> anyhow::Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn view_image_tool_rejects_invalid_image_before_tool_output() -> anyhow::Result<()> { +async fn view_image_tool_turns_invalid_image_into_placeholder() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -1496,13 +1496,12 @@ async fn view_image_tool_rejects_invalid_image_before_tool_output() -> anyhow::R .await; let request = second_mock.single_request(); - let output_text = request - .function_call_output_content_and_success(call_id) - .and_then(|(content, _)| content) - .context("invalid view_image error text present")?; assert_eq!( - output_text, - "unable to process image: invalid or unsupported image data" + request.function_call_output(call_id).get("output"), + Some(&serde_json::json!([{ + "type": "input_text", + "text": "image content omitted because it could not be processed" + }])) ); Ok(()) }