feat(mcp): allow Done via update_task_status with worktree guard
External tasks finished outside a ClaudeDo run had no way to close out their tracking task; update_task_status now permits Done alongside Idle/Queued/Cancelled, refusing it when the task has an active worktree so review_task stays the only path that merges.
This commit is contained in:
@@ -33,7 +33,7 @@ Interfaces (e.g. `IQueueWaker`, `IPrimeClock`, `ITaskStateService`) live in an `
|
|||||||
- **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>()`. 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:
|
- **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` / `Cancelled` — `Cancelled` goes through `TaskStateService.CancelAsync(..., allowFromIdle: true)`, the only caller that opts into cancelling from `Idle`; `PlanningChainCoordinator` relies on `Idle` staying a no-op there by default, since a child parked back to `Idle` mid-chain is a manual opt-out signal), `GetTaskStatusValues`, `ReviewTask` (`approve` / `reject_rerun` / `reject_park` / `cancel` for a WaitingForReview task; approve is review+merge exactly like the hub's `ApproveReview` — unit merge for parents, worktree merge into optional `targetBranch` for childless tasks, conflicts reported in the `ReviewTaskResult`), `RunTaskNow`, `ContinueTask`, `CancelTask`, `DeleteTask`; worktree/git: `GetTaskWorktree`, `GetTaskDiff`, `MergeTask`, `ListWorktrees`, `CleanupTaskWorktree`
|
- `ExternalMcpService` — task CRUD + execution: `ListTaskLists`, `ListTasks`, `GetTask`, `AddTask`, `AddSubtask`, `UpdateTask`, `UpdateTaskStatus` (`Idle` / `Queued` / `Cancelled` / `Done` — `Cancelled` goes through `TaskStateService.CancelAsync(..., allowFromIdle: true)`, the only caller that opts into cancelling from `Idle`; `PlanningChainCoordinator` relies on `Idle` staying a no-op there by default, since a child parked back to `Idle` mid-chain is a manual opt-out signal; `Done` goes through `TaskStateService.ForceSetStatusAsync` — same unconditional write the UI's "set status freely" affordance uses — but is refused here with an active-worktree task, since that would skip `review_task`'s merge), `GetTaskStatusValues`, `ReviewTask` (`approve` / `reject_rerun` / `reject_park` / `cancel` for a WaitingForReview task; approve is review+merge exactly like the hub's `ApproveReview` — unit merge for parents, worktree merge into optional `targetBranch` for childless tasks, conflicts reported in the `ReviewTaskResult`), `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`
|
||||||
- `ConfigMcpTools` — `GetListConfig`, `SetListConfig`, `GetTaskConfig`, `SetTaskConfig`
|
- `ConfigMcpTools` — `GetListConfig`, `SetListConfig`, `GetTaskConfig`, `SetTaskConfig`
|
||||||
|
|||||||
+2
-2
@@ -93,8 +93,8 @@ public sealed class BatchMcpTools
|
|||||||
}
|
}
|
||||||
|
|
||||||
[McpServerTool, Description(
|
[McpServerTool, Description(
|
||||||
"Set the status of many tasks at once. status is 'Idle' (reset to editable) or " +
|
"Set the status of many tasks at once. status is 'Idle', 'Queued', 'Cancelled' or 'Done' only — " +
|
||||||
"'Queued' (enqueue for execution) only — same rule as update_task_status. " +
|
"same rule as update_task_status ('Done' is refused per-item for a task with an active worktree). " +
|
||||||
"Returns one result per id: { taskId, ok, error }. Max 100 ids.")]
|
"Returns one result per id: { taskId, ok, error }. Max 100 ids.")]
|
||||||
public async Task<IReadOnlyList<BatchTaskResult>> BatchUpdateTaskStatus(
|
public async Task<IReadOnlyList<BatchTaskResult>> BatchUpdateTaskStatus(
|
||||||
string[] taskIds, string status, CancellationToken cancellationToken)
|
string[] taskIds, string status, CancellationToken cancellationToken)
|
||||||
|
|||||||
+18
-2
@@ -264,10 +264,12 @@ public sealed class ExternalMcpService
|
|||||||
}
|
}
|
||||||
|
|
||||||
[McpServerTool, Description(
|
[McpServerTool, Description(
|
||||||
"Update a task's status. Only 'Idle', 'Queued' and 'Cancelled' are permitted externally — " +
|
"Update a task's status. Only 'Idle', 'Queued', 'Cancelled' and 'Done' are permitted externally — " +
|
||||||
"use run_task_now for execution control, and review_task to act on a WaitingForReview task. " +
|
"use run_task_now for execution control, and review_task to act on a WaitingForReview task. " +
|
||||||
"Settable: Idle (reset to editable), Queued (enqueue for execution), " +
|
"Settable: Idle (reset to editable), Queued (enqueue for execution), " +
|
||||||
"Cancelled (retire the task without deleting it; it can be reset to Idle later). " +
|
"Cancelled (retire the task without deleting it; it can be reset to Idle later), " +
|
||||||
|
"Done (mark complete; refused if the task has an active worktree — use review_task to approve " +
|
||||||
|
"and merge that worktree instead). " +
|
||||||
"Full lifecycle: Idle → Queued → Running → WaitingForReview → Done | Failed | Cancelled.")]
|
"Full lifecycle: Idle → Queued → Running → WaitingForReview → Done | Failed | Cancelled.")]
|
||||||
public async Task<TaskDto> UpdateTaskStatus(
|
public async Task<TaskDto> UpdateTaskStatus(
|
||||||
string taskId,
|
string taskId,
|
||||||
@@ -300,6 +302,20 @@ public sealed class ExternalMcpService
|
|||||||
throw new InvalidOperationException(cancelResult.Reason ?? "Cannot cancel task.");
|
throw new InvalidOperationException(cancelResult.Reason ?? "Cannot cancel task.");
|
||||||
break;
|
break;
|
||||||
|
|
||||||
|
case TaskStatus.Done:
|
||||||
|
await using (var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken))
|
||||||
|
{
|
||||||
|
var wt = await new WorktreeRepository(ctx).GetByTaskIdAsync(taskId, cancellationToken);
|
||||||
|
if (wt is not null && wt.State == WorktreeState.Active)
|
||||||
|
throw new InvalidOperationException(
|
||||||
|
"Task has an active worktree — use review_task to approve and merge instead.");
|
||||||
|
}
|
||||||
|
|
||||||
|
var doneResult = await _state.ForceSetStatusAsync(taskId, TaskStatus.Done, cancellationToken);
|
||||||
|
if (!doneResult.Ok)
|
||||||
|
throw new InvalidOperationException(doneResult.Reason ?? "Cannot set task to Done.");
|
||||||
|
break;
|
||||||
|
|
||||||
default:
|
default:
|
||||||
throw new InvalidOperationException(
|
throw new InvalidOperationException(
|
||||||
$"Status '{target}' is not settable externally. Use run_task_now or review_task.");
|
$"Status '{target}' is not settable externally. Use run_task_now or review_task.");
|
||||||
|
|||||||
@@ -329,7 +329,13 @@ public sealed class TaskStateService : ITaskStateService
|
|||||||
public async Task<TransitionResult> ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct)
|
public async Task<TransitionResult> ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct)
|
||||||
{
|
{
|
||||||
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
||||||
var affected = await ctx.Tasks
|
var affected = status == TaskStatus.Done
|
||||||
|
? await ctx.Tasks
|
||||||
|
.Where(t => t.Id == taskId)
|
||||||
|
.ExecuteUpdateAsync(s => s
|
||||||
|
.SetProperty(t => t.Status, status)
|
||||||
|
.SetProperty(t => t.FinishedAt, DateTime.UtcNow), ct)
|
||||||
|
: await ctx.Tasks
|
||||||
.Where(t => t.Id == taskId)
|
.Where(t => t.Id == taskId)
|
||||||
.ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, status), ct);
|
.ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, status), ct);
|
||||||
|
|
||||||
|
|||||||
@@ -189,6 +189,22 @@ public sealed class BatchMcpToolsTests : IDisposable
|
|||||||
Assert.Equal(TaskStatus.Queued, (await _tasks.GetByIdAsync(t2.Id))!.Status);
|
Assert.Equal(TaskStatus.Queued, (await _tasks.GetByIdAsync(t2.Id))!.Status);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task BatchUpdateTaskStatus_Done_MixedWorktreeState_ReportsPerItemAndDoesNotAbort()
|
||||||
|
{
|
||||||
|
var listId = await SeedListAsync();
|
||||||
|
var noWorktree = await SeedTaskAsync(listId, "no-wt", TaskStatus.Idle);
|
||||||
|
var missing = "does-not-exist";
|
||||||
|
var sut = BuildSut();
|
||||||
|
|
||||||
|
var results = await sut.BatchUpdateTaskStatus(
|
||||||
|
new[] { noWorktree.Id, missing }, "Done", CancellationToken.None);
|
||||||
|
|
||||||
|
Assert.True(results.Single(r => r.TaskId == noWorktree.Id).Ok);
|
||||||
|
Assert.False(results.Single(r => r.TaskId == missing).Ok);
|
||||||
|
Assert.Equal(TaskStatus.Done, (await _tasks.GetByIdAsync(noWorktree.Id))!.Status);
|
||||||
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task BatchTools_RejectEmptyBatch()
|
public async Task BatchTools_RejectEmptyBatch()
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -395,17 +395,47 @@ public sealed class ExternalMcpServiceTests : IDisposable
|
|||||||
}
|
}
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
public async Task UpdateTaskStatus_Done_StillRejected()
|
public async Task UpdateTaskStatus_WaitingForReview_StillRejected()
|
||||||
{
|
{
|
||||||
var listId = await SeedListAsync();
|
var listId = await SeedListAsync();
|
||||||
var task = await SeedTaskAsync(listId, status: TaskStatus.Idle);
|
var task = await SeedTaskAsync(listId, status: TaskStatus.Idle);
|
||||||
var sut = BuildSut(CreateQueue());
|
var sut = BuildSut(CreateQueue());
|
||||||
|
|
||||||
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
|
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
|
||||||
() => sut.UpdateTaskStatus(task.Id, "Done", CancellationToken.None));
|
() => sut.UpdateTaskStatus(task.Id, "WaitingForReview", CancellationToken.None));
|
||||||
Assert.Contains("not settable externally", ex.Message);
|
Assert.Contains("not settable externally", ex.Message);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task UpdateTaskStatus_Done_NoWorktree_SetsDoneAndFinishedAt()
|
||||||
|
{
|
||||||
|
var listId = await SeedListAsync();
|
||||||
|
var task = await SeedTaskAsync(listId, status: TaskStatus.Idle);
|
||||||
|
var sut = BuildSut(CreateQueue());
|
||||||
|
|
||||||
|
var dto = await sut.UpdateTaskStatus(task.Id, "Done", CancellationToken.None);
|
||||||
|
|
||||||
|
Assert.Equal("Done", dto.Status);
|
||||||
|
var loaded = await _tasks.GetByIdAsync(task.Id);
|
||||||
|
Assert.Equal(TaskStatus.Done, loaded!.Status);
|
||||||
|
Assert.NotNull(loaded.FinishedAt);
|
||||||
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public async Task UpdateTaskStatus_Done_WithActiveWorktree_Throws()
|
||||||
|
{
|
||||||
|
if (!GitAvailable) return;
|
||||||
|
var (task, _, _) = await SeedWorktreeAsync(TaskStatus.WaitingForReview);
|
||||||
|
var sut = BuildSut(CreateQueue());
|
||||||
|
|
||||||
|
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
|
||||||
|
() => sut.UpdateTaskStatus(task.Id, "Done", CancellationToken.None));
|
||||||
|
|
||||||
|
Assert.Contains("active worktree", ex.Message, StringComparison.OrdinalIgnoreCase);
|
||||||
|
var loaded = await _tasks.GetByIdAsync(task.Id);
|
||||||
|
Assert.Equal(TaskStatus.WaitingForReview, loaded!.Status);
|
||||||
|
}
|
||||||
|
|
||||||
private ExternalMcpService NewService() => BuildSut(CreateQueue());
|
private ExternalMcpService NewService() => BuildSut(CreateQueue());
|
||||||
|
|
||||||
private async Task<string> SeedIdleTask(string title = "t")
|
private async Task<string> SeedIdleTask(string title = "t")
|
||||||
|
|||||||
Reference in New Issue
Block a user