mika kuns
d7ebafd556
Nullable filter/patch params across the External/ MCP tool classes (ListTasks, UpdateTask, AddSubtask, ReviewTask, SetMyDay, SetListConfig/SetTaskConfig, CreateList/UpdateList) lacked C# default values, so the generated tool schema marked them required — MCP clients omitting them (the common case) failed. Gave every such parameter a default value. Also registered a call-tool filter (ExternalMcpExceptionFilter) on the external MCP host that translates InvalidOperationException/ArgumentException into McpException, since the SDK's own catch-all discards ex.Message for any other exception type and returns a generic "An error occurred invoking 'X'." string. Added a reflection-based schema test sweeping every [McpServerToolType] class to guard against reintroducing a required-but-nullable parameter.
75 lines
3.2 KiB
C#
75 lines
3.2 KiB
C#
using System.Reflection;
|
|
using ClaudeDo.Worker.External;
|
|
using Microsoft.Extensions.AI;
|
|
using ModelContextProtocol.Server;
|
|
|
|
namespace ClaudeDo.Worker.Tests.External;
|
|
|
|
/// <summary>
|
|
/// MCP clients routinely omit optional arguments. The generated tool schema only marks a
|
|
/// parameter optional when the C# method declares a default value — nullability alone is not
|
|
/// enough (verified against ModelContextProtocol/Microsoft.Extensions.AI 1.2.0 / 10.4.1's
|
|
/// AIJsonUtilities.CreateFunctionJsonSchema, which checks ParameterInfo.IsOptional). This sweeps
|
|
/// every [McpServerToolType] class in the External/ namespace so a future tool can't reintroduce
|
|
/// a nullable-but-required filter parameter.
|
|
/// </summary>
|
|
public sealed class ExternalMcpToolSchemaTests
|
|
{
|
|
private static IEnumerable<MethodInfo> ExternalToolMethods()
|
|
{
|
|
var toolTypes = typeof(ExternalMcpService).Assembly.GetTypes()
|
|
.Where(t => t.Namespace == typeof(ExternalMcpService).Namespace
|
|
&& t.GetCustomAttribute<McpServerToolTypeAttribute>() is not null);
|
|
|
|
foreach (var type in toolTypes)
|
|
foreach (var method in type.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly))
|
|
{
|
|
if (method.GetCustomAttribute<McpServerToolAttribute>() is not null)
|
|
yield return method;
|
|
}
|
|
}
|
|
|
|
[Fact]
|
|
public void NoExternalTool_HasARequiredNullableParameter()
|
|
{
|
|
var nullabilityContext = new NullabilityInfoContext();
|
|
var violations = new List<string>();
|
|
|
|
foreach (var method in ExternalToolMethods())
|
|
{
|
|
var schema = AIJsonUtilities.CreateFunctionJsonSchema(method);
|
|
var required = schema.TryGetProperty("required", out var requiredElement)
|
|
? requiredElement.EnumerateArray().Select(e => e.GetString()).ToHashSet()
|
|
: new HashSet<string?>();
|
|
|
|
foreach (var parameter in method.GetParameters())
|
|
{
|
|
if (parameter.ParameterType == typeof(CancellationToken)) continue;
|
|
if (!required.Contains(parameter.Name)) continue;
|
|
|
|
var isNullableValueType = Nullable.GetUnderlyingType(parameter.ParameterType) is not null;
|
|
var isNullableRefType = !parameter.ParameterType.IsValueType
|
|
&& nullabilityContext.Create(parameter).WriteState == NullabilityState.Nullable;
|
|
|
|
if (isNullableValueType || isNullableRefType)
|
|
{
|
|
violations.Add(
|
|
$"{method.DeclaringType!.Name}.{method.Name}({parameter.Name}) is a nullable " +
|
|
"type but has no default value, so MCP clients omitting it will fail. " +
|
|
"Give it a default value (e.g. '= null').");
|
|
}
|
|
}
|
|
}
|
|
|
|
Assert.True(violations.Count == 0, string.Join(Environment.NewLine, violations));
|
|
}
|
|
|
|
[Fact]
|
|
public void ExternalToolMethods_AreDiscovered()
|
|
{
|
|
// Guards the sweep itself: if this drops to 0, ExternalToolMethods() broke silently
|
|
// (e.g. namespace/attribute mismatch) and the schema test above would pass vacuously.
|
|
Assert.True(ExternalToolMethods().Count() > 20);
|
|
}
|
|
}
|