Repository navigation
Python: fix(core): serialize Pydantic tool results in JSON mode - #9036
Conversation
Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
|
/review |
There was a problem hiding this comment.
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
|
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. |
|
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. |
|
please resolve the comments Anish Mehta (@anishmehta24) |
…antic-json-mode # Conflicts: # python/packages/core/agent_framework/_tools.py
…s invalid UTF-8 bytes
|
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.) |
|
Anish Mehta (@anishmehta24) One security-sensitive correctness issue remains in |
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.
|
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 |
|
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: |
Motivation & Context
A tool that returns a Pydantic model is serialized with
model_dump()and thenjson.dumps(default=str), so enum and datetime fields come out as their Pythonstr():The model gets
Status.OPENback from a tool whose own input schema uses"open".Description & Review Guide
_make_dumpabledumpsBaseModelvalues in JSON mode:model_dump(mode="json", fallback=str)like_mcp.py, or on Pydantic < 2.11, which has nofallback,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 andexclude=Trueapply as usual and only values JSON mode doesn't know becomestr(). 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 ofinvokefalling back tostr(result).FunctionTool._dump_modeland 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
AI assistance details: implementation, tests and this description.
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.Test:
test_parse_result_pydantic_model_uses_json_modeinpackages/core/tests/core/test_types.py; it fails on main. Theparse_resulttests 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.pyandtest_function_invocation_logic.pypass locally except the two aiohttp tests (noaiohttpin my env). ruff check/format and pyright clean.