From c812eeac49f5ebbcf8b9855a3f56ac4940e074d8 Mon Sep 17 00:00:00 2001 From: Claudomator Agent Date: Fri, 10 Jul 2026 19:29:58 +0000 Subject: refactor(scheduler): recurse processStory into a tree walk over every READY builder-role node --- internal/scheduler/story_orchestrator.go | 106 +++++++++++++++++++++---------- 1 file changed, 73 insertions(+), 33 deletions(-) (limited to 'internal/scheduler/story_orchestrator.go') 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) -- cgit v1.2.3