From 1cb5171fba0a7932c903212bb8210111e135986c Mon Sep 17 00:00:00 2001 From: mika kuns Date: Tue, 2 Jun 2026 08:00:13 +0200 Subject: [PATCH] fix(worker): harden review re-run, timestamps, and queue affordance - Clear ReviewFeedback only after a successful re-run so a failed/cancelled run keeps it for a manual retry. - Clear stale StartedAt/FinishedAt when rejecting a task back to the queue. - Only non-planning standalone tasks gate on review (guard PlanningPhase). - Hide "send to queue" for WaitingForReview tasks so review isn't bypassed. Co-Authored-By: Claude Opus 4.7 --- .../ViewModels/Islands/TaskRowViewModel.cs | 2 +- src/ClaudeDo.Worker/Queue/QueueService.cs | 23 ++++++++++++++----- src/ClaudeDo.Worker/Runner/TaskRunner.cs | 3 ++- src/ClaudeDo.Worker/State/TaskStateService.cs | 4 +++- .../Services/QueueServiceTests.cs | 17 +++++++++++--- 5 files changed, 37 insertions(+), 12 deletions(-) diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs index 9d0e025..f9ae5a4 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TaskRowViewModel.cs @@ -67,7 +67,7 @@ public sealed partial class TaskRowViewModel : ViewModelBase public bool IsQueued => Status == TaskStatus.Queued && string.IsNullOrEmpty(BlockedByTaskId); public bool IsWaiting => Status == TaskStatus.Queued && !string.IsNullOrEmpty(BlockedByTaskId); public bool CanRemoveFromQueue => IsQueued || HasQueuedSubtasks; - public bool CanSendToQueue => !IsRunning && !IsQueued && !HasQueuedSubtasks + public bool CanSendToQueue => !IsRunning && !IsQueued && !IsWaitingForReview && !HasQueuedSubtasks && (!IsChild || ParentFinalized); // Parent-level "send plan to queue" — only once the plan is finalized (children Planned). public bool CanQueuePlan => !IsChild && HasPlanningChildren diff --git a/src/ClaudeDo.Worker/Queue/QueueService.cs b/src/ClaudeDo.Worker/Queue/QueueService.cs index 1884382..11b99f2 100644 --- a/src/ClaudeDo.Worker/Queue/QueueService.cs +++ b/src/ClaudeDo.Worker/Queue/QueueService.cs @@ -5,6 +5,7 @@ using ClaudeDo.Worker.Config; using ClaudeDo.Worker.Runner; using ClaudeDo.Worker.State; using Microsoft.EntityFrameworkCore; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; namespace ClaudeDo.Worker.Queue; @@ -188,17 +189,27 @@ public sealed class QueueService : BackgroundService using (var context = _dbFactory.CreateDbContext()) sessionId = (await new TaskRunRepository(context).GetLatestByTaskIdAsync(taskId, ct))?.SessionId; - await _state.ClearReviewFeedbackAsync(taskId, ct); - if (sessionId is not null) { await _runner.ContinueAsync(taskId, feedback, "queue", ct); - return; + } + else + { + task.Description = string.IsNullOrWhiteSpace(task.Description) + ? $"Reviewer feedback: {feedback}" + : $"{task.Description}\n\nReviewer feedback: {feedback}"; + await _runner.RunAsync(task, "queue", ct); } - task.Description = string.IsNullOrWhiteSpace(task.Description) - ? $"Reviewer feedback: {feedback}" - : $"{task.Description}\n\nReviewer feedback: {feedback}"; + // Clear the consumed feedback only once the run reached a successful + // terminal state, so a failed or cancelled run keeps it for a manual retry. + TaskStatus statusAfter; + using (var context = _dbFactory.CreateDbContext()) + statusAfter = await context.Tasks.Where(t => t.Id == taskId) + .Select(t => t.Status).FirstAsync(CancellationToken.None); + if (statusAfter is TaskStatus.WaitingForReview or TaskStatus.Done) + await _state.ClearReviewFeedbackAsync(taskId, CancellationToken.None); + return; } await _runner.RunAsync(task, "queue", ct); diff --git a/src/ClaudeDo.Worker/Runner/TaskRunner.cs b/src/ClaudeDo.Worker/Runner/TaskRunner.cs index d33b660..5c573a8 100644 --- a/src/ClaudeDo.Worker/Runner/TaskRunner.cs +++ b/src/ClaudeDo.Worker/Runner/TaskRunner.cs @@ -324,8 +324,9 @@ public sealed class TaskRunner // after the Claude run already succeeded. // Standalone tasks gate on review; planning children go straight to Done // so the sequential chain (which advances on terminal states) is unaffected. + // Planning parents (PlanningPhase != None) are containers, not reviewable work. var finishedAt = DateTime.UtcNow; - if (task.ParentTaskId is null) + if (task.ParentTaskId is null && task.PlanningPhase == PlanningPhase.None) { await _state.SubmitForReviewAsync(task.Id, finishedAt, result.ResultMarkdown, CancellationToken.None); await _broadcaster.WorkerLog($"Finished \"{task.Title}\" (waiting for review)", WorkerLogLevel.Success, DateTime.UtcNow); diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index a8b7df5..f012700 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -134,7 +134,9 @@ public sealed class TaskStateService : ITaskStateService .Where(t => t.Id == taskId && t.Status == TaskStatus.WaitingForReview) .ExecuteUpdateAsync(s => s .SetProperty(t => t.Status, TaskStatus.Queued) - .SetProperty(t => t.ReviewFeedback, feedback), ct); + .SetProperty(t => t.ReviewFeedback, feedback) + .SetProperty(t => t.StartedAt, (DateTime?)null) + .SetProperty(t => t.FinishedAt, (DateTime?)null), ct); if (affected == 0) return new TransitionResult(false, "Task is not waiting for review; cannot reject."); diff --git a/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs index fc78e8d..7b75e9b 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs @@ -156,13 +156,24 @@ public sealed class QueueServiceTests : IDisposable await service.StartAsync(cts.Token); _waker.Wake(); await done.Task.WaitAsync(TimeSpan.FromSeconds(5)); - cts.Cancel(); Assert.Contains("--resume sess-1", capturedArgs); Assert.Equal("fix the bug", capturedPrompt); - var reloaded = await _taskRepo.GetByIdAsync(task.Id); - Assert.Null(reloaded!.ReviewFeedback); + // Feedback is cleared after the run reaches a successful terminal state (post-run), + // so poll rather than asserting on the handler-fired instant. + var deadline = DateTime.UtcNow.AddSeconds(5); + TaskEntity? reloaded; + do + { + reloaded = await new TaskRepository(_db.CreateContext()).GetByIdAsync(task.Id); + if (reloaded?.ReviewFeedback is null) break; + await Task.Delay(25); + } while (DateTime.UtcNow < deadline); + cts.Cancel(); + + Assert.Equal(TaskStatus.WaitingForReview, reloaded!.Status); + Assert.Null(reloaded.ReviewFeedback); } [Fact]