Skip to content

Python: avoid spurious warnings for staged approvals - #8895

Merged
Eduard van Valkenburg (eavanvalkenburg) merged 2 commits into
microsoft:mainfrom
eavanvalkenburg:approval-replay-binding
Sep 30, 2026
Merged

Eduard van Valkenburg (eavanvalkenburg) merged 2 commits into
microsoft:mainfrom
eavanvalkenburg:approval-replay-binding

Conversation

@eavanvalkenburg

@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) commented Sep 30, 2026 •

Copy link
Copy Markdown
Member

Motivation & Context

Approving or rejecting a tool call with a response created directly from the returned request and the same AgentSession can log an incorrect approval occurrence-identity mismatch, even though the decision is handled correctly.

Batch staging has already validated the decision and consumed the pending request. The later binding pass revisits the original input, cannot find the consumed request, and emits a misleading warning. This fix suppresses only that redundant diagnostic.

Description & Review Guide

  • What are the major changes? Record the exact source response objects that pass trusted batch staging, and suppress the later binder's redundant warning only for those objects. Extend the existing roundtrip test across approval/rejection, streaming/non-streaming, occurrence/legacy request IDs, and conflicting same-ID duplicates. Consumed replay follows the selected streaming mode.
  • What is the impact of these changes? Valid same-session approvals and rejections no longer emit the false mismatch warning. Authorization checks, pending-state handling, execution order, and input filtering remain unchanged. Unmatched duplicates and consumed replay responses still warn; existing legacy deprecation warnings are preserved.
  • What do you want reviewers to focus on? Confirm that diagnostic exemptions use matched source object identity rather than shared approval IDs, preserve trusted occurrence/request rebinding, and do not weaken stale-response or exactly-once protections.

Related Issue

Reported directly; no linked GitHub issue.

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

Copilot review overview

🟡 Changes recommended

Warning suppression is overly broad, and streaming replay plus distinct legacy-ID behavior lack coverage.

Review effort: Balanced
Findings: 1 Medium severity · 2 Low severity

Open (3)
What changed in this PR

Suppresses redundant approval identity warnings after staged decisions are consumed.

Changes:

  • Passes staged approval IDs into response binding.
  • Expands approval/rejection replay assertions.
File Description
python/​packages/​core/​agent_framework/​_tools.py Suppresses warnings for staged approval IDs.
python/​packages/​core/​tests/​core/​test_harness_tool_approval.py Tests quiet resumes and consumed replays.

💡 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/tests/core/test_harness_tool_approval.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.

MAF Automated Review — Iteration 1

Result: No findings
Scope: full PR (1 commit(s)): d17b8563e2fd
Model: gpt-5.6-sol

Overview

The change suppresses the redundant second-pass warning after staged approval decisions have already been validated and consumed, while retaining authoritative session binding and fail-closed removal of unmatched responses. The updated test covers approval and rejection in both initial streaming and non-streaming resume paths, including exactly-once execution and a warning on subsequent consumed replay. No new publishable issue remains after deduplication against the existing unresolved feedback.

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

Merged via the queue into microsoft:main with commit 612858c Sep 30, 2026
45 checks passed
@eavanvalkenburg
Eduard van Valkenburg (eavanvalkenburg) deleted the approval-replay-binding branch September 30, 2026 13:29

This branch was previously deployed

1 inactive deployment
github-app-auth — c2b3c03e Deployed Sep 30, 2026 by eavanvalkenburg via add_label #24116
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

python Usage: [Issues, PRs], Target: Python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants