Add consolidated dotnet_workload tool with enum-based action routing #222
Conversation
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| foreach (var id in workloadIds) | ||
| { | ||
| if (!ParameterValidator.ValidateWorkloadId(id, out var validationError)) | ||
| { | ||
| if (machineReadable) | ||
| { | ||
| var error = new ErrorResponse | ||
| { | ||
| Success = false, | ||
| Errors = new List<ErrorResult> | ||
| { | ||
| new ErrorResult | ||
| { | ||
| Code = "INVALID_PARAMETER", | ||
| Message = validationError!, | ||
| Category = "Validation", | ||
| Hint = "Workload IDs must contain only alphanumeric characters, hyphens, and underscores", | ||
| McpErrorCode = McpErrorCodes.InvalidParams | ||
| } | ||
| }, | ||
| ExitCode = -1 | ||
| }; | ||
| return ErrorResultFactory.ToJson(error); | ||
| } | ||
| return $"Error: {validationError}"; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to address a “missed opportunity to use Where” warning, you replace a foreach loop that checks a condition on each element with iteration over a sequence already filtered by .Where(predicate). This makes the filtering intent explicit and can reduce nesting and conditional noise inside the loop.
Here, we must preserve semantics: we want to find the first invalid workload ID, get its validationError message (from ParameterValidator.ValidateWorkloadId), and return the same error as before. The safest approach is:
- Use
Whereto project the sequence of invalid IDs, but only materialize the first invalid ID to preserve short-circuit-like behavior. - Still call
ValidateWorkloadIdexactly once for that ID so that we can retrievevalidationError. We can’t easily get the out parameter from inside theWherepredicate without making the code more complex than necessary. - Leave the rest of the method (error construction, machineReadable handling, etc.) unchanged.
Concretely, in ValidateWorkloadIds in DotNetMcp/Tools/Cli/DotNetCliTools.Workload.Consolidated.cs:
- Replace the
foreach (var id in workloadIds)loop with:var firstInvalidId = workloadIds.FirstOrDefault(id => !ParameterValidator.ValidateWorkloadId(id, out _));- An
if (firstInvalidId != null)block where we callValidateWorkloadId(firstInvalidId, out var validationError)again to get the message, then build and return the same error responses as before.
- Add
using System.Linq;at the top to enableFirstOrDefault.
This keeps external behavior identical while satisfying the CodeQL recommendation to make the filtering explicit via LINQ.
| @@ -1,4 +1,5 @@ | ||
| using System.Text; | ||
| using System.Linq; | ||
| using DotNetMcp.Actions; | ||
| using ModelContextProtocol.Server; | ||
|
|
||
| @@ -195,32 +196,33 @@ | ||
| } | ||
|
|
||
| // Validate each workload ID | ||
| foreach (var id in workloadIds) | ||
| var firstInvalidId = workloadIds.FirstOrDefault(id => !ParameterValidator.ValidateWorkloadId(id, out _)); | ||
| if (firstInvalidId != null) | ||
| { | ||
| if (!ParameterValidator.ValidateWorkloadId(id, out var validationError)) | ||
| // Re-validate the first invalid ID to capture the validation error message | ||
| ParameterValidator.ValidateWorkloadId(firstInvalidId, out var validationError); | ||
|
|
||
| if (machineReadable) | ||
| { | ||
| if (machineReadable) | ||
| var error = new ErrorResponse | ||
| { | ||
| var error = new ErrorResponse | ||
| Success = false, | ||
| Errors = new List<ErrorResult> | ||
| { | ||
| Success = false, | ||
| Errors = new List<ErrorResult> | ||
| new ErrorResult | ||
| { | ||
| new ErrorResult | ||
| { | ||
| Code = "INVALID_PARAMETER", | ||
| Message = validationError!, | ||
| Category = "Validation", | ||
| Hint = "Workload IDs must contain only alphanumeric characters, hyphens, and underscores", | ||
| McpErrorCode = McpErrorCodes.InvalidParams | ||
| } | ||
| }, | ||
| ExitCode = -1 | ||
| }; | ||
| return ErrorResultFactory.ToJson(error); | ||
| } | ||
| return $"Error: {validationError}"; | ||
| Code = "INVALID_PARAMETER", | ||
| Message = validationError!, | ||
| Category = "Validation", | ||
| Hint = "Workload IDs must contain only alphanumeric characters, hyphens, and underscores", | ||
| McpErrorCode = McpErrorCodes.InvalidParams | ||
| } | ||
| }, | ||
| ExitCode = -1 | ||
| }; | ||
| return ErrorResultFactory.ToJson(error); | ||
| } | ||
| return $"Error: {validationError}"; | ||
| } | ||
|
|
||
| return null; // Validation succeeded |
There was a problem hiding this comment.
Pull request overview
This PR implements the consolidated dotnet_workload tool as part of the strategic tool surface consolidation initiative. It replaces 6 individual workload-related tools with a single enum-driven interface that routes actions through a switch statement pattern.
Key changes:
- Single
DotnetWorkloadmethod withDotnetWorkloadActionenum parameter supporting 6 actions - Shared validation logic via
ValidateWorkloadIdshelper method eliminates duplication - Comprehensive test suite with 39 test cases covering all actions, validation paths, and error conditions
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| DotNetMcp/Tools/Cli/DotNetCliTools.Workload.Consolidated.cs | Implements consolidated workload tool with action routing, validation, and machine-readable error handling |
| DotNetMcp.Tests/Tools/ConsolidatedWorkloadToolTests.cs | Comprehensive test coverage for all 6 actions, parameter validation, and error scenarios |
| foreach (var id in workloadIds) | ||
| { | ||
| if (!ParameterValidator.ValidateWorkloadId(id, out var validationError)) | ||
| { | ||
| if (machineReadable) | ||
| { | ||
| var error = new ErrorResponse | ||
| { | ||
| Success = false, | ||
| Errors = new List<ErrorResult> | ||
| { | ||
| new ErrorResult | ||
| { | ||
| Code = "INVALID_PARAMETER", | ||
| Message = validationError!, | ||
| Category = "Validation", | ||
| Hint = "Workload IDs must contain only alphanumeric characters, hyphens, and underscores", | ||
| McpErrorCode = McpErrorCodes.InvalidParams | ||
| } | ||
| }, | ||
| ExitCode = -1 | ||
| }; | ||
| return ErrorResultFactory.ToJson(error); | ||
| } | ||
| return $"Error: {validationError}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This foreach loop implicitly filters its target sequence - consider filtering the sequence explicitly using '.Where(...)'.
| foreach (var action in actions) | ||
| { | ||
| // Act | ||
| var result = action switch | ||
| { | ||
| DotnetWorkloadAction.Install => await _tools.DotnetWorkload(action, workloadIds: new[] { "test-id" }), | ||
| DotnetWorkloadAction.Uninstall => await _tools.DotnetWorkload(action, workloadIds: new[] { "test-id" }), | ||
| _ => await _tools.DotnetWorkload(action) | ||
| }; | ||
|
|
||
| // Assert | ||
| Assert.NotNull(result); | ||
| // Should not throw or return "Unsupported action" | ||
| Assert.DoesNotContain("Unsupported action", result); | ||
| } |
There was a problem hiding this comment.
This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
There was a problem hiding this comment.
Refactored to use .Select() with Task.WhenAll() for parallel execution. (b7b4d72)
1 similar comment
Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com>
Add global using for action enums and make DotnetWorkload a partial tool method so MCP tool descriptions are generated from XML docs.
|
Reviewer note: This PR switches consolidated tools to rely on MCP’s XML-doc → description source generator.
If you’re reviewing consolidated tools going forward: prefer |
Implementation Plan: Consolidated dotnet_workload Tool
Based on doc/tool-surface-consolidation.md, I need to implement a consolidated
dotnet_workloadtool that replaces the existing 6 individual workload tools.Checklist:
DotnetWorkloadtool method in new fileDotNetCliTools.Workload.Consolidated.csDotnetWorkloadActionenum parameter (List, Info, Search, Install, Update, Uninstall)ConsolidatedWorkloadToolTests.csOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.