Python: Improve function approval resume and replay - #7345
Conversation
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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_idsset 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_resultsfallback logic. The use ofid()for object identity tracking in_collect_approval_responsesis 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' (likeuser_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_requestpath in_collect_approval_responses, which is a new way to consume approval authority (via user_input_request content in replayed history), distinct from the terminalfunction_resultpath 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_resultshas 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_idswhich 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_messagesuses 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, somax_function_callsno longer enforces its documented "total number of individual function invocations" invariant on that path.
Automated review by eavanvalkenburg's agents
|
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. |
|
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. |
|
Read through this against the current 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 approved_function_call = function_call_content.function_call
...
tool = tool_map.get(approved_function_call.name)( 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: Scope, to be accurate rather than alarming: the hosted paths do defend themselves — 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. |
|
@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
42540ae to
93cfc47
Compare
|
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 |
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
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
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_idwere globally unique. These behaviors can hide approved orrejected outcomes from callers, replay stale approval authority, attach results to the wrong logical invocation, or
produce provider-invalid history.
Description & Review Guide
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 rejectedresults 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 intonamed 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 forapproved/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.
call_idvalues no longer merge separate logical rounds.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_changessnapshot fixes remain tracked by #7212, #7125, #7045, #6973, #7043, and #6828 and will be submitted separately.
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.