From 0c075a55b7f3038d1622c388d309ba8226b13d47 Mon Sep 17 00:00:00 2001 From: ErenAta16 Date: Sat, 18 Jul 2026 03:34:11 +0300 Subject: [PATCH] python: fix extract_range reordering messages when preserving function 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. --- .../chat_history_reducer_utils.py | 65 ++++++------------- .../test_chat_history_reducer_utils.py | 23 +++++++ 2 files changed, 42 insertions(+), 46 deletions(-) diff --git a/python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py b/python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py index a403cb21570a..7e254a75e1bf 100644 --- a/python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py +++ b/python/semantic_kernel/contents/history_reducer/chat_history_reducer_utils.py @@ -166,71 +166,44 @@ def extract_range( if end is None: end = len(history) - sliced = list(range(start, end)) - # If we need to preserve call->result pairs, gather them - pair_map = {} + pair_map: dict[int, int] = {} if preserve_pairs: - pairs = get_call_result_pairs(history) - # store in a dict for quick membership checking - # call_idx -> result_idx, and also result_idx -> call_idx - for cidx, ridx in pairs: + for cidx, ridx in get_call_result_pairs(history): pair_map[cidx] = ridx pair_map[ridx] = cidx + # Walk the history once, in order, and decide per-message whether to keep it. + # Deciding keep/skip without ever moving a message out of its original position + # is what keeps the extracted list in chronological order, even when a + # function-call/result pair has other messages interleaved between them. extracted: list[ChatMessageContent] = [] - i = 0 - while i < len(sliced): - idx = sliced[i] + for idx in range(start, end): msg = history[idx] - # If filter_func excludes it, skip it - if filter_func and filter_func(msg): - i += 1 - continue - # skipping system/developer message if msg.role in (AuthorRole.DEVELOPER, AuthorRole.SYSTEM): - i += 1 continue - # If preserve_pairs is on, and there's a paired index, skip or include them both if preserve_pairs and idx in pair_map: paired_idx = pair_map[idx] - # If the pair is within [start, end), we must keep or skip them together if start <= paired_idx < end: - # Check if the pair or itself fails filter_func - if filter_func and (filter_func(history[paired_idx]) or filter_func(msg)): - # skip both - i += 1 - # Also skip the paired index if it's in our current slice - if paired_idx in sliced: - # remove it from the slice so we don't process it again - sliced.remove(paired_idx) + # The pair is fully within [start, end): keep or skip both together, + # but leave each message where it already is in the sequence. + if filter_func and (filter_func(msg) or filter_func(history[paired_idx])): continue - # keep both extracted.append(msg) - if paired_idx > idx: - # We'll skip the pair in the normal iteration by removing from slice - # but add it to extracted right now - extracted.append(history[paired_idx]) - if paired_idx in sliced: - sliced.remove(paired_idx) - else: - # if paired_idx < idx, it might appear later, so skip for now - # but we may have already processed it if i was the 2nd item - # either way, do not add duplicates - pass - i += 1 continue - # If the paired_idx is outside [start, end), there's no conflict - # so we can just do normal logic - extracted.append(msg) - i += 1 - else: - # keep it if filter_func not triggered + # 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 + + # If filter_func excludes it, skip it + if filter_func and filter_func(msg): + continue + + extracted.append(msg) return extracted diff --git a/python/tests/unit/contents/test_chat_history_reducer_utils.py b/python/tests/unit/contents/test_chat_history_reducer_utils.py index d46340915b8a..7e91e9f3c171 100644 --- a/python/tests/unit/contents/test_chat_history_reducer_utils.py +++ b/python/tests/unit/contents/test_chat_history_reducer_utils.py @@ -138,6 +138,29 @@ def test_extract_range_preserve_pairs_call_outside_slice(chat_messages_with_pair # (2,3) do not appear, and that's correct since they're outside this slice. +def test_extract_range_preserve_pairs_keeps_chronological_order(chat_messages_with_pairs): + """ + Regression test: extract_range with preserve_pairs=True must not reorder + messages relative to the original history. + + In the fixture, the (call2, result2) pair sits at indices (5, 8), with an + unrelated user message (6) and an unrelated function result (7) interleaved + between them. Before the fix, extract_range moved the paired result right + after the call as soon as it encountered the call, producing the order + [2, 3, 4, 5, 8, 6, 7] instead of [2, 3, 4, 5, 6, 7, 8]. That silently + scrambled the chronological order of messages handed to summarization. + """ + extracted = extract_range( + chat_messages_with_pairs, + start=2, + end=9, + preserve_pairs=True, + ) + + expected_order = chat_messages_with_pairs[2:9] + assert extracted == expected_order, "extract_range must preserve the original message order" + + def test_locate_summarization_boundary_empty(): # Edge case: empty history => boundary = 0 empty_history = []