Skip to content

fix(responses): normalize custom tool names in non-streaming conversion - #3373

Open
krasumashi wants to merge 1 commit into
decolua:masterfrom
krasumashi:fix/responses-custom-tool-nonstream-set
Open

fix(responses): normalize custom tool names in non-streaming conversion#3373
krasumashi wants to merge 1 commit into
decolua:masterfrom
krasumashi:fix/responses-custom-tool-nonstream-set

Conversation

@krasumashi

Copy link
Copy Markdown

Summary

A Responses-API client that declares a custom tool gets a bodyless HTTP 500 whenever the model actually calls it — after the provider has already answered successfully. Standard function tools are unaffected, which makes it look like a provider or routing fault rather than a translation bug.

Reproduced on v0.5.55 and the defect is still present on master.

Root cause

The Responses request translator collects custom tool names in a Set but exports them as an array:

if (customToolNames.size > 0) result._customToolNames = [...customToolNames];
// open-sse/translator/request/openai-responses.js

handlers/chatCore.js lifts that value off the translated body and forwards it unchanged to the response converters, which ask it for Set semantics:

const custom = customToolNames?.has(fn.name);
// open-sse/handlers/chatCore/nonStreamingHandler.js  (openAICompletionToResponses)
// open-sse/handlers/chatCore/sseToJsonHandler.js     (chatCompletionToResponses)

Arrays have no .has, and optional chaining only guards null/undefined, so this throws TypeError: customToolNames?.has is not a function. The rejection escapes the route handler and Next returns an empty 500.

Two details explain the symptom pattern:

  • The size > 0 guard means the property is absent when no custom tool is declared, so the optional chain short-circuits and ordinary function tools keep working.
  • The throw happens inside the for (… of message.tool_calls) loop, i.e. only once the provider has already returned a tool call — so tokens are spent and then the request dies.

Streaming was never affected: open-sse/utils/stream.js already normalises with new Set(customToolNames || []). This PR applies the same normalisation at the two non-streaming boundaries.

Why it wasn't caught

Every existing test constructs customToolNames as a hand-built Set, while tests/unit/openai-responses-custom-tools.test.js separately asserts the producer emits an array (expect(out._customToolNames).toEqual(["exec"])). Both halves are individually correct; the seam between them was never exercised.

The fix

Normalise at the consumer, accepting Array | Set | null | undefined, without mutating the caller's collection:

const customToolNameSet =
  customToolNames instanceof Set ? customToolNames : new Set(customToolNames || []);

The producer's array contract is deliberately left alone, since existing tests pin it. utils/stream.js is untouched — it is already correct, and changing it would swap a copy for a shared reference for no benefit.

Tests

Added to tests/unit/openai-responses-nonstream.test.js (8 cases for the non-streaming consumer, 1 for the forced-SSE→JSON consumer):

  • null, undefined, [], new Set()function_call
  • ["exec"] and new Set(["exec"])custom_tool_call, byte-identical output
  • non-matching name → function_call
  • raw input fidelity: {"input":"…"} unwraps, with no wrapper leakage
  • multi-line payload preserved verbatim
  • the seam: feeds the real output of openaiResponsesToOpenAIRequest into the converter, asserting Array.isArray(_customToolNames) first

Before the fix these fail with the TypeError; after, the file is 18/18.

Run with:

cd tests && npx vitest run unit/openai-responses-nonstream.test.js unit/openai-responses-custom-tools.test.js

I also ran the full suite before and after the change on the same machine to compare failure sets: no test that passed beforehand fails afterwards. (A plain checkout is not all-green, as CLAUDE.md documents, so the comparison is before/after rather than an absolute pass.)

End-to-end verification

Built from this branch and run against a real provider: a Responses client declaring a custom tool now receives type: "custom_tool_call" with the raw input intact — non-streaming and streaming — and a custom_tool_call_output continuation resumes normally. Multi-line patch-shaped input round-trips byte-for-byte.

This matters for Codex compatibility specifically, since Codex transports apply_patch as a freeform custom tool, so every emitted apply_patch call hits this path.

The Responses request translator collects custom tool names in a Set but
exports them as an array:

    if (customToolNames.size > 0) result._customToolNames = [...customToolNames];
    -- translator/request/openai-responses.js

handlers/chatCore.js lifts that value off the translated body and forwards
it unchanged to the response converters, which asked it for Set semantics:

    const custom = customToolNames?.has(fn.name);
    -- handlers/chatCore/nonStreamingHandler.js  (openAICompletionToResponses)
    -- handlers/chatCore/sseToJsonHandler.js     (chatCompletionToResponses)

Arrays have no `.has`, and optional chaining only guards null/undefined, so
any custom tool call threw TypeError *after* the provider had already
answered successfully. Next surfaced the unhandled rejection as a bodyless
HTTP 500, which reads like a routing or upstream failure rather than a
translation bug. Standard function tools were unaffected, because the
`size > 0` guard means the property is absent when no custom tool is
declared and the optional chain then short-circuits.

Streaming was never affected: utils/stream.js already normalizes with
`new Set(customToolNames || [])`. This applies the same normalization at
the two non-streaming boundaries, accepting Array | Set | null | undefined
and never mutating the caller's collection. The producer's array contract
is deliberately left alone, since tests/unit/openai-responses-custom-tools.js
pins it with `expect(out._customToolNames).toEqual(["exec"])`.

Why this was not caught: every existing test constructed `customToolNames`
as a hand-built Set, so the seam between the producer and these consumers
was never exercised. The added regression test feeds the real output of
openaiResponsesToOpenAIRequest into the converter, and covers null,
undefined, [], Set, matching and non-matching names, raw-input fidelity
(no `{"input":...}` wrapper leakage), multi-line payloads, and the
standard function_call path.

Verified end to end against a patched instance on 127.0.0.1:20129: a
Responses client declaring a custom tool now receives
`type: "custom_tool_call"` with the raw input intact, in both non-streaming
and streaming mode, and a `custom_tool_call_output` continuation resumes
normally.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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