From d6fdc63e4c15ce607432ce4a7d85884f3384d07e Mon Sep 17 00:00:00 2001 From: Relayflow Date: Mon, 21 Sep 2026 22:36:26 +0000 Subject: [PATCH 1/3] done(reason, { detail }): let a flow say why it failed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ctx.done(reason)` took only the reason, so every authored `step_failed` landed on one generic sentence — "No step failed, so there is no step-level evidence to inspect". Cloud run f92bf832 finished that way after twenty successful steps and an opened PR; the reviewer's actual finding ("one P2 remains: `review.clean` was not created") existed and reached no reader. Finding it took opening the Codex reviewer's raw transcript. `done()` now takes an optional `{ detail }`. It is normalized once, at `done()`: redacted with the SDK's existing redactor, trimmed, and bounded to 2,000 Unicode code points including a fixed `… (truncated)` suffix. Redaction runs BEFORE truncation, because cutting a secret in half leaves nothing for an exact-match redaction to find — a truncation that causes the leak. The detail travels inside the terminal marker's deterministic command, so it is a durable journal fact and the IPC verifier's existing command comparison attests it for free. It is carried on the authored root's output, read back by a completed-root resume without recomputing it, and reported as `completionDetail` plus the diagnostic's `detail`; for `step_failed` it replaces the generic sentence. `flows status` prints it as a labelled `authored done("...")` line beside — never instead of — the kernel's own account, which stays `completed / success` because that is what happened. No detail means no change: the marker command, the lowered spec, its canonical hash, the report message and every JSON shape are byte-identical, pinned against fixtures captured from the pre-change tree. Kernel: no production change. The detail is opaque data in a shell string. A regression pins the three kernel properties the SDK relies on — the marker command survives journal persistence and reopen verbatim, re-admitting it under the same admission key returns the same run, and changed marker data is refused as `run_admission_conflict`. Co-Authored-By: Claude Opus 5 --- docs/CLOUD.md | 16 + docs/SURFACE.md | 55 ++- kernel/relayflowd/src/server/tests.rs | 80 ++++ packages/sdk/src/authored-completion.ts | 167 +++++++ packages/sdk/src/authored-flow-executor.ts | 60 +-- packages/sdk/src/authored-node-runner.ts | 10 +- packages/sdk/src/authored-root.ts | 20 +- packages/sdk/src/authored-verdict.ts | 87 ++++ packages/sdk/src/cli/run.ts | 50 ++- packages/sdk/src/cli/status.ts | 47 +- .../tests/authored-completion-detail.test.ts | 425 ++++++++++++++++++ .../sdk/tests/authored-detail-live.test.ts | 116 +++++ .../sdk/tests/authored-node-result.test.ts | 28 ++ packages/sdk/tests/authored-root.test.ts | 103 ++++- .../sdk/tests/authored-status-detail.test.ts | 187 ++++++++ .../sdk/tests/authored-step-failed.test.ts | 7 +- packages/sdk/tests/cloud-read.test.ts | 69 +++ packages/sdk/tsconfig.tests.json | 3 + packages/surface/src/completion.ts | 16 + packages/surface/src/context.ts | 22 +- packages/surface/src/index.ts | 3 +- packages/surface/tests/done-detail.test.ts | 33 ++ 22 files changed, 1541 insertions(+), 63 deletions(-) create mode 100644 packages/sdk/src/authored-completion.ts create mode 100644 packages/sdk/src/authored-verdict.ts create mode 100644 packages/sdk/tests/authored-completion-detail.test.ts create mode 100644 packages/sdk/tests/authored-detail-live.test.ts create mode 100644 packages/sdk/tests/authored-status-detail.test.ts create mode 100644 packages/surface/tests/done-detail.test.ts diff --git a/docs/CLOUD.md b/docs/CLOUD.md index 1fc6bc839..714b075fa 100644 --- a/docs/CLOUD.md +++ b/docs/CLOUD.md @@ -229,6 +229,22 @@ 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 Cloud stores +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. + +Cloud's projection of the report into `error` is the server's, not this +client's; what this repository establishes is that the message reaches a +reader once `error` holds it. + 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 8b98dad6f..54ee843c9 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -999,6 +999,59 @@ 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. + +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 +``` + +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 Cloud stores as a run's +`error` — 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. 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 @@ -1026,7 +1079,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..b48a284c2 100644 --- a/kernel/relayflowd/src/server/tests.rs +++ b/kernel/relayflowd/src/server/tests.rs @@ -119,6 +119,86 @@ 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"); +} + #[test] fn run_start_refuses_invalid_admission_keys() { let directory = tempdir().unwrap(); diff --git a/packages/sdk/src/authored-completion.ts b/packages/sdk/src/authored-completion.ts new file mode 100644 index 000000000..39e21e013 --- /dev/null +++ b/packages/sdk/src/authored-completion.ts @@ -0,0 +1,167 @@ +// 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. + */ +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 = redact(detail, env).trim(); + return redacted.length === 0 ? undefined : bound(redacted); +} + +/** + * 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 + && [...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 bbd66d05e..0b12eeada 100644 --- a/packages/sdk/src/authored-flow-executor.ts +++ b/packages/sdk/src/authored-flow-executor.ts @@ -13,6 +13,12 @@ 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 { type AgentResult, type LlmOptions, @@ -89,6 +95,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[]; } @@ -211,6 +223,7 @@ export async function executeAuthoredFlow( const lifecycle = new AuthoredFlowLifecycle(); let nextStep = 1; let requestedCompletion: LoweredCompletionReason | undefined; + let requestedDetail: string | undefined; const lowerDeterministic = authoredDeterministicRunner( definition.name, journal, journalSteps, budget, { @@ -512,7 +525,7 @@ export async function executeAuthoredFlow( lifecycle, )); }, - done(reason) { + done(reason, doneOptions) { if (!isSurfaceFlowCompletionReason(reason)) { throw new AuthoredFlowExecutionError( 'unsupported_completion', @@ -545,8 +558,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), @@ -619,11 +637,12 @@ export async function executeAuthoredFlow( // 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); + completionMarker(requestedCompletion, requestedDetail), true); return Object.freeze({ ...(options.rootRunId === undefined ? {} : { rootRunId: options.rootRunId }), name: definition.name, completionReason: requestedCompletion, + ...(requestedDetail === undefined ? {} : { completionDetail: requestedDetail }), journalSteps: Object.freeze([...journalSteps]), }); } @@ -643,43 +662,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 1dd97944a..763840f36 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, @@ -211,6 +211,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'); @@ -287,8 +290,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 0b6513042..130c207b9 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; @@ -226,6 +233,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, @@ -368,9 +376,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, }); @@ -381,6 +393,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 8b3386949..1f51f71d0 100644 --- a/packages/sdk/src/cli/run.ts +++ b/packages/sdk/src/cli/run.ts @@ -26,7 +26,10 @@ import type { RunStatus, } from '../protocol.js'; import type { StepType } from '../spec.js'; -import type { LoweredCompletionReason } from '../authored-flow-executor.js'; +import { + singleLineCompletionDetail, + type LoweredCompletionReason, +} from '../authored-completion.js'; import { checkFlow, type CheckReport, @@ -56,6 +59,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; @@ -398,15 +408,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 { @@ -420,7 +445,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}`), }], }, }; @@ -431,7 +458,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}`), }], }, }; @@ -447,9 +476,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 5567b3dee..4eeb9e4ad 100644 --- a/packages/sdk/src/cli/status.ts +++ b/packages/sdk/src/cli/status.ts @@ -12,6 +12,8 @@ import type { CliIo } from '../cli.js'; 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'; @@ -36,6 +38,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; @@ -177,7 +184,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) { io.stdout(canonicalize({ v: 1, ...presented, partial: taken.partial })); } else { @@ -188,13 +195,41 @@ export async function runStatus(args: StatusArgs, io: CliIo, options: StatusOpti } 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 @@ -258,6 +293,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..336919cf2 --- /dev/null +++ b/packages/sdk/tests/authored-completion-detail.test.ts @@ -0,0 +1,425 @@ +// `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('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('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'); + }); +}); + +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-detail-live.test.ts b/packages/sdk/tests/authored-detail-live.test.ts new file mode 100644 index 000000000..f16824259 --- /dev/null +++ b/packages/sdk/tests/authored-detail-live.test.ts @@ -0,0 +1,116 @@ +// 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('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 9a6e2d9d5..f5e09adcd 100644 --- a/packages/sdk/tests/authored-node-result.test.ts +++ b/packages/sdk/tests/authored-node-result.test.ts @@ -101,6 +101,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 548772530..94ba014b2 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: {} }; @@ -445,6 +446,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 cc9298fa2..2633f398c 100644 --- a/packages/sdk/tsconfig.tests.json +++ b/packages/sdk/tsconfig.tests.json @@ -25,6 +25,9 @@ "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-agent-permissions.test.ts", "tests/flow-executor-chain.test.ts", "tests/input-binding.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 a9820ae76..829e274a0 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. * @@ -78,7 +84,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 4a6884f60..0602f303a 100644 --- a/packages/surface/src/index.ts +++ b/packages/surface/src/index.ts @@ -7,8 +7,9 @@ export type { WorkerSummary, CloudHelper, } 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); +}); From 43a3b227d0f3bb0a13bc5aa525e23c88bcb159dc Mon Sep 17 00:00:00 2001 From: Relayflow Date: Mon, 21 Sep 2026 23:00:02 +0000 Subject: [PATCH 2/3] Commit the authored verdict before the marker, and normalize lone surrogates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two P2 findings from the review of #547. **F1 — a committed detail is recovered, not recomputed.** The terminal marker's command embeds the detail and the marker run is opened under a stable admission key, but normalization redacts against `process.env`. A credential rotated or removed while the process was down made the SAME authored sentence normalize differently, so a resumed body retried the marker's key with a drifted spec and the kernel refused it as `run_admission_conflict` — losing an explanation the root had already journaled, for a flow whose every step succeeded. The verdict is now appended to a stream on the root BEFORE the marker run is opened, and a later attempt reaching the same terminal step reuses what was recorded. That is the durability the executor already gives a predicate gate: record the decision first, so the spec built from it is identical on every attempt. A `done()` with no detail commits nothing — its marker command is a function of the reason alone, nothing can drift, and that flow's journal is byte-for-byte the one it always wrote. **F2 — a lone surrogate never reaches a durable write.** `(prose + '\u{1F642}').slice(0, -1)` is ordinary JS trimming of an agent's output and produces a `string` that is not text. `JSON.stringify` escapes it, so the marker command was admitted; 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. Normalization now substitutes U+FFFD, one code unit for one, and `isDurableCompletionDetail` enforces the same invariant at every read boundary. Substituting rather than refusing: the flow has already reached its verdict, and losing the whole explanation over one broken code unit destroys more than it protects. Docs: `docs/CLOUD.md` no longer states the server's report-to-`error` projection as established fact. What this repo pins is the client half — the message this CLI produces and the rendering `flows status --cloud` gives an `error` that holds it; the hosted end to end is named as unconfirmed. Kernel: still no production change. One test pins the decoder property the SDK's normalization exists for — a lone surrogate is refused at the line, and the refusal carries `"id": null`, so no pending request can be matched to it. Co-Authored-By: Claude Opus 5 --- docs/CLOUD.md | 31 ++-- docs/SURFACE.md | 30 +++- kernel/relayflowd/src/server/tests.rs | 46 +++++ .../sdk/src/authored-completion-record.ts | 113 +++++++++++++ packages/sdk/src/authored-completion.ts | 38 ++++- packages/sdk/src/authored-flow-executor.ts | 23 ++- .../tests/authored-completion-detail.test.ts | 38 +++++ .../authored-completion-recovery.test.ts | 158 ++++++++++++++++++ .../sdk/tests/authored-detail-live.test.ts | 27 +++ packages/sdk/tsconfig.tests.json | 1 + 10 files changed, 482 insertions(+), 23 deletions(-) create mode 100644 packages/sdk/src/authored-completion-record.ts create mode 100644 packages/sdk/tests/authored-completion-recovery.test.ts diff --git a/docs/CLOUD.md b/docs/CLOUD.md index 714b075fa..16b78e3f6 100644 --- a/docs/CLOUD.md +++ b/docs/CLOUD.md @@ -231,19 +231,24 @@ 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 Cloud stores -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. - -Cloud's projection of the report into `error` is the server's, not this -client's; what this repository establishes is that the message reaches a -reader once `error` holds it. +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: diff --git a/docs/SURFACE.md b/docs/SURFACE.md index 54ee843c9..9283219eb 100644 --- a/docs/SURFACE.md +++ b/docs/SURFACE.md @@ -1025,6 +1025,12 @@ The detail is normalized once, at `done()`, before anything durable is written: - **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 @@ -1044,13 +1050,27 @@ 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 Cloud stores as a run's -`error` — 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. The unescaped text is in -`completionDetail` and in the diagnostic's `detail` beside it. +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 diff --git a/kernel/relayflowd/src/server/tests.rs b/kernel/relayflowd/src/server/tests.rs index b48a284c2..c1f6e237b 100644 --- a/kernel/relayflowd/src/server/tests.rs +++ b/kernel/relayflowd/src/server/tests.rs @@ -199,6 +199,52 @@ fn deterministic_marker_carrying_json_survives_reopen_and_refuses_changed_detail 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..620e17cb6 --- /dev/null +++ b/packages/sdk/src/authored-completion-record.ts @@ -0,0 +1,113 @@ +// 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) and with no + * detail there is nothing that can drift, so both pass straight through + * without a round trip. + */ +export async function commitAuthoredVerdict( + journal: JournalClient, + rootRunId: string | undefined, + step: string, + verdict: AuthoredVerdictRecord, +): Promise { + if (rootRunId === undefined || verdict.detail === undefined) return verdict; + const committed = await readAuthoredVerdict(journal, rootRunId, step); + if (committed !== undefined) return committed; + 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 index 39e21e013..f46eb5ab6 100644 --- a/packages/sdk/src/authored-completion.ts +++ b/packages/sdk/src/authored-completion.ts @@ -70,6 +70,9 @@ export const COMPLETION_DETAIL_TRUNCATED_SUFFIX = '… (truncated)'; * 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, @@ -90,10 +93,42 @@ export function normalizeCompletionDetail( `done() detail must be a string; received ${typeName(detail)}`, ); } - const redacted = redact(detail, env).trim(); + 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? * @@ -105,6 +140,7 @@ export function normalizeCompletionDetail( 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; } diff --git a/packages/sdk/src/authored-flow-executor.ts b/packages/sdk/src/authored-flow-executor.ts index 0b12eeada..fe46be025 100644 --- a/packages/sdk/src/authored-flow-executor.ts +++ b/packages/sdk/src/authored-flow-executor.ts @@ -19,6 +19,7 @@ import { normalizeCompletionDetail, type LoweredCompletionReason, } from './authored-completion.js'; +import { commitAuthoredVerdict } from './authored-completion-record.js'; import { type AgentResult, type LlmOptions, @@ -636,13 +637,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, requestedDetail), 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, - ...(requestedDetail === undefined ? {} : { completionDetail: requestedDetail }), + completionReason: verdict.reason, + ...(verdict.detail === undefined ? {} : { completionDetail: verdict.detail }), journalSteps: Object.freeze([...journalSteps]), }); } diff --git a/packages/sdk/tests/authored-completion-detail.test.ts b/packages/sdk/tests/authored-completion-detail.test.ts index 336919cf2..f159f5046 100644 --- a/packages/sdk/tests/authored-completion-detail.test.ts +++ b/packages/sdk/tests/authored-completion-detail.test.ts @@ -158,6 +158,26 @@ describe('normalizing done()\'s optional detail', () => { 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); @@ -169,6 +189,17 @@ describe('normalizing done()\'s optional detail', () => { 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)) @@ -199,6 +230,13 @@ describe('the terminal marker that carries the 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', () => { 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..88c5404e7 --- /dev/null +++ b/packages/sdk/tests/authored-completion-recovery.test.ts @@ -0,0 +1,158 @@ +// 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); + +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 index f16824259..357d4897c 100644 --- a/packages/sdk/tests/authored-detail-live.test.ts +++ b/packages/sdk/tests/authored-detail-live.test.ts @@ -82,6 +82,33 @@ export default flow('software-factory', async (f) => { 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()); diff --git a/packages/sdk/tsconfig.tests.json b/packages/sdk/tsconfig.tests.json index 2633f398c..adb4f96f2 100644 --- a/packages/sdk/tsconfig.tests.json +++ b/packages/sdk/tsconfig.tests.json @@ -28,6 +28,7 @@ "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/flow-executor-chain.test.ts", "tests/input-binding.test.ts", From dd6f0b8612957fe44306d4c2d0f0561b0631ed54 Mon Sep 17 00:00:00 2001 From: khaliqgant Date: Tue, 22 Sep 2026 22:07:42 -0700 Subject: [PATCH 3/3] fix(sdk): recover a committed authored verdict before the no-detail shortcut MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit commitAuthoredVerdict returned early when the current attempt's detail was undefined, before reading the root's committed verdict. An optional detail source present on the first execution can be absent on the retry; the committed record must stay authoritative in both loss windows — before marker admission (retry completes without completionDetail) and after (retry drifts the marker spec into run_admission_conflict). Read the root stream first; append only when no record exists and this attempt has a detail. Regressions cover both windows. --- .../sdk/src/authored-completion-record.ts | 14 +++- .../authored-completion-recovery.test.ts | 80 +++++++++++++++++++ 2 files changed, 90 insertions(+), 4 deletions(-) diff --git a/packages/sdk/src/authored-completion-record.ts b/packages/sdk/src/authored-completion-record.ts index 620e17cb6..3c159634a 100644 --- a/packages/sdk/src/authored-completion-record.ts +++ b/packages/sdk/src/authored-completion-record.ts @@ -45,9 +45,14 @@ export interface AuthoredVerdictRecord { * * 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) and with no - * detail there is nothing that can drift, so both pass straight through - * without a round trip. + * 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, @@ -55,9 +60,10 @@ export async function commitAuthoredVerdict( step: string, verdict: AuthoredVerdictRecord, ): Promise { - if (rootRunId === undefined || verdict.detail === undefined) return verdict; + 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, }); diff --git a/packages/sdk/tests/authored-completion-recovery.test.ts b/packages/sdk/tests/authored-completion-recovery.test.ts index 88c5404e7..e3f66b29d 100644 --- a/packages/sdk/tests/authored-completion-recovery.test.ts +++ b/packages/sdk/tests/authored-completion-recovery.test.ts @@ -139,6 +139,86 @@ it('commits the verdict once, and a clean re-execution reads it back', async () 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());