fix(json stream): emit all objects in a chunk, not just the first - #2487
fix(json stream): emit all objects in a chunk, not just the first#2487Diwak4r wants to merge 1 commit into
Conversation
extract_json_from_stream / _async broke the char loop on the first complete object, dropping later JSON objects in the same chunk. Changed to continue so remaining chars are still scanned.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 516435c1b4
ℹ️ 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".
| buffer = [] | ||
| json_started = False | ||
| break | ||
| continue |
There was a problem hiding this comment.
When a complete JSON value and trailing prose arrive in the same provider chunk, this continue resumes scanning outside the JSON value, and any later balanced {...} or [...] prose is emitted because new roots are accepted without validation. For example, before {"a":1} See [section] for details now yields {"a":1}[section], corrupting downstream parsers; before this change the same-chunk suffix was skipped. Consider continuing only across whitespace/separators into another immediate JSON root rather than scanning arbitrary suffix text.
Useful? React with 👍 / 👎.
ErenAta16
left a comment
ErenAta16
left a comment
There was a problem hiding this comment.
Checked this out and ran it against main before and after the change. Three things, and I think they point away from merging as-is.
1. The described bug doesn't reproduce on current main.
Both of the new tests pass without the source change:
main, unmodified:
['{"a":1}{"b":2}'] -> '{"a":1}{"b":2}'
['```json\n{"a":1}{"b":2}\n```'] -> '{"a":1}{"b":2}'
So multiple complete objects in one chunk are already retained. The Unreleased CHANGELOG on main says as much: "retain every JSON object when multiple objects arrive in one streaming chunk". Looks like this was fixed already and the PR is chasing a bug that's since been closed.
2. Consequently both new tests are vacuous.
They're green with the fix reverted, so they don't pin the behavior this PR changes. Worth checking any test added alongside a fix by reverting only the source hunk, a test that stays green isn't protecting anything. (I've been caught by exactly this myself, so no criticism intended, it's just easy to miss.)
3. The change isn't a no-op, and where it does bite it makes output worse.
The break you're changing is not on the "object completed" path. It's on the path where a closing fence arrives while json_started is still True, that is, a truncated JSON object followed by ```. Diffing the two versions over a range of inputs, five of six cases are identical and this is the one that differs:
input: ['```json\n{"a":1', '```{"b":2}']
main -> '{"a":1'
patched -> '{"a":1{"b":2}'
break stops at the fence and emits just the truncated fragment. continue keeps parsing and glues the next object onto the unterminated one, producing {"a":1{"b":2}, which is not valid JSON and will fail downstream parsing in a more confusing way than a plainly truncated fragment would.
That's the same failure shape as #2456 (balanced-bracket spans being concatenated into invalid output), so it'd be trading one instance of that bug for another.
Suggestion: I'd close this unless you have a repro that fails on current main. If you do have one, it'd be worth posting, because it would mean the earlier fix has a gap and that's worth knowing. If the truncated-then-fence case is the thing you actually want to improve, that's a real question but the answer probably isn't continue, it's deciding whether a truncated object should be emitted at all or dropped, and that deserves its own issue.
Happy to take another look if you push a failing-on-main repro.
|
Consolidated and shipped in #2495. Closing this focused patch as superseded; thank you for the contribution. |
When the JSON streaming parser receives a chunk containing multiple complete JSON objects, it only emits the first one and silently drops the rest. This causes data loss during streaming.
Fix: Continue parsing after extracting the first complete JSON object from a chunk, emitting all remaining objects before returning.