fix(acp): fall back to a renamed model id and announce it - #943
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c8e6c8173
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if let Some((fallback_value_id, fallback_label)) = | ||
| sole_digit_insensitive_match(option, value_id) |
There was a problem hiding this comment.
This fallback runs for every persisted select option, including ThoughtLevel, because the resolver receives only a display label rather than the selection category. If an agent replaces a numeric effort ID such as thinking-16000 with the sole option thinking-32000, the skeletons match and Staged silently applies a materially different reasoning budget instead of reporting the pinned effort as unavailable. Limit this heuristic to SessionConfigOptionCategory::Model so model-generation recovery cannot alter unrelated configuration semantics.
Useful? React with 👍 / 👎.
| if let Err(e) = self.store.add_authored_session_message( | ||
| &self.session_id, | ||
| MessageRole::Assistant, | ||
| notice, | ||
| CONFIG_OPTION_FALLBACK_EVENT, |
There was a problem hiding this comment.
Persisting the notice as an ordinary Assistant row makes existing backend consumers treat it as agent output despite the tag. For example, session_completion.rs considers the presence of any assistant message proof that AI handled a failed non-fast-forward push, while project_mcp.rs::last_assistant_output returns the latest assistant row as the child result. When a fallback occurs and the agent completes without emitting assistant text, the notice can therefore classify an unhandled push as succeeded or be returned as the generated result. Either represent this as a distinct presentation event or update all agent-output consumers to exclude config_option_fallback rows.
Useful? React with 👍 / 👎.
`select_option_has_value` re-implemented the grouped/ungrouped traversal that `select_option_values` already does, down to the same conservative `_ => false` arm for unrecognized option shapes. Express it as a search over `select_option_values` instead: same semantics, including `None` for a non-select option, and one fewer place to update when the enum grows a variant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Tier 3 of `resolve_config_option_value` skeletonized the raw pin, so it could only ever reach the catalog shape `session/new` serves. Tier 2's premise is that the Claude bridge advertises `claude-fable-5[1m]`, resolves it to bare `claude-fable-5`, and rebuilds the picker around the resolved id on `session/load`. Stack that with the Fable 5 -> 5.1 rename and the two tiers stop composing: turn 1 resolves `claude-fable-5[1m]` to `claude-fable-5-1[1m]`, then turn 2 sees only bare `claude-fable-5-1`, strips the pin to an absent `claude-fable-5`, and compares skeleton `claudefablem` against `claudefable` — no match, turn dies before `session/prompt`, pin cleared. A pre-existing session resumed after a rename goes straight there with no turn 1 at all. Drop the bracketed hint before comparing letters and all three ids collapse to the same family, so one rung covers both catalog shapes. That costs the free one-way guarantee — the hint's letters no longer survive to keep `opus` away from `opus[1m]` — so state it outright: a row may match only when its hint is absent or equal to the pin's. That also closes a hole the letters had by accident, since a letterless `[200]` hint was invisible to skeletonization. When a catalog serves both shapes of the renamed generation, prefer the hint-exact row so the user's `[1m]` survives; ambiguity within the preferred group stays terminal, and with a single rung there is nothing below to fall through to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
1 participant
When a model generation is renamed underneath a pinned selection —
claude-fable-5[1m]leaving the catalog andclaude-fable-5-1[1m]taking its place — every stored selection naming the old id was stranded, and the turn died beforesession/promptwith the pin reported unavailable.Changes
Digit-insensitive resolution (
crates/acp-client/src/driver.rs) —resolve_config_option_valuegains a third tier after the exact match and the existing hint-stripped match: the unique option row that differs from the pin in digits and punctuation alone. Ambiguity is not guessed — two candidates (Fable 5.1 and 5.2 for a pin naming Fable 5) still report the pin as unavailable. The bracketed context-window hint's letters survive skeletonization, so a bare pin can never be promoted onto a[1m]row.Transcript announcement — unlike the hint-stripped fallback (same model, fires every follow-up, stays log-only), this tier lands on a genuinely different value, so it is surfaced. A new defaulted
MessageWriter::on_config_option_fallbackcarries the notice, fired only after the agent accepts the substitute so the transcript never claims a fallback that did not take effect. One-shot callers that parse the writer buffer as JSON get the no-op default.Persistence — the notice lands as an ordinary assistant row via a new
Store::add_authored_session_message, tagged with aconfig_option_fallbackACP event kind. Because the agent never said it,session/loadreplay can never reproduce it, soreplay_boundaries_from_messagesexcludes such rows — a boundary that can never match would stall the match cursor for the rest of the session.Testing
Unit tests cover the rename resolution, the ambiguous-rename refusal, the bare-pin/hinted-row guard, boundary exclusion of authored rows, and an end-to-end protocol test asserting the renamed id is what gets set and the notice reaches the writer.