diff --git a/docs/superpowers/specs/2026-08-06-planning-chain-fork-base-design.md b/docs/superpowers/specs/2026-08-06-planning-chain-fork-base-design.md new file mode 100644 index 00000000..d6cf8d0c --- /dev/null +++ b/docs/superpowers/specs/2026-08-06-planning-chain-fork-base-design.md @@ -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 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.