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

175 lines
9.6 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# Review, merge & conflict resolution
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
> Last verified against commit `f6cb825` (2026-08-05).
> 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.
**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.
### 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.