diff options
| -rw-r--r-- | internal/scheduler/story_orchestrator.go | 146 | ||||
| -rw-r--r-- | internal/scheduler/story_orchestrator_test.go | 222 |
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 |