fix(core): filter Mistral reasoning content at request boundary#3882
Conversation
wenshao
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI failing: Test (windows-latest, 22.x). — gpt-5.5 via Qwen Code /review
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
The fix is correct on what it sets out to do: a Mistral-only outbound copy of the chat history with reasoning_content stripped from messages, while the shared in-memory history is preserved for providers that still want it. The hostname guard parses the URL and matches api.mistral.ai exactly or as a dot-prefixed subdomain (nice — the api.mistral.ai.evil.example test pins this), the source-history objects are not mutated, and Mistral being last in determineProvider keeps OpenRouter / ModelScope / DashScope aliases routing to their existing providers. Two non-blocking notes for follow-ups:
1. Top-level reasoning body parameter is not stripped (severity: low · confidence: high)
When contentGeneratorConfig.reasoning is set, the pipeline injects a top-level reasoning field into every request, and the Mistral provider's buildRequest only filters messages[].reasoning_content. So a user with reasoning configured at the content-generator level who then points at api.mistral.ai is likely to hit the same kind of 400/422 this PR is meant to fix. Acknowledged as out of scope; a tracking issue would help.
2. magistral is on the marker list but is Mistral's reasoning model (severity: low · confidence: medium)
Magistral currently delivers the chain-of-thought inside <think> tags in content rather than the reasoning_content field, so today the strip is a harmless no-op for that family. If a future Magistral release moves the trace to reasoning_content (the de-facto OpenAI-compatible carrier), this strip would silently drop it on the wire and break multi-turn reasoning. Worth either a FIXME pointing at the assumption, or dropping magistral from the marker list and relying on the hostname check for that family.
Verdict
APPROVE — clean fix for the actual bug; the two notes above are forward-looking, not blocking.
Summary
reasoning_contentremoved from messages before the request is sent.Validation
reasoning_content, then an outbound Mistral request.reasoning_content; non-Mistral providers keep it; the source history still contains it.cd packages/core && npx vitest run src/core/openaiContentGenerator/provider/mistral.test.tssrc/core/openaiContentGenerator/providerpassing with 116/116 tests.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs
Fixes #3304