Merge claudedo/973ea49ec0a642ea9b1ddcff9b71b6fe

docs/explore-notes/review-merge.md

@@ -1,7 +1,7 @@
 # Review, merge & conflict resolution
 
 > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
-> Last verified against commit `20bce9b` (2026-08-06).
+> Last verified against commit `0d1e3b9` (2026-08-06).
 > Drift check: `git log --oneline 20bce9b..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts src/ClaudeDo.Worker/External`
 > Stable structure only (no line numbers). See docs/explore-notes/README.md.
 
@@ -56,6 +56,31 @@ like any other `Done` child once the parent is approved. The MCP surface has no
 An empty branch is still mergeable by design (some tasks — e.g. an audit — legitimately produce
 no diff); this is a visibility fix, not a merge gate.
 
+## Cancel is blocked while a unit merge is draining
+
+`ApproveReview` on a task with children awaits `PlanningMergeOrchestrator.StartAsync` /
+`DrainAsync` synchronously — the parent sits in `WaitingForReview` for the whole (potentially
+minutes-long, one-child-at-a-time) drain. Without a guard, a concurrent `CancelReview` (UI or
+`update_task_status`/`cancel_task` via MCP) could flip the parent to `Cancelled` mid-drain while
+the orchestrator kept merging children's worktrees onto the target branch; `FinalizeParentDoneAsync`
+then finds the parent no longer `WaitingForReview` and gives up, leaving the merged children's
+diffs stranded with no rollback.
+
+`TaskStateService.CancelAsync` now rejects with a `TransitionResult` reason whenever
+`PlanningMergeOrchestrator.HasActiveMerge(taskId)` is true for the task being cancelled, before any
+DB write. Cycle note: `TaskStateService` can't take a direct constructor dependency on
+`PlanningMergeOrchestrator` (which itself depends on `ITaskStateService`), so it takes a lazily-resolved
+`Func<IActiveMergeState>` instead — same cycle-breaking shape as the existing `Func<ITaskStateService>`
+handed to `PlanningChainCoordinator`. `IActiveMergeState` (`Planning/Interfaces/`) is `PlanningMergeOrchestrator`'s
+only public surface `TaskStateService` needs.
+
+The UI mirrors this as polish: `DetailsIslandViewModel.IsMergeDraining` (set on
+`PlanningMergeStartedEvent`, cleared on `PlanningMergeAborted`/`PlanningCompleted` for the bound
+task) gates `CancelReviewCommand`'s `CanExecute` — same shape as `WorktreesOverviewModalViewModel.IsMerging`
+gating `CanMergeAll`. The worker-side guard remains the actual correctness fix; the button gate
+just avoids inviting a click the worker would reject. `CancelReviewAsync`'s catch now raises
+`ErrorReported` (→ shell `FlashFooterError`) instead of swallowing the rejection silently.
+
 ## Post-merge verify gate
 
 A list can set `ListConfigEntity.VerifyCommand` (List Settings modal → Verification).

src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs

@@ -35,6 +35,9 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
     private readonly Action<string, string, string, DateTime> _workerTaskFinishedHandler;
     private readonly Action<string> _workerWorktreeUpdatedHandler;
     private readonly Action<string> _workerTaskUpdatedHandler;
+    private readonly Action<string, string> _workerPlanningMergeStartedHandler;
+    private readonly Action<string> _workerPlanningMergeAbortedHandler;
+    private readonly Action<string> _workerPlanningCompletedHandler;
 
     [ObservableProperty] private bool _isNotesMode;
     [ObservableProperty] private bool _isPrepMode;
@@ -351,6 +354,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
                 ResetAndRetryCommand.NotifyCanExecuteChanged();
                 ContinueCommand.NotifyCanExecuteChanged();
                 SendRoadblockReplyCommand.NotifyCanExecuteChanged();
+                CancelReviewCommand.NotifyCanExecuteChanged();
             }
         };
         _worker.PropertyChanged += _workerPropertyChangedHandler;
@@ -392,6 +396,24 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
         };
         _worker.TaskUpdatedEvent += _workerTaskUpdatedHandler;
 
+        _workerPlanningMergeStartedHandler = (planningTaskId, _) =>
+        {
+            if (Task?.Id == planningTaskId) IsMergeDraining = true;
+        };
+        _worker.PlanningMergeStartedEvent += _workerPlanningMergeStartedHandler;
+
+        _workerPlanningMergeAbortedHandler = planningTaskId =>
+        {
+            if (Task?.Id == planningTaskId) IsMergeDraining = false;
+        };
+        _worker.PlanningMergeAbortedEvent += _workerPlanningMergeAbortedHandler;
+
+        _workerPlanningCompletedHandler = planningTaskId =>
+        {
+            if (Task?.Id == planningTaskId) IsMergeDraining = false;
+        };
+        _worker.PlanningCompletedEvent += _workerPlanningCompletedHandler;
+
         ChildOutcomes.CollectionChanged += (_, _) =>
         {
             Merge.SyncChildOutcomes(HasChildOutcomes, Subtasks.Count);
@@ -409,6 +431,9 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
         _worker.TaskFinishedEvent -= _workerTaskFinishedHandler;
         _worker.WorktreeUpdatedEvent -= _workerWorktreeUpdatedHandler;
         _worker.TaskUpdatedEvent -= _workerTaskUpdatedHandler;
+        _worker.PlanningMergeStartedEvent -= _workerPlanningMergeStartedHandler;
+        _worker.PlanningMergeAbortedEvent -= _workerPlanningMergeAbortedHandler;
+        _worker.PlanningCompletedEvent -= _workerPlanningCompletedHandler;
         AgentSettings.Dispose();
         Prep.Dispose();
     }
@@ -438,6 +463,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
                 DequeueCommand.NotifyCanExecuteChanged();
                 ResetAndRetryCommand.NotifyCanExecuteChanged();
                 ContinueCommand.NotifyCanExecuteChanged();
+                CancelReviewCommand.NotifyCanExecuteChanged();
                 // A state change means a new run/review cycle: the diff must be
                 // re-inspected before merge can be approved again.
                 ReviewDiffViewed = false;
@@ -547,6 +573,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
         OnPropertyChanged(nameof(TaskIdBadge));
         Monitor.Reset();
         RoadblockReplyDraft = string.Empty;
+        IsMergeDraining = false;
         Subtasks.Clear();
         ChildOutcomes.Clear();
         Attachments.Clear();
@@ -1200,13 +1227,27 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
         catch { /* stale review action; broadcast reconciles */ }
     }
 
