From 66a7b2377fdd9b38d0e7e5b6f1810df9c5019310 Mon Sep 17 00:00:00 2001 From: mika kuns Date: Thu, 4 Jun 2026 23:07:25 +0200 Subject: [PATCH] docs(merge): add approve-merge + conflict-preview design spec Co-Authored-By: Claude Opus 4.7 --- ...prove-merge-and-conflict-preview-design.md | 173 ++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 docs/superpowers/specs/2026-06-04-approve-merge-and-conflict-preview-design.md diff --git a/docs/superpowers/specs/2026-06-04-approve-merge-and-conflict-preview-design.md b/docs/superpowers/specs/2026-06-04-approve-merge-and-conflict-preview-design.md new file mode 100644 index 0000000..b8e2739 --- /dev/null +++ b/docs/superpowers/specs/2026-06-04-approve-merge-and-conflict-preview-design.md @@ -0,0 +1,173 @@ +# Approve = Merge → Done, plus Conflict Preview — Design + +**Date:** 2026-06-04 +**Status:** Approved (autonomous — user on break, authorized to continue) +**Author:** brainstormed from issue "Make merge/diff real" + +## Problem + +Approving a `WaitingForReview` task flips it straight to `Done` +(`TaskStateService.ApproveReviewAsync`) and **never merges** its worktree — the +worktree stays `Active`. The user approved three component tasks expecting them +to merge; none did. Separately, there is **no way to see whether a task's +worktree merges cleanly** before acting, and a standalone task has no direct +**Merge** button (single-task merge is only reachable from inside the Diff +modal). + +What is already real (verified): `WorkerHub.MergeTask → TaskMergeService.MergeAsync` +performs a real `git merge --no-ff`, aborts on conflict, and marks the worktree +`Merged`. **Open Diff** opens a real in-app diff. **Merge All Subtasks** +(planning) is real. So the gaps are narrow. + +## Scope decisions (autonomous) + +- **Tab location:** keep the **single "Session" tab** that the recent commit + `ac9bae9` deliberately consolidated. All new controls go in its existing + `MERGE & WORKTREE` block (`WorkConsole.axaml:196`). Do **not** re-introduce a + separate "Actions" tab. +- **Approve target:** Approve merges into the UI-selected merge target + (`SelectedMergeTarget`); when blank, the worker resolves to the repo's current + branch. +- **On conflict:** task stays in `WaitingForReview` (no new status). The conflict + is surfaced inline. No automatic state change to a "blocked" status. +- **Worktree removal on approve:** do **not** remove — merge marks the worktree + `Merged` and existing auto-cleanup handles disposal (matches the single-task + merge default `removeWorktree:false`). +- **Applies to:** standalone leaf tasks with an active worktree. A + `WaitingForReview` task with **no** active worktree (e.g. ran in a sandbox, or + an improvement parent whose children own the worktrees) is just marked `Done` + — current behavior preserved. Planning parents keep "Merge All Subtasks". + +## Acceptance (restated) + +1. Approve a clean-merging task → worktree merged into target, worktree `Merged`, + task `Done`. +2. Approve a conflicting task → task **not** `Done`, conflict surfaced. +3. Opening a Done/WaitingForReview task shows clean/conflict status **without + mutating** the tree (use `git merge-tree`, not a real merge). + +## Architecture + +Three layers, each single-purpose; the only new cross-dependency is +`TaskMergeService → ITaskStateService` (one-way; verify no DI cycle). + +### 1. GitService — non-destructive conflict probe (`ClaudeDo.Data`) + +New method: + +```csharp +public sealed record MergePreview(bool Supported, bool Clean, IReadOnlyList ConflictFiles); + +public async Task PreviewMergeAsync( + string repoDir, string targetBranch, string sourceBranch, CancellationToken ct = default) +``` + +- Runs `git merge-tree --write-tree --name-only ` from `repoDir`. + `merge-tree` computes the merge base itself and writes only loose objects — it + does **not** touch the working tree, index, or refs. +- Exit code `0` → `Clean = true`, no conflict files. +- Exit code `1` → `Clean = false`; conflicted paths are the lines after the + first (tree-OID) line, up to the first blank line. +- Any other outcome (e.g. git too old → "unknown option") → `Supported = false` + (UI shows "mergeability unknown"). + +New helper for the "· N files" count (clean case): +`git diff --name-only ...` (three-dot = changes on source since +the merge base); count non-empty lines. May reuse/extend existing diff helpers. + +### 2. TaskMergeService — preview + approve orchestration (`ClaudeDo.Worker`) + +Inject `ITaskStateService` (verify `PlanningChainCoordinator` has no back-edge to +`TaskMergeService`; if a cycle exists, fall back to orchestrating in the hub). + +```csharp +public sealed record MergePreviewResult(string Status, IReadOnlyList ConflictFiles, int ChangedFileCount); +// Status: "clean" | "conflict" | "unavailable" +public Task PreviewAsync(string taskId, string targetBranch, CancellationToken ct); + +public Task ApproveAndMergeAsync(string taskId, string targetBranch, CancellationToken ct); +``` + +**PreviewAsync:** load context. If no active worktree → `"unavailable"`. Resolve +`targetBranch` (blank → current branch). Call `GitService.PreviewMergeAsync`; map +`Supported=false` → `"unavailable"`, else clean/conflict (+ ChangedFileCount on +clean). + +**ApproveAndMergeAsync:** load context; require `task.Status == WaitingForReview` +(else `Blocked`). Resolve target (blank → current branch). +- **No active worktree** → `_state.ApproveReviewAsync(taskId)` → return + `MergeResult(StatusMerged, [], null)` ("approved, nothing to merge"). +- **Active worktree** → `MergeAsync(taskId, target, removeWorktree:false, + "Merge {branch}", ct)`. On `StatusMerged` → `_state.ApproveReviewAsync(taskId)` + then return the merged result. On `StatusConflict`/`StatusBlocked` → return as-is; + **do not** flip status (task stays `WaitingForReview`). + +`TaskStateService.ApproveReviewAsync` is unchanged (still the sole Status writer; +still runs `OnChildTerminalAsync`). + +### 3. WorkerHub — signatures (`ClaudeDo.Worker`) + +```csharp +public record MergePreviewDto(string Status, IReadOnlyList ConflictFiles, int ChangedFileCount); + +public Task PreviewMerge(string taskId, string targetBranch); // new +public Task ApproveReview(string taskId, string targetBranch); // CHANGED: was void(taskId) +``` + +`ApproveReview` returns the orchestration result so the UI can react to conflicts. +`MergeTask` / `GetMergeTargets` unchanged. + +### 4. UI (`ClaudeDo.Ui`) + +`IWorkerClient` (+ `WorkerClient` + **both test-project fakes** — see memory: +changing `IWorkerClient` breaks hand-rolled fakes): +- Change `Task ApproveReviewAsync(string)` → `Task ApproveReviewAsync(string taskId, string targetBranch)`. +- Add `Task PreviewMergeAsync(string taskId, string targetBranch)`. +- Add `Task MergeTaskAsync(...)` to the **interface** (already on + the concrete client) so the single-task Merge button can use `_worker`. + +`DetailsIslandViewModel`: +- **Load merge targets whenever a worktree exists.** In `BindAsync`, when + `entity.Worktree != null` and the task is not a planning parent, call + `GetMergeTargetsAsync(taskId)` and set `SelectedMergeTarget = DefaultBranch` + (fixes the standalone-task gap where targets were never loaded). +- **Mergeability indicator** properties: `MergePreviewText` (string), + `MergeIsClean` / `MergeIsConflict` (bool, for color). Compute via + `PreviewMergeAsync` when the merge section is shown for an **Active** worktree; + recompute on `SelectedMergeTarget` change. If worktree state is + `Merged/Discarded/Kept`, show that label instead of probing. Text examples: + "Merges cleanly · 7 files" / "Conflicts in a.cs, b.cs" / "Mergeability unknown". +- **Approve** (`ApproveReviewAsync`): pass `SelectedMergeTarget ?? ""`; inspect + result — on `"conflict"` set the conflict indicator + a short notice + ("Approve blocked — resolve conflicts first"); success path relies on the + existing `TaskUpdated` broadcast to refresh. +- **Single-task Merge** (`MergeCommand`): `MergeTaskAsync(taskId, + SelectedMergeTarget ?? "", removeWorktree:false, "Merge task")`; on `"conflict"` + show the conflict indicator. Shown for non-planning tasks with an active + worktree (planning parents keep "Merge All Subtasks"). + +`WorkConsole.axaml` (Session tab, `MERGE & WORKTREE` block): +- Add a status line above the button row bound to `MergePreviewText`, colored + green (`MossBrush`) when `MergeIsClean`, red (`BloodBrush`) when + `MergeIsConflict`, muted otherwise. Use existing tokens/classes only. +- Add a **Merge** button (`MergeCommand`) beside **Open Diff** for the + single-task path. + +## Testing (git-backed, no real Claude) + +In `ClaudeDo.Worker.Tests` (real temp git repos + real SQLite), and/or +`ClaudeDo.Data.Tests` for the pure git probe: +- `GitService.PreviewMergeAsync`: clean branches → `Clean=true`; a real + edit-conflict on the same lines → `Clean=false` with the expected file in + `ConflictFiles`. +- `ApproveAndMergeAsync`: clean worktree → returns `merged`, task is `Done`, + worktree state `Merged`. Conflicting worktree → returns `conflict`, task still + `WaitingForReview`, worktree still `Active`, target branch unmodified + (HEAD unchanged, no `MERGE_HEAD`). +- No-worktree `WaitingForReview` task → returns `merged`, task `Done`. + +## Out of scope + +External difftools, new task statuses, auto-removing worktrees on approve, +re-splitting the console into separate tabs, conflict resolution UI (the existing +`ContinueMerge`/`AbortMerge` paths remain as-is for mid-merge cases).