Python: Fix executor_completed event with non-copyable raw_representation in mixed workflows

[moonbox3]

Motivation and Context

In workflows with mixed LLM agents, the executor_completed event contained an empty AgentResponse because deep-copying the response failed silently when raw_representation held LLM SDK objects (e.g., proto/gRPC responses) that do not support copy.deepcopy.

Fixes #4455

Description

The root cause was that deepcopy was applied to the entire AgentResponse and Content objects, including raw_representation which may contain non-copyable LLM SDK objects. The fix adds custom __deepcopy__ methods to SerializationMixin and Content that shallow-copy raw_representation by reference while deep-copying all other attributes. Regression tests verify that workflows complete correctly when an agent produces a response with a non-copyable raw_representation.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by moonbox3's agent

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 81%

✗ Correctness

The diff adds deepcopy methods to SerializationMixin and Content to preserve raw_representation by shallow reference. Two correctness concerns: (1) Content.deepcopy in _types.py uses bare deepcopy(v, memo) but the diff shows no import of deepcopy from the copy module — if it's not already imported in that file, this will raise a NameError at runtime; (2) SerializationMixin.deepcopy hardcodes the check for "raw_representation" rather than consulting cls.DEFAULT_EXCLUDE, which contradicts the test name claiming it preserves 'DEFAULT_EXCLUDE fields by reference' and means any future non-copyable field added to DEFAULT_EXCLUDE will still be deep-copied and fail.

✓ Security Reliability

The diff adds __deepcopy__ implementations to SerializationMixin and Content that shallow-copy raw_representation while deep-copying all other attributes. The pattern is standard and correctly uses the memo dict for cycle prevention. No injection risks, secrets, resource leaks, or unsafe deserialization found. The shared mutable reference for raw_representation is intentional and well-documented. One minor reliability observation: the field name is hardcoded rather than driven by class configuration, but this is a deliberate scoping choice, not a bug.

✗ Test Coverage

The deepcopy tests are generally well-structured, covering multiple types (Content, Message, AgentResponse, etc.) and an integration test through the workflow executor. However, there is one meaningless assertion (tautology) that should be fixed, and there are notable gaps: no test verifies that the memo dict correctly handles circular references (which the implementation explicitly supports), no test verifies subclass deepcopy behavior (which the type(self) pattern explicitly supports), and the test name 'preserves_excluded_fields_by_reference' is misleading since the implementation hardcodes 'raw_representation' rather than consulting DEFAULT_EXCLUDE.

✗ Design Approach

The __deepcopy__ implementations in both SerializationMixin and Content hardcode the field name "raw_representation" instead of leveraging the existing DEFAULT_EXCLUDE class variable (or a new dedicated class variable). This is an overly narrow fix: if a subclass adds another non-deep-copyable field, the hardcoded check will miss it and deepcopy will crash at runtime. The test is even named test_deepcopy_preserves_excluded_fields_by_reference (plural), implying the intent is to handle all such fields generically, but the implementation only handles one. The correct approach is to consult the class-level set rather than matching a single string literal.

Flagged Issues

• Content.deepcopy in _types.py uses bare deepcopy(v, memo) but no from copy import deepcopy import is added in this diff. If it is not already imported in the file, this will be a NameError at runtime.
• Line 451 in test_serializable_mixin.py has a tautological assertion: assert cloned.value is not obj.value or cloned.value == obj.value is always True regardless of behavior (P ∨ ¬P). It provides no test coverage.
• SerializationMixin.deepcopy hardcodes if k == "raw_representation" instead of using the already-existing DEFAULT_EXCLUDE class variable (or a new SHALLOW_COPY_FIELDS set). Any subclass with a different non-copyable excluded field will still crash on deepcopy. The mechanism to declare these fields already exists — use it.
• Content.deepcopy duplicates the same hardcoded logic. If Content inherits from SerializationMixin, the override is unnecessary. If it doesn't, it should have its own class-level set (e.g., _SHALLOW_COPY_FIELDS) rather than a hardcoded string.

Suggestions

