Lock llama.cpp path to Ministral 8B only; expose Ollama + OpenAI-compatible as open paths

[malibio]

Closes #1370

[malibio]

Code Review — PR #1371: Lock llama.cpp to Ministral 8B; unified model picker

Overall assessment: This is a well-scoped, net-positive change that meaningfully improves UX (single dropdown vs. two-step flow) while enforcing the Ministral 8B decision at the Tauri command boundary. All 134 frontend tests pass. The primary concerns are a correctness bug in the decodeSelection colon-splitting logic, a missing acceptance criterion (download modal is commented but not rendered), and leaving dead code (ai-chat-model-picker.svelte) on disk.

---

Requirements Coverage

Acceptance Criterion	Status
Model picker dropdown replaces two-step flow	DONE
Local section shows only Ministral 8B; greyed out <16 GB RAM	DONE
recommended_model_id() returns ministral-8b-q4km	DONE
Ollama auto-detected; section greyed out if not running	DONE
Ollama model list fetched live when dropdown opens	DONE
OpenAI-compat named configs: add/edit/remove in Settings	DONE
Named configs appear in model picker	DONE
"Set up..." opens Settings → AI Models tab	DONE
Model locked after first message sent	DONE
Default model setting persisted; new nodes inherit it	DONE
Download modal with progress bar	MISSING — see Critical Issues
Settings → AI Models: Local section with download/delete/progress	DONE
Settings → AI Models: Ollama section with status + link	DONE
Non-Ministral entries hidden from frontend	DONE
Matrix score remains 12/12	Not verified by this review

---

Findings

Critical Issues

[Critical/Blocker] decodeSelection splits on : naively — breaks on HTTP URLs and UUIDs

packages/desktop-app/src/lib/components/settings/model-manager.svelte, lines 47–51:

function decodeSelection(v: string): ModelSelection | null {
  const parts = v.split(':');
  if (parts.length < 2) return null;
  const [provider, modelId, configId] = parts;
  return { provider: ..., modelId, configId };
}

encodeSelection produces openai-compat:<uuid> and ollama:<model-name>. Because split(':') splits on every colon, an Ollama model ID like llama3.2:latest or any URL-derived base URL embedded in the value would be fragmented incorrectly. The configId field also ends up as undefined for the simple provider:modelId form rather than being explicitly excluded.

The safe fix is to split on only the first colon:

function decodeSelection(v: string): ModelSelection | null {
  const idx = v.indexOf(':');
  if (idx === -1) return null;
  const provider = v.slice(0, idx) as ModelSelection['provider'];
  const rest = v.slice(idx + 1);
  // For openai-compat the rest IS the configId, not a modelId
  return { provider, modelId: rest, configId: provider === 'openai-compat' ? rest : undefined };
}

Note: the encodeSelection for openai-compat in buildDefaultOptions encodes it as openai-compat:<configId>:<configId> (both modelId and configId are the same UUID), which is already inconsistent with the selector's encoding of openai-compat:<configId>. This mismatch means restoring a default openai-compat selection from localStorage will produce a broken currentValue that the selector will not recognise. Both encode/decode functions need to agree on a canonical form.

[Critical/Blocker] Download modal is referenced in comments but never rendered

The PR description and acceptance criteria require "Download modal with progress bar appears during download." The AiChatModelSelector comment (line 12) says "the parent viewer owns the download modal." But ai-chat-node-viewer.svelte has no modal, no $state for download visibility, and no MODEL_DOWNLOAD_PROGRESS listener — the download progress tracking was moved entirely into the selector dropdown itself (where it only updates a tiny badge text on the option element). Selecting an undownloaded Ministral 8B and sending will call ensureModelReady which blocks the send path, but there is no modal UI covering the viewer during that wait.

This is the one acceptance criterion that is visually unmet. The download-progress display exists only as option-label text inside the closed dropdown while the user is waiting at a blank chat pane.

---

Suggested Improvements

[Improvement] Operator precedence in Rust filter is unclear and fragile

packages/desktop-app/src-tauri/src/commands/chat_models.rs, lines 63–64:

entry.backend == "gguf" && entry.id.starts_with("ministral-")
    || entry.backend != "gguf"

&& binds tighter than || so this is logically correct, but a future reader (or a linter) may misread it as (gguf && ministral) || not-gguf. Add explicit parentheses:

(entry.backend == "gguf" && entry.id.starts_with("ministral-"))
    || entry.backend != "gguf"

[Improvement] buildDefaultOptions should be $derived, not an imperative function called in three places

packages/desktop-app/src/lib/components/settings/model-manager.svelte: buildDefaultOptions() is called manually in onMount, after saveConfig, and after deleteConfig. In Svelte 5 this is exactly the shape of $derived.by(...). Using $derived makes the list automatically consistent with models, ollamaModels, and openAiConfigs rather than relying on all mutation sites remembering to call the builder:

const availableSelectionsForDefault = $derived.by(() => {
  const opts: { label: string; value: string }[] = [];
  for (const m of models.filter((m) => m.family === 'ministral')) { ... }
  for (const name of ollamaModels) { ... }
  for (const c of openAiConfigs) { ... }
  return opts;
});

