docs: add implementation plan for nested-builder-completion-follows-arbitration (piece 4b-1)

1 files changed, 331 insertions, 0 deletions

diff --git a/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md b/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md
new file mode 100644
index 0000000..53537e8
--- /dev/null
+++ b/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md
@@ -0,0 +1,331 @@
+# Nested Builder Completion Follows Arbitration Implementation Plan
+
+> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
+
+**Goal:** A nested (`ParentTaskID != ""`) task with `Agent.Role == "builder"` never reaches `COMPLETED` via the executor's own default completion path — it goes to `READY` instead, exactly mirroring the rule piece 4a already applied to the story root. Every other task (no role, or a non-`builder` role) keeps today's exact behavior.
+
+**Architecture:** `internal/executor.Pool` currently has two places that write `task.StateCompleted` for a nested task: `handleRunResult`'s "no more subtasks, has a parent" branch, and `maybeUnblockParent`'s own "all this parent's children are done, promote it too" branch (since a nested roll-up parent, once unblocked, is itself promoted straight to `COMPLETED`, cascading further up). Both get consolidated into one new helper, `promoteNestedTask`, which is the single place that decides READY-vs-COMPLETED for a nested task and owns triggering the parent-unblock cascade (only in the COMPLETED case). Executor stays completely story-agnostic: it has no idea what a "story" or "arbitrated review" is, it just treats the string `"builder"` in `Agent.Role` as meaning "this task's completion requires external approval before it can be trusted" — the same rule `internal/scheduler.StoryOrchestrator` already applies to a story's root task.
+
+**Tech Stack:** Go, `internal/executor` package only. No changes to `internal/scheduler`, `internal/task`, or any other package in this plan — that's piece 4b-2/4b-3, deliberately deferred (this plan does not export anything new; nothing outside `internal/executor` calls this new behavior yet).
+
+## Global Constraints
+
+- This is piece 4b-1 of `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`'s piece 4, split out as the smaller, safer first step of piece 4b (which itself was split out of piece 4 during grounding work, once it became clear piece 4 requires touching `executor.Pool`'s core nested-task completion path, not just `internal/scheduler`).
+- Root cause this plan closes: `internal/executor.Pool.handleRunResult` currently promotes ANY nested task straight to `COMPLETED` the instant its own execution finishes with no further children — including a `builder`-role subtask spawned via `spawn_subtask`, with zero review of any kind. `internal/scheduler.StoryOrchestrator` never even sees nested subtasks (it only watches `story.RootTaskID` and its top-level DAG-sibling dependents), so today a builder-role subtask's "COMPLETED" carries no more meaning than any other task's — exactly the gap the story root had before piece 4a, one level lower in the stack where the fix must actually live.
+- The `Agent.Role == "builder"` check is a plain string comparison against the existing `evaluatorRoles`/`arbitrationRole`/`retroRole` constants pattern already used in `internal/scheduler/story_orchestrator.go` (this plan does not add a new shared constant across packages — `internal/executor` importing `internal/scheduler` would be a layering violation the wrong direction; a literal `"builder"` string here is the same tolerance the codebase already has for `"builder"` appearing as a literal in `ensureFixAttempt`'s `d.Agent.Role == "builder"` check).
+- No change to `depDoneStates`, `checkDepsReady`, `cascadeFail`, or anything about how a task's own subtasks are counted/waited-on — only the READY-vs-COMPLETED choice for the task itself, once its own children (if any) are done.
+- A `builder`-role task that no story's tree ever adopts will sit at `READY` forever, with its parent stuck `BLOCKED` forever. This is accepted, not a regression to guard against here — the same category of graceful degradation the codebase already tolerates for a role with no active `role_configs` row (`internal/executor.Pool.execute()` logs a warning and dispatches without role resolution rather than failing).
+
+---
+
+### Task 1: Consolidate nested-task promotion into `promoteNestedTask`, redirecting builder-role tasks to READY
+
+**Files:**
+- Modify: `internal/executor/executor.go` (`handleRunResult`, `maybeUnblockParent`; add `promoteNestedTask`)
+- Test: `internal/executor/executor_test.go`
+
+**Interfaces:**
+- Consumes: `task.StateReady`/`task.StateCompleted` (unchanged), `p.store.UpdateTaskState` (unchanged), `p.maybeUnblockParent` (unchanged signature, now called only from inside `promoteNestedTask`).
+- Produces: `func (p *Pool) promoteNestedTask(t *task.Task) task.State` — new, unexported (no cross-package caller in this plan). Callers pass a task known to have `t.ParentTaskID != ""` and no further subtasks of its own; the function decides READY (builder-role) or COMPLETED (everything else, triggering the cascade) and returns whichever it wrote.
+
+- [ ] **Step 1: Add `promoteNestedTask`**
+
+In `internal/executor/executor.go`, find `maybeUnblockParent`'s doc comment and signature (the function immediately preceding it in the file, `RecoverStaleBlocked`'s neighbor further down — search for `func (p *Pool) maybeUnblockParent(parentID string) {` to locate it), and insert this new function immediately **before** it:
+
+```go
+// promoteNestedTask transitions a nested task (t.ParentTaskID != "", with no
+// further subtasks of its own) to the state its own completion should
+// reach, and returns that state. A builder-role nested task goes to READY,
+// not COMPLETED: builder-role tasks -- roll-up or leaf, root or nested --
+// require arbitrated review before their completion can be trusted (see
+// docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md).
+// executor stays story-agnostic here -- it has no idea what a "story" is,
+// it just treats "builder" as always requiring external approval before
+// COMPLETED, uniformly, the same rule internal/scheduler.StoryOrchestrator
+// already applies to a story's root task (see that package's
+// finalizeArbitration). A builder-role task nobody's story tree ever
+// adopts simply sits at READY (and its parent BLOCKED) forever -- the same
+// category of graceful degradation already tolerated elsewhere in this
+// codebase for a role with no active role_configs row.
+//
+// Every other task transitions to COMPLETED exactly as before this change,
+// and this is the one place that triggers the parent-unblock cascade on
+// t.ParentTaskID -- callers must NOT also call maybeUnblockParent
+// themselves; this method owns that decision entirely.
+func (p *Pool) promoteNestedTask(t *task.Task) task.State {
+	if t.Agent.Role == "builder" {
+		if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil {
+			p.logger.Error("promoteNestedTask: update task state", "taskID", t.ID, "error", err)
+		}
+		return task.StateReady
+	}
+	if err := p.store.UpdateTaskState(t.ID, task.StateCompleted); err != nil {
+		p.logger.Error("promoteNestedTask: update task state", "taskID", t.ID, "error", err)
+		return t.State
+	}
+	p.maybeUnblockParent(t.ParentTaskID)
+	return task.StateCompleted
+}
+
+```
+
+- [ ] **Step 2: Update `handleRunResult`'s nested-completion branch**
+
+In the same file, find (inside `handleRunResult`):
+
+```go
+		if subErr == nil && len(subtasks) > 0 {
+			exec.Status = "BLOCKED"
+			if err := p.store.UpdateTaskState(t.ID, task.StateBlocked); err != nil {
+				p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateBlocked, "error", err)
+			}
+		} else if t.ParentTaskID == "" {
+			exec.Status = "READY"
+			if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil {
+				p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateReady, "error", err)
+			}
+		} else {
+			exec.Status = "COMPLETED"
+			if err := p.store.UpdateTaskState(t.ID, task.StateCompleted); err != nil {
+				p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateCompleted, "error", err)
+			}
+			p.maybeUnblockParent(t.ParentTaskID)
+		}
+```
+
+Replace with:
+
+```go
+		if subErr == nil && len(subtasks) > 0 {
+			exec.Status = "BLOCKED"
+			if err := p.store.UpdateTaskState(t.ID, task.StateBlocked); err != nil {
+				p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateBlocked, "error", err)
+			}
+		} else if t.ParentTaskID == "" {
+			exec.Status = "READY"
+			if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil {
+				p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateReady, "error", err)
+			}
+		} else if p.promoteNestedTask(t) == task.StateReady {
+			exec.Status = "READY"
+		} else {
+			exec.Status = "COMPLETED"
+		}
+```
+
+- [ ] **Step 3: Update `maybeUnblockParent`'s own nested-promotion branch**
+
+In the same file, find (inside `maybeUnblockParent`):
+
+```go
+	if parent.State == task.StateBlocked && parent.ParentTaskID != "" {
+		if err := p.store.UpdateTaskState(parentID, task.StateCompleted); err != nil {
+			p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err)
+			return
+		}
+		p.maybeUnblockParent(parent.ParentTaskID)
+		return
+	}
+	if err := p.store.UpdateTaskState(parentID, task.StateReady); err != nil {
+		p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err)
+	}
+}
+```
+
+Replace with:
+
+```go
+	if parent.State == task.StateBlocked && parent.ParentTaskID != "" {
+		p.promoteNestedTask(parent)
+		return
+	}
+	if err := p.store.UpdateTaskState(parentID, task.StateReady); err != nil {
+		p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err)
+	}
+}
+```
+
+- [ ] **Step 4: Add a test proving a builder-role leaf subtask goes to READY, not COMPLETED**
+
+In `internal/executor/executor_test.go`, find `TestPool_Submit_Subtask_GoesToCompleted` (it starts with `func TestPool_Submit_Subtask_GoesToCompleted(t *testing.T) {`) and immediately after its closing brace, add:
+
+```go
+// TestPool_Submit_BuilderRoleSubtask_GoesToReady_NotCompleted proves the
+// core piece-4b mechanism: a nested builder-role task, once its own
+// execution finishes with no further children, goes to READY -- not
+// COMPLETED -- mirroring the story root's own rule (piece 4a): a
+// builder-role task's completion must mean "verified by arbitration", not
+// merely "the agent finished", uniformly regardless of depth.
+func TestPool_Submit_BuilderRoleSubtask_GoesToReady_NotCompleted(t *testing.T) {
+	store := testStore(t)
+	runner := &mockRunner{}
+	runners := map[string]Runner{"claude": runner}
+	logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError}))
+	pool := NewPool(2, runners, store, logger)
+
+	tk := makeTask("builder-sub-1")
+	tk.ParentTaskID = "parent-99" // subtask
+	tk.Agent.Role = "builder"
+	store.CreateTask(tk)
+
+	if err := pool.Submit(context.Background(), tk); err != nil {
+		t.Fatalf("submit: %v", err)
+	}
+
+	result := <-pool.Results()
+	if result.Err != nil {
+		t.Errorf("expected no error, got: %v", result.Err)
+	}
+	if result.Execution.Status != "READY" {
+		t.Errorf("status: want READY, got %q", result.Execution.Status)
+	}
+
+	got, _ := store.GetTask("builder-sub-1")
+	if got.State != task.StateReady {
+		t.Errorf("task state: want READY (builder-role, awaiting arbitrated review), got %v", got.State)
+	}
+}
+```
+
+- [ ] **Step 5: Add a test proving the cascade stops at a builder-role roll-up**
+
+In the same file, find `TestPool_Submit_GrandchildCompletion_CascadesThroughNestedParents` (its full body, ending at its closing `}`) and immediately after it, add:
+
+```go
+// TestPool_MaybeUnblockParent_BuilderRoleRollup_StaysReady_NotCascaded
+// proves the cascade-side of the same rule: when maybeUnblockParent's own
+// "all children done, promote this BLOCKED nested parent" branch fires for
+// a builder-role roll-up, it too goes to READY (not COMPLETED) -- and,
+// because it never reaches COMPLETED, the cascade must NOT continue past it
+// to the grandparent (root stays BLOCKED, not READY), since the roll-up's
+// own arbitrated review hasn't happened yet.
+func TestPool_MaybeUnblockParent_BuilderRoleRollup_StaysReady_NotCascaded(t *testing.T) {
+	store := testStore(t)
+	runner := &mockRunner{}
+	runners := map[string]Runner{"claude": runner}
+	logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError}))
+	pool := NewPool(2, runners, store, logger)
+
+	root := makeTask("builder-nested-root")
+	root.State = task.StateBlocked // already ran, delegated to middle
+	store.CreateTask(root)
+
+	middle := makeTask("builder-nested-middle")
+	middle.ParentTaskID = root.ID
+	middle.Agent.Role = "builder"
+	middle.State = task.StateBlocked // already ran, delegated to grandchild
+	store.CreateTask(middle)
+
+	grandchild := makeTask("builder-nested-grandchild")
+	grandchild.ParentTaskID = middle.ID
+	store.CreateTask(grandchild) // fresh, about to be submitted
+
+	if err := pool.Submit(context.Background(), grandchild); err != nil {
+		t.Fatalf("submit: %v", err)
+	}
+
+	result := <-pool.Results()
+	if result.Err != nil {
+		t.Errorf("expected no error, got: %v", result.Err)
+	}
+	if result.Execution.Status != "COMPLETED" {
+		t.Errorf("grandchild status: want COMPLETED, got %q", result.Execution.Status)
+	}
+
+	gotMiddle, err := store.GetTask(middle.ID)
+	if err != nil {
+		t.Fatalf("get middle: %v", err)
+	}
+	if gotMiddle.State != task.StateReady {
+		t.Errorf("middle state: want READY (builder-role roll-up, awaiting arbitrated review), got %v", gotMiddle.State)
+	}
+
+	gotRoot, err := store.GetTask(root.ID)
+	if err != nil {
+		t.Fatalf("get root: %v", err)
+	}
+	if gotRoot.State != task.StateBlocked {
+		t.Errorf("root state: want unchanged BLOCKED (middle hasn't been arbitrated yet, cascade must not continue), got %v", gotRoot.State)
+	}
+}
+```
+
+- [ ] **Step 6: Add a `RecoverStaleBlocked` test for the same builder-role roll-up case**
+
+In the same file, find `TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent` (its full body, ending at its closing `}`) and immediately after it, add:
+
+```go
+// TestPool_RecoverStaleBlocked_BuilderRoleRollup_StaysReady proves
+// RecoverStaleBlocked (the startup-recovery sweep, which calls
+// maybeUnblockParent for every BLOCKED/QUEUED task) applies the same
+// builder-role READY-not-COMPLETED rule as the live dispatch path -- a
+// server restart must not silently promote a builder-role roll-up straight
+// to COMPLETED just because all its children happen to be done by the time
+// recovery runs.
+func TestPool_RecoverStaleBlocked_BuilderRoleRollup_StaysReady(t *testing.T) {
+	store := testStore(t)
+	logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError}))
+	pool := NewPool(2, map[string]Runner{"claude": &mockRunner{}}, store, logger)
+
+	root := makeTask("recover-builder-root")
+	root.State = task.StateBlocked
+	store.CreateTask(root)
+
+	middle := makeTask("recover-builder-middle")
+	middle.ParentTaskID = root.ID
+	middle.Agent.Role = "builder"
+	middle.State = task.StateBlocked
+	store.CreateTask(middle)
+
+	grandchild := makeTask("recover-builder-grandchild")
+	grandchild.ParentTaskID = middle.ID
+	grandchild.State = task.StateCompleted
+	store.CreateTask(grandchild)
+
+	pool.RecoverStaleBlocked()
+
+	gotMiddle, err := store.GetTask(middle.ID)
+	if err != nil {
+		t.Fatalf("get middle: %v", err)
+	}
+	if gotMiddle.State != task.StateReady {
+		t.Errorf("middle state: want READY (builder-role, awaiting arbitrated review), got %v", gotMiddle.State)
+	}
+
+	gotRoot, err := store.GetTask(root.ID)
+	if err != nil {
+		t.Fatalf("get root: %v", err)
+	}
+	if gotRoot.State != task.StateBlocked {
+		t.Errorf("root state: want unchanged BLOCKED, got %v", gotRoot.State)
+	}
+}
+```
+
+- [ ] **Step 7: Run the full executor package test suite**
+
+Run: `go test ./internal/executor/... -v -run 'TestPool_(Submit|MaybeUnblockParent|RecoverStaleBlocked)'`
+Expected: PASS for every test, including the 3 new ones above and every pre-existing test in this group — in particular `TestPool_Submit_Subtask_GoesToCompleted`, `TestPool_Submit_GrandchildCompletion_CascadesThroughNestedParents`, `TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent`, and `TestPool_MaybeUnblockParent_ResolvesSubtaskThroughCurrentAttempt` must all still pass completely unchanged (none of their fixtures set `Agent.Role`, so `promoteNestedTask`'s builder-role branch never fires for them — confirm this by reading `makeTask`'s definition and confirming it leaves `Agent.Role` at its zero value). Paste the actual output.
+
+- [ ] **Step 8: Run the full repo test suite**
+
+Run: `go build ./...` then `go test ./...` (the entire repo). Both must pass — paste the actual output. If `internal/api` flakes under the race detector on a one-off "sql: database is closed" teardown error unrelated to this task's actual changes, rerun once before treating it as real.
+
+Also run `gofmt -l internal/executor/executor.go internal/executor/executor_test.go` and `gofmt -w` any file it flags that wasn't already gofmt-dirty before your changes (check with `git show HEAD~1:<path> | gofmt -l -` if unsure).
+
+- [ ] **Step 9: Commit locally NOW (before further verification, if running under time pressure)**
+
+Run these exact commands, in order:
+
+```bash
+git add -A
+git commit -m "refactor(executor): nested builder-role tasks go READY not COMPLETED, requiring external arbitrated review"
+git status
+git log --oneline -1
+```
+
+**Do not run `git push`.** This repo's own bare git remote is not mounted inside dispatch containers. Per this repo's own CLAUDE.md ("After a successful run, new commits are pushed from the workspace"), claudomator's ContainerRunner pushes your commits externally once your run finishes successfully with a clean working tree — your only job is a clean local commit and a clean working tree at teardown.
+
+## Mandatory verification disclosure
+
+When you call report_summary, paste the actual terminal output of every command above — literal pass/fail counts, not a claim of success — including the full-repo `go test ./...` run and the final `git status`/`git log` confirmation showing a clean tree with your commit present. If anything here is ambiguous or conflicts with what you find in the actual repo, call ask_user and describe the specific conflict — don't guess or silently decide.