fix(worker): surface empty review ranges and blocked children over MCP
preview_merge/preview_merge_set now report isEmpty (ahead==0, or HandlerBaseCommit==HandlerHeadCommit for a worktree-less list-handler task) instead of leaving an empty branch indistinguishable from a small one. preview_merge also stops throwing for worktree-less handler tasks, falling back to their fixed commit range. review_task's parent approve returns emptyChildren, naming the Done children whose review range contributed nothing before the unit merge lands. TaskRefDto/TaskDto now expose roadblockCount so a CLAUDEDO_BLOCKED child is identifiable over MCP, since it still reaches Done per the unified parent model.
This commit is contained in:
mika kuns
+136
-36
@@ -19,7 +19,10 @@ namespace ClaudeDo.Worker.External;
|
||||
public sealed record TaskListDto(string Id, string Name, string? WorkingDir);
|
||||
public sealed record DeleteTaskResult(bool Deleted, string Id);
|
||||
public sealed record CancelTaskResult(bool Cancelled, string Id);
|
||||
public sealed record ReviewTaskResult(TaskRefDto Task, string? MergeStatus, IReadOnlyList<string> MergeConflicts, string? MergeMessage, string? RepoPath = null);
|
||||
// EmptyChildren is non-null only for a parent's approve (unit merge): the Done children whose
|
||||
// review range (worktree ahead, or HandlerBaseCommit..HandlerHeadCommit for a worktree-less
|
||||
// child) contributed nothing, so a reviewer sees them before approving instead of after.
|
||||
public sealed record ReviewTaskResult(TaskRefDto Task, string? MergeStatus, IReadOnlyList<string> MergeConflicts, string? MergeMessage, string? RepoPath = null, IReadOnlyList<TaskRefDto>? EmptyChildren = null);
|
||||
public sealed record StatusValueDto(string Status, string Meaning);
|
||||
public sealed record RunTaskNowResult(bool Started, string TaskId);
|
||||
|
||||
@@ -35,7 +38,12 @@ public sealed record TaskDto(
|
||||
DateTime? StartedAt,
|
||||
DateTime? FinishedAt,
|
||||
bool IsMyDay,
|
||||
int SortOrder);
|
||||
int SortOrder,
|
||||
// Count of CLAUDEDO_BLOCKED roadblocks the run reported, stamped by TaskRunner on finish.
|
||||
// A planning/improvement child reporting > 0 still goes straight to Done (see
|
||||
// ClaudeDo.Worker/CLAUDE.md → Unified parent model) -- this is the only MCP-visible signal
|
||||
// that it may have delivered nothing despite that Done status.
|
||||
int RoadblockCount = 0);
|
||||
|
||||
// Lean counterpart to TaskDto for writing/status-changing tools: echoes back what changed
|
||||
// without re-sending Description/Result, which the caller just sent or already has.
|
||||
@@ -45,7 +53,8 @@ public sealed record TaskRefDto(
|
||||
string Title,
|
||||
string Status,
|
||||
int SortOrder,
|
||||
bool IsMyDay);
|
||||
bool IsMyDay,
|
||||
int RoadblockCount = 0);
|
||||
|
||||
// tasks is populated when includeDescription=false (the default): lean references, no
|
||||
// Description/Result. tasksFull is populated when includeDescription=true: full tasks incl.
|
||||
@@ -71,11 +80,15 @@ public sealed record MergeContinuationResultDto(
|
||||
bool Merged, string TaskStatus, IReadOnlyList<string> Conflicts,
|
||||
string? RepoPath, string? Message);
|
||||
|
||||
// IsEmpty = the review range contributed nothing (worktree ahead-of-base is empty, or a
|
||||
// worktree-less handler task's HandlerBaseCommit == HandlerHeadCommit) -- distinguishable from
|
||||
// a merge that is merely small, so an empty branch can't be misread as "changedFileCount: 0
|
||||
// means tiny" when it actually means "nothing to review".
|
||||
public sealed record MergePreviewToolDto(
|
||||
string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind);
|
||||
string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, bool IsEmpty = false);
|
||||
|
||||
public sealed record MergePreviewSetEntryDto(
|
||||
string TaskId, string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, string? Error);
|
||||
string TaskId, string Status, IReadOnlyList<string> ConflictFiles, int ChangedFileCount, int Behind, string? Error, bool IsEmpty = false);
|
||||
|
||||
public sealed record FileOverlapDto(string File, IReadOnlyList<string> TaskIds);
|
||||
|
||||
@@ -396,7 +409,10 @@ public sealed class ExternalMcpService
|
||||
"decision='reject_park' → Idle for manual editing (feedback ignored). " +
|
||||
"decision='cancel' → Cancelled. " +
|
||||
"Fails if the task is not currently WaitingForReview (except cancel, which also works while Running/Queued). " +
|
||||
"The result's task field is a lean reference (id, listId, title, status, sortOrder, isMyDay), not the task's description.")]
|
||||
"The result's task field is a lean reference (id, listId, title, status, sortOrder, isMyDay), not the task's description. " +
|
||||
"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.")]
|
||||
public async Task<ReviewTaskResult> ReviewTask(
|
||||
string taskId,
|
||||
string decision,
|
||||
@@ -412,6 +428,7 @@ public sealed class ExternalMcpService
|
||||
IReadOnlyList<string> mergeConflicts = Array.Empty<string>();
|
||||
string? mergeMessage = null;
|
||||
string? repoPath = null;
|
||||
IReadOnlyList<TaskRefDto>? emptyChildren = null;
|
||||
|
||||
if (decision.Trim().ToLowerInvariant() == "approve")
|
||||
{
|
||||
@@ -423,6 +440,10 @@ public sealed class ExternalMcpService
|
||||
|
||||
if (hasChildren)
|
||||
{
|
||||
// Compute before the merge starts -- children are still Done with their own
|
||||
// pre-merge worktree/commit range at this point, so "did it contribute anything"
|
||||
// reflects the review range the reviewer is about to approve.
|
||||
emptyChildren = await GetEmptyDoneChildrenAsync(taskId, cancellationToken);
|
||||
await _planningMerge.StartAsync(taskId, targetBranch ?? "", cancellationToken);
|
||||
var parentDone = (await _tasks.GetByIdAsync(taskId, cancellationToken))!.Status == TaskStatus.Done;
|
||||
mergeStatus = parentDone ? TaskMergeService.StatusMerged : TaskMergeService.StatusConflict;
|
||||
@@ -478,7 +499,51 @@ public sealed class ExternalMcpService
|
||||
|
||||
return new ReviewTaskResult(
|
||||
ToRefDto((await _tasks.GetByIdAsync(taskId, cancellationToken))!),
|
||||
mergeStatus, mergeConflicts, mergeMessage, repoPath);
|
||||
mergeStatus, mergeConflicts, mergeMessage, repoPath, emptyChildren);
|
||||
}
|
||||
|
||||
// Done children about to be unit-merged whose own review range contributed nothing: an
|
||||
// active worktree with zero changed files against its base commit, or a worktree-less
|
||||
// handler child whose HandlerBaseCommit == HandlerHeadCommit. A child with neither (never
|
||||
// committed anything at all) also counts as empty. Best-effort per child -- a diff failure
|
||||
// on one child must not block the caller from seeing the others or from approving.
|
||||
private async Task<IReadOnlyList<TaskRefDto>> GetEmptyDoneChildrenAsync(string parentTaskId, CancellationToken ct)
|
||||
{
|
||||
await using var ctx = await _dbFactory.CreateDbContextAsync(ct);
|
||||
var children = await ctx.Tasks
|
||||
.AsNoTracking()
|
||||
.Include(t => t.Worktree)
|
||||
.Where(t => t.ParentTaskId == parentTaskId && t.Status == TaskStatus.Done)
|
||||
.ToListAsync(ct);
|
||||
|
||||
var empty = new List<TaskRefDto>();
|
||||
foreach (var child in children)
|
||||
{
|
||||
if (await IsChildEmptyAsync(child, ct))
|
||||
empty.Add(ToRefDto(child));
|
||||
}
|
||||
return empty;
|
||||
}
|
||||
|
||||
private async Task<bool> IsChildEmptyAsync(TaskEntity child, CancellationToken ct)
|
||||
{
|
||||
try
|
||||
{
|
||||
if (child.Worktree is not null)
|
||||
{
|
||||
if (!Directory.Exists(child.Worktree.Path)) return false;
|
||||
var files = ParseDiffStatFileNames(
|
||||
await _git.DiffStatAsync(child.Worktree.Path, child.Worktree.BaseCommit, "HEAD", ct));
|
||||
return files.Count == 0;
|
||||
}
|
||||
if (child.HandlerBaseCommit is { Length: > 0 } handlerBase && child.HandlerHeadCommit is { Length: > 0 } handlerHead)
|
||||
return string.Equals(handlerBase, handlerHead, StringComparison.Ordinal);
|
||||
return true;
|
||||
}
|
||||
catch
|
||||
{
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
[McpServerTool, Description("Immediately run a task in the override execution slot (bypasses the agent queue). Returns { started: true, taskId } on success.")]
|
||||
@@ -800,15 +865,18 @@ public sealed class ExternalMcpService
|
||||
"IMPORTANT: a clean preview says nothing about whether the merged result compiles or passes tests — git " +
|
||||
"can merge two changes cleanly (e.g. one file deletes a symbol another file still references) and still " +
|
||||
"break the build. " +
|
||||
"Throws a clear error if the task has no worktree, the worktree is not Active, or the list's working " +
|
||||
"directory is missing from disk.")]
|
||||
"isEmpty=true means the task's review range contributed nothing (no commits ahead of base, or — for a " +
|
||||
"worktree-less list-handler host task — HandlerBaseCommit == HandlerHeadCommit); do not mistake a small " +
|
||||
"changedFileCount for an empty one, check isEmpty instead. " +
|
||||
"Throws a clear error if the task has neither an active worktree nor a handler commit range, or the " +
|
||||
"list's working directory is missing from disk.")]
|
||||
public async Task<MergePreviewToolDto> PreviewMerge(
|
||||
string taskId,
|
||||
string? targetBranch = null,
|
||||
CancellationToken cancellationToken = default)
|
||||
{
|
||||
var (preview, behind, _) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind);
|
||||
var (preview, behind, _, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
return new MergePreviewToolDto(preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, isEmpty);
|
||||
}
|
||||
|
||||
[McpServerTool, Description(
|
||||
@@ -820,7 +888,8 @@ public sealed class ExternalMcpService
|
||||
"which tasks touch it — passing a single taskId always yields an empty overlaps list. " +
|
||||
"IMPORTANT: file-name overlap is a HINT, not a guarantee of a real collision, and its absence is not a " +
|
||||
"guarantee of safety — two tasks touching different files entirely (e.g. one deletes a symbol, another " +
|
||||
"still references it elsewhere) can still collide, and this tool will not flag that case.")]
|
||||
"still references it elsewhere) can still collide, and this tool will not flag that case. " +
|
||||
"isEmpty=true (per entry) means that task's review range contributed nothing — see preview_merge.")]
|
||||
public async Task<MergePreviewSetResultDto> PreviewMergeSet(
|
||||
IReadOnlyList<string> taskIds,
|
||||
string? targetBranch = null,
|
||||
@@ -836,9 +905,9 @@ public sealed class ExternalMcpService
|
||||
{
|
||||
try
|
||||
{
|
||||
var (preview, behind, changedFiles) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
var (preview, behind, changedFiles, isEmpty) = await PreviewMergeCoreAsync(taskId, targetBranch, cancellationToken);
|
||||
entries.Add(new MergePreviewSetEntryDto(
|
||||
taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null));
|
||||
taskId, preview.Status, preview.ConflictFiles, preview.ChangedFileCount, behind, null, isEmpty));
|
||||
filesByTask[taskId] = changedFiles;
|
||||
}
|
||||
catch (InvalidOperationException ex)
|
||||
@@ -861,32 +930,61 @@ public sealed class ExternalMcpService
|
||||
|
||||
// Shared core for PreviewMerge/PreviewMergeSet: throws a clear InvalidOperationException instead of
|
||||
// TaskMergeService.PreviewAsync's silent "unavailable" status, and adds `behind` + the task's own
|
||||
// changed-file list (via diff-stat, not the merge-tree preview) for overlap detection.
|
||||
private async Task<(MergePreviewResult Preview, int Behind, IReadOnlyList<string> ChangedFiles)> PreviewMergeCoreAsync(
|
||||
// changed-file list (via diff-stat, not the merge-tree preview) for overlap detection, plus
|
||||
// `isEmpty`. A worktree-less list-handler host task has no branch to merge-tree-preview at all
|
||||
// (its commits already sit on the list's working dir) — falls back to the fixed
|
||||
// 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<string> ChangedFiles, bool IsEmpty)> PreviewMergeCoreAsync(
|
||||
string taskId, string? targetBranch, CancellationToken ct)
|
||||
{
|
||||
var (_, list, wt) = await LoadWorktreeContextAsync(taskId, ct);
|
||||
if (wt.State != WorktreeState.Active)
|
||||
throw new InvalidOperationException(
|
||||
$"Worktree state must be Active to preview a merge (current: {wt.State}).");
|
||||
if (string.IsNullOrWhiteSpace(list.WorkingDir) || !Directory.Exists(list.WorkingDir))
|
||||
throw new InvalidOperationException("The list's working directory no longer exists.");
|
||||
using var ctx = _dbFactory.CreateDbContext();
|
||||
var task = await new TaskRepository(ctx).GetByIdAsync(taskId, ct)
|
||||
?? throw new InvalidOperationException($"Task {taskId} not found.");
|
||||
var list = await new ListRepository(ctx).GetByIdAsync(task.ListId, ct)
|
||||
?? throw new InvalidOperationException("List not found.");
|
||||
var wt = await new WorktreeRepository(ctx).GetByTaskIdAsync(taskId, ct);
|
||||
|
||||
var preview = await _merge.PreviewAsync(taskId, targetBranch ?? "", ct);
|
||||
if (preview.Status == TaskMergeService.PreviewUnavailable)
|
||||
throw new InvalidOperationException(
|
||||
"Merge preview unavailable for this task (worktree inactive or repo is not a git repository).");
|
||||
if (wt is not null)
|
||||
{
|
||||
if (wt.State != WorktreeState.Active)
|
||||
throw new InvalidOperationException(
|
||||
$"Worktree state must be Active to preview a merge (current: {wt.State}).");
|
||||
if (string.IsNullOrWhiteSpace(list.WorkingDir) || !Directory.Exists(list.WorkingDir))
|
||||
throw new InvalidOperationException("The list's working directory no longer exists.");
|
||||
|
||||
var target = string.IsNullOrWhiteSpace(targetBranch)
|
||||
? await _git.GetCurrentBranchAsync(list.WorkingDir, ct)
|
||||
: targetBranch;
|
||||
var behind = await GitRevListCountAsync(list.WorkingDir, $"{wt.BranchName}..{target}", ct);
|
||||
var preview = await _merge.PreviewAsync(taskId, targetBranch ?? "", ct);
|
||||
if (preview.Status == TaskMergeService.PreviewUnavailable)
|
||||
throw new InvalidOperationException(
|
||||
"Merge preview unavailable for this task (worktree inactive or repo is not a git repository).");
|
||||
|
||||
var changedFiles = Directory.Exists(wt.Path)
|
||||
? ParseDiffStatFileNames(await _git.DiffStatAsync(wt.Path, wt.BaseCommit, "HEAD", ct))
|
||||
: Array.Empty<string>();
|
||||
var target = string.IsNullOrWhiteSpace(targetBranch)
|
||||
? await _git.GetCurrentBranchAsync(list.WorkingDir, ct)
|
||||
: targetBranch;
|
||||
var behind = await GitRevListCountAsync(list.WorkingDir, $"{wt.BranchName}..{target}", ct);
|
||||
|
||||
return (preview, behind, changedFiles);
|
||||
var changedFiles = Directory.Exists(wt.Path)
|
||||
? ParseDiffStatFileNames(await _git.DiffStatAsync(wt.Path, wt.BaseCommit, "HEAD", ct))
|
||||
: Array.Empty<string>();
|
||||
|
||||
return (preview, behind, changedFiles, changedFiles.Count == 0);
|
||||
}
|
||||
|
||||
if (task.HandlerBaseCommit is { Length: > 0 } handlerBase && task.HandlerHeadCommit is { Length: > 0 } handlerHead)
|
||||
{
|
||||
if (string.IsNullOrWhiteSpace(list.WorkingDir) || !Directory.Exists(list.WorkingDir))
|
||||
throw new InvalidOperationException("The list's working directory no longer exists.");
|
||||
|
||||
var isEmpty = string.Equals(handlerBase, handlerHead, StringComparison.Ordinal);
|
||||
var changedFiles = isEmpty
|
||||
? Array.Empty<string>()
|
||||
: ParseDiffStatFileNames(await _git.DiffStatAsync(list.WorkingDir, handlerBase, handlerHead, ct));
|
||||
|
||||
var preview = new MergePreviewResult(TaskMergeService.PreviewClean, Array.Empty<string>(), changedFiles.Count);
|
||||
return (preview, 0, changedFiles, isEmpty);
|
||||
}
|
||||
|
||||
throw new InvalidOperationException($"Task {taskId} has no worktree.");
|
||||
}
|
||||
|
||||
[McpServerTool, Description(
|
||||
@@ -1165,7 +1263,8 @@ public sealed class ExternalMcpService
|
||||
t.StartedAt,
|
||||
t.FinishedAt,
|
||||
t.IsMyDay,
|
||||
t.SortOrder);
|
||||
t.SortOrder,
|
||||
t.RoadblockCount);
|
||||
|
||||
private static TaskRefDto ToRefDto(TaskEntity t) => new(
|
||||
t.Id,
|
||||
@@ -1173,7 +1272,8 @@ public sealed class ExternalMcpService
|
||||
t.Title,
|
||||
t.Status.ToString(),
|
||||
t.SortOrder,
|
||||
t.IsMyDay);
|
||||
t.IsMyDay,
|
||||
t.RoadblockCount);
|
||||
}
|
||||
|
||||
internal static class DailyPrepFilter
|
||||
|
||||
Reference in New Issue
Block a user