Conversation
…allback models Fixes apache#5493 Generated-by: Claude Code
Move the probe-model rule into @maka/core so the Runtime probe and the settings preview share one owner. Refs apache#5493 Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 60f0814. The change moves connection-test model selection into core for both Runtime and the settings preview. With no enabled model, it now probes a fetched account inventory before the shipped fallback, preferring IDs ending in :free; it adds provider and core tests.
I found one P2 in the new inventory path (inline): it can select an explicitly non-chat model and report a valid connection as failed instead of reaching a chat-capable candidate. This should be resolved before merging. The 57 focused core/runtime tests pass locally, and the fetched-main merge-tree and diff check are clean. Only the label check is currently visible for this head; no current-head hosted test is green. The full local build stops on UI component-contract type errors outside the changed files. I did not run a live provider account or packaged Desktop.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| : enabled; | ||
| const accountInventory = listed | ||
| ? [ | ||
| ...discoveredIds.filter(isNoCostModelId), |
There was a problem hiding this comment.
P2: Filter account-inventory probe candidates for chat capability. This path collects every fetched ID and moves every :free ID ahead of the fallback without checking the model's capabilities.chat/output modalities. The same codebase explicitly marks non-text/image-only models as unsupported for chat (isModelExplicitlyUnsupportedForChat), but testConnectionStrict sends a chat probe to this selected ID and never tries the next candidate. With no enabled models and a fetched inventory [image:free (chat:false), chat (chat:true)], a direct probe selected image:free; a valid credential can therefore be shown as a failed connection. Please skip explicitly non-chat inventory entries before selecting the probe model and cover that case in a regression test.
There was a problem hiding this comment.
- The account-inventory candidates now drop entries
isModelExplicitlyUnsupportedForChatrejects before the:freeordering. Enabled ids are left as they are, since those are the user's own choice to test. - Regression tests for your exact shape (
[image:free (chat:false), chat (chat:true)]) inprovider-conformanceand corellm-connections; both fail on 60f0814 withacme/image:freeand pass now. The core test also covers an audio-only:freeentry, and a list with no chat-capable entry falling back to the provider fallback. - To share the check without an import cycle,
isModelExplicitlyUnsupportedForChatmoved besideModelInfoinllm-connections.ts;model-catalogre-exports it, so its existing callers are unchanged.
Local: core 920/920, runtime 3677/3677, runtime-host connection suites 34/34, typecheck for core/runtime/runtime-host/desktop, and check:renderer-architecture --base upstream/main --strict-base pass.
The probe sends one chat request, so an image- or audio-only entry sorted first would report a valid credential as failed. isModelExplicitlyUnsupportedForChat moves beside ModelInfo so the probe can share it without an import cycle; model-catalog re-exports it for existing callers. Refs apache#5493 Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit be46b8f. This follow-up filters fetched account-inventory candidates that are explicitly unsupported for chat before choosing the one-shot connection probe. It moves the existing chat-capability predicate beside ModelInfo, re-exports it for catalog callers, and adds core/runtime regressions for image-only and audio-only entries.
The P2 I reported on the previous head is addressed: with an image-only :free entry ahead of a chat-capable model, the new selector probes the chat model. I found no additional substantiated P0-P3 issue in the new diff and its affected paths. The core and runtime builds and 81 focused tests pass locally; the fetched-main merge-tree and diff check are clean. No current-head hosted checks are visible yet, so this is not a green merge gate. I did not run a live provider account, packaged Desktop, or the full local suite; the earlier full local build stopped on unchanged UI component-contract type errors.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Summary
With nothing enabled, the connection test probed the provider's first hard-coded fallback model even when the account's own model list had already been fetched. For OpenRouter that is
anthropic/claude-sonnet-5, so a credential check billed a premium model the user never chose, and a key with a model allow-list could be reported as failing although it works.The probe now picks, in order: an enabled model (unchanged) → the fetched account inventory,
:freevariants first → the provider fallback. A shipped snapshot (modelSource: 'fallback') or a connection without an inventory keeps the old fallback order, per #1584.The rule now lives in
@maka/coreasconnectionTestModelId, so the Runtime probe and the settings page share one owner. The Test connection button uses it to name the model it will probe in its tooltip, before a request is spent.Fixes #5493
Verification
provider-conformance: 3 new probe tests; the two inventory tests fail onmainwithactual: 'anthropic/claude-sonnet-5'and pass here. File 42/42; full@maka/runtimesuite 3676 pass, 0 fail.llm-connections: newconnectionTestModelIdtest, 15/15.connection-effect-coordinator+connection-effects-protocol: 34/34.typecheckfor core, runtime and desktop; Biome on changed files;check:renderer-architecture;check-locale-hygiene --base upstream/main;knipfor desktop and ui — all pass.npm test, E2E. The tooltip has no automated test; checked by hand (screenshot below).Review focus
:freepreference: OpenRouter's free variants are rate-limited, so a probe could hit a 429 on a busy free model. I followed the issue's expected behavior here; happy to drop the preference if you'd rather keep the first inventory entry.AI use
Tool(s) and scope: Claude Code — root-cause analysis, implementation and tests; reviewed and verified by me. Both commits carry
Generated-by: Claude Code.Checklist
Does this PR entail a change in behavior?