Python: fix order-dependent tool-approval bypass in mixed batches

[RongJie G (CorgiBoyG)]

Motivation & Context

A tool configured with approval_mode="always_require" could be silently bypassed when another call preceded it in the same model tool-call batch. Batch classification depended on the position of a call within the batch rather than on the call itself, so a security-sensitive approval gate could be skipped purely due to call ordering.

Follow-up review surfaced two further fail-open cases in the approval-pausing path, now fixed in this PR (see below).

Description & Review Guide

• What are the major changes?
  In _try_execute_function_call_groups (python/packages/core/agent_framework/_tools.py), the batch-classification loop used to break (or raise) at the first matching call, so whichever call matched first determined the whole batch outcome. It now scans the entire batch and classifies each call individually, even inside an approval-pausing batch. Three fail-open behaviors are corrected:

  1. Order-dependent approval bypass (original issue): an always_require tool is now paused for approval regardless of its position in the batch. Classification travels with each call, not with its position.
  2. Declaration-only sibling executed after approval: the approval branch previously wrapped every sibling as an approval request, so approving the batch drove a declaration-only (or additional) tool into local execution on resume, where it raised because it has no implementation. Declaration-only calls are now surfaced as user-input pauses alongside the approval pauses and never executed locally (spec 004).
  3. Unknown-call termination bypass: an unknown call in a batch with terminate_on_unknown_calls=True was downgraded into a rejectable approval request, so rejecting it (or dropping its response) silently skipped the fail-closed abort. Unknown-call termination is now raised up front as an unconditional fail-closed gate, before any approval is solicited or any sibling executes.

• What is the impact of these changes?
  Approval, declaration-only, and unknown-call termination now compose correctly across pause and resume, independent of call order. No public API change; behavior is corrected only for the previously order-dependent / fail-open cases.

• What do you want reviewers to focus on?
  The per-call classification inside the approval branch (declaration-only surfaced as user input, not as an approval request), and that unknown-call termination is an unconditional fail-closed gate that wins outright.

Validation

• uv run poe test — all 35 Python package tasks passed
• Pyright on the changed files — 0 errors and 0 warnings
• Ruff (lint + format) on the changed files — passed
• git diff --check — passed
• Regression matrix (in test_function_invocation_logic.py): mixed-batch approval enforced in both call orderings; declaration-only surfaced as user input and not executed on approval resume (end-to-end approve then resume); unknown-call fail-closed precedence; nameless unknown call termination. Each new/updated test fails without the fix and passes with it.

Related Issue

Fixes #8079

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 is linked to an issue and there is no other open PR for this issue (see Related Issue above).
• 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.

[Copilot]

🟡 Changes recommended

Unknown-call termination can still be lost after approval resumption, and nameless calls bypass termination.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes order-dependent approval bypasses in mixed Python tool-call batches.

Changes:

• Scans the complete batch before applying approval, declaration-only, or unknown-call handling.
• Adds regression tests for mixed-call ordering.

File summaries

File	Description
python/packages/core/agent_framework/_tools.py	Revises batch classification priority.
python/packages/core/tests/core/test_function_invocation_logic.py	Adds mixed-batch regression tests.

Review details

Suppressed comments (1)

python/packages/core/agent_framework/_tools.py:1815

• This only postpones the unknown-call check for the initial pass. The approval branch subsequently wraps the unknown call as a visible approval request (tool is None), and on resume approval responses are excluded from actionable_calls; _auto_invoke_function then treats the missing tool as hosted and returns without raising. Consequently, terminate_on_unknown_calls=True never terminates this mixed batch after approval. Preserve the deferred unknown call across the pause and apply the configured termination when the approval batch resumes, with a regression test that submits the approval response.

    if not requires_approval and not has_declaration_only_call and unknown_call_name is not None:
        raise KeyError(f'Error: Requested function "{unknown_call_name}" not found.')

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[RongJie G (CorgiBoyG)]

@microsoft-github-policy-service agree

