refactor(scheduler): promote story root to COMPLETED only after arbitration approves it, not eagerly on READY

2 files changed, 125 insertions, 52 deletions

diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go
index 8061b35..19383c6 100644
--- a/internal/scheduler/story_orchestrator.go
+++ b/internal/scheduler/story_orchestrator.go
@@ -226,9 +226,16 @@ 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 = o.autoAccept(st, root)
-	if root.State != task.StateCompleted {
-		return // Builder hasn't reached COMPLETED yet (still running, or not yet READY to auto-accept)
+	// 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
 	}
 
 	// Stage 1: Builder -> Evaluators (+ story -> VALIDATING).
@@ -259,9 +266,13 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
 	}
 	arbitration = o.autoAccept(st, arbitration)
 
-	// Stage 3: Arbitration -> REVIEW_READY.
+	// 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).
 	if arbitration.State == task.StateCompleted {
-		o.finalizeArbitration(st, arbitration)
+		o.finalizeArbitration(st, root, arbitration)
 	}
 }
 
@@ -388,20 +399,32 @@ func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Sto
 }
 
 // finalizeArbitration handles the Arbitration task reaching COMPLETED: it
-// emits KindArbitrationDecided and moves the story to REVIEW_READY or
-// NEEDS_FIX, depending on whether the arbitration task recorded a structured
-// KindVerdictReported event (AgentChannel.ReportVerdict). An arbitration
-// task that never calls report_verdict defaults to REVIEW_READY, preserving
-// the prior behavior for agents that don't report a structured verdict.
+// emits KindArbitrationDecided, then either approves or rejects root based on
+// whether the arbitration task recorded a structured KindVerdictReported
+// event (AgentChannel.ReportVerdict). An arbitration task that never calls
+// report_verdict defaults to approval, preserving the prior behavior for
+// agents that don't report a structured verdict.
+//
+// On approval, root is promoted READY -> COMPLETED *here* — not eagerly the
+// moment it first reached READY — so COMPLETED means "verified by
+// arbitration", uniformly, the same rule this design applies at every depth
+// of a builder-role task tree (see
+// docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md).
+// On rejection, root is left untouched at READY: it has been superseded, and
+// task.CurrentAttempt will resolve future lookups of this position forward
+// to whatever fix-attempt task ensureFixAttempt spawns once the story lands
+// at NEEDS_FIX below — READY already satisfies a dependent's DependsOn (see
+// executor.depDoneStates), so the fix attempt is immediately dispatchable
+// without root ever becoming COMPLETED.
 //
 // Gated on st.Status == "VALIDATING" so repeated ticks (or a story a human
-// already advanced past REVIEW_READY) don't re-emit the event or re-write the
-// status — this is the one place in the orchestrator where the story's own
-// status field, not a structural dependents check, is the idempotency guard,
-// because by this stage there's nothing further to check structurally: the
-// Arbitration task is the last task in the chain, so "does a subsequent task
-// exist" isn't an available signal.
-func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *task.Task) {
+// already advanced past REVIEW_READY) don't re-emit the event, re-promote
+// root, or re-write the status — this is the one place in the orchestrator
+// where the story's own status field, not a structural dependents check, is
+// the idempotency guard, because by this stage there's nothing further to
+// check structurally: the Arbitration task is the last task in the chain, so
+// "does a subsequent task exist" isn't an available signal.
+func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, root, arbitration *task.Task) {
 	if st.Status != "VALIDATING" {
 		return
 	}
@@ -422,17 +445,16 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
 		o.logf("story orchestrator: emit arbitration_decided", "storyID", st.ID, "error", err)
 	}
 