• SerializationMixin.deepcopy hardcodes if k == "raw_representation" instead of checking if k in cls.DEFAULT_EXCLUDE. The test is named test_deepcopy_preserves_excluded_fields_by_reference implying it should generalize to all DEFAULT_EXCLUDE fields. Consider using if k in cls.DEFAULT_EXCLUDE for consistency and future-proofing.
• The test assertion assert cloned.value is not obj.value or cloned.value == obj.value on line 451 of test_serializable_mixin.py is a tautology (always True for any values) and does not actually verify that the string was independently copied. Consider removing it or replacing with a meaningful assertion on a mutable field.
• Consider checking if k in type(self).DEFAULT_EXCLUDE instead of hardcoding if k == "raw_representation" in SerializationMixin.__deepcopy__. This would make the deepcopy behavior consistent with the class's existing exclusion configuration, and would correctly handle subclasses that add other non-copyable fields to DEFAULT_EXCLUDE. Currently, the test name test_deepcopy_preserves_excluded_fields_by_reference implies generic DEFAULT_EXCLUDE support, but only raw_representation is actually handled.
• If Content inherits from SerializationMixin, the duplicated __deepcopy__ in Content could be removed in favor of the mixin's implementation, reducing maintenance surface.
• Add a test with a field in DEFAULT_EXCLUDE other than 'raw_representation' to verify whether the deepcopy behavior is truly generic or hardcoded. Currently test_deepcopy_preserves_excluded_fields_by_reference suggests generality but only tests the hardcoded key.
• Consider adding a test for circular/self-referencing objects to verify the memo dict handling works correctly (e.g., an object whose attribute refers back to itself).
• Consider adding a test for subclass deepcopy to verify the cls = type(self) pattern preserves the correct type.
• If the semantic of DEFAULT_EXCLUDE is too broad (it governs serialization, not copying), introduce a dedicated class variable like SHALLOW_COPY_FIELDS: ClassVar[set[str]] and have __deepcopy__ check against that. This keeps the concerns separate while still being extensible.

---

Automated review by moonbox3's agents

[Copilot]

Pull request overview

Fixes workflow event snapshotting when raw_representation contains non-deepcopy-able SDK objects by making deepcopy preserve raw_representation as a shallow reference while deep-copying all other fields. This prevents executor_completed events from losing response payloads in mixed-LLM workflows.

Changes:

• Added custom __deepcopy__ implementations to SerializationMixin and Content to shallow-copy raw_representation.
• Added regression coverage for deepcopy behavior across core response/message/content types.
• Added a workflow regression test validating executor_completed includes a valid AgentResponse when raw_representation is non-copyable.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
python/packages/core/agent_framework/_serialization.py	Adds SerializationMixin.__deepcopy__ to preserve raw_representation by reference.
python/packages/core/agent_framework/_types.py	Adds Content.__deepcopy__ with the same shallow raw_representation behavior.
python/packages/core/tests/core/test_types.py	Adds unit tests validating deepcopy preserves raw_representation across key core types.
python/packages/core/tests/core/test_serializable_mixin.py	Adds mixin-level tests for deepcopy behavior with excluded/non-excluded fields.
python/packages/core/tests/workflow/test_agent_executor.py	Adds workflow regression test covering executor_completed event data with non-copyable raw_representation.

---

You can also share your feedback on Copilot code review. Take the survey.

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 88%

✓ Correctness

The diff generalizes the __deepcopy__ logic from a single hardcoded field name to a set-based lookup, applied consistently in both SerializationMixin (using DEFAULT_EXCLUDE) and Content (using _SHALLOW_COPY_FIELDS). The changes are logically correct: set membership checks are O(1), the iteration over self.__dict__ is unchanged, and the test properly validates that multiple excluded fields are shallow-copied while other fields are deep-copied. The updated test is actually stricter than the old one, which contained a tautological assertion (cloned.value is not obj.value or cloned.value == obj.value is always True).

✓ Security Reliability

This diff generalizes two __deepcopy__ implementations to use a class-level set (DEFAULT_EXCLUDE / _SHALLOW_COPY_FIELDS) instead of a hardcoded string comparison, allowing multiple fields to be shallow-copied. The change is a straightforward, low-risk refactoring. No injection risks, unsafe deserialization, secrets, resource leaks, or unhandled failure modes were identified. The shallow-copy semantics are well-documented and the test coverage is updated appropriately.

✓ Test Coverage

The SerializationMixin.deepcopy change from a hardcoded field name to a set-based lookup is well-tested: the updated test now verifies multiple excluded fields are shallow-copied and normal fields are deep-copied with meaningful assertions. However, the parallel change in Content.deepcopy (switching from hardcoded 'raw_representation' to _SHALLOW_COPY_FIELDS) has no corresponding test coverage in this diff, leaving that code path unverified.

✗ Design Approach

The diff generalizes hardcoded raw_representation checks to use configurable field sets for shallow-copying during __deepcopy__. The Content class does this correctly by introducing a purpose-specific _SHALLOW_COPY_FIELDS set. However, SerializationMixin reuses DEFAULT_EXCLUDE (a serialization concern) to also drive deep-copy behavior, conflating two orthogonal concepts: 'fields to omit from serialization' and 'fields unsafe to deep-copy'. A field could legitimately be excluded from serialization while still being safe (and necessary) to deep-copy, and vice versa. This coupling will silently produce shallow references for any future field added to DEFAULT_EXCLUDE for serialization reasons alone, risking shared-mutation bugs.

Flagged Issues

• In SerializationMixin.__deepcopy__, reusing DEFAULT_EXCLUDE to decide which fields get shallow-copied conflates 'not serialized' with 'not safe to deep-copy'. These are independent concerns. A field added to DEFAULT_EXCLUDE purely because it shouldn't appear in serialized output will now also be silently shared between the original and its deep-copy, which can cause hard-to-diagnose mutation bugs. Content already models this correctly with a dedicated _SHALLOW_COPY_FIELDS set; SerializationMixin should do the same (e.g., introduce a _SHALLOW_COPY_FIELDS class var, defaulting to {'raw_representation'}, and use that in __deepcopy__).

