Skip to content

Python: Improve function approval resume and replay - #7345

Merged
eavanvalkenburg merged 3 commits into
microsoft:mainfrom
eavanvalkenburg:python-approval-resume-contract
Jul 29, 2026
Merged

Python: Improve function approval resume and replay#7345
eavanvalkenburg merged 3 commits into
microsoft:mainfrom
eavanvalkenburg:python-approval-resume-contract

Conversation

@eavanvalkenburg

@eavanvalkenburg eavanvalkenburg commented Jul 27, 2026

Copy link
Copy Markdown
Member

Motivation & Context

Python approval resume currently relies on mutating caller-owned messages, omits resolved results from resumed
responses, and correlates calls/results as if call_id were globally unique. These behaviors can hide approved or
rejected outcomes from callers, replay stale approval authority, attach results to the wrong logical invocation, or
produce provider-invalid history.

Description & Review Guide

  • What are the major changes?
    • spec (~540 lines) — Adds the Python function-calling-loop contract: provider-neutral flow diagrams,
      approval/replay invariants, scenario-to-test mappings, validation requirements, and core-team coordination
      guidance.
    • actual code changes (~700 changed lines) — Makes approval resume immutable; returns approved and rejected
      results consistently; correlates reused ids by logical occurrence; preserves multi-content execution groups;
      keeps pending history resumable without leaking calls into unrelated model turns; separates tool results from
      follow-up input by role; and aligns the AG-UI result bridge with the same grouped contract.
    • renames and readability improvements (~1,280 changed lines) — Splits the previous monolithic loop into
      named approval-resolution, model-response, streaming, and non-streaming orchestration functions; replaces
      closure/shared-mutation plumbing with explicit processing results; removes dead argument/state plumbing;
      simplifies approval setup; and adds phase-level comments around the main flow.
    • tests & markdown (~1,540 lines) — Adds focused core, history, AG-UI, and OpenAI boundary regressions for
      approved/rejected resume, streaming parity, reused ids, grouped user-input requests, middleware/error limits,
      pending history, and exactly-once execution. Updates Python/core/AG-UI guidance to point maintainers to the
      specification.
    • The code/readability split is approximate because Git counts moved code as both deleted and added.
  • What is the impact of these changes?
    • Approved tools execute exactly once; rejected tools execute zero times and return one terminal rejection result.
    • Caller inputs and earlier responses remain unchanged.
    • Streaming updates, streaming finalization, and non-streaming output follow the same logical transcript.
    • Pending approvals remain resumable without leaking orphan calls into unrelated model turns.
    • Reused call_id values no longer merge separate logical rounds.
    • No public API signatures change.
  • What do you want reviewers to focus on?
    • Occurrence-aware approval correlation and grouped placeholder replacement.
    • Pending versus resolved approval history behavior.
    • Result/request roles and ordering in mixed approval batches.
    • Streaming/non-streaming parity and exactly-once execution.
    • Whether the specification accurately captures the foundation contract.

Related Issue

Fixes #7241
Fixes #7304

This PR intentionally contains only the core approval-resume foundation. The independent compaction, OpenAI
continuation, post-limit filtering, declaration-only streaming, provider-injected AG-UI, and confirm_changes
snapshot fixes remain tracked by #7212, #7125, #7045, #6973, #7043, and #6828 and will be submitted separately.

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.

@github-actions

github-actions Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/ag-ui/agent_framework_ag_ui
   _agent_run.py110911889%183–190, 237–238, 245, 354, 358, 360, 377, 404–405, 518, 532, 536, 540, 543, 548, 553, 562, 565, 572–578, 611, 623, 634, 637, 672, 726–730, 795, 810, 813, 815, 841, 867–869, 927, 929, 931, 934–938, 952, 960–965, 974–975, 1024–1027, 1038, 1046, 1078, 1093, 1107, 1119, 1149, 1153, 1156, 1158, 1198–1200, 1264, 1270–1271, 1276, 1280–1281, 1439, 1447, 1464, 1468, 1513, 1572, 1602–1603, 1725, 1871, 1931, 1948, 1968–1969, 1976, 2084, 2112, 2120, 2122, 2125, 2131, 2186, 2189, 2199–2200, 2207, 2253
