summaryrefslogtreecommitdiff
path: root/docs/superpowers
diff options
context:
space:
mode:
Diffstat (limited to 'docs/superpowers')
-rw-r--r--docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md66
1 files changed, 66 insertions, 0 deletions
diff --git a/docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md b/docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md
new file mode 100644
index 0000000..7d34d57
--- /dev/null
+++ b/docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md
@@ -0,0 +1,66 @@
+# Recursive Arbitrated Review — Design Spec (supersedes part of the 2026-07-08 spec)
+
+**Date:** 2026-07-09
+**Status:** Approved
+**Supersedes:** the "Arbitrated review runs at every node" and "SubtaskSpec gains DependsOn" sections of `2026-07-08-recursive-story-decomposition-design.md`, and the fix-attempt mechanism built by `2026-07-09-story-fix-and-reevaluate-loop.md`, wherever they conflict with this document.
+
+---
+
+## Goal
+
+One recursive mechanism, applied identically at every depth of a story's task tree: every `builder`-role task — leaf or roll-up, root or nested subtask — gets the same decompose-or-implement judgment and the same arbitrated review (Builder → 4 Evaluators → Arbitration → approve/fix). No special case for the root. No separate mechanism for interior nodes.
+
+## Why the shipped story-level fix loop doesn't generalize
+
+The previous plan (`2026-07-09-story-fix-and-reevaluate-loop.md`) made rejection-and-retry work by mutating `story.RootTaskID` to point at a freshly spawned fix-attempt task. That works because `Story` is a distinct row with a field to mutate. It does not generalize: an arbitrary interior subtask has no equivalent — `task.Task` has no "current attempt" pointer, and inventing one (a new field, or cancel-and-ignore semantics in `maybeUnblockParent`) is exactly the kind of special-cased, level-specific machinery this design explicitly rejects.
+
+## The uniform mechanism: resolve "current," don't mutate a pointer
+
+Every position in the tree — the story's root included — is identified by an **anchor task ID**, set once, immutable. "The task currently representing that position" is *resolved*, not stored, by walking forward through the chain of fix-attempt `DependsOn` edges (the exact shape `fixAttemptDepth` already understands, walked forward instead of backward):
+
+```go
+// currentAttempt resolves the task that currently represents the logical
+// position anchored at anchorID: anchorID itself, or — if it was rejected
+// and a fix attempt exists — the fix attempt, walking forward through
+// however many rejections have occurred. A fix-attempt task is identified
+// structurally: a builder-role task whose sole DependsOn entry is the
+// previous attempt's ID (the same shape ensureFixAttempt already creates).
+// This is the one place "which task is current" is ever decided — every
+// other function (auto-accept, ensureEvaluators, maybeUnblockParent) calls
+// this first and operates on the result, never on a raw anchor ID directly.
+func (o *StoryOrchestrator) currentAttempt(anchorID string) (*task.Task, error)
+```
+
+This same function resolves the story's root (anchored at the story's own, now-**immutable** `RootTaskID`) and every subtask position (anchored at whatever task ID `spawn_subtask` first created for that position). One function, one rule, every depth. `story.RootTaskID` stops being written after story creation — the field the earlier plan added `ensureFixAttempt` writes to is removed from that write path; the anchor is set once and read through `currentAttempt` forever after.
+
+## What this replaces from the previous plan
+
+`ensureFixAttempt`'s re-pointing behavior (`st.RootTaskID = fix.ID; st.Status = "IN_PROGRESS"; o.Store.UpdateStory(st)`) is replaced by resolving through `currentAttempt` — the story's `RootTaskID` is never written again after creation. `maxFixAttempts`'s cap logic (walking backward through the same chain shape) is retained essentially unchanged — it already recognizes the chain; this design just also reads it forward.
+
+## Generalizing "done" detection: `maybeUnblockParent` resolves through `currentAttempt`
+
+`internal/executor.Pool.maybeUnblockParent` requires every subtask to be `COMPLETED`. Today a rejected-and-fixed subtask can never satisfy that, because the *rejected* task itself never reaches `COMPLETED` again — only its fix-attempt does, and the fix-attempt is not what `ListSubtasks(parentID)` was originally tracking as "the" child at that position (it's a `DependsOn`-linked descendant of it, potentially several levels of rejection deep). `maybeUnblockParent` must resolve each of `ListSubtasks(parentID)`'s entries through `currentAttempt` before checking its state — so a subtask that was rejected and fixed still satisfies the gate once its *current* attempt reaches `COMPLETED`, without touching state, cancellation, or any new field on the original rejected task at all. A subtask with no rejection history resolves to itself — zero behavior change for the entire existing test suite and every non-recursive-review subtask tree in the system.
+
+## Generalizing arbitrated review's trigger
+
+Today `StoryOrchestrator.processStory` only ever evaluates `story.RootTaskID` (now: `currentAttempt(story.RootTaskID)`). This design extends it to recurse: once a `builder`-role task's own work is done (leaf `READY`, or roll-up `READY` via the generalized `maybeUnblockParent`), it is auto-accepted and put through `ensureEvaluators`/`ensureArbitration` **exactly like the story root is today** — the same two functions, called against any qualifying builder task, not hardcoded to `story.RootTaskID`. A tree walk (reusing the existing `taskTree`/`GET .../task-tree` BFS shape, following both `ParentTaskID` children and `DependsOn` edges) finds every `builder`-role node in a story's tree once its immediate prerequisites are satisfied, and drives each one through the identical Builder→Evaluators→Arbitration→approve-or-fix cycle. Rejection at any depth spawns a fix attempt anchored at that node (`ParentTaskID` = the rejected node's own `ParentTaskID`, `DependsOn = [rejectedID]`) — resolved forward by the same `currentAttempt`, at every level, including the root.
+
+## `Task.AcceptanceCriteria` does not exist yet — this design adds it
+
+The 2026-07-08 spec claimed this field already existed; it does not. `task.Task` gains `AcceptanceCriteria []string`, storage gains an `acceptance_criteria_json` column (mirroring `story.Story`'s existing pattern exactly), and `spawn_subtask` gains an `acceptance_criteria` parameter (mirroring how `depends_on` was exposed) so a decomposing builder can state what each subtask must satisfy. A node with no `AcceptanceCriteria` set (the root, or a subtask whose decomposing parent didn't specify any) is evaluated against the story's own `AcceptanceCriteria` as a fallback — never evaluated against nothing.
+
+## The `builder` role's system prompt still needs to be written
+
+Unchanged from the original spec: none of this mechanism does anything unless the `builder` role's `RoleConfig.SystemPrompt` actually instructs the decompose-or-implement judgment and tells the agent to use `spawn_subtask`'s `depends_on`/`acceptance_criteria` when it decomposes. This is a real deliverable of implementing this design, not a side effect of the code changes above.
+
+## Non-Goals (unchanged from the original spec)
+
+No depth or cost bound — mitigated later by cheap-model tiering, still deferred. No resumable-orchestrator design. `Epic` untouched.
+
+## Implementation order (this design will be delivered as a sequence of plans, smallest/safest first)
+
+1. `Task.AcceptanceCriteria` field + storage column + `spawn_subtask` parameter — additive, no behavior change to anything existing.
+2. `currentAttempt` + generalize `maybeUnblockParent` to resolve through it — the riskiest single piece, since `maybeUnblockParent` is core machinery used by every subtask tree in the system, not just recursively-reviewed ones. Must ship with tests proving zero behavior change for chains of length zero (the overwhelming majority of existing subtask trees).
+3. Migrate the story-level fix loop from `RootTaskID` re-pointing to `currentAttempt` resolution, removing the write path the previous plan added.
+4. Generalize `ensureEvaluators`/`ensureArbitration`/the fix-loop trigger to recurse into `builder`-role subtask trees, not just the story root.
+5. Write the `builder` role's system prompt.