Stop using ServerStartOptions in other commands - #3304
Stop using ServerStartOptions in other commands#3304Alan Zimmer (alzimmermsft) wants to merge 5 commits into
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR removes the pattern of injecting/consuming ServerStartOptions (formerly relied on as “server runtime state”) from commands and services outside ServerStartCommand, and instead threads server runtime context (notably “remote mode”) through CommandContext. This aligns with issue #1404 by preventing a single command’s options type from bleeding into unrelated components.
Changes:
- Introduced
CommandContext.RunningInRemoteModeand populated it from server tool loaders so commands can make transport/runtime decisions withoutServerStartOptions. - Updated OneLake file commands and tests to gate local-only behaviors (e.g.,
--download-file-path) usingcontext.RunningInRemoteMode. - Updated Compute VM/VMSS create flows to avoid
ServerStartOptionsin the service layer by passingrunningInRemoteModeexplicitly and hardening SSH public key resolution accordingly; reorganized ServerStartCommand test coverage (unit vs live).
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Fabric.Mcp.Tools.OneLake/tests/Fabric.Mcp.Tools.OneLake.Tests/Commands/File/FileReadCommandTests.cs | Drops ServerStartOptions DI usage; adds RunningInRemoteMode coverage for download-path rejection. |
| tools/Fabric.Mcp.Tools.OneLake/tests/Fabric.Mcp.Tools.OneLake.Tests/Commands/File/BlobGetCommandTests.cs | Uses CommandContext.RunningInRemoteMode instead of ServerStartOptions.Transport for HTTP-mode validation. |
| tools/Fabric.Mcp.Tools.OneLake/src/Commands/File/FileReadCommand.cs | Removes IOptions<ServerStartOptions> dependency; uses context.RunningInRemoteMode to restrict --download-file-path. |
| tools/Fabric.Mcp.Tools.OneLake/src/Commands/File/BlobGetCommand.cs | Same as above for blob get behavior. |
| tools/Azure.Mcp.Tools.Extension/tests/Azure.Mcp.Tools.Extension.Tests/ExtensionSetupTests.cs | Renames test/comment wording from “ServiceStartOptions” to “ServerStartOptions”. |
| tools/Azure.Mcp.Tools.Extension/src/ExtensionSetup.cs | Updates doc/comment wording to “ServerStartOptions”. |
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/Vmss/VmssCreateCommandTests.cs | Updates mocks for the new runningInRemoteMode service parameter. |
| tools/Azure.Mcp.Tools.Compute/tests/Azure.Mcp.Tools.Compute.Tests/Vm/VmCreateCommandTests.cs | Updates mocks for the new runningInRemoteMode service parameter. |
| tools/Azure.Mcp.Tools.Compute/src/Services/IComputeService.cs | Adds runningInRemoteMode parameter to create APIs so services no longer need ServerStartOptions. |
| tools/Azure.Mcp.Tools.Compute/src/Services/ComputeService.cs | Removes ServerStartOptions dependency; gates SSH key file-path resolution on runningInRemoteMode. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vmss/VmssCreateCommand.cs | Passes context.RunningInRemoteMode into the compute service; minor OS-type validation refactor. |
| tools/Azure.Mcp.Tools.Compute/src/Commands/Vm/VmCreateCommand.cs | Passes context.RunningInRemoteMode into the compute service; minor OS-type validation refactor. |
| servers/Template.Mcp.Server/src/Program.cs | Comment fix: “ServiceStartCommand” → “ServerStartCommand”. |
| servers/Fabric.Mcp.Server/src/Program.cs | Comment fix: “ServiceStartCommand” → “ServerStartCommand”. |
| servers/Azure.Mcp.Server/src/Program.cs | Comment fix: “ServiceStartCommand” → “ServerStartCommand”. |
| core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Tests/Client/CommandUnitTestsBase.cs | Makes Context settable for tests that need to flip runtime flags (but currently introduces a compilation issue). |
| core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Core.Tests/Services/Telemetry/TelemetryServiceTests.cs | Renames variables to “server” wording for ServerStartOptions. |
| core/Microsoft.Mcp.Core/src/Services/Azure/Authentication/AzureCloudConfiguration.cs | Renames ctor parameter from serviceStartOptions → serverStartOptions and updates related comments. |
| core/Microsoft.Mcp.Core/src/Models/Command/CommandContext.cs | Adds RunningInRemoteMode init-only property. |
| core/Microsoft.Mcp.Core/src/Commands/CommandFactory.cs | Variable rename: isServiceStartCommand → isServerStartCommand. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Options/PluginTelemetryOptions.cs | Removes outdated comment about inheriting from ServiceStartOptions. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/NamespaceToolLoader.cs | Sets CommandContext.RunningInRemoteMode based on server options (IsHttpMode). |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ToolLoading/CommandFactoryToolLoader.cs | Sets CommandContext.RunningInRemoteMode based on server options (IsHttpMode). |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/ServiceCollectionExtensions.cs | Renames method parameter to serverStartOptions and updates internal references/comments. |
| core/Microsoft.Mcp.Core/src/Areas/Server/Commands/PluginTelemetryCommand.cs | Removes outdated comment about PluginTelemetryOptions inheriting options. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Services/Azure/Authentication/AzureCloudConfigurationTests.cs | Updates test names/comments for “ServerStartOptions” wording and named argument. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/RegistryDiscoveryStrategyHelper.cs | Minor simplification of ServerStartOptions construction. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/ServiceStartCommandTests.cs | Deletes old combined test file (migrated elsewhere). |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/ServerStartCommandTests.cs | Replaces prior live tests with unit-style tests, but currently contains a compilation-breaking typo in the class name. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/ServerStartCommandLiveTests.cs | Adds back the prior live test coverage in a dedicated file. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/Commands/ToolLoading/SingleProxyToolLoaderTests.cs | Minor simplification of ServerStartOptions construction. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/Commands/ToolLoading/ServerToolLoaderTests.cs | Renames local variables for clarity (“server” wording). |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/Commands/ServiceCollectionExtensionsTests.cs | Comment wording update for IOptions<ServerStartOptions>. |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/Commands/ServiceCollectionExtensionsSerializedTests.cs | Renames locals for clarity (“server” wording). |
| core/Azure.Mcp.Core/tests/Azure.Mcp.Core.Tests/Areas/Server/Commands/Discovery/CommandGroupDiscoveryStrategyTests.cs | Test name wording update (“ServerStartOptions”). |
| core/Azure.Mcp.Core/src/Services/Azure/AzureServiceCollectionExtensions.cs | Comment fix: “ServiceStartCommand” → “ServerStartCommand”. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// </summary> | ||
| public Activity? Activity { get; } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
From an object-oriented design perspective, I don't consider "RunningInRemoteMode" a feature of a command. It feels like an odd spot for this... This might be more of a "server environment" setting. I would rather DI another object associated with server configuration.
What does this PR do?
Removes the usage of
ServerStartOptionsin commands other thanServerStartCommand. Usages within classes called byServerStartCommandare fine and should be left alone.Contextual information about the running server is now passed via
CommandContextfor tools other thanServerStartCommandto use for runtime handling.GitHub issue number?
Fixes #1404
Pre-merge Checklist
servers/Azure.Mcp.Server/README.mdand/orservers/Fabric.Mcp.Server/README.mddocumentationREADME.mdchanges running the script./eng/scripts/Process-PackageReadMe.ps1. See Package READMEToolDescriptionEvaluatorand obtained a score of0.4or more and a top 3 ranking for all related test promptsconsolidated-tools.jsonbreaking-changelabelservers/Azure.Mcp.Server/docs/azmcp-commands.md./eng/scripts/Update-AzCommandsMetadata.ps1to update tool metadata inazmcp-commands.md(required for CI)servers/Azure.Mcp.Server/docs/e2eTestPrompts.mdcrypto mining, spam, data exfiltration, etc.)/azp run mcp - pullrequest - liveto run Live Test Pipeline