Python: Enforce tool selection before dispatch - #8852
Roger Barreto (rogerbarreto) wants to merge 1 commit into
Conversation
Some provider protocols cannot represent every tool-selection constraint. Revalidate fresh calls at the local dispatch boundary while preserving correlated results, execution budgets, and approval resumes.
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
documentation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Approval-resume tests remain permissive and do not verify separation from fresh-call policy filtering.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Enforces tool_choice restrictions at Python’s local function-dispatch boundary.
Changes:
- Rejects disallowed fresh function calls with correlated errors.
- Excludes rejected calls from execution-budget accounting.
- Adds streaming, non-streaming, approval-resume tests and specification mapping.
| File | Description |
|---|---|
python/packages/core/agent_framework/_tools.py |
Filters fresh calls before dispatch. |
python/packages/core/tests/core/test_function_invocation_logic.py |
Adds policy and budget regressions. |
docs/specs/004-python-function-calling-loop.md |
Documents tool-selection behavior. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| options={"tools": [guarded_stream_tool]}, | ||
| options={ | ||
| "tools": [guarded_stream_tool], | ||
| "tool_choice": {"mode": "auto", "allowed_tools": ["guarded_stream_tool"]}, |
Code Coverage OverviewLanguages: Python Python / code-coverage/pythonThe overall line coverage in commit c040c47 in the Show a line coverage summary of the most covered files.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): c040c47161d6
Model: gpt-5.6-sol-fast
Overview
The PR adds a shared pre-dispatch filter that blocks fresh model calls excluded by tool_choice, preserves call/result ordering, and excludes synthetic rejections from the execution budget. The new streaming and non-streaming tests cover the immediate mixed-call and budget cases, while existing approval binding keeps recorded approval resumes separate. Two required-mode transitions still discard active restrictions and can allow a later model turn to execute a forbidden registered tool.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (2 high) across 1 file. Details are attached to the affected lines below.
Affected areas: python/packages/core/agent_framework/_tools.py
| execution = await execute_function_calls( | ||
| function_calls=function_calls, | ||
| options=options, | ||
| allowed_calls, blocked_call_ids = _fresh_function_calls_allowed_by_tool_choice(function_calls, options) |
There was a problem hiding this comment.
When a provider returns only calls excluded by required_function_name, this rejects the batch with zero executions, but both outer loops still clear required mode before the next model turn. A provider that ignores the restriction can then return the same forbidden tool again and it executes locally without the policy. Keep required mode active until at least one permitted call actually executes in both streaming and non-streaming paths.
| allowed_names = set() | ||
| elif required_name := tool_mode.get("required_function_name"): | ||
| allowed_names = {required_name} | ||
| elif (configured_names := tool_mode.get("allowed_tools")) is not None: |
There was a problem hiding this comment.
This allowlist is enforced only for the current round when mode is required: after an allowed call executes, both loops clear the entire tool_choice, so a later model turn can dispatch any registered tool. That defeats the local authorization boundary for providers that do not enforce allowed_tools. Once the required-call obligation is satisfied, preserve the allowlist by transitioning to auto mode rather than dropping it.
| allowed_result_groups = iter(execution.result_groups) | ||
| execution.result_groups = [ | ||
| [_tool_choice_rejection_result(function_call)] | ||
| if id(function_call) in blocked_call_ids | ||
| else next(allowed_result_groups) | ||
| for function_call in function_calls |
There was a problem hiding this comment.
What happens when the allowed batch contains an approval-required call plus a session-deferred executable sibling and also includes a blocked call? _try_execute_function_call_groups returns only the visible approval group, but this comprehension consumes one group for every allowed call, so the deferred sibling hits StopIteration and both streaming and non-streaming requests fail before returning the approval prompt. Could the reassembly key result groups by input occurrence, or preserve an explicit slot for every allowed call?
Motivation & Context
Some provider protocols cannot represent every
tool_choicerestriction. The shared function invocation loop therefore needs to preserve those restrictions at the local dispatch boundary so behavior remains consistent across providers and response modes.Description & Review Guide
none,required_function_name, andallowed_toolsbefore local execution. Excluded calls receive correlated error results without executing or consuming the function-call budget. Streaming, non-streaming, and approval-resume regressions cover the behavior, and the function-loop specification maps the new scenarios.Related Issue
N/A. This maintenance change is not linked to a public issue.
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.