ClaudeDo/docs/explore-notes/review-merge.md

Review, merge & conflict resolution

Explore-note — verify before trusting. Distilled map of a subsystem, not authoritative. Last verified against fc9df7f (2026-08-11), which added the conventional merge-commit default and the MergeProgress broadcast. Planning mode renders per file and DiffLinesView is retired. Drift check: git log --oneline 20bce9b..HEAD -- src/ClaudeDo.Worker/Lifecycle src/ClaudeDo.Worker/State src/ClaudeDo.Worker/Planning src/ClaudeDo.Ui/ViewModels/Conflicts src/ClaudeDo.Worker/External 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.

A blocked planning/improvement child still goes straight to Done per the unified parent model above — it committed nothing, but nothing prevents its (empty) branch from being unit-merged like any other Done child once the parent is approved. The MCP surface has no UI equivalent of ChildOutcomes, so review_task's approve on a parent additionally returns emptyChildren (the Done children whose review range is empty) and every task-returning tool exposes TaskRefDto.roadblockCount → external-mcp.md → ReviewTask/PreviewMerge. An empty branch is still mergeable by design (some tasks — e.g. an audit — legitimately produce no diff); this is a visibility fix, not a merge gate.

Cancel is blocked while a unit merge is draining

ApproveReview on a task with children awaits PlanningMergeOrchestrator.StartAsync / DrainAsync synchronously — the parent sits in WaitingForReview for the whole (potentially minutes-long, one-child-at-a-time) drain. Without a guard, a concurrent CancelReview (UI or update_task_status/cancel_task via MCP) could flip the parent to Cancelled mid-drain while the orchestrator kept merging children's worktrees onto the target branch; FinalizeParentDoneAsync then finds the parent no longer WaitingForReview and gives up, leaving the merged children's diffs stranded with no rollback.

TaskStateService.CancelAsync now rejects with a TransitionResult reason whenever PlanningMergeOrchestrator.HasActiveMerge(taskId) is true for the task being cancelled, before any DB write. Cycle note: TaskStateService can't take a direct constructor dependency on PlanningMergeOrchestrator (which itself depends on ITaskStateService), so it takes a lazily-resolved Func<IActiveMergeState> instead — same cycle-breaking shape as the existing Func<ITaskStateService> handed to PlanningChainCoordinator. IActiveMergeState (Planning/Interfaces/) is PlanningMergeOrchestrator's only public surface TaskStateService needs.

The UI mirrors this as polish: DetailsIslandViewModel.IsMergeDraining (set on PlanningMergeStartedEvent, cleared on PlanningMergeAborted/PlanningCompleted for the bound task) gates CancelReviewCommand's CanExecute — same shape as WorktreesOverviewModalViewModel.IsMerging gating CanMergeAll. The worker-side guard remains the actual correctness fix; the button gate just avoids inviting a click the worker would reject. CancelReviewAsync's catch now raises ErrorReported (→ shell FlashFooterError) instead of swallowing the rejection silently.

Merge commit message

MergeAsync takes a commitMessage, but every caller passes it blank and lets TaskMergeService.DefaultMergeMessage build CommitMessageBuilder.BuildMerge(task.CommitType, list.Name, task.Title, task.Id) → fix(my-list): merge <title> + ClaudeDo-Task: trailer. Same type(scope) shape as the task's own worktree commits, so merges stay Conventional-Commits-clean. The only non-blank caller is the user editing the field in the merge modal, which is prefilled from GetMergeTargets().DefaultCommitMessage (the UI can't build it — it knows neither the commit type nor the list name). Callers used to hand-roll "Merge task: {title}" / "Merge {branch}" / "Merge subtask"; don't add a fourth.

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.

The gate is what makes a merge look broken. A verify command that builds and tests a solution occupies the whole MergeTask hub call — measured at ~5m 46s on this repo. TaskMergeService therefore broadcasts MergeProgress(taskId, phase, elapsedSeconds): PhaseMerging before the per-repo gate wait, PhaseVerifying when the verify starts and again on every ProgressReportInterval tick (30 s, shared with the MCP progress reports), plus one WorkerLog Info line so the footer strip shows it too. The merge modal renders that as a spinner plus phase text; without it the only feedback was the disabled Submit button, which reads as a dead app for minutes while the merge has in fact already landed.

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.

Verify in the preview, not just post-merge

TaskMergeService.PreviewAsync(taskId, targetBranch, runVerify, ct) can additionally build/test a clean git merge-tree result before anything is merged. It never touches the real working tree: the tree PreviewMergeAsync would write is wrapped in a throwaway commit (GitService.CommitTreeAsync, parent = the target branch's current tip) and checked out into a detached-HEAD scratch worktree under the OS temp dir (GitService.WorktreeAddDetachedAsync), which is always removed afterward (finally, non-cancellable cleanup). No verify command configured, or runVerify=false, reproduces the pre-existing preview exactly — no delay, no scratch worktree.

Wired into ExternalMcpService: preview_merge (single task) always requests a verify run when the list has a command configured; preview_merge_set only does when its runVerify parameter is explicitly set (default false) — a set preview never starts N builds unasked. The Hub's PreviewMerge (the UI's live mergeability indicator) always passes runVerify: false, since running a build on every poll would be a bad regression; MergePreviewDto carries the verify fields anyway so a future UI entry point can opt in. Result fields: VerifyExitCode (null = not run; -1 = timed out or failed to start; mirrors the post-merge gate's convention), VerifyDurationMs, VerifyOutputTail (tail of output, only populated on non-zero exit, via the same TailOutput helper the post-merge gate uses).

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 → 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 MergeFileSegments 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 DiffFileViewModels, detecting added/deleted/renamed/binary files and per-line numbers. 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 + one editor per file right (PlanningFiles), combined integration-branch toggle.

Both modes render through DiffAlignment (pure — pairs diff lines into side-by-side rows and computes word-diff spans) and DiffTextView (AvaloniaEdit + TextMate highlighting keyed off the file extension, unified/split layout, optional line wrap, synced scrolling). Files mode hosts one DiffTextView for the selected file; Planning mode hosts one per file in PlanningFiles so each gets its own grammar (one editor can only carry one TextMate grammar). The split/wrap toggles persist to ui.config.json via AppSettings and reach the per-file editors in Planning mode too.