diff options
Diffstat (limited to 'docs/superpowers/plans')
| -rw-r--r-- | docs/superpowers/plans/2026-07-08-subtask-ordering-and-verdict-reporting.md | 978 |
1 files changed, 978 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-07-08-subtask-ordering-and-verdict-reporting.md b/docs/superpowers/plans/2026-07-08-subtask-ordering-and-verdict-reporting.md new file mode 100644 index 0000000..511a68d --- /dev/null +++ b/docs/superpowers/plans/2026-07-08-subtask-ordering-and-verdict-reporting.md @@ -0,0 +1,978 @@ +# Subtask Ordering + Structured Verdict Reporting — 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:** Give `spawn_subtask` the ability to express ordering between the subtasks it creates, and give arbitration a structured, mechanically-parseable pass/fail signal instead of always routing to `REVIEW_READY` regardless of what the arbitration task actually decided. + +**Architecture:** Two small, independent additions to existing extension points. `SubtaskSpec` gains an optional `DependsOn []string` field, wired straight into the `task.DependsOn` the executor already gates dispatch on — no new scheduling logic. A new `AgentChannel.ReportVerdict` method, exposed as a `report_verdict` tool on both transports (native tool-use loop and MCP), mirrors `ProposeRoleConfig` exactly: it records a `KindVerdictReported` event on the calling task's own ID. `StoryOrchestrator.finalizeArbitration` is extended to look for that event on the arbitration task before deciding `REVIEW_READY` vs `NEEDS_FIX`, falling back to today's unconditional `REVIEW_READY` when no verdict event exists (backward compatible with any agent that doesn't call the new tool). + +**Tech Stack:** Go 1.25, SQLite (existing `events` table, additive — no migration needed since events already tolerate any `Kind` string), the existing `mcp` package for MCP tool registration. + +## Global Constraints + +- This plan is Plan 1 of a larger recursive-story-decomposition design + (`docs/superpowers/specs/2026-07-08-recursive-story-decomposition-design.md`). + It does not implement recursion, the `builder` role's decompose-or-implement + judgment, or the story-creation entry point — those are deliberately + out of scope here, planned separately once `internal/scheduler/story_orchestrator_test.go` + (1023 lines of existing convention) has been read in full. +- Both new features must be backward compatible: an agent that never calls + `report_verdict`, and a `spawn_subtask` call that never sets `depends_on`, + must behave exactly as they do today. No existing test may need to change + its expected behavior — only new tests are added. +- Follow existing patterns exactly: `ReportVerdict`/`report_verdict` mirrors + `ProposeRoleConfig`/`propose_role_config` field-for-field, comment-style + for comment-style. `SubtaskSpec.DependsOn` mirrors how `Role` was added to + the same struct ("Backward-compatible default... every pre-existing + caller unchanged"). + +--- + +## File Map + +| File | Change | +|---|---| +| `internal/agentchannel/agentchannel.go` | Add `DependsOn` to `SubtaskSpec`; add `ReportVerdict` to `AgentChannel` interface | +| `internal/event/event.go` | Add `KindVerdictReported` | +| `internal/executor/channel.go` | `storeChannel.SpawnSubtask` sets `child.DependsOn`; add `storeChannel.ReportVerdict` | +| `internal/executor/channel_test.go` | Tests for both | +| `internal/agentloop/tools.go` | Add `depends_on` to `spawn_subtask`'s schema; add `report_verdict` tool + case handler | +| `internal/executor/agentmcp.go` | Add `DependsOn` to `spawnSubtaskInput`; add `reportVerdictInput` + `report_verdict` MCP tool | +| `internal/scheduler/story_orchestrator.go` | `finalizeArbitration` consults `KindVerdictReported` before deciding `REVIEW_READY` vs `NEEDS_FIX` | +| `internal/scheduler/story_orchestrator_test.go` | Tests for the new branch | + +--- + +## Task 1: `SubtaskSpec.DependsOn` + +**Files:** +- Modify: `internal/agentchannel/agentchannel.go:33-43` (`SubtaskSpec`) +- Modify: `internal/executor/channel.go:136-164` (`storeChannel.SpawnSubtask`) +- Test: `internal/executor/channel_test.go` (append) + +**Interfaces:** +- Consumes: nothing new. +- Produces: `SubtaskSpec.DependsOn []string` — when non-empty, the created child task's `task.DependsOn` is set to it, and the existing executor dispatch-gating/cascade-fail-on-dependency-failure machinery (already built for top-level tasks) applies unchanged. Consumed by Task 2's tool-surface wiring (this task only touches the channel layer, not the tools that call it). + +- [ ] **Step 1: Write the failing test** + +Add to `internal/executor/channel_test.go` (find the existing `TestStoreChannel_SpawnSubtask...` tests and add this alongside them — check the file for the exact existing test names first so this doesn't duplicate a helper): + +```go +// TestStoreChannel_SpawnSubtask_SetsDependsOn proves a caller can chain a +// new subtask after a sibling it already spawned, by passing the sibling's +// returned ID back in DependsOn -- the mechanism a decomposing task uses to +// express "this step must wait for that one", not just fan-out-and-wait. +func TestStoreChannel_SpawnSubtask_SetsDependsOn(t *testing.T) { + store := &fakeChannelStore{} + ch := newStoreChannel(store, "parent-task-1") + + firstID, err := ch.SpawnSubtask(context.Background(), agentchannel.SubtaskSpec{ + Name: "step 1", + Instructions: "do the first thing", + }) + if err != nil { + t.Fatalf("spawn first subtask: %v", err) + } + + secondID, err := ch.SpawnSubtask(context.Background(), agentchannel.SubtaskSpec{ + Name: "step 2", + Instructions: "do the second thing", + DependsOn: []string{firstID}, + }) + if err != nil { + t.Fatalf("spawn second subtask: %v", err) + } + + var second *task.Task + for _, ct := range store.createdTasks { + if ct.ID == secondID { + second = ct + } + } + if second == nil { + t.Fatal("second subtask not found in store.createdTasks") + } + if len(second.DependsOn) != 1 || second.DependsOn[0] != firstID { + t.Errorf("expected DependsOn=[%q], got %v", firstID, second.DependsOn) + } +} + +// TestStoreChannel_SpawnSubtask_DependsOnDefaultsEmpty proves the +// backward-compatible default: a caller that never sets DependsOn (every +// pre-existing caller) gets a child task with no dependencies, exactly as +// before this change. +func TestStoreChannel_SpawnSubtask_DependsOnDefaultsEmpty(t *testing.T) { + store := &fakeChannelStore{} + ch := newStoreChannel(store, "parent-task-1") + + id, err := ch.SpawnSubtask(context.Background(), agentchannel.SubtaskSpec{ + Name: "solo step", + Instructions: "do the thing", + }) + if err != nil { + t.Fatalf("spawn subtask: %v", err) + } + + var got *task.Task + for _, ct := range store.createdTasks { + if ct.ID == id { + got = ct + } + } + if got == nil { + t.Fatal("subtask not found in store.createdTasks") + } + if len(got.DependsOn) != 0 { + t.Errorf("expected empty DependsOn by default, got %v", got.DependsOn) + } +} +``` + +`fakeChannelStore.createdTasks` is a `[]*task.Task` slice (confirmed by reading `internal/executor/channel_test.go:18-38` directly), appended to by the fake's `CreateTask` implementation — both tests above search it by ID rather than indexing a map, since that's the actual shape. + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/executor/ -run TestStoreChannel_SpawnSubtask_DependsOn -v` +Expected: FAIL — `SubtaskSpec` has no field `DependsOn` (compile error). + +- [ ] **Step 3: Add the field to `SubtaskSpec`** + +In `internal/agentchannel/agentchannel.go`, change: + +```go + // Role, when non-empty, names an internal/role.RoleConfig the child task + // should dispatch through (task.AgentConfig.Role — see Phase 5's + // role-based dispatch in internal/executor.Pool.execute()). When set, the + // storeChannel implementation leaves Agent.Type/Model empty on the child + // so tier 0 of the role's escalation ladder resolves them, exactly like + // any other role-typed task. Empty (the default) preserves the prior + // hardcoded "claude" behavior for backward compatibility. + Role string +} +``` + +to: + +```go + // Role, when non-empty, names an internal/role.RoleConfig the child task + // should dispatch through (task.AgentConfig.Role — see Phase 5's + // role-based dispatch in internal/executor.Pool.execute()). When set, the + // storeChannel implementation leaves Agent.Type/Model empty on the child + // so tier 0 of the role's escalation ladder resolves them, exactly like + // any other role-typed task. Empty (the default) preserves the prior + // hardcoded "claude" behavior for backward compatibility. + Role string + // DependsOn, when non-empty, names sibling task IDs (returned by a prior + // SpawnSubtask call in the same decomposition) this new subtask must wait + // for — set directly on the created child's task.DependsOn, so the + // existing dispatch-gating and cascade-fail-on-dependency-failure + // machinery (already built for top-level tasks) applies unchanged. Empty + // (the default) preserves today's behavior: every spawned subtask is + // structurally independent/parallel. + DependsOn []string +} +``` + +- [ ] **Step 4: Wire it through in `storeChannel.SpawnSubtask`** + +In `internal/executor/channel.go`, find: + +```go + child := &task.Task{ + ID: uuid.NewString(), + Name: spec.Name, + ParentTaskID: c.taskID, + Agent: agent, + Priority: task.PriorityNormal, + State: task.StatePending, + CreatedAt: now, + UpdatedAt: now, + } +``` + +and change it to: + +```go + child := &task.Task{ + ID: uuid.NewString(), + Name: spec.Name, + ParentTaskID: c.taskID, + Agent: agent, + Priority: task.PriorityNormal, + DependsOn: spec.DependsOn, + State: task.StatePending, + CreatedAt: now, + UpdatedAt: now, + } +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `go test ./internal/executor/ -run TestStoreChannel_SpawnSubtask_DependsOn -v` +Expected: PASS, both tests. + +- [ ] **Step 6: Run the full package suite to check for regressions** + +Run: `go test ./internal/executor/... ./internal/agentchannel/...` +Expected: PASS for everything — this change is purely additive to a struct and one assignment line. + +- [ ] **Step 7: Commit** + +```bash +git add internal/agentchannel/agentchannel.go internal/executor/channel.go internal/executor/channel_test.go +git commit -m "feat(agentchannel): add DependsOn to SubtaskSpec so a decomposing task can order its own subtasks" +``` + +--- + +## Task 2: Expose `depends_on` on the `spawn_subtask` tool (both transports) + +**Files:** +- Modify: `internal/agentloop/tools.go` (schema + case handler) +- Modify: `internal/executor/agentmcp.go` (`spawnSubtaskInput` + tool call) + +**Interfaces:** +- Consumes: `agentchannel.SubtaskSpec.DependsOn` (Task 1). +- Produces: nothing new for later tasks — this is the last mile connecting the primitive to what an agent can actually call. + +- [ ] **Step 1: Add `depends_on` to the native tool-use loop's schema** + +In `internal/agentloop/tools.go`, find the `spawn_subtask` tool definition: + +```go + { + Name: "spawn_subtask", + Description: "Create a child task to be executed separately. Use this to break large work into focused pieces.", + ParametersJSONSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": strProp("short descriptive name for the subtask"), + "instructions": strProp("complete instructions for the subtask agent"), + "model": strProp("optional model override"), + "max_budget_usd": map[string]any{"type": "number", "description": "optional budget cap in USD"}, + "role": strProp("optional role name to dispatch the subtask through instead of a fixed model (e.g. an evaluator role); when set, model is ignored and the role's escalation ladder picks the provider/model"), + }, + "required": []string{"name", "instructions"}, + }, + }, +``` + +Change to (adding `depends_on`): + +```go + { + Name: "spawn_subtask", + Description: "Create a child task to be executed separately. Use this to break large work into focused pieces.", + ParametersJSONSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "name": strProp("short descriptive name for the subtask"), + "instructions": strProp("complete instructions for the subtask agent"), + "model": strProp("optional model override"), + "max_budget_usd": map[string]any{"type": "number", "description": "optional budget cap in USD"}, + "role": strProp("optional role name to dispatch the subtask through instead of a fixed model (e.g. an evaluator role); when set, model is ignored and the role's escalation ladder picks the provider/model"), + "depends_on": map[string]any{"type": "array", "items": map[string]any{"type": "string"}, "description": "optional list of sibling subtask IDs (returned by prior spawn_subtask calls in this same decomposition) this subtask must wait for before it can run"}, + }, + "required": []string{"name", "instructions"}, + }, + }, +``` + +- [ ] **Step 2: Wire the field through the case handler** + +In the same file, find: + +```go + case "spawn_subtask": + var a struct { + Name string `json:"name"` + Instructions string `json:"instructions"` + Model string `json:"model"` + MaxBudgetUSD float64 `json:"max_budget_usd"` + Role string `json:"role"` + } + _ = json.Unmarshal([]byte(argsJSON), &a) + id, ssErr := l.Channel.SpawnSubtask(ctx, agentchannel.SubtaskSpec{ + Name: a.Name, + Instructions: a.Instructions, + Model: a.Model, + MaxBudgetUSD: a.MaxBudgetUSD, + Role: a.Role, + }) +``` + +Change to: + +```go + case "spawn_subtask": + var a struct { + Name string `json:"name"` + Instructions string `json:"instructions"` + Model string `json:"model"` + MaxBudgetUSD float64 `json:"max_budget_usd"` + Role string `json:"role"` + DependsOn []string `json:"depends_on"` + } + _ = json.Unmarshal([]byte(argsJSON), &a) + id, ssErr := l.Channel.SpawnSubtask(ctx, agentchannel.SubtaskSpec{ + Name: a.Name, + Instructions: a.Instructions, + Model: a.Model, + MaxBudgetUSD: a.MaxBudgetUSD, + Role: a.Role, + DependsOn: a.DependsOn, + }) +``` + +- [ ] **Step 3: Add `DependsOn` to the MCP transport's input struct** + +In `internal/executor/agentmcp.go`, find: + +```go +type spawnSubtaskInput struct { + Name string `json:"name" jsonschema:"short descriptive name for the subtask"` + Instructions string `json:"instructions" jsonschema:"complete instructions for the subtask agent"` + Model string `json:"model,omitempty" jsonschema:"optional model override, e.g. sonnet or opus"` + MaxBudgetUSD float64 `json:"max_budget_usd,omitempty" jsonschema:"optional budget cap in USD"` + Role string `json:"role,omitempty" jsonschema:"optional role name to dispatch the subtask through instead of a fixed model (e.g. an evaluator role); when set, model is ignored and the role's escalation ladder picks the provider/model"` +} +``` + +Change to: + +```go +type spawnSubtaskInput struct { + Name string `json:"name" jsonschema:"short descriptive name for the subtask"` + Instructions string `json:"instructions" jsonschema:"complete instructions for the subtask agent"` + Model string `json:"model,omitempty" jsonschema:"optional model override, e.g. sonnet or opus"` + MaxBudgetUSD float64 `json:"max_budget_usd,omitempty" jsonschema:"optional budget cap in USD"` + Role string `json:"role,omitempty" jsonschema:"optional role name to dispatch the subtask through instead of a fixed model (e.g. an evaluator role); when set, model is ignored and the role's escalation ladder picks the provider/model"` + DependsOn []string `json:"depends_on,omitempty" jsonschema:"optional list of sibling subtask IDs (returned by prior spawn_subtask calls in this same decomposition) this subtask must wait for before it can run"` +} +``` + +- [ ] **Step 4: Wire it through the MCP tool call** + +In the same file, find: + +```go + mcp.AddTool(s, &mcp.Tool{ + Name: "spawn_subtask", + Description: "Create a child task to be executed separately. Use this to break large work into focused pieces, then finish your turn.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in spawnSubtaskInput) (*mcp.CallToolResult, any, error) { + id, err := ch.SpawnSubtask(ctx, SubtaskSpec{ + Name: in.Name, + Instructions: in.Instructions, + Model: in.Model, + MaxBudgetUSD: in.MaxBudgetUSD, + Role: in.Role, + }) +``` + +Change to: + +```go + mcp.AddTool(s, &mcp.Tool{ + Name: "spawn_subtask", + Description: "Create a child task to be executed separately. Use this to break large work into focused pieces, then finish your turn.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in spawnSubtaskInput) (*mcp.CallToolResult, any, error) { + id, err := ch.SpawnSubtask(ctx, SubtaskSpec{ + Name: in.Name, + Instructions: in.Instructions, + Model: in.Model, + MaxBudgetUSD: in.MaxBudgetUSD, + Role: in.Role, + DependsOn: in.DependsOn, + }) +``` + +- [ ] **Step 5: Build and run the full suite** + +Run: `go build ./... && go test ./internal/agentloop/... ./internal/executor/...` +Expected: PASS — no new tests in this task (the underlying behavior is already covered by Task 1's tests; this task only threads a field through two already-tested call sites, matching how `Role` itself was threaded through without its own dedicated schema-plumbing test). + +- [ ] **Step 6: Commit** + +```bash +git add internal/agentloop/tools.go internal/executor/agentmcp.go +git commit -m "feat(agentloop,executor): expose depends_on on the spawn_subtask tool" +``` + +--- + +## Task 3: `KindVerdictReported` event kind + +**Files:** +- Modify: `internal/event/event.go` + +**Interfaces:** +- Produces: `event.KindVerdictReported`. Consumed by Task 4 (`storeChannel.ReportVerdict`) and Task 6 (`finalizeArbitration`). + +- [ ] **Step 1: Add the constant** + +In `internal/event/event.go`, find: + +```go + // KindRoleConfigProposed records a single AgentChannel.ProposeRoleConfig + // call (Phase 8): a retro-role agent proposing a new draft role_configs + // version for a role. Attached to the *calling task's* own ID (the retro + // task), payload {role, version} — internal/scheduler.StoryOrchestrator + // reads a retro task's own event stream for these once the task + // completes, to assemble the aggregate KindRetroCaptured payload it + // attaches to the story's ID. Mirrors KindEpicProposed's "one event per + // proposal call" convention. + KindRoleConfigProposed Kind = "role_config_proposed" +) +``` + +Change to: + +```go + // KindRoleConfigProposed records a single AgentChannel.ProposeRoleConfig + // call (Phase 8): a retro-role agent proposing a new draft role_configs + // version for a role. Attached to the *calling task's* own ID (the retro + // task), payload {role, version} — internal/scheduler.StoryOrchestrator + // reads a retro task's own event stream for these once the task + // completes, to assemble the aggregate KindRetroCaptured payload it + // attaches to the story's ID. Mirrors KindEpicProposed's "one event per + // proposal call" convention. + KindRoleConfigProposed Kind = "role_config_proposed" + + // KindVerdictReported records a single AgentChannel.ReportVerdict call: + // an evaluating agent (e.g. an arbitration-role task) reporting a + // structured approve/reject decision instead of leaving it to be + // inferred from free-text summary parsing. Attached to the *calling + // task's own* ID, payload {approved, reasoning} — + // internal/scheduler.StoryOrchestrator's finalizeArbitration reads the + // arbitration task's own event stream for this once the task completes, + // to decide REVIEW_READY vs NEEDS_FIX. Mirrors KindRoleConfigProposed's + // "one event per call, attached to the calling task" convention. + KindVerdictReported Kind = "verdict_reported" +) +``` + +- [ ] **Step 2: Build** + +Run: `go build ./...` +Expected: succeeds (this is a pure addition, nothing consumes it yet). + +- [ ] **Step 3: Commit** + +```bash +git add internal/event/event.go +git commit -m "feat(event): add KindVerdictReported" +``` + +--- + +## Task 4: `AgentChannel.ReportVerdict` + `storeChannel` implementation + +**Files:** +- Modify: `internal/agentchannel/agentchannel.go` (interface) +- Modify: `internal/executor/channel.go` (implementation) +- Test: `internal/executor/channel_test.go` (append) + +**Interfaces:** +- Consumes: `event.KindVerdictReported` (Task 3). +- Produces: `AgentChannel.ReportVerdict(ctx context.Context, approved bool, reasoning string) error`. Consumed by Task 5 (tool exposure). + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/executor/channel_test.go`, mirroring the three `ProposeRoleConfig` tests already in that file (`TestStoreChannel_ProposeRoleConfig_CreatesDraftRow` etc. — read them again if needed, this mirrors their structure, not their content): + +```go +// TestStoreChannel_ReportVerdict_EmitsVerdictReportedEvent proves calling +// ReportVerdict records a KindVerdictReported event attached to the +// *calling task's* own ID, payload {approved, reasoning} — +// internal/scheduler.StoryOrchestrator's finalizeArbitration later reads an +// arbitration task's own event stream for this to decide REVIEW_READY vs +// NEEDS_FIX. Mirrors TestStoreChannel_ProposeRoleConfig_EmitsRoleConfigProposedEvent. +func TestStoreChannel_ReportVerdict_EmitsVerdictReportedEvent(t *testing.T) { + store := &fakeChannelStore{} + ch := newStoreChannel(store, "arbitration-task-1") + + if err := ch.ReportVerdict(context.Background(), false, "the CSS change breaks the running-log panel"); err != nil { + t.Fatalf("ReportVerdict: %v", err) + } + + if len(store.createdEvents) != 1 { + t.Fatalf("expected 1 event, got %d", len(store.createdEvents)) + } + ev := store.createdEvents[0] + if ev.Kind != event.KindVerdictReported { + t.Errorf("expected KindVerdictReported, got %q", ev.Kind) + } + if ev.Actor != event.ActorAgent { + t.Errorf("expected ActorAgent, got %q", ev.Actor) + } + if ev.TaskID != "arbitration-task-1" { + t.Errorf("expected event attached to calling task ID, got %q", ev.TaskID) + } + + var payload struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + } + if err := json.Unmarshal(ev.Payload, &payload); err != nil { + t.Fatalf("unmarshal payload: %v", err) + } + if payload.Approved != false || payload.Reasoning != "the CSS change breaks the running-log panel" { + t.Errorf("unexpected payload: %+v", payload) + } +} + +// TestStoreChannel_ReportVerdict_Approved proves the approved=true case +// round-trips correctly too — not just the rejection path exercised above. +func TestStoreChannel_ReportVerdict_Approved(t *testing.T) { + store := &fakeChannelStore{} + ch := newStoreChannel(store, "arbitration-task-2") + + if err := ch.ReportVerdict(context.Background(), true, "meets all acceptance criteria"); err != nil { + t.Fatalf("ReportVerdict: %v", err) + } + + var payload struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + } + if err := json.Unmarshal(store.createdEvents[0].Payload, &payload); err != nil { + t.Fatalf("unmarshal payload: %v", err) + } + if !payload.Approved { + t.Error("expected approved=true to round-trip as true") + } +} +``` + +- [ ] **Step 2: Run tests to verify they fail** + +Run: `go test ./internal/executor/ -run TestStoreChannel_ReportVerdict -v` +Expected: FAIL — `ReportVerdict` is not a method on `*storeChannel` yet (compile error). + +- [ ] **Step 3: Add `ReportVerdict` to the `AgentChannel` interface** + +In `internal/agentchannel/agentchannel.go`, find: + +```go + // ProposeRoleConfig lets a retro-role agent (internal/scheduler. + // StoryOrchestrator's Phase 8 retro stage) propose a new draft + // role_configs version for a role, after reflecting on a completed + // story's history (task tree, escalations, cost, evaluator/arbitration + // feedback). Implementations create a new "draft"-status role_configs + // row and return its version number — they never touch whatever version + // is currently "active" for that role; promoting a draft to active stays + // a human action via the existing POST /api/roles/{role}/activate. + ProposeRoleConfig(ctx context.Context, config role.RoleConfig) (version int, err error) +} +``` + +Change to: + +```go + // ProposeRoleConfig lets a retro-role agent (internal/scheduler. + // StoryOrchestrator's Phase 8 retro stage) propose a new draft + // role_configs version for a role, after reflecting on a completed + // story's history (task tree, escalations, cost, evaluator/arbitration + // feedback). Implementations create a new "draft"-status role_configs + // row and return its version number — they never touch whatever version + // is currently "active" for that role; promoting a draft to active stays + // a human action via the existing POST /api/roles/{role}/activate. + ProposeRoleConfig(ctx context.Context, config role.RoleConfig) (version int, err error) + // ReportVerdict lets an evaluating agent (e.g. an arbitration-role task) + // report a structured approve/reject decision, instead of leaving it to + // be inferred by parsing the task's free-text summary later. + // Implementations record this as an event attached to the calling task's + // own ID; internal/scheduler.StoryOrchestrator's finalizeArbitration + // reads it back once the reporting task completes to decide REVIEW_READY + // vs NEEDS_FIX. A task that never calls this leaves no such event — + // callers reading it back must treat "no event found" as a distinct, + // backward-compatible case, not an error. + ReportVerdict(ctx context.Context, approved bool, reasoning string) error +} +``` + +- [ ] **Step 4: Implement `storeChannel.ReportVerdict`** + +In `internal/executor/channel.go`, add directly after the existing `ProposeRoleConfig` method (find it — it ends with the closing brace after `return row.Version, nil` / `}`): + +```go +func (c *storeChannel) ReportVerdict(_ context.Context, approved bool, reasoning string) error { + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: approved, Reasoning: reasoning}) + return c.store.CreateEvent(&event.Event{ + TaskID: c.taskID, + Kind: event.KindVerdictReported, + Actor: event.ActorAgent, + Payload: payload, + }) +} +``` + +- [ ] **Step 5: Run tests to verify they pass** + +Run: `go test ./internal/executor/ -run TestStoreChannel_ReportVerdict -v` +Expected: PASS, both tests. + +- [ ] **Step 6: Run the full package suite** + +Run: `go test ./internal/executor/... ./internal/agentchannel/...` +Expected: PASS for everything. + +- [ ] **Step 7: Commit** + +```bash +git add internal/agentchannel/agentchannel.go internal/executor/channel.go internal/executor/channel_test.go +git commit -m "feat(agentchannel,executor): add ReportVerdict for structured approve/reject signals" +``` + +--- + +## Task 5: Expose `report_verdict` as a tool (both transports) + +**Files:** +- Modify: `internal/agentloop/tools.go` (schema + case handler) +- Modify: `internal/executor/agentmcp.go` (input struct + MCP tool) + +**Interfaces:** +- Consumes: `AgentChannel.ReportVerdict` (Task 4). +- Produces: nothing new for later tasks — last mile connecting the primitive to what an agent can call, mirroring `propose_role_config`'s exposure exactly. + +- [ ] **Step 1: Add the tool definition to the native tool-use loop** + +In `internal/agentloop/tools.go`, add directly after the `propose_role_config` tool definition (find it — it's the last entry before the closing of the tool-spec slice, ending with the `escalation_ladder` property's closing braces): + +```go + { + Name: "report_verdict", + Description: "Report a structured approve/reject decision after evaluating another task's work (e.g. as an arbitration-role task). Call this before report_summary when your job is to decide whether the work is acceptable — this is read by the orchestrator to decide the outcome, not just logged for a human to read later.", + ParametersJSONSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "approved": map[string]any{"type": "boolean", "description": "true if the work meets its acceptance criteria and should proceed; false if it needs to be sent back for a fix"}, + "reasoning": strProp("a concise explanation of the decision"), + }, + "required": []string{"approved", "reasoning"}, + }, + }, +``` + +- [ ] **Step 2: Add the case handler** + +In the same file, add directly after the `propose_role_config` case (find `case "propose_role_config":` and its block, ending `return fmt.Sprintf("Proposed role config %s v%d (draft)", a.Role, version), false, nil`): + +```go + case "report_verdict": + var a struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + } + _ = json.Unmarshal([]byte(argsJSON), &a) + if rvErr := l.Channel.ReportVerdict(ctx, a.Approved, a.Reasoning); rvErr != nil { + return "", false, rvErr + } + return "Verdict recorded.", false, nil +``` + +- [ ] **Step 3: Add the MCP input struct** + +In `internal/executor/agentmcp.go`, add directly after `proposeRoleConfigInput`'s `toRoleConfig()` method (find where that method's closing brace is): + +```go +type reportVerdictInput struct { + Approved bool `json:"approved" jsonschema:"true if the work meets its acceptance criteria and should proceed; false if it needs to be sent back for a fix"` + Reasoning string `json:"reasoning" jsonschema:"a concise explanation of the decision"` +} +``` + +- [ ] **Step 4: Add the MCP tool registration** + +In the same file, add directly after the `propose_role_config` `mcp.AddTool` call (find it — ends with `return textResult(fmt.Sprintf("Proposed role config %s v%d (draft)", in.Role, version)), nil, nil\n\t})`): + +```go + mcp.AddTool(s, &mcp.Tool{ + Name: "report_verdict", + Description: "Report a structured approve/reject decision after evaluating another task's work (e.g. as an arbitration-role task). Call this before report_summary when your job is to decide whether the work is acceptable -- this is read by the orchestrator to decide the outcome, not just logged for a human to read later.", + }, func(ctx context.Context, _ *mcp.CallToolRequest, in reportVerdictInput) (*mcp.CallToolResult, any, error) { + if err := ch.ReportVerdict(ctx, in.Approved, in.Reasoning); err != nil { + return nil, nil, err + } + return textResult("Verdict recorded."), nil, nil + }) +``` + +- [ ] **Step 5: Build and run the full suite** + +Run: `go build ./... && go test ./internal/agentloop/... ./internal/executor/...` +Expected: PASS — no new tests in this task, same reasoning as Task 2 (underlying behavior already covered by Task 4's tests). + +- [ ] **Step 6: Commit** + +```bash +git add internal/agentloop/tools.go internal/executor/agentmcp.go +git commit -m "feat(agentloop,executor): expose report_verdict as a tool on both transports" +``` + +--- + +## Task 6: `finalizeArbitration` consults the structured verdict + +**Files:** +- Modify: `internal/scheduler/story_orchestrator.go:386-427` (`finalizeArbitration`) +- Test: `internal/scheduler/story_orchestrator_test.go` (append) + +**Interfaces:** +- Consumes: `event.KindVerdictReported` (Task 3), `StoryStore.ListEvents` (already exists in the `StoryStore` interface — see `internal/scheduler/story_orchestrator.go:113`). +- Produces: nothing new for later tasks — this is the final consumer in this plan. + +**Why this task is safe to do without reading all 1023 lines of the test file:** `finalizeArbitration` is a single, self-contained function with one existing test (`TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady`, read in full during this plan's research) that already exercises exactly the scenario being extended — the fake store's `eventsOfKind`/`setTaskSummary`/`seedStoryWithEvaluators` helpers this task's new tests need are the same ones that existing test already uses. This task adds new tests alongside it using the identical helpers; it does not need to touch or understand the other ~950 lines covering `ensureEvaluators`/`ensureArbitration`/retro, which are untouched by this change. + +- [ ] **Step 1: Write the failing tests** + +Append to `internal/scheduler/story_orchestrator_test.go`, directly after `TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady` (the test you read confirms `seedStoryWithEvaluators(t, task.StateCompleted)` returns `(store, st, evaluators)`, and that after `orch.Tick` spawns the arbitration task, `store.dependentsWithRole(evaluators[0].ID, "planner")` finds it, `store.setTaskState(arb.ID, task.StateCompleted)` completes it, and a second `orch.Tick` triggers `finalizeArbitration` — this task's new tests follow the identical shape, only adding a `ReportVerdict`-equivalent event before the second tick): + +```go +// TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix proves that +// when the arbitration task reports a structured approved=false verdict +// (via a KindVerdictReported event, the same one AgentChannel.ReportVerdict +// records), finalizeArbitration routes the story to NEEDS_FIX instead of +// unconditionally REVIEW_READY. +func TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix(t *testing.T) { + store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted) + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.Tick(context.Background()) // spawns arbitration + + arbitrations := store.dependentsWithRole(evaluators[0].ID, "planner") + if len(arbitrations) != 1 { + t.Fatalf("expected 1 arbitration task, got %d", len(arbitrations)) + } + arb := arbitrations[0] + + // Simulate the arbitration agent calling report_verdict with a rejection + // before finishing, the same event storeChannel.ReportVerdict records. + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: false, Reasoning: "breaks the running-log panel styling"}) + 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) + } + + store.setTaskState(arb.ID, task.StateCompleted) + store.setTaskSummary(arb.ID, "found a real problem") + + orch.Tick(context.Background()) // finalizeArbitration runs + + stories, _ := store.ListStories(storage.StoryFilter{}) + var got *story.Story + for _, s := range stories { + if s.ID == st.ID { + got = s + } + } + if got == nil { + t.Fatal("story not found") + } + if got.Status != "NEEDS_FIX" { + t.Errorf("story status: want NEEDS_FIX, got %q", got.Status) + } +} + +// TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady proves the +// mirror case: an explicit approved=true verdict still routes to +// REVIEW_READY, same as today's unconditional behavior -- this isn't just +// "absence of a verdict defaults to proceed", a real approval also proceeds. +func TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady(t *testing.T) { + store, st, evaluators := seedStoryWithEvaluators(t, task.StateCompleted) + + pool := &fakePool{} + orch := &StoryOrchestrator{Store: store, Pool: pool} + orch.Tick(context.Background()) + + arbitrations := store.dependentsWithRole(evaluators[0].ID, "planner") + arb := arbitrations[0] + + payload, _ := json.Marshal(struct { + Approved bool `json:"approved"` + Reasoning string `json:"reasoning"` + }{Approved: true, Reasoning: "meets all acceptance 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) + } + + store.setTaskState(arb.ID, task.StateCompleted) + store.setTaskSummary(arb.ID, "ship it") + + orch.Tick(context.Background()) + + stories, _ := store.ListStories(storage.StoryFilter{}) + var got *story.Story + for _, s := range stories { + if s.ID == st.ID { + got = s + } + } + if got == nil { + t.Fatal("story not found") + } + if got.Status != "REVIEW_READY" { + t.Errorf("story status: want REVIEW_READY, got %q", got.Status) + } +} +``` + +Note: `TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady` (the existing test, unmodified by this task) never seeds a `KindVerdictReported` event, so it exercises the "no verdict reported" backward-compatible path — after this task's change, it must still pass unchanged, proving the no-verdict-found case still defaults to `REVIEW_READY` exactly as before. + +- [ ] **Step 2: Run tests to verify the new ones fail** + +Run: `go test ./internal/scheduler/ -run TestStoryOrchestrator_Arbitration -v` +Expected: `TestStoryOrchestrator_ArbitrationRejects_SetsStoryNeedsFix` FAILS (story status is `REVIEW_READY`, not `NEEDS_FIX` — `finalizeArbitration` doesn't check for a verdict yet). `TestStoryOrchestrator_ArbitrationApproves_SetsReviewReady` and the pre-existing `TestStoryOrchestrator_ArbitrationCompletes_EmitsDecisionAndReviewReady` PASS already (current unconditional behavior happens to satisfy both). + +- [ ] **Step 3: Implement the verdict check** + +In `internal/scheduler/story_orchestrator.go`, find `finalizeArbitration`: + +```go +func (o *StoryOrchestrator) finalizeArbitration(st *story.Story, arbitration *task.Task) { + if st.Status != "VALIDATING" { + return + } + + payload, _ := json.Marshal(struct { + TaskID string `json:"task_id"` + Summary string `json:"summary"` + }{TaskID: arbitration.ID, Summary: arbitration.Summary}) + 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) + } + + st.Status = "REVIEW_READY" + if err := o.Store.UpdateStory(st); err != nil { + o.logf("story orchestrator: update story to REVIEW_READY", "storyID", st.ID, "error", err) + } +} +``` + +Replace with: + +```go +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) + } +} + +// arbitrationVerdict reads the arbitration task's own event stream for a +// KindVerdictReported event (AgentChannel.ReportVerdict) and returns its +// approved value. hasVerdict is false if no such event was ever recorded — +// callers must treat that as "no structured verdict was reported", not as +// an implicit rejection. +func (o *StoryOrchestrator) arbitrationVerdict(st *story.Story, arbitration *task.Task) (approved bool, hasVerdict bool) { + events, err := o.Store.ListEvents(arbitration.ID, 0) + if err != nil { + o.logf("story orchestrator: list arbitration task events", "storyID", st.ID, "taskID", arbitration.ID, "error", err) + return false, false + } + for _, e := range events { + if e.Kind != event.KindVerdictReported { + continue + } + var payload struct { + Approved bool `json:"approved"` + } + if json.Unmarshal(e.Payload, &payload) == nil { + approved = payload.Approved + hasVerdict = true + } + } + return approved, hasVerdict +} + +// optionalBool returns a pointer to v if present is true, nil otherwise — +// used so KindArbitrationDecided's payload omits "approved" entirely +// (rather than encoding a misleading false) when no structured verdict was +// ever reported. +func optionalBool(v bool, present bool) *bool { + if !present { + return nil + } + return &v +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `go test ./internal/scheduler/ -run TestStoryOrchestrator_Arbitration -v` +Expected: PASS, all three (the two new ones plus the pre-existing unmodified one). + +- [ ] **Step 5: Run the full package suite to check for regressions** + +Run: `go test ./internal/scheduler/...` +Expected: PASS for everything — this change only touches `finalizeArbitration` and adds two new helper functions; nothing else in the 1023-line test file exercises that function differently than before. + +- [ ] **Step 6: Commit** + +```bash +git add internal/scheduler/story_orchestrator.go internal/scheduler/story_orchestrator_test.go +git commit -m "feat(scheduler): finalizeArbitration consults a structured verdict instead of always routing to REVIEW_READY" +``` + +--- + +## Final Verification + +- [ ] Run `go build ./...` — passes. +- [ ] Run `go test ./...` — passes, full repo. +- [ ] Run `grep -rn "ReportVerdict\|report_verdict\|KindVerdictReported\|DependsOn.*SubtaskSpec\|depends_on.*spawn_subtask" internal/` to visually confirm every file in the File Map was actually touched — a missed file in a multi-file feature like this is easy to overlook. |