Skip to content

Python: make tool argument-validation errors self-correcting for the model - #7953

Open
Yashvant Mahadev Hange (YashvantHange) wants to merge 5 commits into
microsoft:mainfrom
YashvantHange:python-self-correcting-arg-errors
Open

Python: make tool argument-validation errors self-correcting for the model#7953
Yashvant Mahadev Hange (YashvantHange) wants to merge 5 commits into
microsoft:mainfrom
YashvantHange:python-self-correcting-arg-errors

Conversation

@YashvantHange

Copy link
Copy Markdown
Contributor

Summary

Fixes #7222.

The default (non-detailed) argument-validation error returned to the model was the generic Error: Argument parsing failed. It named neither the tool nor the offending/missing parameter keys, so a model that made a systematic shape error had nothing to correct against and could retry the identical malformed call indefinitely - the issue includes a live trace of exactly that: four identical retries over ~8 minutes, bounded only by max_function_calls.

The default message now names the tool and the offending/missing keys:

  • For the schema-supplied TypeError path (_validate_arguments_against_schema), the existing message already does this cleanly (e.g. Missing required argument(s) for 'todos_add': todos) - it was just being discarded by the generic wrapper.
  • For the pydantic ValidationError path (typed function tools), a new helper (_format_argument_validation_error) builds a short, structured summary from exc.errors(include_url=False, include_context=False, include_input=False) - the field path and pydantic's short msg, with no raw exception repr and no echo of the submitted argument values.

include_detailed_errors is unchanged: it still only controls whether the full raw exception text is additionally appended on top of the (now more useful) default message.

Test plan

  • Updated the existing argument-validation-error tests (non-streaming, streaming, and the approved-function-call replay path) to assert on the new default message instead of the old generic string.
  • Added a new regression test for the schema-supplied TypeError path specifically, distinct from the pydantic-coercion ValidationError tests.

…model

The default argument-validation error returned to the model was the generic
"Error: Argument parsing failed.", naming neither the tool nor the
offending/missing parameter keys - a model that made a systematic shape
error had nothing to correct against and could retry the identical call
indefinitely. The default message now names the tool and the
offending/missing keys, built from the schema-validation TypeError or a
structured summary of the pydantic ValidationError, without raw exception
text or an echo of the submitted values. include_detailed_errors keeps its
existing behavior of appending the full exception text on top.

Fixes microsoft#7222

Copilot AI 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.

Pull request overview

Improves Python tool argument-validation feedback so models can correct malformed calls.

Changes:

  • Adds concise Pydantic validation summaries.
  • Preserves schema-validation details by default.
  • Updates streaming, non-streaming, and approval-path tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
python/packages/core/agent_framework/_tools.py Formats actionable validation errors.
python/packages/core/tests/core/test_function_invocation_logic.py Updates and expands validation-error coverage.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py
Two follow-ups from Copilot's review on PR microsoft#7953:

- The schema-supplied TypeError path's enum-mismatch message named the
  submitted value in its default text, quietly bypassing the "no value
  echo by default" guarantee the rest of the message does honor.
  _validate_arguments_against_schema now raises a small
  _ToolArgumentValidationError carrying a value-free safe_message
  alongside the full detailed message, and the default result uses the
  former; include_detailed_errors still surfaces the latter unchanged.
- A pydantic ValidationError can carry one entry per invalid item in a
  large submitted container, so the per-field summary is now capped at
  5 entries with an omitted-count suffix, instead of growing unbounded
  with the size of the model's mistake.

Copilot AI 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.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

