fix(agent-mode): switch to the picked model on the first click, not the second - #2669
fix(agent-mode): switch to the picked model on the first click, not the second#2669zeroliu wants to merge 17 commits into
Conversation
![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: ca9b573941
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59b7af68e5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private reconcileAppliedModel(incoming: BackendState): BackendState { | ||
| const applied = this.lastAppliedModel; | ||
| if (applied === null || !incoming.model) return incoming; | ||
| if (incoming.model.current.baseModelId === applied.baseModelId) return incoming; |
There was a problem hiding this comment.
When a reported snapshot keeps the pinned base model but authoritatively changes its effort, this branch accepts the new state without updating lastAppliedModel. For example, after pinning sonnet with default effort, a same-model push can change it to high; if a delayed different-model snapshot then arrives, the reconciliation below restores the obsolete sonnet/default pin and silently undoes the accepted effort change. Update the pinned effort when accepting a same-base snapshot so later stale-model reconciliation preserves the latest trusted selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in 9be1fdc: reconcileAppliedModel now refreshes the pin's effort when trusting a same-model snapshot, so a later stale different-model push restores the latest trusted selection instead of the obsolete apply-time effort. Regression test "preserves a trusted same-model effort change when rejecting a later stale revert" reproduces the bug (fails pre-fix, verified red).
| if (next && provenance === "confirmed") this.rememberAppliedModel(next); | ||
| this.currentState = next && provenance === "reported" ? this.reconcileAppliedModel(next) : next; |
There was a problem hiding this comment.
Every "confirmed" config-option response is treated as a model confirmation, although the Claude and Opencode callers also use this provenance for effort-only writes. If a model change and an effort change overlap, an effort response can carry the older base-model snapshot; this line then replaces both currentState and the pin with that old model, so a later truthful state push is reconciled back to the wrong selection. Effort-only responses should preserve or reconcile the currently pinned base rather than repinning it from an unrelated full-state snapshot.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in 9be1fdc: effort writes now pass "reported" instead of "confirmed" (claude effort + persisted-effort replay, opencode thought_level), so an effort response snapshot is reconciled against the pin rather than re-pinning the model from an unrelated snapshot. "confirmed" is now reserved for genuine model applies. Combined with the pin-effort refresh from the sibling comment, a same-base effort response still lands verbatim and keeps the pin fresh. Regression test "keeps the applied model when an effort apply's response carries a stale model" pins the session-side policy; opencode descriptor tests pin the caller provenance.
| // reported state makes the guard fire; no blink results because no notify | ||
| // fires between this reset and the confirming setModel below. Guard-less | ||
| // setModel backends round-trip regardless, so they're unaffected. | ||
| this.applyState(originalState, "seed"); |
There was a problem hiding this comment.
For set-model backends such as Claude and Codex, this newly unconditional reset removes the optimistic picked model while the real switch is still awaiting its backend response. Although this write itself does not notify, initialize() has just changed the session status from starting to idle, so React's asynchronous external-store render can read this reset state and show the backend default until the switch finishes; a slow setSessionModel therefore makes the picker visibly jump back to the wrong model. Preserve the seeded display separately while making descriptor guards compare against the reported state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate, and we took your suggested direction. Fixed in 9be1fdc: confirmSeededSelection no longer resets the seed; it threads the reported startup state into descriptor.applySelection as an optional third parameter, and the Claude/opencode guards compare against that instead of the seeded session state. The picker keeps showing the pick during the confirmation round-trip while the guard still fires the real switch. Regression test "keeps the optimistic seed visible while the confirming switch is in flight" asserts the pick is visible mid-round-trip (fails pre-fix, verified red).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9be1fdc04c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!incoming.model.availableModels.some((m) => m.baseModelId === applied.baseModelId)) { | ||
| return incoming; |
There was a problem hiding this comment.
When a config_option_update removes the pinned model and reports a fallback model, this branch accepts the fallback but leaves lastAppliedModel pointing at the unavailable selection. If a later catalog update makes the old model available again while the backend remains on the fallback, the next reported snapshot reaches the merge below and rewrites current back to the obsolete pin, causing the picker to disagree with the model actually used. Clear or replace the pin when accepting a state because its applied model is no longer offered.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in 134804a: when a snapshot is accepted because the applied model is no longer offered, reconcileAppliedModel now clears lastAppliedModel — the backend abandoned that model, so its authority ends there, and a later catalog update re-offering it can no longer graft it back over the fallback actually in use. Regression test "clears the pin when the backend drops the applied model" (verified red pre-fix).
| return { | ||
| ...incoming, | ||
| model: { | ||
| ...incoming.model, | ||
| current: { ...incoming.model.current, ...applied }, |
There was a problem hiding this comment.
When an OpenCode stale snapshot is rejected after switching models, replacing only model.current combines the new selection with the old model's metadata. The translator attaches the thought_level options only to the snapshot's current model, so the pinned target's availableModels entry can now have missing or incorrect effortOptions, and apply.effortConfigId can likewise describe the old model. The picker may consequently hide the target model's effort selector or dispatch effort through stale configuration metadata; reconciliation needs to preserve the confirmed target's associated model metadata as well as its current value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. The active-model effort selector (buildEffortSibling) reads the current entry's effortOptions with no prefetch-catalog fallback, so the graft really would hide it, and opencode's thought-level option id is model-specific, so apply.effortConfigId really would dispatch at the stale model's option. Fixed in 134804a: when rejecting a stale revert, the pinned entry's effortOptions and the previous apply.effortConfigId (both derived by the translator from a snapshot's own active model) are carried over from the previous state; the incoming catalog membership, the stale model's own entry, and mode stay authoritative. Regression test "carries the pinned model's effort metadata through a rejected stale revert" (verified red pre-fix).
| if (incoming.model.current.baseModelId === applied.baseModelId) { | ||
| this.lastAppliedModel = { | ||
| baseModelId: applied.baseModelId, | ||
| effort: incoming.model.current.effort ?? null, | ||
| }; |
There was a problem hiding this comment.
A reported snapshot that keeps the same base model is not necessarily newer than the user's confirmed effort selection: for example, a late state_changed or stale setMode response can carry the previous effort after an effort apply has completed. This branch accepts that stale effort and also overwrites lastAppliedModel, so a subsequent different-model stale snapshot restores the obsolete effort again even though the backend remains on the user's newer choice. Track effort confirmations or request ordering rather than treating every same-base reported snapshot as authoritative for the pin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing this one — the observation is real but it's the documented trade-off, not a defect we can fix without breaking a case we support. Same-base pushes must be trusted because they are the only channel by which legitimate external effort changes arrive (opencode config edits ride config_option_update; pinned by the "honors a state_changed that keeps the applied model but changes effort" test), and the wire carries no ordering metadata to tell a late echo from a genuine change — any policy here guesses. The failure mode needs a double race (a stale echo arriving after a completed effort apply's response) and is display-only and self-healing: the backend keeps running the user's effort, and the very next same-base snapshot is trusted verbatim and restores it. That's categorically milder than the model-revert bug this PR fixes (persistent divergence, wrong model executing turns). Building an effort-confirmation/ordering tracker for that transient isn't warranted; happy to revisit if it shows up in practice.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 134804a9bd
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private applyState(next: BackendState | null, provenance: StateProvenance): void { | ||
| if (next && provenance === "confirmed") this.rememberAppliedModel(next); | ||
| this.currentState = next && provenance === "reported" ? this.reconcileAppliedModel(next) : next; |
There was a problem hiding this comment.
When a user selects two models quickly, the picker starts both asynchronous applies without serialization; if the older request resolves after the newer one, this unconditional "confirmed" handling re-pins the older model. Subsequent truthful state_changed snapshots for the newer model are then rejected by reconcileAppliedModel as stale whenever the older model remains in the catalog, leaving both the picker and later prompts on the wrong selection. Track an apply generation or serialize model changes so only the latest request can update lastAppliedModel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in 21f0d1e with the apply-generation approach you suggested: a monotonic modelApplyGeneration marks the newest in-flight user model apply, and a response that resolves after a newer apply began is demoted from "confirmed" to "reported" — it reconciles against the newest pick instead of re-pinning the older model, so later truthful pushes stay honored. Regression test "a superseded model apply response cannot re-pin over the user's newer pick" drives two out-of-order applies and a follow-up stale push (verified red pre-fix).
| return { | ||
| ...incoming, | ||
| model: { | ||
| ...incoming.model, | ||
| apply, | ||
| availableModels, | ||
| current: { ...incoming.model.current, ...applied }, |
There was a problem hiding this comment.
When a successful effort setConfigOption response carries the stale pre-switch model—the race this change explicitly handles—this merge restores both the pinned base model and its previous effort, discarding the effort value that the backend just accepted. Unless a later state_changed notification happens to repair it, the picker and lastAppliedModel continue to show the old effort while the next prompt runs with the new one. Effort writes need to reconcile the stale model snapshot with the requested effort rather than restoring the entire prior selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing this one — the proposed merge guesses on ground truth the wire doesn't give us. If the effort response's snapshot is merely stale (the broadcast race), the backend is on the new base and the effort landed there — adopting the requested effort would be right. But if the snapshot's model was genuinely current when the write was applied, the effort landed on the old model, and grafting the requested effort onto the pinned base would fabricate a selection the backend never confirmed — the exact failure mode the whole-selection restore exists to prevent. Without ordering metadata the two are indistinguishable, so we keep the conservative merge: restore the last selection the backend confirmed for the pinned base. The cost is a display-only stale effort in a double race, healed by the next same-base snapshot (same-base pushes are trusted verbatim); the alternative risks showing an effort the backend never accepted for that model. Same reasoning as the round-2 ordering comment — happy to revisit if it shows up in practice.
| await this.applyModelWireId(descriptor.wire.encode(selection)); | ||
| return; | ||
| } | ||
| await descriptor.applySelection(this, selection); | ||
| await descriptor.applySelection(this, selection, originalState); |
There was a problem hiding this comment.
For a seeded cross-backend pick that now performs a real model switch, this await runs after backendSessionId has been assigned and recomputeStatusIfChanged() has exposed the session as idle. createSession() also returns without awaiting session.ready, while sendPrompt() gates only on getStatus(), so a user can submit during this round-trip and the first prompt is sent using the backend default—the behavior this change is intended to prevent. Keep the session in starting (or make prompt submission await ready) until confirmSeededSelection finishes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. The "sendPrompt is gated until ready" doc claim was not actually enforced: the status gate opens ("idle") before confirmSeededSelection finishes, and nothing on the send path awaited ready. Fixed in 21f0d1e with the second option you offered: runTurn now awaits ready for turns fired during the startup window, so a fast first submit queues behind the model confirmation instead of racing it — the UI stays interactive and the user message posts immediately. A startupConfirmed flag keeps every later turn on the synchronous dispatch path (callers rely on backend.prompt being invoked synchronously within the turn). Regression test "queues a first prompt fired during the seeded-switch round-trip behind the confirmation" asserts the prompt is not sent while the switch is in flight and fires after it confirms (verified red pre-fix).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f0d1eb32
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| modelId, | ||
| }); | ||
| this.currentState = next; | ||
| this.applyState(next, generation === this.modelApplyGeneration ? "confirmed" : "reported"); |
There was a problem hiding this comment.
When rapid model picks overlap and the older request resolves after the newer one, demoting the older response to "reported" only preserves the newer model in AgentSession's cached display state; it does not restore that model on the backend. In the ACP implementation, each completed setSessionModel call updates its wire cache to that request's modelId, so the late older completion can leave the backend/cache on the older model while this line masks the response as the newer pinned model. Serialize model applies or re-dispatch the newest selection after a superseded request completes so the next prompt uses what the picker shows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate, and you're right that the generation demotion only healed the display. Verified the wire-cache claim: AcpBackendProcess.setSessionModel overwrites wire.models.currentModelId (or the model config option's value) with its own request's modelId on completion, so out-of-order completions corrupt the cache — and a backend processing out of order could genuinely end on the older pick. Fixed in dc98fdb with your first option: model applies are now serialized on a per-session chain (setModel and the confirmed model-option setConfigOption dispatch the next request only after the previous response lands; failures don't wedge the chain), so the last pick is the last write on the backend, the wire cache, and the display alike. The generation mechanism is removed — serialization makes the response-reorder class unrepresentable. Tests: "serializes rapid model applies so the last pick is the last write" (asserts the second dispatch is held and lands last) and "a failed model apply does not wedge the serialization chain" (both verified red pre-fix).
| */ | ||
| private reconcileAppliedModel(incoming: BackendState): BackendState { | ||
| const applied = this.lastAppliedModel; | ||
| if (applied === null || !incoming.model) return incoming; |
There was a problem hiding this comment.
When a runtime snapshot has model: null—for example, a config-option backend temporarily stops advertising its model option—this early return preserves lastAppliedModel. If a later snapshot restores the catalog on a fallback model while still offering the previously applied model, reconciliation grafts the stale pin onto that snapshot even though no RPC switched the backend back, so the picker and subsequent effort dispatch can describe the wrong model. A model-less snapshot should invalidate the pin, or the pinned selection must be explicitly reapplied when model state returns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Dismissing this one — model: null means "this snapshot carries no model information", not "the model state was reset". Snapshots without a model dimension are routine partial reports (e.g. responses that only carry mode on some backends), so clearing the pin on them would strip the revert protection in exactly the window it exists for: a transient catalog blip followed by a stale default echo would land unprotected, resurfacing the original bug. This differs from the catalog-drop case fixed earlier (round 2): there the snapshot affirmatively reported the applied model as gone, which is positive evidence the backend abandoned it; a model-less snapshot is absence of evidence. Under the PR's premise — these backends never self-switch the model — a later snapshot that regresses the model while still offering the pick is a stale echo whether or not a model-less blip intervened, so grafting the pin is the correct call. The scenario where it misleads requires the backend to have genuinely self-reverted while keeping the model available, which contradicts that premise.
| // (a settled promise still costs a microtask, and callers rely on | ||
| // `backend.prompt` being invoked synchronously). A failed startup is | ||
| // already surfaced elsewhere — the prompt proceeds and fails as today. | ||
| if (!this.startupConfirmed) await this.ready.then(undefined, () => undefined); |
There was a problem hiding this comment.
When the user submits during the seeded-model round trip and presses Stop or closes the tab before ready resolves, cancel() sends its backend cancellation before any prompt exists and aborts a signal that the single-agent path never reads. After this await completes, runTurn therefore calls backend.prompt() anyway, causing a cancelled or disposed chat to start an agent turn and potentially consume resources with no visible output. Check the queued turn's cancellation/disposal state after the await, or defer cancellation until the prompt has actually started.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in dc98fdb: after the queued turn's await ready completes, runTurn re-checks disposal and the turn's abort signal; a cancelled or disposed turn finalizes its placeholder with "cancelled", suppresses trailing-stream routing, and returns without calling backend.prompt(). Regression test "does not dispatch a prompt whose turn was cancelled while queued behind startup" cancels mid-round-trip and asserts the turn resolves "cancelled" with backend.prompt never invoked (verified red pre-fix).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc98fdb4ce
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await this.ready.then(undefined, () => undefined); | ||
| // The user may have cancelled or closed the chat while this turn was | ||
| // queued — dispatching the prompt now would start a backend turn | ||
| // nobody is watching. | ||
| if (this.disposed || this.abortController?.signal.aborted) { |
There was a problem hiding this comment.
When the user cancels while this turn is waiting for startup confirmation, cancel() does not abort the controller until after await backend.cancel(...). If the model confirmation resolves while that cancellation call is still in flight, this check sees an unaborted signal and dispatches backend.prompt; because the cancellation notification preceded the prompt, the newly started turn may continue and execute tools despite the user's cancellation. Mark the queued turn cancelled before awaiting the backend call, or otherwise record cancellation synchronously.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate. Fixed in bd1fac7: cancel() now aborts the local controller synchronously before awaiting backend.cancel, so a turn queued behind the startup confirmation observes the cancellation regardless of how slow the backend round-trip is. Regression test "a cancel whose backend round-trip is slow still stops a queued first prompt" keeps backend.cancel pending while the model confirmation lands and asserts the turn resolves "cancelled" with no prompt dispatched (verified red pre-fix).
| if (!this.startupConfirmed) { | ||
| await this.ready.then(undefined, () => undefined); |
There was a problem hiding this comment.
While the seeded model confirmation is pending, the session already reports idle, so the user can pick another model; that apply is appended to modelApplyChain behind the startup apply. However, ready covers only confirmSeededSelection, so this await releases as soon as the seeded apply finishes and can dispatch the first prompt while the user's newer apply is still in flight, causing the turn to run on the seeded model rather than the latest pick. The startup gate needs to include model applies queued during that window.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — accurate, good catch on the interaction between the two new mechanisms. Fixed in bd1fac7: the startup gate now drains modelApplyChain after ready resolves (re-checking the tail in case another apply lands while awaiting), so a first prompt fired during the startup window runs on the user's latest pick, not the seed. Regression test "holds a queued first prompt until a model re-pick made during startup lands" re-picks mid-confirmation and asserts the prompt is held until the re-pick's response lands (verified red pre-fix).
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… pick confirmSeededSelection optimistically seeds the display to the picked model, then delegates the real switch to descriptor.applySelection. Claude's applySelection guards its setSessionModel on the session's current model — which the optimistic seed already set to the pick — so the guard saw "already there" and skipped the switch, leaving the session running Claude's default model. The seed was dropped before applySelection only for config-option backends, on a false assumption that setModel-style backends always round-trip. Claude is setModel-style but guards on current, so it broke. Drop the optimistic seed unconditionally so applySelection sees the backend's true reported model and issues the switch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… model opencode (an ACP subprocess) broadcasts config_option_update notifications that the ACP layer synthesizes into state_changed events. One can race a model switch and carry the pre-switch default; because handleSessionEvent applied state_changed unconditionally, the late push clobbered the user's just-applied model back to the default — intermittently. These backends never self-switch the model, so a state_changed that regresses the model away from the last user-applied selection is a stale echo. Track the last applied model and, when an incoming state reverts it (and the backend still offers it), keep it; every other dimension (effort, mode, availableModels) stays authoritative. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d-less backend Adds regression coverage for the audited transitions: a cross-backend Claude seed lands both the picked model and its effort, and a guard-less setModel backend (codex-style) still issues the switch after the seed-drop change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ith provenance Every currentState write now flows through one choke point that encodes the per-dimension ownership policy: confirmed model-apply responses re-pin the user's model, reported snapshots (setMode response, state_changed push) are reconciled against the pin, seeds are trusted verbatim. This closes the setMode bypass, where a mode-switch response computed against pre-switch state could clobber the applied model unguarded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…State generalization Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…firmed opencode routes mode switches through setSessionConfigOption; its response is a whole-state snapshot, not a user model confirmation. Landing it as "confirmed" let a stale snapshot clobber the just-applied model AND re-pin lastAppliedModelBaseId to the stale value, making the reconcile guard rewrite later truthful pushes to the wrong model. Split the entry point by caller intent: model/effort applies stay "confirmed"; mode applies (manager applyMode, replayPersistedMode) go through setModeConfigOption as "reported". Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…it provenance Mode call sites now call applyConfigOption(configId, value, "reported") directly — the provenance at the call site says more than a wrapper name did. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every config-option apply now states its provenance at the call site; the implicit-"confirmed" entry point is gone, so a caller can't land a mode snapshot as a model confirmation by picking the wrong-looking method. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…omment Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…n, provenance kept Keeps the setModel/setMode/setConfigOption family name; the explicit provenance parameter stays, so every caller still states its intent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… stale revert
Rejecting only the stale model grafted the stale effort onto the applied
base (sonnet + gpt-5's "high") — a selection the backend never confirmed.
The pin now carries {baseModelId, effort} from the confirmed response and a
rejected revert restores both. A push that agrees on the model is still
trusted verbatim, so legitimate effort changes keep landing.
Addresses Codex review comment on PR 2669 (stale-effort graft).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…write provenance, seed visibility - reconcileAppliedModel refreshes the pin's effort when trusting a same-model snapshot, so a later stale revert restores the latest trusted selection instead of an obsolete effort. - Effort writes pass "reported" instead of "confirmed": their response snapshots are not model confirmations, so a stale-modeled one can no longer clobber the applied model or corrupt the pin. - confirmSeededSelection no longer resets the optimistic seed; the reported state is threaded to descriptor.applySelection so guards compare truth while the picker keeps showing the pick during the confirmation round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ive-model metadata through a rejected revert - reconcileAppliedModel clears lastAppliedModel when accepting a snapshot because the applied model vanished from the catalog: the backend abandoned it, so a later catalog update re-offering the model must not graft it back over the fallback actually in use. - When rejecting a stale revert, the pinned entry's effortOptions and the apply spec's effortConfigId are carried over from the previous state — the translator derives both from a snapshot's own active model, so taking them from the stale snapshot would hide the pinned model's effort selector and dispatch effort at the wrong option. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…artup turns behind the seeded confirmation - A monotonic modelApplyGeneration marks the newest in-flight user model apply; a response that resolves after a newer apply began is demoted from "confirmed" to "reported", so it reconciles against the newest pick instead of re-pinning the older model. - runTurn now awaits ready for turns fired during the startup window: the status gate opens before confirmSeededSelection finishes, so a fast first submit could race the switch and prompt on the backend default. A startupConfirmed flag keeps every later turn on the synchronous dispatch path callers rely on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rns queued behind startup - Replace the apply-generation demotion with actual serialization: rapid picks previously dispatched concurrent requests whose responses could complete out of order, leaving the ACP wire cache — overwritten by each completion — and potentially the backend itself on the older pick while the display showed the newer one. Dispatch is now sequenced on a per-session chain (failures don't wedge it), so the last pick is the last write everywhere. - A turn queued behind the seeded-model confirmation now re-checks cancellation/disposal after the wait and completes as "cancelled" instead of dispatching a prompt nobody is watching. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd abort locally before the cancel round-trip - The startup gate now drains modelApplyChain after ready: the session reports idle during the seeded confirmation, so a user re-pick queues behind the seeded apply and ready alone released the first prompt onto the seed instead of the latest pick. - cancel() aborts the local controller before awaiting backend.cancel, so a turn queued behind startup observes the cancellation synchronously instead of slipping through while the round-trip is in flight. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
bd1fac7 to
2e2e974
Compare
1 participant
Fixes logancyang/obsidian-copilot-preview#227
Problem
Switching models at the start of an agent session — e.g. OpenCode → Claude Sonnet — didn't take on the first click: it ran the backend default model instead, and a second click was needed to actually reach the pick. The model itself was wrong, not just the picker label.
Investigation (reproduced against the real
AgentSessionbefore fixing) found two independent mechanisms, both leaving the session on the default:confirmSeededSelectionoptimistically seeds the display to the pick, then delegates the real switch todescriptor.applySelection. Claude'sapplySelectionguardssetSessionModelon the session's current model — which the optimistic seed already set to the pick — so it saw "already there" and skipped the switch. The seed was dropped beforeapplySelectiononly for config-option backends, on a false assumption that setModel-style backends always round-trip.config_option_updatenotifications that the ACP layer synthesizes intostate_changed; one can race the switch and carry the pre-switch default.handleSessionEventappliedstate_changedunconditionally, so the late push clobbered the just-applied model back to the default.Fix
descriptor.applySelectionas an explicitreportedStateargument; the Claude/opencode "skip the redundant model write" guards compare against it instead of the seeded session state, so the guard fires the real switch. The optimistic seed stays visible during the confirmation round-trip — an earlier revision dropped the seed instead, butinitialize()has already notified by then, so React's next render would flash the backend default until the switch confirmed (Codex review catch).effortOptionsentry andapply.effortConfigId, which the translator derives from a snapshot's own active model) so the effort selector and dispatch target keep describing the model shown.currentStatewrite through a singleapplyState(next, provenance)choke point encoding the ownership policy —"confirmed"model-apply responses re-pin the user's model,"reported"snapshots (state_changedpush,setModeresponse) are reconciled against the pin,"seed"writes are trusted as transient display state. This closes the previously-unguardedsetModeresponse path by construction; the alternative (leaving remember/reconcile calls scattered at the nine write sites) was rejected in review because each new write site would re-open the bypass class.runTurnqueues a turn fired during the startup window behindreadyso the first prompt can't race the seeded switch onto the backend default (later turns keep their synchronous dispatch path); a turn cancelled or disposed while queued finalizes as"cancelled"without dispatching the prompt; the gate also drains the apply chain (a re-pick made during the window must win over the seed) andcancel()aborts locally before its backend round-trip so a queued turn can't slip through mid-cancellation.setSessionConfigOption, and Claude/opencode route effort through it too, so those responses previously landed as model confirmations — a stale response could clobber the applied model and re-pin it to the stale value, making the guard rewrite later truthful pushes.setConfigOption(configId, value, provenance)requires the caller's intent, and"confirmed"is reserved for genuine model applies (applyModelWireId's config-option channel); effort applies and mode applies (applyMode,replayPersistedMode, effort descriptors) pass"reported". A provenance-less entry point was rejected because it let a caller land a snapshot as a model confirmation without saying so.Scope
Budget gate: PASS — 9 files, +1199/−69; two root-cause fixes plus one provenance-typed write path, a single config-option entry point, and an explicit reported-state argument on
applySelection; the files beyond the core two are exactly the call sites made intent-explicit, and every regression test pins a distinct mechanism.Changes
src/agentMode/session/AgentSession.ts—StateProvenancetype,applyStatesole write path,rememberAppliedModelpin, andreconcileAppliedModelguard (which also refreshes the pin's effort on trusted same-model snapshots); allcurrentStatewrites routed throughapplyState;setConfigOptionrequires the caller's provenance;confirmSeededSelectionkeeps the seed visible and passes the reported state toapplySelection.src/agentMode/session/descriptor.ts—applySelectionaccepts an optionalreportedStatethe guards must compare against.src/agentMode/session/AgentSessionManager.ts—applyMode's config-option path passes"reported".src/agentMode/session/replayPersistedMode.ts— persisted-mode replay passes"reported".src/agentMode/backends/claude/descriptor.ts,src/agentMode/backends/opencode/descriptor.ts— guards compare againstreportedState; effort applies pass"reported".src/agentMode/session/AgentSession.test.ts— nineteen regression tests: Claude guard-vs-seed repro, Claude model+effort landing, guard-less (codex) switch, stalestate_changedclobber repro, stale revert restores the applied effort too (no stale-effort graft), same-modelstate_changedeffort change honored, stalesetModeresponse model preserved, stale config-option mode-apply response model preserved with pin left uncorrupted, seed stays visible while the confirming switch is in flight, trusted same-model effort survives a later stale revert, stale-modeled effort response can't clobber the applied model or corrupt the pin, pin cleared when the backend drops the applied model, pinned model's effort metadata carried through a rejected stale revert, rapid model applies serialized (last pick is last write, failures don't wedge the chain), first prompt queued behind the seeded confirmation, queued turn cancelled mid-wait never dispatches its prompt, queued prompt held for a startup-window re-pick, slow cancel round-trip still stops a queued prompt.src/agentMode/session/replayPersistedMode.test.ts,src/agentMode/backends/opencode/descriptor.test.ts— expectations and mocks updated to the provenance-carrying call.Verification
state_changedstill being honored, stalesetMode/ config-option mode-apply / effort-apply responses no longer clobbering the pick or corrupting it via re-pin, and the optimistic seed staying visible during the confirmation round-trip.npm run lint,npm run format,npm run build(tsc + esbuild) all pass.Planned via otacon (session
otc_izxy2k); reviewed and hardened via otacon review (issue filed as logancyang/obsidian-copilot-preview#227); six Codex review rounds addressed (seed visibility, effort provenance, pin freshness/lifetime, reconcile metadata coherence, apply-generation and startup-prompt races).🤖 Generated with Claude Code