fix(worker): make external MCP filter params optional, surface tool errors
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.
This commit is contained in:
@@ -0,0 +1,48 @@
|
||||
using ClaudeDo.Worker.External;
|
||||
using ModelContextProtocol;
|
||||
using ModelContextProtocol.Protocol;
|
||||
using ModelContextProtocol.Server;
|
||||
|
||||
namespace ClaudeDo.Worker.Tests.External;
|
||||
|
||||
public sealed class ExternalMcpExceptionFilterTests
|
||||
{
|
||||
[Fact]
|
||||
public async Task Wrap_TranslatesInvalidOperationException_PreservingMessage()
|
||||
{
|
||||
McpRequestHandler<CallToolRequestParams, CallToolResult> next =
|
||||
(_, _) => throw new InvalidOperationException("Task abc123 not found.");
|
||||
var wrapped = ExternalMcpExceptionFilter.Wrap(next);
|
||||
|
||||
var ex = await Assert.ThrowsAsync<McpException>(
|
||||
() => wrapped(null!, CancellationToken.None).AsTask());
|
||||
|
||||
Assert.Equal("Task abc123 not found.", ex.Message);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Wrap_TranslatesArgumentException_PreservingMessage()
|
||||
{
|
||||
McpRequestHandler<CallToolRequestParams, CallToolResult> next =
|
||||
(_, _) => throw new ArgumentException("Unknown model alias 'gpt4'.");
|
||||
var wrapped = ExternalMcpExceptionFilter.Wrap(next);
|
||||
|
||||
var ex = await Assert.ThrowsAsync<McpException>(
|
||||
() => wrapped(null!, CancellationToken.None).AsTask());
|
||||
|
||||
Assert.Equal("Unknown model alias 'gpt4'.", ex.Message);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Wrap_PassesThroughSuccessfulResult()
|
||||
{
|
||||
var expected = new CallToolResult();
|
||||
McpRequestHandler<CallToolRequestParams, CallToolResult> next =
|
||||
(_, _) => ValueTask.FromResult(expected);
|
||||
var wrapped = ExternalMcpExceptionFilter.Wrap(next);
|
||||
|
||||
var result = await wrapped(null!, CancellationToken.None);
|
||||
|
||||
Assert.Same(expected, result);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,74 @@
|
||||
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);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user