Conversation
Three significant fixes in the OpenAI shim: 1. Forward reasoning_content from thinking models (DeepSeek R1, o3, etc.) as Anthropic thinking blocks. Streaming: emits content_block_start with type 'thinking', thinking_delta events, and proper close before text content begins. Non-streaming: prepends a thinking block. Supports both reasoning_content and reasoning field aliases. 2. Use max_tokens instead of max_completion_tokens for local providers (Ollama, LM Studio). Older Ollama versions silently ignore max_completion_tokens, causing the user's token budget to be lost. Mirrors the existing GitHub Models fallback pattern. 3. Preserve images in tool_result blocks via buildToolResultContent(). Previously, image blocks were silently dropped (.text was undefined). Now emits OpenAI image_url content parts with base64 data URIs or direct URLs, which the OpenAI Chat Completions API fully supports on tool role messages.
Vasanthdev2004
left a comment
There was a problem hiding this comment.
This is close, but I don't think it's safe to merge yet.
One blocker stood out in src/services/api/openaiShim.ts:560-590:
If a provider emits reasoning_content / reasoning and then goes straight into tool_calls without an intermediate text delta, hasEmittedThinkingStart is still true here, but we still start a tool_use block. The thinking block only gets stopped later at finish time.
That means the stream can have both blocks open at once, and the stop ordering becomes:
content_block_start(thinking)content_block_start(tool_use)- ...
content_block_stop(thinking)content_block_stop(tool_use)
The existing Anthropic-style consumer logic is built around closing a block before the next one starts, so this can produce out-of-order content on reasoning models that think and then call tools.
I think this needs the same close-before-next-block handling for tool calls that the text path already has.
After that is fixed, I'd also want focused regression coverage for:
- reasoning -> tool_calls streaming
- tool_result image forwarding
- local-provider
max_tokensfallback
Address review feedback: if a reasoning model transitions from reasoning_content directly to tool_calls without intermediate text, the thinking block was left open while the tool_use block started. Now closes the thinking block before opening any tool_use block, matching the same pattern already used for text→tool transitions.
|
Good catch @Vasanthdev2004 — fixed in the latest push. The thinking block is now closed before any tool_use block starts, matching the same close-before-next pattern that already existed for text→tool transitions. The stream sequence for reasoning→tool_calls is now: Regarding regression tests — I'll add targeted tests for the three paths you mentioned (reasoning→tool_calls, tool_result images, local max_tokens fallback) in a follow-up commit. |
Three targeted tests requested by reviewer: 1. reasoning → tool_calls streaming: verifies thinking block is opened, receives thinking_delta events with reasoning text, is closed BEFORE the tool_use block starts, and the indices are in correct order. 2. tool_result image forwarding: verifies base64 image blocks in tool results are converted to image_url content parts (not dropped as empty strings), with correct data URI format. 3. local-provider max_tokens fallback: verifies that Ollama/LMStudio URLs receive max_tokens (not max_completion_tokens) and that stream_options is not sent for local providers.
|
Added the three regression tests @Vasanthdev2004 requested:
All 7 tests pass. |
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Rechecked the follow-up changes. The thinking block now closes before tool_use starts, the three regression tests I asked for are in place, and the updated shim test plus build pass locally from my side.
|
please fix conflicts |
|
@auriti please fix conflicts here. lets see if we can merge this |
|
Fresh pass on the current branch from my side:
One note on scope versus current
So if this is still wanted, the next useful step is simply to refresh the branch onto current |
|
@auriti kindly address conflicts please |
|
hello @auriti kindly fix the conflicts when you have time. |
- Strip store field from request body for local providers (Ollama, vLLM) that reject unknown JSON fields with 400 errors - Add Gemini 3.x model context windows and output token limits (gemini-3-flash-preview, gemini-3.1-pro-preview, google/ OpenRouter variants) - Preserve reasoning_content on assistant tool-call message replays for providers that require it (Kimi k2.5, DeepSeek reasoner) - Use conservative max_output_tokens fallback (4096/16384) for unknown 3P models to prevent vLLM/Ollama 400 errors from exceeding max_model_len Consolidates fixes from: #258, #268, #237, #643, #666, #677 Co-authored-by: auriti <auriti@users.noreply.github.com> Co-authored-by: Gustavo-Falci <Gustavo-Falci@users.noreply.github.com> Co-authored-by: lttlin <lttlin@users.noreply.github.com> Co-authored-by: Durannd <Durannd@users.noreply.github.com>
jatmn
left a comment
There was a problem hiding this comment.
Thanks for following up on the earlier review. The requested thinking-block close-before-tool behavior looks addressed now, and the three requested regression tests are also in place. I found one remaining issue below.
Findings
- [P1] Do not send image_url parts in tool-role messages
src/services/api/openaiShim.ts:181
buildToolResultContent()now returns multipart content with{ type: 'image_url' }for tool results, andconvertMessages()places that array on arole: 'tool'Chat Completions message. The OpenAI Chat Completions schema allows array content for tool messages, but only text parts are supported for that role; image parts are accepted on user messages, not tool messages. So a screenshot/browser tool result that includes an image will be sent back as a tool message and the next OpenAI request can fail validation instead of preserving the prior text-only behavior. Please either keep tool-role content text-only for Chat Completions, move image data through a supported user-message representation, or gate this to providers/APIs that explicitly support image parts on tool messages.
|
hello @auriti kindly rebase to main and fix conflicts. |
|
closing as abandoned |
Summary
Three significant fixes in the OpenAI shim addressing long-standing gaps in provider compatibility, reasoning model support, and tool result fidelity.
Changes
1. Forward
reasoning_contentfrom thinking models as Anthropic thinking blocksImpact: Users of DeepSeek R1, o3, and other reasoning models can now see chain-of-thought output.
Previously,
delta.reasoning_contentwas silently ignored — reasoning models appeared to "just think silently" with no visible output until the final answer.Streaming path:
delta.reasoning_content(ordelta.reasoningalias)content_block_startwithtype: 'thinking'thinking_deltaevents with the reasoning textdelta.contentbegins (model switches from reasoning to answering)Non-streaming path:
message.reasoning_content(orreasoningalias){ type: 'thinking', thinking: text, signature: '' }block before text content2. Use
max_tokensfor local providers (Ollama/LM Studio compatibility)Impact: Ollama < 0.5 users get their token budget respected.
Older Ollama versions silently ignore
max_completion_tokensand use their internal default. Now mirrors the existing GitHub Models fallback: for local providers (detected viaisLocalProviderUrl),max_completion_tokensis swapped tomax_tokens.3. Preserve images in
tool_resultblocksImpact: Screenshot tools, browser tools, and any tool returning images now work with OpenAI providers.
Previously, image blocks in tool results were mapped to empty strings via
c.text ?? ''. NewbuildToolResultContent()helper emits properimage_urlcontent parts:data:URItoolrole messages)Relates to #30, #113, #114
Test plan
bun test src/services/api/openaiShim.test.ts— passbun test src/services/api/codexShim.test.ts— pass