diff options
Diffstat (limited to 'docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md')
| -rw-r--r-- | docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md | 331 |
1 files changed, 331 insertions, 0 deletions
diff --git a/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md b/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md new file mode 100644 index 0000000..53537e8 --- /dev/null +++ b/docs/superpowers/plans/2026-07-10-nested-builder-completion-follows-arbitration.md @@ -0,0 +1,331 @@ +# Nested Builder Completion Follows Arbitration Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** A nested (`ParentTaskID != ""`) task with `Agent.Role == "builder"` never reaches `COMPLETED` via the executor's own default completion path — it goes to `READY` instead, exactly mirroring the rule piece 4a already applied to the story root. Every other task (no role, or a non-`builder` role) keeps today's exact behavior. + +**Architecture:** `internal/executor.Pool` currently has two places that write `task.StateCompleted` for a nested task: `handleRunResult`'s "no more subtasks, has a parent" branch, and `maybeUnblockParent`'s own "all this parent's children are done, promote it too" branch (since a nested roll-up parent, once unblocked, is itself promoted straight to `COMPLETED`, cascading further up). Both get consolidated into one new helper, `promoteNestedTask`, which is the single place that decides READY-vs-COMPLETED for a nested task and owns triggering the parent-unblock cascade (only in the COMPLETED case). Executor stays completely story-agnostic: it has no idea what a "story" or "arbitrated review" is, it just treats the string `"builder"` in `Agent.Role` as meaning "this task's completion requires external approval before it can be trusted" — the same rule `internal/scheduler.StoryOrchestrator` already applies to a story's root task. + +**Tech Stack:** Go, `internal/executor` package only. No changes to `internal/scheduler`, `internal/task`, or any other package in this plan — that's piece 4b-2/4b-3, deliberately deferred (this plan does not export anything new; nothing outside `internal/executor` calls this new behavior yet). + +## Global Constraints + +- This is piece 4b-1 of `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`'s piece 4, split out as the smaller, safer first step of piece 4b (which itself was split out of piece 4 during grounding work, once it became clear piece 4 requires touching `executor.Pool`'s core nested-task completion path, not just `internal/scheduler`). +- Root cause this plan closes: `internal/executor.Pool.handleRunResult` currently promotes ANY nested task straight to `COMPLETED` the instant its own execution finishes with no further children — including a `builder`-role subtask spawned via `spawn_subtask`, with zero review of any kind. `internal/scheduler.StoryOrchestrator` never even sees nested subtasks (it only watches `story.RootTaskID` and its top-level DAG-sibling dependents), so today a builder-role subtask's "COMPLETED" carries no more meaning than any other task's — exactly the gap the story root had before piece 4a, one level lower in the stack where the fix must actually live. +- The `Agent.Role == "builder"` check is a plain string comparison against the existing `evaluatorRoles`/`arbitrationRole`/`retroRole` constants pattern already used in `internal/scheduler/story_orchestrator.go` (this plan does not add a new shared constant across packages — `internal/executor` importing `internal/scheduler` would be a layering violation the wrong direction; a literal `"builder"` string here is the same tolerance the codebase already has for `"builder"` appearing as a literal in `ensureFixAttempt`'s `d.Agent.Role == "builder"` check). +- No change to `depDoneStates`, `checkDepsReady`, `cascadeFail`, or anything about how a task's own subtasks are counted/waited-on — only the READY-vs-COMPLETED choice for the task itself, once its own children (if any) are done. +- A `builder`-role task that no story's tree ever adopts will sit at `READY` forever, with its parent stuck `BLOCKED` forever. This is accepted, not a regression to guard against here — the same category of graceful degradation the codebase already tolerates for a role with no active `role_configs` row (`internal/executor.Pool.execute()` logs a warning and dispatches without role resolution rather than failing). + +--- + +### Task 1: Consolidate nested-task promotion into `promoteNestedTask`, redirecting builder-role tasks to READY + +**Files:** +- Modify: `internal/executor/executor.go` (`handleRunResult`, `maybeUnblockParent`; add `promoteNestedTask`) +- Test: `internal/executor/executor_test.go` + +**Interfaces:** +- Consumes: `task.StateReady`/`task.StateCompleted` (unchanged), `p.store.UpdateTaskState` (unchanged), `p.maybeUnblockParent` (unchanged signature, now called only from inside `promoteNestedTask`). +- Produces: `func (p *Pool) promoteNestedTask(t *task.Task) task.State` — new, unexported (no cross-package caller in this plan). Callers pass a task known to have `t.ParentTaskID != ""` and no further subtasks of its own; the function decides READY (builder-role) or COMPLETED (everything else, triggering the cascade) and returns whichever it wrote. + +- [ ] **Step 1: Add `promoteNestedTask`** + +In `internal/executor/executor.go`, find `maybeUnblockParent`'s doc comment and signature (the function immediately preceding it in the file, `RecoverStaleBlocked`'s neighbor further down — search for `func (p *Pool) maybeUnblockParent(parentID string) {` to locate it), and insert this new function immediately **before** it: + +```go +// promoteNestedTask transitions a nested task (t.ParentTaskID != "", with no +// further subtasks of its own) to the state its own completion should +// reach, and returns that state. A builder-role nested task goes to READY, +// not COMPLETED: builder-role tasks -- roll-up or leaf, root or nested -- +// require arbitrated review before their completion can be trusted (see +// docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md). +// executor stays story-agnostic here -- it has no idea what a "story" is, +// it just treats "builder" as always requiring external approval before +// COMPLETED, uniformly, the same rule internal/scheduler.StoryOrchestrator +// already applies to a story's root task (see that package's +// finalizeArbitration). A builder-role task nobody's story tree ever +// adopts simply sits at READY (and its parent BLOCKED) forever -- the same +// category of graceful degradation already tolerated elsewhere in this +// codebase for a role with no active role_configs row. +// +// Every other task transitions to COMPLETED exactly as before this change, +// and this is the one place that triggers the parent-unblock cascade on +// t.ParentTaskID -- callers must NOT also call maybeUnblockParent +// themselves; this method owns that decision entirely. +func (p *Pool) promoteNestedTask(t *task.Task) task.State { + if t.Agent.Role == "builder" { + if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil { + p.logger.Error("promoteNestedTask: update task state", "taskID", t.ID, "error", err) + } + return task.StateReady + } + if err := p.store.UpdateTaskState(t.ID, task.StateCompleted); err != nil { + p.logger.Error("promoteNestedTask: update task state", "taskID", t.ID, "error", err) + return t.State + } + p.maybeUnblockParent(t.ParentTaskID) + return task.StateCompleted +} + +``` + +- [ ] **Step 2: Update `handleRunResult`'s nested-completion branch** + +In the same file, find (inside `handleRunResult`): + +```go + if subErr == nil && len(subtasks) > 0 { + exec.Status = "BLOCKED" + if err := p.store.UpdateTaskState(t.ID, task.StateBlocked); err != nil { + p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateBlocked, "error", err) + } + } else if t.ParentTaskID == "" { + exec.Status = "READY" + if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil { + p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateReady, "error", err) + } + } else { + exec.Status = "COMPLETED" + if err := p.store.UpdateTaskState(t.ID, task.StateCompleted); err != nil { + p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateCompleted, "error", err) + } + p.maybeUnblockParent(t.ParentTaskID) + } +``` + +Replace with: + +```go + if subErr == nil && len(subtasks) > 0 { + exec.Status = "BLOCKED" + if err := p.store.UpdateTaskState(t.ID, task.StateBlocked); err != nil { + p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateBlocked, "error", err) + } + } else if t.ParentTaskID == "" { + exec.Status = "READY" + if err := p.store.UpdateTaskState(t.ID, task.StateReady); err != nil { + p.logger.Error("failed to update task state", "taskID", t.ID, "state", task.StateReady, "error", err) + } + } else if p.promoteNestedTask(t) == task.StateReady { + exec.Status = "READY" + } else { + exec.Status = "COMPLETED" + } +``` + +- [ ] **Step 3: Update `maybeUnblockParent`'s own nested-promotion branch** + +In the same file, find (inside `maybeUnblockParent`): + +```go + if parent.State == task.StateBlocked && parent.ParentTaskID != "" { + if err := p.store.UpdateTaskState(parentID, task.StateCompleted); err != nil { + p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err) + return + } + p.maybeUnblockParent(parent.ParentTaskID) + return + } + if err := p.store.UpdateTaskState(parentID, task.StateReady); err != nil { + p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err) + } +} +``` + +Replace with: + +```go + if parent.State == task.StateBlocked && parent.ParentTaskID != "" { + p.promoteNestedTask(parent) + return + } + if err := p.store.UpdateTaskState(parentID, task.StateReady); err != nil { + p.logger.Error("maybeUnblockParent: update parent state", "parentID", parentID, "error", err) + } +} +``` + +- [ ] **Step 4: Add a test proving a builder-role leaf subtask goes to READY, not COMPLETED** + +In `internal/executor/executor_test.go`, find `TestPool_Submit_Subtask_GoesToCompleted` (it starts with `func TestPool_Submit_Subtask_GoesToCompleted(t *testing.T) {`) and immediately after its closing brace, add: + +```go +// TestPool_Submit_BuilderRoleSubtask_GoesToReady_NotCompleted proves the +// core piece-4b mechanism: a nested builder-role task, once its own +// execution finishes with no further children, goes to READY -- not +// COMPLETED -- mirroring the story root's own rule (piece 4a): a +// builder-role task's completion must mean "verified by arbitration", not +// merely "the agent finished", uniformly regardless of depth. +func TestPool_Submit_BuilderRoleSubtask_GoesToReady_NotCompleted(t *testing.T) { + store := testStore(t) + runner := &mockRunner{} + runners := map[string]Runner{"claude": runner} + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, runners, store, logger) + + tk := makeTask("builder-sub-1") + tk.ParentTaskID = "parent-99" // subtask + tk.Agent.Role = "builder" + store.CreateTask(tk) + + if err := pool.Submit(context.Background(), tk); err != nil { + t.Fatalf("submit: %v", err) + } + + result := <-pool.Results() + if result.Err != nil { + t.Errorf("expected no error, got: %v", result.Err) + } + if result.Execution.Status != "READY" { + t.Errorf("status: want READY, got %q", result.Execution.Status) + } + + got, _ := store.GetTask("builder-sub-1") + if got.State != task.StateReady { + t.Errorf("task state: want READY (builder-role, awaiting arbitrated review), got %v", got.State) + } +} +``` + +- [ ] **Step 5: Add a test proving the cascade stops at a builder-role roll-up** + +In the same file, find `TestPool_Submit_GrandchildCompletion_CascadesThroughNestedParents` (its full body, ending at its closing `}`) and immediately after it, add: + +```go +// TestPool_MaybeUnblockParent_BuilderRoleRollup_StaysReady_NotCascaded +// proves the cascade-side of the same rule: when maybeUnblockParent's own +// "all children done, promote this BLOCKED nested parent" branch fires for +// a builder-role roll-up, it too goes to READY (not COMPLETED) -- and, +// because it never reaches COMPLETED, the cascade must NOT continue past it +// to the grandparent (root stays BLOCKED, not READY), since the roll-up's +// own arbitrated review hasn't happened yet. +func TestPool_MaybeUnblockParent_BuilderRoleRollup_StaysReady_NotCascaded(t *testing.T) { + store := testStore(t) + runner := &mockRunner{} + runners := map[string]Runner{"claude": runner} + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, runners, store, logger) + + root := makeTask("builder-nested-root") + root.State = task.StateBlocked // already ran, delegated to middle + store.CreateTask(root) + + middle := makeTask("builder-nested-middle") + middle.ParentTaskID = root.ID + middle.Agent.Role = "builder" + middle.State = task.StateBlocked // already ran, delegated to grandchild + store.CreateTask(middle) + + grandchild := makeTask("builder-nested-grandchild") + grandchild.ParentTaskID = middle.ID + store.CreateTask(grandchild) // fresh, about to be submitted + + if err := pool.Submit(context.Background(), grandchild); err != nil { + t.Fatalf("submit: %v", err) + } + + result := <-pool.Results() + if result.Err != nil { + t.Errorf("expected no error, got: %v", result.Err) + } + if result.Execution.Status != "COMPLETED" { + t.Errorf("grandchild status: want COMPLETED, got %q", result.Execution.Status) + } + + gotMiddle, err := store.GetTask(middle.ID) + if err != nil { + t.Fatalf("get middle: %v", err) + } + if gotMiddle.State != task.StateReady { + t.Errorf("middle state: want READY (builder-role roll-up, awaiting arbitrated review), got %v", gotMiddle.State) + } + + gotRoot, err := store.GetTask(root.ID) + if err != nil { + t.Fatalf("get root: %v", err) + } + if gotRoot.State != task.StateBlocked { + t.Errorf("root state: want unchanged BLOCKED (middle hasn't been arbitrated yet, cascade must not continue), got %v", gotRoot.State) + } +} +``` + +- [ ] **Step 6: Add a `RecoverStaleBlocked` test for the same builder-role roll-up case** + +In the same file, find `TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent` (its full body, ending at its closing `}`) and immediately after it, add: + +```go +// TestPool_RecoverStaleBlocked_BuilderRoleRollup_StaysReady proves +// RecoverStaleBlocked (the startup-recovery sweep, which calls +// maybeUnblockParent for every BLOCKED/QUEUED task) applies the same +// builder-role READY-not-COMPLETED rule as the live dispatch path -- a +// server restart must not silently promote a builder-role roll-up straight +// to COMPLETED just because all its children happen to be done by the time +// recovery runs. +func TestPool_RecoverStaleBlocked_BuilderRoleRollup_StaysReady(t *testing.T) { + store := testStore(t) + logger := slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})) + pool := NewPool(2, map[string]Runner{"claude": &mockRunner{}}, store, logger) + + root := makeTask("recover-builder-root") + root.State = task.StateBlocked + store.CreateTask(root) + + middle := makeTask("recover-builder-middle") + middle.ParentTaskID = root.ID + middle.Agent.Role = "builder" + middle.State = task.StateBlocked + store.CreateTask(middle) + + grandchild := makeTask("recover-builder-grandchild") + grandchild.ParentTaskID = middle.ID + grandchild.State = task.StateCompleted + store.CreateTask(grandchild) + + pool.RecoverStaleBlocked() + + gotMiddle, err := store.GetTask(middle.ID) + if err != nil { + t.Fatalf("get middle: %v", err) + } + if gotMiddle.State != task.StateReady { + t.Errorf("middle state: want READY (builder-role, awaiting arbitrated review), got %v", gotMiddle.State) + } + + gotRoot, err := store.GetTask(root.ID) + if err != nil { + t.Fatalf("get root: %v", err) + } + if gotRoot.State != task.StateBlocked { + t.Errorf("root state: want unchanged BLOCKED, got %v", gotRoot.State) + } +} +``` + +- [ ] **Step 7: Run the full executor package test suite** + +Run: `go test ./internal/executor/... -v -run 'TestPool_(Submit|MaybeUnblockParent|RecoverStaleBlocked)'` +Expected: PASS for every test, including the 3 new ones above and every pre-existing test in this group — in particular `TestPool_Submit_Subtask_GoesToCompleted`, `TestPool_Submit_GrandchildCompletion_CascadesThroughNestedParents`, `TestPool_RecoverStaleBlocked_PromotesNestedSubtaskParent`, and `TestPool_MaybeUnblockParent_ResolvesSubtaskThroughCurrentAttempt` must all still pass completely unchanged (none of their fixtures set `Agent.Role`, so `promoteNestedTask`'s builder-role branch never fires for them — confirm this by reading `makeTask`'s definition and confirming it leaves `Agent.Role` at its zero value). Paste the actual output. + +- [ ] **Step 8: Run the full repo test suite** + +Run: `go build ./...` then `go test ./...` (the entire repo). Both must pass — paste the actual output. If `internal/api` flakes under the race detector on a one-off "sql: database is closed" teardown error unrelated to this task's actual changes, rerun once before treating it as real. + +Also run `gofmt -l internal/executor/executor.go internal/executor/executor_test.go` and `gofmt -w` any file it flags that wasn't already gofmt-dirty before your changes (check with `git show HEAD~1:<path> | gofmt -l -` if unsure). + +- [ ] **Step 9: Commit locally NOW (before further verification, if running under time pressure)** + +Run these exact commands, in order: + +```bash +git add -A +git commit -m "refactor(executor): nested builder-role tasks go READY not COMPLETED, requiring external arbitrated review" +git status +git log --oneline -1 +``` + +**Do not run `git push`.** This repo's own bare git remote is not mounted inside dispatch containers. Per this repo's own CLAUDE.md ("After a successful run, new commits are pushed from the workspace"), claudomator's ContainerRunner pushes your commits externally once your run finishes successfully with a clean working tree — your only job is a clean local commit and a clean working tree at teardown. + +## Mandatory verification disclosure + +When you call report_summary, paste the actual terminal output of every command above — literal pass/fail counts, not a claim of success — including the full-repo `go test ./...` run and the final `git status`/`git log` confirmation showing a clean tree with your commit present. If anything here is ambiguous or conflicts with what you find in the actual repo, call ask_user and describe the specific conflict — don't guess or silently decide. |