Merge claudedo/24acb89f61094773a0dc76d31bdae274

2026-08-06 11:43:19 +02:00
parent abac5e5150 db4177db83
commit 46e177eb59
1 changed files with 235 additions and 0 deletions
@@ -0,0 +1,235 @@
 # Planning-Chain Children: Fork Base Commit (Design Options, No Implementation)
 **Date:** 2026-08-06
 **Status:** Options evaluated, recommendation given — decision pending (Mika)
 **Scope:** Analysis only. No production code changed by this document.
 ## Problem
 The planning chain gives **ordering**, not **code inheritance**. `PlanningChainCoordinator.SetupChainAsync`
 (`src/ClaudeDo.Worker/Planning/PlanningChainCoordinator.cs:36-72`) links children via
 `BlockedByTaskId` (child[i] blocks on child[i-1]) so they run one at a time. But each child's
 worktree is still created from **main's current HEAD**, not from the predecessor's branch, so
 child N+1 never sees child N's (unmerged) work.
 **Observed failure (unit `44241dcb`, "Installer: Environment-Checks", 2026-08-05):** 7 of 9
 children succeeded; the two that depended on a sibling's output could not:
 - `4e196058` ("Claude Help Me" button) — reported: *"the task spec assumes an existing
  SystemCheckPage/EnvironmentCheckReport/ExecutableResolver, but that code only exists on an
  unmerged sibling branch (claudedo/06aca9b3...). A fast-forward merge of that branch was
  attempted ... but was denied twice by the auto-mode permission classifier. No implementation
  work was done."*
 - `c22cdd06` (Diagnose section) — *"Blocked before implementation could start."*
 Both correctly reported `CLAUDEDO_BLOCKED` and committed nothing (`ahead: 0`). A second-order
 symptom of the same root cause: `ExecutableResolver.cs` was independently created **twice**
 (`40272c0b`, `06aca9b3`, near-identical) and collided as an add/add conflict at unit-merge time.
 ## Ist-Zustand (verified against commit `816f247`, 2026-08-06)
 ### Where the base commit is chosen
 `WorktreeManager.ResolveBaseCommitAsync` — `src/ClaudeDo.Worker/Runner/WorktreeManager.cs:191-207`:
 ```csharp
 private async Task<string> ResolveBaseCommitAsync(TaskEntity task, string workingDir, CancellationToken ct)
 {
    if (task.ParentTaskId is not null)
    {
        var parent = ...;
        if (parent is not null && parent.PlanningPhase == PlanningPhase.None)
        {
            var parentWt = await new WorktreeRepository(ctx).GetByTaskIdAsync(task.ParentTaskId, ct);
            var parentHead = parentWt?.HeadCommit ?? parentWt?.BaseCommit;
            if (parentHead is not null)
                return parentHead;
        }
    }
    return await _git.RevParseHeadAsync(workingDir, ct);   // planning children land here
 }
 ```
 **A "don't fork from main" mechanism already exists** — but only for *improvement* children
 (a non-planning parent's own follow-up subtasks), which base off `task.ParentTaskId`'s worktree
 HEAD. The guard `parent.PlanningPhase == PlanningPhase.None` explicitly **excludes** planning
 children: their `ParentTaskId` points at the planning parent (which has no worktree of its own),
 not at a sibling, so this branch never fires for them and they fall through to `RevParseHeadAsync`
 = main HEAD. This is a deliberate exclusion in the existing code, not an oversight — it simply
 never anticipated that a planning child's *predecessor in the chain* (not its parent) might be
 the thing to inherit from.
 Called from `WorktreeManager.CreateAsync` (`:35`), invoked by `TaskRunner` (`Runner/TaskRunner.cs:309`)
 at the moment a task transitions to `Running` — i.e. worktree creation happens per-run, not once
 at plan-finalize time.
 ### Children already run strictly sequentially — this is not a new constraint
 `SetupChainAsync` sets `BlockedByTaskId` on every child but the first
 (`Planning/PlanningChainCoordinator.cs:61-69`); the queue picker only claims rows with
 `BlockedByTaskId IS NULL`. `OnChildFinishedAsync` (`:92-120`) unblocks the successor **only**
 after the predecessor reaches `Done` (and cascades cancellation down the chain on
 Failed/Cancelled). `OverrideSlotService.RunNow` (`Queue/OverrideSlotService.cs:32-42`) is the one
 path that bypasses this — it calls `StartRunningAsync` directly with no `BlockedByTaskId` check,
 so a user can manually force a later child to run out of turn.
 Net: under the normal (queue-driven) path, by the time child N+1's worktree is created, child N
 is already terminal. Forking N+1 from N's branch tip instead of main's HEAD does **not** introduce
 any new concurrency constraint on the normal path — the sequencing already exists. It only matters
 for the `RunNow` bypass edge case (see Option 1 below).
 ### The merge side already treats the unit as a sequential chain, twice over
 `PlanningMergeOrchestrator.DrainAsync` (`Planning/PlanningMergeOrchestrator.cs:183-229`) merges
 `Done` children into `targetBranch` **one at a time, in `SortOrder`**, via
 `TaskMergeService.MergeAsync`; the first conflict pauses the whole drain
 (`PlanningMergeConflict`, state kept for `ContinueAsync`/`AbortAsync`), and the first
 non-conflict failure **aborts the drain outright** — remaining children are never merged and the
 parent never reaches `Done` (`:208-215`).
 `PlanningAggregator.BuildIntegrationBranchAsync` (`Planning/PlanningAggregator.cs:81-126`) — used
 for the pre-approve combined-diff preview — does the same thing again: builds a scratch
 integration branch off `targetBranch` and `MergeNoFfAsync`s each child's branch in, in
 `SortOrder`, stopping at the first conflict.
 So **both** the actual unit-merge and its preview already model the child set as an ordered
 sequence where one bad link can stall everything downstream. The only place in the whole pipeline
 that still treats planning children as N independent forks of main is worktree creation.
 ## Options evaluated
 ### Option 1 — child forks from the predecessor's branch
 Extend `ResolveBaseCommitAsync` (or a planning-specific sibling of it) to also handle planning
 children: look up the chain predecessor via `BlockedByTaskId` (not `ParentTaskId` — that points at
 the planning parent, which has no worktree) and use its `WorktreeEntity.HeadCommit ?? BaseCommit`
 the same way the improvement-child path already does.
 **What actually breaks, given the Ist-Zustand above:**
 - *Not* a new concurrency constraint (see above) — the chain already serializes execution.
 - *Not* a new "one failure blocks everyone" behavior at merge time — `DrainAsync` already aborts
  the whole drain on the first non-conflict failure, and the integration-branch preview already
  stops at the first conflict. Option 1 brings execution-time behavior in line with what merge-time
  behavior already is, rather than introducing a new failure mode.
 - **Genuinely new:** child N+1's branch now contains N's commits as ancestors. When N is later
  merged into `targetBranch` with `--no-ff` and N+1 is merged afterward, N+1's diff against N's
  content is empty (identical trees) — merges cleanly as a no-op for that slice, verified by the
  same mechanism `DrainAsync` already uses. No new conflict class is introduced; if anything this
  *removes* one: the `ExecutableResolver.cs` add/add collision would not have occurred, since N+1
  would start from a tree that already has the file.
 - **Genuinely new edge case:** `OverrideSlotService.RunNow` on a chain member whose predecessor
  hasn't produced a `WorktreeEntity`/`HeadCommit` yet. Needs an explicit fallback to main HEAD —
  the same `?? ` pattern the improvement-child branch already uses for a predecessor with no
  `HeadCommit` (never committed) covers most of this; a predecessor that was never even *run* needs
  the same fallback-to-`RevParseHeadAsync` the code already falls through to today.
 - **Genuinely new edge case:** a discarded/failed predecessor's worktree row still carries its last
  `HeadCommit`/`BaseCommit` (the row isn't deleted on discard, only `State` flips) — same as the
  improvement-child path already tolerates, so no new handling needed there.
 - Children still go straight to `Done` with no individual review (Unified Parent Model), so there
  is no "reject and rewrite a middle child after a successor already forked from it" scenario to
  worry about — that action doesn't exist in the current state machine.
 **Price:** deeper branch chains (cosmetic — they're squashed away by the sequential merge/prune
 regardless); one new fallback path for the `RunNow` bypass. Meaningfully smaller than it first
 looks, because it extends an existing pattern (improvement children) into a lane whose execution
 and merge sides are *already* sequential — it only fixes the one place that wasn't.
 ### Option 2 — merge predecessor to main immediately on success, before the successor starts
 Keeps children independent worktrees (forked from main-as-it-is-now, updated after each merge),
 but requires a real merge to `main` per child, outside of Approve.
 **Breaks:**
 - Directly contradicts the documented invariant "**Approve is the single review+merge action**"
  (`src/ClaudeDo.Worker/CLAUDE.md` → Status Model; `docs/explore-notes/review-merge.md` → "Approve
  = merge the whole unit"). Unreviewed work would land on `main` automatically, mid-chain, before
  the parent — or the user — ever sees it.
 - Requires calling `TaskMergeService.MergeAsync` from `OnChildFinishedAsync` (state-transition
  code), duplicating what `PlanningMergeOrchestrator` already owns, and doing it against a
  `VerifyCommand`-gated repo N times instead of once — if the gate is configured, a mid-chain
  verify failure now has to be handled somewhere there's currently no error path for it (parent is
  still `WaitingForChildren`, not under merge orchestration yet).
 - Unavoidably touches `TaskStateService`/status-transition semantics, which this task's scope
  explicitly excludes ("Die Status-Logik oder `TaskStateService` anfassen" is out of scope) — this
  option cannot be implemented without doing exactly that.
 **Verdict:** rejected outright — it isn't just costly, it conflicts with a stated architectural
 invariant and this task's own non-goals.
 ### Option 3 — child may merge the predecessor's branch into its own worktree if it needs to
 Least invasive to the base-commit mechanism; delegates the decision to the agent at runtime.
 **Breaks:**
 - This is *exactly* what the blocked child in the observed incident already tried, and it was
  denied twice by the auto-mode permission classifier. The real blocker isn't a missing
  capability, it's that `git merge` trips the classifier's "risky/hard-to-reverse" heuristic
  regardless of target — even though a merge confined to the task's own isolated worktree (never
  touching the shared `workingDir`) is materially lower-risk than a merge on a shared checkout.
  Fixing this option means carving a narrower permission rule (allow `git merge` only inside a
  path under the task's own worktree root), which is a security-policy change, not a merge-model
  change.
 - Even with permission granted, it still relies on the agent (a) noticing it's missing a
  prerequisite, (b) correctly identifying which sibling branch has it, and (c) successfully
  resolving any conflict that surfaces — three separate failure points per occurrence, decided
  fresh by an LLM each time rather than encoded once in the pipeline.
 - Doesn't help children that fail *before* they'd even think to look (e.g., a child whose first
  action is reading a file that doesn't exist yet has no signal to act on).
 **Verdict:** possible as a narrow permission-scope fix layered *on top of* Option 1 (so an agent
 that still needs something from further back than its immediate predecessor isn't stuck), but not
 a substitute for it — on its own it reproduces the exact failure mode from the incident, just with
 one fewer denial.
 ### Option 4 — change nothing; require independent children at planning time
 Zero code changes; pushes the constraint into plan quality.
 **Breaks:**
 - No enforcement exists today (`PlanningSessionManager` doesn't validate structural independence
  between proposed subtasks), and reliably detecting "child B depends on code child A will write"
  from a plan draft is itself an unsolved code-review problem — not something a prompt tweak
  guarantees.
 - A shared-foundation-plus-N-consumers decomposition (exactly the `44241dcb` shape: environment
  checks feeding two UI consumers) is often the *correct* decomposition, not a planning mistake.
  Banning it either forces over-merging into one giant child (defeating the purpose of splitting)
  or requires the planner to reject a structurally sound plan.
 - Reproduces the observed failure verbatim under the same conditions: nothing about this option
  would have caught `44241dcb` before it shipped two dead children.
 **Verdict:** cheapest to write down, but doesn't fix the problem — it relocates it to "the plan
 looked independent but wasn't," which is what already happened.
 ## Recommendation
 **Option 1**, with the `RunNow`-bypass fallback described above, and with Option 3's narrower
 permission-scope fix as an optional follow-up (not a prerequisite) for cases where a child needs
 something from further back in the chain than its immediate predecessor.
 **Why:** Option 1 is not a new mechanism — it's closing the one gap in a pattern that already
 exists twice in this codebase: `WorktreeManager.ResolveBaseCommitAsync` already forks improvement
 children from their parent's HEAD instead of main, and both `PlanningMergeOrchestrator.DrainAsync`
 and `PlanningAggregator.BuildIntegrationBranchAsync` already treat the child set as an ordered
 merge sequence where the first failure stalls everything after it. Planning-child worktree
 creation is the outlier, not the rule. Extending the existing predecessor-lookup pattern (keyed
 off `BlockedByTaskId` instead of `ParentTaskId` for this lane) makes the "sees predecessor's work"
 guarantee hold everywhere the chain already implies it, without touching `TaskStateService`, the
 review model, or introducing a merge-time failure mode that doesn't already exist. Options 2 and 4
 either conflict with a stated invariant/this task's own non-goals, or fail to address the incident
 at all; Option 3 alone reproduces the incident's exact failure.
 **Price to pay knowingly:** a chain member that never got a chance to run (no `WorktreeEntity` yet)
 needs an explicit main-HEAD fallback when its successor is forced via `RunNow` — a few lines,
 mirroring the null-coalescing fallback the improvement-child path already has. No other new failure
 surface was found.
 ## Explicitly out of scope (per task)
 - Implementing Option 1 or any other option — Mika decides first.
 - Any change to `TaskStateService` or status-transition logic.
 - Changing blocked-child visibility/behavior — that's task `001ee94a`, running in parallel; it only
  touches MCP visibility, no production code overlap with this document.