diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index cc4e288a..b01365f9 100644 --- a/docs/explore-notes/review-merge.md +++ b/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` instead — same cycle-breaking shape as the existing `Func` +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). diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 18449758..3b155974 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -35,6 +35,9 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private readonly Action _workerTaskFinishedHandler; private readonly Action _workerWorktreeUpdatedHandler; private readonly Action _workerTaskUpdatedHandler; + private readonly Action _workerPlanningMergeStartedHandler; + private readonly Action _workerPlanningMergeAbortedHandler; + private readonly Action _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,14 +1227,28 @@ 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() { if (Task is null) return; diff --git a/src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs b/src/ClaudeDo.Worker/Planning/Interfaces/IActiveMergeState.cs new file mode 100644 index 00000000..c156fe40 --- /dev/null +++ b/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 instead, resolved +// lazily so both singletons can finish constructing before either calls into the other. +public interface IActiveMergeState +{ + bool HasActiveMerge(string taskId); +} diff --git a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs index 0e49aeb8..dbafc844 100644 --- a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs +++ b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs @@ -13,7 +13,7 @@ namespace ClaudeDo.Worker.Planning; /// A unit-merge conflict currently paused, driven by an MCP session rather than the UI. public sealed record ExternalPlanningMergeConflict(string PlanningTaskId, string SubtaskId); -public sealed class PlanningMergeOrchestrator +public sealed class PlanningMergeOrchestrator : IActiveMergeState { private readonly IDbContextFactory _dbFactory; private readonly TaskMergeService _merge; diff --git a/src/ClaudeDo.Worker/Program.cs b/src/ClaudeDo.Worker/Program.cs index ae85c54d..c4fd4ed1 100644 --- a/src/ClaudeDo.Worker/Program.cs +++ b/src/ClaudeDo.Worker/Program.cs @@ -101,12 +101,16 @@ builder.Services.AddSingleton(); builder.Services.AddSingleton(); builder.Services.AddSingleton>(sp => () => sp.GetRequiredService()); +// PlanningMergeOrchestrator itself depends on ITaskStateService, so TaskStateService can only +// reach it lazily (Func) — same cycle-breaking shape as the Func above. +builder.Services.AddSingleton>(sp => () => sp.GetRequiredService()); builder.Services.AddSingleton(sp => new TaskStateService( sp.GetRequiredService>(), sp.GetRequiredService(), sp.GetRequiredService(), sp.GetRequiredService(), sp.GetRequiredService(), + sp.GetRequiredService>(), sp.GetRequiredService>())); // Agent file management. diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index 855ad5cf..ee551639 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/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 _mergeState; private readonly ILogger _logger; public TaskStateService( @@ -24,6 +25,7 @@ public sealed class TaskStateService : ITaskStateService IQueueWaker waker, PlanningChainCoordinator chain, RunCancellationRegistry runCancels, + Func mergeState, ILogger 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 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 cancelledChildIds; await using (var ctx = await _dbFactory.CreateDbContextAsync(ct)) { diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index f6f21f71..8e16e961 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -55,6 +55,7 @@ public abstract class StubWorkerClient : IWorkerClient public void RaiseHandoffRequested(string taskId, IReadOnlyList survivingTaskIds) => HandoffRequestedEvent?.Invoke(taskId, survivingTaskIds); public void RaisePlanningMergeConflict(string planningTaskId, string subtaskId, IReadOnlyList 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); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs index aba181ae..4eec5c2c 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandReviewActionsTests.cs +++ b/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); + } } diff --git a/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs b/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs index a6399ab0..e9caabad 100644 --- a/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs +++ b/tests/ClaudeDo.Worker.Tests/Infrastructure/TaskStateServiceBuilder.cs @@ -20,7 +20,8 @@ public static class TaskStateServiceBuilder CountingQueueWaker Waker, RunCancellationRegistry RunCancels); - public static Built Build(IDbContextFactory dbFactory) + public static Built Build( + IDbContextFactory dbFactory, Func? 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.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; diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index f0f4af4c..629b666c 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/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 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]