fix: add reasoning_content field for Moonshot AI thinking-enabled models - #364
Conversation
|
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:
WalkthroughThis change adds support for models that require a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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 unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/api/models.rs (1)
267-270: Clarify whether this is the full catalog or only the models.dev cache.
get_models()appendsextra_models()after the cache lookup, so this accessor is narrower than the catalog the rest of the file works with. Callers likesrc/llm/model.rscan therefore miss local-only entries such asminimax-cn/MiniMax-M2.5. Either includeextra_models()here or rename the API so the contract is obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/models.rs` around lines 267 - 270, The current get_models_cache() returns only the cached models from ensure_models_cache() which is narrower than the full catalog used elsewhere (get_models() appends extra_models()), so update get_models_cache() to return the full catalog by calling ensure_models_cache().await, then extend/append the Vec<ModelInfo> with the results of extra_models() (ensuring extra_models() is awaited/collected as needed) so callers (e.g., code using get_models_cache() in model resolution) receive both cache and local-only entries like minimax-cn/MiniMax-M2.5; alternatively, if you prefer a cache-only API, rename get_models_cache() to make the cache-only contract explicit and update all callers accordingly.src/llm/model.rs (1)
1310-1403: Add a regression test for the assistant tool-call history branch.This PR fixes a very specific serialization shape, but there isn't a unit test proving
convert_messages_to_openai()addsreasoning_contentonly for assistant messages withtool_callswhen the flag is enabled. A small test pair here would make this much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 1310 - 1403, Add a regression unit test for convert_messages_to_openai that verifies when needs_reasoning_content is true the function only injects the "reasoning_content" field for Assistant messages that include tool_calls and not for User messages or Assistant messages without tool_calls; construct test inputs with (1) an Assistant message containing a ToolCall, (2) an Assistant message with only text, and (3) a User message with tool results, call convert_messages_to_openai(needs_reasoning_content = true) and assert the serialized outputs include reasoning_content only on the assistant tool-call message and are unchanged otherwise; place the test alongside other unit tests for model serialization and reference convert_messages_to_openai, needs_reasoning_content, tool_calls, and reasoning_content in assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/llm/model.rs`:
- Around line 85-109: The function needs_reasoning_content currently awaits
get_models_cache() before checking local model-name heuristics, which can block
or return false on cache failure; change the logic in needs_reasoning_content to
first perform the fast local substring check on self.model_name (look for
"k2.5", "k2-5", "thinking", "reasoning") and return true immediately if matched,
and only then asynchronously query crate::api::get_models_cache() to consult
ModelInfo.reasoning for models not matched by the heuristic; also ensure any
cache errors/defaults do not short-circuit the heuristic (i.e., treat cache
failures as unknown and fall back to the local check result).
---
Nitpick comments:
In `@src/api/models.rs`:
- Around line 267-270: The current get_models_cache() returns only the cached
models from ensure_models_cache() which is narrower than the full catalog used
elsewhere (get_models() appends extra_models()), so update get_models_cache() to
return the full catalog by calling ensure_models_cache().await, then
extend/append the Vec<ModelInfo> with the results of extra_models() (ensuring
extra_models() is awaited/collected as needed) so callers (e.g., code using
get_models_cache() in model resolution) receive both cache and local-only
entries like minimax-cn/MiniMax-M2.5; alternatively, if you prefer a cache-only
API, rename get_models_cache() to make the cache-only contract explicit and
update all callers accordingly.
In `@src/llm/model.rs`:
- Around line 1310-1403: Add a regression unit test for
convert_messages_to_openai that verifies when needs_reasoning_content is true
the function only injects the "reasoning_content" field for Assistant messages
that include tool_calls and not for User messages or Assistant messages without
tool_calls; construct test inputs with (1) an Assistant message containing a
ToolCall, (2) an Assistant message with only text, and (3) a User message with
tool results, call convert_messages_to_openai(needs_reasoning_content = true)
and assert the serialized outputs include reasoning_content only on the
assistant tool-call message and are unchanged otherwise; place the test
alongside other unit tests for model serialization and reference
convert_messages_to_openai, needs_reasoning_content, tool_calls, and
reasoning_content in assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3af23a4-17ff-4802-8b30-118529e54d74
📒 Files selected for processing (3)
src/api.rssrc/api/models.rssrc/llm/model.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/llm/model.rs (1)
85-107:⚠️ Potential issue | 🟠 MajorCache lookup gates the heuristic, causing false negatives on cache miss.
The current logic checks the
ModelInfo.reasoningflag first and returnsfalseearly if the cache lookup fails or the model isn't found (line 92unwrap_or(false)). This means that for known reasoning models like Kimi K2.5, if the models.dev fetch fails or the model isn't in the catalog, the function returnsfalse— and the originalreasoning_content is missingAPI error can still occur.Consider reordering to check the cheap pattern heuristic first, so known reasoning model names still get the field injected even when the cache is unavailable:
🛠️ Suggested reordering
async fn needs_reasoning_content(&self) -> bool { + // Fast path: check model name patterns first (no network/cache dependency) + let lower = self.model_name.to_lowercase(); + if lower.contains("k2.5") + || lower.contains("k2-5") + || lower.contains("thinking") + || lower.contains("reasoning") + { + return true; + } + // Check ModelInfo reasoning flag from cache (lookup single model instead of cloning entire catalog) let reasoning_enabled = crate::api::lookup_model(&self.full_model_name) .await .map(|m| m.reasoning) .unwrap_or(false); - if !reasoning_enabled { - return false; - } - - // check model name patterns - let lower = self.model_name.to_lowercase(); - if lower.contains("k2.5") - || lower.contains("k2-5") - || lower.contains("thinking") - || lower.contains("reasoning") - { - return true; - } - false + reasoning_enabled }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 85 - 107, The heuristic check in needs_reasoning_content currently gates pattern detection behind the cache lookup (crate::api::lookup_model -> ModelInfo.reasoning), causing false negatives on cache miss; change the logic in needs_reasoning_content to evaluate the cheap model-name pattern first (inspect self.model_name.to_lowercase() for "k2.5", "k2-5", "thinking", "reasoning") and return true if matched, and only if the pattern does not match then consult crate::api::lookup_model(&self.full_model_name).await.map(|m| m.reasoning).unwrap_or(false) to decide the final result, so known reasoning models still require reasoning_content even when the catalog lookup fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/llm/model.rs`:
- Around line 85-107: The heuristic check in needs_reasoning_content currently
gates pattern detection behind the cache lookup (crate::api::lookup_model ->
ModelInfo.reasoning), causing false negatives on cache miss; change the logic in
needs_reasoning_content to evaluate the cheap model-name pattern first (inspect
self.model_name.to_lowercase() for "k2.5", "k2-5", "thinking", "reasoning") and
return true if matched, and only if the pattern does not match then consult
crate::api::lookup_model(&self.full_model_name).await.map(|m|
m.reasoning).unwrap_or(false) to decide the final result, so known reasoning
models still require reasoning_content even when the catalog lookup fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa94efbd-d0b3-428d-a694-b9087de11d88
📒 Files selected for processing (3)
src/api.rssrc/api/models.rssrc/llm/model.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api.rs
6f56874 to
b96e0a7
Compare
b96e0a7 to
13727d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/llm/model.rs (1)
85-92: Consider tightening the substring patterns to reduce false-positive risk.The
"k2.5"and"k2-5"patterns correctly target Kimi models. However,"thinking"and"reasoning"are generic and don't match any models in the current routing configuration (perdefaults_for_providerinrouting.rs). These could inadvertently match future models that don't actually needreasoning_content.If this is intentional future-proofing, it's fine since most APIs ignore unknown fields. If you want tighter matching, consider a provider check (e.g., only apply for moonshot/opencode providers) or remove the speculative patterns until specific models require them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/llm/model.rs` around lines 85 - 92, The needs_reasoning_content method currently matches generic substrings ("thinking" and "reasoning") that may produce false positives; update the implementation in needs_reasoning_content to tighten matching by either (A) removing the speculative "thinking" and "reasoning" checks and only keeping explicit Kimi patterns ("k2.5", "k2-5"), or (B) guard the generic checks with a provider check (e.g., ensure self.provider is Moonshot or Opencode as per defaults_for_provider in routing.rs) so only known providers apply the extra field; modify needs_reasoning_content accordingly to reference self.model_name and self.provider (or equivalent) and ensure behavior is consistent with defaults_for_provider in routing.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/llm/model.rs`:
- Around line 85-92: The needs_reasoning_content method currently matches
generic substrings ("thinking" and "reasoning") that may produce false
positives; update the implementation in needs_reasoning_content to tighten
matching by either (A) removing the speculative "thinking" and "reasoning"
checks and only keeping explicit Kimi patterns ("k2.5", "k2-5"), or (B) guard
the generic checks with a provider check (e.g., ensure self.provider is Moonshot
or Opencode as per defaults_for_provider in routing.rs) so only known providers
apply the extra field; modify needs_reasoning_content accordingly to reference
self.model_name and self.provider (or equivalent) and ensure behavior is
consistent with defaults_for_provider in routing.rs.
The tests added on main assert the baseline conversion behavior, which the false flag preserves; true is only for providers that require a reasoning_content field on every assistant message.
jamiepine
left a comment
jamiepine
left a comment
There was a problem hiding this comment.
Validated against repaired CI; fixed the test call sites for the new needs_reasoning_content flag as a maintainer edit.
2 participants
Summary
Fixes #245 - Moonshot AI models with thinking enabled fail with error: "thinking is enabled but reasoning_content is missing in assistant tool call message".
Root Cause
Moonshot AI models (kimi-k2.5, kimi-k2.5-nvfp4, etc.) that have
reasoning: truecapability require thereasoning_contentfield in assistant tool call messages when tool calls are present. Without this field, the API returns a validation error.The error occurred because
convert_messages_to_openai()was not adding thereasoning_contentfield to assistant messages containing tool calls for reasoning-capable models.Key Changes
Detect reasoning-capable models
Added
needs_reasoning_content()method inSpacebotModelthat:reasoningflag from the cached models listAdd reasoning_content to assistant tool call messages
Updated
convert_messages_to_openai()to:needs_reasoning_contentboolean parameter"reasoning_content": "."field to assistant messages when:tool_callsPass reasoning flag through all provider paths
Updated the OpenAI-compatible provider methods to:
needs_reasoning_content()before message conversionconvert_messages_to_openai()Files Changed
src/llm/model.rsneeds_reasoning_content()method, updatedconvert_messages_to_openai()signature and implementation, wired flag through OpenAI-compatible provider pathsTesting
just gate-prpasses (formatting, clippy, tests, compilation)opencode-go/kimi-k2.5as worker modelCore Fix
The fix adds the required
reasoning_contentfield to assistant tool call messages for Moonshot AI thinking-enabled models. When a model with reasoning capability sends tool calls, the API now receives the mandatoryreasoning_contentfield containing a placeholder value ("."), satisfying the validation requirements.Note
This fix addresses a Moonshot AI API requirement where reasoning-capable models must include the
reasoning_contentfield when sending tool calls. The solution detects reasoning-enabled models via the ModelInfo cache and model name patterns, then injects a placeholder value into assistant tool call messages. This ensures compatibility with Kimi models (kimi-k2.5 and variants) while remaining transparent to non-reasoning models.Written by Tembo for commit c590b65. This will update automatically on new commits.