Skip to content

Keep retained history Source when compaction runs across turns - #934

Open
PratikDhanave (PratikDhanave) wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:fix-compaction-source-mislabel-across-turns
Open

Keep retained history Source when compaction runs across turns#934
PratikDhanave (PratikDhanave) wants to merge 1 commit into
microsoft:mainfrom
PratikDhanaveFork:fix-compaction-source-mislabel-across-turns

Conversation

@PratikDhanave

Copy link
Copy Markdown
Contributor

Problem

markGeneratedMessages (in agent/compaction/provider.go) tells provider-generated messages apart from the caller's original input by *message.Message pointer 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 incoming messages. Each retained history message therefore fails the identity check and is stamped Source = {SourceTypeContextProvider, sourceID}.

Source.Type is 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 original Source; only the summary messages compaction actually generates (which never match an input message) get stamped.

Test

TestNewProvider_KeepsRetainedHistorySourceAcrossTurns drives two turns against one session and asserts the retained first-turn messages keep their original Source. Fails before the fix (prior-turn messages stamped context-provider), passes after. Existing source-stamping tests stay green.

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.
Copilot AI lite review requested due to automatic review settings August 28, 2026 05:39
@PratikDhanave
PratikDhanave (PratikDhanave) requested a review from a team as a code owner August 28, 2026 05:39
@github-actions github-actions Bot added area:agent Changes files in the agent area size:medium At most 100 changed lines across at most 5 files pending-auto-risk Automatic risk classification is in progress parity-approved Go API consistency review found no parity issues labels Aug 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Parity Review — PR #934: Keep retained history Source when compaction runs across turns

Scope: internal-only (unexported implementation fix in agent/compaction/provider.go; no exported API changed)
Changed Go contract: None — only the private markGeneratedMessages helper changes its comparison strategy from pointer identity to content equality
Upstream evidence reviewed:

  • dotnet/src/Microsoft.Agents.AI/Compaction/ChatMessageContentEquality.csContentEquals extension method
  • dotnet/src/Microsoft.Agents.AI/Compaction/CompactionProvider.cs — uses !messageList.Any(x => x.ContentEquals(message)) to identify provider-generated messages
    Result: ✅ Aligned — no parity issues found

Analysis

The Go fix adopts content-based message identity (messageContentEqual) for markGeneratedMessages, precisely mirroring the upstream .NET CompactionProvider which uses ChatMessageContentEquality.ContentEquals for the same purpose. Both implementations compare Role, AuthorName, and content items rather than relying on pointer/reference identity.

No exported Go symbols were added, removed, or changed. The ContextProviderConfig, NewContextProvider, and Strategy types remain unchanged. This fix restores correct cross-turn behavior that was already the documented semantic intent in both Go and .NET.

The public-api-change label was not added because no exported Go API changed.

Generated by Go API Consistency Review Agent · sonnet46 · 29 AIC · ⌖ 5.01 AIC · ⊞ 6.4K ·

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 markGeneratedMessages to 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 Source across 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.

Comment thread agent/compaction/provider.go
@github-actions github-actions Bot added risk:medium Contained production impact requiring normal review depth and removed pending-auto-risk Automatic risk classification is in progress labels Aug 28, 2026

@qmuntal Quim Muntal (qmuntal) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:agent Changes files in the agent area parity-approved Go API consistency review found no parity issues risk:medium Contained production impact requiring normal review depth size:medium At most 100 changed lines across at most 5 files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants