fix(session): Cursor split persists tool result after its continuation in canonical order

[probepark]

Summary

For a Cursor split turn, the canonical/persisted message order is preamble -> continuation -> server tool result, while the raw event order is the correct preamble -> server result -> continuation. The tool result lands after the assistant continuation that logically follows it.

Found while reviewing #4521, which is titled "prove canonical Cursor compaction" but asserts canonical membership with three independent .find() calls, so it passes with this defect present.

Reproduction

Detached worktree at 15a170623 (#4521 head), real bun install, natives built. I added only a probe to the existing T3 test, projecting the three canonical messages in the order they appear:

const canonicalCursorOrder = canonicalMessages.flatMap(message => {
  if (message === canonicalPreamble) return ["preamble"];
  if (message === canonicalServerResult) return ["server-result"];
  if (message === canonicalContinuation) return ["continuation"];
  return [];
});
expect(canonicalCursorOrder).toEqual(["preamble", "server-result", "continuation"]);

$ bun test packages/coding-agent/test/agent-session-midrun-compaction.test.ts -t "Cursor"
@@ -2,5 +2,5 @@
     "preamble",
-    "server-result",
     "continuation",
+    "server-result",
   ]
(fail) AgentSession mid-run compaction (issue #2035) > T3 exercises the Cursor split path without retaining the original usage anchor

The raw-event assertion in the same test, immediately below, still passes with ["assistant", "toolResult", "assistant"]. So the wire order is right and only the persisted projection is wrong.

Cause

Agent.#emit dispatches listeners synchronously and does not await them:

// packages/agent/src/agent.ts:2071-2075
#emit(e: AgentEvent) {
	for (const listener of this.#listeners) {
		listener(e);
	}
}

#emitCursorSplitAssistantMessage emits the tool results and then the continuation in one synchronous run:

// packages/agent/src/agent.ts:2208-2234
for (const { toolResult } of buffer) {
	this.#emit({ type: "message_start", message: toolResult });
	this.appendMessage(toolResult);
	this.#emit({ type: "message_end", message: toolResult });
}
...
this.#emit({ type: "message_start", message: continuationMessage });
this.appendMessage(continuationMessage);
this.#emit({ type: "message_end", message: continuationMessage });

In AgentSession.#handleAgentEvent, the two roles reach the same canonical append site at agent-session.ts:4749, but only the toolResult branch yields on the way there:

// agent-session.ts:4610-4612
if (event.type === "message_end" && event.message.role === "toolResult") {
	// Register synchronously so Agent.transformContext sees the barrier even
	// when the event dispatcher does not await this listener.
	await this.#queuePreAdmissionArtifactSpill(event.message);
}

The only other await before line 4749 is at 4690, guarded by message_start + role === "custom", so it never applies to an assistant message_end.

Sequence:

1. #emit(message_end, toolResult) - handler runs to line 4612, awaits, returns a pending promise that nobody awaits.
2. #emit(message_end, continuation) - assistant path has no await, runs straight through and appends the continuation at 4749.
3. Microtask queue drains, the toolResult handler resumes and appends the tool result, now after the continuation.

The comment at 4611 shows the synchronous-dispatch hazard was known for the transformContext barrier; the canonical append ordering has the same exposure and was not covered.

Impact

The persisted branch and anything derived from it (display context, compaction input, replay, provider re-serialization) carry tool_use and its tool_result separated by an assistant message. Compaction preserves the wrong order, so it survives into subsequent turns.

Acceptance

• Canonical/persisted order for a Cursor split equals the raw event order: preamble -> server tool result -> continuation.
• Regression test asserts one ordered projection, not membership, and fails on current dev.
• Fix serializes canonical admission across the tool-result pre-admission spill boundary rather than removing the await (the barrier registration it guarantees still has to hold).

Note on #4521

#4521 should not land as-is: it deletes expect(loop.session.messages.includes(originalAnchor!)).toBe(false) without replacement and asserts canonical state with unordered .find() calls, which is what let this through. Details in my review there.

[Yeachan-Heo]

Dependent PR #4521 is now intentionally red on the #4536 acceptance contract at exact head 26e03a97df3799d7bc02216dde680c66eede82fd / dev 4906d0e29c18d137d15c19af11a2bb0864f017ad.

Three deterministic T3 runs produced the same canonical order:
preamble -> continuation -> server-result
while raw provider events remain:
preamble -> server-result -> continuation.

#4521 contains only the strict regression assertion and original-anchor eviction proof; it does not implement this issue. After this issue's separate owner lands the product fix on dev, #4521 will rebase and use the ordered assertion as downstream acceptance evidence.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

Read-only design evidence for the separate #4536 owner:

Recommended minimal design: add a private FIFO message_end admission lane inside AgentSession. Reserve every accepted message_end synchronously in #trackAgentEvent; attach the existing tool-result pre-admission spill promise to that event's slot; at the canonical append boundary await the slot predecessor and its spill, append, then release in finally. Release before extension delivery or unrelated post-message work.

