Python: Fix extract_range reordering messages when preserving function call/result pairs#14165
Open
ErenAta16 wants to merge 2 commits into
Open
Python: Fix extract_range reordering messages when preserving function call/result pairs#14165ErenAta16 wants to merge 2 commits into
ErenAta16 wants to merge 2 commits into
Conversation
…n call/result pairs Problem: extract_range() in chat_history_reducer_utils.py is used by ChatHistorySummarizationReducer (when include_function_content_in_summary=True) to pull the slice of history that gets sent to the LLM for summarization, while making sure a function call and its result are never split apart. When a function-call/result pair had other messages interleaved between them, extract_range silently moved the result message to sit right after the call, corrupting the chronological order of the messages handed to the summarizer. Example: history = [user, assistant(call), user, tool(result), assistant] extract_range(..., preserve_pairs=True) returned [user, assistant(call), tool(result), user, assistant] instead of the original order. The "unrelated" user message got pushed past the tool result. Root cause: The old implementation walked a mutable `sliced` index list with a manual cursor `i`. When it hit the first half of a pair, it appended both messages to the output immediately and called `sliced.remove(paired_idx)` to avoid re-processing the second half later. That is what actually moved the second message: it got appended out of turn instead of being left in its original position. Fix: Rewrote extract_range to make a single forward pass over range(start, end) and decide, for each index, whether to keep or skip it without ever moving a message out of its natural position. For pairs fully contained in the slice, keep/skip is still decided together (unchanged behavior), but each message is only appended when its own index comes up in the iteration, so order is preserved. Pairs that are only partially in range keep the old "never orphan" behavior of always keeping the in-range half. Testing: - Wrote a standalone repro script that builds a 5-message history with a call/result pair separated by an unrelated user message, called extract_range(..., preserve_pairs=True), and compared the output order to the input order. Confirmed it fails on the original code (the result moves ahead of the interleaved user message) and passes after the fix. - Confirmed via git stash / stash pop that the bug reproduces on the unmodified code and is resolved after applying the fix. - Added test_extract_range_preserve_pairs_keeps_chronological_order to tests/unit/contents/test_chat_history_reducer_utils.py, reusing the existing chat_messages_with_pairs fixture (its call2/result2 pair already has two unrelated messages between them), asserting extracted equals the original slice rather than just set equality like the existing tests do. - Ran tests/unit/contents/test_chat_history_reducer_utils.py (11 passed), test_chat_history_summarization_reducer.py and test_chat_history_truncation_reducer.py (32 passed), and the full tests/unit/contents/ suite (385 passed, no new warnings). - Ran ruff check and ruff format --check on both changed files.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes extract_range() (used by Python chat history reducers) so that enabling preserve_pairs=True no longer reorders messages when a function call/result pair has unrelated messages interleaved between them, ensuring the summarization input remains chronologically correct.
Changes:
- Rewrote
extract_range()to iterate forward over the requested slice and decide keep/skip per index without relocating paired messages. - Added a regression test asserting that
preserve_pairs=Truepreserves the original message order for an interleaved call/result pair.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py | Refactors extract_range() to preserve chronological order while still coordinating keep/skip decisions for fully in-range call/result pairs. |
| python/tests/unit/contents/test_chat_history_reducer_utils.py | Adds a regression test to ensure extract_range(..., preserve_pairs=True) does not reorder messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+197
to
+200
| # The paired message falls outside [start, end): keep this one unconditionally | ||
| # so a function call is never separated from its result (or vice versa). | ||
| extracted.append(msg) | ||
| i += 1 | ||
| continue |
Author
There was a problem hiding this comment.
Confirmed, this was a real regression. The original implementation checked filter_func unconditionally at the top of the loop before any pairing logic ran, so it always applied to this case too. My rewrite added an unconditional append for the partner-out-of-range branch and lost that check. Fixed in 5d5e2a1, added a regression test covering it, and verified all 12 tests in the file still pass.
The chronological-order fix in the previous commit accidentally dropped filter_func handling for the case where preserve_pairs=True and a message's paired call/result falls outside [start, end). The original implementation always checked filter_func first regardless of pairing; the rewrite added an unconditional append for this specific branch, silently ignoring filter_func for it. Since the partner isn't part of the output either way when it's out of range, there's no pair-coherence decision to make here, so the message should go through filter_func like any unpaired one. Caught by Copilot's automated review on the PR.
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 and Context
extract_range()inchat_history_reducer_utils.pyis whatChatHistorySummarizationReduceruses (wheninclude_function_content_in_summary=True) to pull the slice of chat history that gets sent to the LLM for summarization, while making sure a function call is never separated from its result.When a function-call/result pair had other messages interleaved between them,
extract_rangesilently moved the result message to sit right after the call, corrupting the chronological order of the messages handed to the summarizer. For example:The old implementation walked a mutable
slicedindex list with a manual cursor. When it hit the first half of a pair, it appended both messages to the output immediately and removed the paired index fromslicedso it wouldn't be reprocessed later. That is what actually moved the second message out of its original position.Description
extract_rangeto do a single forward pass overrange(start, end)and decide, per index, whether to keep or skip the message without ever moving it out of its natural position.test_extract_range_preserve_pairs_keeps_chronological_ordertotests/unit/contents/test_chat_history_reducer_utils.py, reusing the existingchat_messages_with_pairsfixture (its call2/result2 pair already has two unrelated messages between them) and asserting the extracted list equals the original slice, not just the same set of messages.Verified with a standalone repro against the unmodified code (via
git stash) that the bug reproduces, and that it's fixed after applying the patch. Rantests/unit/contents/test_chat_history_reducer_utils.py,test_chat_history_summarization_reducer.py,test_chat_history_truncation_reducer.py, and the fulltests/unit/contents/suite — all passing. Also ranruff checkandruff format --checkon both changed files.Contribution Checklist