fix(daemon): surface model selection failures and report executed models - #2768
Conversation
There was a problem hiding this comment.
Review: approve
This PR fixes the silent fallback to the runtime's default model and the default attribution. I found nothing blocking.
What I checked
- Model selection is now strict.
selectSessionConfigthrowsModelSelectionErroronly when the runtime advertises a model selector and the requested model is not offered, is rejected, or is not confirmed ascurrentValue. Runtimes with no selector still skip quietly. If the model is already selected, nothing is sent. Other options (effort, mode, fast mode) still only log a warning when they fail. - Setup failures are cleaned up. When
session/neworsession/loadfails,discardSessiondrops only the localliveandsessionConfigsstate; the adapter's saved session is left alone. InopenRuntimeSession, a model rejection now stops both fallbacks: it no longer retries without the extra MCP servers, and it no longer recreates the session after a failed load. That stops the original error from being hidden as an MCP or resume failure. - Attribution uses the model that actually ran.
recordRuntimeUsageSnapshotreplaces three copies of the same code. It ignoresdefaultand<synthetic>and updates the session status only when the observed model changes. Replies use the observed model first; when it isnull, they fall back to the status bar's model as before. Review footers take both runtime and model from the observed turn, which keeps them correct after the host is evicted.
Non-blocking notes
- Saved chat model choices.
reapplyStickyControlsused to fall back to the agent's configured model when a user's/modelchoice failed. If a saved choice later disappears from the runtime (say, after an adapter upgrade), every turn in that session now fails until the user picks another model. That matches the new product convention, but the error could say how to recover, e.g. "pick another model with /model". - Exact confirmation. After
set_config_option, the runtime must echo the exact requested value ascurrentValue. Current adapters do this, but an adapter that rewrites model ids would now fail. Keep this in mind for future runtime upgrades. - Between turns.
captureTurnModelstill writesnullat the start of each turn while the selector saysdefault, so the status showsdefaultuntil the firstusage_updatearrives. It's cosmetic and corrects itself.
I couldn't run the test suites here (dependencies aren't installed in this checkout). I read the new acp-host and daemon tests for new/load/switch rejection and rejected sticky models; they cover the important paths.
sent by review-bot (Claude Agent · opus) · open in session
There was a problem hiding this comment.
Review: approve (re-review at 2d0a4d0)
The new commit fixes a stale-model problem. Before it, a turn's starting default model could hide newer model information while the turn was still running. Nothing blocking.
What changed since 030e96d
- The current turn tracks its own reported model.
recordRuntimeUsageSnapshotnow returns the concrete model from_claude/model, and the live usage path saves it on the turn asp.signals.runtimeReportedModel. Replies use that value first, then the live status-bar model, then the startingturnModel. Replies no longer read the stored model, which is overwritten withnullat the start of each turn. That settles my earlier third note. - Review footers during a turn.
storedSessionExecutionuses the stored model only when no turn is running, or when the running turn has already reported a model. While a turn is running without one, the footer falls back to the live model picker. That's the right way round: the stored model is either the startingdefaultor the previous turn's model, and neither should hide what the picker shows now. After the host is evicted with no turn running, the stored model is still used as before. - Re-reading the model after the prompt. When the runtime reported no model,
captureTurnModelreads the model picker again afterrunPromptLoopand beforesettleUsage. Cost and the saved model therefore reflect any model switch during the prompt. When a model was reported, the re-read is skipped, so the reported model isn't overwritten.turnModelis already declared withletin the calling function, so this works.
Carried over (non-blocking)
- If a user's saved
/modelchoice disappears from the runtime, every turn in that session fails until they pick another model. The error could mention/modelas the way out. - Model confirmation still needs the runtime to echo back exactly the requested model id.
I didn't run the tests (dependencies aren't installed here). I read the updated daemon-transcript test changes; they cover the live-model and late-usage attribution cases.
sent by review-bot (Claude Agent · opus) · open in session
An explicitly configured model could be rejected by ACP while the daemon continued on the runtime default, logging only
Internal error. Reviews then showeddefaulteven when Claude ACP reported the concrete model in usage metadata.This change stops the turn when an advertised model selection fails and surfaces the requested model plus the runtime's detailed error. Failed setup cannot be reused or retried as an unrelated MCP/resume failure. Runtime-reported models now update session metadata, usage reports, and reply/review attribution, including late usage updates and attribution after host eviction. Concrete execution reports take precedence; otherwise attribution reads the live session selector and saves its final value when the turn completes.
Validation: focused ACP and daemon suites with one worker, daemon typecheck, and repository lint. Regression coverage includes new/load/live model rejection, preserving a resumed session, visible errors before prompting, actual-model reporting while the selector stays on
default, and model metadata changing during a prompt. The attribution and session regression suites pass all 218 tests.Related to #2763.
Created by Codex . GPT-6