Repository navigation
fix(gateway): return mapped finish_reason to client - #3504
Merged
Merged
Conversation
The default branch of transformResponseToOpenai never wrote the parsed finishReason back into the client response, so canonicalizations applied by parseProviderResponse (abort -> upstream_error, and end_turn/tool_use once #3492 lands) were lost for the 24 providers without their own case block. Mirror the provider-specific cases and write the mapped value back when one exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Warning Review limit reached
Next review available in: 23 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
Author
|
/e2e |
Member
Author
|
e2e run 31311802724: 3 failed files, all unrelated to this change:
Everything else: 27 files / ~1900 tests passed. |
steebchen
added this pull request to the merge queue
Aug 9, 2026
steebchen
pushed a commit
that referenced
this pull request
Aug 14, 2026
## Summary
`parseProviderResponse` (Google branch) caches Gemini thought signatures
in
Redis under the id it emits — `${name}_${shortid(24)}`, as
`thought_signature:<id>` — so the signature can be re-injected when the
client
replays the call next turn. The `n > 1` branch of
`transformResponseToOpenai`
(`apps/gateway/src/chat/tools/transform-response-to-openai.ts:454-456`)
regenerated tool calls from the raw parts with
`${name}_${candidateIndex}_${fcIndex}`
ids and dropped `extra_content`, so the id the client echoes back never
matches
the cached key. The next-turn lookup in `chat.ts`
(`redisClient.get("thought_signature:" + toolCall.id)`) misses, the
signature
is not re-injected, and Gemini rejects the replay with *"Corrupted
thought
signature"* — the exact failure the shortid fix (#1448) eliminated on
the
streaming path. The `n > 1` branch is the one path left behind: the
single-candidate path reuses parse's `toolResults` and is consistent.
This is the same "forgotten sibling branch" pattern as #3492 (our
previous
fix) and #3504 (the maintainer's own follow-up).
## Fix
Two changes in the multi-candidate branch:
- **Candidate 0** reuses the tool calls from `toolResults` (what
`parseProviderResponse` already emitted and cached), so the id the
client
receives is exactly the one the signature is cached under.
- **Candidates 1+** (which parse does not process) get the same
treatment
parse applies to candidate 0: a unique `${name}_${shortid(24)}` id, the
inline `extra_content.google.thought_signature`, and the
`setex("thought_signature:<id>", 86400, sig)` Redis entry under the
emitted
id.
Fallback keeps the branch safe: if `toolResults` is absent for candidate
0
(e.g. no tool calls parsed), the shortid path applies.
## Tests
`transform-response-to-openai.spec.ts`:
- The existing multi-candidate test no longer pins the broken
`${name}_${candidateIndex}_${fcIndex}` ids; it asserts the id scheme and
that candidates are distinct.
- New test: `keeps multi-candidate Google tool_call ids in the
thought_signature
cache` — mocks `@llmgateway/cache` (`setex`) and asserts:
- choice 0 reuses the parsed tool calls verbatim (id + inline
signature);
- choice 1 gets a unique id, `extra_content` with its own signature, and
a
`setex("thought_signature:<id>", 86400, sig)` call under the emitted id;
- no `setex` is written under choice 0's id from the transform (parse
already did that).
## Verification
Every command below was run and its output captured. The record is
reproducible — commands included so you can re-run them.
**red — probe against main (c1cd97b), real functions via esbuild,
before the
fix** (harness preserved at the hub):
```
parse toolResults[0].id : get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
+ extra_content.google.thought_signature
Redis writes made by parse : [{"key":"thought_signature:get_weather_xxxxxxxxxxxxxxxxxxxxxxxx", …}]
transform choice 0 tool_calls: id get_weather_0_0, extra_content undefined
RESULT: MISMATCH — next-turn GET thought_signature:get_weather_0_0 misses;
signature lost, Gemini 3 rejects replay
=== control (n=1) ===
CONTROL RESULT: MATCH — single-candidate path is consistent
```
**green — same probe after the fix**:
```
choice 0 tool_calls: id get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
extra_content.google.thought_signature sig-candidate-0
choice 1 tool_calls: id get_weather_xxxxxxxxxxxxxxxxxxxxxxxx
extra_content.google.thought_signature sig-candidate-1
Redis writes: thought_signature:get_weather_xxx = sig-candidate-0
thought_signature:get_weather_xxx = sig-candidate-1
RESULT: MATCH (no bug)
CONTROL RESULT: MATCH — single-candidate path is consistent
```
(The deterministic shortid stub makes both candidate ids equal in the
probe;
the real `shortid` yields unique ids per candidate, asserted in the
spec.)
**green — vitest, full spec**:
```
$ pnpm exec vitest run apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts --no-file-parallelism
✓ apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts (14 tests) 24ms
Test Files 1 passed (1)
Tests 14 passed (14)
[exit code 0]
```
**Area suite** — `vitest run apps/gateway/src/chat/tools`: **593/603
tests,
37/38 files pass**. The only failing file is
`openai-content-filter.spec.ts` (10 tests), a service-harness spec whose
fetch/Redis mocks time out in this environment; it imports only
`openai-content-filter.ts`, which this change does not touch (same class
of
harness failures documented in previous PRs here).
<details><summary>Environment</summary>
```
so: Windows 11 (AMD64)
node --version: v24.14.1
pnpm --version: 9.15.9 (via npx; repo packageManager is pnpm@10.30.3)
vitest: 4.1.8
base: c1cd97b (origin/main @ 08-09)
fix commit: 7e76bf9 (2 files, +177/−13)
```
</details>
**Gates:** eslint ✅ on both files (after `import/order` fix) · prettier
✅ ·
`git diff --check` clean · gateway typecheck ✅ (builds in `build:core`,
turbo 12/12) · `pnpm-lock.yaml` untouched.
**Collisions:** no open PR touches the multi-candidate Google tool_call
ids /
thought_signature path (checked live 2026-08-11). #3486 (reasoning
tokens)
and #3233 (Claude on Azure) also touch `transform-response-to-openai.ts`
but
in unrelated areas (usage tokens / new provider branch); no semantic
overlap.
Not verified: a live multi-turn round-trip against Gemini (no provider
keys on
this machine). The id↔Redis-key chain is covered end-to-end by the
spec's
`setex` assertions and the probe's recorded Redis writes.
---
Disclosure: an AI coding assistant helped locate the drift and draft the
test
scaffolding. The reproduction, the red→green cycle and this write-up
were
verified by running the code; I own the change and will follow up on
review
comments.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved handling of Google responses containing multiple candidates
and tool calls.
* Preserved candidate-specific thought signatures for reliable follow-up
processing.
* Prevented tool-call identifiers and cached data from being overwritten
across candidates.
* Added graceful logging when caching metadata cannot be completed.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Co-authored-by: Codebuff <noreply@codebuff.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #3492. That PR (and #3053 before it) canonicalizes non-standard finish reasons in
parseProviderResponse'sdefault:branch — but for most providers the canonical value never reached the client.transformResponseToOpenaistarts from the raw upstream JSON and only writes the parsedfinishReasonback inside provider-specificcaseblocks; itsdefault:branch updates content, reasoning, annotations, model, metadata and usage, but notfinish_reason.So of the ~38 providers served by the parse
default:branch, only the 14 with their own transform case (openai, groq, xai, aws-mantle, azure, zai, inference.net, together-ai, scx-ai, scx-ai-gp, bytedance, embercloud, meta, sakana) returned the mapped value. The other 24 (deepseek, fireworks, minimax, cerebras, moonshot, perplexity, nebius, deepinfra, canopywave, nanogpt, custom, …) fell through the transformdefault:and still leaked the raw upstream value — verified by running afinish_reason: "tool_use"payload through parse → transform:together-aicame outtool_calls,deepseek/fireworks/minimax/cerebrasstilltool_use.19 of those 24 are in the streaming shared-case list that already canonicalizes (
transform-streaming-to-openai.ts), so the stream/non-stream asymmetry #3492 targets persisted for them.Fix
Write the parsed
finishReasonback in the transformdefault:branch, exactly as the provider-specific cases already do:Standalone this propagates the existing
abort→upstream_errormapping (#3053) to those 24 providers; combined with #3492 it completesend_turn→stop/tool_use→tool_callsfor all providers on the branch. When the upstream sends no finish reason at all, parse yieldsnulland the guard leaves the response untouched.Tests
Three cases added to
transform-response-to-openai.spec.ts:tool_callswritten back fordeepseek(rawtool_usein the upstream JSON), tool calls preserved;upstream_errorwritten back forfireworks(rawabort);nullparsed finishReason leaves the upstream value untouched (cerebras).Verification
vitest runtransform + parse specs: 50/50 ✅apps/gateway/src/chat/toolssuite: all specs passpnpm build: 17/17 ✅pnpm formatclean🤖 Generated with Claude Code