Software factory change

docs/CLOUD.md

@@ -270,6 +270,27 @@ step rows because the run record carries no total, and Cloud stores each
 step's cost as a float — unlike the local view, which adds the journal's
 decimal strings exactly (`run-state.ts`).
 
+A failed run also prints its `error`, which is where an authored verdict's
+detail arrives. A flow that ends `done("step_failed", { detail })` puts that
+sentence in the run report's `step_failed` diagnostic message, and wherever
+Cloud's run record carries that message as the run's `error`,
+`flows status --cloud` renders it under an `error` heading, redacted again on
+the way out. The message is deliberately one line even when the detail the
+flow passed had line breaks in it — the escapes `\n`, `\r`, `\t` and
+`\uXXXX` are how the breaks appear — because this view elides the middle of a
+long multi-line error, and a forty-line detail rendered as forty lines would
+lose exactly the finding it exists to carry. The unescaped detail is in the
+report's own `completionDetail` and in the diagnostic's `detail`. See
+docs/SURFACE.md for the bound and the redaction.
+
+The step between those two — report diagnostic to stored `error` — is the
+server's, and this repository does not establish it. What is pinned here is
+the client half: the message this CLI produces, and the rendering
+`flows status --cloud` gives an `error` that holds it (`cloud-read.test.ts`,
+which injects the diagnostic into a mocked Cloud record). Treat the hosted end
+to end as unconfirmed until a hosted run or the server source says otherwise;
+that is an evidence limit, not a claim that Cloud drops the field.
+
 Every refusal is one `REFUSED [code] message` line naming what to do next:
 
 | code | when |

docs/SURFACE.md

@@ -1080,6 +1080,79 @@ terminal markers are steps that SUCCEED: `done("step_failed")` is the body's
 verdict, not a step that failed, so the terminal marker does not fabricate a
 failing step. Actual step failures still take precedence over authored verdicts.
 
+### Saying why: `done(reason, { detail })`
+
+`done()` takes an optional second argument, `{ detail }`, and it is what a
+reader gets instead of a generic sentence. A flow that knows "one P2 remains:
+`review.clean` was not created" should say so; without it the run record has
+only "its own checks did not pass".
+
+```ts
+f.done("step_failed", { detail: "review found 1 P2: `review.clean` was not created" });
+```
+
+The detail is normalized once, at `done()`, before anything durable is written:
+
+- **Redacted** with the SDK's existing redactor — known token shapes, named
+  credential fields, and the values of secret-looking environment variables.
+  That is a policy, not a promise to recognise every possible secret.
+- **Bounded** to 2,000 Unicode code points *including* the fixed
+  `… (truncated)` suffix, exported as `COMPLETION_DETAIL_MAX_CODE_POINTS`.
+  Over-long details are truncated with that visible marker rather than
+  refused: killing a twenty-step run at its last line because its explanation
+  ran long destroys more evidence than it preserves. Redaction runs BEFORE
+  truncation, because truncating first can split a token so no pattern matches
+  it any more — a truncation that *causes* a leak.
+- **Normalized to absence** when it is empty or whitespace-only. No options,
+  `{}`, `{ detail: undefined }` and `{ detail: "  " }` all mean the same thing
+  as the one-argument call, down to a byte-identical marker command.
+- **Made well-formed**: every lone UTF-16 surrogate becomes U+FFFD. A JS
+  string is code units, not text, and `(prose + "\u{1F642}").slice(0, -1)` —
+  ordinary trimming of an agent's output — leaves a high surrogate with no
+  partner. The journal protocol's JSON decoder refuses such a value, and the
+  refusal carries no request id to answer, so the call never returns. One code
+  unit is substituted for one, so the bound still counts what a reader counts.
+
+A `detail` that is present and not a string, or options that are not an
+object, are refused with `unsupported_completion`. The refusal names the type
+it received and never the value.
+
+With a detail, the marker's stdout is
+`{"completionReason":"<reason>","detail":"<detail>"}`; with none it is exactly
+the `{"completionReason":"<reason>"}` it has always been, so a flow that does
+not opt in keeps its `spec_hash`. The detail is journaled on the authored
+root's output, returned by `flows run --json` as `completionDetail` and on the
+`step_failed` diagnostic's `detail`, and printed by `flows status` as a
+labelled line beside the kernel's own account:
+
+```text
+RUN 9e1a0f2c-…   software-factory   completed   started 20.0s ago   finished success   spend …
+authored done("step_failed"): review found 1 P2: `review.clean` was not created
+steps 1: 1 done
+```
+
+A verdict that carries a detail is **committed before the marker run is
+opened**, on a stream of the flow's own root, exactly as a predicate gate's
+verdict is. That is what makes it survive a resume: redaction reads the
+process environment, so a credential rotated while the process was down would
+otherwise re-normalize the same authored sentence into a different marker
+command — and the marker's admission key is stable, so the kernel would refuse
+the drifted spec as `run_admission_conflict` and the run would lose the
+explanation it had already journaled. A resumed body reuses the committed
+verdict instead of recomputing one. A `done()` with no detail commits nothing:
+its marker command is a function of the reason alone, so there is nothing that
+can drift and nothing to recover.
+
+The kernel facts on the first line do not move: an authored `step_failed` run
+completes with a root step that succeeded, and that stays true and stays
+printed. In the diagnostic *message* — the string a Cloud run's `error` is
+expected to carry — the detail is folded onto one line, with `\n`, `\r`, `\t`
+and `\uXXXX` standing in for control characters, because Cloud's error view
+elides the middle of a long multi-line error. (That projection is the
+server's; see docs/CLOUD.md for what this repository does and does not
+establish about it.) The unescaped text is in `completionDetail` and in the
+diagnostic's `detail` beside it.
+
 `canceled` and `budget_exceeded` are in the type but are refused with
 `unsupported_completion`. They are kernel outcomes, not authored verdicts: the
 kernel records them when it cancels a run or exhausts its budget, and a body
