summaryrefslogtreecommitdiff
path: root/internal
diff options
context:
space:
mode:
Diffstat (limited to 'internal')
-rw-r--r--internal/scheduler/story_orchestrator.go72
-rw-r--r--internal/scheduler/story_orchestrator_test.go105
2 files changed, 125 insertions, 52 deletions
diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go
index 8061b35..19383c6 100644
--- a/internal/scheduler/story_orchestrator.go
+++ b/internal/scheduler/story_orchestrator.go
@@ -226,9 +226,16 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
o.logf("story orchestrator: resolve current root attempt", "storyID", st.ID, "rootTaskID", st.RootTaskID, "error", err)
return
}
- root = o.autoAccept(st, root)
- if root.State != task.StateCompleted {
- return // Builder hasn't reached COMPLETED yet (still running, or not yet READY to auto-accept)
+ // root is NOT auto-accepted here. Unlike evaluator/arbitration tasks,
+ // root is builder-role: its own completion must mean "verified by
+ // arbitration", not merely "the agent finished" -- so it stays READY
+ // through the whole Evaluators/Arbitration cycle below, and only
+ // finalizeArbitration's approval branch ever promotes it to COMPLETED.
+ // READY already satisfies a dependent's DependsOn (see
+ // executor.depDoneStates), so evaluators/arbitration can depend on a
+ // still-READY root without issue.
+ if root.State != task.StateReady {
+ return // not yet finished (still running/blocked); or already COMPLETED, meaning a prior tick's finalizeArbitration already approved it -- nothing further to do
}
// Stage 1: Builder -> Evaluators (+ story -> VALIDATING).
@@ -259,9 +266,13 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) {
}
arbitration = o.autoAccept(st, arbitration)
- // Stage 3: Arbitration -> REVIEW_READY.
+ // Stage 3: Arbitration -> approve (root: READY -> COMPLETED, story ->
+ // REVIEW_READY) or reject (story -> NEEDS_FIX; a fix attempt is spawned
+ // next tick by ensureFixAttempt, depending on the still-READY root --
+ // task.CurrentAttempt resolves future lookups of this position forward
+ // to it).
if arbitration.State == task.StateCompleted {
- o.finalizeArbitration(st, arbitration)
+ o.finalizeArbitration(st, root, arbitration)
}
}
@@ -388,20 +399,32 @@ 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 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.
+// emits KindArbitrationDecided, then either approves or rejects root based on
+// whether the arbitration task recorded a structured KindVerdictReported
+// event (AgentChannel.ReportVerdict). An arbitration task that never calls
+// report_verdict defaults to approval, preserving the prior behavior for
+// agents that don't report a structured verdict.
+//
+// On approval, root is promoted READY -> COMPLETED *here* — not eagerly the
+// moment it first reached READY — so COMPLETED means "verified by
+// arbitration", uniformly, the same rule this design applies at every depth
+// of a builder-role task tree (see
+// docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md).
+// On rejection, root is left untouched at READY: it has been superseded, and
+// task.CurrentAttempt will resolve future lookups of this position forward
+// to whatever fix-attempt task ensureFixAttempt spawns once the story lands
+// at NEEDS_FIX below — READY already satisfies a dependent's DependsOn (see
+// executor.depDoneStates), so the fix attempt is immediately dispatchable
+// without root ever becoming COMPLETED.
//
// 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
-// status — this is the one place in the orchestrator where the story's own
-// status field, not a structural dependents check, is the idempotency guard,
-// because by this stage there's nothing further to check structurally: the
-// Arbitration task is the last task in the chain, so "does a subsequent task
-// exist" isn't an available signal.
-func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *task.Task) {
+// already advanced past REVIEW_READY) don't re-emit the event, re-promote
+// root, or re-write the status — this is the one place in the orchestrator
+// where the story's own status field, not a structural dependents check, is
+// the idempotency guard, because by this stage there's nothing further to
+// check structurally: the Arbitration task is the last task in the chain, so
+// "does a subsequent task exist" isn't an available signal.
+func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, root, arbitration *task.Task) {
if st.Status != "VALIDATING" {
return
}
@@ -422,17 +445,16 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *ta
o.logf("story orchestrator: emit arbitration_decided", "storyID", st.ID, "error", err)
}
- // 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 {
+ // Approved (or no structured verdict reported -- preserves the
+ // prior unconditional-approve default). root only becomes COMPLETED
+ // here, after arbitration.
+ if err := o.Store.UpdateTaskState(root.ID, task.StateCompleted); err != nil {
+ o.logf("story orchestrator: finalize arbitration: promote root to completed", "storyID", st.ID, "taskID", root.ID, "error", err)
+ return
+ }
st.Status = "REVIEW_READY"
}
if err := o.Store.UpdateStory(st); err != nil {
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)
generated by cgit v1.2.3 (git 2.39.1) at 2026-07-16 08:04:29 +0000