flows check knows which specs will park or fail at run time, and says nothing: spend the analysis it already does - #517
Conversation
`flows check` computed the facts that would have prevented five dead runs and said nothing about them. Two of those facts now become diagnostics, and the third turns out not to be a fact. `agent_worker_unresolved` (permanent, check-only): a spec with `agent` steps needs a worker attached for step type `agent`, and without one the run parks at the first such step. `check` already walks those steps to print `REQUIRES`, so it now says what has to be true for a worker to be attached, naming the steps and `--local-agent`. It warns rather than refuses because attachment is unknown to a daemon-free check, not absent; it is a property of the invocation, so it opts in through a new `CheckInvocation` seam and `preflight` stays a pure function of the spec. Scoped to `agent` steps — `--local-agent` attaches no `llm` worker — and counted from the compiled steps, so a YAML helper step is included where `requirements` omits it. Emitted once, in the `REQUIRES` position, because it is a footnote to that line. `gate_path_unreachable` (temporary, retires with #513): an `artifact_exists` gate reads the journaled artifact list and nothing else, so a path inside a prefix the bundled worker's scan skips can never match. That is unsatisfiable, not merely unproven, so preflight refuses it and names the excluded prefix. The exclusion rule moves into `artifact-scan-policy.ts` and the real scan consumes it, so the refusal cannot drift from the scan it describes; a parity test pins the two against a real filesystem walk. Collected before every early return, so a dead gate is not hidden behind an unresolved CLI for a pass or two. `gate_output_not_captured` is deliberately not added. The criterion was to warn for as long as a subprocess_gate's output is lost, and against a live daemon it is not: the kernel journals both tails on the lowered gate step and the CLI renders them. A live-kernel regression pins that capture instead, so the warning becomes warranted at the moment it becomes true. Fixes neither #511 nor #513, and does not round-trip the spec past the daemon's validator (#502).
|
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 refusal assumed the bundled worker's artifact scan is the only writer of `output.artifacts`. It is not: the same worker journals object-shaped JSON stdout and completed Relay task output verbatim, so an agent can report an excluded path itself and satisfy the gate. Refusing rejected those specs at `check`, `run`, `build` and every SDK submission. `gate_path_unreachable` becomes the warning `gate_path_unscanned` — the old name asserted the falsehood — and the message reports the scan's limitation and the routes that bypass it instead of declaring the gate impossible. A regression drives the real `AgentWorker` over an excluded path the fake CLI both writes and reports as JSON, then runs the real lowered gate command over exactly what the worker journaled: the scan records nothing, the gate exits 0. 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: PR #517 at 2732505No remaining production-code correctness finding identified. Prior finding F1 is addressed. Verification remains incomplete because the full SDK suite is not green; Reviewed head: Prior F1: resolvedThe original hard refusal incorrectly treated the artifact scan as the only writer of This intentionally differs from the issue's requested refusal: the excluded prefix does not prove the gate unsatisfiable. Refusing all such gates would restore F1. The scanner's exclusion rule is shared without changing scanning behavior; exact segments, nested exclusions, dotfiles, and lookalikes are covered. Static warnings are collected before environmental early returns. The permanent The subprocess output warning remains conditional on a bug that the new live test does not reproduce. That test checks rendered evidence and journal payloads, with a silent control. This establishes the behavior of this checkout's tested daemon, not the daemon from the original campaign. PR commentsCaptured and reviewed using these literal commands from the repository root: gh pr view 517 --json number,url,title,baseRefName,headRefName,headRefOid,body,comments,reviews > review-artifacts/pr517-head2732505/pr-comments.json
gh api --paginate repos/AgentWorkforce/flows/pulls/517/comments > review-artifacts/pr517-head2732505/inline-comments.json
gh api --paginate repos/AgentWorkforce/flows/pulls/517/reviews > review-artifacts/pr517-head2732505/reviews.jsonFull literal responses: PR body/comments, inline comments, reviews. Both review endpoints returned VerificationCommands below ran from the repository root on the reviewed head. Captured files contain full literal output. No production code, tests, generated files, or docs/evidence were edited by this review. No mutation verification or baseline rerun is claimed. npm --prefix packages/sdk test > review-artifacts/pr517-head2732505/sdk-test.log 2>&1
npm --prefix packages/schema test > review-artifacts/pr517-head2732505/schema-test.log 2>&1Relevant literal SDK output: Schema exited 0. Final literal output: SDK exited 1. Final literal output: The full SDK suite is not green. Failures include unavailable runtime binaries, analyzer/worker-output assertions, and flow-handle consumers. One literal error is: I did not rerun the base commit, so I cannot endorse the PR summary’s claim that all failures are pre-existing/environmental. Resolve these failures or capture an equivalent base-versus-head comparison before treating verification as complete. The changed regression tests passing does not establish that the full suite passes. No separate Rust suite was run; no Rust source changed, and the SDK script built and exercised the daemon identified above. Additional reproduction, using the retained prior-review source: node review-artifacts/pr517/repro.mjs > review-artifacts/pr517-head2732505/repro.log 2>&1Literal output (log): This fixture lacks the model-readiness probe response, so its preflight still refuses with
|
Main's scan now journals dot-directories (.workflow-artifacts/ is the conventional artifact dir) and skips only exact names .git, .relayflowd and node_modules. The shared artifact-scan-policy predicate encodes that set, so gate_path_unscanned warns only where the merged scan truly records nothing. cli.ts keeps both the worker-surface deferral and model provenance.
|
Marking ready for review. This PR was drafted by the Software Garden when its adversarial review withheld signoff. The flow drafts on a failed review and never re-evaluates, so the "not approved" verdict above is a permanent record of one moment, not a current statement — three PRs merged today (#512, #521, #545) were in exactly this state with their findings long since fixed. Re-reading this PR's verdict against the current head: it identifies no open production-code defect. CI is green (6 checks, 0 failures). Whoever reviews this should still read the verdict for the caveats it records — they are real, they are simply not code defects. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb64e47eca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| An `artifact_exists` gate reads the journaled `output.artifacts` list and | ||
| nothing else. The bundled worker's *scan* skips any entry whose name starts | ||
| with `.` and any entry named exactly `node_modules`, so a gate on a path inside | ||
| one of those prefixes cannot rest on the scan, however faithfully the agent |
There was a problem hiding this comment.
Correct the stale dot-entry scan description
This report says the scanner skips every dot-prefixed entry and even shows .workflow-artifacts/... producing the warning, but the committed policy skips only the exact names .git, .relayflowd, and node_modules; docs/SURFACE.md also explicitly says .workflow-artifacts is scanned. Leaving this earlier description in the final report gives reviewers a false account of the shipped behavior, so update the text and example to match the exact-name policy.
AGENTS.md reference: AGENTS.md:L102-L103
Useful? React with 👍 / 👎.
| `npm test` in `packages/sdk`: 2381 passed, 40 failed. All 40 failures are | ||
| pre-existing and environmental — they reproduce identically on a stashed tree | ||
| (same 40 failures, same 7 files): `relayflowd` looked up under | ||
| `kernel/target/{debug,release}/` while this toolchain builds outside the repo, | ||
| and flows needing real harness CLIs. `tsc --noEmit` is clean for `src`, the |
There was a problem hiding this comment.
Attach raw evidence to the verification claims
The report claims exact test totals, identical failures on a stashed tree, and clean type checks, but includes neither the literal commands nor their captured output. Those are verification claims under the repository's evidence policy; without the transcripts, reviewers cannot reproduce the comparison or distinguish the stated environmental failures from regressions in this commit.
AGENTS.md reference: AGENTS.md:L90-L92
Useful? React with 👍 / 👎.
…573) main went red when #517 merged: its artifact-gate test declares an absolute `cwd` on the step, and #512's contained-cwd contract refuses that — `spec.steps[0].cwd: expected a run-root-relative path, not an absolute path`. compileSpec throws, preflight reports `invalid_spec`, and the test's expected `gate_path_unscanned` never happens. Each change is correct alone; only the combination fails, and neither branch could see it because CI does not re-run the suite against the merged tree. The test uses cwd only to spawn its fake `claude` in a temp directory, which is exactly what a run root is for. The step now declares no cwd and the worker carries `runRoot`, matching how #512 restated the duration dispatch. The assertions are unchanged, and reverting #517's `gate_path_unscanned` warning still fails this test, so it guards the same behaviour it did before. Co-authored-by: Proactive Runtime Bot <prpm.dev@gmail.com>
…never outlives its lease (#561) (#576) * test(sdk): reproduce repeated authored CLI probes and expired leases * fix(sdk): reuse authored CLI probes for each run before worker admission * docs: capture lease regression mutation and live repro evidence * test(sdk): keep the artifact-gate cwd regression inside the run root The regression added by #517 declares `cwd` on a spec it hands to `preflight`, and used an `os.tmpdir()` directory — an absolute path. #566 landed one commit earlier and made an absolute `cwd` a compile refusal (`agent-cwd.ts`: a declared `cwd` is run-root-relative, the same rule `relayflowd_core::spec::is_run_root_relative_path` applies at the kernel boundary). Each PR was green alone; together they are not, and `main` at e30226c fails this test with `invalid_spec` where it expects `gate_path_unscanned`. The fixture now makes its directory inside the run root and declares the relative name. Nothing else moves: the warning, `ok`, the real `AgentWorker` dispatch, the empty scan snapshot, the journaled JSON output and the lowered gate command are asserted exactly as before. Reverting this file to its e30226c bytes fails the case and restoring it passes; both captures are in evidence/561/artifact-gates-{red,green}.txt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: classify every check failure against main and re-capture #561 evidence The repository check on this branch failed with 30 tests across five files. None came from #561. Each is classified by reverting this branch's three source files to origin/main (e30226c), re-running and restoring: * artifact-gates (1) — main is red; #566 and #517 conflict semantically. Fixed in the preceding commit. * live-kernel (7) — this sandbox's HOME declares "type": "commonjs" above the checkout, so testdata/preflight's extensionless ESM fixture CLIs load as CommonJS and emit nothing, silently. Local setup only. * authored-node-runtime — Bun 1.3.6 where every workflow pins 1.4.0. * hosted-extension (22) — unprivileged user namespaces denied to this container; bwrap cannot run even once installed. The mutation is re-run at this head. The mutated run reproduces the issue's exact signature — lease_expired on a first attempt that never heartbeated, retry, second attempt success — and the restore is byte-identical by SHA-256. The live-Claude repro could not be re-run: this environment's claude is no longer authenticated. The capture says so rather than the acceptance box claiming a pass it cannot show. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: re-run the #561 mutation at the branch head The committed mutation transcripts were captured before the artifact-gate fix (1ccaaed) landed. Re-run `evidence/561/mutation.patch` against the current head so the transcript matches the bytes a reviewer checks out, and record the restore with `git diff --exit-code` plus a sha256sum. The failing capture carries the issue's exact journal shape at both capacity 1 and the default: attempt 1 completes `lease_expired` with `wallclock_ms: 30011`, a `retry_backoff` sleep follows, and attempt 2 succeeds. Restored, all five cases pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep relayflow working files out of the change * fix(sdk): keep authored CLI probes from starving worker leases Session-Id: 01a0d409-e854-7540-a76e-a2f9cd136946 * fix(sdk): preserve authored probe compatibility and stdin isolation Session-Id: 01a0d409-e854-7540-a76e-a2f9cd136946 * fix(sdk): detect authored option bags without config key Session-Id: 01a0d409-e854-7540-a76e-a2f9cd136946 * fix(sdk): keep authored option bags from becoming config Session-Id: 01a0d409-e854-7540-a76e-a2f9cd136946 --------- Co-authored-by: Relayflow <noreply@agentrelay.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: khaliqgant <khaliqgant@gmail.com>
Spend the analysis
flows checkalready doesflows checkcomputed the facts that would have prevented five dead runs andsaid nothing about them. This spends two of them, and establishes that the
third fact is not a fact.
What changed
agent_worker_unresolved— a check-only warning (permanent)A spec with
agentsteps needs a worker attached for step typeagent.Without one the run parks at the first such step.
flows checkalready walksthose steps to print
REQUIRES codex (step "implement"), …— the analysisexists; it just stopped one sentence short of the remedy.
Design decisions worth naming:
prediction. Worker attachment is unknown to
flows check, not absent:checkis daemon-free by construction, so it cannot see a worker attachedelsewhere. Saying "this will park" would be a guess.
through a new
CheckInvocationseam oncheckFlow/checkAuthoredFlowrather than entering
preflight, which stays a pure function of the specplus environment probes. Only
flows checksets it.flows runknows theanswer (it attaches its own worker under
--local-agent),flows buildandflows deploycheck a spec that will run elsewhere, and an SDK callerreaching
checkAuthoredFlowdirectly is generally running a worker already.agentsteps.--local-agentattaches nollmworker on theYAML path, so counting
llmsteps and naming that flag for them would befalse. They are not counted and not mentioned.
requirements. A YAML helperstep (
slack: { post: … }) compiles to anagentstep carrying a helperenvelope and needs the same SDK worker, but
requirementsreports it as anintegration, dedupes by harness, and includes
llmuse. Compilation is theonly walk that sees every step that needs the worker.
REQUIRESposition, because it reads as a footnote tothat line.
emitCheckReportholds it back from the leading diagnostic batchand emits it once, after
REQUIRESand beforeCHECK PASSED; diagnosticsstay on stderr, report lines stay on stdout.
--jsonreturns before any ofthat and keeps the single ordered
diagnosticsarray.and rerun; the worker question is still open on the next pass, and staying
silent about it is what made an author meet it one dead run at a time.
Known blind spot, documented rather than papered over: an authored
.flow.tsis checked through
checkMcpHeader, which preflights a synthetic one-stepheader spec without compiling the body, so there are no agent steps to count.
Same blind spot as
permissions_unenforced.gate_path_unreachable— a preflight refusal (temporary, retires with #513)An
artifact_existsgate reads the journaledoutput.artifactslist andnothing else. The bundled worker's scan skips any entry whose name starts with
.and any entry named exactlynode_modules. A gate on a path inside one ofthose prefixes can therefore never match, however faithfully the agent writes
the file. That is unsatisfiable, not merely unproven — so, as the issue
suggested, it refuses:
agent-artifacts.tsintosrc/artifact-scan-policy.ts; the real scan nowconsumes it, so the refusal cannot drift from the scan it describes. A
parity test writes all twelve case paths to a temp dir and asserts the real
snapshotWorkspaceFilesoutput is exactly the predicate's complement.that prefix is the directory the author has to move the artifact out of.
the author's literal string against the worker's literal list, so
node_modules-copy/out.md,reports/node_modules.md,review.mdandreports/v1.2/review.mdare all accepted, and a backslash is an ordinaryfilename character rather than a separator.
probeNamedGateruns afterCLI resolution, model governance, scope and budget can each return, so an
author with an unresolved CLI would not learn about the dead gate until a
later pass — or at run time. A test pins the diagnostic order as
['gate_path_unreachable', 'cli_unresolved'].PREFLIGHT_ENVIRONMENT_FAILURE_KINDS, so it also fires atflows buildandon SDK submissions. That is correct — the gate is unsatisfiable under the
bundled worker regardless of host — and it is a property of the bundled
worker, not of the journal protocol:
step.completeaccepts anyoutput,so a custom worker may journal a hidden path.
gate_output_not_captured— not added, because it is not trueThe acceptance criterion was conditional: warn that a
subprocess_gate'soutput is not captured "for as long as that is true". It is not true today.
Verified against a live
relayflowdbefore writing anything:and in the journal itself, on the lowered gate step's
step.completed:A warning claiming otherwise would have been a false diagnostic added to a
change whose whole point is that
checkshould only say what it knows. Whatthe criterion actually asks for is that the warning exist exactly while the
bug does — so instead of the warning, this adds a live-kernel regression
that pins the capture. If capture ever regresses, that test fails, and the
warning becomes warranted at the moment it becomes true.
Tests
tests/artifact-gates.test.ts— six parameterised refusals asserting thefull excluded prefix is named; six lookalike paths that must be accepted;
the scan/predicate parity test against a real
snapshotWorkspaceFilesrun;the early-return ordering test; and an end-to-end
checkFlowrefusal.tests/check-worker-surface.test.ts(new) — message content, singular vsplural and the "and N more" truncation, exit code 0, the YAML helper step
being counted where
requirementsomits it, silence forllm-only anddeterministic-only flows, survival alongside a preflight refusal, opt-in
discipline,
--jsonemitting it once in both the payload and on stderr, andan ordered
CliIotranscript (both streams in one list) pinning theposition after
REQUIRESand beforeCHECK PASSED— in both theREQUIRES-present andREQUIRES-absent shapes.tests/preflight.test.ts—gate_path_unreachableadded to therefusal-reachability list, which asserts set equality against
PREFLIGHT_FAILURE_KINDS.tests/live-kernel.test.ts(new block) — thesubprocess_gatecaptureregression, asserting both the rendered message and the journal payload, with
a silent-gate control so the assertions cannot pass on a fixed string.
npm testinpackages/sdk: 2380 passed, 40 failed. All 40 failures arepre-existing and environmental — they reproduce identically on a stashed tree
(same 40 failures, same 7 files):
relayflowdlooked up underkernel/target/{debug,release}/while this toolchain builds outside the repo,and flows needing real harness CLIs.
tsc --noEmitis clean forsrc, thetype tests, and
tests.Files
src/artifact-scan-policy.tssrc/named-gate-preflight.tsgate_path_unreachablerefusal (retires with #513)src/cli/check-worker-surface.tsagent_worker_unresolvedwarningsrc/agent-artifacts.tssrc/preflight.tssrc/failure-kinds.tssrc/cli/check.tsCheckInvocationopt-in seamsrc/cli.tscheckdispatch; defers the warning to theREQUIRESpositiondocs/SURFACE.mdEach diagnostic is a distinct kind, so it can be suppressed and later deleted
on its own. Both temporary kinds carry their retirement condition in a comment
at the definition site.
Out of scope, unchanged
The underlying bugs (#511, #513) are not fixed, and the spec is not
round-tripped past the daemon's validator (#502).
Note
Medium Risk
Changes check/preflight diagnostics and CLI report ordering across check, run, build, and SDK submissions, but new outcomes are warnings only except unchanged refusal paths.
Overview
flows checknow spends analysis it already had: specs withagentsteps get an opt-inagent_worker_unresolvedwarning (never a refusal) that names the steps and points atflows run --local-agent, emitted as a footnote afterREQUIRESin plain text. Only the CLIcheckpath setsCheckInvocation.warnUnresolvedAgentWorker;run,build, and defaultcheckFlowcallers stay quiet.Preflight adds
gate_path_unscannedwhen anartifact_existspath crosses a scan-skipped segment (exactly.git,.relayflowd, ornode_modules). The exclusion rule moves to sharedartifact-scan-policy.tsso the walk and static analysis stay aligned; the warning is collected before preflight early returns and warns rather than refusing because JSON stdout or custom workers can still journal those paths.A proposed
subprocess_gateoutput warning was not added; a live-kernel test instead pins that gate stdout/stderr are already journaled and reported.Reviewed by Cursor Bugbot for commit eb64e47. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
flows checknow reports two dead-run causes it already had the facts to detect: specs withagentsteps but no attached worker, andartifact_existsgates on paths the bundled artifact scan never records. A third proposed warning was dropped because the bug it warned about does not exist; a live-kernel regression test pins that instead.Agent worker warning
agent_worker_unresolvedwarning names theagentsteps and points atflows run --local-agent.flows checkopts in, soflows run,flows build, and SDK callers get no noise.Unreachable gate warning
gate_path_unscannedwarning forartifact_existspaths under a segment named exactlynode_modules,.git, or.relayflowd— after merging main's scan change, dot-directories like.workflow-artifacts/are scanned and journaled, so only those exact names trigger it.artifact-scan-policy.tsand the real scan consumes it, so the warning cannot drift.flows buildand on SDK submissions; temporary, and retires when the scan exclusion is fixed (artifact_exists can never pass for a dot-directory path: the worker's journaled artifacts list omits them #513).Written for commit eb64e47. Summary will update on new commits.
Fixes #514