Python: Add Python parity sample for invoking Foundry Toolbox tools from declarative workflows#5933
Python: Add Python parity sample for invoking Foundry Toolbox tools from declarative workflows#5933peibekwe wants to merge 3 commits into
Conversation
…arative workflows
github-actions
Bot
changed the title
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Adds a Python declarative workflow sample that mirrors the .NET Foundry Toolbox MCP sample, including toolbox provisioning, authenticated MCP invocation, and summarization through a registered Foundry-backed agent.
Changes:
- Adds a new workflow YAML that lists toolbox tools, invokes Microsoft Learn Docs search and Foundry web search, then summarizes results.
- Adds a Python host script to provision the toolbox, configure MCP authentication, run the workflow, and stream progress/output.
- Adds workflow PowerFx access to environment variables via
Env.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
python/samples/03-workflows/declarative/invoke_foundry_toolbox_mcp/workflow.yaml |
Defines the declarative workflow steps for invoking Foundry Toolbox MCP tools and summarizing results. |
python/samples/03-workflows/declarative/invoke_foundry_toolbox_mcp/main.py |
Adds the runnable Python sample host, toolbox provisioning, authenticated MCP handler, and streaming UI. |
python/packages/declarative/agent_framework_declarative/_workflows/_declarative_base.py |
Exposes process environment variables to workflow PowerFx symbols as Env. |
Two security fixes for PR #5933: 1. Add safe_mode flag to WorkflowFactory (default True) mirroring AgentFactory. Gates =Env.* exposure inside DeclarativeWorkflowState PowerFx symbols via _safe_mode_context, so workflow YAML loaded from untrusted sources no longer leaks the host's full os.environ snapshot into PowerFx evaluation. The flag is also forwarded to the internally-constructed AgentFactory so inline agent definitions follow the same policy. 2. Pin the invoke_foundry_toolbox_mcp sample's _client_provider to the resolved toolbox endpoint. The bearer-authenticated httpx client is now only returned when MCPToolInvocation.server_url matches the toolbox URL case-insensitively; any other URL gets None (the default unauthenticated path), preventing the Foundry AAD bearer token from being attached to a mis-configured or injected server URL. Mirrors the .NET sample's httpClientProvider guard. The sample is updated to opt in to safe_mode=False because its YAML intentionally uses =Env.FOUNDRY_TOOLBOX_* to keep configuration in env vars under the developer's control. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![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: 4 | Confidence: 79%
✓ Correctness
The PR correctly implements safe_mode gating for environment variable exposure in PowerFx expressions and provides a well-structured sample. The contextvar approach is consistent with the existing _try_powerfx_eval usage in _models.py. The URL guard in the sample's client_provider properly prevents token leakage. No blocking correctness issues found.
✓ Security Reliability
The PR properly addresses both previously raised security concerns. Environment variable exposure via PowerFx
Envsymbol is now gated behind the_safe_mode_contextContextVar (default True/safe), matching the existingAgentFactorypattern in_loader.py. The sample's_client_providervalidatesinvocation.server_urlagainst the resolvedtoolbox_endpointbefore attaching the bearer token, preventing credential leakage to unexpected URLs. TheWorkflowFactory.safe_modeparameter defaults to True and must be explicitly opted out. No new security or reliability issues were identified that meet the evidence threshold.
✓ Test Coverage
The PR adds production code changes to
_factory.py(newsafe_modeparameter, contextvar propagation) and_declarative_base.py(newEnvsymbol exposure in_to_powerfx_symbolsgated by_safe_mode_context) but includes no new tests for these code paths. While_try_powerfx_eval's Env gating is tested intest_declarative_models.py, the parallel_to_powerfx_symbolspath and theWorkflowFactory.safe_modeintegration are untested. The sample code itself doesn't need tests, but the library changes do.
✓ Design Approach
I found two design issues. First, the sample's URL guard does not actually fail closed: on a mismatched
serverUrlit still letsDefaultMCPToolHandleropen a connection to that URL with an internally created client. Second, the new workflow-levelsafe_modewiring stores policy in an ambientContextVarat build time, but Env access is checked again at evaluation time, so one workflow factory can silently change another workflow's behavior in the same task.
Automated review by peibekwe's agents
| if invocation.server_url.casefold() != toolbox_endpoint.casefold(): | ||
| print( | ||
| f"[security] Refusing to attach Foundry bearer token to unexpected MCP URL: " | ||
| f"{invocation.server_url}", | ||
| file=sys.stderr, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
This does not fail closed. DefaultMCPToolHandler falls back to an internally-created client when client_provider returns None (_mcp_handler.py:177-182), and _create_entry() still connects to invocation.server_url even without a provided client (_mcp_handler.py:422-437). A tampered serverUrl is still contacted—it just loses the bearer token. Raise an exception instead of returning None to actually reject unexpected URLs, matching the sample's stated intent.
| if invocation.server_url.casefold() != toolbox_endpoint.casefold(): | |
| print( | |
| f"[security] Refusing to attach Foundry bearer token to unexpected MCP URL: " | |
| f"{invocation.server_url}", | |
| file=sys.stderr, | |
| ) | |
| return None | |
| if invocation.server_url.casefold() != toolbox_endpoint.casefold(): | |
| raise ValueError( | |
| f"Unexpected MCP server URL: {invocation.server_url}; expected {toolbox_endpoint}" | |
| ) |
| # contextvar propagates through asyncio tasks spawned from the current | ||
| # context, this value persists into ``workflow.run(...)`` invocations | ||
| # made on the same coroutine. | ||
| _safe_mode_context.set(self.safe_mode) |
There was a problem hiding this comment.
Should we be holding the reset token here? ContextVar.set without .reset(token) mutates the calling context for the rest of its life. If a process creates factory A with safe_mode=False, then factory B with safe_mode=True, then runs workflow A, the PowerFx evaluation reads B's value (True) and Env.* is silently dropped.
The comment promises propagation into workflow.run(...) on the same coroutine, but it also propagates into every subsequent create_workflow_from_definition call, regardless of which factory invokes it. Same shape exists in _loader.py already, so this PR isn't introducing the pattern, but it widens the surface to a new public flag.
Could we instead stash safe_mode on the workflow object and .set it inside workflow.run with a try/finally reset, or run evaluation inside contextvars.copy_context().run(...)?
| # ``safe_mode`` flag. Treat the default as safe even when no factory | ||
| # is in scope (e.g. in unit tests) so opt-in is required. | ||
| if not _safe_mode_context.get(): | ||
| symbols["Env"] = dict(os.environ) |
There was a problem hiding this comment.
Have we thought about scoping this to only the env vars the YAML references? dict(os.environ) hands every PowerFx expression a full snapshot, so any expression in any action of an opted-in workflow can read AZURE_CLIENT_SECRET, OPENAI_API_KEY, etc. Documented as trust-the-YAML, but a single =Concat(Env.SOME_SECRET, ...) slipping into a logged field or a tool argument now exfiltrates. Could we parse referenced Env.X names up front and expose only those, or wrap in a lazy view that records access for audit?
| name=toolbox_name, | ||
| api_version=toolbox_api_version, | ||
| ) | ||
| os.environ["FOUNDRY_TOOLBOX_MCP_SERVER_URL"] = toolbox_endpoint |
There was a problem hiding this comment.
Is mutating os.environ the only way to feed these into the YAML? It sets process-wide state from a sample and only works because safe_mode=False exposes the full env. Anything else loaded later in the same process inherits these values. Could the YAML reference Workflow.Inputs.* or a binding instead, so the sample doesn't model "shove config through process env" as the recommended pattern?
Motivation and Context
Add Python parity sample for invoking Foundry Toolbox tools from declarative workflows.
Matches #5829 for dotnet.
Fixes #5932
Contribution Checklist