Migrate Best Practices tools to new tool design#2951
Conversation
alzimmermsft
requested review from
a team,
conniey,
fanyang-mono and
g2vinay
as code owners
There was a problem hiding this comment.
Pull request overview
This PR migrates the Azure Best Practices and Azure Terraform Best Practices tools to the newer command/option design where options are registered/bound via [Option] attributes and commands use BaseCommand<TOptions, TResult>.
Changes:
- Switched best-practices commands to the
BaseCommand<TOptions, TResult>execution model (options bound as POCOs, noParseResultbinding in tool code). - Replaced
System.CommandLineoption-definition scaffolding with[Option]attributes and removed relatedGlobalUsings.cs/ option definition types. - Simplified JSON context declaration and removed direct
System.CommandLinepackage references from the tool projects.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/Azure.Mcp.Tools.AzureTerraformBestPractices/src/GlobalUsings.cs | Removes global System.CommandLine using (no longer needed by tool code). |
| tools/Azure.Mcp.Tools.AzureTerraformBestPractices/src/Commands/AzureTerraformBestPracticesGetCommand.cs | Migrates command to BaseCommand<EmptyOptions, List<string>> signature. |
| tools/Azure.Mcp.Tools.AzureTerraformBestPractices/src/Azure.Mcp.Tools.AzureTerraformBestPractices.csproj | Uses $(RepoRoot) project reference and removes direct System.CommandLine package reference. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Options/BestPracticesOptions.cs | Introduces [Option] attributes for resource/action binding. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Options/BestPracticesOptionDefinitions.cs | Removes legacy option-definition class. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/GlobalUsings.cs | Removes global System.CommandLine using (no longer needed by tool code). |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/BestPracticesCommand.cs | Migrates command to options-based execution and moves validation into ValidateOptions. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/AzureBestPracticesJsonContext.cs | Converts JSON context to file-scoped partial type declaration. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/AIAppBestPracticesCommand.cs | Migrates command to BaseCommand<EmptyOptions, List<string>> signature. |
| tools/Azure.Mcp.Tools.AzureBestPractices/src/Azure.Mcp.Tools.AzureBestPractices.csproj | Uses $(RepoRoot) project reference and removes direct System.CommandLine package reference. |
Comments suppressed due to low confidence (1)
tools/Azure.Mcp.Tools.AzureTerraformBestPractices/src/Commands/AzureTerraformBestPracticesGetCommand.cs:47
ExecuteAsyncdoesn’t catch exceptions / callHandleException, so failures (e.g., embedded resource lookup/read) can bubble out and bypass the standard error response path. Also, eager static initialization of the embedded text can throw during type initialization, potentially failing tool activation.
private static readonly string s_bestPracticesText = LoadBestPracticesText();
private static string GetBestPracticesText() => s_bestPracticesText;
private static string LoadBestPracticesText()
{
Assembly assembly = typeof(AzureTerraformBestPracticesGetCommand).Assembly;
string resourceName = EmbeddedResourceHelper.FindEmbeddedResource(assembly, "terraform-best-practices-for-azure.txt");
return EmbeddedResourceHelper.ReadEmbeddedResource(assembly, resourceName);
}
public override Task<CommandResponse> ExecuteAsync(CommandContext context, EmptyOptions options, CancellationToken cancellationToken)
{
var bestPractices = GetBestPracticesText();
context.Response.Status = HttpStatusCode.OK;
context.Response.Results = ResponseResult.Create([bestPractices], AzureTerraformBestPracticesJsonContext.Default.ListString);
context.Response.Message = string.Empty;
return Task.FromResult(context.Response);
}
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Nit: grammar fix in the option description below.
| [Option(Description = "The Azure resource type for which to get best practices. Options: 'general' (general Azure), 'azurefunctions' (Azure Functions), 'static-web-app' (Azure Static Web Apps), 'coding-agent' (Coding Agent).")] | ||
| public required string Resource { get; set; } | ||
|
|
||
| [Option(Description = "The action type for the best practices. Options: 'all', 'code-generation', 'deployment'. Note: 'static-web-app' and 'coding-agent' resources only supports 'all'.")] |
There was a problem hiding this comment.
Grammar: resources only supports should be resources only support (plural subject).
| [Option(Description = "The action type for the best practices. Options: 'all', 'code-generation', 'deployment'. Note: 'static-web-app' and 'coding-agent' resources only supports 'all'.")] | |
| [Option(Description = "The action type for the best practices. Options: 'all', 'code-generation', 'deployment'. Note: 'static-web-app' and 'coding-agent' resources only support 'all'.")] |
| command.Validators.Add(commandResult => | ||
| base.ValidateOptions(options, validationResult); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(options.Resource) || string.IsNullOrWhiteSpace(options.Action)) |
There was a problem hiding this comment.
Doesn't ValidateOptions already have this check?
There was a problem hiding this comment.
The base ValidateOptions method is a no-op (empty virtual), so it doesn't perform this check. However, the framework's OptionBinder.BindOptions catches missing required options earlier in the pipeline and throws CommandValidationException (producing "Missing Required options: --resource"). That fires before ValidateOptions is called.
So the null case here is indeed redundant for "missing" options. The whitespace case (--resource " ") could still reach this code since the binder considers the option "present" if any string value is provided. Whether that edge case matters in practice is debatable, but it's a reasonable defense-in-depth choice.
jongio
left a comment
jongio
left a comment
There was a problem hiding this comment.
Clean migration overall. One minor suggestion for idiomatic ConcurrentDictionary usage below. The grammar nit from my earlier review on the option description is still pending.
| var options = BindOptions(parseResult); | ||
|
|
||
| try | ||
| { |
There was a problem hiding this comment.
Since you upgraded to ConcurrentDictionary, you can simplify this to use GetOrAdd for atomic check-and-load:
| { | |
| private static string GetBestPracticesText(string resourceFileName) | |
| { | |
| ArgumentException.ThrowIfNullOrEmpty(resourceFileName); | |
| return s_bestPracticesCache.GetOrAdd(resourceFileName, LoadBestPracticesText); | |
| } |
What does this PR do?
Migrates Best Practices tools to new design where Register and Bind options are based on
Optionattributes.GitHub issue number?
[Link to the GitHub issue this PR addresses]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