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). diff --git a/src/Agent/DefaultTurnLoop.php b/src/Agent/DefaultTurnLoop.php index 072805f..620c3c5 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 @@ -326,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); } @@ -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/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/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 ae53d81..d3d5798 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. @@ -411,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); @@ -552,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 @@ -760,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. // @@ -769,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/Tool/DefineWorkflowTool.php b/src/Tool/DefineWorkflowTool.php index 7f8513a..2c124ce 100644 --- a/src/Tool/DefineWorkflowTool.php +++ b/src/Tool/DefineWorkflowTool.php @@ -31,8 +31,9 @@ public function description(): string { return 'Save a reusable workflow as a PHP class. The class must extend ' . 'Claw\\Workflow\\WorkflowAbstract and implement name(). Keep state in plain fields; ' - . 'write each step as a method marked #[\\Claw\\Workflow\\Step] whose body builds prompts, ' - . 'calls $this->ai(...) for the model and $this->tool(...) for tools, and writes to the fields. ' + . 'write each AI step as a pure method marked #[\\Claw\\Workflow\\StepAI] that returns ' + . 'new Claw\\Workflow\\AiStep($prompt, $tools, $agent) declaring one model exchange, and any ' + . 'deterministic glue as a #[\\Claw\\Workflow\\Step] method that calls $this->tool(...). ' . 'The default run() drives the step methods in declaration order; override run() to ' . 'orchestrate by hand. Set "shared" to make it available to every session.'; } diff --git a/src/Workflow/AiStep.php b/src/Workflow/AiStep.php new file mode 100644 index 0000000..732724d --- /dev/null +++ b/src/Workflow/AiStep.php @@ -0,0 +1,34 @@ + $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/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 @@ -')]` and the base judges its recorded artifact against the rules in + * {@see criticRules()}, re-running the step on the supervisor's guidance while the critic is unhappy. */ final class ReviewFileWorkflow extends WorkflowAbstract { - private string $source = ''; - private string $issues = ''; - private string $proposal = ''; + private string $source = ''; public function name(): string { @@ -33,23 +36,33 @@ protected function read(): void $this->source = $this->tool('read_file', ['path' => (string) $this->param('path')]); } - #[Step] - protected function findIssues(): void + #[StepAI] + protected function findIssues(): AiStep { - $this->issues = $this->ai("List the problems in this file:\n\n" . $this->source); + return new AiStep( + "List the problems in this file:\n\n" . $this->source, + params: [new ParamRequest( + forStep: 'propose', + name: 'issues', + instruction: 'List the problems you found, one per line.', + )], + ); } - #[Step] - protected function propose(): void + #[StepAI(critic: 'solid')] + protected function propose(): AiStep { - $this->proposal = $this->ai("Propose concrete fixes for:\n\n" . $this->issues); - - // A critic as a sub-step: judge the proposal with another ai() call and, if it is weak, - // redo it once. Plain PHP — the author decides when to judge; nothing is baked into step(). - $verdict = $this->ai("Reply only 'ok' or 'weak' — is this proposal solid?\n\n" . $this->proposal); + return new AiStep( + 'Propose concrete fixes for these problems, then record your proposal with the artifact tool ' + . "(label 'proposal'):\n\n" . (string) $this->param('issues'), + ['artifact'], + ); + } - if (str_contains(strtolower($verdict), 'weak')) { - $this->proposal = $this->ai("Make this proposal stronger and more concrete:\n\n" . $this->proposal); - } + /** @return array */ + protected function criticRules(): array + { + return ['solid' => 'the proposal must be concrete and address each listed problem; reject a vague ' + . 'proposal or one that skips a problem, so the step re-runs and strengthens it.']; } } diff --git a/src/Workflow/GenerateIssueWorkflow.php b/src/Workflow/GenerateIssueWorkflow.php index 6cb315d..ffdf2db 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). " @@ -188,8 +210,9 @@ protected function criticRules(): array . 'it is valid PHP, and not whether it is written the way you would have written it — the ' . "validator covers the first and the second is not your call.\n\n" . "What is NOT a defect, because reviewers keep calling it one:\n" - . '- A step body that calls $this->ai("…") or $this->tool(…) IS real work. The model does the ' - . "work inside that call. A `placeholder` means a bare literal return with no such call.\n" + . '- A #[StepAI] that returns a real AiStep (a prompt, with tools), or a #[Step] that calls ' + . 'tools, IS real work — the model does the work inside the declared exchange. A `placeholder` ' + . "is a step that declares nothing to do.\n" . "- Few steps. One step that implements and verifies is the intended shape for a small task.\n" . "- Boilerplate: namespace, strict_types, the class declaration. Rejected at save if wrong.\n\n" . "REJECT when any of these hold — and say which:\n" @@ -199,7 +222,7 @@ protected function criticRules(): array . 'neighbour would absorb without noticing — should be FOLDED into that neighbour. A step ' . 'must justify its own fresh context; a workflow that spends one on a triviality is badly ' . "split. (Having FEW steps is not the fault — a lone meaty step is ideal; a thin step is);\n" - . "- a step is a true placeholder: no ai()/tool() call at all;\n" + . "- a step is a true placeholder: a #[StepAI] returning an empty or trivial AiStep, or a #[Step] that does nothing;\n" . '- the class builds the change as a PHP string and writes it (str_replace/preg_replace ' . 'surgery on source, a heredoc of the new file). It cannot see or fix its own mistakes ' . "that way, and it is how solvers corrupt files — the work must go through the model;\n" @@ -212,7 +235,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}", ]; } @@ -229,14 +252,29 @@ private function taskSummary(): string private function draftPrompt(): string { - // A re-run after the critic rejected the draft: the model still holds its previous attempt in the - // continued conversation, so don't re-state the whole brief — just hand it the findings to fix. + // A re-draft after the save-time validator rejected the class: save() reaches it via back('draft'), + // which re-enters this step FRESH — its exchange was cleared when it first finished — so the model + // does NOT still hold its prior attempt. It must be handed the whole brief again with the complaint + // on top, not just the findings; the findings alone tell it to "fix" a class it can no longer see. $critique = $this->critique(); + $rework = $critique === null ? '' : <<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'); @@ -268,7 +306,7 @@ private function draftPrompt(): string {$chosen} APPROACH; - return <<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 - plain if/while in run() where the flow loops or branches): + choosing steps, NOT a list of steps to stamp out (a step is a `#[StepAI]` method returning an + `AiStep`, or a `#[Step]` CODE method; use plain if/while in run() where the flow loops or + branches): {$recipe} {$approach} - HOW A STEP ACTUALLY DOES WORK — read this twice, it is the part solvers get wrong: - - A step does NOT do the work itself in PHP. You are writing the PLAN; the WORK is done by - a model you drive with `\$this->ai(...)`. Inside that call the model has exactly two moves: - call a TOOL, or `ask` a human. That is the whole vocabulary. Your step's job is to set up - the prompt and let the model act. - - NEVER build the change as a PHP string and write it yourself (no `\$code = "..."; \$this->tool('write_file', ...)`, - no str_replace/preg_replace surgery on source). That is blind: it cannot see or fix its own - mistakes, and it is exactly how solvers corrupt files. To change a file, tell the model to - do it: `\$this->ai('Read src/X.php and add method Y; then run `php -l` on it and fix any - error before you stop.')` — the model reads, edits, and VERIFIES with tools in ONE exchange, - seeing the verifier's output and correcting itself inside the same `ai()` call. - - Verification belongs INSIDE that exchange (the model runs `php -l` / the test gate via the - bash tool and reacts), because a tool result is only visible to the model while the `ai()` - call is still running. A separate later step that runs `php -l` and just RETURNS the error - is useless — once a step returns, no model sees that string. The only thing that re-runs a - step on a bad result is a CRITIC (below). So: either the model verifies-and-fixes within - its own `ai()` exchange, or you gate the step with a critic — never a bare "verify" step. + THE TWO KINDS OF STEP — read this twice, it is the part solvers get wrong: + - An AI step does the real thinking and editing work — but you do NOT do that work in PHP. You + write a PURE method marked `#[StepAI]` that RETURNS a declaration of ONE model exchange — + `return new AiStep(\$prompt, \$tools, \$agent);` — and the base runs, records and (after a + crash) resumes it. The method body has NO side effects: it just builds the prompt and returns. + EXACTLY ONE exchange per #[StepAI]; interleaved computation goes in a neighbouring CODE step. + - A CODE step is deterministic glue: a `protected` method marked `#[Step]` returning void that + reads params and calls tools (`\$this->tool(...)`) — no model call. Use it only for real + mechanical work (e.g. save a value the AI produced). Most solvers need NONE. + - Inside the exchange an #[StepAI] declares, the model does everything through the tools you + exposed: it reads and writes files, runs commands, records artifacts, and can pause to ask a + person. Your method's whole job is the prompt and the tool list. + - NEVER build the change as a PHP string and write it yourself (no `\$code = "..."; ... write_file`, + no str_replace/preg_replace surgery on source). That is blind and corrupts files. To change a + file, tell the MODEL to in the prompt: "Read src/X.php, add method Y, then run `php -l` and fix + any error before you stop." — the model reads, edits and VERIFIES with tools in the ONE exchange, + seeing the verifier's output and correcting itself before it returns. + - Verification belongs INSIDE that one exchange (the model runs `php -l` / the tests via the bash + tool and reacts) — a tool result is only visible while the exchange runs. A SEPARATE later step + that just runs a check and stops is useless; the only thing that re-runs a step on a bad result + is a CRITIC (below). So: either the model verifies-and-fixes within its own exchange, or you + gate the step with a critic — never a bare "verify" step. + + THE SHAPE OF A STEP, EXACTLY — copy this shape; the body is one `return new AiStep(...);` and + nothing else (no code, no side effects, and PHP heredocs/quotes only — never Python triple quotes): + + #[StepAI] + protected function implement(): AiStep + { + return new AiStep( + 'Read src/Foo.php, add the method the ticket asks for, then run `php -l` on the file ' + . 'and fix any error before you stop. Record the finished file with the artifact tool.', + ['read_file', 'write_file', 'bash', 'artifact'], + '{$workerTier}', + ); + } + + A CODE step is `protected function (): void` marked `#[Step]` and calls `\$this->tool(...)`. Hard requirements. Most are checked mechanically when the class is saved and cost you a rejection round if missed — the opening tag and `declare(strict_types=1)`, the namespace and - class name, `extends WorkflowAbstract`, at least one `#[Step]`, that steps are `protected`, + class name, `extends WorkflowAbstract`, at least one step, that steps are `protected`, that every critic name has rules, and the forbidden builtins. The rest are not checked by anything, which makes them the ones to read twice: - the file must begin with the opening tag `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('