diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs index 7c9efc7a..2591ee6f 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs @@ -769,12 +769,16 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable // A predecessor missing from both rowsById and the external cache counts as unmet -- mirrors // QueuePicker/Worker's TaskBlocking.Unmet: an edge pointing at an unresolved/vanished task - // keeps blocking rather than being silently treated as satisfied. + // keeps blocking rather than being silently treated as satisfied. A landed merge satisfies + // the edge too (skipVerify parks a merged task in WaitingForReview until the batch verify); + // the external cache carries no worktree state, so a cross-list predecessor still shows as + // waiting until it reaches Done. private static bool IsUnmet( string id, Dictionary rowsById, IReadOnlyDictionary external) { - if (rowsById.TryGetValue(id, out var p)) return p.Status != TaskStatus.Done; + if (rowsById.TryGetValue(id, out var p)) + return p.Status != TaskStatus.Done && p.WorktreeState != WorktreeState.Merged; if (external.TryGetValue(id, out var e)) return e.Status != TaskStatus.Done; return true; } diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index d3e9c139..2603b18b 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -51,7 +51,7 @@ subfolder within their area; the namespace stays the area namespace. - **Program.cs** — loads config, inits schema, registers DI, configures SignalR on `/hub`, binds to `127.0.0.1:47821` - **TaskStateService** — the **only** component that writes `Status`, `PlanningPhase`, `BlockedByTaskId`, and the `task_dependencies` edges. All transitions return a `TransitionResult` (no exceptions on invalid moves). Wakes the queue and advances the planning chain on child terminal transitions. `SubmitForChildrenAsync` (Running → WaitingForChildren) also chains and immediately queues a normal parent's own Idle/Queued children via `PlanningChainCoordinator.SetupChainAsync(enqueue: true)` — unlike a planning parent (`PlanningPhase != None`), whose children stay untouched here and wait for the user's explicit "Queue plan" (`QueuePlanAsync`); a child already `Running` makes `SetupChainAsync` throw, which is caught and logged rather than failing the parent's own transition. Every transition broadcasts via a private `NotifyAsync(taskId)` — never call `_broadcaster.TaskUpdated` directly from this class, `NotifyAsync` also drives `TicketStatusSync.SyncAsync` (see Tickets below) and a direct call skips that transition's ticket write-back. `SetDependsOnAsync` replaces the task's whole dependency **set** (empty list clears it) and rejects a self-reference, an unknown dependency id, or any link that would create a cycle — a BFS over the whole `task_dependencies` edge set from every proposed predecessor, so a multi-edge loop (A→B→C→A) is caught too, and one bad edge leaves the entire set unwritten. -- **IQueueWaker / IQueuePicker / QueueService** — waker is a singleton `SemaphoreSlim`; picker performs the atomic `Queued → Running` claim filtered by `BlockedByTaskId IS NULL`, `is_manual = 0`, schedule, and "no `task_dependencies` edge whose predecessor isn't `done`" (doubly negated on purpose: zero edges stays eligible, all edges must be done, and an edge pointing at a vanished task keeps blocking); QueueService is a thin `BackgroundService` looping on the waker, dispatching via `TaskRunner`. Per tick it also applies the usage throttle and gate → [usage-monitoring](../../docs/explore-notes/usage-monitoring.md). +- **IQueueWaker / IQueuePicker / QueueService** — waker is a singleton `SemaphoreSlim`; picker performs the atomic `Queued → Running` claim filtered by `BlockedByTaskId IS NULL`, `is_manual = 0`, schedule, and "no `task_dependencies` edge whose predecessor isn't satisfied" — satisfied = `status = 'done'` **or** a landed merge (`worktrees.state = 'merged'`), so a predecessor merged with `skipVerify` and parked in `WaitingForReview` until the batch `verify_merges` no longer stalls its successors (doubly negated on purpose: zero edges stays eligible, all edges must be satisfied, and an edge pointing at a vanished task keeps blocking); QueueService is a thin `BackgroundService` looping on the waker, dispatching via `TaskRunner`. Per tick it also applies the usage throttle and gate → [usage-monitoring](../../docs/explore-notes/usage-monitoring.md). - **RunCancellationRegistry** — taskId → running-run CTS. Lets `TaskStateService.CancelAsync` kill a cancelled task's process without a DI cycle. - **OverrideSlotService** — owns `RunNow` / `ContinueTask`; goes through `TaskStateService.StartRunningAsync` (caller-driven, serialized by slot lock). - **StaleTaskRecovery** — startup-only; calls `TaskStateService.RecoverStaleRunningAsync` to flip orphaned `Running` rows to `Failed`. @@ -67,7 +67,7 @@ not conflated. | `Status` | `Idle`, `Queued`, `Running`, `WaitingForChildren`, `WaitingForReview`, `Done`, `Failed`, `Cancelled` | Lifecycle only. `WaitingForChildren` = parent's own work done, waiting on children. | | `PlanningPhase` | `None`, `Active`, `Finalized` | Parent-only marker. `Active` ≈ legacy `Planning`; `Finalized` ≈ legacy `Planned`. | | `BlockedByTaskId` | nullable FK | Replaces legacy `Waiting`. A queued row with a non-null value is skipped by the picker. Internal to `PlanningChainCoordinator` — resolves (or cascades) on ANY terminal state of the predecessor. Because it self-resolves, `wait_for_task_change` deliberately keeps **waiting** through it instead of reporting `"Blocked"`. | -| `Dependencies` | 0..n rows in `task_dependencies` | User/MCP-declared predecessors (`add_task`/`update_task`), separate from `BlockedByTaskId` because the semantics differ: the picker skips a queued row until **every** dependency reached `Done` -- a Failed/Cancelled dependency does **not** cascade or auto-resolve, the dependent just stays blocked (see `QueuePicker`, `TaskStateService.SetDependsOnAsync`). `get_task`/`list_tasks`/`batch_get_tasks` surface this as `dependsOnTaskIds` + `blocked`/`blockedReason` (the reason names **all** open predecessors, via `External/TaskBlocking`); `wait_for_task_change` reports `"Blocked"` instead of silently running out its timeout. | +| `Dependencies` | 0..n rows in `task_dependencies` | User/MCP-declared predecessors (`add_task`/`update_task`), separate from `BlockedByTaskId` because the semantics differ: the picker skips a queued row until **every** dependency's merge has landed — `Done`, or `worktrees.state = 'merged'` while the batch `verify_merges` is still pending. A Failed/Cancelled dependency does **not** cascade or auto-resolve, the dependent just stays blocked; `revert_merge`/`abort_merge` put the worktree back to `Active` and re-block the successors; a failing `verify_merges` does **not** (the code is on the target branch either way). See `QueuePicker`, `TaskStateService.SetDependsOnAsync`. `get_task`/`list_tasks`/`batch_get_tasks` surface this as `dependsOnTaskIds` + `blocked`/`blockedReason` (the reason names **all** open predecessors, via `External/TaskBlocking`); `wait_for_task_change` reports `"Blocked"` instead of silently running out its timeout. | | `IsManual` | bool | Reminder only the user can do. `EnqueueAsync`/`StartRunningAsync` refuse it, the picker skips it, `GetDailyPrepCandidates` never offers it. An interactive ConPTY session is still allowed. | | `ReviewFeedback` | nullable string | Reviewer's rejection comment; consumed and cleared by `QueueService` on the next re-run. | diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 282eee25..c2240667 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -216,14 +216,19 @@ public sealed class ExternalMcpService .Distinct() .ToList(); - var dependencyStatuses = new Dictionary(); + var predecessors = new Dictionary(); if (dependencyIds.Count > 0) { await using var ctx = await _dbFactory.CreateDbContextAsync(ct); - dependencyStatuses = await ctx.Tasks.AsNoTracking() + predecessors = await ctx.Tasks.AsNoTracking() .Where(t => dependencyIds.Contains(t.Id)) - .Select(t => new { t.Id, t.Status }) - .ToDictionaryAsync(t => t.Id, t => t.Status, ct); + .Select(t => new + { + t.Id, + t.Status, + Merged = t.Worktree != null && t.Worktree.State == WorktreeState.Merged, + }) + .ToDictionaryAsync(t => t.Id, t => (t.Status, t.Merged), ct); } var result = new Dictionary(); @@ -239,7 +244,7 @@ public sealed class ExternalMcpService } else { - var unmet = TaskBlocking.Unmet(t.DependsOnTaskIds, dependencyStatuses); + var unmet = TaskBlocking.Unmet(t.DependsOnTaskIds, predecessors); result[t.Id] = unmet.Count == 0 ? (false, null) : (true, TaskBlocking.Describe(unmet)); } } @@ -290,13 +295,14 @@ public sealed class ExternalMcpService "for normal coding, 'opus' only for complex or cross-cutting work. null inherits the " + "list/global default (normally sonnet).")] string? model = null, - [Description("Id of a task that must reach Done (i.e. be merged) before the picker will claim this one, " + - "even once queued. Shorthand for a one-element dependsOnTaskIds; ignored when that is given. " + + [Description("Id of a task whose merge must have landed before the picker will claim this one, even once " + + "queued (Done, or merged and still awaiting the batch verify). Shorthand for a " + + "one-element dependsOnTaskIds; ignored when that is given. " + "Rejected if it doesn't exist, is this task's own id, or would create a " + "dependency cycle. If that predecessor instead ends up Failed or Cancelled, this task simply " + "stays blocked rather than starving silently -- check get_task/list_tasks' blocked field.")] string? dependsOnTaskId = null, - [Description("Ids of ALL tasks that must reach Done before the picker will claim this one. Use this when a " + + [Description("Ids of ALL tasks whose merges must have landed before the picker will claim this one. Use this when a " + "task wires together, or verifies, the results of several independent tasks -- one waiting on " + "A and B is the normal shape of a wave-style plan. Same rejection rules as dependsOnTaskId.")] string[]? dependsOnTaskIds = null, @@ -460,12 +466,13 @@ public sealed class ExternalMcpService string? title = null, string? description = null, string? commitType = null, - [Description("Id of a task that must reach Done before the picker will claim this one. Shorthand for a " + + [Description("Id of a task whose merge must have landed (Done, or merged and still awaiting the batch " + + "verify) before the picker will claim this one. Shorthand for a " + "one-element dependsOnTaskIds; ignored when that is given. Pass an empty " + "string to clear all existing links; null leaves them unchanged. Rejected if it doesn't exist, " + "is this task's own id, or would create a dependency cycle.")] string? dependsOnTaskId = null, - [Description("Ids of ALL tasks that must reach Done before the picker will claim this one, replacing the " + + [Description("Ids of ALL tasks whose merges must have landed before the picker will claim this one, replacing the " + "task's current set. Pass an empty array to clear it; null leaves it unchanged. Use this when " + "a task wires together, or verifies, the results of several independent tasks.")] string[]? dependsOnTaskIds = null, diff --git a/src/ClaudeDo.Worker/External/TaskBlocking.cs b/src/ClaudeDo.Worker/External/TaskBlocking.cs index 16cb1817..c056a549 100644 --- a/src/ClaudeDo.Worker/External/TaskBlocking.cs +++ b/src/ClaudeDo.Worker/External/TaskBlocking.cs @@ -10,19 +10,22 @@ namespace ClaudeDo.Worker.External; internal static class TaskBlocking { /// - /// The predecessors of that have not reached Done — an id - /// missing from counts as unmet (the picker treats a dangling - /// edge as blocking too). + /// The predecessors of the picker would still skip on: not + /// Done and without a landed merge. Mirrors QueuePicker's SQL predicate — a + /// task merged with skipVerify sits in WaitingForReview until the batch verify runs, + /// yet its code is on the target branch, so it no longer blocks. An id missing from + /// counts as unmet (a dangling edge blocks there too). /// public static List<(string Id, TaskStatus? Status)> Unmet( - IEnumerable dependencyIds, IReadOnlyDictionary statuses) + IEnumerable dependencyIds, + IReadOnlyDictionary predecessors) { var unmet = new List<(string Id, TaskStatus? Status)>(); foreach (var id in dependencyIds) { - if (statuses.TryGetValue(id, out var status)) + if (predecessors.TryGetValue(id, out var p)) { - if (status != TaskStatus.Done) unmet.Add((id, status)); + if (p.Status != TaskStatus.Done && !p.Merged) unmet.Add((id, p.Status)); } else { diff --git a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs index 5501fd3d..58097464 100644 --- a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs +++ b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs @@ -1,5 +1,6 @@ using System.ComponentModel; using ClaudeDo.Data; +using ClaudeDo.Data.Models; using ClaudeDo.Data.Repositories; using Microsoft.EntityFrameworkCore; using ModelContextProtocol; @@ -152,12 +153,17 @@ public sealed class TaskWaitMcpTools .SelectMany(r => r.DependsOnTaskIds) .Distinct() .ToList(); - var dependencyStatuses = dependencyIds.Count == 0 - ? new Dictionary() + var predecessors = dependencyIds.Count == 0 + ? new Dictionary() : await ctx.Tasks.AsNoTracking() .Where(t => dependencyIds.Contains(t.Id)) - .Select(t => new { t.Id, t.Status }) - .ToDictionaryAsync(t => t.Id, t => t.Status, ct); + .Select(t => new + { + t.Id, + t.Status, + Merged = t.Worktree != null && t.Worktree.State == WorktreeState.Merged, + }) + .ToDictionaryAsync(t => t.Id, t => (t.Status, t.Merged), ct); var result = new List(); foreach (var id in taskIds) @@ -175,7 +181,7 @@ public sealed class TaskWaitMcpTools // every chained child and turn the wait into a one-turn-per-poll busy loop. if (row.Status == TaskStatus.Queued && row.BlockedByTaskId is null) { - var unmet = TaskBlocking.Unmet(row.DependsOnTaskIds, dependencyStatuses); + var unmet = TaskBlocking.Unmet(row.DependsOnTaskIds, predecessors); if (unmet.Count > 0) { result.Add(new TaskStatusChangeDto( diff --git a/src/ClaudeDo.Worker/Queue/QueuePicker.cs b/src/ClaudeDo.Worker/Queue/QueuePicker.cs index fdb81088..97fd83a3 100644 --- a/src/ClaudeDo.Worker/Queue/QueuePicker.cs +++ b/src/ClaudeDo.Worker/Queue/QueuePicker.cs @@ -21,11 +21,19 @@ public sealed class QueuePicker : IQueuePicker // Atomic queue claim: UPDATE + RETURNING in a single statement prevents TOCTOU races. // Raw SQL because EF cannot express UPDATE...RETURNING. // Eligible task must be Queued, unblocked (chain and depends-on), not manual, and due - // (or unscheduled). A task_dependencies edge only gates on the dependency's Status='done' - // -- unlike blocked_by_task_id there is no cascade on the dependency failing, so a task - // with a Failed dependency simply stays here, skipped, until someone intervenes. The - // depends-on clause is doubly negated on purpose: "no edge whose predecessor isn't done" - // means a task with zero edges is eligible, ALL edges must be done, and an edge pointing + // (or unscheduled). A task_dependencies edge is satisfied once the predecessor is 'done' + // OR its merge has landed (worktrees.state = 'merged'), because review_task(skipVerify) + // deliberately parks a successfully merged task in WaitingForReview until the batch + // verify_merges runs -- gating on 'done' alone stalled every successor for the whole + // batch. Consequences, all deliberate: a failing verify_merges leaves the merges on the + // target branch, so successors do run (the predecessor's code is there either way); + // revert_merge/abort_merge flip the worktree back to 'active' and re-block; a rejected + // task (reject_rerun/reject_park) never reached 'merged' and stays unmet; a task with no + // worktree (handler task, empty diff) is only ever satisfied via 'done'. Unlike + // blocked_by_task_id there is no cascade on the dependency failing, so a task with a + // Failed dependency simply stays here, skipped, until someone intervenes. The depends-on + // clause is doubly negated on purpose: "no edge whose predecessor isn't satisfied" means + // a task with zero edges is eligible, ALL edges must be satisfied, and an edge pointing // at a vanished task keeps blocking (same as the old single-column form did). // EF SQLite stores DateTime as "yyyy-MM-dd HH:mm:ss.fffffff" — same format used here for comparison. var rows = await ctx.Tasks.FromSqlRaw(""" @@ -41,7 +49,11 @@ public sealed class QueuePicker : IQueuePicker WHERE td.task_id = t.id AND NOT EXISTS ( SELECT 1 FROM tasks d - WHERE d.id = td.depends_on_task_id AND d.status = 'done' + WHERE d.id = td.depends_on_task_id + AND (d.status = 'done' OR EXISTS ( + SELECT 1 FROM worktrees w + WHERE w.task_id = d.id AND w.state = 'merged' + )) ) ) ORDER BY t.sort_order ASC, t.created_at ASC diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 02c1850b..10690c6f 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -472,6 +472,34 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Null(dto.BlockedReason); } + [Fact] + public async Task GetTask_QueuedWithMergedButUnverifiedDependsOn_ReportsBlockedFalse() + { + // Same rule as QueuePicker: a predecessor merged with skipVerify sits in + // WaitingForReview until the batch verify, but the picker already claims the successor -- + // get_task must not contradict that by still calling it blocked. + var listId = await SeedListAsync(); + var predecessor = await SeedTaskAsync(listId, status: TaskStatus.WaitingForReview); + _ctx.Worktrees.Add(new WorktreeEntity + { + TaskId = predecessor.Id, + Path = $"/tmp/{predecessor.Id}", + BranchName = $"claudedo/{predecessor.Id}", + BaseCommit = "abc123", + State = WorktreeState.Merged, + CreatedAt = DateTime.UtcNow, + }); + await _ctx.SaveChangesAsync(); + var task = await SeedTaskAsync(listId, status: TaskStatus.Queued); + await _ctx.SeedDependsOnAsync(task.Id, predecessor.Id); + var sut = BuildSut(CreateQueue()); + + var dto = await sut.GetTask(task.Id, CancellationToken.None); + + Assert.False(dto.Blocked); + Assert.Null(dto.BlockedReason); + } + [Fact] public async Task GetTask_IdleWithDependsOnTaskId_ReportsBlockedFalse_NotYetQueued() { diff --git a/tests/ClaudeDo.Worker.Tests/Queue/QueuePickerTests.cs b/tests/ClaudeDo.Worker.Tests/Queue/QueuePickerTests.cs index d1dcf9f2..458c5f6e 100644 --- a/tests/ClaudeDo.Worker.Tests/Queue/QueuePickerTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Queue/QueuePickerTests.cs @@ -138,6 +138,74 @@ public sealed class QueuePickerTests : IDisposable Assert.Equal(dependent.Id, picked!.Id); } + private async Task SeedWorktreeAsync(string taskId, WorktreeState state) + { + _ctx.Worktrees.Add(new WorktreeEntity + { + TaskId = taskId, + Path = $"/tmp/{taskId}", + BranchName = $"claudedo/{taskId}", + BaseCommit = "abc123", + State = state, + CreatedAt = DateTime.UtcNow, + }); + await _ctx.SaveChangesAsync(); + } + + [Fact] + public async Task ClaimNextAsync_Claims_TaskWhoseDependsOnIsMergedButAwaitingBatchVerify() + { + // review_task(skipVerify: true) leaves a successfully merged task in WaitingForReview + // until verify_merges runs once for the whole batch. Gating on Done alone stalled every + // successor for the entire batch, so a landed merge satisfies the edge. + var listId = await CreateListAsync(); + var dependency = await SeedAsync(listId, status: TaskStatus.WaitingForReview); + await SeedWorktreeAsync(dependency.Id, WorktreeState.Merged); + var dependent = await SeedAsync(listId, dependsOn: [dependency.Id]); + + var picked = await _picker.ClaimNextAsync(DateTime.UtcNow, CancellationToken.None); + + Assert.NotNull(picked); + Assert.Equal(dependent.Id, picked!.Id); + } + + [Fact] + public async Task ClaimNextAsync_Skips_TaskWhoseDependsOnWasRejected() + { + // reject_rerun/reject_park never merge, so the worktree stays Active -- the successor + // must keep waiting even though the predecessor left WaitingForReview. + var listId = await CreateListAsync(); + var dependency = await SeedAsync(listId, status: TaskStatus.Idle); + await SeedWorktreeAsync(dependency.Id, WorktreeState.Active); + await SeedAsync(listId, dependsOn: [dependency.Id]); + + Assert.Null(await _picker.ClaimNextAsync(DateTime.UtcNow, CancellationToken.None)); + } + + [Fact] + public async Task ClaimNextAsync_Skips_TaskWhoseDependsOnMergeWasReverted() + { + // revert_merge / abort_merge put the worktree back to Active -- the edge is unmet again. + var listId = await CreateListAsync(); + var dependency = await SeedAsync(listId, status: TaskStatus.WaitingForReview); + await SeedWorktreeAsync(dependency.Id, WorktreeState.Active); + await SeedAsync(listId, dependsOn: [dependency.Id]); + + Assert.Null(await _picker.ClaimNextAsync(DateTime.UtcNow, CancellationToken.None)); + } + + [Fact] + public async Task ClaimNextAsync_Skips_TaskWhoseWorktreelessDependsOnIsNotDone() + { + // A handler task or an empty-diff run has no worktrees row at all: only 'done' can + // satisfy that edge, and the merged-branch must not accidentally match a missing row. + var listId = await CreateListAsync(); + var dependency = await SeedAsync(listId, status: TaskStatus.WaitingForReview); + await SeedAsync(listId, dependsOn: [dependency.Id]); + + Assert.Null(await _picker.ClaimNextAsync(DateTime.UtcNow, CancellationToken.None)); + } + [Fact] public async Task ClaimNextAsync_Skips_TaskWhoseDependsOnFailed_ButKeepsClaimingOthers() {