feat: add MiMo V2 Pro reasoning support via OpenRouter#67
feat: add MiMo V2 Pro reasoning support via OpenRouter#67
Conversation
Adds reasoning_details round-trip for OpenRouter's MiMo V2 Pro in the same style as the existing reasoning_content path for DeepSeek-R1/Kimi. Request-side reasoning is passed via extra_body so litellm.drop_params can't silently strip it, and _apply_model_overrides now consults the active gateway spec so registry overrides fire for gateway-routed models (previously a latent dead-code bug for any override attached to OpenRouter or AiHubMix).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds an optional Changes
Sequence DiagramsequenceDiagram
participant Client as Client/TTY
participant AgentLoop as Agent Loop
participant Provider as LiteLLM Provider
participant LLM as LLM Service
participant Context as Context Builder
Client->>AgentLoop: send user input
AgentLoop->>Provider: request completion (model, overrides)
Provider->>Provider: apply overrides (_merge_overrides / gateway)
Provider->>LLM: send request (includes extra_body.reasoning)
LLM-->>Provider: response (content, reasoning_content?, reasoning_details?)
Provider->>Provider: _parse_response -> extract/synthesize reasoning_content and reasoning_details
Provider-->>AgentLoop: return LLMResponse (content + reasoning_details)
AgentLoop->>Context: add_assistant_message(content, reasoning_details)
Context->>Context: store assistant message with reasoning_details
AgentLoop-->>Client: present assistant content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/test_mimo.py (1)
20-20: Remove unused import.The
ContextBuilderimport is marked as unused (noqa: F401) with a comment claiming it "mirrors prod code path", but this script doesn't exercise anyContextBuilderfunctionality. The import can be safely removed.🔧 Suggested fix
-from pocketfox.agent.context import ContextBuilder # noqa: F401 (unused direct, but mirrors prod code path)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test_mimo.py` at line 20, Remove the unused import of ContextBuilder in scripts/test_mimo.py: delete the line "from pocketfox.agent.context import ContextBuilder # noqa: F401" (and the trailing noqa comment) since the script does not use ContextBuilder; verify there are no references to ContextBuilder elsewhere in the file and run tests/lint to ensure no remaining unused-import warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/test_mimo.py`:
- Line 20: Remove the unused import of ContextBuilder in scripts/test_mimo.py:
delete the line "from pocketfox.agent.context import ContextBuilder # noqa:
F401" (and the trailing noqa comment) since the script does not use
ContextBuilder; verify there are no references to ContextBuilder elsewhere in
the file and run tests/lint to ensure no remaining unused-import warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: daaa7f8b-cfa3-45b0-bc94-23fd6b849b1e
📒 Files selected for processing (8)
pocketfox/agent/context.pypocketfox/agent/loop.pypocketfox/cli/tty.pypocketfox/providers/base.pypocketfox/providers/litellm_provider.pypocketfox/providers/registry.pyscripts/test_mimo.pytests/test_litellm_reasoning.py
| def _merge_overrides(kwargs: dict[str, Any], overrides: dict[str, Any]) -> None: | ||
| """Merge registry overrides into kwargs, deep-merging extra_body.""" | ||
| for key, val in overrides.items(): | ||
| if key == "extra_body" and isinstance(kwargs.get("extra_body"), dict) and isinstance(val, dict): |
There was a problem hiding this comment.
This looks fishy, why can we have a dict or a non-dict here, and have to fall back on RTTI?
blue-duval
left a comment
blue-duval
left a comment
There was a problem hiding this comment.
Solid PR. The core approach is clean — reasoning_details verbatim round-trip plus synthesized reasoning_content for backwards compatibility. The model_extra fallback for LiteLLM's drop_params behavior is a nice detail.
What I like:
_merge_overrideswith deep merge forextra_body— elegant solution to the drop_params problem- Gateway-based override fix (
_apply_model_overridesnow consultsself._gateway) — good that this came along as a secondary benefit - Tests are thorough and mock cleanly
scripts/test_mimo.pyis well-documented with clear exit codes
Minor things:
-
_apply_model_overridesearly return: The loop breaks after the first pattern match (return). Iffind_by_modelandself._gatewaymatched the same pattern, the first wins. Currently harmless becausefind_by_modelreturnsNonefor gateway models — but if that changes, it's a silent bug. Worth a comment at minimum. -
Synthesis with mixed segment types:
reasoning_detailscould theoretically contain differenttypevalues (not justreasoning.text). The synthesis logic concatenates everything with.get("text", "")— works, but if OpenRouter ever sends non-text segments, empty strings end up in the synthesis. Probably not an issue today. -
Docstring coverage: CodeRabbit is right to flag this —
_merge_overridesand_fake_responsehave docstrings, but the new test functions don't. I know test functions are self-explanatory, but if the coverage rule wants 80%... -
Minor in
test_merge_overrides_non_extra_body_keys_overwrite: The test checks shallow overwrite, but_merge_overridesdoeskwargs[key] = valfor non-extra_bodykeys — that's an override, not a merge. Test name is correct, but in the context of the "deep merge" tests it could be confusing. Very minor.
Overall: LGTM after gustos. The docstring thing is the only thing CodeRabbit blocks — whether you fix that or dismiss the check is your call. 👍
2 participants
Summary
LLMResponse.reasoning_detailsand threads it throughadd_assistant_message→loop.py/tty.pyso OpenRouter's MiMo V2 Pro reasoning segments round-trip verbatim in multi-turn historyextra_body={"reasoning": {"enabled": True}}through a new OpenRouter registry override —extra_bodybypasseslitellm.drop_params=True, which would otherwise silently strip the unknown kwarg_apply_model_overrides: it only consultedfind_by_model(), which skips gateways, so anymodel_overridesattached to OpenRouter or AiHubMix was dead code. It now also consultsself._gatewayFiles
pocketfox/providers/base.py— newreasoning_detailsfield onLLMResponsepocketfox/providers/litellm_provider.py—_merge_overrideshelper, gateway-aware override lookup,reasoning_detailsextraction withmodel_extrafallback, synthesizesreasoning_contentfrom details when server populates details-onlypocketfox/providers/registry.py— OpenRouter spec gains("xiaomi/mimo", {"extra_body": {"reasoning": {"enabled": True}}})pocketfox/agent/context.py—add_assistant_messagegainsreasoning_detailsparampocketfox/agent/loop.py+pocketfox/cli/tty.py— forwardresponse.reasoning_detailstests/test_litellm_reasoning.py— 9 new unit testsscripts/test_mimo.py— manual end-to-end smoke test (requires `OPENROUTER_API_KEY`)Test plan
Notes
The gateway-override fix is a secondary benefit — it unblocks any future per-model override on OpenRouter/AiHubMix, not just MiMo.
Summary by CodeRabbit
New Features
Tests
Chores