mika kuns
5d1d2d89d0
review_task/continue_merge on a planning parent always leaves conflicts in the tree, and the UI auto-opened the resolver on every PlanningMergeConflict broadcast regardless of who started the merge -- so a running Claude session resolving a unit-merge conflict could race a human editing the same shared checkout in a resolver window neither of them asked for. PlanningMergeOrchestrator.StartAsync now takes an externallyDriven flag (set by ExternalMcpService's MCP-driven review_task path, left false for the UI's ApproveReview) that rides along on the PlanningMergeConflict broadcast. The UI only auto-opens the resolver when it's false; otherwise it shows a persistent banner with a manual "Open resolver" button, cleared on PlanningMergeAborted/PlanningCompleted. A new GetActiveExternalConflictsAsync query (checked against GitService.IsMidMergeAsync rather than the in-memory flag alone) lets the UI resync the banner on reconnect instead of trusting a one-shot broadcast that isn't replayed after a restart. The childless single-task conflict path was checked and needed no change -- it only broadcasts the generic TaskUpdated, never PlanningMergeConflict.
200 lines
12 KiB
Markdown
200 lines
12 KiB
Markdown
# Review, merge & conflict resolution
|
||
|
||
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
||
> Last verified against commit `8247a74` (2026-08-06).
|
||
> Drift check: `git log --oneline f6cb825..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts`
|
||
> Stable structure only (no line numbers). See docs/explore-notes/README.md.
|
||
|
||
Covers the review→merge path: `TaskStateService` review transitions, `TaskMergeService`,
|
||
`PlanningMergeOrchestrator`, the post-merge verify gate, and the UI conflict resolver.
|
||
|
||
## Approve = merge the whole unit
|
||
|
||
`ApproveReview` (hub) and `review_task` approve (MCP) are the **single** review+merge action.
|
||
There is no separate "Merge all" entry.
|
||
|
||
- **Task with children** → drives `PlanningMergeOrchestrator`: merges the parent worktree if
|
||
`Active`, then each `Done` child in order, then sets the parent `Done`. A mid-merge conflict
|
||
pauses for `ContinuePlanningMerge` / `AbortPlanningMerge`.
|
||
- **Childless task** → `TaskMergeService.ApproveAndMergeAsync`. A conflict keeps the task in
|
||
`WaitingForReview`.
|
||
- **No active worktree** (sandbox run) → straight to `Done`.
|
||
|
||
Review transitions all live in `TaskStateService`: `SubmitForReviewAsync`,
|
||
`SubmitForChildrenAsync`, `ApproveReviewAsync`, `RejectToQueueAsync`, `RejectToIdleAsync`,
|
||
`ClearReviewFeedbackAsync`.
|
||
|
||
`ReviewFeedback` (nullable string on `TaskEntity`) is the reviewer's rejection comment: set by
|
||
`RejectToQueueAsync`, consumed and cleared by `QueueService` on the next re-run, where it
|
||
becomes the next-turn prompt of the resumed Claude session.
|
||
|
||
## Unified parent model
|
||
|
||
Every parent — planning **or** improvement — flows
|
||
`… → WaitingForChildren → WaitingForReview → Done`, advanced by the single
|
||
`TaskStateService.TryAdvanceParentAsync`. It surfaces any `WaitingForChildren` parent for
|
||
review once all children are terminal; failed/cancelled children are **annotated on the
|
||
result, not wedged**.
|
||
|
||
- A planning parent enters `WaitingForChildren` at `FinalizePlanningAsync` (or
|
||
`WaitingForReview` directly if it has no children).
|
||
- An improvement parent enters it from `TaskRunner.HandleSuccess` when its run spawned children.
|
||
- Planning/improvement **children** go straight to `Done` — no individual review. Only the
|
||
parent is reviewed.
|
||
|
||
A child that hits a roadblock (fails, or reports `CLAUDEDO_BLOCKED` roadblocks) does **not**
|
||
advance the parent — the parent stays in `WaitingForChildren` until every child is terminal.
|
||
The UI surfaces blocked children on the parent's Session tab (`ChildOutcomes` + a "children
|
||
need attention" band) so the roadblock is visible without forcing a transition.
|
||
|
||
## Post-merge verify gate
|
||
|
||
A list can set `ListConfigEntity.VerifyCommand` (List Settings modal → Verification).
|
||
Null/blank (the default) = **no gate**, behavior bit-identical to before the feature existed.
|
||
|
||
When set, `TaskMergeService` runs it via `VerifyCommandRunner` (`cmd.exe /c <command>`,
|
||
10-minute fixed timeout, output tail-captured) in `list.WorkingDir` right after a successful
|
||
`MergeNoFfAsync` / `ContinueMergeAsync` **and** worktree cleanup, but **before** the task is
|
||
allowed to reach `Done`.
|
||
|
||
| Outcome | Effect |
|
||
|---|---|
|
||
| Exit 0 | Unchanged flow — worktree `Merged`, task `Done` if it was `WaitingForReview`. |
|
||
| Non-zero exit or timeout | The git merge is **deliberately left in place** (no auto-revert — that's a separate, unbuilt feature). The worktree is still marked `Merged` (it's already gone from disk when `removeWorktree` was requested), but the task stays out of `Done`. |
|
||
|
||
On failure `MergeResult.Status` comes back `TaskMergeService.StatusVerifyFailed`
|
||
(`"verify_failed"`) with an output excerpt in `ErrorMessage`. This flows through
|
||
`MergeResultDto` (hub) and `ReviewTaskResult` (`review_task`) unchanged, because both already
|
||
treat any non-`blocked`/`conflict` status generically. All three UI merge entry points handle it
|
||
explicitly (detail-pane Approve, merge modal, worktrees batch) — a generic fallback there showed
|
||
the raw status string instead of the failure.
|
||
|
||
**Worktree-less approvals are gated too.** A task with no active `WorktreeEntity` — a sandbox run,
|
||
or a list-handler task that commits straight into `list.WorkingDir` — skips the merge entirely,
|
||
but `ApproveAndMergeAsync` still runs the verify command (same per-repo gate, working dir =
|
||
`list.WorkingDir`) before the task may reach `Done`. Without that, the run that lands the most on
|
||
the target branch at once would be the one run nothing checks.
|
||
|
||
**Serialization:** a process-wide `ConcurrentDictionary<string, SemaphoreSlim>` keyed by
|
||
`list.WorkingDir` serializes `MergeAsync` / `ContinueMergeAsync` (git ops + verify) per repo,
|
||
so a verify run can't be interrupted by a second merge landing in the same working dir
|
||
mid-build.
|
||
|
||
## `MergeCommit` and revert
|
||
|
||
`WorktreeEntity.MergeCommit` (nullable) is the SHA of the merge commit this worktree's branch
|
||
produced on the target branch. Stamped by `TaskMergeService` the moment a merge/continue-merge
|
||
succeeds, written **only** by `WorktreeRepository.SetMergedAsync` (which atomically sets
|
||
`State=Merged` and stamps the SHA in one update).
|
||
|
||
It is the only thing that makes `revert_merge` possible without heuristically searching
|
||
`git log` — see [external-mcp.md](external-mcp.md) → `RevertMerge`. Null for any worktree
|
||
merged before the field existed.
|
||
|
||
## Review gate in the UI
|
||
|
||
**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. The gate **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**.
|
||
|
||
Implementation: `MergeSectionViewModel` owns merge-target selection, the mergeability
|
||
indicator (`MergePreviewPresenter` over `PreviewMergeAsync`), and `OpenDiffAsync` /
|
||
`ReviewCombinedDiffCommand` — both build a `DiffViewerViewModel`, call `ShowDiffViewer`, and
|
||
fire the `DiffViewed` callback. `HasReviewableDiff` reports whether anything is inspectable
|
||
and feeds the gate.
|
||
|
||
## Conflict resolver (in-app Rider-style 3-pane merge editor)
|
||
|
||
`ConflictResolverViewModel` + `Views/Conflicts/ConflictResolverView`. Handles **both**
|
||
single-task and planning unit-merge conflicts.
|
||
|
||
### Model
|
||
|
||
Single-task mode starts the conflict merge, then parses each conflicted file into stable and
|
||
conflict `MergeFileSegment`s via the worker's `GetMergeConflictDocuments`. Types live in
|
||
`ConflictModels`: `MergeFile` / `MergeFileSegment` / `MergeConflictBlock`.
|
||
|
||
Exposed per active file: `ActiveOursText` / `ActiveResultText` / `ActiveTheirsText`
|
||
(reconstructed from `MergeFile.OursText/ResultText/TheirsText`; Result seeds unresolved
|
||
conflicts with Ours), plus `ActiveFile` / `SelectFileCommand` (multi-file switcher),
|
||
`Current` / `Next` / `Previous` (focused-conflict nav), a per-file `PositionText` readout,
|
||
per-block `AcceptOurs/Theirs/Both/Base` + `MergeFile.Compose`, and `CanContinue` gated on
|
||
**every file resolved + no binary**. Each file is written via `WriteConflictResolution`.
|
||
|
||
**Planning mode** via `OpenForPlanningAsync(parentId, subtaskId)` loads the current subtask's
|
||
mid-merge conflicts **without re-starting the merge** and routes continue/abort to
|
||
`ContinuePlanningMerge` / `AbortPlanningMerge`, so a unit-merge conflict re-opens the editor
|
||
per subtask via the `PlanningMergeConflict` broadcast.
|
||
|
||
**Except when an MCP session is driving the merge.** `PlanningMergeOrchestrator.StartAsync` takes
|
||
an `externallyDriven` bool (default `false`; `ExternalMcpService.review_task`'s parent-with-children
|
||
path passes `true`, since a running Claude session — not the UI — will resolve conflicts via
|
||
`continue_merge`/`abort_merge`). The flag rides along on the `PlanningMergeConflict` broadcast
|
||
(4th arg); `IslandsShellViewModel.OnPlanningMergeConflict` only auto-opens the resolver when it's
|
||
`false` — otherwise it shows a persistent, non-auto-dismissing banner
|
||
(`IsExternalMergeBannerVisible`) with a manual "Open resolver" button, cleared on
|
||
`PlanningMergeAborted`/`PlanningCompleted`. This exists because two parties (a human and the
|
||
driving session) could otherwise end up editing the same shared checkout at once.
|
||
`PlanningMergeOrchestrator.GetActiveExternalConflictsAsync` (hub: `GetActiveExternalPlanningMergeConflicts`)
|
||
re-derives this state by checking `GitService.IsMidMergeAsync` rather than trusting the in-memory
|
||
flag alone, so a UI restart mid-merge (or a stale entry left behind if something resolved the
|
||
repo outside the normal Continue/Abort path) can't show a phantom banner — the Ui calls it on
|
||
`ConnectionRestoredEvent`. The childless single-task conflict path
|
||
(`TaskMergeService.ApproveAndMergeAsync`) has no equivalent broadcast — it only sends the generic
|
||
`TaskUpdated` — so it never auto-opened the resolver and needed no change.
|
||
|
||
### View
|
||
|
||
Three **AvaloniaEdit** panes showing the whole file: MAIN/ours (read-only) | editable Result |
|
||
INCOMING/theirs (read-only). TextMate highlighting by extension (theme `StyleInclude` in
|
||
`App.axaml`).
|
||
|
||
- A code-behind `IBackgroundRenderer` tints each conflict block (unresolved/resolved) across
|
||
panes. Tints live in `Tokens.axaml` (`Merge*TintBrush`).
|
||
- An `IReadOnlySectionProvider` + `TextAnchor` regions keep **only conflict spans** editable in
|
||
Result; edits flow back to the block.
|
||
- Each unresolved conflict starts **EMPTY** (a thin marker bar).
|
||
- The between-pane gutter controls **toggle** each side in/out of the result: `›`/`‹` add
|
||
MAIN/INCOMING in click order (first pick on top), clicking again removes that side — so a
|
||
conflict can take main, incoming, both, or **neither**.
|
||
- `FilesSummary` shows how many files still have conflicts. The three panes share a
|
||
proportional synced vertical scroll.
|
||
- A conflict overview ruler right of the Result pane (`ConflictMap`) maps every conflict in the
|
||
file proportionally; click a tick to jump. Useful for long files.
|
||
|
||
### Entry points
|
||
|
||
Review **Approve** on conflict, and the **Merge** button in the Diff window (a conflicting
|
||
`MergeTask` hands off via `RequestConflictResolution`).
|
||
|
||
## Hub methods
|
||
|
||
- Review/merge: `ApproveReview(taskId, targetBranch) -> MergeResultDto`,
|
||
`ContinuePlanningMerge` / `AbortPlanningMerge`, `PreviewMerge(taskId, targetBranch) ->
|
||
MergePreviewDto`, `RejectReviewToQueue`, `RejectReviewToIdle`, `CancelReview`, `MergeTask`,
|
||
`GetMergeTargets`
|
||
- Single-task conflict resolver: `StartConflictMerge`, `GetMergeConflictDocuments`,
|
||
`WriteConflictResolution`, `ContinueConflictMerge`, `AbortConflictMerge` — note the
|
||
service-level `TaskMergeService.ContinueMergeAsync` / `AbortMergeAsync` keep their own names.
|
||
- Broadcast events: `PlanningMergeStarted`, `PlanningSubtaskMerged`, `PlanningMergeConflict`,
|
||
`PlanningMergeAborted`, `PlanningCompleted`
|
||
|
||
## Diff stack (UI)
|
||
|
||
`UnifiedDiffParser` (static) parses `git diff` output into `DiffFileViewModel`s, detecting
|
||
added/deleted/renamed/binary files and per-line numbers; `Flatten` injects file-header rows for
|
||
a combined single-pane view. `DiffModels.cs` holds the shared types (`DiffLineViewModel`,
|
||
`DiffFileViewModel`, `DiffLineKind`, `DiffFileStatus`, `SubtaskDiffRow`,
|
||
`DiffTreeNodeViewModel`, `DiffTree`).
|
||
|
||
`DiffViewerViewModel` is one unified read-only viewer with two modes:
|
||
- **Files** — dirty worktree / branch-vs-base / commit-range. Loads via `GitService`, folder
|
||
file-tree left + per-file diff pane right, Merge button for a live branch source.
|
||
- **Planning** — per-subtask diffs via `GetPlanningAggregateAsync`, subtask list left + flat
|
||
diff right, combined integration-branch toggle.
|
||
|
||
`DiffLinesView` renders per-file content with binary/empty placeholders.
|