fix(inference): error on a misconfigured vision model instead of substituting (#5146 P1, P6) - #5262
Conversation
…tituting Follow-up to tinyhumansai#5253/tinyhumansai#5254 for the two defects the Ollama end-to-end pass found (tinyhumansai#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 tinyhumansai#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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVision image inputs now require valid padded or unpadded base64. Vision model resolution no longer silently substitutes chat-only models, and blank model IDs fail before pull requests. Bundled vision failures remain warnings, while status and asset tests reflect updated fallback semantics. ChangesVision validation and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant VisionService
participant ModelResolver
participant PayloadExtractor
participant Ollama
Client->>VisionService: submit image_refs
VisionService->>ModelResolver: resolve vision model
ModelResolver-->>VisionService: usable model or actionable error
VisionService->>PayloadExtractor: validate image_refs
PayloadExtractor-->>VisionService: base64 payload or rejection
VisionService->>Ollama: send vision request
Ollama-->>VisionService: response
VisionService-->>Client: result or validation error
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
bug
rust-core
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a98d8916ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/openhuman/agent/multimodal_tests.rs (1)
137-144: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResidual gap worth documenting: some paths are valid base64.
The rejection works only because these fixtures contain
./-/~. A path made purely of alphabet chars and/— e.g.photos/catpic(13 chars, decodable unpadded) — still passes validation and gets forwarded as image bytes. Not fixable without risking real base64 rejection, but a note (or a#[test]documenting the known limitation) would keep a future reader from over-trusting the "filesystem path is not accepted" wording inextract_ollama_image_payload's doc comment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/multimodal_tests.rs` around lines 137 - 144, Add a focused regression/documentation test near extract_ollama_image_payload_rejects_a_filesystem_path demonstrating that a base64-compatible path such as photos/catpic is accepted due to ambiguity. Update the extract_ollama_image_payload documentation to state that filesystem paths composed solely of valid base64 characters may be indistinguishable from encoded image data, without changing the current validation behavior.src/openhuman/agent/multimodal.rs (1)
225-236: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueValidation decodes the whole payload (twice for unpadded input).
STANDARD.decodeallocates the full decoded image just to be discarded, and for unpadded input it runs a second full decode viaSTANDARD_NO_PAD. For multi-MB images that is exactly the copy the comment says it wants to avoid. A cheaper equivalent is a single alphabet/length check, or reuse one buffer viadecode_slicewithdecoded_len_estimate.Optional; current behavior is correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/agent/multimodal.rs` around lines 225 - 236, The payload validation in the multimodal image-reference parsing flow currently decodes the entire image, potentially twice, despite only needing syntax validation. Replace the `STANDARD.decode`/`STANDARD_NO_PAD.decode` calls with a single allocation-free alphabet and length validation, or reuse one buffer sized via `decoded_len_estimate` and `decode_slice`, while continuing to accept both padded and unpadded Base64 and return `None` for invalid payloads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs`:
- Around line 61-76: Update bootstrap() handling for ensure_models_available
errors in the VisionMode::Bundled branch: when the vision model ID is blank or
unavailable, set vision_state to "missing" and continue bootstrapping instead of
returning early. Preserve existing error handling for other model roles and
continue the remaining preloads and ready-state transition.
In `@src/openhuman/inference/local/service/vision_embed.rs`:
- Around line 570-582: Remove or rewrite the older
vision_prompt_never_routes_images_at_a_chat_only_model test, which still expects
successful execution and substitution to moondream:1.8b-v2-q4_K_S. Align it with
the current resolver behavior by asserting an error for the configured chat-only
vision_model_id, or delete it as superseded by
chat_only_vision_model_errors_without_substituting_or_pulling.
In `@src/openhuman/inference/model_ids.rs`:
- Around line 236-242: Update effective_vision_model_id and the shared
vision-capability logic so status/diagnostic resolution only evaluates whether
the aliased model is vision-capable, without logging or constructing the
request-time suggestion message. Keep warning and message formatting exclusively
in resolve_vision_model_id, while preserving the agreement validated by
effective_and_resolved_vision_ids_agree_on_usability.
---
Nitpick comments:
In `@src/openhuman/agent/multimodal_tests.rs`:
- Around line 137-144: Add a focused regression/documentation test near
extract_ollama_image_payload_rejects_a_filesystem_path demonstrating that a
base64-compatible path such as photos/catpic is accepted due to ambiguity.
Update the extract_ollama_image_payload documentation to state that filesystem
paths composed solely of valid base64 characters may be indistinguishable from
encoded image data, without changing the current validation behavior.
In `@src/openhuman/agent/multimodal.rs`:
- Around line 225-236: The payload validation in the multimodal image-reference
parsing flow currently decodes the entire image, potentially twice, despite only
needing syntax validation. Replace the
`STANDARD.decode`/`STANDARD_NO_PAD.decode` calls with a single allocation-free
alphabet and length validation, or reuse one buffer sized via
`decoded_len_estimate` and `decode_slice`, while continuing to accept both
padded and unpadded Base64 and return `None` for invalid payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 310a531d-4ca5-4982-baa6-00130f6f9267
📒 Files selected for processing (7)
src/openhuman/agent/multimodal.rssrc/openhuman/agent/multimodal_tests.rssrc/openhuman/inference/local/service/ollama_admin/model_pull.rssrc/openhuman/inference/local/service/ollama_admin_tests.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/inference/model_ids.rssrc/openhuman/inference/vision_models.rs
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| src/openhuman/inference/model_ids.rs | Core logic change: enforce_vision_capability now returns Result instead of substituting; VisionModelChoice/DEFAULT_OLLAMA_VISION_MODEL removed; apply_vision_alias extracted; effective_vision_model_id returns empty string for chat-only models; resolve_vision_model_id replaces resolve_vision_model_choice. Tests rewritten to match new semantics. |
| src/openhuman/agent/multimodal.rs | Adds base64 validation to extract_ollama_image_payload — absolute paths caught by length-gated prefix check, data URIs and bare strings validated via STANDARD/STANDARD_NO_PAD decode. Engine selection by len % 4 avoids double-decoding large payloads. |
| src/openhuman/inference/local/service/ollama_admin/model_pull.rs | Blank-model guard added at the top of ensure_ollama_model_available; VisionMode::Bundled arm now soft-fails for misconfigured vision models instead of aborting the entire bootstrap. |
| src/openhuman/inference/local/service/vision_embed.rs | Stale substitution tests deleted and replaced with chat_only_vision_model_errors_without_substituting_or_pulling (asserts pull count == 0) and non_base64_image_reference_is_rejected_with_guidance. |
| src/openhuman/inference/local/service/assets.rs | Replaces effective_vision_model_id with resolve_vision_model_id(config)? in the on-demand vision availability check. |
| src/openhuman/inference/vision_models.rs | Module-doc corrected: removes false claim that Ollama silently discards images for chat-only models (measured false against Ollama 0.32.5), replaces with accurate justifications. |
| src/openhuman/agent/multimodal_tests.rs | Seven new/updated tests covering data URI validation, path rejection, JPEG disambiguation by length, residual ambiguity documentation, and padded/unpadded bare base64. |
| src/openhuman/inference/local/service/ollama_admin_tests.rs | Two new integration tests: blank-model guard (no mock needed) and bundled vision misconfiguration not aborting bootstrap. |
Reviews (2): Last reviewed commit: "perf(multimodal): pick the base64 engine..." | Re-trigger Greptile
…sai#5260) `vendor/tinyhumans-sdk` gained `futures` and `tokio` dependencies, but the submodule bump in tinyhumansai#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 (tinyhumansai#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.
…ansai#5146 P1, P6) 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.
…g twice 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.
|
Pushed fixes for the red CI and every review item. CI failure — root cause was not in this PR
Fixed by merging main and adding the two missing entries to both Cargo worlds — no version changes. Verified locally: Two review findings were genuine CI-blocking test breaksThe lock gate failed before the test lanes ran, so these were hidden:
Behaviour fixes
|
|
@coderabbitai review |
|
To use Codex here, create a Codex account and connect to github. |
|
🐇🔍 ✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
The full diff is in good shape. Here's what I verified across the push. CI / Cargo.lock — root-cause analysis is correct. A submodule bump that skips Rust lanes propagates silently until a Rust-touching PR merges with main. The two missing entries are the right surgical fix. Deleted test (
Bootstrap abort (
Length-based path rejection — Length-based engine selection —
Everything checks out. The fixes are correct, test coverage is comprehensive, and the |
coderabbitai
Bot
added
the
agent
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/openhuman/inference/local/service/ollama_admin/model_pull.rs (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a correlation field to this new warning.
The prefix is stable, but this bootstrap event cannot be correlated with adjacent flow logs. As per coding guidelines, use
log/tracingatdebug/tracewith stable prefixes and correlation fields for new or changed flows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs` around lines 44 - 47, Update the warning in the model-pull error path to include the applicable stable correlation field alongside %err, while preserving the existing "[local_ai] bundled vision model is unusable; continuing without vision" prefix and behavior. Use the correlation value already available in the surrounding bootstrap flow rather than introducing a new identifier.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/inference/local/service/assets.rs`:
- Around line 706-711: Move the model_ids::resolve_vision_model_id(config)? call
ahead of ensure_ollama_server in the vision-model setup flow, then pass the
resolved model into the availability check. Preserve the existing actionable
configuration error for blank or chat-only IDs and avoid probing Ollama when
resolution fails.
In `@src/openhuman/inference/local/service/ollama_admin_tests.rs`:
- Around line 1533-1550: Update the test around
LocalAiService::ensure_models_available to preserve the pre-existing
OPENHUMAN_OLLAMA_BASE_URL value. Replace manual set_var/remove_var cleanup with
an RAII environment guard that restores the prior value on drop, including
during unwinding.
In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs`:
- Around line 29-52: Update the vision error handling in the VisionMode::Bundled
branch so the actionable err is preserved when later embedding, STT, or TTS
preloads succeed and update status.warning. Store the vision-specific warning
separately or merge it with subsequent warnings, while retaining vision_state =
"missing"; extend the relevant test to cover a successful later preload and
verify the vision failure reason remains available.
---
Nitpick comments:
In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs`:
- Around line 44-47: Update the warning in the model-pull error path to include
the applicable stable correlation field alongside %err, while preserving the
existing "[local_ai] bundled vision model is unusable; continuing without
vision" prefix and behavior. Use the correlation value already available in the
surrounding bootstrap flow rather than introducing a new identifier.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 86d55cb8-bb3d-4b48-807e-b94c772e7a56
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
src/openhuman/agent/multimodal.rssrc/openhuman/agent/multimodal_tests.rssrc/openhuman/inference/local/service/assets.rssrc/openhuman/inference/local/service/ollama_admin/model_pull.rssrc/openhuman/inference/local/service/ollama_admin_tests.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/inference/model_ids.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- src/openhuman/agent/multimodal_tests.rs
- src/openhuman/agent/multimodal.rs
- src/openhuman/inference/local/service/vision_embed.rs
- src/openhuman/inference/model_ids.rs
…lling Ollama (tinyhumansai#5146 P1) 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.
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
✅ Action performedReview finished.
|
… tests 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.
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Two tests asserted `MedullaNoBaseUrl` and got `MedullaNoSessionToken`, failing Rust Core Coverage on every Rust-touching PR since tinyhumansai#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: tinyhumansai#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 tinyhumansai#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.
|
Update on the two
Fixed in Flagging clearly: this is unrelated to this PR's vision fix and is carried here only because it fails Rust Core Coverage on every Rust-touching openhuman PR (main's own CI Lite run skips the Rust lanes, so it is invisible there). Same reason this PR carries the (edited: the original had shell-escaping artefacts — content unchanged) |
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
✅ Action performedFull review finished. |
coderabbitai
Bot
removed
the
agent
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/openhuman/inference/local/service/ollama_admin/model_pull.rs (1)
22-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe buffered vision warning is silently lost if a later preload fails.
vision_warningis only written tostatus.warningat line 80-82, which is reached only if the embedding/STT/TTS preloads above it all succeed (each uses?). If, say, the embedding preload fails and returns early, the function exits via?before ever reaching that final block —vision_statestays"missing"(set earlier) but the actionable reason captured invision_warningis discarded entirely, even though the whole point of buffering it was so it "survives" until the end.This narrows to a specific but real scenario: a chat-only
vision_model_idand an embedding/STT/TTS preload failure occurring together. The vision reason is still recoverable at request time viaresolve_vision_model_id, so this isn't a total loss of information, but the immediate bootstrap status becomes less informative than intended.♻️ Publish the vision warning regardless of how the remaining preloads finish
- let embedding_model = model_ids::effective_embedding_model_id(config); - if config.local_ai.preload_embedding_model { - self.ensure_ollama_model_available(config, &embedding_model, "embedding") - .await?; - self.status.lock().embedding_state = "ready".to_string(); - } - - if config.local_ai.preload_stt_model { - self.ensure_stt_asset_available(config).await?; - } - - if config.local_ai.preload_tts_voice { - 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(()) + let remaining: Result<(), String> = async { + let embedding_model = model_ids::effective_embedding_model_id(config); + if config.local_ai.preload_embedding_model { + self.ensure_ollama_model_available(config, &embedding_model, "embedding") + .await?; + self.status.lock().embedding_state = "ready".to_string(); + } + + if config.local_ai.preload_stt_model { + self.ensure_stt_asset_available(config).await?; + } + + if config.local_ai.preload_tts_voice { + self.ensure_tts_asset_available(config).await?; + } + + Ok(()) + } + .await; + + // Publish the vision reason regardless of whether a later preload also + // failed — losing it here would drop the only place bootstrap explains + // *why* vision is missing. + if let Some(err) = vision_warning { + self.status.lock().warning = Some(err); + } + + remainingA regression test exercising a failing later preload (mirroring
a_later_preload_does_not_bury_the_vision_failure_reasonbut with the/api/pullmock returning an error) would catch this.As per path instructions,
**/*.{ts,tsx,rs}: "Untested code is incomplete; add tests for new or changed behavior."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs` around lines 22 - 84, Ensure the buffered warning from the vision handling in the model preload function is published to status.warning even when a later embedding, STT, or TTS preload fails; replace the early-return propagation around those preload operations with cleanup-aware handling that records the failure, publishes vision_warning, then returns the preload error. Add a regression test covering a later preload failure while a bundled vision model resolution also fails, and verify the final status retains the actionable vision warning.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/openhuman/inference/local/service/ollama_admin/model_pull.rs`:
- Around line 22-84: Ensure the buffered warning from the vision handling in the
model preload function is published to status.warning even when a later
embedding, STT, or TTS preload fails; replace the early-return propagation
around those preload operations with cleanup-aware handling that records the
failure, publishes vision_warning, then returns the preload error. Add a
regression test covering a later preload failure while a bundled vision model
resolution also fails, and verify the final status retains the actionable vision
warning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4626c74-eb6e-4d96-bdcf-34bbe818d4e3
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockapp/src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/openhuman/agent/multimodal.rssrc/openhuman/agent/multimodal_tests.rssrc/openhuman/inference/local/service/assets.rssrc/openhuman/inference/local/service/ollama_admin/model_pull.rssrc/openhuman/inference/local/service/ollama_admin_tests.rssrc/openhuman/inference/local/service/vision_embed.rssrc/openhuman/inference/model_ids.rssrc/openhuman/inference/vision_models.rssrc/openhuman/medulla/ops.rs
coderabbitai
Bot
added
the
agent
`tests/agent_multimodal_public.rs` still asserted that a bare filesystem path round-trips out of `extract_ollama_image_payload` — the exact behaviour tinyhumansai#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.
|
@coderabbitai review |
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
✅ Action performedReview finished.
|
… model
`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.
There was a problem hiding this comment.
M3gA-Mind has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Follow-up to #5253 / #5254, from the Ollama end-to-end test pass on #5146 Part 1.
Fixed here: P1 (vision misconfiguration) and P6 (non-base64 image references), plus the P5 doc correction.
Already fixed on main: P2 — see below, no change needed.
Deliberately not changed: P4 — a product decision, flagged at the end.
Summary
local_ai.vision_model_idno longer gets silently swapped for a different model, auto-pulled (~1.7 GB, no visible progress), and then failed withollama vision returned empty content. It is now an actionable error naming the model the user actually configured.ensure_ollama_model_availablerefuses a blank model id outright instead ofPOST /api/pull-ing a nameless model — one guard covering every role, which also closes the original "nameless pull retried three times" complaint at its root.image_refsno longer reaches Ollama as if it were image bytes; both payload forms are validated as base64 and the caller is told what the parameter accepts.P1 — a chat-only vision model was silently substituted, then auto-pulled
Setting
vision_model_idto a text-only model produced three surprises in a row and named none of them:enforce_vision_capabilitysilently swapped in the default vision model;ollama vision returned empty content.Nothing in that chain mentioned the model they had configured. Observed end to end against Ollama 0.32.5:
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 (#5146 §2.1). So:
enforce_vision_capabilityreturnsResult; a chat-only id becomes an actionable error naming the model, the capability it lacks, and the remedy — the same treatment the unconfigured case already got. The wording is modelled on the tinyagents Ollama embeddings adapter, which already gets this right (Ollama embedding model \x` is not installed at . Run `ollama pull x` …`).effective_vision_model_idreports a chat-only model as""rather than a substitute. This matters beyond reporting: six callers feed it straight intoensure_ollama_model_available, and that is precisely how a model nobody chose got downloaded.ensure_ollama_model_availablerejects a blank id before any network work, naming the unconfigured role.VisionModelChoiceand itsreplacedfield are removed — they existed to explain a substitution that no longer happens.DEFAULT_OLLAMA_VISION_MODELgoes with them: it was only ever the substitute, so there is nothing left to default to.DEFAULT_LOW_VISION_MODEL(same value) stays as themoondreamalias target and the low-tier bundled model.P2 — already fixed on main, no change here
The report found that
lookup_key_for_slugreturnsOk("")for a missing key, so #5254'smap_errwrap never fired and the user still saw a rawanthropic returned HTTP 401.That was fixed before #5254 merged. The merged version gained the guard during review (
resolve_cloud_slug):…together with
role_uses_implicit_cloud_fallback, so an explicitly-routed provider still builds without a stored key, anda_readable_profile_with_no_stored_key_is_treated_as_missing_credentialspins the message, the named local chat model, and the scope carve-out. The report predated that merge; no second guard was added here.P6 — a filesystem path was forwarded as image bytes
extract_ollama_image_payloadreturned any non-data:string verbatim, so a path — a natural reading ofimage_refs— was sent to Ollama as image data:That error names neither the parameter nor the path, and points at Ollama rather than the caller. Both forms are now validated as base64 (padded and unpadded, so producers that omit
=are not newly rejected), andvision_promptreports whatimage_refsaccepts.Contract change, called out deliberately: a bare non-base64 string used to pass through, and the existing test asserting that is updated. The
extract_ollama_image_payload_supports_data_urisfixture also changesabcd==→YWJjZA==, becauseabcd==is not decodable base64 at all (6 chars) and only ever passed because nothing validated it.P5 — corrected module doc
vision_modelsjustified itself by claiming 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/chatand/v1/chat/completionsall reject cleanly:The registry is still worth keeping, for reasons that do not depend on the old claim: a raw upstream 400 is not actionable, resolving up front avoids a wasted model load or pull, and older builds — plus other OpenAI-compatible local servers reached through the same code path — make no such guarantee. The doc now says that, so nobody later deletes the registry on a false premise.
P4 — flagged, not changed (product call)
try_create_local_runtime_chat_modelpassesrequire_session: true, so a local-only Ollama user with no backend session cannot chat at all:Storing an
app-sessiontoken does not help —scheduler_gate::is_signed_out()short-circuits and only clears via backend revalidation. The code comment frames the gate as deliberate anti-abuse, so this may well be intended policy. But as shipped it means a signed-out user cannot use a local model, which presents as "Ollama is broken". Left untouched — this needs a product decision, not a patch.Impact
moondreamalias all behave exactly as before.effective_vision_model_idkeeps itsStringsignature, so none of its six callers changed.Related
N/A: #5146 covers further parts (1.x, 2.2, 2.3, 3, 4, 5) beyond this fix, so it stays open.Submission Checklist
moondreamalias still resolving, a pull-counter test asserting zero downloads for a misconfigured vision model, the blank-id pull guard across roles, and four P6 cases (path, non-base64 data URI, padded/unpadded bare base64, trimming). Three tests that asserted the old substituting behaviour were rewritten rather than deleted.src/openhuman/inference/**andsrc/openhuman/agent/multimodal*, all covered by the tests above. Not measured locally: per the task constraint no cargo command was run. CI'srust-core-coverage/coverage-gateare the arbiter.N/A: error-handling correction, no feature row added, removed or renamed.## Related—N/A: no feature IDs affected.N/A: not a release-cut surface.Closes #NNN—N/A: #5146 stays open, see Related.AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/5146-vision-misconfig-and-fallback-keya98d8916eValidation Run
pnpm --filter openhuman-app format:check—N/A: no files under app/ changed.pnpm typecheck—N/A: no TypeScript changed.model_ids.rs,vision_embed.rs,ollama_admin_tests.rs,multimodal_tests.rs.N/A: app/src-tauri unchanged.Validation Blocked
command:cargo check/cargo test/cargo fmterror:cargo commands were prohibited by the task directive for this change.impact:nothing here is compiler-checked. Two hazards were found and pre-empted by inspection:DEFAULT_OLLAMA_VISION_MODELwould have become test-only and trippeddead_code(it is removed, since it no longer has a production role), and the new P6 test would have dialled a real Ollama because the payload check runs after model availability (it now uses a mock listing the model as installed). Reviewers may want to look hardest at the elided lifetime onapply_vision_alias(&str) -> &strand the axummoveclosure capturing anArc<AtomicUsize>in the pull-counter test.Behavior Changes
vision_model_iderrors instead of substituting-and-pulling; a blank model id errors instead of being pulled; a non-base64image_refsentry is rejected with guidance instead of forwarded.ollama vision returned empty content, the user is told which configured model cannot accept images and what to switch to.Parity Contract
moondream/moondream:1.8balias rewrite, the tier gate (vision_mode_for_config), andeffective_vision_model_id'sStringsignature and its six call sites are all unchanged.effective_and_resolved_vision_ids_agree_on_usability(the two resolvers cannot drift),resolve_vision_model_id_still_applies_the_moondream_alias,chat_only_vision_model_errors_without_substituting_or_pulling(asserts a pull count of zero), andensure_ollama_model_available_rejects_a_blank_model_id.Duplicate / Superseded PR Handling
Summary by CodeRabbit
image_refsguidance.ollama pullguidance.