fix(providers): complete repaired tool call streams#104714
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 117db716cb
ℹ️ 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".
| delta: typeof record.partialArgs === "string" ? record.partialArgs : "{}", | ||
| partial: message, | ||
| }); | ||
| stream.push({ type: "toolcall_end", contentIndex, toolCall: record, partial: message }); |
There was a problem hiding this comment.
Complete the runtime promotion path too
When a provider does not opt into this SDK wrapper and instead relies on the embedded-agent wrapStreamFnPromoteStandaloneTextToolCalls path, the same normalizer still receives a start/delta-only factory in src/agents/embedded-agent-runner/run/attempt.tool-call-normalization.ts:932-949, and src/agents/embedded-agent-runner/run/attempt.ts:3162-3168 applies that wrapper to active sessions. Those promoted streams still emit toolcall_start, toolcall_delta, then done, so streaming consumers waiting for toolcall_end remain broken outside the provider-local wrapper fixed here.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed July 11, 2026, 5:28 PM ET / 21:28 UTC. Summary PR surface: Source +1, Tests +19, Docs +1. Total +21 across 4 files. Reproducibility: yes. The release-base helper visibly omits Review metrics: none identified. Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsDo we have a high-confidence way to reproduce the issue? Yes. The release-base helper visibly omits Is this the best way to solve the issue? Yes. Completing the lifecycle in the existing shared release helper is narrower and safer than backporting current main’s larger stream-normalization refactor. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against fc0fd1bddaa3. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +1, Tests +19, Docs +1. Total +21 across 4 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
Merged via squash.
|
What Problem This Solves
The July release branch promotes serialized plain-text tool calls for LM Studio, xAI, and Ollama, but its shared compatibility wrapper emits only
toolcall_startandtoolcall_deltabeforedone. Plugin Prerelease exposed this as deterministic LM Studio failures, and the incomplete lifecycle can leave streaming consumers without the finalized tool-call event.Why This Change Was Made
Emit the provider-neutral
toolcall_endevent from the existing shared promotion helper, matching theAssistantMessageEventcontract and currentmainbehavior. The shared and Ollama tests now assert both lifecycle ordering and the finalized payload. This avoids backporting the much larger stream-normalization refactor while fixing the exact release-owned contract gap.User Impact
LM Studio, xAI, and Ollama serialized tool-call repair now completes the streaming lifecycle before the final message, so consumers can finalize repaired calls consistently.
Evidence
oxfmtandgit diff --check: green.