Python: Resolve workflow request info from pending state - #7500
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
Add authoritative pending request lookup and workflow-agent resolution, retain legacy parsing through trusted compatibility mappings, and migrate first-party consumers while preserving the existing wire shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a53fe20b-c3f0-4583-badc-d5deac7c1049
c40e830 to
68ecde7
Compare
GitHub Actions
Inactive
Motivation & Context
Keep workflow request-info resolution consistent with retained pending state while preserving existing transport and type compatibility.
Description & Review Guide
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.