From c715c96af9d8a9c3d788e09cc8979b8e8daf1cca Mon Sep 17 00:00:00 2001 From: mika kuns Date: Mon, 10 Aug 2026 13:39:55 +0200 Subject: [PATCH] feat(mcp): paths filter for get_task_diff, subset hint for preview_merge_set get_task_diff gained an optional paths param (git pathspec, both stat and full-diff modes) so a large multi-file task can be narrowed to the files actually in question instead of shipping the whole diff. preview_merge_set now also reports Subsets: pairs where one task's changed files are a proper subset of another's in the same set, the strongest available post-hoc signal that a task may be redundant with another. --- src/ClaudeDo.Data/Git/GitService.cs | 34 +++++-- .../ViewModels/Modals/DiffViewerViewModel.cs | 4 +- .../External/ExternalMcpService.cs | 68 +++++++++++--- .../Planning/PlanningAggregator.cs | 2 +- src/ClaudeDo.Worker/Runner/WorktreeManager.cs | 2 +- .../External/ExternalMcpServiceTests.cs | 94 ++++++++++++++++++- 6 files changed, 174 insertions(+), 30 deletions(-) diff --git a/src/ClaudeDo.Data/Git/GitService.cs b/src/ClaudeDo.Data/Git/GitService.cs index 651796b4..8678ef7f 100644 --- a/src/ClaudeDo.Data/Git/GitService.cs +++ b/src/ClaudeDo.Data/Git/GitService.cs @@ -143,10 +143,12 @@ public sealed class GitService /// (committed-on-branch changes + uncommitted work). Used for viewing a Claude /// task's total impact relative to where the branch started. /// - public async Task GetBranchDiffAsync(string worktreePath, string baseRef, CancellationToken ct = default) + public async Task GetBranchDiffAsync( + string worktreePath, string baseRef, IReadOnlyList? paths = null, CancellationToken ct = default) { - var (exitCode, stdout, _) = await RunGitAsync(worktreePath, - ["diff", baseRef], ct); + var args = new List { "diff", baseRef }; + AppendPathFilter(args, paths); + var (exitCode, stdout, _) = await RunGitAsync(worktreePath, args, ct); if (exitCode == 0 && !string.IsNullOrWhiteSpace(stdout)) return stdout; // Fallback: whatever the worktree has vs HEAD (uncommitted only). @@ -158,24 +160,38 @@ public sealed class GitService /// task's changes after its worktree has been merged away (the commits survive on /// the target branch even though the worktree directory and branch ref are gone). /// - public async Task GetCommitRangeDiffAsync(string repoDir, string baseCommit, string headCommit, CancellationToken ct = default) + public async Task GetCommitRangeDiffAsync( + string repoDir, string baseCommit, string headCommit, IReadOnlyList? paths = null, CancellationToken ct = default) { - var (exitCode, stdout, stderr) = await RunGitAsync(repoDir, - ["diff", $"{baseCommit}..{headCommit}"], ct); + var args = new List { "diff", $"{baseCommit}..{headCommit}" }; + AppendPathFilter(args, paths); + var (exitCode, stdout, stderr) = await RunGitAsync(repoDir, args, ct); if (exitCode != 0) throw new InvalidOperationException($"git diff {baseCommit}..{headCommit} failed (exit {exitCode}): {stderr}"); return stdout; } - public async Task DiffStatAsync(string worktreePath, string baseCommit, string headCommit, CancellationToken ct = default) + public async Task DiffStatAsync( + string worktreePath, string baseCommit, string headCommit, IReadOnlyList? paths = null, CancellationToken ct = default) { - var (exitCode, stdout, stderr) = await RunGitAsync(worktreePath, - ["diff", "--stat", $"{baseCommit}..{headCommit}"], ct); + var args = new List { "diff", "--stat", $"{baseCommit}..{headCommit}" }; + AppendPathFilter(args, paths); + var (exitCode, stdout, stderr) = await RunGitAsync(worktreePath, args, ct); if (exitCode != 0) throw new InvalidOperationException($"git diff --stat failed (exit {exitCode}): {stderr}"); return stdout.Trim(); } + // Appends a `-- ` pathspec filter so git itself narrows the diff instead of the + // caller filtering the result after the fact (works identically for --stat and full diffs). + // No-op when paths is null/empty so existing callers see no behavior change. + private static void AppendPathFilter(List args, IReadOnlyList? paths) + { + if (paths is not { Count: > 0 }) return; + args.Add("--"); + args.AddRange(paths); + } + public async Task GetFileDiffAsync(string worktreePath, string? baseCommit, string relativePath, CancellationToken ct = default) { string[] args = string.IsNullOrEmpty(baseCommit) diff --git a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs index 070fd8a7..3ef7285b 100644 --- a/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Modals/DiffViewerViewModel.cs @@ -162,9 +162,9 @@ public sealed partial class DiffViewerViewModel : ViewModelBase try { raw = FromCommitRange && BaseRef is not null && HeadCommit is not null - ? await _git.GetCommitRangeDiffAsync(WorktreePath, BaseRef, HeadCommit, ct) + ? await _git.GetCommitRangeDiffAsync(WorktreePath, BaseRef, HeadCommit, ct: ct) : BaseRef is not null - ? await _git.GetBranchDiffAsync(WorktreePath, BaseRef, ct) + ? await _git.GetBranchDiffAsync(WorktreePath, BaseRef, ct: ct) : await _git.GetDiffAsync(WorktreePath, ct); } catch (Exception ex) diff --git a/src/ClaudeDo.Worker/External/ExternalMcpService.cs b/src/ClaudeDo.Worker/External/ExternalMcpService.cs index 23fc6319..af4847f1 100644 --- a/src/ClaudeDo.Worker/External/ExternalMcpService.cs +++ b/src/ClaudeDo.Worker/External/ExternalMcpService.cs @@ -100,8 +100,15 @@ public sealed record MergePreviewSetEntryDto( public sealed record FileOverlapDto(string File, IReadOnlyList TaskIds); +// TaskId's changed-file list is a non-empty PROPER subset of SupersetTaskId's -- e.g. one task +// deletes two files and another deletes just one of those same two. Stronger than an Overlaps +// entry (which only says some files are shared): a real subset is the best available machine +// signal that TaskId's work may already be entirely covered by SupersetTaskId. +public sealed record SubsetRelationDto(string TaskId, string SupersetTaskId); + public sealed record MergePreviewSetResultDto( - IReadOnlyList Tasks, IReadOnlyList Overlaps); + IReadOnlyList Tasks, IReadOnlyList Overlaps, + IReadOnlyList Subsets); public sealed record WorktreeListItemDto( string? TaskId, string Path, string Branch, @@ -620,7 +627,7 @@ public sealed class ExternalMcpService { if (!Directory.Exists(child.Worktree.Path)) return false; var files = ParseDiffStatFileNames( - await _git.DiffStatAsync(child.Worktree.Path, child.Worktree.BaseCommit, "HEAD", ct)); + await _git.DiffStatAsync(child.Worktree.Path, child.Worktree.BaseCommit, "HEAD", ct: ct)); return files.Count == 0; } if (child.HandlerBaseCommit is { Length: > 0 } handlerBase && child.HandlerHeadCommit is { Length: > 0 } handlerHead) @@ -714,6 +721,10 @@ public sealed class ExternalMcpService [Description("false (default): the full unified diff, capped at 200 KB. true: a --stat summary with " + "per-file insertion/deletion counts — start here when the diff may be large.")] bool stat = false, + [Description("Restrict the diff to these paths (relative to the repo root), e.g. after a --stat pass or a " + + "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) { var (repoPath, baseCommit, headCommit) = await LoadDiffRangeAsync(taskId, cancellationToken); @@ -722,13 +733,13 @@ public sealed class ExternalMcpService if (stat) { - var diffStat = await _git.DiffStatAsync(repoPath, baseCommit, headCommit ?? "HEAD", cancellationToken); + var diffStat = await _git.DiffStatAsync(repoPath, baseCommit, headCommit ?? "HEAD", paths, cancellationToken); return new TaskDiffDto(diffStat, ParseDiffStatFileNames(diffStat), false, diffStat.Length); } var diff = headCommit is null - ? await _git.GetBranchDiffAsync(repoPath, baseCommit, cancellationToken) - : await _git.GetCommitRangeDiffAsync(repoPath, baseCommit, headCommit, cancellationToken); + ? await _git.GetBranchDiffAsync(repoPath, baseCommit, paths, cancellationToken) + : await _git.GetCommitRangeDiffAsync(repoPath, baseCommit, headCommit, paths, cancellationToken); var files = ParseDiffFileNames(diff); if (diff.Length <= maxBytes) @@ -949,11 +960,13 @@ public sealed class ExternalMcpService [McpServerTool, Description( "Plan a batch merge: preview_merge for several tasks against the same targetBranch, plus a file-overlap " + "check between them. Per entry you get preview_merge's fields, or error instead when that task could not " + - "be previewed (it is then left out of the overlap computation). overlaps names, for each file touched by " + - "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.")] + "be previewed (it is then left out of the overlap/subset computation). overlaps names, for each file " + + "touched by MORE THAN ONE of the given tasks, which tasks touch it — a single taskId always yields no " + + "overlaps. subsets flags a stronger relation: TaskId's changed files are a PROPER subset of " + + "SupersetTaskId's — the strongest hint you get post-hoc that TaskId may be redundant with SupersetTaskId, " + + "worth checking before merging both. IMPORTANT: neither overlap nor subset is a safety guarantee — 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.")] public async Task PreviewMergeSet( IReadOnlyList taskIds, [Description("Branch to preview every task against; defaults to the repo's current branch.")] @@ -990,7 +1003,36 @@ public sealed class ExternalMcpService .OrderBy(o => o.File, StringComparer.OrdinalIgnoreCase) .ToList(); - return new MergePreviewSetResultDto(entries, overlaps); + var subsets = FindSubsetRelations(filesByTask); + + return new MergePreviewSetResultDto(entries, overlaps, subsets); + } + + // A task's changed-file set is flagged only against another task's STRICTLY LARGER set that + // contains every one of its files -- an empty set (isEmpty task) never qualifies, since that + // case is already surfaced via IsEmpty and would otherwise match every other task trivially. + private static List FindSubsetRelations(Dictionary> filesByTask) + { + var fileSets = filesByTask.ToDictionary( + kv => kv.Key, + kv => new HashSet(kv.Value, StringComparer.OrdinalIgnoreCase)); + + var subsets = new List(); + foreach (var (taskId, fileSet) in fileSets) + { + if (fileSet.Count == 0) continue; + foreach (var (otherTaskId, otherFileSet) in fileSets) + { + if (otherTaskId == taskId || otherFileSet.Count <= fileSet.Count) continue; + if (fileSet.IsSubsetOf(otherFileSet)) + subsets.Add(new SubsetRelationDto(taskId, otherTaskId)); + } + } + + return subsets + .OrderBy(s => s.TaskId, StringComparer.OrdinalIgnoreCase) + .ThenBy(s => s.SupersetTaskId, StringComparer.OrdinalIgnoreCase) + .ToList(); } // Shared core for PreviewMerge/PreviewMergeSet: throws a clear InvalidOperationException instead of @@ -1029,7 +1071,7 @@ public sealed class ExternalMcpService var behind = await GitRevListCountAsync(list.WorkingDir, $"{wt.BranchName}..{target}", ct); var changedFiles = Directory.Exists(wt.Path) - ? ParseDiffStatFileNames(await _git.DiffStatAsync(wt.Path, wt.BaseCommit, "HEAD", ct)) + ? ParseDiffStatFileNames(await _git.DiffStatAsync(wt.Path, wt.BaseCommit, "HEAD", ct: ct)) : Array.Empty(); return (preview, behind, changedFiles, changedFiles.Count == 0); @@ -1043,7 +1085,7 @@ public sealed class ExternalMcpService var isEmpty = string.Equals(handlerBase, handlerHead, StringComparison.Ordinal); var changedFiles = isEmpty ? Array.Empty() - : ParseDiffStatFileNames(await _git.DiffStatAsync(list.WorkingDir, handlerBase, handlerHead, ct)); + : ParseDiffStatFileNames(await _git.DiffStatAsync(list.WorkingDir, handlerBase, handlerHead, ct: ct)); var preview = new MergePreviewResult(TaskMergeService.PreviewClean, Array.Empty(), changedFiles.Count); return (preview, 0, changedFiles, isEmpty); diff --git a/src/ClaudeDo.Worker/Planning/PlanningAggregator.cs b/src/ClaudeDo.Worker/Planning/PlanningAggregator.cs index 44f9f506..73c90b72 100644 --- a/src/ClaudeDo.Worker/Planning/PlanningAggregator.cs +++ b/src/ClaudeDo.Worker/Planning/PlanningAggregator.cs @@ -65,7 +65,7 @@ public sealed class PlanningAggregator string unified; try { - unified = await _git.GetBranchDiffAsync(wt.Path, wt.BaseCommit, ct); + unified = await _git.GetBranchDiffAsync(wt.Path, wt.BaseCommit, ct: ct); } catch (Exception ex) { diff --git a/src/ClaudeDo.Worker/Runner/WorktreeManager.cs b/src/ClaudeDo.Worker/Runner/WorktreeManager.cs index 1836887b..98d795bc 100644 --- a/src/ClaudeDo.Worker/Runner/WorktreeManager.cs +++ b/src/ClaudeDo.Worker/Runner/WorktreeManager.cs @@ -147,7 +147,7 @@ public sealed class WorktreeManager await _git.CommitAsync(ctx.WorktreePath, message, ct); var head = await _git.RevParseHeadAsync(ctx.WorktreePath, ct); - var diffStat = await _git.DiffStatAsync(ctx.WorktreePath, ctx.BaseCommit, head, ct); + var diffStat = await _git.DiffStatAsync(ctx.WorktreePath, ctx.BaseCommit, head, ct: ct); using var context = _dbFactory.CreateDbContext(); var wtRepo = new WorktreeRepository(context); diff --git a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs index 4f7ef325..eae29a1b 100644 --- a/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/External/ExternalMcpServiceTests.cs @@ -769,7 +769,7 @@ public sealed class ExternalMcpServiceTests : IDisposable var sut = BuildSut(CreateQueue()); await Assert.ThrowsAsync( - () => sut.GetTaskDiff(task.Id, false, CancellationToken.None)); + () => sut.GetTaskDiff(task.Id, false, null, CancellationToken.None)); } [Fact] @@ -785,12 +785,52 @@ public sealed class ExternalMcpServiceTests : IDisposable await mgr.CommitIfChangedAsync(wt, task, list, CancellationToken.None); var sut = BuildSut(CreateQueue()); - var diff = await sut.GetTaskDiff(task.Id, false, CancellationToken.None); + var diff = await sut.GetTaskDiff(task.Id, false, null, CancellationToken.None); Assert.Contains("added.txt", diff.Files); Assert.False(diff.Truncated); } + [Fact] + public async Task GetTaskDiff_WithPathsFilter_ReturnsOnlyRequestedFile() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var (task, list, wt) = await SeedWorktreeAsync(); + File.WriteAllText(Path.Combine(wt.WorktreePath, "added.txt"), "content"); + File.WriteAllText(Path.Combine(wt.WorktreePath, "other.txt"), "other 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 diff = await sut.GetTaskDiff(task.Id, false, ["added.txt"], CancellationToken.None); + + Assert.Equal(new[] { "added.txt" }, diff.Files); + Assert.DoesNotContain("other.txt", diff.Content); + } + + [Fact] + public async Task GetTaskDiff_StatMode_WithPathsFilter_ReturnsOnlyRequestedFile() + { + if (!GitAvailable) { Assert.True(true, "git not available -- skipping"); return; } + + var (task, list, wt) = await SeedWorktreeAsync(); + File.WriteAllText(Path.Combine(wt.WorktreePath, "added.txt"), "content"); + File.WriteAllText(Path.Combine(wt.WorktreePath, "other.txt"), "other 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 diff = await sut.GetTaskDiff(task.Id, true, ["other.txt"], CancellationToken.None); + + Assert.Equal(new[] { "other.txt" }, diff.Files); + Assert.DoesNotContain("added.txt", diff.Content); + } + [Fact] public async Task GetTaskDiff_WorktreeLessHandlerTask_UsesHandlerCommitRangeOverListWorkingDir() { @@ -819,7 +859,7 @@ public sealed class ExternalMcpServiceTests : IDisposable } var sut = BuildSut(CreateQueue()); - var diff = await sut.GetTaskDiff(task.Id, false, CancellationToken.None); + var diff = await sut.GetTaskDiff(task.Id, false, null, CancellationToken.None); Assert.Contains("handled.txt", diff.Files); Assert.False(diff.Truncated); @@ -850,7 +890,7 @@ public sealed class ExternalMcpServiceTests : IDisposable } var sut = BuildSut(CreateQueue()); - var diff = await sut.GetTaskDiff(task.Id, true, CancellationToken.None); + var diff = await sut.GetTaskDiff(task.Id, true, null, CancellationToken.None); Assert.Contains("handled.txt", diff.Content); } @@ -1637,6 +1677,52 @@ public sealed class ExternalMcpServiceTests : IDisposable Assert.Equal( new[] { taskA.Id, taskB.Id }.OrderBy(x => x), overlap.TaskIds.OrderBy(x => x)); + // Equal file sets overlap but neither is a PROPER subset of the other. + Assert.Empty(result.Subsets); + } + + [Fact] + public async Task PreviewMergeSet_TaskFilesAreProperSubsetOfAnother_ReportsSubset() + { + 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 }); + var subsetTask = await SeedTaskAsync(listId, "subset", TaskStatus.WaitingForReview); + var supersetTask = await SeedTaskAsync(listId, "superset", TaskStatus.WaitingForReview); + + await CreateActiveWorktreeAsync(repo, subsetTask.Id, "shared.txt", "shared change\n"); + + var wtPath = Path.Combine(Path.GetTempPath(), $"wt_{Guid.NewGuid():N}"); + _worktreeCleanups.Add((repo.RepoDir, wtPath)); + var branch = $"claudedo/{supersetTask.Id[..8]}"; + GitRepoFixture.RunGit(repo.RepoDir, "worktree", "add", "-b", branch, wtPath, repo.BaseCommit); + File.WriteAllText(Path.Combine(wtPath, "shared.txt"), "shared change too\n"); + File.WriteAllText(Path.Combine(wtPath, "extra.txt"), "extra\n"); + GitRepoFixture.RunGit(wtPath, "add", "-A"); + GitRepoFixture.RunGit(wtPath, "commit", "-m", "edit shared+extra"); + using (var ctx = _db.CreateContext()) + { + ctx.Worktrees.Add(new WorktreeEntity + { + TaskId = supersetTask.Id, Path = wtPath, BranchName = branch, + BaseCommit = repo.BaseCommit, + HeadCommit = GitRepoFixture.RunGit(wtPath, "rev-parse", "HEAD").Trim(), + State = WorktreeState.Active, CreatedAt = DateTime.UtcNow, + }); + await ctx.SaveChangesAsync(); + } + + var sut = BuildSut(CreateQueue()); + var result = await sut.PreviewMergeSet([subsetTask.Id, supersetTask.Id], "main", CancellationToken.None); + + var relation = Assert.Single(result.Subsets); + Assert.Equal(subsetTask.Id, relation.TaskId); + Assert.Equal(supersetTask.Id, relation.SupersetTaskId); } [Fact]