Skip to content

No advisory deterministic step: v2 gates every deterministic step on exit code, so repair-before-failure flows all re-invent '|| true' - #521

Merged
kjgbot merged 5 commits into
mainfrom
relayflow/flows-software-garden-b99b1be8
Sep 23, 2026
Merged

kjgbot merged 5 commits into
mainfrom
relayflow/flows-software-garden-b99b1be8

Conversation

@agent-relay-code

@agent-relay-code agent-relay-code Bot commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

Advisory deterministic outcomes: onNonZero: record and the steps_green gate

Closes the "no advisory deterministic step" gap. v2 gated every deterministic
step on exit code with no opt-out, so every repair-before-failure flow
re-invented <command> || true — including this repo's own v1→v2 bridge
(flows/spec-builder.ts) and flows/diagnose/orchestration.spec.ts's
advisoryGate.

|| true discards the exit code. A later gate has nothing to read, so the flow
either re-runs the command or builds an evidence journal outside the kernel.
Worse, it is indistinguishable from success: a flow that forgets one gate ships
red work silently.

What an author writes now

const tests = await f.run('npm test', { onNonZero: 'record' });
if (!tests.ok) {
  await f.agent('fixer', { task: `Fix these failures:\n${tests.output}` });
}
- id: check
  type: deterministic
  command: npm test
  onNonZero: record      # 'fail' (default) | 'record'

- id: green
  type: deterministic
  command: ./ship.sh
  verification: { type: steps_green, ids: [recheck] }

tests.ok, tests.exitCode, tests.stdout and tests.stderr are all read back
from the step's journaled {exit_code, stdout_tail, stderr_tail} envelope.
Nothing is re-run and nothing is re-measured — that is the claim || true
cannot make.

Acceptance

