Skip to content

fix(codex): queue a differing model/effort pick as a fresh turn instead of steering - #889

Open
matt2e wants to merge 1 commit into
mainfrom
on-next-turn
Open

matt2e wants to merge 1 commit into
mainfrom
on-next-turn

Conversation

@matt2e

@matt2e matt2e commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Bug

A model or effort picked for a Codex agent that is already mid-turn was silently dropped. When a mention arrives for a conversation lane with an active Codex turn, src/bundled/codex/runtime.ts sends it as turn/steer, and that branch never read the delivery's model / effort. Because an unpicked later turn deliberately returns to the agent's defaults (picks are per-turn by design), the pick never applied anywhere — not to the running turn and not to any later one. The window also covers a turn's startup, since lane.active is set before thread/start / turn/start.

Claude Code is unaffected: its runtime applies picks mid-turn via set_model / effort control requests, so the remainder of a running turn switches immediately. No change is made there.

Why queue a fresh turn (and not the alternatives)

  1. Forwarding the pick on the steer path is impossible. The Codex app-server's turn/steer request carries only input and context: upstream TurnSteerParams has no model or effort fields; only TurnStartParams does. So the only ways to honour a pick on a busy agent are to queue a fresh turn or to interrupt.
  2. Copy-only (clarifying the picker text) would leave the pick silently dropped. Not a fix.
  3. Disabling the picker while the agent is busy would need a new "agent is working" status surface across service, runtimes, and UI (~60–120 LOC in five files), or would couple the composer to the STOPGAP 3-second typing pulse from feat(agents2): show agents working: 👀💬 reactions, local typing, Codex settles #860 (flicker, startup gap). It also removes a legitimate action that Claude Code doesn't need removed.
  4. Interrupting and restarting with the new settings destroys in-progress work, publishes "interrupted", and leaves <turn_aborted> noise in the thread.
  5. Making picks sticky per agent/conversation reverses the deliberate, documented per-turn design (service.ts contract, runtime comment, composer clearing picks after each send).

Queueing is ~10 LOC. It reuses the lane's existing tail queue and the existing "turn already finished → start instead of steer" fallback in the steer branch, and it makes the picker's existing "next run" copy true as written. Matching or absent picks still steer exactly as before.

Change

  • src/bundled/codex/runtime.ts — Active records the resolved model / effort (set in execute before thread/start, so they are available once active.ready resolves). In the steer branch, the active.finished fallback is widened: if the request carries a model or effort that differs from the running turn's, route to this.start(...) so the message runs as the next turn with the picked settings.
  • src/bundled/codex/runtime.test.ts — a differing pick test (model+effort pick, then effort-only pick, both arriving mid-turn) asserts zero turn/steer calls and three sequential turn/starts on the same thread (low → deep-model/xhigh → test-model/high) with replies in order. A matching pick / no pick test confirms both still steer with a single turn/start.
  • src/bundled/codex/README.md — one sentence in the steering paragraph describing the new behaviour.

Verification

  • vitest run src/bundled/codex/runtime.test.ts: 51/51 pass.
  • Fail-then-pass confirmed: with the runtime.ts change reverted, the differing-pick test fails (pick is steered and dropped); restored, it passes.
  • tsc --noEmit, biome check, git diff --check clean; pre-commit hooks passed.
  • Not exercised against a live Codex app-server; behaviour is covered by the protocol fixture only.

Known tradeoffs

  • start() sets the lane status to "Starting" while the old turn is still finishing — cosmetic.
  • A plain follow-up sent after a queued pick still steers the active turn, so it lands before the queued one.
  • A pick for a model not in the agent's list differs from the resolved active.model and therefore queues, even though execute falls back to the agent's own model. Harmless; the message simply runs as a fresh turn.

Origin

Addresses review comment #856 (comment) on #856, which identified this behaviour (correctly, for Codex only).

Reply posted on the original thread: #856 (comment)

🤖 Generated with Claude Code

…ad of steering

A mention that arrived while the lane already had an active Codex turn was
always sent as `turn/steer`, which never read the delivery's model or
effort, so a pick made for a busy agent was silently dropped. The Codex
app-server's `turn/steer` request carries only input and context; model
and effort exist solely on `turn/start`, so the pick cannot be forwarded
on the steer path.

Record the resolved model and effort on the active turn, and when a
mid-turn pick differs from either, queue the message on the lane's
existing tail (as the "turn already finished" fallback does) so it runs
as the next turn with the picked settings. Matching or absent picks
still steer. Claude Code applies picks mid-turn already and is unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
@matt2e
matt2e requested a review from a team as a code owner October 12, 2026 08:47

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No changes requested; I found no actionable defect in this change.

Star Lord’s automated source review via Wes’s account: head 22d2047ce57b9cc9d3977e98b384db360b229ddc, base ecfbb8a5835dd0fb04e48111ef406a975f2c7715.

The change reuses the existing lane queue and per-turn resolution without making picks sticky; matching or absent picks retain steering. Source-only: I did not run tests or the app, and the author-reported protocol-fixture checks do not establish live Codex/native acceptance.

(request.model && request.model !== active.model) ||
(request.effort && request.effort !== active.effort)
) {
this.start(request, text, key, lane, entry);

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.

🤖 [P2] Keep later follow-ups behind a queued model/effort pick

While turn 1 is active, send A with a differing model/effort, then B with no pick (or a pick matching turn 1). A takes this early return and queues behind lane.tail, but B still steers turn 1 and updates its reply handle. B therefore executes before the earlier A. Fixture probes confirmed a turn/steer targeting turn 1 while A remained queued; the selected settings are preserved for A, but message order is not.

Once a differing pick is queued, route subsequent follow-ups behind that queue boundary rather than continuing to steer the old turn. Add a regression test for differing-pick → plain-follow-up ordering.

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.

3 participants