From 063a6c1661a91370e2f70d7fab670acd09561619 Mon Sep 17 00:00:00 2001 From: Claudomator Agent Date: Fri, 10 Jul 2026 05:49:36 +0000 Subject: refactor(scheduler): generalize finalizeArbitration to any builder-role node, add nested fix-attempt spawning and acceptance-criteria fallback --- internal/scheduler/story_orchestrator.go | 198 +++++++++++++----- internal/scheduler/story_orchestrator_test.go | 277 +++++++++++++++++++++++++- 2 files changed, 420 insertions(+), 55 deletions(-) (limited to 'internal/scheduler') diff --git a/internal/scheduler/story_orchestrator.go b/internal/scheduler/story_orchestrator.go index 19383c6..d190ba7 100644 --- a/internal/scheduler/story_orchestrator.go +++ b/internal/scheduler/story_orchestrator.go @@ -260,7 +260,7 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) { } // Stage 2: Evaluators -> Arbitration. - arbitration, ok := o.ensureArbitration(ctx, st, evaluators) + arbitration, ok := o.ensureArbitration(ctx, st, root, evaluators) if !ok { return } @@ -272,7 +272,7 @@ func (o *StoryOrchestrator) processStory(ctx context.Context, st *story.Story) { // task.CurrentAttempt resolves future lookups of this position forward // to it). if arbitration.State == task.StateCompleted { - o.finalizeArbitration(st, root, arbitration) + o.finalizeArbitration(ctx, st, root, arbitration) } } @@ -332,13 +332,18 @@ func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Stor } } + criteria := root.AcceptanceCriteria + if len(criteria) == 0 { + criteria = st.AcceptanceCriteria + } + spawnedAny := false for _, r := range evaluatorRoles { if _, ok := found[r]; ok { continue } - nt, err := o.spawnRoleTask(ctx, fmt.Sprintf("%s: %s", r, st.Name), r, []string{root.ID}, root, - fmt.Sprintf("Evaluate the changes made by task %s against the %q dimension for story %q.\n\nStory spec:\n%s", root.ID, r, st.Name, st.Spec)) + nt, err := o.spawnRoleTask(ctx, fmt.Sprintf("%s: %s", r, st.Name), r, []string{root.ID}, "", root, + fmt.Sprintf("Evaluate the changes made by task %s against the %q dimension for story %q.\n\nStory spec:\n%s\n\nAcceptance criteria:\n%s", root.ID, r, st.Name, st.Spec, formatAcceptanceCriteria(criteria))) if err != nil { o.logf("story orchestrator: spawn evaluator", "storyID", st.ID, "role", r, "error", err) continue @@ -369,7 +374,9 @@ func (o *StoryOrchestrator) ensureEvaluators(ctx context.Context, st *story.Stor // structural: it looks for an existing "planner"-role dependent of the first // evaluator task that depends on every evaluator ID, rather than relying on // story.Status (which a human can freely rewrite via PUT /api/stories/{id}). -func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Story, evaluators []*task.Task) (*task.Task, bool) { +// node is the task being evaluated (root or nested) -- used only to read its +// own AcceptanceCriteria, falling back to the story's if node has none set. +func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Story, node *task.Task, evaluators []*task.Task) (*task.Task, bool) { ids := make([]string, len(evaluators)) for i, ev := range evaluators { ids[i] = ev.ID @@ -386,11 +393,15 @@ func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Sto } } + criteria := node.AcceptanceCriteria + if len(criteria) == 0 { + criteria = st.AcceptanceCriteria + } instructions := fmt.Sprintf( - "Arbitrate the 4 evaluator verdicts for story %q (task %s). Read each evaluator task's summary/events "+ - "and decide whether the story is ready to ship. Acceptance criteria:\n%s", - st.Name, st.ID, formatAcceptanceCriteria(st.AcceptanceCriteria)) - nt, err := o.spawnRoleTask(ctx, "Arbitration: "+st.Name, arbitrationRole, ids, evaluators[0], instructions) + "Arbitrate the 4 evaluator verdicts for task %q (%s) within story %q. Read each evaluator task's summary/events "+ + "and decide whether this task is ready to ship. Acceptance criteria:\n%s", + node.Name, node.ID, st.Name, formatAcceptanceCriteria(criteria)) + nt, err := o.spawnRoleTask(ctx, "Arbitration: "+st.Name, arbitrationRole, ids, "", evaluators[0], instructions) if err != nil { o.logf("story orchestrator: spawn arbitration", "storyID", st.ID, "error", err) return nil, false @@ -398,38 +409,63 @@ func (o *StoryOrchestrator) ensureArbitration(ctx context.Context, st *story.Sto return nt, true } -// 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 +// finalizeArbitration handles an Arbitration task reaching COMPLETED for any +// builder-role node in a story's tree -- root or nested -- and either +// approves or rejects that node 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 +// Idempotency is structural and keyed to this specific arbitration task, not +// story.Status (a story-level field only the root position has any claim +// to): it checks the story's own event stream for a KindArbitrationDecided +// event already referencing arbitration.ID, and does nothing further if +// found. This is what lets this same function run for every builder node in +// a story's tree without a node-specific analog of story.Status to gate on. +// +// On approval, node is promoted READY -> COMPLETED *here* -- not eagerly the +// moment it first reached READY -- so COMPLETED means "verified by +// arbitration", uniformly, at every depth (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 +// On rejection, node 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. +// to whatever fix-attempt gets spawned. // -// 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" { +// st.Status is only ever touched here when node IS the story's root +// (node.ParentTaskID == "", true only for the position anchored at +// st.RootTaskID within this story's own tree): REVIEW_READY on approval, +// NEEDS_FIX on rejection -- exactly as before this function was +// generalized. A rejected ROOT's fix-attempt is still spawned by +// ensureFixAttempt on the next tick, polling st.Status == "NEEDS_FIX" -- +// unchanged, since a Story row is a natural, already-existing anchor for +// that trigger. A rejected NESTED node has no story-level field to poll, so +// its fix-attempt is spawned directly, right here (spawnNestedFixAttempt, +// below), respecting the same maxFixAttempts cap via fixAttemptDepth. Both +// paths produce the identical semantic outcome (a same-role, same-position +// 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. +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 { + o.logf("story orchestrator: finalize arbitration: list story events", "storyID", st.ID, "error", err) return } + for _, e := range events { + if e.Kind != event.KindArbitrationDecided { + continue + } + var payload struct { + TaskID string `json:"task_id"` + } + if json.Unmarshal(e.Payload, &payload) == nil && payload.TaskID == arbitration.ID { + return // already finalized + } + } - approved, _, hasVerdict := o.arbitrationVerdict(st, arbitration) + approved, reasoning, hasVerdict := o.arbitrationVerdict(st, arbitration) + isRoot := node.ParentTaskID == "" payload, _ := json.Marshal(struct { TaskID string `json:"task_id"` @@ -446,19 +482,68 @@ func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, root, arbitrati } 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) + if isRoot { + st.Status = "NEEDS_FIX" + 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) + } return } + o.spawnNestedFixAttempt(ctx, st, node, reasoning) + return + } + + // Approved (or no structured verdict reported -- preserves the prior + // unconditional-approve default). 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 + } + if isRoot { 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) + } } - 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) +} + +// spawnNestedFixAttempt handles a rejected NESTED builder node +// (node.ParentTaskID != "") -- the counterpart to ensureFixAttempt, which +// handles the root position via st.Status == "NEEDS_FIX" polling. A nested +// position has no story-level field to poll, so its fix-attempt is spawned +// directly here, the moment rejection is discovered. Idempotency and the +// maxFixAttempts cap mirror ensureFixAttempt exactly: look for an existing +// role-matched dependent before spawning, and refuse to spawn a new one past +// the cap (leaving node's position permanently unresolved, node's own parent +// stuck BLOCKED forever -- the same "leave it for a human" resolution the +// root's own cap already accepts, per this design's Non-Goals). +func (o *StoryOrchestrator) spawnNestedFixAttempt(ctx context.Context, st *story.Story, node *task.Task, reasoning string) { + dependents, err := o.Store.ListDependents(node.ID) + if err != nil { + o.logf("story orchestrator: nested fix attempt: list node dependents", "storyID", st.ID, "nodeID", node.ID, "error", err) + return + } + for _, d := range dependents { + if d.Agent.Role == node.Agent.Role { + return // fix attempt already spawned + } + } + + if depth := o.fixAttemptDepth(node); depth >= maxFixAttempts { + o.logf("story orchestrator: nested fix attempt: max fix attempts reached, leaving node rejected for a human", "storyID", st.ID, "nodeID", node.ID, "depth", depth) + return + } + + if reasoning == "" { + reasoning = "(no arbitration reasoning found)" + } + instructions := fmt.Sprintf( + "A previous attempt at task %q (%s) within story %q was rejected by arbitration. Fix the issues and resubmit.\n\n"+ + "Acceptance criteria:\n%s\n\nArbitration's rejection reasoning:\n%s", + node.Name, node.ID, st.Name, formatAcceptanceCriteria(node.AcceptanceCriteria), reasoning) + if _, err := o.spawnRoleTask(ctx, "Fix attempt: "+node.Name, node.Agent.Role, []string{node.ID}, node.ParentTaskID, node, instructions); err != nil { + o.logf("story orchestrator: nested fix attempt: spawn", "storyID", st.ID, "nodeID", node.ID, "error", err) } } @@ -564,7 +649,7 @@ func (o *StoryOrchestrator) ensureFixAttempt(ctx context.Context, st *story.Stor "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) + 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 @@ -679,7 +764,7 @@ func (o *StoryOrchestrator) processRetro(ctx context.Context, st *story.Story) { } if retro == nil { instructions := o.buildRetroInstructions(st, root, arbitration) - nt, err := o.spawnRoleTask(ctx, "Retro: "+st.Name, retroRole, []string{arbitration.ID}, root, instructions) + nt, err := o.spawnRoleTask(ctx, "Retro: "+st.Name, retroRole, []string{arbitration.ID}, "", root, instructions) if err != nil { o.logf("story orchestrator: retro: spawn", "storyID", st.ID, "error", err) return @@ -957,19 +1042,26 @@ func (o *StoryOrchestrator) maybeEmitVerdict(st *story.Story, ev *task.Task) { } } -// spawnRoleTask creates a new role-typed, top-level task (no ParentTaskID — -// these are DAG siblings via DependsOn, not delegated subtasks; see -// internal/executor.Pool.cascadeFail's doc comment for why that distinction -// matters) and submits it to the pool. Agent.Type/Model are left empty so -// Phase 5's role-based dispatch resolves them from the role's escalation -// ladder on first dispatch (internal/executor.Pool.execute()); if no active -// role_configs row exists for the role, that same code path logs a warning -// and dispatches without role resolution — an accepted degraded mode we -// don't special-case here either. -func (o *StoryOrchestrator) spawnRoleTask(ctx context.Context, name, roleName string, dependsOn []string, template *task.Task, instructions string) (*task.Task, error) { +// spawnRoleTask creates a new role-typed task and submits it to the pool. +// parentTaskID is "" for the DAG-sibling tasks every caller except +// spawnNestedFixAttempt creates (Evaluators, Arbitration, Retro, and the +// root's own fix-attempts spawned by ensureFixAttempt are always top-level — +// see internal/executor.Pool.cascadeFail's doc comment for why that +// distinction matters); spawnNestedFixAttempt is the one caller that passes +// a non-empty parentTaskID, so a rejected nested node's fix-attempt lands at +// the same tree position as the node it supersedes (required for +// task.CurrentAttempt's ParentTaskID-equality matching to resolve it). +// Agent.Type/Model are left empty so Phase 5's role-based dispatch resolves +// them from the role's escalation ladder on first dispatch +// (internal/executor.Pool.execute()); if no active role_configs row exists +// for the role, that same code path logs a warning and dispatches without +// role resolution — an accepted degraded mode we don't special-case here +// either. +func (o *StoryOrchestrator) spawnRoleTask(ctx context.Context, name, roleName string, dependsOn []string, parentTaskID string, template *task.Task, instructions string) (*task.Task, error) { nt := &task.Task{ ID: uuid.NewString(), Name: name, + ParentTaskID: parentTaskID, Project: template.Project, RepositoryURL: template.RepositoryURL, Agent: task.AgentConfig{ diff --git a/internal/scheduler/story_orchestrator_test.go b/internal/scheduler/story_orchestrator_test.go index a13659d..03d4437 100644 --- a/internal/scheduler/story_orchestrator_test.go +++ b/internal/scheduler/story_orchestrator_test.go @@ -444,6 +444,279 @@ func TestStoryOrchestrator_EmitsEvalVerdict_OncePerEvaluator(t *testing.T) { } } +// TestStoryOrchestrator_FinalizeArbitration_NestedNodeApproved_DoesNotTouchStoryStatus +// proves finalizeArbitration, generalized to operate on any builder-role +// node (not just the story root), promotes a NESTED node to COMPLETED on +// approval without touching story.Status at all -- REVIEW_READY is reserved +// for the actual root position (node.ParentTaskID == ""). +func TestStoryOrchestrator_FinalizeArbitration_NestedNodeApproved_DoesNotTouchStoryStatus(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("nested-approve-root", task.StateBlocked) + store.tasks[root.ID] = root + st := newStoryWithRoot("nested-approve-story", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + nested := builderTask("nested-approve-node", task.StateReady) + nested.ParentTaskID = root.ID + store.tasks[nested.ID] = nested + + arb := &task.Task{ + ID: "nested-approve-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "ship it", + } + store.tasks[arb.ID] = arb + 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) + } + + 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.StateCompleted { + t.Errorf("nested node State = %v, want COMPLETED", got.State) + } + + stories, _ := store.ListStories(storage.StoryFilter{}) + var gotStory *story.Story + for _, s := range stories { + if s.ID == st.ID { + gotStory = s + } + } + if gotStory.Status != "IN_PROGRESS" { + t.Errorf("story Status = %q, want unchanged IN_PROGRESS (only root's own arbitration should touch story.Status)", gotStory.Status) + } +} + +// TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_SpawnsFixAttemptDirectly +// proves the nested-rejection path: since a nested position has no +// story-level field to poll (unlike the root's NEEDS_FIX/ensureFixAttempt +// flow), its fix-attempt is spawned immediately, right here -- same role, +// same ParentTaskID as the rejected node (so task.CurrentAttempt resolves +// it correctly), and story.Status is left untouched. +func TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_SpawnsFixAttemptDirectly(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("nested-reject-root", task.StateBlocked) + store.tasks[root.ID] = root + st := newStoryWithRoot("nested-reject-story", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + nested := builderTask("nested-reject-node", task.StateReady) + nested.ParentTaskID = root.ID + store.tasks[nested.ID] = nested + + arb := &task.Task{ + ID: "nested-reject-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "found a real problem", + } + store.tasks[arb.ID] = arb + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: false, Reasoning: "breaks the widget"}) + 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) + } + + 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("rejected nested node State = %v, want unchanged READY", got.State) + } + + fixAttempts := store.dependentsWithRole(nested.ID, "builder") + if len(fixAttempts) != 1 { + t.Fatalf("expected exactly 1 fix-attempt task depending on the rejected nested node, got %d", len(fixAttempts)) + } + fix := fixAttempts[0] + if fix.ParentTaskID != nested.ParentTaskID { + t.Errorf("fix attempt ParentTaskID = %q, want %q (same position as the rejected node)", fix.ParentTaskID, nested.ParentTaskID) + } + if fix.State != task.StateQueued { + t.Errorf("fix attempt State = %v, want QUEUED", fix.State) + } + + stories, _ := store.ListStories(storage.StoryFilter{}) + var gotStory *story.Story + for _, s := range stories { + if s.ID == st.ID { + gotStory = s + } + } + if gotStory.Status != "IN_PROGRESS" { + t.Errorf("story Status = %q, want unchanged IN_PROGRESS", gotStory.Status) + } +} + +// TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_CapsAtMaxFixAttempts +// mirrors the root's own safety net (TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts) +// for a nested position: once a chain of maxFixAttempts consecutive fix +// attempts already exists at this position, no further fix attempt is +// spawned. +func TestStoryOrchestrator_FinalizeArbitration_NestedNodeRejected_CapsAtMaxFixAttempts(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("nested-cap-root", task.StateBlocked) + store.tasks[root.ID] = root + st := newStoryWithRoot("nested-cap-story", root.ID, "IN_PROGRESS") + store.stories[st.ID] = st + + nested := builderTask("nested-cap-node", task.StateReady) + nested.ParentTaskID = root.ID + store.tasks[nested.ID] = nested + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + + // Build a chain of maxFixAttempts prior fix-attempt tasks at this same + // nested position, ending at prev -- simulating a position that has + // already exhausted its automatic fix attempts. + prev := nested + for i := 0; i < maxFixAttempts; i++ { + nt, err := orch.spawnRoleTask(context.Background(), fmt.Sprintf("Fix attempt %d", i), "builder", []string{prev.ID}, root.ID, root, "fix it") + if err != nil { + t.Fatalf("seed fix-attempt chain: %v", err) + } + prev = nt + } + + arb := &task.Task{ + ID: "nested-cap-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "still broken", + } + store.tasks[arb.ID] = arb + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: false, Reasoning: "still broken"}) + 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) + } + + orch.finalizeArbitration(context.Background(), st, prev, arb) + + 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)) + } +} + +// TestStoryOrchestrator_FinalizeArbitration_RootStillSetsStoryStatus proves +// the generalized finalizeArbitration still drives story.Status exactly as +// before when node IS the root position (node.ParentTaskID == "") -- the +// one case where touching story.Status is still correct, now decided by +// node.ParentTaskID rather than being the function's only mode. +func TestStoryOrchestrator_FinalizeArbitration_RootStillSetsStoryStatus(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("root-status-root", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("root-status-story", root.ID, "VALIDATING") + store.stories[st.ID] = st + + arb := &task.Task{ + ID: "root-status-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "ship it", + } + store.tasks[arb.ID] = arb + 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) + } + + 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.StateCompleted { + t.Errorf("root State = %v, want 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 != "REVIEW_READY" { + t.Errorf("story Status = %q, want REVIEW_READY (root's own approval must still drive it)", gotStory.Status) + } +} + +// TestStoryOrchestrator_FinalizeArbitration_IdempotentViaArbitrationDecidedEvent +// proves the generalized idempotency check (a KindArbitrationDecided event +// already referencing this arbitration.ID) correctly no-ops a repeated call +// for the same already-decided arbitration -- the mechanism every node in a +// story's tree now shares, replacing the root-only story.Status == +// "VALIDATING" gate. +func TestStoryOrchestrator_FinalizeArbitration_IdempotentViaArbitrationDecidedEvent(t *testing.T) { + store := newFakeStoryStore() + root := builderTask("idempotent-root", task.StateReady) + store.tasks[root.ID] = root + st := newStoryWithRoot("idempotent-story", root.ID, "VALIDATING") + store.stories[st.ID] = st + + arb := &task.Task{ + ID: "idempotent-arb", + Name: "Arbitration", + Agent: task.AgentConfig{Role: arbitrationRole}, + State: task.StateCompleted, + Summary: "ship it", + } + store.tasks[arb.ID] = arb + 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) + } + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.finalizeArbitration(context.Background(), st, root, arb) + orch.finalizeArbitration(context.Background(), st, root, arb) + orch.finalizeArbitration(context.Background(), st, root, arb) + + decided := store.eventsOfKind(event.KindArbitrationDecided) + if len(decided) != 1 { + t.Fatalf("expected exactly 1 arbitration_decided event after 3 calls, got %d", len(decided)) + } +} + // TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady is // verification item (d): arbitration reaching COMPLETED emits // KindArbitrationDecided and moves the story to REVIEW_READY. @@ -788,7 +1061,7 @@ func TestStoryOrchestrator_EnsureFixAttempt_FindsExistingSpawnedAttempt(t *testi 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") + 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) } @@ -827,7 +1100,7 @@ func TestStoryOrchestrator_NeedsFix_CapsAtMaxFixAttempts(t *testing.T) { // 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") + 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) } -- cgit v1.2.3