diff --git a/CHANGELOG.md b/CHANGELOG.md index cfd6d6ee8f..a2c43ba358 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,7 @@ quieter, and Fleet runs can be checked before they spend anything. - **[@cenab](https://github.com/cenab)** — requested the Tsubasa provider row and supplied its endpoint, key and model values ([#6695](https://github.com/Hmbown/Codewhale/issues/6695)). - **[@BX166](https://github.com/BX166)** — reported the AICraft provider row missing its key console, docs link and guidance, and supplied the values ([#6616](https://github.com/Hmbown/Codewhale/issues/6616)). - **[@Water-Run](https://github.com/Water-Run)** — ingested namespaced model-only catalog entries so models present only in the canonical `models` map reach the offering list ([#6400](https://github.com/Hmbown/Codewhale/pull/6400)), and retired the blanket dead-code allowance with its unused feature stages, tightening the budget to match ([#6402](https://github.com/Hmbown/Codewhale/pull/6402)). +- **[@SparkofSpike](https://github.com/SparkofSpike)** — let making room survive a provider request-body limit (HTTP 413) by shrinking, then replacing, inline images for that one summary pass ([#6642](https://github.com/Hmbown/Codewhale/pull/6642)). ### Added diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index 99a75f2b9d..4467a60770 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -53,6 +53,7 @@ quieter, and Fleet runs can be checked before they spend anything. - **[@cenab](https://github.com/cenab)** — requested the Tsubasa provider row and supplied its endpoint, key and model values ([#6695](https://github.com/Hmbown/Codewhale/issues/6695)). - **[@BX166](https://github.com/BX166)** — reported the AICraft provider row missing its key console, docs link and guidance, and supplied the values ([#6616](https://github.com/Hmbown/Codewhale/issues/6616)). - **[@Water-Run](https://github.com/Water-Run)** — ingested namespaced model-only catalog entries so models present only in the canonical `models` map reach the offering list ([#6400](https://github.com/Hmbown/Codewhale/pull/6400)), and retired the blanket dead-code allowance with its unused feature stages, tightening the budget to match ([#6402](https://github.com/Hmbown/Codewhale/pull/6402)). +- **[@SparkofSpike](https://github.com/SparkofSpike)** — let making room survive a provider request-body limit (HTTP 413) by shrinking, then replacing, inline images for that one summary pass ([#6642](https://github.com/Hmbown/Codewhale/pull/6642)). ### Added diff --git a/crates/tui/src/compaction.rs b/crates/tui/src/compaction.rs index c5f7bae411..5545b73c74 100644 --- a/crates/tui/src/compaction.rs +++ b/crates/tui/src/compaction.rs @@ -3,6 +3,7 @@ use anyhow::Result; use std::collections::HashMap; use std::fmt::Write; +use std::sync::Arc; use std::time::Duration; use crate::config::DEFAULT_TEXT_MODEL; @@ -65,9 +66,20 @@ pub struct CompactionConfig { pub retained_user_message_tokens: usize, } +/// Host callback for user-visible progress during a compaction pass. +/// +/// Compaction runs inside the engine's provider boundary, where the engine +/// owns the only channel that reaches the person. Injecting a sink lets a +/// downgrade (re-encoding images after an HTTP 413, say) say what it is doing +/// while it is doing it, instead of surfacing only when the pass ends. +pub trait CompactionNoticeSink: Send + Sync + std::fmt::Debug { + /// Deliver one already-rendered, user-visible sentence. + fn notice(&self, message: String); +} + /// Host-prepared configuration carried from compaction eligibility through /// the replacement-history commit. -#[derive(Debug, Clone, PartialEq)] +#[derive(Clone)] pub struct PreparedCompactionEnvelope { pub config: CompactionConfig, /// Durable handoff owner; set by the engine, never added to the stable prefix. @@ -80,6 +92,33 @@ pub struct PreparedCompactionEnvelope { /// request that omits it shares no cacheable prefix with the turn it /// summarizes and re-bills the whole history uncached. pub reasoning_effort: Option, + /// User-visible progress sink supplied by the host that owns the run + /// (the interactive engine, typically). `None` keeps every notice in the + /// log only — the pass itself never depends on it. + pub notice_sink: Option>, +} + +impl std::fmt::Debug for PreparedCompactionEnvelope { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("PreparedCompactionEnvelope") + .field("config", &self.config) + .field("session_id", &self.session_id) + .field("tools", &self.tools) + .field("reasoning_effort", &self.reasoning_effort) + .field("notice_sink", &self.notice_sink.as_ref().map(|_| "")) + .finish() + } +} + +impl PartialEq for PreparedCompactionEnvelope { + /// The notice sink is host plumbing, not envelope content: two passes over + /// the same config are equal whether or not their host delivers notices. + fn eq(&self, other: &Self) -> bool { + self.config == other.config + && self.session_id == other.session_id + && self.tools == other.tools + && self.reasoning_effort == other.reasoning_effort + } } impl PreparedCompactionEnvelope { @@ -90,6 +129,7 @@ impl PreparedCompactionEnvelope { session_id: None, tools: None, reasoning_effort: None, + notice_sink: None, } } } @@ -1358,6 +1398,7 @@ pub async fn compact_messages_safe( system_prompt, prepared.tools.as_deref(), prepared.reasoning_effort.as_deref(), + prepared.notice_sink.as_deref(), &mut quality_retries, invocation_usage, ) @@ -1522,6 +1563,7 @@ async fn compact_messages( None, None, None, + None, &mut quality_retries, &mut invocation_usage, ) @@ -1536,6 +1578,7 @@ async fn compact_messages_with_metadata( system_prompt: Option<&SystemPrompt>, tools: Option<&[Tool]>, reasoning_effort: Option<&str>, + notice_sink: Option<&dyn CompactionNoticeSink>, quality_retries: &mut u32, invocation_usage: &mut Usage, ) -> Result<(Vec, Option, CompactionCoverage)> { @@ -1550,6 +1593,7 @@ async fn compact_messages_with_metadata( system_prompt, tools, reasoning_effort, + notice_sink, quality_retries, invocation_usage, ) @@ -1784,6 +1828,7 @@ async fn create_summary( system_prompt: Option<&SystemPrompt>, tools: Option<&[Tool]>, reasoning_effort: Option<&str>, + notice_sink: Option<&dyn CompactionNoticeSink>, quality_retries: &mut u32, invocation_usage: &mut Usage, ) -> Result { @@ -1813,6 +1858,12 @@ async fn create_summary( }); let mut quality_retry_used = false; + // Request-size ladder: an HTTP 413 caps the request *body*, which the + // token-side budget cannot predict (a flat per-image token estimate can + // hide megabytes of base64). A refused summary call gets two byte-side + // downgrades before it fails — re-encode the inline images smaller, then + // replace them with text notes — each followed by exactly one retry. + let mut size_ladder = RequestSizeLadder::Start; loop { // Codex compaction is a normal model generation over the existing // cached prefix. Do the same here: the resolved route decides how @@ -1833,7 +1884,10 @@ async fn create_summary( let cost_scope = crate::cost_status::scope_token(); let response = match client.create_message(request).await { Ok(response) => response, - Err(err) if is_context_window_error(&err) => { + // A byte-side size rejection can also read like a length problem + // (gateway HTML pages carry their own wording); the size ladder + // owns it, not the drop-oldest ladder. + Err(err) if !is_request_too_large_error(&err) && is_context_window_error(&err) => { if !drop_oldest_history_messages(&mut request_messages) { logging::warn( "Compaction summary input is over the context window with nothing \ @@ -1848,6 +1902,74 @@ async fn create_summary( )); continue; } + Err(err) if is_request_too_large_error(&err) => match size_ladder { + RequestSizeLadder::Start => { + // Decoding, resizing and re-encoding megabytes of inline + // images is CPU-bound work; run it off the async worker so + // the engine keeps servicing events while it happens. + let mut outbound = std::mem::take(&mut request_messages); + let joined = tokio::task::spawn_blocking(move || { + let shrunk = crate::image_attach::shrink_images_for_request(&mut outbound); + (outbound, shrunk) + }) + .await; + let (outbound, shrunk) = match joined { + Ok(joined) => joined, + Err(join_error) => { + return Err(err.context(format!( + "The summary request exceeded the provider's request-body limit and re-encoding its inline images failed: {join_error}" + ))); + } + }; + request_messages = outbound; + if shrunk.images_seen == 0 { + return Err(err.context( + "The summary request exceeded the provider's request-body limit and the history carries no inline images to re-encode", + )); + } + size_ladder = RequestSizeLadder::ImagesShrunk; + if shrunk.images > 0 { + let message = format!( + "Making room exceeded the provider's request-body limit (HTTP 413); re-encoded {} inline image(s) smaller ({} to {}) and is retrying the summary.", + shrunk.images, + crate::image_attach::human_bytes(shrunk.bytes_before), + crate::image_attach::human_bytes(shrunk.bytes_after), + ); + logging::warn(&message); + deliver_compaction_notice(notice_sink, message); + continue; + } + // Images are present but every one already fits its share of + // the byte budget, and the body was still refused: the cap + // sits below the budget. A retry would send identical bytes, + // so replace the images now instead of failing the pass. + if let Err(replace_error) = + replace_inline_images_for_retry(&mut request_messages, notice_sink) + { + return Err(err.context(format!( + "The summary request exceeded the provider's request-body limit; {replace_error}" + ))); + } + size_ladder = RequestSizeLadder::ImagesReplaced; + continue; + } + RequestSizeLadder::ImagesShrunk => { + if let Err(replace_error) = + replace_inline_images_for_retry(&mut request_messages, notice_sink) + { + return Err(err.context(format!( + "The summary request still exceeded the provider's request-body limit and {replace_error}" + ))); + } + size_ladder = RequestSizeLadder::ImagesReplaced; + continue; + } + RequestSizeLadder::ImagesReplaced => { + return Err(err.context( + "The summary request exceeded the provider's request-body limit even after re-encoding and then replacing every inline image", + )); + } + }, Err(err) => return Err(err), }; @@ -1933,6 +2055,75 @@ no replacement checkpoint was committed", } } +/// How far the request-size ladder for one summary call has descended. +/// +/// The ladder exists because HTTP 413 rejects the request *body* by bytes, +/// which the token-side context budget that governs compaction cannot see. +/// Each rung is one retry: re-encode the inline images under a byte budget, +/// then replace them with text notes. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum RequestSizeLadder { + /// No size downgrade applied yet. + Start, + /// Inline images were re-encoded smaller. + ImagesShrunk, + /// Inline images were replaced with text notes. + ImagesReplaced, +} + +/// Whether the provider refused the request body for size (HTTP 413 and its +/// common wordings). A smaller payload can succeed where this one did not, +/// which is exactly what the request-size ladder trades on. +/// +/// Walks the whole error chain: the rejection may be stated by a fronting +/// gateway (an HTML page from openresty reading "413 Request Entity Too +/// Large") and then wrapped again by the client's own error text, so the +/// top-level message alone is not reliable. +fn is_request_too_large_error(e: &anyhow::Error) -> bool { + e.chain().any(|cause| { + let lower = cause.to_string().to_lowercase(); + // The client always renders the status code into the message + // ("HTTP 413"), so that token is the primary signal; the phrase list + // below catches gateways that state the condition without it. + lower.contains("http 413") + || lower.contains("payload too large") + || lower.contains("request entity too large") + || lower.contains("request body too large") + || lower.contains("length limit exceeded") + }) +} + +/// Forward one user-visible progress sentence, when the host supplied a sink. +fn deliver_compaction_notice(sink: Option<&dyn CompactionNoticeSink>, message: String) { + if let Some(sink) = sink { + sink.notice(message); + } +} + +/// Replace every inline image for one retry and announce it. +/// +/// Shared by the two rungs that can reach the replace step: after a shrink +/// pass that changed something, and directly when the images already fit the +/// byte budget but the body was refused anyway. +fn replace_inline_images_for_retry( + messages: &mut [Message], + notice_sink: Option<&dyn CompactionNoticeSink>, +) -> Result { + let replaced = crate::image_attach::replace_images_with_placeholders( + messages, + "the summary request exceeded the provider's request-body limit (HTTP 413)", + ); + if replaced == 0 { + anyhow::bail!("no inline images were left to replace"); + } + let message = format!( + "Making room still exceeded the provider's request-body limit; replaced {replaced} inline image(s) with text notes for this summary pass and is retrying." + ); + logging::warn(&message); + deliver_compaction_notice(notice_sink, message); + Ok(replaced) +} + fn is_context_window_error(e: &anyhow::Error) -> bool { let text = e.to_string(); if crate::error_taxonomy::classify_error_message(&text) @@ -2368,6 +2559,348 @@ mod tests { 2. Key technical concepts — sqlite. 7. Pending tasks — finish the fixed clock. \ 8. Current work — rerunning the session tests."; + /// A real PNG whose bytes are worth shrinking. Noise defeats compression, + /// which is the point: the shrink ladder must actually re-encode. + fn noisy_png_bytes(width: u32, height: u32) -> Vec { + use image::ImageEncoder as _; + let mut pixels = image::RgbImage::new(width, height); + for (x, y, pixel) in pixels.enumerate_pixels_mut() { + *pixel = image::Rgb([ + (x.wrapping_mul(31) ^ y.wrapping_mul(17)) as u8, + (x.wrapping_mul(7) ^ y.wrapping_mul(29)) as u8, + (x.wrapping_add(y).wrapping_mul(13)) as u8, + ]); + } + let mut bytes = Vec::new(); + image::codecs::png::PngEncoder::new_with_quality( + &mut bytes, + image::codecs::png::CompressionType::Fast, + image::codecs::png::FilterType::NoFilter, + ) + .write_image( + pixels.as_raw(), + width, + height, + image::ExtendedColorType::Rgb8, + ) + .expect("encode fixture png"); + bytes + } + + fn png_data_url(width: u32, height: u32) -> String { + use base64::Engine as _; + format!( + "data:image/png;base64,{}", + base64::engine::general_purpose::STANDARD.encode(noisy_png_bytes(width, height)) + ) + } + + fn user_image_message(data_url: &str) -> Message { + Message { + role: Role::User, + content: vec![ + ContentBlock::Text { + text: "look at this screenshot".to_string(), + cache_control: None, + }, + ContentBlock::ImageUrl { + image_url: ImageUrlContent { + url: data_url.to_string(), + }, + }, + ], + } + } + + /// Total inline-image URL bytes a request carries, the byte side an HTTP + /// 413 boundary actually measures. + fn inline_image_bytes(request: &MessageRequest) -> usize { + request + .messages + .iter() + .flat_map(|message| message.content.iter()) + .map(|block| match block { + ContentBlock::ImageUrl { image_url } => image_url.url.len(), + ContentBlock::ToolResult { content_blocks, .. } => { + content_blocks.as_ref().map_or(0, |blocks| { + blocks.iter().fold(0, |sum, block| { + let nested = block + .get("data") + .and_then(serde_json::Value::as_str) + .map_or(0, str::len); + sum + nested + }) + }) + } + _ => 0, + }) + .sum() + } + + fn summary_content() -> Vec { + vec![ContentBlock::Text { + text: "1. Primary request: keep working on the session store. \ + 7. Pending: rerun the tests." + .to_string(), + cache_control: None, + }] + } + + fn request_body_413() -> anyhow::Error { + anyhow::anyhow!( + "LLM error: HTTP 413: Failed to buffer the request body: length limit exceeded" + ) + } + + /// The same boundary stated by a fronting gateway (openresty) instead of + /// the API's own body reader. Reported on 2026-09-26; the ladder must + /// treat it as the same byte-side rejection. + fn request_body_413_html_gateway() -> anyhow::Error { + anyhow::anyhow!( + "LLM error: HTTP 413: DeepSeek API returned an HTML error page (HTTP 413): \ + 413 Request Entity Too Large 413 Request Entity Too Large openresty" + ) + } + + #[derive(Debug, Default)] + struct RecordingNoticeSink { + messages: std::sync::Mutex>, + } + + impl RecordingNoticeSink { + fn messages(&self) -> Vec { + self.messages.lock().expect("notice sink").clone() + } + } + + impl CompactionNoticeSink for RecordingNoticeSink { + fn notice(&self, message: String) { + self.messages.lock().expect("notice sink").push(message); + } + } + + /// A 900x900 noise PNG exceeds the 2 MiB inline-image budget, so the + /// ladder must actually rewrite it. + fn body_413_retry_envelope( + sink: std::sync::Arc, + ) -> PreparedCompactionEnvelope { + let mut envelope = prepared(&CompactionConfig { + enabled: false, + ..CompactionConfig::default() + }); + envelope.notice_sink = Some(sink); + envelope + } + + #[tokio::test] + async fn request_body_413_reencodes_images_smaller_and_retries() { + let _environment = crate::test_support::lock_test_env(); + let messages = vec![ + msg("user", "context before the screenshots"), + user_image_message(&png_data_url(900, 900)), + ]; + let client = ScriptedSummaryClient::with_outcomes(vec![ + Err(request_body_413()), + Ok(summary_content()), + ]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let result = compact_messages_safe(&client, &messages, None, &envelope, &mut usage).await; + assert!( + result.is_ok(), + "the retry after shrinking must succeed: {result:?}" + ); + + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 2, "one rejection, one retry"); + let first = inline_image_bytes(&requests[0]); + let second = inline_image_bytes(&requests[1]); + assert!(first > 0, "the fixture must carry the image"); + assert!( + second > 0 && second < first, + "the retry must carry smaller image bytes ({second} < {first})" + ); + let notices = sink.messages(); + assert_eq!(notices.len(), 1, "one notice per downgrade: {notices:?}"); + assert!( + notices[0].contains("re-encoded") && notices[0].contains("413"), + "the notice must name the downgrade: {notices:?}" + ); + } + + #[tokio::test] + async fn request_body_413_after_shrinking_replaces_images_with_notes() { + let _environment = crate::test_support::lock_test_env(); + let messages = vec![ + msg("user", "context before the screenshots"), + user_image_message(&png_data_url(900, 900)), + ]; + let client = ScriptedSummaryClient::with_outcomes(vec![ + Err(request_body_413()), + Err(request_body_413()), + Ok(summary_content()), + ]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let result = compact_messages_safe(&client, &messages, None, &envelope, &mut usage).await; + assert!( + result.is_ok(), + "the retry after replacing images must succeed: {result:?}" + ); + + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 3, "reject, shrink retry, replace retry"); + let first = inline_image_bytes(&requests[0]); + let second = inline_image_bytes(&requests[1]); + let third = inline_image_bytes(&requests[2]); + assert!(second > 0 && second < first); + assert_eq!(third, 0, "the last rung carries no image bytes"); + let note_present = requests[2].messages.iter().any(|message| { + message.content.iter().any(|block| match block { + ContentBlock::Text { text, .. } => text.contains("omitted from this summary pass"), + _ => false, + }) + }); + assert!(note_present, "the summarizer is told what was there"); + let notices = sink.messages(); + assert_eq!(notices.len(), 2, "one notice per rung: {notices:?}"); + assert!(notices[1].contains("replaced"), "{notices:?}"); + } + + #[tokio::test] + async fn request_body_413_from_a_gateway_html_page_enters_the_same_ladder() { + let _environment = crate::test_support::lock_test_env(); + let messages = vec![ + msg("user", "context before the screenshots"), + user_image_message(&png_data_url(900, 900)), + ]; + let client = ScriptedSummaryClient::with_outcomes(vec![ + Err(request_body_413_html_gateway()), + Ok(summary_content()), + ]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let result = compact_messages_safe(&client, &messages, None, &envelope, &mut usage).await; + assert!( + result.is_ok(), + "the gateway-page rejection must enter the ladder: {result:?}" + ); + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 2, "one rejection, one retry"); + assert!( + inline_image_bytes(&requests[1]) < inline_image_bytes(&requests[0]), + "the retry must carry smaller image bytes" + ); + let notices = sink.messages(); + assert_eq!( + notices.len(), + 1, + "the user hears about the downgrade: {notices:?}" + ); + assert!(notices[0].contains("413"), "{notices:?}"); + } + + #[tokio::test] + async fn request_body_413_with_in_budget_images_skips_the_noop_retry_and_replaces() { + // The images fit their share of the 2 MiB budget, yet the endpoint + // refused the body: the cap sits below the budget. The ladder must not + // report "no images" (which would fail the pass outright) and must not + // resend identical bytes — it goes straight to the replace rung. + let _environment = crate::test_support::lock_test_env(); + let messages = vec![ + msg("user", "context before the screenshot"), + user_image_message(&png_data_url(64, 64)), + ]; + let client = ScriptedSummaryClient::with_outcomes(vec![ + Err(request_body_413()), + Ok(summary_content()), + ]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let result = compact_messages_safe(&client, &messages, None, &envelope, &mut usage).await; + assert!( + result.is_ok(), + "in-budget images must still let the ladder finish: {result:?}" + ); + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 2, "replace directly, no identical retry"); + assert!( + inline_image_bytes(&requests[0]) > 0, + "the fixture carried the image" + ); + assert_eq!( + inline_image_bytes(&requests[1]), + 0, + "the retry carries notes, not bytes" + ); + let notices = sink.messages(); + assert_eq!( + notices.len(), + 1, + "one notice for the replace rung: {notices:?}" + ); + assert!(notices[0].contains("replaced"), "{notices:?}"); + } + + #[tokio::test] + async fn request_body_413_without_inline_images_fails_with_context() { + let _environment = crate::test_support::lock_test_env(); + let messages = vec![msg("user", "no images anywhere in this history")]; + let client = ScriptedSummaryClient::with_outcomes(vec![Err(request_body_413())]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let error = compact_messages_safe(&client, &messages, None, &envelope, &mut usage) + .await + .expect_err("nothing left to shrink, the pass must fail"); + let text = format!("{error:#}"); + assert!( + text.contains("request-body limit"), + "the failure must name the boundary: {text}" + ); + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 1, "no pointless identical retry"); + assert!(sink.messages().is_empty(), "nothing was downgraded"); + } + + #[tokio::test] + async fn request_body_413_after_replacements_fails_with_the_full_ladder() { + let _environment = crate::test_support::lock_test_env(); + let messages = vec![ + msg("user", "context before the screenshots"), + user_image_message(&png_data_url(900, 900)), + ]; + let client = ScriptedSummaryClient::with_outcomes(vec![ + Err(request_body_413()), + Err(request_body_413()), + Err(request_body_413()), + ]); + let sink = std::sync::Arc::new(RecordingNoticeSink::default()); + let envelope = body_413_retry_envelope(sink.clone()); + let mut usage = Usage::default(); + + let error = compact_messages_safe(&client, &messages, None, &envelope, &mut usage) + .await + .expect_err("every rung refused, the pass must fail"); + let text = format!("{error:#}"); + assert!( + text.contains("even after re-encoding and then replacing"), + "the failure must report the full ladder: {text}" + ); + let requests = client.requests.lock().expect("requests").clone(); + assert_eq!(requests.len(), 3, "two retries, then stop"); + assert_eq!(sink.messages().len(), 2, "both rungs announced"); + } + struct ScriptedSummaryClient { responses: std::sync::Mutex>>>, requests: std::sync::Mutex>, diff --git a/crates/tui/src/core/engine/compaction.rs b/crates/tui/src/core/engine/compaction.rs index 3e0f15f33b..276958959e 100644 --- a/crates/tui/src/core/engine/compaction.rs +++ b/crates/tui/src/core/engine/compaction.rs @@ -11,6 +11,23 @@ pub(super) struct CompactionPass { pub usage: Usage, } +/// Engine-side sink for compaction downgrade notices. +/// +/// Delivered as `Event::Status`, the same channel other engine status lines +/// use, so a long recovery says what it is doing while it runs. A full event +/// channel drops the notice instead of stalling the pass; the same sentence +/// is already in the log via `logging::warn`. +#[derive(Debug)] +struct EngineCompactionNoticeSink { + tx: mpsc::Sender, +} + +impl crate::compaction::CompactionNoticeSink for EngineCompactionNoticeSink { + fn notice(&self, message: String) { + let _ = self.tx.try_send(Event::Status { message }); + } +} + impl Engine { pub(super) async fn emit_compaction_started( &mut self, @@ -183,6 +200,9 @@ impl Engine { .get_or_insert_with(|| self.config.workspace.clone()); let mut prepared = PreparedCompactionEnvelope::new(config); prepared.session_id = Some(self.session.id.clone()); + prepared.notice_sink = Some(std::sync::Arc::new(EngineCompactionNoticeSink { + tx: self.tx_event.clone(), + })); // The summary request must carry the reasoning tier the turn sends: // reasoning routes render it at the head of the prompt, so omitting // it forfeited the whole cached history prefix (#6540). @@ -709,3 +729,25 @@ pub(super) fn is_provider_rejection(err: &anyhow::Error) -> bool { | ErrorCategory::Timeout ) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::compaction::CompactionNoticeSink as _; + + /// The engine sink is the one link between a compaction downgrade and the + /// person watching: the notice must land on the status line, not only in + /// the log. + #[tokio::test] + async fn compaction_notice_sink_delivers_a_status_event() { + let (tx, mut rx) = mpsc::channel(4); + let sink = EngineCompactionNoticeSink { tx }; + sink.notice("Making room re-encoded 2 inline image(s)".to_string()); + match rx.recv().await { + Some(Event::Status { message }) => { + assert!(message.contains("re-encoded"), "{message}"); + } + other => panic!("expected a Status event, got {other:?}"), + } + } +} diff --git a/crates/tui/src/image_attach.rs b/crates/tui/src/image_attach.rs index d7e1918690..efa88da0a9 100644 --- a/crates/tui/src/image_attach.rs +++ b/crates/tui/src/image_attach.rs @@ -46,7 +46,10 @@ use anyhow::{Result, bail}; use codewhale_protocol::runtime::{ MAX_RUNTIME_IMAGE_BYTES, MAX_RUNTIME_IMAGE_TOTAL_BYTES, MAX_RUNTIME_IMAGES, RuntimeImageInput, }; -use image::{DynamicImage, ImageReader, Limits}; +use image::codecs::jpeg::JpegEncoder; +use image::codecs::png::{CompressionType, FilterType as PngFilter, PngEncoder}; +use image::imageops::FilterType; +use image::{DynamicImage, ExtendedColorType, GenericImageView, ImageEncoder, ImageReader, Limits}; use std::io::Cursor; use std::path::Path; @@ -260,7 +263,7 @@ impl std::fmt::Display for ImageAttachError { impl std::error::Error for ImageAttachError {} -fn human_bytes(bytes: usize) -> String { +pub(crate) fn human_bytes(bytes: usize) -> String { if bytes >= 1024 * 1024 { format!("{:.1} MB", bytes as f64 / (1024.0 * 1024.0)) } else if bytes >= 1024 { @@ -768,6 +771,355 @@ pub fn strip_images_when_unsupported( stripped } +/// Total inline-image byte budget for one compaction retry that follows a +/// provider body-size rejection. +/// +/// An HTTP 413 boundary caps the request *body*, and the token-side context +/// budget that governs compaction cannot see it: an image that costs a flat +/// token estimate can still spend megabytes of base64. Nothing consults this +/// budget on the happy path — it exists only to recover a summary call that +/// has already been refused, where the cheapest useful move is to re-encode +/// the inline images under a cap at or below the smallest provider body limits +/// seen in practice and retry. +pub(crate) const COMPACTION_IMAGE_TOTAL_BUDGET_BYTES: usize = 2 * 1024 * 1024; + +/// Smallest per-image share of that budget, so an image-heavy session still +/// re-encodes each image instead of dividing the budget down to nothing. +const COMPACTION_IMAGE_MIN_BUDGET_BYTES: usize = 96 * 1024; + +/// Longest edge a re-encoded inline image keeps, in pixels. +const COMPACTION_IMAGE_MAX_EDGE: u32 = 1024; + +/// Longest edge the shrink ladder descends to before giving up on an image. +const COMPACTION_IMAGE_MIN_EDGE: u32 = 128; + +/// JPEG quality for re-encoded images that carry no meaningful alpha. +const COMPACTION_IMAGE_JPEG_QUALITY: u8 = 80; + +/// What one inline-image shrink pass changed. +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ShrunkInlineImages { + /// Images that were re-encoded smaller. + pub images: usize, + /// Inline images the request carried, rewritten or not. + /// + /// `images == 0` alone cannot tell "nothing to do because every image + /// already fits" from "there were no images"; a request-size ladder must + /// not conflate the two, because only the second makes the next rung + /// pointless. + pub images_seen: usize, + /// Decoded bytes of those images before the pass. + pub bytes_before: usize, + /// Decoded bytes of those images after the pass. + pub bytes_after: usize, +} + +/// Re-encode every inline image in `messages` under a total byte budget. +/// +/// Covers both carriers an outbound request can hold: an `image_url` block, +/// and the stored tool-result shape (`{"type":"image","mime_type","data"}`) +/// that the wire projection reads back out via +/// [`provider_tool_result_image_refs`]. +/// +/// `images == 0` means nothing was rewritten — either there were no inline +/// images, or each was already under its share of the budget. Check +/// [`ShrunkInlineImages::images_seen`] to tell those apart: a caller that is +/// still looking at a body-size rejection must climb to the next rung rather +/// than resend the same bytes, and when images are present but nothing was +/// rewritten, the next rung is the only one that can still change the payload. +pub(crate) fn shrink_images_for_request( + messages: &mut [codewhale_models::Message], +) -> ShrunkInlineImages { + shrink_images_for_request_with_budget(messages, COMPACTION_IMAGE_TOTAL_BUDGET_BYTES) +} + +/// Budget-parameterized core of [`shrink_images_for_request`], kept separate +/// so tests can drive it with small budgets and small images. +pub(crate) fn shrink_images_for_request_with_budget( + messages: &mut [codewhale_models::Message], + budget: usize, +) -> ShrunkInlineImages { + let sizes = inline_image_sizes(messages); + let total: usize = sizes.iter().sum(); + let images_seen = sizes.len(); + if total <= budget { + // Images may be present and simply already fit: report them as seen + // without rewriting, so a caller can tell "nothing to shrink" from + // "nothing there". + return ShrunkInlineImages { + images_seen, + ..ShrunkInlineImages::default() + }; + } + // A share of the budget per image, floored so a few large images still come + // back readable. With many images the floor can push the total past + // `budget`; the caller's next rung (replace with notes) covers that case, + // not a tighter share here. + let per_image = (budget / sizes.len()).max(COMPACTION_IMAGE_MIN_BUDGET_BYTES); + let mut outcome = ShrunkInlineImages::default(); + for message in messages.iter_mut() { + for block in &mut message.content { + match block { + ContentBlock::ImageUrl { image_url } => { + if let Some((url, shrunk)) = shrink_data_url(&image_url.url, per_image) { + image_url.url = url; + outcome.images += 1; + outcome.bytes_before += shrunk.before; + outcome.bytes_after += shrunk.after; + } + } + ContentBlock::ToolResult { content_blocks, .. } => { + let Some(blocks) = content_blocks.as_mut() else { + continue; + }; + for value in blocks.iter_mut() { + if let Some(shrunk) = shrink_stored_tool_image(value, per_image) { + outcome.images += 1; + outcome.bytes_before += shrunk.before; + outcome.bytes_after += shrunk.after; + } + } + } + _ => {} + } + } + } + outcome.images_seen = images_seen; + outcome +} + +/// Replace every inline image with a text note that keeps the image's +/// existence visible without its bytes. +/// +/// This is the last rung of the request-size ladder. A summary pass that still +/// exceeds the provider's body cap after re-encoding has nothing left to give +/// but the pixels; the note keeps the fact that a tool or the user supplied an +/// image — and how large it was — so the handoff can still say so instead of +/// silently dropping the detail. Session history keeps the real image; this +/// only rewrites the outbound copy. +/// +/// Returns the number of images replaced. +pub(crate) fn replace_images_with_placeholders( + messages: &mut [codewhale_models::Message], + reason: &str, +) -> usize { + let mut replaced = 0; + for message in messages.iter_mut() { + for block in &mut message.content { + match block { + ContentBlock::ImageUrl { image_url } => { + let bytes = parse_data_url(&image_url.url) + .map(|(_, payload)| decoded_len_estimate(payload)); + *block = ContentBlock::Text { + text: image_placeholder_note(1, bytes, reason), + cache_control: None, + }; + replaced += 1; + } + ContentBlock::ToolResult { + content, + content_blocks, + .. + } => { + let Some(blocks) = content_blocks.as_ref() else { + continue; + }; + let mut count = 0usize; + let mut bytes = 0usize; + for value in blocks { + if let Some((_, payload)) = stored_tool_image_payload(value) { + count += 1; + bytes += decoded_len_estimate(payload); + } + } + if count == 0 { + continue; + } + // Same shape as `strip_images_when_unsupported`: the + // wire projection reads tool images back out of + // `content_blocks` and would count a text block placed + // there as an omitted image. + *content_blocks = None; + *content = format!( + "{content}\n{}", + image_placeholder_note(count, Some(bytes), reason) + ); + replaced += count; + } + _ => {} + } + } + } + replaced +} + +/// The in-band note that replaces an inline image byte payload for one +/// summary pass. It tells the summarizer what was there and what to do +/// about it (refer to it as an image; never invent its contents), rather than +/// leaving a silent gap. +fn image_placeholder_note(count: usize, bytes: Option, reason: &str) -> String { + let size = bytes.map_or(String::new(), |bytes| format!(" (~{})", human_bytes(bytes))); + format!( + "[{count} image(s){size} omitted from this summary pass: {reason}. \ + The image(s) remain in the session. Refer to them only as images the \ + conversation included; do not describe or guess what they showed.]" + ) +} + +/// Base64 length to decoded length: four characters carry three bytes. +fn decoded_len_estimate(payload: &str) -> usize { + payload.len() / 4 * 3 +} + +/// Decoded-size estimates for every inline image, in request order. +fn inline_image_sizes(messages: &[codewhale_models::Message]) -> Vec { + let mut sizes = Vec::new(); + for message in messages { + for block in &message.content { + match block { + ContentBlock::ImageUrl { image_url } => { + if let Some((_, payload)) = parse_data_url(&image_url.url) { + sizes.push(decoded_len_estimate(payload)); + } + } + ContentBlock::ToolResult { content_blocks, .. } => { + if let Some(blocks) = content_blocks.as_ref() { + for value in blocks { + if let Some((_, payload)) = stored_tool_image_payload(value) { + sizes.push(decoded_len_estimate(payload)); + } + } + } + } + _ => {} + } + } + } + sizes +} + +/// The `(mime_type, data)` pair of a stored tool-result image block, if the +/// block is one. +fn stored_tool_image_payload(value: &serde_json::Value) -> Option<(&str, &str)> { + if value.get("type").and_then(serde_json::Value::as_str) != Some("image") { + return None; + } + let mime_type = value.get("mime_type").and_then(serde_json::Value::as_str)?; + let data = value.get("data").and_then(serde_json::Value::as_str)?; + Some((mime_type, data)) +} + +/// One rewritten image, in decoded bytes. +#[derive(Debug, Clone, Copy)] +struct ShrunkPayload { + before: usize, + after: usize, +} + +/// Re-encode a `data:` URL image under `budget`, returning the replacement +/// URL plus the byte delta. +fn shrink_data_url(url: &str, budget: usize) -> Option<(String, ShrunkPayload)> { + let (_, payload) = parse_data_url(url)?; + let (mime, data, shrunk) = shrink_base64_image(payload, budget)?; + Some((format!("data:{mime};base64,{data}"), shrunk)) +} + +/// Re-encode one stored tool-result image block in place. +fn shrink_stored_tool_image(value: &mut serde_json::Value, budget: usize) -> Option { + let (_, payload) = stored_tool_image_payload(value)?; + let (mime, data, shrunk) = shrink_base64_image(payload, budget)?; + let object = value.as_object_mut()?; + object.insert("mime_type".to_string(), serde_json::Value::String(mime)); + object.insert("data".to_string(), serde_json::Value::String(data)); + Some(shrunk) +} + +/// Re-encode one base64 inline image to fit `budget` decoded bytes. +/// +/// `None` means the image already fits, cannot be decoded, or the re-encode +/// came back no smaller — so a caller's "did anything change" tally stays +/// honest about what a retry would actually send. +fn shrink_base64_image(data: &str, budget: usize) -> Option<(String, String, ShrunkPayload)> { + let bytes = STANDARD.decode(data).ok()?; + if bytes.len() <= budget { + return None; + } + let (decoded, _, _) = decode_and_guard_image(&bytes).ok()?; + let (mime, encoded) = reencode_within_budget(decoded, budget)?; + if encoded.len() >= bytes.len() { + return None; + } + Some(( + mime.to_string(), + STANDARD.encode(&encoded), + ShrunkPayload { + before: bytes.len(), + after: encoded.len(), + }, + )) +} + +/// Encode `image` down a short ladder until it fits `budget`. +/// +/// Rung order: longest edge capped at [`COMPACTION_IMAGE_MAX_EDGE`], then +/// halved until [`COMPACTION_IMAGE_MIN_EDGE`]. Alpha-bearing images stay PNG +/// (JPEG would flatten transparency); everything else becomes JPEG, which is +/// what actually makes screenshots and artwork small. The smallest rung wins +/// even when nothing fits `budget`, because a smaller-than-before payload is +/// still progress for the retry. +fn reencode_within_budget(image: DynamicImage, budget: usize) -> Option<(&'static str, Vec)> { + let mut current = image; + let (mut width, mut height) = current.dimensions(); + if width.max(height) > COMPACTION_IMAGE_MAX_EDGE { + let scale = f64::from(COMPACTION_IMAGE_MAX_EDGE) / f64::from(width.max(height)); + let scaled_width = ((f64::from(width) * scale).round() as u32).max(1); + let scaled_height = ((f64::from(height) * scale).round() as u32).max(1); + current = current.resize(scaled_width, scaled_height, FilterType::Lanczos3); + (width, height) = current.dimensions(); + } + let mut smallest: Option<(&'static str, Vec)> = None; + loop { + if let Some(candidate) = encode_inline_image(¤t) + && smallest + .as_ref() + .is_none_or(|(_, best)| candidate.1.len() < best.len()) + { + smallest = Some(candidate); + } + if smallest + .as_ref() + .is_some_and(|(_, bytes)| bytes.len() <= budget) + { + break; + } + if width.max(height) <= COMPACTION_IMAGE_MIN_EDGE { + break; + } + (width, height) = ((width / 2).max(1), (height / 2).max(1)); + current = current.resize(width, height, FilterType::Lanczos3); + } + smallest +} + +/// One encoder pass: PNG when alpha matters, JPEG otherwise. +fn encode_inline_image(image: &DynamicImage) -> Option<(&'static str, Vec)> { + let (width, height) = image.dimensions(); + let mut bytes = Vec::new(); + if image.color().has_alpha() { + let rgba = image.to_rgba8(); + PngEncoder::new_with_quality(&mut bytes, CompressionType::Best, PngFilter::Adaptive) + .write_image(rgba.as_raw(), width, height, ExtendedColorType::Rgba8) + .ok()?; + Some(("image/png", bytes)) + } else { + let rgb = image.to_rgb8(); + JpegEncoder::new_with_quality(&mut bytes, COMPACTION_IMAGE_JPEG_QUALITY) + .write_image(rgb.as_raw(), width, height, ExtendedColorType::Rgb8) + .ok()?; + Some(("image/jpeg", bytes)) + } +} + /// Render dropped-attachment notices as a block the model will read. /// /// Wrapped in a tag rather than appended as bare prose so the model can tell diff --git a/crates/tui/src/image_attach/tests.rs b/crates/tui/src/image_attach/tests.rs index b6b8df7f17..2944dc332b 100644 --- a/crates/tui/src/image_attach/tests.rs +++ b/crates/tui/src/image_attach/tests.rs @@ -26,6 +26,140 @@ fn sniffs_every_accepted_format_from_magic_bytes() { ); } +/// Noise defeats compression, so the shrink ladder has to re-encode for real. +fn noise_payload(width: u32, height: u32) -> Vec { + use image::ImageEncoder as _; + let mut pixels = image::RgbImage::new(width, height); + for (x, y, pixel) in pixels.enumerate_pixels_mut() { + *pixel = image::Rgb([ + (x.wrapping_mul(31) ^ y.wrapping_mul(17)) as u8, + (x.wrapping_mul(7) ^ y.wrapping_mul(29)) as u8, + (x.wrapping_add(y).wrapping_mul(13)) as u8, + ]); + } + let mut bytes = Vec::new(); + image::codecs::png::PngEncoder::new_with_quality( + &mut bytes, + image::codecs::png::CompressionType::Fast, + image::codecs::png::FilterType::NoFilter, + ) + .write_image( + pixels.as_raw(), + width, + height, + image::ExtendedColorType::Rgb8, + ) + .expect("encode fixture png"); + bytes +} + +fn image_blocks_fixture() -> Vec { + let payload = STANDARD.encode(noise_payload(320, 320)); + vec![ + codewhale_models::Message { + role: Role::User, + content: vec![ + ContentBlock::Text { + text: "look at this".to_string(), + cache_control: None, + }, + ContentBlock::ImageUrl { + image_url: ImageUrlContent { + url: format!("data:image/png;base64,{payload}"), + }, + }, + ], + }, + codewhale_models::Message { + role: Role::User, + content: vec![ContentBlock::ToolResult { + tool_use_id: "call_1".to_string(), + content: "Read image file [image/png]".to_string(), + is_error: None, + content_blocks: Some(vec![serde_json::json!({ + "type": "image", + "mime_type": "image/png", + "data": payload, + })]), + }], + }, + ] +} + +#[test] +fn compaction_shrink_rewrites_both_image_carriers_under_budget() { + let budget = 64 * 1024; + let mut messages = image_blocks_fixture(); + let outcome = shrink_images_for_request_with_budget(&mut messages, budget); + assert_eq!(outcome.images, 2, "both carriers are rewritten"); + assert!(outcome.bytes_after < outcome.bytes_before); + + let ContentBlock::ImageUrl { image_url } = &messages[0].content[1] else { + panic!("image block survives the shrink"); + }; + let (mime, payload) = parse_data_url(&image_url.url).expect("data url"); + assert!(matches!(mime, "image/png" | "image/jpeg"), "{mime}"); + let bytes = STANDARD.decode(payload).expect("base64"); + assert!(bytes.len() <= budget, "{} <= {budget}", bytes.len()); + assert_eq!(sniff_media_type(&bytes), Some(mime)); + + let ContentBlock::ToolResult { content_blocks, .. } = &messages[1].content[0] else { + panic!("tool result survives the shrink"); + }; + let block = &content_blocks.as_ref().expect("blocks")[0]; + assert_eq!(block["type"], "image"); + let nested = STANDARD + .decode(block["data"].as_str().expect("data")) + .expect("nested base64"); + assert!(nested.len() <= budget); + assert_eq!( + sniff_media_type(&nested), + block["mime_type"].as_str(), + "the mime must match the rewritten bytes" + ); +} + +#[test] +fn compaction_shrink_leaves_an_under_budget_history_untouched() { + let mut messages = image_blocks_fixture(); + let before = messages.clone(); + let outcome = shrink_images_for_request_with_budget(&mut messages, 32 * 1024 * 1024); + assert_eq!(outcome.images, 0, "a fitting request is never rewritten"); + assert_eq!( + outcome.images_seen, 2, + "the request still carried images, and the ladder must be able to tell" + ); + assert_eq!(messages, before); +} + +#[test] +fn compaction_placeholder_keeps_the_image_visible_as_text() { + let mut messages = image_blocks_fixture(); + let replaced = replace_images_with_placeholders( + &mut messages, + "the summary request exceeded the provider's request-body limit (HTTP 413)", + ); + assert_eq!(replaced, 2); + + let ContentBlock::Text { text, .. } = &messages[0].content[1] else { + panic!("image_url becomes a note"); + }; + assert!(text.contains("omitted from this summary pass"), "{text}"); + assert!(text.contains("do not describe"), "{text}"); + + let ContentBlock::ToolResult { + content, + content_blocks, + .. + } = &messages[1].content[0] + else { + panic!("tool result survives"); + }; + assert!(content_blocks.is_none(), "no base64 may remain"); + assert!(content.contains("1 image(s)"), "{content}"); + assert!(content.contains("Read image file [image/png]"), "{content}"); +} + #[test] fn sniffing_ignores_the_extension_and_believes_the_bytes() { // A JPEG named .png must be declared image/jpeg, or the provider diff --git a/docs/CONTRIBUTORS.md b/docs/CONTRIBUTORS.md index 1f0c4ae70f..8149f3f0c5 100644 --- a/docs/CONTRIBUTORS.md +++ b/docs/CONTRIBUTORS.md @@ -37,6 +37,7 @@ notes, and relevant issue/PR comments. - **[aboimpinto](https://github.com/aboimpinto)** — restored a green Linux full-workspace test gate without loosening any test, twice ([#6581](https://github.com/Hmbown/Codewhale/pull/6581), [#6666](https://github.com/Hmbown/Codewhale/pull/6666)). - **[dajiaohuang](https://github.com/dajiaohuang)** — validated `config set` values against the settings schema ([#6568](https://github.com/Hmbown/Codewhale/pull/6568)). - **[Water-Run](https://github.com/Water-Run)** — ingested namespaced model-only catalog entries so models present only in the canonical `models` map reach the offering list ([#6400](https://github.com/Hmbown/Codewhale/pull/6400)), and retired the blanket dead-code allowance and its unused feature stages, tightening the budget to match ([#6402](https://github.com/Hmbown/Codewhale/pull/6402)). +- **[Sh1Zuku / SparkofSpike](https://github.com/SparkofSpike)** — let making room survive a provider request-body limit (HTTP 413) by shrinking, then replacing, inline images for that one summary pass ([#6642](https://github.com/Hmbown/Codewhale/pull/6642)). **Reports and reproductions** diff --git a/docs/public-surface-facts.json b/docs/public-surface-facts.json index 27529840a7..87eae1d470 100644 --- a/docs/public-surface-facts.json +++ b/docs/public-surface-facts.json @@ -233,6 +233,7 @@ "@dajiaohuang", "@Water-Run", "@BX166", + "@SparkofSpike", "@cenab" ] }, diff --git a/web/lib/release-credits.ts b/web/lib/release-credits.ts index de178d7192..7a911cfe8b 100644 --- a/web/lib/release-credits.ts +++ b/web/lib/release-credits.ts @@ -25,6 +25,7 @@ export const RELEASE_CONTRIBUTORS: string[] = [ "@aboimpinto", "@dajiaohuang", "@Water-Run", + "@SparkofSpike", ]; /**