Conversation
A steer waited for the agent's application verdict with no bound tied to the turn: request-transport runtimes only settle that verdict when the agent answers the held extension request or the connection closes, so stopping the target turn left steerSessionLocked awaiting forever - the guide stayed in pending_apply (which ordinary dispatch deliberately skips) and the per-session steer mutation queue stayed blocked. A prompt-transport stop instead rejected the verdict into the generic error disposition, which also left the entry untouched. steerSessionLocked now races the verdict against the target turn's prompt-run settlement. When the turn settles first, the guide gets a terminal history status (canceled on stop, failed when the turn ended without an answer) and the response is stale-turn; the catch path applies the same settlement for rejected verdicts. Confirmed refusals and pre-submission rejections keep the existing requeue behavior, and unknown delivery results are deliberately terminal rather than re-queued - the provider may still take the held request, and replaying could duplicate it. The application waiter stays registered until the agent answers or the connection closes, so a late verdict cannot resurrect the stopped turn. Fixes LodyAI#666
![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: 855eced8e4
ℹ️ 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".
| const application = await Promise.race([ | ||
| steerRun.applied, | ||
| ownedPromptRun.promptOutcome.then( | ||
| () => 'prompt-run-settled' as const, | ||
| () => 'prompt-run-settled' as const | ||
| ), |
There was a problem hiding this comment.
When a request-transport steer’s target prompt settles before its _session/steering request answers, this race selects prompt-run-settled even if the later answer is AgentSteerNotDeliveredError. AgentClient.steerPrompt explicitly allows this ordering because an upstream turn response can arrive before the steer request’s refusal; terminalizing the guide here loses a provably unaccepted message instead of requeueing it for ordinary dispatch.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L60-L61
Useful? React with 👍 / 👎.
| return reject( | ||
| 'stale-turn', | ||
| runtime.cancelRequested | ||
| ? 'The target turn was stopped before the agent answered the steer' | ||
| : 'The target turn ended before the agent answered the steer' | ||
| ); |
There was a problem hiding this comment.
When an application notification arrives after this branch returns, AgentClient.handleSteerApplied resolves steerRun.applied with a lease, installs waiter.released as steerApplicationBarrier, and waits for lease.release(). Because the losing race result is no longer consumed, nothing releases that lease, so the barrier remains pending and every subsequent sessionUpdate for this session blocks indefinitely; the late lease must be consumed and released without transferring ownership.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L59
Useful? React with 👍 / 👎.
The settlement race answers the steer RPC and terminalizes the guide without waiting for the held request's verdict, but it also dropped that verdict entirely. Two consequences flagged in review: - An agent-issued refusal (`AgentSteerNotDeliveredError` is only produced by the agent's own invalid-request answer) arrived after the terminal write and was lost, even though a provably undelivered guide belongs back in ordinary dispatch. - A late acceptance resolved `steerRun.applied` with a lease nobody consumed: `handleSteerApplied` had already installed `waiter.released` as the session's application barrier and blocks every later `sessionUpdate` until that lease is released, so the session's update stream wedged. The race branch now attaches a non-blocking verdict consumer after the terminal write: a late acceptance is released immediately, and a late refusal requeues the guide by flipping back only the terminal status that same branch wrote, behind the ordinary requeue guards (not active, not already handled). Ambiguous rejections such as a closed connection keep the terminal status, and a taken steer is never replayed.
|
Both findings confirmed as real — thank you for the precise analysis. Fixed in 3aa4fbc. 1. Preserve provable refusals after the settlement race The race branch now attaches a non-blocking consumer to the pending verdict right after the terminal write (the response itself still never waits on the held request, so the original stranding stays fixed). When the verdict later rejects with 2. Release the lease from a late steer acknowledgement The same consumer releases a late acceptance's lease immediately: Regression tests (new, in
Full |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3aa4fbc3d3
ℹ️ 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 (!queueable) { | ||
| return; |
There was a problem hiding this comment.
When the steer’s history entry has not yet reached this daemon as the late AgentSteerNotDeliveredError arrives—the supported RPC-before-history ordering—queueable remains false and this return leaves the terminal-without-entry record intact without publishing latestUserMsgId. Although the refusal proves that the agent never accepted the message, the watcher subsequently treats the late entry as terminal instead of dispatching it, permanently stranding the guide; requeue via its pointer even while the entry is absent, as the ordinary refusal path does.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L61
Useful? React with 👍 / 👎.
| await this.upsertSessionMeta(sessionId, { | ||
| latestUserMsgId: userTurnId, | ||
| lastMissingHistoryUserMsgId: undefined, | ||
| }); |
There was a problem hiding this comment.
If another user turn is published while the held steer request remains unanswered, this late callback unconditionally replaces that newer turn’s latestUserMsgId with the older refused steer. After the chronological history scan dispatches the older steer, lastHandledUserMsgId can equal this overwritten pointer, making hasPendingUserTurnActivation false and allowing the watcher to unload while the newer entry remains pending and never runs; the requeue must avoid replacing an activation published after this steer.
Useful? React with 👍 / 👎.
Two review findings on the late-refusal requeue path: - When the steer's history entry has not synced to this daemon (the supported RPC-before-history ordering), `queueable` stayed false and the early return kept the race branch's terminal-without-entry record without publishing the pointer, so the late-syncing entry was repaired back to terminal and the guide stranded. The refusal proves non-delivery, so the entry-absent case now requeues through the pointer like the ordinary refusal path does and drops the stale record; the late-syncing entry dispatches via the `pending_apply` pointer match in `findNextDispatchableUserTurn`. - Publishing the pointer unconditionally could replace a newer activation published while the steer request was held. After the chronological scan dispatched the requeued steer, `lastHandledUserMsgId` equaled the overwritten pointer, the watcher unloaded, and the newer turn never ran. The pointer write now treats the slot as occupied while `latestUserMsgId` names a live activation (the same suppression slots dispatch uses), and leaves the scan that activation keeps running reach the requeued steer.
|
Both findings verified and fixed in 78f90ed: 1. Requeue before history synchronization — correct. When the steer's history entry has not synced to this daemon, 2. Preserve newer activations — correct, and it is the same rule Tests: five new cases — entry absent → pointer requeue + stale record cleared; entry absent while a newer activation holds the pointer → no pointer write, record still cleared; entry present under a live newer activation → flip to |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78f90edb61
ℹ️ 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 (pointerHoldsNewerActivation) { | ||
| this.deps.logger.info( | ||
| `[${sessionId}] Steer ${userTurnId} refused after its turn ended; left for the dispatch scan while activation ${pointer} holds the pointer` | ||
| ); | ||
| return; |
There was a problem hiding this comment.
When the refused steer entry is still absent and latestUserMsgId names a newer activation, this branch clears the terminal-without-entry record and returns without giving the refused steer any durable activation. When its pending_apply entry later syncs, findNextDispatchableUserTurn skips it because the pointer still names the newer turn; after that turn is handled, the watcher can unload and the proven-undelivered guide remains stranded. Preserve a durable way to activate this steer after the newer activation instead of relying on the dispatch scan.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L61
Useful? React with 👍 / 👎.
| await this.upsertSessionMeta(sessionId, { | ||
| latestUserMsgId: userTurnId, | ||
| lastMissingHistoryUserMsgId: undefined, | ||
| }); |
There was a problem hiding this comment.
A user can publish a newer turn after the meta snapshot at line 1775 but while getOrCreateSessionDoc or updateHistory is awaited; this write then replaces that newer latestUserMsgId with the older refused steer. After the chronological scan runs the older entry, lastHandledUserMsgId equals the overwritten pointer and the watcher may unload with the newer turn still pending. Fresh evidence in this revision is that the newer-activation guard is computed before two awaited operations, so it does not address publications during the read-to-write window.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L53-L57
Useful? React with 👍 / 👎.
| await this.upsertSessionMeta(sessionId, { | ||
| latestUserMsgId: userTurnId, | ||
| lastMissingHistoryUserMsgId: undefined, | ||
| }); |
There was a problem hiding this comment.
When lastMissingHistoryUserMsgId names an older turn rather than this refused steer, this patch clears that permanent negative acknowledgement unconditionally. If the older payload subsequently syncs with a pending status, findNextDispatchableUserTurn no longer excludes it and can execute a turn whose delivery failure was already surfaced, potentially duplicating a user resend. Only clear the tombstone when it names userTurnId; otherwise carry it through the pointer update.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L33-L37
Useful? React with 👍 / 👎.
…tles Three review findings on the late-refusal requeue: - The pointer guard ran on the meta snapshot taken before the awaited history work, so a newer send publishing its activation while the flip to `pending` was awaited still got clobbered. The decision now runs on a meta re-read after the history work — the session contract forbids read-await-rewrite of producer-owned slots. - The requeue dropped the race branch's terminal-without-entry record before checking the pointer, so an entry that had not synced with a live activation owning the pointer kept nothing durable: the late-syncing entry stranded as undispatchable `pending_apply`. That corner now keeps the record so the late entry is repaired to its terminal status — visible non-delivery, the same terminal settlement the race branch already reported — instead of relying on a dispatch scan that can never see it. - The pointer write cleared `lastMissingHistoryUserMsgId` unconditionally, destroying the permanent one-shot ack for an older turn and re-admitting it once its payload synced. The tombstone is now cleared only when it names the requeued steer itself. The bilingual agent note describes the revised requeue semantics.
github-actions
Bot
added
the
status:needs-pr-attention
|
@yetuge, this pull request needs updates before review. It is marked If the PR remains invalid for 7 days, it will be closed and marked Policy findings |
|
All three findings verified and fixed in 7fc09af: 1. Retain activation for an unsynced refused steer — true. With the pointer owned by a live activation and the entry still absent, dropping the record left nothing durable, and a late-syncing 2. Recheck the dispatch pointer after awaited history work — true, and the guard was worse than merely late: computed before two awaits, it was exactly the read-await-rewrite the session contract forbids. The pointer decision now runs on a meta re-read after 3. Preserve unrelated missing-history tombstones — true. The patch now includes Regression tests (the first three fail on the previous head, all four pass now): absent entry under a live activation keeps the record; an activation published inside the awaited history write is observed by the pointer decision; a tombstone naming an older turn survives the pointer write; a tombstone naming the requeued steer itself is still cleared. The file's suite is 121/121; the full One scope note, consistent with the previous reply: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fc09af51a
ℹ️ 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.setTerminalUserTurnStatus( | ||
| options.sessionId, | ||
| sessionDoc, | ||
| options.userTurnId, | ||
| runtime.cancelRequested ? 'canceled' : 'failed' |
There was a problem hiding this comment.
When the supported RPC-before-history ordering leaves this steer entry absent, setTerminalUserTurnStatus records the cancellation only in the in-memory terminalTurnStatusWithoutEntry map. If the daemon restarts before history synchronizes, that record disappears; the later pending_apply entry still matches latestUserMsgId, so findNextDispatchableUserTurn dispatches it normally, potentially duplicating a steer the provider accepted after the stop. Persist a durable terminal/activation marker before returning.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L58-L59
Useful? React with 👍 / 👎.
| this.deps.logger.info( | ||
| `[${sessionId}] Steer ${userTurnId} refused after its turn ended; activation ${pointer} holds the pointer, keeping the recorded terminal status for the late entry` | ||
| ); | ||
| return; |
There was a problem hiding this comment.
When the steer entry is still unsynced and another turn owns latestUserMsgId, this branch keeps the guide terminal even though AgentSteerNotDeliveredError proves the agent rejected it. The late entry is therefore repaired to canceled and the user's guide is never executed; a durable queued activation is needed rather than replacing the newer pointer. Fresh evidence in this revision is that this new branch explicitly preserves the terminal record and returns for that case.
AGENTS.md reference: apps/cli/src/session/AGENTS.md:L60-L61
Useful? React with 👍 / 👎.
|
Heads-up after pushing 7fc09af: the reconcile-policy check now reports the PR at 1146 changed lines, which per |
|
Both findings verified against 1. Persist terminal settlement before returning stale-turn — confirmed real. The race branch's 2. Requeue the refused steer behind the newer activation — also confirmed, and it is the mirror image of the previous round's finding #1. Dropping the record stranded the guide (previous round); keeping it leaves the guide terminal forever whenever another activation owns the pointer (this round). Both behaviours the single Closing either finding therefore needs a second, durable channel: a queued-activation slot for a refused steer, or a durable terminal marker for the race branch. Either way it is a new field in the shared There is also a practical constraint: the PR is at 1146 lines and If a maintainer confirms the direction, I can prepare the durable queued-activation slot (schema + predicate + watcher lifecycle + regression tests) on top of this branch. Alternatively, if you would rather keep the slot ownership exactly as documented in |
1 participant
Related issue
Closes #666
Problem / pressure
A steer submitted while a turn is running must not outlive its target turn, but
steerSessionLockedwaited for the agent's application verdict with no bound tied to that turn:pending_apply(a status ordinary dispatch deliberately skips), and the per-session steer mutation queue stayed blocked behind the hung await.errordisposition that also left the history entry untouched.Either way the user's message was stranded exactly as described in #666: absent from the provider conversation, absent from ordinary dispatch, with no terminal state.
Summary
steerSessionLockednow racessteerRun.appliedagainst the target turn's prompt-run settlement (the prompt run is the cancellation owner, so its settlement is the structural bound — no arbitrary timeout):setTerminalUserTurnStatus(canceledwhen a stop was requested,failedwhen the turn ended without an answer) and the response isstale-turn. The catch path applies the same settlement when a post-submission stop rejects the verdict, so both transports converge.AgentSteerNotDeliveredError) and pre-submission rejections keep the existing requeue-to-dispatch behavior unchanged; the terminal path only covers the unknown post-submission window.implemented/bug-fix/2026-09-13-steer-settles-with-stopped-turn+.zh.md) records the decision perAGENTS.md.Visual explanation
flowchart TD A["steerSessionLocked<br/>submits steerPrompt"] --> B{"Promise.race"} B -- "verdict resolves" --> C["existing application path<br/>(ownership handoff, unchanged)"] B -- "verdict rejects:<br/>AgentSteerNotDeliveredError" --> D["requeue to ordinary dispatch<br/>(confirmed refusal, unchanged)"] B -- "prompt run settles first<br/>(stop or end)" --> E["terminal settlement:<br/>canceled if stop requested, else failed;<br/>response stale-turn"] B -- "verdict rejects otherwise<br/>+ stop requested" --> E B -- "verdict rejects otherwise,<br/>no stop" --> F["error disposition<br/>(unchanged)"]Simple-change justification: the diff is 347 changed lines but they are one control-flow change in a single function plus its regressions; the Mermaid view above is the complete decision surface.
Before / after
steerSessionnever returns, guide stayspending_apply, steer mutation queue blockedstale-turn, guide settledcanceled, queue unblockederrordisposition, entry untouchedTest plan
npx vitest run tests/session-execution-service.test.ts(apps/cli): 111 passed, including two new regressions that reproduce [Bug] Timed-out steer remains pending_apply after its target turn is stopped #666 — a held verdict plus a stopped turn now resolvesstale-turnand writes thecanceledstatus (previously the test timed out at 30s with the response pending), and a rejected verdict on a stopped turn converges to the same settlement (previouslyerrorwith no history write). Both new tests fail on the parent commit.apps/clisuite with and without the change: identical failure set (88 FAIL lines, name-by-name; 61 failing tests are pre-existing on this Windows machine — real-process login flows, native binary downloads,better-sqlite3-adjacent suites), 0 new / 0 disappeared; the with-change run additionally passes the two new tests.oxlint(repo gate is--quiet): 0 errors on both changed files;prettier --checkclean;pnpm run docs status/docs checkoutput for the two new notes showsstatus: implemented,translation: current,missingTranslation: false.pnpm run typecheckin apps/cli reports the same three pre-existing cross-package module-resolution errors as the parent commit (untouched packages:@lody/shared/platform-kindsubpath,better-sqlite3types).Context handoff
Instructions for reviewing agents
steerSessionLockedinapps/cli/src/session/session-execution-service.ts— thePromise.racebound and the two settlement branches (post-race and catch); confirm the requeue path forAgentSteerNotDeliveredErrorand the pre-submission rejections are untouched.failed; leaving the agent-client waiter registered until answer/connection close.failedstatus for a non-stop settlement is a judgment call, not a documented product decision.Authoring context
canceledwhile the agent may execute — matching the report's requirement that a late result must not resurrect stopped state.pending_applyto a terminal status.agentClient.steerPromptabort wiring.failed-vs-canceleddistinction for non-stop settlements.Original user prompt
Show original prompt
(Verbatim triggering instruction. It references a local scheduled-task prompt file that directs an
automated run: follow up on tracked pull requests, then — when quality gates pass — claim one
unclaimed issue in an agent-core TypeScript repository, fix it with regression tests, and open a
pull request. This PR is that run's output for issue #666.)