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 24 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 ｜ 𝕏
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 ｜ 𝕏
"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 ｜ 𝕏
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 ｜ 𝕏
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 ｜ 𝕏
|
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 (
|
Review dispositionsReplied on each thread. Summary, since they span three separate pullfrog runs:
Each fix was validated against the code before being taken, and none of the suggested diffs were applied verbatim — two of them (the nested ternary, and Coverage gap worth flagging: the last successful pullfrog run predates |
There was a problem hiding this comment.
Important
Five of the nine prior findings are genuinely fixed — I verified each against source, replied, and resolved the threads. The two new defects here share one shape: each replacement mechanism this PR introduced was wired into the new-session path but not the live-session path. The config-level model memory was deleted and replaced by per-run attribution that no in-place set-model ever writes; the replace-the-menu bug was replaced by a union that only NewSessionSurface performs.
Reviewed changes — five commits since my last review (9ceb386 → acca89dc, 23 files, +684/−225). I read the whole 9ceb386..acca89d delta rather than the supplied incremental range, since the last Pullfrog review sat at 9ceb386.
Fixes I verified and resolved:
probeSecretnow takes the service (1e930040) and refuses both a cross-service account and a service-less one. The test asserting the stand-in relay is never contacted is the right assertion — a message-only check would not prove the key stayed off the wire.beginRunnotifies (1e930040), and the regression test asserts thesession.changedframe itself rather than reading back throughsession.list, which was never the stale layer.throughAccountreplaces thecatalog?.defaultModelarm (acca89dc), keeping thedefaultModelsarm intact as the constraint required. Declining my nested-ternary suggestion for an explicit flag was the better call.- The draft pin is now
typeof localAccount === 'string', with display resolution split off separately. Splitting "what do we show" from "what do we pin" is a real improvement over my suggested guard, which still conflated them. persistPickedModelis deleted rather than fixed. I agree with the reasoning on the PR: the agent's configured default is an explicitly managed setting that automation and IM threads read, and one thread's pick must not overwrite it.
I also confirmed withAccountEnabled correctly clears activeAccountId when the default account is disabled, and that the new enabledAccountIds absent-means-all default keeps a newly added account from needing a Settings visit for agents the user never narrowed.
🟠 A mid-session model pick is not recorded, so a relaunch silently reverts it
Anchored inline on lifecycle-service.ts. switchModel's same-account fast path forwards through sendInput and writes nothing; runOf (lifecycle-service.ts:560) is the only writer of SessionRun.model, and its four call sites (107, 182, 438, 485) are all launch points. So pinnedOptions replays the model the run started on, not the one the user last chose.
I dispatched a specialist specifically to falsify this and it held on all four escape hatches: session.attach → replay() only emits model-update to the reconnecting client (live-session.ts:167-182) and never writes a run; nothing reads LiveSession.currentModel into beginRun/pinnedOptions/resolveForRecord; and the adapters take opts.model — which is pinnedOptions — over any transcript-side model (opencode's server-side session record is the lone accidental exception, and only because it reads got.data.model when !opts.model).
Before this PR the deleted usePersistPickedModel covered this through config. Its replacement covers a narrower set of relaunches, so agent-adapter/AGENTS.md:116 currently overstates the guarantee — "a thread keeps its own pick" is true of the model a run resolved to, not of the pick made through the very onSetModel path the same bullet describes two sentences later.
The test at engine-session-records.test.ts:1164 pins the no-record behavior (runs stays length 1) but stops before the consequence; no test performs a same-account switch and then resumes.
🟠 The union fix landed on the composer only — the live thread still replaces its menu
Anchored inline on conversation-surface.tsx:196, which is still accountModels ?? conversation.availableModels. shell-frame.tsx:252 feeds it the same accountModelOptions output NewSessionSurface receives, so the exact scenario acca89dc fixed for drafts persists for running threads: a live pi or opencode session on its own CLI login has its adapter-advertised catalog discarded in favour of an unrelated vendor account's models, because that account is bindable for pi/opencode (resolve.ts:149-151 returns native for any protocol).
enabledAccountIds adds a second way in. Disabling every account for an agent yields [] — present, and [] is truthy for ?? — which still discards conversation.availableModels. I checked what [] actually renders rather than assuming: composer.tsx:802-807 falls back to AGENT_MODEL_OPTIONS[agentKind], so claude-code/codex/grok-build quietly drop to the curated static table. opencode and pi have no entry in that table, so modelOptions is undefined, hasModels is false, and the control still renders (via selectedModelId !== null) showing the current model over a menu with nothing in it.
The composer recovers from all of this — once withAccountEnabled clears the default account, throughAccount is false and pickableModels unions the adapter catalog back in. Nothing does that here.
I had previously dismissed this call site as "only reachable if the user clears the set after start, where empty is arguably correct." The per-agent enabled set makes it reachable from a Settings toggle that says nothing about live threads, and the CLI-login case above doesn't require any toggling at all, so I'm re-raising it. It also compounds the still-open switchesAccount(opt, undefined) === false gap: those foreign entries carry no restart hint, yet picking one takes switchModel's relaunch branch.
ℹ️ Nitpicks
Four doc/edge items inline — two comments that now describe the opposite of what the code does (default-models.ts on [] blocking sends, view.ts on later-added accounts joining the list), a stale-pin edge in pinnedOptions, and leftover "bound account" wording on the accountModels prop.
Still open from earlier reviews and not re-raised here: applyProviderDefaults never validating config.model against account.models; the switchesAccount undefined-account asymmetry; effort/approval-policy loss across a relaunch (accepted as pre-existing on rewritePrompt too, and correctly deferred); the e2e storage-key literal.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
Important
The mid-run pick now survives a relaunch, but model-update isn't the "a switch was accepted" signal the new comment assumes — adapters also emit it at launch to reflect what they resolved, and that reflection is now latched as a durable model override.
Re-reviewed acca89dc..f91b7235 (11 files, +194/−36). All six threads from the previous review are addressed; five are resolved, and session-record-registry.ts:213 stays open because the fix it asked for has a residual gap (inline on provider-config.ts).
Landed well
setRunModel's "notifies nobody" rationale checks out —SessionInfoSchemainschema/src/model/session/record.tsprojectshistoryIdandaccountIdbut notmodel, so there's no identity change to broadcast.pickableModelspromoted toshell/agent-models.tsand made null-tolerant is the right shape, andconversation-surface.tsxnow derivesthroughAccountfromaccountIdinstead of??-replacing the live catalog. The union entries keep distinctmodelChoiceKeys (account entries carryaccountId, adapter-catalog entries don't), so no key collision.- Both new engine tests fail without their fix — I checked the deleted-account one refuses to pass against the pre-
f91b723resolver.
Two things to look at, both inline.
Claude Opus ｜ 𝕏
… by what resolved for it Two fields each carried a request and an observation: `StartOptions.config.accountId` was both the client's pin and the resolver's echo of what resolved, and `SessionRun.model` was documented as what a run resolved to while `pinnedOptions` replayed it as a choice. Splitting both removes the patches that reconciled them.
There was a problem hiding this comment.
Important
edda106's split is right and both review-7 findings are structurally fixed. But the per-run pin the whole design now rests on is never written to disk: session_runs has no columns for accountId, model, effort or approvalPolicyId, so setRunIntent's persist(record) drops the four fields it just recorded. After a daemon restart a thread silently returns to activeAccountId.
Re-reviewed f91b7235..5bd3c4df (13 files, +297/−148) — edda1063 plus a docs-only commit. All 11 prior pullfrog threads across reviews 4, 6 and 7 are already resolved with substantive author replies; nothing to retire.
Both review-7 findings are fixed structurally rather than patched. Splitting the request (StartOptions.accountId) from the resolution (AppliedProviderDefaults.accountId) removes the early-return ordering trap outright — no widened guard needed. And moving pick-recording out of session-event-processor.ts into applyInput → recordAccepted means an adapter's launch-time model-update reflection can never be mistaken for user intent, which is what made the declined endedAt guard genuinely unnecessary. Review 4's deferred PRRT_kwDOS8Iqx86XMT0_ (mid-session effort/approval lost on relaunch) is also closed by pinning those two fields.
Checked and clear
Four hypotheses I chased and falsified — recording them so they aren't re-raised:
- Pinning
effort/approvalPolicyIdoff a non-throwing call is not bogus.packages/host/agent-adapter/src/base.ts:206-215—onSetApprovalPolicyandonSetEffortboth default toPromise.reject(...), so "didn't throw" really does mean the adapter accepted it.onSetModeat:202returnsPromise.resolve(), which is exactly whymodeIdis correctly excluded from the pin. - Client-side
accountIdwiring is complete.workbench.tsx:383,468→control-channel.ts:103,272; the per-session pin reaches the daemon on both the start and the switch path. - The wire floor is right. 75/75 — every schema change in this PR is additive-optional, so the
MIN_COMPATIBLE_WIRE_VERSIONbump to 75 alongsideWIRE_PROTOCOL_VERSIONis consistent with what actually changed. - Routing every
agent.inputthroughapplyInputdoes not regress plainset-model.LiveSession.runforks into the session scope (live-session.ts:68-78) rather than serializing, so the widened path does not hold the permit-1 semaphore across a turn; andsendInputalready threwnot_foundviarequireSessionbefore this PR, soswitchModel's newconflict: Session is not runningis not a new failure mode.
One observation, no change requested
If the pinned account later stops being bindable (revoked OAuth, a service whose templated endpoint is no longer filled), the thread refuses to resume rather than falling back to the configured default. That asymmetry is deliberate and documented — packages/foundation/providers/AGENTS.md says an unavailable binding "must fail a session start loudly" — so I'm not asking for a fallback. Worth noting the interaction, though: today a daemon restart is what clears such a strand, and only because the pin isn't persisted. Fixing the finding above removes that accidental escape hatch, so it's worth deciding then whether a stranded thread needs a user-visible way out.
Claude Opus ｜ 𝕏
`session_runs` had no columns for the account, model, effort or approval tier, so every pin was dropped on write and parsed back as `undefined` after any daemon restart — a thread moved to another account silently returned to the agent's default. The engine's in-memory store round-trips whole objects and cannot see this, hence the store-level test.
There was a problem hiding this comment.
✅ No new issues found. The persistence fix is complete and I verified it end-to-end, including the parts a migration generator can quietly get wrong.
Reviewed changes — one commit since the prior review (5bd3c4df → 6293aad2, 7 files, +999/−0, purely additive), closing review 8's finding that the per-run pin never reached disk.
- Four nullable columns on
session_runs—account_id,model,effort,approval_policy_idinapps/daemon/src/db/schema.ts, with a doc comment stating why each must round-trip. All five optional fields onSessionRunSchemanow have a column; nothing is left behind. - Both mapping sites —
save'svalues(...)andtoRecord'sruns.map(...)insession-store.ts,?? nullon write and?? undefinedon read, matching the existinghistoryIdtreatment. - Generated migration
0009_add_session_run_pin— I checked the three things a hand-edit or a bad regen breaks:0009_snapshot.prevId(df784a45…) equals0008_snapshot.id, the journalwhen(1786331414776) is monotonic after0008's, and the fourALTER TABLE … ADDstatements are valid SQLite. GivenreconcileMigrationLedgerkeys "already applied" on hash andwhen, this is exactly the shape that upgrades cleanly and re-runs on nobody. - A store-level round-trip test —
apps/daemon/src/__tests__/session-store.test.ts. The first case deep-equals a fully-populated two-run record across twocreateSessionStoreinstances, which is the assertion that actually fails against the old mapping; a looser check on one field would not have. Both tests pass locally. - The trap recorded in
apps/daemon/AGENTS.md— "a record field with no column is dropped in silence", naming the three edits plus a migration and stating thatInMemorySessionStorestructurally cannot catch a miss. That is the durable half of this fix, and it is in the right file.
I also traced the fix past the repo: the migration reaches packaged desktop users, not just dev. vite.main.config.mts:66-68 cpSyncs apps/daemon/drizzle into out/drizzle, and scripts/verify-artifacts.mts:109 gates the asar on out/drizzle/meta/_journal.json — so a dropped migrations folder fails the release rather than shipping a daemon that boots without the columns.
ℹ️ The stranded-pin question review 8 deferred — it answers itself
Review 8 flagged that a daemon restart was, accidentally, the only thing clearing a thread pinned to an account that had stopped being bindable, and deferred "does a stranded thread need a user-visible way out?" until the pin was durable. It is durable now, so: there is already a way out, and no change is needed.
resolveAccount (packages/host/engine/src/agent/provider-config.ts:90-96) iterates [opts.accountId, config?.activeAccountId] and accounts.finds each id, so an account deleted from the pool simply misses on the pin and falls through to the agent's default. Only a present-but-unavailable account strands the thread, and both repairing it (re-auth, filling the templated endpoint) and deleting it clear the strand from Settings. Blunt, but reachable without a terminal — and refusing loudly in the meantime is the documented policy in packages/foundation/providers/AGENTS.md. Recording this so it isn't re-opened.
Claude Opus ｜ 𝕏
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.