diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index 7a56023d..df7994e3 100644 --- a/docs/explore-notes/review-merge.md +++ b/docs/explore-notes/review-merge.md @@ -1,7 +1,7 @@ # Review, merge & conflict resolution > **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative. -> Last verified against `2f3f938` (2026-08-07), which finished the diff-viewer rework: +> Last verified against `6a2a19c` (2026-08-10), which added the preview-time verify build. > Planning mode renders per file and `DiffLinesView` is retired. > Drift check: `git log --oneline 20bce9b..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts src/ClaudeDo.Worker/External` > Stable structure only (no line numbers). See docs/explore-notes/README.md. @@ -115,6 +115,26 @@ the target branch at once would be the one run nothing checks. so a verify run can't be interrupted by a second merge landing in the same working dir mid-build. +### Verify in the preview, not just post-merge + +`TaskMergeService.PreviewAsync(taskId, targetBranch, runVerify, ct)` can additionally build/test a +clean `git merge-tree` result *before* anything is merged. It never touches the real working tree: +the tree `PreviewMergeAsync` would write is wrapped in a throwaway commit +(`GitService.CommitTreeAsync`, parent = the target branch's current tip) and checked out into a +detached-HEAD scratch worktree under the OS temp dir (`GitService.WorktreeAddDetachedAsync`), which +is always removed afterward (`finally`, non-cancellable cleanup). No verify command configured, or +`runVerify=false`, reproduces the pre-existing preview exactly — no delay, no scratch worktree. + +Wired into `ExternalMcpService`: `preview_merge` (single task) always requests a verify run when the +list has a command configured; `preview_merge_set` only does when its `runVerify` parameter is +explicitly set (default `false`) — a set preview never starts N builds unasked. The Hub's +`PreviewMerge` (the UI's live mergeability indicator) always passes `runVerify: false`, since +running a build on every poll would be a bad regression; `MergePreviewDto` carries the +verify fields anyway so a future UI entry point can opt in. Result fields: `VerifyExitCode` (null = +not run; `-1` = timed out or failed to start; mirrors the post-merge gate's convention), +`VerifyDurationMs`, `VerifyOutputTail` (tail of output, only populated on non-zero exit, via the +same `TailOutput` helper the post-merge gate uses). + ## `MergeCommit` and revert `WorktreeEntity.MergeCommit` (nullable) is the SHA of the merge commit this worktree's branch diff --git a/src/ClaudeDo.Data/Git/GitService.cs b/src/ClaudeDo.Data/Git/GitService.cs index 651796b4..a96c1a88 100644 --- a/src/ClaudeDo.Data/Git/GitService.cs +++ b/src/ClaudeDo.Data/Git/GitService.cs @@ -3,7 +3,10 @@ using System.Text; namespace ClaudeDo.Data.Git; -public sealed record MergePreview(bool Supported, bool Clean, IReadOnlyList 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 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()); + // stdout is just the written tree's oid on a clean merge. + return new MergePreview(true, true, Array.Empty(), stdout.Trim()); if (exitCode == 1) { @@ -377,6 +381,62 @@ public sealed class GitService return new MergePreview(false, false, Array.Empty()); } + /// Resolves (a branch, tag, or SHA) to a full commit SHA. + public async Task 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(); + } + + /// + /// Creates a new commit object wrapping with + /// as its single parent, writing only a loose object — no ref is created or moved. + /// + public async Task 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(); + } + + /// + /// Materializes into a new, branchless worktree at + /// (detached HEAD) — used to build/verify a merge-tree + /// result without ever creating a branch or touching the real working tree. + /// + 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(); + } + } + /// Count of files that differ on since its merge base with the target. public async Task CountChangedFilesAsync( string repoDir, string targetBranch, string sourceBranch, CancellationToken ct = default) diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 23fc6319..5cca8e0c 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -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 ConflictFiles, int ChangedFileCount, int Behind, bool IsEmpty = false); + string Status, IReadOnlyList 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 ConflictFiles, int ChangedFileCount, int Behind, string? Error, bool IsEmpty = false); + string TaskId, string Status, IReadOnlyList 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 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 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 PreviewMergeSet( IReadOnlyList 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 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)."); diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index 7add000e..71d72a52 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -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 ConflictFiles, string? ErrorMessage); -public record MergePreviewDto(string Status, IReadOnlyList 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 ConflictFiles, int ChangedFileCount, + int? VerifyExitCode = null, long? VerifyDurationMs = null, string? VerifyOutputTail = null); public record MergeTargetsDto(string DefaultBranch, IReadOnlyList LocalBranches); public record MergeConflictDocumentsDto(string TaskId, IReadOnlyList Files); public record ConflictDocumentDto(string Path, bool IsBinary, IReadOnlyList Segments); diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index e98c1c05..a5389e3c 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -19,10 +19,17 @@ public sealed record MergeTargets( string DefaultBranch, IReadOnlyList 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 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 PreviewAsync(string taskId, string targetBranch, CancellationToken ct) + public Task PreviewAsync(string taskId, string targetBranch, CancellationToken ct) + => PreviewAsync(taskId, targetBranch, runVerify: false, ct); + + /// + /// Non-destructive merge preview via . When + /// 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. + /// + public async Task 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(), 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(), count); + + if (!runVerify || string.IsNullOrWhiteSpace(verifyCommand) || preview.TreeOid is null) + return new MergePreviewResult(PreviewClean, Array.Empty(), count); + + var (exitCode, durationMs, outputTail) = await RunPreviewVerifyAsync( + list.WorkingDir, target, preview.TreeOid, verifyCommand, ct); + return new MergePreviewResult(PreviewClean, Array.Empty(), count, exitCode, durationMs, outputTail); + } + + /// + /// 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. + /// + 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 ApproveAndMergeAsync(string taskId, string targetBranch, CancellationToken ct) diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 4f7ef325..ccd73ed5 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -10,6 +10,7 @@ using ClaudeDo.Worker.Planning; using ClaudeDo.Worker.Queue; using ClaudeDo.Worker.Runner; using ClaudeDo.Worker.Tests.Infrastructure; +using ClaudeDo.Worker.Tests.Services; using ClaudeDo.Worker.Usage; using ClaudeDo.Worker.Worktrees; using Microsoft.AspNetCore.SignalR; @@ -123,13 +124,15 @@ public sealed class ExternalMcpServiceTests : IDisposable return task; } - private ExternalMcpService BuildSut(QueueService queue) + private ExternalMcpService BuildSut(QueueService queue) => BuildSut(queue, new VerifyCommandRunner()); + + private ExternalMcpService BuildSut(QueueService queue, IVerifyCommandRunner verify) { var git = new GitService(); var factory = _db.CreateFactory(); var maintenance = new WorktreeMaintenanceService(factory, git, NullLogger.Instance); var state = TaskStateServiceBuilder.Build(factory).State; - var merge = new TaskMergeService(factory, git, _broadcaster, state, new VerifyCommandRunner(), NullLogger.Instance); + var merge = new TaskMergeService(factory, git, _broadcaster, state, verify, NullLogger.Instance); var aggregator = new PlanningAggregator(factory, git, NullLogger.Instance); var planningMerge = new PlanningMergeOrchestrator( factory, merge, aggregator, _broadcaster, git, state, NullLogger.Instance); @@ -1609,6 +1612,36 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Contains("working directory", ex.Message, StringComparison.OrdinalIgnoreCase); } + [Fact] + public async Task PreviewMerge_VerifyCommandConfigured_RunsInScratchWorktreeAndDoesNotTouchTarget() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var (task, list, wt) = await SeedWorktreeAsync(TaskStatus.WaitingForReview); + File.WriteAllText(Path.Combine(wt.WorktreePath, "added.txt"), "content"); + var cfg = new WorkerConfig { WorktreeRootStrategy = "sibling" }; + var mgr = new WorktreeManager(new GitService(), _db.CreateFactory(), cfg, NullLogger.Instance); + await mgr.CommitIfChangedAsync(wt, task, list, CancellationToken.None); + + using (var ctx = _db.CreateContext()) + await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity { ListId = list.Id, VerifyCommand = "dotnet build" }); + + var git = new GitService(); + var target = await git.GetCurrentBranchAsync(list.WorkingDir!, CancellationToken.None); + var headBefore = await git.RevParseHeadAsync(list.WorkingDir!, CancellationToken.None); + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(0, false, "ok") }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var result = await sut.PreviewMerge(task.Id, target, CancellationToken.None); + + Assert.Equal(TaskMergeService.PreviewClean, result.Status); + Assert.Equal(0, result.VerifyExitCode); + Assert.Equal("dotnet build", fakeVerify.CapturedCommand); + Assert.NotEqual(list.WorkingDir, fakeVerify.CapturedWorkingDir); + Assert.Equal(headBefore, await git.RevParseHeadAsync(list.WorkingDir!, CancellationToken.None)); + Assert.False(await git.HasChangesAsync(list.WorkingDir!, CancellationToken.None)); + } + [Fact] public async Task PreviewMergeSet_TwoTasksSharedFile_ReportsOverlap() { @@ -1627,7 +1660,7 @@ public sealed class ExternalMcpServiceTests : IDisposable await CreateActiveWorktreeAsync(repo, taskB.Id, "shared.txt", "b change\n"); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMergeSet([taskA.Id, taskB.Id], "main", CancellationToken.None); + var result = await sut.PreviewMergeSet([taskA.Id, taskB.Id], "main", cancellationToken: CancellationToken.None); Assert.Equal(2, result.Tasks.Count); Assert.All(result.Tasks, t => Assert.Null(t.Error)); @@ -1648,7 +1681,7 @@ public sealed class ExternalMcpServiceTests : IDisposable var target = await new GitService().GetCurrentBranchAsync(list.WorkingDir!, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMergeSet([task.Id], target, CancellationToken.None); + var result = await sut.PreviewMergeSet([task.Id], target, cancellationToken: CancellationToken.None); Assert.Single(result.Tasks); Assert.Empty(result.Overlaps); @@ -1665,7 +1698,7 @@ public sealed class ExternalMcpServiceTests : IDisposable var target = await new GitService().GetCurrentBranchAsync(list.WorkingDir!, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMergeSet([task.Id, noWorktreeTask.Id], target, CancellationToken.None); + var result = await sut.PreviewMergeSet([task.Id, noWorktreeTask.Id], target, cancellationToken: CancellationToken.None); Assert.Equal(2, result.Tasks.Count); var ok = result.Tasks.Single(t => t.TaskId == task.Id); @@ -1710,7 +1743,7 @@ public sealed class ExternalMcpServiceTests : IDisposable await CreateActiveWorktreeAsync(repo, fullTask.Id, "full.txt", "content\n"); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMergeSet([emptyTask.Id, fullTask.Id], "main", CancellationToken.None); + var result = await sut.PreviewMergeSet([emptyTask.Id, fullTask.Id], "main", cancellationToken: CancellationToken.None); var emptyEntry = result.Tasks.Single(t => t.TaskId == emptyTask.Id); var fullEntry = result.Tasks.Single(t => t.TaskId == fullTask.Id); @@ -1720,6 +1753,54 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.False(fullEntry.IsEmpty); } + [Fact] + public async Task PreviewMergeSet_DefaultRunVerifyFalse_NeverInvokesRunnerEvenWithCommandConfigured() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + using (var ctx = _db.CreateContext()) + await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity { ListId = listId, VerifyCommand = "dotnet build" }); + var task = await SeedTaskAsync(listId, "a", TaskStatus.WaitingForReview); + await CreateActiveWorktreeAsync(repo, task.Id, "added.txt", "content\n"); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(1, false, "should never run") }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var result = await sut.PreviewMergeSet([task.Id], "main", cancellationToken: CancellationToken.None); + + Assert.Null(result.Tasks.Single().VerifyExitCode); + Assert.Null(fakeVerify.CapturedCommand); + } + + [Fact] + public async Task PreviewMergeSet_RunVerifyTrue_InvokesRunnerPerTask() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + using (var ctx = _db.CreateContext()) + await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity { ListId = listId, VerifyCommand = "dotnet build" }); + var task = await SeedTaskAsync(listId, "a", TaskStatus.WaitingForReview); + await CreateActiveWorktreeAsync(repo, task.Id, "added.txt", "content\n"); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(0, false, "ok") }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var result = await sut.PreviewMergeSet([task.Id], "main", runVerify: true, cancellationToken: CancellationToken.None); + + Assert.Equal(0, result.Tasks.Single().VerifyExitCode); + Assert.Equal("dotnet build", fakeVerify.CapturedCommand); + } + // ── AddTask model override ──────────────────────────────────────────────── [Fact] diff --git a/tests/ClaudeDo.Worker.Tests/Runner/GitServicePreviewMergeTests.cs b/tests/ClaudeDo.Worker.Tests/Runner/GitServicePreviewMergeTests.cs index 88c5ef20..8f612b66 100644 --- a/tests/ClaudeDo.Worker.Tests/Runner/GitServicePreviewMergeTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Runner/GitServicePreviewMergeTests.cs @@ -61,4 +61,43 @@ public class GitServicePreviewMergeTests : IDisposable Assert.Equal(headBefore, GitRepoFixture.RunGit(repo.RepoDir, "rev-parse", "HEAD").Trim()); Assert.False(await git.IsMidMergeAsync(repo.RepoDir)); } + + [Fact] + public async Task PreviewMergeAsync_Clean_ReturnsTreeOidMaterializableIntoDetachedWorktree() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var git = new GitService(); + var baseBranch = await git.GetCurrentBranchAsync(repo.RepoDir); + + GitRepoFixture.RunGit(repo.RepoDir, "checkout", "-b", "feature"); + File.WriteAllText(Path.Combine(repo.RepoDir, "newfile.txt"), "x\n"); + GitRepoFixture.RunGit(repo.RepoDir, "add", "-A"); + GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "feat"); + GitRepoFixture.RunGit(repo.RepoDir, "checkout", baseBranch); + + var preview = await git.PreviewMergeAsync(repo.RepoDir, baseBranch, "feature", CancellationToken.None); + Assert.True(preview.Clean); + Assert.False(string.IsNullOrWhiteSpace(preview.TreeOid)); + + // The tree a clean preview would produce can be wrapped in a commit and checked out into + // a detached, branchless scratch worktree -- the technique the verify-in-preview feature + // relies on -- without ever touching the real working tree. + var parentSha = await git.RevParseAsync(repo.RepoDir, baseBranch, CancellationToken.None); + var commitSha = await git.CommitTreeAsync(repo.RepoDir, preview.TreeOid!, parentSha, "scratch", CancellationToken.None); + + var scratchPath = Path.Combine(Path.GetTempPath(), $"scratch_{Guid.NewGuid():N}"); + await git.WorktreeAddDetachedAsync(repo.RepoDir, scratchPath, commitSha, CancellationToken.None); + try + { + Assert.True(File.Exists(Path.Combine(scratchPath, "newfile.txt"))); + } + finally + { + await git.WorktreeRemoveAsync(repo.RepoDir, scratchPath, force: true, CancellationToken.None); + } + + Assert.False(Directory.Exists(scratchPath)); + Assert.False(await git.HasChangesAsync(repo.RepoDir, CancellationToken.None)); + } } diff --git a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs index 2abefead..dcae7880 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs @@ -589,6 +589,154 @@ public class TaskMergeServiceTests : IDisposable Assert.Equal(TaskMergeService.PreviewUnavailable, preview.Status); } + [Fact] + public async Task PreviewAsync_RunVerifyTrue_NoVerifyCommandConfigured_NeverInvokesRunner() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, repo.RepoDir, TaskStatus.WaitingForReview); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "x\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(1, false, "should never run") }; + var (svc, _) = BuildService(db, fakeVerify); + var target = await new GitService().GetCurrentBranchAsync(repo.RepoDir); + + var preview = await svc.PreviewAsync(task.Id, target, runVerify: true, CancellationToken.None); + + Assert.Equal(TaskMergeService.PreviewClean, preview.Status); + Assert.Null(preview.VerifyExitCode); + Assert.Null(fakeVerify.CapturedCommand); + } + + [Fact] + public async Task PreviewAsync_RunVerifyFalse_VerifyCommandConfigured_NeverInvokesRunner() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, repo.RepoDir, TaskStatus.WaitingForReview); + await SeedVerifyCommand(db, list.Id, "dotnet build"); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "x\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(1, false, "should never run") }; + var (svc, _) = BuildService(db, fakeVerify); + var target = await new GitService().GetCurrentBranchAsync(repo.RepoDir); + + var preview = await svc.PreviewAsync(task.Id, target, CancellationToken.None); + + Assert.Equal(TaskMergeService.PreviewClean, preview.Status); + Assert.Null(preview.VerifyExitCode); + Assert.Null(fakeVerify.CapturedCommand); + } + + [Fact] + public async Task PreviewAsync_RunVerifyTrue_VerifyPasses_ReturnsCleanBuildAndLeavesTargetUntouched() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, repo.RepoDir, TaskStatus.WaitingForReview); + await SeedVerifyCommand(db, list.Id, "dotnet build"); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "x\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var fakeVerify = new FakeVerifyCommandRunner + { + Result = new VerifyCommandResult(0, false, "build ok"), + FileToCheck = "added.txt", + }; + var (svc, _) = BuildService(db, fakeVerify); + var git = new GitService(); + var target = await git.GetCurrentBranchAsync(repo.RepoDir); + var statusBefore = await git.GetStatusPorcelainAsync(repo.RepoDir); + var headBefore = await git.RevParseHeadAsync(repo.RepoDir); + + var preview = await svc.PreviewAsync(task.Id, target, runVerify: true, CancellationToken.None); + + Assert.Equal(TaskMergeService.PreviewClean, preview.Status); + Assert.Equal(0, preview.VerifyExitCode); + Assert.NotNull(preview.VerifyDurationMs); + Assert.Equal("dotnet build", fakeVerify.CapturedCommand); + Assert.NotEqual(repo.RepoDir, fakeVerify.CapturedWorkingDir); + Assert.True(fakeVerify.WorkingDirHadFile, "the merged file must be present in the scratch worktree at verify time"); + + // Scratch worktree removed afterward; the real target working tree never changed. + Assert.False(Directory.Exists(fakeVerify.CapturedWorkingDir)); + Assert.Equal(statusBefore, await git.GetStatusPorcelainAsync(repo.RepoDir)); + Assert.Equal(headBefore, await git.RevParseHeadAsync(repo.RepoDir)); + } + + [Fact] + public async Task PreviewAsync_RunVerifyTrue_VerifyFails_ReturnsExitCodeAndOutputTailWithoutTouchingTarget() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, repo.RepoDir, TaskStatus.WaitingForReview); + await SeedVerifyCommand(db, list.Id, "dotnet build"); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "x\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(1, false, "build broke: CS0103") }; + var (svc, _) = BuildService(db, fakeVerify); + var git = new GitService(); + var target = await git.GetCurrentBranchAsync(repo.RepoDir); + var headBefore = await git.RevParseHeadAsync(repo.RepoDir); + + var preview = await svc.PreviewAsync(task.Id, target, runVerify: true, CancellationToken.None); + + // The merge-tree result itself is still clean -- only the build/test of it failed. + Assert.Equal(TaskMergeService.PreviewClean, preview.Status); + Assert.Equal(1, preview.VerifyExitCode); + Assert.Contains("build broke: CS0103", preview.VerifyOutputTail); + Assert.False(Directory.Exists(fakeVerify.CapturedWorkingDir)); + Assert.Equal(headBefore, await git.RevParseHeadAsync(repo.RepoDir)); + } + + [Fact] + public async Task PreviewAsync_RunVerifyTrue_CleansUpScratchWorktreeRegistration() + { + if (!GitRepoFixture.IsGitAvailable()) return; + var repo = NewRepo(); + var db = NewDb(); + var (list, task) = await SeedListAndTask(db, repo.RepoDir, TaskStatus.WaitingForReview); + await SeedVerifyCommand(db, list.Id, "dotnet build"); + + var wtMgr = BuildWorktreeManager(db); + var wtCtx = await wtMgr.CreateAsync(task, list, CancellationToken.None); + _wtCleanups.Add((repo.RepoDir, wtCtx.WorktreePath)); + File.WriteAllText(Path.Combine(wtCtx.WorktreePath, "added.txt"), "x\n"); + await wtMgr.CommitIfChangedAsync(wtCtx, task, list, CancellationToken.None); + + var fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(0, false, "ok") }; + var (svc, _) = BuildService(db, fakeVerify); + var target = await new GitService().GetCurrentBranchAsync(repo.RepoDir); + + await svc.PreviewAsync(task.Id, target, runVerify: true, CancellationToken.None); + + var worktreeListing = GitRepoFixture.RunGit(repo.RepoDir, "worktree", "list", "--porcelain"); + Assert.DoesNotContain(fakeVerify.CapturedWorkingDir!, worktreeListing); + } + [Fact] public async Task ApproveAndMergeAsync_CleanWorktree_MergesAndMarksDone() { @@ -1159,10 +1307,17 @@ internal sealed class FakeVerifyCommandRunner : IVerifyCommandRunner public string? CapturedWorkingDir { get; private set; } public string? CapturedCommand { get; private set; } + // Set before the call to snapshot, at call time, whether this file exists in workingDir -- + // lets a preview-verify test prove the scratch worktree actually contains the merged content. + public string? FileToCheck { get; set; } + public bool? WorkingDirHadFile { get; private set; } + public Task RunAsync(string workingDir, string command, TimeSpan timeout, CancellationToken ct) { CapturedWorkingDir = workingDir; CapturedCommand = command; + if (FileToCheck is not null) + WorkingDirHadFile = File.Exists(Path.Combine(workingDir, FileToCheck)); return Task.FromResult(Result); } }