fix(messages): add ToolReturnPart to ModelResponsePart union#5723
fix(messages): add ToolReturnPart to ModelResponsePart union#5723Bartok9 wants to merge 5 commits into
ToolReturnPart to ModelResponsePart union#5723Conversation
9224d6d to
c866101
Compare
Closes pydantic#5721. `ModelRequestPart` already carries `Annotated[ToolReturnPart, pydantic.Tag('tool-return')]`, but `ModelResponsePart` omitted the base `ToolReturnPart` — it only listed the `builtin-*` native variants. `_agent_graph` directly constructs base `ToolReturnPart` instances for user-defined / output-tool results and stores them on a `ModelResponse`, so persisting and reloading that history (resumed runs, durable workflows, AG-UI / Vercel adapters) failed to deserialize with a `union_tag_invalid` ValidationError on the `'tool-return'` discriminator. Fix: add the base `ToolReturnPart` to the `ModelResponsePart` discriminated union. Because that makes `ToolReturnPart` a valid member everywhere `ModelResponse.parts` is consumed, the new variant is now handled explicitly at each site rather than falling through to `assert_never`: - Model request mappers (anthropic, openai, google, gemini, groq, mistral, cohere, huggingface, xai, outlines, test): skip the part — framework-stored user tool returns are not replayed to the provider, mirroring the existing CompactionPart handling. - `function._estimate_usage`: count the tool-return content (shares NativeToolReturnPart path). - `_agent_graph` event loop, AG-UI and Vercel adapters: no streamed/rendered event. - `_event_stream` part start/end dispatch: grouped with the other delta-less parts. - `PartStartEvent.previous_part_kind` / `PartEndEvent.next_part_kind`: add `'tool-return'`. - `_parts_manager._resolve_provider_name`: getattr guard (ToolReturnPart has no provider_name). - `google._handle_executable_code_streaming`: narrow return to its actual NativeToolCallPart; `_decode_inline_thought_signature`: short-circuit for ToolReturnPart. Tests: - `test_messages.py`: ToolReturnPart in a ModelResponse round-trips (python + json); the ToolSearchReturnPart subclass still narrows to its own tag in a ModelRequest. - `test_model_function.py`: `_estimate_usage` handles a response-embedded ToolReturnPart. Verification: `uv run pyright` clean (0 errors), `uv run ruff format --check` / `ruff check` clean, targeted message + function-model + UI adapter suites pass.
c866101 to
46b8a08
Compare
ToolReturnPart to ModelResponsePart union
…s-manager typing Completes the `ToolReturnPart`-in-`ModelResponsePart` fan-out and addresses review findings: - `BedrockConverseModel._map_messages` was missed and raised `AssertionError` for a base `ToolReturnPart` in a `ModelResponse` (its guard was `else: assert isinstance(item, ToolCallPart)`, not `assert_never`, so pyright didn't catch it). Restructure the tail to an explicit `ToolCallPart` branch + `assert_never`, with a no-op `ToolReturnPart` branch matching the other providers. - `_parts_manager._resolve_provider_name` narrows with `isinstance(ToolReturnPart)` instead of `getattr`, keeping `provider_name` access statically typed. - Add the missing `# pragma: no cover` on the `_agent_graph` streaming branch (siblings all have it). - Correct the repeated branch comments: a base `ToolReturnPart` reaches a `ModelResponse` via user-constructed message history, not "stored on a ModelResponse" by the framework.
…_parts` `ModelResponse.otel_message_parts` maps `NativeToolReturnPart` to a `tool_call_response` part but silently dropped the base `ToolReturnPart`, which is now reachable in user-constructed message history. Reuse `BaseToolReturnPart.otel_message_parts` so it renders the same way (without the `builtin` flag) instead of being omitted from telemetry. (`otel_events` deliberately left unchanged: it represents no tool-return part — not `NativeToolReturnPart` either — so omitting the base part there is pre-existing-consistent.)
| | Annotated[NativeToolCallPart, pydantic.Tag('builtin-tool-call')] | ||
| | Annotated[NativeToolSearchReturnPart, pydantic.Tag('builtin-tool-search-return')] | ||
| | Annotated[NativeToolReturnPart, pydantic.Tag('builtin-tool-return')] | ||
| | Annotated[ToolReturnPart, pydantic.Tag('tool-return')] |
There was a problem hiding this comment.
@DouweM This is the key design decision in the PR and I think it warrants explicit maintainer sign-off before proceeding.
Context: Issue #5721 was auto-generated by a roundtrip-sweep bot. No maintainer has commented on the issue or endorsed a specific approach. The PR author acknowledges the design choice in the PR description ("If you'd prefer a narrower serialization-only approach… happy to refactor").
The question: Should ToolReturnPart be a valid member of ModelResponsePart? The framework itself never places a ToolReturnPart into a ModelResponse — _agent_graph.py always stores tool returns in ModelRequest.parts, and model adapters use NativeToolReturnPart for builtin returns. The only way a ToolReturnPart ends up in a ModelResponse is via user-constructed message history, which would currently fail at deserialization.
Trade-offs:
- This approach (widening the union): fixes the deserialization crash and is defensive, but forces every consumer of
ModelResponse.partsacross ~20 files to handle a case the framework never produces. All new handler branches are# pragma: no coverdead code. - Alternative: serialization-only fix (e.g. a lenient custom discriminator that gracefully handles unknown/unexpected tags without widening the static type union): narrower change, fewer files touched, but less type-safe.
- Alternative: reject at construction: validate that
ModelResponse.partsdoesn't contain request-side parts, making the user error explicit rather than silently accepting it.
The right call depends on whether "user-constructed ModelResponse containing ToolReturnPart" is something the framework should support or discourage.
There was a problem hiding this comment.
David's AICA here: Decision on the design question (Douwe is away, so I'm making the call): we'll keep the union-widening approach — adding the base ToolReturnPart to ModelResponsePart. It's the approach with direct precedent and explicit version-policy cover; the two alternatives both reintroduce problems we've deliberately avoided.
Reasoning:
- Precedent. This is the same pattern established in feat: native tool search on Anthropic and OpenAI, with custom search strategies on any provider #5143 (native tool search) and Add server-side compaction support via
OpenAICompactionandAnthropicCompactioncapabilities #4943 (compaction): each side's discriminated union names every concrete part type for that side, and consumers handle new members with explicit branches — includingpass # pragma: no coverno-ops in provider_map_messages. The new bedrock branch sits literally next toelif isinstance(item, CompactionPart | FilePart): pass(bedrock.py:1066). So the pragma'd no-op branches this comment frames as a cost of widening are established house style, not new debt. - Version policy.
docs/version-policy.mdstates that adding new message parts (and their discriminator surface, e.g. theprevious_part_kind/next_part_kindLiterals) is not a breaking change, and "always code defensively when consuming message parts or event streams." That covers both the "breaking-ish Literal change" and the "forces consumers to handle a new case" concerns — defensive consumption is the documented contract. - Why not the alternatives. A lenient/serialization-only discriminator would leave
ToolReturnPartpresent in aModelResponseat runtime but absent from the staticModelResponseParttype — a permanent type-honesty gap that pushes users toward theisinstancechecks we explicitly don't want them to need. Reject-at-construction has no precedent for message parts and would break deserializing already-persisted histories that contain this shape, which is the actual [roundtrip-sweep] ModelResponsePart:ToolReturnPartwithpart_kind='tool-return'missing from discriminated union — breaks message history round-trip #5721 repro.
One correction to the framing above: the framework never produces a base ToolReturnPart on a ModelResponse — it routes tool returns into ModelRequest.parts. The shape is reachable only via user-constructed or deserialized message history, which is exactly the round-trip case #5721 hits, so the fix belongs at the type/deserialization layer.
…_name The 100% coverage gate flagged _parts_manager.py:533 — the `isinstance(existing_part, ToolReturnPart)` short-circuit added when ToolReturnPart joined the ModelResponsePart union. That branch is defensive (no public caller passes a ToolReturnPart as existing_part) so no existing test exercised it. Add a focused unit test asserting the resolver returns the incoming provider_name for a ToolReturnPart (which carries none of its own).
|
Pushed a small test to green up CI. The only failing job was That branch is defensive — no public caller passes a Locally: line 533 now covered, |
- exercise `_estimate_usage` via a public `Agent(FunctionModel)` run instead of importing the private function, so the test goes through the public API - correct the test docstring: a base `ToolReturnPart` reaches a `ModelResponse` only via user-constructed/deserialized history, not framework production - hoist the function-local `ToolReturnPart` import to the module-top import block
biswajeetdev
left a comment
There was a problem hiding this comment.
The fix correctly closes the gap: ToolReturnPart is a valid member of the ModelResponsePart union (representing a tool result in user-constructed message history) but was missing from the discriminated union definition, causing deserialization failures when a persisted conversation contained one.
Three places touched, all consistent:
messages.py— addsToolReturnPartto the union with its discriminator tag_parts_manager.py— short-circuits_resolve_provider_namesinceToolReturnPartcarries noprovider_name; returning the incomingprovider_nameunchanged is correct_agent_graph.py—passonToolReturnPartin the stream iterator since user-defined tool returns in pre-constructed history produce no streamed event
The # pragma: no cover on the _agent_graph.py branch is honest — this path requires user-constructed message history with a ToolReturnPart, which is hard to reach via the normal test harness. A note in the PR or a follow-up issue tracking a dedicated integration test for this path would be helpful, so it does not stay permanently uncovered.
Summary
ToolReturnPart(part_kind='tool-return') is a member of theModelRequestPartdiscriminated union but was missing fromModelResponsePart, even though both unions share the same discriminator. As a result, aToolReturnPartembedded in aModelResponseserializes fine but fails to deserialize with aunion_tag_invalidValidationError— the writer emits the'tool-return'tag, the reader finds no response-union member claiming it.This PR adds the base
ToolReturnParttoModelResponsePartand handles the new variant everywhereModelResponse.partsis consumed, so the change stays fully type-checked (noassert_neverregressions).Motivation
Closes #5721.
This is a type-system + serialization-contract consistency fix, not a fix for a shape the framework emits today. To be precise about reachability (the issue's "Root Cause" is inaccurate on this point — see my comment there):
ToolReturnParton aModelResponse. The framework synthesizes tool returns (including the output-tool "Final result processed." parts) intooutput_parts: list[ModelRequestPart]→ aModelRequest. Verified empirically: across a tool-calling run, everyToolReturnPartsits on aModelRequest; the matchingModelResponseholds theToolCallPart. Soall_messages_json()round-trips — and therefore resumed runs, durable workflows, and the AG-UI/Vercel adapters that round-trip persisted history — are unaffected in normal operation.ToolReturnPartwithpart_kind='tool-return'missing from discriminated union — breaks message history round-trip #5721): code that buildsModelResponse(parts=[ToolReturnPart(...)])itself, then persists and reloads it.Why fix it anyway:
ModelRequestPartalready admitsToolReturnPartunder the same shared discriminator. Apart_kindthat one union accepts and its sibling rejects is a latent hole in a public, documented serialization contract.ToolSearchReturnPart(aToolReturnPartsubclass) round-trips via its own'tool-search-return'tag (_TYPED_PART_TAGS); only the base part fell through. Adding the base member completes an existing pattern and cannot shadow the subclass — pinned bytest_tool_search_return_part_in_model_request_still_narrows.NativeToolReturnPartsibling path (see below): a BedrockAssertionErrorand an OTel telemetry drop.(If a narrower serialization-only approach is preferred — e.g. routing the tag without widening the static union — happy to refactor. But that desyncs the type from its discriminator; widening the union is what makes the round-trip type-correct end to end.)
What changed
The union addition is the core fix. Because it makes
ToolReturnParta valid member everywhereModelResponse.partsis iterated, every consumer now handles the new variant explicitly instead of falling throughassert_never:CompactionPart/FilePartskip pattern.function._estimate_usageNativeToolReturnParttoken path._agent_graphevent loop, AG-UI, Vercel adapters_event_streamstart/end dispatchModelResponse.otel_message_partstool_call_responseNativeToolReturnPart(minusbuiltin) so a response-embedded tool return isn't dropped from telemetry. (otel_eventsv1 unchanged — it emits no tool-return part at all, not even forNativeToolReturnPart.)PartStartEvent.previous_part_kind/PartEndEvent.next_part_kind'tool-return'literal_parts_manager._resolve_provider_nameisinstancenarrowToolReturnParthas noprovider_nameand is never tracked by the streaming parts manager; narrowing keepsprovider_nameaccess statically typed.google._handle_executable_code_streamingNativeToolCallPart(its real return)Two latent bugs the fan-out surfaced
AssertionError.BedrockConverseModel._map_messagesguarded its response-part loop with a bareelse: assert isinstance(item, ToolCallPart)rather thanassert_never, so the missing branch wasn't caught at type-check time and raised at runtime. Its tail is now restructured to an explicitToolCallPartbranch +assert_never, matching the other providers and catching any future union member statically.ModelResponse.otel_message_partsmappedNativeToolReturnPartto atool_call_responsepart but silently dropped the baseToolReturnPart(the ladder has noassert_never, so nothing flagged it). Now mapped via the sharedBaseToolReturnPart.otel_message_parts.The provider/UI skip branches are
# pragma: no cover(defensive, like theirCompactionPart/FilePartsiblings); the round-trip and OTel guarantees are pinned by thetest_messages.pytests below.Verification
pre-commit(fullpyrighttypecheck,ruff format --check,ruff check) — cleanuv run pytest tests/test_messages.py— passes (round-trip, subclass-narrowing, OTel)uv run pytest tests/models/test_model_function.py— passes (_estimate_usageregression)_map_messagesno longer raises on a user-constructedModelResponse([ToolReturnPart(...)]); the previously-crashing path now maps cleanly.Tests added
tests/test_messages.py::test_tool_return_part_in_model_response_round_tripstests/test_messages.py::test_tool_search_return_part_in_model_request_still_narrowstests/test_messages.py::test_tool_return_part_in_model_response_otel_message_partstests/models/test_model_function.py::test_estimate_usage_handles_tool_return_part_in_response