Skip to content

fix(translator): treat an empty tool_calls array as no tool calls - #3254

Closed
ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/openai-format-empty-tool-calls
Closed

ntdatt812 wants to merge 1 commit into
decolua:masterfrom
ntdatt812:fix/openai-format-empty-tool-calls

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Summary

filterToOpenAIFormat treats an assistant message as "carries tool calls" whenever msg.tool_calls is truthy. An empty array is truthy, so tool_calls: [] trips both shortcuts that check it:

// normalisation pass — returns before any block filtering
if (msg.role === ROLE.ASSISTANT && msg.tool_calls) return msg;

// empty-message filter — keeps the message unconditionally
if (msg.role === ROLE.ASSISTANT && msg.tool_calls) return true;

Two things follow from that. An assistant turn with tool_calls: [] keeps its Claude-only blocks — thinking, redacted_thinking, unstripped signature — because it never reaches the block filter. And a turn whose content is blank survives the filter that exists to drop exactly those. Providers that reject either shape reject the whole request, and the failure surfaces far from here.

tool_calls: [] is not hypothetical in this codebase — #3234 and #3236 both come from upstreams that attach an empty array to ordinary content deltas.

The fix

Both guards become msg.tool_calls?.length, which is what the rest of the codebase already does:

  • translator/request/claude-to-openai.js:100 — msg.tool_calls && msg.tool_calls.length > 0
  • executors/kiro.js:208 — delta.tool_calls?.length

Messages with real tool calls are unaffected.

Tests

tests/unit/openai-format-empty-tool-calls.test.js, three cases:

  • Claude-only blocks are still stripped when tool_calls: [] is present
  • an assistant turn left with no usable content is still dropped
  • a message with a real tool call keeps both its tool_calls and its content

The first two fail on master and pass with the change:

before:  × still strips Claude-only blocks   → expected [ 'thinking', 'text' ] to not include 'thinking'
         × still drops an assistant turn     → expected length 1 but got 2
after:   3 passed

Regression check

Per CLAUDE.md the suite is not green on a plain checkout, so I ran it both ways and diffed the failing set rather than reading the totals.

master:      Test Files 29 failed | 146 passed (178)   Tests 92 failed | 1652 passed
this branch: Test Files 29 failed | 147 passed (179)   Tests 92 failed | 1655 passed

Identical failing sets — 192 failure lines each way, no test moved in either direction. The delta is exactly the three added cases.

One thing I noticed

open-sse/transformer/responsesTransformer.js:374 has the same if (delta.tool_calls) shape, and open-sse/handlers/responsesHandler.js is the only importer of it — while nothing imports handleResponsesCore in turn. The live /v1/responses route (src/app/api/v1/responses/route.js) goes through handleChat and the translator registry instead. So that pair looks like it has been left behind rather than being a second live path, and I left it alone. Worth confirming on your side — if it is dead, it is a duplicate of the conversion in translator/response/openai-responses.js and probably wants removing.

filterToOpenAIFormat took two shortcuts for assistant messages carrying
tool_calls: skip content normalisation, and never drop the message. Both
were gated on plain truthiness, and an empty array is truthy.

An assistant turn arriving as tool_calls: [] therefore kept its Claude-only
blocks (thinking, redacted_thinking) instead of having them stripped, and
survived the empty-message filter with blank content. Upstreams that reject
either shape rejected the whole request.

Switches both guards to tool_calls?.length, matching claude-to-openai.js
and the kiro executor. Messages with real tool calls are unaffected.
@ntdatt812
ntdatt812 force-pushed the fix/openai-format-empty-tool-calls branch from ecf90bb to c7095f7 Compare August 15, 2026 02:06
@ntdatt812

Copy link
Copy Markdown
Contributor Author

Rebased check against current master: this is still open, still applies cleanly, and the bug it fixes is still there.

Since I opened this, #10a923da1 ("fix(responses): don't close message on empty tool_calls array", @chisewaguri) landed and fixed the same class of bug — an empty tool_calls array being treated as if it carried tool calls — in open-sse/translator/response/openai-responses.js.

This PR is the sibling case in the other translator. open-sse/translator/formats/openai.js on master today:

26:    // Keep assistant messages with tool_calls as-is
27:    if (msg.role === ROLE.ASSISTANT && msg.tool_calls) return msg;
...
67:    // Always keep assistant messages with tool_calls
68:    if (msg.role === ROLE.ASSISTANT && msg.tool_calls) return true;

Both are truthiness checks on the array itself, so tool_calls: [] takes the branch. filterToOpenAIFormat then keeps an assistant message that has neither content nor tool calls, and the upstream sees an empty assistant turn. The fix is the same shape as the one already accepted: msg.tool_calls?.length.

Diff is 4 lines in the source plus a regression test. Happy to rebase or split it further if that helps.

@ntdatt812

Copy link
Copy Markdown
Contributor Author

Superseded by #3878, which carries this commit unchanged, rebased onto current master and grouped with the other fixes in the same area. Closing here so the two do not sit in the queue as duplicates — reopen if you would rather review it on its own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant