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.
This commit is contained in:
@@ -70,7 +70,7 @@ not conflated.
|
|||||||
Allowed transitions (enforced by `TaskStateService`):
|
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)
|
Queued → Running | Cancelled | Idle | Failed (OverrideSlotService preflight gap)
|
||||||
Running → WaitingForReview (standalone success, no children)
|
Running → WaitingForReview (standalone success, no children)
|
||||||
| WaitingForChildren (parent with pending children)
|
| WaitingForChildren (parent with pending children)
|
||||||
@@ -80,8 +80,14 @@ WaitingForReview → Done (approve) | Queued (reject-rerun, +feedback) | Idle
|
|||||||
Done → Idle (re-run)
|
Done → Idle (re-run)
|
||||||
Failed → Idle | Queued
|
Failed → Idle | Queued
|
||||||
Cancelled → 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
|
**Unified parent model.** Every parent — planning *or* improvement — flows
|
||||||
`… → WaitingForChildren → WaitingForReview → Done` via the single `TryAdvanceParentAsync`.
|
`… → WaitingForChildren → WaitingForReview → Done` via the single `TryAdvanceParentAsync`.
|
||||||
Planning/improvement **children** go straight to `Done`; only the parent is reviewed.
|
Planning/improvement **children** go straight to `Done`; only the parent is reviewed.
|
||||||
|
|||||||
@@ -382,14 +382,19 @@ public sealed class TaskStateService : ITaskStateService
|
|||||||
public async Task<TransitionResult> MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct)
|
public async Task<TransitionResult> MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct)
|
||||||
{
|
{
|
||||||
await using var ctx = await _dbFactory.CreateDbContextAsync(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
|
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
|
.ExecuteUpdateAsync(s => s
|
||||||
.SetProperty(t => t.Status, TaskStatus.Done)
|
.SetProperty(t => t.Status, TaskStatus.Done)
|
||||||
.SetProperty(t => t.FinishedAt, finishedAt), ct);
|
.SetProperty(t => t.FinishedAt, finishedAt), ct);
|
||||||
|
|
||||||
if (affected == 0)
|
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);
|
await _broadcaster.TaskUpdated(taskId);
|
||||||
return new TransitionResult(true, null);
|
return new TransitionResult(true, null);
|
||||||
|
|||||||
@@ -98,6 +98,21 @@ public sealed class TaskDoneDequeueHubTests : IDisposable
|
|||||||
Assert.Equal(TaskStatus.Running, reloaded!.Status);
|
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 ──
|
// ── UnsetTaskDone ──
|
||||||
|
|
||||||
[Fact]
|
[Fact]
|
||||||
|
|||||||
@@ -504,6 +504,24 @@ public sealed class TaskStateServiceTests : IDisposable
|
|||||||
Assert.Equal(TaskStatus.Running, await GetStatusAsync(id));
|
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]
|
[Fact]
|
||||||
public async Task UnmarkDoneAsync_FromDone_TransitionsToIdle()
|
public async Task UnmarkDoneAsync_FromDone_TransitionsToIdle()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user