fix(chat): tool calls fixes & e2e tests - #675
Conversation
WalkthroughExtended streaming and response parsing to better handle tool_calls (anchoring, ID generation, and finish_reason logic), added tool-aware OpenAI routing and provider endpoint branching, sanitized tool parameter schemas, and introduced duplicated non-streaming E2E tests asserting tool-call flows and logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Gateway
participant Provider
rect rgb(250,250,253)
note over Gateway: Streaming tool-call accumulation & normalization
Client->>Gateway: POST /chat (messages + tools, stream?)
Gateway->>Provider: Stream request (provider-specific)
Provider-->>Gateway: streaming chunks (content_block_delta / tool_call fragments)
alt Anthropic content_block_delta
note right of Gateway: chunks include _contentBlockIndex
Gateway->>Gateway: Match/merge by _contentBlockIndex
else Other providers
note right of Gateway: chunks reference tool_call id
Gateway->>Gateway: Match/merge by id (Vertex uses indexed ids)
end
Gateway->>Gateway: Strip internal metadata (_contentBlockIndex), normalize finish_reason/tool_calls
Provider-->>Gateway: Final assistant content + assembled tool_calls
Gateway-->>Client: Streamed/aggregated response (public-safe)
end
sequenceDiagram
autonumber
participant Client
participant Gateway
participant OpenAI_Endpoint
rect rgb(250,250,253)
note over Gateway: OpenAI endpoint selection depends on hasExistingToolCalls & supportsReasoning
Client->>Gateway: POST /chat (messages may include tools/tool_calls)
Gateway->>Gateway: Detect hasExistingToolCalls flag
alt hasExistingToolCalls = false and supportsReasoning
Gateway->>OpenAI_Endpoint: /v1/responses (Responses API) with cleaned messages + sanitized tools
else
Gateway->>OpenAI_Endpoint: /v1/chat/completions (Chat API) with chat-style payload
end
OpenAI_Endpoint-->>Gateway: Response
Gateway-->>Client: Normalized assistant message (finish_reason/tool_calls adjusted)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/models/src/provider-api.ts (1)
28-36: btoa is not available in Node; base64 handling will throw
btoawill error server-side. Use Buffer when available and avoid materializing large binary strings.Apply:
- const isBase64 = url.includes(";base64,"); - const base64Data = isBase64 ? data : btoa(data); + const isBase64 = url.includes(";base64,"); + const base64Data = isBase64 + ? data + : (typeof Buffer !== "undefined" + ? Buffer.from(decodeURIComponent(data), "binary").toString("base64") + : btoa(decodeURIComponent(data)));and
- const uint8Array = new Uint8Array(arrayBuffer); - const binaryString = Array.from(uint8Array, (byte) => - String.fromCharCode(byte), - ).join(""); - const base64 = btoa(binaryString); + const uint8Array = new Uint8Array(arrayBuffer); + let base64: string; + if (typeof Buffer !== "undefined") { + base64 = Buffer.from(uint8Array).toString("base64"); + } else { + const binaryString = Array.from(uint8Array, (b) => String.fromCharCode(b)).join(""); + base64 = btoa(binaryString); + }Also consider a fetch timeout to avoid hangs on large files.
Also applies to: 98-104
apps/gateway/src/chat/chat.ts (2)
670-676: Unstable IDs for Google streaming tool calls prevent dedupe/mergeUsing
Date.now()yields a new id each chunk, so repeated functionCall parts cannot be matched/accumulated.Apply:
- id: part.functionCall.name + "_" + Date.now() + "_" + index, + id: `${part.functionCall.name}_${index}`,If multiple candidates stream, the index stays stable per candidate-part sequence, enabling consistent merging.
3413-3441: Anthropic tool-call merge by content block index is incorrect; array index != content_block indexYou index
streamingToolCalls[newCall._contentBlockIndex], butcontent_blockindices span all blocks (text/thinking/tool), not just tool_use. This often returns undefined; arguments won’t accumulate.Apply this merge fix:
- let existingCall = null; - // For Anthropic content_block_delta events, match by content block index - if (usedProvider === "anthropic" && newCall._contentBlockIndex !== undefined) { - existingCall = streamingToolCalls[newCall._contentBlockIndex]; - } else { - // For other providers and Anthropic content_block_start, match by ID - existingCall = streamingToolCalls.find((call) => call.id === newCall.id); - } + let existingCall = null; + // Prefer content_block index map for Anthropic partial_json deltas + if (usedProvider === "anthropic" && newCall._contentBlockIndex !== undefined) { + const mappedIdx = toolCallIndexMap[newCall._contentBlockIndex]; + if (mappedIdx !== undefined) existingCall = streamingToolCalls[mappedIdx]; + } else { + existingCall = streamingToolCalls.find((call) => call.id === newCall.id); + } @@ - // Clean up temporary fields and add new tool call - const cleanCall = { ...newCall }; + // Clean up temporary fields and add new tool call + const cleanCall = { ...newCall }; delete cleanCall._contentBlockIndex; - streamingToolCalls.push(cleanCall); + const pos = streamingToolCalls.push(cleanCall) - 1; + // For Anthropic tool_use starts, record the mapping for later deltas + if (usedProvider === "anthropic" && data.type === "content_block_start") { + toolCallIndexMap[data.index] = pos; + }And declare a per-stream map near the other accumulators:
// add near: let streamingToolCalls = null; const toolCallIndexMap: Record<number, number> = {};
🧹 Nitpick comments (2)
apps/gateway/src/chat/chat.ts (2)
1271-1273: Nit: pass actual provider when extracting imagesThis path always passes "google-ai-studio". Pass
usedProviderfor clarity (both Google branches share logic today).- delta.images = extractImagesFromProvider(data, "google-ai-studio"); + delta.images = extractImagesFromProvider(data, usedProvider);
226-236: Widespreadanyviolates our TS guidelineSeveral public helpers use
anyin params/returns, making downstream typing brittle (see parseProviderResponse, streaming transforms, extractors).Scope a follow-up to:
- Define unions for provider streaming events (Anthropic/Google/OpenAI)
- Type
toolResults/images with small interfaces- Replace
any[]withArray<...>across extractors
I can generate minimal types to cover these hotspots without overhauling the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/gateway/src/api.e2e.ts(1 hunks)apps/gateway/src/chat/chat.ts(4 hunks)packages/models/src/provider-api.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
packages/models/src/provider-api.tsapps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
packages/models/src/provider-api.tsapps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
packages/models/src/provider-api.tsapps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.ts
**/*.e2e.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name end-to-end test files with the .e2e.ts suffix
Files:
apps/gateway/src/api.e2e.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware
Files:
apps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.ts
🧬 Code graph analysis (2)
packages/models/src/provider-api.ts (1)
packages/db/src/types.ts (2)
tools(44-44)tool(18-21)
apps/gateway/src/api.e2e.ts (2)
apps/gateway/src/index.ts (1)
app(43-43)packages/db/src/schema.ts (2)
message(382-398)log(243-305)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / run
- GitHub Check: e2e / run
🔇 Additional comments (1)
packages/models/src/provider-api.ts (1)
390-391: Responsesinputmessage format may be too looseYou forward raw messages (string content) without converting to Responses content parts. If Responses now expects structured items (e.g., input_text/tool_result), this will 400. Verify and, if needed, wrap strings into content parts.
| test.each(toolCallModels)( | ||
| "tool calls with result $model", | ||
| getTestOptions(), | ||
| async ({ model }) => { | ||
| const res = await app.request("/v1/chat/completions", { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| Authorization: `Bearer real-token`, | ||
| }, | ||
| body: JSON.stringify({ | ||
| model: model, | ||
| messages: [ | ||
| { | ||
| role: "system", | ||
| content: | ||
| "You are Noemi, a thoughtful and clear assistant. Your tone is calm, minimal, and human. You write with intention—never too much, never too little. You avoid clichés, speak simply, and offer helpful, grounded answers. When needed, you ask good questions. You don't try to impress—you aim to clarify. You may use metaphors if they bring clarity, but you stay sharp and sincere. You're here to help the user think clearly and move forward, not to overwhelm or overperform.", | ||
| }, | ||
| { | ||
| role: "user", | ||
| content: "web search for the best ai notetaker apps!!!!", | ||
| }, | ||
| { | ||
| role: "assistant", | ||
| content: "", | ||
| tool_calls: [ | ||
| { | ||
| id: "toolu_015dgN1nk5Ay12iN8e16XPbs", | ||
| type: "function", | ||
| function: { | ||
| name: "webSearch", | ||
| arguments: '{"query":"best AI notetaker apps 2024"}', | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| role: "tool", | ||
| content: | ||
| '{"type":"webSearch","query":"best AI notetaker apps 2024","results":[{"title":"My Deep Dive into 25+ AI Note-Taking Apps (The Brutally ... - Reddit","href":"https://www.reddit.com/r/Zoom/comments/1jtbxkf/my_deep_dive_into_25_ai_notetaking_apps_the/","description":"The Good: Think Obsidian meets Miro. Whiteboard-style interface for connecting notes visually. AI assistant can generate summaries and do ..."},{"title":"The 9 best AI meeting assistants in 2025 - Zapier","href":"https://zapier.com/blog/best-ai-meeting-assistant/","description":"Granola automatically transcribes, summarizes, and analyzes your meetings. It also acts as a live notepad, allowing you to manually jot down ..."},{"title":"The Best AI Tools for Taking Notes in 2025 - PCMag","href":"https://www.pcmag.com/picks/best-ai-tools-taking-notes","description":"The popular note-taking app Notion now has AI tools. Notion AI excels at answering questions about your existing data, generating text from a prompt you give it ..."},{"title":"Top 5 BEST AI Note-Taking Apps (Better than Notion?) - YouTube","href":"https://www.youtube.com/watch?v=wGLd43TkCGc","description":"Voicenotes is a voice‑to‑text powerhouse that transcribes and extracts action items in one tap. · Saner is A distraction‑free workspace built for ..."},{"title":"9 Best AI Note-Taking Apps Built For Your Meetings - Quil\'s AI","href":"https://quil.ai/2024/09/12/9-best-ai-note-taking-apps-built-for-your-meetings/","description":"Quil.ai: The AI Note-taker Built for Recruiting Firms. 2. Notion: Write, Plan, Organize. 3. Jamie AI: The Bot-Free AI Note-taker."}],"timestamp":"2025-08-29T01:20:29.553Z"}', | ||
| tool_call_id: "toolu_015dgN1nk5Ay12iN8e16XPbs", | ||
| }, | ||
| ], | ||
| tools: [ | ||
| { | ||
| type: "function", | ||
| function: { | ||
| name: "webSearch", | ||
| description: "Search the web for information", | ||
| parameters: { | ||
| type: "object", | ||
| properties: { | ||
| query: { | ||
| type: "string", | ||
| description: "Search query", | ||
| }, | ||
| }, | ||
| required: ["query"], | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| tool_choice: "auto", | ||
| }), | ||
| }); | ||
|
|
||
| const json = await res.json(); | ||
| if (logMode) { | ||
| console.log( | ||
| "tool calls with empty content response:", | ||
| JSON.stringify(json, null, 2), | ||
| ); | ||
| } | ||
|
|
||
| // Log error response if status is not 200 | ||
| if (res.status !== 200) { | ||
| console.log( | ||
| `Error ${res.status} - tool calls with empty content response:`, | ||
| JSON.stringify(json, null, 2), | ||
| ); | ||
| } | ||
|
|
||
| expect(res.status).toBe(200); | ||
| expect(json).toHaveProperty("choices"); | ||
| expect(json.choices).toHaveLength(1); | ||
| expect(json.choices[0]).toHaveProperty("message"); | ||
|
|
||
| const message = json.choices[0].message; | ||
| expect(message).toHaveProperty("role", "assistant"); | ||
|
|
||
| // Should have proper content (not empty) as a response to the tool call | ||
| expect(message).toHaveProperty("content"); | ||
| expect(typeof message.content).toBe("string"); | ||
| expect(message.content.length).toBeGreaterThan(0); | ||
|
|
||
| // Should have finish reason as stop (not tool_calls since this is a response) | ||
| expect(json.choices[0]).toHaveProperty("finish_reason", "stop"); | ||
|
|
||
| // Validate logs | ||
| const log = await validateLogs(); | ||
| expect(log.streamed).toBe(false); | ||
|
|
||
| // Validate usage | ||
| expect(json).toHaveProperty("usage"); | ||
| expect(json.usage).toHaveProperty("prompt_tokens"); | ||
| expect(json.usage).toHaveProperty("completion_tokens"); | ||
| expect(json.usage).toHaveProperty("total_tokens"); | ||
| expect(typeof json.usage.prompt_tokens).toBe("number"); | ||
| expect(typeof json.usage.completion_tokens).toBe("number"); | ||
| expect(typeof json.usage.total_tokens).toBe("number"); | ||
| expect(json.usage.prompt_tokens).toBeGreaterThan(0); | ||
| expect(json.usage.completion_tokens).toBeGreaterThan(0); | ||
| expect(json.usage.total_tokens).toBeGreaterThan(0); | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Assert no residual tool_calls in final turn and log
Strengthen the test to ensure the assistant returned an actual answer (no further tool calls) and logs don’t record provider-emitted tool calls in this “with result” path.
Apply:
- const log = await validateLogs();
+ const log = await validateLogs();
expect(log.streamed).toBe(false);
+ // final turn should be a normal answer, not another tool call
+ expect(json.choices[0].finish_reason).toBe("stop");
+ expect(log.toolResults).toBeNull();Would you like a streaming variant of this test for parity?
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/gateway/src/api.e2e.ts around lines 886 to 1001, strengthen the test by
asserting that the final assistant message contains no residual tool_calls and
that the request/processing logs do not record provider-emitted tool calls:
after obtaining message, add an assertion that message.tool_calls is either
undefined/null or an empty array (and fail if any tool_call entries exist), and
extend the validateLogs() checks to assert the logs object does not contain
provider/tool-call entries (e.g., no providerToolCalls or no log entries with
type "tool_call"); keep assertions consistent with existing log shape returned
by validateLogs().
| // Clean messages by removing tool_calls and tool_call_id fields | ||
| // Also transform 'tool' role to 'user' role since responses API doesn't support 'tool' role | ||
| const cleanedMessages = messages.map((msg) => { | ||
| const { tool_calls: _, tool_call_id: __, ...cleanMsg } = msg; | ||
|
|
||
| // Transform tool role to user role for responses API compatibility | ||
| if (cleanMsg.role === "tool") { | ||
| return { | ||
| ...cleanMsg, | ||
| role: "user", | ||
| }; | ||
| } | ||
|
|
||
| return cleanMsg; | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
OpenAI Responses: dropping tool_calls/tool_call_id and coercing 'tool' → 'user' likely breaks tool-result turns
Removing tool_calls and tool_call_id and rewriting role "tool" to "user" loses the association between prior function calls and their results for reasoning models routed via the Responses API. The model will not see tool outputs, and follow-up assistant replies may be wrong.
Please confirm the correct, current Responses API shape for passing prior tool calls and tool results. If Responses requires content items, map:
- assistant+tool_calls → function_call items
- tool messages → tool_result items referencing the call id
I can provide a concrete mapping once you confirm the expected payload. Would you like me to adjust this block to preserve tool context for Responses?
🌐 Web query:
OpenAI Responses API: How do you include previous tool calls and their results in `input`? Are `role:"tool"` messages supported, or must we send `{"type":"tool_result","tool_call_id":...}` items? Point to the official doc/example.
💡 Result:
Short answer: Use the assistant message with a "tool_calls" entry (id + function info) followed by a separate message with role:"tool", tool_call_id set to that id, and content set to the tool result. You do not send {"type":"tool_result",...} — the expected format is assistant.tool_calls then a role:"tool" message that references tool_call_id. See the official example. [1][2]
Sources:
- OpenAI Cookbook example showing tool-calling flow (assistant message with tool call, then a tool message with tool_call_id and content). [1]
- Official docs / examples referenced on OpenAI forums showing the same message shape (assistant.tool_calls → tool role message with tool_call_id → assistant interpreting the result). [2]
Preserve tool_calls and tool-role messages for Responses API
Don’t strip tool_calls/tool_call_id or coerce role:"tool" to "user"—the Responses API expects an assistant message with tool_calls followed by a role:"tool" message referencing that tool_call_id to relay the tool result.
Location: packages/models/src/provider-api.ts lines 372–386
🤖 Prompt for AI Agents
In packages/models/src/provider-api.ts around lines 372–386, the code currently
strips tool_calls and tool_call_id and coerces role:"tool" to "user"; revert
that behaviour so tool_calls and tool_call_id are preserved and role:"tool"
remains unchanged for messages destined to the Responses API. Update the mapping
to remove only unsupported/internal fields (if any) but keep tool_calls and
tool_call_id intact, and do not change the role value; ensure resulting messages
still conform to the Responses API schema (preserve assistant message with
tool_calls followed by the tool message that references tool_call_id).
| functionDeclarations: tools.map((tool: any) => { | ||
| // Remove additionalProperties and $schema from parameters as Google doesn't accept them | ||
| const { | ||
| additionalProperties: _additionalProperties, | ||
| $schema: _$schema, | ||
| ...cleanParameters | ||
| } = tool.function.parameters || {}; | ||
| return { | ||
| name: tool.function.name, | ||
| description: tool.function.description, | ||
| parameters: cleanParameters, | ||
| }; | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Sanitize tool parameter schemas deeply (nested $schema/additionalProperties)
Only top-level keys are stripped. Nested objects may still carry these fields, causing Google 400s. Consider a recursive prune before assigning parameters.
Apply this refactor in the mapper:
function stripMeta(o: unknown): unknown {
if (!o || typeof o !== "object") return o;
if (Array.isArray(o)) return o.map(stripMeta);
// shallow clone and recurse
const { additionalProperties, $schema, ...rest } = o as Record<string, unknown>;
for (const k of Object.keys(rest)) rest[k] = stripMeta(rest[k]);
return rest;
}Then:
- const { additionalProperties: _additionalProperties, $schema: _$schema, ...cleanParameters } = tool.function.parameters || {};
+ const cleanParameters = stripMeta(tool.function.parameters || {});🤖 Prompt for AI Agents
In packages/models/src/provider-api.ts around lines 612 to 624, the mapper
currently only strips top-level additionalProperties and $schema from
tool.function.parameters which leaves nested occurrences and causes Google 400
errors; implement a recursive sanitizer (e.g., stripMeta) that returns
primitives as-is, maps arrays by recursing, and for objects shallow-clones while
removing additionalProperties and $schema then recursing into each remaining
property; replace the existing shallow destructure with parameters:
stripMeta(tool.function.parameters) so all nested occurrences are removed before
assigning parameters.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/gateway/src/chat/chat.ts (3)
657-672: Anthropic tool_use deltas: matching by array index breaks for mixed content blocks
streamingToolCalls[newCall._contentBlockIndex]assumes content-block index == tool-call ordinal, which fails when non-tool blocks precede tool_use. Result: arguments won’t merge into the right call.
- Include
_contentBlockIndexon content_block_start.- Match by
_contentBlockIndexvia find(), not array indexing.- Keep
_contentBlockIndexduring streaming; strip before persisting/logging.@@ - if ( - data.type === "content_block_start" && - data.content_block?.type === "tool_use" - ) { + if ( + data.type === "content_block_start" && + data.content_block?.type === "tool_use" + ) { return [ { - id: data.content_block.id, + id: data.content_block.id, + _contentBlockIndex: data.index, type: "function", function: { name: data.content_block.name, arguments: "", }, }, ]; } @@ - if (data.type === "content_block_delta" && data.delta?.partial_json) { + if (data.type === "content_block_delta" && data.delta?.partial_json) { return [ { _contentBlockIndex: data.index, // Use this for matching type: "function", function: { name: "", arguments: data.delta.partial_json, }, }, ]; }
3422-3450: Fix Anthropic streaming merge: match by content-block index; don’t delete anchor prematurelyDeleting
_contentBlockIndexon insert prevents future deltas from matching. Also, array-index addressing is wrong; use a property match.- for (const newCall of toolCallsChunk) { - let existingCall = null; - // For Anthropic content_block_delta events, match by content block index - if ( - usedProvider === "anthropic" && - newCall._contentBlockIndex !== undefined - ) { - existingCall = - streamingToolCalls[newCall._contentBlockIndex]; - } else { - // For other providers and Anthropic content_block_start, match by ID - existingCall = streamingToolCalls.find( - (call) => call.id === newCall.id, - ); - } + for (const newCall of toolCallsChunk) { + let existingCall = null; + if ( + usedProvider === "anthropic" && + newCall._contentBlockIndex !== undefined + ) { + existingCall = streamingToolCalls.find( + (call) => call._contentBlockIndex === newCall._contentBlockIndex, + ); + } else if (newCall.id) { + existingCall = streamingToolCalls.find((call) => call.id === newCall.id); + } @@ - } else { - // Clean up temporary fields and add new tool call - const cleanCall = { ...newCall }; - delete cleanCall._contentBlockIndex; - streamingToolCalls.push(cleanCall); - } + } else { + // Keep _contentBlockIndex for internal matching; strip before persisting + streamingToolCalls.push({ ...newCall }); + }Outside this range, before persisting/logging:
// sanitize before insertLog / cache writes const toolResultsClean = streamingToolCalls?.map(({ _contentBlockIndex, ...rest }) => rest) || null;Then use
toolResults: toolResultsClean.
3759-3771: Streaming cache metadata omits toolResults; cached replays then log null toolResultsYou read
metadata.toolResultson cache hit (Line 2605), but you never write it here. Add sanitized toolResults to metadata.metadata: { model: usedModel, provider: usedProvider, finishReason: finishReason, totalChunks: streamingChunks.length, duration: duration, completed: true, + toolResults: + (streamingToolCalls + ? streamingToolCalls.map(({ _contentBlockIndex, ...rest }) => rest) + : null), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/gateway/src/chat/chat.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/gateway/src/chat/chat.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/chat/chat.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/gateway/src/chat/chat.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware
Files:
apps/gateway/src/chat/chat.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / run
- GitHub Check: e2e / run
🔇 Additional comments (3)
apps/gateway/src/chat/chat.ts (3)
415-418: Finish reason mapping for OpenAI Responses “completed” → stop — LGTMThis aligns with downstream expectations and avoids leaking spurious tool_calls.
440-448: Guard prevents false “tool_calls” when no tool results — LGTMCorrectly normalizes to “stop” when content is present but no tool_calls exist.
1333-1339: Gate finish_reason for Google streaming chunks same as non-streaming Mirror the non-streaming finish_reason logic to avoid spurious “tool_calls”; since per-chunk handling can’t track accumulated tool calls, defer final normalization to the server or only emit “tool_calls” on the last chunk.
Enhanced e2e tests to include tool call validations, usage reporting, and detailed assertions for result completeness and consistency. Removed unused model filtering logic.
Google's API doesn't accept additionalProperties and $schema fields in tool function parameters. This fix removes these JSON Schema fields when transforming tools for Google providers. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed two critical tool call bugs in streaming response handling: 1. **Anthropic streaming tool call ID bug**: Fixed content_block_delta events that incorrectly used `data.index` (which doesn't exist) to generate tool IDs. Now properly matches tool call deltas to their corresponding tool calls using content block index. 2. **Google tool ID collision risk**: Enhanced Google provider tool ID generation to include index suffix alongside timestamp to prevent potential ID collisions in rapid successive tool calls. These fixes ensure tool call arguments are properly accumulated during streaming and prevent tool call validation failures in e2e tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed issue where tool_calls and tool_call_id fields were being passed through to the OpenAI responses API in the input messages, causing "Unknown parameter: 'input[2].tool_calls'" errors. The responses API expects clean message objects without these OpenAI-specific tool call fields. Now properly strips tool_calls and tool_call_id from messages before sending to the responses API. Resolves tool call issues with GPT-5 and other reasoning-capable models that use the responses API format. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed issue where 'tool' role messages were being passed to OpenAI responses API, causing "Invalid value: 'tool'" errors since the responses API only supports 'assistant', 'system', 'developer', and 'user' roles. Now properly transforms 'tool' role messages to 'user' role for responses API compatibility while maintaining the tool functionality through the separate tools array. Resolves tool call role validation errors with GPT-5 and other reasoning-capable models. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed multiple issues with finish_reason determination in tool call scenarios: 1. **Fixed finish_reason for OpenAI responses API**: Removed incorrect logic that set finish_reason to "tool_calls" based on presence of tool_calls in response. Now properly respects the actual API status. 2. **Fixed finish_reason for Google streaming**: Corrected logic that incorrectly used hasFunctionCalls presence to set finish_reason to "tool_calls". 3. **Added ZAI provider fix**: ZAI API incorrectly returns finish_reason: "tool_calls" even for final responses. Added correction logic to detect when there's content but no new tool_calls, and properly set finish_reason to "stop". These fixes ensure finish_reason is "tool_calls" only when the assistant is making tool calls, not when responding after tool use. Resolves AssertionError for glm-4.5-airx and other models expecting finish_reason: "stop" for chat completion responses. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Enhanced ZAI response logic to address issues with incorrect `finish_reason` values for specific models. Added safeguards to properly handle tool calls and prevent invalid states when processing final responses. Updates include: 1. Added `messages` parameter to pass chat history for validation. 2. Adjusted tool call detection and `finish_reason` mapping logic. 3. Applied fixes specifically for models `glm-4.5-airx` and `glm-4.5-flash` exhibiting test failures. Improves accuracy of response handling and resolves assertion failures in affected test cases.
a8b0b93 to
adf7189
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/models/src/provider-api.ts (2)
372-386: OpenAI Responses: do not strip tool_calls/tool_call_id or coerce role "tool" → "user"This breaks tool-result linking and follow-up reasoning for Responses. Preserve tool_calls and tool role; strip only internal fields.
Apply:
- // Clean messages by removing tool_calls and tool_call_id fields - // Also transform 'tool' role to 'user' role since responses API doesn't support 'tool' role - const cleanedMessages = messages.map((msg) => { - const { tool_calls: _, tool_call_id: __, ...cleanMsg } = msg; - - // Transform tool role to user role for responses API compatibility - if (cleanMsg.role === "tool") { - return { - ...cleanMsg, - role: "user", - }; - } - - return cleanMsg; - }); + // Preserve tool_calls/tool_call_id and role:"tool" for Responses + const messagesForResponses = messages.map((msg) => { + const { _debug, ...rest } = msg; // drop only transient internals + return rest; + }); @@ - input: cleanedMessages, + input: messagesForResponses,To confirm current Responses schema around prior tool calls/results:
OpenAI Responses API (Aug 2025): What is the exact, supported way to include prior tool calls and their tool results in the `input`? Are `role:"tool"` messages with `tool_call_id` supported, or is a different shape required? Please provide the official doc/example.Also applies to: 390-391
612-624: Sanitize tool parameter schemas deeply (nested $schema/additionalProperties)Current destructure only strips top-level keys; nested occurrences still cause Google 400s. Use a recursive prune.
Apply within this block:
- functionDeclarations: tools.map((tool: any) => { - // Remove additionalProperties and $schema from parameters as Google doesn't accept them - const { - additionalProperties: _additionalProperties, - $schema: _$schema, - ...cleanParameters - } = tool.function.parameters || {}; - return { - name: tool.function.name, - description: tool.function.description, - parameters: cleanParameters, - }; - }), + functionDeclarations: tools.map((tool: any) => ({ + name: tool.function.name, + description: tool.function.description, + parameters: stripMeta(tool.function.parameters || {}), + })),Add once in this file (top-level helper):
function stripMeta(o: unknown): unknown { if (o === null || typeof o !== "object") return o; if (Array.isArray(o)) return o.map(stripMeta); const { additionalProperties, $schema, ...rest } = o as Record<string, unknown>; for (const k of Object.keys(rest)) rest[k] = stripMeta(rest[k]); return rest; }apps/gateway/src/chat/chat.ts (2)
709-711: Vertex streaming tool_call IDs must be deterministic (drop Date.now())Use candidate index + position to enable accumulation across chunks.
- .map((part: any, index: number) => ({ - id: part.functionCall.name + "_" + Date.now() + "_" + index, + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${data.candidates?.[0]?.index ?? 0}_${index}`,
836-841: Gate Google finish_reason "tool_calls" on actual functionCall partsAvoid reporting tool_calls when none were parsed.
- finish_reason: - finishReason === "STOP" - ? "stop" - : finishReason === "tool_calls" || - finishReason === "function_call" - ? "tool_calls" - : finishReason?.toLowerCase() || "stop", + finish_reason: + finishReason === "STOP" + ? "stop" + : ((finishReason === "tool_calls" || finishReason === "function_call") && + (toolResults && toolResults.length > 0)) + ? "tool_calls" + : finishReason?.toLowerCase() || "stop",
🧹 Nitpick comments (2)
apps/gateway/src/chat/chat.ts (2)
339-346: Google (non-streaming): use deterministic tool_call IDs instead of Date.now()Timestamps prevent idempotent merges and make tests flaky. Derive from stable fields.
- .map((part: any) => ({ - id: part.functionCall.name + "_" + Date.now(), // Google doesn't provide ID, so generate one + .map((part: any) => ({ + id: `${part.functionCall.name}_${json.candidates?.[0]?.index ?? 0}_0`,
3450-3478: Anthropic streaming accumulation: index-based lookup is brittleAccessing by array index assumes order of content_block_start before deltas and 1:1 index alignment. Use a Map keyed by content block index for O(1) safe merges.
Example (outside this range): maintain
- const toolCallsByIndex = new Map<number, any>();
- On start: toolCallsByIndex.set(data.index, newCall)
- On delta: const existing = toolCallsByIndex.get(data.index)
- When flushing: streamingToolCalls = Array.from(toolCallsByIndex.values())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/gateway/src/api.e2e.ts(1 hunks)apps/gateway/src/chat/chat.ts(10 hunks)packages/models/src/provider-api.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/gateway/src/api.e2e.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
packages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
packages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
packages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/chat/chat.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/chat/chat.ts⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: e2e / run
- GitHub Check: build / run
🔇 Additional comments (2)
apps/gateway/src/chat/chat.ts (2)
419-427: LGTM: Responses finish_reason gating on tool callsCorrectly maps "completed" → "tool_calls" only when toolResults exist; else "stop".
226-230: All parseProviderResponse calls include the new messages parameter.
This reverts commit adf7189.
This reverts commit 8713827.
…s API" This reverts commit 001bf4b.
…es API" This reverts commit 100bffb.
Improved ZAI and OpenAI compatibility by addressing finish_reason inaccuracies and unsupported tool role messages. Changes include: 1. Added support for `messages` parameter in parseProviderResponse. 2. Resolved ZAI-specific finish_reason issues for glm-4.5 models. 3. Cleaned and transformed tool-related fields for OpenAI responses API.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/gateway/src/chat/chat.ts (3)
337-346: Vertex/AI Studio: make tool_call IDs deterministic (non-streaming too)Using Date.now() produces non-deterministic IDs and complicates deduplication/logging.
- .map((part: any) => ({ - id: part.functionCall.name + "_" + Date.now(), // Google doesn't provide ID, so generate one + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${json.candidates?.[0]?.index ?? 0}_${index}`, type: "function", function: { name: part.functionCall.name, arguments: JSON.stringify(part.functionCall.args || {}), }, })) || null;
670-700: Anthropic streaming: include content block index on tool_start and keep partials internalYou add
_contentBlockIndexfor deltas, but not for starts, and later merge assumes index-based access. Provide_contentBlockIndexon starts to enable reliable accumulation.if ( data.type === "content_block_start" && data.content_block?.type === "tool_use" ) { return [ { id: data.content_block.id, + _contentBlockIndex: data.index, type: "function", function: { name: data.content_block.name, arguments: "", }, }, ]; } @@ - if (data.type === "content_block_delta" && data.delta?.partial_json) { + if (data.type === "content_block_delta" && data.delta?.partial_json) { // Return a partial tool call with the index to help with matching return [ { _contentBlockIndex: data.index, // Use this for matching type: "function", function: { name: "", arguments: data.delta.partial_json, }, }, ]; }Additionally, prefer an internal index map over positional array access in the merge (see next comment).
3453-3480: Fix Anthropic tool_call accumulation: current index-based access can drop/duplicate args
streamingToolCalls[newCall._contentBlockIndex]assumes array indices match content block indices, which often isn’t true. Result: missed merges and duplicate calls.Apply:
- // For Anthropic content_block_delta events, match by content block index - if ( - usedProvider === "anthropic" && - newCall._contentBlockIndex !== undefined - ) { - existingCall = - streamingToolCalls[newCall._contentBlockIndex]; - } else { - // For other providers and Anthropic content_block_start, match by ID - existingCall = streamingToolCalls.find( - (call) => call.id === newCall.id, - ); - } + // Use an internal map to correlate content_block_index -> array slot + let existingCall = null; + if (usedProvider === "anthropic" && newCall._contentBlockIndex !== undefined) { + const slot = anthropicToolCallsByIndex.get(newCall._contentBlockIndex); + if (slot !== undefined) { + existingCall = streamingToolCalls[slot]; + } + } else if (newCall.id) { + existingCall = streamingToolCalls.find((call) => call.id === newCall.id); + } @@ - // Clean up temporary fields and add new tool call - const cleanCall = { ...newCall }; - delete cleanCall._contentBlockIndex; - streamingToolCalls.push(cleanCall); + // Add new tool call and register index mapping when available + const cleanCall = { ...newCall }; + delete (cleanCall as any)._contentBlockIndex; + streamingToolCalls.push(cleanCall); + if (usedProvider === "anthropic" && newCall._contentBlockIndex !== undefined) { + anthropicToolCallsByIndex.set( + newCall._contentBlockIndex, + streamingToolCalls.length - 1, + ); + }Add this helper near the other streaming state (around Line 3060):
// Map Anthropic content block index -> position in streamingToolCalls const anthropicToolCallsByIndex = new Map<number, number>();
♻️ Duplicate comments (2)
apps/gateway/src/chat/chat.ts (2)
709-716: Vertex streaming tool_call IDs shouldn’t use Date.now(); derive deterministic IDs (repeat)Use stable ID composed from function name + candidate index + per-candidate part index; enables accumulation across chunks.
- .map((part: any, index: number) => ({ - id: part.functionCall.name + "_" + Date.now() + "_" + index, + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${data.candidates?.[0]?.index ?? 0}_${index}`, type: "function", function: { name: part.functionCall.name, arguments: JSON.stringify(part.functionCall.args || {}), }, })) || null
449-476: Don’t mutate provider JSON; switch to local normalization (repeat)You change
json.choices[0].finish_reasonand deletemessage.tool_calls. This risks divergence across logs/cache/upstream payloads.Use local variables and ignore unexpected tool calls instead:
- if (hasNewToolCalls) { - finishReason = "stop"; - // Also update JSON to match - if (json.choices?.[0]) { - json.choices[0].finish_reason = "stop"; - delete json.choices[0].message.tool_calls; - } - } + if (hasNewToolCalls) { + // Normalize locally; do not edit provider JSON + finishReason = "stop"; + toolResults = null; + }
🧹 Nitpick comments (4)
apps/gateway/src/chat/chat.ts (4)
226-230: Add messages param: good, but replaceanywith safer typesAvoid
anyin TS. Preferunknownwith targeted narrowing; and type messages as a minimal shape you actually use.Apply:
-function parseProviderResponse( - usedProvider: Provider, - json: any, - messages: any[] = [], -) { +function parseProviderResponse( + usedProvider: Provider, + json: Record<string, unknown>, + messages: Array<{ role: ChatMessage["role"]; content: unknown; name?: string }> = [], +) {
834-839: Gate Google finish_reason “tool_calls” on actual tool calls for all casesCurrently only “STOP” is gated. Also gate when provider reports “tool_calls”/“function_call” but parsed toolResults are empty.
- finish_reason: - finishReason === "STOP" - ? toolResults && toolResults.length > 0 - ? "tool_calls" - : "stop" - : finishReason?.toLowerCase() || "stop", + finish_reason: + finishReason === "STOP" + ? toolResults && toolResults.length > 0 + ? "tool_calls" + : "stop" + : ((finishReason === "tool_calls" || finishReason === "function_call") && + toolResults && toolResults.length > 0) + ? "tool_calls" + : finishReason?.toLowerCase() || "stop",
1311-1311: Minor: pass actual provider to image extractorHardcoding "google-ai-studio" is confusing; use
usedProvider.- delta.images = extractImagesFromProvider(data, "google-ai-studio"); + delta.images = extractImagesFromProvider(data, usedProvider);
1005-1024: Token calculation fallback: prefer== nullchecks to avoid treating 0 as missingNot a blocker for this PR, but
!promptTokens/!completionTokenstreats 0 as falsy. Use== nullguards to avoid unnecessary estimation when legitimate 0s occur (e.g., tool-only or reasoning-only outputs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/gateway/src/chat/chat.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/gateway/src/chat/chat.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/chat/chat.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/gateway/src/chat/chat.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/chat/chat.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/chat/chat.ts🧬 Code graph analysis (1)
apps/gateway/src/chat/chat.ts (1)
packages/models/src/models.ts (1)
Provider(18-18)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e / run
🔇 Additional comments (2)
apps/gateway/src/chat/chat.ts (2)
419-427: Correctly gate OpenAI Responses finish_reason on presence of tool callsGood: “completed” maps to tool_calls only when toolResults exist. LGTM.
4045-4045: Caller updated to pass messages to parseProviderResponseGood integration; enables better token estimation and ZAI logic. LGTM.
f855184 to
b146ff3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/gateway/src/chat/chat.ts (3)
3542-3570: Fix Anthropic streaming tool_call accumulation for content_block_deltaCurrent merge by array index can miss matches; ensure deltas slot into the correct index when provided.
- // Clean up temporary fields and add new tool call - const cleanCall = { ...newCall }; - delete cleanCall._contentBlockIndex; - streamingToolCalls.push(cleanCall); + // Clean up temporary fields and add new tool call + const cleanCall = { ...newCall }; + const idx = cleanCall._contentBlockIndex; + delete cleanCall._contentBlockIndex; + if (usedProvider === "anthropic" && idx !== undefined) { + streamingToolCalls[idx] = cleanCall; + } else { + streamingToolCalls.push(cleanCall); + }Additionally, set _contentBlockIndex on content_block_start in extractToolCallsFromProvider so starts also have the index (can do in a follow-up if preferred).
4137-4141: Remove production logging of response metadata (PII risk, large payloads)These logs can leak base64 images and provider/model details in prod.
- // Debug: Log images found in response - console.log("Gateway - parseProviderResponse extracted images:", images); - console.log("Gateway - Used provider:", usedProvider); - console.log("Gateway - Used model:", usedModel); + // Consider enabling under a debug flag only (env-gated) if needed locally
4194-4199: Normalize totalTokens to string for DB consistencyOther token fields are stringified; totalTokens should be too.
- totalTokens: - totalTokens || - ( - (calculatedPromptTokens || 0) + (calculatedCompletionTokens || 0) - ).toString(), + totalTokens: + totalTokens != null + ? String(totalTokens) + : String((calculatedPromptTokens || 0) + (calculatedCompletionTokens || 0)),
♻️ Duplicate comments (3)
apps/gateway/src/api.e2e.ts (1)
977-987: Assert no residual tool_calls and null toolResults in logs for “with result” pathPrevent regressions where providers leak tool_calls in the final answer or logs still contain toolResults.
- // Should have proper content (not empty) as a response to the tool call + // Should have proper content (not empty) as a response to the tool call expect(message).toHaveProperty("content"); expect(typeof message.content).toBe("string"); expect(message.content.length).toBeGreaterThan(0); - // Should have finish reason as stop (not tool_calls since this is a response) + // Final turn should not contain residual tool_calls + expect( + !message.tool_calls || (Array.isArray(message.tool_calls) && message.tool_calls.length === 0) + ).toBe(true); + + // Should have finish reason as stop (not tool_calls since this is a response) expect(json.choices[0]).toHaveProperty("finish_reason", "stop"); // Validate logs const log = await validateLogs(); expect(log.streamed).toBe(false); + expect(log.toolResults).toBeNull(); + expect(log.unifiedFinishReason?.toLowerCase()).toBe("stop");Also applies to: 988-1003
apps/gateway/src/chat/chat.ts (2)
709-716: Vertex/Google streaming tool_call IDs should be deterministic, not Date.now()Use candidate and part indices. This also enables accumulation/debugging parity with non-streaming.
- .map((part: any, index: number) => ({ - id: part.functionCall.name + "_" + Date.now() + "_" + index, + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${data.candidates?.[0]?.index ?? 0}_${index}`, type: "function",
449-476: Don’t mutate upstream provider JSON; fix ZAI finish_reason correctionIn-place edits to json risk divergence between logs, cache, and the response. Set local variables instead.
- if (hasNewToolCalls) { - finishReason = "stop"; - // Also update JSON to match - if (json.choices?.[0]) { - json.choices[0].finish_reason = "stop"; - delete json.choices[0].message.tool_calls; - } - } + if (hasNewToolCalls) { + finishReason = "stop"; + toolResults = null; // ignore unexpected tool_calls in tool-response turn + }
🧹 Nitpick comments (3)
apps/gateway/src/chat/chat.ts (3)
340-346: Use deterministic IDs for Google tool calls (non-streaming)Avoid Date.now()-based IDs to keep IDs stable for testing/logging.
- .map((part: any) => ({ - id: part.functionCall.name + "_" + Date.now(), // Google doesn't provide ID, so generate one + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${json.candidates?.[0]?.index ?? 0}_${index}`, type: "function",
4087-4089: Avoid type assertion on status codeReturning res.status is sufficient; asserting “as 400” is misleading.
- return c.json(originalError, res.status as 400); + return c.json(originalError, res.status);
2156-2315: Refactor fallback to use available providers
The fallback clause hardcodesusedProvider = "openai"andusedModel = "gpt-5-nano", which may not be inavailableProvidersand can cause 400/500 errors. Consider an optional refactor to pick the cheapest provider from all models whoseproviderIdis inavailableProvidersand whosecontextSizemeetsrequiredContextSize, throwing an HTTP 400 if none qualify:--- a/apps/gateway/src/chat/chat.ts +++ b/apps/gateway/src/chat/chat.ts @@ -2311,7 +2311,28 @@ - } else { - // Default fallback if no suitable model is found - use cheapest allowed model - usedModel = "gpt-5-nano"; - usedProvider = "openai"; - } + } else { + // Fallback: choose cheapest available provider across all models meeting context + const candidates = models + .filter((m) => m.id !== "auto" && m.id !== "custom") + .flatMap((m) => + m.providers + .filter((p) => availableProviders.includes(p.providerId)) + .filter((p) => (p.contextSize ?? 8192) >= requiredContextSize) + .map((p) => ({ model: m, provider: p })), + ); + if (candidates.length > 0) { + const cheapest = candidates.reduce((acc, cur) => { + const price = ((cur.provider.inputPrice || 0) + (cur.provider.outputPrice || 0)) / 2; + const accPrice = ((acc.provider.inputPrice || 0) + (acc.provider.outputPrice || 0)) / 2; + return price < accPrice ? cur : acc; + }); + usedProvider = cheapest.provider.providerId as Provider; + usedModel = cheapest.provider.modelName as any; + } else { + throw new HTTPException(400, { message: "No available provider meets context requirements" }); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/gateway/src/api.e2e.ts(1 hunks)apps/gateway/src/chat/chat.ts(17 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/api.e2e.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/api.e2e.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/api.e2e.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/api.e2e.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/chat/chat.tsapps/gateway/src/api.e2e.ts**/*.e2e.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name end-to-end test files with the .e2e.ts suffix
Files:
apps/gateway/src/api.e2e.ts🧬 Code graph analysis (2)
apps/gateway/src/chat/chat.ts (3)
packages/models/src/models.ts (3)
Provider(18-18)ModelDefinition(89-130)models(132-147)apps/ui/src/hooks/useChats.ts (1)
ChatMessage(16-23)packages/models/src/provider-api.ts (1)
getCheapestFromAvailableProviders(874-898)apps/gateway/src/api.e2e.ts (2)
apps/gateway/src/index.ts (1)
app(43-43)packages/db/src/schema.ts (2)
message(382-398)log(243-305)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build / run
- GitHub Check: e2e / run
Revised tool ID logic to include candidate index and part index for improved uniqueness.
Revised test cases to validate either `message.content` is non-empty or `message.tool_calls` is present. Adjusted test content to ensure exact details are considered when querying the weather tool.
Refactored to detect existing tool calls in messages and switch to the appropriate API. Simplified message cleaning logic to align with API requirements.
Added logic to check for existing tool calls or tool role in messages. This ensures the correct API (Chat Completions or Responses API) is selected based on message content.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/models/src/provider-api.ts (1)
681-693: Deeply strip unsupported schema keys in Google tool parameters (fix nested $schema/additionalProperties).Current destructure removes only top-level keys; nested ones will still 400. Use a recursive sanitizer.
Apply within this hunk:
- functionDeclarations: tools.map((tool: any) => { - // Remove additionalProperties and $schema from parameters as Google doesn't accept them - const { - additionalProperties: _additionalProperties, - $schema: _$schema, - ...cleanParameters - } = tool.function.parameters || {}; - return { - name: tool.function.name, - description: tool.function.description, - parameters: cleanParameters, - }; - }), + functionDeclarations: tools.map((tool: any) => { + const cleanParameters = stripMeta(tool.function.parameters || {}); + return { + name: tool.function.name, + description: tool.function.description, + parameters: cleanParameters, + }; + }),Add once near the top of the file:
function stripMeta(o: unknown): unknown { if (!o || typeof o !== "object") return o; if (Array.isArray(o)) return o.map(stripMeta); const { additionalProperties, $schema, ...rest } = o as Record<string, unknown>; for (const k of Object.keys(rest)) rest[k] = stripMeta(rest[k]); return rest; }apps/gateway/src/api.e2e.ts (1)
890-1006: Strengthen “tool calls with result” assertions: forbid residual tool_calls and ensure logs reflect a final answer.Prevent regressions where providers emit another tool call after a tool result.
const message = json.choices[0].message; expect(message).toHaveProperty("role", "assistant"); - // Should have proper content (not empty) as a response to the tool call - expect(message).toHaveProperty("content"); - // verify either content is string or tool_calls is present - expect(message.content || message.tool_calls).toBeTruthy(); + // Should have proper content (not empty) as a response to the tool call + expect(message).toHaveProperty("content"); + expect(typeof message.content === "string" && message.content.length > 0).toBe(true); + + // Final turn should not contain any further tool calls + expect(!message.tool_calls || message.tool_calls.length === 0).toBe(true); - // Should have finish reason as stop (not tool_calls since this is a response) - // TODO THIS IS FAILING ON SOME MODELS - // expect(json.choices[0]).toHaveProperty("finish_reason", "stop"); + // Finish reason should not indicate more tool calls + expect(json.choices[0].finish_reason).not.toBe("tool_calls"); // Validate logs const log = await validateLogs(); expect(log.streamed).toBe(false); + expect(log.toolResults).toBeNull();apps/gateway/src/chat/chat.ts (2)
453-480: Don’t mutate upstream JSON for ZAI; normalize locally instead.In-place edits risk log/cache divergence. Keep finishReason = "stop" but do not modify json; null out toolResults locally.
- if (hasNewToolCalls) { - finishReason = "stop"; - // Also update JSON to match - if (json.choices?.[0]) { - json.choices[0].finish_reason = "stop"; - delete json.choices[0].message.tool_calls; - } - } + if (hasNewToolCalls) { + finishReason = "stop"; + toolResults = null; // ignore unexpected tool_calls for these models + }
713-715: Vertex streaming: avoid Date.now() in tool_call IDs; use deterministic IDs.Use function name + candidate index + part index to keep IDs stable per chunk.
- .map((part: any, index: number) => ({ - id: part.functionCall.name + "_" + Date.now() + "_" + index, + .map((part: any, index: number) => ({ + id: `${part.functionCall.name}_${data.candidates?.[0]?.index ?? 0}_${index}`, type: "function",
🧹 Nitpick comments (3)
packages/models/src/provider-api.ts (1)
752-760: Default the new parameter.Minor: give hasExistingToolCalls a default to avoid passing undefined.
-export function getProviderEndpoint( +export function getProviderEndpoint( provider: ProviderId, baseUrl?: string, model?: string, token?: string, stream?: boolean, supportsReasoning?: boolean, - hasExistingToolCalls?: boolean, + hasExistingToolCalls: boolean = false, ): string {apps/gateway/src/api.e2e.ts (1)
675-676: Minor: the added directive is fine but noisy.If it’s not asserting behavior, consider removing to reduce LLM variance.
apps/gateway/src/chat/chat.ts (1)
3557-3585: Streaming tool_call merge: also backfill missing function.name on deltas.When partial_json arrives before name, copy from existingCall.function.name.
- if (existingCall) { + if (existingCall) { // Accumulate function arguments if (newCall.function?.arguments) { existingCall.function.arguments = (existingCall.function.arguments || "") + newCall.function.arguments; } + if (!newCall.function?.name && existingCall.function?.name) { + // keep name consistent across deltas + // no-op; existingCall already has name + } } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/gateway/src/api.e2e.ts(2 hunks)apps/gateway/src/chat/chat.ts(11 hunks)packages/models/src/provider-api.ts(6 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use localStorage instead of cookies for client-side data persistence
Files:
apps/gateway/src/api.e2e.tspackages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
**/*.{js,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,ts}: Use drizzle with the latest object syntax for database operations
For read queries, always usedb().query.<table>.findMany()ordb().query.<table>.findFirst()
Files:
apps/gateway/src/api.e2e.tspackages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Never use
as anyor: anyin TypeScript files.
Files:
apps/gateway/src/api.e2e.tspackages/models/src/provider-api.tsapps/gateway/src/chat/chat.ts
**/*.e2e.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Name end-to-end test files with the .e2e.ts suffix
Files:
apps/gateway/src/api.e2e.ts
apps/{api,gateway}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/{api,gateway}/**/*.{ts,tsx}: Use Zod schemas for validation in backend services
Use Hono as the web framework for backend routes and middleware.findMany() or db().query.
apps/{api,gateway}/**/*.{ts,tsx}: Use Drizzle with the latest object syntax for database operations
For read queries, use db().query..findFirst() Files:
apps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.tsapps/gateway/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
In apps/gateway (Hono), always use Hono + Zod + OpenAPI for validation and typesafety
Files:
apps/gateway/src/api.e2e.tsapps/gateway/src/chat/chat.ts🧬 Code graph analysis (3)
apps/gateway/src/api.e2e.ts (3)
apps/gateway/src/index.ts (1)
app(43-43)apps/api/src/index.ts (1)
app(29-29)packages/db/src/schema.ts (2)
message(382-398)log(243-305)packages/models/src/provider-api.ts (1)
packages/db/src/types.ts (2)
tools(44-44)tool(18-21)apps/gateway/src/chat/chat.ts (2)
packages/models/src/models.ts (1)
Provider(18-18)packages/db/src/types.ts (1)
toolResults(45-45)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e / run
🔇 Additional comments (10)
packages/models/src/provider-api.ts (4)
443-448: Good call: gate Responses API when prior tool calls exist.This avoids dropping tool context; detection via role === "tool" or tool_calls is correct.
457-496: OpenAI Responses path only when no existing tool calls — LGTM.The guard on supportsReasoning && supportsResponsesApi && !hasExistingToolCalls is sound.
884-899: Endpoint routing condition — LGTM.Using /v1/responses only when supported and there are no existing tool calls is correct.
1008-1009: Validation path param — LGTM.Passing false for hasExistingToolCalls here is appropriate.
apps/gateway/src/chat/chat.ts (6)
343-345: Deterministic IDs for Google (non-streaming) — LGTM.Stable IDs improve merging and logging.
423-431: Finish reason for OpenAI Responses based on actual tool calls — LGTM.Prevents false “stop” when tool_calls exist.
690-704: Anthropic partial tool args anchoring — LGTM._index-based anchoring via _contentBlockIndex is a solid approach; cleanup before storing is good.
2569-2574: Good: route away from Responses when prior tool context exists.Aligns gateway routing with provider behavior.
2582-2590: Plumbing hasExistingToolCalls into getProviderEndpoint — LGTM.Ensures consistent endpoint selection.
4150-4151: Passing messages into parseProviderResponse — LGTM.Enables provider-specific finish_reason fixes that depend on prior turns.
| function parseProviderResponse( | ||
| usedProvider: Provider, | ||
| json: any, | ||
| messages: any[] = [], | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type strictly: avoid any per repo guideline.
Use BaseMessage[] and unknown for json.
-function parseProviderResponse(
- usedProvider: Provider,
- json: any,
- messages: any[] = [],
-) {
+function parseProviderResponse(
+ usedProvider: Provider,
+ json: unknown,
+ messages: BaseMessage[] = [],
+) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseProviderResponse( | |
| usedProvider: Provider, | |
| json: any, | |
| messages: any[] = [], | |
| ) { | |
| function parseProviderResponse( | |
| usedProvider: Provider, | |
| json: unknown, | |
| messages: BaseMessage[] = [], | |
| ) { |
🤖 Prompt for AI Agents
In apps/gateway/src/chat/chat.ts around lines 230 to 234, the function signature
uses any for json and messages which violates the repo typing guideline; change
the signature to use json: unknown and messages: BaseMessage[] (import or
reference BaseMessage type) and update the function body to safely
narrow/validate json (type guards or runtime checks) before accessing its
properties, and treat messages as BaseMessage[] so any downstream operations use
the strict type; ensure any casts are localized and justified with checks.
1 participant
Enhanced e2e tests to include tool call validations, usage reporting, and detailed assertions for result completeness and consistency. Removed unused model filtering logic.
Summary by CodeRabbit
Bug Fixes
New Features
Tests