Skip to content

fix: guard neighbour lookups in replace_blank (IndexError on trailing space)#318

Closed
koriyoshi2041 wants to merge 1 commit into
OpenBMB:mainfrom
koriyoshi2041:fix/replace-blank-bounds
Closed

fix: guard neighbour lookups in replace_blank (IndexError on trailing space)#318
koriyoshi2041 wants to merge 1 commit into
OpenBMB:mainfrom
koriyoshi2041:fix/replace-blank-bounds

Conversation

@koriyoshi2041

Copy link
Copy Markdown

replace_blank (src/voxcpm/utils/text_normalize.py) crashes on text ending in a space, and mishandles text starting with a space.

for i, c in enumerate(text):
    if c == " ":
        if (text[i + 1].isascii() and text[i + 1] != " ") and (text[i - 1].isascii() and text[i - 1] != " "):
            out_str.append(c)
    else:
        out_str.append(c)

The intent is to keep a space only when it sits between two ASCII word characters, but the neighbour lookups aren't bounds-checked:

  • A trailing space makes text[i + 1] raise IndexError: string index out of range. replace_blank("hello ") crashes. This is reachable from TextNormalizer.normalize() for any zh text that ends in a space.
  • A leading space makes text[i - 1] read text[-1] (the last character), so the decision wraps around and a leading space can be wrongly kept.

Fix: only keep the space at interior positions (0 < i < len(text) - 1), so edge spaces are dropped (they aren't between two words) and no out-of-range access happens. Interior spacing (e.g. "a b" kept, "中 文""中文") is unchanged.

Added tests/test_text_normalize.py covering trailing/leading/interior spaces and a CJK-adjacent space.

Note: I verified the function's behaviour directly (it's pure string logic), but couldn't run the full test locally because importing the package pulls in torch/torchaudio and a native torchaudio lib fails to load on my machine — CI should exercise the added test normally.

replace_blank keeps a space only when both neighbours are ASCII non-space
characters, but it indexed text[i + 1] / text[i - 1] without bounds checks.
A trailing space raised IndexError, and a leading space read text[-1]
(wrapping to the last character) and was wrongly kept. Restrict the check to
interior positions so edge spaces are dropped and no out-of-range access
occurs; interior spacing is unchanged.

@a710128 a710128 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.

Thanks for the clean fix! 🙏

I pulled the branch down and reproduced both bugs against main:

  • replace_blank("hello ")IndexError: string index out of range (trailing space, text[i + 1])
  • replace_blank(" hello")' hello' — the leading space is wrongly kept because text[i - 1] wraps around to the last character

The 0 < i < len(text) - 1 guard is the right minimal fix: edge spaces aren't between two words, so dropping them is correct, and interior spacing is unchanged.

I also ran your test file locally (had to pip install -e . first, which pulls torch — that's likely why it didn't run on your machine), and all 3 tests pass. I spot-checked a few extra cases too, all behaving as expected:

  • "a b""ab", "中 a""中a", "a 中""a中", "" / " " / " " all safe

LGTM, approving. 🚀

Optional (non-blocking): a couple more assertions like "a b" → "ab" and CJK/ASCII mixed cases would lock down the semantics nicely, but the current coverage is already solid.

@a710128

a710128 commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Thank you for catching this and for the clear write-up — your root-cause analysis (trailing-space IndexError + leading-space negative-index wrap-around) is spot on, and the fix is correct.

Just a heads up: this overlaps with #320, which targets the same bug. Both PRs even add a tests/test_text_normalize.py, so they'd conflict on merge. We're planning to go with #320 since its tests stub the heavy dependencies and run in isolation (no torch/torchaudio import chain — which is also why you couldn't run them locally here), with a bit broader coverage.

So we'll close this one as a duplicate, but your report and fix were genuinely valuable — thanks for taking the time to dig in and explain it so clearly. 🙏

@koriyoshi2041

Copy link
Copy Markdown
Author

Thanks for checking this and for the detailed note. #320 covers the same boundary-space bug with broader isolated tests, so closing this one to keep the queue clean.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants