fix(inference): error on a misconfigured vision model instead of substituting (#5146 P1, P6)

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<String> {
-    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

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 =

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?;
             }

src/openhuman/inference/local/service/ollama_admin/model_pull.rs

@@ -19,19 +19,44 @@ 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<String> = None;
+
         match presets::vision_mode_for_config(&config.local_ai) {
             VisionMode::Disabled => {
                 self.status.lock().vision_state = "disabled".to_string();
             }
             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(());