Python: [Breaking] Additional bug fix for declarative workflows#6489
Python: [Breaking] Additional bug fix for declarative workflows#6489peibekwe wants to merge 9 commits into
Conversation
github-actions
Bot
changed the title
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR adjusts the declarative workflow approval/yield-resume flow so that resumed tool invocations are bound to the framework-delivered original_request payload (the exact data that was approved), rather than relying on side-channel state stored under per-executor keys.
Changes:
- Update
InvokeFunctionToolExecutorandInvokeMcpToolActionExecutorto resume fromoriginal_requestand to pass an explicitrequest_idintoctx.request_info(...)so the framework pending key matches the payload ID. - Remove internal “approval state” persistence/cleanup logic and update existing unit tests accordingly.
- Add a dedicated regression test suite pinning the approval-binding contract (including concurrency/out-of-order resume scenarios) and surface MCP
connection_namein approval request UX.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| python/samples/03-workflows/declarative/invoke_mcp_tool/main.py | Displays MCP connection_name in the approval prompt output. |
| python/packages/declarative/tests/test_invoke_mcp_tool_executor.py | Updates MCP approval-flow tests to reflect payload-bound resume behavior. |
| python/packages/declarative/tests/test_function_tool_executor.py | Removes state-key-based approval assertions; aligns tests with payload-bound resume. |
| python/packages/declarative/tests/test_declarative_approval_binding.py | Adds regression tests for approval request/response binding and concurrency behavior. |
| python/packages/declarative/agent_framework_declarative/_workflows/_executors_tools.py | Stops storing approval state; uses explicit request IDs and resumes from original_request. |
| python/packages/declarative/agent_framework_declarative/_workflows/_executors_mcp.py | Stops storing approval state; adds connection_name to request payload and resumes from original_request. |
| python/packages/declarative/agent_framework_declarative/_workflows/init.py | Removes exports related to prior approval-state persistence helpers. |
![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: 3 | Confidence: 92%
✓ Correctness
Clean refactoring that removes side-channel approval state (ToolApprovalState, _MCPToolApprovalState) in favor of sourcing invocation parameters directly from the original_request payload delivered by the framework. The design eliminates potential 'approve X then call Y' attacks, supports checkpoint-restore without prior state, handles concurrent pending approvals correctly, and keeps secrets out of checkpoint state. The code is well-tested with comprehensive regression tests. No correctness issues found.
✓ Failure Modes
This PR cleanly removes stateful side-channel approval state (ToolApprovalState, _MCPToolApprovalState) in favor of using the original_request payload as the source of truth for resumed invocations. The request_info API accepts the request_id kwarg correctly, output configuration is safely re-derived from the immutable action definition, and headers are properly re-evaluated on resume. No concrete silent failure paths or operational failure modes were identified in the changed code.
✓ Design Approach
The approval-binding fix correctly pins MCP invocation inputs to the approved request, but the MCP resume path still re-derives result-routing behavior from mutable workflow state. That leaves a silent post-approval drift where an approved tool call can write/send its output to a different conversation or visibility path than the one in effect when approval was requested.
Automated review by peibekwe's agents
peibekwe
changed the title
| tool_name = original_request.tool_name | ||
| server_url = original_request.server_url | ||
| server_label = original_request.server_label | ||
| arguments = original_request.arguments | ||
| connection_name = getattr(original_request, "connection_name", None) | ||
| metadata: dict[str, Any] = getattr(original_request, "metadata", None) or {} |
There was a problem hiding this comment.
Nit: do we need to unpack all of these, most seem to be used only once
Description
Additional bug fix for declarative workflows
Contribution Checklist