fix(tui): render terminal assistant messages - #1059
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@codex review |
![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: 8745889da9
ℹ️ 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".
| // providers precede it with deltas, but valid non-streaming | ||
| // providers may emit only this terminal message. | ||
| m.lastAssistantText = p.Content | ||
| m.renderActiveAssistantBubble() |
There was a problem hiding this comment.
When an earlier tool-calling step emitted text deltas, responseStarted remains true after handleToolStart appends the tool card. If the later terminal step is non-streaming, this call reaches renderActiveAssistantBubble, which uses ReplaceTailLines(activeAssistantLineCount, ...) even though the viewport tail is now the tool card; it therefore removes part or all of that card and leaves its recorded line offsets stale. This can occur with the supported per-turn mix of deltas/tool calls followed by a final-only response, so the terminal reconciliation must append or replace the tracked assistant bubble without treating unrelated tail content as that bubble.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed at exact head ebe2d9a85954cea45e0763cabec57ebba747e08c.
The regression now drives the realistic sequence assistant.message.delta -> tool.call.started -> tool.output.delta -> tool.call.completed -> assistant.message, replays the tool completion and terminal event, then replays terminal completion and starts a later run. It asserts the early assistant bubble, tool card, and final-only assistant bubble remain ordered and appear exactly once, the transcript finalizes once, and the later run records normally.
The minimal ownership fix closes the preceding assistant bubble when handleToolStart appends the tool card; the next provider step starts a fresh accumulator. Terminal replay is also ignored once that run's response is finalized.
Verification: focused normal/race, complete TUI normal/race, all cmd/harnesscli/..., and foreground non-TTY ./scripts/test-regression.sh all pass; coverage is 85.6% with zero uncovered functions. The unrelated hosted fixture race remains #1044 / PR #1045 and was not folded into this PR.
|
@codex review Please re-review exact head |
|
@codex review Final exact head is now |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
00ccede to
9b7cdde
Compare
|
@codex review Please review exact rebased head |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
9b7cdde to
6ce1877
Compare
2026-08-01 exact-head promotion evidencePublished exact candidate: Exact base: The candidate was rebased from the prior PR head and independently reviewed. The review found one additional P2: Strict red-green follow-up:
Authoritative foreground verification on exact No baseline failure was waived. The branch was updated with an exact force-with-lease from old remote head |
6ce1877 to
3506e01
Compare
1 participant
Closes #1056
Related to #1000 as an adjacent TUI reliability fix; this is not a scheduled-automation child issue.
Summary
The TUI now treats a non-empty terminal
assistant.messageevent as the authoritative full assistant response. Valid non-streaming providers that emitTurn.Contentwithout deltas therefore render a visible assistant bubble and finalize the existing transcript entry onrun.completed.The reducer remains idempotent for streaming providers: an identical delta-assembled response plus terminal full message renders once, while a differing terminal full message replaces the partial content authoritatively.
Root cause
StartSSEBridgeWithOptionsalready forwardedassistant.message, and harnessd already persisted and served it.Model.Updatehandled onlyassistant.message.delta; consequentlylastAssistantTextstayed empty for final-only responses and the existingSSEDoneMsgpath had nothing to finalize into the TUI transcript.The documented fake-provider contract permits
Turn.Contentwith optionalDeltas, so the reproduction was valid provider behavior rather than contract misuse.Scope and impact
run.completed.3c38a401if streamed replies duplicate or terminal reconciliation regresses.Test-first evidence
Red before implementation:
Failed because final-only content left
LastAssistantText()empty and a differing terminal full message left the partial delta text authoritative.Green after implementation:
All passed. The full regression gate completed normal tests, full race, coverage, and reported
coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0)followed by[regression] PASS.Real TUI reproduction and proof
Unmodified
origin/mainatfedcf6073135deb7cce1fa49921aa698a9cc7cd7, real tmux-hosted harnessd plus real 120x40 TUI:run_e4e9aadf-fdf5-417e-bbc6-9640fd80e5aarun_f9ea72e5-3250-4152-b65b-dc177d9d3107LUNA_NONSTREAM_REPLY_ONE,LUNA_NONSTREAM_REPLY_TWOassistant.messagethenrun.completed, with no deltaFixed branch, same real path, three consecutive turns:
run_35981662-52fb-461f-8908-6fa3b55a0792run_a9c4d801-8e7f-4a1d-a923-6517f44bbcferun_73d2c720-be6f-4d81-aa8c-de9feca8a625ISSUE1056_FIXED_REPLY_ONE,_TWO, and_THREEeach rendered exactly onceThe visible
0%/200Kcontext indicator remained expected for 25/37/49 tokens because sub-1% usage rounds to zero; cost increased each turn and this PR does not change usage accounting.Documentation
Added the issue plan and cross-surface impact map and updated the required plan index plus long-term, engineering, observational, and system logs.
Checklist
Exact-head review follow-up (
ebe2d9a8)Addressed review comment 3687040783 and the additional Luna exact-head finding in the same #1056 acceptance boundary:
lastAssistantText, preserving post-run copy behavior.RunStartedMsgresets finalization so later turns render and record normally.The behavior regression drives:
assistant.message.delta -> tool.call.started -> tool.output.delta -> tool.call.completed -> assistant.message -> reconnect re-delivery -> run.completed -> terminal/completion replay -> later run.It asserts early bubble -> tool card -> final bubble order, exact-once viewport content, exact-once transcript finalization, and a distinct later response.
Red evidence:
early=35 tool=85 final=59).Verification at
ebe2d9a8:cmd/harnesscli/tuinormal and race: PASSgo test ./cmd/harnesscli/... -count=1: PASS./scripts/test-regression.sh: PASS (normal, full race, E2E, coverage)coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0)The hosted fixture race remains owned by #1044 / PR #1045 and is not changed here; it did not reproduce in this foreground full-race run.
Rebase verification (
b3afc7ec->9b7cdde3)Rebased cleanly without conflicts onto exact
origin/mainb3afc7e; the published PR head is 9b7cdde. Range-diff preserved the final-only assistant-message reconciliation, mixed delta/tool/final viewport ownership fix, reconnect/terminal replay idempotency, and later-turn behavior. The only contextual difference is the expected #1045 plan-index entry now supplied by the new base.Verification on the rebased exact head:
go test ./cmd/harnesscli/tui -run 'TestSSEEventMsg_AssistantMessage_MixedToolStepPreservesViewportAndReconnectIdempotency' -count=1: PASSgo test -race ./cmd/harnesscli/tui -run 'TestSSEEventMsg_AssistantMessage_MixedToolStepPreservesViewportAndReconnectIdempotency' -count=1: PASS./scripts/test-regression.sh: PASS (normal, full race, E2E, coverage)coveragegate: PASS (total=85.6%, min=80.0%, zero-functions=0)