mika kuns
79f90a9a8e
TaskStateService.CancelAsync allowed cancelling a WaitingForReview task even while PlanningMergeOrchestrator was mid-drain on it: ApproveReview awaits the whole multi-subtask merge synchronously, so a concurrent CancelReview (UI or MCP) could flip the parent to Cancelled while the orchestrator kept merging children onto the target branch, then FinalizeParentDoneAsync would find the parent no longer WaitingForReview and give up - leaving the merged diffs stranded with no rollback. CancelAsync now rejects with a clear reason when HasActiveMerge(taskId) is true. TaskStateService can't take PlanningMergeOrchestrator as a direct constructor dependency (circular back to ITaskStateService), so it takes a lazily-resolved Func<IActiveMergeState> instead, mirroring the existing Func<ITaskStateService> cycle-break already used for PlanningChainCoordinator. UI polish: DetailsIslandViewModel.IsMergeDraining gates CancelReviewCommand's CanExecute (same shape as WorktreesOverviewModalViewModel.IsMerging), and the command's catch now raises ErrorReported instead of swallowing the rejection silently.
234 lines
14 KiB
Markdown
234 lines
14 KiB
Markdown
# Review, merge & conflict resolution
|
||
|
||
> **Explore-note — verify before trusting.** Distilled map of a subsystem, not authoritative.
|
||
> Last verified against commit `0d1e3b9` (2026-08-06).
|
||
> 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](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.
|
||
|
||
## 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.
|