Repository navigation
fix(codex): correlate dispatch identity on the echoed client message id - #19331
brennanb2025 wants to merge 5 commits into
Conversation
Codex coalesces a turn/start issued while a turn is running into that turn: same turn id back, no second turn/started. dispatchCodexTurn stamped every accepted send ordinal 0, so a rapid second send adopted the first message's identity and duplicated as a second bubble on any restore. Both correlation legs already flowed — Orca sends clientUserMessageId and the userMessage echo carries it back as clientId — but the echo was discarded. A per-session echo-waiter registry (mirroring the Claude adapter's dispatch waiters) now settles each dispatch on its own echo, with the identity computed by the translator's CodexTurnOrdinals so the settled key matches history replay exactly. Dispatch windows, per live measurement on codex-cli 0.153.4: - coalesced send (response names a turn already in flight): wait the full request timeout — the echo defers until the running turn yields (+6.15s measured), and the outbox honestly shows 'dispatching'. - fresh turn: short ack grace (echo +1.73s measured); on expiry with a sole waiter and no clientId echo ever seen, keep today's positional ordinal-0 fallback for old builds, which is provably correct there. - anything else: unknown, and a late echo upgrades the submission via onDispatchSettledLate -> settleLateDispatch.
|
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds Codex echo waiters that correlate Merge Risk: ⚪ Minimal · up to The correlation, lifecycle settlement, and coalesced-turn handling changes have no unresolved material merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the defect, fix, scope, and tests, but it omits most required template sections. It has no linked issue, visual-proof statement, testing checklist, AI disclosure, review section, boundary checklist, notes, or final checklist. Resolution Complete the repository template. Add an issue reference under Linked Issue, state N/A with a reason under Visual Proof, complete the Testing checkboxes and list tested platforms, provide the required AI Disclosure, Review, Agent skill upstream boundary, Notes, and Checklist sections. Add the ELI5, What Changed, and Why headings or map the existing content to those sections. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This PR replaces dispatchCodexTurn's blanket ordinal: 0 stamp with provider-echo correlation, so a send coalesced into a running Codex turn adopts its true ordinal instead of the first message's identity — the source of the duplicate-prompt bubble on thread restore.
- Echo waiter registry (
codex-structured-dispatch-echo.ts) — a dispatched send registers a waiter keyed onclientMessageId; the liveuserMessageecho settles it with the translator-projected identity, with retired-waiter and null-clientId(old-build) reconciliation. - Coalesced-send classification (
codex-structured-turn-start.ts) —startCodexTurnnow reportsstartedNotificationObserved, and a response naming a turn already inactiveTurnIds(with no ownturn/started) is treated as coalesced, earning the full echo window rather than the ordinal-0 fallback. - Active-turn tracking (
codex-structured-provider-events.ts) —turn/started/turn/completedmaintainsession.activeTurnIds, recorded before cancellation or admission consumes a frame. - Late settlement wiring — expired/dispatched echoes settle through the existing provider-generic
settleStructuredAgentSessionLateDispatch, mirrored from the Claude adapter. - Regression coverage — a real-journal + restore repro (
journal-rapid-send-turn-coalescing.test.ts) that fails before the fix, plus unit tests for the waiter registry and updated adapter/replay/runtime fixtures.
The two-window design, sawClientIdEcho latch, positional-fallback gating, and late-settlement interplay all check out against the shared journal reducer (which ignores repeat accepted/rejected dispatches) and the existing Claude correlation it mirrors.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Review completed on
The same-model review/fix/re-review loop and initial/final readiness audits found no remaining proven production issues. Validation: 125 tests passed across 14 suites, node typecheck passed with an 8 GB heap, and changed-file lint/format checks passed. Five regression checks failed against the original production code and passed with the fixes. Compatibility limit: providers without echoed client IDs leave ambiguous sends unconfirmed; subsequent ID-less sends may stay unconfirmed for that acquired session. Existing corrupted journals are not repaired by this PR. Rendered Electron validation and screenshot evidence remain with the coordinator's Grok QA agent; this comment does not claim that QA passed or that the PR is ready to merge. CI for the latest push must also complete. |
There was a problem hiding this comment.
Important
One existing test now fails: the fresh-turn-evidence fence breaks codex-structured-session-cancel.test.ts. See the inline comment.
Reviewed changes
This delta fences the positional ordinal-0 fallback behind positive fresh-turn evidence rather than the previous !coalesced negation.
- Lazy freshness getter +
dispose()—startCodexTurnnow returns astartedNotificationObservedgetter (re-evaluated after the echo grace, so a lateturn/startedstill counts) and adispose()that clears the notification timer and unregisters the turn-id waiter. - Active-turn membership moves to notifications only —
dispatchCodexTurnno longer addsturnIdtoactiveTurnIds;trackCodexActiveTurnowns it onturn/started/turn/completed, so a completed turn is not resurrected by a lateturn/startresponse. retired.length === 0joins the fallback guard — a send only gets the ordinal-0 fallback when no waiter has ever retired, so a resumed/ambiguous send degrades tounknowninstead of corruption.- ID-less echo reconciliation removed —
resolveCodexUserMessageEchono longer settles a sole candidate or retired waiter on aclientId === nullframe, since such frames can belong to an earlier send.
The registry and dispatchCodexTurn tests still pass locally; the one regression is in the cancel-suite fixture, which was not updated to emit turn/started.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This delta fixes the failing cancel-suite fixture flagged in the prior review, no production code changes.
- Emit
turn/startedin the resend fixture —codex-structured-session-cancel.test.ts'sturn/startroute now fires the fresh-turn notification with the incremented turn id, so the positional fallback fences correctly and the immediate-resend dispatch assertsacceptedwithturn-2(suite is green again). - Asserted the initial dispatch — the first
client-1dispatch is now pinned toaccepted { turnId: 'turn-1' }instead of being left unasserted.
The cancel, close, adapter, dispatch-echo, and rapid-send tests all pass locally.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Electron QA — rendered validation on
|
| before restart | after restart | |
|---|---|---|
Reply with exactly: RAPID_ONE |
1 | 1 |
Reply with exactly: RAPID_TWO |
1 | 1 |
| total bubbles | 2 | 2 |
Pre-fix this is exactly where the second prompt appeared a second time, with no reply attached.
Two user bubbles right, two matching replies left, after restore. ("Provider exited: recorded pid
absent on host" is expected — the restart killed the provider child and the transcript came back
from the journal.)
Not claimed
No before/after screenshot pair: the "before" would need a dev build of the pre-fix commit. The
red-then-green repro test (journal-rapid-send-turn-coalescing.test.ts, three bubbles for two
prompts without the fix) is the evidence for that direction, not this screenshot.
Independent gates on this head
pnpm tc clean · 1063 test files / 10,646 tests passed, 0 failed (codex, native-chat, claude,
runtime) · changed-file quality gate 0 findings · CI 17 pass / 0 fail.
Main landed the same turn-tracking independently for rewind-busy detection, so this drops our duplicate tracker and reads its set. The auto-merge had also produced two activeTurnIds declarations and two initialisers.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This delta merges the Codex rewind/fork feature from main and reconciles activeTurnIds ownership with the dispatch-echo work.
- Consolidated
activeTurnIdsownership — this PR'strackCodexActiveTurnhelper was removed and main'sobserveCodexRewindActivitynow owns theturn/startedadd /turn/completeddelete, so the coalesced-send classifier and rewind's busy-fence share one source of truth at the sametranslateNotificationcall site. - Added a
dispatchPendingfence —dispatch()sets/clears adispatchPendingflag aroundcaptureBaseline+dispatchCodexTurn; rewind treats an in-flight dispatch as busy. - Adopted the rewind feature wiring —
CodexSessiongained the rewind-required fields (threadId,historyPath,historyMode,dispatchPending,prompts).
Verified the two active-turn trackers are semantically identical (same thread/turn-id guards, same call site), and that tc:node typecheck plus the codex dispatch-echo/adapter/cancel/close/options/rewind suites, the rapid-send E2E, and the runtime integration/replay/exit suites are all green.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Superseded by #20138, which derives the Codex dispatch identity from the echoed user message. |
The defect
A
turn/startissued while a Codex turn is running is coalesced into that turn: the app-serverreturns the same turn id, emits no second
turn/started, and both user messages become items ofone turn at ordinals 0 and 2.
dispatchCodexTurnignored this and stampedordinal: 0on every accepted send, under a commentasserting "Codex records the user message first in a turn" — false for a coalesced send. So the
second submission adopted the first message's identity,
applyDispatchre-pointed that alias to it,and the second message's true identity was owned by nobody. On any thread restore — the 15s eviction
re-attach or an app restart — its history echo failed both alias and fingerprint reconciliation and
was appended as a fresh bubble: the prompt renders twice, and the copy has no reply.
resolveDispatch's own docstring predicted it: a mismatched identity "would silently give the usera second copy of their own message."
The fix
Both legs of the correlation already flowed — Orca sends
clientUserMessageIdon everyturn/start, and the echo comes back carrying it. The identity was computed one line above thepoint where it was discarded. This stops discarding it.
That is also how the sibling provider already works: Claude's adapter never fabricates an identity,
it correlates the provider's echo on a client-minted id and settles late through
settleStructuredAgentSessionLateDispatch, which was already provider-generic. Codex was the oddone out. No change to
AgentSessionDispatchOutcome, no change to shared journal code, no wire orschema change — only the Codex adapter moves.
Classifying the send
An observed
turn/startedoverrides the in-flight check, because on builds that ack before namingthe turn, a fresh turn's own notification lands first and would otherwise read as coalesced.
Two windows, from measurement
Echo latency measured on codex-cli 0.153.4: +1.73s for a fresh turn, +6.15s for a coalesced
send, which waits for the running turn to yield. An in-flight dispatch renders as
dispatching,while
unknownrenders as "Message delivery is unconfirmed."dispatchingmeanwhile; degrading after ~1s wouldtell the user a delivered message was unconfirmed.
The common path gains no latency.
clientIdecho, the positional fallback is disabled permanently, soa misclassified send degrades to
unknownplus late settlement rather than to corruption.Tests
The repro drives the real
dispatchCodexTurnagainst a host that answers exactly as the probedapp-server does — same turn id for both starts, echoes at ordinals 0 and 2 — into the real journal,
then replays restore. It was written failing (three bubbles for two prompts) and its assertions are
unchanged; the fix turns it green through the mechanism. Plus 9 unit tests for the waiter registry
and an end-to-end coalesced-ordinal case through a real translator.
Not included
Journals already corrupted are not repaired. The reconciler that could do it
(
reconcileSubmissions) has no production caller today; wiring it into restore is a separatechange.