fix(session): track terminal assistant before admission wait - #4565
fix(session): track terminal assistant before admission wait#4565Yeachan-Heo wants to merge 1 commit into
Conversation
Canonical message admission serialization made externally emitted terminals race their own message_end: the agent_end handler could read #lastAssistantMessage before the message_end handler assigned it, so post-turn continuation logic (deep-interview stop handling) ran against the previous turn's assistant — double-processing one stop and skipping another. Capture the terminal assistant synchronously, before the admission wait, mirroring the reservation that is already synchronous. T2 mid-run maintenance is updated to the chronological contract: with canonical admission persisting seed order, the oldest orphan tool result falls outside the protectRecentTurns fence, so maintenance correctly prunes (canonical truncate notice) instead of compacting; the paired result, both steering messages, and the codex close are still asserted. Lore-id: 71c2e9b0 Constraint: must not reorder persistence — only the synchronous bookkeeping capture moves Rejected: awaiting admission inside the agent_end handler | would serialize terminals behind unrelated spills Confidence: high Scope-risk: narrow Reversibility: trivial Tested: deep-interview continuation 22/22; mid-run maintenance 19/19; midrun compaction 18/18; spill 5/5; cursor-exec + concurrent 58/58; coding-agent check green Not-tested: live provider terminals in production traffic
![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: 47f0c30816
ℹ️ 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 (event.type === "message_end" && event.message.role === "assistant") { | ||
| this.#lastAssistantMessage = event.message; |
There was a problem hiding this comment.
When a host bridge or replay emits two complete lifecycles back-to-back without awaiting the asynchronous handlers, the second message_end executes this assignment before the first agent_end resumes from #emitSessionEvent and #goalRuntime.onAgentEnd. The first terminal then consumes the second turn's assistant at #lastAssistantMessage ?? fallbackAssistant and clears the field, so retry, compaction, and deep-interview handling can skip the first stop and process the second stop twice. Cache assistants by terminal/run identity, or prefer the assistant carried by the matching agent_end, rather than using one shared mutable slot.
Useful? React with 👍 / 👎.
|
Superseded by #4564 — closing unmerged as a duplicate lane. Overlap evidence:
Coverage comparison:
Unique evidence preserved from this lane (recorded on #4564): the deep-interview root-cause trace — an externally emitted This PR's current CI red is only the intentional — |
|
Closing unmerged: superseded by canonical owner #4564 (see signed overlap-evidence comment above). |
Externally emitted terminals dispatch agent_end immediately after message_end. With the admission predecessor contended (gated spill), the message_end handler parked before its post-persistence capture site, so the agent_end handler's post-turn read resolved #lastAssistantMessage to the PREVIOUS turn's assistant: a mid-loop toolUse capture short-circuited stop handling via hasToolCalls and silently skipped the terminal's deep-interview continuation; an errored capture misclassified the terminal for retry. Adversarial provenance: identified by closed duplicate PR #4565's terminal-assistant-before-wait reasoning; proven real on #4564's head with an instrumented probe (agent_end resolved the previous turn's timestamp while the terminal admission was parked) and pinned by a discriminating test that is red without this capture and green with it. Subsumes #4565 alongside the uncontended fast path: contended and uncontended admissions now both expose the terminal synchronously. Lore-id: issue-4536-postmerge-regressions Constraint: single-source capture; post-persistence block keeps side effects only Constraint: do not regress the duplicate-emission wedge fix Confidence: high Scope-risk: narrow Reversibility: easy Tested: discriminating capture test red-without/green-with; deep-interview 22/22; 2261 17/17; maintenance 19/19; spill 7/7; midrun-compaction 18/18; message-pipeline 24/24; check green
1 participant
Summary
Post-merge repair for the #4521 canonical admission serialization fallout on
dev:#lastAssistantMessageis captured synchronously in themessage_endhandler, before the canonical-admission wait. Externally emitted terminals (host bridges/replays/tests) dispatchagent_endimmediately aftermessage_end; with serialized admission theagent_endhandler could otherwise read the previous turn's assistant and run post-turn continuation logic (deep-interview stop handling) against the wrong turn — double-processing one stop and skipping another.protectRecentTurnsfence and maintenance prunes (canonical truncate notice) instead of compacting. The paired result, both steering messages, the codex provider close, and the absence of a compaction entry are still asserted.Verification
agent-session-deep-interview-continuation.test.ts: 22/22 (was 4 fails on merged dev)agent-session-midrun-maintenance.test.ts: 19/19 (was 1 fail on merged dev)agent-session-midrun-compaction.test.ts18/18; spill suite 5/5; cursor-exec + concurrent 58/58bun --cwd=packages/coding-agent run check: green47f0c308169d2a2d2f2d8f074333bba92fe1fc4d6902f5b2dfPre-existing local-env red (sixel pet-mode, sdk-workflow-gate-emitter) reproduces identically on the pre-merge base and is not touched here.
Follow-up to #4521. Fixes the post-merge shard-4 red introduced by 65128cc/e1dd34d3dd.
gajae.pr-review-verdict.v1 needs-human sha256:580168f60d58748845c9160b370591c28d1bbf3b0516b5eab20c1ea3a31b61b0 reviewer:human reviewer-id:pending-independent-review evidence:awaiting-authenticated-exact-head-review-47f0c30816