🤖 fix: require provider stream terminal events#3415
Merged
Conversation
Treat the provider finish part as the proof that a streamed assistant response completed. Non-empty streams that end before a terminal finish now persist a retryable stream_truncated partial instead of committing partial text as success; empty streams keep the existing safe internal retry path. The Copilot Responses adapter now mirrors the same invariant by emitting an error when the SSE stream ends before a terminal Responses event, while treating response.incomplete as a terminal length finish and response.failed as an error. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `.95`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=3.95 -->
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Update model-only stream manager unit tests so their mocked successful streams include the terminal finish event now required before finalization. --- _Generated with `mux` • Model: `openai:gpt-5.5` • Thinking: `xhigh` • Cost: `.95`_ <!-- mux-attribution: model=openai:gpt-5.5 thinking=xhigh costs=3.95 -->
Member
Author
|
@codex review Pushed a follow-up test-only fix for the unit-test mocks that needed to emit the terminal finish event under the new invariant. |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
ethanndickson
added a commit
that referenced
this pull request
Jun 2, 2026
Codex pointed out that the previous discriminator lived in StreamManager
and treated every `finish` part with `(unified: "other", raw: undefined)`
as truncated. The LanguageModelV2 contract permits adapters to emit
that shape as a legitimate terminal event, so the heuristic was too
broad for the provider-agnostic StreamManager.
Move the fix to the boundary where the synthesized default originates.
The @ai-sdk/openai adapter family — Responses, Chat Completions, legacy
Completions — initializes its internal finishReason to
`{ unified: "other", raw: undefined }` and unconditionally emits that
value from its TransformStream.flush() at end-of-stream, even when no
terminal SSE event arrived. The SDK's mappers only return
`unified: "other"` paired with a defined `raw`, so within this adapter
family the `(other, undefined)` shape is unreachable except as the
uninitialized default. Dropping it is safe and intentionally scoped to
the OpenAI provider construction path (and the Copilot path, which
reuses the same adapter).
Implementation: introduce
src/node/services/openAISynthesizedFinishFilter.ts which exposes
`wrapOpenAIModelToFilterSynthesizedFinish(model)`. The wrapper pipes
`doStream`'s output through a TransformStream that drops only the
synthesized-default finish part; all other parts pass through unchanged.
Apply the wrapper at the two `createOpenAI(...)` callsites in
providerModelFactory.ts. With the synthesized finish dropped, the
existing `!receivedTerminalEvent` branch in StreamManager handles a
clean upstream EOF as `stream_truncated` exactly as PR #3415 intended.
Revert the StreamManager-side heuristic and tests from the previous
commit so StreamManager stays provider-agnostic.
LeonidasZhak
pushed a commit
to LeonidasZhak/mux
that referenced
this pull request
Jun 5, 2026
coder#3441) ## Summary Drop `ai`'s synthesized-default `finish` part inside `StreamManager` so that PR coder#3415's missing-terminal-event guard turns a clean upstream EOF into a retryable `stream_truncated` error for **both** OpenAI and Anthropic providers, instead of silently committing partial output as if the assistant finished cleanly. ## Background PR coder#3415 added a `receivedTerminalEvent` guard in `StreamManager` that surfaces a missing terminal SSE event as a retryable `stream_truncated` error. That guard only fires when the SDK stream ends without emitting any `finish` part at all. Empirically that branch was unreachable: every real OpenAI and Anthropic stream ends with a `finish` part — but on truncated upstreams the part is a **synthesized default**, not a real terminal signal. The synthesis originates inside the `ai` package's `streamText`. Its internal `runStep` TransformStream initializes: ```js let stepFinishReason = "other"; let stepRawFinishReason = void 0; ``` and unconditionally emits those values from its own `flush()` at end-of-stream — even when the upstream SSE closed before any terminal event arrived. So every adapter ends up looking, at the StreamManager boundary, like it cleanly finished with `(other, undefined)` regardless of whether it actually did. Per-provider truncation behavior, observed in the installed source: - **OpenAI** (`@ai-sdk/openai`): each adapter (Responses, Chat Completions, legacy Completions) initializes its own `finishReason = { unified: "other", raw: undefined }` and emits it from its own `flush()`. `streamText` normalizes that to `(other, undefined)` and forwards it. - **Anthropic** (`@ai-sdk/anthropic`): the adapter has no `flush()` and only emits its `finish` part on a real `message_stop`. On a truncated stream there is no adapter-level finish at all — and `streamText`'s `runStep.flush()` then synthesizes the same `(other, undefined)` part. Same symptom at the StreamManager layer, two different SDK-internal causes. ## Implementation Filter the synthesized default at the `streamText` → `StreamManager` boundary — the layer that actually produces it. In `StreamManager.processStreamWithCleanup`'s `case "finish":` handler, treat a part whose normalized `finishReason === "other"` **and** `rawFinishReason === undefined` as a non-event: do not set `receivedTerminalEvent = true`. The existing `!receivedTerminalEvent` branch below then routes the stream to `handleTruncatedStreamCompletion`, which writes a retryable `stream_truncated` partial with the streamed text preserved. **Why the discriminator is safe (empirical):** - **OpenAI** — `mapOpenAIResponseFinishReason` and `mapOpenAIFinishReason` only return `unified: "other"` from their `default:` branches, which are reached via `isResponseFinishedChunk` / `isResponseFailedChunk`, both of which carry a defined `raw` value. The `(other, undefined)` shape is therefore unreachable as a real OpenAI finish. - **Anthropic** — `mapAnthropicStopReason` only returns `"other"` for the `"compaction"` case and the `default:` fallback. Both call sites in the adapter (`message_delta` and `message_start` handlers) pair the unified reason with `raw: value.message.stop_reason` (a defined string from the API). `(other, undefined)` is unreachable as a real Anthropic finish. - **streamText's own flush** — the only path in this layer that produces `(other, undefined)` is the synthesized default in `runStep`'s end-of-stream flush. So the discriminator distinguishes precisely between "the SDK fabricated a finish to keep the type system happy" and "the model genuinely finished with `other`". A defensive test guards the false-positive surface: a real `(other, "compaction")` finish must pass through as a clean completion. ## Risks Behavioral change is localized to streams that previously committed partial output silently on a clean truncated EOF. After this change those surface as retryable `stream_truncated` errors — the UX PR coder#3415 originally intended. Regression surface is the synthesized-default discriminator itself: a false positive would treat a legitimate `(other, undefined)` finish as truncated, triggering an unnecessary retry. We mitigate by: 1. Tying the discriminator to the empirically-unreachable `(other, undefined)` shape, verified against the OpenAI and Anthropic mappers (see Implementation). 2. A regression test that asserts real `(other, <raw>)` finishes (e.g. Anthropic's `"compaction"`) still complete cleanly. If a future provider adapter does emit `(other, undefined)` as a real terminal finish, the worst case is a retry — preferable to silently committing partial output as a clean completion. ## Pains The first revision moved this same heuristic into `StreamManager` and was correctly flagged by Codex as theoretically too broad — the public `LanguageModelV2` contract permits any adapter to emit `(other, undefined)` as a legitimate terminal finish. The second revision scoped a similar filter to the `@ai-sdk/openai` adapter callsites, which was contract-safe but did not actually fix the bug: `streamText`'s `runStep.flush()` re-synthesizes the identical part one layer up, and it produced no fix at all for Anthropic where the adapter has no `flush()` to filter in the first place. This revision returns the fix to `StreamManager` but now with concrete evidence — gathered from reading `node_modules/{ai,@ai-sdk/openai,@ai-sdk/anthropic}/dist/index.js` — that the `(other, undefined)` shape is unreachable from the two real adapter mappers we care about, and is uniquely produced by `streamText`'s own flush. The discriminator's safety is a property of the two SDKs in use, not a guarantee of the public V2 contract. --- _Generated with `mux` • Model: `anthropic:claude-opus-4-7` • Thinking: `xhigh` • Cost: `$60.15`_ <!-- mux-attribution: model=anthropic:claude-opus-4-7 thinking=xhigh costs=60.15 -->
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
1 participant
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
Requires provider stream completion to be witnessed by the AI SDK
finishpart before Mux finalizes an assistant message. If a non-empty stream closes before that terminal event, Mux now persists a retryablestream_truncatedpartial instead of committing partial text tochat.jsonlas a successful response.Background
This mirrors the stream-terminal invariant from coder/coder#25074 and coder/fantasy#33: Anthropic streams must reach
message_stop, and OpenAI Responses streams must reach a terminal lifecycle event (response.completed,response.incomplete, orresponse.failed) before the response is considered complete. A clean network EOF can be a proxy or provider drop and is not enough to prove semantic completion.Implementation
StreamManagernow records the terminalfinishpart as the proof required for the success path and classifies non-empty missing-terminal completions as retryablestream_truncatederrors. Empty completions keep the existing safe internal retry path. The Copilot Responses adapter now enforces the same Responses lifecycle locally, mapsresponse.incompleteto a length finish, and surfacesresponse.failedas an error instead of falling through to EOF handling.Already-streamed content is preserved, not discarded
If the stream produced output before closing without a terminal event, that output is not thrown away. The final
flushPartialWriteruns before the truncation guard fires, andpersistStreamErrorwrites aMuxMessagetopartial.jsonwhosepartsarray contains every streamed part accumulated so far (metadata.partial = true,metadata.errorType = "stream_truncated"). The success-path call tohistoryService.updateHistoryis unreachable in this branch, so nothing partial leaks intochat.jsonl. The frontend surfaces the partial turn with a retry barrier; the user sees the streamed text and can retry, and becausestream_truncatedis omitted fromNON_RETRYABLE_STREAM_ERRORS, retry is enabled by default. The newstreamManager.test.tsregression test pins this: after a truncated stream,partial.partsstill contains the streamed text-delta andpartial.metadata.errorType === "stream_truncated".Risks
This makes the success path stricter for every provider routed through
streamText. If a provider adapter fails to emit afinishpart for a genuinely complete response, Mux will now surface a retryable partial instead of committing it. That is intentional for Anthropic and OpenAI Responses and safer than silently finalizing truncated output.Generated with
mux• Model:openai:gpt-5.5• Thinking:xhigh• Cost:$3.95