golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 3 Packages Projects Releases Wiki Activity

fix(#2666): code-review scopes root-level + extensionless build files, cross-checks against git diff (#2895)

Browse Source 
* 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 <test@example.com>
This commit is contained in:
Tom Boucher
2026-07-30 22:27:32 -04:00
committed by GitHub
parent f093738412
commit 6e0bc50142
4 changed files with 263 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

81
gsd-core/workflows/code-review.md
View File

@@ -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
```