diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index ebb15cdf..7ad0549e 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -61,7 +61,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. | +| `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"`. | | `DependsOnTaskId` | nullable FK | User/MCP-declared predecessor (`add_task`/`update_task`), separate from `BlockedByTaskId` because the semantics differ: the picker only skips a queued row while the dependency's `Status` isn't `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 `blocked`/`blockedReason`; `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/TaskWaitMcpTools.cs b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs index 6d3da27b..0f6f91fe 100644 --- a/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs +++ b/src/ClaudeDo.Worker/External/TaskWaitMcpTools.cs @@ -44,10 +44,11 @@ public sealed class TaskWaitMcpTools [McpServerTool, Description( "Blocks until at least one of the given tasks leaves Queued/Running -- use this instead of " + "polling get_task in a loop. Returns immediately if a task is already outside Queued/Running " + - "(an unknown id reports status \"NotFound\" and counts as changed). A Queued task the picker " + - "will not claim yet (a planning-chain predecessor, or a depends_on link whose target isn't " + - "Done) also reports immediately as status \"Blocked\" with blockedReason set, instead of " + - "silently waiting out the full timeout. Pitfall: a planning parent goes Running -> " + + "(an unknown id reports status \"NotFound\" and counts as changed). A Queued task held by a " + + "depends_on link whose target isn't Done reports immediately as status \"Blocked\" with " + + "blockedReason set, instead of silently waiting out the full timeout -- that link never " + + "resolves on its own. A planning-chain block does NOT report that way: it clears by itself " + + "when the predecessor finishes, so the wait simply continues. Pitfall: a planning parent goes Running -> " + "WaitingForChildren while its children are still working, so by default waiting on a parent " + "returns early; see treatWaitingForChildrenAsBusy. Sends MCP progress " + "pings every 30s while waiting so a long wait survives the calling client's own idle-silence " + @@ -135,14 +136,13 @@ public sealed class TaskWaitMcpTools continue; } - if (row.Status == TaskStatus.Queued) + // A planning-chain BlockedByTaskId is deliberately NOT reported as "Blocked": it + // resolves on its own the moment the predecessor reaches any terminal state + // (PlanningChainCoordinator), so it's exactly what a caller waiting on a queued + // fan-out wants to keep waiting through. Reporting it would return instantly for + // every chained child and turn the wait into a one-turn-per-poll busy loop. + if (row.Status == TaskStatus.Queued && row.BlockedByTaskId is null) { - if (row.BlockedByTaskId is not null) - { - result.Add(new TaskStatusChangeDto(id, "Blocked", - $"Blocked by planning-chain predecessor {row.BlockedByTaskId}.")); - continue; - } if (row.DependsOnTaskId is not null) { var known = dependencyStatuses.TryGetValue(row.DependsOnTaskId, out var depStatus); diff --git a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs index 4d6f6dd7..80d400f2 100644 --- a/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/TaskWaitMcpToolsTests.cs @@ -198,23 +198,21 @@ public sealed class TaskWaitMcpToolsTests : IDisposable } [Fact] - public async Task WaitForTaskChange_QueuedWithBlockedByTaskId_ReportsBlockedImmediately() + public async Task WaitForTaskChange_QueuedWithBlockedByTaskId_KeepsWaiting_BecauseTheChainResolvesItself() { + // A planning-chain link clears automatically once the predecessor goes terminal, so it + // must NOT short-circuit the wait -- otherwise waiting on a queued fan-out returns + // instantly for every chained child and the caller burns a turn per poll. var predecessor = await SeedTaskAsync(TaskStatus.Queued); var task = await SeedTaskAsync(TaskStatus.Queued); task.BlockedByTaskId = predecessor.Id; await _tasks.UpdateAsync(task); var sut = BuildSut(); - var sw = Stopwatch.StartNew(); - var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 30, cancellationToken: CancellationToken.None); + var result = await sut.WaitForTaskChange([task.Id], timeoutSeconds: 1, cancellationToken: CancellationToken.None); - sw.Stop(); - Assert.False(result.TimedOut); - var change = Assert.Single(result.Changed); - Assert.Equal("Blocked", change.Status); - Assert.Contains(predecessor.Id, change.BlockedReason); - Assert.True(sw.Elapsed < TimeSpan.FromSeconds(2), $"took {sw.Elapsed}"); + Assert.True(result.TimedOut); + Assert.Empty(result.Changed); } [Fact]