Fix Kimi tool-call payload normalization for reasoning_content - #1467
Conversation
Summary of ChangesHello @dusty-du, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves critical issues within the Kimi executor related to chat completion requests. It introduces robust normalization logic to ensure that assistant messages containing tool calls always have a valid Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces normalization logic for Kimi tool-call payloads to address issues with reasoning_content and tool_call_id linkage. The changes are well-structured and include comprehensive unit tests covering various scenarios. The core logic in normalizeKimiToolMessageLinks correctly handles the described normalization cases.
My review includes a couple of suggestions for performance improvements in the new normalization function. Specifically, I've pointed out potential optimizations regarding repeated JSON modifications within a loop and a more efficient slice removal technique. These are not critical but could improve performance for payloads with many messages.
| removePending := func(id string) { | ||
| for idx := range pending { | ||
| if pending[idx] != id { | ||
| continue | ||
| } | ||
| pending = append(pending[:idx], pending[idx+1:]...) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of removePending uses append to remove an element from the slice, which is an O(N) operation as it may require shifting all subsequent elements. Since the order of elements in the pending slice does not appear to be significant for the logic, you can achieve the same result with an O(1) operation by swapping the element to be removed with the last element and then shrinking the slice. This is more efficient, especially if the pending slice could grow.
_FUNC_START_removePending := func(id string) {
for i, pID := range pending {
if pID == id {
// Swap with the last element and shrink the slice for O(1) removal.
pending[i] = pending[len(pending)-1]
pending = pending[:len(pending)-1]
return
}
}
}_FUNC_END_| next, err := sjson.SetBytes(out, path, reasoningText) | ||
| if err != nil { | ||
| return body, fmt.Errorf("kimi executor: failed to set assistant reasoning_content: %w", err) | ||
| } | ||
| out = next |
There was a problem hiding this comment.
Each call to sjson.SetBytes inside this loop can cause a full copy and reallocation of the JSON byte slice out. If a payload has many messages that need patching, this could become a performance bottleneck. A more performant approach would be to unmarshal the messages array into a Go slice of maps (e.g., []map[string]any), modify this slice in memory, and then marshal it back to JSON. Finally, you can replace the original messages array in the payload with a single sjson.SetBytes call outside the loop.
|
I assume this is the PR you'd like to have tested, I tried it with Zed again and it seems like the issue presists: My zed config: EDIT: |
|
Status update: I can confirm the issue only presists for Zed (zed.dev) and in OpenCode with Oh My Opencode it works now! |
Please edit your Zed config for this model, chat_completions should be set to true, and prompt_cache_key should be set to false. Thanks for testing, let me know if any issues arise. |
|
That did it! Thanks for the comment!
|
|
PR is complete and ready for your review @luispater |
…-reasoning-content Fix Kimi tool-call payload normalization for reasoning_content
…-reasoning-content Fix Kimi tool-call payload normalization for reasoning_content
3 participants
Follow-up to: https://github.com/router-for-me/CLIProxyAPIPlus/pull/182#issuecomment-3861001374
Problem
Kimi rejects chat completion requests when an assistant message that contains tool_calls is missing a non-empty reasoning_content field. We also saw tool message linkage issues when only call_id is present.
Changes