fix(openai→claude): suppress empty/duplicate tool_use content_block_start#2926
fix(openai→claude): suppress empty/duplicate tool_use content_block_start#2926slicenferqin wants to merge 8 commits into
Conversation
…tart When translating OpenAI streaming responses to Anthropic format, `convertOpenAIStreamingChunkToAnthropic` emits a `content_block_start` event with `name` whenever a chunk's `tool_calls[].function.name` field exists. Two failure modes: 1. **Empty name passes through.** `gjson.Result.Exists()` returns true for `"name": ""` and `"name": null`. The empty string flows into `accumulator.Name` and into the emitted `content_block_start`, which makes downstream Anthropic-format clients (Amp CLI's tool_use normalizer in particular) skip the block as malformed: "Skipping tool_use normalization due to missing name". 2. **Duplicate emissions.** Some upstream providers send the `function.name` field redundantly in multiple streaming chunks for the same tool index. The original code emitted a fresh `content_block_start` every time, producing two start events for one logical tool block. This breaks parsers that expect one start per index and surfaces in Amp as a JSON Parse error. Fix: - Reject empty-string names before assigning to `accumulator.Name`. - Track per-tool emission state with `ToolCallAccumulator.StartEmitted` and only emit `content_block_start` once per tool index, after a non-empty name has been observed. Reproduction: connect Amp CLI through CLIProxyAPI's openai-compatibility provider and run any task that triggers concurrent tool calls (e.g., "explore this repo" → multiple parallel Read/Grep). Without the patch, ~10–20% of runs fail with "JSON Parse error: Unable to parse JSON string" and the assistant turn is lost. With the patch: 12/12 reliability runs pass and a real repo-exploration task completes cleanly.
|
This pull request targeted The base branch has been automatically changed to |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a StartEmitted flag to the ToolCallAccumulator to prevent duplicate content_block_start events during OpenAI-to-Anthropic streaming translation. The review feedback highlights the need to ensure cleanup loops also check this flag to avoid protocol violations, suggests optimizing the tool name assignment logic, and warns about potential non-sequential block indices if tool calls are suppressed.
| // streaming chunks (or repeat it); without this guard we emit duplicate | ||
| // content_block_start events, which crashes Anthropic-format clients | ||
| // (e.g. Amp's tool_use normalizer). | ||
| StartEmitted bool |
There was a problem hiding this comment.
The introduction of StartEmitted is a correct approach to prevent duplicate content_block_start events. However, this change allows param.ToolCallsAccumulator to contain entries for tool calls that were never successfully started (e.g., if the stream ends before a non-empty name is received).
Ensure that the cleanup loops (at lines 307 and 371 in the full file) are updated to check accumulator.StartEmitted before emitting content_block_delta or content_block_stop events. Sending a stop event for a block index that was never started is a violation of the Anthropic streaming protocol and can cause client-side errors.
| if name := function.Get("name"); name.Exists() && name.String() != "" { | ||
| accumulator.Name = util.MapToolName(param.ToolNameMap, name.String()) | ||
| } |
There was a problem hiding this comment.
The tool name is currently updated in every chunk that contains a non-empty name, even after the content_block_start event has been emitted. This is redundant. Wrapping this logic in a check for !accumulator.StartEmitted improves efficiency and ensures the name remains consistent with what was originally emitted.
if !accumulator.StartEmitted {
if name := function.Get("name"); name.Exists() && name.String() != "" {
accumulator.Name = util.MapToolName(param.ToolNameMap, name.String())
}
}| accumulator.Name = util.MapToolName(param.ToolNameMap, name.String()) | ||
| } | ||
|
|
||
| if !accumulator.StartEmitted && accumulator.Name != "" { |
There was a problem hiding this comment.
By suppressing the emission of content_block_start for tool calls with empty names, this logic may create gaps in the Anthropic content block indices. Since toolContentBlockIndex is called at line 229 (outside the diff) for every tool call seen, an index is reserved even if the block is never emitted. Anthropic expects sequential indices (0, 1, 2...). If a tool call is suppressed, the next block (e.g., a text block) will have an index that skips the suppressed one, which may cause protocol errors in strict clients. Consider refactoring the index assignment to occur only when a block is actually emitted.
![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: bec7c56145
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| accumulator.Name = util.MapToolName(param.ToolNameMap, name.String()) | ||
| } | ||
|
|
||
| if !accumulator.StartEmitted && accumulator.Name != "" { |
There was a problem hiding this comment.
By gating content_block_start on name.String() != "", this path now leaves StartEmitted false for tool calls that never receive a non-empty name, but the finish handlers still unconditionally emit content_block_delta/content_block_stop for every accumulator entry. In that scenario the stream contains delta/stop events for a block that was never started, which violates Anthropic event ordering and can cause strict clients to fail parsing the assistant turn instead of merely ignoring that tool call.
Useful? React with 👍 / 👎.
…ted tool blocks Address review feedback on PR router-for-me#2926: - Skip content_block_delta/stop emission in both cleanup loops (streaming finish_reason path and [DONE] path) when StartEmitted is false. Without this, tool calls whose start was suppressed (empty name throughout the stream) still got delta/stop events, violating Anthropic event ordering. - Defer toolContentBlockIndex assignment until the moment we emit content_block_start, so suppressed tool calls don't burn a sequential index and leave gaps that strict clients reject. - Gate name assignment on !StartEmitted so a repeated name field on later chunks can't drift from what was already announced.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bab86cdd0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if name := function.Get("name"); name.Exists() && name.String() != "" { | ||
| accumulator.Name = util.MapToolName(param.ToolNameMap, name.String()) | ||
| } | ||
| } | ||
|
|
||
| if !accumulator.StartEmitted && accumulator.Name != "" { | ||
| stopThinkingContentBlock(param, &results) |
There was a problem hiding this comment.
This change suppresses content_block_start when function.name is empty, but the stream still marks SawToolCall for every tool_calls delta and later maps the final stop reason to tool_use. In the exact malformed-stream case this patch handles (all tool names empty/null), the response now ends with stop_reason: tool_use while emitting no tool_use content blocks at all, which can break Anthropic-compatible clients that require at least one tool block before executing tools. Please derive stop reason from emitted tool blocks (e.g., any StartEmitted) instead of raw tool_calls presence.
Useful? React with 👍 / 👎.
|
Hi @luispater — heads up that I just pushed The original PR (commit
Brief recap for context:
Let me know how you'd like to proceed. Thanks! |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
This change hardens OpenAI→Anthropic streaming translation for tool_use blocks by (1) suppressing empty function.name values, (2) preventing duplicate content_block_start emissions for the same tool index, and (3) ensuring we never emit delta/stop events (or reserve block indexes) for tool blocks that never successfully started.
What changed
- Added
ToolCallAccumulator.StartEmittedto track whether a tool block has already emittedcontent_block_start. - Only records a non-empty
function.nameprior to start emission; repeated name fields after start cannot drift the announced name. - Defers
toolContentBlockIndex()reservation until the momentcontent_block_startis emitted, preventing index gaps for suppressed tool calls. - Skips
content_block_delta/content_block_stopemission in both cleanup paths whenStartEmittedis false, preserving Anthropic event ordering.
Key findings
- The approach is minimal and aligns with Anthropic streaming expectations (exactly one
content_block_startper tool block, and no stop without start). - Deferring index allocation until emit-time is a good fit for “name never materializes” scenarios and avoids strict-client issues with sparse indexes.
Suggested follow-ups (non-blocking)
- Consider requiring a non-empty tool call
idbefore emittingcontent_block_start, similar to the name guard, to avoid ever announcing an emptycontent_block.idif an upstream sends fields out-of-order. - Consider tightening the name check to ensure
nameis actually a JSON string type before mapping/using it.
Test plan
- CI:
pr-test-buildjob reports success. - Recommended additional coverage (if feasible): a small unit test matrix for streaming chunks covering
name:"",name:null, repeatednameacross chunks, and multiple tool indexes to assert no duplicate starts and no stop-without-start behavior.
This is an automated Codex review result and still requires manual verification by a human reviewer.
…aming tests Address maintainer follow-up review on PR router-for-me#2926: - Require accumulator.ID != "" before emitting content_block_start. Without this, an upstream that sends `name` first and `id` later would cause SanitizeClaudeToolID's empty-input fallback path to invent a synthetic `toolu_<nanos>_<n>` id and announce it on the wire, even though the real id arrives in a later chunk. - Tighten the name guard to require gjson.String type, so a malformed upstream sending `"name": 123` or other non-string values no longer silently coerces into the announced tool name. - Tighten the id accumulator the same way (string type, non-empty value) for symmetry and to avoid stomping a previously-set id with a later "id":"" chunk. - Add openai_claude_response_test.go covering: empty name throughout, null name, non-string name, repeated name across chunks, mixed suppressed-and-valid tool indexes (no index gap), and id-arrives-late (start deferred until id is present, no fallback id announced).
|
Hi @luispater — thanks for the thorough review! Just pushed 1. Non-empty 2. Name as JSON-string type only (your second nit) 3. Unit test matrix (your test plan)
All six pass locally; Heads up: this commit will trip |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a9080438e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if function := toolCall.Get("function"); function.Exists() { | ||
| if name := function.Get("name"); name.Exists() { | ||
| accumulator.Name = util.MapToolName(param.ToolNameMap, name.String()) | ||
| // Only record the name until content_block_start has been | ||
| // emitted. Upstreams sometimes send "name":"" or repeat the |
There was a problem hiding this comment.
convertOpenAIStreamingChunkToAnthropic now requires both name and id before emitting content_block_start, but that emission check is nested under if function.Exists(). If an upstream sends function.name first and later sends id in a delta without a function object, accumulator.ID is stored but the start is never emitted, and the finish handlers then skip the call because StartEmitted stays false. This drops a real tool call for providers that split tool-call fields across chunks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — that's a real regression I introduced in 4a90804. Fixed in a912284e:
Lifted the emit gate out of the if function.Exists() branch so it re-evaluates every chunk once both Name and ID are observed, regardless of whether the current delta carries a function object. Both guards remain (Name != "" and ID != ""), so the deferred-id behavior is preserved and we still never announce a SanitizeClaudeToolID fallback.
Added TestStreamingTool_IDInDeltaWithoutFunction covering exactly this split-chunk pattern: function.name in chunk 1, id alone in chunk 2 (no function field), arguments in chunk 3, finish_reason in chunk 4 — asserts one content_block_start with the real id and one content_block_stop. Test fails on 4a90804 and passes on a912284e.
…nction-less delta Codex review on 4a90804 caught a regression introduced by the new id-non-empty guard: the emit check was nested inside `if function.Exists()`, so an upstream that splits tool fields across chunks — function.name in one delta, then id alone in a later delta whose tool_call entry has no `function` object — would store the id but never re-evaluate the emit gate, dropping the tool call entirely on finish. Lift the emit check out of the function block so it runs every chunk once both name and id are observed, regardless of whether the current chunk carries a `function` object. Both guards remain (Name != "" and ID != ""), so the new path still defers emission until the real id is available and never announces a SanitizeClaudeToolID fallback. Add TestStreamingTool_IDInDeltaWithoutFunction covering the exact split-chunk pattern (name in chunk 1 with function, id in chunk 2 without function, arguments in chunk 3, finish in chunk 4).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a912284ec1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Skip tool calls whose start was never emitted (e.g. empty | ||
| // name throughout the stream). Emitting delta/stop for a | ||
| // block that has no start violates Anthropic event ordering. | ||
| if !accumulator.StartEmitted { | ||
| continue |
There was a problem hiding this comment.
When suppressed tool calls are skipped here, the stream can end up with zero emitted tool_use blocks while message_delta.stop_reason is still forced to tool_use via SawToolCall, which is set before validating whether a start was actually emitted. This creates an internally inconsistent Anthropic stream that strict clients may reject (they see tool_use stop reason but no executable tool block). Fresh evidence: the new id validation path (id must be a non-empty string) can now suppress all starts even when function.name is present, so this mismatch is no longer limited to empty-name inputs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for flagging — I think this one is already covered by 0376ace though, and just pushed f83f1df which adds a regression test for exactly the scenario you describe.
SawToolCall, which is set before validating whether a start was actually emitted
This was true before 0376ace but not after. In 0376ace the assignment moved into the emit-success block (line 308 of the post-fix file, immediately after accumulator.StartEmitted = true), gated on the same Name != "" && ID != "" checks that gate emission. So SawToolCall now means "we actually announced a tool_use start" rather than "raw tool_calls field was seen."
the new id validation path can now suppress all starts even when function.name is present, so this mismatch is no longer limited to empty-name inputs
Right, but that case is what f83f1df exercises end-to-end — TestStreamingTool_StopReasonWhenIDNeverArrives:
- chunk 1:
function.name = "do_it", noid→ name accumulated, emit deferred (id missing) - chunk 2:
function.arguments = "{}"→ args accumulated, still no id - chunk 3:
finish_reason = "tool_calls"(upstream claims tool calls)
With 0376ace in place: SawToolCall stays false (no emit happened), so the finish handler hits the new fallback branch (case reason == "tool_calls": → param.FinishReason = "stop"), and mapOpenAIFinishReasonToAnthropic resolves to end_turn. Final stream: zero tool_use blocks, stop_reason: end_turn — internally consistent. The test asserts both halves and passes locally on f83f1df (would fail on 4a90804, which is the version that introduced the id guard without the SawToolCall move).
Happy to be wrong here — if you can point at a specific input where the mismatch still reproduces on 0376ace / f83f1df, I'll dig in. Otherwise I'd lean toward marking this resolved.
…uppressed Self-review caught an unresolved P1 from chatgpt-codex-connector's Apr 20 review on bec7c56 ("Downgrade stop reason when all tool starts are suppressed") that slipped past the prior follow-up commit. Bug: param.SawToolCall was set true on every raw tool_calls delta, even when content_block_start was later suppressed by the name/id guards. With finish_reason="tool_calls" from upstream, the stream ended with stop_reason=tool_use and zero tool_use content blocks — strict Anthropic clients reject this as a malformed turn. Two-part fix: 1. Move param.SawToolCall = true into the emit block, alongside accumulator.StartEmitted = true. The flag now means "at least one tool_use start was actually announced" rather than "raw tool_calls field was seen". Updated the field doc comment to match. 2. In the finish_reason handler, when SawToolCall is false but the upstream finish_reason is "tool_calls" (suppressed-only stream), downgrade param.FinishReason to "stop" so the resulting Anthropic stop_reason is "end_turn" instead of "tool_use". Tests: - Strengthen TestStreamingTool_EmptyNameThroughout to assert stop_reason != "tool_use" (it would have caught this regression). - Add TestStreamingTool_StopReasonWithEmittedTool to lock in the positive path (stop_reason stays "tool_use" when a real tool was emitted). - Add TestStreamingTool_StopReasonMixedSuppressedAndValid to confirm one suppressed + one emitted tool still yields stop_reason="tool_use".
|
Self-review pass on the branch caught one more issue — pushed Background: when I responded to your Apr 21 reply I said the follow-up commit
The second one is real: Fix in
Test deltas:
Apologies for the back-and-forth — this should have been bundled in |
… name does Regression coverage for the codex review scenario on 0376ace: a stream where function.name arrives and is valid, but no tool-call id is ever sent. The new id-non-empty emit guard suppresses content_block_start, and once SawToolCall is gated on actual emission (also from 0376ace), the upstream "finish_reason: tool_calls" must downgrade to "stop" so the stream isn't internally inconsistent (stop_reason=tool_use with zero tool_use blocks).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f83f1dfc85
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // present. Both guards remain: name+id non-empty so we never | ||
| // announce a fallback id from SanitizeClaudeToolID's | ||
| // empty-input path. | ||
| if !accumulator.StartEmitted && accumulator.Name != "" && accumulator.ID != "" { |
There was a problem hiding this comment.
This emit gate can still fire on chunks that arrive after a finish_reason chunk has already finalized the stream state. In that case, ContentBlocksStopped is already true (set in the finish handler), so a late id update (e.g. name seen earlier, id arriving later in a function-less delta) will emit a new content_block_start that never gets a matching delta/stop, and may even be emitted after message_stop when usage was sent with the finish chunk. That produces invalid Anthropic event ordering for strict clients.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — this one is a real bug, and the trigger condition you described is exactly right. Fixed in 4d6b2cd.
The "re-evaluate emit gate on every chunk" change in a912284 (added to make the deferred-id case work for split-chunk providers) didn't account for chunks arriving after the finish-handler had already finalized the stream:
- chunk 1:
function.nameonly, noid→ emit deferred - chunk 2:
finish_reason: tool_calls+usage→ cleanup loop runs,ContentBlocksStopped = true,message_delta+message_stopemitted (downgraded toend_turnbecauseSawToolCallis still false) - chunk 3:
idarrives at last → previous gate (!StartEmitted && Name != "" && ID != "") was true, so acontent_block_startwas emitted aftermessage_stop. Strict clients reject that ordering.
Fix is the obvious one — added !param.ContentBlocksStopped to the emit gate. Late tool fields arriving post-finalization are now silently dropped; the stream stays consistent with the already-downgraded stop_reason: end_turn.
New test TestStreamingTool_LateIDAfterFinalization reproduces exactly that 3-chunk sequence and asserts both that no tool_use start emits AND that no content_block_* event lands after message_stop. Fails on f83f1df, passes on 4d6b2cd.
Thanks for sticking with this thread — that's a genuine ordering violation I would have shipped.
Codex review on f83f1df caught a real ordering bug introduced by the "re-evaluate emit gate every chunk" fix: once a finish_reason chunk has already run the cleanup loop and set ContentBlocksStopped, a later chunk that finally completes the (name, id) pair would still emit a content_block_start. With usage in the same finalize chunk, that emit lands AFTER message_stop — invalid Anthropic event ordering, rejected by strict clients. Add !param.ContentBlocksStopped to the emit gate. Late tool fields arriving after finalization are now silently dropped; the stream remains internally consistent with stop_reason already downgraded to end_turn earlier. Add TestStreamingTool_LateIDAfterFinalization which fires three chunks in sequence (name only, then finish_reason+usage, then id alone) and asserts both that no tool_use start is emitted AND that no content_block_* event lands after message_stop.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d6b2cd5c2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // chunk has already finalized the stream) from emitting a | ||
| // content_block_start out of order — possibly even after | ||
| // message_stop, which strict Anthropic clients reject. | ||
| if !accumulator.StartEmitted && accumulator.Name != "" && accumulator.ID != "" && !param.ContentBlocksStopped { |
There was a problem hiding this comment.
The new emit gate now requires accumulator.ID != "" before sending content_block_start, which means any streaming provider that sends a valid tool name/arguments but never provides tool_calls[].id will have that tool call silently dropped. In this path StartEmitted stays false, cleanup skips delta/stop for that tool, and the stream is downgraded to end_turn, so clients never execute the tool despite receiving a real tool invocation payload. This is a regression from prior behavior where SanitizeClaudeToolID could generate a valid fallback ID and keep the tool call usable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fair regression call. Fixed in b99ae8e.
I had read @luispater's "require non-empty id before emitting content_block_start" too narrowly and applied it as a hard drop. Your point is right: the prior behavior used SanitizeClaudeToolID's empty-input fallback to keep name-but-no-id streams usable, and removing it without a backstop is a regression for any provider that omits tool_calls[].id entirely.
Compromise that preserves both intents:
- Out-of-order (id arrives later) — the inline emit still defers until
name && idare both present, so we never announce a fallback id while a real id is still on the way (maintainer's original concern, unchanged). - Never arrives — the cleanup loops in both the
finish_reasonand[DONE]handlers now belated-emitcontent_block_startfor any accumulator with a valid name butStartEmitted=false.SanitizeClaudeToolIDsynthesizes the fallback id from the empty input, the matching delta+stop follow in the same loop, andSawToolCallflips so the finalstop_reasonmaps back totool_use.
Refactored the inline emit body into emitToolUseStart() so the inline path and both cleanup paths share the same announcement logic.
Test changes:
TestStreamingTool_StopReasonWhenIDNeverArrivesflipped to assert the new positive behavior: onetool_usestart with atoolu_<nanos>_<n>id,stop_reason=tool_use.TestStreamingTool_LateIDAfterFinalizationadjusted: chunk 2's cleanup belated-emits with the synthetic id (stream is correctly addressable), and chunk 3's late id is silently absorbed (the inlineStartEmittedguard already blocks re-emit). The ordering assertion (nocontent_block_*aftermessage_stop) still holds.- Empty-name / null-name cases still drop entirely —
Name == ""guard at cleanup is the floor.
Mind taking another pass?
Codex review on 4d6b2cd flagged a regression from the new id-non-empty emit gate: a stream that supplies a valid function.name but never sends tool_calls[].id had its tool call silently dropped, even though the prior behavior used SanitizeClaudeToolID's empty-input fallback to synthesize a usable id. The fix preserves both intents: - Out-of-order case (maintainer's original concern): the inline emit still defers until name+id are both present, so we never announce a fallback id while a real id is still on the way. - Never-arrives case (codex's catch): the cleanup loops in the finish_reason and [DONE] handlers now belated-emit content_block_start for any accumulator with a valid name but StartEmitted=false. SanitizeClaudeToolID synthesizes a fallback id from the empty input, the matching delta+stop follow in the same loop, and SawToolCall flips so the final stop_reason maps to "tool_use" instead of being downgraded. Refactor the inline emit body into emitToolUseStart() so the inline path and both cleanup paths share the same announcement logic (block index allocation, thinking/text stop ordering, SawToolCall toggle). Tests: - TestStreamingTool_StopReasonWhenIDNeverArrives now asserts the new positive behavior (1 tool_use start with toolu_<nanos>_<n> id, stop_reason=tool_use) instead of the old "drop entirely" behavior. - TestStreamingTool_LateIDAfterFinalization adjusted to expect the belated emit in chunk 2 and confirm chunk 3's late id is absorbed silently (StartEmitted gates the inline path). - All other tests unchanged and passing — the empty-name and null-name paths still drop entirely (Name == "" guard at cleanup), and out-of-order-but-arrives paths still emit exactly once with the real id.
|
@luispater — status update on this thread. The review settled on Scope did grow past the initial 1-file fix while addressing the bot review thread, so happy to squash to a single commit before merge if that reads better — or keep it granular for traceability, your call. No urgency on my end, just leaving a marker for whenever this comes back into your queue. |
2 participants
Problem
When
internal/translator/openai/claude/openai_claude_response.gotranslates streaming OpenAI responses into Anthropic format, two issues inconvertOpenAIStreamingChunkToAnthropiccan corrupttool_useblocks:Empty
namepasses through.gjson.Result.Exists()returns true for"name": ""and"name": null. The empty string flows intoaccumulator.Nameand the emittedcontent_block_start. Anthropic-format clients then reject the block — Amp CLI's normalizer logsSkipping tool_use normalization due to missing nameand the tool isn't surfaced.Duplicate
content_block_startemissions. Some upstream providers (or proxied stacks like dmxapi.cn) send thefunction.namefield redundantly across multiple streaming chunks for the same tool index. The original code emits a freshcontent_block_startevery time, producing two start events for one logical tool block. This breaks parsers that expect one start per index — Amp surfaces it asJSON Parse error: Unable to parse JSON stringand the assistant turn is lost.Reproduction
Connect Amp CLI through CLIProxyAPI's
openai-compatibilityprovider to an upstream that occasionally re-sendsfunction.name(Chinese relay stations likedmxapi.cndo). Run any task that triggers concurrent tool calls:Without this patch, ~10–20% of repo-exploration runs fail with
JSON Parse error. Logs show 11+Skipping tool_use normalization due to missing namewarnings per failed run.Fix
accumulator.Name.ToolCallAccumulator.StartEmittedflag and only emitcontent_block_startonce per tool index, after a non-empty name has been observed.Verification
Skipping tool_use normalizationwarnings per runNotes
content_block_startevents.