python/packages/core/agent_framework/_tools.py:1456

  • include_input=False only removes Pydantic's input field; it does not make loc or msg value-free. For example, a failed dict[str, int] entry places the submitted dictionary key in loc, and a ValueError from a custom validator can embed the submitted value in msg. Rendering both verbatim therefore exposes model-provided data even when include_detailed_errors=False, contrary to this helper's contract. Build the summary from a bounded top-level parameter path and a whitelist of value-free messages derived from the error type, and add regressions for dynamic mapping keys/custom validators.
        raw_errors = exception.errors(include_url=False, include_context=False, include_input=False)
        offenses = [
            f"{'.'.join(str(segment) for segment in error['loc']) or tool_name} ({error['msg']})"
            for error in raw_errors[:_MAX_VALIDATION_ERROR_DETAILS]

python/packages/core/agent_framework/_tools.py:1465

  • The plain TypeError fallback exposes arbitrary exception text by default. Pydantic v2 lets a TypeError raised by a user-supplied model validator propagate from model_validate; if that message contains the submitted value or application detail, it now bypasses include_detailed_errors. Only _ToolArgumentValidationError has a vetted safe message, so keep unknown TypeError text behind the detailed-errors flag and return a generic tool-named summary here.
    return f"Error: {exception}"

Comment thread python/packages/core/agent_framework/_tools.py Outdated
Per python/AGENTS.md, changes to the function-calling loop's error paths
must be represented in docs/specs/004-python-function-calling-loop.md's
scenario matrix. Adds the "Actionable validation-error message" row
covering the new value-safe, capped default message and its regression
tests, and lists microsoft#7222 under related issues.
Comment thread python/packages/core/agent_framework/_tools.py Outdated
Two follow-ups from PR microsoft#7953 review:

- @moonbox3: a schema-supplied tool's missing/unexpected key list had no
  cap, so a model submitting an object with many unexpected keys could
  make the default error result itself unbounded. _validate_arguments_
  against_schema now formats both lists through a shared _format_field_list
  helper with the same omitted-count cap already applied to pydantic
  ValidationError details.
- Copilot (suppressed finding): the fallback branch in
  _format_argument_validation_error forwarded str(exception) for any
  TypeError that isn't the framework's own vetted _ToolArgumentValidationError,
  which would leak unvetted exception text by default if one ever reached
  that point. It now returns a generic, tool-named message instead; the raw
  text still surfaces via include_detailed_errors like any other exception.

Spec doc and its scenario-to-test mapping updated to match.
Fixes a real value leak in the shipped code, not just a correction of
something already said: a dict-typed field's invalid entry puts the
caller's own key verbatim in pydantic's ValidationError.loc (confirmed:
{"mapping": {"secret-abc": "bad"}} puts 'secret-abc' straight into loc),
and the default message rendered loc as-is. In review I'd argued this
was "the same category as the unexpected/missing field names this PR
already surfaces by design" - that was wrong. A dict key is data the
caller chose; a declared field name is static shape from the tool's own
schema. Confirmed end to end: before this fix, the default (non-detailed)
message echoed the submitted key; after, it shows a fixed placeholder.

_redact_dynamic_loc_segments walks a ValidationError's loc against the
tool's own JSON schema (resolving $ref/$defs for nested models): a
segment is named only when the schema declares it as a properties key or
a list index; a segment indexing into a dict-typed field's
additionalProperties is replaced with a placeholder instead of echoed.
Declared field names, including nested and list-indexed ones, are
unaffected - verified with a companion test so the fix doesn't
over-redact the tool's own static shape, which is what makes the default
message actionable in the first place.
@YashvantHange

Copy link
Copy Markdown
Contributor Author

Evan Mattson (@moonbox3) — going back over my own replies on this PR after a mistake on a different one (I'd told you FanOutEdgeRunner was safe from a race it wasn't; you reproduced it anyway). Found two things I got wrong here too, one of which is a real bug in what's already merged onto this branch. Fixed both in 90eae530.

1. The dict-key leak — this one's real, not just a wrong thing I said. Copilot flagged that include_input=False doesn't stop a dict-typed field's own key from appearing in loc. I dismissed it: "the same category as the unexpected/missing field names this PR already surfaces by design." That's wrong. A declared field name is static shape from the tool's own schema; a dict key is data the caller chose. Confirmed it end to end — before the fix, {"mapping": {"super-secret-api-key-abc123": "bad"}} put the key straight into the default (non-detailed) message. _redact_dynamic_loc_segments now walks loc against the tool's own schema ($ref/$defs resolved for nested models) and only names a segment when the schema declares it as a field or list index; a dict key gets a fixed placeholder instead. Verified both directions — the leak is gone, and nested/list-indexed declared field names (item.title, items[0].title) still show in full.

2. The "not currently reachable" claim on the TypeError fallback — also wrong, but no live consequence. I said _validate_arguments_against_schema was the only source of TypeError on this path. Tested it: pydantic wraps a validator's ValueError/AssertionError into ValidationError, but not TypeError — that escapes bare. So a tool author's @model_validator mistakenly raising TypeError does reach that "unreachable" branch. The fallback I'd already written happens to handle it correctly regardless (generic tool-named message, no leak) — I checked. So no code change needed there, just correcting what I told you.

Both confirmed with actual reproductions, not just re-reading the code — didn't want to just take my own word for it a second time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: [Feature]: make tool argument-validation errors self-correcting for the model

3 participants