Repository navigation
fix(staged): keep the final assistant message whole across mid-message tool calls - #971
Merged
Merged
Conversation
Background subagents forward their tool calls into the parent's ACP feed while the parent may still be streaming text. Every tool call finalized the writer's open assistant row, so one ACP message was persisted as two rows with a tool card between them — the first fragment (sometimes just "The") read as a truncated final message. A tool call now only flushes the open row and marks it interrupted. The driver remembers the provider messageId of the streamed text and, when the next chunk carries the same id, calls the new `MessageWriter::resume_interrupted_text` hook so the text keeps extending that row; any other id (or none) starts a new row as before. The hook defaults to a no-op, so one-shot and probe writers are unchanged. The driver also stops clearing its response buffer at tool calls, so an SDK result for the resumed message still certifies against the whole text and reaches `finish_completed_response` (note-completion detection). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
A mid-message tool call now leaves the assistant row open behind the tool
rows, and the writer keeps extending it in place. The pane's since-poll only
re-fetched the tail (`id >= lastId`), so that row's later writes never
reached the screen: the user kept seeing the first fragment ("The") above
the tool cards until the session was reopened.
`get_session_messages_since` gains a `refetch_ids` parameter mirroring the
metadata query's, and the pane passes the latest assistant row whenever it
is no longer the tail — the only row the writer can still be extending. The
tail merge moves into `messagePoll.ts`, replacing a re-fetched row only when
it changed so unchanged rows keep their identity. The web server treats
`refetchIds` as optional for older bundles.
A terminal status event now triggers one last poll, so the final text the
teardown writes just before the status flips is not missed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
Since tool calls stopped clearing the response buffer, a turn that ended after a tool call with no further text could no longer certify its SDK result: unless the result happened to extend the pre-call text, `accept_result` retired the response without recording `response_result` or running `finish_completed_response`, and non-success results lost their metadata row too. When the buffered message was interrupted by a tool call, a result that does not match it is now treated as text that was never streamed, as it was when the buffer was empty: it is injected (if successful), recorded, and can finish the response. The writer's interrupted flag makes that fallback land in a new row. A result that extends the buffer now carries the buffered message id, so after an interruption its suffix resumes the same row instead of splitting it. The driver also marks a tool call as interrupting unconditionally, the same as the writer does. With an empty buffer there is no message id to match, so behaviour is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The final poll fired after a terminal `session-status-changed` could overlap an interval tick that was still awaiting its fetch: `stopPolling()` reset `pollInFlight`, so the guard let it through. Both polls then read the same `lastId`, and whichever merge ran second cut only the last cached row before splicing its tail, duplicating every row the first merge had already added. The terminal branch now goes through `finishPolling()`, which notes whether a tick is in flight before tearing the interval down. With none in flight it polls immediately as before; otherwise it sets `finalPollPending` and the tick's `finally` runs the poll once it settles, so the two never share a cursor and the final poll still runs exactly once. `stopPolling()` clears the flag so a close or session switch drops a pending poll, and the tick's own "session is done" exit only clears the interval, so it cannot discard a final poll requested during that same tick. `mergeSinceMessages` no longer assumes the last cached row is `lastId`: it finds the cursor row from the end and splices the fetched tail at that position, so a stale cursor replaces the rows both fetches saw instead of appending them again. The unchanged-tail check compares the whole span from the cursor, so an overlapping poll that returns rows already applied still yields no change, and `appended` is true only when the tail reaches past the last cached row. A cursor that is not cached falls back to replacing the last row as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…er fetch misses `finishPolling` went through `stopPolling()`, which reset `pollInFlight` while the tick it had just observed was still awaiting its fetch. For the rest of that await the guard in `poll()` was open, so a `startPolling()` in the window (the queue-drain running event that often follows a terminal status, or a send) could arm an interval whose first tick ran concurrently with the in-flight one against the same cursor — the overlap the previous commit set out to close. The terminal branch now only clears the timer and either defers or runs the final poll; `pollInFlight` is owned solely by the tick's `finally`. `stopPolling()` is unchanged, so a close or session switch still drops a pending final poll. `mergeSinceMessages` assumed the fetched tail was at least as fresh as the cached span from the cursor. When an older fetch settled after a newer one, its shorter tail replaced the whole span and the rows it did not reach vanished until the next tick — which, after the final poll, never comes. The merge now keeps the cached rows past a tail that ends before the cached tail, and the unchanged check accepts a shorter tail whose rows are all identical, so such a fetch yields no change at all. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
`mergeSinceMessages` kept the cached rows an older fetch did not reach with `base.slice(cut + tail.length)`, which assumes the tail holds one row per cached row from the cursor on. A cached row between the cursor and the tail's last id that the tail omits (e.g. one that no longer passes the visible-message filter) shifted the slice, so a row the tail already contained was kept a second time and its id appeared twice. The keep is now id-based: every row from the cursor on whose id is past the tail's last id. That is empty whenever the tail reaches the cached end, so the `previousTail` guard is gone, and a skipped row neither duplicates its successor nor reappears. The doc comment also states the merge's limit: for a row both the cache and the fetch hold it cannot tell which copy is newer, so it relies on the pane serialising since-fetches (`pollInFlight` / `finalPollPending`) rather than being a full defence against out-of-order fetches. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Background subagents forward their tool calls into the parent's ACP feed while the parent may still be streaming text. Every tool call finalized the writer's open assistant row, so one ACP message was persisted as two rows with a tool card between them. The first fragment (sometimes just "The") then looked like a truncated final message.
Changes
Writer / ACP driver
messageId, the driver calls the newMessageWriter::resume_interrupted_texthook so the text keeps extending the same row. The hook is a no-op by default, so one-shot and probe writers behave as before.finish_completed_response.Chat pane polling
get_session_messages_sincetakes a newrefetch_idsparameter. While the latest assistant row is no longer the tail, the pane passes its id so later writes to that row show up. The web server treatsrefetchIdsas optional so older bundles still work.messagePoll.ts, with tests:finalPollPending) so the two never share a cursor and duplicate rows. Only the tick itself resetspollInFlight.🤖 Generated with Claude Code