Skip to content

Python: bind approvals to stable call occurrences (#7383) - #7988

Open
Eduard van Valkenburg (eavanvalkenburg) wants to merge 6 commits into
microsoft:mainfrom
eavanvalkenburg:approval-replay-storage
Open

Python: bind approvals to stable call occurrences (#7383)#7988
Eduard van Valkenburg (eavanvalkenburg) wants to merge 6 commits into
microsoft:mainfrom
eavanvalkenburg:approval-replay-storage

Conversation

@eavanvalkenburg

Copy link
Copy Markdown
Member

Motivation & Context

Session-backed approval binding now reconstructs executable calls from trusted pending state, but local approval identity still depends on provider call_id. Missing or reused provider IDs can make separate approval occurrences ambiguous, while changing identifiers outright would strand pending approvals serialized by older releases.

This change separates Agent Framework occurrence identity from provider/service call-result correlation and introduces an explicit compatibility path for stored legacy approvals.

Description & Review Guide

  • What are the major changes?
    • Assign one stable Content.id to each newly actionable local function_call while preserving its provider/service call_id.
    • Use the occurrence ID for new local approval requests and binding. Empty local call_id values temporarily fall back to that ID with a FutureWarning; deserialization never rewrites stored identities.
    • Preserve exact one-time resume for legacy pending snapshots that lack an occurrence ID, with a migration warning and fail-closed handling for mismatched new identities.
    • Carry the identity split through policy middleware, AG-UI approval lifecycle and legacy message compatibility, and OpenAI streaming correlation, including interleaved choice-local tool-call fragments.
    • Clarify that the function-loop specification is a stable contract rather than a per-PR changelog.
  • What is the impact of these changes?
    • A stale approval cannot authorize a newer local call merely because the provider reused call_id.
    • Existing serialized pending approvals remain resumable once. Legacy or ambiguous transport shapes warn or fail without executing and retain canonical occurrence-ID behavior for retry.
    • Hosted approvals keep their provider-issued request IDs. Provider call_id remains the function-call/result correlation key.
    • The guarantee remains scoped to trusted, serialized session-state evolution; it does not claim durable exactly-once execution across concurrent workers, crashes, or mutable application storage.
  • What do you want reviewers to focus on?
    • The legacy-versus-occurrence-aware binding boundary and its removal path.
    • AG-UI compatibility behavior for canonical resumes, historical messages, reused IDs, and confirm_changes.
    • Streaming identity stability and coalescing across OpenAI choice-local tool-call indexes.

Related Issue

Fixes #7383

Supersedes #7581. PR #7631 already landed the initial record/rebind/consume mechanism; this PR carries forward the remaining identity, replay, and compatibility work identified in #7581's review.

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.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335

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

Introduces stable function-call occurrence IDs to prevent stale or reused provider call IDs from authorizing newer calls.

Changes:

  • Separates framework occurrence identity from provider correlation IDs.
  • Adds legacy approval migration and AG-UI compatibility paths.
  • Extends streaming correlation and regression coverage.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
python/packages/openai/tests/openai/test_openai_chat_completion_client.py Tests choice-local indexes.
python/packages/openai/agent_framework_openai/_chat_completion_client.py Preserves choice indexes.
python/packages/core/tests/test_security.py Tests policy approval identity.
python/packages/core/tests/core/test_types.py Tests identity serialization and warnings.
python/packages/core/tests/core/test_function_invocation_logic.py Tests binding and streaming behavior.
python/packages/core/AGENTS.md Documents identity conventions.
python/packages/core/agent_framework/security.py Keys policy approvals by occurrence.
python/packages/core/agent_framework/_types.py Adds function-call occurrence IDs.
python/packages/core/agent_framework/_tools.py Implements identity assignment and binding.
python/packages/ag-ui/tests/ag_ui/test_run.py Tests interrupt identities.
python/packages/ag-ui/tests/ag_ui/test_endpoint.py Tests resume compatibility.
python/packages/ag-ui/tests/ag_ui/test_agent_wrapper_comprehensive.py Updates approval lifecycle tests.
python/packages/ag-ui/AGENTS.md Documents AG-UI identity separation.
python/packages/ag-ui/agent_framework_ag_ui/_run_common.py Selects canonical interrupt IDs.
python/packages/ag-ui/agent_framework_ag_ui/_message_adapters.py Propagates occurrence IDs.
python/packages/ag-ui/agent_framework_ag_ui/_approval_state.py Stores call and interrupt IDs separately.
python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Handles canonical and legacy resumes.
python/AGENTS.md Clarifies specification maintenance.
docs/specs/004-python-function-calling-loop.md Defines occurrence-aware approval behavior.
.github/skills/pull-requests/SKILL.md Adds stable-specification guidance.

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_types.py
Comment thread python/packages/ag-ui/agent_framework_ag_ui/_run_common.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py

@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.

MAF Automated Review — Iteration 1

Result: Findings reported
Scope: full PR (1 commit(s)): 402857a93d38
Model: gpt-5.6-sol

Overview

The PR cleanly separates provider correlation IDs from framework occurrence IDs and adds strong fail-closed binding, stored-call reconstruction, one-time consumption, and lifecycle tests. The remaining risks are concentrated in integrations and compatibility edges: streaming can still collapse distinct indexed calls when a provider reuses an ID, DevUI does not return the new nested occurrence identity, and AG-UI's legacy migration path mishandles service-session history and can perform unbounded quadratic matching.

Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
4 verified findings remained after source verification (2 high, 2 medium) across 2 files. Details are attached to the affected lines below.

Affected areas: python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py, python/packages/core/agent_framework/_tools.py

Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py
Comment thread python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Outdated
Comment thread python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@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.

MAF Automated Review — Iteration 2

Result: Findings reported
Scope: 2 net-new commit(s): ac395a52223a, 033174c7fbba
Model: gpt-5.6-sol

Overview

The change establishes a clear split between stable local occurrence identity and provider correlation IDs, with strong fail-closed approval binding, trusted snapshots, retry preservation, and targeted compatibility tests. The remaining risks are in streaming identity edge cases, one legacy AG-UI compatibility path, and package dependency metadata that permits an incompatible core installation.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
4 verified findings remained after source verification (2 high, 2 medium) across 3 files. Details are attached to the affected lines below.

Affected areas: python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py, python/packages/core/agent_framework/_tools.py, python/packages/openai/agent_framework_openai/_chat_completion_client.py

Comment thread python/packages/openai/agent_framework_openai/_chat_completion_client.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py
Comment thread python/packages/openai/agent_framework_openai/_chat_completion_client.py Outdated
Comment thread python/packages/ag-ui/agent_framework_ag_ui/_agent_run.py Outdated
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4e11fa76-de8e-4e85-9d86-aadc89ba6335
@eavanvalkenburg

Copy link
Copy Markdown
Member Author

/review

@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.

MAF Automated Review — Iteration 3

Result: No findings
Scope: 3 net-new commit(s): ae5bdade0467, 0d49394b64f9, b08a397ac778
Model: gpt-5.6-sol

Overview

The PR separates provider call_id values from stable local occurrence IDs, preserves those identities through OpenAI streaming and hosted approval adapters, and adds regression coverage for late provider IDs and reused IDs. Approval replay continues to bind against trusted pending snapshots and fails closed on mismatches. The remaining custom-stream ambiguity either duplicates already-supplied feedback or lacks enough provider-contract evidence to support a new publishable finding.

Reviewed the supplied incremental change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.

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: Bind tool-approval responses to surfaced approval requests

2 participants