From f106c890b3735e59ca22983c82d0928a8c71b51e Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 6 Aug 2026 10:20:54 +0200 Subject: [PATCH] fix(ui): refresh the run session id live, close the stale handoff pane A task selected before its run started kept LatestRunSessionId null, so the roadblock reply box and Continue stayed dead until it was re-selected. The handoff left the Phase 1-2 tile open so its last message could be read, but its process is gone by then and the terminal renders empty -- a dead placeholder. It is closed on handoff now. Also drops the WARN flood from worktree cleanup (already-unregistered worktree and already-deleted branch are normal outcomes, not failures) and replaces the fixed sleep in UsageGate_TransitionLogging_FiresOncePerChange with the polling helper that already sits three lines below it in the same file. --- docs/open.md | 33 ++++++++++++++----- .../Islands/DetailsIslandViewModel.cs | 16 +++++++++ .../ViewModels/MissionControlViewModel.cs | 12 ++++--- .../Worktrees/WorktreeMaintenanceService.cs | 26 ++++++++++++--- .../Services/QueueServiceTests.cs | 14 ++++---- 5 files changed, 79 insertions(+), 22 deletions(-) diff --git a/docs/open.md b/docs/open.md index f1d6d535..a5241c32 100644 --- a/docs/open.md +++ b/docs/open.md @@ -9,13 +9,6 @@ Checkliste lebt in `docs/verification-handoff.md`. ## Bugs (offen) -- **Settings-Modal: Leeren eines NumericUpDown wirft `InvalidCastException`** (gefunden bei der Sichtprüfung 2026-08-06). `NumericUpDown.Value` ist `decimal?`; sobald das Textfeld leer ist (der normale Weg, einen Wert zu ändern: alten Wert löschen, neuen tippen) schreibt die TwoWay-Bindung `null` in eine nicht-nullbare Ziel-Property. Betrifft **acht** Felder: `ModelPresetRowViewModel.MaxTurns` (`decimal`), `General.MaxTurnsCeiling`, `General.MaxParallelExecutions`, `General.UsageGateFiveHourPct`, `General.UsageGateSevenDayPct`, `Worktrees.WorktreeAutoCleanupDays`, `Prime.DailyPrepMaxTasks`, `OnlineInbox.PollIntervalSeconds` (alle `int`). Nur `AgentConfigEditorViewModel.MaxTurns` ist `decimal?` und deshalb heil. Fix-Optionen: (a) die Properties nullable machen (wie im Agent-Editor) und an den Verwendungsstellen coercen, ODER (b) ein kleiner `IValueConverter`, der beim ConvertBack `null` auf `BindingOperations.DoNothing` abbildet — eine Klasse plus acht Binding-Änderungen, keine DTO-/Validate-Umbauten. - -- **Roadblock-Reply (und Continue) bleiben tot, wenn der Task während des Laufs offen war** (Sichtprüfung 2026-08-06). `LatestRunSessionId` wird ausschließlich im Auswahl-Ladepfad gesetzt (`DetailsIslandViewModel.cs:619`). Hat man den Task schon offen, während er läuft und in den Roadblock läuft, war die Session-Id beim Auswählen noch `null`, und kein Live-Refresh holt sie nach: `RefreshBoundTaskAsync` (804) aktualisiert nur die Task-Zeile, `RefreshWorktreeAsync` (821) nur Worktree/Diff, der `TaskFinished`-Handler (364-369) ruft nur diese beiden. Ergebnis: Antwortfeld deaktiviert samt „keine Session"-Hinweis, obwohl es eine Session gibt — nach erneutem Öffnen des Tasks funktioniert es. Derselbe Gate hängt an `CanContinue` (1053), der Continue-Button ist also gleichzeitig tot. Fix: `LatestRunSessionId` beim Run-Ende mitziehen (`TaskRunRepository.GetLatestByTaskIdAsync` im `TaskFinished`-Pfad). -- **Verify-Gate wird für worktree-lose Tasks komplett übersprungen** (Sichtprüfung 2026-08-06). `TaskMergeService.ApproveAndMergeAsync` (Zeile 537-543): hat ein Task keinen aktiven `WorktreeEntity`, geht der Pfad direkt auf `_state.ApproveReviewAsync` → `Done` und meldet `merged` — der Gate läuft nie, das `verifyCommand` aus `LoadMergeContextAsync` wird auf Zeile 532 sogar explizit verworfen (`var (task, list, wt, _)`). Betroffen sind genau die **List-Handler-Tasks** (per Design worktree-los, arbeiten via `HandlerBaseCommit`/`HandlerHeadCommit` direkt in `list.WorkingDir`) und Sandbox-Runs — also ausgerechnet der Lauf, der am meisten auf einmal nach main bringt. Reproduziert: bei gesetztem `VerifyCommand = cmd /c exit 1` hat „Approve & Merge" den Handler-Task kommentarlos auf `Done` gesetzt. Fix: den Gate auch in diesem Zweig laufen lassen (Arbeitsverzeichnis ist `list.WorkingDir`), bevor `ApproveReviewAsync` gerufen wird. -- **Verify-Gate ist nur an einem von drei Merge-Einstiegspunkten verdrahtet** (Sichtprüfung 2026-08-06, reproduziert mit `VerifyCommand = cmd /c exit 1`). Der Gate greift funktional korrekt (Merge landet, Task bleibt aus `Done`), aber nur `DetailsIslandViewModel.ApproveReviewAsync` (Zeile 1125) kennt `verify_failed` und zeigt `result.ErrorMessage`. `MergeModalViewModel` (Zeile 113-115) fällt in den `default`-Zweig und zeigt wörtlich **„Unknown status: verify_failed"** — der Output-Tail des Verify-Kommandos wird verworfen. `WorktreesOverviewModalViewModel` (Zeile 405) mappt ihn auf generisches `BatchMergeOutcome.Failed`, der Grund geht ebenfalls verloren. Fix: beide Stellen um einen `verify_failed`-Zweig ergänzen, der `result.ErrorMessage` durchreicht (Text existiert schon als `vm.detailsIsland.verifyFailed`). -- **List-Handler-Handoff lässt eine leere Geister-Pane zurück** (Sichtprüfung 2026-08-06). Nach `handoff_list_handler` öffnet `MissionControlViewModel.OpenMergeHelperHandoffConPtySessionAsync` (Zeile 255) bewusst eine **zweite** Pane für dieselbe Task-Id und lässt die erste stehen — laut Kommentar (Zeile 250-254), damit Mika die letzte Nachricht der alten Session noch lesen kann. In der Praxis ist die alte Pane nach dem Prozessende aber **komplett leer**, es gibt also nichts mehr zu lesen; es bleibt nur ein toter Kachel-Platzhalter, den man von Hand schließen muss. Fix-Optionen: (a) die alte Pane beim Handoff automatisch schließen, ODER (b) ihren letzten Output erhalten und sie sichtbar als „beendet" markieren — die aktuelle Zwischenlösung erfüllt ihren Zweck nicht. -- **Worktree-Cleanup loggt „schon erledigt" als WARN** (Sichtprüfung 2026-08-06). `WorktreeMaintenanceService.TryRemoveAsync` (Zeilen 132-198) behandelt zwei völlig normale Ausgänge wie Fehlschläge: `git worktree remove` → `fatal: '' is not a working tree` (git kennt den Worktree längst nicht mehr) und `git branch -D` → `error: branch '…' not found` (nach dem Merge schon gelöscht). Pro DB-Zeile also zweimal `LogWarning` inkl. Stacktrace; `BroadcastLogSink` spiegelt jedes Warn in den Footer-Strip → beim Abräumen von ~20 veralteten Zeilen sieht es aus wie ein Dauerfehler, obwohl der Cleanup korrekt durchläuft. Fix: diese beiden bekannten Meldungen auf `LogDebug`, WARN nur für echte Fehler. - **Verwaiste git-Worktrees sind für die App unsichtbar** (gefunden 2026-08-06). In `C:\Private\ClaudeDo` waren nach dem Cleanup 22 git-registrierte Worktrees + 26 `claudedo/*`-Branches vorhanden, die ClaudeDo-DB kannte davon nur zwei. Die Worktrees-Übersicht listet ausschließlich Zeilen aus `worktrees`, also kann der Nutzer diese Reste nicht über die App entfernen — und es gibt keinen Sweep dafür (`OrphanRecovery` räumt nur Task-Zeilen auf, keine Worktrees). Wunsch: entweder ein Startup-Abgleich `git worktree list` ↔ DB, der Unbekannte als „untracked" in die Übersicht aufnimmt, oder mindestens eine Warnung mit Anzahl. ## UX / Nits (offen) @@ -47,7 +40,7 @@ verifiziert und deshalb hier entfernt. Es bleiben die Entscheidungen, die daran ## Beobachtung (offen — Entscheidung Mika) - **`--permission-mode auto` + Modell `haiku` → Writes werden denied:** Kontrolliert verifiziert (CLI 2.1.207): unter dem Default-Mode `auto` bekommt **sonnet** Writes auto-approved (`permission_denials:[]`), **haiku** wird `denied` (`permission_denials:[Write]`, keine Datei) — eine haiku-Task macht unter `auto` still nichts und landet ohne Änderung in `WaitingForReview`. Normalbetrieb (Default = sonnet) nicht betroffen. KEINE CLI-Regression, sondern modellabhängiges `auto`-Verhalten. Optionen falls es nervt: haiku aus der Auswahl nehmen, ODER Runner auf `acceptEdits`/`bypassPermissions` (modell-unabhängig). Mika: erstmal beobachten. Siehe Memory `auto_permission_haiku_footgun`. -- **`QueueServiceTests.UsageGate_TransitionLogging_FiresOncePerChange` ist zeitbasiert flaky, unabhängig von dieser Session:** Schlägt reproduzierbar fehl (`Expected 1, Actual 0` Warn-Log-Aufrufe), sowohl solo (`--filter`) als auch im Vollauf, auf einem sauberen `git worktree add` gegen `main` (bdee731) — also **kein** durch diese Abschluss-Session verursachter Regress (die Session hat keine `.cs`-Datei angefasst). Ursache: der Test verlässt sich auf einen festen `Task.Delay(200)`, um mehrere 50-ms-Backstop-Ticks abzuwarten (Kommentar im Test: „Several backstop ticks (50ms interval) all observe the same blocked state"); auf einer stark ausgelasteten Maschine (hier: viele parallele ClaudeDo-Worktrees/Builds) reicht das Fenster nicht immer. Zum Vergleich: derselbe Test lief in einer zweiten, isolierten Verifikation (Scratch-Merge für den Environment-Checks-Task) sauber durch (876/876). Fix wäre ein Poll-basiertes Warten statt fixem Sleep — aber außerhalb des Scopes dieser Doku/Verifikations-Session (keine Code-Änderung angefasst). +- ~~**`QueueServiceTests.UsageGate_TransitionLogging_FiresOncePerChange` ist zeitbasiert flaky**~~ — **am 2026-08-06 gefixt**: der feste `Task.Delay(200)` ist durch die schon im selben File vorhandene `AssertStableCountAsync` ersetzt (pollt bis der erste Backstop-Tick geloggt hat, wartet dann eine Karenzzeit und stellt sicher, dass kein weiterer Tick nachlegt). Historischer Befund zur Einordnung: Schlägt reproduzierbar fehl (`Expected 1, Actual 0` Warn-Log-Aufrufe), sowohl solo (`--filter`) als auch im Vollauf, auf einem sauberen `git worktree add` gegen `main` (bdee731) — also **kein** durch diese Abschluss-Session verursachter Regress (die Session hat keine `.cs`-Datei angefasst). Ursache: der Test verlässt sich auf einen festen `Task.Delay(200)`, um mehrere 50-ms-Backstop-Ticks abzuwarten (Kommentar im Test: „Several backstop ticks (50ms interval) all observe the same blocked state"); auf einer stark ausgelasteten Maschine (hier: viele parallele ClaudeDo-Worktrees/Builds) reicht das Fenster nicht immer. Zum Vergleich: derselbe Test lief in einer zweiten, isolierten Verifikation (Scratch-Merge für den Environment-Checks-Task) sauber durch (876/876). Fix wäre ein Poll-basiertes Warten statt fixem Sleep — aber außerhalb des Scopes dieser Doku/Verifikations-Session (keine Code-Änderung angefasst). --- @@ -88,6 +81,30 @@ verifiziert und deshalb hier entfernt. Es bleiben die Entscheidungen, die daran `dotnet test` invocation as the configured command) — only fast synthetic commands (`exit N`, `ping` for timeout) were exercised. +## Offene Verifikation (2026-08-06, Fix-Batch aus der Sichtprüfung) + +Fünf Findings der Sichtprüfung sind gefixt, Build + Tests grün, aber **noch nicht in der App +nachgeprüft** (der laufende Build ist älter — erst nach Neuinstallation testbar): + +- **NumericUpDown-Leeren wirft nicht mehr:** neuer `KeepLastNumberConverter` bildet beim + ConvertBack `null` auf `BindingOperations.DoNothing` ab, an allen acht nicht-nullbaren + NumericUpDowns im Settings-Modal verdrahtet. Prüfen: Wert löschen und neu tippen — keine + Exception, alter Wert bleibt stehen, bis eine echte Zahl kommt. Damit ist auch der offene Rest + von „MaxTurnsCeiling ändern → Hinweistexte ziehen mit" nachholbar. +- **Verify-Gate greift jetzt auch ohne Worktree:** Approve auf einem List-Handler-Task mit + fehlschlagendem `VerifyCommand` muss `verify_failed` liefern und den Task aus `Done` halten + (vorher ging er kommentarlos auf `Done`). +- **`verify_failed` im Merge-Modal und in der Worktrees-Übersicht:** Modal zeigt statt „Unknown + status" den echten Fehlertext und schließt sich **nicht** automatisch; die Batch-Übersicht + zeigt `VerifyFailed` und markiert die Zeile trotzdem als `Merged` (der Merge ist ja gelandet). +- **Roadblock-Reply/Continue live:** Task offen lassen, während er in den Roadblock läuft — das + Antwortfeld und Continue müssen **ohne** erneutes Öffnen aktiv werden. +- **Cleanup-Log:** ein Worktree-Cleanup über veraltete DB-Zeilen darf keine WARN-Flut mehr im + Footer erzeugen (die „schon erledigt"-Fälle gehen auf Debug). +- **Handoff schließt die alte Kachel:** beim Übergang von Phase 2 auf Phase 3 muss die alte + Merge-Helper-Kachel verschwinden und die neue an ihrer Stelle stehen — keine leere Geister-Pane + mehr. (Entscheidung Mika 2026-08-06: automatisch schließen statt Output erhalten.) + ## Offene Verifikation (2026-08-06, Resume planning session) `planning_session_id` wurde nie befüllt (der Setter hatte keinen Aufrufer), weil die interaktive diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index 7746b542..0a1ca8a2 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -366,6 +366,10 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable if (Task?.Id != taskId) return; _ = RefreshWorktreeAsync(taskId); _ = RefreshChildOutcomeAsync(taskId); + // The run only gets a session id once it has started, so a task that was already + // selected while it ran still holds the null from selection time. Without this the + // roadblock reply box and Continue stay dead until the task is re-selected. + _ = RefreshLatestRunSessionIdAsync(taskId); }; _worker.TaskFinishedEvent += _workerTaskFinishedHandler; @@ -801,6 +805,18 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable // Refreshes the bound row itself (status, planning phase, worktree/branch mirrors, etc.) from // the DB. Kept independent of TasksIslandViewModel's own handler: the row instance backing // Task may have been replaced by a full list reload, so this must not assume it stayed live. + private async System.Threading.Tasks.Task RefreshLatestRunSessionIdAsync(string taskId) + { + try + { + await using var ctx = await _dbFactory.CreateDbContextAsync(); + var latestRun = await new TaskRunRepository(ctx).GetLatestByTaskIdAsync(taskId); + if (Task?.Id != taskId) return; + LatestRunSessionId = latestRun?.SessionId; + } + catch { /* best-effort */ } + } + private async System.Threading.Tasks.Task RefreshBoundTaskAsync(string taskId) { try diff --git a/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs b/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs index 25895c02..c25d2e7a 100644 --- a/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/MissionControlViewModel.cs @@ -248,14 +248,18 @@ public sealed partial class MissionControlViewModel : ViewModelBase, IDisposable } // List-handler handoff: the running session called handoff_list_handler at the end of Phase 2. - // Opens a SECOND ConPTY tile for the SAME handler task id to carry out Phases 3-5 — deliberately - // NOT deduped by TaskId like OpenMergeHelperConPtySessionAsync above, since the original tile is - // meant to stay open (Mika closes it by hand once he has seen its last message). No new task is - // created here; see InteractiveLaunchSpecService.BuildForMergeHelperHandoffAsync. + // Replaces the Phase 1-2 tile with a fresh one for the SAME handler task id, which carries out + // Phases 3-5. The old tile used to be left open so its last message could still be read, but + // its process is gone by then and the terminal renders empty — it was only ever a dead + // placeholder to close by hand. No new task is created here; see + // InteractiveLaunchSpecService.BuildForMergeHelperHandoffAsync. public async System.Threading.Tasks.Task OpenMergeHelperHandoffConPtySessionAsync(string taskId, IReadOnlyList survivingTaskIds) { if (string.IsNullOrEmpty(taskId) || survivingTaskIds is not { Count: > 0 }) return; + foreach (var stale in ConPtySessions.Where(s => s.TaskId == taskId).ToList()) + CloseConPtySession(stale); + var baseTitle = Loc.T("missionControl.mergeHelperTitle"); var title = baseTitle + Loc.T("missionControl.mergeHelperHandoffTitleSuffix"); try diff --git a/src/ClaudeDo.Worker/Worktrees/WorktreeMaintenanceService.cs b/src/ClaudeDo.Worker/Worktrees/WorktreeMaintenanceService.cs index e95d0036..418eb3d4 100644 --- a/src/ClaudeDo.Worker/Worktrees/WorktreeMaintenanceService.cs +++ b/src/ClaudeDo.Worker/Worktrees/WorktreeMaintenanceService.cs @@ -143,8 +143,15 @@ public sealed class WorktreeMaintenanceService } catch (Exception ex) { - _logger.LogWarning(ex, - "git worktree remove failed for {Path}; falling back to directory delete", row.Path); + // "is not a working tree" means git already forgot this worktree (pruned, or + // removed by hand). Deleting the directory is then the whole job, not a fallback + // from a real failure -- warning about it floods the footer log strip when a + // batch of stale rows is cleaned up. + if (IsAlreadyUnregistered(ex)) + _logger.LogDebug(ex, "git no longer tracks {Path}; deleting the directory", row.Path); + else + _logger.LogWarning(ex, + "git worktree remove failed for {Path}; falling back to directory delete", row.Path); try { if (Directory.Exists(row.Path)) Directory.Delete(row.Path, recursive: true); } catch (Exception delEx) { @@ -186,8 +193,12 @@ public sealed class WorktreeMaintenanceService } catch (Exception ex) { - _logger.LogWarning(ex, "Failed to delete branch {Branch} for worktree {Path}", - row.BranchName, row.Path); + // Already deleted -- the normal case once the branch has been merged. + if (IsBranchAlreadyGone(ex)) + _logger.LogDebug(ex, "branch {Branch} was already deleted", row.BranchName); + else + _logger.LogWarning(ex, "Failed to delete branch {Branch} for worktree {Path}", + row.BranchName, row.Path); } } } @@ -197,5 +208,12 @@ public sealed class WorktreeMaintenanceService return (true, branchDeleted); } + // Both are "already done" outcomes, not failures — see the call sites. + private static bool IsAlreadyUnregistered(Exception ex) => + ex.Message.Contains("is not a working tree", StringComparison.OrdinalIgnoreCase); + + private static bool IsBranchAlreadyGone(Exception ex) => + ex.Message.Contains("' not found", StringComparison.Ordinal); + private sealed record WorktreeRow(string TaskId, string Path, string BranchName, string? WorkingDir); } diff --git a/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs b/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs index 8a3d28de..f9a28fae 100644 --- a/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs +++ b/tests/ClaudeDo.Worker.Tests/Services/QueueServiceTests.cs @@ -459,15 +459,17 @@ public sealed class QueueServiceTests : IDisposable using var cts = new CancellationTokenSource(); await service.StartAsync(cts.Token); - // Several backstop ticks (50ms interval) all observe the same blocked state. - await Task.Delay(200); - cts.Cancel(); + // Wait for the first backstop tick to log the transition, then keep observing: every + // further tick (50ms interval) sees the same blocked state and must stay silent. A fixed + // sleep was flaky here — on a loaded machine no tick fit inside the window at all. + await AssertStableCountAsync(CountWarnCalls, 1); - var warnCalls = _hubContext.Proxy.Calls - .Count(c => c.Method == "WorkerLog" && (WorkerLogLevel)c.Args[1]! == WorkerLogLevel.Warn); - Assert.Equal(1, warnCalls); + cts.Cancel(); } + private int CountWarnCalls() => _hubContext.Proxy.Calls + .Count(c => c.Method == "WorkerLog" && (WorkerLogLevel)c.Args[1]! == WorkerLogLevel.Warn); + // Polls until `read()` reaches `expected` (or times out), then waits a further grace period // to make sure the count doesn't keep climbing past it — needed because slot fills happen // concurrently and a fixed sleep is either flaky (too short) or slow (too long).