packages/core/agent_framework
   _sessions.py5454192%130–132, 134–135, 152–153, 155–157, 235–236, 369, 700–704, 719, 749, 786–787, 801, 803, 822, 824, 907, 913, 947, 1018, 1022, 1032, 1172, 1188, 1321, 1335–1336, 1359, 1381, 1391, 1433
   _tools.py13438393%230–231, 408, 410, 423, 448–450, 458, 476, 490, 497, 504, 527, 529, 536, 544, 679, 713–715, 718–720, 722, 728, 779–781, 806, 832, 836, 874–876, 880, 1053, 1065, 1072–1075, 1096, 1104, 1118–1120, 1490, 1573, 1601, 1623, 1675–1676, 1733, 1780, 1787–1788, 1878, 1940–1941, 1952, 2022, 2036, 2039, 2052, 2055, 2078, 2085, 2094, 2098, 2123, 2157, 2225, 2254–2255, 2352, 2380, 2420, 2423, 2480, 2633, 2714, 3193
TOTAL45838447490% 

Python Unit Test Overview

Tests Skipped Failures Errors Time
9459 34 💤 0 ❌ 0 🔥 2m 26s ⏱️

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR overhauls the Python function-invocation/approval-resume flow to make approval resume immutable, stream and return terminal tool results consistently, correlate approvals/results by occurrence (even when call_id is reused), and prevent approval control wrappers from being replayed into later model turns. It also updates OpenAI Responses API continuation handling so service-side storage skips replaying approval requests while still sending new approval responses, and adds a normative function-calling loop specification plus expanded regression coverage.

Changes:

  • Refactors the core function-invocation layer to split approval-resolution vs model tool-call processing, enforce immutable normalization, and unify streaming/non-streaming semantics.
  • Adds extensive regression tests for approval resume ordering, replay/occurrence correlation, history filtering, limits/termination behavior, and OpenAI service-side continuation approval serialization.
  • Introduces a Python function-calling loop specification and links it from the Python AGENTS docs.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
python/packages/openai/tests/openai/test_openai_chat_client.py Updates tests to assert approval requests are stripped but approval responses are preserved under service-side storage continuation.
python/packages/openai/AGENTS.md Documents service-side continuation rules for approval request vs response serialization.
python/packages/openai/agent_framework_openai/_chat_client.py Ensures function_approval_response is serialized even when request_uses_service_side_storage=True, while function_approval_request is skipped.
python/packages/core/tests/core/test_harness_tool_approval.py Adds end-to-end approval-resume tests for immutability, reasoning replay grouping, history filtering, user-input pauses, and iteration-budget interactions.
python/packages/core/tests/core/test_function_invocation_logic.py Strengthens assertions around approval processing immutability/inertness and adds targeted unit tests for replay deduplication and occurrence-aware correlation.
python/packages/core/AGENTS.md Documents the new approval-resume invariants and history filtering expectations.
python/packages/core/agent_framework/_tools.py Major refactor: occurrence-aware approval normalization, explicit approval-resolution step, streamlined loop helpers, and shared policy for limits/errors/termination.
python/packages/core/agent_framework/_sessions.py Filters approval request/response wrappers out of history replay before later model calls.
python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Aligns with updated _try_execute_function_calls signature (removes unused attempt_idx).
python/AGENTS.md Adds guidance that function-calling loop changes must follow the new spec and require extra validation.
docs/specs/004-python-function-calling-loop.md Adds the function-calling loop contract, diagrams, scenario→test matrix, and validation checklist.

Comment thread python/packages/core/agent_framework/_tools.py Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 4 | Confidence: 80% | Result: All clear

Reviewed: Security Reliability, Test Coverage, Failure Modes, Design Approach


Automated review by eavanvalkenburg's agents

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated Code Review

Reviewers: 5 | Confidence: 84%

✓ Correctness

No actionable issues found in this dimension.

✓ Security Reliability

