Python: Prevent workflow checkpoints from being mutated outside of storage implementations - #7847
Python: Prevent workflow checkpoints from being mutated outside of storage implementations#7847Tao Chen (TaoChenOSU) wants to merge 3 commits into
Conversation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Strengthens Python workflow checkpoint isolation across state and storage boundaries.
Changes:
- Deep-copies state during export and restoration.
- Returns copied checkpoints from in-memory storage.
- Adds state, runner, and storage conformance tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
python/packages/core/agent_framework/_workflows/_state.py |
Adds deep-copy state boundaries. |
python/packages/core/agent_framework/_workflows/_checkpoint.py |
Documents ownership and copies in-memory reads. |
python/packages/core/tests/workflow/test_state.py |
Tests nested state isolation. |
python/packages/core/tests/workflow/test_runner.py |
Tests storage-less restoration isolation. |
python/packages/core/tests/workflow/test_checkpoint_storage_conformance.py |
Adds backend ownership conformance tests. |
python/packages/core/agent_framework/_workflows/_agent.py |
Formatting-only change. |
python/packages/core/agent_framework/_harness/_loop.py |
Formatting-only changes. |
python/packages/core/tests/core/test_harness_loop.py |
Import-order-only change. |
Suppressed comments (1)
python/packages/core/agent_framework/_workflows/_state.py:121
- The same custom
__deepcopy__behavior means restore isolation is still incomplete: importing a checkpoint containingContent/otherSerializationMixinvalues copiesraw_representationby reference, so later live-state mutation can mutate the caller's checkpoint. Please use a checkpoint-specific clone/sanitization policy rather than relying on genericdeepcopyfor the restoration boundary.
self._committed.update(copy.deepcopy(state))
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): a5062c6c7bc2
Model: gpt-5.6-sol
Overview
The review found 2 verified inline finding(s).
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 2 files. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_checkpoint.py, python/packages/core/agent_framework/_workflows/_state.py
There was a problem hiding this comment.
MAF Automated Review — Iteration 2
Result: Findings reported
Scope: 2 net-new commit(s): 588d16565ae6, ae8cd879229b
Model: gpt-5.6-sol
Overview
The review found 2 verified inline finding(s).
Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 medium) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_state.py
|
Tao Chen (@TaoChenOSU) I also found and opened #7859 while working on the state/checkpoint behavior addressed in this PR. #7859 is a separate issue: pending The underlying state consistency area is related to this PR, but the trigger and fix are different:
I can implement the #7859 fix separately with a focused regression test, or include it in this PR if you would prefer to keep the related state-consistency fixes together. #7859 is currently under triage, so please let me know which approach you prefer. |
Shivani . (Shivani767)
left a comment
There was a problem hiding this comment.
Thanks for pulling this together, Tao Chen (@TaoChenOSU) — this is a solid consolidation of the ownership work from #7712 / #7697.
What looks good
- The
CheckpointStoragemethod docs now state the copy-on-save / copy-on-read contract clearly, andInMemoryCheckpointStoragematches it onsave,load,list_checkpoints, andget_latest. - Moving conformance into
test_checkpoint_storage_conformance.py(with both memory and file backends) is much easier to maintain than growingtest_checkpoint.pyfurther. - Extending
_conformance_checkpointto cover nested mutables instate,messages,pending_request_info_events, andmetadataaddresses the earlier completeness gap. - Isolating
Stateatset/get/export_state/import_stateis the right companion fix for #7685; the new state tests (including the get → mutate → set path) make the intended caller contract explicit.
Follow-up (already tracked)
- Agree with treating the intentional
Content/SerializationMixinraw_representationaliasing underdeepcopyas out of scope for this PR and tracking it in #7851.
Nit (non-blocking)
- The formatting-only edits in
_harness/_loop.pyand_workflows/_agent.pylook unrelated to the ownership fix; fine if they came from repo formatters, otherwise they could be dropped to keep the diff focused.
LGTM from my side. Appreciate you taking the broader fix in one PR.
Thanks! Let's keep this PR scoped and address the other issue in another PR. Feel free to work on it. Much appreciated! |
Motivation & Context
Closes #7683 and #7685
Supersedes #7697, #7712, #7830, #7848
Description & Review Guide
Related Issue
Fixes #7683, #7685
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.