Software factory change - #520
agent-relay-code[bot] wants to merge 2 commits into
Conversation
A lowered gate had five bare `process.exit(1)` sites, and word_count_bounds four more, each producing exit 1 with empty stdout_tail and stderr_tail for reasons unrelated to the author's command: a from_output/in_output_at path the producer output does not have, a non-text selection, a NUL byte, a child that never started, a child killed by a signal. A binding that drifted was indistinguishable from a gate that genuinely failed. Route every non-verdict exit through a bounded `fail()` that writes one 400-character line to fd 2 via writeSync — process.stderr.write is async on a pipe and the adjacent process.exit would drop the diagnostic. Nothing goes to fd 1, because references_input and word_count_bounds read their verdict from stdout. Spawn failures carry the error code and the input's byte count, so an argument/environment-size failure is diagnosable without printing the payload. word_count_bounds stops discarding its wc stderr. Keep `stdio: 'inherit'` rather than the buffering the ticket proposed. The kernel pipes and drains a deterministic step's stdout/stderr (exec_det.rs:78), so the command's bytes already reach the journal as written; buffering them for a post-wait flush would lose exactly what a process-group SIGKILL on timeout makes most valuable. Live-daemon tests confirm both tails were already journaled before this change, and now pin it — including partial output surviving a timeout. `flows logs <run-id>` is left explicitly unmet: it reads Cloud's runner log and no producer for that route exists in this repository. See summary.md. Co-Authored-By: Claude <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
The review rejected the report, not the code: it labeled the change mutation-verified while supplying only aggregate counts, elided the daemon path to `...`, and dismissed a red package run as pre-existing without the baseline invocation or its output. Re-execute everything and commit the transcripts under evidence/511-named-gate-diagnostics/, each one the captured output of the command that heads it, ending in its exit status. The mutation halves now record blob hashes against `git rev-parse` at both revisions and an empty `git status`, so "byte-for-byte" is checkable. The pre-existing claim now rests on a baseline run of the same three files with packages/sdk reverted to e21caad in the same working directory: 30 failed, 23 passed, 14 skipped at both revisions, same cases and line numbers. 22 of those failures are two reachable installs of @relayflows/surface, one in an ancestor of the checkout, missing the WeakMap brand check — literal resolver output included. State the `flows logs <run-id>` gap as a named work item: the Cloud runner must publish a deterministic step's tails into the logs route, no producer for it exists here, and #511 is not fully closed until that end-to-end evidence exists. Correct the ticket number. The work implements #511; the first commit, the PR title and the PR body cite #507, which is the f.gitlab writeback issue. The pushed commit is not amended, and the PR body still needs the same fix. Co-Authored-By: Claude <noreply@anthropic.com>
|
Relayflow: the adversarial review did not pass. This branch is not approved: the flow stopped here and did not mark it ready to merge. Review of PR #520, head Disposition: no production-code defect identified in this diff; ticket acceptance remains incomplete.
The previous evidence finding is resolved in the committed report: The implementation follows the reviewed plan. Inherited child descriptors are the deterministic executor's capture pipes ( All available PR comments and reviews were fetched, including paginated issue comments, inline comments, and submitted reviews. The only discussion comment is CodeRabbit's explicit skipped-review notice; inline comments and submitted reviews are empty. The body also contains automated summaries. Exact commands and full responses are captured in pr.txt. Supplementary edge checks used edges.mjs. They exercise generated-command capture, not journal persistence. Literal command and captured output (transcript): The affected package was run at this head. Full command and all captured output, including the failing assertions and stacks: npm-test.txt. cd packages/sdk
RELAYFLOWD_BIN=/home/daytona/.relayflows-toolchain/target/2962130851/debug/relayflowd npm testLiteral result excerpts: Kernel build, SDK typecheck/build, and test typecheck completed before Vitest; their output is in the same transcript. The package is not green. Failures are in
Only |
A failing named gate now says why (#507)
What the ticket reported, and what the evidence actually shows
The ticket proposed replacing
stdio: 'inherit'innamed-gate-lowering.tswith
spawnSyncbuffering, on the theory that the daemon captures the gatecommand's stdio nowhere.
That is not what the code does, and the replacement would make things
worse.
kernel/relayflowd/src/exec_det.rs:78createsStdio::piped()forthe deterministic step's stdout and stderr and drains both on reader threads.
The lowered gate is a deterministic step, so
inherithands the author'scommand those very pipes. Its bytes reach the journal as they are written.
This is not an argument from reading alone. The new
tests/named-gate-journal.test.tsstarts a realrelayflowd, runs a loweredsubprocess_gate, and readsstep.completed.payload.outputback for thegenerated
produce.gatestep. Both tails are there, on a failing gate and ona passing one. Those two cases passed against unmodified production code.
The ticket's stated root cause does not reproduce here, and the reported
macOS incident has not been replayed; nothing below claims it has.
Buffering would also have cost something real: the kernel SIGKILLs the whole
process group on timeout, so output held for a post-wait flush is destroyed
exactly when it is the only account of the failure.
stdio: 'inherit'is keptand now has a regression test that pins it — a gate printing a marker and then
sleeping past a 750 ms timeout still journals the marker.
The defect that is real: exits that say nothing
The gate program had five bare
process.exit(1)sites, andword_count_boundsfour more. Each produced precisely the shape the ticket describes —
exit_code: 1, emptystdout_tail, emptystderr_tail— but for reasons that havenothing to do with the author's command:
from_output/in_output_atselected a path the producer output does nothave (the command never ran at all);
spawnSyncreturned anerrorand never started the child (this is wherean
E2BIGwould land);statusisnull.A
references_inputgate whose binding has drifted, and asubprocess_gatewhose producer changed shape, both report as "the gate failed" with nothing
attached. That is the undiagnosable failure, and it is now fixed.
The change
packages/sdk/src/named-gate-lowering.tsonly.A
fail(message)helper in the shared preamble writes one bounded line tofd 2 and exits 1. Every non-verdict exit routes through it. Specifics:
writeSync(2, ...), notprocess.stderr.write. On a pipe the latter isasynchronous and the
process.exiton the same line would drop thediagnostic — losing the message precisely when it is the only evidence.
references_inputverifies viaoutput_containson stdout andword_count_boundsvia an anchored decimalpattern, so one stray byte on fd 1 would change a verdict. Three tests assert
stdout stays empty (or stays the bare count) when a diagnostic fires.
\r\ncollapsed to spaces,so an author-controlled label cannot forge extra log lines.
carry the error code and the input's byte count, which is what makes an
argument/environment-size failure diagnosable without printing the payload.
word_count_boundsadditionally stops discarding itswcstderr:error,signal, nonzero status and malformed output each get their own cause with abounded 200-byte suffix of what
wcsaid. Previously a missing or brokenwcwas indistinguishable from a word count out of bounds.
Deliberately unchanged, per the reviewed plan:
stdio: 'inherit', theFLOWS_INPUTtransport, exit normalization, verification predicates, thejournal schema, retry policy, and the NUL guard's two-layer escaping. Ordinary
predicate mismatches in
references_input/regex_match/artifact_existskeep their bare
exit 1— a predicate that simply did not match is not acapture failure and needs no new output.
Tests
tests/named-gate-diagnostics.test.ts(17 cases) runs the actual commandthe compiler emits — obtained through
compileSpec+lowerNamedGates, not acopied helper — under piped stdio. It covers stream capture on pass and fail,
each selection failure with a
THE-COMMAND-RANsentinel asserted absent,NUL rejection (with a literal backslash-zero case proving legitimate input
still runs), SIGKILL, stdout non-contamination for every affected gate, and the
wcfailure modes via aPATH-ordered shim.The spawn-error case deserves a note. An oversized outer
FLOWS_INPUTwouldstop the test's own gate process from starting and prove nothing about the
inner spawn; a nonexistent command starts
/bin/shfine and exits 127. So thetest puts a
nodeshim first onPATHthat execs the real interpreter with--require, and the preload hooksModule._loadto return anode:child_processwhosespawnSyncreportsE2BIGwith a null status. Thereal serialized program runs against a stubbed syscall. No production injection
hook was added.
tests/named-gate-journal.test.ts(5 cases) is the acceptance evidence: alive daemon, assertions on the persisted
step.completedoutput of thegenerated gate step. It covers a failing gate, a passing gate, a selection
diagnostic, partial tails surviving a timeout, and a gate on an agent step
— the incident's shape, where the envelope is selected whole rather than as
stdout_tail. That case also asserts the failing gate's stderr reaches theauthored failure report's message.
Acceptance
named-gate-journal.test.ts, live daemon. Already true before this change; now pinned.output.word_count_boundswas the other child-spawning gate and was blind to its own child's failure; fixed.artifact_exists,regex_match,references_inputspawn nothing and need no change.flows logs <run-id>shows the failureWhy
flows logsis left openflows logsis Cloud-only.packages/sdk/src/cloud-read.ts:504issuesGET /api/v1/workflows/runs/<id>/logs; there is no local-journal fallback, andthis repository contains no producer for that log — no write side of the
route exists here.
docs/CLOUD.md:449-452states it directly: "what theseroutes serve is Cloud's own record of the run, not the kernel journal."
A test could mock a log body containing these markers, but that would prove
only that the renderer prints what it is given — it could not show the gate's
stderr was ever published. The missing integration is the Cloud runner /
harness that writes runner logs, which lives outside this repository. I have
not substituted
flows replay(local-journal only) or added a new local logscommand; both would be scope the ticket did not ask for. This criterion should
be tracked against the Cloud repository.
Validation
Run from
packages/sdk. The kernel target lives outside the tree(
ops/cargo.shredirectsCARGO_TARGET_DIR), andtest:prepexportsRELAYFLOWD_BINinside a subshell that does not reach vitest, so it is passedexplicitly here.
Mutation verification — production file reverted with
git stash push packages/sdk/src/named-gate-lowering.ts, rebuilt, both new files run:The 9 that pass are the honest baseline: stream capture already worked.
Restored byte-for-byte with
git stash pop, rebuilt, re-run:Required package command —
RELAYFLOWD_BIN=... npm test(kernel build,typecheck, build, test typecheck, vitest):
The 30 failures are pre-existing and unrelated. Verified by running the
same files against the reverted production code: identical counts, identical
line numbers.
tests/authored-node-runtime.test.ts— suite-level failure at line 18,expected '1.3.6' to be '1.4.0': this sandbox's bun is older than theversion the test pins. All 14 cases skip. This is the file that exercises
word_count_boundsend-to-end, so it was checked first and specifically; itdoes not run here for want of a prerequisite, not because of this change.
tests/live-kernel.test.ts— 8 cases needing agent CLIs unavailable here.tests/stuck-run-triage.test.ts— 22 cases failing ingetFlowDefinitionwith "expected an @relayflows/surface flow handle", asurface module-resolution problem in this checkout.
No kernel code changed, so the Rust suite was not run.
tsconfig.tests.jsongains the two new files sotypecheck:testscovers them;it passes clean. No file under
docs/evidenceand no generated file wasedited.
Note
Medium Risk
Changes generated gate runtime behavior for failure paths operators rely on for triage; scope is limited to SDK lowering and stderr-only diagnostics, with strong new tests, but any gate verification edge case could affect run outcomes.
Overview
Fixes #511 by making lowered named gates emit a single bounded stderr line when they fail before or instead of a normal command verdict—selection misses, bad input shape, NUL bytes, spawn errors, signals, and broken
wc—instead of exiting1with empty journal tails.named-gate-lowering.tsadds a sharedfail()that usesfs.writeSync(2, …)(so the message is not dropped on immediateprocess.exit) and keeps diagnostics off stdout, sincereferences_inputandword_count_boundsread verdicts there.stdio: 'inherit'forsubprocess_gateis unchanged; new tests pin that stream capture and journal persistence vianamed-gate-diagnostics.test.tsand live-daemonnamed-gate-journal.test.ts.The PR also adds
evidence/511-named-gate-diagnostics/andsummary.mdwith mutation checks and test transcripts.flows logsshowing gate stderr in Cloud is explicitly not closed here (no log producer in-repo).Reviewed by Cursor Bugbot for commit 200e7c2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes #511: named gates that failed before running their command used to exit 1 with empty stdout and stderr tails, so a drifted binding and a dead child were indistinguishable from a real gate failure. Every non-verdict exit now writes one bounded line to stderr with the cause, and
stdio: 'inherit'is kept because the kernel already journals the command's output as written — buffering would lose it on timeout.Bug Fixes
word_count_boundsreports whywcfailed (missing or brokenwc, signal, malformed output) instead of looking like an out-of-bounds count.references_inputandword_count_boundsverify against stays clean.evidence/511-named-gate-diagnostics/so the mutation and baseline runs are checkable.flows logs <run-id>is intentionally not closed; it reads Cloud's runner log, and no producer for that route exists in this repository.Written for commit 200e7c2. Summary will update on new commits.