Skip to content

fix(chunker): preserve URLs in CharacterTextChunker (#2510) - #2515

Open
zichen0116 wants to merge 1 commit into
MemTensor:mainfrom
zichen0116:fix/charactertext-chunker-url
Open

zichen0116 wants to merge 1 commit into
MemTensor:mainfrom
zichen0116:fix/charactertext-chunker-url

Conversation

@zichen0116

Copy link
Copy Markdown

Description

CharacterTextChunker replaces URLs with __URL_<n>__ placeholders before
splitting, but RecursiveCharacterTextSplitter can cut right through a
placeholder (e.g. __URL / _0__ landing in different chunks). The URL can
then no longer be restored: it disappears from the output and placeholder
fragments leak into the chunks.

CharacterTextChunker(chunk_size=10, chunk_overlap=0).chunk("安装文档:https://example.com/install 请查看。")
# before: ['安装文档:__URL', '_0__', '请查看', '。']
# after:  ['安装文档:', 'https://example.com/install', '请查看。']

The fix takes the first approach suggested in the issue: when the input
contains URLs, delegate to SimpleTextSplitter with the same
chunk_size/chunk_overlap. It already keeps placeholders indivisible
(see #2228), so this reuses the tested protection instead of adding a
second boundary-alignment implementation. Inputs without URLs keep the
previous recursive-splitter path byte-for-byte.

Related Issue (Required): Fixes #2510

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit Test
  • Test Script Or Test Steps (please provide)

New tests/chunkers/test_charactertext_chunker.py (5 tests):
test_url_preserved_with_small_chunk_size is the exact reproduction from
the issue — confirmed failing before the fix (2 of the URL tests fail on
unpatched code) and passing after. Also covers overlap, a URL longer than
chunk_size (kept whole, may oversize one chunk), multiple URLs, and a
no-URL test pinning the recursive-splitter output unchanged.

$ pytest tests/chunkers/test_charactertext_chunker.py -q
5 passed
$ pytest tests/chunkers/ -q
28 passed, 1 pre-existing failure in test_sentence_chunker (missing
`chonkie` in this env; fails on unpatched tree too)

ruff check and ruff format --check pass on both changed files.

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 | 我已添加测试以证明我的修复有效或功能正常
  • I have linked the issue to this PR (if applicable) | 我已将 issue 链接到此 PR(如果适用)

Copilot AI balanced review requested due to automatic review settings October 17, 2026 19:16
@Memtensor-AI Memtensor-AI added area:core MOS 编排层 / 框架底座 / 跨模块问题 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理 labels Oct 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@Memtensor-AI

Copy link
Copy Markdown
Collaborator

🤖 Open Code Review

Target: PR #2515
Task: fd60ffe058cd2cf9
Base: main
Head: fix/charactertext-chunker-url

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


1. tests/chunkers/test_charactertext_chunker.py (L16)

importorskip is used as a skip guard here, but its return value is bound to langchain_text_splitters and never read — the actual symbol (RecursiveCharacterTextSplitter) is re-imported locally inside test_text_without_url_keeps_recursive_splitting. Drop the assignment and import the class once at module level, then remove the local import in the test.

💡 Suggested Change

Before:

langchain_text_splitters = pytest.importorskip("langchain_text_splitters")

After:

pytest.importorskip("langchain_text_splitters")

from langchain_text_splitters import RecursiveCharacterTextSplitter

2. tests/chunkers/test_charactertext_chunker.py (L84-L89)

This control test hardcodes a third copy of the separator list (already duplicated in charactertext_chunker.py and simple_chunker.py) and asserts exact list equality against it. If the chunker's separators are later tuned, this fails even though no-URL behavior is still correct, and the failure doesn't distinguish a real regression from an intentional change. Assert delegation to the instance's own recursive splitter instead, which pins the behavior without duplicating the constant.

💡 Suggested Change

Before:

    expected = RecursiveCharacterTextSplitter(
        chunk_size=10,
        chunk_overlap=0,
        length_function=len,
        separators=["\n\n", "\n", "。", "!", "?", ". ", "! ", "? ", " ", ""],
    ).split_text(text)

After:

    assert chunks == splitter.chunker.split_text(text)

3. src/memos/chunkers/charactertext_chunker.py (L43)

protect_urls(text) on the line above is now only used by the non-URL path: when url_map is non-empty, protected_text is discarded and SimpleTextSplitter.chunk(text, ...) re-protects the raw text internally (see _simple_split_text). For URL-containing documents this runs the full-text URL regex substitution twice and allocates a second placeholder map, and the discarded protected_text reads as if it were being passed downstream. A cheap re.search(self._URL_PATTERN, text) check keeps a single protection pass per call.

💡 Suggested Change

Before:

        if url_map:

After:

        if re.search(self._URL_PATTERN, text):

Generated by cloud-assistant via Open Code Review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:core MOS 编排层 / 框架底座 / 跨模块问题 status:in-progress Someone or AI is working on it | 人工或 AI 正在处理

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] CharacterTextChunker loses URLs when splitting their placeholders

4 participants