From 7f337b36c7b1e8dbe76b2b10b48a57ce533d8283 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Fri, 21 Aug 2026 11:44:36 +0200 Subject: [PATCH] fix(worker): route done-toggle and dequeue through guarded TaskStateService transitions The task-list done toggle (both islands) and RemoveFromQueue wrote TaskEntity.Status directly via EF, bypassing TaskStateService: no TaskUpdated broadcast, no guard against a concurrent picker claim (lost update), and no status-based filter. Added guarded MarkDoneAsync/UnmarkDoneAsync/DequeueToIdleAsync transitions plus matching hub methods (SetTaskDone/UnsetTaskDone/DequeueTask) and IWorkerClient wrappers; the three UI call sites now route through the hub with optimistic-then-revert row updates and ErrorReported on failure. RemoveFromQueueAsync dequeues each queued child individually through the same guarded path instead of cascading via a raw EF update. Also closes two hub guard gaps: UpdateListConfig's delete branch now preserves a list's SerializeOnFileOverlap flag instead of dropping it, and SubmitTaskForReview's Idle/Failed status gate now runs before either mutation branch so a Done/Cancelled task can't get committed or stamped and then rejected. --- src/ClaudeDo.Localization/locales/de.json | 4 +- src/ClaudeDo.Localization/locales/en.json | 4 +- .../Services/Interfaces/IWorkerClient.cs | 10 ++ src/ClaudeDo.Ui/Services/WorkerClient.cs | 9 + .../Islands/DetailsIslandViewModel.cs | 30 ++-- .../Islands/TasksIslandViewModel.cs | 78 ++++++--- src/ClaudeDo.Worker/CLAUDE.md | 2 +- src/ClaudeDo.Worker/Hub/WorkerHub.cs | 39 ++++- .../State/Interfaces/ITaskStateService.cs | 10 ++ src/ClaudeDo.Worker/State/TaskStateService.cs | 46 +++++ tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs | 3 + .../DetailsIslandErrorFeedbackTests.cs | 70 ++++++++ .../TasksIslandErrorFeedbackTests.cs | 132 +++++++++++++++ .../TasksIslandRemoveFromQueueTests.cs | 28 ++- .../Hub/ListConfigHubTests.cs | 101 +++++++++++ .../Hub/MergeHelperTaskHubTests.cs | 45 +++++ .../Hub/TaskDoneDequeueHubTests.cs | 160 ++++++++++++++++++ .../PlanningMergeOrchestratorTests.cs | 3 + .../State/TaskStateServiceTests.cs | 78 +++++++++ .../UiVm/TasksIslandViewModelPlanningTests.cs | 3 + 20 files changed, 806 insertions(+), 49 deletions(-) create mode 100644 tests/ClaudeDo.Worker.Tests/Hub/ListConfigHubTests.cs create mode 100644 tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs diff --git a/src/ClaudeDo.Localization/locales/de.json b/src/ClaudeDo.Localization/locales/de.json index dbbada2e..9efceff1 100644 --- a/src/ClaudeDo.Localization/locales/de.json +++ b/src/ClaudeDo.Localization/locales/de.json @@ -685,7 +685,7 @@ "planningBadge": { "active": "PLANUNG", "finalized": "GEPLANT" }, "taskRow": { "createdPrefix": "Erstellt {0}", "stepsText": "{0}/{1} Schritte" }, "queue": { "baseDirtyWarning": "Achtung: Das Repo dieser Liste hat uncommittete Änderungen ({0} geändert, {1} untracked) — ein neuer Worktree startet vom letzten Commit und enthält sie nicht." }, - "tasksIsland": { "completedHeader": "ABGESCHLOSSEN", "completedHeaderCount": "ABGESCHLOSSEN · {0}", "planningOpenFailed": "Planungssitzung konnte nicht geöffnet werden: {0}", "planningResumeFailed": "Planungssitzung konnte nicht fortgesetzt werden: {0}", "approveFailed": "Genehmigen & Mergen fehlgeschlagen: {0}", "cancelReviewFailed": "Prüfung abbrechen fehlgeschlagen: {0}", "sendToQueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "queuePlanBlockedInteractive": "Plan kann nicht in die Warteschlange gestellt werden — {0} hat eine offene interaktive Sitzung und muss zuerst geschlossen werden.", "moveRunningRejected": "Ein laufender Task kann nicht in eine andere Liste verschoben werden.", "moveWorktreeRejected": "Verschieben nicht möglich — dieser Task hat einen aktiven Worktree, der auf sein aktuelles Repo zeigt.", "moveRepoConfirm": "Unterschiedliche Repos — {0} → {1}. Task trotzdem verschieben?", "moveConfirmUnavailable": "Verschieben nicht möglich — der Bestätigungsdialog ist nicht verfügbar.", "quickClaudeNoWorkingDir": "Für diese Liste ist kein Arbeitsverzeichnis konfiguriert.", "quickClaudeDirMissing": "Arbeitsverzeichnis existiert nicht mehr: {0}", "setStatusFailed": "Status konnte nicht aktualisiert werden: {0}", "cancelFailed": "Abbrechen fehlgeschlagen: {0}", "rejectToQueueFailed": "Zurückweisen in die Warteschlange fehlgeschlagen: {0}", "rejectToIdleFailed": "Zurückweisen fehlgeschlagen: {0}", "deleteTaskConfirm": "\"{0}\" löschen? Dies kann nicht rückgängig gemacht werden.", "deleteTaskFailed": "Löschen fehlgeschlagen: {0}" }, + "tasksIsland": { "completedHeader": "ABGESCHLOSSEN", "completedHeaderCount": "ABGESCHLOSSEN · {0}", "planningOpenFailed": "Planungssitzung konnte nicht geöffnet werden: {0}", "planningResumeFailed": "Planungssitzung konnte nicht fortgesetzt werden: {0}", "approveFailed": "Genehmigen & Mergen fehlgeschlagen: {0}", "cancelReviewFailed": "Prüfung abbrechen fehlgeschlagen: {0}", "sendToQueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "queuePlanBlockedInteractive": "Plan kann nicht in die Warteschlange gestellt werden — {0} hat eine offene interaktive Sitzung und muss zuerst geschlossen werden.", "moveRunningRejected": "Ein laufender Task kann nicht in eine andere Liste verschoben werden.", "moveWorktreeRejected": "Verschieben nicht möglich — dieser Task hat einen aktiven Worktree, der auf sein aktuelles Repo zeigt.", "moveRepoConfirm": "Unterschiedliche Repos — {0} → {1}. Task trotzdem verschieben?", "moveConfirmUnavailable": "Verschieben nicht möglich — der Bestätigungsdialog ist nicht verfügbar.", "quickClaudeNoWorkingDir": "Für diese Liste ist kein Arbeitsverzeichnis konfiguriert.", "quickClaudeDirMissing": "Arbeitsverzeichnis existiert nicht mehr: {0}", "setStatusFailed": "Status konnte nicht aktualisiert werden: {0}", "cancelFailed": "Abbrechen fehlgeschlagen: {0}", "rejectToQueueFailed": "Zurückweisen in die Warteschlange fehlgeschlagen: {0}", "rejectToIdleFailed": "Zurückweisen fehlgeschlagen: {0}", "deleteTaskConfirm": "\"{0}\" löschen? Dies kann nicht rückgängig gemacht werden.", "deleteTaskFailed": "Löschen fehlgeschlagen: {0}", "toggleDoneFailed": "Erledigt-Status konnte nicht aktualisiert werden: {0}", "removeFromQueueFailed": "Aus der Warteschlange entfernen fehlgeschlagen: {0}" }, "diff": { "loadFailed": "Diff konnte nicht geladen werden: {0}", "noChanges": "Keine Änderungen anzuzeigen.", "unavailable": "Diff nicht mehr verfügbar — Commit-Bereich unvollständig." }, "planningDiff": { "hubError": "Kombinierte Vorschau konnte nicht erstellt werden (Hub-Fehler).", "conflict": "Kombinierte Vorschau nicht möglich: Teilaufgabe {0} steht im Konflikt mit einer früheren Teilaufgabe ({1} Dateien).", "buildFailed": "Kombinierte Vorschau konnte nicht erstellt werden: {0}" }, "merge": { "commitMessage": "chore: merge {0}", "progressMerging": "Wird zusammengeführt…", "progressVerifying": "Verify-Kommando der Liste läuft… ({0})", "workerOfflineBranches": "Worker offline — Branches können nicht aufgelistet werden.", "loadBranchesFailed": "Branches konnten nicht geladen werden: {0}", "merged": "Zusammengeführt.", "conflict": "Merge-Konflikt — Ziel-Branch wiederhergestellt. Manuell oder über Fortsetzen lösen, dann erneut versuchen.", "blocked": "Blockiert: {0}", "verifyFailed": "Merge ist gelandet, aber das Verify-Kommando der Liste ist fehlgeschlagen — die Aufgabe wurde nicht auf Erledigt gesetzt.", "untrackedCollision": "Merge abgelehnt — er würde eine unversionierte Datei im Ziel-Arbeitsverzeichnis überschreiben.", "unknownStatus": "Unbekannter Status: {0}", "mergeFailed": "Merge fehlgeschlagen: {0}" }, @@ -703,7 +703,7 @@ "worktreesTab": { "workerOffline": "Worker offline.", "removed": "{0} Worktree(s) entfernt.", "blocked": "Zwangsentfernung nicht möglich: {0} Aufgabe(n) laufen noch. Brich sie zuerst ab.", "removedFrom": "{0} Worktree(s) von {1} Aufgabe(n) entfernt.", "cleanupFailed": "Aufräumen fehlgeschlagen: {0}", "resetFailed": "Zurücksetzen fehlgeschlagen: {0}" }, "worktreesOverview": { "titleAll": "Worktrees", "titleList": "Worktrees — {0}", "listFallback": "Liste", "cleanupFailed": "Aufräumen fehlgeschlagen.", "cleanupFailedDetailed": "Aufräumen fehlgeschlagen: {0}", "removed": "{0} Worktree(s) entfernt.", "discardConfirm": "Worktree für \"{0}\" verwerfen? Nicht committete Arbeit geht verloren.", "discardFailed": "Worktree konnte nicht verworfen werden.", "keepFailed": "Worktree konnte nicht behalten werden.", "cannotForceRunning": "Eine laufende Aufgabe kann nicht zwangsweise entfernt werden.", "forceRemoveFailed": "Zwangsentfernung fehlgeschlagen.", "forceRemoveFailedDetailed": "Zwangsentfernung fehlgeschlagen: {0}", "batchProgress": "Merge {0}/{1}…", "batchDone": "{0} gemergt, {1} zu lösen." }, "listSettings": { "untitled": "Unbenannt" }, - "detailsIsland": { "verifyFailed": "Merge ist erfolgt, aber das Verifikationskommando der Liste ist fehlgeschlagen — die Aufgabe wurde nicht auf 'Erledigt' gesetzt.", "untrackedCollision": "Merge abgelehnt — er würde eine unversionierte Datei im Ziel-Arbeitsverzeichnis überschreiben.", "stopOffline": "Worker offline — Task kann nicht gestoppt werden.", "stopFailed": "Stoppen fehlgeschlagen: {0}", "enqueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "dequeueFailed": "Aus der Warteschlange entfernen fehlgeschlagen: {0}", "resetAndRetryFailed": "Zurücksetzen & erneut versuchen fehlgeschlagen: {0}" }, + "detailsIsland": { "verifyFailed": "Merge ist erfolgt, aber das Verifikationskommando der Liste ist fehlgeschlagen — die Aufgabe wurde nicht auf 'Erledigt' gesetzt.", "untrackedCollision": "Merge abgelehnt — er würde eine unversionierte Datei im Ziel-Arbeitsverzeichnis überschreiben.", "stopOffline": "Worker offline — Task kann nicht gestoppt werden.", "stopFailed": "Stoppen fehlgeschlagen: {0}", "enqueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "dequeueFailed": "Aus der Warteschlange entfernen fehlgeschlagen: {0}", "resetAndRetryFailed": "Zurücksetzen & erneut versuchen fehlgeschlagen: {0}", "toggleDoneFailed": "Erledigt-Status konnte nicht aktualisiert werden: {0}" }, "lists": { "localSuffix": "{0} / lokal", "smartMyDay": "Mein Tag", "smartImportant": "Wichtig", "smartPlanned": "Geplant", "virtualQueue": "Warteschlange", "virtualRunning": "Läuft", "virtualReview": "Prüfung", "newList": "Neue Liste", "findingsNotFound": "Für diese Liste gibt es noch keine Findings.", "findingsOpenFailed": "Findings-Ordner konnte nicht geöffnet werden: {0}" }, "repoImport": { "loadFailed": "Gespeicherte Ordner konnten nicht geladen werden: {0}", "saveFailed": "Ordner konnten nicht gespeichert werden: {0}" } }, diff --git a/src/ClaudeDo.Localization/locales/en.json b/src/ClaudeDo.Localization/locales/en.json index cdd93357..e9a07b2c 100644 --- a/src/ClaudeDo.Localization/locales/en.json +++ b/src/ClaudeDo.Localization/locales/en.json @@ -685,7 +685,7 @@ "planningBadge": { "active": "PLANNING", "finalized": "PLANNED" }, "taskRow": { "createdPrefix": "Created {0}", "stepsText": "{0}/{1} steps" }, "queue": { "baseDirtyWarning": "Heads up: this list's repo has uncommitted changes ({0} modified, {1} untracked) — a new worktree starts from the last commit and won't include them." }, - "tasksIsland": { "completedHeader": "COMPLETED", "completedHeaderCount": "COMPLETED · {0}", "planningOpenFailed": "Couldn't open planning session: {0}", "planningResumeFailed": "Couldn't resume planning session: {0}", "approveFailed": "Approve & merge failed: {0}", "cancelReviewFailed": "Cancel review failed: {0}", "sendToQueueFailed": "Send to queue failed: {0}", "queuePlanBlockedInteractive": "Can't queue the plan — {0} has an open interactive session and must be closed first.", "moveRunningRejected": "Can't move a running task to another list.", "moveWorktreeRejected": "Can't move — this task has an active worktree pointing at its current repo.", "moveRepoConfirm": "Different repos — {0} → {1}. Move the task anyway?", "moveConfirmUnavailable": "Can't move — the confirmation dialog isn't available.", "quickClaudeNoWorkingDir": "This list has no working directory configured.", "quickClaudeDirMissing": "Working directory no longer exists: {0}", "setStatusFailed": "Failed to update status: {0}", "cancelFailed": "Cancel failed: {0}", "rejectToQueueFailed": "Reject to queue failed: {0}", "rejectToIdleFailed": "Reject failed: {0}", "deleteTaskConfirm": "Delete \"{0}\"? This cannot be undone.", "deleteTaskFailed": "Delete failed: {0}" }, + "tasksIsland": { "completedHeader": "COMPLETED", "completedHeaderCount": "COMPLETED · {0}", "planningOpenFailed": "Couldn't open planning session: {0}", "planningResumeFailed": "Couldn't resume planning session: {0}", "approveFailed": "Approve & merge failed: {0}", "cancelReviewFailed": "Cancel review failed: {0}", "sendToQueueFailed": "Send to queue failed: {0}", "queuePlanBlockedInteractive": "Can't queue the plan — {0} has an open interactive session and must be closed first.", "moveRunningRejected": "Can't move a running task to another list.", "moveWorktreeRejected": "Can't move — this task has an active worktree pointing at its current repo.", "moveRepoConfirm": "Different repos — {0} → {1}. Move the task anyway?", "moveConfirmUnavailable": "Can't move — the confirmation dialog isn't available.", "quickClaudeNoWorkingDir": "This list has no working directory configured.", "quickClaudeDirMissing": "Working directory no longer exists: {0}", "setStatusFailed": "Failed to update status: {0}", "cancelFailed": "Cancel failed: {0}", "rejectToQueueFailed": "Reject to queue failed: {0}", "rejectToIdleFailed": "Reject failed: {0}", "deleteTaskConfirm": "Delete \"{0}\"? This cannot be undone.", "deleteTaskFailed": "Delete failed: {0}", "toggleDoneFailed": "Couldn't update done state: {0}", "removeFromQueueFailed": "Remove from queue failed: {0}" }, "diff": { "loadFailed": "Failed to load diff: {0}", "noChanges": "No changes to show.", "unavailable": "Diff no longer available — commit range incomplete." }, "planningDiff": { "hubError": "Could not build combined preview (hub error).", "conflict": "Cannot build combined preview: subtask {0} conflicts with an earlier subtask ({1} files).", "buildFailed": "Could not build combined preview: {0}" }, "merge": { "commitMessage": "chore: merge {0}", "progressMerging": "Merging…", "progressVerifying": "Running the list's verify command… ({0})", "workerOfflineBranches": "Worker offline — cannot list branches.", "loadBranchesFailed": "Failed to load branches: {0}", "merged": "Merged.", "conflict": "Merge conflict — target branch restored. Resolve manually or via Continue, then retry.", "blocked": "Blocked: {0}", "verifyFailed": "Merge landed, but the list's verify command failed — the task was kept out of Done.", "untrackedCollision": "Merge refused — it would overwrite an untracked file in the target working directory.", "unknownStatus": "Unknown status: {0}", "mergeFailed": "Merge failed: {0}" }, @@ -703,7 +703,7 @@ "worktreesTab": { "workerOffline": "Worker offline.", "removed": "Removed {0} worktree(s).", "blocked": "Cannot force-remove: {0} task(s) still running. Cancel them first.", "removedFrom": "Removed {0} worktree(s) from {1} task(s).", "cleanupFailed": "Cleanup failed: {0}", "resetFailed": "Reset failed: {0}" }, "worktreesOverview": { "titleAll": "Worktrees", "titleList": "Worktrees — {0}", "listFallback": "list", "cleanupFailed": "Cleanup failed.", "cleanupFailedDetailed": "Cleanup failed: {0}", "removed": "Removed {0} worktree(s).", "discardConfirm": "Discard the worktree for \"{0}\"? Uncommitted work will be lost.", "discardFailed": "Failed to discard worktree.", "keepFailed": "Failed to keep worktree.", "cannotForceRunning": "Cannot force-remove a running task.", "forceRemoveFailed": "Force remove failed.", "forceRemoveFailedDetailed": "Force remove failed: {0}", "batchProgress": "Merging {0}/{1}…", "batchDone": "Merged {0}, {1} need resolution." }, "listSettings": { "untitled": "Untitled" }, - "detailsIsland": { "verifyFailed": "Merge landed, but the list's verify command failed — the task was kept out of Done.", "untrackedCollision": "Merge refused — it would overwrite an untracked file in the target working directory.", "stopOffline": "Worker offline — can't stop the task.", "stopFailed": "Stop failed: {0}", "enqueueFailed": "Queue failed: {0}", "dequeueFailed": "Remove from queue failed: {0}", "resetAndRetryFailed": "Reset & retry failed: {0}" }, + "detailsIsland": { "verifyFailed": "Merge landed, but the list's verify command failed — the task was kept out of Done.", "untrackedCollision": "Merge refused — it would overwrite an untracked file in the target working directory.", "stopOffline": "Worker offline — can't stop the task.", "stopFailed": "Stop failed: {0}", "enqueueFailed": "Queue failed: {0}", "dequeueFailed": "Remove from queue failed: {0}", "resetAndRetryFailed": "Reset & retry failed: {0}", "toggleDoneFailed": "Couldn't update done state: {0}" }, "lists": { "localSuffix": "{0} / local", "smartMyDay": "My Day", "smartImportant": "Important", "smartPlanned": "Planned", "virtualQueue": "Queue", "virtualRunning": "Running", "virtualReview": "Review", "newList": "New list", "findingsNotFound": "No findings yet for this list.", "findingsOpenFailed": "Couldn't open findings folder: {0}" }, "repoImport": { "loadFailed": "Couldn't load remembered folders: {0}", "saveFailed": "Couldn't save folders: {0}" } }, diff --git a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs index 08976c92..1598aac2 100644 --- a/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/Interfaces/IWorkerClient.cs @@ -96,6 +96,16 @@ public interface IWorkerClient : INotifyPropertyChanged /// a list whose working dir has uncommitted changes (null otherwise) — a new worktree forks /// from the last commit, not the working tree, so those changes won't be included. Task SetTaskStatusAsync(string taskId, TaskStatus status); + /// Guarded "manual done toggle" (checks the task is currently Idle server-side + /// before flipping it to Done) — replaces a raw EF write in the task-list checkbox path. + Task SetTaskDoneAsync(string taskId); + /// Guarded un-toggle (checks the task is currently Done server-side before + /// flipping it back to Idle). + Task UnsetTaskDoneAsync(string taskId); + /// Guarded "remove from queue" (checks the task is currently Queued server-side + /// and clears BlockedByTaskId in the same update) — call once per row (parent and each + /// queued child) rather than cascading server-side. + Task DequeueTaskAsync(string taskId); Task ApproveReviewAsync(string taskId, string targetBranch); Task PreviewMergeAsync(string taskId, string targetBranch); Task MergeTaskAsync(string taskId, string targetBranch, bool removeWorktree, string commitMessage); diff --git a/src/ClaudeDo.Ui/Services/WorkerClient.cs b/src/ClaudeDo.Ui/Services/WorkerClient.cs index 0c75dbd6..b1a20da9 100644 --- a/src/ClaudeDo.Ui/Services/WorkerClient.cs +++ b/src/ClaudeDo.Ui/Services/WorkerClient.cs @@ -536,6 +536,15 @@ public partial class WorkerClient : ObservableObject, IAsyncDisposable, IWorkerC return result?.BaseDirty; } + public Task SetTaskDoneAsync(string taskId) + => InvokeTimedAsync("SetTaskDone", () => _hub.InvokeAsync("SetTaskDone", taskId)); + + public Task UnsetTaskDoneAsync(string taskId) + => InvokeTimedAsync("UnsetTaskDone", () => _hub.InvokeAsync("UnsetTaskDone", taskId)); + + public Task DequeueTaskAsync(string taskId) + => InvokeTimedAsync("DequeueTask", () => _hub.InvokeAsync("DequeueTask", taskId)); + public async Task ApproveReviewAsync(string taskId, string targetBranch) { LastApproveTarget = targetBranch; diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 712b2351..0a9cf3eb 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -1024,17 +1024,25 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private async System.Threading.Tasks.Task ToggleDoneAsync() { if (Task is null) return; - Task.Done = !Task.Done; - await using var ctx = _dbFactory.CreateDbContext(); - var repo = new TaskRepository(ctx); - var entity = await repo.GetByIdAsync(Task.Id); - if (entity is null) return; - entity.Status = Task.Done - ? ClaudeDo.Data.Models.TaskStatus.Done - : ClaudeDo.Data.Models.TaskStatus.Idle; - Task.Status = entity.Status; - Monitor.ApplyState(entity.Status); - await repo.UpdateAsync(entity); + + var newDone = !Task.Done; + var previousStatus = Task.Status; + Task.Done = newDone; + var newStatus = newDone ? ClaudeDo.Data.Models.TaskStatus.Done : ClaudeDo.Data.Models.TaskStatus.Idle; + Task.Status = newStatus; + Monitor.ApplyState(newStatus); + try + { + if (newDone) await _worker.SetTaskDoneAsync(Task.Id); + else await _worker.UnsetTaskDoneAsync(Task.Id); + } + catch (Exception ex) + { + Task.Done = !newDone; + Task.Status = previousStatus; + Monitor.ApplyState(previousStatus); + ErrorReported?.Invoke(Loc.T("vm.detailsIsland.toggleDoneFailed", ex.Message)); + } } [RelayCommand] diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs index a77150ed..0b5fb0a2 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs @@ -1028,14 +1028,23 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable [RelayCommand] private async Task ToggleDoneAsync(TaskRowViewModel row) { - row.Done = !row.Done; - await using var db = await _dbFactory.CreateDbContextAsync(); - var entity = await db.Tasks.FirstOrDefaultAsync(t => t.Id == row.Id); - if (entity != null) + if (_worker is null) return; + + var newDone = !row.Done; + var previousStatus = row.Status; + row.Done = newDone; + row.Status = newDone ? TaskStatus.Done : TaskStatus.Idle; + try { - entity.Status = row.Done ? TaskStatus.Done : TaskStatus.Idle; - row.Status = entity.Status; - await db.SaveChangesAsync(); + if (newDone) await _worker.SetTaskDoneAsync(row.Id); + else await _worker.UnsetTaskDoneAsync(row.Id); + } + catch (Exception ex) + { + row.Done = !newDone; + row.Status = previousStatus; + ErrorReported?.Invoke(Loc.T("vm.tasksIsland.toggleDoneFailed", ex.Message)); + return; } Regroup(); UpdateSubtitle(); @@ -1248,39 +1257,56 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable [RelayCommand] private async Task RemoveFromQueueAsync(TaskRowViewModel? row) { - if (row is null) return; - await using var db = await _dbFactory.CreateDbContextAsync(); - var entity = await db.Tasks.FirstOrDefaultAsync(t => t.Id == row.Id); - if (entity is null) return; + if (row is null || _worker is null) return; // Cascade to queued children when present — covers both planning parents // (PlanningPhase != None) and bare parents that have a manually-queued // chain. The X button's visibility is gated by the same condition // (HasQueuedSubtasks), so the handler matches what the user can see. - var queuedChildren = await db.Tasks - .Where(t => t.ParentTaskId == row.Id && t.Status == TaskStatus.Queued) - .ToListAsync(); - foreach (var c in queuedChildren) + List queuedChildIds; + await using (var db = await _dbFactory.CreateDbContextAsync()) { - c.Status = TaskStatus.Idle; - c.BlockedByTaskId = null; + queuedChildIds = await db.Tasks.AsNoTracking() + .Where(t => t.ParentTaskId == row.Id && t.Status == TaskStatus.Queued) + .Select(t => t.Id) + .ToListAsync(); } - if (entity.Status == TaskStatus.Queued) - entity.Status = TaskStatus.Idle; - await db.SaveChangesAsync(); - foreach (var c in queuedChildren) + var failures = new List(); + + foreach (var childId in queuedChildIds) { - var childRow = Items.FirstOrDefault(r => r.Id == c.Id); - if (childRow is not null) + var childRow = Items.FirstOrDefault(r => r.Id == childId); + if (childRow is null) continue; + var previousBlockedBy = childRow.BlockedByTaskId; + childRow.Status = TaskStatus.Idle; + childRow.BlockedByTaskId = null; + try { await _worker.DequeueTaskAsync(childId); } + catch (Exception ex) { - childRow.Status = TaskStatus.Idle; - childRow.BlockedByTaskId = null; + childRow.Status = TaskStatus.Queued; + childRow.BlockedByTaskId = previousBlockedBy; + failures.Add(ex.Message); } } + if (row.Status == TaskStatus.Queued) + { row.Status = TaskStatus.Idle; - row.HasQueuedSubtasks = false; + try { await _worker.DequeueTaskAsync(row.Id); } + catch (Exception ex) + { + row.Status = TaskStatus.Queued; + failures.Add(ex.Message); + } + } + + row.HasQueuedSubtasks = queuedChildIds + .Select(id => Items.FirstOrDefault(r => r.Id == id)) + .Any(r => r?.Status == TaskStatus.Queued); + + if (failures.Count > 0) + ErrorReported?.Invoke(Loc.T("vm.tasksIsland.removeFromQueueFailed", string.Join("; ", failures))); Regroup(); UpdateSubtitle(); diff --git a/src/ClaudeDo.Worker/CLAUDE.md b/src/ClaudeDo.Worker/CLAUDE.md index 1fc41ac1..b224009f 100644 --- a/src/ClaudeDo.Worker/CLAUDE.md +++ b/src/ClaudeDo.Worker/CLAUDE.md @@ -69,7 +69,7 @@ not conflated. Allowed transitions (enforced by `TaskStateService`): ``` -Idle → Queued | Running (RunNow) | Cancelled (external update_task_status only, allowFromIdle: true) +Idle → Queued | Running (RunNow) | Done (manual toggle) | Cancelled (external update_task_status only, allowFromIdle: true) Queued → Running | Cancelled | Idle | Failed (OverrideSlotService preflight gap) Running → WaitingForReview (standalone success, no children) | WaitingForChildren (parent with pending children) diff --git a/src/ClaudeDo.Worker/Hub/WorkerHub.cs b/src/ClaudeDo.Worker/Hub/WorkerHub.cs index 211f0457..4fb4fcde 100644 --- a/src/ClaudeDo.Worker/Hub/WorkerHub.cs +++ b/src/ClaudeDo.Worker/Hub/WorkerHub.cs @@ -686,16 +686,18 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub var sessionSkills = SkillsToJson(dto.SessionSkills); var verifyCommand = dto.VerifyCommand.NullIfBlank(); - if (model is null && systemPrompt is null && agentPath is null && dto.MaxTurns is null && sessionSkills is null && verifyCommand is null) + // Preserve SerializeOnFileOverlap: it has no UI/hub affordance yet (set via + // set_list_config or directly against ListConfigEntity), so a save from this path + // must not silently drop it -- neither by deleting the row nor by overwriting it. + var existing = await repo.GetConfigAsync(dto.ListId); + var hasUnrelatedSettings = existing?.SerializeOnFileOverlap ?? false; + + if (model is null && systemPrompt is null && agentPath is null && dto.MaxTurns is null && sessionSkills is null && verifyCommand is null && !hasUnrelatedSettings) { await repo.DeleteConfigAsync(dto.ListId); } else { - // Preserve SerializeOnFileOverlap: it has no UI/hub affordance yet (set via - // set_list_config or directly against ListConfigEntity), so a save from this path - // must not silently clear it. - var existing = await repo.GetConfigAsync(dto.ListId); await repo.SetConfigAsync(new ListConfigEntity { ListId = dto.ListId, @@ -735,6 +737,28 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub result.BaseDirty is { } w ? new BaseDirtyWarningDto(w.ModifiedCount, w.UntrackedCount) : null); } + // Guarded "manual done toggle" affordance (task list checkbox) -- checks the expected + // starting status server-side so a concurrent picker claim isn't silently overwritten. + public async Task SetTaskDone(string taskId) + { + var result = await _state.MarkDoneAsync(taskId, DateTime.UtcNow, Context.ConnectionAborted); + if (!result.Ok) throw new HubException(result.Reason ?? "mark done failed"); + } + + public async Task UnsetTaskDone(string taskId) + { + var result = await _state.UnmarkDoneAsync(taskId, Context.ConnectionAborted); + if (!result.Ok) throw new HubException(result.Reason ?? "unmark done failed"); + } + + // Guarded "remove from queue" affordance. Called once per row (parent, then each queued + // child) by the UI so a cascade never bypasses the same-status guard. + public async Task DequeueTask(string taskId) + { + var result = await _state.DequeueToIdleAsync(taskId, Context.ConnectionAborted); + if (!result.Ok) throw new HubException(result.Reason ?? "dequeue failed"); + } + public Task ApproveReview(string taskId, string targetBranch) => HubGuard(async () => { @@ -914,6 +938,11 @@ public sealed class WorkerHub : Microsoft.AspNetCore.SignalR.Hub throw new InvalidOperationException("Can't submit a running or queued task — interrupt it first."); if (task.Status is TaskStatus.WaitingForReview or TaskStatus.WaitingForChildren) throw new InvalidOperationException("Task is already awaiting review."); + // SubmitInteractiveForReviewAsync below only accepts Idle/Failed -- check it up front too, + // before either mutation branch, so a Done or Cancelled task can't get committed / have its + // HandlerHeadCommit stamped and then be rejected, stranding the work on an orphaned branch. + if (task.Status is not (TaskStatus.Idle or TaskStatus.Failed)) + throw new InvalidOperationException("Task must be Idle or Failed to submit for review."); var worktree = await new WorktreeRepository(ctx).GetByTaskIdAsync(taskId, Context.ConnectionAborted); if (worktree is not null) diff --git a/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs b/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs index 9e257ef1..22aa2d34 100644 --- a/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs +++ b/src/ClaudeDo.Worker/State/Interfaces/ITaskStateService.cs @@ -21,6 +21,16 @@ public interface ITaskStateService Task ForceSetStatusAsync(string taskId, ClaudeDo.Data.Models.TaskStatus status, CancellationToken ct); + // Guarded "manual done toggle" affordance (task list checkbox): unlike ForceSetStatusAsync, + // these check the expected starting status in the same ExecuteUpdateAsync filter so a + // concurrent picker claim (Idle/Done -> Running) is not silently overwritten. + Task MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct); + Task UnmarkDoneAsync(string taskId, CancellationToken ct); + + // Guarded "remove from queue" affordance. Expects the task to still be Queued; clears + // BlockedByTaskId in the same update so a queued chain child never ends up Idle-but-blocked. + Task DequeueToIdleAsync(string taskId, CancellationToken ct); + Task StartPlanningAsync(string parentId, CancellationToken ct); Task FinalizePlanningAsync(string parentId, CancellationToken ct); diff --git a/src/ClaudeDo.Worker/State/TaskStateService.cs b/src/ClaudeDo.Worker/State/TaskStateService.cs index 4040b2d1..0d473e82 100644 --- a/src/ClaudeDo.Worker/State/TaskStateService.cs +++ b/src/ClaudeDo.Worker/State/TaskStateService.cs @@ -379,6 +379,52 @@ public sealed class TaskStateService : ITaskStateService return new TransitionResult(true, null); } + public async Task MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct) + { + await using var ctx = await _dbFactory.CreateDbContextAsync(ct); + var affected = await ctx.Tasks + .Where(t => t.Id == taskId && t.Status == TaskStatus.Idle) + .ExecuteUpdateAsync(s => s + .SetProperty(t => t.Status, TaskStatus.Done) + .SetProperty(t => t.FinishedAt, finishedAt), ct); + + if (affected == 0) + return new TransitionResult(false, "Task is not Idle; cannot mark done."); + + await _broadcaster.TaskUpdated(taskId); + return new TransitionResult(true, null); + } + + public async Task UnmarkDoneAsync(string taskId, CancellationToken ct) + { + await using var ctx = await _dbFactory.CreateDbContextAsync(ct); + var affected = await ctx.Tasks + .Where(t => t.Id == taskId && t.Status == TaskStatus.Done) + .ExecuteUpdateAsync(s => s.SetProperty(t => t.Status, TaskStatus.Idle), ct); + + if (affected == 0) + return new TransitionResult(false, "Task is not Done; cannot unmark."); + + await _broadcaster.TaskUpdated(taskId); + return new TransitionResult(true, null); + } + + public async Task DequeueToIdleAsync(string taskId, CancellationToken ct) + { + await using var ctx = await _dbFactory.CreateDbContextAsync(ct); + var affected = await ctx.Tasks + .Where(t => t.Id == taskId && t.Status == TaskStatus.Queued) + .ExecuteUpdateAsync(s => s + .SetProperty(t => t.Status, TaskStatus.Idle) + .SetProperty(t => t.BlockedByTaskId, (string?)null), ct); + + if (affected == 0) + return new TransitionResult(false, "Task is not queued; cannot remove from queue."); + + await _broadcaster.TaskUpdated(taskId); + return new TransitionResult(true, null); + } + public async Task StartPlanningAsync(string parentId, CancellationToken ct) { await using var ctx = await _dbFactory.CreateDbContextAsync(ct); diff --git a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs index b2f9a8d5..148ddd29 100644 --- a/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs +++ b/tests/ClaudeDo.Ui.Tests/StubWorkerClient.cs @@ -98,6 +98,9 @@ public abstract class StubWorkerClient : IWorkerClient public virtual Task UpdateSessionSkillAsync(string sourceUrl) => Task.CompletedTask; public virtual Task RemoveSessionSkillAsync(string sourceUrl) => Task.CompletedTask; public virtual Task SetTaskStatusAsync(string taskId, TaskStatus status) => Task.FromResult(null); + public virtual Task SetTaskDoneAsync(string taskId) => Task.CompletedTask; + public virtual Task UnsetTaskDoneAsync(string taskId) => Task.CompletedTask; + public virtual Task DequeueTaskAsync(string taskId) => Task.CompletedTask; public virtual Task ApproveReviewAsync(string taskId, string targetBranch) => Task.FromResult(null); public virtual Task PreviewMergeAsync(string taskId, string targetBranch) => Task.FromResult(null); public virtual Task MergeTaskAsync(string taskId, string targetBranch, bool removeWorktree, string commitMessage) => Task.FromResult(new MergeResultDto("merged", System.Array.Empty(), null)); diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandErrorFeedbackTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandErrorFeedbackTests.cs index 99fb3bdc..d9148920 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandErrorFeedbackTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandErrorFeedbackTests.cs @@ -73,6 +73,8 @@ public class DetailsIslandErrorFeedbackTests : IDisposable public override bool IsConnected => Connected; public Exception? ThrowOnCancelTask; public Exception? ThrowOnSetTaskStatus; + public Exception? ThrowOnSetTaskDone; + public Exception? ThrowOnUnsetTaskDone; public override Task CancelTaskAsync(string taskId) { @@ -85,6 +87,18 @@ public class DetailsIslandErrorFeedbackTests : IDisposable if (ThrowOnSetTaskStatus is not null) throw ThrowOnSetTaskStatus; return Task.FromResult(null); } + + public override Task SetTaskDoneAsync(string taskId) + { + if (ThrowOnSetTaskDone is not null) throw ThrowOnSetTaskDone; + return Task.CompletedTask; + } + + public override Task UnsetTaskDoneAsync(string taskId) + { + if (ThrowOnUnsetTaskDone is not null) throw ThrowOnUnsetTaskDone; + return Task.CompletedTask; + } } private DetailsIslandViewModel BuildVm(StubWorkerClient worker) @@ -179,4 +193,60 @@ public class DetailsIslandErrorFeedbackTests : IDisposable Assert.NotNull(reportedError); Assert.Contains("reset offline", reportedError); } + + [Fact] + public async Task ToggleDone_MarkDone_WhenWorkerThrows_ReportsError_AndRevertsTask() + { + var worker = new ThrowingWorkerClient { ThrowOnSetTaskDone = new Exception("mark done offline") }; + var vm = BuildVm(worker); + vm.Bind(new TaskRowViewModel { Id = "task-toggle-done-throw", Status = TaskStatus.Idle, Done = false }); + vm.Monitor.ApplyState(TaskStatus.Idle); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + await vm.ToggleDoneCommand.ExecuteAsync(null); + + Assert.NotNull(reportedError); + Assert.Contains("mark done offline", reportedError); + Assert.False(vm.Task!.Done); + Assert.Equal(TaskStatus.Idle, vm.Task!.Status); + } + + [Fact] + public async Task ToggleDone_Untoggle_WhenWorkerThrows_ReportsError_AndRevertsTask() + { + var worker = new ThrowingWorkerClient { ThrowOnUnsetTaskDone = new Exception("unmark done offline") }; + var vm = BuildVm(worker); + vm.Bind(new TaskRowViewModel { Id = "task-untoggle-done-throw", Status = TaskStatus.Done, Done = true }); + vm.Monitor.ApplyState(TaskStatus.Done); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + await vm.ToggleDoneCommand.ExecuteAsync(null); + + Assert.NotNull(reportedError); + Assert.Contains("unmark done offline", reportedError); + Assert.True(vm.Task!.Done); + Assert.Equal(TaskStatus.Done, vm.Task!.Status); + } + + [Fact] + public async Task ToggleDone_MarkDone_WhenWorkerSucceeds_UpdatesTask_NoError() + { + var worker = new ThrowingWorkerClient(); + var vm = BuildVm(worker); + vm.Bind(new TaskRowViewModel { Id = "task-toggle-done-ok", Status = TaskStatus.Idle, Done = false }); + vm.Monitor.ApplyState(TaskStatus.Idle); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + await vm.ToggleDoneCommand.ExecuteAsync(null); + + Assert.Null(reportedError); + Assert.True(vm.Task!.Done); + Assert.Equal(TaskStatus.Done, vm.Task!.Status); + } } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandErrorFeedbackTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandErrorFeedbackTests.cs index 3e740b77..a8680f00 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandErrorFeedbackTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandErrorFeedbackTests.cs @@ -57,6 +57,10 @@ public class TasksIslandErrorFeedbackTests : IDisposable public Exception? ThrowOnCancelTask; public Exception? ThrowOnRejectToQueue; public Exception? ThrowOnRejectToIdle; + public Exception? ThrowOnSetTaskDone; + public Exception? ThrowOnUnsetTaskDone; + public Exception? ThrowOnDequeueTask; + public readonly List DequeuedTaskIds = new(); public override Task SetTaskStatusAsync(string taskId, TaskStatus status) { @@ -81,6 +85,25 @@ public class TasksIslandErrorFeedbackTests : IDisposable if (ThrowOnRejectToIdle is not null) throw ThrowOnRejectToIdle; return Task.CompletedTask; } + + public override Task SetTaskDoneAsync(string taskId) + { + if (ThrowOnSetTaskDone is not null) throw ThrowOnSetTaskDone; + return Task.CompletedTask; + } + + public override Task UnsetTaskDoneAsync(string taskId) + { + if (ThrowOnUnsetTaskDone is not null) throw ThrowOnUnsetTaskDone; + return Task.CompletedTask; + } + + public override Task DequeueTaskAsync(string taskId) + { + DequeuedTaskIds.Add(taskId); + if (ThrowOnDequeueTask is not null) throw ThrowOnDequeueTask; + return Task.CompletedTask; + } } [Fact] @@ -146,4 +169,113 @@ public class TasksIslandErrorFeedbackTests : IDisposable Assert.NotNull(reportedError); Assert.Contains("reject-to-idle offline", reportedError); } + + [Fact] + public async Task ToggleDone_MarkDone_WhenWorkerThrows_RaisesErrorReported_AndRevertsRow() + { + var worker = new ThrowingWorkerClient { ThrowOnSetTaskDone = new Exception("mark done offline") }; + var vm = new TasksIslandViewModel(new TestDbFactory(NewContext), worker); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + var row = new TaskRowViewModel { Id = "task-toggle-done-1", Status = TaskStatus.Idle, Done = false }; + await vm.ToggleDoneCommand.ExecuteAsync(row); + + Assert.NotNull(reportedError); + Assert.Contains("mark done offline", reportedError); + Assert.False(row.Done); + Assert.Equal(TaskStatus.Idle, row.Status); + } + + [Fact] + public async Task ToggleDone_Untoggle_WhenWorkerThrows_RaisesErrorReported_AndRevertsRow() + { + var worker = new ThrowingWorkerClient { ThrowOnUnsetTaskDone = new Exception("unmark done offline") }; + var vm = new TasksIslandViewModel(new TestDbFactory(NewContext), worker); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + var row = new TaskRowViewModel { Id = "task-toggle-done-2", Status = TaskStatus.Done, Done = true }; + await vm.ToggleDoneCommand.ExecuteAsync(row); + + Assert.NotNull(reportedError); + Assert.Contains("unmark done offline", reportedError); + Assert.True(row.Done); + Assert.Equal(TaskStatus.Done, row.Status); + } + + [Fact] + public async Task ToggleDone_MarkDone_WhenWorkerSucceeds_UpdatesRow_NoError() + { + var worker = new ThrowingWorkerClient(); + var vm = new TasksIslandViewModel(new TestDbFactory(NewContext), worker); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + var row = new TaskRowViewModel { Id = "task-toggle-done-3", Status = TaskStatus.Idle, Done = false }; + await vm.ToggleDoneCommand.ExecuteAsync(row); + + Assert.Null(reportedError); + Assert.True(row.Done); + Assert.Equal(TaskStatus.Done, row.Status); + } + + [Fact] + public async Task RemoveFromQueue_WhenWorkerThrows_RaisesErrorReported_AndRevertsRow() + { + var worker = new ThrowingWorkerClient { ThrowOnDequeueTask = new Exception("dequeue offline") }; + var vm = new TasksIslandViewModel(new TestDbFactory(NewContext), worker); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + var row = new TaskRowViewModel { Id = "task-dequeue-1", Status = TaskStatus.Queued }; + await vm.RemoveFromQueueCommand.ExecuteAsync(row); + + Assert.NotNull(reportedError); + Assert.Contains("dequeue offline", reportedError); + Assert.Equal(TaskStatus.Queued, row.Status); + } + + [Fact] + public async Task RemoveFromQueue_CascadesToQueuedChildren_ViaGuardedHubCall() + { + var listId = Guid.NewGuid().ToString(); + var parentId = Guid.NewGuid().ToString(); + var childId = Guid.NewGuid().ToString(); + await using (var db = NewContext()) + { + db.Lists.Add(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow }); + db.Tasks.Add(new TaskEntity + { + Id = parentId, ListId = listId, Title = "parent", Status = TaskStatus.Queued, + Number = 1, CreatedAt = DateTime.UtcNow, + }); + db.Tasks.Add(new TaskEntity + { + Id = childId, ListId = listId, Title = "child", Status = TaskStatus.Queued, + Number = 2, ParentTaskId = parentId, BlockedByTaskId = parentId, CreatedAt = DateTime.UtcNow, + }); + await db.SaveChangesAsync(); + } + + var worker = new ThrowingWorkerClient(); + var vm = new TasksIslandViewModel(new TestDbFactory(NewContext), worker); + var parentRow = new TaskRowViewModel { Id = parentId, Status = TaskStatus.Queued, HasQueuedSubtasks = true }; + var childRow = new TaskRowViewModel { Id = childId, ParentTaskId = parentId, Status = TaskStatus.Queued, BlockedByTaskId = parentId }; + vm.Items.Add(parentRow); + vm.Items.Add(childRow); + + await vm.RemoveFromQueueCommand.ExecuteAsync(parentRow); + + Assert.Contains(childId, worker.DequeuedTaskIds); + Assert.Contains(parentId, worker.DequeuedTaskIds); + Assert.Equal(TaskStatus.Idle, childRow.Status); + Assert.Null(childRow.BlockedByTaskId); + Assert.Equal(TaskStatus.Idle, parentRow.Status); + Assert.False(parentRow.HasQueuedSubtasks); + } } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandRemoveFromQueueTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandRemoveFromQueueTests.cs index 63432720..21a26352 100644 --- a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandRemoveFromQueueTests.cs +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandRemoveFromQueueTests.cs @@ -1,5 +1,6 @@ using ClaudeDo.Data; using ClaudeDo.Data.Models; +using ClaudeDo.Ui.Services; using ClaudeDo.Ui.ViewModels.Islands; using Microsoft.EntityFrameworkCore; using TaskStatus = ClaudeDo.Data.Models.TaskStatus; @@ -39,8 +40,31 @@ public class TasksIslandRemoveFromQueueTests : IDisposable public ClaudeDoDbContext CreateDbContext() => _create(); } - private TasksIslandViewModel BuildViewModel() => - new(new TestDbFactory(NewContext), worker: null); + // RemoveFromQueueAsync now routes the dequeue through IWorkerClient.DequeueTaskAsync (the + // guarded TaskStateService transition) instead of writing the DB directly. This fake performs + // the same write the real hub would, against the same DB, so these tests still exercise the + // ViewModel's cascade/read logic without needing a live worker. + private sealed class DequeuingWorkerClient : StubWorkerClient + { + private readonly IDbContextFactory _dbFactory; + public DequeuingWorkerClient(IDbContextFactory dbFactory) => _dbFactory = dbFactory; + + public override async Task DequeueTaskAsync(string taskId) + { + await using var db = await _dbFactory.CreateDbContextAsync(); + var entity = await db.Tasks.FirstOrDefaultAsync(t => t.Id == taskId && t.Status == TaskStatus.Queued); + if (entity is null) return; + entity.Status = TaskStatus.Idle; + entity.BlockedByTaskId = null; + await db.SaveChangesAsync(); + } + } + + private TasksIslandViewModel BuildViewModel() + { + var factory = new TestDbFactory(NewContext); + return new(factory, worker: new DequeuingWorkerClient(factory)); + } private async Task SeedParentWithChainAsync( string parentId, diff --git a/tests/ClaudeDo.Worker.Tests/Hub/ListConfigHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/ListConfigHubTests.cs new file mode 100644 index 00000000..4980e8c8 --- /dev/null +++ b/tests/ClaudeDo.Worker.Tests/Hub/ListConfigHubTests.cs @@ -0,0 +1,101 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Data.Repositories; +using ClaudeDo.Worker.Hub; +using ClaudeDo.Worker.Tests.Infrastructure; +using Xunit; + +namespace ClaudeDo.Worker.Tests.Hub; + +/// UpdateListConfig's "all fields blank -> delete the row" branch used to delete unconditionally, +/// silently dropping SerializeOnFileOverlap -- a flag with no UI/hub affordance of its own (set +/// only via set_list_config or directly against ListConfigEntity). +public sealed class ListConfigHubTests : IDisposable +{ + private readonly DbFixture _db = new(); + + public void Dispose() => _db.Dispose(); + + private WorkerHub CreateHub() + { + var factory = _db.CreateFactory(); + var broadcaster = new HubBroadcaster(new CapturingHubContext()); + var hub = new WorkerHub( + null!, null!, null!, null!, broadcaster, factory, + null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, null!, + null!, new ClaudeDo.Worker.Online.OnlineInboxConfig(), new ClaudeDo.Worker.Online.OnlineTokenStore(), + new ClaudeDo.Worker.Runner.PendingQuestionRegistry(), null!); + hub.Clients = new FakeHubCallerClients(new RecordingClientProxy()); + hub.Context = new FakeHubCallerContext(); + return hub; + } + + // Each helper opens (and disposes) its own short-lived context/repository -- ListRepository's + // GetConfigAsync doesn't AsNoTracking(), so reusing one long-lived instance across a hub call + // that writes via a *different* context would return a stale, identity-mapped entity. + + private async Task SeedListAsync() + { + var listId = Guid.NewGuid().ToString(); + await using var ctx = _db.CreateContext(); + await new ListRepository(ctx).AddAsync(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow }); + return listId; + } + + private async Task SeedConfigAsync(string listId, string? model = null, bool serializeOnFileOverlap = false) + { + await using var ctx = _db.CreateContext(); + await new ListRepository(ctx).SetConfigAsync(new ListConfigEntity + { + ListId = listId, Model = model, SerializeOnFileOverlap = serializeOnFileOverlap, + }); + } + + private async Task GetConfigAsync(string listId) + { + await using var ctx = _db.CreateContext(); + return await new ListRepository(ctx).GetConfigAsync(listId); + } + + [Fact] + public async Task UpdateListConfig_AllBlank_NoUnrelatedSettings_DeletesRow() + { + var hub = CreateHub(); + var listId = await SeedListAsync(); + await SeedConfigAsync(listId, model: "opus"); + + await hub.UpdateListConfig(new UpdateListConfigDto(listId, null, null, null)); + + Assert.Null(await GetConfigAsync(listId)); + } + + [Fact] + public async Task UpdateListConfig_AllBlank_WithSerializeOnFileOverlap_KeepsFlag_RowSurvives() + { + var hub = CreateHub(); + var listId = await SeedListAsync(); + await SeedConfigAsync(listId, model: "opus", serializeOnFileOverlap: true); + + await hub.UpdateListConfig(new UpdateListConfigDto(listId, null, null, null)); + + var config = await GetConfigAsync(listId); + Assert.NotNull(config); + Assert.True(config!.SerializeOnFileOverlap); + Assert.Null(config.Model); + } + + [Fact] + public async Task UpdateListConfig_WithModel_UpsertsNormally_PreservesSerializeOnFileOverlap() + { + var hub = CreateHub(); + var listId = await SeedListAsync(); + await SeedConfigAsync(listId, serializeOnFileOverlap: true); + + await hub.UpdateListConfig(new UpdateListConfigDto(listId, "opus", null, null)); + + var config = await GetConfigAsync(listId); + Assert.NotNull(config); + Assert.Equal("opus", config!.Model); + Assert.True(config.SerializeOnFileOverlap); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/Hub/MergeHelperTaskHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/MergeHelperTaskHubTests.cs index 2114f562..09d9bdf8 100644 --- a/tests/ClaudeDo.Worker.Tests/Hub/MergeHelperTaskHubTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Hub/MergeHelperTaskHubTests.cs @@ -214,6 +214,51 @@ public sealed class MergeHelperTaskHubTests : IDisposable var hub = CreateHub(); await Assert.ThrowsAsync(() => hub.SubmitTaskForReview(task.Id)); } + + // The status gate (Idle or Failed) must run before either mutation branch below, so a + // Done/Cancelled task can never get its worktree committed or its HandlerHeadCommit stamped + // and then rejected by SubmitInteractiveForReviewAsync, stranding the work. + + [Fact] + public async Task SubmitTaskForReview_DoneTask_WorktreePath_ThrowsBeforeCommit_WorktreeUntouched() + { + var listId = await SeedListAsync(Path.GetTempPath()); + var task = await SeedTaskAsync(listId, TaskStatus.Done); + var worktree = new WorktreeEntity + { + TaskId = task.Id, + Path = Path.Combine(Path.GetTempPath(), $"cd_no_such_worktree_{Guid.NewGuid():N}"), + BranchName = "claudedo/does-not-matter", + BaseCommit = "abc123", + State = WorktreeState.Active, + CreatedAt = DateTime.UtcNow, + }; + await new WorktreeRepository(_ctx).AddAsync(worktree); + + var hub = CreateHub(); + var ex = await Assert.ThrowsAsync(() => hub.SubmitTaskForReview(task.Id)); + Assert.Contains("Idle or Failed", ex.Message); + + var reloadedWorktree = await new WorktreeRepository(_ctx).GetByTaskIdAsync(task.Id); + Assert.Null(reloadedWorktree!.HeadCommit); + var reloadedTask = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Done, reloadedTask!.Status); + } + + [Fact] + public async Task SubmitTaskForReview_CancelledTask_HandlerPath_ThrowsBeforeStamp_HandlerHeadCommitUntouched() + { + var listId = await SeedListAsync(Path.GetTempPath()); + var task = await SeedTaskAsync(listId, TaskStatus.Cancelled, handlerBaseCommit: "abc123"); + + var hub = CreateHub(); + var ex = await Assert.ThrowsAsync(() => hub.SubmitTaskForReview(task.Id)); + Assert.Contains("Idle or Failed", ex.Message); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Null(reloaded!.HandlerHeadCommit); + Assert.Equal(TaskStatus.Cancelled, reloaded.Status); + } } // RecordingClientProxy / FakeHubCallerClients / FakeHubCallerContext are defined once for the diff --git a/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs b/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs new file mode 100644 index 00000000..3a169773 --- /dev/null +++ b/tests/ClaudeDo.Worker.Tests/Hub/TaskDoneDequeueHubTests.cs @@ -0,0 +1,160 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Data.Repositories; +using ClaudeDo.Worker.Hub; +using ClaudeDo.Worker.Tests.Infrastructure; +using Microsoft.AspNetCore.SignalR; +using Xunit; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Worker.Tests.Hub; + +/// Covers the guarded "manual done toggle" (SetTaskDone/UnsetTaskDone) and "remove from queue" +/// (DequeueTask) hub methods added to replace the UI's raw EF writes -- each checks the expected +/// starting status server-side so a concurrent picker claim can't be silently overwritten. +public sealed class TaskDoneDequeueHubTests : IDisposable +{ + private readonly DbFixture _db = new(); + private readonly ClaudeDoDbContext _ctx; + private readonly TaskRepository _tasks; + private readonly ListRepository _lists; + private readonly RecordingClientProxy _proxy = new(); + + public TaskDoneDequeueHubTests() + { + _ctx = _db.CreateContext(); + _tasks = new TaskRepository(_ctx); + _lists = new ListRepository(_ctx); + } + + public void Dispose() + { + _ctx.Dispose(); + _db.Dispose(); + } + + private WorkerHub CreateHub() + { + var factory = _db.CreateFactory(); + var built = TaskStateServiceBuilder.Build(factory); + var hub = new WorkerHub( + null!, null!, null!, null!, null!, factory, null!, null!, null!, + null!, null!, null!, null!, null!, null!, null!, built.State, null!, null!, + null!, new ClaudeDo.Worker.Online.OnlineInboxConfig(), new ClaudeDo.Worker.Online.OnlineTokenStore(), + new ClaudeDo.Worker.Runner.PendingQuestionRegistry(), null!); + hub.Clients = new FakeHubCallerClients(_proxy); + hub.Context = new FakeHubCallerContext(); + return hub; + } + + private async Task SeedListAsync() + { + var listId = Guid.NewGuid().ToString(); + await _lists.AddAsync(new ListEntity { Id = listId, Name = "L", CreatedAt = DateTime.UtcNow }); + return listId; + } + + private async Task SeedTaskAsync( + string listId, TaskStatus status, string? blockedByTaskId = null, string title = "T") + { + var task = new TaskEntity + { + Id = Guid.NewGuid().ToString(), + ListId = listId, + Title = title, + Status = status, + BlockedByTaskId = blockedByTaskId, + CreatedAt = DateTime.UtcNow, + }; + await _tasks.AddAsync(task); + return task; + } + + // ── SetTaskDone ── + + [Fact] + public async Task SetTaskDone_FromIdle_TransitionsToDone() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.Idle); + + var hub = CreateHub(); + await hub.SetTaskDone(task.Id); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Done, reloaded!.Status); + } + + [Fact] + public async Task SetTaskDone_FromRunning_Throws_AndLeavesStatusUnchanged() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.Running); + + var hub = CreateHub(); + await Assert.ThrowsAsync(() => hub.SetTaskDone(task.Id)); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Running, reloaded!.Status); + } + + // ── UnsetTaskDone ── + + [Fact] + public async Task UnsetTaskDone_FromDone_TransitionsToIdle() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.Done); + + var hub = CreateHub(); + await hub.UnsetTaskDone(task.Id); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Idle, reloaded!.Status); + } + + [Fact] + public async Task UnsetTaskDone_FromRunning_Throws_AndLeavesStatusUnchanged() + { + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.Running); + + var hub = CreateHub(); + await Assert.ThrowsAsync(() => hub.UnsetTaskDone(task.Id)); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Running, reloaded!.Status); + } + + // ── DequeueTask ── + + [Fact] + public async Task DequeueTask_FromQueued_TransitionsToIdle_AndClearsBlockedByTaskId() + { + var listId = await SeedListAsync(); + var pred = await SeedTaskAsync(listId, TaskStatus.Queued); + var task = await SeedTaskAsync(listId, TaskStatus.Queued, blockedByTaskId: pred.Id); + + var hub = CreateHub(); + await hub.DequeueTask(task.Id); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Idle, reloaded!.Status); + Assert.Null(reloaded.BlockedByTaskId); + } + + [Fact] + public async Task DequeueTask_FromRunning_Throws_AndLeavesStatusUnchanged() + { + // Simulates the picker having already claimed the task between the UI reading its + // row and the dequeue call landing -- the DB status must not be clobbered. + var listId = await SeedListAsync(); + var task = await SeedTaskAsync(listId, TaskStatus.Running); + + var hub = CreateHub(); + await Assert.ThrowsAsync(() => hub.DequeueTask(task.Id)); + + var reloaded = await _tasks.GetByIdAsync(task.Id); + Assert.Equal(TaskStatus.Running, reloaded!.Status); + } +} diff --git a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs index 216a1f82..8a25f8c3 100644 --- a/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Planning/PlanningMergeOrchestratorTests.cs @@ -871,6 +871,9 @@ file sealed class ApproveObservingTaskStateService : ITaskStateService public Task RejectToIdleAsync(string taskId, CancellationToken ct) => _inner.RejectToIdleAsync(taskId, ct); public Task ClearReviewFeedbackAsync(string taskId, CancellationToken ct) => _inner.ClearReviewFeedbackAsync(taskId, ct); public Task ForceSetStatusAsync(string taskId, TaskStatus status, CancellationToken ct) => _inner.ForceSetStatusAsync(taskId, status, ct); + public Task MarkDoneAsync(string taskId, DateTime finishedAt, CancellationToken ct) => _inner.MarkDoneAsync(taskId, finishedAt, ct); + public Task UnmarkDoneAsync(string taskId, CancellationToken ct) => _inner.UnmarkDoneAsync(taskId, ct); + public Task DequeueToIdleAsync(string taskId, CancellationToken ct) => _inner.DequeueToIdleAsync(taskId, ct); public Task StartPlanningAsync(string parentId, CancellationToken ct) => _inner.StartPlanningAsync(parentId, ct); public Task FinalizePlanningAsync(string parentId, CancellationToken ct) => _inner.FinalizePlanningAsync(parentId, ct); public Task BlockOnAsync(string taskId, string predecessorTaskId, CancellationToken ct) => _inner.BlockOnAsync(taskId, predecessorTaskId, ct); diff --git a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs index da7886c9..2909f490 100644 --- a/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/State/TaskStateServiceTests.cs @@ -477,6 +477,84 @@ public sealed class TaskStateServiceTests : IDisposable Assert.Equal(TaskStatus.Running, await GetStatusAsync(id)); } + // ─── MarkDoneAsync / UnmarkDoneAsync ───────────────────────────────── + + [Fact] + public async Task MarkDoneAsync_FromIdle_TransitionsToDone_AndStampsFinishedAt() + { + var id = await SeedTaskAsync(TaskStatus.Idle); + var now = DateTime.UtcNow; + + var result = await _sut.MarkDoneAsync(id, now, default); + + Assert.True(result.Ok); + var t = await GetTaskAsync(id); + Assert.Equal(TaskStatus.Done, t.Status); + Assert.Equal(now, t.FinishedAt); + } + + [Fact] + public async Task MarkDoneAsync_FromRunning_Rejects_AndLeavesStatusUnchanged() + { + var id = await SeedTaskAsync(TaskStatus.Running); + + var result = await _sut.MarkDoneAsync(id, DateTime.UtcNow, default); + + Assert.False(result.Ok); + Assert.Equal(TaskStatus.Running, await GetStatusAsync(id)); + } + + [Fact] + public async Task UnmarkDoneAsync_FromDone_TransitionsToIdle() + { + var id = await SeedTaskAsync(TaskStatus.Done); + + var result = await _sut.UnmarkDoneAsync(id, default); + + Assert.True(result.Ok); + Assert.Equal(TaskStatus.Idle, await GetStatusAsync(id)); + } + + [Fact] + public async Task UnmarkDoneAsync_FromRunning_Rejects_AndLeavesStatusUnchanged() + { + var id = await SeedTaskAsync(TaskStatus.Running); + + var result = await _sut.UnmarkDoneAsync(id, default); + + Assert.False(result.Ok); + Assert.Equal(TaskStatus.Running, await GetStatusAsync(id)); + } + + // ─── DequeueToIdleAsync ─────────────────────────────────────────────── + + [Fact] + public async Task DequeueToIdleAsync_FromQueued_TransitionsToIdle_AndClearsBlockedByTaskId() + { + var pred = await SeedTaskAsync(TaskStatus.Queued); + var id = await SeedTaskAsync(TaskStatus.Queued, blockedBy: pred); + + var result = await _sut.DequeueToIdleAsync(id, default); + + Assert.True(result.Ok); + var t = await GetTaskAsync(id); + Assert.Equal(TaskStatus.Idle, t.Status); + Assert.Null(t.BlockedByTaskId); + } + + [Fact] + public async Task DequeueToIdleAsync_FromRunning_Rejects_AndLeavesStatusUnchanged() + { + // Simulates the picker having already claimed the task between the UI reading + // its row and the dequeue call landing. + var id = await SeedTaskAsync(TaskStatus.Running); + + var result = await _sut.DequeueToIdleAsync(id, default); + + Assert.False(result.Ok); + Assert.Equal(TaskStatus.Running, await GetStatusAsync(id)); + } + // ─── StartPlanningAsync ─────────────────────────────────────────────── [Fact] diff --git a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs index 3be0098b..9a180cf5 100644 --- a/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs +++ b/tests/ClaudeDo.Worker.Tests/UiVm/TasksIslandViewModelPlanningTests.cs @@ -65,6 +65,9 @@ sealed class FakeWorkerClient : IWorkerClient SetTaskStatusCalls.Add((taskId, status)); return Task.FromResult(null); } + public Task SetTaskDoneAsync(string taskId) => Task.CompletedTask; + public Task UnsetTaskDoneAsync(string taskId) => Task.CompletedTask; + public Task DequeueTaskAsync(string taskId) => Task.CompletedTask; public Task ApproveReviewAsync(string taskId, string targetBranch) => Task.FromResult(null); public Task PreviewMergeAsync(string taskId, string targetBranch) => Task.FromResult(null); public Task MergeTaskAsync(string taskId, string targetBranch, bool removeWorktree, string commitMessage) => Task.FromResult(new MergeResultDto("merged", System.Array.Empty(), null));