Files
ClaudeDo/docs/superpowers/specs/2026-08-06-planning-chain-fork-base-design.md
T

236 lines
14 KiB
Markdown

# 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.