diff --git a/docs/explore-notes/worker-task-pipeline.md b/docs/explore-notes/worker-task-pipeline.md index a6241b0c..a145b83e 100644 --- a/docs/explore-notes/worker-task-pipeline.md +++ b/docs/explore-notes/worker-task-pipeline.md @@ -50,6 +50,9 @@ How a task moves Queued → Running → terminal, across `src/ClaudeDo.Worker` - **Done** CompleteAsync (Running → Done) — top-level success. - **WaitingForReview** SubmitForReviewAsync (Running → WaitingForReview) — review gate. - **WaitingForChildren** SubmitForChildrenAsync (Running → WaitingForChildren) — blocks on children. + Advances to WaitingForReview via `TryAdvanceParentAsync` once every remaining child is + terminal (Done/Failed/Cancelled) — including zero children left, e.g. after the last child + is deleted (`WorkerHub.DeleteTask` / `ExternalMcpService.DeleteTask` both call it). - **Failed** FailAsync (Running/Queued → Failed). - **Cancelled** CancelAsync (Running/Queued/WaitingForReview/WaitingForChildren → Cancelled). diff --git a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs index a807b6b3..221dbcf0 100644 --- a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs @@ -57,6 +57,10 @@ public interface IWorkerClient : INotifyPropertyChanged Task GetPendingQuestionAsync(string taskId); Task ResetTaskAsync(string taskId); Task CancelTaskAsync(string taskId); + /// Deletes a task via the worker (mirrors the MCP delete_task tool), so a deleted + /// child correctly advances a WaitingForChildren parent. Returns (false, message) instead of + /// throwing when the task has children or is running, preserving the FK-error UX. + Task<(bool Ok, string? Error)> DeleteTaskAsync(string taskId); Task> GetAgentsAsync(); Task RefreshAgentsAsync(); Task RestoreDefaultAgentsAsync(); diff --git a/src/ClaudeDo.Ui/Services/WorkerClient.cs b/src/ClaudeDo.Ui/Services/WorkerClient.cs index 3523f584..2e089c9f 100644 --- a/src/ClaudeDo.Ui/Services/WorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/WorkerClient.cs @@ -327,6 +327,19 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC await _hub.InvokeAsync("CancelTask", taskId); } + public async Task<(bool Ok, string? Error)> DeleteTaskAsync(string taskId) + { + try + { + await _hub.InvokeAsync("DeleteTask", taskId, CancellationToken.None); + return (true, null); + } + catch (HubException ex) + { + return (false, ex.Message); + } + } + public async Task WakeQueueAsync() { await _hub.InvokeAsync("WakeQueue"); diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 0a1ca8a2..18449758 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -946,18 +946,14 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable var ok = await ConfirmAsync($"Delete \"{row.Title}\"? This cannot be undone."); if (!ok) return; } - try - { - await using var ctx = _dbFactory.CreateDbContext(); - var repo = new TaskRepository(ctx); - await repo.DeleteAsync(row.Id); - } - catch (Microsoft.EntityFrameworkCore.DbUpdateException ex) when ( - ex.Message.Contains("FOREIGN KEY", StringComparison.OrdinalIgnoreCase) - || ex.InnerException?.Message.Contains("FOREIGN KEY", StringComparison.OrdinalIgnoreCase) == true) + // Routed through the worker (mirrors the MCP delete_task tool) so a deleted child + // correctly advances a WaitingForChildren parent — a direct-repo delete from here used + // to bypass TaskStateService.TryAdvanceParentAsync and could wedge the parent forever. + var (deleted, error) = await _worker.DeleteTaskAsync(row.Id); + if (!deleted) { if (ShowErrorAsync != null) - await ShowErrorAsync("This task has child tasks. Discard the planning session or delete child tasks first."); + await ShowErrorAsync(error ?? "Delete failed."); return; } if (DeleteFromList != null) diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index 02e80436..1fc3f6bf 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -380,6 +380,38 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub public bool CancelTask(string taskId) => _queue.CancelTask(taskId); + // Mirrors ExternalMcpService.DeleteTask so a UI-initiated delete gets the same + // TryAdvanceParentAsync side effect — a direct-repo delete from the details pane + // used to skip it, permanently wedging a WaitingForChildren parent whose last + // child was deleted from there. + public async Task DeleteTask(string taskId, CancellationToken cancellationToken) + { + await using var ctx = await _dbFactory.CreateDbContextAsync(cancellationToken); + var repo = new TaskRepository(ctx); + var task = await repo.GetByIdAsync(taskId, cancellationToken) + ?? throw new HubException("task not found"); + if (task.Status == TaskStatus.Running) + throw new HubException("Cannot delete a running task. Cancel it first."); + + try + { + await repo.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 HubException("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); + } + public void WakeQueue() => _waker.Wake(); public async Task> GetAgents() => await _agentService.ScanAsync(); diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index c04fe777..73a73d3e 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -505,8 +505,8 @@ public sealed class TaskStateService : ITaskStateService .Select(t => t.Status) .ToListAsync(CancellationToken.None); } - if (childStatuses.Count == 0) return; - + // No early-out on an empty list: zero children left (e.g. the last one was just + // deleted) counts as "all terminal" — .All() on an empty sequence is vacuously true. bool allTerminal = childStatuses.All(s => s == TaskStatus.Done || s == TaskStatus.Failed || s == TaskStatus.Cancelled); if (!allTerminal) return; diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index 75a79abd..f6f21f71 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -79,6 +79,7 @@ public abstract class StubWorkerClient : IWorkerClient public virtual Task GetPendingQuestionAsync(string taskId) => Task.FromResult(PendingQuestion); public virtual Task ResetTaskAsync(string taskId) => Task.CompletedTask; public virtual Task CancelTaskAsync(string taskId) => Task.CompletedTask; + public virtual Task<(bool Ok, string? Error)> DeleteTaskAsync(string taskId) => Task.FromResult<(bool, string?)>((true, null)); public virtual Task> GetAgentsAsync() => Task.FromResult(new List()); public virtual Task RefreshAgentsAsync() => Task.CompletedTask; public virtual Task RestoreDefaultAgentsAsync() => Task.FromResult(null); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDeleteTaskTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDeleteTaskTests.cs new file mode 100644 index 00000000..3036b27a --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandDeleteTaskTests.cs @@ -0,0 +1,117 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Ui.Services; +using ClaudeDo.Ui.ViewModels.Islands; +using Microsoft.EntityFrameworkCore; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +public class DetailsIslandDeleteTaskTests : IDisposable +{ + private readonly string _dbPath; + + public DetailsIslandDeleteTaskTests() + { + _dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_delete_task_test_{Guid.NewGuid():N}.db"); + using var ctx = NewContext(); + ctx.Database.EnsureCreated(); + } + + public void Dispose() + { + try { File.Delete(_dbPath); } catch { } + try { File.Delete(_dbPath + "-wal"); } catch { } + try { File.Delete(_dbPath + "-shm"); } catch { } + } + + private ClaudeDoDbContext NewContext() + { + var opts = new DbContextOptionsBuilder() + .UseSqlite($"Data Source={_dbPath}") + .Options; + return new ClaudeDoDbContext(opts); + } + + private sealed class TestDbFactory : IDbContextFactory + { + private readonly Func _create; + public TestDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + } + + private sealed class NullServiceProvider : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } + + private sealed class StubNotesApi : ClaudeDo.Ui.Services.Interfaces.INotesApi + { + public Task> ListAsync(DateOnly day) => + Task.FromResult(new List()); + public Task AddAsync(DateOnly day, string text) => + Task.FromResult(null); + public Task UpdateAsync(string id, string text) => Task.CompletedTask; + public Task DeleteAsync(string id) => Task.CompletedTask; + } + + private sealed class RecordingWorkerClient : StubWorkerClient + { + public override bool IsConnected => true; + public (bool Ok, string? Error) Result { get; set; } = (true, null); + public string? DeletedTaskId { get; private set; } + + public override Task<(bool Ok, string? Error)> DeleteTaskAsync(string taskId) + { + DeletedTaskId = taskId; + return Task.FromResult(Result); + } + } + + private DetailsIslandViewModel BuildVm(RecordingWorkerClient worker) + { + var factory = new TestDbFactory(NewContext); + return new DetailsIslandViewModel(factory, worker, new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); + } + + [Fact] + public async Task DeleteTask_Succeeds_RemovesFromListAndClosesDetail() + { + var worker = new RecordingWorkerClient { Result = (true, null) }; + var vm = BuildVm(worker); + vm.Bind(new TaskRowViewModel { Id = "task-del-1", Status = TaskStatus.Idle }); + + TaskRowViewModel? removed = null; + vm.DeleteFromList = row => { removed = row; return Task.CompletedTask; }; + var closed = false; + vm.CloseDetail = () => closed = true; + + await vm.DeleteTaskCommand.ExecuteAsync(null); + + Assert.Equal("task-del-1", worker.DeletedTaskId); + Assert.Equal("task-del-1", removed?.Id); + Assert.True(closed); + } + + [Fact] + public async Task DeleteTask_WhenTaskHasChildren_SurfacesErrorAndKeepsDetailOpen() + { + const string fkMessage = "This task has child tasks. Discard the planning session or delete child tasks first."; + var worker = new RecordingWorkerClient { Result = (false, fkMessage) }; + var vm = BuildVm(worker); + vm.Bind(new TaskRowViewModel { Id = "task-del-2", Status = TaskStatus.WaitingForChildren }); + + var deleteFromListCalled = false; + vm.DeleteFromList = _ => { deleteFromListCalled = true; return Task.CompletedTask; }; + var closed = false; + vm.CloseDetail = () => closed = true; + string? reportedError = null; + vm.ShowErrorAsync = msg => { reportedError = msg; return Task.CompletedTask; }; + + await vm.DeleteTaskCommand.ExecuteAsync(null); + + Assert.Equal(fkMessage, reportedError); + Assert.False(deleteFromListCalled); + Assert.False(closed); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/Hub/DeleteTaskHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/DeleteTaskHubTests.cs new file mode 100644 index 00000000..a0ff1339 --- /dev/null +++ b/tests/ClaudeDo.Worker.Tests/Hub/DeleteTaskHubTests.cs @@ -0,0 +1,107 @@ +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 DeleteTaskHubTests : 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<(string ParentId, string ChildId)> SeedParentWithChildAsync(TaskStatus childStatus) + { + 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 repo = new TaskRepository(ctx); + var parentId = Guid.NewGuid().ToString(); + await repo.AddAsync(new TaskEntity + { + Id = parentId, ListId = listId, Title = "Parent", + Status = TaskStatus.WaitingForChildren, PlanningPhase = PlanningPhase.Finalized, + CreatedAt = DateTime.UtcNow, CommitType = "feat", + }); + + var childId = Guid.NewGuid().ToString(); + await repo.AddAsync(new TaskEntity + { + Id = childId, ListId = listId, Title = "Child", ParentTaskId = parentId, + Status = childStatus, CreatedAt = DateTime.UtcNow, CommitType = "feat", + }); + + return (parentId, childId); + } + + [Fact] + public async Task DeleteTask_LastChildOfWaitingForChildrenParent_AdvancesParentToWaitingForReview() + { + var (parentId, childId) = await SeedParentWithChildAsync(TaskStatus.Idle); + var hub = CreateHub(); + + await hub.DeleteTask(childId, default); + + await using var ctx = _db.CreateContext(); + Assert.Null(await ctx.Tasks.FindAsync(childId)); + var parent = await ctx.Tasks.FindAsync(parentId); + Assert.Equal(TaskStatus.WaitingForReview, parent!.Status); + } + + [Fact] + public async Task DeleteTask_RunningTask_Throws_AndDoesNotDelete() + { + var (_, childId) = await SeedParentWithChildAsync(TaskStatus.Running); + var hub = CreateHub(); + + await Assert.ThrowsAsync(() => hub.DeleteTask(childId, default)); + + await using var ctx = _db.CreateContext(); + Assert.NotNull(await ctx.Tasks.FindAsync(childId)); + } + + [Fact] + public async Task DeleteTask_TaskWithChildren_ThrowsFriendlyForeignKeyMessage() + { + var (parentId, _) = await SeedParentWithChildAsync(TaskStatus.Idle); + var hub = CreateHub(); + + var ex = await Assert.ThrowsAsync(() => hub.DeleteTask(parentId, default)); + + Assert.Contains("child tasks", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public async Task DeleteTask_Missing_Throws() + { + var hub = CreateHub(); + + await Assert.ThrowsAsync(() => hub.DeleteTask("does-not-exist", default)); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index a2ce8a7b..fd06b7ed 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs @@ -479,4 +479,42 @@ public sealed class TaskStateServiceTests : IDisposable Assert.Equal(TaskStatus.Queued, t2.Status); Assert.Equal(c1, t2.BlockedByTaskId); } + + // ─── TryAdvanceParentAsync ──────────────────────────────────────────── + + [Fact] + public async Task TryAdvanceParentAsync_ZeroChildren_AdvancesToWaitingForReview() + { + // Mirrors deleting the last remaining child: no terminal transition fires for it, + // so the caller invokes TryAdvanceParentAsync directly with zero children left. + var parent = await SeedTaskAsync(TaskStatus.WaitingForChildren, phase: PlanningPhase.Finalized); + + await _sut.TryAdvanceParentAsync(parent); + + Assert.Equal(TaskStatus.WaitingForReview, await GetStatusAsync(parent)); + } + + [Fact] + public async Task TryAdvanceParentAsync_AllChildrenTerminal_AdvancesToWaitingForReview() + { + var parent = await SeedTaskAsync(TaskStatus.WaitingForChildren, phase: PlanningPhase.Finalized); + await SeedTaskAsync(TaskStatus.Done, parentId: parent); + await SeedTaskAsync(TaskStatus.Done, parentId: parent); + + await _sut.TryAdvanceParentAsync(parent); + + Assert.Equal(TaskStatus.WaitingForReview, await GetStatusAsync(parent)); + } + + [Fact] + public async Task TryAdvanceParentAsync_SomeChildrenStillRunning_DoesNotAdvance() + { + var parent = await SeedTaskAsync(TaskStatus.WaitingForChildren, phase: PlanningPhase.Finalized); + await SeedTaskAsync(TaskStatus.Done, parentId: parent); + await SeedTaskAsync(TaskStatus.Running, parentId: parent); + + await _sut.TryAdvanceParentAsync(parent); + + Assert.Equal(TaskStatus.WaitingForChildren, await GetStatusAsync(parent)); + } } diff --git a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs index 735ea2d4..250927d3 100644 --- a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs +++ b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs @@ -47,6 +47,7 @@ sealed class FakeWorkerClient : IWorkerClient public Task GetPendingQuestionAsync(string taskId) => Task.FromResult(null); public Task ResetTaskAsync(string taskId) => Task.CompletedTask; public Task CancelTaskAsync(string taskId) => Task.CompletedTask; + public Task<(bool Ok, string? Error)> DeleteTaskAsync(string taskId) => Task.FromResult<(bool, string?)>((true, null)); public Task> GetAgentsAsync() => Task.FromResult(new List()); public Task RefreshAgentsAsync() => Task.CompletedTask; public Task RestoreDefaultAgentsAsync() => Task.FromResult(null);