Ticket clause Where it is held
A deterministic step can be red without failing the run verify.rs a_recorded_nonzero_exit_passes_its_gate_and_names_the_code; relayflowd/tests/advisory_deterministic.rs
Its exit code and output are readable by a later step from the journal authored-advisory-run-live.test.ts "resolves to the journaled exit code instead of ending the flow" (asserts the repair branch received 2 failing tests)
A gate can assert "these recorded steps were all green" without re-running steps-green.ts + advisory-outcome.test.ts "steps_green lowering"
`

Design decisions worth reviewing

Recording is a policy about exit codes, and only about exit codes. A
timeout, a signal, the executor's -1 no-exit-status sentinel, a declared
content or schema gate, and a budget all stay fatal under either policy. -1
in particular is refused rather than handed back as a plausible code, mirroring
the kernel's own rule — otherwise a killed process would reach an author's
if (!result.ok) as a real red verdict.

Red is allowed, not invisible. The kernel labels the gate
exit_code:recorded and names the code in the verification detail;
flows check annotates the step with [onNonZero: record]. Reporting it as an
ordinary exit_code gate would have moved the || true ambiguity into the
inspection output.

The default is normalized away at both boundaries. onNonZero: fail is
dropped in compileStep and again in toKernelStep, and OnNonZero::is_default
skips it on the Rust side — so every spec written before this field keeps its
exact canonical bytes and its exact spec_hash. A parity test asserts that
directly.

steps_green is compiler-lowered, not a kernel gate. It becomes a separate
fatal deterministic step bound to the sources' envelopes, so the kernel never
sees the SDK spelling. testdata/advisory-repair.* pins the lowered output,
which is what proves the kernel parses what the SDK actually emits. The
generated gate never inherits its host's recording policy, or a red gate could
not fail anything.

Overload order on f.run. The two literal overloads come first so an
omitted or 'fail' policy keeps the Step<string> every existing body is
written against; only the literal 'record' widens the result. A third
union-returning overload covers a policy held in a variable, where the author
narrows it themselves rather than having one branch guessed for them.
packages/surface/tests/run-on-non-zero.test-d.ts pins all of this, including
that a recorded result does not assign to string.

A misspelled policy is refused, never defaulted. Falling back to the fatal
default would delete the branch the author wrote below the call. Refused in
validateSpec for declarative specs and before any ordinal is consumed in
f.run.

Bug found and fixed along the way

An authored .gate() lowers to its own kernel step (<id>.gate) that runs
after the producer. readCompletedStepOutput read only the producer's
step.completed entry, so a failed gate on a successful command resolved the
operation as though it had passed — the child run was terminally failed and
the author never heard about it.

This reproduces on the default policy too, so it predates this change, but
it is the same invisibility the recording policy exists to remove, one layer up,
and record would have widened its reach. readCompletedStepOutput now also
checks the run's own terminal reason from the entries it already reads.
authored-advisory-run-live.test.ts covers it under both policies and asserts
the failed step is named as run-1.gate, not the producer.

Evidence

  • cd kernel && sh ../ops/cargo.sh test — all suites pass, 0 failures
    (includes 6 new verify.rs tests, 2 new spec/tests.rs tests, 2 new
    relayflowd/tests/advisory_deterministic.rs integration tests, and the new
    spec_parity.rs fixture test).
  • npm --prefix packages/schema test — 78 pass (the new advisory-repair
    fixture is picked up by the existing parity walk).
  • npm --prefix packages/surface test — 46 pass, 9 files.
  • npm --prefix packages/surface run typecheck:regressions — clean, including
    the new run-on-non-zero.test-d.ts.
  • npm --prefix packages/sdk test — 2401 pass. 7 files fail, all confirmed
    failing identically at pristine HEAD
    (verified by stashing and rebuilding):
    live-kernel, webhook-live, mcp, provider-trigger-executor,
    communication-mixed-resume need kernel/target/{debug,release}/relayflowd,
    which ops/cargo.sh deliberately builds outside the repo;
    stuck-run-triage and authored-node-runtime hit a vitest module-duplication
    issue for flow files imported from outside the package root.
  • Schema regeneration is reproducible: regenerating over the tracked file is
    byte-identical.

Four suites needed updating for this change rather than being broken by it:
verb-field-lint (its fail-closed pin demands a sample value for every new
step field), validate and gate-contract (the unknown_gate_kind
enumeration), and preflight (its converse test requires every declared
refusal kind to be reachable through the public boundary — the three
steps_green kinds now have scenarios).

Not in scope

Retry policy (maxIterations) and agent-step failure semantics, per the ticket.


Note

Medium Risk
Touches kernel verification, spec hashing, and authored step outcome reading—core flow control paths—with broad SDK/surface API surface, though defaults and canonical hashing are preserved and coverage is heavy.

Overview
Adds repair-before-failure for deterministic steps: onNonZero: 'record' (YAML on_non_zero) lets a command finish with a nonzero exit while journaling code and output tails, so dependents can branch on real evidence instead of || true. The kernel gains an OnNonZero policy (verify.rs labels satisfied reds exit_code:recorded); timeouts, signals, -1, and other gates stay fatal.

f.run gains overloads: default still returns stdout string; record returns RunResult (ok, exitCode, streams). The SDK/compiler/schema expose the field with fail omitted from canonical bytes for hash stability.

Adds declarative steps_green, compiler-lowered to a separate fatal gate step that reads journaled envelopes (not re-run). flows check annotates recording steps; docs in SURFACE.md describe the pattern.

Also fixes authored .gate() child runs: readCompletedStepOutput now fails when the run ends non-success, so a failed gate step is not treated as a passing producer.

Reviewed by Cursor Bugbot for commit d036684. Bugbot is set up for automated code reviews on this repo. Configure here.


Summary by cubic

Deterministic steps previously failed the flow on any nonzero exit, forcing repair flows to hide failures with || true. They can now use onNonZero: 'record' to continue with the real exit code and output journaled, while steps_green provides an explicit later gate for final success.

New Features

  • Adds onNonZero to specs and f.run; recorded runs return ok, exitCode, stdout, and stderr read back from the journal.
  • Adds steps_green, which checks journaled exit codes without rerunning commands.
  • Keeps fail as the default, rejects invalid policies, and preserves existing canonical hashes.

Bug Fixes

  • Authored .gate() now reports failures from its generated gate step instead of resolving the producer as successful.
  • Restores f.run step registration in the authored step DAG so status --json reports after instead of undefined.
  • Timeouts, signals, content and schema gates, and budgets remain fatal under either exit policy.

Written for commit 7c018b4. Summary will update on new commits.

Review in cubic

Fixes #509

Relayflow and others added 2 commits September 20, 2026 17:47
… not hidden

v2 gated every deterministic step on exit code with no opt-out, so every
repair-before-failure flow re-invented `<command> || true`. That workaround
discards the exit code: a later gate has nothing to read, and a forgotten
gate is indistinguishable from success, so a flow can ship red work silently.

A deterministic step can now declare `onNonZero: record`. The kernel keeps
the command's real exit code and output tails in the journal, passes the step
so dependents run, and labels the gate `exit_code:recorded` so no reader can
mistake it for a green command. `f.run(cmd, { onNonZero: 'record' })` resolves
to a `RunResult` read back from that journaled envelope, so the branch below it
reads the code and output the command actually produced. A later step asserts
freshness with `{ type: steps_green, ids: [...] }` rather than re-running.

The policy covers exit codes only. Timeouts, signals, the `-1` no-exit-status
sentinel, declared content and schema gates, and budgets all stay fatal under
either policy, and the default is normalized out of the canonical bytes so
every existing spec keeps its exact hash.

Also fixes a pre-existing hole this feature would have widened: an authored
`.gate()` lowers to its own kernel step that runs after the producer, and
`readCompletedStepOutput` read only the producer's entry — so a failed gate
resolved the operation as though the command had passed. It now checks the
run's verdict too, which is the same invisibility the recording policy exists
to remove, one layer up.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 20, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 8e560727-7903-4aba-8a16-bf6997bb7280

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread packages/sdk/src/steps-green.ts
…are-garden-b99b1be8

# Conflicts:
#	kernel/relayflowd-core/src/spec/tests.rs
#	packages/sdk/src/authored-flow-executor.ts
#	packages/sdk/src/authored-step-output.ts
#	packages/sdk/src/authored-worker-step.ts

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d036684. Configure here.

Comment thread packages/sdk/src/authored-flow-executor.ts
The merge hoisted runOperation for onNonZero but dropped the node
argument, so f.run steps never entered the step graph and status --json
reported after: undefined. Restore the {} node (a command is not a
display label) on both the record and fail paths, matching main.
…Options)

Picks up the #547 authored-verdict recovery fix on main. Verified:
surface+sdk typecheck clean; advisory-outcome 23, authored-step-graph-live,
authored-advisory-run-live 16, authored-completion-recovery 5 green.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

No advisory deterministic step: v2 gates every deterministic step on exit code, so repair-before-failure flows all re-invent '|| true'

2 participants