Repository navigation
fix(core): apply prompt agent selection at the delivery boundary - #59290
houman-moallemi wants to merge 4 commits into
Conversation
github-actions
Bot
added
the
needs:compliance
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core part looks right: a prompt that switches agent is queued, and agent.selected is published when it is delivered, so the running turn keeps its agent. I ran the changed core, app and TUI transport tests (all pass) but didn't reproduce the bug end to end in the TUI. Type checks couldn't run here because bun check is missing, on the base too.
Public API: POST /api/session/:sessionID/prompt (session.prompt in both SDKs) gets a new optional agent field, and SessionInbox.UserPayload (the stored inbox payload) gets an optional agent. Both are additive and backwards compatible. Callers that don't send agent behave as before, and switchAgent is unchanged.
Concerns, most important first (details inline):
- App drops the model choice. In
packages/app/src/composer/submit.ts, when the agent differs the wholeapplySelectionis skipped, includingswitchModel. That happens in idle sessions too. The updated test now expects["admit", "send"]with no model switch even though the model changes, so the test encodes the regression. - The queue can be bypassed. Promotion publishes
agent.selectedfor any delivered user item, steered ones included. If a user moves the queued prompt to steer (steerInbox, the existing move-to-steer UI), the agent switch lands mid-turn again. Either keep agent-changing items in the queue or apply the switch only on queue promotion. - Sessions with no stored agent. When
session.agentis undefined (for example sessions created through the SDK and prompted without an agent), every steer that carries the default agent is forced into the queue. - Other TUI paths still switch straight away. Slash commands and blank Enter in
packages/tui/src/component/prompt/index.tsxstill callcommitSelection()→switchAgent, so a command sent while busy still reconfigures the running turn. - Overlap with #54779. It adds
agent(and model) to prompt admission too, with different steer behaviour. The two will conflict, so one approach should be settled on.
| } | ||
|
|
||
| const commit = () => (value.delivery === "steer" ? applySelection(session, value.selection, track, settle) : settle()) | ||
| const agentChanged = session.current()?.agent !== value.selection.agent |
There was a problem hiding this comment.
When the agent differs, this skips applySelection entirely, so the model and variant the user picked are never sent (switchModel doesn't run). That happens for idle sessions too, not only busy ones. The updated test (calls: ["admit", "send"] while the model goes from old to the selection's model) now expects this. Could the model switch still run here and only the agent part move onto the prompt?
There was a problem hiding this comment.
Fixed in 663ae8d: applySelection now takes an applyAgent flag, so a steered prompt that changes the agent still runs the revert settle and the model switch — only the agent part rides the prompt payload. Queue delivery keeps the settle-only behavior it had before.
Updated the tests accordingly: the agent-differs cases now expect switch-model again. The retry case surfaced one more detail — the auto-retry reuses the first commit (committed ??= commit()), so a retried admission makes no second model request.
| return Effect.gen(function* () { | ||
| // Promotion is a turn boundary, the only safe point to apply a prompt's agent | ||
| // selection: the projector persists it before the message becomes visible. | ||
| if (row.type === "user") { |
There was a problem hiding this comment.
This runs for every promoted user row, including steers. Forcing delivery: "queue" at admission isn't enough on its own: Session.steerInbox (the existing move-to-steer action in the app and TUI) can turn the queued item into a steer. It then promotes at the next step boundary of the running turn and publishes agent.selected mid-turn, which is the bug this fixes. Consider applying the agent only when a queued item is promoted, or refusing or ignoring the agent change for steered items.
There was a problem hiding this comment.
Fixed in 663ae8d, in two layers:
SessionInbox.steernow refuses user rows whose payload agent differs from the stored session agent — the item stays queued instead of becoming a steer.- Promotion applies the payload agent only for rows with
delivery === "queue", so a steered row can never flip the agent of a running turn (this also closes the flip-back case where an older steer delivers in the same promote window after a queued row already flipped the agent).
Added a test: queueing an agent-changing prompt and calling steerInbox leaves it queued with no InboxDeliveryChanged event.
| // change reconfigures permissions and tools, which is only safe at a turn boundary. | ||
| // Queueing it defers the selection to the idle boundary where promotion applies it. | ||
| const request = | ||
| input.agent !== undefined && input.agent !== session.agent ? { ...input, delivery: "queue" as const } : input |
There was a problem hiding this comment.
When session.agent is undefined (a session created and prompted without an agent, for example through the SDK), any prompt that carries an agent, including the default one the TUI and app always send, counts as an agent change and is forced into the queue, so steering stops working for that session. Should this compare against the resolved default agent rather than the raw stored value?
There was a problem hiding this comment.
Fixed in 663ae8d: prompts into sessions without a stored agent now compare against the configured default (Agent.Service.select(), resolved inside the session's location services so config reloads are honored) rather than the raw stored value. Carrying the default agent still steers; a real change still queues.
Filed a follow-up for the remaining eager paths (slash commands, blank Enter): to be linked once opened.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
663ae8d to
3ddd54a
Compare
There was a problem hiding this comment.
The new commit fixes all three earlier points: steered prompts switch the model again, an agent-changing prompt can't be moved to steer, and sessions without a stored agent compare against the default. I ran the reproduction script from #59289 in the TUI with a simulated model. On the base, the agent switches inside the running Plan turn. With this PR, the Plan turn finishes as Plan and the switch starts the next turn as Build. Before/after screenshots below.
Public API is unchanged since my last review: only the optional agent field on session.prompt and SessionInbox.UserPayload, which is backwards compatible.
New problem, seen in the same run: after the switch, the Build turn is followed by an extra model request whose input is only two copies of the "You are NO LONGER in Plan mode" reminder (step 5 in the after screenshot). Base sends the reminder once and makes no extra request. Details inline.
Also inline: moving an agent-changing prompt to steer now succeeds without moving anything.
| // selection: the projector persists it before the message becomes visible. Only | ||
| // queued rows apply it — a steered row delivers into the running turn, whose | ||
| // agent must not change mid-flight. | ||
| if (row.type === "user" && row.delivery === "queue") { |
There was a problem hiding this comment.
With agent.selected now published during promotion, right before the runner's next step, the plan plugin adds its leave reminder twice. Its session.agent.selected subscriber sends one through session.synthetic, but asynchronously. Meanwhile its context hook runs for the new step, doesn't see that reminder yet, and adds a second one. Both arrive as steers after the step, so the runner makes another model request with only the two reminders. In the #59289 reproduction: base sends the reminder once (requests 3, counts [0,1,1]); this PR sends 5 requests with counts [0,0,0,1,2], and the last one is that extra step. Since every agent switch in the TUI and app now goes through this path, I'd expect it on every switch, not only while busy (I only ran the busy case). The switch and its reminder should land together with the prompt, for example by publishing the selection before the runner prepares the turn, or by letting the plugin see the pending switch.
There was a problem hiding this comment.
Fixed in 4a29b3a: promotion's AgentSelected now carries source: "delivery", and the plan plugin's event subscriber skips those — the switch lands at a turn boundary where the context hook owns the reminder anyway, so it injects synchronously into the new turn's request (one copy, no extra steer, no follow-up request). Explicit switches keep the subscriber-driven reminder as before.
Regression test in test/plugin/plan.test.ts: a delivery-time switch persists nothing from the subscriber, and the context hook then injects the leave reminder exactly once.
| .pipe(Effect.orDie) | ||
| // An agent-changing prompt must stay queued: promoting it as a steer would | ||
| // apply the selection mid-turn and reconfigure the running turn. | ||
| if (payload.agent !== undefined && payload.agent !== (session?.agent ?? undefined)) return |
There was a problem hiding this comment.
This returns success without moving anything. Session.steerInbox then wakes the runner and the HTTP call succeeds, but the item stays queued and no InboxDeliveryChanged event is published, so a client that moves it optimistically gets no error to undo. A typed failure, like the one for an already-steered item, would let the app and TUI show why. This check also compares against session?.agent rather than the default agent that Session.prompt now uses, so for sessions with no stored agent a queued prompt carrying the default agent can't be moved to steer.
There was a problem hiding this comment.
Fixed in 4a29b3a: the refusal moved into Session.steerInbox, which now fails with the same InboxConflictError an already-steered item gets, so app and TUI can show why and undo an optimistic move. The comparison also uses the same effective agent Session.prompt uses (the configured default, resolved through the session's location services), so a session without a stored agent can steer prompts carrying the default agent again.
There was a problem hiding this comment.
The new push only rebases the branch onto the current dev. Both commits are unchanged, so nothing new needs review, and the two open threads (the duplicate Plan reminder and the silent steer refusal) still apply. I re-ran the #59289 reproduction on the new head. The running Plan turn still keeps its agent. The extra model request with two copies of the "NO LONGER in Plan mode" reminder still appears after the switch.
The public API hasn't changed since my last review: an optional agent field on session.prompt and SessionInbox.UserPayload, which is backwards compatible.
4a29b3a to
e090b0f
Compare
There was a problem hiding this comment.
The new commits fix the steer refusal (thread 5): steerInbox now fails with InboxConflictError and compares against the default agent. They also fix the doubled reminder from thread 4, but not the extra model request. I re-ran the #59289 reproduction in the TUI: the Plan turn still keeps its agent, and each request now carries the leave reminder at most once (counts [0,0,0,1,1], previously [0,0,0,1,2]). After the Build turn answers, though, one more request still runs whose only new input is the reminder ("Step 5 done." in the screenshot). Details inline.
Public API: the session.agent.selected event gets a new optional source: "delivery" field. Together with the optional agent on session.prompt and SessionInbox.UserPayload, all of this is additive and backwards compatible.
The push also rebases onto the latest dev, which pulls in two unrelated upstream commits (V2_HTTP_API_AUDIT.md deleted, install edited). Those come from dev, not from this PR.
| } | ||
| // A delivery-time switch lands at a turn boundary: the context hook below owns | ||
| // its reminder synchronously, and a synthetic here would race it and double. | ||
| if (event.data.source === "delivery") return undefined |
There was a problem hiding this comment.
This fixes the doubled reminder, but the extra model request is still there. The context hook adds the reminder to the request in memory, then saves it with session.synthetic(..., resume: false) while the step is running. That saved message is still pending when the turn ends, so the runner starts another step for it. In the #59289 reproduction the 5th request ends with Next: implement it → Step 4 done. → the leave reminder, and the model answers again. #58287 reports the same hook behaviour for default_agent: plan, and #58395 is trying to fix it there. Because every TUI and app agent switch now goes through this path, each switch costs one unrequested request. Saving the reminder before the delivered prompt (as part of promotion) instead of from the context hook would avoid the extra turn.
There was a problem hiding this comment.
Fixed in c9fb3c0: the context hook no longer persists the reconciled reminder — it only splices it into the current request. The hook re-derives the reminder on every request by design (that is how it already repairs compaction/revert stripping), so the model still sees it exactly once per request. Nothing is left pending when the turn ends, so the runner goes idle instead of starting the reminder-only step. Explicit-switch reminders from the event subscriber are unchanged, and tests now assert in-memory reconciliation without persistence.
Submitting a prompt while a turn runs eagerly called session.switchAgent, which reconfigured the running turn mid-flight: the composer's new agent replaced the running agent's permissions, tools, and system prompt, and the plan plugin told the model its restrictions were lifted. Prompts now carry their agent selection through admission. A selection that differs from the session's agent is queued at admission, applied by the promotion projector at the turn boundary, and the runner re-takes its context selection when the persisted agent changed. Same-selection prompts keep steering. The TUI and app composers pass the selection on the prompt instead of switching eagerly.
- applySelection gains an applyAgent mode: steered prompts that change the agent still switch the model; only the agent defers to the prompt - admission.steer refuses agent-differing user rows so move-to-steer cannot promote an agent change mid-turn - promotion applies the payload agent only for queued rows, so steered rows can never flip the agent of a running turn - sessions without a stored agent compare against the configured default agent, so SDK sessions still steer prompts carrying it - test fixtures provide the Agent service where prompt admission resolves it
- promotion's AgentSelected carries source: "delivery"; the plan plugin's event subscriber skips it — the context hook owns the mode reminder at a turn boundary, so the switch no longer steers an extra request holding two reminder copies - steerInbox fails with InboxConflictError instead of silently refusing: clients can undo an optimistic move, and the refusal compares the same effective default (configured default agent for sessions without a stored one) that prompt admission uses
The context hook persisted each reconciled reminder as a synthetic. A reminder saved while the turn runs is still pending when the turn ends, so the runner starts one more step whose only input is the reminder. The hook re-derives the reminder on every request anyway, so it now only splices it into the current request; the session stays idle afterwards. Switch-persisted reminders from explicit switches are unchanged.
e090b0f to
c9fb3c0
Compare
Issue for this PR
Closes #59289
Type of change
What does this PR do?
Submitting a prompt while a turn is running called
session.switchAgenteagerly. That publishesagent.selectedimmediately, the projector persists it, and the runner re-reads the agent at every step boundary — so the running turn got reconfigured mid-flight: new permissions, new tool snapshot, and the plan plugin told the model it was no longer restricted. Reproduced on 2.1.4 (V2,dev): a plan turn kept executing after the composer was flipped to build and the next prompt submitted, and it gained write access.The fix carries the agent selection on the admitted prompt instead of applying it out of band. Admission queues a prompt whose agent differs from the session's, so an agent change can only ever land at a turn boundary. Promotion publishes
agent.selected(withprevious) right before the message becomes visible, and the runner re-takes its context selection when the persisted agent actually changed. The TUI and app composers pass the selection on the prompt rather than callingswitchAgentfirst. Model switching behavior is untouched.How did you verify your code works?
agent.selected+agent-switchedmessage); a selection matching the session agent still steers. Both fail on the pre-fix core (verified by stashing the source changes).Visual
Optional, for user-facing changes: a small looping animated SVG (240×72) of the change, shown in the desktop app's "What's new in beta". Replace the block below or delete this section.
Checklist
Added by the opencode bot (AI-generated).
Related: #48356, #58287, #59219, #57013
Public API: adds optional
agentto session.prompt (HTTP, SDKs) and SessionInbox.UserPayload, and optionalsource: "delivery"to the session.agent.selected event (backwards compatible)