[RongJie G (CorgiBoyG)]

Addressed the review findings in d3c94e7:

• Unknown calls are now classified from the underlying call carried by an approval response, so terminate_on_unknown_calls=True is still enforced after approval resume.
• Added a separate boolean sentinel so a nameless call is not confused with “no unknown call found.”
• Extended the regression coverage for both cases. Both tests fail without the follow-up implementation and pass with it.

Validation: all 35 Python package test tasks pass; targeted Ruff checks and git diff --check pass.

[Yufeng He (he-yufeng)]

I came here from #8079 with my own draft fix for the order-dependence, and after working through this branch I've put mine aside: this covers the original bypass plus two fail-open cases I had not isolated. Verified locally against current main (2c49f50):

• With main's _tools.py swapped in, the order-independence pair and the approval-resume test fail; the declaration-only sibling does get wrapped as a visible approval request on main (the tool_name in declaration_only_tool_names disjunct in the visible condition), so approving a mixed batch really does drive it into local execution and raise. All five new tests pass on this branch.
• Full test_function_invocation_logic.py on the branch: 195 passed, 2 failed, both pre-existing (aiohttp not installed in my env; same two fail on main). No regressions.
• The approval-branch declaration-only handling matches the standalone has_declaration_only_call path (user_input_request plus id backfill), so both pause surfaces behave the same way.

One semantic surface worth naming for the core team's precedence call: the unknown-call scan now also sees approval responses via _underlying_function_call, where main only scanned actionable calls. I could not find a regression path there. Hosted approvals are filtered out before they reach this function, and a resumed approval for a call that is no longer in the tool map fails at execution time on main anyway; this just fails it earlier, at classification, with the same KeyError. Still, it is a deliberate widening, so flagging it explicitly.

On precedence itself: the old comment claimed user-input pause beats unknown-call termination, but break-on-first-hit could only honor that when the pause-worthy call happened to come first, so the documented invariant was never really in force. Failing closed on the unknown call before any approval is solicited is the defensible reading for a security gate; spec 004 is silent on mixed-batch precedence, so whichever way the core team lands, classification-by-position has to go. Holding my own draft (which preserves the pause-first reading) until that confirmation, same as this one.

[RongJie G (CorgiBoyG)]

Appreciate you working all the way through this, Yufeng He (@he-yufeng), and for setting your own draft aside — the independent main-swap on the order-independence pair and the approval-resume case is a genuinely useful second pair of eyes on a path that's easy to get wrong. Glad to be sharpening this together.

One correction on the widening I flagged, because I'd rather state it precisely than let a convenient assumption stand. I went back and exercised the resume path against main directly instead of reasoning about it: for an approval response whose tool is no longer in the map, _auto_invoke_function hits the hosted-tool assumption (the else branch around L1509–1512) and returns the content unchanged — it doesn't raise. So main doesn't fail this at execution time; it silently passes the dangling call through. Under terminate_on_unknown_calls=True, this branch now fails it closed at classification with the same KeyError. That makes it a real behavior change, not the same failure surfaced earlier, so I didn't want it to ride implicitly.

The blast radius stays narrow, as you noted. Hosted/MCP approvals never reach the scan — _collect_approval_responses filters them out first. What's left is a local tool that paused for approval and then left the tool map before the resume, with termination enabled; in that window main waves the call through and this fails it closed. I think fail-closed is the right reading once the caller has explicitly opted into terminating on unknown calls, but it's a deliberate choice and squarely a precedence call for the core team.

On precedence itself we landed in the same place: the old comment claimed the user-input pause beat unknown-call termination, but break-on-first-hit only honored that when the pause-worthy call happened to come first, so the documented invariant was never actually in force. I've corrected the comment to match the fail-closed ordering the code implements. spec 004 is silent on mixed-batch precedence, so I'm keeping this in draft until the core team confirms direction — the classification-by-position removal stands either way, and I'm happy to move to pause-first if that's the call.