feat(providers,schema,engine,workbench): retire the default model — user-selected model sets from provider model lists - #426
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.
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 ｜ 𝕏
A delegated subscription is an account like any other, so the host creates it from the runtime probe instead of waiting for the user to import it from a suggestion card.
…ot connected' The badge read as a connection state while it only meant no agent falls back to this account, so a signed-in subscription and a working key both reported not connected.
… the list header Neither answered a question the list itself does not: the rows are the account count, and the binding tally described a fallback the user never reads at that altitude.
There was a problem hiding this comment.
ℹ️ Adoption itself is sound and the tests are real. One comment in
engine.tsasserts a client-revalidation property that doesn't exist — inline. Plus one product decision worth confirming deliberately, since the deleted card was the only opt-out.
Reviewed changes — three commits since review 9 (6293aad2 → 3a32223, 13 files, +185/−133): detected agent-CLI logins are now adopted into the account pool by the daemon, and the client-side suggestion card that used to do it is removed.
adoptDetectedLogins(packages/host/engine/src/agent/detected-logins.ts) — mints oneacc_OAuth account per logged-in CLI that no account represents yet, and binds nothing. I checked the read-then-write the comment warns about: bothProviderConfigStore.updateimplementations (provider-config.ts,apps/daemon/src/provider-store.ts) are synchronous, sogetAccounts()→await update(...)has no interleaving point and the claim holds today. It rests entirely on that — a store that ever awaits a disk write insideupdatereopens the window the comment closes.- Wired into
AgentRuntimeService'sonChanged, so adoption runs on every probe result.detectedLoginsfilters on already-represented, so repeats are no-ops. - Three engine tests asserting the right things —
expect(providerStore.get()).toEqual({})pins "adopted but not bound", which is the property everything below depends on, and the concurrent-write case asserts the other side's account survives rather than just checking a count. - The suggestion card, the account/bound count header, and the
unbound"Not connected" badge are gone fromaccount-master-list.tsx, withonAdoptDetected/handleAdoptDetectedremoved behind them. I checked for orphans:settings.providers.detected,accountCount,boundCountandunboundare dropped from both locales; the survivingdetectedaten.ts:997isruntimeSource.detected, still used;loggedInstill has three call sites;PlusIconandServiceIconare still referenced in the file. - The send gate is untouched — I traced this specifically because review 2's finding lived exactly here. An adopted OAuth account is bindable only for its own agent (
resolve.ts:48-52), and the only two OAuth services are claude-code and codex, both of which have staticAGENT_MODEL_OPTIONSentries. SoaccountModelOptions[kind]going from absent to[]unions back to the identical list, andthroughAccountkeys onactiveAccountId, which adoption never sets. No agent's menu shrinks andsendBlockedcannot arm from an adopted account.
Adoption has no opt-out, and deleting an adopted account undoes itself
No line to anchor this to, and it may well be the intent — but it's a behavior change that the diff makes silently, so it's worth confirming rather than inheriting. The card this delta deletes was the opt-in; nothing replaces it. detected-logins.ts's doc owns the headline consequence — "a user who deletes the account gets it back on the next probe" — and the reasoning (the pool describes what exists) is coherent. Two second-order effects it doesn't cover:
- It isn't only "the next probe".
startSeedcommits withpushUnchanged: true, soonChanged— and therefore adoption — fires on every daemon restart even when the runtime snapshot is identical. There is no state in which a deleted CLI-login account stays deleted while the CLI stays logged in. - Re-adoption mints a new
acc_uuid. Anything keyed on the old id doesn't follow it:providers[kind].enabledAccountIds,activeAccountId, and theSessionRun.accountIdpins this PR just made durable on existing threads. So a delete/re-adopt cycle doesn't restore the prior state — it silently unpins threads that were running on that account.
Neither is a defect if the answer is "the user manages this by logging out of the CLI." If it isn't, an opt-out flag is a lot cheaper than bringing the card back.
Claude Opus ｜ 𝕏
…one place Both sides need the same ordered list — the composer to show its head, the daemon to start on it — so it cannot be derived independently per side.
The composer read two daemon-configured defaults to decide what an unpicked draft runs on and whether the agent's own catalog was a candidate at all. It now reads the list: the accounts' models lead it, the agent's own follow, and with no account the agent's advertised default is the only honest thing to show.
… per-agent state The account dialog carried a second axis — a star naming the agent's fallback, and a model select on that one pairing — whose whole job was to fill defaults nothing reads any more. What is left is one switch, and the rows that read it now answer 'which agents offer this account'.
…ring one BREAKING: `ProviderConfig` loses `activeAccountId` and `model`, and `config.account.create-and-bind` is gone, so both wire versions move to 76. A request that names no account or model resolves to the head of the agent's enabled-account model list — the entry the composer already shows — so neither side stores an answer the other can contradict. `loadConfig` folds an old default account into the enabled list rather than dropping the access it granted.
…carries The menu drew from three sources and the account switches governed one of them, so turning a subscription off left its models in place. The agent's own catalog is no longer a source at all: the curated table moves to the provider layer, where an adopted CLI login is seeded with it, and every row in the picker now belongs to an account a switch can take away.
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.