diff --git a/CLAUDE.md b/CLAUDE.md index 0bac406..0177473 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -900,9 +900,18 @@ endpoint. a deploy-time decision, not a request-time one). - `hybrid → hybrid` allowed. - `hybrid → local` allowed (the inlined description replays as text). -- `local → hybrid` rejected — the stored transcript has raw images in the - first user message and OpenRouter providers don't accept that shape - consistently. Regenerate the insight in hybrid mode instead. +- `local → hybrid` allowed. The stored transcript carries the raw photo in + its first user message, so each hybrid turn swaps it for a local vision + description (cached per photo) in the copy sent to OpenRouter, then puts + the photo back before saving (`InsightChatService::inline_images_as_text`). + If the local model can't describe it, the turn fails. A stored model id is + only reused on the same backend (`continuation_model`). + +**Photos never go to OpenRouter.** `OpenRouterClient` refuses any request +carrying image data (`IMAGES_STAY_LOCAL`) and its `describe_image` always +errors — the client is the choke point, so a caller bug fails the turn +instead of uploading a photo. Image work in hybrid mode goes through +`ResolvedBackend::local()`. **Insight Chat Continuation:** @@ -933,9 +942,9 @@ clients whether chat is available for a given insight. Backend routing rules (matches agentic-insight generation): - Stored `backend` on the insight row is authoritative by default. -- `request.backend` may override per-turn. `local -> hybrid` is rejected in - v1 (would require on-the-fly visual-description rewrite); `hybrid -> local` - replays verbatim since the description is already inlined as text. +- `request.backend` may override per-turn, in either direction (see the + cross-replay matrix above); `hybrid -> local` replays verbatim since the + description is already inlined as text. - `request.model` overrides the chat model (an Ollama id in local mode, an OpenRouter id in hybrid mode). diff --git a/src/ai/insight_chat.rs b/src/ai/insight_chat.rs index 2cad73b..1cc329e 100644 --- a/src/ai/insight_chat.rs +++ b/src/ai/insight_chat.rs @@ -119,8 +119,16 @@ pub struct InsightChatService { generator: Arc, insight_dao: Arc>>, chat_locks: ChatLockMap, + /// Local vision descriptions of transcript images, keyed by a hash of + /// the image data, so a local-generated chat continued on a text-only + /// (hybrid) model describes its photo once rather than every turn. + image_descriptions: Arc>>, } +/// Cap on cached image descriptions; the cache is cleared when it fills. +/// One entry per photo chatted about since the server started. +const IMAGE_DESCRIPTION_CACHE_MAX: usize = 256; + impl InsightChatService { pub fn new( generator: Arc, @@ -131,9 +139,62 @@ impl InsightChatService { generator, insight_dao, chat_locks, + image_descriptions: Arc::new(Mutex::new(HashMap::new())), } } + /// Prepare a replayed transcript for a backend that can't take images. + /// + /// A chat generated on the local backend carries the photo itself in its + /// first user message. Continuing it on hybrid would send that photo to + /// OpenRouter — which the OpenRouter client refuses — so each image is + /// described by the local vision model and the text takes its place in + /// the copy sent to the model. The returned stash puts the images back + /// before the transcript is saved, so the stored chat keeps its photo and + /// a later local turn still sees it. + /// + /// A no-op for backends that take images inline. If the local model + /// can't describe an image the turn fails: going ahead without the + /// photo would quietly change what the chat is about. + async fn inline_images_as_text( + &self, + backend: &ResolvedBackend, + messages: &mut [ChatMessage], + ) -> Result { + if backend.images_inline { + return Ok(ImageStash::default()); + } + let mut descriptions: HashMap = HashMap::new(); + for image in transcript_images(messages) { + let key = image_cache_key(&image); + let cached = self + .image_descriptions + .lock() + .ok() + .and_then(|c| c.get(&key).cloned()); + let description = match cached { + Some(d) => d, + None => { + let d = backend.local().describe_image(&image).await.map_err(|e| { + anyhow!( + "couldn't describe the photo with the local vision model, so this \ + chat can't continue on a text-only model: {e}" + ) + })?; + if let Ok(mut cache) = self.image_descriptions.lock() { + if cache.len() >= IMAGE_DESCRIPTION_CACHE_MAX { + cache.clear(); + } + cache.insert(key, d.clone()); + } + d + } + }; + descriptions.insert(image, description); + } + Ok(apply_image_descriptions(messages, &descriptions)) + } + /// Accessor for the insight DAO (used by async job completion). pub fn insight_dao(&self) -> &Arc>> { &self.insight_dao @@ -285,11 +346,12 @@ impl InsightChatService { let stored_model = insight.model_version.clone(); let overrides = SamplingOverrides { - model: req - .model - .clone() - .or_else(|| Some(stored_model.clone())) - .filter(|m| !m.is_empty()), + model: continuation_model( + req.model.clone(), + &stored_model, + &stored_backend, + kind.as_str(), + ), num_ctx: req.num_ctx, temperature: req.temperature, top_p: req.top_p, @@ -298,6 +360,9 @@ impl InsightChatService { enable_thinking: req.enable_thinking, }; let backend = self.generator.resolve_backend(kind, &overrides).await?; + // Text-only backend (hybrid) on a transcript holding the photo: + // swap it for a local description. Restored before saving. + let image_stash = self.inline_images_as_text(&backend, &mut messages).await?; let model_used = backend.model().to_string(); span.set_attribute("model", model_used.clone()); @@ -449,6 +514,8 @@ impl InsightChatService { // Drop the per-turn iteration-budget note from the system message // before we persist so it doesn't snowball on each subsequent turn. restore_system_content(&mut messages, original_system_content); + // Put the photo back so the stored transcript keeps it. + image_stash.restore(&mut messages); // Append mode: undo the per-turn system-prompt override so the // stored transcript keeps the original baked persona. Amend mode: @@ -979,11 +1046,12 @@ impl InsightChatService { let stored_model = insight.model_version.clone(); let overrides = SamplingOverrides { - model: req - .model - .clone() - .or_else(|| Some(stored_model.clone())) - .filter(|m| !m.is_empty()), + model: continuation_model( + req.model.clone(), + &stored_model, + &stored_backend, + kind.as_str(), + ), num_ctx: req.num_ctx, temperature: req.temperature, top_p: req.top_p, @@ -992,6 +1060,9 @@ impl InsightChatService { enable_thinking: req.enable_thinking, }; let backend = self.generator.resolve_backend(kind, &overrides).await?; + // Text-only backend (hybrid) on a transcript holding the photo: + // swap it for a local description. Restored before saving. + let image_stash = self.inline_images_as_text(&backend, &mut messages).await?; let model_used = backend.model().to_string(); // llm.* attributes on the turn-level span (issue #130): clients can't // know local-vs-hybrid, but the turn knows its resolved backend. @@ -1064,6 +1135,8 @@ impl InsightChatService { } restore_system_content(&mut messages, original_system_content); + // Put the photo back so the stored transcript keeps it. + image_stash.restore(&mut messages); if !req.amend { restore_system_prompt_override(&mut messages, override_stash); @@ -1455,11 +1528,12 @@ impl InsightChatService { let stored_model = insight.model_version.clone(); let overrides = SamplingOverrides { - model: req - .model - .clone() - .or_else(|| Some(stored_model.clone())) - .filter(|m| !m.is_empty()), + model: continuation_model( + req.model.clone(), + &stored_model, + &stored_backend, + kind.as_str(), + ), num_ctx: req.num_ctx, temperature: req.temperature, top_p: req.top_p, @@ -1468,6 +1542,9 @@ impl InsightChatService { enable_thinking: req.enable_thinking, }; let backend = self.generator.resolve_backend(kind, &overrides).await?; + // Text-only backend (hybrid) on a transcript holding the photo: + // swap it for a local description. Restored before saving. + let image_stash = self.inline_images_as_text(&backend, &mut messages).await?; let model_used = backend.model().to_string(); // Tool set — images_inline mode + first user turn carries an image → @@ -1533,6 +1610,8 @@ impl InsightChatService { // Drop the per-turn iteration-budget note before persisting so it // doesn't snowball on subsequent turns. restore_system_content(&mut messages, original_system_content); + // Put the photo back so the stored transcript keeps it. + image_stash.restore(&mut messages); // Append mode: undo the per-turn system-prompt override. // Amend mode: keep it — it becomes the new row's system message. @@ -2071,34 +2150,122 @@ fn resolve_date_taken_for_context( }) } -/// Validate a stored→effective backend transition for a chat continuation. -/// Continuation runs against a transcript that was generated with a specific -/// backend; the only blocked transition is `local → hybrid`, because the -/// stored transcript has images embedded in the first user message and the -/// hybrid path (OpenRouter chat with describe-then-inline) can't replay -/// raw image bytes through OpenRouter consistently across providers. -/// `hybrid → local` is allowed (the inlined description replays verbatim -/// as text). +/// Validate the backend a chat continuation is switching to. +/// +/// Every transition is allowed now. `local → hybrid` used to be refused +/// because a local transcript carries the raw photo; the turn now replaces +/// it with a local vision description before anything reaches OpenRouter +/// (`InsightChatService::inline_images_as_text`), and the OpenRouter client +/// refuses image data outright as a backstop. `hybrid → local` replays the +/// inlined description as text. `stored` is kept for the call sites' +/// symmetry and future rules. /// /// Whether "local" routes through Ollama or llama-swap is decided at /// startup by `LLM_BACKEND`; both share the same transcript shape from /// the chat-replay perspective. -fn validate_cross_replay(stored: &str, effective: &str) -> Result<()> { +fn validate_cross_replay(_stored: &str, effective: &str) -> Result<()> { if !matches!(effective, "local" | "hybrid") { bail!( "unknown backend '{}'; expected 'local' or 'hybrid'", effective ); } - if stored == "local" && effective == "hybrid" { - bail!( - "switching from local to hybrid mid-chat isn't supported; \ - regenerate the insight in hybrid mode if you want OpenRouter chat" - ); - } Ok(()) } +/// The model to continue a chat with when the request names none: the one +/// the insight was generated with, but only on the same backend. After a +/// switch, the stored id belongs to the other backend (a llama-swap slot is +/// not an OpenRouter model), so the new backend's default is used instead. +fn continuation_model( + requested: Option, + stored_model: &str, + stored_backend: &str, + effective_backend: &str, +) -> Option { + requested + .or_else(|| (stored_backend == effective_backend).then(|| stored_model.to_string())) + .filter(|m| !m.is_empty()) +} + +/// Every distinct image in a transcript, in order of first appearance. +fn transcript_images(messages: &[ChatMessage]) -> Vec { + let mut seen = std::collections::HashSet::new(); + messages + .iter() + .flat_map(|m| m.images.iter().flatten()) + .filter(|img| seen.insert((*img).clone())) + .cloned() + .collect() +} + +fn image_cache_key(image: &str) -> u64 { + use std::hash::{Hash, Hasher}; + let mut h = std::collections::hash_map::DefaultHasher::new(); + image.hash(&mut h); + h.finish() +} + +/// What `apply_image_descriptions` took out of a transcript, so it can be +/// put back before the transcript is saved. +#[derive(Debug, Default)] +struct ImageStash { + entries: Vec, +} + +#[derive(Debug)] +struct StashedImages { + index: usize, + original_content: String, + rewritten_content: String, + images: Option>, +} + +impl ImageStash { + /// Put the original content and images back. Each entry is matched by + /// index *and* by still carrying the rewritten text, so a turn that + /// reshaped the transcript can't get the wrong message restored. + fn restore(self, messages: &mut [ChatMessage]) { + for e in self.entries { + if let Some(m) = messages.get_mut(e.index) + && m.content == e.rewritten_content + { + m.content = e.original_content; + m.images = e.images; + } + } + } +} + +/// Replace each image in `messages` with its description, appended to the +/// message text. Returns what was taken out, for `ImageStash::restore`. +fn apply_image_descriptions( + messages: &mut [ChatMessage], + descriptions: &HashMap, +) -> ImageStash { + let mut stash = ImageStash::default(); + for (idx, m) in messages.iter_mut().enumerate() { + let Some(images) = m.images.as_ref().filter(|i| !i.is_empty()) else { + continue; + }; + let mut content = m.content.clone(); + for img in images { + if let Some(desc) = descriptions.get(img) { + content.push_str(&format!( + "\n\nVisual description of the photo (from the local vision model):\n{desc}" + )); + } + } + stash.entries.push(StashedImages { + index: idx, + original_content: std::mem::replace(&mut m.content, content.clone()), + rewritten_content: content, + images: m.images.take(), + }); + } + stash +} + /// Pick the backend label for bootstrap. Bootstrap has no stored insight /// to defer to (that's continuation's behaviour), so the default is /// `"local"`. Returns an error if the supplied label is non-empty but @@ -3672,17 +3839,90 @@ mod tests { } #[test] - fn cross_replay_rejects_local_to_hybrid() { - let err = validate_cross_replay("local", "hybrid").unwrap_err(); - assert!(format!("{}", err).contains("local to hybrid")); - } - - #[test] - fn cross_replay_allows_supported_transitions() { + fn cross_replay_allows_every_known_transition() { assert!(validate_cross_replay("local", "local").is_ok()); assert!(validate_cross_replay("hybrid", "hybrid").is_ok()); // Hybrid → local replays the inlined description as plain text. assert!(validate_cross_replay("hybrid", "local").is_ok()); + // Local → hybrid swaps the photo for a local description first. + assert!(validate_cross_replay("local", "hybrid").is_ok()); + } + + #[test] + fn continuation_model_drops_the_stored_model_across_a_switch() { + // Same backend: keep the model the insight was generated with. + assert_eq!( + continuation_model(None, "gemma4-31", "local", "local").as_deref(), + Some("gemma4-31") + ); + // Switched: a llama-swap slot name is not an OpenRouter model id. + assert_eq!( + continuation_model(None, "gemma4-31", "local", "hybrid"), + None + ); + // An explicit request always wins. + assert_eq!( + continuation_model(Some("x/y".into()), "gemma4-31", "local", "hybrid").as_deref(), + Some("x/y") + ); + assert_eq!(continuation_model(None, "", "local", "local"), None); + } + + fn with_image(text: &str, img: &str) -> ChatMessage { + let mut m = ChatMessage::user(text.to_string()); + m.images = Some(vec![img.to_string()]); + m + } + + #[test] + fn transcript_images_lists_each_image_once() { + let msgs = vec![ + ChatMessage::system("sys"), + with_image("describe", "IMG_A"), + ChatMessage::user("more"), + with_image("again", "IMG_A"), + ]; + assert_eq!(transcript_images(&msgs), vec!["IMG_A".to_string()]); + } + + #[test] + fn photo_is_swapped_for_text_then_restored_exactly() { + let original = vec![ + ChatMessage::system("sys"), + with_image("What's in this photo?", "IMG_A"), + assistant_text("A beach."), + ChatMessage::user("who's there?"), + ]; + let mut msgs = original.clone(); + let descriptions = + HashMap::from([("IMG_A".to_string(), "Two people on sand.".to_string())]); + + let stash = apply_image_descriptions(&mut msgs, &descriptions); + // Nothing the text-only model receives carries image data. + assert!(msgs.iter().all(|m| m.images.is_none())); + assert!(msgs[1].content.starts_with("What's in this photo?")); + assert!(msgs[1].content.contains("Two people on sand.")); + + // The turn appends its exchange, then the stash is restored before + // saving: the stored transcript keeps the photo, unchanged. + msgs.push(assistant_text("Two friends.")); + stash.restore(&mut msgs); + assert_eq!( + serde_json::to_string(&msgs[..original.len()]).unwrap(), + serde_json::to_string(&original).unwrap() + ); + assert_eq!(msgs[1].images, Some(vec!["IMG_A".to_string()])); + } + + #[test] + fn restore_leaves_a_message_the_turn_changed() { + let mut msgs = vec![with_image("q", "IMG_A")]; + let descriptions = HashMap::from([("IMG_A".to_string(), "desc".to_string())]); + let stash = apply_image_descriptions(&mut msgs, &descriptions); + msgs[0].content = "something else entirely".into(); + stash.restore(&mut msgs); + assert_eq!(msgs[0].content, "something else entirely"); + assert!(msgs[0].images.is_none()); } #[test]