From ff7197c410d03a1543c16d4a001e4ae05772849d Mon Sep 17 00:00:00 2001 From: mika kuns Date: Fri, 2 Oct 2026 16:03:59 +0200 Subject: [PATCH] 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). --- docs/explore-notes/external-mcp.md | 14 ++-- src/ClaudeDo.Worker/CLAUDE.md | 16 +++-- .../External/ExternalMcpService.cs | 4 +- src/ClaudeDo.Worker/Hub/WorkerHub.cs | 9 +-- .../Lifecycle/TaskMergeService.cs | 2 +- .../State/Interfaces/ITaskStateService.cs | 6 +- src/ClaudeDo.Worker/State/TaskStateService.cs | 67 +++++++++---------- .../PlanningMergeOrchestratorTests.cs | 4 +- .../State/TaskStateServiceTests.cs | 52 ++++++++++++++ 9 files changed, 113 insertions(+), 61 deletions(-) diff --git a/docs/explore-notes/external-mcp.md b/docs/explore-notes/external-mcp.md index e668ebce..f6ddfd47 100644 --- a/docs/explore-notes/external-mcp.md +++ b/docs/explore-notes/external-mcp.md @@ -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 diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 134911b2..309c51f1 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -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`. diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 57e41932..da227092 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -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; diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index 9f541949..e5815422 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -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( diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index ded7c337..4b1f31f7 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -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}", diff --git a/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs b/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs index d298c479..1f67f185 100644 --- a/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs +++ b/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs @@ -11,7 +11,7 @@ public interface ITaskStateService Task FailAsync( string taskId, DateTime finishedAt, string? error, CancellationToken ct, string failureReason = "error", int? turnsUsed = null, int? maxTurns = null); - Task CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false); + Task CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool retire = false); Task ResetToIdleAsync(string taskId, CancellationToken ct); Task ApproveReviewAsync(string taskId, CancellationToken ct); @@ -19,9 +19,9 @@ public interface ITaskStateService Task RejectToIdleAsync(string taskId, CancellationToken ct); Task ClearReviewFeedbackAsync(string taskId, CancellationToken ct); - Task ForceSetStatusAsync(string taskId, ClaudeDo.Data.Models.TaskStatus status, CancellationToken ct); + Task 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 MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct); diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index e465d642..0732fe94 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -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 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,49 +381,42 @@ 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 ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) + /// Done/WaitingForReview → WaitingForReview after a reverted merge, so the task can be re-approved. + public async Task 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 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.Running) - .ExecuteUpdateAsync(s => s - .SetProperty(t => t.Status, TaskStatus.Done) - .SetProperty(t => t.FinishedAt, finishedAt), 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.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 running or no longer exists; cannot mark done."); + 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); } diff --git a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs index eb2365ec..e2543762 100644 --- a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs @@ -895,12 +895,12 @@ file sealed class ApproveObservingTaskStateService : ITaskStateService public Task SubmitForChildrenAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForChildrenAsync(taskId, finishedAt, result, ct); public Task 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 CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false) => _inner.CancelAsync(taskId, finishedAt, ct, allowFromIdle); + public Task CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool retire = false) => _inner.CancelAsync(taskId, finishedAt, ct, retire); public Task ResetToIdleAsync(string taskId, CancellationToken ct) => _inner.ResetToIdleAsync(taskId, ct); public Task RejectToQueueAsync(string taskId, string feedback, CancellationToken ct) => _inner.RejectToQueueAsync(taskId, feedback, ct); public Task RejectToIdleAsync(string taskId, CancellationToken ct) => _inner.RejectToIdleAsync(taskId, ct); public Task ClearReviewFeedbackAsync(string taskId, CancellationToken ct) => _inner.ClearReviewFeedbackAsync(taskId, ct); - public Task ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) => _inner.ForceSetStatusAsync(taskId, status, ct); + public Task ReopenForReviewAsync(string taskId, CancellationToken ct) => _inner.ReopenForReviewAsync(taskId, ct); public Task MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct) => _inner.MarkDoneAsync(taskId, finishedAt, ct); public Task UnmarkDoneAsync(string taskId, CancellationToken ct) => _inner.UnmarkDoneAsync(taskId, ct); public Task DequeueToIdleAsync(string taskId, CancellationToken ct) => _inner.DequeueToIdleAsync(taskId, ct); diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index 2e7781ed..699db7f9 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs @@ -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() {