Skip to content

Update new option binding handling#2915

Open
alzimmermsft wants to merge 8 commits into
microsoft:mainfrom
alzimmermsft:UpdateOptionBindingHandling
Open

Update new option binding handling#2915
alzimmermsft wants to merge 8 commits into
microsoft:mainfrom
alzimmermsft:UpdateOptionBindingHandling

Conversation

@alzimmermsft

@alzimmermsft alzimmermsft commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Updates new option binding handling with the following updates:

  • Add support for Aliases when binding command-line options.
  • Add support for DefaultValue when binding command-line options.
  • Replaced Dictionary lookup for handling with more flexible OptionTypeHandler.CreateFromDescriptor. This better supports string? and arity handling.
  • Cache OptionTypeHandlers for each TOption processed as Options are handled in a way where they aren't mutated once created.
  • Added new extension methods for new binding pattern as we no longer need to deal with side-effect modifications to Option with when changing requiredness that existed in the old binding pattern.

GitHub issue number?

[Link to the GitHub issue this PR addresses]

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Created a changelog entry if the change falls among the following: new feature, bug fix, UI/UX update, breaking change, or updated dependencies. Follow the changelog entry guide
  • For MCP tool changes:
    • One tool per PR: This PR adds or modifies only one MCP tool for faster review cycles
    • Updated servers/Azure.Mcp.Server/README.md and/or servers/Fabric.Mcp.Server/README.md documentation
    • Validate README.md changes running the script ./eng/scripts/Process-PackageReadMe.ps1. See Package README
    • For new or modified tool descriptions, ran ToolDescriptionEvaluator and obtained a score of 0.4 or more and a top 3 ranking for all related test prompts
    • For tools with new names, including new tools or renamed tools, update consolidated-tools.json
    • For renamed tools, follow the Tool Rename Checklist and tag the PR with the breaking-change label
    • For new tools associated with Azure services or publicly available tools/APIs/products, add URL to documentation in the PR description
  • Extra steps for Azure MCP Server tool changes:
    • Updated command list in servers/Azure.Mcp.Server/docs/azmcp-commands.md
    • Ran ./eng/scripts/Update-AzCommandsMetadata.ps1 to update tool metadata in azmcp-commands.md (required for CI)
    • Updated test prompts in servers/Azure.Mcp.Server/docs/e2eTestPrompts.md
    • 👉 For Community (non-Microsoft team member) PRs:
      • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
      • Manual tests run: added comment /azp run mcp - pullrequest - live to run Live Test Pipeline

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Not ready to approve

The new OptionTypeHandler has critical correctness issues (enum multi-value binding type mismatch and unsafe IEnumerable element-type extraction) and alias support lacks regression tests.

Pull request overview

This PR refactors the core option-binding pipeline used by commands to improve option creation/binding flexibility (notably adding alias support), moving from a type-handler dictionary to descriptor-driven OptionTypeHandler creation with caching.

Changes:

  • Introduces OptionTypeHandler to create System.CommandLine.Option instances from OptionDescriptor (including support for Aliases) and bind values via option-instance lookup.
  • Updates option descriptor and binder behavior to better account for nullability and multi-value arity, and removes the old enum validator helper in favor of in-handler validation.
  • Adjusts related extensions/docs and adopts primary constructor usage in one command base class.
File summaries
File Description
core/Microsoft.Mcp.Core/tests/Microsoft.Mcp.Core.Tests/Options/OptionBinderTests.cs Updates array arity expectation in unit tests (nullable array now treated as ZeroOrMore).
core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs Adds the new descriptor-driven option creation/binding implementation (including aliases, nullability/multi-value handling, enum validation).
core/Microsoft.Mcp.Core/src/Options/OptionNameConvention.cs Moves the type into the Options namespace and minor formatting.
core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs Makes Description required, adds Aliases, and updates descriptor creation from [Option].
core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs Switches to cached OptionTypeHandler[] per options type for registration and binding.
core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs Adds Aliases to the attribute contract.
core/Microsoft.Mcp.Core/src/Options/EnumOptionValidator.cs Removes the prior enum option validator helper (logic moved into OptionTypeHandler).
core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs Adds an internal option-instance-based getter helper for the new binding approach.
core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs Adds a matching internal option-instance-based getter helper for the new binding approach.
core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs Adds XML doc comments for option validation hook.
core/Azure.Mcp.Core/src/Commands/Subscription/SubscriptionCommand`2.cs Converts to a primary constructor and simplifies field initialization.

Copilot's findings

  • Files reviewed: 11/11 changed files
  • Comments generated: 11

Note

Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.

Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs Outdated
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs Outdated
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs
Comment thread core/Microsoft.Mcp.Core/src/Extensions/ParseResultExtensions.cs Outdated
Comment thread core/Microsoft.Mcp.Core/src/Extensions/CommandResultExtensions.cs Outdated
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs
Comment thread core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Untriaged

Development

Successfully merging this pull request may close these issues.

2 participants