Skip to content

fix(cli): reconcile tool results while turns are live - #3524

Merged
Astro-Han merged 3 commits into
apache:mainfrom
mikemikimike:fix/3521-reconcile-tool-results
Aug 23, 2026
Merged

fix(cli): reconcile tool results while turns are live#3524
Astro-Han merged 3 commits into
apache:mainfrom
mikemikimike:fix/3521-reconcile-tool-results

Conversation

@mikemikimike

Copy link
Copy Markdown
Contributor

Summary

Fixes #3521.

Runtime Host live tool_result events 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

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex investigated Issue #3521, implemented the CLI change, and prepared the regression fix. The commit includes the required Generated-by: Codex trailer.

Checklist

  • Tests cover the existing reconciliation behavior and build/type checks pass
  • Full CLI suite passes locally (12 unrelated Windows managed-setup failures noted above)

Does this PR entail a change in behavior?

  • Yes — live tool cards now receive durable results before turn termination
  • No

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han

Copy link
Copy Markdown
Contributor

Reviewed at c484ae65. The problem is real and the shape of the fix is right — a tool_result is exactly the moment the live transcript is stale, and routing it through the existing #publishTranscriptReplacement path keeps one authority for transcript state rather than adding a second. CI is green on this head.

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.

#refreshLiveTranscript starts a loadCurrentMessages fetch per tool_result and publishes whatever resolves, with no sequencing between in-flight requests. A turn with several tool calls — the ordinary path, not an edge case — produces overlapping fetches, and the last one to resolve wins regardless of which snapshot is newer. When an earlier request resolves late, the live transcript visibly regresses to a state missing the newest tool result, which is the symptom this PR is fixing.

#refreshTerminalTranscript has the same shape but fires once per turn end, so overlap was not really available to it. Something like a per-turn sequence number captured before the fetch and re-checked before publishing would close this without changing the design.

[P2] The staleness guard checks #sessionId but not #sessionGeneration.

#refreshLiveTranscript guards with if (this.#sessionId !== sessionId) return;. That is copied from #refreshTerminalTranscript, but #sessionGeneration is incremented on resume/reopen of the same session id (runtime-host-session-driver.ts:599, :805), so an id comparison alone cannot detect a fetch left over from a previous generation of the same session. #assertCurrentSession checks both, and the onGoalChanged handler a few lines above the new call site checks both with a comment explaining exactly this case: "A closing channel from a previous session can still be draining a frame when the swap happens; only the live session may publish."

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.

onToolResult is a new optional seam on RuntimeHostSessionChannelOptions and a new 'tool_result' member of MakaTranscriptReplacementReason, and nothing exercises either. A test that emits two tool_result events and asserts what gets published would also pin down the ordering question above, so the two are worth doing together.

One small note, not a finding: #emit invokes #onToolResult before this.#queue(event.turnId).push(event). Since the refresh refetches from the Runtime Host rather than reading local queue state, I don't think this is a live problem — flagging it only because the ordering reads as accidental rather than chosen, and a future change that made the callback read local state would inherit a subtle bug.

@mikemikimike

Copy link
Copy Markdown
Contributor Author

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.

@mikemikimike
mikemikimike requested a review from Astro-Han August 23, 2026 05:30

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Astro-Han
Astro-Han merged commit d604987 into apache:main Aug 23, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(cli): TUI tool cards settle to "(no output)" and stay stuck once scrolled out of the viewport

2 participants