Skip to content

Migrate Best Practices tools to new tool design #2951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alzimmermsft wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Migrate Best Practices tools to new tool design #2951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd67896
Migrate Best Practices tools to new tool design
alzimmermsft Jun 25, 2026
9eed92d
PR feedback
alzimmermsft Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Azure.Mcp.Tools.AzureBestPractices.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
<EmbeddedResource Include="**\Resources\*.json" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Mcp.Core\src\Azure.Mcp.Core.csproj" />
<ProjectReference Include="$(RepoRoot)core\Azure.Mcp.Core\src\Azure.Mcp.Core.csproj" />
</ItemGroup>
<ItemGroup />
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.DependencyInjection" />
<PackageReference Include="ModelContextProtocol" />
<PackageReference Include="System.CommandLine" />
</ItemGroup>
</Project>
6 changes: 2 additions & 4 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/AIAppBestPracticesCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ namespace Azure.Mcp.Tools.AzureBestPractices.Commands;
ReadOnly = true,
Secret = false,
LocalRequired = false)]
public sealed class AIAppBestPracticesCommand(ILogger<AIAppBestPracticesCommand> logger) : BaseCommand<EmptyOptions>
public sealed class AIAppBestPracticesCommand(ILogger<AIAppBestPracticesCommand> logger) : BaseCommand<EmptyOptions, List<string>>
{
private readonly ILogger<AIAppBestPracticesCommand> _logger = logger;
private static readonly string s_bestPracticesText = LoadBestPracticesText();
Expand All @@ -51,9 +51,7 @@ private static string LoadEmbeddedText(string fileName)
return EmbeddedResourceHelper.ReadEmbeddedResource(assembly, resourceName);
}

protected override EmptyOptions BindOptions(ParseResult parseResult) => new();

