Conversation
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Review: PR #666 — Preserve reasoning content on tool call replays (head 9220234)
CI not reported. 2 files, +128/-0. Closes #522.
What this fixes
Issue #522: When convertMessages() strips Anthropic thinking blocks from assistant messages with tool_calls, reasoning-capable OpenAI-compatible providers (like Kimi) that require reasoning_content in follow-up requests get a 400 error. The thinking content was being discarded entirely instead of preserved as reasoning_content.
Code changes
openaiShim.ts — Two focused changes:
-
Single assistant message with tool_use + thinking (line ~345): Extracts thinking blocks → joins with
\n\n→ sets asreasoning_contenton the OpenAI assistant message. Only non-empty thinking text is preserved. ✅ -
Coalescing consecutive assistant messages (line ~455): When merging two adjacent assistant messages, concatenates
reasoning_contentwith\n\nseparator. This preserves reasoning across coalesced turns. ✅
openaiShim.test.ts — Two new test cases:
-
Single message test: Verifies that an assistant message with
thinking+tool_useblocks getsreasoning_contentset correctly on the OpenAI message. ✅ -
Coalescing test: Verifies that two consecutive assistant messages (one with thinking+tool_use, one with text) merge into a single message that preserves both
reasoning_contentandtool_calls. ✅
Assessment
- Correctly targets issue #522 root cause
- Preserves Anthropic
thinkingas OpenAI-compatiblereasoning_content— proper cross-format translation - Coalescing logic handles the join correctly
- Test coverage is good — both the single and coalesced paths are tested
- No regression risk for non-reasoning providers (they ignore
reasoning_content)
🟡 Non-blocking
1. Empty thinking blocks are filtered but not null thinking
The filter typeof b.thinking === 'string' && b.thinking !== '' correctly skips empty strings, but what about { type: 'thinking', thinking: null }? In the Anthropic API, thinking is always a string if present, so this is likely fine, but an explicit && b.thinking != null would be extra safe.
2. CI not reported yet
Please confirm CI passes on the current head.
Verdict: Approve-ready ✅
Clean, focused fix for #522. Correct cross-format translation of reasoning content with good test coverage.
- 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
jatmn
left a comment
There was a problem hiding this comment.
Please resolve conflict, happy to re-review after
…e in generic OpenAI provider test (Twigpine#1228) Fix the deserializeMessagesWithInterruptDetection test so it correctly expects stripped thinking blocks for generic OpenAI- compatible providers, and passes regardless of host environment variables. Problem ━━━━━━━ Reviewer jatmn flagged a P1 blocker: the "third-party provider" test branch mocks getAPIProvider() as 'openai' but does not clear the host environment. When the developer's shell has: OPENAI_BASE_URL=https://api.deepseek.com OPENAI_MODEL=deepseek-chat …the test silently passes locally because inferRemoteModel- OpenAIShimConfig('deepseek-chat') activates preserveReasoningContent via the includes('deepseek') branch. On CI or clean machines, the assertion expecting preserved thinking blocks fails. This was the only failing test across the 3 modified files, making the PR's stated "All 108 tests pass" claim incorrect for the current code. Fix ━━━━ 1. Delete process.env.OPENAI_BASE_URL and OPENAI_MODEL before the generic OpenAI test block, so shouldPreserveThinkingBlocks- ForProviderReplay() correctly returns false regardless of what the developer has in their shell. 2. Update the assertion to expect stripped thinking blocks ([{ type: 'text', text: 'Here is my answer.' }]) — the correct behavior for a provider without preserveReasoningContent. 3. Add a MiMo branch below (OPENAI_MODEL=mimo-v2.5-pro) that asserts thinking blocks ARE preserved when preserveReasoningContent is active, covering both sides of the conditional. 4. Update inline comments to clarify why env vars must be cleared and why thinking blocks are stripped vs preserved across each provider branch. Verification ━━━━━━━━━━━━ All 3 affected test files pass with 0 failures: bun test src/utils/conversationRecovery.hooks.test.ts → 2/2 pass bun test src/utils/conversationRecovery.test.ts → 5/5 pass bun test src/services/api/openaiShim.test.ts → 107/107 pass Related reasoning_content PRs (same domain, independent fixes): Twigpine#666 — preserve reasoning content on tool call replays (draft) Twigpine#783 — consolidate 3P provider compatibility fixes (merged) Twigpine#1155 — restore reasoning support for proxy/localhost (open) Twigpine#1201 — tighten reasoning_content heuristic (merged)
|
Closing as abandoned |
3 participants
Problem
Reasoning-capable OpenAI-compatible providers such as Kimi require assistant tool-call messages in the replayed history to include
reasoning_content. OpenClaude currently strips Anthropicthinkingblocks when converting assistant messages for the OpenAI shim, so follow-up requests can containtool_callswithout the required reasoning payload and fail with a 400 error.Root Cause
convertMessages()removesthinkingblocks from assistant history to avoid leaking Anthropic-only content types into OpenAI-compatible providers, but it does not preserve that reasoning anywhere when the same assistant turn also containstool_useblocks.Fix
Preserve assistant thinking text as
reasoning_contenton replayed OpenAI-compatible assistant messages, including after assistant-message coalescing, while continuing to omit Anthropic-only block types fromcontent.Validation
~/.bun/bin/bun install~/.bun/bin/bun test ./src/services/api/openaiShim.test.tsCloses #522