This is a large, well-structured PR that hardens the Python function-calling loop. The security-relevant changes (approval validation, immutable message copies, occurrence-aware correlation, history filtering) are generally sound. The deferred_approval_ids set in the AG-UI transport is populated but never consumed, which is dead code but not a bug since the deferred approvals are implicitly left in-place by the existing _replace_approval_contents_with_results fallback logic. The use of id() for object identity tracking in _collect_approval_responses is safe within the function scope. One minor reliability concern exists around the shallow copy in _copy_messages_for_function_invocation — Content objects are shared between the copied and original message lists, meaning mutations to Content attributes on the 'private copy' (like user_input_request = True) technically leak back. However, in practice this only occurs for freshly-created model response content (not caller-provided), so the stated immutability contract holds for caller inputs.

✓ Test Coverage

This is a well-tested PR with comprehensive coverage for the refactored function-calling loop. The spec, tests, and implementation are well-aligned. The main gap is missing dedicated test coverage for the is_follow_up_request path in _collect_approval_responses, which is a new way to consume approval authority (via user_input_request content in replayed history), distinct from the terminal function_result path that has explicit coverage. All other new behaviors (deferred provider-injected tools, non-adjacent compaction pairing, unexecutable tool content dropping, immutable approval boundaries) have appropriate test coverage through either direct unit tests or integration tests. The PR has extensive test coverage for its core changes: occurrence-based approval correlation, non-adjacent compaction grouping, post-limit output filtering, and history inertness of resolved approvals. The refactored _replace_approval_contents_with_results has dedicated tests for reused call_ids, multi-content groups, placeholder replacement, and deduplication. The new streaming/non-streaming parametrized tests cover middleware termination and error-limit edge cases. One moderate gap exists: the standard streaming approval-resume happy path (approve/reject → results yielded → model text) lacks a dedicated test at the FunctionCallingChatClient level, though it is covered at the Agent/harness level. New behaviors — post-limit content filtering, approval resume immutability, reasoning replay, history filtering, user input propagation, middleware termination, error limits, and the OpenAI approval response serialization change — all have dedicated tests with meaningful assertions. One minor gap: the error-limit approval test does not explicitly assert the model wasn't re-invoked (unlike the middleware-termination test which does), though the assertion on fallback text implicitly guards against it.

✓ Failure Modes

The changes are well-structured and the failure mode handling is generally solid. The occurrence-aware approval correlation, compaction linking, and provider-injected tool deferral all handle their error/edge cases correctly. The only notable dead code is deferred_approval_ids which is populated but never consumed — a minor cleanup omission, not a failure path. The exception-swallowing pattern in AG-UI _resolve_approval_responses (line 1344-1346) is pre-existing and correctly produces per-approval error results downstream. The _filter_approval_control_messages uses shallow copy appropriately.

✓ Design Approach

I found one design-level correctness issue in the AG-UI approval-resume path: it still assumes each approved tool produces exactly one terminal function_result, so an approved tool that pauses again for user input is converted into a synthetic failure instead of surfacing its follow-up request(s). I did not find a second well-supported issue in the files shown here. I found one design issue in the new approval-resume loop: it undercounts executed approved tools when the tool returns user-input request content instead of a function_result, so max_function_calls no longer enforces its documented "total number of individual function invocations" invariant on that path.


Automated review by eavanvalkenburg's agents

Comment thread python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py Outdated
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Addressed the latest automated review in af5572f. In addition to the two inline fixes, this adds direct coverage for follow-up requests consuming only their matching approval occurrence, FunctionInvocationLayer streaming approve/reject result ordering, and the approval error-limit path making exactly the required final no-tool model call. The unused AG-UI deferred approval id set was removed. No change was made for the shallow-copy observation because caller-owned Content is not mutated on that path, while deep-copying all Content would add cost and undermine the identity correlation used by approval normalization.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

Added focused AG-UI defensive-path coverage in 19a8134: grouped executor failure fallback, non-list confirm_changes tool metadata, and malformed confirm_changes argument JSON. The PR-added executable lines in agent_framework_ag_ui/_agent_run.py are now 55/55 line-covered.

Comment thread python/packages/core/agent_framework/_tools.py
Comment thread python/packages/core/agent_framework/_sessions.py
Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_compaction.py Outdated
@Palo-Alto-AI-Research-Lab