public override Task<CommandResponse> ExecuteAsync(CommandContext context, ParseResult parseResult, CancellationToken cancellationToken)
public override Task<CommandResponse> ExecuteAsync(CommandContext context, EmptyOptions options, CancellationToken cancellationToken)
{
try
{
Expand Down
5 changes: 1 addition & 4 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/AzureBestPracticesJsonContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,4 @@ namespace Azure.Mcp.Tools.AzureBestPractices.Commands;

[JsonSerializable(typeof(List<string>))]
[JsonSourceGenerationOptions(PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase)]
internal partial class AzureBestPracticesJsonContext : JsonSerializerContext
{

}
internal partial class AzureBestPracticesJsonContext : JsonSerializerContext;
78 changes: 28 additions & 50 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Commands/BestPracticesCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Collections.Concurrent;
using System.Net;
using System.Reflection;
using System.Text;
using Azure.Mcp.Tools.AzureBestPractices.Options;
using Microsoft.Extensions.Logging;
using Microsoft.Mcp.Core.Commands;
using Microsoft.Mcp.Core.Extensions;
using Microsoft.Mcp.Core.Helpers;
using Microsoft.Mcp.Core.Models.Command;

Expand All @@ -32,70 +32,48 @@ it belongs to the Azure Best Practices category.
ReadOnly = true,
Secret = false,
LocalRequired = false)]
public sealed class BestPracticesCommand(ILogger<BestPracticesCommand> logger) : BaseCommand<BestPracticesOptions>
public sealed class BestPracticesCommand(ILogger<BestPracticesCommand> logger) : BaseCommand<BestPracticesOptions, List<string>>
{
private readonly ILogger<BestPracticesCommand> _logger = logger;
private static readonly Dictionary<string, string> s_bestPracticesCache = new();
private static readonly ConcurrentDictionary<string, string> s_bestPracticesCache = [];

Comment thread
alzimmermsft marked this conversation as resolved.
protected override void RegisterOptions(Command command)
public override void ValidateOptions(BestPracticesOptions options, ValidationResult validationResult)
{
command.Options.Add(BestPracticesOptionDefinitions.Resource);
command.Options.Add(BestPracticesOptionDefinitions.Action);
command.Validators.Add(commandResult =>
base.ValidateOptions(options, validationResult);

if (string.IsNullOrWhiteSpace(options.Resource) || string.IsNullOrWhiteSpace(options.Action))

@KarishmaGhiya KarishmaGhiya Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Doesn't ValidateOptions already have this check?

@jongio jongio Jun 30, 2026

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.

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.

{
validationResult.Errors.Add("Both resource and action parameters are required.");
}
else
{
commandResult.TryGetValue(BestPracticesOptionDefinitions.Resource, out string? resource);
commandResult.TryGetValue(BestPracticesOptionDefinitions.Action, out string? action);
bool validResource = options.Resource == "general" || options.Resource == "azurefunctions" || options.Resource == "static-web-app" || options.Resource == "coding-agent";
bool validAction = options.Action == "all" || options.Action == "code-generation" || options.Action == "deployment";

if (string.IsNullOrWhiteSpace(resource) || string.IsNullOrWhiteSpace(action))
if (!validResource)
{
commandResult.AddError("Both resource and action parameters are required.");
validationResult.Errors.Add("Invalid resource. Must be 'general', 'azurefunctions', 'static-web-app', or 'coding-agent'.");
}
else
if (!validAction)
{
bool validResource = resource == "general" || resource == "azurefunctions" || resource == "static-web-app" || resource == "coding-agent";
bool validAction = action == "all" || action == "code-generation" || action == "deployment";

if (!validResource)
{
commandResult.AddError("Invalid resource. Must be 'general', 'azurefunctions', 'static-web-app', or 'coding-agent'.");
}
if (!validAction)
{
commandResult.AddError("Invalid action. Must be 'all', 'code-generation' or 'deployment'.");
}
if (resource == "static-web-app" && action != "all")
{
commandResult.AddError("The 'static-web-app' resource only supports 'all' action.");
}
if (resource == "coding-agent" && action != "all")
{
commandResult.AddError("The 'coding-agent' resource only supports 'all' action.");
}
validationResult.Errors.Add("Invalid action. Must be 'all', 'code-generation' or 'deployment'.");
}
});
}

protected override BestPracticesOptions BindOptions(ParseResult parseResult)
{
return new BestPracticesOptions
{
Resource = parseResult.GetValueOrDefault<string>(BestPracticesOptionDefinitions.Resource.Name),
Action = parseResult.GetValueOrDefault<string>(BestPracticesOptionDefinitions.Action.Name)
};
if (options.Resource == "static-web-app" && options.Action != "all")
{
validationResult.Errors.Add("The 'static-web-app' resource only supports 'all' action.");
}
if (options.Resource == "coding-agent" && options.Action != "all")
{
validationResult.Errors.Add("The 'coding-agent' resource only supports 'all' action.");
}
}
}

