fix(worker): make re-approve after revert_merge do a real merge, not a silent no-op
ApproveAndMergeAsync treated any non-Active worktree (including Kept, the
state RevertMergeAsync leaves behind) the same as "never had a worktree",
so re-approving a reverted task ran only the verify gate and marked the
task Done without merging anything back.
Now a Kept worktree is distinguished: if its branch is gone (the common
case, since approve normally deletes it), approve returns Blocked instead
of a fake "merged". If the branch survives, its tip is already an ancestor
of the target (from the original merge), so a plain `git merge` would
silently no-op ("Already up to date."); cherry-picking the original merge
commit's own diff (-m 1) is what actually restores the content, recorded
as a fresh MergeCommit.
This commit is contained in:
@@ -356,6 +356,38 @@ public sealed class GitService
|
|||||||
throw new InvalidOperationException($"git revert --abort failed (exit {exitCode}): {stderr}");
|
throw new InvalidOperationException($"git revert --abort failed (exit {exitCode}): {stderr}");
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// 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.
|
||||||
|
/// </summary>
|
||||||
|
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<bool> 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<List<string>> ListConflictedFilesAsync(string repoDir, CancellationToken ct = default)
|
public async Task<List<string>> ListConflictedFilesAsync(string repoDir, CancellationToken ct = default)
|
||||||
{
|
{
|
||||||
var (exitCode, stdout, stderr) = await RunGitAsync(repoDir,
|
var (exitCode, stdout, stderr) = await RunGitAsync(repoDir,
|
||||||
|
|||||||
@@ -986,6 +986,48 @@ public sealed class TaskMergeService
|
|||||||
if (task.Status != TaskStatus.WaitingForReview)
|
if (task.Status != TaskStatus.WaitingForReview)
|
||||||
return Blocked("task is not waiting for review");
|
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)
|
if (wt is null || wt.State != WorktreeState.Active)
|
||||||
{
|
{
|
||||||
// There is nothing left to merge -- a sandbox run, or a list-handler task that
|
// 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);
|
return await MergeAsync(taskId, target, removeWorktree: true, commitMessage: "", leaveConflictsInTree, ct, progress, skipVerify);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Redoes a merge that <see cref="RevertMergeAsync"/> undid (or that predates it, if the branch
|
||||||
|
/// was already merged before its worktree reached <see cref="WorktreeState.Kept"/>), by
|
||||||
|
/// cherry-picking the original merge commit's own diff onto <paramref name="targetBranch"/> --
|
||||||
|
/// see <see cref="GitService.CherryPickMergeCommitAsync"/> for why a plain re-merge can't be
|
||||||
|
/// used once the branch tip is already an ancestor of the target. Mirrors <see cref="MergeAsync"/>'s
|
||||||
|
/// shape (gate, dirty/mid-op checks, cleanup, verify gate, approve) but produces a cherry-pick
|
||||||
|
/// commit instead of a merge commit, and <see cref="WorktreeRepository.SetMergedAsync"/> still
|
||||||
|
/// records it as this task's <see cref="WorktreeEntity.MergeCommit"/>.
|
||||||
|
/// </summary>
|
||||||
|
private async Task<MergeResult> ReapplyRevertedMergeAsync(
|
||||||
|
TaskEntity task, ListEntity list, WorktreeEntity wt, string targetBranch, string? verifyCommand,
|
||||||
|
CancellationToken ct, IProgress<ProgressNotificationValue>? 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<string>(), 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<string>(), cleanupWarning);
|
||||||
|
}
|
||||||
|
finally { gate.Release(); }
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>
|
/// <summary>
|
||||||
/// The batch counterpart to the per-merge verify gate: runs the list's verify command ONCE
|
/// 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
|
/// (under the same per-repo gate) and, on success, promotes every listed task to Done via
|
||||||
|
|||||||
@@ -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);
|
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]
|
[Fact]
|
||||||
public async Task RevertMergeAsync_UncommittedChangesInTarget_ReturnsBlocked()
|
public async Task RevertMergeAsync_UncommittedChangesInTarget_ReturnsBlocked()
|
||||||
{
|
{
|
||||||
|
|||||||
Reference in New Issue
Block a user