feat(providers,schema,engine,workbench): retire the default model — user-selected model sets from provider model lists - #426
feat(providers,schema,engine,workbench): retire the default model — user-selected model sets from provider model lists#426PeronGH wants to merge 17 commits into
Conversation
Each endpoint service names the URL that lists the ids it serves, spelled out rather than derived from a variant's baseUrl and protocol: DeepSeek's `/anthropic` variant would derive `/anthropic/v1/models` and Vercel's bare-origin one a root `/models`, and neither route exists. Both Cloudflare entries serve no list at all, so they stay absent and those accounts remain freeform-only.
…el source An account now carries the models the user selected (`Account.models`) instead of one free-text default, and the pick itself lives per agent as `ProviderConfig.model`. Nothing falls back to the agent's own choice any more, so a bound agent with no pick refuses to start rather than running on a model the user never chose; an agent with no account bound keeps resolving its own. `config.probe-models` now names a service and lets the daemon resolve the list URL from the catalog, so a saved account is probed by id and its stored secret never travels back out to the client. Both wire versions move: removing `Account.model`, renaming `defaultModel`, and dropping the `null` tier from `StartOptions.model` are breaking. `loadConfig` carries both old fields over on read, since zod would otherwise strip them and silently lose every existing user's configured model. The model inputs are gone from the account forms; the multi-select that replaces them lands with the picker work.
…known provider A picked model id comes from the service's own model list and carries no provider, while opencode routes only by `providerID/modelID`. `resolveModelRef` qualifies it with `config.knownProvider`; with neither half available the ref is still refused, since a stored unroutable id would report a successful switch while every prompt silently omitted the field. Both reflection paths compare resolved refs and emit the id the user picked rather than opencode's prefixed readback, so the client's selected set still matches what the session reports.
… list Every account form now carries a model set instead of a free-text default: fetch the ids the service serves, tick the ones to keep, and add any missing id by hand. Endpoints that serve no list — the Cloudflare gateways, custom accounts — are freeform only, which is the same control with the fetch button absent. Sources differ by account and are injected rather than read in the form, since the forms are presentation and only the settings page sits inside the data-plane provider tree: a catalog service is probed with the unsaved secret, a saved account by id, and a subscription reads codex's start catalog or, for claude-code, the curated table it has no enumeration API to replace. A picked id the list stops returning is kept and stays ticked. It is either a hand-typed entry or one the vendor retired, and dropping it would change the account's model set behind the user's back on the next fetch.
… to send without one The composer and the new-session surface now read the set picked on the agent's bound account, which outranks both the adapter-advertised catalog and the curated table: a claude-code account pointing at DeepSeek stops offering Anthropic ids it cannot reach. Present-but-empty and absent mean different things in that set, and the send gate turns on the difference. An account bound with nothing picked blocks sending, matching the daemon's own refusal instead of discovering it a round trip later. An agent with no account bound is absent, still resolves its own model, and is not blocked. AGENT_DEFAULT_MODELS is gone: guessing a provider's model is exactly what the picked set replaces, and an unresolved model now blocks the send rather than silently starting on a vendor default. Rebinding an agent drops a pick the new account does not list, since keeping it would run the next session on a model that account never offered.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The reshaped config.probe-models frame lets a caller pair any saved account's secret with any service's model-list URL. A crafted frame sends a stored Anthropic key to openrouter.ai. Details inline on request-handler.ts.
The direction here is right, and the schema work is careful — ProviderConfig.model's "Not a fallback default: unset means no session can start" is exactly the doc comment that makes the new contract legible, and AGENT_DEFAULT_MODELS is removed cleanly (I grepped: zero stragglers). The MIN_COMPATIBLE_WIRE_VERSION 68→74 bump is the correct call for a field rename plus a removed null tier, and the PR body already says partial upgrades are off the table.
Two anchored defects below, plus one scope question that has no line to point at.
The orphan-model drop exists in only one place, and it's client-side
withBinding (packages/client/workbench/src/settings/providers/view.ts) now takes accounts and drops providers[kind].model when the newly bound account doesn't offer it. Good — but that's the rebind path only.
Nothing re-runs it when the account's models set is edited. handleUpdate in providers-settings.tsx calls saveAccounts and never touches providers, so a user who unchecks the very model an agent is configured to run on leaves providers[kind].model pointing at an id the account no longer offers. On the engine side, applyProviderDefaults (packages/host/engine/src/agent/provider-config.ts:137) does next.model = config.model verbatim — the account's models set is used only to build the credential/endpoint bundle, never to validate the model string. The new accountBound && model === undefined guard in start-options-resolver.ts doesn't fire either, because the model is present, just wrong.
Net effect: the user gets a provider-level failure mid-start ("model X is not available…", or a 400 from the vendor) instead of the clean, local "no model selected" refusal this PR is built to produce. Since the engine is where the invariant is now enforced, that's probably where membership belongs too — the client-side drop is a nicety, not the guarantee. Worth deciding deliberately rather than leaving the two halves out of step.
Smaller notes
packages/host/agent-adapter/src/__tests__/pi-model.test.ts:177still assertsstart({ model: null })preserves "an explicit null model reset for the Pi provider default". That tier is no longer expressible on the wire now thatAgentStartInput.modeldropped.nullable(), so the test is pinning adapter behavior no client can reach. Not wrong, just worth knowing it's decorative now.- The six vendor model-list URLs in
catalog.tsare hardcoded and I could not verify them from here (no network). Worth one manual pass, particularlydeepseek'shttps://api.deepseek.com/modelsand the?limit=1000on anthropic-api.
Claude Opus ｜ 𝕏
| private probeSecret( | ||
| credential: Extract<AgentRequest, { kind: 'config.probe-models' }>['credential'], | ||
| ): AccountSecret { | ||
| if (credential.type === 'inline') return credential.secret; | ||
| const account = this.providers | ||
| .getAccounts() | ||
| .find((candidate) => candidate.id === credential.accountId); | ||
| if (!account) throw new Error('Account not found'); | ||
| if (account.credential.type === 'oauth') { | ||
| throw new Error('A subscription login holds no secret to read the model list with'); | ||
| } | ||
| return account.credential; | ||
| } |
There was a problem hiding this comment.
The destination URL and the credential come from two independent client-controlled fields, and nothing checks they agree.
At line 154 the URL is derived purely from payload.service:
const source = modelListSource(payload.service);
...
const models = await this.probeModels(source, this.probeSecret(payload.credential));and probeSecret below resolves the secret purely from credential.accountId. So a config.probe-models frame carrying service: 'openrouter' plus credential: {type:'account', accountId: '<the-anthropic-account>'} makes the daemon send the stored Anthropic key to https://openrouter.ai/api/v1/models.
That directly contradicts this method's own docstring. "A saved account is named by id rather than shipping its secret back out to the client" keeps the secret away from the client, but the caller still gets to choose which third party receives it — which is the part that actually matters. The blast radius is bounded (six catalog URLs, not arbitrary-URL SSRF, and the client is loopback-local), but "only a local process can exfiltrate your API keys to a competitor" is still a weaker guarantee than the one written here.
The information needed for the check already exists: Account.service (packages/foundation/schema/src/model/account.ts:58). Thread the service id in and compare. Note the undefined case — custom and pre-catalog accounts have no service, and those should be refused rather than allowed through, since modelListSource only ever resolves a catalog id anyway.
| private probeSecret( | |
| credential: Extract<AgentRequest, { kind: 'config.probe-models' }>['credential'], | |
| ): AccountSecret { | |
| if (credential.type === 'inline') return credential.secret; | |
| const account = this.providers | |
| .getAccounts() | |
| .find((candidate) => candidate.id === credential.accountId); | |
| if (!account) throw new Error('Account not found'); | |
| if (account.credential.type === 'oauth') { | |
| throw new Error('A subscription login holds no secret to read the model list with'); | |
| } | |
| return account.credential; | |
| } | |
| /** The secret to probe with. A saved account is named by id rather than shipping its secret back | |
| * out to the client and in again; an oauth login holds none, so it cannot be probed. The account | |
| * must belong to the service being probed, or the caller could aim one vendor's key at another. */ | |
| private probeSecret( | |
| service: string, | |
| credential: Extract<AgentRequest, { kind: 'config.probe-models' }>['credential'], | |
| ): AccountSecret { | |
| if (credential.type === 'inline') return credential.secret; | |
| const account = this.providers | |
| .getAccounts() | |
| .find((candidate) => candidate.id === credential.accountId); | |
| if (!account) throw new Error('Account not found'); | |
| if (account.service !== service) { | |
| throw new Error('Account does not belong to the service being probed'); | |
| } | |
| if (account.credential.type === 'oauth') { | |
| throw new Error('A subscription login holds no secret to read the model list with'); | |
| } | |
| return account.credential; | |
| } |
| // The catalog default is what the agent's own config would start on, so it yields to anything the | ||
| // user expressed through LinkCode. Nothing guesses past it: an unresolved model blocks the send | ||
| // rather than starting a session on a model nobody chose. | ||
| const displayedModel = | ||
| selectedModel ?? | ||
| (defaultModels === null | ||
| ? null | ||
| : (defaultModels?.[provider] ?? | ||
| catalog?.defaultModel ?? | ||
| AGENT_DEFAULT_MODELS[provider] ?? | ||
| null)); | ||
| (defaultModels === null ? null : (defaultModels?.[provider] ?? catalog?.defaultModel ?? null)); |
There was a problem hiding this comment.
The new comment says "an unresolved model blocks the send rather than starting a session on a model nobody chose" — but the catalog?.defaultModel arm on line 195 means it doesn't.
catalog.defaultModel is still populated by the adapters (native/claude-code.ts:623, native/pi/adapter.ts:223, native/codex/adapter.ts:418-426). Take an agent with an account bound and a non-empty picked set, where the user has never chosen a model — so selectedModels[provider] is undefined, preferredModels?.[provider] is absent, and configuredDefaultModels has no entry because providers[kind].model is unset. Then:
selectedModelisnull, so the??on line 194 does not short-circuit;displayedModelresolves tocatalog.defaultModel, a non-null string;boundSetis defined, sosendBlocked = (boundSet !== undefined && displayedModel === null)isfalse;resolveModel(pickable, displayedModel)finds nothing, so the composer displays a model the bound account may not even offer;- submit (line 240) evaluates
localModel === null ? undefined : (selectedModel ?? undefined)→undefined; - the daemon rejects with
No model selected for <kind>.
That's the exact round trip this PR set out to eliminate, and it shows up in the default state for any freshly bound account. Note the fix must keep the defaultModels?.[provider] arm — a configured model legitimately submits as undefined and gets refilled by applyProviderDefaults — and drop only the catalog fallback:
| // The catalog default is what the agent's own config would start on, so it yields to anything the | |
| // user expressed through LinkCode. Nothing guesses past it: an unresolved model blocks the send | |
| // rather than starting a session on a model nobody chose. | |
| const displayedModel = | |
| selectedModel ?? | |
| (defaultModels === null | |
| ? null | |
| : (defaultModels?.[provider] ?? | |
| catalog?.defaultModel ?? | |
| AGENT_DEFAULT_MODELS[provider] ?? | |
| null)); | |
| (defaultModels === null ? null : (defaultModels?.[provider] ?? catalog?.defaultModel ?? null)); | |
| // The catalog default is what the agent's own config would start on, so it yields to anything the | |
| // user expressed through LinkCode. Nothing guesses past it: an unresolved model blocks the send | |
| // rather than starting a session on a model nobody chose. A bound account narrows that further — | |
| // its picked set is the only source, so the adapter's own default is not a candidate at all. | |
| const displayedModel = | |
| selectedModel ?? | |
| (defaultModels === null | |
| ? null | |
| : (defaultModels?.[provider] ?? | |
| (accountModels?.[provider] === undefined ? catalog?.defaultModel : undefined) ?? | |
| null)); |
A live session's account is fixed at spawn — credentials and base URL are injected once — so the client needs to know it to scope that session's model menu. Nothing recorded it: the resolved account existed only inside `applyProviderDefaults`. `accountConfigBundle` now echoes `accountId` into the resolved config, which `resolveAccount` already reads on the way in, so the same key serves both directions and a client can pin a session to one account. Each run lifts just that id into `SessionRun`, and `SessionInfo` reports the latest run's, mirroring how `historyId` already works — a rebind between runs is legitimate, so only the newest describes what a session is actually talking to. Only the id is persisted; the rest of `config` carries secrets.
The new-session menu now spans every account `resolveBinding` accepts for the agent, grouped per account, so choosing a model also chooses which account serves it. One agent reaches several providers without a trip through Settings — previously the menu showed only the single bound account's set, which made it offer less than the curated table it replaced. A live session's menu stays scoped to its own account: credentials and base URL are injected at spawn, so offering another account's models would advertise a switch the adapter cannot make. Model identity becomes (account, model). Two accounts legitimately serve the same id — a direct DeepSeek key and an OpenRouter one both list `deepseek-v4-pro` — and the menu previously used the bare id as both its React key and its radio value, which would collapse the two into one unselectable row. `modelChoiceKey` keys them apart, the pick hands back the whole entry rather than a string to re-parse, and `resolveModel` takes an account tiebreak so the trigger label names the right one. The chosen account rides `config.accountId`, which `resolveAccount` already honours ahead of the bound one.
The model an agent runs on was remembered twice: `providers[kind].model` on the daemon and `modelsByProvider` in renderer localStorage. Two owners meant Settings and the composer could disagree, and a scheduled or script session ignored whatever the composer last used. The accepted pick now writes daemon config, carrying the account it came from — choosing a model is also choosing who serves it, so leaving the old binding would run the next session on an account that never listed that model. The client copy is gone and the persisted store moves to v6 so a stale blob cannot resurrect a memory with no owner. Written once a selection is known to have been accepted rather than on the menu click, keeping the existing confirm-then-remember discipline: an abandoned draft never rewrites config, and a provider that rejects a model leaves the previous one standing. The pick still takes effect on the session immediately — it rides the start options either way. Nothing re-sends a configured model at session start now; the daemon resolves it, so the client specifying it again could only let the two disagree.
|
Three commits pushed that close a gap the first five left open. The Provider → Harness rename follows separately, so the textual sweep does not hide logic. What was missingThe original five made the picker read the one account bound to an agent, which narrowed it — claude-code offered whatever was ticked on one account instead of the whole curated table it replaced.
How it works nowThe new-session menu aggregates every account A live session's menu stays scoped to its own account. Credentials and base URL are injected at spawn, so offering another account's models would advertise a switch the adapter cannot make — opencode rejects cross-provider outright, and claude-code would send the new id to the old endpoint. That required the session to start recording its account, which nothing did. The accepted pick writes Three findings worth attention in reviewThe menu-keying bug was real, not hypothetical. The send gate from Configured models are no longer re-sent at session start. Three tests asserted the remembered pick travels in the submission; that only held while the memory was client-side and invisible to the daemon. The daemon now resolves Deliberate deviationThe pick takes effect on the session immediately, but the persisted default is written only once the selection is confirmed — keeping the existing Checks
Still not driven in the real app, which remains the gap for this PR overall. The manual pass now needs two accounts bindable to one agent: confirm the new-session menu groups both, that picking a DeepSeek model starts a session actually running on DeepSeek, that Settings shows the rebind after the first turn, that the live thread's menu shows only that account's models, and that two accounts sharing a model id give two separately selectable entries. |
There was a problem hiding this comment.
Caution
The new commits widen the send-gate from bound to bindable. One unrelated API-key account in the pool is now enough to replace opencode's own model menu with that account's models and block the composer — while the daemon, which gates on activeAccountId, would have started the session fine. Details inline on default-models.ts.
The account-pinned model pick is a good design, and the pieces that make it work are careful: modelChoiceKey identifies an entry by (accountId, id) rather than id alone, resolveModel's account scoping falls back instead of returning nothing, and handleModelChange writing model and account together is the right shape. Account.models' doc comment states the new contract plainly.
Three new defects below, all in the two newest commits. Independent of these, both threads from the previous review are still open at this HEAD — probeSecret still takes no service argument, and the catalog?.defaultModel arm in displayedModel is unchanged. Not re-raised here.
Two comments in this PR contradict each other, and the tests encode the wrong one
start-options-resolver.ts:52-54, on the daemon's new refusal:
With an account bound, its selected set is the only model source and nothing falls back to the agent's own choice. Unbound agents keep running on whatever they resolve themselves.
accountModelOptions' doc, on the client's:
[]says "bindable, nothing picked yet" and blocks sends the way the daemon does.
Those describe different gates. The daemon keys on providers[kind].activeAccountId !== undefined; the client keys on whether any account could theoretically back the agent — which for opencode and pi is nearly every API-key account in the pool (resolve.ts:149-151 returns native for every protocol, and resolve.ts:66-69 keeps a service-less bare key bindable everywhere). opencode and pi are exactly the agents the PR body says must keep running on their own CLI login.
The new tests preserve the conflation rather than catching it. new-session-surface.test.tsx:898 is titled "refuses to send when an account is bound but no model is picked" and comments "Bound with an empty set: the daemon would refuse this start, so the composer does too" — but the prop it passes, accountModels={{ 'claude-code': [] }}, is what accountModelOptions produces for a bindable account, per default-models.test.ts:104-111 ("keeps a bindable-but-unpicked one empty"). The test passes while asserting a parity the code does not have, so the gap is invisible from the suite.
Whichever way you settle it, the two gates should read the same field — otherwise the composer and the daemon will keep disagreeing about which agents are startable.
Claude Opus ｜ 𝕏
| export function accountModelOptions( | ||
| accounts: Accounts | undefined, | ||
| ): Partial<Record<AgentKind, ModelOption[]>> { | ||
| const options: Partial<Record<AgentKind, ModelOption[]>> = {}; | ||
| for (const kind of AgentKindSchema.options) { | ||
| const bindable = (accounts ?? []).filter( | ||
| (account) => resolveBinding(account, kind).tier !== 'unavailable', | ||
| ); | ||
| if (bindable.length === 0) continue; | ||
| options[kind] = bindable.flatMap((account) => modelOptionsOf(account)); | ||
| } |
There was a problem hiding this comment.
bindable filters on resolveBinding(...).tier !== 'unavailable' — could this account back the agent, not is it bound. For opencode and pi, bind() returns native for every protocol (resolve.ts:149-151), and an account with no catalog service stays bindable everywhere (resolve.ts:66-69). So one API-key account added for codex makes options.opencode present for a user whose opencode authenticates through its own opencode auth login.
Presence is load-bearing twice over in new-session-surface.tsx:
pickable = bindableSet ?? dynamicModels ?? AGENT_MODEL_OPTIONS[provider](line 206) — a present entry discards opencode's own live catalog, so the menu shows an unrelated vendor's models.sendBlocked = bindableSet !== undefined && displayedModel === null(line 383).
opencode is the one adapter that deliberately publishes no defaultModel (native/opencode/adapter.ts:751), and providers.opencode.model is unset because the account was never bound — so displayedModel is null and the send is blocked. The user's only way forward is to pick a model belonging to a vendor account they never meant to use for opencode, which then pins accountId to it and rebinds the agent.
It gets worse when that account has no picked models. models is optional and modelOptionsOf does account.models ?? [], so options[kind] is [] — present, hence blocking, but with nothing in the menu. default-models.test.ts:104-111 pins exactly this ("keeps a bindable-but-unpicked one empty"). The agent is then unstartable from the composer and no interaction there can recover it.
Meanwhile start-options-resolver.ts:52-54 says the opposite for the same configuration: "Unbound agents keep running on whatever they resolve themselves." It gates on activeAccountId, so it would have started this session.
The PR's intent — one agent drawing models from several accounts — argues against narrowing this function to the bound account, since the wider menu is the feature. The narrower fix is downstream: gate the send on the agent actually being bound (the field the daemon uses), and have pickable union the account sets with the adapter catalog instead of replacing it, so an unbound agent keeps its own models on offer.
| if (newlyConfirmed.model === undefined && newlyConfirmed.effort === undefined) return; | ||
| rememberSelection(submission.kind, newlyConfirmed); | ||
| if (newlyConfirmed.effort !== undefined) { | ||
| rememberSelection(submission.kind, { effort: newlyConfirmed.effort }); | ||
| } | ||
| // The model lands in daemon config rather than a client store, together with the account it | ||
| // came from, so Settings shows the rebind and non-composer sessions inherit the pick. | ||
| if (newlyConfirmed.model) { | ||
| void persistPickedModel( | ||
| submission.kind, | ||
| newlyConfirmed.model, | ||
| submission.accountId, | ||
| ).catch(noop); | ||
| } |
There was a problem hiding this comment.
persistPickedModel runs only on the late-confirmation path, so the ordinary case never persists.
newlyConfirmedStartupSelection requires initial.model === null (startup-selection.ts:59) — that is, it fires only when the adapter failed to reflect the pick at session start. When the adapter reflects immediately, reflectedStartupSelection at line 392 already set startupSelection.model = requested.model, so the precondition is false, newlyConfirmed.model is undefined, and the guard on line 415 returns before line 421 is reached.
Nothing else records it now: selectionPatch returns only { effortsByProvider }, modelsByProvider is gone from the v6 store, and the preferredModels prop was removed from NewSessionSurface. The next draft's defaultModels reads providers[kind].model, which was never written.
So a composer model pick survives only when the provider is slow to confirm it. On claude-code and codex, which reflect at start, the user re-picks on every new session — and usePersistPickedModel's own doc says it is called "once a selection is known to have been accepted", which is precisely what immediate reflection is.
Persisting from startupSelection when it already carries a confirmed model, with the late promotion kept for adapters that only confirm after the first turn, covers both. The existing "don't erase a newer live selection" reasoning still argues for keeping the late path conditional.
| model: localModel === null ? undefined : (selectedModel ?? undefined), | ||
| // Pins the session to the account whose entry was picked; without it the daemon would fall | ||
| // back to whichever account happens to be bound. | ||
| ...(localModel !== null && | ||
| modelOption?.accountId !== undefined && { accountId: modelOption.accountId }), |
There was a problem hiding this comment.
This guard excludes an explicit reset, but not an untouched draft — and an untouched draft can still pin an account.
selectedAccounts is only ever written by handleModelChange/handleResetModel, so on a draft where the user never opened the model menu, localAccount and localModel are both undefined. localModel !== null is therefore true, and selectedAccountId is undefined, which makes resolveModel skip the account filter entirely (agent-models.ts:67-68) and return the first entry whose id matches.
accountModelOptions flat-maps accounts in pool order, so when two accounts serve the same model id — a direct vendor account and an OpenRouter/gateway relay, the exact case modelChoiceKey exists for, and which default-models.test.ts:113-119 covers — the winner is whichever account happens to come first in the array. That id ships as accountId, and resolveAccount prefers it over providers[kind].activeAccountId (provider-config.ts:90), so the session runs on a different account's key and base URL than the one the user bound. Nothing in the composer looks any different.
Gating on the account the user actually picked keeps the bound account authoritative for untouched drafts, and still pins correctly once a menu entry is chosen (handleModelChange writes next.accountId ?? null).
| model: localModel === null ? undefined : (selectedModel ?? undefined), | |
| // Pins the session to the account whose entry was picked; without it the daemon would fall | |
| // back to whichever account happens to be bound. | |
| ...(localModel !== null && | |
| modelOption?.accountId !== undefined && { accountId: modelOption.accountId }), | |
| model: localModel === null ? undefined : (selectedModel ?? undefined), | |
| // Pins the session to the account whose entry was picked; without it the daemon would fall | |
| // back to whichever account happens to be bound. | |
| ...(localModel !== null && localAccount != null && { accountId: localAccount }), |
"Provider" meant two different things in adjacent UI: the composer's provider picker chose the *agent*, while the Providers settings page means accounts. `onOpenProviderSettings: (kind: AgentKind) => void` had both in one signature. Agent-meaning UI text and client identifiers now say harness — `selectableHarnesses`, `onHarnessChange`, `lastHarness`, and the composer's menu label. Account-meaning strings keep "provider", `AgentKind` and every wire and daemon term are untouched, and `groupModelsByProvider` stays as the one genuine model-provider use. Same UI/i18n-only discipline already recorded for Thread/`session`. Two things this turned up. Translation keys are not typechecked, so the rename would have silently emptied strings — a test asserting the old menu label is what caught it. And the store test had hand-copied its storage key, which drifted at the previous version bump and had quietly turned the malformed-blob test into a vacuous pass; the key is now exported and imported, and the test verified to fail against a well-formed blob.
|
Why"Provider" meant two different things in adjacent UI. The composer's provider picker chose the agent; the Providers settings page means accounts. What movedAgent-meaning UI text and client identifiers now say harness:
Unchanged on purpose: account-meaning strings (the settings nav Two things the sweep exposedTranslation keys are not typechecked. A test had drifted from the store it tests. Deliberately out of scope
Checks
Pure rename: no behaviour moved, so the manual pass still outstanding for this PR is the one described in the previous comment. |
There was a problem hiding this comment.
ℹ️ The rename is clean — nothing new is broken by it. The findings from both earlier reviews still reproduce at this HEAD, untouched by this commit, so this isn't an approval.
Reviewed changes — one commit since the prior review (8634d2c → 12fde61): a UI/i18n-only rename of the agent meaning of "provider" to "harness".
- Renamed the agent-meaning client identifiers —
selectableProviders→selectableHarnesses,onProviderChange→onHarnessChange,NewSessionDraft.initialProvider→initialHarness,lastProvider→lastHarness, across the composer, the new-session surface, the workbench, and the desktop history-import tab.AgentKindand every wire/daemon term are untouched, andgroupModelsByProvideris left as the genuine model-provider use. I read the commit line by line: it is mechanical — no branch, guard, or ordering changed. - Renamed three interpolation placeholders and one label —
{provider}→{harness}inplugins.discoveryFailed,plugins.runtimeMissingandhistoryImport.panelTitle, pluscomposer.provider→composer.harness, in both locales. Checked each against its call site (plugins-tab.tsx:81,92,history-import-tab.tsx:74,composer-controls.tsx:296); no{provider}placeholder is left in either locale, and no call site passes a name the string no longer has. - Bumped the defaults storage key to
:v7and exported it — correct per the frontend rule (bump on any shape change), and it costs released users nothing beyond the:v6bump already in this PR. The test's hand-copied:v5had been writing to a key nothing read; with the key imported, the malformed-blob fixture really is rejected —'unsupported'is not inEffortLevelSchema(low|medium|high|xhigh|max|ultra|ultracode), so that case is no longer vacuous. - Recorded the terminology rule in
AGENTS.md— provider = account/service, harness = agent, withgroupModelsByProvidernamed as the exception.
I ran the suites this commit touches — new-session-defaults-store, new-session-surface, and the ui plugins tests: 55 tests, all passing.
Claude Opus ｜ 𝕏
| export interface NewSessionDefaultsState { | ||
| /** Provider of the last successful new-session submit; null before the first (→ claude-code). */ | ||
| lastProvider: AgentKind | null; | ||
| lastHarness: AgentKind | null; |
There was a problem hiding this comment.
The rename stops short of the rest of this file. The doc comment two lines up still says v7 renamed lastProvider to lastHarness (fine — that's history) but line 44's /** Provider of the last successful new-session submit; null before the first (→ claude-code). */ now describes a field named lastHarness, and effortsByProvider is still keyed by AgentKind, i.e. by harness, not by provider. Same for the provider: AgentKind parameters on remember and rememberSelection.
If effortsByProvider is going to be renamed at all, it has to happen inside this same :v7 window — renaming it in a later commit costs users a second persisted-state discard for one logical change. If leaving it is deliberate (e.g. you plan to key efforts by account later), the doc line should say so, because right now the name and its key type disagree.
Technical details
# Finish the harness rename inside the defaults store
## Affected sites
- line 40 — doc comment references the pre-rename name in prose describing current state
- line 44 — `/** Provider of the last successful new-session submit … */` documents `lastHarness`
- lines 25, 45, 102-107 — `effortsByProvider`, keyed by `AgentKind` (a harness)
- lines 49, 54, 59, 65-66, 86, 96 — `provider: AgentKind` parameters and locals
## Required outcome
Every agent-meaning identifier and doc line in this file reads "harness"; if `effortsByProvider` is renamed, the rename lands under the `:v7` bump already in this commit so users pay one discard rather than two.
## Open questions for the human
Is leaving `effortsByProvider` under the old name deliberate (future re-keying by account), or an oversight?
Live account switching, via the existing restart machineryFour commits (
|
There was a problem hiding this comment.
Caution
The new cross-account switch works, but the client is never told it happened. beginRun is the only writer of the accountId the UI reads and the only record mutator that skips onChanged, so after a switch the composer still names the old account — which inverts the "restarts this thread" hint it just showed. Details inline on session-record-registry.ts.
Reviewed changes — four commits since the prior review (12fde61 → c8c91ca), adding live cross-account model switching:
SessionLifecycleService.switchModel(lifecycle-service.ts:336-403) — a same-account pick forwards in place viasendInput; a cross-account pick relaunches the session under the same id and resumes the provider transcript.agent.inputroutes to it only whenset-modelcarries anaccountId(request-handler.ts), so the old path is untouched for every other input. The guard order is right and deliberately so: busy / no-transcript / no-resume are all asked beforestopForReplacement, and the comment at line 375 says why. I checked the TOCTOU — the whole body runs under the permit-1 session semaphore, soisBusycannot go stale before the teardown.- Three extracted helpers —
resolveForRecord,launchRun,resumeStrategy, withbranchandresumerefactored onto them.launchRunbecoming the single writer of a run entry is a real simplification. - Per-run account attribution —
beginRun(sessionId, accountId?, historyId?),latestAccountId(reverse scan, correctly documented as "a rebind between runs is legitimate"), andaccountIdadded toSessionRun/SessionInfo/ thelist()projection. The schema doc states the contract plainly: "Latest run's account — what the session is talking to now." set-modelgains an optionalaccountIdthrough the whole client stack (control-channel→client-core→sdk→operations), additively, so the existing wire floor still holds.- The restart hint —
switchesAccount+ModelMenuItem+modelSwitchRestartsin both locales. I verified all three render branches of the model menu pass the hint, the string exists inzh-cn.ts(the type source) anden.ts, neither has an interpolation placeholder, and it renders as real text a screen reader reaches. - Docs — the new
agent-adapter/AGENTS.mdbullet ("A live session can change account, but never in place") is exactly the right place for this, and pi's resume column flip to✓matches the code.
I traced the two things most likely to be wrong here and they are fine: the resumed transcript is not duplicated (the seed's coveredBySeed dedupes by message/tool id), and a failed relaunch is cleaned up properly — startLive's tapError calls discardFailedStart regardless of replyTo, which releases the simulator MCP token, so there is no leak. The engine test suite's live account switching block is genuine coverage, not theatre: it asserts run 2 carries acc_second and that resumedWith.config.apiKey === 'sk-second', and the no-resume case asserts adapters[0].stopped === false to prove the refusal precedes teardown.
Three defects below. All five findings from the three earlier reviews still reproduce at this HEAD — probeSecret still takes no service, displayedModel keeps the catalog?.defaultModel arm, accountModelOptions still keys on bindable, workbench.tsx:412 still early-returns before persistPickedModel, and the submit spread still guards on localModel !== null. Not re-raised here; that is why this isn't an approval.
🔴 The relaunch is invisible to the client, and the hint inverts because of it
Anchored inline on session-record-registry.ts. The chain, since it crosses three packages:
beginRun calls persist() but not onChanged() — the only record mutator that doesn't. Every sibling does: register, importRecord, delete, bindHistoryId, setTitleFromContent, setProviderTitle. session.changed is the only revalidation cue for listSessions (client.ts:1217, "the payload is a cue to revalidate through listSessions"), and handleModelChange (workbench.tsx:465-482) never calls mutate() either. launchRun is invoked with replyTo: undefined, and startLive sends session.started only when replyTo !== undefined, so that frame doesn't arrive as a substitute. I checked the whole setModel path down through sdk/operations.ts for a cache invalidation and there is none.
The one path that would have healed it is disarmed by design. bindHistoryId fires onChanged, but it early-returns on run.historyId === historyId (line 127) — and switchModel passes the resumed historyId straight into beginRun, so the new adapter's session-ref announces the id the record already holds and the notify is skipped. The registry's own doc comment at line 141 names this case ("historyId is known up front only when the relaunch resumes a transcript"). So the staleness is not a brief window; it persists until some unrelated session is created, removed, or retitled.
accountId reaches the menu as active?.accountId off that list (shell-frame.tsx:249, desktop-shell.tsx:457). After switching acc_A → acc_B the UI still believes acc_A, so switchesAccount is evaluated against the wrong side and the hint reverses: entries for acc_B — the account the session is now on, where a pick is a harmless in-place set-model — are labelled "Switching account restarts this thread and resumes it", while entries for acc_A, which now genuinely do relaunch, are labelled nothing at all. The second thread restart is the one the feature exists to warn about, and it happens silently. resolveModel(pickable, displayedModel, currentAccountId) is scoped by the same stale id, so a reflected model resolves against the wrong account's entries.
🟠 A session with no recorded account gets no hint, but the daemon still relaunches it
Anchored inline on agent-models.ts. switchesAccount returns false when currentAccountId is undefined, but switchModel compares records.accountId(sessionId) === accountId — and undefined === 'acc_x' is false, so it takes the relaunch branch. A session running on an agent's own CLI login or the legacy providers[kind].apiKey fallback records no account at all, and those are precisely the sessions whose menu is filled with other accounts' models, because accountModelOptions keys on bindable rather than bound (the still-open default-models.ts thread).
The new test pins the undefined case as intentional, but with a draft rationale — "A draft has no running account" — while the same function also serves the live thread through ConversationSurface. The draft is already covered by accountSwitchRestarts defaulting to false, so the undefined arm isn't what protects it.
🟠 Mid-session effort and approval-policy don't survive the relaunch
lifecycle-service.ts:391 — anchored inline. resolveForRecord's override is Pick<StartOptions, 'model' | 'config'>, so the relaunch re-derives everything else from the record (kind, cwd) plus daemon config. The state a user set on the live session via agent.input — set-effort, set-approval-policy — is cached on the LiveSession instance (live-session.ts:145-146, 169-171), which stopForReplacement destroys; the new instance starts empty. snapshot() replays that cache to an attaching client, not across a relaunch.
This is correct for branch and resume, which the user understands as restarts. switchModel is presented as picking a model from a menu, so an unannounced effort reset is a different contract. agent-adapter/AGENTS.md:112 notes StartOptions.effort enters through onSetEffort before onStart, so carrying it is a matter of threading the live values into the override.
ℹ️ Nitpicks
- The hint promises a restart in cases the daemon refuses outright.
accountSwitchRestartsis hardcodedtrueat theConversationSurfacecall site, butswitchModelrejects rather than relaunching when the session is busy, has nohistoryId, or the agent can't resume —grok-buildis✗for resume in the capability matrix yet✓ (next turn)forset-model, so every cross-account entry in its menu advertises a restart-and-resume that can only produce an error in the banner. All three conditions are knowable client-side (status,historyIdand capabilities are already on the session). - The relaunch has no rollback.
stopForReplacementprecedeslaunchRun, so a relaunch that fails on the new account's credential leaves a thread that was mid-conversation stopped. Cleanup and the error reply are both correct — this is the design's cost, not a bug — but it is reachable from a single menu click, and combined with the missingsession.changedthe sidebar won't show the stopped status either. - Test gaps that match the findings above: nothing covers a
launchRunfailure after teardown, nothing asserts effort survives (or doesn't) a relaunch, and no test rendersaccountSwitchRestartswith a multi-account list to assert the hint text —switchesAccounthas unit coverage, but the prop-to-rendered-hint path does not.
Claude Opus ｜ 𝕏
| this.persist(record); | ||
| } |
There was a problem hiding this comment.
beginRun is the only mutator here that persists without notifying — register, importRecord, delete, bindHistoryId, setTitleFromContent and setProviderTitle all call onChanged. It is also the only writer of the accountId this PR just added to the list() projection (line 85), which the constructor's own doc calls "membership and identity" — the category that is supposed to notify. SessionInfoSchema states the contract directly: "Latest run's account — what the session is talking to now."
session.changed is the only revalidation cue for listSessions (client.ts:1217), and the cross-account switch has no other announcement: launchRun is called with replyTo: undefined, so startLive skips session.started, and handleModelChange (workbench.tsx:465-482) never calls mutate(). I followed the setModel path down through sdk/operations.ts looking for an invalidation and there is none.
The path that looks like it would recover is closed off. bindHistoryId does notify, but it early-returns on run.historyId === historyId (line 127) — and switchModel hands the resumed historyId to beginRun, so the new adapter's session-ref reports an id the record already has and the notify never fires. Your comment on line 141 describes exactly this case. Staleness therefore lasts until an unrelated session is created, deleted, or retitled.
Downstream, accountId arrives as active?.accountId (shell-frame.tsx:249, desktop-shell.tsx:457), so switchesAccount compares against the pre-switch account and the restart hint reverses: the new account's entries claim they restart the thread, the old account's entries — the ones that now actually do — say nothing.
| this.persist(record); | |
| } | |
| this.persist(record); | |
| // A new run re-points the record's identity projection (`accountId`, `historyId`), so the | |
| // session list must revalidate or clients keep naming the previous account. | |
| this.onChanged(sessionId, 'updated'); | |
| } |
| return ( | ||
| currentAccountId !== undefined && | ||
| option.accountId !== undefined && | ||
| option.accountId !== currentAccountId | ||
| ); |
There was a problem hiding this comment.
currentAccountId !== undefined suppresses the hint for a session with no recorded account, but the daemon does not agree that the question is inapplicable there — switchModel tests this.records.accountId(sessionId) === accountId (lifecycle-service.ts:353), and undefined === 'acc_x' is false, so it takes the relaunch branch and tears the adapter down.
That state is reachable rather than hypothetical. SessionInfo.accountId comes from resolvedAccountId, which reads config.accountId — written only by accountConfigBundle, and only when an account actually resolves. A session running on an agent's own CLI login (opencode, pi) or on the legacy providers[kind].apiKey fallback records no account at all. Those are exactly the sessions whose menu is populated with other accounts' models, because accountModelOptions keys on bindable rather than bound (the still-open thread on default-models.ts). opencode is ✓ for resume, so the relaunch really does proceed.
The new test pins this arm deliberately, but its stated reason is about the draft — "A draft has no running account" — and the draft is already covered by accountSwitchRestarts defaulting to false, since only ConversationSurface sets it. So the undefined check isn't what protects the draft; it only suppresses the hint on the live path where the daemon will act. Matching the daemon's comparison closes the gap, and the doc comment above should then lose "Unknown accounts on either side mean the question doesn't apply" — only an unknown account on the option side makes it inapplicable.
| return ( | |
| currentAccountId !== undefined && | |
| option.accountId !== undefined && | |
| option.accountId !== currentAccountId | |
| ); | |
| return option.accountId !== undefined && option.accountId !== currentAccountId; |
| const launchRun = this.launchRun.bind(this); | ||
| const resumeStrategy = this.resumeStrategy.bind(this); | ||
| return Effect.gen(function* () { | ||
| const resolved = yield* resolveForRecord(record, { model, config: { accountId } }); |
There was a problem hiding this comment.
The override is Pick<StartOptions, 'model' | 'config'>, so everything except the model and the account bundle is re-derived from the record (kind, cwd) and daemon config. Anything the user set on the live session through agent.input is not among those: set-effort and set-approval-policy land on the LiveSession instance (live-session.ts:145-146, 169-171), and stopForReplacement on the next line destroys it. The replacement starts with an empty cache, and snapshot() replays that state to an attaching client, not across a relaunch — so the new adapter comes up on whatever effort the resolver supplies, which for a client-side effort pick is nothing.
For branch and resume this is fine; both read as restarts to the user. switchModel reads as picking a menu entry, so a silent effort or approval-policy reset is a different contract than the hint sets up. Per agent-adapter/AGENTS.md:112, StartOptions.effort enters through onSetEffort before onStart, so the live values can be threaded into the override here — the session is still alive at this point, so they are still readable.
There was a problem hiding this comment.
ℹ️ The e2e fix is correct and it repairs a genuinely broken CI job — one nitpick inline. Every finding from the four earlier reviews still reproduces at this HEAD, untouched by this commit, so this isn't an approval.
Reviewed changes — one commit since the prior review (c8c91ca → 9ceb386), touching a single file: apps/webview/e2e/browser-smoke.e2e.mts (+14 / −7).
- Repointed the new-chat isolation seed at the live storage key and field —
linkcode.workbench.new-session-defaults:v5/lastProvider→:v7/lastHarness. I traced why this mattered: the old seed landed on a key nothing reads, so the store rehydrated empty,lastHarnessstayednull, and the new-session surface fell back to claude-code. The mock host pins claude-code at{ status: 'missing' }(dev-mock-host.ts:230),deriveAgentRuntimeCuesturns that into{ state: 'missing', downloadable: true }, andnew-session-surface.tsx:384makes any cue setsendBlocked— so the subsequent.click()could only sit out its actionability timeout.piis{ status: 'available', source: 'builtin' }and draws no cue, so the new seed really does produce a sendable composer. This e2e runs in CI (ci.yml:215, the Webview Browser Entry job), so the four prior commits were red there. - Added a fail-fast diagnostic before the Send click — replaces the opaque actionability timeout with a message naming the storage key and the store module. I checked this doesn't introduce a race:
isDisabled()takes an unretried snapshot where.click()used to auto-wait, but nothing in this fixture can flip Send from disabled to enabled after the precedingeditor.fill. The locator itself filters on[contenteditable="true"], sofillalready gates ondisabled={pending || !selected}being false;deriveAgentRuntimeCuesyields{}while runtimes load and never produces a cue forpi; and the mock's account pool is[], soaccountModelOptionsreturns{},bindableSetisundefined, and this PR's new model send-gate never arms.
Claude Opus ｜ 𝕏
| const webviewDir = fileURLToPath(new URL('..', import.meta.url)); | ||
| const daemonDir = fileURLToPath(new URL('../../daemon', import.meta.url)); | ||
| const viteCli = fileURLToPath(new URL('../../bin/vite.js', import.meta.resolve('vite'))); | ||
| const newSessionDefaultsKey = 'linkcode.workbench.new-session-defaults:v7'; |
There was a problem hiding this comment.
This key moved twice inside this PR (:v5 → :v6 → :v7), and this line is the second place a hand-copied copy drifted. The first was new-session-defaults-store.test.ts, which this PR fixed structurally — by importing the constant, with a comment saying the mismatch had turned a test vacuous. Here the same obligation is only a code comment, so the next bump silently breaks this e2e again and the sole signal is a red CI job.
Technical details
# Pin the e2e's storage key to `NEW_SESSION_DEFAULTS_STORAGE_KEY` at compile time
## Affected sites
- `apps/webview/e2e/browser-smoke.e2e.mts:18` — `newSessionDefaultsKey` is a hand-copied string literal; nothing fails if it drifts from the store.
- `apps/webview/e2e/browser-smoke.e2e.mts:76` — `lastHarness` is likewise hand-copied. `PersistedNewSessionDefaultsSchema` is `.partial()`, so a stale field name is dropped by `safeParse` rather than rejected, which is exactly why this drift is silent.
- `packages/client/workbench/src/surface/new-session-defaults-store.ts:19` — the constant exists and is exported for precisely this reason, but is not re-exported from the package barrel (`src/index.ts` covers `./surface/*` selectively and omits this module).
## Required outcome
- A future bump of the storage key or a rename of a persisted field fails `pnpm typecheck` rather than only the webview e2e job. `apps/webview/e2e/tsconfig.json` is already a root `tsconfig.json` reference, so the e2e file is covered by the solution build.
## Suggested approach
A runtime import is not viable — the e2e runs under plain `node` type-stripping and the workbench barrel pulls in React, zustand and CSS. A type-only pin costs nothing at runtime:
```ts
const newSessionDefaultsKey: typeof import('@linkcode/workbench').NEW_SESSION_DEFAULTS_STORAGE_KEY =
'linkcode.workbench.new-session-defaults:v7';
```
`export const NEW_SESSION_DEFAULTS_STORAGE_KEY = '…'` already infers the string literal type, so the assignment stops compiling the moment the key changes. This needs one line added to `packages/client/workbench/src/index.ts` to put the module on the barrel (the package's `AGENTS.md` forbids consumers deep-importing other paths).
## Open questions for the human
- Worth doing the same for the persisted field names, or is the code comment enough there? A `keyof` pin would need `PersistedNewSessionDefaults` exported too, which is more surface than the key alone.
|
Your Claude subscription has hit its usage limit. It resets at 4:20pm (UTC). Re-trigger Pullfrog after the reset, or add an Add repo secret → · Model settings → · Setup docs → · Ask in Discord →
|
Follow-up: two account surfaces, and a CI fixFour commits since the last update (
|
Closes CODE-574.
Model selection was a free-text field plus a fixed per-agent list that ignored which account was bound. This replaces both with a set of model ids picked per account: fetch what the service serves, tick what you want, and the composer offers exactly that. Ids can also be typed by hand, which is how endpoints that serve no list work.
Not a bug fix — nothing was broken. It removes the guessing.
Breaking
WIRE_PROTOCOL_VERSIONandMIN_COMPATIBLE_WIRE_VERSIONboth move to 74:Account.modelis removed,ProviderConfig.defaultModelrenamed tomodel, andStartOptions.modelloses itsnulltier. Rebuild and restart the daemon and all clients together — a peer below the floor has its frames refused and dies in the handshake timeout.loadConfigmigrates on read (Account.model→models: [{id}],defaultModel→model). Without it zod strips the unknown keys and existing users lose their configured model.Commits
3a0686402d5dc3a1Account.models,ProviderConfig.model, probe reshape, wire 74, migration0bae7397f7435f85c430eba8AGENT_DEFAULT_MODELSremovalNon-obvious bits
The list URL is hardcoded per service rather than derived from the resolved variant, because derivation is wrong wherever variants sit on different paths — DeepSeek's
anthropicvariant would give/anthropic/v1/models, Vercel's bare-origin one a root/models. All six URLs checked against vendor docs; both Cloudflare services carry none, since/compathas no models route.In the account's model set, present-but-empty and absent differ.
[]means an account is bound with nothing picked, so sending is blocked to match the daemon's refusal. Absent means no account is bound, where the agent resolves its own model and is not blocked — blocking there would break opencode/pi on their own auth.Ids only, no metadata. Both provider-routed agents accept a bare id and fill the rest themselves, and for pi declaring a model it already knows is harmful:
applyModelsJsonreplaces on id match, so redeclaringdeepseek-v4-prooverwrites its real 1M context window with pi's 128k default. Checked against opencode's config schema (allModelfields optional, v1 and v2) and pi'smodelFromJson.Fetch sources are injected rather than called in the forms, which are presentation and sit outside the data-plane provider tree. Catalog services probe with the unsaved secret, saved accounts probe by id so the stored secret stays daemon-side, and subscriptions read codex's start catalog or — for claude-code, which has no enumeration API — the curated table.
Selection lives in the edit form rather than the account detail pane as CODE-574 task 5 described, since
EditAccountFormalready routes non-OAuth accounts throughCustomAccountForm.Checks
pnpm check:ciexits 0.pnpm test: 2742 passing. Two failures are outside this change's module graph — the knownpackages/host/assetsregistry loopback test, andpackages/host/enginerun-command's timeout assertion, which passes in isolation and whose imports (node:child_process,effect,foxts/noop,../observability) this branch never touches.Not driven in the real app. Needs a live DeepSeek key: add the account, Refresh, multi-select, confirm the composer offers that set for every bound agent, confirm send is blocked with none picked, start a session on a picked model, and confirm an upgraded config keeps its previous model.
Follow-ups
cloudflare-gatewayentry points at/compat/chat/completions, which Cloudflare has deprecated in favour ofapi.cloudflare.com/client/v4/accounts/{ACCOUNT_ID}/ai/v1/chat/completions— a different URL shape, so it needs its own issue./v1/modelsreturns per-model effort capabilities anddisplay_namein a response we already parse; useful for the effort picker.