docs(merge): add approve-merge + conflict-preview design spec
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
mika kuns
@@ -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<string> ConflictFiles);
|
||||||
|
|
||||||
|
public async Task<MergePreview> PreviewMergeAsync(
|
||||||
|
string repoDir, string targetBranch, string sourceBranch, CancellationToken ct = default)
|
||||||
|
```
|
||||||
|
|
||||||
|
- Runs `git merge-tree --write-tree --name-only <target> <source>` 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 <target>...<source>` (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<string> ConflictFiles, int ChangedFileCount);
|
||||||
|
// Status: "clean" | "conflict" | "unavailable"
|
||||||
|
public Task<MergePreviewResult> PreviewAsync(string taskId, string targetBranch, CancellationToken ct);
|
||||||
|
|
||||||
|
public Task<MergeResult> 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<string> ConflictFiles, int ChangedFileCount);
|
||||||
|
|
||||||
|
public Task<MergePreviewDto> PreviewMerge(string taskId, string targetBranch); // new
|
||||||
|
public Task<MergeResultDto> 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<MergeResultDto?> ApproveReviewAsync(string taskId, string targetBranch)`.
|
||||||
|
- Add `Task<MergePreviewDto?> PreviewMergeAsync(string taskId, string targetBranch)`.
|
||||||
|
- Add `Task<MergeResultDto> 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).
|
||||||
Reference in New Issue
Block a user