public override Task<CommandResponse> ExecuteAsync(CommandContext context, ParseResult parseResult, CancellationToken cancellationToken)
public override Task<CommandResponse> ExecuteAsync(CommandContext context, BestPracticesOptions options, CancellationToken cancellationToken)
{
if (!Validate(parseResult.CommandResult, context.Response).IsValid)
{
return Task.FromResult(context.Response);
}

var options = BindOptions(parseResult);

try
{

@jongio jongio Jun 30, 2026

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.

Since you upgraded to ConcurrentDictionary, you can simplify this to use GetOrAdd for atomic check-and-load:

Suggested change
{
private static string GetBestPracticesText(string resourceFileName)
{
ArgumentException.ThrowIfNullOrEmpty(resourceFileName);
return s_bestPracticesCache.GetOrAdd(resourceFileName, LoadBestPracticesText);
}

var resourceFileName = GetResourceFileName(options.Resource!, options.Action!);
var resourceFileName = GetResourceFileName(options.Resource, options.Action);
var bestPractices = GetBestPracticesText(resourceFileName);

context.Response.Status = HttpStatusCode.OK;
Expand Down
4 changes: 0 additions & 4 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/GlobalUsings.cs
View file
Open in desktop

This file was deleted.

26 changes: 0 additions & 26 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Options/BestPracticesOptionDefinitions.cs
View file
Open in desktop

This file was deleted.

9 changes: 7 additions & 2 deletions tools/Azure.Mcp.Tools.AzureBestPractices/src/Options/BestPracticesOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using Microsoft.Mcp.Core.Options;

namespace Azure.Mcp.Tools.AzureBestPractices.Options;

public sealed class BestPracticesOptions
{
public string? Resource { get; set; }
public string? Action { get; set; }
[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'.")]

@jongio jongio Jun 25, 2026

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.

Grammar: resources only supports should be resources only support (plural subject).

Suggested change
[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'.")]

public required string Action { get; set; }
}
4 changes: 2 additions & 2 deletions ...BestPractices/tests/Azure.Mcp.Tools.AzureBestPractices.Tests/BestPracticesCommandTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public async Task ExecuteAsync_MissingResource_ReturnsBadRequest()
// Assert
Assert.NotNull(response);
Assert.Equal(HttpStatusCode.BadRequest, response.Status);
Assert.Contains("Both resource and action parameters are required", response.Message);
Assert.Contains("Missing Required options: --resource", response.Message);
}

[Fact]
Expand All @@ -180,6 +180,6 @@ public async Task ExecuteAsync_MissingAction_ReturnsBadRequest()
// Assert
Assert.NotNull(response);
Assert.Equal(HttpStatusCode.BadRequest, response.Status);
Assert.Contains("Both resource and action parameters are required", response.Message);
Assert.Contains("Missing Required options: --action", response.Message);
}
}
3 changes: 1 addition & 2 deletions ....Tools.AzureTerraformBestPractices/src/Azure.Mcp.Tools.AzureTerraformBestPractices.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,11 @@
<EmbeddedResource Include="**\Resources\*.json" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="..\..\..\core\Azure.Mcp.Core\src\Azure.Mcp.Core.csproj" />
<ProjectReference Include="$(RepoRoot)core\Azure.Mcp.Core\src\Azure.Mcp.Core.csproj" />
</ItemGroup>
<ItemGroup />
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.DependencyInjection" />
<PackageReference Include="ModelContextProtocol" />
<PackageReference Include="System.CommandLine" />
</ItemGroup>
</Project>
8 changes: 2 additions & 6 deletions ...p.Tools.AzureTerraformBestPractices/src/Commands/AzureTerraformBestPracticesGetCommand.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

using System.Net;
using System.Reflection;
using Microsoft.Extensions.Logging;
using Microsoft.Mcp.Core.Commands;
using Microsoft.Mcp.Core.Helpers;
using Microsoft.Mcp.Core.Models.Command;
Expand All @@ -25,9 +24,8 @@ the Azure Best Practices category.
ReadOnly = true,
Secret = false,
LocalRequired = false)]
public sealed class AzureTerraformBestPracticesGetCommand(ILogger<AzureTerraformBestPracticesGetCommand> logger) : BaseCommand<EmptyOptions>
public sealed class AzureTerraformBestPracticesGetCommand() : BaseCommand<EmptyOptions, List<string>>
{
private readonly ILogger<AzureTerraformBestPracticesGetCommand> _logger = logger;
private static readonly string s_bestPracticesText = LoadBestPracticesText();

private static string GetBestPracticesText() => s_bestPracticesText;
Expand All @@ -39,9 +37,7 @@ private static string LoadBestPracticesText()
return EmbeddedResourceHelper.ReadEmbeddedResource(assembly, resourceName);
}

protected override EmptyOptions BindOptions(ParseResult parseResult) => new();

public override Task<CommandResponse> ExecuteAsync(CommandContext context, ParseResult parseResult, CancellationToken cancellationToken)
public override Task<CommandResponse> ExecuteAsync(CommandContext context, EmptyOptions options, CancellationToken cancellationToken)
{
var bestPractices = GetBestPracticesText();
context.Response.Status = HttpStatusCode.OK;
Expand Down
4 changes: 0 additions & 4 deletions tools/Azure.Mcp.Tools.AzureTerraformBestPractices/src/GlobalUsings.cs
View file
Open in desktop

This file was deleted.