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 <noreply@anthropic.com>
This commit is contained in:
Mika Kuns
2026-09-16 15:43:38 +02:00
committed by mika kuns
co-authored by Claude Sonnet 5
parent 1325ab9438
commit 684b0115e5
3 changed files with 442 additions and 35 deletions
+294 -32
View File
@@ -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<RemovedSymbolDto>? RemovedSymbols = null);
public sealed record RemovedSymbolDto(string Name, string File);
public sealed record FileOverlapDto(string File, IReadOnlyList<string> TaskIds, IReadOnlyList<int> Numbers);
@@ -71,9 +80,16 @@ public sealed record FileOverlapDto(string File, IReadOnlyList<string> 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<MergePreviewSetEntryDto> Tasks, IReadOnlyList<FileOverlapDto> Overlaps,
IReadOnlyList<SubsetRelationDto> Subsets);
IReadOnlyList<SubsetRelationDto> Subsets, IReadOnlyList<SymbolReferenceWarningDto> SymbolWarnings);
public sealed record RevertMergeResultDto(
bool Reverted, string? RevertCommit, IReadOnlyList<string> Conflicts, string? Message);
@@ -469,7 +485,7 @@ public sealed class MergeMcpTools
IProgress<ProgressNotificationValue>? 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<MergePreviewSetResultDto> PreviewMergeSet(
IReadOnlyList<string> 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<MergePreviewSetEntryDto>();
var filesByTask = new Dictionary<string, IReadOnlyList<string>>();
var numbersByTask = new Dictionary<string, int>();
var repoPathByTask = new Dictionary<string, string>();
var baseRefByTask = new Dictionary<string, string>();
var headRefByTask = new Dictionary<string, string?>();
var diffLinesByTask = new Dictionary<string, List<DiffLine>>();
var removedSymbolsByTask = new Dictionary<string, IReadOnlyList<RemovedSymbolDto>>();
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<SymbolReferenceWarningDto> FindSymbolWarnings(
Dictionary<string, IReadOnlyList<RemovedSymbolDto>> removedSymbolsByTask,
Dictionary<string, int> numbersByTask,
Dictionary<string, List<DiffLine>> diffLinesByTask)
{
var warnings = new List<SymbolReferenceWarningDto>();
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<List<FileOverlapDto>> ComputeOverlapsAsync(
Dictionary<string, IReadOnlyList<string>> filesByTask, Dictionary<string, int> numbersByTask,
Dictionary<string, string> repoPathByTask, Dictionary<string, string> baseRefByTask,
Dictionary<string, string?> headRefByTask, CancellationToken ct)
{
var overlaps = new List<FileOverlapDto>();
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<string> taskIds, Dictionary<string, int> 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<List<string>?> FindJsonKeyCollisionTasksAsync(
string file, List<string> taskIds,
Dictionary<string, string> repoPathByTask, Dictionary<string, string> baseRefByTask,
Dictionary<string, string?> headRefByTask, CancellationToken ct)
{
var keysByTask = new Dictionary<string, HashSet<string>>();
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<string>();
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<string?> 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<string> changedPaths)
{
changedPaths = new HashSet<string>(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<string, string> flattened)
{
flattened = new Dictionary<string, string>(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<string, string> 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/<path> b/<path>` 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<DiffLine> ParseDiffLines(string diff)
{
var result = new List<DiffLine>();
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+(?<name>[A-Za-z_]\w*)" +
@"|event\s+[\w<>\[\],.?]+\s+(?<name>[A-Za-z_]\w*)" +
@"|[\w<>\[\],.?]+\??\s+(?<name>[A-Za-z_]\w*)\s*(?:\(|\{|=>|;|=))",
RegexOptions.Compiled);
private static List<RemovedSymbolDto> ExtractRemovedSymbols(List<DiffLine> diffLines)
{
var seen = new HashSet<(string Name, string File)>();
var result = new List<RemovedSymbolDto>();
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<string> ChangedFiles, bool IsEmpty, IReadOnlyList<string> StaleFiles, int Number, bool ExpectsNoDiff)> PreviewMergeCoreAsync(
private async Task<(MergePreviewResult Preview, int Behind, IReadOnlyList<string> ChangedFiles, bool IsEmpty, IReadOnlyList<string> StaleFiles, int Number, bool ExpectsNoDiff, string RepoPath, string BaseRef, string? HeadRef)> PreviewMergeCoreAsync(
string taskId, string? targetBranch, bool runVerify, CancellationToken ct,
IProgress<ProgressNotificationValue>? 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<string>(), task.Number, task.ExpectsNoDiff);
return (preview, 0, changedFiles, isEmpty, Array.Empty<string>(), task.Number, task.ExpectsNoDiff,
list.WorkingDir, handlerBase, handlerHead);
}
throw new InvalidOperationException($"Task {taskId} has no worktree.");