fix(acp): coalesce trailing reasoning so answer text doesn't split the thought block - #96
Open
ematvey wants to merge 2 commits into
Open
fix(acp): coalesce trailing reasoning so answer text doesn't split the thought block#96ematvey wants to merge 2 commits into
ematvey wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the ACP session event translation so that a late thinking_delta that arrives after answer text has begun does not split the client-visible “thought block” (avoiding [thinking][text][thinking][text] in ACP clients like Zed). It does this by briefly buffering the first answer text chunk when thinking has been seen, then flushing it once a small hold window elapses or before tool-call events.
Changes:
- Add reasoning/text coalescing state to
PiAcpSessionwith a configurable hold window (PI_ACP_THINK_HOLD_MS). - Flush held text before tool-call related assistant events and before tool execution start events.
- Add a regression test covering the “trailing thinking after text starts” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/acp/session.ts |
Buffers initial text when thinking is present to prevent splitting thought blocks; adds flush points around tool-call/tool execution events. |
test/component/session-events.test.ts |
Adds a regression test to ensure trailing thinking deltas coalesce into the same thought block and text is not emitted until after the hold window. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review: - Flush held text before flushEmits in agent_settled and the prompt error path, so buffered chunks aren't dropped if the turn settles before the hold timer fires. - Treat PI_ACP_THINK_HOLD_MS=0 as a disabled hold (>= 0) instead of silently falling back to 200ms. - Use t.after for env cleanup in the coalescing test; add a regression test for flush-on-settle.
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
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.
What
Pi RPC stream can include a trailing
thinking_deltaafter the final answer text has already started. I have this with local vLLM provider, but I suspect it is not limited to that. pi-acp relayedtext_deltaandthinking_deltain arrival order, so ACP clients saw:(i.e. one long thinking block, a couple of answer words, then a short "tail of thinking" block, then the rest of the answer).
Root cause
Pi core is correct: it tags deltas with
contentIndexand renders by block index. pi-acp'shandlePiEventdroppedcontentIndexand keyed blocks by arrival contiguity, so the trailingthinking_delta(contentIndex 0) was splitinto a second block.
Fix
In
src/acp/session.ts, hold the first text chunk briefly while thinking is present (PI_ACP_THINK_HOLD_MS, default 200ms) so a trailing reasoning delta stays in the same thought block. Flush held text before tool-call events and at turn boundaries; once thinking has clearly ended, text streams directly with no added latency.Testing
coalesces trailing thinking into the same thought block (no text split)intest/component/session-events.test.ts.96 passed, 0 failed.--mode rpcdelta stream.Notes
PI_ACP_THINK_HOLD_MS(ms). Set it to0to disable the hold (next-tick flush) — a trade-off of a small first-chunk delay for earlier text streaming in non-reasoning-heavy providers.