Repository navigation
Python: fix(tools): answer a function call whose arguments are not a JSON object - #9573
H23 (Madin-H23) wants to merge 5 commits into
Conversation
…JSON object Closes microsoft#9562 Content.parse_arguments() wraps a payload that is not a JSON object under a single "raw" key. That is the right answer for a caller that wants to see what the model sent, but the function-calling loop used it as the arguments of the call, so a model that answered with something that is not a JSON object had its tool invoked with an argument the tool never declared, and the model was never told that its call was malformed. The loop now parses the payload strictly and answers a call whose arguments are not a JSON object the way it already answers a missing required property: with an argument-validation error result that names the cause, so the model can retry. parse_arguments() keeps its documented behaviour for direct callers.
agent-framework-automation
Bot
added
the
python
There was a problem hiding this comment.
🟡 Changes recommended
Whitespace-only payloads still bypass validation and reach tools as bogus raw arguments.
1 open issue
What changed in this PR
Addresses #9562 by rejecting malformed function-call arguments in the execution loop while preserving the public Content.parse_arguments() behavior.
Changes:
- Adds strict parsing through the existing argument-validation error path.
- Adds regression tests for malformed payloads and valid object arguments.
| File | Description |
|---|---|
| python/packages/core/tests/core/test_tools.py | Tests malformed argument rejection and valid tool execution. |
| python/packages/core/agent_framework/_tools.py | Validates argument payloads before invoking tools. |
Review effort: Balanced
| model can retry it. | ||
| """ | ||
| arguments = function_call.arguments | ||
| if isinstance(arguments, str) and arguments.strip(): |
The strict parse skipped a payload that was only whitespace, so such a call still reached the tool as a bogus "raw" argument. A payload that is not empty has to be a JSON object, and whitespace is not one.
|
Good catch — the whitespace-only payload slipped through the strict parse, and the tool was still invoked with a bogus The parse skipped any payload whose stripped form was empty, so a call whose arguments were only whitespace took the lenient path. A payload that is not empty has to be a JSON object, and whitespace is not one, so the check now applies to every non-empty payload. The malformed-argument test is parametrized over the shapes from the issue and now covers the whitespace-only payload as well. It fails against the first commit of this pull request — the tool is invoked with
|
Somay kousis (Somay-kousis)
left a comment
Somay kousis (Somay-kousis)
left a comment
There was a problem hiding this comment.
Reviewed at 84de7d5 (merge base 08851e6).
What I ran
- New tests on the PR head: 5 passed.
- Same tests with
_tools.pyreverted to the merge base: all 4 malformed cases fail (tool gets called), the well-formed case passes. So the tests catch the bug. tests/core/test_tools.pyon head: 156 passed.tests/coreon head: 6792 passed, 133 skipped.ruff checkandruff format --check(0.17.0) on both changed files: clean.- A probe script through
_try_execute_function_call_groups, head vs merge base, with inputs the tests don't cover.
The fix does what the issue asks. A tool with a permissive schema no longer runs with raw=..., and Content.parse_arguments() keeps its public fallback. One behaviour change for existing tools is worth a decision before merge.
1. Zero-parameter tools that used to run now fail on "null" and whitespace (worth considering)
_tools.py:2178-2183 rejects every non-empty string that does not decode to a dict. That also catches typed tools with no parameters. On the merge base they ran fine, because the extra raw key was dropped by the tool's input model:
@tool
def now() -> str: ...| arguments | merge base | PR head |
|---|---|---|
"" |
runs | runs |
"{}" |
runs | runs |
"null" |
runs | Error: Argument parsing failed. |
" " |
runs | Error: Argument parsing failed. |
"not {json" |
runs | Error: Argument parsing failed. |
So "" means "no arguments" but " " and "null" are errors. That costs the model an extra round trip, and with max_iterations low it could end the run without the tool result. I have not confirmed that a provider sends "null" or whitespace for a no-arg call; that part is a suspicion. If you want to keep the strict check, one option is to treat null and whitespace-only payloads like "" (empty arguments) and reject only payloads that decode to something else or don't decode at all. Your call. It is the opposite of the earlier bot suggestion, so a maintainer may want to weigh in.
2. Approval-required tools still ask the human first (nit)
With approval_mode="always_require" and arguments="[1,2]", the loop still emits a function_approval_request carrying [1,2]. After the human approves, _auto_invoke_function returns Error: Argument parsing failed. (probe output in the evidence). Nothing unsafe happens, but the user approves a call that can never run. The check could also run where approval requests are built. A test for this path would help either way.
3. Malformed calls skip function middleware (nit, by design?)
The new check at _tools.py:2267 sits before the middleware branch at _tools.py:2304. Middleware that logs or audits tool calls no longer sees these calls. On the merge base it saw them with {"raw": ...}. Fine if intended. Worth one line in the docstring.
4. Description vs default config (nit)
The description says the error "names the cause, so the model can retry". With the default include_detailed_errors=False, the model gets only Error: Argument parsing failed.. The cause shows up only in exception or with detailed errors on. This matches the existing missing-required path, so the code is fine. The wording just oversells it a little.
Not checked
Real provider payloads (OpenAI, Azure, Ollama, vLLM), the streaming path end to end, the MCP path in _mcp.py:1529 (it still uses parse_arguments()), pyright/mypy, and anything outside packages/core.
|
Thank you — this is the most useful review we have had on this pull request, and the first point in particular is a real finding. We verified each item against the code at 1. Zero-parameter tools and Your table matches the code. The guard is We keep the strict rule for two reasons. First, it is what the issue asks for: "Arguments that are not a JSON object are invalid arguments", answered the way a missing required property is answered. Second, your own observation decides it: you did not find a provider that sends Your alternative is reasonable, and we would take it if a maintainer prefers it — under it, no payload can still reach a tool carrying an undeclared 2. The approval path — agreed, this is a gap and we will fix it Confirmed: the check runs inside 3. Middleware no longer sees malformed calls — intended, and we will say so in the code Confirmed: the check sits before the middleware branch, so a logging or auditing middleware no longer records a malformed payload as a tool call. That is deliberate — a payload that is not arguments is not a tool call, and we do not want it counted as one. We will put that in the docstring so the next reader does not have to rediscover it. 4. The description oversells the error — agreed, fixing the wording You are right. With the default One scope point you raised under "not checked" You noted that Not checked on our side either: real provider payloads, the streaming path end to end, and We will come back with the follow-up commit for points 2, 3 and 4. |
…roval A payload that is not arguments cannot become a working call by approving it, so the model is now told what went wrong instead of a human being asked to approve a call that can only fail. The guard is the one the invocation path already uses. The guard's docstring now records that it runs before the middleware pipeline, so middleware does not see a malformed payload as a tool call.
|
Here is the follow-up for points 2, 3 and 4, and the answer to the reachability question you raised under "not checked". It is in 2. The approval path — fixed The check now also runs where the approval request is built, so a call whose arguments are not arguments is answered before a human is asked to approve it. It is the same guard the invocation path uses, and the call is answered the way a missing required property is answered, with the cause in the The new test 3. Middleware ordering — recorded The guard's docstring now states that it runs before the middleware pipeline, so a malformed payload is not seen by middleware as a tool call. 4. Description wording — corrected The description now says that the cause is carried in the result's The MCP conversion path — the reachability question, answered I traced it rather than assuming. This pull request is therefore scoped to the function-calling loop, which is what the issue asks for — "the loop should answer them the way it answers a missing required property" — and what its reproduction exercises. The sampling path needs a decision that does not exist yet: what an MCP client should receive for a call that cannot run. That is the same kind of maintainer-level call as the trade-off you raised, so I would rather ask than guess, and extending the guard there would change the MCP error surface of a route this change does not touch. I have recorded the reachability analysis here and will put that question to the maintainers alongside the one you raised. Verification
|
Eduard van Valkenburg (eavanvalkenburg)
left a comment
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
One function-loop contract issue should be addressed before merge.
|
|
||
| parsed_args: dict[str, Any] = dict(function_call_content.parse_arguments() or {}) | ||
| try: | ||
| parsed_args = _parse_function_call_arguments(function_call_content, tool.name) |
There was a problem hiding this comment.
Can we move this validation into the innermost handler, after the function middleware has run? Middleware should receive the raw parsed mapping and have a chance to repair the arguments before we return an error, consistent with the existing function-loop contract. If the arguments are still invalid after middleware, then return the argument-validation result immediately before tool execution.
Address review feedback: the function-loop contract lets middleware receive the raw parsed mapping and repair it before any final validation, so the not-a-JSON-object answer moves into the innermost handler. - A payload that is not a JSON object keeps reaching middleware as the raw parsed mapping; argument preparation is skipped for it, since the model would silently drop the wrapper as an extra field. - The innermost handler answers a call whose arguments are still the wrapped payload, immediately before the tool would run; a repaired call executes like any other. - A malformed call is no longer answered before approval: middleware may still repair it once the call is approved.
|
Moved, in
Tests: the pre-approval assertion is replaced with one asserting the approval request surfaces; a middleware-repair case (raw wrapper -> repaired arguments -> tool runs) and an unrepaired case (middleware passes it through -> answered before execution) cover the new contract. |
Somay kousis (Somay-kousis)
left a comment
Somay kousis (Somay-kousis)
left a comment
There was a problem hiding this comment.
Re-reviewed at 130f0f9. Moving the check after middleware works for the cases in the new tests: unrepaired payloads are still answered (my earlier probe gives the same table as at 84de7d5), a middleware that replaces the arguments gets a normal call, and test_tools, test_function_invocation_logic and test_middleware pass here (710 passed). One gap in how "repaired" is decided.
A middleware that adds a key makes the malformed payload count as repaired
final_function_handler treats the call as repaired whenever context.arguments differs from the wrapped mapping (_tools.py:2397, :2402-2403). A middleware that injects a value into every call (a user or tenant id, a trace id) changes the mapping without touching the wrapper, so the payload goes through:
class InjectUser(FunctionMiddleware):
async def process(self, context, call_next):
context.arguments = {**context.arguments, "user": "u1"}
await call_next()With arguments="[1,2]" through _try_execute_function_call_groups:
| tool | middleware | tool called with | result |
|---|---|---|---|
permissive (properties: {}) |
none | not called | Error: Argument parsing failed. |
permissive (properties: {}) |
InjectUser |
{'raw': [1, 2], 'user': 'u1'} |
ok |
@tool def typed(user: str = "", limit: int = 0) |
InjectUser |
{'user': 'u1', 'limit': 0} |
ok |
The second row is the issue's original symptom (the tool runs with an argument it never declared), and in the third the model never learns its call was malformed. Both rows behave the same at the merge base, so this path is unchanged by the PR rather than a regression, but it means the fix only holds when no such middleware is installed.
One option: treat the call as still unrepaired while the wrapper's raw entry is present with its original value and the tool doesn't declare a raw parameter, instead of comparing the whole mapping. A test with an injecting middleware would pin it.
Not rechecked: the approval change (now sent for approval by design, as you explained) and point 1, which is the maintainer's call.
…ving the payload unrepaired Address review follow-up: the repaired/unrepaired decision compared the whole arguments mapping, so a middleware that appends a key to every call (a user or trace id) preserved the wrapped payload while counting as a repair, and the tool still ran with the undeclared raw argument. The wrapper now counts as repaired only when its original raw entry is gone or rewritten. A raw entry the tool itself declares is ordinary arguments and never counts as the wrapper, which also restores the pre-guard behaviour for tools that accept raw by name.
|
Fixed in the follow-up commit on the branch (pushed via the Git data API after SSH kept resetting - the tree is identical to the local commit): the wrapper counts as unrepaired exactly as you proposed, and an injecting middleware no longer un-marks it. The repaired test is now narrow: the call counts as repaired only when the original
Tests: your two injecting-middleware rows are pinned ( |
Motivation & Context
A model that answers a function call with something that is not a JSON object has not sent arguments, and the tool should not run as if it had.
Content.parse_arguments()wraps such a payload under a single"raw"key:That is the right answer for a caller that wants to see what the model sent. The function-calling loop, however, used the result as the arguments of the call. For a tool whose schema has no required properties and does not set
additionalProperties: false, the loop's own checks then pass —rawis just an extra key, and extra keys are allowed — so the tool was invoked withraw=...as if the model had made a well-formed call:A missing required argument or a wrong type is answered with an error the model can correct on; a malformed payload was not reported anywhere. When the tool is proxied to an MCP server, the malformed call reaches the server carrying a bogus
rawargument.Description & Review Guide
What are the major changes?
The function-calling loop now parses the payload strictly (
_parse_function_call_arguments) and answers a call whose arguments are not a JSON object the way it already answers a missing required property: with an argument-validation error result. The cause ("arguments are not a JSON object") is carried in the result'sexceptionfield; with the defaultinclude_detailed_errors=Falsethe model-visible message isError: Argument parsing failed., and the cause is appended to that message only when detailed errors are enabled. That is the same convention the existing missing-required-property path uses.Content.parse_arguments()keeps its documented"raw"wrapping for direct callers, so nothing changes for them.A call whose arguments are not arguments is now answered in both places where it could otherwise have been acted on: the invocation path, and the path that builds an approval request. A human is therefore no longer asked to approve a call that cannot run.
What is the impact of these changes?
The tool is no longer invoked with an argument it never declared, and the model is told what went wrong. A well-formed call is unaffected.
One behaviour change is worth calling out: a payload that is not a JSON object is now rejected for every tool, including a tool whose schema takes no parameters. On
main, such a call ("null", whitespace-only, or malformed text) reached a zero-parameter tool with the extrarawkey dropped by its input model and ran; it is now answered with the argument-validation error. Empty arguments ("") and a well-formed object are unaffected.What do you want reviewers to focus on?
Where the strictness lives. The check is in the loop rather than in
parse_arguments()on purpose:parse_arguments()is a public API with a documented"raw"fallback, and the loop is the only caller that turns the result into a tool invocation, so that is the place where a payload that is not arguments has to be rejected.The error reuses the existing
_FunctionArgumentValidationErrorpath, so a malformed call produces the same kind of result as a missing required property, including the redacted message when argument snapshots are in play.The check is in the loop, so it covers the in-process
FunctionToolinvocation path. The MCP conversion in_mcp.pybuilds the outgoingtool_useinput fromContent.parse_arguments()and is not covered by this change; whether a malformed model-authored payload can reach that conversion is being checked separately.Related Issue
Fixes #9562
The direction follows the expectation stated in the issue: a function call whose arguments are not a JSON object must be answered with an error instead of reaching the tool with an undeclared
rawargument. The issue does not carry theready-for-implementationlabel yet. We asked on the issue on 2026-10-22, and we will close this pull request if there is no confirmation by 2026-10-27.AI Assistance
AI assistance details: the issue analysis, the implementation in
agent_framework/_tools.py, the regression tests, and this description were drafted with an AI coding agent (MiniMax M3.1 Flash via the ZCode CLI) under the repository's AI-Assisted Contributions Policy. What I verified myself: on the unmodified file the tool is invoked with{'raw': 'not {json'}(and with{'raw': [1, 2]}for a valid JSON array), which is the behaviour the issue reports; the new tests fail there and pass with the change, and a well-formed call still reaches the tool with its arguments unchanged; the fullpackages/core/testssuite,ruff checkandruff format --checkwith the pinned 0.17.0, andpyrighton the changed file are unchanged apart from this fix.Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.