fix(reasoning): detect thinking toggle from chat template and override parser state - #1031
Merged
Merged
Conversation
github-actions
Bot
added
tokenizer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenamed Changes
Sequence DiagramsequenceDiagram
participant User
participant GRPC as gRPC<br/>Request Handler
participant ChatTemplate
participant Tokenizer
participant Parser as Reasoning<br/>Parser
participant StreamProc as Streaming<br/>Processor
User->>GRPC: send request (may include chat_template_kwargs / <think>)
GRPC->>ChatTemplate: detect_thinking_toggle(template)
ChatTemplate-->>GRPC: ThinkingToggle + think_in_prefill
GRPC->>Tokenizer: tokenizer.thinking_toggle()
Tokenizer-->>GRPC: ThinkingToggle
GRPC->>GRPC: extract_thinking_from_kwargs(kwargs)
GRPC->>GRPC: should_mark_reasoning_started(user_pref, tokenizer)
alt mark reasoning
GRPC->>Parser: reset() (if pooled)
GRPC->>Parser: mark_reasoning_started()
alt think in prefill
GRPC->>Parser: mark_think_start_stripped()
end
end
GRPC->>Parser: detect_and_parse_reasoning(...)
Parser-->>StreamProc: parsed reasoning chunks
StreamProc-->>User: stream parsed output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements support for thinking toggles in chat templates, allowing models to dynamically enter reasoning mode based on template-defined defaults or user-provided arguments. Key changes include the addition of a mark_reasoning_started method to the ReasoningParser trait, logic to detect toggle configurations in Jinja2 templates, and integration within the model gateway's response processing. Review feedback suggests resetting pooled parsers before use to prevent state leakage between requests, updating the base parser to correctly handle tag stripping when reasoning is pre-started, and adopting AST traversal for more robust template variable detection.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
model_gateway/src/routers/grpc/regular/processor.rs (2)
106-127:⚠️ Potential issue | 🔴 CriticalMissing
parser.reset()before state modification can cause cross-request state leakage.Pooled parsers are shared across requests. After request 1 calls
mark_reasoning_started(),self.in_reasoning = true. If request 2 hasuser_thinking = Some(false)orThinkingToggle::None,should_mark_reasoning_started()returns false but the parser'sin_reasoningis stilltruefrom request 1. This causesdetect_and_parse_reasoningto misclassify normal content as reasoning.The
detect_and_parse_reasoningmethod readsself.in_reasoningbut doesn't reset it:let in_reasoning = self.in_reasoning || text.contains(&self.config.think_start_token);🐛 Proposed fix: Reset parser state before each use
let mut parser = pooled_parser.lock().await; + // Reset parser state to baseline before each request + parser.reset(); + // If the template injected `<think>` in the prefill (thinking toggle // is supported and effectively ON), start in reasoning mode. if utils::should_mark_reasoning_started(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 106 - 127, After acquiring the pooled parser lock (after let mut parser = pooled_parser.lock().await), call parser.reset() to clear any residual state before you inspect or modify it; then proceed to call utils::should_mark_reasoning_started(...) and, if true, parser.mark_reasoning_started() as before — this prevents cross-request state leakage affecting parser.detect_and_parse_reasoning() which reads self.in_reasoning. Ensure the reset occurs before any use of parser (e.g., before mark_reasoning_started() and detect_and_parse_reasoning()) so each request starts with a clean parser state.
580-604:⚠️ Potential issue | 🔴 CriticalSame state leakage issue exists in the Messages API path.
The pooled parser needs to be reset before conditionally calling
mark_reasoning_started()to avoid cross-request state contamination.🐛 Proposed fix
let mut parser = pooled_parser.lock().await; + // Reset parser state to baseline before each request + parser.reset(); + // If the template injected `<think>` in the prefill, start in reasoning mode. { let user_thinking = match &messages_request.thinking {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 580 - 604, The pooled parser is not being reset before using it for a new request, which allows state (like prior reasoning) to leak; before checking should_mark_reasoning_started and calling parser.mark_reasoning_started(), reset the parser instance obtained from pooled_parser.lock().await (e.g., call parser.reset() or the parser's equivalent clear/reset method) so it starts with a clean state, then proceed with the existing logic that checks messages_request.thinking, calls utils::should_mark_reasoning_started(...), and later calls parser.detect_and_parse_reasoning(&processed_text).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 106-127: After acquiring the pooled parser lock (after let mut
parser = pooled_parser.lock().await), call parser.reset() to clear any residual
state before you inspect or modify it; then proceed to call
utils::should_mark_reasoning_started(...) and, if true,
parser.mark_reasoning_started() as before — this prevents cross-request state
leakage affecting parser.detect_and_parse_reasoning() which reads
self.in_reasoning. Ensure the reset occurs before any use of parser (e.g.,
before mark_reasoning_started() and detect_and_parse_reasoning()) so each
request starts with a clean parser state.
- Around line 580-604: The pooled parser is not being reset before using it for
a new request, which allows state (like prior reasoning) to leak; before
checking should_mark_reasoning_started and calling
parser.mark_reasoning_started(), reset the parser instance obtained from
pooled_parser.lock().await (e.g., call parser.reset() or the parser's equivalent
clear/reset method) so it starts with a clean state, then proceed with the
existing logic that checks messages_request.thinking, calls
utils::should_mark_reasoning_started(...), and later calls
parser.detect_and_parse_reasoning(&processed_text).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1ae3c90-5605-4936-b84e-492438dd3b83
📒 Files selected for processing (20)
crates/reasoning_parser/src/factory.rscrates/reasoning_parser/src/parsers/base.rscrates/reasoning_parser/src/parsers/cohere_cmd.rscrates/reasoning_parser/src/parsers/deepseek_r1.rscrates/reasoning_parser/src/parsers/glm45.rscrates/reasoning_parser/src/parsers/kimi.rscrates/reasoning_parser/src/parsers/minimax.rscrates/reasoning_parser/src/parsers/nano_v3.rscrates/reasoning_parser/src/parsers/qwen3.rscrates/reasoning_parser/src/parsers/step3.rscrates/reasoning_parser/src/traits.rscrates/tokenizer/src/cache/mod.rscrates/tokenizer/src/chat_template.rscrates/tokenizer/src/huggingface.rscrates/tokenizer/src/tiktoken.rscrates/tokenizer/src/traits.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/utils/mod.rsmodel_gateway/src/routers/grpc/utils/parsers.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
model_gateway/src/routers/grpc/regular/processor.rs (1)
576-596:⚠️ Potential issue | 🟠 MajorThe new override is still bypassed for default-on and always-reasoning models.
Line 593 correctly asks
should_mark_reasoning_started(...), but this block only runs afterreasoning_parser_availablewas computed fromThinkingConfig::Enabledearlier in the function. That means the non-streaming Messages path still skips reasoning parsing whenmessages_request.thinkingisNoneonThinkingToggle::DefaultOn, and also for models that emit thinking tokens regardless of the request toggle. In both cases the reasoning content falls back into normal text/tool parsing, so this path still misses the bug the PR is fixing.Suggested fix
- // Check parser availability - // Only run reasoning parser when the user explicitly enabled thinking in the request. - // Without this gate, the reasoning parser misclassifies normal text and tool call JSON - // as thinking content, breaking tool use and producing incorrect content blocks. - let separate_reasoning = matches!( - &messages_request.thinking, - Some(messages::ThinkingConfig::Enabled { .. }) - ); - let reasoning_parser_available = separate_reasoning - && utils::check_reasoning_parser_availability( + // Keep reasoning parsing available whenever the model has a parser. + let reasoning_parser_available = utils::check_reasoning_parser_availability( &self.reasoning_parser_factory, self.configured_reasoning_parser.as_deref(), &messages_request.model, ); @@ - if separate_reasoning && !reasoning_parser_available { + if !reasoning_parser_available { tracing::debug!( "No reasoning parser found for model '{}', reasoning content will not be separated", messages_request.model ); }Based on learnings: in
model_gateway/src/routers/grpc/regular/processor.rs, keep attempting reasoning parsing whenever a reasoning parser is available regardless ofThinkingConfig, because some models always emit thinking tokens and the parser is the right place to separate them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 576 - 596, The reasoning parser block is currently gated by the earlier computed reasoning_parser_available (derived from ThinkingConfig), causing models that always emit thinking tokens to skip reasoning parsing; change the logic to attempt reasoning parsing whenever a parser is available regardless of messages_request.thinking. Concretely, call utils::get_reasoning_parser(&self.reasoning_parser_factory, self.configured_reasoning_parser.as_deref(), &messages_request.model) and acquire pooled_parser/parser.reset() even when messages_request.thinking is None (i.e., remove the earlier ThinkingConfig-based guard that sets reasoning_parser_available), then keep the should_mark_reasoning_started(...) check and parser.mark_reasoning_started() as-is so models that emit thinking tokens are still handled by the reasoning parser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 576-596: The reasoning parser block is currently gated by the
earlier computed reasoning_parser_available (derived from ThinkingConfig),
causing models that always emit thinking tokens to skip reasoning parsing;
change the logic to attempt reasoning parsing whenever a parser is available
regardless of messages_request.thinking. Concretely, call
utils::get_reasoning_parser(&self.reasoning_parser_factory,
self.configured_reasoning_parser.as_deref(), &messages_request.model) and
acquire pooled_parser/parser.reset() even when messages_request.thinking is None
(i.e., remove the earlier ThinkingConfig-based guard that sets
reasoning_parser_available), then keep the should_mark_reasoning_started(...)
check and parser.mark_reasoning_started() as-is so models that emit thinking
tokens are still handled by the reasoning parser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e869d22c-204c-4299-99a7-79cd0bcb1aa5
📒 Files selected for processing (1)
model_gateway/src/routers/grpc/regular/processor.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/tokenizer/src/chat_template.rs`:
- Around line 60-87: The detect_thinking_toggle function uses brittle exact
substring checks causing misclassification; normalize the template (lowercase,
collapse whitespace) and broaden pattern detection in
detect_thinking_toggle/ThinkingToggle to catch common variants: match "if not
thinking" and "if not thinking is defined", any "not thinking" negation forms,
"thinking is defined" and "is defined" variants, and flexible assignment forms
like "set thinking *= *false" or "set enable_thinking *= *false" (allow optional
spaces and optional `=`), plus variants like "enable_thinking is defined" and
"enable_thinking == false"; implement these checks with simple regexes or
normalized substring matches instead of exact literals so DefaultOff and
DefaultOn are determined robustly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71c82f23-5979-4d24-a3c5-0e5cf2271d74
📒 Files selected for processing (1)
crates/tokenizer/src/chat_template.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/tokenizer/src/chat_template.rs (1)
129-146:⚠️ Potential issue | 🟠 MajorKeep walking until
think_in_prefillis settled.
Detector::run()now returnsthink_in_prefill, butwalk_stmt()still exits as soon as any OpenAI-format flag is found. If the template hits a message loop before a later{% if add_generation_prompt %}block,detect_all_with_ast()returnsthink_in_prefill = falseand the streaming path never treats the prefilled<think>as already consumed.Suggested change
fn walk_stmt(&mut self, stmt: &Stmt) { - // Early exit if we've already detected an OpenAI pattern - if self.flags.any() { + // Stop only once both outputs are known. + if self.flags.any() && self.think_in_prefill { return; }Also applies to: 283-288, 425-431
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tokenizer/src/chat_template.rs` around lines 129 - 146, Detector::run currently returns think_in_prefill but walk_stmt short-circuits as soon as an OpenAI-format flag is found, causing detect_all_with_ast to miss later `{% if add_generation_prompt %}` blocks; update the traversal so Detector::run calls walk_stmt and lets the AST walk complete (or explicitly continues walking after finding flags) until think_in_prefill is settled; remove or alter any early returns in walk_stmt that stop traversal on finding OpenAI flags (refer to Detector::run, Detector::walk_stmt, think_in_prefill and detect_all_with_ast) and apply the same fix to the other short-circuiting spots referenced (around the other occurrences noted).model_gateway/src/routers/grpc/regular/processor.rs (1)
502-518:⚠️ Potential issue | 🟠 MajorKeep Messages reasoning parsing available even when no override is needed.
should_mark_reasoning_started()only tells you whether to pre-mark the parser. Using it to computereasoning_parser_availabledisables parsing for no-toggle models and for models that still emit reasoning whenThinkingConfigis disabled/unsupported, which pushes reasoning into the finalMessagetext instead of separating it. Keep the availability check unconditional and use the flag only at themark_reasoning_started()call.Based on learnings: in `model_gateway/src/routers/grpc/regular/processor.rs`, always attempt reasoning parsing whenever a parser is available; the parser handles `ThinkingConfig::Disabled` internally.Suggested fix
- let separate_reasoning = + let thinking_override = utils::should_mark_reasoning_started(user_thinking, tokenizer.as_ref()); - let reasoning_parser_available = separate_reasoning - && utils::check_reasoning_parser_availability( + let reasoning_parser_available = utils::check_reasoning_parser_availability( &self.reasoning_parser_factory, self.configured_reasoning_parser.as_deref(), &messages_request.model, ); @@ - if separate_reasoning && !reasoning_parser_available { + if thinking_override && !reasoning_parser_available { tracing::debug!( "No reasoning parser found for model '{}', reasoning content will not be separated", messages_request.model ); } @@ - if separate_reasoning { + if thinking_override { parser.mark_reasoning_started(); }Also applies to: 586-592
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/regular/processor.rs` around lines 502 - 518, The code currently gates parser availability using utils::should_mark_reasoning_started(), which disables parsing for no-toggle models; instead always call utils::check_reasoning_parser_availability(...) (using self.reasoning_parser_factory, self.configured_reasoning_parser.as_deref(), &messages_request.model) to set reasoning_parser_available, and keep the separate_reasoning flag only for deciding when to call utils::mark_reasoning_started(tokenizer.as_ref(), ...); in short, remove should_mark_reasoning_started() from the availability check and only use it for mark_reasoning_started() so the parser runs whenever available.
♻️ Duplicate comments (1)
crates/tokenizer/src/chat_template.rs (1)
61-87:⚠️ Potential issue | 🟠 MajorNormalize and broaden the toggle detector before using it for parser overrides.
This still keys off a handful of exact
contains()strings. Common spacing/negation variants likeif not thinking,thinking is defined, or differently formattedset ... = falseassignments will fall through to the wrongThinkingToggle, and that misclassification now feeds directly into parser pre-marking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/tokenizer/src/chat_template.rs` around lines 61 - 87, The current detect_thinking_toggle uses fragile exact contains() checks; normalize the template to lowercase and collapse whitespace then use broad, word-boundary regex checks in detect_thinking_toggle to catch common variants (e.g. "if not thinking", "not thinking", "thinking is defined", "is defined", "if thinking", and spacing variants around '='). Specifically: lowercase the template, replace runs of whitespace with single spaces, and match patterns like r"\bset\s+(thinking|enable_thinking)\s*=\s*false\b" and r"\b(if\s+not\s+thinking|not\s+thinking|thinking\s+is\s+defined|thinking\s+is\b|if\s+thinking)\b" and r"\b(enable_thinking|thinking)\b" (use separate checks for presence vs explicit default-off); keep returning the same enums (ThinkingToggle::None, ::DefaultOff, ::DefaultOn) but derive them from these normalized/regex matches so spacing/case/negation variants are handled reliably by the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/reasoning_parser/src/parsers/base.rs`:
- Around line 159-165: Add a focused regression test that exercises the
prefill-override hooks by creating a parser instance with always_in_reasoning =
false and driving it through the streaming path so that mark_reasoning_started()
and mark_think_start_stripped() are both invoked; specifically, write a test
that simulates streaming input that triggers the think-start detection and
asserts the parser's state (in_reasoning == true and stripped_think_start ==
true) and any expected outputs or callbacks, using the parser constructor and
streaming methods used in base.rs to reproduce the exact failure mode.
In `@crates/reasoning_parser/src/parsers/minimax.rs`:
- Around line 79-84: The methods mark_reasoning_started and
mark_think_start_stripped currently only update self.base so
parse_reasoning_streaming_incremental still treats the next input as the first
chunk and prepends a synthetic "<think>"; update these hooks to also clear the
MiniMaxParser-level first-chunk flag (e.g. set self.is_first_chunk = false or
the equivalent field used in parse_reasoning_streaming_incremental) so that when
MiniMaxParser::parse_reasoning_streaming_incremental runs after pre-marking it
will not prepend another "<think>".
In `@model_gateway/src/routers/grpc/regular/streaming.rs`:
- Around line 1579-1595: The code currently gates instantiation of the reasoning
parser on thinking_override, which prevents parser creation for
ThinkingToggle::None models that rely on the parser's internal
always_in_reasoning behavior; change the reasoning_parser_available computation
to call utils::check_reasoning_parser_availability(...) directly (i.e., remove
the leading "thinking_override &&" conjunction), leaving thinking_override,
utils::should_mark_reasoning_started(original_request.thinking,
tokenizer.as_ref()), and tokenizer.think_in_prefill() unchanged so
process_messages_reasoning() can instantiate the parser whenever the
factory/config indicate availability.
---
Outside diff comments:
In `@crates/tokenizer/src/chat_template.rs`:
- Around line 129-146: Detector::run currently returns think_in_prefill but
walk_stmt short-circuits as soon as an OpenAI-format flag is found, causing
detect_all_with_ast to miss later `{% if add_generation_prompt %}` blocks;
update the traversal so Detector::run calls walk_stmt and lets the AST walk
complete (or explicitly continues walking after finding flags) until
think_in_prefill is settled; remove or alter any early returns in walk_stmt that
stop traversal on finding OpenAI flags (refer to Detector::run,
Detector::walk_stmt, think_in_prefill and detect_all_with_ast) and apply the
same fix to the other short-circuiting spots referenced (around the other
occurrences noted).
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 502-518: The code currently gates parser availability using
utils::should_mark_reasoning_started(), which disables parsing for no-toggle
models; instead always call utils::check_reasoning_parser_availability(...)
(using self.reasoning_parser_factory,
self.configured_reasoning_parser.as_deref(), &messages_request.model) to set
reasoning_parser_available, and keep the separate_reasoning flag only for
deciding when to call utils::mark_reasoning_started(tokenizer.as_ref(), ...); in
short, remove should_mark_reasoning_started() from the availability check and
only use it for mark_reasoning_started() so the parser runs whenever available.
---
Duplicate comments:
In `@crates/tokenizer/src/chat_template.rs`:
- Around line 61-87: The current detect_thinking_toggle uses fragile exact
contains() checks; normalize the template to lowercase and collapse whitespace
then use broad, word-boundary regex checks in detect_thinking_toggle to catch
common variants (e.g. "if not thinking", "not thinking", "thinking is defined",
"is defined", "if thinking", and spacing variants around '='). Specifically:
lowercase the template, replace runs of whitespace with single spaces, and match
patterns like r"\bset\s+(thinking|enable_thinking)\s*=\s*false\b" and
r"\b(if\s+not\s+thinking|not\s+thinking|thinking\s+is\s+defined|thinking\s+is\b|if\s+thinking)\b"
and r"\b(enable_thinking|thinking)\b" (use separate checks for presence vs
explicit default-off); keep returning the same enums (ThinkingToggle::None,
::DefaultOff, ::DefaultOn) but derive them from these normalized/regex matches
so spacing/case/negation variants are handled reliably by the function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c82c6faa-9f2e-486f-9412-5a73c8f7b902
📒 Files selected for processing (18)
crates/reasoning_parser/src/parsers/base.rscrates/reasoning_parser/src/parsers/cohere_cmd.rscrates/reasoning_parser/src/parsers/deepseek_r1.rscrates/reasoning_parser/src/parsers/glm45.rscrates/reasoning_parser/src/parsers/kimi.rscrates/reasoning_parser/src/parsers/minimax.rscrates/reasoning_parser/src/parsers/nano_v3.rscrates/reasoning_parser/src/parsers/qwen3.rscrates/reasoning_parser/src/parsers/step3.rscrates/reasoning_parser/src/traits.rscrates/tokenizer/src/cache/mod.rscrates/tokenizer/src/chat_template.rscrates/tokenizer/src/huggingface.rscrates/tokenizer/src/tiktoken.rscrates/tokenizer/src/traits.rsmodel_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rsmodel_gateway/src/routers/grpc/utils/parsers.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@model_gateway/src/routers/grpc/regular/processor.rs`:
- Around line 583-596: The reasoning parser creation is currently gated by
matches!(messages_request.thinking, Some(ThinkingConfig::Enabled { .. })),
preventing default-ON templates from using reasoning when
messages_request.thinking is None; change the logic so you attempt to
construct/use the reasoning parser whenever a reasoning parser is available
(i.e., remove the explicit Enabled-only gate), then let
should_mark_reasoning_started(messages_request.thinking, tokenizer.as_ref())
decide whether to call parser.mark_reasoning_started(); ensure the code still
respects explicit Disabled by allowing should_mark_reasoning_started to suppress
reasoning and that parser.reset()/parser usage remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0335fd8-bd90-4004-8af5-2c4500d7f51b
📒 Files selected for processing (2)
model_gateway/src/routers/grpc/regular/processor.rsmodel_gateway/src/routers/grpc/regular/streaming.rs
Member
Author
|
Need to further improve for one case: When user sends the wrong thinking toggle in chat completions. For instance, sends |
CatherineSue
force-pushed
the
fix/reasoning-parser-thinking-toggle
branch
from
April 3, 2026 15:01
6bb54e6 to
24ea003
Compare
slin1237
approved these changes
Apr 3, 2026
…untime Some chat templates inject <think> in the prefill when thinking is enabled, so the model outputs reasoning directly without emitting <think> itself. The reasoning parser needs to start in reasoning mode for these models. Changes: - Add ThinkingToggle enum (None/DefaultOn/DefaultOff) with detection from the chat template. Most models default ON; DeepSeek V3.1 defaults OFF. - Add mark_reasoning_started() to ReasoningParser trait. - Rename initial_in_reasoning -> always_in_reasoning to clarify it is for models that always reason (DeepSeek R1) vs toggle-based models. - Add should_mark_reasoning_started(user_thinking, tokenizer) helper. - Wire into all 4 response processing paths. - Register DeepSeek V3.1, Kimi-K2.5, and Kimi-K2-Thinking parsers with correct patterns in the parser factory. Signed-off-by: Chang Su <chang.s.su@oracle.com>
The non-streaming paths use pooled (shared) parsers. Without reset(), mark_reasoning_started() sets in_reasoning=true permanently on the shared instance, causing all subsequent requests to misclassify normal content as reasoning. Add parser.reset() before the thinking override check to ensure clean state for each request. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
- Combine content format and think-in-prefill AST detection into a single pass (detect_all_with_ast). No more double parsing. - Add mark_think_start_stripped() to parser trait for streaming paths where <think> was in the prefill and should not be stripped again. - Fix Messages API: use should_mark_reasoning_started() for the separate_reasoning gate so thinking-default-ON models get reasoning parsing even when ThinkingConfig is None. - Only call mark_think_start_stripped() in streaming paths where stripped_think_start is actually used. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…it opt-in should_mark_reasoning_started() answers "should we call mark_reasoning_started on the parser?" — NOT "should we run the reasoning parser at all?". Using it for the separate_reasoning gate broke DeepSeek R1 on the Messages API (ThinkingToggle::None returns false, skipping reasoning parsing even with ThinkingConfig::Enabled). Revert Messages API to the original explicit ThinkingConfig::Enabled check for gating reasoning parser activation. The should_mark_reasoning_started() call remains for the parser state override only. Signed-off-by: Chang Su <chang.s.su@oracle.com>
The MiniMax-M2 template always injects <think> in the prefill. Replace the synthetic <think> prepend hack (is_first_chunk + format!) with always_in_reasoning=true, which naturally treats output as reasoning until </think>. Same behavior, no hack. Signed-off-by: Chang Su <chang.s.su@oracle.com>
…prompt
The early exit fired after content format flags were set, before
reaching the {% if add_generation_prompt %} block (which always
appears after the message loop). This caused think_in_prefill to
stay false for templates that inject <think> in the generation prompt.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
Use trailing space in string patterns ("if thinking ", "thinking is ",
"set thinking ") to prevent matching longer identifiers like
thinking_mode or message.thinking (gpt-oss).
Verified against all 16 models — zero misclassifications.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
DeepSeek V3.1 uses {% if add_generation_prompt and ns.is_last_user %}
which is a BinOp, not a bare Var. Walk the expression tree to find
add_generation_prompt anywhere in compound conditions.
Signed-off-by: Chang Su <chang.s.su@oracle.com>
extract_thinking_from_kwargs now uses ThinkingKeyName to only check the key the template recognizes (enable_thinking for Qwen3/GLM/Nemotron, thinking for DeepSeek V3.1/Kimi-K2.5). This prevents mismatches where the user passes enable_thinking=false for a model that only reads thinking — the template would ignore it and keep thinking ON, but our detection would incorrectly think it's OFF. Signed-off-by: Chang Su <chang.s.su@oracle.com>
Signed-off-by: Chang Su <chang.s.su@oracle.com>
CatherineSue
force-pushed
the
fix/reasoning-parser-thinking-toggle
branch
from
April 3, 2026 17:08
0e7d1c8 to
24d2158
Compare
2 of 4 tasks
2 tasks done
This was referenced Jun 10, 2026
5 tasks
This was referenced Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Problem
On
main, reasoning content parsing is broken for models whose chat template injects<think>in the prefill when thinking is enabled (Qwen3.5, Nemotron, GLM-5, DeepSeek V3.1, Kimi-K2.5).When thinking is ON, these templates emit
<think>as part ofadd_generation_prompt— the token is in the prefill, not in the model output. The model starts outputting reasoning content immediately without generating<think>itself. But the reasoning parser (configured withinitial_in_reasoning=false) waits for a<think>tag in the generated output that never comes. All reasoning content is misclassified as normalcontentinstead ofreasoning_content.Models that work correctly on
main:<think>— model generates it → parser finds itinitial_in_reasoning=true→ no need to find<think>Models broken on
main:<think>in prefill → parser never sees it → reasoning misclassifiedSolution
Detect template properties at tokenizer init time and override parser state at runtime:
ThinkingToggle(None/DefaultOn/DefaultOff) — detected via string matching from the templateThinkingKeyName(EnableThinking/Thinking) — which kwarg the template reads, soextract_thinking_from_kwargsonly checks the correct key and avoids mismatches (e.g. user sendsenable_thinking=falsefor athinking-based template like Kimi-K2.5)think_in_prefill— AST detection of<think>inside{% if add_generation_prompt %}blocks (single-pass with content format detection)should_mark_reasoning_started(user_thinking, tokenizer)— combines user request with template defaultmark_reasoning_started()+mark_think_start_stripped()— called when thinking is ON and the template injects<think>in prefillAlso renamed
initial_in_reasoningtoalways_in_reasoningto clarify it is for models that always reason (DeepSeek R1) regardless of any template toggle.Template Behavior (verified by rendering each template)
Category A — Template does NOT inject
<think>when ON. Model generates it.enable_thinkingassistant\n(bare)assistant\n+<think>\n\n</think>\n\nenable_thinking<|assistant|>(bare)<|assistant|>\n+<think></think>Category B — Template DOES inject
<think>when ON. Parser needsmark_reasoning_started()+mark_think_start_stripped().enable_thinkingassistant\n+<think>\nassistant\n+<think>\n\n</think>\n\nenable_thinkingassistant\n+<think>\nassistant\n+<think></think>enable_thinking<|assistant|>+<think><|assistant|>+</think>thinking<|Assistant|>+<think><|Assistant|>+</think>thinking...+<think>...+<think></think>Category C — No thinking toggle. Model always (or never) reasons.
always_in_reasoningtruetrue<think>\n, ignores kwargstruetrue<think>\nChanges
crates/tokenizer/src/chat_template.rs: AddThinkingToggle,ThinkingKeyNameenums,detect_thinking_toggle(), andthink_in_prefillAST detection (single-pass with content format)crates/tokenizer/src/traits.rs: Addthinking_toggle(),thinking_key_name(),think_in_prefill()to Tokenizer traitcrates/reasoning_parser/src/traits.rs: Addmark_reasoning_started()andmark_think_start_stripped(), renameinitial_in_reasoningtoalways_in_reasoningcrates/reasoning_parser/src/parsers/minimax.rs: Simplify toalways_in_reasoning=true(remove synthetic<think>prepend hack)crates/reasoning_parser/src/parsers/*.rs: Implement new trait methods, remove stale inline commentscrates/reasoning_parser/src/factory.rs: Register DeepSeek V3.1, Kimi-K2.5, and Kimi-K2-Thinking parsersmodel_gateway/src/routers/grpc/utils/parsers.rs: Addshould_mark_reasoning_started(),extract_thinking_from_kwargs()(key-name-aware)model_gateway/src/routers/grpc/regular/processor.rs: Wire override, reset pooled parsers before each requestmodel_gateway/src/routers/grpc/regular/streaming.rs: Wire override for streaming pathsTest Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspasses