Skip to content

Python: Preserve approval decisions under OpenAI continuation - #7407

Merged
eavanvalkenburg merged 2 commits into
microsoft:mainfrom
eavanvalkenburg:python-openai-approval-continuation
Jul 30, 2026
Merged

Python: Preserve approval decisions under OpenAI continuation#7407
eavanvalkenburg merged 2 commits into
microsoft:mainfrom
eavanvalkenburg:python-openai-approval-continuation

Conversation

@eavanvalkenburg

@eavanvalkenburg eavanvalkenburg commented Jul 29, 2026

Copy link
Copy Markdown
Member

Motivation & Context

OpenAI service-side continuation already stores the approval request, but the current serializer drops the new
approval response as well. The service therefore never receives the user's approved or rejected decision.

Description & Review Guide

  • What are the major changes?
    • Continue omitting the already stored function_approval_request.
    • Always serialize the current function_approval_response as mcp_approval_response.
    • Cover approved and rejected decisions with and without service-side storage.
    • Keep this PR's spec change to its unique scenario row; Python: Preserve declaration-only streaming metadata #7409 owns the shared gap and related-issue cleanup.
  • What is the impact of these changes?
    • Approval-paused OpenAI Responses runs can resume under previous_response_id or conversation continuation.
    • Stored server-issued request items are still not duplicated.
    • Stateless behavior remains unchanged.
    • Centralizing shared spec cleanup prevents repeated same-hunk conflicts as the focused PRs merge.
  • What do you want reviewers to focus on?
    • The request-versus-response storage boundary.
    • Approved/rejected payload parity.

Related Issue

Fixes #7125

Shared spec reconciliation is in #7409, which should merge after #7407 and #7408.

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.

Copilot AI review requested due to automatic review settings July 29, 2026 20:10
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Jul 29, 2026
@eavanvalkenburg
eavanvalkenburg marked this pull request as ready for review July 29, 2026 20:12

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 fixes OpenAI Responses API continuation under service-side storage by ensuring the user’s tool-approval decision is not dropped during serialization, so approval-paused runs can reliably resume with previous_response_id / conversation_id.

Changes:

  • Update the OpenAI chat client serializer to skip only stored function_approval_request items under service-side storage, while still serializing function_approval_response as mcp_approval_response.
  • Expand unit test coverage to validate both approved and rejected decisions under storage-on vs storage-off behavior.
  • Document the request/response storage boundary and update the function-calling loop spec to reflect the covered scenario.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
python/packages/openai/agent_framework_openai/_chat_client.py Stops stripping function_approval_response under service-side storage, preserving approval decisions for continuation.
python/packages/openai/tests/openai/test_openai_chat_client.py Adds parameterized test coverage ensuring approval responses are kept (approved/rejected) while requests are omitted under storage.
python/packages/openai/AGENTS.md Documents how approval request vs response behave under service-side continuation and warns about manual history replay.
docs/specs/004-python-function-calling-loop.md Updates required invariants/coverage lists to include service-side approval decision behavior and its regression test.

@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Python Test Coverage

Python Test Coverage Report •
FileStmtsMissCoverMissing
packages/openai/agent_framework_openai
   _chat_client.py137011791%312, 325, 675–679, 687–690, 696–700, 750–757, 759–761, 768–770, 828, 836, 859, 1076, 1135, 1137, 1139, 1141, 1207, 1221, 1301, 1311, 1316, 1359, 1471–1472, 1487, 1760, 1867, 1872–1873, 1956, 1966, 1993, 1999, 2009, 2015, 2020, 2026, 2031–2032, 2112, 2156, 2159–2162, 2176, 2186–2187, 2199, 2241, 2306, 2323, 2326, 2353–2355, 2394, 2411, 2414, 2476, 2483, 2520–2521, 2556, 2594–2595, 2613–2614, 2786–2787, 2805, 2891–2899, 3077, 3092, 3142, 3152–3154, 3181–3183, 3193–3194, 3200, 3215, 3348–3349
TOTAL45992447990% 

Python Unit Test Overview

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

