diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index b727fc25..1f6a3d97 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -15,6 +15,7 @@ using ClaudeDo.Worker.Queue; using ClaudeDo.Worker.State; using ClaudeDo.Worker.Worktrees; using Microsoft.EntityFrameworkCore; +using ModelContextProtocol; using ModelContextProtocol.Server; using TaskStatus = ClaudeDo.Data.Models.TaskStatus; @@ -644,7 +645,7 @@ public sealed class ExternalMcpService "means the merge stopped on conflicts, with the files listed. emptyChildren (parent approve only) lists " + "the Done children about to be unit-merged whose own review range contributed nothing (e.g. a child that " + "reported CLAUDEDO_BLOCKED and committed no code) — check it before trusting that every child actually " + - "delivered something." + McpToolDocs.LeanTaskRef + McpToolDocs.TaskNumberHint)] + "delivered something." + McpToolDocs.LeanTaskRef + McpToolDocs.TaskNumberHint + McpToolDocs.ProgressHint)] public async Task ReviewTask( string taskId, [Description("'approve', 'reject_rerun', 'reject_park' or 'cancel'.")] @@ -660,8 +661,15 @@ public sealed class ExternalMcpService "in the working tree (repoPath in the result) so you can resolve them and call continue_merge, " + "or abort_merge to cancel.")] bool leaveConflictsInTree = false, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { + // First report fires before any git/verify work starts -- an approve that has to wait on + // the per-repo merge gate (another merge/verify already running) must not sit silent long + // enough to trip Claude Code's ~300s MCP idle-silence abort before RunVerifyGateAsync's own + // periodic reports even begin. + progress?.Report(new ProgressNotificationValue { Progress = 0, Message = "review_task started" }); + taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); var task = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); @@ -702,7 +710,7 @@ public sealed class ExternalMcpService } else { - var r = await _merge.ApproveAndMergeAsync(taskId, targetBranch ?? "", leaveConflictsInTree, cancellationToken); + var r = await _merge.ApproveAndMergeAsync(taskId, targetBranch ?? "", leaveConflictsInTree, cancellationToken, progress); if (r.Status == TaskMergeService.StatusBlocked) throw new InvalidOperationException(r.ErrorMessage ?? "approve failed"); mergeStatus = r.Status; @@ -947,7 +955,7 @@ public sealed class ExternalMcpService [McpServerTool, Description( "Merge a Done task's worktree branch into targetBranch. For a task still in WaitingForReview prefer " + "review_task, which merges as part of approving. merged=true carries the new mergeCommit SHA; on conflict " + - "merged=false and conflicts lists the affected files.")] + "merged=false and conflicts lists the affected files." + McpToolDocs.ProgressHint)] public async Task MergeTask( string taskId, string targetBranch = "main", @@ -961,8 +969,11 @@ public sealed class ExternalMcpService "leave the conflict markers in the working tree at repoPath (conflictsInTree=true) so you can " + "resolve them there and call continue_merge, or abort_merge to cancel.")] bool leaveConflictsInTree = false, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { + progress?.Report(new ProgressNotificationValue { Progress = 0, Message = "merge_task started" }); + taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); var task = await _tasks.GetByIdAsync(taskId, cancellationToken) ?? throw new InvalidOperationException($"Task {taskId} not found."); @@ -988,7 +999,7 @@ public sealed class ExternalMcpService var commitMessage = $"Merge task branch for: {task.Title}"; var result = await _merge.MergeAsync( - taskId, targetBranch, removeWorktree: false, commitMessage, leaveConflictsInTree, cancellationToken); + taskId, targetBranch, removeWorktree: false, commitMessage, leaveConflictsInTree, cancellationToken, progress); if (result.Status == TaskMergeService.StatusMerged) { @@ -1226,15 +1237,16 @@ public sealed class ExternalMcpService "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.")] + "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, - CancellationToken cancellationToken = default) + 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); + var (preview, behind, _, isEmpty, staleFiles, _) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify: true, cancellationToken, progress); return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, isEmpty, preview.VerifyExitCode, preview.VerifyDurationMs, preview.VerifyOutputTail, staleFiles); } @@ -1251,7 +1263,7 @@ public sealed class ExternalMcpService "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.")] + "since builds run one at a time." + McpToolDocs.ProgressHint)] public async Task PreviewMergeSet( IReadOnlyList taskIds, [Description("Branch to preview every task against; defaults to the repo's current branch.")] @@ -1259,7 +1271,8 @@ public sealed class ExternalMcpService [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) + CancellationToken cancellationToken = default, + IProgress? progress = null) { if (taskIds is null || taskIds.Count == 0) throw new InvalidOperationException("taskIds must contain at least one task id."); @@ -1274,7 +1287,7 @@ public sealed class ExternalMcpService { try { - var (preview, behind, changedFiles, isEmpty, staleFiles, number) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken); + var (preview, behind, changedFiles, isEmpty, staleFiles, number) = 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)); @@ -1343,7 +1356,8 @@ 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, IReadOnlyList StaleFiles, int Number)> PreviewMergeCoreAsync( - string taskId, string? targetBranch, bool runVerify, CancellationToken ct) + string taskId, string? targetBranch, bool runVerify, CancellationToken ct, + IProgress? progress = null) { using var ctx = _dbFactory.CreateDbContext(); var task = await new TaskRepository(ctx).GetByIdAsync(taskId, ct) @@ -1360,7 +1374,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 ?? "", runVerify, ct); + var preview = await _merge.PreviewAsync(taskId, targetBranch ?? "", runVerify, ct, progress); 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/External/McpToolDocs.cs b/src/ClaudeDo.Worker/External/McpToolDocs.cs index 5b570049..c0b029a9 100644 --- a/src/ClaudeDo.Worker/External/McpToolDocs.cs +++ b/src/ClaudeDo.Worker/External/McpToolDocs.cs @@ -45,4 +45,15 @@ internal static class McpToolDocs "'theirs' side (the branch being merged in), plus an optional 'base' — the common-ancestor text for that " + "hunk, shown between a third '|||||||' marker and the '=======' separator. base is null when git recorded " + "none for that hunk."; + + /// + /// A merge/verify gate here can run for minutes -- warns the caller that MCP progress pings + /// keep the call alive across the calling client's own idle-silence abort. Same rationale as + /// WaitForTaskChange's own clause (External/TaskWaitMcpTools.cs), worded for a merge instead + /// of a wait. + /// + public const string ProgressHint = + " Sends MCP progress pings periodically while a merge or verify gate runs, so a long call survives the " + + "calling client's own idle-silence abort (Claude Code defaults to killing an MCP call after ~300s of " + + "silence) -- this is not guaranteed by every possible MCP client."; } diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index ac02d7b4..2f530602 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -6,6 +6,7 @@ using ClaudeDo.Data.Repositories; using ClaudeDo.Worker.Hub; using ClaudeDo.Worker.State; using Microsoft.EntityFrameworkCore; +using ModelContextProtocol; using TaskStatus = ClaudeDo.Data.Models.TaskStatus; namespace ClaudeDo.Worker.Lifecycle; @@ -68,6 +69,14 @@ public sealed class TaskMergeService // per-list configurable value on top of what the spec calls for. private static readonly TimeSpan VerifyTimeout = TimeSpan.FromMinutes(10); + // Mirrors TaskWaitMcpTools.ProgressReportInterval (External/TaskWaitMcpTools.cs): a verify + // run can take up to VerifyTimeout, well past Claude Code's ~300s MCP idle-silence abort, so + // RunReportingProgressAsync below reports on this cadence to keep the calling review_task/ + // merge_task/preview_merge* call alive. Not readonly -- tests shrink it to observe a report + // without waiting 30s. A separate field from TaskWaitMcpTools' own (rather than sharing it) + // so shrinking one for a test can't race the other's tests. + internal static TimeSpan ProgressReportInterval = TimeSpan.FromSeconds(30); + // Serializes merge (+ verify) against the same repo working dir: a verify command running // in list.WorkingDir must not see a second merge land mid-build. Keyed by working dir since // TaskMergeService is a process-wide singleton and merges across different lists are independent. @@ -122,14 +131,16 @@ public sealed class TaskMergeService /// CLAUDE.md — only the Done transition is withheld). /// private async Task RunVerifyGateAsync( - string? verifyCommand, string workingDir, CancellationToken ct) + string? verifyCommand, string workingDir, CancellationToken ct, + IProgress? progress = null) { if (string.IsNullOrWhiteSpace(verifyCommand)) return null; VerifyCommandResult result; try { - result = await _verify.RunAsync(workingDir, verifyCommand, VerifyTimeout, ct); + result = await RunReportingProgressAsync( + _verify.RunAsync(workingDir, verifyCommand, VerifyTimeout, ct), progress, "verify gate running"); } catch (Exception ex) { @@ -146,6 +157,26 @@ public sealed class TaskMergeService return new MergeResult(StatusVerifyFailed, Array.Empty(), $"{reason}\n{TailOutput(result.Output)}"); } + /// + /// Awaits while reporting MCP progress every + /// so a caller waiting on a long verify run doesn't hit + /// the MCP client's own idle-silence abort. No-op passthrough when + /// is null (every non-MCP caller, e.g. the Hub). + /// + private static async Task RunReportingProgressAsync( + Task work, IProgress? progress, string message) + { + if (progress is null) return await work; + + var sw = System.Diagnostics.Stopwatch.StartNew(); + while (true) + { + var finished = await Task.WhenAny(work, Task.Delay(ProgressReportInterval)); + if (finished == work) return await work; + progress.Report(new ProgressNotificationValue { Progress = 0, Message = $"{message} ({sw.Elapsed:mm\\:ss})" }); + } + } + private static string TailOutput(string output, int maxChars = 4000) { var trimmed = output.Trim(); @@ -335,7 +366,8 @@ public sealed class TaskMergeService bool removeWorktree, string commitMessage, bool leaveConflictsInTree, - CancellationToken ct) + CancellationToken ct, + IProgress? progress = null) { var (task, list, wt, verifyCommand) = await LoadMergeContextAsync(taskId, ct); @@ -425,7 +457,7 @@ public sealed class TaskMergeService await MarkWorktreeMergedAsync(taskId, mergeSha, ct); await RebaseOthersAfterMergeAsync(task, list, targetBranch, oldTargetTip, mergeSha, ct); - var verifyFailure = await RunVerifyGateAsync(verifyCommand, list.WorkingDir, ct); + var verifyFailure = await RunVerifyGateAsync(verifyCommand, list.WorkingDir, ct, progress); if (verifyFailure is not null) { _logger.LogWarning("Verify command failed after merging task {TaskId}: {Reason}", taskId, verifyFailure.ErrorMessage); @@ -722,7 +754,8 @@ public sealed class TaskMergeService /// delay, no scratch worktree. /// public async Task PreviewAsync( - string taskId, string targetBranch, bool runVerify, CancellationToken ct) + string taskId, string targetBranch, bool runVerify, CancellationToken ct, + IProgress? progress = null) { var (_, list, wt, verifyCommand) = await LoadMergeContextAsync(taskId, ct); @@ -753,7 +786,7 @@ public sealed class TaskMergeService return new MergePreviewResult(PreviewClean, Array.Empty(), count); var (exitCode, durationMs, outputTail) = await RunPreviewVerifyAsync( - list.WorkingDir, target, preview.TreeOid, verifyCommand, ct); + list.WorkingDir, target, preview.TreeOid, verifyCommand, ct, progress); return new MergePreviewResult(PreviewClean, Array.Empty(), count, exitCode, durationMs, outputTail); } @@ -764,7 +797,8 @@ public sealed class TaskMergeService /// cancellation. /// private async Task<(int ExitCode, long DurationMs, string OutputTail)> RunPreviewVerifyAsync( - string repoDir, string targetBranch, string treeOid, string verifyCommand, CancellationToken ct) + string repoDir, string targetBranch, string treeOid, string verifyCommand, CancellationToken ct, + IProgress? progress = null) { var sw = System.Diagnostics.Stopwatch.StartNew(); var scratchPath = Path.Combine(Path.GetTempPath(), "claudedo-preview-verify", Guid.NewGuid().ToString("N")); @@ -778,7 +812,8 @@ public sealed class TaskMergeService VerifyCommandResult result; try { - result = await _verify.RunAsync(scratchPath, verifyCommand, VerifyTimeout, ct); + result = await RunReportingProgressAsync( + _verify.RunAsync(scratchPath, verifyCommand, VerifyTimeout, ct), progress, "merge preview verify running"); } catch (Exception ex) { @@ -808,7 +843,8 @@ public sealed class TaskMergeService => ApproveAndMergeAsync(taskId, targetBranch, leaveConflictsInTree: false, ct); public async Task ApproveAndMergeAsync( - string taskId, string targetBranch, bool leaveConflictsInTree, CancellationToken ct) + string taskId, string targetBranch, bool leaveConflictsInTree, CancellationToken ct, + IProgress? progress = null) { var (task, list, wt, verifyCommand) = await LoadMergeContextAsync(taskId, ct); @@ -828,7 +864,7 @@ public sealed class TaskMergeService await verifyGate.WaitAsync(ct); try { - var failed = await RunVerifyGateAsync(verifyCommand, list.WorkingDir!, ct); + var failed = await RunVerifyGateAsync(verifyCommand, list.WorkingDir!, ct, progress); if (failed is not null) return failed; } finally { verifyGate.Release(); } @@ -850,7 +886,7 @@ public sealed class TaskMergeService // MergeAsync transitions the task WaitingForReview -> Done on a successful merge. // Remove the worktree on approve (matching the unit-merge path) so merged // worktrees don't pile up; the merge commit on the target branch is the record. - return await MergeAsync(taskId, target, removeWorktree: true, $"Merge {wt.BranchName}", leaveConflictsInTree, ct); + return await MergeAsync(taskId, target, removeWorktree: true, $"Merge {wt.BranchName}", leaveConflictsInTree, ct, progress); } private static MergeResult Blocked(string reason) => diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 38360877..447210a0 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -16,6 +16,7 @@ using ClaudeDo.Worker.Usage; using ClaudeDo.Worker.Worktrees; using Microsoft.AspNetCore.SignalR; using Microsoft.Extensions.Logging.Abstractions; +using ModelContextProtocol; using TaskStatus = ClaudeDo.Data.Models.TaskStatus; namespace ClaudeDo.Worker.Tests.External; @@ -566,6 +567,79 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Equal(WorktreeState.Merged, verify.Worktrees.Single(w => w.TaskId == task.Id).State); } + // Regression coverage for the incident this fixes: an approve whose post-merge verify gate + // runs long enough gets no MCP traffic at all until RunVerifyGateAsync returns, so Claude + // Code's ~300s idle-silence abort kills the call -- the merge had already landed and + // committed, but the task never reached Done and every dependsOn successor stayed blocked + // until someone noticed and force-set the status by hand. Shrinks TaskMergeService's own + // progress interval (not TaskWaitMcpTools') to observe a report without waiting 30s real time. + [Fact] + public async Task ReviewTask_ApproveWithSlowVerifyCommand_ReportsProgressWellBeforeIdleTimeout() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = TaskMergeService.ProgressReportInterval; + TaskMergeService.ProgressReportInterval = TimeSpan.FromMilliseconds(50); + try + { + var (task, list, wt) = await SeedWorktreeAsync(TaskStatus.WaitingForReview); + File.WriteAllText(Path.Combine(wt.WorktreePath, "feature.txt"), "x\n"); + 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 fakeVerify = new FakeVerifyCommandRunner + { + Result = new VerifyCommandResult(0, false, "ok"), + Delay = TimeSpan.FromMilliseconds(300), + }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var reports = new List(); + var progress = new Progress(reports.Add); + + var result = await sut.ReviewTask( + task.Id, "approve", null, null, cancellationToken: CancellationToken.None, progress: progress); + + Assert.Equal(TaskMergeService.StatusMerged, result.MergeStatus); + Assert.Equal("Done", result.Task.Status); + // Progress marshals via the SynchronizationContext captured at construction; give + // any queued callbacks a beat to run before asserting on `reports`. + await Task.Delay(200); + Assert.Contains(reports, r => r.Message != null && r.Message.Contains("verify gate running")); + } + finally + { + TaskMergeService.ProgressReportInterval = original; + } + } + + [Fact] + public async Task ReviewTask_ApproveWithVerifyCommand_NoProgressToken_DoesNotThrow() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var (task, list, wt) = await SeedWorktreeAsync(TaskStatus.WaitingForReview); + File.WriteAllText(Path.Combine(wt.WorktreePath, "feature.txt"), "x\n"); + 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 fakeVerify = new FakeVerifyCommandRunner { Result = new VerifyCommandResult(0, false, "ok") }; + var sut = BuildSut(CreateQueue(), fakeVerify); + + var result = await sut.ReviewTask(task.Id, "approve", null, null, cancellationToken: CancellationToken.None); + + Assert.Equal(TaskMergeService.StatusMerged, result.MergeStatus); + Assert.Equal("Done", result.Task.Status); + } + [Fact] public async Task ReviewTask_Approve_ParentWithChildren_RunsUnitMerge() { diff --git a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs index c711dc22..e1c4ce3e 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/TaskMergeServiceTests.cs @@ -1675,13 +1675,19 @@ internal sealed class FakeVerifyCommandRunner : IVerifyCommandRunner public string? FileToCheck { get; set; } public bool? WorkingDirHadFile { get; private set; } - public Task RunAsync(string workingDir, string command, TimeSpan timeout, CancellationToken ct) + // Simulates a slow build/test run so a progress-reporting test can observe at least one + // report without the real verify command ever taking that long. + public TimeSpan? Delay { get; set; } + + public async 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); + if (Delay is { } delay) + await Task.Delay(delay, ct); + return Result; } }