Suggestions

• Consider whether DEFAULT_EXCLUDE (a serialization concept) is the right set to drive deepcopy shallow-copy semantics in SerializationMixin. The Content class introduces a dedicated _SHALLOW_COPY_FIELDS for this purpose, which is arguably cleaner because a field could be excluded from serialization for reasons unrelated to deep-copy safety. If the two sets are guaranteed to always be identical for SerializationMixin, this is fine; otherwise a dedicated set (like Content uses) would be more robust.
• Consider whether DEFAULT_EXCLUDE (used for serialization exclusion) and shallow-copy concern should remain coupled, or whether _SHALLOW_COPY_FIELDS (as done in _types.py) is a better pattern to adopt in SerializationMixin as well, to avoid a future subclass inadvertently getting shallow-copy behavior just because it excluded a mutable field from serialization.
• Add a test for Content.deepcopy that verifies _SHALLOW_COPY_FIELDS are preserved by reference (shallow-copied) while other attributes are deep-copied. The Content class received the same refactor as SerializationMixin but has no test coverage for the new set-based logic.
• Consider adding a test that verifies subclass override of _SHALLOW_COPY_FIELDS / DEFAULT_EXCLUDE works correctly (e.g., a subclass that extends the set with additional fields).

---

Automated review by moonbox3's agents

[moonbox3]

Automated Code Review

Reviewers: 4 | Confidence: 92%

✓ Correctness

The diff cleanly decouples deep-copy semantics from serialization exclusion by introducing _SHALLOW_COPY_FIELDS to replace the prior reuse of DEFAULT_EXCLUDE in __deepcopy__. The implementation is correct: the new class variable defaults to {"raw_representation"}, the __deepcopy__ method properly references it, and the tests thoroughly verify both the shallow-copy behavior for _SHALLOW_COPY_FIELDS members and the deep-copy behavior for DEFAULT_EXCLUDE members that are no longer shallow-copied. No correctness issues found.

✓ Security Reliability

Clean refactor that decouples deepcopy shallow-copy behavior from serialization exclusion by introducing a dedicated _SHALLOW_COPY_FIELDS class variable. This is a reliability improvement: DEFAULT_EXCLUDE fields like additional_properties are now correctly deep-copied, preventing unintended shared-state mutations between original and cloned objects. No security or reliability issues found. Tests adequately cover the new behavior and the regression case.

✓ Test Coverage

The diff decouples deepcopy behaviour from DEFAULT_EXCLUDE by introducing _SHALLOW_COPY_FIELDS. The existing tests were properly renamed and updated, and three new tests were added covering the key behavioural change (DEFAULT_EXCLUDE fields are now deep-copied, _SHALLOW_COPY_FIELDS remain shallow, integration tests on Content and ChatResponse). Coverage is solid for the main scenarios. One minor gap: no test verifies the interaction when a field appears in both DEFAULT_EXCLUDE and _SHALLOW_COPY_FIELDS simultaneously, which would confirm _SHALLOW_COPY_FIELDS takes precedence. This is a suggestion, not a blocker.

✓ Design Approach

The change correctly identifies and fixes a design flaw where DEFAULT_EXCLUDE (a serialization concern) was conflated with deep-copy safety (a copy concern). Introducing _SHALLOW_COPY_FIELDS properly separates these orthogonal responsibilities, and the tests verify the decoupling. The approach is sound and the implementation is clean.

Suggestions

• Add a test for the edge case where a field is listed in both DEFAULT_EXCLUDE and _SHALLOW_COPY_FIELDS, confirming _SHALLOW_COPY_FIELDS wins and the field is shallow-copied. This documents the intended precedence.
• Consider adding a simple assertion that SerializationMixin._SHALLOW_COPY_FIELDS == {'raw_representation'} to lock down the base-class default and catch accidental changes.

---

Automated review by moonbox3's agents

[markwallace-microsoft]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/core/agent_framework
_serialization.py	120	4	96%	516, 533, 543, 611
_types.py	1025	72	92%	58, 67–68, 122, 127, 146, 148, 152, 156, 158, 160, 162, 180, 184, 210, 232, 237, 242, 246, 276, 671–672, 1180, 1251, 1268, 1286, 1304, 1314, 1358, 1490–1492, 1680, 1771–1776, 1801, 1969, 1981, 2230, 2251, 2346, 2575, 2778, 2846, 2861, 2882, 3087–3089, 3092–3094, 3098, 3103, 3107, 3191–3193, 3222, 3276, 3295–3296, 3299–3303, 3309
TOTAL	22695	2815	87%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
4714	20 💤	0 ❌	0 🔥	1m 14s ⏱️