fix(generation): make final-review-pass-gate failures attributable (not an opaque exit 1)

src/product/generation/final-review-gate.test.ts

@@ -0,0 +1,84 @@
+import { describe, expect, it } from 'vitest';
+
+import {
+  buildFinalReviewPassGateCommand,
+  GATE_BLOCKED_MARKER,
+  GATE_MISSING_MARKER_PREFIX,
+} from './final-review-gate.js';
+
+const ARTIFACTS = '.workflow-artifacts/generated/demo/update-last-week';
+
+function gate(overrides: { successMarker?: string } = {}): string {
+  return buildFinalReviewPassGateCommand({
+    artifactsDir: ARTIFACTS,
+    checks: [
+      {
+        presenceTest: `grep -qF RICKY_CHILD_CLAUDE_FINAL_FIX_READY '${ARTIFACTS}/claude-final-fix.md'`,
+        missingDetail: `${ARTIFACTS}/claude-final-fix.md is missing RICKY_CHILD_CLAUDE_FINAL_FIX_READY`,
+      },
+      {
+        presenceTest: `grep -qF RICKY_CHILD_CODEX_FINAL_FIX_READY '${ARTIFACTS}/codex-final-fix.md'`,
+        missingDetail: `${ARTIFACTS}/codex-final-fix.md is missing RICKY_CHILD_CODEX_FINAL_FIX_READY`,
+      },
+    ],
+    successMarker: overrides.successMarker ?? 'RICKY_CHILD_FRESH_EYES_LOOP_READY',
+  });
+}
+
+describe('buildFinalReviewPassGateCommand', () => {
+  it('checks the BLOCKED sentinel before any marker grep', () => {
+    const command = gate();
+    const blockedIdx = command.indexOf('BLOCKED_NO_COMMIT.md');
+    const firstGrepIdx = command.indexOf('grep -qF RICKY_CHILD_CLAUDE_FINAL_FIX_READY');
+    expect(blockedIdx).toBeGreaterThan(-1);
+    expect(firstGrepIdx).toBeGreaterThan(-1);
+    expect(blockedIdx).toBeLessThan(firstGrepIdx);
+  });
+
+  it('emits a distinct, greppable marker plus the agent evidence when blocked', () => {
+    const command = gate();
+    expect(command).toContain(`echo '${GATE_BLOCKED_MARKER}' >&2`);
+    expect(command).toContain(`cat '${ARTIFACTS}/BLOCKED_NO_COMMIT.md' >&2`);
+    // Distinct exit code so the failure is attributable, not a generic exit 1.
+    expect(command).toContain('exit 3');
+  });
+
+  it('makes each marker check quiet with an explicit missing-marker diagnostic', () => {
+    const command = gate();
+    // Quiet grep — matched lines must not leak into captured output and look
+    // like success while the command actually failed.
+    expect(command).toContain('grep -qF RICKY_CHILD_CLAUDE_FINAL_FIX_READY');
+    expect(command).not.toMatch(/grep -F RICKY_CHILD_CLAUDE_FINAL_FIX_READY(?!\w)/);
+    expect(command).toContain(
+      `${GATE_MISSING_MARKER_PREFIX}: ${ARTIFACTS}/claude-final-fix.md is missing RICKY_CHILD_CLAUDE_FINAL_FIX_READY`,
+    );
+    expect(command).toContain(
+      `${GATE_MISSING_MARKER_PREFIX}: ${ARTIFACTS}/codex-final-fix.md is missing RICKY_CHILD_CODEX_FINAL_FIX_READY`,
+    );
+  });
+
+  it('still echoes the success marker last so the gate can pass', () => {
+    const command = gate();
+    // shellQuote wraps the marker in single quotes for safe shell embedding.
+    expect(command.trimEnd().endsWith("echo 'RICKY_CHILD_FRESH_EYES_LOOP_READY'")).toBe(true);
+  });
+
+  it('shell-quotes the success marker (defends against future callers passing metacharacters)', () => {
+    const command = gate({ successMarker: 'DONE $(touch /tmp/pwned)' });
+    expect(command).toContain("echo 'DONE $(touch /tmp/pwned)'");
+    expect(command).not.toContain('echo DONE $(touch');
+  });
+
+  it('guards the blocked-evidence cat so a failing read does not short-circuit exit 3', () => {
+    const command = gate();
+    // The cat call must be inside an `if !` block (so set -e doesn't
+    // terminate the script if cat itself fails) followed by an
+    // unconditional `exit 3`.
+    expect(command).toMatch(/if ! cat .* >&2; then[\s\S]*?fi[\s\S]*?exit 3/);
+  });
+
+  it('does not leave a trailing bare `test ! -f` clause that fails opaquely', () => {
+    const command = gate();
+    expect(command).not.toContain('test ! -f');
+  });
+});