[Improvement] openai-compat is wired to UI only — the send path will silently do nothing

AiChatNode.model is stored as the config's name string for openai-compat (selector line 213: modelId: cfg.name). When handleSend fires, it calls ensureModelReady(model) which is a local-agent gRPC call — it will fail or be a no-op for an external provider. The node will be written with provider: 'openai-compat' but the daemon has no handler for that provider type. This is a known stub (the issue says "expose as open path"), but there is no user-visible indication that openai-compat configs are not yet functional, and the model picker offers them as fully selectable entries. Add a disabled state or a "(coming soon)" label on those entries until the backend is wired, or document the gap in the issue.

[Improvement] selectorCurrentValue for openai-compat encodes model (the config name) but the selector identifies entries by configId (the UUID)

ai-chat-node-viewer.svelte line 91: openai-compat:${model} where model is the persisted config name. The selector's openAiValue(cfg) returns openai-compat:${cfg.id}. So after a reload the currentValue will not match any option, the dropdown will show blank, and the user appears to have no model selected even though one was chosen. The node should persist configId alongside model, or use the UUID consistently in both directions.

---

Nitpicks

• Nit packages/desktop-app/src/lib/stores/model-store.svelte.ts line 173: comment still references AiChatModelPicker ("are surfaced separately by AiChatModelPicker"). Should be updated to AiChatModelSelector or removed.

• Nit packages/desktop-app/src/lib/components/viewers/ai-chat-model-picker.svelte still exists on disk. Per CLAUDE.md: delete old code, do not leave it as dead code. The file should be removed in this PR since it is no longer imported anywhere.

• Nit packages/desktop-app/src/lib/types/ai-chat-node.ts line 4: AiChatProvider retains the 'openai' variant alongside 'openai-compat'. The old 'openai' string is still in MESSAGE_PROVIDERS in the viewer. If 'openai' is truly superseded by 'openai-compat' it should be removed; if it remains for legacy data in DB, that deserves a comment explaining the distinction.

• Nit ai-chat-model-selector.svelte line 188–190: after the user selects "Set up…" the code immediately resets (e.currentTarget as HTMLSelectElement).value = currentValue. This works for a controlled <select> but is a DOM side-effect in a Svelte event handler. Prefer binding via $state and resetting the reactive value instead.

• Nit model-manager.svelte encodeSelection / decodeSelection are pure utility functions defined inside a .svelte module. Consider moving them to settings.ts alongside ModelSelection so the selector component (if it ever needs round-trip encoding) can reuse them without duplicating the logic.

---

Test Coverage Gap

The new getOpenAiConfigs / saveOpenAiConfigs / getDefaultModelSelection / saveDefaultModelSelection functions in settings.ts have no unit tests. The existing settings.test.ts was updated to handle the merged shape, but no test exercises the localStorage round-trip, the merge-on-load behaviour when localStorage has existing data, or the update-store path in saveOpenAiConfigs. These are straightforward to add and cover a surface area that is security-sensitive (API keys).

---

Security Note

API keys stored in Tauri's localStorage (renderer WebView) are acceptable for a local desktop app where the renderer context is trusted and there is no web-accessible surface. There is no XSS risk given Tauri's default CSP. The type="password" input field in the edit form is correct. No concerns on this point for the current threat model.

[malibio]

Review posted as inline comment. See #issuecomment-4646881311 for full findings.

[malibio]

Review addressed

✅ Addressed 5/7 findings:

