Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876
Fix reasoning_content error in tool calls for kimi-k2.5 and other thi…#876jdhxyy wants to merge 1 commit intosipeed:mainfrom
Conversation
nikolasdehor
left a comment
There was a problem hiding this comment.
Good targeted fix for #588. The field naming (ReasoningContent) is consistent with the existing Message struct. Two observations: (1) Incomplete fix? This PR preserves reasoning_content in the outbound serialization (stripSystemParts), but I don't see where response.ReasoningContent is copied into the assistant Message during tool loop execution. Without that, the ReasoningContent field will be empty for assistant messages created in toolloop.go. PR #889 addresses this in toolloop.go:109 — you may need to add that piece here too, or coordinate with that PR. (2) Unrelated .gitignore change: The __debug_bin patterns are fine but would be cleaner as a separate commit. (3) Test: Consider adding a test for stripSystemParts that verifies ReasoningContent is preserved. The core approach is correct — please verify the tool loop path is also covered.
🔍 Deep Code ReviewThanks for the fix! The approach is correct, but I have some suggestions. 🔬 Problem AnalysisRoot cause verified:
This caused:
✅ Fix AssessmentThe fix is correct - adding
|
| Type | Count |
|---|---|
| 🚨 Critical Issues | 0 |
| 1 (missing request serialization test) | |
| 💡 Suggestions | 2 |
Verdict: The fix is correct and can be merged after adding the test. Please add the test case above to ensure reasoning_content is properly preserved in multi-turn conversations.
The openaiMessage struct and stripSystemParts() were not carrying over the ReasoningContent field when serializing conversation history for API requests. This caused thinking models (e.g. kimi-k2.5) to receive incomplete assistant messages on subsequent turns, resulting in 400 errors from the Moonshot API. Add the ReasoningContent field to openaiMessage and copy it in stripSystemParts(). Also add a test to verify reasoning_content is preserved when sending conversation history. Fixes sipeed#588 Related: sipeed#876 Co-Authored-By: Claude Opus 4.6 <[email protected]>
|
same as #946 |
The openaiMessage struct and stripSystemParts() were not carrying over the ReasoningContent field when serializing conversation history for API requests. This caused thinking models (e.g. kimi-k2.5) to receive incomplete assistant messages on subsequent turns, resulting in 400 errors from the Moonshot API. Add the ReasoningContent field to openaiMessage and copy it in stripSystemParts(). Also add a test to verify reasoning_content is preserved when sending conversation history. Fixes sipeed#588 Related: sipeed#876 Co-Authored-By: Claude Opus 4.6 <[email protected]>
The openaiMessage struct and stripSystemParts() were not carrying over the ReasoningContent field when serializing conversation history for API requests. This caused thinking models (e.g. kimi-k2.5) to receive incomplete assistant messages on subsequent turns, resulting in 400 errors from the Moonshot API. Add the ReasoningContent field to openaiMessage and copy it in stripSystemParts(). Also add a test to verify reasoning_content is preserved when sending conversation history. Fixes sipeed#588 Related: sipeed#876 Co-Authored-By: Claude Opus 4.6 <[email protected]>
The openaiMessage struct and stripSystemParts() were not carrying over the ReasoningContent field when serializing conversation history for API requests. This caused thinking models (e.g. kimi-k2.5) to receive incomplete assistant messages on subsequent turns, resulting in 400 errors from the Moonshot API. Add the ReasoningContent field to openaiMessage and copy it in stripSystemParts(). Also add a test to verify reasoning_content is preserved when sending conversation history. Fixes sipeed#588 Related: sipeed#876 Co-Authored-By: Claude Opus 4.6 <[email protected]>
…nking models
📝 Description
🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes #588
📚 Technical Context (Skip for Docs)
🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist