path: root/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md

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