From fd635d2a4f897bbe626fbcf7238a0004d599aabc Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:09:52 +0000 Subject: [PATCH 01/16] docs: design the resumable step model and the typed human-request pause Two sibling subjects from one investigation into the ask/wait flow: a run's pause for a person becomes one typed request (design/human-requests.md), and a step becomes an atomic unit that re-runs whole or replays from a record so there is no mid-step resume to build (design/workflow-resume.md). DECISIONS records the human-request decision; INDEX points at both. --- dev/DECISIONS.md | 26 +++++++ dev/INDEX.md | 1 + dev/design/human-requests.md | 141 ++++++++++++++++++++++++++++++++++ dev/design/workflow-resume.md | 109 ++++++++++++++++++++++++++ 4 files changed, 277 insertions(+) create mode 100644 dev/design/human-requests.md create mode 100644 dev/design/workflow-resume.md diff --git a/dev/DECISIONS.md b/dev/DECISIONS.md index 7f439e7..4b193b2 100644 --- a/dev/DECISIONS.md +++ b/dev/DECISIONS.md @@ -418,3 +418,29 @@ pass commits per note and writes each note's row last: an overtaken index is sta Separately, `ATTR_TIMEOUT => 4` was removed — measured, pdo_sqlite's default busy timeout on this build is 60000 ms, so that line lowered it fifteenfold while its comment claimed to raise it. The same line is still in `ProjectStore::open()`. + +--- + +## 2026-07-25 — A run that needs a person raises one typed request + +**Decision.** A run that cannot proceed without a human raises ONE typed `request` — +`{ id, run, kind, prompt, payload, options }`, durable in the journal, resolved by a `resolution` that +names it by `id`. `WaitingHuman` stops being five undistinguished reasons (a parked question, a solver +to approve, a ticket to split, a spent budget, an exhausted strategy) and becomes the state a request +puts the ticket in; the board, the panel and the API all read the one shape. Budget is one such request: +`enforceBudget()`, when the total is spent, records a `budget` request and STOPS rather than calling the +ask channel — the person raises the limit out of band and resumes. `BudgetPolicy::Ask`, `parseExtraTokens` +and the in-run top-up are deleted. Designed in [`design/human-requests.md`](design/human-requests.md); +not yet built. + +**Why.** `WaitingHuman` was already reached from five places for five different human jobs, all identical +on the board — the reason lived only in a `report()` string the dashboard never structured. And the +budget path was wrong twice: to handle "no tokens" it spent tokens (the ask channel's front tier is the +supervisor, a model call, which cannot authorize a budget anyway), and on a resumed run that call reached +the human gate before the parked worker and — because the gate matched answers to the run by a FIFO +cursor, not to the question by `id` — consumed the human's answer to the worker's question. Matching a +resolution to its request by `id` closes that at the root, for every kind. + +**Open.** One `resolve` endpoint keyed by request id vs. per-kind endpoints (leaning one). Whether all +five sites convert at once or `question` + `budget` land first. Whether `IssueStatus` gains a per-kind +hint or the board reads the kind off the open request. See the design doc. diff --git a/dev/INDEX.md b/dev/INDEX.md index 5d515d4..00a5e40 100644 --- a/dev/INDEX.md +++ b/dev/INDEX.md @@ -37,6 +37,7 @@ file rather than overwriting the first, so the reasoning behind what was built s |---|---| | The knowledge base, as built | [`design/knowledge-base.md`](design/knowledge-base.md) | | The knowledge base, second pass | [`design/knowledge-base-next.md`](design/knowledge-base-next.md) | +| A run's request to a person | [`design/human-requests.md`](design/human-requests.md) | ## Hot paths diff --git a/dev/design/human-requests.md b/dev/design/human-requests.md new file mode 100644 index 0000000..e11f123 --- /dev/null +++ b/dev/design/human-requests.md @@ -0,0 +1,141 @@ +# A run's request to a person + +A run that cannot go on without a human today lands the ticket in `WaitingHuman` and writes a line of +prose to explain — and it does this from five different places for five different reasons, every one of +which looks identical on the board. The person opens the ticket to discover what is even being asked. +This makes the pause a single typed thing: a run raises ONE request, the request says what it is and how +it can be answered, and everything downstream — the board, the panel, the API, resume — reads that one +shape. + +## Why one shape + +`WaitingHuman` is already overloaded. A run reaches it when the model asked a person a question and is +parked on the answer; when a generated solver waits to be approved before it runs; when a ticket judged +too big has no sub-issues and nobody has said what the pieces are; when triage ran out of strategies and +handed it back; and — once this lands — when the budget is spent. These are not the same request: the +person answers a question, approves code, writes sub-tickets, or raises a limit. But the ticket carries +none of that distinction. The reason lives in a `report()` string the dashboard never structured, so the +board shows a column of identical cards each meaning something different. + +One typed request settles all of them at once, and it is the same shape the ask-gate already reaches for — +a durable `question`/`answer` pair — generalized so every other reason inherits the same durability and the +same resume for nothing. + +## Shape + +A request and its resolution, both durable in the run's journal: + +``` +request { id, run, kind, prompt, payload, options } -- what the run needs +resolution { ref -> request.id, data } -- what the person gave +``` + +The OPEN request of a run is its newest `request` with no `resolution` pointing back at it — exactly how +`openGate()` reads `question` against `answer` today. A resolution names its request by `id`, and that is +the whole point: a reply resolves the request it was given, never "the oldest unanswered one." + +`question`/`answer` become `request(kind: question)` / `resolution`; the gate's `answeredAfter` + cursor +generalize to `resolvedAfter` + cursor over all kinds. Nothing about the durability machinery is new — it +is the gate's, widened. + +## The pause has two forms, and the person sees neither + +Every pause records the request and sets `WaitingHuman`. Underneath, how the run WAITS differs, and that +difference is an optimization the person never sees: + +- **Live block.** The run coroutine is alive and parks on a channel for the resolution — the current gate: + the worker asked a question, the answer may arrive in seconds, and blocking lets the run continue + in-process without a relaunch. The journal is the durable fallback if the process dies while parked, + which is the whole of issue #87's fix. +- **Durable stop.** The run records the request and EXITS. There is nothing to wait on in-process — the + resolution is an out-of-band act (raise the budget, approve the solver) that may take a day, so holding a + coroutine open buys nothing. Resume relaunches the run, which reads its snapshot and the resolution and + carries on. + +The person, the board, and the API see one thing either way: an open request of some kind, with options. + +## Resolution and resume + +A resolution is the durable input that lets resume proceed; it is not itself the resume. The run resumes +through the same snapshot-and-replay machinery every restart uses (`workflow_state` plus the recorded +exchange); the resolution is simply the fact the resumed run was blocked on. An answer becomes the next +turn of the parked exchange; a raised budget lets `enforceBudget` pass; an approval lets the solver run. +Same engine, different durable fact. + +## The kinds + +| kind | raised when | payload | options | +|---|---|---|---| +| `question` | the model needs a person to decide (the `[question]` gate) | the question text | answer (free text) | +| `budget` | the run's token/time total is spent | spent, limit, tree context | give +N and resume · stop | +| `approve-solver` | a generated solver is written but not yet run | the solver source | run · reject (with reason) | +| `split` | a ticket judged too big has no sub-issues | the ticket, and why it is too big | write sub-issues and continue | +| `strategy` | triage has no strategy left to escalate to | the failure reason | take it manually · reformulate · close | + +The table is the map of today's scattered `setIssueStatus(WaitingHuman)` sites onto one mechanism. Each +row is a real place that reaches `WaitingHuman` now (`HttpGateSpeaker`, and `IssueRunner`'s +`ensureSolver` / `reportDecomposition` / `giveBackToProjectManager`); the change is that each RAISES a +typed request instead of setting a status and writing prose beside it. + +## Budget is a request, not a question + +The budget pause is why this document has the shape it does, and it carries a decision worth stating on +its own. + +`enforceBudget()` runs when the run's total is spent. Under the old `BudgetPolicy::Ask` it reacted by +calling the ask channel — "enter extra tokens to continue" — which is wrong twice over. First, the ask +channel's front tier is the supervisor, an agent: to handle "there are no tokens" it spends tokens making +a model call, and the supervisor cannot authorize a budget anyway. Second, on a resumed run that call +reaches the human gate before the parked worker does, and the gate — matching answers to the run by a +FIFO cursor rather than to the question by `id` — hands the budget check the human's answer to the +WORKER's question. The answer is consumed, parsed as a token count, comes back zero, the run stops, and +the person's reply is gone. + +So budget stops being an in-run question. `enforceBudget`, when the total is spent, raises a `budget` +request and STOPS — no channel, no model call. The ticket goes to `WaitingHuman` like any other request. +The person raises the limit out of band and resumes; on resume the budget is above zero, `enforceBudget` +passes, the run continues. `BudgetPolicy::Ask`, `parseExtraTokens`, and the in-run top-up are deleted. +Matching a resolution to its request by `id` closes the answer-stealing bug at the root, for every kind, +not only this one. + +## The dashboard + +- **Board.** A `WaitingHuman` card carries a badge of its request kind, so the person sees what is wanted + without opening it — a question, a budget, and a solver to approve are three different jobs and should + not look alike. +- **The request panel.** Opening the ticket shows the open request: its `prompt` (what is needed and + why), its `payload` as context (the question, the budget figures, the solver source, the split brief), + and its `options` as real controls — a text box for a question, a +N field with give/stop for a budget, + run/reject for a solver, a sub-issue editor for a split. +- **The bell.** A new open request rings the notification bell, on the channel that already carries run + events. +- **Live.** The request appears and clears over the board's live transport; resolving it takes the ticket + out of `WaitingHuman` without a reload. +- **History.** A resolved request stays in the ticket's timeline — the generalization of the + question/answer pair the chat renders today. + +## The API + +- `GET issue/{id}` includes the open request, if any: `{ kind, prompt, payload, options }`. The UI needs + nothing else to render the panel. +- `POST issue/{id}/resolve { requestId, ... }` supersedes the answer-only endpoint. It resolves a request + BY ID — which is what makes a resolution land on its own request — and rejects a body whose kind does + not match the open request. + +## What this rests on + +- **The resume machinery** — a run resuming into a recorded exchange, and, for a parked question, + continuing it from the resolution — is a sibling subject, captured with the two-step-kind cycle it + belongs to, not here. +- **The gate's answer-by-`id`** matching is the small change that both fixes the answer-stealing bug and + makes a typed resolution possible; the `ref` is already stored, only the read side ignores it. + +## Open + +- Whether `resolve` is one endpoint keyed by request id or a small set per kind. One endpoint is cleaner + and inherently id-addressed; per-kind endpoints read more explicitly. Leaning to one. +- Whether every current `WaitingHuman` site converts in one pass, or `question` + `budget` land first + (the two with a live path, and the two this discussion produced) and the rest follow as their UIs are + built. +- Whether `IssueStatus` gains a per-kind hint or the board reads the kind off the open request. Reading + the request keeps one source of truth; a status hint is cheaper to query. Not decided. diff --git a/dev/design/workflow-resume.md b/dev/design/workflow-resume.md new file mode 100644 index 0000000..f9f864c --- /dev/null +++ b/dev/design/workflow-resume.md @@ -0,0 +1,109 @@ +# Resuming a run without re-running the model + +A run must survive a crash and carry on — including a run paused waiting for a person. Today resume is +at STEP granularity only: a step is arbitrary imperative PHP that re-runs top to bottom on resume, and +the one thing stitched back is the first `ai()` call's history. A step parked mid-exchange on a human +question is not durably recoverable — the exchange at the park point is never persisted, so on resume the +model is asked again and the supervisor can answer in the person's place. + +The fix is not "resume in the middle of a step" — PHP cannot serialise a paused call stack. It is to make +a step an atomic unit that either re-runs whole (cheap, deterministic) or replays from a record (the +model was expensive and must not be re-run). Then there is no middle to resume to. + +## Two kinds of step + +Distinguished by attribute — `#[Step]` already exists, `#[StepAI]` is added — so the base knows the kind +by reflection BEFORE the method runs, and the resume path decides skip / re-run / replay without invoking +the body. + +- **`#[Step]` — a CODE step.** Pure, deterministic glue. May call `$this->tool(...)` any number of times. + On resume it is RE-RUN WHOLE: cheap, nothing recorded, no middle to come back to. Its one contract is + that re-running is safe (no non-idempotent side effect) — the sharpest asymmetry in the model, enforced + by discipline, not by the type. +- **`#[StepAI]` — an AI step.** A PURE method that builds a prompt from durable inputs and returns an + {@see AiStep} declaration (prompt, tools, agent, params); critic and maxRounds ride on the attribute. + It does no work and has no side effects — the base runs the ONE `ai()` exchange it declares. EXACTLY ONE + exchange per AI step: that single exchange is the atomic replay unit; two would force per-call recording, + the very complexity this removes. Interleaved computation goes into neighbouring CODE steps. + +## The cycle + +`run()` drives the steps in declaration order (both kinds), honouring `back()` exactly as today. Each is +handed to `step()`, which now branches at the top: a `#[StepAI]` method goes to `runAiStep()`, everything +else keeps the existing imperative path. The branch is the whole seam; the cycle is otherwise unchanged. + +## Resume of an AI step + +`runAiStep()` asks the exchange store what it already holds for this `(run, step)` and acts on it: + +- **EMPTY** → first run: open the declared prompt. +- **SETTLED** (recorded, ending on a real assistant answer) → REPLAY: take the recorded final text, **no + model call**. A resume never re-buys a turn already produced. +- **PARKED** (recorded, ending on an unanswered `[question]`) → CONTINUE: the ask channel returns the + human's answer (from the journal, once matched by request id — see human-requests.md) and it becomes the + next turn. The model is not asked again; the supervisor is not consulted a second time. + +Then the critic loop, unchanged in spirit — judge the AI output, while unhappy let the supervisor guide a +re-run, bounded by maxRounds — reusing the existing `critic()` / `superviseStep()`. + +The prerequisite that makes PARKED possible: the turn loop persists the exchange AT THE PARK POINT, before +it blocks the ask channel on a `[question]`. Today the checkpoint fires only after a tool-turn, so a park +leaves nothing recorded. `DefaultTurnLoop::pendingQuestion()` reads the recorded tail back to tell SETTLED +from PARKED. + +## The AI step's output + +Reuses the existing handoff machinery: after the accepted work, the engine CONTINUES the step's own +conversation with a dedicated extraction request. Two sinks, one mechanism: + +- **handoff** — the prose baton to the next step (what `formPendingHandoff()` already does). +- **param** — when a later CODE step needs a machine-readable value (a word, a path, an id), the same kind + of request asks the model for exactly that value and `setParam()` addresses it to that step, which reads + it with `param()`. Declared on the `AiStep` so deciding it is part of designing the step. + +## The gaps, and how they close + +Named so the machinery handles them rather than shipping them: + +1. **The park checkpoint.** Add one checkpoint call in the turn loop's `[question]` branch, before it + blocks — otherwise PARKED has nothing to resume from. +2. **Extraction must not overwrite the work.** `extractParams()` and `formPendingHandoff()` continue the + step's conversation and would checkpoint into the same `(run, step)` exchange row, so a crash mid- + extraction leaves the row holding the extraction Q&A — which resume would replay as the work. They run + under a guard (the same shape as `$reviewing`) that suppresses that checkpoint. +3. **Artifacts on REPLAY.** `$this->artifacts[$step]` is transient, filled only by tool calls made THIS + process; a SETTLED replay makes none, so the critic and extraction see none. They are read back from + the journal on replay (`TraceReader`), never by re-executing the recorded `artifact` tool calls — that + would re-run the shell behind the evidence channel, which a free replay must not. +4. **The reviewer has no ask channel.** A critic's exchange should never park on a `[question]`; the + review palette is built without `EnvKey::Ask`, closing that off structurally rather than trying to make + a reviewer-park resumable. +5. **SETTLED vs PARKED is a durable flag, not a text sniff.** The checkpoint records WHY it fired (a + mid-turn checkpoint vs a park), so detection does not lean on finding the literal `[question]` in the + tail. (First cut may sniff; the flag is the honest version.) +6. **The critic round counter is durable.** Persisted with the snapshot so a crash mid-critic-loop does + not restart the count and let a step exceed maxRounds across incarnations. + +## Coexistence and migration + +`#[Step]` and `#[StepAI]` run side by side. The old imperative path is untouched, so every existing solver +and test keeps working; new work is written declaratively. The generator's prompt moves to the new rules +(a pre/post CODE step around a declarative AI step) once the machinery is proven. The old path — and the +duplicated critic loop coexistence leaves behind — is deleted when nothing writes imperative `ai()` steps +any more. The duplication is a named, temporary cost of not rewriting a load-bearing class in one motion. + +## Sequence + +Each increment lands green on its own. + +1. **Park durability.** `DefaultTurnLoop` checkpoints at the park; `pendingQuestion()` reads it back. + Foundational, tiny, safe. +2. **The AI step.** `AiStep`, `#[StepAI]`, the `step()` branch, `runAiStep()` with EMPTY / SETTLED / + PARKED and the critic loop, coexisting. Gaps 2–4 closed here. +3. **Output extraction.** handoff (reused) + declared `param` extraction. +4. **Typed requests + budget.** The human-requests.md work: `request`/`resolution`, answer-by-id, budget + as a request. Gap 1 of human-requests (budget stops calling `ask()`). +5. **The generator.** Move `GenerateIssueWorkflow`'s prompt to pre/post-CODE + declarative-AI; migrate the + shipped workflows; then delete the old imperative path. + +This document is the engine half; the person-facing half is [`human-requests.md`](human-requests.md). From 1dba331ebb93397f787c5025213cbbbc8c45b818 Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:09:57 +0000 Subject: [PATCH 02/16] feat(agent): persist the exchange at the human-question park The [question] turn is a no-tool turn the tool-turn checkpoint never reaches, so a crash while parked left nothing recorded and a resume re-asked the model. The loop now checkpoints before it blocks on the answer, and pendingQuestion() reads that tail back to tell a park (continue from the answer) from a settled exchange (replay). First increment of design/workflow-resume.md. --- src/Agent/DefaultTurnLoop.php | 45 ++++++++++++++++++- tests/Agent/DefaultTurnLoopTest.php | 68 +++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 2 deletions(-) diff --git a/src/Agent/DefaultTurnLoop.php b/src/Agent/DefaultTurnLoop.php index 072805f..7869a77 100644 --- a/src/Agent/DefaultTurnLoop.php +++ b/src/Agent/DefaultTurnLoop.php @@ -252,9 +252,17 @@ public function run(array $history): TurnResult // A turn carrying the [question] marker is the latter: route it to the channel, inject // the answer as the next user turn, and continue the same loop (context stays whole). if ($this->ask !== null) { - $question = $this->extractQuestion($response->text ?? ''); + $question = self::extractQuestion($response->text ?? ''); if ($question !== null) { + // Persist the exchange BEFORE blocking on the answer: this turn — the [question] — is + // a no-tool turn the tool-turn checkpoint below never reaches, so without this a crash + // while parked leaves nothing recorded and a resume re-asks the model instead of + // continuing from the answer. {@see pendingQuestion()} reads this tail back on resume. + if ($this->checkpoint !== null) { + ($this->checkpoint)($history); + } + $answer = $this->ask->reply($question); if ($answer !== null) { // null = the chain passed up, no one answered @@ -418,7 +426,7 @@ private function keepGoing(int $turnNo): bool * else null (a normal final answer). The marker is stripped; a bare marker with no question * falls back to a real prompt rather than echoing the literal "[question]" at the channel. */ - private function extractQuestion(string $text): ?string + private static function extractQuestion(string $text): ?string { if (!str_contains($text, self::QUESTION_MARKER)) { return null; @@ -428,4 +436,37 @@ private function extractQuestion(string $text): ?string return $question === '' ? 'The worker paused for input but gave no question.' : $question; } + + /** + * The unanswered [question] a checkpointed exchange ended on, or null when the last turn is an + * ordinary finished answer (an empty history, or a tail still asking for tools, is not a park). + * + * A resumed AI step reads this off its recorded exchange to tell a PARK — continue from the human's + * answer — from a SETTLED exchange it should replay. It is the same marker the live loop routes on, + * read from the durable tail rather than from a fresh response. + * + * @param list $history + */ + public static function pendingQuestion(array $history): ?string + { + $last = $history === [] ? null : $history[array_key_last($history)]; + + if (!$last instanceof Message || $last->role !== Role::Assistant) { + return null; + } + + $text = ''; + + foreach ($last->content as $block) { + if ($block instanceof ToolUseBlock) { + return null; // a turn still asking for tools was never a park + } + + if ($block instanceof TextBlock) { + $text .= $block->text; + } + } + + return self::extractQuestion($text); + } } diff --git a/tests/Agent/DefaultTurnLoopTest.php b/tests/Agent/DefaultTurnLoopTest.php index 6e86443..83a6879 100644 --- a/tests/Agent/DefaultTurnLoopTest.php +++ b/tests/Agent/DefaultTurnLoopTest.php @@ -627,6 +627,74 @@ public function withoutAnAskChannelAQuestionMarkerIsJustTheFinalAnswer(): void Assert::same($agent->requests[0]->system, 's'); // system prompt left untouched } + #[Test] + public function checkpointsTheExchangeBeforeBlockingOnAQuestion(): void + { + // The [question] turn is a no-tool turn the tool-turn checkpoint never reaches. Without a + // checkpoint AT THE PARK, a crash while waiting leaves nothing recorded and a resume re-asks the + // model. So the loop writes the exchange down — ending on the question — before it blocks. + $agent = new ScriptedAgent( + new AgentResponse([new TextBlock('[question] which file?')], [], StopReason::EndTurn, new Usage(), '[question] which file?'), + new AgentResponse([new TextBlock('done')], [], StopReason::EndTurn, new Usage(), 'done'), + ); + $ask = new class () implements SpeakerInterface { + public function name(): SpeakerRole + { + return SpeakerRole::Human; + } + + public function reply(string $incoming): string + { + return 'src/Foo.php'; + } + }; + + $checkpoints = []; + $loop = new DefaultTurnLoop( + $agent, + new RecordingExecutor(), + 'm', + 's', + new Registry(), + ask: $ask, + checkpoint: function (array $history) use (&$checkpoints): void { + $checkpoints[] = $history; + }, + ); + + $loop->run([Message::userText('go')]); + + // Exactly one checkpoint — at the park; the answered turn and the final 'done' are no-tool turns. + Assert::count($checkpoints, 1); + $parked = $checkpoints[0]; + + // Two messages — user(go) + assistant([question]) — so it was written BEFORE the answer was + // appended (that would make three), and its tail reads back as a park, not a settled answer. + Assert::count($parked, 2); + Assert::same(DefaultTurnLoop::pendingQuestion($parked), 'which file?'); + } + + #[Test] + public function pendingQuestionTellsAParkFromASettledExchange(): void + { + $parked = [ + Message::userText('go'), + new Message(Role::Assistant, [new TextBlock('[question] which file?')]), + ]; + Assert::same(DefaultTurnLoop::pendingQuestion($parked), 'which file?'); // a park -> the question + + $settled = [ + Message::userText('go'), + new Message(Role::Assistant, [new TextBlock('all done')]), + ]; + Assert::same(DefaultTurnLoop::pendingQuestion($settled), null); // an ordinary answer -> null + + $midTool = [new Message(Role::Assistant, [new ToolUseBlock('t', 'echo', [])])]; + Assert::same(DefaultTurnLoop::pendingQuestion($midTool), null); // a tail asking for tools -> null + + Assert::same(DefaultTurnLoop::pendingQuestion([]), null); // empty -> null + } + #[Test] public function stopsTheExchangeWhenTheTurnBudgetIsSpent(): void { From 84fe9df2ef997663202f51ae39e2d4c961b80503 Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:22:43 +0000 Subject: [PATCH 03/16] feat(workflow): the base drives, records and resumes a #[StepAI] step A step is now one of two kinds, told apart by attribute so the resume path decides continue-vs-re-run before running the body. A #[StepAI] method is pure: it returns an AiStep declaration and the base runs the one exchange it names. On resume the base continues the recorded conversation instead of re-running the model, and a crash while PARKED on a person's answer resumes by continuing from that answer rather than asking again. Coexists with the imperative #[Step] path, untouched, so every existing solver and test keeps working. Second increment of design/workflow-resume.md. --- src/Workflow/AiStep.php | 31 +++++++ src/Workflow/StepAI.php | 21 +++++ src/Workflow/WorkflowAbstract.php | 107 +++++++++++++++++++++++- tests/Workflow/WorkflowAbstractTest.php | 80 ++++++++++++++++++ 4 files changed, 236 insertions(+), 3 deletions(-) create mode 100644 src/Workflow/AiStep.php create mode 100644 src/Workflow/StepAI.php diff --git a/src/Workflow/AiStep.php b/src/Workflow/AiStep.php new file mode 100644 index 0000000..608b3dd --- /dev/null +++ b/src/Workflow/AiStep.php @@ -0,0 +1,31 @@ + $tools null = every tool (default); a list = only those; [] = none — the same + * contract {@see WorkflowAbstract::ai()} already used + * @param ?string $agent a named agent role (worker/reviewer/planner/…) whose model runs this + * exchange, or null for the run's default + */ + public function __construct( + public string $prompt, + public ?array $tools = null, + public ?string $agent = null, + ) { + } +} diff --git a/src/Workflow/StepAI.php b/src/Workflow/StepAI.php new file mode 100644 index 0000000..870148b --- /dev/null +++ b/src/Workflow/StepAI.php @@ -0,0 +1,21 @@ +isAiStep($name)) { + $this->runAiStep($name); // a #[StepAI] step is driven by the base, not by an imperative body + + return; + } + $this->enforceBudget(); // don't begin a new step once the run's budget is spent $tracer = $this->tracer(); @@ -425,6 +431,101 @@ protected function step(string $name): void $this->env->findStore()->clearExchange($this->runId, $name); } + /** Whether $name is an AI step (marked {@see StepAI}) — read by reflection, without running the body. */ + private function isAiStep(string $name): bool + { + return new \ReflectionMethod($this, $name)->getAttributes(StepAI::class) !== []; + } + + /** + * Drive a {@see StepAI} step. The method itself is pure — it returns an {@see AiStep} declaration and + * does nothing else; the base runs the one exchange it names, records it as it happens, and on resume + * CONTINUES the recorded conversation rather than re-running the model from the top. The step's output + * is its handoff, formed from this exchange for the next step, exactly as an imperative step's is. + * + * A crash while the step was PARKED on a person's answer resumes here: {@see runAiExchange()} finds + * the recorded exchange ending on an unanswered [question] and continues it from the answer. The body + * is never re-entered after a resume and is pure anyway, so there is nothing to do twice. + */ + private function runAiStep(string $name): void + { + $this->enforceBudget(); + + $tracer = $this->tracer(); + $span = $tracer?->enterStep($name); + $previousStep = $this->currentStep; + $this->currentStep = $name; + $workHistory = []; + + // A back() into this step re-enters it fresh (its exchange was cleared when it finished); continuing + // the prior attempt is a refinement the imperative path has and this one will grow. Clear the arming + // so it does not leak to a later step. + if ($this->reentryStep === $name) { + $this->reentryStep = null; + $this->reentryReason = ''; + } + + try { + $this->artifacts[$name] = []; + $this->writtenPaths = []; + + $declaration = $this->{$name}(); + + if (!$declaration instanceof AiStep) { + throw new \LogicException("step '{$name}' is marked #[StepAI] but did not return an AiStep declaration"); + } + + $this->formPendingHandoff(); // form the PREVIOUS step's handoff before this one's exchange runs + $this->runAiExchange($name, $declaration); + $workHistory = $this->lastHistory; + $this->recordWrittenFiles($workHistory); + $this->emitWrittenFileArtifacts(); + } finally { + $this->currentStep = $previousStep; + $tracer?->exit($span); + } + + $this->pendingHandoff = ['name' => $name, 'history' => $workHistory]; + $this->stepHistory[$name] = $workHistory; + $this->done[] = $name; + $this->env->findStore()->save($this->runId, $this->captureState(), $this->done); + $this->env->findStore()->clearExchange($this->runId, $name); + } + + /** + * Run — or continue — the one exchange a {@see StepAI} declares. What the store already holds for this + * (run, step) decides which: + * + * - PARKED — the recorded exchange ends on an unanswered [question]: continue it from the person's + * answer, which becomes the next user turn, so the model picks up where it paused instead of being + * asked the same thing again. This is the resume the two-kind model exists to make deterministic. + * - otherwise (nothing recorded, or a partial non-parked tail) — open the declared prompt. A partial + * tail is carried in as prior context and the in-flight turn is re-run, which is cheap and correct + * since a non-parked tail was never the answer. + * + * The exchange goes through {@see runTurns()} like any other, so its palette, agent, ask channel, + * budget and per-turn checkpointing are exactly an imperative step's ai() call's. + */ + private function runAiExchange(string $name, AiStep $step): void + { + $recorded = $this->env->findStore()->loadExchange($this->runId, $name); + $pending = $recorded === [] ? null : DefaultTurnLoop::pendingQuestion($recorded); + + if ($pending !== null) { + $ask = $this->env->find(EnvKey::Ask); + + if (!$ask instanceof SpeakerInterface) { + throw new WorkflowException("step '{$name}' paused on a person's answer, but no ask channel is configured to resume it"); + } + + $this->runTurns($ask->reply($pending) ?? '', $step->tools, $step->agent, $recorded); + + return; + } + + $this->runTurns($step->prompt, $step->tools, $step->agent, $recorded); + } + /** Read a value from the run's environment — this scope, then the parent project settings. */ protected function find(EnvKey|string $key): mixed { @@ -1654,8 +1755,8 @@ protected function log(string $action, string $message = '', array $context = [] } /** - * The workflow's step methods (those marked {@see Step}), in declaration order — what the - * default run() drives. + * The workflow's step methods (those marked {@see Step} or {@see StepAI}), in declaration order — + * what the default run() drives. * * @return list */ @@ -1664,7 +1765,7 @@ private function stepMethods(): array $names = []; foreach (new \ReflectionClass($this)->getMethods() as $method) { - if ($method->getAttributes(Step::class) !== []) { + if ($method->getAttributes(Step::class) !== [] || $method->getAttributes(StepAI::class) !== []) { $names[] = $method->getName(); } } diff --git a/tests/Workflow/WorkflowAbstractTest.php b/tests/Workflow/WorkflowAbstractTest.php index 749364d..8727db1 100644 --- a/tests/Workflow/WorkflowAbstractTest.php +++ b/tests/Workflow/WorkflowAbstractTest.php @@ -7,6 +7,7 @@ use Claw\Agent\AgentInterface; use Claw\Agent\AgentResponse; use Claw\Agent\Budget; +use Claw\Agent\Message; use Claw\Agent\Role; use Claw\Agent\SpeakerInterface; use Claw\Agent\SpeakerRole; @@ -26,11 +27,13 @@ use Claw\Trace\ArrayTraceSink; use Claw\Trace\Tracer; use Claw\Trace\TraceRecordInterface; +use Claw\Workflow\AiStep; use Claw\Workflow\BudgetPolicy; use Claw\Workflow\Environment; use Claw\Workflow\EnvKey; use Claw\Workflow\InMemoryStateStore; use Claw\Workflow\Step; +use Claw\Workflow\StepAI; use Claw\Workflow\Tool; use Claw\Workflow\WorkflowAbstract; use Claw\Workflow\WorkflowStateStoreInterface; @@ -221,6 +224,83 @@ public function go(): string Assert::same($wf->calls, 1); // the model's tool call reached the workflow method } + #[Test] + public function anAiStepRunsTheDeclaredExchangeAndCompletes(): void + { + // A #[StepAI] method is PURE: it returns an AiStep and does no work. The base runs the one + // exchange it declares, then marks the step done — the method never touches the model itself. + $store = new InMemoryStateStore(); + $worker = new ScriptedAgent($this->answer('done')); + $wf = new class ($this->config(worker: $worker, store: $store), 'r1') extends WorkflowAbstract { + public function name(): string + { + return 'ai'; + } + + #[StepAI] + protected function work(): AiStep + { + return new AiStep('do the work'); + } + }; + + $wf->run(); + + Assert::count($worker->requests, 1); // the base ran the declared exchange + Assert::same($this->lastUserText($worker), 'do the work'); // with the declared prompt + Assert::same($store->load('r1')['done'], ['work']); // and recorded the step as done + } + + #[Test] + public function anAiStepParkedOnAQuestionResumesFromTheAnswer(): void + { + // The prior life got as far as asking a person and parked — the recorded exchange ends on the + // [question]. A fresh instance must CONTINUE from the answer, not re-run the model from the top. + $store = new InMemoryStateStore(); + $store->saveExchange('r1', 'work', [ + Message::userText('do the work'), + new Message(Role::Assistant, [new TextBlock('[question] which file?')]), + ]); + + $channel = new class () implements SpeakerInterface { + public ?string $heard = null; + + public function name(): SpeakerRole + { + return SpeakerRole::Human; + } + + public function reply(string $incoming): string + { + $this->heard = $incoming; + + return 'src/Foo.php'; + } + }; + + $worker = new ScriptedAgent($this->answer('done')); + $env = $this->config(worker: $worker, store: $store)->set(EnvKey::Ask, $channel); + $wf = new class ($env, 'r1') extends WorkflowAbstract { + public function name(): string + { + return 'ai'; + } + + #[StepAI] + protected function work(): AiStep + { + return new AiStep('do the work'); + } + }; + + $wf->run(); + + Assert::same($channel->heard, 'which file?'); // resumed by asking the parked question + Assert::count($worker->requests, 1); // one model call — to CONTINUE, not re-ask + Assert::same($this->lastUserText($worker), 'src/Foo.php'); // the answer went in as the next turn + Assert::same($store->load('r1')['done'], ['work']); // and the step then completed + } + #[Test] public function toolResolvesThroughTheRegistryAndReturnsItsContent(): void { From 38cf073c6996800fa19316f92478610a15511f67 Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:43:24 +0000 Subject: [PATCH 04/16] feat(workflow): a #[StepAI] step can carry a critic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The critic and its round cap ride on the attribute, so the resume path reads them without running the body — the same reason the kind is an attribute. The review loop is the imperative path's, reused: the two deterministic guards and the AI critic are pulled into a shared verdictFor() so their wording cannot drift between the two kinds, and the base drives an AI step's re-run by continuing the prior exchange with the supervisor's guidance as its next turn. A reentered guard keeps the critic's own ai() from reloading the step's exchange as its context. Part of design/workflow-resume.md (increment 3). --- src/Workflow/StepAI.php | 15 +++ src/Workflow/WorkflowAbstract.php | 138 +++++++++++++++++++----- tests/Workflow/WorkflowAbstractTest.php | 64 +++++++++++ 3 files changed, 190 insertions(+), 27 deletions(-) diff --git a/src/Workflow/StepAI.php b/src/Workflow/StepAI.php index 870148b..c7c9644 100644 --- a/src/Workflow/StepAI.php +++ b/src/Workflow/StepAI.php @@ -13,9 +13,24 @@ * decides continue-vs-re-run BEFORE it ever calls the body. That is the whole reason the kind is an * attribute and not the method's return type: reflection reads it without running anything. * + * A critic rides on the attribute, not on the returned declaration, so the resume path can read it (and + * the round cap) without running the body — the same reason the kind itself is an attribute. The base + * judges the step's recorded artifacts against it exactly as it does an imperative step's. + * * @see dev/design/workflow-resume.md */ #[\Attribute(\Attribute::TARGET_METHOD)] final class StepAI { + /** + * @param ?string $critic the critic name this step's output is judged against (null = no critic); + * its rules live in {@see WorkflowAbstract::criticRules()} under this key + * @param ?int $maxRounds soft cap on critic rework rounds before the run escalates to the + * supervisor (null = the workflow default) + */ + public function __construct( + public ?string $critic = null, + public ?int $maxRounds = null, + ) { + } } diff --git a/src/Workflow/WorkflowAbstract.php b/src/Workflow/WorkflowAbstract.php index 23dc666..4febe12 100644 --- a/src/Workflow/WorkflowAbstract.php +++ b/src/Workflow/WorkflowAbstract.php @@ -332,7 +332,7 @@ protected function step(string $name): void try { $step = $this->stepAttribute($name); // reflect the Step attribute once, read both fields off it - $rubric = $this->criticRubric($step, $name); + $rubric = $this->criticRubric($step?->critic, $name); $this->critique = null; // Run the step; if it declares a critic, judge the ARTIFACTS it produced (its reviewable @@ -340,7 +340,7 @@ protected function step(string $name): void // supervisor guide a re-run — until the critic passes, the supervisor accepts/stops, the // soft round cap escalates, or the budget runs out. $round = 0; - $maxRounds = $this->maxRounds($step); + $maxRounds = $this->maxRounds($step?->maxRounds); $workHistory = []; $resume = []; // the prior attempt's conversation; a re-run continues it (empty on the first attempt) $priorAttempt = null; // fingerprint of the previous attempt's artifacts — see the identical-attempt stop @@ -383,17 +383,13 @@ protected function step(string $name): void $this->artifacts[$name], ); - if ($workHistory === [] && $this->artifacts[$name] === []) { - $verdict = Verdict::reject("step '{$name}' produced nothing: no model/tool work and no artifact. A step " - . 'must do real work and leave a result; if it needs no review, it should carry no critic.'); - } elseif ($priorAttempt !== null && $attempt === $priorAttempt) { - $verdict = Verdict::reject('the re-run produced BYTE-IDENTICAL output to the previous attempt — the ' - . 'guidance changed nothing about the work. Another round cannot help: either the ' - . 'finding is wrong, or this step needs different guidance or a person.'); - } else { - $verdict = $this->critic($name, $rubric, $artifacts); - } - + $verdict = $this->verdictFor( + $name, + $rubric, + $artifacts, + $workHistory === [] && $this->artifacts[$name] === [], + $priorAttempt !== null && $attempt === $priorAttempt, + ); $priorAttempt = $attempt; if ($verdict->passes()) { @@ -455,6 +451,8 @@ private function runAiStep(string $name): void $span = $tracer?->enterStep($name); $previousStep = $this->currentStep; $this->currentStep = $name; + $this->critique = null; + $this->reentered[$name] = true; // this step's exchange is base-driven; a critic's own ai() must not reload it $workHistory = []; // A back() into this step re-enters it fresh (its exchange was cleared when it finished); continuing @@ -466,21 +464,20 @@ private function runAiStep(string $name): void } try { - $this->artifacts[$name] = []; - $this->writtenPaths = []; - $declaration = $this->{$name}(); if (!$declaration instanceof AiStep) { throw new \LogicException("step '{$name}' is marked #[StepAI] but did not return an AiStep declaration"); } + $attribute = $this->aiAttribute($name); + $rubric = $this->criticRubric($attribute?->critic, $name); + $maxRounds = $this->maxRounds($attribute?->maxRounds); + $this->formPendingHandoff(); // form the PREVIOUS step's handoff before this one's exchange runs - $this->runAiExchange($name, $declaration); - $workHistory = $this->lastHistory; - $this->recordWrittenFiles($workHistory); - $this->emitWrittenFileArtifacts(); + $workHistory = $this->reviewedExchange($name, $declaration, $rubric, $maxRounds); } finally { + $this->critique = null; $this->currentStep = $previousStep; $tracer?->exit($span); } @@ -492,6 +489,76 @@ private function runAiStep(string $name): void $this->env->findStore()->clearExchange($this->runId, $name); } + /** The {@see StepAI} attribute on $name, instantiated, or null when the method is not an AI step. */ + private function aiAttribute(string $name): ?StepAI + { + $attributes = new \ReflectionMethod($this, $name)->getAttributes(StepAI::class); + + return $attributes === [] ? null : $attributes[0]->newInstance(); + } + + /** + * The AI step's exchange under its critic. Round 0 runs (or resumes) the declared exchange; while the + * critic is unhappy the supervisor guides a re-run that CONTINUES the prior attempt with the guidance + * as its next turn — until the critic passes, the supervisor accepts/stops, or the cap escalates. With + * no rubric it is a single exchange. The judge/guard/supervise is the same the imperative path uses + * (via {@see verdictFor()} and {@see superviseStep()}); only PRODUCING an attempt differs — a declared + * exchange, not a hand-written body. + * + * @return list + */ + private function reviewedExchange(string $name, AiStep $step, ?string $rubric, int $maxRounds): array + { + $round = 0; + $priorAttempt = null; + $workHistory = []; + + while (true) { + $this->artifacts[$name] = []; + $this->writtenPaths = []; + + if ($round === 0) { + $this->runAiExchange($name, $step); + } else { + $this->runTurns((string) $this->critique, $step->tools, $step->agent, $workHistory); // continue with guidance + } + + $workHistory = $this->lastHistory; // capture BEFORE the critic, whose own ai() clobbers lastHistory + $this->recordWrittenFiles($workHistory); + $this->emitWrittenFileArtifacts(); + + if ($rubric === null) { + break; + } + + $artifacts = $this->renderArtifacts($this->artifacts[$name]); + $attempt = array_map(static fn (Artifact $a): array => [$a->label, $a->kind, $a->value], $this->artifacts[$name]); + $verdict = $this->verdictFor( + $name, + $rubric, + $artifacts, + $workHistory === [] && $this->artifacts[$name] === [], + $priorAttempt !== null && $attempt === $priorAttempt, + ); + $priorAttempt = $attempt; + + if ($verdict->passes()) { + break; + } + + $guidance = $this->superviseStep($name, $artifacts, $verdict, ++$round, $maxRounds); + + if ($guidance === null) { + break; // the supervisor accepted the work as-is + } + + $this->critique = $guidance; + $this->enforceBudget(); + } + + return $workHistory; + } + /** * Run — or continue — the one exchange a {@see StepAI} declares. What the store already holds for this * (run, step) decides which: @@ -1301,10 +1368,8 @@ private function stepAttribute(string $name): ?Step * actual rules live in {@see criticRules()}, keyed by that name. Null when the step has no critic. * An unknown name is a generation bug — fail loud rather than judge against an empty rubric. */ - private function criticRubric(?Step $step, string $name): ?string + private function criticRubric(?string $critic, string $name): ?string { - $critic = $step?->critic; - if ($critic === null || $critic === '') { return null; } @@ -1318,14 +1383,33 @@ private function criticRubric(?Step $step, string $name): ?string return $rules; } - /** The soft critic-round cap for a step — its `#[Step(maxRounds: N)]`, else the workflow default. */ - private function maxRounds(?Step $step): int + /** The soft critic-round cap a step declared (`#[Step(maxRounds: N)]` / `#[StepAI(...)]`), else the default. */ + private function maxRounds(?int $max): int { - $max = $step?->maxRounds; - return $max !== null && $max > 0 ? $max : self::DEFAULT_MAX_ROUNDS; } + /** + * The verdict on one attempt: the two deterministic guards first — a step that produced nothing, or a + * re-run byte-identical to the last, neither worth an AI critic's round — else the AI critic. Shared by + * both step kinds so the guards and their wording cannot drift between them. + */ + private function verdictFor(string $name, string $rubric, string $artifacts, bool $producedNothing, bool $identical): Verdict + { + if ($producedNothing) { + return Verdict::reject("step '{$name}' produced nothing: no model/tool work and no artifact. A step " + . 'must do real work and leave a result; if it needs no review, it should carry no critic.'); + } + + if ($identical) { + return Verdict::reject('the re-run produced BYTE-IDENTICAL output to the previous attempt — the ' + . 'guidance changed nothing about the work. Another round cannot help: either the ' + . 'finding is wrong, or this step needs different guidance or a person.'); + } + + return $this->critic($name, $rubric, $artifacts); + } + /** * The rules each critic judges by, keyed by the name used in `#[Step(critic: '')]`. A * workflow that uses critics overrides this to spell out, per critic, the concrete criteria the diff --git a/tests/Workflow/WorkflowAbstractTest.php b/tests/Workflow/WorkflowAbstractTest.php index 8727db1..8ddd7d6 100644 --- a/tests/Workflow/WorkflowAbstractTest.php +++ b/tests/Workflow/WorkflowAbstractTest.php @@ -301,6 +301,70 @@ protected function work(): AiStep Assert::same($store->load('r1')['done'], ['work']); // and the step then completed } + #[Test] + public function anAiStepUnderACriticReRunsOnTheSupervisorsGuidance(): void + { + // A #[StepAI] carries its critic on the attribute. The critic rejects the first attempt; the + // supervisor's guidance drives a re-run that CONTINUES the exchange; the second attempt passes — + // the same review loop the imperative path uses, over a declared exchange instead of a body. + $worker = new ScriptedAgent( + $this->toolUse('artifact', ['label' => 'work', 'text' => 'first attempt']), // attempt 1 records its output + $this->answer('done'), // attempt 1 finishes + $this->toolUse('verdict', [ // critic 1 rejects + 'decision' => 'reject', + 'rubric_item' => 'the work must be tested', + 'fact' => 'the work has no test', + ]), + $this->answer('reviewed'), // closes critic 1's exchange + $this->toolUse('artifact', ['label' => 'work', 'text' => 'second attempt']), // attempt 2 records a different output + $this->answer('done'), // attempt 2 finishes on the guidance + $this->answer('OK'), // critic 2 accepts + ); + $supervisor = new class () implements SpeakerInterface { + public ?string $heard = null; + + public function name(): SpeakerRole + { + return SpeakerRole::Supervisor; + } + + public function reply(string $incoming): string + { + $this->heard = $incoming; + + return 'add the missing test'; + } + }; + + $store = new InMemoryStateStore(); + $env = $this->config(worker: $worker, store: $store)->set(EnvKey::Ask, $supervisor); + $wf = new class ($env, 'r1') extends WorkflowAbstract { + public function name(): string + { + return 'ai'; + } + + protected function criticRules(): array + { + return ['reviewed' => 'the work must be tested']; + } + + #[StepAI(critic: 'reviewed')] + protected function work(): AiStep + { + return new AiStep('do the work'); + } + }; + + $wf->run(); + + // the reject reached the supervisor with the rubric item and the observed fact + Assert::true(str_contains((string) $supervisor->heard, 'Rubric item violated: the work must be tested')); + Assert::true(str_contains((string) $supervisor->heard, 'Observed: the work has no test')); + Assert::same($store->load('r1')['done'], ['work']); // the step completed after the accepted re-run + Assert::true(\count($worker->requests) >= 4); // attempt, critic, re-run, critic — a re-run happened + } + #[Test] public function toolResolvesThroughTheRegistryAndReturnsItsContent(): void { From 28a767956b86c966cebc71523f3d22c737d01a9a Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 09:47:00 +0000 Subject: [PATCH 05/16] feat(workflow): a #[StepAI] step extracts machine-readable params for later code steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An AiStep may declare ParamRequests: after the work settles, the base continues the step's own conversation to ask the model for exactly that value and pins it with setParam() for the named step, which reads it with param(). It is the handoff mechanism addressed to a step in code rather than the next step's model. The extraction runs under an $extracting guard so its turns are not checkpointed into the step's exchange row — otherwise a crash mid-extraction would leave that row holding the extraction Q&A, which a resume would replay as the step's work. Completes increment 3 of design/workflow-resume.md. --- src/Workflow/AiStep.php | 11 +++--- src/Workflow/ParamRequest.php | 29 ++++++++++++++++ src/Workflow/WorkflowAbstract.php | 46 +++++++++++++++++++++++-- tests/Workflow/WorkflowAbstractTest.php | 41 ++++++++++++++++++++++ 4 files changed, 121 insertions(+), 6 deletions(-) create mode 100644 src/Workflow/ParamRequest.php diff --git a/src/Workflow/AiStep.php b/src/Workflow/AiStep.php index 608b3dd..732724d 100644 --- a/src/Workflow/AiStep.php +++ b/src/Workflow/AiStep.php @@ -17,15 +17,18 @@ final readonly class AiStep { /** - * @param ?list $tools null = every tool (default); a list = only those; [] = none — the same - * contract {@see WorkflowAbstract::ai()} already used - * @param ?string $agent a named agent role (worker/reviewer/planner/…) whose model runs this - * exchange, or null for the run's default + * @param ?list $tools null = every tool (default); a list = only those; [] = none — the + * same contract {@see WorkflowAbstract::ai()} already used + * @param ?string $agent a named agent role (worker/reviewer/planner/…) whose model runs + * this exchange, or null for the run's default + * @param list $params machine-readable values a later CODE step needs — extracted from + * this exchange's accepted answer and delivered via setParam() */ public function __construct( public string $prompt, public ?array $tools = null, public ?string $agent = null, + public array $params = [], ) { } } diff --git a/src/Workflow/ParamRequest.php b/src/Workflow/ParamRequest.php new file mode 100644 index 0000000..145d907 --- /dev/null +++ b/src/Workflow/ParamRequest.php @@ -0,0 +1,29 @@ +formPendingHandoff(); // form the PREVIOUS step's handoff before this one's exchange runs $workHistory = $this->reviewedExchange($name, $declaration, $rubric, $maxRounds); + $this->extractParams($declaration, $workHistory); // hand any declared machine-readable values to later steps } finally { $this->critique = null; $this->currentStep = $previousStep; @@ -559,6 +568,39 @@ private function reviewedExchange(string $name, AiStep $step, ?string $rubric, i return $workHistory; } + /** + * Deliver a {@see StepAI}'s machine-readable outputs. For each declared {@see ParamRequest}, CONTINUE + * the accepted work conversation with a dedicated request for exactly that value and pin it with + * {@see setParam()} for the target step — the same mechanism {@see formPendingHandoff()} uses for the + * prose handoff, addressed to a named step instead of the next one automatically. Runs under + * {@see $extracting} so its own turns are not checkpointed into the step's exchange row. + * + * @param list $workHistory + */ + private function extractParams(AiStep $step, array $workHistory): void + { + if ($step->params === [] || $workHistory === []) { + return; + } + + $this->extracting = true; + + try { + foreach ($step->params as $request) { + $value = trim($this->runTurns( + "Before this step ends, answer EXACTLY this and nothing else: {$request->instruction}\n\n" + . 'Reply with only the value — no prose, no explanation, no quotes.', + [], + 'extract', + $workHistory, + )); + $this->setParam($request->forStep, $request->name, $value); + } + } finally { + $this->extracting = false; + } + } + /** * Run — or continue — the one exchange a {@see StepAI} declares. What the store already holds for this * (run, step) decides which: @@ -1132,8 +1174,8 @@ private function makeTurnLoop(Environment $scope, string $system, ?SpeakerInterf // has no idea what a step is; it only knows a turn has landed. /** @param list $history */ function (array $history): void { - if ($this->reviewing) { - return; // a critic's conversation is not the step's; see $reviewing + if ($this->reviewing || $this->extracting) { + return; // a critic's or an extraction's conversation is not the step's work } $this->env->findStore()->saveExchange($this->runId, $this->currentStep, $history); diff --git a/tests/Workflow/WorkflowAbstractTest.php b/tests/Workflow/WorkflowAbstractTest.php index 8ddd7d6..333507b 100644 --- a/tests/Workflow/WorkflowAbstractTest.php +++ b/tests/Workflow/WorkflowAbstractTest.php @@ -32,6 +32,7 @@ use Claw\Workflow\Environment; use Claw\Workflow\EnvKey; use Claw\Workflow\InMemoryStateStore; +use Claw\Workflow\ParamRequest; use Claw\Workflow\Step; use Claw\Workflow\StepAI; use Claw\Workflow\Tool; @@ -365,6 +366,46 @@ protected function work(): AiStep Assert::true(\count($worker->requests) >= 4); // attempt, critic, re-run, critic — a re-run happened } + #[Test] + public function anAiStepExtractsAParamForALaterCodeStep(): void + { + // A #[StepAI] hands a machine-readable value to a later CODE step: after the work settles the base + // asks the model for exactly that value and pins it; the code step reads it with param(). + $worker = new ScriptedAgent( + $this->answer('I read the ticket; the change is localized'), // the work exchange + $this->answer('simple'), // the extraction: only the value + ); + $store = new InMemoryStateStore(); + $wf = new class ($this->config(worker: $worker, store: $store), 'r1') extends WorkflowAbstract { + public string $seen = ''; + + public function name(): string + { + return 'ai'; + } + + #[StepAI] + protected function assess(): AiStep + { + return new AiStep('assess the size of the change', params: [ + new ParamRequest(forStep: 'route', name: 'size', instruction: 'One word: simple or complex.'), + ]); + } + + #[Step] + protected function route(): void + { + $this->seen = (string) $this->param('size'); + } + }; + + $wf->run(); + + Assert::same($wf->seen, 'simple'); // the code step read the extracted value + Assert::same($store->load('r1')['done'], ['assess', 'route']); // both steps ran, in order + Assert::count($worker->requests, 2); // the work exchange plus one extraction call + } + #[Test] public function toolResolvesThroughTheRegistryAndReturnsItsContent(): void { From 8c21ac930a869f03c6dfb908418d1b907b6ad355 Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:28:38 +0000 Subject: [PATCH 06/16] refactor(workflow): a spent budget stops the run, it never asks the channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reacting to "there are no tokens left" by calling the ask channel spent tokens to ask a model whether to continue — the front tier is the supervisor, a model call — and could not authorise a budget anyway; worse, on a resumed run that call reached the human gate before the parked worker and consumed the human's answer. So enforceBudget now just throws its resumable stop; BudgetPolicy, parseExtraTokens and the CLAW_BUDGET_POLICY knob are gone. Turning a budget stop into a resumable WaitingHuman pause the operator settles out of band is the next step. See DECISIONS 2026-07-25 and design/human-requests.md. --- src/Config.php | 14 -------- src/Run/IssueRunner.php | 2 -- src/Workflow/BudgetPolicy.php | 20 ------------ src/Workflow/EnvKey.php | 1 - src/Workflow/WorkflowAbstract.php | 43 ++++--------------------- tests/Workflow/WorkflowAbstractTest.php | 40 ++++++----------------- 6 files changed, 16 insertions(+), 104 deletions(-) delete mode 100644 src/Workflow/BudgetPolicy.php diff --git a/src/Config.php b/src/Config.php index e8c4aaf..40a1760 100644 --- a/src/Config.php +++ b/src/Config.php @@ -53,10 +53,6 @@ final class Config private const DEFAULT_LIMIT = 0; // 0 = no limit, for every budget cap below - private const DEFAULT_BUDGET_POLICY = 'stop'; - - private const BUDGET_POLICIES = ['stop', 'ask']; - /** * The GLOBAL library of ready-made workflows: the ones offered to every project. A path rather * than a folder inside the app home, because these are written and reviewed by people and belong @@ -89,7 +85,6 @@ private function __construct( public readonly int $budgetSeconds = 0, public readonly int $turnTokens = 0, public readonly int $turnSeconds = 0, - public readonly string $budgetPolicy = self::DEFAULT_BUDGET_POLICY, public readonly string $library = self::DEFAULT_LIBRARY, ) { } @@ -138,14 +133,6 @@ public static function load(string $path = '.env'): self ); } - $budgetPolicy = strtolower($get('CLAW_BUDGET_POLICY') ?? self::DEFAULT_BUDGET_POLICY); - - if (!\in_array($budgetPolicy, self::BUDGET_POLICIES, true)) { - throw new ConfigException( - "Unknown CLAW_BUDGET_POLICY '{$budgetPolicy}', expected one of: " . implode(', ', self::BUDGET_POLICIES) - ); - } - $keyVar = self::API_KEY_VARS[$agent]; $apiKey = $get($keyVar) ?? $get('CLAW_API_KEY'); @@ -187,7 +174,6 @@ public static function load(string $path = '.env'): self turnTokens: (int) ($get('CLAW_TURN_TOKENS') ?? self::DEFAULT_LIMIT), turnSeconds: (int) ($get('CLAW_TURN_SECONDS') ?? self::DEFAULT_LIMIT), library: $get('CLAW_LIBRARY') ?? \dirname(__DIR__) . '/' . self::DEFAULT_LIBRARY, - budgetPolicy: $budgetPolicy, ); } diff --git a/src/Run/IssueRunner.php b/src/Run/IssueRunner.php index ae53d81..9d5c920 100644 --- a/src/Run/IssueRunner.php +++ b/src/Run/IssueRunner.php @@ -32,7 +32,6 @@ use Claw\Trace\Level; use Claw\Trace\Tracer; use Claw\Trace\TraceReader; -use Claw\Workflow\BudgetPolicy; use Claw\Workflow\Environment; use Claw\Workflow\EnvKey; use Claw\Workflow\GenerateIssueWorkflow; @@ -214,7 +213,6 @@ public function run(Issue $issue): int ->set(EnvKey::Budget, new Budget($this->treeAllowance($issue), (float) $this->config->budgetSeconds)) ->set(EnvKey::TurnTokenLimit, $this->config->turnTokens) ->set(EnvKey::TurnTimeLimit, (float) $this->config->turnSeconds) - ->set(EnvKey::BudgetPolicy, BudgetPolicy::from($this->config->budgetPolicy)) // The same cap the chat path has always put on a tool run, finally on the path that needs it // more: here nobody is watching, so a `bash` waiting forever on a prompt nobody will type // holds the run open with no clock ticking against it. diff --git a/src/Workflow/BudgetPolicy.php b/src/Workflow/BudgetPolicy.php deleted file mode 100644 index 907a22c..0000000 --- a/src/Workflow/BudgetPolicy.php +++ /dev/null @@ -1,20 +0,0 @@ -budgetPolicy() === BudgetPolicy::Ask) { - $channel = $this->env->find(EnvKey::Ask); - - if ($channel instanceof SpeakerInterface) { - $extra = $this->parseExtraTokens($channel->reply( - "Budget spent: {$budget->reason()}. Enter extra tokens to continue, or nothing to stop.", - )); - - if ($extra > 0) { - $budget->raise($extra); - $this->tracer()?->log('budget', "raised by {$extra} tokens", [], Level::Notice); - - return; - } - } - } - throw WorkflowException::stopped('run stopped: ' . $budget->reason()); } - /** The configured reaction to a spent run total — {@see BudgetPolicy::Stop} when unset. */ - private function budgetPolicy(): BudgetPolicy - { - $policy = $this->env->find(EnvKey::BudgetPolicy); - - return $policy instanceof BudgetPolicy ? $policy : BudgetPolicy::Stop; - } - - /** A positive token top-up parsed from an ask answer (e.g. "+100000"), or 0 to stop. */ - private function parseExtraTokens(?string $answer): int - { - $digits = ltrim(trim((string) $answer), '+'); - - return $digits !== '' && ctype_digit($digits) ? (int) $digits : 0; - } - /** Read a numeric environment value (a budget cap), or 0.0 when unset/non-numeric. */ private function numEnv(EnvKey $key): float { diff --git a/tests/Workflow/WorkflowAbstractTest.php b/tests/Workflow/WorkflowAbstractTest.php index 333507b..5960130 100644 --- a/tests/Workflow/WorkflowAbstractTest.php +++ b/tests/Workflow/WorkflowAbstractTest.php @@ -28,7 +28,6 @@ use Claw\Trace\Tracer; use Claw\Trace\TraceRecordInterface; use Claw\Workflow\AiStep; -use Claw\Workflow\BudgetPolicy; use Claw\Workflow\Environment; use Claw\Workflow\EnvKey; use Claw\Workflow\InMemoryStateStore; @@ -673,38 +672,17 @@ public function stepThrowsWhenTheRunBudgetIsSpent(): void } #[Test] - public function askPolicyRaisesTheBudgetAndContinuesOnATopUp(): void + public function anExhaustedBudgetStopsAndNeverAsksTheChannel(): void { + // Budget is not an in-run question: reacting to "no tokens" by spending tokens to ask a model is a + // contradiction, and on a resumed run that ask could eat a person's answer meant for the worker. + // So a spent budget just stops — resumably — and the ask channel, even when present, is untouched. $budget = new Budget(tokenLimit: 10); $budget->spend(10); // already exhausted $channel = new class () implements SpeakerInterface { - public function name(): SpeakerRole - { - return SpeakerRole::Human; - } + public bool $asked = false; - public function reply(string $incoming): string - { - return '+100'; // grant 100 more tokens - } - }; - $env = $this->config(worker: new ScriptedAgent($this->answer('done'))) - ->set(EnvKey::Budget, $budget) - ->set(EnvKey::BudgetPolicy, BudgetPolicy::Ask) - ->set(EnvKey::Ask, $channel); - $wf = new ProbeWorkflow($env, 'r1'); - - Assert::same($wf->callAi('hi'), 'done'); // topped up, so the call proceeds - } - - #[Test] - public function askPolicyStopsWhenNoTopUpIsGiven(): void - { - $budget = new Budget(tokenLimit: 10); - $budget->spend(10); - - $channel = new class () implements SpeakerInterface { public function name(): SpeakerRole { return SpeakerRole::Human; @@ -712,12 +690,13 @@ public function name(): SpeakerRole public function reply(string $incoming): string { - return ''; // decline -> stop + $this->asked = true; + + return '+100'; // a top-up the run must NOT honour any more } }; $env = $this->config() ->set(EnvKey::Budget, $budget) - ->set(EnvKey::BudgetPolicy, BudgetPolicy::Ask) ->set(EnvKey::Ask, $channel); $wf = new ProbeWorkflow($env, 'r1'); @@ -729,7 +708,8 @@ public function reply(string $incoming): string $threw = true; } - Assert::true($threw); + Assert::true($threw); // an exhausted budget stops the run + Assert::false($channel->asked); // and it did NOT consult the ask channel } #[Test] From 3806805114335dac1db9e0629ad0ca53f819a4d3 Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:44:20 +0000 Subject: [PATCH 07/16] feat(run): a spent budget pauses the ticket for a person, it does not fail the run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A budget stop is marked (WorkflowException::budgetSpent) so the run path tells it from the other deliberate stops. On it the ticket goes to WaitingHuman and the run is left resumable — its status untouched — so raising the limit and running the issue again picks it up where it stopped, rather than failing the run and handing it back to triage, which would throw away work the budget only interrupted. Both the solver and direct paths honour it. Advances design/human-requests.md (budget as a request); the visible typed request + UI is still to come. --- src/Exceptions/WorkflowException.php | 18 ++++++- src/Run/IssueRunner.php | 46 ++++++++++++++--- src/Workflow/WorkflowAbstract.php | 2 +- tests/Run/IssueRunnerTest.php | 65 +++++++++++++++++++++++++ tests/Workflow/WorkflowAbstractTest.php | 8 +-- 5 files changed, 124 insertions(+), 15 deletions(-) diff --git a/src/Exceptions/WorkflowException.php b/src/Exceptions/WorkflowException.php index a954e69..c19ebd2 100644 --- a/src/Exceptions/WorkflowException.php +++ b/src/Exceptions/WorkflowException.php @@ -20,8 +20,11 @@ */ final class WorkflowException extends ClawException { - public function __construct(string $message, public readonly bool $deliberate = false) - { + public function __construct( + string $message, + public readonly bool $deliberate = false, + public readonly bool $budget = false, + ) { parent::__construct($message); } @@ -30,4 +33,15 @@ public static function stopped(string $message): self { return new self($message, deliberate: true); } + + /** + * A run halted because its TOTAL budget is spent — a deliberate, RESUMABLE stop that is not a failure. + * The run path turns it into a WaitingHuman pause the operator settles by raising the limit, rather + * than a failed run handed back to triage. {@see $budget} tells it apart from the other deliberate + * stops (a supervisor `stop`, review rounds exhausted), which are genuine dead ends for the ticket. + */ + public static function budgetSpent(string $message): self + { + return new self($message, deliberate: true, budget: true); + } } diff --git a/src/Run/IssueRunner.php b/src/Run/IssueRunner.php index 9d5c920..d3d5798 100644 --- a/src/Run/IssueRunner.php +++ b/src/Run/IssueRunner.php @@ -409,6 +409,11 @@ private function runDirect(RunContext $ctx): int throw $cancellation; } catch (\Throwable $e) { $ctx->tracer->exit($span); + + if ($e instanceof WorkflowException && $e->budget) { + return $this->pauseForBudget($ctx, $e->getMessage()); // paused on budget, not failed + } + $this->failRun($ctx, "run #{$ctx->runId} failed: {$e->getMessage()}"); $this->giveBackToProjectManager($ctx->issue, "the direct attempt failed: {$e->getMessage()}", $ctx->runId); @@ -550,6 +555,24 @@ private function failRun(RunContext $ctx, string $reason): void $this->frontend->report($reason, true); } + /** + * A run whose budget ran out is PAUSED, not failed: the ticket goes to WaitingHuman and the run is + * left RESUMABLE — its status is untouched, so raising the limit and running the issue again picks it + * up where it stopped. Handing it back to triage as a failure would throw away work the budget only + * interrupted. Returns 0: a pause is not an error. + */ + private function pauseForBudget(RunContext $ctx, string $reason): int + { + $ctx->store->setIssueStatus($ctx->issue->id, IssueStatus::WaitingHuman); + $ctx->tracer->log('run-paused', "{$reason} — raise the budget and run issue #{$ctx->issue->id} again", [], Level::Notice); + $this->frontend->report( + "Run #{$ctx->runId} paused: {$reason}. Raise the budget and run issue #{$ctx->issue->id} again to resume.", + false, + ); + + return 0; + } + private function giveBackToProjectManager(Issue $issue, string $reason, string $runId): void { // FIRST, stop the issue claiming to be in progress. The run set it to InProgress at the start @@ -758,6 +781,15 @@ private function runSolver(RunContext $ctx, bool $repairable = true): int } catch (\Cancellation $cancellation) { throw $cancellation; // a cancelled run must stop — never "repair" a cancellation } catch (\Throwable $e) { + // A budget stop is a PAUSE, not a failure: the run is left resumable and the ticket waits + // on a person to raise the limit, not on triage. Caught before the deliberate-stop branch + // below, which would otherwise fail the run and hand it back. + if ($e instanceof WorkflowException && $e->budget) { + $ctx->tracer->exit($solverSpan); + + return $this->pauseForBudget($ctx, $e->getMessage()); + } + // Repair answers ONE question: is this workflow's code broken? Three kinds of failure // arrive here and only one of them is that. // @@ -767,14 +799,12 @@ private function runSolver(RunContext $ctx, bool $repairable = true): int // then runs that invention. Measured: a 400 from a malformed history had the supervisor // rewrite a workflow whose source it could not even read. // - // A DELIBERATE stop is the run doing what it was told: the budget ran out, the supervisor - // said `stop`, a step exhausted its review rounds with nobody to escalate to. The code is - // not merely innocent here, it is being repaired AGAINST a decision that was taken on - // purpose — the supervisor was sent to rewrite the class it had itself just stopped, and - // the cheapest way to satisfy "fix the cause of: run stopped by the supervisor" is to - // delete the critic that stopped it. A budget stop was worse still: the repair's own - // model call hit the same spent budget, so the ticket came back reading "the solver - // crashed and could not be repaired: run stopped: budget spent". + // A DELIBERATE stop is the run doing what it was told: the supervisor said `stop`, or a + // step exhausted its review rounds with nobody to escalate to. (A budget stop is handled + // just above, as a PAUSE, not here.) The code is not merely innocent, it is being repaired + // AGAINST a decision taken on purpose — the supervisor sent to rewrite the class it had + // itself just stopped, and the cheapest way to satisfy "fix the cause of: run stopped by + // the supervisor" is to delete the critic that stopped it. // // Everything else — TypeError, ParseError, a step calling a method that is not there — // really can mean the generated code is broken, which is what repair is for. diff --git a/src/Workflow/WorkflowAbstract.php b/src/Workflow/WorkflowAbstract.php index 4772b86..09c43ba 100644 --- a/src/Workflow/WorkflowAbstract.php +++ b/src/Workflow/WorkflowAbstract.php @@ -1355,7 +1355,7 @@ private function enforceBudget(): void return; } - throw WorkflowException::stopped('run stopped: ' . $budget->reason()); + throw WorkflowException::budgetSpent('run stopped: ' . $budget->reason()); } /** Read a numeric environment value (a budget cap), or 0.0 when unset/non-numeric. */ diff --git a/tests/Run/IssueRunnerTest.php b/tests/Run/IssueRunnerTest.php index 39d404b..4ec8f0a 100644 --- a/tests/Run/IssueRunnerTest.php +++ b/tests/Run/IssueRunnerTest.php @@ -255,6 +255,40 @@ public function aDeliberateStopIsNotTreatedAsBrokenCodeAndSoIsNotRewritten(): vo } } + #[Test] + public function aBudgetStopPausesTheRunAsWaitingHumanRatherThanFailingIt(): void + { + $projectsDir = self::tempDir(); + $projectFolder = self::tempDir(); + + try { + $store = self::registerProject($projectsDir, $projectFolder); + + for ($i = 0; $i < 20; $i++) { + $store->addIssue("filler {$i}"); + } + $issue = $store->addIssue('a task whose run runs out of budget'); + + $workflows = WorkflowStore::solvers($projectsDir, $store->project()->id); + $solver = self::solverName($issue->id); + $workflows->write($solver, self::pausingSolverCode($workflows->namespaceFor(true), $solver), true); + + $frontend = new RecordingRunFrontend(); + $runner = new IssueRunner($projectsDir, $store, self::config($projectsDir), new ScriptedAgent(), $frontend); + + Assert::same($runner->run($issue), 0); // a pause is not a failure + + // The ticket waits on a person to raise the budget; the run is left resumable, not rewritten, + // and not handed back to triage. + Assert::same($store->loadIssue($issue->id)->status, IssueStatus::WaitingHuman); + Assert::false(is_file($workflows->path($solver . 'R1', true))); + Assert::false($frontend->reported('repairing')); + } finally { + self::rmrf($projectsDir); + self::rmrf($projectFolder); + } + } + #[Test] public function theDirectPathNeedsAVerdictItCannotGiveItself(): void { @@ -455,6 +489,37 @@ public function implement(): void PHP; } + /** A solver whose step halts because the budget is spent — a PAUSE, not a failure. */ + private static function pausingSolverCode(string $namespace, string $class): string + { + return <<set(EnvKey::Ask, $channel); $wf = new ProbeWorkflow($env, 'r1'); - $threw = false; + $marked = false; try { $wf->callAi('hi'); - } catch (WorkflowException) { - $threw = true; + } catch (WorkflowException $e) { + $marked = $e->budget; // tagged a budget stop, so the run path can PAUSE the ticket, not fail it } - Assert::true($threw); // an exhausted budget stops the run + Assert::true($marked); // an exhausted budget stops the run, marked as a budget stop Assert::false($channel->asked); // and it did NOT consult the ask channel } From 64db0288f78e2933e723dcde4649379d1933df8e Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 10:56:01 +0000 Subject: [PATCH 08/16] refactor(workflow): quality pass over the resumable-step changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a comment-and-quality review of the branch: the attempt fingerprint is one private helper instead of two copies, so both step kinds decide "the same" the same way; isAiStep() defers to aiAttribute() rather than reflecting a second time; and the stale comments are corrected — the turn loop no longer claims a single checkpoint site, and the class and DEFAULT_MAX_ROUNDS docs name the #[StepAI] kind alongside #[Step]. --- src/Agent/DefaultTurnLoop.php | 6 +++--- src/Workflow/WorkflowAbstract.php | 31 ++++++++++++++++++++----------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/Agent/DefaultTurnLoop.php b/src/Agent/DefaultTurnLoop.php index 7869a77..620c3c5 100644 --- a/src/Agent/DefaultTurnLoop.php +++ b/src/Agent/DefaultTurnLoop.php @@ -334,9 +334,9 @@ public function run(array $history): TurnResult $history[] = new Message(Role::User, $results); $this->tracer?->exit($turn); - // A turn has landed and the history is whole: every tool_use answered. This is the only - // instant it is safe to write down, which is why the loop offers it rather than leaving the - // caller to guess when to snapshot. + // A turn has landed and the history is whole: every tool_use answered. One of the two instants + // it is safe to write the exchange down — the other is the [question] park above, which + // checkpoints before it blocks so a crash while waiting can resume from the question. if ($this->checkpoint !== null) { ($this->checkpoint)($history); } diff --git a/src/Workflow/WorkflowAbstract.php b/src/Workflow/WorkflowAbstract.php index 09c43ba..86d43c5 100644 --- a/src/Workflow/WorkflowAbstract.php +++ b/src/Workflow/WorkflowAbstract.php @@ -37,9 +37,10 @@ * - {@see run()} is just the entry point — by default it drives the step methods in order, but * the author may override it and orchestrate by hand (plain if/while), calling step() as needed. * - * A critic, though, IS machinery here: a step can declare {@see Step::$critic}, and the driver judges - * the step's RESULT (the method's return value) against it on the reviewer role; while it falls short, - * the supervisor (the ask channel) guides a re-run — a declarative aspect, not a hand-written sub-step. + * A critic, though, IS machinery here: a step — imperative {@see Step} or declarative {@see StepAI} — + * can declare one, and the driver judges the step's recorded artifacts against it on the reviewer role; + * while it falls short, the supervisor (the ask channel) guides a re-run — a declarative aspect, not a + * hand-written sub-step. * * The critic is a gate the step cannot open from the inside, and there is no longer any way for a step * to open it from the inside: a worker does its work and returns, and whether the run ends is decided @@ -54,8 +55,8 @@ abstract class WorkflowAbstract implements WorkflowInterface * a step and let it fix itself once or twice; if two rounds do not close the findings, the problem is * usually a mismatch (the step's prompt vs the critic's rubric) or a task that truly needs a human, not * "one more try" — so we escalate rather than churn dozens of rounds burning tokens. A step that - * legitimately churns (e.g. a test gate) raises it per case via `#[Step(maxRounds: N)]`. A checkpoint, - * not a hard kill; the budget is still the ultimate backstop. + * legitimately churns (e.g. a test gate) raises it per case via `#[Step(maxRounds: N)]` (or + * `#[StepAI(maxRounds: N)]`). A checkpoint, not a hard kill; the budget is still the ultimate backstop. */ private const int DEFAULT_MAX_ROUNDS = 2; @@ -386,10 +387,7 @@ protected function step(string $name): void // supervisor the fact instead and let it settle the round (accept, redirect, or stop). // Byte-exact on purpose: evidence that legitimately varies (timings) simply never matches, // and the guard stays out of the way. - $attempt = array_map( - static fn (Artifact $a): array => [$a->label, $a->kind, $a->value], - $this->artifacts[$name], - ); + $attempt = $this->attemptFingerprint($name); $verdict = $this->verdictFor( $name, @@ -438,7 +436,7 @@ protected function step(string $name): void /** Whether $name is an AI step (marked {@see StepAI}) — read by reflection, without running the body. */ private function isAiStep(string $name): bool { - return new \ReflectionMethod($this, $name)->getAttributes(StepAI::class) !== []; + return $this->aiAttribute($name) !== null; } /** @@ -541,7 +539,7 @@ private function reviewedExchange(string $name, AiStep $step, ?string $rubric, i } $artifacts = $this->renderArtifacts($this->artifacts[$name]); - $attempt = array_map(static fn (Artifact $a): array => [$a->label, $a->kind, $a->value], $this->artifacts[$name]); + $attempt = $this->attemptFingerprint($name); $verdict = $this->verdictFor( $name, $rubric, @@ -1400,6 +1398,17 @@ private function maxRounds(?int $max): int return $max !== null && $max > 0 ? $max : self::DEFAULT_MAX_ROUNDS; } + /** + * A step attempt's fingerprint — its artifacts as [label, kind, value] triples — for the + * byte-identical-rerun guard. One definition so both step kinds decide "the same" the same way. + * + * @return list + */ + private function attemptFingerprint(string $name): array + { + return array_map(static fn (Artifact $a): array => [$a->label, $a->kind, $a->value], $this->artifacts[$name]); + } + /** * The verdict on one attempt: the two deterministic guards first — a step that produced nothing, or a * re-run byte-identical to the last, neither worth an AI critic's round — else the AI critic. Shared by From 4822c62944f50da8c0415b5b1179ca19cb4905eb Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:36:11 +0000 Subject: [PATCH 09/16] fix(workflow): a back() into a #[StepAI] step carries its reason as guidance --- src/Workflow/WorkflowAbstract.php | 7 +++-- tests/Workflow/WorkflowAbstractTest.php | 40 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/src/Workflow/WorkflowAbstract.php b/src/Workflow/WorkflowAbstract.php index 86d43c5..9b56323 100644 --- a/src/Workflow/WorkflowAbstract.php +++ b/src/Workflow/WorkflowAbstract.php @@ -462,9 +462,12 @@ private function runAiStep(string $name): void $workHistory = []; // A back() into this step re-enters it fresh (its exchange was cleared when it finished); continuing - // the prior attempt is a refinement the imperative path has and this one will grow. Clear the arming - // so it does not leak to a later step. + // the prior attempt is a refinement the imperative path has and this one will grow. The reason is + // carried in as this re-entry's guidance so the pure body can fold it into the AiStep it declares + // (via critique()) — the same contract back() documents and the imperative path already honours; + // without this the reason was silently dropped. Clear the arming so it does not leak to a later step. if ($this->reentryStep === $name) { + $this->critique = $this->reentryReason; $this->reentryStep = null; $this->reentryReason = ''; } diff --git a/tests/Workflow/WorkflowAbstractTest.php b/tests/Workflow/WorkflowAbstractTest.php index f237d55..165ed83 100644 --- a/tests/Workflow/WorkflowAbstractTest.php +++ b/tests/Workflow/WorkflowAbstractTest.php @@ -405,6 +405,46 @@ protected function route(): void Assert::count($worker->requests, 2); // the work exchange plus one extraction call } + #[Test] + public function aBackIntoAnAiStepCarriesItsReasonAsGuidance(): void + { + // back() into a #[StepAI] must hand the step its reason: the pure body folds critique() into the + // AiStep it declares, exactly as the imperative path does. Without it the reason was dropped and the + // re-run repeated the first prompt — the contract back() documents, unhonoured on the declared path. + $worker = new ScriptedAgent($this->answer('first'), $this->answer('second')); + $store = new InMemoryStateStore(); + $wf = new class ($this->config(worker: $worker, store: $store), 'r1') extends WorkflowAbstract { + private bool $sentBack = false; + + public function name(): string + { + return 'ai'; + } + + #[StepAI] + protected function work(): AiStep + { + return new AiStep($this->critique() ?? 'do the work'); + } + + #[Step] + protected function gate(): void + { + if ($this->sentBack) { + return; + } + + $this->sentBack = true; + $this->back('work', 'try the other approach'); + } + }; + + $wf->run(); + + Assert::count($worker->requests, 2); // work ran twice: first + the re-run + Assert::same($this->lastUserText($worker), 'try the other approach'); // the back reason drove the re-run + } + #[Test] public function toolResolvesThroughTheRegistryAndReturnsItsContent(): void { From d082aeda89d76abd5a7fba51d91ecf1fd8cfae6d Mon Sep 17 00:00:00 2001 From: Edmond <1571649+edmonddantes@users.noreply.github.com> Date: Sat, 25 Jul 2026 12:56:12 +0000 Subject: [PATCH 10/16] refactor(workflow): the generator drives its own steps declaratively (#[StepAI]) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit understand/assess/draft/save move off the imperative ai() path onto #[StepAI]+#[Step]. State flows on the engine's channels, not fields: the plan and difficulty reach draft as addressed params; draft's model records the solver via the artifact tool (a pure #[StepAI] cannot call artifact()) and hands the source to save as a code param; save runs define_workflow and, on a validator reject, re-drafts once via back() instead of an ai() revise callback. Coexists with the old path — generated solvers and every other workflow are untouched. --- src/Workflow/GenerateIssueWorkflow.php | 176 ++++++++++--------- tests/Workflow/GenerateIssueWorkflowTest.php | 67 ++++--- 2 files changed, 141 insertions(+), 102 deletions(-) diff --git a/src/Workflow/GenerateIssueWorkflow.php b/src/Workflow/GenerateIssueWorkflow.php index 6cb315d..866beb3 100644 --- a/src/Workflow/GenerateIssueWorkflow.php +++ b/src/Workflow/GenerateIssueWorkflow.php @@ -4,6 +4,7 @@ namespace Claw\Workflow; +use Claw\Exceptions\WorkflowException; use Claw\Tool\Registry; /** @@ -74,22 +75,23 @@ final class GenerateIssueWorkflow extends WorkflowAbstract isolates. This is the concrete test for "is this step worth it". RECIPE; - private string $plan = ''; - private string $code = ''; - - /** The worker tier assess() decided this task warrants — folded into the draft's step routing. */ - private string $workerTier = 'worker'; - private string $difficulty = 'moderate'; + /** One bounded repair: on a validator reject at save, back() re-drafts once, then a second failure throws. */ + private bool $repairAttempted = false; public function name(): string { return 'generate-issue-workflow'; } - #[Step] - protected function understand(): void + /** + * A pure {@see StepAI}: it DECLARES the planning exchange, the base runs it. The plan reaches + * {@see draft()} as an addressed `plan` param (and {@see assess()}, the next step, through the handoff) + * — the declarative model carries nothing in fields, so what a later step needs is passed on a channel. + */ + #[StepAI] + protected function understand(): AiStep { - $this->plan = $this->ai( + return new AiStep( 'You are planning how to solve a task by writing a workflow. Inspect the project if it ' . 'helps (read_file, list_files), then, in a few concrete sentences: outline the steps a ' . 'workflow should take to solve this task, AND assess whether the project is mature with ' @@ -97,74 +99,93 @@ protected function understand(): void . "whether a human must approve the design before it is implemented:\n\n" . $this->taskSummary(), ['read_file', 'list_files'], 'worker-smart', // planning a whole workflow is heavy thinking — use the strong tier + params: [new ParamRequest( + forStep: 'draft', + name: 'plan', + instruction: 'Restate your plan as a few concrete sentences: the steps a workflow should take ' + . 'to solve this task, and whether a human must approve the design before it is implemented.', + )], ); } /** - * Judge how hard the task is and pick the model tier the GENERATED solver should run its steps - * on: a trivial fix wastes money on the strong model, a subtle change needs it. The verdict - * ('worker' vs 'worker-smart') is folded into the draft, so the solver routes its `ai()` calls to - * the chosen tier. Kept as its own step so the decision — and its reasoning — is visible in the trace. + * Judge how hard the task is; the one-word verdict reaches {@see draft()} as an addressed `difficulty` + * param, which routes both the drafting tier and the tier the GENERATED solver runs its own steps on — + * a trivial fix wastes money on the strong model, a subtle change needs it. Its own step so the decision + * and its reasoning stay visible in the trace; the plan it judges against arrives through the handoff. + * + * The verdict is a param, not a parsed first line: extraction asks for exactly the bare word, so the + * reasoning sentence can never be mistaken for it. {@see draftPrompt()} maps anything unreadable to + * `moderate` — the middle, which cannot be wrong in an expensive direction. */ - #[Step] - protected function assess(): void + #[StepAI] + protected function assess(): AiStep { - $verdict = $this->ai( - 'Rate how hard this coding task is for an AI to solve correctly. Your FIRST LINE must be ' - . 'one bare word and nothing else — simple, moderate or complex. No label, no punctuation, ' - . 'no backticks: a first line reading "Complexity: simple" is not one of those words and ' - . "will be read as moderate. Put one sentence of reasoning on the line after it.\n\n" - . 'Simple = a localized, mechanical change; complex = subtle logic, wide blast radius, or ' - . "design judgement.\n\nTask:\n{$this->taskSummary()}\n\nPlan:\n{$this->plan}", + return new AiStep( + 'Rate how hard this coding task is for an AI to solve correctly — simple, moderate or complex — ' + . 'and give one sentence of reasoning. Simple = a localized, mechanical change; complex = subtle ' + . "logic, wide blast radius, or design judgement.\n\nTask:\n{$this->taskSummary()}", [], 'supervisor-smart', + params: [new ParamRequest( + forStep: 'draft', + name: 'difficulty', + instruction: 'One bare word and nothing else — simple, moderate or complex. ' + . 'No label, no punctuation, no backticks.', + )], ); - - // Classify on the FIRST LINE only — the one-word verdict — not the whole reply: the reasoning - // sentence routinely names the other tiers ("not a simple change"), which would misclassify. - // - // The match is EXACT. It used to be `str_contains($word, 'complex')` tested before 'simple', so - // the entirely ordinary reply "Complexity: simple" tokenized to "complexity:", contained - // "complex", and routed a trivial task to the expensive model — the verdict inverted by the very - // check written to protect it. Anything unreadable now lands on `moderate`, which is the middle - // and cannot be wrong in an expensive direction. - $word = strtolower(trim((string) strtok(trim($verdict), "\r\n"), " \t`*_.:—–-")); - $this->difficulty = match ($word) { - 'simple', 'complex' => $word, - default => 'moderate', - }; - - // A simple task runs cheap; anything with real judgement gets the strong tier. - $this->workerTier = $this->difficulty === 'simple' ? 'worker' : 'worker-smart'; } /** - * Write the solver, then have it reviewed by the `solverReview` critic — "will it actually solve the - * task", not "is it valid PHP" (the validator covers that). The critic gates the step, so a rejected - * draft RE-RUNS here (continuing this conversation, see {@see WorkflowAbstract::ai()}) and is re-judged - * — the worker's fix can't slip through unreviewed, which is how a bad draft used to escape. + * Declare the drafting exchange under the `solverReview` critic — "will it actually solve the task", + * not "is it valid PHP" (the validator covers that). A pure {@see StepAI} cannot record an artifact + * itself, so the model RECORDS the solver source by calling the `artifact` tool; the critic judges that + * artifact, and the source reaches {@see save()} as an addressed `code` param. On a critic reject the + * base re-runs the exchange on the supervisor's guidance, so a bad draft cannot slip through unreviewed. */ - #[Step(critic: 'solverReview')] - protected function draft(): string + #[StepAI(critic: 'solverReview')] + protected function draft(): AiStep { - // [] = the model returns the class CODE, it does not act with tools - $this->code = $this->extractCode($this->ai($this->draftPrompt(), [], 'worker-smart')); - - // The generated class IS this step's output — record it as the artifact the critic judges. (A - // codegen step produces no run artifacts, and that is correct; the artifact is the source itself.) - $this->artifact('solver-class', $this->code, type: 'solver'); - - return $this->code; // a rejection re-runs draft with the findings + return new AiStep( + $this->draftPrompt(), + ['artifact'], // the only move the drafter needs: record the source it writes + 'worker-smart', + params: [new ParamRequest( + forStep: 'save', + name: 'code', + instruction: 'Output the complete PHP source of the solver class you recorded with the ' + . 'artifact tool — exactly and nothing else, no prose and no markdown fences.', + )], + ); } + /** + * Save the drafted solver through `define_workflow` (which validates, then stores it). On a validator + * reject the source is re-drafted ONCE: back() into {@see draft()} hands it the complaint via critique() + * and it rewrites; a second reject surfaces to the run path. A CODE step, so it never calls the model + * itself — the repair is a real re-draft, not a hidden ai() call inside save. + */ #[Step] protected function save(): void { - $this->code = $this->saveGeneratedWorkflow( - (string) $this->param('solverName'), - $this->code, - fn (string $rejection): string => $this->reviseCode("The class you wrote was rejected: {$rejection}"), - ); + $code = $this->extractCode((string) $this->param('code')); + $result = $this->tool('define_workflow', [ + 'name' => (string) $this->param('solverName'), + 'code' => $code, + 'shared' => true, + ]); + + if (str_contains($result, self::WORKFLOW_SAVED_MARKER)) { + return; + } + + if ($this->repairAttempted) { + throw new WorkflowException($result); // a second failure surfaces to the run path + } + + $this->repairAttempted = true; + $this->back('draft', "The generated class was rejected when it was saved: {$result}\n\n" + . 'Rewrite the FULL class fixing exactly that, and record it again with the artifact tool.'); } /** @@ -176,6 +197,7 @@ protected function save(): void protected function criticRules(): array { $recipe = self::RECIPE; + $plan = (string) $this->param('plan'); return [ 'solverReview' => "You are reviewing the GENERATED SOURCE of a solver class (the step's artifact). " @@ -212,7 +234,7 @@ protected function criticRules(): array . "- the plan below describes work this class simply does not do.\n\n" . "Here is what the author was working from, so you judge against the same thing.\n\n" . "The task:\n{$this->taskSummary()}\n\n" - . "The plan:\n{$this->plan}\n\n" + . "The plan:\n{$plan}\n\n" . "The rules it was told to follow when choosing steps:\n{$recipe}", ]; } @@ -235,9 +257,18 @@ private function draftPrompt(): string if ($critique !== null) { return "A reviewer REJECTED the workflow you just wrote:\n\n{$critique}\n\n" - . 'Rewrite the FULL class fixing exactly those problems, keeping the rest. Reply with only the PHP code.'; + . 'Rewrite the FULL class fixing exactly those problems, keeping the rest, and record the ' + . 'corrected source again with the artifact tool.'; } + // The plan and the difficulty arrive as addressed params (understand() and assess() set them); + // an unreadable difficulty maps to `moderate` — the middle, which cannot be wrong in an expensive + // direction — so the drafter and the generated solver are always routed to a real tier. + $plan = (string) $this->param('plan'); + $difficulty = strtolower(trim((string) $this->param('difficulty'))); + $difficulty = \in_array($difficulty, ['simple', 'complex'], true) ? $difficulty : 'moderate'; + $workerTier = $difficulty === 'simple' ? 'worker' : 'worker-smart'; + $namespace = (string) $this->param('solverNamespace'); $class = (string) $this->param('solverName'); $toolDocs = $this->availableTools(); @@ -277,7 +308,7 @@ private function draftPrompt(): string {$task} Plan: - {$this->plan} + {$plan} How to decide the steps — this is the MODEL of how a workflow works and the principle for choosing steps, NOT a list of steps to stamp out (a step is one or more #[Step] methods; use @@ -315,7 +346,7 @@ class name, `extends WorkflowAbstract`, at least one `#[Step]`, that steps are ` - implement `public function name(): string` - keep state in plain typed properties - write each step as a `protected` method marked `#[Step]` (NOT public, NOT private — the base run() drives them and the code is rejected otherwise); the default run() runs them in declaration order - - GRANULARITY — SCALE the number of steps to the task's difficulty (assessed as **{$this->difficulty}**). A SIMPLE/trivial task needs only 1–3 steps — and a SINGLE step (implement-and-verify in one) is perfectly fine for it; do NOT force the full phase-by-phase recipe onto a simple task; the extra steps cost more and add failure surface for no benefit. A MODERATE task wants a handful of focused steps. Reserve the full breakdown (design / review / implement-per-method / test / deliver) for genuinely COMPLEX work. The reason to split AT ALL: each step's `ai()` starts with a fresh, lean context (one fat step re-sends a huge growing history — expensive), and a small step is a unit a critic can check. BUT every step must be a COHERENT unit of REAL work that produces something validatable (an artifact, a passing gate) — never a step that just asks a question or restates a plan. When in doubt, FEWER steps: prefer the smallest decomposition that still lets each piece be verified. Not one giant step, and not a parade of ceremonial ones. + - GRANULARITY — SCALE the number of steps to the task's difficulty (assessed as **{$difficulty}**). A SIMPLE/trivial task needs only 1–3 steps — and a SINGLE step (implement-and-verify in one) is perfectly fine for it; do NOT force the full phase-by-phase recipe onto a simple task; the extra steps cost more and add failure surface for no benefit. A MODERATE task wants a handful of focused steps. Reserve the full breakdown (design / review / implement-per-method / test / deliver) for genuinely COMPLEX work. The reason to split AT ALL: each step's `ai()` starts with a fresh, lean context (one fat step re-sends a huge growing history — expensive), and a small step is a unit a critic can check. BUT every step must be a COHERENT unit of REAL work that produces something validatable (an artifact, a passing gate) — never a step that just asks a question or restates a plan. When in doubt, FEWER steps: prefer the smallest decomposition that still lets each piece be verified. Not one giant step, and not a parade of ceremonial ones. - a step's OUTPUT goes into one of TWO channels — NEVER its return value (the engine ignores what a step returns): (a) `\$this->artifact('