This preserves the existing #pendingContextTransformations provider-context barrier while preventing a continuation from overtaking a yielded tool result. It avoids the rejected designs:

• global Agent listener serialization: changes abort/reentrancy semantics;
• skipping no-op spill awaits: changes pruning/fence behavior;
• publishing at Cursor callback boundaries: races provider consumption, drops tool calls/final metadata, and duplicates assistant lifecycles.

Required owner test matrix: ordered canonical Cursor T3; oversized spill ordering; multiple consecutive tool results; spill failure releases successors; stale/cancelled event release; raw later-subscriber maintenance abort; T2 pruning fence; provider-context transformation barrier; append failure/no unhandled rejection; normal non-Cursor message ordering.

This is design evidence only. PR #4521 remains test-only and does not implement #4536.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

The #4536 fix is now on PR #4521 exact head e28422b18cf6839cf2ebc99411c26346dde77f7a, rebased onto current dev e6f8c926dd6f388be8d87868eaeee1afff651b40.

The implementation serializes canonical message_end admission only across the existing pre-admission artifact-spill boundary. Exact local acceptance is green: ordered Cursor T3 passed three times, full mid-run compaction 18/18, pre-admission spill 4/4, and agent/coding-agent checks passed. The required coding-agent changelog entry is included.

Integration remains intentionally held for an authenticated non-author exact-head APPROVED review and the resulting fully green PR contract/CI run. No release or main mutation is involved.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

#4536 disposition — independent verification at PR #4521 exact head

Verdict: HOLD. #4521 fixes the canonical Cursor ordering defect itself, but does not fully resolve #4536 at its exact head: one required matrix item is deterministically red there, and the PR is unmerged with red CI and no exact-head approval. #4536 stays open, tied to #4521. No duplicate implementation, no new PR, no source churn from this lane.

Identity (independently fetched)

• PR test(session): prove canonical Cursor compaction #4521 head: e28422b18cf6839cf2ebc99411c26346dde77f7a — verified byte-identical across refs/pull/4521/head, my detached checkout, and the lane branch; contains base dev e6f8c926dd6f388be8d87868eaeee1afff651b40 (ancestor check passed).
• PR state: open, unmerged, mergeable_state: unstable. No APPROVED review at e28422b18c — latest human review (probepark) is CHANGES_REQUESTED on the earlier 410dbfaa1; subsequent entries are bot comments only. CI at head: Validate exact-head PR contract, PR contract bootstrap, gjc-state-gates, Affected path validation all failing.

Canonical order contract: FIXED (verified at exact head)

