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:
@@ -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`.
|
- **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).
|
- **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`.
|
- **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<T>()`. 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<T>()`. 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`
|
- `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.
|
- `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`
|
- `ListMcpTools` — `CreateList`, `UpdateList`, `DeleteList`
|
||||||
|
|||||||
+4
-2
@@ -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.")]
|
[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(
|
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)
|
_ = await _lists.GetByIdAsync(listId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"List {listId} not found.");
|
?? 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.")]
|
[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(
|
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)
|
_ = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
||||||
|
|||||||
@@ -0,0 +1,32 @@
|
|||||||
|
using ModelContextProtocol;
|
||||||
|
using ModelContextProtocol.Protocol;
|
||||||
|
using ModelContextProtocol.Server;
|
||||||
|
|
||||||
|
namespace ClaudeDo.Worker.External;
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// The MCP SDK's own call-tool catch-all only preserves ex.Message for <see cref="McpException"/> —
|
||||||
|
/// 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.
|
||||||
|
/// </summary>
|
||||||
|
public static class ExternalMcpExceptionFilter
|
||||||
|
{
|
||||||
|
public static McpRequestHandler<CallToolRequestParams, CallToolResult> Wrap(
|
||||||
|
McpRequestHandler<CallToolRequestParams, CallToolResult> 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);
|
||||||
|
}
|
||||||
|
};
|
||||||
|
}
|
||||||
+13
-13
@@ -107,9 +107,9 @@ public sealed class ExternalMcpService
|
|||||||
"Valid status values: Idle, Queued, Running, WaitingForReview, WaitingForChildren, Done, Failed, Cancelled.")]
|
"Valid status values: Idle, Queued, Running, WaitingForReview, WaitingForChildren, Done, Failed, Cancelled.")]
|
||||||
public async Task<IReadOnlyList<TaskDto>> ListTasks(
|
public async Task<IReadOnlyList<TaskDto>> ListTasks(
|
||||||
string listId,
|
string listId,
|
||||||
string? createdBy,
|
string? createdBy = null,
|
||||||
string? status,
|
string? status = null,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
TaskStatus? statusFilter = null;
|
TaskStatus? statusFilter = null;
|
||||||
if (!string.IsNullOrWhiteSpace(status))
|
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.")]
|
[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<TaskDto> UpdateTask(
|
public async Task<TaskDto> UpdateTask(
|
||||||
string taskId,
|
string taskId,
|
||||||
string? title,
|
string? title = null,
|
||||||
string? description,
|
string? description = null,
|
||||||
string? commitType,
|
string? commitType = null,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
var task = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
var task = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
||||||
@@ -219,8 +219,8 @@ public sealed class ExternalMcpService
|
|||||||
public async Task<TaskDto> AddSubtask(
|
public async Task<TaskDto> AddSubtask(
|
||||||
string taskId,
|
string taskId,
|
||||||
string title,
|
string title,
|
||||||
int? orderNum,
|
int? orderNum = null,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
if (string.IsNullOrWhiteSpace(title))
|
if (string.IsNullOrWhiteSpace(title))
|
||||||
throw new InvalidOperationException("title is required.");
|
throw new InvalidOperationException("title is required.");
|
||||||
@@ -300,8 +300,8 @@ public sealed class ExternalMcpService
|
|||||||
public async Task<TaskDto> ReviewTask(
|
public async Task<TaskDto> ReviewTask(
|
||||||
string taskId,
|
string taskId,
|
||||||
string decision,
|
string decision,
|
||||||
string? feedback,
|
string? feedback = null,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
_ = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
_ = await _tasks.GetByIdAsync(taskId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
||||||
@@ -619,8 +619,8 @@ public sealed class ExternalMcpService
|
|||||||
public async Task<TaskDto> SetMyDay(
|
public async Task<TaskDto> SetMyDay(
|
||||||
string taskId,
|
string taskId,
|
||||||
bool isMyDay,
|
bool isMyDay,
|
||||||
int? sortOrder,
|
int? sortOrder = null,
|
||||||
CancellationToken cancellationToken)
|
CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
await using var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken);
|
await using var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken);
|
||||||
|
|
||||||
|
|||||||
+3
-2
@@ -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'.")]
|
[McpServerTool, Description("Create a new task list. workingDir sets the git repo tasks run against; commitType defaults to 'chore'.")]
|
||||||
public async Task<ListSummaryDto> CreateList(
|
public async Task<ListSummaryDto> CreateList(
|
||||||
string name, string? workingDir, string? commitType, CancellationToken cancellationToken)
|
string name, string? workingDir = null, string? commitType = null, CancellationToken cancellationToken = default)
|
||||||
{
|
{
|
||||||
if (string.IsNullOrWhiteSpace(name))
|
if (string.IsNullOrWhiteSpace(name))
|
||||||
throw new InvalidOperationException("name is required.");
|
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.")]
|
[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<ListSummaryDto> UpdateList(
|
public async Task<ListSummaryDto> 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)
|
var entity = await _lists.GetByIdAsync(listId, cancellationToken)
|
||||||
?? throw new InvalidOperationException($"List {listId} not found.");
|
?? throw new InvalidOperationException($"List {listId} not found.");
|
||||||
|
|||||||
@@ -282,6 +282,7 @@ if (cfg.ExternalMcpPort > 0)
|
|||||||
externalBuilder.Services.AddScoped<AttachmentMcpTools>();
|
externalBuilder.Services.AddScoped<AttachmentMcpTools>();
|
||||||
externalBuilder.Services.AddMcpServer()
|
externalBuilder.Services.AddMcpServer()
|
||||||
.WithHttpTransport()
|
.WithHttpTransport()
|
||||||
|
.WithRequestFilters(f => f.AddCallToolFilter(ExternalMcpExceptionFilter.Wrap))
|
||||||
.WithTools<ExternalMcpService>()
|
.WithTools<ExternalMcpService>()
|
||||||
.WithTools<BatchMcpTools>()
|
.WithTools<BatchMcpTools>()
|
||||||
.WithTools<ListMcpTools>()
|
.WithTools<ListMcpTools>()
|
||||||
|
|||||||
@@ -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