Skip to content

test(gateway): unit regressions for _split_text_chunks under a custom length unit - #55843

Open
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:fix/stream-chunk-utf16-oversize
Open

test(gateway): unit regressions for _split_text_chunks under a custom length unit#55843
MaxFreedomPollard wants to merge 1 commit into
NousResearch:mainfrom
MaxFreedomPollard:fix/stream-chunk-utf16-oversize

Conversation

@MaxFreedomPollard

@MaxFreedomPollard MaxFreedomPollard commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Rebased and rescoped: the code fix landed on main via the competing fix in this cluster (the fallback split now uses the codepoint budget, and the progressive existing-message overflow path was corrected as well — both verified on current main).

What remains from this PR is direct unit coverage: the suite exercises the UTF-16 paths at integration level but has no direct unit tests of _split_text_chunks with a non-codepoint len_fn. This adds the three regression cases (emoji no-newline fallback, newline-in-first-half, default-len behaviour unchanged) from the original report (#55844).

@MaxFreedomPollard

Copy link
Copy Markdown
Contributor Author

Issue with reproduction: #55844

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jun 30, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Competing fix cluster for the _split_text_chunks no-newline UTF-16/codepoint bug: OPEN #42523 fixes the same code path in the same function (precomputed _cp_budget), and this PR (#55843) carries the matching issue #55844 plus broader emoji regression tests. Earliest is #42523. Also part of the UTF-16 split family (#11467/#23455/#11170/#11522). Flagging for a maintainer to pick one canonical fix.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the fallback chunking defect; the diagnosis is correct on current main.

Problems

  • The same UTF-16-unit/codepoint-index mismatch remains in the progressive existing-message overflow path: gateway/stream_consumer.py:710-716 computes _cp_budget, then compares against and slices at _safe_limit. That path can still send an oversized emoji chunk before fallback is entered.

Suggested changes

  • Apply the _cp_budget half-check and fallback slice to gateway/stream_consumer.py:703-716 as well.
  • Add an integration-level regression that exercises that existing-message overflow path with a UTF-16 length function.

Automated hermes-sweeper review.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 15, 2026
@MaxFreedomPollard
MaxFreedomPollard force-pushed the fix/stream-chunk-utf16-oversize branch from 3083e76 to a1b23aa Compare July 25, 2026 04:17
@MaxFreedomPollard MaxFreedomPollard changed the title fix(gateway): fallback chunking emits oversized Telegram messages for emoji text (UTF-16 unit vs codepoint) test(gateway): unit regressions for _split_text_chunks under a custom length unit Jul 25, 2026
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure P3 Low — cosmetic, nice to have and removed type/bug Something isn't working platform/telegram Telegram bot adapter P2 Medium — degraded but workaround exists labels Jul 25, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Re-triage correction: the current head is a test-only follow-up after the fallback implementation landed elsewhere. It retains direct UTF-16 chunking coverage; the remaining progressive existing-message overflow concern is a separate code-path gap noted in review. Related to #42523 and #55844, not a competing implementation duplicate.

@MaxFreedomPollard
MaxFreedomPollard force-pushed the fix/stream-chunk-utf16-oversize branch from a1b23aa to 49c0116 Compare July 25, 2026 05:10
@alt-glitch alt-glitch added the area/streaming Streaming responses: gateway delivery, provider wire label Jul 25, 2026
@MaxFreedomPollard
MaxFreedomPollard force-pushed the fix/stream-chunk-utf16-oversize branch 3 times, most recently from 0782064 to 1126e24 Compare July 25, 2026 06:09
@alt-glitch alt-glitch removed the sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages label Jul 25, 2026
@MaxFreedomPollard
MaxFreedomPollard force-pushed the fix/stream-chunk-utf16-oversize branch from 1126e24 to c0071b7 Compare July 31, 2026 03:52
… custom length unit

The code fix (fallback split honoring the platform length unit, and the
progressive existing-message overflow path) has landed on main via the
competing fix; the suite still has no direct unit tests of
_split_text_chunks with a non-codepoint len_fn. Add the emoji/UTF-16
regression cases: no-newline fallback, newline-in-first-half, and
default-len behaviour unchanged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/streaming Streaming responses: gateway delivery, provider wire comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants