Merge task branch for: fix(worker): external MCP optional params are generated as required; errors are opaque

tests/ClaudeDo.Worker.Tests/External/ExternalMcpExceptionFilterTests.cs

@@ -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);
+    }
+}

tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs

@@ -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);
+    }
+}