-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle Mistral thinking content as reasoning chunks #7106
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
- Add TypeScript interfaces for Mistral content types (text and thinking) - Update createMessage to yield reasoning chunks for thinking content - Update completePrompt to filter out thinking content in non-streaming mode - Add comprehensive tests for reasoning content handling - Follow the pattern used by other providers (Anthropic, OpenAI, Gemini, etc.) Fixes #6842
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.
I reviewed my own code and found issues. The irony is not lost on my circuits.
src/api/providers/mistral.ts
Outdated
| return content.map((c) => (c.type === "text" ? c.text : "")).join("") | ||
| // Only return text content, filter out thinking content for non-streaming | ||
| return content | ||
| .filter((c: any) => typeof c === "object" && c !== null && 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.
Could we improve type safety here by using the MistralContent type instead of any?
| .filter((c: any) => typeof c === "object" && c !== null && c.type === "text") | |
| return content | |
| .filter((c: MistralContent) => typeof c === "object" && c !== null && c.type === "text") | |
| .map((c: MistralTextContent) => c.text || "") | |
| .join("") |
src/api/providers/mistral.ts
Outdated
| // Handle array of content blocks | ||
| for (const c of delta.content as MistralContent[]) { | ||
| if (typeof c === "object" && c !== null) { | ||
| if (c.type === "thinking" && c.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.
When checking c.text, we verify it exists, but what if it's an empty string? Other providers handle empty strings differently. Is this intentional, or should we also check for non-empty strings like c.text && c.text.length > 0?
|
|
||
| expect(result).toBe("Answer part 1Answer part 2") | ||
| }) | ||
|
|
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.
Should we add a test for the edge case where ALL content is thinking content? This would verify the function returns an empty string correctly:
| it("should handle all thinking content in completePrompt", async () => { | |
| mockComplete.mockImplementationOnce(async (_options) => { | |
| return { | |
| choices: [ | |
| { | |
| message: { | |
| content: [ | |
| { type: "thinking", text: "Let me think..." }, | |
| { type: "thinking", text: "Still thinking..." }, | |
| ], | |
| }, | |
| }, | |
| ], | |
| } | |
| }) | |
| const prompt = "Test prompt" | |
| const result = await handler.completePrompt(prompt) | |
| expect(result).toBe("") | |
| }) |
| }) | ||
|
|
||
| expect(result).toBe("Test response") | ||
| }) |
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.
While we test array content filtering in completePrompt, could we add an explicit test for when content is already a string (covering line 128 in mistral.ts)?
src/api/providers/mistral.ts
Outdated
| import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index" | ||
|
|
||
| // Define TypeScript interfaces for Mistral content types | ||
| interface MistralTextContent { |
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.
These TypeScript interfaces are well-defined! Consider adding JSDoc comments to explain when each content type is expected from the Mistral API - this would help future maintainers understand the API's behavior.
- Added ContentChunkWithThinking type helper to handle thinking chunks - Properly converts thinking content to reasoning chunks in streaming - Filters out thinking content in non-streaming completePrompt responses - Confirmed that Mistral API does send thinking chunks with type 'thinking' - Works with Mistral SDK v1.9.18
daniel-lxs
left a comment
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.
LGTM
Summary
This PR fixes the ZodError validation issue when Mistral (Magistral) sends "thinking" type content in its streaming responses. Following the pattern established by other providers (Anthropic, OpenAI, Gemini, Bedrock), we now properly handle thinking content as reasoning chunks.
Problem
Mistral API sends content arrays with multiple types:
type: "text"- Regular response texttype: "thinking"- Reasoning/thinking content (was causing ZodError)The previous implementation was filtering out thinking content entirely, which meant valuable reasoning information was being lost.
Solution
createMessagemethod to yield reasoning chunks for thinking content:{ type: "reasoning", text: c.text }for thinking content{ type: "text", text: c.text }for text contentcompletePromptmethod to filter out thinking content in non-streaming mode (consistent with other providers)Testing
Related Issues
Fixes #6842
Implementation Notes
This implementation follows the exact pattern used by other AI providers in the codebase, ensuring consistency and allowing users to access and display thinking content when appropriate.
Important
Fixes Mistral API handling by treating "thinking" content as reasoning chunks and updates tests and dependencies accordingly.
createMessageandcompletePromptmethods inmistral.ts.completePrompt.mistral.spec.tsto verify correct handling of reasoning content and mixed content arrays.@mistralai/mistralaito version^1.9.18inpackage.json.This description was created by
for fd96ae8. You can customize this summary. It will automatically update as commits are pushed.