refactor(scheduler): generalize finalizeArbitration to any builder-role node, add nested fix-attempt spawning and acceptance-criteria fallback

1 files changed, 275 insertions, 2 deletions

diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go
index a13659d..03d4437 100644
--- a/internal/scheduler/story_orchestrator_test.go
+++ b/internal/scheduler/story_orchestrator_test.go
@@ -444,6 +444,279 @@ func TestStoryOrchestrator_EmitsEvalVerdict_OncePerEvaluator(t *testing.T) {
 	}
 }
 
+// TestStoryOrchestrator_FinalizeArbitration_NestedNodeApproved_DoesNotTouchStoryStatus
+// proves finalizeArbitration, generalized to operate on any builder-role
+// node (not just the story root), promotes a NESTED node to COMPLETED on
+// approval without touching story.Status at all -- REVIEW_READY is reserved
+// for the actual root position (node.ParentTaskID == "").
+func TestStoryOrchestrator_FinalizeArbitration_NestedNodeApproved_DoesNotTouchStoryStatus(t *testing.T) {
+	store := newFakeStoryStore()
+	root := builderTask("nested-approve-root", task.StateBlocked)
+	store.tasks[root.ID] = root
+	st := newStoryWithRoot("nested-approve-story", root.ID, "IN_PROGRESS")
+	store.stories[st.ID] = st
+
+	nested := builderTask("nested-approve-node", task.StateReady)
+	nested.ParentTaskID = root.ID
+	store.tasks[nested.ID] = nested
+
+	arb := &task.Task{
+		ID:      "nested-approve-arb",
+		Name:    "Arbitration",
+		Agent:   task.AgentConfig{Role: arbitrationRole},
+		State:   task.StateCompleted,
+		Summary: "ship it",
+	}
+	store.tasks[arb.ID] = arb
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: true, Reasoning: "meets criteria"})
+	if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload}); err != nil {
+		t.Fatalf("seed verdict event: %v", err)
+	}
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.finalizeArbitration(context.Background(), st, nested, arb)
+
+	got, err := store.GetTask(nested.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.State != task.StateCompleted {
+		t.Errorf("nested node State = %v, want COMPLETED", got.State)
+	}
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var gotStory *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			gotStory = s
+		}
+	}
+	if gotStory.Status != "IN_PROGRESS" {
+		t.Errorf("story Status = %q, want unchanged IN_PROGRESS (only root's own arbitration should touch story.Status)", gotStory.Status)
+	}
+}
+
+// TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_SpawnsFixAttemptDirectly
+// proves the nested-rejection path: since a nested position has no
+// story-level field to poll (unlike the root's NEEDS_FIX/ensureFixAttempt
+// flow), its fix-attempt is spawned immediately, right here -- same role,
+// same ParentTaskID as the rejected node (so task.CurrentAttempt resolves
+// it correctly), and story.Status is left untouched.
+func TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_SpawnsFixAttemptDirectly(t *testing.T) {
+	store := newFakeStoryStore()
+	root := builderTask("nested-reject-root", task.StateBlocked)
+	store.tasks[root.ID] = root
+	st := newStoryWithRoot("nested-reject-story", root.ID, "IN_PROGRESS")
+	store.stories[st.ID] = st
+
+	nested := builderTask("nested-reject-node", task.StateReady)
+	nested.ParentTaskID = root.ID
+	store.tasks[nested.ID] = nested
+
+	arb := &task.Task{
+		ID:      "nested-reject-arb",
+		Name:    "Arbitration",
+		Agent:   task.AgentConfig{Role: arbitrationRole},
+		State:   task.StateCompleted,
+		Summary: "found a real problem",
+	}
+	store.tasks[arb.ID] = arb
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: false, Reasoning: "breaks the widget"})
+	if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload}); err != nil {
+		t.Fatalf("seed verdict event: %v", err)
+	}
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.finalizeArbitration(context.Background(), st, nested, arb)
+
+	got, err := store.GetTask(nested.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got.State != task.StateReady {
+		t.Errorf("rejected nested node State = %v, want unchanged READY", got.State)
+	}
+
+	fixAttempts := store.dependentsWithRole(nested.ID, "builder")
+	if len(fixAttempts) != 1 {
+		t.Fatalf("expected exactly 1 fix-attempt task depending on the rejected nested node, got %d", len(fixAttempts))
+	}
+	fix := fixAttempts[0]
+	if fix.ParentTaskID != nested.ParentTaskID {
+		t.Errorf("fix attempt ParentTaskID = %q, want %q (same position as the rejected node)", fix.ParentTaskID, nested.ParentTaskID)
+	}
+	if fix.State != task.StateQueued {
+		t.Errorf("fix attempt State = %v, want QUEUED", fix.State)
+	}
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var gotStory *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			gotStory = s
+		}
+	}
+	if gotStory.Status != "IN_PROGRESS" {
+		t.Errorf("story Status = %q, want unchanged IN_PROGRESS", gotStory.Status)
+	}
+}
+
+// TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_CapsAtMaxFixAttempts
+// mirrors the root's own safety net (TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts)
+// for a nested position: once a chain of maxFixAttempts consecutive fix
+// attempts already exists at this position, no further fix attempt is
+// spawned.
+func TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_CapsAtMaxFixAttempts(t *testing.T) {
+	store := newFakeStoryStore()
+	root := builderTask("nested-cap-root", task.StateBlocked)
+	store.tasks[root.ID] = root
+	st := newStoryWithRoot("nested-cap-story", root.ID, "IN_PROGRESS")
+	store.stories[st.ID] = st
+
+	nested := builderTask("nested-cap-node", task.StateReady)
+	nested.ParentTaskID = root.ID
+	store.tasks[nested.ID] = nested
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+
+	// Build a chain of maxFixAttempts prior fix-attempt tasks at this same
+	// nested position, ending at prev -- simulating a position that has
+	// already exhausted its automatic fix attempts.
+	prev := nested
+	for i := 0; i < maxFixAttempts; i++ {
+		nt, err := orch.spawnRoleTask(context.Background(), fmt.Sprintf("Fix attempt %d", i), "builder", []string{prev.ID}, root.ID, root, "fix it")
+		if err != nil {
+			t.Fatalf("seed fix-attempt chain: %v", err)
+		}
+		prev = nt
+	}
+
+	arb := &task.Task{
+		ID:      "nested-cap-arb",
+		Name:    "Arbitration",
+		Agent:   task.AgentConfig{Role: arbitrationRole},
+		State:   task.StateCompleted,
+		Summary: "still broken",
+	}
+	store.tasks[arb.ID] = arb
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: false, Reasoning: "still broken"})
+	if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload}); err != nil {
+		t.Fatalf("seed verdict event: %v", err)
+	}
+
+	orch.finalizeArbitration(context.Background(), st, prev, arb)
+
+	fixAttempts := store.dependentsWithRole(prev.ID, "builder")
+	if len(fixAttempts) != 0 {
+		t.Fatalf("expected no new fix-attempt task once maxFixAttempts is reached, got %d", len(fixAttempts))
+	}
+}
+
+// TestStoryOrchestrator_FinalizeArbitration_RootStillSetsStoryStatus proves
+// the generalized finalizeArbitration still drives story.Status exactly as
+// before when node IS the root position (node.ParentTaskID == "") -- the
+// one case where touching story.Status is still correct, now decided by
+// node.ParentTaskID rather than being the function's only mode.
+func TestStoryOrchestrator_FinalizeArbitration_RootStillSetsStoryStatus(t *testing.T) {
+	store := newFakeStoryStore()
+	root := builderTask("root-status-root", task.StateReady)
+	store.tasks[root.ID] = root
+	st := newStoryWithRoot("root-status-story", root.ID, "VALIDATING")
+	store.stories[st.ID] = st
+
+	arb := &task.Task{
+		ID:      "root-status-arb",
+		Name:    "Arbitration",
+		Agent:   task.AgentConfig{Role: arbitrationRole},
+		State:   task.StateCompleted,
+		Summary: "ship it",
+	}
+	store.tasks[arb.ID] = arb
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: true, Reasoning: "meets criteria"})
+	if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload}); err != nil {
+		t.Fatalf("seed verdict event: %v", err)
+	}
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.finalizeArbitration(context.Background(), st, root, arb)
+
+	gotRoot, err := store.GetTask(root.ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if gotRoot.State != task.StateCompleted {
+		t.Errorf("root State = %v, want COMPLETED", gotRoot.State)
+	}
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var gotStory *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			gotStory = s
+		}
+	}
+	if gotStory.Status != "REVIEW_READY" {
+		t.Errorf("story Status = %q, want REVIEW_READY (root's own approval must still drive it)", gotStory.Status)
+	}
+}
+
+// TestStoryOrchestrator_FinalizeArbitration_IdempotentViaArbitrationDecidedEvent
+// proves the generalized idempotency check (a KindArbitrationDecided event
+// already referencing this arbitration.ID) correctly no-ops a repeated call
+// for the same already-decided arbitration -- the mechanism every node in a
+// story's tree now shares, replacing the root-only story.Status ==
+// "VALIDATING" gate.
+func TestStoryOrchestrator_FinalizeArbitration_IdempotentViaArbitrationDecidedEvent(t *testing.T) {
+	store := newFakeStoryStore()
+	root := builderTask("idempotent-root", task.StateReady)
+	store.tasks[root.ID] = root
+	st := newStoryWithRoot("idempotent-story", root.ID, "VALIDATING")
+	store.stories[st.ID] = st
+
+	arb := &task.Task{
+		ID:      "idempotent-arb",
+		Name:    "Arbitration",
+		Agent:   task.AgentConfig{Role: arbitrationRole},
+		State:   task.StateCompleted,
+		Summary: "ship it",
+	}
+	store.tasks[arb.ID] = arb
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: true, Reasoning: "meets criteria"})
+	if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload}); err != nil {
+		t.Fatalf("seed verdict event: %v", err)
+	}
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.finalizeArbitration(context.Background(), st, root, arb)
+	orch.finalizeArbitration(context.Background(), st, root, arb)
+	orch.finalizeArbitration(context.Background(), st, root, arb)
+
+	decided := store.eventsOfKind(event.KindArbitrationDecided)
+	if len(decided) != 1 {
+		t.Fatalf("expected exactly 1 arbitration_decided event after 3 calls, got %d", len(decided))
+	}
+}
+
 // TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady is
 // verification item (d): arbitration reaching COMPLETED emits
 // KindArbitrationDecided and moves the story to REVIEW_READY.
