diff options
Diffstat (limited to 'internal/scheduler')
| -rw-r--r-- | internal/scheduler/story_orchestrator.go | 20 | ||||
| -rw-r--r-- | internal/scheduler/story_orchestrator_test.go | 118 |
2 files changed, 134 insertions, 4 deletions
diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go index a526552..31efa39 100644 --- a/internal/scheduler/story_orchestrator.go +++ b/internal/scheduler/story_orchestrator.go @@ -486,6 +486,16 @@ func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Sto // fix-attempt, capped) through different wiring suited to where a // story-level anchor exists (root) or doesn't (nested) -- not a special case // in the review mechanism itself, which stays 100% identical at every depth. +// Approval requires an EXPLICIT verdict with approved=true; an explicit +// rejection OR no structured verdict reported at all both reject the node. +// This is fail-closed by design: an earlier version defaulted to approval +// when no verdict was reported ("preserving prior behavior for agents that +// don't report a structured verdict"). A live production run (2026-07-11) +// demonstrated why that default is dangerous for a review gate: an +// arbitration agent that never calls report_verdict (because its role had +// no system prompt telling it to -- see SeedRoleConfigs) shipped work an +// evaluator had already found factually wrong, entirely undetected, because +// "no verdict" was silently treated the same as "approved." func (o *StoryOrchestrator) finalizeArbitration(ctx context.Context, st *story.Story, node, arbitration *task.Task) { events, err := o.Store.ListEvents(st.ID, 0) if err != nil { @@ -521,7 +531,10 @@ func (o *StoryOrchestrator) finalizeArbitration(ctx context.Context, st *story.S o.logf("story orchestrator: emit arbitration_decided", "storyID", st.ID, "error", err) } - if hasVerdict && !approved { + if !hasVerdict || !approved { + if !hasVerdict { + o.logf("story orchestrator: finalize arbitration: no verdict reported, treating as rejection (fail-closed)", "storyID", st.ID, "taskID", node.ID, "arbitrationID", arbitration.ID) + } if isRoot { st.Status = "NEEDS_FIX" if err := o.Store.UpdateStory(st); err != nil { @@ -533,9 +546,8 @@ func (o *StoryOrchestrator) finalizeArbitration(ctx context.Context, st *story.S return } - // Approved (or no structured verdict reported -- preserves the prior - // unconditional-approve default). node only becomes COMPLETED here, - // after arbitration. + // Approved -- an explicit, structured verdict with approved=true. node + // only becomes COMPLETED here, after arbitration. if err := o.Store.UpdateTaskState(node.ID, task.StateCompleted); err != nil { o.logf("story orchestrator: finalize arbitration: promote node to completed", "storyID", st.ID, "taskID", node.ID, "error", err) return diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go index 08bd155..34e828a 100644 --- a/internal/scheduler/story_orchestrator_test.go +++ b/internal/scheduler/story_orchestrator_test.go @@ -735,6 +735,15 @@ func TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady(t *t arb := arbitrations[0] store.setTaskState(arb.ID, task.StateCompleted) store.setTaskSummary(arb.ID, "ship it") + // finalizeArbitration is fail-closed (no verdict = rejection): an + // explicit approved=true verdict is required to reach REVIEW_READY. + payload0, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: true, Reasoning: "meets criteria"}) + if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: payload0}); err != nil { + t.Fatalf("seed verdict event: %v", err) + } // Tick 2: arbitration is now COMPLETED. orch.Tick(context.Background()) @@ -1236,6 +1245,15 @@ func TestStoryOrchestrator_AutoAcceptsReadyArbitration(t *testing.T) { // task — without ever calling POST /api/tasks/{id}/accept. store.setTaskState(arb.ID, task.StateReady) store.setTaskSummary(arb.ID, "approved") + // finalizeArbitration is fail-closed (no verdict = rejection): an + // explicit approved=true verdict is required to reach REVIEW_READY. + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: true, Reasoning: "meets 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) + } // Tick 2: orchestrator must auto-accept READY -> COMPLETED itself. orch.Tick(context.Background()) @@ -1426,6 +1444,16 @@ func TestStoryOrchestrator_EndToEnd(t *testing.T) { // Arbitration's execution succeeds (READY, not COMPLETED). store.setTaskState(arb.ID, task.StateReady) store.setTaskSummary(arb.ID, "approved") + // finalizeArbitration is fail-closed (no verdict = rejection): an + // explicit approved=true verdict is required for the story to reach + // REVIEW_READY and root to be promoted to COMPLETED below. + arbPayload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: true, Reasoning: "meets criteria"}) + if err := store.CreateEvent(&event.Event{TaskID: arb.ID, Kind: event.KindVerdictReported, Actor: event.ActorAgent, Payload: arbPayload}); err != nil { + t.Fatalf("seed verdict event: %v", err) + } orch.Tick(context.Background()) arbAfter, err := store.GetTask(arb.ID) @@ -1921,3 +1949,93 @@ func TestStoryOrchestrator_EndToEnd_NestedSubtask(t *testing.T) { t.Fatalf("expected REVIEW_READY once root's own arbitration approves it, got %q", statusOf()) } } + +// TestStoryOrchestrator_FinalizeArbitration_RootNoVerdict_TreatedAsRejection +// proves the fail-closed default added 2026-07-11: an arbitration task that +// completes without ever calling report_verdict (no KindVerdictReported +// event at all) must be treated as a REJECTION, not a silent approval. This +// replaces the earlier "no verdict defaults to approval" behavior, which a +// live production run demonstrated could ship work an evaluator had already +// flagged as factually wrong, entirely undetected. +func TestStoryOrchestrator_FinalizeArbitration_RootNoVerdict_TreatedAsRejection(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("no-verdict-root", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("no-verdict-story", root.ID, "VALIDATING") + store.stories[st.ID] = st + + arb := &task.Task{ + ID: "no-verdict-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "", // never called report_summary or report_verdict + } + store.tasks[arb.ID] = arb + // Deliberately no KindVerdictReported event seeded. + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.finalizeArbitration(context.Background(), st, root, arb) + + gotRoot, err := store.GetTask(root.ID) + if err != nil { + t.Fatal(err) + } + if gotRoot.State != task.StateReady { + t.Errorf("root State = %v, want unchanged READY (no verdict must not promote to COMPLETED)", gotRoot.State) + } + + stories, _ := store.ListStories(storage.StoryFilter{}) + var gotStory *story.Story + for _, s := range stories { + if s.ID == st.ID { + gotStory = s + } + } + if gotStory.Status != "NEEDS_FIX" { + t.Errorf("story Status = %q, want NEEDS_FIX (no verdict must be treated as rejection, not approval)", gotStory.Status) + } +} + +// TestStoryOrchestrator_FinalizeArbitration_NestedNoVerdict_SpawnsFixAttempt +// mirrors the root case above for a nested position: no structured verdict +// reported must spawn a fix-attempt directly, exactly like an explicit +// rejection would, not silently promote the node to COMPLETED. +func TestStoryOrchestrator_FinalizeArbitration_NestedNoVerdict_SpawnsFixAttempt(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("no-verdict-nested-root", task.StateBlocked) + store.tasks[root.ID] = root + st := newStoryWithRoot("no-verdict-nested-story", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + nested := builderTask("no-verdict-nested-node", task.StateReady) + nested.ParentTaskID = root.ID + store.tasks[nested.ID] = nested + + arb := &task.Task{ + ID: "no-verdict-nested-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + } + store.tasks[arb.ID] = arb + // Deliberately no KindVerdictReported event seeded. + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.finalizeArbitration(context.Background(), st, nested, arb) + + got, err := store.GetTask(nested.ID) + if err != nil { + t.Fatal(err) + } + if got.State != task.StateReady { + t.Errorf("nested node State = %v, want unchanged READY (no verdict must not promote to COMPLETED)", got.State) + } + + fixAttempts := store.dependentsWithRole(nested.ID, "builder") + if len(fixAttempts) != 1 { + t.Fatalf("expected exactly 1 fix-attempt task spawned for a no-verdict arbitration, got %d", len(fixAttempts)) + } +} |