From 5946926b94279819ce1aabc69eee19c513ea8433 Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 8 Sep 2026 14:40:32 -0400 Subject: [PATCH] fix(#4460): rework test to not depend on Tier 2's broken bash (#4461) A fresh code-review pass found the test's original approach (concatenate and execute Tier 1 + Tier 2 + Tier 3 verbatim, matching the issue's own reproduction) cannot run: Tier 2's own fence -- untouched by this diff -- is not currently parseable bash. Two unescaped `"` inside its embedded `node -e "..."` regex literal (`raw.replace(/^['"]|['"]$/g, '')`) terminate the outer double-quoted string early, which breaks bash's PARSE of the whole concatenated script even though Tier 2's body never executes under --files. Independently confirmed via manual extraction and execution before accepting the finding. This is a real, separately-filed, already-queued sibling issue (#4461, filed by #4460's own reporter specifically to avoid folding it in here) -- not fixed in this PR. Instead reworked the test to run only Tier 1 + Tier 3 verbatim, seeding the Tier-2-equivalent REVIEW_FILES state directly for the "without --files" case (documented in the module docblock, explaining why Tier 2 isn't sourced and pointing at #4461). Also fixed a nit from the same review pass: a code comment overstated Tier 2's guard as "immediately above" when it's ~150 lines away. Manually re-verified both test cases against a real git fixture with a GNU-realpath-compatible `realpath` (matching gsd-test's Linux bench -- this Mac's BSD realpath lacks the `-m` flag Tier 1 uses, a SEPARATE pre-existing portability gap surfaced during this check, masked on Linux CI, not touched by this fix) before re-running the full suite. Co-Authored-By: Claude Sonnet 5 --- gsd-core/workflows/code-review.md | 2 +- ...view-tier3-files-override-scoping.test.cjs | 49 ++++++++++++++----- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index eeedf96e3..7bf8b15bf 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -298,7 +298,7 @@ elif [ -z "$FILES_OVERRIDE" ] && [ -n "$DIFF_BASE" ]; then # reached anyway and the #2666 cross-check below appended the whole phase # diff onto an explicit user-supplied file list, contradicting line 144's # "Skip SUMMARY/git scoping entirely when --files is provided" and Tier 2's - # own --files guard immediately above. + # own --files guard (line 150). # #2666 cross-check: SUMMARY yielded a non-empty (possibly partial) scope. # Warn about — and add — any changed files the SUMMARY extractor did not surface, # so a partial result can no longer silently ship an incomplete review scope. diff --git a/tests/code-review-tier3-files-override-scoping.test.cjs b/tests/code-review-tier3-files-override-scoping.test.cjs index 8464b2bdb..5c8ee2866 100644 --- a/tests/code-review-tier3-files-override-scoping.test.cjs +++ b/tests/code-review-tier3-files-override-scoping.test.cjs @@ -14,9 +14,22 @@ * whole phase diff onto an explicit user-supplied file list. * * Mirrors the issue's own verified reproduction methodology: extract the - * Tier 1/2/3 fences VERBATIM from code-review.md (never reimplemented), - * set only the prerequisite variables, run against a real constructed git - * fixture. + * Tier 1 and Tier 3 fences VERBATIM from code-review.md (never + * reimplemented), set only the prerequisite variables, run against a real + * constructed git fixture. + * + * Tier 2's own fence is DELIBERATELY NOT extracted-and-executed here — a + * code-review pass on this fix found it is not currently parseable bash at + * all (two unescaped `"` characters inside its embedded `node -e "..."` + * regex literal terminate the outer double-quoted string early, breaking + * bash's parse of the WHOLE script even though Tier 2's body never + * executes under `--files`). That defect is real, already reported, and + * already queued as its own issue (#4461, filed separately by #4460's own + * reporter: "the Tier-2 SUMMARY-extraction fence is not parseable bash + * (same file, different defect)") — fixing it here would be exactly the + * scope creep the reporter took care to avoid. Until #4461 lands, the + * "without --files" case below seeds the REVIEW_FILES state Tier 2 would + * have produced directly, rather than sourcing Tier 2's broken fence. */ const { describe, test } = require('node:test'); @@ -80,27 +93,38 @@ function buildFixture(tmpDir) { } } -function runTiers(tmpDir, { filesOverride }) { +/** + * Runs Tier 1 (verbatim) then Tier 3 (verbatim) in sequence. `seedReviewFiles` + * stands in for what Tier 2 would have produced when `filesOverride` is unset + * (Tier 2 itself is not sourced — see the module docblock for why) — an empty + * array when omitted, matching Tier 2's own real behavior when no SUMMARY + * yields anything. + */ +function runTiers(tmpDir, { filesOverride, seedReviewFiles = [] }) { const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); const tier1 = extractFirstBashBlockAfter(content, '**Tier 1 — --files override', '**Tier 2 —'); - const tier2 = extractFirstBashBlockAfter(content, '**Tier 2 — SUMMARY.md extraction', '**Tier 3 —'); const tier3 = extractFirstBashBlockAfter(content, '**Tier 3 — Git diff fallback', '**Post-processing'); const filesArrayInit = filesOverride ? `FILES_ARRAY=(${filesOverride})` : 'FILES_ARRAY=()'; + const seedInit = seedReviewFiles.length + ? `REVIEW_FILES=(${seedReviewFiles.map((f) => `"${f}"`).join(' ')})` + : 'REVIEW_FILES=()'; const script = [ '#!/usr/bin/env bash', 'set -uo pipefail', `FILES_OVERRIDE="${filesOverride || ''}"`, filesArrayInit, - 'REVIEW_FILES=()', + tier1, + // Tier 1 unconditionally resets REVIEW_FILES=() when FILES_OVERRIDE is + // set; the seed only matters (and only applies) when it is not, exactly + // mirroring Tier 2 running in FILES_OVERRIDE's absence. + `if [ -z "$FILES_OVERRIDE" ]; then ${seedInit}; fi`, 'PHASE_DIR=".planning/phases/03-demo"', 'PADDED_PHASE="03"', 'LAST_REVIEW_COMMIT=""', - tier1, - tier2, tier3, 'printf \'%s\\n\' "${REVIEW_FILES[@]}"', ].join('\n'); @@ -145,15 +169,18 @@ describe('#4460: code-review.md Tier 3 does not widen an explicit --files overri } }); - test('without --files, the #2666 cross-check still widens a partial SUMMARY scope (no regression to the cross-check itself)', () => { + test('without --files, the #2666 cross-check still widens a partial (Tier-2-equivalent) scope (no regression to the cross-check itself)', () => { const tmpDir = fs.realpathSync(createTempDir('gsd-4460-')); try { buildFixture(tmpDir); - const files = runTiers(tmpDir, { filesOverride: '' }); + // seedReviewFiles stands in for Tier 2's real output (["src/alpha.js"], + // the file the fixture's SUMMARY lists) — see the module docblock for + // why Tier 2's own fence isn't sourced here. + const files = runTiers(tmpDir, { filesOverride: '', seedReviewFiles: ['src/alpha.js'] }); assert.deepEqual( files, ['src/alpha.js', 'src/beta.js', 'src/delta.js', 'src/epsilon.js', 'src/gamma.js'], - `without --files, the cross-check must still widen the partial SUMMARY scope, got: ${JSON.stringify(files)}`, + `without --files, the cross-check must still widen a partial scope, got: ${JSON.stringify(files)}`, ); } finally { cleanup(tmpDir);