From 684b0115e506ee018afb6dbaf9d4d6bcca147041 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Wed, 16 Sep 2026 12:40:01 +0200 Subject: [PATCH] feat(worker): preview_merge_set flags removed symbols and locale-key overlaps Overlap detection was file-name-only: 9/15 tasks touching en.json/de.json in run #316 all got flagged even though only one pair actually collided on a key, and a removed method with no textual file conflict (#297 vs #286) went unflagged entirely. - removedSymbols per task entry: a line-pattern scan of `-` diff lines for C# method/property/field/type/event declarations (skips .axaml/.json). - symbolWarnings at the set level: cross-references those names against every other task's `+` lines, catching same-symbol collisions across files that overlaps/subsets (both purely file-based) can't see. - overlaps narrows to key-path level for .json files (base commit vs. task branch head, both sides parsed and flattened) so two tasks adding different locale keys to the same file no longer collide; unparsable JSON on either side falls back to the old file-level check. - GitService.ShowFileAsync: `git show ref:path`, used to read a task branch's json content at its base/head commit. Out of scope per the task: no Roslyn/parser, no AXAML analysis, and the merge-helper prompt in PromptFiles.cs doesn't mention the new fields yet. Build: dotnet build src/ClaudeDo.App (pulls in Ui+Data) + src/ClaudeDo.Worker, both clean. Tests: dotnet test tests/ClaudeDo.Worker.Tests --filter "FullyQualifiedName~MergeMcpTools|FullyQualifiedName~ExternalMcpToolSchema" -- 51 passed, 0 failed. Co-Authored-By: Claude Sonnet 5 --- src/ClaudeDo.Data/Git/GitService.cs | 10 + src/ClaudeDo.Worker/External/MergeMcpTools.cs | 326 ++++++++++++++++-- .../External/MergeMcpToolsTests.cs | 141 +++++++- 3 files changed, 442 insertions(+), 35 deletions(-) diff --git a/src/ClaudeDo.Data/Git/GitService.cs b/src/ClaudeDo.Data/Git/GitService.cs index 3e6f1f07..789b26ca 100644 --- a/src/ClaudeDo.Data/Git/GitService.cs +++ b/src/ClaudeDo.Data/Git/GitService.cs @@ -558,6 +558,16 @@ public sealed class GitService return exitCode == 0 ? stdout : null; } + /// + /// Content of at via `git show revision:path`. + /// Null when the path doesn't exist at that revision (new, deleted, or renamed file). + /// + public async Task ShowFileAsync(string repoDir, string revision, string path, CancellationToken ct = default) + { + var (exitCode, stdout, _) = await RunGitAsync(repoDir, ["show", $"{revision}:{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/External/MergeMcpTools.cs b/src/ClaudeDo.Worker/External/MergeMcpTools.cs index 24952f56..95d977bd 100644 --- a/src/ClaudeDo.Worker/External/MergeMcpTools.cs +++ b/src/ClaudeDo.Worker/External/MergeMcpTools.cs @@ -1,6 +1,8 @@ using System.ComponentModel; using System.Diagnostics; using System.Text; +using System.Text.Json; +using System.Text.RegularExpressions; using ClaudeDo.Data; using ClaudeDo.Data.Git; using ClaudeDo.Data.Models; @@ -61,7 +63,14 @@ public sealed record MergePreviewSetEntryDto( // Mirrors TaskEntity.ExpectsNoDiff -- lets the merge helper tell a deliberately empty diff // (verification/audit/research task) apart from a task that silently failed to commit, both // of which otherwise show IsEmpty=true with no other distinguishing signal. - bool ExpectsNoDiff = false); + bool ExpectsNoDiff = false, + // Top-level C# declarations (methods/properties/fields/types/events) this task's diff removes + // -- a line-pattern scan over `-` lines, not a parser: false positives are possible, a missed + // removal is not the intent. Skips .axaml/.json. Null for an entry with Error set. A name + // another task in the same set still touches on a `+` line surfaces in SymbolWarnings below. + IReadOnlyList? RemovedSymbols = null); + +public sealed record RemovedSymbolDto(string Name, string File); public sealed record FileOverlapDto(string File, IReadOnlyList TaskIds, IReadOnlyList Numbers); @@ -71,9 +80,16 @@ public sealed record FileOverlapDto(string File, IReadOnlyList TaskIds, // signal that TaskId's work may already be entirely covered by SupersetTaskId. public sealed record SubsetRelationDto(string TaskId, int Number, string SupersetTaskId, int SupersetNumber); +// RemovedByTaskId's diff removes Symbol (in RemovedInFile); ReferencedByTaskId's diff still touches +// that name on a `+` line elsewhere -- no textual file conflict, so Overlaps alone misses it (e.g. +// one task deletes a method, a sibling task's edit in a different file still calls it). +public sealed record SymbolReferenceWarningDto( + string Symbol, string RemovedInFile, string RemovedByTaskId, int RemovedByNumber, + string ReferencedByTaskId, int ReferencedByNumber, string ReferencedInFile); + public sealed record MergePreviewSetResultDto( IReadOnlyList Tasks, IReadOnlyList Overlaps, - IReadOnlyList Subsets); + IReadOnlyList Subsets, IReadOnlyList SymbolWarnings); public sealed record RevertMergeResultDto( bool Reverted, string? RevertCommit, IReadOnlyList Conflicts, string? Message); @@ -469,7 +485,7 @@ public sealed class MergeMcpTools IProgress? progress = null) { taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); - var (preview, behind, _, isEmpty, staleFiles, _, _) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken, progress); + var (preview, behind, _, isEmpty, staleFiles, _, _, _, _, _) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken, progress); return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, isEmpty, preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail, staleFiles); } @@ -477,16 +493,21 @@ public sealed class MergeMcpTools [McpServerTool, Description( "Plan a batch merge: preview_merge for several tasks against the same targetBranch, plus a file-overlap " + "check between them. Per entry you get preview_merge's fields, or error instead when that task could not " + - "be previewed (it is then left out of the overlap/subset computation). overlaps names, for each file " + - "touched by MORE THAN ONE of the given tasks, which tasks touch it — a single taskId always yields no " + - "overlaps. subsets flags a stronger relation: TaskId's changed files are a PROPER subset of " + + "be previewed (it is then left out of the overlap/subset/symbol computation). overlaps names, for each " + + "file touched by MORE THAN ONE of the given tasks, which tasks touch it — a single taskId always yields " + + "no overlaps; for a .json file this is narrowed to nested-key-path level (parsing both the base commit " + + "and the task branch's head), so two tasks adding different keys to the same locale file are not flagged " + + "and only a genuinely shared key is — unparsable JSON on either side falls back to the plain file-level " + + "check. subsets flags a stronger relation: TaskId's changed files are a PROPER subset of " + "SupersetTaskId's — the strongest hint you get post-hoc that TaskId may be redundant with SupersetTaskId, " + - "worth checking before merging both. IMPORTANT: neither overlap nor subset is a safety guarantee — two " + - "tasks touching entirely different files (one deleting a symbol, another still referencing it) can still " + - "collide unflagged, and as with preview_merge a clean result does not mean the merge builds. " + - "runVerify=false (default) never builds — set it true to also run each task's list's verify command in a " + - "scratch worktree per entry (same fields as preview_merge); this can take a long time across many tasks, " + - "since builds run one at a time." + McpToolDocs.ProgressHint)] + "worth checking before merging both. removedSymbols (per task entry) lists C# methods/properties/fields/" + + "types/events that task's diff deletes (a line-pattern scan, not a parser — false positives possible, " + + "skips .axaml/.json); symbolWarnings then cross-references those names against every OTHER task's added " + + "lines in the same set, catching a same-symbol collision even when the two tasks touch entirely different " + + "files. IMPORTANT: none of overlaps/subsets/symbolWarnings is a safety guarantee, and as with preview_merge " + + "a clean result does not mean the merge builds. runVerify=false (default) never builds — set it true to " + + "also run each task's list's verify command in a scratch worktree per entry (same fields as preview_merge); " + + "this can take a long time across many tasks, since builds run one at a time." + McpToolDocs.ProgressHint)] public async Task PreviewMergeSet( IReadOnlyList taskIds, [Description("Branch to preview every task against; defaults to the repo's current branch.")] @@ -505,18 +526,37 @@ public sealed class MergeMcpTools var entries = new List(); var filesByTask = new Dictionary>(); var numbersByTask = new Dictionary(); + var repoPathByTask = new Dictionary(); + var baseRefByTask = new Dictionary(); + var headRefByTask = new Dictionary(); + var diffLinesByTask = new Dictionary>(); + var removedSymbolsByTask = new Dictionary>(); for (var i = 0; i < taskIds.Count; i++) { var taskId = taskIds[i]; try { - var (preview, behind, changedFiles, isEmpty, staleFiles, number, expectsNoDiff) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken, progress); - entries.Add(new MergePreviewSetEntryDto( - taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null, isEmpty, - preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail, staleFiles, number, expectsNoDiff)); + var (preview, behind, changedFiles, isEmpty, staleFiles, number, expectsNoDiff, repoPath, baseRef, headRef) = + await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken, progress); filesByTask[taskId] = changedFiles; numbersByTask[taskId] = number; + repoPathByTask[taskId] = repoPath; + baseRefByTask[taskId] = baseRef; + headRefByTask[taskId] = headRef; + + var diff = headRef is null + ? await _git.GetBranchDiffAsync(repoPath, baseRef, ct: cancellationToken) + : await _git.GetCommitRangeDiffAsync(repoPath, baseRef, headRef, ct: cancellationToken); + var diffLines = ParseDiffLines(diff); + diffLinesByTask[taskId] = diffLines; + var removedSymbols = ExtractRemovedSymbols(diffLines); + removedSymbolsByTask[taskId] = removedSymbols; + + entries.Add(new MergePreviewSetEntryDto( + taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null, isEmpty, + preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail, staleFiles, number, + expectsNoDiff, removedSymbols)); } catch (InvalidOperationException ex) { @@ -529,23 +569,243 @@ public sealed class MergeMcpTools ProgressReporter.ReportItem(progress, "Previewing merges", i + 1, taskIds.Count); } - var overlaps = filesByTask - .SelectMany(kv => kv.Value.Select(f => (File: f, TaskId: kv.Key))) - .GroupBy(x => x.File, StringComparer.OrdinalIgnoreCase) - .Where(g => g.Select(x => x.TaskId).Distinct().Count() > 1) - .Select(g => - { - var ids = g.Select(x => x.TaskId).Distinct().ToList(); - return new FileOverlapDto(g.Key, ids, ids.Select(id => numbersByTask[id]).ToList()); - }) - .OrderBy(o => o.File, StringComparer.OrdinalIgnoreCase) - .ToList(); - + var overlaps = await ComputeOverlapsAsync(filesByTask, numbersByTask, repoPathByTask, baseRefByTask, headRefByTask, cancellationToken); var subsets = FindSubsetRelations(filesByTask, numbersByTask); + var symbolWarnings = FindSymbolWarnings(removedSymbolsByTask, numbersByTask, diffLinesByTask); - return new MergePreviewSetResultDto(entries, overlaps, subsets); + return new MergePreviewSetResultDto(entries, overlaps, subsets, symbolWarnings); } + // Cross-references each task's removed symbols (Teil 1) against every OTHER task's added + // lines in the same set -- a same-named symbol collision that overlaps/subsets (both purely + // file-based) can never catch, since the two tasks may not share a single file. + private static List FindSymbolWarnings( + Dictionary> removedSymbolsByTask, + Dictionary numbersByTask, + Dictionary> diffLinesByTask) + { + var warnings = new List(); + foreach (var (taskId, removed) in removedSymbolsByTask) + { + foreach (var symbol in removed) + { + foreach (var (otherTaskId, diffLines) in diffLinesByTask) + { + if (otherTaskId == taskId) continue; + var hit = diffLines.FirstOrDefault(l => l.Sign == '+' && ContainsWord(l.Text, symbol.Name)); + if (hit.File is not null) + warnings.Add(new SymbolReferenceWarningDto( + symbol.Name, symbol.File, taskId, numbersByTask[taskId], + otherTaskId, numbersByTask[otherTaskId], hit.File)); + } + } + } + + return warnings + .OrderBy(w => w.Symbol, StringComparer.Ordinal) + .ThenBy(w => w.RemovedByTaskId, StringComparer.Ordinal) + .ThenBy(w => w.ReferencedByTaskId, StringComparer.Ordinal) + .ToList(); + } + + // File-level overlap for everything except .json; for .json, narrows to nested-key-path + // collisions between the tasks that touch it (Teil 2) -- unless either side fails to parse, + // in which case it falls back to the plain file-level check for that file. + private async Task> ComputeOverlapsAsync( + Dictionary> filesByTask, Dictionary numbersByTask, + Dictionary repoPathByTask, Dictionary baseRefByTask, + Dictionary headRefByTask, CancellationToken ct) + { + var overlaps = new List(); + + var byFile = filesByTask + .SelectMany(kv => kv.Value.Select(f => (File: f, TaskId: kv.Key))) + .GroupBy(x => x.File, StringComparer.OrdinalIgnoreCase); + + foreach (var group in byFile) + { + var taskIds = group.Select(x => x.TaskId).Distinct().ToList(); + if (taskIds.Count <= 1) continue; + + if (!group.Key.EndsWith(".json", StringComparison.OrdinalIgnoreCase)) + { + overlaps.Add(MakeOverlap(group.Key, taskIds, numbersByTask)); + continue; + } + + var colliding = await FindJsonKeyCollisionTasksAsync( + group.Key, taskIds, repoPathByTask, baseRefByTask, headRefByTask, ct); + if (colliding is null) + overlaps.Add(MakeOverlap(group.Key, taskIds, numbersByTask)); // unparsable -- fall back + else if (colliding.Count > 1) + overlaps.Add(MakeOverlap(group.Key, colliding, numbersByTask)); + } + + return overlaps.OrderBy(o => o.File, StringComparer.OrdinalIgnoreCase).ToList(); + } + + private static FileOverlapDto MakeOverlap(string file, IReadOnlyList taskIds, Dictionary numbersByTask) + => new(file, taskIds, taskIds.Select(id => numbersByTask[id]).ToList()); + + // Null = give up and fall back to file-level (JSON unparsable on some side for some task). + // Otherwise: the subset of taskIds whose changed/added/removed key paths in `file` intersect + // with at least one other task's -- two tasks touching disjoint keys in the same file never appear. + private async Task?> FindJsonKeyCollisionTasksAsync( + string file, List taskIds, + Dictionary repoPathByTask, Dictionary baseRefByTask, + Dictionary headRefByTask, CancellationToken ct) + { + var keysByTask = new Dictionary>(); + foreach (var taskId in taskIds) + { + var baseContent = await ReadFileContentAsync(repoPathByTask[taskId], baseRefByTask[taskId], file, ct); + var headContent = await ReadFileContentAsync(repoPathByTask[taskId], headRefByTask[taskId], file, ct); + if (!TryDiffJsonKeyPaths(baseContent, headContent, out var keys)) + return null; + keysByTask[taskId] = keys; + } + + var colliding = new HashSet(); + foreach (var a in taskIds) + foreach (var b in taskIds) + { + if (a == b) continue; + if (keysByTask[a].Overlaps(keysByTask[b])) + { + colliding.Add(a); + colliding.Add(b); + } + } + + return taskIds.Where(colliding.Contains).ToList(); + } + + // gitRef null = read the live working tree off disk (mirrors GetBranchDiffAsync's own base-vs- + // working-tree comparison); non-null = `git show ref:path` (handler task's fixed commit range). + private async Task ReadFileContentAsync(string repoPath, string? gitRef, string relativePath, CancellationToken ct) + { + if (gitRef is null) + { + var full = Path.Combine(repoPath, relativePath); + return File.Exists(full) ? await File.ReadAllTextAsync(full, ct) : null; + } + return await _git.ShowFileAsync(repoPath, gitRef, relativePath, ct); + } + + private static bool TryDiffJsonKeyPaths(string? baseContent, string? headContent, out HashSet changedPaths) + { + changedPaths = new HashSet(StringComparer.Ordinal); + if (!TryFlattenJson(baseContent, out var baseMap) || !TryFlattenJson(headContent, out var headMap)) + return false; + + foreach (var (path, value) in headMap) + if (!baseMap.TryGetValue(path, out var baseValue) || baseValue != value) + changedPaths.Add(path); + foreach (var path in baseMap.Keys) + if (!headMap.ContainsKey(path)) + changedPaths.Add(path); + + return true; + } + + // Missing content (new/deleted file, either side) is an empty object, not a parse failure -- + // only actually-malformed JSON triggers the file-level fallback. + private static bool TryFlattenJson(string? content, out Dictionary flattened) + { + flattened = new Dictionary(StringComparer.Ordinal); + if (string.IsNullOrWhiteSpace(content)) + return true; + + try + { + using var doc = JsonDocument.Parse(content); + FlattenJson(doc.RootElement, "", flattened); + return true; + } + catch (JsonException) + { + return false; + } + } + + private static void FlattenJson(JsonElement element, string prefix, Dictionary into) + { + if (element.ValueKind == JsonValueKind.Object) + { + foreach (var prop in element.EnumerateObject()) + FlattenJson(prop.Value, prefix.Length == 0 ? prop.Name : $"{prefix}.{prop.Name}", into); + } + else + { + into[prefix] = element.GetRawText(); + } + } + + // One line of a unified diff, tagged with the file it belongs to ('-'/'+' only; context lines + // are dropped). File is set from the `diff --git a/ b/` header, which (unlike + // `+++ b/...`) is present and names the real path even for a deleted file (`+++ /dev/null`). + private readonly record struct DiffLine(string? File, char Sign, string Text); + + private static List ParseDiffLines(string diff) + { + var result = new List(); + string? currentFile = null; + foreach (var raw in diff.Split('\n')) + { + var line = raw.TrimEnd('\r'); + if (line.StartsWith("diff --git a/", StringComparison.Ordinal)) + { + var rest = line["diff --git a/".Length..]; + var sep = rest.IndexOf(" b/", StringComparison.Ordinal); + currentFile = sep >= 0 ? rest[..sep] : rest; + continue; + } + if (currentFile is null) continue; + if (line.StartsWith("---", StringComparison.Ordinal) || line.StartsWith("+++", StringComparison.Ordinal) || + line.StartsWith("@@", StringComparison.Ordinal) || line.StartsWith("index ", StringComparison.Ordinal)) + continue; + + if (line.Length > 0 && line[0] == '-') + result.Add(new DiffLine(currentFile, '-', line[1..])); + else if (line.Length > 0 && line[0] == '+') + result.Add(new DiffLine(currentFile, '+', line[1..])); + } + return result; + } + + // Deliberately a line-pattern scan, not a Roslyn parse: requires an explicit access modifier + // (this codebase's style never omits one on a real member/type), which is also what keeps a + // removed local variable or statement inside a method body from matching. + private static readonly Regex RemovedDeclarationRegex = new( + @"^\s*(?:\[[^\]]*\]\s*)*(?:public|private|protected|internal)(?:\s+(?:static|readonly|sealed|abstract|virtual|override|async|partial|const|new|extern|unsafe|required))*\s+" + + @"(?:(?:class|struct|interface|enum|record)\s+(?[A-Za-z_]\w*)" + + @"|event\s+[\w<>\[\],.?]+\s+(?[A-Za-z_]\w*)" + + @"|[\w<>\[\],.?]+\??\s+(?[A-Za-z_]\w*)\s*(?:\(|\{|=>|;|=))", + RegexOptions.Compiled); + + private static List ExtractRemovedSymbols(List diffLines) + { + var seen = new HashSet<(string Name, string File)>(); + var result = new List(); + foreach (var line in diffLines) + { + if (line.Sign != '-' || line.File is null) continue; + if (line.File.EndsWith(".axaml", StringComparison.OrdinalIgnoreCase) || + line.File.EndsWith(".json", StringComparison.OrdinalIgnoreCase)) + continue; + + var match = RemovedDeclarationRegex.Match(line.Text); + if (!match.Success) continue; + + if (seen.Add((match.Groups["name"].Value, line.File))) + result.Add(new RemovedSymbolDto(match.Groups["name"].Value, line.File)); + } + return result; + } + + private static bool ContainsWord(string text, string word) + => Regex.IsMatch(text, $@"\b{Regex.Escape(word)}\b"); + // A task's changed-file set is flagged only against another task's STRICTLY LARGER set that // contains every one of its files -- an empty set (isEmpty task) never qualifies, since that // case is already surfaced via IsEmpty and would otherwise match every other task trivially. @@ -581,7 +841,7 @@ public sealed class MergeMcpTools // (its commits already sit on the list's working dir) — falls back to the fixed // HandlerBaseCommit..HandlerHeadCommit range, reporting a synthetic "clean" preview of that // range's own diff-stat instead of throwing "has no worktree". - private async Task<(MergePreviewResult Preview, int Behind, IReadOnlyList ChangedFiles, bool IsEmpty, IReadOnlyList StaleFiles, int Number, bool ExpectsNoDiff)> PreviewMergeCoreAsync( + private async Task<(MergePreviewResult Preview, int Behind, IReadOnlyList ChangedFiles, bool IsEmpty, IReadOnlyList StaleFiles, int Number, bool ExpectsNoDiff, string RepoPath, string BaseRef, string? HeadRef)> PreviewMergeCoreAsync( string taskId, string? targetBranch, bool runVerify, CancellationToken ct, IProgress? progress = null) { @@ -623,7 +883,8 @@ public sealed class MergeMcpTools .Intersect(targetChangedSinceFork, StringComparer.OrdinalIgnoreCase) .ToList(); - return (preview, behind, changedFiles, changedFiles.Count == 0, staleFiles, task.Number, task.ExpectsNoDiff); + return (preview, behind, changedFiles, changedFiles.Count == 0, staleFiles, task.Number, task.ExpectsNoDiff, + wt.Path, wt.BaseCommit, null); } if (task.HandlerBaseCommit is { Length: > 0 } handlerBase && task.HandlerHeadCommit is { Length: > 0 } handlerHead) @@ -640,7 +901,8 @@ public sealed class MergeMcpTools // No fork point to diff against: a handler task commits straight onto the list's // working dir instead of a branch, so there is nothing else that could have "changed // in the target since the fork". - return (preview, 0, changedFiles, isEmpty, Array.Empty(), task.Number, task.ExpectsNoDiff); + return (preview, 0, changedFiles, isEmpty, Array.Empty(), task.Number, task.ExpectsNoDiff, + list.WorkingDir, handlerBase, handlerHead); } throw new InvalidOperationException($"Task {taskId} has no worktree."); diff --git a/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs index 58e6676b..6838b065 100644 --- a/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs @@ -163,12 +163,13 @@ public sealed class MergeMcpToolsTests : IDisposable } private async Task CreateActiveWorktreeAsync( - GitRepoFixture repo, string taskId, string fileName, string content) + GitRepoFixture repo, string taskId, string fileName, string content, string? baseCommit = null) { + baseCommit ??= repo.BaseCommit; var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}"); _worktreeCleanups.Add((repo.RepoDir, wtPath)); var branch = $"claudedo/{taskId[..8]}"; - GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", branch, wtPath, repo.BaseCommit); + GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", branch, wtPath, baseCommit); File.WriteAllText(Path.Combine(wtPath, fileName), content); GitRepoFixture.RunGit(wtPath, "add", fileName); GitRepoFixture.RunGit(wtPath, "commit", "-m", $"edit {fileName}"); @@ -177,7 +178,7 @@ public sealed class MergeMcpToolsTests : IDisposable ctx.Worktrees.Add(new WorktreeEntity { TaskId = taskId, Path = wtPath, BranchName = branch, - BaseCommit = repo.BaseCommit, + BaseCommit = baseCommit, HeadCommit = GitRepoFixture.RunGit(wtPath, "rev-parse", "HEAD").Trim(), State = WorktreeState.Active, CreatedAt = DateTime.UtcNow, }); @@ -1317,4 +1318,138 @@ public sealed class MergeMcpToolsTests : IDisposable Assert.Equal(0, result.Tasks.Single().VerifyExitCode); Assert.Equal("dotnet build", fakeVerify.CapturedCommand); } + + [Fact] + public async Task PreviewMergeSet_RemovedSymbolReferencedByAnotherTask_ReportsSymbolWarning() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + File.WriteAllText(Path.Combine(repo.RepoDir, "Foo.cs"), + "public class Foo\n{\n private void RestartWorkerStatus()\n {\n }\n}\n"); + File.WriteAllText(Path.Combine(repo.RepoDir, "Bar.cs"), "public class Bar\n{\n}\n"); + GitRepoFixture.RunGit(repo.RepoDir, "add", "-A"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "add Foo/Bar"); + var baseCommit = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim(); + + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + var removerTask = await SeedTaskAsync(listId, "removes", TaskStatus.WaitingForReview); + var referencerTask = await SeedTaskAsync(listId, "references", TaskStatus.WaitingForReview); + + await CreateActiveWorktreeAsync(repo, removerTask.Id, "Foo.cs", "public class Foo\n{\n}\n", baseCommit); + await CreateActiveWorktreeAsync(repo, referencerTask.Id, "Bar.cs", + "public class Bar\n{\n private void Call()\n {\n RestartWorkerStatus();\n }\n}\n", baseCommit); + + var sut = BuildSut(CreateQueue()); + var result = await sut.PreviewMergeSet( + [removerTask.Id, referencerTask.Id], "main", cancellationToken: CancellationToken.None); + + var removerEntry = result.Tasks.Single(t => t.TaskId == removerTask.Id); + Assert.Contains(removerEntry.RemovedSymbols!, s => s.Name == "RestartWorkerStatus" && s.File == "Foo.cs"); + + var warning = Assert.Single(result.SymbolWarnings); + Assert.Equal("RestartWorkerStatus", warning.Symbol); + Assert.Equal("Foo.cs", warning.RemovedInFile); + Assert.Equal(removerTask.Id, warning.RemovedByTaskId); + Assert.Equal(referencerTask.Id, warning.ReferencedByTaskId); + Assert.Equal("Bar.cs", warning.ReferencedInFile); + } + + [Fact] + public async Task PreviewMergeSet_TwoTasksAddDifferentJsonKeys_ReportsNoOverlap() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + File.WriteAllText(Path.Combine(repo.RepoDir, "en.json"), "{}\n"); + GitRepoFixture.RunGit(repo.RepoDir, "add", "-A"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "add en.json"); + var baseCommit = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim(); + + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + var taskA = await SeedTaskAsync(listId, "a", TaskStatus.WaitingForReview); + var taskB = await SeedTaskAsync(listId, "b", TaskStatus.WaitingForReview); + + await CreateActiveWorktreeAsync(repo, taskA.Id, "en.json", "{\"a\": {\"one\": \"1\"}}\n", baseCommit); + await CreateActiveWorktreeAsync(repo, taskB.Id, "en.json", "{\"a\": {\"two\": \"2\"}}\n", baseCommit); + + var sut = BuildSut(CreateQueue()); + var result = await sut.PreviewMergeSet([taskA.Id, taskB.Id], "main", cancellationToken: CancellationToken.None); + + Assert.Empty(result.Overlaps); + } + + [Fact] + public async Task PreviewMergeSet_TwoTasksChangeSameJsonKey_ReportsOverlap() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + File.WriteAllText(Path.Combine(repo.RepoDir, "en.json"), "{}\n"); + GitRepoFixture.RunGit(repo.RepoDir, "add", "-A"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "add en.json"); + var baseCommit = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim(); + + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + var taskA = await SeedTaskAsync(listId, "a", TaskStatus.WaitingForReview); + var taskB = await SeedTaskAsync(listId, "b", TaskStatus.WaitingForReview); + + await CreateActiveWorktreeAsync(repo, taskA.Id, "en.json", "{\"a\": {\"one\": \"1\"}}\n", baseCommit); + await CreateActiveWorktreeAsync(repo, taskB.Id, "en.json", "{\"a\": {\"one\": \"2\"}}\n", baseCommit); + + var sut = BuildSut(CreateQueue()); + var result = await sut.PreviewMergeSet([taskA.Id, taskB.Id], "main", cancellationToken: CancellationToken.None); + + var overlap = Assert.Single(result.Overlaps); + Assert.Equal("en.json", overlap.File); + Assert.Equal( + new[] { taskA.Id, taskB.Id }.OrderBy(x => x), + overlap.TaskIds.OrderBy(x => x)); + } + + [Fact] + public async Task PreviewMergeSet_UnparsableJsonOnOneSide_FallsBackToFileOverlap() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + File.WriteAllText(Path.Combine(repo.RepoDir, "en.json"), "{}\n"); + GitRepoFixture.RunGit(repo.RepoDir, "add", "-A"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "add en.json"); + var baseCommit = GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim(); + + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + var taskA = await SeedTaskAsync(listId, "a", TaskStatus.WaitingForReview); + var taskB = await SeedTaskAsync(listId, "b", TaskStatus.WaitingForReview); + + // taskA leaves the file syntactically broken -- both sides must still be flagged since + // there's no way to tell whether the keys actually collide. + await CreateActiveWorktreeAsync(repo, taskA.Id, "en.json", "{ not valid json", baseCommit); + await CreateActiveWorktreeAsync(repo, taskB.Id, "en.json", "{\"a\": {\"one\": \"2\"}}\n", baseCommit); + + var sut = BuildSut(CreateQueue()); + var result = await sut.PreviewMergeSet([taskA.Id, taskB.Id], "main", cancellationToken: CancellationToken.None); + + var overlap = Assert.Single(result.Overlaps); + Assert.Equal("en.json", overlap.File); + Assert.Equal( + new[] { taskA.Id, taskB.Id }.OrderBy(x => x), + overlap.TaskIds.OrderBy(x => x)); + } }