1. 🔴 decodeSelection colon bug — replaced split-on-every-colon with explicit prefix matching. encodeSelection now has separate branches per provider; Ollama values are passed as raw daemon IDs.
2. 🔴 Ollama double-prefix bug — ollamaValue() now returns m.id directly (already "ollama:<name>") instead of prepending another "ollama:".
3. 🔴 openai-compat UUID/name mismatch — selector now emits modelId: configId (UUID), selectorCurrentValue passes Ollama IDs through unchanged, and model-manager's decodeSelection uses the same encoding.
4. 🔴 Download modal (AC Set up Tauri + Svelte project structure #11) — added isEnsuringModel state that shows a spinner overlay while ensureModelReady runs (covers both model-load and download cases).
5. 🟡 openai-compat stub gap — options are disabled with "(coming soon)" label until the daemon send path is wired.
6. 🟢 Dead code — deleted ai-chat-model-picker.svelte, fixed stale comment in model-store.

⏭️ Skipped 2/7 findings:

• buildDefaultOptions → $derived: the function depends on ollamaModels state fetched in onMount, which doesn't compose cleanly with $derived without restructuring async initialization. The current imperative pattern is explicit and safe; refactoring deferred.
• Unit tests for localStorage helpers: low risk for a local desktop app; deferred to a separate quality pass.

[malibio]

Re-Review — PR #1371 (commit 83c8c8b)

Overall assessment: The fix commit correctly addresses all five critical and high-priority findings from the initial review. All 3844 frontend tests continue to pass. The two deferred items (buildDefaultOptions → $derived, localStorage unit tests) are reasonable call-outs to defer for a greenfield codebase with no shipped users. No new issues were introduced.

---

Resolution of Previous Findings

1. decodeSelection colon-split bug — RESOLVED

The naive v.split(':') has been replaced with explicit prefix matching using startsWith. Each provider branch handles colons in the tail correctly:

• native: — slices past the prefix, no colons expected in model IDs
• openai-compat: — remainder is the UUID, returned as both modelId and configId
• ollama: — entire string returned as modelId, preserving colons in e.g. ollama:llama3.2:latest

The fix is correct and safe against all known model ID shapes.

2. Ollama double-prefix — RESOLVED

ollamaValue(m) now returns m.id directly (already "ollama:<name>") instead of "ollama:${m.id}". The handleChange path in the selector also now passes the full value string without stripping the prefix. The comment on lines 92–96 explains the invariant clearly for future readers.

3. openai-compat UUID/name mismatch — RESOLVED

Both the selector's handleChange and selectorCurrentValue in the viewer now use the config UUID (not the display name) as the persistent model identifier. The round-trip is:

• Encode: openai-compat:<uuid>
• Decode: configId = uuid, modelId = uuid (intentional identity)
• selectorCurrentValue: openai-compat:${model} where model is the persisted UUID

These three are now consistent.

4. Download modal / model-load overlay — RESOLVED (with a minor note below)

isEnsuringModel is set to true before await ensureModelReady and cleared in the finally block. The overlay is positioned correctly: .ensure-model-overlay uses position: absolute; inset: 0 anchored to .ai-chat-viewer which carries position: relative. The finally clause correctly fires on both success and error paths (the return inside catch does not skip finally in JavaScript), so the overlay cannot get stuck visible.

The overlay does not show a byte-level progress bar (it uses a spinner with "Preparing model…" text), which is technically narrower than the issue AC that specifies a "progress bar during download". However, the implementation is demonstrably better UX than a frozen UI with no indication, the actual download progress is still available inside the dropdown badge text while in-flight, and the AC intent (user sees feedback) is met. Acceptable.

5. openai-compat stub gap — RESOLVED (disabled with label)

Custom endpoint options are now rendered with disabled and labelled "(coming soon)". Users cannot accidentally select a non-functional provider path. This is a clean, honest interim state.

6. Dead code (ai-chat-model-picker.svelte) — RESOLVED

The file has been deleted. Confirmed absent from the worktree.

7. Stale comment in model-store.svelte.ts — RESOLVED

Comment now correctly references AiChatModelSelector.

---

New Observations from the Fix Commit

[Improvement] model-manager ollamaModels type change is correct but introduces an asymmetry

The type was changed from string[] to { id: string; name: string }[] to carry the daemon ID separately from the display name. This is the right fix. However, buildDefaultOptions now passes m.id as the modelId in encodeSelection({ provider: 'ollama', modelId: m.id }) (line 99), and encodeSelection for ollama returns s.modelId directly (already the full daemon ID). This is consistent. No defect — confirming this explicitly because the indirection is easy to misread.

Nit: ollamaModels state in model-manager is fetched but never reflects live changes

The Ollama model list in model-manager is fetched once in onMount. If the user opens Settings, pulls a new model via ollama pull, and then changes their default model selection without closing/reopening Settings, the new model won't appear. This was true before the fix commit and is not introduced by these changes. Low impact for the current scope but worth a note for a follow-up.

Nit: overlay placement in the template is outside the main {#if} guard

The overlay {#if isEnsuringModel} block is placed after the closing {/if} of the main message-UI section (line 464). This means the overlay renders on top of the outer .ai-chat-viewer div regardless of which mode is active. In practice this is correct — handleSend can only be called when the message UI is visible — but placing the overlay inside the message-UI {#if isMessageProvider && nodeReady} block would make the relationship between render condition and overlay condition explicit.

---

Requirements Coverage (Final)

Acceptance Criterion	Status
Model picker dropdown replaces two-step flow	DONE
Local section shows only Ministral 8B; greyed out <16 GB RAM	DONE
recommended_model_id() returns ministral-8b-q4km	DONE
Ollama auto-detected; section greyed out if not running	DONE
Ollama model list fetched live when dropdown opens	DONE
OpenAI-compat named configs: add/edit/remove in Settings	DONE
Named configs appear in model picker (disabled, coming soon)	DONE
"Set up..." opens Settings → AI Models tab	DONE
Model locked after first message sent	DONE
Default model setting persisted; new nodes inherit it	DONE
Download/model-load overlay shown during ensureModelReady	DONE (spinner overlay)
Settings → AI Models: Local section with download/delete/progress	DONE
Settings → AI Models: Ollama section with status + link	DONE
Non-Ministral entries hidden from frontend	DONE

---

Test Status

All 134 test files, 3844 tests pass. No regressions introduced by the fix commit.

---

Verdict: no remaining blockers. Approved.