From b1c78f0d2e94f5ab5a364589c5cd82dca6be189c Mon Sep 17 00:00:00 2001 From: sim Date: Tue, 8 Sep 2026 14:29:57 -0400 Subject: [PATCH] fix(#4460): gate Tier 3's #2666 cross-check on FILES_OVERRIDE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit code-review.md states (line 144) "Skip SUMMARY/git scoping entirely when --files is provided." Tier 2 honors this via `if [ -z "$FILES_OVERRIDE" ]`, but Tier 3's #2666 SUMMARY/diff cross-check had no FILES_OVERRIDE reference at all -- reached via `elif [ -n "$DIFF_BASE" ]` whenever REVIEW_FILES was already non-empty (true under --files, since Tier 1 fills it), so it silently appended the whole phase's changed files onto an explicit user-supplied file list. --files is documented as the highest-precedence scoping tier (D-08) and is the flag Tier 3's own fail-closed path recommends when no reliable diff base is found; a user narrowing a review to two files silently got the whole phase instead, and the reviewer agent spent its budget on files nobody asked about. Gated the elif on the same condition Tier 2 already uses: elif [ -z "$FILES_OVERRIDE" ] && [ -n "$DIFF_BASE" ]; then The issue's own narrowest suggested form, reasoned through against two alternatives (wrapping the whole Tier-3 fence, or changing the stated invariant instead) -- both explicitly rejected there for good reasons concurred with after reading the surrounding code. Added tests/code-review-tier3-files-override-scoping.test.cjs, mirroring the issue's own verified reproduction methodology: extracts the Tier 1/2/3 fences VERBATIM from code-review.md (never reimplemented) and runs them against a real constructed git fixture matching the issue's own scenario exactly (5 files, a SUMMARY listing only 1). Confirms --files stays scoped to exactly the requested file, and separately confirms the #2666 cross-check still widens a genuinely partial SUMMARY scope when --files is absent (proving this is a gate, not a blanket disable). Emitted-Drift-Ack-Growth: code-review.md — #4460 gates the Tier-3 #2666 cross-check on FILES_OVERRIDE, matching Tier 2's own guard, net +453 bytes Co-Authored-By: Claude Sonnet 5 --- .changeset/lively-quails-forage.md | 5 + gsd-core/workflows/code-review.md | 8 +- ...view-tier3-files-override-scoping.test.cjs | 162 ++++++++++++++++++ 3 files changed, 174 insertions(+), 1 deletion(-) create mode 100644 .changeset/lively-quails-forage.md create mode 100644 tests/code-review-tier3-files-override-scoping.test.cjs diff --git a/.changeset/lively-quails-forage.md b/.changeset/lively-quails-forage.md new file mode 100644 index 000000000..ab25f2b9e --- /dev/null +++ b/.changeset/lively-quails-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 0 +--- +**`/gsd-code-review --files` no longer silently widens back to the whole phase** — Tier 3's SUMMARY/diff cross-check ran regardless of an explicit `--files` override, appending the rest of the phase's changed files onto a scope the user had deliberately narrowed. diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 3aa2b5407..eeedf96e3 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -292,7 +292,13 @@ if [ ${#REVIEW_FILES[@]} -eq 0 ]; then echo "Warning: No phase commits found for '${PADDED_PHASE}'. Cannot determine reliable diff scope." echo "Use --files flag to specify files explicitly: /gsd:code-review ${PHASE_ARG} --files=file1,file2,..." fi -elif [ -n "$DIFF_BASE" ]; then +elif [ -z "$FILES_OVERRIDE" ] && [ -n "$DIFF_BASE" ]; then + # #4460: gated on FILES_OVERRIDE being unset — without this, REVIEW_FILES is + # already non-empty under --files (Tier 1 filled it), so this elif was + # 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. # #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 new file mode 100644 index 000000000..8464b2bdb --- /dev/null +++ b/tests/code-review-tier3-files-override-scoping.test.cjs @@ -0,0 +1,162 @@ +// allow-test-rule: source-text-is-the-product (see #4460) +// Workflow markdown is the installed orchestration contract — this file's +// text IS what the reviewer flow runs at runtime. + +'use strict'; + +/** + * Regression coverage for #4460: code-review.md states (line 144) "Skip + * SUMMARY/git scoping entirely when --files is provided." Tier 2 honors + * this via `if [ -z "$FILES_OVERRIDE" ]`, but Tier 3's `#2666` cross-check + * had no `FILES_OVERRIDE` reference at all — reached via `elif [ -n + * "$DIFF_BASE" ]` whenever `REVIEW_FILES` was already non-empty (true + * under `--files`, since Tier 1 fills it), so it silently appended the + * 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. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'code-review.md'); + +/** + * Extract the FIRST ```bash fence appearing after `startAnchor` and before + * `stopAnchor` (or end of file when `stopAnchor` is null). + */ +function extractFirstBashBlockAfter(content, startAnchor, stopAnchor) { + const start = content.indexOf(startAnchor); + assert.ok(start !== -1, `code-review.md must contain the anchor "${startAnchor}"`); + const stop = stopAnchor ? content.indexOf(stopAnchor, start + startAnchor.length) : content.length; + assert.ok(!stopAnchor || stop !== -1, `code-review.md must contain the anchor "${stopAnchor}" after "${startAnchor}"`); + const region = content.slice(start, stop); + + const fenceStart = region.indexOf('```bash'); + assert.ok(fenceStart !== -1, `no \`\`\`bash fence found between "${startAnchor}" and its stop anchor`); + const fenceEnd = region.indexOf('```', fenceStart + '```bash'.length); + assert.ok(fenceEnd !== -1, `unterminated \`\`\`bash fence after "${startAnchor}"`); + return region.slice(fenceStart + '```bash'.length, fenceEnd); +} + +function seedFixtureRepo(dir) { + gitOrThrow(['init', '-q'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 't@example.com'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'T'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +function writeAndCommit(dir, relPath, content, message) { + const abs = path.join(dir, relPath); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + gitOrThrow(['add', '-A'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['commit', '-q', '-m', message], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +// Matches the issue's own fixture exactly: a phase dir with one SUMMARY +// listing only src/alpha.js, and 5 commits adding src/{alpha,beta,gamma, +// delta,epsilon}.js. +function buildFixture(tmpDir) { + seedFixtureRepo(tmpDir); + writeAndCommit(tmpDir, 'README.md', '# init\n', 'chore: init'); + writeAndCommit( + tmpDir, + '.planning/phases/03-demo/03-01-SUMMARY.md', + '---\nkey_files:\n created:\n - src/alpha.js\n---\n# Summary\n', + 'feat(03-01): phase 3 plan 1', + ); + for (const name of ['alpha', 'beta', 'gamma', 'delta', 'epsilon']) { + writeAndCommit(tmpDir, `src/${name}.js`, `${name}\n`, `feat(03-01): add ${name}`); + } +} + +function runTiers(tmpDir, { filesOverride }) { + 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 script = [ + '#!/usr/bin/env bash', + 'set -uo pipefail', + `FILES_OVERRIDE="${filesOverride || ''}"`, + filesArrayInit, + 'REVIEW_FILES=()', + 'PHASE_DIR=".planning/phases/03-demo"', + 'PADDED_PHASE="03"', + 'LAST_REVIEW_COMMIT=""', + tier1, + tier2, + tier3, + 'printf \'%s\\n\' "${REVIEW_FILES[@]}"', + ].join('\n'); + + const scriptPath = path.join(tmpDir, '.tier-script.sh'); + fs.writeFileSync(scriptPath, script); + + const output = execFileSync('bash', [scriptPath], { + cwd: tmpDir, + encoding: 'utf8', + timeout: GIT_FIXTURE_TIMEOUT_MS, + }); + return output.split('\n').map((l) => l.trim()).filter(Boolean).sort(); +} + +describe('#4460: code-review.md Tier 3 does not widen an explicit --files override', () => { + const workflowContent = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const tier3Fence = extractFirstBashBlockAfter(workflowContent, '**Tier 3 — Git diff fallback', '**Post-processing'); + + test('the #2666 cross-check elif references FILES_OVERRIDE (the gate exists)', () => { + const crossCheckIdx = tier3Fence.indexOf('#2666 cross-check'); + assert.ok(crossCheckIdx !== -1, 'Tier 3 fence must contain the #2666 cross-check comment'); + const elifLine = tier3Fence.slice(0, crossCheckIdx).split('\n').filter((l) => l.trim().startsWith('elif')).pop(); + assert.ok( + elifLine && elifLine.includes('FILES_OVERRIDE'), + `the elif guarding the #2666 cross-check must reference FILES_OVERRIDE, got: ${JSON.stringify(elifLine)}`, + ); + }); + + test('real execution: --files=src/alpha.js stays scoped to exactly that file (issue #4460 repro)', () => { + const tmpDir = fs.realpathSync(createTempDir('gsd-4460-')); + try { + buildFixture(tmpDir); + const files = runTiers(tmpDir, { filesOverride: 'src/alpha.js' }); + assert.deepEqual( + files, + ['src/alpha.js'], + `--files override must not be widened by Tier 3's cross-check, got: ${JSON.stringify(files)}`, + ); + } finally { + cleanup(tmpDir); + } + }); + + test('without --files, the #2666 cross-check still widens a partial SUMMARY scope (no regression to the cross-check itself)', () => { + const tmpDir = fs.realpathSync(createTempDir('gsd-4460-')); + try { + buildFixture(tmpDir); + const files = runTiers(tmpDir, { filesOverride: '' }); + 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)}`, + ); + } finally { + cleanup(tmpDir); + } + }); +});