-	// Documented simplification (Phase 7b) closed: a structured
-	// KindVerdictReported event (AgentChannel.ReportVerdict) on the
-	// arbitration task, if present, now decides REVIEW_READY vs NEEDS_FIX.
-	// An arbitration task that never calls report_verdict (hasVerdict ==
-	// false) preserves the prior unconditional REVIEW_READY behavior — a
-	// human or chatbot can still manually set NEEDS_FIX via the existing
-	// PUT /api/stories/{id} for a story arbitrated by an agent that doesn't
-	// report a structured verdict.
 	if hasVerdict && !approved {
 		st.Status = "NEEDS_FIX"
 	} else {
+		// Approved (or no structured verdict reported -- preserves the
+		// prior unconditional-approve default). root only becomes COMPLETED
+		// here, after arbitration.
+		if err := o.Store.UpdateTaskState(root.ID, task.StateCompleted); err != nil {
+			o.logf("story orchestrator: finalize arbitration: promote root to completed", "storyID", st.ID, "taskID", root.ID, "error", err)
+			return
+		}
 		st.Status = "REVIEW_READY"
 	}
 	if err := o.Store.UpdateStory(st); err != nil {
diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go
index 200e38e..a13659d 100644
--- a/internal/scheduler/story_orchestrator_test.go
+++ b/internal/scheduler/story_orchestrator_test.go
@@ -213,13 +213,15 @@ func newStoryWithRoot(id, rootTaskID, status string) *story.Story {
 	return &story.Story{ID: id, Name: "Test Story", Status: status, RootTaskID: rootTaskID}
 }
 
-// TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes is verification
-// item (a): a builder task reaching COMPLETED for a story spawns exactly 4
+// TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderReady is verification
+// item (a): a builder task reaching READY for a story spawns exactly 4
 // evaluator tasks with correct roles/depends_on, moves the story to
-// VALIDATING.
-func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes(t *testing.T) {
+// VALIDATING. The builder itself stays READY (not COMPLETED) — see
+// TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved
+// for that specific assertion.
+func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderReady(t *testing.T) {
 	store := newFakeStoryStore()
-	root := builderTask("builder-1", task.StateCompleted)
+	root := builderTask("builder-1", task.StateReady)
 	store.tasks[root.ID] = root
 	st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS")
 	store.stories[st.ID] = st
@@ -282,7 +284,7 @@ func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes(t *testing.T) {
 // duplicates.
 func TestStoryOrchestrator_DoesNotDuplicateEvaluators(t *testing.T) {
 	store := newFakeStoryStore()
-	root := builderTask("builder-1", task.StateCompleted)
+	root := builderTask("builder-1", task.StateReady)
 	store.tasks[root.ID] = root
 	st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS")
 	store.stories[st.ID] = st
@@ -321,7 +323,13 @@ func evaluatorTask(id, rootID, role string, state task.State) *task.Task {
 func seedStoryWithEvaluators(t *testing.T, evalState task.State) (*fakeStoryStore, *story.Story, []*task.Task) {
 	t.Helper()
 	store := newFakeStoryStore()
-	root := builderTask("builder-1", task.StateCompleted)
+	// root is READY, not COMPLETED: under the new rule, root only reaches
+	// COMPLETED once its own arbitration approves it (see
+	// finalizeArbitration), and every caller of this helper is simulating a
+	// point in the pipeline before that approval has happened yet.
+	// seedDoneStory (below) explicitly promotes root to COMPLETED itself for
+	// its own already-approved narrative.
+	root := builderTask("builder-1", task.StateReady)
 	store.tasks[root.ID] = root
 	st := newStoryWithRoot("story-1", root.ID, "VALIDATING")
 	store.stories[st.ID] = st
@@ -654,6 +662,25 @@ func seedNeedsFixStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task,
 	return store, got, oldRoot, evaluators, arb
 }
 
+// TestStoryOrchestrator_ArbitrationRejects_LeavesRootReadyNotCompleted proves
+// the core "verified before complete" invariant this plan adds: a rejected
+// root is left at READY — never promoted to COMPLETED. Only
+// finalizeArbitration's approval branch ever writes root to COMPLETED; a
+// rejection must never look "done" to anything that trusts task.State
+// directly, the same gate a nested subtask's parent will rely on once
+// arbitrated review recurses into subtask trees (a future plan).
+func TestStoryOrchestrator_ArbitrationRejects_LeavesRootReadyNotCompleted(t *testing.T) {
+	store, _, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	got, err := store.GetTask(oldRoot.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.State != task.StateReady {
+		t.Errorf("rejected root State = %v, want READY (must never be promoted to COMPLETED on rejection)", got.State)
+	}
+}
+
 // TestStoryOrchestrator_NeedsFix_SpawnsFixAttemptAndRepointsRoot proves the
 // core mechanism: a story at NEEDS_FIX gets a new builder-role fix-attempt
 // task spawned (depending on the rejected root), and the story is re-pointed
@@ -857,14 +884,18 @@ func TestStoryOrchestrator_DoesNothing_WhenBuilderNotComplete(t *testing.T) {
 	}
 }
 
-// TestStoryOrchestrator_AutoAcceptsReadyBuilder is the core regression test
-// for the auto-accept fix: a builder task sitting at READY (execution
-// succeeded, awaiting what would otherwise be a manual
-// POST /api/tasks/{id}/accept) is transitioned to COMPLETED by the
-// orchestrator itself — with no external accept call — and, because that
-// unblocks Stage 1 in the same tick, the 4 evaluators are spawned
-// immediately too.
-func TestStoryOrchestrator_AutoAcceptsReadyBuilder(t *testing.T) {
+// TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved
+// replaces the old "auto-accept the builder immediately" regression test: a
+// builder task sitting at READY (execution succeeded, awaiting what would
+// otherwise be a manual POST /api/tasks/{id}/accept) triggers the 4
+// evaluators to spawn in the same tick — but, unlike evaluators/arbitration
+// tasks, the builder itself is NOT auto-accepted to COMPLETED here. It stays
+// READY (which already satisfies a dependent's DependsOn — see
+// executor.depDoneStates) until its own arbitration approves it (see
+// finalizeArbitration). This is what makes "COMPLETED" mean "verified",
+// uniformly — the same rule a nested builder subtask will need once
+// arbitrated review recurses into subtask trees.
+func TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved(t *testing.T) {
 	store := newFakeStoryStore()
 	root := builderTask("builder-1", task.StateReady)
 	store.tasks[root.ID] = root
@@ -879,13 +910,13 @@ func TestStoryOrchestrator_AutoAcceptsReadyBuilder(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if got.State != task.StateCompleted {
-		t.Fatalf("builder should be auto-accepted to COMPLETED, got %v", got.State)
+	if got.State != task.StateReady {
+		t.Fatalf("builder must stay READY until arbitration approves it, got %v", got.State)
 	}
 
 	deps, _ := store.ListDependents(root.ID)
 	if len(deps) != 4 {
-		t.Fatalf("expected 4 evaluator tasks spawned in the same tick the builder auto-accepts, got %d", len(deps))
+		t.Fatalf("expected 4 evaluator tasks spawned in the same tick the builder reaches READY, got %d", len(deps))
 	}
 }
 
@@ -969,7 +1000,7 @@ func TestStoryOrchestrator_AutoAcceptsReadyArbitration(t *testing.T) {
 // an unrelated READY task that merely happens to exist in the store.
 func TestStoryOrchestrator_AutoAccept_DoesNotTouchUnrelatedReadyTask(t *testing.T) {
 	store := newFakeStoryStore()
-	root := builderTask("builder-1", task.StateCompleted)
+	root := builderTask("builder-1", task.StateReady)
 	store.tasks[root.ID] = root
 	st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS")
 	store.stories[st.ID] = st
@@ -1031,18 +1062,21 @@ func TestStoryOrchestrator_SkipsTerminalStories(t *testing.T) {
 }
 
 // TestStoryOrchestrator_EndToEnd drives a story through the full chain —
-// builder complete -> evaluators -> arbitration -> REVIEW_READY — using only
+// builder ready -> evaluators -> arbitration -> REVIEW_READY — using only
 // the fake store/pool, proving the whole ceremony holds together end to end
 // at the orchestrator level (verification item 5, the higher-level test).
 //
 // Every task in the chain is driven to READY (never directly to COMPLETED),
 // mirroring exactly what executor.Pool.handleRunResult does for a real
-// top-level task whose execution succeeds — proving the orchestrator's own
+// top-level task whose execution succeeds. Evaluators and arbitration are
+// still carried the rest of the way to COMPLETED by the orchestrator's own
 // auto-accept (not an external POST /api/tasks/{id}/accept call, which this
-// test never makes) is what carries each task the rest of the way to
-// COMPLETED and advances the story. The only accept call anywhere in this
-// flow is the story-level one, which isn't part of this test — it's covered
-// separately in internal/api's story accept-gate tests.
+// test never makes) — but the builder itself is the one exception: it stays
+// READY through the whole cycle and is only promoted to COMPLETED once
+// arbitration actually approves it, proving "COMPLETED means verified" holds
+// end to end, not just in isolated unit tests. The only accept call anywhere
+// in this flow is the story-level one, which isn't part of this test — it's
+// covered separately in internal/api's story accept-gate tests.
 func TestStoryOrchestrator_EndToEnd(t *testing.T) {
 	store := newFakeStoryStore()
 	root := builderTask("builder-1", task.StatePending)
@@ -1068,8 +1102,8 @@ func TestStoryOrchestrator_EndToEnd(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if rootAfter.State != task.StateCompleted {
-		t.Fatalf("builder should be auto-accepted to COMPLETED without an accept call, got %v", rootAfter.State)
+	if rootAfter.State != task.StateReady {
+		t.Fatalf("builder must stay READY until arbitration approves it (not auto-accepted eagerly), got %v", rootAfter.State)
 	}
 
 	deps, _ := store.ListDependents(root.ID)
@@ -1135,6 +1169,14 @@ func TestStoryOrchestrator_EndToEnd(t *testing.T) {
 		t.Fatalf("arbitration should be auto-accepted to COMPLETED, got %v", arbAfter.State)
 	}
 
+	rootFinal, err := store.GetTask(root.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if rootFinal.State != task.StateCompleted {
+		t.Fatalf("builder should finally be promoted to COMPLETED now that arbitration approved it, got %v", rootFinal.State)
+	}
+
 	if statusOf() != "REVIEW_READY" {
 		t.Fatalf("expected REVIEW_READY after arbitration completes, got %q", statusOf())
 	}
@@ -1168,6 +1210,15 @@ func seedDoneStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, []*
 	}
 	store.tasks[arb.ID] = arb
 
+	// By the time a story reaches DONE, its root has already been through a
+	// real (or simulated) approval and is COMPLETED. seedStoryWithEvaluators
+	// seeds root at READY (the pre-arbitration state every other caller of
+	// that helper needs), so promote it here to match this fixture's own
+	// DONE narrative.
+	if err := store.UpdateTaskState(st.RootTaskID, task.StateCompleted); err != nil {
+		t.Fatalf("promote root to completed for DONE fixture: %v", err)
+	}
+
 	root, err := store.GetTask(st.RootTaskID)
 	if err != nil {
 		t.Fatal(err)