refactor(scheduler): recurse processStory into a tree walk over every READY builder-role node

1 files changed, 73 insertions, 33 deletions

diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go
index d190ba7..a526552 100644
--- a/internal/scheduler/story_orchestrator.go
+++ b/internal/scheduler/story_orchestrator.go
@@ -212,9 +212,21 @@ func (o *StoryOrchestrator) logf(msg string, args ...any) {
 	}
 }
 
-// processStory advances a single story by at most one stage per tick (it
-// returns as soon as it finds a stage that isn't ready to progress yet — the
-// next tick picks up where this one left off).
+// processStory advances every currently-READY builder-role node in a
+// story's tree -- root or nested, any depth -- by up to one stage per tick
+// (each node's own progression through Evaluators -> Arbitration ->
+// approve/reject is handled by processBuilderNode, below). This no longer
+// stops at the first node that isn't ready yet: a story's tree can have
+// several builder nodes progressing independently and concurrently. In
+// particular, a still-BLOCKED root can have a nested subtask that has
+// already reached READY and needs its own arbitration before it can turn
+// COMPLETED -- and internal/executor.Pool.maybeUnblockParent requires every
+// subtask's task.CurrentAttempt to resolve to COMPLETED before promoting a
+// BLOCKED parent. Stopping at root the way the single-root version did would
+// deadlock any story with nested subtasks: root can never become READY
+// while the very node that would produce that COMPLETED sits unprocessed,
+// waiting for a tick that never reaches past root's own not-yet-READY
+// state.
 func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
 	if st.Status == "NEEDS_FIX" {
 		o.ensureFixAttempt(ctx, st)
@@ -226,20 +238,41 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
 		o.logf("story orchestrator: resolve current root attempt", "storyID", st.ID, "rootTaskID", st.RootTaskID, "error", err)
 		return
 	}
-	// root is NOT auto-accepted here. Unlike evaluator/arbitration tasks,
-	// root is builder-role: its own completion must mean "verified by
-	// arbitration", not merely "the agent finished" -- so it stays READY
-	// through the whole Evaluators/Arbitration cycle below, and only
-	// finalizeArbitration's approval branch ever promotes it to COMPLETED.
-	// READY already satisfies a dependent's DependsOn (see
-	// executor.depDoneStates), so evaluators/arbitration can depend on a
-	// still-READY root without issue.
-	if root.State != task.StateReady {
-		return // not yet finished (still running/blocked); or already COMPLETED, meaning a prior tick's finalizeArbitration already approved it -- nothing further to do
+	if root.State == task.StateCompleted {
+		// The whole tree is already fully resolved: root only ever reaches
+		// COMPLETED via finalizeArbitration's approval branch, which --
+		// per the maybeUnblockParent invariant above -- can't happen until
+		// every nested builder node beneath it is COMPLETED too. Nothing
+		// left to walk.
+		return
 	}
 
-	// Stage 1: Builder -> Evaluators (+ story -> VALIDATING).
-	evaluators, ok := o.ensureEvaluators(ctx, st, root)
+	for _, node := range o.taskTree(root.ID) {
+		if node.Agent.Role != "builder" || node.State != task.StateReady {
+			continue
+		}
+		o.processBuilderNode(ctx, st, node)
+	}
+}
+
+// processBuilderNode drives one READY builder-role node -- root or nested --
+// through Evaluators -> Arbitration -> approve/reject, advancing it by at
+// most one stage per tick (the next tick picks up where this one left off).
+// This is the exact per-root body processStory ran directly before this
+// design recursed into nested subtask trees; extracting it into its own
+// function changed nothing about what it does, only how many nodes call it
+// per tick.
+//
+// node is NOT auto-accepted here. Unlike evaluator/arbitration tasks, node
+// is builder-role: its own completion must mean "verified by arbitration",
+// not merely "the agent finished" -- so it stays READY through the whole
+// Evaluators/Arbitration cycle below, and only finalizeArbitration's
+// approval branch ever promotes it to COMPLETED. READY already satisfies a
+// dependent's DependsOn (see executor.depDoneStates), so evaluators/
+// arbitration can depend on a still-READY node without issue.
+func (o *StoryOrchestrator) processBuilderNode(ctx context.Context, st *story.Story, node *task.Task) {
+	// Stage 1: Builder -> Evaluators.
+	evaluators, ok := o.ensureEvaluators(ctx, st, node)
 	if !ok {
 		return // not all 4 could be found/created yet; retry next tick
 	}
@@ -260,19 +293,20 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
 	}
 
 	// Stage 2: Evaluators -> Arbitration.
-	arbitration, ok := o.ensureArbitration(ctx, st, root, evaluators)
+	arbitration, ok := o.ensureArbitration(ctx, st, node, evaluators)
 	if !ok {
 		return
 	}
 	arbitration = o.autoAccept(st, arbitration)
 
