fix(codex): avoid rescanning fragmented jsonl frames - #8439
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c1db600ee
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Stream.splitLines, | ||
| Stream.runForEach(handleLine), |
There was a problem hiding this comment.
Add focused coverage for fragmented JSONL frames
This replaces the JSONL framing state, but the commit adds no test that splits one JSON object across stdin chunks or delivers multiple frames in one chunk; the existing protocol tests only enqueue complete newline-terminated frames. Those boundaries are precisely the behavior being changed, so a buffering or final-frame-flushing regression could strand pending requests without CI detecting it. Add a focused protocol test covering fragmented, coalesced, and unterminated-at-EOF input.
AGENTS.md reference: AGENTS.md:L108-L108
Useful? React with 👍 / 👎.
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — This is a small, self-contained Codex transport fix that replaces repeated JSONL buffer rescanning with Effect's standard line splitter while preserving fragmented-frame and EOF handling. It changes only the incoming framing implementation and introduces no schema, security, billing, infrastructure, or cross-component behavior. You can add or adjust custom eligibility rules. Learn more. |
Replaced the manual buffered JSONL parser with Effect's
Stream.splitLines.Added regression coverage for messages fragmented across stdio chunks and multiple messages arriving in one chunk.
Why
The previous reader concatenated each chunk with the pending remainder and split the entire accumulated string again. Large fragmented Codex messages repeatedly scanned a growing buffer, delaying output processing.
Stream.splitLineshandles chunk boundaries and final unterminated lines without custom framing state.Checklist
Model: GPT-5.6-sol. Harness: T3 Code.
Note
Medium Risk
Changes JSONL framing and EOF behavior on the Codex app-server transport; incorrect line splitting could drop or mis-parse messages, though the change aligns with a standard stream primitive.
Overview
Replaces the custom stdio JSONL reader in
makeCodexAppServerPatchedProtocolwith Effect’sStream.splitLines, removing theremainderref and per-chunk re-split of the full buffer.Incoming stdin is now
decodeText→splitLines→runForEach(handleLine), so line boundaries are handled incrementally instead of repeatedly scanning a growing concatenated string (which delayed processing of large fragmented Codex messages). Stream-end handling is simplified: on successful stdin completion it goes straight tohandleTerminationwithCodexAppServerInputStreamEndedError(orterminationError), without a final flush of a leftover partial line.Reviewed by Cursor Bugbot for commit 8c1db60. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Avoid rescanning fragmented JSONL frames in
makeCodexAppServerPatchedProtocolReplaces the manual chunk-accumulation and
Ref-based remainder buffer in protocol.ts withStream.splitLines, then runshandleLinedirectly on each emitted line. This removes per-chunk rescanning of buffered data.'\r'stripping is dropped (delegated toStream.splitLinessemantics), and on successful stdin completion the pipeline no longer flushes a stored remainder before invoking termination handling.Macroscope summarized 8c1db60.