@@ -788,7 +1061,7 @@ func TestStoryOrchestrator_EnsureFixAttempt_FindsExistingSpawnedAttempt(t *testi
 	pool := &fakePool{}
 	orch := &StoryOrchestrator{Store: store, Pool: pool}
 
-	preexisting, err := orch.spawnRoleTask(context.Background(), "Fix attempt: pre-existing", "builder", []string{oldRoot.ID}, oldRoot, "fix it")
+	preexisting, err := orch.spawnRoleTask(context.Background(), "Fix attempt: pre-existing", "builder", []string{oldRoot.ID}, "", oldRoot, "fix it")
 	if err != nil {
 		t.Fatalf("seed pre-existing fix attempt: %v", err)
 	}
@@ -827,7 +1100,7 @@ func TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts(t *testing.T) {
 	// story that has already exhausted its automatic fix attempts.
 	prev := oldRoot
 	for i := 0; i < maxFixAttempts; i++ {
-		nt, err := orch.spawnRoleTask(context.Background(), fmt.Sprintf("Fix attempt %d", i), "builder", []string{prev.ID}, oldRoot, "fix it")
+		nt, err := orch.spawnRoleTask(context.Background(), fmt.Sprintf("Fix attempt %d", i), "builder", []string{prev.ID}, "", oldRoot, "fix it")
 		if err != nil {
 			t.Fatalf("seed fix-attempt chain: %v", err)
 		}