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. |
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.
08609f5 to
623b4b0
Compare
|
Thank you for the detailed review, @moonbox3! I've addressed both concerns: Concern 1: Fixed - The latest commit now KEEPS deferred approval IDs in
Concern 2: Tests don't verify real tool execution Added test -
A complete end-to-end test with real provider-injected tools and their side effects would require the full harness execution flow during |
623b4b0 to
ce0e239
Compare
…ejected (microsoft#7043) When user approves a mix of static and provider-injected tools, partition them by tool map membership. Execute only static tools (available now). Keep deferred provider-injected tool approvals in fcc_todo but DON'T add results for them. This allows _replace_approval_contents_with_results to find them in fcc_todo but without results, so they stay as approval responses in messages (not marked as rejected). The harness can then handle them during agent.run() when provider tools are registered. Previous approach removed them from fcc_todo, causing them to be treated as rejected ("Tool call invocation was rejected by user"). Added test verifying provider-injected approvals don't produce rejection errors.
ce0e239 to
5e5e97b
Compare
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