fix(cli): reconcile tool results while turns are live - #3524
Conversation
Fixes apache#3521 Generated-by: Codex
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — good catch on the underlying problem. The live tool_result carrying status without body, and the card freezing at (no output) once it scrolls out of view, is a real gap, and reusing the existing reconciliation seam is the right way in.
Reviewed at exact head c484ae65c85a6862fbd5e465bd2212398bc73bc7. test is still running, so this is a comment rather than an approval.
[P2, ordinary path] Concurrent refreshes can publish an older snapshot over a newer one.
#refreshLiveTranscript (packages/cli/src/runtime-host-session-driver.ts:1083-1089) starts an independent loadCurrentMessages per tool_result, and the only guard before publishing is this.#sessionId !== sessionId. That checks identity, not ordering — there's no generation counter, watermark, or serialisation, so if two tools settle in quick succession, the read triggered by A can resolve after the read triggered by B and overwrite it.
The visible result is a regression rather than a stale value: pi-transcript.ts:395-402 treats a tool present in the live view but absent from the arriving snapshot as removed, so the newest tool result the user just saw disappears until the next refresh, reconnect, or terminal replacement.
I reproduced it by injecting two deferred transcript reads into the existing driver fixture and resolving the newer one first — the replacement lengths come out [2, 1]. That probe was temporary and isn't in the branch.
A per-session request generation, with stale responses dropped, would close it; so would serialising or coalescing the refreshes. The minimum property is that an older snapshot must never overwrite a newer one.
Nothing else stood out. I confirmed the durable append happens before the live event is emitted, so this isn't a read-too-early problem, and the session guard does correctly block cross-session results — it just doesn't order within one session.
This review was AI-assisted. Findings were verified against the exact head listed above; any mistakes are mine to correct — please push back where I got it wrong.
|
Reviewed at Three things below. None of them says the approach is wrong; they're about the new call site firing much more often than the one it was modelled on. [P2] Overlapping refreshes have no ordering, so the transcript can go backwards.
[P2] The staleness guard checks
Same reasoning as above about frequency: the pattern was tolerable once per turn, and this call site fires many times per turn. [P3] No test covers the new callback.
One small note, not a finding: |
|
Addressed review feedback in commit 7b407a4. Live transcript refreshes now use a monotonic sequence so older in-flight reads cannot publish after newer tool results; completion validates both session ID and session generation. Added a regression test for overlapping tool-result refreshes and the tool_result replacement reason. Verification: npm --workspace maka-agent run build passed; targeted node --test passed (1/1); git diff --check passed. Full CLI pretest remains blocked by existing unrelated TypeScript errors in packages/eval/src/harbor-external-subject.ts. |
Astro-Han
left a comment
There was a problem hiding this comment.
Approving 0c45713d4c8e442f1aa39b573c0d6629e8b7d85d. Required test is completed / success bound to that exact SHA. No P0–P3.
Re-review at the current head. All three earlier findings were re-derived from the diff rather than accepted on the strength of the "Addressed in 7b407a4" note.
Both [P2]s are closed by one coherent change rather than two patches. #refreshLiveTranscript now takes sessionGeneration as a parameter and captures const refreshSequence = ++this.#liveTranscriptRefreshSequence before the fetch; the completion guard checks all three facts together:
if (
this.#sessionId !== sessionId ||
this.#sessionGeneration !== sessionGeneration ||
refreshSequence !== this.#liveTranscriptRefreshSequence
) {
return;
}That closes the ordering hole — a late-resolving earlier fetch can no longer publish over a newer one, so the transcript cannot regress to a state missing the newest tool result — and it closes the generation hole, since a fetch left over from a previous generation of the same session id is now rejected the way #assertCurrentSession and the neighbouring onGoalChanged handler already did.
The sequence comparison is strict equality against the latest issued value, so only the most recent refresh may publish. For a live transcript that is the correct policy rather than merely a safe one.
The [P3] is closed too: 34 lines of regression covering overlapping tool-result refreshes and the tool_result replacement reason — which, as noted before, is what pins the ordering behaviour rather than just exercising the new seam. The third commit is pure formatting (one call collapsed onto a single line).
Disclosure, because it changes what this approval is worth: this is an AI review. Under CONTRIBUTING.md §Review it does not count as the required independent human review. It means the code has been checked, not that the gate is open — merge still needs a committer other than the author to give LGTM and to decide.
Summary
Fixes #3521.
Runtime Host live
tool_resultevents intentionally omit durable content. The CLI previously reconciled those details only when a turn terminated, so cards that scrolled into terminal scrollback could remain stuck at(no output). This change refreshes the durable transcript when each live tool result settles and reuses the existing transcript replacement/reconciliation path.Compatibility
No protocol or persisted-message changes. The live event contract remains unchanged; this only adds a CLI-side durable transcript refresh.
Verification
npm install— passed; repository dependency patches applied.npm run build— passed.npm --workspace maka-agent run typecheck— passed.npm --workspace maka-agent test— 359 passed, 12 failed in existing Windows managed-setup environment tests unrelated to this change; 1 skipped.git diff --check— passed.AI use
Tool(s) and scope: Codex investigated Issue #3521, implemented the CLI change, and prepared the regression fix. The commit includes the required
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?