Python: Restrict workflow type deserialization - #7500
Python: Restrict workflow type deserialization#7500Evan Mattson (moonbox3) wants to merge 1 commit into
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
documentation
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Introduces an authoritative, pending-state-based resolver for Python workflow request_info handling (including workflow-as-agent HITL), avoiding reliance on copied transport payloads for request data and Python type resolution while keeping the current wire shape compatible during a deprecation window.
Changes:
- Add
Workflow.get_pending_request_info(request_id)andWorkflowAgent.resolve_request_info(content)to resolve pending request-info data from workflow-held state with strict correlation validation. - Replace import-based type resolution with a safer compatibility mechanism (
serialize_typeregistry +deserialize_typeallowlist/registry/builtins), and deprecate legacy dict parsers with warnings. - Update first-party samples/docs and expand unit test coverage for tampering, correlation semantics, checkpoint rehydration, and generic response-type compatibility.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| python/samples/03-workflows/README.md | Updates sample description to reflect authoritative pending request resolution for workflow-as-agent HITL. |
| python/samples/03-workflows/orchestrations/README.md | Updates handoff workflow-as-agent sample description to reflect authoritative HITL request resolution. |
| python/samples/03-workflows/agents/workflow_as_agent_human_in_the_loop.py | Migrates sample to agent.resolve_request_info(...) and uses authoritative request IDs for responses. |
| python/samples/03-workflows/agents/handoff_workflow_as_agent.py | Migrates request extraction to agent.resolve_request_info(...) and adds runtime type validation. |
| python/packages/orchestrations/tests/test_handoff.py | Adds coverage ensuring workflow-as-agent request-info remains usable with generic response type metadata. |
| python/packages/core/tests/workflow/test_workflow_agent.py | Adds extensive resolve_request_info tests (correlation, tampering, replay/stale behavior, data non-trust). |
| python/packages/core/tests/workflow/test_typing_utils.py | Adds tests for safer type serialization/deserialization, allowlists, ambiguity handling, and non-import behavior. |
| python/packages/core/tests/workflow/test_request_info_event_rehydrate.py | Adds tests ensuring legacy reconstruction paths don’t import payload-selected modules and validates restored pending lookup. |
| python/packages/core/tests/workflow/test_request_info_and_response.py | Adds tests for non-consuming pending lookup and consumption semantics after successful responses. |
| python/packages/core/AGENTS.md | Documents the request-info trust boundary and preferred resolution APIs; notes legacy deprecations. |
| python/packages/core/agent_framework/_workflows/_workflow.py | Adds get_pending_request_info public API backed by runner context pending state. |
| python/packages/core/agent_framework/_workflows/_typing_utils.py | Reworks serialize_type/deserialize_type to avoid payload-driven imports and support explicit trusted mappings/registry. |
| python/packages/core/agent_framework/_workflows/_events.py | Deprecates WorkflowEvent.from_dict, adds allowed_types, and routes through safer deserialize_type. |
| python/packages/core/agent_framework/_workflows/_agent.py | Deprecates legacy request-info args parsing and adds resolve_request_info to validate and resolve authoritative pending events. |
| docs/decisions/0033-authoritative-workflow-request-info-resolution.md | Adds ADR documenting the design decision, drivers, and migration plan. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 49%
✗ Correctness
Found one backward-compatibility bug: legacy typing-generic wire names are serialized but can no longer be deserialized.
✓ Security Reliability
I could not complete a verifiable review: every read tool call (view, grep, bash) in this session was blocked by the environment, so I was unable to open the changed production files (_typing_utils.py, _events.py, _agent.py, _workflow.py) or the surrounding runner-context/checkpoint code needed to confirm behavior. Reading the diff alone, the change is coherent with its stated rationale: deserialize_type no longer imports payload-selected modules and resolves only against builtins, a process-local weakref registry, and exact per-call allowed_types; Workflow.get_pending_request_info provides a non-consuming authoritative lookup; and WorkflowAgent.resolve_request_info strictly correlates call_id / arguments.request_id / request_event.request_id and compares only safely comparable metadata while ignoring copied request data. The main residual risk I would want a maintainer to consider is the deliberate loss of cross-process type resolution (a serialized name unknown to the current process now raises ValueError instead of importing), which the ADR explicitly acknowledges as a compatibility-window trade-off. I am reporting zero findings because I could not verify any concrete issue against source.
✓ Test Coverage
The generic compatibility test covers only serialization, missing a backward-compatibility regression in deserialization.
✓ Failure Modes
I could not perform this review. All read-only tool calls in this session (view, grep, bash) were rejected by the environment with 'unexpected user permission response', so I was unable to open python/packages/core/agent_framework/_workflows/_typing_utils.py, _agent.py, _workflow.py, _events.py, the checkpoint/serialization code paths, or the test files needed to verify any suspected failure mode. Per the evidence discipline requirements, I am omitting all findings rather than emitting unverified claims. Areas that a reviewer with working file access should verify: (1) deserialize_type no longer imports modules, so any cross-process consumer that previously relied on import-based resolution (e.g. checkpoint restore in a fresh process, distributed/devui hosts) must be confirmed to resolve types through serialize_type registration or an explicit allowed_types mapping, otherwise restore fails closed with 'Unknown serialized type'; (2) the process-global weakref registry in serialize_type and its Lock interacting with async event loops; (3) the warnings.catch_warnings() suppression inside RequestInfoFunctionArgs.from_dict, which mutates global warning filter state and is not thread-safe (note: the test asserting exactly one recorded warning appears to depend on this suppression, so any change there must keep that test passing).
✓ Design Approach
I was unable to perform the required verification pass: every file-read tool call in this session (view, grep, bash) failed with a permission error, so I could not open _typing_utils.py, _workflow.py, _agent.py, the checkpoint serialization paths, or any existing tests in the checkout. Reviewing the diff text alone, the design is coherent and internally consistent: pending workflow state becomes the single authority for request-info data and Python types, Workflow.get_pending_request_info is non-consuming and delegates to the runner context, WorkflowAgent.resolve_request_info validates only safely-comparable correlation and metadata fields while ignoring copied request data, and the legacy from_dict entry points are retained behind DeprecationWarnings with an allowed_types escape hatch so the wire shape is unchanged. The tests in the diff directly assert each of these properties (identity-preserving repeated lookup, consumption only on successful response, tamper rejection per field, no-import type resolution, restored-checkpoint lookup). The one area I would have wanted to check but could not is whether any cross-process checkpoint-restore path depends on deserialize_type resolving a type that was never passed through serialize_type in the loading process, since the registry is process-local; because I could not open the checkpoint code to confirm or refute that, I am omitting it as a finding rather than reporting it unverified. No findings are emitted.
Flagged Issues
-
python/packages/core/agent_framework/_workflows/_typing_utils.py:351leaves legacy typing aliases such astyping.Listunregistered and unresolvable during deserialization.
Automated review by moonbox3's agents
|
Flagged issue
Source: automated DevFlow PR review |
GitHub Actions
Inactive
GitHub Actions
Inactive
|
Addressed the DevFlow compatibility finding in 55ba857: supported historical |
55ba857 to
c40e830
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
c40e830 to
68ecde7
Compare
GitHub Actions
Inactive
There was a problem hiding this comment.
Agent Framework Skill Review
Completed passes: 5
Review passes
- Correctness (
gpt-5.6-sol) — No high-confidence issues found in this pass. - Security Reliability (
claude-opus-4.8) — No high-confidence issues found in this pass. - Test Coverage (
gpt-5.6-sol) — No high-confidence issues found in this pass. - Failure Modes (
claude-opus-4.8) — No high-confidence issues found in this pass. - Design Approach (
claude-opus-4.8) — No high-confidence issues found in this pass.
No high-severity findings survived the skill-backed review and verification pipeline.
Reviewed by moonbox3's agents
68ecde7 to
1e3f667
Compare
GitHub Actions
Active
1e3f667 to
5957b31
Compare
GitHub Actions
Active
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 5957b31d065b
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 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_workflows/_typing_utils.py
5957b31 to
d061cfc
Compare
GitHub Actions
Active
Evan Mattson (moonbox3)
changed the title
GitHub Actions
Active
Resolve request-info type names only from exact caller-provided mappings or already-loaded module namespaces. Remove payload-selected imports and add focused regression coverage for both request and response type fields. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a53fe20b-c3f0-4583-badc-d5deac7c1049
d061cfc to
52932ca
Compare
GitHub Actions
Active
|
/review |
There was a problem hiding this comment.
MAF Automated Review — Iteration 2
Result: No findings
Scope: full PR (1 commit(s)): 52932cadfe16
Model: gpt-5.6-sol
Overview
The change replaces payload-selected imports with exact caller allowlists or lookups in already-loaded module namespaces while preserving the existing wire shape and optional API compatibility. Exact-name validation, raw module-dictionary lookup, runtime-type checks, and no-import tests provide concrete guardrails. The compatibility risks identified by the panel overlap supplied prior findings or existing feedback, so no new publishable Critical/High/Medium finding remains.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
Motivation & Context
Make workflow request-info type reconstruction explicit and predictable.
Description & Review Guide
allowed_types.from_dictAPIs.Related Issue
None.
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.