@@ -1107,7 +1180,7 @@ The exit codes are part of the surface contract:
 | Exit | Outcome |
 |---:|---|
 | `0` | The run completed with `completionReason: success`; deliberate declination also carries a `run_declined` diagnostic locally. |
-| `1` | The run failed with a declared `completionReason`, or a transport, runtime, or daemon protocol error left the outcome unknown. A `step_failed` run names the failing step and its per-step `completionReason`, plus the exit code and output tails the journal recorded for it. An authored `done("step_failed")` exits `1` as well, and says so without naming a step, because no step failed — the body declared the verdict. |
+| `1` | The run failed with a declared `completionReason`, or a transport, runtime, or daemon protocol error left the outcome unknown. A `step_failed` run names the failing step and its per-step `completionReason`, plus the exit code and output tails the journal recorded for it. An authored `done("step_failed")` exits `1` as well, and says so without naming a step, because no step failed — the body declared the verdict. With a `detail`, that detail replaces the generic sentence and is reported as `completionDetail`. |
 | `2` | The command was refused before a journal write: invalid input, failed preflight, unreachable daemon, or a `run_not_found` resume target. |
 | `3` | The run parked. `PARKED [run_parked]` names the step and its `llm` or `agent` type, and distinguishes an unavailable worker from a `needs_human` recovery wait. An authored body parked on `f.human` reports the question, who it is for, and the `flows answer` invocation that records the decision (see *Human gates* below). |
 

kernel/relayflowd/src/server/tests.rs

@@ -119,6 +119,132 @@ fn run_start_admission_key_recovers_the_same_run_and_refuses_spec_drift() {
     assert_eq!(drifted.error.unwrap().code, "run_admission_conflict");
 }
 
