[Manohar Paturi (ManoharPaturi)]

Motivation & Context

get_mcp_tool accepts an approval_mode dict and its docstring says the always_require_approval and never_require_approval keys can be used together. When both were supplied, the second assignment overwrote the first, so the always list was silently dropped and those tools were sent to the API as auto approved. The declarative loader already builds the combined mapping for the same API field, so the static helper was the outlier.

Description & Review Guide

(Scope update: the same two assignment overwrite exists in FoundryChatClient.get_mcp_tool, so the combined mapping fix and a matching regression test are applied there too, per review.)

• What are the major changes? The two branches now build one combined require_approval mapping instead of assigning it twice. Single key behavior is unchanged.
• What is the impact of these changes? Tools listed under always_require_approval keep requiring approval when a never list is also present. No other call sites are affected.
• What do you want reviewers to focus on? The merged shape mirrors _loader.py for McpServerToolSpecifyApprovalMode, which is the closest sibling for the same field.

Related Issue

Fixes #9498

AI Assistance

• No material AI assistance was used.
• This is an AI-assisted contribution. I reviewed, understood, and verified all submitted content and accept responsibility for it.

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR links to an agreed issue with no competing open PR, or the Related Issue section documents a trivial-change or repository-automation exception.
• This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

[Eduard van Valkenburg (eavanvalkenburg)]

The OpenAI helper fix is correct, but the same public approval-mode contract remains broken in the Foundry helper.

[Eduard van Valkenburg (eavanvalkenburg)]

Please apply this same combined-mapping fix and regression coverage to FoundryChatClient.get_mcp_tool in python/packages/foundry/agent_framework_foundry/_chat_client.py. That public sibling currently has the identical two-assignment overwrite, so supplying both lists still drops always_require_approval for Foundry users and silently weakens the requested approval policy.

[Manohar Paturi (ManoharPaturi)]

Done. FoundryChatClient.get_mcp_tool now builds the same combined require_approval mapping, with a matching regression test asserting both lists survive together. It fails on the previous head of this branch and passes now, the full foundry chat client suite passes (104 tests), and ruff is clean on both files. Amended into the branch with the title updated to name both helpers.

[Manohar Paturi (ManoharPaturi)]

The Public API Compatibility check fails on every open PR in this repo right now (e.g. #9602, #9607, and all of this author's open PRs), so it looks like a repo-wide CI issue rather than something introduced by this change.

The job crashes inside griffe while loading the base (main) snapshot, before it compares the PR changes — the traceback points at the base copy of the package:

griffe._internal.exceptions.AliasResolutionError: Could not resolve alias
agent_framework._workflows._edge.EdgeGroup.builtin_type pointing at builtins.type
(in /tmp/python-api-base-.../python/packages/core/agent_framework/_workflows/_edge.py:315)

Example run: https://github.com/microsoft/agent-framework/actions/runs/39810016173/job/119933441343

None of the changes in this PR touch _workflows/_edge.py. Filed an issue for the CI failure; happy to help debug if useful.

[Eduard van Valkenburg (eavanvalkenburg)]

Manohar Paturi (@ManoharPaturi) Please take a look at the inline typed-mapping finding.

[Eduard van Valkenburg (eavanvalkenburg)]

Manohar Paturi (@ManoharPaturi) The combined accumulator is typed as dict[str, Any], but mcp["require_approval"] requires the SDK’s McpRequireApprovalMcpToolApprovalFilter mapping. This assignment fails Pyright and Pyrefly on the current head. Please initialize the accumulator with McpRequireApprovalMcpToolApprovalFilter() instead of a generic dictionary, preserving the combined-list logic and empty-mapping guard.

[Manohar Paturi (ManoharPaturi)]

Confirmed and fixed in c744def. The accumulator is now McpRequireApprovalMcpToolApprovalFilter with its Always and Never filter objects constructed explicitly, so the require_approval assignment type checks against the Mcp tool parameter; the combined list logic and the empty mapping guard are unchanged, as is the emitted JSON. Verified with pyrefly directly: the file reports 1 error on the previous head and 0 errors now, the mcp and full chat client suites still pass (583 tests across openai and foundry files), and ruff check and format are clean. The foundry helper builds an azure MCPTool rather than the openai parameter, so its accumulator stays a plain dict and never trips this check.

[Eduard van Valkenburg (eavanvalkenburg)]

Manohar Paturi (@ManoharPaturi) The code review is clean and the two earlier threads are substantively addressed. The remaining approval blocker is the PR body: it omits the required Contribution Checklist entirely. Please add the checklist and confirm the applicable build/test, contribution-guideline, agreed-issue/no-competing-PR, and non-breaking-change items. I am not treating missing AI-assistance detail by itself as a blocker.

[Manohar Paturi (ManoharPaturi)]

Eduard van Valkenburg (@eavanvalkenburg) the Contribution Checklist is back in the PR body with the non-breaking item confirmed, and the same fix is applied to my other four open Python PRs (#9502, #9507, #9512, #9608) so none of them hit the same blocker. The typed accumulator fix from your inline finding is already in c744def with the pyrefly before/after in the thread. Everything can be unblocked for approval.