Python: fix(core): resume nested agent-as-tool approval instead of silently no-oping - #8442
Sehastrajit (Sehastrajit-S) wants to merge 9 commits into
Conversation
…lently no-oping When a sub-agent used via as_tool() itself requires approval for one of its own tools, the approval request correctly surfaced to the caller, but sending the approval back did nothing: the outer agent looked up the nested tool's name in its own tool map, never found it, assumed it was a hosted tool, and silently dropped the response without executing anything. This tags a propagated nested approval with the owning outer tool call (name, call_id, original arguments) so the response can be routed back to that owner instead of being matched by the inner tool's name. as_tool()'s wrapper now also persists a resumable child session across the pause, and replays the approval into the child agent's run on resume instead of restarting it from the original task text. Fixes microsoft#4963
GitHub Actions
Active
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
|
@microsoft-github-policy-service agree |
There was a problem hiding this comment.
🟡 Changes recommended
Approval-result correlation, rejection handling, batching, deeper nesting, middleware, and propagated-session behavior remain incorrect.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes nested Agent.as_tool() approval resumption by preserving ownership metadata and child sessions.
Changes:
- Routes nested approval responses back through the owning tool.
- Persists resumable child-agent sessions.
- Adds a regression test for approved nested execution.
File summaries
| File | Description |
|---|---|
_tools.py |
Adds nested approval routing and metadata. |
_agents.py |
Persists and resumes child sessions. |
test_agents.py |
Tests a single approved nested tool call. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 7
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
…s-tool approvals Addresses review feedback on the initial fix for microsoft#4963. The first pass fixed the basic no-op but had several remaining correctness gaps: - The resumed result was keyed by the outer wrapper's own call id, but local resolution matches on the approval response's embedded (inner) call id, so the result was silently dropped. Now emits one result keyed by the inner call id plus a pairing result keyed by the outer call id, so both the spliced-in inner call and the still-unresolved outer call get a matching function_result instead of one being left dangling for a real provider to reject. - Rejected nested approvals never reached the resume path at all (only approved ones did), so the sub-agent was never told "no" and its call never settled. Nested-owned responses now route through resume regardless of the decision. - A single owner slot in additional_properties was overwritten by each level of an A -> B -> C wrapper chain, losing the inner level's own identity. Replaced with a stack of owner frames, one per level, resolved one hop at a time. - The resume bypassed the function middleware pipeline entirely by calling the owner tool's invoke() directly. It now recurses through _auto_invoke_function so middleware runs exactly as it would on a fresh call. - Two nested approvals answered in the same turn were resumed independently and concurrently, racing to restore/save the same stored child session. Responses for the same owner are now grouped and replayed together in one resumed run. Also fixes a regression introduced by the first pass: the owner-call-id marker was carried through FunctionInvocationContext.kwargs, which is the host-facing channel some callers (e.g. Skills' resource dispatch) spread into a tool's own **kwargs, so it leaked into what a plain tool observed. Moved to metadata, which is framework-internal only. Known remaining limitation, out of scope here: propagate_session=True shares the child's state dict directly with the parent's, including the framework's own internal approval-tracking keys, so a sub-agent's pending approval bookkeeping can collide with the parent's. That is a pre-existing gap in propagate_session's state-sharing design, not something introduced by nested-tool-approval resume, and fixing it needs namespacing approval state by agent identity across the session module -- a separate, larger change. Added regression tests: wire-level pairing (both call ids present with matching results, not just silently accepted by a mock that doesn't validate its input), rejection resume, three-level nesting, and two simultaneous nested approvals answered together.
|
Sehastrajit (@Sehastrajit-S) Thanks for the contribution. Before this is ready, please address and resolve the seven open review discussions: |
GitHub Actions
Active
|
Pushed a rework addressing the Copilot review findings:
Also caught and fixed a regression from the first pass in the process: the owner-call-id marker was briefly carried through Added tests for wire-level pairing (asserting both call ids are actually present with matching results, not just that a mock silently accepts whatever it's given), rejection resume, three-level nesting, and two simultaneous nested approvals answered in one turn. One limitation I'm leaving out of scope here: Full core test suite (13k+ tests) passes, along with ruff and pyright. |
GitHub Actions
Active
|
Okay Eduard van Valkenburg (@eavanvalkenburg) sir, I am working to solve them step by step! |
- Unquote the nested_resume forward-reference type annotations; with from __future__ import annotations they don't need quoting, and pyupgrade --py310-plus rewrites them, which the pre-commit hook flags as a diff. - Add the established type: ignore / ty: ignore comment pair to the _inner_get_response monkeypatch in the new regression test, matching the pattern already used for equivalent monkeypatches elsewhere in this file. No behavior change.
GitHub Actions
Active
There was a problem hiding this comment.
🔵 Needs a closer look
Nested session persistence, recursive middleware execution, and transcript reconstruction require final human validation.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
|
Hi Mr. Eduard van Valkenburg (@eavanvalkenburg), its ready for you to have an another look whenever you have time. Let me know if anything else needs addressing. |
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
documentation
| call_id=inner_call_id, | ||
| result="Nested approval response processed; further approval is required.", | ||
| ) |
There was a problem hiding this comment.
The second approval now surfaces correctly. One remaining gap is history replay when that second approval is answered: this terminal result survives in parent history, but the first inner call exists only inside the resolved approval wrapper, which history filtering removes. The outer model then receives:
assistant: call(owner)
tool: result(first)
assistant: call(second)
tool: result(second), result(owner)
There is no matching call(first), and the owner's result comes after the next assistant call.
Could we extend test_as_tool_resumes_consecutive_nested_approvals to capture the messages passed to the outer chat client on the third run (initial request -> approve first -> approve second), with propagate_session=False and both streaming modes? It should assert that every tool result has a matching call and that all calls in an assistant turn receive their results before the next assistant turn. The current execution-count and queued-text assertions don't check that boundary. The interaction between this follow-up result construction and resolved-approval history filtering looks like the place to investigate.
Fixes a history replay gap where a resolved nested approval request was deleted instead of unwrapped, orphaning its result. Fixes duplicate owner placeholders on chains of three or more consecutive approvals. Fixes misordered results when a simultaneous approval round also reveals a new pending request. Fixes a mixed batch hidden sibling silently dropping its own nested pause. Adds regression tests for three level nesting, simultaneous approvals, partial approve and reject within one group, three or more simultaneous approvals, and streaming mode for all multi round chains.
Head branch was pushed to by a user without write access
GitHub Actions
Active
|
Hi Eduard van Valkenburg (@eavanvalkenburg) and Jose Alvarez (@jpalvarezl),
I also added regression tests covering three level nesting, simultaneous approvals, partial approve and reject within one group, three or more simultaneous approvals, and streaming mode for all the multi round chains. The full core suite, ruff, pyright, and ty all pass. Would you kindly please verify this when you have a chance? Thank you so much for the thorough review so far, it has genuinely made this fix much more solid. |
There was a problem hiding this comment.
🟡 Changes recommended
Nested ownership trust, middleware termination, and child-session lifecycle issues can cause unauthorized or malformed execution.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
python/packages/core/agent_framework/_tools.py:3221
- Provider-emitted approval requests can carry this reserved property:
_process_model_function_callsstores a directfunction_approval_requestunchanged, so its owner stack is not necessarily framework-authenticated. Copying that stack into the rebound response lets_nested_owner_keyroute the user's decision to any local tool and arguments named by the provider, bypassing the tool represented by the approval request (and even that local tool's normal approval classification). Strip this property at the provider boundary and keep nested ownership in server-only state or attach provenance that cannot originate in model content.
trusted_owner_stack = request.additional_properties.get(_NESTED_TOOL_APPROVAL_OWNER_STACK_KEY)
if trusted_owner_stack is None:
rebound_properties.pop(_NESTED_TOOL_APPROVAL_OWNER_STACK_KEY, None)
else:
rebound_properties[_NESTED_TOOL_APPROVAL_OWNER_STACK_KEY] = copy.deepcopy(trusted_owner_stack)
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Fixes an unversioned ToolApprovalState migration path that retained execution-bearing approval state instead of invalidating it. Fixes a child session lookup that could load an abandoned session left behind by an earlier, unrelated call reusing the same provider call_id. Fixes MiddlewareTermination during a nested resume leaving every call after the first in a group with no result. Fixes a Content object mutation that could corrupt a message another session had already persisted. Fixes a placeholder tracking flag that was not cleared for a reused call_id from an abandoned chain. Corrects a stale test reference in the function calling loop spec. Adds a regression test for the reused call_id case and keeps the existing 19 nested approval tests passing.
GitHub Actions
Active
There was a problem hiding this comment.
🟡 Changes recommended
Middleware-generated approvals can still be discarded during nested replay, and abandoned child sessions accumulate indefinitely.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
python/packages/core/agent_framework/_agents.py:762
- Abandoned nested approvals are never evicted from this map. The approval layer explicitly allows a later surfaced batch to replace an abandoned one, but cleanup here only occurs if the original owner call is eventually resumed to completion. Repeating that flow with new call IDs retains one serialized child transcript per abandoned task indefinitely, causing unbounded parent-session growth. Reconcile these snapshots when active approval authority is replaced or otherwise add bounded cleanup.
child_sessions[owner_call_id] = session.to_dict()
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
Fixes MiddlewareTermination raised during a nested resume silently discarding a function_approval_request instead of surfacing it, when outer level middleware itself requires approval on the synthetic re-invocation of an as_tool wrapper. Fixes a migrated ToolApprovalState object always warning about discarded pending state, even when its queues were empty, because the check tested for key presence instead of actual content. Adds regression tests for both, plus a test confirming the existing warning still fires when a legacy object actually has queued state.
GitHub Actions
Active
There was a problem hiding this comment.
🟡 Changes recommended
Middleware approvals can lose their interrupted continuation, while result reordering and stale session state introduce additional correctness risks.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
python/packages/core/agent_framework/_tools.py:2857
- A terminal middleware short-circuit settles this owner without invoking
Agent.as_tool()'s wrapper, but that wrapper is the only code that removes its stored child session (_agents.py:752-765). If the child was paused before this replay,_af_agent_tool_child_sessions[owner_call_id]remains permanently unreachable; an already-emitted placeholder flag is also not cleared here as it is on the normal completion path. Ensure terminal owner completion clears the nested continuation bookkeeping so long-lived parent sessions do not accumulate abandoned child snapshots.
python/packages/core/agent_framework/_tools.py:3135 - Empty serialized approval queues trigger this warning even though nothing was discarded.
ToolApprovalMiddlewarewritesqueued_approval_requestsandcollected_approval_responsesas empty lists (_harness/_tool_approval.py:206-210), so an ordinary unversioned state can hit this path and emit the inaccurate reissue warning. Use the same non-empty check already used for object-form migration below.
- Files reviewed: 8/8 changed files
- Comments generated: 3
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
| for index, inner_call_id in enumerate(inner_call_ids): | ||
| resumed_result = getattr(owner_result, "result", None) | ||
| group = [Content.from_function_result(call_id=inner_call_id, result=resumed_result)] |
| if isinstance(exc.result, Content) and exc.result.type == "function_approval_request": | ||
| # Middleware (e.g. policy enforcement) itself requires approval mid-resume. This | ||
| # is a new pause, not a terminal result -- the ordinary (non-nested) path passes | ||
| # this content type through untouched for the same reason (see | ||
| # _auto_invoke_function's own MiddlewareTermination handling above). Re-tag it | ||
| # with this level's own owner identity, the same way | ||
| # _try_resume_nested_tool_approval_group's own UserInputRequiredException | ||
| # handling propagates a fresh nested pause, or the request and the resume it | ||
| # belongs to are silently lost. | ||
| remaining_stack = stack[:-1] if stack else [] | ||
| repropagated = copy.copy(exc.result) | ||
| repropagated.additional_properties = dict(repropagated.additional_properties) | ||
| repropagated.call_id = owner_call_id | ||
| if not repropagated.id: | ||
| repropagated.id = owner_call_id | ||
| new_stack = [*remaining_stack, dict(frame)] | ||
| repropagated.additional_properties[_NESTED_TOOL_APPROVAL_OWNER_STACK_KEY] = new_stack |
| execution_tasks = [ | ||
| contextvars.copy_context().run(asyncio.create_task, _run_call(function_call)) | ||
| for function_call in ordinary_calls | ||
| ] + [ | ||
| contextvars.copy_context().run(asyncio.create_task, _run_owner_group(calls)) for calls in owner_groups.values() |
| for index, inner_call_id in enumerate(inner_call_ids): | ||
| resumed_result = getattr(owner_result, "result", None) | ||
| group = [Content.from_function_result(call_id=inner_call_id, result=resumed_result)] | ||
| if index == 0 and not already_paired: | ||
| group.append(Content.from_function_result(call_id=owner_call_id, result=resumed_result)) |
There was a problem hiding this comment.
Could we keep the child call result out of the outer model-bound transcript? With a service-managed outer response, the provider knows only owner_call_id, but this also adds a result for inner_call_id. OpenAI omits inline calls under previous_response_id while still sending every result as a function_call_output, so the resume references a call the outer service never issued and is rejected. Could the child pair remain in the child session and only the owner result be sent to the outer provider?
| if isinstance(exc.result, Content) and exc.result.type == "function_approval_request": | ||
| # Middleware (e.g. policy enforcement) itself requires approval mid-resume. This | ||
| # is a new pause, not a terminal result -- the ordinary (non-nested) path passes | ||
| # this content type through untouched for the same reason (see |
There was a problem hiding this comment.
Could we pull the repeated nested-pause result construction into one helper used by both UserInputRequiredException and MiddlewareTermination? _try_resume_nested_tool_approval_group and _run_owner_group separately retag the request, update hidden siblings, create one result per inner call, and maintain _NESTED_OWNER_PLACEHOLDER_EMITTED_STATE_KEY. Something like _build_nested_pause_result_groups(...) would keep those ordering and pairing rules in one place, so every pause path cannot accidentally implement them differently.
There was a problem hiding this comment.
What happens when MiddlewareFailure is raised while replaying a nested approval? responses_to_execute contains the child approval responses, so settlement submits inner_call_id outputs even though the outer service is waiting on owner_call_id; that request is rejected and the best-effort failure is swallowed, leaving the outer continuation stranded. Could this settlement unwrap nested ownership and settle the provider-owned owner call instead?
|
Thank you for the substantial effort and thoughtful iteration on this. After working through the design, we are going to address #4963 with a smaller contract: child tools used through |
Motivation & Context
When a sub-agent used via
Agent.as_tool()internally requires approval for one of its own tools, the approval request correctly surfaced to the caller, but sending the approval back silently did nothing: the outer agent looked up the nested tool's name in its own tool map, never found it, assumed it was a hosted tool, and dropped the response without executing anything.Description & Review Guide
_execute_single_function_callnow tags a propagated nested approval request (raised viaUserInputRequiredExceptionfrom inside a wrapper such asAgent.as_tool()) with a stack of the outer tool calls that own it, one frame per level of nesting, stored inadditional_properties._try_resume_nested_tool_approval_groupdetects that tag when one or morefunction_approval_responses underlying tool isn't in the currenttool_map, groups responses that share the same owner (so simultaneous nested approvals are replayed together, not raced independently), and replays them through the owner by recursing into_auto_invoke_function(so middleware runs the same as on a fresh call). It returns a result keyed by each response's own (inner) call id, plus a pairing result keyed by the owner's own call id, so both slots in the transcript resolve correctly for a real provider._resolve_approval_responsesroutes nested-owned responses through resume regardless of the approve/reject decision, so a rejection still reaches the sub-agent and its call still settles.Agent.as_tool()'s wrapper persists a resumable child session in the parent session's state (keyed by the outer call id, cleaned up once resolved) whenpropagate_session=False, and on resume feeds the approval response(s) into the child agent's run instead of restarting it from the original task text.FunctionInvocationContext.metadata, notkwargs, so it never leaks into the host-facing runtime kwargs a tool or Skills resource observes.Agent.as_tool()sub-agents can now have their own approval-gated tools, including through multiple levels of nesting, with correct resolution, rejection handling, and middleware behavior._tools.py(_NESTED_TOOL_APPROVAL_OWNER_STACK_KEYand its use in_execute_single_function_call/_try_resume_nested_tool_approval_group), and the grouping logic in_try_execute_function_call_groups.Known, documented, out-of-scope limitation:
propagate_session=Trueshares the child's state dict directly with the parent's, including the framework's own internal approval-tracking keys, so a sub-agent's pending-approval bookkeeping can collide with the parent's. That's a pre-existing gap inpropagate_session's state-sharing design, not something this fix introduces; fixing it needs namespacing approval state by agent identity across the session module, a separate, larger change.Related Issue
Fixes #4963
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.