From 2c8a5c17dd43bee329f62a9c662d04571b9cd2d7 Mon Sep 17 00:00:00 2001 From: Claudomator Agent Date: Fri, 10 Jul 2026 03:03:50 +0000 Subject: refactor(executor): nested builder-role tasks go READY not COMPLETED, requiring external arbitrated review --- internal/executor/executor.go | 46 +++++++++--- internal/executor/executor_test.go | 139 +++++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 9 deletions(-) (limited to 'internal/executor') 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 -- cgit v1.2.3