Add MiniMax-M3 and MiniMax-M2.7 to the LLM model picker - #285
Open
octo-patch wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe change adds MiniMax-M3 and MiniMax-M2.7 to the supported model list. Both models disable explicit reasoning settings and strip think tags. Tests verify registration, configuration, cleaned summaries, and transcript preservation. ChangesMiniMax model support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Tests/AppContextServiceTests.swift`:
- Around line 78-97: Update testMiniMaxModelsArePredefinedAndStripThinkTags to
run AppContextService.activitySummary for each MiniMax model, asserting hidden
reasoning is removed and cleaned transcript text remains for both MiniMax-M3 and
MiniMax-M2.7. Within the same model loop, also assert each configuration’s
reasoningEffort and includeReasoning are nil.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 70417265-ab57-47ac-8e37-aab73c0aedd1
📒 Files selected for processing (2)
Sources/ModelConfiguration.swiftTests/AppContextServiceTests.swift
Comment on lines
+78
to
+97
| private static func testMiniMaxModelsArePredefinedAndStripThinkTags() { | ||
| expect(ModelConfiguration.llmModels.contains("MiniMax-M3"), "MiniMax-M3 is missing from the picker") | ||
| expect(ModelConfiguration.llmModels.contains("MiniMax-M2.7"), "MiniMax-M2.7 is missing from the picker") | ||
|
|
||
| for model in ["MiniMax-M3", "MiniMax-M2.7"] { | ||
| let config = ModelConfiguration.config(for: model) | ||
| expect(config.shouldStripThinkTags, "MiniMax model should strip think tags: \(model)") | ||
| } | ||
|
|
||
| let reasoningOutput = """ | ||
| <think> | ||
| Hidden reasoning should never reach the transcript. | ||
| </think> | ||
| Cleaned transcript text. | ||
| """ | ||
| let summary = AppContextService.activitySummary(from: reasoningOutput, model: "MiniMax-M3") | ||
| expect(summary?.contains("Hidden reasoning") == false, "MiniMax reasoning leaked into summary") | ||
| expect(summary?.contains("Cleaned transcript text.") == true, "MiniMax summary dropped the transcript text") | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover both MiniMax models in the cleanup assertions.
The loop checks shouldStripThinkTags for both models, but AppContextService.activitySummary runs only for MiniMax-M3. A MiniMax-M2.7 cleanup regression could pass the test. Also assert that reasoningEffort and includeReasoning are nil for both models, because those fields are part of the new configuration contract.
Proposed test adjustment
+ let reasoningOutput = """
+ <think>
+ Hidden reasoning should never reach the transcript.
+ </think>
+ Cleaned transcript text.
+ """
+
for model in ["MiniMax-M3", "MiniMax-M2.7"] {
let config = ModelConfiguration.config(for: model)
expect(config.shouldStripThinkTags, "MiniMax model should strip think tags: \(model)")
- }
-
- let reasoningOutput = """
- <think>
- Hidden reasoning should never reach the transcript.
- </think>
- Cleaned transcript text.
- """
- let summary = AppContextService.activitySummary(from: reasoningOutput, model: "MiniMax-M3")
- expect(summary?.contains("Hidden reasoning") == false, "MiniMax reasoning leaked into summary")
- expect(summary?.contains("Cleaned transcript text.") == true, "MiniMax summary dropped the transcript text")
+ expect(config.reasoningEffort == nil, "MiniMax model should not set reasoning effort: \(model)")
+ expect(config.includeReasoning == nil, "MiniMax model should not include reasoning: \(model)")
+
+ let summary = AppContextService.activitySummary(from: reasoningOutput, model: model)
+ expect(summary?.contains("Hidden reasoning") == false, "MiniMax reasoning leaked into summary: \(model)")
+ expect(summary?.contains("Cleaned transcript text.") == true, "MiniMax summary dropped transcript text: \(model)")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static func testMiniMaxModelsArePredefinedAndStripThinkTags() { | |
| expect(ModelConfiguration.llmModels.contains("MiniMax-M3"), "MiniMax-M3 is missing from the picker") | |
| expect(ModelConfiguration.llmModels.contains("MiniMax-M2.7"), "MiniMax-M2.7 is missing from the picker") | |
| for model in ["MiniMax-M3", "MiniMax-M2.7"] { | |
| let config = ModelConfiguration.config(for: model) | |
| expect(config.shouldStripThinkTags, "MiniMax model should strip think tags: \(model)") | |
| } | |
| let reasoningOutput = """ | |
| <think> | |
| Hidden reasoning should never reach the transcript. | |
| </think> | |
| Cleaned transcript text. | |
| """ | |
| let summary = AppContextService.activitySummary(from: reasoningOutput, model: "MiniMax-M3") | |
| expect(summary?.contains("Hidden reasoning") == false, "MiniMax reasoning leaked into summary") | |
| expect(summary?.contains("Cleaned transcript text.") == true, "MiniMax summary dropped the transcript text") | |
| } | |
| private static func testMiniMaxModelsArePredefinedAndStripThinkTags() { | |
| expect(ModelConfiguration.llmModels.contains("MiniMax-M3"), "MiniMax-M3 is missing from the picker") | |
| expect(ModelConfiguration.llmModels.contains("MiniMax-M2.7"), "MiniMax-M2.7 is missing from the picker") | |
| let reasoningOutput = """ | |
| <think> | |
| Hidden reasoning should never reach the transcript. | |
| </think> | |
| Cleaned transcript text. | |
| """ | |
| for model in ["MiniMax-M3", "MiniMax-M2.7"] { | |
| let config = ModelConfiguration.config(for: model) | |
| expect(config.shouldStripThinkTags, "MiniMax model should strip think tags: \(model)") | |
| expect(config.reasoningEffort == nil, "MiniMax model should not set reasoning effort: \(model)") | |
| expect(config.includeReasoning == nil, "MiniMax model should not include reasoning: \(model)") | |
| let summary = AppContextService.activitySummary(from: reasoningOutput, model: model) | |
| expect(summary?.contains("Hidden reasoning") == false, "MiniMax reasoning leaked into summary: \(model)") | |
| expect(summary?.contains("Cleaned transcript text.") == true, "MiniMax summary dropped transcript text: \(model)") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Tests/AppContextServiceTests.swift` around lines 78 - 97, Update
testMiniMaxModelsArePredefinedAndStripThinkTags to run
AppContextService.activitySummary for each MiniMax model, asserting hidden
reasoning is removed and cleaned transcript text remains for both MiniMax-M3 and
MiniMax-M2.7. Within the same model loop, also assert each configuration’s
reasoningEffort and includeReasoning are nil.
6 tasks
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
1 participant
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.
Reason: Add MiniMax-M3 and MiniMax-M2.7 to the predefined LLM model picker.
Changes
MiniMax-M3andMiniMax-M2.7toModelConfiguration.llmModelsso they appear in the post-processing, fallback, and context model dropdowns instead of requiring manual entry.ModelConfiguration.config(for:)withshouldStripThinkTags: true, so their reasoning/think blocks are stripped from cleaned transcripts the same way other reasoning models are handled.Why strip think tags
Both models emit reasoning content (MiniMax-M3 adaptively, MiniMax-M2.7 always on). Without stripping, the reasoning text would leak into the pasted transcript. FreeFlow already strips think tags for other reasoning models via
ModelConfiguration.stripThinkTags, so the new entries reuse that existing path.Checks
The repo builds with
swiftcon macOS (seeMakefile); no Swift toolchain is available in this environment, so the change was validated by static checks: brace/paren balance, and the added test asserting the models are predefined and that think tags are stripped.A new test
testMiniMaxModelsArePredefinedAndStripThinkTagswas added toTests/AppContextServiceTests.swiftcovering:ModelConfiguration.llmModels,config(for:)returnsshouldStripThinkTags == truefor both,Summary by CodeRabbit
New Features
Tests