Serialize tool results with Pydantic's JSON mode - #2204
Conversation
`_normalize_result`'s `default` hook called `model_dump()`, which uses
the default `mode="python"`. That leaves `datetime`, `date`, `UUID`,
`Decimal`, `Enum` and `set` fields as native Python objects, so
`json.dumps` hands them straight back to the same hook, which does not
recognise them and raises `TypeError`.
The raise happens inside `wrapped_handler`'s try block, which converts
any exception into the deliberately redacted failure result. A tool
returning a perfectly ordinary model such as
class Issue(BaseModel):
id: uuid.UUID
created_at: datetime.datetime
therefore reports "Invoking this tool produced an error. Detailed
information is not available." to the model on every call, with no
indication to the author of what went wrong.
Switching to `model_dump(mode="json")` makes Pydantic coerce each field
to a JSON-native type before `json.dumps` sees it, which matches what
the TypeScript SDK already does via `JSON.stringify` (JS `Date`
implements `toJSON`). Models whose fields are all JSON primitives
serialize exactly as before.
There was a problem hiding this comment.
Pull request overview
Updates Python tool-result normalization to serialize Pydantic models using JSON mode.
Changes:
- Converts Pydantic-native values into JSON-compatible representations.
- Adds regression coverage for UUID, datetime, Decimal, and enum fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
python/copilot/tools.py |
Uses Pydantic JSON-mode serialization. |
python/test_tools.py |
Adds non-primitive field serialization coverage. |
| assert result.result_type == "success" | ||
|
|
||
| def test_pydantic_model_with_non_primitive_fields_is_serialized(self): | ||
| class IssueState(enum.StrEnum): |
|
Correction to the verification section above: I originally ran the suite without the shared test harness, so I reported a partial run (184 passed) and described the four uncollectable modules as environmental. I've since followed the CONTRIBUTING step I'd skipped — Plus the rest of the CI gate from
So there are no outstanding failures on this branch. Apologies for the noise in the original description. Observed on darwin / Python 3.12.13 / Node v26.5.0, against |
Fixes #2203
What's wrong
_normalize_result'sdefaulthook callsmodel_dump(), which uses the defaultmode="python". That leavesdatetime,date,UUID,Decimal,Enumandsetfields as native Python objects, sojson.dumpspasses them straight back into the same hook, which doesn't recognise them and raisesTypeError.That raise happens inside
wrapped_handler'stryblock, which converts any exception into the deliberately redacted failure result. So a tool returning a perfectly ordinary model:reports
"Invoking this tool produced an error. Detailed information is not available."to the model on every call, and the tool author sees no indication of why.The fix
model_dump(mode="json")makes Pydantic coerce each field to a JSON-native type beforejson.dumpssees it. Models whose fields are all JSON primitives serialize exactly as before, so this is backwards compatible.This matches what the TypeScript SDK already does via
JSON.stringify(JSDateimplementstoJSON()).Verification
Added
test_pydantic_model_with_non_primitive_fields_is_serializedto the existingTestNormalizeResultclass, coveringUUID,datetime,DecimalandStrEnumin one model.It fails on the unfixed code with the reported error:
and passes with the change. Full local run:
uv run pytest test_tools.py→ 41 passed (40 before, plus the new test)uv run pytest --ignore=e2e --ignore=test_client.py --ignore=test_commands_and_elicitation.py --ignore=test_e2e_harness_cli_path.py→ 184 passeduv run ruff check .→ All checks passeduv run ruff format --check→ already formattedThe four ignored modules fail at collection with
RuntimeError: CLI not found for testsin my environment; I confirmed they fail identically on unmodifiedmain, so that is environmental and unrelated to this change.Scope
Deliberately minimal — one line of behaviour change plus a test. For fuller parity with the TypeScript SDK you may also want fallbacks in
defaultfordataclasses.asdict,datetime/date/time.isoformat(),UUID/Decimal→str,Enum→.valueandset→list, so a plaindictcarrying those types works too. I left that out to keep the diff focused — happy to add it if you'd prefer it here.