Add validation for service start --transport option#511
Merged
jongio merged 10 commits intomicrosoft:mainfrom Sep 22, 2025
Merged
Add validation for service start --transport option#511jongio merged 10 commits intomicrosoft:mainfrom
jongio merged 10 commits intomicrosoft:mainfrom
Conversation
jongio
requested review from
a team,
regexrowboat,
srnagar,
xiangyan99 and
xiaomi7732
as code owners
jongio
requested review from
JonathanCrd,
fanyang-mono,
jairmyree,
joshfree,
tmeschter and
vukelich
Contributor
There was a problem hiding this comment.
Pull Request Overview
Adds validation for the --transport option in ServiceStartCommand to prevent the server from hanging on invalid input and introduces tests for invalid and valid transport values. Also performs cleanup by removing unused using directives across multiple files. Adds targeted unit tests for transport validation logic.
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.Search/tests/Azure.Mcp.Tools.Search.UnitTests/Index/IndexGetCommandTests.cs | Removed unused static using. |
| tools/Azure.Mcp.Tools.Monitor/src/Commands/HealthModels/BaseMonitorHealthModelsCommand.cs | Removed unused usings. |
| tools/Azure.Mcp.Tools.Monitor/src/Commands/BaseWorkspaceMonitorCommand.cs | Removed unused using. |
| tools/Azure.Mcp.Tools.Foundry/src/Services/Models/CognitiveServicesAccountDeploymentProperties.cs | Removed unused using. |
| tools/Azure.Mcp.Tools.Foundry/src/Services/Models/CognitiveServicesAccountDeploymentData.cs | Removed unused using. |
| tools/Azure.Mcp.Tools.EventGrid/src/Services/IEventGridService.cs | Removed unused using. |
| tools/Azure.Mcp.Tools.EventGrid/src/Services/EventGridService.cs | Removed unused usings. |
| tools/Azure.Mcp.Tools.ApplicationInsights/tests/.../RecommendationListCommandTests.cs | Removed unused usings. |
| core/Azure.Mcp.Core/tests/.../CommandResultExtensionsTests.cs | Removed unused using. |
| core/Azure.Mcp.Core/tests/.../ServiceStartCommandTests.cs | Added tests for transport option validation. |
| core/Azure.Mcp.Core/src/Areas/Server/Commands/ServiceStartCommand.cs | Added transport retrieval and validation logic. |
jongio
requested review from
ericshape,
jeremyfrosti and
qianwens
as code owners
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
core/Azure.Mcp.Core/tests/Azure.Mcp.Core.UnitTests/Areas/Server/ServiceStartCommandTests.cs:1
- The test calls a private method
ValidateTransportbut this method doesn't exist in the test class. This test will fail at runtime. The method should either be implemented in the test class or use reflection to call the private method in the command class.
// Copyright (c) Microsoft Corporation.
alzimmermsft
approved these changes
Sep 22, 2025
alzimmermsft
approved these changes
Sep 22, 2025
…commands to work with BindOptions
…lity and early returns; add BindOptions method in ToolsListCommand; enhance BaseCommand with option binding and error handling improvements.
…ode enum - Updated assertions in SessionHostListCommandTests to replace integer status codes with HttpStatusCode enum values for better readability and maintainability. - Made similar updates across various unit test files including SessionHostUserSessionListCommandTests, CreateWorkbooksCommandTests, DeleteWorkbooksCommandTests, ListWorkbooksCommandTests, ShowWorkbooksCommandTests, UpdateWorkbooksCommandTests, and Public API command tests. - Ensured consistency in error handling by using HttpStatusCode for all relevant assertions.
…roved clarity and consistency across commands and tests.
… enum for improved clarity and maintainability.
…SerializeCorrectly test to use HttpStatusCode enum for improved clarity and consistency.
alzimmermsft
approved these changes
Sep 22, 2025
colbytimm
pushed a commit
to colbytimm/microsoft-mcp
that referenced
this pull request
Sep 27, 2025
* Add validation for service start --transport option * Update inheritance model to allow ServiceStart and other BaseCommand commands to work with BindOptions * Formatting * Refactor validation method to remove nullable parameter and update tests accordingly * Make BindOptions method abstract to enforce implementation in derived classes * Refactor validation logic in ServiceStartCommand for improved readability and early returns; add BindOptions method in ToolsListCommand; enhance BaseCommand with option binding and error handling improvements. * Refactor HTTP status code assertions in unit tests to use HttpStatusCode enum - Updated assertions in SessionHostListCommandTests to replace integer status codes with HttpStatusCode enum values for better readability and maintainability. - Made similar updates across various unit test files including SessionHostUserSessionListCommandTests, CreateWorkbooksCommandTests, DeleteWorkbooksCommandTests, ListWorkbooksCommandTests, ShowWorkbooksCommandTests, UpdateWorkbooksCommandTests, and Public API command tests. - Ensured consistency in error handling by using HttpStatusCode for all relevant assertions. * Refactor HTTP status code handling to use HttpStatusCode enum for improved clarity and consistency across commands and tests. * Refactor error handling in GetCosmosClientAsync to use HttpStatusCode enum for improved clarity and maintainability. * Refactor status code assertion in ExecuteAsync_EnrichedClusterFields_SerializeCorrectly test to use HttpStatusCode enum for improved clarity and consistency.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Add validation to the transport option so the server doesn't hang when an invalid option is provided.
GitHub issue number?
Fixes #311
Pre-merge Checklist
servers/Azure.Mcp.Server/CHANGELOG.mdand/orservers/Fabric.Mcp.Server/CHANGELOG.mdfor product changes (features, bug fixes, UI/UX, updated dependencies)servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentation/docs/azmcp-commands.mdand/or/docs/fabric-commands.mdToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test prompts/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline