fix(flows): say why software-factory ends step_failed on a failed review - #113
Conversation
When the adversarial review leaves no review.clean, the run outcome only
says "its own checks did not pass" (Cloud run f92bf832, cloud#3919). done()
takes only a reason today, so a deterministic report-review-findings step
now prints a bounded excerpt of review.md (or a fixed fallback) to its
stdout, and the stop message repeats it, before done("step_failed").
Move the text into done("step_failed", { detail }) once
AgentWorkforce/flows#542 ships. Clean reviews are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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. |
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds a bounded command that reads filtered findings from ChangesReview findings reporting
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GeneratedFlow
participant FindingsCommand
participant ReviewFile
GeneratedFlow->>FindingsCommand: Run after a non-clean review
FindingsCommand->>ReviewFile: Read and filter review.md
FindingsCommand-->>GeneratedFlow: Return bounded findings or fallback text
GeneratedFlow-->>GeneratedFlow: Append findings to console.error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. A rabbit reads each line, Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // did not pass", so this step prints why (a bounded excerpt of review.md) | ||
| // where the journal and flows status show it. Move this text into | ||
| // done("step_failed", { detail }) once AgentWorkforce/flows#542 ships. | ||
| const reviewFindings = (await f.run(reportReviewFindingsCommand)).trim(); |
There was a problem hiding this comment.
🟡 Second review reports stale findings
When the second reviewer omits review.md, reviewFindings reports the first review's findings. Only review.clean is removed between rounds, so obsolete findings bypass the missing-review fallback.
Learn more
Traditional workflows can run two adversarial reviews. The first failed review writes review.md, then the fixer changes the branch before the second review. The loop removes review.clean before each reviewer but preserves review.md, so a second reviewer that creates neither artifact leaves the first review's file in place. The new report command sees a non-empty file and presents those earlier findings as the reason the final review failed.
Example: Round one reports “P2: retry is broken.” The fixer repairs retries. Round two fails to create either artifact, so the run reports the already-fixed retry finding instead of saying the final reviewer left no report.
Recommended fix: Remove review.md immediately before each adversary runs, alongside review.clean. This preserves round-one findings for the fixer, then clears them before round two so the reporter's missing-review fallback can activate.
Was this helpful? React with 👍 or 👎 to provide feedback.
![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: 04ac36f424
ℹ️ 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".
| 'if [ -s review.md ]; then size=$(findings | wc -c | tr -d " "); fi', | ||
| 'if [ "$size" -eq 0 ]; then echo "relayflow report-review-findings: review.clean absent and review.md missing or empty" && exit 0; fi', | ||
| 'echo "relayflow report-review-findings: review.clean absent; remaining findings from review.md:"', | ||
| `findings | head -c ${REVIEW_FINDINGS_BODY_LIMIT}`, |
There was a problem hiding this comment.
When the sanitized review exceeds 1,700 bytes and this byte boundary falls inside a multibyte character, head -c emits invalid UTF-8; for example, 1,699 ASCII bytes followed by an emoji leaves only its first byte in stdout. Because f.run, the journal, and flows status consume this output as text, the excerpt can be rejected or rendered with a replacement character precisely where a finding is being reported. Truncate at the last complete UTF-8 character within the byte limit instead.
Useful? React with 👍 / 👎.
|
Preview deployed!
This is a Cloudflare Workers preview version of this PR's build. |
2 participants
Why
When the software-factory review leaves no
review.clean, the flow ends withdone("step_failed"). The run outcome then says only:Evidence: Cloud run
f92bf832-7848-58d8-b5ca-da8e3b849f1c(AgentWorkforce/cloud#3919). All 20 steps succeeded. The second reviewer wrote "One P2 remains: cleanup can report success while an ambiguous allocation stays invisible through all three sweeps ...", but the recorded outcome gave no reason.What (interim, path B)
ctx.done()accepts only a reason in the pinned@relayflows/surface(2.0.22) and in the latest published version (2.0.25):done(reason: FlowCompletionReason): void. AgentWorkforce/flows#542 proposesdone(reason, { detail }). Until that ships:FLOW_REPORT_REVIEW_FINDINGS_COMMANDruns as a deterministic step (report-review-findings) right after the review-blocked step and just beforedone("step_failed"). It prints an excerpt ofreview.mdto stdout and always exits 0. The journal andflows statusboth show that stdout.FLOW_REVIEW_FINDINGS_LIMIT(2,000 bytes). When text is cut, the output says so and points toreview-blocked.md.review.mdis missing or empty, the step prints the fixed sentencereview.clean absent and review.md missing or empty.console.error) now includes the same excerpt.done("step_failed", { detail }).Success and failure are decided exactly as before. A clean review never runs the new step.
Tests
web/lib/test/flow-workflows.test.ts: the real command runs undershagainst fixtures. Covered cases: findings printed; a long review bounded and marked as cut; control characters and blank lines stripped; fallback for a missing, empty or whitespace-onlyreview.md; exit code 0 in every case.web/lib/test/flow-onboarding.test.ts: the generated flow is executed. On an unclean review the step runs after the review-blocked step and is the last step, and the findings appear in the stop message. On a clean review, for every preset, the step never runs.Mutation proof (each mutation applied on its own, then reverted)
Results
npm test(web): 231 passed, 1 failed. The failure isFLOW_CHECK_RUN_COMMAND > runs the resolved default end to endand it fails the same way on untouchedorigin/mainon this machine. The cause is the local environment:bunsits in the same directory asnode, and the test assumes it is absent.npx tsc --noEmitinweband inrouter: clean.npm run verify:recommended-flows: passes.No changelog entry, because this is a web-only change.
🤖 Generated with Claude Code
Note
Low Risk
Web-only observability for an existing failure path; review pass/fail behavior is unchanged and output is bounded with exit 0.
Overview
When an adversarial review leaves no
review.clean, the flow still ends withdone("step_failed"), but the run outcome alone does not explain what the reviewer found. This PR adds an interim report-review-findings step (untildone("step_failed", { detail })in AgentWorkforce/flows#542).A new
FLOW_REPORT_REVIEW_FINDINGS_COMMANDruns right after the review-blocked step and beforedone("step_failed"). It prints a sanitized, byte-capped excerpt ofreview.mdto stdout (always exit 0), so the journal andflows statusshow the reason. The same text is appended to the flow’sconsole.errorstop message. Clean reviews never run this step.Tests cover the real shell command (truncation, control-character stripping, empty/missing
review.md) and generated-flow behavior (ordering on failed reviews; absent on success).Reviewed by Cursor Bugbot for commit 04ac36f. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a deterministic report-review-findings step so runs that end in
done("step_failed")on an unclean review record why, instead of only "its own checks did not pass".review.md(stripped of control characters and blank lines, cut at 1,700 bytes) to the step's stdout, or a fixed fallback when the file is missing or empty; the stop message repeats the excerpt.done(reason, { detail })ships, move this text into the detail and drop the step.Written for commit 04ac36f. Summary will update on new commits.