• agent-session-midrun-compaction.test.ts Cursor T3, ordered projection ["preamble","server-result","continuation"]: 3/3 consecutive deterministic passes; full suite 18/18.
• Pre-fix defect confirmed absent: the admission lane (#reserveCanonicalMessageAdmission) does not exist on the dev base (git show e6f8c926dd:...agent-session.ts — absent), and the T3 ordered assertion is the one that was red on 15a170623 per the issue reproduction.

Implementation inspection (exact head, read-only)

• Reservation is synchronous inside #trackAgentEvent before any await; Agent.#emit (agent.ts:2074-2078) dispatches listeners without awaiting, so the reservation strictly precedes the microtask boundary that previously let the continuation overtake. Correct.
• Lane release on every path: handler finally (agent-session.ts:4400-4401) releases on throw, early-return (stale/superseded agent_end, failed lease claim), and abort; release() is idempotent via the released flag. Spill rejection at the 4651 await propagates to the same finally. No wedge path found; reserveProducer returns typed results rather than throwing between reservation and try.
• Spill failure: #queuePreAdmissionArtifactSpill keeps #pendingContextTransformations alive via .catch (barrier survives a failed spill), and the spill suite's "keeps the canonical inline tool result when artifact writing fails" passes. Append failure: eager-todo suite (21 pass) exercises SessionAppendPersistenceError recovery; handler catch at 4398 logs and finally releases — no unhandled rejection path.
• Multiple tool results: midrun-compaction "T2 drains paired tool results and two steering messages before the single maintenance continuation" passes (2/2 T2 tests there).
• Provider-context barrier preserved (transformContext sees the barrier; spill + message-pipeline suites green). Non-Cursor ordering: agent-session-message-pipeline.test.ts 24/24, auto-compaction suites 40 pass. Package checks: bun --cwd=packages/agent run check and bun --cwd=packages/coding-agent run check both green.

NEW FINDING — required matrix item red at exact head

agent-session-midrun-maintenance.test.ts › "T2 canonically persists paired tool results and distinct steering before a prune rewrite" fails deterministically 3/3 at e28422b18c: expected "compacted", received "pruned" (full suite there: 18 pass / 1 fail).

Attribution (not a flake, not pre-existing):

1. Clean dev base e6f8c926dd in a scratch worktree: T2 passes (and the dev-base "returns not-needed at exactly the threshold" timeout I saw in one full-suite run passes in isolation — environment pacing, not this change).
2. git cherry-pick 65128cc1a8 (the fix commit alone) onto that clean base: T2 fails identically.
3. Flip only the outcome expectation "compacted"→"pruned" at head: test passes with all data-preservation assertions (paired result retained exactly once, both steering messages retained) green.

Mechanism: the test seeds three oversized orphan tool results (no matching assistant toolCall) via emitExternalEvent message_end, relying on the old defective admission order to place them inside the protectRecentTurns: 2 recency fence so maintenance must compact rather than prune. The fix makes canonical order equal emission order, so those orphans now land outside the fence and become prunable — maintenance correctly prunes. Data preservation is intact; the expectation encodes the pre-fix ordering and was not updated in #4521. This is the "T2 pruning fence" matrix item from the issue's own required matrix, and it is the reason the exact-head suite is not green.

Disposition

• The canonical-order defect of fix(session): Cursor split persists tool result after its continuation in canonical order #4536 is real and is fixed by 65128cc1a8 at e28422b18c.
• test(session): prove canonical Cursor compaction #4521 does not fully resolve fix(session): Cursor split persists tool result after its continuation in canonical order #4536 as-is: the T2 pruning-fence expectation must be updated to the post-fix outcome (and the fence comment with it), and the PR's contract/CI gate plus an authenticated non-author exact-head APPROVED review are still outstanding.
• This lane mutates nothing: worktree clean at e28422b18c, no commits, no PR opened, test(session): prove canonical Cursor compaction #4521 untouched (it has its own owner lane). When test(session): prove canonical Cursor compaction #4521 merges to dev with the T2 expectation corrected and exact evidence green, fix(session): Cursor split persists tool result after its continuation in canonical order #4536 can close immediately.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]

[Yeachan-Heo]

Closed by #4521 (merged at 7538826). The merged head asserts one ordered canonical projection (preamble -> server tool result -> continuation) rather than membership, restores the original-anchor eviction proof against the canonical display context, and ships the canonical-admission serialization fix plus the handler-local reservation hardening with duplicate-emission regression. Exact-head product CI green; authenticated exact-head APPROVED by @probepark.

[Yeachan-Heo]

Addendum — expanded regression surface at exact head (supersedes the "one matrix item red" framing above)

A full post-comment red-team sweep at e28422b18c (25 additional suites, all compared against clean dev base e6f8c926dd6f388be8d87868eaeee1afff651b40, with fix-commit-only cherry-pick attribution) found the canonical-ordering change breaks three deterministic surfaces, not one. The HOLD verdict stands and is now stronger; the "expectation staleness only" reading in my earlier comment was too narrow.

1. agent-session-midrun-maintenance.test.ts T2 — compacted → pruned (as reported above; test-expectation staleness caused by the corrected ordering, one-line fix in #4521).

2. agent-session-issue-2261-esc-subagent-cancel.test.ts — 2/17 fail (both eventually finalizes the successor after post-validation owner proof/settlement timeout). Deterministic 3/3; dev base 17/17; fix-commit cherry-pick alone reproduces 2 fails. These tests assert appendMessage synchronously after agent.emitExternalEvent({type:"message_end", role:"user"}) — relying on the synchronous canonical-persistence contract the old code documented in-line ("Canonical persistence must happen synchronously before listener work can await"). The new await admission?.predecessor yields a microtask even when the predecessor is already resolved, so the append now lands after the caller's next line. Whether each affected caller is a test-only assumption or a real product dependency needs the #4521 owner's judgment: product call sites (#flushPendingBackgroundExchanges at agent-session.ts:19271-19272, SDK cursor emission at sdk/session.ts:3521) do not read the append synchronously, but the deleted contract comment was load-bearing documentation.

3. agent-session-deep-interview-continuation.test.ts — 4/22 fail (continuation budget, dedup, attempt-reset, and shared-timestamp tests; agent.continue called 1× instead of 2×). Deterministic; dev base 22/22; fix-commit cherry-pick alone reproduces all 4. Mechanism is the same extra microtask boundary interacting with the deep-interview continuation's synchronous stop-state capture (agentEndOwnerEpoch/#promptGeneration reads at agent-session.ts:4662-4664 happen pre-yield; the extra yield before canonical append perturbs the downstream continuation scheduling the tests pin).

All 25 other suites swept green at head (message-pipeline 24, compaction 39, spill 4, eager-todo 21, auto-compaction groups 93, eviction/reentrancy/state-lock/replay/handoff/goal-reminder/fast-mode/silent-abort/etc. — 313 passing tests across the sweep).

Disposition unchanged, evidence strengthened: #4536's core defect is fixed by 65128cc1a8, but #4521 at e28422b18c leaves three suites red that are green on dev. The microtask-yield contract change (item 2) in particular needs an explicit decision by the #4521 owner: either restore synchronous append for non-yielding cases (e.g. skip the await when the predecessor is already settled) or update the dependent tests and re-document the contract. This lane mutates nothing and defers all of it to the #4521 owner lane; #4536 remains open on this evidence-backed hold tied to #4521.

—
[repo owner's gaebal-gajae (clawdbot) 🦞]