retries_exhausted reports only the last attempt's failure and hides the first - #519
Conversation
A step refused by a pre-receive hook, then retried into a different error, reported only the retry's error. "The push was rejected" and "nothing was staged" point at different bugs, and the first one was invisible without reading the journal by hand. The kernel already appends a step.completed per attempt; the reader kept one candidate and overwrote it. It now accumulates a per-step history, compares the attempts' unbounded journal records rather than their display excerpts, and says so explicitly when they differ — a retry that fails differently usually means the earlier attempt had a side effect. The same rewrite covers agent and llm steps, which the deterministic-only reader returned nothing for, reading the daemon's bounded render when the kernel nulled their output. Additive throughout: no new diagnostic kind, the scalar fields still describe the terminal attempt, and a single-attempt failure renders unchanged. 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 |
Review P2: the new per-attempt history could still hide the first error. A
first attempt rejected by `output_contains` journals exit 0, empty tails and
the verdict that is the entire reason it failed — and `selectEvidence` dropped
`verification.detail` whenever any field happened to be process-shaped, so that
attempt rendered as "exit=0 — no failure evidence recorded". The error was in
the journal and in neither the attempt object nor the message, which `flows
logs` then cannot recover: Cloud keeps the printed bytes and nothing else.
The detail is now withheld only when it would repeat bytes printed beside it —
the daemon's `{exit_code, stdout_tail, stderr_tail}` render of a worker failure
(`worker_failure_detail`), and the `exit_code` gate's bare `exit code was <n>`
next to the exit code it restates (`verify.rs`). Every other verdict is kept,
on each attempt and on the terminal clause.
Also from the review's case 2: `compareAttempts` called an attempt that
journaled nothing a changed cause, so a crash followed by a real error reported
"an earlier attempt may have had side effects" on no evidence at all. Only
attempts that recorded something are compared now; a missing account makes the
comparison `unknown`, which is what docs/SURFACE.md already promised. A
truncated render is still compared — truncation can only make two accounts look
more alike than they were.
Co-Authored-By: Claude <noreply@anthropic.com>
Asserting only that the first rejection appears left the rest of the line free to say anything. Against the real daemon a deterministic step's verdict is `exit code was 1` — the same number printed two words to its left — so pinning the whole line is what proves the suppression rule fires end to end rather than only over hand-built journal entries.
|
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 #519Head: P2 — Honor transcript truncation before reporting unchanged failuresLocation:
This affects an agent whose JSON output is promoted by Include the transcript's failure truncation flag when determining whether matching records can establish Reproduction: source. It uses the production transcript digest builder and journal reader/renderer, with journal-shaped entries. It is not a live worker or daemon test. Command from repository root (requires the built SDK): node review-artifacts/pr519-transcript-truncation.mjs > review-artifacts/pr519-transcript-truncation.log 2>&1Exit status: 1. Complete captured output: Previous findingThe previous review's missing deterministic gate verdict and missing-account comparison case are resolved by this head. The original reproduction was rerun without modification: node review-artifacts/pr519-repro.mjs > review-artifacts/pr519-current-repro.log 2>&1Exit status: 0. Complete captured output: Scope and PR commentsRead Read the current PR body, conversation comments, submitted reviews, and inline comments with: gh pr view 519 --json number,url,baseRefName,headRefOid,body,comments,reviews > review-artifacts/pr519-current-comments.json
gh api repos/AgentWorkforce/flows/pulls/519/comments --paginate > review-artifacts/pr519-current-inline.jsonCaptured responses: PR discussion, inline comments. Submitted reviews and inline comments are empty ( The Cloud tests mock the runner-log endpoint. They establish rendering of captured diagnostics, not actual hosted capture. No live Cloud verification or mutation verification was performed. VerificationKernel command, run from sh ../ops/cargo.sh test -p relayflowd-core > ../review-artifacts/pr519-current-core-tests.log 2>&1Exit status: 0. Literal result lines: Complete captured output: kernel tests. SDK command, run from npm test > ../../review-artifacts/pr519-current-sdk-tests.log 2>&1Exit status: 1. Literal captured lines for the changed tests, the existing diagnostic tests, and suite totals: Complete captured output, including all failure messages and stack traces: SDK tests. The full SDK suite is not green. The changed test files pass, but do not cover the transcript truncation finding. This review did not rerun the clean base and therefore does not independently classify the 41 failures and unhandled error as pre-existing. The PR body’s baseline claim is not substituted for a captured baseline run. |
…are-garden-b6fc6e0f # Conflicts: # docs/SURFACE.md # packages/sdk/src/cli/step-failure.ts
|
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: 562fcb884f
ℹ️ 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 attempt that journaled nothing about itself cannot agree with | ||
| // another one; it can only fail to disagree. | ||
| recorded: exitCodes.length > 0 || accounts.some(account => account !== undefined), |
There was a problem hiding this comment.
Treat crash completion reasons as recorded evidence
When an attempt is abandoned as crashed or lease_expired, abandonment_actions journals that typed completionReason but intentionally leaves output and verification empty, so this expression marks the attempt unrecorded. If a later attempt then reaches a normal terminal failure, compareAttempts filters out the abandoned attempt and reports unknown instead of differs, suppressing the side-effect warning precisely when RFC Appendix A says a crashed agent may have left a dirty workspace. Count non-policy completion reasons as evidence, while retaining the existing normalization of verification_failed and retries_exhausted.
AGENTS.md reference: AGENTS.md:L16-L18
Useful? React with 👍 / 👎.
…are-garden-b6fc6e0f # Conflicts: # packages/sdk/tests/cloud-read.test.ts
…are-garden-b6fc6e0f
…are-garden-b6fc6e0f # Conflicts: # packages/sdk/src/authored-node-runner.ts # packages/sdk/src/failure-kinds.ts
retries_exhaustednow reports every failed attempt, not only the lastThe bug
A commit-and-push step was refused by a GitLab pre-receive hook for a
committer-identity reason. The retry re-ran a commit that had already landed,
died on
nothing staged inside the declared scope, and reported that. Theterminal
retries_exhaustedcarried the last attempt's evidence and nothingelse, so the real rejection was invisible without opening the journal by hand —
and "the push was rejected" and "nothing was staged" point at different bugs.
The evidence was never lost. The kernel appends a
step.completedentry perattempt (
completion_actions,kernel/relayflowd-core/src/machine.rs); thereader threw the earlier ones away.
What changed
The diagnostic reads every failed attempt.
stepFailureDetails(
packages/sdk/src/cli/step-failure.ts) now accumulates a per-step history asit walks the journal instead of overwriting one candidate. The history is keyed
by step, so interleaved steps never pool their attempts and a page boundary
never splits one step's record. A later success clears the history: what a
step that eventually succeeded printed on the way is not the diagnosis of a
later, different failure.
StepFailedDetailsgained two additive fields (failure-kinds.ts):attempts?: StepAttemptFailure[], present only when the step failed more thanonce, and
attemptEvidence?: 'differs' | 'unchanged' | 'unknown'. The existingscalar fields still describe the terminal attempt; no diagnostic kind was added
or renamed, and a single-attempt failure renders exactly as before.
Divergent failures are named explicitly.
compareAttempts(
packages/sdk/src/cli/step-evidence.ts, new) fingerprints each attempt fromits unbounded journal record — not the display excerpt — because two attempts
sharing a 256-byte prefix are not the same failure. Two label pairs are
normalised first: the kernel relabels an identical cause
verification_failedon a retry and
retries_exhaustedon the terminal attempt, which is a policydecision, not a different failure.
unknownis returned honestly when anattempt journaled no account of itself, or when its producer had already
truncated one — equal evidence that was never complete is not evidence of
equality.
Agent and llm steps are covered too. The old
deterministicFailureDetailsreturned
undefinedfor any run without adeterministicstep, which is everyf.agentandf.llmstep. Those steps'{exit_code, stdout_tail, stderr_tail}is nulled out of
outputbypreserve_failure_outputand survives only as thedaemon's bounded
verification.detailrender (worker_failure_detail,kernel/relayflowd/src/engine/remote.rs).selectEvidencereads both shapes.The history crosses the IPC boundary.
stepFailedFrame(
authored-node-runner.ts) validatesattemptselement by element on the sameterms as the frame that carries it. A malformed element is dropped rather than
voiding the whole history: a report naming three of four attempts still carries
the first attempt's error, which is the fact the terminal scalars cannot supply.
Output
A real run, real daemon, real YAML flow with
maxIterations: 2:attempt=2/2is printed besideretries_exhaustedbecause a deterministicstep's default budget is 1, so a single failed attempt with no retry at all
terminates under that same reason. The vocabulary does not change (AGENTS.md
rule 7); printing the facts beside it stops it being misread.
Acceptance
retries_exhaustedincludes the first attempt's errortests/step-attempt-history.test.ts— "reports the first attempt's error beside the last and says they differ";tests/retried-step-failure.test.ts(live kernel, end to end)compareAttemptstests: identical excerpts over divergent records still reportdiffers; a changed gate verdict behind identical process output is seen; the exhaustion relabel is not called a different failureflows logs <run-id>shows every attempttests/cloud-read.test.ts— "prints every attempt of a retried step the runner log captured", built from the real producerrenderStepEvidenceso the assertion cannot drift from the CLI's actual outputThe cloud runner log is external to this repo:
runCloudLogsCliprints itverbatim, redacted, with no elision, and there is no journal-export endpoint.
Criterion 3 is therefore satisfied by the CLI diagnostic itself carrying every
attempt.
docs/CLOUD.mdrecords the one limit that remains — the log is arecording, so a run executed by an older build carries only the terminal
attempt.
Out of scope, and left alone
Retry policy and counts are unchanged. Nothing here decides whether a step with
side effects should be retried at all; the diagnostic only reports that an
earlier attempt's evidence differs, which is the observation an operator needs
to make that call.
Tests
Kernel contract — three tests pinning the journal facts the reader depends on
(per-attempt output preserved beside an identical verdict; the
verification_failed→retries_exhaustedrelabel over byte-identicalevidence; a worker-reported reason not rewritten by the retry branch):
SDK:
The 40 failures are pre-existing and environmental, not caused by this
change. Verified by taking a baseline:
git stash push --include-untracked,re-running the same seven files on a clean tree, and getting the identical 40
failures. Causes, all sandbox:
spawn .../kernel/target/debug/relayflowd ENOENT— those tests hardcodekernel/target/debug, butops/cargo.shsetsCARGO_TARGET_DIRto$HOME/.relayflows-toolchain/target/<key>, deliberately outside the worktree.bun --version1.3.6 against an expected 1.4.0.expected an @relayflows/surface flow handle— module identity between thebuilt
distand the source copy.New SDK tests:
tests/step-attempt-history.test.ts(18 tests — historycollection, comparison, redaction-before-bounding, page boundaries, IPC frame
round-trip) and
tests/retried-step-failure.test.ts(one live-kernel run thatasserts the rendered JSON diagnostic and then reads the journal back to pin
the kernel contract it relies on).
Files
step-evidence.tsis a new sibling rather than more lines instep-failure.ts: raw extraction, bounding, redaction and comparison are oneconcern, and the reader plus the renderer are another. Same reason
step-attempt-history.test.tsis a new file rather than 300 more lines in theexisting 339-line
step-failure-diagnostic.test.ts(AGENTS.md: filesapproaching 500 lines are a design smell).
Note
Medium Risk
Changes failure diagnostics and JSON/
--jsonshape for multi-attempt steps (additive fields) and redaction/bounding of attempt excerpts; behavior is well-tested but affects operator-facing error output on retried runs.Overview
Retried step failures now surface every attempt, not only the terminal
retries_exhaustedrecord. The journal reader walks all failedstep.completedentries per step, keeps terminal scalars unchanged, and adds optionalattemptsplusattemptEvidence(differs|unchanged|unknown) when there were multiple failures.New
step-evidence.tscentralizes journal extraction (deterministic output vs agentverification.detail), redacted 256-byte per-attempt excerpts, unboundedfailureCausecomparison (so shared tail bytes orverification_failed→retries_exhaustedrelabels do not hide real differences), andAttempts:rendering with a side-effect warning when evidence differs.stepFailedFrameand authored paths forward the new fields over IPC.Docs (
SURFACE.md,CLOUD.md) describe the expanded diagnostic and that hostedflows logsonly shows what the runner printed (no journal export). Kernel tests pin per-attempt journaling without changing retry policy. SDK tests cover history, gates, redaction, cloud logs, and a live kernel retried-step scenario.Reviewed by Cursor Bugbot for commit b9ae9ac. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes
retries_exhaustedto report every failed attempt, so a retry that fails differently no longer hides the first attempt's error — exactly when the first attempt's failure is the diagnosis. Fixes #506.attemptsandattemptEvidence.verification_failed→retries_exhaustedrelabel as the same cause.unknown, and an attempt that journaled no evidence is listed but never compared, so neither can raise the side-effect warning without evidence.Written for commit b9ae9ac. Summary will update on new commits.
Fixes #506