The worker-parked message names --local-agent only for 'flows run' on a spec path, and 'flows resume --local-agent' silently ignores the flag - #518
Conversation
…gent from being ignored
The remedy for a worker park was a string literal welded into
`classifyOutcome` behind `command === 'run' && !isAuthoredFlowPath(path)`, so
an authored `.flow.ts` and a parked `flows resume` both got nothing, and
someone who had already passed `--local-agent` was told to pass it again. The
authored omission was deliberate — a bare `flows run --local-agent <path>` is
refused for want of `--input` — but silence read as "your infrastructure is
missing a worker" when the fix was one flag away.
Make the remedy a value: a closed union and one formatter in
`cli/local-agent-remedy.ts`, with a single decision function shared by both
authored boundaries so a run and a resume of the same root cannot disagree. A
spec run names a new run (byte-identical prefix to before), a spec resume names
this run, an authored flow names a new run carrying the `--input` the parked one
was started with — never a fabricated `{}`, which would name a different
invocation. An already-attached worker gets the truth instead: none was
eligible for this step.
Carry the cause structurally rather than by wording. `agent_parked` covers both
"nothing is attached" and the kernel's `needs_human` recovery wait, and only the
first is fixed by a worker, so `ParkCause` now travels from `classifyOutcome`
through the authored error and the Bun-to-Node IPC frame, validated on the way
back in. An unclassified park gets no guess.
Honour `flows resume --local-agent` on declarative runs — that path existed but
had never been exercised end to end — and refuse it on authored roots, where the
worker stream is admitted at run start and pinned into the root metadata. The
two guards that knew this threw bare `Error`s and surfaced as `protocol_error`,
blaming the daemon for an invocation mistake; they are now exit-2
`local_agent_unavailable` refusals returned before any worker attaches and
before the journal is touched. An authored resume that parks for want of a
worker is likewise reported as the park it is, not a protocol failure.
Both acceptance criteria are asserted live: a parked authored flow's printed
remedy is fed back through a shell verbatim and must complete the run, and a
parked spec run resumed with the flag must progress rather than repeat its
message.
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 |
…arser accept The remedy a parked authored resume prints repeats the recorded `--input` inline, because the journal keeps the input document and not the word that carried it. Running that command back refused with exit 2 `input_unreadable` for any input past a few hundred bytes: `parseDirectInput` opens by stat-ing the argument as a path, and a JSON object longer than a filename fails `ENAMETOOLONG` rather than `ENOENT`, which the catch read as "a file is there and I could not look at it". So the one command offered to someone whose run had parked died before a worker could attach. Name the condition instead of the errno: `namesNoFile` accepts `ENAMETOOLONG` alongside `ENOENT`, because no path component may exceed 255 bytes on any filesystem this runs on, so an argument that long is not a filename anybody could have meant. Permission and I/O errors still refuse — a path is there and unreadable, and guessing inline JSON would parse a filename as a document. The shell has its own ceiling, and it is lower than the input limit. Linux caps one `execve` argument at `MAX_ARG_STRLEN` (128KiB) while `--input` admits a mebibyte, so a run started from an ordinary input file can record more than any shell will carry, and printing it yields `Argument list too long` — no better than the park it answers. The recorded input becomes a closed union: rendered inline when it fits, and otherwise stated in prose with its size and a pointer back to the input file, because a truncation or a placeholder would look runnable. A park reported by `flows run` is unaffected: it echoes the `--input` word that invocation was given, so a run started from a file still names that file. Both halves are asserted where they broke. The live regression parks a run started from an input file too long to be a filename, feeds the printed remedy back through a shell verbatim, and requires the run to complete; the resume test requires an oversized input to produce a sentence and no `--input '`. 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 #518 at No new actionable code defects found. The prior P2 file-input finding is resolved. Clean signoff is withheld for unresolved validation; Reviewed the full two-commit diff, changed tests, docs, PR description, conversation comments, inline comments, and reviews. The review covered worker attachment on declarative resume, authored admission refusals, human versus worker park classification, error propagation across Node IPC, shell quoting, input-file recovery, and oversized recorded input. No implementation files or gates were edited during this review. The previous report's 311-byte input reproduction now reaches Remaining validation gap. The full SDK command exits 1: seven test files fail and Vitest reports an unhandled error. Failures include missing Full SDK command, working directory npm test > ../../review-artifacts/current-sdk-tests.log 2>&1Exit status: 1. Full unedited stdout/stderr: current-sdk-tests.log. Literal final output: Literal changed-test result lines from that same run: Independent reproduction, working directory repository root: node review-artifacts/current-repro-input-remedy.mjs > review-artifacts/current-repro-input-remedy.log 2>&1Exit status: 0. Script and captured output. The script asserts the initial park, flagged-resume refusal, and successful printed remedy. It also records plain resume for inspection; it does not assert that plain resume succeeds. Plain resume encounters a terminal authored root ( The first rerun of the old reproduction script could not report the successful result because it assumed a diagnostic was always present; its output records that script error. The current script above handles an empty diagnostics array. No mutation-verification claim is made. PR discussion commands, working directory repository root: gh pr view 518 --json number,url,baseRefName,headRefOid,comments,reviews > review-artifacts/current-pr.json
gh api --paginate repos/AgentWorkforce/flows/pulls/518/comments > review-artifacts/current-pr-inline-comments.json
gh api --paginate repos/AgentWorkforce/flows/pulls/518/reviews > review-artifacts/current-pr-reviews.jsonCaptured responses: PR and conversation comments, inline comments, reviews. Both API list responses are literally |
khaliqgant
changed the title
|
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7dd8a1a26
ℹ️ 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".
| `npm run typecheck` clean. Full `packages/sdk` suite: **2382 passed, 40 failed** — | ||
| the same 40, test for test, on a clean `git stash` of this branch's HEAD. They | ||
| are environmental: `kernel/target/debug/relayflowd` does not exist in this | ||
| sandbox (the live suites that resolve it via `ops/cargo.sh` all pass), and | ||
| `stuck-run-triage.test.ts` fails on an unrelated surface-handle assertion. |
There was a problem hiding this comment.
The committed report asserts that npm run typecheck was clean, gives full-suite pass/fail counts, claims the failures were identical on a clean stash, and attributes them to specific environmental causes, but includes neither the literal commands nor their captured output. Add the required transcripts or narrow/remove these claims so reviewers can reproduce the stated evidence.
AGENTS.md reference: AGENTS.md:L90-L96
Useful? React with 👍 / 👎.
| if (error instanceof AuthoredFlowExecutionError && (error.code === 'agent_parked' || error.code === 'llm_parked')) { | ||
| return { | ||
| exitCode: 3, | ||
| report: { | ||
| ...base, |
There was a problem hiding this comment.
This change adds roughly 120 net lines of authored-root admission, remedy construction, and parked-report handling to cli/run.ts, taking the already oversized module to 1,017 lines. Move the new authored-resume policy/report construction into a focused module rather than further expanding the central runner; the repository explicitly treats files approaching 500 lines as a design smell requiring justification or a split.
AGENTS.md reference: AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
…are-garden-4e32b39f # Conflicts: # summary.md
…are-garden-4e32b39f
2 participants
Name the missing worker in every park, and stop
resume --local-agentfrom being ignoredSummary
A run that parks for want of an agent worker now names the invocation that
supplies one — on every path that can park, not just
flows runagainst a specfile. And
flows resume --local-agent, which the usage line has alwaysadvertised, is now either honoured or refused; it is never accepted and ignored.
Two defects, one cause: the remedy was a string literal welded into
classifyOutcome, guarded bycommand === 'run' && !isAuthoredFlowPath(path)..flow.tsgot nothing. The omission was deliberate — the bareflows run --local-agent '<path>'would be refused for want of--input—but the result read as "your infrastructure is missing a worker" when the fix
was one flag away.
flows resumegot nothing, because of thecommandguard.--local-agentgot told to pass it again, orgot silence. Neither is the truth, which is that a worker was offered and none
of them was eligible for the step.
What changed
The remedy became a value.
src/cli/local-agent-remedy.tsholds a closedunion —
none | attached | spec-run | spec-resume | authored-run— and oneformatter. A park decides which remedy it has; only the formatter decides how
it reads. Both authored boundaries share one decision function,
authoredWorkerRemedy, soflows runon a.flow.tsandflows resumeon thesame root cannot disagree.
flows run <spec>flows run --local-agent '<path>'— byte-identical prefix to before, plus the--data-dirthis invocation usedflows resume <run-id>on a spec runflows resume --data-dir <dir> --local-agent <run-id>— this run, which is resumable.flow.tsflows run --local-agent '<path>' --input '<input>', repeating the input the parked run was started with--local-agentalready passedneeds_humanrecovery waitWhere the journal recorded no input, the authored case states the requirement in
prose rather than emitting
--input '{}': a fabricated input names a differentinvocation of the flow than the one that parked.
The cause travels as a value, not as wording.
agent_parkedcovers both "nothingis attached" and the kernel's
needs_humanrecovery wait, and only the first isfixed by a worker. A new
ParkCause(src/failure-kinds.ts, beside the rest ofthe run vocabulary) is set by
classifyOutcome, carried onRunReport.parkCauseand
AuthoredFlowExecutionError.parkCause, forwarded across the Bun→Node IPCerror frame, and validated on the way back in (
parkCauseFrame, fail closedto
undefined). An unclassified park gets no guess.flows resume --local-agentnow means something.That code existed; nothing had ever exercised it end to end, which is how the
field report lost a cycle.
--local-agentis admitted at run start andpinned into the root's metadata — a resume can only reproduce the surface the
run began with. The two guards that already knew this threw bare
Errors andsurfaced as
protocol_error/RUN <id> unknown, blaming the daemon for aninvocation mistake. They are now exit-2
REFUSED [local_agent_unavailable],returned before any worker attaches and before the resume touches the
journal, each naming the opposite remedy: start a new run, or resume with the
flag you dropped.
Also fixed on the way through. An authored resume that parked for want of a
worker fell through to
protocolFailure; it is now the exit-3 park it is, withthe child run holding the evidence and the root it was resumed from kept
separate (
runId/rootRunId).Acceptance
Both criteria from the report are asserted against the built CLI and a real
relayflowd, not against mocks..flow.tsrun at an agent step prints a runnable remedy."tests/local-agent-live.test.tsparks a real authored flow started with--input '{"task":"it'"'"'s a plan","n":1}', then feeds the printed line backthrough
sh -cverbatim and asserts the flow completes and the agentwrapper really ran. Runnable is proven by running it, so the quoting, the flag
order and the argument values are all covered at once.
tests/yaml-local-agent-live.test.tsparks a spec run, resumes it with--local-agent, and asserts exit 0 /completionReason: success— and thatthe first park's message does not reappear. A companion test takes the case
where the flag genuinely cannot help (a declared workspace surface no local
worker holds) and asserts the second message differs and says a worker was
attached but ineligible.
Tests
New:
tests/local-agent-remedy.test.ts(11) — every remedy shape, with a realsh -cround-trip of each rendered command and a hostile input(
{"shell":"$(rm -rf /)"}) and path (my flows/a b's.flow.ts).tests/resume-local-agent.test.ts(7) — both refusals, proven to attach nothingand journal nothing, proven not to be
protocol_error, plus the authored resumepark.
Extended:
classify-outcome(+4, including the authored-suppression orderingthat would otherwise double the clause),
direct-run-failure(+4),yaml-local-agent-live(+2),local-agent-live(+1).npm run typecheckclean. Fullpackages/sdksuite: 2382 passed, 40 failed —the same 40, test for test, on a clean
git stashof this branch's HEAD. Theyare environmental:
kernel/target/debug/relayflowddoes not exist in thissandbox (the live suites that resolve it via
ops/cargo.shall pass), andstuck-run-triage.test.tsfails on an unrelated surface-handle assertion.Docs
docs/SURFACE.md§5 gains Naming the worker a park is missing — the remedytable, the two parks that deliberately print nothing, and the run-start admission
rule that makes the authored resume a refusal. The exit-
2row now listslocal_agent_unavailable.Not in scope
Whether
--local-agentshould be the default, and the empty observer URL on--local-agentruns (#341).Note
Medium Risk
Changes user-visible CLI exit-2/exit-3 behaviour and resume admission for
--local-agent; mistakes could mis-route recovery, though extensive tests and fail-closed cause validation limit blast radius.Overview
Exit-3 worker parks now append runnable, shell-quoted
--local-agentguidance everywhere it applies, instead of a YAML-only hint onflows runor silence for authored flows and resumes.Park reporting adds structural
ParkCause(worker_unavailablevsneeds_human) onRunReportandAuthoredFlowExecutionError, forwarded over the authored Node IPC frame and validated withparkCauseFrame, so the CLI recommends attaching a worker only when that is the fix. A newlocal-agent-remedymodule turns that into concrete commands: new spec run, resume this spec run, or restart an authored.flow.tswith the same--input(verbatim argument, prose when input is missing, or size guidance when inline JSON exceedsexecvelimits). If--local-agentwas already passed, the message says no eligible worker was attached instead of repeating the flag.flows resume --local-agenton authored roots is now exit 2local_agent_unavailablebefore journal/worker touch when the flag mismatches what was pinned at run start; declarative resumes still honour the flag. Authored resume parks get the same remedy as direct runs, withrootRunId/runIdseparated.Supporting fixes: shared
shell-wordquoting;parseDirectInputtreatsENAMETOOLONGlike “not a file” so long inline JSON is notinput_unreadable;docs/SURFACE.mddocuments the remedy table;summary.mdremoved; broad live and unit tests assert copy-pasteable commands.Reviewed by Cursor Bugbot for commit 2b9d0c1. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Worker-park reports now always name a runnable
--local-agentremedy when a worker is needed, andflows resume --local-agenteither moves the run forward or refuses — never accept-and-ignore. Also removes the stalesummary.md, resolved as a merge conflict againstorigin/main.Bug Fixes
.flow.tsparks print a runnableflows run --local-agent '<path>' --input '<input>'command repeating the input the parked run started with; when none was recorded, the report states the requirement instead of emitting--input '{}'.flows resume --local-agent <run-id>, and a spec run keeps its byte-identical command with the--data-dirappended.worker_unavailableorneeds_human, so recovery waits and unclassified parks never get worker advice, and an already-attached run is told no worker was eligible instead of to pass the flag again.flows resume --local-agentnow attaches a worker and drives the parked step on declarative runs; against an authored root it is refused with exit 2local_agent_unavailablebefore a worker attaches or the journal is touched, naming the run that fixes it.protocol_error.--inputlonger than a filename is no longer refused asinput_unreadable, and recorded inputs larger than a shell argument are stated in prose with their size instead of printed as an unrunnable command.summary.md.Written for commit 2b9d0c1. Summary will update on new commits.
Fixes #504