fix(worker): propagate unit-merge failures instead of reporting success
A child merge that came back blocked/verify_failed/untracked_collision during a parent/children unit merge used to vanish: DrainAsync only logged it server-side, PlanningMergeAborted carried no reason, and ApproveReview/review_task always reported StatusMerged for a task with children regardless of the real outcome, so a failed unit merge left the parent stuck with no visible error. - PlanningMergeOrchestrator.StartAsync/ContinueAsync/DrainAsync now return a PlanningMergeResult(Status, Reason) instead of void, and PlanningMergeAborted carries that reason to the UI. - WorkerHub.ApproveReview and ExternalMcpService.ReviewTask's approve branch propagate the real status/reason for a parent with children instead of hardcoding "merged" (or masking a non-conflict failure as "conflict"). - StartAsync now requires the parent to already be WaitingForReview for improvement parents too, not just planning ones, so a stale caller can no longer trigger a partial child merge. - HasActiveMerge now also covers the window between the last child merging and FinalizeParentDoneAsync completing, closing a gap where a concurrent Cancel could race the parent's own approve-to-Done transition. - IslandsShellViewModel.OnPlanningMergeAborted flashes the reason via FlashFooterError instead of only clearing the external-merge banner.
This commit is contained in:
mika kuns
@@ -4,6 +4,7 @@ using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Worker.Hub;
|
||||
using ClaudeDo.Worker.Lifecycle;
|
||||
using ClaudeDo.Worker.Planning;
|
||||
using ClaudeDo.Worker.State;
|
||||
using ClaudeDo.Worker.Tests.Infrastructure;
|
||||
using Microsoft.AspNetCore.SignalR;
|
||||
using Microsoft.Extensions.Logging.Abstractions;
|
||||
@@ -71,7 +72,9 @@ public sealed class PlanningMergeOrchestratorTests : IDisposable
|
||||
|
||||
var (orch, calls) = BuildOrchestrator(db);
|
||||
|
||||
await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
var result = await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
Assert.Equal(TaskMergeService.StatusMerged, result.Status);
|
||||
Assert.Null(result.Reason);
|
||||
|
||||
using var ctx = db.CreateContext();
|
||||
var planning = ctx.Tasks.Single(t => t.Id == parentId);
|
||||
@@ -139,14 +142,16 @@ public sealed class PlanningMergeOrchestratorTests : IDisposable
|
||||
var (parentId, subA, subB, subC) = await SeedPlanningThreeChildrenMiddleConflictsAsync(db, repo);
|
||||
|
||||
var (orch, spy) = BuildOrchestrator(db);
|
||||
await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
var startResult = await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
Assert.Equal(TaskMergeService.StatusConflict, startResult.Status);
|
||||
|
||||
Assert.Contains(spy, c => c.Method == "PlanningSubtaskMerged" && (string)c.Args[1]! == subA);
|
||||
Assert.Contains(spy, c => c.Method == "PlanningMergeConflict" && (string)c.Args[1]! == subB);
|
||||
|
||||
File.WriteAllText(Path.Combine(repo.RepoDir, "README.md"), "resolved\n");
|
||||
|
||||
await orch.ContinueAsync(parentId, CancellationToken.None);
|
||||
var continueResult = await orch.ContinueAsync(parentId, CancellationToken.None);
|
||||
Assert.Equal(TaskMergeService.StatusMerged, continueResult.Status);
|
||||
|
||||
using var ctx = db.CreateContext();
|
||||
Assert.Equal(TaskStatus.Done, ctx.Tasks.Single(t => t.Id == parentId).Status);
|
||||
@@ -608,31 +613,35 @@ public sealed class PlanningMergeOrchestratorTests : IDisposable
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Parent is Cancelled before the orchestrator finalizes (simulates a race where the user
|
||||
/// cancels the parent while the merge drain is in progress). After the drain completes,
|
||||
/// ApproveReviewAsync sees Status != WaitingForReview and refuses — parent must stay
|
||||
/// Cancelled and PlanningCompleted must not be broadcast.
|
||||
/// Guard (a): StartAsync now requires the parent to be WaitingForReview up front, for
|
||||
/// improvement parents as much as planning ones. Before this guard existed, a parent that had
|
||||
/// already left WaitingForReview (e.g. cancelled by a race, or a stale second Approve click)
|
||||
/// still had its children merged during the drain, only to have the final ApproveReviewAsync
|
||||
/// refuse at the very end — by then the child worktrees were already unrecoverably merged.
|
||||
/// The fixed behaviour rejects up front: nothing gets touched.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task StartAsync_ParentCancelledBeforeFinalize_StatusRemainsAndNoPlanningCompleted()
|
||||
public async Task StartAsync_ParentNotWaitingForReview_ThrowsWithoutMergingChildren()
|
||||
{
|
||||
var db = NewDb();
|
||||
var repo = NewRepo();
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
|
||||
|
||||
// Improvement parent (PlanningPhase.None) seeded as Cancelled — simulates the race
|
||||
// where a user or another thread cancelled the parent during the merge drain.
|
||||
// Improvement parent (PlanningPhase.None) seeded as Cancelled.
|
||||
var (parentId, subA, subB) = await SeedCancelledParentWithDoneChildrenAsync(db, repo);
|
||||
|
||||
var (orch, calls) = BuildOrchestrator(db);
|
||||
await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
|
||||
var ex = await Assert.ThrowsAsync<InvalidOperationException>(
|
||||
() => orch.StartAsync(parentId, "main", CancellationToken.None));
|
||||
Assert.Contains("not WaitingForReview", ex.Message);
|
||||
|
||||
using var ctx = db.CreateContext();
|
||||
Assert.Equal(TaskStatus.Cancelled, ctx.Tasks.Single(t => t.Id == parentId).Status);
|
||||
Assert.DoesNotContain(calls, c => c.Method == "PlanningCompleted");
|
||||
// Child worktrees were still merged during the drain
|
||||
Assert.Equal(WorktreeState.Merged, ctx.Worktrees.Single(w => w.TaskId == subA).State);
|
||||
Assert.Equal(WorktreeState.Merged, ctx.Worktrees.Single(w => w.TaskId == subB).State);
|
||||
Assert.Empty(calls);
|
||||
// Children must stay untouched — the guard rejects before any merge is attempted.
|
||||
Assert.Equal(WorktreeState.Active, ctx.Worktrees.Single(w => w.TaskId == subA).State);
|
||||
Assert.Equal(WorktreeState.Active, ctx.Worktrees.Single(w => w.TaskId == subB).State);
|
||||
}
|
||||
|
||||
private async Task<(string parentId, string subA, string subB)> SeedCancelledParentWithDoneChildrenAsync(
|
||||
@@ -677,4 +686,196 @@ public sealed class PlanningMergeOrchestratorTests : IDisposable
|
||||
|
||||
return (parentId, subA, subB);
|
||||
}
|
||||
|
||||
// ─── Unit-merge failure propagation (blocked/verify_failed/untracked_collision) ─────────
|
||||
|
||||
/// <summary>
|
||||
/// A child whose branch has no common history with the target branch makes the underlying
|
||||
/// `git merge --no-ff` refuse outright (no conflict markers at all) — TaskMergeService reports
|
||||
/// this as StatusBlocked, not StatusConflict. Before this fix that outcome vanished: DrainAsync
|
||||
/// only logged it server-side and broadcast a bare PlanningMergeAborted, and ApproveReview
|
||||
/// always returned StatusMerged regardless. Now StartAsync must surface the real status/reason,
|
||||
/// and the broadcast must carry that reason.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task StartAsync_ChildMergeBlocked_ReturnsBlockedResultAndBroadcastsReason()
|
||||
{
|
||||
var db = NewDb();
|
||||
var repo = NewRepo();
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
|
||||
|
||||
var (parentId, _) = await SeedImprovementParentWithOneUnrelatedHistoryChildAsync(db, repo);
|
||||
|
||||
var (orch, spy) = BuildOrchestrator(db);
|
||||
|
||||
var result = await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
|
||||
Assert.Equal(TaskMergeService.StatusBlocked, result.Status);
|
||||
Assert.False(string.IsNullOrWhiteSpace(result.Reason));
|
||||
|
||||
using var ctx = db.CreateContext();
|
||||
// The parent was never finalized — it stays wherever it was (WaitingForReview here).
|
||||
Assert.Equal(TaskStatus.WaitingForReview, ctx.Tasks.Single(t => t.Id == parentId).Status);
|
||||
|
||||
var abortedCall = Assert.Single(spy, c => c.Method == "PlanningMergeAborted");
|
||||
Assert.Equal(parentId, (string)abortedCall.Args[0]!);
|
||||
Assert.Equal(result.Reason, (string?)abortedCall.Args[1]);
|
||||
Assert.DoesNotContain(spy, c => c.Method == "PlanningCompleted");
|
||||
}
|
||||
|
||||
private async Task<(string parentId, string subA)> SeedImprovementParentWithOneUnrelatedHistoryChildAsync(
|
||||
DbFixture db, GitRepoFixture repo)
|
||||
{
|
||||
using var ctx = db.CreateContext();
|
||||
|
||||
var listId = Guid.NewGuid().ToString();
|
||||
ctx.Lists.Add(new ListEntity
|
||||
{
|
||||
Id = listId, Name = "test", CreatedAt = DateTime.UtcNow,
|
||||
WorkingDir = repo.RepoDir,
|
||||
});
|
||||
|
||||
var parentId = Guid.NewGuid().ToString();
|
||||
ctx.Tasks.Add(new TaskEntity
|
||||
{
|
||||
Id = parentId, ListId = listId, Title = "improve", CreatedAt = DateTime.UtcNow,
|
||||
Status = TaskStatus.WaitingForReview, PlanningPhase = PlanningPhase.None, SortOrder = 0,
|
||||
Number = ++_numberSeed,
|
||||
});
|
||||
|
||||
var subA = Guid.NewGuid().ToString();
|
||||
ctx.Tasks.Add(new TaskEntity
|
||||
{
|
||||
Id = subA, ListId = listId, Title = "child A", CreatedAt = DateTime.UtcNow,
|
||||
ParentTaskId = parentId, Status = TaskStatus.Done, SortOrder = 1,
|
||||
Number = ++_numberSeed,
|
||||
});
|
||||
await ctx.SaveChangesAsync();
|
||||
|
||||
SeedWorktreeUnrelatedHistory(ctx, repo, subA, "fileA.txt", "content A");
|
||||
await ctx.SaveChangesAsync();
|
||||
|
||||
return (parentId, subA);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Seeds a worktree on a branch with no common ancestor with the target branch, so
|
||||
/// `git merge --no-ff` refuses with "refusing to merge unrelated histories" — a real,
|
||||
/// deterministic StatusBlocked trigger with no conflict files, as opposed to the
|
||||
/// files.Count > 0 path the other fixtures exercise.
|
||||
/// </summary>
|
||||
private void SeedWorktreeUnrelatedHistory(ClaudeDoDbContext ctx, GitRepoFixture repo, string taskId, string filename, string content)
|
||||
{
|
||||
var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}");
|
||||
_wtCleanups.Add((repo.RepoDir, wtPath));
|
||||
var branch = $"claudedo/{taskId[..8]}";
|
||||
const string emptyTreeSha = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";
|
||||
var orphanRoot = GitRepoFixture.RunGit(repo.RepoDir, "commit-tree", emptyTreeSha, "-m", "orphan root").Trim();
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "branch", branch, orphanRoot);
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", wtPath, branch);
|
||||
File.WriteAllText(Path.Combine(wtPath, filename), content);
|
||||
GitRepoFixture.RunGit(wtPath, "add", filename);
|
||||
GitRepoFixture.RunGit(wtPath, "commit", "-m", $"add {filename}");
|
||||
var head = GitRepoFixture.RunGit(wtPath, "rev-parse", "HEAD").Trim();
|
||||
|
||||
ctx.Worktrees.Add(new WorktreeEntity
|
||||
{
|
||||
TaskId = taskId,
|
||||
Path = wtPath,
|
||||
BranchName = branch,
|
||||
BaseCommit = orphanRoot,
|
||||
HeadCommit = head,
|
||||
DiffStat = null,
|
||||
State = WorktreeState.Active,
|
||||
CreatedAt = DateTime.UtcNow,
|
||||
});
|
||||
}
|
||||
|
||||
// ─── Guard (b): HasActiveMerge must span the finalize window ─────────────────────────────
|
||||
|
||||
/// <summary>
|
||||
/// Once the last child has merged, DrainAsync clears CurrentSubtaskId before calling
|
||||
/// FinalizeParentDoneAsync (which flips the parent to Done). Before this fix, HasActiveMerge
|
||||
/// went false in that window, so TaskStateService.CancelAsync's "a merge is in progress"
|
||||
/// guard stopped protecting the parent for the whole duration of the finalize call. This test
|
||||
/// observes HasActiveMerge from inside ApproveReviewAsync (the call FinalizeParentDoneAsync
|
||||
/// makes) to prove the window is now covered, and that the flag clears once the drain returns.
|
||||
/// </summary>
|
||||
[Fact]
|
||||
public async Task Drain_FinalizeWindow_HasActiveMergeStaysTrueUntilFinalizeCompletes()
|
||||
{
|
||||
var db = NewDb();
|
||||
var repo = NewRepo();
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
|
||||
|
||||
var (parentId, _, _) = await SeedImprovementParentWithTwoDoneChildrenAsync(db, repo);
|
||||
|
||||
var fakeHub = new OrchestratorFakeHubContext();
|
||||
var broadcaster = new HubBroadcaster(fakeHub);
|
||||
var git = new GitService();
|
||||
var factory = db.CreateFactory();
|
||||
var built = TaskStateServiceBuilder.Build(factory);
|
||||
var merge = new TaskMergeService(
|
||||
factory, git, broadcaster, built.State, new VerifyCommandRunner(), NullLogger<TaskMergeService>.Instance);
|
||||
var aggregator = new PlanningAggregator(factory, git, NullLogger<PlanningAggregator>.Instance);
|
||||
|
||||
PlanningMergeOrchestrator? orchRef = null;
|
||||
bool? activeDuringApprove = null;
|
||||
var observingState = new ApproveObservingTaskStateService(built.State, () =>
|
||||
{
|
||||
activeDuringApprove = orchRef!.HasActiveMerge(parentId);
|
||||
});
|
||||
|
||||
var orch = new PlanningMergeOrchestrator(
|
||||
factory, merge, aggregator, broadcaster, git, observingState, NullLogger<PlanningMergeOrchestrator>.Instance);
|
||||
orchRef = orch;
|
||||
|
||||
var result = await orch.StartAsync(parentId, "main", CancellationToken.None);
|
||||
|
||||
Assert.Equal(TaskMergeService.StatusMerged, result.Status);
|
||||
Assert.True(activeDuringApprove, "HasActiveMerge must still be true while FinalizeParentDoneAsync's ApproveReviewAsync runs.");
|
||||
Assert.False(orch.HasActiveMerge(parentId), "state must be cleared once the drain (incl. finalize) fully completes.");
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Test-only decorator that invokes a callback right before delegating
|
||||
/// ApproveReviewAsync — everything else passes straight through to the real service.</summary>
|
||||
file sealed class ApproveObservingTaskStateService : ITaskStateService
|
||||
{
|
||||
private readonly ITaskStateService _inner;
|
||||
private readonly Action _onApprove;
|
||||
|
||||
public ApproveObservingTaskStateService(ITaskStateService inner, Action onApprove)
|
||||
{
|
||||
_inner = inner;
|
||||
_onApprove = onApprove;
|
||||
}
|
||||
|
||||
public Task<TransitionResult> ApproveReviewAsync(string taskId, CancellationToken ct)
|
||||
{
|
||||
_onApprove();
|
||||
return _inner.ApproveReviewAsync(taskId, ct);
|
||||
}
|
||||
|
||||
public Task<TransitionResult> EnqueueAsync(string taskId, CancellationToken ct) => _inner.EnqueueAsync(taskId, ct);
|
||||
public Task<TransitionResult> StartRunningAsync(string taskId, DateTime startedAt, CancellationToken ct) => _inner.StartRunningAsync(taskId, startedAt, ct);
|
||||
public Task<TransitionResult> CompleteAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.CompleteAsync(taskId, finishedAt, result, ct);
|
||||
public Task<TransitionResult> SubmitForReviewAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForReviewAsync(taskId, finishedAt, result, ct);
|
||||
public Task<TransitionResult> SubmitInteractiveForReviewAsync(string taskId, DateTime finishedAt, CancellationToken ct) => _inner.SubmitInteractiveForReviewAsync(taskId, finishedAt, ct);
|
||||
public Task<TransitionResult> SubmitForChildrenAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForChildrenAsync(taskId, finishedAt, result, ct);
|
||||
public Task<TransitionResult> FailAsync(string taskId, DateTime finishedAt, string? error, CancellationToken ct, string failureReason = "error", int? turnsUsed = null, int? maxTurns = null)
|
||||
=> _inner.FailAsync(taskId, finishedAt, error, ct, failureReason, turnsUsed, maxTurns);
|
||||
public Task<TransitionResult> CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false) => _inner.CancelAsync(taskId, finishedAt, ct, allowFromIdle);
|
||||
public Task<TransitionResult> ResetToIdleAsync(string taskId, CancellationToken ct) => _inner.ResetToIdleAsync(taskId, ct);
|
||||
public Task<TransitionResult> RejectToQueueAsync(string taskId, string feedback, CancellationToken ct) => _inner.RejectToQueueAsync(taskId, feedback, ct);
|
||||
public Task<TransitionResult> RejectToIdleAsync(string taskId, CancellationToken ct) => _inner.RejectToIdleAsync(taskId, ct);
|
||||
public Task<TransitionResult> ClearReviewFeedbackAsync(string taskId, CancellationToken ct) => _inner.ClearReviewFeedbackAsync(taskId, ct);
|
||||
public Task<TransitionResult> ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) => _inner.ForceSetStatusAsync(taskId, status, ct);
|
||||
public Task<TransitionResult> StartPlanningAsync(string parentId, CancellationToken ct) => _inner.StartPlanningAsync(parentId, ct);
|
||||
public Task<TransitionResult> FinalizePlanningAsync(string parentId, CancellationToken ct) => _inner.FinalizePlanningAsync(parentId, ct);
|
||||
public Task<TransitionResult> BlockOnAsync(string taskId, string predecessorTaskId, CancellationToken ct) => _inner.BlockOnAsync(taskId, predecessorTaskId, ct);
|
||||
public Task<TransitionResult> UnblockAsync(string taskId, CancellationToken ct) => _inner.UnblockAsync(taskId, ct);
|
||||
public Task<TransitionResult> SetDependsOnAsync(string taskId, string? dependsOnTaskId, CancellationToken ct) => _inner.SetDependsOnAsync(taskId, dependsOnTaskId, ct);
|
||||
public Task TryAdvanceParentAsync(string parentId) => _inner.TryAdvanceParentAsync(parentId);
|
||||
public Task<int> RecoverStaleRunningAsync(string reason, CancellationToken ct) => _inner.RecoverStaleRunningAsync(reason, ct);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user