feat(insight-chat): allow switching a local photo chat to hybrid
A local insight's transcript carries the photo itself, so continuing on hybrid was refused. Each hybrid turn now has the local vision model describe the photo (cached per image), sends the description in its place, and puts the photo back before saving, so the stored chat and later local turns keep it. If the local description fails, the turn fails. The stored model id is only reused on the same backend, so a llama-swap slot isn't sent to OpenRouter. Checked end to end against a recording fake OpenRouter on all three continuation endpoints: no image data left, the stored transcript kept the photo, and switching back to local saw the photo again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
cc11884272
commit
ba39628a0d
2 files changed
+292
-43
No files matched your search
@@ -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).
|
||||
|
||||
|
||||
+277
-37
@@ -119,8 +119,16 @@ pub struct InsightChatService {
|
||||
generator: Arc<InsightGenerator>,
|
||||
insight_dao: Arc<Mutex<Box<dyn InsightDao>>>,
|
||||
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<Mutex<HashMap<u64, String>>>,
|
||||
}
|
||||
|
||||
/// 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<InsightGenerator>,
|
||||
@@ -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<ImageStash> {
|
||||
if backend.images_inline {
|
||||
return Ok(ImageStash::default());
|
||||
}
|
||||
let mut descriptions: HashMap<String, String> = 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<Mutex<Box<dyn InsightDao>>> {
|
||||
&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<String>,
|
||||
stored_model: &str,
|
||||
stored_backend: &str,
|
||||
effective_backend: &str,
|
||||
) -> Option<String> {
|
||||
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<String> {
|
||||
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<StashedImages>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
struct StashedImages {
|
||||
index: usize,
|
||||
original_content: String,
|
||||
rewritten_content: String,
|
||||
images: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
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<String, String>,
|
||||
) -> 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]
|
||||
|
||||
Reference in new issue
Block a user