diff --git a/dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs b/dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs index bfbdd4b5eb5..aa4cf085d35 100644 --- a/dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs +++ b/dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs @@ -263,6 +263,12 @@ protected override async IAsyncEnumerable RunCoreStreamingA if (cappedRequests is not null) { + this.EnsureRequestIdsUnique( + cappedRequests, + state, + session, + includeSurfacedRequests: recordedAnyCappedRequest); + // The first update carrying requests supersedes any earlier batch; later updates in this // same turn add to it, since every one of them reaches the caller. if (recordedAnyCappedRequest) @@ -348,6 +354,8 @@ protected override async IAsyncEnumerable RunCoreStreamingA yield break; } + this.EnsureRequestIdsUnique(streamedApprovalRequests, state, session, includeSurfacedRequests: false); + // 4. Classify the collected approval requests against standing rules and auto-approval rules. List unapproved = []; foreach (var tarc in streamedApprovalRequests) @@ -418,6 +426,7 @@ private void RecordSurfacedApprovalRequestsFromMessages( if (requests is not null) { + this.EnsureRequestIdsUnique(requests, state, session, includeSurfacedRequests: false); ResetSurfacedApprovalRequests(state, requests); this._sessionState.SaveState(session, state); } @@ -431,12 +440,6 @@ private void RecordSurfacedApprovalRequestsFromMessages( /// /// A new batch from the inner agent supersedes any previous one, so stale entries from an abandoned /// approval cycle cannot later authorize a standing rule. - /// - /// Request ids are unique by construction, so keying by id loses nothing: they derive from tool call ids, and - /// inference services correlate a tool call to its result by that id alone. A duplicate id would already have - /// broken that correlation upstream. Nothing a caller sends reaches this dictionary — every entry originates - /// from the inner agent's own response — so a caller cannot manufacture a collision here. - /// /// private static void ResetSurfacedApprovalRequests(ToolApprovalState state, IReadOnlyList requests) { @@ -477,6 +480,61 @@ private static ToolApprovalRequestContent SnapshotRequest(ToolApprovalRequestCon return request; } + /// + /// Rejects duplicate active approval request IDs. + /// + private void EnsureRequestIdsUnique( + List requests, + ToolApprovalState state, + AgentSession? session, + bool includeSurfacedRequests) + { + if (requests.Count == 0) + { + return; + } + + var knownRequestIds = new HashSet(StringComparer.Ordinal); + if (includeSurfacedRequests) + { + foreach (string requestId in state.SurfacedApprovalRequests.Keys) + { + knownRequestIds.Add(requestId); + } + } + + HashSet? duplicateRequestIds = null; + + foreach (var request in requests) + { + if (knownRequestIds.Add(request.RequestId)) + { + continue; + } + + (duplicateRequestIds ??= new(StringComparer.Ordinal)).Add(request.RequestId); + } + + if (duplicateRequestIds is null || duplicateRequestIds.Count == 0) + { + return; + } + + foreach (string requestId in duplicateRequestIds) + { + state.SurfacedApprovalRequests.Remove(requestId); + state.QueuedApprovalRequests.RemoveAll( + queued => string.Equals(queued.RequestId, requestId, StringComparison.Ordinal)); + state.CollectedApprovalResponses.RemoveAll( + response => string.Equals(response.RequestId, requestId, StringComparison.Ordinal)); + } + + this._sessionState.SaveState(session, state); + + throw new InvalidOperationException( + $"Tool approval request IDs '{string.Join("', '", duplicateRequestIds.OrderBy(id => id, StringComparer.Ordinal))}' were used more than once."); + } + /// /// Re-evaluates queued approval requests against current rules and auto-approval rules, and auto-approves any that now match. /// @@ -606,6 +664,12 @@ private async ValueTask ProcessAndQueueOutboundApprovalRequestsAsync( AgentRunOptions? options, IReadOnlyCollection requestMessages) { + var requests = responseMessages + .SelectMany(message => message.Contents) + .OfType() + .ToList(); + this.EnsureRequestIdsUnique(requests, state, session, includeSurfacedRequests: false); + // Pass 1: Scan all response messages and classify each approval request. // Auto-approved requests (matching a standing rule or auto-approval rule) have their // responses collected immediately, preserving the original request order, and are diff --git a/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs b/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs index ad785d0e9e2..a80e7f24c96 100644 --- a/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs +++ b/dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs @@ -3285,6 +3285,205 @@ public async Task RunStreamingAsync_AutoApprovalRule_ApprovesMatchingToolAsync() #endregion + #region Duplicate approval request IDs + + /// + /// Verify that one request ID cannot refer to two different tool calls. + /// + [Fact] + public async Task RunAsync_DifferentCallsWithSameRequestId_ThrowsAsync() + { + // Arrange + var harmless = new ToolApprovalRequestContent( + "shared-id", + new FunctionCallContent("shared-call", "BenignLookup", new Dictionary { ["query"] = "weather" })); + var privileged = new ToolApprovalRequestContent( + "shared-id", + new FunctionCallContent("shared-call", "RunPrivilegedShell", new Dictionary { ["command"] = "sensitive-command" })); + var innerAgent = CreateMockAgent(new AgentResponse([new ChatMessage(ChatRole.Assistant, [harmless, privileged])])); + var agent = new ToolApprovalAgent(innerAgent.Object); + + // Act + var exception = await Assert.ThrowsAsync( + () => agent.RunAsync([new ChatMessage(ChatRole.User, "Go")], new ChatClientAgentSession())); + + // Assert + Assert.Contains("shared-id", exception.Message, StringComparison.Ordinal); + Assert.DoesNotContain("RunPrivilegedShell", exception.Message, StringComparison.Ordinal); + Assert.DoesNotContain("sensitive-command", exception.Message, StringComparison.Ordinal); + } + + /// + /// Verify that repeating the exact same request under one ID is rejected. + /// + [Fact] + public async Task RunAsync_EquivalentCallsWithSameRequestId_ThrowsAsync() + { + // Arrange + var first = new ToolApprovalRequestContent( + "shared-id", + new FunctionCallContent("shared-call", "BenignLookup", new Dictionary { ["query"] = "weather" })); + var repeated = new ToolApprovalRequestContent( + "shared-id", + new FunctionCallContent("shared-call", "BenignLookup", new Dictionary { ["query"] = "weather" })); + var agent = new ToolApprovalAgent( + CreateMockAgent(new AgentResponse([new ChatMessage(ChatRole.Assistant, [first, repeated])])).Object); + + // Act & Assert + await Assert.ThrowsAsync( + () => agent.RunAsync([new ChatMessage(ChatRole.User, "Go")], new ChatClientAgentSession())); + } + + /// + /// Verify that streaming batches are checked before any conflicting request is surfaced. + /// + [Fact] + public async Task RunStreamingAsync_DifferentCallsWithSameRequestId_ThrowsBeforeYieldAsync() + { + // Arrange + var harmless = new ToolApprovalRequestContent("shared-id", new FunctionCallContent("shared-call", "BenignLookup")); + var privileged = new ToolApprovalRequestContent("shared-id", new FunctionCallContent("shared-call", "RunPrivilegedShell")); + var innerAgent = CreateMockStreamingAgent( + [ + new AgentResponseUpdate(ChatRole.Assistant, new List { harmless }), + new AgentResponseUpdate(ChatRole.Assistant, new List { privileged }), + ]); + var agent = new ToolApprovalAgent(innerAgent.Object); + var yielded = new List(); + + // Act + var exception = await Assert.ThrowsAsync(async () => + { + await foreach (var update in agent.RunStreamingAsync( + [new ChatMessage(ChatRole.User, "Go")], + new ChatClientAgentSession())) + { + yielded.Add(update); + } + }); + + // Assert + Assert.Empty(yielded); + Assert.Contains("shared-id", exception.Message, StringComparison.Ordinal); + } + + /// + /// Verify that the final non-streaming turn at the auto-approval cap also rejects conflicting IDs. + /// + [Fact] + public async Task RunAsync_CappedResponseWithDifferentCallsUnderSameRequestId_ThrowsAsync() + { + // Arrange + var callCount = 0; + var innerAgent = new Mock(); + innerAgent + .Protected() + .Setup>("RunCoreAsync", + ItExpr.IsAny>(), + ItExpr.IsAny(), + ItExpr.IsAny(), + ItExpr.IsAny()) + .ReturnsAsync(() => ++callCount == 1 + ? new AgentResponse([new ChatMessage(ChatRole.Assistant, + [new ToolApprovalRequestContent("auto", new FunctionCallContent("auto-call", "AutoTool"))])]) + : new AgentResponse([new ChatMessage(ChatRole.Assistant, + [ + new ToolApprovalRequestContent("shared-id", new FunctionCallContent("shared-call", "BenignLookup")), + new ToolApprovalRequestContent("shared-id", new FunctionCallContent("shared-call", "RunPrivilegedShell")), + ])])); + var agent = new ToolApprovalAgent(innerAgent.Object, new ToolApprovalAgentOptions + { + AutoApprovalRules = [ToolApprovalAgent.AllToolsAutoApprovalRule], + MaxAutoApprovalIterations = 1, + }); + + // Act & Assert + await Assert.ThrowsAsync( + () => agent.RunAsync([new ChatMessage(ChatRole.User, "Go")], new ChatClientAgentSession())); + } + + /// + /// Verify that the final streaming turn at the auto-approval cap invalidates every ID repeated by a later update. + /// + [Fact] + public async Task RunStreamingAsync_CappedUpdatesWithMultipleDuplicateIds_InvalidatesAllAsync() + { + // Arrange + var callCount = 0; + var firstA = new ToolApprovalRequestContent("id-a", new FunctionCallContent("call-a", "ToolA")); + var firstB = new ToolApprovalRequestContent("id-b", new FunctionCallContent("call-b", "ToolB")); + var innerAgent = new Mock(); + innerAgent + .Protected() + .Setup>("RunCoreStreamingAsync", + ItExpr.IsAny>(), + ItExpr.IsAny(), + ItExpr.IsAny(), + ItExpr.IsAny()) + .Returns(() => ++callCount == 1 + ? ToAsyncEnumerableAsync( + [ + new AgentResponseUpdate(ChatRole.Assistant, new List + { + new ToolApprovalRequestContent("auto", new FunctionCallContent("auto-call", "AutoTool")), + }), + ]) + : ToAsyncEnumerableAsync( + [ + new AgentResponseUpdate(ChatRole.Assistant, new List + { + firstA, + firstB, + }), + new AgentResponseUpdate(ChatRole.Assistant, new List + { + new ToolApprovalRequestContent("id-a", new FunctionCallContent("repeated-call-a", "DifferentToolA")), + new ToolApprovalRequestContent("id-b", new FunctionCallContent("repeated-call-b", "DifferentToolB")), + }), + ])); + var agent = new ToolApprovalAgent(innerAgent.Object, new ToolApprovalAgentOptions + { + AutoApprovalRules = [ToolApprovalAgent.AllToolsAutoApprovalRule], + MaxAutoApprovalIterations = 1, + }); + var session = new ChatClientAgentSession(); + + // Act + var exception = await Assert.ThrowsAsync(async () => + { + await foreach (var _ in agent.RunStreamingAsync( + [new ChatMessage(ChatRole.User, "Go")], + session)) + { + } + }); + + var nextA = new ToolApprovalRequestContent("next-a", new FunctionCallContent("next-call-a", "ToolA")); + var nextB = new ToolApprovalRequestContent("next-b", new FunctionCallContent("next-call-b", "ToolB")); + var followUpAgent = new ToolApprovalAgent( + CreateMockAgent(new AgentResponse([new ChatMessage(ChatRole.Assistant, [nextA, nextB])])).Object); + + var firstResponse = await followUpAgent.RunAsync( + [new ChatMessage(ChatRole.User, + [ + firstA.CreateAlwaysApproveToolResponse(), + firstB.CreateAlwaysApproveToolResponse(), + ])], + session); + + var secondResponse = await followUpAgent.RunAsync( + [new ChatMessage(ChatRole.User, [nextA.CreateResponse(approved: true)])], + session); + + // Assert + Assert.Contains("id-a", exception.Message, StringComparison.Ordinal); + Assert.Contains("id-b", exception.Message, StringComparison.Ordinal); + Assert.Equal("next-a", firstResponse.Messages.SelectMany(m => m.Contents).OfType().Single().RequestId); + Assert.Equal("next-b", secondResponse.Messages.SelectMany(m => m.Contents).OfType().Single().RequestId); + } + + #endregion + #region Auto-approval iteration cap ///