Visitar URL original
Python: fix(core): serialize Pydantic tool results in JSON mode by anishmehta24 · Pull Request #9036 · microsoft/agent-framework · GitHub
Skip to content

Python: fix(core): serialize Pydantic tool results in JSON mode - #9036

Merged
Eduard van Valkenburg (eavanvalkenburg) merged 11 commits into
microsoft:mainfrom
anishmehta24:fix/tool-result-pydantic-json-mode
Nov 11, 2026
Merged

Eduard van Valkenburg (eavanvalkenburg) merged 11 commits into
microsoft:mainfrom
anishmehta24:fix/tool-result-pydantic-json-mode

Conversation

@anishmehta24

@anishmehta24 Anish Mehta (anishmehta24) commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Motivation & Context

A tool that returns a Pydantic model is serialized with model_dump() and then json.dumps(default=str), so enum and datetime fields come out as their Python str():

class Ticket(BaseModel):
    status: Status        # Enum, Status.OPEN = "open"
    created: datetime

FunctionTool.parse_result(Ticket(status=Status.OPEN, created=datetime(2024, 1, 1, 12)))[0].text
# '{"status": "Status.OPEN", "created": "2024-01-01 12:00:00"}'
# Pydantic's JSON: {"status":"open","created":"2024-01-01T12:00:00"}

The model gets Status.OPEN back from a tool whose own input schema uses "open".

Description & Review Guide

  • What are the major changes? _make_dumpable dumps BaseModel values in JSON mode: model_dump(mode="json", fallback=str) like _mcp.py, or on Pydantic < 2.11, which has no fallback, to_jsonable_python(model, by_alias=False, serialize_unknown=True), which uses the same JSON serializer. Either way Pydantic dumps the whole model, so serializers, computed fields, declared field types and exclude=True apply as usual and only values JSON mode doesn't know become str(). If Pydantic still can't dump the model (bytes that aren't valid UTF-8, a reference cycle), it is replaced by a placeholder and a warning is logged, instead of invoke falling back to str(result).
  • What is the impact of these changes? Pydantic results (top level, in lists and dicts) are serialized the way Pydantic writes JSON. Plain str/int/float fields are unchanged.
  • What do you want reviewers to focus on? FunctionTool._dump_model and the placeholder for models Pydantic can't dump.

Related Issue

N/A - trivial change: a bug fix in _make_dumpable (Pydantic tool results dumped in JSON mode). The repro is above and the PR adds a regression test that fails on main.

AI Assistance

  • No material AI assistance was used.
  • This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

AI assistance details: implementation, tests and this description.

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Test: test_parse_result_pydantic_model_uses_json_mode in packages/core/tests/core/test_types.py; it fails on main. The parse_result tests pass on pydantic 2.0.3, 2.10.6 and 2.14.1 (the fallback tests run both dump paths). test_types.py, test_tools.py and test_function_invocation_logic.py pass locally except the two aiohttp tests (no aiohttp in my env). ruff check/format and pyright clean.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@agent-framework-automation agent-framework-automation Bot added the python Usage: [Issues, PRs], Target: Python label Oct 4, 2026
@eavanvalkenburg

Copy link
Copy Markdown
Member

/review

@github-actions github-actions 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.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 4ba0a5cfbd3d
Model: gpt-5.6-sol

Overview

The PR correctly targets Pydantic's JSON-mode conversions and adds a focused regression test for enums and datetimes, while retaining the existing paths for strings, Content, and non-Pydantic values. However, JSON mode can reject otherwise valid Pydantic models, and the normal invocation fallback then exposes the model's raw string representation, including excluded fields. This creates a concrete credential-disclosure path and breaks direct serialization callers.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
1 verified finding remained after source verification (1 high) across 1 file. Details are attached to the affected lines below.

Affected areas: python/packages/core/agent_framework/_tools.py

@eavanvalkenburg

Copy link
Copy Markdown
Member

Anish Mehta (@anishmehta24) Could you add the related issue this fixes, or briefly document why this qualifies for the trivial-change exception? The current body does not answer that required template field.

@anishmehta24

Copy link
Copy Markdown
Contributor Author

There is no issue for this one. I filled in the trivial-change exception under Related Issue (one-line fix with a repro and a regression test), and added the AI Assistance section.

@eavanvalkenburg

Copy link
Copy Markdown
Member

please resolve the comments Anish Mehta (@anishmehta24)

@agent-framework-automation

Copy link
Copy Markdown

Anish Mehta (@anishmehta24), this pull request is waiting on your response. If there is no further activity within 4 days, it will be closed automatically. (This is an automated message.)

@agent-framework-automation agent-framework-automation Bot added the requested-info Information or updates are needed from the author; inactive PRs may be closed automatically. label Nov 4, 2026
@eavanvalkenburg

Copy link
Copy Markdown
Member

Anish Mehta (@anishmehta24) One security-sensitive correctness issue remains in python/packages/core/agent_framework/_tools.py:1458-1460. The recursive fallback serializes the raw attribute independently instead of preserving the parent field’s declared Pydantic schema. If a field is declared as a public base model but contains a subclass with credentials, an unencodable bytes value triggers the fallback and exposes subclass-only fields that normal Pydantic serialization omits. I reproduced this with a base model containing data: bytes, a subclass adding api_key, and an envelope declaring the base type: base omits api_key, while this head includes it for direct, list, and dictionary fields. Please preserve declared field and container-element serialization schemas during recursive fallback, with regressions proving subclass-only credentials remain omitted.

The fallback dumped raw attribute values by their own class, so a subclass in a
field declared as its base sent its subclass-only fields. Escape the values JSON
mode can't encode in a copy of the model and let Pydantic dump the copy instead.
@anishmehta24

Copy link
Copy Markdown
Contributor Author

Eduard van Valkenburg (@eavanvalkenburg) fixed in 317e9bb. The fallback no longer dumps raw attribute values. It escapes the invalid UTF-8 bytes in a copy of the model and lets Pydantic dump that copy, so the declared field types decide the output and api_key stays out. Added your case as a test for direct, list and dict fields.

@agent-framework-automation agent-framework-automation Bot removed the requested-info Information or updates are needed from the author; inactive PRs may be closed automatically. label Nov 5, 2026
@anishmehta24

Copy link
Copy Markdown
Contributor Author

Eduard van Valkenburg (@eavanvalkenburg) thanks. All five came from the fallback editing a copy of the model and dumping it field by field, so I removed it in b40e2dc. Pydantic now always dumps the whole model: model_dump(mode="json", fallback=str), or on versions without fallback, to_jsonable_python(..., serialize_unknown=True), which uses the same JSON serializer. If that fails (non-UTF-8 bytes, a cycle), the model is replaced by a placeholder with a warning, never str(model). Added a regression for each case; the fallback ones run both dump paths.

Merged via the queue into microsoft:main with commit 6d79abf Nov 11, 2026
51 checks passed

This branch was previously deployed

1 inactive deployment
github-app-auth — b40e2dc1 Deployed Nov 9, 2026 by anishmehta24 via add_label #27436
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants