Python: Defer provider-injected tool approvals to harness in-run execution#7091
Python: Defer provider-injected tool approvals to harness in-run execution#7091kartikmadan11 wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes an AG-UI transport bug where approved tool calls were executed before before_run could register provider-injected tools on the SessionContext, causing missing-tool execution failures and retry loops. The update defers approvals for tool names not present in the static tool map so they can be handled later during in-run execution when the full tool set is available.
Changes:
- Partition approved tool calls by membership in the static tool map; execute only statically-available tools immediately and defer the rest.
- Remove deferred approvals from
fcc_todoso approval-result substitution andTOOL_CALL_RESULTemission don’t inject placeholder failures for provider-injected tools. - Add a unit test covering the “mixed static + provider-injected tool approvals” scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py | Defer execution/substitution for approved tools absent from the static tool map to avoid injecting false failure results. |
| python/packages/ag-ui/tests/ag_ui/test_endpoint.py | Adds regression coverage ensuring provider-injected tool approvals don’t produce the transport’s placeholder error result. |
kartikmadan11
force-pushed
the
fix/7043-provider-tool-approval-defer
branch
from
48ba94d to
5c8664d
Compare
_resolve_approval_responses executes approved tool calls before agent.run, but provider-injected tools (FileAccessProvider, etc.) only exist after before_run registers them on the SessionContext. The transport's tool map lacks these tools, so it injects "Error: Tool call invocation failed." and the model retries in a loop. Partition approved responses by tool map membership: execute tools the transport can resolve now; leave provider-injected approvals in messages for the harness ToolApprovalMiddleware to handle in-run with the full per-invocation tool set. Fixes microsoft#7043
kartikmadan11
force-pushed
the
fix/7043-provider-tool-approval-defer
branch
from
5c8664d to
08609f5
Compare
| # Exclude deferred entries so _replace_approval_contents_with_results skips them. | ||
| if deferred_ids: | ||
| for deferred_id in deferred_ids: | ||
| fcc_todo.pop(deferred_id, None) |
There was a problem hiding this comment.
Could we keep deferred approvals out of _replace_approval_contents_with_results without removing their IDs from fcc_todo? That helper treats every approved response absent from fcc_todo as rejected, so this line rewrites the deferred call to Error: Tool call invocation was rejected by user. before agent.run can hand it to ToolApprovalMiddleware. A direct PR-head repro with file_access_write and no static tools produces that rejection result, meaning the approved side effect and real result from #7043 still never happen.
|
|
||
| # Provider tool should NOT have produced an error result - it should be deferred. | ||
| # Check that no "Error: Tool call invocation failed." appears for the provider tool. | ||
| provider_results = [r for r in tool_results if r.get("toolCallId") == "call_provider"] |
There was a problem hiding this comment.
Could this exercise a real provider-injected tool and assert its side effect plus exactly one real TOOL_CALL_RESULT? This loop passes when provider_results is empty, so it accepts the original user-visible failure where approval produces no write and no result. That is why the provider-only test stays green even while the resolver rewrites the approved call as a rejection before the harness runs.
Motivation & Context
When an agent uses approval mode and the user approves a mix of static tools and provider-injected tools (e.g. tools registered by
FileAccessProvider,CodeInterpreterProvider),_resolve_approval_responsesattempted to execute all approved tool calls immediately - before the agent'sbefore_runhooks fire. Provider-injected tools are only registered on theSessionContextduringbefore_run, so the transport's tool map does not contain them yet. This causedError: Tool call invocation failed.to be injected as a result, which the model then retried in a loop.Description & Review Guide
What are the major changes?
In
_resolve_approval_responses(_agent_run.py), approved responses are now partitioned by tool map membership before execution. Responses whose tool name is absent from the static tool map are deferred: they are removed fromfcc_todo(so_replace_approval_contents_with_resultsdoes not try to substitute them) and left in messages forToolApprovalMiddlewareto process in-run with the full per-invocation tool set. Only statically available tools are executed immediately by the transport.What is the impact of these changes?
Approving provider-injected tools no longer produces spurious error results. Static tools continue to execute and emit
TOOL_CALL_RESULTevents as before. No changes to public API or types.What do you want reviewers to focus on?
The deferral logic and the
fcc_todo.popcleanup - specifically whether there are edge cases where a tool name could legitimately be absent from the tool map for reasons other than being provider-injected.Related Issue
Fixes #7043
Contribution Checklist