Skip to content

fix(translator): six format-conversion defects (#3243, #3247, #3254, #3369, #3492, replaces 6 PRs) - #3878

Open
ntdatt812 wants to merge 5 commits into
decolua:masterfrom
ntdatt812:bundle/translator-format
Open

ntdatt812 wants to merge 5 commits into
decolua:masterfrom
ntdatt812:bundle/translator-format

Conversation

@ntdatt812

Copy link
Copy Markdown
Contributor

Six translator/handler fixes that were sitting in separate PRs (#3243, #3247, #3254, #3369, #3492/#3528, #3549), rebased onto current master and sent as one review. Each is an independent commit, so any one of them can be dropped without touching the others.

All six are format-conversion defects: a request or response crossed a format boundary and lost a field that the target API requires.

commit defect
1b042a1c reasoning.effort was sent flat to openai-responses targets, which nest it
0f23a482 a non-streaming binary-transport reply was returned in the upstream's format, not the client's
8805156e tool_calls: [] was treated as "has tool calls", producing an empty tool_use block
74658a51 a tool result arriving without an id emitted tool_use_id: undefined; Anthropic rejects the whole request
65339392 a request with no stream key defaulted to streaming, so non-streaming clients got SSE
d43850f6 the CommandCode error chunk opened without a role, so clients that key on the first delta dropped it

Verification

npx vitest@3 run --config tests/vitest.config.js \
  tests/unit/{commandcode-to-openai,nonstream-binary-transport-source-format-3199,openai-format-empty-tool-calls,stream-mode-default-3492,thinking-responses-nested-3154,tool-result-missing-id-3362}.test.js \
  tests/translator/thinking-unified.test.js
→ 7 files, 107 passed

Two notes on running these, because both cost me time:

  • The alias @/ only resolves with --config tests/vitest.config.js; without it collection dies on Failed to load url @/lib/dataDir.js.
  • vitest 2.x cannot run any test that reaches open-sse/translator/index.js. It fails with TypeError: register is not a function at the top-level register(...) calls. That is not a defect in this repo: index.js deliberately relies on function-declaration hoisting to survive the import cycle (the comment above var requestRegistry says so), and Vite 5's SSR transform turns the named import into a call-time property access that is not yet populated. I confirmed the same failure on clean master, on master as of 15 Aug, and on this repo's own tests/translator/bugs-antigravity.test.js — so it predates these changes and is not caused by them. vitest 3 runs them correctly.

Each claim was checked by reverting it and confirming which test dies; no test passes for a reason other than the fix.

One of the six did not survive that check as it stood. Reverting the call site in chatCore.js for #3492 left stream-mode-default-3492.test.js green: the tests exercised clientRequestedStreaming in isolation and nothing observed the wiring — the same shape of bug the fix is about. handleChatCore takes ~30 collaborators and cannot be driven from a unit test, so 18e67517 pins the call site at the source level instead. Reverting chatCore.js now fails 3 of that file's 10 cases.

…port replies

Providers on a binary or proprietary transport — kiro EventStream, cursor
protobuf, commandcode NDJSON — decode their upstream reply to OpenAI Chat
Completions inside their own executor. translateNonStreamingResponse has no
branch for those target formats, so it fell through and returned the raw
chat.completion body regardless of what the client asked for: a stream:false
request to /v1/messages handed a Claude client `object:"chat.completion"`, and
LiteLLM's anthropic/ prefix died on KeyError: 'content'. The streaming path was
already correct.

Convert on the way out when the body carries a choices array and the client
speaks Claude or Responses.

Fixes decolua#3199
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.
Claude requests with tool history intermittently failed upstream with
"messages.N.content.0.tool_result.tool_use_id: Field required".

ensureToolCallIds repairs an assistant tool_call whose id is missing or
malformed, but both tool-result branches were guarded on the id already being
truthy:

    if (msg.role === "tool" && msg.tool_call_id && !PATTERN.test(...))
    if (block.type === "tool_result" && block.tool_use_id && !PATTERN.test(...))

So a result that arrives with no id at all skips validation entirely, and
openai-to-claude then emits tool_use_id: undefined. Anthropic rejects the whole
request, which is why it looks intermittent — it depends on whether the client
dropped the id on that turn. hasToolResults() matches on the same field, so
fixMissingToolResponses also read the result as absent and spliced in an empty
duplicate next to it.

Sanitizing cannot invent an id, so pair it instead: a result with no id takes
the oldest tool call from the preceding assistant turn that nothing has
answered yet. That is the id upstream is expecting, and it survives parallel
tool calls and out-of-order results because a claimed id is removed from the
open set. A well-formed id is still passed through untouched and a malformed
one is still sanitized.

With nothing to pair with — a result that follows no tool call — the id is
generated as before rather than left absent: it keeps the request well-formed
instead of guaranteeing a 400.

The repair lives in the shared normalization pass, so every target that
consumes these ids (claude, gemini, kiro, cursor) gets it, not just
openai-to-claude.

Reported in decolua#3362.
…olua#3492)

chatCore resolved the response framing with:

    let stream = providerRequiresStreaming ? true : (body.stream !== false);

The OpenAI API defines stream with a default of false, so a body that
omits the key is a non-streaming request. `!== false` reads an absent key
as "stream", and the handler took the SSE branch for a payload it had
already built as a complete chat.completion object: text/event-stream on
the wire, plus a data: [DONE] concatenated onto valid JSON. Strict parsers
reject that, so every non-streaming call from an AI-SDK-based client fails
while interactive streaming clients look healthy.

The correct predicate was already sitting on the line above, computed and
used further down for the forced-stream-to-JSON branch. Move it into
chatCore/streamMode.js, keep the Gemini and Antigravity carve-out (their
endpoints carry the mode in the path, and only the streaming ones are
routed here), and read the mode from it.

The Accept: application/json branch below stays: it only ever forces
non-streaming, so it is now a no-op for this case rather than the only
thing that rescued it - and a client that sends no Accept header, like
the curl reproduction in the issue, was never rescued at all.
The decolua#3492 tests exercised clientRequestedStreaming in isolation, so
reverting the call site in chatCore.js left the file green — the wiring
was unobserved. handleChatCore takes ~30 collaborators and cannot be
driven from a unit test, so assert the call site at the source level.

Reverting chatCore.js now fails 3 of the 10 cases.
@ntdatt812
ntdatt812 force-pushed the bundle/translator-format branch from 18e6751 to 36d4cf1 Compare September 24, 2026 02:37
@ntdatt812

Copy link
Copy Markdown
Contributor Author

Rebased onto master (39e36d3d). Was CONFLICTING, now clean. Two commits came out of the bundle rather than being merged forward, because master has since decided both questions the other way:

  • fix(commandcode): open the error chunk with the assistant role — withdrawn. That branch now throws instead of emitting the error as content, so the stream handler marks the stream errored. Giving the emitted chunk a role has nothing left to apply to.

  • fix(translator): nest reasoning effort for openai-responses targets — withdrawn, and I would like your read on it. d1de3245 (22 Sep) added tests/unit/thinking-budget-max-level.test.js, which asserts reasoning_effort stays flat on an OPENAI_RESPONSES target:

    const out = applyThinking(FORMATS.OPENAI_RESPONSES, "gpt-5.6-sol", body, "codex");
    expect(out?.reasoning_effort).toBe("max");

    That is the opposite of what the commit did, so it is out. Worth asking anyway: the OpenAI Responses API reads reasoning: { effort } and ignores a top-level reasoning_effort, so for a genuine /v1/responses upstream the flat key is dropped on the floor. If codex is a wire that wants the flat key and only some responses targets want it nested, the split is per provider rather than per format and I am happy to send that separately. If you meant flat everywhere, nothing to do.

What remains is five commits, unchanged in intent:

commit what
f2688315 non-streaming binary-transport replies are built in the client's format, not the source's (#3199)
d052cd8f an empty tool_calls: [] is no tool calls, not a tool call with no fields
ea7492dd a tool result that arrives without an id is matched to its call instead of dropped (#3362)
350c8ce7 a request with no stream key is non-streaming (#3492)
36d4cf18 pins that chatCore reads the mode from streamMode.js

Two conflicts were merges rather than choices, and both keep your side: thinkingUnified.js keeps the new display passthrough, and chatCore.js keeps the token-saver block — only the inlined streaming predicate is replaced by requestedStreaming(body, sourceFormat).

npx vitest@3 run --config tests/vitest.config.js: 89 failures, and they are the same 89 by name as on master at 39e36d3d — compared per test name, not by count.

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