refactor(worker): drop ForceSetStatusAsync, route every status write through a guarded transition
Mark-done now fires OnChildTerminalAsync, so ticking a chain child or the last open child off by hand unblocks the successor and advances the parent. The hub's Mark-as menu and update_task_status map onto MarkDone/Cancel(retire)/Enqueue/Reset; revert_merge uses the new ReopenForReviewAsync (Done|WaitingForReview -> WaitingForReview).
This commit is contained in:
mika kuns
@@ -109,13 +109,13 @@ Description/Result) in `tasksFull` instead (`tasks` null). Filtering by `created
|
||||
happens before the lean/full projection either way.
|
||||
|
||||
**`UpdateTaskStatus`** accepts `Idle` / `Queued` / `Cancelled` / `Done` only.
|
||||
- `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, because a child parked back to `Idle` mid-chain is
|
||||
a manual opt-out signal.
|
||||
- `Done` goes through `TaskStateService.ForceSetStatusAsync` (the same unconditional write the
|
||||
UI's "set status freely" affordance uses) but is **refused** for a task with an active
|
||||
worktree, since that would skip `review_task`'s merge.
|
||||
- `Cancelled` goes through `TaskStateService.CancelAsync(..., retire: true)` — like the UI's
|
||||
"Mark as cancelled", it opts into cancelling from `Idle`/`Failed`. `PlanningChainCoordinator`
|
||||
relies on `Idle` staying a no-op there by default, because a child parked back to `Idle`
|
||||
mid-chain is a manual opt-out signal.
|
||||
- `Done` goes through `TaskStateService.MarkDoneAsync` (the done toggle, so a chain successor
|
||||
unblocks and the parent advances) but is **refused** for a task with an active worktree,
|
||||
since that would skip `review_task`'s merge.
|
||||
|
||||
**`AddTask`** — always creates the task; also returns `possibleDuplicates` (up to 3, id/title/status
|
||||
only, no descriptions) — open (non-terminal) tasks in the *same list* whose normalized title
|
||||
|
||||
@@ -76,23 +76,29 @@ not conflated.
|
||||
Allowed transitions (enforced by `TaskStateService`):
|
||||
|
||||
```
|
||||
Idle → Queued | Running (RunNow) | Cancelled (external update_task_status only, allowFromIdle: true)
|
||||
Idle → Queued | Running (RunNow) | WaitingForReview (interactive submit)
|
||||
| Cancelled (Mark as / update_task_status, retire: true)
|
||||
Queued → Running | Cancelled | Idle | Failed (OverrideSlotService preflight gap)
|
||||
Running → WaitingForReview (standalone success, no children)
|
||||
| WaitingForChildren (parent with pending children)
|
||||
| Done (planning/improvement child success) | Failed | Cancelled
|
||||
WaitingForChildren → WaitingForReview (all children terminal) | Cancelled
|
||||
WaitingForReview → Done (approve) | Queued (reject-rerun, +feedback) | Idle (reject-park) | Cancelled
|
||||
Done → Idle (re-run)
|
||||
Failed → Idle | Queued
|
||||
Done → Idle (re-run) | WaitingForReview (revert_merge)
|
||||
Failed → Idle | Queued | WaitingForReview (interactive submit) | Cancelled (retire)
|
||||
Cancelled → Idle | Queued
|
||||
|
||||
any except Running → Done (manual done toggle, SetTaskDone)
|
||||
any except Running → Done (manual done toggle, SetTaskDone) | Queued (Enqueue) | Idle (Reset)
|
||||
```
|
||||
|
||||
There is no unguarded status write: the hub's "Mark as" menu and `update_task_status` map every
|
||||
status onto one of the transitions above (`Running`/`Waiting*` can't be set by hand).
|
||||
|
||||
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).
|
||||
nothing to merge; a worktree task's branch just stays Active for the worktrees overview). Like
|
||||
every Done transition it fires `OnChildTerminalAsync`, so a chain successor unblocks and a
|
||||
`WaitingForChildren` parent advances.
|
||||
|
||||
**Unified parent model.** Every parent — planning *or* improvement — flows
|
||||
`… → WaitingForChildren → WaitingForReview → Done` via the single `TryAdvanceParentAsync`.
|
||||
|
||||
+2
-2
@@ -594,7 +594,7 @@ public sealed class ExternalMcpService
|
||||
break;
|
||||
|
||||
case TaskStatus.Cancelled:
|
||||
var cancelResult = await _state.CancelAsync(taskId, DateTime.UtcNow, cancellationToken, allowFromIdle: true);
|
||||
var cancelResult = await _state.CancelAsync(taskId, DateTime.UtcNow, cancellationToken, retire: true);
|
||||
if (!cancelResult.Ok)
|
||||
throw new InvalidOperationException(cancelResult.Reason ?? "Cannot cancel task.");
|
||||
break;
|
||||
@@ -611,7 +611,7 @@ public sealed class ExternalMcpService
|
||||
"Task has an active worktree — use review_task to approve and merge instead.");
|
||||
}
|
||||
|
||||
var doneResult = await _state.ForceSetStatusAsync(taskId, TaskStatus.Done, cancellationToken);
|
||||
var doneResult = await _state.MarkDoneAsync(taskId, DateTime.UtcNow, cancellationToken);
|
||||
if (!doneResult.Ok)
|
||||
throw new InvalidOperationException(doneResult.Reason ?? "Cannot set task to Done.");
|
||||
break;
|
||||
|
||||
@@ -693,14 +693,15 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub
|
||||
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;
|
||||
// Idle goes through ResetToIdleAsync so StartedAt/FinishedAt/Result are cleared;
|
||||
// other statuses keep the unconditional "set status freely" affordance.
|
||||
// Every status routes through its guarded transition; the rest (Running, the Waiting*
|
||||
// states) are only reachable through the lifecycle itself.
|
||||
var result = parsed switch
|
||||
{
|
||||
TaskStatus.Queued => await _state.EnqueueAsync(taskId, Context.ConnectionAborted),
|
||||
TaskStatus.Idle => await _state.ResetToIdleAsync(taskId, Context.ConnectionAborted),
|
||||
_ => await _state.ForceSetStatusAsync(taskId, parsed, Context.ConnectionAborted),
|
||||
TaskStatus.Done => await _state.MarkDoneAsync(taskId, DateTime.UtcNow, Context.ConnectionAborted),
|
||||
TaskStatus.Cancelled => await _state.CancelAsync(taskId, DateTime.UtcNow, Context.ConnectionAborted, retire: true),
|
||||
_ => throw new HubException($"status {parsed} cannot be set by hand"),
|
||||
};
|
||||
if (!result.Ok) throw new HubException(result.Reason ?? "set status failed");
|
||||
return new SetTaskStatusResultDto(
|
||||
|
||||
@@ -765,7 +765,7 @@ public sealed class TaskMergeService
|
||||
await new WorktreeRepository(ctx).SetStateAsync(taskId, WorktreeState.Kept, ct);
|
||||
}
|
||||
await _broadcaster.WorktreeUpdated(taskId);
|
||||
await _state.ForceSetStatusAsync(taskId, TaskStatus.WaitingForReview, ct);
|
||||
await _state.ReopenForReviewAsync(taskId, ct);
|
||||
|
||||
_logger.LogInformation(
|
||||
"Reverted merge of task {TaskId} (merge commit {MergeSha}) via revert commit {RevertSha}",
|
||||
|
||||
@@ -11,7 +11,7 @@ public interface ITaskStateService
|
||||
Task<TransitionResult> FailAsync(
|
||||
string taskId, DateTime finishedAt, string? error, CancellationToken ct,
|
||||
string failureReason = "error", int? turnsUsed = null, int? maxTurns = null);
|
||||
Task<TransitionResult> CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false);
|
||||
Task<TransitionResult> CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool retire = false);
|
||||
Task<TransitionResult> ResetToIdleAsync(string taskId, CancellationToken ct);
|
||||
|
||||
Task<TransitionResult> ApproveReviewAsync(string taskId, CancellationToken ct);
|
||||
@@ -19,9 +19,9 @@ public interface ITaskStateService
|
||||
Task<TransitionResult> RejectToIdleAsync(string taskId, CancellationToken ct);
|
||||
Task<TransitionResult> ClearReviewFeedbackAsync(string taskId, CancellationToken ct);
|
||||
|
||||
Task<TransitionResult> ForceSetStatusAsync(string taskId, ClaudeDo.Data.Models.TaskStatus status, CancellationToken ct);
|
||||
Task<TransitionResult> ReopenForReviewAsync(string taskId, CancellationToken ct);
|
||||
|
||||
// Guarded "manual done toggle" affordance (task list checkbox): unlike ForceSetStatusAsync,
|
||||
// Guarded "manual done toggle" affordance (task list checkbox):
|
||||
// these check the expected starting status in the same ExecuteUpdateAsync filter so a
|
||||
// concurrent picker claim (Idle/Done -> Running) is not silently overwritten.
|
||||
Task<TransitionResult> MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct);
|
||||
|
||||
@@ -297,13 +297,13 @@ public sealed class TaskStateService : ITaskStateService
|
||||
return new TransitionResult(true, null);
|
||||
}
|
||||
|
||||
// allowFromIdle: only the external update_task_status(Cancelled) path sets this — it lets
|
||||
// an Idle task be retired without deleting it. Every other caller (hub CancelReview,
|
||||
// PlanningChainCoordinator's chain-walk, batch cancel) relies on Idle staying a no-op here;
|
||||
// PlanningChainCoordinator specifically uses "parked back to Idle" as a deliberate opt-out
|
||||
// signal, so do not flip this default.
|
||||
// retire: only the user-facing "mark cancelled" paths (update_task_status, the row's
|
||||
// "Mark as" menu) set this — it lets an Idle or Failed task be retired without deleting it.
|
||||
// Every other caller (hub CancelReview, PlanningChainCoordinator's chain-walk, batch cancel)
|
||||
// relies on Idle staying a no-op here; PlanningChainCoordinator specifically uses "parked
|
||||
// back to Idle" as a deliberate opt-out signal, so do not flip this default.
|
||||
public async Task<TransitionResult> CancelAsync(
|
||||
string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false)
|
||||
string taskId, DateTime finishedAt, CancellationToken ct, bool retire = false)
|
||||
{
|
||||
// A unit merge drains its children's worktrees onto the target branch while the parent
|
||||
// sits in WaitingForReview for the whole (potentially minutes-long) drain. Cancelling out
|
||||
@@ -321,7 +321,7 @@ public sealed class TaskStateService : ITaskStateService
|
||||
(t.Status == TaskStatus.Running || t.Status == TaskStatus.Queued
|
||||
|| t.Status == TaskStatus.WaitingForReview
|
||||
|| t.Status == TaskStatus.WaitingForChildren
|
||||
|| (allowFromIdle && t.Status == TaskStatus.Idle)))
|
||||
|| (retire && (t.Status == TaskStatus.Idle || t.Status == TaskStatus.Failed))))
|
||||
.ExecuteUpdateAsync(s => s
|
||||
.SetProperty(t => t.Status, TaskStatus.Cancelled)
|
||||
.SetProperty(t => t.FinishedAt, finishedAt), ct);
|
||||
@@ -381,35 +381,26 @@ public sealed class TaskStateService : ITaskStateService
|
||||
return new TransitionResult(true, null);
|
||||
}
|
||||
|
||||
// Unconditional status write — bypasses transition rules. Used by the UI's
|
||||
// "set status freely" affordance; intentionally no guards (caller may strand
|
||||
// the runner if used while a task is executing). It also bypasses chain/parent
|
||||
// advancement — forcing a chain child or a WaitingForChildren parent's child to a
|
||||
// terminal status here does not unblock its successor or re-check the parent.
|
||||
public async Task<TransitionResult> ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct)
|
||||
/// <summary>Done/WaitingForReview → WaitingForReview after a reverted merge, so the task can be re-approved.</summary>
|
||||
public async Task<TransitionResult> ReopenForReviewAsync(string taskId, CancellationToken ct)
|
||||
{
|
||||
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
||||
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)
|
||||
.ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, status), ct);
|
||||
var affected = await ctx.Tasks
|
||||
.Where(t => t.Id == taskId &&
|
||||
(t.Status == TaskStatus.Done || t.Status == TaskStatus.WaitingForReview))
|
||||
.ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, TaskStatus.WaitingForReview), ct);
|
||||
|
||||
if (affected == 0)
|
||||
return new TransitionResult(false, "Task not found.");
|
||||
return new TransitionResult(false, "Task is not Done or waiting for review; cannot reopen.");
|
||||
|
||||
if (status == TaskStatus.Queued) _waker.Wake();
|
||||
await NotifyAsync(taskId);
|
||||
return new TransitionResult(true, null);
|
||||
}
|
||||
|
||||
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
|
||||
@@ -423,7 +414,9 @@ public sealed class TaskStateService : ITaskStateService
|
||||
|
||||
if (affected == 0)
|
||||
return new TransitionResult(false, "Task is running or no longer exists; cannot mark done.");
|
||||
}
|
||||
|
||||
await OnChildTerminalAsync(taskId, TaskStatus.Done);
|
||||
await NotifyAsync(taskId);
|
||||
return new TransitionResult(true, null);
|
||||
}
|
||||
|
||||
@@ -895,12 +895,12 @@ file sealed class ApproveObservingTaskStateService : ITaskStateService
|
||||
public Task<TransitionResult> SubmitForChildrenAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForChildrenAsync(taskId, finishedAt, result, ct);
|
||||
public Task<TransitionResult> FailAsync(string taskId, DateTime finishedAt, string? error, CancellationToken ct, string failureReason = "error", int? turnsUsed = null, int? maxTurns = null)
|
||||
=> _inner.FailAsync(taskId, finishedAt, error, ct, failureReason, turnsUsed, maxTurns);
|
||||
public Task<TransitionResult> CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false) => _inner.CancelAsync(taskId, finishedAt, ct, allowFromIdle);
|
||||
public Task<TransitionResult> CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool retire = false) => _inner.CancelAsync(taskId, finishedAt, ct, retire);
|
||||
public Task<TransitionResult> ResetToIdleAsync(string taskId, CancellationToken ct) => _inner.ResetToIdleAsync(taskId, ct);
|
||||
public Task<TransitionResult> RejectToQueueAsync(string taskId, string feedback, CancellationToken ct) => _inner.RejectToQueueAsync(taskId, feedback, ct);
|
||||
public Task<TransitionResult> RejectToIdleAsync(string taskId, CancellationToken ct) => _inner.RejectToIdleAsync(taskId, ct);
|
||||
public Task<TransitionResult> ClearReviewFeedbackAsync(string taskId, CancellationToken ct) => _inner.ClearReviewFeedbackAsync(taskId, ct);
|
||||
public Task<TransitionResult> ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) => _inner.ForceSetStatusAsync(taskId, status, ct);
|
||||
public Task<TransitionResult> ReopenForReviewAsync(string taskId, CancellationToken ct) => _inner.ReopenForReviewAsync(taskId, ct);
|
||||
public Task<TransitionResult> MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct) => _inner.MarkDoneAsync(taskId, finishedAt, ct);
|
||||
public Task<TransitionResult> UnmarkDoneAsync(string taskId, CancellationToken ct) => _inner.UnmarkDoneAsync(taskId, ct);
|
||||
public Task<TransitionResult> DequeueToIdleAsync(string taskId, CancellationToken ct) => _inner.DequeueToIdleAsync(taskId, ct);
|
||||
|
||||
@@ -534,6 +534,58 @@ public sealed class TaskStateServiceTests : IDisposable
|
||||
Assert.Equal(TaskStatus.Done, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task MarkDoneAsync_OnChainChild_UnblocksSuccessor()
|
||||
{
|
||||
var parent = await SeedTaskAsync(TaskStatus.WaitingForChildren, phase: PlanningPhase.Finalized);
|
||||
var c0 = await SeedTaskAsync(TaskStatus.Queued, parentId: parent, sortOrder: 0);
|
||||
var c1 = await SeedTaskAsync(TaskStatus.Queued, parentId: parent, sortOrder: 1, blockedBy: c0);
|
||||
|
||||
await _sut.MarkDoneAsync(c0, DateTime.UtcNow, default);
|
||||
|
||||
Assert.Null((await GetTaskAsync(c1)).BlockedByTaskId);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task MarkDoneAsync_OnLastOpenChild_AdvancesParentToReview()
|
||||
{
|
||||
var parent = await SeedTaskAsync(TaskStatus.WaitingForChildren, phase: PlanningPhase.Finalized);
|
||||
await SeedTaskAsync(TaskStatus.Done, parentId: parent);
|
||||
var open = await SeedTaskAsync(TaskStatus.Failed, parentId: parent);
|
||||
|
||||
await _sut.MarkDoneAsync(open, DateTime.UtcNow, default);
|
||||
|
||||
Assert.Equal(TaskStatus.WaitingForReview, await GetStatusAsync(parent));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(TaskStatus.Done, true)]
|
||||
[InlineData(TaskStatus.WaitingForReview, true)]
|
||||
[InlineData(TaskStatus.Idle, false)]
|
||||
[InlineData(TaskStatus.Running, false)]
|
||||
public async Task ReopenForReviewAsync_OnlyFromDoneOrReview(TaskStatus from, bool ok)
|
||||
{
|
||||
var id = await SeedTaskAsync(from);
|
||||
|
||||
var result = await _sut.ReopenForReviewAsync(id, default);
|
||||
|
||||
Assert.Equal(ok, result.Ok);
|
||||
Assert.Equal(ok ? TaskStatus.WaitingForReview : from, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(TaskStatus.Idle)]
|
||||
[InlineData(TaskStatus.Failed)]
|
||||
public async Task CancelAsync_Retire_AllowsIdleAndFailed(TaskStatus from)
|
||||
{
|
||||
var id = await SeedTaskAsync(from);
|
||||
|
||||
var result = await _sut.CancelAsync(id, DateTime.UtcNow, default, retire: true);
|
||||
|
||||
Assert.True(result.Ok);
|
||||
Assert.Equal(TaskStatus.Cancelled, await GetStatusAsync(id));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task UnmarkDoneAsync_FromDone_TransitionsToIdle()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user