diff options
| -rw-r--r-- | internal/scheduler/story_orchestrator.go | 20 | ||||
| -rw-r--r-- | internal/scheduler/story_orchestrator_test.go | 118 | ||||
| -rw-r--r-- | internal/storage/seed.go | 32 | ||||
| -rw-r--r-- | internal/storage/seed_test.go | 52 |
4 files changed, 212 insertions, 10 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)) + } +} diff --git a/internal/storage/seed.go b/internal/storage/seed.go index b452969..e7a0ba0 100644 --- a/internal/storage/seed.go +++ b/internal/storage/seed.go @@ -75,6 +75,7 @@ func (s *DB) SeedRoleConfigs() error { configJSON string }{ {role: "builder", configJSON: builderRoleConfigJSON}, + {role: "planner", configJSON: plannerRoleConfigJSON}, } for _, seed := range seeds { if _, err := s.GetActiveRoleConfig(seed.role); err == nil { @@ -120,3 +121,34 @@ const builderRoleConfigJSON = `{ } ] }` + +// plannerRoleConfigJSON is the default role.RoleConfig for the "planner" +// role -- claudomator's arbitration role (see StoryOrchestrator.ensureArbitration +// in internal/scheduler/story_orchestrator.go). Added 2026-07-11 after a +// live production run demonstrated a real gap: with no system prompt at +// all, an arbitration agent completed without ever calling report_verdict, +// and finalizeArbitration's then-fail-open default (no verdict = approve) +// silently shipped work an evaluator had already flagged as factually +// wrong. finalizeArbitration was made fail-closed in the same change (no +// verdict now means rejection, not approval) -- this system prompt is the +// other half of that fix: telling the agent the tool exists and that +// calling it is mandatory, not just "hoping" a fail-closed default alone is +// enough forcing function. See report_verdict's schema in +// internal/executor/agentmcp.go's reportVerdictInput (approved bool, +// reasoning string). +const plannerRoleConfigJSON = `{ + "role": "planner", + "system_prompt": "You are operating as the arbitration role in Claudomator's recursive arbitrated-review system. You have been dispatched because a builder-role task's work has already been reviewed by 4 independent evaluators (quality, security, correctness, performance). Your job is to read each evaluator's findings and render the final verdict: does this work meet its acceptance criteria and ship, or does it need to go back for a fix?\n\n## You MUST call report_verdict before finishing\n\nThis is not optional. If you finish without calling report_verdict, the system treats that the same as an explicit rejection -- NOT an approval. There is no safe default here: skipping this call always sends the work back for a fix, even if it was actually fine. So every single time: read the evaluators' findings, form a judgment, and call report_verdict with approved (true or false) and reasoning before you finish.\n\n## How to read the evaluators' findings\n\nEach evaluator recorded its assessment as that task's summary and/or events -- your own task instructions name each evaluator task to check. Read all 4 before deciding. Weigh their findings against the acceptance criteria you were given, not against an abstract standard of perfection. A nitpick that doesn't affect correctness, security, or whether the work does what it claims is not grounds for rejection; a finding that the work is factually wrong, broken, or misses a stated acceptance criterion is.\n\n## Approve or reject\n\n- approved: true -- the work meets its acceptance criteria. The builder task is promoted to COMPLETED.\n- approved: false -- reject, with reasoning specific enough that whoever picks up the resulting fix-attempt knows exactly what to fix. A fresh fix-attempt task is automatically spawned carrying your reasoning verbatim.\n\nYour reasoning field is read by the next attempt at this exact task if you reject it -- write it as instructions for that future agent, not as a report to a human.", + "escalation_ladder": [ + { + "candidates": [{"provider": "claude", "model": "sonnet"}], + "selection_mode": "single", + "max_retries": 1 + }, + { + "candidates": [{"provider": "claude", "model": "opus"}], + "selection_mode": "single", + "max_retries": 0 + } + ] +}` diff --git a/internal/storage/seed_test.go b/internal/storage/seed_test.go index cd7c8e7..5b5b674 100644 --- a/internal/storage/seed_test.go +++ b/internal/storage/seed_test.go @@ -2,6 +2,7 @@ package storage import ( "encoding/json" + "strings" "testing" "github.com/thepeterstone/claudomator/internal/role" @@ -43,6 +44,43 @@ func TestSeedRoleConfigs_CreatesAndActivatesBuilder(t *testing.T) { } } +// TestSeedRoleConfigs_CreatesAndActivatesPlanner proves the arbitration +// role's system prompt is seeded too (added 2026-07-11 alongside +// finalizeArbitration's fail-closed default -- see that const's doc +// comment for why: a live run showed an arbitration agent with no system +// prompt at all never called report_verdict). Specifically checks the +// prompt actually names the report_verdict tool, since that's the whole +// point of this seed. +func TestSeedRoleConfigs_CreatesAndActivatesPlanner(t *testing.T) { + db := testDB(t) + + if err := db.SeedRoleConfigs(); err != nil { + t.Fatalf("SeedRoleConfigs: %v", err) + } + + row, err := db.GetActiveRoleConfig("planner") + if err != nil { + t.Fatalf("GetActiveRoleConfig(planner): %v", err) + } + if row.Version != 1 { + t.Errorf("expected version 1, got %d", row.Version) + } + + var cfg role.RoleConfig + if err := json.Unmarshal([]byte(row.ConfigJSON), &cfg); err != nil { + t.Fatalf("unmarshal seeded config: %v", err) + } + if cfg.Role != "planner" { + t.Errorf("cfg.Role = %q, want planner", cfg.Role) + } + if !strings.Contains(cfg.SystemPrompt, "report_verdict") { + t.Error("cfg.SystemPrompt does not mention report_verdict -- the whole point of this seed is mandating that call") + } + if len(cfg.EscalationLadder) != 2 { + t.Fatalf("expected 2 escalation tiers, got %d", len(cfg.EscalationLadder)) + } +} + func TestSeedRoleConfigs_Idempotent_DoesNotDuplicateOrOverwrite(t *testing.T) { db := testDB(t) @@ -53,12 +91,14 @@ func TestSeedRoleConfigs_Idempotent_DoesNotDuplicateOrOverwrite(t *testing.T) { t.Fatalf("second SeedRoleConfigs: %v", err) } - versions, err := db.ListRoleConfigVersions("builder") - if err != nil { - t.Fatalf("ListRoleConfigVersions: %v", err) - } - if len(versions) != 1 { - t.Fatalf("expected exactly 1 version after calling SeedRoleConfigs twice, got %d", len(versions)) + for _, r := range []string{"builder", "planner"} { + versions, err := db.ListRoleConfigVersions(r) + if err != nil { + t.Fatalf("ListRoleConfigVersions(%s): %v", r, err) + } + if len(versions) != 1 { + t.Fatalf("%s: expected exactly 1 version after calling SeedRoleConfigs twice, got %d", r, len(versions)) + } } } |