From f80af122e946b2d623fb4b86ad7d6c16a0193958 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Wed, 26 Aug 2026 10:27:56 +0200 Subject: [PATCH] fix(worker): allow the done toggle from any non-Running status MarkDoneAsync was Idle-only, so a finished task (typically a list-handler run with no worktree to merge) could not be ticked off. Guard on Running instead and cover the other statuses with tests. --- src/ClaudeDo.Worker/CLAUDE.md | 8 +++++++- src/ClaudeDo.Worker/State/TaskStateService.cs | 9 +++++++-- .../Hub/TaskDoneDequeueHubTests.cs | 15 +++++++++++++++ .../State/TaskStateServiceTests.cs | 18 ++++++++++++++++++ 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 85d1da94..c1bec3fe 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -70,7 +70,7 @@ not conflated. Allowed transitions (enforced by `TaskStateService`): ``` -Idle → Queued | Running (RunNow) | Done (manual toggle) | Cancelled (external update_task_status only, allowFromIdle: true) +Idle → Queued | Running (RunNow) | Cancelled (external update_task_status only, allowFromIdle: true) Queued → Running | Cancelled | Idle | Failed (OverrideSlotService preflight gap) Running → WaitingForReview (standalone success, no children) | WaitingForChildren (parent with pending children) @@ -80,8 +80,14 @@ WaitingForReview → Done (approve) | Queued (reject-rerun, +feedback) | Idle Done → Idle (re-run) Failed → Idle | Queued Cancelled → Idle | Queued + +any except Running → Done (manual done toggle, SetTaskDone) ``` +The manual done toggle is the one cross-cutting transition — it ticks a task off from **any** +status except `Running`, skipping the merge on purpose (a worktree-less list-handler task has +nothing to merge; a worktree task's branch just stays Active for the worktrees overview). + **Unified parent model.** Every parent — planning *or* improvement — flows `… → WaitingForChildren → WaitingForReview → Done` via the single `TryAdvanceParentAsync`. Planning/improvement **children** go straight to `Done`; only the parent is reviewed. diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index 0d473e82..7193e985 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -382,14 +382,19 @@ public sealed class TaskStateService : ITaskStateService public async Task MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct) { await using var ctx = await _dbFactory.CreateDbContextAsync(ct); + // The done toggle is a manual "tick it off" affordance and works from every status + // except Running (a live run keeps writing the row, so the picker/runner would race). + // WaitingForReview is deliberately included: ticking a finished task done skips the + // merge on purpose -- for a worktree-less list-handler task there is nothing to merge, + // and for a worktree task the branch just stays Active for the worktrees overview. var affected = await ctx.Tasks - .Where(t => t.Id == taskId && t.Status == TaskStatus.Idle) + .Where(t => t.Id == taskId && t.Status != TaskStatus.Running) .ExecuteUpdateAsync(s => s .SetProperty(t => t.Status, TaskStatus.Done) .SetProperty(t => t.FinishedAt, finishedAt), ct); if (affected == 0) - return new TransitionResult(false, "Task is not Idle; cannot mark done."); + return new TransitionResult(false, "Task is running or no longer exists; cannot mark done."); await _broadcaster.TaskUpdated(taskId); return new TransitionResult(true, null); diff --git a/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs index 3a169773..32571b56 100644 --- a/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs @@ -98,6 +98,21 @@ public sealed class TaskDoneDequeueHubTests : IDisposable Assert.Equal(TaskStatus.Running, reloaded!.Status); } + // Regression: the guard was Idle-only, so a finished task (typically a list-handler run, + // which has no worktree to merge) could no longer be ticked off from the task card. + [Fact] + public async Task SetTaskDone_FromWaitingForReview_TransitionsToDone() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.WaitingForReview); + + var hub = CreateHub(); + await hub.SetTaskDone(task.Id); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Done, reloaded!.Status); + } + // ── UnsetTaskDone ── [Fact] diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index 2909f490..521f239e 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs @@ -504,6 +504,24 @@ public sealed class TaskStateServiceTests : IDisposable Assert.Equal(TaskStatus.Running, await GetStatusAsync(id)); } + // The done toggle is a manual affordance and must work from any non-Running status -- + // notably WaitingForReview, where it deliberately skips the merge. + [Theory] + [InlineData(TaskStatus.Queued)] + [InlineData(TaskStatus.WaitingForReview)] + [InlineData(TaskStatus.WaitingForChildren)] + [InlineData(TaskStatus.Failed)] + [InlineData(TaskStatus.Cancelled)] + public async Task MarkDoneAsync_FromNonRunningStatus_TransitionsToDone(TaskStatus from) + { + var id = await SeedTaskAsync(from); + + var result = await _sut.MarkDoneAsync(id, DateTime.UtcNow, default); + + Assert.True(result.Ok); + Assert.Equal(TaskStatus.Done, await GetStatusAsync(id)); + } + [Fact] public async Task UnmarkDoneAsync_FromDone_TransitionsToIdle() {