From 0675944ae6914b36b8de51e89642377f84119043 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 22:43:40 -0700 Subject: [PATCH 01/19] feat: attach images mentioned in the prompt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@shot.png` reported "binary, skipped". That is never what mentioning an image means, and it was the only thing the TUI could say: an image cannot be inlined as text, and there was no path for one to reach the model. Mentioned images are now attached to the turn as image content blocks. The syntax is the one users already know, so nothing new has to be discovered, and the dead end it replaces was actively misleading. Engine side: a user message can carry attachments, and the engine holds blocks for the next turn. They are taken rather than read when the message is built — an attachment belongs to exactly one turn, and leaking it forward would re-send an image the user already shared. The setter replaces rather than appends, so a cancelled composer cannot accumulate images across attempts. Images are decoded when the turn starts rather than at mention time, so a large screenshot stays off the heap until it is needed and a read failure becomes a note instead of blocking the prompt. Only formats the API accepts are attached; other binaries still report why they were skipped rather than being sent as something unreadable. --- crates/cli/src/ui/modern/app.rs | 4 ++ crates/cli/src/ui/modern/mentions.rs | 79 ++++++++++++++++++++++++++++ crates/cli/src/ui/modern/run.rs | 24 +++++++++ crates/lib/src/llm/message.rs | 54 +++++++++++++++++++ crates/lib/src/query/mod.rs | 24 ++++++++- docs/tui/KEYBINDINGS.md | 7 +++ 6 files changed, 190 insertions(+), 2 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 41f57048..72ed2158 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -423,6 +423,8 @@ pub struct App { pub command_palette: Option, /// Ctrl+M / `/model` in-TUI model picker. pub model_picker: Option, + /// Image files mentioned in the prompt, attached to the next turn. + pub pending_images: Vec, /// User keybindings. Construction installs the built-in defaults /// only; the run loop injects the registry loaded from /// `keybindings.json` at startup. Constructors must not read the @@ -597,6 +599,7 @@ impl App { pending_task_output: None, command_palette: None, model_picker: None, + pending_images: Vec::new(), keybindings: std::sync::Arc::new( crate::ui::keybindings::KeybindingRegistry::defaults(), ), @@ -1714,6 +1717,7 @@ impl App { ) { Some(expansion) => { mention_notes = expansion.notes; + self.pending_images = expansion.images; expansion.prompt } None => text.clone(), diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index 9e3134b5..fecda3d2 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -166,6 +166,11 @@ pub struct MentionExpansion { pub prompt: String, /// Short human-readable notes about anything skipped or truncated. pub notes: Vec, + /// Image files to attach to the turn as content blocks. An image + /// cannot be inlined as text, so `@shot.png` used to report + /// "binary, skipped" — which is never what the user meant by + /// mentioning one. + pub images: Vec, } /// Inline the contents of every `@path` mention in `text`. @@ -182,6 +187,7 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let mut seen: HashSet = HashSet::new(); let mut body = String::new(); let mut notes: Vec = Vec::new(); + let mut images: Vec = Vec::new(); let mut used = 0usize; let mut over_budget = 0usize; @@ -207,6 +213,15 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let label = display_path(cwd, &path, &raw); (label, "directory", list_dir(&path)) } + Resolved::File(path) if is_image(&path) => { + // Attached rather than inlined; the model receives it as + // an image block on the turn. + let label = display_path(cwd, &path, &raw); + notes.push(format!("@{raw} — attached as an image")); + images.push(path); + let _ = label; + continue; + } Resolved::File(path) => match read_text_capped(&path, cap) { Ok(content) => (display_path(cwd, &path, &raw), "file", content), Err(reason) => { @@ -238,6 +253,7 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { Some(MentionExpansion { prompt: format!("{text}{body}"), notes, + images, }) } @@ -294,6 +310,18 @@ fn resolve_mention(cwd: &Path, raw: &str) -> Result { } } +/// Extensions the model can be shown directly. Kept to the formats the +/// API accepts, so an unsupported image still reports a clear reason +/// rather than being attached and rejected upstream. +const IMAGE_EXTENSIONS: &[&str] = &["png", "jpg", "jpeg", "gif", "webp"]; + +fn is_image(path: &Path) -> bool { + path.extension() + .and_then(|e| e.to_str()) + .map(|e| e.to_ascii_lowercase()) + .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) +} + /// True when `path` resolves inside `cwd`. Both sides are canonicalized. fn contained_in(cwd: &Path, path: &Path) -> bool { let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); @@ -664,6 +692,57 @@ mod tests { assert_eq!(out.prompt.matches(" blocks.push(block), + Err(e) => { + app.transcript + .push(super::app::TranscriptItem::System(format!( + "could not attach {}: {e}", + path.display() + ))); + } + } + } + if let Ok(mut eng) = session.engine().try_lock() { + eng.set_pending_attachments(blocks); + } + } let sink = ChannelSink::new(eng_tx.clone()); match session.spawn_turn(prompt.clone(), sink).await { Ok(handle) => { diff --git a/crates/lib/src/llm/message.rs b/crates/lib/src/llm/message.rs index 05c7e23c..8e56feff 100644 --- a/crates/lib/src/llm/message.rs +++ b/crates/lib/src/llm/message.rs @@ -240,6 +240,29 @@ pub enum StopReason { } /// Helper to create a user message with text content. +/// A user message carrying attachments alongside its text. +/// +/// Images go *before* the text: a model reads the prompt as being about +/// the images it has just been shown, and the reverse order reads as an +/// afterthought. +pub fn user_message_with_attachments( + text: impl Into, + attachments: Vec, +) -> Message { + let text = text.into(); + let mut content = attachments; + if !text.is_empty() { + content.push(ContentBlock::Text { text }); + } + Message::User(UserMessage { + uuid: Uuid::new_v4(), + timestamp: chrono::Utc::now().to_rfc3339(), + content, + is_meta: false, + is_compact_summary: false, + }) +} + pub fn user_message(text: impl Into) -> Message { Message::User(UserMessage { uuid: Uuid::new_v4(), @@ -505,6 +528,37 @@ pub fn messages_to_api_params_cached(messages: &[Message]) -> Vec>, session_allows: Arc>>, + /// Content blocks to prepend to the next user message (images from + /// the composer). Consumed by the turn that follows. + pending_attachments: Vec, permission_prompter: Option>, question_asker: Option>, /// Cached system prompt (rebuilt only when inputs change). @@ -219,6 +222,7 @@ impl QueryEngine { crate::memory::extraction::ExtractionState::new(), )), session_allows: Arc::new(tokio::sync::Mutex::new(std::collections::HashSet::new())), + pending_attachments: Vec::new(), permission_prompter: None, question_asker: None, cached_system_prompt: None, @@ -276,6 +280,14 @@ impl QueryEngine { /// (see `tools::executor`), so the interactive TUI would silently execute /// mutating tools under `ask` mode. The CLI installs a prompter on the /// interactive path only; one-shot/non-interactive runs leave it unset. + /// Attach content blocks to the next user turn. + /// + /// Replaces rather than appends, so a cancelled composer cannot + /// accumulate images across attempts. + pub fn set_pending_attachments(&mut self, blocks: Vec) { + self.pending_attachments = blocks; + } + pub fn set_permission_prompter(&mut self, prompter: Arc) { self.permission_prompter = Some(prompter); } @@ -736,8 +748,16 @@ impl QueryEngine { user_input: &str, sink: &dyn StreamSink, ) -> crate::error::Result<()> { - // Add the user message to history. - let user_msg = user_message(user_input); + // Add the user message to history, with anything the caller + // attached for this turn. Taken rather than read: an attachment + // belongs to exactly one turn, and leaking it into the next one + // would re-send an image the user already shared. + let attachments = std::mem::take(&mut self.pending_attachments); + let user_msg = if attachments.is_empty() { + user_message(user_input) + } else { + crate::llm::message::user_message_with_attachments(user_input, attachments) + }; self.state.push_message(user_msg); // UserPromptSubmit fires once per user turn, as soon as the diff --git a/docs/tui/KEYBINDINGS.md b/docs/tui/KEYBINDINGS.md index 638aa9b6..eb12c2ab 100644 --- a/docs/tui/KEYBINDINGS.md +++ b/docs/tui/KEYBINDINGS.md @@ -172,3 +172,10 @@ binding that runs never discards the prompt you were composing. The file is read once at startup; `/keybindings` lists the bindings active in the current session — after editing the file, restart to apply. + +## Images + +Mention an image the way you mention a file — `@screenshot.png` — and it is +attached to the turn as an image rather than inlined as text. Supported: +`png`, `jpg`/`jpeg`, `gif`, `webp`. Other binaries are still skipped with a +reason. From 9428001df60aae55aca45cb401f6708df75d70d4 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 22:58:59 -0700 Subject: [PATCH 02/19] fix(tui): bound image attachments, normalize image MIME, drop stale ones Image mentions bypassed every mention budget: each path was accepted without a size or count limit, then read whole and base64-encoded on the UI thread at turn start. Give attachments their own budget (3 MiB per image, 8 MiB and 4 images per prompt), enforced before a path is accepted and again at load time, since the file can grow in between. A file whose size cannot be read is refused rather than attached. Media-type inference matched the extension case-sensitively while mention detection lowercased it, so a mentioned shot.PNG was attached as application/octet-stream and rejected upstream. Only the mention branch assigned pending_images, so replacing a staged prompt (two interjections while a turn cancels) sent the previous prompt's image with the replacement. Clear on every enqueue, set the engine's attachments unconditionally so an empty set clears a stale one, and restore both prompt and images together when a spawn fails. --- crates/cli/src/ui/modern/app.rs | 61 ++++++++++++ crates/cli/src/ui/modern/mentions.rs | 141 +++++++++++++++++++++++++-- crates/cli/src/ui/modern/run.rs | 41 +++++--- crates/lib/src/llm/message.rs | 63 ++++++++++-- crates/lib/src/query/mod.rs | 12 +-- 5 files changed, 279 insertions(+), 39 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 72ed2158..a5162eab 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -1669,6 +1669,12 @@ impl App { /// Resolve user text into a turn: expand `/skill` invocations the same /// way slash dispatch does via `commands::execute` skill lookup. fn enqueue_turn(&mut self, text: String) { + // Attachments belong to exactly one prompt. A staged prompt can be + // replaced before it starts (two interjections while a turn is + // cancelling), and only the mention branch assigns `pending_images` + // — so clear here, or the replacement turn would carry the previous + // prompt's image and disclose a file the user did not mean to send. + self.pending_images.clear(); let mut mention_notes: Vec = Vec::new(); let (display, prompt) = match try_expand_skill_slash_full(&text, &self.cwd, self.disable_skill_shell) { @@ -4501,4 +4507,59 @@ mod tests { app.submit(); assert_eq!(app.pending_submit.as_deref(), Some("hello there")); } + + fn write_png(app: &App, name: &str) { + std::fs::write( + std::path::Path::new(&app.cwd).join(name), + [0x89, b'P', b'N', b'G', 0x0d, 0x0a, 0x1a, 0x0a], + ) + .unwrap(); + } + + #[test] + fn submitting_an_image_mention_stages_it_for_the_turn() { + let (_dir, mut app) = app_in_workspace(); + write_png(&app, "shot.png"); + type_input(&mut app, "look at @shot.png"); + app.submit(); + assert_eq!(app.pending_images.len(), 1, "image was not staged"); + } + + /// Replacing a staged prompt must drop its attachments: the second + /// prompt would otherwise ship the first one's image and disclose a + /// file the user never meant to send with it. + #[test] + fn replacing_a_staged_prompt_drops_its_images() { + let (_dir, mut app) = app_in_workspace(); + write_png(&app, "shot.png"); + type_input(&mut app, "look at @shot.png"); + app.submit(); + assert_eq!(app.pending_images.len(), 1, "precondition"); + + // Interject again with a prompt that mentions nothing. + type_input(&mut app, "actually never mind"); + app.interject(); + assert_eq!(app.pending_submit.as_deref(), Some("actually never mind")); + assert!( + app.pending_images.is_empty(), + "stale image carried onto the replacement prompt" + ); + } + + /// The skill branch never touches `pending_images` either, so a skill + /// prompt must not inherit the previous prompt's attachment. + #[test] + fn a_replacement_slash_command_drops_staged_images() { + let (_dir, mut app) = app_in_workspace(); + write_png(&app, "shot.png"); + type_input(&mut app, "look at @shot.png"); + app.submit(); + assert_eq!(app.pending_images.len(), 1, "precondition"); + + app.enqueue_turn_from_command("summarize the repo".into()); + assert!( + app.pending_images.is_empty(), + "stale image carried onto a command-produced turn" + ); + } } diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index fecda3d2..5ae2ce55 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -26,6 +26,21 @@ pub const MAX_FILE_BYTES: usize = 64 * 1024; /// the worst case at four full-size files; the rest are skipped with a note. pub const MAX_TOTAL_BYTES: usize = 256 * 1024; +/// Per-image cap. An attachment is read whole and base64-encoded on the UI +/// thread, and base64 inflates by 4/3, so 3 MiB is the largest file that +/// still fits the 5 MB per-image payload the providers accept. +pub const MAX_IMAGE_BYTES: usize = 3 * 1024 * 1024; + +/// Cap across every image in one prompt. Images do not consume the text +/// budget — they never enter the prompt string — so they need a budget of +/// their own; without one `@*.png` over a screenshot directory can freeze +/// or OOM the CLI before the request is ever built. +pub const MAX_TOTAL_IMAGE_BYTES: usize = 8 * 1024 * 1024; + +/// Upper bound on attachments per prompt, independent of their size: each +/// one costs a full re-encode and a large block of context. +pub const MAX_IMAGES: usize = 4; + /// Upper bound on directory entries examined for one completion, so a /// pathological directory cannot stall the UI thread. pub const MAX_SCAN_ENTRIES: usize = 4_000; @@ -189,7 +204,9 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let mut notes: Vec = Vec::new(); let mut images: Vec = Vec::new(); let mut used = 0usize; + let mut image_bytes = 0usize; let mut over_budget = 0usize; + let mut over_image_budget = 0usize; for raw in mentions { if !seen.insert(raw.clone()) { @@ -202,6 +219,27 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { continue; } }; + // Attached rather than inlined; the model receives it as an image + // block on the turn. Budgeted before the path is accepted — the + // loader would otherwise encode an unbounded blob on the UI thread. + if let Resolved::File(ref path) = resolved + && is_image(path) + { + if images.len() >= MAX_IMAGES { + over_image_budget += 1; + continue; + } + match image_size_within_cap(path) { + Ok(len) if image_bytes + len <= MAX_TOTAL_IMAGE_BYTES => { + image_bytes += len; + notes.push(format!("@{raw} — attached as an image")); + images.push(path.clone()); + } + Ok(_) => over_image_budget += 1, + Err(reason) => notes.push(format!("@{raw} — {reason}")), + } + continue; + } let remaining = MAX_TOTAL_BYTES.saturating_sub(used); if remaining == 0 { over_budget += 1; @@ -213,15 +251,6 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let label = display_path(cwd, &path, &raw); (label, "directory", list_dir(&path)) } - Resolved::File(path) if is_image(&path) => { - // Attached rather than inlined; the model receives it as - // an image block on the turn. - let label = display_path(cwd, &path, &raw); - notes.push(format!("@{raw} — attached as an image")); - images.push(path); - let _ = label; - continue; - } Resolved::File(path) => match read_text_capped(&path, cap) { Ok(content) => (display_path(cwd, &path, &raw), "file", content), Err(reason) => { @@ -249,6 +278,12 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { MAX_TOTAL_BYTES / 1024 )); } + if over_image_budget > 0 { + notes.push(format!( + "{over_image_budget} image(s) skipped — at most {MAX_IMAGES} images / {} MiB per prompt", + MAX_TOTAL_IMAGE_BYTES / (1024 * 1024) + )); + } Some(MentionExpansion { prompt: format!("{text}{body}"), @@ -322,6 +357,28 @@ fn is_image(path: &Path) -> bool { .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) } +/// Byte length of an image that is small enough to attach, or the reason it +/// must be refused. +/// +/// Fail-closed: a file whose size cannot be determined is refused rather +/// than attached, because the loader would otherwise read and base64-encode +/// an unbounded blob on the UI thread. Re-checked at load time as well as at +/// mention time — the file can grow in between. +pub fn image_size_within_cap(path: &Path) -> Result { + let len = std::fs::metadata(path) + .map_err(|e| format!("image unreadable ({e})"))? + .len(); + let len = usize::try_from(len).map_err(|_| "image too large".to_string())?; + if len > MAX_IMAGE_BYTES { + return Err(format!( + "image too large ({:.1} MiB, max {} MiB)", + len as f64 / (1024.0 * 1024.0), + MAX_IMAGE_BYTES / (1024 * 1024) + )); + } + Ok(len) +} + /// True when `path` resolves inside `cwd`. Both sides are canonicalized. fn contained_in(cwd: &Path, path: &Path) -> bool { let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); @@ -732,6 +789,72 @@ mod tests { assert_eq!(out.images.len(), 3, "{:?}", out.notes); } + /// A single oversized image is refused before it is ever read: the + /// loader base64-encodes on the UI thread, so an unbounded file froze + /// or OOM'd the CLI before the request was built. + #[test] + fn an_oversized_image_is_refused_with_a_note() { + let dir = fixture(); + fs::write( + dir.path().join("huge.png"), + vec![0u8; MAX_IMAGE_BYTES + 1024], + ) + .unwrap(); + let out = expand_mentions("look at @huge.png", dir.path()).expect("expanded"); + assert!(out.images.is_empty(), "oversized image was attached"); + assert!( + out.notes.iter().any(|n| n.contains("image too large")), + "no note explaining the refusal: {:?}", + out.notes + ); + } + + #[test] + fn image_attachments_are_capped_by_count() { + let dir = fixture(); + for i in 0..(MAX_IMAGES + 3) { + fs::write(dir.path().join(format!("s{i}.png")), [0x89, 0, 1]).unwrap(); + } + let text: String = (0..(MAX_IMAGES + 3)) + .map(|i| format!("@s{i}.png ")) + .collect(); + let out = expand_mentions(&text, dir.path()).expect("expanded"); + assert_eq!(out.images.len(), MAX_IMAGES, "{:?}", out.notes); + assert!( + out.notes.iter().any(|n| n.contains("image(s) skipped")), + "no note about the skipped images: {:?}", + out.notes + ); + } + + #[test] + fn image_attachments_are_capped_by_total_bytes() { + let dir = fixture(); + // Each is under the per-image cap; together they exceed the total. + let each = MAX_TOTAL_IMAGE_BYTES / 3 + 1024; + assert!(each <= MAX_IMAGE_BYTES, "fixture must stay under the cap"); + for name in ["a.png", "b.png", "c.png"] { + fs::write(dir.path().join(name), vec![0u8; each]).unwrap(); + } + let out = expand_mentions("@a.png @b.png @c.png", dir.path()).expect("expanded"); + assert_eq!( + out.images.len(), + 2, + "total image budget not enforced: {:?}", + out.notes + ); + assert!(out.notes.iter().any(|n| n.contains("image(s) skipped"))); + } + + /// Fail-closed: an image whose size cannot be read is refused, not + /// attached and hoped for. + #[test] + fn an_unmeasurable_image_is_refused() { + let dir = fixture(); + let missing = dir.path().join("gone.png"); + assert!(image_size_within_cap(&missing).is_err()); + } + /// A non-image binary still reports why it was skipped, rather than /// being attached as something the model cannot read. #[test] diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index b31f70c3..031592b0 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -576,23 +576,30 @@ pub(super) async fn event_loop( // starts, and lets a read failure be reported as a note // instead of blocking the prompt. let images = std::mem::take(&mut app.pending_images); - if !images.is_empty() { - let mut blocks = Vec::new(); - for path in images { - match agent_code_lib::llm::message::image_block_from_file(&path) { - Ok(block) => blocks.push(block), - Err(e) => { - app.transcript - .push(super::app::TranscriptItem::System(format!( - "could not attach {}: {e}", - path.display() - ))); - } + let mut blocks = Vec::new(); + for path in &images { + // Size re-checked here, not just at mention time: the file + // can grow in between, and this is the read that would + // actually allocate it. + let loaded = super::mentions::image_size_within_cap(path) + .and_then(|_| agent_code_lib::llm::message::image_block_from_file(path)); + match loaded { + Ok(block) => blocks.push(block), + Err(e) => { + app.transcript + .push(super::app::TranscriptItem::System(format!( + "could not attach {}: {e}", + path.display() + ))); } } - if let Ok(mut eng) = session.engine().try_lock() { - eng.set_pending_attachments(blocks); - } + } + // Set unconditionally, awaiting the lock rather than skipping on + // contention: an empty set clears anything a previous attempt + // staged, so no turn can inherit another turn's attachment. + { + let engine = session.engine(); + engine.lock().await.set_pending_attachments(blocks); } let sink = ChannelSink::new(eng_tx.clone()); match session.spawn_turn(prompt.clone(), sink).await { @@ -602,8 +609,10 @@ pub(super) async fn event_loop( } Err(e) => { // Should be rare: TUI serializes turns. Put the prompt - // back so the next idle loop can retry. + // and its attachments back so the next idle loop retries + // them together. app.pending_submit = Some(prompt); + app.pending_images = images; app.status_message = format!("turn busy: {e}"); app.dirty = true; } diff --git a/crates/lib/src/llm/message.rs b/crates/lib/src/llm/message.rs index 8e56feff..5aa805ac 100644 --- a/crates/lib/src/llm/message.rs +++ b/crates/lib/src/llm/message.rs @@ -273,6 +273,23 @@ pub fn user_message(text: impl Into) -> Message { }) } +/// Media type for an image path, inferred from its extension. +/// +/// Case-insensitive: callers that decide *whether* a path is an image +/// normalize the extension, so `shot.PNG` must not fall through to a +/// generic media type the provider then rejects. +pub fn image_media_type(path: &std::path::Path) -> Option<&'static str> { + let ext = path.extension()?.to_str()?.to_ascii_lowercase(); + Some(match ext.as_str() { + "png" => "image/png", + "jpg" | "jpeg" => "image/jpeg", + "gif" => "image/gif", + "webp" => "image/webp", + "svg" => "image/svg+xml", + _ => return None, + }) +} + /// Helper to create an image content block from a file path. /// /// Reads the file, base64-encodes it, and infers the media type @@ -280,14 +297,7 @@ pub fn user_message(text: impl Into) -> Message { pub fn image_block_from_file(path: &std::path::Path) -> Result { let data = std::fs::read(path).map_err(|e| format!("Failed to read image: {e}"))?; - let media_type = match path.extension().and_then(|e| e.to_str()) { - Some("png") => "image/png", - Some("jpg" | "jpeg") => "image/jpeg", - Some("gif") => "image/gif", - Some("webp") => "image/webp", - Some("svg") => "image/svg+xml", - _ => "application/octet-stream", - }; + let media_type = image_media_type(path).unwrap_or("application/octet-stream"); use std::io::Write; let mut encoded = String::new(); @@ -546,6 +556,43 @@ mod tests { assert!(!u.is_meta, "an attachment turn is still real user input"); } + /// Mention detection accepts `shot.PNG` by lowercasing the extension; + /// inferring the media type case-sensitively handed the provider an + /// `application/octet-stream` block it rejects. + #[test] + fn the_media_type_is_inferred_case_insensitively() { + use std::path::Path; + for (name, expected) in [ + ("shot.PNG", "image/png"), + ("shot.png", "image/png"), + ("photo.Jpeg", "image/jpeg"), + ("photo.JPG", "image/jpeg"), + ("anim.GIF", "image/gif"), + ("pic.WebP", "image/webp"), + ("art.SVG", "image/svg+xml"), + ] { + assert_eq!( + image_media_type(Path::new(name)), + Some(expected), + "wrong media type for {name}" + ); + } + assert_eq!(image_media_type(Path::new("blob.bin")), None); + assert_eq!(image_media_type(Path::new("noext")), None); + } + + #[test] + fn an_uppercase_image_file_still_gets_a_real_media_type() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("SHOT.PNG"); + std::fs::write(&path, [0x89, b'P', b'N', b'G']).unwrap(); + let block = image_block_from_file(&path).expect("encoded"); + let ContentBlock::Image { media_type, .. } = block else { + panic!("expected an image block"); + }; + assert_eq!(media_type, "image/png"); + } + #[test] fn an_attachment_with_no_text_carries_only_the_attachment() { let img = ContentBlock::Image { diff --git a/crates/lib/src/query/mod.rs b/crates/lib/src/query/mod.rs index 8dc8cf0a..975eca03 100644 --- a/crates/lib/src/query/mod.rs +++ b/crates/lib/src/query/mod.rs @@ -274,12 +274,6 @@ impl QueryEngine { sink.on_context_usage(used, DEFAULT_CONTEXT_WINDOW); } - /// Install the interactive permission prompter. - /// - /// Without this, an `Ask` permission decision falls through to auto-allow - /// (see `tools::executor`), so the interactive TUI would silently execute - /// mutating tools under `ask` mode. The CLI installs a prompter on the - /// interactive path only; one-shot/non-interactive runs leave it unset. /// Attach content blocks to the next user turn. /// /// Replaces rather than appends, so a cancelled composer cannot @@ -288,6 +282,12 @@ impl QueryEngine { self.pending_attachments = blocks; } + /// Install the interactive permission prompter. + /// + /// Without this, an `Ask` permission decision falls through to auto-allow + /// (see `tools::executor`), so the interactive TUI would silently execute + /// mutating tools under `ask` mode. The CLI installs a prompter on the + /// interactive path only; one-shot/non-interactive runs leave it unset. pub fn set_permission_prompter(&mut self, prompter: Arc) { self.permission_prompter = Some(prompter); } From a07d06107616b0107cddeb1a404b59492c755eaa Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:07:28 -0700 Subject: [PATCH 03/19] fix(llm): encode image payloads and re-apply the image budget on load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The base64 helper only emitted output from `flush()`, and the single caller dropped the writer after `write_all`, so every image block shipped with an empty `data` string — the provider received no image at all. Replace the writer with a whole-input encoder and cover it with the RFC 4648 vectors, which is exactly where the padded remainders were lost. Attachment loading re-checked only the per-image cap, so four files that each grew from 2 MiB to 3 MiB after being mentioned would load 12 MiB against an advertised 8 MiB bound. Move loading into `load_image_blocks`, which re-applies the size, total and count limits and reads through a capped reader rather than trusting the size it just measured. --- crates/cli/src/ui/modern/mentions.rs | 136 +++++++++++++++++++++++++++ crates/cli/src/ui/modern/run.rs | 21 +---- crates/lib/src/llm/message.rs | 130 +++++++++++++------------ 3 files changed, 211 insertions(+), 76 deletions(-) diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index 5ae2ce55..44806094 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -357,6 +357,25 @@ fn is_image(path: &Path) -> bool { .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) } +/// Read at most `cap` bytes, refusing a file that has more. +/// +/// `take(cap + 1)` so exceeding the cap is *observed* rather than silently +/// truncated — a half-read image would be sent as a corrupt attachment. +fn read_capped(path: &Path, cap: usize) -> Result, String> { + let file = std::fs::File::open(path).map_err(|e| format!("image unreadable ({e})"))?; + let mut data = Vec::new(); + file.take(cap as u64 + 1) + .read_to_end(&mut data) + .map_err(|e| format!("image unreadable ({e})"))?; + if data.len() > cap { + return Err(format!( + "image too large (over {} MiB)", + cap / (1024 * 1024) + )); + } + Ok(data) +} + /// Byte length of an image that is small enough to attach, or the reason it /// must be refused. /// @@ -379,6 +398,58 @@ pub fn image_size_within_cap(path: &Path) -> Result { Ok(len) } +/// Read and encode staged image attachments, re-applying the full budget. +/// +/// Returns the blocks to attach plus a note for every path refused. The +/// budget is enforced again here, not only at mention time: a staged file +/// can grow (or be replaced) between the mention and the turn actually +/// starting, and this is the point where the bytes are really read and +/// base64-encoded. Every limit is re-checked — per-image size, the +/// per-prompt total, and the count — so no combination of edits between +/// the two points can exceed what was advertised. +pub fn load_image_blocks( + paths: &[PathBuf], +) -> (Vec, Vec) { + let mut blocks = Vec::new(); + let mut notes = Vec::new(); + let mut total = 0usize; + for path in paths { + let name = path.display(); + if blocks.len() >= MAX_IMAGES { + notes.push(format!( + "could not attach {name}: at most {MAX_IMAGES} images" + )); + continue; + } + if let Err(reason) = image_size_within_cap(path) { + notes.push(format!("could not attach {name}: {reason}")); + continue; + } + // Read through a cap rather than trusting the size just measured: + // the file can still grow between the stat and the read, and an + // unbounded read is exactly what the budget exists to prevent. + let data = match read_capped(path, MAX_IMAGE_BYTES) { + Ok(data) => data, + Err(reason) => { + notes.push(format!("could not attach {name}: {reason}")); + continue; + } + }; + if total + data.len() > MAX_TOTAL_IMAGE_BYTES { + notes.push(format!( + "could not attach {name}: {} MiB total image limit reached", + MAX_TOTAL_IMAGE_BYTES / (1024 * 1024) + )); + continue; + } + total += data.len(); + blocks.push(agent_code_lib::llm::message::image_block_from_bytes( + path, &data, + )); + } + (blocks, notes) +} + /// True when `path` resolves inside `cwd`. Both sides are canonicalized. fn contained_in(cwd: &Path, path: &Path) -> bool { let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); @@ -846,6 +917,71 @@ mod tests { assert!(out.notes.iter().any(|n| n.contains("image(s) skipped"))); } + /// The budget is re-applied when the bytes are actually read: a staged + /// file can grow between the mention and the turn starting, so checking + /// only the per-image cap there let the per-prompt total be exceeded. + #[test] + fn loading_reapplies_the_total_image_budget() { + let dir = fixture(); + let each = MAX_TOTAL_IMAGE_BYTES / 3 + 1024; + let mut paths = Vec::new(); + for name in ["a.png", "b.png", "c.png"] { + let p = dir.path().join(name); + fs::write(&p, vec![0u8; each]).unwrap(); + paths.push(p); + } + let (blocks, notes) = load_image_blocks(&paths); + assert_eq!(blocks.len(), 2, "total budget not enforced at load time"); + assert!( + notes.iter().any(|n| n.contains("total image limit")), + "no note about the refused image: {notes:?}" + ); + } + + #[test] + fn loading_refuses_a_file_that_grew_past_the_per_image_cap() { + let dir = fixture(); + let path = dir.path().join("grew.png"); + fs::write(&path, vec![0u8; MAX_IMAGE_BYTES + 1]).unwrap(); + let (blocks, notes) = load_image_blocks(&[path]); + assert!(blocks.is_empty(), "oversized image was attached at load"); + assert!( + notes.iter().any(|n| n.contains("too large")), + "no note about the refusal: {notes:?}" + ); + } + + #[test] + fn loading_caps_the_attachment_count() { + let dir = fixture(); + let mut paths = Vec::new(); + for i in 0..(MAX_IMAGES + 2) { + let p = dir.path().join(format!("s{i}.png")); + fs::write(&p, [0x89, b'P', b'N', b'G']).unwrap(); + paths.push(p); + } + let (blocks, notes) = load_image_blocks(&paths); + assert_eq!(blocks.len(), MAX_IMAGES); + assert_eq!(notes.len(), 2, "{notes:?}"); + } + + /// The encoder used to emit nothing unless flushed, so an attachment + /// reached the provider as an empty payload. + #[test] + fn loading_produces_a_non_empty_encoded_payload() { + use agent_code_lib::llm::message::ContentBlock; + let dir = fixture(); + let path = dir.path().join("shot.png"); + fs::write(&path, [0x89, b'P', b'N', b'G']).unwrap(); + let (blocks, notes) = load_image_blocks(&[path]); + assert_eq!(blocks.len(), 1, "{notes:?}"); + let ContentBlock::Image { media_type, data } = &blocks[0] else { + panic!("expected an image block"); + }; + assert_eq!(media_type, "image/png"); + assert_eq!(data, "iVBORw=="); + } + /// Fail-closed: an image whose size cannot be read is refused, not /// attached and hoped for. #[test] diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 031592b0..6d4097c1 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -576,23 +576,10 @@ pub(super) async fn event_loop( // starts, and lets a read failure be reported as a note // instead of blocking the prompt. let images = std::mem::take(&mut app.pending_images); - let mut blocks = Vec::new(); - for path in &images { - // Size re-checked here, not just at mention time: the file - // can grow in between, and this is the read that would - // actually allocate it. - let loaded = super::mentions::image_size_within_cap(path) - .and_then(|_| agent_code_lib::llm::message::image_block_from_file(path)); - match loaded { - Ok(block) => blocks.push(block), - Err(e) => { - app.transcript - .push(super::app::TranscriptItem::System(format!( - "could not attach {}: {e}", - path.display() - ))); - } - } + let (blocks, refused) = super::mentions::load_image_blocks(&images); + for note in refused { + app.transcript + .push(super::app::TranscriptItem::System(note)); } // Set unconditionally, awaiting the lock rather than skipping on // contention: an empty set clears anything a previous attempt diff --git a/crates/lib/src/llm/message.rs b/crates/lib/src/llm/message.rs index 5aa805ac..6ce62e3a 100644 --- a/crates/lib/src/llm/message.rs +++ b/crates/lib/src/llm/message.rs @@ -296,73 +296,56 @@ pub fn image_media_type(path: &std::path::Path) -> Option<&'static str> { /// from the file extension. pub fn image_block_from_file(path: &std::path::Path) -> Result { let data = std::fs::read(path).map_err(|e| format!("Failed to read image: {e}"))?; - - let media_type = image_media_type(path).unwrap_or("application/octet-stream"); - - use std::io::Write; - let mut encoded = String::new(); - { - let mut encoder = base64_encode_writer(&mut encoded); - encoder - .write_all(&data) - .map_err(|e| format!("base64 error: {e}"))?; - } - - Ok(ContentBlock::Image { - media_type: media_type.to_string(), - data: encoded, - }) + Ok(image_block_from_bytes(path, &data)) } -/// Simple base64 encoder (no external dependency). -fn base64_encode_writer(output: &mut String) -> Base64Writer<'_> { - Base64Writer { - output, - buffer: Vec::new(), +/// Build an image block from bytes already in hand. +/// +/// Lets a caller that must bound the read do it itself and still get the +/// same media-type inference; `path` is used only for its extension. +pub fn image_block_from_bytes(path: &std::path::Path, data: &[u8]) -> ContentBlock { + ContentBlock::Image { + media_type: image_media_type(path) + .unwrap_or("application/octet-stream") + .to_string(), + data: base64_encode(data), } } -struct Base64Writer<'a> { - output: &'a mut String, - buffer: Vec, -} - -impl<'a> std::io::Write for Base64Writer<'a> { - fn write(&mut self, buf: &[u8]) -> std::io::Result { - self.buffer.extend_from_slice(buf); - Ok(buf.len()) +/// Base64 (RFC 4648, padded) without an external dependency. +/// +/// A whole-input function rather than an `io::Write` adaptor: the writer +/// this replaced only encoded from `flush()`, and every caller dropped it +/// after `write_all`, so every image block shipped with an empty payload. +fn base64_encode(data: &[u8]) -> String { + const CHARS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; + let mut out = String::with_capacity(data.len().div_ceil(3) * 4); + let mut chunks = data.chunks_exact(3); + for c in &mut chunks { + let (b0, b1, b2) = (c[0] as usize, c[1] as usize, c[2] as usize); + out.push(CHARS[b0 >> 2] as char); + out.push(CHARS[((b0 & 3) << 4) | (b1 >> 4)] as char); + out.push(CHARS[((b1 & 0xf) << 2) | (b2 >> 6)] as char); + out.push(CHARS[b2 & 0x3f] as char); } - fn flush(&mut self) -> std::io::Result<()> { - const CHARS: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/"; - let mut i = 0; - while i + 2 < self.buffer.len() { - let b0 = self.buffer[i] as usize; - let b1 = self.buffer[i + 1] as usize; - let b2 = self.buffer[i + 2] as usize; - self.output.push(CHARS[b0 >> 2] as char); - self.output.push(CHARS[((b0 & 3) << 4) | (b1 >> 4)] as char); - self.output - .push(CHARS[((b1 & 0xf) << 2) | (b2 >> 6)] as char); - self.output.push(CHARS[b2 & 0x3f] as char); - i += 3; + match *chunks.remainder() { + [b0] => { + let b0 = b0 as usize; + out.push(CHARS[b0 >> 2] as char); + out.push(CHARS[(b0 & 3) << 4] as char); + out.push('='); + out.push('='); } - let remaining = self.buffer.len() - i; - if remaining == 1 { - let b0 = self.buffer[i] as usize; - self.output.push(CHARS[b0 >> 2] as char); - self.output.push(CHARS[(b0 & 3) << 4] as char); - self.output.push('='); - self.output.push('='); - } else if remaining == 2 { - let b0 = self.buffer[i] as usize; - let b1 = self.buffer[i + 1] as usize; - self.output.push(CHARS[b0 >> 2] as char); - self.output.push(CHARS[((b0 & 3) << 4) | (b1 >> 4)] as char); - self.output.push(CHARS[(b1 & 0xf) << 2] as char); - self.output.push('='); + [b0, b1] => { + let (b0, b1) = (b0 as usize, b1 as usize); + out.push(CHARS[b0 >> 2] as char); + out.push(CHARS[((b0 & 3) << 4) | (b1 >> 4)] as char); + out.push(CHARS[(b1 & 0xf) << 2] as char); + out.push('='); } - Ok(()) + _ => {} } + out } /// Helper to create a user message with an image. @@ -587,10 +570,39 @@ mod tests { let path = dir.path().join("SHOT.PNG"); std::fs::write(&path, [0x89, b'P', b'N', b'G']).unwrap(); let block = image_block_from_file(&path).expect("encoded"); - let ContentBlock::Image { media_type, .. } = block else { + let ContentBlock::Image { media_type, data } = block else { panic!("expected an image block"); }; assert_eq!(media_type, "image/png"); + // The encoder this replaced only produced output from `flush()`, + // which no caller invoked: every image shipped with empty data. + assert_eq!(data, "iVBORw==", "image payload was not encoded"); + } + + /// RFC 4648 §10 test vectors — the padded remainders are exactly where + /// the previous encoder silently produced nothing. + #[test] + fn base64_matches_the_rfc_vectors() { + for (input, expected) in [ + ("", ""), + ("f", "Zg=="), + ("fo", "Zm8="), + ("foo", "Zm9v"), + ("foob", "Zm9vYg=="), + ("fooba", "Zm9vYmE="), + ("foobar", "Zm9vYmFy"), + ] { + assert_eq!(base64_encode(input.as_bytes()), expected, "input {input:?}"); + } + } + + #[test] + fn base64_encodes_every_byte_value() { + let all: Vec = (0..=255u8).collect(); + let encoded = base64_encode(&all); + assert_eq!(encoded.len(), 344, "256 bytes encode to 344 base64 chars"); + assert!(encoded.starts_with("AAECAwQF")); + assert!(encoded.ends_with("+/w==")); } #[test] From 26be7b40b15964c3ba5f5b1a01cd479001c62497 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:09:19 -0700 Subject: [PATCH 04/19] test(tui): restamp golden frames for the 0.28.0 version line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merging the v0.28.0 release into this branch changed the version shown in the status bar; the four golden frames still asserted 0.27.0. Only that token differs — the style rows are untouched. --- crates/cli/tests/snapshots/idle_frame.txt | 2 +- crates/cli/tests/snapshots/permission_modal.txt | 2 +- crates/cli/tests/snapshots/transcript_basic.txt | 2 +- crates/cli/tests/snapshots/transcript_light.txt | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/cli/tests/snapshots/idle_frame.txt b/crates/cli/tests/snapshots/idle_frame.txt index 5bfb4883..10f7a6ef 100644 --- a/crates/cli/tests/snapshots/idle_frame.txt +++ b/crates/cli/tests/snapshots/idle_frame.txt @@ -1,5 +1,5 @@ == glyphs == -agent-code 0.27.0 test-model NORMAL /w +agent-code 0.28.0 test-model NORMAL /w ──────────────────────────────────────────────────────────────────────────────── transcript diff --git a/crates/cli/tests/snapshots/permission_modal.txt b/crates/cli/tests/snapshots/permission_modal.txt index a3dc85a3..f75f17ac 100644 --- a/crates/cli/tests/snapshots/permission_modal.txt +++ b/crates/cli/tests/snapshots/permission_modal.txt @@ -1,5 +1,5 @@ == glyphs == -agent-code 0.27.0 test-model NORMAL /w +agent-code 0.28.0 test-model NORMAL /w ──────────────────────────────────────────────────────────────────────────────── ⠋ action required diff --git a/crates/cli/tests/snapshots/transcript_basic.txt b/crates/cli/tests/snapshots/transcript_basic.txt index 88332f57..70a96c8b 100644 --- a/crates/cli/tests/snapshots/transcript_basic.txt +++ b/crates/cli/tests/snapshots/transcript_basic.txt @@ -1,5 +1,5 @@ == glyphs == -agent-code 0.27.0 test-model NORMAL /w +agent-code 0.28.0 test-model NORMAL /w ──────────────────────────────────────────────────────────────────────────────── transcript diff --git a/crates/cli/tests/snapshots/transcript_light.txt b/crates/cli/tests/snapshots/transcript_light.txt index 4c904a8f..6bcdefd7 100644 --- a/crates/cli/tests/snapshots/transcript_light.txt +++ b/crates/cli/tests/snapshots/transcript_light.txt @@ -1,5 +1,5 @@ == glyphs == -agent-code 0.27.0 test-model NORMAL /w +agent-code 0.28.0 test-model NORMAL /w ──────────────────────────────────────────────────────────────────────────────── transcript From 964c7c4ffd9c591d4241318fd28dc15ae594e947 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:17:33 -0700 Subject: [PATCH 05/19] fix(tui): revalidate staged image paths at load time MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An image path is accepted during mention expansion but opened later, when the turn actually starts, and the loader trusted the earlier decision. Replacing the file in between defeated every check `resolve_mention` had made: a symlink to a file outside the workspace was read and encoded into the request, and a FIFO would have hung the UI thread inside `open`. Re-canonicalize at load time and re-establish workspace containment, the `.git/` exclusion, the extension and regular-file status on the resolved target, then open with `O_NOFOLLOW | O_NONBLOCK` and decide on the descriptor's own `fstat` — the name can change again after the check, but the opened file cannot. --- crates/cli/src/ui/modern/mentions.rs | 140 +++++++++++++++++++++++++-- crates/cli/src/ui/modern/run.rs | 3 +- 2 files changed, 133 insertions(+), 10 deletions(-) diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index 44806094..360eb2a8 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -357,12 +357,72 @@ fn is_image(path: &Path) -> bool { .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) } +/// Re-check a staged attachment against the rules its mention passed. +/// +/// A path is accepted at mention time but opened later, when the turn +/// starts. Anything can happen in between: replacing `shot.png` with a +/// symlink out of the workspace would otherwise leak that file's bytes to +/// the provider, and replacing it with a FIFO would hang the UI on `open`. +/// So containment, `.git/`, the extension and regular-file status are all +/// re-established here, on the *canonical* target. +fn revalidate_image(cwd: &Path, path: &Path) -> Result { + let canon = path + .canonicalize() + .map_err(|e| format!("image unreadable ({e})"))?; + if !contained_in(cwd, &canon) { + return Err("image left the workspace".into()); + } + let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); + if let Ok(rel) = canon.strip_prefix(&cwd_canon) + && rel.components().any(|c| c.as_os_str() == ".git") + { + return Err("image is inside .git/".into()); + } + if !is_image(&canon) { + return Err("not an image".into()); + } + if !std::fs::symlink_metadata(&canon) + .map_err(|e| format!("image unreadable ({e})"))? + .is_file() + { + return Err("not a regular file".into()); + } + Ok(canon) +} + +/// Open a validated path without trusting it to still be a regular file. +/// +/// `O_NOFOLLOW` refuses a symlink swapped in after canonicalization, and +/// `O_NONBLOCK` keeps `open` from hanging on a FIFO; the `fstat` on the +/// descriptor is what finally decides, since it describes the file that +/// was actually opened rather than whatever the name points at now. +fn open_regular_file(path: &Path) -> Result { + let mut options = std::fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); + } + let file = options + .open(path) + .map_err(|e| format!("image unreadable ({e})"))?; + if !file + .metadata() + .map_err(|e| format!("image unreadable ({e})"))? + .is_file() + { + return Err("not a regular file".into()); + } + Ok(file) +} + /// Read at most `cap` bytes, refusing a file that has more. /// /// `take(cap + 1)` so exceeding the cap is *observed* rather than silently /// truncated — a half-read image would be sent as a corrupt attachment. fn read_capped(path: &Path, cap: usize) -> Result, String> { - let file = std::fs::File::open(path).map_err(|e| format!("image unreadable ({e})"))?; + let file = open_regular_file(path)?; let mut data = Vec::new(); file.take(cap as u64 + 1) .read_to_end(&mut data) @@ -406,8 +466,11 @@ pub fn image_size_within_cap(path: &Path) -> Result { /// starting, and this is the point where the bytes are really read and /// base64-encoded. Every limit is re-checked — per-image size, the /// per-prompt total, and the count — so no combination of edits between -/// the two points can exceed what was advertised. +/// the two points can exceed what was advertised. The path itself is +/// re-validated too: acceptance at mention time says nothing about what +/// the name points at once the turn finally starts. pub fn load_image_blocks( + cwd: &Path, paths: &[PathBuf], ) -> (Vec, Vec) { let mut blocks = Vec::new(); @@ -421,14 +484,21 @@ pub fn load_image_blocks( )); continue; } - if let Err(reason) = image_size_within_cap(path) { + let path = match revalidate_image(cwd, path) { + Ok(p) => p, + Err(reason) => { + notes.push(format!("could not attach {name}: {reason}")); + continue; + } + }; + if let Err(reason) = image_size_within_cap(&path) { notes.push(format!("could not attach {name}: {reason}")); continue; } // Read through a cap rather than trusting the size just measured: // the file can still grow between the stat and the read, and an // unbounded read is exactly what the budget exists to prevent. - let data = match read_capped(path, MAX_IMAGE_BYTES) { + let data = match read_capped(&path, MAX_IMAGE_BYTES) { Ok(data) => data, Err(reason) => { notes.push(format!("could not attach {name}: {reason}")); @@ -444,7 +514,7 @@ pub fn load_image_blocks( } total += data.len(); blocks.push(agent_code_lib::llm::message::image_block_from_bytes( - path, &data, + &path, &data, )); } (blocks, notes) @@ -930,7 +1000,7 @@ mod tests { fs::write(&p, vec![0u8; each]).unwrap(); paths.push(p); } - let (blocks, notes) = load_image_blocks(&paths); + let (blocks, notes) = load_image_blocks(dir.path(), &paths); assert_eq!(blocks.len(), 2, "total budget not enforced at load time"); assert!( notes.iter().any(|n| n.contains("total image limit")), @@ -943,7 +1013,7 @@ mod tests { let dir = fixture(); let path = dir.path().join("grew.png"); fs::write(&path, vec![0u8; MAX_IMAGE_BYTES + 1]).unwrap(); - let (blocks, notes) = load_image_blocks(&[path]); + let (blocks, notes) = load_image_blocks(dir.path(), &[path]); assert!(blocks.is_empty(), "oversized image was attached at load"); assert!( notes.iter().any(|n| n.contains("too large")), @@ -960,7 +1030,7 @@ mod tests { fs::write(&p, [0x89, b'P', b'N', b'G']).unwrap(); paths.push(p); } - let (blocks, notes) = load_image_blocks(&paths); + let (blocks, notes) = load_image_blocks(dir.path(), &paths); assert_eq!(blocks.len(), MAX_IMAGES); assert_eq!(notes.len(), 2, "{notes:?}"); } @@ -973,7 +1043,7 @@ mod tests { let dir = fixture(); let path = dir.path().join("shot.png"); fs::write(&path, [0x89, b'P', b'N', b'G']).unwrap(); - let (blocks, notes) = load_image_blocks(&[path]); + let (blocks, notes) = load_image_blocks(dir.path(), &[path]); assert_eq!(blocks.len(), 1, "{notes:?}"); let ContentBlock::Image { media_type, data } = &blocks[0] else { panic!("expected an image block"); @@ -982,6 +1052,58 @@ mod tests { assert_eq!(data, "iVBORw=="); } + /// A staged path is opened long after it was accepted. Swapping it for + /// a symlink out of the workspace must not leak the target's bytes to + /// the provider. + #[cfg(unix)] + #[test] + fn loading_refuses_a_path_swapped_for_an_escaping_symlink() { + let outside = tempfile::tempdir().expect("tempdir"); + let secret = outside.path().join("secret.png"); + fs::write(&secret, b"exfiltrate me").unwrap(); + + let dir = fixture(); + let staged = dir.path().join("shot.png"); + fs::write(&staged, [0x89, b'P', b'N', b'G']).unwrap(); + let out = expand_mentions("look at @shot.png", dir.path()).expect("expanded"); + assert_eq!(out.images.len(), 1, "precondition"); + + // The turn has not started yet; the file is replaced underneath it. + fs::remove_file(&staged).unwrap(); + std::os::unix::fs::symlink(&secret, &staged).unwrap(); + + let (blocks, notes) = load_image_blocks(dir.path(), &out.images); + assert!(blocks.is_empty(), "read a file outside the workspace"); + assert!( + notes.iter().any(|n| n.contains("left the workspace")), + "no note about the escape: {notes:?}" + ); + } + + /// Replacing the staged file with a FIFO used to hang the UI thread on + /// `open`; it must be refused instead. + #[cfg(unix)] + #[test] + fn loading_refuses_a_path_swapped_for_a_fifo() { + let dir = fixture(); + let staged = dir.path().join("shot.png"); + fs::write(&staged, [0x89, b'P', b'N', b'G']).unwrap(); + let out = expand_mentions("look at @shot.png", dir.path()).expect("expanded"); + assert_eq!(out.images.len(), 1, "precondition"); + + fs::remove_file(&staged).unwrap(); + let c = std::ffi::CString::new(staged.as_os_str().as_encoded_bytes()).unwrap(); + // SAFETY: `c` is a valid NUL-terminated path for the duration. + assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o600) }, 0, "mkfifo"); + + let (blocks, notes) = load_image_blocks(dir.path(), &out.images); + assert!(blocks.is_empty(), "attached a FIFO"); + assert!( + notes.iter().any(|n| n.contains("not a regular file")), + "no note about the FIFO: {notes:?}" + ); + } + /// Fail-closed: an image whose size cannot be read is refused, not /// attached and hoped for. #[test] diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 6d4097c1..a04054f0 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -576,7 +576,8 @@ pub(super) async fn event_loop( // starts, and lets a read failure be reported as a note // instead of blocking the prompt. let images = std::mem::take(&mut app.pending_images); - let (blocks, refused) = super::mentions::load_image_blocks(&images); + let (blocks, refused) = + super::mentions::load_image_blocks(std::path::Path::new(&app.cwd), &images); for note in refused { app.transcript .push(super::app::TranscriptItem::System(note)); From 432de705555b96c246b01f38e81ca9148f64d4d6 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:27:22 -0700 Subject: [PATCH 06/19] fix(tui): read a mentioned image while its path is still validated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Attachments carried a path from mention expansion to the start of the turn, and re-opened it there. Nothing about a path survives that wait: replacing the file — or any ancestor directory — with a symlink pointed the later open at a file outside the workspace that had never been validated, and re-checking the name only moved the window rather than closing it, since the check and the open are two separate lookups. Read and encode the image where the text mentions are already read: immediately after `resolve_mention`, on the path it just validated. The turn then carries content blocks, not names, so there is nothing left to re-check and no second lookup to redirect. Peak memory stays bounded by the same budget as before, which is what made deferring the read unnecessary. `O_NOFOLLOW | O_NONBLOCK` plus the descriptor's own fstat keep the one remaining open honest: it cannot follow a symlink swapped in after the check, and it cannot block on a FIFO. --- crates/cli/src/ui/modern/app.rs | 6 +- crates/cli/src/ui/modern/mentions.rs | 349 +++++++++------------------ crates/cli/src/ui/modern/run.rs | 17 +- 3 files changed, 125 insertions(+), 247 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index a5162eab..2a60ef63 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -423,8 +423,10 @@ pub struct App { pub command_palette: Option, /// Ctrl+M / `/model` in-TUI model picker. pub model_picker: Option, - /// Image files mentioned in the prompt, attached to the next turn. - pub pending_images: Vec, + /// Images mentioned in the prompt, attached to the next turn. Already + /// encoded: they are read while the mention is being validated, so no + /// path has to be trusted a second time when the turn starts. + pub pending_images: Vec, /// User keybindings. Construction installs the built-in defaults /// only; the run loop injects the registry loaded from /// `keybindings.json` at startup. Constructors must not read the diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index 360eb2a8..a36a9e18 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -175,17 +175,24 @@ pub fn mention_text(candidate: &str) -> String { } /// Result of inlining `@path` mentions into a prompt. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone)] pub struct MentionExpansion { /// What the engine should receive: the user's text plus file blocks. pub prompt: String, /// Short human-readable notes about anything skipped or truncated. pub notes: Vec, - /// Image files to attach to the turn as content blocks. An image - /// cannot be inlined as text, so `@shot.png` used to report - /// "binary, skipped" — which is never what the user meant by - /// mentioning one. - pub images: Vec, + /// Images to attach to the turn as content blocks. An image cannot be + /// inlined as text, so `@shot.png` used to report "binary, skipped" — + /// which is never what the user meant by mentioning one. + /// + /// Encoded here, not at turn start. Carrying paths meant re-opening + /// them after an unbounded delay, and nothing about a path survives + /// that wait: swapping the file (or any ancestor directory) for a + /// symlink pointed the later `open` at a file outside the workspace + /// that had never been validated. Reading the bytes while the path is + /// still the one `resolve_mention` just checked is the same discipline + /// the text mentions above follow, and it leaves nothing to re-check. + pub images: Vec, } /// Inline the contents of every `@path` mention in `text`. @@ -202,7 +209,7 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let mut seen: HashSet = HashSet::new(); let mut body = String::new(); let mut notes: Vec = Vec::new(); - let mut images: Vec = Vec::new(); + let mut images: Vec = Vec::new(); let mut used = 0usize; let mut image_bytes = 0usize; let mut over_budget = 0usize; @@ -220,8 +227,9 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { } }; // Attached rather than inlined; the model receives it as an image - // block on the turn. Budgeted before the path is accepted — the - // loader would otherwise encode an unbounded blob on the UI thread. + // block on the turn. Budgeted before a byte is read — an image is + // held whole and base64-encoded, so an unbounded one would freeze + // or OOM the UI thread. if let Resolved::File(ref path) = resolved && is_image(path) { @@ -229,11 +237,20 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { over_image_budget += 1; continue; } - match image_size_within_cap(path) { - Ok(len) if image_bytes + len <= MAX_TOTAL_IMAGE_BYTES => { - image_bytes += len; + if image_bytes >= MAX_TOTAL_IMAGE_BYTES { + over_image_budget += 1; + continue; + } + match read_image_capped(path, MAX_IMAGE_BYTES) { + // Checked after the read, so the note distinguishes "this + // one is too big" from "the prompt is full"; the read is + // capped either way, so the peak stays bounded. + Ok(data) if image_bytes + data.len() <= MAX_TOTAL_IMAGE_BYTES => { + image_bytes += data.len(); notes.push(format!("@{raw} — attached as an image")); - images.push(path.clone()); + images.push(agent_code_lib::llm::message::image_block_from_bytes( + path, &data, + )); } Ok(_) => over_image_budget += 1, Err(reason) => notes.push(format!("@{raw} — {reason}")), @@ -357,45 +374,13 @@ fn is_image(path: &Path) -> bool { .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) } -/// Re-check a staged attachment against the rules its mention passed. +/// Open a path that `resolve_mention` just validated, without trusting the +/// name to still mean the same file. /// -/// A path is accepted at mention time but opened later, when the turn -/// starts. Anything can happen in between: replacing `shot.png` with a -/// symlink out of the workspace would otherwise leak that file's bytes to -/// the provider, and replacing it with a FIFO would hang the UI on `open`. -/// So containment, `.git/`, the extension and regular-file status are all -/// re-established here, on the *canonical* target. -fn revalidate_image(cwd: &Path, path: &Path) -> Result { - let canon = path - .canonicalize() - .map_err(|e| format!("image unreadable ({e})"))?; - if !contained_in(cwd, &canon) { - return Err("image left the workspace".into()); - } - let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); - if let Ok(rel) = canon.strip_prefix(&cwd_canon) - && rel.components().any(|c| c.as_os_str() == ".git") - { - return Err("image is inside .git/".into()); - } - if !is_image(&canon) { - return Err("not an image".into()); - } - if !std::fs::symlink_metadata(&canon) - .map_err(|e| format!("image unreadable ({e})"))? - .is_file() - { - return Err("not a regular file".into()); - } - Ok(canon) -} - -/// Open a validated path without trusting it to still be a regular file. -/// -/// `O_NOFOLLOW` refuses a symlink swapped in after canonicalization, and -/// `O_NONBLOCK` keeps `open` from hanging on a FIFO; the `fstat` on the -/// descriptor is what finally decides, since it describes the file that -/// was actually opened rather than whatever the name points at now. +/// `O_NOFOLLOW` refuses a symlink swapped in since the check, `O_NONBLOCK` +/// keeps `open` from hanging on a FIFO, and the `fstat` on the descriptor +/// is what finally decides — it describes the file that was actually +/// opened rather than whatever the name points at now. fn open_regular_file(path: &Path) -> Result { let mut options = std::fs::OpenOptions::new(); options.read(true); @@ -406,10 +391,10 @@ fn open_regular_file(path: &Path) -> Result { } let file = options .open(path) - .map_err(|e| format!("image unreadable ({e})"))?; + .map_err(|e| format!("unreadable ({})", e.kind()))?; if !file .metadata() - .map_err(|e| format!("image unreadable ({e})"))? + .map_err(|e| format!("unreadable ({})", e.kind()))? .is_file() { return Err("not a regular file".into()); @@ -417,16 +402,17 @@ fn open_regular_file(path: &Path) -> Result { Ok(file) } -/// Read at most `cap` bytes, refusing a file that has more. +/// Read an image, refusing one larger than `cap` rather than truncating it. /// -/// `take(cap + 1)` so exceeding the cap is *observed* rather than silently -/// truncated — a half-read image would be sent as a corrupt attachment. -fn read_capped(path: &Path, cap: usize) -> Result, String> { +/// `take(cap + 1)` so exceeding the cap is *observed*: a half-read image +/// would otherwise be attached as a corrupt payload. The cap is what keeps +/// an oversized file from being held and base64-encoded on the UI thread. +fn read_image_capped(path: &Path, cap: usize) -> Result, String> { let file = open_regular_file(path)?; let mut data = Vec::new(); file.take(cap as u64 + 1) .read_to_end(&mut data) - .map_err(|e| format!("image unreadable ({e})"))?; + .map_err(|e| format!("unreadable ({})", e.kind()))?; if data.len() > cap { return Err(format!( "image too large (over {} MiB)", @@ -436,90 +422,6 @@ fn read_capped(path: &Path, cap: usize) -> Result, String> { Ok(data) } -/// Byte length of an image that is small enough to attach, or the reason it -/// must be refused. -/// -/// Fail-closed: a file whose size cannot be determined is refused rather -/// than attached, because the loader would otherwise read and base64-encode -/// an unbounded blob on the UI thread. Re-checked at load time as well as at -/// mention time — the file can grow in between. -pub fn image_size_within_cap(path: &Path) -> Result { - let len = std::fs::metadata(path) - .map_err(|e| format!("image unreadable ({e})"))? - .len(); - let len = usize::try_from(len).map_err(|_| "image too large".to_string())?; - if len > MAX_IMAGE_BYTES { - return Err(format!( - "image too large ({:.1} MiB, max {} MiB)", - len as f64 / (1024.0 * 1024.0), - MAX_IMAGE_BYTES / (1024 * 1024) - )); - } - Ok(len) -} - -/// Read and encode staged image attachments, re-applying the full budget. -/// -/// Returns the blocks to attach plus a note for every path refused. The -/// budget is enforced again here, not only at mention time: a staged file -/// can grow (or be replaced) between the mention and the turn actually -/// starting, and this is the point where the bytes are really read and -/// base64-encoded. Every limit is re-checked — per-image size, the -/// per-prompt total, and the count — so no combination of edits between -/// the two points can exceed what was advertised. The path itself is -/// re-validated too: acceptance at mention time says nothing about what -/// the name points at once the turn finally starts. -pub fn load_image_blocks( - cwd: &Path, - paths: &[PathBuf], -) -> (Vec, Vec) { - let mut blocks = Vec::new(); - let mut notes = Vec::new(); - let mut total = 0usize; - for path in paths { - let name = path.display(); - if blocks.len() >= MAX_IMAGES { - notes.push(format!( - "could not attach {name}: at most {MAX_IMAGES} images" - )); - continue; - } - let path = match revalidate_image(cwd, path) { - Ok(p) => p, - Err(reason) => { - notes.push(format!("could not attach {name}: {reason}")); - continue; - } - }; - if let Err(reason) = image_size_within_cap(&path) { - notes.push(format!("could not attach {name}: {reason}")); - continue; - } - // Read through a cap rather than trusting the size just measured: - // the file can still grow between the stat and the read, and an - // unbounded read is exactly what the budget exists to prevent. - let data = match read_capped(&path, MAX_IMAGE_BYTES) { - Ok(data) => data, - Err(reason) => { - notes.push(format!("could not attach {name}: {reason}")); - continue; - } - }; - if total + data.len() > MAX_TOTAL_IMAGE_BYTES { - notes.push(format!( - "could not attach {name}: {} MiB total image limit reached", - MAX_TOTAL_IMAGE_BYTES / (1024 * 1024) - )); - continue; - } - total += data.len(); - blocks.push(agent_code_lib::llm::message::image_block_from_bytes( - &path, &data, - )); - } - (blocks, notes) -} - /// True when `path` resolves inside `cwd`. Both sides are canonicalized. fn contained_in(cwd: &Path, path: &Path) -> bool { let cwd_canon = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); @@ -639,6 +541,7 @@ fn truncate_utf8(s: String, cap: usize) -> (String, Option) { #[cfg(test)] mod tests { use super::*; + use agent_code_lib::llm::message::ContentBlock; use std::fs; fn fixture() -> tempfile::TempDir { @@ -908,7 +811,11 @@ mod tests { "image was not attached: {:?}", out.notes ); - assert!(out.images[0].ends_with("shot.png")); + let ContentBlock::Image { media_type, data } = &out.images[0] else { + panic!("expected an image block"); + }; + assert_eq!(media_type, "image/png"); + assert!(!data.is_empty(), "image was attached with an empty payload"); assert!( out.notes.iter().any(|n| n.contains("attached as an image")), "no note explaining the attachment: {:?}", @@ -987,130 +894,106 @@ mod tests { assert!(out.notes.iter().any(|n| n.contains("image(s) skipped"))); } - /// The budget is re-applied when the bytes are actually read: a staged - /// file can grow between the mention and the turn starting, so checking - /// only the per-image cap there let the per-prompt total be exceeded. - #[test] - fn loading_reapplies_the_total_image_budget() { - let dir = fixture(); - let each = MAX_TOTAL_IMAGE_BYTES / 3 + 1024; - let mut paths = Vec::new(); - for name in ["a.png", "b.png", "c.png"] { - let p = dir.path().join(name); - fs::write(&p, vec![0u8; each]).unwrap(); - paths.push(p); - } - let (blocks, notes) = load_image_blocks(dir.path(), &paths); - assert_eq!(blocks.len(), 2, "total budget not enforced at load time"); - assert!( - notes.iter().any(|n| n.contains("total image limit")), - "no note about the refused image: {notes:?}" - ); - } - - #[test] - fn loading_refuses_a_file_that_grew_past_the_per_image_cap() { - let dir = fixture(); - let path = dir.path().join("grew.png"); - fs::write(&path, vec![0u8; MAX_IMAGE_BYTES + 1]).unwrap(); - let (blocks, notes) = load_image_blocks(dir.path(), &[path]); - assert!(blocks.is_empty(), "oversized image was attached at load"); - assert!( - notes.iter().any(|n| n.contains("too large")), - "no note about the refusal: {notes:?}" - ); - } - - #[test] - fn loading_caps_the_attachment_count() { - let dir = fixture(); - let mut paths = Vec::new(); - for i in 0..(MAX_IMAGES + 2) { - let p = dir.path().join(format!("s{i}.png")); - fs::write(&p, [0x89, b'P', b'N', b'G']).unwrap(); - paths.push(p); - } - let (blocks, notes) = load_image_blocks(dir.path(), &paths); - assert_eq!(blocks.len(), MAX_IMAGES); - assert_eq!(notes.len(), 2, "{notes:?}"); - } - - /// The encoder used to emit nothing unless flushed, so an attachment - /// reached the provider as an empty payload. + /// The bytes are read while the mention is being validated, so the + /// block the turn ships is the file that passed the check — not + /// whatever the name pointed at some seconds later. #[test] - fn loading_produces_a_non_empty_encoded_payload() { - use agent_code_lib::llm::message::ContentBlock; + fn an_attached_image_carries_its_encoded_bytes() { let dir = fixture(); - let path = dir.path().join("shot.png"); - fs::write(&path, [0x89, b'P', b'N', b'G']).unwrap(); - let (blocks, notes) = load_image_blocks(dir.path(), &[path]); - assert_eq!(blocks.len(), 1, "{notes:?}"); - let ContentBlock::Image { media_type, data } = &blocks[0] else { + fs::write(dir.path().join("shot.png"), [0x89, b'P', b'N', b'G']).unwrap(); + let out = expand_mentions("@shot.png", dir.path()).expect("expanded"); + assert_eq!(out.images.len(), 1, "{:?}", out.notes); + let ContentBlock::Image { media_type, data } = &out.images[0] else { panic!("expected an image block"); }; assert_eq!(media_type, "image/png"); assert_eq!(data, "iVBORw=="); } - /// A staged path is opened long after it was accepted. Swapping it for - /// a symlink out of the workspace must not leak the target's bytes to - /// the provider. + /// A symlink pointing out of the workspace is refused, so an image + /// mention cannot exfiltrate a file the workspace never contained. #[cfg(unix)] #[test] - fn loading_refuses_a_path_swapped_for_an_escaping_symlink() { + fn an_image_symlinked_outside_the_workspace_is_refused() { let outside = tempfile::tempdir().expect("tempdir"); let secret = outside.path().join("secret.png"); fs::write(&secret, b"exfiltrate me").unwrap(); let dir = fixture(); - let staged = dir.path().join("shot.png"); - fs::write(&staged, [0x89, b'P', b'N', b'G']).unwrap(); - let out = expand_mentions("look at @shot.png", dir.path()).expect("expanded"); - assert_eq!(out.images.len(), 1, "precondition"); - - // The turn has not started yet; the file is replaced underneath it. - fs::remove_file(&staged).unwrap(); - std::os::unix::fs::symlink(&secret, &staged).unwrap(); + std::os::unix::fs::symlink(&secret, dir.path().join("shot.png")).unwrap(); - let (blocks, notes) = load_image_blocks(dir.path(), &out.images); - assert!(blocks.is_empty(), "read a file outside the workspace"); + let out = expand_mentions("look at @shot.png", dir.path()).expect("expanded"); + assert!(out.images.is_empty(), "read a file outside the workspace"); assert!( - notes.iter().any(|n| n.contains("left the workspace")), - "no note about the escape: {notes:?}" + out.notes + .iter() + .any(|n| n.contains("outside the workspace")), + "no note about the escape: {:?}", + out.notes ); } - /// Replacing the staged file with a FIFO used to hang the UI thread on - /// `open`; it must be refused instead. + /// A FIFO named like an image must not be attached — and must not hang + /// the UI thread inside `open` while it waits for a writer. #[cfg(unix)] #[test] - fn loading_refuses_a_path_swapped_for_a_fifo() { + fn a_fifo_named_like_an_image_is_refused() { let dir = fixture(); - let staged = dir.path().join("shot.png"); - fs::write(&staged, [0x89, b'P', b'N', b'G']).unwrap(); + let path = dir.path().join("shot.png"); + let c = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()).unwrap(); + // SAFETY: `c` is a valid NUL-terminated path for the call. + assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o600) }, 0, "mkfifo"); + let out = expand_mentions("look at @shot.png", dir.path()).expect("expanded"); - assert_eq!(out.images.len(), 1, "precondition"); + assert!(out.images.is_empty(), "attached a FIFO"); + assert!( + out.notes.iter().any(|n| n.contains("not a regular file")), + "no note about the FIFO: {:?}", + out.notes + ); + } - fs::remove_file(&staged).unwrap(); - let c = std::ffi::CString::new(staged.as_os_str().as_encoded_bytes()).unwrap(); - // SAFETY: `c` is a valid NUL-terminated path for the duration. + /// `open_regular_file` is the last line of defence if a path stops + /// being a regular file between the check and the open. + #[cfg(unix)] + #[test] + fn opening_a_non_regular_file_is_refused() { + let dir = fixture(); + let path = dir.path().join("pipe.png"); + let c = std::ffi::CString::new(path.as_os_str().as_encoded_bytes()).unwrap(); + // SAFETY: `c` is a valid NUL-terminated path for the call. assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o600) }, 0, "mkfifo"); + assert_eq!( + open_regular_file(&path).unwrap_err(), + "not a regular file", + "a FIFO must never be opened for attachment" + ); + } - let (blocks, notes) = load_image_blocks(dir.path(), &out.images); - assert!(blocks.is_empty(), "attached a FIFO"); + /// A symlink swapped in after the check must not be followed by the + /// open that reads the bytes. + #[cfg(unix)] + #[test] + fn opening_refuses_to_follow_a_symlink() { + let outside = tempfile::tempdir().expect("tempdir"); + let target = outside.path().join("secret.png"); + fs::write(&target, b"exfiltrate me").unwrap(); + let dir = fixture(); + let link = dir.path().join("link.png"); + std::os::unix::fs::symlink(&target, &link).unwrap(); assert!( - notes.iter().any(|n| n.contains("not a regular file")), - "no note about the FIFO: {notes:?}" + open_regular_file(&link).is_err(), + "open followed a symlink out of the workspace" ); } - /// Fail-closed: an image whose size cannot be read is refused, not - /// attached and hoped for. + /// Fail-closed: an image that cannot be read is refused, not attached + /// and hoped for. #[test] - fn an_unmeasurable_image_is_refused() { + fn an_unreadable_image_is_refused() { let dir = fixture(); let missing = dir.path().join("gone.png"); - assert!(image_size_within_cap(&missing).is_err()); + assert!(read_image_capped(&missing, MAX_IMAGE_BYTES).is_err()); } /// A non-image binary still reports why it was skipped, rather than diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index a04054f0..5c444859 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -570,24 +570,17 @@ pub(super) async fn event_loop( if turn.is_none() && let Some(prompt) = app.pending_submit.take() { - // Load any mentioned images and hand them to the engine for - // this turn. Decoding here rather than at mention time keeps - // a big screenshot out of memory until the turn actually - // starts, and lets a read failure be reported as a note - // instead of blocking the prompt. + // Hand this turn's images to the engine. They were read and + // encoded during mention expansion, while the path was still + // the one that had just been validated — nothing is opened + // here, so there is no second chance to point them elsewhere. let images = std::mem::take(&mut app.pending_images); - let (blocks, refused) = - super::mentions::load_image_blocks(std::path::Path::new(&app.cwd), &images); - for note in refused { - app.transcript - .push(super::app::TranscriptItem::System(note)); - } // Set unconditionally, awaiting the lock rather than skipping on // contention: an empty set clears anything a previous attempt // staged, so no turn can inherit another turn's attachment. { let engine = session.engine(); - engine.lock().await.set_pending_attachments(blocks); + engine.lock().await.set_pending_attachments(images.clone()); } let sink = ChannelSink::new(eng_tx.clone()); match session.spawn_turn(prompt.clone(), sink).await { From 8d8a3576233a29202f2de078a07d339b8c5e8c95 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:38:27 -0700 Subject: [PATCH 07/19] fix(tui): resolve staged images through directory descriptors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `O_NOFOLLOW` guards only the final component, so an image staged as `sub/shot.png` could still be redirected by replacing `sub` with a symlink: the open walked the new ancestor and the fstat that followed merely confirmed the outside file was regular. Windows had no no-follow handling at all. Open each component relative to the descriptor of the one before it, starting from the workspace root, refusing symlinks the whole way down — a rearranged tree cannot steer a lookup that never restarts. Windows uses `FILE_FLAG_OPEN_REPARSE_POINT`, which turns a swapped-in symlink into a reparse point that fails the regular-file check rather than redirecting the read. The descriptor is then held until the turn starts, so the bytes are read from the file that was validated rather than from a name resolved a second time, and that read moves to the blocking pool: a workspace on a slow mount was stalling the event loop, and with it redraws and cancellation, for the length of the read. --- crates/cli/src/ui/modern/app.rs | 8 +- crates/cli/src/ui/modern/mentions.rs | 324 ++++++++++++++++++++++----- crates/cli/src/ui/modern/run.rs | 31 ++- 3 files changed, 296 insertions(+), 67 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 2a60ef63..1f25421c 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -423,10 +423,10 @@ pub struct App { pub command_palette: Option, /// Ctrl+M / `/model` in-TUI model picker. pub model_picker: Option, - /// Images mentioned in the prompt, attached to the next turn. Already - /// encoded: they are read while the mention is being validated, so no - /// path has to be trusted a second time when the turn starts. - pub pending_images: Vec, + /// Images mentioned in the prompt, attached to the next turn. Held as + /// open descriptors from the moment their mention was validated, so + /// starting the turn needs no second look at any path. + pub pending_images: Vec, /// User keybindings. Construction installs the built-in defaults /// only; the run loop injects the registry loaded from /// `keybindings.json` at startup. Constructors must not read the diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index a36a9e18..f233d9a0 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -181,18 +181,17 @@ pub struct MentionExpansion { pub prompt: String, /// Short human-readable notes about anything skipped or truncated. pub notes: Vec, - /// Images to attach to the turn as content blocks. An image cannot be - /// inlined as text, so `@shot.png` used to report "binary, skipped" — - /// which is never what the user meant by mentioning one. + /// Images to attach to the turn, held open. An image cannot be inlined + /// as text, so `@shot.png` used to report "binary, skipped" — which is + /// never what the user meant by mentioning one. /// - /// Encoded here, not at turn start. Carrying paths meant re-opening - /// them after an unbounded delay, and nothing about a path survives - /// that wait: swapping the file (or any ancestor directory) for a - /// symlink pointed the later `open` at a file outside the workspace - /// that had never been validated. Reading the bytes while the path is - /// still the one `resolve_mention` just checked is the same discipline - /// the text mentions above follow, and it leaves nothing to re-check. - pub images: Vec, + /// Descriptors, not paths. A path handed to the turn would be resolved + /// a second time, and swapping the file — or any ancestor directory — + /// for a symlink in between pointed that second lookup at a file + /// outside the workspace which had never been validated. Opening while + /// the mention is being checked leaves nothing to look up again; the + /// bytes are read later, off the UI thread, from these descriptors. + pub images: Vec, } /// Inline the contents of every `@path` mention in `text`. @@ -209,7 +208,7 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { let mut seen: HashSet = HashSet::new(); let mut body = String::new(); let mut notes: Vec = Vec::new(); - let mut images: Vec = Vec::new(); + let mut images: Vec = Vec::new(); let mut used = 0usize; let mut image_bytes = 0usize; let mut over_budget = 0usize; @@ -237,20 +236,11 @@ pub fn expand_mentions(text: &str, cwd: &Path) -> Option { over_image_budget += 1; continue; } - if image_bytes >= MAX_TOTAL_IMAGE_BYTES { - over_image_budget += 1; - continue; - } - match read_image_capped(path, MAX_IMAGE_BYTES) { - // Checked after the read, so the note distinguishes "this - // one is too big" from "the prompt is full"; the read is - // capped either way, so the peak stays bounded. - Ok(data) if image_bytes + data.len() <= MAX_TOTAL_IMAGE_BYTES => { - image_bytes += data.len(); + match stage_image(cwd, path) { + Ok((staged, len)) if image_bytes + len <= MAX_TOTAL_IMAGE_BYTES => { + image_bytes += len; notes.push(format!("@{raw} — attached as an image")); - images.push(agent_code_lib::llm::message::image_block_from_bytes( - path, &data, - )); + images.push(staged); } Ok(_) => over_image_budget += 1, Err(reason) => notes.push(format!("@{raw} — {reason}")), @@ -374,20 +364,124 @@ fn is_image(path: &Path) -> bool { .is_some_and(|e| IMAGE_EXTENSIONS.contains(&e.as_str())) } -/// Open a path that `resolve_mention` just validated, without trusting the -/// name to still mean the same file. +/// A validated image, held open until the turn that carries it starts. +/// +/// The descriptor *is* the validation result. Handing the turn a path +/// would mean resolving that name a second time, and a name resolved +/// twice can mean two different files — the whole point of opening here. +#[derive(Debug, Clone)] +pub struct StagedImage { + /// Kept for the media type and for error messages only; never + /// re-opened. + pub path: PathBuf, + pub file: std::sync::Arc, +} + +/// Open `path` without letting any component of it be redirected. +/// +/// `path` must already be canonical and inside `root`. Each component is +/// opened relative to the descriptor of the one before it, refusing +/// symlinks — so replacing an ancestor directory (or the file itself) +/// between validation and this open cannot walk the read outside the +/// workspace. Resolving the pathname again instead would re-run the whole +/// lookup against a tree the attacker has had time to rearrange. /// -/// `O_NOFOLLOW` refuses a symlink swapped in since the check, `O_NONBLOCK` -/// keeps `open` from hanging on a FIFO, and the `fstat` on the descriptor -/// is what finally decides — it describes the file that was actually -/// opened rather than whatever the name points at now. -fn open_regular_file(path: &Path) -> Result { +/// The final `fstat` is on the descriptor, so it describes the file that +/// was actually opened rather than whatever the name means afterwards. +#[cfg(unix)] +fn open_beneath(root: &Path, path: &Path) -> Result { + use std::ffi::CString; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::io::{AsRawFd, FromRawFd}; + + let rel = path + .strip_prefix(root) + .map_err(|_| "outside the workspace".to_string())?; + + // The workspace root is the trust anchor: it is the directory the + // session was started in, not something a mention chose. + let mut dir = std::fs::File::open(root).map_err(|e| format!("unreadable ({})", e.kind()))?; + + let components: Vec<_> = rel.components().collect(); + let Some((last, parents)) = components.split_last() else { + return Err("not a regular file".into()); + }; + for component in components.iter() { + // A canonical path relative to its own prefix has only normal + // components; anything else means the assumption broke. + if !matches!(component, std::path::Component::Normal(_)) { + return Err("not a usable path".into()); + } + } + + for component in parents { + let name = CString::new(component.as_os_str().as_bytes()) + .map_err(|_| "not a usable path".to_string())?; + // SAFETY: `dir` is an open directory descriptor and `name` is a + // valid NUL-terminated path for the duration of the call. + let fd = unsafe { + libc::openat( + dir.as_raw_fd(), + name.as_ptr(), + libc::O_RDONLY | libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_CLOEXEC, + ) + }; + if fd < 0 { + return Err(format!( + "unreadable ({})", + std::io::Error::last_os_error().kind() + )); + } + // SAFETY: `fd` was just returned by `openat` and is owned here. + dir = unsafe { std::fs::File::from_raw_fd(fd) }; + } + + let name = + CString::new(last.as_os_str().as_bytes()).map_err(|_| "not a usable path".to_string())?; + // SAFETY: as above; `O_NONBLOCK` additionally keeps a FIFO swapped in + // for the file from blocking this call until a writer appears. + let fd = unsafe { + libc::openat( + dir.as_raw_fd(), + name.as_ptr(), + libc::O_RDONLY | libc::O_NOFOLLOW | libc::O_NONBLOCK | libc::O_CLOEXEC, + ) + }; + if fd < 0 { + return Err(format!( + "unreadable ({})", + std::io::Error::last_os_error().kind() + )); + } + // SAFETY: `fd` was just returned by `openat` and is owned here. + let file = unsafe { std::fs::File::from_raw_fd(fd) }; + if !file + .metadata() + .map_err(|e| format!("unreadable ({})", e.kind()))? + .is_file() + { + return Err("not a regular file".into()); + } + Ok(file) +} + +/// Windows has no `openat`; `FILE_FLAG_OPEN_REPARSE_POINT` opens a +/// swapped-in symlink or junction *as* the reparse point, which then fails +/// the regular-file check below rather than redirecting the read. Creating +/// a symlink there needs a privilege ordinary accounts lack, so the +/// remaining ancestor race is not reachable without one. +#[cfg(not(unix))] +fn open_beneath(root: &Path, path: &Path) -> Result { + const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000; + if !path.starts_with(root) { + return Err("outside the workspace".into()); + } let mut options = std::fs::OpenOptions::new(); options.read(true); - #[cfg(unix)] + #[cfg(windows)] { - use std::os::unix::fs::OpenOptionsExt; - options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK); + use std::os::windows::fs::OpenOptionsExt; + options.custom_flags(FILE_FLAG_OPEN_REPARSE_POINT); } let file = options .open(path) @@ -402,24 +496,92 @@ fn open_regular_file(path: &Path) -> Result { Ok(file) } -/// Read an image, refusing one larger than `cap` rather than truncating it. -/// -/// `take(cap + 1)` so exceeding the cap is *observed*: a half-read image -/// would otherwise be attached as a corrupt payload. The cap is what keeps -/// an oversized file from being held and base64-encoded on the UI thread. -fn read_image_capped(path: &Path, cap: usize) -> Result, String> { - let file = open_regular_file(path)?; - let mut data = Vec::new(); - file.take(cap as u64 + 1) - .read_to_end(&mut data) - .map_err(|e| format!("unreadable ({})", e.kind()))?; - if data.len() > cap { +/// Stage a validated image: open it now, and measure it from the +/// descriptor so an oversized file is refused before anything is read. +fn stage_image(cwd: &Path, path: &Path) -> Result<(StagedImage, usize), String> { + let root = cwd.canonicalize().unwrap_or_else(|_| cwd.to_path_buf()); + let file = open_beneath(&root, path)?; + let len = file + .metadata() + .map_err(|e| format!("unreadable ({})", e.kind()))? + .len(); + let len = usize::try_from(len).map_err(|_| "image too large".to_string())?; + if len > MAX_IMAGE_BYTES { return Err(format!( - "image too large (over {} MiB)", - cap / (1024 * 1024) + "image too large ({:.1} MiB, max {} MiB)", + len as f64 / (1024.0 * 1024.0), + MAX_IMAGE_BYTES / (1024 * 1024) )); } - Ok(data) + Ok(( + StagedImage { + path: path.to_path_buf(), + file: std::sync::Arc::new(file), + }, + len, + )) +} + +/// Read and encode staged images from the descriptors already held. +/// +/// Blocking work — call it off the UI thread. No path is resolved here: +/// the files were opened when their mentions were validated, so this reads +/// the bytes of those files and nothing else, however the workspace has +/// been rearranged since. +pub fn encode_staged_images( + images: Vec, +) -> (Vec, Vec) { + use std::io::{Seek, SeekFrom}; + + let mut blocks = Vec::new(); + let mut notes = Vec::new(); + let mut total = 0usize; + for image in images.into_iter().take(MAX_IMAGES) { + let name = image.path.display(); + let mut handle: &std::fs::File = &image.file; + // Rewind: a turn that failed to spawn is retried with the same + // descriptors, and a spent offset would re-encode an empty file. + if let Err(e) = handle.seek(SeekFrom::Start(0)) { + notes.push(format!( + "could not attach {name}: unreadable ({})", + e.kind() + )); + continue; + } + let mut data = Vec::new(); + // Capped again: the file can still grow after it was measured, and + // this is the read that would actually hold the bytes. + if let Err(e) = handle + .take(MAX_IMAGE_BYTES as u64 + 1) + .read_to_end(&mut data) + { + notes.push(format!( + "could not attach {name}: unreadable ({})", + e.kind() + )); + continue; + } + if data.len() > MAX_IMAGE_BYTES { + notes.push(format!( + "could not attach {name}: image too large (over {} MiB)", + MAX_IMAGE_BYTES / (1024 * 1024) + )); + continue; + } + if total + data.len() > MAX_TOTAL_IMAGE_BYTES { + notes.push(format!( + "could not attach {name}: {} MiB total image limit reached", + MAX_TOTAL_IMAGE_BYTES / (1024 * 1024) + )); + continue; + } + total += data.len(); + blocks.push(agent_code_lib::llm::message::image_block_from_bytes( + &image.path, + &data, + )); + } + (blocks, notes) } /// True when `path` resolves inside `cwd`. Both sides are canonicalized. @@ -811,8 +973,10 @@ mod tests { "image was not attached: {:?}", out.notes ); - let ContentBlock::Image { media_type, data } = &out.images[0] else { - panic!("expected an image block"); + assert!(out.images[0].path.ends_with("shot.png")); + let (blocks, notes) = encode_staged_images(out.images); + let ContentBlock::Image { media_type, data } = &blocks[0] else { + panic!("expected an image block: {notes:?}"); }; assert_eq!(media_type, "image/png"); assert!(!data.is_empty(), "image was attached with an empty payload"); @@ -903,8 +1067,9 @@ mod tests { fs::write(dir.path().join("shot.png"), [0x89, b'P', b'N', b'G']).unwrap(); let out = expand_mentions("@shot.png", dir.path()).expect("expanded"); assert_eq!(out.images.len(), 1, "{:?}", out.notes); - let ContentBlock::Image { media_type, data } = &out.images[0] else { - panic!("expected an image block"); + let (blocks, notes) = encode_staged_images(out.images); + let ContentBlock::Image { media_type, data } = &blocks[0] else { + panic!("expected an image block: {notes:?}"); }; assert_eq!(media_type, "image/png"); assert_eq!(data, "iVBORw=="); @@ -964,7 +1129,7 @@ mod tests { // SAFETY: `c` is a valid NUL-terminated path for the call. assert_eq!(unsafe { libc::mkfifo(c.as_ptr(), 0o600) }, 0, "mkfifo"); assert_eq!( - open_regular_file(&path).unwrap_err(), + open_beneath(dir.path(), &path).unwrap_err(), "not a regular file", "a FIFO must never be opened for attachment" ); @@ -982,18 +1147,63 @@ mod tests { let link = dir.path().join("link.png"); std::os::unix::fs::symlink(&target, &link).unwrap(); assert!( - open_regular_file(&link).is_err(), + open_beneath(dir.path(), &link).is_err(), "open followed a symlink out of the workspace" ); } + /// The attack the descriptors exist for: swap an *ancestor directory* + /// of a staged image for a symlink pointing outside the workspace + /// before the turn starts. `O_NOFOLLOW` on the final component alone + /// would not have caught this; the read must still see the file that + /// was validated, not the one the name now reaches. + #[cfg(unix)] + #[test] + fn a_staged_image_survives_its_directory_being_swapped() { + let outside = tempfile::tempdir().expect("tempdir"); + fs::write(outside.path().join("shot.png"), b"exfiltrate me").unwrap(); + + let dir = fixture(); + fs::create_dir(dir.path().join("sub")).unwrap(); + fs::write(dir.path().join("sub/shot.png"), [0x89, b'P', b'N', b'G']).unwrap(); + let out = expand_mentions("@sub/shot.png", dir.path()).expect("expanded"); + assert_eq!(out.images.len(), 1, "{:?}", out.notes); + + // The turn has not started yet; the whole directory is replaced. + fs::remove_dir_all(dir.path().join("sub")).unwrap(); + std::os::unix::fs::symlink(outside.path(), dir.path().join("sub")).unwrap(); + + let (blocks, notes) = encode_staged_images(out.images); + let ContentBlock::Image { data, .. } = &blocks[0] else { + panic!("expected an image block: {notes:?}"); + }; + assert_eq!( + data, "iVBORw==", + "read the swapped-in file, not the staged one" + ); + } + + /// And the open itself refuses to walk through a symlinked ancestor. + #[cfg(unix)] + #[test] + fn opening_refuses_a_symlinked_ancestor_directory() { + let outside = tempfile::tempdir().expect("tempdir"); + fs::write(outside.path().join("shot.png"), b"exfiltrate me").unwrap(); + let dir = fixture(); + std::os::unix::fs::symlink(outside.path(), dir.path().join("sub")).unwrap(); + assert!( + open_beneath(dir.path(), &dir.path().join("sub/shot.png")).is_err(), + "open walked through a symlinked ancestor" + ); + } + /// Fail-closed: an image that cannot be read is refused, not attached /// and hoped for. #[test] fn an_unreadable_image_is_refused() { let dir = fixture(); let missing = dir.path().join("gone.png"); - assert!(read_image_capped(&missing, MAX_IMAGE_BYTES).is_err()); + assert!(stage_image(dir.path(), &missing).is_err()); } /// A non-image binary still reports why it was skipped, rather than diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 5c444859..935932ab 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -570,17 +570,36 @@ pub(super) async fn event_loop( if turn.is_none() && let Some(prompt) = app.pending_submit.take() { - // Hand this turn's images to the engine. They were read and - // encoded during mention expansion, while the path was still - // the one that had just been validated — nothing is opened - // here, so there is no second chance to point them elsewhere. + // Encode this turn's images. The descriptors were opened when + // their mentions were validated, so nothing is resolved here — + // and the read runs on the blocking pool, because a workspace + // on a slow mount would otherwise stall the event loop (and + // with it redraws and cancellation) for the whole read. let images = std::mem::take(&mut app.pending_images); + let restore = images.clone(); + let blocks = if images.is_empty() { + Vec::new() + } else { + let (blocks, notes) = match tokio::task::spawn_blocking(move || { + super::mentions::encode_staged_images(images) + }) + .await + { + Ok(out) => out, + Err(e) => (Vec::new(), vec![format!("could not attach images: {e}")]), + }; + for note in notes { + app.transcript + .push(super::app::TranscriptItem::System(note)); + } + blocks + }; // Set unconditionally, awaiting the lock rather than skipping on // contention: an empty set clears anything a previous attempt // staged, so no turn can inherit another turn's attachment. { let engine = session.engine(); - engine.lock().await.set_pending_attachments(images.clone()); + engine.lock().await.set_pending_attachments(blocks); } let sink = ChannelSink::new(eng_tx.clone()); match session.spawn_turn(prompt.clone(), sink).await { @@ -593,7 +612,7 @@ pub(super) async fn event_loop( // and its attachments back so the next idle loop retries // them together. app.pending_submit = Some(prompt); - app.pending_images = images; + app.pending_images = restore; app.status_message = format!("turn busy: {e}"); app.dirty = true; } From 7dac5d1c9818a905b5bbc43af9c1a6550162390a Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:45:40 -0700 Subject: [PATCH 08/19] fix(tui): anchor the image root and reject reparse-point ancestors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The descriptor walk was only as trustworthy as the directory it started from, and that one was opened by name like any other: replacing the workspace directory itself through a writable parent pointed every relative open below it at the wrong tree. Open the canonical root with `O_DIRECTORY | O_NOFOLLOW` so a swapped root is refused rather than followed; a session started in a symlinked directory is unaffected, because that symlink is resolved before the root is opened. On Windows the reparse-point flag only ever covered the final component, and a junction — unlike a symlink — needs no privilege to create, so an attacker could redirect an ancestor and have the read escape. Reject any ancestor between the root and the file that has become a reparse point. --- crates/cli/src/ui/modern/mentions.rs | 91 +++++++++++++++++++++++++--- 1 file changed, 81 insertions(+), 10 deletions(-) diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index f233d9a0..df697e3e 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -398,9 +398,20 @@ fn open_beneath(root: &Path, path: &Path) -> Result { .strip_prefix(root) .map_err(|_| "outside the workspace".to_string())?; - // The workspace root is the trust anchor: it is the directory the - // session was started in, not something a mention chose. - let mut dir = std::fs::File::open(root).map_err(|e| format!("unreadable ({})", e.kind()))?; + // The workspace root is the trust anchor, so it is opened with the same + // suspicion as every component below it: `root` is canonical, so its + // final component is a real directory unless someone replaced it since + // — which is exactly what `O_NOFOLLOW` refuses. A session started in a + // symlinked directory still works, because canonicalization resolved + // that symlink before this point. + let mut dir = { + use std::os::unix::fs::OpenOptionsExt; + std::fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_DIRECTORY | libc::O_NOFOLLOW | libc::O_CLOEXEC) + .open(root) + .map_err(|e| format!("unreadable ({})", e.kind()))? + }; let components: Vec<_> = rel.components().collect(); let Some((last, parents)) = components.split_last() else { @@ -465,17 +476,40 @@ fn open_beneath(root: &Path, path: &Path) -> Result { Ok(file) } -/// Windows has no `openat`; `FILE_FLAG_OPEN_REPARSE_POINT` opens a -/// swapped-in symlink or junction *as* the reparse point, which then fails -/// the regular-file check below rather than redirecting the read. Creating -/// a symlink there needs a privilege ordinary accounts lack, so the -/// remaining ancestor race is not reachable without one. +/// Windows has no `openat`, so the ancestors are checked explicitly. +/// +/// `FILE_FLAG_OPEN_REPARSE_POINT` covers the final component: a symlink or +/// junction swapped in for the file is opened *as* the reparse point and +/// fails the regular-file check below rather than redirecting the read. +/// That flag does nothing for the directories above it, and creating a +/// junction needs no special privilege, so every ancestor between the +/// workspace root and the file is rejected outright if it has become a +/// reparse point. #[cfg(not(unix))] fn open_beneath(root: &Path, path: &Path) -> Result { const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000; - if !path.starts_with(root) { - return Err("outside the workspace".into()); + let rel = path + .strip_prefix(root) + .map_err(|_| "outside the workspace".to_string())?; + + let mut ancestor = root.to_path_buf(); + for component in rel.components() { + if !matches!(component, std::path::Component::Normal(_)) { + return Err("not a usable path".into()); + } + ancestor.push(component); + if ancestor == path { + break; + } + if std::fs::symlink_metadata(&ancestor) + .map_err(|e| format!("unreadable ({})", e.kind()))? + .file_type() + .is_symlink() + { + return Err("not a regular file".into()); + } } + let mut options = std::fs::OpenOptions::new(); options.read(true); #[cfg(windows)] @@ -1183,6 +1217,43 @@ mod tests { ); } + /// The root is the trust anchor, so it gets the same treatment: if the + /// workspace directory itself has been replaced by a symlink, every + /// descriptor below it would be relative to the wrong tree. + #[cfg(unix)] + #[test] + fn opening_refuses_a_symlinked_workspace_root() { + let outside = tempfile::tempdir().expect("tempdir"); + fs::write(outside.path().join("shot.png"), b"exfiltrate me").unwrap(); + let holder = tempfile::tempdir().expect("tempdir"); + let root = holder.path().join("workspace"); + std::os::unix::fs::symlink(outside.path(), &root).unwrap(); + assert!( + open_beneath(&root, &root.join("shot.png")).is_err(), + "opened through a symlinked workspace root" + ); + } + + /// …but a session legitimately started in a symlinked directory still + /// works, because the root is canonicalized before it is opened. + #[cfg(unix)] + #[test] + fn a_symlinked_workspace_still_attaches_images() { + let real = tempfile::tempdir().expect("tempdir"); + fs::write(real.path().join("shot.png"), [0x89, b'P', b'N', b'G']).unwrap(); + let holder = tempfile::tempdir().expect("tempdir"); + let link = holder.path().join("workspace"); + std::os::unix::fs::symlink(real.path(), &link).unwrap(); + + let out = expand_mentions("@shot.png", &link).expect("expanded"); + assert_eq!( + out.images.len(), + 1, + "a symlinked workspace stopped working: {:?}", + out.notes + ); + } + /// And the open itself refuses to walk through a symlinked ancestor. #[cfg(unix)] #[test] From c3ecc010c147dd06a460efde82f473f7ee579d16 Mon Sep 17 00:00:00 2001 From: emal Date: Sun, 26 Jul 2026 23:54:09 -0700 Subject: [PATCH 09/19] fix(tui): reject every Windows reparse-point ancestor by attribute The ancestor check asked `FileType::is_symlink`, which keys off the name-surrogate bit in the reparse tag rather than on whether traversing the entry leaves the workspace. Test `FILE_ATTRIBUTE_REPARSE_POINT` instead: it refuses junctions, symlinks and any tag the filesystem grows later, which is the only form of the check that stays correct as tags are added. --- crates/cli/src/ui/modern/mentions.rs | 30 +++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index df697e3e..a9526cfb 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -476,6 +476,28 @@ fn open_beneath(root: &Path, path: &Path) -> Result { Ok(file) } +/// True when this entry is a reparse point of any kind. +/// +/// The attribute, not `FileType::is_symlink`: that predicate keys off the +/// name-surrogate bit in the reparse tag, which is a property of how the +/// tag is meant to be interpreted rather than of whether traversing the +/// entry leaves the workspace. Testing the attribute refuses every reparse +/// tag — junction, symlink, and whatever else the filesystem grows — +/// which is the only answer that stays correct as tags are added. +#[cfg(not(unix))] +fn is_reparse_point(meta: &std::fs::Metadata) -> bool { + #[cfg(windows)] + { + use std::os::windows::fs::MetadataExt; + const FILE_ATTRIBUTE_REPARSE_POINT: u32 = 0x0000_0400; + return meta.file_attributes() & FILE_ATTRIBUTE_REPARSE_POINT != 0; + } + #[cfg(not(windows))] + { + meta.file_type().is_symlink() + } +} + /// Windows has no `openat`, so the ancestors are checked explicitly. /// /// `FILE_FLAG_OPEN_REPARSE_POINT` covers the final component: a symlink or @@ -501,11 +523,9 @@ fn open_beneath(root: &Path, path: &Path) -> Result { if ancestor == path { break; } - if std::fs::symlink_metadata(&ancestor) - .map_err(|e| format!("unreadable ({})", e.kind()))? - .file_type() - .is_symlink() - { + let meta = std::fs::symlink_metadata(&ancestor) + .map_err(|e| format!("unreadable ({})", e.kind()))?; + if is_reparse_point(&meta) { return Err("not a regular file".into()); } } From 269b6c60f2b97bc3d3ca0b3ad63ac7f2fc251d3b Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 00:00:23 -0700 Subject: [PATCH 10/19] fix(tui): judge the opened image by its reparse attribute on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FILE_FLAG_OPEN_REPARSE_POINT` opens a reparse point rather than refusing it, and `is_file` only excludes the tags `is_symlink` recognizes — so a placeholder or any other non-name-surrogate tag was accepted as an ordinary file. Test the attribute on the descriptor's own metadata before accepting it. Also records what the Windows path cannot do: its ancestor checks and its open are separate lookups, because Win32 offers no handle-relative open. --- crates/cli/src/ui/modern/mentions.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/crates/cli/src/ui/modern/mentions.rs b/crates/cli/src/ui/modern/mentions.rs index a9526cfb..322bb89d 100644 --- a/crates/cli/src/ui/modern/mentions.rs +++ b/crates/cli/src/ui/modern/mentions.rs @@ -507,6 +507,16 @@ fn is_reparse_point(meta: &std::fs::Metadata) -> bool { /// junction needs no special privilege, so every ancestor between the /// workspace root and the file is rejected outright if it has become a /// reparse point. +/// +/// Unlike the Unix path this is not atomic: the ancestor checks and the +/// open are separate lookups, and Win32 has no handle-relative open to +/// close that gap — only `NtCreateFile` with a root directory handle can, +/// which is undocumented-adjacent FFI this crate does not otherwise carry. +/// What remains is a local attacker who can already write inside the +/// workspace and must win a race, and who could instead simply put the +/// bytes they want in an image file there and have it attached with no +/// race at all. The bound worth having — that a mention cannot *quietly* +/// reach outside the workspace — is what the checks above provide. #[cfg(not(unix))] fn open_beneath(root: &Path, path: &Path) -> Result { const FILE_FLAG_OPEN_REPARSE_POINT: u32 = 0x0020_0000; @@ -540,11 +550,14 @@ fn open_beneath(root: &Path, path: &Path) -> Result { let file = options .open(path) .map_err(|e| format!("unreadable ({})", e.kind()))?; - if !file + let meta = file .metadata() - .map_err(|e| format!("unreadable ({})", e.kind()))? - .is_file() - { + .map_err(|e| format!("unreadable ({})", e.kind()))?; + // The descriptor decides, and a reparse point is judged by its + // attribute: `is_file` only excludes the tags `is_symlink` recognizes, + // so a placeholder or any other non-name-surrogate tag would otherwise + // be accepted here as an ordinary file. + if is_reparse_point(&meta) || !meta.is_file() { return Err("not a regular file".into()); } Ok(file) From bb958fd00cf11a378a6762742da050363e2181a4 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 00:12:26 -0700 Subject: [PATCH 11/19] fix: detach image encoding from the loop and show attachments to hooks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Awaiting the encode inline parked the one loop the TUI has: the work was on the blocking pool, but redraws and Ctrl+C waited for it anyway, so a slow mount still froze the interface. Send the result back through a channel and pick it up in a select arm — the same shape the drill-in output reads already use — and re-arm the prompt with its blocks when it lands. A cancel during the read cannot stop it, so the result is dropped when it arrives instead of being sent with a turn nobody asked for. `UserPromptSubmit` is documented to see the full prompt so hooks can scan or audit what goes out, but attachments were invisible there: an image reached the provider without the configured instrumentation ever knowing. Both prompt hooks now receive a description of each attachment — type, media type, encoded size — which is what an audit needs, without putting megabytes of base64 through every hook invocation. --- crates/cli/src/ui/modern/app.rs | 25 ++++++++++ crates/cli/src/ui/modern/run.rs | 88 +++++++++++++++++++++++---------- crates/lib/src/llm/message.rs | 73 +++++++++++++++++++++++++++ crates/lib/src/query/mod.rs | 7 +++ 4 files changed, 167 insertions(+), 26 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 1f25421c..ca4b2d94 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -427,6 +427,9 @@ pub struct App { /// open descriptors from the moment their mention was validated, so /// starting the turn needs no second look at any path. pub pending_images: Vec, + /// The same images once read and encoded off the UI thread, waiting for + /// the turn they belong to to start. + pub pending_attachments: Vec, /// User keybindings. Construction installs the built-in defaults /// only; the run loop injects the registry loaded from /// `keybindings.json` at startup. Constructors must not read the @@ -602,6 +605,7 @@ impl App { command_palette: None, model_picker: None, pending_images: Vec::new(), + pending_attachments: Vec::new(), keybindings: std::sync::Arc::new( crate::ui::keybindings::KeybindingRegistry::defaults(), ), @@ -1676,7 +1680,10 @@ impl App { // cancelling), and only the mention branch assigns `pending_images` // — so clear here, or the replacement turn would carry the previous // prompt's image and disclose a file the user did not mean to send. + // Both stages are cleared: one holds descriptors, the other the + // bytes already read from them. self.pending_images.clear(); + self.pending_attachments.clear(); let mut mention_notes: Vec = Vec::new(); let (display, prompt) = match try_expand_skill_slash_full(&text, &self.cwd, self.disable_skill_shell) { @@ -4564,4 +4571,22 @@ mod tests { "stale image carried onto a command-produced turn" ); } + + /// Encoded bytes are staged separately from the descriptors, so a + /// replaced prompt has to drop both — otherwise an image that had + /// already been read would still ride along with the new prompt. + #[test] + fn replacing_a_prompt_drops_already_encoded_attachments() { + let (_dir, mut app) = app_in_workspace(); + app.pending_attachments = vec![agent_code_lib::llm::message::ContentBlock::Image { + media_type: "image/png".into(), + data: "iVBORw==".into(), + }]; + type_input(&mut app, "a different prompt"); + app.submit(); + assert!( + app.pending_attachments.is_empty(), + "encoded image carried onto the replacement prompt" + ); + } } diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 935932ab..22b633f4 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -333,6 +333,22 @@ pub(super) async fn event_loop( // back here, so a slow filesystem never blocks the event loop. let (task_out_tx, mut task_out_rx) = tokio::sync::mpsc::unbounded_channel::<(String, Result)>(); + // Image attachments are read and encoded the same way: detached, with + // the result landing in a select arm. Awaiting the work inline would + // park this loop — the only one there is — so a workspace on a slow + // mount would stop redraws and Ctrl+C until the read finished. + #[allow(clippy::type_complexity)] + let (img_tx, mut img_rx) = tokio::sync::mpsc::unbounded_channel::<( + String, + Vec, + Vec, + )>(); + // Set while an encode is in flight: the turn it belongs to must not + // start without it, and a second one must not be queued behind it. + let mut encoding = false; + // A cancel that lands mid-encode cannot stop the read, so the result + // is dropped when it arrives instead. + let mut discard_encoding = false; // Seed the pane once so tasks adopted from a previous process show // before the first turn arms the periodic poll. app.sync_background_tasks(manager_rows(&task_manager).await); @@ -566,40 +582,37 @@ pub(super) async fn event_loop( } } - // Start a pending turn if idle. + // Hand a prompt's images to the blocking pool. The descriptors were + // opened when their mentions were validated, so nothing is resolved + // here; the result comes back through `img_rx` and re-arms the + // prompt, which keeps this loop free to redraw and to take a Ctrl+C + // while a slow mount is being read. if turn.is_none() + && !encoding + && !app.pending_images.is_empty() && let Some(prompt) = app.pending_submit.take() { - // Encode this turn's images. The descriptors were opened when - // their mentions were validated, so nothing is resolved here — - // and the read runs on the blocking pool, because a workspace - // on a slow mount would otherwise stall the event loop (and - // with it redraws and cancellation) for the whole read. let images = std::mem::take(&mut app.pending_images); - let restore = images.clone(); - let blocks = if images.is_empty() { - Vec::new() - } else { - let (blocks, notes) = match tokio::task::spawn_blocking(move || { - super::mentions::encode_staged_images(images) - }) - .await - { - Ok(out) => out, - Err(e) => (Vec::new(), vec![format!("could not attach images: {e}")]), - }; - for note in notes { - app.transcript - .push(super::app::TranscriptItem::System(note)); - } - blocks - }; + let tx = img_tx.clone(); + encoding = true; + tokio::task::spawn_blocking(move || { + let (blocks, notes) = super::mentions::encode_staged_images(images); + let _ = tx.send((prompt, blocks, notes)); + }); + } + + // Start a pending turn if idle. + if turn.is_none() + && !encoding + && let Some(prompt) = app.pending_submit.take() + { + let blocks = std::mem::take(&mut app.pending_attachments); // Set unconditionally, awaiting the lock rather than skipping on // contention: an empty set clears anything a previous attempt // staged, so no turn can inherit another turn's attachment. { let engine = session.engine(); - engine.lock().await.set_pending_attachments(blocks); + engine.lock().await.set_pending_attachments(blocks.clone()); } let sink = ChannelSink::new(eng_tx.clone()); match session.spawn_turn(prompt.clone(), sink).await { @@ -612,7 +625,7 @@ pub(super) async fn event_loop( // and its attachments back so the next idle loop retries // them together. app.pending_submit = Some(prompt); - app.pending_images = restore; + app.pending_attachments = blocks; app.status_message = format!("turn busy: {e}"); app.dirty = true; } @@ -624,6 +637,11 @@ pub(super) async fn event_loop( if let Some(ref h) = turn { h.cancel(); } + // A read already handed to the blocking pool cannot be stopped, + // so its result is thrown away when it lands. + if encoding { + discard_encoding = true; + } app.cancel_requested = false; } @@ -843,6 +861,24 @@ pub(super) async fn event_loop( Some((id, out)) = task_out_rx.recv() => { app.show_task_output(&id, out); } + // Encoded image attachments coming back from the blocking pool. + // The prompt is re-armed with them so the turn starts on the + // next pass through the loop above. + Some((prompt, blocks, notes)) = img_rx.recv() => { + encoding = false; + if discard_encoding { + // Cancelled while the read was in flight: the bytes are + // dropped rather than sent with a turn nobody asked for. + discard_encoding = false; + } else { + for note in notes { + app.transcript.push(super::app::TranscriptItem::System(note)); + } + app.pending_attachments = blocks; + app.pending_submit = Some(prompt); + } + app.dirty = true; + } // Background-task rows (`&` shell jobs, workflows, monitors). // Gated on work that can still change: polling while any // rows exist at all would tick forever once a subagent row diff --git a/crates/lib/src/llm/message.rs b/crates/lib/src/llm/message.rs index 6ce62e3a..28b7c4ee 100644 --- a/crates/lib/src/llm/message.rs +++ b/crates/lib/src/llm/message.rs @@ -187,6 +187,18 @@ impl ContentBlock { } } + /// The wire `type` tag of this block, for logs and hook payloads. + pub fn kind_name(&self) -> &'static str { + match self { + ContentBlock::Text { .. } => "text", + ContentBlock::ToolUse { .. } => "tool_use", + ContentBlock::ToolResult { .. } => "tool_result", + ContentBlock::Thinking { .. } => "thinking", + ContentBlock::Image { .. } => "image", + ContentBlock::Document { .. } => "document", + } + } + /// Extract tool use info, if this is a tool_use block. pub fn as_tool_use(&self) -> Option<(&str, &str, &serde_json::Value)> { match self { @@ -273,6 +285,32 @@ pub fn user_message(text: impl Into) -> Message { }) } +/// Describe attachment blocks for hook payloads and logs. +/// +/// Metadata, not payloads: a hook needs to know that an image of a given +/// type and size is going out — enough to audit or refuse it — and putting +/// megabytes of base64 into every hook invocation would serve nothing. +pub fn describe_attachments(blocks: &[ContentBlock]) -> Vec { + blocks + .iter() + .map(|block| match block { + ContentBlock::Image { media_type, data } => serde_json::json!({ + "type": "image", + "media_type": media_type, + "encoded_bytes": data.len(), + }), + ContentBlock::Document { + media_type, data, .. + } => serde_json::json!({ + "type": "document", + "media_type": media_type, + "encoded_bytes": data.len(), + }), + other => serde_json::json!({ "type": other.kind_name() }), + }) + .collect() +} + /// Media type for an image path, inferred from its extension. /// /// Case-insensitive: callers that decide *whether* a path is an image @@ -579,6 +617,41 @@ mod tests { assert_eq!(data, "iVBORw==", "image payload was not encoded"); } + /// A `UserPromptSubmit` hook is documented to see the whole prompt so + /// it can scan or audit it; an attached image that appeared nowhere in + /// the payload went out unexamined. + #[test] + fn attachments_are_described_for_hooks() { + let blocks = vec![ + ContentBlock::Image { + media_type: "image/png".into(), + data: "iVBORw==".into(), + }, + ContentBlock::Document { + media_type: "application/pdf".into(), + data: "JVBER".into(), + title: None, + }, + ContentBlock::Text { + text: "hello".into(), + }, + ]; + let described = describe_attachments(&blocks); + assert_eq!(described[0]["type"], "image"); + assert_eq!(described[0]["media_type"], "image/png"); + assert_eq!(described[0]["encoded_bytes"], 8); + assert_eq!(described[1]["type"], "document"); + assert_eq!(described[1]["media_type"], "application/pdf"); + assert_eq!(described[2]["type"], "text"); + // Metadata only: the bytes themselves would bloat every hook call. + assert!(described[0].get("data").is_none()); + } + + #[test] + fn describing_no_attachments_is_empty() { + assert!(describe_attachments(&[]).is_empty()); + } + /// RFC 4648 §10 test vectors — the padded remainders are exactly where /// the previous encoder silently produced nothing. #[test] diff --git a/crates/lib/src/query/mod.rs b/crates/lib/src/query/mod.rs index 975eca03..00fa755f 100644 --- a/crates/lib/src/query/mod.rs +++ b/crates/lib/src/query/mod.rs @@ -753,6 +753,11 @@ impl QueryEngine { // belongs to exactly one turn, and leaking it into the next one // would re-send an image the user already shared. let attachments = std::mem::take(&mut self.pending_attachments); + // Described for the hooks below before the blocks are moved into + // the message. A `UserPromptSubmit` hook exists to see everything + // the turn sends; an image that only showed up as silence there + // would slip past exactly the audit it was configured for. + let attachment_info = crate::llm::message::describe_attachments(&attachments); let user_msg = if attachments.is_empty() { user_message(user_input) } else { @@ -771,6 +776,7 @@ impl QueryEngine { None, &serde_json::json!({ "user_input": user_input, + "attachments": attachment_info, "turn": self.state.turn_count + 1, }), Some(&self.cancel), @@ -791,6 +797,7 @@ impl QueryEngine { &serde_json::json!({ "turn": self.state.turn_count + 1, "user_input_preview": user_input.chars().take(200).collect::(), + "attachments": attachment_info, }), Some(&self.cancel), ) From 0076c090899b4d1f8fe6fa8ddc05eccc171fcc56 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 00:22:52 -0700 Subject: [PATCH 12/19] fix(tui): leave the streaming phase when a staged read is cancelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cancelling while attachments were being read took the prompt and spawned no turn, so nothing was left to end the phase: the UI stayed in Streaming for good, queueing every later prompt behind a turn that never existed and aiming each further Ctrl+C at nothing. Reset the turn state when the discarded result lands — deliberately not `mark_turn_idle`, which announces a finished turn, because this one never started. Also records why the prompt hooks only observe attachments: a non-zero exit there does not veto the turn the way PreToolUse does, and changing that would block prompts for anyone whose hook exits non-zero today. --- crates/cli/src/ui/modern/app.rs | 54 +++++++++++++++++++++++++++++++++ crates/cli/src/ui/modern/run.rs | 5 +++ crates/lib/src/query/mod.rs | 6 ++++ 3 files changed, 65 insertions(+) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index ca4b2d94..db3956d6 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -2584,6 +2584,28 @@ impl App { self.dirty = true; } + /// Give up on a prompt whose attachments were still being read when the + /// user cancelled. + /// + /// No turn was ever spawned for it, so nothing else will leave the + /// streaming phase, and every later prompt would queue behind a turn + /// that does not exist. Deliberately not `mark_turn_idle`: that + /// announces a *finished* turn, and this one never started. + pub fn abandon_staged_attachments(&mut self) { + self.pending_images.clear(); + self.pending_attachments.clear(); + self.turn_live = false; + self.turn_started_at = None; + self.phase = if self.modals.is_empty() { + Phase::Idle + } else { + Phase::Permission + }; + self.cancel_requested = false; + self.status_message = "cancelled".into(); + self.dirty = true; + } + pub fn tick(&mut self) { self.tick = self.tick.wrapping_add(1); if let Some((_, ref mut left)) = self.toast { @@ -4572,6 +4594,38 @@ mod tests { ); } + /// Cancelling while the attachments are being read leaves no turn to + /// finish, so the phase has to be reset here or every later prompt + /// queues behind one that never existed. + #[test] + fn cancelling_during_attachment_encoding_returns_to_idle() { + let (_dir, mut app) = app_in_workspace(); + write_png(&app, "shot.png"); + type_input(&mut app, "look at @shot.png"); + app.submit(); + assert_eq!(app.phase, Phase::Streaming, "precondition"); + + // The run loop takes the prompt to start encoding; the user cancels + // before the read comes back. + let _ = app.pending_submit.take(); + app.abandon_staged_attachments(); + + assert_eq!( + app.phase, + Phase::Idle, + "stuck in streaming after a cancelled encode" + ); + assert!(app.pending_images.is_empty()); + assert!(app.pending_attachments.is_empty()); + + // And the next prompt starts a turn instead of queueing behind the + // turn that never was. + type_input(&mut app, "next"); + app.submit(); + assert_eq!(app.pending_submit.as_deref(), Some("next")); + assert!(app.queue.is_empty(), "prompt was queued, not sent"); + } + /// Encoded bytes are staged separately from the descriptors, so a /// replaced prompt has to drop both — otherwise an image that had /// already been read would still ride along with the new prompt. diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 22b633f4..adc6f1b7 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -869,7 +869,12 @@ pub(super) async fn event_loop( if discard_encoding { // Cancelled while the read was in flight: the bytes are // dropped rather than sent with a turn nobody asked for. + // The prompt was taken and no turn was ever spawned, so + // nothing else will end the streaming phase — leaving it + // set would queue every later prompt behind a turn that + // does not exist. discard_encoding = false; + app.abandon_staged_attachments(); } else { for note in notes { app.transcript.push(super::app::TranscriptItem::System(note)); diff --git a/crates/lib/src/query/mod.rs b/crates/lib/src/query/mod.rs index 00fa755f..1bf7ba85 100644 --- a/crates/lib/src/query/mod.rs +++ b/crates/lib/src/query/mod.rs @@ -769,6 +769,12 @@ impl QueryEngine { // prompt is in history and before any PreTurn / LLM work. Hooks // at this event see the FULL prompt (no truncation) so they can // do content scanning, redaction logging, or compliance audits. + // + // Observation only: unlike PreToolUse, a non-zero exit here does + // not veto the turn. Wiring that up would start blocking prompts + // for every deployment whose prompt hook happens to exit non-zero + // today — a `grep` that finds nothing is enough — so it is a + // deliberate change for its own PR, not a side effect of this one. let _user_prompt_submit_results = self .hooks .run_hooks( From cd9a283c3eb3090892d571e04cd58c9855512d47 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 00:40:50 -0700 Subject: [PATCH 13/19] fix(compaction): bound the image bytes history keeps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An image is charged a flat vision estimate, so no token threshold ever notices it — yet the base64 payload stays in history for the rest of the session and is reserialized into every later request. A few screenshots were enough to push requests past what a provider accepts, with the memory to match, and none of the three compaction strategies could see the weight to act on it. Bound the bytes directly: keep the most recent 12 MiB of image payloads and replace older ones with text naming what was dropped, so the model still knows an image was there and the message keeps a valid shape. Runs once per turn, after the new turn's own images are counted as the most recent. --- crates/lib/src/query/mod.rs | 13 +++ crates/lib/src/services/compact.rs | 156 +++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) diff --git a/crates/lib/src/query/mod.rs b/crates/lib/src/query/mod.rs index 1bf7ba85..078e29ce 100644 --- a/crates/lib/src/query/mod.rs +++ b/crates/lib/src/query/mod.rs @@ -764,6 +764,19 @@ impl QueryEngine { crate::llm::message::user_message_with_attachments(user_input, attachments) }; self.state.push_message(user_msg); + // Images are the one block a token threshold cannot see: each is + // charged a flat vision estimate, so compaction never fires on the + // megabytes they actually add to every later request. Bound what + // history retains here instead, once the new turn's own images are + // in and counted as the most recent. + let dropped = + compact::evict_old_images(&mut self.state.messages, compact::MAX_RETAINED_IMAGE_BYTES); + if dropped > 0 { + tracing::debug!( + dropped, + "evicted image payloads beyond the retention budget" + ); + } // UserPromptSubmit fires once per user turn, as soon as the // prompt is in history and before any PreTurn / LLM work. Hooks diff --git a/crates/lib/src/services/compact.rs b/crates/lib/src/services/compact.rs index bf8e6262..9142ff27 100644 --- a/crates/lib/src/services/compact.rs +++ b/crates/lib/src/services/compact.rs @@ -308,6 +308,62 @@ pub fn estimate_compactable_tokens(messages: &[Message], keep_recent: usize) -> freed } +/// Encoded image bytes kept in history, newest first. +/// +/// Images are the one block whose cost is measured in megabytes rather +/// than tokens: a screenshot is charged a flat vision estimate, so no +/// token threshold notices it, yet every later request reserializes and +/// resends the whole payload. A handful of screenshots in a long session +/// is enough to push requests past what a provider will accept. 12 MiB +/// keeps a few recent images available to the model while bounding what +/// the session can carry. +pub const MAX_RETAINED_IMAGE_BYTES: usize = 12 * 1024 * 1024; + +/// Drop image payloads once history holds more than +/// [`MAX_RETAINED_IMAGE_BYTES`] of them, oldest first. +/// +/// The block is replaced by text naming what was dropped, so the model +/// still knows an image was there and the conversation stays valid; +/// only the bytes go. Returns the number of images dropped. +pub fn evict_old_images(messages: &mut [Message], budget_bytes: usize) -> usize { + // Newest first: recent images are the ones still being talked about. + let mut sites: Vec<(usize, usize, usize, String)> = Vec::new(); + for (msg_idx, msg) in messages.iter().enumerate() { + let content = match msg { + Message::User(u) => &u.content, + Message::Assistant(a) => &a.content, + Message::System(_) => continue, + }; + for (block_idx, block) in content.iter().enumerate() { + if let ContentBlock::Image { media_type, data } = block { + sites.push((msg_idx, block_idx, data.len(), media_type.clone())); + } + } + } + + let mut kept = 0usize; + let mut evicted = 0usize; + for (msg_idx, block_idx, len, media_type) in sites.into_iter().rev() { + if kept + len <= budget_bytes { + kept += len; + continue; + } + let placeholder = ContentBlock::Text { + text: format!( + "[image dropped from context — {media_type}, {:.1} MB]", + len as f64 / (1000.0 * 1000.0) + ), + }; + match &mut messages[msg_idx] { + Message::User(u) => u.content[block_idx] = placeholder, + Message::Assistant(a) => a.content[block_idx] = placeholder, + Message::System(_) => continue, + } + evicted += 1; + } + evicted +} + /// Perform microcompact: clear stale tool results to free tokens. /// /// Replaces the content of old tool_result blocks with a placeholder, @@ -687,6 +743,106 @@ mod tests { }) } + fn user_with_image(bytes: usize) -> Message { + crate::llm::message::user_message_with_attachments( + "look at this", + vec![ContentBlock::Image { + media_type: "image/png".into(), + data: "A".repeat(bytes), + }], + ) + } + + fn image_payload_bytes(messages: &[Message]) -> usize { + messages + .iter() + .filter_map(|m| match m { + Message::User(u) => Some(&u.content), + Message::Assistant(a) => Some(&a.content), + Message::System(_) => None, + }) + .flatten() + .filter_map(|b| match b { + ContentBlock::Image { data, .. } => Some(data.len()), + _ => None, + }) + .sum() + } + + /// Images are charged a flat vision estimate, so no token threshold + /// ever notices the megabytes they add to every later request. The + /// bytes have to be bounded on their own. + #[test] + fn old_image_payloads_are_evicted_beyond_the_budget() { + let budget = 1000; + let mut messages = vec![ + user_with_image(600), + assistant_text("first"), + user_with_image(600), + assistant_text("second"), + user_with_image(600), + ]; + let evicted = evict_old_images(&mut messages, budget); + assert_eq!(evicted, 2, "older images were kept"); + assert!( + image_payload_bytes(&messages) <= budget, + "retained {} bytes over a {budget} budget", + image_payload_bytes(&messages) + ); + // The newest image survives: it is the one still being discussed. + assert!( + matches!( + messages[4].clone(), + Message::User(u) if u.content.iter().any(|b| matches!(b, ContentBlock::Image { .. })) + ), + "the newest image was dropped" + ); + } + + /// The block is replaced, not removed: the model still learns an image + /// was there, and the message keeps a valid shape. + #[test] + fn an_evicted_image_leaves_a_note_in_its_place() { + let mut messages = vec![user_with_image(500), user_with_image(500)]; + assert_eq!(evict_old_images(&mut messages, 600), 1); + let Message::User(u) = &messages[0] else { + panic!("expected a user message"); + }; + let text = u + .content + .iter() + .find_map(|b| b.as_text()) + .expect("no text block replaced the image"); + assert!(text.contains("image dropped from context"), "{text}"); + assert!(text.contains("image/png"), "{text}"); + } + + #[test] + fn images_within_the_budget_are_untouched() { + let mut messages = vec![user_with_image(100), user_with_image(100)]; + let before = image_payload_bytes(&messages); + assert_eq!(evict_old_images(&mut messages, 1000), 0); + assert_eq!(image_payload_bytes(&messages), before); + } + + /// Running twice must not keep rewriting history: once the payloads + /// are gone there is nothing left to evict. + #[test] + fn eviction_is_idempotent() { + let mut messages = vec![user_with_image(600), user_with_image(600)]; + assert_eq!(evict_old_images(&mut messages, 1000), 1); + assert_eq!(evict_old_images(&mut messages, 1000), 0); + } + + /// A single image larger than the whole budget is still dropped — + /// keeping it would blow the bound it exists to enforce. + #[test] + fn an_image_larger_than_the_budget_is_evicted() { + let mut messages = vec![user_with_image(2000)]; + assert_eq!(evict_old_images(&mut messages, 1000), 1); + assert_eq!(image_payload_bytes(&messages), 0); + } + fn assistant_tool_use(ids: &[&str]) -> Message { Message::Assistant(AssistantMessage { uuid: Uuid::new_v4(), From 597d0578d537d675a71bb32fb15ccd4bb560c715 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 00:47:40 -0700 Subject: [PATCH 14/19] fix(compaction): bound the number of retained images too Bytes are not the only limit a provider enforces: Anthropic accepts at most 100 images per request, and a session attaching small thumbnails each turn reaches that count while using a fraction of the byte budget. Evict on whichever bound is hit first, keeping the 32 most recent. --- crates/lib/src/query/mod.rs | 7 ++-- crates/lib/src/services/compact.rs | 53 +++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/crates/lib/src/query/mod.rs b/crates/lib/src/query/mod.rs index 078e29ce..5306a010 100644 --- a/crates/lib/src/query/mod.rs +++ b/crates/lib/src/query/mod.rs @@ -769,8 +769,11 @@ impl QueryEngine { // megabytes they actually add to every later request. Bound what // history retains here instead, once the new turn's own images are // in and counted as the most recent. - let dropped = - compact::evict_old_images(&mut self.state.messages, compact::MAX_RETAINED_IMAGE_BYTES); + let dropped = compact::evict_old_images( + &mut self.state.messages, + compact::MAX_RETAINED_IMAGE_BYTES, + compact::MAX_RETAINED_IMAGES, + ); if dropped > 0 { tracing::debug!( dropped, diff --git a/crates/lib/src/services/compact.rs b/crates/lib/src/services/compact.rs index 9142ff27..8bd6252c 100644 --- a/crates/lib/src/services/compact.rs +++ b/crates/lib/src/services/compact.rs @@ -319,13 +319,26 @@ pub fn estimate_compactable_tokens(messages: &[Message], keep_recent: usize) -> /// the session can carry. pub const MAX_RETAINED_IMAGE_BYTES: usize = 12 * 1024 * 1024; +/// Image blocks kept in history, newest first. +/// +/// Bytes are not the only limit: providers also cap how many images one +/// request may carry (Anthropic accepts 100), and a session that attaches +/// small thumbnails every turn reaches that count long before it reaches +/// any byte budget. 32 leaves generous headroom under the provider cap. +pub const MAX_RETAINED_IMAGES: usize = 32; + /// Drop image payloads once history holds more than -/// [`MAX_RETAINED_IMAGE_BYTES`] of them, oldest first. +/// [`MAX_RETAINED_IMAGE_BYTES`] or [`MAX_RETAINED_IMAGES`] of them, +/// oldest first. /// /// The block is replaced by text naming what was dropped, so the model /// still knows an image was there and the conversation stays valid; /// only the bytes go. Returns the number of images dropped. -pub fn evict_old_images(messages: &mut [Message], budget_bytes: usize) -> usize { +pub fn evict_old_images( + messages: &mut [Message], + budget_bytes: usize, + budget_count: usize, +) -> usize { // Newest first: recent images are the ones still being talked about. let mut sites: Vec<(usize, usize, usize, String)> = Vec::new(); for (msg_idx, msg) in messages.iter().enumerate() { @@ -343,9 +356,11 @@ pub fn evict_old_images(messages: &mut [Message], budget_bytes: usize) -> usize let mut kept = 0usize; let mut evicted = 0usize; + let mut kept_count = 0usize; for (msg_idx, block_idx, len, media_type) in sites.into_iter().rev() { - if kept + len <= budget_bytes { + if kept + len <= budget_bytes && kept_count < budget_count { kept += len; + kept_count += 1; continue; } let placeholder = ContentBlock::Text { @@ -782,7 +797,7 @@ mod tests { assistant_text("second"), user_with_image(600), ]; - let evicted = evict_old_images(&mut messages, budget); + let evicted = evict_old_images(&mut messages, budget, 100); assert_eq!(evicted, 2, "older images were kept"); assert!( image_payload_bytes(&messages) <= budget, @@ -804,7 +819,7 @@ mod tests { #[test] fn an_evicted_image_leaves_a_note_in_its_place() { let mut messages = vec![user_with_image(500), user_with_image(500)]; - assert_eq!(evict_old_images(&mut messages, 600), 1); + assert_eq!(evict_old_images(&mut messages, 600, 100), 1); let Message::User(u) = &messages[0] else { panic!("expected a user message"); }; @@ -821,7 +836,7 @@ mod tests { fn images_within_the_budget_are_untouched() { let mut messages = vec![user_with_image(100), user_with_image(100)]; let before = image_payload_bytes(&messages); - assert_eq!(evict_old_images(&mut messages, 1000), 0); + assert_eq!(evict_old_images(&mut messages, 1000, 100), 0); assert_eq!(image_payload_bytes(&messages), before); } @@ -830,8 +845,28 @@ mod tests { #[test] fn eviction_is_idempotent() { let mut messages = vec![user_with_image(600), user_with_image(600)]; - assert_eq!(evict_old_images(&mut messages, 1000), 1); - assert_eq!(evict_old_images(&mut messages, 1000), 0); + assert_eq!(evict_old_images(&mut messages, 1000, 100), 1); + assert_eq!(evict_old_images(&mut messages, 1000, 100), 0); + } + + /// Providers cap how many images a request may carry, so a session of + /// small thumbnails hits that limit long before any byte budget. + #[test] + fn the_retained_image_count_is_bounded_too() { + let mut messages: Vec = (0..10).map(|_| user_with_image(10)).collect(); + let evicted = evict_old_images(&mut messages, 1_000_000, 4); + assert_eq!(evicted, 6, "count budget was not enforced"); + let remaining = messages + .iter() + .filter(|m| match m { + Message::User(u) => u + .content + .iter() + .any(|b| matches!(b, ContentBlock::Image { .. })), + _ => false, + }) + .count(); + assert_eq!(remaining, 4, "more images retained than the count budget"); } /// A single image larger than the whole budget is still dropped — @@ -839,7 +874,7 @@ mod tests { #[test] fn an_image_larger_than_the_budget_is_evicted() { let mut messages = vec![user_with_image(2000)]; - assert_eq!(evict_old_images(&mut messages, 1000), 1); + assert_eq!(evict_old_images(&mut messages, 1000, 100), 1); assert_eq!(image_payload_bytes(&messages), 0); } From 9c066864986351807479a419a20b08c6bbd3f71b Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 01:03:44 -0700 Subject: [PATCH 15/19] fix(tui): do not overwrite a prompt sent while images are encoding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A slash command that produces a prompt is staged directly rather than queued, so it can land while an earlier prompt's images are still being read. Restoring that earlier prompt then overwrote it and its turn was lost without a trace — and simply keeping the newer prompt would have been worse, since the encoded blocks would have ridden along on it. The newer prompt keeps its turn and its own attachments; the superseded one returns to the head of the queue with a note that its images did not come with it. Neither prompt is lost and no image can land on a turn it was not meant for. --- crates/cli/src/ui/modern/app.rs | 80 +++++++++++++++++++++++++++++++++ crates/cli/src/ui/modern/run.rs | 3 +- 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index f4684412..381d3be0 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -2712,6 +2712,37 @@ impl App { self.dirty = true; } + /// Take a prompt's images once they have been read and encoded. + /// + /// Another prompt can arrive while the read is running — a slash + /// command that produces a prompt is staged directly rather than + /// queued — and that one has its own attachments staged with it. + /// Overwriting it would lose it silently; keeping it and attaching + /// *these* blocks would be worse still, putting one prompt's image on + /// another's turn. So the newer prompt keeps its turn, this one goes + /// back to the head of the queue, and the user is told its images did + /// not come with it. + pub fn accept_encoded_attachments( + &mut self, + prompt: String, + blocks: Vec, + ) { + if self.pending_submit.is_some() { + self.queue.push_front(prompt); + if !blocks.is_empty() { + self.transcript.push(TranscriptItem::System( + "another prompt was sent first — the queued prompt's images were not attached" + .into(), + )); + } + self.dirty = true; + return; + } + self.pending_attachments = blocks; + self.pending_submit = Some(prompt); + self.dirty = true; + } + /// Give up on a prompt whose attachments were still being read when the /// user cancelled. /// @@ -5193,6 +5224,55 @@ mod tests { assert!(rendered.contains(""), "not escaped: {rendered:?}"); } + fn png_block() -> agent_code_lib::llm::message::ContentBlock { + agent_code_lib::llm::message::ContentBlock::Image { + media_type: "image/png".into(), + data: "iVBORw==".into(), + } + } + + #[test] + fn encoded_attachments_arm_their_own_prompt() { + let (_dir, mut app) = app_in_workspace(); + app.accept_encoded_attachments("look at @shot.png".into(), vec![png_block()]); + assert_eq!(app.pending_submit.as_deref(), Some("look at @shot.png")); + assert_eq!(app.pending_attachments.len(), 1); + } + + /// A slash command that produces a prompt is staged directly, not + /// queued, so it can land while the images are still being read. It + /// must not be overwritten — and must not inherit the other prompt's + /// image either. + #[test] + fn a_prompt_sent_while_encoding_is_not_overwritten() { + let (_dir, mut app) = app_in_workspace(); + app.enqueue_turn_from_command("show me the diff".into()); + assert_eq!(app.pending_submit.as_deref(), Some("show me the diff")); + + app.accept_encoded_attachments("look at @shot.png".into(), vec![png_block()]); + + assert_eq!( + app.pending_submit.as_deref(), + Some("show me the diff"), + "the newer prompt was overwritten" + ); + assert!( + app.pending_attachments.is_empty(), + "the newer prompt inherited another prompt's image" + ); + assert_eq!( + app.queue.front().map(String::as_str), + Some("look at @shot.png"), + "the superseded prompt was dropped instead of queued" + ); + assert!( + app.transcript + .iter() + .any(|i| matches!(i, TranscriptItem::System(s) if s.contains("were not attached"))), + "no note that the images did not come along" + ); + } + /// A cancel abandons only the prompt that was being read for. If the /// user interjected with another image in the meantime, that prompt is /// still coming and must keep its own attachment — clearing it would diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 7ebf82ff..bf8caf56 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -921,8 +921,7 @@ pub(super) async fn event_loop( for note in notes { app.transcript.push(super::app::TranscriptItem::System(note)); } - app.pending_attachments = blocks; - app.pending_submit = Some(prompt); + app.accept_encoded_attachments(prompt, blocks); } app.dirty = true; } From 6ac0e96c18185d4e1c696c0a673645b2872a3514 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 01:11:15 -0700 Subject: [PATCH 16/19] fix(tui): hold a superseded prompt with its images, never as queue text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Queuing the superseded prompt put the *expanded* text back in the queue, which still opens with the original mention. Dispatching it ran `expand_mentions` again: the turn silently re-attached an image the user had been told was not attached, resolving the path a second time and sending whatever the file held by then — the delayed second lookup this branch exists to avoid. Hold the prompt and its already-encoded blocks together instead, and send them as one unit when the turn frees up. Nothing re-resolves, nothing re-reads, and the file that goes out is the one that was validated. A cancel drops it, so nothing follows on its own after the user stops. --- crates/cli/src/ui/modern/app.rs | 107 +++++++++++++++++++++++++++----- crates/cli/src/ui/modern/run.rs | 6 ++ 2 files changed, 96 insertions(+), 17 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 381d3be0..0273183b 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -446,6 +446,12 @@ pub struct App { /// The same images once read and encoded off the UI thread, waiting for /// the turn they belong to to start. pub pending_attachments: Vec, + /// A prompt whose images were already read when another prompt took + /// the turn, held with those images so it can be sent as it was. + /// Never queued as text: the queue re-expands what it holds, which + /// would resolve the mention a second time and read the file again. + #[allow(clippy::type_complexity)] + pub deferred_prompt: Option<(String, Vec)>, /// Whether `ui.edit_mode` asked for vi bindings. pub vi_mode: bool, /// Composer mode when `vi_mode` is on. @@ -630,6 +636,7 @@ impl App { model_picker: None, pending_images: Vec::new(), pending_attachments: Vec::new(), + deferred_prompt: None, vi_mode: false, composer_mode: ComposerMode::Insert, vi_pending_d: false, @@ -2719,22 +2726,20 @@ impl App { /// queued — and that one has its own attachments staged with it. /// Overwriting it would lose it silently; keeping it and attaching /// *these* blocks would be worse still, putting one prompt's image on - /// another's turn. So the newer prompt keeps its turn, this one goes - /// back to the head of the queue, and the user is told its images did - /// not come with it. + /// another's turn. So the newer prompt keeps its turn and this one is + /// held aside *with its blocks* until the turn frees up — not queued + /// as text, which would re-expand the mention and read the file a + /// second time, sending bytes the staged turn never had. pub fn accept_encoded_attachments( &mut self, prompt: String, blocks: Vec, ) { if self.pending_submit.is_some() { - self.queue.push_front(prompt); - if !blocks.is_empty() { - self.transcript.push(TranscriptItem::System( - "another prompt was sent first — the queued prompt's images were not attached" - .into(), - )); - } + self.transcript.push(TranscriptItem::System( + "another prompt was sent first — sending this one with its images next".into(), + )); + self.deferred_prompt = Some((prompt, blocks)); self.dirty = true; return; } @@ -2743,6 +2748,23 @@ impl App { self.dirty = true; } + /// Send a deferred prompt once the turn it was waiting behind is gone. + /// + /// Restored with the blocks it was encoded with, so the file is never + /// read again. Held back while another prompt's descriptors are still + /// staged, so the two cannot be mixed. + pub fn rearm_deferred_prompt(&mut self) { + if self.pending_submit.is_some() || !self.pending_images.is_empty() { + return; + } + if let Some((prompt, blocks)) = self.deferred_prompt.take() { + self.pending_attachments = blocks; + self.pending_submit = Some(prompt); + self.phase = Phase::Streaming; + self.dirty = true; + } + } + /// Give up on a prompt whose attachments were still being read when the /// user cancelled. /// @@ -2757,6 +2779,8 @@ impl App { /// interjection with its own images — and clearing that would send it /// as a text-only turn. pub fn abandon_staged_attachments(&mut self) { + // A cancel means nothing more should go out on its own. + self.deferred_prompt = None; self.turn_live = false; self.turn_started_at = None; self.phase = if self.pending_submit.is_some() { @@ -5260,17 +5284,66 @@ mod tests { app.pending_attachments.is_empty(), "the newer prompt inherited another prompt's image" ); + assert!( + app.deferred_prompt.is_some(), + "the superseded prompt was dropped" + ); + assert!( + app.queue.iter().all(|q| q != "look at @shot.png"), + "the expanded prompt was queued as text and would re-expand" + ); + } + + /// The deferred prompt comes back with the blocks it was encoded + /// with. Queuing it as text would re-run `expand_mentions`, reading + /// the file a second time and sending bytes the staged turn never had + /// — after telling the user it had already been read. + #[test] + fn a_deferred_prompt_returns_with_its_original_blocks() { + let (_dir, mut app) = app_in_workspace(); + app.enqueue_turn_from_command("show me the diff".into()); + app.accept_encoded_attachments("look at @shot.png".into(), vec![png_block()]); + + // The turn it was waiting behind starts and finishes. + let _ = app.pending_submit.take(); + app.rearm_deferred_prompt(); + + assert_eq!(app.pending_submit.as_deref(), Some("look at @shot.png")); assert_eq!( - app.queue.front().map(String::as_str), - Some("look at @shot.png"), - "the superseded prompt was dropped instead of queued" + app.pending_attachments.len(), + 1, + "the deferred prompt lost its images" ); + assert!(app.deferred_prompt.is_none(), "deferred prompt sent twice"); + } + + /// Re-arming must not race another prompt's staged descriptors: those + /// belong to a different turn. + #[test] + fn a_deferred_prompt_waits_for_staged_descriptors_to_clear() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + write_png(&app, "later.png"); + type_input(&mut app, "later @later.png"); + app.submit(); + let _ = app.pending_submit.take(); + assert_eq!(app.pending_images.len(), 1, "precondition"); + + app.rearm_deferred_prompt(); assert!( - app.transcript - .iter() - .any(|i| matches!(i, TranscriptItem::System(s) if s.contains("were not attached"))), - "no note that the images did not come along" + app.pending_submit.is_none(), + "sent a deferred prompt while another's descriptors were staged" ); + assert!(app.deferred_prompt.is_some(), "deferred prompt lost"); + } + + /// A cancel means nothing more goes out on its own. + #[test] + fn cancelling_drops_a_deferred_prompt() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + app.abandon_staged_attachments(); + assert!(app.deferred_prompt.is_none(), "cancel left a prompt armed"); } /// A cancel abandons only the prompt that was being read for. If the diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index bf8caf56..581dd5e8 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -624,6 +624,12 @@ pub(super) async fn event_loop( } } + // A prompt that was held aside for a turn that has since gone can + // send now, with the blocks it was already encoded with. + if turn.is_none() && !encoding { + app.rearm_deferred_prompt(); + } + // Hand a prompt's images to the blocking pool. The descriptors were // opened when their mentions were validated, so nothing is resolved // here; the result comes back through `img_rx` and re-arms the From f486a4251e27cd5c944ceb3342a3660cdb50a6fd Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 01:21:31 -0700 Subject: [PATCH 17/19] fix(tui): make cancel reach a held prompt and free the staged turn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three faults in the same interaction: A prompt held aside behind a live turn survived that turn's cancellation and then sent itself, file and all, after the user had stopped. Cancel now drops it — unless a prompt is already staged, which is interject saying it cancelled in order to send something else, and the held prompt keeps its place behind that. Cancelling during a read only marked the result for discard: the phase stayed Streaming until a read that may never finish came back, queueing every later prompt behind a turn that never started. The encode now carries an id; a cancel forgets it, releases the staged turn immediately, and the late result is recognised as stale when it lands. A held prompt was armed at the top of the loop, so after its turn was reaped it waited for an unrelated event, and the ordinary queue could overtake it. It is armed as soon as the handle is taken, ahead of the queue it was submitted before. --- crates/cli/src/ui/modern/app.rs | 44 ++++++++++++++++++++++++ crates/cli/src/ui/modern/run.rs | 61 +++++++++++++++++++-------------- 2 files changed, 80 insertions(+), 25 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 0273183b..05d63e7e 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -2748,6 +2748,19 @@ impl App { self.dirty = true; } + /// Drop anything that would send itself after a cancel. + /// + /// Interject cancels the live turn *in order to* send something it has + /// already staged, so a prompt waiting to submit means this is a + /// redirect rather than a stop — and a held prompt keeps its place + /// behind it. A bare Ctrl+C stages nothing, and then nothing may + /// follow on its own. + pub fn cancel_pending_followups(&mut self) { + if self.pending_submit.is_none() { + self.deferred_prompt = None; + } + } + /// Send a deferred prompt once the turn it was waiting behind is gone. /// /// Restored with the blocks it was encoded with, so the file is never @@ -5337,6 +5350,37 @@ mod tests { assert!(app.deferred_prompt.is_some(), "deferred prompt lost"); } + /// Ctrl+C on the turn a prompt is waiting behind must stop that prompt + /// too — otherwise the file goes out after the user stopped the flow. + #[test] + fn cancelling_a_turn_drops_a_prompt_waiting_behind_it() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + // A bare cancel: nothing staged to send. + app.cancel_pending_followups(); + assert!( + app.deferred_prompt.is_none(), + "an image prompt would have sent itself after a cancel" + ); + } + + /// Interject cancels the live turn in order to send something else, so + /// the held prompt keeps its place behind the interjection. + #[test] + fn interjecting_keeps_a_prompt_waiting_behind_it() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + type_input(&mut app, "do this instead"); + app.interject(); + assert_eq!(app.pending_submit.as_deref(), Some("do this instead")); + + app.cancel_pending_followups(); + assert!( + app.deferred_prompt.is_some(), + "interject dropped a prompt it only meant to go ahead of" + ); + } + /// A cancel means nothing more goes out on its own. #[test] fn cancelling_drops_a_deferred_prompt() { diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 581dd5e8..69bb232f 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -345,16 +345,18 @@ pub(super) async fn event_loop( // mount would stop redraws and Ctrl+C until the read finished. #[allow(clippy::type_complexity)] let (img_tx, mut img_rx) = tokio::sync::mpsc::unbounded_channel::<( + u64, String, Vec, Vec, )>(); - // Set while an encode is in flight: the turn it belongs to must not - // start without it, and a second one must not be queued behind it. - let mut encoding = false; - // A cancel that lands mid-encode cannot stop the read, so the result - // is dropped when it arrives instead. - let mut discard_encoding = false; + // Identifies the encode in flight, if any: the turn it belongs to must + // not start without it, and a second one must not be queued behind it. + // A cancel forgets the id rather than waiting — the read cannot be + // stopped, so its result is recognised as stale when it lands and the + // staged turn is released immediately. + let mut encode_seq = 0u64; + let mut active_encode: Option = None; // Seed the pane once so tasks adopted from a previous process show // before the first turn arms the periodic poll. app.sync_background_tasks(manager_rows(&task_manager).await); @@ -626,7 +628,7 @@ pub(super) async fn event_loop( // A prompt that was held aside for a turn that has since gone can // send now, with the blocks it was already encoded with. - if turn.is_none() && !encoding { + if turn.is_none() && active_encode.is_none() { app.rearm_deferred_prompt(); } @@ -636,22 +638,24 @@ pub(super) async fn event_loop( // prompt, which keeps this loop free to redraw and to take a Ctrl+C // while a slow mount is being read. if turn.is_none() - && !encoding + && active_encode.is_none() && !app.pending_images.is_empty() && let Some(prompt) = app.pending_submit.take() { let images = std::mem::take(&mut app.pending_images); let tx = img_tx.clone(); - encoding = true; + encode_seq += 1; + let id = encode_seq; + active_encode = Some(id); tokio::task::spawn_blocking(move || { let (blocks, notes) = super::mentions::encode_staged_images(images); - let _ = tx.send((prompt, blocks, notes)); + let _ = tx.send((id, prompt, blocks, notes)); }); } // Start a pending turn if idle. if turn.is_none() - && !encoding + && active_encode.is_none() && let Some(prompt) = app.pending_submit.take() { let blocks = std::mem::take(&mut app.pending_attachments); @@ -685,10 +689,16 @@ pub(super) async fn event_loop( if let Some(ref h) = turn { h.cancel(); } - // A read already handed to the blocking pool cannot be stopped, - // so its result is thrown away when it lands. - if encoding { - discard_encoding = true; + // Nothing may follow on its own after a cancel — including a + // prompt held aside behind the turn being cancelled. Interject + // is the exception and says so by having staged its own prompt. + app.cancel_pending_followups(); + // A read already handed to the blocking pool cannot be stopped. + // Forget its id instead: the result is stale when it lands, and + // the staged turn is released now rather than after a read that + // may never finish. + if active_encode.take().is_some() { + app.abandon_staged_attachments(); } app.cancel_requested = false; } @@ -763,6 +773,12 @@ pub(super) async fn event_loop( } app.mark_turn_idle(); + // A prompt held aside with its images was submitted before + // anything in the queue, so it goes first — and it goes now + // rather than waiting for whatever event next wakes the + // loop, since nothing else would arm it. + app.rearm_deferred_prompt(); + // Queue handling (plan §M5): auto-send the head on a clean // finish; on abort/error keep the queue and tell the user. // Interject leaves `pending_submit` set so we start it even @@ -912,18 +928,13 @@ pub(super) async fn event_loop( // Encoded image attachments coming back from the blocking pool. // The prompt is re-armed with them so the turn starts on the // next pass through the loop above. - Some((prompt, blocks, notes)) = img_rx.recv() => { - encoding = false; - if discard_encoding { - // Cancelled while the read was in flight: the bytes are + Some((id, prompt, blocks, notes)) = img_rx.recv() => { + if active_encode != Some(id) { + // Cancelled while this read was in flight: the state it + // belonged to was released then, so the bytes are simply // dropped rather than sent with a turn nobody asked for. - // The prompt was taken and no turn was ever spawned, so - // nothing else will end the streaming phase — leaving it - // set would queue every later prompt behind a turn that - // does not exist. - discard_encoding = false; - app.abandon_staged_attachments(); } else { + active_encode = None; for note in notes { app.transcript.push(super::app::TranscriptItem::System(note)); } From 32a63063f4e13a58482c7bbe72200d5a98e65060 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 01:29:36 -0700 Subject: [PATCH 18/19] fix(tui): hold prompts in order and tie them to their conversation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Dropping the read in flight also cleared the held prompts, so an interject arriving mid-encode lost the very prompt the interject rule exists to protect. Whether held prompts survive is now the cancel policy's decision alone; abandoning a read says only that this read is being dropped. A second prompt could be held before the first had gone, and the single slot silently replaced it — after telling the user it would be sent next. They are held in a deque and sent in the order they were submitted. An encode now carries the conversation it was submitted in. The engine lock is free while it runs, so `/clear`, `/resume` and `/rewind` can replace the conversation underneath it; the result is dropped when that happens, and replacing a conversation takes its staged attachments with it — a file staged for the old thread must not surface in the new one. --- crates/cli/src/ui/modern/app.rs | 115 +++++++++++++++++++++++++------- crates/cli/src/ui/modern/run.rs | 15 ++++- 2 files changed, 105 insertions(+), 25 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index 05d63e7e..d9f20d6a 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -446,12 +446,16 @@ pub struct App { /// The same images once read and encoded off the UI thread, waiting for /// the turn they belong to to start. pub pending_attachments: Vec, - /// A prompt whose images were already read when another prompt took - /// the turn, held with those images so it can be sent as it was. - /// Never queued as text: the queue re-expands what it holds, which - /// would resolve the mention a second time and read the file again. + /// Prompts whose images were already read when another prompt took + /// the turn, each held with its own images so it can be sent as it + /// was. Never queued as text: the queue re-expands what it holds, + /// which would resolve the mention a second time and read the file + /// again. A deque because a second one can be held before the first + /// has gone, and the order they were submitted in is the order they + /// must be sent in. #[allow(clippy::type_complexity)] - pub deferred_prompt: Option<(String, Vec)>, + pub deferred_prompts: + std::collections::VecDeque<(String, Vec)>, /// Whether `ui.edit_mode` asked for vi bindings. pub vi_mode: bool, /// Composer mode when `vi_mode` is on. @@ -636,7 +640,7 @@ impl App { model_picker: None, pending_images: Vec::new(), pending_attachments: Vec::new(), - deferred_prompt: None, + deferred_prompts: std::collections::VecDeque::new(), vi_mode: false, composer_mode: ComposerMode::Insert, vi_pending_d: false, @@ -2434,6 +2438,12 @@ impl App { pub fn new_conversation(&mut self) { self.conversation_epoch = self.conversation_epoch.wrapping_add(1); self.todos.clear(); + // Anything staged belonged to the conversation being replaced. + // Sending it into the new one would attach a file to a thread the + // user never attached it to. + self.deferred_prompts.clear(); + self.pending_images.clear(); + self.pending_attachments.clear(); self.dirty = true; } @@ -2739,7 +2749,7 @@ impl App { self.transcript.push(TranscriptItem::System( "another prompt was sent first — sending this one with its images next".into(), )); - self.deferred_prompt = Some((prompt, blocks)); + self.deferred_prompts.push_back((prompt, blocks)); self.dirty = true; return; } @@ -2757,7 +2767,7 @@ impl App { /// follow on its own. pub fn cancel_pending_followups(&mut self) { if self.pending_submit.is_none() { - self.deferred_prompt = None; + self.deferred_prompts.clear(); } } @@ -2770,7 +2780,7 @@ impl App { if self.pending_submit.is_some() || !self.pending_images.is_empty() { return; } - if let Some((prompt, blocks)) = self.deferred_prompt.take() { + if let Some((prompt, blocks)) = self.deferred_prompts.pop_front() { self.pending_attachments = blocks; self.pending_submit = Some(prompt); self.phase = Phase::Streaming; @@ -2792,8 +2802,10 @@ impl App { /// interjection with its own images — and clearing that would send it /// as a text-only turn. pub fn abandon_staged_attachments(&mut self) { - // A cancel means nothing more should go out on its own. - self.deferred_prompt = None; + // Held prompts are not touched here: whether they survive is the + // cancel policy's call (`cancel_pending_followups`), and an + // interject reaching this path still means only that *this* read + // is being dropped. self.turn_live = false; self.turn_started_at = None; self.phase = if self.pending_submit.is_some() { @@ -5298,7 +5310,7 @@ mod tests { "the newer prompt inherited another prompt's image" ); assert!( - app.deferred_prompt.is_some(), + !app.deferred_prompts.is_empty(), "the superseded prompt was dropped" ); assert!( @@ -5327,7 +5339,10 @@ mod tests { 1, "the deferred prompt lost its images" ); - assert!(app.deferred_prompt.is_none(), "deferred prompt sent twice"); + assert!( + app.deferred_prompts.is_empty(), + "deferred prompt sent twice" + ); } /// Re-arming must not race another prompt's staged descriptors: those @@ -5335,7 +5350,8 @@ mod tests { #[test] fn a_deferred_prompt_waits_for_staged_descriptors_to_clear() { let (_dir, mut app) = app_in_workspace(); - app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); write_png(&app, "later.png"); type_input(&mut app, "later @later.png"); app.submit(); @@ -5347,7 +5363,7 @@ mod tests { app.pending_submit.is_none(), "sent a deferred prompt while another's descriptors were staged" ); - assert!(app.deferred_prompt.is_some(), "deferred prompt lost"); + assert!(!app.deferred_prompts.is_empty(), "deferred prompt lost"); } /// Ctrl+C on the turn a prompt is waiting behind must stop that prompt @@ -5355,11 +5371,12 @@ mod tests { #[test] fn cancelling_a_turn_drops_a_prompt_waiting_behind_it() { let (_dir, mut app) = app_in_workspace(); - app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); // A bare cancel: nothing staged to send. app.cancel_pending_followups(); assert!( - app.deferred_prompt.is_none(), + app.deferred_prompts.is_empty(), "an image prompt would have sent itself after a cancel" ); } @@ -5369,25 +5386,77 @@ mod tests { #[test] fn interjecting_keeps_a_prompt_waiting_behind_it() { let (_dir, mut app) = app_in_workspace(); - app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); type_input(&mut app, "do this instead"); app.interject(); assert_eq!(app.pending_submit.as_deref(), Some("do this instead")); app.cancel_pending_followups(); assert!( - app.deferred_prompt.is_some(), + !app.deferred_prompts.is_empty(), "interject dropped a prompt it only meant to go ahead of" ); } - /// A cancel means nothing more goes out on its own. + /// Dropping the read in flight says nothing about prompts already + /// held: whether those survive is the cancel policy's call, and an + /// interject that lands mid-encode must not lose them. #[test] - fn cancelling_drops_a_deferred_prompt() { + fn abandoning_a_read_leaves_held_prompts_alone() { let (_dir, mut app) = app_in_workspace(); - app.deferred_prompt = Some(("earlier @a.png".into(), vec![png_block()])); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); + type_input(&mut app, "do this instead"); + app.interject(); + + app.cancel_pending_followups(); app.abandon_staged_attachments(); - assert!(app.deferred_prompt.is_none(), "cancel left a prompt armed"); + + assert_eq!( + app.deferred_prompts.len(), + 1, + "an interject during an encode lost a held prompt" + ); + } + + /// Held prompts are sent in the order they were submitted, so a second + /// one cannot overwrite the first. + #[test] + fn held_prompts_keep_their_order() { + let (_dir, mut app) = app_in_workspace(); + app.enqueue_turn_from_command("busy".into()); + app.accept_encoded_attachments("first @a.png".into(), vec![png_block()]); + app.accept_encoded_attachments("second @b.png".into(), vec![png_block()]); + assert_eq!(app.deferred_prompts.len(), 2, "a held prompt was dropped"); + + let _ = app.pending_submit.take(); + app.rearm_deferred_prompt(); + assert_eq!(app.pending_submit.as_deref(), Some("first @a.png")); + let _ = app.pending_submit.take(); + app.rearm_deferred_prompt(); + assert_eq!(app.pending_submit.as_deref(), Some("second @b.png")); + } + + /// A cleared, resumed or rewound conversation takes its attachments + /// with it: a file staged for the old thread must not surface in the + /// new one. + #[test] + fn a_replaced_conversation_drops_staged_attachments() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); + app.pending_attachments = vec![png_block()]; + write_png(&app, "shot.png"); + type_input(&mut app, "look at @shot.png"); + app.submit(); + assert_eq!(app.pending_images.len(), 1, "precondition"); + + app.new_conversation(); + + assert!(app.deferred_prompts.is_empty(), "held prompt survived"); + assert!(app.pending_images.is_empty(), "descriptors survived"); + assert!(app.pending_attachments.is_empty(), "blocks survived"); } /// A cancel abandons only the prompt that was being read for. If the diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 964cd104..3f222e0f 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -345,6 +345,7 @@ pub(super) async fn event_loop( // mount would stop redraws and Ctrl+C until the read finished. #[allow(clippy::type_complexity)] let (img_tx, mut img_rx) = tokio::sync::mpsc::unbounded_channel::<( + u64, u64, String, Vec, @@ -623,9 +624,13 @@ pub(super) async fn event_loop( encode_seq += 1; let id = encode_seq; active_encode = Some(id); + // Stamped with the conversation it was submitted in: the engine + // lock is free while this runs, so `/clear`, `/resume` or + // `/rewind` can replace the conversation underneath it. + let epoch = app.conversation_epoch; tokio::task::spawn_blocking(move || { let (blocks, notes) = super::mentions::encode_staged_images(images); - let _ = tx.send((id, prompt, blocks, notes)); + let _ = tx.send((id, epoch, prompt, blocks, notes)); }); } @@ -904,11 +909,17 @@ pub(super) async fn event_loop( // Encoded image attachments coming back from the blocking pool. // The prompt is re-armed with them so the turn starts on the // next pass through the loop above. - Some((id, prompt, blocks, notes)) = img_rx.recv() => { + Some((id, epoch, prompt, blocks, notes)) = img_rx.recv() => { if active_encode != Some(id) { // Cancelled while this read was in flight: the state it // belonged to was released then, so the bytes are simply // dropped rather than sent with a turn nobody asked for. + } else if epoch != app.conversation_epoch { + // The conversation it was submitted in has been cleared, + // resumed or rewound. Starting it now would attach the + // file to a thread the user never attached it to. + active_encode = None; + app.abandon_staged_attachments(); } else { active_encode = None; for note in notes { From cae6632c50c02888b2de161889fb391fce56a1b7 Mon Sep 17 00:00:00 2001 From: emal Date: Mon, 27 Jul 2026 01:41:39 -0700 Subject: [PATCH 19/19] fix(tui): say when a cancel is a redirect, and drop stale encodes at once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A staged prompt was taken as proof that the cancel came from interject, but a slash command can stage one at any moment — including while the user is pressing Ctrl+C to stop everything, which then let a held image prompt send itself anyway. Interject and queue send-now now say outright that they cancelled in order to send; every other cancel is a stop. A replaced conversation also waited for the read in flight to come back before releasing anything, so on a slow mount every prompt in the new conversation queued behind an encode belonging to a conversation nobody was looking at any more. The loop notices the epoch change itself and drops the encode there and then. --- crates/cli/src/ui/modern/app.rs | 62 +++++++++++++++++++++++++++++---- crates/cli/src/ui/modern/run.rs | 15 ++++++++ 2 files changed, 71 insertions(+), 6 deletions(-) diff --git a/crates/cli/src/ui/modern/app.rs b/crates/cli/src/ui/modern/app.rs index d9f20d6a..e03fedf8 100644 --- a/crates/cli/src/ui/modern/app.rs +++ b/crates/cli/src/ui/modern/app.rs @@ -453,6 +453,11 @@ pub struct App { /// again. A deque because a second one can be held before the first /// has gone, and the order they were submitted in is the order they /// must be sent in. + /// Set when a cancel was asked for *in order to send something else* + /// (interject, queue send-now). A staged prompt alone cannot tell the + /// difference: a slash command can stage one at any moment, including + /// while the user is pressing Ctrl+C to stop everything. + pub cancel_is_interject: bool, #[allow(clippy::type_complexity)] pub deferred_prompts: std::collections::VecDeque<(String, Vec)>, @@ -640,6 +645,7 @@ impl App { model_picker: None, pending_images: Vec::new(), pending_attachments: Vec::new(), + cancel_is_interject: false, deferred_prompts: std::collections::VecDeque::new(), vi_mode: false, composer_mode: ComposerMode::Insert, @@ -2055,6 +2061,7 @@ impl App { )); // enqueue_turn clears input (already empty) and sets pending_submit. self.enqueue_turn(text); + self.cancel_is_interject = true; self.request_cancel(); } else { self.enqueue_turn(text); @@ -2677,6 +2684,7 @@ impl App { "queue send-now — cancelling turn…".into(), )); self.enqueue_turn(text); + self.cancel_is_interject = true; self.request_cancel(); } else { self.enqueue_turn(text); @@ -2760,13 +2768,14 @@ impl App { /// Drop anything that would send itself after a cancel. /// - /// Interject cancels the live turn *in order to* send something it has - /// already staged, so a prompt waiting to submit means this is a - /// redirect rather than a stop — and a held prompt keeps its place - /// behind it. A bare Ctrl+C stages nothing, and then nothing may - /// follow on its own. + /// Interject and queue send-now cancel the live turn *in order to* + /// send something else, and say so; a held prompt then keeps its place + /// behind that. Every other cancel is a stop, and nothing may follow + /// it on its own — a prompt merely sitting in `pending_submit` proves + /// nothing, since a slash command can stage one at any moment, + /// including while the user is pressing Ctrl+C. pub fn cancel_pending_followups(&mut self) { - if self.pending_submit.is_none() { + if !std::mem::take(&mut self.cancel_is_interject) { self.deferred_prompts.clear(); } } @@ -5388,6 +5397,8 @@ mod tests { let (_dir, mut app) = app_in_workspace(); app.deferred_prompts .push_back(("earlier @a.png".into(), vec![png_block()])); + // Interject only cancels when there is a turn to cancel. + app.phase = Phase::Streaming; type_input(&mut app, "do this instead"); app.interject(); assert_eq!(app.pending_submit.as_deref(), Some("do this instead")); @@ -5399,6 +5410,44 @@ mod tests { ); } + /// A staged prompt proves nothing on its own: a slash command can put + /// one there at any moment, including while the user is pressing + /// Ctrl+C to stop everything. Only an interject says it cancelled in + /// order to send. + #[test] + fn a_bare_cancel_drops_held_prompts_even_with_a_command_staged() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); + // A slash command stages its prompt directly, without interjecting. + app.enqueue_turn_from_command("show me the diff".into()); + assert!(app.pending_submit.is_some(), "precondition"); + + app.cancel_pending_followups(); + assert!( + app.deferred_prompts.is_empty(), + "a bare cancel let an image prompt follow on its own" + ); + } + + /// Queue send-now is the same bargain as interject: it cancels in + /// order to send, so held prompts keep their place. + #[test] + fn queue_send_now_keeps_held_prompts() { + let (_dir, mut app) = app_in_workspace(); + app.deferred_prompts + .push_back(("earlier @a.png".into(), vec![png_block()])); + app.queue.push_back("queued work".into()); + app.phase = Phase::Streaming; + app.queue_send_selected(); + + app.cancel_pending_followups(); + assert!( + !app.deferred_prompts.is_empty(), + "queue send-now dropped a held prompt" + ); + } + /// Dropping the read in flight says nothing about prompts already /// held: whether those survive is the cancel policy's call, and an /// interject that lands mid-encode must not lose them. @@ -5407,6 +5456,7 @@ mod tests { let (_dir, mut app) = app_in_workspace(); app.deferred_prompts .push_back(("earlier @a.png".into(), vec![png_block()])); + app.phase = Phase::Streaming; type_input(&mut app, "do this instead"); app.interject(); diff --git a/crates/cli/src/ui/modern/run.rs b/crates/cli/src/ui/modern/run.rs index 3f222e0f..5480a3c2 100644 --- a/crates/cli/src/ui/modern/run.rs +++ b/crates/cli/src/ui/modern/run.rs @@ -358,6 +358,10 @@ pub(super) async fn event_loop( // staged turn is released immediately. let mut encode_seq = 0u64; let mut active_encode: Option = None; + // The conversation the loop last saw, so a `/clear`, `/resume` or + // `/rewind` can be noticed the moment it happens rather than when a + // read that may never finish comes back. + let mut seen_epoch = app.conversation_epoch; // Seed the pane once so tasks adopted from a previous process show // before the first turn arms the periodic poll. app.sync_background_tasks(manager_rows(&task_manager).await); @@ -603,6 +607,17 @@ pub(super) async fn event_loop( } } + // A replaced conversation invalidates a read in flight at once: + // waiting for it would hold every prompt in the new conversation + // behind an encode that belongs to a conversation nobody is + // looking at any more. + if app.conversation_epoch != seen_epoch { + seen_epoch = app.conversation_epoch; + if active_encode.take().is_some() { + app.abandon_staged_attachments(); + } + } + // A prompt that was held aside for a turn that has since gone can // send now, with the blocks it was already encoded with. if turn.is_none() && active_encode.is_none() {