Repository navigation
Conversation
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fixes #56455. I ran the issue's stream (answer, then a finish_reason: "stop" chunk, then a reasoning-only chunk) in the TUI with the simulated model. The base shows OpenAI Chat received content after the finish reason; with this change the step finishes normally (screenshots below). The OpenAI Chat, OpenAI-compatible and OpenRouter test files pass, and tsc --noEmit on packages/ai is clean. The public API is unchanged.
Absorbing every late delta also removes the only guard against some malformed tool-call streams. These now pass silently instead of failing:
- Repeated tool call: if the provider sends a tool call again after the finish (same
id), it is returned twice, so the tool would run twice. - Unidentified late tool delta: a tool delta after the finish with no
id/name(e.g. leftover argument text for a call that was already finished at the finish chunk) is dropped without any error.
Details are inline. Before the finish chunk, both shapes are errors, and they should stay errors (or be deduplicated) after it too.
The formatting-only hunks in openai-chat.ts and openai-compatible-chat.test.ts (schema struct, lowerUserMessage, lowerToolMessages, lowerMessages, the system-message expectations) are unrelated to the fix and can be dropped to keep the diff focused.
For context, #56457 fixes the same issue more narrowly: it only ignores reasoning after the finish.
Related: #56457
| return [{ ...state, usage }, events] as const | ||
| } | ||
|
|
||
| // Deltas that arrive after a terminal chunk are absorbed into the open |
There was a problem hiding this comment.
Calls finalized at the first finish chunk are removed from tools by finishAll, so a later tool delta can no longer be matched to them. If a host sends the same call again after the finish (same id, name and arguments), it starts a second tool and the response has two identical toolCalls, so the tool would run twice. Before this change that stream failed. Could late deltas whose id matches an already finalized call be ignored, or rejected, rather than starting a new call?
There was a problem hiding this comment.
Addressed in bee7f9b: the parser now records the ids of every call finalized in-step and drops trailing deltas whose resolved id matches one, so a replayed call can no longer open as a second call and run twice. Covered by a new test (ignores a replay of an already finalized tool call) that sends the identical index/id/name/arguments call after the stop and asserts a single tool call, unchanged input, and the "tool-calls" finish upgrade. Full packages/ai suite: 1888 pass / 0 fail.
| // unconfirmed — the same rule that applies when the finish arrives. | ||
| const truncatedFinish = | ||
| state.finishReason?.normalized === "content-filter" || state.finishReason?.normalized === "length" | ||
| const lateToolEvents = |
There was a problem hiding this comment.
Only state.tools are finalized here. A tool delta that arrives after the finish without id/name goes into pendingTools, and nothing checks it at the end. The "missing id or name" check in step() only runs on the first terminal chunk. So content, stop, then { tool_calls: [{ index: 0, function: { arguments: "{\"a\":1}" } }] } settles as stop with no tool calls, and the data is dropped silently. The same happens to an argument fragment for a call that was already finished. Please fail on leftover pendingTools here, the way the finish chunk does, so this kind of stream doesn't silently drop data.
There was a problem hiding this comment.
Addressed in bee7f9b: finishEvents now fails with the same "OpenAI Chat tool call delta is missing id or name" error when the step settles with unresolvable pendingTools left over after a finish chunk, treating it like the mid-stream check does. Truncation finishes (length, content-filter) keep their existing leniency, and streams without a finish reason are untouched. Covered by a new test (fails on tool fragments left unresolvable after the finish); full packages/ai suite: 1888 pass / 0 fail.
|
the truncation tests stream the tool call before the finish chunk. nothing covers a tool call that opens after a length or content-filter finish, which is the new post-finish path finishEvents gates with truncatedFinish. the stop-upgrade case has a test, this one does not. worth pinning so the rule cannot regress silently. |
There was a problem hiding this comment.
Both earlier points are fixed: a repeated tool call with the same id is now dropped, and a late tool delta with no id/name now fails instead of being lost. I re-ran the #56455 reproduction in the TUI and the step still finishes normally. The OpenAI Chat, OpenAI-compatible and OpenRouter test files pass (137), and tsc --noEmit on packages/ai is clean. The public API is unchanged.
The new end-of-stream check has one false positive: an empty trailing tool delta ({ index: 0, function: { arguments: "" } }) after a tool_calls finish now fails the step, while the base accepts it (details inline).
| // leniency here. | ||
| const truncatedFinish = | ||
| state.finishReason?.normalized === "content-filter" || state.finishReason?.normalized === "length" | ||
| if (state.finishReason !== undefined && !truncatedFinish && Object.keys(state.pendingTools).length > 0) |
There was a problem hiding this comment.
This also fails on empty fragments. With the stream tool_calls:[{index:0,id:"c1",function:{name:"lookup",arguments:"{}"}}], then a tool_calls finish, then tool_calls:[{index:0,function:{arguments:""}}], the step now fails with "missing id or name". The base ignores that chunk (it isn't late content) and returns the call. Could you only count pending fragments that carry an id, a name or non-empty arguments, or skip empty late deltas before they reach pendingTools? A small test for that shape would help too.
There was a problem hiding this comment.
Addressed in 3eff40a: the close-settle check now only counts fragments that carry an id, a name, or non-empty argument text � finishEvents were parked in pendingTools as a no-op and sank too. Base behavior for that chunk is restored: the already-finalized call settles, the stream succeeds. Covered by the new test (ignores trailing no-op tool deltas after a finalized call) in both test files, mirroring your exact stream. Full packages/ai suite after the change: 1890 pass / 0 fail, focused suites openai-chat 86 + compatible 31, typecheck clean.
There was a problem hiding this comment.
The empty-fragment issue is fixed. An empty trailing tool delta after the finish no longer fails the step, while late fragments that carry an id, a name or argument text still do. I re-checked the earlier tool-call cases (a repeated call returned once, an unidentified late fragment rejected, a late split-args call parsed) and re-ran the #56455 reproduction in the TUI: the step finishes normally. The OpenAI Chat, OpenAI-compatible and OpenRouter test files pass (139), and tsc --noEmit on packages/ai is clean. The public API is unchanged. No new concerns.
|
Pinned in |
There was a problem hiding this comment.
The new commit only adds tests (late tool calls under length/content-filter stay unconfirmed); they are small and clear. My earlier points stay fixed.
The dev merge changes the picture. #58851 already keeps text and reasoning that arrive after the finish chunk, so #56455 and the finish-before-content gateway case are fixed on dev (I checked both with dev's parser). #58851 deliberately still rejects tool-call deltas after the finish. The only behaviour this pull request still adds is accepting those late tool calls, plus the safeguards around them. That reverses the rule #58851 kept, so a maintainer should decide whether late tool calls should be accepted at all. If they should, the description and the Closes #56455 line should say this is now about late tool calls. Also, the first-finish-wins logic in step() now duplicates dev's finishReason: state.finishReason ?? finishReason; one of them can go.
Tests: openai-chat, openai-compatible-chat, openrouter pass (144), and tsc --noEmit on packages/ai is clean. The public API is unchanged.
ndavat
changed the title
github-actions
Bot
added
the
needs:compliance
|
Thanks � all three points addressed:
Full packages/ai suite after the dedupe: 1933 tests, 0 fail, typecheck and |
There was a problem hiding this comment.
The duplicated first-finish-wins logic is gone. step() now relies on dev's state.finishReason ?? finishReason, and because tool calls are only finalized on the first finish chunk, a later chunk with its own finish reason still can't change the outcome. I re-checked that and the earlier tool-call cases: a repeated call is returned once, an unidentified late fragment fails, an empty late fragment is ignored, and a late split-args call parses. The PR's own tests still cover truncated finishes. The OpenAI Chat, OpenAI-compatible and OpenRouter test files pass (144), and tsc --noEmit on packages/ai is clean. The public API is unchanged.
No new concerns. The remaining question is a maintainer decision: whether to accept fully identified tool calls after the finish, which #58851 deliberately rejects.
github-actions
Bot
removed
the
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
There was a problem hiding this comment.
This push only merges dev; nothing under packages/ai changed, so my previous review still holds. The public API is unchanged. The open question is unchanged too: whether to accept fully identified tool calls after the finish, which #58851 deliberately rejects.
There was a problem hiding this comment.
This push only merges dev (util, app and UI changes); nothing under packages/ai changed, so my previous review still holds. The public API is unchanged. The open question remains whether to accept fully identified tool calls after the finish, which #58851 deliberately rejects.
There was a problem hiding this comment.
This push only merges dev (CI workflow changes); nothing under packages/ai changed, so my previous review still holds. The public API is unchanged. The open question remains whether to accept fully identified tool calls after the finish, which #58851 deliberately rejects.
There was a problem hiding this comment.
This push only merges dev; nothing under packages/ai changed, so my previous review still holds. The public API is unchanged. The open question remains whether to accept fully identified tool calls after the finish, which #58851 deliberately rejects.
…-post-finish-deltas # Conflicts: # packages/ai/src/protocols/openai-chat.ts
|
Synced the branch with current The only merge conflict was Full |
Issue for this PR
Does not close anything: the text/reasoning half of #56455 already landed in #58851. This PR now proposes the half #58851 deliberately left out, as a follow-up.
Type of change
What does this PR do?
#56455 reported a stream where real output arrives after
finish_reason. #58851 merged and now absorbs late text and reasoning, but intentionally keeps failing on late tool-call deltas.I originally opened this PR before #58851 to absorb all late deltas. Since it was rebased onto the merge of
dev, the only behavior this adds versusdevis the part #58851 rejected: a fully-identified tool call that opens afterfinish_reasonstill finalizes, instead of failing the whole step. This also replaces #58851's rejection test (rejects tool changes after a terminal chunk) with the absorb coverage and its safeguards pinned below, since the two pin opposite outcomes for the same stream.Why I think this is worth doing:
finish_reason, re-emits buffered content before it) as a workaround, and the PR would make that unnecessary.The review rounds on this PR pinned the safety edges:
tool-callsfinish upgrade is unchanged.length/content-filterfinishes stay strict: late calls remain unconfirmed and never execute.One thing I want to be clear about: #58851 authors chose rejection deliberately, and this PR reverses that choice. Post-finish ordering is outside what the OpenAI contract pins, so I have no spec argument, just the gateway evidence above. If maintainers prefer the strict rule, closing this PR is fine and the workaround stays.
How did you verify your code works?
Fully verified with the test suite and by replaying the issue's stream shape:
packages/aifull suite: 1,933 tests, 0 failures, 28 skipped (includes the tests named in the review threads:ignores a replay of an already finalized tool call,fails on tool fragments left unresolvable after the finish,ignores trailing no-op tool deltas after a finalized call,does not finalize tool calls that open after a truncated finish, plus the truncation and absorb coverage).bun run typecheckonpackages/aiand rootbun run checkboth pass.Screenshots / recordings
Not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #56455, #56457