diff --git a/src/ClaudeDo.Data/Git/GitService.cs b/src/ClaudeDo.Data/Git/GitService.cs index aa10b335..6fcd0f3a 100644 --- a/src/ClaudeDo.Data/Git/GitService.cs +++ b/src/ClaudeDo.Data/Git/GitService.cs @@ -376,6 +376,32 @@ public sealed class GitService throw new InvalidOperationException($"git add '{path}' failed (exit {exitCode}): {stderr}"); } + /// + /// Stages as deleted (`git rm -f`), removing it from both the index + /// and, if present, the working tree. Used to resolve a merge conflict to "keep the + /// deletion" — unlike , which would stage an empty file as a + /// tracked, zero-byte file rather than actually removing it. + /// + public async Task RemovePathAsync(string repoDir, string path, CancellationToken ct = default) + { + var (exitCode, _, stderr) = await RunGitAsync(repoDir, ["rm", "-f", "--", path], ct); + if (exitCode != 0) + throw new InvalidOperationException($"git rm '{path}' failed (exit {exitCode}): {stderr}"); + } + + /// + /// Content of at a merge stage (1=base, 2=ours, 3=theirs) via + /// `git show :<stage>:<path>`. Null when that stage doesn't exist for the path — + /// the conventional way to tell a modify/delete or rename/delete conflict apart from an + /// ordinary content conflict (git never writes `<<<<<<<` markers for those, + /// it just leaves whichever side wasn't deleted in the working tree). + /// + public async Task ShowConflictStageAsync(string repoDir, int stage, string path, CancellationToken ct = default) + { + var (exitCode, stdout, _) = await RunGitAsync(repoDir, ["show", $":{stage}:{path}"], ct, trimOutput: false); + return exitCode == 0 ? stdout : null; + } + /// /// Non-destructive mergeability probe via `git merge-tree --write-tree`. Writes only /// loose objects — the working tree, index, and refs are left untouched. diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index e37059b1..59ddc1f6 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -520,20 +520,48 @@ public sealed class TaskMergeService var oldTargetTip = await _git.RevParseHeadAsync(list.WorkingDir, ct); // Validate BEFORE staging: `git add` marks a conflicted path resolved regardless of - // its content, so an unresolved file with markers still in it would otherwise get - // staged (and committed) as-is. Check text content for markers first; binary files - // can't carry markers, so they're left to the post-stage index check below. + // its content, so an unresolved file needs a positively-checked resolution, not just + // "whatever happens to be on disk". For an ordinary content conflict, git always + // starts the file with markers, so their absence reliably means someone resolved it + // (in-app or in an external editor). A modify/delete or rename/delete conflict never + // gets markers at all (GetConflictDocumentsAsync) — the same marker-less state also + // describes a file nobody has touched — so those are additionally checked against + // what git itself left on disk by default (whichever side wasn't deleted); still + // matching that default means still unresolved. var unresolved = await _git.ListConflictedFilesAsync(list.WorkingDir, ct); var stillConflicted = new List(); + var toAdd = new List(); + var toRemove = new List(); foreach (var path in unresolved) { var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); - string text; - try { text = await File.ReadAllTextAsync(full, ct); } - catch { continue; } + string? text = null; + if (File.Exists(full)) + { + try { text = await File.ReadAllTextAsync(full, ct); } + catch { /* treated as unresolved below */ } + } - if (!LooksBinary(text) && ConflictMarkerParser.HasConflicts(text)) + if (text is not null && (LooksBinary(text) || ConflictMarkerParser.HasConflicts(text))) + { stillConflicted.Add(path); + continue; + } + + var oursStage = await _git.ShowConflictStageAsync(list.WorkingDir, 2, path, ct); + var theirsStage = await _git.ShowConflictStageAsync(list.WorkingDir, 3, path, ct); + var isModifyDeleteStyle = oursStage is null || theirsStage is null; + if (isModifyDeleteStyle && + NormalizeLineEndings(text ?? "") == NormalizeLineEndings(oursStage ?? theirsStage ?? "")) + { + stillConflicted.Add(path); + continue; + } + + if (text is null || text.Length == 0) + toRemove.Add(path); + else + toAdd.Add(path); } if (stillConflicted.Count > 0) @@ -550,8 +578,10 @@ public sealed class TaskMergeService // Stage exactly the resolved conflict paths — never `git add -A`, which would sweep // untracked/unrelated changes left by other sessions into this merge commit (the // target working dir is shared). - foreach (var path in unresolved) + foreach (var path in toAdd) await _git.AddPathAsync(list.WorkingDir, path, ct); + foreach (var path in toRemove) + await _git.RemovePathAsync(list.WorkingDir, path, ct); var remaining = await _git.ListConflictedFilesAsync(list.WorkingDir, ct); if (remaining.Count > 0) @@ -680,6 +710,15 @@ public sealed class TaskMergeService /// /// Reads each conflicted working-tree file and parses its conflict markers into line-level /// segments (with the diff3 merge base when present). Binary files are flagged and skipped. + /// A path with no text markers on disk is either an ordinary content conflict that's already + /// been resolved (git keeps both index stages until the path is staged, whether or not the + /// working tree still looks conflicted — reads as a single stable segment, 0 hunks left), or + /// a modify/delete or rename/delete conflict — git never writes markers for those at all, it + /// just leaves whichever side wasn't deleted sitting in the working tree, so the same + /// "no markers" state also describes an untouched file. The index tells the two apart: a + /// modify/delete-style conflict only ever populates ONE of the ours/theirs stages. Those are + /// synthesized into a single whole-file conflict block straight from the stages (empty side + /// = that side deleted the path) so the resolver still shows a real choice. /// public async Task GetConflictDocumentsAsync(string taskId, CancellationToken ct) { @@ -692,21 +731,53 @@ public sealed class TaskMergeService foreach (var path in files) { var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); - string text; - try { text = await File.ReadAllTextAsync(full, ct); } - catch { text = ""; } + string? diskText = null; + if (File.Exists(full)) + { + try { diskText = await File.ReadAllTextAsync(full, ct); } + catch { /* unreadable — fall through to the index stages below */ } + } - if (LooksBinary(text)) + if (diskText is not null && LooksBinary(diskText)) { result.Add(new ConflictDocumentContent(path, true, Array.Empty())); continue; } - result.Add(new ConflictDocumentContent(path, false, ConflictMarkerParser.Parse(text))); + if (diskText is not null && ConflictMarkerParser.HasConflicts(diskText)) + { + result.Add(new ConflictDocumentContent(path, false, ConflictMarkerParser.Parse(diskText))); + continue; + } + + var ours = await _git.ShowConflictStageAsync(list.WorkingDir, 2, path, ct); + var theirs = await _git.ShowConflictStageAsync(list.WorkingDir, 3, path, ct); + if (ours is not null && theirs is not null) + { + // Ordinary two-sided conflict, already resolved (no markers left) but not yet staged. + result.Add(new ConflictDocumentContent(path, false, + new[] { MergeSegment.Stable(diskText ?? "") })); + continue; + } + + if (LooksBinary(ours ?? "") || LooksBinary(theirs ?? "")) + { + result.Add(new ConflictDocumentContent(path, true, Array.Empty())); + continue; + } + + var @base = await _git.ShowConflictStageAsync(list.WorkingDir, 1, path, ct); + result.Add(new ConflictDocumentContent( + path, false, new[] { MergeSegment.Conflict(ours ?? "", @base, theirs ?? "") })); } return new ConflictDocuments(taskId, result); } + // Working-tree checkouts can go through autocrlf while `git show :stage:path` never does — + // normalize before comparing the two or an untouched file reads as "resolved" on a machine + // with autocrlf enabled. + private static string NormalizeLineEndings(string text) => text.Replace("\r\n", "\n").Replace('\r', '\n'); + // A NUL byte in the head of the file is the conventional binary sniff. private static bool LooksBinary(string text) { @@ -722,6 +793,15 @@ public sealed class TaskMergeService if (string.IsNullOrWhiteSpace(list.WorkingDir)) throw new InvalidOperationException("list has no working directory"); + if (content.Length == 0) + { + // An empty resolution for a whole-file conflict (modify/delete, rename/delete) means + // "keep the deletion" — `git add` on an empty file would instead stage it as a + // tracked, zero-byte file, which is not what accepting the deleted side means. + await _git.RemovePathAsync(list.WorkingDir, path, ct); + return; + } + var full = Path.Combine(list.WorkingDir, path.Replace('/', Path.DirectorySeparatorChar)); await File.WriteAllTextAsync(full, content, ct); await _git.AddPathAsync(list.WorkingDir, path, ct); diff --git a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs index 7db0c193..af53837d 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs @@ -1109,6 +1109,63 @@ public class TaskMergeServiceTests : IDisposable Assert.Contains(file.Segments, s => !s.IsConflict && s.Text.Contains("line1")); } + // git never writes <<<<<<< markers for a modify/delete conflict -- it just leaves the + // modifying side's content sitting in the working tree, which used to read as an + // already-resolved stable file (no conflict at all) to both GetConflictDocumentsAsync and + // ContinueMergeAsync's marker scan, so `continue_merge` would silently keep the modification. + [Fact] + public async Task ModifyDeleteConflict_IsSynthesizedAndCannotBeSilentlyStaged() + { + if (!GitRepoFixture.IsGitAvailable()) return; + + var db = NewDb(); + var repo = NewRepo(); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}"); + _wtCleanups.Add((repo.RepoDir, wtPath)); + GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", "claudedo/modify-delete", wtPath, repo.BaseCommit); + File.WriteAllText(Path.Combine(wtPath, "README.md"), "# modified by branch\n"); + GitRepoFixture.RunGit(wtPath, "commit", "-am", "branch modifies README"); + + GitRepoFixture.RunGit(repo.RepoDir, "rm", "README.md"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "main deletes README"); + + var (_, task) = await SeedListAndTask(db, workingDir: repo.RepoDir, status: TaskStatus.Done); + await SeedWorktree(db, task.Id, wtPath, "claudedo/modify-delete", repo.BaseCommit); + + var (svc, _) = BuildService(db); + var merge = await svc.MergeAsync(task.Id, "main", removeWorktree: false, "msg", + leaveConflictsInTree: true, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusConflict, merge.Status); + + // No <<<<<<< markers anywhere -- git left the branch's modification as-is. + Assert.DoesNotContain("<<<<<<<", File.ReadAllText(Path.Combine(repo.RepoDir, "README.md"))); + + var docs = await svc.GetConflictDocumentsAsync(task.Id, CancellationToken.None); + var file = Assert.Single(docs.Files); + Assert.EndsWith("README.md", file.Path.Replace('\\', '/')); + Assert.False(file.IsBinary); + var conflict = Assert.Single(file.Segments.Where(s => s.IsConflict).ToList()); + Assert.Equal("", conflict.Ours); // ours (main) deleted it + Assert.Contains("modified by branch", conflict.Theirs); // theirs (branch) modified it + + // Nobody resolved it — continue must refuse rather than silently keeping the modification. + var headBefore = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim(); + var refused = await svc.ContinueMergeAsync(task.Id, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusConflict, refused.Status); + Assert.Contains("README.md", refused.ConflictFiles); + Assert.Equal(headBefore, GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim()); + Assert.True(await new GitService().IsMidMergeAsync(repo.RepoDir)); + + // Explicitly resolving to the deleted (ours) side must `git rm`, not stage an empty file. + await svc.WriteResolutionAsync(task.Id, "README.md", "", CancellationToken.None); + var result = await svc.ContinueMergeAsync(task.Id, CancellationToken.None); + Assert.Equal(TaskMergeService.StatusMerged, result.Status); + Assert.False(File.Exists(Path.Combine(repo.RepoDir, "README.md"))); + Assert.False(await new GitService().IsMidMergeAsync(repo.RepoDir)); + } + [Fact] public async Task ApproveAndMergeAsync_NoWorktree_MarksDone() {