fix(openai-shim): preserve tool result images and local token caps#659
Conversation
Keep tool-result images as real image_url parts for OpenAI-compatible requests and use max_tokens for local providers like Ollama and LM Studio.
|
This is the narrowed follow-up to the old #237 branch, rebuilt from current
I intentionally left out the reasoning/thinking changes that current
|
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Review: Preserve tool result images and local token caps
Reviewed on head e5ae42a. CI green ✅. 2 files, +242/-17.
This is a clean, well-scoped follow-up to #237 — only the two residual gaps that current main hasn't absorbed yet. The diff is focused and the tests are good.
✅ max_tokens fallback for local providers
The isLocal branch correctly extends the existing isGithub || isMistral gate to also include isLocalProviderUrl. This is the right behavior — local providers like Ollama and LM Studio don't support max_completion_tokens (it's an OpenAI-specific field), and they do support max_tokens. The isLocalProviderUrl function is already imported and used in the same file for stream_options, so this is consistent.
Test coverage: one test for local provider (localhost:11434) asserting max_tokens: 64 and max_completion_tokens: undefined, one for remote provider (api.openai.com) asserting the reverse. Good.
✅ Tool result image forwarding
The old code flattened tool result images into [image:image/png] placeholder strings — which means the image data was completely lost and the provider received a useless text marker. The new code converts them to proper image_url content parts with data:image/png;base64,... URIs, matching the same pattern already used in convertContentBlocks for user/assistant messages.
The single-part optimization is nice: when there's only one text block and no error, it returns a plain string (backward-compatible). When there are images or multiple parts, it returns the array. The isError handling is also cleaner — prepending Error: to the first text part instead of wrapping the entire serialized content.
Test coverage: three tests covering (1) single image tool result → image_url data URI, (2) mixed text+image tool result → multipart content, (3) the old placeholder test updated to expect the new format. All assertions look correct.
✅ stream_options removal for local providers
The local provider test also asserts stream_options: undefined, which is correct — the existing code at line 1213 already skips stream_options for local providers, and the test is non-streaming anyway. No behavioral change here.
🔶 One observation (non-blocking)
The OpenAI Chat Completions API formally types tool message content as string | ChatCompletionContentPartText[] — image_url parts are not documented in the tool message schema. In practice, many providers (Ollama, LM Studio, etc.) handle them fine because they process all content parts generically, and the old behavior (losing the image entirely) was strictly worse. So this is a clear improvement even if some strict API validators might flag it.
If you want to be maximally safe, you could add a provider-aware gate: only emit image_url parts for providers known to support them in tool messages (local providers, GitHub, etc.), and fall back to the placeholder for others. But that's an optimization — the current approach is a strict upgrade over the old [image:image/png] lossy behavior.
Verdict: Approve-ready ✅
Clean, focused, well-tested. Two real fixes: (1) images no longer silently dropped from tool results, (2) local providers get max_tokens instead of the unsupported max_completion_tokens. No logic regressions. The image forwarding is strictly better than the placeholder approach.
…itlawb#659) Keep tool-result images as real image_url parts for OpenAI-compatible requests and use max_tokens for local providers like Ollama and LM Studio.
Summary
tool_resultimages as realimage_urlcontent parts for OpenAI-compatible requestsmax_tokensinstead ofmax_completion_tokensfor local providers like Ollama and LM StudioWhy
The earlier PR #237 is now mostly absorbed by
main, but two useful gaps remained:tool_resultimages were still degraded instead of being forwarded as real multipart image contentmax_tokensfallbackThis PR keeps only that residual value in a current, reviewable branch.
Changes
src/services/api/openaiShim.tstool_resultimages asimage_urlparts and applymax_tokensfallback to local providerssrc/services/api/openaiShim.test.tsTest plan
bun test src/services/api/openaiShim.test.ts(validated in the main working tree before isolating this branch)image_urlmax_tokensand omitmax_completion_tokensNotes
main.origin/mainto avoid carrying the old conflict-heavy history.