fix(data): return content when llama3/mistral tool extractors find no calls#10592
fix(data): return content when llama3/mistral tool extractors find no calls#10592he-yufeng wants to merge 1 commit into
Conversation
… calls Llama3ToolUtils.tool_extractor and MistralToolUtils.tool_extractor returned the raw list comprehension, so a payload that parses to an empty list (e.g. "[]") yielded [] instead of the original content string. tool_extractor returns str when there are no function calls; the Qwen3.5/Seed/LFM2 extractors already guard this with 'return results if results else content' (Seed via hiyouga#10408). Apply the same guard to the llama3 and mistral extractors. Add tests asserting both return the content string for an empty tool array. Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request modifies the tool extractors in src/llamafactory/data/tool_utils.py to return the original content string instead of an empty list when no tool calls are extracted, and adds corresponding unit tests in tests/data/test_formatter.py. The review feedback correctly identifies potential TypeError vulnerabilities if the parsed tools are not dictionaries, as the current try-except block only catches KeyError. It is recommended to validate the structure of the tools before processing them.
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.
| results = [ | ||
| FunctionCall(tool["name"], json.dumps(tool["parameters"], ensure_ascii=False)) for tool in tools | ||
| ] |
There was a problem hiding this comment.
If tools contains elements that are not dictionaries (e.g., if the model outputs a list of integers or strings, or nested lists), accessing tool["name"] or tool["parameters"] will raise a TypeError (e.g., 'int' object is not subscriptable). Since the try-except block only catches KeyError, this will cause an unhandled exception and crash the extraction process.
To make this more robust and adhere to defensive programming practices, we should validate that each tool in tools is a dictionary and contains the required keys before attempting to construct the FunctionCall objects.
| results = [ | |
| FunctionCall(tool["name"], json.dumps(tool["parameters"], ensure_ascii=False)) for tool in tools | |
| ] | |
| if not all(isinstance(tool, dict) and "name" in tool and "parameters" in tool for tool in tools): | |
| return content | |
| results = [ | |
| FunctionCall(tool["name"], json.dumps(tool["parameters"], ensure_ascii=False)) for tool in tools | |
| ] |
| tools = [tools] if not isinstance(tools, list) else tools | ||
| try: | ||
| return [FunctionCall(tool["name"], json.dumps(tool["arguments"], ensure_ascii=False)) for tool in tools] | ||
| results = [FunctionCall(tool["name"], json.dumps(tool["arguments"], ensure_ascii=False)) for tool in tools] |
There was a problem hiding this comment.
Similarly, if tools contains elements that are not dictionaries, accessing tool["name"] or tool["arguments"] will raise a TypeError which is not caught by the except KeyError: block.
We should validate that each tool in tools is a dictionary and contains the required keys before attempting to construct the FunctionCall objects.
| results = [FunctionCall(tool["name"], json.dumps(tool["arguments"], ensure_ascii=False)) for tool in tools] | |
| if not all(isinstance(tool, dict) and "name" in tool and "arguments" in tool for tool in tools): | |
| return content | |
| results = [FunctionCall(tool["name"], json.dumps(tool["arguments"], ensure_ascii=False)) for tool in tools] |
What does this PR do?
tool_extractorreturns astr(the original content) when the assistant message contains no function calls, and alist[FunctionCall]otherwise.Llama3ToolUtils.tool_extractorandMistralToolUtils.tool_extractorreturned the list comprehension directly, so a payload that parses to an empty list (e.g."[]") produced[]instead of the content string:The Qwen3.5, Seed, and LFM2 extractors already guard this with
return results if results else content(Seed via #10408). This PR applies the same guard to the llama3 and mistral extractors so they stay consistent with the documentedstr | list[FunctionCall]contract.Tests
Added
test_llama3_tool_extractor_empty_returns_contentandtest_mistral_tool_extractor_empty_returns_contentintests/data/test_formatter.py, assertingToolFormatter(tool_format=...).extract("[]") == "[]". Both fail before the change (they return[]) and pass after.