-    [RelayCommand]
+    // True while a unit merge is actively draining this task's subtasks onto the target
+    // branch — set from PlanningMergeStarted, cleared on PlanningMergeAborted/Completed.
+    // Mirrors the worker-side guard in TaskStateService.CancelAsync (the real correctness
+    // fix); this just keeps the button from inviting a click the worker will reject anyway.
+    [ObservableProperty]
+    [NotifyCanExecuteChangedFor(nameof(CancelReviewCommand))]
+    private bool _isMergeDraining;
+
+    [RelayCommand(CanExecute = nameof(CanCancelReview))]
     private async System.Threading.Tasks.Task CancelReviewAsync()
     {
         if (Task is null || !_worker.IsConnected) return;
         try { await _worker.CancelReviewAsync(Task.Id); }
-        catch { /* stale review action; broadcast reconciles */ }
+        catch (Exception ex)
+        {
+            ErrorReported?.Invoke(ex.Message);
         }
+    }
+
+    private bool CanCancelReview() =>
+        Task != null && _worker.IsConnected && !IsMergeDraining;
 
     private async System.Threading.Tasks.Task ReloadAttachmentsAsync()
     {

src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs

@@ -0,0 +1,10 @@
+namespace ClaudeDo.Worker.Planning;
+
+// Lets TaskStateService guard CancelAsync against an in-progress unit merge without a
+// hard constructor cycle back to PlanningMergeOrchestrator (which itself depends on
+// ITaskStateService) — TaskStateService takes a Func<IActiveMergeState> instead, resolved
+// lazily so both singletons can finish constructing before either calls into the other.
+public interface IActiveMergeState
+{
+    bool HasActiveMerge(string taskId);
+}

src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs

@@ -13,7 +13,7 @@ namespace ClaudeDo.Worker.Planning;
 /// <summary>A unit-merge conflict currently paused, driven by an MCP session rather than the UI.</summary>
 public sealed record ExternalPlanningMergeConflict(string PlanningTaskId, string SubtaskId);
 
-public sealed class PlanningMergeOrchestrator
+public sealed class PlanningMergeOrchestrator : IActiveMergeState
 {
     private readonly IDbContextFactory<ClaudeDoDbContext> _dbFactory;
     private readonly TaskMergeService _merge;

src/ClaudeDo.Worker/Program.cs

@@ -101,12 +101,16 @@ builder.Services.AddSingleton<IQueuePicker, QueuePicker>();
 builder.Services.AddSingleton<RunCancellationRegistry>();
 
 builder.Services.AddSingleton<Func<ITaskStateService>>(sp => () => sp.GetRequiredService<ITaskStateService>());
+// PlanningMergeOrchestrator itself depends on ITaskStateService, so TaskStateService can only
+// reach it lazily (Func<IActiveMergeState>) — same cycle-breaking shape as the Func above.
+builder.Services.AddSingleton<Func<IActiveMergeState>>(sp => () => sp.GetRequiredService<PlanningMergeOrchestrator>());
 builder.Services.AddSingleton<ITaskStateService>(sp => new TaskStateService(
     sp.GetRequiredService<IDbContextFactory<ClaudeDoDbContext>>(),
     sp.GetRequiredService<HubBroadcaster>(),
     sp.GetRequiredService<IQueueWaker>(),
     sp.GetRequiredService<PlanningChainCoordinator>(),
     sp.GetRequiredService<RunCancellationRegistry>(),
+    sp.GetRequiredService<Func<IActiveMergeState>>(),
     sp.GetRequiredService<ILogger<TaskStateService>>()));
 
 // Agent file management.

src/ClaudeDo.Worker/State/TaskStateService.cs

@@ -16,6 +16,7 @@ public sealed class TaskStateService : ITaskStateService
     private readonly IQueueWaker _waker;
     private readonly PlanningChainCoordinator _chain;
     private readonly RunCancellationRegistry _runCancels;
+    private readonly Func<IActiveMergeState> _mergeState;
     private readonly ILogger<TaskStateService> _logger;
 
     public TaskStateService(
@@ -24,6 +25,7 @@ public sealed class TaskStateService : ITaskStateService
         IQueueWaker waker,
         PlanningChainCoordinator chain,
         RunCancellationRegistry runCancels,
+        Func<IActiveMergeState> mergeState,
         ILogger<TaskStateService> logger)
     {
         _dbFactory = dbFactory;
@@ -31,6 +33,7 @@ public sealed class TaskStateService : ITaskStateService
         _waker = waker;
         _chain = chain;
         _runCancels = runCancels;
+        _mergeState = mergeState;
         _logger = logger;
     }
 
@@ -255,6 +258,14 @@ public sealed class TaskStateService : ITaskStateService
     public async Task<TransitionResult> CancelAsync(
         string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false)
     {
+        // A unit merge drains its children's worktrees onto the target branch while the parent
+        // sits in WaitingForReview for the whole (potentially minutes-long) drain. Cancelling out
+        // from under it would flip the parent to Cancelled while the orchestrator keeps merging —
+        // FinalizeParentDoneAsync then finds the parent no longer WaitingForReview and gives up,
+        // leaving the merged children's diffs stranded with no way to roll them back.
+        if (_mergeState().HasActiveMerge(taskId))
+            return new TransitionResult(false, "A merge is in progress for this task; wait for it to finish before cancelling.");
+
         List<string> cancelledChildIds;
         await using (var ctx = await _dbFactory.CreateDbContextAsync(ct))
         {

tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs

@@ -55,6 +55,7 @@ public abstract class StubWorkerClient : IWorkerClient
     public void RaiseHandoffRequested(string taskId, IReadOnlyList<string> survivingTaskIds) => HandoffRequestedEvent?.Invoke(taskId, survivingTaskIds);
     public void RaisePlanningMergeConflict(string planningTaskId, string subtaskId, IReadOnlyList<string> files, bool externallyDriven)
         => PlanningMergeConflictEvent?.Invoke(planningTaskId, subtaskId, files, externallyDriven);
+    public void RaisePlanningMergeStarted(string planningTaskId, string targetBranch) => PlanningMergeStartedEvent?.Invoke(planningTaskId, targetBranch);
     public void RaisePlanningMergeAborted(string planningTaskId) => PlanningMergeAbortedEvent?.Invoke(planningTaskId);
     public void RaisePlanningCompleted(string planningTaskId) => PlanningCompletedEvent?.Invoke(planningTaskId);
 

tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs

@@ -161,4 +161,69 @@ public class DetailsIslandReviewActionsTests : IDisposable
 
         Assert.Equal(worker.ExceptionMessage, reportedError);
     }
+
+    [Fact]
+    public void CancelReview_IsDisabled_WhileUnitMergeIsDraining()
+    {
+        var worker = new RecordingWorkerClient();
+        var vm = BuildVm(worker);
+        vm.Bind(new TaskRowViewModel { Id = "task-drain-1", Status = TaskStatus.WaitingForReview });
+
+        Assert.True(vm.CancelReviewCommand.CanExecute(null));
+
+        worker.RaisePlanningMergeStarted("task-drain-1", "main");
+        Assert.False(vm.CancelReviewCommand.CanExecute(null));
+
+        worker.RaisePlanningCompleted("task-drain-1");
+        Assert.True(vm.CancelReviewCommand.CanExecute(null));
+    }
+
+    [Fact]
+    public void CancelReview_ReDisables_UntilMergeAborted()
+    {
+        var worker = new RecordingWorkerClient();
+        var vm = BuildVm(worker);
+        vm.Bind(new TaskRowViewModel { Id = "task-drain-2", Status = TaskStatus.WaitingForReview });
+
+        worker.RaisePlanningMergeStarted("task-drain-2", "main");
+        Assert.False(vm.CancelReviewCommand.CanExecute(null));
+
+        worker.RaisePlanningMergeAborted("task-drain-2");
+        Assert.True(vm.CancelReviewCommand.CanExecute(null));
+    }
+
+    [Fact]
+    public void CancelReview_UnaffectedByMergeEvents_ForADifferentTask()
+    {
+        var worker = new RecordingWorkerClient();
+        var vm = BuildVm(worker);
+        vm.Bind(new TaskRowViewModel { Id = "task-drain-3", Status = TaskStatus.WaitingForReview });
+
+        worker.RaisePlanningMergeStarted("some-other-parent", "main");
+
+        Assert.True(vm.CancelReviewCommand.CanExecute(null));
+    }
+
+    private sealed class ThrowingCancelWorkerClient : StubWorkerClient
+    {
+        public override bool IsConnected => true;
+        public string ExceptionMessage { get; init; } =
+            "A merge is in progress for this task; wait for it to finish before cancelling.";
+        public override Task CancelReviewAsync(string taskId) => throw new Exception(ExceptionMessage);
+    }
+
+    [Fact]
+    public async Task CancelReview_WhenWorkerThrows_RaisesErrorReported()
+    {
+        var worker = new ThrowingCancelWorkerClient();
+        var vm = BuildVm(worker);
+        vm.Bind(new TaskRowViewModel { Id = "task-cancel-err-1", Status = TaskStatus.WaitingForReview });
+
+        string? reportedError = null;
+        vm.ErrorReported += msg => reportedError = msg;
+
+        await vm.CancelReviewCommand.ExecuteAsync(null);
+
+        Assert.Equal(worker.ExceptionMessage, reportedError);
+    }
 }

tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs

@@ -20,7 +20,8 @@ public static class TaskStateServiceBuilder
         CountingQueueWaker Waker,
         RunCancellationRegistry RunCancels);
 
-    public static Built Build(IDbContextFactory<ClaudeDoDbContext> dbFactory)
+    public static Built Build(
+        IDbContextFactory<ClaudeDoDbContext> dbFactory, Func<IActiveMergeState>? mergeState = null)
     {
         var hub = new CapturingHubContext();
         var broadcaster = new HubBroadcaster(hub);
@@ -35,12 +36,19 @@ public static class TaskStateServiceBuilder
             waker,
             chain,
             runCancels,
+            mergeState ?? (() => NoActiveMergeState.Instance),
             NullLogger<TaskStateService>.Instance);
 
         return new Built(state, chain, hub, () => waker.Count, waker, runCancels);
     }
 }
 
