Repository navigation
Python: Fix extract_range reordering messages when preserving function call/result pairs - #14165
ErenAta16 (ErenAta16) wants to merge 5 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.
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.
| # 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 |
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.
Roger Barreto (rogerbarreto)
added
the
python
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
PRABHU KIRAN VANDRANKI (VANDRANKI)
left a comment
There was a problem hiding this comment.
This is a community review, not a maintainer approval, so it does not clear the merge gate.
I traced the rewritten extract_range in chat_history_reducer_utils.py against the diff and I think the fix is correct.
The core change: instead of eagerly appending the paired message and mutating sliced/i when the loop hits the earlier half of a pair, the new version does one for idx in range(start, end) pass and lets each message get appended only when its own index comes up naturally. Since a call's index is always less than its result's index, the loop always reaches the call first and just appends it, then reaches the result later at its own natural position and appends that too. Nothing is ever pulled forward, so order is preserved automatically without any special-casing. That is a nice simplification over the old "insert the partner immediately, then delete it from sliced so it isn't revisited" approach.
I checked the new regression test test_extract_range_preserve_pairs_keeps_chronological_order against the chat_messages_with_pairs fixture in the same file: the (call2, result2) pair really does sit at indices (5, 8) with the unrelated message at 6 and the unrelated result at 7 in between, so the test exercises the exact interleaving scenario described in the PR. Good that it asserts list equality (extracted == expected_order) rather than the pre-existing test_extract_range_preserve_pairs's set(extracted) == set(expected_slice), since the set comparison is exactly what let the ordering bug through unnoticed before.
I also traced test_extract_range_preserve_pairs_honors_filter_func_when_partner_out_of_range by hand against the fixture (start=3, end=8) and the assertions hold given the rewritten "paired_idx outside range -> apply filter_func to this message alone" branch.
One thing worth flagging for the maintainers, not a blocker on this PR: pair_map here still does pair_map[cidx] = ridx (single int, last-write-wins) rather than tracking every result index for a call. That is a real bug for parallel function calls in the same assistant message, but it is a separate, pre-existing issue from the one this PR fixes, now tracked as #14486 links back to this PR, and #14485 / PR #14488 is a competing fix for that specific collision. Since this PR only touches the ordering logic and inherits the existing pair_map construction unchanged, I don't think it needs to solve that here, just flagging so whichever of this PR or #14488 lands second rebases cleanly, as #14488's own description already notes.
gh pr checks shows CLA and label checks passing, no CI failure. The PR does show as BEHIND the base branch right now, so a rebase will likely be needed before merge either way.
Nice fix, this has been sitting since 2026-07-18 with no linked issue, which is probably why it stalled. #14486 now gives it one.
GitHub Actions
Failure
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