From 2533395adcfd8b7b3c56a6a043e32de8a6e10954 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 19:04:05 +0000 Subject: [PATCH 1/4] Initial plan From fb396b9174288ecdd5af08cf862600aa981cbff2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 19:11:17 +0000 Subject: [PATCH 2/4] Fix tools/list infinite loop by removing notification from ListTools handler and adding change detection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP server was entering an infinite tools/list loop because: 1. HandleListToolsAsync sent tools/list_changed notification after refreshing 2. The client responded with another tools/list request 3. This created a feedback loop: list → changed → list → changed Fix: - Remove SendToolsListChangedNotificationAsync from HandleListToolsAsync (the client already gets the fresh list since it requested it) - Add change detection to RefreshResourceToolMapAsync (returns bool Changed) - Only send tools/list_changed in HandleCallToolAsync when tools actually changed - RefreshToolsTool always sends notification (explicit user action) Co-authored-by: maddymontaquila <12660687+maddymontaquila@users.noreply.github.com> --- src/Aspire.Cli/Commands/AgentMcpCommand.cs | 16 +++-- .../Mcp/IMcpResourceToolRefreshService.cs | 4 +- .../Mcp/McpResourceToolRefreshService.cs | 7 +- src/Aspire.Cli/Mcp/Tools/RefreshToolsTool.cs | 2 +- .../Commands/AgentMcpCommandTests.cs | 67 +++++++++++++++++++ 5 files changed, 87 insertions(+), 9 deletions(-) diff --git a/src/Aspire.Cli/Commands/AgentMcpCommand.cs b/src/Aspire.Cli/Commands/AgentMcpCommand.cs index bb239d4ee48..3aff0911c2a 100644 --- a/src/Aspire.Cli/Commands/AgentMcpCommand.cs +++ b/src/Aspire.Cli/Commands/AgentMcpCommand.cs @@ -147,8 +147,12 @@ private async ValueTask HandleListToolsAsync(RequestContext new Tool @@ -193,8 +197,12 @@ private async ValueTask HandleCallToolAsync(RequestContext /// The cancellation token. - /// The refreshed resource tool map. - Task> RefreshResourceToolMapAsync(CancellationToken cancellationToken); + /// A tuple containing the refreshed resource tool map and a flag indicating whether the tool set changed. + Task<(IReadOnlyDictionary ToolMap, bool Changed)> RefreshResourceToolMapAsync(CancellationToken cancellationToken); /// /// Sends a tools list changed notification to connected MCP clients. diff --git a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs index 95ecf656214..7efe95ec9f7 100644 --- a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs +++ b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs @@ -71,7 +71,7 @@ public async Task SendToolsListChangedNotificationAsync(CancellationToken cancel } /// - public async Task> RefreshResourceToolMapAsync(CancellationToken cancellationToken) + public async Task<(IReadOnlyDictionary ToolMap, bool Changed)> RefreshResourceToolMapAsync(CancellationToken cancellationToken) { _logger.LogDebug("Refreshing resource tool map."); @@ -117,10 +117,13 @@ public async Task> RefreshResourc lock (_lock) { + var changed = !_resourceToolMap.Keys.OrderBy(k => k, StringComparer.Ordinal) + .SequenceEqual(refreshedMap.Keys.OrderBy(k => k, StringComparer.Ordinal), StringComparer.Ordinal); + _resourceToolMap = refreshedMap; _selectedAppHostPath = selectedAppHostPath; _invalidated = false; - return _resourceToolMap; + return (_resourceToolMap, changed); } } } diff --git a/src/Aspire.Cli/Mcp/Tools/RefreshToolsTool.cs b/src/Aspire.Cli/Mcp/Tools/RefreshToolsTool.cs index c8d3d5c4fef..e0a919c7333 100644 --- a/src/Aspire.Cli/Mcp/Tools/RefreshToolsTool.cs +++ b/src/Aspire.Cli/Mcp/Tools/RefreshToolsTool.cs @@ -19,7 +19,7 @@ public override JsonElement GetInputSchema() public override async ValueTask CallToolAsync(CallToolContext context, CancellationToken cancellationToken) { - var resourceToolMap = await refreshService.RefreshResourceToolMapAsync(cancellationToken).ConfigureAwait(false); + var (resourceToolMap, _) = await refreshService.RefreshResourceToolMapAsync(cancellationToken).ConfigureAwait(false); await refreshService.SendToolsListChangedNotificationAsync(cancellationToken).ConfigureAwait(false); var totalToolCount = KnownMcpTools.All.Count + resourceToolMap.Count; diff --git a/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs b/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs index dd7cb3e3587..d7168279ddd 100644 --- a/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs +++ b/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs @@ -347,6 +347,73 @@ public async Task McpServer_CallTool_RefreshTools_ReturnsResult() Assert.Equal(NotificationMethods.ToolListChangedNotification, notification.Method); } + [Fact] + public async Task McpServer_ListTools_DoesNotSendToolsListChangedNotification() + { + // Arrange - Create a mock backchannel with a resource that has MCP tools + // This simulates the db-mcp scenario where resource tools become available + var mockBackchannel = new TestAppHostAuxiliaryBackchannel + { + Hash = "test-apphost-hash", + IsInScope = true, + AppHostInfo = new AppHostInformation + { + AppHostPath = Path.Combine(_workspace.WorkspaceRoot.FullName, "TestAppHost", "TestAppHost.csproj"), + ProcessId = 12345 + }, + ResourceSnapshots = + [ + new ResourceSnapshot + { + Name = "db-mcp", + DisplayName = "DB MCP", + ResourceType = "Container", + State = "Running", + McpServer = new ResourceSnapshotMcpServer + { + EndpointUrl = "http://localhost:8080/mcp", + Tools = + [ + new Tool + { + Name = "query_database", + Description = "Query a database" + } + ] + } + } + ] + }; + + // Register the mock backchannel so resource tools will be discovered + _backchannelMonitor.AddConnection(mockBackchannel.Hash, mockBackchannel.SocketPath, mockBackchannel); + + // Set up a channel to detect any tools/list_changed notifications + var notificationCount = 0; + await using var notificationHandler = _mcpClient.RegisterNotificationHandler( + NotificationMethods.ToolListChangedNotification, + (notification, cancellationToken) => + { + Interlocked.Increment(ref notificationCount); + return default; + }); + + // Act - Call ListTools which should discover the resource tools via refresh + // but should NOT send a tools/list_changed notification (that would cause an infinite loop) + var tools = await _mcpClient.ListToolsAsync(cancellationToken: _cts.Token).DefaultTimeout(); + + // Give a small window for any spurious notification to arrive + await Task.Delay(200, _cts.Token); + + // Assert - tools should include the resource tool + Assert.NotNull(tools); + var dbMcpTool = tools.FirstOrDefault(t => t.Name == "db_mcp_query_database"); + Assert.NotNull(dbMcpTool); + + // Assert - no tools/list_changed notification should have been sent + Assert.Equal(0, notificationCount); + } + [Fact] public async Task McpServer_CallTool_UnknownTool_ReturnsError() { From fd19fcf9cc63c4d21c3d7cebf9930cd4ca529d9c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 6 Feb 2026 19:12:36 +0000 Subject: [PATCH 3/4] Use HashSet.SetEquals for more efficient tool change detection Co-authored-by: maddymontaquila <12660687+maddymontaquila@users.noreply.github.com> --- src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs index 7efe95ec9f7..9269a95709b 100644 --- a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs +++ b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs @@ -117,8 +117,8 @@ public async Task SendToolsListChangedNotificationAsync(CancellationToken cancel lock (_lock) { - var changed = !_resourceToolMap.Keys.OrderBy(k => k, StringComparer.Ordinal) - .SequenceEqual(refreshedMap.Keys.OrderBy(k => k, StringComparer.Ordinal), StringComparer.Ordinal); + var changed = _resourceToolMap.Count != refreshedMap.Count + || !_resourceToolMap.Keys.ToHashSet(StringComparer.Ordinal).SetEquals(refreshedMap.Keys); _resourceToolMap = refreshedMap; _selectedAppHostPath = selectedAppHostPath; From f7b0df45467218f952a11e617d006826c03cbda4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 13 Feb 2026 16:50:01 +0000 Subject: [PATCH 4/4] Address review feedback: use no-allocation key comparison and bounded channel wait in test - Replace HashSet.SetEquals with count check + iterate keys + ContainsKey to avoid allocation under lock in McpResourceToolRefreshService. - Replace Task.Delay(200) in test with Channel.ReadAsync + CancellationTokenSource timeout for more deterministic negative assertion. Co-authored-by: sebastienros <1165805+sebastienros@users.noreply.github.com> --- .../Mcp/McpResourceToolRefreshService.cs | 14 +++++++-- .../Commands/AgentMcpCommandTests.cs | 29 ++++++++++++++++--- 2 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs index 9269a95709b..7ae5d78daed 100644 --- a/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs +++ b/src/Aspire.Cli/Mcp/McpResourceToolRefreshService.cs @@ -117,8 +117,18 @@ public async Task SendToolsListChangedNotificationAsync(CancellationToken cancel lock (_lock) { - var changed = _resourceToolMap.Count != refreshedMap.Count - || !_resourceToolMap.Keys.ToHashSet(StringComparer.Ordinal).SetEquals(refreshedMap.Keys); + var changed = _resourceToolMap.Count != refreshedMap.Count; + if (!changed) + { + foreach (var key in _resourceToolMap.Keys) + { + if (!refreshedMap.ContainsKey(key)) + { + changed = true; + break; + } + } + } _resourceToolMap = refreshedMap; _selectedAppHostPath = selectedAppHostPath; diff --git a/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs b/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs index d7168279ddd..916d5290270 100644 --- a/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs +++ b/tests/Aspire.Cli.Tests/Commands/AgentMcpCommandTests.cs @@ -402,15 +402,36 @@ public async Task McpServer_ListTools_DoesNotSendToolsListChangedNotification() // but should NOT send a tools/list_changed notification (that would cause an infinite loop) var tools = await _mcpClient.ListToolsAsync(cancellationToken: _cts.Token).DefaultTimeout(); - // Give a small window for any spurious notification to arrive - await Task.Delay(200, _cts.Token); - // Assert - tools should include the resource tool Assert.NotNull(tools); var dbMcpTool = tools.FirstOrDefault(t => t.Name == "db_mcp_query_database"); Assert.NotNull(dbMcpTool); - // Assert - no tools/list_changed notification should have been sent + // Assert - no tools/list_changed notification should have been sent. + // Use a bounded wait via TryRead to catch any late-arriving notification + // without relying on an arbitrary Task.Delay. + using var timeoutCts = new CancellationTokenSource(TimeSpan.FromMilliseconds(200)); + var notificationChannel = Channel.CreateUnbounded(); + await using var channelHandler = _mcpClient.RegisterNotificationHandler( + NotificationMethods.ToolListChangedNotification, + (notification, _) => + { + notificationChannel.Writer.TryWrite(notification); + return default; + }); + + var received = false; + try + { + await notificationChannel.Reader.ReadAsync(timeoutCts.Token); + received = true; + } + catch (OperationCanceledException) + { + // Expected — no notification arrived within the timeout + } + + Assert.False(received, "tools/list_changed notification should not be sent during tools/list handling"); Assert.Equal(0, notificationCount); }