diff options
Diffstat (limited to 'internal/scheduler/story_orchestrator_test.go')
| -rw-r--r-- | internal/scheduler/story_orchestrator_test.go | 105 |
1 files changed, 78 insertions, 27 deletions
diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go index 200e38e..a13659d 100644 --- a/internal/scheduler/story_orchestrator_test.go +++ b/internal/scheduler/story_orchestrator_test.go @@ -213,13 +213,15 @@ func newStoryWithRoot(id, rootTaskID, status string) *story.Story { return &story.Story{ID: id, Name: "Test Story", Status: status, RootTaskID: rootTaskID} } -// TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes is verification -// item (a): a builder task reaching COMPLETED for a story spawns exactly 4 +// TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderReady is verification +// item (a): a builder task reaching READY for a story spawns exactly 4 // evaluator tasks with correct roles/depends_on, moves the story to -// VALIDATING. -func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes(t *testing.T) { +// VALIDATING. The builder itself stays READY (not COMPLETED) — see +// TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved +// for that specific assertion. +func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderReady(t *testing.T) { store := newFakeStoryStore() - root := builderTask("builder-1", task.StateCompleted) + root := builderTask("builder-1", task.StateReady) store.tasks[root.ID] = root st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") store.stories[st.ID] = st @@ -282,7 +284,7 @@ func TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes(t *testing.T) { // duplicates. func TestStoryOrchestrator_DoesNotDuplicateEvaluators(t *testing.T) { store := newFakeStoryStore() - root := builderTask("builder-1", task.StateCompleted) + root := builderTask("builder-1", task.StateReady) store.tasks[root.ID] = root st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") store.stories[st.ID] = st @@ -321,7 +323,13 @@ func evaluatorTask(id, rootID, role string, state task.State) *task.Task { func seedStoryWithEvaluators(t *testing.T, evalState task.State) (*fakeStoryStore, *story.Story, []*task.Task) { t.Helper() store := newFakeStoryStore() - root := builderTask("builder-1", task.StateCompleted) + // root is READY, not COMPLETED: under the new rule, root only reaches + // COMPLETED once its own arbitration approves it (see + // finalizeArbitration), and every caller of this helper is simulating a + // point in the pipeline before that approval has happened yet. + // seedDoneStory (below) explicitly promotes root to COMPLETED itself for + // its own already-approved narrative. + root := builderTask("builder-1", task.StateReady) store.tasks[root.ID] = root st := newStoryWithRoot("story-1", root.ID, "VALIDATING") store.stories[st.ID] = st @@ -654,6 +662,25 @@ func seedNeedsFixStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, return store, got, oldRoot, evaluators, arb } +// TestStoryOrchestrator_ArbitrationRejects_LeavesRootReadyNotCompleted proves +// the core "verified before complete" invariant this plan adds: a rejected +// root is left at READY — never promoted to COMPLETED. Only +// finalizeArbitration's approval branch ever writes root to COMPLETED; a +// rejection must never look "done" to anything that trusts task.State +// directly, the same gate a nested subtask's parent will rely on once +// arbitrated review recurses into subtask trees (a future plan). +func TestStoryOrchestrator_ArbitrationRejects_LeavesRootReadyNotCompleted(t *testing.T) { + store, _, oldRoot, _, _ := seedNeedsFixStory(t) + + got, err := store.GetTask(oldRoot.ID) + if err != nil { + t.Fatal(err) + } + if got.State != task.StateReady { + t.Errorf("rejected root State = %v, want READY (must never be promoted to COMPLETED on rejection)", got.State) + } +} + // 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 @@ -857,14 +884,18 @@ func TestStoryOrchestrator_DoesNothing_WhenBuilderNotComplete(t *testing.T) { } } -// TestStoryOrchestrator_AutoAcceptsReadyBuilder is the core regression test -// for the auto-accept fix: a builder task sitting at READY (execution -// succeeded, awaiting what would otherwise be a manual -// POST /api/tasks/{id}/accept) is transitioned to COMPLETED by the -// orchestrator itself — with no external accept call — and, because that -// unblocks Stage 1 in the same tick, the 4 evaluators are spawned -// immediately too. -func TestStoryOrchestrator_AutoAcceptsReadyBuilder(t *testing.T) { +// TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved +// replaces the old "auto-accept the builder immediately" regression test: a +// builder task sitting at READY (execution succeeded, awaiting what would +// otherwise be a manual POST /api/tasks/{id}/accept) triggers the 4 +// evaluators to spawn in the same tick — but, unlike evaluators/arbitration +// tasks, the builder itself is NOT auto-accepted to COMPLETED here. It stays +// READY (which already satisfies a dependent's DependsOn — see +// executor.depDoneStates) until its own arbitration approves it (see +// finalizeArbitration). This is what makes "COMPLETED" mean "verified", +// uniformly — the same rule a nested builder subtask will need once +// arbitrated review recurses into subtask trees. +func TestStoryOrchestrator_ReadyBuilder_SpawnsEvaluators_StaysReadyUntilApproved(t *testing.T) { store := newFakeStoryStore() root := builderTask("builder-1", task.StateReady) store.tasks[root.ID] = root @@ -879,13 +910,13 @@ func TestStoryOrchestrator_AutoAcceptsReadyBuilder(t *testing.T) { if err != nil { t.Fatal(err) } - if got.State != task.StateCompleted { - t.Fatalf("builder should be auto-accepted to COMPLETED, got %v", got.State) + if got.State != task.StateReady { + t.Fatalf("builder must stay READY until arbitration approves it, got %v", got.State) } deps, _ := store.ListDependents(root.ID) if len(deps) != 4 { - t.Fatalf("expected 4 evaluator tasks spawned in the same tick the builder auto-accepts, got %d", len(deps)) + t.Fatalf("expected 4 evaluator tasks spawned in the same tick the builder reaches READY, got %d", len(deps)) } } @@ -969,7 +1000,7 @@ func TestStoryOrchestrator_AutoAcceptsReadyArbitration(t *testing.T) { // an unrelated READY task that merely happens to exist in the store. func TestStoryOrchestrator_AutoAccept_DoesNotTouchUnrelatedReadyTask(t *testing.T) { store := newFakeStoryStore() - root := builderTask("builder-1", task.StateCompleted) + root := builderTask("builder-1", task.StateReady) store.tasks[root.ID] = root st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") store.stories[st.ID] = st @@ -1031,18 +1062,21 @@ func TestStoryOrchestrator_SkipsTerminalStories(t *testing.T) { } // TestStoryOrchestrator_EndToEnd drives a story through the full chain — -// builder complete -> evaluators -> arbitration -> REVIEW_READY — using only +// builder ready -> evaluators -> arbitration -> REVIEW_READY — using only // the fake store/pool, proving the whole ceremony holds together end to end // at the orchestrator level (verification item 5, the higher-level test). // // Every task in the chain is driven to READY (never directly to COMPLETED), // mirroring exactly what executor.Pool.handleRunResult does for a real -// top-level task whose execution succeeds — proving the orchestrator's own +// top-level task whose execution succeeds. Evaluators and arbitration are +// still carried the rest of the way to COMPLETED by the orchestrator's own // auto-accept (not an external POST /api/tasks/{id}/accept call, which this -// test never makes) is what carries each task the rest of the way to -// COMPLETED and advances the story. The only accept call anywhere in this -// flow is the story-level one, which isn't part of this test — it's covered -// separately in internal/api's story accept-gate tests. +// test never makes) — but the builder itself is the one exception: it stays +// READY through the whole cycle and is only promoted to COMPLETED once +// arbitration actually approves it, proving "COMPLETED means verified" holds +// end to end, not just in isolated unit tests. The only accept call anywhere +// in this flow is the story-level one, which isn't part of this test — it's +// covered separately in internal/api's story accept-gate tests. func TestStoryOrchestrator_EndToEnd(t *testing.T) { store := newFakeStoryStore() root := builderTask("builder-1", task.StatePending) @@ -1068,8 +1102,8 @@ func TestStoryOrchestrator_EndToEnd(t *testing.T) { if err != nil { t.Fatal(err) } - if rootAfter.State != task.StateCompleted { - t.Fatalf("builder should be auto-accepted to COMPLETED without an accept call, got %v", rootAfter.State) + if rootAfter.State != task.StateReady { + t.Fatalf("builder must stay READY until arbitration approves it (not auto-accepted eagerly), got %v", rootAfter.State) } deps, _ := store.ListDependents(root.ID) @@ -1135,6 +1169,14 @@ func TestStoryOrchestrator_EndToEnd(t *testing.T) { t.Fatalf("arbitration should be auto-accepted to COMPLETED, got %v", arbAfter.State) } + rootFinal, err := store.GetTask(root.ID) + if err != nil { + t.Fatal(err) + } + if rootFinal.State != task.StateCompleted { + t.Fatalf("builder should finally be promoted to COMPLETED now that arbitration approved it, got %v", rootFinal.State) + } + if statusOf() != "REVIEW_READY" { t.Fatalf("expected REVIEW_READY after arbitration completes, got %q", statusOf()) } @@ -1168,6 +1210,15 @@ func seedDoneStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, []* } store.tasks[arb.ID] = arb + // By the time a story reaches DONE, its root has already been through a + // real (or simulated) approval and is COMPLETED. seedStoryWithEvaluators + // seeds root at READY (the pre-arbitration state every other caller of + // that helper needs), so promote it here to match this fixture's own + // DONE narrative. + if err := store.UpdateTaskState(st.RootTaskID, task.StateCompleted); err != nil { + t.Fatalf("promote root to completed for DONE fixture: %v", err) + } + root, err := store.GetTask(st.RootTaskID) if err != nil { t.Fatal(err) |