From 6709ad8975f0839eab5ddce7899e6b12acd7a209 Mon Sep 17 00:00:00 2001 From: charley-oai Date: Fri, 9 Jan 2026 21:33:45 -0800 Subject: [PATCH 1/2] Label attached images so agent can understand in-message labels (#8950) Agent wouldn't "see" attached images and would instead try to use the view_file tool: image In this PR, we wrap image content items in XML tags with the name of each image (now just a numbered name like `[Image #1]`), so that the model can understand inline image references (based on name). We also put the image content items above the user message which the model seems to prefer (maybe it's more used to definitions being before references). We also tweak the view_file tool description which seemed to help a bit Results on a simple eval set of images: Before image After image ```json [ { "id": "single_describe", "prompt": "Describe the attached image in one sentence.", "images": ["image_a.png"] }, { "id": "single_color", "prompt": "What is the dominant color in the image? Answer with a single color word.", "images": ["image_b.png"] }, { "id": "orientation_check", "prompt": "Is the image portrait or landscape? Answer in one sentence.", "images": ["image_c.png"] }, { "id": "detail_request", "prompt": "Look closely at the image and call out any small details you notice.", "images": ["image_d.png"] }, { "id": "two_images_compare", "prompt": "I attached two images. Are they the same or different? Briefly explain.", "images": ["image_a.png", "image_b.png"] }, { "id": "two_images_captions", "prompt": "Provide a short caption for each image (Image 1, Image 2).", "images": ["image_c.png", "image_d.png"] }, { "id": "multi_image_rank", "prompt": "Rank the attached images from most colorful to least colorful.", "images": ["image_a.png", "image_b.png", "image_c.png"] }, { "id": "multi_image_choice", "prompt": "Which image looks more vibrant? Answer with 'Image 1' or 'Image 2'.", "images": ["image_b.png", "image_d.png"] } ] ``` --- codex-rs/core/src/codex.rs | 18 + codex-rs/core/src/event_mapping.rs | 88 +++- .../core/src/tools/handlers/view_image.rs | 13 +- codex-rs/core/src/tools/spec.rs | 8 +- codex-rs/core/tests/suite/view_image.rs | 14 + codex-rs/protocol/src/models.rs | 164 +++++-- codex-rs/tui/src/bottom_pane/chat_composer.rs | 446 +++++++----------- codex-rs/tui/src/bottom_pane/mod.rs | 11 +- codex-rs/tui/src/bottom_pane/textarea.rs | 82 ++++ codex-rs/tui/src/chatwidget.rs | 30 +- codex-rs/tui/src/chatwidget/tests.rs | 4 +- .../tui2/src/bottom_pane/chat_composer.rs | 403 +++++++--------- codex-rs/tui2/src/bottom_pane/mod.rs | 11 +- codex-rs/tui2/src/bottom_pane/textarea.rs | 82 ++++ codex-rs/tui2/src/chatwidget.rs | 30 +- codex-rs/tui2/src/chatwidget/tests.rs | 4 +- 16 files changed, 800 insertions(+), 608 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f411cf9aab..00ed4fba0d 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1540,6 +1540,24 @@ impl Session { } } + /// Returns the input if there was no task running to inject into + pub async fn inject_response_items( + &self, + input: Vec, + ) -> Result<(), Vec> { + let mut active = self.active_turn.lock().await; + match active.as_mut() { + Some(at) => { + let mut ts = at.turn_state.lock().await; + for item in input { + ts.push_pending_input(item); + } + Ok(()) + } + None => Err(input), + } + } + pub async fn get_pending_input(&self) -> Vec { let mut active = self.active_turn.lock().await; match active.as_mut() { diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index 6ab6291a4b..fe592236c1 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -9,6 +9,10 @@ use codex_protocol::models::ReasoningItemContent; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::models::ResponseItem; use codex_protocol::models::WebSearchAction; +use codex_protocol::models::is_image_close_tag_text; +use codex_protocol::models::is_image_open_tag_text; +use codex_protocol::models::is_local_image_close_tag_text; +use codex_protocol::models::is_local_image_open_tag_text; use codex_protocol::user_input::UserInput; use tracing::warn; use uuid::Uuid; @@ -32,9 +36,17 @@ fn parse_user_message(message: &[ContentItem]) -> Option { let mut content: Vec = Vec::new(); - for content_item in message.iter() { + for (idx, content_item) in message.iter().enumerate() { match content_item { ContentItem::InputText { text } => { + if (is_local_image_open_tag_text(text) || is_image_open_tag_text(text)) + && (matches!(message.get(idx + 1), Some(ContentItem::InputImage { .. }))) + || (idx > 0 + && (is_local_image_close_tag_text(text) || is_image_close_tag_text(text)) + && matches!(message.get(idx - 1), Some(ContentItem::InputImage { .. }))) + { + continue; + } if is_session_prefix(text) || is_user_shell_command_text(text) { return None; } @@ -177,6 +189,80 @@ mod tests { } } + #[test] + fn skips_local_image_label_text() { + let image_url = "data:image/png;base64,abc".to_string(); + let label = codex_protocol::models::local_image_open_tag_text(1); + let user_text = "Please review this image.".to_string(); + + let item = ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputText { text: label }, + ContentItem::InputImage { + image_url: image_url.clone(), + }, + ContentItem::InputText { + text: "".to_string(), + }, + ContentItem::InputText { + text: user_text.clone(), + }, + ], + }; + + let turn_item = parse_turn_item(&item).expect("expected user message turn item"); + + match turn_item { + TurnItem::UserMessage(user) => { + let expected_content = vec![ + UserInput::Image { image_url }, + UserInput::Text { text: user_text }, + ]; + assert_eq!(user.content, expected_content); + } + other => panic!("expected TurnItem::UserMessage, got {other:?}"), + } + } + + #[test] + fn skips_unnamed_image_label_text() { + let image_url = "data:image/png;base64,abc".to_string(); + let label = codex_protocol::models::image_open_tag_text(); + let user_text = "Please review this image.".to_string(); + + let item = ResponseItem::Message { + id: None, + role: "user".to_string(), + content: vec![ + ContentItem::InputText { text: label }, + ContentItem::InputImage { + image_url: image_url.clone(), + }, + ContentItem::InputText { + text: codex_protocol::models::image_close_tag_text(), + }, + ContentItem::InputText { + text: user_text.clone(), + }, + ], + }; + + let turn_item = parse_turn_item(&item).expect("expected user message turn item"); + + match turn_item { + TurnItem::UserMessage(user) => { + let expected_content = vec![ + UserInput::Image { image_url }, + UserInput::Text { text: user_text }, + ]; + assert_eq!(user.content, expected_content); + } + other => panic!("expected TurnItem::UserMessage, got {other:?}"), + } + } + #[test] fn skips_user_instructions_and_env() { let items = vec![ diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index 3c0670e3e0..87dd7207b1 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -11,7 +11,9 @@ use crate::tools::context::ToolPayload; use crate::tools::handlers::parse_arguments; use crate::tools::registry::ToolHandler; use crate::tools::registry::ToolKind; -use codex_protocol::user_input::UserInput; +use codex_protocol::models::ContentItem; +use codex_protocol::models::ResponseInputItem; +use codex_protocol::models::local_image_content_items_with_label_number; pub struct ViewImageHandler; @@ -63,8 +65,15 @@ impl ToolHandler for ViewImageHandler { } let event_path = abs_path.clone(); + let content: Vec = + local_image_content_items_with_label_number(&abs_path, None); + let input = ResponseInputItem::Message { + role: "user".to_string(), + content, + }; + session - .inject_input(vec![UserInput::LocalImage { path: abs_path }]) + .inject_response_items(vec![input]) .await .map_err(|_| { FunctionCallError::RespondToModel( diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index 76f983292e..0a66b41403 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -8,6 +8,7 @@ use crate::tools::handlers::apply_patch::create_apply_patch_json_tool; use crate::tools::handlers::collab::DEFAULT_WAIT_TIMEOUT_MS; use crate::tools::handlers::collab::MAX_WAIT_TIMEOUT_MS; use crate::tools::registry::ToolRegistryBuilder; +use codex_protocol::models::VIEW_IMAGE_TOOL_NAME; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; @@ -412,10 +413,9 @@ fn create_view_image_tool() -> ToolSpec { )]); ToolSpec::Function(ResponsesApiTool { - name: "view_image".to_string(), - description: - "Attach a local image (by filesystem path) to the thread context for this turn." - .to_string(), + name: VIEW_IMAGE_TOOL_NAME.to_string(), + description: "View a local image from the filesystem (only use if given a full filepath by the user, and the image isn't already attached to the thread context within tags)." + .to_string(), strict: false, parameters: JsonSchema::Object { properties, diff --git a/codex-rs/core/tests/suite/view_image.rs b/codex-rs/core/tests/suite/view_image.rs index eedc524592..034254f7c5 100644 --- a/codex-rs/core/tests/suite/view_image.rs +++ b/codex-rs/core/tests/suite/view_image.rs @@ -216,6 +216,20 @@ async fn view_image_tool_attaches_local_image() -> anyhow::Result<()> { let image_message = find_image_message(&body).expect("pending input image message not included in request"); + let content_items = image_message + .get("content") + .and_then(Value::as_array) + .expect("image message has content array"); + assert_eq!( + content_items.len(), + 1, + "view_image should inject only the image content item (no tag/label text)" + ); + assert_eq!( + content_items[0].get("type").and_then(Value::as_str), + Some("input_image"), + "view_image should inject only an input_image content item" + ); let image_url = image_message .get("content") .and_then(Value::as_array) diff --git a/codex-rs/protocol/src/models.rs b/codex-rs/protocol/src/models.rs index 69a682f2df..bec2a35f5a 100644 --- a/codex-rs/protocol/src/models.rs +++ b/codex-rs/protocol/src/models.rs @@ -180,6 +180,48 @@ fn local_image_error_placeholder( } } +pub const VIEW_IMAGE_TOOL_NAME: &str = "view_image"; + +const IMAGE_OPEN_TAG: &str = ""; +const IMAGE_CLOSE_TAG: &str = ""; +const LOCAL_IMAGE_OPEN_TAG_PREFIX: &str = ""; +const LOCAL_IMAGE_CLOSE_TAG: &str = IMAGE_CLOSE_TAG; + +pub fn image_open_tag_text() -> String { + IMAGE_OPEN_TAG.to_string() +} + +pub fn image_close_tag_text() -> String { + IMAGE_CLOSE_TAG.to_string() +} + +pub fn local_image_label_text(label_number: usize) -> String { + format!("[Image #{label_number}]") +} + +pub fn local_image_open_tag_text(label_number: usize) -> String { + let label = local_image_label_text(label_number); + format!("{LOCAL_IMAGE_OPEN_TAG_PREFIX}{label}{LOCAL_IMAGE_OPEN_TAG_SUFFIX}") +} + +pub fn is_local_image_open_tag_text(text: &str) -> bool { + text.strip_prefix(LOCAL_IMAGE_OPEN_TAG_PREFIX) + .is_some_and(|rest| rest.ends_with(LOCAL_IMAGE_OPEN_TAG_SUFFIX)) +} + +pub fn is_local_image_close_tag_text(text: &str) -> bool { + is_image_close_tag_text(text) +} + +pub fn is_image_open_tag_text(text: &str) -> bool { + text == IMAGE_OPEN_TAG +} + +pub fn is_image_close_tag_text(text: &str) -> bool { + text == IMAGE_CLOSE_TAG +} + fn invalid_image_error_placeholder( path: &std::path::Path, error: impl std::fmt::Display, @@ -203,6 +245,53 @@ fn unsupported_image_error_placeholder(path: &std::path::Path, mime: &str) -> Co } } +pub fn local_image_content_items_with_label_number( + path: &std::path::Path, + label_number: Option, +) -> Vec { + match load_and_resize_to_fit(path) { + Ok(image) => { + let mut items = Vec::with_capacity(3); + if let Some(label_number) = label_number { + items.push(ContentItem::InputText { + text: local_image_open_tag_text(label_number), + }); + } + items.push(ContentItem::InputImage { + image_url: image.into_data_url(), + }); + if label_number.is_some() { + items.push(ContentItem::InputText { + text: LOCAL_IMAGE_CLOSE_TAG.to_string(), + }); + } + items + } + Err(err) => { + if matches!(&err, ImageProcessingError::Read { .. }) { + vec![local_image_error_placeholder(path, &err)] + } else if err.is_invalid_image() { + vec![invalid_image_error_placeholder(path, &err)] + } else { + let Some(mime_guess) = mime_guess::from_path(path).first() else { + return vec![local_image_error_placeholder( + path, + "unsupported MIME type (unknown)", + )]; + }; + let mime = mime_guess.essence_str().to_owned(); + if !mime.starts_with("image/") { + return vec![local_image_error_placeholder( + path, + format!("unsupported MIME type `{mime}`"), + )]; + } + vec![unsupported_image_error_placeholder(path, &mime)] + } + } + } +} + impl From for ResponseItem { fn from(item: ResponseInputItem) -> Self { match item { @@ -296,41 +385,27 @@ pub enum ReasoningItemContent { impl From> for ResponseInputItem { fn from(items: Vec) -> Self { + let mut image_index = 0; Self::Message { role: "user".to_string(), content: items .into_iter() - .filter_map(|c| match c { - UserInput::Text { text } => Some(ContentItem::InputText { text }), - UserInput::Image { image_url } => Some(ContentItem::InputImage { image_url }), - UserInput::LocalImage { path } => match load_and_resize_to_fit(&path) { - Ok(image) => Some(ContentItem::InputImage { - image_url: image.into_data_url(), - }), - Err(err) => { - if matches!(&err, ImageProcessingError::Read { .. }) { - Some(local_image_error_placeholder(&path, &err)) - } else if err.is_invalid_image() { - Some(invalid_image_error_placeholder(&path, &err)) - } else { - let Some(mime_guess) = mime_guess::from_path(&path).first() else { - return Some(local_image_error_placeholder( - &path, - "unsupported MIME type (unknown)", - )); - }; - let mime = mime_guess.essence_str().to_owned(); - if !mime.starts_with("image/") { - return Some(local_image_error_placeholder( - &path, - format!("unsupported MIME type `{mime}`"), - )); - } - Some(unsupported_image_error_placeholder(&path, &mime)) - } - } - }, - UserInput::Skill { .. } => None, // Skill bodies are injected later in core + .flat_map(|c| match c { + UserInput::Text { text } => vec![ContentItem::InputText { text }], + UserInput::Image { image_url } => vec![ + ContentItem::InputText { + text: image_open_tag_text(), + }, + ContentItem::InputImage { image_url }, + ContentItem::InputText { + text: image_close_tag_text(), + }, + ], + UserInput::LocalImage { path } => { + image_index += 1; + local_image_content_items_with_label_number(&path, Some(image_index)) + } + UserInput::Skill { .. } => Vec::new(), // Skill bodies are injected later in core }) .collect::>(), } @@ -770,6 +845,33 @@ mod tests { Ok(()) } + #[test] + fn wraps_image_user_input_with_tags() -> Result<()> { + let image_url = "data:image/png;base64,abc".to_string(); + + let item = ResponseInputItem::from(vec![UserInput::Image { + image_url: image_url.clone(), + }]); + + match item { + ResponseInputItem::Message { content, .. } => { + let expected = vec![ + ContentItem::InputText { + text: image_open_tag_text(), + }, + ContentItem::InputImage { image_url }, + ContentItem::InputText { + text: image_close_tag_text(), + }, + ]; + assert_eq!(content, expected); + } + other => panic!("expected message response but got {other:?}"), + } + + Ok(()) + } + #[test] fn local_image_read_error_adds_placeholder() -> Result<()> { let dir = tempdir()?; diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 620b52d9f9..2e7e2241b9 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -46,6 +46,7 @@ use crate::style::user_message_style; use codex_common::fuzzy_match::fuzzy_match; use codex_protocol::custom_prompts::CustomPrompt; use codex_protocol::custom_prompts::PROMPTS_CMD_PREFIX; +use codex_protocol::models::local_image_label_text; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -59,7 +60,7 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::path::Path; +use std::collections::HashSet; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; @@ -274,10 +275,12 @@ impl ChatComposer { // normalize_pasted_path already handles Windows → WSL path conversion, // so we can directly try to read the image dimensions. match image::image_dimensions(&path_buf) { - Ok((w, h)) => { + Ok((width, height)) => { tracing::info!("OK: {pasted}"); - let format_label = pasted_image_format(&path_buf).label(); - self.attach_image(path_buf, w, h, format_label); + tracing::debug!("image dimensions={}x{}", width, height); + let format = pasted_image_format(&path_buf); + tracing::debug!("attached image format={}", format.label()); + self.attach_image(path_buf); true } Err(err) => { @@ -408,13 +411,9 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { - let file_label = path - .file_name() - .map(|name| name.to_string_lossy().into_owned()) - .unwrap_or_else(|| "image".to_string()); - let base_placeholder = format!("{file_label} {width}x{height}"); - let placeholder = self.next_image_placeholder(&base_placeholder); + pub fn attach_image(&mut self, path: PathBuf) { + let image_number = self.attached_images.len() + 1; + let placeholder = local_image_label_text(image_number); // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. self.textarea.insert_element(&placeholder); @@ -477,22 +476,6 @@ impl ChatComposer { } } - fn next_image_placeholder(&mut self, base: &str) -> String { - let text = self.textarea.text(); - let mut suffix = 1; - loop { - let placeholder = if suffix == 1 { - format!("[{base}]") - } else { - format!("[{base} #{suffix}]") - }; - if !text.contains(&placeholder) { - return placeholder; - } - suffix += 1; - } - } - pub(crate) fn insert_str(&mut self, text: &str) { self.textarea.insert_str(text); self.sync_popups(); @@ -818,47 +801,43 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - if let Ok((w, h)) = image::image_dimensions(&path_buf) { - // Remove the current @token (mirror logic from insert_selected_path without inserting text) - // using the flat text and byte-offset cursor API. - let cursor_offset = self.textarea.cursor(); - let text = self.textarea.text(); - // Clamp to a valid char boundary to avoid panics when slicing. - let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); - let before_cursor = &text[..safe_cursor]; - let after_cursor = &text[safe_cursor..]; + match image::image_dimensions(&path_buf) { + Ok((width, height)) => { + tracing::debug!("selected image dimensions={}x{}", width, height); + // Remove the current @token (mirror logic from insert_selected_path without inserting text) + // using the flat text and byte-offset cursor API. + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; - // Determine token boundaries in the full text. - let start_idx = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); - let end_rel_idx = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_idx = safe_cursor + end_rel_idx; + // Determine token boundaries in the full text. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = safe_cursor + end_rel_idx; - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); - let format_label = match Path::new(&sel_path) - .extension() - .and_then(|e| e.to_str()) - .map(str::to_ascii_lowercase) - { - Some(ext) if ext == "png" => "PNG", - Some(ext) if ext == "jpg" || ext == "jpeg" => "JPEG", - _ => "IMG", - }; - self.attach_image(path_buf, w, h, format_label); - // Add a trailing space to keep typing fluid. - self.textarea.insert_str(" "); - } else { - // Fallback to plain path insertion if metadata read fails. - self.insert_selected_path(&sel_path); + self.attach_image(path_buf); + // Add a trailing space to keep typing fluid. + self.textarea.insert_str(" "); + } + Err(err) => { + tracing::trace!("image dimensions lookup failed: {err}"); + // Fallback to plain path insertion if metadata read fails. + self.insert_selected_path(&sel_path); + } } } else { // Non-image: inserting file path. @@ -1464,20 +1443,29 @@ impl ChatComposer { } } - // For non-char inputs (or after flushing), handle normally. - // Special handling for backspace on placeholders - if let KeyEvent { - code: KeyCode::Backspace, - .. - } = input - && self.try_remove_any_placeholder_at_cursor() + // Backspace at the start of an image placeholder should delete that placeholder (rather + // than deleting content before it). Do this without scanning the full text by consulting + // the textarea's element list. + if matches!(input.code, KeyCode::Backspace) + && self.try_remove_image_element_at_cursor_start() { return (InputResult::None, true); } - // Normal input handling + // For non-char inputs (or after flushing), handle normally. + // Track element removals so we can drop any corresponding placeholders without scanning + // the full text. (Placeholders are atomic elements; when deleted, the element disappears.) + let elements_before = if self.pending_pastes.is_empty() && self.attached_images.is_empty() { + None + } else { + Some(self.textarea.element_payloads()) + }; + self.textarea.input(input); - let text_after = self.textarea.text(); + + if let Some(elements_before) = elements_before { + self.reconcile_deleted_elements(elements_before); + } // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1499,176 +1487,69 @@ impl ChatComposer { } } - // Check if any placeholders were removed and remove their corresponding pending pastes - self.pending_pastes - .retain(|(placeholder, _)| text_after.contains(placeholder)); - - // Keep attached images in proportion to how many matching placeholders exist in the text. - // This handles duplicate placeholders that share the same visible label. - if !self.attached_images.is_empty() { - let mut needed: HashMap = HashMap::new(); - for img in &self.attached_images { - needed - .entry(img.placeholder.clone()) - .or_insert_with(|| text_after.matches(&img.placeholder).count()); - } - - let mut used: HashMap = HashMap::new(); - let mut kept: Vec = Vec::with_capacity(self.attached_images.len()); - for img in self.attached_images.drain(..) { - let total_needed = *needed.get(&img.placeholder).unwrap_or(&0); - let used_count = used.entry(img.placeholder.clone()).or_insert(0); - if *used_count < total_needed { - kept.push(img); - *used_count += 1; - } - } - self.attached_images = kept; - } - (InputResult::None, true) } - /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. - /// Returns true if a placeholder was removed. - fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { - // Clamp the cursor to a valid char boundary to avoid panics when slicing. - let text = self.textarea.text(); - let p = Self::clamp_to_char_boundary(text, self.textarea.cursor()); - - // Try image placeholders first - let mut out: Option<(usize, String)> = None; - // Detect if the cursor is at the end of any image placeholder. - // If duplicates exist, remove the specific occurrence's mapping. - for (i, img) in self.attached_images.iter().enumerate() { - let ph = &img.placeholder; - if p < ph.len() { - continue; - } - let start = p - ph.len(); - if text.get(start..p) != Some(ph.as_str()) { - continue; - } - - // Count the number of occurrences of `ph` before `start`. - let mut occ_before = 0usize; - let mut search_pos = 0usize; - while search_pos < start { - let segment = match text.get(search_pos..start) { - Some(s) => s, - None => break, - }; - if let Some(found) = segment.find(ph) { - occ_before += 1; - search_pos += found + ph.len(); - } else { - break; - } - } - - // Remove the occ_before-th attached image that shares this placeholder label. - out = if let Some((remove_idx, _)) = self - .attached_images - .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) - { - Some((remove_idx, ph.clone())) - } else { - Some((i, ph.clone())) - }; - break; - } - if let Some((idx, placeholder)) = out { - self.textarea.replace_range(p - placeholder.len()..p, ""); - self.attached_images.remove(idx); - return true; + fn try_remove_image_element_at_cursor_start(&mut self) -> bool { + if self.attached_images.is_empty() { + return false; } - // Also handle when the cursor is at the START of an image placeholder. - // let result = 'out: { - let out: Option<(usize, String)> = 'out: { - for (i, img) in self.attached_images.iter().enumerate() { - let ph = &img.placeholder; - if p + ph.len() > text.len() { - continue; - } - if text.get(p..p + ph.len()) != Some(ph.as_str()) { - continue; - } - - // Count occurrences of `ph` before `p`. - let mut occ_before = 0usize; - let mut search_pos = 0usize; - while search_pos < p { - let segment = match text.get(search_pos..p) { - Some(s) => s, - None => break 'out None, - }; - if let Some(found) = segment.find(ph) { - occ_before += 1; - search_pos += found + ph.len(); - } else { - break 'out None; - } - } - - if let Some((remove_idx, _)) = self - .attached_images - .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) - { - break 'out Some((remove_idx, ph.clone())); - } else { - break 'out Some((i, ph.clone())); - } - } - None + let p = self.textarea.cursor(); + let Some(payload) = self.textarea.element_payload_starting_at(p) else { + return false; + }; + let Some(idx) = self + .attached_images + .iter() + .position(|img| img.placeholder == payload) + else { + return false; }; - if let Some((idx, placeholder)) = out { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.attached_images.remove(idx); - return true; + self.textarea.replace_range(p..p + payload.len(), ""); + self.attached_images.remove(idx); + self.relabel_attached_images_and_update_placeholders(); + true + } + + fn reconcile_deleted_elements(&mut self, elements_before: Vec) { + let elements_after: HashSet = + self.textarea.element_payloads().into_iter().collect(); + + let mut removed_any_image = false; + for removed in elements_before + .into_iter() + .filter(|payload| !elements_after.contains(payload)) + { + self.pending_pastes.retain(|(ph, _)| ph != &removed); + + if let Some(idx) = self + .attached_images + .iter() + .position(|img| img.placeholder == removed) + { + self.attached_images.remove(idx); + removed_any_image = true; + } } - // Then try pasted-content placeholders - if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { - if p < ph.len() { - return None; - } - let start = p - ph.len(); - if text.get(start..p) == Some(ph.as_str()) { - Some(ph.clone()) - } else { - None - } - }) { - self.textarea.replace_range(p - placeholder.len()..p, ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; + if removed_any_image { + self.relabel_attached_images_and_update_placeholders(); } + } - // Also handle when the cursor is at the START of a pasted-content placeholder. - if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { - if p + ph.len() > text.len() { - return None; + fn relabel_attached_images_and_update_placeholders(&mut self) { + for idx in 0..self.attached_images.len() { + let expected = local_image_label_text(idx + 1); + let current = self.attached_images[idx].placeholder.clone(); + if current == expected { + continue; } - if text.get(p..p + ph.len()) == Some(ph.as_str()) { - Some(ph.clone()) - } else { - None - } - }) { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; + + self.attached_images[idx].placeholder = expected.clone(); + let _renamed = self.textarea.replace_element_payload(¤t, &expected); } - - false } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -3477,12 +3358,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone(), 32, 16, "PNG"); + composer.attach_image(path.clone()); composer.handle_paste(" hi".into()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image1.png 32x16] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3501,11 +3382,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3526,21 +3407,15 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image_dup.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); composer.handle_paste(" ".into()); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); let text = composer.textarea.text().to_string(); - assert!(text.contains("[image_dup.png 10x5]")); - assert!(text.contains("[image_dup.png 10x5 #2]")); - assert_eq!( - composer.attached_images[0].placeholder, - "[image_dup.png 10x5]" - ); - assert_eq!( - composer.attached_images[1].placeholder, - "[image_dup.png 10x5 #2]" - ); + assert!(text.contains("[Image #1]")); + assert!(text.contains("[Image #2]")); + assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); + assert_eq!(composer.attached_images[1].placeholder, "[Image #2]"); } #[test] @@ -3555,7 +3430,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone(), 20, 10, "PNG"); + composer.attach_image(path.clone()); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3566,7 +3441,7 @@ mod tests { // Re-add and test backspace in middle: should break the placeholder string // and drop the image mapping (same as text placeholder behavior). - composer.attach_image(path, 20, 10, "PNG"); + composer.attach_image(path); let placeholder2 = composer.attached_images[0].placeholder.clone(); // Move cursor to roughly middle of placeholder if let Some(start_pos) = composer.textarea.text().find(&placeholder2) { @@ -3598,7 +3473,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3607,16 +3482,11 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); assert_eq!(composer.attached_images.len(), 1); - assert!( - composer - .textarea - .text() - .starts_with("[image_multibyte.png 10x5]") - ); + assert!(composer.textarea.text().starts_with("[Image #1]")); } #[test] - fn deleting_one_of_duplicate_image_placeholders_removes_matching_entry() { + fn deleting_one_of_duplicate_image_placeholders_removes_one_entry() { let (tx, _rx) = unbounded_channel::(); let sender = AppEventSender::new(tx); let mut composer = ChatComposer::new( @@ -3630,10 +3500,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1, 10, 5, "PNG"); + composer.attach_image(path1); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone(), 10, 5, "PNG"); + composer.attach_image(path2.clone()); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3647,25 +3517,66 @@ mod tests { let new_text = composer.textarea.text().to_string(); assert_eq!( - 0, + 1, new_text.matches(&placeholder1).count(), - "first placeholder removed" + "one placeholder remains after deletion" + ); + assert_eq!( + 0, + new_text.matches(&placeholder2).count(), + "second placeholder was relabeled" ); assert_eq!( 1, - new_text.matches(&placeholder2).count(), - "second placeholder remains" + new_text.matches("[Image #1]").count(), + "remaining placeholder relabeled to #1" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[image_dup2.png 10x5]".to_string() + placeholder: "[Image #1]".to_string() }], composer.attached_images, "one image mapping remains" ); } + #[test] + fn deleting_first_text_element_renumbers_following_text_element() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let path1 = PathBuf::from("/tmp/image_first.png"); + let path2 = PathBuf::from("/tmp/image_second.png"); + + // Insert two adjacent atomic elements. + composer.attach_image(path1); + composer.attach_image(path2.clone()); + assert_eq!(composer.textarea.text(), "[Image #1][Image #2]"); + assert_eq!(composer.attached_images.len(), 2); + + // Delete the first element using normal textarea editing (Delete at cursor start). + composer.textarea.set_cursor(0); + composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + + // Remaining image should be renumbered and the textarea element updated. + assert_eq!(composer.attached_images.len(), 1); + assert_eq!(composer.attached_images[0].path, path2); + assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); + assert_eq!(composer.textarea.text(), "[Image #1]"); + } + #[test] fn pasting_filepath_attaches_image() { let tmp = tempdir().expect("create TempDir"); @@ -3686,12 +3597,7 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!( - composer - .textarea - .text() - .starts_with("[codex_tui_test_paste_image.png 3x2] ") - ); + assert!(composer.textarea.text().starts_with("[Image #1] ")); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index 8579597865..f795ea64f1 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -542,16 +542,9 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { + pub(crate) fn attach_image(&mut self, path: PathBuf) { if self.view_stack.is_empty() { - self.composer - .attach_image(path, width, height, format_label); + self.composer.attach_image(path); self.request_redraw(); } } diff --git a/codex-rs/tui/src/bottom_pane/textarea.rs b/codex-rs/tui/src/bottom_pane/textarea.rs index 4fc673a11d..f2ed40758d 100644 --- a/codex-rs/tui/src/bottom_pane/textarea.rs +++ b/codex-rs/tui/src/bottom_pane/textarea.rs @@ -715,6 +715,88 @@ impl TextArea { // ===== Text elements support ===== + pub fn element_payloads(&self) -> Vec { + self.elements + .iter() + .filter_map(|e| self.text.get(e.range.clone()).map(str::to_string)) + .collect() + } + + pub fn element_payload_starting_at(&self, pos: usize) -> Option { + let pos = pos.min(self.text.len()); + let elem = self.elements.iter().find(|e| e.range.start == pos)?; + self.text.get(elem.range.clone()).map(str::to_string) + } + + /// Renames a single text element in-place, keeping it atomic. + /// + /// This is intended for cases where the element payload is an identifier (e.g. a placeholder) + /// that must be updated without converting the element back into normal text. + pub fn replace_element_payload(&mut self, old: &str, new: &str) -> bool { + let Some(idx) = self + .elements + .iter() + .position(|e| self.text.get(e.range.clone()) == Some(old)) + else { + return false; + }; + + let range = self.elements[idx].range.clone(); + let start = range.start; + let end = range.end; + if start > end || end > self.text.len() { + return false; + } + + let removed_len = end - start; + let inserted_len = new.len(); + let diff = inserted_len as isize - removed_len as isize; + + self.text.replace_range(range, new); + self.wrap_cache.replace(None); + self.preferred_col = None; + + // Update the modified element's range. + self.elements[idx].range = start..(start + inserted_len); + + // Shift element ranges that occur after the replaced element. + if diff != 0 { + for (j, e) in self.elements.iter_mut().enumerate() { + if j == idx { + continue; + } + if e.range.end <= start { + continue; + } + if e.range.start >= end { + e.range.start = ((e.range.start as isize) + diff) as usize; + e.range.end = ((e.range.end as isize) + diff) as usize; + continue; + } + + // Elements should not partially overlap each other; degrade gracefully by + // snapping anything intersecting the replaced range to the new bounds. + e.range.start = start.min(e.range.start); + e.range.end = (start + inserted_len).max(e.range.end.saturating_add_signed(diff)); + } + } + + // Update the cursor position to account for the edit. + self.cursor_pos = if self.cursor_pos < start { + self.cursor_pos + } else if self.cursor_pos <= end { + start + inserted_len + } else { + ((self.cursor_pos as isize) + diff) as usize + }; + self.cursor_pos = self.clamp_pos_to_nearest_boundary(self.cursor_pos); + + // Keep element ordering deterministic. + self.elements.sort_by_key(|e| e.range.start); + + true + } + pub fn insert_element(&mut self, text: &str) { let start = self.clamp_pos_for_insertion(self.cursor_pos); self.insert_str_at(start, text); diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 16685793b5..e3fd7a891e 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1593,12 +1593,13 @@ impl ChatWidget { { match paste_image_to_temp_png() { Ok((path, info)) => { - self.attach_image( - path, + tracing::debug!( + "pasted image size={}x{} format={}", info.width, info.height, - info.encoded_format.label(), + info.encoded_format.label() ); + self.attach_image(path); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1651,18 +1652,9 @@ impl ChatWidget { } } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { - tracing::info!( - "attach_image path={path:?} width={width} height={height} format={format_label}", - ); - self.bottom_pane - .attach_image(path, width, height, format_label); + pub(crate) fn attach_image(&mut self, path: PathBuf) { + tracing::info!("attach_image path={path:?}"); + self.bottom_pane.attach_image(path); self.request_redraw(); } @@ -2012,14 +2004,14 @@ impl ChatWidget { return; } - if !text.is_empty() { - items.push(UserInput::Text { text: text.clone() }); - } - for path in image_paths { items.push(UserInput::LocalImage { path }); } + if !text.is_empty() { + items.push(UserInput::Text { text: text.clone() }); + } + if let Some(skills) = self.bottom_pane.skills() { let skill_mentions = find_skill_mentions(&text, skills); for skill in skill_mentions { diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 3ec8f9e2fb..5d1ac36638 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -1114,8 +1114,8 @@ async fn ctrl_c_cleared_prompt_is_recoverable_via_history() { chat.bottom_pane.insert_str("draft message "); chat.bottom_pane - .attach_image(PathBuf::from("/tmp/preview.png"), 24, 42, "png"); - let placeholder = "[preview.png 24x42]"; + .attach_image(PathBuf::from("/tmp/preview.png")); + let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder), "expected placeholder {placeholder:?} in composer text" diff --git a/codex-rs/tui2/src/bottom_pane/chat_composer.rs b/codex-rs/tui2/src/bottom_pane/chat_composer.rs index 22e62bb4fb..118dc8cd3d 100644 --- a/codex-rs/tui2/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui2/src/bottom_pane/chat_composer.rs @@ -49,6 +49,7 @@ use crate::style::user_message_style; use codex_common::fuzzy_match::fuzzy_match; use codex_protocol::custom_prompts::CustomPrompt; use codex_protocol::custom_prompts::PROMPTS_CMD_PREFIX; +use codex_protocol::models::local_image_label_text; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; @@ -62,7 +63,7 @@ use codex_core::skills::model::SkillMetadata; use codex_file_search::FileMatch; use std::cell::RefCell; use std::collections::HashMap; -use std::path::Path; +use std::collections::HashSet; use std::path::PathBuf; use std::time::Duration; use std::time::Instant; @@ -287,10 +288,12 @@ impl ChatComposer { // normalize_pasted_path already handles Windows → WSL path conversion, // so we can directly try to read the image dimensions. match image::image_dimensions(&path_buf) { - Ok((w, h)) => { + Ok((width, height)) => { tracing::info!("OK: {pasted}"); - let format_label = pasted_image_format(&path_buf).label(); - self.attach_image(path_buf, w, h, format_label); + tracing::debug!("image dimensions={}x{}", width, height); + let format = pasted_image_format(&path_buf); + tracing::debug!("attached image format={}", format.label()); + self.attach_image(path_buf); true } Err(err) => { @@ -342,12 +345,9 @@ impl ChatComposer { } /// Attempt to start a burst by retro-capturing recent chars before the cursor. - pub fn attach_image(&mut self, path: PathBuf, width: u32, height: u32, _format_label: &str) { - let file_label = path - .file_name() - .map(|name| name.to_string_lossy().into_owned()) - .unwrap_or_else(|| "image".to_string()); - let placeholder = format!("[{file_label} {width}x{height}]"); + pub fn attach_image(&mut self, path: PathBuf) { + let image_number = self.attached_images.len() + 1; + let placeholder = local_image_label_text(image_number); // Insert as an element to match large paste placeholder behavior: // styled distinctly and treated atomically for cursor/mutations. self.textarea.insert_element(&placeholder); @@ -735,47 +735,43 @@ impl ChatComposer { if is_image { // Determine dimensions; if that fails fall back to normal path insertion. let path_buf = PathBuf::from(&sel_path); - if let Ok((w, h)) = image::image_dimensions(&path_buf) { - // Remove the current @token (mirror logic from insert_selected_path without inserting text) - // using the flat text and byte-offset cursor API. - let cursor_offset = self.textarea.cursor(); - let text = self.textarea.text(); - // Clamp to a valid char boundary to avoid panics when slicing. - let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); - let before_cursor = &text[..safe_cursor]; - let after_cursor = &text[safe_cursor..]; + match image::image_dimensions(&path_buf) { + Ok((width, height)) => { + tracing::debug!("selected image dimensions={}x{}", width, height); + // Remove the current @token (mirror logic from insert_selected_path without inserting text) + // using the flat text and byte-offset cursor API. + let cursor_offset = self.textarea.cursor(); + let text = self.textarea.text(); + // Clamp to a valid char boundary to avoid panics when slicing. + let safe_cursor = Self::clamp_to_char_boundary(text, cursor_offset); + let before_cursor = &text[..safe_cursor]; + let after_cursor = &text[safe_cursor..]; - // Determine token boundaries in the full text. - let start_idx = before_cursor - .char_indices() - .rfind(|(_, c)| c.is_whitespace()) - .map(|(idx, c)| idx + c.len_utf8()) - .unwrap_or(0); - let end_rel_idx = after_cursor - .char_indices() - .find(|(_, c)| c.is_whitespace()) - .map(|(idx, _)| idx) - .unwrap_or(after_cursor.len()); - let end_idx = safe_cursor + end_rel_idx; + // Determine token boundaries in the full text. + let start_idx = before_cursor + .char_indices() + .rfind(|(_, c)| c.is_whitespace()) + .map(|(idx, c)| idx + c.len_utf8()) + .unwrap_or(0); + let end_rel_idx = after_cursor + .char_indices() + .find(|(_, c)| c.is_whitespace()) + .map(|(idx, _)| idx) + .unwrap_or(after_cursor.len()); + let end_idx = safe_cursor + end_rel_idx; - self.textarea.replace_range(start_idx..end_idx, ""); - self.textarea.set_cursor(start_idx); + self.textarea.replace_range(start_idx..end_idx, ""); + self.textarea.set_cursor(start_idx); - let format_label = match Path::new(&sel_path) - .extension() - .and_then(|e| e.to_str()) - .map(str::to_ascii_lowercase) - { - Some(ext) if ext == "png" => "PNG", - Some(ext) if ext == "jpg" || ext == "jpeg" => "JPEG", - _ => "IMG", - }; - self.attach_image(path_buf, w, h, format_label); - // Add a trailing space to keep typing fluid. - self.textarea.insert_str(" "); - } else { - // Fallback to plain path insertion if metadata read fails. - self.insert_selected_path(&sel_path); + self.attach_image(path_buf); + // Add a trailing space to keep typing fluid. + self.textarea.insert_str(" "); + } + Err(err) => { + tracing::trace!("image dimensions lookup failed: {err}"); + // Fallback to plain path insertion if metadata read fails. + self.insert_selected_path(&sel_path); + } } } else { // Non-image: inserting file path. @@ -1381,20 +1377,28 @@ impl ChatComposer { } } - // For non-char inputs (or after flushing), handle normally. - // Special handling for backspace on placeholders - if let KeyEvent { - code: KeyCode::Backspace, - .. - } = input - && self.try_remove_any_placeholder_at_cursor() + // Backspace at the start of an image placeholder should delete that placeholder (rather + // than deleting content before it). Do this without scanning the full text by consulting + // the textarea's element list. + if matches!(input.code, KeyCode::Backspace) + && self.try_remove_image_element_at_cursor_start() { return (InputResult::None, true); } - // Normal input handling + // Track element removals so we can drop any corresponding placeholders without scanning + // the full text. (Placeholders are atomic elements; when deleted, the element disappears.) + let elements_before = if self.pending_pastes.is_empty() && self.attached_images.is_empty() { + None + } else { + Some(self.textarea.element_payloads()) + }; + self.textarea.input(input); - let text_after = self.textarea.text(); + + if let Some(elements_before) = elements_before { + self.reconcile_deleted_elements(elements_before); + } // Update paste-burst heuristic for plain Char (no Ctrl/Alt) events. let crossterm::event::KeyEvent { @@ -1416,176 +1420,69 @@ impl ChatComposer { } } - // Check if any placeholders were removed and remove their corresponding pending pastes - self.pending_pastes - .retain(|(placeholder, _)| text_after.contains(placeholder)); - - // Keep attached images in proportion to how many matching placeholders exist in the text. - // This handles duplicate placeholders that share the same visible label. - if !self.attached_images.is_empty() { - let mut needed: HashMap = HashMap::new(); - for img in &self.attached_images { - needed - .entry(img.placeholder.clone()) - .or_insert_with(|| text_after.matches(&img.placeholder).count()); - } - - let mut used: HashMap = HashMap::new(); - let mut kept: Vec = Vec::with_capacity(self.attached_images.len()); - for img in self.attached_images.drain(..) { - let total_needed = *needed.get(&img.placeholder).unwrap_or(&0); - let used_count = used.entry(img.placeholder.clone()).or_insert(0); - if *used_count < total_needed { - kept.push(img); - *used_count += 1; - } - } - self.attached_images = kept; - } - (InputResult::None, true) } - /// Attempts to remove an image or paste placeholder if the cursor is at the end of one. - /// Returns true if a placeholder was removed. - fn try_remove_any_placeholder_at_cursor(&mut self) -> bool { - // Clamp the cursor to a valid char boundary to avoid panics when slicing. - let text = self.textarea.text(); - let p = Self::clamp_to_char_boundary(text, self.textarea.cursor()); - - // Try image placeholders first - let mut out: Option<(usize, String)> = None; - // Detect if the cursor is at the end of any image placeholder. - // If duplicates exist, remove the specific occurrence's mapping. - for (i, img) in self.attached_images.iter().enumerate() { - let ph = &img.placeholder; - if p < ph.len() { - continue; - } - let start = p - ph.len(); - if text.get(start..p) != Some(ph.as_str()) { - continue; - } - - // Count the number of occurrences of `ph` before `start`. - let mut occ_before = 0usize; - let mut search_pos = 0usize; - while search_pos < start { - let segment = match text.get(search_pos..start) { - Some(s) => s, - None => break, - }; - if let Some(found) = segment.find(ph) { - occ_before += 1; - search_pos += found + ph.len(); - } else { - break; - } - } - - // Remove the occ_before-th attached image that shares this placeholder label. - out = if let Some((remove_idx, _)) = self - .attached_images - .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) - { - Some((remove_idx, ph.clone())) - } else { - Some((i, ph.clone())) - }; - break; - } - if let Some((idx, placeholder)) = out { - self.textarea.replace_range(p - placeholder.len()..p, ""); - self.attached_images.remove(idx); - return true; + fn try_remove_image_element_at_cursor_start(&mut self) -> bool { + if self.attached_images.is_empty() { + return false; } - // Also handle when the cursor is at the START of an image placeholder. - // let result = 'out: { - let out: Option<(usize, String)> = 'out: { - for (i, img) in self.attached_images.iter().enumerate() { - let ph = &img.placeholder; - if p + ph.len() > text.len() { - continue; - } - if text.get(p..p + ph.len()) != Some(ph.as_str()) { - continue; - } - - // Count occurrences of `ph` before `p`. - let mut occ_before = 0usize; - let mut search_pos = 0usize; - while search_pos < p { - let segment = match text.get(search_pos..p) { - Some(s) => s, - None => break 'out None, - }; - if let Some(found) = segment.find(ph) { - occ_before += 1; - search_pos += found + ph.len(); - } else { - break 'out None; - } - } - - if let Some((remove_idx, _)) = self - .attached_images - .iter() - .enumerate() - .filter(|(_, img2)| img2.placeholder == *ph) - .nth(occ_before) - { - break 'out Some((remove_idx, ph.clone())); - } else { - break 'out Some((i, ph.clone())); - } - } - None + let p = self.textarea.cursor(); + let Some(payload) = self.textarea.element_payload_starting_at(p) else { + return false; + }; + let Some(idx) = self + .attached_images + .iter() + .position(|img| img.placeholder == payload) + else { + return false; }; - if let Some((idx, placeholder)) = out { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.attached_images.remove(idx); - return true; + self.textarea.replace_range(p..p + payload.len(), ""); + self.attached_images.remove(idx); + self.relabel_attached_images_and_update_placeholders(); + true + } + + fn reconcile_deleted_elements(&mut self, elements_before: Vec) { + let elements_after: HashSet = + self.textarea.element_payloads().into_iter().collect(); + + let mut removed_any_image = false; + for removed in elements_before + .into_iter() + .filter(|payload| !elements_after.contains(payload)) + { + self.pending_pastes.retain(|(ph, _)| ph != &removed); + + if let Some(idx) = self + .attached_images + .iter() + .position(|img| img.placeholder == removed) + { + self.attached_images.remove(idx); + removed_any_image = true; + } } - // Then try pasted-content placeholders - if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { - if p < ph.len() { - return None; - } - let start = p - ph.len(); - if text.get(start..p) == Some(ph.as_str()) { - Some(ph.clone()) - } else { - None - } - }) { - self.textarea.replace_range(p - placeholder.len()..p, ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; + if removed_any_image { + self.relabel_attached_images_and_update_placeholders(); } + } - // Also handle when the cursor is at the START of a pasted-content placeholder. - if let Some(placeholder) = self.pending_pastes.iter().find_map(|(ph, _)| { - if p + ph.len() > text.len() { - return None; + fn relabel_attached_images_and_update_placeholders(&mut self) { + for idx in 0..self.attached_images.len() { + let expected = local_image_label_text(idx + 1); + let current = self.attached_images[idx].placeholder.clone(); + if current == expected { + continue; } - if text.get(p..p + ph.len()) == Some(ph.as_str()) { - Some(ph.clone()) - } else { - None - } - }) { - self.textarea.replace_range(p..p + placeholder.len(), ""); - self.pending_pastes.retain(|(ph, _)| ph != &placeholder); - return true; + + self.attached_images[idx].placeholder = expected.clone(); + let _renamed = self.textarea.replace_element_payload(¤t, &expected); } - - false } fn handle_shortcut_overlay_key(&mut self, key_event: &KeyEvent) -> bool { @@ -3408,12 +3305,12 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image1.png"); - composer.attach_image(path.clone(), 32, 16, "PNG"); + composer.attach_image(path.clone()); composer.handle_paste(" hi".into()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image1.png 32x16] hi"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1] hi"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3432,11 +3329,11 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image2.png"); - composer.attach_image(path.clone(), 10, 5, "PNG"); + composer.attach_image(path.clone()); let (result, _) = composer.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE)); match result { - InputResult::Submitted(text) => assert_eq!(text, "[image2.png 10x5]"), + InputResult::Submitted(text) => assert_eq!(text, "[Image #1]"), _ => panic!("expected Submitted"), } let imgs = composer.take_recent_submission_images(); @@ -3457,7 +3354,7 @@ mod tests { false, ); let path = PathBuf::from("/tmp/image3.png"); - composer.attach_image(path.clone(), 20, 10, "PNG"); + composer.attach_image(path.clone()); let placeholder = composer.attached_images[0].placeholder.clone(); // Case 1: backspace at end @@ -3468,7 +3365,7 @@ mod tests { // Re-add and test backspace in middle: should break the placeholder string // and drop the image mapping (same as text placeholder behavior). - composer.attach_image(path, 20, 10, "PNG"); + composer.attach_image(path); let placeholder2 = composer.attached_images[0].placeholder.clone(); // Move cursor to roughly middle of placeholder if let Some(start_pos) = composer.textarea.text().find(&placeholder2) { @@ -3500,7 +3397,7 @@ mod tests { // Insert an image placeholder at the start let path = PathBuf::from("/tmp/image_multibyte.png"); - composer.attach_image(path, 10, 5, "PNG"); + composer.attach_image(path); // Add multibyte text after the placeholder composer.textarea.insert_str("日本語"); @@ -3509,12 +3406,7 @@ mod tests { composer.handle_key_event(KeyEvent::new(KeyCode::Backspace, KeyModifiers::NONE)); assert_eq!(composer.attached_images.len(), 1); - assert!( - composer - .textarea - .text() - .starts_with("[image_multibyte.png 10x5]") - ); + assert!(composer.textarea.text().starts_with("[Image #1]")); } #[test] @@ -3532,10 +3424,10 @@ mod tests { let path1 = PathBuf::from("/tmp/image_dup1.png"); let path2 = PathBuf::from("/tmp/image_dup2.png"); - composer.attach_image(path1, 10, 5, "PNG"); + composer.attach_image(path1); // separate placeholders with a space for clarity composer.handle_paste(" ".into()); - composer.attach_image(path2.clone(), 10, 5, "PNG"); + composer.attach_image(path2.clone()); let placeholder1 = composer.attached_images[0].placeholder.clone(); let placeholder2 = composer.attached_images[1].placeholder.clone(); @@ -3550,24 +3442,60 @@ mod tests { let new_text = composer.textarea.text().to_string(); assert_eq!( 0, - new_text.matches(&placeholder1).count(), - "first placeholder removed" + new_text.matches(&placeholder2).count(), + "second placeholder was relabeled" ); assert_eq!( 1, - new_text.matches(&placeholder2).count(), - "second placeholder remains" + new_text.matches("[Image #1]").count(), + "remaining placeholder relabeled to #1" ); assert_eq!( vec![AttachedImage { path: path2, - placeholder: "[image_dup2.png 10x5]".to_string() + placeholder: "[Image #1]".to_string() }], composer.attached_images, "one image mapping remains" ); } + #[test] + fn deleting_first_text_element_renumbers_following_text_element() { + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = unbounded_channel::(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new( + true, + sender, + false, + "Ask Codex to do anything".to_string(), + false, + ); + + let path1 = PathBuf::from("/tmp/image_first.png"); + let path2 = PathBuf::from("/tmp/image_second.png"); + + // Insert two adjacent atomic elements. + composer.attach_image(path1); + composer.attach_image(path2.clone()); + assert_eq!(composer.textarea.text(), "[Image #1][Image #2]"); + assert_eq!(composer.attached_images.len(), 2); + + // Delete the first element using normal textarea editing (Delete at cursor start). + composer.textarea.set_cursor(0); + composer.handle_key_event(KeyEvent::new(KeyCode::Delete, KeyModifiers::NONE)); + + // Remaining image should be renumbered and the textarea element updated. + assert_eq!(composer.attached_images.len(), 1); + assert_eq!(composer.attached_images[0].path, path2); + assert_eq!(composer.attached_images[0].placeholder, "[Image #1]"); + assert_eq!(composer.textarea.text(), "[Image #1]"); + } + #[test] fn pasting_filepath_attaches_image() { let tmp = tempdir().expect("create TempDir"); @@ -3588,12 +3516,7 @@ mod tests { let needs_redraw = composer.handle_paste(tmp_path.to_string_lossy().to_string()); assert!(needs_redraw); - assert!( - composer - .textarea - .text() - .starts_with("[codex_tui_test_paste_image.png 3x2] ") - ); + assert!(composer.textarea.text().starts_with("[Image #1] ")); let imgs = composer.take_recent_submission_images(); assert_eq!(imgs, vec![tmp_path]); diff --git a/codex-rs/tui2/src/bottom_pane/mod.rs b/codex-rs/tui2/src/bottom_pane/mod.rs index 8258d6b50d..887fb3e64d 100644 --- a/codex-rs/tui2/src/bottom_pane/mod.rs +++ b/codex-rs/tui2/src/bottom_pane/mod.rs @@ -529,16 +529,9 @@ impl BottomPane { self.request_redraw(); } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { + pub(crate) fn attach_image(&mut self, path: PathBuf) { if self.view_stack.is_empty() { - self.composer - .attach_image(path, width, height, format_label); + self.composer.attach_image(path); self.request_redraw(); } } diff --git a/codex-rs/tui2/src/bottom_pane/textarea.rs b/codex-rs/tui2/src/bottom_pane/textarea.rs index 4fc673a11d..f2ed40758d 100644 --- a/codex-rs/tui2/src/bottom_pane/textarea.rs +++ b/codex-rs/tui2/src/bottom_pane/textarea.rs @@ -715,6 +715,88 @@ impl TextArea { // ===== Text elements support ===== + pub fn element_payloads(&self) -> Vec { + self.elements + .iter() + .filter_map(|e| self.text.get(e.range.clone()).map(str::to_string)) + .collect() + } + + pub fn element_payload_starting_at(&self, pos: usize) -> Option { + let pos = pos.min(self.text.len()); + let elem = self.elements.iter().find(|e| e.range.start == pos)?; + self.text.get(elem.range.clone()).map(str::to_string) + } + + /// Renames a single text element in-place, keeping it atomic. + /// + /// This is intended for cases where the element payload is an identifier (e.g. a placeholder) + /// that must be updated without converting the element back into normal text. + pub fn replace_element_payload(&mut self, old: &str, new: &str) -> bool { + let Some(idx) = self + .elements + .iter() + .position(|e| self.text.get(e.range.clone()) == Some(old)) + else { + return false; + }; + + let range = self.elements[idx].range.clone(); + let start = range.start; + let end = range.end; + if start > end || end > self.text.len() { + return false; + } + + let removed_len = end - start; + let inserted_len = new.len(); + let diff = inserted_len as isize - removed_len as isize; + + self.text.replace_range(range, new); + self.wrap_cache.replace(None); + self.preferred_col = None; + + // Update the modified element's range. + self.elements[idx].range = start..(start + inserted_len); + + // Shift element ranges that occur after the replaced element. + if diff != 0 { + for (j, e) in self.elements.iter_mut().enumerate() { + if j == idx { + continue; + } + if e.range.end <= start { + continue; + } + if e.range.start >= end { + e.range.start = ((e.range.start as isize) + diff) as usize; + e.range.end = ((e.range.end as isize) + diff) as usize; + continue; + } + + // Elements should not partially overlap each other; degrade gracefully by + // snapping anything intersecting the replaced range to the new bounds. + e.range.start = start.min(e.range.start); + e.range.end = (start + inserted_len).max(e.range.end.saturating_add_signed(diff)); + } + } + + // Update the cursor position to account for the edit. + self.cursor_pos = if self.cursor_pos < start { + self.cursor_pos + } else if self.cursor_pos <= end { + start + inserted_len + } else { + ((self.cursor_pos as isize) + diff) as usize + }; + self.cursor_pos = self.clamp_pos_to_nearest_boundary(self.cursor_pos); + + // Keep element ordering deterministic. + self.elements.sort_by_key(|e| e.range.start); + + true + } + pub fn insert_element(&mut self, text: &str) { let start = self.clamp_pos_for_insertion(self.cursor_pos); self.insert_str_at(start, text); diff --git a/codex-rs/tui2/src/chatwidget.rs b/codex-rs/tui2/src/chatwidget.rs index 12e6d0d896..f5af1bfe67 100644 --- a/codex-rs/tui2/src/chatwidget.rs +++ b/codex-rs/tui2/src/chatwidget.rs @@ -1452,12 +1452,13 @@ impl ChatWidget { { match paste_image_to_temp_png() { Ok((path, info)) => { - self.attach_image( - path, + tracing::debug!( + "pasted image size={}x{} format={}", info.width, info.height, - info.encoded_format.label(), + info.encoded_format.label() ); + self.attach_image(path); } Err(err) => { tracing::warn!("failed to paste image: {err}"); @@ -1510,18 +1511,9 @@ impl ChatWidget { } } - pub(crate) fn attach_image( - &mut self, - path: PathBuf, - width: u32, - height: u32, - format_label: &str, - ) { - tracing::info!( - "attach_image path={path:?} width={width} height={height} format={format_label}", - ); - self.bottom_pane - .attach_image(path, width, height, format_label); + pub(crate) fn attach_image(&mut self, path: PathBuf) { + tracing::info!("attach_image path={path:?}"); + self.bottom_pane.attach_image(path); self.request_redraw(); } @@ -1818,14 +1810,14 @@ impl ChatWidget { return; } - if !text.is_empty() { - items.push(UserInput::Text { text: text.clone() }); - } - for path in image_paths { items.push(UserInput::LocalImage { path }); } + if !text.is_empty() { + items.push(UserInput::Text { text: text.clone() }); + } + if let Some(skills) = self.bottom_pane.skills() { let skill_mentions = find_skill_mentions(&text, skills); for skill in skill_mentions { diff --git a/codex-rs/tui2/src/chatwidget/tests.rs b/codex-rs/tui2/src/chatwidget/tests.rs index 45a4247f63..9b7a825e0e 100644 --- a/codex-rs/tui2/src/chatwidget/tests.rs +++ b/codex-rs/tui2/src/chatwidget/tests.rs @@ -1065,8 +1065,8 @@ async fn ctrl_c_cleared_prompt_is_recoverable_via_history() { chat.bottom_pane.insert_str("draft message "); chat.bottom_pane - .attach_image(PathBuf::from("/tmp/preview.png"), 24, 42, "png"); - let placeholder = "[preview.png 24x42]"; + .attach_image(PathBuf::from("/tmp/preview.png")); + let placeholder = "[Image #1]"; assert!( chat.bottom_pane.composer_text().ends_with(placeholder), "expected placeholder {placeholder:?} in composer text" From 0728d23c511fab9b1ae29b2980bfd28308850752 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sat, 10 Jan 2026 08:25:38 -0800 Subject: [PATCH 2/2] fix: update tests so `bazel test //codex-rs/core:core-all-test` passes on my Mac --- codex-rs/core/tests/common/responses.rs | 20 +- codex-rs/core/tests/suite/approvals.rs | 55 +++++- .../core/tests/suite/shell_serialization.rs | 1 + codex-rs/core/tests/suite/unified_exec.rs | 177 +++++++++--------- 4 files changed, 158 insertions(+), 95 deletions(-) diff --git a/codex-rs/core/tests/common/responses.rs b/codex-rs/core/tests/common/responses.rs index 710d03fc7f..8a68b3e938 100644 --- a/codex-rs/core/tests/common/responses.rs +++ b/codex-rs/core/tests/common/responses.rs @@ -509,12 +509,17 @@ pub fn ev_apply_patch_function_call(call_id: &str, patch: &str) -> Value { } pub fn ev_shell_command_call(call_id: &str, command: &str) -> Value { - let args = serde_json::json!({ "command": command }); + let args = serde_json::json!({ "command": command, "login": false }); ev_shell_command_call_with_args(call_id, &args) } pub fn ev_shell_command_call_with_args(call_id: &str, args: &serde_json::Value) -> Value { - let arguments = serde_json::to_string(args).expect("serialize shell command arguments"); + let mut args = args.clone(); + if let serde_json::Value::Object(map) = &mut args { + map.entry("login".to_string()) + .or_insert_with(|| serde_json::Value::Bool(false)); + } + let arguments = serde_json::to_string(&args).expect("serialize shell command arguments"); ev_function_call(call_id, "shell_command", &arguments) } @@ -527,17 +532,18 @@ pub fn ev_apply_patch_shell_call(call_id: &str, patch: &str) -> Value { pub fn ev_apply_patch_shell_call_via_heredoc(call_id: &str, patch: &str) -> Value { let script = format!("apply_patch <<'EOF'\n{patch}\nEOF\n"); - let args = serde_json::json!({ "command": ["bash", "-lc", script] }); + let args = serde_json::json!({ "command": ["bash", "-c", script] }); let arguments = serde_json::to_string(&args).expect("serialize apply_patch arguments"); ev_function_call(call_id, "shell", &arguments) } pub fn ev_apply_patch_shell_command_call_via_heredoc(call_id: &str, patch: &str) -> Value { - let args = serde_json::json!({ "command": format!("apply_patch <<'EOF'\n{patch}\nEOF\n") }); - let arguments = serde_json::to_string(&args).expect("serialize apply_patch arguments"); - - ev_function_call(call_id, "shell_command", &arguments) + let args = serde_json::json!({ + "command": format!("apply_patch <<'EOF'\n{patch}\nEOF\n"), + "login": false, + }); + ev_shell_command_call_with_args(call_id, &args) } pub fn sse_failed(id: &str, code: &str, message: &str) -> String { diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 51f1c31f17..cda844e9e5 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -30,9 +30,11 @@ use pretty_assertions::assert_eq; use regex_lite::Regex; use serde_json::Value; use serde_json::json; -use std::env; use std::fs; +use std::path::Path; use std::path::PathBuf; +use std::sync::OnceLock; +use tempfile::Builder; use wiremock::Mock; use wiremock::MockServer; use wiremock::ResponseTemplate; @@ -45,6 +47,52 @@ enum TargetPath { OutsideWorkspace(&'static str), } +fn candidate_is_outside_tmp(candidate: &Path) -> bool { + let temp_dir = std::env::temp_dir(); + if candidate.starts_with(&temp_dir) { + return false; + } + if cfg!(unix) && candidate.starts_with(Path::new("/tmp")) { + return false; + } + if let Some(tmpdir) = std::env::var_os("TMPDIR") { + let tmpdir = PathBuf::from(tmpdir); + if candidate.starts_with(&tmpdir) { + return false; + } + } + true +} + +fn outside_workspace_root() -> &'static PathBuf { + static OUTSIDE_ROOT: OnceLock = OnceLock::new(); + OUTSIDE_ROOT.get_or_init(|| { + let mut candidates = Vec::new(); + if cfg!(unix) { + candidates.push(PathBuf::from("/var/tmp")); + } + if let Some(home) = std::env::var_os("HOME").or_else(|| std::env::var_os("USERPROFILE")) { + candidates.push(PathBuf::from(home)); + } + for candidate in candidates { + if !candidate.is_dir() || !candidate_is_outside_tmp(&candidate) { + continue; + } + if let Ok(dir) = Builder::new() + .prefix("codex-outside-") + .tempdir_in(&candidate) + { + return dir.keep(); + } + } + Builder::new() + .prefix("codex-outside-") + .tempdir() + .expect("create outside workspace temp dir") + .keep() + }) +} + impl TargetPath { fn resolve_for_patch(self, test: &TestCodex) -> (PathBuf, String) { match self { @@ -53,9 +101,7 @@ impl TargetPath { (path, name.to_string()) } TargetPath::OutsideWorkspace(name) => { - let path = env::current_dir() - .expect("current dir should be available") - .join(name); + let path = outside_workspace_root().join(name); (path.clone(), path.display().to_string()) } } @@ -187,6 +233,7 @@ fn shell_event( ) -> Result { let mut args = json!({ "command": command, + "login": false, "timeout_ms": timeout_ms, }); if sandbox_permissions.requires_escalated_permissions() { diff --git a/codex-rs/core/tests/suite/shell_serialization.rs b/codex-rs/core/tests/suite/shell_serialization.rs index 6969b6533d..1527946f5c 100644 --- a/codex-rs/core/tests/suite/shell_serialization.rs +++ b/codex-rs/core/tests/suite/shell_serialization.rs @@ -49,6 +49,7 @@ fn shell_responses( let command = shlex::try_join(command)?; let parameters = json!({ "command": command, + "login": false, "timeout_ms": 2_000, }); Ok(vec![ diff --git a/codex-rs/core/tests/suite/unified_exec.rs b/codex-rs/core/tests/suite/unified_exec.rs index 2e2fd34e67..a9c385d6cc 100644 --- a/codex-rs/core/tests/suite/unified_exec.rs +++ b/codex-rs/core/tests/suite/unified_exec.rs @@ -46,6 +46,15 @@ fn extract_output_text(item: &Value) -> Option<&str> { }) } +fn exec_command_args(mut args: Value) -> Value { + if let Value::Object(map) = &mut args { + if map.contains_key("cmd") { + map.entry("login".to_string()).or_insert(Value::Bool(false)); + } + } + args +} + #[derive(Debug)] struct ParsedUnifiedExecOutput { chunk_id: Option, @@ -171,10 +180,10 @@ async fn unified_exec_intercepts_apply_patch_exec_command() -> Result<()> { "*** Begin Patch\n*** Add File: uexec_apply.txt\n+hello from unified exec\n*** End Patch"; let command = format!("apply_patch <<'EOF'\n{patch}\nEOF\n"); let call_id = "uexec-apply-patch"; - let args = json!({ + let args = exec_command_args(json!({ "cmd": command, "yield_time_ms": 250, - }); + })); let responses = vec![ sse(vec![ @@ -299,11 +308,11 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-begin-event"; - let args = json!({ + let args = exec_command_args(json!({ "shell": "bash".to_string(), "cmd": "/bin/echo hello unified exec".to_string(), "yield_time_ms": 250, - }); + })); let responses = vec![ sse(vec![ @@ -342,7 +351,7 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> { }) .await; - assert_command(&begin_event.command, "-lc", "/bin/echo hello unified exec"); + assert_command(&begin_event.command, "-c", "/bin/echo hello unified exec"); assert_eq!(begin_event.cwd, cwd.path()); @@ -374,11 +383,11 @@ async fn unified_exec_resolves_relative_workdir() -> Result<()> { std::fs::create_dir_all(cwd.path().join(&workdir_rel))?; let call_id = "uexec-workdir-relative"; - let args = json!({ + let args = exec_command_args(json!({ "cmd": "pwd", "yield_time_ms": 250, "workdir": workdir_rel.to_string_lossy().to_string(), - }); + })); let responses = vec![ sse(vec![ @@ -452,11 +461,11 @@ async fn unified_exec_respects_workdir_override() -> Result<()> { std::fs::create_dir_all(&workdir)?; let call_id = "uexec-workdir"; - let args = json!({ + let args = exec_command_args(json!({ "cmd": "pwd", "yield_time_ms": 250, "workdir": workdir.to_string_lossy().to_string(), - }); + })); let responses = vec![ sse(vec![ @@ -528,16 +537,16 @@ async fn unified_exec_emits_exec_command_end_event() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-end-event"; - let args = json!({ + let args = exec_command_args(json!({ "cmd": "/bin/echo END-EVENT".to_string(), "yield_time_ms": 250, - }); + })); let poll_call_id = "uexec-end-event-poll"; - let poll_args = json!({ + let poll_args = exec_command_args(json!({ "chars": "", "session_id": 1000, "yield_time_ms": 250, - }); + })); let responses = vec![ sse(vec![ @@ -615,10 +624,10 @@ async fn unified_exec_emits_output_delta_for_exec_command() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-delta-1"; - let args = json!({ + let args = exec_command_args(json!({ "cmd": "printf 'HELLO-UEXEC'", "yield_time_ms": 1000, - }); + })); let responses = vec![ sse(vec![ @@ -688,10 +697,10 @@ async fn unified_exec_full_lifecycle_with_background_end_event() -> Result<()> { let call_id = "uexec-full-lifecycle"; // This timing force the long-standing PTY - let args = json!({ + let args = exec_command_args(json!({ "cmd": "sleep 0.5; printf 'HELLO-FULL-LIFECYCLE'", "yield_time_ms": 1000, - }); + })); let responses = vec![ sse(vec![ @@ -794,17 +803,17 @@ async fn unified_exec_emits_terminal_interaction_for_write_stdin() -> Result<()> } = builder.build(&server).await?; let open_call_id = "uexec-open"; - let open_args = json!({ + let open_args = exec_command_args(json!({ "cmd": "/bin/bash -i", "yield_time_ms": 200, - }); + })); let stdin_call_id = "uexec-stdin-delta"; - let stdin_args = json!({ + let stdin_args = exec_command_args(json!({ "chars": "echo WSTDIN-MARK\\n", "session_id": 1000, "yield_time_ms": 800, - }); + })); let responses = vec![ sse(vec![ @@ -893,33 +902,33 @@ async fn unified_exec_terminal_interaction_captures_delayed_output() -> Result<( } = builder.build(&server).await?; let open_call_id = "uexec-delayed-open"; - let open_args = json!({ + let open_args = exec_command_args(json!({ "cmd": "sleep 3 && echo MARKER1 && sleep 3 && echo MARKER2", "yield_time_ms": 10, - }); + })); // Poll stdin three times: first for no output, second after the first marker, // and a final long poll to capture the second marker. let first_poll_call_id = "uexec-delayed-poll-1"; - let first_poll_args = json!({ + let first_poll_args = exec_command_args(json!({ "chars": "", "session_id": 1000, "yield_time_ms": 10, - }); + })); let second_poll_call_id = "uexec-delayed-poll-2"; - let second_poll_args = json!({ + let second_poll_args = exec_command_args(json!({ "chars": "", "session_id": 1000, "yield_time_ms": 4000, - }); + })); let third_poll_call_id = "uexec-delayed-poll-3"; - let third_poll_args = json!({ + let third_poll_args = exec_command_args(json!({ "chars": "", "session_id": 1000, "yield_time_ms": 6000, - }); + })); let responses = vec![ sse(vec![ @@ -1083,18 +1092,18 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> { } = builder.build(&server).await?; let open_call_id = "uexec-open-session"; - let open_args = json!({ + let open_args = exec_command_args(json!({ "shell": "bash".to_string(), "cmd": "sleep 0.1".to_string(), "yield_time_ms": 10, - }); + })); let poll_call_id = "uexec-poll-empty"; - let poll_args = json!({ + let poll_args = exec_command_args(json!({ "chars": "", "session_id": 1000, "yield_time_ms": 150, - }); + })); let responses = vec![ sse(vec![ @@ -1166,7 +1175,7 @@ async fn unified_exec_emits_one_begin_and_one_end_event() -> Result<()> { let open_event = &begin_events[0]; - assert_command(&open_event.command, "-lc", "sleep 0.1"); + assert_command(&open_event.command, "-c", "sleep 0.1"); assert!( open_event.interaction_input.is_none(), @@ -1199,11 +1208,11 @@ async fn exec_command_reports_chunk_and_exit_metadata() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-metadata"; - let args = serde_json::json!({ + let args = exec_command_args(serde_json::json!({ "cmd": "printf 'token one token two token three token four token five token six token seven'", "yield_time_ms": 500, "max_output_tokens": 6, - }); + })); let responses = vec![ sse(vec![ @@ -1306,10 +1315,10 @@ async fn unified_exec_respects_early_exit_notifications() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec-early-exit"; - let args = serde_json::json!({ + let args = exec_command_args(serde_json::json!({ "cmd": "sleep 0.05", "yield_time_ms": 31415, - }); + })); let responses = vec![ sse(vec![ @@ -1401,20 +1410,20 @@ async fn write_stdin_returns_exit_metadata_and_clears_session() -> Result<()> { let send_call_id = "uexec-cat-send"; let exit_call_id = "uexec-cat-exit"; - let start_args = serde_json::json!({ + let start_args = exec_command_args(serde_json::json!({ "cmd": "/bin/cat", "yield_time_ms": 500, - }); - let send_args = serde_json::json!({ + })); + let send_args = exec_command_args(serde_json::json!({ "chars": "hello unified exec\n", "session_id": 1000, "yield_time_ms": 500, - }); - let exit_args = serde_json::json!({ + })); + let exit_args = exec_command_args(serde_json::json!({ "chars": "\u{0004}", "session_id": 1000, "yield_time_ms": 500, - }); + })); let responses = vec![ sse(vec![ @@ -1560,24 +1569,24 @@ async fn unified_exec_emits_end_event_when_session_dies_via_stdin() -> Result<() } = builder.build(&server).await?; let start_call_id = "uexec-end-on-exit-start"; - let start_args = serde_json::json!({ + let start_args = exec_command_args(serde_json::json!({ "cmd": "/bin/cat", "yield_time_ms": 200, - }); + })); let echo_call_id = "uexec-end-on-exit-echo"; - let echo_args = serde_json::json!({ + let echo_args = exec_command_args(serde_json::json!({ "chars": "bye-END\n", "session_id": 1000, "yield_time_ms": 300, - }); + })); let exit_call_id = "uexec-end-on-exit"; - let exit_args = serde_json::json!({ + let exit_args = exec_command_args(serde_json::json!({ "chars": "\u{0004}", "session_id": 1000, "yield_time_ms": 500, - }); + })); let responses = vec![ sse(vec![ @@ -1670,10 +1679,10 @@ async fn unified_exec_closes_long_running_session_at_turn_end() -> Result<()> { let call_id = "uexec-long-running"; let command = format!("printf '%s' $$ > '{pid_path_str}' && exec sleep 3000"); - let args = json!({ + let args = exec_command_args(json!({ "cmd": command, "yield_time_ms": 250, - }); + })); let responses = vec![ sse(vec![ @@ -1769,17 +1778,17 @@ async fn unified_exec_reuses_session_via_stdin() -> Result<()> { } = builder.build(&server).await?; let first_call_id = "uexec-start"; - let first_args = serde_json::json!({ + let first_args = exec_command_args(serde_json::json!({ "cmd": "/bin/cat", "yield_time_ms": 200, - }); + })); let second_call_id = "uexec-stdin"; - let second_args = serde_json::json!({ + let second_args = exec_command_args(serde_json::json!({ "chars": "hello unified exec\n", "session_id": 1000, "yield_time_ms": 500, - }); + })); let responses = vec![ sse(vec![ @@ -1900,17 +1909,17 @@ PY "#; let first_call_id = "uexec-lag-start"; - let first_args = serde_json::json!({ + let first_args = exec_command_args(serde_json::json!({ "cmd": script, "yield_time_ms": 25, - }); + })); let second_call_id = "uexec-lag-poll"; - let second_args = serde_json::json!({ + let second_args = exec_command_args(serde_json::json!({ "chars": "", "session_id": 1000, "yield_time_ms": 2_000, - }); + })); let responses = vec![ sse(vec![ @@ -2011,17 +2020,17 @@ async fn unified_exec_timeout_and_followup_poll() -> Result<()> { } = builder.build(&server).await?; let first_call_id = "uexec-timeout"; - let first_args = serde_json::json!({ + let first_args = exec_command_args(serde_json::json!({ "cmd": "sleep 0.5; echo ready", "yield_time_ms": 10, - }); + })); let second_call_id = "uexec-poll"; - let second_args = serde_json::json!({ + let second_args = exec_command_args(serde_json::json!({ "chars": "", "session_id": 1000, "yield_time_ms": 800, - }); + })); let responses = vec![ sse(vec![ @@ -2123,11 +2132,11 @@ PY "#; let call_id = "uexec-large-output"; - let args = serde_json::json!({ + let args = exec_command_args(serde_json::json!({ "cmd": script, "max_output_tokens": 100, "yield_time_ms": 500, - }); + })); let responses = vec![ sse(vec![ @@ -2202,10 +2211,10 @@ async fn unified_exec_runs_under_sandbox() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec"; - let args = serde_json::json!({ + let args = exec_command_args(serde_json::json!({ "cmd": "echo 'hello'", "yield_time_ms": 500, - }); + })); let responses = vec![ sse(vec![ @@ -2282,17 +2291,17 @@ async fn unified_exec_python_prompt_under_seatbelt() -> Result<()> { } = builder.build(&server).await?; let startup_call_id = "uexec-python-seatbelt"; - let startup_args = serde_json::json!({ + let startup_args = exec_command_args(serde_json::json!({ "cmd": format!("{} -i", python.display()), "yield_time_ms": 1_500, - }); + })); let exit_call_id = "uexec-python-exit"; - let exit_args = serde_json::json!({ + let exit_args = exec_command_args(serde_json::json!({ "chars": "exit()\n", "session_id": 1000, "yield_time_ms": 1_500, - }); + })); let responses = vec![ sse(vec![ @@ -2396,9 +2405,9 @@ async fn unified_exec_runs_on_all_platforms() -> Result<()> { } = builder.build(&server).await?; let call_id = "uexec"; - let args = serde_json::json!({ + let args = exec_command_args(serde_json::json!({ "cmd": "echo 'hello crossplat'", - }); + })); let responses = vec![ sse(vec![ @@ -2472,17 +2481,17 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { const FILLER_SESSIONS: i32 = MAX_SESSIONS_FOR_TEST - 1; let keep_call_id = "uexec-prune-keep"; - let keep_args = serde_json::json!({ + let keep_args = exec_command_args(serde_json::json!({ "cmd": "/bin/cat", "yield_time_ms": 250, - }); + })); let prune_call_id = "uexec-prune-target"; // Give the sleeper time to exit before the filler sessions trigger pruning. - let prune_args = serde_json::json!({ + let prune_args = exec_command_args(serde_json::json!({ "cmd": "sleep 1", "yield_time_ms": 1_250, - }); + })); let mut events = vec![ev_response_created("resp-prune-1")]; events.push(ev_function_call( @@ -2497,10 +2506,10 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { )); for idx in 0..FILLER_SESSIONS { - let filler_args = serde_json::json!({ + let filler_args = exec_command_args(serde_json::json!({ "cmd": format!("echo filler {idx}"), "yield_time_ms": 250, - }); + })); let call_id = format!("uexec-prune-fill-{idx}"); events.push(ev_function_call( &call_id, @@ -2510,11 +2519,11 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { } let keep_write_call_id = "uexec-prune-keep-write"; - let keep_write_args = serde_json::json!({ + let keep_write_args = exec_command_args(serde_json::json!({ "chars": "still alive\n", "session_id": 1000, "yield_time_ms": 500, - }); + })); events.push(ev_function_call( keep_write_call_id, "write_stdin", @@ -2522,11 +2531,11 @@ async fn unified_exec_prunes_exited_sessions_first() -> Result<()> { )); let probe_call_id = "uexec-prune-probe"; - let probe_args = serde_json::json!({ + let probe_args = exec_command_args(serde_json::json!({ "chars": "should fail\n", "session_id": 1001, "yield_time_ms": 500, - }); + })); events.push(ev_function_call( probe_call_id, "write_stdin",