[Anish Mehta (anishmehta24)]

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"), with fallback=str on Pydantic versions that have it, like _mcp.py). If JSON mode fails for a model (bytes that aren't valid UTF-8, or an arbitrary type on Pydantic < 2.11), those values are escaped in a copy of the model and Pydantic dumps the copy, so the declared field types, JSON-only serializers and exclude=True still apply. A field that still can't be encoded is left out.
• 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? The fallback in FunctionTool._dump_model. Models that JSON mode can encode never reach it.

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 (23 passed) on pydantic 2.0.3, 2.10.6 and 2.14.1. In the core suite the only failures locally are MCP tests (no mcp package in my env), test_declared_feature_indexes_match_registry and the aiohttp invocation tests, which fail the same way on main here. ruff check/format clean.

[Copilot]

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

[Eduard van Valkenburg (eavanvalkenburg)]

/review

[github-actions]

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

[Eduard van Valkenburg (eavanvalkenburg)]

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.

[Anish Mehta (anishmehta24)]

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.

[Eduard van Valkenburg (eavanvalkenburg)]

Anish Mehta (@anishmehta24) I found two remaining blocking serialization gaps on the current head:

• python/packages/core/agent_framework/_tools.py:1285: on supported Pydantic 2.0.x, type(model).model_computed_fields is a class property rather than an iterable. If JSON dumping enters this fallback, it raises TypeError; FunctionTool.invoke can then fall back to str(result), which can include values from fields marked exclude=True.
• python/packages/core/agent_framework/_tools.py:1291-1295: the fallback recurses only when the field itself is a BaseModel. A list[BaseModel] or dict[str, BaseModel] falls back in Python mode as a whole, bypassing nested JSON-only serializers and potentially emitting raw values.

Please make computed-field enumeration version-safe and recursively preserve JSON-mode serializers and exclusions inside model containers, with regressions for Pydantic 2.0.x and nested model lists/maps.

[Anish Mehta (anishmehta24)]

Eduard van Valkenburg (@eavanvalkenburg) both fixed in 4295bcd.

• The fallback no longer reads model_computed_fields. It takes the field names from model.model_dump(by_alias=False), which already leaves excluded fields out and works the same on every 2.x version.
• When a field falls back and its value is a model, list, tuple, set or dict, the value is walked and every model inside goes through the same per-field dump, so nested JSON-only serializers and exclusions still apply. Fields with their own serializer keep the python-mode value instead, so that serializer isn't bypassed.

Added tests for a computed field on the fallback path and for list[Model] / dict[str, Model] fields. The parse_result tests pass on pydantic 2.0.3, 2.10.6 and 2.14.1. On 2.0.3 the previous commit failed all the fallback tests with the TypeError.

[Eduard van Valkenburg (eavanvalkenburg)]

please resolve the comments Anish Mehta (@anishmehta24)

[agent-framework-automation]

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.)

[Eduard van Valkenburg (eavanvalkenburg)]

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.

[Anish Mehta (anishmehta24)]

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.