diff options
Diffstat (limited to 'internal/scheduler')
| -rw-r--r-- | internal/scheduler/story_orchestrator.go | 84 | ||||
| -rw-r--r-- | internal/scheduler/story_orchestrator_test.go | 100 |
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 |