diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index f2c28f70..e8a47bd0 100644 --- a/docs/explore-notes/review-merge.md +++ b/docs/explore-notes/review-merge.md @@ -23,7 +23,8 @@ There is no separate "Merge all" entry. silently skipped there. - **Everything else** — a childless task, or a parent whose children were absorbed into its unit branch → `TaskMergeService.ApproveAndMergeAsync` on the task's own worktree. A conflict keeps - the task in `WaitingForReview`. + the task in `WaitingForReview`. A planning parent with any child not `Done` is `blocked` there + (`subtask X is not Done (status …)`, same as the orchestrator) — no half plan on the target. - **Child of a unit** (parent has an `Active` worktree) → `ApproveAndMergeAsync` absorbs it into the unit branch (`UnitBranchService.AbsorbChildAsync`, ff-only) instead of merging to the target. - **No active worktree** (sandbox run) → straight to `Done`. diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index fa1735bf..09c8cadf 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -105,6 +105,9 @@ review. The child row goes `Merged` with `MergeCommit` null (so `revert_merge` o whole unit), its worktree and branch are removed, the parent row's `HeadCommit`/`DiffStat` move along. A child that can't fast-forward (e.g. started in parallel via Run now) is `Failed` — no merge fallback; in the approve path it stays `WaitingForReview` with a `blocked` result. +A planning parent is only approvable once **every** child is `Done` — `ApproveAndMergeAsync` +blocks with `subtask X is not Done (status …)`, the orchestrator's rule, so a failed/cancelled child +never lands half a plan on the target (improvement parents have no such gate, as before). `UnitBranchService` never writes task status (it would close a DI cycle through `PlanningChainCoordinator`). ⚠️ `QueuePicker` counts `worktrees.state='merged'` as a satisfied dependency, so an absorbed child satisfies a `dependsOn` edge although nothing reached `main` yet. diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index 9d99bbbd..00481abb 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -1013,6 +1013,20 @@ public sealed class TaskMergeService if (task.Status != TaskStatus.WaitingForReview) return Blocked("task is not waiting for review"); + // A plan lands whole or not at all -- same rule (and message) as PlanningMergeOrchestrator.StartAsync, + // which a unit-branch parent no longer passes through. + if (task.PlanningPhase != PlanningPhase.None) + { + using var ctx = _dbFactory.CreateDbContext(); + var notDone = await ctx.Tasks.AsNoTracking() + .Where(c => c.ParentTaskId == taskId && c.Status != TaskStatus.Done) + .OrderBy(c => c.SortOrder) + .Select(c => new { c.Id, c.Status }) + .FirstOrDefaultAsync(ct); + if (notDone is not null) + return Blocked($"subtask {notDone.Id} is not Done (status {notDone.Status})"); + } + // An interactive child of a unit lands in the unit branch, not on the target; the parent's // own approve merges the unit. A failed fast-forward keeps the child in review. if (task.ParentTaskId is not null && _unitBranch is not null) diff --git a/src/ClaudeDo.Worker/Runner/UnitBranchService.cs b/src/ClaudeDo.Worker/Runner/UnitBranchService.cs index 98d66b32..dd4edb35 100644 --- a/src/ClaudeDo.Worker/Runner/UnitBranchService.cs +++ b/src/ClaudeDo.Worker/Runner/UnitBranchService.cs @@ -88,8 +88,9 @@ public sealed class UnitBranchService var (code, stderr) = await _git.MergeFastForwardOnlyAsync(parentWt.Path, childWt.BranchName, ct); if (code != 0) return new AbsorbResult(true, - $"Unit branch '{parentWt.BranchName}' cannot be fast-forwarded to '{childWt.BranchName}' — the child " + - $"did not start from the unit's current head (e.g. it was run in parallel via Run now). git: {stderr}"); + $"Unit branch '{parentWt.BranchName}' cannot be fast-forwarded to '{childWt.BranchName}': {stderr.Trim()} " + + "(usually the child did not start from the unit's current head, e.g. run in parallel via Run now, " + + "or the unit worktree has local changes)"); var head = await _git.RevParseHeadAsync(parentWt.Path, ct); var diffStat = await _git.DiffStatAsync(parentWt.Path, parentWt.BaseCommit, head, ct: ct); diff --git a/tests/ClaudeDo.Worker.Tests/Runner/UnitBranchTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/UnitBranchTests.cs index a360478e..9fd0af51 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/UnitBranchTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/UnitBranchTests.cs @@ -214,6 +214,7 @@ public sealed class UnitBranchTests : IDisposable Assert.Equal(TaskStatus.Failed, child.Status); Assert.Equal(movedHead, GitRepoFixture.RunGit(_repo.RepoDir, "rev-parse", unit.BranchName).Trim()); Assert.Contains("fast-forward", child.Result); + Assert.Contains("Not possible to fast-forward", child.Result); // git's own stderr is surfaced Assert.Equal(WorktreeState.Active, (await WorktreeOf(Child1))!.State); } @@ -233,6 +234,23 @@ public sealed class UnitBranchTests : IDisposable Assert.Equal(WorktreeState.Merged, (await WorktreeOf(Parent))!.State); } + [Fact] + public async Task Approving_a_planning_parent_with_an_unfinished_child_is_blocked() + { + await _unit.EnsureUnitWorktreeAsync(Parent, default); + await RunChild(Child1, Writes()); + await SetStatus(Child2, TaskStatus.Failed); + await SetStatus(Parent, TaskStatus.WaitingForReview); + + var result = await MergeService().ApproveAndMergeAsync(Parent, "main", default); + + Assert.Equal(TaskMergeService.StatusBlocked, result.Status); + Assert.Equal($"subtask {Child2} is not Done (status Failed)", result.ErrorMessage); + Assert.Equal(TaskStatus.WaitingForReview, (await TaskOf(Parent)).Status); + Assert.Equal(WorktreeState.Active, (await WorktreeOf(Parent))!.State); + Assert.Equal(_repo.BaseCommit, GitRepoFixture.RunGit(_repo.RepoDir, "rev-parse", "main").Trim()); + } + [Fact] public async Task Approving_an_interactive_child_absorbs_it_instead_of_merging_to_main() {