Repository navigation
flows check never talks to the daemon, so it structurally cannot catch spec/daemon skew - #619
Conversation
The acceptance case for #489's class is that a field the installed relayflowd rejects is refused by `flows check`, naming the same field and step. The stubbed-daemon cases pin the diagnostic shaping; the live case only pinned acceptance of the canonical ladder. This adds the negative half against the built binary: the field is injected into the submitted JSON (no such skew exists at this commit, since both halves ship from it), the real validator refuses it, and the report must name `steps[1]`, map it to the authored step `edit`, and not blame the compiler for an authored step. Mutation-verified: evidence/daemon-check/live-skew-{mutated,restored}.txt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
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 |
|
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 #619Reviewed head: Verdict: changes requested. One P2 finding remains; P2 — Require successful subprocess completion before claiming daemon acceptanceLocation:
This is an abnormal-process edge case, not a claim that the current Rust validator routinely emits inconsistent output. The Rust command currently pairs accepting output with exit 0; the SDK should enforce that contract at the subprocess boundary. Require ReproductionFrom the repository root, after building the SDK, run the following literal command. It uses temporary validator executables and a package-valid deterministic spec; it creates no run or journal. python3 - <<'PY'
import json, os, pathlib, subprocess, tempfile
root = pathlib.Path.cwd()
with tempfile.TemporaryDirectory(prefix='pr619-status-') as directory:
d = pathlib.Path(directory)
flow = d / 'flow.json'
flow.write_text(json.dumps({'version':'0.1.0','steps':[{'id':'hello','type':'deterministic','command':'echo hello'}]}))
for name, ending in [('exit2', 'exit 2'), ('sigterm', 'kill -TERM $$')]:
validator = d / name
validator.write_text('#!/bin/sh\ncat >/dev/null\nprintf \'%s\\n\' \'{"ok":true,"protocol":0,"spec_version":"0.1.0"}\'\n' + ending + '\n')
validator.chmod(0o755)
direct = subprocess.run([str(validator), 'validate-spec'], input='{}', text=True, capture_output=True)
env = dict(os.environ, RELAYFLOWD_BIN=str(validator))
checked = subprocess.run(['node',str(root/'packages/sdk/dist/cli.js'),'check','--against-daemon','--json',str(flow)], env=env, text=True, capture_output=True)
report = json.loads(checked.stdout)
print(json.dumps({'case': name, 'validator_exit': direct.returncode, 'validator_stdout': direct.stdout.strip(), 'check_exit': checked.returncode, 'check_ok': report['ok'], 'validation': report['validation'], 'diagnostics': report['diagnostics']}))
PYCaptured output: Scope and PR discussionReviewed the implementation and regression-test diff against Read the PR conversation and queried both review endpoints with pagination. The only conversation comment was CodeRabbit's “Review skipped” notice; it is not review approval. There were no inline comments or submitted reviews: gh api --paginate repos/agentworkforce/flows/pulls/619/comments
gh api --paginate repos/agentworkforce/flows/pulls/619/reviewsCaptured output, respectively: Verification performed in this reviewThe following are focused checks, not a claim that the full SDK or Rust workspace suite passed in this review. The PR describes historical full-suite failures and subsequent environment adjustments; this review does not independently certify those broader claims. No mutation verification was performed. cd packages/sdk && npm run build && npx vitest run tests/check-daemon-validation.test.ts tests/cli.test.ts tests/cli-watch.test.ts tests/relay-cli-surface.test.tsCaptured output (exit 0): RELAYFLOWD_BIN="$PWD/kernel/target/release/relayflowd" packages/sdk/node_modules/.bin/vitest run --root packages/sdk tests/live-kernel.test.ts -t 'validates the canonical ladder|refuses a field the built daemon'Captured output (exit 0): /home/daytona/.cargo/bin/cargo test --manifest-path kernel/Cargo.toml --release -p relayflowd --test validate_specCaptured output (exit 0): |
…are-garden-774f819f Session-Id: 0e078a4c-f89d-4e74-9919-00e1e9a08879
…ptance spawnSync reports no error for a nonzero exit or a signal, so a validator that printed an accepting envelope and then failed or was killed was reported as validation.mode: daemon. That now yields abnormal_exit (refusal in required mode, daemon_unvalidated warning in auto mode). Daemon refusals are still reported whatever the exit status. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Session-Id: 0e078a4c-f89d-4e74-9919-00e1e9a08879
|
Addressed the adversarial review's P2 in a0b66c6, after merging
Regressions in
Before/after: with the old |
|
bugbot run |
|
@coderabbitai review |
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a0b66c6. Configure here.
…are-garden-774f819f # Conflicts: # packages/sdk/src/cli.ts Session-Id: 0e078a4c-f89d-4e74-9919-00e1e9a08879
|
Merged |
Combine both flows check options in cli.ts, cli-commands.ts, cli-watch.ts (the watch child forwards the daemon mode and --explain-warnings), and the CLI surface samples. #621's JSON byte-identity test now builds its expected report in the CLI's daemon validation mode, since check --json carries the validation field. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Session-Id: 0e078a4c-f89d-4e74-9919-00e1e9a08879
|
Merged |
1 participant
flows checkcould accept a declarative spec that the installed runtime wouldrefuse. It now submits the compiled kernel spec to
relayflowd validate-specwhen available, preserving the daemon's field and step diagnostic. The validator
creates no run or journal.
--against-daemonrequires a verdict;--no-daemon-checkkeeps local checking available. Both have environmentcounterparts, and watch mode forwards the selection.
Pass output names the validating binary or explains why acceptance is unproven;
JSON reports include the same validation status on pass and refusal. Generated
gate indices are mapped against the submitted spec and identified as compiler
lowering issues. SDK callers, run, build and deploy retain local preflight unless
explicitly opting into validation.
Scope follows reviewed-plan.md: this covers declarative YAML/JSON specs. Authored
TypeScript bodies still lower only at execution, so the authored-body case
reported in #489 is not covered. Required mode refuses to claim validation for
those bodies. Older runtimes including 2.0.19 lack this command: auto mode warns,
required mode refuses. The verdict is from the resolved binary, not a different
build already serving a socket. Release-artifact
--against-daemonenforcementand authored-body capability probing remain follow-ons; no workflow files changed.
Verification is not fully green. The complete SDK run below recorded 45 test
failures plus one unhandled error. Its new-flag coverage failure was fixed by
adding both invocations to the existing coverage table, without changing the
assertion; that file's rerun is included. Plugin-extension tests hit ENOSPC while
the first Rust workspace build filled the disk; they passed after task-created
build caches were removed. Remaining failures include bwrap network-namespace
restrictions, Bun 1.3.6 versus expected 1.4.0, live analyzer/worker-output
expectations, and event-await exit-status expectations. No claim is made that
these remaining failures were reproduced on the base commit. The full SDK suite
was not repeated after the invocation-table repair; the minimal-PATH rerun covers
the affected check/CLI suites, not the entire SDK.
The Rust workspace was retried without debug symbols/incremental artifacts after
a disk-full linker crash. The normal ops wrapper initially found no default
private rustup toolchain. SDK test preparation used
/tmp/daemon-check-toolchainwith
cargoandrustupsymlinked to the installed/home/daytona/.cargoand/home/daytona/.rustup. All build artifacts remained outside the repository.Literal commands and captured output follow. Full captures, including initial
failures, are in evidence/daemon-check.
sdk-final.txt
Full captured output.
sdk-targeted-final.txt
Full captured output.
cli-surface-final.txt
Full captured output.
live-ladder.txt
Full captured output.
rust-targeted-final.txt
CARGO_TARGET_DIR=/tmp/relayflows-daemon-check-target CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0 /home/daytona/.cargo/bin/cargo test --manifest-path kernel/Cargo.toml -p relayflowd --test validate_specFull captured output.
rust-workspace-retry.txt
CARGO_TARGET_DIR=/tmp/relayflows-daemon-check-target CARGO_PROFILE_DEV_DEBUG=0 CARGO_PROFILE_TEST_DEBUG=0 CARGO_INCREMENTAL=0 /home/daytona/.cargo/bin/cargo test --manifest-path kernel/Cargo.toml -j 2Full captured output.
typecheck-final.txt
Full captured output.
sdk-minimal-path.txt
Full captured output.
package-gate.txt
Full captured output.
standalone-build.txt
Full captured output.
standalone-check.txt
Full captured output.
sdk-full.txt
Full captured output.
plugin-extension-retry.txt
Full captured output.
Mutation verification used an isolated copy of the current SDK source and build,
so the concurrently running full suite was never exposed to mutated code. The
reproducible mutation script disables the
refusal branch, tests unit and real CLI skew, then restores both files byte for
byte. It separately changes submitted-step indexing back to authored-step
indexing, tests generated and subsequent authored steps, and restores again.
All eight complete captures follow; hashes are in the last capture.
mutation-refusal-unit-mutated.txt
mutation-refusal-cli-mutated.txt
mutation-refusal-unit-restored.txt
mutation-refusal-cli-restored.txt
mutation-indices-generated-mutated.txt
mutation-indices-authored-mutated.txt
mutation-indices-generated-restored.txt
mutation-indices-authored-restored.txt
mutations.txt
Checks
The checks fail on the base commit too, so these failures were not introduced by this change: they come from the repository itself or from the environment the checks ran in. This pull request is a draft until someone looks.
What ran (.relayflow/check.sh)
Output on this branch (last 80 lines)
Output on the base commit (last 80 lines)
What the repair agent found
Check failures on relayflow/flows-software-garden-774f819f
.relayflow/check.shfailed in the SDK vitest step: 3 files, 15 tests, and asecond run surfaced a fourth cause behind them. None of it came from this
branch's change; every cause is a property of this machine, and three of the
four are now handled inside
.relayflow/check.sh(uncommitted, as instructed).After those four fixes the script passes end to end:
(
sh .relayflow/check.sh, captured in full at /tmp/check-rerun2.log; theworking tree is clean afterwards —
git status --porcelainprints nothing.)What follows is what each cause is and how it was established.
1.
tests/authored-node-runtime.test.ts— bun version, fixed in check.shThe file's
beforeAllasserts the exact bun CI pins:Every workflow
check.shmirrors installs bun throughoven-sh/setup-bun@v2withbun-version: 1.4.0(
.github/workflows/cloud-runtime-artifact.yml:56,surface-package.yml:38,schema-publish.yml:41); this machine ships 1.3.6 andcheck.shonly assertedthat some bun was on PATH. It now installs the pinned bun next to the way the
existing block installs a missing cargo, and puts it first on PATH. Verified:
2.
tests/live-kernel.test.ts— an ancestorpackage.json, fixed in check.shSeven tests failed with a null step output, e.g.
The journalled detail names the cause:
Every one of the seven drives an agent step through an extensionless fixture in
testdata/preflight(analyze-story-*-cli,echo-model-cli,tick-slot-report-cli). Those fixtures are#!/usr/bin/env nodescripts with atop-level
import, and Node decides an extensionless file's module type fromthe nearest
package.json. The repository has none, so on a CI runner thesearch finds nothing and the file loads as ESM. Here the search leaves the
checkout and finds
/home/daytona/package.json, which declares"type": "commonjs"— the fixture then produces no output at all and exits 0,so the handshake never happens.
Controlled repro, same fixture, only the ancestor manifest differing:
(
fixture-probe/with-cjs-ancestor/package.jsonis{"type":"commonjs"}; theother tree has no ancestor manifest. The fixture and
wrapper-session.mjsarebyte-identical copies of the repository's.)
And end to end, by restoring CI's module resolution with one fixture-local
manifest:
versus the same command without that file:
Tests 7 failed | 25 passed | 1 skipped.check.shnow writes that manifest before the suite and removes it on exit(
trap … EXIT HUP INT TERM), so the working tree is never left dirty. It isdeliberately not committed: in CI the file would be a no-op, and the repo's
fixtures are not what is wrong here — the home directory of this machine is.
A maintainer who wants the fixtures hermetic against any ancestor manifest can
commit that one line; this branch is not the place for it.
3.
tests/hosted-extension-protocol.test.ts— bubblewrap, not fixable hereEight of its 27 tests exec the hosted-extension sandbox and fail with the error
check.sh's own header already documents for three other files:Confirmed to be the environment, not the tree:
The workflow provisions this on an ephemeral VM it owns
(
sysctl -w kernel.apparmor_restrict_unprivileged_userns=0); heresysctl -wis denied. This file is a fourth instance of an already-documented limit, so it
joins the three
--excludeentries, which costs the 19 tests in it that do nottouch the sandbox — vitest cannot skip part of a file. Those 19 and the 8
sandbox tests are unverified on this machine and are verified by CI.
4. Disk exhaustion in the SDK suite — handled in check.sh
With bun pinned,
tests/authored-node-runtime.test.tsran for the first time —and six of its cases plus one
authored-root.test.tscase then failed on space,not logic:
The disk is 10G.
cargo test --workspaceleaves a 3.8Gkernel/target/debugtree, and each authored-node-runtime case copies a dereferenced
@relayflows/surfaceinstall into /tmp. Nothing after the kernel step reads thedebug artifacts — the suite runs against the release binary through
RELAYFLOWD_BIN— socheck.shnow removes that tree right after the kerneltests and prints
df -h ., which took the suite from 2.7G of headroom to 6.7G:The next run rebuilds the debug artifacts; that is the whole cost. CI's runner
has the headroom and needs none of this.
Not an environment issue
Nothing above touches
relayflowd validate-spec,flows check --against-daemon, or any file this branch changed. The branch's own gates passhere:
cargo test --release -p relayflowd --test validate_spec(3 passed),vitest run tests/check-daemon-validation.test.ts(20 passed), and the twolive-daemon cases in
tests/live-kernel.test.ts— all of them inside the greenrun above as well.
Summary of the (uncommitted) check.sh changes
testdata/preflight/package.json({"type":"module"}) for theduration of the run and remove it on exit, restoring CI's module resolution
for the extensionless agent-CLI fixtures.
rm -rf kernel/target/debugafter the kernel tests, for disk headroom.tests/hosted-extension-protocol.test.ts, the fourthbubblewrap-executing SDK test file, beside the three already excluded.
Fixes #502
Summary by cubic
Makes
flows checkcatch spec/daemon skew by submitting the compiled kernel spec to the newrelayflowd validate-specsubcommand when a supported binary is available, and refusing specs the installed runtime rejects using the daemon's own field and step diagnostics. Validation is stateless: no run or journal is created, and the data dir is left untouched. The branch also merges main including #621--explain-warnings, so watch mode now forwards both--against-daemon/--no-daemon-checkand--explain-warnings, andcheck --jsonreports the new validation field.--against-daemonrequires a verdict;--no-daemon-checkkeeps local checking; both have environment counterparts.--jsonreports the same validation status.abnormal_exit, refusing in required mode and warning in auto mode, while refusals are honored regardless of exit status.validate-specwarn in auto mode and refuse in required mode.Written for commit 02230c5. Summary will update on new commits.
Note
Medium Risk
Changes the
flows checkcontract and spawnsrelayflowdfor validation, so CLI/SDK skew handling and binary discovery affect whether specs pass locally; runtime execution paths are unchanged unless users opt into daemon validation.Overview
flows checkcan now prove declarative YAML/JSON specs against the installed runtime, not just local compile/schema work. After lowering, the CLI pipes the same canonical kernel specrun.startwould submit to a new statelessrelayflowd validate-specpath (parse + validate, no socket, run, or journal).Modes and UX: default auto validates when the resolved binary supports it;
--against-daemon/FLOWS_CHECK_AGAINST_DAEMON=1require a verdict (exit 2 ondaemon_validation_unavailable);--no-daemon-check/FLOWS_NO_DAEMON_CHECK=1skip binary discovery. Watch mode forwards the choice. HumanCHECK PASSEDlines and--jsonreports gain avalidationobject (daemonvslocalplus binary/protocol/reason). Daemon refusals surface asinvalid_specwith step/field mapping; indices on generated gate steps are mapped to submitted steps and labeled as compiler lowering bugs when appropriate.Scope limits: authored
.flow.tsbodies still only get local checks (authored_body); older binaries withoutvalidate-specwarn in auto and refuse in required mode. Acceptance requires an OK envelope and exit 0.Also adds
validate_specmodule export on the daemon, SDK tests (check-daemon-validation, CLI surface flags),FLOWS_NO_DAEMON_CHECK=1in vitest so the default suite stays spawn-free, docs in SURFACE.md / EDITOR.md, andevidence/daemon-checkcapture artifacts.Reviewed by Cursor Bugbot for commit 02230c5. Bugbot is set up for automated code reviews on this repo. Configure here.