Skip to content

fix(streaming): decode CR, LF and CRLF server-sent events across chunks (carry #6439) - #6446

Merged
lidge-jun merged 3 commits into
devfrom
codex/carry-6439-sse-cr
Oct 2, 2026
Merged

lidge-jun merged 3 commits into
devfrom
codex/carry-6439-sse-cr

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

Providers that frame server-sent events with a lone CR, the third line ending the event-stream spec allows, had events dropped or merged, because the shared decoder in src/lib/sse-decoder.ts only split on LF. This carries oocheol's #6439 so the decoder accepts LF, CRLF and lone CR, including a CRLF split across fetch chunks. A CR at the end of one chunk followed by an LF at the start of the next counts as one line ending.

  • The first two commits are fix(streaming): decode CR-delimited server-sent events #6439 as written (author: oocheol). A CR never enters the line buffer, so the CR-trim budget step in acceptLine is removed.
  • The follow-up commit replaces fix(streaming): decode CR-delimited server-sent events #6439's per-character delimiter loop with cached native indexOf cursors for the next CR and LF. Every provider stream goes through this decoder, so scanning stays native and linear, and LF-only or CR-only chunks never rescan for a delimiter that has run out.
  • It also adds explicit cases: CRLF split across chunks (with and without an empty chunk between), LF followed by CR, 500 records in one chunk for each framing, a UTF-8 code point split next to a CR, and raw versus escaped CR in data. It gives the Korean guide section a Korean heading and notes the cursor search in structure/transports/byte-accounting.md.

Behaviour notes for review:

  • A raw CR inside a data value now ends the line, as the spec requires. JSON payloads escape CR, so real provider frames are unaffected.
  • The CR no longer counts toward retained line bytes, and the trim allocation is gone. A CRLF stream sitting exactly at the transient or event budget can now pass where it previously failed by one byte. The logical event cap itself (TRANSLATOR_MAX_SSE_EVENT_BYTES) is unchanged, and limit+1 still fails for every framing.
  • Follow-up, left out of scope: several ad hoc LF-only frame splitters that do not use this decoder (src/claude/outbound.ts, src/vision/anthropic-describe.ts, src/lab/conformance/sse-normalize.ts).

Supersedes #6439.

Co-authored-by: 정우철 oocheol@naver.com

Verification

사용자 지시로 로컬 테스트 미실행, hosted CI가 유일한 검증.

Per maintainer instruction, no local test, typecheck or build result is offered as evidence for this PR. The only verification is the hosted CI run at head a41a4ed3cde8acc2f9a563cc549d5f9450b60a78, which includes the full test suite (test 1/4 to test 4/4), the macOS and Windows legs, typecheck, structure, and privacy gates. A read-only independent code review of the decoder and tests also returned PASS; it is review evidence, not test evidence.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults. No credential, auth, workflow or dependency changes. The decoder's translator-budget ownership was checked for leaks.

Summary by CodeRabbit

  • Bug Fixes
    • Streaming responses now correctly recognize LF, CRLF, and standalone CR line endings, including when delimiters are split across network chunks.
    • Improved handling of mixed and incomplete lines, UTF-8 content, and event delivery before the stream closes.
  • Documentation
    • Added English and Korean guidance on supported streaming line endings.

oocheol and others added 3 commits October 2, 2026 19:38
…LF framing

Carry follow-up for #6439. Replace the per-character delimiter loop with cached
indexOf cursors for the next CR and LF, so the shared decoder keeps native,
linear scanning on every provider stream. Add explicit cases for a CRLF split
between chunks (including an empty chunk in between), LFCR, many records in one
chunk for each framing, a UTF-8 code point split next to CR, and raw versus
escaped CR in data. Give the Korean guide section a Korean heading and record
the cursor search in the byte-accounting owner doc.

Co-authored-by: 정우철 <oocheol@naver.com>
@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner October 2, 2026 10:48
@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-02T10:50:39.419750Z a41a4ed PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added the bug Something isn't working label Oct 2, 2026
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 73a9d578-ca2a-4047-92a3-926fe9660245

📥 Commits

Reviewing files that changed from the base of the PR and between a300b57 and a41a4ed.

📒 Files selected for processing (7)
  • docs-site/src/content/docs/guides/codex-integration.md
  • docs-site/src/content/docs/ko/guides/codex-integration.md
  • scripts/test-layout/layout.json
  • src/lib/sse-decoder.ts
  • structure/transports/byte-accounting.md
  • tests/fixtures/test-layout-expected.json
  • tests/responses/sse-carriage-return.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The shared SSE decoder now treats CR, LF, and CRLF as line terminators, including CRLF split across chunks. Added tests cover delimiter combinations, chunk boundaries, UTF-8 splitting, byte-budget release, and dispatch before stream closure. English and Korean guides and byte-accounting documentation describe the behavior.

Changes

SSE line-ending support

Layer / File(s) Summary
CR-aware line decoding
src/lib/sse-decoder.ts
The decoder recognizes CR, LF, and CRLF as line delimiters. It handles CRLF across chunks and caches delimiter search positions to avoid rescanning.
Tests and documentation
tests/responses/sse-carriage-return.test.ts, tests/fixtures/test-layout-expected.json, scripts/test-layout/layout.json, structure/transports/byte-accounting.md, docs-site/src/content/docs/guides/codex-integration.md, docs-site/src/content/docs/ko/guides/codex-integration.md
Tests cover delimiter combinations, chunk boundaries, UTF-8 splitting, byte-budget release, and dispatch before stream closure. Test mappings and documentation describe the supported line endings and decoder behavior.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to a41a4

The expanded SSE line-ending support appears ready to merge after normal checks; no concrete remaining issue is established.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a41a4

The checked streaming size limits and downstream payload checks remain in place, and no introduced security issue was established. Incomplete before-and-after and cancellation coverage prevents a definitive minimal-risk assessment.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — An upstream capable of supplying response bytes can now deliver CR-framed records through the shared decoder to existing consumers. The demonstrated exposure is streaming response interpretation and budgeted accumulation, not a new authentication or privileged-action interface. Exhaustive tenant and service reachability is not established.

Trust Boundaries and Controls

  • observed — The routed collect and start symbols create synthetic byte streams inside tests. In the demonstrated production consumer, framing does not replace JSON parsing or object/type checks, and a completed snapshot remains preferred over accumulated text.

Resilience and Maintainability Implications

  • inferred — Resource release is supported when decoding completes, fails, or the iterator is closed. Signal cancellation settles pending reads, but cancellation alone does not resume an iterator already suspended at a yield. Whether every higher-level cancellation path closes that iterator remains unverified; this is not established as a regression introduced by CR support.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (5 skipped: 5 …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: SSE decoding now supports CR, LF, and CRLF line endings across chunks. The issue reference is relevant and does not obscure the primary chang…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner Author

Maintainer integration decision (dev, admin bypass per MAINTAINERS.md): merging at exact head a41a4ed.

  • Single hosted CI run on this head: 31 pass, 8 path-filtered skips, 0 failing (test 1/4-4/4, gates, structure, docs build, docker smoke, npm-global, keyring, storage policy, api usage, hygiene, enforce-target, react-doctor, CodeRabbit). Local tests were intentionally not run for this lane; hosted CI is the verification.
  • No unresolved review threads; merge-tree against current dev is clean.
  • Coordinator review of src/lib/sse-decoder.ts: CR, LF and CRLF terminate lines, a CR at a chunk end followed by LF at the next chunk start is one delimiter (skipLineFeed survives across chunks and empty decodes), and the scan stays linear via cached indexOf positions. No auth, credential, workflow or logging changes.
  • Carries fix(streaming): decode CR-delimited server-sent events #6439 by @oocheol with Co-authored-by credit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants