Skip to content

fix(core): scope the missing-default-model health warning to the workspace default - #3514

Merged
Astro-Han merged 3 commits into
apache:mainfrom
GabrielDrapor:fix/health-default-model-scope
Aug 23, 2026
Merged

fix(core): scope the missing-default-model health warning to the workspace default#3514
Astro-Han merged 3 commits into
apache:mainfrom
GabrielDrapor:fix/health-default-model-scope

Conversation

@GabrielDrapor

@GabrielDrapor GabrielDrapor commented Aug 22, 2026

Copy link
Copy Markdown
Contributor
image

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

projectHostConnections projects defaultModel onto exactly one connection — the catalog's default target. Every other enabled connection carries an empty defaultModel by construction. That is the documented normal state: reconcileConnectionAfterEnabledModelsChange says 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").

healthSignalFromConnection predates that doctrine: it treats any enabled connection with an empty defaultModel as a per-connection configuration gap — warning + blocksSend — so with N enabled connections the health page permanently shows N−1 unresolvable warnings.

Fix

healthSignalFromConnection now takes { workspaceHasDefaultTarget } (the caller derives it from the same projected list — exactly one connection carries a non-empty defaultModel):

  • default target exists elsewhere → the connection reports an informational 不是工作区的默认模型来源 with a detail pointing at explicit per-task model selection and 设置 · 通用, and never blocks send;
  • no default anywhere → the send-blocking 等待选择默认模型 warning remains on every enabled connection — the one state where a new chat genuinely cannot start;
  • disabled connections and the validation-layer signals for the configured target are unchanged.

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

…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
@GabrielDrapor
GabrielDrapor force-pushed the fix/health-default-model-scope branch from 11b6c58 to 60674fc Compare August 22, 2026 15:50
@GabrielDrapor

Copy link
Copy Markdown
Contributor Author

Amended in place at head 60674fc63 after an independent adversarial pass (Codex) caught a real P1 in the first cut: the informational early-return sat BEFORE the validation branches, so a non-default connection with needs_reauth/failed-test state would have reported the friendly info note instead of its blocking validation error — and a connection with an empty enabled-model set would have claimed "explicit selection works" with nothing to select.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] test is red on this head, and it's this PR's own text — Biome reformats two new blocks in health.test.ts. Running npm run format:check locally reproduces it.
  • [P2] The new configuration/info state reaches the English Health Center copy through a branch that predates it, so an enabled connection gets described as Connection 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 },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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: '不是工作区的默认模型来源。',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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
@GabrielDrapor

Copy link
Copy Markdown
Contributor Author

Both addressed in a follow-up commit at head 94aa32847:

  • [P1] format:check — the two new health.test.ts blocks are reformatted; npm run format:check is clean locally (my mistake: the fixture edits landed after the last biome pass).
  • [P2] English copy — the configuration branch in settings-health-copy.ts now splits three ways, anchored on the producer's message strings (the same convention the runtime_probe branch already uses on the producer's detail): the non-default state renders as Not the workspace default model source., the empty-inventory warning as No models are enabled on this connection., each with a matching English detail pointing at explicit per-task selection / the connection detail view. Disabled and missing-default keep their existing strings.

I went with the split rather than rendering the producer's own message because the producer's strings are Chinese and this file is the EN surface — but happy to switch to a structured reason code on HealthSignal in a follow-up if you'd rather remove the string-anchor pattern altogether.

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) =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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_disabledrepair_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.

@Astro-Han

Copy link
Copy Markdown
Contributor

CI has now run and is green on 94aa328473652a178199613f7aacf875bf74a526test is completed / success. It had never run before; the workflow was parked at action_required, which I approved as a maintainer.

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
@GabrielDrapor

Copy link
Copy Markdown
Contributor Author

Fixed at head 96d5a6aca — the narrowest fix you outlined, with one structural addition: the derivation moved into core as workspaceHasDefaultModelTarget(connections) (requiring the holder to be enabled), exported beside the signal it feeds, so the desktop caller and the tests share the one predicate and cannot drift.

Behavior with a disabled default holder is now exactly the invariant's reading: workspaceHasDefaultTarget = false → every other enabled connection escalates back to the send-blocking 等待选择默认模型 warning (the workspace genuinely has no default), and the holder itself keeps its Connection is disabled. info. Regression covers the disabled-holder case specifically — predicate both ways, plus the escalation of a sibling connection's signal.

Thanks for approving the fork workflow run as well. core 607, desktop 1150 — green; format:check clean at this head.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit e665bc4 into apache:main Aug 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants