diff --git a/docs/CLOUD.md b/docs/CLOUD.md index 8b96554af..364d2d8c6 100644 --- a/docs/CLOUD.md +++ b/docs/CLOUD.md @@ -270,6 +270,27 @@ step rows because the run record carries no total, and Cloud stores each step's cost as a float — unlike the local view, which adds the journal's decimal strings exactly (`run-state.ts`). +A failed run also prints its `error`, which is where an authored verdict's +detail arrives. A flow that ends `done("step_failed", { detail })` puts that +sentence in the run report's `step_failed` diagnostic message, and wherever +Cloud's run record carries that message as the run's `error`, +`flows status --cloud` renders it under an `error` heading, redacted again on +the way out. The message is deliberately one line even when the detail the +flow passed had line breaks in it — the escapes `\n`, `\r`, `\t` and +`\uXXXX` are how the breaks appear — because this view elides the middle of a +long multi-line error, and a forty-line detail rendered as forty lines would +lose exactly the finding it exists to carry. The unescaped detail is in the +report's own `completionDetail` and in the diagnostic's `detail`. See +docs/SURFACE.md for the bound and the redaction. + +The step between those two — report diagnostic to stored `error` — is the +server's, and this repository does not establish it. What is pinned here is +the client half: the message this CLI produces, and the rendering +`flows status --cloud` gives an `error` that holds it (`cloud-read.test.ts`, +which injects the diagnostic into a mocked Cloud record). Treat the hosted end +to end as unconfirmed until a hosted run or the server source says otherwise; +that is an evidence limit, not a claim that Cloud drops the field. + Every refusal is one `REFUSED [code] message` line naming what to do next: | code | when | diff --git a/docs/SURFACE.md b/docs/SURFACE.md index 4fc9f3565..fc50f0927 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -1080,6 +1080,79 @@ terminal markers are steps that SUCCEED: `done("step_failed")` is the body's verdict, not a step that failed, so the terminal marker does not fabricate a failing step. Actual step failures still take precedence over authored verdicts. +### Saying why: `done(reason, { detail })` + +`done()` takes an optional second argument, `{ detail }`, and it is what a +reader gets instead of a generic sentence. A flow that knows "one P2 remains: +`review.clean` was not created" should say so; without it the run record has +only "its own checks did not pass". + +```ts +f.done("step_failed", { detail: "review found 1 P2: `review.clean` was not created" }); +``` + +The detail is normalized once, at `done()`, before anything durable is written: + +- **Redacted** with the SDK's existing redactor — known token shapes, named + credential fields, and the values of secret-looking environment variables. + That is a policy, not a promise to recognise every possible secret. +- **Bounded** to 2,000 Unicode code points *including* the fixed + `… (truncated)` suffix, exported as `COMPLETION_DETAIL_MAX_CODE_POINTS`. + Over-long details are truncated with that visible marker rather than + refused: killing a twenty-step run at its last line because its explanation + ran long destroys more evidence than it preserves. Redaction runs BEFORE + truncation, because truncating first can split a token so no pattern matches + it any more — a truncation that *causes* a leak. +- **Normalized to absence** when it is empty or whitespace-only. No options, + `{}`, `{ detail: undefined }` and `{ detail: " " }` all mean the same thing + as the one-argument call, down to a byte-identical marker command. +- **Made well-formed**: every lone UTF-16 surrogate becomes U+FFFD. A JS + string is code units, not text, and `(prose + "\u{1F642}").slice(0, -1)` — + ordinary trimming of an agent's output — leaves a high surrogate with no + partner. The journal protocol's JSON decoder refuses such a value, and the + refusal carries no request id to answer, so the call never returns. One code + unit is substituted for one, so the bound still counts what a reader counts. + +A `detail` that is present and not a string, or options that are not an +object, are refused with `unsupported_completion`. The refusal names the type +it received and never the value. + +With a detail, the marker's stdout is +`{"completionReason":"","detail":""}`; with none it is exactly +the `{"completionReason":""}` it has always been, so a flow that does +not opt in keeps its `spec_hash`. The detail is journaled on the authored +root's output, returned by `flows run --json` as `completionDetail` and on the +`step_failed` diagnostic's `detail`, and printed by `flows status` as a +labelled line beside the kernel's own account: + +```text +RUN 9e1a0f2c-… software-factory completed started 20.0s ago finished success spend … +authored done("step_failed"): review found 1 P2: `review.clean` was not created +steps 1: 1 done +``` + +A verdict that carries a detail is **committed before the marker run is +opened**, on a stream of the flow's own root, exactly as a predicate gate's +verdict is. That is what makes it survive a resume: redaction reads the +process environment, so a credential rotated while the process was down would +otherwise re-normalize the same authored sentence into a different marker +command — and the marker's admission key is stable, so the kernel would refuse +the drifted spec as `run_admission_conflict` and the run would lose the +explanation it had already journaled. A resumed body reuses the committed +verdict instead of recomputing one. A `done()` with no detail commits nothing: +its marker command is a function of the reason alone, so there is nothing that +can drift and nothing to recover. + +The kernel facts on the first line do not move: an authored `step_failed` run +completes with a root step that succeeded, and that stays true and stays +printed. In the diagnostic *message* — the string a Cloud run's `error` is +expected to carry — the detail is folded onto one line, with `\n`, `\r`, `\t` +and `\uXXXX` standing in for control characters, because Cloud's error view +elides the middle of a long multi-line error. (That projection is the +server's; see docs/CLOUD.md for what this repository does and does not +establish about it.) The unescaped text is in `completionDetail` and in the +diagnostic's `detail` beside it. + `canceled` and `budget_exceeded` are in the type but are refused with `unsupported_completion`. They are kernel outcomes, not authored verdicts: the kernel records them when it cancels a run or exhausts its budget, and a body @@ -1107,7 +1180,7 @@ The exit codes are part of the surface contract: | Exit | Outcome | |---:|---| | `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. | +| `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. | | `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). | diff --git a/kernel/relayflowd/src/server/tests.rs b/kernel/relayflowd/src/server/tests.rs index c960de3c9..c1f6e237b 100644 --- a/kernel/relayflowd/src/server/tests.rs +++ b/kernel/relayflowd/src/server/tests.rs @@ -119,6 +119,132 @@ fn run_start_admission_key_recovers_the_same_run_and_refuses_spec_drift() { assert_eq!(drifted.error.unwrap().code, "run_admission_conflict"); } +/// flows#545: an authored `done("step_failed", { detail })` travels as JSON +/// inside the terminal marker's deterministic command. +/// +/// The SDK relies on three kernel properties for that to be durable evidence +/// rather than a claim: the command is journaled verbatim and survives a +/// reopen, re-admitting the identical spec under the same admission key +/// returns the same run without spawning a second one, and a spec whose +/// marker data changed is refused instead of quietly admitted. No kernel +/// change was needed for the detail — it is opaque data in a shell string — +/// so this test exists to keep it that way. +#[test] +fn deterministic_marker_carrying_json_survives_reopen_and_refuses_changed_detail() { + let directory = tempdir().unwrap(); + let data_dir = directory.path(); + let hub = Arc::new(ProtocolHub::default()); + let (writer, _peer) = shared_writer(); + let marker = |detail: &str| { + format!( + r#"printf '%s' '{{"completionReason":"step_failed","detail":"{detail}"}}'"# + ) + }; + let start = |id: &str, command: &str| { + json!({"id": id, "verb": "run.start", "params": { + "admission_key": "authored-child:complete-2", + "spec": {"name": "software-factory/complete-2", "steps": [{ + "id": "complete-2", "type": "deterministic", "command": command, + }]} + }}) + .to_string() + }; + + let detail = "review found 1 P2: review.clean was not created"; + let first = request(data_dir, &hub, 1, &writer, &start("one", &marker(detail))); + assert!(first.ok, "first admission failed: {:?}", first.error); + let run_id = first.result.unwrap()["run_id"].as_str().unwrap().to_owned(); + + // Same spec, same key: the same run, not a second effect. + let retried = request(data_dir, &hub, 1, &writer, &start("two", &marker(detail))); + assert!(retried.ok, "idempotent retry failed: {:?}", retried.error); + assert_eq!(retried.result.unwrap()["run_id"], run_id); + + // Reopened from disk by this request, the journal holds exactly one + // spawn, and the marker command came back character for character. + let read = request( + data_dir, + &hub, + 1, + &writer, + &json!({"id":"read","verb":"journal.read","params":{"run_id": run_id}}).to_string(), + ); + assert!(read.ok, "journal.read failed: {:?}", read.error); + let entries = read.result.unwrap(); + let spawned: Vec<_> = entries["entries"] + .as_array() + .unwrap() + .iter() + .filter(|entry| entry["entry_type"] == "run.spawned") + .collect(); + assert_eq!(spawned.len(), 1, "one admitted run, one spawn"); + assert_eq!( + spawned[0]["payload"]["spec"]["steps"][0]["command"] + .as_str() + .unwrap(), + marker(detail), + ); + + // One character of the detail differs: a different spec under an identity + // that is already spoken for, and the kernel fails closed rather than + // letting a second verdict take the first one's place. + let drifted = request( + data_dir, + &hub, + 1, + &writer, + &start("three", &marker("review found 2 P2s")), + ); + assert!(!drifted.ok); + assert_eq!(drifted.error.unwrap().code, "run_admission_conflict"); +} + +/// flows#545: a lone UTF-16 surrogate is refused at the line, with no id. +/// +/// This is the kernel property the SDK's detail normalization exists for. A +/// JavaScript string is code units, not text, so trimming an agent's output — +/// `(prose + "\u{1F642}").slice(0, -1)` — produces a `string` whose last half +/// of a surrogate pair has no partner. `JSON.stringify` escapes it, so the +/// frame is syntactically sendable; this decoder is where it stops. The +/// refusal carries `"id": null`, because the id is inside the frame that did +/// not parse — so it resolves no pending client request, and a caller that is +/// waiting on one waits forever. Normalizing before the write +/// (`packages/sdk/src/authored-completion.ts`) is the only place that can +/// keep an authored explanation out of this. +#[test] +fn a_lone_surrogate_in_a_request_is_refused_with_no_request_id() { + let directory = tempdir().unwrap(); + let hub = Arc::new(ProtocolHub::default()); + let (writer, _peer) = shared_writer(); + + let refused = request( + directory.path(), + &hub, + 1, + &writer, + r#"{"id":"probe","verb":"hello","params":{"protocol":0,"client":"review found 1 P2: \ud83d"}}"#, + ); + + assert!(!refused.ok); + assert_eq!( + refused.id, + Value::Null, + "a refusal no pending request can be matched to" + ); + assert_eq!(refused.error.unwrap().code, "bad_request"); + + // The complete pair is ordinary text and is accepted, so what the decoder + // refuses above is the lone half, not the emoji. + let paired = request( + directory.path(), + &hub, + 1, + &writer, + r#"{"id":"probe","verb":"hello","params":{"protocol":0,"client":"review found 1 P2: \ud83d\ude42"}}"#, + ); + assert!(paired.ok, "a well-formed pair was refused: {:?}", paired.error); +} + #[test] fn run_start_refuses_invalid_admission_keys() { let directory = tempdir().unwrap(); diff --git a/packages/sdk/src/authored-completion-record.ts b/packages/sdk/src/authored-completion-record.ts new file mode 100644 index 000000000..3c159634a --- /dev/null +++ b/packages/sdk/src/authored-completion-record.ts @@ -0,0 +1,119 @@ +// The authored verdict, written to the root BEFORE the marker is admitted. +// +// The terminal marker's command embeds the detail, and the marker run is +// opened under a stable admission key (`authored-child:`). +// Those two facts together mean the detail has to be a durable decision, not a +// recomputed one: normalization redacts against `process.env`, so a credential +// rotated or removed while this process was down makes the SAME authored +// sentence normalize differently. A body resumed into that environment would +// retry the marker's admission key with a drifted spec, and the kernel refuses +// that as `run_admission_conflict` — losing an explanation the root had +// already journaled, for a flow whose every step succeeded. +// +// So the verdict is appended to a stream on the root run before the marker run +// is opened, and a later attempt reaching the same terminal step reuses what +// is recorded. That is exactly the durability the executor already gives a +// predicate gate (`applyPredicateGate`): record the decision first, so the +// spec built from it is identical on every attempt and author code is never +// asked the question twice. +// +// Nothing is recorded for a `done()` with no detail. Without one the marker +// command is a function of the reason alone — no environment, no drift, and +// nothing to recover — so a flow that does not opt in writes exactly the +// journal it has always written. + +import { + isDurableCompletionDetail, + isLoweredCompletion, + type LoweredCompletionReason, +} from './authored-completion.js'; +import type { JournalClient } from './journal-client.js'; + +export const AUTHORED_VERDICT_STREAM = 'authored-verdict'; + +/** Versioned so a reader can tell this record from a future one. */ +const RECORD_KIND = 'relayflows.authored-verdict.v1'; + +export interface AuthoredVerdictRecord { + readonly reason: LoweredCompletionReason; + /** Recorded only when the body passed one; see the module comment. */ + readonly detail?: string; +} + +/** + * Commit this body's verdict, or recover the one this root already committed. + * + * Returns the verdict the marker must be built from — the recorded one when + * the root holds it, this attempt's otherwise. With no root run there is + * nothing durable to write to (the non-durable executor seam), so that path + * passes straight through without a round trip. A root run is always read + * first: a committed record stays authoritative even for an attempt that has + * no detail of its own — an optional source can be present on the first + * execution and gone on the retry, and "this attempt has no detail" is not + * "this root never recorded one". Only when no record exists AND this attempt + * has no detail does it return without appending, preserving the no-detail + * marker, spec hash, and journal exactly as before. + */ +export async function commitAuthoredVerdict( + journal: JournalClient, + rootRunId: string | undefined, + step: string, + verdict: AuthoredVerdictRecord, +): Promise { + if (rootRunId === undefined) return verdict; + const committed = await readAuthoredVerdict(journal, rootRunId, step); + if (committed !== undefined) return committed; + if (verdict.detail === undefined) return verdict; + await journal.streamAppend(rootRunId, AUTHORED_VERDICT_STREAM, { + verdict: RECORD_KIND, step, reason: verdict.reason, detail: verdict.detail, + }); + return verdict; +} + +/** + * The verdict this root recorded for `step`, or `undefined`. + * + * The FIRST valid record wins: it is the one the marker was — or was about to + * be — admitted from, and a later attempt's differently-redacted text must not + * displace it. Records this version does not recognise, and details that fail + * the durable check every other read boundary applies, are skipped rather than + * trusted; the marker then drifts and the kernel refuses it loudly, which is + * the right outcome for a record this process cannot read. + */ +export async function readAuthoredVerdict( + journal: JournalClient, + rootRunId: string, + step: string, +): Promise { + let offset = 0; + for (;;) { + const page = await journal.streamRead(rootRunId, AUTHORED_VERDICT_STREAM, offset, 1000); + for (const message of page.messages) { + // `stream.read` returns either the envelope or the bare message, + // matching how the executor reads predicate verdicts. + const raw = (message as { message?: unknown }).message ?? message; + if (isVerdictRecord(raw) && raw.step === step) { + return Object.freeze({ reason: raw.reason, detail: raw.detail }); + } + } + if (page.messages.length === 0 || page.next_offset <= offset) break; + offset = page.next_offset; + } + return undefined; +} + +interface StoredVerdict { + readonly verdict: typeof RECORD_KIND; + readonly step: string; + readonly reason: LoweredCompletionReason; + readonly detail: string; +} + +function isVerdictRecord(value: unknown): value is StoredVerdict { + if (typeof value !== 'object' || value === null || Array.isArray(value)) return false; + const record = value as Partial; + return record.verdict === RECORD_KIND + && typeof record.step === 'string' && record.step.length > 0 + && isLoweredCompletion(record.reason) + && isDurableCompletionDetail(record.detail); +} diff --git a/packages/sdk/src/authored-completion.ts b/packages/sdk/src/authored-completion.ts new file mode 100644 index 000000000..f46eb5ab6 --- /dev/null +++ b/packages/sdk/src/authored-completion.ts @@ -0,0 +1,203 @@ +// The authored verdict: which completions this runtime lowers, the durable +// marker that carries one, and the optional detail that says why. +// +// Split out of `authored-flow-executor.ts` so the CLI report and the status +// projection can read this vocabulary without importing the executor — and +// with it every provider adapter, MCP client and helper the executor pulls +// in. Nothing here does I/O or reads a clock. + +import { AuthoredFlowExecutionError } from './authored-flow-error.js'; +import { redact } from './redact.js'; + +/** + * The completion reasons an authored body may declare and this executor lowers. + * + * `FlowCompletionReason` is wider than this on purpose — it is the journal's + * run vocabulary plus authored verdicts — but the two sets drifting silently is + * exactly what made a type-valid `done("step_failed")` die at runtime as + * `unsupported_completion`. Every gate that asks "is this a completion this + * runtime can lower?" now asks this one function, so a reason cannot be + * accepted in one place and rejected in another. + * + * These are internal cross-module helpers for the authored seam (the executor, + * the durable root, the IPC verifier, the CLI report and `flows status`), NOT + * public SDK surface. `src/index.ts` deliberately re-exports nothing from this + * module — keep it that way, or the whole authored seam leaks with them. + */ +export const LOWERED_COMPLETIONS = ['success', 'needs_human', 'step_failed', 'declined'] as const; +export type LoweredCompletionReason = (typeof LOWERED_COMPLETIONS)[number]; + +export function isLoweredCompletion(value: unknown): value is LoweredCompletionReason { + return typeof value === 'string' && (LOWERED_COMPLETIONS as readonly string[]).includes(value); +} + +/** + * The bound on a normalized completion detail: 2,000 Unicode code points in + * the FINAL string, {@link COMPLETION_DETAIL_TRUNCATED_SUFFIX} included. + * + * Code points, not `String.length`: `.length` counts UTF-16 code units, so an + * emoji-heavy detail measured that way is half the length a reader would call + * it. The number matches `@relayflows/surface`'s + * `COMPLETION_DETAIL_MAX_CODE_POINTS`, which is what an author sees; it is the + * same order as the kernel's gate-detail cap + * (`kernel/relayflowd/src/engine/remote.rs`) but deliberately NOT the same + * bound — that one takes 2,000 characters and then appends its suffix, so its + * final string is longer than the number it advertises. + */ +export const COMPLETION_DETAIL_MAX_CODE_POINTS = 2000; + +/** Fixed width, so the bound above can reserve exactly its room. */ +export const COMPLETION_DETAIL_TRUNCATED_SUFFIX = '… (truncated)'; + +/** + * Read `done()`'s optional second argument into the detail the marker carries. + * + * Absence has four spellings — no argument, `undefined`, `{}`, and + * `{ detail: undefined }` — and all four mean the same thing: no detail, and a + * marker command byte-identical to the one the one-argument call has always + * produced. Whitespace-only joins them: say nothing, or say something. + * + * A `detail` that is present and not a string is a programming error the type + * system already catches for TypeScript callers; the runtime check is here for + * the same reason `isSurfaceFlowCompletionReason` is, and it refuses with the + * existing closed `unsupported_completion` code rather than widening the + * taxonomy. Refusals name the type they got, never the value — a malformed + * detail is exactly the kind of text that may carry a credential. + * + * Redaction runs BEFORE truncation, and the order is load-bearing rather than + * cosmetic: truncating first can split a token so no pattern matches it any + * more, which is a truncation that *causes* a leak. What redaction recognises + * is the SDK's existing policy — known token shapes, named credential fields, + * and the values of secret-looking environment variables (`redact.ts`) — not a + * guarantee to recognise every possible secret. + * + * Lone surrogates are substituted here too, so what leaves this function is a + * well-formed string the journal protocol can carry — see {@link wellFormed}. + */ +export function normalizeCompletionDetail( + options: unknown, + env: NodeJS.ProcessEnv = process.env, +): string | undefined { + if (options === undefined) return undefined; + if (typeof options !== 'object' || options === null || Array.isArray(options)) { + throw new AuthoredFlowExecutionError( + 'unsupported_completion', + `done() options must be an object with an optional string "detail"; received ${typeName(options)}`, + ); + } + const detail = (options as { detail?: unknown }).detail; + if (detail === undefined) return undefined; + if (typeof detail !== 'string') { + throw new AuthoredFlowExecutionError( + 'unsupported_completion', + `done() detail must be a string; received ${typeName(detail)}`, + ); + } + const redacted = wellFormed(redact(detail, env)).trim(); + return redacted.length === 0 ? undefined : bound(redacted); +} + +/** + * Lone UTF-16 surrogates, as code points. + * + * Under `u` a subject string is iterated by CODE POINT, so a well-formed pair + * is one code point above U+FFFF and never enters this class. What the class + * matches is exactly a surrogate with no partner. + */ +const LONE_SURROGATE = /[\uD800-\uDFFF]/u; +const LONE_SURROGATES = /[\uD800-\uDFFF]/gu; + +/** + * Replace every lone surrogate with U+FFFD, the replacement character. + * + * A JavaScript string is UTF-16 code units, not text. Slicing a character off + * the end of an agent's output — `(prose + '\u{1F642}').slice(0, -1)` — leaves + * a high surrogate with no partner, and the type system calls the result a + * `string`. Nothing downstream does. + * `JSON.stringify` escapes it, so the marker COMMAND is admitted; the raw + * detail then travels in the root's `step.complete` output, where the kernel's + * JSON decoder refuses it ("unexpected end of hex escape") and answers with a + * `bad_request` carrying a null request id — which resolves no pending + * request, so the call hangs and the authored explanation never lands. + * + * Substituting is right here where refusing is not: the flow has already done + * its work and reached a verdict, and losing the whole explanation over one + * broken code unit destroys more than it protects. One code unit in, one code + * unit out, so the bound below still counts what a reader would count. + */ +function wellFormed(text: string): string { + return LONE_SURROGATE.test(text) ? text.replace(LONE_SURROGATES, '\uFFFD') : text; +} + +/** + * Is this a detail a durable record may carry? + * + * The gate every read boundary uses — the completed root's output, and the + * IPC frame the Bun runner returns — so an over-long or wrong-typed value + * fails closed there instead of flowing into a report as if it had been + * journaled by this process. + */ +export function isDurableCompletionDetail(value: unknown): value is string { + return typeof value === 'string' + && value.length > 0 + && !LONE_SURROGATE.test(value) + && [...value].length <= COMPLETION_DETAIL_MAX_CODE_POINTS; +} + +/** + * The detail as ONE line, for a diagnostic message. + * + * Cloud renders a run's `error` through a line-oriented view that elides the + * middle of a long one (`cli/cloud-read.ts` `errorLines`), so a forty-line + * detail can lose the very finding it exists to carry. Escaping the breaks + * keeps the whole detail on the single line that renderer cannot elide, and + * keeps control characters out of a terminal. The unescaped normalized detail + * still travels in the structured fields beside it. + */ +export function singleLineCompletionDetail(detail: string): string { + return detail.replace(/[\u0000-\u001F\u007F-\u009F]/gu, (char) => + NAMED_ESCAPE[char] ?? `\\u${char.codePointAt(0)!.toString(16).padStart(4, '0')}`); +} + +const NAMED_ESCAPE: Record = { '\n': '\\n', '\r': '\\r', '\t': '\\t' }; + +/** + * The deterministic command that carries an authored verdict into the journal. + * + * `success` with no detail lowers to `:` because success needs no marker: the + * marker run's own kernel `success` already IS that record. Every other + * lowered verdict is something the kernel's completion vocabulary cannot + * express on a step that *succeeded*, so it travels as data on stdout and is + * read back from `step.completed`. It is deliberately not lowered as a failing + * command: no step failed here, and a fabricated failure would put bogus + * evidence in the journal for a flow whose steps all ran correctly. + * + * A detail rides in the same JSON object, so the marker stays the one durable + * record of the verdict and the IPC verifier's existing command comparison + * checks the detail against the journal for free. With no detail the command + * is byte-identical to the one this function has always returned, which is + * what keeps `spec_hash` stable for every flow that does not opt in. + */ +export function completionMarker(reason: LoweredCompletionReason, detail?: string): string { + if (detail === undefined) { + return reason === 'success' ? ':' : `printf '%s' '{"completionReason":"${reason}"}'`; + } + // `JSON.stringify` escapes quotes, backslashes and every control character, + // so the command is one line and the only shell-significant character left + // in it is `'` — which the single-quote escape below closes. + const json = JSON.stringify({ completionReason: reason, detail }); + return `printf '%s' '${json.replaceAll("'", "'\\''")}'`; +} + +function typeName(value: unknown): string { + if (value === null) return 'null'; + if (Array.isArray(value)) return 'an array'; + return `a ${typeof value}`; +} + +function bound(text: string): string { + const points = [...text]; + if (points.length <= COMPLETION_DETAIL_MAX_CODE_POINTS) return text; + const keep = COMPLETION_DETAIL_MAX_CODE_POINTS - [...COMPLETION_DETAIL_TRUNCATED_SUFFIX].length; + return `${points.slice(0, keep).join('')}${COMPLETION_DETAIL_TRUNCATED_SUFFIX}`; +} diff --git a/packages/sdk/src/authored-flow-executor.ts b/packages/sdk/src/authored-flow-executor.ts index ebf9c28a1..ecf7b0135 100644 --- a/packages/sdk/src/authored-flow-executor.ts +++ b/packages/sdk/src/authored-flow-executor.ts @@ -13,6 +13,13 @@ import { AuthoredBudget } from './authored-budget.js'; import { assertMemoryReachable, authoredMemory, scriptMemoryScope } from './authored-memory.js'; import { authoredDeterministicRunner, authoredWorkerRunner } from './authored-worker-step.js'; import { isSurfaceFlowCompletionReason, isSurfaceRunCompletionReason } from './authored-step-output.js'; +import { + completionMarker, + isLoweredCompletion, + normalizeCompletionDetail, + type LoweredCompletionReason, +} from './authored-completion.js'; +import { commitAuthoredVerdict } from './authored-completion-record.js'; import { type AgentResult, type LlmOptions, @@ -96,6 +103,12 @@ export interface AuthoredFlowExecutionResult { readonly rootRunId?: string; readonly name: string; readonly completionReason: LoweredCompletionReason; + /** + * Why, when the body said. Normalized at `done()` — redacted, trimmed and + * bounded — and absent for every one-argument call, so the durable output, + * the IPC frame and the report keep the shapes they had. + */ + readonly completionDetail?: string; readonly journalSteps: readonly AuthoredFlowJournalStep[]; } @@ -243,6 +256,7 @@ export async function executeAuthoredFlow( const stepEdges = (step: string): AuthoredStepEdges | undefined => lifecycle.stepEdges(step); let nextStep = 1; let requestedCompletion: LoweredCompletionReason | undefined; + let requestedDetail: string | undefined; const lowerDeterministic = authoredDeterministicRunner( definition.name, journal, journalSteps, budget, { @@ -553,7 +567,7 @@ export async function executeAuthoredFlow( displayLabel(typeof name === 'string' ? name : undefined), )); }, - done(reason) { + done(reason, doneOptions) { if (!isSurfaceFlowCompletionReason(reason)) { throw new AuthoredFlowExecutionError( 'unsupported_completion', @@ -586,8 +600,13 @@ export async function executeAuthoredFlow( reason, ); } + // Validated before the completion is marked, so a malformed `detail` + // leaves the flow exactly as any other refused `done()` does: not + // completed, and still able to report the real refusal. + const detail = normalizeCompletionDetail(doneOptions); lifecycle.markCompletion(); requestedCompletion = reason; + requestedDetail = detail; }, cloud: unsupportedCloud( () => assertOperationAllowed('cloud', definition.name, requestedCompletion), @@ -661,12 +680,27 @@ export async function executeAuthoredFlow( // not as a fabricated kernel run.completed reason. The marker step reports // what the body decided; it is not itself a step that failed. The CLI turns // the verdict into the exit code (success/declined 0, needs_human 3, step_failed 1). - await lowerDeterministic(`complete-${nextStep}`, - completionMarker(requestedCompletion), true); + // + // The verdict is committed to the root BEFORE the marker run is opened, and + // a resumed body reuses what was committed rather than recomputing it + // (authored-completion-record.ts). A detail is redacted against + // `process.env`, so a credential rotated while this process was down would + // otherwise re-normalize the same authored sentence differently, retry the + // marker's stable admission key with a drifted spec, and lose the + // explanation the root had already journaled to `run_admission_conflict`. + // With no detail there is no environment in the command and nothing to + // commit, so that path is untouched. + const terminalId = `complete-${nextStep}`; + const verdict = await commitAuthoredVerdict(journal, options.rootRunId, terminalId, { + reason: requestedCompletion, + ...(requestedDetail === undefined ? {} : { detail: requestedDetail }), + }); + await lowerDeterministic(terminalId, completionMarker(verdict.reason, verdict.detail), true); return Object.freeze({ ...(options.rootRunId === undefined ? {} : { rootRunId: options.rootRunId }), name: definition.name, - completionReason: requestedCompletion, + completionReason: verdict.reason, + ...(verdict.detail === undefined ? {} : { completionDetail: verdict.detail }), journalSteps: Object.freeze([...journalSteps]), }); } @@ -686,43 +720,6 @@ function unsupportedVerb(verb: string): AuthoredFlowExecutionError { ); } -/** - * The completion reasons an authored body may declare and this executor lowers. - * - * `FlowCompletionReason` is wider than this on purpose — it is the journal's - * run vocabulary plus authored verdicts — but the two sets drifting silently is - * exactly what made a type-valid `done("step_failed")` die at runtime as - * `unsupported_completion`. Every gate that asks "is this a completion this - * runtime can lower?" now asks this one function, so a reason cannot be - * accepted in one place and rejected in another. - * - * These three are internal cross-module helpers for the authored seam (the - * executor, the durable root, the IPC verifier and the CLI report), NOT public - * SDK surface. `src/index.ts` deliberately re-exports nothing from this module - * — keep it that way, or the whole authored seam leaks with them. - */ -export const LOWERED_COMPLETIONS = ['success', 'needs_human', 'step_failed', 'declined'] as const; -export type LoweredCompletionReason = (typeof LOWERED_COMPLETIONS)[number]; - -export function isLoweredCompletion(value: unknown): value is LoweredCompletionReason { - return typeof value === 'string' && (LOWERED_COMPLETIONS as readonly string[]).includes(value); -} - -/** - * The deterministic command that carries an authored verdict into the journal. - * - * `success` lowers to `:` because success needs no marker: the marker run's own - * kernel `success` already IS that record. Every other lowered verdict is - * something the kernel's completion vocabulary cannot express on a step that - * *succeeded*, so it travels as data on stdout and is read back from - * `step.completed`. It is deliberately not lowered as a failing command: no - * step failed here, and a fabricated failure would put bogus evidence in the - * journal for a flow whose steps all ran correctly. - */ -export function completionMarker(reason: LoweredCompletionReason): string { - return reason === 'success' ? ':' : `printf '%s' '{"completionReason":"${reason}"}'`; -} - function assertOperationAllowed( verb: string, flowName: string, diff --git a/packages/sdk/src/authored-node-runner.ts b/packages/sdk/src/authored-node-runner.ts index a06f77661..0636305ac 100644 --- a/packages/sdk/src/authored-node-runner.ts +++ b/packages/sdk/src/authored-node-runner.ts @@ -8,7 +8,7 @@ import { isAbsolute, join } from 'node:path'; import type { Readable } from 'node:stream'; import type { AuthoredRootMetadata } from './authored-root.js'; import type { AuthoredExecutionRuntime, AuthoredFlowExecutionResult, ExecuteAuthoredFlowOptions } from './authored-flow-executor.js'; -import { completionMarker, isLoweredCompletion } from './authored-flow-executor.js'; +import { completionMarker, isDurableCompletionDetail, isLoweredCompletion } from './authored-completion.js'; import { AuthoredFlowExecutionError, AuthoredHumanParked, type AuthoredFlowExecutionErrorCode, type AuthoredHumanWait, @@ -212,6 +212,9 @@ export async function verifyAuthoredNodeResult( const invalid = (why = ''): never => { throw new Error(`authored runtime result has no matching durable completion${process.env['FLOWS_VERIFIER_DEBUG'] && why ? ` (${why})` : ''}`); }; if (result.rootRunId !== rootRunId || result.name !== metadata.flowName || !isLoweredCompletion(result.completionReason) + // Type- and bound-check the claimed detail here, so the marker comparison + // below compares two values this process would itself have produced. + || (result.completionDetail !== undefined && !isDurableCompletionDetail(result.completionDetail)) || !Array.isArray(result.journalSteps) || result.journalSteps.length === 0) invalid('frame'); const terminal = result.journalSteps.at(-1)!; if (!terminal || !/^complete-[1-9][0-9]*$/.test(terminal.id)) invalid('terminal'); @@ -288,8 +291,11 @@ export async function verifyAuthoredNodeResult( // rejected a frame whose reason is not lowerable at all — that one is // the runtime validation of untrusted IPC, and it is why nothing has // to be re-asserted here just to satisfy the type. + // The detail is inside the marker, so this one comparison also + // attests it: a frame cannot claim a detail the journal does not + // hold, drop one it does, or alter a character of it. if (step?.type !== 'deterministic' - || step.command !== completionMarker(result.completionReason)) invalid('marker'); + || step.command !== completionMarker(result.completionReason, result.completionDetail)) invalid('marker'); } } } finally { journal.close(); } diff --git a/packages/sdk/src/authored-root.ts b/packages/sdk/src/authored-root.ts index 55f0dda95..72b916d1c 100644 --- a/packages/sdk/src/authored-root.ts +++ b/packages/sdk/src/authored-root.ts @@ -4,7 +4,9 @@ import { createHash, randomUUID } from 'node:crypto'; import { readFile } from 'node:fs/promises'; import { canonicalize } from './canonical.js'; import { compileSpec, toKernelSpec } from './compile.js'; -import { executeAuthoredFlow, isLoweredCompletion, type AuthoredFlowExecutionResult } from './authored-flow-executor.js'; +import { executeAuthoredFlow, type AuthoredFlowExecutionResult } from './authored-flow-executor.js'; +import { isDurableCompletionDetail, isLoweredCompletion } from './authored-completion.js'; +import { AUTHORED_ROOT_KIND } from './authored-verdict.js'; import { loadAuthoredFlow, type LoadedAuthoredFlow, @@ -19,7 +21,12 @@ import { AuthoredFlowExecutionError, AuthoredHumanParked } from './authored-flow import { readOpenHumanWaits } from './authored-human.js'; import { isSurfaceCompletionReason } from './authored-step-output.js'; -const ROOT_KIND = 'relayflows.authored-root.v1'; +/** + * Taken from the projection module rather than spelled twice: `flows status` + * has to recognise the same root this file writes, and it must not import the + * executor to do it. + */ +const ROOT_KIND = AUTHORED_ROOT_KIND; export interface AuthoredRootExtension { readonly name: string; @@ -230,6 +237,7 @@ async function driveRoot( dispatch.run_id, dispatch.step_id, dispatch.attempt, dispatch.idempotency_key, 'success', { output: { name: result.name, completionReason: result.completionReason, + ...(result.completionDetail === undefined ? {} : { completionDetail: result.completionDetail }), journalSteps: result.journalSteps, ...(result.executionRuntime === undefined ? {} : { executionRuntime: result.executionRuntime }) }, started_pins: dispatch.pins, end_pins: dispatch.pins, @@ -372,9 +380,13 @@ async function completedRootResult( if (!isCompletedRootOutput(output)) { throw new Error('completed authored root has no durable result'); } + // The stored detail, never a freshly computed one: redaction reads the + // CURRENT environment, so recomputing here would let a completed run report + // something its journal does not hold. return Object.freeze({ name: output.name, completionReason: output.completionReason, + ...(output.completionDetail === undefined ? {} : { completionDetail: output.completionDetail }), journalSteps: Object.freeze(output.journalSteps.map(step => Object.freeze({ ...step }))), rootRunId, }); @@ -385,6 +397,10 @@ function isCompletedRootOutput(value: unknown): value is Omit; return typeof output.name === 'string' && isLoweredCompletion(output.completionReason) + // A malformed detail fails the whole readback rather than being dropped: + // silently discarding it would report the verdict without the evidence + // the journal says it was recorded with. + && (output.completionDetail === undefined || isDurableCompletionDetail(output.completionDetail)) && Array.isArray(output.journalSteps) && output.journalSteps.every(step => typeof step === 'object' && step !== null && typeof step.id === 'string' && typeof step.runId === 'string' diff --git a/packages/sdk/src/authored-verdict.ts b/packages/sdk/src/authored-verdict.ts new file mode 100644 index 000000000..78d58d563 --- /dev/null +++ b/packages/sdk/src/authored-verdict.ts @@ -0,0 +1,87 @@ +// Project an authored flow's own verdict out of its root journal. +// +// `foldRunState` answers what the KERNEL recorded: the root step succeeded, so +// the run completed with `success`. That is true and must stay printed. It is +// also not what a reader asking "why did this fail?" needs when the body +// declared `step_failed` — a verdict the kernel's vocabulary cannot express on +// a step that succeeded, so it travels as output on the root's own +// `step.completed`. +// +// This is a separate, narrow projection rather than a branch inside the fold: +// the fold is generic over every run and has no business knowing what an +// authored root is, and the executor this vocabulary comes from must not +// become a dependency of `flows status`. + +import { + isDurableCompletionDetail, + isLoweredCompletion, + type LoweredCompletionReason, +} from './authored-completion.js'; +import type { JournalEvent } from './journal-reader.js'; + +/** The `instruction` discriminator `authored-root.ts` writes into the root step. */ +export const AUTHORED_ROOT_KIND = 'relayflows.authored-root.v1'; + +export interface AuthoredVerdict { + readonly reason: LoweredCompletionReason; + /** Absent when the body called `done()` with one argument, as most do. */ + readonly detail?: string; +} + +/** + * The verdict an authored root attested, or `null` when this journal holds none. + * + * `null` is the answer for an ordinary run, for an authored run still in + * flight, and for one whose root output is malformed. The last of those + * matters: an ordinary flow may name a step `authored-root`, and a worker may + * journal anything into a step's output, so the step id alone attests nothing. + * A verdict is reported only when the run's own spec declares the authored + * root — carrying {@link AUTHORED_ROOT_KIND} metadata — and the root's last + * `step.completed` is a terminal kernel `success` whose output is a complete, + * in-bounds authored result. A retry or park completion is not terminal and a + * later attempt may still change the answer, so neither is promoted. + * + * This is a projection for a read-only view, so a malformed record yields no + * verdict rather than an error: refusing to render a whole status because one + * output field is the wrong type would replace the answer with a worse one. + */ +export function authoredVerdictOf(events: readonly JournalEvent[]): AuthoredVerdict | null { + const spawned = events[0]; + if (spawned === undefined || spawned.entry_type !== 'run.spawned') return null; + if (!declaresAuthoredRoot(spawned)) return null; + + const completed = [...events].reverse().find((event) => + event.entry_type === 'step.completed' && event.step_id === 'authored-root'); + const payload = record(completed?.payload); + if (payload === null || payload['completionReason'] !== 'success') return null; + const disposition = payload['disposition']; + if (disposition !== undefined && disposition !== 'step_done') return null; + + const output = record(payload['output']); + if (output === null || typeof output['name'] !== 'string') return null; + const reason = output['completionReason']; + if (!isLoweredCompletion(reason)) return null; + const detail = output['completionDetail']; + if (detail !== undefined && !isDurableCompletionDetail(detail)) return null; + + return Object.freeze({ reason, ...(detail === undefined ? {} : { detail }) }); +} + +function declaresAuthoredRoot(spawned: JournalEvent): boolean { + const spec = record(record(spawned.payload)?.['spec']); + const steps = spec?.['steps']; + if (!Array.isArray(steps)) return false; + const step = record(steps[0]); + if (step === null || step['id'] !== 'authored-root') return false; + if (typeof step['instruction'] !== 'string') return false; + try { + return record(JSON.parse(step['instruction']))?.['kind'] === AUTHORED_ROOT_KIND; + } catch { + return false; + } +} + +function record(value: unknown): Record | null { + return value !== null && typeof value === 'object' && !Array.isArray(value) + ? value as Record : null; +} diff --git a/packages/sdk/src/cli/run.ts b/packages/sdk/src/cli/run.ts index 475bd3702..1f0aba7f3 100644 --- a/packages/sdk/src/cli/run.ts +++ b/packages/sdk/src/cli/run.ts @@ -26,8 +26,11 @@ import type { RunStatus, } from '../protocol.js'; import type { StepType } from '../spec.js'; +import { + singleLineCompletionDetail, + type LoweredCompletionReason, +} from '../authored-completion.js'; import { DEFAULT_LOCAL_AGENT_CAPACITY } from '../worker-slots.js'; -import type { LoweredCompletionReason } from '../authored-flow-executor.js'; import { checkFlow, type CheckReport, @@ -57,6 +60,13 @@ export interface RunReport { socketPath?: string; status?: RunStatus; completionReason?: RunCompletionReason; + /** + * What the body passed to `done(reason, { detail })`, normalized: redacted, + * trimmed, and bounded to 2,000 code points. Absent for every one-argument + * call, so an existing report keeps its exact shape — and absent on the + * paths where no authored body declared the outcome at all. + */ + completionDetail?: string; completedSteps?: number; reuse?: { fromRunId: string; reusedSteps: number; executedSteps: number }; parkedStep?: ParkedStep; @@ -405,15 +415,30 @@ export function authoredCompletion( command: RunCommand, base: RunReport, socketPath: string, - result: { name: string; completionReason: LoweredCompletionReason; journalSteps: readonly unknown[] }, + result: { + name: string; + completionReason: LoweredCompletionReason; + completionDetail?: string; + journalSteps: readonly unknown[]; + }, runId: string | undefined, ): RunExecution { const common: RunReport = { ...fromBase(command, base), ...(runId === undefined ? {} : { runId }), socketPath, + ...(result.completionDetail === undefined ? {} : { completionDetail: result.completionDetail }), completedSteps: result.journalSteps.length, }; + // Two spellings of the same fact, on purpose. `detail` is the structured + // one — the `StepFailedDetails` key the diagnostic already declares, here + // describing the AUTHOR's verdict rather than the daemon's account of a + // failing step — and keeps the body's own line breaks. `said` is the same + // text folded onto one line for the message, because Cloud renders a run's + // `error` through a view that elides the middle of a long one. + const detail = result.completionDetail; + const evidence = detail === undefined ? {} : { detail }; + const said = detail === undefined ? '' : singleLineCompletionDetail(detail); switch (result.completionReason) { case 'success': return { @@ -427,7 +452,9 @@ export function authoredCompletion( ...common, ok: true, status: 'completed', completionReason: 'success', diagnostics: [...base.diagnostics, { severity: 'declined', kind: 'run_declined', - message: 'Flow deliberately chose not to act on this input.', + ...evidence, + message: 'Flow deliberately chose not to act on this input.' + + (detail === undefined ? '' : ` ${said}`), }], }, }; @@ -438,7 +465,9 @@ export function authoredCompletion( ...common, ok: false, status: 'parked', diagnostics: [...base.diagnostics, { severity: 'parked', kind: 'run_parked', - message: `Flow "${result.name}" needs_human; see the journal for accumulated blockers.`, + ...evidence, + message: `Flow "${result.name}" needs_human; see the journal for accumulated blockers.` + + (detail === undefined ? '' : ` ${said}`), }], }, }; @@ -454,9 +483,16 @@ export function authoredCompletion( ...common, ok: false, status: 'failed', completionReason: 'step_failed', diagnostics: [...base.diagnostics, { severity: 'failure', kind: 'step_failed', - message: `Flow "${result.name}" declared done("step_failed"): its own checks did not pass. ` - + 'No step failed, so there is no step-level evidence to inspect; the journal holds ' - + 'every step the flow ran before it decided.', + ...evidence, + // With a detail, the flow's own account REPLACES the generic + // sentence: "no step-level evidence to inspect" is the only thing + // there is to say when the body said nothing, and saying it + // beside a real explanation would bury the explanation. + message: detail === undefined + ? `Flow "${result.name}" declared done("step_failed"): its own checks did not pass. ` + + 'No step failed, so there is no step-level evidence to inspect; the journal holds ' + + 'every step the flow ran before it decided.' + : `Flow "${result.name}" declared done("step_failed"): ${said}`, }], }, }; diff --git a/packages/sdk/src/cli/status.ts b/packages/sdk/src/cli/status.ts index bd61adb75..0955fa20c 100644 --- a/packages/sdk/src/cli/status.ts +++ b/packages/sdk/src/cli/status.ts @@ -13,6 +13,8 @@ import { AUTHORED_STEP_STREAM, foldAuthoredStepRecords, type AuthoredStepRecord import { canonicalize } from '../canonical.js'; import { DEFAULT_DATA_DIR } from '../daemon-connection.js'; import { JournalReadError, walkJournal, type JournalEvent } from '../journal-client.js'; +import { singleLineCompletionDetail } from '../authored-completion.js'; +import { authoredVerdictOf, type AuthoredVerdict } from '../authored-verdict.js'; import { redact } from '../redact.js'; import { foldRunState, RunStateError, type RunView, type StepView } from '../run-state.js'; import { DATA_DIR_ENV, RUN_ID_ENV, STEP_ID_ENV } from '../step-env.js'; @@ -37,6 +39,11 @@ export interface StatusArgs { export const DEFAULT_TAIL_LINES = 20; /** A gate's `detail` may be 2,000 chars (engine/remote.rs); the view shows the head. */ const DETAIL_LIMIT = 1024; +// An authored completion detail is NOT held to `DETAIL_LIMIT`. That bound is +// the head of a gate render the journal holds in full; this text is already +// bounded to 2,000 code points at `done()`, it is the whole of what the flow +// said about its own verdict, and cutting it at 1,024 would drop the finding +// as often as not. const BUSY_RETRIES = 5; const BUSY_RETRY_DELAY_MS = 50; @@ -178,7 +185,7 @@ export async function runStatus(args: StatusArgs, io: CliIo, options: StatusOpti } } - const presented = present(view, thisStep, tails, env); + const presented = present(view, thisStep, tails, env, authoredVerdictOf(taken.events)); if (args.json) { const authored = authoredSteps(taken.events, env); io.stdout(canonicalize({ @@ -237,13 +244,41 @@ function authoredSteps(events: readonly JournalEvent[], env: NodeJS.ProcessEnv): } type PresentedStep = StepView & { tails: StepTails }; -type Presented = Omit & { this_step: string | null; steps: PresentedStep[] }; +type Presented = Omit & { + this_step: string | null; + steps: PresentedStep[]; + /** + * The verdict the BODY declared, beside — never instead of — the kernel + * facts above. `status`, `completion_reason` and every step stay exactly + * what the journal recorded, because they are true: an authored + * `step_failed` run completes with a root step that succeeded. + * + * Present only when the body actually said why. A one-argument `done()` + * adds nothing a reader could not already see from `completion_reason`, and + * emitting a null here for every legacy and non-authored run would change a + * shape that nobody asked to change. + */ + authored_completion?: { reason: string; detail: string }; +}; /** Apply the redaction and size bounds the on-disk model does not have. */ -function present(view: RunView, thisStep: string | null, tails: Map, env: NodeJS.ProcessEnv): Presented { +function present( + view: RunView, + thisStep: string | null, + tails: Map, + env: NodeJS.ProcessEnv, + authored: AuthoredVerdict | null, +): Presented { return { ...view, this_step: thisStep, + ...(authored?.detail === undefined ? {} : { + // Redacted again on the way out. It was redacted before it was + // journaled, by this same redactor against a different environment; + // doing it here too costs nothing and keeps this module the one place + // that decides what reaches an agent-facing page. + authored_completion: { reason: authored.reason, detail: redact(authored.detail, env) }, + }), steps: view.steps.map((step) => ({ ...step, // An agent names the files it writes, and it inherits this process's @@ -307,6 +342,12 @@ function renderText(view: Presented, partial: string[], wantTails: boolean): str + (view.spend.dollars_unmetered ? ' (unmetered)' : ''); const finished = view.completion_reason === null ? '' : ` finished ${view.completion_reason}`; lines.push(`RUN ${view.run_id} ${safe(view.name)} ${view.status} started ${formatDuration(view.now_ms - view.spawned_at_ms)} ago${finished} spend ${spend}`); + // Labelled, so nobody reads a failure explanation as a claim that the run's + // kernel status is anything other than the line above says it is. + if (view.authored_completion !== undefined) { + lines.push(`authored done("${safe(view.authored_completion.reason)}"): ` + + safe(singleLineCompletionDetail(view.authored_completion.detail))); + } const counts = (['done', 'running', 'pending', 'backoff', 'waiting', 'needs_human'] as const) .filter((key) => view.counts[key] > 0).map((key) => `${view.counts[key]} ${key}`); // The denominator is always printed, even for zero steps, so a reader can diff --git a/packages/sdk/tests/authored-completion-detail.test.ts b/packages/sdk/tests/authored-completion-detail.test.ts new file mode 100644 index 000000000..f159f5046 --- /dev/null +++ b/packages/sdk/tests/authored-completion-detail.test.ts @@ -0,0 +1,463 @@ +// `done(reason, { detail })`: the flow says why, and the reason survives. +// +// Cloud run f92bf832-7848-58d8-b5ca-da8e3b849f1c finished `step_failed` after +// twenty successful steps and an opened PR. The reviewer's finding — "one P2 +// remains: cleanup can report success while an ambiguous allocation stays +// invisible through all three sweeps" — existed, and neither the run report +// nor `flows status --cloud` held a word of it. These tests pin the path that +// carries it: normalization at `done()`, the durable marker, the root's +// output, the IPC verifier, and the report. + +import { createHash } from 'node:crypto'; +import { execFileSync } from 'node:child_process'; +import { rmSync } from 'node:fs'; +import type { Server } from 'node:net'; +import { flow, type Ctx } from '@relayflows/surface'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { + COMPLETION_DETAIL_MAX_CODE_POINTS, + COMPLETION_DETAIL_TRUNCATED_SUFFIX, + completionMarker, + isDurableCompletionDetail, + normalizeCompletionDetail, + singleLineCompletionDetail, +} from '../src/authored-completion.js'; +import { canonicalize } from '../src/canonical.js'; +import { executeAuthoredFlow } from '../src/authored-flow-executor.js'; +import { authoredCompletion, type RunReport } from '../src/cli/run.js'; +import { JournalClient } from '../src/journal-client.js'; +import { + kernelDialectError, + sendOk, + sendResult, + sockPath, + startLoopback, +} from './journal-client-loopback.js'; + +/** The reviewer's sentence from cloud#3919, which the run record threw away. */ +const FINDING = 'One P2 remains: cleanup can report success while an ambiguous ' + + 'allocation stays invisible through all three sweeps — `review.clean` was not created.'; + +const LOWERED = ['success', 'needs_human', 'step_failed', 'declined'] as const; + +/** + * The complete lowered spec and its canonical hash for every no-detail + * completion, captured from the tree BEFORE this change (commit 16237b6) with + * the same loopback this file uses. + * + * Marker-string equality alone would not catch a lowering that changed around + * the marker, so the whole spec is hashed. A flow that passes no detail must + * produce these bytes exactly, or a released flow's `spec_hash` moved and its + * memoized children stopped matching. + */ +const PRE_CHANGE_SPEC: Record = { + success: { + sha256: '2bca7a2d3291b698623671e3527c0fbfa9fca0ad0a032996749d663b27f7549a', + json: '{"name":"stability-success/complete-1","steps":[{"command":":","depends_on":[],"id":"complete-1","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"deterministic","verification":{}}],"version":"0.1.0"}', + }, + needs_human: { + sha256: 'd8504f8cc11ff05d8a389f9c177a06d905e9083c8fa006783c866ef2ec892b61', + json: '{"name":"stability-needs_human/complete-1","steps":[{"command":"printf \'%s\' \'{\\"completionReason\\":\\"needs_human\\"}\'","depends_on":[],"id":"complete-1","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"deterministic","verification":{}}],"version":"0.1.0"}', + }, + step_failed: { + sha256: 'e749f41b3b319b10ab77e2ee57a5be4b8eecd8bbd5fe20d5c73eb980d111ed70', + json: '{"name":"stability-step_failed/complete-1","steps":[{"command":"printf \'%s\' \'{\\"completionReason\\":\\"step_failed\\"}\'","depends_on":[],"id":"complete-1","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"deterministic","verification":{}}],"version":"0.1.0"}', + }, + declined: { + sha256: 'f62919dc650b5ffcaa79f0b763b732a2fe5ebd9e36e924f26753d0077111242f', + json: '{"name":"stability-declined/complete-1","steps":[{"command":"printf \'%s\' \'{\\"completionReason\\":\\"declined\\"}\'","depends_on":[],"id":"complete-1","max_iterations":1,"retry":{"initial_backoff_ms":100,"jitter_percent":20,"max_backoff_ms":60000,"multiplier":2},"type":"deterministic","verification":{}}],"version":"0.1.0"}', + }, +}; + +describe('normalizing done()\'s optional detail', () => { + it.each([ + ['no options at all', undefined], + ['empty options', {}], + ['an explicitly undefined detail', { detail: undefined }], + ['an empty detail', { detail: '' }], + ['a whitespace-only detail', { detail: ' \n\t ' }], + ])('reads %s as no detail', (_label, options) => { + expect(normalizeCompletionDetail(options, {})).toBeUndefined(); + }); + + it.each([ + ['null', null, 'options must be an object'], + ['an array', ['why'], 'options must be an object'], + ['a bare string', 'why', 'options must be an object'], + ['a number', 7, 'options must be an object'], + ['a non-string detail', { detail: 7 }, 'detail must be a string'], + ['a null detail', { detail: null }, 'detail must be a string'], + ['an array detail', { detail: ['why'] }, 'detail must be a string'], + ])('refuses %s with unsupported_completion', (_label, options, fragment) => { + expect(() => normalizeCompletionDetail(options, {})) + .toThrowError(expect.objectContaining({ code: 'unsupported_completion' })); + expect(() => normalizeCompletionDetail(options, {})).toThrowError(fragment); + }); + + it('names the type it got, never the value — a malformed detail may be a credential', () => { + let message = ''; + try { normalizeCompletionDetail({ detail: { token: 'rk_live_NEVERPRINTTHIS' } }, {}); } + catch (error) { message = (error as Error).message; } + expect(message).toContain('detail must be a string; received a object'); + expect(message).not.toContain('NEVERPRINTTHIS'); + expect(message).not.toContain('rk_live'); + }); + + it('keeps prose, trimming only the surrounding whitespace', () => { + expect(normalizeCompletionDetail({ detail: `\n${FINDING}\n` }, {})).toBe(FINDING); + }); + + it('redacts a known token shape and a secret environment value', () => { + const env = { RELAY_API_KEY: 'super-secret-workspace-material' }; + const detail = `link ot_live_abc123DEF456 and key ${env.RELAY_API_KEY}`; + const normalized = normalizeCompletionDetail({ detail }, env)!; + expect(normalized).not.toContain('ot_live_abc123DEF456'); + expect(normalized).not.toContain('super-secret-workspace-material'); + expect(normalized).toBe('link [redacted] and key [redacted:RELAY_API_KEY]'); + }); + + it.each([ + [COMPLETION_DETAIL_MAX_CODE_POINTS - 1, false], + [COMPLETION_DETAIL_MAX_CODE_POINTS, false], + [COMPLETION_DETAIL_MAX_CODE_POINTS + 1, true], + ])('bounds a %i-code-point detail (truncated: %s)', (length, truncated) => { + const normalized = normalizeCompletionDetail({ detail: 'a'.repeat(length) }, {})!; + expect([...normalized]).toHaveLength(Math.min(length, COMPLETION_DETAIL_MAX_CODE_POINTS)); + expect(normalized.endsWith(COMPLETION_DETAIL_TRUNCATED_SUFFIX)).toBe(truncated); + }); + + it('counts code points, not UTF-16 units, so an emoji detail is not halved', () => { + // 3,000 astral code points is 6,000 UTF-16 units. A `.length` bound would + // cut this at 1,000 emoji; the documented bound cuts at the suffix. + const normalized = normalizeCompletionDetail({ detail: '🙂'.repeat(3000) }, {})!; + const points = [...normalized]; + expect(points).toHaveLength(COMPLETION_DETAIL_MAX_CODE_POINTS); + expect(normalized.endsWith(COMPLETION_DETAIL_TRUNCATED_SUFFIX)).toBe(true); + expect(points.filter((point) => point === '🙂')) + .toHaveLength(COMPLETION_DETAIL_MAX_CODE_POINTS - [...COMPLETION_DETAIL_TRUNCATED_SUFFIX].length); + }); + + it('redacts BEFORE it truncates, so the cut cannot create a leak', () => { + // A secret environment value is redacted by exact match (`replaceAll`). + // Cut it in half first and there is nothing left for that match to find, + // so the truncation is what journals a fragment of live material. + const secret = 'S'.repeat(40); + const env = { RELAY_API_KEY: secret }; + const prefix = `${'x'.repeat(1959)} `; + const raw = `${prefix}${secret} tail`; + const keep = COMPLETION_DETAIL_MAX_CODE_POINTS - [...COMPLETION_DETAIL_TRUNCATED_SUFFIX].length; + expect([...raw].length).toBeGreaterThan(COMPLETION_DETAIL_MAX_CODE_POINTS); + expect(raw.indexOf(secret)).toBeLessThan(keep); + expect(raw.indexOf(secret) + secret.length).toBeGreaterThan(keep); + + const normalized = normalizeCompletionDetail({ detail: raw }, env)!; + expect(normalized).not.toContain(secret); + expect(normalized).not.toContain('S'.repeat(8)); + // Redacting first also shrank it back under the bound, so nothing was cut. + expect(normalized).toBe(`${prefix}[redacted:RELAY_API_KEY] tail`); + expect(normalized.endsWith(COMPLETION_DETAIL_TRUNCATED_SUFFIX)).toBe(false); + }); + + it.each([ + ['a high surrogate left by slicing an emoji off the end', `${FINDING} \u{1F642}`.slice(0, -1), `${FINDING} \uFFFD`], + ['a low surrogate left by slicing one off the front', '\u{1F642} tail'.slice(1), '\uFFFD tail'], + ['a lone surrogate in the middle of prose', 'P2 \uD800 remains', 'P2 \uFFFD remains'], + ])('substitutes %s', (_label, raw, expected) => { + // `('review found 1 P2: ' + '\u{1F642}').slice(0, -1)` is ordinary JS string + // trimming of reviewer output, and it produces a `string` that is not + // text. `JSON.stringify` escapes it, so the marker COMMAND is admitted — + // and then the raw detail in the root's `step.complete` output is refused + // by the kernel's JSON decoder with a null-id `bad_request` that resolves + // no pending request, so the call never returns and the explanation is lost. + expect(normalizeCompletionDetail({ detail: raw }, {})).toBe(expected); + }); + + it('leaves well-formed text alone, surrogate pairs included', () => { + for (const detail of [FINDING, '\u{1F642} kept whole', '検査は \u{1F642} で終わった']) { + expect(normalizeCompletionDetail({ detail }, {})).toBe(detail); + } + }); + + it('gates a durable value on the same bound it wrote', () => { + expect(isDurableCompletionDetail(FINDING)).toBe(true); + expect(isDurableCompletionDetail('a'.repeat(COMPLETION_DETAIL_MAX_CODE_POINTS))).toBe(true); + expect(isDurableCompletionDetail('a'.repeat(COMPLETION_DETAIL_MAX_CODE_POINTS + 1))).toBe(false); + expect(isDurableCompletionDetail('🙂'.repeat(COMPLETION_DETAIL_MAX_CODE_POINTS))).toBe(true); + expect(isDurableCompletionDetail('🙂'.repeat(COMPLETION_DETAIL_MAX_CODE_POINTS + 1))).toBe(false); + expect(isDurableCompletionDetail('')).toBe(false); + expect(isDurableCompletionDetail(7)).toBe(false); + expect(isDurableCompletionDetail(undefined)).toBe(false); + }); + + it('refuses a durable value the journal protocol cannot carry', () => { + // The same invariant normalization enforces, enforced again at every read + // boundary: a lone surrogate is a string the kernel's JSON decoder + // rejects, so it is not a value a durable record may claim to hold. + expect(isDurableCompletionDetail(`${FINDING} \u{1F642}`.slice(0, -1))).toBe(false); + expect(isDurableCompletionDetail('\uDC00 orphan low')).toBe(false); + expect(isDurableCompletionDetail(`${FINDING} \u{1F642}`)).toBe(true); + expect(isDurableCompletionDetail(normalizeCompletionDetail( + { detail: `${FINDING} \u{1F642}`.slice(0, -1) }, {}))).toBe(true); + }); + + it('folds a multiline detail onto one line without losing a character of it', () => { + const multiline = 'P1 none\nP2 one: review.clean missing\r\nP3 none\ttabbed\u0007bell'; + expect(singleLineCompletionDetail(multiline)) + .toBe('P1 none\\nP2 one: review.clean missing\\r\\nP3 none\\ttabbed\\u0007bell'); + expect(singleLineCompletionDetail(FINDING)).toBe(FINDING); + }); +}); + +describe('the terminal marker that carries the detail', () => { + it.each(LOWERED)('leaves done("%s") with no detail byte-identical', (reason) => { + expect(completionMarker(reason)).toBe(reason === 'success' + ? ':' : `printf '%s' '{"completionReason":"${reason}"}'`); + expect(completionMarker(reason, undefined)).toBe(completionMarker(reason)); + }); + + it.each([ + ['plain prose', FINDING], + ['single quotes', `it's the reviewer's 'clean' file`], + ['double quotes and backslashes', 'said "no" at C:\\work\\review.md'], + ['newlines and tabs', 'P1: none\nP2: one\r\n\tindented'], + ['command substitution', 'ran $(id) and `whoami` and ${HOME}'], + ['a shell terminator', "'; rm -rf /tmp/nothing; echo '"], + ['unicode', '検査は 🙂 で終わった — naïve'], + ])('executes as a shell command whose stdout is the verdict (%s)', (_label, detail) => { + const command = completionMarker('step_failed', detail); + const stdout = execFileSync('/bin/sh', ['-c', command], { encoding: 'utf8' }); + expect(JSON.parse(stdout)).toEqual({ completionReason: 'step_failed', detail }); + // One line, so the journaled command stays greppable. + expect(command).not.toContain('\n'); + }); + + it('carries a normalized sliced-emoji detail through the shell unchanged', () => { + const detail = normalizeCompletionDetail({ detail: `${FINDING} \u{1F642}`.slice(0, -1) }, {})!; + const stdout = execFileSync('/bin/sh', ['-c', completionMarker('step_failed', detail)], { encoding: 'utf8' }); + expect(JSON.parse(stdout)).toEqual({ completionReason: 'step_failed', detail }); + expect(detail).toBe(`${FINDING} \uFFFD`); + }); +}); + +describe('an executed flow that declares why', () => { + let path: string; + let server: Server; + let nextRun = 1; + const startedSpecs: Record[] = []; + const stepByRun = new Map(); + + beforeAll(() => { + path = sockPath(); + server = startLoopback(path, { + hello: (ctx) => sendOk(ctx), + 'run.start': (ctx, params) => { + const error = kernelDialectError(params.spec); + if (error !== null) { + ctx.send({ id: ctx.id, ok: false, error: { code: 'invalid_spec', message: error } }); + return; + } + const spec = params.spec as Record; + const step = (spec['steps'] as Record[])[0]!; + const runId = `detail-run-${nextRun++}`; + startedSpecs.push(spec); + stepByRun.set(runId, { id: step['id'] as string, command: step['command'] as string }); + sendResult(ctx, { + run_id: runId, status: 'completed', completion_reason: 'success', completed_steps: 1, + }); + }, + 'journal.read': (ctx, params) => { + const step = stepByRun.get(params.run_id as string)!; + sendResult(ctx, { + entries: [{ + entry_type: 'step.completed', + step_id: step.id, + payload: { completionReason: 'success', output: { exit_code: 0, stdout_tail: '', stderr_tail: '' } }, + }], + }); + }, + }); + }); + + afterAll(async () => { + await new Promise((resolve) => server.close(() => resolve())); + rmSync(path, { force: true }); + }); + + async function execute(name: string, body: (f: Ctx) => Promise) { + startedSpecs.length = 0; + const client = new JournalClient(path, { requestTimeoutMs: 2000 }); + await client.connect(); + await client.hello(name); + try { + return await executeAuthoredFlow(flow(name, body), client); + } finally { + client.close(); + } + } + + it('journals the detail inside the marker and returns it on the result', async () => { + const result = await execute('software-factory', async (f) => { + await f.run('printf ok'); + f.done('step_failed', { detail: FINDING }); + }); + + expect(result.completionReason).toBe('step_failed'); + expect(result.completionDetail).toBe(FINDING); + // Every step, marker included, still SUCCEEDED: the verdict is data the + // body declared, not a step that failed. + expect(result.journalSteps.map((step) => step.completionReason)).toEqual(['success', 'success']); + const marker = (startedSpecs.at(-1) as { steps: Array<{ command: string }> }).steps[0]!.command; + expect(JSON.parse(execFileSync('/bin/sh', ['-c', marker], { encoding: 'utf8' }))) + .toEqual({ completionReason: 'step_failed', detail: FINDING }); + }); + + it.each(LOWERED)('lowers a detail on done("%s") too', async (reason) => { + const result = await execute(`detailed-${reason}`, async (f) => { + f.done(reason, { detail: `why ${reason}` }); + }); + expect(result.completionReason).toBe(reason); + expect(result.completionDetail).toBe(`why ${reason}`); + const marker = (startedSpecs[0] as { steps: Array<{ command: string }> }).steps[0]!.command; + expect(JSON.parse(execFileSync('/bin/sh', ['-c', marker], { encoding: 'utf8' }))) + .toEqual({ completionReason: reason, detail: `why ${reason}` }); + }); + + it.each(LOWERED)('leaves the whole lowered spec of a no-detail done("%s") unchanged', async (reason) => { + const fixture = PRE_CHANGE_SPEC[reason]; + for (const options of [undefined, {}, { detail: undefined }, { detail: ' ' }] as const) { + startedSpecs.length = 0; + const client = new JournalClient(path, { requestTimeoutMs: 2000 }); + await client.connect(); + await client.hello(`stability-${reason}`); + try { + await executeAuthoredFlow(flow(`stability-${reason}`, async (f) => { + if (options === undefined) f.done(reason); else f.done(reason, options); + }), client); + } finally { + client.close(); + } + const json = canonicalize(startedSpecs[0]); + expect(json).toBe(fixture.json); + expect(createHash('sha256').update(json).digest('hex')).toBe(fixture.sha256); + } + }); + + it('carries the normalized detail, not the author\'s raw string', async () => { + const result = await execute('redacted-at-done', async (f) => { + f.done('step_failed', { detail: ` leaked ot_live_abc123DEF456 in review ` }); + }); + expect(result.completionDetail).toBe('leaked [redacted] in review'); + expect(canonicalize(startedSpecs[0])).not.toContain('ot_live_'); + }); + + it.each([ + ['non-object options', 'why'], + ['a non-string detail', { detail: 7 }], + ])('refuses %s and does not complete the flow', async (_label, options) => { + await expect(execute(`refused-${_label.replaceAll(' ', '-')}`, async (f) => { + (f.done as (reason: 'step_failed', options: unknown) => void)('step_failed', options); + })).rejects.toMatchObject({ code: 'unsupported_completion' }); + // Refused before `markCompletion`, so no marker was journaled. + expect(startedSpecs).toHaveLength(0); + }); + + it('keeps the kernel-owned refusals ahead of any options check', async () => { + await expect(execute('kernel-owned-with-detail', async (f) => { + (f.done as (reason: 'canceled', options: unknown) => void)('canceled', { detail: 7 }); + })).rejects.toMatchObject({ code: 'unsupported_completion', completionReason: 'canceled' }); + }); + + it('keeps duplicate completion and post-completion operations unchanged', async () => { + await expect(execute('duplicate-with-detail', async (f) => { + f.done('step_failed', { detail: 'first' }); + f.done('success', { detail: 'second' }); + })).rejects.toMatchObject({ code: 'duplicate_completion' }); + + await expect(execute('after-completion-with-detail', async (f) => { + f.done('step_failed', { detail: 'done already' }); + await f.run('printf late'); + })).rejects.toMatchObject({ code: 'operation_after_completion' }); + }); +}); + +describe('the report a detail-bearing completion produces', () => { + const base: RunReport = { ok: false, command: 'run', resolutions: [], diagnostics: [] }; + const result = ( + completionReason: 'success' | 'needs_human' | 'step_failed' | 'declined', + completionDetail?: string, + ) => ({ + name: 'software-factory', + completionReason, + ...(completionDetail === undefined ? {} : { completionDetail }), + journalSteps: [{}, {}], + }); + + it('replaces the generic step_failed sentence with what the flow said', () => { + const execution = authoredCompletion('run', base, '/sock', result('step_failed', FINDING), 'root'); + + expect(execution.exitCode).toBe(1); + expect(execution.report.status).toBe('failed'); + expect(execution.report.completionReason).toBe('step_failed'); + expect(execution.report.completionDetail).toBe(FINDING); + const diagnostic = execution.report.diagnostics.at(-1)!; + expect(diagnostic.kind).toBe('step_failed'); + expect(diagnostic.message).toBe(`Flow "software-factory" declared done("step_failed"): ${FINDING}`); + expect(diagnostic.message).not.toContain('no step-level evidence to inspect'); + expect((diagnostic as { detail?: string }).detail).toBe(FINDING); + }); + + it('keeps the no-detail step_failed message byte-identical', () => { + const execution = authoredCompletion('resume', base, '/sock', result('step_failed'), 'root'); + expect(execution.report.diagnostics.at(-1)!.message).toBe( + 'Flow "software-factory" declared done("step_failed"): its own checks did not pass. ' + + 'No step failed, so there is no step-level evidence to inspect; the journal holds ' + + 'every step the flow ran before it decided.', + ); + expect(execution.report.completionDetail).toBeUndefined(); + expect('completionDetail' in execution.report).toBe(false); + expect((execution.report.diagnostics.at(-1) as { detail?: string }).detail).toBeUndefined(); + }); + + it('names no step, because no step failed', () => { + const execution = authoredCompletion('run', base, '/sock', result('step_failed', FINDING), 'root'); + const diagnostic = execution.report.diagnostics.at(-1) as { stepId?: string; exitCode?: number }; + expect(diagnostic.stepId).toBeUndefined(); + expect(diagnostic.exitCode).toBeUndefined(); + }); + + it('folds a multiline detail onto the message line and keeps the original beside it', () => { + const multiline = 'P1: none\nP2: review.clean was not created\nP3: none'; + const execution = authoredCompletion('run', base, '/sock', result('step_failed', multiline), 'root'); + const diagnostic = execution.report.diagnostics.at(-1)!; + expect(diagnostic.message).toContain('P1: none\\nP2: review.clean was not created\\nP3: none'); + expect(diagnostic.message).not.toContain('\n'); + expect((diagnostic as { detail?: string }).detail).toBe(multiline); + expect(execution.report.completionDetail).toBe(multiline); + }); + + it('adds the detail to needs_human and declined without moving their exit codes', () => { + const parked = authoredCompletion('run', base, '/sock', result('needs_human', 'blocked on the allocation'), 'root'); + expect(parked.exitCode).toBe(3); + expect(parked.report.status).toBe('parked'); + expect(parked.report.diagnostics.at(-1)!.kind).toBe('run_parked'); + expect(parked.report.diagnostics.at(-1)!.message).toBe( + 'Flow "software-factory" needs_human; see the journal for accumulated blockers. blocked on the allocation'); + + const declined = authoredCompletion('run', base, '/sock', result('declined', 'no ticket in the input'), 'root'); + expect(declined.exitCode).toBe(0); + expect(declined.report.ok).toBe(true); + expect(declined.report.status).toBe('completed'); + expect(declined.report.completionReason).toBe('success'); + expect(declined.report.diagnostics.at(-1)!.kind).toBe('run_declined'); + expect(declined.report.diagnostics.at(-1)!.message).toBe( + 'Flow deliberately chose not to act on this input. no ticket in the input'); + }); + + it('reports a successful flow\'s detail without inventing a diagnostic', () => { + const execution = authoredCompletion('run', base, '/sock', result('success', 'all three sweeps clean'), 'root'); + expect(execution.exitCode).toBe(0); + expect(execution.report.ok).toBe(true); + expect(execution.report.completionDetail).toBe('all three sweeps clean'); + expect(execution.report.diagnostics).toEqual([]); + }); +}); diff --git a/packages/sdk/tests/authored-completion-recovery.test.ts b/packages/sdk/tests/authored-completion-recovery.test.ts new file mode 100644 index 000000000..e3f66b29d --- /dev/null +++ b/packages/sdk/tests/authored-completion-recovery.test.ts @@ -0,0 +1,238 @@ +// A committed verdict survives the environment it was redacted in. +// +// The terminal marker's command embeds the detail, and the marker run is +// opened under a stable admission key. Normalization redacts against +// `process.env`, so a credential rotated or removed while the process was down +// makes the SAME authored sentence normalize differently — and a body resumed +// into that environment would retry the marker's admission key with a drifted +// spec. The kernel refuses that as `run_admission_conflict`, and a flow whose +// every step succeeded loses the explanation its root had already journaled. +// +// These run against the real daemon and inject the loss at the executor seam: +// the marker's admission is committed in the kernel and the response never +// reaches this process. That is not a claim of a process-kill end-to-end test; +// it is the exact window the completed-root resume test cannot cover, because +// that one starts with the root output already durable. + +import { afterEach, expect, it } from 'vitest'; +import { flow } from '@relayflows/surface'; +import { executeAuthoredFlow } from '../src/authored-flow-executor.js'; +import { readAuthoredVerdict } from '../src/authored-completion-record.js'; +import type { JournalClient } from '../src/journal-client.js'; +import { chainFixture } from './flow-chain-fixture.js'; + +const cleanup: Array<() => Promise> = []; +afterEach(async () => { for (const close of cleanup.splice(0)) await close(); }); + +/** The synthetic credential; the name is what `redact.ts` treats as secret. */ +const SECRET_NAME = 'RELAYFLOWS_RECOVERY_TEST_SECRET'; +const FIRST = 'opaque-recovery-value-123'; +const ROTATED = 'rotated-recovery-value-456'; +const RAW = `review found credential ${FIRST} in the agent's output`; +const REDACTED = `review found credential [redacted:${SECRET_NAME}] in the agent's output`; + +/** + * A root that stays mutable: an agent step pinned to a stream no worker holds + * parks, so the body's own children and stream appends are admitted against a + * live run — the same shape `authored-run-failure-evidence.test.ts` opens. + */ +async function openRoot(journal: JournalClient): Promise { + const outcome = await journal.runStart({ + version: '0.1.0', + name: 'recovery-root', + steps: [{ + id: 'authored-root', type: 'agent', instruction: '{}', + surfaces: { streams: [{ stream: 'recovery-root-stream' }] }, + recovery_mode: 'reset', max_iterations: 8, + }], + } as never); + return outcome.run_id; +} + +/** + * Run the body once, losing the response to its FIRST `run.start` — the + * terminal marker's, for a body with no other steps. The kernel keeps the + * admission; this process is told nothing. Returns the run id it never saw. + */ +async function loseTheMarkerResponse( + journal: JournalClient, run: () => Promise, +): Promise { + const start = journal.runStart.bind(journal); + let admitted: string | undefined; + const client = journal as { runStart: JournalClient['runStart'] }; + client.runStart = async (...args) => { + const outcome = await start(...args); + if (admitted === undefined) { + admitted = outcome.run_id; + throw new Error('injected loss after the marker was admitted'); + } + return outcome; + }; + try { + await expect(run()).rejects.toThrow('injected loss after the marker was admitted'); + } finally { + client.runStart = start; + } + if (admitted === undefined) throw new Error('the marker was never admitted'); + return admitted; +} + +it('reuses the committed detail after the redaction environment rotates', async () => { + const fixture = chainFixture(); + cleanup.push(() => fixture.close()); + const journal = await fixture.connect(); + const rootRunId = await openRoot(journal); + const handle = flow('redaction-recovery', async (f) => { + f.done('step_failed', { detail: RAW }); + }); + const run = () => executeAuthoredFlow(handle, journal, undefined, + { rootRunId, dataDir: fixture.data }); + const prior = process.env[SECRET_NAME]; + + try { + process.env[SECRET_NAME] = FIRST; + const admitted = await loseTheMarkerResponse(journal, run); + + // The credential is rotated while this process is "down". The author's + // sentence is unchanged; only what redaction makes of it has moved, and + // recomputing the marker from it is what the kernel refuses as + // `run_admission_conflict`. + process.env[SECRET_NAME] = ROTATED; + const result = await run(); + + // The verdict reached the root before the marker did, so it was readable + // even though nothing had learned what the marker's run id was. + expect(await readAuthoredVerdict(journal, rootRunId, 'complete-1')) + .toEqual({ reason: 'step_failed', detail: REDACTED }); + expect(result.completionReason).toBe('step_failed'); + expect(result.completionDetail).toBe(REDACTED); + expect(result.completionDetail).not.toContain(FIRST); + // The SAME marker run, not a second one: the recovered detail rebuilt the + // identical spec, so the stable admission key returned its existing run. + expect(result.journalSteps.map((step) => step.id)).toEqual(['complete-1']); + expect(result.journalSteps.at(-1)!.runId).toBe(admitted); + } finally { + if (prior === undefined) delete process.env[SECRET_NAME]; + else process.env[SECRET_NAME] = prior; + } +}, 60_000); + +it('commits the verdict once, and a clean re-execution reads it back', async () => { + const fixture = chainFixture(); + cleanup.push(() => fixture.close()); + const journal = await fixture.connect(); + const rootRunId = await openRoot(journal); + const handle = flow('committed-once', async (f) => { + f.done('declined', { detail: 'the ticket names no repository' }); + }); + const run = () => executeAuthoredFlow(handle, journal, undefined, + { rootRunId, dataDir: fixture.data }); + + const first = await run(); + const second = await run(); + + expect(second.completionDetail).toBe(first.completionDetail); + expect(second.journalSteps.at(-1)!.runId).toBe(first.journalSteps.at(-1)!.runId); + // One record on the stream, from the first execution; the second recovered + // it rather than appending its own. + const page = await journal.streamRead(rootRunId, 'authored-verdict', 0, 1000); + expect(page.messages).toHaveLength(1); +}, 60_000); + +/** The env var that stands in for an optional detail source a retry can lose. */ +const OPTIONAL_DETAIL = 'RELAYFLOWS_RECOVERY_OPTIONAL_DETAIL'; +const OPTIONAL_TEXT = 'review found 1 P2: cleanup remains ambiguous'; + +/** A body whose detail exists only while `OPTIONAL_DETAIL` is set. */ +function optionalDetailFlow() { + return flow('optional-detail', async (f) => { + f.done('step_failed', { detail: process.env[OPTIONAL_DETAIL] }); + }); +} + +it('recovers a committed verdict when a no-detail retry follows a pre-marker loss', async () => { + const fixture = chainFixture(); + cleanup.push(() => fixture.close()); + const journal = await fixture.connect(); + const rootRunId = await openRoot(journal); + const run = () => executeAuthoredFlow(optionalDetailFlow(), journal, undefined, + { rootRunId, dataDir: fixture.data }); + const prior = process.env[OPTIONAL_DETAIL]; + + try { + process.env[OPTIONAL_DETAIL] = OPTIONAL_TEXT; + // Die BEFORE the marker is admitted: runStart never reaches the kernel. + const start = journal.runStart.bind(journal); + const client = journal as { runStart: JournalClient['runStart'] }; + client.runStart = async () => { throw new Error('injected loss before the marker was admitted'); }; + try { + await expect(run()).rejects.toThrow('injected loss before the marker was admitted'); + } finally { + client.runStart = start; + } + + // The verdict still reached the root stream first. + expect(await readAuthoredVerdict(journal, rootRunId, 'complete-1')) + .toEqual({ reason: 'step_failed', detail: OPTIONAL_TEXT }); + + // The retry's optional source is gone, so this attempt has no detail at + // all. The committed record must still be authoritative: "no detail this + // attempt" is not "no detail was ever recorded". + delete process.env[OPTIONAL_DETAIL]; + const result = await run(); + expect(result.completionReason).toBe('step_failed'); + expect(result.completionDetail).toBe(OPTIONAL_TEXT); + // And no second verdict record was appended for the no-detail attempt. + const page = await journal.streamRead(rootRunId, 'authored-verdict', 0, 1000); + expect(page.messages).toHaveLength(1); + } finally { + if (prior === undefined) delete process.env[OPTIONAL_DETAIL]; + else process.env[OPTIONAL_DETAIL] = prior; + } +}, 60_000); + +it('reuses the admitted marker when a no-detail retry follows a post-marker loss', async () => { + const fixture = chainFixture(); + cleanup.push(() => fixture.close()); + const journal = await fixture.connect(); + const rootRunId = await openRoot(journal); + const run = () => executeAuthoredFlow(optionalDetailFlow(), journal, undefined, + { rootRunId, dataDir: fixture.data }); + const prior = process.env[OPTIONAL_DETAIL]; + + try { + process.env[OPTIONAL_DETAIL] = OPTIONAL_TEXT; + const admitted = await loseTheMarkerResponse(journal, run); + + // Retry without the optional source: rebuilding the marker with no detail + // would drift the spec under the same admission key and the kernel would + // refuse it as `run_admission_conflict`. Recovering the committed record + // rebuilds the identical spec instead. + delete process.env[OPTIONAL_DETAIL]; + const result = await run(); + expect(result.completionReason).toBe('step_failed'); + expect(result.completionDetail).toBe(OPTIONAL_TEXT); + expect(result.journalSteps.at(-1)!.runId).toBe(admitted); + } finally { + if (prior === undefined) delete process.env[OPTIONAL_DETAIL]; + else process.env[OPTIONAL_DETAIL] = prior; + } +}, 60_000); + +it('leaves a one-argument done() writing nothing to the verdict stream', async () => { + const fixture = chainFixture(); + cleanup.push(() => fixture.close()); + const journal = await fixture.connect(); + const rootRunId = await openRoot(journal); + const handle = flow('no-detail', async (f) => { f.done('step_failed'); }); + + const result = await executeAuthoredFlow(handle, journal, undefined, + { rootRunId, dataDir: fixture.data }); + + expect(result.completionReason).toBe('step_failed'); + expect('completionDetail' in result).toBe(false); + // Nothing to drift and nothing to recover: the marker command is a function + // of the reason alone, so this flow's journal is the one it always wrote. + const page = await journal.streamRead(rootRunId, 'authored-verdict', 0, 1000); + expect(page.messages).toEqual([]); +}, 60_000); diff --git a/packages/sdk/tests/authored-detail-live.test.ts b/packages/sdk/tests/authored-detail-live.test.ts new file mode 100644 index 000000000..357d4897c --- /dev/null +++ b/packages/sdk/tests/authored-detail-live.test.ts @@ -0,0 +1,143 @@ +// End to end against a real daemon: the flow says why, and every reader sees it. +// +// The mocked suites pin each seam; this one pins the path. A flow declares +// `done("step_failed", { detail })`, the durable runner journals it, and the +// same sentence comes back out of `flows run --json`, out of the journal the +// kernel wrote, and out of `flows status` — text and JSON — for a run whose +// every step succeeded. + +import { writeFileSync } from 'node:fs'; +import { afterEach, expect, it } from 'vitest'; +import { chainFixture } from './flow-chain-fixture.js'; + +const cleanups: Array<() => Promise> = []; +afterEach(async () => { for (const close of cleanups.splice(0)) await close(); }); + +/** The reviewer sentence cloud#3919 threw away, quotes and backtick included. */ +const FINDING = 'One P2 remains: cleanup can report success while an ambiguous ' + + 'allocation stays invisible through all three sweeps — `review.clean` was not created.'; + +it('carries done("step_failed", { detail }) into the report, the journal and flows status', async () => { + const runtime = chainFixture(); + cleanups.push(() => runtime.close()); + const client = await runtime.connect(); + writeFileSync(runtime.flowPath, `import { flow } from '@relayflows/surface'; +export default flow('software-factory', async (f) => { + await f.run('printf reviewed'); + f.done('step_failed', { detail: ${JSON.stringify(FINDING)} }); +}); +`); + const flags = ['--data-dir', runtime.data, '--no-observer-link']; + + const run = runtime.invoke('run', runtime.flowPath, '--input', '{}', ...flags, '--json'); + expect(run.status, run.stderr + run.stdout).toBe(1); + const report = JSON.parse(run.stdout); + expect(report).toMatchObject({ + ok: false, status: 'failed', completionReason: 'step_failed', completedSteps: 2, + completionDetail: FINDING, + }); + const diagnostic = report.diagnostics.at(-1); + expect(diagnostic.kind).toBe('step_failed'); + expect(diagnostic.detail).toBe(FINDING); + expect(diagnostic.message).toBe(`Flow "software-factory" declared done("step_failed"): ${FINDING}`); + expect(diagnostic.message).not.toContain('no step-level evidence to inspect'); + expect(run.stderr).toContain(`FAILED [step_failed] Flow "software-factory" declared done("step_failed"): ${FINDING}`); + + // The journal is the record: the root's output and the marker child's stdout. + const root = await client.journalRead(report.runId, 1); + const completed = root.entries.find((entry: any) => entry.entry_type === 'step.completed') as any; + expect(completed.payload).toMatchObject({ + completionReason: 'success', + output: { completionReason: 'step_failed', completionDetail: FINDING }, + }); + const children = completed.payload.output.journalSteps as Array<{ runId: string; completionReason: string }>; + expect(children.map((child) => child.completionReason)).toEqual(['success', 'success']); + const marker = await client.journalRead(children[1]!.runId, 1); + const markerStdout = (marker.entries.find((entry: any) => + entry.entry_type === 'step.completed') as any).payload.output.stdout_tail as string; + expect(JSON.parse(markerStdout)).toEqual({ completionReason: 'step_failed', detail: FINDING }); + + // `flows status`, from the journal on disk alone. + const status = runtime.invoke('status', report.runId, '--data-dir', runtime.data); + expect(status.status, status.stderr + status.stdout).toBe(0); + const lines = status.stdout.trim().split('\n'); + // The kernel's own account is unchanged: this run completed with success. + expect(lines[0]).toContain('completed'); + expect(lines[0]).toContain('finished success'); + expect(lines[1]).toBe(`authored done("step_failed"): ${FINDING}`); + + const statusJson = runtime.invoke('status', report.runId, '--data-dir', runtime.data, '--json'); + expect(statusJson.status, statusJson.stderr + statusJson.stdout).toBe(0); + const view = JSON.parse(statusJson.stdout); + expect(view.authored_completion).toEqual({ reason: 'step_failed', detail: FINDING }); + expect(view.status).toBe('completed'); + expect(view.completion_reason).toBe('success'); + + // Resuming a completed root returns the stored detail and repeats no effect. + const resumed = runtime.invoke('resume', report.runId, ...flags, '--json'); + expect(resumed.status, resumed.stderr + resumed.stdout).toBe(1); + expect(JSON.parse(resumed.stdout)).toMatchObject({ + command: 'resume', runId: report.runId, completionReason: 'step_failed', completionDetail: FINDING, + }); + expect(await client.journalRead(report.runId, 1)).toEqual(root); +}, 60_000); + +it('reports a detail a caller sliced through an emoji, instead of hanging on it', async () => { + // `(prose + '\u{1F642}').slice(0, -1)` is ordinary JS trimming of reviewer + // output, and it leaves a high surrogate with no partner. Before + // normalization the marker command was admitted (JSON.stringify escapes it) + // and the raw detail then went out in the root's `step.complete` output, + // where the kernel's JSON decoder answered `bad_request` with a null request + // id — which resolves no pending request, so the CLI produced no report at + // all: no stdout, no stderr, and a wait that only a timeout ended. + const runtime = chainFixture(); + cleanups.push(() => runtime.close()); + await runtime.connect(); + writeFileSync(runtime.flowPath, `import { flow } from '@relayflows/surface'; +export default flow('split-emoji', async (f) => { + f.done('step_failed', { detail: ('review found 1 P2: ' + '\u{1F642}').slice(0, -1) }); +}); +`); + + const run = runtime.invoke('run', runtime.flowPath, '--input', '{}', + '--data-dir', runtime.data, '--no-observer-link', '--json'); + + expect(run.status, run.stderr + run.stdout).toBe(1); + const report = JSON.parse(run.stdout); + expect(report.completionDetail).toBe('review found 1 P2: \uFFFD'); + expect(report.diagnostics.at(-1).message) + .toBe('Flow "split-emoji" declared done("step_failed"): review found 1 P2: \uFFFD'); +}, 60_000); + +it('leaves a one-argument done("step_failed") reporting exactly as it always did', async () => { + const runtime = chainFixture(); + cleanups.push(() => runtime.close()); + const client = await runtime.connect(); + writeFileSync(runtime.flowPath, `import { flow } from '@relayflows/surface'; +export default flow('software-factory', async (f) => { + f.done('step_failed'); +}); +`); + const run = runtime.invoke('run', runtime.flowPath, '--input', '{}', + '--data-dir', runtime.data, '--no-observer-link', '--json'); + expect(run.status, run.stderr + run.stdout).toBe(1); + const report = JSON.parse(run.stdout); + expect('completionDetail' in report).toBe(false); + expect(report.diagnostics.at(-1).message).toBe( + 'Flow "software-factory" declared done("step_failed"): its own checks did not pass. ' + + 'No step failed, so there is no step-level evidence to inspect; the journal holds ' + + 'every step the flow ran before it decided.'); + expect('detail' in report.diagnostics.at(-1)).toBe(false); + + const completed = (await client.journalRead(report.runId, 1)).entries + .find((entry: any) => entry.entry_type === 'step.completed') as any; + expect('completionDetail' in completed.payload.output).toBe(false); + const marker = await client.journalRead( + completed.payload.output.journalSteps.at(-1).runId, 1); + expect((marker.entries.find((entry: any) => + entry.entry_type === 'step.completed') as any).payload.output.stdout_tail) + .toBe('{"completionReason":"step_failed"}'); + + const status = runtime.invoke('status', report.runId, '--data-dir', runtime.data, '--json'); + expect('authored_completion' in JSON.parse(status.stdout)).toBe(false); +}, 60_000); diff --git a/packages/sdk/tests/authored-node-result.test.ts b/packages/sdk/tests/authored-node-result.test.ts index 8b9c57528..02d5c5d57 100644 --- a/packages/sdk/tests/authored-node-result.test.ts +++ b/packages/sdk/tests/authored-node-result.test.ts @@ -102,6 +102,34 @@ describe('authored IPC result durable verification',()=>{ if (accepted) await expect(verified).resolves.toBeUndefined(); else await expect(verified).rejects.toThrow('no matching durable completion'); }); + it('attests a claimed detail against the marker the journal actually holds',async()=>{ + const detail='review found 1 P2: review.clean was not created'; + const marker=(value:string)=>`printf '%s' '${JSON.stringify({completionReason:'step_failed',detail:value})}'`; + const claimed={...result(),completionReason:'step_failed' as const,completionDetail:detail}; + records.set('child-2',entries(marker(detail))); + await expect(verifyAuthoredNodeResult(claimed,metadata,'root','socket')).resolves.toBeUndefined(); + + // A frame that alters one character of what the journal recorded. + records.set('child-2',entries(marker(`${detail}!`))); + await expect(verifyAuthoredNodeResult(claimed,metadata,'root','socket')).rejects.toThrow('no matching durable completion'); + + // A frame that claims a detail over a marker that carries none. + records.set('child-2',entries(`printf '%s' '{"completionReason":"step_failed"}'`)); + await expect(verifyAuthoredNodeResult(claimed,metadata,'root','socket')).rejects.toThrow('no matching durable completion'); + + // A frame that drops a detail the marker does hold. + records.set('child-2',entries(marker(detail))); + await expect(verifyAuthoredNodeResult({...result(),completionReason:'step_failed'},metadata,'root','socket')) + .rejects.toThrow('no matching durable completion'); + }); + it.each([ + ['a non-string detail',7], + ['an over-long detail','a'.repeat(2001)], + ['an empty detail',''], + ] as const)('refuses %s on the frame before it reaches the marker comparison',async(_label,detail)=>{ + const claimed={...result(),completionDetail:detail} as unknown as AuthoredFlowExecutionResult; + await expect(verifyAuthoredNodeResult(claimed,metadata,'root','socket')).rejects.toThrow('no matching durable completion'); + }); it('reads successful terminal evidence beyond 100-entry journal pages',async()=>{ const original=records.get('child-1')!; records.set('child-1',[original[0]!,...Array.from({length:248},()=>({entry_type:'worker.stream'})),...original.slice(1)]); diff --git a/packages/sdk/tests/authored-root.test.ts b/packages/sdk/tests/authored-root.test.ts index acc8367da..c09dae9dc 100644 --- a/packages/sdk/tests/authored-root.test.ts +++ b/packages/sdk/tests/authored-root.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { flow } from '@relayflows/surface'; +import { flow, type Ctx } from '@relayflows/surface'; import { getFlowDefinition } from '@relayflows/surface/runtime'; import { loadAuthoredFlow, type LoadedAuthoredFlow } from '../src/authored-flow-loader.js'; import { @@ -31,6 +31,7 @@ const surface = Object.freeze({ class RootPeer extends EventEmitter { readonly completions: Array<{ attempt: number; reason: string }> = []; + readonly outputs: Array | undefined> = []; readonly waits: Array<{ attempt: number; wait: Record }> = []; heartbeats = 0; @@ -60,8 +61,10 @@ class RootPeer extends EventEmitter { attempt: number, _idempotencyKey: string, reason: string, + result?: { output?: unknown }, ): Promise { this.completions.push({ attempt, reason }); + this.outputs.push(result?.output as Record | undefined); if (reason === 'success') return outcome(runId, 'completed', 'success'); if (attempt >= 8) return outcome(runId, 'failed', 'step_failed'); queueMicrotask(() => this.emit('step.dispatch', dispatch(runId, attempt + 1))); @@ -228,6 +231,101 @@ describe('durable authored root', () => { )).rejects.toThrow('completed authored root has no durable result'); }); + it('journals the authored detail on the root step output', async () => { + const loaded = await fixture(false, 0, async (f: Ctx) => { + f.done('step_failed', { detail: 'review found 1 P2: review.clean was not created' }); + }); + const journal = new RootJournal(); + + const result = await executeDurableAuthoredFlow( + loaded, journal as unknown as JournalClient, undefined, + { dataDir: '/unused', admissionKey: 'with-detail' }, + ); + + expect(result.completionDetail).toBe('review found 1 P2: review.clean was not created'); + expect(journal.peer.outputs.at(-1)).toMatchObject({ + name: 'flagship', + completionReason: 'step_failed', + completionDetail: 'review found 1 P2: review.clean was not created', + }); + }); + + it('leaves the root output without the key when the body passed no detail', async () => { + const loaded = await fixture(); + const journal = new RootJournal(); + + await executeDurableAuthoredFlow( + loaded, journal as unknown as JournalClient, undefined, + { dataDir: '/unused', admissionKey: 'no-detail' }, + ); + + const output = journal.peer.outputs.at(-1)!; + expect(output['completionReason']).toBe('success'); + expect('completionDetail' in output).toBe(false); + }); + + it('recovers the stored detail from a completed root without re-running the body', async () => { + let bodyRuns = 0; + const loaded = await fixture(false, 0, async (f: Ctx) => { + bodyRuns += 1; + f.done('step_failed', { detail: 'recomputed from the CURRENT environment' }); + }); + const journal = new RootJournal(); + journal.startStatus = outcome(journal.runId, 'completed', 'success'); + journal.entries = [completedEntry('step_failed', 'as journaled on the first attempt')]; + + const result = await executeDurableAuthoredFlow( + loaded, journal as unknown as JournalClient, undefined, + { dataDir: '/unused', admissionKey: 'completed-with-detail' }, + ); + + // The DURABLE detail, not one recomputed here: redaction reads the current + // environment, so a second computation can differ from what was recorded. + expect(result).toMatchObject({ + completionReason: 'step_failed', + completionDetail: 'as journaled on the first attempt', + }); + expect(bodyRuns).toBe(0); + expect(journal.peer.completions).toEqual([]); + }); + + it('reads back a legacy completed root that carries no detail', async () => { + const loaded = await fixture(); + const journal = new RootJournal(); + journal.startStatus = outcome(journal.runId, 'completed', 'success'); + journal.entries = [completedEntry('step_failed')]; + + const result = await executeDurableAuthoredFlow( + loaded, journal as unknown as JournalClient, undefined, + { dataDir: '/unused', admissionKey: 'legacy-no-detail' }, + ); + + expect(result.completionReason).toBe('step_failed'); + expect(result.completionDetail).toBeUndefined(); + expect('completionDetail' in result).toBe(false); + }); + + it.each([ + ['a non-string detail', 7], + ['an over-long detail', 'a'.repeat(2001)], + ['an empty detail', ''], + ])('fails closed on %s in the completed root output', async (_label, detail) => { + const loaded = await fixture(); + const journal = new RootJournal(); + journal.startStatus = outcome(journal.runId, 'completed', 'success'); + journal.entries = [{ + entry_type: 'step.completed', step_id: 'authored-root', + payload: { completionReason: 'success', output: { + name: 'flagship', completionReason: 'step_failed', journalSteps: [], completionDetail: detail, + } }, + }]; + + await expect(executeDurableAuthoredFlow( + loaded, journal as unknown as JournalClient, undefined, + { dataDir: '/unused', admissionKey: `malformed-detail-${String(detail).length}` }, + )).rejects.toThrow('completed authored root has no durable result'); + }); + it('recovers a same-daemon start retry whose original worker lost its dispatch', async () => { const loaded = await fixture(); const journal = new RootJournal(); @@ -426,11 +524,12 @@ function spawnedEntry(loaded: LoadedAuthoredFlow): Record { }; } -function completedEntry(reason = 'success'): Record { +function completedEntry(reason = 'success', detail?: string): Record { return { entry_type: 'step.completed', step_id: 'authored-root', payload: { completionReason: 'success', output: { name: 'flagship', completionReason: reason, journalSteps: [], + ...(detail === undefined ? {} : { completionDetail: detail }), } }, }; } diff --git a/packages/sdk/tests/authored-status-detail.test.ts b/packages/sdk/tests/authored-status-detail.test.ts new file mode 100644 index 000000000..291f1a195 --- /dev/null +++ b/packages/sdk/tests/authored-status-detail.test.ts @@ -0,0 +1,187 @@ +// `flows status` on an authored run that said why it failed. +// +// The kernel facts stay exactly what the journal recorded — the root step +// succeeded, so the run completed with `success` — and the body's own verdict +// is a separate, labelled line beside them. Nothing here rewrites +// `run.completed` or makes a successful step look failed. + +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it } from 'vitest'; +import { authoredVerdictOf, AUTHORED_ROOT_KIND } from '../src/authored-verdict.js'; +import { canonicalize } from '../src/canonical.js'; +import { parseStatusArgs, runStatus, type StatusOptions } from '../src/cli/status.js'; +import type { JournalEvent } from '../src/journal-client.js'; +import { writeJournalFixture } from './journal-fixture.js'; + +const RUN_ID = '9e1a0f2c-5b0e-4a61-9f8c-1d2e3f4a5b6c'; +const T0 = 1_760_000_000_000; +const FINDING = 'One P2 remains: cleanup can report success while an ambiguous ' + + 'allocation stays invisible through all three sweeps — `review.clean` was not created.'; + +const directories: string[] = []; +afterEach(() => { + for (const directory of directories.splice(0)) rmSync(directory, { recursive: true, force: true }); +}); + +interface RootOptions { + detail?: unknown; + reason?: unknown; + /** Omit the authored-root authority metadata: an ordinary flow may use the id. */ + kind?: string | null; + disposition?: string; + stepCompletionReason?: string; + omitOutput?: boolean; +} + +function journal(options: RootOptions = {}): JournalEvent[] { + const instruction = options.kind === null ? 'just a task for an agent' : JSON.stringify({ + kind: options.kind ?? AUTHORED_ROOT_KIND, + flowName: 'software-factory', + flowPath: '/flows/software-factory.flow.ts', + inputPresent: false, + }); + const output = { + name: 'software-factory', + completionReason: options.reason ?? 'step_failed', + journalSteps: [{ id: 'run-1', runId: 'child-1', completionReason: 'success' }], + ...(options.detail === undefined ? {} : { completionDetail: options.detail }), + }; + const event = ( + seq: number, entry_type: string, step_id: string | null, at_ms: number, payload: unknown, + ): JournalEvent => ({ seq, segment_id: 1, entry_type, run_id: RUN_ID, step_id, attempt: step_id === null ? null : 1, at_ms, payload }); + return [ + event(1, 'run.spawned', null, T0, { spec: { + name: 'software-factory', version: '0.1.0', + steps: [{ id: 'authored-root', type: 'agent', instruction, depends_on: [] }], + } }), + event(2, 'step.attempt.started', 'authored-root', T0 + 10, { lease_deadline_ms: T0 + 30_000 }), + event(3, 'step.completed', 'authored-root', T0 + 20_000, { + completionReason: options.stepCompletionReason ?? 'success', + disposition: options.disposition ?? 'step_done', + ...(options.omitOutput === true ? {} : { output }), + }), + event(4, 'run.completed', null, T0 + 20_001, { completionReason: 'success' }), + ]; +} + +function fixture(events: JournalEvent[]) { + const dataDir = mkdtempSync(join(tmpdir(), 'authored-status-')); + directories.push(dataDir); + writeJournalFixture(dataDir, RUN_ID, events).writer.close(); + return dataDir; +} + +async function status(argv: string[], options: StatusOptions = {}) { + const stdout: string[] = []; + const stderr: string[] = []; + const parsed = parseStatusArgs(argv); + expect(parsed, `\`flows status ${argv.join(' ')}\` did not parse`).toBeDefined(); + const code = await runStatus(parsed!, { stdout: (l) => stdout.push(l), stderr: (l) => stderr.push(l) }, + { env: {}, now: () => T0 + 60_000, ...options }); + return { code, stdout, stderr }; +} + +describe('projecting the authored verdict out of a root journal', () => { + it('reads the verdict and its detail', () => { + expect(authoredVerdictOf(journal({ detail: FINDING }))) + .toEqual({ reason: 'step_failed', detail: FINDING }); + }); + + it('reads a verdict with no detail, and reports no detail key', () => { + const verdict = authoredVerdictOf(journal())!; + expect(verdict.reason).toBe('step_failed'); + expect('detail' in verdict).toBe(false); + }); + + it.each([ + ['an ordinary step that merely shares the name', { kind: null }], + ['a root declaring some other authority kind', { kind: 'relayflows.something-else.v1' }], + ['a completion that is a retry, not a terminal', { disposition: 'retry' }], + ['a completion that parked', { disposition: 'park' }], + ['a root step the kernel did not complete with success', { stepCompletionReason: 'worker_error' }], + ['a completion carrying no output at all', { omitOutput: true }], + ['an output whose reason is not a lowered completion', { reason: 'invented' }], + ['an output whose detail is not a string', { detail: 7 }], + ['an output whose detail is over the bound', { detail: 'a'.repeat(2001) }], + ] as const)('attests nothing for %s', (_label, options) => { + expect(authoredVerdictOf(journal(options))).toBeNull(); + }); + + it('attests nothing for a journal that is not a run at all', () => { + expect(authoredVerdictOf([])).toBeNull(); + expect(authoredVerdictOf(journal().slice(1))).toBeNull(); + }); +}); + +describe('flows status on a detail-bearing authored run', () => { + it('prints the authored verdict beside the kernel facts, not instead of them', async () => { + const output = await status(['--data-dir', fixture(journal({ detail: FINDING })), RUN_ID]); + + expect(output.code).toBe(0); + expect(output.stderr).toEqual([]); + // The kernel's own account is unchanged and still first. + expect(output.stdout[0]).toContain('completed'); + expect(output.stdout[0]).toContain('finished success'); + expect(output.stdout[1]).toBe(`authored done("step_failed"): ${FINDING}`); + // …and the root step is still the success it was. + expect(output.stdout.find((line) => line.includes('authored-root'))).toContain('✓'); + }); + + it('keeps the full 2,000-code-point detail, past the gate-detail limit', async () => { + const long = `${'a'.repeat(1_900)} the finding is at the very end`; + const output = await status(['--data-dir', fixture(journal({ detail: long })), RUN_ID]); + expect(output.stdout[1]).toBe(`authored done("step_failed"): ${long}`); + expect(output.stdout[1]!.length).toBeGreaterThan(1_024); + }); + + it('folds a multiline detail onto one line and strips control characters', async () => { + const multiline = 'P1: none\nP2: review.clean was not created\nP3: none\u0007'; + const output = await status(['--data-dir', fixture(journal({ detail: multiline })), RUN_ID]); + expect(output.stdout[1]) + .toBe('authored done("step_failed"): P1: none\\nP2: review.clean was not created\\nP3: none\\u0007'); + // One line, not three: the breaks are escaped, not printed. + expect(output.stdout[2]!.startsWith('steps ')).toBe(true); + }); + + it('exposes reason and detail in --json, canonically', async () => { + const output = await status(['--json', '--data-dir', fixture(journal({ detail: FINDING })), RUN_ID]); + const view = JSON.parse(output.stdout[0]!); + expect(view.authored_completion).toEqual({ reason: 'step_failed', detail: FINDING }); + // The kernel facts are untouched. + expect(view.status).toBe('completed'); + expect(view.completion_reason).toBe('success'); + expect(output.stdout[0]).toBe(canonicalize(view)); + }); + + it('redacts a secret on the way out, in both renderings', async () => { + const secret = 'super-secret-workspace-material'; + const detail = `review failed against ot_live_abc123DEF456 with key ${secret}`; + const dataDir = fixture(journal({ detail })); + const env = { RELAY_API_KEY: secret }; + const text = await status(['--data-dir', dataDir, RUN_ID], { env }); + const json = await status(['--json', '--data-dir', dataDir, RUN_ID], { env }); + for (const line of [...text.stdout, ...json.stdout]) { + expect(line).not.toContain(secret); + expect(line).not.toContain('ot_live_abc123DEF456'); + } + expect(text.stdout[1]).toContain('[redacted]'); + expect(text.stdout[1]).toContain('[redacted:RELAY_API_KEY]'); + }); + + it.each([ + ['a one-argument done()', journal()], + ['an ordinary run that happens to name a step authored-root', journal({ kind: null, detail: FINDING })], + ['a malformed authored output', journal({ detail: 7 })], + ])('adds no line and no JSON key for %s', async (_label, events) => { + const dataDir = fixture(events); + const text = await status(['--data-dir', dataDir, RUN_ID]); + expect(text.stdout[1]).not.toContain('authored done('); + expect(text.stdout[1]!.startsWith('steps ')).toBe(true); + const json = await status(['--json', '--data-dir', dataDir, RUN_ID]); + const view = JSON.parse(json.stdout[0]!); + expect('authored_completion' in view).toBe(false); + expect(json.stdout[0]).toBe(canonicalize(view)); + }); +}); diff --git a/packages/sdk/tests/authored-step-failed.test.ts b/packages/sdk/tests/authored-step-failed.test.ts index 2e9898be7..48db387ea 100644 --- a/packages/sdk/tests/authored-step-failed.test.ts +++ b/packages/sdk/tests/authored-step-failed.test.ts @@ -2,11 +2,8 @@ import { rmSync } from 'node:fs'; import type { Server } from 'node:net'; import { flow } from '@relayflows/surface'; import { afterAll, beforeAll, describe, expect, it } from 'vitest'; -import { - completionMarker, - executeAuthoredFlow, - isLoweredCompletion, -} from '../src/authored-flow-executor.js'; +import { completionMarker, isLoweredCompletion } from '../src/authored-completion.js'; +import { executeAuthoredFlow } from '../src/authored-flow-executor.js'; import { authoredCompletion, type RunReport } from '../src/cli/run.js'; import { JournalClient } from '../src/journal-client.js'; import { diff --git a/packages/sdk/tests/cloud-read.test.ts b/packages/sdk/tests/cloud-read.test.ts index 3bdfe82ea..e859ad266 100644 --- a/packages/sdk/tests/cloud-read.test.ts +++ b/packages/sdk/tests/cloud-read.test.ts @@ -14,6 +14,7 @@ import { errorLines, parseLogsArgs, parseRunsArgs, runCloudLogsCli, runCloudRunsCli, runCloudStatusCli, } from '../src/cli/cloud-read.js'; import { parseStatusArgs } from '../src/cli/status.js'; +import { authoredCompletion, type RunReport } from '../src/cli/run.js'; const RUN = '20d04c99-3fa8-48c9-9286-92d364a5bc2e'; const CONNECTION = { apiUrl: 'https://cloud-contract.example', token: 'test-scoped-cloud-token', env: {} }; @@ -482,6 +483,74 @@ describe('flows status --cloud', () => { }); }); +/** + * Client contract only. These tests prove that a detail-bearing diagnostic + * message, once it is a run's `error`, reaches a reader through `flows status + * --cloud` in both renderings. They do NOT establish how the server derives + * `error` from the CLI's report — that projection lives in agentrelay.com, is + * not in this repo, and is not verified here. + */ +describe('flows status --cloud on a run whose error is an authored detail', () => { + const FINDING = 'One P2 remains: cleanup can report success while an ambiguous ' + + 'allocation stays invisible through all three sweeps — `review.clean` was not created.'; + + /** The real report message, not a hand-written imitation of one. */ + function diagnosticMessage(detail: string): string { + const base: RunReport = { ok: false, command: 'run', resolutions: [], diagnostics: [] }; + const execution = authoredCompletion('run', base, '/sock', { + name: 'software-factory', completionReason: 'step_failed', completionDetail: detail, journalSteps: [{}], + }, RUN); + return execution.report.diagnostics.at(-1)!.message; + } + + function failedRun(error: string) { + cloud((path) => { + if (path === `/api/v1/workflows/runs/${RUN}`) { + return { body: { ...RUN_DETAIL, status: 'failed', completionReason: 'step_failed', error } }; + } + if (path === `/api/v1/workflows/runs/${RUN}/steps`) return { body: { steps: [] } }; + return { status: 404, body: { error: 'Run not found' } }; + }); + } + + it('prints the reviewer finding instead of the generic sentence', async () => { + const message = diagnosticMessage(FINDING); + expect(message).toContain(FINDING); + failedRun(message); + const out = io(); + expect(await runCloudStatusCli({ runId: RUN, json: false }, out.io, { ...CONNECTION, now: () => 1 })).toBe(0); + const rendered = out.stdout.join('\n'); + expect(rendered).toContain('error'); + expect(rendered).toContain(FINDING); + expect(rendered).not.toContain('no step-level evidence to inspect'); + }); + + it('keeps a finding in the MIDDLE of a long detail, because the message is one line', async () => { + // `errorLines` elides the middle of a multi-line error (HEAD 2, TAIL 12). + // A forty-line detail rendered as forty lines would lose exactly this. + const lines = Array.from({ length: 40 }, (_, index) => `P3-${index}: nothing to report here`); + lines[20] = FINDING; + const message = diagnosticMessage(lines.join('\n')); + failedRun(message); + const out = io(); + await runCloudStatusCli({ runId: RUN, json: false }, out.io, { ...CONNECTION, now: () => 1 }); + const rendered = out.stdout.join('\n'); + expect(rendered).not.toContain('more lines (full text: --json)'); + expect(rendered).toContain(FINDING); + }); + + it('carries the message through --json and redacts a token in it', async () => { + const message = diagnosticMessage(`${FINDING} see ot_live_abc123DEF456`); + failedRun(message); + const out = io(); + await runCloudStatusCli({ runId: RUN, json: true }, out.io, { ...CONNECTION, now: () => 1 }); + const payload = JSON.parse(out.stdout[0]!) as { run: { error: string } }; + expect(payload.run.error).toContain(FINDING); + expect(payload.run.error).not.toContain('ot_live_abc123DEF456'); + expect(payload.run.error).toContain('[redacted]'); + }); +}); + describe('refusals', () => { it('names `agent-relay cloud login` when there is no credential at all', async () => { vi.stubEnv('FLOWS_CLOUD_TOKEN', undefined); diff --git a/packages/sdk/tsconfig.tests.json b/packages/sdk/tsconfig.tests.json index a8aeec620..a58e1106a 100644 --- a/packages/sdk/tsconfig.tests.json +++ b/packages/sdk/tsconfig.tests.json @@ -25,6 +25,10 @@ "tests/authored-flow-lifecycle-executor.test.ts", "tests/authored-flow-operation.test.ts", "tests/authored-flow.test.ts", + "tests/authored-completion-detail.test.ts", + "tests/authored-status-detail.test.ts", + "tests/authored-detail-live.test.ts", + "tests/authored-completion-recovery.test.ts", "tests/authored-agent-permissions.test.ts", "tests/hosted-extension-isolation.test.ts", "tests/hosted-extension-routing.test.ts", diff --git a/packages/surface/src/completion.ts b/packages/surface/src/completion.ts index 01cc27347..bcfeecb72 100644 --- a/packages/surface/src/completion.ts +++ b/packages/surface/src/completion.ts @@ -31,3 +31,19 @@ export const FLOW_COMPLETION_REASONS = [ ] as const; export type FlowCompletionReason = (typeof FLOW_COMPLETION_REASONS)[number]; + +/** + * The bound on the optional `done()` detail, in Unicode code points of the + * FINAL normalized string — truncation suffix included. + * + * Stated in code points rather than `String.length` on purpose: `.length` + * counts UTF-16 code units, so an emoji-heavy detail measured that way is + * half the length a reader would call it. The number is the same order the + * kernel uses for a gate's free-text detail + * (`kernel/relayflowd/src/engine/remote.rs`), but it is not the same bound: + * that one takes 2,000 characters and then appends its suffix. + * + * The runtime enforces this; it is exported so an author can measure a detail + * before passing one. + */ +export const COMPLETION_DETAIL_MAX_CODE_POINTS = 2000; diff --git a/packages/surface/src/context.ts b/packages/surface/src/context.ts index 7dd467d54..c340c7bac 100644 --- a/packages/surface/src/context.ts +++ b/packages/surface/src/context.ts @@ -49,6 +49,12 @@ export interface LlmOptions { model?: string; } +/** The optional second argument to {@link Ctx.done}. */ +export interface DoneOptions { + /** Why the flow reached this verdict; redacted, bounded, and journaled. */ + detail?: string; +} + /** * The context a journal-backed runtime injects into a flow body. * @@ -80,7 +86,21 @@ export interface Ctx extends Helpers { * no-op that returns true. A name must appear in the flow header's `hooks`. */ hook(name: string, input: unknown): Step; - done(reason: FlowCompletionReason): void; + /** + * End the flow with an authored verdict, and optionally say why. + * + * `options.detail` is free prose the flow already knows — "review found 1 + * P2: `review.clean` was not created" — and it is what a reader gets instead + * of a generic sentence. It is journaled with the verdict, so it survives + * into the run report and `flows status`; the runtime redacts it and bounds + * it to `COMPLETION_DETAIL_MAX_CODE_POINTS` code points, truncating + * with a visible marker rather than refusing an over-long one. + * + * Whitespace-only is the same as saying nothing: it normalizes to absence, + * and the verdict reports exactly as the one-argument call does. A `detail` + * that is present and not a string is refused. + */ + done(reason: FlowCompletionReason, options?: DoneOptions): void; cloud: CloudHelper; memory: MemoryHelper; } diff --git a/packages/surface/src/index.ts b/packages/surface/src/index.ts index 2daff08e3..5fc92d100 100644 --- a/packages/surface/src/index.ts +++ b/packages/surface/src/index.ts @@ -11,8 +11,9 @@ export type { CloudBabysitterTurnDelivery, CloudBabysitterTurnReceipt, } from "./cloud.js"; -export type { AgentOptions, AgentResult, PermissionsSpec, LlmOptions, Ctx } from "./context.js"; +export type { AgentOptions, AgentResult, DoneOptions, PermissionsSpec, LlmOptions, Ctx } from "./context.js"; export { + COMPLETION_DETAIL_MAX_CODE_POINTS, COMPLETION_REASONS, RUN_COMPLETION_REASONS, FLOW_COMPLETION_REASONS, diff --git a/packages/surface/tests/done-detail.test.ts b/packages/surface/tests/done-detail.test.ts new file mode 100644 index 000000000..ba47191a9 --- /dev/null +++ b/packages/surface/tests/done-detail.test.ts @@ -0,0 +1,33 @@ +import { expect, it } from 'vitest'; +import { COMPLETION_DETAIL_MAX_CODE_POINTS, type Ctx, type DoneOptions } from '../src/index.js'; + +// The authoring contract, checked by the compiler. The one-argument call is +// first because it is the one every existing flow makes: adding an optional +// second parameter must not make any of them stop type-checking. +const author = (f: Ctx): void => { + f.done('step_failed'); + f.done('step_failed', {}); + f.done('step_failed', { detail: undefined }); + f.done('step_failed', { detail: 'review found 1 P2: `review.clean` was not created' }); + f.done('needs_human', { detail: 'the allocation is ambiguous' }); + f.done('declined', { detail: 'no ticket in the input' }); + f.done('success', { detail: 'all three sweeps clean' }); + // @ts-expect-error a detail is prose, not a number + f.done('step_failed', { detail: 5 }); + // @ts-expect-error the options are an object, not the detail itself + f.done('step_failed', 'review found 1 P2'); + // @ts-expect-error unknown option + f.done('step_failed', { reason: 'review found 1 P2' }); + // @ts-expect-error done takes at most two arguments + f.done('step_failed', {}, {}); + // @ts-expect-error step-only reason, unchanged by the new argument + f.done('verification_failed', { detail: 'x' }); +}; +void author; + +const options: DoneOptions = { detail: 'exported for authors who build one' }; +void options; + +it('publishes the detail bound authors are held to', () => { + expect(COMPLETION_DETAIL_MAX_CODE_POINTS).toBe(2000); +});