fix(core): scope the missing-default-model health warning to the workspace default - #3514
Conversation
…space default
The connection catalog projects defaultModel onto exactly ONE connection
— the default target — so with a default configured, every other enabled
connection carries an empty defaultModel by construction. The health
check treated that documented normal state ('设置 · 通用 is the one
control for which model a new chat starts on', per
reconcileConnectionAfterEnabledModelsChange) as a per-connection
configuration gap: 等待选择默认模型, warning, blocksSend — and sent users
hunting the provider detail page for a per-connection default picker
that deliberately does not exist.
The signal now takes the workspace context: with a default target
somewhere, an enabled connection without one reports an informational
'不是工作区的默认模型来源' (explicitly-selected models on that connection
work; the default lives in 设置 · 通用) and never blocks send. Only when
NO default exists anywhere does the send-blocking warning remain — the
one state where a new chat genuinely cannot start.
Co-Authored-By: Claude <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
11b6c58 to
60674fc
Compare
|
Amended in place at head The "not the workspace default source" note now sits AFTER the validation branches (a failing test on a non-default connection surfaces exactly as before), and a non-default connection with zero enabled models reports a non-blocking warning (没有启用任何模型) pointing at the connection detail page instead of the info note. Regressions cover both orderings. core 606, desktop 1150 — green. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for this — the framing is right. Projecting defaultModel onto a single Host default target means every other enabled connection legitimately has no default, and warning "pick a default model" on each of them was an unresolvable false alarm. Scoping it to the workspace-level check fixes a real annoyance.
I found two things before this can land. Details inline. The second one lands in a file this PR doesn't touch, so I've anchored it to the producer.
- [P1]
testis red on this head, and it's this PR's own text — Biome reformats two new blocks inhealth.test.ts. Runningnpm run format:checklocally reproduces it. - [P2] The new
configuration/infostate reaches the English Health Center copy through a branch that predates it, so an enabled connection gets described asConnection is disabled.The Chinese copy is correct; only the English mapping is wrong.
I verified the rest: disabled connections, an unvalidated default target, a workspace with no default at all, explicit non-default model selection, and an empty model inventory all behave as intended, and needs_reauth / failed validation still win over the new info branch. @maka/core passes 606/606 locally. I did not find a duplicate authority — workspaceHasDefaultTarget is derived from the Host projection, not a second source of truth.
| connection({ defaultModel: '' }), | ||
| 20, | ||
| { workspaceHasDefaultTarget: false }, | ||
| ); |
There was a problem hiding this comment.
[P1] This is what's turning test red on this head.
Biome wants these new multi-argument calls reformatted, so npm run format:check exits 1. Same for the reauth block just below. It's not pre-existing noise on main — it's deterministic on the text this PR adds.
npm run format should be all it takes.
| status: 'info', | ||
| source: 'settings', | ||
| checkedAt, | ||
| message: '不是工作区的默认模型来源。', |
There was a problem hiding this comment.
[P2] This new info state is correct here, but it reaches an English consumer that predates it and describes it wrongly.
apps/desktop/src/renderer/locales/settings-health-copy.ts:131 maps every llm_connection + configuration signal to just two strings:
if (signal.layer === 'configuration') return signal.status === 'info' ? 'Connection is disabled.' : 'Select a default model.';So this signal — an enabled, usable connection that simply isn't the workspace default — is shown to English users as Connection is disabled., the opposite of what is true.
The warning branch just above (没有启用任何模型。) has the same problem: it renders as Select a default model., sending the user to the wrong screen. The right action there is to enable a model in that connection's detail view.
Your Chinese messages are accurate; it's the English mapping that fell behind. Either split it three ways to match the producer, or have the consumer render the signal's own message/detail so a future producer state can't silently inherit old wording.
Status colour and blocksSend are right throughout — it's only the words.
…ion states Review follow-up on apache#3514. The English mapping collapsed every llm_connection configuration signal into two strings, so the new informational state (an enabled connection that simply is not the workspace default) rendered as 'Connection is disabled.' — the opposite of the truth — and the no-enabled-models warning rendered as 'Select a default model.', pointing at the wrong screen. The configuration branch now splits three ways anchored on the producer's message strings (the same convention the runtime_probe branch uses on the producer's detail), with matching English details pointing at explicit per-task selection / the connection detail view. Also reformats the two new health.test.ts blocks that were failing format:check. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
|
Both addressed in a follow-up commit at head
I went with the split rather than rendering the producer's own core 606, desktop 1150 — green. |
| // default target): with a default configured somewhere, other enabled | ||
| // connections carry an empty `defaultModel` by construction, and their | ||
| // signal must say "not the default source", not "misconfigured". | ||
| const workspaceHasDefaultTarget = connections.some((connection) => |
There was a problem hiding this comment.
[P2] workspaceHasDefaultTarget counts a disabled connection, so the one state where sends are actually blocked shows an all-clear health page.
const workspaceHasDefaultTarget = connections.some((connection) =>
Boolean(connection.defaultModel),
);projectHostConnections assigns defaultModel purely from catalog.defaultTarget?.connectionId, with no reference to connection.enabled (runtime-host-connections-ipc-main.ts:328-335). So a disabled connection that still holds the default target keeps a non-empty defaultModel, and this predicate reports true.
Reachable through ordinary UI, no special setup: disable the connection that currently holds the default target. connections:update has no guard for that case — it neither clears nor transfers defaultTarget — and the storage-layer update() does not touch it either.
Measured on this head's build:
workspaceHasDefaultTarget = true
A (default, disabled) info blocksSend=false | connection is disabled
B info blocksSend=false | not the workspace default model source
C info blocksSend=false | not the workspace default model source
send-path readiness: {"ready":false,"reason":"connection_disabled"}
Not a single warning, blocksSend=false everywhere — and sending is blocked. The send path resolves the default without consulting enabled (session-catalog-coordinator.ts:769-782), then isConnectionReady returns connection_disabled → repair_required.
Why this is the PR's own invariant failing, not an adjacent bug. The stated rule is that blocksSend should be true exactly when a new conversation genuinely cannot start. "The default source is switched off" is that state. The user discovers it by hitting the composer's repair prompt — which is precisely the surprise the health page exists to prevent. The narrowing is right in principle; it is one predicate too broad.
Narrowest fix consistent with the intent:
const workspaceHasDefaultTarget = connections.some(
(connection) => Boolean(connection.defaultModel) && connection.enabled,
);A disabled default holder then correctly falls out of "a default exists", and its own signal can escalate from info to a warning that blocks send. Worth a regression test for the disabled-default-holder case specifically, since the existing suite passes 7/7 without covering it.
On the change overall: the direction is right and the comment explains it well — with a default configured somewhere, the other enabled connections carrying an empty defaultModel is a projection artifact, and calling that "misconfigured" was noise. Narrowing to "not the default source" is the correct signal. This finding is about which connections count as having a default, not about the narrowing itself.
On CI: this head had zero check runs — an external-fork workflow parked at action_required, which reads like "pending" but means it never started. Approved as a maintainer so it can report. Flagging it because the reviewing line caught it rather than reading the absence of red as green.
Independent line, provisional judgment sealed before any existing review was read. Verified at the gate 2026-08-23 13:05 UTC.
|
CI has now run and is green on Green CI does not clear the [P2]. The disabled-default-holder case is not covered by any existing test, which is precisely why the suite passes at 7/7 while the health page reports all-clear in a state where sending is blocked. The build passing and the invariant holding are different claims here. Holding off on approval until that predicate is narrowed (and ideally pinned by a regression). Everything else on this change checks out — the details are in #3514 (comment) |
…fault Review follow-up on apache#3514. projectHostConnections assigns defaultModel purely from the default target's connection id, with no reference to enabled — so disabling the connection that holds the default target (ordinary UI; nothing clears or transfers defaultTarget) left workspaceHasDefaultTarget true: every signal read info/blocksSend=false while send-path readiness failed with connection_disabled. The one state where sends are genuinely blocked showed an all-clear health page. The derivation now requires the holder to be enabled, and lives in core as workspaceHasDefaultModelTarget beside the signal it feeds, so the caller and the tests cannot drift. With a disabled holder, the other enabled connections escalate back to the send-blocking warning — the workspace genuinely has no default. Regression covers the disabled-holder case specifically. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj
|
Fixed at head Behavior with a disabled default holder is now exactly the invariant's reading: Thanks for approving the fork workflow run as well. core 607, desktop 1150 — green; |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 96d5a6aca9d8c9d4234b551d43f2ab3bfa8301cd. Required test is completed / success bound to that exact SHA. No P0–P3.
Re-review at the current head. All three earlier findings were re-derived rather than accepted as fixed.
The [P1] that was turning test red — Biome formatting on the new health.test.ts blocks — is resolved; test is green on this exact SHA.
The [P2] about a disabled default holder is fixed at the right layer, which is the part worth noting. workspaceHasDefaultModelTarget in packages/core/src/health.ts now requires connection.enabled, and it is exported so the IPC caller and the tests read the same derivation instead of each carrying their own copy. The comment above it states the reason plainly: the catalog projects defaultModel from the default target's connection id, so a disabled holder keeps its projected value while being unable to serve a new chat — counting it showed an all-clear health page in precisely the state where sends fail with connection_disabled. Fixing this by exporting one predicate rather than patching the call site is what stops the two from drifting apart later.
The [P2] about the English consumer is fixed: settings-health-copy.ts now separates "no workspace default source" from "connection has no enabled models", so an enabled non-default connection is no longer rendered as disabled, and an empty model inventory no longer sends the user off to pick a workspace default.
New regression coverage includes the disabled-holder case and sibling escalation; HealthSignal suite 8/8.
Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review. It means the code has been checked, not that the gate is open — merge still needs a committer other than the author to give LGTM and to decide.
Symptom
The 健康 tab shows
等待选择默认模型(warning, 阻塞发送) for every enabled LLM connection that is not the workspace default — three at once in the report that surfaced this (Openrouter / Codex OAuth / Moonshot). The user then goes hunting in the provider detail page for a per-connection default-model setting… which deliberately does not exist.(Before-screenshot to follow in a comment — the original capture is being re-attached.)
Root cause — the signal contradicts the connection model's own doctrine
projectHostConnectionsprojectsdefaultModelonto exactly one connection — the catalog's default target. Every other enabled connection carries an emptydefaultModelby construction. That is the documented normal state:reconcileConnectionAfterEnabledModelsChangesays which model a new chat starts on is 设置 · 通用's one control, and explicitly declines to offer a per-connection picker elsewhere ("the connection is then simply not the workspace default … which is what it is").healthSignalFromConnectionpredates that doctrine: it treats any enabled connection with an emptydefaultModelas a per-connection configuration gap — warning +blocksSend— so with N enabled connections the health page permanently shows N−1 unresolvable warnings.Fix
healthSignalFromConnectionnow takes{ workspaceHasDefaultTarget }(the caller derives it from the same projected list — exactly one connection carries a non-emptydefaultModel):不是工作区的默认模型来源with a detail pointing at explicit per-task model selection and 设置 · 通用, and never blocks send;等待选择默认模型warning remains on every enabled connection — the one state where a new chat genuinely cannot start;Regression
health.test.ts: non-default connection with a workspace default → info, non-blocking; no default anywhere → warning, blocking; configured target keeps its validation signal.Suites: core 606, desktop 1150 — green; biome clean.
Co-Authored-By: Claude noreply@anthropic.com
https://claude.ai/code/session_01TMwYxgNEbz2RFmuK6AXGcj