diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 9d40c302..ff9a40be 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -228,4 +228,4 @@ list-only — there is no task-level override — and is written via `set_list_c - The worker runs standalone — start it separately from the UI. Loopback only (127.0.0.1). - `--permission-mode auto` by default; legacy `bypassPermissions` settings map to `auto` at dispatch time. `acceptEdits`, `plan`, `default` pass through unchanged. - Worktree branches follow `claudedo/{id}`. -- **An MCP tool that can run longer than ~5s reports progress.** Staying silent lets the MCP client abort after 300s idle while the worker keeps working — the caller sees an abort even though the operation is still running. +- **An MCP tool that can run longer than ~5s reports progress.** Staying silent lets the MCP client abort after 300s idle while the worker keeps working — the caller sees an abort even though the operation is still running. `Lifecycle/ProgressReporter` is the one implementation (elapsed-time reporting via `RunAsync`, per-item `i/n` via `ReportItem`) — thread an `IProgress? progress = null` parameter through instead of writing another polling loop. diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index cdcb7a20..a8c9ecad 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -703,7 +703,7 @@ public sealed class ExternalMcpService // externallyDriven: true — this call came from an MCP session, not the UI's // Approve button. A unit-merge conflict must not auto-open the in-app resolver; // the driving session resolves it via continue_merge/abort_merge instead. - await _planningMerge.StartAsync(taskId, targetBranch ?? "", cancellationToken, externallyDriven: true); + await _planningMerge.StartAsync(taskId, targetBranch ?? "", cancellationToken, externallyDriven: true, progress); var parentDone = (await _tasks.GetByIdAsync(taskId, cancellationToken))!.Status == TaskStatus.Done; mergeStatus = parentDone ? TaskMergeService.StatusMerged : TaskMergeService.StatusConflict; if (!parentDone) @@ -1037,8 +1037,12 @@ public sealed class ExternalMcpService "Finish an in-progress conflicted merge once you have resolved the conflict markers in the working tree " + "(repoPath from merge_task/review_task). Pass the PARENT task id to continue a parent/children unit merge. " + "merged=false with conflicts listed means markers are still present — resolve them and call again. " + - "Throws if there is no in-progress merge for the task; use abort_merge to cancel a paused merge instead.")] - public async Task ContinueMerge(string taskId, CancellationToken cancellationToken) + "Throws if there is no in-progress merge for the task; use abort_merge to cancel a paused merge instead." + + McpToolDocs.ProgressHint)] + public async Task ContinueMerge( + string taskId, + CancellationToken cancellationToken = default, + IProgress? progress = null) { taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); var task = await _tasks.GetByIdAsync(taskId, cancellationToken) @@ -1053,7 +1057,7 @@ public sealed class ExternalMcpService if (_planningMerge.HasActiveMerge(taskId)) { - await _planningMerge.ContinueAsync(taskId, cancellationToken); + await _planningMerge.ContinueAsync(taskId, cancellationToken, progress); var parent = (await _tasks.GetByIdAsync(taskId, cancellationToken))!; if (parent.Status == TaskStatus.Done) { @@ -1080,7 +1084,7 @@ public sealed class ExternalMcpService } else { - var r = await _merge.ContinueMergeAsync(taskId, cancellationToken); + var r = await _merge.ContinueMergeAsync(taskId, cancellationToken, progress); if (r.Status == TaskMergeService.StatusMerged) { merged = true; @@ -1464,20 +1468,29 @@ public sealed class ExternalMcpService [McpServerTool, Description( "Survey every worktree ClaudeDo tracks — use it to find leftovers to clean up. Only worktrees recorded in " + - "the ClaudeDo database appear here, and headCommit is empty when the path is missing from disk.")] - public async Task> ListWorktrees(CancellationToken cancellationToken) + "the ClaudeDo database appear here, and headCommit is empty when the path is missing from disk." + + McpToolDocs.ProgressHint)] + public async Task> ListWorktrees( + CancellationToken cancellationToken = default, + IProgress? progress = null) { var rows = await _maintenance.GetOverviewAsync(null, cancellationToken); - var results = await Task.WhenAll(rows.Select(async row => - { - var isDirty = row.PathExistsOnDisk && await TryGetIsDirtyAsync(row.Path, cancellationToken); - var headCommit = row.PathExistsOnDisk - ? (await TryRunGitAsync(row.Path, ["rev-parse", "HEAD"], cancellationToken) ?? "") - : ""; - return new WorktreeListItemDto( - row.TaskId, row.TaskNumber, row.Path, row.BranchName, headCommit, - isDirty, row.State == WorktreeState.Merged); - })); + // One git status + one rev-parse per row, run concurrently -- with many tracked worktrees + // (this tool's whole point is surfacing leftovers nobody cleaned up) that can still take a + // while, so this reports on the same elapsed-time cadence as a single long git call rather + // than per-row (rows finish out of order under Task.WhenAll, so there's no natural i/n). + var results = await ProgressReporter.RunAsync( + Task.WhenAll(rows.Select(async row => + { + var isDirty = row.PathExistsOnDisk && await TryGetIsDirtyAsync(row.Path, cancellationToken); + var headCommit = row.PathExistsOnDisk + ? (await TryRunGitAsync(row.Path, ["rev-parse", "HEAD"], cancellationToken) ?? "") + : ""; + return new WorktreeListItemDto( + row.TaskId, row.TaskNumber, row.Path, row.BranchName, headCommit, + isDirty, row.State == WorktreeState.Merged); + })), + ProgressReportInterval, progress, "surveying worktrees"); return results; } diff --git a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs index 1c273d5b..e43630b4 100644 --- a/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs +++ b/src/ClaudeDo.Worker/Lifecycle/TaskMergeService.cs @@ -492,7 +492,8 @@ public sealed class TaskMergeService CancellationToken ct) => MergeAsync(taskId, targetBranch, removeWorktree, commitMessage, leaveConflictsInTree: false, ct); - public async Task ContinueMergeAsync(string taskId, CancellationToken ct) + public async Task ContinueMergeAsync( + string taskId, CancellationToken ct, IProgress? progress = null) { var (task, list, wt, verifyCommand) = await LoadMergeContextAsync(taskId, ct); @@ -558,7 +559,7 @@ public sealed class TaskMergeService var targetBranch = await _git.GetCurrentBranchAsync(list.WorkingDir, 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 continuing merge of task {TaskId}: {Reason}", taskId, verifyFailure.ErrorMessage); diff --git a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs index fd0963ab..a58bb24c 100644 --- a/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs +++ b/src/ClaudeDo.Worker/Planning/PlanningMergeOrchestrator.cs @@ -6,6 +6,7 @@ using ClaudeDo.Worker.Hub; using ClaudeDo.Worker.Lifecycle; using ClaudeDo.Worker.State; using Microsoft.EntityFrameworkCore; +using ModelContextProtocol; using TaskStatus = ClaudeDo.Data.Models.TaskStatus; namespace ClaudeDo.Worker.Planning; @@ -57,7 +58,8 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState } public async Task StartAsync( - string parentTaskId, string targetBranch, CancellationToken ct, bool externallyDriven = false) + string parentTaskId, string targetBranch, CancellationToken ct, bool externallyDriven = false, + IProgress? progress = null) { string workingDir; List children; @@ -121,7 +123,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState throw new InvalidOperationException($"Merge already in progress for {parentTaskId}."); await _broadcaster.PlanningMergeStarted(parentTaskId, targetBranch); - await DrainAsync(parentTaskId, ct); + await DrainAsync(parentTaskId, ct, progress); } /// True when a unit merge for this parent is paused on a conflict (in-memory state). @@ -145,14 +147,15 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState return result; } - public async Task ContinueAsync(string planningTaskId, CancellationToken ct) + public async Task ContinueAsync( + string planningTaskId, CancellationToken ct, IProgress? progress = null) { if (!_states.TryGetValue(planningTaskId, out var state) || state.CurrentSubtaskId is null) throw new InvalidOperationException( "no in-progress merge to continue; if the worker was restarted during a conflict, use AbortPlanningMerge to reset the repository"); var current = state.CurrentSubtaskId; - var result = await _merge.ContinueMergeAsync(current, ct); + var result = await _merge.ContinueMergeAsync(current, ct, progress); if (result.Status == TaskMergeService.StatusConflict) { @@ -173,7 +176,7 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState await _broadcaster.PlanningSubtaskMerged(planningTaskId, current); state.CurrentSubtaskId = null; - await DrainAsync(planningTaskId, ct); + await DrainAsync(planningTaskId, ct, progress); } public async Task AbortAsync(string planningTaskId, CancellationToken ct) @@ -213,7 +216,8 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState // Parent remains WaitingForReview — Approve will restart the unit merge from scratch. } - private async Task DrainAsync(string planningTaskId, CancellationToken ct) + private async Task DrainAsync( + string planningTaskId, CancellationToken ct, IProgress? progress = null) { if (!_states.TryGetValue(planningTaskId, out var state)) return; @@ -229,7 +233,8 @@ public sealed class PlanningMergeOrchestrator : IActiveMergeState removeWorktree: true, commitMessage: "", // blank -> TaskMergeService builds the conventional default leaveConflictsInTree: true, - ct); + ct, + progress); if (result.Status == TaskMergeService.StatusConflict) { diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 320b416b..19978591 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -694,6 +694,76 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Equal(WorktreeState.Merged, verify.Worktrees.Single(w => w.TaskId == childId).State); } + // Regression coverage for the parent/children unit-merge path specifically: PlanningMergeOrchestrator + // used to drain every child's merge (and verify gate) without ever forwarding review_task's own + // progress token, so a multi-child approve with a slow verify command reported only the single + // "review_task started" tick and then went silent for as long as every child's gate took combined. + [Fact] + public async Task ReviewTask_Approve_ParentWithChildren_SlowVerifyCommand_ReportsProgress() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = TaskMergeService.ProgressReportInterval; + TaskMergeService.ProgressReportInterval = TimeSpan.FromMilliseconds(50); + try + { + var repo = new GitRepoFixture(); + _repos.Add(repo); + GitRepoFixture.RunGit(repo.RepoDir, "branch", "-m", "main"); + + var listId = Guid.NewGuid().ToString(); + var parentId = Guid.NewGuid().ToString(); + var childId = Guid.NewGuid().ToString(); + using (var ctx = _db.CreateContext()) + { + ctx.Lists.Add(new ListEntity { Id = listId, Name = "L", WorkingDir = repo.RepoDir, CreatedAt = DateTime.UtcNow }); + ctx.Tasks.Add(new TaskEntity { Id = parentId, ListId = listId, Title = "plan", CreatedAt = DateTime.UtcNow, + Status = TaskStatus.WaitingForReview, PlanningPhase = PlanningPhase.Finalized, Number = ++_numberSeed }); + ctx.Tasks.Add(new TaskEntity { Id = childId, ListId = listId, Title = "child", CreatedAt = DateTime.UtcNow, + ParentTaskId = parentId, Status = TaskStatus.Done, SortOrder = 1, Number = ++_numberSeed }); + + var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}"); + _worktreeCleanups.Add((repo.RepoDir, wtPath)); + var branch = $"claudedo/{childId[..8]}"; + GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", branch, wtPath, repo.BaseCommit); + File.WriteAllText(Path.Combine(wtPath, "child.txt"), "c\n"); + GitRepoFixture.RunGit(wtPath, "add", "child.txt"); + GitRepoFixture.RunGit(wtPath, "commit", "-m", "add child.txt"); + ctx.Worktrees.Add(new WorktreeEntity + { + TaskId = childId, Path = wtPath, BranchName = branch, + BaseCommit = repo.BaseCommit, + HeadCommit = GitRepoFixture.RunGit(wtPath, "rev-parse", "HEAD").Trim(), + State = WorktreeState.Active, CreatedAt = DateTime.UtcNow, + }); + await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity { ListId = listId, VerifyCommand = "dotnet build" }); + await ctx.SaveChangesAsync(); + } + + 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( + parentId, "approve", null, "main", cancellationToken: CancellationToken.None, progress: progress); + + Assert.Equal(TaskMergeService.StatusMerged, result.MergeStatus); + Assert.Equal("Done", result.Task.Status); + 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_Approve_ParentWithChildren_ReportsEmptyChildByName() { @@ -1316,6 +1386,33 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Contains(rows, r => r.TaskId == task.Id); } + // list_worktrees fans out one git status + one rev-parse per tracked worktree under + // Task.WhenAll -- no natural i/n since rows finish out of order, so it reports on the same + // elapsed-time cadence as CleanupTaskWorktree above. + [Fact] + public async Task ListWorktrees_ReportsProgressWhileSurveying() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = ExternalMcpService.ProgressReportInterval; + ExternalMcpService.ProgressReportInterval = TimeSpan.Zero; + try + { + await SeedWorktreeAsync(); + var sut = BuildSut(CreateQueue()); + var progress = new SyncProgress(); + + var rows = await sut.ListWorktrees(CancellationToken.None, progress); + + Assert.NotEmpty(rows); + Assert.NotEmpty(progress.Reports); + } + finally + { + ExternalMcpService.ProgressReportInterval = original; + } + } + // ── CleanupTaskWorktree ──────────────────────────────────────────────────── [Fact] @@ -1711,6 +1808,52 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.False(await new GitService().IsMidMergeAsync(list.WorkingDir!)); } + // Regression coverage for the same class of incident as ReviewTask_ApproveWithSlowVerifyCommand + // above, but for the resume path: continue_merge re-runs the verify gate too, and until this + // fix it never forwarded its progress token into RunVerifyGateAsync at all. + [Fact] + public async Task ContinueMerge_SlowVerifyCommand_ReportsProgressWellBeforeIdleTimeout() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = TaskMergeService.ProgressReportInterval; + TaskMergeService.ProgressReportInterval = TimeSpan.FromMilliseconds(50); + try + { + var (task, list, _) = await SeedConflictingWorktreeAsync(TaskStatus.WaitingForReview); + var plainSut = BuildSut(CreateQueue()); + await plainSut.ReviewTask(task.Id, "approve", null, null, + leaveConflictsInTree: true, CancellationToken.None); + + File.WriteAllText(Path.Combine(list.WorkingDir!, "README.md"), "# resolved\n"); + GitRepoFixture.RunGit(list.WorkingDir!, "add", "README.md"); + + 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.ContinueMerge(task.Id, CancellationToken.None, progress); + + Assert.True(result.Merged); + Assert.Equal("Done", result.TaskStatus); + 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 ContinueMerge_MarkersStillPresent_ReportsConflicts() {