From dbd28edd59ed03f21f22b270cbc227dc064d901c Mon Sep 17 00:00:00 2001 From: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com> Date: Sun, 18 Oct 2026 19:58:43 +0530 Subject: [PATCH 1/2] Python: merge both approval lists in the OpenAI and Foundry MCP tool helpers Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com> --- .../agent_framework_foundry/_chat_client.py | 10 ++++++++-- .../tests/foundry/test_foundry_chat_client.py | 17 +++++++++++++++++ .../agent_framework_openai/_chat_client.py | 10 ++++++++-- .../tests/openai/test_openai_chat_client.py | 17 +++++++++++++++++ 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/python/packages/foundry/agent_framework_foundry/_chat_client.py b/python/packages/foundry/agent_framework_foundry/_chat_client.py index 8ef77c8c461..86dccf57090 100644 --- a/python/packages/foundry/agent_framework_foundry/_chat_client.py +++ b/python/packages/foundry/agent_framework_foundry/_chat_client.py @@ -807,10 +807,16 @@ def get_mcp_tool( if isinstance(approval_mode, str): mcp["require_approval"] = "always" if approval_mode == "always_require" else "never" else: + # Both keys can be supplied together ("and/or" per the docstring), so build + # one combined mapping instead of letting the second assignment overwrite + # the first, which silently dropped the always-approval tool list. + require_approval: dict[str, Any] = {} if always_require := approval_mode.get("always_require_approval"): - mcp["require_approval"] = {"always": {"tool_names": always_require}} + require_approval["always"] = {"tool_names": always_require} if never_require := approval_mode.get("never_require_approval"): - mcp["require_approval"] = {"never": {"tool_names": never_require}} + require_approval["never"] = {"tool_names": never_require} + if require_approval: + mcp["require_approval"] = require_approval return mcp diff --git a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py index 490fb2cb3ce..3f4cfb4374f 100644 --- a/python/packages/foundry/tests/foundry/test_foundry_chat_client.py +++ b/python/packages/foundry/tests/foundry/test_foundry_chat_client.py @@ -1038,6 +1038,23 @@ async def test_bad_request_error_non_content_filter() -> None: assert "failed to complete the prompt" in str(exc_info.value) +def test_get_mcp_tool_merges_both_approval_lists() -> None: + """Both approval lists must survive: overwriting the first with the second would + silently auto-approve tools the caller asked to gate.""" + tool_config = FoundryChatClient.get_mcp_tool( + name="MCP", + url="https://mcp.example", + approval_mode={ + "always_require_approval": ["dangerous_tool"], + "never_require_approval": ["safe_tool"], + }, + ) + assert tool_config["require_approval"] == { + "always": {"tool_names": ["dangerous_tool"]}, + "never": {"tool_names": ["safe_tool"]}, + } + + def test_get_mcp_tool_with_project_connection_id() -> None: tool_config = FoundryChatClient.get_mcp_tool( name="Docs MCP", diff --git a/python/packages/openai/agent_framework_openai/_chat_client.py b/python/packages/openai/agent_framework_openai/_chat_client.py index a5c18ebda85..34d00c1d143 100644 --- a/python/packages/openai/agent_framework_openai/_chat_client.py +++ b/python/packages/openai/agent_framework_openai/_chat_client.py @@ -1619,10 +1619,16 @@ def get_mcp_tool( if isinstance(approval_mode, str): mcp["require_approval"] = "always" if approval_mode == "always_require" else "never" else: + # Both keys can be supplied together ("and/or" per the docstring), so build + # one combined mapping instead of letting the second assignment overwrite + # the first, which silently dropped the always-approval tool list. + require_approval: dict[str, Any] = {} if always_require := approval_mode.get("always_require_approval"): - mcp["require_approval"] = {"always": {"tool_names": always_require}} + require_approval["always"] = {"tool_names": always_require} if never_require := approval_mode.get("never_require_approval"): - mcp["require_approval"] = {"never": {"tool_names": never_require}} + require_approval["never"] = {"tool_names": never_require} + if require_approval: + mcp["require_approval"] = require_approval return mcp diff --git a/python/packages/openai/tests/openai/test_openai_chat_client.py b/python/packages/openai/tests/openai/test_openai_chat_client.py index 15cadc9d9b9..41949ba4d30 100644 --- a/python/packages/openai/tests/openai/test_openai_chat_client.py +++ b/python/packages/openai/tests/openai/test_openai_chat_client.py @@ -6476,6 +6476,23 @@ def test_prepare_tools_for_openai_with_mcp() -> None: assert "require_approval" in mcp +def test_get_mcp_tool_merges_both_approval_lists() -> None: + """Both approval lists must survive: overwriting the first with the second would + silently auto-approve tools the caller asked to gate.""" + tool_config = OpenAIChatClient.get_mcp_tool( + name="MCP", + url="https://mcp.example", + approval_mode={ + "always_require_approval": ["dangerous_tool"], + "never_require_approval": ["safe_tool"], + }, + ) + assert tool_config["require_approval"] == { + "always": {"tool_names": ["dangerous_tool"]}, + "never": {"tool_names": ["safe_tool"]}, + } + + @pytest.mark.parametrize("allowed_tools", [None, [], ["read_only"]], ids=["omitted", "empty", "nonempty"]) async def test_get_mcp_tool_preserves_allowed_tools_in_request(allowed_tools: list[str] | None) -> None: tool_config = OpenAIChatClient.get_mcp_tool( From c744def32f7c3c208bf70876d7a887905066fbee Mon Sep 17 00:00:00 2001 From: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com> Date: Mon, 26 Oct 2026 21:26:13 +0530 Subject: [PATCH 2/2] Python: type the combined MCP approval filter with the SDK mapping The combined accumulator was a plain dict, which fails Pyright and Pyrefly at the require_approval assignment on the typed Mcp tool parameter. It is now the SDK's McpRequireApprovalMcpToolApprovalFilter with its Always and Never filter objects, preserving the combined list logic and the empty mapping guard; the emitted JSON is unchanged. Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com> --- .../openai/agent_framework_openai/_chat_client.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/python/packages/openai/agent_framework_openai/_chat_client.py b/python/packages/openai/agent_framework_openai/_chat_client.py index 34d00c1d143..b27888eb128 100644 --- a/python/packages/openai/agent_framework_openai/_chat_client.py +++ b/python/packages/openai/agent_framework_openai/_chat_client.py @@ -112,6 +112,9 @@ CodeInterpreterContainerCodeInterpreterToolAuto, ImageGeneration, Mcp, + McpRequireApprovalMcpToolApprovalFilter, + McpRequireApprovalMcpToolApprovalFilterAlways, + McpRequireApprovalMcpToolApprovalFilterNever, ) from openai.types.responses.web_search_tool_param import WebSearchToolParam from pydantic import BaseModel, TypeAdapter, ValidationError @@ -1622,11 +1625,13 @@ def get_mcp_tool( # Both keys can be supplied together ("and/or" per the docstring), so build # one combined mapping instead of letting the second assignment overwrite # the first, which silently dropped the always-approval tool list. - require_approval: dict[str, Any] = {} + require_approval = McpRequireApprovalMcpToolApprovalFilter() if always_require := approval_mode.get("always_require_approval"): - require_approval["always"] = {"tool_names": always_require} + require_approval["always"] = McpRequireApprovalMcpToolApprovalFilterAlways( + tool_names=always_require + ) if never_require := approval_mode.get("never_require_approval"): - require_approval["never"] = {"tool_names": never_require} + require_approval["never"] = McpRequireApprovalMcpToolApprovalFilterNever(tool_names=never_require) if require_approval: mcp["require_approval"] = require_approval