diff --git a/.changeset/clever-voles-swim.md b/.changeset/clever-voles-swim.md new file mode 100644 index 000000000..56bf634fc --- /dev/null +++ b/.changeset/clever-voles-swim.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3434 +--- +**`/gsd:code-review-fix --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) diff --git a/gsd-core/workflows/code-review-fix.md b/gsd-core/workflows/code-review-fix.md index 9db67f98c..6a0a902d5 100644 --- a/gsd-core/workflows/code-review-fix.md +++ b/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. -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. + 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, 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. @@ -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---/); diff --git a/scripts/lint-allow-test-rule-refs.ceiling.json b/scripts/lint-allow-test-rule-refs.ceiling.json index 28a04739f..da43d9eb0 100644 --- a/scripts/lint-allow-test-rule-refs.ceiling.json +++ b/scripts/lint-allow-test-rule-refs.ceiling.json @@ -1,4 +1,4 @@ { - "maxFiles": 299, + "maxFiles": 300, "grace": 3 } diff --git a/tests/code-review-fix-pipeline-regression.test.cjs b/tests/code-review-fix-pipeline-regression.test.cjs new file mode 100644 index 000000000..da17e8563 --- /dev/null +++ b/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 … region. */ +function stepRegion(src, name) { + const open = src.indexOf(``); + assert.notStrictEqual(open, -1, `step "${name}" not found in workflow`); + const close = src.indexOf('', open); + assert.notStrictEqual(close, -1, `step "${name}" has no closing `); + return src.slice(open, close + ''.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', + ); + }); +}); diff --git a/tests/emitted-drift-acks/3190-code-review-fix-auto-rewrite-review.json b/tests/emitted-drift-acks/3190-code-review-fix-auto-rewrite-review.json new file mode 100644 index 000000000..f22f09126 --- /dev/null +++ b/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." + } +}