Repository navigation
fix(sse): refine kimi thinking handling and add unit tests - #3191
Conversation
- Remove debug file write (writeHandlerBodyToLocalFile) from transformRequest - Import ExecuteInput type for future extensibility - Add tsconfig include path for .build/next/dev/dev/types - Add unit test for assistant tool call reasoning_content injection
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| type ProviderCredentials, | ||
| } from "./base.ts"; | ||
|
|
||
| import { ExecuteInput, type ProviderCredentials } from "./base.ts"; |
There was a problem hiding this comment.
WARNING: Extra space in import statement between { and ExecuteInput - should be { ExecuteInput for consistent formatting
| if (!hasActiveKimiThinking(kimiBody)) return kimiBody; | ||
|
|
||
| let modified = false; | ||
| const sourceMessages = Array.isArray(kimiBody.messages) ? kimiBody.messages : record.messages; |
There was a problem hiding this comment.
SUGGESTION: The fallback record.messages on line 102 is unnecessary. By line 93, we've already verified record.messages is an array, and disableKimiPreservedThinking() only modifies the thinking field without affecting messages. Can simplify to const sourceMessages = kimiBody.messages;
| type ProviderCredentials, | ||
| } from "./base.ts"; | ||
|
|
||
| import { ExecuteInput, type ProviderCredentials } from "./base.ts"; |
There was a problem hiding this comment.
WARNING: Extra space in import statement between { and ExecuteInput - should be { ExecuteInput for consistent formatting
| if (!hasActiveKimiThinking(kimiBody)) return kimiBody; | ||
|
|
||
| let modified = false; | ||
| const sourceMessages = Array.isArray(kimiBody.messages) ? kimiBody.messages : record.messages; |
There was a problem hiding this comment.
SUGGESTION: The fallback record.messages on line 102 is redundant. disableKimiPreservedThinking returns {...body, thinking: {...}} which preserves all existing fields including messages. Since line 93 already verified record.messages is an array, this fallback cannot be reached.
| const cleanedBody = super.transformRequest(model, body, stream, credentials); | ||
| return applyKimiRequestDefaults(cleanedBody); | ||
| } | ||
|
|
There was a problem hiding this comment.
SUGGESTION: Trailing whitespace on this line (before closing brace) - should be removed for clean code.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (3 files)
|
| type ProviderCredentials, | ||
| } from "./base.ts"; | ||
|
|
||
| import { ExecuteInput, type ProviderCredentials } from "./base.ts"; |
There was a problem hiding this comment.
CRITICAL: Extra space in import statement - should be { ExecuteInput for consistent formatting
| if (!hasActiveKimiThinking(kimiBody)) return kimiBody; | ||
|
|
||
| let modified = false; | ||
| const sourceMessages = Array.isArray(kimiBody.messages) ? kimiBody.messages : record.messages; |
There was a problem hiding this comment.
WARNING: Redundant fallback record.messages - disableKimiPreservedThinking preserves all existing fields including messages, and line 93 already verified the array type
| ) as { messages?: Array<Record<string, unknown>> }; | ||
|
|
||
| assert.ok(Array.isArray(transformed.messages)); | ||
| assert.equal(transformed.messages?.[1]?.reasoning_content, ""); |
There was a problem hiding this comment.
CRITICAL: Test assertion mismatch. The code sets reasoning_content to NON_ANTHROPIC_THINKING_PLACEHOLDER ("(prior reasoning summary unavailable)") when input has reasoning_content: null, but the test expects "". The placeholder is required by Kimi (see claudeHelper.ts:10-11 - empty thinking strings cause 400 errors).
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Files Reviewed (2 files)
Reviewed by laguna-m.1-20260312:free · 1,275,854 tokens |
|
Thanks @bypanghu — the Kimi thinking handling is genuinely tricky (the #2900
Once those are addressed I'll re-review and validate. Leaving open for your update. |
|
ok, I will fix this |
1：There are often errors in calling kimi in claude, because kimi only adapts to the openai protocol, and there are resoning_content in it, but claude does not have this. |
|
Thanks @bypanghu! 🙏 The refined Kimi thinking handling (reasoning_content injection for assistant tool-call turns, with the new thinking/tool_use block helpers) looks solid and the unit tests (4/4) pass. I restored the trailing newline on |
dfcaeba
into
cheaper-inference:release/v3.8.11
| type ProviderCredentials, | ||
| } from "./base.ts"; | ||
|
|
||
| import { ExecuteInput, type ProviderCredentials } from "./base.ts"; |
There was a problem hiding this comment.
WARNING: Extra space in import statement between { and ExecuteInput. Additionally, ExecuteInput is imported but never used in this file — KimiExecutor does not override execute() and transformRequest doesn't use this type.
| if (!hasActiveKimiThinking(kimiBody)) return kimiBody; | ||
|
|
||
| let modified = false; | ||
| const sourceMessages = Array.isArray(kimiBody.messages) ? kimiBody.messages : record.messages; |
There was a problem hiding this comment.
WARNING: Redundant fallback record.messages — disableKimiPreservedThinking returns {...body, thinking: {...}} preserving all existing fields including messages. Since line 93 already verified record.messages is an array, this fallback cannot be reached.
…s + add contributor hall Consolidate the split [Unreleased]/[3.8.11] sections into one, add entries for every merged contributor PR that was missing credit (#3170/#3171/#3172 @pizzav-xyz, #3185/#3195 @zhiru, #3188 @xz-dev, #3189/#3203/#3204/#3241 @wilsonicdev, #3191 @bypanghu, #3206 @juandisay, #3217 @oyi77, #3226 @miracuves, #3187/#3200 maintainer), drop stale v3.8.8 leftovers (#2958/#2959 already shipped) and 3 empty v3.8.10 stub headers.
…nference#3191) Refine Kimi thinking handling (reasoning_content for tool-call turns) + tests. Integrated into release/v3.8.11.
…s + add contributor hall Consolidate the split [Unreleased]/[3.8.11] sections into one, add entries for every merged contributor PR that was missing credit (cheaper-inference#3170/cheaper-inference#3171/cheaper-inference#3172 @pizzav-xyz, cheaper-inference#3185/cheaper-inference#3195 @zhiru, cheaper-inference#3188 @xz-dev, cheaper-inference#3189/cheaper-inference#3203/cheaper-inference#3204/cheaper-inference#3241 @wilsonicdev, cheaper-inference#3191 @bypanghu, cheaper-inference#3206 @juandisay, cheaper-inference#3217 @oyi77, cheaper-inference#3226 @miracuves, cheaper-inference#3187/cheaper-inference#3200 maintainer), drop stale v3.8.8 leftovers (cheaper-inference#2958/cheaper-inference#2959 already shipped) and 3 empty v3.8.10 stub headers.
…nference#3191) Refine Kimi thinking handling (reasoning_content for tool-call turns) + tests. Integrated into release/v3.8.11.
…s + add contributor hall Consolidate the split [Unreleased]/[3.8.11] sections into one, add entries for every merged contributor PR that was missing credit (cheaper-inference#3170/cheaper-inference#3171/cheaper-inference#3172 @pizzav-xyz, cheaper-inference#3185/cheaper-inference#3195 @zhiru, cheaper-inference#3188 @xz-dev, cheaper-inference#3189/cheaper-inference#3203/cheaper-inference#3204/cheaper-inference#3241 @wilsonicdev, cheaper-inference#3191 @bypanghu, cheaper-inference#3206 @juandisay, cheaper-inference#3217 @oyi77, cheaper-inference#3226 @miracuves, cheaper-inference#3187/cheaper-inference#3200 maintainer), drop stale v3.8.8 leftovers (cheaper-inference#2958/cheaper-inference#2959 already shipped) and 3 empty v3.8.10 stub headers.
…nference#3191) Refine Kimi thinking handling (reasoning_content for tool-call turns) + tests. Integrated into release/v3.8.11.
…s + add contributor hall Consolidate the split [Unreleased]/[3.8.11] sections into one, add entries for every merged contributor PR that was missing credit (cheaper-inference#3170/cheaper-inference#3171/cheaper-inference#3172 @pizzav-xyz, cheaper-inference#3185/cheaper-inference#3195 @zhiru, cheaper-inference#3188 @xz-dev, cheaper-inference#3189/cheaper-inference#3203/cheaper-inference#3204/cheaper-inference#3241 @wilsonicdev, cheaper-inference#3191 @bypanghu, cheaper-inference#3206 @juandisay, cheaper-inference#3217 @oyi77, cheaper-inference#3226 @miracuves, cheaper-inference#3187/cheaper-inference#3200 maintainer), drop stale v3.8.8 leftovers (cheaper-inference#2958/cheaper-inference#2959 already shipped) and 3 empty v3.8.10 stub headers.
Summary
Related Issues
Validation
npm run lintnpm run test:unitnpm run test:coverage>= 60%for statements, lines, functions, and branchesTests Added Or Updated
Coverage Notes
src/,open-sse/,electron/, orbin/, explain which tests cover the change.Reviewer Notes