.NET: Bind tool-approval responses to surfaced approval requests by rogerbarreto · Pull Request #7111 · microsoft/agent-framework

rogerbarreto · 2026-07-14T13:08:38Z

Motivation and Context

Tool approvals in the .NET stack are honored from the ToolApprovalResponseContent carried in the message stream. This change hardens that flow so an approved tool call always corresponds to the approval request the framework actually surfaced, keeping the human-in-the-loop control aligned with what a user was asked to approve.

Description

Adds ApprovalResponseBindingChatClient, registered as the outermost decorator above FunctionInvokingChatClient in the default ChatClientAgent pipeline:

Outbound: records each model-originated ToolApprovalRequestContent into the session AgentSessionStateBag, keyed by request id.
Inbound: binds each ToolApprovalResponseContent to its recorded request, rebinds the response tool call to the recorded call, consumes matched entries for one-time use, and honors only approvals tied to a framework-issued request.
Outside an agent run (no session): no-op passthrough with a warning.

Applies the same binding in the ToolApprovalAgent harness by tracking surfaced requests and binding collected responses to them during a queue cycle.

Adds ChatClientAgentOptions.DisableApprovalResponseBinding (default off) and a UseApprovalResponseBinding builder extension for custom chat client stacks. The Foundry hosting path already reconstructs the call from the server-side approval map and is unaffected.

Contribution Checklist

Unit tests for the decorator and the harness; full unit suite green
dotnet format clean on changed files

Harden the tool-approval flow so an approved tool call always matches the request the framework surfaced for approval. Add ApprovalResponseBindingChatClient as the outermost decorator above FunctionInvokingChatClient. It records each model-originated ToolApprovalRequestContent in the session state and, on the next request, binds every ToolApprovalResponseContent to its recorded request: the response tool call is rebound to the recorded call, matched entries are consumed for one-time use, and only approvals tied to a framework-issued request take effect. Apply the same binding in the ToolApprovalAgent harness by tracking the requests it surfaces and binding collected responses to them during a queue cycle. Add ChatClientAgentOptions.DisableApprovalResponseBinding (default off) and a UseApprovalResponseBinding builder extension for custom chat client stacks. Includes unit tests for the decorator and the harness.

github-actions

Automated Code Review

Reviewers: 5 | Confidence: 87%

✓ Correctness

The PR correctly implements approval-response binding at two levels: (1) AprovalResponseBindingChatClient as a DelegatingChatClient decorator that records model-originated requests and validates/rebinds inbound responses against them, and (2) ToolAprovalAgent harness binding during queue cycles. The logic for recording, matching, rebinding, consuming entries, and dropping forged/unbound content is sound. State management via AgentSessionStateBag and ToolApprovalState persists correctly across turns. The streaming and non-streaming paths are consistent. No correctness, security, or race condition issues found.

✓ Security Reliability

This PR adds a well-designed security hardening layer that binds inbound tool-approval responses to model-originated approval requests the framework surfaced, preventing forged or substituted tool calls from being honored. The implementation is sound: it uses AsyncLocal-based session scoping for thread safety, consumes matched entries to prevent replay, and rebinds tool calls to the framework-recorded originals. One minor defense-in-depth gap exists where duplicate responses with the same RequestId in a single inbound batch could all pass through (though rebound to the same legitimate tool call).

✓ Test Coverage

The PR adds comprehensive unit tests for the non-streaming path of AprovalResponseBindingChatClient (7 tests covering passthrough, recording, forged responses, rebinding, rejection, consumption, and no-session scenarios) and two security-focused ToolApprovalAgent tests. However, the streaming path (GetStreamingResponseAsync) of the ApprovalResponseBindingChatClient has a completely distinct recording mechanism (accumulating requests across async iterations in a try/finally block) that is entirely untested. There is also no test verifying the DisableApprovalResponseBinding option actually prevents the decorator from being registered.

✓ Failure Modes

