diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index d5287389..76158f0a 100644 --- a/docs/explore-notes/review-merge.md +++ b/docs/explore-notes/review-merge.md @@ -76,6 +76,23 @@ DB write. Cycle note: `TaskStateService` can't take a direct constructor depende handed to `PlanningChainCoordinator`. `IActiveMergeState` (`Planning/Interfaces/`) is `PlanningMergeOrchestrator`'s only public surface `TaskStateService` needs. +`HasActiveMerge` also stays true through the window between the last child merging and +`FinalizeParentDoneAsync` returning (a separate `State.IsFinalizing` flag — `CurrentSubtaskId` is +already null there since no subtask is left to merge). Without it, a `Cancel` racing the finalize +call's own `ApproveReviewAsync` could slip past the guard for that whole call. `StartAsync` also now +requires the parent to already be `WaitingForReview` — for planning **and** improvement parents — +before touching anything; previously only planning parents got an (indirect, children-only) check, +so a stale UI click or a second caller on an improvement parent that had already left +`WaitingForReview` could still drive a partial child merge before the final approve refused. + +Unit-merge failures used to vanish silently: a child merge that came back +`blocked`/`verify_failed`/`untracked_collision` (not `conflict`) was only logged server-side, and +`ApproveReview`/`review_task` approve always reported success (`StatusMerged`) regardless. +`StartAsync`/`ContinueAsync`/the private `DrainAsync` now return a `PlanningMergeResult(Status, +Reason)` that both hub paths propagate as the real status, and `PlanningMergeAborted` carries that +reason so the UI's `IslandsShellViewModel.OnPlanningMergeAborted` can `FlashFooterError` it instead +of only clearing the banner. + 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` diff --git a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs index 4d0f9535..08976c92 100644 --- a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs @@ -54,7 +54,9 @@ public interface IWorkerClient : INotifyPropertyChanged /// is true when an MCP session (not the UI) started the unit merge — the resolver must not /// auto-open in that case. event Action, bool>? PlanningMergeConflictEvent; - event Action? PlanningMergeAbortedEvent; + /// (planningTaskId, reason). reason is set when the merge stopped on a real failure + /// (blocked/verify_failed/untracked_collision) rather than a deliberate abort. + event Action? PlanningMergeAbortedEvent; event Action? PlanningCompletedEvent; event Action? PrimeFired; diff --git a/src/ClaudeDo.Ui/Services/WorkerClient.cs b/src/ClaudeDo.Ui/Services/WorkerClient.cs index 62d99a2a..0c75dbd6 100644 --- a/src/ClaudeDo.Ui/Services/WorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/WorkerClient.cs @@ -70,7 +70,7 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC public event Action? PlanningMergeStartedEvent; public event Action? PlanningSubtaskMergedEvent; public event Action, bool>? PlanningMergeConflictEvent; - public event Action? PlanningMergeAbortedEvent; + public event Action? PlanningMergeAbortedEvent; public event Action? PlanningCompletedEvent; public event Action? PrimeFired; @@ -199,9 +199,9 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC Dispatcher.UIThread.Post(() => PlanningMergeConflictEvent?.Invoke(planningTaskId, subtaskId, conflictedFiles, externallyDriven)); }); - _hub.On("PlanningMergeAborted", planningTaskId => + _hub.On("PlanningMergeAborted", (planningTaskId, reason) => { - Dispatcher.UIThread.Post(() => PlanningMergeAbortedEvent?.Invoke(planningTaskId)); + Dispatcher.UIThread.Post(() => PlanningMergeAbortedEvent?.Invoke(planningTaskId, reason)); }); _hub.On("PlanningCompleted", planningTaskId => diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 34ade86c..f9f7b68e 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -36,7 +36,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private readonly Action _workerWorktreeUpdatedHandler; private readonly Action _workerTaskUpdatedHandler; private readonly Action _workerPlanningMergeStartedHandler; - private readonly Action _workerPlanningMergeAbortedHandler; + private readonly Action _workerPlanningMergeAbortedHandler; private readonly Action _workerPlanningCompletedHandler; [ObservableProperty] private bool _isNotesMode; @@ -432,7 +432,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable }; _worker.PlanningMergeStartedEvent += _workerPlanningMergeStartedHandler; - _workerPlanningMergeAbortedHandler = planningTaskId => + _workerPlanningMergeAbortedHandler = (planningTaskId, _) => { if (Task?.Id == planningTaskId) IsMergeDraining = false; }; diff --git a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs index 608733e6..0a13dea0 100644 --- a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs @@ -218,7 +218,14 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable _ = OpenPlanningConflictAsync(planningTaskId, subtaskId); } - public void OnPlanningMergeAborted(string planningTaskId) => ClearExternalMergeConflict(planningTaskId); + public void OnPlanningMergeAborted(string planningTaskId, string? reason = null) + { + ClearExternalMergeConflict(planningTaskId); + // A deliberate abort/conflict pause and a real merge failure both flow through this + // event; only the latter carries a reason worth surfacing in the footer strip. + if (!string.IsNullOrWhiteSpace(reason)) + FlashFooterError(reason); + } public void OnPlanningMergeCompleted(string planningTaskId) => ClearExternalMergeConflict(planningTaskId); private void ClearExternalMergeConflict(string planningTaskId) diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index ff9a40be..1fc41ac1 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -174,7 +174,9 @@ launch specs · worktrees · agents/settings/lists · reports/notes/prep · diag - `PlanningMergeStarted` - `PlanningSubtaskMerged` - `PlanningMergeConflict` -- `PlanningMergeAborted` +- `PlanningMergeAborted` (`planningTaskId, reason` — every call site passes a human-readable + reason now, from a real merge failure's `ErrorMessage` to a plain "Merge aborted."; the UI + flashes it via `FlashFooterError`) - `PlanningCompleted` - `RefineStarted` - `RefineFinished` diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index a8c9ecad..fcfea6f2 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -703,16 +703,21 @@ public sealed class ExternalMcpService // externallyDriven: true — this call came from an MCP session, not the UI's // Approve button. A unit-merge conflict must not auto-open the in-app resolver; // the driving session resolves it via continue_merge/abort_merge instead. - await _planningMerge.StartAsync(taskId, targetBranch ?? "", cancellationToken, externallyDriven: true, progress); - var parentDone = (await _tasks.GetByIdAsync(taskId, cancellationToken))!.Status == TaskStatus.Done; - mergeStatus = parentDone ? TaskMergeService.StatusMerged : TaskMergeService.StatusConflict; - if (!parentDone) + var startResult = await _planningMerge.StartAsync(taskId, targetBranch ?? "", cancellationToken, externallyDriven: true, progress); + if (startResult.Status == TaskMergeService.StatusBlocked) + throw new InvalidOperationException(startResult.Reason ?? "approve failed"); + mergeStatus = startResult.Status; + if (startResult.Status == TaskMergeService.StatusConflict) { var list = await _lists.GetByIdAsync(task.ListId, cancellationToken); repoPath = list?.WorkingDir; mergeMessage = "unit merge paused on a conflict — markers left in the working tree; " + "resolve them then call continue_merge with the parent task id, or abort_merge to cancel"; } + else if (startResult.Status != TaskMergeService.StatusMerged) + { + mergeMessage = startResult.Reason; + } } else { diff --git a/src/ClaudeDo.Worker/Hub/HubBroadcaster.cs b/src/ClaudeDo.Worker/Hub/HubBroadcaster.cs index 872cf56f..34b9fcbd 100644 --- a/src/ClaudeDo.Worker/Hub/HubBroadcaster.cs +++ b/src/ClaudeDo.Worker/Hub/HubBroadcaster.cs @@ -64,8 +64,8 @@ public sealed class HubBroadcaster : IPrimeBroadcaster, IRefineBroadcaster string planningTaskId, string subtaskId, IReadOnlyList files, bool externallyDriven) => _hub.Clients.All.SendAsync("PlanningMergeConflict", planningTaskId, subtaskId, files, externallyDriven); - public Task PlanningMergeAborted(string planningTaskId) => - _hub.Clients.All.SendAsync("PlanningMergeAborted", planningTaskId); + public Task PlanningMergeAborted(string planningTaskId, string? reason) => + _hub.Clients.All.SendAsync("PlanningMergeAborted", planningTaskId, reason); public Task PlanningCompleted(string planningTaskId) => _hub.Clients.All.SendAsync("PlanningCompleted", planningTaskId); diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index e0af7013..211f0457 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -744,8 +744,10 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub if (hasChildren) { - await _planningMergeOrchestrator.StartAsync(taskId, targetBranch ?? "", CancellationToken.None); - return new MergeResultDto(TaskMergeService.StatusMerged, Array.Empty(), null); + var unitResult = await _planningMergeOrchestrator.StartAsync(taskId, targetBranch ?? "", CancellationToken.None); + if (unitResult.Status == TaskMergeService.StatusBlocked) + throw new HubException(unitResult.Reason ?? "approve failed"); + return new MergeResultDto(unitResult.Status, Array.Empty(), unitResult.Reason); } var r = await _mergeService.ApproveAndMergeAsync(taskId, targetBranch ?? "", CancellationToken.None); diff --git a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs index a58bb24c..ee4a93f1 100644 --- a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs +++ b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs @@ -14,6 +14,16 @@ 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); +/// Outcome of a unit-merge drive (StartAsync/ContinueAsync). Status mirrors +/// 's status strings — StatusMerged/StatusConflict on the two +/// "everything worked (so far)" paths, or the real blocked/verify_failed/untracked_collision +/// status with its Reason when a child merge (or the final approve) failed outright. +public sealed record PlanningMergeResult(string Status, string? Reason) +{ + public static readonly PlanningMergeResult Merged = new(TaskMergeService.StatusMerged, null); + public static readonly PlanningMergeResult Conflict = new(TaskMergeService.StatusConflict, null); +} + public sealed class PlanningMergeOrchestrator : IActiveMergeState { private readonly IDbContextFactory _dbFactory; @@ -35,6 +45,12 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState /// merge must not auto-pop the in-app resolver — the driving session owns resolution. public required bool ExternallyDriven { get; init; } public string? CurrentSubtaskId { get; set; } + /// True from the moment the last child has merged until FinalizeParentDoneAsync + /// returns. CurrentSubtaskId is already null in this window (no subtask left to merge), + /// so HasActiveMerge needs this separate flag — otherwise a Cancel racing the finalize call + /// would slip past TaskStateService.CancelAsync's guard while the parent is still being + /// flipped to Done. + public bool IsFinalizing { get; set; } } private readonly ConcurrentDictionary _states = new(); @@ -57,7 +73,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState _logger = logger; } - public async Task StartAsync( + public async Task StartAsync( string parentTaskId, string targetBranch, CancellationToken ct, bool externallyDriven = false, IProgress? progress = null) { @@ -65,6 +81,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState List children; bool isPlanning; bool parentHasWorktree; + TaskStatus parentStatus; using (var ctx = _dbFactory.CreateDbContext()) { @@ -79,6 +96,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState children = parent.Children.OrderBy(c => c.SortOrder).ToList(); isPlanning = parent.PlanningPhase != PlanningPhase.None; parentHasWorktree = parent.Worktree is { State: WorktreeState.Active }; + parentStatus = parent.Status; } if (isPlanning) @@ -95,6 +113,13 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState } } + // Applies to planning AND improvement parents alike -- a stale UI click or a second + // caller after the parent already left WaitingForReview (e.g. cancelled, or a previous + // Approve already drove it to Done) must not kick off a partial re-merge of its children. + if (parentStatus != TaskStatus.WaitingForReview) + throw new InvalidOperationException( + $"planning task '{parentTaskId}' is not WaitingForReview (status: {parentStatus})"); + if (await _git.IsMidMergeAsync(workingDir, ct)) throw new InvalidOperationException( "repo is mid-merge; use AbortPlanningMerge to reset the repository, then Approve again"); @@ -123,12 +148,13 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState throw new InvalidOperationException($"Merge already in progress for {parentTaskId}."); await _broadcaster.PlanningMergeStarted(parentTaskId, targetBranch); - await DrainAsync(parentTaskId, ct, progress); + return await DrainAsync(parentTaskId, ct, progress); } - /// True when a unit merge for this parent is paused on a conflict (in-memory state). + /// True when a unit merge for this parent is paused on a conflict, or is in the + /// window between the last child merging and FinalizeParentDoneAsync completing. public bool HasActiveMerge(string parentTaskId) => - _states.TryGetValue(parentTaskId, out var s) && s.CurrentSubtaskId is not null; + _states.TryGetValue(parentTaskId, out var s) && (s.CurrentSubtaskId is not null || s.IsFinalizing); /// Externally-driven unit merges currently paused on a conflict, so the UI can /// recover this on reconnect instead of relying solely on the one-shot broadcast. Checked @@ -147,7 +173,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState return result; } - public async Task ContinueAsync( + public async Task ContinueAsync( string planningTaskId, CancellationToken ct, IProgress? progress = null) { if (!_states.TryGetValue(planningTaskId, out var state) || state.CurrentSubtaskId is null) @@ -160,7 +186,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState if (result.Status == TaskMergeService.StatusConflict) { await _broadcaster.PlanningMergeConflict(planningTaskId, current, result.ConflictFiles, state.ExternallyDriven); - return; + return PlanningMergeResult.Conflict; } if (result.Status != TaskMergeService.StatusMerged) @@ -169,14 +195,14 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState "Planning continue blocked on subtask {Subtask}: {Msg}", current, result.ErrorMessage); _states.TryRemove(planningTaskId, out _); - await _broadcaster.PlanningMergeAborted(planningTaskId); - return; + await _broadcaster.PlanningMergeAborted(planningTaskId, result.ErrorMessage); + return new PlanningMergeResult(result.Status, result.ErrorMessage); } await _broadcaster.PlanningSubtaskMerged(planningTaskId, current); state.CurrentSubtaskId = null; - await DrainAsync(planningTaskId, ct, progress); + return await DrainAsync(planningTaskId, ct, progress); } public async Task AbortAsync(string planningTaskId, CancellationToken ct) @@ -191,7 +217,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState await _merge.AbortMergeAsync(state.CurrentSubtaskId, ct); _states.TryRemove(planningTaskId, out _); - await _broadcaster.PlanningMergeAborted(planningTaskId); + await _broadcaster.PlanningMergeAborted(planningTaskId, "Merge aborted."); } private async Task AbortStatelessAsync(string planningTaskId, CancellationToken ct) @@ -212,14 +238,16 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState _logger.LogInformation( "Stateless abort of mid-merge for planning task {ParentId} (post-restart recovery)", planningTaskId); - await _broadcaster.PlanningMergeAborted(planningTaskId); + await _broadcaster.PlanningMergeAborted( + planningTaskId, "Merge aborted after a worker restart. Approve again to restart the merge."); // Parent remains WaitingForReview — Approve will restart the unit merge from scratch. } - private async Task DrainAsync( + private async Task DrainAsync( string planningTaskId, CancellationToken ct, IProgress? progress = null) { - if (!_states.TryGetValue(planningTaskId, out var state)) return; + if (!_states.TryGetValue(planningTaskId, out var state)) + return new PlanningMergeResult(TaskMergeService.StatusBlocked, "no merge state found for this planning task"); var keepState = false; try @@ -241,7 +269,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState await _broadcaster.PlanningMergeConflict( planningTaskId, subtaskId, result.ConflictFiles, state.ExternallyDriven); keepState = true; - return; + return PlanningMergeResult.Conflict; } if (result.Status != TaskMergeService.StatusMerged) @@ -249,17 +277,25 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState _logger.LogWarning( "Planning merge blocked on subtask {Subtask}: {Msg}", subtaskId, result.ErrorMessage); - await _broadcaster.PlanningMergeAborted(planningTaskId); - return; // keepState stays false → finally removes the state entry + await _broadcaster.PlanningMergeAborted(planningTaskId, result.ErrorMessage); + return new PlanningMergeResult(result.Status, result.ErrorMessage); // keepState stays false → finally removes the state entry } await _broadcaster.PlanningSubtaskMerged(planningTaskId, subtaskId); } + // No subtask left to merge, but the parent isn't Done yet -- HasActiveMerge must keep + // reporting true through this window (CurrentSubtaskId is already null) so a Cancel + // racing FinalizeParentDoneAsync's ApproveReviewAsync call is still refused. state.CurrentSubtaskId = null; - var finalized = await FinalizeParentDoneAsync(planningTaskId, state.IsPlanning, ct); + state.IsFinalizing = true; + var (finalized, reason) = await FinalizeParentDoneAsync(planningTaskId, state.IsPlanning, ct); if (finalized) + { await _broadcaster.PlanningCompleted(planningTaskId); + return PlanningMergeResult.Merged; + } + return new PlanningMergeResult(TaskMergeService.StatusBlocked, reason); } finally { @@ -267,7 +303,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState } } - private async Task FinalizeParentDoneAsync(string parentTaskId, bool isPlanning, CancellationToken ct) + private async Task<(bool Ok, string? Reason)> FinalizeParentDoneAsync(string parentTaskId, bool isPlanning, CancellationToken ct) { var result = await _state.ApproveReviewAsync(parentTaskId, ct); if (!result.Ok) @@ -288,7 +324,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState _logger.LogWarning( "Unit-merge drain completed but parent {ParentTaskId} could not be finalized (status: {Status}): {Reason}", parentTaskId, current, result.Reason); - return false; + return (false, result.Reason); } } @@ -299,6 +335,6 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState catch (Exception ex) { _logger.LogWarning(ex, "integration branch cleanup failed"); } } - return true; + return (true, null); } } diff --git a/tests/ClaudeDo.Ui.Tests/IslandsShellViewModelExternalMergeTests.cs b/tests/ClaudeDo.Ui.Tests/IslandsShellViewModelExternalMergeTests.cs index c3166e9a..d85843d2 100644 --- a/tests/ClaudeDo.Ui.Tests/IslandsShellViewModelExternalMergeTests.cs +++ b/tests/ClaudeDo.Ui.Tests/IslandsShellViewModelExternalMergeTests.cs @@ -1,5 +1,6 @@ using System; using System.Threading.Tasks; +using ClaudeDo.Data.Models; using ClaudeDo.Ui.ViewModels; using Xunit; @@ -43,6 +44,31 @@ public class IslandsShellViewModelExternalMergeTests Assert.False(vm.IsExternalMergeBannerVisible); } + // Covers the unit-merge-error-propagation fix: a real merge failure (blocked/verify_failed/ + // untracked_collision) reaches the UI as a reason on PlanningMergeAborted, and must surface + // in the footer strip rather than silently vanishing once the banner is cleared. + [Fact] + public void Aborted_WithReason_FlashesFooterError() + { + var vm = new IslandsShellViewModel(); + + vm.OnPlanningMergeAborted("plan1", "child merge blocked: unrelated histories"); + + Assert.Equal(WorkerLogLevel.Error, vm.WorkerLogLevel); + Assert.Contains("child merge blocked: unrelated histories", vm.WorkerLogText); + Assert.True(vm.IsWorkerLogVisible); + } + + [Fact] + public void Aborted_WithoutReason_DoesNotFlashFooterError() + { + var vm = new IslandsShellViewModel(); + + vm.OnPlanningMergeAborted("plan1", null); + + Assert.False(vm.IsWorkerLogVisible); + } + [Fact] public void Completed_ClearsBanner() { diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index 2cc8bc53..0a556936 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -36,7 +36,7 @@ public abstract class StubWorkerClient : IWorkerClient public event Action? PlanningMergeStartedEvent; public event Action? PlanningSubtaskMergedEvent; public event Action, bool>? PlanningMergeConflictEvent; - public event Action? PlanningMergeAbortedEvent; + public event Action? PlanningMergeAbortedEvent; public event Action? PlanningCompletedEvent; public event Action? PrimeFired; public event Action? UsageUpdatedEvent; @@ -60,7 +60,7 @@ public abstract class StubWorkerClient : IWorkerClient public void RaiseOperationProgress(string opKey, string phase, int current, int total) => OperationProgressEvent?.Invoke(opKey, phase, current, total); public void RaiseMergeProgress(string taskId, string phase, int elapsedSeconds) => MergeProgressEvent?.Invoke(taskId, phase, elapsedSeconds); public void RaisePlanningMergeStarted(string planningTaskId, string targetBranch) => PlanningMergeStartedEvent?.Invoke(planningTaskId, targetBranch); - public void RaisePlanningMergeAborted(string planningTaskId) => PlanningMergeAbortedEvent?.Invoke(planningTaskId); + public void RaisePlanningMergeAborted(string planningTaskId, string? reason = null) => PlanningMergeAbortedEvent?.Invoke(planningTaskId, reason); public void RaisePlanningCompleted(string planningTaskId) => PlanningCompletedEvent?.Invoke(planningTaskId); public void RaisePrepStarted() => PrepStartedEvent?.Invoke(); diff --git a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs index 58a34efc..216a1f82 100644 --- a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs @@ -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 } /// - /// 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. /// [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( + () => 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) ───────── + + /// + /// 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. + /// + [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); + } + + /// + /// 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. + /// + 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 ───────────────────────────── + + /// + /// 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. + /// + [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.Instance); + var aggregator = new PlanningAggregator(factory, git, NullLogger.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.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."); + } +} + +/// Test-only decorator that invokes a callback right before delegating +/// ApproveReviewAsync — everything else passes straight through to the real service. +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 ApproveReviewAsync(string taskId, CancellationToken ct) + { + _onApprove(); + return _inner.ApproveReviewAsync(taskId, ct); + } + + public Task EnqueueAsync(string taskId, CancellationToken ct) => _inner.EnqueueAsync(taskId, ct); + public Task StartRunningAsync(string taskId, DateTime startedAt, CancellationToken ct) => _inner.StartRunningAsync(taskId, startedAt, ct); + public Task CompleteAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.CompleteAsync(taskId, finishedAt, result, ct); + public Task SubmitForReviewAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForReviewAsync(taskId, finishedAt, result, ct); + public Task SubmitInteractiveForReviewAsync(string taskId, DateTime finishedAt, CancellationToken ct) => _inner.SubmitInteractiveForReviewAsync(taskId, finishedAt, ct); + public Task SubmitForChildrenAsync(string taskId, DateTime finishedAt, string? result, CancellationToken ct) => _inner.SubmitForChildrenAsync(taskId, finishedAt, result, ct); + public Task 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 CancelAsync(string taskId, DateTime finishedAt, CancellationToken ct, bool allowFromIdle = false) => _inner.CancelAsync(taskId, finishedAt, ct, allowFromIdle); + public Task ResetToIdleAsync(string taskId, CancellationToken ct) => _inner.ResetToIdleAsync(taskId, ct); + public Task RejectToQueueAsync(string taskId, string feedback, CancellationToken ct) => _inner.RejectToQueueAsync(taskId, feedback, ct); + public Task RejectToIdleAsync(string taskId, CancellationToken ct) => _inner.RejectToIdleAsync(taskId, ct); + public Task ClearReviewFeedbackAsync(string taskId, CancellationToken ct) => _inner.ClearReviewFeedbackAsync(taskId, ct); + public Task ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) => _inner.ForceSetStatusAsync(taskId, status, ct); + public Task StartPlanningAsync(string parentId, CancellationToken ct) => _inner.StartPlanningAsync(parentId, ct); + public Task FinalizePlanningAsync(string parentId, CancellationToken ct) => _inner.FinalizePlanningAsync(parentId, ct); + public Task BlockOnAsync(string taskId, string predecessorTaskId, CancellationToken ct) => _inner.BlockOnAsync(taskId, predecessorTaskId, ct); + public Task UnblockAsync(string taskId, CancellationToken ct) => _inner.UnblockAsync(taskId, ct); + public Task SetDependsOnAsync(string taskId, string? dependsOnTaskId, CancellationToken ct) => _inner.SetDependsOnAsync(taskId, dependsOnTaskId, ct); + public Task TryAdvanceParentAsync(string parentId) => _inner.TryAdvanceParentAsync(parentId); + public Task RecoverStaleRunningAsync(string reason, CancellationToken ct) => _inner.RecoverStaleRunningAsync(reason, ct); } diff --git a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs index 5b9b38b7..3be0098b 100644 --- a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs +++ b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs @@ -118,7 +118,7 @@ sealed class FakeWorkerClient : IWorkerClient public event Action? PlanningMergeStartedEvent; public event Action? PlanningSubtaskMergedEvent; public event Action, bool>? PlanningMergeConflictEvent; - public event Action? PlanningMergeAbortedEvent; + public event Action? PlanningMergeAbortedEvent; public event Action? PlanningCompletedEvent; public event Action? PrimeFired; public event Action? UsageUpdatedEvent;