Repository navigation
fix(gateway): cache multi-candidate tool call ids - #3539
Conversation
…t_signature cache
parseProviderResponse caches Gemini thought signatures under the id it
emits (${name}_${shortid(24)}); the n>1 branch of transformResponseToOpenai
regenerated ${name}_${candidateIndex}_${fcIndex} ids and dropped
extra_content, so the next-turn thought_signature:<id> lookup missed and
Gemini rejected the replay with "Corrupted thought signature". Reuse the
parsed tool calls for candidate 0 (the one parse processes) and give
candidates 1+ the same treatment: unique shortid id, inline signature, and
the Redis entry under the emitted id.
Co-Authored-By: Codebuff <noreply@codebuff.com>
WalkthroughGoogle multi-candidate tool-call transformation now generates unique IDs for additional candidates, preserves candidate-specific thought signatures, caches signatures in Redis, and logs cache-write failures. Tests cover unique IDs, signature preservation, and cache isolation. ChangesGoogle tool-call signature preservation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GoogleCandidate
participant TransformResponse
participant Redis
participant Logger
GoogleCandidate->>TransformResponse: Provide candidate tool call and thought signature
TransformResponse->>Redis: Cache signature under emitted tool-call ID
Redis-->>TransformResponse: Return cache result
TransformResponse->>Logger: Log Redis write failure when applicable
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts (1)
304-399: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the cache-failure branch.
The new source path logs through
logger.errorwhenredisClient.setexrejects. No test drives that branch, so a regression there stays silent. Add a case that rejectssetexMockonce and asserts the transform still returns the tool call with its inlineextra_content.Also consider moving
setexMock.mockClear()into abeforeEachso later tests inherit a clean mock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts` around lines 304 - 399, Add a test for the redisClient.setex rejection path exercised by transformResponseToOpenai, configuring setexMock to reject once and asserting the transform still returns the tool call with its inline extra_content. Move setexMock.mockClear() into beforeEach so every test starts with a clean mock, while preserving the existing multi-candidate cache assertions.apps/gateway/src/chat/tools/transform-response-to-openai.ts (1)
476-505: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the tool-call construction and signature caching into a shared helper, and drop the
anyannotation.The id scheme, the
thought_signature:<id>key, and the 86400 TTL are now duplicated between this transformer andparseProviderResponse. A single helper keeps the two writers in sync and removes the magic TTL. The coding guidelines require DRY and prohibitanyunless absolutely necessary; a small local type is enough here.♻️ Suggested shape
- const toolCall: any = { + const toolCall: { + id: string; + type: "function"; + function: { name: string; arguments: string }; + extra_content?: { + google: { thought_signature: string }; + }; + } = { id: `${part.functionCall.name}_${shortid(24)}`, type: "function",Then move the
extra_contentassignment plus theredisClient.setexcall into a sharedcacheGoogleThoughtSignature(id, signature)helper used by both this file andparseProviderResponse, with the TTL exported as a named constant.As per coding guidelines: "never use
anyoras anyunless absolutely necessary" and "Apply DRY principles for reusable code".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/gateway/src/chat/tools/transform-response-to-openai.ts` around lines 476 - 505, Extract the tool-call creation and Google thought-signature caching from the mapped callback into a shared helper used by this transformer and parseProviderResponse. Replace toolCall’s any annotation with a local type, centralize the existing thought_signature:<id> key and 86400-second TTL in an exported named constant, and have cacheGoogleThoughtSignature handle extra_content assignment plus redisClient.setex while preserving the current error logging.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/gateway/src/chat/tools/transform-response-to-openai.spec.ts`:
- Around line 304-399: Add a test for the redisClient.setex rejection path
exercised by transformResponseToOpenai, configuring setexMock to reject once and
asserting the transform still returns the tool call with its inline
extra_content. Move setexMock.mockClear() into beforeEach so every test starts
with a clean mock, while preserving the existing multi-candidate cache
assertions.
In `@apps/gateway/src/chat/tools/transform-response-to-openai.ts`:
- Around line 476-505: Extract the tool-call creation and Google
thought-signature caching from the mapped callback into a shared helper used by
this transformer and parseProviderResponse. Replace toolCall’s any annotation
with a local type, centralize the existing thought_signature:<id> key and
86400-second TTL in an exported named constant, and have
cacheGoogleThoughtSignature handle extra_content assignment plus
redisClient.setex while preserving the current error logging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5e4e75c-797d-4bb4-a3f4-d5d6c91cf197
📒 Files selected for processing (2)
apps/gateway/src/chat/tools/transform-response-to-openai.spec.tsapps/gateway/src/chat/tools/transform-response-to-openai.ts
steebchen
changed the title
2 participants
Summary
parseProviderResponse(Google branch) caches Gemini thought signatures inRedis under the id it emits —
${name}_${shortid(24)}, asthought_signature:<id>— so the signature can be re-injected when the clientreplays the call next turn. The
n > 1branch oftransformResponseToOpenai(
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 matchesthe cached key. The next-turn lookup in
chat.ts(
redisClient.get("thought_signature:" + toolCall.id)) misses, the signatureis 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 > 1branch is the one path left behind: thesingle-candidate path reuses parse's
toolResultsand 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:
toolResults(whatparseProviderResponsealready emitted and cached), so the id the clientreceives is exactly the one the signature is cached under.
parse applies to candidate 0: a unique
${name}_${shortid(24)}id, theinline
extra_content.google.thought_signature, and thesetex("thought_signature:<id>", 86400, sig)Redis entry under the emittedid.
Fallback keeps the branch safe: if
toolResultsis absent for candidate 0(e.g. no tool calls parsed), the shortid path applies.
Tests
transform-response-to-openai.spec.ts:${name}_${candidateIndex}_${fcIndex}ids; it asserts the id scheme andthat candidates are distinct.
keeps multi-candidate Google tool_call ids in the thought_signature cache— mocks@llmgateway/cache(setex) and asserts:extra_contentwith its own signature, and asetex("thought_signature:<id>", 86400, sig)call under the emitted id;setexis written under choice 0's id from the transform (parsealready 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):
green — same probe after the fix:
(The deterministic shortid stub makes both candidate ids equal in the probe;
the real
shortidyields unique ids per candidate, asserted in the spec.)green — vitest, full spec:
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 whosefetch/Redis mocks time out in this environment; it imports only
openai-content-filter.ts, which this change does not touch (same class ofharness failures documented in previous PRs here).
Environment
Gates: eslint ✅ on both files (after
import/orderfix) · prettier ✅ ·git diff --checkclean · gateway typecheck ✅ (builds inbuild:core,turbo 12/12) ·
pnpm-lock.yamluntouched.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.tsbutin 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
setexassertions 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.
Summary by CodeRabbit