The PR adds a well-structured two-layer approval response binding mechanism: AprovalResponseBindingChatClient as a pipeline decorator (handling the general case and single-request flows via the inner agent's session state) and ToolApprovalAgent harness binding (handling the multi-request queue cycle). The layers use independent state stores (session StateBag vs ToolApprovalState.SurfacedApprovalRequests) and don't interfere. Pending entries are consumed atomically after binding, preventing replay. The no-session passthrough is intentional (tested) and logged. Exception paths between consume-and-execute are acceptable: if FICC throws after an approval is consumed, a retry naturally re-surfaces the request for fresh approval. No silent failures, lost errors, or exploitable races identified.

✓ Design Approach

The new binding layer closes the forged/substituted-response hole, but both implementations still honor the same approval request more than once when a caller duplicates the same RequestId in a single inbound turn. That leaves a replay path open inside the very code path that is supposed to enforce one-time use of surfaced approvals.

Automated review by rogerbarreto's agents

Copilot

Pull request overview

This PR hardens the .NET tool-approval flow by binding approval responses to the specific approval requests that the framework surfaced, preventing forged or substituted tool calls from being honored downstream (notably by FunctionInvokingChatClient).

Changes:

Adds ApprovalResponseBindingChatClient and wires it into the default ChatClientAgent pipeline as the outermost decorator (with opt-out via ChatClientAgentOptions.DisableApprovalResponseBinding).
Adds a UseApprovalResponseBinding builder extension for custom chat client stacks.
Extends the ToolApprovalAgent harness and adds new unit tests covering forged/substituted approval response scenarios.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs	New decorator that records surfaced approval requests and binds inbound approval responses to them.
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientExtensions.cs	Registers the binding decorator as the outermost default middleware (unless disabled).
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientBuilderExtensions.cs	Adds `UseApprovalResponseBinding` for custom pipelines.
dotnet/src/Microsoft.Agents.AI/ChatClient/ChatClientAgentOptions.cs	Adds `DisableApprovalResponseBinding` option and includes it in `Clone()`.
dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalState.cs	Persists surfaced approval requests for binding in the harness.
dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs	Tracks surfaced requests and attempts to bind collected responses to surfaced requests during approval cycles.
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ApprovalResponseBindingChatClientTests.cs	New unit tests for request recording, binding/rebinding, replay prevention, and no-session behavior.
dotnet/tests/Microsoft.Agents.AI.UnitTests/ChatClient/ChatClientAgentTests.cs	Updates expectations for the default outermost decorator type.
dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs	Adds harness security tests for forged/substituted approvals during queue cycles.

Comments suppressed due to low confidence (1)

dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs:266

In the streaming path, surfaced approval requests are only recorded when unapproved.Count > 1. With approval-response binding now keyed off SurfacedApprovalRequests, a single unapproved request can be surfaced without ever being recorded, leaving subsequent responses unbindable (and allowing forged/substituted approvals to pass through unchanged). Record the surfaced request(s) whenever unapproved.Count > 0, and keep the queueing of excess requests conditional.

            // 5. Queue excess unapproved requests and yield only the first to the caller.
            if (unapproved.Count > 1)
            {
                // Record every unapproved request as surfaced so the caller's responses can be bound to a
                // model-originated request during the queue cycle.
                RecordSurfacedApprovalRequests(state, unapproved);

                state.QueuedApprovalRequests.AddRange(unapproved.GetRange(1, unapproved.Count - 1));
            }

            this._sessionState.SaveState(session, state);
            yield return new AgentResponseUpdate(ChatRole.Assistant, [unapproved[0]]);

Address review feedback on the approval-response binding decorator: consume a matched request from the per-turn lookup so a duplicate response with the same request id in one turn is honored only once, and return the materialized message list instead of the original enumerable so a single-use sequence is not enumerated twice. Rename the local pending list to pendingRequests for clarity. Adds a duplicate-response regression test.

…he harness Address review feedback on ToolApprovalAgent: store a snapshot of each surfaced/pending approval request (cloned tool call with a copied arguments dictionary) so a later mutation of the caller-visible instance cannot change the recorded call used to bind the response, and consume a surfaced request on match so a duplicate response with the same request id in one pass is honored only once. Apply both symmetrically in the harness and the ApprovalResponseBindingChatClient decorator. Adds regression tests for the snapshot and duplicate-response cases.

westey-m · 2026-07-15T10:40:45Z

+    private static bool ContainsApprovalContent(IEnumerable<ChatMessage> messages)
+    {
+        foreach (var message in messages)
+        {
+            if (ContainsApprovalContent(message))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }


nit: A few of these helper methods can be shortened to a single linq query, e.g.

Suggested change

private static bool ContainsApprovalContent(IEnumerable<ChatMessage> messages)

{

foreach (var message in messages)

{

if (ContainsApprovalContent(message))

{

return true;

}

}

return false;

}

private static bool ContainsApprovalContent(IEnumerable<ChatMessage> messages) =>

messages.Any(ContainsApprovalContent);

westey-m · 2026-07-15T10:48:47Z

+        var pendingRequests = LoadPendingApprovalRequests(session);
+        var byRequestId = new Dictionary<string, ToolApprovalRequestContent>(StringComparer.Ordinal);


Consider storing the dictionary in state. This means we don't need to re-create the dictionary on each run after we already created a list on deserialization. Instead we just create the dictionary on deserialization.

westey-m · 2026-07-15T10:51:15Z

+            byRequestId[request.RequestId] = request;
+        }
+
+        var result = new List<ChatMessage>(messageList.Count);


This code introduces a few new allocations, even when no messages were modified. Consider only creating the list when needed.

westey-m · 2026-07-15T10:56:45Z

+                            // Rebind the tool call to the model-originated call so the approved call matches the
+                            // tool name and arguments that were surfaced for approval.
+                            newContents.Add(new ToolApprovalResponseContent(response.RequestId, response.Approved, matchedRequest.ToolCall)
+                            {
+                                Reason = response.Reason,
+                            });
+                            (consumedRequestIds ??= new(StringComparer.Ordinal)).Add(response.RequestId);


Since this code is to avoid an extreme edge case, rebuilding the content even when all is fine seems expensive. It results in a new list of contents and new message list each time, even if all the approval response content match.
Consider comparing the ToolApprovalRequestContent/ToolApprovalResponseContent instead, and only rebuilding if there are differences.

westey-m · 2026-07-15T11:00:12Z

+            pendingRequests.RemoveAll(r => consumedRequestIds.Contains(r.RequestId));
+            SavePendingApprovalRequests(pendingRequests, session);


I'm not aware of any case where a caller is able to avoid supplying all ApprovalResponses for all ApprovalRequests. Without it the service call would fail. We could consider just clearing out all pending requests on each call.

westey-m · 2026-07-15T11:03:00Z

+    /// strengthens the human-in-the-loop approval control; disable it only when approval binding is enforced elsewhere.
+    /// </para>
+    /// <para>
+    /// This option has no effect when <see cref="UseProvidedChatClientAsIs"/> is <see langword="true"/>.


The harness agent uses UseProvidedChatClientAsIs, so we probably want to also add this manually to the harness.

westey-m · 2026-07-15T11:12:27Z

+        var surfacedByRequestId = new Dictionary<string, ToolApprovalRequestContent>(StringComparer.Ordinal);
+        foreach (var surfaced in state.SurfacedApprovalRequests)


Consider making state.SurfacedApprovalRequests a dictionary, so that we don't have to rebuild the dictionary each time.

westey-m · 2026-07-15T11:15:10Z

+            surfacedByRequestId[surfaced.RequestId] = surfaced;
+        }
+
+        HashSet<string>? consumedRequestIds = null;


