Repository navigation
Software factory change - #539
Merged
Merged
Conversation
The step-failure render cut every captured stream to its last 1,024 bytes. For any runner that streams results and prints its totals last — TAP (`node --test`), cargo, vitest, pytest — that last kilobyte is by construction the summary and the final few passing cases, so the one `not ok` was exactly the part that got cut. The report proved a failure happened and withheld which one, pushing diagnosis back to a local re-run. The bytes were never lost by the kernel: `exec_det.rs` captures 64 KiB per stream and `preserve_failure_output` keeps it on a failed deterministic completion. This is a render fix, and no kernel change is needed. `cli/step-excerpt.ts` selects three disjoint ranges of the journaled field in source order — a head of whole lines, the lines in the elided middle that match a built-in failure marker (TAP `not ok`, `FAIL`/`FAILED`, a vitest or jest glyph, cargo's `... FAILED` and `---- <case> stdout ----`, Rust's `panicked at`), and a tail — inside a 4,096-byte bound, with the elision stated in band. Markers are matched on an escape-stripped view of each line, so a coloured header still matches; journal data is not mutated. The header becomes `Stdout (captured excerpt):`. `(last 1,024 bytes)` was a promise about the excerpt's shape made where it could no longer be true; the marker now makes it in band, only when it is. The absence of a marker means the field the journal carried fit whole — not that the command printed nothing more, which no renderer can know. Field names `stdoutTail`/`stderrTail` are unchanged for compatibility; the new semantics are documented on them and in docs/SURFACE.md. Co-Authored-By: Claude Opus 5 <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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 377b277. Configure here.
Review of #539 found the reported bug one layout further in. The tail is cut at a byte offset, not a line boundary, so a matching line longer than the tail's share has its `not ok` and its test name ahead of the cut while the tail shows only the uninformative rest. `selectHighlights` skipped such a line as "already visible in the tail", and the omission counter treated any overlap as visibility, so the excerpt neither named the failing case nor disclosed that it had recognised one. A match is covered only when its own start is on screen, because the start is what names the case. So the middle now ends where the tail begins on a line, not where its byte cut lands: a line the tail starts in the middle of is a highlight candidate like any other, shown from its own start, whole or under `… line truncated …`. The omission counter asks the same question, so a crossing line the share cannot fit is counted in `… +N more matching lines …` rather than passed off as seen. A kept prefix can end exactly where the tail begins. Printing those as two ranges would announce a truncation and an elided region between two halves of one unbroken run of source, so the tail starts at the highlight instead — the same bytes, one fewer false claim. Four regressions pin it: the reviewer's case; a 400-stream sweep sliding the failing line one byte at a time across the boundary, covering every alignment including its first byte landing exactly on the cut; the no-invented-cut rendering; and the omission count. Mutation-verified both ways — the whole fix reverted fails three of the four, the merge alone reverted fails the fourth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
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 #539 — current headReviewed #539 at DispositionNo additional actionable code defect found in this diff. The previous P2 Previous finding and edge casesThe current selector includes a matching line whose start precedes the tail cut, The 64 KiB kernel capture limit remains a documented limitation, not a new PR commentsThe CodeRabbit issue comment says review was skipped. Cursor's inline comment Commands used to retrieve the comments (repository root): gh pr view 539 --json comments,reviews > review-artifacts/current/comments.json
gh api --paginate repos/AgentWorkforce/flows/issues/539/comments --jq '.[] | {id,body}' > review-artifacts/current/issue-comments.json
gh api --paginate repos/AgentWorkforce/flows/pulls/539/comments --jq '.[] | {id,body,path,original_line}' > review-artifacts/current/inline-final.json
gh api --paginate repos/AgentWorkforce/flows/pulls/539/reviews --jq '.[] | {id,body,state,commit_id}' > review-artifacts/current/reviews.jsonVerificationThe full SDK command includes kernel build, SDK typechecking/build, test Previous reproduction (repository root), exit 0node review-artifacts/tail-boundary.mjs > review-artifacts/current/tail-boundary.log 2>&1Focused tests (packages/sdk), exit 0npx vitest run tests/step-failure-excerpt.test.ts tests/step-failure-diagnostic.test.ts tests/resume-failure.test.ts tests/direct-run-failure.test.ts tests/authored-run-failure-evidence.test.ts tests/authored-node-result.test.ts > ../../review-artifacts/current/focused.log 2>&1Full affected package suite (packages/sdk), exit 1npm test > ../../review-artifacts/current/sdk-tests.log 2>&1Complete captured output: |
kjgbot
marked this pull request as ready for review
September 22, 2026 20:56
Member
Superseding final review: GO / CLEANThis supersedes the earlier adversarial validation hold. I independently reviewed PR #539 at exact head Cursor's prior tail-boundary finding is fixed at the current head: a failure line whose prefix is before a byte-cut tail is now selected by its own start, visibility/omission accounting uses that same boundary, and an adjacent highlight/tail is merged without inventing an elision. The 400-alignment regression sweep and long crossing-line cases pin the fix. GitHub now marks that sole review thread Current-main-plus-PR validation: The daemon above was freshly built from current Exact-head GitHub Actions run I also exercised the newer current-main composition: 2,724 tests passed, including all #539-focused and newly landed authored-parallel coverage. The only local non-passes were explicit environment gates unrelated to this patch (host Bun GitHub currently reports Verdict: clean and merge-ready at this exact head. The PR is already out of draft; the owner was correct to mark it ready without changing the SHA. Any new commit would require revalidation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A failure report that names the failing test
Fixes: Failure report shows only the last 1,024 bytes, which never contains
the failing test.
Branch:
relayflow/flows-software-garden-c2deb4cd· Commit:377b277The problem, and where the bytes were lost
A deterministic step's failure report cut each captured stream to its last
1,024 bytes. For any runner that streams results and prints its totals last —
TAP (
node --test), cargo, vitest, pytest — that last kilobyte is byconstruction the summary and the final few passing cases, so the one
not okwas exactly the part that got cut.
The bytes were never lost by the kernel.
kernel/relayflowd/src/exec_det.rscaptures the last 64 KiB of each stream, and
preserve_failure_output(
kernel/relayflowd-core/src/machine.rs) keepsoutputintact on a faileddeterministic completion. The SDK read the whole field and then threw it away
at
packages/sdk/src/cli/step-failure.ts'stail(). This is a render fix.No kernel change is made, and none is needed.
What changed
New:
packages/sdk/src/cli/step-excerpt.ts(formatStepExcerpt). Itselects three disjoint ranges of the journaled field, in source order, inside a
4,096-byte bound:
The elision is stated in band. Markers are matched against an escape-stripped
comparison view of each line, so a coloured
FAILheader still matches;journal data is never mutated, and display sanitising stays a separate, later
step. The marker set is TAP
not ok,FAIL,FAILED, a vitest/jest failureglyph, cargo's
... FAILEDand---- <case> stdout ----headings, and Rust'spanicked at— each with a test pinning a real line from that runner. They arehints, not a parser: an unrecognised format still gets head-and-tail context.
Per the plan review, the budget is a fixed 4,096 bytes and the markers are
built in. No environment variable and no
failureHighlightspec field areadded here: a parser for a presentation setting that throws while extracting
failure evidence would remove otherwise-readable evidence from the report
(
classifyOutcomecatches extraction failures and reportsCould not inspect the failed step), and a new step key would mean a kernel spec change to solvea rendering problem.
Header.
Stdout (last 1,024 bytes):becomesStdout (captured excerpt):.(last 1,024 bytes)was a promise about the excerpt's shape made where itcould no longer be true; the elision marker now makes it in band, only when it
is. Field names
stdoutTail/stderrTailare unchanged for compatibility, withthe new semantics documented on them and in
docs/SURFACE.md.What it now prints
A 14-case TAP run with chatty subtest diagnostics, 52,744 bytes captured:
The same stream under the old render — every visible line is
okor a total,which is the reported bug exactly:
What this does not promise
Stated here because the excerpt now makes a claim about itself, and the claim
has to be exact.
step is the last 64 KiB the kernel captured. A failure printed before that
window is not here to be found.
does not mean the command printed nothing more.
agent/llmstep's evidence still survives only as the daemon's renderin
verification.detail, capped at 2,000 chars by the daemon. This helps adeterministic step fully and an agent step only up to that cap. Raising it is
a separate change and is out of scope.
Files
packages/sdk/src/cli/step-excerpt.tspackages/sdk/src/cli/step-failure.tstail()→formatStepExcerpt; header textpackages/sdk/src/failure-kinds.tsdocs/SURFACE.mdpackages/sdk/tests/step-failure-excerpt.test.tspackages/sdk/tests/step-failure-diagnostic.test.ts--jsonpackages/sdk/tests/authored-run-failure-evidence.test.tspackages/sdk/tests/authored-node-result.test.tspackages/sdk/tests/{direct-run,resume}-failure.test.tsVerification
Every command below was run from
packages/sdk, and the output is pasted, notsummarised.
Typecheck and build
The touched tests
Full suite — and the failures that are not mine
npm test40 tests fail, and all 40 fail identically without this change. They are
environmental in this sandbox:
live-kernel,webhook-live,provider-trigger-executorandmcpspawn a hardcodedkernel/target/debug/relayflowd, which does not exist becauseops/cargo.shkeeps
CARGO_TARGET_DIRoutside the repo;stuck-run-triageandauthored-node-runtimefail onexpected an @relayflows/surface flow handle;live-kernelalso wants a real analyzer CLI. None of the 40 mentions a stepfailure, an excerpt or a stream tail.
Baseline, established by stashing the change, rebuilding and running the same
seven files:
The same seven files with the change restored (
git stash pop, rebuild), samecommand:
Identical:
40 failed | 50 passed | 18 skipped (108)either way.Mutation verification
One claim, done to the letter of AGENTS.md. The change reverted is the
extraction path itself:
formatStepExcerptat the four call sites inevidence()replaced by the old 1,024-bytetail(). The test is thelive-kernel regression, which runs a real
relayflowd, a real command and thereal journal.
Before — the file's hash, then the mutation:
cp src/cli/step-failure.ts /tmp/step-failure.ts.orig && sha256sum src/cli/step-failure.tsMutated — the test fails, at the assertion it exists for:
npx vitest run tests/authored-run-failure-evidence.test.ts \ -t 'names the failing case from the middle of a real test run'The failing assertion is at
tests/authored-run-failure-evidence.test.ts:120,which is the rendered excerpt. The assertion above it —
expect(journaled).toBe(expected), read back from the child run's journal —passed under the mutation. That is the bug in one pair of assertions: the
journal held the whole stream and the renderer cut the line out of it.
Restored byte-for-byte, and green again:
Same hash before and after. Nothing else in this change is labelled
mutation-verified.
Regression coverage
tests/step-failure-excerpt.test.ts(38 tests) pins the contract: streamsunder, at and one byte over the budget; head/highlight/tail selection; byte
accounting (
shown + elided == input, asserted arithmetically); one markertest per supported runner, plain, indented and ANSI-coloured; unknown formats
getting context without claiming a match; head/tail matches not repeated; two
identical lines at two positions counted as two observations; the 20-line cap
with its
+N more matching linescount; a matched line too long to fit shownin part under
… line truncated …; CRLF, tabs, control and bidi characters; nosplit multibyte characters and no invented U+FFFD while keeping one the capture
itself produced; byte bounds at 256, 1,024, 4,096 and 8,192; and a refusal for
an unusable budget.
The live-kernel regression asserts its own fixture before it asserts the
render: the failure sits past the first 4 KiB and before the last 4 KiB of a
~13 KiB stream, inside the kernel's 64 KiB capture, and is absent from the last
1,024 bytes — so it cannot be satisfied by a larger budget or by head-and-tail
context alone.
Not changed, deliberately
kernel/— no behaviour change, so no new Rust tests. If scope ever expandsto capture, that needs kernel-level regressions of its own.
cli/status.ts'sDETAIL_LIMIT— a different surface with a differentfailure mode.
completionReason, exit status andevidence precedence in
step-failure.tsare untouched.plan.mdandreviewed-plan.mdare left intact and uncommitted, as is thisfile.
Note
Low Risk
Render-only SDK change to failure diagnostics; no kernel or persistence behavior change, with broad unit and integration tests.
Overview
Replaces the last 1,024-byte tail of step failure stdout/stderr with
formatStepExcerpt, a 4,096-byte captured excerpt that keeps a head, failure-marker lines from the elided middle (TAPnot ok, pytest/vitest/cargo/Rust patterns), and a tail, with in-band elision when the journal field does not fit whole.step-failure.tsroutes journalstdout_tail/stderr_tail, worker failure excerpts, and fallbackdetailthrough the new formatter; human labels change from(last 1,024 bytes)to(captured excerpt).stdoutTail/stderrTailJSON keys are unchanged;docs/SURFACE.mdandfailure-kinds.tsdocument the new semantics and limits (64 KiB kernel capture, no elision marker ⇒ field fit whole).Tests add
step-failure-excerpt.test.tsand extend CLI, live-kernel, and authored IPC coverage so middle-of-stream failures (e.g. TAP) appear in diagnostics instead of only summary lines.Reviewed by Cursor Bugbot for commit a754dc6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes step failure reports so the failing test is no longer cut out of the capture.
not okwas always the part dropped.formatStepExcerptnow prints a head, failure-marker lines from the middle, and a tail within a 4,096-byte budget, stating the elision in band; a stream that fits is printed whole with no marker.not ok,FAIL/FAILED, vitest/jest glyphs, cargo's... FAILEDand---- <case> stdout ----, and Rust'spanicked at; unrecognized formats still get head-and-tail context.… line truncated …) so the test name still appears; a line is counted as shown only when its start is on screen, which also keeps the omission count honest.(last 1,024 bytes)to(captured excerpt); field namesstdoutTail/stderrTailstay unchanged. This is a render-only fix — no kernel change.Written for commit a754dc6. Summary will update on new commits.