From 67a730308fa2ca5962a3c6e0ebc8e94eab150f4a Mon Sep 17 00:00:00 2001 From: ErenAta16 Date: Wed, 29 Jul 2026 18:15:44 +0300 Subject: [PATCH 1/2] Python: fix paragraph merge comparing word count to token budget _split_text_paragraph folds a short trailing paragraph back into the one before it. The guard for that merge counted words on both paragraphs (len(text.split(" "))) and compared the sum to max_tokens, which is measured with token_counter. Those are different units, so with few long words the guard passed and produced a paragraph over the limit. With the default counter (len(text) // 4) and max_tokens=20, the input ["a" * 80, "b" * 16] merged 20 + 4 tokens into a single 24-token paragraph, because the check saw 1 + 1 = 2 words. Build the merged paragraph first and check it with token_counter. The split/join round-trip the old code did was an identity for single-space splits, so dropping it changes nothing else. --- python/semantic_kernel/text/text_chunker.py | 15 ++++++-------- python/tests/unit/text/test_text_chunker.py | 23 +++++++++++++++++++++ 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/python/semantic_kernel/text/text_chunker.py b/python/semantic_kernel/text/text_chunker.py index f79796399b6d..c730bb2a62cb 100644 --- a/python/semantic_kernel/text/text_chunker.py +++ b/python/semantic_kernel/text/text_chunker.py @@ -137,15 +137,12 @@ def _split_text_paragraph(text: list[str], max_tokens: int, token_counter: Calla sec_last_para = paragraphs[-2] if token_counter(last_para) < max_tokens / 4: - last_para_tokens = last_para.split(" ") - sec_last_para_tokens = sec_last_para.split(" ") - last_para_token_count = len(last_para_tokens) - sec_last_para_token_count = len(sec_last_para_tokens) - - if last_para_token_count + sec_last_para_token_count <= max_tokens: - sec_last_para = " ".join(sec_last_para_tokens) + NEWLINE - last_para = " ".join(last_para_tokens) - new_sec_last_para = sec_last_para + last_para + # Compare against `token_counter`, not `len(text.split(" "))`. Word counts and token + # counts are different units, so the old check could pass while the merged paragraph + # was over `max_tokens` -- exactly the limit this function exists to keep. + new_sec_last_para = f"{sec_last_para}{NEWLINE}{last_para}" + + if token_counter(new_sec_last_para) <= max_tokens: paragraphs[-2] = new_sec_last_para.strip() paragraphs.pop() diff --git a/python/tests/unit/text/test_text_chunker.py b/python/tests/unit/text/test_text_chunker.py index 648f84b752c5..5ade4dd9404f 100644 --- a/python/tests/unit/text/test_text_chunker.py +++ b/python/tests/unit/text/test_text_chunker.py @@ -8,6 +8,7 @@ split_plaintext_lines, split_plaintext_paragraph, ) +from semantic_kernel.text.text_chunker import _split_text_paragraph, _token_counter NEWLINE = os.linesep @@ -532,3 +533,25 @@ def test_split_md_on_newlines(): max_token_per_line = 15 split = split_markdown_paragraph(test, max_token_per_line) assert expected == split + + +def test_short_last_paragraph_merge_respects_max_tokens(): + """Folding the last paragraph back must not push it over `max_tokens`.""" + max_tokens = 20 + # The default counter is len(text) // 4, so these are 20 and 4 tokens but one word each. + lines = ["a" * 80, "b" * 16] + + paragraphs = _split_text_paragraph(lines, max_tokens) + + assert all(_token_counter(p) <= max_tokens for p in paragraphs), [_token_counter(p) for p in paragraphs] + + +def test_short_last_paragraph_still_merges_when_it_fits(): + """A trailing paragraph that does fit is still folded back.""" + max_tokens = 20 + lines = ["a" * 20, "b" * 8] + + paragraphs = _split_text_paragraph(lines, max_tokens) + + assert len(paragraphs) == 1 + assert _token_counter(paragraphs[0]) <= max_tokens From 59bebfcbaed629c56703608f937600fd91251486 Mon Sep 17 00:00:00 2001 From: ErenAta16 <149434812+ErenAta16@users.noreply.github.com> Date: Sat, 15 Aug 2026 21:49:17 +0300 Subject: [PATCH 2/2] Python: keep the paragraph merge decision independent of os.linesep Measuring the joined paragraph with `token_counter` made the merge depend on the host line ending. NEWLINE is `os.linesep`, two characters on Windows and one elsewhere, and `_token_counter` is `len(text) // 4`, so a paragraph sitting on the limit merges on Linux and does not on Windows. Eight existing chunker tests encode the merged form and fail on Windows without this. Normalise the line endings for the comparison, the way `_split_str_lines` already normalises its input. The emitted text still uses NEWLINE. Also fixes the fixture in test_short_last_paragraph_still_merges_when_it_fits: 5 and 2 tokens never split into two paragraphs, so the tail-merge block it was meant to cover never ran. --- python/semantic_kernel/text/text_chunker.py | 7 +++++- python/tests/unit/text/test_text_chunker.py | 28 +++++++++++++++++++-- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/python/semantic_kernel/text/text_chunker.py b/python/semantic_kernel/text/text_chunker.py index c730bb2a62cb..4f10f5ff7a23 100644 --- a/python/semantic_kernel/text/text_chunker.py +++ b/python/semantic_kernel/text/text_chunker.py @@ -142,7 +142,12 @@ def _split_text_paragraph(text: list[str], max_tokens: int, token_counter: Calla # was over `max_tokens` -- exactly the limit this function exists to keep. new_sec_last_para = f"{sec_last_para}{NEWLINE}{last_para}" - if token_counter(new_sec_last_para) <= max_tokens: + # Measure with newlines normalised, the way `_split_str_lines` already normalises its + # input. NEWLINE is `os.linesep`, two characters on Windows and one everywhere else, + # and a paragraph that has absorbed several lines carries several of them. With a + # length-based counter that is enough to move the total across the limit, so the same + # input would chunk differently per platform. + if token_counter(new_sec_last_para.replace("\r\n", "\n")) <= max_tokens: paragraphs[-2] = new_sec_last_para.strip() paragraphs.pop() diff --git a/python/tests/unit/text/test_text_chunker.py b/python/tests/unit/text/test_text_chunker.py index 5ade4dd9404f..a6bdf04f0924 100644 --- a/python/tests/unit/text/test_text_chunker.py +++ b/python/tests/unit/text/test_text_chunker.py @@ -547,11 +547,35 @@ def test_short_last_paragraph_merge_respects_max_tokens(): def test_short_last_paragraph_still_merges_when_it_fits(): - """A trailing paragraph that does fit is still folded back.""" + """A trailing paragraph that does fit is still folded back. + + The lengths matter. The splitting loop measures the paragraph it has accumulated *including* + the newline it appended, so 58 characters count as 15 tokens rather than 14, and 15 + 4 + 1 + reaches `max_tokens` and starts a second paragraph. The tail merge then folds them back. + Shorter lines never split in the first place, which leaves one paragraph and skips the merge + block entirely, so the test would pass without exercising anything. + """ max_tokens = 20 - lines = ["a" * 20, "b" * 8] + lines = ["a" * 58, "b" * 16] paragraphs = _split_text_paragraph(lines, max_tokens) assert len(paragraphs) == 1 assert _token_counter(paragraphs[0]) <= max_tokens + + +def test_short_last_paragraph_merge_is_not_platform_dependent(): + """The merge decision must not depend on `os.linesep`. + + The candidate paragraph is joined with NEWLINE, which is two characters on Windows and one + elsewhere. Measuring that joined string with a length-based counter let the same input chunk + differently per platform, so the separator is normalised for the comparison. + """ + max_tokens = 15 + lines = ["Seriously, this is the end. We're finished. All set. Bye.", "Done."] + + paragraphs = _split_text_paragraph(lines, max_tokens) + + # 57 chars + 5 chars: 15 tokens joined by "\n", 16 joined by "\r\n". The verdict is the same + # on both, and matches what the surrounding chunker tests have always expected. + assert len(paragraphs) == 1