Copy link
Copy Markdown

Read through this against the current main because we had built the same class of hardening as a filter in Semantic Kernel (#14199), which @moonbox3 rightly closed in favour of Agent Framework — with an invitation to raise gaps here. This PR covers considerably more of the transcript contract than we had, and the occurrence-ordered correlation is a better answer to call_id reuse than the digest comparison we were using.

One thing that looks adjacent but not covered, flagging it in case it is deliberate rather than missed:

The approval response is still not bound to the request the framework surfaced. In _auto_invoke_function, the approved path takes the call out of the caller-supplied response and executes that:

approved_function_call = function_call_content.function_call
...
tool = tool_map.get(approved_function_call.name)

(python/packages/core/agent_framework/_tools.py, line 1485 on 3a163d6 — unchanged by this PR; correlation elsewhere matches on content.id in fcc_todo, i.e. identity, not payload.) So a response carrying a valid request id but a different name or different arguments than what was surfaced is executed as sent.

This is the parity item with .NET #7111 ("Bind tool-approval responses to surfaced approval requests", merged 21 July): it records issued requests in session state, rebinds the response to the recorded call, consumes matched entries for one-time use, and honors only approvals tied to a framework-issued request — on by default. Python has the pieces already: _store_already_approved_approval_requests / _pop_already_approved_approval_responses keep issued requests in session state; what is absent is the rebind-and-consume step against them.

Scope, to be accurate rather than alarming: the hosted paths do defend themselves — ag-ui consumes pending entries explicitly to prevent replay (_consume_pending_approval_entry), and foundry_hosting reconstructs from its server-side ApprovalStorage. The exposure is the core path used by callers driving agent_framework directly and round-tripping approvals through their own transport, where the client is the only source of the executed call.

Happy to open a separate issue per @moonbox3's invitation so it can be triaged on its own rather than widening this PR — or to send a Python PR mirroring #7111's approach (record, rebind, consume, default-on with an opt-out) once this one lands, since it touches the same functions and should not race it. Whichever you prefer; and if this is already handled in a layer I did not read, say so and I will drop it.

@eavanvalkenburg

Copy link
Copy Markdown
Member Author

@Palo-Alto-AI-Research-Lab thank you for the careful comparison and for calling out the Python parity gap. I agree this should be tracked separately rather than widening this PR. I opened #7383, including the dependency that implementation must wait until #7345 merges.

Make approval resume immutable and occurrence-aware, return grouped approved and rejected results consistently, preserve pending approval history without model-orphaned calls, and align streaming, non-streaming, and AG-UI result boundaries.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ee1d250-d1e2-4c6f-8c36-aae0d94fe7a1
@eavanvalkenburg
eavanvalkenburg force-pushed the python-approval-resume-contract branch from 42540ae to 93cfc47 Compare July 29, 2026 11:13
@eavanvalkenburg eavanvalkenburg changed the title Python: Harden function calling loop and approval replay Python: Harden function approval resume and replay Jul 29, 2026
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

This PR has been narrowed to the core approval-resume foundation for #7241 and #7304. Independent compaction, OpenAI continuation, post-limit filtering, declaration-only streaming, provider-injected AG-UI, and confirm_changes fixes were removed and remain tracked by their own issues for separate draft PRs. The previous consolidated head is archived on the fork branch approval-resume-contract-full.

Simplify approval-resolution setup and add phase-level comments around the key function invocation orchestration paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ee1d250-d1e2-4c6f-8c36-aae0d94fe7a1
EOF && git push origin python-approval-resume-contract
@eavanvalkenburg eavanvalkenburg changed the title Python: Harden function approval resume and replay Python: Improve function approval resume and replay Jul 29, 2026
Resolve the function invocation test placement conflict by retaining the occurrence-correlation regression block alongside the updated main test structure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ee1d250-d1e2-4c6f-8c36-aae0d94fe7a1
EOF && git push origin python-approval-resume-contract
@eavanvalkenburg
eavanvalkenburg added this pull request to the merge queue Jul 29, 2026
Merged via the queue into microsoft:main with commit 5987a67 Jul 29, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

6 participants