src/product/generation/final-review-gate.ts

@@ -0,0 +1,97 @@
+// Shared builder for the generated child-workflow `final-review-pass-gate`
+// command.
+//
+// Why this exists: the original gate joined marker greps and a
+// `test ! -f BLOCKED_NO_COMMIT.md` clause under `set -e` / `&&`. When an
+// agent deliberately wrote BLOCKED_NO_COMMIT.md (the "I cannot proceed
+// safely, do not commit" protocol), the gate aborted with a bare exit 1 and
+// the captured output showed only the *successful* marker greps — masking
+// the real cause. Operators saw "grep ... Command failed (exit code 1)"
+// with the success markers in stdout and assumed a grep-target mismatch,
+// and the auto-fix loop treated a deliberate "needs a human" signal as a
+// retryable INVALID_ARTIFACT, looping for hours.
+//
+// This builder produces a gate command that:
+//   1. Checks the BLOCKED sentinel FIRST and, if present, prints a distinct
+//      `RICKY_CHILD_BLOCKED_NO_COMMIT` marker plus the agent's evidence to
+//      stderr and exits with a dedicated code — so the failure is
+//      attributable and can be routed to escalation rather than retry.
+//   2. Runs each marker presence check quietly and, on failure, prints an
+//      explicit `RICKY_CHILD_GATE_MISSING_MARKER: <detail>` line — so a
+//      genuinely missing marker is diagnosable and never hidden behind a
+//      previous check's matched-line output.
+
+function shellQuote(value: string): string {
+  return `'${value.replace(/'/g, `'\\''`)}'`;
+}
+
+export interface GateMarkerCheck {
+  /**
+   * A shell expression that exits 0 (and produces no stdout) when the marker
+   * is present. Keep it quiet — matched lines must not leak into the gate's
+   * captured output.
+   */
+  presenceTest: string;
+  /**
+   * Human/diagnostic detail appended after `RICKY_CHILD_GATE_MISSING_MARKER: `
+   * when `presenceTest` fails.
+   */
+  missingDetail: string;
+}
+
+export interface FinalReviewPassGateOptions {
+  /** Directory holding the child workflow's review/fix artifacts. */
+  artifactsDir: string;
+  /** Ordered marker presence checks (claude first, then codex, etc.). */
+  checks: GateMarkerCheck[];
+  /** Token echoed on success to satisfy the gate's output assertion. */
+  successMarker: string;
+}
+
+export const GATE_BLOCKED_MARKER = 'RICKY_CHILD_BLOCKED_NO_COMMIT';
+export const GATE_MISSING_MARKER_PREFIX = 'RICKY_CHILD_GATE_MISSING_MARKER';
+
+/**
+ * Build the multi-line shell script for `final-review-pass-gate`. Returned as
+ * a single string so it can be embedded as a step `command` by either
+ * renderer regardless of how that renderer assembles its command.
+ */
+export function buildFinalReviewPassGateCommand(options: FinalReviewPassGateOptions): string {
+  const blockedPath = `${options.artifactsDir}/BLOCKED_NO_COMMIT.md`;
+  const lines: string[] = [
+    'set -e',
+    `if [ -f ${shellQuote(blockedPath)} ]; then`,
+    `  echo ${shellQuote(GATE_BLOCKED_MARKER)} >&2`,
+    `  echo ${shellQuote(
+      'Child workflow gate refused: an agent wrote BLOCKED_NO_COMMIT.md and did not produce a clean signoff. '
+      + 'This needs a human decision, not an auto-retry. Evidence:',
+    )} >&2`,
+    // `set -e` is active above. A failing `cat` (file removed between the
+    // `-f` check and this read, file not readable, etc.) would terminate
+    // the script before `exit 3` and regress blocked routing back to a
+    // generic non-attributable exit. Guard the cat so we always reach
+    // `exit 3`; emit a fallback marker on stderr if the evidence couldn't
+    // be read so the blocked routing is still observable.
+    `  if ! cat ${shellQuote(blockedPath)} >&2; then`,
+    `    echo ${shellQuote(`${GATE_BLOCKED_MARKER}: unable to read BLOCKED_NO_COMMIT.md evidence`)} >&2`,
+    '  fi',
+    '  exit 3',
+    'fi',
+  ];
+  for (const check of options.checks) {
+    lines.push(
+      `if ! { ${check.presenceTest}; }; then`,
+      `  echo ${shellQuote(`${GATE_MISSING_MARKER_PREFIX}: ${check.missingDetail}`)} >&2`,
+      '  exit 1',
+      'fi',
+    );
+  }
+  // Shell-quote the success marker for consistency with every other
+  // dynamic value emitted in this script. Current callers pass the safe
+  // constant `'RICKY_CHILD_FRESH_EYES_LOOP_READY'`, but this is an exported
+  // shared builder; preserving the quoting discipline prevents future
+  // callers from accidentally injecting shell metacharacters via the
+  // option.
+  lines.push(`echo ${shellQuote(options.successMarker)}`);
+  return lines.join('\n');
+}

src/product/generation/master-workflow-renderer.ts

@@ -6,6 +6,7 @@ import {
 } from '../../shared/constants.js';
 import { planMasterExecution, type ChildWorkflowPlan, type MasterExecutionPlan } from '../orchestration/index.js';
 import { deriveTestCommand } from './template-renderer.js';
+import { buildFinalReviewPassGateCommand } from './final-review-gate.js';
 import type {
   DeterministicGate,
   PatternDecision,
@@ -504,13 +505,20 @@ function childWorkflowSource(child: ChildWorkflowPlan, spec: NormalizedWorkflowS
     '    .step("final-review-pass-gate", {',
     '      type: "deterministic",',
     '      dependsOn: ["final-fix-codex"],',
-    `      command: ${literal([
-      'set -e',
-      `grep -F RICKY_CHILD_CLAUDE_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/claude-final-fix.md`)}`,
-      `grep -F RICKY_CHILD_CODEX_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/codex-final-fix.md`)}`,
-      `test ! -f ${shellQuote(`${artifactsDir}/BLOCKED_NO_COMMIT.md`)}`,
-      'echo RICKY_CHILD_FRESH_EYES_LOOP_READY',
-    ].join('\n'))},`,
+    `      command: ${literal(buildFinalReviewPassGateCommand({
+      artifactsDir,
+      checks: [
+        {
+          presenceTest: `grep -qF RICKY_CHILD_CLAUDE_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/claude-final-fix.md`)}`,
+          missingDetail: `${artifactsDir}/claude-final-fix.md is missing RICKY_CHILD_CLAUDE_FINAL_FIX_READY`,
+        },
+        {
+          presenceTest: `grep -qF RICKY_CHILD_CODEX_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/codex-final-fix.md`)}`,
+          missingDetail: `${artifactsDir}/codex-final-fix.md is missing RICKY_CHILD_CODEX_FINAL_FIX_READY`,
+        },
+      ],
+      successMarker: 'RICKY_CHILD_FRESH_EYES_LOOP_READY',
+    }))},`,
     '      captureOutput: true,',
     '      failOnError: true,',
     '    })',
@@ -521,8 +529,10 @@ function childWorkflowSource(child: ChildWorkflowPlan, spec: NormalizedWorkflowS
       'set -e',
       validationCommand,
       'git diff --name-only',
-      `grep -F RICKY_CHILD_CLAUDE_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/claude-final-fix.md`)}`,
-      `grep -F RICKY_CHILD_CODEX_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/codex-final-fix.md`)}`,
+      // Quiet greps with explicit diagnostics — a missing marker here must
+      // not be hidden behind the previous grep's matched-line output.
+      `if ! grep -qF RICKY_CHILD_CLAUDE_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/claude-final-fix.md`)}; then echo ${shellQuote(`RICKY_CHILD_GATE_MISSING_MARKER: ${artifactsDir}/claude-final-fix.md is missing RICKY_CHILD_CLAUDE_FINAL_FIX_READY`)} >&2; exit 1; fi`,
+      `if ! grep -qF RICKY_CHILD_CODEX_FINAL_FIX_READY ${shellQuote(`${artifactsDir}/codex-final-fix.md`)}; then echo ${shellQuote(`RICKY_CHILD_GATE_MISSING_MARKER: ${artifactsDir}/codex-final-fix.md is missing RICKY_CHILD_CODEX_FINAL_FIX_READY`)} >&2; exit 1; fi`,
       'echo RICKY_CHILD_FINAL_VALIDATION_READY',
     ].join('\n'))},`,
     '      captureOutput: true,',