diff --git a/docs/explore-notes/review-merge.md b/docs/explore-notes/review-merge.md index cb74c29d..2351dfc1 100644 --- a/docs/explore-notes/review-merge.md +++ b/docs/explore-notes/review-merge.md @@ -184,9 +184,9 @@ detached-HEAD scratch worktree under the OS temp dir (`GitService.WorktreeAddDet 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 +Wired into `ExternalMcpService`: both `preview_merge` (single task) and `preview_merge_set` only +request a verify run when their `runVerify` parameter is explicitly set (default `false`) — a +preview never starts a build 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 = diff --git a/src/ClaudeDo.Data/PromptFiles.cs b/src/ClaudeDo.Data/PromptFiles.cs index 19eeff4e..14340a7f 100644 --- a/src/ClaudeDo.Data/PromptFiles.cs +++ b/src/ClaudeDo.Data/PromptFiles.cs @@ -523,7 +523,7 @@ public static class PromptFiles - Clean merge → mergeStatus "merged_verify_pending" (expected: the task deliberately stays WaitingForReview until verify_merges passes) or "merged" (list has no verify command; the task is Done). Either way, move on. - Conflict (markers left in the working tree, repoPath returned) → resolve it below. - After each merge, main has moved — a clean preview_merge_set result from before this merge is now stale for the remaining tasks. If you are unsure whether an earlier overlap warning still matters, call preview_merge for the next task again before merging it. + After each merge, main has moved — a clean preview_merge_set result from before this merge is now stale for the remaining tasks. If you are unsure whether an earlier overlap warning still matters, call preview_merge for the next task again before merging it (default runVerify=false — this is just a conflict recheck, not a build; the verify command still runs once via verify_merges after the last merge). Every branch in this run forked from the same base, so conflicts between them are the NORMAL case, not a failure. Resolve them and keep going; do not abandon the run because a merge conflicted. diff --git a/src/ClaudeDo.Worker/External/MergeMcpTools.cs b/src/ClaudeDo.Worker/External/MergeMcpTools.cs index 3cca7122..24952f56 100644 --- a/src/ClaudeDo.Worker/External/MergeMcpTools.cs +++ b/src/ClaudeDo.Worker/External/MergeMcpTools.cs @@ -446,26 +446,30 @@ public sealed class MergeMcpTools "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. 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. staleFiles lists files this branch touches that the target branch ALSO " + - "changed since this branch's fork point — a more honest staleness signal than `behind` alone, since a " + - "branch can be far behind yet touch nothing the target changed, or barely behind yet collide on the one " + - "file that matters (always empty for a worktree-less handler task, which has no fork point). Throws if the " + - "task has neither an active worktree nor a handler commit range, or the list's working directory is missing " + - "from disk." + McpToolDocs.ProgressHint)] + "branch even when the preview is clean. runVerify=false (default) never builds. runVerify=true additionally " + + "builds/tests a clean preview in a scratch worktree (still without touching the real working tree), if the " + + "list has a verify command configured — 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/verifyDurationMs/verifyOutputTail stay null without runVerify=true or without a configured " + + "verify command. isEmpty=true means the task's review range contributed nothing; check that flag rather " + + "than reading a small changedFileCount as empty. staleFiles lists files this branch touches that the " + + "target branch ALSO changed since this branch's fork point — a more honest staleness signal than `behind` " + + "alone, since a branch can be far behind yet touch nothing the target changed, or barely behind yet " + + "collide on the one file that matters (always empty for a worktree-less handler task, which has no fork " + + "point). Throws if the task has neither an active worktree nor a handler commit range, or the list's " + + "working directory is missing from disk." + McpToolDocs.ProgressHint)] public async Task PreviewMerge( string taskId, [Description("Branch to preview against; defaults to the repo's current branch.")] string? targetBranch = null, + [Description("true: also run the verify command (if configured), building in a scratch worktree. " + + "false (default): no build.")] + bool runVerify = false, CancellationToken cancellationToken = default, IProgress? progress = null) { taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); - var (preview, behind, _, isEmpty, staleFiles, _, _) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify: true, 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); } diff --git a/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs b/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs index 0b7b3e5a..58e6676b 100644 --- a/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/MergeMcpToolsTests.cs @@ -793,7 +793,7 @@ public sealed class MergeMcpToolsTests : IDisposable var headBefore = await git.RevParseHeadAsync(list.WorkingDir!, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, target, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, target, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewClean, result.Status); Assert.Empty(result.ConflictFiles); @@ -815,7 +815,7 @@ public sealed class MergeMcpToolsTests : IDisposable var target = await new GitService().GetCurrentBranchAsync(list.WorkingDir!, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, target, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, target, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewClean, result.Status); Assert.Equal(0, result.ChangedFileCount); @@ -843,7 +843,7 @@ public sealed class MergeMcpToolsTests : IDisposable } var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, null, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, null, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewClean, result.Status); Assert.Equal(0, result.ChangedFileCount); @@ -875,7 +875,7 @@ public sealed class MergeMcpToolsTests : IDisposable } var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, null, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, null, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewClean, result.Status); Assert.Equal(1, result.ChangedFileCount); @@ -893,7 +893,7 @@ public sealed class MergeMcpToolsTests : IDisposable var headBefore = await git.RevParseHeadAsync(list.WorkingDir!, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, target, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, target, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewConflict, result.Status); Assert.Contains("README.md", result.ConflictFiles); @@ -910,7 +910,7 @@ public sealed class MergeMcpToolsTests : IDisposable var sut = BuildSut(CreateQueue()); await Assert.ThrowsAsync( - () => sut.PreviewMerge(task.Id, null, CancellationToken.None)); + () => sut.PreviewMerge(task.Id, null, cancellationToken: CancellationToken.None)); } [Fact] @@ -928,7 +928,7 @@ public sealed class MergeMcpToolsTests : IDisposable var sut = BuildSut(CreateQueue()); var ex = await Assert.ThrowsAsync( - () => sut.PreviewMerge(task.Id, null, CancellationToken.None)); + () => sut.PreviewMerge(task.Id, null, cancellationToken: CancellationToken.None)); Assert.Contains("working directory", ex.Message, StringComparison.OrdinalIgnoreCase); } @@ -952,7 +952,7 @@ public sealed class MergeMcpToolsTests : IDisposable 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); + var result = await sut.PreviewMerge(task.Id, target, runVerify: true, cancellationToken: CancellationToken.None); Assert.Equal(TaskMergeService.PreviewClean, result.Status); Assert.Equal(0, result.VerifyExitCode); @@ -962,6 +962,34 @@ public sealed class MergeMcpToolsTests : IDisposable Assert.False(await git.HasChangesAsync(list.WorkingDir!, CancellationToken.None)); } + [Fact] + public async Task PreviewMerge_DefaultRunVerifyFalse_NeverInvokesRunnerEvenWithCommandConfigured() + { + 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 fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(0, false, "ok") }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var result = await sut.PreviewMerge(task.Id, target, cancellationToken: CancellationToken.None); + + Assert.Equal(TaskMergeService.PreviewClean, result.Status); + Assert.Null(result.VerifyExitCode); + Assert.Null(result.VerifyDurationMs); + Assert.Null(result.VerifyOutputTail); + Assert.Null(fakeVerify.CapturedCommand); + } + [Fact] public async Task PreviewMerge_TargetChangedSameFileSinceFork_ReportsStaleFiles() { @@ -982,7 +1010,7 @@ public sealed class MergeMcpToolsTests : IDisposable GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "main also touched shared.txt"); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, "main", CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, "main", cancellationToken: CancellationToken.None); Assert.Equal(1, result.Behind); Assert.NotNull(result.StaleFiles); @@ -1010,7 +1038,7 @@ public sealed class MergeMcpToolsTests : IDisposable GitRepoFixture.RunGit(repo.RepoDir, "commit", "-m", "main touched something else"); var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, "main", CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, "main", cancellationToken: CancellationToken.None); Assert.Equal(1, result.Behind); Assert.NotNull(result.StaleFiles); @@ -1042,7 +1070,7 @@ public sealed class MergeMcpToolsTests : IDisposable } var sut = BuildSut(CreateQueue()); - var result = await sut.PreviewMerge(task.Id, null, CancellationToken.None); + var result = await sut.PreviewMerge(task.Id, null, cancellationToken: CancellationToken.None); Assert.NotNull(result.StaleFiles); Assert.Empty(result.StaleFiles!);