From 6e0bc501427835f24a2abc5a16c9df17b843ec49 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Thu, 30 Jul 2026 22:27:32 -0400 Subject: [PATCH] fix(#2666): code-review scopes root-level + extensionless build files, cross-checks against git diff (#2895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test(#2666): add regression + docs-parity guards for code-review file scoper The Tier-2 SUMMARY.md extractor dropped every repository-root file (no `/`) and every extensionless build file (Dockerfile/Makefile/etc.) via an AND-joined predicate. Adds behavioral tests against the pure-function mirror plus docs-parity structural guards that bind the shipped workflow .md to the fix. RED: the docs-parity guards fail against the pre-fix shipped predicate. * fix(#2666): accept root-level + extensionless build files in code-review scope; intersect-and-warn Two coordinated edits to gsd-core/workflows/code-review.md compute_file_scope: (A) Tier-2 SUMMARY extractor: replace the AND-joined predicate `/\\//.test(raw) && /\\.[A-Za-z0-9]+$/.test(raw)` (which required BOTH a directory separator AND a trailing extension, silently dropping every root-level file and every extensionless build file) with a relaxed predicate that accepts any path with a trailing extension OR a known extensionless build basename (Dockerfile/Containerfile/Makefile/Justfile/Procfile). (B) Tier-3: convert the eq-zero git-diff gate into an intersect-and-warn — whenever a reliable diff base is available, cross-check the SUMMARY scope against `git diff --name-only` and warn about (then add) any changed files the SUMMARY extractor did not surface. Portable (bash 3.2, no associative arrays) so a partial SUMMARY result can no longer silently ship an incomplete review scope. * chore(#2666): changeset fragment * fix(#2666): use exact whole-line matching (grep -Fxq) in Tier-3 cross-check Adversarial review found the unanchored `case "$IN_SCOPE" in *"$file"$\\n*` substring membership test would false-match: a root-level `Dockerfile` in the diff substring-matches an already-scoped `docker/Dockerfile`, silently skipping it — reintroducing the exact class of silent-scope-loss bug this PR fixes. Switch to `grep -Fxq` (exact whole-line match). Add docs-parity guard for exact matching + the basename-collision regression. * fix(#2666): resolve gsd-test failures — paraphrase predicate in comment, ack code-review.md growth gsd-test caught 3 issues on 7469c3f22: 1. The docs-parity guard fired on the .md COMMENT which restated the buggy predicate verbatim — paraphrase the comment so it no longer contains the exact string the guard detects. 2. Cascade subtest failure from #1. 3. emitted-attribution: code-review.md grew 2814 bytes — acknowledge the deliberate #2666 growth in tests/emitted-drift-ack.json. * chore(#2666): backfill changeset PR number 2895 --------- Co-authored-by: Test --- .changeset/witty-pumas-wave.md | 5 + gsd-core/workflows/code-review.md | 81 ++++++-- .../code-review-pipeline-regression.test.cjs | 192 +++++++++++++++++- tests/emitted-drift-ack.json | 3 + 4 files changed, 263 insertions(+), 18 deletions(-) create mode 100644 .changeset/witty-pumas-wave.md diff --git a/.changeset/witty-pumas-wave.md b/.changeset/witty-pumas-wave.md new file mode 100644 index 000000000..42e29b32f --- /dev/null +++ b/.changeset/witty-pumas-wave.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2895 +--- +**Code-review now scopes repository-root and extensionless build files (Dockerfile, Makefile, .gitlab-ci.yml, renovate.json, AGENTS.md)** — the SUMMARY.md file extractor no longer silently drops every root-level path and every extensionless build file, and a partial SUMMARY scope is now cross-checked against `git diff` with a warning naming any changed files it missed. (#2666) diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 774854cc0..27f72edbb 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -185,7 +185,21 @@ if [ -z "$FILES_OVERRIDE" ]; then raw = raw.replace(/^['"]|['"]$/g, ''); raw = raw.replace(/\s+\([^)]*\)\s*$/, ''); raw = raw.split(/\s+—\s/)[0].trim(); - if (/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)) { + // #2666: accept root-level paths (no `/`) and known extensionless build + // files, not only nested paths with a trailing extension. The pre-fix + // guard required BOTH a directory separator AND a trailing dot-extension, + // which silently dropped every repository-root file (Dockerfile, + // renovate.json, AGENTS.md, package.json, .gitlab-ci.yml, …) and every + // extensionless build file anywhere in the tree (**/Dockerfile, **/Makefile). + // Prose bullets are rejected by the known-filename / has-extension + // distinction, with the post-processing existence check (`[ -f ]`) as a + // backstop — a prose string is never a real file on disk. + const KNOWN_EXTENSIONLESS_BUILD_FILES = new Set([ + 'dockerfile', 'containerfile', 'makefile', 'justfile', 'procfile', + ]); + const hasExtension = /\.[A-Za-z0-9]+$/.test(raw); + const basename = raw.split('/').pop().toLowerCase(); + if (hasExtension || KNOWN_EXTENSIONLESS_BUILD_FILES.has(basename)) { files.push(raw); } } @@ -210,38 +224,73 @@ if [ -z "$FILES_OVERRIDE" ]; then fi ``` -**Tier 3 — Git diff fallback (per D-02):** +**Tier 3 — Git diff fallback (per D-02) and SUMMARY/diff cross-check (per #2666):** -If no SUMMARY.md files found OR no files extracted from them: +If no SUMMARY.md files found OR no files extracted from them, fall back to the git diff. +Additionally, whenever a reliable diff base is available, cross-check the SUMMARY scope +against the diff and warn about (then add) any changed files the SUMMARY extractor did not +surface — so a partial SUMMARY result can no longer silently mask the rest of the phase. ```bash +# Compute diff base from phase commits — fail closed if no reliable base found +PHASE_COMMITS=$(git log --oneline --all --grep="${PADDED_PHASE}" --format="%H" 2>/dev/null) +DIFF_BASE="" +if [ -n "$PHASE_COMMITS" ]; then + DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1)^ + # Verify the parent commit exists (first commit in repo has no parent) + if ! git rev-parse "${DIFF_BASE}" >/dev/null 2>&1; then + DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1) + fi +fi + if [ ${#REVIEW_FILES[@]} -eq 0 ]; then - # Compute diff base from phase commits — fail closed if no reliable base found - PHASE_COMMITS=$(git log --oneline --all --grep="${PADDED_PHASE}" --format="%H" 2>/dev/null) - - if [ -n "$PHASE_COMMITS" ]; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1)^ - - # Verify the parent commit exists (first commit in repo has no parent) - if ! git rev-parse "${DIFF_BASE}" >/dev/null 2>&1; then - DIFF_BASE=$(echo "$PHASE_COMMITS" | tail -1) - fi - + # Full git-diff fallback (per D-02): SUMMARY scoping yielded nothing. + if [ -n "$DIFF_BASE" ]; then # Run git diff with specific exclusions (per D-03) DIFF_FILES=$(git diff --name-only "${DIFF_BASE}..HEAD" -- . \ ':!.planning/' ':!ROADMAP.md' ':!STATE.md' \ ':!*-SUMMARY.md' ':!*-VERIFICATION.md' ':!*-PLAN.md' \ ':!package-lock.json' ':!yarn.lock' ':!Gemfile.lock' ':!poetry.lock' 2>/dev/null) - + while IFS= read -r file; do [ -n "$file" ] && REVIEW_FILES+=("$file") done <<< "$DIFF_FILES" - + echo "File scope: ${#REVIEW_FILES[@]} files from git diff (base: ${DIFF_BASE})" else # Fail closed — no reliable diff base found. Do not use arbitrary HEAD~N. 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 + # #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_FILES=$(git diff --name-only "${DIFF_BASE}..HEAD" -- . \ + ':!.planning/' ':!ROADMAP.md' ':!STATE.md' \ + ':!*-SUMMARY.md' ':!*-VERIFICATION.md' ':!*-PLAN.md' \ + ':!package-lock.json' ':!yarn.lock' ':!Gemfile.lock' ':!poetry.lock' 2>/dev/null) + + # Build a newline-delimited list of already-scoped files for exact membership + # testing (portable — bash 3.2 on macOS has no associative arrays). grep -Fxq + # matches the WHOLE line exactly, so a short basename (e.g. root `Dockerfile`) + # does NOT substring-match a longer scoped path (e.g. `docker/Dockerfile`). + IN_SCOPE=$(printf '%s\n' "${REVIEW_FILES[@]}") + + MISSING_FROM_SUMMARY=() + while IFS= read -r file; do + [ -z "$file" ] && continue + # Exact whole-line match; grep nonzero-exit => not in scope. + if printf '%s\n' "${REVIEW_FILES[@]}" | grep -Fxq -- "$file" 2>/dev/null; then + : # already scoped + else + MISSING_FROM_SUMMARY+=("$file"); REVIEW_FILES+=("$file") + fi + done <<< "$DIFF_FILES" + + if [ ${#MISSING_FROM_SUMMARY[@]} -gt 0 ]; then + echo "Warning: SUMMARY scope was missing ${#MISSING_FROM_SUMMARY[@]} changed file(s) the git diff surfaced; adding them to the review scope:" + printf ' - %s\n' "${MISSING_FROM_SUMMARY[@]}" + fi fi ``` diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index 8b55d568e..c941724f2 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -38,7 +38,24 @@ const REVIEWER_PATH = path.join(ROOT, 'agents', 'gsd-code-reviewer.md'); // This mirrors the logic in code-review.md lines 172-184 exactly. // If those lines change, this function must be updated in tandem (and the // docs-parity assertions below will catch a mismatch at the regex level). -// --------------------------------------------------------------------------- +// +// #2666: the acceptance predicate accepts root-level paths (no `/`) and known +// extensionless build files (Dockerfile/Makefile/etc.), not only nested paths +// with a trailing extension. Prose bullets are rejected by the known-filename / +// has-extension distinction (plus the post-processing existence check backstop +// in the shipped workflow). +const KNOWN_EXTENSIONLESS_BUILD_FILES = new Set([ + 'dockerfile', 'containerfile', 'makefile', 'justfile', 'procfile', +]); +function isAcceptablePath(raw) { + // A trailing `.`+alphanumerics extension qualifies (root-level OR nested): + // package.json, renovate.json, .gitlab-ci.yml, AGENTS.md, app/foo.tsx + if (/\.[A-Za-z0-9]+$/.test(raw)) return true; + // Known extensionless build filename (basename, case-insensitive): Dockerfile, Makefile, … + const base = raw.split('/').pop(); + if (KNOWN_EXTENSIONLESS_BUILD_FILES.has(base.toLowerCase())) return true; + return false; +} function parseKeyFiles(yaml) { const files = []; let inSection = null; @@ -53,7 +70,7 @@ function parseKeyFiles(yaml) { // Order matters: parens BEFORE em-dash because em-dashes can appear inside parens raw = raw.replace(/\s+\([^)]*\)\s*$/, ''); raw = raw.split(/\s+—\s/)[0].trim(); - if (/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)) { + if (isAcceptablePath(raw)) { files.push(raw); } } @@ -160,6 +177,107 @@ describe('Bug 1 — compute_file_scope SUMMARY parser', () => { assert.deepStrictEqual(files, ['app/page.tsx']); }); + // #2666 — the Tier-2 extractor must NOT drop repository-root files (no `/`) + // or known extensionless build files. Pre-fix the buggy predicate + // `/\//.test(raw) && /\.[A-Za-z0-9]+$/.test(raw)` dropped every root-level + // path and every extensionless build file anywhere in the tree. + test('#2666 RED: root-level files with extensions are accepted (package.json, renovate.json, .gitlab-ci.yml, AGENTS.md)', () => { + const yaml = [ + 'key-files:', + ' modified:', + ' - package.json', + ' - renovate.json', + ' - .gitlab-ci.yml', + ' - AGENTS.md', + ' - CLAUDE.md', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual( + files.sort(), + ['.gitlab-ci.yml', 'AGENTS.md', 'CLAUDE.md', 'package.json', 'renovate.json'], + 'root-level files with extensions must not be dropped for lacking a directory separator', + ); + }); + + test('#2666: nested extensionless build files are accepted (docker/Dockerfile, web/Makefile)', () => { + const yaml = [ + 'key-files:', + ' modified:', + ' - docker/Dockerfile', + ' - web/Makefile', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files.sort(), ['docker/Dockerfile', 'web/Makefile']); + }); + + test('#2666: root-level extensionless build files are accepted (Dockerfile, Makefile, Justfile, Containerfile, Procfile)', () => { + const yaml = [ + 'key-files:', + ' created:', + ' - Dockerfile', + ' - Makefile', + ' - Justfile', + ' - Containerfile', + ' - Procfile', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual( + files.sort(), + ['Containerfile', 'Dockerfile', 'Justfile', 'Makefile', 'Procfile'], + ); + }); + + test('#2666 acceptance #1: the reporter 10-file Docker+CI phase yields all 10 paths', () => { + const yaml = [ + 'key-files:', + ' created:', + ' - Dockerfile', + ' - .gitlab-ci.yml', + ' - renovate.json', + ' - AGENTS.md', + ' - CLAUDE.md', + ' - docs/DEVELOPMENT.md', + ' - scripts/version-consistency-gate.mjs', + ' - web/package.json', + ' - web/version_management.md', + ' - web/update-version.cjs', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual( + files.sort(), + [ + '.gitlab-ci.yml', 'AGENTS.md', 'CLAUDE.md', 'Dockerfile', + 'docs/DEVELOPMENT.md', 'renovate.json', 'scripts/version-consistency-gate.mjs', + 'web/package.json', 'web/update-version.cjs', 'web/version_management.md', + ], + 'the full reporter phase must scope all 10 files, including Dockerfile + root files', + ); + }); + + test('#2666 negative-space: a path-like prose bullet with no extension and unknown basename is rejected', () => { + // `topic/mode/date filters` has a `/` but no extension and an unknown basename — + // the pre-fix predicate dropped it (good), the relaxed predicate must STILL drop it. + const yaml = [ + 'key-decisions:', + ' - topic/mode/date filters', + 'key-files:', + ' created:', + ' - app/page.tsx', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files, ['app/page.tsx']); + }); + + test('#2666 negative-space: em-dash/parenthetical stripping still works on an accepted root file', () => { + const yaml = [ + 'key-files:', + ' modified:', + ' - Dockerfile — multi-stage build', + ].join('\n'); + const files = parseKeyFiles(yaml); + assert.deepStrictEqual(files, ['Dockerfile']); + }); + // Docs-parity: the workflow .md must contain the hyphen-aware boundary regex // so what we tested above is actually what is deployed. test('code-review.md contains hyphen-aware boundary regex [\\w-]+', () => { @@ -192,6 +310,76 @@ describe('Bug 1 — compute_file_scope SUMMARY parser', () => { 'Script must split on em-dash to strip narrative' ); }); + + // #2666 docs-parity: the shipped workflow must NOT still carry the buggy + // AND-joined predicate that required BOTH a `/` and a trailing extension — + // that predicate dropped every root-level file and every extensionless build + // file. Catches a revert of the #2666 fix. + test('#2666 docs-parity: compute_file_scope does not contain the buggy slash-and-extension predicate', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const scriptStart = src.indexOf('const files = [];'); + const scriptEnd = src.indexOf('if (files.length)', scriptStart); + const scriptSection = src.slice(scriptStart, scriptEnd); + assert.ok( + !scriptSection.includes('/\\//.test(raw) && /\\.[A-Za-z0-9]+$/.test(raw)'), + 'compute_file_scope must not use the buggy AND-joined /\\//.test(raw) && /\\.[A-Za-z0-9]+$/.test(raw) ' + + 'predicate (#2666) — it drops every root-level and extensionless build file. Found section:\n' + + scriptSection + ); + }); + + // #2666 docs-parity: the shipped workflow must reference the known + // extensionless build filenames so Dockerfile/Makefile/etc. are accepted. + test('#2666 docs-parity: compute_file_scope accepts known extensionless build files (Dockerfile)', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + const scriptStart = src.indexOf('const files = [];'); + const scriptEnd = src.indexOf('if (files.length)', scriptStart); + const scriptSection = src.slice(scriptStart, scriptEnd); + assert.ok( + /dockerfile/i.test(scriptSection), + 'compute_file_scope must reference known extensionless build filenames (e.g. Dockerfile) ' + + 'so they are not dropped (#2666). Found section:\n' + scriptSection + ); + }); + + // #2666 docs-parity: the Tier-3 git-diff fallback must intersect with the + // SUMMARY scope and warn on dropped files (not only fire on zero Tier-2 hits). + test('#2666 docs-parity: Tier-3 intersects/warns against git diff --name-only', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + // The shipped workflow must compute git diff --name-only AND emit a warning + // when the diff contains files the SUMMARY extractor did not surface. + assert.ok( + src.includes('git diff --name-only'), + 'code-review.md must run `git diff --name-only` to cross-check the SUMMARY scope (#2666)' + ); + assert.ok( + /warn|missing|not surfaced|did not|not in/i.test(src), + 'code-review.md must warn when git diff contains files the SUMMARY extractor dropped (#2666)' + ); + }); + + // #2666 docs-parity: the membership test must be EXACT whole-line matching + // (grep -Fxq), not an unanchored `case` substring match — otherwise a short + // basename in the diff (root `Dockerfile`) substring-matches a longer scoped + // path (`docker/Dockerfile`) and is silently skipped, reintroducing the bug. + test('#2666 docs-parity: Tier-3 cross-check uses exact whole-line matching (grep -Fxq), not substring case', () => { + const src = fs.readFileSync(WORKFLOW_PATH, 'utf8'); + assert.ok( + src.includes('grep -Fxq'), + 'code-review.md Tier-3 cross-check must use grep -Fxq (exact whole-line match) for membership ' + + 'testing, not an unanchored `case` substring match that would skip a root `Dockerfile` ' + + 'whose name appears as a suffix of an already-scoped `docker/Dockerfile` (#2666)' + ); + // The unanchored substring `case "$IN_SCOPE" in` membership test must NOT be + // present — it would false-match a basename suffix. Plain substring check (no + // regex, so no CRLF-fragility): the grep -Fxq positive guard above proves the + // correct mechanism; this negative guard catches a revert to the `case` form. + assert.ok( + !src.includes('case "$IN_SCOPE"'), + 'code-review.md Tier-3 must not use the unanchored `case "$IN_SCOPE"` substring membership ' + + 'test (#2666) — use grep -Fxq for exact whole-line matching' + ); + }); }); // --------------------------------------------------------------------------- diff --git a/tests/emitted-drift-ack.json b/tests/emitted-drift-ack.json index eadab7bd7..fa2f84cde 100644 --- a/tests/emitted-drift-ack.json +++ b/tests/emitted-drift-ack.json @@ -12,6 +12,9 @@ }, "next.md": { "reason": "#2800 — same derived-flag-loop change as autonomous.md. next.md needed no relocation (its launcher preamble already precedes the loop), so the growth here is only the loop plus the comment recording that --all and --text stay literal because they are convergence controls, not reviewer lanes." + }, + "code-review.md": { + "reason": "#2666: the Tier-2 SUMMARY extractor predicate is relaxed to accept root-level and extensionless build files (Dockerfile/Makefile/etc.), and the Tier-3 git-diff fallback is converted from an eq-zero gate into an intersect-and-warn that cross-checks the SUMMARY scope against `git diff --name-only` with exact whole-line matching (grep -Fxq) and warns about + adds any changed files the extractor missed. Growth is the relaxed predicate + the portable cross-check branch + their explanatory comments." } } }