From 5d1d2d89d014645e6fac85aaad9f4b5281ba28bf Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 12:04:52 +0200 Subject: [PATCH] fix(ui): suppress auto-open conflict resolver during MCP-driven merges review_task/continue_merge on a planning parent always leaves conflicts in the tree, and the UI auto-opened the resolver on every PlanningMergeConflict broadcast regardless of who started the merge -- so a running Claude session resolving a unit-merge conflict could race a human editing the same shared checkout in a resolver window neither of them asked for. PlanningMergeOrchestrator.StartAsync now takes an externallyDriven flag (set by ExternalMcpService's MCP-driven review_task path, left false for the UI's ApproveReview) that rides along on the PlanningMergeConflict broadcast. The UI only auto-opens the resolver when it's false; otherwise it shows a persistent banner with a manual "Open resolver" button, cleared on PlanningMergeAborted/PlanningCompleted. A new GetActiveExternalConflictsAsync query (checked against GitService.IsMidMergeAsync rather than the in-memory flag alone) lets the UI resync the banner on reconnect instead of trusting a one-shot broadcast that isn't replayed after a restart. The childless single-task conflict path was checked and needed no change -- it only broadcasts the generic TaskUpdated, never PlanningMergeConflict. --- docs/explore-notes/review-merge.md | 19 +++- src/ClaudeDo.Localization/locales/de.json | 4 + src/ClaudeDo.Localization/locales/en.json | 4 + .../Services/Interfaces/IWorkerClient.cs | 8 +- src/ClaudeDo.Ui/Services/WorkerClient.cs | 11 +- .../ViewModels/IslandsShellViewModel.cs | 57 +++++++++- src/ClaudeDo.Ui/Views/MainWindow.axaml | 22 ++++ .../External/ExternalMcpService.cs | 5 +- src/ClaudeDo.Worker/Hub/HubBroadcaster.cs | 5 +- src/ClaudeDo.Worker/Hub/WorkerHub.cs | 12 +++ .../Planning/PlanningMergeOrchestrator.cs | 42 +++++++- ...IslandsShellViewModelExternalMergeTests.cs | 101 ++++++++++++++++++ tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs | 8 +- .../PlanningMergeOrchestratorTests.cs | 92 ++++++++++++++++ .../UiVm/TasksIslandViewModelPlanningTests.cs | 4 +- 15 files changed, 379 insertions(+), 15 deletions(-) create mode 100644 tests/ClaudeDo.Ui.Tests/IslandsShellViewModelExternalMergeTests.cs diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index e4e0674e..27198bb9 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 `f6cb825` (2026-08-05). +> Last verified against commit `8247a74` (2026-08-06). > Drift check: `git log --oneline f6cb825..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts` > Stable structure only (no line numbers). See docs/explore-notes/README.md. @@ -129,6 +129,23 @@ mid-merge conflicts **without re-starting the merge** and routes continue/abort `ContinuePlanningMerge` / `AbortPlanningMerge`, so a unit-merge conflict re-opens the editor per subtask via the `PlanningMergeConflict` broadcast. +**Except when an MCP session is driving the merge.** `PlanningMergeOrchestrator.StartAsync` takes +an `externallyDriven` bool (default `false`; `ExternalMcpService.review_task`'s parent-with-children +path passes `true`, since a running Claude session — not the UI — will resolve conflicts via +`continue_merge`/`abort_merge`). The flag rides along on the `PlanningMergeConflict` broadcast +(4th arg); `IslandsShellViewModel.OnPlanningMergeConflict` only auto-opens the resolver when it's +`false` — otherwise it shows a persistent, non-auto-dismissing banner +(`IsExternalMergeBannerVisible`) with a manual "Open resolver" button, cleared on +`PlanningMergeAborted`/`PlanningCompleted`. This exists because two parties (a human and the +driving session) could otherwise end up editing the same shared checkout at once. +`PlanningMergeOrchestrator.GetActiveExternalConflictsAsync` (hub: `GetActiveExternalPlanningMergeConflicts`) +re-derives this state by checking `GitService.IsMidMergeAsync` rather than trusting the in-memory +flag alone, so a UI restart mid-merge (or a stale entry left behind if something resolved the +repo outside the normal Continue/Abort path) can't show a phantom banner — the Ui calls it on +`ConnectionRestoredEvent`. The childless single-task conflict path +(`TaskMergeService.ApproveAndMergeAsync`) has no equivalent broadcast — it only sends the generic +`TaskUpdated` — so it never auto-opened the resolver and needed no change. + ### View Three **AvaloniaEdit** panes showing the whole file: MAIN/ours (read-only) | editable Result | diff --git a/src/ClaudeDo.Localization/locales/de.json b/src/ClaudeDo.Localization/locales/de.json index 1c24e254..31cb136f 100644 --- a/src/ClaudeDo.Localization/locales/de.json +++ b/src/ClaudeDo.Localization/locales/de.json @@ -629,6 +629,10 @@ "available": "Update verfügbar: v", "updateNow": "Jetzt aktualisieren", "dismiss": "Ausblenden" + }, + "externalMerge": { + "banner": "Eine Claude-Session löst gerade einen Merge-Konflikt auf — bitte keine Dateien im Repository bearbeiten.", + "open": "Resolver öffnen" } }, "vm": { diff --git a/src/ClaudeDo.Localization/locales/en.json b/src/ClaudeDo.Localization/locales/en.json index b32e2859..6c6d3df0 100644 --- a/src/ClaudeDo.Localization/locales/en.json +++ b/src/ClaudeDo.Localization/locales/en.json @@ -629,6 +629,10 @@ "available": "Update available: v", "updateNow": "Update now", "dismiss": "Dismiss" + }, + "externalMerge": { + "banner": "A Claude session is resolving a merge conflict — don't edit files in the repository.", + "open": "Open resolver" } }, "vm": { diff --git a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs index a74a9b8b..9ea22681 100644 --- a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs @@ -35,7 +35,10 @@ public interface IWorkerClient : INotifyPropertyChanged event Action? PlanningMergeStartedEvent; event Action? PlanningSubtaskMergedEvent; - event Action>? PlanningMergeConflictEvent; + /// (planningTaskId, subtaskId, conflictedFiles, externallyDriven). externallyDriven + /// 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; event Action? PlanningCompletedEvent; @@ -117,6 +120,9 @@ public interface IWorkerClient : INotifyPropertyChanged Task BuildPlanningIntegrationBranchAsync(string planningTaskId, string targetBranch); Task ContinuePlanningMergeAsync(string planningTaskId); Task AbortPlanningMergeAsync(string planningTaskId); + /// Unit merges currently paused on a conflict that an MCP session started. Called + /// on (re)connect to recover the "don't auto-open" banner state after a UI restart. + Task> GetActiveExternalPlanningMergeConflictsAsync(); Task QueuePlanningSubtasksAsync(string parentTaskId, CancellationToken ct = default); Task GetWeekReportAsync(DateOnly start, DateOnly end); Task GenerateWeekReportAsync(DateOnly start, DateOnly end); diff --git a/src/ClaudeDo.Ui/Services/WorkerClient.cs b/src/ClaudeDo.Ui/Services/WorkerClient.cs index 553551d1..bc36efdb 100644 --- a/src/ClaudeDo.Ui/Services/WorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/WorkerClient.cs @@ -66,7 +66,7 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC public event Action? PlanningMergeStartedEvent; public event Action? PlanningSubtaskMergedEvent; - public event Action>? PlanningMergeConflictEvent; + public event Action, bool>? PlanningMergeConflictEvent; public event Action? PlanningMergeAbortedEvent; public event Action? PlanningCompletedEvent; @@ -182,9 +182,9 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC Dispatcher.UIThread.Post(() => PlanningSubtaskMergedEvent?.Invoke(planningTaskId, subtaskId)); }); - _hub.On>("PlanningMergeConflict", (planningTaskId, subtaskId, conflictedFiles) => + _hub.On, bool>("PlanningMergeConflict", (planningTaskId, subtaskId, conflictedFiles, externallyDriven) => { - Dispatcher.UIThread.Post(() => PlanningMergeConflictEvent?.Invoke(planningTaskId, subtaskId, conflictedFiles)); + Dispatcher.UIThread.Post(() => PlanningMergeConflictEvent?.Invoke(planningTaskId, subtaskId, conflictedFiles, externallyDriven)); }); _hub.On("PlanningMergeAborted", planningTaskId => @@ -578,6 +578,10 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC await _hub.InvokeAsync("AbortPlanningMerge", planningTaskId); } + public async Task> GetActiveExternalPlanningMergeConflictsAsync() + => await TryInvokeAsync>("GetActiveExternalPlanningMergeConflicts") + ?? []; + public async Task QueuePlanningSubtasksAsync(string parentTaskId, CancellationToken ct = default) { await _hub.InvokeAsync("QueuePlanningSubtasksAsync", parentTaskId, ct); @@ -692,6 +696,7 @@ public sealed record LaunchSpec( IReadOnlyDictionary Env); public sealed record ForceRemoveResultDto(bool Removed, string? Reason); +public sealed record PlanningMergeConflictStateDto(string PlanningTaskId, string SubtaskId); public sealed record PendingQuestionDto(string TaskId, string QuestionId, string Question); public sealed record OnlineInboxStateDto( diff --git a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs index a64631e4..2f7884ec 100644 --- a/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/IslandsShellViewModel.cs @@ -102,6 +102,12 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable [ObservableProperty] private string? _updateBannerLatestVersion; private bool _bannerDismissedThisSession; + // planningTaskId -> subtaskId, for unit-merge conflicts an MCP session (not the UI) started. + // Kept as a dictionary so a later conflict on the same planning task updates in place instead + // of piling up duplicate entries. + private readonly Dictionary _externalMergeConflicts = new(); + [ObservableProperty] private bool _isExternalMergeBannerVisible; + [ObservableProperty] private double _windowWidth = 1280; @@ -178,13 +184,59 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable _primeStatusTimer.Start(); } - private void OnPlanningMergeConflict(string planningTaskId, string subtaskId, IReadOnlyList conflictedFiles) + public void OnPlanningMergeConflict( + string planningTaskId, string subtaskId, IReadOnlyList conflictedFiles, bool externallyDriven) { // Already on UI thread (WorkerClient dispatches via Dispatcher.UIThread.Post). + if (externallyDriven) + { + // An MCP session (review_task/continue_merge) is driving this merge — it owns + // resolution. Auto-opening the resolver here raced with the session's own writes + // (two parties editing the same shared checkout at once); show a banner instead and + // leave the resolver reachable only via a deliberate click. + _externalMergeConflicts[planningTaskId] = subtaskId; + IsExternalMergeBannerVisible = true; + return; + } + // A unit-merge conflict resolves in the same in-app 3-way editor as a single-task merge. _ = OpenPlanningConflictAsync(planningTaskId, subtaskId); } + public void OnPlanningMergeAborted(string planningTaskId) => ClearExternalMergeConflict(planningTaskId); + public void OnPlanningMergeCompleted(string planningTaskId) => ClearExternalMergeConflict(planningTaskId); + + private void ClearExternalMergeConflict(string planningTaskId) + { + _externalMergeConflicts.Remove(planningTaskId); + IsExternalMergeBannerVisible = _externalMergeConflicts.Count > 0; + } + + /// Re-syncs the external-merge banner from the worker on (re)connect — the + /// one-shot PlanningMergeConflict broadcast isn't replayed after a UI restart, so this is + /// the recovery path. The worker checks MERGE_HEAD before reporting a conflict as active, + /// so a session that died mid-merge without cleaning up doesn't leave a stale banner up. + private async Task RefreshExternalMergeConflictsAsync() + { + if (Worker is null) return; + IReadOnlyList active; + try { active = await Worker.GetActiveExternalPlanningMergeConflictsAsync(); } + catch { return; } + + _externalMergeConflicts.Clear(); + foreach (var c in active) + _externalMergeConflicts[c.PlanningTaskId] = c.SubtaskId; + IsExternalMergeBannerVisible = _externalMergeConflicts.Count > 0; + } + + [RelayCommand] + private Task OpenExternalMergeConflictAsync() + { + if (_externalMergeConflicts.Count == 0) return Task.CompletedTask; + var (planningTaskId, subtaskId) = _externalMergeConflicts.First(); + return OpenPlanningConflictAsync(planningTaskId, subtaskId); + } + private async Task OpenPlanningConflictAsync(string planningTaskId, string subtaskId) { if (ConflictResolverFactory is null || Dialogs is null) return; @@ -287,6 +339,9 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable }; Worker.WorkerLogReceivedEvent += OnWorkerLogReceived; Worker.PlanningMergeConflictEvent += OnPlanningMergeConflict; + Worker.PlanningMergeAbortedEvent += OnPlanningMergeAborted; + Worker.PlanningCompletedEvent += OnPlanningMergeCompleted; + Worker.ConnectionRestoredEvent += () => _ = RefreshExternalMergeConflictsAsync(); Worker.PrimeFired += OnPrimeFired; _clearTimer.Elapsed += (_, _) => { diff --git a/src/ClaudeDo.Ui/Views/MainWindow.axaml b/src/ClaudeDo.Ui/Views/MainWindow.axaml index cfb59514..360e6b9a 100644 --- a/src/ClaudeDo.Ui/Views/MainWindow.axaml +++ b/src/ClaudeDo.Ui/Views/MainWindow.axaml @@ -258,5 +258,27 @@ + + + + + +