Skip to content

Python: merge both approval lists in get_mcp_tool - #9499

Open
Manohar Paturi (ManoharPaturi) wants to merge 2 commits into
microsoft:mainfrom
ManoharPaturi:fix/mcp-tool-approval-mode-merge
Open

Manohar Paturi (ManoharPaturi) wants to merge 2 commits into
microsoft:mainfrom
ManoharPaturi:fix/mcp-tool-approval-mode-merge

Conversation

@ManoharPaturi

@ManoharPaturi Manohar Paturi (ManoharPaturi) commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

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

  • 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.

Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

require_approval["never"] = {"tool_names": never_require}
if require_approval:
mcp["require_approval"] = require_approval

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
@ManoharPaturi

Copy link
Copy Markdown
Contributor Author

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.

This branch was successfully deployed

1 active deployment
github-app-auth — 91ba8b41 Deployed Oct 22, 2026 by ManoharPaturi via add_label #25947
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: get_mcp_tool silently drops the always approval list when both approval lists are supplied

2 participants