# 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.