Can we just remove from the dictionary directly rather than having another hashset?

westey-m · 2026-07-15T11:23:21Z

+        var known = new HashSet<string>(StringComparer.Ordinal);
+        foreach (var surfaced in state.SurfacedApprovalRequests)
+        {
+            known.Add(surfaced.RequestId);
+        }


SurfacedApprovalRequests should always be empty when we call RecordSurfacedApprovalRequests, since this is called when we get a response back from the service. When we call the service there can be no outstanding approval requests, as this would cause the service to throw.

So this code shouldn't be necessary, and removing it helps us avoid another hashset allocation.

westey-m · 2026-07-15T11:25:55Z


            // Queue fully resolved — caller should proceed to call the inner agent.
+            // Clear any surfaced requests left over from the completed queue cycle.
+            state.SurfacedApprovalRequests.Clear();


Since this is cleaned up in CollectApprovalResponsesFromMessages, we probably don't need to clear here. It might make sense to have a debug assert though to make sure it's empty, so that we can catch any code bugs.

westey-m · 2026-07-15T11:43:12Z

+    /// </para>
+    /// </remarks>
+    [JsonPropertyName("surfacedApprovalRequests")]
+    public List<ToolApprovalRequestContent> SurfacedApprovalRequests { get; set; } = new();


We should also mark the PR as a breaking change, since adding this will mean that any serialized PRs with outstanding approval requests, will have this empty, and therefore have their approval responses rejected, since they don't exist in this list.

Copilot AI review requested due to automatic review settings July 14, 2026 13:08

giles17 added the .NET Usage: [Issues, PRs], Target: .Net label Jul 14, 2026

rogerbarreto temporarily deployed to integration July 14, 2026 13:08 — with GitHub Actions Inactive

rogerbarreto self-assigned this Jul 14, 2026

Copilot started reviewing on behalf of rogerbarreto July 14, 2026 13:09 View session

github-actions Bot reviewed Jul 14, 2026

View reviewed changes

Comment thread dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs

Comment thread dotnet/src/Microsoft.Agents.AI/ChatClient/ApprovalResponseBindingChatClient.cs

Comment thread dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs

rogerbarreto temporarily deployed to integration July 14, 2026 14:33 — with GitHub Actions Inactive

peibekwe reviewed Jul 14, 2026

View reviewed changes

Comment thread dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs Outdated

peibekwe reviewed Jul 14, 2026

View reviewed changes

Comment thread dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs

peibekwe approved these changes Jul 14, 2026

View reviewed changes

rogerbarreto temporarily deployed to integration July 14, 2026 17:30 — with GitHub Actions Inactive

westey-m reviewed Jul 15, 2026

View reviewed changes

		var pendingRequests = LoadPendingApprovalRequests(session);
		var byRequestId = new Dictionary<string, ToolApprovalRequestContent>(StringComparer.Ordinal);

		pendingRequests.RemoveAll(r => consumedRequestIds.Contains(r.RequestId));
		SavePendingApprovalRequests(pendingRequests, session);

		var surfacedByRequestId = new Dictionary<string, ToolApprovalRequestContent>(StringComparer.Ordinal);
		foreach (var surfaced in state.SurfacedApprovalRequests)

Uh oh!

Conversation

rogerbarreto commented Jul 14, 2026

Motivation and Context

Description

Contribution Checklist

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Automated Code Review

✓ Correctness

✓ Security Reliability

✓ Test Coverage

✓ Failure Modes

✓ Design Approach

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

westey-m Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westey-m Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westey-m Jul 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

westey-m Jul 15, 2026 •

edited

Loading

westey-m Jul 15, 2026 •

edited

Loading

westey-m Jul 15, 2026 •

edited

Loading