fix(openai-chat): complete streams that end after output without terminal - #793
Merged
Merged
Conversation
…hout terminal" This reverts commit 2093bac.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughOpenAI chat EOF handling now distinguishes answer text from reasoning, usage, and buffered tool calls. Text-bearing streams without terminal signals can complete; streams with pending tool calls or no answer text fail closed. Tests cover newline-free, empty, reasoning-only, usage-only, and incomplete tool-call streams. ChangesOpenAI stream EOF 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)
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 `@src/adapters/openai-chat.ts`:
- Around line 807-823: Update the EOF handling around flushToolCalls in the
stream processing flow to detect pending tool calls before flushing them; when
the stream lacks a terminal signal, emit the truncation error without yielding
any tool_call_end events. Ensure usage-only EOF frames cannot bypass this
pending-call validation and incorrectly produce done, and add regression
coverage asserting the error response contains no tool_call_end.
🪄 Autofix (Beta)
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: 31e56125-7204-4ea5-86ec-f408148c0b00
📒 Files selected for processing (2)
src/adapters/openai-chat.tstests/openai-chat-eof.test.ts
![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: 0c829dc5a2
ℹ️ 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".
Fail pending-tool and reasoning/usage-only truncations before flushing tool calls, and only complete when answer text or finish_reason is present.
Collaborator
Author
Verdict: approve-comment
|
lidge-jun
added a commit
that referenced
this pull request
Jul 31, 2026
…complete
At reader EOF the adapter flushed buffered tool calls and only then judged
whether the stream had terminated cleanly. `flushToolCalls()` emits
`tool_call_end`, so a socket that died mid-argument handed the client
`{"cmd":"l` as a COMPLETED call: a truncation reported as a successful tool
invocation, which is worse than an error because nothing downstream can tell
it apart from a real one.
The integrity check now runs before the flush. This adapter can still decide
at that point because it buffers tool calls; the Anthropic adapter forwards
fragments live and genuinely cannot, which is why the equivalent fix there
was reverted (see devlog 260731_pr_merge_round/021).
A usage frame does not launder it either. Usage counts as a terminal signal
for text streams, and without the separate branch a provider that sent usage
after dying mid-tool would still flush the partial call.
What this deliberately does NOT do is the other half of #735. PR #793 (and
#773 before it, merged then reverted by #792 with no recorded reason)
additionally completes text-only streams that end without a terminal frame.
That is a policy change, not a bug fix: it inverts two existing fail-closed
assertions in tests/openai-chat-eof.test.ts, which currently pin "content
without a terminator is truncation". Whether a relay's dropped terminal frame
should be forgiven is the maintainer's call, and the missing revert rationale
on #792 is exactly the context needed to make it. Attempting both halves here
is how the previous round burned three repair rounds on the Anthropic side.
So the narrowing half lands and the widening half does not. #735 stays open.
Tests, verified by ablation (disable the guard -> 13 pass / 2 fail, restore ->
15/15):
- a half-assembled tool call at EOF errors, emitting no tool_call_end or done
- a trailing usage frame does not rescue it
- a tool call closed by finish_reason still completes normally -- the control,
without which the guard could fire on every well-formed tool turn
Adjacent suites green: adapter-usage, openai-chat-parallel-stream.
2 tasks
Keep the restored lidge-jun#773 finish-less EOF rules (answer text required; pending tool calls fail closed before flush) while taking tip's clearer mid-tool-call truncation message.
Windows CI flakes when Bun's default 5s test budget races star-state's 5s auth kill on a slow credential helper. Raise only these two route tests to cover AUTH+API budgets.
Collaborator
Author
|
Merging this. Why it helps: Providers that close a chat stream after emitting answer text without [DONE] / inish_reason no longer hang the client, while mid-assembly tool-call EOFs still fail closed before a fabricated ool_call_end. |
|
@Wibias @lidge-jun need your help bro。not only openai-chat adapter has this question,but also openai-response adapter have。 |
Collaborator
Author
Huh? What do you mean exactly? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
38173bf7onfix/735-openai-chat-eof: accept finish-less EOF when text was emitted; fail closed when EOF leaves pending tool calls mid-assemblyTest plan
bun test tests/openai-chat-eof.test.tsSummary by CodeRabbit