.NET: Python: keep the approval response under service-side storage so a paused run resumes - #7133
Conversation
…used run resumes _prepare_message_for_openai handled function_approval_response and function_approval_request in one case and skipped both under service-side storage. That is right for the request (a server-issued item the server already has, so replaying it inline duplicates it -- microsoft#3295) but wrong for the response: the response is the user's approval decision, references the prior request by approval_request_id, and is not itself a server-stored item, so stripping it means the decision never reaches the model and an approval-paused run never resumes (approval-gated tools can never execute under service-side storage). Split the two into separate cases: the approval request keeps the storage skip (like function_call), the approval response is always sent (like function_result). Fixes microsoft#7125
There was a problem hiding this comment.
Pull request overview
This PR fixes a Python OpenAI client serialization bug that prevented approval-gated runs from resuming when continuing via service-side storage (previous_response_id / conversation_id). It ensures the user’s function_approval_response is still sent to the model under continuation requests, while continuing to skip replaying the server-issued function_approval_request to avoid duplicate-item errors.
Changes:
- Split
_prepare_message_for_openaihandling sofunction_approval_requestis skipped under service-side storage butfunction_approval_responseis always serialized. - Added a regression test asserting the approval response is preserved when
request_uses_service_side_storage=True.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/packages/openai/agent_framework_openai/_chat_client.py | Separates approval request vs response handling to prevent dropping the user’s approval decision under service-side storage. |
| python/packages/openai/tests/openai/test_openai_chat_client.py | Adds a regression test to ensure approval responses survive continuation requests. |
| # not itself a server-stored item, so it must reach the model even | ||
| # under service-side storage; skipping it leaves the run paused | ||
| # forever (#7125). Same treatment as function_result above. | ||
| prepared = self._prepare_content_for_openai( |
There was a problem hiding this comment.
Should we distinguish a fresh approval from one replayed by a HistoryProvider here? Providers store input messages and reload them with _attribution; on a later previous_response_id/conversation_id continuation this branch re-sends the already-consumed decision, regressing #3295's server/local replay boundary and making the existing test_prepare_messages_strips_approval_items_under_storage fail. Could we keep the fresh un-attributed response but continue skipping when request_uses_service_side_storage and replays_local_storage?
|
Hey, @he-yufeng are you still working on this? |
Keep the new tool-loop replay tests from upstream and reconcile the pre-existing strips test with this PR's contract: the approval request stays stripped under service-side storage, the approval response is kept because it is the input that resumes the paused run.
|
Rebased on current main and reconciled with the tool-loop replay tests that landed meanwhile (#7233). One thing worth flagging for review: this PR now also amends the pre-existing test_prepare_messages_strips_approval_items_under_storage. That test asserted both approval requests and approval responses get stripped under service-side storage. The request stripping is correct (server-issued item, replaying it duplicates), but the response is the user's fresh decision that resumes the paused run, so the test now asserts request stripped, response kept. All 275 tests in the file pass locally. |
|
Thank you @he-yufeng for tracking down the service-side approval-response serialization bug and adding the focused OpenAI coverage. Draft PR #7345 includes the clean request/response split together with the core approval-resume and history fixes. Closing this PR as superseded so the combined behavior can be reviewed in one place. |
Summary
Fixes #7125.
When an approval-gated run pauses for tool approval and the follow-up request continues a stored response (service-side storage:
previous_response_id/conversation_id), the user'sfunction_approval_responsewas silently stripped from the outgoing request, so the approval decision never reached the model. The model then re-issued the same approval request and the run paused again — approval-gated tools could never execute under service-side storage.Root cause
_prepare_message_for_openaihandledfunction_approval_responseandfunction_approval_requestin onecaseand skipped both under service-side storage. That is correct for the request (a server-issued item the server already has via the prior response, so replaying it inline duplicates it — #3295), but wrong for the response: the response is the user's decision, references the prior request byapproval_request_id, and is not itself a server-stored item, so it must reach the model.Fix
Split the two into separate cases: the approval request keeps the storage skip (same treatment as
function_call), the approval response is always sent (same treatment asfunction_resultdirectly above it).Test
test_prepare_message_for_openai_keeps_approval_response_under_service_side_storageasserts the response survives withrequest_uses_service_side_storage=True(before the fix the prepared result is empty). The existingtest_prepare_message_for_openai_with_function_approval_responsealready pins the storage-off serialization.I verified the change by reading: the existing storage-off test confirms the
mcp_approval_responseserialization, and the fix mirrors thefunction_resultbranch directly above. A local pytest run was blocked by a workspace-install issue in my environment (agent_frameworkcore not resolving underuv run), so the added test is exercised by CI rather than a local run.