fix(daemon): decode raw stdout Buffer chunks safely in the shared JSON-line stream parser - #7465
fix(daemon): decode raw stdout Buffer chunks safely in the shared JSON-line stream parser#7465vecsop wants to merge 1 commit into
Conversation
…N-line stream parser
pi-rpc/session.ts and acp/session.ts both feed createJsonLineStream()
raw, non-setEncoding'd stdout Buffer chunks. Decoding each chunk
independently (pi-rpc/session.ts's own chunk.toString('utf8'), or the
implicit Buffer coercion `buffer += chunk` performs inside feed())
corrupts any multi-byte UTF-8 character split across a chunk boundary
into U+FFFD, since neither decode has memory of the previous chunk's
trailing bytes.
feed() now owns decoding itself via one TextDecoder per stream
instance ({ stream: true }, so a split sequence's leading bytes are
held back until the rest arrives on a later feed() call), instead of
depending on every caller to setEncoding('utf8') first. pi-rpc's own
pre-decode is removed (it ran ahead of feed and would keep corrupting
text before the fixed decoder ever saw it); acp/session.ts needed only
a corrected Buffer | string parameter type in place of the misleading
`string` annotation that helped this go unnoticed.
json-event-stream.ts / copilot-stream.ts / claude-stream.ts have the
same-looking feed(chunk: string) shape but are not touched here: their
only real callers (server.ts, connectionTest.ts) already call
setEncoding('utf8') before attaching a 'data' listener, and Node's
StringDecoder is itself stream-safe, so by the time feed() sees a
chunk there it's already a complete, valid string. Verified with the
same split-byte harness before scoping this PR to the two paths that
actually reproduce.
|
Hey @vecsop — thanks for tightening the shared decoder path here; moving UTF-8 boundary handling into the shared parser is the right direction. Could you fill in the PR body with Why, What users will see, Surface area, and Validation? I also added |
mrcfps
left a comment
There was a problem hiding this comment.
@vecsop Thanks for pinning the UTF-8 split at the shared parser instead of hoping every stdout listener remembers setEncoding.
I walked the changed ranges in createJsonLineStream, the pi-rpc and ACP session data handlers, and the new chunk-boundary tests. feed() now keeps one TextDecoder with { stream: true } for raw Buffer chunks, leaves already-decoded strings alone, and flush() ends the decoder so a truncated sequence becomes U+FFFD instead of disappearing. Removing pi-rpc's per-chunk toString('utf8') is necessary for that decoder to ever see the split bytes. The regression cases cover a CJK 3-byte split, an emoji 4-byte split, and a string caller that already went through setEncoding.
Nice, focused bugfix — appreciate the careful scoping away from the stream parsers that were already safe.
🔁 Powered by Looper · runner=reviewer · agent=omp · An autonomous AI dev team for your GitHub repos.
|
Queued for QA validation — this change is on a live daemon runtime path, and now that the PR has review approval plus green CI, the next step is a manual QA pass before merge. |
Fixes #
Why
agent-protocol/core/json-line-stream.ts'screateJsonLineStream().feed()accumulates incoming chunks withbuffer += chunk. Two of its real callers hand it raw, non-setEncoding'd stdoutBufferchunks:agent-protocol/pi-rpc/session.ts(used by thepiadapter) never callssetEncodingonstdoutand, before this PR, additionally ran each chunk through its ownchunk.toString('utf8')ahead offeed;agent-protocol/acp/session.ts(used by every ACP-backed adapter) callssetEncoding('utf8')onchild.stderrbut never onchild.stdout— the pre-existing(chunk: string)parameter annotation on that handler was misleading, the runtime value is a raw Buffer.Decoding a Buffer chunk independently of its neighbors — whether via
chunk.toString('utf8')or the implicit coercionbuffer += chunkperforms — has no memory of a previous chunk's trailing bytes. A multi-byte UTF-8 character (any CJK character, most emoji) whose bytes straddle a chunk boundary gets corrupted into U+FFFD on each side of the split independently. Assistant text and tool-call arguments streamed over these two paths can contain arbitrary UTF-8, so this is a live, reproducible corruption bug for any non-ASCII content that happens to land on a chunk boundary, not a contrived edge case.What users will see
piadapter no longer occasionally corrupt CJK/emoji characters in streamed assistant text into �.Surface area
Files touched: the shared parser (
json-line-stream.ts) plus the two call sites that needed a type/decode-ownership correction (pi-rpc/session.ts,acp/session.ts), and a new regression test file.json-event-stream.ts/copilot-stream.ts/claude-stream.tshave the identical-lookingfeed(chunk: string) { buffer += chunk; ... }shape but are not touched here: their only real callers (server.ts,connectionTest.ts) already callsetEncoding('utf8')before attaching a'data'listener, and Node'sStringDecoder(whichsetEncodinguses internally) is itself stream-safe for multi-byte sequences split across the underlying Buffer reads — by the timefeed()sees a chunk on those paths, it's already a complete, valid string. Verified this with the same split-byte harness used for the two paths that do reproduce, before deciding what to touch, rather than assuming all four parsers were equally affected because they look the same. Happy to harden those three the same way in a follow-up if maintainers would rather not depend on every caller rememberingsetEncoding— kept this PR's diff limited to paths with a demonstrated failure.Bug fix verification
apps/daemon/tests/agent-protocol/json-line-stream-chunk-boundary.test.ts— two tests reproduce each real call site's exact shape (raw Buffer chunks split mid multi-byte-character): one modeled onpi-rpc/session.ts's stream (a CJK character split), one onacp/session.ts's (an emoji split). A third test pins the companion invariant: a caller that did callsetEncoding('utf8')hands over an already-decoded string, which must pass through unchanged rather than be re-run throughTextDecoder.decode(which expects raw bytes).mainand green on this branch? Yes. Confirmed by revertingfeed's decode change andpi-rpc/session.ts'sparser.feed(chunk)call locally and re-running: 2 of the 3 new tests fail with U+FFFD in the reassembled JSON on the pre-fix code, all 3 pass after.Validation
pnpm --filter @open-design/daemon typecheck— clean, 0 errors.pnpm guard— clean.pnpm exec vitest run -c vitest.config.ts tests/agent-protocol/json-line-stream-chunk-boundary.test.ts tests/acp.test.ts tests/pi-rpc.test.ts— 151/153 passed. The 2 failures (pi-rpc.test.tssymlink-upload tests) areEPERM: operation not permitted, symlinkin my local sandbox (no Windows Developer Mode / symlink privilege) and reproduce identically on a cleanmaincheckout with none of this PR's changes applied — pre-existing environment limitation, not a regression from this change.