fix(flows): say why software-factory ends step_failed on a failed review

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<typeof wor
   // found, so the step below still drafts the pull request and posts the
   // findings to it.
   const reviewBlockedCommand = ${JSON.stringify(FLOW_REVIEW_BLOCKED_COMMAND)};
+  const reportReviewFindingsCommand = ${JSON.stringify(FLOW_REPORT_REVIEW_FINDINGS_COMMAND)};
   let clean = false;
   for (let round = 0; round < ${workflow === 'traditional' ? 2 : 1}; round++) {
     await f.run("rm -f review.clean");
@@ -656,9 +693,14 @@ export function workflowCode(workflow: WorkflowId, agents: ReturnType<typeof wor
   // Unresolved feedback stops the flow short of approval.
   if (!clean) {
     await f.run(reviewBlockedCommand);
+    // report-review-findings: the run outcome says only that "its own checks
+    // 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();
     // Says only what is certain: the step above reports per branch whether it
     // could draft the pull request or comment on it.
-    console.error("The adversarial review did not pass. The findings are in review-blocked.md, and on the pull request if it could be reached. This branch is not approved.");
+    console.error("The adversarial review did not pass. The findings are in review-blocked.md, and on the pull request if it could be reached. This branch is not approved.\\n" + reviewFindings);
     return f.done("step_failed");
   }` });
   sections.push({ id: 'gate', code: `  // Require approving reviews and passing CI checks in GitHub branch rules.

web/lib/test/flow-onboarding.test.ts

@@ -1,6 +1,6 @@
 import { describe, expect, it } from 'vitest';
 import ts from 'typescript';
-import { FLOW_BASE_CHECK_COMMAND, FLOW_CHECK_BLOCKED_COMMAND, FLOW_CHECK_RUN_COMMAND, FLOW_DROP_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 } from '../flow-workflows';
+import { FLOW_BASE_CHECK_COMMAND, FLOW_CHECK_BLOCKED_COMMAND, FLOW_CHECK_RUN_COMMAND, FLOW_DROP_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_VALIDATE_CHANGE_METADATA_COMMAND } from '../flow-workflows';
 import { cloudBlockedReason, cloudConnectionsHref, DEFAULT_FACTORY, factorySource, isMarkdownOnly, MARKDOWN_ONLY_CLOUD_NOTE, readFactoryDraft, canContinue, primaryAgent, onboardingPath, accessibleOnboardingStep, type FactoryDraft } from '../flow-onboarding';
 import { localInput } from '../flow-local';
 
@@ -42,6 +42,7 @@ async function runFactory(clean: boolean[], _approved = true, issue = matchingIs
         if (command.endsWith(FLOW_BASE_CHECK_COMMAND)) return baseline;
         if (command.endsWith(FLOW_PUBLISH_CHECK_COMMAND)) return publish;
         if (command.endsWith(FLOW_VALIDATE_CHANGE_METADATA_COMMAND)) return 'valid';
+        if (command === FLOW_REPORT_REVIEW_FINDINGS_COMMAND) return 'relayflow report-review-findings: review.clean absent; remaining findings from review.md:\nOne P2 remains.\n';
         return command.startsWith('test -f') ? (clean[index++] ? 'yes' : 'no') : command.startsWith('mktemp') ? '/tmp/relay-prototypes.test' : command === 'git rev-parse HEAD' ? 'abc123' : '';
       },
       human: async () => { 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);

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<string, string>) => 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<string, string>[]) {
+      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