From 166021049a8f9303f4a59b2c60f627eea0bf2c34 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 14:30:24 +0200 Subject: [PATCH] fix(ui): close interactive-session gate bypasses in reset-and-retry and plan queueing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reset & Retry discarded the branch and queued an autonomous run even while the user had an interactive ConPTY pane open on the task, and finalizing a plan queued every child unconditionally (the hub has no notion of a UI-hosted session) — both bypassed the HasInteractiveSession gate added for CanSendToQueue. CanResetAndRetry now checks it too, with a subscription on the bound task so the command re-evaluates when the flag flips without Task itself changing; SendToQueueAsync now blocks queuing the whole plan and surfaces the affected child titles when any child has an open session. --- src/ClaudeDo.Localization/locales/de.json | 2 +- src/ClaudeDo.Localization/locales/en.json | 2 +- .../Islands/DetailsIslandViewModel.cs | 26 +++- .../Islands/TasksIslandViewModel.cs | 13 ++ .../DetailsIslandResetAndRetryTests.cs | 136 ++++++++++++++++++ ...asksIslandQueuePlanInteractiveGateTests.cs | 129 +++++++++++++++++ 6 files changed, 305 insertions(+), 3 deletions(-) create mode 100644 tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandResetAndRetryTests.cs create mode 100644 tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandQueuePlanInteractiveGateTests.cs diff --git a/src/ClaudeDo.Localization/locales/de.json b/src/ClaudeDo.Localization/locales/de.json index dd39480e..ed7768e5 100644 --- a/src/ClaudeDo.Localization/locales/de.json +++ b/src/ClaudeDo.Localization/locales/de.json @@ -647,7 +647,7 @@ "taskStatus": { "idle": "Leerlauf", "queued": "In Warteschlange", "running": "Läuft", "waitingForReview": "Wartet auf Prüfung", "waitingForChildren": "Wartet auf Teilaufgaben", "done": "Fertig", "failed": "Fehlgeschlagen", "cancelled": "Abgebrochen", "parked": "Geparkt", "interactive": "Interaktiv" }, "planningBadge": { "active": "PLANUNG", "finalized": "GEPLANT" }, "taskRow": { "createdPrefix": "Erstellt {0}", "stepsText": "{0}/{1} Schritte" }, - "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}", "sendToQueueFailed": "In die Warteschlange stellen fehlgeschlagen: {0}", "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}" }, + "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}", "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}" }, "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": "Merge-Aufgabe: {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.", "unknownStatus": "Unbekannter Status: {0}", "mergeFailed": "Merge fehlgeschlagen: {0}" }, diff --git a/src/ClaudeDo.Localization/locales/en.json b/src/ClaudeDo.Localization/locales/en.json index d63f7474..486944ca 100644 --- a/src/ClaudeDo.Localization/locales/en.json +++ b/src/ClaudeDo.Localization/locales/en.json @@ -647,7 +647,7 @@ "taskStatus": { "idle": "Idle", "queued": "Queued", "running": "Running", "waitingForReview": "Waiting for Review", "waitingForChildren": "Waiting for Subtasks", "done": "Done", "failed": "Failed", "cancelled": "Cancelled", "parked": "Parked", "interactive": "Interactive" }, "planningBadge": { "active": "PLANNING", "finalized": "PLANNED" }, "taskRow": { "createdPrefix": "Created {0}", "stepsText": "{0}/{1} steps" }, - "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}", "sendToQueueFailed": "Send to queue failed: {0}", "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}" }, + "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}", "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}" }, "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": "Merge task: {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.", "unknownStatus": "Unknown status: {0}", "mergeFailed": "Merge failed: {0}" }, diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 3b155974..b08a15e8 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -423,6 +423,8 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable public void Dispose() { + if (_subscribedTask is not null) + _subscribedTask.PropertyChanged -= OnBoundTaskPropertyChanged; Monitor.PropertyChanged -= OnMonitorPropertyChanged; Monitor.Dispose(); Loc.LanguageChanged -= _langChangedHandler; @@ -907,14 +909,33 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable Merge.SyncWorktree(WorktreePath, WorktreeBaseCommit, WorktreeHeadCommit, WorktreeStateLabel, _listWorkingDir); + // Tracks whichever row we last subscribed to, so a later Task switch can unsubscribe the old + // one cleanly even though the generated OnTaskChanged only hands us the new value. + private TaskRowViewModel? _subscribedTask; + partial void OnTaskChanged(TaskRowViewModel? value) { + if (_subscribedTask is not null) + _subscribedTask.PropertyChanged -= OnBoundTaskPropertyChanged; + _subscribedTask = value; + if (value is not null) + value.PropertyChanged += OnBoundTaskPropertyChanged; + ReviewDiffViewed = false; Merge.SyncTaskContext(Task?.Id, Task?.Title, Task?.IsPlanningParent == true); NotifySessionSections(); OnPropertyChanged(nameof(CanAcceptDrop)); } + // The bound row's HasInteractiveSession can flip from outside (Mission Control opening/closing + // a ConPTY pane) without Task itself changing, so ResetAndRetryCommand needs its own listener + // to stay in sync with the gate in CanResetAndRetry. + private void OnBoundTaskPropertyChanged(object? sender, System.ComponentModel.PropertyChangedEventArgs e) + { + if (e.PropertyName == nameof(TaskRowViewModel.HasInteractiveSession)) + ResetAndRetryCommand.NotifyCanExecuteChanged(); + } + [RelayCommand] private void CloseDetails() => CloseDetail?.Invoke(); @@ -1133,8 +1154,11 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable catch { /* offline */ } } + // Reset & Retry discards the branch/uncommitted work and queues an autonomous run into the + // same worktree — must stay off while the user is hand-editing it in an interactive ConPTY + // pane, same reasoning as TaskRowViewModel.CanSendToQueue. private bool CanResetAndRetry() => - Task != null && _worker.IsConnected && ShowResetAndRetry; + Task != null && _worker.IsConnected && ShowResetAndRetry && !Task.HasInteractiveSession; // Set once the user opens the diff/combined-diff for the current review. Reset on // task switch and on every state change (a new run means a new diff to read), so diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs index 42035039..f2a7d0cc 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/TasksIslandViewModel.cs @@ -837,6 +837,19 @@ public sealed partial class TasksIslandViewModel : ViewModelBase, IDisposable // A finalized planning parent queues its plan (children sequentially), not itself. if (row.CanQueuePlan) { + // The hub's QueuePlanningSubtasksAsync queues every Idle child unconditionally — it has + // no notion of a UI-hosted ConPTY session. Block the whole plan if any child has one open, + // otherwise that child's worktree would get an autonomous run racing the user's own edits. + var interactiveChildren = Items + .Where(r => r.ParentTaskId == row.Id && r.HasInteractiveSession) + .Select(r => r.Title) + .ToList(); + if (interactiveChildren.Count > 0) + { + ErrorReported?.Invoke(Loc.T( + "vm.tasksIsland.queuePlanBlockedInteractive", string.Join(", ", interactiveChildren))); + return; + } await QueuePlanningSubtasksAsync(row); return; } diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandResetAndRetryTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandResetAndRetryTests.cs new file mode 100644 index 00000000..66705855 --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/DetailsIslandResetAndRetryTests.cs @@ -0,0 +1,136 @@ +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; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// Covers the Reset & Retry gate: it discards the branch/uncommitted work and queues an +// autonomous run into the same worktree, so it must stay off while an interactive ConPTY +// pane is open on the task (mirrors TaskRowViewModel.CanSendToQueue's HasInteractiveSession gate). +public class DetailsIslandResetAndRetryTests : IDisposable +{ + private readonly string _dbPath; + + public DetailsIslandResetAndRetryTests() + { + _dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_reset_retry_test_{Guid.NewGuid():N}.db"); + using var ctx = NewContext(); + ctx.Database.EnsureCreated(); + } + + public void Dispose() + { + try { File.Delete(_dbPath); } catch { } + try { File.Delete(_dbPath + "-wal"); } catch { } + try { File.Delete(_dbPath + "-shm"); } catch { } + } + + private ClaudeDoDbContext NewContext() + { + var opts = new DbContextOptionsBuilder() + .UseSqlite($"Data Source={_dbPath}") + .Options; + return new ClaudeDoDbContext(opts); + } + + private sealed class TestDbFactory : IDbContextFactory + { + private readonly Func _create; + public TestDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + } + + private sealed class NullServiceProvider : IServiceProvider + { + public object? GetService(Type serviceType) => null; + } + + private sealed class StubNotesApi : ClaudeDo.Ui.Services.Interfaces.INotesApi + { + public Task> ListAsync(DateOnly day) => + Task.FromResult(new List()); + public Task AddAsync(DateOnly day, string text) => + Task.FromResult(null); + public Task UpdateAsync(string id, string text) => Task.CompletedTask; + public Task DeleteAsync(string id) => Task.CompletedTask; + } + + private sealed class RecordingWorkerClient : StubWorkerClient + { + public override bool IsConnected => true; + } + + private DetailsIslandViewModel BuildVm(StubWorkerClient worker) + { + var factory = new TestDbFactory(NewContext); + return new DetailsIslandViewModel( + factory, worker, new NullServiceProvider(), new StubNotesApi(), new ClaudeDo.Ui.Services.MergeCoordinator()); + } + + [Fact] + public void ResetAndRetry_IsDisabled_WhileTaskHasInteractiveSession() + { + var vm = BuildVm(new RecordingWorkerClient()); + var row = new TaskRowViewModel { Id = "task-reset-1", Status = TaskStatus.Failed, HasInteractiveSession = true }; + vm.Bind(row); + vm.Monitor.ApplyState(TaskStatus.Failed); + + Assert.True(vm.ShowResetAndRetry); + Assert.False(vm.ResetAndRetryCommand.CanExecute(null)); + } + + [Fact] + public void ResetAndRetry_ReEnables_WhenInteractiveSessionCloses() + { + var vm = BuildVm(new RecordingWorkerClient()); + var row = new TaskRowViewModel { Id = "task-reset-2", Status = TaskStatus.Failed, HasInteractiveSession = true }; + vm.Bind(row); + vm.Monitor.ApplyState(TaskStatus.Failed); + + Assert.False(vm.ResetAndRetryCommand.CanExecute(null)); + + var canExecuteChangedRaised = false; + vm.ResetAndRetryCommand.CanExecuteChanged += (_, _) => canExecuteChangedRaised = true; + + // The row is the same instance Mission Control flips when a ConPTY pane closes — + // the command's CanExecute must react without the bound Task itself changing. + row.HasInteractiveSession = false; + + Assert.True(canExecuteChangedRaised); + Assert.True(vm.ResetAndRetryCommand.CanExecute(null)); + } + + [Fact] + public void ResetAndRetry_IsEnabled_ForTerminalTaskWithoutInteractiveSession() + { + var vm = BuildVm(new RecordingWorkerClient()); + var row = new TaskRowViewModel { Id = "task-reset-3", Status = TaskStatus.Cancelled }; + vm.Bind(row); + vm.Monitor.ApplyState(TaskStatus.Cancelled); + + Assert.True(vm.ResetAndRetryCommand.CanExecute(null)); + } + + [Fact] + public void ResetAndRetry_StaysDisabled_AfterSwitchingToAnotherInteractiveTask() + { + var vm = BuildVm(new RecordingWorkerClient()); + var firstRow = new TaskRowViewModel { Id = "task-reset-4", Status = TaskStatus.Failed }; + vm.Bind(firstRow); + vm.Monitor.ApplyState(TaskStatus.Failed); + Assert.True(vm.ResetAndRetryCommand.CanExecute(null)); + + var secondRow = new TaskRowViewModel { Id = "task-reset-5", Status = TaskStatus.Failed, HasInteractiveSession = true }; + vm.Bind(secondRow); + vm.Monitor.ApplyState(TaskStatus.Failed); + + Assert.False(vm.ResetAndRetryCommand.CanExecute(null)); + + // Flipping the old (unsubscribed) row must not resurrect the command for the new task. + firstRow.HasInteractiveSession = true; + Assert.False(vm.ResetAndRetryCommand.CanExecute(null)); + } +} diff --git a/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandQueuePlanInteractiveGateTests.cs b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandQueuePlanInteractiveGateTests.cs new file mode 100644 index 00000000..da5e5967 --- /dev/null +++ b/tests/ClaudeDo.Ui.Tests/ViewModels/TasksIslandQueuePlanInteractiveGateTests.cs @@ -0,0 +1,129 @@ +using ClaudeDo.Data; +using ClaudeDo.Data.Models; +using ClaudeDo.Ui.ViewModels.Islands; +using Microsoft.EntityFrameworkCore; +using TaskStatus = ClaudeDo.Data.Models.TaskStatus; + +namespace ClaudeDo.Ui.Tests.ViewModels; + +// The hub's QueuePlanningSubtasksAsync queues every Idle child of a finalized planning parent +// unconditionally — it has no notion of a UI-hosted ConPTY session. SendToQueueAsync must block +// the whole plan when any child has an open interactive session, otherwise that child's worktree +// gets an autonomous run racing the user's own hand-driven edits. +public class TasksIslandQueuePlanInteractiveGateTests : IDisposable +{ + private readonly string _dbPath; + + public TasksIslandQueuePlanInteractiveGateTests() + { + _dbPath = Path.Combine(Path.GetTempPath(), $"claudedo_queueplan_gate_{Guid.NewGuid():N}.db"); + using var ctx = NewContext(); + ctx.Database.EnsureCreated(); + } + + public void Dispose() + { + try { File.Delete(_dbPath); } catch { } + try { File.Delete(_dbPath + "-wal"); } catch { } + try { File.Delete(_dbPath + "-shm"); } catch { } + } + + private ClaudeDoDbContext NewContext() + { + var opts = new DbContextOptionsBuilder() + .UseSqlite($"Data Source={_dbPath}") + .Options; + return new ClaudeDoDbContext(opts); + } + + private sealed class TestDbFactory : IDbContextFactory + { + private readonly Func _create; + public TestDbFactory(Func create) => _create = create; + public ClaudeDoDbContext CreateDbContext() => _create(); + } + + private sealed class RecordingWorkerClient : StubWorkerClient + { + public override bool IsConnected => true; + public string? QueuedParentId; + public override Task QueuePlanningSubtasksAsync(string parentTaskId, CancellationToken ct = default) + { + QueuedParentId = parentTaskId; + return Task.CompletedTask; + } + } + + private TasksIslandViewModel BuildViewModel(StubWorkerClient worker) => + new(new TestDbFactory(NewContext), worker); + + [Fact] + public async Task SendToQueue_FinalizedPlan_BlockedWhenAChildHasInteractiveSession() + { + var worker = new RecordingWorkerClient(); + var vm = BuildViewModel(worker); + + var parent = new TaskRowViewModel + { + Id = "parent-1", + Status = TaskStatus.Idle, + PlanningPhase = PlanningPhase.Finalized, + HasPlanningChildren = true, + }; + var interactiveChild = new TaskRowViewModel + { + Id = "child-1", + Title = "Fix the flaky test", + ParentTaskId = "parent-1", + Status = TaskStatus.Idle, + HasInteractiveSession = true, + }; + var otherChild = new TaskRowViewModel + { + Id = "child-2", + ParentTaskId = "parent-1", + Status = TaskStatus.Idle, + }; + vm.Items.Add(parent); + vm.Items.Add(interactiveChild); + vm.Items.Add(otherChild); + + Assert.True(parent.CanQueuePlan); + + string? reportedError = null; + vm.ErrorReported += msg => reportedError = msg; + + await vm.SendToQueueCommand.ExecuteAsync(parent); + + Assert.Null(worker.QueuedParentId); + Assert.NotNull(reportedError); + Assert.Contains("Fix the flaky test", reportedError); + } + + [Fact] + public async Task SendToQueue_FinalizedPlan_ProceedsWhenNoChildHasInteractiveSession() + { + var worker = new RecordingWorkerClient(); + var vm = BuildViewModel(worker); + + var parent = new TaskRowViewModel + { + Id = "parent-2", + Status = TaskStatus.Idle, + PlanningPhase = PlanningPhase.Finalized, + HasPlanningChildren = true, + }; + var child = new TaskRowViewModel + { + Id = "child-3", + ParentTaskId = "parent-2", + Status = TaskStatus.Idle, + }; + vm.Items.Add(parent); + vm.Items.Add(child); + + await vm.SendToQueueCommand.ExecuteAsync(parent); + + Assert.Equal("parent-2", worker.QueuedParentId); + } +}