fix(worker): block approve of a planning parent with an unfinished child, surface git stderr on ff failure
This commit is contained in:
@@ -23,7 +23,8 @@ There is no separate "Merge all" entry.
|
|||||||
silently skipped there.
|
silently skipped there.
|
||||||
- **Everything else** — a childless task, or a parent whose children were absorbed into its unit
|
- **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
|
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
|
- **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.
|
the unit branch (`UnitBranchService.AbsorbChildAsync`, ff-only) instead of merging to the target.
|
||||||
- **No active worktree** (sandbox run) → straight to `Done`.
|
- **No active worktree** (sandbox run) → straight to `Done`.
|
||||||
|
|||||||
@@ -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
|
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
|
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.
|
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
|
`UnitBranchService` never writes task status (it would close a DI cycle through
|
||||||
`PlanningChainCoordinator`). ⚠️ `QueuePicker` counts `worktrees.state='merged'` as a satisfied
|
`PlanningChainCoordinator`). ⚠️ `QueuePicker` counts `worktrees.state='merged'` as a satisfied
|
||||||
dependency, so an absorbed child satisfies a `dependsOn` edge although nothing reached `main` yet.
|
dependency, so an absorbed child satisfies a `dependsOn` edge although nothing reached `main` yet.
|
||||||
|
|||||||
@@ -1013,6 +1013,20 @@ public sealed class TaskMergeService
|
|||||||
if (task.Status != TaskStatus.WaitingForReview)
|
if (task.Status != TaskStatus.WaitingForReview)
|
||||||
return Blocked("task is not waiting for review");
|
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
|
// 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.
|
// own approve merges the unit. A failed fast-forward keeps the child in review.
|
||||||
if (task.ParentTaskId is not null && _unitBranch is not null)
|
if (task.ParentTaskId is not null && _unitBranch is not null)
|
||||||
|
|||||||
@@ -88,8 +88,9 @@ public sealed class UnitBranchService
|
|||||||
var (code, stderr) = await _git.MergeFastForwardOnlyAsync(parentWt.Path, childWt.BranchName, ct);
|
var (code, stderr) = await _git.MergeFastForwardOnlyAsync(parentWt.Path, childWt.BranchName, ct);
|
||||||
if (code != 0)
|
if (code != 0)
|
||||||
return new AbsorbResult(true,
|
return new AbsorbResult(true,
|
||||||
$"Unit branch '{parentWt.BranchName}' cannot be fast-forwarded to '{childWt.BranchName}' — the child " +
|
$"Unit branch '{parentWt.BranchName}' cannot be fast-forwarded to '{childWt.BranchName}': {stderr.Trim()} " +
|
||||||
$"did not start from the unit's current head (e.g. it was run in parallel via Run now). git: {stderr}");
|
"(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 head = await _git.RevParseHeadAsync(parentWt.Path, ct);
|
||||||
var diffStat = await _git.DiffStatAsync(parentWt.Path, parentWt.BaseCommit, head, ct: ct);
|
var diffStat = await _git.DiffStatAsync(parentWt.Path, parentWt.BaseCommit, head, ct: ct);
|
||||||
|
|||||||
@@ -214,6 +214,7 @@ public sealed class UnitBranchTests : IDisposable
|
|||||||
Assert.Equal(TaskStatus.Failed, child.Status);
|
Assert.Equal(TaskStatus.Failed, child.Status);
|
||||||
Assert.Equal(movedHead, GitRepoFixture.RunGit(_repo.RepoDir, "rev-parse", unit.BranchName).Trim());
|
Assert.Equal(movedHead, GitRepoFixture.RunGit(_repo.RepoDir, "rev-parse", unit.BranchName).Trim());
|
||||||
Assert.Contains("fast-forward", child.Result);
|
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);
|
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);
|
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]
|
[Fact]
|
||||||
public async Task Approving_an_interactive_child_absorbs_it_instead_of_merging_to_main()
|
public async Task Approving_an_interactive_child_absorbs_it_instead_of_merging_to_main()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user