From a98d8916ec5a35f79e4b1c3c966984c688701623 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 12:19:18 +0530 Subject: [PATCH 1/9] fix(inference): error on a misconfigured vision model instead of substituting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #5253/#5254 for the two defects the Ollama end-to-end pass found (#5146 P1, P6) plus the P5 doc correction. P2 needed no change — see below. P1 — chat-only vision model Setting `local_ai.vision_model_id` to a text-only model produced three surprises in a row and named none of them. `enforce_vision_capability` silently swapped in the default vision model; the caller then auto-pulled that model (~1.7 GB, no visible progress, ~6 minutes); and the substitute answers many prompt phrasings with an empty string, so the user finally saw `ollama vision returned empty content`. Nothing in that chain mentioned the model they had actually configured. Substituting is the wrong shape whichever default is chosen — the user made an explicit choice and it was overridden, the same class of bug as a silent provider switch. `enforce_vision_capability` now returns `Result`, and a chat-only id becomes an actionable error naming the offending model, the capability it lacks, and the remedy — the same treatment the unconfigured case already got, and modelled on the tinyagents Ollama embeddings adapter's missing-model message. Because several callers feed `effective_vision_model_id` straight into `ensure_ollama_model_available`, that function now reports a chat-only model as "" rather than a substitute, which keeps the pull paths off a model nobody asked for. `ensure_ollama_model_available` also rejects a blank id outright instead of `POST /api/pull`-ing a nameless model — the same guard closes the original "nameless pull retried three times" complaint at its root, for every role rather than just vision. `VisionModelChoice` and its `replaced` field go away: they existed to explain a substitution that no longer happens. `DEFAULT_OLLAMA_VISION_MODEL` goes with them — it was only ever the substitute, and there is nothing left to default to. `DEFAULT_LOW_VISION_MODEL` (same value) stays as the `moondream` alias target and low-tier bundled model. P6 — non-base64 image references `extract_ollama_image_payload` returned any non-`data:` string verbatim, so a caller passing a filesystem path — a natural reading of `image_refs` — had that path forwarded as image bytes and got back Ollama's `illegal base64 data at input byte 19`, which names neither the parameter nor the path. Both forms are now validated as base64 (padded or unpadded), and `vision_prompt` says what `image_refs` actually accepts. This changes a documented contract: a bare non-base64 string used to pass through. The existing test asserting that is updated, as is a fixture whose `abcd==` payload is not decodable base64 at all and only passed because nothing validated it. P5 — corrected module doc `vision_models` justified itself with the claim that Ollama silently discards images sent to a chat-only model, yielding a confident hallucination. Measured against Ollama 0.32.5 that is false: `/api/generate`, `/api/chat` and `/v1/chat/completions` all return a clean 400. The registry is still worth having — a raw upstream 400 is not actionable, resolving first avoids a wasted model load or pull, and older builds and other OpenAI-compatible local servers make no such guarantee — so the doc now says that instead. Not changed - P2 (empty key on a fallback role surfacing as a raw provider 401) is already fixed on main: the merged #5254 gained the `implicit_fallback && key.trim().is_empty()` bail in `resolve_cloud_slug` during review, with `a_readable_profile_with_no_stored_key_is_treated_as_missing_credentials` covering it. The report predated that merge. - P4 (local-only inference requiring a backend session) is untouched — it is a product decision, flagged in the PR body rather than changed here. --- src/openhuman/agent/multimodal.rs | 45 ++- src/openhuman/agent/multimodal_tests.rs | 54 ++- .../local/service/ollama_admin/model_pull.rs | 16 + .../local/service/ollama_admin_tests.rs | 43 +++ .../inference/local/service/vision_embed.rs | 152 ++++++--- src/openhuman/inference/model_ids.rs | 321 +++++++++++------- src/openhuman/inference/vision_models.rs | 47 ++- 7 files changed, 490 insertions(+), 188 deletions(-) diff --git a/src/openhuman/agent/multimodal.rs b/src/openhuman/agent/multimodal.rs index bac74e454d..eca86d034f 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,41 @@ fn latest_user_message(messages: &[ChatMessage]) -> Option<&ChatMessage> { messages.iter().rev().find(|m| m.role == "user") } +/// 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 payload = if image_ref.starts_with("data:") { 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; + } + // 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. + if STANDARD.decode(payload).is_err() && STANDARD_NO_PAD.decode(payload).is_err() { + 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..329263e192 100644 --- a/src/openhuman/agent/multimodal_tests.rs +++ b/src/openhuman/agent/multimodal_tests.rs @@ -123,9 +123,52 @@ 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()); +} + +#[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 +179,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/ollama_admin/model_pull.rs b/src/openhuman/inference/local/service/ollama_admin/model_pull.rs index b9761227cd..ca910bc005 100644 --- a/src/openhuman/inference/local/service/ollama_admin/model_pull.rs +++ b/src/openhuman/inference/local/service/ollama_admin/model_pull.rs @@ -58,6 +58,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..89f589d75a 100644 --- a/src/openhuman/inference/local/service/ollama_admin_tests.rs +++ b/src/openhuman/inference/local/service/ollama_admin_tests.rs @@ -1461,3 +1461,46 @@ 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}"); +} diff --git a/src/openhuman/inference/local/service/vision_embed.rs b/src/openhuman/inference/local/service/vision_embed.rs index 7d14691931..acf9ce3420 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 @@ -573,24 +567,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 +601,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 +612,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 +623,71 @@ 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}" + ); + assert!( + !err.contains(crate::openhuman::inference::model_ids::DEFAULT_LOW_VISION_MODEL), + "error must not point the user at a model they never chose: {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..bf9dc8552e 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,57 @@ fn raw_chat_model_id(config: &Config) -> String { raw.to_string() } -/// Resolve the vision model for status / reporting surfaces. +/// Apply the alias rewrite that maps a family name onto the pinned tag we +/// actually ship (`moondream` -> `moondream:1.8b-v2-q4_K_S`). /// -/// 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. -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(); - } +/// 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(); - let resolved = if lower == "moondream:1.8b" || lower == "moondream" { + if lower == "moondream:1.8b" || lower == "moondream" { DEFAULT_LOW_VISION_MODEL } 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, +/// Resolve the vision model for status / reporting surfaces. +/// +/// 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. +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(); + } + enforce_vision_capability(apply_vision_alias(raw)).unwrap_or_default() } -/// 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: 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() { +/// 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): +/// +/// - **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 +261,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 +471,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 +507,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 +520,111 @@ 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}" + ); + assert!( + !err.contains(DEFAULT_LOW_VISION_MODEL), + "the error must not tell the user to pull a model they never chose: {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), From 8a5eba19187327ecc67ea4f81929d542b5b5bb45 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 16:34:32 +0530 Subject: [PATCH 2/9] chore(cargo): refresh the lock for the tinyhumans-sdk bump (#5260) `vendor/tinyhumans-sdk` gained `futures` and `tokio` dependencies, but the submodule bump in #5260 landed without regenerating either `Cargo.lock`. The Rust Quality lane runs `scripts/check-linux-tls-dependencies.sh`, whose `cargo tree --locked` then refuses to proceed: error: cannot update the lock file ... because --locked was passed Main's own CI Lite run skipped the Rust lanes (#5260 touched no Rust source), so this only surfaces on Rust-touching PRs, which build the merge with main. Adds the two missing dependency entries to both Cargo worlds. No version changes. --- Cargo.lock | 4 +++- app/src-tauri/Cargo.lock | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) 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", ] From e11a60550fdd0d13eb640fdf915119218076c45f Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 16:34:44 +0530 Subject: [PATCH 3/9] fix(inference): address review on the vision-misconfig guard (#5146 P1, P6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of these were CI-blocking test breaks that the failing lock gate hid: - `vision_prompt_never_routes_images_at_a_chat_only_model` still asserted the substitution this PR removes (`expect("vision prompt should succeed")` on a config that now errors). Superseded by `chat_only_vision_model_errors_without_substituting_or_pulling`; deleted. - `!err.contains(DEFAULT_LOW_VISION_MODEL)` can never hold: `VISION_MODEL_SUGGESTIONS[0]` *is* `DEFAULT_LOW_VISION_MODEL`, so the suggestion list always puts it in the message. Both copies re-pin the real contract — it is offered as an example to pick, not announced as a substitute that already ran. Behaviour fixes: - `download_asset("vision")` fed the blanked effective id into `ensure_ollama_model_available`, which answered "no vision model is configured" to someone who configured one. Resolve instead, so the error names the model that cannot accept images. - A chat-only vision model no longer aborts the whole bootstrap. `ensure_models_available` returned early, leaving the service `degraded` and skipping the embedding/STT/TTS preloads — punishing chat for a vision-only mistake. The reason is recorded on the status and startup continues. - `effective_vision_model_id` no longer routes through `enforce_vision_capability` only to discard its `Err`: that logged a warning and formatted the full suggestion string on every status poll. - `extract_ollama_image_payload` rejects absolute paths that happen to be valid base64 (`/tmp/foo`). Length is what separates them from a real payload: base64 for a JPEG legitimately begins `/9j/`. The residual ambiguity for base64-shaped relative paths is documented and pinned by a test. --- src/openhuman/agent/multimodal.rs | 36 +++++++++- src/openhuman/agent/multimodal_tests.rs | 41 ++++++++++++ .../inference/local/service/assets.rs | 7 +- .../local/service/ollama_admin/model_pull.rs | 30 +++++++-- .../local/service/ollama_admin_tests.rs | 65 +++++++++++++++++++ .../inference/local/service/vision_embed.rs | 54 +++------------ src/openhuman/inference/model_ids.rs | 29 ++++++++- 7 files changed, 205 insertions(+), 57 deletions(-) diff --git a/src/openhuman/agent/multimodal.rs b/src/openhuman/agent/multimodal.rs index eca86d034f..fb9266d29d 100644 --- a/src/openhuman/agent/multimodal.rs +++ b/src/openhuman/agent/multimodal.rs @@ -199,6 +199,33 @@ 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, of a length base64 permits — `photos/catpics` — is genuinely +/// indistinguishable from a short payload and is still accepted. Tightening +/// that would start rejecting real base64, so it is left alone deliberately. +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. /// @@ -213,7 +240,8 @@ fn latest_user_message(messages: &[ChatMessage]) -> Option<&ChatMessage> { /// 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 { - let payload = 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(',')?; image_ref.split_at(comma_idx + 1).1.trim() } else { @@ -222,6 +250,12 @@ pub fn extract_ollama_image_payload(image_ref: &str) -> Option { 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 diff --git a/src/openhuman/agent/multimodal_tests.rs b/src/openhuman/agent/multimodal_tests.rs index 329263e192..b42c7c573e 100644 --- a/src/openhuman/agent/multimodal_tests.rs +++ b/src/openhuman/agent/multimodal_tests.rs @@ -143,6 +143,47 @@ fn extract_ollama_image_payload_rejects_a_filesystem_path() { 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() { + // 14 characters — `len % 4 == 2`, which unpadded base64 accepts. (A + // 13-character name like `photos/catpic` is `len % 4 == 1`, a length base64 + // never produces, so the decode below already rejects that one.) + assert_eq!( + extract_ollama_image_payload("photos/catpics").as_deref(), + Some("photos/catpics") + ); + assert!(extract_ollama_image_payload("photos/catpic").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()); diff --git a/src/openhuman/inference/local/service/assets.rs b/src/openhuman/inference/local/service/assets.rs index 2c224eed4c..2bf296ad5e 100644 --- a/src/openhuman/inference/local/service/assets.rs +++ b/src/openhuman/inference/local/service/assets.rs @@ -703,7 +703,12 @@ impl LocalAiService { ); } self.ensure_ollama_server(config).await?; - let model = model_ids::effective_vision_model_id(config); + // 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. + let model = model_ids::resolve_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 ca910bc005..2dbb4fd093 100644 --- a/src/openhuman/inference/local/service/ollama_admin/model_pull.rs +++ b/src/openhuman/inference/local/service/ollama_admin/model_pull.rs @@ -26,12 +26,30 @@ 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!( + %err, + "[local_ai] bundled vision model is unusable; continuing without vision" + ); + let mut status = self.status.lock(); + status.vision_state = "missing".to_string(); + status.warning = Some(err); + } + }, } let embedding_model = model_ids::effective_embedding_model_id(config); diff --git a/src/openhuman/inference/local/service/ollama_admin_tests.rs b/src/openhuman/inference/local/service/ollama_admin_tests.rs index 89f589d75a..fa7ec83c15 100644 --- a/src/openhuman/inference/local/service/ollama_admin_tests.rs +++ b/src/openhuman/inference/local/service/ollama_admin_tests.rs @@ -1504,3 +1504,68 @@ async fn ensure_ollama_model_available_rejects_a_blank_model_id() { .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; + + 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}" + ); +} diff --git a/src/openhuman/inference/local/service/vision_embed.rs b/src/openhuman/inference/local/service/vision_embed.rs index acf9ce3420..12ab06820b 100644 --- a/src/openhuman/inference/local/service/vision_embed.rs +++ b/src/openhuman/inference/local/service/vision_embed.rs @@ -470,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] @@ -626,9 +582,15 @@ mod tests { 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(crate::openhuman::inference::model_ids::DEFAULT_LOW_VISION_MODEL), - "error must not point the user at a model they never chose: {err}" + 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), diff --git a/src/openhuman/inference/model_ids.rs b/src/openhuman/inference/model_ids.rs index bf9dc8552e..2ffb528a17 100644 --- a/src/openhuman/inference/model_ids.rs +++ b/src/openhuman/inference/model_ids.rs @@ -233,12 +233,26 @@ fn apply_vision_alias(raw: &str) -> &str { /// 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(); } - enforce_vision_capability(apply_vision_alias(raw)).unwrap_or_default() + let resolved = apply_vision_alias(raw); + if vision_models::is_vision_capable(resolved) { + resolved.to_string() + } else { + String::new() + } } /// Resolve the vision model for a real vision request. @@ -571,9 +585,18 @@ mod tests { 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(DEFAULT_LOW_VISION_MODEL), - "the error must not tell the user to pull a model they never chose: {err}" + !err.contains("selected vision model `moondream"), + "the error must name the user's model as the problem, not a suggestion: {err}" ); } From 9a68340f1572be6645b540d2dcabefe9c0b8647d Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 16:35:37 +0530 Subject: [PATCH 4/9] perf(multimodal): pick the base64 engine by length instead of decoding twice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validating an unpadded payload ran a full decode under `STANDARD`, threw the error away, then decoded the whole thing again under `STANDARD_NO_PAD` — two allocations of a multi-MB image to answer a yes/no question. A complete group (`len % 4 == 0`) is exactly what `STANDARD` accepts, padded or not, so length alone selects the right engine and the second decode disappears. --- src/openhuman/agent/multimodal.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/openhuman/agent/multimodal.rs b/src/openhuman/agent/multimodal.rs index fb9266d29d..7b37fa01cf 100644 --- a/src/openhuman/agent/multimodal.rs +++ b/src/openhuman/agent/multimodal.rs @@ -261,7 +261,18 @@ pub fn extract_ollama_image_payload(image_ref: &str) -> Option { // 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. - if STANDARD.decode(payload).is_err() && STANDARD_NO_PAD.decode(payload).is_err() { + // + // 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)" ); From 4b1bced94121b85e0a1664ede3b76e4f4ca0ed73 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 16:52:24 +0530 Subject: [PATCH 5/9] fix(inference): keep the vision failure reason and resolve before dialling Ollama (#5146 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three review items from the latest pass: - `status.warning` is where `ensure_ollama_model_available` writes its transient "Pulling …" progress, so publishing the vision failure at the point it happened let a later embedding/STT/TTS pull bury it — leaving `vision_state = "missing"` with no explanation of why. The reason is now held and published after every other preload, where last-write-wins is the behaviour we want: a standing configuration problem outlives transient progress text. - `download_asset("vision")` probed the Ollama server before resolving the model id, so a misconfigured id came back as a connection error for a problem that is answerable without the network. Resolve first, then dial. - The new bootstrap warning carries `vision_model_id` and `vision_state` correlation fields alongside the stable `[local_ai]` prefix. `a_later_preload_does_not_bury_the_vision_failure_reason` drives a real embedding pull (the mock lists the model only once pulled, so the download path runs and writes its own warning) and asserts the vision reason is still there. --- .../inference/local/service/assets.rs | 6 +- .../local/service/ollama_admin/model_pull.rs | 20 ++++- .../local/service/ollama_admin_tests.rs | 88 +++++++++++++++++++ 3 files changed, 110 insertions(+), 4 deletions(-) diff --git a/src/openhuman/inference/local/service/assets.rs b/src/openhuman/inference/local/service/assets.rs index 2bf296ad5e..461afefa8a 100644 --- a/src/openhuman/inference/local/service/assets.rs +++ b/src/openhuman/inference/local/service/assets.rs @@ -702,13 +702,17 @@ impl LocalAiService { .to_string(), ); } - self.ensure_ollama_server(config).await?; // 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?; 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 2dbb4fd093..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(); @@ -42,12 +48,13 @@ impl LocalAiService { // 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" ); - let mut status = self.status.lock(); - status.vision_state = "missing".to_string(); - status.warning = Some(err); + self.status.lock().vision_state = "missing".to_string(); + vision_warning = Some(err); } }, } @@ -67,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(()) } diff --git a/src/openhuman/inference/local/service/ollama_admin_tests.rs b/src/openhuman/inference/local/service/ollama_admin_tests.rs index fa7ec83c15..97ef42ae16 100644 --- a/src/openhuman/inference/local/service/ollama_admin_tests.rs +++ b/src/openhuman/inference/local/service/ollama_admin_tests.rs @@ -1569,3 +1569,91 @@ async fn bundled_vision_misconfiguration_does_not_abort_the_rest_of_bootstrap() "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}" + ); +} From fdfb463e502d0dd42b5f8658631e764c56795ce4 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 17:50:34 +0530 Subject: [PATCH 6/9] test(inference): fix two fixtures I got wrong on the vision-misconfig tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both failed on Rust Core Coverage; both were my error, not the code's. - `photos/catpics` is not decodable base64. Length and alphabet are necessary but not sufficient: a partial trailing group must carry zero spare bits, and its final `s` does not, so the decoder rejects it as non-canonical and the ref was returned as `None`. The ambiguity fixture is now `photos/cats1` — 12 characters, a whole number of groups, so canonicality is not in question. The narrower-than-it-looks window is spelled out in both the test and the `looks_like_absolute_path` doc, since "looks like base64" is the exact intuition that produced the bad fixture. - `preload_embedding_model` defaults to TRUE, so `bundled_vision_misconfiguration_does_not_abort_the_rest_of_bootstrap` ran the embedding preload against a mock serving no `/api/pull` and failed with a 404 that had nothing to do with vision. It now disables that preload explicitly; the vision-plus-later-preload interaction has its own test that supplies a working pull route. --- src/openhuman/agent/multimodal.rs | 5 ++++- src/openhuman/agent/multimodal_tests.rs | 18 +++++++++++++----- .../local/service/ollama_admin_tests.rs | 5 +++++ 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/src/openhuman/agent/multimodal.rs b/src/openhuman/agent/multimodal.rs index 7b37fa01cf..ddbf15cb9e 100644 --- a/src/openhuman/agent/multimodal.rs +++ b/src/openhuman/agent/multimodal.rs @@ -219,9 +219,12 @@ const MAX_PATH_SHAPED_REF_LEN: usize = 64; /// does every Windows path (`:` and `\` are likewise outside it). /// /// **Known residual ambiguity:** a relative path built only from base64 -/// characters, of a length base64 permits — `photos/catpics` — is genuinely +/// 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 } diff --git a/src/openhuman/agent/multimodal_tests.rs b/src/openhuman/agent/multimodal_tests.rs index b42c7c573e..60e5e2fbe5 100644 --- a/src/openhuman/agent/multimodal_tests.rs +++ b/src/openhuman/agent/multimodal_tests.rs @@ -174,14 +174,22 @@ fn extract_ollama_image_payload_still_accepts_a_bare_jpeg_payload() { /// `looks_like_absolute_path`. #[test] fn extract_ollama_image_payload_cannot_reject_a_base64_shaped_relative_path() { - // 14 characters — `len % 4 == 2`, which unpadded base64 accepts. (A - // 13-character name like `photos/catpic` is `len % 4 == 1`, a length base64 - // never produces, so the decode below already rejects that one.) + // 12 characters — a whole number of 4-character groups, so it decodes and + // is trivially canonical. assert_eq!( - extract_ollama_image_payload("photos/catpics").as_deref(), - Some("photos/catpics") + 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] diff --git a/src/openhuman/inference/local/service/ollama_admin_tests.rs b/src/openhuman/inference/local/service/ollama_admin_tests.rs index 97ef42ae16..b50368e392 100644 --- a/src/openhuman/inference/local/service/ollama_admin_tests.rs +++ b/src/openhuman/inference/local/service/ollama_admin_tests.rs @@ -1541,6 +1541,11 @@ async fn bundled_vision_misconfiguration_does_not_abort_the_rest_of_bootstrap() // 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; From 6232a4197f59683539f940bf604f732520f611e8 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 18:19:51 +0530 Subject: [PATCH 7/9] test(medulla): pin the unconfigured reason that is actually reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two tests asserted `MedullaNoBaseUrl` and got `MedullaNoSessionToken`, failing Rust Core Coverage on every Rust-touching PR since #5245 — main's own CI Lite run skipped the Rust lanes, so it went unseen there. Not a behaviour choice, which is why this is a test-only change: #5245 made `resolve::base_url` fall back to `effective_backend_api_url`, and that ends at the compiled-in `DEFAULT_API_BASE_URL` (`https://api.tinyhumans.ai`). It cannot return `None`, so the `NoBaseUrl` arm is unreachable and "no backend configured" is no longer a state the product can be in. The unconfigured state that remains is signed-out, which is what these now pin. The env isolation stays: it is what proves the fallback rather than an override is answering. The `NotConfigured::NoBaseUrl` variant is left in place — retiring it is the #5245 owner's call, not a side effect of unbreaking CI. Unrelated to this PR's vision fix; carried here only because it blocks the coverage gate fleet-wide. Happy to move it to its own PR. --- src/openhuman/medulla/ops.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) 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")); } } From aa6a76d724ffabdf1d0686a7d70732e1c5e63a75 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 19:33:17 +0530 Subject: [PATCH 8/9] test(multimodal): update the public path assertion to the P6 contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tests/agent_multimodal_public.rs` still asserted that a bare filesystem path round-trips out of `extract_ollama_image_payload` — the exact behaviour #5146 P6 removes. It is an integration target, so the lib suite passing said nothing about it, and the earlier lib failures were masking it in the coverage lane. Now pins `None`, with the reason inline. Trimming is unchanged; the trimmed value simply has to be base64 to come back. Checked the other two callers in `tests/`: the raw-coverage e2e uses a real padded data URI and a blank string, and `worker_b_domain_e2e` passes an empty `image_refs`, so both already match the new contract. --- tests/agent_multimodal_public.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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"); From 39edb223a23e19d0d792e57888b3412b53bea5a1 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Wed, 29 Jul 2026 20:24:24 +0530 Subject: [PATCH 9/9] test(inference): assets vision state is ready when the mock lists the model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `local_service_assets_and_whisper_fallback_use_fake_files_and_binaries` asserted `vision.state == "ondemand"`, but its own mock `/api/tags` lists `vision-ready` and the config sets `vision_model_id = "vision-ready"`. With the model present, `VisionMode::Ondemand if vision_ready` reports "ready" — the same mock and the same code path that make the `chat` and `embedding` assertions two lines above pass. Pre-existing and main-side, not caused by this PR: the resolution (`assets.rs:23`) and the readiness check (`assets.rs:161`) are byte-identical at the merge base, `effective_vision_model_id("vision-ready")` returns the same id before and after this PR's changes (`is_vision_capable` matches the `vision` marker and was not touched — only its comments were), and this PR edits `assets.rs` solely inside `download_asset`. It surfaced now only because the lock, medulla and multimodal failures were aborting the coverage lane before this target ran. Carried here for the same reason as the other two main-side fixes on this branch: it fails the coverage gate for any PR that touches inference. --- .../inference_voice_http_round23_raw_coverage_e2e.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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");