fix(mcp): keep wait_for_task_change waiting through a planning-chain block

This commit is contained in:
mika kuns
2026-08-11 08:32:52 +02:00
parent f31b1b4fb2
commit 7106bf754f
3 changed files with 19 additions and 21 deletions
+1 -1
View File
@@ -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. |
+11 -11
View File
@@ -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);
@@ -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]