Skip to content

[data] fix MiniMax tool_extractor to return content when no tool call parses#10599

Open
he-yufeng wants to merge 1 commit into
hiyouga:mainfrom
he-yufeng:fix/minimax-tool-extractor-content-v2
Open

[data] fix MiniMax tool_extractor to return content when no tool call parses#10599
he-yufeng wants to merge 1 commit into
hiyouga:mainfrom
he-yufeng:fix/minimax-tool-extractor-content-v2

Conversation

@he-yufeng

Copy link
Copy Markdown

What does this PR do?

MiniMaxM1ToolUtils.tool_extractor and MiniMaxM2ToolUtils.tool_extractor returned an empty list when the wrapper tag (<tool_calls> / <minimax:tool_call>) was present but nothing parsed inside it — M1 skips bad-JSON lines, M2 finds no <invoke> block. An empty list silently drops the original assistant content downstream.

The sibling extractors SeedToolUtils, Qwen35ToolUtils and LFM2ToolUtils already guard this with return results if results else content. This applies the same guard to MiniMax M1/M2 so a wrapped-but-unparseable response keeps its content instead of vanishing.

Added tests/data/test_tool_utils.py covering the no-valid-call and empty-wrapper paths for both extractors, plus a valid-call case to confirm normal parsing is unchanged.

Verification

PYTHONPATH=src pytest tests/data/test_tool_utils.py — 5 passed (the four no-call assertions fail before this change, pass after). ruff check and ruff format --check clean.

… parses

MiniMaxM1/M2 tool_extractor returned an empty list when the wrapper tag was
present but nothing parsed (M1 skips bad-JSON lines, M2 finds no <invoke> block),
silently dropping the original content. The sibling extractors (Seed / Qwen3.5 /
LFM2) already return content in that case; this aligns MiniMax with them and adds
unit tests for the no-call and empty-wrapper paths.

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request updates the tool extractors for MiniMaxM1 and MiniMaxM2 to return the original content as a fallback when no valid tool calls are parsed, and adds corresponding unit tests. The review feedback points out a potential vulnerability to runtime crashes (such as KeyError or TypeError) in MiniMaxM1ToolUtils.tool_extractor if the parsed JSON does not contain the expected keys, and suggests expanding the exception handling to make the parser more robust.

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.

continue

return results
return results if results else content

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

While this fallback improvement is great, MiniMaxM1ToolUtils.tool_extractor is vulnerable to runtime crashes if the content inside <tool_calls> is valid JSON but does not match the expected structure (e.g., it is missing the "name" or "arguments" keys, or is not a dictionary). For example, if the model outputs <tool_calls> {"invalid_key": "value"} </tool_calls>, json.loads will succeed, but accessing tool_call["name"] or tool_call["arguments"] will raise a KeyError (or TypeError if it's a list/string), crashing the extraction. Consider catching (json.JSONDecodeError, KeyError, TypeError) in the try-except block to make the parser more robust.

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.

1 participant