fix(server): transport result.answer carries the last segment, reset set aligned with the stream layer - #34
Merged
Conversation
… turn Close the transport-level accumulator left behind by the per-segment rendering work (session-isolation/06) and the run-mirror routing (session-isolation/02): acp.mjs#prompt accumulated result.answer / result.thinking across the whole turn, and streamAcpPrompt copied that concatenation over its own per-segment value when the prompt settled. Every consumer of the settled value then saw turn-long text — the [send] result log, the ● finalize rewrite, the no-usage token estimate and the empty-turn note. Semantics chosen: result.answer / result.thinking carry the LAST segment. acp.mjs now mirrors streamAcpPrompt's lastChunkKind discriminator — same-kind chunks append (streaming growth), any kind change or intervening event (thought ↔ message, tool_call, tool_update, plan_update, ...) resets. A segments array was considered and rejected: streamAcpPrompt is the transport's only consumer and it wants exactly the last segment, so the array would be unused surface in a reviewed public-projection repo. The `|| r.answer` fallback in the settle path is kept and now agrees with the callback-derived value (both sides use the same segment rule); it only matters when the transport saw no chunks of that kind. Tests: new lib/acp-transport-answer.test.js — transport-level pins (multi-segment turn resolves to the final segment; same-kind growth stays one segment; thought/message resets mirror the live ▲ / ● discriminator; a tool_update between message segments starts a new segment) and integration pins through the real runMcodeAcp (no-usage token estimate computed from segment text; a thought-only turn still produces the empty-turn note). Regenerated release/public-source.json for the new test file. Live check: isolated instance on port 18096 with a fake two-segment engine — [send] result logs "answer":"part two" (previously "part one part two"). Gates: pnpm typecheck 0 errors; relevant suites 156 tests × 3 runs green; pnpm test:webapp 213 pass; pnpm build OK.
Acceptance follow-up to the transport per-segment accumulator: the first cut reset result.lastChunkKind on ANY non-chunk session event, while the server-side lastChunkKind only breaks the chain on chat-line-breaking tool events. A usage_update or session_info_update interleaving MID-segment would reset the transport accumulator early, and the `|| r.answer` settle fallback in streamAcpPrompt would then overwrite r.answer with a post-event fragment — truncating the ● finalize rewrite, the token estimate and the empty-turn note input. Chose aligning the reset set (option a) over a shared helper: the two sides speak different event vocabularies (u.sessionUpdate vs c.kind) across the transport boundary and the thought ↔ message kind-change reset would stay duplicated anyway, so a helper would only partially dedup while coupling the zero-dep transport to more server surface. The reset set is now pinned by tests in both directions and both sites carry cross-referencing comments. Reset set after this change: tool_call and tool_call_update (chat-line breaking) plus the thought ↔ message kind change. usage_update, plan_update, session_info_update, mode/goal/config updates and other non-rendering events keep the segment accumulating. Tests: new transport pin — mid-segment usage_update / session_info_update / plan_update events do NOT reset (answer keeps accumulating across them) while tool_call / tool_call_update DO (existing pin). File now 7 tests. Gates: test file 3× green (7 pass per run); relevant suite 91 pass; pnpm typecheck 0 errors.
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.
What
Closes ticket 07 — the transport layer's turn-long accumulator made
[send] result.answer(and the log line, ● finalize rewrite, token estimate, empty-turn note derived from it) a concatenation of every segment, inconsistent with the per-segment semantics established in #30/#32.acp.mjsmirrors the stream layer'slastChunkKinddiscriminator: same-kind chunks append; chat-line-breaking events (tool_call,tool_call_update) and the thought↔message kind change reset.usage_update/plan_update/session_info_update/mode events keep accumulating (they can interleave mid-segment; an early reset would truncate the answer before the settle merge).result.answer/result.thinkingcarry the LAST segment —streamAcpPromptis the transport's only consumer; a segments array would be unused surface.Acceptance (independent agent, 2 rounds)
Round 1 PASS-WITH-CONCERNS — flagged the reset-set drift (broad transport resets vs chat-line-breaking stream resets). Round 2 PASS after alignment: the bidirectional pin drives usage/session_info/plan events mid-segment and asserts accumulation holds (fails on both the old concatenating code and the briefly-broad reset). Gates: test file 7/7 ×3 · relevant suite 91 pass · typecheck 0 · test:webapp 213 · build ✓. Live: fake two-segment engine logs
answer:"part two".Full
pnpm verifydeferred to CI.