Comment thread python/packages/openai/AGENTS.md
@eavanvalkenburg
eavanvalkenburg force-pushed the python-openai-approval-continuation branch from acdde65 to b22beee Compare July 30, 2026 06:59
@eavanvalkenburg
eavanvalkenburg added this pull request to the merge queue Jul 30, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jul 30, 2026
@eavanvalkenburg
eavanvalkenburg force-pushed the python-openai-approval-continuation branch from b22beee to 3415dc7 Compare July 30, 2026 07:41
eavanvalkenburg and others added 2 commits July 30, 2026 10:20
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 1ee1d250-d1e2-4c6f-8c36-aae0d94fe7a1
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 1ee1d250-d1e2-4c6f-8c36-aae0d94fe7a1
@eavanvalkenburg
eavanvalkenburg added this pull request to the merge queue Jul 30, 2026
Merged via the queue into microsoft:main with commit 95ec5b7 Jul 30, 2026
39 checks passed
@antsok

antsok commented Jul 30, 2026

Copy link
Copy Markdown

@eavanvalkenburg

Thanks for this — the continuation case it fixes is real. Reporting a regression it introduces for approvals on local tools, found while bumping an app from 33bc9c06 to 143386fe.

What changed

The PR splits one arm into two and drops the storage guard from the response side:

-                case "function_approval_response" | "function_approval_request":
+                case "function_approval_request":
                     if request_uses_service_side_storage:
                         continue
                     ...
+                case "function_approval_response":
+                    prepared = self._prepare_content_for_openai(...)   # no guard

So under request_uses_service_side_storage=True the request is omitted and the response is always emitted. _prepare_content_for_openai then serializes it as:

case "function_approval_response":
    return {"type": "mcp_approval_response", "approval_request_id": content.id, "approve": content.approved}

Why that breaks for a local tool

The change is sound for a hosted/MCP approval: the service issued and stored the mcp_approval_request, so replaying it would duplicate, and the response must reach the service.

It assumes every function_approval_response has a server-stored mcp_approval_request to pair with. A local FunctionTool registered approval_mode="always_require" has none — it was never sent to the service, has no server_label, and is resolved entirely in-process. Emitting its response produces an mcp_approval_response whose approval_request_id is a local call_id that the service has never seen:

400 invalid_request_error: The following MCP approval requests have approval responses but weren't passed as input: call_OWTUay5B7jQPNHM8IGvIFaC7

Before this PR the shared guard kept the pair consistent — both omitted under storage, both sent without it — so this could not happen. The old test name says as much:
test_prepare_messages_strips_approval_items_under_storage.

There's a hint the local case wasn't in scope: the sibling function_approval_request arm reads server_label and emits None for a local tool, which is not a valid mcp_approval_request either.

Repro

Any Responses-API run with service-side storage where a local tool is approval-gated:

  1. register a local FunctionTool with approval_mode="always_require";
  2. let the model call it; approve;
  3. the next request carries mcp_approval_response with no matching request → 400.

Observed on Azure Foundry (gpt-5.4-mini) via an AG-UI/CopilotKit surface, three consecutive runs.
The outgoing payload, unedited apart from wrapping:

'input': [
  {'type': 'message', 'role': 'user', 'content': [...]},
  {'call_id': 'call_OWTUay…', 'type': 'function_call_output', 'output': "File 'notes393.md' not found."},
  {'type': 'mcp_approval_response', 'approval_request_id': 'call_OWTUay…', 'approve': True},
  ...

call_OWTUay… is a local file_access_read from the harness's file-access provider. Item census
over the failing run: 10 function_approval_request, 5 function_approval_response, and exactly 1
mcp_approval_response — the leak.

Suggested fix

Gate the response arm on the same hosted test core already uses
(agent_framework._tools._is_hosted_tool_approval, i.e. function_call.additional_properties
carrying a server_label):

case "function_approval_response":
    if not _is_hosted_tool_approval(content):
        continue          # local approvals are resolved in-process; the service has no request
    prepared = self._prepare_content_for_openai(...)

…and consider the same guard on the request arm rather than emitting server_label: None.

On the AGENTS.md note

Applications that manually replay message history must not send that same approval response again
on later turns.

Worth revisiting: an application that replays nothing by hand still hits this, because the framework's own HistoryProvider.before_run replays the response for it. That filter is order-dependent — details in a companion comment on #7345. So the burden this note places on
applications currently cannot be discharged by an application doing everything right.

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

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: function_approval_response is dropped from the request under service-side storage, so an approval-paused run never resumes

5 participants