fix(sse): treat a data frame that parses to a non-record as malformed (#1219) - #1240
Conversation
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
|
✅ Deterministic PR hygiene checks passed. |
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe SSE parsers now validate parsed JSON as non-null objects before reading event fields. Invalid shapes use parser-specific handling. Tests cover null, scalar, and array frames across all affected parsers. ChangesSSE shape validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/web-search/parse.ts`:
- Around line 156-169: Update the malformed-JSON warning in the SSE parsing flow
around parseSidecarSSE to omit payload.slice(0, 120) entirely, logging only the
frame length and a fixed malformed-JSON classification. Add a regression test
using a unique payload marker and verify console.warn does not receive or
contain that marker, while preserving the existing early return.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: da7c462f-5ac4-401c-8acd-723657d9f536
📒 Files selected for processing (5)
src/adapters/anthropic.tssrc/adapters/google.tssrc/adapters/openai-chat.tssrc/web-search/parse.tstests/sse-null-data-frame.test.ts
48f55bf to
b01a623
Compare
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/sse-null-data-frame.test.ts`:
- Around line 145-146: Remove the `event.type === "text"` comparison from the
events assertion in the SSE null-data-frame test, leaving only the valid
`"text_delta"` discriminator while preserving the existing `"done"` event
assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ed8febd5-4d97-479b-a104-b4bcf61f4071
📒 Files selected for processing (3)
src/adapters/openai-chat.tssrc/web-search/parse.tstests/sse-null-data-frame.test.ts
b01a623 to
8585263
Compare
8585263 to
f155138
Compare
|
Rebased onto current Two review findings from CodeRabbit are resolved, both in-thread:
One disclosure on the "CI green locally" box. It is not from this change. Controlled by checking out Flagging it rather than quietly ticking the box, since I cannot honestly claim a fully green local run at this |
|
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The crash diagnosis is valid, and the Anthropic plus web-search shape guards have the right non-terminal behavior. The OpenAI Chat and Google behavior must change before merge, though.
The reporter supplied a real healthy stream where data: null appears between answer deltas and the later finish_reason / [DONE]. On this head, both adapters classify that frame like invalid JSON and terminate immediately, so the reported provider still fails even though the TypeError is gone. The new tests currently lock in that incorrect behavior (is a terminal malformed frame and parity with {not json}).
Please make valid-JSON non-record frames return continue in openai-chat.ts and google.ts, keep sawAnyFrame = true after the Google shape guard, and replace the terminal-parity assertions with both of these regressions:
- a mid-stream
data: nullfollowed by a valid finish chunk and[DONE]completes successfully; - a stream containing only non-record frames still reaches the existing truncation/empty-stream error, so skipping does not create a silent success.
Keep Anthropic/web-search non-terminal and payload-safe as they are. Also rebase the result onto current dev; this head is now 22 commits behind. I can re-review once the corrected semantics and fresh-base CI are present.
`JSON.parse("null")` returns null rather than throwing, so a try/catch around
the parse cannot see it. Four SSE frame parsers cast the result straight to
`Record<string, unknown>` and dereferenced it on the next line, so a `data: null`
frame walked past the malformed-frame guard and crashed the parser mid-stream.
Validate the shape at each site instead of asserting it. A non-record frame is
skipped, not terminated on: the reporter's capture shows `data: null` arriving as
a benign padding frame BETWEEN content deltas, with the legitimate finish_reason
chunk and [DONE] right behind it, so failing there discards an answer that has
already fully arrived — they measured the complete text landing and the turn
failing anyway. openai-chat and google now `continue`; anthropic drops the frame
so the message_stop check still governs; the web-search sidecar warns and skips.
An unparseable frame stays terminal in every parser, unchanged.
Skipping cannot mask a broken stream. A stream carrying only non-record frames
sets neither `finishReason` nor `sawUserFacingOutput` in openai-chat, and the
google guard returns before `sawAnyFrame`, so both still fail closed on the
existing EOF terminal-signal check. Both properties are covered by regressions.
`src/web-search/parse.ts` is not named in the report. It has the identical
defect and sits in the web-search path the reporter was actually running.
The other eight SSE data-frame parsers were audited and are already correct:
they parse into `unknown` and gate on a record predicate before dereferencing.
Neither web-search warning copies the frame's content. The new non-record branch
reports length and JSON shape only, and the pre-existing malformed-JSON warning
one line above stops logging `payload.slice(0, 120)`: an upstream SSE payload can
carry model output or credential material, and a frame that failed to parse is
the least trustworthy content there is. A regression test asserts that a unique
marker in the payload never reaches `console.warn` on either path.
Closes lidge-jun#1219
f155138 to
965dd99
Compare
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Current HEAD =='
git rev-parse HEAD
echo '== Changed files relative to merge base with dev =='
base="$(git merge-base HEAD dev 2>/dev/null || git merge-base HEAD origin/dev)"
git diff --stat "$base"...HEAD
git diff --unified=80 "$base"...HEAD -- \
src/adapters/openai-chat.ts \
src/adapters/google.ts \
src/adapters/anthropic.ts \
src/web-search/parse.ts \
tests/sse-null-data-frame.test.ts
echo '== Structural outlines =='
ast-grep outline src/adapters/openai-chat.ts --items all --view expanded
ast-grep outline src/adapters/google.ts --items all --view expanded
ast-grep outline src/adapters/anthropic.ts --items all --view expanded
ast-grep outline src/web-search/parse.ts --items all --view expanded
ast-grep outline tests/sse-null-data-frame.test.ts --items all --view expanded
echo '== Existing parser malformed-frame handling and tests =='
rg -n -C 6 --glob '*.ts' \
'(JSON\.parse|malformed|unparseable|Invalid .*frame|parseSidecarSSE|data\.type|data\.choices)' \
src/adapters/openai-chat.ts src/adapters/google.ts src/adapters/anthropic.ts src/web-search/parse.ts tests/sse-null-data-frame.test.tsLength of output: 1627 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD metadata =='
git show -s --format='commit=%H%nparents=%P%nsubject=%s' HEAD
echo '== Local and remote refs =='
git branch -a
git tag --contains HEAD || true
git remote -v || true
echo '== Commit ancestry and changed files =='
git diff-tree --no-commit-id --name-status -r HEAD
git diff --stat HEAD^ HEAD
git diff --unified=70 HEAD^ HEAD -- \
src/adapters/openai-chat.ts \
src/adapters/google.ts \
src/adapters/anthropic.ts \
src/web-search/parse.ts \
tests/sse-null-data-frame.test.ts
echo '== Relevant source regions at HEAD =='
for file in \
src/adapters/openai-chat.ts \
src/adapters/google.ts \
src/adapters/anthropic.ts \
src/web-search/parse.ts \
tests/sse-null-data-frame.test.ts
do
echo "----- $file -----"
rg -n -C 12 '(JSON\.parse|parseSidecarSSE|data\.type|data\.choices|malformed|unparseable)' "$file" || true
doneLength of output: 84967 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== OpenAI Chat: handler call sites and terminal path =='
sed -n '940,1160p' src/adapters/openai-chat.ts
echo '== Google: handler call sites and terminal path =='
sed -n '490,710p' src/adapters/google.ts
echo '== Anthropic: dropped-frame helper and stream terminal path =='
rg -n -C 10 'function debugDroppedFrame|const debugDroppedFrame|debugDroppedFrame\s*=|emitDone|message_stop|finally' src/adapters/anthropic.ts
sed -n '960,1150p' src/adapters/anthropic.ts
echo '== Web-search: complete parser and payload logging rules =='
sed -n '141,250p' src/web-search/parse.ts
fd -HI '^\.coderabbit\.ya?ml$|^coderabbit\.ya?ml$' . -x sh -c 'echo "----- $1 -----"; cat "$1"' sh {} \;
echo '== Direct uses of the changed diagnostic helper =='
rg -n -C 3 '\bdebugDroppedFrame\s*\(' src --glob '*.ts'Length of output: 50516
I found no blocking issues in the inspected SSE parser changes.
The sandbox checkout was commit
|
|
You and @brunoflma are right, and I was wrong. Corrected in What I got wrong. I read #1219's body and its synthetic repro, treated There was also a tell inside my own diff that I should have caught without the capture: I made Changes
Tests — replaced the terminal-parity assertions, and added a third case beyond the two you asked for:
All three run across Verification (via
Not claiming a fully green local run: the Two nested defects of the same class, deliberately NOT in this PR. While confirming the skip semantics I checked one level deeper than the frame root. These are unguarded in the same way and reproduce today:
@Ingwannu — ready for re-review when you have a window. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/sse-null-data-frame.test.ts`:
- Around line 19-22: Update the comment above the parity assertions in the test
to remove the claim that every valid-JSON non-record frame is handled like
unparseable JSON. Describe the parser-specific behavior instead: malformed JSON
terminates the stream, while OpenAI Chat and Google skip non-record frames and
continue processing later frames; keep the existing assertions unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9317767a-d9d1-47d2-b94e-034e62fa6295
📒 Files selected for processing (3)
src/adapters/google.tssrc/adapters/openai-chat.tstests/sse-null-data-frame.test.ts
| // The syntactically-invalid control. It was already handled correctly before this fix; every | ||
| // assertion below is written as parity against it so the test states the actual requirement — | ||
| // a valid-JSON non-record frame is treated exactly like an unparseable one — rather than | ||
| // re-encoding each adapter's terminal message and drifting when those messages change. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the malformed-frame parity comment.
Lines 19-22 state that every non-record frame is treated like invalid JSON. This is false for the OpenAI Chat and Google adapters. Lines 121-129 and 151-159 verify that malformed JSON terminates the stream, while a non-record frame is skipped and later frames continue.
Describe parser-specific handling instead.
Proposed fix
-// assertion below is written as parity against it so the test states the actual requirement —
-// a valid-JSON non-record frame is treated exactly like an unparseable one — rather than
-// re-encoding each adapter's terminal message and drifting when those messages change.
+// assertion below preserves each parser's established malformed-frame behavior. A valid-JSON
+// non-record frame is skipped or dropped according to that parser's behavior, while invalid JSON
+// keeps its existing error handling.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // The syntactically-invalid control. It was already handled correctly before this fix; every | |
| // assertion below is written as parity against it so the test states the actual requirement — | |
| // a valid-JSON non-record frame is treated exactly like an unparseable one — rather than | |
| // re-encoding each adapter's terminal message and drifting when those messages change. | |
| // The syntactically-invalid control. It was already handled correctly before this fix; every | |
| // assertion below preserves each parser's established malformed-frame behavior. A valid-JSON | |
| // non-record frame is skipped or dropped according to that parser's behavior, while invalid JSON | |
| // keeps its existing error handling. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/sse-null-data-frame.test.ts` around lines 19 - 22, Update the comment
above the parity assertions in the test to remove the claim that every
valid-JSON non-record frame is handled like unparseable JSON. Describe the
parser-specific behavior instead: malformed JSON terminates the stream, while
OpenAI Chat and Google skip non-record frames and continue processing later
frames; keep the existing assertions unchanged.
3 participants
Closes #1219
Problem
JSON.parsereturns a value, not necessarily an object.JSON.parse("null")returnsnullwithout throwing, so atry/catchwrapped around the parse cannot see it. Four SSE frame parsers cast the result straight toRecord<string, unknown>and dereference it on the next line, so a syntactically validdata: nullframe walks past the malformed-frame guard and crashes the parser mid-stream.You observed it through the web-search loop on
adapter: openai-chat, where upstream instability makes odd frames frequent, but nothing about it is provider-specific or web-search-specific — it reaches any streamed request.src/adapters/openai-chat.ts:961chunk.error:970yieldmalformed frame + terminatesrc/adapters/google.ts:502chunk.error:510yieldmalformed frame + terminatesrc/adapters/anthropic.ts:989data.type:995debugDroppedFrame+continuesrc/web-search/parse.ts:157data.type:161console.warn+returnThe fourth row is not in the report. The issue names three adapters.
src/web-search/parse.tshas the identical defect, and it sits directly in the web-search path you were actually running —parseSidecarSSEwraps its read loop intry/finallywith nocatch, so theTypeErrorescapes the function. Fixing only the three reported sites would have left your own code path broken.Two failure modes, not one. Property access on a non-null primitive is legal in JS, so only
nullactually throws:data:payloadtypeofnullobjectnull is not an object42/"text"/true[]object(array)The crash is the reported bug. The silent acceptance is the same defect one step milder: a malformed frame is treated as a well-formed frame carrying nothing, so it is neither surfaced nor counted. Both are fixed by validating the shape rather than asserting it — which is what your proposed patch does, so this follows it.
Audit of the remaining SSE parsers — no other site is affected. Eight further data-frame parsers were checked; all parse into
unknownand gate on a local record predicate before any dereference, so they were already correct:src/chat/outbound.ts:510and:678(isRec),src/claude/outbound.ts:624and:869(isRec),src/server/claude-messages.ts:177(isRec),src/web-search/anthropic-executor.ts:78(isRec),src/vision/anthropic-describe.ts:63(isRecord),src/adapters/openai-responses.ts:1220(isPlainObject).Fix
Validate the parse result instead of asserting it, at all four sites. The guard is written against the shape (
null, non-object, array) rather than againstnullalone, so the silent-empty case is closed together with the crash.Each site keeps its own existing malformed-frame handling rather than acquiring a new one — this PR changes what counts as malformed, not what happens next:
src/adapters/openai-chat.ts,src/adapters/google.ts— emit the existingmalformed upstream SSE data frameerror and terminate, exactly as the invalid-JSON path does.src/adapters/anthropic.ts—debugDroppedFrameandcontinue, so the pre-existingmessage_stoptruncation check still decides the stream's outcome.src/web-search/parse.ts— warn and skip the frame.The guard uses the shape-check form already present in these files (the
wrappedchecks insrc/adapters/google.ts, therawChoicecheck insrc/adapters/openai-chat.ts), so no new helper or import is introduced.Tests
New
tests/sse-null-data-frame.test.ts— 25 tests, covering all four production sites against all five non-record payloads (null,42,"text",true,[]).Assertions are written as parity against the syntactically-invalid control (
{not json}) rather than by re-encoding each parser's terminal message. That states the actual requirement — a valid-JSON non-record frame is handled exactly like an unparseable one — and does not drift when those messages change. The control also proves the pre-existing guard still works, so a regression breaking both would not pass silently.Beyond the shape matrix:
data: [DONE]stays terminal onopenai-chatand does not become a clean completion;anthropicdoes not prevent a later well-formed frame from being read through todone;Verification on this branch (all runs via
bun scripts/test.ts, i.e.--isolateplus the isolatedCODEX_HOME/OPENCODEX_HOMEenvironment):devwithout the production change, so the coverage is proven to bite rather than merely to pass;bun x tsc --noEmit— output byte-identical to a stashed clean-devcontrol (both exit 2 on one pre-existing@napi-rs/keyringmodule-resolution error local to my machine);bun scripts/privacy-scan.ts— passed.On the Windows leg. This was developed on Windows, where the suite is known-red — #1059 tracks ~207 pre-existing failures and the leg is dispatch-only. Rather than report that as noise, I controlled the affected area directly: the 74
codex-*andcli-*files that contain every failure were run on this branch and on a stashed cleandev.So this change adds no failure to the known-red set, and none of those files is reachable from the four it touches.
Notes
One behavioural change beyond the crash fix. On
openai-chatandgoogle, adata:frame carrying a non-null non-record (42,"text",true,[]) is now reported as a malformed frame and terminates the stream, where before it was silently ignored. This is deliberate — such a frame is not valid in either wire format, and silently continuing is how a broken stream reaches the truncation guard with a misleading cause.anthropicand the web-search sidecar keep their non-terminal handling, so nothing that previously completed a turn now fails.No payload content is added to any log. The new
src/web-search/parse.tsbranch reports only the frame length and its JSON shape (null/array/number/ …), not a slice of the payload. The pre-existing invalid-JSON warning one line above still logspayload.slice(0, 120); that line is untouched, but the new path deliberately does not copy the pattern.Overlap with #1194. That PR changes the
data:field-prefix handling insrc/adapters/openai-chat.tsandsrc/web-search/parse.ts, among others. The changes are adjacent but disjoint — #1194 decides which lines become payloads, this PR decides what a parsed payload must look like. Whichever lands first, the other is a trivial rebase; happy to rebase onto #1194 if you would rather take them in that order.The reporter also asked for a
debugProviderDiagnosticon the malformed-frame path, noting the failure leaves no server-side trace. That is deliberately not in this PR — it is a diagnostics change with its own shape (which channel, at what level, and whether frame content may be recorded), and it should not ride along with a crash fix. Happy to take it as a follow-up, or to leave it to @brunoflma who raised it.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
Bug Fixes
Tests