fix(openai-chat): ignore empty data: frames; tighten unspaced [DONE] test - #1249
Conversation
|
✅ Deterministic PR hygiene checks passed. |
📝 WalkthroughWalkthroughThe OpenAI SSE parser now ignores empty ChangesOpenAI SSE empty-frame handling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
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 |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@coderabbitai review |
|
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The code change itself is focused and correct: I independently passed the 41 affected SSE/EOF tests, bun run typecheck, and bun run privacy:scan on 20c7afb5. Ignoring an empty data: value and making the sentinel-only regression test non-vacuous are both worth keeping.
The remaining blocker is branch freshness. This head is now 22 commits behind current dev (d55b903d), so it is outside the repository readiness window. Please rebase onto the current integration head, rerun the same focused checks, and let the required CI complete. I will approve the exact rebased head if the diff remains this two-file change.
20c7afb to
cf390cd
Compare
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR diff ==='
git diff --find-renames origin/dev...HEAD -- src/adapters/openai-chat.ts tests/sse-unspaced-data-fields.test.ts
echo
echo '=== Adapter outline ==='
ast-grep outline src/adapters/openai-chat.ts --items all
echo
echo '=== Changed adapter context ==='
git diff --unified=80 origin/dev...HEAD -- src/adapters/openai-chat.ts
echo
echo '=== Changed test context ==='
git diff --unified=100 origin/dev...HEAD -- tests/sse-unspaced-data-fields.test.ts
echo
echo '=== Related SSE parser and test references ==='
rg -n -C 3 --glob '*.{ts,tsx,js,mjs,cjs}' \
'data:\[DONE\]|data:\s*\[DONE\]|JSON\.parse\(|text/event-stream|unspaced' \
src testsLength of output: 1060 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Current checkout and available refs ==='
git status --short --branch
git branch -a
git remote -v
git show-ref --heads --remotes 2>/dev/null || true
echo
echo '=== Pull request metadata ==='
gh pr view 1249 --repo lidge-jun/opencodex \
--json baseRefName,baseRefOid,headRefName,headRefOid,url
echo
echo '=== Current changed files relative to HEAD parent ==='
git diff --name-status HEAD^ HEAD -- \
src/adapters/openai-chat.ts tests/sse-unspaced-data-fields.test.ts
echo
echo '=== Current commit diff ==='
git diff --find-renames --unified=100 HEAD^ HEAD -- \
src/adapters/openai-chat.ts tests/sse-unspaced-data-fields.test.ts
echo
echo '=== Adapter source map and relevant context ==='
ast-grep outline src/adapters/openai-chat.ts --items all
rg -n -C 12 --glob '*.ts' \
'JSON\.parse\(|\[DONE\]|data:|event-stream' \
src/adapters/openai-chat.ts tests/sse-unspaced-data-fields.test.tsLength of output: 57488 🏁 Script executed: #!/bin/bash
set -euo pipefail
base='517f4460408bd871b40c3650261b23f7c542a873'
head='cf390cdba7ee1625b33e4a37cf6abde0966364fc'
echo '=== Exact PR file summary ==='
git diff --stat "$base" "$head"
git diff --name-status "$base" "$head"
echo
echo '=== Exact PR diff ==='
git diff --check "$base" "$head"
git diff --find-renames --unified=40 "$base" "$head"
echo
echo '=== OpenAI stream line-splitting and EOF handling ==='
sed -n '1040,1155p' src/adapters/openai-chat.ts
echo
echo '=== SSE field decoder implementation ==='
ast-grep outline src/lib/sse-decoder.ts --items all
sed -n '1,220p' src/lib/sse-decoder.ts
echo
echo '=== Current line-numbered changed regions ==='
nl -ba src/adapters/openai-chat.ts | sed -n '946,972p'
nl -ba tests/sse-unspaced-data-fields.test.ts | sed -n '100,145p'Length of output: 114078 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='lidge-jun/opencodex'
pr='1249'
echo '=== Canonical PR metadata ==='
gh api "repos/$repo/pulls/$pr" \
--jq '{number, state, draft, base: .base.sha, head: .head.sha, changed_files, commits, additions, deletions}'
echo
echo '=== Canonical PR changed files ==='
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | {filename, status, additions, deletions, changes, patch}'
echo
echo '=== PR commits ==='
gh api --paginate "repos/$repo/pulls/$pr/commits?per_page=100" \
--jq '.[] | {sha, message: .commit.message}'
echo
echo '=== Check status ==='
gh pr checks "$pr" --repo "$repo" || true
echo
echo '=== Current source and test line ranges ==='
awk 'NR >= 946 && NR <= 972 { printf "%d:%s\n", NR, $0 }' src/adapters/openai-chat.ts
awk 'NR >= 100 && NR <= 145 { printf "%d:%s\n", NR, $0 }' tests/sse-unspaced-data-fields.test.tsLength of output: 8816
The empty-payload guard at The regression tests are effective:
GitHub reports all current PR checks as passing. You are interacting with an AI system. |
…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.
cf390cd to
8fd93a1
Compare
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Temporarily closing and reopening to re-trigger the Cross-platform CI run, which was created without any jobs after the force-push (conclusion: action_required). No code change; head stays 8fd93a1. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
Rebase done per review; CI runs need an approval/rerun@Ingwannu @lidge-jun @Wibias — the head is rebased onto the current integration head ( Local verification on the rebased head
Checklist re-ticked, gate marked the PR ready, maintainers notified. One blocker left: the Cross-platform CI and React Doctor runs for this head were created without any jobs and concluded |
3 participants
Summary
Small follow-up to #1194, carrying forward the two pieces of the superseded #1188 that the maintainer review called out as worth keeping (see the closing comment on #1188):
data:frames in the OpenAI Chat adapter.sseFieldValue(from fix(sse): accept unspaceddata:fields across six parsers (#1170) #1194) correctly yields an empty value for a baredata:line, buthandleDataLinethen falls intoJSON.parse("")and terminates the stream withmalformed upstream SSE data frame. Some gateways emit baredata:lines as heartbeat keep-alives; they now short-circuit as nothing-to-parse, matching the tolerant behavior of the other SSE call sites (claude-messages.ts,chat/outbound.ts, whichcatch { continue }).[DONE]regression test to sentinel-only. The mergedaccepts an unspaced [DONE] sentineltest carries a preceding answer frame, so the finish-less EOF fallback would emitdoneeven if unspaced[DONE]parsing regressed. The test now drives the parser with thedata:[DONE]frame alone, so it fails unless the sentinel is actually recognized (this is the same weakness CodeRabbit flagged on fix(openai-chat): accept SSE data fields without the optional space (#1170) #1188).No other parser behavior changes.
Verification
bun x tsc --noEmit— clean.bun test tests/sse-unspaced-data-fields.test.ts tests/openai-chat-eof.test.ts— 41/41 pass (1 new test, 1 tightened).bun run privacy:scan— passed.517f4460, 0 commits behind at push time). Diff remains the same two files.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit
data:messages.