Repository navigation
Python: merge both approval lists in get_mcp_tool - #9499
Manohar Paturi (ManoharPaturi) wants to merge 2 commits into
Conversation
Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
Eduard van Valkenburg (eavanvalkenburg)
left a comment
There was a problem hiding this comment.
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 | ||
|
|
There was a problem hiding this comment.
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>
|
The 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: Example run: https://github.com/microsoft/agent-framework/actions/runs/39810016173/job/119933441343 None of the changes in this PR touch |
Motivation & Context
get_mcp_toolaccepts anapproval_modedict and its docstring says thealways_require_approvalandnever_require_approvalkeys 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
require_approvalmapping instead of assigning it twice. Single key behavior is unchanged.always_require_approvalkeep requiring approval when a never list is also present. No other call sites are affected._loader.pyforMcpServerToolSpecifyApprovalMode, which is the closest sibling for the same field.Related Issue
Fixes #9498
AI Assistance