fix(#3190): commit review.md in --auto loop; fix report env var (#3434)

* fix(#3190): commit review.md in --auto loop; fix report env var

Three coupled defects in gsd-core/workflows/code-review-fix.md:

- The --auto re-review loop overwrote REVIEW.md each iteration but the
  single docs commit staged only REVIEW-FIX.md, so the committed REVIEW.md
  stayed at iteration 1 and contradicted the committed REVIEW-FIX.md. The
  --auto commit now stages the converged REVIEW.md alongside REVIEW-FIX.md
  (guarded on AUTO_MODE; non-auto single-pass runs unchanged).
- The two inline frontmatter validators (HAS_STATUS, FIX_FRONTMATTER)
  exported REVIEW_PATH into a node -e body that reads process.env.
  FIX_REPORT_PATH, so the status check was always empty and REVIEW-FIX.md
  was never committed. Both now export FIX_REPORT_PATH.
- On successful convergence the spent .iterN.md backups are removed so the
  phase directory is clean; they are retained on degradation for post-mortem.

Regression test: tests/code-review-fix-pipeline-regression.test.cjs.

* chore(#3190): set changeset pr to 3434

---------

Co-authored-by: sim <sim@local>

.changeset/clever-voles-swim.md

@@ -0,0 +1,5 @@
+---
+type: Fixed
+pr: 3434
+---
+**`/gsd:code-review-fix <phase> --auto` now commits the converged REVIEW.md alongside REVIEW-FIX.md and reliably commits REVIEW-FIX.md at all** — the --auto re-review loop overwrote REVIEW.md every iteration but the workflow's single docs commit staged only REVIEW-FIX.md, so the committed REVIEW.md stayed at iteration 1 and contradicted the committed REVIEW-FIX.md (and the converged REVIEW.md plus .iterN.md backups survived only as uncommitted working-tree state). Separately, the two inline frontmatter validators exported REVIEW_PATH into a node -e body that reads process.env.FIX_REPORT_PATH, so the status check was always empty and REVIEW-FIX.md was never committed (the user was wrongly told the agent produced malformed output). The validators now export FIX_REPORT_PATH, the --auto commit stages REVIEW.md too, and spent .iterN.md backups are removed on successful convergence (retained on degradation). Non-auto single-pass runs are unchanged. (#3190)

gsd-core/workflows/code-review-fix.md

@@ -256,7 +256,11 @@ if [ "$AUTO_MODE" = "true" ]; then
   # Total fix passes = MAX_ITERATIONS. Loop uses -lt (not -le) intentionally.
   ITERATION=1
   MAX_ITERATIONS=3
-  
+  # #3190: track whether the loop converged (re-review came back clean) vs
+  # degraded (hit the cap). Convergence determines whether the .iterN.md backups
+  # are spent scratch (cleaned below) or retained for post-mortem analysis.
+  CONVERGED=false
+
   while [ $ITERATION -lt $MAX_ITERATIONS ]; do
     ITERATION=$((ITERATION + 1))
     
@@ -315,6 +319,7 @@ ${AGENT_SKILLS_REVIEWER}")
     " 2>/dev/null)
     
     if [ "$NEW_STATUS" = "clean" ]; then
+      CONVERGED=true
       echo ""
       echo "✓ All issues resolved after iteration ${ITERATION}."
       break
@@ -353,14 +358,24 @@ ${AGENT_SKILLS_FIXER}")
     echo ""
     echo "⚠ Reached maximum iterations (${MAX_ITERATIONS}). Remaining issues documented in REVIEW-FIX.md."
   fi
+
+  # #3190: on convergence the .iterN.md backups are spent scratch — their
+  # stated purpose is post-mortem analysis "if iterations degrade", and
+  # convergence means no degradation. Remove them so the phase directory is
+  # clean (no dirty REVIEW.md or backup files after the run). They are RETAINED
+  # when the loop degraded (hit MAX_ITERATIONS / fixer failure) so the
+  # post-mortem trail survives. Backup CREATION (cp … .iterN.md) is unchanged.
+  if [ "$CONVERGED" = "true" ]; then
+    rm -f "${REVIEW_PATH%.md}.iter"*.md "${FIX_REPORT_PATH%.md}.iter"*.md 2>/dev/null || true
+  fi
 fi
 ```
 
 Key design decisions for --auto (addresses ALL review HIGH concerns):
 1. **Re-review scope**: Uses REVIEW_FILES_ARRAY from original REVIEW.md frontmatter, falling back to full phase scope. Scope is NOT lost between iterations. Uses portable while-read loop (bash 3.2+ compatible, handles spaces in paths).
 2. **Artifact semantics**: REVIEW.md is overwritten by each re-review (latest review state). REVIEW-FIX.md is overwritten by each fixer iteration (latest fix state with iteration count). There is ONE final version of each artifact, not per-iteration copies.
-   Backup files (.iterN.md) preserve history for post-mortem analysis if iterations degrade.
+   Backup files (.iterN.md) preserve history for post-mortem analysis if iterations degrade. On successful convergence (#3190) the backups are spent scratch and removed; on degradation (hit MAX_ITERATIONS / fixer failure) they are retained for post-mortem.
-3. **Commit timing**: Fix commits happen per-finding inside the agent. REVIEW-FIX.md is NOT committed until step 7 (after ALL iterations complete). Only ONE docs commit for REVIEW-FIX.md, not one per iteration.
+3. **Commit timing**: Fix commits happen per-finding inside the agent. REVIEW-FIX.md is NOT committed until step 7 (after ALL iterations complete). Only ONE docs commit, not one per iteration. In --auto that single commit also stages the converged REVIEW.md alongside REVIEW-FIX.md (#3190), so the two committed artifacts agree — the initial code-review commit held iteration-1 REVIEW.md content, and the --auto re-review loop overwrote it each iteration.
 </step>
 
 <step name="commit_fix_report">
@@ -369,7 +384,8 @@ After ALL iterations complete (or single pass in non-auto mode), validate and co
 ```bash
 if [ -f "${FIX_REPORT_PATH}" ]; then
   # Validate REVIEW-FIX.md has valid YAML frontmatter with status field
-  HAS_STATUS=$(REVIEW_PATH="${REVIEW_PATH}" node -e "
+  # #3190: export FIX_REPORT_PATH (the var the body reads), not REVIEW_PATH.
+  HAS_STATUS=$(FIX_REPORT_PATH="${FIX_REPORT_PATH}" node -e "
     const fs = require('fs');
     const content = fs.readFileSync(process.env.FIX_REPORT_PATH, 'utf-8');
     const match = content.replace(/\r\n/g, '\n').match(/^---\n([\s\S]*?)\n---/);
@@ -380,9 +396,19 @@ if [ -f "${FIX_REPORT_PATH}" ]; then
     echo "REVIEW-FIX.md created at ${FIX_REPORT_PATH}"
     
     if [ "$COMMIT_DOCS" = "true" ]; then
+      # #3190: --auto's re-review loop overwrote REVIEW.md each iteration
+      # (auto_iteration_loop), but the only prior REVIEW.md commit is the
+      # initial code-review pass (iteration-1 content). Stage the converged
+      # REVIEW.md alongside REVIEW-FIX.md in this single docs commit so the two
+      # committed artifacts agree. Non-auto single-pass runs never rewrite
+      # REVIEW.md, so it is left untouched (guarded on AUTO_MODE).
+      COMMIT_FILES=("${FIX_REPORT_PATH}")
+      if [ "$AUTO_MODE" = "true" ] && [ -f "${REVIEW_PATH}" ]; then
+        COMMIT_FILES+=("${REVIEW_PATH}")
+      fi
       gsd_run query commit \
         "docs(${PADDED_PHASE}): add code review fix report" \
-        --files "${FIX_REPORT_PATH}"
+        --files "${COMMIT_FILES[@]}"
     fi
   else
     echo "Warning: REVIEW-FIX.md has invalid frontmatter (no status field). Not committing."
@@ -426,7 +452,8 @@ Extract frontmatter fields:
 
 ```bash
 # Extract only the YAML frontmatter block (between first two --- lines)
-FIX_FRONTMATTER=$(REVIEW_PATH="${REVIEW_PATH}" node -e "
+# #3190: export FIX_REPORT_PATH (the var the body reads), not REVIEW_PATH.
+FIX_FRONTMATTER=$(FIX_REPORT_PATH="${FIX_REPORT_PATH}" node -e "
   const fs = require('fs');
   const content = fs.readFileSync(process.env.FIX_REPORT_PATH, 'utf-8');
   const match = content.replace(/\r\n/g, '\n').match(/^---\n([\s\S]*?)\n---/);

scripts/lint-allow-test-rule-refs.ceiling.json

@@ -1,4 +1,4 @@
 {
-  "maxFiles": 299,
+  "maxFiles": 300,
   "grace": 3
 }

tests/code-review-fix-pipeline-regression.test.cjs

@@ -0,0 +1,185 @@
+// allow-test-rule: source-text-is-the-product (see #3190)
+// The workflow .md IS the product: its embedded bash/node snippets are loaded
+// and executed verbatim by the agent host at runtime. Testing that the deployed
+// commit-fix step exports the env var its own inline `node -e` body reads, and
+// that the docs commit stages the converged REVIEW.md alongside REVIEW-FIX.md,
+// asserts the deployed contract — not an implementation detail. There is no
+// runtime API that exposes these inlined snippets; the text IS the spec.
+//
+// Pattern mirrors tests/code-review-pipeline-regression.test.cjs: a behavioral
+// run of the inline parser body (via process-seam) PLUS docs-parity on the
+// workflow text. See .gsd/bug/fix-3190-code-review-fix-auto-rewrite-review/.
+
+'use strict';
+
+const { describe, test } = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('node:fs');
+const path = require('node:path');
+const { runNode } = require('./helpers/process-seam.cjs');
+const { createTempDir, cleanup } = require('./helpers.cjs');
+
+const ROOT = path.resolve(__dirname, '..');
+const WORKFLOW_PATH = path.join(ROOT, 'gsd-core', 'workflows', 'code-review-fix.md');
+
+// ---------------------------------------------------------------------------
+// The FIX_REPORT frontmatter validator body, mirrored from code-review-fix.md's
+// commit_fix_report HAS_STATUS inline `node -e` script. If those lines change,
+// this must be updated in tandem (the docs-parity assertions below catch a
+// mismatch at the env-name level).
+//
+// Bug (#3190): the deployed workflow launches this body with REVIEW_PATH
+// exported but the body reads process.env.FIX_REPORT_PATH — so at runtime the
+// readFileSync throws and HAS_STATUS silently becomes "". The behavioral tests
+// below prove the BODY is sound when FIX_REPORT_PATH is wired, and that the
+// mis-wiring is the sole failure mode.
+// ---------------------------------------------------------------------------
+const VALIDATOR_BODY = `
+  const fs = require('fs');
+  const content = fs.readFileSync(process.env.FIX_REPORT_PATH, 'utf-8');
+  const match = content.replace(/\\r\\n/g, '\\n').match(/^---\\n([\\s\\S]*?)\\n---/);
+  if (match && /status:/.test(match[1])) { console.log('valid'); } else { console.log('invalid'); }
+`;
+
+const VALID_FIX_REPORT = [
+  '---',
+  'status: all_fixed',
+  'findings_in_scope: 3',
+  'fixed: 3',
+  'skipped: 0',
+  'iteration: 2',
+  '---',
+  '',
+  'All findings resolved across 2 iterations.',
+].join('\n');
+
+/** Slice the workflow text of a single <step name="...">…</step> region. */
+function stepRegion(src, name) {
+  const open = src.indexOf(`<step name="${name}">`);
+  assert.notStrictEqual(open, -1, `step "${name}" not found in workflow`);
+  const close = src.indexOf('</step>', open);
+  assert.notStrictEqual(close, -1, `step "${name}" has no closing </step>`);
+  return src.slice(open, close + '</step>'.length);
+}
+
+describe('#3190 — code-review-fix --auto REVIEW.md commit + FIX_REPORT_PATH env wiring', () => {
+  // -------------------------------------------------------------------------
+  // Criterion 3 — behavioral: the validator body correctly detects a valid
+  // status field WHEN the env var it reads is actually exported.
+  // -------------------------------------------------------------------------
+  test('T1 — validator body prints "valid" for a well-formed REVIEW-FIX.md when FIX_REPORT_PATH is set', () => {
+    const dir = createTempDir();
+    const report = path.join(dir, '05-REVIEW-FIX.md');
+    fs.writeFileSync(report, VALID_FIX_REPORT, 'utf8');
+    try {
+      const res = runNode(['-e', VALIDATOR_BODY], {
+        env: { ...process.env, FIX_REPORT_PATH: report },
+      });
+      assert.strictEqual(res.exitCode, 0, `validator should exit 0; stderr:\n${res.stderr}`);
+      assert.strictEqual(res.stdout.trim(), 'valid');
+    } finally {
+      cleanup(dir);
+    }
+  });
+
+  // -------------------------------------------------------------------------
+  // Criterion 3 — behavioral bug demo: the SAME body, with FIX_REPORT_PATH
+  // unset (exactly what today's `REVIEW_PATH=`-only export produces in the
+  // subshell), throws inside readFileSync → stdout is empty → HAS_STATUS="" →
+  // the "invalid frontmatter — Not committing" branch fires unconditionally.
+  // This proves the env-name coupling is load-bearing and that the body is not
+  // the culprit.
+  // -------------------------------------------------------------------------
+  test('T2 — validator body does NOT print "valid" when FIX_REPORT_PATH is unset (the mis-wiring)', () => {
+    const env = { ...process.env };
+    delete env.FIX_REPORT_PATH;
+    delete env.REVIEW_PATH;
+    const res = runNode(['-e', VALIDATOR_BODY], { env });
+    // The throw crashes node (non-zero), stdout has no "valid" — exactly the
+    // silent empty string the 2>/dev/null command substitution yields in bash.
+    assert.ok(
+      !res.stdout.includes('valid'),
+      `stdout should not contain "valid" when FIX_REPORT_PATH is unset; got: ${res.stdout}`,
+    );
+  });
+
+  // -------------------------------------------------------------------------
+  // Criterion 3 — docs-parity: the commit_fix_report HAS_STATUS validator must
+  // export FIX_REPORT_PATH (the var its body reads), not REVIEW_PATH.
+  // -------------------------------------------------------------------------
+  test('T3 — commit_fix_report HAS_STATUS exports FIX_REPORT_PATH, not REVIEW_PATH', () => {
+    const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
+    const region = stepRegion(src, 'commit_fix_report');
+    assert.ok(
+      region.includes('HAS_STATUS=$(FIX_REPORT_PATH="${FIX_REPORT_PATH}" node -e'),
+      'HAS_STATUS must launch node with FIX_REPORT_PATH exported to match the process.env.FIX_REPORT_PATH its body reads',
+    );
+    assert.ok(
+      !region.includes('HAS_STATUS=$(REVIEW_PATH="${REVIEW_PATH}" node -e'),
+      'HAS_STATUS must not export REVIEW_PATH into a script that reads process.env.FIX_REPORT_PATH',
+    );
+  });
+
+  // -------------------------------------------------------------------------
+  // Criterion 3 — docs-parity: the present_results FIX_FRONTMATTER extractor
+  // must export FIX_REPORT_PATH, not REVIEW_PATH (else every summary field is
+  // blank).
+  // -------------------------------------------------------------------------
+  test('T4 — present_results FIX_FRONTMATTER exports FIX_REPORT_PATH, not REVIEW_PATH', () => {
+    const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
+    const region = stepRegion(src, 'present_results');
+    assert.ok(
+      region.includes('FIX_FRONTMATTER=$(FIX_REPORT_PATH="${FIX_REPORT_PATH}" node -e'),
+      'FIX_FRONTMATTER must launch node with FIX_REPORT_PATH exported to match the process.env.FIX_REPORT_PATH its body reads',
+    );
+    assert.ok(
+      !region.includes('FIX_FRONTMATTER=$(REVIEW_PATH="${REVIEW_PATH}" node -e'),
+      'FIX_FRONTMATTER must not export REVIEW_PATH into a script that reads process.env.FIX_REPORT_PATH',
+    );
+  });
+
+  // -------------------------------------------------------------------------
+  // Criterion 1 — docs-parity: in --auto the single docs commit must stage the
+  // converged REVIEW.md alongside REVIEW-FIX.md, gated on AUTO_MODE (non-auto
+  // single-pass runs never rewrite REVIEW.md and stay out of scope).
+  // -------------------------------------------------------------------------
+  test('T5 — commit_fix_report stages REVIEW.md alongside REVIEW-FIX.md when AUTO_MODE', () => {
+    const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
+    const region = stepRegion(src, 'commit_fix_report');
+    assert.ok(
+      region.includes('AUTO_MODE'),
+      'commit_fix_report must be aware of AUTO_MODE (only --auto rewrites REVIEW.md)',
+    );
+    assert.ok(
+      region.includes('REVIEW_PATH') && region.includes('COMMIT_FILES'),
+      'commit_fix_report must build a COMMIT_FILES list that includes REVIEW_PATH in --auto',
+    );
+    assert.ok(
+      region.includes('--files "${COMMIT_FILES[@]}"'),
+      'the docs commit must stage the assembled COMMIT_FILES array (REVIEW-FIX.md [+ REVIEW.md in --auto])',
+    );
+  });
+
+  // -------------------------------------------------------------------------
+  // Criterion 2 — docs-parity: on successful convergence the spent .iterN.md
+  // backups are removed so the phase directory is clean. Backup CREATION is
+  // unchanged (out of scope); they are retained when the loop degrades.
+  // -------------------------------------------------------------------------
+  test('T6 — auto_iteration_loop removes spent .iterN.md backups on convergence', () => {
+    const src = fs.readFileSync(WORKFLOW_PATH, 'utf8');
+    const region = stepRegion(src, 'auto_iteration_loop');
+    assert.ok(
+      region.includes('CONVERGED'),
+      'auto_iteration_loop must track a CONVERGED flag distinguishing convergence from degradation',
+    );
+    assert.ok(
+      /rm -f[\s\S]*?\.iter[\s\S]*?\*[\s\S]*?\.md/.test(region),
+      'on convergence the loop must remove spent .iterN.md backups (rm -f … .iter*.md)',
+    );
+    // Backups are still CREATED before each overwrite (the unchanged mechanism).
+    assert.ok(
+      region.includes('.iter${ITERATION}.md'),
+      'backup creation (cp … .iter${ITERATION}.md) must remain intact',
+    );
+  });
+});

tests/emitted-drift-acks/3190-code-review-fix-auto-rewrite-review.json

@@ -0,0 +1,6 @@
+{
+  "version": 1,
+  "paths": {
+    "code-review-fix.md": "#3190: the --auto convergence loop rewrote REVIEW.md every re-review iteration but never recommitted it (committed REVIEW.md stayed at iteration 1, contradicting the committed REVIEW-FIX.md), and the commit-fix step's two inline frontmatter validators exported REVIEW_PATH into a node -e body that reads process.env.FIX_REPORT_PATH — so HAS_STATUS/FIX_FRONTMATTER were always empty and REVIEW-FIX.md was never committed at all. The fix renames the exported env var to FIX_REPORT_PATH at both validator sites, stages the converged REVIEW.md alongside REVIEW-FIX.md in the single --auto docs commit (guarded on AUTO_MODE so non-auto single-pass runs are untouched), and removes the spent .iterN.md backups on successful convergence (retained on degradation for post-mortem). Growth is the deployed contract for those three coupled fixes plus inline rationale comments; the regression test is tests/code-review-fix-pipeline-regression.test.cjs."
+  }
+}