feat(scheduler): automatically spawn and re-point a fix attempt when a story is rejected

2 files changed, 360 insertions, 8 deletions

diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go
index b70b487..c3d5215 100644
--- a/internal/scheduler/story_orchestrator.go
+++ b/internal/scheduler/story_orchestrator.go
@@ -216,6 +216,11 @@ func (o *StoryOrchestrator) logf(msg string, args ...any) {
 // 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).
 func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
+	if st.Status == "NEEDS_FIX" {
+		o.ensureFixAttempt(ctx, st)
+		return
+	}
+
 	root, err := o.Store.GetTask(st.RootTaskID)
 	if err != nil {
 		o.logf("story orchestrator: get root task", "storyID", st.ID, "rootTaskID", st.RootTaskID, "error", err)
@@ -401,7 +406,7 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
 		return
 	}
 
-	approved, hasVerdict := o.arbitrationVerdict(st, arbitration)
+	approved, _, hasVerdict := o.arbitrationVerdict(st, arbitration)
 
 	payload, _ := json.Marshal(struct {
 		TaskID   string `json:"task_id"`
@@ -437,28 +442,32 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
 
 // arbitrationVerdict reads the arbitration task's own event stream for a
 // KindVerdictReported event (AgentChannel.ReportVerdict) and returns its
-// approved value. hasVerdict is false if no such event was ever recorded —
-// callers must treat that as "no structured verdict was reported", not as
-// an implicit rejection.
-func (o *StoryOrchestrator) arbitrationVerdict(st *story.Story, arbitration *task.Task) (approved bool, hasVerdict bool) {
+// approved value and reasoning text. hasVerdict is false if no such event was
+// ever recorded — callers must treat that as "no structured verdict was
+// reported", not as an implicit rejection. reasoning feeds ensureFixAttempt's
+// automatic fix-attempt instructions; finalizeArbitration itself only needs
+// approved/hasVerdict and discards reasoning.
+func (o *StoryOrchestrator) arbitrationVerdict(st *story.Story, arbitration *task.Task) (approved bool, reasoning string, hasVerdict bool) {
 	events, err := o.Store.ListEvents(arbitration.ID, 0)
 	if err != nil {
 		o.logf("story orchestrator: list arbitration task events", "storyID", st.ID, "taskID", arbitration.ID, "error", err)
-		return false, false
+		return false, "", false
 	}
 	for _, e := range events {
 		if e.Kind != event.KindVerdictReported {
 			continue
 		}
 		var payload struct {
-			Approved bool `json:"approved"`
+			Approved  bool   `json:"approved"`
+			Reasoning string `json:"reasoning"`
 		}
 		if json.Unmarshal(e.Payload, &payload) == nil {
 			approved = payload.Approved
+			reasoning = payload.Reasoning
 			hasVerdict = true
 		}
 	}
-	return approved, hasVerdict
+	return approved, reasoning, hasVerdict
 }
 
 // optionalBool returns a pointer to v if present is true, nil otherwise —
@@ -472,6 +481,127 @@ func optionalBool(v bool, present bool) *bool {
 	return &v
 }
 
+// maxFixAttempts caps automatic fix-attempt spawning (see ensureFixAttempt)
+// to prevent an unbounded loop if arbitration keeps rejecting a story's
+// work. This is a simple, hardcoded safety net, not real cost/escalation
+// tiering — the design spec's Non-Goals explicitly deferred "no depth or
+// cost bound in this design... left as a distinct future piece of work".
+// Once hit, the story stays at NEEDS_FIX exactly like it does today, for a
+// human to intervene manually via PUT /api/stories/{id}.
+const maxFixAttempts = 3
+
+// ensureFixAttempt handles a story sitting at NEEDS_FIX: it spawns one new
+// top-level builder-role task depending on the rejected root (purely for
+// structural discoverability/audit trail — the rejected root is already
+// COMPLETED, so this dependency is immediately satisfied), whose
+// instructions carry the original story spec/acceptance criteria plus the
+// arbitration's rejection reasoning, then re-points st.RootTaskID at the new
+// task and resets st.Status to IN_PROGRESS. The very next tick re-enters
+// processStory's normal Builder->Evaluators->Arbitration flow against the
+// new root, completely unchanged — no new pipeline, the existing one
+// re-entered.
+//
+// Idempotency is structural, mirroring every other stage in this file: it
+// looks for an existing builder-role dependent of the rejected root before
+// spawning a new one, so calling this repeatedly (or after a restart between
+// "task spawned" and "story updated") never spawns duplicates. The
+// maxFixAttempts cap is only checked in the "spawn a new one" branch — a
+// dependent that was already committed to being spawned still gets
+// re-pointed to, regardless of the cap, since refusing to do so would leave
+// a task dangling with nothing tracking it.
+func (o *StoryOrchestrator) ensureFixAttempt(ctx context.Context, st *story.Story) {
+	oldRoot, err := o.Store.GetTask(st.RootTaskID)
+	if err != nil {
+		o.logf("story orchestrator: fix attempt: get rejected root task", "storyID", st.ID, "rootTaskID", st.RootTaskID, "error", err)
+		return
+	}
+
+	dependents, err := o.Store.ListDependents(oldRoot.ID)
+	if err != nil {
+		o.logf("story orchestrator: fix attempt: list root dependents", "storyID", st.ID, "error", err)
+		return
+	}
+	var fix *task.Task
+	for _, d := range dependents {
+		if d.Agent.Role == "builder" {
+			fix = d
+			break
+		}
+	}
+
+	if fix == nil {
+		if depth := o.fixAttemptDepth(oldRoot); depth >= maxFixAttempts {
+			o.logf("story orchestrator: fix attempt: max fix attempts reached, leaving story at NEEDS_FIX for a human", "storyID", st.ID, "depth", depth)
+			return
+		}
+		reasoning := o.rejectionReasoning(st, oldRoot)
+		instructions := fmt.Sprintf(
+			"A previous attempt at story %q (task %s) was rejected by arbitration. Fix the issues and resubmit.\n\n"+
+				"Story spec:\n%s\n\nAcceptance criteria:\n%s\n\nArbitration's rejection reasoning:\n%s",
+			st.Name, oldRoot.ID, st.Spec, formatAcceptanceCriteria(st.AcceptanceCriteria), reasoning)
+		nt, err := o.spawnRoleTask(ctx, "Fix attempt: "+st.Name, "builder", []string{oldRoot.ID}, oldRoot, instructions)
+		if err != nil {
+			o.logf("story orchestrator: fix attempt: spawn", "storyID", st.ID, "error", err)
+			return
+		}
+		fix = nt
+	}
+
+	st.RootTaskID = fix.ID
+	st.Status = "IN_PROGRESS"
+	if err := o.Store.UpdateStory(st); err != nil {
+		o.logf("story orchestrator: fix attempt: repoint story root", "storyID", st.ID, "error", err)
+	}
+}
+
+// rejectionReasoning finds the rejected root's evaluator/arbitration chain
+// (read-only, via findEvaluators/findArbitration — the same lookups
+// processRetro uses) and returns the arbitration task's structured
+// KindVerdictReported reasoning, falling back to its plain-text Summary if no
+// structured verdict was reported, or a placeholder if neither is available.
+func (o *StoryOrchestrator) rejectionReasoning(st *story.Story, oldRoot *task.Task) string {
+	evaluators, ok := o.findEvaluators(oldRoot.ID)
+	if !ok {
+		return "(no arbitration reasoning found)"
+	}
+	arbitration, ok := o.findArbitration(evaluators)
+	if !ok {
+		return "(no arbitration reasoning found)"
+	}
+	_, reasoning, hasVerdict := o.arbitrationVerdict(st, arbitration)
+	if hasVerdict && reasoning != "" {
+		return reasoning
+	}
+	if arbitration.Summary != "" {
+		return arbitration.Summary
+	}
+	return "(no arbitration reasoning found)"
+}
+
+// fixAttemptDepth walks backward through builder-role tasks connected by a
+// single DependsOn edge — the exact shape ensureFixAttempt creates — counting
+// how many fix attempts precede root. Stops as soon as it finds a task with
+// anything other than exactly one DependsOn entry (the original, non-fix-
+// attempt root, which was created by whatever process first submitted the
+// story). Bounded by maxFixAttempts+1 iterations since callers only care
+// whether the cap is hit, not the exact depth beyond that.
+func (o *StoryOrchestrator) fixAttemptDepth(root *task.Task) int {
+	depth := 0
+	current := root
+	for depth <= maxFixAttempts {
+		if len(current.DependsOn) != 1 {
+			break
+		}
+		prev, err := o.Store.GetTask(current.DependsOn[0])
+		if err != nil {
+			break
+		}
+		depth++
+		current = prev
+	}
+	return depth
+}
+
 // processRetro is the Phase 8 retro stage: it runs for every story the Tick
 // loop has already observed at status DONE (see Tick above). Unlike
 // processStory's stages, it never spawns or advances a Builder/Evaluator/
diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go
index 0d96840..0ace7e7 100644
--- a/internal/scheduler/story_orchestrator_test.go
+++ b/internal/scheduler/story_orchestrator_test.go
@@ -599,6 +599,228 @@ func TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady(t *testing.T) {
 	}
 }
 
+// seedNeedsFixStory drives a story through the full Builder -> Evaluators ->
+// Arbitration chain to a rejected verdict (mirrors
+// TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix's own setup),
+// leaving it at status NEEDS_FIX with its original (rejected) root task
+// still in the store -- the starting point ensureFixAttempt operates on.
+// Returns the store, the story (as currently persisted), the rejected root
+// task, the evaluators, and the arbitration task.
+func seedNeedsFixStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, []*task.Task, *task.Task) {
+	t.Helper()
+	store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted)
+	oldRoot, err := store.GetTask(st.RootTaskID)
+	if err != nil {
+		t.Fatalf("get root: %v", err)
+	}
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.Tick(context.Background()) // spawns arbitration
+
+	arbitrations := store.dependentsWithRole(evaluators[0].ID, "planner")
+	if len(arbitrations) != 1 {
+		t.Fatalf("expected 1 arbitration task, got %d", len(arbitrations))
+	}
+	arb := arbitrations[0]
+
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: false, Reasoning: "breaks the running-log panel styling"})
+	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)
+	}
+	store.setTaskState(arb.ID, task.StateCompleted)
+	store.setTaskSummary(arb.ID, "found a real problem")
+
+	orch.Tick(context.Background()) // finalizeArbitration runs, sets NEEDS_FIX
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var got *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			got = s
+		}
+	}
+	if got == nil || got.Status != "NEEDS_FIX" {
+		t.Fatalf("setup failed: story = %+v, want status NEEDS_FIX", got)
+	}
+	return store, got, oldRoot, evaluators, arb
+}
+
+// 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
+// at it (RootTaskID updated, Status reset to IN_PROGRESS) so the very next
+// tick re-enters the normal Builder->Evaluators->Arbitration flow against
+// the new attempt.
+func TestStoryOrchestrator_NeedsFix_SpawnsFixAttemptAndRepointsRoot(t *testing.T) {
+	store, st, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.Tick(context.Background())
+
+	fixAttempts := store.dependentsWithRole(oldRoot.ID, "builder")
+	if len(fixAttempts) != 1 {
+		t.Fatalf("expected exactly 1 fix-attempt task depending on the rejected root, got %d", len(fixAttempts))
+	}
+	fix := fixAttempts[0]
+	if fix.State != task.StateQueued {
+		t.Errorf("fix attempt State = %v, want QUEUED", fix.State)
+	}
+	if fix.ParentTaskID != "" {
+		t.Errorf("fix attempt ParentTaskID = %q, want empty (top-level, DAG sibling not delegated subtask)", fix.ParentTaskID)
+	}
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var got *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			got = s
+		}
+	}
+	if got == nil {
+		t.Fatal("story not found")
+	}
+	if got.RootTaskID != fix.ID {
+		t.Errorf("story RootTaskID = %q, want the new fix-attempt task %q", got.RootTaskID, fix.ID)
+	}
+	if got.Status != "IN_PROGRESS" {
+		t.Errorf("story Status = %q, want IN_PROGRESS", got.Status)
+	}
+}
+
+// TestStoryOrchestrator_NeedsFix_FixAttemptInstructionsIncludeRejectionReasoning
+// proves the fix attempt's instructions carry the arbitration's structured
+// rejection reasoning (read via the KindVerdictReported event), not just a
+// generic "try again" -- the whole point of automating this loop is that the
+// next attempt has something concrete to act on.
+func TestStoryOrchestrator_NeedsFix_FixAttemptInstructionsIncludeRejectionReasoning(t *testing.T) {
+	store, _, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.Tick(context.Background())
+
+	fixAttempts := store.dependentsWithRole(oldRoot.ID, "builder")
+	if len(fixAttempts) != 1 {
+		t.Fatalf("expected exactly 1 fix-attempt task, got %d", len(fixAttempts))
+	}
+	instructions := fixAttempts[0].Agent.Instructions
+	if !strings.Contains(instructions, "breaks the running-log panel styling") {
+		t.Errorf("fix-attempt instructions do not include the arbitration's rejection reasoning:\n%s", instructions)
+	}
+	if !strings.Contains(instructions, "Test Story") {
+		t.Errorf("fix-attempt instructions do not include the story name:\n%s", instructions)
+	}
+}
+
+// TestStoryOrchestrator_NeedsFix_IdempotentAcrossTicks proves repeated ticks
+// don't spawn more than one fix-attempt task -- after the first tick the
+// story's status flips to IN_PROGRESS, so the NEEDS_FIX branch simply isn't
+// re-entered on subsequent ticks (the normal Builder-not-complete-yet path
+// takes over instead, since the fix attempt is freshly QUEUED).
+func TestStoryOrchestrator_NeedsFix_IdempotentAcrossTicks(t *testing.T) {
+	store, _, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.Tick(context.Background())
+	orch.Tick(context.Background())
+	orch.Tick(context.Background())
+
+	fixAttempts := store.dependentsWithRole(oldRoot.ID, "builder")
+	if len(fixAttempts) != 1 {
+		t.Fatalf("expected exactly 1 fix-attempt task after repeated ticks, got %d", len(fixAttempts))
+	}
+}
+
+// TestStoryOrchestrator_EnsureFixAttempt_FindsExistingSpawnedAttempt proves
+// ensureFixAttempt's own structural idempotency: simulating a restart
+// between "fix task spawned" and "story RootTaskID/Status updated to point
+// at it" -- calling ensureFixAttempt again must find the already-spawned
+// builder-role dependent rather than spawning a second one, and still
+// complete the re-pointing.
+func TestStoryOrchestrator_EnsureFixAttempt_FindsExistingSpawnedAttempt(t *testing.T) {
+	store, st, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	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")
+	if err != nil {
+		t.Fatalf("seed pre-existing fix attempt: %v", err)
+	}
+
+	orch.ensureFixAttempt(context.Background(), st)
+
+	fixAttempts := store.dependentsWithRole(oldRoot.ID, "builder")
+	if len(fixAttempts) != 1 {
+		t.Fatalf("expected exactly 1 fix-attempt task (the pre-existing one, not a new one), got %d", len(fixAttempts))
+	}
+	if fixAttempts[0].ID != preexisting.ID {
+		t.Errorf("expected ensureFixAttempt to reuse the pre-existing task %s, got a different one %s", preexisting.ID, fixAttempts[0].ID)
+	}
+	if st.RootTaskID != preexisting.ID {
+		t.Errorf("story RootTaskID = %q, want the pre-existing fix attempt %q", st.RootTaskID, preexisting.ID)
+	}
+	if st.Status != "IN_PROGRESS" {
+		t.Errorf("story Status = %q, want IN_PROGRESS", st.Status)
+	}
+}
+
+// TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts proves the safety net:
+// once a chain of maxFixAttempts consecutive fix attempts already exists,
+// ensureFixAttempt stops spawning new ones and leaves the story at
+// NEEDS_FIX, exactly like today's fully-manual behavior.
+func TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts(t *testing.T) {
+	store, st, oldRoot, _, _ := seedNeedsFixStory(t)
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+
+	// Build a chain of maxFixAttempts prior fix-attempt tasks, each
+	// depending on the one before it, ending at oldRoot -- simulating a
+	// 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")
+		if err != nil {
+			t.Fatalf("seed fix-attempt chain: %v", err)
+		}
+		prev = nt
+	}
+	st.RootTaskID = prev.ID
+	if err := store.UpdateStory(st); err != nil {
+		t.Fatalf("persist chained root: %v", err)
+	}
+
+	orch.Tick(context.Background())
+
+	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))
+	}
+
+	stories, _ := store.ListStories(storage.StoryFilter{})
+	var got *story.Story
+	for _, s := range stories {
+		if s.ID == st.ID {
+			got = s
+		}
+	}
+	if got.Status != "NEEDS_FIX" {
+		t.Errorf("story Status = %q, want NEEDS_FIX (cap reached, no further automation)", got.Status)
+	}
+}
+
 // TestStoryOrchestrator_DoesNothing_WhenBuilderNotComplete proves the
 // orchestrator is inert for a story whose builder task hasn't reached
 // COMPLETED yet and isn't auto-acceptable either (still RUNNING — not