Repository navigation
fix(tool_parser): parse all parallel invokes in a minimax_m2 wrapper - #1824
Conversation
…l wrapper MiniMax M2 emits parallel tool calls as multiple <invoke> blocks inside a single <minimax:tool_call> wrapper (Anthropic-style). The parser extracted only the first: non-streaming used invoke_extractor.captures() (first match), and streaming jumped to the wrapper-end after one </invoke>, discarding the rest. This zeroed the BFCL parallel/parallel_multiple/live_parallel categories on the SMG arm (it returned 1 call where vLLM returned N). Non-streaming now iterates captures_iter over every <invoke> in the wrapper; streaming resets per-invoke state after each </invoke> and advances the tool index, closing the wrapper only when </minimax:tool_call> precedes the next <invoke>. Adds non-streaming + streaming parallel-in-one-wrapper tests. Signed-off-by: key4ng <rukeyang@gmail.com>
📝 WalkthroughWalkthroughThe MiniMax M2 tool-call parser is refactored to support multiple parallel ChangesMiniMax M2 parallel invoke parsing
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
tests
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean fix. The → change for non-streaming and the state-machine rework for streaming both look correct. Traced through the parallel-invoke streaming path (single chunk, split chunks, wrapper-close edge cases) and the tool_index accounting is sound. Tests cover the key cases well.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the MinimaxM2Parser to support parsing and streaming multiple parallel <invoke> blocks within a single <minimax:tool_call> wrapper. It refactors parse_tool_call to return a list of tool calls using captures_iter, simplifies the parser's state machine by removing the waiting_for_tool_call_end state, and adds unit tests to verify both complete and incremental parsing of parallel tool calls. There are no review comments, so no feedback is provided.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2b49b218b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.function_name_sent = false; | ||
| self.current_function_name.clear(); | ||
| self.current_parameters.clear(); | ||
| self.current_tool_id += 1; | ||
| continue; |
There was a problem hiding this comment.
When a streaming delta contains two <invoke> blocks in the same <minimax:tool_call> wrapper, this new path advances to the next invoke only after parse_and_stream_parameters(&buffer_copy, tools) has already scanned every <parameter> tag still in self.buffer. That means parameters from the second invoke are emitted on the first tool call (overwriting same-named args like location), and then the second tool is parsed again with its own args. Slice parameter parsing to the text before the first </invoke> before resetting state and continuing to the next invoke.
Useful? React with 👍 / 👎.
![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
🤖 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 `@crates/tool_parser/tests/tool_parser_minimax_m2.rs`:
- Around line 276-294: The streaming test in the chunk parsing loop should
accumulate not just tool names but also the streamed parameters from each
invoke. Modify the test to track the parameters field from each call in
result.calls alongside the names vector, then add assertions after the loop to
verify that the first invoke's parameters contain Beijing and the second
invoke's parameters contain Shanghai, which will catch any cross-contamination
of arguments between the two invoke slots.
🪄 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: f8a7b262-eede-4ad8-8f52-e06114b14e59
📒 Files selected for processing (2)
crates/tool_parser/src/parsers/minimax_m2.rscrates/tool_parser/tests/tool_parser_minimax_m2.rs
| let mut names = Vec::new(); | ||
| let mut max_index = 0usize; | ||
| for chunk in chunks { | ||
| let result = parser.parse_incremental(chunk, &tools).await.unwrap(); | ||
| for call in result.calls { | ||
| if let Some(name) = call.name { | ||
| names.push(name); | ||
| } | ||
| max_index = max_index.max(call.tool_index); | ||
| } | ||
| } | ||
|
|
||
| assert_eq!( | ||
| names.len(), | ||
| 2, | ||
| "both invokes must stream a tool name, got {names:?}" | ||
| ); | ||
| assert_eq!(max_index, 1, "second invoke must use tool_index 1"); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert streamed argument content per invoke.
The streaming test only checks tool names and max_index. Since the regression being fixed is about chained invokes within one wrapper, accumulating each invoke's streamed parameters and asserting the first carries Beijing and the second Shanghai would catch argument cross-contamination between the two <invoke> slots, not just the count.
💚 Proposed test strengthening
let mut names = Vec::new();
+ let mut args = vec![String::new(), String::new()];
let mut max_index = 0usize;
for chunk in chunks {
let result = parser.parse_incremental(chunk, &tools).await.unwrap();
for call in result.calls {
if let Some(name) = call.name {
names.push(name);
}
+ args[call.tool_index].push_str(&call.parameters);
max_index = max_index.max(call.tool_index);
}
}
assert_eq!(
names.len(),
2,
"both invokes must stream a tool name, got {names:?}"
);
assert_eq!(max_index, 1, "second invoke must use tool_index 1");
+ assert!(args[0].contains("Beijing"), "first invoke args: {}", args[0]);
+ assert!(args[1].contains("Shanghai"), "second invoke args: {}", args[1]);
}📝 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.
| let mut names = Vec::new(); | |
| let mut max_index = 0usize; | |
| for chunk in chunks { | |
| let result = parser.parse_incremental(chunk, &tools).await.unwrap(); | |
| for call in result.calls { | |
| if let Some(name) = call.name { | |
| names.push(name); | |
| } | |
| max_index = max_index.max(call.tool_index); | |
| } | |
| } | |
| assert_eq!( | |
| names.len(), | |
| 2, | |
| "both invokes must stream a tool name, got {names:?}" | |
| ); | |
| assert_eq!(max_index, 1, "second invoke must use tool_index 1"); | |
| } | |
| let mut names = Vec::new(); | |
| let mut args = vec![String::new(), String::new()]; | |
| let mut max_index = 0usize; | |
| for chunk in chunks { | |
| let result = parser.parse_incremental(chunk, &tools).await.unwrap(); | |
| for call in result.calls { | |
| if let Some(name) = call.name { | |
| names.push(name); | |
| } | |
| args[call.tool_index].push_str(&call.parameters); | |
| max_index = max_index.max(call.tool_index); | |
| } | |
| } | |
| assert_eq!( | |
| names.len(), | |
| 2, | |
| "both invokes must stream a tool name, got {names:?}" | |
| ); | |
| assert_eq!(max_index, 1, "second invoke must use tool_index 1"); | |
| assert!(args[0].contains("Beijing"), "first invoke args: {}", args[0]); | |
| assert!(args[1].contains("Shanghai"), "second invoke args: {}", args[1]); | |
| } |
🤖 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 `@crates/tool_parser/tests/tool_parser_minimax_m2.rs` around lines 276 - 294,
The streaming test in the chunk parsing loop should accumulate not just tool
names but also the streamed parameters from each invoke. Modify the test to
track the parameters field from each call in result.calls alongside the names
vector, then add assertions after the loop to verify that the first invoke's
parameters contain Beijing and the second invoke's parameters contain Shanghai,
which will catch any cross-contamination of arguments between the two invoke
slots.
2 participants
Description
Problem
On the nightly BFCL A/B, MiniMax-M2.7 scored 0 on the SMG arm for every parallel category —
parallel,parallel_multiple,live_parallel,live_parallel_multiple— while pure vLLM scored normally (e.g.live_parallel75 → 0). Per-case dumps showed SMG returned 1 tool call where vLLM returned 2 (16/16 failed live_parallel cases returned ≤1 call), failing BFCL'swrong_countcheck.Root cause: MiniMax M2 emits parallel calls as multiple
<invoke>blocks inside one<minimax:tool_call>wrapper (Anthropic-style), butminimax_m2.rsonly extracted the first:invoke_extractor.captures(block)returns only the first match.</invoke>it jumped straight to the wrapper-end (waiting_for_tool_call_end), discarding any later<invoke>in the same wrapper.(vLLM's reference parser does
invoke_complete_regex.findall(text)— all invokes.)Solution
parse_tool_callnow iteratescaptures_iterover every<invoke>in the wrapper and returns aVec<ToolCall>;parse_tool_calls_from_textextends with all of them.</invoke>, reset per-invoke state and advance the tool index, then either parse the next<invoke>in the wrapper or close the wrapper when</minimax:tool_call>precedes the next<invoke>. Removed the now-unusedwaiting_for_tool_call_endstate.Changes
crates/tool_parser/src/parsers/minimax_m2.rs— multi-invoke extraction (non-streaming + streaming).crates/tool_parser/tests/tool_parser_minimax_m2.rs— addtest_minimax_parallel_invokes_in_one_wrapper(non-streaming) andtest_minimax_streaming_parallel_invokes_in_one_wrapper(streaming).Test Plan
cargo test -p tool-parser— all pass (33 in the minimax suite incl. the 2 new; full crate green).cargo +nightly fmt/cargo clippy -p tool-parser --all-targets— clean.Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Bug Fixes
Tests