feat(worker): build/test the merge preview, not just the post-merge merge
preview_merge could only see file-overlap cleanliness via git merge-tree, never whether the result compiles -- the two costliest findings of the 2026-08-06 batch run were both merge-tree-clean but build-broken. When a list has a verify command configured, a clean preview is now additionally materialized (via a commit-tree + detached scratch worktree, outside the real repo, always cleaned up) and built/tested there, without ever touching the real working tree. preview_merge always attempts a verify run when a command is configured; preview_merge_set only does when its new runVerify parameter is set (default off), so a set preview never starts N builds unasked. The post-merge verify gate is unchanged.
This commit is contained in:
@@ -3,7 +3,10 @@ using System.Text;
|
||||
|
||||
namespace ClaudeDo.Data.Git;
|
||||
|
||||
public sealed record MergePreview(bool Supported, bool Clean, IReadOnlyList<string> ConflictFiles);
|
||||
// TreeOid is only populated when Clean — the tree `merge-tree --write-tree` would produce,
|
||||
// usable to materialize the merge result into a scratch worktree without touching the real
|
||||
// working tree, index, or refs (see GitService.CommitTreeAsync / WorktreeAddDetachedAsync).
|
||||
public sealed record MergePreview(bool Supported, bool Clean, IReadOnlyList<string> ConflictFiles, string? TreeOid = null);
|
||||
|
||||
public sealed class GitService
|
||||
{
|
||||
@@ -357,7 +360,8 @@ public sealed class GitService
|
||||
["merge-tree", "--write-tree", "--name-only", targetBranch, sourceBranch], ct);
|
||||
|
||||
if (exitCode == 0)
|
||||
return new MergePreview(true, true, Array.Empty<string>());
|
||||
// stdout is just the written tree's oid on a clean merge.
|
||||
return new MergePreview(true, true, Array.Empty<string>(), stdout.Trim());
|
||||
|
||||
if (exitCode == 1)
|
||||
{
|
||||
@@ -377,6 +381,62 @@ public sealed class GitService
|
||||
return new MergePreview(false, false, Array.Empty<string>());
|
||||
}
|
||||
|
||||
/// <summary>Resolves <paramref name="revision"/> (a branch, tag, or SHA) to a full commit SHA.</summary>
|
||||
public async Task<string> RevParseAsync(string repoDir, string revision, CancellationToken ct = default)
|
||||
{
|
||||
var (exitCode, stdout, stderr) = await RunGitAsync(repoDir, ["rev-parse", revision], ct);
|
||||
if (exitCode != 0)
|
||||
throw new InvalidOperationException($"git rev-parse '{revision}' failed (exit {exitCode}): {stderr}");
|
||||
return stdout.Trim();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Creates a new commit object wrapping <paramref name="treeOid"/> with <paramref name="parentSha"/>
|
||||
/// as its single parent, writing only a loose object — no ref is created or moved.
|
||||
/// </summary>
|
||||
public async Task<string> CommitTreeAsync(
|
||||
string repoDir, string treeOid, string parentSha, string message, CancellationToken ct = default)
|
||||
{
|
||||
var (exitCode, stdout, stderr) = await RunGitAsync(repoDir,
|
||||
["commit-tree", treeOid, "-p", parentSha, "-m", message], ct);
|
||||
if (exitCode != 0)
|
||||
throw new InvalidOperationException($"git commit-tree failed (exit {exitCode}): {stderr}");
|
||||
return stdout.Trim();
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Materializes <paramref name="commitish"/> into a new, branchless worktree at
|
||||
/// <paramref name="worktreePath"/> (detached HEAD) — used to build/verify a merge-tree
|
||||
/// result without ever creating a branch or touching the real working tree.
|
||||
/// </summary>
|
||||
public async Task WorktreeAddDetachedAsync(
|
||||
string repoDir, string worktreePath, string commitish, CancellationToken ct = default)
|
||||
{
|
||||
await WorktreeAddGate.WaitAsync(ct);
|
||||
try
|
||||
{
|
||||
const int maxAttempts = 3;
|
||||
for (var attempt = 1; ; attempt++)
|
||||
{
|
||||
var (exitCode, _, stderr) = await RunGitAsync(repoDir,
|
||||
["worktree", "add", "--detach", worktreePath, commitish], ct);
|
||||
if (exitCode == 0)
|
||||
return;
|
||||
|
||||
var transient = stderr.Contains("commondir", StringComparison.OrdinalIgnoreCase)
|
||||
|| stderr.Contains("failed to read", StringComparison.OrdinalIgnoreCase);
|
||||
if (!transient || attempt >= maxAttempts)
|
||||
throw new InvalidOperationException($"git worktree add --detach failed (exit {exitCode}): {stderr}");
|
||||
|
||||
await Task.Delay(150 * attempt, ct);
|
||||
}
|
||||
}
|
||||
finally
|
||||
{
|
||||
WorktreeAddGate.Release();
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>Count of files that differ on <paramref name="sourceBranch"/> since its merge base with the target.</summary>
|
||||
public async Task<int> CountChangedFilesAsync(
|
||||
string repoDir, string targetBranch, string sourceBranch, CancellationToken ct = default)
|
||||
|
||||
+29
-14
@@ -92,11 +92,17 @@ public sealed record MergeContinuationResultDto(
|
||||
// worktree-less handler task's HandlerBaseCommit == HandlerHeadCommit) -- distinguishable from
|
||||
// a merge that is merely small, so an empty branch can't be misread as "changedFileCount: 0
|
||||
// means tiny" when it actually means "nothing to review".
|
||||
// VerifyExitCode/VerifyDurationMs/VerifyOutputTail are null unless a verify run was attempted
|
||||
// (see PreviewMerge/PreviewMergeSet descriptions) -- 0 exit means the merge-tree result built/
|
||||
// tested clean; a non-zero or -1 (timeout/failed to start) exit means it doesn't, with the tail
|
||||
// of its output in VerifyOutputTail.
|
||||
public sealed record MergePreviewToolDto(
|
||||
string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, bool IsEmpty = false);
|
||||
string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, bool IsEmpty = false,
|
||||
int? VerifyExitCode = null, long? VerifyDurationMs = null, string? VerifyOutputTail = null);
|
||||
|
||||
public sealed record MergePreviewSetEntryDto(
|
||||
string TaskId, string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, string? Error, bool IsEmpty = false);
|
||||
string TaskId, string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, string? Error, bool IsEmpty = false,
|
||||
int? VerifyExitCode = null, long? VerifyDurationMs = null, string? VerifyOutputTail = null);
|
||||
|
||||
public sealed record FileOverlapDto(string File, IReadOnlyList<string> TaskIds);
|
||||
|
||||
@@ -931,19 +937,22 @@ public sealed class ExternalMcpService
|
||||
"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.")]
|
||||
"when the preview is clean. If the list has a verify command configured, a clean preview is additionally " +
|
||||
"built/tested in a scratch worktree (still without touching the real working tree) — verifyExitCode 0 means " +
|
||||
"it built clean, non-zero or -1 (timeout/failed to start) means it doesn't, with the tail of its output in " +
|
||||
"verifyOutputTail; verifyExitCode stays null when no verify command is configured. 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.")]
|
||||
string? targetBranch = null,
|
||||
CancellationToken cancellationToken = default)
|
||||
{
|
||||
var (preview, behind, _, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, isEmpty);
|
||||
var (preview, behind, _, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify: true, cancellationToken);
|
||||
return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, isEmpty,
|
||||
preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail);
|
||||
}
|
||||
|
||||
[McpServerTool, Description(
|
||||
@@ -953,11 +962,16 @@ public sealed class ExternalMcpService
|
||||
"MORE THAN ONE of the given tasks, which tasks touch it — a single taskId always yields no overlaps. " +
|
||||
"IMPORTANT: overlap is a HINT and its absence is not safety — 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.")]
|
||||
"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.")]
|
||||
public async Task<MergePreviewSetResultDto> PreviewMergeSet(
|
||||
IReadOnlyList<string> taskIds,
|
||||
[Description("Branch to preview every task against; defaults to the repo's current branch.")]
|
||||
string? targetBranch = null,
|
||||
[Description("true: also run the verify command (if configured) for each task, one build at a time. " +
|
||||
"false (default): no builds, however many tasks are given.")]
|
||||
bool runVerify = false,
|
||||
CancellationToken cancellationToken = default)
|
||||
{
|
||||
if (taskIds is null || taskIds.Count == 0)
|
||||
@@ -970,9 +984,10 @@ public sealed class ExternalMcpService
|
||||
{
|
||||
try
|
||||
{
|
||||
var (preview, behind, changedFiles, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
var (preview, behind, changedFiles, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken);
|
||||
entries.Add(new MergePreviewSetEntryDto(
|
||||
taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null, isEmpty));
|
||||
taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null, isEmpty,
|
||||
preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail));
|
||||
filesByTask[taskId] = changedFiles;
|
||||
}
|
||||
catch (InvalidOperationException ex)
|
||||
@@ -1001,7 +1016,7 @@ public sealed class ExternalMcpService
|
||||
// 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)> PreviewMergeCoreAsync(
|
||||
string taskId, string? targetBranch, CancellationToken ct)
|
||||
string taskId, string? targetBranch, bool runVerify, CancellationToken ct)
|
||||
{
|
||||
using var ctx = _dbFactory.CreateDbContext();
|
||||
var task = await new TaskRepository(ctx).GetByIdAsync(taskId, ct)
|
||||
@@ -1018,7 +1033,7 @@ public sealed class ExternalMcpService
|
||||
if (string.IsNullOrWhiteSpace(list.WorkingDir) || !Directory.Exists(list.WorkingDir))
|
||||
throw new InvalidOperationException("The list's working directory no longer exists.");
|
||||
|
||||
var preview = await _merge.PreviewAsync(taskId, targetBranch ?? "", ct);
|
||||
var preview = await _merge.PreviewAsync(taskId, targetBranch ?? "", runVerify, ct);
|
||||
if (preview.Status == TaskMergeService.PreviewUnavailable)
|
||||
throw new InvalidOperationException(
|
||||
"Merge preview unavailable for this task (worktree inactive or repo is not a git repository).");
|
||||
|
||||
@@ -88,7 +88,12 @@ public record ForceRemoveResultDto(bool Removed, string? Reason);
|
||||
public record PlanningMergeConflictStateDto(string PlanningTaskId, string SubtaskId);
|
||||
public record PendingQuestionDto(string TaskId, string QuestionId, string Question);
|
||||
public record MergeResultDto(string Status, IReadOnlyList<string> ConflictFiles, string? ErrorMessage);
|
||||
public record MergePreviewDto(string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount);
|
||||
// Verify* fields are always null on this path today -- the UI's live mergeability indicator never
|
||||
// requests a verify run (that would mean an unrequested build on every preview poll); they exist so
|
||||
// MergePreviewDto matches TaskMergeService.MergePreviewResult should a caller opt in later.
|
||||
public record MergePreviewDto(
|
||||
string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount,
|
||||
int? VerifyExitCode = null, long? VerifyDurationMs = null, string? VerifyOutputTail = null);
|
||||
public record MergeTargetsDto(string DefaultBranch, IReadOnlyList<string> LocalBranches);
|
||||
public record MergeConflictDocumentsDto(string TaskId, IReadOnlyList<ConflictDocumentDto> Files);
|
||||
public record ConflictDocumentDto(string Path, bool IsBinary, IReadOnlyList<MergeSegmentDto> Segments);
|
||||
|
||||
@@ -19,10 +19,17 @@ public sealed record MergeTargets(
|
||||
string DefaultBranch,
|
||||
IReadOnlyList<string> LocalBranches);
|
||||
|
||||
// VerifyExitCode/VerifyDurationMs/VerifyOutputTail are only populated when the caller asked for
|
||||
// a verify run (runVerify=true) AND the list has a verify command configured AND the preview came
|
||||
// back clean — a conflicting or unavailable preview never reaches the verify step. -1 is used for
|
||||
// VerifyExitCode when the command timed out or failed to start (mirrors the post-merge gate).
|
||||
public sealed record MergePreviewResult(
|
||||
string Status,
|
||||
IReadOnlyList<string> ConflictFiles,
|
||||
int ChangedFileCount);
|
||||
int ChangedFileCount,
|
||||
int? VerifyExitCode = null,
|
||||
long? VerifyDurationMs = null,
|
||||
string? VerifyOutputTail = null);
|
||||
|
||||
public sealed record ConflictDocuments(
|
||||
string TaskId,
|
||||
@@ -500,9 +507,21 @@ public sealed class TaskMergeService
|
||||
return new MergeTargets(current, branches);
|
||||
}
|
||||
|
||||
public async Task<MergePreviewResult> PreviewAsync(string taskId, string targetBranch, CancellationToken ct)
|
||||
public Task<MergePreviewResult> PreviewAsync(string taskId, string targetBranch, CancellationToken ct)
|
||||
=> PreviewAsync(taskId, targetBranch, runVerify: false, ct);
|
||||
|
||||
/// <summary>
|
||||
/// Non-destructive merge preview via <see cref="GitService.PreviewMergeAsync"/>. When
|
||||
/// <paramref name="runVerify"/> is true and the list has a verify command configured, a clean
|
||||
/// preview is additionally materialized into a scratch worktree (outside the target repo, always
|
||||
/// cleaned up) and built/tested there — the real working tree is never touched. No verify
|
||||
/// command configured, or runVerify=false, reproduces the pre-existing behavior exactly: no
|
||||
/// delay, no scratch worktree.
|
||||
/// </summary>
|
||||
public async Task<MergePreviewResult> PreviewAsync(
|
||||
string taskId, string targetBranch, bool runVerify, CancellationToken ct)
|
||||
{
|
||||
var (_, list, wt, _) = await LoadMergeContextAsync(taskId, ct);
|
||||
var (_, list, wt, verifyCommand) = await LoadMergeContextAsync(taskId, ct);
|
||||
|
||||
if (wt is null || wt.State != WorktreeState.Active)
|
||||
return new MergePreviewResult(PreviewUnavailable, Array.Empty<string>(), 0);
|
||||
@@ -520,7 +539,60 @@ public sealed class TaskMergeService
|
||||
return new MergePreviewResult(PreviewConflict, preview.ConflictFiles, 0);
|
||||
|
||||
var count = await _git.CountChangedFilesAsync(list.WorkingDir, target, wt.BranchName, ct);
|
||||
return new MergePreviewResult(PreviewClean, Array.Empty<string>(), count);
|
||||
|
||||
if (!runVerify || string.IsNullOrWhiteSpace(verifyCommand) || preview.TreeOid is null)
|
||||
return new MergePreviewResult(PreviewClean, Array.Empty<string>(), count);
|
||||
|
||||
var (exitCode, durationMs, outputTail) = await RunPreviewVerifyAsync(
|
||||
list.WorkingDir, target, preview.TreeOid, verifyCommand, ct);
|
||||
return new MergePreviewResult(PreviewClean, Array.Empty<string>(), count, exitCode, durationMs, outputTail);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Builds the tree a clean merge-tree preview would produce into a scratch, detached-HEAD
|
||||
/// worktree (never a branch, never the real working tree) and runs the verify command there.
|
||||
/// The scratch worktree lives under the OS temp dir and is always removed, even on failure or
|
||||
/// cancellation.
|
||||
/// </summary>
|
||||
private async Task<(int ExitCode, long DurationMs, string OutputTail)> RunPreviewVerifyAsync(
|
||||
string repoDir, string targetBranch, string treeOid, string verifyCommand, CancellationToken ct)
|
||||
{
|
||||
var sw = System.Diagnostics.Stopwatch.StartNew();
|
||||
var scratchPath = Path.Combine(Path.GetTempPath(), "claudedo-preview-verify", Guid.NewGuid().ToString("N"));
|
||||
try
|
||||
{
|
||||
var parentSha = await _git.RevParseAsync(repoDir, targetBranch, ct);
|
||||
var commitSha = await _git.CommitTreeAsync(repoDir, treeOid, parentSha, "claudedo preview verify", ct);
|
||||
Directory.CreateDirectory(Path.GetDirectoryName(scratchPath)!);
|
||||
await _git.WorktreeAddDetachedAsync(repoDir, scratchPath, commitSha, ct);
|
||||
|
||||
VerifyCommandResult result;
|
||||
try
|
||||
{
|
||||
result = await _verify.RunAsync(scratchPath, verifyCommand, VerifyTimeout, ct);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return (-1, sw.ElapsedMilliseconds, $"verify command failed to start: {ex.Message}");
|
||||
}
|
||||
|
||||
var exitCode = result.TimedOut ? -1 : result.ExitCode;
|
||||
var tail = exitCode == 0 ? "" : TailOutput(result.Output);
|
||||
return (exitCode, sw.ElapsedMilliseconds, tail);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
return (-1, sw.ElapsedMilliseconds, $"failed to prepare merge preview for verification: {ex.Message}");
|
||||
}
|
||||
finally
|
||||
{
|
||||
// Best-effort cleanup on the un-cancelled token — a caller-cancelled ct must not
|
||||
// leave the scratch worktree behind.
|
||||
try { await _git.WorktreeRemoveAsync(repoDir, scratchPath, force: true, CancellationToken.None); }
|
||||
catch { /* nothing more we can do */ }
|
||||
try { if (Directory.Exists(scratchPath)) Directory.Delete(scratchPath, recursive: true); }
|
||||
catch { /* best-effort */ }
|
||||
}
|
||||
}
|
||||
|
||||
public Task<MergeResult> ApproveAndMergeAsync(string taskId, string targetBranch, CancellationToken ct)
|
||||
|
||||
Reference in New Issue
Block a user