From d7ebafd5562f6d50c735291a32deef9099ce9631 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 23 Jul 2026 18:21:46 +0200 Subject: [PATCH] fix(worker): make external MCP filter params optional, surface tool errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- src/ClaudeDo.Worker/CLAUDE.md | 2 +- .../External/ConfigMcpTools.cs | 6 +- .../External/ExternalMcpExceptionFilter.cs | 32 ++++++++ .../External/ExternalMcpService.cs | 26 +++---- src/ClaudeDo.Worker/External/ListMcpTools.cs | 5 +- src/ClaudeDo.Worker/Program.cs | 1 + .../ExternalMcpExceptionFilterTests.cs | 48 ++++++++++++ .../External/ExternalMcpToolSchemaTests.cs | 74 +++++++++++++++++++ 8 files changed, 176 insertions(+), 18 deletions(-) create mode 100644 src/ClaudeDo.Worker/External/ExternalMcpExceptionFilter.cs create mode 100644 tests/ClaudeDo.Worker.Tests/External/ExternalMcpExceptionFilterTests.cs create mode 100644 tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 3d38a61a..ec50924e 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -32,7 +32,7 @@ Interfaces (e.g. `IQueueWaker`, `IPrimeClock`, `ITaskStateService`) live in an ` - **IQueueWaker / IQueuePicker / QueueService** — waker is a singleton `SemaphoreSlim`; picker performs the atomic `Queued → Running` claim filtered by `BlockedByTaskId IS NULL` and schedule; QueueService is a thin `BackgroundService` that loops on the waker and dispatches via `TaskRunner`. - **OverrideSlotService** — owns `RunNow` / `ContinueTask`; goes through `TaskStateService.StartRunningAsync` (caller-driven, serialized by slot lock). - **StaleTaskRecovery** — startup-only service; calls `TaskStateService.RecoverStaleRunningAsync` to flip orphaned `Running` rows to `Failed`. -- **External/*** — always-on MCP tools for general Claude sessions, scoped to *starting* and *observing* sessions (no worktree/merge, multi-turn, planning, or app-settings writes). Auth via optional `X-ClaudeDo-Key` header. Registered explicitly in `Program.cs`'s external app via `.WithTools()`. Organized by concern: +- **External/*** — always-on MCP tools for general Claude sessions, scoped to *starting* and *observing* sessions (no worktree/merge, multi-turn, planning, or app-settings writes). Auth via optional `X-ClaudeDo-Key` header. Registered explicitly in `Program.cs`'s external app via `.WithTools()`. Every optional/filter parameter across these tools must carry a C# default value (e.g. `string? status = null`) — the MCP schema only marks a parameter optional when it has one; nullability alone doesn't do it (`ExternalMcpToolSchemaTests` guards this by reflection). `ExternalMcpExceptionFilter.Wrap` is registered as a call-tool filter so `InvalidOperationException`/`ArgumentException` messages survive as `McpException` — otherwise the SDK's own catch-all replaces any non-`McpException` with a generic "An error occurred invoking 'X'." Organized by concern: - `ExternalMcpService` — task CRUD + execution: `ListTaskLists`, `ListTasks`, `GetTask`, `AddTask`, `AddSubtask`, `UpdateTask`, `UpdateTaskStatus` (`Idle` / `Queued`), `GetTaskStatusValues`, `ReviewTask` (`approve` / `reject_rerun` / `reject_park` / `cancel` for a WaitingForReview task), `RunTaskNow`, `ContinueTask`, `CancelTask`, `DeleteTask`; worktree/git: `GetTaskWorktree`, `GetTaskDiff`, `MergeTask`, `ListWorktrees`, `CleanupTaskWorktree` - `BatchMcpTools` — best-effort batch variants that loop the `ExternalMcpService` single-entity methods (sequential — the scoped DbContext is not thread-safe; merge/review stay single-task): `BatchGetTasks`, `BatchAddTasks`, `BatchUpdateTaskStatus`, `BatchCancelTasks`, `BatchDeleteTasks`, `BatchSetMyDay`, `BatchCleanupTaskWorktrees`. Every tool returns a per-item result array ({ id/index, ok, error?, … }) — a failing item never aborts the rest — and rejects batches over 100 items. - `ListMcpTools` — `CreateList`, `UpdateList`, `DeleteList` diff --git a/src/ClaudeDo.Worker/External/ConfigMcpTools.cs b/src/ClaudeDo.Worker/External/ConfigMcpTools.cs index 12af5267..b13cc518 100644 --- a/src/ClaudeDo.Worker/External/ConfigMcpTools.cs +++ b/src/ClaudeDo.Worker/External/ConfigMcpTools.cs @@ -31,7 +31,8 @@ public sealed class ConfigMcpTools [McpServerTool, Description("Set a list's default model/system prompt/agent path/max turns. Passing all four as null clears the list config.")] public async Task SetListConfig( - string listId, string? model, string? systemPrompt, string? agentPath, int? maxTurns, CancellationToken cancellationToken) + string listId, string? model = null, string? systemPrompt = null, string? agentPath = null, + int? maxTurns = null, CancellationToken cancellationToken = default) { _ = await _lists.GetByIdAsync(listId, cancellationToken) ?? throw new InvalidOperationException($"List {listId} not found."); @@ -53,7 +54,8 @@ public sealed class ConfigMcpTools [McpServerTool, Description("Set per-task config overrides (model/system prompt/agent path/max turns). Pass null for any field to clear that override.")] public async Task SetTaskConfig( - string taskId, string? model, string? systemPrompt, string? agentPath, int? maxTurns, CancellationToken cancellationToken) + string taskId, string? model = null, string? systemPrompt = null, string? agentPath = null, + int? maxTurns = null, CancellationToken cancellationToken = default) { _ = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); diff --git a/src/ClaudeDo.Worker/External/ExternalMcpExceptionFilter.cs b/src/ClaudeDo.Worker/External/ExternalMcpExceptionFilter.cs new file mode 100644 index 00000000..112653dc --- /dev/null +++ b/src/ClaudeDo.Worker/External/ExternalMcpExceptionFilter.cs @@ -0,0 +1,32 @@ +using ModelContextProtocol; +using ModelContextProtocol.Protocol; +using ModelContextProtocol.Server; + +namespace ClaudeDo.Worker.External; + +/// +/// The MCP SDK's own call-tool catch-all only preserves ex.Message for — +/// any other exception type is replaced with a generic "An error occurred invoking 'X'." with no detail. +/// This filter translates the expected validation exceptions thrown across the External/ tool classes +/// (task/list not found, bad status, unknown model, etc.) into McpException so callers see why a call failed. +/// +public static class ExternalMcpExceptionFilter +{ + public static McpRequestHandler Wrap( + McpRequestHandler next) => + async (request, cancellationToken) => + { + try + { + return await next(request, cancellationToken); + } + catch (InvalidOperationException ex) + { + throw new McpException(ex.Message, ex); + } + catch (ArgumentException ex) + { + throw new McpException(ex.Message, ex); + } + }; +} diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index ef1a59b2..15d18cc8 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -107,9 +107,9 @@ public sealed class ExternalMcpService "Valid status values: Idle, Queued, Running, WaitingForReview, WaitingForChildren, Done, Failed, Cancelled.")] public async Task> ListTasks( string listId, - string? createdBy, - string? status, - CancellationToken cancellationToken) + string? createdBy = null, + string? status = null, + CancellationToken cancellationToken = default) { TaskStatus? statusFilter = null; if (!string.IsNullOrWhiteSpace(status)) @@ -193,10 +193,10 @@ public sealed class ExternalMcpService [McpServerTool, Description("Update an existing task's title, description, and/or commit type. Pass null to leave a field unchanged. Refuses if the task is currently Running.")] public async Task UpdateTask( string taskId, - string? title, - string? description, - string? commitType, - CancellationToken cancellationToken) + string? title = null, + string? description = null, + string? commitType = null, + CancellationToken cancellationToken = default) { var task = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); @@ -219,8 +219,8 @@ public sealed class ExternalMcpService public async Task AddSubtask( string taskId, string title, - int? orderNum, - CancellationToken cancellationToken) + int? orderNum = null, + CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(title)) throw new InvalidOperationException("title is required."); @@ -300,8 +300,8 @@ public sealed class ExternalMcpService public async Task ReviewTask( string taskId, string decision, - string? feedback, - CancellationToken cancellationToken) + string? feedback = null, + CancellationToken cancellationToken = default) { _ = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); @@ -619,8 +619,8 @@ public sealed class ExternalMcpService public async Task SetMyDay( string taskId, bool isMyDay, - int? sortOrder, - CancellationToken cancellationToken) + int? sortOrder = null, + CancellationToken cancellationToken = default) { await using var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken); diff --git a/src/ClaudeDo.Worker/External/ListMcpTools.cs b/src/ClaudeDo.Worker/External/ListMcpTools.cs index c4e8f467..facb9445 100644 --- a/src/ClaudeDo.Worker/External/ListMcpTools.cs +++ b/src/ClaudeDo.Worker/External/ListMcpTools.cs @@ -22,7 +22,7 @@ public sealed class ListMcpTools [McpServerTool, Description("Create a new task list. workingDir sets the git repo tasks run against; commitType defaults to 'chore'.")] public async Task CreateList( - string name, string? workingDir, string? commitType, CancellationToken cancellationToken) + string name, string? workingDir = null, string? commitType = null, CancellationToken cancellationToken = default) { if (string.IsNullOrWhiteSpace(name)) throw new InvalidOperationException("name is required."); @@ -42,7 +42,8 @@ public sealed class ListMcpTools [McpServerTool, Description("Rename a list and/or change its working dir and default commit type. Pass null to leave a field unchanged.")] public async Task UpdateList( - string listId, string? name, string? workingDir, string? commitType, CancellationToken cancellationToken) + string listId, string? name = null, string? workingDir = null, string? commitType = null, + CancellationToken cancellationToken = default) { var entity = await _lists.GetByIdAsync(listId, cancellationToken) ?? throw new InvalidOperationException($"List {listId} not found."); diff --git a/src/ClaudeDo.Worker/Program.cs b/src/ClaudeDo.Worker/Program.cs index 2cb01428..1fc0758e 100644 --- a/src/ClaudeDo.Worker/Program.cs +++ b/src/ClaudeDo.Worker/Program.cs @@ -282,6 +282,7 @@ if (cfg.ExternalMcpPort > 0) externalBuilder.Services.AddScoped(); externalBuilder.Services.AddMcpServer() .WithHttpTransport() + .WithRequestFilters(f => f.AddCallToolFilter(ExternalMcpExceptionFilter.Wrap)) .WithTools() .WithTools() .WithTools() diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpExceptionFilterTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpExceptionFilterTests.cs new file mode 100644 index 00000000..12c68abb --- /dev/null +++ b/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 next = + (_, _) => throw new InvalidOperationException("Task abc123 not found."); + var wrapped = ExternalMcpExceptionFilter.Wrap(next); + + var ex = await Assert.ThrowsAsync( + () => wrapped(null!, CancellationToken.None).AsTask()); + + Assert.Equal("Task abc123 not found.", ex.Message); + } + + [Fact] + public async Task Wrap_TranslatesArgumentException_PreservingMessage() + { + McpRequestHandler next = + (_, _) => throw new ArgumentException("Unknown model alias 'gpt4'."); + var wrapped = ExternalMcpExceptionFilter.Wrap(next); + + var ex = await Assert.ThrowsAsync( + () => wrapped(null!, CancellationToken.None).AsTask()); + + Assert.Equal("Unknown model alias 'gpt4'.", ex.Message); + } + + [Fact] + public async Task Wrap_PassesThroughSuccessfulResult() + { + var expected = new CallToolResult(); + McpRequestHandler next = + (_, _) => ValueTask.FromResult(expected); + var wrapped = ExternalMcpExceptionFilter.Wrap(next); + + var result = await wrapped(null!, CancellationToken.None); + + Assert.Same(expected, result); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpToolSchemaTests.cs new file mode 100644 index 00000000..8be47df3 --- /dev/null +++ b/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; + +/// +/// 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. +/// +public sealed class ExternalMcpToolSchemaTests +{ + private static IEnumerable ExternalToolMethods() + { + var toolTypes = typeof(ExternalMcpService).Assembly.GetTypes() + .Where(t => t.Namespace == typeof(ExternalMcpService).Namespace + && t.GetCustomAttribute() is not null); + + foreach (var type in toolTypes) + foreach (var method in type.GetMethods(BindingFlags.Public | BindingFlags.Instance | BindingFlags.DeclaredOnly)) + { + if (method.GetCustomAttribute() is not null) + yield return method; + } + } + + [Fact] + public void NoExternalTool_HasARequiredNullableParameter() + { + var nullabilityContext = new NullabilityInfoContext(); + var violations = new List(); + + 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(); + + 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); + } +}