.NET: fix: JSON Serialization issue with MultiPartyConversation#5653
.NET: fix: JSON Serialization issue with MultiPartyConversation#5653
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to fix .NET workflow checkpoint/session round-tripping for handoff workflows by making handoff-specific state types available to the workflows JSON source generator and by exposing MultiPartyConversation history for serialization.
Changes:
- Added source-generated JSON metadata for
HandoffSharedStateandHandoffAgentHostState. - Updated
MultiPartyConversationwith JSON serialization attributes and a deserialization constructor for chat history. - Added JSON round-trip unit tests covering handoff shared/host state serialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
dotnet/tests/Microsoft.Agents.AI.Workflows.UnitTests/JsonSerializationTests.cs |
Adds regression tests for handoff state JSON round-tripping. |
dotnet/src/Microsoft.Agents.AI.Workflows/WorkflowsJsonUtilities.cs |
Registers handoff state types in the workflows JSON source-generation context. |
dotnet/src/Microsoft.Agents.AI.Workflows/Specialized/MultiPartyConversation.cs |
Exposes conversation history to STJ and adds constructor-based deserialization. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 77%
✗ Correctness
The
MultiPartyConversationJSON constructor has a parameter name mismatch that will break deserialization. The constructor parameterhistorydoesn't match the field name_history(case-insensitive), so System.Text.Json cannot bind the serialized_historyJSON property to the constructor parameter. The existingSessionCheckpointCachein this codebase correctly uses matching names (e.g., propertyCheckpointIndex↔ parametercheckpointIndex). Additionally,HandoffSharedState.Conversationis a get-only property of a non-collection type with no[JsonConstructor], so it cannot be deserialized withoutJsonObjectCreationHandling.Populate.
✓ Security Reliability
The PR adds JSON serialization support for MultiPartyConversation, HandoffSharedState, and HandoffAgentHostState types. The main reliability concern is the lack of null-guard in the new JsonConstructor for MultiPartyConversation — if deserialized with a null history (from corrupted checkpoint data or a serializer naming mismatch), all methods will throw NullReferenceException. The change is otherwise straightforward and well-tested.
✓ Test Coverage
The PR adds JSON serialization support for MultiPartyConversation, HandoffSharedState, and HandoffAgentHostState, with four new roundtrip tests. Test coverage is adequate: it covers empty and populated HandoffSharedState, plus HandoffAgentHostState with and without IncomingState (taking-turn vs not-taking-turn). The tests implicitly verify that the [JsonConstructor] on MultiPartyConversation works correctly and that _mutex is initialized post-deserialization (via CloneAllMessages which locks). No significant test coverage gaps were found.
✗ Design Approach
I found one blocking design issue. The change adds source-generated JSON metadata and a serializable wrapper for
MultiPartyConversation, but it still does not giveSystem.Text.Jsona supported way to restore theConversationobject insideHandoffSharedState, so the checkpointing fix is likely aimed at the symptom rather than the actual deserialization contract.
Flagged Issues
- HandoffSharedState.Conversation is a getter-only property (HandoffStartExecutor.cs:24-28) of a non-collection custom type, with no [JsonConstructor] on HandoffSharedState and no JsonObjectCreationHandling.Populate configured. Registering the types in WorkflowsJsonUtilities does not actually make the shared conversation round-trip. A more durable fix is to give HandoffSharedState a settable/init-only serialized shape or explicitly enable populate semantics.
Automated review by lokitoth's agents
When MultiPartyConversation gets saved during checkpointing, the data for the chat history is not persisted, resulting in failures to deserialize after. The fix is to make the history visible to the source generated serialization code.
5d9eb8e to
8383e05
Compare
Motivation and Context
When MultiPartyConversation gets saved during checkpointing, the data for the chat history is not persisted, resulting in failures to deserialize after.
Huge thanks to @alex-bush for finding and root-causing the issue!
Description
The fix is to make the history visible to the source generated serialization code.
Contribution Checklist
- [ ] Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.