fix: OpenAI adapter tool calling compatibility#149
fix: OpenAI adapter tool calling compatibility#149claude-code-best merged 1 commit intoclaude-code-best:mainfrom
Conversation
Two fixes for OpenAI-compatible provider compatibility: 1. Sanitize JSON Schema `const` → `enum` in tool parameters. Many OpenAI-compatible endpoints (Ollama, DeepSeek, vLLM, etc.) do not support the `const` keyword in JSON Schema. Recursively convert `const: value` to `enum: [value]` which is semantically equivalent. 2. Force stop_reason to `tool_use` when tool_calls are present. Some backends incorrectly return finish_reason "stop" even when the response contains tool_calls. Without this fix, the query loop treats the response as a normal end_turn and never executes the requested tools.
📝 WalkthroughWalkthroughThe changes add JSON schema sanitization for Anthropic-to-OpenAI tool conversion, automatically replacing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/services/api/openai/__tests__/streamAdapter.test.ts (1)
168-188: Add a non-stopregression case for tool calls.This new test is great for the
"stop"path; please add one forfinish_reason: "length"+tool_callsto ensure only the"stop"case is overridden.🧪 Suggested additional test
+ test('keeps max_tokens stop_reason when tool_calls present and finish_reason is length', async () => { + const events = await collectEvents([ + makeChunk({ + choices: [{ + index: 0, + delta: { + tool_calls: [{ index: 0, id: 'call_1', function: { name: 'bash', arguments: '{"cmd":"ls"}' } }], + }, + finish_reason: null, + }], + }), + makeChunk({ + choices: [{ index: 0, delta: {}, finish_reason: 'length' }], + }), + ]) + + const msgDelta = events.find(e => e.type === 'message_delta') as any + expect(msgDelta.delta.stop_reason).toBe('max_tokens') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/openai/__tests__/streamAdapter.test.ts` around lines 168 - 188, Add a sibling test that mirrors the existing "forces tool_use stop_reason when tool_calls present but finish_reason is stop" case but uses finish_reason: 'length' instead of 'stop' to assert we do NOT override the stop_reason; use collectEvents and makeChunk the same way, then find the message_delta (msgDelta) and expect msgDelta.delta.stop_reason to be 'length' (or unchanged) to ensure only the 'stop' finish_reason path is forced to 'tool_use'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/api/openai/streamAdapter.ts`:
- Around line 264-265: The current logic sets stopReason = hasToolCalls ?
'tool_use' : mapFinishReason(choice.finish_reason) regardless of the original
finish reason; change this so the 'tool_use' override is applied only when
toolBlocks.size > 0 AND choice.finish_reason === 'stop' (otherwise preserve
mapped reasons like 'length' or 'content_filter'). In other words, update the
computation of stopReason (symbols: toolBlocks, hasToolCalls, stopReason,
mapFinishReason, choice.finish_reason) so it checks choice.finish_reason ===
'stop' before substituting 'tool_use', and fall back to
mapFinishReason(choice.finish_reason) for all other finish reasons.
---
Nitpick comments:
In `@src/services/api/openai/__tests__/streamAdapter.test.ts`:
- Around line 168-188: Add a sibling test that mirrors the existing "forces
tool_use stop_reason when tool_calls present but finish_reason is stop" case but
uses finish_reason: 'length' instead of 'stop' to assert we do NOT override the
stop_reason; use collectEvents and makeChunk the same way, then find the
message_delta (msgDelta) and expect msgDelta.delta.stop_reason to be 'length'
(or unchanged) to ensure only the 'stop' finish_reason path is forced to
'tool_use'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15edcb8c-6f1d-4756-bb44-266c66c0ec41
📒 Files selected for processing (4)
src/services/api/openai/__tests__/convertTools.test.tssrc/services/api/openai/__tests__/streamAdapter.test.tssrc/services/api/openai/convertTools.tssrc/services/api/openai/streamAdapter.ts
| const hasToolCalls = toolBlocks.size > 0 | ||
| const stopReason = hasToolCalls ? 'tool_use' : mapFinishReason(choice.finish_reason) |
There was a problem hiding this comment.
Scope the tool_use override to finish_reason === 'stop' only.
Current logic overrides length/content_filter too when tool blocks exist, which can misreport truncated/filtered completions as executable tool calls.
💡 Proposed fix
- const hasToolCalls = toolBlocks.size > 0
- const stopReason = hasToolCalls ? 'tool_use' : mapFinishReason(choice.finish_reason)
+ const hasToolCalls = toolBlocks.size > 0
+ const mappedStopReason = mapFinishReason(choice.finish_reason)
+ const stopReason =
+ hasToolCalls && choice.finish_reason === 'stop'
+ ? 'tool_use'
+ : mappedStopReason📝 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.
| const hasToolCalls = toolBlocks.size > 0 | |
| const stopReason = hasToolCalls ? 'tool_use' : mapFinishReason(choice.finish_reason) | |
| const hasToolCalls = toolBlocks.size > 0 | |
| const mappedStopReason = mapFinishReason(choice.finish_reason) | |
| const stopReason = | |
| hasToolCalls && choice.finish_reason === 'stop' | |
| ? 'tool_use' | |
| : mappedStopReason |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api/openai/streamAdapter.ts` around lines 264 - 265, The current
logic sets stopReason = hasToolCalls ? 'tool_use' :
mapFinishReason(choice.finish_reason) regardless of the original finish reason;
change this so the 'tool_use' override is applied only when toolBlocks.size > 0
AND choice.finish_reason === 'stop' (otherwise preserve mapped reasons like
'length' or 'content_filter'). In other words, update the computation of
stopReason (symbols: toolBlocks, hasToolCalls, stopReason, mapFinishReason,
choice.finish_reason) so it checks choice.finish_reason === 'stop' before
substituting 'tool_use', and fall back to mapFinishReason(choice.finish_reason)
for all other finish reasons.
Summary
Fixes two OpenAI-compatible provider compatibility issues identified by comparing with the CCB proxy implementation:
constsanitization: Recursively convertconst: value→enum: [value]in tool parameter schemas. Many OpenAI-compatible endpoints (Ollama, DeepSeek, vLLM, etc.) rejectconstin JSON Schema, causing tool definitions to fail silently.tool_usestop_reason when tool_calls present: Some backends returnfinish_reason: "stop"even when the response contains tool_calls. Without this fix, the query loop treats it asend_turnand never executes the tools — the model's tool call is silently dropped.Files changed
src/services/api/openai/convertTools.ts— addsanitizeJsonSchema()recursive processorsrc/services/api/openai/streamAdapter.ts— override stop_reason when toolBlocks.size > 0Test plan
constin top-level, nested, andanyOf/oneOf/allOfschemas all converted correctlyfinish_reason: "stop"with tool_calls now correctly yieldsstop_reason: "tool_use"finish_reason: "stop"without tools still yieldsend_turnSummary by CodeRabbit
Bug Fixes
Tests