diff --git a/docs/SURFACE.md b/docs/SURFACE.md index d7064dfa6..3c43ea83a 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -1384,7 +1384,7 @@ The exit codes are part of the surface contract: |---:|---| | `0` | The run completed with `completionReason: success`; deliberate declination also carries a `run_declined` diagnostic locally. | | `1` | The run failed with a declared `completionReason`, or a transport, runtime, or daemon protocol error left the outcome unknown. A `step_failed` run names the failing step and its per-step `completionReason`, plus the exit code and output tails the journal recorded for it. An authored `done("step_failed")` exits `1` as well, and says so without naming a step, because no step failed — the body declared the verdict. With a `detail`, that detail replaces the generic sentence and is reported as `completionDetail`. | -| `2` | The command was refused before a journal write: invalid input, failed preflight, unreachable daemon, or a `run_not_found` resume target. | +| `2` | The command was refused before a journal write: invalid input, failed preflight, unreachable daemon, a `run_not_found` resume target, or a `--local-agent` the named run cannot honour (`local_agent_unavailable`, below). | | `3` | The run parked. `PARKED [run_parked]` names the step and its `llm` or `agent` type, and distinguishes an unavailable worker from a `needs_human` recovery wait. An authored body parked on `f.human` reports the question, who it is for, and the `flows answer` invocation that records the decision (see *Human gates* below). | Without an attached worker, reaching an `llm` or `agent` step returns a durable @@ -1405,6 +1405,52 @@ If the lease expires without a completion, the command fails closed instead of polling forever. A manual-recovery agent whose worker dies parks in `needs_human`; the same exit-3 report says it is waiting for human recovery. +### Naming the worker a park is missing + +An exit-3 park for want of an agent worker names an invocation that supplies +one. Which invocation depends on what parked, and each command is rendered +shell-quoted with the `--data-dir` this invocation actually used, so it is +runnable as printed: + +| Parked | Printed remedy | +|---|---| +| `flows run ` | `flows run --local-agent ''` — a new run. | +| `flows resume ` on a spec run | `flows resume --local-agent ` — *this* run, which is resumable. | +| An authored `.flow.ts` | `flows run --local-agent '' --input ''`, repeating the input the parked run was started with. | + +The authored line repeats `--input` because a directly run `.flow.ts` without +one is refused (`input_missing`); when the journal recorded no input, the report +states that requirement in prose rather than substituting `--input '{}'`, which +would name a different invocation of the flow than the one that parked. Every +line carries the standing caveat that declared workspace or stream surfaces +require a worker holding their pins. + +A park reported by `flows run` repeats the `--input` word that invocation was +given, so a run started from a file names that file. A resume has only the +journal, which records the input document and not the word that carried it, so +it renders the input inline — accepted, because an argument that cannot be a +filename is read as inline JSON rather than as an unreadable path. A recorded +input larger than one `execve` argument (`MAX_ARG_STRLEN`, 128KiB, against the +1MiB `--input` ceiling) is stated in prose with its size, naming the input file +to pass: a printed command that dies with `Argument list too long` is no +better than the park it answers. + +Two parks print no remedy, on purpose. A `needs_human` recovery wait says +nothing about workers, because attaching one does not clear it. And when +`--local-agent` was already passed, the report says a worker is attached and +none was eligible for the step — never "pass `--local-agent`" to someone who +just did. + +For an authored root, `--local-agent` is admitted at run start: the worker +stream is pinned into the root's metadata, so a resume can only reproduce the +surface the run began with. `flows resume --local-agent` against an authored +root started without one is refused before any worker attaches and before the +resume touches the journal — exit 2, `REFUSED [local_agent_unavailable]`, +naming the new run to start. A resume that drops the flag a root *was* pinned +with is refused the same way, naming the resume that keeps it. On a declarative +run the flag is honoured rather than refused: it attaches a worker and drives +the parked step. The flag is never accepted and ignored. + ### Human gates: `f.human` ```ts diff --git a/packages/sdk/src/authored-flow-error.ts b/packages/sdk/src/authored-flow-error.ts index fa9ad9dcf..d5ed2251f 100644 --- a/packages/sdk/src/authored-flow-error.ts +++ b/packages/sdk/src/authored-flow-error.ts @@ -1,5 +1,5 @@ import type { HumanRecipient } from './human-to.js'; -import type { StepFailedDetails } from './failure-kinds.js'; +import type { ParkCause, StepFailedDetails } from './failure-kinds.js'; import type { CompletionReason as ProtocolCompletionReason, RunCompletionReason as ProtocolRunCompletionReason, @@ -42,6 +42,18 @@ export type AuthoredFlowExecutionErrorCode = export class AuthoredFlowExecutionError extends Error { /** Set by the durable root driver after the child error crosses any IPC boundary. */ rootRunId?: string; + /** + * Why an `agent_parked`/`llm_parked` child run parked, as the child's own + * classification established it (`RunReport.parkCause`). + * + * Set by the worker runner beside the code, because the code alone does not + * say: `agent_parked` covers both "nothing is attached to run this step" and + * the kernel's `needs_human` recovery wait, and only the first is fixed by + * attaching a worker. The CLI boundary reads this field rather than matching + * the message, so the remedy it prints cannot drift from the cause. Absent + * means unestablished — the boundary then says nothing about workers. + */ + parkCause?: ParkCause; constructor( readonly code: AuthoredFlowExecutionErrorCode, message: string, diff --git a/packages/sdk/src/authored-human.ts b/packages/sdk/src/authored-human.ts index cf3c01eb0..402a4852c 100644 --- a/packages/sdk/src/authored-human.ts +++ b/packages/sdk/src/authored-human.ts @@ -1,5 +1,6 @@ import { AuthoredFlowExecutionError, type AuthoredHumanWait } from './authored-flow-error.js'; import type { JournalClient } from './journal-client.js'; +import { shellWord } from './shell-word.js'; /** * The answer contract for a parked `f.human`. @@ -144,7 +145,3 @@ export function resumeCommand(runId: string, dataDir?: string, localAgent = fals const dir = dataDir === undefined ? '' : ` --data-dir ${shellWord(dataDir)}`; return `flows resume${dir}${localAgent ? ' --local-agent' : ''} ${runId}`; } - -function shellWord(value: string): string { - return /^[A-Za-z0-9_./=:@%+,-]+$/.test(value) ? value : `'${value.replaceAll("'", "'\\''")}'`; -} diff --git a/packages/sdk/src/authored-node-entry.ts b/packages/sdk/src/authored-node-entry.ts index b03bdef73..90705e6ef 100644 --- a/packages/sdk/src/authored-node-entry.ts +++ b/packages/sdk/src/authored-node-entry.ts @@ -88,9 +88,12 @@ try { // `details` is the failing child step's journal evidence. Without it the // parent can only re-render the message, and the machine-readable // diagnostic loses the command's exit code and output tails. + // `parkCause` travels for the same reason: it is the difference between + // "attach a worker" and "a human has to recover this", and this process is + // the only one that saw the child's classification. ...(error instanceof AuthoredFlowExecutionError ? { code: error.code, completionReason: error.completionReason, runId: error.runId, - details: error.details } : {}), + details: error.details, parkCause: error.parkCause } : {}), ...(error instanceof AuthoredHumanParked ? { wait: error.wait } : {}) }); process.exitCode = 1; } finally { diff --git a/packages/sdk/src/authored-node-runner.ts b/packages/sdk/src/authored-node-runner.ts index 0636305ac..eae279a12 100644 --- a/packages/sdk/src/authored-node-runner.ts +++ b/packages/sdk/src/authored-node-runner.ts @@ -13,7 +13,7 @@ import { AuthoredFlowExecutionError, AuthoredHumanParked, type AuthoredFlowExecutionErrorCode, type AuthoredHumanWait, } from './authored-flow-error.js'; -import type { StepFailedDetails } from './failure-kinds.js'; +import type { ParkCause, StepFailedDetails } from './failure-kinds.js'; import { HUMAN_WAIT_ID } from './authored-human.js'; import { assertAuthoredPromiseHooks } from './authored-runtime-capability.js'; @@ -137,13 +137,18 @@ export async function runAuthoredInNode( else if (message.type === 'wait') options.onWait?.(message.event); else if (message.type === 'result') result = { ...message.result, executionRuntime: runtime }; else if (message.type === 'error') { - failure = message.code === 'human_parked' && isHumanWaitFrame(message.wait) && message.runId === rootRunId - ? new AuthoredHumanParked(message.wait, rootRunId) - : typeof message.code === 'string' - ? new AuthoredFlowExecutionError(message.code as AuthoredFlowExecutionErrorCode, - message.message, message.completionReason, message.runId, - stepFailedFrame(message.details)) - : new Error(message.message); + if (message.code === 'human_parked' && isHumanWaitFrame(message.wait) && message.runId === rootRunId) { + failure = new AuthoredHumanParked(message.wait, rootRunId); + } else if (typeof message.code === 'string') { + const authored = new AuthoredFlowExecutionError(message.code as AuthoredFlowExecutionErrorCode, + message.message, message.completionReason, message.runId, + stepFailedFrame(message.details)); + // Validated, not trusted, like `details`: an unrecognised cause + // is dropped so the boundary says nothing about workers rather + // than acting on a value this frame could have invented. + authored.parkCause = parkCauseFrame(message.parkCause); + failure = authored; + } else failure = new Error(message.message); } else throw new Error('unknown authored runtime message'); } catch (error) { stop(error instanceof Error ? error : new Error('invalid authored runtime message')); } } @@ -174,6 +179,17 @@ function isHumanWaitFrame(value: unknown): value is AuthoredHumanWait { && typeof wait.to === 'string' && wait.to !== ''; } +/** + * A park cause arriving over IPC, accepted only as one of the two values the + * type admits. Anything else — absent, misspelled, a different type — becomes + * `undefined`, which the remedy formatter reads as "unestablished" and answers + * with silence. Guessing `worker_unavailable` here would let a malformed frame + * put "attach a worker" on a park a worker cannot clear. + */ +export function parkCauseFrame(value: unknown): ParkCause | undefined { + return value === 'worker_unavailable' || value === 'needs_human' ? value : undefined; +} + /** * Step evidence arriving over IPC, reduced to the fields `StepFailedDetails` * declares and the types it declares them as. The child is the same pinned diff --git a/packages/sdk/src/authored-worker-step.ts b/packages/sdk/src/authored-worker-step.ts index ac936fec9..7fdc781f0 100644 --- a/packages/sdk/src/authored-worker-step.ts +++ b/packages/sdk/src/authored-worker-step.ts @@ -78,7 +78,7 @@ export function authoredWorkerRunner( const execution = await classifyOutcome(journal, 'run', outcome, report, '', waitOptions); if (execution.exitCode === 3) { const parked = execution.report.parkedStep; - throw new AuthoredFlowExecutionError( + const error = new AuthoredFlowExecutionError( step.type === 'llm' ? 'llm_parked' : 'agent_parked', execution.report.diagnostics.at(-1)?.message ?? `flow "${definition.name}" step "${id}" parked` @@ -87,6 +87,14 @@ export function authoredWorkerRunner( undefined, outcome.run_id, ); + // Both `agent_parked` and `llm_parked` cover two unrelated situations — + // nothing attached to run the step, and the kernel's `needs_human` + // recovery wait after a worker attempt failed — and only the first is + // fixed by attaching a worker. The child's classification already knows + // which; carry it so the CLI boundary can name a remedy without + // re-deriving one from this message's wording. + error.parkCause = execution.report.parkCause; + throw error; } if (execution.exitCode !== 0) { // `execution.report.completionReason` is the RUN's reason (normally diff --git a/packages/sdk/src/cli/direct-run.ts b/packages/sdk/src/cli/direct-run.ts index 97bb4732d..ca7253c8d 100644 --- a/packages/sdk/src/cli/direct-run.ts +++ b/packages/sdk/src/cli/direct-run.ts @@ -16,6 +16,7 @@ import { DirectInputError, parseDirectInput } from '../direct-input.js'; import { JournalClient } from '../journal-client.js'; import { inputFailureReport } from './check.js'; import { checkAuthoredTriggers } from './check-triggers.js'; +import { authoredInput, authoredWorkerRemedy, localAgentRemedy } from './local-agent-remedy.js'; import { authoredCompletion, authoredHumanParked, @@ -164,12 +165,26 @@ export async function runDirectFlow( ...base, ok: false, runId: error.runId, + rootRunId: error.rootRunId, socketPath, status: 'parked', + parkCause: error.parkCause, diagnostics: [...base.diagnostics, { severity: 'parked', kind: 'run_parked', - message: error.message, + // The remedy is rendered here rather than by the child's own + // `classifyOutcome`, which is deliberately silent for authored + // paths: this is the only frame that knows both the flow path and + // the `--input` argument a new run has to repeat, and it knows + // whether a worker was already attached. + // + // `inputArgument` is the word this process was invoked with, so a + // run started from a file names that file rather than the megabyte + // inside it — the thing the journal can no longer tell a resume. + message: error.message + localAgentRemedy(authoredWorkerRemedy( + error.parkCause, options.localAgent === true, + { path, input: authoredInput(inputArgument), dataDir }, + )), }], }, }; diff --git a/packages/sdk/src/cli/local-agent-remedy.ts b/packages/sdk/src/cli/local-agent-remedy.ts new file mode 100644 index 000000000..490268fb5 --- /dev/null +++ b/packages/sdk/src/cli/local-agent-remedy.ts @@ -0,0 +1,160 @@ +import { resumeCommand } from '../authored-human.js'; +import type { ParkCause } from '../failure-kinds.js'; +import { shellQuote, shellWord } from '../shell-word.js'; + +/** + * What to tell someone whose run stopped for want of an agent worker. + * + * A closed union rather than a bag of optional fields: the remedy for a spec + * run is a *new* run, for a spec resume it is *this* run, and for an authored + * `.flow.ts` it is a new run that must carry the same `--input` — three + * different sentences, and no combination of them is meaningful. `attached` + * is the case the message used to get wrong by omission: a worker WAS offered + * and none of them was eligible, so naming the flag again is not the fix. + */ +export type LocalAgentRemedy = + /** Nothing to say: not a worker park, or the caller renders it elsewhere. */ + | { kind: 'none' } + | { kind: 'attached' } + | { kind: 'spec-run'; path: string; dataDir?: string } + | { kind: 'spec-resume'; runId: string; dataDir?: string } + | { kind: 'authored-run'; path: string; input: AuthoredInput; dataDir?: string }; + +/** + * The `--input` argument a new run of an authored flow would have to repeat. + * + * Three outcomes, not an optional string, because each is a different sentence + * and only one of them ends in a command. None of them is `{}`: fabricating an + * input would hand over a command that runs a different flow invocation than + * the one that parked. + */ +export type AuthoredInput = + /** The argument this run was started with, rendered back verbatim. */ + | { kind: 'inline'; argument: string } + /** The journal recorded no input argument to repeat. */ + | { kind: 'absent' } + /** Recorded, but too many bytes to hand a shell as one word. */ + | { kind: 'oversized'; bytes: number }; + +/** + * The largest `--input` value worth printing inline, in bytes. + * + * Linux caps a single `execve` argument at `MAX_ARG_STRLEN` — 32 pages, + * 131072 bytes including the terminating NUL — independently of the much + * larger `ARG_MAX` total. `MAX_DIRECT_INPUT_BYTES` admits a full mebibyte, so + * a run started from a perfectly ordinary input file can have recorded more + * input than any shell can pass back. Printing it anyway yields a command that + * dies with `Argument list too long` before the CLI is even reached, which is + * no better than the park it was meant to answer. + */ +const MAX_INLINE_INPUT_BYTES = 131_071; + +/** How a recorded input argument should be rendered, given its size. */ +export function authoredInput(argument: string | undefined): AuthoredInput { + if (argument === undefined) return { kind: 'absent' }; + const bytes = Buffer.byteLength(argument, 'utf8'); + return bytes > MAX_INLINE_INPUT_BYTES ? { kind: 'oversized', bytes } : { kind: 'inline', argument }; +} + +/** + * The declared-surface caveat, stated as the condition it is. + * + * A step that declares a workspace or a stream needs a worker holding those + * pins, and the local agent worker holds only its own stream. That is worth + * saying next to the command — but it is a caveat on the remedy, not a + * diagnosis of this park, and it is phrased so it cannot be read as one. + */ +const PINS = 'Declared workspace or stream surfaces require a worker that holds their pins.'; + +/** + * The remedy clause appended to a worker-park diagnostic, with a leading + * space, or `''` when there is nothing to add. + */ +export function localAgentRemedy(remedy: LocalAgentRemedy): string { + switch (remedy.kind) { + case 'none': + return ''; + case 'attached': + // Never "pass --local-agent": it was passed. The honest report is that + // a worker was offered and the daemon matched none of them to this step. + return ' A local agent worker is already attached to this run, so passing --local-agent again' + + ' changes nothing: no attached worker was eligible for this step.' + + ` ${PINS} The local agent worker holds only its own stream.`; + case 'spec-run': + return ` To start a new run with a local agent worker: ${ + newRunCommand(remedy.path, undefined, remedy.dataDir)}. ${PINS}`; + case 'spec-resume': + // This run is resumable, so the remedy is this run — not a new one. + return ` To continue this run with a local agent worker: ${ + resumeCommand(remedy.runId, remedy.dataDir, true)}. ${PINS}`; + case 'authored-run': + return authoredRunRemedy(remedy); + } +} + +/** + * Either a runnable new run, or the requirement stated in prose. + * + * Prose, not a command, whenever the `--input` cannot be printed as something + * a shell would deliver intact: a `.flow.ts` invocation without its `--input` + * is refused (`input_missing`), and a placeholder or a truncation in its place + * is worse than a sentence, because it looks runnable. + */ +function authoredRunRemedy(remedy: Extract): string { + if (remedy.input.kind === 'inline') { + return ` To start a new run with a local agent worker: ${ + newRunCommand(remedy.path, remedy.input.argument, remedy.dataDir)}. ${PINS}`; + } + const requirement = ' A local agent worker is admitted at run start, so this run needs a new one.' + + ` Starting one needs --local-agent, the flow path ${shellQuote(remedy.path)}, and the same` + + ' --input this flow takes; '; + return requirement + (remedy.input.kind === 'absent' + ? 'the journal recorded no input argument to repeat here.' + // Naming the size says which input, and says why this clause is prose: + // the recorded value is larger than one `execve` argument may be, so a + // printed `--input ''` would fail before the CLI ran at all. + : `the journal recorded ${remedy.input.bytes} bytes of input, more than a shell` + + ' can carry in one argument, so pass the input file this run was started from.') + + ` ${PINS}`; +} + +/** + * Which remedy an authored `.flow.ts` park gets, decided once for both authored + * boundaries (`cli/direct-run.ts` on a fresh run, `resumeFlow` on a resume). + * + * The cause is trusted, not guessed: anything other than `worker_unavailable` — + * including an absent cause — yields `none`, so a `needs_human` recovery wait is + * never answered with "attach a worker", and an unclassified park says nothing + * at all rather than something plausible. + * + * `path` is optional because the caller may only have the journal to go on. A + * park with no known flow path yields `none` for the same reason an unknown + * input yields prose: the point of this clause is a command someone can run. + */ +export function authoredWorkerRemedy( + parkCause: ParkCause | undefined, + attached: boolean, + run: { path?: string; input: AuthoredInput; dataDir?: string }, +): LocalAgentRemedy { + if (parkCause !== 'worker_unavailable') return { kind: 'none' }; + if (attached) return { kind: 'attached' }; + if (run.path === undefined) return { kind: 'none' }; + return { kind: 'authored-run', path: run.path, input: run.input, dataDir: run.dataDir }; +} + +/** + * `flows run --local-agent [--input ] [--data-dir ]`. + * + * The flag/path prefix is byte-for-byte what the spec-run message has always + * emitted, so the one thing people already grep for still matches. `--input` + * and `--data-dir` follow the path because the parser is order-insensitive + * (cli.ts `parseRunArgs`), and keeping the prefix intact matters more than + * mirroring the usage line's flag order. + */ +function newRunCommand(path: string, input: string | undefined, dataDir: string | undefined): string { + return `flows run --local-agent ${shellQuote(path)}` + + (input === undefined ? '' : ` --input ${shellQuote(input)}`) + // `shellWord` here, matching `resumeCommand`/`answerCommand`: a data dir is + // this machine's path, and the two renderings must not disagree. + + (dataDir === undefined ? '' : ` --data-dir ${shellWord(dataDir)}`); +} diff --git a/packages/sdk/src/cli/run.ts b/packages/sdk/src/cli/run.ts index f71cb8183..21c6f3b42 100644 --- a/packages/sdk/src/cli/run.ts +++ b/packages/sdk/src/cli/run.ts @@ -15,12 +15,16 @@ import { socketPathFor } from '../daemon-connection.js'; import { ensureDaemon, type EnsureDaemonOptions } from '../daemon-lifecycle.js'; import { isAuthoredFlowPath } from '../direct-input.js'; import { daemonRefusal } from './daemon-refusal.js'; -import type { RunFailureKind, RunWarningKind, StepFailedDetails } from '../failure-kinds.js'; +import { + authoredInput, authoredWorkerRemedy, localAgentRemedy, + type AuthoredInput, type LocalAgentRemedy, +} from './local-agent-remedy.js'; +import type { ParkCause, RunFailureKind, RunWarningKind, StepFailedDetails } from '../failure-kinds.js'; import { inspectionHint, renderInspection, renderStepEvidence, stepFailureDetails } from './step-failure.js'; import { JournalClient, JournalProtocolError } from '../journal-client.js'; import { attachLocalAgent } from '../local-agent.js'; import { LlmWorker } from '../llm-worker.js'; -import { readAuthoredRootMetadata, resumeDurableAuthoredFlow } from '../authored-root.js'; +import { readAuthoredRootMetadata, resumeDurableAuthoredFlow, type AuthoredRootMetadata } from '../authored-root.js'; import type { RunCompletionReason, RunOutcome, @@ -71,6 +75,17 @@ export interface RunReport { completedSteps?: number; reuse?: { fromRunId: string; reusedSteps: number; executedSteps: number }; parkedStep?: ParkedStep; + /** + * Why an exit-3 park happened, when classification established it. + * + * Carried structurally so a consumer — including this package's own authored + * boundaries — can tell "nothing is attached to run this step" from "the + * kernel is waiting for a human to recover it" without matching on the + * rendered message. Absent means the cause was not established, never + * `worker_unavailable` by default: recommending a worker for a `needs_human` + * park is the wrong answer, and guessing it is worse than saying nothing. + */ + parkCause?: ParkCause; /** The open `f.human` question a parked authored run is waiting on. */ humanWait?: AuthoredHumanWait; /** `flows answer` only: the answer it recorded. */ @@ -209,15 +224,32 @@ export async function resumeFlow( let authoredLlm: LlmWorker | undefined; let llmFailure: unknown; let authoredLlmClient: JournalClient | undefined; + // Hoisted so the catch below can build the authored park remedy from the same + // metadata this resume was admitted against — the journal is the only place + // the flow path and the original `--input` survive a process boundary. + let authoredRoot: AuthoredRootMetadata | undefined; const workerCapacity = options.agentCapacity ?? DEFAULT_LOCAL_AGENT_CAPACITY; try { - const authoredRoot = await readAuthoredRootMetadata(client, runId); + authoredRoot = await readAuthoredRootMetadata(client, runId); if (authoredRoot !== undefined) { + // Both worker-surface mismatches are refusals, not protocol failures. + // They used to throw bare `Error`s, which landed on `protocol_error` + // ("RUN unknown") and told nobody what to do instead; and the + // second one is the exact complaint this change answers — `--local-agent` + // accepted on a run that cannot grow one is worse than rejected, because + // the run parks again with a message that has not changed. if (authoredRoot.localAgentStream !== undefined && !options.localAgent) { - throw new Error('authored root requires --local-agent to resume its pinned worker surface'); + return localAgentRefusal(base, runId, socketPath, + `Run "${runId}" was started with a local agent worker surface, so resuming it needs the same flag.` + + ` To continue this run: ${resumeCommand(runId, dataDir, true)}.`); } if (authoredRoot.localAgentStream === undefined && options.localAgent) { - throw new Error('authored root was started without a local agent worker surface'); + return localAgentRefusal(base, runId, socketPath, + `Run "${runId}" was started without a local agent worker surface:` + + ' --local-agent is admitted at run start; start a new run.' + + localAgentRemedy(authoredWorkerRemedy('worker_unavailable', false, { + path: authoredRoot.flowPath, input: authoredInputArgument(authoredRoot), dataDir, + }))); } if (options.localAgent) { authoredAgent = await attachLocalAgent( @@ -285,6 +317,34 @@ export async function resumeFlow( if (error instanceof AuthoredFlowExecutionError && (error.code === 'step_failed' || error.code === 'gate_failed')) { return authoredStepFailure('resume', base, socketPath, error, runId); } + // An authored resume that parks for want of a worker is a park, reported + // like the run path's — and with the same remedy, because an authored root + // admits its worker at run start: resuming again cannot attach one, so the + // only honest instruction is a new run carrying the same `--input`. + // `runId` here is the CHILD run holding the evidence; the root this resume + // named stays separate, so `flows` can still collect it. + if (error instanceof AuthoredFlowExecutionError && (error.code === 'agent_parked' || error.code === 'llm_parked')) { + return { + exitCode: 3, + report: { + ...base, + ok: false, + runId: error.runId ?? runId, + rootRunId: runId, + socketPath, + status: 'parked', + parkCause: error.parkCause, + diagnostics: [...base.diagnostics, { + severity: 'parked', + kind: 'run_parked', + message: error.message + localAgentRemedy(authoredWorkerRemedy( + error.parkCause, options.localAgent === true, + { path: authoredRoot?.flowPath, input: authoredInputArgument(authoredRoot), dataDir }, + )), + }], + }, + }; + } if (error instanceof AuthoredHumanParked) { return authoredHumanParked('resume', base, socketPath, error, { dataDir, localAgent: options.localAgent === true }); } @@ -318,6 +378,45 @@ export async function resumeFlow( } } +/** + * A `--local-agent` mismatch, refused before the resume touches the journal. + * + * Exit 2 and `ok` left false by `emptyReport`: nothing ran, so this is the same + * shape as every other pre-journal refusal on this surface (docs/SURFACE.md §5). + * It returns rather than throwing so no worker is attached on the way out. + */ +function localAgentRefusal( + base: RunReport, runId: string, socketPath: string, message: string, +): RunExecution { + return { + exitCode: 2, + report: { ...base, runId, socketPath, + diagnostics: [...base.diagnostics, { severity: 'refusal', kind: 'local_agent_unavailable', message }] }, + }; +} + +/** + * The `--input` argument a new run of this authored root would have to repeat, + * rendered back from the journaled metadata. + * + * `absent` when the root recorded no input, and also when the recorded input is + * something `JSON.stringify` cannot render — the remedy formatter then says an + * input is required instead of printing a command that would be refused as + * `input_invalid`. What it must never do is substitute `{}`: that is a + * different invocation of the flow than the one that parked. + * + * Unlike `runDirectFlow`, this side has only the decoded value: the journal + * records the input, not the `--input` word that carried it, so a run started + * from a file comes back as inline JSON. `authoredInput` is what keeps that + * honest about its own size — a recorded input can be larger than a shell + * argument even when the file that supplied it was unremarkable. + */ +function authoredInputArgument(metadata: AuthoredRootMetadata | undefined): AuthoredInput { + if (metadata === undefined || !metadata.inputPresent) return { kind: 'absent' }; + const rendered = JSON.stringify(metadata.input); + return authoredInput(typeof rendered === 'string' ? rendered : undefined); +} + /** * A step that ran and failed, reported as the run failure it is. * @@ -708,25 +807,14 @@ export async function classifyOutcome( report: { ...report, parkedStep, + parkCause: needsHuman ? 'needs_human' : 'worker_unavailable', diagnostics: [...report.diagnostics, { severity: 'parked', kind: 'run_parked', message: needsHuman ? `Run "${current.run_id}" parked at step "${parkedStep.id}" (${parkedStep.type}): waiting for human recovery after the worker attempt failed.` : `Run "${current.run_id}" parked at step "${parkedStep.id}" (${parkedStep.type}): no worker is attached for step type "${parkedStep.type}".` - // Only suggest the `--local-agent` remedy for YAML flows. - // Authored TS flows require `--input`; the bare command below - // would be refused (Cursor Bugbot flagged as LOW on flows#293). - // For TS we omit the hint rather than fabricate a syntactically - // valid but semantically wrong command — the direct-run refusal - // for TS already names its own missing --input. - + (command === 'run' - && parkedStep.type === 'agent' - && !options.localAgent - && base.path !== undefined - && !isAuthoredFlowPath(base.path) - ? ` To start a new run with a local agent worker: flows run --local-agent '${base.path.replace(/'/g, "'\\''")}'. Declared workspace or stream surfaces require a worker that holds their pins.` - : ''), + + localAgentRemedy(declarativeWorkerRemedy(command, current.run_id, parkedStep, base, options)), }], }, }; @@ -736,6 +824,39 @@ export async function classifyOutcome( ), current.run_id); } +/** + * Which remedy a worker park gets from the declarative classifier. + * + * An authored `.flow.ts` is deliberately SILENT here, and the suppression is + * tested first on purpose. Its remedy is a new run carrying the same `--input`, + * an argument this function does not have; `runDirectFlow` and `resumeFlow` + * render it once at the authored boundary where the input (or the journaled + * metadata holding it) is in hand. `classifyOutcome` also runs once per + * authored `f.agent` CHILD run, so answering here as well would append a + * remedy to every child message on the way out — and, on resume, a second + * contradictory one after the attached-worker branch below. + * + * `llm` parks get nothing, unchanged: the declarative verbs attach an + * agent-only worker (local-agent.ts advertises `['agent']`), so `--local-agent` + * is not a remedy for an `llm` step outside the authored path, which attaches + * an `LlmWorker` of its own. + */ +function declarativeWorkerRemedy( + command: RunCommand, + runId: string, + parkedStep: ParkedStep, + base: CheckReport | RunReport, + options: RunLifecycleOptions, +): LocalAgentRemedy { + const dataDir = options.dataDir; + if (parkedStep.type !== 'agent') return { kind: 'none' }; + if (base.path !== undefined && isAuthoredFlowPath(base.path)) return { kind: 'none' }; + if (options.localAgent === true) return { kind: 'attached' }; + if (command === 'resume') return { kind: 'spec-resume', runId, dataDir }; + if (command === 'run' && base.path !== undefined) return { kind: 'spec-run', path: base.path, dataDir }; + return { kind: 'none' }; +} + interface OutOfBandInspection { status: RunStatus; parkedStep?: ParkedStep; diff --git a/packages/sdk/src/direct-input.ts b/packages/sdk/src/direct-input.ts index a5e367da5..407628f8b 100644 --- a/packages/sdk/src/direct-input.ts +++ b/packages/sdk/src/direct-input.ts @@ -39,7 +39,7 @@ export function parseDirectInput(argument: string | undefined): unknown { } } catch (error) { if (error instanceof DirectInputError) throw error; - if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { + if (!namesNoFile(error)) { throw new DirectInputError('input_unreadable', `Input file "${argument}" could not be inspected.`); } } @@ -56,6 +56,27 @@ export function parseDirectInput(argument: string | undefined): unknown { } } +/** + * Whether the `stat` failed because the argument names no file at all, rather + * than because a file exists and could not be inspected. + * + * `ENOENT` is the ordinary "nothing there". `ENAMETOOLONG` is the same answer + * for a longer argument: no path component may exceed the filesystem's limit + * (255 bytes on ext4 and on APFS), so an inline JSON object of a few hundred + * bytes cannot be a filename on any filesystem this runs on. Calling that + * `input_unreadable` refused every inline input longer than a filename — the + * recorded-input recovery commands `cli/local-agent-remedy.ts` prints for a + * parked run among them, which is how it was found. + * + * Anything else — a permission error, an I/O error — still refuses, because it + * means a path is there and this process could not look at it. Guessing that + * such an argument was inline JSON would parse a filename as a document. + */ +function namesNoFile(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException).code; + return code === 'ENOENT' || code === 'ENAMETOOLONG'; +} + function tooLarge(argument: string, fromFile: boolean): DirectInputError { const sourceKind = fromFile ? `Input file "${argument}"` : 'Inline input'; return new DirectInputError( diff --git a/packages/sdk/src/failure-kinds.ts b/packages/sdk/src/failure-kinds.ts index 1d9fdde67..de9c6c85b 100644 --- a/packages/sdk/src/failure-kinds.ts +++ b/packages/sdk/src/failure-kinds.ts @@ -134,6 +134,14 @@ export const RUN_FAILURE_KINDS = [ 'gate_failed', /** `flows answer` named a wait the run is not asking: unknown, or already answered. */ 'human_wait_unknown', + /** + * `--local-agent` cannot be honoured for this invocation, so it is refused + * rather than accepted and ignored. A local agent worker is admitted at run + * start and pinned into the authored root's metadata; a resume can only + * reproduce the surface the root was started with. Exit 2, before any worker + * attaches and before the resume touches the journal. + */ + 'local_agent_unavailable', ] as const; /** @@ -165,6 +173,20 @@ export type PreflightWarningKind = (typeof PREFLIGHT_WARNING_KINDS)[number]; export type RunFailureKind = (typeof RUN_FAILURE_KINDS)[number]; export type RunWarningKind = (typeof RUN_WARNING_KINDS)[number]; +/** + * Why a run parked, for the reporting side that has to name a remedy. + * + * `worker_unavailable` is a step nothing is attached to run — the one case + * `--local-agent` fixes. `needs_human` is the kernel's manual-recovery wait + * after a worker attempt already failed, and attaching a worker does not clear + * it. Both arrive as exit 3 under `run_parked`, so the distinction has to + * travel as a value: it lives here, beside the run vocabulary, because the + * classifier (cli/run.ts), the authored error boundary (authored-flow-error.ts) + * and the remedy formatter (cli/local-agent-remedy.ts) all need the same one + * and none of them may depend on the others. + */ +export type ParkCause = 'worker_unavailable' | 'needs_human'; + /** * Optional evidence on the existing step_failed diagnostic, not a new kind. * diff --git a/packages/sdk/src/shell-word.ts b/packages/sdk/src/shell-word.ts new file mode 100644 index 000000000..587d7262b --- /dev/null +++ b/packages/sdk/src/shell-word.ts @@ -0,0 +1,24 @@ +/** + * POSIX shell quoting for the invocations diagnostics tell people to run. + * + * Dependency-free on purpose: every module that renders a command reaches for + * this, and two of them (`cli/run.ts` and `authored-human.ts`) already import + * each other's neighbours. A copy per call site is how one of them ends up + * emitting a path with a space in it unquoted. + */ + +/** `value` as a single word, quoted only when it would not survive a shell. */ +export function shellWord(value: string): string { + return /^[A-Za-z0-9_./=:@%+,-]+$/.test(value) ? value : shellQuote(value); +} + +/** + * `value` as a single-quoted word, always. + * + * Used where the rendered argument is arbitrary author data — a flow path or a + * JSON `--input` — and the quotes are worth keeping even when unnecessary, so + * the command reads the same for `a.flow.ts` and for `my flows/a.flow.ts`. + */ +export function shellQuote(value: string): string { + return `'${value.replaceAll("'", "'\\''")}'`; +} diff --git a/packages/sdk/tests/classify-outcome.test.ts b/packages/sdk/tests/classify-outcome.test.ts index 95a5d9be2..a79d9b354 100644 --- a/packages/sdk/tests/classify-outcome.test.ts +++ b/packages/sdk/tests/classify-outcome.test.ts @@ -86,3 +86,58 @@ describe('classifyOutcome', () => { expect(execution.report.parkedStep).toBeUndefined(); }); }); + +const parkedOnAgent: RunGetResult = { + run_id: RUN_ID, + status: 'parked', + steps: { work: { type: 'agent', state: 'runnable' } as never }, + budget: { tokens_in: 0, tokens_out: 0, dollars: '0' }, +}; + +async function classifyPark( + command: 'run' | 'resume', + report: Partial, + options: Record, +): Promise { + const { client } = clientReturning([parkedOnAgent]); + const execution = await classifyOutcome( + client, command, parked, { ...base, ...report } as never, '/tmp/sock', options); + expect(execution.exitCode).toBe(3); + expect(execution.report.parkCause).toBe('worker_unavailable'); + return execution.report.diagnostics.at(-1)!.message; +} + +describe('the remedy on a worker park', () => { + /// The reported defect: the `command === 'run'` guard meant a parked resume + /// printed "no worker is attached" and stopped there, so the obvious next + /// move — resume WITH a worker — went unsaid. A spec run is resumable, so + /// the remedy is this run, not a new one. + it('tells a parked declarative resume to continue this run with a worker', async () => { + const message = await classifyPark('resume', { specPath: 'spec.yaml' }, { dataDir: '/tmp/d' }); + expect(message).toContain(`flows resume --data-dir /tmp/d --local-agent ${RUN_ID}`); + expect(message).not.toContain('flows run'); + }); + + it('tells a parked declarative run to start a new one with a worker', async () => { + const message = await classifyPark('run', { path: 'flows/hello.flow.yaml' }, { dataDir: '/tmp/d' }); + // The prefix that has always been emitted, unchanged, with the data dir + // this invocation actually used appended. + expect(message).toContain(`flows run --local-agent 'flows/hello.flow.yaml' --data-dir /tmp/d`); + }); + + /// An authored `.flow.ts` is silent HERE on purpose. This classifier runs + /// once per authored child run and has no access to the `--input` a new run + /// must repeat; `direct-run.ts` and `resumeFlow` render it once, where both + /// are known. Appending here as well would also double the clause on resume. + it('leaves the authored remedy to the boundary that knows the input', async () => { + const message = await classifyPark('run', { path: 'flows/hello.flow.ts' }, { dataDir: '/tmp/d' }); + expect(message).not.toContain('--local-agent'); + }); + + it('never tells someone who passed --local-agent to pass it again', async () => { + const message = await classifyPark( + 'resume', { specPath: 'spec.yaml' }, { dataDir: '/tmp/d', localAgent: true }); + expect(message).toContain('no attached worker was eligible'); + expect(message).not.toMatch(/flows (run|resume)/); + }); +}); diff --git a/packages/sdk/tests/direct-input.test.ts b/packages/sdk/tests/direct-input.test.ts index 88ff2174c..1d39edf34 100644 --- a/packages/sdk/tests/direct-input.test.ts +++ b/packages/sdk/tests/direct-input.test.ts @@ -161,6 +161,19 @@ describe('direct .flow.ts input through the built CLI and live runtime', () => { expect(file.stdout).toContain('completionReason: success'); expect(readFileSync(fileOutput, 'utf8')).toBe('file value'); + // Inline JSON longer than one path component (255 bytes on ext4). The + // parser `stat`s the argument before treating it as a document, and that + // `stat` fails ENAMETOOLONG rather than ENOENT; refusing it as + // `input_unreadable` rejected every inline input longer than a filename, + // the recovery commands this CLI prints for a parked run included. + const longOutput = join(directory, 'long.txt'); + const longValue = 'x'.repeat(300); + const longInline = JSON.stringify({ output: longOutput, value: longValue }); + expect(longInline.length).toBeGreaterThan(255); + const long = invokeCli(['run', FLOW, '--input', longInline, '--data-dir', dataDir]); + expect(long.status, long.stderr).toBe(0); + expect(readFileSync(longOutput, 'utf8')).toBe(longValue); + const controlOutput = join(directory, 'control.txt'); const control = invokeCli([ 'run', CONTROL_FLOW, '--input', JSON.stringify({ output: controlOutput }), diff --git a/packages/sdk/tests/direct-run-failure.test.ts b/packages/sdk/tests/direct-run-failure.test.ts index 51765f0eb..9b4f427ba 100644 --- a/packages/sdk/tests/direct-run-failure.test.ts +++ b/packages/sdk/tests/direct-run-failure.test.ts @@ -1,3 +1,6 @@ +import { mkdtempSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { expect, it, vi } from 'vitest'; import { runDirectFlow } from '../src/cli/direct-run.js'; import { AuthoredFlowExecutionError } from '../src/authored-flow-executor.js'; @@ -64,6 +67,64 @@ it('uses the worker cause when the authored executor only saw a generic disconne expect(result.exitCode).toBe(1); expect(JSON.stringify(result.report)).toContain('worker transport closed'); }); +/** + * A `.flow.ts` used to be the one path that parked for want of a worker and + * said nothing about how to fix it: `classifyOutcome` suppressed the hint + * because the bare `flows run --local-agent ` it emitted would have been + * unrunnable without the flow's `--input`. The remedy now comes from this + * boundary, which holds the path, the input and the attachment state. + */ +function parkedForWantOfWorker(): AuthoredFlowExecutionError { + const error = new AuthoredFlowExecutionError('agent_parked', + 'Run "child-run" parked at step "agent-1" (agent): no worker is attached for step type "agent".', + undefined, 'child-run'); + error.parkCause = 'worker_unavailable'; + return error; +} + +it('names a runnable new run, with the same --input, when an authored flow parks for want of a worker', async () => { + vi.mocked(executeDurableAuthoredFlow).mockRejectedValueOnce(parkedForWantOfWorker()); + const result = await runDirectFlow('my flows/a.flow.ts', '{"plan":"v2"}', '/tmp/unused'); + expect(result.exitCode).toBe(3); + expect(result.report.parkCause).toBe('worker_unavailable'); + const message = result.report.diagnostics.at(-1)!.message; + // Quoted path, the original input repeated verbatim, and the data dir this + // run used — everything a copy-paste needs, in one command. + expect(message).toContain( + `flows run --local-agent 'my flows/a.flow.ts' --input '{"plan":"v2"}' --data-dir /tmp/unused`); +}); + +it('does not tell someone who already passed --local-agent to pass it again', async () => { + vi.mocked(executeDurableAuthoredFlow).mockRejectedValueOnce(parkedForWantOfWorker()); + const result = await runDirectFlow('flow.ts', '{}', '/tmp/unused', { localAgent: true }); + expect(result.exitCode).toBe(3); + const message = result.report.diagnostics.at(-1)!.message; + expect(message).toContain('already attached'); + expect(message).not.toContain('flows run --local-agent'); +}); + +it('says nothing about workers when the kernel is waiting for a human to recover the step', async () => { + const error = parkedForWantOfWorker(); + error.parkCause = 'needs_human'; + vi.mocked(executeDurableAuthoredFlow).mockRejectedValueOnce(error); + const result = await runDirectFlow('flow.ts', '{}', '/tmp/unused'); + expect(result.exitCode).toBe(3); + // Attaching a worker does not clear a manual-recovery wait, so recommending + // one would send someone to fix the wrong thing. + expect(result.report.diagnostics.at(-1)!.message).not.toContain('--local-agent'); +}); + +it('repeats the --input argument as given, including a file path', async () => { + const file = join(mkdtempSync(join(tmpdir(), 'flows-input-')), 'plan.json'); + writeFileSync(file, '{"plan":"v2"}'); + vi.mocked(executeDurableAuthoredFlow).mockRejectedValueOnce(parkedForWantOfWorker()); + // `--input` takes inline JSON *or* a file (direct-input.ts). Re-rendering the + // argument verbatim keeps the second run reading the same file; re-rendering + // the parsed value would inline a snapshot of it instead. + const result = await runDirectFlow('flow.ts', file, '/tmp/unused'); + expect(result.report.diagnostics.at(-1)!.message).toContain(`--input '${file}'`); +}); + it('reports the root separately from the child holding failure evidence', async () => { const error = new AuthoredFlowExecutionError('step_failed', 'child failed', undefined, 'child-run'); error.rootRunId = 'root-run'; diff --git a/packages/sdk/tests/local-agent-live.test.ts b/packages/sdk/tests/local-agent-live.test.ts index f818f1639..040b0bb21 100644 --- a/packages/sdk/tests/local-agent-live.test.ts +++ b/packages/sdk/tests/local-agent-live.test.ts @@ -53,9 +53,31 @@ function fixture(exitCode = 0, workspace?: string, delayMs = 0) { // Bound a stuck fixture process, allowing startup/preflight before the // kernel's independently enforced worker lease. UX timing is measured by // the separate empty-cache cold-start transcript, not this cleanup ceiling. - return { root, marker, invoke: (...flags: string[]) => spawnSync(process.execPath, - [cli, 'run', 'hello.flow.ts', '--input', '{}', '--local-agent', '--data-dir', join(root, 'data'), ...flags], - { cwd: root, encoding: 'utf8', timeout: 90000, env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }) }; + const spawn = (argv: string[]) => spawnSync(process.execPath, [cli, ...argv], + { cwd: root, encoding: 'utf8', timeout: 90000, env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }); + return { + root, marker, + invoke: (...flags: string[]) => spawn( + ['run', 'hello.flow.ts', '--input', '{}', '--local-agent', '--data-dir', join(root, 'data'), ...flags]), + /** The same flow, with a caller's `--input` and no worker offered: it parks. */ + park: (input: string, ...flags: string[]) => spawn( + ['run', 'hello.flow.ts', '--input', input, '--data-dir', join(root, 'data'), ...flags]), + resume: (runId: string, ...flags: string[]) => spawn( + ['resume', runId, '--data-dir', join(root, 'data'), ...flags]), + /** An input document on disk, named by the path this returns. */ + inputFile: (contents: string) => { + const path = join(root, 'input.json'); + writeFileSync(path, contents); + return path; + }, + /** + * A shell line, run as written. Used to execute a printed remedy verbatim — + * the only assertion that actually proves "runnable", since it exercises the + * quoting, the flag order and the argument values all at once. + */ + shell: (script: string) => spawnSync('sh', ['-c', script], + { cwd: root, encoding: 'utf8', timeout: 90000, env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }), + }; } describe('built CLI local agent against a real daemon', () => { @@ -85,6 +107,86 @@ describe('built CLI local agent against a real daemon', () => { expect(result.stderr).toContain('✗ agent-1'); expect(result.stderr).not.toContain('[agent: completed]'); }); + /** + * The headline complaint: an authored `.flow.ts` that parked at an `f.agent` + * step printed no remedy at all, because the bare `flows run --local-agent + * ` the spec path emitted would have been refused for want of + * `--input`. It read as "your infrastructure is missing a worker" when the + * actual fix was one flag away. + * + * "Runnable" is asserted by running it: the printed line goes back through a + * shell verbatim, so the flow path, the `--input` this run was started with + * and their quoting are all proven at once rather than string-matched. + */ + it('prints a remedy that runs the parked authored flow to completion', () => { + const f = fixture(); + // An input with a quote and a space: the two characters that make the + // difference between a copy-pasteable command and a broken one. + const input = '{"task":"it\'s a plan","n":1}'; + + const parked = f.park(input, '--json', '--no-observer-link'); + + expect(parked.status, parked.stderr + parked.stdout).toBe(3); + expect(JSON.parse(parked.stdout)).toMatchObject({ status: 'parked', parkCause: 'worker_unavailable' }); + const message = JSON.parse(parked.stdout).diagnostics.at(-1).message as string; + const remedy = /: (flows run [^\n]*?)\. [A-Z]/.exec(message)?.[1]; + expect(remedy, message).toBeDefined(); + expect(remedy).toContain("--local-agent 'hello.flow.ts'"); + expect(existsSync(f.marker)).toBe(false); + + const rerun = f.shell(remedy!.replace(/^flows /, + `${JSON.stringify(process.execPath)} ${JSON.stringify(cli)} `) + ' --json --no-observer-link'); + + expect(rerun.status, rerun.stderr + rerun.stdout).toBe(0); + expect(JSON.parse(rerun.stdout)).toMatchObject({ ok: true, status: 'completed', completionReason: 'success' }); + // The agent really ran. Exit 0 is also what proves the shell delivered the + // input unmangled: a quote lost in the round trip leaves invalid JSON, and + // a `.flow.ts` with unparseable `--input` is refused at exit 2. + expect(readFileSync(f.marker, 'utf8')).toBe('hello'); + }, 90_000); + + /** + * The same acceptance, one process further on: the run was started from an + * input FILE, so the resume refusal can only render the recorded input back + * inline — the journal keeps the document, not the word that carried it. + * + * A 300-byte document is ordinary and its inline form is not a legal + * filename: no path component may exceed 255 bytes, so `parseDirectInput`'s + * opening `stat` failed ENAMETOOLONG. Read as "could not be inspected", that + * turned the printed recovery command into an exit-2 `input_unreadable`, and + * a remedy that cannot be run is the same dead end as no remedy at all. + */ + it('prints a remedy that runs, for a run started from an input file too long to be one', () => { + const f = fixture(); + const task = 'x'.repeat(300); + const inputPath = f.inputFile(JSON.stringify({ task })); + + const parked = f.park(inputPath, '--json', '--no-observer-link'); + + expect(parked.status, parked.stderr + parked.stdout).toBe(3); + const rootRunId = JSON.parse(parked.stdout).rootRunId as string; + + // The obvious recovery, and the one the field report tried: resume the + // parked root with the flag. An authored root admits its worker at run + // start, so this is refused — with the new run to start instead. + const refused = f.resume(rootRunId, '--local-agent', '--json', '--no-observer-link'); + + expect(refused.status, refused.stderr + refused.stdout).toBe(2); + const message = JSON.parse(refused.stdout).diagnostics.at(-1).message as string; + const remedy = /: (flows run [^\n]*?)\. [A-Z]/.exec(message)?.[1]; + expect(remedy, message).toBeDefined(); + // Inline, because that is all the journal can give back — and longer than + // any filename, which is the whole of this regression. + expect(remedy).toContain(`--input '${JSON.stringify({ task })}'`); + + const rerun = f.shell(remedy!.replace(/^flows /, + `${JSON.stringify(process.execPath)} ${JSON.stringify(cli)} `) + ' --json --no-observer-link'); + + expect(rerun.status, rerun.stderr + rerun.stdout).toBe(0); + expect(JSON.parse(rerun.stdout)).toMatchObject({ ok: true, status: 'completed', completionReason: 'success' }); + expect(readFileSync(f.marker, 'utf8')).toBe('hello'); + }, 90_000); + it('refuses a workspace it cannot pin before invoking the agent', () => { const f = fixture(0, 'repo'); const result = f.invoke(); diff --git a/packages/sdk/tests/local-agent-remedy.test.ts b/packages/sdk/tests/local-agent-remedy.test.ts new file mode 100644 index 000000000..b5e9ffa10 --- /dev/null +++ b/packages/sdk/tests/local-agent-remedy.test.ts @@ -0,0 +1,160 @@ +import { execFileSync } from 'node:child_process'; +import { describe, expect, it } from 'vitest'; + +import { authoredInput, authoredWorkerRemedy, localAgentRemedy } from '../src/cli/local-agent-remedy.js'; +import { parseDirectInput } from '../src/direct-input.js'; + +/** + * A rendered remedy is only worth printing if a shell reproduces the exact + * arguments it was built from. `printf '%s\n'` on each word is the cheapest + * honest check: it makes the shell itself do the splitting and unquoting, so a + * path with a space, an apostrophe or a `$(...)` in it either round-trips or + * fails loudly here instead of in someone's terminal. + */ +function argv(command: string): string[] { + const out = execFileSync('sh', ['-c', `for word in ${command}; do printf '%s\\n' "$word"; done`], + { encoding: 'utf8' }); + return out.split('\n').slice(0, -1); +} + +/** The command inside a remedy clause: between `: ` and the sentence's `. `. */ +function command(clause: string): string { + return /: (flows [^\n]*?)\. [A-Z]/.exec(clause)![1]!; +} + +describe('localAgentRemedy', () => { + it('says nothing when there is nothing to say', () => { + expect(localAgentRemedy({ kind: 'none' })).toBe(''); + }); + + it('renders a runnable new run for a declarative spec', () => { + const clause = localAgentRemedy({ kind: 'spec-run', path: 'flows/hello.flow.yaml', dataDir: '/tmp/d' }); + // The prefix people already grep for, byte for byte, with the data dir + // appended: `parseRunArgs` is order-insensitive, so this still parses. + expect(clause).toContain(`flows run --local-agent 'flows/hello.flow.yaml' --data-dir /tmp/d`); + expect(argv(command(clause))) + .toEqual(['flows', 'run', '--local-agent', 'flows/hello.flow.yaml', '--data-dir', '/tmp/d']); + }); + + it('points a parked declarative resume at this run, not a new one', () => { + const clause = localAgentRemedy({ kind: 'spec-resume', runId: '01RUN', dataDir: '/tmp/d' }); + // A resumable run is continued, not restarted: naming `flows run` here + // would throw away everything the run already journaled. + expect(clause).toContain('flows resume --data-dir /tmp/d --local-agent 01RUN'); + expect(clause).not.toContain('flows run'); + }); + + it('never tells an already-attached caller to pass the same flag again', () => { + const clause = localAgentRemedy({ kind: 'attached' }); + expect(clause).toContain('already attached'); + expect(clause).not.toMatch(/flows (run|resume)/); + // The honest report: a worker was offered and none of them was eligible. + expect(clause).toContain('no attached worker was eligible'); + }); + + it('carries the authored --input through the shell unchanged', () => { + const argument = '{"plan":"v2","note":"it\'s fine","shell":"$(rm -rf /)"}'; + const clause = localAgentRemedy({ + kind: 'authored-run', path: "my flows/a b's.flow.ts", input: { kind: 'inline', argument }, + }); + expect(argv(command(clause))) + .toEqual(['flows', 'run', '--local-agent', "my flows/a b's.flow.ts", '--input', argument]); + }); + + /** + * A resume renders the recorded input back as inline JSON, and the journal + * keeps everything the run was started with — so this clause routinely + * carries far more than a filename's worth of bytes. `parseDirectInput` + * `stat`s the argument first, and a `stat` of a 300-byte word fails + * ENAMETOOLONG rather than ENOENT; treating that as "could not be inspected" + * made the printed remedy fail at exit 2 instead of attaching a worker. + */ + it('renders an input longer than a filename as a word both the shell and the parser accept', () => { + const argument = JSON.stringify({ task: 'x'.repeat(300) }); + expect(argument.length).toBeGreaterThan(255); + + const clause = localAgentRemedy({ + kind: 'authored-run', path: 'a.flow.ts', input: { kind: 'inline', argument }, + }); + + const words = argv(command(clause)); + expect(words).toEqual(['flows', 'run', '--local-agent', 'a.flow.ts', '--input', argument]); + // The parser the printed command actually reaches, on the word the shell + // actually delivers: it must read as inline JSON, not as an unreadable file. + expect(parseDirectInput(words.at(-1)!)).toEqual({ task: 'x'.repeat(300) }); + }); + + it('states the requirement rather than inventing an input it does not have', () => { + const clause = localAgentRemedy({ kind: 'authored-run', path: 'a.flow.ts', input: { kind: 'absent' } }); + expect(clause).toContain('no input argument to repeat here'); + // It names `--input` as a requirement but never supplies a value: `{}` + // would start a different invocation than the one that parked, and a + // `` would be a shell redirect rather than an argument. With + // no value there is no command to print either. + expect(clause).not.toMatch(/--input '/); + expect(clause).not.toContain('flows run'); + }); + + /** + * `MAX_DIRECT_INPUT_BYTES` admits a mebibyte; Linux `execve` admits 128KiB + * in one argument. An input file between the two is ordinary, and printing + * its contents back inline yields a command that dies with `Argument list + * too long` before the CLI is reached. + */ + it('refuses to print an input larger than a shell argument, and says so', () => { + const clause = localAgentRemedy({ + kind: 'authored-run', path: 'a.flow.ts', input: { kind: 'oversized', bytes: 200_000 }, + }); + expect(clause).toContain('200000 bytes of input'); + expect(clause).toContain('pass the input file this run was started from'); + expect(clause).not.toMatch(/--input '/); + expect(clause).not.toContain('flows run --local-agent'); + }); +}); + +describe('authoredInput', () => { + it('keeps an argument a shell can carry', () => { + expect(authoredInput('{"plan":"v2"}')).toEqual({ kind: 'inline', argument: '{"plan":"v2"}' }); + }); + + it('reports an absent argument as absent rather than empty', () => { + expect(authoredInput(undefined)).toEqual({ kind: 'absent' }); + }); + + /** + * The boundary is measured in bytes, not characters: `execve` counts bytes, + * so a multi-byte document has to be weighed the way the kernel weighs it. + */ + it('classifies by encoded bytes at the execve limit', () => { + expect(authoredInput('x'.repeat(131_071))).toMatchObject({ kind: 'inline' }); + expect(authoredInput('x'.repeat(131_072))).toEqual({ kind: 'oversized', bytes: 131_072 }); + // 65_536 two-byte characters is 131_072 bytes: over the limit despite + // being half its length in UTF-16 code units. + expect(authoredInput('é'.repeat(65_536))).toEqual({ kind: 'oversized', bytes: 131_072 }); + }); +}); + +describe('authoredWorkerRemedy', () => { + const run = { path: 'a.flow.ts', input: { kind: 'inline', argument: '{}' } as const, dataDir: '/tmp/d' }; + + it('answers a worker park with a new run', () => { + expect(authoredWorkerRemedy('worker_unavailable', false, run)) + .toEqual({ kind: 'authored-run', ...run }); + }); + + it('answers an attached worker park without repeating the flag', () => { + expect(authoredWorkerRemedy('worker_unavailable', true, run)).toEqual({ kind: 'attached' }); + }); + + /// Attaching a worker does not clear the kernel's manual-recovery wait, and + /// an unestablished cause is not evidence of one. Both stay silent rather + /// than sending someone to fix a thing that is not broken. + it.each([['needs_human'], [undefined]] as const)('says nothing for %s', (cause) => { + expect(authoredWorkerRemedy(cause, false, run)).toEqual({ kind: 'none' }); + }); + + it('says nothing when the flow path is unknown', () => { + expect(authoredWorkerRemedy('worker_unavailable', false, { input: { kind: 'absent' } })) + .toEqual({ kind: 'none' }); + }); +}); diff --git a/packages/sdk/tests/resume-local-agent.test.ts b/packages/sdk/tests/resume-local-agent.test.ts new file mode 100644 index 000000000..8246cda1d --- /dev/null +++ b/packages/sdk/tests/resume-local-agent.test.ts @@ -0,0 +1,255 @@ +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { AuthoredFlowExecutionError } from '../src/authored-flow-error.js'; +import * as authoredRoot from '../src/authored-root.js'; +import { resumeFlow } from '../src/cli/run.js'; +import { socketPathFor } from '../src/daemon-connection.js'; +import * as daemonLifecycle from '../src/daemon-lifecycle.js'; +import { JournalClient } from '../src/journal-client.js'; +import * as localAgent from '../src/local-agent.js'; +import { PROTOCOL_VERSION } from '../src/protocol.js'; + +const RUN_ID = '01M2NDS2RYK3MH6YBSB1CHA9SE'; +const SHA = 'a'.repeat(64); +const SURFACE = { + packageName: '@relayflows/surface', version: '2.0.0', + packageSha256: SHA, runtimeSha256: SHA, +}; + +const directories: string[] = []; + +afterEach(() => { + vi.restoreAllMocks(); + for (const dir of directories.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +/** + * A daemon whose journal holds one authored root, scripted as this suite needs + * it: the pinned worker stream, and the input the root was started with. + * + * The metadata is built as the real thing — `readAuthoredRootMetadata` parses + * and validates the `run.spawned` step instruction, so a stubbed + * `journalRead` exercises the same admission the daemon would. Mocking + * `authored-root.js` would not reach `cli/run.ts`'s own import of it (see + * resume-failure.test.ts), and would silently skip the branch under test. + */ +function daemonWithAuthoredRoot( + root: { localAgentStream?: string; input?: unknown; inputPresent?: boolean } = {}, +): { dataDir: string; attached: () => number; resumed: () => number } { + const dir = mkdtempSync(join(tmpdir(), 'flows-resume-agent-')); + directories.push(dir); + let attached = 0; + let resumed = 0; + const metadata = { + kind: 'relayflows.authored-root.v1', + flowName: 'ship', flowPath: '/work/my flows/ship.flow.ts', sourceSha256: SHA, + surface: SURFACE, sources: [{ path: '/work/my flows/ship.flow.ts', sourceSha256: SHA, surface: SURFACE }], + inputPresent: root.inputPresent ?? true, + ...(root.inputPresent === false ? {} : { input: root.input ?? { plan: 'v2' } }), + ...(root.localAgentStream === undefined ? {} : { localAgentStream: root.localAgentStream }), + }; + vi.spyOn(daemonLifecycle, 'ensureDaemon').mockResolvedValue({ + kind: 'attached', socketPath: socketPathFor(dir), connection: null, + }); + vi.spyOn(JournalClient.prototype, 'connect').mockResolvedValue(undefined); + vi.spyOn(JournalClient.prototype, 'hello').mockResolvedValue({ + protocol: PROTOCOL_VERSION, server: 'relayflowd-test', + }); + vi.spyOn(JournalClient.prototype, 'close').mockReturnValue(undefined); + vi.spyOn(JournalClient.prototype, 'journalRead').mockResolvedValue({ + entries: [{ entry_type: 'run.spawned', payload: { spec: { steps: [{ + id: 'authored-root', type: 'agent', instruction: JSON.stringify(metadata), + surfaces: { streams: [{ stream: 'authored-root-01H' }] }, + }] } } }], + } as never); + vi.spyOn(JournalClient.prototype, 'runResume').mockImplementation(async () => { + resumed += 1; + throw new Error('runResume should not be reached by a refused resume'); + }); + vi.spyOn(localAgent, 'attachLocalAgent').mockImplementation(async () => { + attached += 1; + throw new Error('attachLocalAgent should not be reached by a refused resume'); + }); + return { dataDir: dir, attached: () => attached, resumed: () => resumed }; +} + +describe('flows resume --local-agent against an authored root', () => { + /** + * The complaint this change answers: the flag was accepted, ignored, and the + * run parked again with a byte-identical message. It is now a refusal that + * names what to do instead — and it refuses before anything is attached or + * journaled, so the run is left exactly as it was. + */ + it('refuses --local-agent on a run that was started without one, naming the new run to start', async () => { + const daemon = daemonWithAuthoredRoot({ input: { plan: 'v2' } }); + + const result = await resumeFlow(RUN_ID, daemon.dataDir, { localAgent: true }); + + expect(result.exitCode).toBe(2); + expect(result.report.diagnostics.at(-1)).toMatchObject({ + severity: 'refusal', kind: 'local_agent_unavailable', + }); + const message = result.report.diagnostics.at(-1)!.message; + expect(message).toContain('--local-agent is admitted at run start; start a new run'); + // Runnable, with the flow path quoted and the root's own input repeated. + expect(message).toContain( + `flows run --local-agent '/work/my flows/ship.flow.ts' --input '{"plan":"v2"}'`); + // Refused means refused: no worker, no journal write. + expect(daemon.attached()).toBe(0); + expect(daemon.resumed()).toBe(0); + }); + + it('refuses a resume that drops the flag the root was pinned with', async () => { + const daemon = daemonWithAuthoredRoot({ localAgentStream: 'local-agent-0a1b2c3d' }); + + const result = await resumeFlow(RUN_ID, daemon.dataDir, {}); + + expect(result.exitCode).toBe(2); + expect(result.report.diagnostics.at(-1)).toMatchObject({ kind: 'local_agent_unavailable' }); + // This run IS resumable with the flag, so the remedy is this run, not a + // new one — the opposite instruction to the case above. + expect(result.report.diagnostics.at(-1)!.message) + .toContain(`flows resume --data-dir ${daemon.dataDir} --local-agent ${RUN_ID}`); + expect(daemon.attached()).toBe(0); + expect(daemon.resumed()).toBe(0); + }); + + /** + * The refusal must be a refusal, not a relabelled protocol failure: these + * used to throw bare `Error`s and surface as `protocol_error` with + * `RUN unknown`, which blamed the daemon for an invocation mistake. + */ + it('does not report either refusal as a protocol failure', async () => { + for (const [root, options] of [ + [{}, { localAgent: true }], + [{ localAgentStream: 'local-agent-0a1b2c3d' }, {}], + ] as const) { + const daemon = daemonWithAuthoredRoot(root); + const result = await resumeFlow(RUN_ID, daemon.dataDir, options); + expect(result.report.diagnostics.map(diagnostic => diagnostic.kind)) + .not.toContain('protocol_error'); + expect(JSON.stringify(result.report)).not.toContain('could not complete the resume request'); + vi.restoreAllMocks(); + } + }); + + /** + * The refusal renders the recorded input back inline, because the journal + * keeps the document and not the `--input` word that carried it. An input + * this run may legitimately have been started with — `parseDirectInput` + * admits a mebibyte — can exceed what `execve` carries in one argument + * (`MAX_ARG_STRLEN`, 128KiB), and a printed command that dies with + * `Argument list too long` is no remedy. State the requirement instead. + */ + it('states the input requirement when the recorded input outgrows a shell argument', async () => { + const daemon = daemonWithAuthoredRoot({ input: { task: 'x'.repeat(200_000) } }); + + const result = await resumeFlow(RUN_ID, daemon.dataDir, { localAgent: true }); + + expect(result.exitCode).toBe(2); + const message = result.report.diagnostics.at(-1)!.message; + expect(message).toContain('--local-agent is admitted at run start; start a new run'); + expect(message).toContain('bytes of input, more than a shell can carry in one argument'); + // No command rather than an unrunnable one — and the refusal must not + // itself grow to the size of the input it is describing. + expect(message).not.toMatch(/--input '/); + expect(message.length).toBeLessThan(1_000); + }); + + it('states the input requirement when the root recorded no input to repeat', async () => { + const daemon = daemonWithAuthoredRoot({ inputPresent: false }); + + const result = await resumeFlow(RUN_ID, daemon.dataDir, { localAgent: true }); + + expect(result.exitCode).toBe(2); + // Never `--input '{}'`: that would name a different invocation of the flow + // than the one this run was started with. + expect(result.report.diagnostics.at(-1)!.message).toContain('no input argument to repeat here'); + expect(result.report.diagnostics.at(-1)!.message).not.toMatch(/--input '/); + }); +}); + +/** + * An authored resume that reaches an agent step with no worker to run it. + * `resumeFlow` used to leave this on `protocolFailure`; even once classified, + * the park said nothing about the fix, and the fix is not "resume again" — + * an authored root admits its worker at run start. + */ +function daemonParkingAuthoredResume(parkCause?: 'worker_unavailable' | 'needs_human'): string { + const dir = mkdtempSync(join(tmpdir(), 'flows-resume-park-')); + directories.push(dir); + const metadata = { + kind: 'relayflows.authored-root.v1', + flowName: 'ship', flowPath: '/work/ship.flow.ts', sourceSha256: SHA, + surface: SURFACE, sources: [{ path: '/work/ship.flow.ts', sourceSha256: SHA, surface: SURFACE }], + inputPresent: true, input: { plan: 'v2' }, + }; + vi.spyOn(daemonLifecycle, 'ensureDaemon').mockResolvedValue({ + kind: 'attached', socketPath: socketPathFor(dir), connection: null, + }); + vi.spyOn(JournalClient.prototype, 'connect').mockResolvedValue(undefined); + vi.spyOn(JournalClient.prototype, 'hello').mockResolvedValue({ + protocol: PROTOCOL_VERSION, server: 'relayflowd-test', + }); + vi.spyOn(JournalClient.prototype, 'close').mockReturnValue(undefined); + vi.spyOn(JournalClient.prototype, 'journalRead').mockResolvedValue({ + entries: [{ entry_type: 'run.spawned', payload: { spec: { steps: [{ + id: 'authored-root', type: 'agent', instruction: JSON.stringify(metadata), + surfaces: { streams: [{ stream: 'authored-root-01H' }] }, + }] } } }], + } as never); + // The park surfaces as the authored error the child's classification threw. + // Injected at the driver rather than deeper down: the unit under test is + // `resumeFlow`'s catch, which inspects only the error — and the real driver + // would first try to load the pinned source this fixture does not have. + vi.spyOn(authoredRoot, 'resumeDurableAuthoredFlow').mockImplementation(async () => { + const error = new AuthoredFlowExecutionError('agent_parked', + `Run "child-run" parked at step "agent-1" (agent): no worker is attached for step type "agent".`, + undefined, 'child-run'); + error.parkCause = parkCause; + throw error; + }); + return dir; +} + +describe('an authored resume that parks for want of a worker', () => { + it('reports the park with a runnable new run, not another identical resume', async () => { + const dir = daemonParkingAuthoredResume('worker_unavailable'); + + const result = await resumeFlow(RUN_ID, dir, {}); + + expect(result.exitCode).toBe(3); + expect(result.report.status).toBe('parked'); + expect(result.report.parkCause).toBe('worker_unavailable'); + // The child holds the evidence; the root this resume named stays separate. + expect(result.report.runId).toBe('child-run'); + expect(result.report.rootRunId).toBe(RUN_ID); + const message = result.report.diagnostics.at(-1)!.message; + expect(message).toContain(`flows run --local-agent '/work/ship.flow.ts' --input '{"plan":"v2"}'`); + // Telling someone to resume again is what produced the identical second + // park this whole change exists to stop. + expect(message).not.toContain('flows resume'); + }); + + it('says nothing about workers when a human has to recover the step', async () => { + const dir = daemonParkingAuthoredResume('needs_human'); + + const result = await resumeFlow(RUN_ID, dir, {}); + + expect(result.exitCode).toBe(3); + expect(result.report.diagnostics.at(-1)!.message).not.toContain('--local-agent'); + }); + + it('says nothing when the cause was never established', async () => { + const dir = daemonParkingAuthoredResume(undefined); + + const result = await resumeFlow(RUN_ID, dir, {}); + + expect(result.exitCode).toBe(3); + // Fail closed: an unclassified park gets no guess. + expect(result.report.diagnostics.at(-1)!.message).not.toContain('--local-agent'); + }); +}); diff --git a/packages/sdk/tests/yaml-local-agent-live.test.ts b/packages/sdk/tests/yaml-local-agent-live.test.ts index 1e0844aa7..b2cfa0dd4 100644 --- a/packages/sdk/tests/yaml-local-agent-live.test.ts +++ b/packages/sdk/tests/yaml-local-agent-live.test.ts @@ -48,12 +48,24 @@ function fixture(source: 'step' | 'named' | 'flow' | 'project' = 'step', instruc steps: [step], }; writeFileSync(join(root, 'flows/hello.flow.yaml'), stringify(spec)); - // Run from outside the flow directory to exercise checked relative CLI binding. - return { root, invoke: (localAgent = true) => spawnSync(process.execPath, [ - cli, 'run', 'flows/hello.flow.yaml', '--json', '--no-observer-link', - '--data-dir', join(root, 'data'), ...(localAgent ? ['--local-agent'] : []), + const flows = (argv: string[]) => spawnSync(process.execPath, [ + cli, ...argv, '--json', '--no-observer-link', '--data-dir', join(root, 'data'), ], { cwd: root, encoding: 'utf8', timeout: 30_000, - env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }) }; + env: { ...process.env, RELAYFLOWD_BIN: relayflowd } }); + // Run from outside the flow directory to exercise checked relative CLI binding. + return { + root, + invoke: (localAgent = true) => + flows(['run', 'flows/hello.flow.yaml', ...(localAgent ? ['--local-agent'] : [])]), + resume: (runId: string, localAgent = true) => + flows(['resume', ...(localAgent ? ['--local-agent'] : []), runId]), + }; +} + +/** The `run_parked` diagnostic of a `--json` report, as the CLI rendered it. */ +function parkMessage(result: { stdout: string }): string { + const report = JSON.parse(result.stdout) as { diagnostics: Array<{ kind: string; message: string }> }; + return report.diagnostics.find(diagnostic => diagnostic.kind === 'run_parked')!.message; } describe('YAML --local-agent through the built CLI and real daemon', () => { @@ -86,6 +98,56 @@ describe('YAML --local-agent through the built CLI and real daemon', () => { expect(result.stderr).toContain("flows run --local-agent 'flows/hello.flow.yaml'"); }); + /** + * The reported defect, end to end: `flows resume` advertises `[--local-agent]` + * (cli.ts usage), so on a declarative run the flag must MOVE the run — not be + * accepted and ignored, leaving a second park whose message is byte-identical + * to the first. The field report lost a cycle to exactly that. + * + * The `run` park keeps naming a NEW run, unchanged and pinned by the test + * above; resuming this one with the flag is the second documented route, and + * it is the one that had never been exercised end to end. + */ + it('resumes a parked spec run with --local-agent instead of parking identically again', () => { + const f = fixture(); + const parked = f.invoke(false); + expect(parked.status, parked.stderr + parked.stdout).toBe(3); + const { runId } = JSON.parse(parked.stdout); + const first = parkMessage(parked); + + const resumed = f.resume(runId); + + expect(resumed.status, resumed.stderr + resumed.stdout).toBe(0); + expect(JSON.parse(resumed.stdout)).toMatchObject({ + ok: true, runId, status: 'completed', completionReason: 'success', + }); + // The acceptance criterion, stated as itself: whatever happened, it was not + // the same park a second time. + expect(resumed.stdout).not.toContain(first); + }); + + /** + * And when the flag genuinely cannot help — the declared workspace surface no + * local worker holds — the second message must still differ from the first, + * because the honest report is that a worker WAS offered and none was + * eligible. "Pass --local-agent" to someone who just passed it is the failure + * mode this whole change exists to remove. + */ + it('parks a second time with a different message when the attached worker is not eligible', () => { + const f = fixture('step', 'hello', true); + const parked = f.invoke(false); + expect(parked.status, parked.stderr + parked.stdout).toBe(3); + const { runId } = JSON.parse(parked.stdout); + + const resumed = f.resume(runId); + + expect(resumed.status, resumed.stderr + resumed.stdout).toBe(3); + const second = parkMessage(resumed); + expect(second).not.toBe(parkMessage(parked)); + expect(second).toContain('no attached worker was eligible for this step'); + expect(second).not.toMatch(/flows (run|resume)/); + }); + it('reports the agent process failure', () => { const result = fixture('step', 'fail').invoke(); expect(result.status, result.stderr + result.stdout).toBe(1); diff --git a/summary.md b/summary.md deleted file mode 100644 index e92fc88ed..000000000 --- a/summary.md +++ /dev/null @@ -1,204 +0,0 @@ -# Spend the analysis `flows check` already does - -`flows check` computed the facts that would have prevented five dead runs and -said nothing about them. This spends two of them, and establishes that the -third fact is not a fact. - -## What changed - -### `agent_worker_unresolved` — a check-only warning (permanent) - -A spec with `agent` steps needs a worker *attached for step type `agent`*. -Without one the run parks at the first such step. `flows check` already walks -those steps to print `REQUIRES codex (step "implement"), …` — the analysis -exists; it just stopped one sentence short of the remedy. - -``` -REQUIRES claude (step "implement") -WARNING [agent_worker_unresolved] 2 agent steps ("implement", "review") require an -attached worker. For a local run, use `flows run --local-agent `, unless you -already attach an agent worker for this daemon. `flows check` does not verify -worker attachment: with no worker attached the run parks at the first agent step. -``` - -Design decisions worth naming: - -- **Warning, never a refusal, worded as a requirement rather than a - prediction.** Worker attachment is *unknown* to `flows check`, not absent: - `check` is daemon-free by construction, so it cannot see a worker attached - elsewhere. Saying "this will park" would be a guess. -- **It is a property of the invocation, not of the spec,** so it opts in - through a new `CheckInvocation` seam on `checkFlow` / `checkAuthoredFlow` - rather than entering `preflight`, which stays a pure function of the spec - plus environment probes. Only `flows check` sets it. `flows run` knows the - answer (it attaches its own worker under `--local-agent`), `flows build` and - `flows deploy` check a spec that will run elsewhere, and an SDK caller - reaching `checkAuthoredFlow` directly is generally running a worker already. -- **Scoped to `agent` steps.** `--local-agent` attaches no `llm` worker on the - YAML path, so counting `llm` steps and naming that flag for them would be - false. They are not counted and not mentioned. -- **Counted from the compiled steps, not from `requirements`.** A YAML helper - step (`slack: { post: … }`) compiles to an `agent` step carrying a helper - envelope and needs the same SDK worker, but `requirements` reports it as an - integration, dedupes by harness, and includes `llm` use. Compilation is the - only walk that sees every step that needs the worker. -- **Emitted in the `REQUIRES` position,** because it reads as a footnote to - that line. `emitCheckReport` holds it back from the leading diagnostic batch - and emits it once, after `REQUIRES` and before `CHECK PASSED`; diagnostics - stay on stderr, report lines stay on stdout. `--json` returns before any of - that and keeps the single ordered `diagnostics` array. -- **Emitted alongside a preflight refusal.** An environment refusal is fixed - and rerun; the worker question is still open on the next pass, and staying - silent about it is what made an author meet it one dead run at a time. - -Known blind spot, documented rather than papered over: an authored `.flow.ts` -is checked through `checkMcpHeader`, which preflights a synthetic one-step -header spec without compiling the body, so there are no agent steps to count. -Same blind spot as `permissions_unenforced`. - -### `gate_path_unscanned` — a preflight warning (temporary, retires with #513) - -An `artifact_exists` gate reads the journaled `output.artifacts` list and -nothing else. The bundled worker's *scan* skips any entry whose name starts -with `.` and any entry named exactly `node_modules`, so a gate on a path inside -one of those prefixes cannot rest on the scan, however faithfully the agent -writes the file: - -``` -WARNING [gate_path_unscanned] Step "review" gates on artifact_exists path -".workflow-artifacts/rust/review.md", but the bundled agent worker's artifact scan -records nothing under ".workflow-artifacts": it skips entries whose name starts with -"." and entries named "node_modules". The gate reads the journaled output.artifacts -and never the disk, so writing the file is not enough: the step has to report the -path itself — as object-shaped JSON stdout carrying its own "artifacts" array, as a -completed Relay task output, or from a custom worker. If the gate is meant to rest -on the scan, write the artifact to a path the scan records. -``` - -The issue asked for a refusal, on the premise that such a gate is statically -unsatisfiable. That premise is false, and review F1 is right to reject it: the -scan is only one writer of `output.artifacts`. The same **bundled** worker -promotes object-shaped JSON stdout — and a completed Relay task's output — -verbatim into `output` (`worker.ts`), so an agent that answers with its own -`{"artifacts": [...]}` puts a hidden path in the list and the gate passes; and -`step.complete` accepts any `output` from any worker. Which route a step takes -is a run-time fact, so this reports the scan's limitation and leaves the verdict -to the run. A regression runs the real `AgentWorker` over a fake `claude` that -writes `.workflow-artifacts/review.md` *and* reports it as JSON, then runs the -real lowered gate command over exactly what the worker journaled: the scan -records nothing, the gate exits 0. A refusal would have rejected that spec at -`check`, `run`, `build` and every SDK submission. - -- **One rule, two consumers.** The exclusion predicate moved out of - `agent-artifacts.ts` into `src/artifact-scan-policy.ts`; the real scan now - consumes it, so the warning cannot drift from the scan it describes. A - parity test writes all twelve case paths to a temp dir and asserts the real - `snapshotWorkspaceFiles` output is exactly the predicate's complement. -- **The whole excluded prefix is named,** not the offending segment alone — - that prefix is the directory an author relying on the scan has to move the - artifact out of. -- **Segments are compared exactly, with no normalization.** The gate matches - the author's literal string against the worker's literal list, so - `node_modules-copy/out.md`, `reports/node_modules.md`, `review.md` and - `reports/v1.2/review.md` say nothing, and a backslash is an ordinary - filename character rather than a separator. -- **Collected before preflight's early returns.** `probeNamedGate` runs after - CLI resolution, model governance, scope and budget can each return, so an - author with an unresolved CLI would not learn about the unscanned path until - a later pass — or at run time. A test pins the diagnostic order as - `['gate_path_unscanned', 'cli_unresolved']`. -- **It lives in `PREFLIGHT_WARNING_KINDS`,** so it travels with every public - preflight consumer — `flows check`, `run`, `build` and SDK submissions — and - refuses none of them. - -### `gate_output_not_captured` — **not added**, because it is not true - -The acceptance criterion was conditional: warn that a `subprocess_gate`'s -output is not captured *"for as long as that is true"*. It is not true today. -Verified against a live `relayflowd` before writing anything: - -``` -"stepId": "emit.gate", "completionReason": "retries_exhausted", "exitCode": 1, -"stdoutTail": "GATE_STDOUT_MARKER\n", "stderrTail": "GATE_STDERR_MARKER\n" -``` - -and in the journal itself, on the lowered gate step's `step.completed`: - -```json -"output": { "exit_code": 1, "stdout_tail": "GATE_STDOUT_MARKER\n", - "stderr_tail": "GATE_STDERR_MARKER\n" } -``` - -A warning claiming otherwise would have been a false diagnostic added to a -change whose whole point is that `check` should only say what it knows. What -the criterion actually asks for is that the warning exist exactly while the -bug does — so instead of the warning, this adds a **live-kernel regression -that pins the capture**. If capture ever regresses, that test fails, and the -warning becomes warranted at the moment it becomes true. - -## Tests - -- `tests/artifact-gates.test.ts` — six parameterised warnings asserting the - full excluded prefix is named and that none of them refuses; six lookalike - paths that must say nothing; the scan/predicate parity test against a real - `snapshotWorkspaceFiles` run; the early-return ordering test; an end-to-end - `checkFlow` case pinning the warning and the unrelated refusal beside it; - and the F1 regression described above, which drives the real `AgentWorker` - and the real lowered gate command over an excluded path the agent reports - itself. -- `tests/check-worker-surface.test.ts` (new) — message content, singular vs - plural and the "and N more" truncation, exit code 0, the YAML helper step - being counted where `requirements` omits it, silence for `llm`-only and - deterministic-only flows, survival alongside a preflight refusal, opt-in - discipline, `--json` emitting it once in both the payload and on stderr, and - an **ordered `CliIo` transcript** (both streams in one list) pinning the - position after `REQUIRES` and before `CHECK PASSED` — in both the - `REQUIRES`-present and `REQUIRES`-absent shapes. -- `tests/preflight.test.ts` — `gate_path_unscanned` added to the - warning-reachability list, which asserts set equality against - `PREFLIGHT_WARNING_KINDS`. -- `tests/live-kernel.test.ts` (new block) — the `subprocess_gate` capture - regression, asserting both the rendered message and the journal payload, with - a silent-gate control so the assertions cannot pass on a fixed string. - -`npm test` in `packages/sdk`: 2381 passed, 40 failed. All 40 failures are -pre-existing and environmental — they reproduce identically on a stashed tree -(same 40 failures, same 7 files): `relayflowd` looked up under -`kernel/target/{debug,release}/` while this toolchain builds outside the repo, -and flows needing real harness CLIs. `tsc --noEmit` is clean for `src`, the -type tests, and `tests`. - -## Files - -| File | Change | -| --- | --- | -| `src/artifact-scan-policy.ts` | new — the scan's exclusion rule as a pure predicate, shared | -| `src/named-gate-preflight.ts` | new — the `gate_path_unscanned` warning (retires with #513) | -| `src/cli/check-worker-surface.ts` | new — the `agent_worker_unresolved` warning | -| `src/agent-artifacts.ts` | consumes the extracted predicate instead of its own copy | -| `src/preflight.ts` | collects gate scan coverage before every early return | -| `src/failure-kinds.ts` | the two new kinds, each with its retirement note | -| `src/cli/check.ts` | `CheckInvocation` opt-in seam | -| `src/cli.ts` | opts in at the `check` dispatch; defers the warning to the `REQUIRES` position | -| `docs/SURFACE.md` | documents both kinds where the facts they describe already live | - -Each diagnostic is a distinct kind, so it can be suppressed and later deleted -on its own. Both temporary kinds carry their retirement condition in a comment -at the definition site. - -## Out of scope, unchanged - -The underlying bugs (#511, #513) are not fixed, and the spec is not -round-tripped past the daemon's validator (#502). - -## Merge with origin/main (conflict resolution note) - -Main deliberately broadened the artifact scan so dot-directories like -`.workflow-artifacts/` are journaled and gate-able; only exact names `.git`, -`.relayflowd` and `node_modules` are skipped, at any depth. This branch's -shared `artifact-scan-policy.ts` predicate now encodes THAT set (not the older -dot-prefix rule), so `gate_path_unscanned` warns only on paths the merged scan -can never record — `node_modules/**`, `.git/**`, `.relayflowd/**` — and the -`.workflow-artifacts/` gates main enabled produce no warning. `cli.ts` keeps -both the `agent_worker_unresolved` deferral and main's model-provenance -helper; `agent-artifacts.ts` keeps the shared-predicate import.