Repository navigation
Fix SSE stream-wide byte order mark handling - #344094
Nikhil Kumar (Nikhil-4404) wants to merge 7 commits into
Conversation
Assisted-by: OpenAI Codex
There was a problem hiding this comment.
🟢 Approval recommended
The narrowly scoped decoder fix has targeted regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
This PR fixes byte order mark handling in VS Code’s shared server-sent events parser so that only a stream-leading BOM is stripped.
Changes:
- Preserve decoder state across lines while flushing incomplete UTF-8 at line endings.
- Add regression coverage for BOM placement, chunk boundaries, line endings, and incomplete UTF-8.
No actionable issues remain. Tests were reviewed but not executed.
| File | Description |
|---|---|
src/vs/base/test/common/sseParser.test.ts |
Adds BOM and incomplete UTF-8 regression tests. |
src/vs/base/common/sseParser.ts |
Preserves stream-wide BOM state when decoding lines. |
Review effort: Balanced
|
|
@microsoft-github-policy-service agree |
Rahul (rahul05ranjan)
left a comment
Rahul (rahul05ranjan)
left a comment
There was a problem hiding this comment.
Verified the core claim against TextDecoder semantics and a port of the parser.
The bug is real, and it's sneakier than "stream-wide BOM": calling decode() without { stream: true } flushes the shared decoder and resets its BOM handling, and the old code took that path on every completed line, not just once. Confirmed directly:
const d = new TextDecoder();
d.decode(Uint8Array.of(0xEF,0xBB,0xBF,0x41)); // "A" — BOM stripped at stream start
d.decode(Uint8Array.of(0xEF,0xBB,0xBF,0x42)); // "B" — BOM stripped AGAIN after the flushSo any \uFEFF that starts a decode after a line boundary got silently eaten — e.g. a data: payload containing a BOM could lose it (or worse, a \uFEFFdata: line could morph into a real data: field and inject content into the event). I ported feed/processLine/processField/dispatchEvent and ran your test's matrix (leading BOM × \n/\r\n/\r × chunk sizes 1/2/3/whole): the old code returns first\nignored\nlast, then ignored too, then kept — all three mid-stream BOMs eaten — while the new code produces exactly the asserted events in all 24 combinations.
Decoding the terminator itself and trimming with .slice(0, -1) is the right trick: it keeps the decoder's stream state alive, and since \n/\r are always a single UTF-16 code unit, the trim can never cut half of a surrogate pair. The incomplete-UTF-8-before-EOL case is preserved too (fed a lone 0xE2 before the terminator → data: \uFFFD, matching your second test), and the CRLF shapes (\r at the end of one chunk, \n leading the next) still round-trip through endedOnCR correctly in my port. A BOM split across feed calls at the true stream start is still stripped once, as it should be.
One nit: the comment says the terminator decode "flush[es] incomplete UTF-8" — it doesn't flush (the decoder stays in stream mode); the pending sequence errors out because \n can't continue it. Same observable result, but the wording could confuse someone auditing decoder state later.
Great test coverage — the chunk-size loop over all three EOLs is exactly the matrix this needed.
|
Thanks for verifying the fix and tests. Agreed—I’ll clarify the wording to “replace incomplete UTF-8” while preserving the decoder’s streaming state. |
The SSE parser resets
TextDecoderBOM detection after each line. As a result,data: first\n\uFEFFdata: second\n\nincorrectly producesfirst\nsecond. Only a stream-leading BOM should be removed; the later BOM-prefixed field name should be ignored, producingfirst.Keep the decoder streaming across lines. Include the ASCII line terminator in decoding and remove it from the decoded string, so incomplete UTF-8 is still replaced at the line boundary instead of leaking into the next field.
Add native regression tests for the initial-versus-later BOM distinction across LF, CRLF and CR line endings and chunk boundaries, plus incomplete UTF-8 before a newline. Public APIs and callers do not change.
Validation
Issue association
A focused issue has not yet been linked in this description.
AI assistance
OpenAI Codex assisted with investigation, implementation, tests and this description. No independent human review or CLA signing is claimed.