Introduce the new options pattern by hallipr · Pull Request #2637 · microsoft/mcp

hallipr · 2026-05-12T20:27:09Z

What does this PR do?

This pull request introduces significant refactoring to the Azure MCP command and options infrastructure, primarily to simplify option binding and validation, and to improve consistency across commands. The changes also include updates to retry policy configuration, test adjustments, and new project references.

GitHub issue number?

[Link to the GitHub issue this PR addresses]

Pre-merge Checklist

alzimmermsft · 2026-05-12T23:09:39Z

-public class SubscriptionListOptions : GlobalOptions;
+public class SubscriptionListOptions : SubscriptionOptions
+{
+    public string? ResourceGroup { get; set; }


Doesn't make sense for this to continue having this option

true, but I didn't want to affect the user-facing api in this PR. All options are preserved with their existing "requiredness", name and description

alzimmermsft · 2026-05-13T00:43:06Z

+
+    public AuthMethod? AuthMethod { get; set; }
+
+    [Option(Name = "retry")]


What happens to the Name here given that this is annotated on a model which has another set of values?

The name becomes a prefix to the properties of the RetrtyPolicyOptions class.

And the parent object in the inputSchema would be retry: {}? I don't think that would be clear to the LLM. Maybe we can have Name override the property name and a different annotation for the prefix on child properties. Also, would the current prefix apply only to direct children or nested ones too?

alzimmermsft · 2026-05-13T01:35:33Z

 using NSubstitute;
 using Xunit;

+using Microsoft.Mcp.Core.Areas.Server.Models;


Seems to be in an odd spot

alzimmermsft · 2026-05-13T01:39:34Z

+        if (options.Tool != null && options.Tool.Length > 0)
+        {
+            options.Mode = ModeTypes.All;
+        }


Well, this is really interesting behavior... doesn't seem right for another option to implicitly override another like this. Not an issue in this PR but something that probably should be discussed as a follow up.

alzimmermsft · 2026-05-13T01:43:26Z

        if (hasNamespace && hasTool)
        {
-            commandResult.AddError("The --namespace and --tool options cannot be used together. Please specify either --namespace to filter by service namespace or --tool to filter by specific tool names, but not both.");
+            validationResult.AddError("The --namespace and --tool options cannot be used together. Please specify either --namespace to filter by service namespace or --tool to filter by specific tool names, but not both.");


Comment for tracking, not an issue in this PR.

Using --namespace, etc. ties the error message to a CLI-like mode. MCP parameters don't use -- prefixes and would just be namespace here.

Can we write an issue to clean this up so that our error messages return based on the mode used (CLI or MCP right now). In CLI we use --namespace ... options and in MCP we use namespace ... parameters.

alzimmermsft · 2026-05-13T01:45:29Z

    /// </summary>
-    [JsonPropertyName("mode")]
-    public string? Mode { get; set; } = ModeTypes.NamespaceProxy;
+    public string? Mode { get; set; }


Another thing noticed due to changes here but isn't due to the PR.

We should make an enum for Mode instead of is being a string

alzimmermsft · 2026-05-13T16:11:40Z


                return (int)response.Status;
            }
+            catch (CommandValidationException ex)


When does this get thrown here? It seems that BaseCommand should handle catching CommandValidationException and using HandleException to process it by having BindOptions and ValidateOptions in the same block.

alzimmermsft · 2026-05-13T16:20:55Z


    public List<string> Errors { get; } = [];
+
+    public void AddError(string error) => Errors.Add(error);


If we're adding this, should we make Errors a private field instead of a property?

alzimmermsft · 2026-05-13T20:37:07Z

+    private static readonly DiagnosticDescriptor Rule = new(
+        DiagnosticId,
+        title: "Optional group contains required member",
+        messageFormat: "Nullable complex property '{0}' contains non-nullable member '{1}'. All properties within a nullable complex type must be nullable.",


Does this need to handle chains more deeply nested than one level? For example, Foo.Bar.Baz where Foo is nullable, Bar is nullable, but Baz is non-null.

Maybe this checks recursively?

alzimmermsft · 2026-05-13T20:41:00Z

+        while (current is not null)
+        {
+            if (current.IsGenericType &&
+                current.Name == "BaseCommand" &&


Can be a follow-up but it also seems that we should have a rule where a command never implements IBaseCommand directly, as the current state of this PR that wouldn't function properly.

Or the design in this PR needs to be changed so that BaseCommand doesn't contain common implementation needed by all command.

alzimmermsft · 2026-05-13T20:50:28Z

+    public string Type { get; set; } = "string";
+
+    [JsonPropertyName("required")]
+    [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)]


Should we be ignoring this in JSON serialization when the value is false?

alzimmermsft · 2026-05-13T20:51:28Z

+    /// Resolves the subscription from the parse result, falling back to Azure CLI profile
+    /// or AZURE_SUBSCRIPTION_ID environment variable.


This really should be the docstring for the default implementation, not the interface. The interface should allow the implementation to do whatever it wants.

alzimmermsft · 2026-05-13T20:52:18Z

+    /// <summary>
+    /// A description of what the option controls. Used in help text and by AI agents.
+    /// </summary>
+    public string? Description { get; init; }


I thought the plan was for Description to be non-null

alzimmermsft · 2026-05-13T20:59:09Z

+        PropertyInfo[] allProperties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);
+
+        // When a derived class hides a base property with 'new', GetProperties returns both.
+        // Keep only the most-derived version (the one whose DeclaringType is closest to 'type').


Is this a behavior handled by how type.GetProperties works where it checks super types first for their properties? If not, the check below seems wrong as it just checks IsAssignableFrom without checking depth.

github-project-automation Bot added this to Azure MCP Server May 12, 2026

github-project-automation Bot moved this to Untriaged in Azure MCP Server May 12, 2026

github-actions Bot added the tools-AppConfig AppConfig label May 12, 2026

hallipr force-pushed the users/pahallis/options branch from 1da22c4 to 1a5225c Compare May 12, 2026 20:28

alzimmermsft reviewed May 12, 2026

View reviewed changes

alzimmermsft reviewed May 13, 2026

View reviewed changes

hallipr force-pushed the users/pahallis/options branch 2 times, most recently from 2b56c66 to 534b462 Compare May 13, 2026 01:05

alzimmermsft reviewed May 13, 2026

View reviewed changes

hallipr force-pushed the users/pahallis/options branch from 534b462 to d74dad9 Compare May 13, 2026 04:44

alzimmermsft reviewed May 13, 2026

View reviewed changes

Introduce new options pattern

b64f194

hallipr force-pushed the users/pahallis/options branch from 7b6a3f0 to b64f194 Compare May 14, 2026 07:20


		public AuthMethod? AuthMethod { get; set; }

		[Option(Name = "retry")]


		public List<string> Errors { get; } = [];

		public void AddError(string error) => Errors.Add(error);

		/// Resolves the subscription from the parse result, falling back to Azure CLI profile
		/// or AZURE_SUBSCRIPTION_ID environment variable.

Conversation

hallipr commented May 12, 2026

What does this PR do?

GitHub issue number?

Pre-merge Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alzimmermsft May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alzimmermsft May 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alzimmermsft May 13, 2026 •

edited

Loading

alzimmermsft May 13, 2026 •

edited

Loading