Add availability check for NuGet Solver MCP Server tool#7149
Conversation
db16929
| private const string AuthStatusDetermined = "c936efcc-6baa-4ad3-9c2b-7ba750acf18f"; | ||
| private const string ServiceName = "Microsoft.VisualStudio.Copilot.SolutionContextProvider"; | ||
| private const string NuGetMCPServerName = "nuget"; | ||
| private const string NuGetSolverToolName = "get-nuget-solver"; |
There was a problem hiding this comment.
Is this the best place to define these strings, particularly as we add more entry points to MCP tools in the NuGet MCP Server from Settings and PM UI? Also, I'm wondering if hardcoding these strings is the only solution. Maybe, at least, could we make sure all these definitions contain "MCP" in the name so that a "Find all" can help us catch all the places that would need to be updated if we ever change the tool names? Thus, could we change NuGetSolverToolName to NuGetSolverMCPToolName?
There was a problem hiding this comment.
Is this the best place to define these strings, particularly as we add more entry points to MCP tools in the NuGet MCP Server from Settings and PM UI? Also, I'm wondering if hardcoding these strings is the only solution. Maybe, at least, could we make sure all these definitions contain "MCP" in the name so that a "Find all" can help us catch all the places that would need to be updated if we ever change the tool names? Thus, could we change NuGetSolverToolName to NuGetSolverMCPToolName?
Thanks for the questions @aortiz-msft ! You bring up great points, especially to consider where to put these strings as our dependency on the MCP Server tools grows. One option would be to have something like an McpServerToolNameConstants class that we can refer to. It still has hard-coded strings as constants, but at least they're all centralized in one place.
I put the constants inline where they're used as the first iterative step. We can pull them out into a constants class as our requirements grow. We can engineer something shares or syncs between the repos as well but that will require adding a dependency that I think would be best to consider later, if needed and not right now. The current solution is the lowest effort but I think it can benefit from using a centralized constants class that would be an improvement.
Thanks to the suggestion by @kartheekp-ms here, I created an issue to add a comment in the mcp.json file to update the constants when the naming changes in that file. When I tackle that I can also move these constants out of this file and into a separate McpConstants class. Thoughts?
There was a problem hiding this comment.
@aortiz-msft and @kartheekp-ms I have a follow-up PR ready: #7168
aortiz-msft
left a comment
There was a problem hiding this comment.
Sorry for the post-merge feedback.
Bug
Fixes: https://github.com/NuGet/Client.Engineering/issues/3631
Description
Check availability of NuGet Solver MCP Server tool. If unavailable, show a warning with instructions to enable the NuGet MCP Server, and block the operation. Note: the NuGet Solver MCP Server tool will be available as long as the NuGet MCP Server itself is enabled.
Warning dialog:
PR Checklist
Added testsLink to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.