From 04ac36f424517db088376b7130766e29d85585f5 Mon Sep 17 00:00:00 2001 From: Khaliq Date: Mon, 21 Sep 2026 09:12:44 -0700 Subject: [PATCH] fix(flows): say why software-factory ends step_failed on a failed review 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 --- web/lib/flow-workflows.ts | 44 +++++++++++++++++++++++++- web/lib/test/flow-onboarding.test.ts | 14 +++++++-- web/lib/test/flow-workflows.test.ts | 46 +++++++++++++++++++++++++++- 3 files changed, 100 insertions(+), 4 deletions(-) diff --git a/web/lib/flow-workflows.ts b/web/lib/flow-workflows.ts index aa9171b..5de471f 100644 --- a/web/lib/flow-workflows.ts +++ b/web/lib/flow-workflows.ts @@ -427,6 +427,42 @@ export const FLOW_REVIEW_BLOCKED_COMMAND = [ 'if gh pr comment --body-file review-blocked.md >/dev/null 2>&1; then echo "relayflow: posted the unresolved review to the pull request."; else echo "relayflow: could not comment on the pull request; review-blocked.md still holds the findings." >&2; fi', ].join('; '); +/** Upper bound, in bytes, on everything FLOW_REPORT_REVIEW_FINDINGS_COMMAND prints. */ +export const FLOW_REVIEW_FINDINGS_LIMIT = 2000; +const REVIEW_FINDINGS_BODY_LIMIT = 1700; + +/** + * Says why a run that ends in `done("step_failed")` on a failed review failed. + * + * The runtime records that ending as "its own checks did not pass. No step + * failed, so there is no step-level evidence to inspect": every step succeeded, + * so nothing in the run outcome says what the reviewer found. Cloud run + * f92bf832 (AgentWorkforce/cloud#3919) did all 20 steps and ended on exactly + * that, while its second reviewer had written "One P2 remains: ...". + * + * `done()` takes only a reason today. AgentWorkforce/flows#542 proposes + * `done("step_failed", { detail })`; once that ships, pass this text as the + * detail and drop this step. Until then the findings go to this step's stdout, + * which the journal keeps and `flows status` shows for the step. + * + * review.md is agent-authored, so this prints a bounded excerpt, never the + * file: blank lines and control characters removed, cut at + * REVIEW_FINDINGS_BODY_LIMIT bytes, and the whole output stays within + * FLOW_REVIEW_FINDINGS_LIMIT. Like the commands above it always exits 0. + */ +export const FLOW_REPORT_REVIEW_FINDINGS_COMMAND = [ + 'export LC_ALL=C', + `findings() { tr -d '\\000-\\010\\013-\\037\\177' < review.md | sed '/^[[:space:]]*$/d'; }`, + 'size=0', + '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}`, + 'echo', + `if [ "$size" -gt ${REVIEW_FINDINGS_BODY_LIMIT} ]; then echo "relayflow report-review-findings: cut at ${REVIEW_FINDINGS_BODY_LIMIT} of $size bytes; the full review is in review-blocked.md."; fi`, + 'exit 0', +].join('; '); + export function workflowAgents(selected: readonly string[]) { const builder = selected.filter(isCodingAgent)[0] ?? 'claude'; const reviewer = selected.filter(isCodingAgent).find(id => id !== builder) ?? builder; @@ -627,6 +663,7 @@ export function workflowCode(workflow: WorkflowId, agents: ReturnType { throw new Error('Interactive human approval is unsupported'); }, @@ -368,7 +369,7 @@ describe('software factory onboarding', () => { }); it('marks the pull request and parks, never approves, if all reviews fail', async () => { - const { calls, finish } = await runFactory([false, false, false]); + const { calls, finish, errors } = await runFactory([false, false, false]); expect(calls.filter(call => call.startsWith('adversary-'))).toHaveLength(2); expect(calls).not.toContain('human'); // done("step_failed") is the honest reason, and since the 2.0.15 pin the @@ -380,6 +381,14 @@ describe('software factory onboarding', () => { expect(finish).toBe('step_failed'); expect(calls).toContain(FLOW_REVIEW_BLOCKED_COMMAND); expect(calls.indexOf(FLOW_REVIEW_BLOCKED_COMMAND)).toBeGreaterThan(calls.lastIndexOf('adversary-2:codex')); + // The run outcome says only "its own checks did not pass", so the findings + // are printed by a step of their own just before done("step_failed"), and + // repeated in the stop message (AgentWorkforce/flows#542 would carry them + // on done() itself). + expect(calls.indexOf(FLOW_REPORT_REVIEW_FINDINGS_COMMAND)).toBeGreaterThan(calls.indexOf(FLOW_REVIEW_BLOCKED_COMMAND)); + expect(calls.at(-1)).toBe(FLOW_REPORT_REVIEW_FINDINGS_COMMAND); + expect(errors.join('\n')).toContain('One P2 remains.'); + expect(factorySource(completed)).toContain('AgentWorkforce/flows#542'); expect(withoutComments(factorySource(completed))).toContain('f.done("step_failed")'); // Named in the generated flow itself, so a reader meets the release that // made the honest reason lowerable rather than guessing. @@ -394,6 +403,7 @@ describe('software factory onboarding', () => { // with the same reason, so the difference has to be visible somewhere. It // is — a clean run never marks the pull request as unapproved. expect(calls).not.toContain(FLOW_REVIEW_BLOCKED_COMMAND); + expect(calls).not.toContain(FLOW_REPORT_REVIEW_FINDINGS_COMMAND); expect(calls.some(call => call.includes('pr merge'))).toBe(false); expect(factorySource({ ...completed, workflow })).not.toContain('f.human('); expect(calls).toContain(FLOW_CHECK_RUN_COMMAND); diff --git a/web/lib/test/flow-workflows.test.ts b/web/lib/test/flow-workflows.test.ts index df580ee..c23b6d2 100644 --- a/web/lib/test/flow-workflows.test.ts +++ b/web/lib/test/flow-workflows.test.ts @@ -5,7 +5,8 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import { FLOW_BASE_CHECK_COMMAND, FLOW_CHECK_REPORT_COMMAND, FLOW_CHECK_RESOLVE_COMMAND, FLOW_CHECK_RUN_COMMAND, FLOW_CHECK_SCRIPT, - FLOW_DROP_WORKING_FILES_COMMAND, FLOW_EXCLUDE_WORKING_FILES_COMMAND, FLOW_OPEN_CHANGE_COMMAND, FLOW_PREPARE_CHANGE_METADATA_COMMAND, FLOW_PUBLISH_CHECK_COMMAND, FLOW_REVIEW_BLOCKED_COMMAND, FLOW_VALIDATE_CHANGE_METADATA_COMMAND, + FLOW_DROP_WORKING_FILES_COMMAND, FLOW_EXCLUDE_WORKING_FILES_COMMAND, FLOW_OPEN_CHANGE_COMMAND, FLOW_PREPARE_CHANGE_METADATA_COMMAND, FLOW_PUBLISH_CHECK_COMMAND, FLOW_REPORT_REVIEW_FINDINGS_COMMAND, FLOW_REVIEW_BLOCKED_COMMAND, FLOW_REVIEW_FINDINGS_LIMIT, + FLOW_VALIDATE_CHANGE_METADATA_COMMAND, } from '../flow-workflows'; /** @@ -301,6 +302,49 @@ describe('FLOW_REVIEW_BLOCKED_COMMAND', () => { }); }); +/** + * The step that says why a failed review failed. The run outcome for + * done("step_failed") names no step and no finding (Cloud run f92bf832), so + * until done() carries a detail (AgentWorkforce/flows#542) this step's stdout + * is where the reason is recorded. review.md is agent-authored: the output + * must stay bounded, and like every other step it must exit 0. + */ +describe('FLOW_REPORT_REVIEW_FINDINGS_COMMAND', () => { + const report = (files: Record) => sh(FLOW_REPORT_REVIEW_FINDINGS_COMMAND, fixture(files)); + + it('prints the remaining findings from review.md', () => { + const { code, stdout } = report({ 'review.md': '## Findings\n\nOne P2 remains: cleanup can report success while an allocation stays invisible.\n' }); + expect(code).toBe(0); + expect(stdout).toContain('report-review-findings: review.clean absent; remaining findings from review.md:'); + expect(stdout).toContain('One P2 remains: cleanup can report success while an allocation stays invisible.'); + expect(stdout).not.toContain('cut at'); + }); + + it('bounds a long review and says it was cut', () => { + const review = '## Findings\n\n' + Array.from({ length: 400 }, (_, index) => `- P3 finding ${index}: ${'x'.repeat(60)}`).join('\n') + '\nTHE-LAST-LINE\n'; + const { code, stdout } = report({ 'review.md': review }); + expect(code).toBe(0); + expect(Buffer.byteLength(stdout)).toBeLessThanOrEqual(FLOW_REVIEW_FINDINGS_LIMIT); + expect(stdout).toContain('- P3 finding 0:'); + expect(stdout).not.toContain('THE-LAST-LINE'); + expect(stdout).toContain('the full review is in review-blocked.md.'); + }); + + it('drops terminal control characters and blank lines from the agent-written text', () => { + const { stdout } = report({ 'review.md': 'P1: \u001b[31mred\u001b[0m\n\n\n\nP2: next\n' }); + expect(stdout).not.toContain('\u001b'); + expect(stdout).toContain('P1: [31mred[0m\nP2: next'); + }); + + it('falls back to a fixed sentence when review.md is missing or empty', () => { + for (const files of [{}, { 'review.md': '' }, { 'review.md': '\n \n' }] as Record[]) { + const { code, stdout } = report(files); + expect(code).toBe(0); + expect(stdout.trim()).toBe('relayflow report-review-findings: review.clean absent and review.md missing or empty'); + } + }); +}); + /** * The check that decides whether there is anything to publish. Its single * stdout token is what the flow branches on, and getting it wrong either opens