refactor(executor): nested builder-role tasks go READY not COMPLETED, requiring external arbitrated review

2 files changed, 176 insertions, 9 deletions

diff --git a/internal/executor/executor.go b/internal/executor/executor.go
index 03b368e..659bcda 100644
--- a/internal/executor/executor.go
+++ b/internal/executor/executor.go
@@ -533,12 +533,10 @@ func (p *Pool) handleRunResult(ctx context.Context, t *task.Task, exec *storage.
 			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"
-			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)
 		}
 	}
 
@@ -1227,6 +1225,40 @@ func withFailureHistory(t *task.Task, execs []*storage.Execution, err error) *ta
 // parentID is itself a subtask (ParentTaskID set) -- mirroring
 // handleRunResult's own subtask-completion branch, since a subtask never
 // needs a human accept regardless of whether it got there by finishing its
+// 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
+}
+
 // own direct work or by having all of its delegated children finish. That
 // COMPLETED transition recurses into maybeUnblockParent(parent.ParentTaskID)
 // so a chain of nested decomposition (subtask spawns subtask spawns
@@ -1269,11 +1301,7 @@ func (p *Pool) maybeUnblockParent(parentID string) {
 		}
 	}
 	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)
+		p.promoteNestedTask(parent)
 		return
 	}
 	if err := p.store.UpdateTaskState(parentID, task.StateReady); err != nil {
diff --git a/internal/executor/executor_test.go b/internal/executor/executor_test.go
index d505a3a..a2228c3 100644
--- a/internal/executor/executor_test.go
+++ b/internal/executor/executor_test.go
@@ -218,6 +218,42 @@ func TestPool_Submit_Subtask_GoesToCompleted(t *testing.T) {
 	}
 }
 
+// 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)
+	}
+}
+
 type fakeGate struct{ deny map[string]bool }
 
 func (g fakeGate) Allow(provider string, _ float64) (bool, error) { return !g.deny[provider], nil }
@@ -1126,6 +1162,63 @@ func TestPool_Submit_GrandchildCompletion_CascadesThroughNestedParents(t *testin
 	}
 }
 
+// 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)
+	}
+}
+
 // TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent proves
 // RecoverStaleBlocked no longer skips BLOCKED subtask-parents -- before
 // this fix, its "only promote actual parents" filter (ParentTaskID == "")
@@ -1169,6 +1262,52 @@ func TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent(t *testing.T) {
 	}
 }
 
+// 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)
+	}
+}
+
 // TestPool_MaybeUnblockParent_ResolvesSubtaskThroughCurrentAttempt proves
 // the parent's "are all subtasks done" gate resolves each subtask through
 // task.CurrentAttempt first -- so a subtask that was superseded by a