.NET: Fix compaction updates with repeated todo messages - #8838
Draft
JS (Punisheroot) wants to merge 4 commits into
Draft
JS (Punisheroot) wants to merge 4 commits into
JS (Punisheroot) wants to merge 4 commits into
Conversation
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Input summary messages are excluded from RawMessageCount, allowing the reported message-loss behavior to persist.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes repeated todo messages causing new user messages to be omitted during compaction updates.
Changes:
- Uses the previously indexed message count as the append boundary.
- Adds regression coverage for repeated todo messages and generated summaries.
| File | Description |
|---|---|
CompactionMessageIndex.cs |
Updates incremental append detection. |
CompactionMessageIndexTests.cs |
Adds index regression tests. |
CompactionProviderTests.cs |
Adds provider-level regression coverage. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Chat-reducer group replacement leaves the new persisted prefix metadata inconsistent, causing reduced history to be rebuilt on subsequent turns.
Review effort: Balanced
Findings: 1
Resolved since last review (1)
Comment on lines
184
to
+187
| state.MessageGroups.Clear(); | ||
| state.MessageGroups.AddRange(messageIndex.Groups); | ||
| state.ProcessedInputMessageCount = messageIndex.ProcessedInputMessageCount; | ||
| state.InputSummaryGroupIndices = messageIndex.InputSummaryGroupIndices; |
This branch was successfully deployed
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.
Motivation & Context
When CompactionProvider and TodoProvider are used together, TodoProvider can append the same empty todo-list message on consecutive turns. CompactionMessageIndex.Update() treated a matching final message as evidence that nothing had changed, so new user messages could be omitted from the context sent to the model.
Description & Review Guide
RawMessageCountand record which summary groups came from input history. Before appending, compare the represented input prefix while skipping only summaries inserted by compaction strategies; rebuild when the prefix changes even if the saved boundary still contains the same todo message. Older state without summary provenance is rebuilt once when summaries make the prefix ambiguous. Add index and provider regression tests for repeated todo messages, summary provenance, replaced or shifted history, legacy state, and serialized sessions.Related Issue
Fixes #7395
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after a language prefix) - a workflow keeps the label and title prefix in sync automatically.