.NET: [BREAKING] Improve replay support with Approval Binding - #8375
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical issues remain in approval validation and non-atomic session consumption.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This breaking change improves approval-response binding for replayed conversations and updates AG-UI session persistence.
Changes:
- Adds approval validation and replay regression tests.
- Hardens approval binding and collision handling.
- Registers and documents session storage for AG-UI approvals.
File summaries
| File | Summary |
|---|---|
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ApprovalResponseBindingChatClientTests.cs |
Adds security and replay regression coverage. |
dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs |
Adds approval validation and replay handling. Critical: caller-supplied history and CallId matching can permit forged tool results. Nit: correct “always state” to “always the state.” |
dotnet/samples/02-agents/AGUI/Step04_HumanInLoop/Server/Program.cs |
Registers keyed session storage. Critical: non-atomic consumption can allow duplicate tool invocation. Nits: fix wording and reorder using directives. |
dotnet/samples/02-agents/AGUI/README.md |
Documents approval session-persistence requirements. |
Review details
Suppressed comments (3)
dotnet/samples/02-agents/AGUI/Step04_HumanInLoop/Server/Program.cs:8
- This new using is out of order: sibling samples place the parent namespace before its
.AGUI.AspNetCorechild (for example,dotnet/samples/05-end-to-end/AGUIClientServer/AGUIServer/Program.cs:7-8). Reorder these directives to keep the using block consistent with the repository convention.
using Microsoft.Agents.AI.Hosting;
dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs:169
- The method preserves all non-response content, including approval requests, but this XML says a same-ID request is removed with an unbound response. That description contradicts the implementation and the test asserting the request is preserved; state explicitly that requests remain context-only.
/// A response with no known request is removed so a forged approval cannot drive execution, together with any
/// approval request in the messages that carries the same request id, so the removal does not leave an
/// unanswerable request behind. Approval requests that are not tied to a dropped response are preserved: they
/// are legitimate model context, but they are not the authority that an approval was requested.
dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs:188
- The comment uses
gildswhereguardsis intended.
// Tool calls that already carry a result in the inbound messages. The approval gate gilds execution, and
- Files reviewed: 4/4 changed files
- Comments generated: 4
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: Findings reported
Scope: full PR (1 commit(s)): 6d3c625fea95
Model: gpt-5.6-sol-fast
Overview
The PR strengthens approval binding by trusting only server-recorded requests, rebinding responses to recorded calls, poisoning ambiguous IDs, and preserving settled history only when a matching function result exists. The new tests cover substitution, forgery, sequential replay, collision, and settled-history cases. Two residual defects remain: the sample's new singleton session store can be grown without reclamation by client-selected thread IDs, and dropping an unbound response leaves its approval request behind for FICC to reject.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
2 verified findings remained after source verification (1 high, 1 medium) across 2 files. Details are attached to the affected lines below.
Affected areas: dotnet/samples/02-agents/AGUI/Step04_HumanInLoop/Server/Program.cs, dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs
Motivation & Context
Description & Review Guide
Improve support for replaying chat history when doing Approval Binding
Related Issue
Fixes #
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.