+/// flows#545: an authored `done("step_failed", { detail })` travels as JSON
+/// inside the terminal marker's deterministic command.
+///
+/// The SDK relies on three kernel properties for that to be durable evidence
+/// rather than a claim: the command is journaled verbatim and survives a
+/// reopen, re-admitting the identical spec under the same admission key
+/// returns the same run without spawning a second one, and a spec whose
+/// marker data changed is refused instead of quietly admitted. No kernel
+/// change was needed for the detail — it is opaque data in a shell string —
+/// so this test exists to keep it that way.
+#[test]
+fn deterministic_marker_carrying_json_survives_reopen_and_refuses_changed_detail() {
+    let directory = tempdir().unwrap();
+    let data_dir = directory.path();
+    let hub = Arc::new(ProtocolHub::default());
+    let (writer, _peer) = shared_writer();
+    let marker = |detail: &str| {
+        format!(
+            r#"printf '%s' '{{"completionReason":"step_failed","detail":"{detail}"}}'"#
+        )
+    };
+    let start = |id: &str, command: &str| {
+        json!({"id": id, "verb": "run.start", "params": {
+            "admission_key": "authored-child:complete-2",
+            "spec": {"name": "software-factory/complete-2", "steps": [{
+                "id": "complete-2", "type": "deterministic", "command": command,
+            }]}
+        }})
+        .to_string()
+    };
+
+    let detail = "review found 1 P2: review.clean was not created";
+    let first = request(data_dir, &hub, 1, &writer, &start("one", &marker(detail)));
+    assert!(first.ok, "first admission failed: {:?}", first.error);
+    let run_id = first.result.unwrap()["run_id"].as_str().unwrap().to_owned();
+
+    // Same spec, same key: the same run, not a second effect.
+    let retried = request(data_dir, &hub, 1, &writer, &start("two", &marker(detail)));
+    assert!(retried.ok, "idempotent retry failed: {:?}", retried.error);
+    assert_eq!(retried.result.unwrap()["run_id"], run_id);
+
+    // Reopened from disk by this request, the journal holds exactly one
+    // spawn, and the marker command came back character for character.
+    let read = request(
+        data_dir,
+        &hub,
+        1,
+        &writer,
+        &json!({"id":"read","verb":"journal.read","params":{"run_id": run_id}}).to_string(),
+    );
+    assert!(read.ok, "journal.read failed: {:?}", read.error);
+    let entries = read.result.unwrap();
+    let spawned: Vec<_> = entries["entries"]
+        .as_array()
+        .unwrap()
+        .iter()
+        .filter(|entry| entry["entry_type"] == "run.spawned")
+        .collect();
+    assert_eq!(spawned.len(), 1, "one admitted run, one spawn");
+    assert_eq!(
+        spawned[0]["payload"]["spec"]["steps"][0]["command"]
+            .as_str()
+            .unwrap(),
+        marker(detail),
+    );
+
+    // One character of the detail differs: a different spec under an identity
+    // that is already spoken for, and the kernel fails closed rather than
+    // letting a second verdict take the first one's place.
+    let drifted = request(
+        data_dir,
+        &hub,
+        1,
+        &writer,
+        &start("three", &marker("review found 2 P2s")),
+    );
+    assert!(!drifted.ok);
+    assert_eq!(drifted.error.unwrap().code, "run_admission_conflict");
+}
+
+/// flows#545: a lone UTF-16 surrogate is refused at the line, with no id.
+///
+/// This is the kernel property the SDK's detail normalization exists for. A
+/// JavaScript string is code units, not text, so trimming an agent's output —
+/// `(prose + "\u{1F642}").slice(0, -1)` — produces a `string` whose last half
+/// of a surrogate pair has no partner. `JSON.stringify` escapes it, so the
+/// frame is syntactically sendable; this decoder is where it stops. The
+/// refusal carries `"id": null`, because the id is inside the frame that did
+/// not parse — so it resolves no pending client request, and a caller that is
+/// waiting on one waits forever. Normalizing before the write
+/// (`packages/sdk/src/authored-completion.ts`) is the only place that can
+/// keep an authored explanation out of this.
+#[test]
+fn a_lone_surrogate_in_a_request_is_refused_with_no_request_id() {
+    let directory = tempdir().unwrap();
+    let hub = Arc::new(ProtocolHub::default());
+    let (writer, _peer) = shared_writer();
+
+    let refused = request(
+        directory.path(),
+        &hub,
+        1,
+        &writer,
+        r#"{"id":"probe","verb":"hello","params":{"protocol":0,"client":"review found 1 P2: \ud83d"}}"#,
+    );
+
+    assert!(!refused.ok);
+    assert_eq!(
+        refused.id,
+        Value::Null,
+        "a refusal no pending request can be matched to"
+    );
+    assert_eq!(refused.error.unwrap().code, "bad_request");
+
+    // The complete pair is ordinary text and is accepted, so what the decoder
+    // refuses above is the lone half, not the emoji.
+    let paired = request(
+        directory.path(),
+        &hub,
+        1,
+        &writer,
+        r#"{"id":"probe","verb":"hello","params":{"protocol":0,"client":"review found 1 P2: \ud83d\ude42"}}"#,
+    );
+    assert!(paired.ok, "a well-formed pair was refused: {:?}", paired.error);
+}
+
 #[test]
 fn run_start_refuses_invalid_admission_keys() {
     let directory = tempdir().unwrap();

packages/sdk/src/authored-completion-record.ts

@@ -0,0 +1,119 @@
+// The authored verdict, written to the root BEFORE the marker is admitted.
+//
+// The terminal marker's command embeds the detail, and the marker run is
+// opened under a stable admission key (`authored-child:<hash(root, step)>`).
+// Those two facts together mean the detail has to be a durable decision, not a
+// recomputed one: normalization redacts against `process.env`, so a credential
+// rotated or removed while this process was down makes the SAME authored
+// sentence normalize differently. A body resumed into that environment would
+// retry the marker's admission key with a drifted spec, and the kernel refuses
+// that as `run_admission_conflict` — losing an explanation the root had
+// already journaled, for a flow whose every step succeeded.
+//
+// So the verdict is appended to a stream on the root run before the marker run
+// is opened, and a later attempt reaching the same terminal step reuses what
+// is recorded. That is exactly the durability the executor already gives a
+// predicate gate (`applyPredicateGate`): record the decision first, so the
+// spec built from it is identical on every attempt and author code is never
+// asked the question twice.
+//
+// Nothing is recorded for a `done()` with no detail. Without one the marker
+// command is a function of the reason alone — no environment, no drift, and
+// nothing to recover — so a flow that does not opt in writes exactly the
+// journal it has always written.
+
+import {
+  isDurableCompletionDetail,
+  isLoweredCompletion,
+  type LoweredCompletionReason,
+} from './authored-completion.js';
+import type { JournalClient } from './journal-client.js';
+
+export const AUTHORED_VERDICT_STREAM = 'authored-verdict';
+
+/** Versioned so a reader can tell this record from a future one. */
+const RECORD_KIND = 'relayflows.authored-verdict.v1';
+
+export interface AuthoredVerdictRecord {
+  readonly reason: LoweredCompletionReason;
+  /** Recorded only when the body passed one; see the module comment. */
+  readonly detail?: string;
+}
+
+/**
+ * Commit this body's verdict, or recover the one this root already committed.
+ *
+ * Returns the verdict the marker must be built from — the recorded one when
+ * the root holds it, this attempt's otherwise. With no root run there is
+ * nothing durable to write to (the non-durable executor seam), so that path
+ * passes straight through without a round trip. A root run is always read
+ * first: a committed record stays authoritative even for an attempt that has
+ * no detail of its own — an optional source can be present on the first
+ * execution and gone on the retry, and "this attempt has no detail" is not
+ * "this root never recorded one". Only when no record exists AND this attempt
+ * has no detail does it return without appending, preserving the no-detail
+ * marker, spec hash, and journal exactly as before.
+ */
+export async function commitAuthoredVerdict(
+  journal: JournalClient,
+  rootRunId: string | undefined,
+  step: string,
+  verdict: AuthoredVerdictRecord,
+): Promise<AuthoredVerdictRecord> {
+  if (rootRunId === undefined) return verdict;
+  const committed = await readAuthoredVerdict(journal, rootRunId, step);
+  if (committed !== undefined) return committed;
+  if (verdict.detail === undefined) return verdict;
+  await journal.streamAppend(rootRunId, AUTHORED_VERDICT_STREAM, {
+    verdict: RECORD_KIND, step, reason: verdict.reason, detail: verdict.detail,
+  });
+  return verdict;
+}
+
+/**
+ * The verdict this root recorded for `step`, or `undefined`.
+ *
+ * The FIRST valid record wins: it is the one the marker was — or was about to
+ * be — admitted from, and a later attempt's differently-redacted text must not
+ * displace it. Records this version does not recognise, and details that fail
+ * the durable check every other read boundary applies, are skipped rather than
+ * trusted; the marker then drifts and the kernel refuses it loudly, which is
+ * the right outcome for a record this process cannot read.
+ */
+export async function readAuthoredVerdict(
+  journal: JournalClient,
+  rootRunId: string,
+  step: string,
+): Promise<AuthoredVerdictRecord | undefined> {
+  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<StoredVerdict>;
+  return record.verdict === RECORD_KIND
+    && typeof record.step === 'string' && record.step.length > 0
+    && isLoweredCompletion(record.reason)
+    && isDurableCompletionDetail(record.detail);
+}