From b3a2daf40d620334b945e374fb0ecc47e2894c49 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Tue, 9 Jun 2026 11:14:43 +0200 Subject: [PATCH] refactor(worker): single parent-advance path for planning + improvement Collapse TryCompleteParentAsync (planning -> Done) and TryAdvanceImprovementParentAsync (improvement -> WaitingForReview) into one TryAdvanceParentAsync that surfaces any WaitingForChildren parent for review once all children are terminal. Planning parents no longer auto-complete. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ClaudeDo.Data/CLAUDE.md | 2 +- .../Repositories/TaskRepository.cs | 27 ---- src/ClaudeDo.Worker/State/TaskStateService.cs | 22 +-- .../TaskRepositoryParentCompletionTests.cs | 139 ------------------ .../Runner/TaskRunnerParentCompletionTests.cs | 75 ---------- .../WaitingForChildrenLifecycleTests.cs | 34 +++++ 6 files changed, 41 insertions(+), 258 deletions(-) delete mode 100644 tests/ClaudeDo.Worker.Tests/Repositories/TaskRepositoryParentCompletionTests.cs delete mode 100644 tests/ClaudeDo.Worker.Tests/Runner/TaskRunnerParentCompletionTests.cs diff --git a/src/ClaudeDo.Data/CLAUDE.md b/src/ClaudeDo.Data/CLAUDE.md index d479da8..a338987 100644 --- a/src/ClaudeDo.Data/CLAUDE.md +++ b/src/ClaudeDo.Data/CLAUDE.md @@ -19,7 +19,7 @@ Shared data layer: models, repositories, SQLite infrastructure, and git operatio All repositories use EF Core LINQ queries via `ClaudeDoDbContext`. The atomic `Queued -> Running` claim lives in the Worker's `QueuePicker` (uses `FromSqlRaw`), not here. -- **TaskRepository** — CRUD, planning helpers (`CreateChildAsync`, `SetPlanningStartedAsync`, `DiscardPlanningAsync`, `TryCompleteParentAsync`, `UpdateChildAsync`), `UpdateAgentSettingsAsync` (model / system-prompt / agent-path overrides). Status-mutation primitives `MarkRunningAsync` / `MarkDoneAsync` / `MarkFailedAsync` / `FlipAllRunningToFailedAsync` are `internal` and called only by `TaskStateService` in the worker. `CreateChildAsync` produces children with `Status=Idle, PlanningPhase=None`; once their parent's `PlanningPhase` becomes `Finalized`, the chain coordinator queues them. +- **TaskRepository** — CRUD, planning helpers (`CreateChildAsync`, `SetPlanningStartedAsync`, `DiscardPlanningAsync`, `UpdateChildAsync`), `UpdateAgentSettingsAsync` (model / system-prompt / agent-path overrides). Status-mutation primitives `MarkRunningAsync` / `MarkDoneAsync` / `MarkFailedAsync` / `FlipAllRunningToFailedAsync` are `internal` and called only by `TaskStateService` in the worker. `CreateChildAsync` produces children with `Status=Idle, PlanningPhase=None`; once their parent's `PlanningPhase` becomes `Finalized`, the chain coordinator queues them. - **ListRepository** — CRUD, `GetConfigAsync` / `SetConfigAsync` (upsert) / `DeleteConfigAsync` for `list_config` - **WorktreeRepository** — CRUD, `UpdateHeadAsync`, `SetStateAsync` - **TaskRunRepository**, **SubtaskRepository**, **AppSettingsRepository** diff --git a/src/ClaudeDo.Data/Repositories/TaskRepository.cs b/src/ClaudeDo.Data/Repositories/TaskRepository.cs index d034f54..1ca872b 100644 --- a/src/ClaudeDo.Data/Repositories/TaskRepository.cs +++ b/src/ClaudeDo.Data/Repositories/TaskRepository.cs @@ -474,32 +474,5 @@ public sealed class TaskRepository return chainIds.Count; } - public async Task TryCompleteParentAsync( - string parentId, - CancellationToken ct = default) - { - var parent = await _context.Tasks.AsNoTracking().FirstOrDefaultAsync(t => t.Id == parentId, ct); - if (parent is null || parent.PlanningPhase != PlanningPhase.Finalized) return; - - var children = await _context.Tasks - .Where(t => t.ParentTaskId == parentId) - .Select(t => t.Status) - .ToListAsync(ct); - - if (children.Count == 0) return; - - bool allTerminal = children.All(s => s == TaskStatus.Done || s == TaskStatus.Failed); - if (!allTerminal) return; - - bool anyFailed = children.Any(s => s == TaskStatus.Failed); - var finalStatus = anyFailed ? TaskStatus.Failed : TaskStatus.Done; - var finishedAt = DateTime.UtcNow; - await _context.Tasks - .Where(t => t.Id == parentId) - .ExecuteUpdateAsync(s => s - .SetProperty(t => t.Status, finalStatus) - .SetProperty(t => t.FinishedAt, finishedAt), ct); - } - #endregion } diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index d756f20..cea4edc 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -386,28 +386,18 @@ public sealed class TaskStateService : ITaskStateService try { - await using var ctx = await _dbFactory.CreateDbContextAsync(CancellationToken.None); - await new TaskRepository(ctx).TryCompleteParentAsync(parentId, CancellationToken.None); + await TryAdvanceParentAsync(parentId); } catch (Exception ex) { - _logger.LogWarning(ex, "TryCompleteParent failed for {ParentId}", parentId); - } - - try - { - await TryAdvanceImprovementParentAsync(parentId); - } - catch (Exception ex) - { - _logger.LogWarning(ex, "TryAdvanceImprovementParent failed for {ParentId}", parentId); + _logger.LogWarning(ex, "TryAdvanceParent failed for {ParentId}", parentId); } } - // Improvement parents sit in WaitingForChildren while their suggested children run. - // Once every child is terminal (Done/Failed/Cancelled) the parent surfaces for review; - // a failed or cancelled child does not wedge the parent — it is flagged on the result. - private async Task TryAdvanceImprovementParentAsync(string parentId) + // Any parent (planning or improvement) sitting in WaitingForChildren surfaces for review + // once every child is terminal (Done/Failed/Cancelled). A failed or cancelled child does + // not wedge the parent — it is flagged on the result. + private async Task TryAdvanceParentAsync(string parentId) { string? parentResult; List childStatuses; diff --git a/tests/ClaudeDo.Worker.Tests/Repositories/TaskRepositoryParentCompletionTests.cs b/tests/ClaudeDo.Worker.Tests/Repositories/TaskRepositoryParentCompletionTests.cs deleted file mode 100644 index fa7c439..0000000 --- a/tests/ClaudeDo.Worker.Tests/Repositories/TaskRepositoryParentCompletionTests.cs +++ /dev/null @@ -1,139 +0,0 @@ -using ClaudeDo.Data; -using ClaudeDo.Data.Models; -using ClaudeDo.Data.Repositories; -using ClaudeDo.Worker.Tests.Infrastructure; -using Microsoft.EntityFrameworkCore; -using TaskStatus = ClaudeDo.Data.Models.TaskStatus; - -namespace ClaudeDo.Worker.Tests.Repositories; - -public sealed class TaskRepositoryParentCompletionTests : IDisposable -{ - private readonly DbFixture _db = new(); - private readonly ClaudeDoDbContext _ctx; - private readonly TaskRepository _tasks; - private readonly ListRepository _lists; - - public TaskRepositoryParentCompletionTests() - { - _ctx = _db.CreateContext(); - _tasks = new TaskRepository(_ctx); - _lists = new ListRepository(_ctx); - } - - public void Dispose() { _ctx.Dispose(); _db.Dispose(); } - - private async Task ListAsync() - { - var id = Guid.NewGuid().ToString(); - await _lists.AddAsync(new ListEntity { Id = id, Name = "L", CreatedAt = DateTime.UtcNow }); - return id; - } - - private async Task PlannedParentAsync(string listId) - { - var parent = new TaskEntity - { - Id = Guid.NewGuid().ToString(), - ListId = listId, - Title = "p", - Status = TaskStatus.Idle, - PlanningPhase = PlanningPhase.Finalized, - CreatedAt = DateTime.UtcNow, - CommitType = "chore", - }; - await _tasks.AddAsync(parent); - return parent; - } - - private async Task ChildAsync(string listId, string parentId, TaskStatus status) - { - var child = new TaskEntity - { - Id = Guid.NewGuid().ToString(), - ListId = listId, - Title = "c", - Status = status, - CreatedAt = DateTime.UtcNow, - CommitType = "chore", - ParentTaskId = parentId, - }; - await _tasks.AddAsync(child); - return child; - } - - [Fact] - public async Task TryCompleteParentAsync_AllChildrenDone_ParentBecomesDone() - { - var listId = await ListAsync(); - var parent = await PlannedParentAsync(listId); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - - await _tasks.TryCompleteParentAsync(parent.Id); - - var loaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(TaskStatus.Done, loaded!.Status); - Assert.NotNull(loaded.FinishedAt); - } - - [Fact] - public async Task TryCompleteParentAsync_OneFailedRestDone_ParentBecomesFailed() - { - var listId = await ListAsync(); - var parent = await PlannedParentAsync(listId); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - await ChildAsync(listId, parent.Id, TaskStatus.Failed); - - await _tasks.TryCompleteParentAsync(parent.Id); - - var loaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(TaskStatus.Failed, loaded!.Status); - Assert.NotNull(loaded.FinishedAt); - } - - [Fact] - public async Task TryCompleteParentAsync_OneStillRunning_ParentStaysPlanned() - { - var listId = await ListAsync(); - var parent = await PlannedParentAsync(listId); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - await ChildAsync(listId, parent.Id, TaskStatus.Running); - - await _tasks.TryCompleteParentAsync(parent.Id); - - var loaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(TaskStatus.Idle, loaded!.Status); - Assert.Equal(PlanningPhase.Finalized, loaded.PlanningPhase); - Assert.Null(loaded.FinishedAt); - } - - [Fact] - public async Task TryCompleteParentAsync_ChildStillIdle_ParentStaysFinalized() - { - var listId = await ListAsync(); - var parent = await PlannedParentAsync(listId); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - await ChildAsync(listId, parent.Id, TaskStatus.Idle); - - await _tasks.TryCompleteParentAsync(parent.Id); - - var loaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(PlanningPhase.Finalized, loaded!.PlanningPhase); - } - - [Fact] - public async Task TryCompleteParentAsync_ParentIsNotFinalized_NoChange() - { - var listId = await ListAsync(); - var parent = await PlannedParentAsync(listId); - await _ctx.Database.ExecuteSqlRawAsync( - "UPDATE tasks SET planning_phase = 'active' WHERE id = {0}", parent.Id); - await ChildAsync(listId, parent.Id, TaskStatus.Done); - - await _tasks.TryCompleteParentAsync(parent.Id); - - var loaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(PlanningPhase.Active, loaded!.PlanningPhase); - } -} diff --git a/tests/ClaudeDo.Worker.Tests/Runner/TaskRunnerParentCompletionTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/TaskRunnerParentCompletionTests.cs deleted file mode 100644 index ff1d4e6..0000000 --- a/tests/ClaudeDo.Worker.Tests/Runner/TaskRunnerParentCompletionTests.cs +++ /dev/null @@ -1,75 +0,0 @@ -using ClaudeDo.Data; -using ClaudeDo.Data.Models; -using ClaudeDo.Data.Repositories; -using ClaudeDo.Worker.Tests.Infrastructure; -using TaskStatus = ClaudeDo.Data.Models.TaskStatus; - -namespace ClaudeDo.Worker.Tests.Runner; - -public sealed class TaskRunnerParentCompletionTests : IDisposable -{ - private readonly DbFixture _db = new(); - private readonly ClaudeDoDbContext _ctx; - private readonly TaskRepository _tasks; - private readonly ListRepository _lists; - - public TaskRunnerParentCompletionTests() - { - _ctx = _db.CreateContext(); - _tasks = new TaskRepository(_ctx); - _lists = new ListRepository(_ctx); - } - - public void Dispose() { _ctx.Dispose(); _db.Dispose(); } - - [Fact] - public async Task ChildMarkedDone_LastOne_ParentFinalized() - { - var listId = Guid.NewGuid().ToString(); - await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow }); - - var parent = new TaskEntity - { - Id = Guid.NewGuid().ToString(), - ListId = listId, - Title = "p", - Status = TaskStatus.Idle, - PlanningPhase = PlanningPhase.Finalized, - CreatedAt = DateTime.UtcNow, - CommitType = "chore", - }; - await _tasks.AddAsync(parent); - - var c1 = new TaskEntity - { - Id = Guid.NewGuid().ToString(), - ListId = listId, - Title = "c1", - Status = TaskStatus.Done, - CreatedAt = DateTime.UtcNow, - CommitType = "chore", - ParentTaskId = parent.Id, - }; - var c2 = new TaskEntity - { - Id = Guid.NewGuid().ToString(), - ListId = listId, - Title = "c2", - Status = TaskStatus.Running, - CreatedAt = DateTime.UtcNow, - CommitType = "chore", - ParentTaskId = parent.Id, - }; - await _tasks.AddAsync(c1); - await _tasks.AddAsync(c2); - - // Simulate the runner finishing the second child: - await _tasks.MarkDoneAsync(c2.Id, DateTime.UtcNow, "done"); - if (c2.ParentTaskId is not null) - await _tasks.TryCompleteParentAsync(c2.ParentTaskId); - - var parentLoaded = await _tasks.GetByIdAsync(parent.Id); - Assert.Equal(TaskStatus.Done, parentLoaded!.Status); - Assert.NotNull(parentLoaded.FinishedAt); - } -} diff --git a/tests/ClaudeDo.Worker.Tests/WaitingForChildrenLifecycleTests.cs b/tests/ClaudeDo.Worker.Tests/WaitingForChildrenLifecycleTests.cs index 146ca41..8bbb01d 100644 --- a/tests/ClaudeDo.Worker.Tests/WaitingForChildrenLifecycleTests.cs +++ b/tests/ClaudeDo.Worker.Tests/WaitingForChildrenLifecycleTests.cs @@ -126,4 +126,38 @@ public sealed class WaitingForChildrenLifecycleTests : IDisposable var result = await _built.State.EnqueueAsync("kid", default); Assert.False(result.Ok); } + + [Fact] + public async Task SequentialChildren_parent_only_advances_after_both_are_terminal() + { + // Arrange: parent WaitingForChildren, child1 Done, child2 blocked by child1 (Running) + using (var ctx = _db.CreateContext()) + { + ctx.Lists.Add(new ListEntity { Id = "l1", Name = "L", CreatedAt = DateTime.UtcNow }); + ctx.Tasks.Add(new TaskEntity { Id = "par", ListId = "l1", Title = "Parent", + Status = TaskStatus.WaitingForChildren, CreatedAt = DateTime.UtcNow }); + ctx.Tasks.Add(new TaskEntity { Id = "c1", ListId = "l1", Title = "Child1", + Status = TaskStatus.Running, ParentTaskId = "par", CreatedAt = DateTime.UtcNow }); + ctx.Tasks.Add(new TaskEntity { Id = "c2", ListId = "l1", Title = "Child2", + Status = TaskStatus.Running, ParentTaskId = "par", BlockedByTaskId = "c1", + CreatedAt = DateTime.UtcNow }); + await ctx.SaveChangesAsync(); + } + + // Act: complete child1 — parent must stay WaitingForChildren because child2 is still running + await _built.State.CompleteAsync("c1", DateTime.UtcNow, "c1 ok", default); + using (var ctx = _db.CreateContext()) + { + var par = await new TaskRepository(ctx).GetByIdAsync("par"); + Assert.Equal(TaskStatus.WaitingForChildren, par!.Status); + } + + // Act: complete child2 — now all children are terminal; parent must advance + await _built.State.CompleteAsync("c2", DateTime.UtcNow, "c2 ok", default); + using (var ctx = _db.CreateContext()) + { + var par = await new TaskRepository(ctx).GetByIdAsync("par"); + Assert.Equal(TaskStatus.WaitingForReview, par!.Status); + } + } }