Python: Fix compaction message-id collisions and tool-loop summary persistence#6299
Conversation
Fixes two bugs in the compaction strategies: - microsoft#5237: incremental group annotation assigned message ids by position within the re-annotated slice, so moving the re-annotation start back to a previous group start restarted ids at 0 and produced collisions (e.g. a user message reusing an assistant message's id), merging groups and causing tool-result compaction to wrongly exclude messages. group_messages/_ensure_message_ids now take an id_offset and guard against existing-id collisions; annotate_message_groups threads the slice start index through as the offset. - microsoft#4991: the function-invocation loop copied the message list each iteration, so summaries inserted by compaction landed in a throwaway copy and were lost across tool-loop iterations (only the persistent excluded flags survived). _prepare_messages_for_model_call now compacts the list in place when messages is a list, so inserted summaries persist. Adds regression tests (incremental id uniqueness, existing-id collision avoidance, idempotency, and tool-loop summary persistence including streaming and conversation-id modes). Also adds a summarization.py sample demonstrating SummarizationStrategy directly with a real client, and reworks advanced.py with tool-call groups and a real summarizer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Fixes two Python compaction bugs that could corrupt group tracking: (1) message-id collisions during incremental group annotation, and (2) loss of compaction-inserted summary messages across tool-loop iterations due to operating on throwaway message-list copies.
Changes:
- Make auto-assigned
Message.message_idgeneration stable for incremental annotation by threading anid_offsetthrough grouping/id assignment. - Compact message lists in place (when the input is a
list) so inserted summaries persist across function-invocation loop iterations. - Add regression tests for id uniqueness/collision avoidance and tool-loop/compaction idempotency; update compaction samples (new summarization sample, enhanced advanced sample).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/packages/core/agent_framework/_compaction.py | Adds id_offset support to prevent incremental re-annotation id collisions; wires offset through annotation. |
| python/packages/core/agent_framework/_clients.py | Applies compaction against the caller’s list when possible so inserted summaries persist through tool loops. |
| python/packages/core/tests/core/test_compaction.py | Adds regression tests for incremental id uniqueness, collision avoidance, and tool-result compaction idempotency. |
| python/packages/core/tests/core/test_clients.py | Adds regression tests ensuring summaries persist across tool-loop iterations (streaming/non-streaming) and respects conversation-id mode behavior. |
| python/samples/02-agents/compaction/summarization.py | New sample demonstrating SummarizationStrategy directly with a real summarizing client. |
| python/samples/02-agents/compaction/advanced.py | Reworks sample to include tool-call groups and a real summarizer under a composed token-budget strategy. |
| python/samples/02-agents/compaction/README.md | Updates documentation to include the new/updated samples and their run commands. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 92%
✓ Correctness
The PR correctly fixes both reported bugs: (1) message-id collisions in incremental annotation by threading an absolute offset through
_ensure_message_ids, and (2) tool-loop summary persistence by compacting the caller's list in-place when it's already a list. The logic is sound, regression tests are well-designed, and the idempotency/conversation-id edge cases are properly handled. No blocking correctness issues found.
✓ Security Reliability
This PR fixes two compaction bugs (message-id collisions and tool-loop summary persistence) with well-tested, sound implementations. The collision avoidance in
_ensure_message_idsis correctly bounded (the while-loop terminates becauseexisting_idsis finite and the counter is monotonically increasing). The in-place mutation design for_prepare_messages_for_model_callis explicitly documented and validated by regression tests. No security vulnerabilities, resource leaks, injection risks, or unhandled failure modes were found.
✓ Test Coverage
The PR adds comprehensive regression tests for both bugs fixed. The id-collision fix (#5237) has two unit tests covering incremental uniqueness and existing-id avoidance. The tool-loop summary persistence fix (#4991) has three integration tests covering non-streaming, streaming, and conversation-id modes, plus a unit test verifying idempotency after summary insertion. Test assertions are meaningful (checking actual summary content and uniqueness, not just no-exception). No significant test coverage gaps were found for the changed production behavior.
✗ Design Approach
I found one design issue in the message-id collision fix: it only guards against ids already present in the re-annotated slice, so incremental append/suffix re-annotation can still collide with
msg_Nids that already exist in the preserved prefix and recreate the group-merge problem the PR is trying to eliminate.
Flagged Issues
-
_ensure_message_idsonly checks for collisions inside the suffix being re-annotated, but existing incremental callers such asextend_compaction_messages(python/packages/core/agent_framework/_compaction.py:523-529) annotate only the appended tail. If the preserved prefix already contains a user-supplied id likemsg_2, appending at index 2 will still assignmsg_2to the new message, and_group_id_forderives group ids frommessage_id(_compaction.py:101-104), so the old and new groups collapse together again.
Automated review by eavanvalkenburg's agents
Addresses PR review on microsoft#5237: _ensure_message_ids only guarded against collisions within the re-annotated slice. A preexisting (e.g. user-supplied) id in the preserved prefix could still be reassigned in the suffix when the id was numerically out of position, merging groups across the re-annotation boundary again. group_messages/_ensure_message_ids now accept reserved_ids, and annotate_message_groups passes the preserved prefix's ids so auto-assigned suffix ids never collide across the full list. Adds a regression test reproducing the out-of-position prefix-id collision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Motivation and Context
Two bugs in the compaction strategies produced incorrect behavior:
_excludedflags survived), leaving groups dropped without their replacement summary.Description
group_messages/_ensure_message_idsnow accept anid_offsetand guard against collisions with existing ids;annotate_message_groupsthreads the slice start index through as the offset, so incremental annotation produces stable, unique ids._prepare_messages_for_model_callnow compacts the message list in place whenmessagesis a list, so summaries inserted during compaction persist across tool-loop iterations alongside the already-persistent excluded flags.summarization.pydemonstratingSummarizationStrategydirectly with a real summarizing client, and reworkedadvanced.pywith tool-call groups, a real summarizer, and a composed token-budget strategy.Contribution Checklist