From 299c7b749c0b226309c6d6ff01de0381c2e84e54 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 26 Aug 2026 15:01:13 +0200 Subject: [PATCH] fix(worker): close status-force bypasses on Running tasks update_task_status Idle/Done, WorkerHub.SetTaskStatus, and the UI's Mark Done/Cancelled menu items could all force a Running task's status without going through TaskStateService's guards, leaving its CLI process untracked and its worktree writes silently dropped. Idle now routes through the already-guarded TaskStateService.ResetToIdleAsync instead of the repo's unconditional ResetToManualAsync; Done and SetTaskStatus explicitly reject Running; the context menu disables both entries with a tooltip while running. TaskRunner also now logs a warning whenever a terminal transition (SubmitForChildren/SubmitForReview/Complete/Fail) is rejected instead of silently discarding the run result. DeleteTask's MCP path gets the same friendly foreign-key message WorkerHub.DeleteTask already had for a task with children. --- .../ViewModels/Islands/TaskRowViewModel.cs | 8 ++ .../Views/Islands/TaskRowView.axaml.cs | 4 +- .../External/ExternalMcpService.cs | 29 +++++-- src/ClaudeDo.Worker/Hub/WorkerHub.cs | 7 ++ src/ClaudeDo.Worker/Runner/TaskRunner.cs | 16 +++- .../External/ExternalMcpServiceTests.cs | 44 ++++++++++ .../Hub/SetTaskStatusHubTests.cs | 86 +++++++++++++++++++ 7 files changed, 183 insertions(+), 11 deletions(-) create mode 100644 tests/ClaudeDo.Worker.Tests/Hub/SetTaskStatusHubTests.cs diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs index 58564405..0142e77a 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs @@ -171,6 +171,13 @@ public sealed partial class TaskRowViewModel : ViewModelBase IsRunning ? (true, null) : (false, Loc.T("tasks.reasonNotRunning")); public string? CancelDisabledReason => CancelGate().Reason; + // Gates "Mark Done"/"Mark Cancelled" in the context menu — forcing either status while the + // task is actively running would strand the CLI process untracked (WorkerHub.SetTaskStatus + // rejects it server-side too; this is the UI-visible half of that guard). + private (bool Can, string? Reason) MarkAsGate() => + IsRunning ? (false, Loc.T("tasks.reasonAlreadyRunning")) : (true, null); + public string? MarkAsDisabledReason => MarkAsGate().Reason; + // "Open quick session" has no precondition today (see A1's file header) — kept as a // placeholder so the menu wiring is uniform; always null until a real gate exists. public string? QuickSessionDisabledReason => null; @@ -318,6 +325,7 @@ public sealed partial class TaskRowViewModel : ViewModelBase OnPropertyChanged(nameof(CancelDisabledReason)); OnPropertyChanged(nameof(RefineDisabledReason)); OnPropertyChanged(nameof(PlanningDisabledReason)); + OnPropertyChanged(nameof(MarkAsDisabledReason)); } partial void OnParentTaskIdChanged(string? value) diff --git a/src/ClaudeDo.Ui/Views/Islands/TaskRowView.axaml.cs b/src/ClaudeDo.Ui/Views/Islands/TaskRowView.axaml.cs index 274d48b4..93a56e8d 100644 --- a/src/ClaudeDo.Ui/Views/Islands/TaskRowView.axaml.cs +++ b/src/ClaudeDo.Ui/Views/Islands/TaskRowView.axaml.cs @@ -67,8 +67,8 @@ public partial class TaskRowView : UserControl // "Mark as" — exclusive-choice hiding inside is unchanged; the header is never empty. var markAs = new MenuItem { Header = Loc.T("tasks.ctxMarkAs"), DataContext = row }; - markAs.Items.Add(MakeItem("tasks.ctxMarkDone", OnSetStatusClick, tag: "Done")); - markAs.Items.Add(MakeItem("tasks.ctxMarkCancelled", OnSetStatusClick, tag: "Cancelled")); + markAs.Items.Add(MakeItem("tasks.ctxMarkDone", OnSetStatusClick, tag: "Done", reason: row.MarkAsDisabledReason)); + markAs.Items.Add(MakeItem("tasks.ctxMarkCancelled", OnSetStatusClick, tag: "Cancelled", reason: row.MarkAsDisabledReason)); markAs.Items.Add(new Separator()); markAs.Items.Add(MakeItem("tasks.ctxMarkManual", OnToggleManualClick, !row.IsManual)); markAs.Items.Add(MakeItem("tasks.ctxMarkClaudeTask", OnToggleManualClick, row.IsManual)); diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index e858a1fc..31eb07ec 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -592,8 +592,9 @@ public sealed class ExternalMcpService string taskId, [Description("'Idle' (reset to editable), 'Queued' (enqueue for execution), 'Cancelled' (retire without " + "deleting; can be reset to Idle later) or 'Done' (mark complete; refused if the task has an " + - "active worktree — use review_task to approve and merge that worktree instead). No other " + - "value is settable externally.")] + "active worktree — use review_task to approve and merge that worktree instead). 'Idle' and " + + "'Done' are both refused while the task is Running — cancel it first. No other value is " + + "settable externally.")] string status, CancellationToken cancellationToken) { @@ -609,8 +610,9 @@ public sealed class ExternalMcpService switch (target) { case TaskStatus.Idle: - await _tasks.ResetToManualAsync(taskId, cancellationToken); - await _broadcaster.TaskUpdated(taskId); + var idleResult = await _state.ResetToIdleAsync(taskId, cancellationToken); + if (!idleResult.Ok) + throw new InvalidOperationException(idleResult.Reason ?? "Cannot reset task to Idle."); break; case TaskStatus.Queued: @@ -627,6 +629,9 @@ public sealed class ExternalMcpService break; case TaskStatus.Done: + if (task.Status == TaskStatus.Running) + throw new InvalidOperationException("Cannot mark a running task Done — cancel it first."); + await using (var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken)) { var wt = await new WorktreeRepository(ctx).GetByTaskIdAsync(taskId, cancellationToken); @@ -870,7 +875,21 @@ public sealed class ExternalMcpService if (task.Status == TaskStatus.Running) throw new InvalidOperationException("Cannot delete a running task. Cancel it first."); - await _tasks.DeleteAsync(taskId, cancellationToken); + try + { + await _tasks.DeleteAsync(taskId, cancellationToken); + } + // TaskRepository.DeleteAsync uses ExecuteDeleteAsync, which bypasses SaveChanges and + // surfaces provider errors directly as SqliteException rather than DbUpdateException. + catch (Exception ex) when ( + (ex is Microsoft.Data.Sqlite.SqliteException || ex.InnerException is Microsoft.Data.Sqlite.SqliteException) + && (ex.Message.Contains("FOREIGN KEY", StringComparison.OrdinalIgnoreCase) + || ex.InnerException?.Message.Contains("FOREIGN KEY", StringComparison.OrdinalIgnoreCase) == true)) + { + throw new InvalidOperationException( + "This task has child tasks. Discard the planning session or delete child tasks first."); + } + if (task.ParentTaskId is not null) await _state.TryAdvanceParentAsync(task.ParentTaskId); await _broadcaster.TaskUpdated(taskId); diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index d10bc8cb..afcfcbea 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -586,6 +586,13 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub { if (!Enum.TryParse(status, ignoreCase: true, out var parsed)) throw new HubException($"unknown status: {status}"); + + await using var ctx = await _dbFactory.CreateDbContextAsync(Context.ConnectionAborted); + var current = await new TaskRepository(ctx).GetByIdAsync(taskId, Context.ConnectionAborted) + ?? throw new HubException("task not found"); + if (current.Status == TaskStatus.Running) + throw new HubException("Cannot change the status of a running task. Cancel it first."); + // Queueing goes through the gated transition so draft subtasks can't be queued; // other statuses keep the unconditional "set status freely" affordance. var result = parsed == TaskStatus.Queued diff --git a/src/ClaudeDo.Worker/Runner/TaskRunner.cs b/src/ClaudeDo.Worker/Runner/TaskRunner.cs index 23d1170c..ce6dd541 100644 --- a/src/ClaudeDo.Worker/Runner/TaskRunner.cs +++ b/src/ClaudeDo.Worker/Runner/TaskRunner.cs @@ -550,7 +550,9 @@ public sealed class TaskRunner if (isStandalone && pendingChildren.Count > 0) { - await _state.SubmitForChildrenAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + var transition = await _state.SubmitForChildrenAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + if (!transition.Ok) + _logger.LogWarning("SubmitForChildrenAsync rejected for task {TaskId}: {Reason}", task.Id, transition.Reason); foreach (var child in pendingChildren) await _state.EnqueueAsync(child.Id, CancellationToken.None); await _broadcaster.WorkerLog( @@ -560,13 +562,17 @@ public sealed class TaskRunner } else if (isStandalone) { - await _state.SubmitForReviewAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + var transition = await _state.SubmitForReviewAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + if (!transition.Ok) + _logger.LogWarning("SubmitForReviewAsync rejected for task {TaskId}: {Reason}", task.Id, transition.Reason); await _broadcaster.WorkerLog($"Finished #{task.Number} \"{task.Title}\" (waiting for review)", WorkerLogLevel.Success, DateTime.UtcNow); await _broadcaster.TaskFinished(slot, task.Id, "waiting_for_review", finishedAt); } else { - await _state.CompleteAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + var transition = await _state.CompleteAsync(task.Id, finishedAt, reviewResult, CancellationToken.None); + if (!transition.Ok) + _logger.LogWarning("CompleteAsync rejected for task {TaskId}: {Reason}", task.Id, transition.Reason); await _broadcaster.WorkerLog($"Finished #{task.Number} \"{task.Title}\" (done)", WorkerLogLevel.Success, DateTime.UtcNow); await _broadcaster.TaskFinished(slot, task.Id, "done", finishedAt); } @@ -583,8 +589,10 @@ public sealed class TaskRunner try { var finishedAt = DateTime.UtcNow; - await _state.FailAsync(taskId, finishedAt, error, CancellationToken.None, + var transition = await _state.FailAsync(taskId, finishedAt, error, CancellationToken.None, failureReason, turnCount > 0 ? turnCount : null, maxTurns); + if (!transition.Ok) + _logger.LogWarning("FailAsync rejected for task {TaskId}: {Reason}", taskId, transition.Reason); await _broadcaster.WorkerLog($"Finished #{taskNumber} \"{taskTitle}\" (failed)", WorkerLogLevel.Error, DateTime.UtcNow); await _broadcaster.TaskFinished(slot, taskId, "failed", finishedAt); _logger.LogWarning("Task {TaskId} failed (turns={Turns}): {Error}", taskId, turnCount, error); diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 56426233..82f8c9d0 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -900,6 +900,21 @@ public sealed class ExternalMcpServiceTests : IDisposable sut.DeleteTask(task.Id, CancellationToken.None)); } + [Fact] + public async Task DeleteTask_WithChildren_ThrowsUnderstandableMessage() + { + var listId = await SeedListAsync(); + var parent = await SeedTaskAsync(listId, status: TaskStatus.WaitingForChildren); + await SeedTaskAsync(listId, status: TaskStatus.Idle, parentId: parent.Id); + var sut = BuildSut(CreateQueue()); + + var ex = await Assert.ThrowsAsync(() => + sut.DeleteTask(parent.Id, CancellationToken.None)); + + Assert.Contains("child task", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.NotNull(await _tasks.GetByIdAsync(parent.Id)); + } + [Fact] public async Task DeleteTask_NotFound_Throws() { @@ -981,6 +996,35 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Equal(TaskStatus.WaitingForReview, loaded!.Status); } + [Fact] + public async Task UpdateTaskStatus_Idle_OnRunningTask_Throws() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, status: TaskStatus.Running); + var sut = BuildSut(CreateQueue()); + + await Assert.ThrowsAsync( + () => sut.UpdateTaskStatus(task.Id, "Idle", CancellationToken.None)); + + var loaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Running, loaded!.Status); + } + + [Fact] + public async Task UpdateTaskStatus_Done_OnRunningTask_Throws() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, status: TaskStatus.Running); + var sut = BuildSut(CreateQueue()); + + var ex = await Assert.ThrowsAsync( + () => sut.UpdateTaskStatus(task.Id, "Done", CancellationToken.None)); + + Assert.Contains("running", ex.Message, StringComparison.OrdinalIgnoreCase); + var loaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Running, loaded!.Status); + } + private ExternalMcpService NewService() => BuildSut(CreateQueue()); private async Task SeedIdleTask(string title = "t") diff --git a/tests/ClaudeDo.Worker.Tests/Hub/SetTaskStatusHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/SetTaskStatusHubTests.cs new file mode 100644 index 00000000..ccdbb563 --- /dev/null +++ b/tests/ClaudeDo.Worker.Tests/Hub/SetTaskStatusHubTests.cs @@ -0,0 +1,86 @@ +using ClaudeDo.Data.Models; +using ClaudeDo.Data.Repositories; +using ClaudeDo.Worker.Hub; +using ClaudeDo.Worker.Tests.Infrastructure; +using Microsoft.AspNetCore.SignalR; +using Microsoft.EntityFrameworkCore; +using Xunit; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Worker.Tests.Hub; + +public sealed class SetTaskStatusHubTests : IDisposable +{ + private readonly DbFixture _db = new(); + + public void Dispose() => _db.Dispose(); + + private WorkerHub CreateHub() + { + var factory = _db.CreateFactory(); + var built = TaskStateServiceBuilder.Build(factory); + var broadcaster = new HubBroadcaster(new CapturingHubContext()); + var hub = new WorkerHub( + null!, null!, null!, null!, broadcaster, factory, + null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, + built.State, + null!, null!, + null!, new ClaudeDo.Worker.Online.OnlineInboxConfig(), new ClaudeDo.Worker.Online.OnlineTokenStore(), + new ClaudeDo.Worker.Runner.PendingQuestionRegistry(), null!); + hub.Clients = new FakeHubCallerClients(new RecordingClientProxy()); + hub.Context = new FakeHubCallerContext(); + return hub; + } + + private async Task SeedTaskAsync(TaskStatus status) + { + using var ctx = _db.CreateContext(); + var listId = Guid.NewGuid().ToString(); + await new ListRepository(ctx).AddAsync(new ListEntity + { + Id = listId, Name = "L", CreatedAt = DateTime.UtcNow, + }); + + var taskId = Guid.NewGuid().ToString(); + await new TaskRepository(ctx).AddAsync(new TaskEntity + { + Id = taskId, ListId = listId, Title = "T", + Status = status, CreatedAt = DateTime.UtcNow, CommitType = "feat", + }); + return taskId; + } + + [Fact] + public async Task SetTaskStatus_RunningTask_Throws_AndLeavesStatusUnchanged() + { + var taskId = await SeedTaskAsync(TaskStatus.Running); + var hub = CreateHub(); + + await Assert.ThrowsAsync(() => hub.SetTaskStatus(taskId, "Done")); + + await using var ctx = _db.CreateContext(); + var task = await ctx.Tasks.FindAsync(taskId); + Assert.Equal(TaskStatus.Running, task!.Status); + } + + [Fact] + public async Task SetTaskStatus_IdleTask_StillSucceeds() + { + var taskId = await SeedTaskAsync(TaskStatus.Idle); + var hub = CreateHub(); + + await hub.SetTaskStatus(taskId, "Cancelled"); + + await using var ctx = _db.CreateContext(); + var task = await ctx.Tasks.FindAsync(taskId); + Assert.Equal(TaskStatus.Cancelled, task!.Status); + } + + [Fact] + public async Task SetTaskStatus_Missing_Throws() + { + var hub = CreateHub(); + + await Assert.ThrowsAsync(() => hub.SetTaskStatus("does-not-exist", "Done")); + } +}