fix(claude-code): retain the whole unprocessed suffix, not just its last turn - #2999
Open
merlinr68 wants to merge 1 commit into
Open
fix(claude-code): retain the whole unprocessed suffix, not just its last turn#2999merlinr68 wants to merge 1 commit into
merlinr68 wants to merge 1 commit into
Conversation
…ast turn
Full-session retention selects the unprocessed transcript suffix using the
persisted message offset, then passed that suffix through last-turn slicing on
every retain after the first. The cursor was advanced to the end of the full
transcript regardless, so any message removed by that second window was skipped
permanently.
messages_to_retain = all_messages[retention_progress.start_index:]
retain_full_window = retention_progress.start_index == 0 # narrows it again
The invariant that breaks: once plan_retention() establishes
[start_index, len(all_messages)) as the unprocessed range, every message in that
range must be formatted, filtered by retention policy, or left uncommitted — it
must not be dropped by a second positional window while commit_retention()
records the range as processed.
Claude Code makes the impact far worse. Tool results are stored as role="user"
messages, so the last-turn scan usually anchors on a tool result rather than the
human prompt; with the resolved default retainToolCalls=false (config.py DEFAULTS
and settings.json both set False) that message extracts to empty and the entire
retain is skipped.
Measured on a real 1,500-message transcript, formatting the same 30-message
unprocessed suffix:
last-turn, tools off (default) -> 0 chars — retain skipped
full-window, tools off -> 3,697 chars across 6 messages
And one session's retain history: first chunk 54,317 chars, every later chunk
287-2,319 chars regardless of how much work happened between them.
The existing offset continues to prevent duplicate retention; role, memory-tag
and tool-content filtering are unchanged.
Tests drive a SECOND retain in every case, because the defect only appears once a
cursor exists — on a fresh session start_index == 0 and the old expression
evaluates True by accident. Reverting the fix fails 3 of the 5 new tests; 206
pass with it.
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.
The invariant that breaks
Full-session retention selects the unprocessed transcript suffix using the persisted offset, then narrows it a second time:
retain_full_window=Falsemakesprepare_retention_transcript()keep only the last turn. Butcommit_retention()advances the cursor to the end of the whole suffix either way:So the rule that should hold —
— is violated: messages are dropped by a second positional window while the cursor records them as processed. That loss is permanent.
This holds on its own merits. Claude Code's transcript shape then makes it dramatically worse.
Why it's severe in Claude Code specifically
Tool results are stored as
role: "user"messages. So thelast_user_idxscan usually anchors on a tool result rather than the human prompt. With the resolved defaultretainToolCalls: false(bothconfig.pyDEFAULTSandsettings.jsonsetFalse; theTruefallback atretain.py:149is unreachable becauseload_config()always supplies the key), that message extracts to empty —prepare_retention_transcriptreturns(None, 0), and the entire retain is skipped while the cursor still advances.In an agentic session the trailing message is a tool result most of the time, so this is the common case, not an edge case.
Measurements
Formatting the same 30-message unprocessed suffix from a real 1,500-message transcript:
One session's actual retain history:
The first retain (
start_index == 0) captures everything; every later one captures a fragment, regardless of how much work happened in between.Fix
messages_to_retainis already the not-yet-retained slice, so windowing it again is redundant at best and lossy at worst. There's no duplicate-send risk — the cursor, not the window, is what prevents that. Role, memory-tag and tool-content filtering are unchanged.This also restores the behaviour the surrounding comments already describe (
retain.py:108-115): "each retain only sends messages not retained by a previous Stop hook."The
commit_retention()on an empty transcript is deliberately left alone. With this fix, "empty" can only mean the entire unprocessed suffix was filtered out by policy, so advancing is correct — and not committing would make every later Stop reprocess the same suffix forever.Tests
5 new tests in
tests/test_retain_incremental_window.py. Every one drives a second retain, because the defect only appears once a cursor exists — on a fresh sessionstart_index == 0and the old expression evaluatesTrueby accident. My first draft omitted this and passed against the unfixed code; mutation testing caught it.Verification:
ruff checkcleanDocstring
retain_full_windowwas documented as "retain all messages (chunked mode)", which invited exactly this misuse. Reworded to say it means the caller has already selected the complete window, and to warn that callers advancing a retention cursor must not passFalse.Related, not included
slice_last_turns_by_user_boundary()counts everyrole == "user"message as a turn boundary, so it has the same tool-result confusion. It affects chunked mode (retain.py:120) and recall context composition (content.py:81), sorecallContextTurns: Ncan compose N tool results instead of N turns. That's a wider semantic change with a shared call site, so it deserves its own PR — happy to open one.Same family as #2869 (SessionEnd retain cancelled at shutdown) and #2974 (retain never sent
MemoryItem.timestamp).