Skip to content

test(sse): cover the split boundary the existing test claims to cover - #274

Merged
argszero merged 1 commit into
mainfrom
fix/sse-split-boundary-coverage
Sep 21, 2026
Merged

argszero merged 1 commit into
mainfrom
fix/sse-split-boundary-coverage

Conversation

@argszero

Copy link
Copy Markdown
Owner

Summary

sse_helpers_split_across_chunks (src/sse.rs:1628) is documented as covering "the event is split across
two chunks and a UTF-8 character straddles the boundary". Its second chunk boundary, however, sits after
an ASCII " — the multi-byte character sits wholly inside the second chunk. Measured: replacing
append_utf8_safe with a naive implementation (per-chunk String::from_utf8_lossy, no remainder carried)
produces byte-identical output on that input, so the test cannot fail for the bug it claims to guard.

This corrects the comment to describe what the input actually exercises, and adds two tests that put a
character inside a UTF-8 sequence so the guard can fail.

This is not a bug fix. Production code is correct on every input measured (12 legs, listed below); the
defect is in the test's claim, not in the code it guards.

Related Issue

Changes

  • src/sse.rs (test module only):
    • sse_helpers_split_across_chunks — kept, same input, comment corrected to say what it actually
      covers (a chunk concatenation, not a straddling character).
    • sse_helpers_split_inside_a_multibyte_char (new, 6 legs) — 3-byte 你 split 2+1 / 1+2 / 1+1+1
      and 4-byte 😀 split 1+3 / 2+2 / 3+1. Each leg asserts the character survives, that no
      U+FFFD appears, and that the buffer is drained.
    • sse_helpers_keep_an_incomplete_char_in_the_remainder (new, 2 legs) — half a character is withheld in
      remainder and emitted once completed; a genuinely invalid byte yields exactly one U+FFFD and
      parsing continues (A\u{FFFD}B). White-box on purpose: this is the property a naive implementation
      loses.
  • Byte-anchored inputs: the new legs feed [0xE4, 0xBD, 0xA0] / [0xF0, 0x9F, 0x98, 0x80] as bytes, not
    as string literals — a literal would make the test share the decoder assumption it is supposed to check.

Why the new legs can fail (measured, not asserted)

input correct naive (per-chunk from_utf8_lossy) distinguishable
the existing test's chunks (…{\"a\":\" | 你\"}\n\n) event: x␊data: {"a":"你"} byte-identical ❌ false
a real split [E4 BD] | [A0] 你 �� (two U+FFFD) ✅ true

Compiled A/B against a verbatim copy of the live helpers (rustc --test, four arms, all as declared):

arm declared result
new tests + correct helper green 3 passed / 0 failed
new tests + naive helper red on the new legs 1 passed / 2 failed
old test + naive helper green 1 passed / 0 failed
old test + correct helper green 1 passed / 0 failed

The third row is the point of this PR: the existing test passes against a broken implementation.

Re-measured in-tree on this branch (not on the standalone sketch): the whole body of append_utf8_safe was
substituted in place with the naive implementation, cargo test sse:: was run, and the file was then restored
and checked byte-identical (md5 536da10ed48033ce77600cb779b60567 before and after; git diff --stat unchanged).
Same verdict: retained test ok, both new tests FAILED (panics at src/sse.rs:1659 and :1677).

Honest boundary

  • ⛔ No production defect is claimed. 12 measured legs (3-byte and 4-byte splits in every position, clean
    boundary, invalid byte, CRLF/LF mixed framing, strip_sse_field in all four shapes) are correct on the
    current code.
  • append_utf8_safe's remainder.len() > 3 branch is unreachable — a valid incomplete UTF-8 prefix is at
    most 3 bytes, so error_len() == None can never leave more. It is a defensive branch, left untouched.
  • Blast radius is one file: append_utf8_safe / take_sse_block have no consumer outside src/sse.rs.

Tests

  • cargo test passes — 310 passed / 0 failed measured on this branch (main is 308; the delta is
    exactly the two tests added here, and src/sse.rs's #[test] count goes 19 → 21)
  • cargo fmt --check passes
  • cargo clippy --all-targets -- -D warnings passes
  • New unit tests added (2)
  • The new legs are demonstrated to fail against a naive implementation while the retained test still
    passes on that same naive implementation (A/B above, 4 arms, each arm's expectation declared up front)
  • Invariants after the edit (measured before → after): fn sse_helpers_split_across_chunks 1 → 1 (retained,
    not replaced); fn sse_helpers_split_inside_a_multibyte_char 0 → 1;
    fn sse_helpers_keep_an_incomplete_char_in_the_remainder 0 → 1; #[test] in src/sse.rs 19 → 21;
    the byte anchor 0xE4, 0xBD, 0xA0 0 → 1; FFFD occurrences 1 → 3

Checklist

  • Branch name follows the convention (fix/…)
  • Commit message follows the convention (test(sse): …)
  • PR targets the default branch (main)
  • Tests added for the change
  • No unrelated changes (test-only; no production code touched)

`sse_helpers_split_across_chunks` declares that a UTF-8 character straddles
the chunk boundary, but its boundary falls after an ASCII `"`: the multi-byte
character sits wholly inside the second chunk. A naive implementation
(per-chunk `String::from_utf8_lossy`, no remainder carried) therefore produces
byte-identical output on that input, so the test cannot fail for the bug it
claims to guard.

Keep the test and its input, correct the comment to describe what it actually
exercises, and add two tests that split a character inside its UTF-8 sequence:

- `sse_helpers_split_inside_a_multibyte_char`: 3-byte and 4-byte characters
  split in every position, asserting the character survives, no U+FFFD appears
  and the buffer drains.
- `sse_helpers_keep_an_incomplete_char_in_the_remainder`: the incomplete tail
  is withheld until completed; a genuinely invalid byte yields exactly one
  U+FFFD and parsing resumes.

Both feed the character as raw bytes so the test does not share the decoder
assumption it is meant to check. Test-only: no production code changes.
@argszero

Copy link
Copy Markdown
Owner Author

@/Users/argszero/scm/github.com/argszero/aitokenpool/.emrg/sessions/emrg-evolution-aitokenpool-opensource-task/tmp/c2168-review.txt

@argszero
argszero merged commit 7a078d8 into main Sep 21, 2026
1 check passed
@argszero
argszero deleted the fix/sse-split-boundary-coverage branch September 21, 2026 13:15
@argszero argszero mentioned this pull request Sep 24, 2026
10 tasks
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.

1 participant