fix(cli): keep transcript refreshes monotonic - #3568
Conversation
Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Independent review of 78051ef8ed909c25de0ba54ac38e4a0432737b26.
No P0–P2. Exact-head hosted checks: test is completed / success bound to that SHA. audit and package did not run on this head — recorded as not-run rather than counted as green.
The fix is the right shape
The two refresh paths had asymmetric protection. #refreshLiveTranscript already guarded on session identity, generation, and its own #liveTranscriptRefreshSequence; #refreshTerminalTranscript guarded on session identity alone. Because the sequence counter was scoped to live refreshes only, terminal and live reads could not be ordered against each other at all.
This head collapses both onto a single #transcriptRefreshSequence and gives the terminal path the two guards it was missing. That is the minimal change that makes the two paths comparable, rather than adding a third mechanism to arbitrate between them.
The division of labour between the two counters is worth stating because it is what makes this correct: #sessionGeneration answers identity (is this still the same attachment), #transcriptRefreshSequence answers recency (did something newer already publish). Neither is asked to do the other's job, and there is no timestamp anywhere — the sequence increments synchronously on callback entry, so ordering is arrival ordering and there is no same-millisecond ambiguity to resolve.
Both failure directions are covered, and neither is over-broad
onTranscriptReplaced increments the sequence before publishing, which invalidates every read started by the retired channel. The comment at :1036-1038 states exactly this, and it is accurate.
- Terminal: a slow live read resolving after the turn went terminal finds its sequence superseded and drops. No new tool results follow a terminal turn, so nothing legitimate is discarded.
- Reconnect: in-flight reads from the old channel are invalidated, while reads issued after the snapshot carry a higher sequence and publish normally. This is the part that could easily have been implemented as a blanket post-reconnect rejection; it is not.
- The reverse race is also closed: a terminal read arriving after a reconnect publish is stopped by the same comparison.
#refreshTerminalTranscript has exactly one call site (:1031), which passes the captured sessionGeneration, so the new parameter cannot be reached with a stale or absent generation.
The tests can actually fail
This is the failure mode I most expected in a concurrency fix — a test that executes the two operations in order and asserts the result, which stays green against the unfixed code.
These are not that. Both new cases use a deferred promise to hold the live read unresolved until after the terminal or reconnect publish has already happened, which reproduces the real interleaving rather than a sequential approximation. Our reviewer confirmed this by reverting the production hunk to the previous code and rebuilding: the two new tests go red 2/2, and restoring the fix returns the suite to 48/48 green.
Limitations, stated because they bound the above
Review was performed on Linux. The evidence is source reading plus a locally built red/green run of the test suite; the fix was not reproduced against a real reconnecting network channel, so interleaving coverage rests on the test harness rather than on a live reconnect. audit and package have no result on this head.
AI-assisted review. The counter unification, the guard set on each path, the single call site, and the deferred-promise interleaving were each verified against the source at this exact head, and the red/green behaviour was verified by reverting and rebuilding. Under CONTRIBUTING.md §Review this does not replace the required independent human review; merge remains a human decision.
Summary
Keep Runtime Host transcript replacement monotonic across live tool-result reads, terminal reconciliation, and reconnect snapshots.
A tool result starts an asynchronous snapshot read. Before this change, that older read could resolve after a newer terminal or reconnect snapshot and replace the completed transcript with its pinned, stale contents. Terminal reads also were not ordered against other transcript refreshes.
Use the existing monotonic request sequence for every asynchronous transcript snapshot, invalidate retired-channel reads when a reconnect snapshot lands, and bind terminal reads to the active session generation. Two controlled race tests cover terminal and reconnect ordering.
Verification
npm --workspace maka-agent test— 392/392 passednpx biome lint packages/cli/src/runtime-host-session-driver.ts packages/cli/src/__tests__/runtime-host-session-driver.test.tsgit diff --check origin/main...HEADAI use
Select exactly one:
Tool(s) and scope: Codex audited recent merges, reproduced the transcript race, implemented the ordering fix, and added the regression tests.
Checklist
Does this PR entail a change in behavior?