Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 2 additions & 9 deletions core/Microsoft.Mcp.Core/src/Commands/BaseCommand`2.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,15 +121,8 @@ protected virtual void HandleException(CommandContext context, Exception ex)
if (ex is CommandValidationException cve)
{
response.Status = cve.StatusCode;
// If specific missing options are provided, format a consistent message
if (cve.MissingOptions is { Count: > 0 })
{
response.Message = $"{MissingRequiredOptionsPrefix}{string.Join(", ", cve.MissingOptions)}";
}
else
{
response.Message = cve.Message;
}
response.Message = cve.Message;

// Include the command validation exception message as it should be safe. Requires custom validators to
// exclude any sensitive information from their error messages.
context.Activity?.SetTag(TagName.ExceptionMessage, response.Message);
Expand Down
27 changes: 8 additions & 19 deletions core/Microsoft.Mcp.Core/src/Options/OptionAttribute.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,22 +18,6 @@ namespace Microsoft.Mcp.Core.Options;
[AttributeUsage(AttributeTargets.Property, Inherited = true, AllowMultiple = false)]
public sealed class OptionAttribute : Attribute
{
/// <summary>
/// Initializes a new instance with default values.
/// Use named properties to override: <c>[Option(Name = "x", Description = "y", Hidden = true)]</c>.
/// </summary>
public OptionAttribute() { }

/// <summary>
/// Initializes a new instance with a description.
/// Shorthand for <c>[Option(Description = "...")]</c>.
/// </summary>
/// <param name="description">A description of what the option controls.</param>
public OptionAttribute(string description)
{
Description = description;
}

/// <summary>
/// Override the CLI option name (without the "--" prefix).
/// When null, the name is derived from the property name in kebab-case.
Expand All @@ -48,15 +32,20 @@ public OptionAttribute(string description)
/// <summary>
/// A description of what the option controls. Used in help text and by AI agents.
/// </summary>
public string? Description { get; init; }
public required string Description { get; init; }

/// <summary>
/// A default value for the option when a value is not provided. Must match the property type being attributed.
/// </summary>
public object? DefaultValue { get; init; }

/// <summary>
/// Whether the option is hidden from help output.
/// Whether the option is hidden from help output. Default is false.
/// </summary>
public bool Hidden { get; init; } = false;

/// <summary>
/// Whether the option allows an empty or whitespace-only string as a valid value. Default handling is to reject such values.
/// </summary>
public bool Hidden { get; init; }
public bool AllowEmptyOrWhiteSpaceString { get; init; } = false;
}
3 changes: 1 addition & 2 deletions core/Microsoft.Mcp.Core/src/Options/OptionBinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static class OptionBinder
{
var instance = (TOptions)CreateInstance(typeof(TOptions));
List<string> missingOptions = [];
List<string> errors = [];
List<string> errors = [.. parseResult.Errors.Select(e => e.Message)];
Dictionary<PropertyInfo, object>? parentInstances = null;
var handlers = s_optionTypeHandlers.GetOrAdd(typeof(TOptions), _ => GetOptionTypeHandlers<TOptions>());

Expand Down Expand Up @@ -116,7 +116,6 @@ public static class OptionBinder

throw new CommandValidationException(
string.Join('\n', messages),
HttpStatusCode.BadRequest,

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.

Which status code do we now bubble up to the root JSON object?

missingOptions: missingOptions);
}

Expand Down
24 changes: 24 additions & 0 deletions core/Microsoft.Mcp.Core/src/Options/OptionContainerAttribute.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

namespace Microsoft.Mcp.Core.Options;

/// <summary>
/// Attribute for complex option containers (aka, classes that are used in option definitions that contain options themselves).
/// <para>
/// Properties with this attribute must be complex types that contain other options.
/// </para>
/// </summary>
[AttributeUsage(AttributeTargets.Property, Inherited = true, AllowMultiple = false)]
public sealed class OptionContainerAttribute : Attribute
{
/// <summary>
/// The prefix to use for the options in the container.
/// If null, the property name (in kebab-case) is used as the prefix.
/// <para>
/// For example, if the property is named "Vault" and the prefix is "v", the options in the container will be prefixed with "--v-".
/// If the property is named "Vault" and the prefix is null, the options in the container will be prefixed with "--vault-".
/// </para>
/// </summary>
public string? Prefix { get; init; }
}
89 changes: 51 additions & 38 deletions core/Microsoft.Mcp.Core/src/Options/OptionDescriptor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,31 @@ public class OptionDescriptor
{
public required string Name { get; init; }
public required string Description { get; init; }
public string[] Aliases { get; init; } = [];
public required string[] Aliases { get; init; }
public bool Required { get; init; }
public bool Hidden { get; init; }
public object? DefaultValue { get; init; }
public bool AllowEmptyOrWhiteSpaceString { get; init; }
public required PropertyInfo TargetProperty { get; init; }
public required Type Type { get; init; }
public PropertyInfo? ParentProperty { get; init; }

public static OptionDescriptor[] FromType<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties)] T>() where T : class
{
List<OptionDescriptor> optionDescriptors = [];
NullabilityInfoContext nullabilityContext = new();
CollectDescriptors(typeof(T), prefix: null, optionDescriptors, nullabilityContext);
CollectDescriptors(typeof(T), null, optionDescriptors, new(), true, null);
return [.. optionDescriptors];
}

[UnconditionalSuppressMessage("Trimming", "IL2070:UnrecognizedReflectionPattern",
Justification = "Nested option types are rooted by the application.")]
private static void CollectDescriptors(Type type, string? prefix, List<OptionDescriptor> descriptors, NullabilityInfoContext nullabilityContext, bool parentRequired = true, PropertyInfo? parentProperty = null)
private static void CollectDescriptors(
Type type,
string? prefix,
List<OptionDescriptor> descriptors,
NullabilityInfoContext nullabilityContext,
bool parentRequired,
PropertyInfo? parentProperty)
{
PropertyInfo[] allProperties = type.GetProperties(BindingFlags.Public | BindingFlags.Instance);

Expand All @@ -53,28 +59,27 @@ private static void CollectDescriptors(Type type, string? prefix, List<OptionDes
continue;
}

OptionAttribute? optionAttribute = property.GetCustomAttribute<OptionAttribute>();
// Only include properties with [Option]
if (optionAttribute == null)
var optionAttribute = property.GetCustomAttribute<OptionAttribute>();
var optionContainerAttribute = property.GetCustomAttribute<OptionContainerAttribute>();
// Only include properties with [Option] or [OptionContainer]
if (optionAttribute == null && optionContainerAttribute == null)
{
continue;
}

string name = optionAttribute?.Name ?? OptionNameConvention.ToKebabCase(property.Name);

if (!string.IsNullOrEmpty(prefix))
if (optionAttribute != null && optionContainerAttribute != null)
{
name = $"{prefix}-{name}";
throw new InvalidOperationException("Properties can only be attributed with [Option] or [OptionContainer], not both.");
}

var required = Attribute.IsDefined(property, typeof(RequiredMemberAttribute));
if (IsComplexType(property.PropertyType))
{
// Flatten nested complex types with a prefix.
CollectDescriptors(property.PropertyType, name, descriptors, nullabilityContext, parentRequired: required, parentProperty: property);
}
else
var complex = IsComplexType(property.PropertyType);
if (optionAttribute != null)
{
if (complex)
{
throw new InvalidOperationException("Complex properties cannot use [Option] attribute. Use [OptionContainer] instead.");
}
if (!parentRequired && required)
{
throw new InvalidOperationException(
Expand All @@ -86,24 +91,45 @@ private static void CollectDescriptors(Type type, string? prefix, List<OptionDes

descriptors.Add(new OptionDescriptor
{
Name = name,
Aliases = optionAttribute!.Aliases ?? [],
Description = optionAttribute!.Description!,
Name = GetNameOrPrefix(optionAttribute.Name, prefix, property.Name),
Aliases = optionAttribute.Aliases ?? [],
Description = optionAttribute.Description,
Type = property.PropertyType,
Required = required,
Hidden = optionAttribute?.Hidden ?? false,
DefaultValue = optionAttribute?.DefaultValue,
Hidden = optionAttribute.Hidden,
DefaultValue = optionAttribute.DefaultValue,
AllowEmptyOrWhiteSpaceString = optionAttribute.AllowEmptyOrWhiteSpaceString,
TargetProperty = property,
ParentProperty = parentProperty
});
}

if (optionContainerAttribute != null)
{
if (!complex)
{
throw new InvalidOperationException("Non-complex properties cannot use [OptionContainer] attribute. Use [Option] instead.");
}
// Flatten nested complex types with a prefix.
CollectDescriptors(property.PropertyType, GetNameOrPrefix(optionContainerAttribute.Prefix, prefix, property.Name), descriptors, nullabilityContext, required, property);
}
}
}

private static string GetNameOrPrefix(string? attributeNameOrPrefix, string? prefix, string propertyName)
{
string name = attributeNameOrPrefix ?? OptionNameConvention.ToKebabCase(propertyName);
if (!string.IsNullOrEmpty(prefix))
{
name = $"{prefix}-{name}";
}
return name;
}

private static bool IsComplexType(Type type)
{
// Unwrap Nullable<T>
Type underlying = Nullable.GetUnderlyingType(type) ?? type;
var underlying = Nullable.GetUnderlyingType(type) ?? type;

if (IsScalarType(underlying))
{
Expand All @@ -114,7 +140,7 @@ private static bool IsComplexType(Type type)
// String is an IEnumerable<char>, but we treat it as a scalar type, so it will be handled above.
if (underlying.IsArray || underlying.IsAssignableTo(typeof(System.Collections.IEnumerable)))
{
Type? elementType = GetCollectionElementType(underlying);
var elementType = GetCollectionElementType(underlying);
if (elementType is not null && !IsScalarType(elementType))
{
throw new InvalidOperationException(
Expand All @@ -130,7 +156,7 @@ private static bool IsComplexType(Type type)

private static bool IsScalarType(Type type)
{
Type underlying = Nullable.GetUnderlyingType(type) ?? type;
var underlying = Nullable.GetUnderlyingType(type) ?? type;
return underlying.IsPrimitive ||
underlying.IsEnum ||
underlying == typeof(string) ||
Expand All @@ -156,17 +182,4 @@ private static bool IsScalarType(Type type)
.Select(i => i.GetGenericArguments()[0])
.FirstOrDefault();
}

private static bool IsNullable(PropertyInfo property, NullabilityInfoContext nullabilityContext)
{
// Nullable value types (e.g., TimeSpan?)
if (Nullable.GetUnderlyingType(property.PropertyType) is not null)
{
return true;
}

// Nullable reference types (e.g., string?)
NullabilityInfo nullabilityInfo = nullabilityContext.Create(property);
return nullabilityInfo.WriteState == NullabilityState.Nullable;
}
}
34 changes: 29 additions & 5 deletions core/Microsoft.Mcp.Core/src/Options/OptionTypeHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,13 +125,14 @@ private static (Option, Func<ParseResult, object?>)? CreateOptionAndBinder(
// Enums are handled as a string with additional binding.
if (typeof(string) == type || type.IsEnum)
{
Action<OptionResult>? validateEmptyOrWhiteSpace = descriptor.AllowEmptyOrWhiteSpaceString ? null : ValidateEmptyOrWhiteSpace;
if (isNullable && isMulti)
return CreateOptionAndBinderHelper<string[]?>(descriptor);
return CreateOptionAndBinderHelper<string[]?>(descriptor, validateEmptyOrWhiteSpace);
if (isMulti)
return CreateOptionAndBinderHelper<string[]>(descriptor);
return CreateOptionAndBinderHelper<string[]>(descriptor, validateEmptyOrWhiteSpace);
if (isNullable)
return CreateOptionAndBinderHelper<string?>(descriptor);
return CreateOptionAndBinderHelper<string>(descriptor);
return CreateOptionAndBinderHelper<string?>(descriptor, validateEmptyOrWhiteSpace);
return CreateOptionAndBinderHelper<string>(descriptor, validateEmptyOrWhiteSpace);
}
if (typeof(bool) == type)
{
Expand Down Expand Up @@ -306,13 +307,19 @@ private static (Option, Func<ParseResult, object?>)? CreateOptionAndBinder(
return null;
}

private static (Option, Func<ParseResult, object?>) CreateOptionAndBinderHelper<T>(OptionDescriptor descriptor)
private static (Option, Func<ParseResult, object?>) CreateOptionAndBinderHelper<T>(
OptionDescriptor descriptor,
Action<OptionResult>? emptyOrWhiteSpaceValidator = null)
{
var option = new Option<T>($"--{descriptor.Name}", [.. descriptor.Aliases.Select(a => $"--{a}")]);
if (descriptor.DefaultValue != null)
{
option.DefaultValueFactory = _ => (T)descriptor.DefaultValue;
}
if (emptyOrWhiteSpaceValidator != null)
{
option.Validators.Add(emptyOrWhiteSpaceValidator);
}
return (option, parseResult => parseResult.GetValueOrDefaultWithoutName(option));
}

Expand All @@ -326,4 +333,21 @@ private static ArgumentArity GetArgumentArity(Type type, bool isNullable, bool i
return ArgumentArity.ZeroOrOne;
return typeof(bool) == type ? ArgumentArity.ZeroOrOne : ArgumentArity.ExactlyOne;
}

private static void ValidateEmptyOrWhiteSpace(OptionResult result)
{
// Calling on an Option that isn't required, has a default, or doesn't allow values to be passed skips checking.
// This matches previous behavior.
var option = result.Option;
if (!option.Required || option.HasDefaultValue || option.Arity.MaximumNumberOfValues == 0)
{
return;
}

var hasNonEmptyOrWhiteSpaceValues = result.Tokens is { Count: > 0 } && result.Tokens.Any(t => !string.IsNullOrWhiteSpace(t.Value));
if (!hasNonEmptyOrWhiteSpaceValues)
{
result.AddError("Option was configured to require non-empty, non-whitespace values but only empty or whitespace values were provided.");
}
Comment thread
alzimmermsft marked this conversation as resolved.
Outdated
}
}
Loading
Loading