diff --git a/Cargo.lock b/Cargo.lock index 15b9c82afe..df4abb5378 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7308,7 +7308,7 @@ dependencies = [ "jaq-core", "jaq-json", "jaq-std", - "reqwest 0.12.28", + "reqwest", "serde", "serde_json", "thiserror 2.0.18", @@ -7322,11 +7322,13 @@ name = "tinyhumans-sdk" version = "0.1.0" dependencies = [ "base64 0.22.1", + "futures", "percent-encoding", "reqwest", "serde", "serde_json", "thiserror 2.0.18", + "tokio", "url", ] diff --git a/app/src-tauri/Cargo.lock b/app/src-tauri/Cargo.lock index 94ad8b9e79..593fe519e7 100644 --- a/app/src-tauri/Cargo.lock +++ b/app/src-tauri/Cargo.lock @@ -9038,11 +9038,13 @@ name = "tinyhumans-sdk" version = "0.1.0" dependencies = [ "base64 0.22.1", + "futures", "percent-encoding", "reqwest 0.12.28", "serde", "serde_json", "thiserror 2.0.18", + "tokio", "url", ] diff --git a/src/openhuman/agent/multimodal.rs b/src/openhuman/agent/multimodal.rs index bac74e454d..ddbf15cb9e 100644 --- a/src/openhuman/agent/multimodal.rs +++ b/src/openhuman/agent/multimodal.rs @@ -2,7 +2,10 @@ use crate::openhuman::agent::messages::ChatMessage; use crate::openhuman::config::{ build_runtime_proxy_client_with_timeouts, MultimodalConfig, MultimodalFileConfig, }; -use base64::{engine::general_purpose::STANDARD, Engine as _}; +use base64::{ + engine::general_purpose::{STANDARD, STANDARD_NO_PAD}, + Engine as _, +}; use flate2::read::GzDecoder; use reqwest::Client; use sha2::{Digest, Sha256}; @@ -196,19 +199,89 @@ fn latest_user_message(messages: &[ChatMessage]) -> Option<&ChatMessage> { messages.iter().rev().find(|m| m.role == "user") } +/// Longest bare reference still treated as a possible filesystem path. +/// +/// 64 base64 characters decode to 48 bytes. No real image comes in under that +/// (a minimal GIF is ~35 bytes, the smallest valid PNG ~67), so a path-prefixed +/// reference this short cannot be image data. +const MAX_PATH_SHAPED_REF_LEN: usize = 64; + +/// `true` when a **bare** reference is shaped like an absolute filesystem path +/// rather than base64 image data. +/// +/// Shape alone cannot decide this: `/` is in the standard base64 alphabet, and +/// a bare base64 JPEG legitimately begins `/9j/`. The length bound is what +/// separates the two — a real payload carrying that prefix runs to thousands of +/// characters, while `/tmp/foo` does not. +/// +/// Relative paths (`./x`, `~/x`, `../x`) need no check here: `.` and `~` are +/// outside the base64 alphabet, so the decode below already rejects them, as it +/// does every Windows path (`:` and `\` are likewise outside it). +/// +/// **Known residual ambiguity:** a relative path built only from base64 +/// characters that also decodes cleanly — `photos/cats1` — is genuinely +/// indistinguishable from a short payload and is still accepted. Tightening +/// that would start rejecting real base64, so it is left alone deliberately. +/// The window is narrower than it looks: the length must not be `4n + 1`, and a +/// partial trailing group must carry zero spare bits, which an arbitrary word +/// rarely does. +fn looks_like_absolute_path(payload: &str) -> bool { + payload.starts_with('/') && payload.len() < MAX_PATH_SHAPED_REF_LEN +} + +/// The base64 payload Ollama's `images` array expects, or `None` when +/// `image_ref` does not carry one. +/// +/// Accepts a `data:` URI (payload after the comma) or a bare base64 string. +/// +/// **Both forms are validated as base64** (#5146 P6). This used to return any +/// non-`data:` string verbatim, so a caller that passed a filesystem path — a +/// very natural reading of the `image_refs` parameter name — had that path +/// forwarded to Ollama as if it were image bytes, and got back +/// `illegal base64 data at input byte 19`. That error names neither the +/// parameter nor the path, and points at Ollama rather than at the caller. +/// Returning `None` instead lets the caller say something useful about which +/// reference it could not use. pub fn extract_ollama_image_payload(image_ref: &str) -> Option { - if image_ref.starts_with("data:") { + let is_data_uri = image_ref.starts_with("data:"); + let payload = if is_data_uri { let comma_idx = image_ref.find(',')?; - let (_, payload) = image_ref.split_at(comma_idx + 1); - let payload = payload.trim(); - if payload.is_empty() { - None - } else { - Some(payload.to_string()) - } + image_ref.split_at(comma_idx + 1).1.trim() } else { - Some(image_ref.trim().to_string()).filter(|value| !value.is_empty()) + image_ref.trim() + }; + if payload.is_empty() { + return None; + } + if !is_data_uri && looks_like_absolute_path(payload) { + tracing::debug!( + "[multimodal] image reference is shaped like a filesystem path, not image bytes" + ); + return None; + } + // Decode to validate only — the encoded form is what goes on the wire, and + // re-encoding would just burn a copy of a multi-MB image. Accept both + // padded and unpadded alphabets: real data URIs are padded, but some + // producers omit the `=`, and rejecting those would be a new regression + // rather than the fix this is. + // + // Length picks the engine rather than trying both: a complete group + // (`len % 4 == 0`) is exactly what `STANDARD` accepts, with or without + // trailing `=`, and only a partial group needs `STANDARD_NO_PAD`. Trying + // both in sequence decoded a multi-MB image twice for every unpadded + // payload. + let is_base64 = if payload.len() % 4 == 0 { + STANDARD.decode(payload).is_ok() + } else { + STANDARD_NO_PAD.decode(payload).is_ok() + }; + if !is_base64 { + tracing::debug!( + "[multimodal] image reference is not base64 (a filesystem path is not accepted here)" + ); + return None; } + Some(payload.to_string()) } /// Strip every `[FILE:…]` marker from `content` and return the cleaned diff --git a/src/openhuman/agent/multimodal_tests.rs b/src/openhuman/agent/multimodal_tests.rs index c0ebb58f01..60e5e2fbe5 100644 --- a/src/openhuman/agent/multimodal_tests.rs +++ b/src/openhuman/agent/multimodal_tests.rs @@ -123,9 +123,101 @@ async fn prepare_messages_rejects_oversized_local_image() { #[test] fn extract_ollama_image_payload_supports_data_uris() { - let payload = extract_ollama_image_payload("data:image/png;base64,abcd==") + // `YWJjZA==` is base64 for "abcd". The fixture used to be `abcd==`, which + // is not decodable base64 at all (6 chars); it only passed because the + // payload was returned verbatim without validation (#5146 P6). + let payload = extract_ollama_image_payload("data:image/png;base64,YWJjZA==") .expect("payload should be extracted"); - assert_eq!(payload, "abcd=="); + assert_eq!(payload, "YWJjZA=="); +} + +// ── #5146 P6: a reference that is not base64 must not reach Ollama ────────── + +#[test] +fn extract_ollama_image_payload_rejects_a_filesystem_path() { + // The bug: a path was forwarded verbatim as if it were image bytes, and + // Ollama answered `illegal base64 data at input byte 19` — an error that + // names neither the parameter nor the path. + assert!(extract_ollama_image_payload("/tmp/vision-test.png").is_none()); + assert!(extract_ollama_image_payload("./relative/img.jpg").is_none()); + assert!(extract_ollama_image_payload("~/Pictures/shot.png").is_none()); +} + +/// The fixtures above are rejected by the base64 alphabet (`.`, `-`, `~`), so +/// they never exercised the path check. An absolute path built only from +/// alphabet characters decodes cleanly and used to be forwarded as image bytes. +#[test] +fn extract_ollama_image_payload_rejects_a_path_that_is_also_valid_base64() { + // `/tmp/foo` is 8 alphabet characters: valid unpadded base64 for 6 bytes. + assert!(base64::engine::general_purpose::STANDARD_NO_PAD + .decode("/tmp/foo") + .is_ok()); + assert!(extract_ollama_image_payload("/tmp/foo").is_none()); + assert!(extract_ollama_image_payload("/Users/alice/tmp/foo").is_none()); +} + +/// The counterweight to the check above: base64 for a JPEG starts `/9j/`, so +/// rejecting every leading `/` would break real payloads. Length decides. +#[test] +fn extract_ollama_image_payload_still_accepts_a_bare_jpeg_payload() { + let jpeg = format!("/9j/4AAQSkZJRgABAQ{}", "A".repeat(64)); + assert!(jpeg.len() >= 64, "fixture must clear the path-shape bound"); + assert_eq!( + extract_ollama_image_payload(&jpeg).as_deref(), + Some(jpeg.as_str()) + ); +} + +/// Documents the residual ambiguity rather than pretending it is closed: a +/// relative path of pure base64 characters, of a length base64 permits, cannot +/// be told apart from a short payload, so it is still accepted. See +/// `looks_like_absolute_path`. +#[test] +fn extract_ollama_image_payload_cannot_reject_a_base64_shaped_relative_path() { + // 12 characters — a whole number of 4-character groups, so it decodes and + // is trivially canonical. + assert_eq!( + extract_ollama_image_payload("photos/cats1").as_deref(), + Some("photos/cats1") + ); + + // Most relative paths are NOT ambiguous, for two reasons that are easy to + // mistake for the check above doing the work: + // - `len % 4 == 1` is a length base64 never produces; + // - a partial trailing group must have its discarded low bits zero, and + // an arbitrary word almost never does. `photos/catpics` decodes as far + // as the alphabet is concerned, but its final `s` carries non-zero + // spare bits, so the decoder rejects it as non-canonical. + assert!(extract_ollama_image_payload("photos/catpic").is_none()); + assert!(extract_ollama_image_payload("photos/catpics").is_none()); +} + +#[test] +fn extract_ollama_image_payload_rejects_a_non_base64_data_uri_payload() { + assert!(extract_ollama_image_payload("data:image/png;base64,/tmp/not-base64.png").is_none()); +} + +#[test] +fn extract_ollama_image_payload_accepts_bare_base64_padded_and_unpadded() { + // Bare base64 stays supported — this path is how the agent hands an + // already-encoded image straight through. + assert_eq!( + extract_ollama_image_payload("YWJjZA==").as_deref(), + Some("YWJjZA==") + ); + // Some producers omit padding; rejecting those would be a new regression. + assert_eq!( + extract_ollama_image_payload("YWJjZA").as_deref(), + Some("YWJjZA") + ); +} + +#[test] +fn extract_ollama_image_payload_trims_before_validating() { + assert_eq!( + extract_ollama_image_payload(" YWJjZA== ").as_deref(), + Some("YWJjZA==") + ); } #[test] @@ -136,10 +228,9 @@ fn helpers_cover_marker_count_payload_and_message_composition() { ]; assert_eq!(count_image_markers(&messages), 2); assert!(contains_image_markers(&messages)); - assert_eq!( - extract_ollama_image_payload(" local-ref ").as_deref(), - Some("local-ref") - ); + // `local-ref` is not base64 (`-` is outside the standard alphabet), so it + // is no longer passed through as an image payload (#5146 P6). + assert!(extract_ollama_image_payload(" local-ref ").is_none()); assert!(extract_ollama_image_payload("data:image/png;base64, ").is_none()); let composed = diff --git a/src/openhuman/inference/local/service/assets.rs b/src/openhuman/inference/local/service/assets.rs index 2c224eed4c..461afefa8a 100644 --- a/src/openhuman/inference/local/service/assets.rs +++ b/src/openhuman/inference/local/service/assets.rs @@ -702,8 +702,17 @@ impl LocalAiService { .to_string(), ); } + // Resolve rather than take the effective id: the latter blanks + // a chat-only model, and a blank id makes + // `ensure_ollama_model_available` answer "no vision model is + // configured" to a user who configured one. Naming the model + // that cannot accept images is the whole point of #5146 P1. + // + // Before probing Ollama, too: a misconfigured id is answerable + // without the network, and reaching the server first would hand + // back a connection error for a problem that is purely local. + let model = model_ids::resolve_vision_model_id(config)?; self.ensure_ollama_server(config).await?; - let model = model_ids::effective_vision_model_id(config); self.ensure_ollama_model_available(config, &model, "vision") .await?; } diff --git a/src/openhuman/inference/local/service/ollama_admin/model_pull.rs b/src/openhuman/inference/local/service/ollama_admin/model_pull.rs index b9761227cd..c63130741d 100644 --- a/src/openhuman/inference/local/service/ollama_admin/model_pull.rs +++ b/src/openhuman/inference/local/service/ollama_admin/model_pull.rs @@ -19,6 +19,12 @@ impl LocalAiService { self.ensure_ollama_model_available(config, &chat_model, "chat") .await?; + // Held until every other preload has run. `ensure_ollama_model_available` + // writes `status.warning` for its own transient "Pulling …" progress, so + // publishing the vision reason here would let a later embedding/STT/TTS + // pull bury it and leave `vision_state = "missing"` with no explanation. + let mut vision_warning: Option = None; + match presets::vision_mode_for_config(&config.local_ai) { VisionMode::Disabled => { self.status.lock().vision_state = "disabled".to_string(); @@ -26,12 +32,31 @@ impl LocalAiService { VisionMode::Ondemand => { self.status.lock().vision_state = "idle".to_string(); } - VisionMode::Bundled => { - let vision_model = model_ids::effective_vision_model_id(config); - self.ensure_ollama_model_available(config, &vision_model, "vision") - .await?; - self.status.lock().vision_state = "ready".to_string(); - } + VisionMode::Bundled => match model_ids::resolve_vision_model_id(config) { + Ok(vision_model) => { + self.ensure_ollama_model_available(config, &vision_model, "vision") + .await?; + self.status.lock().vision_state = "ready".to_string(); + } + Err(err) => { + // A vision model the user misconfigured must not take the + // whole local runtime down with it. `bootstrap()` returns + // on the first `ensure_models_available` error, so + // propagating here would leave the service `degraded` and + // skip the embedding/STT/TTS preloads and the ready state — + // punishing chat for a vision-only mistake. Record it and + // carry on; `resolve_vision_model_id` raises the same + // message again, actionably, at request time. + tracing::warn!( + vision_model_id = %config.local_ai.vision_model_id.trim(), + vision_state = "missing", + %err, + "[local_ai] bundled vision model is unusable; continuing without vision" + ); + self.status.lock().vision_state = "missing".to_string(); + vision_warning = Some(err); + } + }, } let embedding_model = model_ids::effective_embedding_model_id(config); @@ -49,6 +74,13 @@ impl LocalAiService { self.ensure_tts_asset_available(config).await?; } + // Last write wins, which is the point: whatever the preloads left behind + // is transient progress text, while this is a standing configuration + // problem the user has to act on. + if let Some(err) = vision_warning { + self.status.lock().warning = Some(err); + } + Ok(()) } @@ -58,6 +90,22 @@ impl LocalAiService { model_id: &str, label: &str, ) -> Result<(), String> { + // #5146 P1: never pull a nameless model. `effective_*_model_id` returns + // an empty string when there is no usable model for a role, and several + // callers feed that straight in here. Without this guard that became a + // `POST /api/pull` with a blank name, retried three times, ending in an + // opaque error — and it is the same code path that silently pulled a + // ~1.7 GB vision substitute the user never chose. Fail immediately and + // say which role is unconfigured instead. + let model_id = model_id.trim(); + if model_id.is_empty() { + return Err(format!( + "no {label} model is configured for the local runtime, so there is nothing to \ + download. Set the {label} model in Settings → Local AI (or pick a provider for \ + the {label} workload) before retrying." + )); + } + let base_url = ollama_base_url_from_config(config); if self.has_model_at(&base_url, model_id).await? { return Ok(()); diff --git a/src/openhuman/inference/local/service/ollama_admin_tests.rs b/src/openhuman/inference/local/service/ollama_admin_tests.rs index 0cfd8a786b..b50368e392 100644 --- a/src/openhuman/inference/local/service/ollama_admin_tests.rs +++ b/src/openhuman/inference/local/service/ollama_admin_tests.rs @@ -1461,3 +1461,204 @@ async fn non_404_status_does_not_trigger_the_fallback() { "the /api/tags fallback must not run for a non-404 status" ); } + +// ── #5146 P1: never pull a model the user did not choose ──────────────────── + +/// A blank model id must fail immediately with a message about configuration, +/// never becoming a `POST /api/pull` for a nameless model. +/// +/// `effective_*_model_id` returns an empty string when a role has no usable +/// model, and several callers feed that straight into +/// `ensure_ollama_model_available`. Before this guard that produced a nameless +/// pull retried three times before failing opaquely — and it is the same path +/// that silently downloaded a ~1.7 GB vision substitute. +/// +/// No mock server is needed: the guard must reject before any network work, so +/// a test that reaches the network would itself be the failure. +#[tokio::test] +async fn ensure_ollama_model_available_rejects_a_blank_model_id() { + let _guard = crate::openhuman::inference::inference_test_guard(); + + let config = Config::default(); + let service = LocalAiService::new(&config); + + for blank in ["", " ", "\t"] { + let err = service + .ensure_ollama_model_available(&config, blank, "vision") + .await + .expect_err("a blank model id must not be pulled"); + assert!( + err.contains("vision"), + "error must name the role that is unconfigured: {err}" + ); + assert!( + err.contains("nothing to download"), + "error must say no download will happen: {err}" + ); + } + + // The label is carried through, so embedding reads as an embedding problem. + let err = service + .ensure_ollama_model_available(&config, "", "embedding") + .await + .expect_err("a blank model id must not be pulled"); + assert!(err.contains("embedding"), "got: {err}"); +} + +/// A vision model the user misconfigured must not take the whole local runtime +/// down with it. +/// +/// `bootstrap()` returns on the first `ensure_models_available` error, so +/// propagating a chat-only vision model out of the `Bundled` branch left the +/// service `degraded` and skipped the remaining preloads and the ready state — +/// punishing chat for a vision-only mistake. The reason is recorded on the +/// status instead, and `resolve_vision_model_id` raises it again, actionably, +/// at request time. +#[tokio::test] +async fn bundled_vision_misconfiguration_does_not_abort_the_rest_of_bootstrap() { + let _guard = crate::openhuman::inference::inference_test_guard(); + + // The chat model is present, so only vision can fail here. + let app = Router::new().route( + "/api/tags", + get(|| async { + Json(json!({ + "models": [ + {"name": "gemma3:1b-it-qat", "modified_at": "", "size": 1u64, "digest": "d"} + ] + })) + }), + ); + let base = spawn_mock(app).await; + unsafe { + std::env::set_var("OPENHUMAN_OLLAMA_BASE_URL", &base); + } + + let mut config = Config::default(); + config.local_ai.runtime_enabled = true; + config.local_ai.selected_tier = Some("custom".to_string()); + config.local_ai.chat_model_id = "gemma3:1b-it-qat".to_string(); + // A valid chat model on Ollama, but it cannot accept images. + config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); + config.local_ai.preload_vision_model = true; + // `preload_embedding_model` defaults to TRUE, and the mock serves no + // `/api/pull`, so leaving it on made the embedding preload 404 and this + // test fail for a reason that has nothing to do with vision. The + // later-preload interaction is covered by its own test below. + config.local_ai.preload_embedding_model = false; + + let service = LocalAiService::new(&config); + let result = service.ensure_models_available(&config).await; + + unsafe { + std::env::remove_var("OPENHUMAN_OLLAMA_BASE_URL"); + } + + result.expect("a chat-only vision model must not fail the whole bootstrap"); + + let status = service.status.lock(); + assert_eq!( + status.vision_state, "missing", + "the unusable vision model must be visible as missing, not ready" + ); + let warning = status + .warning + .clone() + .expect("the reason vision is unavailable must be surfaced"); + assert!( + warning.contains("gemma3n:e4b-it-q8_0"), + "the warning must name the model the user configured: {warning}" + ); + assert!( + warning.contains("not vision-capable"), + "the warning must say what is wrong with it: {warning}" + ); +} + +/// The vision reason must still be readable after a later preload runs. +/// +/// `ensure_ollama_model_available` writes `status.warning` for its own transient +/// "Pulling …" progress, so publishing the vision failure at the point it +/// happens let the embedding pull bury it — leaving `vision_state = "missing"` +/// with no explanation of why. The reason is published after every other +/// preload for exactly this reason. +#[tokio::test] +async fn a_later_preload_does_not_bury_the_vision_failure_reason() { + use axum::routing::post; + use std::sync::atomic::{AtomicBool, Ordering}; + use std::sync::Arc; + + let _guard = crate::openhuman::inference::inference_test_guard(); + + // The embedding model appears only once it has been pulled, so the preload + // takes the full download path — the one that writes `status.warning`. + let pulled = Arc::new(AtomicBool::new(false)); + let tags_flag = pulled.clone(); + let pull_flag = pulled.clone(); + let app = Router::new() + .route( + "/api/tags", + get(move || { + let pulled = tags_flag.clone(); + async move { + let mut models = vec![ + json!({"name": "gemma3:1b-it-qat", "modified_at": "", "size": 1u64, "digest": "d"}), + ]; + if pulled.load(Ordering::SeqCst) { + models.push( + json!({"name": "bge-m3", "modified_at": "", "size": 2u64, "digest": "d"}), + ); + } + Json(json!({ "models": models })) + } + }), + ) + .route( + "/api/pull", + post(move || { + let pulled = pull_flag.clone(); + async move { + pulled.store(true, Ordering::SeqCst); + Json(json!({ "status": "success" })) + } + }), + ); + let base = spawn_mock(app).await; + unsafe { + std::env::set_var("OPENHUMAN_OLLAMA_BASE_URL", &base); + } + + let mut config = Config::default(); + config.local_ai.runtime_enabled = true; + config.local_ai.selected_tier = Some("custom".to_string()); + config.local_ai.chat_model_id = "gemma3:1b-it-qat".to_string(); + config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); + config.local_ai.preload_vision_model = true; + config.local_ai.embedding_model_id = "bge-m3".to_string(); + config.local_ai.preload_embedding_model = true; + + let service = LocalAiService::new(&config); + let result = service.ensure_models_available(&config).await; + + unsafe { + std::env::remove_var("OPENHUMAN_OLLAMA_BASE_URL"); + } + + result.expect("a chat-only vision model must not fail the whole bootstrap"); + assert!( + pulled.load(Ordering::SeqCst), + "the embedding preload must have pulled" + ); + + let status = service.status.lock(); + assert_eq!(status.vision_state, "missing"); + assert_eq!(status.embedding_state, "ready"); + let warning = status + .warning + .clone() + .expect("the vision reason must survive the embedding preload"); + assert!( + warning.contains("gemma3n:e4b-it-q8_0"), + "the embedding pull's progress text must not have buried it: {warning}" + ); +} diff --git a/src/openhuman/inference/local/service/vision_embed.rs b/src/openhuman/inference/local/service/vision_embed.rs index 7d14691931..12ab06820b 100644 --- a/src/openhuman/inference/local/service/vision_embed.rs +++ b/src/openhuman/inference/local/service/vision_embed.rs @@ -53,22 +53,22 @@ impl LocalAiService { } self.bootstrap(config).await; - // Resolve through `resolve_vision_model_choice` rather than + // Resolve through `resolve_vision_model_id` rather than // `effective_vision_model_id`: the latter returns an empty string when - // no vision model is configured, which used to be handed straight to + // there is no usable vision model, which used to be handed straight to // `ensure_ollama_model_available` and became a nameless `POST // /api/pull` retried three times before failing opaquely (#5146). // The resolver guarantees a non-empty, vision-capable id or a message // that says what to configure. - // NOTE: this Err arm is defence in depth, not a reachable branch here. - // `resolve_vision_model_choice` fails only when no vision model is - // configured, and that same condition makes `vision_mode_for_config` - // report `Disabled` (a blank `vision_model_id` cannot match any - // vision-enabled preset, so the tier resolves to `Custom` -> Disabled), - // which returns above. The empty case is covered directly in - // `model_ids::tests::resolve_vision_model_id_errors_when_unconfigured`. - let choice = match model_ids::resolve_vision_model_choice(config) { - Ok(choice) => choice, + // + // Since #5146 P1 it also refuses a *chat-only* configured model instead + // of substituting one. That arm IS reachable: `vision_mode_for_config` + // only checks the tier, so a user on a vision-enabled tier who points + // `vision_model_id` at their chat model reaches here and now gets told + // exactly that, rather than having a 1.7 GB substitute pulled behind + // their back. + let vision_model = match model_ids::resolve_vision_model_id(config) { + Ok(model) => model, Err(error) => { self.status.lock().vision_state = "missing".to_string(); tracing::warn!( @@ -79,25 +79,9 @@ impl LocalAiService { return Err(error); } }; - let vision_model = choice.model; - // A capability substitution means we are about to talk about a model the - // user never named. Carry that into the error text so "pull moondream" - // cannot read as a non-sequitur to someone who configured gemma3. - let substitution_note = choice - .replaced - .as_deref() - .map(|configured| { - format!( - " Your configured `{configured}` cannot accept images, so OpenHuman \ - selected `{vision_model}` instead; set `local_ai.vision_model_id` to \ - a vision-capable model to choose your own." - ) - }) - .unwrap_or_default(); tracing::debug!( target: "local_ai::vision", model = %vision_model, - substituted_for = ?choice.replaced, "[local_ai:vision] resolved vision-capable model" ); @@ -111,14 +95,16 @@ impl LocalAiService { tracing::warn!( target: "local_ai::vision", model = %vision_model, - substituted_for = ?choice.replaced, %error, "[local_ai:vision] vision model unavailable" ); + // `vision_model` is now always the model the user configured, so + // "pull it" can no longer name a model they never chose — the + // substitution note this used to carry has no case left to cover. return Err(format!( "local vision model `{vision_model}` is not available: {error}. \ Pull it with `ollama pull {vision_model}`, or route the vision \ - workload to a cloud provider with `vision_provider`.{substitution_note}" + workload to a cloud provider with `vision_provider`." )); } @@ -127,7 +113,15 @@ impl LocalAiService { .filter_map(|reference| multimodal::extract_ollama_image_payload(reference)) .collect(); if images.is_empty() { - return Err("no valid image payloads were provided".to_string()); + // #5146 P6: the most common cause is a caller passing a filesystem + // path. Say what this parameter actually takes rather than leaving + // the caller to discover it from Ollama's "illegal base64 data". + return Err(format!( + "none of the {} supplied image reference(s) carried a usable image payload. \ + `image_refs` takes a `data:image/...;base64,` URI or a bare base64 \ + string — a filesystem path is not read from disk here.", + image_refs.len() + )); } // Vision generation is background LLM-bound work; gate it through @@ -476,50 +470,6 @@ mod tests { ); } - /// A chat-only model configured for vision must never be the model that - /// receives the images. - /// - /// Ollama accepts an `images` array against a text-only model, discards it, - /// and answers from the prompt alone, so passing `gemma3n` through would - /// return a fabricated description rather than an error. - #[tokio::test] - async fn vision_prompt_never_routes_images_at_a_chat_only_model() { - let _guard = crate::openhuman::inference::inference_test_guard(); - - let base = spawn_mock(mock_ollama_echoing_requested_model( - "moondream:1.8b-v2-q4_K_S", - )) - .await; - unsafe { - std::env::set_var("OPENHUMAN_OLLAMA_BASE_URL", &base); - } - - let mut config = enabled_config(); - // Text-only on Ollama, despite sharing a prefix with multimodal gemma3. - config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); - let service = ready_service(&config); - - let result = service - .vision_prompt( - &config, - "describe", - &["data:image/png;base64,QUJD".to_string()], - None, - ) - .await; - - unsafe { - std::env::remove_var("OPENHUMAN_OLLAMA_BASE_URL"); - } - - let model_used = result.expect("vision prompt should succeed"); - assert_ne!( - model_used, "gemma3n:e4b-it-q8_0", - "images must never be sent to a chat-only model" - ); - assert_eq!(model_used, "moondream:1.8b-v2-q4_K_S"); - } - /// A configured-but-unpullable vision model must report a vision problem /// naming the model and the `ollama pull` that fixes it. #[tokio::test] @@ -573,24 +523,32 @@ mod tests { assert_eq!(service.status.lock().vision_state, "missing"); } - /// greptile #5253: when the capability guard swaps a chat-only model for a - /// vision-capable default, an unavailable-model error must say so. Without - /// this the user is told to `ollama pull moondream:…` having configured - /// `gemma3n:…`, with nothing connecting the two. + /// #5146 P1: a chat-only `vision_model_id` must fail *before* any network + /// work, naming the configured model — no substitution, and above all no + /// pull of a model the user never chose. + /// + /// The mock deliberately offers a working `/api/pull`. If the request ever + /// reaches it, the substitution is back and this test fails on the assert + /// that no pull was attempted rather than on a transport error. #[tokio::test] - async fn unavailable_error_explains_a_capability_substitution() { + async fn chat_only_vision_model_errors_without_substituting_or_pulling() { use axum::routing::get; + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; let _guard = crate::openhuman::inference::inference_test_guard(); + let pulls = Arc::new(AtomicUsize::new(0)); + let pull_counter = pulls.clone(); let app = Router::new() .route("/api/tags", get(|| async { Json(json!({ "models": [] })) })) .route( "/api/pull", - post(|| async { - ( - axum::http::StatusCode::INTERNAL_SERVER_ERROR, - "pull refused", - ) + post(move || { + let pulls = pull_counter.clone(); + async move { + pulls.fetch_add(1, Ordering::SeqCst); + Json(json!({ "status": "success" })) + } }), ); let base = spawn_mock(app).await; @@ -599,7 +557,6 @@ mod tests { } let mut config = enabled_config(); - // Chat-only: the guard substitutes the vision-capable default. config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); let service = ready_service(&config); @@ -611,7 +568,7 @@ mod tests { None, ) .await - .expect_err("an unpullable substituted model must fail"); + .expect_err("a chat-only vision model must fail"); unsafe { std::env::remove_var("OPENHUMAN_OLLAMA_BASE_URL"); @@ -622,12 +579,77 @@ mod tests { "error must name the model the user actually configured: {err}" ); assert!( - err.contains("cannot accept images"), - "error must explain why it was replaced: {err}" + err.contains("not vision-capable"), + "error must explain what is wrong with it: {err}" + ); + // The suggestion list legitimately contains `DEFAULT_LOW_VISION_MODEL`, + // so its mere presence proves nothing. What must not happen is the + // message presenting it as the model that *ran*; the `pulls` and + // `vision_state` assertions below pin that nothing was fetched behind + // the user's back. + assert!( + err.contains("for example"), + "a vision-capable model must be offered as an example to choose, never as a \ + substitute that was already applied: {err}" + ); + assert_eq!( + pulls.load(Ordering::SeqCst), + 0, + "no model may be downloaded for a vision request the user misconfigured" + ); + assert_eq!(service.status.lock().vision_state, "missing"); + } + + /// #5146 P6: a reference that is not base64 (a filesystem path is the + /// common case) must produce a message about what `image_refs` accepts, + /// not Ollama's `illegal base64 data at input byte 19`. + #[tokio::test] + async fn non_base64_image_reference_is_rejected_with_guidance() { + use axum::routing::get; + let _guard = crate::openhuman::inference::inference_test_guard(); + + // The payload check runs *after* model availability, so the model must + // read as already installed or this would dial a real Ollama. + let app = Router::new().route( + "/api/tags", + get(|| async { + Json(json!({ + "models": [ + { "name": "llava:7b", "modified_at": "", "size": 0u64, "digest": "a" } + ] + })) + }), + ); + let base = spawn_mock(app).await; + unsafe { + std::env::set_var("OPENHUMAN_OLLAMA_BASE_URL", &base); + } + + let mut config = enabled_config(); + config.local_ai.vision_model_id = "llava:7b".to_string(); + let service = ready_service(&config); + + let err = service + .vision_prompt( + &config, + "describe", + &["/tmp/vision-test.png".to_string()], + None, + ) + .await + .expect_err("a filesystem path is not an image payload"); + + unsafe { + std::env::remove_var("OPENHUMAN_OLLAMA_BASE_URL"); + } + + assert!( + err.contains("base64"), + "error must say what the parameter accepts: {err}" ); assert!( - err.contains(crate::openhuman::inference::model_ids::DEFAULT_OLLAMA_VISION_MODEL), - "error must name the substitute it is asking the user to pull: {err}" + err.contains("filesystem path"), + "error must name the mistake the caller actually made: {err}" ); } } diff --git a/src/openhuman/inference/model_ids.rs b/src/openhuman/inference/model_ids.rs index 0bfccb0cf9..2ffb528a17 100644 --- a/src/openhuman/inference/model_ids.rs +++ b/src/openhuman/inference/model_ids.rs @@ -15,15 +15,23 @@ use crate::openhuman::inference::vision_models::{self, VISION_MODEL_SUGGESTIONS} pub(crate) const DEFAULT_OLLAMA_MODEL: &str = "gemma3:1b-it-qat"; -/// Default local vision model. Must name a genuinely vision-capable model: -/// it is the fallback whenever a configured vision id turns out to be -/// chat-only, so a chat model here would defeat the whole guard (#5146). +/// The pinned Moondream build that the `moondream` / `moondream:1.8b` +/// shorthands resolve to, and the low-RAM tier's bundled vision model. /// -/// Moondream is the smallest vision model that is pullable with no extra -/// setup (~1.7 GB across model + projector layers), which keeps the fallback -/// affordable on the low-RAM tiers where vision is most likely to be enabled -/// on demand. -pub(crate) const DEFAULT_OLLAMA_VISION_MODEL: &str = "moondream:1.8b-v2-q4_K_S"; +/// Must name a genuinely vision-capable model: it is what an alias rewrite +/// lands on, and what the "for example …" suggestions point users at. +/// +/// Moondream is the smallest vision model pullable with no extra setup +/// (~1.7 GB across model + projector layers), which keeps it affordable on the +/// low-RAM tiers where vision is most likely to be enabled on demand. +/// +/// There is deliberately no `DEFAULT_OLLAMA_VISION_MODEL` any more (#5146 P1). +/// It existed only as the substitute for a chat-only `vision_model_id`, and +/// that substitution is exactly the bug: the user's explicit choice was +/// overridden, this model was auto-pulled behind their back, and the request +/// then failed with `ollama vision returned empty content`. A misconfigured +/// vision model is now an actionable error, so there is nothing left to +/// default *to*. pub(crate) const DEFAULT_LOW_VISION_MODEL: &str = "moondream:1.8b-v2-q4_K_S"; pub(crate) const DEFAULT_OLLAMA_EMBED_MODEL: &str = "bge-m3"; @@ -80,30 +88,52 @@ fn enforce_mvp_chat_allowlist(resolved: &str) -> String { MVP_DEFAULT_CHAT_MODEL.to_string() } -/// Guarantee a vision request never reaches a chat-only model. +/// Guarantee a vision request never reaches a chat-only model: `Ok(id)` when +/// `resolved` accepts image input, `Err(actionable message)` when it does not. +/// +/// The tier restriction is enforced upstream by +/// [`crate::openhuman::inference::presets::vision_mode_for_config`], which +/// reports `VisionMode::Disabled` for the tiers that ship no vision model. What +/// is left for this function is the capability question alone. +/// +/// # Why this errors instead of substituting (#5146 P1) +/// +/// It used to swap in the default vision model and return that, which produced +/// the worst failure in the whole vision path: a user who set a chat-only +/// `vision_model_id` got a *different* model silently selected, that model +/// auto-pulled (~1.7 GB with no visible progress), and then — since the +/// substitute answers many prompt phrasings with an empty string — the cryptic +/// `ollama vision returned empty content`. Three surprises deep, none of them +/// naming the actual mistake. /// -/// This replaces the previous `MVP_ALLOWED_VISION_MODELS = &[""]` allowlist, -/// which matched only the empty string and therefore rewrote *every* -/// configured vision model to `""` — including genuinely vision-capable ones. -/// Callers then sent an empty model name to Ollama, which is not a clean -/// failure: `ensure_ollama_model_available` tried to `POST /api/pull` a -/// nameless model and retried three times before surfacing an opaque error -/// (#5146 §Part 1). +/// Substituting is the wrong shape regardless of which default is chosen: the +/// user made an explicit choice and it was silently overridden, the same class +/// of bug as a silent provider switch (#5146 §2.1). Say what is wrong and let +/// them fix it. The message deliberately mirrors the tinyagents Ollama +/// embeddings adapter, which names the offending model and a concrete next step +/// in one line. /// -/// The tier restriction that allowlist was standing in for is enforced -/// upstream by [`crate::openhuman::inference::presets::vision_mode_for_config`], -/// which reports `VisionMode::Disabled` for the tiers that ship no vision -/// model. What is left for this function is the capability question alone. -fn enforce_vision_capability(resolved: &str) -> String { +/// An earlier incarnation of this guard was an allowlist +/// (`MVP_ALLOWED_VISION_MODELS = &[""]`) that matched only the empty string and +/// so rewrote *every* configured vision model to `""`, including capable ones — +/// which is how the nameless `POST /api/pull` in `ensure_ollama_model_available` +/// came about. Both that bug and its replacement failed the same way: they +/// answered "which model?" with something the user never asked for. +fn enforce_vision_capability(resolved: &str) -> Result { if vision_models::is_vision_capable(resolved) { - return resolved.to_string(); + return Ok(resolved.to_string()); } tracing::warn!( resolved, - fallback = DEFAULT_OLLAMA_VISION_MODEL, - "[local_ai] configured vision model is chat-only, falling back to a vision-capable default" + "[local_ai] configured vision model is chat-only; refusing to substitute" ); - DEFAULT_OLLAMA_VISION_MODEL.to_string() + let suggestions = VISION_MODEL_SUGGESTIONS.join("`, `"); + Err(format!( + "the selected vision model `{resolved}` is not vision-capable — it cannot accept image \ + input. Set `local_ai.vision_model_id` to a vision-capable model (for example \ + `{suggestions}`) and pull it with `ollama pull `, or route the vision workload \ + to a cloud provider with `vision_provider`." + )) } fn enforce_mvp_embedding_allowlist(resolved: &str) -> String { @@ -171,50 +201,71 @@ fn raw_chat_model_id(config: &Config) -> String { raw.to_string() } +/// Apply the alias rewrite that maps a family name onto the pinned tag we +/// actually ship (`moondream` -> `moondream:1.8b-v2-q4_K_S`). +/// +/// This is *not* a substitution: it resolves to the same model the user asked +/// for, so it never needs to be reported to them. +fn apply_vision_alias(raw: &str) -> &str { + let lower = raw.to_ascii_lowercase(); + if lower == "moondream:1.8b" || lower == "moondream" { + DEFAULT_LOW_VISION_MODEL + } else { + raw + } +} + /// Resolve the vision model for status / reporting surfaces. /// -/// An empty return means "vision is not configured" and is a legitimate -/// state (the low tiers ship no vision model). A non-empty return is -/// **always** a vision-capable id. Call [`resolve_vision_model_choice`] instead -/// when about to issue an actual vision request — it turns the -/// not-configured case into an actionable error rather than an empty string -/// that downstream code would send to Ollama verbatim. +/// An empty return means "there is no **usable** vision model" — either none is +/// configured (a legitimate state; the low tiers ship no vision model) or the +/// configured one cannot accept images. A non-empty return is always a +/// vision-capable id. +/// +/// Since #5146 P1 this no longer substitutes a default for a chat-only +/// configured model. That matters beyond reporting: several callers feed this +/// straight into `ensure_ollama_model_available`, so a substituted id here was +/// how a model the user never chose got auto-pulled. Returning empty keeps the +/// pull paths off a model nobody asked for, and +/// `ensure_ollama_model_available` rejects a blank id outright rather than +/// pulling a nameless model. +/// +/// Call [`resolve_vision_model_id`] instead when about to issue an actual +/// vision request — it distinguishes "not configured" from "not vision-capable" +/// and returns an actionable message for each. +/// +/// The capability predicate is consulted directly here rather than by calling +/// [`enforce_vision_capability`] and discarding its `Err`: that helper emits a +/// `tracing::warn!` and formats the full suggestion message, and this resolver +/// feeds polled status/diagnostics surfaces. Routing through it would log a +/// warning and burn a `format!` on *every poll* for anyone with a misconfigured +/// `vision_model_id`. The warning belongs at request time, where it is +/// actionable; `effective_and_resolved_vision_ids_agree_on_usability` keeps the +/// two paths pinned to the same verdict. pub(crate) fn effective_vision_model_id(config: &Config) -> String { let raw = config.local_ai.vision_model_id.trim(); if raw.is_empty() { return String::new(); } - let lower = raw.to_ascii_lowercase(); - let resolved = if lower == "moondream:1.8b" || lower == "moondream" { - DEFAULT_LOW_VISION_MODEL + let resolved = apply_vision_alias(raw); + if vision_models::is_vision_capable(resolved) { + resolved.to_string() } else { - raw - }; - enforce_vision_capability(resolved) -} - -/// The vision model a request will actually use, plus what it displaced. -pub(crate) struct VisionModelChoice { - /// The vision-capable model id to send to Ollama. - pub(crate) model: String, - /// The configured id that was swapped out because it is chat-only. - /// - /// `Some` means the user asked for one model and is getting another, which - /// every downstream *error* must say out loud: a bare "`moondream:...` is - /// not available, pull it" is actively misleading when the user configured - /// `gemma3:1b-it-qat` and never mentioned moondream (greptile, #5253). - pub(crate) replaced: Option, + String::new() + } } -/// Resolve the vision model for a real vision request, reporting any -/// capability substitution. +/// Resolve the vision model for a real vision request. +/// +/// Never returns an empty id, and never silently swaps the user's choice. The +/// two failure modes get distinct, actionable messages (#5146 §Part 1, P1): /// -/// Never returns an empty id: when no vision model is configured the caller -/// gets a message naming what to set and which models to pull, instead of -/// silently shipping an empty model name to Ollama (#5146 §Part 1). -pub(crate) fn resolve_vision_model_choice(config: &Config) -> Result { - let resolved = effective_vision_model_id(config); - if resolved.trim().is_empty() { +/// - **nothing configured** — say what to set and which models to pull; +/// - **configured but chat-only** — name the offending model, because "pull +/// `moondream:…`" is a non-sequitur to someone who configured `gemma3:1b`. +pub(crate) fn resolve_vision_model_id(config: &Config) -> Result { + let raw = config.local_ai.vision_model_id.trim(); + if raw.is_empty() { let suggestions = VISION_MODEL_SUGGESTIONS.join("`, `"); tracing::warn!("[local_ai] vision request with no vision model configured"); return Err(format!( @@ -224,20 +275,7 @@ pub(crate) fn resolve_vision_model_choice(config: &Config) -> Result - // the pinned tag) resolves to a different string but is the same model the - // user asked for, so it is not something they need to be told about. - let configured = config.local_ai.vision_model_id.trim(); - let replaced = (!configured.is_empty() - && !vision_models::is_vision_capable(configured) - && !resolved.eq_ignore_ascii_case(configured)) - .then(|| configured.to_string()); - - Ok(VisionModelChoice { - model: resolved, - replaced, - }) + enforce_vision_capability(apply_vision_alias(raw)) } pub(crate) fn effective_embedding_model_id(config: &Config) -> String { @@ -447,29 +485,32 @@ mod tests { } } - /// #5146 §Part 1: a chat-only model must never be returned as the vision - /// model. Ollama silently drops the `images` array for such a model, so - /// passing it through would produce a fabricated description instead of an - /// error. + /// #5146 §Part 1 / P1: a chat-only model must never be returned as the + /// vision model — and must not be quietly swapped for one either. Both + /// resolvers report it as unusable so no pull path can act on it. #[test] - fn chat_only_vision_model_falls_back_to_a_vision_capable_default() { + fn chat_only_vision_model_resolves_to_nothing_usable() { let mut config = test_config(); for chat_only in ["gemma3n:e4b-it-q8_0", "gemma3:1b-it-qat", "llama3.1:8b"] { config.local_ai.vision_model_id = chat_only.to_string(); - let resolved = effective_vision_model_id(&config); - assert_eq!(resolved, DEFAULT_OLLAMA_VISION_MODEL); - assert!(vision_models::is_vision_capable(&resolved)); + assert_eq!( + effective_vision_model_id(&config), + "", + "{chat_only} must not resolve to a substitute" + ); + assert!( + resolve_vision_model_id(&config).is_err(), + "{chat_only} must be an actionable error at request time" + ); } } - /// The default must itself be vision-capable — it is the fallback the - /// guard above lands on, so a chat-only default would defeat the guard. + /// The pinned default must itself be vision-capable: it is what the + /// `moondream` alias resolves to, and what the "for example …" suggestions + /// point users at, so a chat-only default would send them in a circle. #[test] fn default_vision_model_is_vision_capable() { - assert!(!DEFAULT_OLLAMA_VISION_MODEL.is_empty()); - assert!(vision_models::is_vision_capable( - DEFAULT_OLLAMA_VISION_MODEL - )); + assert!(!DEFAULT_LOW_VISION_MODEL.is_empty()); assert!(vision_models::is_vision_capable(DEFAULT_LOW_VISION_MODEL)); } @@ -480,7 +521,7 @@ mod tests { let mut config = test_config(); config.local_ai.vision_model_id = String::new(); - let err = resolve_vision_model_choice(&config) + let err = resolve_vision_model_id(&config) .err() .expect("expected a vision error"); assert!( @@ -493,47 +534,120 @@ mod tests { ); // Whitespace-only is the same "not configured" state. config.local_ai.vision_model_id = " ".to_string(); - assert!(resolve_vision_model_choice(&config).is_err()); + assert!(resolve_vision_model_id(&config).is_err()); } #[test] - fn resolve_vision_model_id_returns_a_vision_capable_model_when_configured() { + fn resolve_vision_model_id_returns_the_configured_model_when_it_can_see() { let mut config = test_config(); config.local_ai.vision_model_id = "llava:7b".to_string(); - assert_eq!( - resolve_vision_model_choice(&config).unwrap().model, - "llava:7b" - ); + assert_eq!(resolve_vision_model_id(&config).unwrap(), "llava:7b"); + } - // Even a chat-only configured id resolves to something that can see. - config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); - let resolved = resolve_vision_model_choice(&config).unwrap().model; - assert!(vision_models::is_vision_capable(&resolved)); + /// An alias rewrite resolves to a different string but is the *same* model + /// the user asked for, so it stays silent and must keep working. + #[test] + fn resolve_vision_model_id_still_applies_the_moondream_alias() { + let mut config = test_config(); + for alias in ["moondream", "moondream:1.8b", "MoonDream"] { + config.local_ai.vision_model_id = alias.to_string(); + let resolved = resolve_vision_model_id(&config) + .unwrap_or_else(|e| panic!("alias {alias} must resolve, got: {e}")); + assert_eq!(resolved, DEFAULT_LOW_VISION_MODEL); + assert!(vision_models::is_vision_capable(&resolved)); + } } + // ── #5146 P1: a chat-only vision model errors, never substitutes ───────── + + /// The headline P1 regression. A chat-only `vision_model_id` used to be + /// silently swapped for the default vision model, which was then + /// auto-pulled (~1.7 GB, no progress) and answered many prompts with an + /// empty string — surfacing as `ollama vision returned empty content`. #[test] - fn resolve_vision_model_choice_reports_only_capability_substitutions() { + fn resolve_vision_model_id_errors_on_a_chat_only_model_instead_of_substituting() { let mut config = test_config(); + config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); - // A vision-capable id is used as-is, with nothing to report. - config.local_ai.vision_model_id = "llava:7b".to_string(); - let choice = resolve_vision_model_choice(&config).unwrap(); - assert_eq!(choice.model, "llava:7b"); - assert_eq!(choice.replaced, None); + let err = resolve_vision_model_id(&config) + .err() + .expect("a chat-only vision model must be an error, not a substitution"); + + assert!( + err.contains("gemma3n:e4b-it-q8_0"), + "the error must name the model the user actually configured: {err}" + ); + assert!( + err.contains("not vision-capable"), + "the error must say what is wrong with it: {err}" + ); + assert!( + err.contains("vision_model_id"), + "the error must name the key to change: {err}" + ); + // `DEFAULT_LOW_VISION_MODEL` is also `VISION_MODEL_SUGGESTIONS[0]`, so + // asserting its absence would assert against the suggestion list + // itself. The contract is the framing: it is offered as one example to + // pick from, not announced as the model that replaced the user's. + assert!( + err.contains("for example"), + "a vision-capable model must be offered as an example to choose, never as a \ + substitute that was already applied: {err}" + ); + assert!( + !err.contains("selected vision model `moondream"), + "the error must name the user's model as the problem, not a suggestion: {err}" + ); + } - // A chat-only id is replaced, and the configured id is reported so the - // caller can explain the swap instead of naming a model out of nowhere. + /// The auto-pull half of P1: several callers feed + /// `effective_vision_model_id` straight into `ensure_ollama_model_available`, + /// so a substituted id here is exactly how an unchosen model got downloaded. + /// Empty keeps those paths off it (and `ensure_ollama_model_available` + /// rejects a blank id rather than pulling a nameless model). + #[test] + fn effective_vision_model_id_is_empty_for_a_chat_only_model() { + let mut config = test_config(); config.local_ai.vision_model_id = "gemma3n:e4b-it-q8_0".to_string(); - let choice = resolve_vision_model_choice(&config).unwrap(); - assert!(vision_models::is_vision_capable(&choice.model)); - assert_eq!(choice.replaced.as_deref(), Some("gemma3n:e4b-it-q8_0")); + assert_eq!( + effective_vision_model_id(&config), + "", + "a chat-only model must not resolve to a substitute that a pull path would download" + ); - // An alias rewrite resolves to a different string but is the same model - // the user asked for, so it must not be reported as a substitution. - config.local_ai.vision_model_id = "moondream".to_string(); - let choice = resolve_vision_model_choice(&config).unwrap(); - assert!(vision_models::is_vision_capable(&choice.model)); - assert_eq!(choice.replaced, None); + // Unchanged for the two states that already worked. + config.local_ai.vision_model_id = String::new(); + assert_eq!(effective_vision_model_id(&config), ""); + config.local_ai.vision_model_id = "llava:7b".to_string(); + assert_eq!(effective_vision_model_id(&config), "llava:7b"); + } + + /// `effective_vision_model_id` and `resolve_vision_model_id` must agree on + /// which models are usable — a non-empty effective id that the resolver + /// rejects (or vice versa) would put status surfaces and request-time + /// behaviour out of sync. + #[test] + fn effective_and_resolved_vision_ids_agree_on_usability() { + let mut config = test_config(); + for candidate in [ + "llava:7b", + "gemma3n:e4b-it-q8_0", + "moondream", + "llama3.2:3b", + "", + ] { + config.local_ai.vision_model_id = candidate.to_string(); + let effective = effective_vision_model_id(&config); + let resolved = resolve_vision_model_id(&config); + assert_eq!( + effective.is_empty(), + resolved.is_err(), + "disagreement for {candidate:?}: effective={effective:?} resolved={resolved:?}" + ); + if let Ok(model) = resolved { + assert_eq!(effective, model); + } + } } #[test] diff --git a/src/openhuman/inference/vision_models.rs b/src/openhuman/inference/vision_models.rs index 7a583729a7..feab6d72e9 100644 --- a/src/openhuman/inference/vision_models.rs +++ b/src/openhuman/inference/vision_models.rs @@ -1,14 +1,38 @@ //! Which local model IDs can actually accept image input. //! -//! Routing a vision request at a chat-only model is **not** a loud failure on -//! Ollama. `POST /api/generate` accepts the `images` array against any model, -//! silently discards it when the model has no vision encoder, and answers from -//! the prompt text alone. The caller gets back a confident, entirely -//! hallucinated description rather than an error (#5146, §Part 1). +//! Every vision path resolves its model through this registry first, so a +//! vision tool-call either reaches a genuinely vision-capable model or surfaces +//! a clear error. See [`crate::openhuman::inference::model_ids`]. //! -//! Every vision path therefore resolves its model through this registry first, -//! so a vision tool-call either reaches a genuinely vision-capable model or -//! surfaces a clear error. See [`crate::openhuman::inference::model_ids`]. +//! ## Why resolve up front rather than let Ollama reject it +//! +//! This module originally claimed that routing a vision request at a chat-only +//! model is *not* a loud failure — that Ollama accepts the `images` array +//! against any model, silently discards it, and answers from the prompt text +//! alone, yielding a confident hallucination. +//! +//! **That was measured against Ollama 0.32.5 and does not hold** (#5146 P5). +//! All three endpoints — `/api/generate`, `/api/chat`, and the OpenAI-compatible +//! `/v1/chat/completions` — reject the request cleanly: +//! +//! ```text +//! HTTP 400 {"error":{"code":400, +//! "message":"Multimodal data provided, but model does not support multimodal requests.", +//! "type":"invalid_request_error"}} +//! ``` +//! +//! The registry is still worth having, for reasons that do not depend on the +//! old claim: +//! +//! - a raw upstream 400 is not an actionable message — resolving first lets us +//! name the model, the capability, and the remedy; +//! - it lets the caller fail *before* spending a model load or a pull; +//! - older Ollama builds, and other OpenAI-compatible local servers reached +//! through the same code path, make no such guarantee. +//! +//! Keep this note accurate: a future reader deciding whether the registry can +//! be deleted must not rely on a silent-discard premise that no current Ollama +//! exhibits. //! //! The families below were verified against the live Ollama library registry //! (`GET https://registry.ollama.ai/v2/library//manifests/` plus the @@ -181,9 +205,10 @@ mod tests { #[test] fn every_suggested_vision_model_is_vision_capable() { - // The suggestions are quoted verbatim in the user-facing error from - // `model_ids::resolve_vision_model_choice`; a chat-only entry here would - // send users to a model that silently ignores their image. + // The suggestions are quoted verbatim in the user-facing errors from + // `model_ids::resolve_vision_model_id` — both the "nothing configured" + // and the "not vision-capable" arms. A chat-only entry here would send + // a user straight back into the error they are trying to escape. for suggestion in VISION_MODEL_SUGGESTIONS { assert!( is_vision_capable(suggestion), diff --git a/src/openhuman/medulla/ops.rs b/src/openhuman/medulla/ops.rs index 23954a1808..1f301438bc 100644 --- a/src/openhuman/medulla/ops.rs +++ b/src/openhuman/medulla/ops.rs @@ -262,12 +262,19 @@ mod tests { /// Serialize env mutation: tests modify process-globals. static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Clearing every base-URL env var no longer produces `MedullaNoBaseUrl`. + /// + /// Since #5245 `resolve::base_url` falls back to + /// `effective_backend_api_url`, which ends at the compiled-in + /// `DEFAULT_API_BASE_URL` — so it never returns `None` and the + /// `NoBaseUrl` arm is unreachable. "No backend configured" stopped being a + /// state the product can be in; the unconfigured state that remains is + /// "signed out", which is what this now pins. The env isolation is kept + /// because it is what proves the fallback, rather than an override, is + /// answering. #[test] fn not_configured_encodes_an_expected_user_state_envelope() { let config = Config::default(); - // No api_url and no env override, so resolution fails on base URL. - // Isolate environment to ensure BACKEND_URL and VITE_BACKEND_URL do not - // provide a fallback through effective_backend_api_url. let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let _env_medulla = EnvGuard::remove(resolve::MEDULLA_BASE_URL_ENV); let _env_backend = EnvGuard::remove("BACKEND_URL"); @@ -282,7 +289,7 @@ mod tests { decoded .data .and_then(|d| d["kind"].as_str().map(String::from)), - Some("MedullaNoBaseUrl".to_string()) + Some("MedullaNoSessionToken".to_string()) ); } @@ -344,6 +351,10 @@ mod tests { .await .expect("status never errors"); assert!(!out.value.configured); - assert_eq!(out.value.reason.as_deref(), Some("MedullaNoBaseUrl")); + // `NoSessionToken`, not `NoBaseUrl` — see + // `not_configured_encodes_an_expected_user_state_envelope`: the base URL + // always resolves to a compiled-in default, so signed-out is the only + // unconfigured state left to report. + assert_eq!(out.value.reason.as_deref(), Some("MedullaNoSessionToken")); } } diff --git a/tests/agent_multimodal_public.rs b/tests/agent_multimodal_public.rs index c0967bd297..178a81f0a5 100644 --- a/tests/agent_multimodal_public.rs +++ b/tests/agent_multimodal_public.rs @@ -22,9 +22,13 @@ fn marker_helpers_cover_mixed_content_and_payload_extraction() { extract_ollama_image_payload("data:image/png;base64,abcd").as_deref(), Some("abcd") ); + // #5146 P6 reversed this: a filesystem path used to be forwarded verbatim as + // if it were image bytes, and Ollama answered `illegal base64 data at input + // byte 19` — an error naming neither the parameter nor the path. Trimming + // still happens, but the trimmed value has to be base64 to be returned. assert_eq!( extract_ollama_image_payload(" /tmp/a.png ").as_deref(), - Some("/tmp/a.png") + None ); let (cleaned_unclosed, refs_unclosed) = parse_image_markers("broken [IMAGE:/tmp/a.png"); assert_eq!(cleaned_unclosed, "broken [IMAGE:/tmp/a.png"); diff --git a/tests/raw_coverage/inference_voice_http_round23_raw_coverage_e2e.rs b/tests/raw_coverage/inference_voice_http_round23_raw_coverage_e2e.rs index 8f06af79f7..d35aef2198 100644 --- a/tests/raw_coverage/inference_voice_http_round23_raw_coverage_e2e.rs +++ b/tests/raw_coverage/inference_voice_http_round23_raw_coverage_e2e.rs @@ -289,7 +289,12 @@ async fn local_service_assets_and_whisper_fallback_use_fake_files_and_binaries() assert!(assets.ollama_available); assert_eq!(assets.chat.state, "ready"); assert_eq!(assets.embedding.state, "ready"); - assert_eq!(assets.vision.state, "ondemand"); + // `ready`, not `ondemand`: the mock's `/api/tags` lists `vision-ready` + // alongside the chat and embedding models asserted above, and the configured + // `vision_model_id` IS `vision-ready`. On-demand vision whose model is + // already pulled reports ready — `VisionMode::Ondemand if vision_ready`. + // The old expectation contradicted this test's own fixture. + assert_eq!(assets.vision.state, "ready"); assert_eq!(assets.stt.state, "ready"); assert_eq!(assets.tts.state, "ondemand");