diff options
Diffstat (limited to 'CLAUDE.md')
| -rw-r--r-- | CLAUDE.md | 216 |
1 files changed, 110 insertions, 106 deletions
@@ -364,110 +364,125 @@ are still out of scope — see that type's non-goals. `/api/projects`/`/api/tasks` (only chatbot MCP, agent MCP, and WebSocket require it). -#### Story orchestrator (Phase 7b) +#### Story orchestrator (Phase 7b, generalized into a recursive arbitrated-review engine by Phases superseding this doc's original Phase 7b text — see `docs/superpowers/specs/2026-07-09-recursive-arbitrated-review-design.md`) `internal/scheduler.StoryOrchestrator` (same file group as `Scheduler`, see `internal/scheduler/story_orchestrator.go`) is a poll-based watcher (`Tick`/`Run`, `DefaultStoryPollInterval` = 15s), not a hook into -`executor.Pool.handleRunResult`. This is a deliberate choice, not the default: -every task the orchestrator reacts to (the story's Builder/root task, the 4 -Evaluators, the Arbitration task) is a **top-level** task -(`parent_task_id == ""`), and per the task state machine above, the *only* -way one of them would ever reach `COMPLETED` on its own is via a -human/chatbot `POST /api/tasks/{id}/accept` (`READY → COMPLETED`, in -`internal/api`'s `acceptTask`) — `handleRunResult` only ever lands a -top-level task at `READY` or `BLOCKED`, never `COMPLETED` directly. Rather -than requiring a human to make that call 6 times per story (builder + 4 -evaluators + arbitration), **the orchestrator auto-accepts these specific -tasks itself** (`autoAccept`, see below) — so the *only* manual accept -anywhere in this chain is the final story-level -`POST /api/stories/{id}/accept`. A `handleRunResult` hook would never observe -either transition (the human-driven one or this orchestrator's own -auto-accept) cleanly, since both happen outside `executor`; polling -sidesteps the question of *how*/*by whom* a task reached `COMPLETED` -entirely — the same way `Scheduler` doesn't care how a task got to `FAILED`. - -Stage-by-stage: - -1. **Builder → Evaluators**: once a story's `root_task_id` task is `READY`, - the orchestrator auto-accepts it to `COMPLETED` itself (see "Auto-accept" - below); once it's `COMPLETED`, spawns 4 new top-level tasks - (`Store.CreateTask`, not `SpawnSubtask` — no `parent_task_id`, - `depends_on: [root_task_id]`) with `agent.role` = - `evaluator_quality`/`evaluator_security`/`evaluator_correctness`/ - `evaluator_performance`, queues and submits each to the pool, and sets the - story's `status` to `VALIDATING`. Idempotency is **structural**, not a - marker: it inspects `Store.ListDependents(root_task_id)` for existing - tasks whose role already matches one of the four, and only creates the - missing ones — safe to call every tick forever, and safe across a process - restart (unlike an in-memory guard would be). -2. **Per-evaluator verdicts**: each Evaluator task is likewise auto-accepted - from `READY` to `COMPLETED`. As each is observed `COMPLETED`, emits +`executor.Pool.handleRunResult`. Every task the orchestrator reacts to (the +story's Builder/root task, any nested `builder`-role subtask, the 4 +Evaluators, the Arbitration task) reaches whatever state change matters to it +outside `executor` — either via `internal/executor.Pool.promoteNestedTask` +landing a nested builder subtask at `READY` (not `COMPLETED`) rather than +`handleRunResult`, or via this orchestrator's own writes — so polling +sidesteps the question of *how*/*by whom* a task reached a given state +entirely, the same way `Scheduler` doesn't care how a task got to `FAILED`. + +**The core rule, applied identically at every depth of a story's task tree — +root or nested, no special case:** a `builder`-role task's own `READY` does +**not** mean done. It means "awaiting arbitration." `processStory` walks the +*entire* tree rooted at the story's current root position (`taskTree`, a BFS +over both `parent_task_id` children and `depends_on` edges) every tick, +finds every currently-`READY` `builder`-role node at any depth, and drives +each one through `processBuilderNode`: Evaluators → Arbitration → +approve-or-reject. A node is promoted `READY → COMPLETED` **only** by +`finalizeArbitration`'s approval branch — never by an eager auto-accept. This +walk is unconditional on the root's own state (except a `root == COMPLETED` +short-circuit): a nested subtask can be sitting `READY`, awaiting its own +arbitration, while its parent is still `BLOCKED` — and `maybeUnblockParent` +(see "Generalizing done detection" below) will not promote that parent out +of `BLOCKED` until the nested position's *current attempt* reaches +`COMPLETED`. Stopping the walk at a not-yet-`READY` root would deadlock any +story with nested subtasks. + +Per-node stages (`processBuilderNode`, called once per qualifying node per tick): + +1. **node → Evaluators**: spawns 4 new top-level tasks (`Store.CreateTask`, + not `SpawnSubtask` — no `parent_task_id`, `depends_on: [node.id]`) with + `agent.role` = `evaluator_quality`/`evaluator_security`/ + `evaluator_correctness`/`evaluator_performance`, using `node`'s own + `acceptance_criteria` (falling back to the story's if unset). Idempotency + is **structural**: it inspects `Store.ListDependents(node.id)` for + existing tasks whose role already matches one of the four, and only + creates the missing ones. `story.status` is only ever set to `VALIDATING` + here when `node` **is** the story's actual root (`node.parent_task_id == + ""`) — a nested node's own evaluators spawning has no claim on + story-level bookkeeping. +2. **Per-evaluator verdicts**: each Evaluator task is auto-accepted from + `READY` to `COMPLETED` (evaluators, unlike builder-role nodes, *are* + trivially auto-accepted — there's nothing further to arbitrate about an + evaluator's own completion). As each is observed `COMPLETED`, emits `event.KindEvalVerdict` — payload `{task_id, role, summary}` — attached to - the **story's** ID (not the evaluator task's ID), via - `GET /api/stories/{id}/events` (new in this phase, mirrors - `GET /api/tasks/{id}/events` but resolves existence via `GetStory`). - Attaching to the story ID means one call surfaces every verdict for a - story; `events.task_id` has no enforced FK, which is exactly the tolerance - 7a's `event.Kind` doc comments anticipated. De-duplication here is an - in-memory, per-process "already emitted" set keyed by evaluator task ID - (mirrors `Scheduler.handled` exactly) — the only place in this type that - isn't structurally idempotent, because there's no persisted marker for - "this specific verdict was already emitted"; a restart can produce at most - one duplicate `eval_verdict` event per evaluator, never more. -3. **Evaluators → Arbitration**: once all 4 Evaluators for a story are + the **story's** ID via `GET /api/stories/{id}/events`. De-duplication is + an in-memory, per-process "already emitted" set keyed by evaluator task ID + (mirrors `Scheduler.handled`) — a restart can produce at most one + duplicate `eval_verdict` event per evaluator, never more. +3. **Evaluators → Arbitration**: once all 4 Evaluators for `node` are `COMPLETED`, spawns one more top-level task, `agent.role: "planner"`, - `depends_on` = all 4 evaluator task IDs. Idempotency is again structural: - looks for an existing `planner`-role dependent of the first evaluator task - whose `depends_on` already contains all 4 IDs, rather than trusting - `story.status` (a human can freely rewrite it via `PUT /api/stories/{id}`). -4. **Arbitration → REVIEW_READY**: the Arbitration task is likewise - auto-accepted from `READY` to `COMPLETED`. When it reaches `COMPLETED`, - emits `event.KindArbitrationDecided` (payload `{task_id, summary}`, - attached to the story ID) and sets `status` to `REVIEW_READY`. **Gated on - `status == "VALIDATING"`** — the one place this type's idempotency check - is the story's own status field rather than a structural "does a - downstream task exist" check, because the Arbitration task is the last - task in the chain, so there's no further task existence to check against. - **Documented simplification:** this does **not** parse the Arbitration - task's summary for an approve/reject verdict — it always routes to - `REVIEW_READY`. A human or chatbot who reads the summary and disagrees - sets `NEEDS_FIX` manually via `PUT /api/stories/{id}`. A later phase could - close this gap with a dedicated verdict-reporting tool for the Arbitration - task (a new `AgentChannel` method) instead of parsing free text. + `depends_on` = all 4 evaluator task IDs, using `node`'s own acceptance + criteria. Idempotency is again structural: looks for an existing + `planner`-role dependent of the first evaluator task whose `depends_on` + already contains all 4 IDs. +4. **Arbitration → approve or reject**: the Arbitration task is likewise + auto-accepted from `READY` to `COMPLETED`. `finalizeArbitration` reads the + Arbitration task's own event stream for a structured `KindVerdictReported` + event (`AgentChannel.ReportVerdict`, the `report_verdict` tool) and + branches on it — an arbitration task that never calls `report_verdict` + defaults to approval. Idempotency is keyed to the specific arbitration + task (a `KindArbitrationDecided` event on the *story's* stream already + referencing `arbitration.id`), not a story-status field — this is what + lets the same function run for every node in the tree. + - **Approved**: `node` is promoted `READY → COMPLETED`. If `node` is the + root, `story.status` also moves to `REVIEW_READY`. + - **Rejected, root**: `story.status` moves to `NEEDS_FIX`; the next tick's + `ensureFixAttempt` spawns a new top-level `builder`-role fix-attempt + depending on the rejected root and resets `story.status` to + `IN_PROGRESS` (capped at `maxFixAttempts = 3` consecutive attempts via + `fixAttemptDepth`, walking the `depends_on` chain backward). `node` + itself is left at `READY` forever, superseded. + - **Rejected, nested**: no story-level field to poll, so + `spawnNestedFixAttempt` spawns the fix-attempt **directly**, right here + — same role, same `parent_task_id` as the rejected node, capped by the + same `maxFixAttempts`. Same semantic outcome as the root path, different + plumbing suited to where a story-level anchor exists (root) or doesn't + (nested) — not a special case in the review mechanism itself. + - Either way, `story.RootTaskID` is **never mutated** after story + creation — it's an immutable anchor. `task.CurrentAttempt(store, + anchorID)` (in `internal/task/currentattempt.go`) resolves "the task + currently representing this position" by walking forward through + however many fix-attempt `depends_on` links exist (a fix-attempt is + identified structurally: same `parent_task_id`, same `agent.role`, sole + `depends_on` entry pointing at the task it supersedes). Every caller + that needs "the current root" — `processStory`, `ensureFixAttempt`, + `processRetro` — resolves through this first. 5. **Human accept-gate (the only one)**: `POST /api/stories/{id}/accept` - (new in this phase) mirrors `handleAcceptTask`'s pattern: only valid from - `REVIEW_READY` (409 otherwise), transitions to `DONE`, emits - `event.KindHumanAccepted` (payload `{story_id, from, to}`, attached to the - story ID). Because of auto-accept (below), this is the *only* - story-status transition — indeed the only task/story state transition at - all — in this whole chain that requires a human/chatbot to act. - -**Auto-accept** (`StoryOrchestrator.autoAccept`): on every tick, for the -story's root/Builder task and each structurally-discovered -Evaluator/Arbitration dependent (i.e. only tasks the orchestrator has already -established are part of *this* story's pipeline — never a blanket "auto- -accept every READY task" sweep), if that task is `READY`, the orchestrator -transitions it straight to `COMPLETED` itself, using the exact same -state-machine-respecting write `internal/api`'s `acceptTask` uses -(`Store.UpdateTaskState`, which wraps `storage.DB.UpdateTaskStateBy` — -validates `task.ValidTransition` and writes the `state_change` event -atomically, not a raw/unchecked write). This is what makes the story-level -accept-gate the system's only required human/chatbot touchpoint: without it, -a human would have to separately `POST /api/tasks/{id}/accept` the Builder, -each of the 4 Evaluators, and the Arbitration task, since none of those -top-level tasks can reach `COMPLETED` any other way. `FAILED`/`TIMED_OUT`/ -`CANCELLED`/`BUDGET_EXCEEDED` outcomes for these same tasks are untouched by -this — they still go through `Scheduler`'s existing retry-then-escalate path -exactly as before; auto-accept only ever fires on the `READY` (success) -path. + mirrors `handleAcceptTask`'s pattern: only valid from `REVIEW_READY` (409 + otherwise), transitions to `DONE`, emits `event.KindHumanAccepted` + (payload `{story_id, from, to}`, attached to the story ID). This remains + the *only* story-status transition anywhere in this whole chain that + requires a human/chatbot to act. + +**Generalizing "done" detection** (`internal/executor.Pool.maybeUnblockParent`): +requires every one of a `BLOCKED` parent's subtasks to resolve (via +`task.CurrentAttempt`) to `COMPLETED` before promoting the parent to `READY` +— a nested `builder`-role subtask that was rejected-and-fixed still +satisfies this once its *current* attempt reaches `COMPLETED`, without +touching the originally-rejected task's own state at all. A subtask with no +rejection history resolves to itself. `evaluator_quality`/`evaluator_security`/`evaluator_correctness`/ -`evaluator_performance`/`planner`/`builder` `role_configs` rows are assumed to -be seeded separately (this phase doesn't create them); a role-typed task -spawned here with no matching active `role_configs` row dispatches in the -same degraded (no role resolution) mode `Pool.execute()` already logs a -warning and falls back to for any other role-typed task. +`evaluator_performance`/`planner` `role_configs` rows are assumed to be +seeded separately (nothing in this codebase creates them) — a role-typed +task spawned here with no matching active `role_configs` row dispatches in +the same degraded (no role resolution) mode `Pool.execute()` already logs a +warning and falls back to for any other role-typed task. The `builder` role +is the one exception: `internal/storage.SeedRoleConfigs()` (called from +`internal/cli/serve.go` on every startup, idempotent, never overwrites a +human-authored active version) seeds a real `builder` system prompt +teaching the decompose-or-implement judgment and `spawn_subtask`'s +`role`/`depends_on`/`acceptance_criteria` parameters — without it, a +`builder`-role task has no guidance at all about when to decompose versus +implement directly, which is the entire point of the recursive design above. ### Epic-proposal tool (Phase 7c) @@ -602,17 +617,6 @@ Same pattern as `task.Priority`/`RetryConfig` below: `internal/role.RoleConfig.T The phase description for `Scheduler.escalateAskUserTimeout` didn't specify behavior when a BLOCKED task is already at its role's top escalation tier (or has no active role config at all). Leaving the task stuck BLOCKED forever seemed worse than the alternative, so it still resumes the task (unblocking it is the priority) at its *current* provider/model — no `UpdateTaskAgent` call — and marks that `KindEscalated` event `final: true` so it's visible in the event stream as "resumed without escalating" rather than a genuine tier bump. A later phase could reconsider this (e.g. leaving the task BLOCKED and only flagging it) if it turns out best-effort auto-resumption at the same tier isn't the right call for every role. -### StoryOrchestrator's Arbitration outcome always routes to REVIEW_READY - -`internal/scheduler.StoryOrchestrator.finalizeArbitration` does not parse the -Arbitration task's summary for an approve/reject verdict — every completed -Arbitration moves the story to `REVIEW_READY`, never directly to `NEEDS_FIX`. -A human or chatbot who reads the Arbitration summary and disagrees must -manually set the story to `NEEDS_FIX` via `PUT /api/stories/{id}`. A later -phase could close this by giving the Arbitration task a dedicated -verdict-reporting tool (a new `AgentChannel` method) instead of parsing free -text. - ### StoryOrchestrator's per-evaluator verdict de-dup is in-memory only `StoryOrchestrator.handledVerdicts` (keyed by evaluator task ID) prevents |