Repository navigation
fix(gateway): raw finish_reason when not streaming - #3492
Merged
steebchen merged 1 commit intoAug 9, 2026
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe provider-response parser now normalizes Groq ChangesFinish-reason normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
The default branch of parseProviderResponse only mapped abort, so the OpenAI-compatible providers it serves (groq, together-ai, deepseek, xai, minimax, ...) leaked provider-native end_turn/tool_use to the client over stream:false, while the streaming path canonicalizes that same provider list. Mirror the mistral/novita branch: end_turn -> stop, tool_use -> tool_calls.
pacocartones
force-pushed
the
fix/openai-compat-finish-reason
branch
from
August 9, 2026 16:54
8847455 to
3d74c9f
Compare
pacocartones
pushed a commit
to pacocartones/llmgateway
that referenced
this pull request
Aug 9, 2026
## Summary Follow-up to theopenco#3492. That PR (and theopenco#3053 before it) canonicalizes non-standard finish reasons in `parseProviderResponse`'s `default:` branch — but for most providers the canonical value never reached the client. `transformResponseToOpenai` starts from the raw upstream JSON and only writes the parsed `finishReason` back inside provider-specific `case` blocks; its `default:` branch updates content, reasoning, annotations, model, metadata and usage, but **not** `finish_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 transform `default:` and still leaked the raw upstream value — verified by running a `finish_reason: "tool_use"` payload through parse → transform: `together-ai` came out `tool_calls`, `deepseek`/`fireworks`/`minimax`/`cerebras` still `tool_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 theopenco#3492 targets persisted for them. ## Fix Write the parsed `finishReason` back in the transform `default:` branch, exactly as the provider-specific cases already do: ```ts if (transformedResponse.choices?.[0] && finishReason !== null) { transformedResponse.choices[0].finish_reason = finishReason; } ``` Standalone this propagates the existing `abort` → `upstream_error` mapping (theopenco#3053) to those 24 providers; combined with theopenco#3492 it completes `end_turn` → `stop` / `tool_use` → `tool_calls` for all providers on the branch. When the upstream sends no finish reason at all, parse yields `null` and the guard leaves the response untouched. ## Tests Three cases added to `transform-response-to-openai.spec.ts`: - mapped `tool_calls` written back for `deepseek` (raw `tool_use` in the upstream JSON), tool calls preserved; - mapped `upstream_error` written back for `fireworks` (raw `abort`); - `null` parsed finishReason leaves the upstream value untouched (`cerebras`). ## Verification - `vitest run` transform + parse specs: 50/50 ✅ - full `apps/gateway/src/chat/tools` suite: all specs pass - `pnpm build`: 17/17 ✅ - `pnpm format` clean 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
steebchen
added a commit
that referenced
this pull request
Aug 9, 2026
## Problem Two unrelated ways a scoped test run reports failures that have nothing to do with the change under test. Both showed up while verifying #3492. **1. Video log-content assertions hardcode `http://localhost:4001`.** `gateway-api-test-harness.ts` and two `videos.spec.ts` assertions compare against a literal `http://localhost:4001`, but the gateway builds that URL from `getGatewayPublicBaseUrl()` (i.e. `GATEWAY_URL`). Any worktree following AGENTS.md's "isolated stack per worktree" guidance runs on an offset `GATEWAY_PORT`, so three video tests fail: ``` AssertionError: expected 'http://localhost:4801' to be 'http://localhost:4001' ❯ gateway-api-test-harness.ts:278 ``` The tests were asserting the port, not the behaviour. **2. `ocr.e2e.ts` runs unconditionally and hardcodes `mistral-ocr-latest`.** It only skipped when `LLM_MISTRAL_API_KEY` was entirely absent, so a Mistral key *without* the separate OCR entitlement fails with an upstream `{"detail":"Invalid API Key"}` → 401 on **every** e2e run, including ones scoped to an unrelated model: ``` $ TEST_MODELS="together-ai/gpt-oss-20b" FULL_MODE=true pnpm test:e2e FAIL apps/gateway/src/ocr.e2e.ts > /v1/ocr extracts a document via mistral-ocr-latest Tests 1 failed | 102 passed ``` OCR is also billed per page, so it should not run on every unrelated scoped run in the first place. ## Approach **Port**: assert against the same helpers the gateway itself uses — `getGatewayPublicBaseUrl()` in the harness, `buildGatewayVideoLogContentUrl()` in `videos.spec.ts`. The tests now verify the URL matches what the app produces, on any port, and keep working on the `http://localhost:4001` default. **OCR**: OCR models already live in the catalogue (`output: ["ocr"]`, `ocr: true`), so drive the suite from it like the rerank/speech/transcription suites do. Adds an `ocrModels` list to `chat-helpers.e2e.ts` mirroring `rerankModels` (same TEST_MODELS/TEST_PROVIDERS, deactivation, env-var and stability filters), and `ocr.e2e.ts` becomes `test.each(ocrModels)`. The mapping is marked `test: "skip"`, which makes the suite opt-in — `TEST_MODELS` overrides `test: "skip"`, so it runs exactly when you ask for it: ```bash TEST_MODELS="mistral/mistral-ocr-latest" pnpm test:e2e ``` The "rejects an unknown model with 400" case is pure request validation, rejected before any provider is contacted, so it no longer needs a key gate and runs always. ## Verification `videos.spec.ts` — 50/50 pass on all three configurations (previously 3 failures on the first): | `GATEWAY_URL` | before | after | |---|---|---| | `http://localhost:4801` (offset stack) | 3 failed | 50 passed | | `http://localhost:4001` (explicit) | 50 passed | 50 passed | | unset (falls back to :4001) | 50 passed | 50 passed | `ocr.e2e.ts`: ``` $ pnpm test:e2e # default Testing 0 ocr model configurations ✓ /v1/ocr rejects an unknown model with 400 Tests 3 passed $ TEST_MODELS="mistral/mistral-ocr-latest" pnpm test:e2e # opt-in Testing 1 ocr model configurations × /v1/ocr extracts a document via 'mistral/mistral-ocr-latest' # this machine's key lacks the OCR entitlement ``` The scoped run that previously failed is now clean: ``` $ TEST_MODELS="together-ai/gpt-oss-20b" FULL_MODE=true pnpm test:e2e Test Files 29 passed | 2 skipped (31) Tests 102 passed | 88 skipped (190) ``` Gates: `pnpm build` 17/17 · `pnpm lint` 17/17 · `packages/models` 131/131 · full `apps/gateway` unit suite 2171 passed. One failure there — `openai-content-filter.spec.ts > logs missing moderation credentials` — reproduces identically on unmodified `main` with these changes stashed; it is the known local `.env` leakage (the spec assumes no OpenAI key is configured) and is untouched by this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Expanded OCR extraction coverage across configured providers and models, including region-specific and selectively enabled test scenarios. * Added consistent validation for unknown OCR models, regardless of provider credentials. * Updated video log URL checks to work with configured gateway environments instead of relying on local addresses. * **Chores** * Mistral OCR tests are now opt-in because they require separate OCR access. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
2 participants
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
Non-streaming chat completions leak provider-native
finish_reasonvalues(
end_turn,tool_use) to the client for the ~29 OpenAI-compatible providersrouted through the
default:branch ofparseProviderResponse(
apps/gateway/src/chat/tools/parse-provider-response.ts) — groq, deepseek,xai, together-ai, fireworks, minimax, zai, … The streaming path canonicalizes
exactly that provider list (
transform-streaming-to-openai.ts, the sharedcasemappingend_turn→stop,tool_use→tool_calls,abort→upstream_error), and the non-streaming path already does it formistral/novita— but not for the rest.So the same request returns a canonical finish reason with
stream: trueand a non-canonical one with
stream: false. OpenAI-compatible clients keybehavior off the canonical set — the Vercel AI SDK only dispatches tool-call
handling on
finish_reason === "tool_calls"— so atool_usefinish overstream: falsesilently never runs the tool. This is the non-streaming mirrorof #2019, which fixed the same class in streaming for the same reason.
The
default:branch already canonicalizesabort→upstream_error(#3053),so this extends an existing, maintained mapping rather than adding a new
concept.
Fix
Mirror the sibling
mistral/novitabranch in thedefault:branch: mapend_turn→stopandtool_use→tool_callsalongside the existingaborthandling. +6/−1 lines, additive only; providers with their own
case(
aws-bedrock,anthropic,google-*,mistral,novita,alibaba) areuntouched.
Scope note: the branch serves 38 provider ids; for 29 of them the streaming
path already applies this exact mapping (the shared
caselist minus thethree with their own non-streaming branch), so this restores stream/non-stream
symmetry for those and extends the same canonicalization to the remaining
ones. Downstream per-provider fix-ups that key off the canonical values (the
zaiandscx-aiblocks right below) now see the mapped value, which onlymoves those paths towards more canonicalization.
alibabakeeps its owncase and is deliberately out of scope.
Tests
Two cases added to the existing
describe("openai-format finish reason mapping")block inparse-provider-response.spec.ts(next to the minimaxaborttest):tool_use→tool_callsfortogether-ai, with the tool call payloadpassing through;
end_turn→stopforgroq.Verification
Every command below was run and its output captured verbatim. The record is
reproducible — the exact commands are included so you can re-run them yourself.
red — new spec without the fix (only parse-provider-response.ts stashed) (must fail without the fix)
green — full spec with the fix (must pass) — 3 runs, identical exit code
Environment and output hashes
Gates (local, Windows):
tsc --noEmitonapps/gateway✅ · eslint ✅ on bothtouched files · prettier ✅ on both touched files (LF form, as committed) ·
git diff --checkclean · fullturbo run build --env-mode=loose✅ (17/17,including the
gatewaytypecheck). Area suitevitest run apps/gateway/src/chat: 40/43 files, 601/634 tests ✅ — all 33 failures sit inthree service-harness specs (
chat-resilience,managed-credentials,openai-content-filter) that need Postgres/Redis and fail withECONNREFUSED 127.0.0.1:6379on this Docker-less machine; none of themimports the touched module.
Not verified: a live round-trip against groq/together-ai (no provider keys on
this machine). The mapping itself is covered by the unit tests above, and the
affected provider list is the one the streaming branch already enumerates.
Disclosure: an AI coding assistant helped locate the asymmetry 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.
Summary by CodeRabbit
Bug Fixes
Tests