Decode split UTF-8 sequences correctly in NDJSON streams - #6998
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: d12579d The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The reproduction test is clear and currently fails as expected, but the implementation fix described in the PR body is missing from the branch.
Reviewed changes
packages/effect/test/rpc/RpcSerialization.test.ts— adds a single regression test that splits the 3-byte UTF-8 encoding of€(U+20AC) across twondjsondecode calls.
⚠️ Implementation fix not yet present
The PR title and body describe a bug in RpcSerialization.ts:131 (TextDecoder.decode(bytes) without streaming mode), but the only change on the branch is the reproduction test. The test fails as expected:
expected [ { value: '€' } ] to deeply equal [ { value: '���' } ]
Per the PR body, the fix should be added to the same branch before this PR is merged.
Technical details
# Missing streaming UTF-8 decode fix
## Affected sites
- packages/effect/src/unstable/rpc/RpcSerialization.ts:123 — `TextDecoder` is constructed without planning for streaming state.
- packages/effect/src/unstable/rpc/RpcSerialization.ts:131 — `decoder.decode(bytes)` finalizes the chunk, so trailing bytes of an incomplete UTF-8 sequence are replaced and lost.
## Required outcome
- Splitting any valid UTF-8 multibyte character across arbitrary byte boundaries must not corrupt the decoded message.
- `ndJsonRpc` (which delegates to `makeNdjson`) must also be fixed automatically.
- Existing `maxBufferSize` behavior and tests must continue to pass.
## Suggested approach
Keep the `TextDecoder` instance scoped to the parser and use `decoder.decode(bytes, { stream: true })` so the decoder carries incomplete sequences across calls.ℹ️ Nitpicks
- The split point in the test is hardcoded via
bytes.indexOf(0xe2) + 1; this is clear for the specific 3-byte sequence but does not exercise every split byte of a multibyte character. Once the fix lands, consider adding one or two extra cases (split after byte 2 of 3, and a 4-byte character such as U+1F600) to prevent regression at other boundaries.
@v0 or keep the SHA fresh with Dependabot ｜ Fix it ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The implementation fix addresses the prior review feedback.
Reviewed changes
packages/effect/src/unstable/rpc/RpcSerialization.ts— changeddecoder.decode(bytes)todecoder.decode(bytes, { stream: true })inmakeNdjson, keeping theTextDecoderinstance scoped to the parser so incomplete multibyte UTF-8 sequences carry across chunk boundaries.packages/effect/test/rpc/RpcSerialization.test.ts— added a regression test splitting the 3-byte UTF-8 encoding of€across two decode calls..changeset/fix-ndjson-split-utf8.md— added a patch changeset for theeffectpackage.
The focused test suite passes (pnpm --filter effect test --run test/rpc/RpcSerialization.test.ts). The only remaining observation is a minor coverage consideration: the test currently exercises one split point of a 3-byte character; additional cases (second-byte split, 4-byte character) could further harden the regression suite.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
2 participants
Summary
A valid multibyte UTF-8 character split across transport chunks is replaced by invalid replacement characters before NDJSON parsing.
Important
This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.
NDJSON decoding corrupts split UTF-8
Module:
RpcSerializationAudit ID:
unstable-services-rpc-ndjson-utf8-chunkSeverity / confidence: high / high
What happens
A valid multibyte UTF-8 character split across transport chunks is replaced by invalid replacement characters before NDJSON parsing.
Why it happens
Each byte chunk is finalized with TextDecoder.decode(bytes) without streaming mode, so partial multibyte sequences are replaced instead of carried into the next chunk.
Expected behavior
A framed streaming parser must decode valid UTF-8 independently of transport chunk boundaries.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/unstable/rpc/RpcSerialization.ts:123-147View problematic code at
packages/effect/src/unstable/rpc/RpcSerialization.ts:123-147View exact lines on GitHub
Reproduction
pnpm test --run packages/effect/test/rpc/RpcSerialization.test.tsObserved failure: FAIL: U+20AC decoded as three U+FFFD replacement characters.
Implementation handoff
The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.
pnpm test --run packages/effect/test/rpc/RpcSerialization.test.tsAudit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715funstable-services-rpc-ndjson-utf8-chunkCloses EFF-435