Skip to content

feat(provider): preserve reasoning_details across turns and configure MiniMax reasoning_split via registry#1438

Open
mengxr1234 wants to merge 6 commits intoHKUDS:mainfrom
mengxr1234:feat/minimax-reasoning-details
Open

feat(provider): preserve reasoning_details across turns and configure MiniMax reasoning_split via registry#1438
mengxr1234 wants to merge 6 commits intoHKUDS:mainfrom
mengxr1234:feat/minimax-reasoning-details

Conversation

@mengxr1234
Copy link
Copy Markdown

Summary

This PR improves reasoning metadata continuity across multi-turn conversations and configures MiniMax interleaved reasoning in a provider-configurable way.

Changes

  • Added reasoning_details to provider response model.
  • Preserved reasoning_details in assistant message history for:
    • main agent loop
    • subagent loop
  • Allowed reasoning_details through message sanitization in LiteLLM provider.
  • Parsed reasoning_details from LiteLLM response (message and provider-specific fields fallback).
  • Moved MiniMax reasoning_split enablement to provider registry model_overrides for:
    • minimax-m2.5
    • minimax-m2.1
    • minimax-m2

Why

  • Prevent loss of reasoning metadata between turns.
  • Keep provider-specific behavior declarative in registry instead of hardcoding in request flow.
  • Improve maintainability and extensibility for future provider-specific options.

Validation

  • Ran Python compile checks on touched core files.
  • Verified branch contains only 6 changed files relevant to this feature.

…niMax reasoning_split via registry overrides
Copy link
Copy Markdown
Collaborator

@chengyongru chengyongru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR addresses a real need (preserving reasoning metadata across turns), but has some issues that should be addressed before merging.


Issues

1. reasoning_split Parameter Passing Method (Critical)

According to litellm MiniMax documentation, reasoning_split should be passed via extra_body, not directly in kwargs:

# From litellm docs:
stream = client.chat.completions.create(
    model="minimax/MiniMax-M2.1",
    messages=[...],
    extra_body={"reasoning_split": True},  # <-- via extra_body
    stream=True,
)

However, this PR adds it directly to kwargs via model_overrides:

model_overrides=(
    ("minimax-m2.5", {"reasoning_split": True}),
    ...
)

And _apply_model_overrides does:

kwargs.update(overrides)  # adds reasoning_split directly to kwargs

Since litellm.drop_params = True is set in LiteLLMProvider.__init__, the reasoning_split parameter will likely be dropped silently because it's not a recognized litellm parameter.

Recommendation: Either:

  1. Add support for extra_body in _apply_model_overrides
  2. Or verify that litellm actually accepts reasoning_split as a direct kwarg (I couldn't find evidence of this)

2. Code Duplication in subagent.py

The subagent loop directly constructs the assistant message dict instead of using ContextBuilder.add_assistant_message():

# subagent.py (new code)
{
    "role": "assistant",
    "content": response.content or "",
    "tool_calls": tool_call_dicts,
    **({"reasoning_content": response.reasoning_content} if response.reasoning_content is not None else {}),
    **({"thinking_blocks": response.thinking_blocks} if response.thinking_blocks else {}),
    **({"reasoning_details": response.reasoning_details} if response.reasoning_details is not None else {}),
}

vs:

# loop.py (existing code)
messages = self.context.add_assistant_message(
    messages, response.content, tool_call_dicts,
    reasoning_content=response.reasoning_content,
    thinking_blocks=response.thinking_blocks,
    reasoning_details=response.reasoning_details,
)

This creates inconsistency and potential bugs if add_assistant_message is updated in the future.

Recommendation: Refactor subagent.py to use add_assistant_message for consistency, or extract the logic into a shared helper function.

3. Missing Tests

No test code is included to verify:

  • reasoning_details is correctly preserved across turns
  • model_overrides correctly applies reasoning_split to MiniMax models
  • The feature works end-to-end with actual MiniMax API

Minor Suggestions

  • Consider adding a comment explaining what reasoning_details contains (MiniMax interleaved CoT metadata per the field comment)
  • The model_overrides pattern matching (pattern in model_lower) is case-insensitive but substring-based - this could accidentally match unintended models

Verification Needed

Before merging, please verify:

  1. Does reasoning_split=True actually reach the MiniMax API when passed directly in kwargs?
  2. Does the response actually contain reasoning_details in the expected format?

You can test by adding a debug log in _parse_response:

if reasoning_details:
    logger.debug(f"Got reasoning_details: {reasoning_details}")

@mengxr1234
Copy link
Copy Markdown
Author

Thanks for the review — addressed all requested points.

What I changed

  1. Switched MiniMax reasoning_split injection to extra_body={"reasoning_split": true} (instead of direct kwargs).
  2. Updated model override application to merge extra_body safely.
  3. Removed assistant-message duplication in subagent.py by reusing shared assistant message construction from ContextBuilder.
  4. Added focused tests for MiniMax override behavior and extra_body merge.

Validation

  • pytest -q tests/test_context_prompt_cache.py -k "reasoning_split or extra_body" passed
  • pytest -q tests/test_commands.py -k "sanitize_messages_keeps_reasoning_details" passed
  • python -m py_compile on touched files passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants