Validate the computed chunk_overlap (a float ≥ 1.0 silently drops all content)#604
Validate the computed chunk_overlap (a float ≥ 1.0 silently drops all content)#604aaravanmay wants to merge 1 commit into
Conversation
…drop all content TokenChunker.__init__ only checked `chunk_overlap >= chunk_size` for int inputs (isinstance(chunk_overlap, int)). A float chunk_overlap >= 1.0 is converted to int(chunk_overlap * chunk_size), which can be >= chunk_size, but the guard skips it. That makes the step (chunk_size - chunk_overlap) zero or negative, so the range() in _token_group_generator yields nothing and chunk() returns [] - the entire document is silently dropped (e.g. indexed as nothing in a RAG pipeline). The docstring already promises "ValueError: If ... chunk_overlap >= chunk_size". This validates the computed self.chunk_overlap (ints and floats alike). Adds a regression test (character tokenizer, no model/LLM) that fails on main and passes with the fix, plus a test confirming a valid fractional overlap (0.1) still chunks normally.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTokenChunker now validates the computed ChangesChunk overlap validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request adds validation to ensure that computed chunk overlaps in TokenChunker do not exceed the chunk size or fall below zero, preventing silent content dropping. It also includes regression tests for these cases. The review feedback identifies a subtle bug where negative float overlaps (like -0.05) truncate to 0 and bypass the validation, and suggests checking the raw input directly. Additionally, it recommends adding test cases to cover negative overlaps.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # Validate the COMPUTED overlap, not just the int input: a float chunk_overlap >= 1.0 | ||
| # is turned into int(chunk_overlap * chunk_size), which can be >= chunk_size and was | ||
| # skipped by the int-only guard above. That makes the step (chunk_size - chunk_overlap) | ||
| # zero or negative, so range() yields nothing and chunk() silently returns [] - the | ||
| # whole document is dropped. Enforce the docstring's contract for ints and floats alike. | ||
| if self.chunk_overlap >= self.chunk_size or self.chunk_overlap < 0: | ||
| raise ValueError("chunk_overlap must be >= 0 and less than chunk_size") |
There was a problem hiding this comment.
There is a subtle correctness issue here with negative float overlaps. If a user passes a negative float overlap like -0.05 with chunk_size=10, the expression int(-0.05 * 10) evaluates to int(-0.5), which truncates to 0 in Python. As a result, self.chunk_overlap becomes 0, and the check self.chunk_overlap < 0 will evaluate to False, silently bypassing the validation. Checking the raw chunk_overlap input directly for negative values resolves this issue.
| # Validate the COMPUTED overlap, not just the int input: a float chunk_overlap >= 1.0 | |
| # is turned into int(chunk_overlap * chunk_size), which can be >= chunk_size and was | |
| # skipped by the int-only guard above. That makes the step (chunk_size - chunk_overlap) | |
| # zero or negative, so range() yields nothing and chunk() silently returns [] - the | |
| # whole document is dropped. Enforce the docstring's contract for ints and floats alike. | |
| if self.chunk_overlap >= self.chunk_size or self.chunk_overlap < 0: | |
| raise ValueError("chunk_overlap must be >= 0 and less than chunk_size") | |
| # Validate the computed overlap and ensure the input overlap is non-negative. | |
| # A negative float like -0.05 can truncate to 0 via int(), bypassing the < 0 check on self.chunk_overlap. | |
| if chunk_overlap < 0 or self.chunk_overlap >= self.chunk_size: | |
| raise ValueError("chunk_overlap must be >= 0 and less than chunk_size") |
| def test_float_overlap_ge_one_is_rejected(): | ||
| with pytest.raises(ValueError): | ||
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=1.5) |
There was a problem hiding this comment.
It is highly recommended to add test cases verifying that negative overlaps (both integer and float) are correctly rejected during initialization.
| def test_float_overlap_ge_one_is_rejected(): | |
| with pytest.raises(ValueError): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=1.5) | |
| def test_float_overlap_ge_one_is_rejected(): | |
| with pytest.raises(ValueError): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=1.5) | |
| def test_negative_overlap_is_rejected(): | |
| with pytest.raises(ValueError): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=-0.5) | |
| with pytest.raises(ValueError): | |
| TokenChunker(tokenizer="character", chunk_size=10, chunk_overlap=-1) |
|
Following up on this PR — I found it with faultline, an open-source tester I built that feeds agents/pipelines deliberately broken inputs and fails CI when they silently produce wrong output (no LLM judge, deterministic). I've already written a 30-line suite for chonkie that would have caught this overlap bug automatically. Want me to open it as a separate PR with a non-blocking GitHub Action so you can watch it for a week before deciding? Zero config on your side — if it's noisy, close it, no hard feelings. |
TokenChunker.__init__only checkschunk_overlap >= chunk_sizeforintinputs. Afloatoverlap is converted toint(chunk_overlap * chunk_size), which can be>= chunk_sizewhile skipping that int-only guard — so the stepchunk_size - chunk_overlapgoes<= 0,range()yields nothing, andchunk()returns[]: the whole document is silently dropped, with no error. The docstring already documentsValueError: If ... chunk_overlap >= chunk_size.For example, on the current release:
This validates the computed
self.chunk_overlap(ints and floats alike) right after it's assigned:Includes a regression test (character tokenizer, no model download) that fails on
mainand passes with the change, plus a test that a valid fractional overlap (e.g.0.1) still chunks normally.Found while building a small fault-injection tester for agent tools.
Summary by CodeRabbit
Bug Fixes
Tests