fix(openai-chat): accept SSE data fields without the optional space (#1170) - #1188
fix(openai-chat): accept SSE data fields without the optional space (#1170)#1188Yuxin-Qiao wants to merge 3 commits into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
📝 WalkthroughWalkthroughThe OpenAI Chat SSE parser now accepts spaced and unspaced ChangesOpenAI Chat SSE parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 `@tests/openai-chat-eof.test.ts`:
- Around line 313-321: Update the “unspaced data:[DONE] sentinel terminates the
stream” test to isolate sentinel handling by removing the preceding answer
frame, or replacing it with a frame lacking answer text and finish_reason. Keep
the assertions verifying a final done event and no error, so success depends
specifically on parsing data:[DONE].
🪄 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: 174df927-1c78-4f2b-812d-26e3cb46fbaf
📒 Files selected for processing (2)
src/adapters/openai-chat.tstests/openai-chat-eof.test.ts
…un#1170) Address CodeRabbit review on lidge-jun#1188: the preceding answer frame carried finish_reason, so the EOF fallback could emit done even if unspaced [DONE] handling were broken. Drive the test with the sentinel frame alone so it fails unless data:[DONE] is parsed.
…un#1170) Address CodeRabbit review on lidge-jun#1188: the preceding answer frame carried finish_reason, so the EOF fallback could emit done even if unspaced [DONE] handling were broken. Drive the test with the sentinel frame alone so it fails unless data:[DONE] is parsed.
9783663 to
0f5ea8f
Compare
…un#1170) Address CodeRabbit review on lidge-jun#1188: the preceding answer frame carried finish_reason, so the EOF fallback could emit done even if unspaced [DONE] handling were broken. Drive the test with the sentinel frame alone so it fails unless data:[DONE] is parsed.
0f5ea8f to
57588f9
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
|
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Changed files and diff ==='
git diff --find-renames --unified=80 HEAD^ HEAD -- \
src/adapters/openai-chat.ts \
tests/openai-chat-eof.test.ts
echo '=== Parser outline ==='
ast-grep outline src/adapters/openai-chat.ts --items all || true
echo '=== EOF test outline ==='
ast-grep outline tests/openai-chat-eof.test.ts --items all || trueLength of output: 12316 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base HEAD origin/dev 2>/dev/null || git merge-base HEAD dev 2>/dev/null || true)"
if [ -z "$base" ]; then
echo "Unable to determine the dev merge base."
git branch -a --no-color
exit 0
fi
echo "=== Merge base ==="
printf '%s\n' "$base"
echo "=== PR commits ==="
git log --oneline --reverse "${base}..HEAD"
echo "=== Full PR diff ==="
git diff --find-renames --unified=40 "${base}..HEAD" -- \
src/adapters/openai-chat.ts \
tests/openai-chat-eof.test.ts
echo "=== parseStream region ==="
start="$(rg -n 'parseStream\s*[:(]' src/adapters/openai-chat.ts | head -1 | cut -d: -f1 || true)"
if [ -n "$start" ]; then
sed -n "${start},$((start + 300))p" src/adapters/openai-chat.ts | nl -ba -v "$start"
fi
echo "=== SSE framing tests ==="
start="$(rg -n 'SSE data-field framing' tests/openai-chat-eof.test.ts | head -1 | cut -d: -f1 || true)"
if [ -n "$start" ]; then
sed -n "${start},$((start + 120))p" tests/openai-chat-eof.test.ts | nl -ba -v "$start"
fiLength of output: 1291 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Recent commits ==="
git log --oneline --decorate -12
echo "=== Adapter stream parser ==="
sed -n '720,1160p' src/adapters/openai-chat.ts | nl -ba -v 720
echo "=== EOF test file ==="
sed -n '1,420p' tests/openai-chat-eof.test.ts | nl -ba -v 1
echo "=== Current parser framing expressions ==="
rg -n -C 8 'data:|\\[DONE\\]|finish_reason|TextDecoderStream|JSON\.parse' \
src/adapters/openai-chat.ts
echo "=== Diffs in the last three commits ==="
git diff --find-renames --unified=30 HEAD~3..HEAD -- \
src/adapters/openai-chat.ts \
tests/openai-chat-eof.test.ts 2>/dev/null || \
git diff --find-renames --unified=30 HEAD~1..HEAD -- \
src/adapters/openai-chat.ts \
tests/openai-chat-eof.test.tsLength of output: 1997 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered() {
awk -v start="$2" 'BEGIN { n = start } { printf "%6d %s\n", n++, $0 }' "$1"
}
echo "=== Parser implementation: Lines 720-1160 ==="
sed -n '720,1160p' src/adapters/openai-chat.ts > /tmp/openai-chat-parser.txt
numbered /tmp/openai-chat-parser.txt 720
echo "=== EOF regression tests: Lines 1-420 ==="
sed -n '1,420p' tests/openai-chat-eof.test.ts > /tmp/openai-chat-eof.txt
numbered /tmp/openai-chat-eof.txt 1
echo "=== Parser fix commit ==="
git show --format=fuller --find-renames --unified=60 17a006f2 -- \
src/adapters/openai-chat.ts \
tests/openai-chat-eof.test.ts
echo "=== Sentinel-test follow-up ==="
git show --format=fuller --find-renames --unified=40 57588f9a -- \
tests/openai-chat-eof.test.tsLength of output: 68234
|
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/openai-chat-eof.test.ts`:
- Around line 301-311: Strengthen the test in the “unspaced data:{...} frames
with a final finish_reason and no [DONE] complete” case so it verifies the
terminal finish_reason was processed, not merely EOF completion after prior
output. Assert that the final done event records the stop reason, or remove the
preceding “hi” content so completion depends on the finish-only frame.
🪄 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: a993039b-ef8a-415c-9318-3f6c8b1f8b77
📒 Files selected for processing (2)
src/adapters/openai-chat.tstests/openai-chat-eof.test.ts
…idge-jun#1170) Address the second CodeRabbit review on lidge-jun#1188: with answer text already emitted, the EOF fallback would yield done even if the terminal finish_reason frame were dropped. Switch the terminal frame to finish_reason "length" and assert stopReason "max_tokens" — the fallback emits done without a stopReason, so the assertion now fails unless the terminal frame was processed. (The literal suggestion of expect.any(String) would not hold for "stop", which maps to no stopReason.)
…idge-jun#1170) Some OpenAI-compatible gateways emit valid SSE data fields with no space after the colon (`data:{...}`, `data:[DONE]`). The stream parser only matched `data: `, so every frame — including the terminal finish_reason chunk — was dropped and the turn failed as a false truncation. Match the field name per the SSE spec (value may begin immediately after the colon; one optional leading space is stripped), and skip bare `data:` keep-alive lines instead of reporting them as malformed frames. Regression coverage: unspaced frames with finish_reason and no [DONE] complete, unspaced [DONE] terminates, bare data: lines are ignored, and a genuinely truncated unspaced stream still fails closed.
…un#1170) Address CodeRabbit review on lidge-jun#1188: the preceding answer frame carried finish_reason, so the EOF fallback could emit done even if unspaced [DONE] handling were broken. Drive the test with the sentinel frame alone so it fails unless data:[DONE] is parsed.
…idge-jun#1170) Address the second CodeRabbit review on lidge-jun#1188: with answer text already emitted, the EOF fallback would yield done even if the terminal finish_reason frame were dropped. Switch the terminal frame to finish_reason "length" and assert stopReason "max_tokens" — the fallback emits done without a stopReason, so the assertion now fails unless the terminal frame was processed. (The literal suggestion of expect.any(String) would not hold for "stop", which maps to no stopReason.)
e635178 to
20f693a
Compare
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Closing this as superseded by #1194. #1194 was merged into I reviewed the current #1188 head for correctness, edge cases, leaks, and security and did not find a separate merge-blocking issue worth preserving this PR for. One small behaviour here may still be worth carrying forward separately: ignoring an empty If we want those, I’d move them into a small follow-up against current Thanks for the work on this. |
|
Follow-up opened as #1249 against current dev, carrying the two pieces called out here: empty |
…test Follow-up to lidge-jun#1194 carrying forward the two pieces of lidge-jun#1188 that the superseding PR did not take, per maintainer review: - A bare `data:` line (heartbeat-style keep-alive on some gateways) now yields no payload and is skipped, instead of falling into JSON.parse("") and terminating the stream as a malformed frame. - The unspaced [DONE] regression test is driven by the sentinel frame alone: with a preceding answer frame, the finish-less EOF fallback could emit done even if unspaced [DONE] parsing were broken.
…test Follow-up to lidge-jun#1194 carrying forward the two pieces of lidge-jun#1188 that the superseding PR did not take, per maintainer review: - A bare `data:` line (heartbeat-style keep-alive on some gateways) now yields no payload and is skipped, instead of falling into JSON.parse("") and terminating the stream as a malformed frame. - The unspaced [DONE] regression test is driven by the sentinel frame alone: with a preceding answer frame, the finish-less EOF fallback could emit done even if unspaced [DONE] parsing were broken.
2 participants
Summary
Closes #1170.
Some OpenAI-compatible gateways emit valid SSE data fields with no space after the colon (
data:{...},data:[DONE]). The OpenAI Chat stream parser only matcheddata:(colon + space), so every frame — including the terminalfinish_reasonchunk — was silently dropped, and the turn failed with a false truncation error (upstream stream ended without a terminal signal).Per the WHATWG SSE spec, the field value may begin immediately after the colon; one optional leading space is stripped when present.
Change:
handleDataLineinsrc/adapters/openai-chat.tsnow matches thedata:field name regardless of the following space and trims the payload. A baredata:line (heartbeat-style keep-alive on some gateways) carries no payload and is skipped rather than reported as a malformed frame. No other parser behavior changes: spaced framing,[DONE], inline error envelopes, usage accounting, and the fail-closed truncation paths are all untouched.Regression coverage (
tests/openai-chat-eof.test.ts, new describe block):data:{...}frames with a final non-nullfinish_reasonand no[DONE]complete (the exact framing from the issue report).data:[DONE]terminates the stream — driven by the sentinel frame alone, so the finish_reason EOF fallback cannot mask broken unspaced[DONE]handling (CodeRabbit review finding, addressed).data:line is ignored, not reported as a malformed frame.[DONE], nofinish_reason, no answer text) still fails closed.Verification
bun run typecheck— clean.bun test tests/openai-chat-eof.test.ts— 28/28 pass (4 new).bun run teston the rebased branch — 9569 tests; the only failing names are 11provider management validationtests plustwo processes at the post-approval management seam serialize instead of interleaving. The identical set fails on a pristineorigin/devworktree (eeae008) without this change, so they are pre-existing environment-dependent failures unrelated to this PR.tests/crash-guard.test.tstimed out once under full-suite load and passes standalone on both this branch and baseline.[DONE]test could pass via the EOF fallback) was fixed in the follow-up commit; no other review findings are outstanding.dev(0 commits behind at push time).Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met: