From 2aaaa23912a7ab6d36c806e3e759d65336711607 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Fri, 24 Jul 2026 12:52:57 +0200 Subject: [PATCH] feat(review): gate Approve & Merge behind opening the diff Approve & Merge is disabled until the pending changes have been inspected: DetailsIslandViewModel tracks ReviewDiffViewed (reset on task switch and on every state change), MergeSectionViewModel raises DiffViewed when a diff or combined diff is opened and exposes HasReviewableDiff, and a hint sits next to the button. A review with nothing to inspect (sandbox run, no worktree) approves straight through. ClaudeDo-Task: f9809a93 --- CLAUDE.md | 2 +- src/ClaudeDo.Ui/CLAUDE.md | 2 +- src/ClaudeDo.Ui/Design/IslandStyles.axaml | 7 ++++ .../Islands/DetailsIslandViewModel.cs | 37 +++++++++++++++++- .../Islands/MergeSectionViewModel.cs | 14 +++++++ .../Views/Islands/Detail/WorkConsole.axaml | 10 +++++ .../DetailsIslandReviewActionsTests.cs | 39 +++++++++++++++++++ 7 files changed, 108 insertions(+), 3 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 44372b20..f1421711 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,7 +39,7 @@ Each project has its own `CLAUDE.md` — those are the living per-project docs. - EF Core migrations manage schema (Migrations/ folder in ClaudeDo.Data) - `IDbContextFactory` used by singleton consumers (e.g. Worker) - Entity configuration via `IEntityTypeConfiguration` in Configuration/ folder -- Task status flow: Idle | Queued -> Running -> WaitingForReview -> Done | Failed | Cancelled. A task that spawns/has children passes through WaitingForChildren first, then surfaces for review once every child is terminal — this is the single parent model for both planning and improvement parents (planning/improvement *children* themselves go straight to Done, only the parent is reviewed). From review you can approve, reject-rerun (Queued, resumes the session with feedback), reject-park (Idle), or cancel. Approve is the single review+merge action: a childless task merges its own worktree then Done (conflicts keep it in WaitingForReview); a task with children drives the unit merge (parent worktree if any + each Done child in order, with conflict continue/abort). Tasks with no active worktree (sandbox run) approve straight to Done. +- Task status flow: Idle | Queued -> Running -> WaitingForReview -> Done | Failed | Cancelled. A task that spawns/has children passes through WaitingForChildren first, then surfaces for review once every child is terminal — this is the single parent model for both planning and improvement parents (planning/improvement *children* themselves go straight to Done, only the parent is reviewed). From review you can approve, reject-rerun (Queued, resumes the session with feedback), reject-park (Idle), or cancel. Approve is the single review+merge action: a childless task merges its own worktree then Done (conflicts keep it in WaitingForReview); a task with children drives the unit merge (parent worktree if any + each Done child in order, with conflict continue/abort). Tasks with no active worktree (sandbox run) approve straight to Done. In the detail pane, **Approve & Merge is gated behind opening the diff**: when there is something to inspect (worktree diff / merged range / children combined diff) the button stays disabled until the diff or combined-diff viewer has been opened once, and re-locks per run (any state change resets it); tasks with nothing to inspect are never gated. The row-level quick-approve in the task list is an intentional bypass. - Worktree state flow: Active -> Merged | Discarded | Kept - The queue picker claims tasks by `Status=Queued` (with `BlockedByTaskId IS NULL`); the legacy tag system was removed - Interfaces live in an `Interfaces/` subfolder beside their consumers (namespace unchanged) diff --git a/src/ClaudeDo.Ui/CLAUDE.md b/src/ClaudeDo.Ui/CLAUDE.md index 0208ca8b..907c0e0d 100644 --- a/src/ClaudeDo.Ui/CLAUDE.md +++ b/src/ClaudeDo.Ui/CLAUDE.md @@ -36,7 +36,7 @@ Design/ — Tokens.axaml (design tokens; merged before styles) + IslandStyle - **IslandsShellViewModel** — root coordinator; owns the three island VMs and the `WorkerClient`, wires cross-island events (selection, notes/prep mode, conflict resolution), owns connection state, the update banner, the inline worker-log strip (clickable → Log Visualizer overlay via `OpenLogVisualizerCommand`; `FlashFooterError` surfaces UI-action failures + the worker's Serilog Warn/Error there), responsive-layout flags (`ShowLists`/`ShowDetails` by window width), `PrimeStatus` flash, and the modal openers (About, RepoImport, WeeklyReport, WorktreesOverview, WorkerConnection help, LogVisualizer) plus `RestartWorkerAsync`/`CheckForUpdatesAsync`. Hosts `UpdateCheckService`. - **ListsIslandViewModel** — smart lists (My Day, Important, Planned, virtual queued/running/review), user lists, selection, list CRUD, drag-reorder, badge counts, opens list settings / repo import / worktrees overview, `OpenInExplorer`/`OpenInTerminal`. - **TasksIslandViewModel** — open/overdue/completed groups for the selected list with hierarchy-aware regrouping; task CRUD, drag-reorder, toggle done/star, schedule, enqueue/dequeue, cancel; review actions (approve, reject-rerun, reject-park, cancel); planning session lifecycle (open/resume/discard/finalize, `QueuePlanningSubtasksAsync`); `RefineTask`, `OpenConPtySessionRequested` (embedded ConPTY terminal), `PickUpInTerminalAsync`; MyDay extras (`IsMyDayList`, `ClearDayCommand`, `ShowPrepLogCommand`) and the pinned Notes pseudo-row (`ShowNotesRow`, `OpenNotesCommand`). Raises `NotesRequested`/`PrepRequested` events consumed by the shell. -- **DetailsIslandViewModel** — the detail pane for a bound `TaskRowViewModel`. Owns live-log streaming (`Log` via `StreamLineFormatter`), debounced title/description editing, subtasks, session-outcome/roadblock split (splits `Result` at the roadblock marker into two cards), the three-tab work console (`output`/`git`/`session`), child surfacing (`ChildOutcomes` rows plus `ChildrenNeedingAttention`/`HasChildrenNeedingAttention` — children that failed, were cancelled, await review, or reported roadblocks — drive an attention band on the Session tab, which is only visible when `HasChildOutcomes`), and the modes: `IsNotesMode` (hosts `NotesEditorViewModel`), `IsPrepMode`, computed `IsTaskDetailVisible = !IsNotesMode && !IsPrepMode`. Three concerns are extracted into section VMs exposed as properties: **AgentConfigEditorViewModel** (scope=Task; per-task Model/MaxTurns/AgentPath overrides with `InheritedBadge` + `InheritanceResolver`, additive SystemPrompt, debounced auto-save; exposed as `AgentSettings`), **MergeSectionViewModel** (merge-target selection, mergeability indicator via `MergePreviewPresenter` over `PreviewMergeAsync`, `OpenDiffAsync` and `ReviewCombinedDiffCommand` — both build a `DiffViewerViewModel` and call `ShowDiffViewer`), **PrepPanelViewModel** (daily-prep panel: `PrepLog`, `PlanDayCommand` → `RunDailyPrepNowAsync`, persisted last run via `GetLastPrepLogAsync`). Attachments: `Attachments` (`ObservableCollection`), `IsDragOver`, `DropStatus`, `CanAcceptDrop`, `AddFilesAsync`, `RemoveAttachmentCommand`; loads on task change; `ComposedPreview` includes attachment paths. Writes directly via `new AttachmentStore()` + `new TaskAttachmentRepository(ctx)`. Helper rows (`ChildOutcomeRowViewModel`, `SubtaskRowViewModel`, `LogLineViewModel`, `AttachmentRowViewModel`) live in the same file. +- **DetailsIslandViewModel** — the detail pane for a bound `TaskRowViewModel`. Owns live-log streaming (`Log` via `StreamLineFormatter`), debounced title/description editing, subtasks, session-outcome/roadblock split (splits `Result` at the roadblock marker into two cards), the three-tab work console (`output`/`git`/`session`), child surfacing (`ChildOutcomes` rows plus `ChildrenNeedingAttention`/`HasChildrenNeedingAttention` — children that failed, were cancelled, await review, or reported roadblocks — drive an attention band on the Session tab, which is only visible when `HasChildOutcomes`), and the modes: `IsNotesMode` (hosts `NotesEditorViewModel`), `IsPrepMode`, computed `IsTaskDetailVisible = !IsNotesMode && !IsPrepMode`. Three concerns are extracted into section VMs exposed as properties: **AgentConfigEditorViewModel** (scope=Task; per-task Model/MaxTurns/AgentPath overrides with `InheritedBadge` + `InheritanceResolver`, additive SystemPrompt, debounced auto-save; exposed as `AgentSettings`), **MergeSectionViewModel** (merge-target selection, mergeability indicator via `MergePreviewPresenter` over `PreviewMergeAsync`, `OpenDiffAsync` and `ReviewCombinedDiffCommand` — both build a `DiffViewerViewModel`, call `ShowDiffViewer`, and fire the `DiffViewed` callback; `HasReviewableDiff` reports whether anything is inspectable, feeding the review gate), **PrepPanelViewModel** (daily-prep panel: `PrepLog`, `PlanDayCommand` → `RunDailyPrepNowAsync`, persisted last run via `GetLastPrepLogAsync`). Attachments: `Attachments` (`ObservableCollection`), `IsDragOver`, `DropStatus`, `CanAcceptDrop`, `AddFilesAsync`, `RemoveAttachmentCommand`; loads on task change; `ComposedPreview` includes attachment paths. Writes directly via `new AttachmentStore()` + `new TaskAttachmentRepository(ctx)`. Helper rows (`ChildOutcomeRowViewModel`, `SubtaskRowViewModel`, `LogLineViewModel`, `AttachmentRowViewModel`) live in the same file. - **TaskRowViewModel** / **ListNavItemViewModel** — lightweight display VMs (task row: status, planning phase, parent/blocked links, roadblock count, computed `IsDraft`/`IsPlanned`/`IsChild`/`IsPlanningParent`/`CanRefine`; list row: kind Smart/Virtual/User, count, icon/dot keys, drop hints). - **NotesEditorViewModel** — day navigator + bullet CRUD for daily notes via `INotesApi`. - **Modal VMs** — `SettingsModalViewModel` (four tabs: General, Worktrees, Files prompt-paths, Prime Claude incl. `DailyPrepMaxTasks` + prime-schedule rows), `ListSettingsModalViewModel` (name, working dir, commit type, delete list; hosts shared `AgentConfigEditorViewModel` as `Agent` property (scope=List) — save delegates to `Agent.SaveAsync()`), `RepoImportModalViewModel` (bulk-create lists from git repos found under chosen parents; already-wired repos disabled), `WeeklyReportModalViewModel` (range pickers default "since last standup weekday → today", cached per range, markdown via MarkdownView), `MergeModalViewModel` (single-task merge form, called from the diff modal), `WorktreesOverviewModalViewModel` (global/per-list worktree rows, batch merge + state ops), `UnfinishedPlanningModalViewModel` (Resume/FinalizeNow/Discard for a draft planning session), `WorkerConnectionModalViewModel` (offline help), `AboutModalViewModel`, `LogVisualizerViewModel` (worker logs, last 30 min, all levels + a warn/error-only filter; loads via `GetRecentLogsAsync`). diff --git a/src/ClaudeDo.Ui/Design/IslandStyles.axaml b/src/ClaudeDo.Ui/Design/IslandStyles.axaml index 59a9c035..8310bce7 100644 --- a/src/ClaudeDo.Ui/Design/IslandStyles.axaml +++ b/src/ClaudeDo.Ui/Design/IslandStyles.axaml @@ -1162,6 +1162,13 @@ + + diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs index fc5ce670..7c6b7647 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/DetailsIslandViewModel.cs @@ -296,6 +296,15 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable AgentSettings = new AgentConfigEditorViewModel(worker, AgentConfigScope.Task); Merge = new MergeSectionViewModel(worker, services); + Merge.DiffViewed = () => ReviewDiffViewed = true; + Merge.PropertyChanged += (_, e) => + { + if (e.PropertyName == nameof(MergeSectionViewModel.HasReviewableDiff)) + { + ApproveReviewCommand.NotifyCanExecuteChanged(); + OnPropertyChanged(nameof(ShowReviewDiffHint)); + } + }; Prep = new PrepPanelViewModel(worker); Notes = new NotesEditorViewModel(_notesApi); @@ -399,6 +408,11 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable DequeueCommand.NotifyCanExecuteChanged(); ResetAndRetryCommand.NotifyCanExecuteChanged(); ContinueCommand.NotifyCanExecuteChanged(); + // A state change means a new run/review cycle: the diff must be + // re-inspected before merge can be approved again. + ReviewDiffViewed = false; + ApproveReviewCommand.NotifyCanExecuteChanged(); + OnPropertyChanged(nameof(ShowReviewDiffHint)); AgentSettings.IsRunning = IsRunning; NotifySessionSections(); OnPropertyChanged(nameof(CanAcceptDrop)); @@ -752,6 +766,7 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable partial void OnTaskChanged(TaskRowViewModel? value) { + ReviewDiffViewed = false; Merge.SyncTaskContext(Task?.Id, Task?.Title, Task?.IsPlanningParent == true); NotifySessionSections(); OnPropertyChanged(nameof(CanAcceptDrop)); @@ -981,7 +996,20 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable private bool CanResetAndRetry() => Task != null && _worker.IsConnected && ShowResetAndRetry; - [RelayCommand] + // 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 + // Approve & Merge stays blocked until the pending changes have been inspected. + [ObservableProperty] + [NotifyCanExecuteChangedFor(nameof(ApproveReviewCommand))] + [NotifyPropertyChangedFor(nameof(ShowReviewDiffHint))] + private bool _reviewDiffViewed; + + // True while a review is pending, there is a diff to inspect, and it has not been + // opened yet — drives the "open the diff first" hint next to Approve & Merge. + public bool ShowReviewDiffHint => + IsWaitingForReview && Merge.HasReviewableDiff && !ReviewDiffViewed; + + [RelayCommand(CanExecute = nameof(CanApproveReview))] private async System.Threading.Tasks.Task ApproveReviewAsync() { if (Task is null || !_worker.IsConnected) return; @@ -999,6 +1027,13 @@ public sealed partial class DetailsIslandViewModel : ViewModelBase, IDisposable } } + // Force the diff to have been opened before a merge can happen — but only when + // there is actually something to inspect (a childless sandbox run with no worktree + // has no diff, so it approves straight through). + private bool CanApproveReview() => + Task != null && _worker.IsConnected && IsWaitingForReview + && (!Merge.HasReviewableDiff || ReviewDiffViewed); + [RelayCommand(CanExecute = nameof(HasReviewFeedback))] private async System.Threading.Tasks.Task RejectReviewAsync() { diff --git a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs index 7510a779..2a00a78a 100644 --- a/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs +++ b/src/ClaudeDo.Ui/ViewModels/Islands/MergeSectionViewModel.cs @@ -48,6 +48,15 @@ public sealed partial class MergeSectionViewModel : ViewModelBase public Func? ShowDiffViewer { get; set; } public Func? ShowMergeModal { get; set; } + // Raised when the user opens a diff/combined-diff for the current task, so the + // review gate can record that the changes were inspected before merging. + public Action? DiffViewed { get; set; } + + // True when there is something to inspect before merging (a live worktree diff, + // a merged commit range, or a planning/children combined diff). When false there + // is nothing to read, so the review gate must not block approve. + public bool HasReviewableDiff => CanOpenDiff() || CanReviewDiff(); + public MergeSectionViewModel(IWorkerClient worker, IServiceProvider services) { _worker = worker; @@ -69,6 +78,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase _worktreeStateLabel = worktreeState; _listWorkingDir = listWorkDir; OnPropertyChanged(nameof(ShowMergeSection)); + OnPropertyChanged(nameof(HasReviewableDiff)); OpenDiffCommand.NotifyCanExecuteChanged(); OpenWorktreeCommand.NotifyCanExecuteChanged(); } @@ -79,6 +89,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase TaskTitle = taskTitle; _isPlanningParent = isPlanningParent; OnPropertyChanged(nameof(ShowMergeSection)); + OnPropertyChanged(nameof(HasReviewableDiff)); } internal void SyncChildOutcomes(bool hasChildOutcomes, int subtaskCount) @@ -86,6 +97,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase _hasChildOutcomes = hasChildOutcomes; _subtaskCount = subtaskCount; OnPropertyChanged(nameof(ShowMergeSection)); + OnPropertyChanged(nameof(HasReviewableDiff)); ReviewCombinedDiffCommand.NotifyCanExecuteChanged(); } @@ -128,6 +140,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase var vm = _services.GetRequiredService(); vm.ConfigurePlanning(TaskId, SelectedMergeTarget ?? "main"); await vm.LoadAsync(); + DiffViewed?.Invoke(); await ShowDiffViewer(vm); } @@ -157,6 +170,7 @@ public sealed partial class MergeSectionViewModel : ViewModelBase else return; await vm.LoadAsync(); + DiffViewed?.Invoke(); await ShowDiffViewer(vm); } diff --git a/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml b/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml index 0e6e2314..2eb3bbdf 100644 --- a/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml +++ b/src/ClaudeDo.Ui/Views/Islands/Detail/WorkConsole.axaml @@ -348,6 +348,16 @@ + + + + + +