-	// Stage 3: Arbitration -> approve (root: READY -> COMPLETED, story ->
-	// REVIEW_READY) or reject (story -> NEEDS_FIX; a fix attempt is spawned
-	// next tick by ensureFixAttempt, depending on the still-READY root --
-	// task.CurrentAttempt resolves future lookups of this position forward
-	// to it).
+	// Stage 3: Arbitration -> approve (node: READY -> COMPLETED, and if node
+	// is the story's root, story -> REVIEW_READY) or reject (if node is the
+	// root, story -> NEEDS_FIX and a fix attempt is spawned next tick by
+	// ensureFixAttempt; otherwise finalizeArbitration spawns the nested
+	// fix-attempt directly). task.CurrentAttempt resolves future lookups of
+	// this position forward to whatever fix-attempt gets spawned.
 	if arbitration.State == task.StateCompleted {
-		o.finalizeArbitration(ctx, st, root, arbitration)
+		o.finalizeArbitration(ctx, st, node, arbitration)
 	}
 }
 
@@ -309,18 +343,24 @@ func (o *StoryOrchestrator) autoAccept(st *story.Story, t *task.Task) *task.Task
 	return &accepted
 }
 
-// ensureEvaluators returns the 4 Evaluator tasks fanned out from root,
-// spawning any missing ones. Idempotency is structural, not a marker on the
-// story: it looks at root's actual dependents and checks which of
+// ensureEvaluators returns the 4 Evaluator tasks fanned out from node (root
+// or nested -- any builder-role task processStory's tree walk has found
+// READY), spawning any missing ones. Idempotency is structural, not a marker
+// on the story: it looks at node's actual dependents and checks which of
 // evaluatorRoles are already represented, so calling this repeatedly for the
-// same story never spawns duplicates (test (b) in the phase description) —
+// same node never spawns duplicates (test (b) in the phase description) —
 // even across a process restart, unlike a purely in-memory guard would be.
+// story.Status is only ever set to VALIDATING here when node IS the story's
+// root (node.ParentTaskID == "") -- mirroring finalizeArbitration's own
+// isRoot gate on story.Status writes; a nested node's evaluators spawning
+// must not touch story-level bookkeeping that only the root position has
+// any claim to.
 // Returns ok=false if any missing evaluator couldn't be created this tick
 // (transient store error); the caller retries on the next tick.
-func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Story, root *task.Task) ([]*task.Task, bool) {
-	dependents, err := o.Store.ListDependents(root.ID)
+func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Story, node *task.Task) ([]*task.Task, bool) {
+	dependents, err := o.Store.ListDependents(node.ID)
 	if err != nil {
-		o.logf("story orchestrator: list root dependents", "storyID", st.ID, "error", err)
+		o.logf("story orchestrator: list node dependents", "storyID", st.ID, "error", err)
 		return nil, false
 	}
 	found := make(map[string]*task.Task, len(evaluatorRoles))
@@ -332,7 +372,7 @@ func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Stor
 		}
 	}
 
-	criteria := root.AcceptanceCriteria
+	criteria := node.AcceptanceCriteria
 	if len(criteria) == 0 {
 		criteria = st.AcceptanceCriteria
 	}
@@ -342,8 +382,8 @@ func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Stor
 		if _, ok := found[r]; ok {
 			continue
 		}
-		nt, err := o.spawnRoleTask(ctx, fmt.Sprintf("%s: %s", r, st.Name), r, []string{root.ID}, "", root,
-			fmt.Sprintf("Evaluate the changes made by task %s against the %q dimension for story %q.\n\nStory spec:\n%s\n\nAcceptance criteria:\n%s", root.ID, r, st.Name, st.Spec, formatAcceptanceCriteria(criteria)))
+		nt, err := o.spawnRoleTask(ctx, fmt.Sprintf("%s: %s", r, st.Name), r, []string{node.ID}, "", node,
+			fmt.Sprintf("Evaluate the changes made by task %s against the %q dimension for story %q.\n\nStory spec:\n%s\n\nAcceptance criteria:\n%s", node.ID, r, st.Name, st.Spec, formatAcceptanceCriteria(criteria)))
 		if err != nil {
 			o.logf("story orchestrator: spawn evaluator", "storyID", st.ID, "role", r, "error", err)
 			continue
@@ -352,7 +392,7 @@ func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Stor
 		spawnedAny = true
 	}
 
-	if spawnedAny {
+	if spawnedAny && node.ParentTaskID == "" {
 		st.Status = "VALIDATING"
 		if err := o.Store.UpdateStory(st); err != nil {
 			o.logf("story orchestrator: update story to VALIDATING", "storyID", st.ID, "error", err)