From aeb1a5eb82f973830d59fa746f58a2dcd3b0b3c2 Mon Sep 17 00:00:00 2001 From: Mika Kuns Date: Mon, 17 Aug 2026 09:29:51 +0200 Subject: [PATCH] feat(worker): report progress for cleanup_task_worktree, get_task_diff, preview_merge_set Wraps the single-element git/worktree long-runners in ProgressReporter.RunAsync (time-based, ExternalMcpService.ProgressReportInterval) and adds i/n reporting to preview_merge_set's per-task loop, so these MCP calls survive Claude Code's ~300s idle-silence abort instead of leaving the caller with no signal that work is still happening. --- .../External/ExternalMcpService.cs | 33 +++++-- .../External/ExternalMcpServiceTests.cs | 91 +++++++++++++++++++ 2 files changed, 116 insertions(+), 8 deletions(-) diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 0e06797c..cdcb7a20 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -184,6 +184,12 @@ public sealed record DailyPrepDataDto( [McpServerToolType] public sealed class ExternalMcpService { + // Mirrors TaskMergeService.ProgressReportInterval / TaskWaitMcpTools.ProgressReportInterval: + // a separate field (not shared) so shrinking one for a test can't race another's tests. Used + // by the single-element long-runners here (GetTaskDiff, CleanupTaskWorktree) that wrap a bare + // GitService/WorktreeMaintenanceService call with no natural i/n to report instead. + internal static TimeSpan ProgressReportInterval = TimeSpan.FromSeconds(30); + private readonly TaskRepository _tasks; private readonly ListRepository _lists; private readonly QueueService _queue; @@ -897,7 +903,8 @@ public sealed class ExternalMcpService "conflict report already narrowed down which files matter. Omit/empty for every changed file " + "— the default and the only prior behavior. Works in both stat and full-diff mode.")] IReadOnlyList? paths = null, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); var (repoPath, baseCommit, headCommit) = await LoadDiffRangeAsync(taskId, cancellationToken); @@ -906,13 +913,17 @@ public sealed class ExternalMcpService if (stat) { - var diffStat = await _git.DiffStatAsync(repoPath, baseCommit, headCommit ?? "HEAD", paths, cancellationToken); + var diffStat = await ProgressReporter.RunAsync( + _git.DiffStatAsync(repoPath, baseCommit, headCommit ?? "HEAD", paths, cancellationToken), + ProgressReportInterval, progress, "computing diff stat"); return new TaskDiffDto(diffStat, ParseDiffStatFileNames(diffStat), false, diffStat.Length); } var diff = headCommit is null - ? await _git.GetBranchDiffAsync(repoPath, baseCommit, paths, cancellationToken) - : await _git.GetCommitRangeDiffAsync(repoPath, baseCommit, headCommit, paths, cancellationToken); + ? await ProgressReporter.RunAsync( + _git.GetBranchDiffAsync(repoPath, baseCommit, paths, cancellationToken), ProgressReportInterval, progress, "computing diff") + : await ProgressReporter.RunAsync( + _git.GetCommitRangeDiffAsync(repoPath, baseCommit, headCommit, paths, cancellationToken), ProgressReportInterval, progress, "computing diff"); var files = ParseDiffFileNames(diff); if (diff.Length <= maxBytes) @@ -1283,8 +1294,9 @@ public sealed class ExternalMcpService var filesByTask = new Dictionary>(); var numbersByTask = new Dictionary(); - foreach (var taskId in taskIds) + for (var i = 0; i < taskIds.Count; i++) { + var taskId = taskIds[i]; try { var (preview, behind, changedFiles, isEmpty, staleFiles, number) = await PreviewMergeCoreAsync(taskId, targetBranch, runVerify, cancellationToken, progress); @@ -1301,6 +1313,8 @@ public sealed class ExternalMcpService taskId, TaskMergeService.PreviewUnavailable, Array.Empty(), 0, 0, ex.Message, Number: task?.Number)); } + + ProgressReporter.ReportItem(progress, "Previewing merges", i + 1, taskIds.Count); } var overlaps = filesByTask @@ -1475,7 +1489,8 @@ public sealed class ExternalMcpService [Description("false (default): refuse a worktree with uncommitted changes. true: remove it anyway, losing " + "those changes.")] bool force = false, - CancellationToken cancellationToken = default) + CancellationToken cancellationToken = default, + IProgress? progress = null) { taskId = await TaskIdResolver.ResolveAsync(_tasks, taskId, cancellationToken); @@ -1490,14 +1505,16 @@ public sealed class ExternalMcpService if (!force && Directory.Exists(wt.Path)) { - var isDirty = await _git.HasChangesAsync(wt.Path, cancellationToken); + var isDirty = await ProgressReporter.RunAsync( + _git.HasChangesAsync(wt.Path, cancellationToken), ProgressReportInterval, progress, "checking worktree for changes"); if (isDirty) throw new InvalidOperationException( "Worktree has uncommitted changes. Use force=true to remove anyway (changes will be lost)."); } var path = wt.Path; - var result = await _maintenance.ForceRemoveAsync(taskId, cancellationToken); + var result = await ProgressReporter.RunAsync( + _maintenance.ForceRemoveAsync(taskId, cancellationToken), ProgressReportInterval, progress, "removing worktree"); return new CleanupWorktreeResult(result.Removed, path, result.BranchDeleted, task.Number); } diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 447210a0..320b416b 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -52,6 +52,14 @@ internal sealed class ExternalFakeHubContext : IHubContext public IGroupManager Groups => throw new NotImplementedException(); } +// A synchronously-collecting IProgress: Progress marshals through the SynchronizationContext +// captured at construction, which is unreliable to assert on immediately in a test. +file sealed class SyncProgress : IProgress +{ + public readonly List Reports = new(); + public void Report(T value) => Reports.Add(value); +} + public sealed class ExternalMcpServiceTests : IDisposable { private readonly DbFixture _db = new(); @@ -1220,6 +1228,41 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Contains("handled.txt", diff.Content); } + // get_task_diff has no natural i/n (it's one git process, not a loop over items), so it + // reports on a timer like merge_task's verify gate. Shrinking the interval to zero forces a + // report on every poll of the still-running git process instead of waiting out a real + // interval -- Task.Delay(TimeSpan.Zero) resolves synchronously, so the diff process (a real + // subprocess, never already complete when polled) always loses the race at least once. + [Fact] + public async Task GetTaskDiff_ReportsProgressWhileDiffing() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = ExternalMcpService.ProgressReportInterval; + ExternalMcpService.ProgressReportInterval = TimeSpan.Zero; + try + { + var (task, list, wt) = await SeedWorktreeAsync(); + 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); + + var sut = BuildSut(CreateQueue()); + var progress = new SyncProgress(); + + var diff = await sut.GetTaskDiff(task.Id, false, null, CancellationToken.None, progress); + + Assert.Contains("added.txt", diff.Files); + Assert.NotEmpty(progress.Reports); + Assert.All(progress.Reports, r => Assert.Contains("computing diff", r.Message)); + } + finally + { + ExternalMcpService.ProgressReportInterval = original; + } + } + // ── MergeTask ────────────────────────────────────────────────────────────── [Fact] @@ -1311,6 +1354,33 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.False(Directory.Exists(wt.WorktreePath)); } + // cleanup_task_worktree has no natural i/n either (dirty check + one removal, not a loop), so + // it reports on the same timer/zero-interval technique as GetTaskDiff above. + [Fact] + public async Task CleanupTaskWorktree_ReportsProgressWhileRemoving() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var original = ExternalMcpService.ProgressReportInterval; + ExternalMcpService.ProgressReportInterval = TimeSpan.Zero; + try + { + var (task, _, wt) = await SeedWorktreeAsync(TaskStatus.Done); + var sut = BuildSut(CreateQueue()); + var progress = new SyncProgress(); + + var result = await sut.CleanupTaskWorktree(task.Id, false, CancellationToken.None, progress); + + Assert.True(result.Removed); + Assert.False(Directory.Exists(wt.WorktreePath)); + Assert.NotEmpty(progress.Reports); + } + finally + { + ExternalMcpService.ProgressReportInterval = original; + } + } + // ── GetTaskConfig ───────────────────────────────────────────────────────── private ConfigMcpTools BuildConfigSut() => new(_lists, _tasks, _broadcaster, _db.CreateFactory()); @@ -2350,6 +2420,27 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Empty(result.Overlaps); } + [Fact] + public async Task PreviewMergeSet_ReportsProgressPerTask() + { + var listId = await SeedListAsync(); + var t1 = await SeedTaskAsync(listId, "a"); + var t2 = await SeedTaskAsync(listId, "b"); + var sut = BuildSut(CreateQueue()); + var progress = new SyncProgress(); + + // Neither task has a worktree -- each entry fails individually (PreviewUnavailable), but + // the loop still reports progress per task since a per-task InvalidOperationException + // doesn't abort the batch. + await sut.PreviewMergeSet( + [t1.Id, t2.Id], "main", cancellationToken: CancellationToken.None, progress: progress); + + Assert.Equal(2, progress.Reports.Count); + Assert.Equal("Previewing merges (1/2)", progress.Reports[0].Message); + Assert.Equal("Previewing merges (2/2)", progress.Reports[1].Message); + Assert.Equal(2, progress.Reports[1].Total); + } + [Fact] public async Task PreviewMergeSet_OneTaskWithoutWorktree_ReportsErrorWithoutAbortingOthers() {