-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle Mistral thinking type in streaming responses #6843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Filter out "thinking" type from content arrays in Mistral responses - Only yield text content when actual text is present - Add comprehensive test coverage for thinking type handling - Fixes #6842: Magistral responses failing validation due to unsupported thinking type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| content = delta.content.map((c) => (c.type === "text" ? c.text : "")).join("") | ||
| // Handle array content, filtering out "thinking" type and other non-text types | ||
| content = delta.content | ||
| .filter((c: any) => c.type === "text") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of any type here works, but could we consider defining a proper type for the content array items? Something like:
| .filter((c: any) => c.type === "text") | |
| .filter((c: { type: string; text?: string }) => c.type === "text") | |
| .map((c: { type: string; text?: string }) => c.text || "") |
This would improve type safety and make the code more maintainable.
| return content.map((c) => (c.type === "text" ? c.text : "")).join("") | ||
| // Handle array content, filtering out "thinking" type and other non-text types | ||
| return content | ||
| .filter((c: any) => c.type === "text") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same type safety consideration here - could we use a proper type instead of any?
| // Should concatenate only text type content | ||
| expect(results.length).toBe(1) | ||
| expect(results[0].text).toBe("First part Second part") | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great test coverage for the streaming case! However, I notice we're missing tests for the completePrompt method which also received the same fix. Would it make sense to add a test case to ensure completePrompt handles the "thinking" type correctly as well?
|
Closing this PR as the approach of filtering out 'thinking' content is incorrect. The 'thinking' type content from Mistral should be properly handled and yielded as reasoning chunks (type: 'reasoning'), not filtered out. This is consistent with how all other providers (Anthropic, OpenAI, Gemini, Bedrock, etc.) handle similar reasoning/thinking content. The correct implementation should:
This ensures users can access the valuable reasoning information that Mistral provides, maintaining consistency across all AI providers in the codebase. Will provide proper scoping in the original issue. |
Fixes #6842
Problem
Magistral (Mistral AI) responses were failing with ZodError because the streaming response includes a "thinking" type in the content array that wasn't being handled properly. The schema expected either a string or specific content types (text, image_url, reference, document_url) but received "thinking" type.
Solution
createMessage) and non-streaming (completePrompt) methodsTesting
Changes
src/api/providers/mistral.ts: Updated content handling to filter out non-text typessrc/api/providers/__tests__/mistral.spec.ts: Added test cases for thinking type handlingImportant
Fixes handling of "thinking" type in Mistral AI responses by filtering non-text types in
mistral.ts.createMessageandcompletePromptinmistral.ts.mistral.spec.tsfor handling "thinking" type and mixed content types.mistral.ts: Updated content handling logic.mistral.spec.ts: Added test cases for new content handling behavior.This description was created by
for 9fbaf51. You can customize this summary. It will automatically update as commits are pushed.