Skip to content

fix(daemon): surface model selection failures and report executed models - #2768

Merged
zfy0701 merged 2 commits into
mainfrom
codex/report-model-selection-failures
Oct 2, 2026
Merged

zfy0701 merged 2 commits into
mainfrom
codex/report-model-selection-failures

Conversation

@zfy0701

@zfy0701 zfy0701 commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

An explicitly configured model could be rejected by ACP while the daemon continued on the runtime default, logging only Internal error. Reviews then showed default even 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

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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. selectSessionConfig throws ModelSelectionError only when the runtime advertises a model selector and the requested model is not offered, is rejected, or is not confirmed as currentValue. 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/new or session/load fails, discardSession drops only the local live and sessionConfigs state; the adapter's saved session is left alone. In openRuntimeSession, 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. recordRuntimeUsageSnapshot replaces three copies of the same code. It ignores default and <synthetic> and updates the session status only when the observed model changes. Replies use the observed model first; when it is null, 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

  1. Saved chat model choices. reapplyStickyControls used to fall back to the agent's configured model when a user's /model choice 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".
  2. Exact confirmation. After set_config_option, the runtime must echo the exact requested value as currentValue. Current adapters do this, but an adapter that rewrites model ids would now fail. Keep this in mind for future runtime upgrades.
  3. Between turns. captureTurnModel still writes null at the start of each turn while the selector says default, so the status shows default until the first usage_update arrives. 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

@agentconnect-md-test agentconnect-md-test Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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. recordRuntimeUsageSnapshot now returns the concrete model from _claude/model, and the live usage path saves it on the turn as p.signals.runtimeReportedModel. Replies use that value first, then the live status-bar model, then the starting turnModel. Replies no longer read the stored model, which is overwritten with null at the start of each turn. That settles my earlier third note.
  • Review footers during a turn. storedSessionExecution uses 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 starting default or 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, captureTurnModel reads the model picker again after runPromptLoop and before settleUsage. 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. turnModel is already declared with let in the calling function, so this works.

Carried over (non-blocking)

  1. If a user's saved /model choice disappears from the runtime, every turn in that session fails until they pick another model. The error could mention /model as the way out.
  2. 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

@zfy0701
zfy0701 merged commit 61a8ec0 into main Oct 2, 2026
14 checks passed
@zfy0701
zfy0701 deleted the codex/report-model-selection-failures branch October 2, 2026 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant