Keep retained history Source when compaction runs across turns - #934
Conversation
markGeneratedMessages told provider-generated messages apart from original
input by *message.Message pointer identity. That only holds on the first,
session-less turn. With a session the index is rebuilt from persisted
state.MessageGroups (or a deserialized session), whose message pointers
differ from this turn's input, so every retained history message failed the
identity check and was stamped Source={context-provider, sourceID}.
Source.Type is load-bearing - the default history provider's store filter
and context-provider attribution key on it - so this relabeled genuine
user/assistant history as provider-generated on every multi-turn run.
Match this turn's input by content instead of pointer identity, so retained
prior-turn history keeps its original Source and only the summary messages
compaction actually generates are stamped.
github-actions
Bot
added
area:agent
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect Source attribution for retained history messages when the compaction provider runs across multiple turns with a session (where message pointers are reconstructed from persisted state and no longer match the current turn’s input pointers).
Changes:
- Update
markGeneratedMessagesto identify “provider-generated” messages by content equality rather than pointer identity. - Add a helper (
containsMessageByContent) to support content-based membership checks. - Add a regression test ensuring retained prior-turn history keeps its original
Sourceacross turns in a single session.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
agent/compaction/provider.go |
Switch source-stamping logic from pointer-identity to content-based matching to avoid relabeling retained history across turns. |
agent/compaction/compaction_test.go |
Add a multi-turn session test asserting retained history messages are not mislabeled as context-provider generated. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
github-actions
Bot
added
risk:medium
Quim Muntal (qmuntal)
left a comment
Quim Muntal (qmuntal)
left a comment
There was a problem hiding this comment.
This is not fully at parity with .NET yet. The content-equality check matches .NET, but .NET also marks messages restored from compaction state as message history before calling Update.
Without that step, retained Go messages keep their stale source, often the zero source from the first turn. The default history filter then stores them again. I reproduced this on the PR head: after two turns, stored history was [u1 a1 u1 u2 a2].
Could we mirror the .NET pre-update history attribution and extend the test through the history-provider lifecycle? It should verify that retained messages are history-attributed, new messages are not, and stored history contains no duplicates.
Match .NET's CompactionProvider: when the message index is rebuilt from persisted state, stamp every restored message with the history source before folding in this turn's input, and skip already-history messages in markGeneratedMessages. Together with the content-equality check this keeps genuine prior-turn history attributed as chat history (not context-provider generated) and stops it being re-stored as new messages at the end of a run.
|
Addressed — now at parity with the .NET
Net effect: genuine prior-turn history is attributed as chat history (not context-provider generated) and isn't re-stored at the end of the run. The regression test now asserts the retained messages carry the history source. |
github-actions
Bot
added
pending-auto-risk
Go API Consistency ReviewScope: user-visible behavior (bug fix), internal-only (no exported API change) Changed Go contract: None — no exported identifiers added, removed, or changed. The fix modifies the unexported Upstream evidence reviewed: No equivalent compaction provider was found in Result: No parity issues. This is a self-contained bug fix to Go-internal implementation details. The corrected behavior (retained history messages keep their original
|
github-actions
Bot
added
risk:medium
Problem
markGeneratedMessages(inagent/compaction/provider.go) tells provider-generated messages apart from the caller's original input by*message.Messagepointer identity. That invariant only holds on the first, session-less turn.With a session, on every subsequent turn the index is rebuilt from persisted
state.MessageGroups(NewMessageIndex(...)+index.Update(messages)) — or from a deserialized session — whose retained-history messages are different pointers from this turn's incomingmessages. Each retained history message therefore fails the identity check and is stampedSource = {SourceTypeContextProvider, sourceID}.Source.Typeis load-bearing: the default history provider's store filter (notSourceTypes(SourceTypeHistoryProvider)) and context-provider attribution key on it, so this relabels genuine user/assistant history as provider-generated on every multi-turn run (and after any session serialize/deserialize round-trip).Fix
Identify provider-generated messages by comparing against this turn's input by content (
messageContentEqual) rather than by pointer identity. Retained prior-turn history content-matches the incoming messages and keeps its originalSource; only the summary messages compaction actually generates (which never match an input message) get stamped.Test
TestNewProvider_KeepsRetainedHistorySourceAcrossTurnsdrives two turns against one session and asserts the retained first-turn messages keep their originalSource. Fails before the fix (prior-turn messages stampedcontext-provider), passes after. Existing source-stamping tests stay green.