feat(reasoning_parser): add Mistral/Magistral reasoning parser - #1816
feat(reasoning_parser): add Mistral/Magistral reasoning parser#1816slin1237 wants to merge 1 commit into
Conversation
Magistral reasoning models emit reasoning between [THINK] and [/THINK] markers. Adds a BaseReasoningParser-backed parser (always_in_reasoning=false) and routes only the `magistral` pattern to it, so plain Mistral-Small / Mixtral (non-reasoning) models still fall through to passthrough. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesMistralParser for Magistral Reasoning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 a new MistralParser to support Mistral/Magistral reasoning models, which utilize [THINK] and [/THINK] markers. The parser is registered in the ParserFactory under the pattern "magistral", and comprehensive unit tests are added to verify correct routing and parsing behavior (ensuring plain Mistral models correctly fall back to passthrough). The review feedback suggests deriving Debug and Clone on the public MistralParser struct to improve usability and align with Rust API guidelines.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub struct MistralParser { | ||
| base: BaseReasoningParser, | ||
| } |
There was a problem hiding this comment.
To adhere to Rust API guidelines and general best practices, public structs should implement common traits like Debug and Clone whenever possible. Since BaseReasoningParser already implements both, we can easily derive them for MistralParser to improve usability and debuggability.
| pub struct MistralParser { | |
| base: BaseReasoningParser, | |
| } | |
| #[derive(Debug, Clone)] | |
| pub struct MistralParser { | |
| base: BaseReasoningParser, | |
| } |
References
- Prefer deriving 'Debug' using '#[derive(Debug)]' instead of manually implementing 'std::fmt::Debug' in Rust, unless manual implementation is necessary (e.g., when some fields do not implement 'Debug').
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good. Clean implementation following the established parser pattern. The magistral-only routing is correctly scoped so non-reasoning Mistral/Mixtral models aren't misrouted, and the negative test (test_plain_mistral_is_not_reasoning) locks that in. Good test coverage across streaming edge cases (split markers) and reset behavior.
0 🔴 Important · 0 🟡 Nit · 0 🟣 Pre-existing
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6d40c81d6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| &mut self, | ||
| text: &str, | ||
| ) -> Result<ParserResult, ParseError> { | ||
| self.base.parse_reasoning_streaming_incremental(text) |
There was a problem hiding this comment.
When a streaming chunk ends with only the beginning of the end marker after some reasoning text, this delegation to BaseReasoningParser emits and clears the whole buffer instead of retaining the partial marker. For example [THINK]thought -> more[/TH -> INK]answer will emit more[/TH as reasoning, lose the buffered prefix, and then never recognize [/THINK], so the final answer stays hidden as reasoning. The new tests only cover a partial marker when it is the entire chunk; real chunk boundaries can split after content as well.
Useful? React with 👍 / 👎.
| // Magistral is the reasoning member of the Mistral family. Match only | ||
| // `magistral` (not a broad `mistral`/`mixtral` substring) so plain | ||
| // Mistral-Small / Mixtral non-reasoning models are not misrouted here. | ||
| registry.register_pattern("magistral", "mistral"); |
There was a problem hiding this comment.
Auto-detection only registers the magistral substring, so official Mistral reasoning model IDs such as mistralai/Ministral-3-8B-Reasoning-2512 fall through to passthrough even though vLLM documents serving them with --reasoning-parser mistral (https://docs.vllm.ai/projects/recipes/en/latest/Mistral/Ministral-3-Reasoning.html). In deployments relying on model-name auto-detection, their [THINK]...[/THINK] blocks will be emitted as normal content; add a narrow ministral + reasoning style pattern rather than broad mistral.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale because it has not had any activity within 14 days. It will be automatically closed if no further activity occurs within 16 days. Leave a comment if you feel this pull request should remain open. Thank you! |
|
Hi @slin1237, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
1 participant
Description
Problem
SMG cannot extract reasoning from Mistral's Magistral models, which emit reasoning between
[THINK]/[/THINK]markers.Solution
Add a Magistral reasoning parser and route only the
magistralmodel pattern to it — deliberately not matching broadmistral/mixtral, so non-reasoning Mistral models keep passthrough behavior.Changes
crates/reasoning_parser/src/parsers/mistral.rs):[THINK]/[/THINK]markers viaBaseReasoningParser(always_in_reasoning=false— the template does not prefill the start token).mistral; route patternmagistral→mistral. Plainmistral-small/mixtralresolve topassthrough(asserted in tests).Test Plan
cargo +nightly fmt --all— cleancargo clippy -p reasoning-parser --all-targets -- -D warnings— cleancargo test -p reasoning-parser— 82 passed, 0 failed. New tests cover complete extraction, whitespace preservation, truncated traces (no end token), streaming with the start/end marker split across chunk boundaries ([TH|INK],[/TH|INK]), passthrough, reset, and the factory routing (magistral → reasoning; plain mistral/mixtral → passthrough).Checklist
cargo +nightly fmtpassescargo clippy --all-targets -- -D warningspasses (reasoning-parser)Summary by CodeRabbit
Release Notes
New Features
Tests