diff --git a/src/ClaudeDo.Data/Git/GitService.cs b/src/ClaudeDo.Data/Git/GitService.cs index 6fcd0f3a..5cb0fb30 100644 --- a/src/ClaudeDo.Data/Git/GitService.cs +++ b/src/ClaudeDo.Data/Git/GitService.cs @@ -356,6 +356,38 @@ public sealed class GitService throw new InvalidOperationException($"git revert --abort failed (exit {exitCode}): {stderr}"); } + /// + /// Re-applies a merge commit's own diff (against its first parent, `-m 1`) as a new commit on + /// top of HEAD. Needed to redo a merge that was previously reverted: once a branch tip is an + /// ancestor of HEAD, `git merge` on that branch again is a silent "Already up to date." no-op + /// (ancestry, not content, is what merge checks) -- cherry-picking the original merge commit's + /// diff is what actually restores the content. + /// + public async Task<(int ExitCode, string Stderr)> CherryPickMergeCommitAsync( + string repoDir, string mergeCommitSha, CancellationToken ct = default) + { + var (exitCode, _, stderr) = await RunGitAsync(repoDir, + ["-c", "merge.conflictStyle=diff3", "cherry-pick", "-m", "1", "--no-edit", mergeCommitSha], ct); + return (exitCode, stderr); + } + + public async Task IsMidCherryPickAsync(string repoDir, CancellationToken ct = default) + { + var (exitCode, stdout, _) = await RunGitAsync(repoDir, ["rev-parse", "--git-dir"], ct); + if (exitCode != 0) return false; + var gitDir = stdout.Trim(); + if (!Path.IsPathRooted(gitDir)) + gitDir = Path.Combine(repoDir, gitDir); + return File.Exists(Path.Combine(gitDir, "CHERRY_PICK_HEAD")); + } + + public async Task CherryPickAbortAsync(string repoDir, CancellationToken ct = default) + { + var (exitCode, _, stderr) = await RunGitAsync(repoDir, ["cherry-pick", "--abort"], ct); + if (exitCode != 0) + throw new InvalidOperationException($"git cherry-pick --abort failed (exit {exitCode}): {stderr}"); + } + public async Task> ListConflictedFilesAsync(string repoDir, CancellationToken ct = default) { var (exitCode, stdout, stderr) = await RunGitAsync(repoDir, diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index 146a01b5..257af556 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -986,6 +986,48 @@ public sealed class TaskMergeService if (task.Status != TaskStatus.WaitingForReview) return Blocked("task is not waiting for review"); + if (wt is not null && wt.State == WorktreeState.Kept) + { + // Kept means either RevertMergeAsync undid a previous merge, or the user manually + // parked an active worktree -- either way its branch may still exist. Re-running the + // real merge (not the "nothing to merge" no-op below) is the only way approve can be + // trusted after a revert; if the branch is truly gone (the common case once the + // original approve already deleted it via removeWorktree:true) there is nothing left + // to merge and that must come back as Blocked, never a silent fake "merged". + if (string.IsNullOrWhiteSpace(list.WorkingDir) || !await _git.IsGitRepoAsync(list.WorkingDir, ct)) + return Blocked("list has no working directory"); + + var branches = await _git.ListLocalBranchesAsync(list.WorkingDir, ct); + if (!branches.Contains(wt.BranchName, StringComparer.Ordinal)) + return Blocked( + $"worktree was reverted and its branch '{wt.BranchName}' no longer exists — cannot re-approve; discard the task or start a fresh worktree instead."); + + var reviveTarget = string.IsNullOrWhiteSpace(targetBranch) + ? await _git.GetCurrentBranchAsync(list.WorkingDir, ct) + : targetBranch; + + // A branch that was already merged once (reverted or not) is already an ancestor of + // the target tip -- `git merge` on it again is a silent "Already up to date." no-op, + // since merge only checks ancestry, never content. Detect that case up front and + // reapply the original merge commit's own diff instead of falling into the normal + // merge path below, which would report success without actually restoring anything. + var branchTip = await _git.RevParseAsync(list.WorkingDir, wt.BranchName, ct); + var targetTip = await _git.RevParseAsync(list.WorkingDir, reviveTarget, ct); + if (await _git.IsAncestorAsync(list.WorkingDir, branchTip, targetTip, ct) == true) + { + if (string.IsNullOrWhiteSpace(wt.MergeCommit)) + return Blocked("worktree branch has no new commits to merge and no merge commit is recorded to reapply"); + + return await ReapplyRevertedMergeAsync( + task, list, wt, reviveTarget, verifyCommand, ct, progress, skipVerify); + } + + using (var ctx = _dbFactory.CreateDbContext()) + await new WorktreeRepository(ctx).SetStateAsync(taskId, WorktreeState.Active, ct); + await _broadcaster.WorktreeUpdated(taskId); + wt.State = WorktreeState.Active; + } + if (wt is null || wt.State != WorktreeState.Active) { // There is nothing left to merge -- a sandbox run, or a list-handler task that @@ -1031,6 +1073,107 @@ public sealed class TaskMergeService return await MergeAsync(taskId, target, removeWorktree: true, commitMessage: "", leaveConflictsInTree, ct, progress, skipVerify); } + /// + /// Redoes a merge that undid (or that predates it, if the branch + /// was already merged before its worktree reached ), by + /// cherry-picking the original merge commit's own diff onto -- + /// see for why a plain re-merge can't be + /// used once the branch tip is already an ancestor of the target. Mirrors 's + /// shape (gate, dirty/mid-op checks, cleanup, verify gate, approve) but produces a cherry-pick + /// commit instead of a merge commit, and still + /// records it as this task's . + /// + private async Task ReapplyRevertedMergeAsync( + TaskEntity task, ListEntity list, WorktreeEntity wt, string targetBranch, string? verifyCommand, + CancellationToken ct, IProgress? progress, bool skipVerify) + { + var taskId = task.Id; + var workingDir = list.WorkingDir!; + await _broadcaster.OperationProgress(taskId, PhaseMerging, 0, 0); + + var gate = GetMergeGate(workingDir); + await gate.WaitAsync(ct); + try + { + if (await _git.IsMidMergeAsync(workingDir, ct)) + return Blocked("target working directory is mid-merge"); + if (await _git.IsMidCherryPickAsync(workingDir, ct)) + return Blocked("target working directory is mid-cherry-pick"); + if (await _git.HasChangesAsync(workingDir, includeUntracked: false, ct)) + return Blocked("target working tree has uncommitted changes"); + + var currentBranch = await _git.GetCurrentBranchAsync(workingDir, ct); + if (!string.Equals(currentBranch, targetBranch, StringComparison.Ordinal)) + { + try { await _git.CheckoutBranchAsync(workingDir, targetBranch, ct); } + catch (Exception ex) { return Blocked($"failed to switch target branch: {ex.Message}"); } + } + + var oldTargetTip = await _git.RevParseHeadAsync(workingDir, ct); + + var (exitCode, stderr) = await _git.CherryPickMergeCommitAsync(workingDir, wt.MergeCommit!, ct); + if (exitCode != 0) + { + try { await _git.CherryPickAbortAsync(workingDir, ct); } + catch (Exception ex) + { + _logger.LogError(ex, "git cherry-pick --abort failed after conflict — repo is mid-cherry-pick"); + return Blocked($"reapply conflict and abort failed: {ex.Message} — repo is mid-cherry-pick, resolve manually"); + } + return Blocked($"reapplying the reverted merge conflicted and was aborted: {stderr}"); + } + + var mergeSha = await _git.RevParseHeadAsync(workingDir, ct); + + string? cleanupWarning = null; + try + { + await _git.WorktreeRemoveAsync(workingDir, wt.Path, force: false, ct); + try { await _git.BranchDeleteAsync(workingDir, wt.BranchName, force: false, ct); } + catch (Exception ex) + { + _logger.LogWarning(ex, "branch delete failed for {Branch}", wt.BranchName); + cleanupWarning = $"worktree removed, branch delete failed: {ex.Message}"; + } + } + catch (Exception ex) + { + _logger.LogWarning(ex, "worktree remove failed for {Path}", wt.Path); + cleanupWarning = $"worktree remove failed: {ex.Message}"; + } + + await MarkWorktreeMergedAsync(taskId, mergeSha, ct); + await RebaseOthersAfterMergeAsync(task, list, targetBranch, oldTargetTip, mergeSha, ct); + + if (!string.IsNullOrWhiteSpace(verifyCommand) && skipVerify) + { + await _broadcaster.WorkerLog( + $"Re-merged #{task.Number} \"{task.Title}\" into {targetBranch} after a revert — verify deferred to batch", + WorkerLogLevel.Info, DateTime.UtcNow); + return new MergeResult(StatusMergedVerifyPending, Array.Empty(), cleanupWarning); + } + + var verifyFailure = await RunVerifyGateAsync( + verifyCommand, workingDir, ct, progress, + elapsed => _ = _broadcaster.OperationProgress(taskId, PhaseVerifying, (int)elapsed.TotalSeconds, 0)); + if (verifyFailure is not null) + { + _logger.LogWarning("Verify command failed after reapplying reverted merge for task {TaskId}: {Reason}", taskId, verifyFailure.ErrorMessage); + await _broadcaster.WorkerLog($"Verify failed for #{task.Number} \"{task.Title}\" after re-merge into {targetBranch}", WorkerLogLevel.Warn, DateTime.UtcNow); + return verifyFailure; + } + + await ApproveIfWaitingForReviewAsync(task, ct); + _logger.LogInformation( + "Reapplied reverted merge for task {TaskId} branch {Branch} into {Target}", + taskId, wt.BranchName, targetBranch); + await _broadcaster.WorkerLog($"Re-merged #{task.Number} \"{task.Title}\" into {targetBranch} after a revert", WorkerLogLevel.Success, DateTime.UtcNow); + + return new MergeResult(StatusMerged, Array.Empty(), cleanupWarning); + } + finally { gate.Release(); } + } + /// /// The batch counterpart to the per-merge verify gate: runs the list's verify command ONCE /// (under the same per-repo gate) and, on success, promotes every listed task to Done via diff --git a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs index 1917bfd4..103e69aa 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs @@ -1536,6 +1536,91 @@ public class TaskMergeServiceTests : IDisposable Assert.Contains(proxy.Calls, c => c.Method == "WorktreeUpdated" && c.Args[0] is string s && s == task.Id); } + [Fact] + public async Task ApproveAndMergeAsync_AfterRevert_BranchGone_ReturnsBlockedInsteadOfFakeMerge() + { + if (!GitRepoFixture.IsGitAvailable()) return; + + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, workingDir: repo.RepoDir, status: TaskStatus.WaitingForReview); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "new\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var (svc, _) = BuildService(db); + var currentBranch = await new GitService().GetCurrentBranchAsync(repo.RepoDir); + + // Approve removes the worktree AND deletes its branch (removeWorktree: true), so after a + // later revert there is genuinely nothing left to re-merge. + var approveResult = await svc.ApproveAndMergeAsync(task.Id, currentBranch, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusMerged, approveResult.Status); + + var revertResult = await svc.RevertMergeAsync(task.Id, currentBranch, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusReverted, revertResult.Status); + + var reapproveResult = await svc.ApproveAndMergeAsync(task.Id, currentBranch, CancellationToken.None); + + Assert.Equal(TaskMergeService.StatusBlocked, reapproveResult.Status); + Assert.Contains("branch", reapproveResult.ErrorMessage ?? "", StringComparison.OrdinalIgnoreCase); + Assert.Contains("no longer exists", reapproveResult.ErrorMessage ?? ""); + + using var ctx = db.CreateContext(); + var updated = await new TaskRepository(ctx).GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.WaitingForReview, updated!.Status); + var wt = await new WorktreeRepository(ctx).GetByTaskIdAsync(task.Id); + Assert.Equal(WorktreeState.Kept, wt!.State); + } + + [Fact] + public async Task ApproveAndMergeAsync_AfterRevert_BranchStillExists_ReallyMergesAgain() + { + if (!GitRepoFixture.IsGitAvailable()) return; + + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, workingDir: repo.RepoDir, status: TaskStatus.WaitingForReview); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "new\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var (svc, _) = BuildService(db); + var currentBranch = await new GitService().GetCurrentBranchAsync(repo.RepoDir); + + // Merge (not approve) with removeWorktree: false, so the branch and worktree dir survive + // the merge -- e.g. a manual merge_task call, or a merge whose cleanup was skipped. + var mergeResult = await svc.MergeAsync(task.Id, currentBranch, removeWorktree: false, + commitMessage: "Merge task", ct: CancellationToken.None); + Assert.Equal(TaskMergeService.StatusMerged, mergeResult.Status); + + string? firstMergeCommit; + using (var preRevertCtx = db.CreateContext()) + firstMergeCommit = (await new WorktreeRepository(preRevertCtx).GetByTaskIdAsync(task.Id))!.MergeCommit; + + var revertResult = await svc.RevertMergeAsync(task.Id, currentBranch, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusReverted, revertResult.Status); + Assert.False(File.Exists(Path.Combine(repo.RepoDir, "added.txt"))); + + var reapproveResult = await svc.ApproveAndMergeAsync(task.Id, currentBranch, CancellationToken.None); + + Assert.Equal(TaskMergeService.StatusMerged, reapproveResult.Status); + Assert.True(File.Exists(Path.Combine(repo.RepoDir, "added.txt"))); + + using var ctx = db.CreateContext(); + var updated = await new TaskRepository(ctx).GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Done, updated!.Status); + var wt = await new WorktreeRepository(ctx).GetByTaskIdAsync(task.Id); + Assert.Equal(WorktreeState.Merged, wt!.State); + Assert.False(string.IsNullOrWhiteSpace(wt.MergeCommit)); + Assert.NotEqual(firstMergeCommit, wt.MergeCommit); + } + [Fact] public async Task RevertMergeAsync_UncommittedChangesInTarget_ReturnsBlocked() {