+file sealed class NoActiveMergeState : IActiveMergeState
+{
+    public static readonly NoActiveMergeState Instance = new();
+    public bool HasActiveMerge(string taskId) => false;
+}
+
 public sealed class CountingQueueWaker : IQueueWaker
 {
     private int _count;

tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs

@@ -1,6 +1,7 @@
 using ClaudeDo.Data;
 using ClaudeDo.Data.Models;
 using ClaudeDo.Data.Repositories;
+using ClaudeDo.Worker.Planning;
 using ClaudeDo.Worker.State;
 using ClaudeDo.Worker.Tests.Infrastructure;
 using Microsoft.EntityFrameworkCore;
@@ -8,6 +9,12 @@ using TaskStatus = ClaudeDo.Data.Models.TaskStatus;
 
 namespace ClaudeDo.Worker.Tests.State;
 
+file sealed class FakeActiveMergeState : IActiveMergeState
+{
+    public HashSet<string> ActiveTaskIds { get; } = new();
+    public bool HasActiveMerge(string taskId) => ActiveTaskIds.Contains(taskId);
+}
+
 public sealed class TaskStateServiceTests : IDisposable
 {
     private readonly DbFixture _db = new();
@@ -319,6 +326,34 @@ public sealed class TaskStateServiceTests : IDisposable
         Assert.Equal(TaskStatus.Done, await GetStatusAsync(id));
     }
 
+    [Fact]
+    public async Task CancelAsync_WhileUnitMergeDraining_Rejects_AndDoesNotMutate()
+    {
+        var mergeState = new FakeActiveMergeState();
+        var built = TaskStateServiceBuilder.Build(_factory, () => mergeState);
+        var id = await SeedTaskAsync(TaskStatus.WaitingForReview);
+        mergeState.ActiveTaskIds.Add(id);
+
+        var result = await built.State.CancelAsync(id, DateTime.UtcNow, default);
+
+        Assert.False(result.Ok);
+        Assert.Equal(TaskStatus.WaitingForReview, await GetStatusAsync(id));
+    }
+
+    [Fact]
+    public async Task CancelAsync_AfterUnitMergeDrainCompletes_Succeeds()
+    {
+        var mergeState = new FakeActiveMergeState();
+        var built = TaskStateServiceBuilder.Build(_factory, () => mergeState);
+        var id = await SeedTaskAsync(TaskStatus.WaitingForReview);
+        // Merge drain finished (or never started) for this task — cancel behaves as before.
+
+        var result = await built.State.CancelAsync(id, DateTime.UtcNow, default);
+
+        Assert.True(result.Ok);
+        Assert.Equal(TaskStatus.Cancelled, await GetStatusAsync(id));
+    }
+
     // ─── ResetToIdleAsync ─────────────────────────────────────────────────
 
     [Fact]