Fix null compaction content silently clearing conversation history#200
Merged
Fix null compaction content silently clearing conversation history#200
Conversation
The API can send a compaction block where the delta content is null rather than a string. Two problems resulted from this: 1. `'' += null` coerces to the string "null", so the summary stored in the accumulator was literally the word "null" rather than empty. 2. The stop handler always pushed the compaction block to completed and emitted compaction_complete regardless of whether any real content arrived — causing the conversation history to be cleared even though no valid summary existed. Fix: coerce null deltas to empty string, and skip the push/emit at stop time if the accumulated content is empty. The compaction_start event still fires so the UI can show that compaction was attempted, but the history is not wiped and no garbage summary is reported.
Both test files had compaction message helpers built with wrong assumptions: - role was 'user' but compaction blocks come back on assistant messages - field was 'summary' (non-existent) instead of 'content: string | null' - included 'llm_identifier' which is not in BetaCompactionBlockParam - used 'as unknown as' / 'satisfies' casts to paper over the type errors With the correct shape the casts are unnecessary. The replayHistory spec also moves the two compaction tests from the user-messages describe block to the assistant-messages describe block, where they belong.
bananabot9000
approved these changes
Apr 6, 2026
Collaborator
bananabot9000
left a comment
bananabot9000
left a comment
There was a problem hiding this comment.
Deep review completed. The null compaction bug is a silent destruction chain: JS string coercion ('' + null = 'null') -> unconditional push to #completed -> Conversation.push() wipes history. Fix is correct with defense in depth: ?? on delta AND if(acc.content) on stop. replayHistory change is purely display -- pure function, zero side effects, only called for CLI session resume rendering. It fixes 3 pre-existing bugs (wrong role, wrong field, fabricated type cast) but cannot affect real logic. One recommendation: add a MessageStream unit test feeding null compaction deltas to directly test the critical path. Approve. 🍌
The most critical fix — null compaction delta not wiping conversation history — had no direct test. Add a MessageStream spec that feeds the stream a compaction block with null delta content and asserts: - result.blocks does not contain a compaction block (history is safe) - compaction_complete is still emitted with the fallback message Also cover the happy path: valid content produces a block in result.blocks and emits compaction_complete with the actual summary text.
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
2 participants
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
When the API sends a compaction block with
nullcontent (indicating compaction failed), two bugs caused the conversation history to be silently destroyed:'' += nullin the delta handler coerces to the string"null", so the accumulator held a garbage summary rather than being empty.content_block_stophandler unconditionally pushed the compaction block to#completedand emittedcompaction_complete— triggeringConversation.push()to clear all history even though no valid summary existed.A third pre-existing bug:
replayHistorywas looking for compaction blocks onusermessages and reading a non-existentsummaryfield. Compaction blocks land onassistantmessages (viahandleAssistantMessages) and the stored field iscontent.Fix
MessageStream.ts+= event.delta.content ?? ''content_block_stop: only push to#completedwhenacc.contentis non-empty; always emitcompaction_complete(using'No compaction summary received'as fallback) so the UI still shows something in the compaction blockreplayHistory.tsuserbranch to theassistantblock loopblock.contentinstead of the non-existentblock.summaryTest helpers (
Conversation.spec.ts,replayHistory.spec.ts)compactionMsg/compactionhelpers:role: 'assistant', fieldcontent: string | null, no fabricated fields, no casts needed