From a0ba49b5a81ce702a080e81f31de96abc30a4d3d Mon Sep 17 00:00:00 2001 From: David Danialy Date: Mon, 14 Sep 2026 14:59:43 -0700 Subject: [PATCH 1/4] fix(tui): keep user images as clickable placeholders --- src/tui/ui.rs | 117 +++++++++++++------------------------------------- 1 file changed, 30 insertions(+), 87 deletions(-) diff --git a/src/tui/ui.rs b/src/tui/ui.rs index be8aa59..bb4e4ee 100644 --- a/src/tui/ui.rs +++ b/src/tui/ui.rs @@ -1321,10 +1321,8 @@ fn refresh_transcript_cache_with_images(app: &mut App, images: &mut ImageRuntime fn user_block_rows( message: &UserMessage, width: usize, - reserve_images: bool, ) -> (Vec, Vec) { let mut rows = Vec::new(); - let mut placements = Vec::new(); for (line_index, text) in message.text.split('\n').enumerate() { rows.extend(wrap_linked_tagged( &[( @@ -1333,32 +1331,8 @@ fn user_block_rows( )], width, )); - if reserve_images { - for (source, _) in message - .images - .iter() - .enumerate() - .filter(|(_, image)| image.line == line_index) - { - let row = rows.len(); - rows.extend((0..RESERVED_ROWS).map(|_| { - ( - Line::default(), - (None, None, None), - Vec::new(), - String::new(), - ) - })); - placements.push(CachedTranscriptImage { - block: None, - source, - row, - destination: None, - }); - } - } } - (rows, placements) + (rows, Vec::new()) } /// Place viewports at complete image boundaries before wrapping following prose. @@ -1564,7 +1538,7 @@ fn single_transcript_block_rows( ) -> (Vec, Vec) { let block = &app.blocks[block_index]; let (block_lines, call) = match block { - Block::User(message) => return user_block_rows(message, width, reserve_images), + Block::User(message) => return user_block_rows(message, width), Block::Agent(text) => return agent_block_rows(text, block_index, width, reserve_images), Block::AgentParts(parts) => { return agent_parts_rows(parts, block_index, width, reserve_images); @@ -5752,23 +5726,22 @@ mod tests { } #[test] - fn image_rows_preserve_text_image_text_display_order() { + fn user_image_placeholders_preserve_explicit_text_newlines() { let image = UserImage::new("AQID".into(), "image/png".into(), 1).unwrap(); let message = UserMessage { - text: "before\n[Image #1]\nafter".into(), + text: "before\n[Image #1](file:///tmp/image.png)\n\nafter\n".into(), images: vec![image], }; - let (rows, placements) = user_block_rows(&message, 40, true); + let (rows, placements) = user_block_rows(&message, 40); - assert_eq!(placements.len(), 1); - let after = &rows[placements[0].row + usize::from(super::RESERVED_ROWS)].0; - assert!( - after - .spans - .iter() - .any(|span| span.content.contains("after")) + assert!(placements.is_empty()); + assert_eq!( + rows.iter().map(|row| line_text(&row.0)).collect::>(), + ["› before", " Image #1", " ", " after", " "] ); + assert_eq!(rows[1].2.len(), 1); + assert_eq!(rows[1].2[0].url, "file:///tmp/image.png"); } #[test] @@ -5839,7 +5812,7 @@ mod tests { } #[test] - fn image_rows_are_fixed_and_decoding_is_lazy() { + fn user_images_remain_clickable_placeholders_with_image_runtime() { let mut png = std::io::Cursor::new(Vec::new()); image::DynamicImage::new_rgb8(400, 200) .write_to(&mut png, image::ImageFormat::Png) @@ -5862,61 +5835,31 @@ mod tests { })); let mut images = crate::tui::image::ImageRuntime::with_picker(Picker::halfblocks()); - refresh_transcript_cache_with_images(&mut app, &mut images, 12); - assert_eq!(images.cached_entries(), 0, "layout must not decode images"); - let narrow = app.transcript_cache[0].as_ref().unwrap(); - assert_eq!(narrow.images.len(), 1); - assert!(narrow.rows.len() > 1); - let narrow_rows = narrow.rows.len(); - assert_eq!(app.transcript_prefixes.last().copied(), Some(narrow_rows)); - - refresh_transcript_cache_with_images(&mut app, &mut images, 40); - assert_eq!(images.cached_entries(), 0, "width changes stay lazy"); - let wide = app.transcript_cache[0].as_ref().unwrap(); - assert_eq!(wide.images.len(), 1); - assert_eq!(wide.rows.len(), narrow_rows); + for width in [12, 40] { + refresh_transcript_cache_with_images(&mut app, &mut images, width); + let cached = app.transcript_cache[0].as_ref().unwrap(); + assert!(cached.images.is_empty()); + assert_eq!(cached.rows.len(), 1); + assert_eq!(line_text(&cached.rows[0].0), "› Image #1"); + assert_eq!(cached.rows[0].2.len(), 1); + assert_eq!(cached.rows[0].2[0].url, "file:///tmp/image.png"); + assert_eq!(app.transcript_prefixes.last().copied(), Some(1)); + } let mut terminal = Terminal::new(TestBackend::new(40, 20)).unwrap(); terminal .draw(|frame| draw(frame, &mut app, &mut images)) .unwrap(); - assert_eq!(images.cached_entries(), 0, "visible image decode is queued"); - wait_for_image_decode(&mut images); - terminal - .draw(|frame| draw(frame, &mut app, &mut images)) - .unwrap(); - assert_eq!(images.cached_entries(), 1, "visible image is rendered"); - assert!(buffer_contains_black_image_cell( - terminal.backend().buffer() - )); - let reserved_rows = app.transcript_cache[0].as_ref().unwrap().rows.len(); - - images.clear(); - assert_eq!( - images.cached_entries(), - 0, - "decoded image cache was evicted" - ); - terminal - .draw(|frame| draw(frame, &mut app, &mut images)) - .unwrap(); - assert_eq!(images.cached_entries(), 0, "evicted image decode is queued"); - wait_for_image_decode(&mut images); - terminal - .draw(|frame| draw(frame, &mut app, &mut images)) - .unwrap(); - assert_eq!( - images.cached_entries(), - 1, - "an evicted visible image is rendered again after decoding" - ); - assert!(buffer_contains_black_image_cell( + assert!(!images.pending(), "user images must not queue decoding"); + assert_eq!(images.cached_entries(), 0); + assert!(!buffer_contains_black_image_cell( terminal.backend().buffer() )); - assert_eq!( - app.transcript_cache[0].as_ref().unwrap().rows.len(), - reserved_rows, - "cache eviction cannot remove reserved transcript rows" + assert!( + app.row_links + .iter() + .flatten() + .any(|hit| hit.url == "file:///tmp/image.png") ); } From 44992b69e997a851775fb6b794b85f397cfa97a7 Mon Sep 17 00:00:00 2001 From: David Danialy Date: Mon, 14 Sep 2026 15:09:41 -0700 Subject: [PATCH 2/4] fix(tui): link replayed user images without source URIs --- src/tui/app.rs | 42 +++++++++++++++++++++++--- src/tui/mod.rs | 2 +- src/tui/ui.rs | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 120 insertions(+), 5 deletions(-) diff --git a/src/tui/app.rs b/src/tui/app.rs index 5d2dfd8..6e03bac 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -327,9 +327,6 @@ fn replace_image_uri_on_line( source_uri: Option<&str>, local_uri: Option<&str>, ) { - let Some(source_uri) = source_uri else { - return; - }; let start = if line == 0 { 0 } else if let Some((index, _)) = text.match_indices('\n').nth(line - 1) { @@ -340,6 +337,21 @@ fn replace_image_uri_on_line( let end = text[start..] .find('\n') .map_or(text.len(), |offset| start + offset); + let Some(source_uri) = source_uri else { + // URI-less image blocks get their own generated placeholder line during + // translation. Link only that label, never arbitrary neighboring text. + let label = &text[start..end]; + if let Some(local_uri) = local_uri + && let Some(number) = label + .strip_prefix("[Image #") + .and_then(|label| label.strip_suffix(']')) + && !number.is_empty() + && number.bytes().all(|byte| byte.is_ascii_digit()) + { + text.insert_str(end, &format!("({local_uri})")); + } + return; + }; let destination = markdown::image_label_link_destinations(&text[start..end]) .into_iter() .find_map(|(range, uri)| (uri == source_uri).then_some(range)); @@ -1749,7 +1761,7 @@ impl App { if image .source_uri .as_deref() - .is_some_and(|uri| uri.starts_with("file:")) + .is_none_or(|uri| uri.starts_with("file:")) { let uri = self.attachment_cache.image_uri(image.key); replace_image_uri_on_line( @@ -5182,6 +5194,28 @@ mod tests { ); } + #[test] + fn uri_less_user_image_rewrite_only_links_generated_placeholder_lines() { + for label in [ + "[Image #1]", + "[Image #12]", + "prose [Image #1]", + "`[Image #1]`", + "[Image #]", + "[Image #x]", + "[Image #1](https://example.com/image.png)", + ] { + let mut text = format!("before\n{label}\nafter"); + replace_image_uri_on_line(&mut text, 1, None, Some("file:///tmp/local.png")); + let expected = if matches!(label, "[Image #1]" | "[Image #12]") { + format!("before\n{label}(file:///tmp/local.png)\nafter") + } else { + format!("before\n{label}\nafter") + }; + assert_eq!(text, expected); + } + } + #[test] fn replayed_native_image_links_are_reused_and_live_for_the_session() { use base64::Engine as _; diff --git a/src/tui/mod.rs b/src/tui/mod.rs index 676fbf5..fda6e5c 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -630,7 +630,7 @@ fn spawn_background_workers( image .source_uri .as_deref() - .is_some_and(|uri| uri.starts_with("file:")) + .is_none_or(|uri| uri.starts_with("file:")) }) .take(64) .filter_map(|image| { diff --git a/src/tui/ui.rs b/src/tui/ui.rs index bb4e4ee..aacc9ba 100644 --- a/src/tui/ui.rs +++ b/src/tui/ui.rs @@ -5863,6 +5863,87 @@ mod tests { ); } + #[tokio::test] + async fn uri_less_user_images_translate_to_local_placeholder_links_without_preview_gaps() { + use crate::tui::{ + BackgroundCompletion, QueuedUpdate, spawn_background_workers, user_message_of, + }; + use agent_client_protocol::schema::v2::{ContentBlock, ImageContent, TextContent}; + + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::new_rgb8(2, 1) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + let bytes = png.into_inner(); + let encoded = base64::engine::general_purpose::STANDARD.encode(&bytes); + let (completed, mut completions) = tokio::sync::mpsc::channel(1); + let workers = spawn_background_workers(completed).unwrap(); + + // Full replay messages and replay chunks share this translation path. + for append in [false, true] { + for payload in [encoded.as_str(), "invalid base64"] { + let (text, sources) = user_message_of(vec![ + ContentBlock::Text(TextContent::new("before")), + ContentBlock::Image(ImageContent::new(payload, "image/png")), + ContentBlock::Text(TextContent::new("after")), + ]); + assert_eq!(text, "before\n[Image #1]\nafter"); + assert!( + workers + .try_update(QueuedUpdate { + generation: None, + update: crate::tui::app::Update::UserMessage { + id: "replayed".into(), + text, + images: sources, + append, + }, + }) + .is_ok() + ); + let BackgroundCompletion::Update { + queued, + images: prepared, + } = completions.recv().await.unwrap() + else { + panic!("expected update") + }; + let mut app = App::new( + PathBuf::from("/tmp/kit"), + "openai-subscription".into(), + "gpt-5.4".into(), + "0:0".into(), + ); + app.apply_materialized(queued.update, prepared); + let mut images = crate::tui::image::ImageRuntime::with_picker(Picker::halfblocks()); + for width in [12, 40] { + refresh_transcript_cache_with_images(&mut app, &mut images, width); + let cached = app.transcript_cache[0].as_ref().unwrap(); + assert!(cached.images.is_empty()); + assert_eq!(cached.rows.len(), 3); + assert_eq!(app.transcript_prefixes.last().copied(), Some(3)); + } + let cached = app.transcript_cache[0].as_ref().unwrap(); + let links = &cached.rows[1].2; + if payload == encoded { + assert_eq!(line_text(&cached.rows[1].0), " Image #1"); + assert_eq!(links.len(), 1); + let path = url::Url::parse(&links[0].url) + .unwrap() + .to_file_path() + .unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), bytes); + app.start_session("replacement".into()); + assert!(!path.exists(), "session switch must release the local file"); + } else { + assert!(links.is_empty(), "invalid data must not create a dead link"); + } + assert!(!images.pending()); + assert_eq!(images.cached_entries(), 0); + } + } + } + #[test] fn transcript_width_change_rebuilds_cached_rows() { let mut app = App::new( From 58120aefb16b7e542087f334d36bb3cacab1cf63 Mon Sep 17 00:00:00 2001 From: David Danialy Date: Mon, 14 Sep 2026 15:43:07 -0700 Subject: [PATCH 3/4] fix(tui): preserve bounded access to replayed user images --- docs/user/tui-and-sessions.md | 4 +- src/tui/app.rs | 416 +++++++++++++++++++++++++++++++++- src/tui/attachment.rs | 180 +++++++++++++++ src/tui/hyperlinks.rs | 21 ++ src/tui/markdown.rs | 72 +++++- src/tui/mod.rs | 225 +++++++++++++++++- src/tui/ui.rs | 57 ++++- 7 files changed, 954 insertions(+), 21 deletions(-) diff --git a/docs/user/tui-and-sessions.md b/docs/user/tui-and-sessions.md index 9620a08..4782682 100644 --- a/docs/user/tui-and-sessions.md +++ b/docs/user/tui-and-sessions.md @@ -101,13 +101,13 @@ An accepted file appears in the editor as `[Image #N]` or `[Audio #N]`. Add surr If you press `Enter` while native clipboard pastes are pending, submission waits for those pastes without blocking the terminal. A failed paste or further editing cancels automatic submission and keeps the draft available. Clearing the prompt or switching sessions discards stale clipboard results so they cannot populate another draft. -Native clipboard reads, clipboard image encoding, and reconstructed-image validation run on a bounded background worker so they do not pause terminal input or redraws. Clipboard files and reconstructed image links use bounded, session-owned temporary storage. On resume, Kit reconstructs local image links from available message content without reading stale paths or downloading URLs. Invalid or unavailable content, or an exhausted temporary-file budget, leaves the placeholder without a local image link. Kit deletes its temporary files on session switch or exit; original files you attached by path are not deleted. +Native clipboard reads, clipboard image encoding, and reconstructed-image validation run on a bounded background worker so they do not pause terminal input or redraws. Clipboard files and reconstructed image links use bounded, session-owned temporary storage. On resume, Kit reconstructs local image links from available message content without reading stale paths or downloading URLs. The stable reconstructed-link cache holds at most 64 files / 64 MiB. If that budget is exhausted but image bytes are still retained, the underlined placeholder remains openable with an ordinary Kit click: a bounded worker validates and materializes it on demand. These overflow placeholders do not support terminal modifier-click / OSC-8 opening. Opened overflow files are reused by image identity and retained until session switch or exit, so opening another image does not invalidate a file already handed to a viewer. This separate, non-evicting pool holds at most 64 files / 32 MiB (matching the retained image-source byte budget), with the unchanged 10 MiB per-image limit. Once the pool is full, Kit shows a notice instead of removing older files; previously opened images remain openable. Starting a new session releases these budgets. Retained base64 image sources are separately capped at 32 MiB; exceeding that limit shows a notice, and labels without either retained bytes or a stable link cannot be opened on demand. Invalid or unavailable content cannot be opened; Kit shows a notice if an on-demand open fails. Kit deletes its temporary files on session switch or exit; original files you attached by path are not deleted. Terminal compatibility: OSC-8 links are understood by current VS Code, Ghostty, Kitty, WezTerm, iTerm2, and many other terminals, subject to their link settings. For older terminals, use Kit's ordinary-click fallback. Multiplexers such as tmux must support or pass through the keyboard, paste, and hyperlink sequences; terminal-native shortcuts can differ from a direct session. Clipboard access and `file://` paths are local to the machine running Kit: over SSH, Kit cannot read your laptop's clipboard or make remote temporary files accessible to your local editor. Transfer an image to the remote host and paste its path instead. The model-facing prompt retains canonical `file://` Markdown links, while Kit also reads and sends the file bytes because remote providers cannot access local files. Image and audio acceptance remains model-dependent. Kit supports these request shapes through OpenRouter and OpenAI subscription; an individual model can still reject a modality it does not support. Video is not supported. -User-attached images render inline as a bounded static first frame when Kit detects Kitty, Sixel, or iTerm2 graphics support. No setting is required. Unsupported terminals, malformed or oversized images, and decode failures retain the safe clickable attachment label. Animated GIF and WebP files currently show only their first frame. Structured assistant and tool images use the same static preview renderer. Audio is not played. Only bounded `file://`, `http://`, and `https://` links are displayed; base64 and `data:` URLs are never copied into terminal text or Markdown links. +User-attached images remain compact attachment labels without inline previews. Structured assistant and tool images use the static preview renderer; animated GIF and WebP previews show only their first frame. Audio is not played. Only bounded `file://`, `http://`, and `https://` links are displayed; base64 and `data:` URLs are never copied into terminal text or Markdown links. ### Interrupt a running turn or quit diff --git a/src/tui/app.rs b/src/tui/app.rs index 6e03bac..3a0676d 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -82,6 +82,8 @@ pub enum Update { images: Vec, append: bool, }, + /// A bounded worker request from an internal image-placeholder click. + OpenUserImage(UserImage), /// Agent prose, either appended as a chunk or replaced by an upsert. AgentMessage { id: String, @@ -326,13 +328,13 @@ fn replace_image_uri_on_line( line: usize, source_uri: Option<&str>, local_uri: Option<&str>, -) { +) -> Option> { let start = if line == 0 { 0 } else if let Some((index, _)) = text.match_indices('\n').nth(line - 1) { index + 1 } else { - return; + return None; }; let end = text[start..] .find('\n') @@ -341,29 +343,35 @@ fn replace_image_uri_on_line( // URI-less image blocks get their own generated placeholder line during // translation. Link only that label, never arbitrary neighboring text. let label = &text[start..end]; - if let Some(local_uri) = local_uri - && let Some(number) = label - .strip_prefix("[Image #") - .and_then(|label| label.strip_suffix(']')) + if let Some(number) = label + .strip_prefix("[Image #") + .and_then(|label| label.strip_suffix(']')) && !number.is_empty() && number.bytes().all(|byte| byte.is_ascii_digit()) { - text.insert_str(end, &format!("({local_uri})")); + if let Some(local_uri) = local_uri { + text.insert_str(end, &format!("({local_uri})")); + } + return Some(start..end); } - return; + return None; }; let destination = markdown::image_label_link_destinations(&text[start..end]) .into_iter() .find_map(|(range, uri)| (uri == source_uri).then_some(range)); if let Some(destination) = destination { let destination = start + destination.start..start + destination.end; + let label_end = destination.start - 1; + let label_start = text[start..label_end].rfind('[')? + start; if let Some(local_uri) = local_uri { text.replace_range(destination, local_uri); } else { // Remove the complete `](destination)` suffix, leaving the label as plain text. text.replace_range(destination.start - 2..destination.end + 1, "]"); } + return Some(label_start..label_end); } + None } #[derive(Clone, Debug, PartialEq, Eq)] @@ -415,6 +423,7 @@ pub(super) enum ClipboardMode { } pub enum Action { + OpenUserImage(UserImage), Voice(String), None, Redraw, @@ -541,9 +550,16 @@ pub struct UserImage { pub(super) source_uri: Option, /// Source line after which the fixed image viewport is reserved. pub(super) line: usize, + /// Exact plain-label bytes trusted by translation/rewrite, never a parsed URI. + pub(super) open_label: Option>, } impl UserImage { + /// Internal hit target only: never sent to an OS URI handler or OSC 8. + pub(super) fn open_target(&self) -> String { + format!("kit-image:{}", blake3::Hash::from(self.key).to_hex()) + } + pub(super) fn new(data: String, mime_type: String, line: usize) -> Option { Self::with_source(data, mime_type, line, None) } @@ -584,6 +600,7 @@ impl UserImage { mime_type, source_uri, line, + open_label: None, }) } } @@ -1757,19 +1774,34 @@ impl App { self.next_attachment = self.next_attachment.max(self.submitted_attachment); } let mut images = images; - for image in &images { + for index in 0..images.len() { + let image = &images[index]; if image .source_uri .as_deref() .is_none_or(|uri| uri.starts_with("file:")) { let uri = self.attachment_cache.image_uri(image.key); - replace_image_uri_on_line( + let old_len = text.len(); + let label = replace_image_uri_on_line( &mut text, image.line, image.source_uri.as_deref(), uri.as_deref(), ); + if let Some(label) = label { + // Later rewrites may precede already associated labels (ACP + // image order need not match text order). + for previous in &mut images[..index] { + if let Some(range) = &mut previous.open_label + && range.start >= label.end + { + range.start = range.start + text.len() - old_len; + range.end = range.end + text.len() - old_len; + } + } + images[index].open_label = uri.is_none().then_some(label); + } } } let existing_index = self.message_blocks.get(&id).copied(); @@ -1786,6 +1818,7 @@ impl App { self.retained_image_source_bytes = self.retained_image_source_bytes.saturating_sub(replaced); } + let source_count = images.len(); images.retain(|image| { let retained = self .retained_image_source_bytes @@ -1797,6 +1830,9 @@ impl App { true } }); + if images.len() < source_count { + self.toast("image source limit reached; start a new session to retain more images"); + } if let Some(index) = existing_index { let mut changed = false; match (&mut self.blocks[index], role) { @@ -1816,8 +1852,13 @@ impl App { } let line_offset = existing.text.bytes().filter(|&byte| byte == b'\n').count(); + let byte_offset = existing.text.len(); existing.text.push_str(&text); for image in &mut images { + if let Some(range) = &mut image.open_label { + range.start += byte_offset; + range.end += byte_offset; + } image.line += line_offset; } existing.images.extend(std::mem::take(&mut images)); @@ -2009,6 +2050,22 @@ impl App { } pub(super) fn apply_materialized(&mut self, update: Update, images: Vec) { + if matches!(update, Update::OpenUserImage(_)) { + use super::attachment::RetainOpenedError; + let result = images + .into_iter() + .next() + .ok_or(RetainOpenedError::InvalidPath) + .and_then(|image| self.attachment_cache.retain_opened(image)); + match result { + Ok(uri) => open_url(&uri), + Err(RetainOpenedError::Exhausted) => { + self.toast("image open limit reached; start a new session to open more images") + } + Err(RetainOpenedError::InvalidPath) => self.toast("image could not be opened"), + } + return; + } for image in images { self.attachment_cache.admit(image); } @@ -2017,6 +2074,7 @@ impl App { pub fn apply(&mut self, update: Update) { match update { + Update::OpenUserImage(_) => {} Update::VoicePromptAccepted { .. } => {} Update::A2aAddress(address) => self.a2a = address, Update::SessionCatalog(result) => { @@ -4450,6 +4508,23 @@ impl App { return Action::None; } if let Some(url) = self.clicked_link(column, offset) { + if url.starts_with("kit-image:") { + return self + .blocks + .iter() + .find_map(|block| { + let Block::User(message) = block else { + return None; + }; + message + .images + .iter() + .find(|image| image.open_target() == url) + .cloned() + .map(Action::OpenUserImage) + }) + .unwrap_or(Action::None); + } open_url(&url); return Action::None; } @@ -5216,6 +5291,323 @@ mod tests { } } + #[tokio::test] + async fn exhausted_image_cache_keeps_placeholders_openable_on_demand() { + use super::super::{BackgroundCompletion, QueuedUpdate, spawn_background_workers}; + use base64::Engine as _; + let (completed, mut completions) = tokio::sync::mpsc::channel(1); + let workers = spawn_background_workers(completed).unwrap(); + let mut app = app(); + let mut first_uri = None; + for color in 0..65u8 { + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(image::RgbImage::from_pixel( + 1, + 1, + image::Rgb([color, 0, 0]), + )) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + let image = UserImage::new( + base64::engine::general_purpose::STANDARD.encode(png.into_inner()), + "image/png".into(), + 0, + ) + .unwrap(); + assert!( + workers + .try_update(QueuedUpdate::for_session( + 0, + Update::UserMessage { + id: color.to_string(), + text: "[Image #1]".into(), + images: vec![image], + append: false, + } + )) + .is_ok() + ); + let BackgroundCompletion::Update { queued, images } = completions.recv().await.unwrap() + else { + panic!("expected update"); + }; + app.apply_materialized(queued.update, images); + if color == 0 { + let Block::User(message) = &app.blocks[0] else { + panic!("user"); + }; + first_uri = app.attachment_cache.image_uri(message.images[0].key); + } + } + let Block::User(last) = app.blocks.last().unwrap() else { + panic!("user"); + }; + assert_eq!(last.text, "[Image #1]"); + let key = last.images[0].key; + assert!(app.attachment_cache.image_uri(key).is_none()); + let mut terminal = + ratatui::Terminal::new(ratatui::backend::TestBackend::new(60, 20)).unwrap(); + let mut runtime = super::super::image::ImageRuntime::with_picker( + ratatui_image::picker::Picker::halfblocks(), + ); + terminal + .draw(|frame| super::super::ui::draw(frame, &mut app, &mut runtime)) + .unwrap(); + let (row, hit) = app + .row_links + .iter() + .enumerate() + .find_map(|(row, hits)| { + hits.iter() + .find(|hit| hit.url.starts_with("kit-image:")) + .map(|hit| (row, hit.clone())) + }) + .unwrap(); + let mouse = |kind| MouseEvent { + kind, + column: (app.transcript_left + hit.start) as u16, + row: (app.transcript_top + row) as u16, + modifiers: KeyModifiers::NONE, + }; + let down = mouse(MouseEventKind::Down(MouseButton::Left)); + let up = mouse(MouseEventKind::Up(MouseButton::Left)); + app.handle_mouse(down); + let Action::OpenUserImage(image) = app.handle_mouse(up) else { + panic!("expected open action"); + }; + assert_eq!(image.key, key); + assert!( + workers + .try_update(QueuedUpdate::for_session(0, Update::OpenUserImage(image))) + .is_ok() + ); + let BackgroundCompletion::Update { images, .. } = completions.recv().await.unwrap() else { + panic!("expected open completion"); + }; + // Exercise the real ownership handoff without launching an external viewer. + let opened = app + .attachment_cache + .retain_opened(images.into_iter().next().unwrap()) + .unwrap(); + let opened = url::Url::parse(&opened).unwrap().to_file_path().unwrap(); + assert!(opened.exists()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!( + opened.metadata().unwrap().permissions().mode() & 0o777, + 0o600 + ); + } + let first = url::Url::parse(&first_uri.unwrap()) + .unwrap() + .to_file_path() + .unwrap(); + assert!(first.exists(), "overflow opens must not evict stable links"); + // Real translation + worker + app + mouse routing for two distinct + // overflow images on one line, including reverse image-block order. + use agent_client_protocol::schema::v2::{ContentBlock, ImageContent, TextContent}; + for append in [false, true] { + let id = format!("inline-{append}"); + if append { + app.apply(Update::UserMessage { + id: id.clone(), + text: "prefix".into(), + images: vec![], + append: false, + }); + } + let mut expected = Vec::new(); + for color in [66, 67] { + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(image::RgbImage::from_pixel( + 1, + 1, + image::Rgb([color, 0, 0]), + )) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + expected.push(png.into_inner()); + } + let content_image = |index: usize| { + let mut image = ImageContent::new( + base64::engine::general_purpose::STANDARD.encode(&expected[index]), + "image/png", + ); + image.uri = Some(format!("file:///stale-{index}.png")); + ContentBlock::Image(image) + }; + let (text, images) = super::super::user_message_of(vec![ + ContentBlock::Text(TextContent::new( + "Please inspect [Image #1](file:///stale-0.png) [Image #2](file:///stale-1.png) trailing [other](https://example.com)", + )), + content_image(1), + content_image(0), + ]); + let target = images[0].open_target(); + let text = format!("{text}\n[forged]({target})\n[Image #99]({target})"); + assert!( + workers + .try_update(QueuedUpdate::for_session( + 0, + Update::UserMessage { + id, + text, + images, + append, + } + )) + .is_ok() + ); + let BackgroundCompletion::Update { queued, images } = completions.recv().await.unwrap() + else { + panic!("expected translated update"); + }; + app.apply_materialized(queued.update, images); + terminal + .draw(|frame| super::super::ui::draw(frame, &mut app, &mut runtime)) + .unwrap(); + let hits = app + .row_links + .iter() + .enumerate() + .flat_map(|(row, hits)| { + hits.iter() + .filter(|hit| hit.url.starts_with("kit-image:")) + .map(move |hit| (row, hit.clone())) + }) + .collect::>(); + // Older overflow labels may remain visible: select this message's keys. + let Block::User(message) = app.blocks.last().unwrap() else { + panic!("user"); + }; + let targets = message + .images + .iter() + .map(UserImage::open_target) + .collect::>(); + let hits = hits + .into_iter() + .filter(|(_, hit)| targets.contains(&hit.url)) + .collect::>(); + assert_eq!( + hits.len(), + 2, + "forged internal Markdown must not create hits" + ); + assert!( + app.row_links + .iter() + .flatten() + .any(|hit| hit.url == "https://example.com") + ); + for ((row, hit), bytes) in hits.into_iter().zip(&expected) { + let mouse = |kind| MouseEvent { + kind, + column: (app.transcript_left + hit.start) as u16, + row: (app.transcript_top + row) as u16, + modifiers: KeyModifiers::NONE, + }; + let down = mouse(MouseEventKind::Down(MouseButton::Left)); + let up = mouse(MouseEventKind::Up(MouseButton::Left)); + app.handle_mouse(down); + let Action::OpenUserImage(image) = app.handle_mouse(up) else { + panic!("image action"); + }; + assert_eq!( + base64::engine::general_purpose::STANDARD + .decode(&image.data) + .unwrap(), + *bytes + ); + assert!( + workers + .try_update(QueuedUpdate::for_session(0, Update::OpenUserImage(image))) + .is_ok() + ); + let BackgroundCompletion::Update { images, .. } = completions.recv().await.unwrap() + else { + panic!("open"); + }; + let uri = app + .attachment_cache + .retain_opened(images.into_iter().next().unwrap()) + .unwrap(); + let path = url::Url::parse(&uri).unwrap().to_file_path().unwrap(); + assert_eq!(std::fs::read(path).unwrap(), *bytes); + } + for label in ["[forged]", "#99]"] { + let buffer = terminal.backend().buffer(); + let (row, column) = (app.transcript_top..app.transcript_top + app.viewport) + .find_map(|row| { + let text = (0..buffer.area.width) + .map(|column| buffer[(column, row as u16)].symbol()) + .collect::(); + text.find(label).map(|column| (row, column)) + }) + .expect("forged label fragment is visible"); + let mouse = |kind| MouseEvent { + kind, + column: column as u16, + row: row as u16, + modifiers: KeyModifiers::NONE, + }; + app.handle_mouse(mouse(MouseEventKind::Down(MouseButton::Left))); + assert!(matches!( + app.handle_mouse(mouse(MouseEventKind::Up(MouseButton::Left))), + Action::None + )); + } + // Remove only the test transcript so duplicate keys in the next case + // cannot inflate visible hit counts; retain the exhausted real cache. + app.blocks.clear(); + app.message_blocks.clear(); + } + app.start_session("next".into()); + assert!(!opened.exists()); + assert!(!first.exists()); + } + + #[test] + fn exhausted_open_pool_reports_actionable_notice_without_launching() { + use base64::Engine as _; + let mut app = app(); + for color in 0..=64 { + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(image::RgbImage::from_pixel( + 1, + 1, + image::Rgb([color, 0, 0]), + )) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + let image = UserImage::new( + base64::engine::general_purpose::STANDARD.encode(png.into_inner()), + "image/png".into(), + 0, + ) + .unwrap(); + let materialized = super::super::attachment::materialize_image( + image.key, + &image.data, + &image.mime_type, + usize::MAX, + ) + .unwrap(); + if color < 64 { + app.attachment_cache.retain_opened(materialized).unwrap(); + } else { + // Admission failure takes the real completion path, which must + // report the refusal rather than launch an unowned path. + app.apply_materialized(Update::OpenUserImage(image), vec![materialized]); + assert_eq!( + app.toast_text(), + Some("image open limit reached; start a new session to open more images",) + ); + } + } + } + #[test] fn replayed_native_image_links_are_reused_and_live_for_the_session() { use base64::Engine as _; @@ -5316,6 +5708,10 @@ mod tests { }); } + assert_eq!( + app.toast_text(), + Some("image source limit reached; start a new session to retain more images") + ); assert_eq!(app.retained_image_source_bytes, source_bytes * 3); assert!(app.retained_image_source_bytes <= MAX_RETAINED_IMAGE_SOURCE_BYTES); assert!(matches!( diff --git a/src/tui/attachment.rs b/src/tui/attachment.rs index 27e138e..4e0dd03 100644 --- a/src/tui/attachment.rs +++ b/src/tui/attachment.rs @@ -6,6 +6,7 @@ use tempfile::{Builder, TempPath}; const MAX_SESSION_FILES: usize = 64; const MAX_SESSION_BYTES: usize = 64 * 1024 * 1024; +const MAX_OPENED_BYTES: usize = super::app::MAX_RETAINED_IMAGE_SOURCE_BYTES; const MAX_DECODED_ALLOCATION: u64 = 64 * 1024 * 1024; const MAX_DIMENSION: u32 = 8_192; const MAX_IMAGE_BYTES: usize = 10 * 1024 * 1024; @@ -121,12 +122,52 @@ pub(super) fn materialize_image( pub(super) struct SessionAttachmentCache { files: HashMap<[u8; 32], Arc>, bytes: usize, + // Opened overflow files stay alive for external viewers until session cleanup. + opened: HashMap<[u8; 32], Arc>, + opened_bytes: usize, +} + +#[derive(Debug, PartialEq, Eq)] +pub(super) enum RetainOpenedError { + Exhausted, + InvalidPath, } impl SessionAttachmentCache { + /// Retains overflow images without invalidating paths handed to external + /// viewers. Repeated keys reuse their original file, even when the pool is + /// full. The separate pool is bounded by the retained image-source budget. + pub(super) fn retain_opened( + &mut self, + image: MaterializedImage, + ) -> Result { + if let Some(file) = self + .files + .get(&image.key) + .or_else(|| self.opened.get(&image.key)) + { + return url::Url::from_file_path(file.path()) + .map(|uri| uri.to_string()) + .map_err(|()| RetainOpenedError::InvalidPath); + } + if self.opened.len() >= MAX_SESSION_FILES + || image.bytes > MAX_OPENED_BYTES.saturating_sub(self.opened_bytes) + { + return Err(RetainOpenedError::Exhausted); + } + let uri = url::Url::from_file_path(image.file.path()) + .map_err(|()| RetainOpenedError::InvalidPath)? + .to_string(); + self.opened_bytes += image.bytes; + self.opened.insert(image.key, image.file); + Ok(uri) + } + pub(super) fn clear(&mut self) { self.files.clear(); self.bytes = 0; + self.opened.clear(); + self.opened_bytes = 0; } /// Admits a worker-produced file without evicting existing session links. @@ -154,6 +195,145 @@ impl SessionAttachmentCache { mod tests { use super::*; + fn overflow_image(color: u8) -> MaterializedImage { + materialize_image( + [color; 32], + &STANDARD.encode(source(ImageFormat::Png, color)), + "image/png", + MAX_IMAGE_BYTES, + ) + .unwrap() + } + + #[test] + fn overflow_pool_retains_a_and_b_until_clear_or_drop() { + for clear in [false, true] { + let mut cache = SessionAttachmentCache::default(); + let a = overflow_image(1); + let a_path = a.file.path().to_owned(); + cache.retain_opened(a).unwrap(); + let b = overflow_image(2); + let b_path = b.file.path().to_owned(); + cache.retain_opened(b).unwrap(); + assert!(a_path.exists()); + assert!(b_path.exists()); + if clear { + cache.clear(); + assert!(!a_path.exists()); + assert!(!b_path.exists()); + cache.retain_opened(overflow_image(1)).unwrap(); + } + drop(cache); + assert!(!a_path.exists()); + assert!(!b_path.exists()); + } + } + + #[test] + fn overflow_pool_reuses_duplicates_and_refuses_new_files_when_full() { + let mut cache = SessionAttachmentCache::default(); + let mut paths = Vec::new(); + let mut first_uri = String::new(); + for color in 0..MAX_SESSION_FILES { + let image = overflow_image(color as u8); + paths.push(image.file.path().to_owned()); + let uri = cache.retain_opened(image).unwrap(); + if color == 0 { + first_uri = uri; + } + } + let duplicate = overflow_image(0); + let duplicate_path = duplicate.file.path().to_owned(); + assert_eq!(cache.retain_opened(duplicate).unwrap(), first_uri); + assert!(!duplicate_path.exists()); + let refused = overflow_image(MAX_SESSION_FILES as u8); + let refused_path = refused.file.path().to_owned(); + assert_eq!( + cache.retain_opened(refused), + Err(RetainOpenedError::Exhausted) + ); + assert!(!refused_path.exists()); + assert!(paths.iter().all(|path| path.exists())); + cache.clear(); + assert!(paths.iter().all(|path| !path.exists())); + cache.retain_opened(overflow_image(0)).unwrap(); + } + + #[test] + fn overflow_pool_refuses_byte_exhaustion_without_evicting() { + let mut cache = SessionAttachmentCache::default(); + let mut paths = Vec::new(); + // Trailing PNG bytes are preserved by materialization, allowing the + // real validation and storage APIs to exercise the byte budget. + for color in 0..3 { + let mut bytes = source(ImageFormat::Png, color); + bytes.resize(MAX_IMAGE_BYTES, 0); + let image = materialize_image( + [color; 32], + &STANDARD.encode(bytes), + "image/png", + MAX_IMAGE_BYTES, + ) + .unwrap(); + paths.push(image.file.path().to_owned()); + cache.retain_opened(image).unwrap(); + } + let mut bytes = source(ImageFormat::Png, 3); + bytes.resize(MAX_IMAGE_BYTES, 0); + let image = materialize_image( + [3; 32], + &STANDARD.encode(bytes), + "image/png", + MAX_IMAGE_BYTES, + ) + .unwrap(); + let refused_path = image.file.path().to_owned(); + assert_eq!( + cache.retain_opened(image), + Err(RetainOpenedError::Exhausted) + ); + assert!(!refused_path.exists()); + assert!(paths.iter().all(|path| path.exists())); + cache.clear(); + assert!(paths.iter().all(|path| !path.exists())); + cache.retain_opened(overflow_image(0)).unwrap(); + } + + #[tokio::test] + async fn stale_on_demand_completion_releases_private_file() { + use super::super::app::{Update, UserImage}; + use super::super::{ + ActiveSessionRoute, BackgroundCompletion, QueuedUpdate, accept_queued_update, + spawn_background_workers, + }; + let (completed, mut completions) = tokio::sync::mpsc::channel(1); + let workers = spawn_background_workers(completed).unwrap(); + let image = UserImage::new( + STANDARD.encode(source(ImageFormat::Png, 1)), + "image/png".into(), + 0, + ) + .unwrap(); + assert!( + workers + .try_update(QueuedUpdate::for_session(1, Update::OpenUserImage(image))) + .is_ok() + ); + let BackgroundCompletion::Update { queued, images } = completions.recv().await.unwrap() + else { + panic!("expected update"); + }; + let path = images[0].file.path().to_owned(); + assert!(path.exists()); + let route = Arc::new(std::sync::Mutex::new(ActiveSessionRoute { + id: "new".into(), + generation: 2, + })); + assert!(accept_queued_update(&route, queued).is_none()); + drop(images); + assert!(!path.exists()); + } + fn source(format: ImageFormat, color: u8) -> Vec { let image = image::RgbImage::from_pixel(1, 1, image::Rgb([color, 40, 60])); let mut bytes = Cursor::new(Vec::new()); diff --git a/src/tui/hyperlinks.rs b/src/tui/hyperlinks.rs index df5ffe0..345fa79 100644 --- a/src/tui/hyperlinks.rs +++ b/src/tui/hyperlinks.rs @@ -73,6 +73,11 @@ impl HyperlinkRenderer { continue; }; for hit in hits { + // Internal on-demand targets require a normal TUI click; + // terminal modifier-click must not dispatch them to the OS. + if hit.url.starts_with("kit-image:") { + continue; + } let Some(start) = transcript_left .checked_add(hit.start) .and_then(|value| u16::try_from(value).ok()) @@ -268,6 +273,22 @@ mod tests { } } + #[test] + fn internal_image_targets_are_not_emitted_as_native_hyperlinks() { + let buffer = Buffer::with_lines([Line::from("[Image #1]")]); + let rows = vec![vec![LinkHit { + start: 0, + end: 10, + url: "kit-image:internal".into(), + }]]; + let mut renderer = HyperlinkRenderer::default(); + let prepared = renderer.prepare(&frame(&buffer), &rows, 0, 0, false); + let capture = Capture::default(); + let mut backend = CrosstermBackend::new(capture.clone()); + renderer.draw(&mut backend, prepared).unwrap(); + assert!(!has_nonempty_open(&capture.bytes())); + } + #[test] fn emits_native_open_and_close_around_linked_cells_and_preserves_cursor() { let buffer = Buffer::with_lines([Line::from("sent Image #1")]); diff --git a/src/tui/markdown.rs b/src/tui/markdown.rs index f7cf3fc..1fd957b 100644 --- a/src/tui/markdown.rs +++ b/src/tui/markdown.rs @@ -733,6 +733,7 @@ pub(super) fn image_label_link_destinations(source: &str) -> Vec<(Range, false, 0, Some(&mut destinations), + &[], ); destinations } @@ -802,6 +803,17 @@ pub(super) fn inline_spans(source: &str, base: Style) -> Vec { inline_with_link_destinations(source, base, false) } +/// Render internal image hits only at caller-associated source ranges. The +/// ordinary Markdown parser still rejects internal URI schemes, including when +/// an attacker copies a real image target into another link. +pub(super) fn inline_spans_with_image_labels( + source: &str, + base: Style, + labels: &[(Range, String)], +) -> Vec { + inline_with_link_destinations_and_ranges(source, base, false, 0, None, labels) +} + /// Splits inline links, emphasis, and code spans out of one line of Markdown. fn inline(source: &str, base: Style) -> Vec { inline_with_link_destinations(source, base, true) @@ -812,7 +824,7 @@ fn inline_with_link_destinations( base: Style, show_link_destinations: bool, ) -> Vec { - inline_with_link_destinations_and_ranges(source, base, show_link_destinations, 0, None) + inline_with_link_destinations_and_ranges(source, base, show_link_destinations, 0, None, &[]) } fn inline_with_link_destinations_and_ranges( @@ -821,6 +833,7 @@ fn inline_with_link_destinations_and_ranges( show_link_destinations: bool, offset: usize, mut destinations: Option<&mut Vec<(Range, String)>>, + labels: &[(Range, String)], ) -> Vec { let mut spans = Vec::new(); let mut plain = String::new(); @@ -831,6 +844,30 @@ fn inline_with_link_destinations_and_ranges( .map(|index| (index, &rest[index..])); let image = image_references(rest).into_iter().next(); let link = next_link(rest); + let consumed = offset + source.len() - rest.len(); + let label = labels + .iter() + .find(|(range, _)| range.start >= consumed && range.end <= consumed + rest.len()); + if let Some((range, target)) = label { + let start = range.start - consumed; + let end = range.end - consumed; + if marker.as_ref().is_none_or(|(index, _)| start < *index) + && image.as_ref().is_none_or(|(range, _)| start <= range.start) + && link.as_ref().is_none_or(|link| start <= link.start) + { + plain.push_str(&rest[..start]); + if !plain.is_empty() { + spans.push(plain_span(std::mem::take(&mut plain), base)); + } + spans.push(link_span( + rest[start..end].to_owned(), + base.add_modifier(Modifier::UNDERLINED), + target, + )); + rest = &rest[end..]; + continue; + } + } if let Some((range, _)) = image && marker .as_ref() @@ -943,6 +980,7 @@ fn inline_with_link_destinations_and_ranges( show_link_destinations, offset + source.len() - body.len(), destinations.as_deref_mut(), + labels, )); } rest = &body[close + delimiter.len()..]; @@ -999,6 +1037,38 @@ mod tests { .collect() } + #[test] + fn trusted_image_labels_preserve_surrounding_emphasis_and_reject_forged_uris() { + let text = + "**Please [Image #1] inspect** [other](https://example.com) [forged](kit-image:one)"; + let start = text.find("[Image #1]").unwrap(); + let spans = super::inline_spans_with_image_labels( + text, + ratatui::style::Style::default(), + &[(start..start + "[Image #1]".len(), "kit-image:one".into())], + ); + let links = spans + .iter() + .filter(|span| span.url.is_some()) + .collect::>(); + assert_eq!(links.len(), 2); + assert_eq!(links[0].span.content, "[Image #1]"); + assert!( + links[0] + .span + .style + .add_modifier + .contains(ratatui::style::Modifier::BOLD) + ); + assert_eq!(links[1].url.as_deref(), Some("https://example.com")); + let visible = spans + .iter() + .map(|span| span.span.content.as_ref()) + .collect::(); + assert!(!visible.contains("**")); + assert!(visible.contains("[forged](kit-image:one)")); + } + #[test] fn renders_aligned_pipe_tables() { let rendered = diff --git a/src/tui/mod.rs b/src/tui/mod.rs index fda6e5c..a6e0c8b 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -625,13 +625,17 @@ fn spawn_background_workers( let images = match &queued.update { Update::UserMessage { images, .. } => images .iter() - .take_while(|_| !update_stopping.load(Ordering::Acquire)) + // File-backed placeholders need materialization before they can + // open. Give them priority over URI-less image candidates. .filter(|image| { image .source_uri .as_deref() - .is_none_or(|uri| uri.starts_with("file:")) + .is_some_and(|uri| uri.starts_with("file:")) }) + .chain(images.iter().filter(|image| image.source_uri.is_none())) + .take_while(|_| !update_stopping.load(Ordering::Acquire)) + // Bound attempts, including failed decodes, across both groups. .take(64) .filter_map(|image| { let prepared = attachment::materialize_image( @@ -647,6 +651,14 @@ fn spawn_background_workers( Some(prepared) }) .collect(), + Update::OpenUserImage(image) => attachment::materialize_image( + image.key, + &image.data, + &image.mime_type, + 10 * 1024 * 1024, + ) + .into_iter() + .collect(), _ => Vec::new(), }; if !send_background_completion( @@ -2471,6 +2483,18 @@ pub async fn run_with_reasoning_effort_and_openrouter_key( )?; events = EventStream::new(); } + Action::OpenUserImage(image) => { + // Snapshot only; release the guard before queueing work. + // Poison rejects this request rather than selecting another session. + let generation = transition_session.lock().ok().map(|route| route.generation); + if let Some(generation) = generation + && background_workers.try_update(QueuedUpdate::for_session( + generation, Update::OpenUserImage(image), + )).is_err() + { + app.note("image worker is busy; click again to open"); + } + } Action::Copy(text) => { execute!(terminal.backend_mut(), Print(osc52(&text))) .map_err(agent_client_protocol::Error::into_internal_error)?; @@ -3744,7 +3768,9 @@ fn user_message_of(blocks: Vec) -> (String, Vec) { .map(|(line, uri)| (first_line + line, uri)), ); text.push_str(&content); - separate_after_image = false; + if !content.is_empty() { + separate_after_image = false; + } } } } @@ -5234,6 +5260,87 @@ mod tests { } } + #[tokio::test] + async fn background_image_worker_prioritizes_files_and_bounds_decode_attempts() { + use super::app::UserImage; + + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::new_rgb8(1, 1) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + let encoded = STANDARD.encode(png.into_inner()); + let (completed, mut completions) = tokio::sync::mpsc::channel(1); + let workers = super::spawn_background_workers(completed).unwrap(); + + for invalid_files in [0, 63, 64] { + let mut images: Vec<_> = (0..64) + .map(|_| UserImage::new("not base64!".into(), "image/png".into(), 0).unwrap()) + .collect(); + images.extend((0..invalid_files).map(|_| { + UserImage::with_source( + "not base64!".into(), + "image/png".into(), + 0, + Some("file:///invalid.png".into()), + ) + .unwrap() + })); + images.push( + UserImage::with_source( + encoded.clone(), + "image/png".into(), + 0, + Some("file:///valid.png".into()), + ) + .unwrap(), + ); + assert!( + workers + .try_update(QueuedUpdate::for_session( + 1, + Update::UserMessage { + id: "user".into(), + text: "[Image #1](file:///valid.png)".into(), + images, + append: false, + }, + )) + .is_ok() + ); + let completion = + tokio::time::timeout(std::time::Duration::from_secs(5), completions.recv()) + .await + .unwrap() + .unwrap(); + let BackgroundCompletion::Update { queued, images } = completion else { + panic!("expected image worker completion"); + }; + assert_eq!(images.len(), usize::from(invalid_files < 64)); + let mut app = App::new( + "/tmp/kit".into(), + "provider".into(), + "model".into(), + "a2a".into(), + ); + app.apply_materialized(queued.update, images); + let super::app::Block::User(message) = &app.blocks[0] else { + panic!("expected user"); + }; + if invalid_files < 64 { + let (_, uri) = super::markdown::image_label_links(&message.text) + .pop() + .unwrap(); + let path = url::Url::parse(&uri).unwrap().to_file_path().unwrap(); + assert_eq!( + std::fs::read(path).unwrap(), + STANDARD.decode(&encoded).unwrap() + ); + } else { + assert_eq!(message.text, "[Image #1]"); + } + } + } + #[test] fn assistant_image_translation_defers_decode_to_worker() { let update = agent_content_update( @@ -6707,6 +6814,118 @@ mod tests { assert_eq!(images[0].line, 1); } + #[test] + fn sequential_user_image_chunks_preserve_materialized_links_and_spacing() { + use super::{app::Block, attachment}; + + let mut app = App::new( + PathBuf::from("/tmp"), + "provider".into(), + "model".into(), + "a2a".into(), + ); + let image_block = |value| { + let mut png = std::io::Cursor::new(Vec::new()); + image::DynamicImage::ImageRgb8(image::RgbImage::from_pixel( + 1, + 1, + image::Rgb([value, 0, 0]), + )) + .write_to(&mut png, image::ImageFormat::Png) + .unwrap(); + ContentBlock::Image(wire::ImageContent::new( + STANDARD.encode(png.into_inner()), + "image/png", + )) + }; + for content in [ + ContentBlock::Text(TextContent::new("before")), + image_block(0), + ContentBlock::Text(TextContent::new("")), + ContentBlock::Text(TextContent::new("after")), + image_block(255), + ContentBlock::Text(TextContent::new("end")), + ] { + let notification = UpdateSessionNotification::new( + "session", + SessionUpdate::UserMessageChunk( + serde_json::from_value(json!({ + "messageId": "user", + "content": content, + })) + .unwrap(), + ), + ); + let updates = translate_for_session(notification, "session"); + assert_eq!(updates.len(), 1); + for update in updates { + let Update::UserMessage { + id, images, append, .. + } = &update + else { + panic!("expected a user message chunk"); + }; + assert_eq!(id, "user"); + assert!(*append); + let materialized = images + .iter() + .map(|image| { + assert!(image.source_uri.is_none()); + attachment::materialize_image( + image.key, + &image.data, + &image.mime_type, + usize::MAX, + ) + .unwrap() + }) + .collect(); + app.apply_materialized(update, materialized); + } + } + + let [Block::User(message)] = app.blocks.as_slice() else { + panic!("expected one user message"); + }; + assert_eq!(message.images.len(), 2); + assert_ne!(message.images[0].key, message.images[1].key); + assert_eq!(message.images[0].line, 1); + assert_eq!(message.images[1].line, 3); + let lines: Vec<_> = message.text.lines().collect(); + assert_eq!(lines.len(), 5); + assert_eq!(lines[0], "before"); + assert_eq!(lines[2], "after"); + assert_eq!(lines[4], "end"); + let links = super::markdown::image_label_links(&message.text); + assert_eq!(links.len(), 2); + assert_ne!(links[0].1, links[1].1); + for ((line, uri), image) in links.iter().zip(&message.images) { + assert_eq!(*line, image.line); + assert!(uri.starts_with("file://")); + let path = url::Url::parse(uri).unwrap().to_file_path().unwrap(); + assert_eq!( + std::fs::read(path).unwrap(), + STANDARD.decode(&image.data).unwrap() + ); + } + } + + #[test] + fn user_image_placeholder_separator_survives_empty_text() { + let (text, images) = user_message_of(vec![ + ContentBlock::Image(agent_client_protocol::schema::v2::ImageContent::new( + "AQID", + "image/png", + )), + ContentBlock::Text(TextContent::new("")), + ContentBlock::Text(TextContent::new("after")), + ]); + + assert_eq!(text, "[Image #1]\nafter"); + assert_eq!(images.len(), 1); + assert_eq!(images[0].line, 0); + } + #[test] fn image_deduplication_requires_the_exact_trusted_link() { let actual = "file:///tmp/actual.png"; diff --git a/src/tui/ui.rs b/src/tui/ui.rs index aacc9ba..136899b 100644 --- a/src/tui/ui.rs +++ b/src/tui/ui.rs @@ -1323,12 +1323,31 @@ fn user_block_rows( width: usize, ) -> (Vec, Vec) { let mut rows = Vec::new(); + let mut line_start = 0; for (line_index, text) in message.text.split('\n').enumerate() { + let mut line = user_line("", line_index == 0); + let mut labels = message + .images + .iter() + .filter_map(|image| { + let range = image.open_label.as_ref()?; + (range.start >= line_start && range.end <= line_start + text.len()).then(|| { + ( + range.start - line_start..range.end - line_start, + image.open_target(), + ) + }) + }) + .collect::>(); + labels.sort_by_key(|(range, _)| range.start); + line.spans.extend(markdown::inline_spans_with_image_labels( + text, + theme::bold(theme::text_color()), + &labels, + )); + line_start += text.len() + 1; rows.extend(wrap_linked_tagged( - &[( - user_line(text, line_index == 0), - (None, None, Some(line_index)), - )], + &[(line, (None, None, Some(line_index)))], width, )); } @@ -5936,7 +5955,35 @@ mod tests { app.start_session("replacement".into()); assert!(!path.exists(), "session switch must release the local file"); } else { - assert!(links.is_empty(), "invalid data must not create a dead link"); + assert_eq!(links.len(), 1); + assert!( + links[0].url.starts_with("kit-image:"), + "invalid data must not create a dead native file link" + ); + let Block::User(message) = &app.blocks[0] else { + panic!("expected user"); + }; + assert!( + workers + .try_update(QueuedUpdate { + generation: None, + update: crate::tui::app::Update::OpenUserImage( + message.images[0].clone() + ), + }) + .is_ok() + ); + let BackgroundCompletion::Update { queued, images } = + completions.recv().await.unwrap() + else { + panic!("expected open completion"); + }; + assert!( + images.is_empty(), + "invalid data must not create a temporary file" + ); + app.apply_materialized(queued.update, images); + assert_eq!(app.toast_text(), Some("image could not be opened")); } assert!(!images.pending()); assert_eq!(images.cached_entries(), 0); From ac41b8fc1d8cc6807a3bd7194c4d2f48f590561e Mon Sep 17 00:00:00 2001 From: David Danialy Date: Mon, 14 Sep 2026 16:12:57 -0700 Subject: [PATCH 4/4] fix(tui): reuse opened image files and quiet repeated limit notices Address review findings on the clickable placeholder flow: - Admission and link lookup now consult the opened pool, so a key retained by a click is neither materialized twice nor left as a kit-image label that re-decodes on every click. - Clicks resolve cached keys directly instead of cloning the source and round-tripping the image worker. - The image source limit toast fires once per session rather than on every later image-bearing message or resume replay. - A disconnected image worker reports "unavailable" instead of "busy; click again". - Drop the dead user-block image render arm and the vestigial placement tuple from user_block_rows. - Stop duplicating the 10 MiB materialization limit at the open site. Co-Authored-By: Claude Fable 5.1 --- src/tui/app.rs | 101 +++++++++++++++++++++++++++++++++--------- src/tui/attachment.rs | 33 +++++++++++--- src/tui/mod.rs | 18 +++++--- src/tui/ui.rs | 20 ++++----- 4 files changed, 125 insertions(+), 47 deletions(-) diff --git a/src/tui/app.rs b/src/tui/app.rs index 3a0676d..9fb10bc 100644 --- a/src/tui/app.rs +++ b/src/tui/app.rs @@ -548,7 +548,7 @@ pub struct UserImage { pub(super) data: String, pub(super) mime_type: String, pub(super) source_uri: Option, - /// Source line after which the fixed image viewport is reserved. + /// Source line carrying this image's clickable placeholder label. pub(super) line: usize, /// Exact plain-label bytes trusted by translation/rewrite, never a parsed URI. pub(super) open_label: Option>, @@ -828,6 +828,7 @@ pub struct App { pub(super) transcript_prefixes: Vec, pub(super) transcript_cache_width: usize, retained_image_source_bytes: usize, + image_source_limit_noticed: bool, attachment_cache: SessionAttachmentCache, next_transcript_revision: u64, transcript_focus_index: Option, @@ -1095,6 +1096,7 @@ impl App { transcript_prefixes: vec![0], transcript_cache_width: 0, retained_image_source_bytes: 0, + image_source_limit_noticed: false, attachment_cache: SessionAttachmentCache::default(), next_transcript_revision: 0, transcript_focus_index: None, @@ -1830,7 +1832,9 @@ impl App { true } }); - if images.len() < source_count { + // Notify once per session; later messages and resume replays stay quiet. + if images.len() < source_count && !self.image_source_limit_noticed { + self.image_source_limit_noticed = true; self.toast("image source limit reached; start a new session to retain more images"); } if let Some(index) = existing_index { @@ -2926,6 +2930,7 @@ impl App { self.transcript_prefixes.push(0); self.transcript_cache_width = 0; self.retained_image_source_bytes = 0; + self.image_source_limit_noticed = false; self.attachment_cache.clear(); self.transcript_focus_index = None; self.clear_attachments(); @@ -4509,21 +4514,24 @@ impl App { } if let Some(url) = self.clicked_link(column, offset) { if url.starts_with("kit-image:") { - return self - .blocks - .iter() - .find_map(|block| { - let Block::User(message) = block else { - return None; - }; - message - .images - .iter() - .find(|image| image.open_target() == url) - .cloned() - .map(Action::OpenUserImage) - }) - .unwrap_or(Action::None); + let Some(image) = self.blocks.iter().find_map(|block| { + let Block::User(message) = block else { + return None; + }; + message + .images + .iter() + .find(|image| image.open_target() == url) + }) else { + return Action::None; + }; + // Files retained by an earlier open or admission need no worker + // round trip; only misses copy the source and materialize. + if let Some(uri) = self.attachment_cache.image_uri(image.key) { + open_url(&uri); + return Action::None; + } + return Action::OpenUserImage(image.clone()); } open_url(&url); return Action::None; @@ -5407,6 +5415,7 @@ mod tests { // Real translation + worker + app + mouse routing for two distinct // overflow images on one line, including reverse image-block order. use agent_client_protocol::schema::v2::{ContentBlock, ImageContent, TextContent}; + let mut opened_uris = Vec::new(); for append in [false, true] { let id = format!("inline-{append}"); if append { @@ -5490,17 +5499,36 @@ mod tests { .into_iter() .filter(|(_, hit)| targets.contains(&hit.url)) .collect::>(); - assert_eq!( - hits.len(), - 2, - "forged internal Markdown must not create hits" - ); assert!( app.row_links .iter() .flatten() .any(|hit| hit.url == "https://example.com") ); + if append { + // Replayed keys were retained by the earlier opens: they render + // as stable file links, and the worker's fresh files are not + // admitted a second time. + assert!(hits.is_empty(), "opened keys must not stay kit-image"); + let mut linked = message + .images + .iter() + .map(|image| app.attachment_cache.image_uri(image.key).unwrap()) + .collect::>(); + linked.sort(); + let mut expected_uris = opened_uris.clone(); + expected_uris.sort(); + assert_eq!(linked, expected_uris); + for uri in &opened_uris { + assert!(app.row_links.iter().flatten().any(|hit| &hit.url == uri)); + } + continue; + } + assert_eq!( + hits.len(), + 2, + "forged internal Markdown must not create hits" + ); for ((row, hit), bytes) in hits.into_iter().zip(&expected) { let mouse = |kind| MouseEvent { kind, @@ -5535,6 +5563,7 @@ mod tests { .unwrap(); let path = url::Url::parse(&uri).unwrap().to_file_path().unwrap(); assert_eq!(std::fs::read(path).unwrap(), *bytes); + opened_uris.push(uri); } for label in ["[forged]", "#99]"] { let buffer = terminal.backend().buffer(); @@ -5718,6 +5747,34 @@ mod tests { app.blocks.last(), Some(Block::User(message)) if message.images.is_empty() )); + + // Later refusals stay quiet so the notice cannot clobber other toasts. + app.toast("copied resume command"); + let image = UserImage::new("A".repeat(source_bytes), "image/png".into(), 0).unwrap(); + app.apply(Update::UserMessage { + id: "image-4".into(), + text: "[Image #4]".into(), + images: vec![image], + append: false, + }); + assert_eq!(app.toast_text(), Some("copied resume command")); + + // A fresh session notifies again. + app.start_session("next".into()); + app.toast = None; + for index in 0..4 { + let image = UserImage::new("A".repeat(source_bytes), "image/png".into(), 0).unwrap(); + app.apply(Update::UserMessage { + id: format!("next-{index}"), + text: format!("[Image #{index}]"), + images: vec![image], + append: false, + }); + } + assert_eq!( + app.toast_text(), + Some("image source limit reached; start a new session to retain more images") + ); } fn compose(app: &mut App, script: &str) { diff --git a/src/tui/attachment.rs b/src/tui/attachment.rs index 4e0dd03..dd8d542 100644 --- a/src/tui/attachment.rs +++ b/src/tui/attachment.rs @@ -141,11 +141,7 @@ impl SessionAttachmentCache { &mut self, image: MaterializedImage, ) -> Result { - if let Some(file) = self - .files - .get(&image.key) - .or_else(|| self.opened.get(&image.key)) - { + if let Some(file) = self.file(image.key) { return url::Url::from_file_path(file.path()) .map(|uri| uri.to_string()) .map_err(|()| RetainOpenedError::InvalidPath); @@ -171,8 +167,9 @@ impl SessionAttachmentCache { } /// Admits a worker-produced file without evicting existing session links. + /// Keys already retained by an open reuse that file instead. pub(super) fn admit(&mut self, image: MaterializedImage) { - if self.files.contains_key(&image.key) + if self.file(image.key).is_some() || self.files.len() >= MAX_SESSION_FILES || image.bytes > MAX_SESSION_BYTES.saturating_sub(self.bytes) { @@ -182,8 +179,13 @@ impl SessionAttachmentCache { self.files.insert(image.key, image.file); } + fn file(&self, key: [u8; 32]) -> Option<&Arc> { + self.files.get(&key).or_else(|| self.opened.get(&key)) + } + + /// Resolves a stable link for keys held by either pool. pub(super) fn image_uri(&self, key: [u8; 32]) -> Option { - let file = self.files.get(&key)?; + let file = self.file(key)?; url::Url::from_file_path(file.path()) .ok() .map(|uri| uri.to_string()) @@ -229,6 +231,23 @@ mod tests { } } + #[test] + fn admit_and_lookup_reuse_files_retained_by_open() { + let mut cache = SessionAttachmentCache::default(); + let opened = cache.retain_opened(overflow_image(1)).unwrap(); + assert_eq!(cache.image_uri([1; 32]), Some(opened.clone())); + let duplicate = overflow_image(1); + let duplicate_path = duplicate.file.path().to_owned(); + cache.admit(duplicate); + assert!( + !duplicate_path.exists(), + "opened keys must not be admitted twice" + ); + assert!(cache.files.is_empty()); + assert_eq!(cache.bytes, 0); + assert_eq!(cache.image_uri([1; 32]), Some(opened)); + } + #[test] fn overflow_pool_reuses_duplicates_and_refuses_new_files_when_full() { let mut cache = SessionAttachmentCache::default(); diff --git a/src/tui/mod.rs b/src/tui/mod.rs index a6e0c8b..d2f967a 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -651,11 +651,13 @@ fn spawn_background_workers( Some(prepared) }) .collect(), + // Sources were bounded at ingest; materialization enforces + // its own per-image limit. Update::OpenUserImage(image) => attachment::materialize_image( image.key, &image.data, &image.mime_type, - 10 * 1024 * 1024, + usize::MAX, ) .into_iter() .collect(), @@ -2487,12 +2489,16 @@ pub async fn run_with_reasoning_effort_and_openrouter_key( // Snapshot only; release the guard before queueing work. // Poison rejects this request rather than selecting another session. let generation = transition_session.lock().ok().map(|route| route.generation); - if let Some(generation) = generation - && background_workers.try_update(QueuedUpdate::for_session( + if let Some(generation) = generation { + match background_workers.try_update(QueuedUpdate::for_session( generation, Update::OpenUserImage(image), - )).is_err() - { - app.note("image worker is busy; click again to open"); + )) { + Ok(()) => {} + Err(error) => match *error { + std::sync::mpsc::TrySendError::Full(_) => app.note("image worker is busy; click again to open"), + std::sync::mpsc::TrySendError::Disconnected(_) => app.note("image worker is unavailable"), + }, + } } } Action::Copy(text) => { diff --git a/src/tui/ui.rs b/src/tui/ui.rs index 136899b..2a9cf35 100644 --- a/src/tui/ui.rs +++ b/src/tui/ui.rs @@ -1144,11 +1144,11 @@ fn draw_transcript(frame: &mut Frame<'_>, app: &mut App, images: &mut ImageRunti } continue; } - let sources = match app.blocks.get(block_index) { - Some(Block::User(message)) => &message.images, - Some(Block::Tool(call)) => &call.images, - _ => continue, + // User images render as clickable labels, never as inline viewports. + let Some(Block::Tool(call)) = app.blocks.get(block_index) else { + continue; }; + let sources = &call.images; let Some(source) = sources.get(source_index) else { continue; }; @@ -1318,10 +1318,7 @@ fn refresh_transcript_cache_with_images(app: &mut App, images: &mut ImageRuntime } } -fn user_block_rows( - message: &UserMessage, - width: usize, -) -> (Vec, Vec) { +fn user_block_rows(message: &UserMessage, width: usize) -> Vec { let mut rows = Vec::new(); let mut line_start = 0; for (line_index, text) in message.text.split('\n').enumerate() { @@ -1351,7 +1348,7 @@ fn user_block_rows( width, )); } - (rows, Vec::new()) + rows } /// Place viewports at complete image boundaries before wrapping following prose. @@ -1557,7 +1554,7 @@ fn single_transcript_block_rows( ) -> (Vec, Vec) { let block = &app.blocks[block_index]; let (block_lines, call) = match block { - Block::User(message) => return user_block_rows(message, width), + Block::User(message) => return (user_block_rows(message, width), Vec::new()), Block::Agent(text) => return agent_block_rows(text, block_index, width, reserve_images), Block::AgentParts(parts) => { return agent_parts_rows(parts, block_index, width, reserve_images); @@ -5752,9 +5749,8 @@ mod tests { images: vec![image], }; - let (rows, placements) = user_block_rows(&message, 40); + let rows = user_block_rows(&message, 40); - assert!(placements.is_empty()); assert_eq!( rows.iter().map(|row| line_text(&row.0)).collect::>(), ["› before", " Image #1", " ", " after", " "]