fix(chunkers): make SimpleTextSplitter fallback URL-safe (#2200)

[hijzy]

Summary

• Make SimpleTextSplitter inherit BaseChunker so the fallback has protect_urls and restore_urls, matching the existing downstream class layout.
• Keep URL placeholders atomic at chunk boundaries and stop after the final chunk so overlap does not emit repeated shrinking tails.
• Prevent silent text corruption when user input or a URL contains placeholder-like text such as URL_0. A collision-free namespace is selected for those inputs, and restoration uses one regex pass so restored URLs are not processed again.
• Reject invalid chunk_size and chunk_overlap combinations that would lose text or create excessive duplicate chunks.
• Move the expanded fallback regression suite to tests/chunkers/test_simple_chunker_fallback.py so it does not collide with an existing downstream test of the same component.

Related issues: #2115, #2200

Compatibility

• Normal input keeps the existing short placeholder form and therefore preserves the established chunk-boundary behavior.
• No new dependency is introduced.
• A squash/cherry-pick simulation of the final diff onto the current downstream baseline completed without conflicts in the chunker source or tests.

How Has This Been Tested?

• make format: passed; Ruff reported 616 files unchanged.
• Full tests/chunkers suite: 24 passed.
• Forced fallback integration smoke test with langchain_text_splitters unavailable: passed, URL remained intact and no placeholder leaked.
• Downstream merged-tree tests: 23 passed, including the existing downstream SimpleTextSplitter tests and this PR regression suite.
• URL-heavy benchmark after the single-pass restoration change: 10,000 URLs in 0.048s and 20,000 URLs in 0.169s on the local test machine.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Refactor (keeps URL protection shared through BaseChunker and aligns the fallback hierarchy)

Checklist

• I have performed a self-review of my own code | 我已自行检查了自己的代码
• I have commented my code in hard-to-understand areas | 我已在难以理解的地方对代码进行了注释
• I have added tests that prove my fix is effective or that my feature works | 我已添加测试以证明我的修复有效或功能正常
• Documentation change is not applicable; this fixes internal fallback behavior without changing the public API.
• Related issues are linked above.
• I have mentioned the person who will review this PR | 我已提及将审查此 PR 的人

Reviewer Checklist

• Required checks passed
• Tests have been provided

[Memtensor-AI]

🤖 Open Code Review

Target: PR #2228
Task: 64dcd4c62d980dd4
Base: main
Head: dev-v2.0.29

🔍 OpenCodeReview found 4 issue(s) in this PR.

---

1. src/memos/chunkers/base.py (L23-L25)

The class-level _URL_PLACEHOLDER_PATTERN is compiled once and hardcoded to match only the fixed prefix __URL_. However, protect_urls may generate dynamic placeholders with a random hex segment (e.g. __URL_a1b2c3d4_0__) when the default prefix collides with content in text.

The restore_urls method uses this fixed pattern to scan and replace placeholders. Because the regex is fixed at class definition time and does not receive the actual placeholder_prefix used during protection, it relies entirely on the pattern (?:[0-9a-f]+_)? to cover the dynamic case. This works incidentally only if the dynamic prefix's hex part is all lowercase — which uuid.uuid4().hex does guarantee in CPython today, but is an undocumented implementation detail.

More importantly, the design creates a structural mismatch: protect_urls decides the placeholder format at runtime, but restore_urls uses a statically compiled pattern. If the text itself contains a string like __URL_deadbeef_99__ that was never a URL placeholder (e.g. it was literally in the input), restore_urls will still attempt a url_map.get(...) on it — returning it unchanged only because it is absent from url_map. This is benign today but fragile.

Consider returning the compiled pattern (or the prefix string) from protect_urls alongside url_map, and passing it into restore_urls, so the two methods are always in sync:

def protect_urls(self, text: str) -> tuple[str, dict[str, str], re.Pattern]:
    ...
    placeholder_pattern = re.compile(
        rf"{re.escape(placeholder_prefix)}\d+__"
    )
    ...
    return protected_text, url_map, placeholder_pattern

def restore_urls(self, text: str, url_map: dict[str, str], placeholder_pattern: re.Pattern) -> str:
    return placeholder_pattern.sub(
        lambda match: url_map.get(match.group(0), match.group(0)), text
    )

This makes the contract explicit and eliminates the implicit coupling between a runtime-chosen prefix and a compile-time regex.

💡 Suggested Change

Before:

    _URL_PLACEHOLDER_PATTERN = re.compile(
        rf"{re.escape(_URL_PLACEHOLDER_PREFIX)}(?:[0-9a-f]+_)?\d+__"
    )

After:

    # No longer needed as a class attribute; generate per protect_urls call
    # and pass the compiled pattern into restore_urls explicitly.

---

2. src/memos/chunkers/simple_chunker.py (L100)

When _align_end_to_placeholder returns placeholder_end (the branch taken when placeholder_start - start <= chunk_overlap), the new end value can exceed text_len. This happens because _align_end_to_placeholder is called inside the if end < text_len: branch, yet it can push end beyond that bound. While Python slicing silently truncates and the if end >= text_len: break guard on line 106 fires correctly, the loop body still calls protected_text[start:end].strip() with an end larger than text_len. This is currently safe but fragile: any future refactor that uses end as an index (e.g., protected_text[end]) rather than a slice bound would produce an IndexError. Consider capping the return value:

return min(placeholder_end, len(protected_text))  # or pass text_len in

or add an assertion/clamp inside _simple_split_text immediately after the call:

end = min(end, text_len)

💡 Suggested Change

Before:

                end = self._align_end_to_placeholder(end, start, chunk_overlap, placeholder_spans)

After:

                end = min(
                    self._align_end_to_placeholder(end, start, chunk_overlap, placeholder_spans),
                    text_len,
                )

---

3. src/memos/chunkers/simple_chunker.py (L111)

In _align_start_to_placeholder, when next_start lands inside a placeholder and placeholder_start <= previous_start, the method returns placeholder_end. However, if placeholder_end itself lands inside a second, adjacent placeholder (e.g., two URLs with no characters between them), the returned start is still inside a placeholder. The outer while loop then enters _align_end_to_placeholder with that start, potentially producing a chunk that begins mid-placeholder.

From base.py, protect_urls inserts placeholders by sequential re.sub replacement, so two consecutive URLs in the original text produce back-to-back placeholders with no characters between them (e.g., __URL_0____URL_1__). This is a reachable case (see the parametrized test test_url_protection_round_trip_preserves_exact_input which includes "https://one.example/a https://two.example/b ..."). The single-pass loop in both alignment helpers does not iterate to re-check after moving start or end.

Suggestion: wrap the alignment call in a loop (or use a while inside the helper) until no span contains the result, or sort spans and binary-search to find the outermost safe boundary.

💡 Suggested Change

Before:

            start = self._align_start_to_placeholder(next_start, start, placeholder_spans)

After:

            start = self._align_start_to_placeholder(next_start, start, placeholder_spans)
            # Re-check in case the resolved start lands inside another adjacent placeholder.
            start = self._align_start_to_placeholder(start, start, placeholder_spans)

---

4. tests/chunkers/test_simple_chunker_fallback.py (L78)

The guard assert len(chunks) < 10 is so loose that it provides almost no regression protection. For chunk_size=100, chunk_overlap=20 on this ~240-character input the expected chunk count is ~3. An implementation regressing to produce 9 chunks (a 3× explosion) would still pass silently. Derive the bound from the input and parameters instead:

max_expected = math.ceil((len(text) - chunk_size) / (chunk_size - chunk_overlap)) + 1  # ≈ 3
assert len(chunks) <= max_expected + 1  # allow one extra for boundary adjustment

Or simply assert the tighter property directly, e.g. assert len(chunks) <= 5.

Generated by cloud-assistant via Open Code Review.

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (9/9 executed). memos_python_core/changed-repo-python: 9/9. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-477e71d1dfcfc2e0-20260806193914: 103/104 passed, 1 failed — these do NOT affect the PR verdict; review the branch manually.

Branch: dev-v2.0.29

[Memtensor-AI]

✅ Automated Test Results: PASSED

All tests passed (21/21 executed). memos_python_core/changed-repo-python: 21/21. Duration: 5s [advisory, non-gating] AI-generated tests on branch test/auto-gen-64dcd4c62d980dd4-20260806204020: 65/65 passed — these do NOT affect the PR verdict; review the branch manually.

Branch: dev-v2.0.29