Repository navigation
fix(acp): keep the selection when a model or agent switch fails - #59790
andreivince wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix works. I ran the new wire tests against the base: the failed model switch and failed mode switch cases (plus the independent-agent and rejected-newer-switch cases) fail there and pass with this change. All 107 ACP tests pass.
Public API: unchanged. Attached gets two new fields, but it's internal to the ACP adapter.
The bug only needs the selection to be written after the switch succeeds. The PR also adds a per-field revision counter and a pending set to handle server events arriving while a switch is in flight. AGENTS.md asks for buy-in before adding complexity for rare races, so it would help to say whether you've seen these orderings in practice. If not, please consider keeping just the write-after-success (plus the echo suppression if the extra config_option_update actually matters). Details inline. The tests also add about 175 lines, mostly race scenarios; see inline.
Related: #48759
| select: Effect.fnUntraced(function* (attached, change) { | ||
| yield* Ref.update(attached.selection, (selection) => ({ ...selection, ...change })) | ||
| const key = "model" in change ? "model" : "modeID" | ||
| const revision = attached.revision[key] |
There was a problem hiding this comment.
Moving the Ref.update after the switch call is enough for #59789. The revision counter and the pending set on Attached guard against races that weren't reported, and AGENTS.md asks for buy-in before adding that kind of complexity. The counter also only catches server events that arrive while the switch is in flight. If two switches both succeed and the older response comes back last, the older value is still written until the server's events correct it. So this adds state without fully settling ordering. Could this be reduced to write-after-success, plus only what is needed to avoid a duplicate config_option_update from the switch's own echo?
| ]) | ||
| }) | ||
|
|
||
| test("keeps a newer server selection received before a switch responds", async () => { |
There was a problem hiding this comment.
The test.each above, covering the failed model and mode switches, is the regression test this fix needs. This test and the overlapping-switch tests below (lines 144 and 180) add about 120 lines of hand-resolved promises and until polling to check specific request and event orderings. They're hard to follow and tie the suite to timing details. Please drop them, or cut them down to at most one small in-flight case if you keep the revision logic.
There was a problem hiding this comment.
The new commits address both earlier points. The revision counter is gone, so select now just writes the selection after the switch succeeds, and the pending set only stops the switch's own server event from sending an extra config_option_update. The tests are cut down to the failed-switch test.each.
I checked it with the issue's reproduction. On the base, a failed model switch leads to effort not found: high, and a failed agent switch leaves the mode reported as plan. With this PR, both keep the original selection. The ACP tests (102) pass, and there are no type errors in the ACP files.
Public API: unchanged. No further concerns.
Issue for this PR
Closes #59789
Type of change
What does this PR do?
A failed model or agent switch used to leave the rejected value in the local selection, so later requests could use a choice the server had never accepted.
The requested value is now saved after the switch succeeds. A pending set keeps the server event for that switch from sending an extra
config_option_update. The revision counters and ordering tests have been removed.How did you verify your code works?
The retained model and agent failure cases in
packages/cli/test/acp/catalog.test.tsfail on dev8d083cb64dand pass here. All 102 ACP tests andbun run checkpass locally. I also rechecked notification suppression with and without the pending set.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #48759