feat(scheduler): finalizeArbitration consults a structured verdict instead of always routing to REVIEW_READY

2 files changed, 165 insertions, 19 deletions

diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go
index b8e1b4c..b70b487 100644
--- a/internal/scheduler/story_orchestrator.go
+++ b/internal/scheduler/story_orchestrator.go
@@ -70,10 +70,9 @@ var evaluatorRoles = []string{
 }
 
 // arbitrationRole is the role assigned to the single task spawned once all
-// Evaluators for a story complete. It depends on all 4 Evaluator tasks and,
-// per this phase's documented simplification (see finalizeArbitration),
-// always routes the story to REVIEW_READY on completion rather than parsing
-// its summary for an approve/reject verdict.
+// Evaluators for a story complete. It depends on all 4 Evaluator tasks and
+// routes the story to REVIEW_READY or NEEDS_FIX based on a structured
+// KindVerdictReported event (see finalizeArbitration/arbitrationVerdict).
 const arbitrationRole = "planner"
 
 // retroRole is the role assigned to the single task spawned once a story
@@ -384,16 +383,11 @@ 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.
-//
-// Documented simplification (Phase 7b, see CLAUDE.md Design Debt): this does
-// NOT parse the arbitration task's summary for an approve/reject verdict —
-// it always routes to REVIEW_READY. A human or chatbot who reads the
-// arbitration summary and disagrees can manually set the story to NEEDS_FIX
-// via the existing PUT /api/stories/{id}. A later phase could close this gap
-// by giving the arbitration task a dedicated verdict-reporting tool (e.g. a
-// new AgentChannel method) whose structured output this orchestrator could
-// trust instead of free-text parsing.
+// 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.
 //
 // 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
@@ -407,10 +401,13 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
 		return
 	}
 
+	approved, hasVerdict := o.arbitrationVerdict(st, arbitration)
+
 	payload, _ := json.Marshal(struct {
-		TaskID  string `json:"task_id"`
-		Summary string `json:"summary"`
-	}{TaskID: arbitration.ID, Summary: arbitration.Summary})
+		TaskID   string `json:"task_id"`
+		Summary  string `json:"summary"`
+		Approved *bool  `json:"approved,omitempty"`
+	}{TaskID: arbitration.ID, Summary: arbitration.Summary, Approved: optionalBool(approved, hasVerdict)})
 	if err := o.Store.CreateEvent(&event.Event{
 		TaskID:  st.ID,
 		Kind:    event.KindArbitrationDecided,
@@ -420,10 +417,59 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
 		o.logf("story orchestrator: emit arbitration_decided", "storyID", st.ID, "error", err)
 	}
 
-	st.Status = "REVIEW_READY"
+	// 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 {
+		st.Status = "REVIEW_READY"
+	}
 	if err := o.Store.UpdateStory(st); err != nil {
-		o.logf("story orchestrator: update story to REVIEW_READY", "storyID", st.ID, "error", err)
+		o.logf("story orchestrator: update story status after arbitration", "storyID", st.ID, "status", st.Status, "error", err)
+	}
+}
+
+// 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) {
+	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
+	}
+	for _, e := range events {
+		if e.Kind != event.KindVerdictReported {
+			continue
+		}
+		var payload struct {
+			Approved bool `json:"approved"`
+		}
+		if json.Unmarshal(e.Payload, &payload) == nil {
+			approved = payload.Approved
+			hasVerdict = true
+		}
+	}
+	return approved, hasVerdict
+}
+
+// optionalBool returns a pointer to v if present is true, nil otherwise —
+// used so KindArbitrationDecided's payload omits "approved" entirely
+// (rather than encoding a misleading false) when no structured verdict was
+// ever reported.
+func optionalBool(v bool, present bool) *bool {
+	if !present {
+		return nil
 	}
+	return &v
 }
 
 // processRetro is the Phase 8 retro stage: it runs for every story the Tick
diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go
index b811206..0d96840 100644
--- a/internal/scheduler/story_orchestrator_test.go
+++ b/internal/scheduler/story_orchestrator_test.go
@@ -499,6 +499,106 @@ func TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady(t *t
 	}
 }
 
+// TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix proves that
+// when the arbitration task reports a structured approved=false verdict
+// (via a KindVerdictReported event, the same one AgentChannel.ReportVerdict
+// records), finalizeArbitration routes the story to NEEDS_FIX instead of
+// unconditionally REVIEW_READY.
+func TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix(t *testing.T) {
+	store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted)
+
+	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]
+
+	// Simulate the arbitration agent calling report_verdict with a rejection
+	// before finishing, the same event storeChannel.ReportVerdict records.
+	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
+
+	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.Status != "NEEDS_FIX" {
+		t.Errorf("story status: want NEEDS_FIX, got %q", got.Status)
+	}
+}
+
+// TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady proves the
+// mirror case: an explicit approved=true verdict still routes to
+// REVIEW_READY, same as today's unconditional behavior -- this isn't just
+// "absence of a verdict defaults to proceed", a real approval also proceeds.
+func TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady(t *testing.T) {
+	store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted)
+
+	pool := &fakePool{}
+	orch := &StoryOrchestrator{Store: store, Pool: pool}
+	orch.Tick(context.Background())
+
+	arbitrations := store.dependentsWithRole(evaluators[0].ID, "planner")
+	arb := arbitrations[0]
+
+	payload, _ := json.Marshal(struct {
+		Approved  bool   `json:"approved"`
+		Reasoning string `json:"reasoning"`
+	}{Approved: true, Reasoning: "meets all acceptance 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)
+	}
+
+	store.setTaskState(arb.ID, task.StateCompleted)
+	store.setTaskSummary(arb.ID, "ship it")
+
+	orch.Tick(context.Background())
+
+	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.Status != "REVIEW_READY" {
+		t.Errorf("story status: want REVIEW_READY, got %q", 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