fix(merge): refuse a merge that would overwrite an untracked file in the target

Before merging (or staging a conflict resolution's git add -A), compare the
branch's newly-added paths against what's currently untracked in the target
working directory. A collision aborts with a new untracked_collision status
naming the path and size, surfaced through merge_task/review_task,
preview_merge/preview_merge_set (which merge-tree alone can't see), and the
UI merge paths via FlashFooterError/ShowErrorAsync instead of a silent
catch{}. git's own preflight already refuses this while the path stays
untracked at merge time; this closes the gap once a path becomes trackable
in between (e.g. an unrelated conflict resolution's git add -A) or across
the continue_merge TOCTOU window.
This commit is contained in:
mika kuns
2026-08-10 12:15:41 +02:00
parent 6a2a19cc9e
commit afe1b68f46
9 changed files with 269 additions and 13 deletions
+10 -7
View File
@@ -929,13 +929,16 @@ public sealed class ExternalMcpService
[McpServerTool, Description(
"Check whether a task would merge cleanly before touching anything — `git merge-tree --write-tree`, so the " +
"working tree, index and HEAD are untouched. status is 'clean' or 'conflict' (conflictFiles lists where git " +
"would stop); behind counts commits on targetBranch not yet on this branch, which flags a stale branch even " +
"when the preview is clean. IMPORTANT: a clean preview says nothing about whether the result compiles or " +
"passes tests — git can merge two changes cleanly (one file deleting a symbol another still references) and " +
"still break the build. isEmpty=true means the task's review range contributed nothing; check that flag " +
"rather than reading a small changedFileCount as empty. Throws if the task has neither an active worktree " +
"nor a handler commit range, or the list's working directory is missing from disk.")]
"working tree, index and HEAD are untouched. status is 'clean', 'conflict' (conflictFiles lists where git " +
"would stop), or 'untracked_collision' (conflictFiles lists a path the branch adds that already exists, " +
"untracked, in the target working directory — merge-tree can't see the working tree at all, so this is the " +
"only way to catch it before a real merge either refuses or, if that path became tracked in the meantime, " +
"silently overwrites it); behind counts commits on targetBranch not yet on this branch, which flags a stale " +
"branch even when the preview is clean. IMPORTANT: a clean preview says nothing about whether the result " +
"compiles or passes tests — git can merge two changes cleanly (one file deleting a symbol another still " +
"references) and still break the build. isEmpty=true means the task's review range contributed nothing; " +
"check that flag rather than reading a small changedFileCount as empty. Throws if the task has neither an " +
"active worktree nor a handler commit range, or the list's working directory is missing from disk.")]
public async Task<MergePreviewToolDto> PreviewMerge(
string taskId,
[Description("Branch to preview against; defaults to the repo's current branch.")]
@@ -46,6 +46,7 @@ public sealed class TaskMergeService
public const string StatusBlocked = "blocked";
public const string StatusAborted = "aborted";
public const string StatusVerifyFailed = "verify_failed";
public const string StatusUntrackedCollision = "untracked_collision";
public const string StatusReverted = "reverted";
public const string StatusConflictAborted = "conflict_aborted";
@@ -53,6 +54,7 @@ public sealed class TaskMergeService
public const string PreviewClean = "clean";
public const string PreviewConflict = "conflict";
public const string PreviewUnavailable = "unavailable";
public const string PreviewUntrackedCollision = "untracked_collision";
// The verify command is a trusted, list-owner-configured build/test invocation (not
// per-request user input), so a generous fixed timeout is enough — no need for a
@@ -143,6 +145,82 @@ public sealed class TaskMergeService
return trimmed.Length <= maxChars ? trimmed : trimmed[^maxChars..];
}
/// <summary>
/// Guards against a merge silently overwriting a file that sits untracked in the target
/// working directory: git already refuses that itself while the path is still untracked at
/// merge time, but a path that only became trackable in between (e.g. an unrelated conflict
/// resolution's `git add -A` sweeping it up) loses that protection. Compares the paths
/// <paramref name="branchName"/> newly added since its merge base with <paramref name="targetRef"/>
/// against what's currently untracked in <paramref name="workingDir"/> — reusing
/// <see cref="GitService.GetCommittedFilesAsync"/> (run against the branch's own worktree, where
/// HEAD is the branch tip) and <see cref="GitService.GetStatusPorcelainAsync"/> rather than adding
/// new git plumbing. Returns null when there's nothing to flag.
/// </summary>
private async Task<MergeResult?> CheckUntrackedCollisionsAsync(
string workingDir, string targetRef, string worktreePath, string branchName, CancellationToken ct)
{
var mergeBase = await _git.MergeBaseAsync(workingDir, targetRef, branchName, ct);
if (mergeBase is null) return null;
List<string> addedByBranch;
try
{
addedByBranch = ParseAddedPaths(await _git.GetCommittedFilesAsync(worktreePath, mergeBase, ct));
}
catch { return null; } // worktree missing — nothing to compare, let the merge report its own outcome
if (addedByBranch.Count == 0) return null;
var untracked = ParseUntrackedPaths(await _git.GetStatusPorcelainAsync(workingDir, ct));
if (untracked.Count == 0) return null;
var untrackedSet = new HashSet<string>(untracked, StringComparer.OrdinalIgnoreCase);
var collisions = addedByBranch.Where(untrackedSet.Contains).ToList();
if (collisions.Count == 0) return null;
var details = collisions.Select(p => $"{p} ({DescribeSize(workingDir, p)})");
return new MergeResult(StatusUntrackedCollision, collisions,
"merge would overwrite untracked file(s) that exist in the target working directory: " +
string.Join(", ", details));
}
private static string DescribeSize(string workingDir, string relativePath)
{
try
{
var full = Path.Combine(workingDir, relativePath.Replace('/', Path.DirectorySeparatorChar));
return $"{new FileInfo(full).Length} bytes";
}
catch { return "size unknown"; }
}
// Only "A" (pure add) entries — a path the branch modifies but that already exists on the
// target's own history is a normal merge, not a collision with something untracked.
private static List<string> ParseAddedPaths(string nameStatus)
{
var result = new List<string>();
foreach (var raw in nameStatus.Split('\n', StringSplitOptions.RemoveEmptyEntries))
{
var line = raw.TrimEnd('\r');
if (line.Length < 2 || line[0] != 'A') continue;
var tab = line.IndexOf('\t');
if (tab >= 0) result.Add(line[(tab + 1)..].Trim());
}
return result;
}
private static List<string> ParseUntrackedPaths(string porcelain)
{
var result = new List<string>();
foreach (var raw in porcelain.Split('\n', StringSplitOptions.RemoveEmptyEntries))
{
var line = raw.TrimEnd('\r');
if (line.StartsWith("?? ", StringComparison.Ordinal))
result.Add(line[3..].Trim().Trim('"'));
}
return result;
}
private async Task MarkWorktreeMergedAsync(string taskId, string mergeCommitSha, CancellationToken ct)
{
using (var ctx = _dbFactory.CreateDbContext())
@@ -198,6 +276,9 @@ public sealed class TaskMergeService
catch (Exception ex) { return Blocked($"failed to switch target branch: {ex.Message}"); }
}
var collision = await CheckUntrackedCollisionsAsync(list.WorkingDir, "HEAD", wt.Path, wt.BranchName, ct);
if (collision is not null) return collision;
var (exitCode, stderr) = await _git.MergeNoFfAsync(list.WorkingDir, wt.BranchName, commitMessage, ct);
if (exitCode != 0)
{
@@ -315,6 +396,13 @@ public sealed class TaskMergeService
if (stillConflicted.Count > 0)
return new MergeResult(StatusConflict, stillConflicted, "conflicts not fully resolved");
// Closes the window between the original `git merge` (which already refused any
// untracked collision at that point) and this call: an untracked file matching one
// of the branch's own added paths could have appeared in the meantime, and `add -A`
// below stages the whole working directory, not just the conflicted paths.
var collision = await CheckUntrackedCollisionsAsync(list.WorkingDir, "HEAD", wt.Path, wt.BranchName, ct);
if (collision is not null) return collision;
await _git.AddAllAsync(list.WorkingDir, ct);
var remaining = await _git.ListConflictedFilesAsync(list.WorkingDir, ct);
@@ -513,6 +601,12 @@ public sealed class TaskMergeService
? await _git.GetCurrentBranchAsync(list.WorkingDir, ct)
: targetBranch;
// merge-tree (below) is a pure object-level check — it has no idea the working directory
// has an untracked file that would block (or, worse, silently be lost by) the real merge.
var collision = await CheckUntrackedCollisionsAsync(list.WorkingDir, target, wt.Path, wt.BranchName, ct);
if (collision is not null)
return new MergePreviewResult(PreviewUntrackedCollision, collision.ConflictFiles, 0);
var preview = await _git.PreviewMergeAsync(list.WorkingDir, target, wt.BranchName, ct);
if (!preview.Supported)
return new MergePreviewResult(PreviewUnavailable, Array.Empty<string>(), 0);