Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves state persistence APIs to work better with PortableValue by fixing serialization round-trip issues and preventing object type abstractions that can leak JsonElement objects. The changes ensure PortableValue can serve as a supertype for all stored values without double-wrapping or deserialization cache corruption.
Key changes:
- Enhanced StateManager and StateScope to properly handle PortableValue as a wrapper type
- Fixed PortableValue deserialization caching bugs that caused corruption and exceptions
- Added comprehensive test coverage for PortableValue round-trip scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| StateManagerTests.cs | Added comprehensive tests for PortableValue state persistence scenarios |
| PortableValueTests.cs | New test file covering PortableValue serialization round-trips and delayed deserialization |
| JsonSerializationTests.cs | Made helper methods internal for reuse in new tests |
| PortableValue.cs | Fixed deserialization caching logic and added robust cache management |
| StateScope.cs | Added special handling for PortableValue wrapper behavior |
| StateManager.cs | Enhanced to prevent object type reads and handle PortableValue wrapping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
6770b01 to
a965764
Compare
crickman
approved these changes
Oct 9, 2025
a965764 to
98453e3
Compare
TaoChenOSU
approved these changes
Oct 9, 2025
Member
Author
|
PR failed to merge because IT fails due to breaking change on |
crickman
approved these changes
Oct 9, 2025
ReubenBond
pushed a commit
to ReubenBond/agent-framework
that referenced
this pull request
Oct 28, 2025
…microsoft#1367) * fix: Make State Persistence APIs work better with PortableValue * test: Temporarily disable checking for T=object in ReadStateAsync
arisng
pushed a commit
to arisng/agent-framework
that referenced
this pull request
Feb 2, 2026
…microsoft#1367) * fix: Make State Persistence APIs work better with PortableValue * test: Temporarily disable checking for T=object in ReadStateAsync
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
With the changes to make our type system work a better with transparent serialization round-tripping during checkpointing, we replaced the
object-based catch-all inMessageRouterandRouteBuilderwith one takingPortableValue, sinceobjectas a target-type can result in truncation or leakingJsonElementobjects when interacting with JSON serialization.We did not make a similar change to state persistence, which means that it is unclear how to get a "supertype" of everything to operate on when a specific type is not known. Users could get into trouble writing arbitrary types and trying to read back
object. They could usePortableValuebut this cauesIt also exposed a pair of bugs:
IDelayedDeserialization(and would leak if requested to convert toobject),PortableValuethrows out ofIs<T>/As<T>.Description
PortableValuewill correctly wrap non-PV objects in it, but also avoid double-wrapping if it was stored as a PV in the first place.Closes #1361
Contribution Checklist
Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.