diff options
| author | Peter Stone <thepeterstone@gmail.com> | 2026-07-09 02:49:39 +0000 |
|---|---|---|
| committer | Peter Stone <thepeterstone@gmail.com> | 2026-07-09 02:49:39 +0000 |
| commit | e152afba1e08eb6ce35f2e3e50837dda3ca51630 (patch) | |
| tree | 37c897886908c723b96950151addfb134e3f9885 /docs/superpowers/specs | |
| parent | ef22ba51d426690cb500aad6fdce0db92bba7f72 (diff) | |
docs: add design spec for recursive arbitrated review (supersedes root-only fix loop)
Replaces the story.RootTaskID re-pointing mechanism shipped in
2026-07-09-story-fix-and-reevaluate-loop.md with a uniform
currentAttempt() resolution that works identically at every depth of
a story's task tree, not just the root. Also corrects the 2026-07-08
spec's incorrect claim that Task.AcceptanceCriteria already exists.
Diffstat (limited to 'docs/superpowers/specs')
| -rw-r--r-- | docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md | 66 |
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. |
