fix(claude-do): merge Bug: conflict resolver no longer opens on a merge conflict
ClaudeDo-Task: 612a70a9-814e-4300-acec-1384262588cd
This commit is contained in:
@@ -252,6 +252,11 @@ Single-task mode starts the conflict merge, then parses each conflicted file int
|
||||
conflict `MergeFileSegment`s via the worker's `GetMergeConflictDocuments`. Types live in
|
||||
`ConflictModels`: `MergeFile` / `MergeFileSegment` / `MergeConflictBlock`.
|
||||
|
||||
⚠️ **The resolver restarts the merge itself** (`StartConflictMerge`, `leaveConflictsInTree`), so the caller's
|
||||
`autostash` consent must travel with it (`MergeCoordinator.ResolveConflictAsync(..., autostash)` →
|
||||
`OpenAsync(target, autostash)` → hub). Without it a dirty target answers `dirty_target`, the
|
||||
resolver stays closed after the stash-retry approve already reported `conflict`.
|
||||
|
||||
Exposed per active file: `ActiveOursText` / `ActiveResultText` / `ActiveTheirsText`
|
||||
(reconstructed from `MergeFile.OursText/ResultText/TheirsText`; Result seeds unresolved
|
||||
conflicts with Ours), plus `ActiveFile` / `SelectFileCommand` (multi-file switcher),
|
||||
|
||||
@@ -136,7 +136,7 @@ public interface IWorkerClient : INotifyPropertyChanged
|
||||
Task CancelReviewAsync(string taskId);
|
||||
|
||||
// ── Conflict resolution (worker hub side implemented by Layer C) ──
|
||||
Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch);
|
||||
Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch, bool autostash = false);
|
||||
Task<MergeConflictDocumentsDto> GetMergeConflictDocumentsAsync(string taskId);
|
||||
Task WriteConflictResolutionAsync(string taskId, string path, string resolvedContent);
|
||||
Task<MergeResultDto> ContinueConflictMergeAsync(string taskId);
|
||||
|
||||
@@ -14,8 +14,10 @@ namespace ClaudeDo.Ui.Services;
|
||||
public sealed class MergeCoordinator
|
||||
{
|
||||
/// Set once at composition to the shell's resolver entry. Null (headless/tests) ⇒ no-op.
|
||||
public Func<string, string, Task>? Handler { get; set; }
|
||||
public Func<string, string, bool, Task>? Handler { get; set; }
|
||||
|
||||
public Task ResolveConflictAsync(string taskId, string targetBranch) =>
|
||||
Handler?.Invoke(taskId, targetBranch) ?? Task.CompletedTask;
|
||||
/// <summary>Opens the resolver for <paramref name="taskId"/>; <paramref name="autostash"/> is the
|
||||
/// caller's stash consent, repeated because the resolver restarts the merge on its own.</summary>
|
||||
public Task ResolveConflictAsync(string taskId, string targetBranch, bool autostash = false) =>
|
||||
Handler?.Invoke(taskId, targetBranch, autostash) ?? Task.CompletedTask;
|
||||
}
|
||||
|
||||
@@ -399,8 +399,8 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC
|
||||
"MergeTask", taskId, targetBranch, removeWorktree, commitMessage, autostash));
|
||||
}
|
||||
|
||||
public Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch)
|
||||
=> InvokeTimedAsync<MergeResultDto>("StartConflictMerge", () => _hub.InvokeAsync<MergeResultDto>("StartConflictMerge", taskId, targetBranch));
|
||||
public Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch, bool autostash = false)
|
||||
=> InvokeTimedAsync<MergeResultDto>("StartConflictMerge", () => _hub.InvokeAsync<MergeResultDto>("StartConflictMerge", taskId, targetBranch, autostash));
|
||||
|
||||
public Task<MergeConflictDocumentsDto> GetMergeConflictDocumentsAsync(string taskId)
|
||||
=> InvokeTimedAsync<MergeConflictDocumentsDto>("GetMergeConflictDocuments", () => _hub.InvokeAsync<MergeConflictDocumentsDto>("GetMergeConflictDocuments", taskId));
|
||||
|
||||
@@ -133,14 +133,15 @@ public sealed partial class ConflictResolverViewModel : ObservableObject
|
||||
}
|
||||
|
||||
/// <summary>Starts the conflict merge and loads the conflicted files as line-level segments.
|
||||
/// Returns true when there is something to resolve (caller should show the dialog).</summary>
|
||||
public async Task<bool> OpenAsync(string targetBranch)
|
||||
/// Returns true when there is something to resolve (caller should show the dialog).
|
||||
/// <paramref name="autostash"/> carries the caller's stash consent: a dirty target is otherwise refused.</summary>
|
||||
public async Task<bool> OpenAsync(string targetBranch, bool autostash = false)
|
||||
{
|
||||
IsBusy = true;
|
||||
Error = null;
|
||||
try
|
||||
{
|
||||
var start = await _worker.StartConflictMergeAsync(_taskId, targetBranch);
|
||||
var start = await _worker.StartConflictMergeAsync(_taskId, targetBranch, autostash);
|
||||
if (!string.Equals(start.Status, "conflict", StringComparison.Ordinal))
|
||||
{
|
||||
// Any non-conflict outcome here means nothing was left mid-merge for this dialog
|
||||
|
||||
@@ -1295,9 +1295,11 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
// Nothing landed -- the target working dir has uncommitted changes (often another
|
||||
// session's WIP in the shared main checkout). Offer the one-shot stash retry; git
|
||||
// restores the changes itself once the merge is through.
|
||||
var stashed = false;
|
||||
if (result?.Status == "dirty_target" && Dialogs != null
|
||||
&& await Dialogs.ConfirmAsync(Loc.T("vm.detailsIsland.stashAndMergeConfirm")))
|
||||
{
|
||||
stashed = true;
|
||||
ApproveOp.Report(Loc.T("ops.merge.merging"));
|
||||
result = await _worker.ApproveReviewAsync(
|
||||
taskId, Merge.SelectedMergeTarget ?? "", autostash: true);
|
||||
@@ -1306,7 +1308,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable
|
||||
if (result?.Status == "dirty_target" && Dialogs != null)
|
||||
await Dialogs.ShowErrorAsync(result.ErrorMessage ?? Loc.T("vm.detailsIsland.dirtyTarget"));
|
||||
else if (!hasChildren && result?.Status == "conflict")
|
||||
await _merge.ResolveConflictAsync(taskId, Merge.SelectedMergeTarget ?? "");
|
||||
await _merge.ResolveConflictAsync(taskId, Merge.SelectedMergeTarget ?? "", stashed);
|
||||
// The merge itself already landed; the verify command failed, so the task stayed
|
||||
// out of Done. Surface that instead of silently looking like nothing happened.
|
||||
else if (!hasChildren && result?.Status == "verify_failed" && Dialogs != null)
|
||||
|
||||
@@ -109,13 +109,15 @@ public sealed partial class IslandsShellViewModel : ViewModelBase, IDisposable
|
||||
BringToFront?.Invoke();
|
||||
}
|
||||
|
||||
public async Task RequestConflictResolutionAsync(string taskId, string targetBranch)
|
||||
public async Task RequestConflictResolutionAsync(string taskId, string targetBranch, bool autostash = false)
|
||||
{
|
||||
if (ConflictResolverFactory is null || Dialogs is null) return;
|
||||
var vm = ConflictResolverFactory(taskId);
|
||||
var hasConflicts = await vm.OpenAsync(targetBranch);
|
||||
var hasConflicts = await vm.OpenAsync(targetBranch, autostash);
|
||||
if (hasConflicts)
|
||||
await Dialogs.ShowConflictResolverAsync(vm);
|
||||
else if (vm.Error is { } error)
|
||||
FlashFooterError(error);
|
||||
}
|
||||
|
||||
[ObservableProperty] private bool _isUpdateBannerVisible;
|
||||
|
||||
@@ -139,7 +139,7 @@ public sealed partial class MergeModalViewModel : ViewModelBase
|
||||
// which re-starts the merge leaving conflicts in the tree.
|
||||
RoutedToResolver = true;
|
||||
CloseAction?.Invoke();
|
||||
await _merge.ResolveConflictAsync(TaskId, SelectedBranch!);
|
||||
await _merge.ResolveConflictAsync(TaskId, SelectedBranch!, autostash);
|
||||
break;
|
||||
case "blocked":
|
||||
ErrorMessage = Loc.T("vm.merge.blocked", result.ErrorMessage ?? "");
|
||||
|
||||
@@ -543,12 +543,12 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub
|
||||
return new MergePreviewDto(p.Status, p.ConflictFiles, p.ChangedFileCount);
|
||||
});
|
||||
|
||||
public Task<MergeResultDto> StartConflictMerge(string taskId, string targetBranch)
|
||||
public Task<MergeResultDto> StartConflictMerge(string taskId, string targetBranch, bool autostash = false)
|
||||
=> HubGuard(async () =>
|
||||
{
|
||||
var r = await _mergeService.MergeAsync(
|
||||
taskId, targetBranch ?? "", removeWorktree: false, commitMessage: "",
|
||||
leaveConflictsInTree: true, CancellationToken.None);
|
||||
leaveConflictsInTree: true, CancellationToken.None, autostash: autostash);
|
||||
if (r.Status == TaskMergeService.StatusBlocked)
|
||||
throw new HubException(r.ErrorMessage ?? "merge blocked");
|
||||
return new MergeResultDto(r.Status, r.ConflictFiles, r.ErrorMessage);
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
using System;
|
||||
using System.Threading.Tasks;
|
||||
using ClaudeDo.Data.Models;
|
||||
using ClaudeDo.Ui.Services;
|
||||
using ClaudeDo.Ui.ViewModels;
|
||||
using Xunit;
|
||||
|
||||
@@ -124,4 +125,26 @@ public class IslandsShellViewModelExternalMergeTests
|
||||
// click must degrade gracefully rather than throw, same as the existing UI-driven path.
|
||||
await vm.OpenExternalMergeConflictCommand.ExecuteAsync(null);
|
||||
}
|
||||
|
||||
private sealed class DirtyTargetWorker : StubWorkerClient
|
||||
{
|
||||
public override Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch, bool autostash = false) =>
|
||||
Task.FromResult(new MergeResultDto("dirty_target", Array.Empty<string>(), "target working tree has uncommitted changes"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task RequestConflictResolution_ResolverRefused_ShowsWhyInTheFooter()
|
||||
{
|
||||
// Regression: a refusal (dirty target, ...) left the resolver closed with no explanation anywhere.
|
||||
var vm = new IslandsShellViewModel
|
||||
{
|
||||
Dialogs = new RecordingDialogService(),
|
||||
ConflictResolverFactory = id => new ClaudeDo.Ui.ViewModels.Conflicts.ConflictResolverViewModel(new DirtyTargetWorker(), id),
|
||||
};
|
||||
|
||||
await vm.RequestConflictResolutionAsync("t1", "main");
|
||||
|
||||
Assert.Equal(WorkerLogLevel.Error, vm.WorkerLogLevel);
|
||||
Assert.Contains("uncommitted changes", vm.WorkerLogText);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -127,7 +127,7 @@ public abstract class StubWorkerClient : IWorkerClient
|
||||
public virtual Task RejectReviewToQueueAsync(string taskId, string feedback) => Task.CompletedTask;
|
||||
public virtual Task RejectReviewToIdleAsync(string taskId) => Task.CompletedTask;
|
||||
public virtual Task CancelReviewAsync(string taskId) => Task.CompletedTask;
|
||||
public virtual Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch) => Task.FromResult(new MergeResultDto("conflict", System.Array.Empty<string>(), null));
|
||||
public virtual Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch, bool autostash = false) => Task.FromResult(new MergeResultDto("conflict", System.Array.Empty<string>(), null));
|
||||
public virtual Task<MergeConflictDocumentsDto> GetMergeConflictDocumentsAsync(string taskId) => Task.FromResult(new MergeConflictDocumentsDto(taskId, System.Array.Empty<ConflictDocumentDto>()));
|
||||
public virtual Task WriteConflictResolutionAsync(string taskId, string path, string resolvedContent) => Task.CompletedTask;
|
||||
public virtual Task<MergeResultDto> ContinueConflictMergeAsync(string taskId) => Task.FromResult(new MergeResultDto("merged", System.Array.Empty<string>(), null));
|
||||
|
||||
@@ -30,9 +30,14 @@ public class ConflictResolverViewModelTests
|
||||
public string? ContinuedPlanningParent;
|
||||
public string? AbortedPlanningParent;
|
||||
|
||||
public override Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch)
|
||||
/// <summary>Models a target working tree with uncommitted changes: refused unless the caller consented to the stash.</summary>
|
||||
public bool DirtyTarget;
|
||||
|
||||
public override Task<MergeResultDto> StartConflictMergeAsync(string taskId, string targetBranch, bool autostash = false)
|
||||
{
|
||||
StartCalled = true;
|
||||
if (DirtyTarget && !autostash)
|
||||
return Task.FromResult(new MergeResultDto("dirty_target", System.Array.Empty<string>(), "target working tree has uncommitted changes"));
|
||||
return Task.FromResult(new MergeResultDto("conflict", new[] { "README.md" }, null));
|
||||
}
|
||||
|
||||
@@ -63,6 +68,19 @@ public class ConflictResolverViewModelTests
|
||||
public override Task AbortConflictMergeAsync(string taskId) { Aborted = true; return Task.CompletedTask; }
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task OpenAsync_DirtyTarget_OpensOnlyWithStashConsent()
|
||||
{
|
||||
// Regression: a dirty target refused the resolver's own merge restart, so it never opened.
|
||||
var refused = new ConflictResolverViewModel(new FakeWorker { DirtyTarget = true }, "task-1");
|
||||
Assert.False(await refused.OpenAsync("main"));
|
||||
Assert.Equal("target working tree has uncommitted changes", refused.Error);
|
||||
|
||||
var consented = new ConflictResolverViewModel(new FakeWorker { DirtyTarget = true }, "task-1");
|
||||
Assert.True(await consented.OpenAsync("main", autostash: true));
|
||||
Assert.Null(consented.Error);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task OpenAsync_LoadsConflicts_AndBlocksContinueUntilResolved()
|
||||
{
|
||||
|
||||
@@ -60,7 +60,7 @@ public class DetailsIslandConflictSeamTests : IDisposable
|
||||
string? capturedTarget = null;
|
||||
var coordinator = new MergeCoordinator
|
||||
{
|
||||
Handler = (tid, target) =>
|
||||
Handler = (tid, target, _) =>
|
||||
{
|
||||
capturedTaskId = tid;
|
||||
capturedTarget = target;
|
||||
@@ -77,4 +77,35 @@ public class DetailsIslandConflictSeamTests : IDisposable
|
||||
Assert.Equal(taskId, capturedTaskId);
|
||||
Assert.Equal("main", capturedTarget);
|
||||
}
|
||||
|
||||
/// <summary>Models the worker: a dirty target refuses the merge unless the caller consented to the stash.</summary>
|
||||
private sealed class DirtyTargetConflictWorkerClient : StubWorkerClient
|
||||
{
|
||||
public override bool IsConnected => true;
|
||||
public override Task<MergeResultDto?> ApproveReviewAsync(string taskId, string targetBranch, bool autostash = false) =>
|
||||
Task.FromResult<MergeResultDto?>(autostash
|
||||
? new MergeResultDto("conflict", new[] { "a.cs" }, null)
|
||||
: new MergeResultDto("dirty_target", Array.Empty<string>(), "target working tree has uncommitted changes"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task ApproveReview_StashRetryEndsInConflict_HandsTheStashConsentToTheResolver()
|
||||
{
|
||||
// Regression: the resolver restarts the merge itself; without the consent it hit the same
|
||||
// dirty target, got "dirty_target" and silently never opened.
|
||||
bool? capturedAutostash = null;
|
||||
var coordinator = new MergeCoordinator
|
||||
{
|
||||
Handler = (_, _, autostash) => { capturedAutostash = autostash; return Task.CompletedTask; },
|
||||
};
|
||||
|
||||
var vm = BuildVm(new DirtyTargetConflictWorkerClient(), coordinator);
|
||||
vm.Dialogs = new RecordingDialogService { ConfirmResult = true };
|
||||
vm.Bind(new TaskRowViewModel { Id = "task-dirty-1", Status = TaskStatus.WaitingForReview });
|
||||
vm.Merge.SelectedMergeTarget = "main";
|
||||
|
||||
await vm.ApproveReviewCommand.ExecuteAsync(null);
|
||||
|
||||
Assert.True(capturedAutostash);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,6 +26,9 @@ public class MergeModalViewModelTests
|
||||
/// Result for a call with autostash=false; the autostash retry always merges.
|
||||
public MergeResultDto CleanResult { get; set; } = new("merged", new List<string>(), null);
|
||||
|
||||
/// Result for the autostash retry.
|
||||
public MergeResultDto StashedResult { get; set; } = new("merged", new List<string>(), null);
|
||||
|
||||
public override Task<MergeTargetsDto?> GetMergeTargetsAsync(string taskId) =>
|
||||
Task.FromResult(Targets);
|
||||
|
||||
@@ -36,7 +39,7 @@ public class MergeModalViewModelTests
|
||||
CapturedAutostash = autostash;
|
||||
return BlockMerge
|
||||
? MergeGate.Task
|
||||
: Task.FromResult(autostash ? new MergeResultDto("merged", new List<string>(), null) : CleanResult);
|
||||
: Task.FromResult(autostash ? StashedResult : CleanResult);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -189,4 +192,26 @@ public class MergeModalViewModelTests
|
||||
|
||||
Assert.Null(vm.ProgressMessage);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Stash_retry_ending_in_conflict_hands_the_stash_consent_to_the_resolver()
|
||||
{
|
||||
// Regression: the resolver restarts the merge itself and needs the same consent, or the
|
||||
// dirty target refuses it and the resolver never opens.
|
||||
var worker = new Worker
|
||||
{
|
||||
CleanResult = new("dirty_target", new List<string>(), "uncommitted changes"),
|
||||
StashedResult = new("conflict", new List<string> { "a.cs" }, null),
|
||||
};
|
||||
bool? capturedAutostash = null;
|
||||
var vm = new MergeModalViewModel(worker, new MergeCoordinator
|
||||
{
|
||||
Handler = (_, _, autostash) => { capturedAutostash = autostash; return Task.CompletedTask; },
|
||||
});
|
||||
await vm.InitializeAsync("task-1", "do the thing");
|
||||
|
||||
await vm.StashAndMergeCommand.ExecuteAsync(null);
|
||||
|
||||
Assert.True(capturedAutostash);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -141,7 +141,7 @@ public class WorktreesOverviewBatchMergeTests
|
||||
(string Task, string Target)? captured = null;
|
||||
var coordinator = new ClaudeDo.Ui.Services.MergeCoordinator
|
||||
{
|
||||
Handler = (taskId, target) => { captured = (taskId, target); return System.Threading.Tasks.Task.CompletedTask; },
|
||||
Handler = (taskId, target, _) => { captured = (taskId, target); return System.Threading.Tasks.Task.CompletedTask; },
|
||||
};
|
||||
var vm = NewVm(coordinator);
|
||||
vm.SelectedTarget = "release";
|
||||
|
||||
@@ -1567,6 +1567,50 @@ public class TaskMergeServiceTests : IDisposable
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "merge", "--abort");
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task MergeAsync_LeaveConflicts_DirtyTarget_NeedsAutostashAndAbortRestoresTheChanges()
|
||||
{
|
||||
// The resolver restarts the merge via leaveConflictsInTree; on a dirty target that only
|
||||
// works with the caller's stash consent, and git hands the stash back on abort.
|
||||
if (!GitRepoFixture.IsGitAvailable()) return;
|
||||
|
||||
var db = NewDb();
|
||||
var repo = NewRepo();
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main");
|
||||
|
||||
File.WriteAllText(Path.Combine(repo.RepoDir, "README.md"), "# main change\n");
|
||||
File.WriteAllText(Path.Combine(repo.RepoDir, "other.txt"), "tracked\n");
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "add", "-A");
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "main change");
|
||||
|
||||
var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}");
|
||||
_wtCleanups.Add((repo.RepoDir, wtPath));
|
||||
GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", "claudedo/t1", wtPath, repo.BaseCommit);
|
||||
File.WriteAllText(Path.Combine(wtPath, "README.md"), "# branch change\n");
|
||||
GitRepoFixture.RunGit(wtPath, "commit", "-am", "branch change");
|
||||
|
||||
var (_, task) = await SeedListAndTask(db, workingDir: repo.RepoDir, status: TaskStatus.WaitingForReview);
|
||||
await SeedWorktree(db, task.Id, wtPath, "claudedo/t1", repo.BaseCommit);
|
||||
|
||||
File.WriteAllText(Path.Combine(repo.RepoDir, "other.txt"), "dirty\n");
|
||||
|
||||
var (svc, _) = BuildService(db);
|
||||
|
||||
var refused = await svc.MergeAsync(task.Id, "main", false, "msg", leaveConflictsInTree: true, CancellationToken.None);
|
||||
Assert.Equal(TaskMergeService.StatusDirtyTarget, refused.Status);
|
||||
|
||||
var result = await svc.MergeAsync(
|
||||
task.Id, "main", false, "msg", leaveConflictsInTree: true, CancellationToken.None, autostash: true);
|
||||
Assert.Equal(TaskMergeService.StatusConflict, result.Status);
|
||||
Assert.Contains("README.md", result.ConflictFiles);
|
||||
Assert.True(await new GitService().IsMidMergeAsync(repo.RepoDir));
|
||||
|
||||
await svc.AbortMergeAsync(task.Id, CancellationToken.None);
|
||||
|
||||
Assert.False(await new GitService().IsMidMergeAsync(repo.RepoDir));
|
||||
Assert.Equal("dirty\n", File.ReadAllText(Path.Combine(repo.RepoDir, "other.txt")).Replace("\r\n", "\n"));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task WriteResolutionAsync_ThenContinue_CompletesMerge()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user