diff options
Diffstat (limited to 'docs')
| -rw-r--r-- | docs/superpowers/plans/2026-07-09-root-completion-follows-arbitration.md | 699 |
1 files changed, 699 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-07-09-root-completion-follows-arbitration.md b/docs/superpowers/plans/2026-07-09-root-completion-follows-arbitration.md new file mode 100644 index 0000000..04c43a7 --- /dev/null +++ b/docs/superpowers/plans/2026-07-09-root-completion-follows-arbitration.md @@ -0,0 +1,699 @@ +# Root Completion Follows Arbitration Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** A story's root (builder-role) task only transitions to `COMPLETED` once its own arbitration approves it — never eagerly the moment it reaches `READY`. Rejection leaves it at `READY` forever (superseded; `task.CurrentAttempt` resolves forward to whatever fix attempt gets spawned). + +**Architecture:** `StoryOrchestrator.processStory` stops calling `autoAccept` on the root task. `finalizeArbitration` gains a `root *task.Task` parameter and, on approval, is the sole place that ever promotes root to `COMPLETED`; on rejection it leaves root untouched. `READY` already satisfies a dependent's `DependsOn` (`executor.depDoneStates` includes both `StateCompleted` and `StateReady`), so a rejected-but-READY root is still immediately dispatchable as the anchor for `ensureFixAttempt`'s fix-attempt task — no change needed there. Evaluator and Arbitration tasks are not `builder`-role — they are the review mechanism itself, not something reviewed — so they keep their existing immediate-auto-accept behavior unchanged. + +**Tech Stack:** Go, existing `internal/scheduler` package, `internal/task`. + +## Global Constraints + +- This is piece 4a of `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`'s piece 4 ("Generalize `ensureEvaluators`/`ensureArbitration`/the fix-loop trigger to recurse into `builder`-role subtask trees"), split out as the smaller, safer first step: fixing the root's own completion semantics before generalizing the trigger to walk subtask trees (piece 4b, a separate future plan). +- Root cause this plan closes: today, `task.State == COMPLETED` for the story root means only "the agent's own execution finished" — arbitration is a side-annotation on `story.Status`, never touching the task's own state. That's tolerable only because `Story` has a separate status field to carry "has this been verified" independently of any task's raw state. A nested subtask in piece 4b will have no such separate wrapper — the only signal its parent's `maybeUnblockParent` can check is the subtask's own `task.State`. So `COMPLETED` must mean "verified by arbitration", uniformly, starting here at the root, before piece 4b can safely reuse the same mechanism one level down. +- No change to evaluator/arbitration/retro task auto-accept behavior — only the root's own completion semantics change. +- No change to `ensureFixAttempt`, `fixAttemptDepth`, `ensureEvaluators`, `ensureArbitration`, `rejectionReasoning`, `findEvaluators`, `findArbitration`, `taskTree`, `processRetro`, `autoAccept` itself, `maybeEmitVerdict`, `spawnRoleTask`, `dependsOnAll`, `formatAcceptanceCriteria` — verify by running the full existing test suite for this file and confirming these functions' own tests are unaffected (some of their *callers'* test fixtures do change — see Task 1 below). + +--- + +### Task 1: Root only reaches COMPLETED via arbitration approval + +**Files:** +- Modify: `internal/scheduler/story_orchestrator.go` (`processStory`, `finalizeArbitration`) +- Modify: `internal/scheduler/story_orchestrator_test.go` (fixture + several tests, detailed below) + +**Interfaces:** +- Consumes: `task.CurrentAttempt` (unchanged), `o.autoAccept` (unchanged, still used for evaluators/arbitration), `o.ensureEvaluators`/`o.ensureArbitration` (unchanged), `o.arbitrationVerdict` (unchanged), `executor.depDoneStates` (unchanged, already includes both `StateCompleted` and `StateReady` — confirmed by reading `internal/executor/executor.go`, no change needed there). +- Produces: `finalizeArbitration`'s new signature `func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, root, arbitration *task.Task)` — note the added `root` parameter, second positional argument. + +- [ ] **Step 1: Update `processStory` to stop auto-accepting root** + +In `internal/scheduler/story_orchestrator.go`, find: + +```go +func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) { + if st.Status == "NEEDS_FIX" { + o.ensureFixAttempt(ctx, st) + return + } + + root, err := task.CurrentAttempt(o.Store, st.RootTaskID) + if err != nil { + 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) + } + + // Stage 1: Builder -> Evaluators (+ story -> VALIDATING). + evaluators, ok := o.ensureEvaluators(ctx, st, root) + if !ok { + return // not all 4 could be found/created yet; retry next tick + } +``` + +Replace with: + +```go +func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) { + if st.Status == "NEEDS_FIX" { + o.ensureFixAttempt(ctx, st) + return + } + + root, err := task.CurrentAttempt(o.Store, st.RootTaskID) + if err != nil { + o.logf("story orchestrator: resolve current root attempt", "storyID", st.ID, "rootTaskID", st.RootTaskID, "error", err) + return + } + // 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). + evaluators, ok := o.ensureEvaluators(ctx, st, root) + if !ok { + return // not all 4 could be found/created yet; retry next tick + } +``` + +- [ ] **Step 2: Update the Stage 3 call site to pass `root` into `finalizeArbitration`** + +In the same file, immediately below (still inside `processStory`), find: + +```go + // Stage 3: Arbitration -> REVIEW_READY. + if arbitration.State == task.StateCompleted { + o.finalizeArbitration(st, arbitration) + } +} +``` + +Replace with: + +```go + // 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, root, arbitration) + } +} +``` + +- [ ] **Step 3: Update `finalizeArbitration` to take `root` and promote it on approval** + +In the same file, find the full function (including its doc comment): + +```go +// 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. +// +// 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) { + if st.Status != "VALIDATING" { + return + } + + approved, _, hasVerdict := o.arbitrationVerdict(st, arbitration) + + payload, _ := json.Marshal(struct { + 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, + Actor: event.ActorSystem, + Payload: payload, + }); err != nil { + 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 { + st.Status = "REVIEW_READY" + } + if err := o.Store.UpdateStory(st); err != nil { + o.logf("story orchestrator: update story status after arbitration", "storyID", st.ID, "status", st.Status, "error", err) + } +} +``` + +Replace with: + +```go +// finalizeArbitration handles the Arbitration task reaching COMPLETED: it +// 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, 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 + } + + approved, _, hasVerdict := o.arbitrationVerdict(st, arbitration) + + payload, _ := json.Marshal(struct { + 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, + Actor: event.ActorSystem, + Payload: payload, + }); err != nil { + o.logf("story orchestrator: emit arbitration_decided", "storyID", st.ID, "error", err) + } + + 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 { + o.logf("story orchestrator: update story status after arbitration", "storyID", st.ID, "status", st.Status, "error", err) + } +} +``` + +- [ ] **Step 4: Update `seedStoryWithEvaluators` to seed root at READY, not COMPLETED** + +In `internal/scheduler/story_orchestrator_test.go`, find: + +```go +func seedStoryWithEvaluators(t *testing.T, evalState task.State) (*fakeStoryStore, *story.Story, []*task.Task) { + t.Helper() + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateCompleted) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "VALIDATING") + store.stories[st.ID] = st +``` + +Replace with: + +```go +func seedStoryWithEvaluators(t *testing.T, evalState task.State) (*fakeStoryStore, *story.Story, []*task.Task) { + t.Helper() + store := newFakeStoryStore() + // 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 +``` + +- [ ] **Step 5: Update `seedDoneStory` to explicitly promote root to COMPLETED** + +In the same file, find: + +```go +func seedDoneStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, []*task.Task, *task.Task) { + t.Helper() + store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted) + st.Status = "DONE" + + ids := make([]string, len(evaluators)) + for i, ev := range evaluators { + ids[i] = ev.ID + } + arb := &task.Task{ + ID: "arbitration-1", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + DependsOn: ids, + State: task.StateCompleted, + Summary: "ship it", + } + store.tasks[arb.ID] = arb + + root, err := store.GetTask(st.RootTaskID) + if err != nil { + t.Fatal(err) + } + return store, st, root, evaluators, arb +} +``` + +Replace with: + +```go +func seedDoneStory(t *testing.T) (*fakeStoryStore, *story.Story, *task.Task, []*task.Task, *task.Task) { + t.Helper() + store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted) + st.Status = "DONE" + + ids := make([]string, len(evaluators)) + for i, ev := range evaluators { + ids[i] = ev.ID + } + arb := &task.Task{ + ID: "arbitration-1", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + DependsOn: ids, + State: task.StateCompleted, + Summary: "ship it", + } + 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) + } + return store, st, root, evaluators, arb +} +``` + +- [ ] **Step 6: Rename and update `TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes`** + +In the same file, find the full test (including its doc comment): + +```go +// TestStoryOrchestrator_SpawnsEvaluators_WhenBuilderCompletes is verification +// item (a): a builder task reaching COMPLETED 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) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateCompleted) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +Replace with: + +```go +// 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. 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.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +(The rest of this test's body — from `pool := &fakePool{}` through the final closing brace — is unchanged. Only the doc comment, function name, and the `builderTask` state argument change.) + +- [ ] **Step 7: Update `TestStoryOrchestrator_DoesNotDuplicateEvaluators`'s root seed** + +In the same file, find: + +```go +func TestStoryOrchestrator_DoesNotDuplicateEvaluators(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateCompleted) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +Replace with: + +```go +func TestStoryOrchestrator_DoesNotDuplicateEvaluators(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +(Rest of the test body unchanged.) + +- [ ] **Step 8: Replace `TestStoryOrchestrator_AutoAcceptsReadyBuilder`** + +In the same file, find the full test (including its doc comment): + +```go +// 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) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.Tick(context.Background()) + + got, err := store.GetTask(root.ID) + if err != nil { + t.Fatal(err) + } + if got.State != task.StateCompleted { + t.Fatalf("builder should be auto-accepted to COMPLETED, 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)) + } +} +``` + +Replace with: + +```go +// 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 + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.Tick(context.Background()) + + got, err := store.GetTask(root.ID) + if err != nil { + t.Fatal(err) + } + 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 reaches READY, got %d", len(deps)) + } +} +``` + +- [ ] **Step 9: Update `TestStoryOrchestrator_AutoAccept_DoesNotTouchUnrelatedReadyTask`'s root seed** + +In the same file, find: + +```go +func TestStoryOrchestrator_AutoAccept_DoesNotTouchUnrelatedReadyTask(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateCompleted) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +Replace with: + +```go +func TestStoryOrchestrator_AutoAccept_DoesNotTouchUnrelatedReadyTask(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("builder-1", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("story-1", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st +``` + +(Rest of the test body unchanged. `TestStoryOrchestrator_SkipsTerminalStories`, a few lines further down, keeps its root seeded at `task.StateCompleted` unchanged — its own doc comment already says "even if (hypothetically) their root task is COMPLETED", and `Tick`'s own top-level DONE/CANCELLED skip means `processStory` is never reached for it regardless of root's state, so it is unaffected by this plan and must NOT be modified.) + +- [ ] **Step 10: Update `TestStoryOrchestrator_EndToEnd`** + +In the same file, find: + +```go + // Builder's execution succeeds (RUNNING -> READY, exactly like + // handleRunResult) — no POST /api/tasks/{id}/accept call here. + store.setTaskState(root.ID, task.StateReady) + orch.Tick(context.Background()) + + rootAfter, err := store.GetTask(root.ID) + 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) + } +``` + +Replace with: + +```go + // Builder's execution succeeds (RUNNING -> READY, exactly like + // handleRunResult) — no POST /api/tasks/{id}/accept call here. + store.setTaskState(root.ID, task.StateReady) + orch.Tick(context.Background()) + + rootAfter, err := store.GetTask(root.ID) + if err != nil { + t.Fatal(err) + } + if rootAfter.State != task.StateReady { + t.Fatalf("builder must stay READY until arbitration approves it (not auto-accepted eagerly), got %v", rootAfter.State) + } +``` + +Then, further down in the same test, find: + +```go + // Arbitration's execution succeeds (READY, not COMPLETED). + store.setTaskState(arb.ID, task.StateReady) + store.setTaskSummary(arb.ID, "approved") + orch.Tick(context.Background()) + + arbAfter, err := store.GetTask(arb.ID) + if err != nil { + t.Fatal(err) + } + if arbAfter.State != task.StateCompleted { + t.Fatalf("arbitration should be auto-accepted to COMPLETED, got %v", arbAfter.State) + } + + if statusOf() != "REVIEW_READY" { + t.Fatalf("expected REVIEW_READY after arbitration completes, got %q", statusOf()) + } + decided := store.eventsOfKind(event.KindArbitrationDecided) + if len(decided) != 1 { + t.Fatalf("expected exactly 1 arbitration_decided event, got %d", len(decided)) + } +} +``` + +Replace with: + +```go + // Arbitration's execution succeeds (READY, not COMPLETED). + store.setTaskState(arb.ID, task.StateReady) + store.setTaskSummary(arb.ID, "approved") + orch.Tick(context.Background()) + + arbAfter, err := store.GetTask(arb.ID) + if err != nil { + t.Fatal(err) + } + if arbAfter.State != task.StateCompleted { + 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()) + } + decided := store.eventsOfKind(event.KindArbitrationDecided) + if len(decided) != 1 { + t.Fatalf("expected exactly 1 arbitration_decided event, got %d", len(decided)) + } +} +``` + +Also update this test's doc comment. Find: + +```go +// TestStoryOrchestrator_EndToEnd drives a story through the full chain — +// builder complete -> 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 +// 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. +func TestStoryOrchestrator_EndToEnd(t *testing.T) { +``` + +Replace with: + +```go +// TestStoryOrchestrator_EndToEnd drives a story through the full chain — +// 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. 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) — 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) { +``` + +- [ ] **Step 11: Add a new test proving rejection leaves root at READY, never COMPLETED** + +In the same file, immediately after `seedNeedsFixStory`'s closing brace (before `TestStoryOrchestrator_NeedsFix_SpawnsFixAttemptAndRepointsRoot` or whatever the next test in the file is), add: + +```go +// 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) + } +} +``` + +- [ ] **Step 12: Run the full scheduler package test suite** + +Run: `go test ./internal/scheduler/... -v -run 'TestStoryOrchestrator'` +Expected: PASS for every test in the file, including the renamed/rewritten ones above, the 3 unmodified fix-loop tests (`TestStoryOrchestrator_NeedsFix_SpawnsFixAttemptAndRepointsRoot`, `TestStoryOrchestrator_EnsureFixAttempt_FindsExistingSpawnedAttempt`, `TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts` — none of these assert on root's exact state before rejection, only on the fix-attempt task's own shape and the story's status/RootTaskID, so they are unaffected), the retro test cluster (unaffected — `seedDoneStory` now explicitly promotes root to COMPLETED to match its own narrative), and the two arbitration-verdict tests (`TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix`, `TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady` — unaffected, since they only assert on story.Status, not root.State). Paste the actual output. + +- [ ] **Step 13: Run the full repo test suite** + +Run: `go build ./...` then `go test ./...` (the entire repo). Both must pass — paste the actual output. If `internal/api` flakes under the race detector on a one-off "sql: database is closed" teardown error unrelated to this task's actual changes, rerun once before treating it as real. + +Also run `gofmt -l internal/scheduler/story_orchestrator.go internal/scheduler/story_orchestrator_test.go` before committing, and `gofmt -w` any file it flags that wasn't already gofmt-dirty before your changes (check with `git show HEAD:<path> | gofmt -l -` if unsure). + +- [ ] **Step 14: Commit and push** + +Run these exact commands, in order, confirming each succeeds: + +```bash +git remote -v +git add -A +git commit -m "refactor(scheduler): promote story root to COMPLETED only after arbitration approves it, not eagerly on READY" +git push origin main +git rev-parse HEAD +git ls-remote origin main +``` + +Do this commit+push step immediately after Step 11 (all edits done) if you are running under time/turn pressure — do not wait until Steps 12/13's verification is fully done to make your first commit attempt; verify locally first if you can, but a working local commit that's pushed is more valuable than a perfect but unpushed one. If `git ls-remote origin main`'s SHA doesn't match `git rev-parse HEAD`, the push did not really land — this container may not have this repository mounted at `origin`; if so, note that in your summary rather than treating it as your own error (a prior task in this same repo discovered that pushes are handled externally by claudomator's ContainerRunner after your run completes successfully — see this repo's own CLAUDE.md, "After a successful run, new commits are pushed from the workspace"). Run `git status` and `git log --oneline -1` and paste that output regardless. |