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

fix(#4461): make code-review summary extraction shell-safe (#4533)

Browse Source 
* fix(#4461): make code-review summary extraction shell-safe

Emitted-Drift-Ack-Growth: code-review.md — use a literal heredoc for shell-safe SUMMARY parsing

* chore: add changeset for #4533

* fix(#4461): keep heredoc outside command substitution

* chore: rerun CI after Windows timeout

* test(#4461): execute the summary heredoc adversarially

* test(#4461): normalize adversarial paths for Git Bash

* test(#4461): keep adversarial fixture valid on Windows

* test(#4461): scope heredoc regression claim

---------

Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
Michel Moreira
2026-09-16 04:23:11 -03:00
committed by GitHub
parent 740ba0d8a3
commit 49f313d611
3 changed files with 123 additions and 38 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/safe-herons-parse.md Normal file
View File

@@ -0,0 +1,5 @@
---
type: Fixed
pr: 4533
---
**`/gsd-code-review` can parse phase SUMMARY files without shell syntax errors** — the embedded JavaScript now runs from a literal heredoc and receives the SUMMARY path through `argv`, so quotes and backticks cannot break the workflow command. (#4461)

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

@@ -151,6 +151,49 @@ If --files NOT provided:
if [ -z "$FILES_OVERRIDE" ]; then
SUMMARIES=$(ls "${PHASE_DIR}"/*-SUMMARY.md 2>/dev/null)
REVIEW_FILES=()
# Keep the literal heredoc outside command substitution: Bash 3.2 (the
# system Bash on macOS) reparses heredoc bodies nested directly in $(...).
extract_summary_files() {
node - "$1" 2>/dev/null <<'NODE'
const fs = require('fs');
const content = fs.readFileSync(process.argv[2], 'utf-8');
const match = content.replace(/\r\n/g, '\n').match(/^---\n([\s\S]*?)\n---/);
if (!match) { process.exit(0); }
const yaml = match[1];
const files = [];
let inSection = null;
for (const line of yaml.split('\n')) {
if (/^\s+created:/.test(line)) { inSection = 'created'; continue; }
if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; }
if (/^\s*[\w-]+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; }
if (inSection && /^\s+-\s+(.+)/.test(line)) {
let raw = line.match(/^\s+-\s+(.+)/)[1].trim();
raw = raw.replace(/^['"]|['"]$/g, '');
raw = raw.replace(/\s+\([^)]*\)\s*$/, '');
raw = raw.split(/\s+—\s/)[0].trim();
// #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);
}
}
}
if (files.length) console.log(files.join('\n'));
NODE
}
if [ -n "$SUMMARIES" ]; then
# Rewrapped through unquoted command substitution (gsd-core#4109): a bare
@@ -167,44 +210,7 @@ if [ -z "$FILES_OVERRIDE" ]; then
# Extract key_files.created and key_files.modified using node for reliable YAML parsing
# This avoids fragile awk parsing that breaks on indentation differences
EXTRACTED=$(node -e "
const fs = require('fs');
const content = fs.readFileSync('$summary', 'utf-8');
const match = content.replace(/\r\n/g, '\n').match(/^---\n([\s\S]*?)\n---/);
if (!match) { process.exit(0); }
const yaml = match[1];
const files = [];
let inSection = null;
for (const line of yaml.split('\n')) {
if (/^\s+created:/.test(line)) { inSection = 'created'; continue; }
if (/^\s+modified:/.test(line)) { inSection = 'modified'; continue; }
if (/^\s*[\w-]+:/.test(line) && !/^\s*-/.test(line)) { inSection = null; continue; }
if (inSection && /^\s+-\s+(.+)/.test(line)) {
let raw = line.match(/^\s+-\s+(.+)/)[1].trim();
raw = raw.replace(/^['"]|['"]$/g, '');
raw = raw.replace(/\s+\([^)]*\)\s*$/, '');
raw = raw.split(/\s+—\s/)[0].trim();
// #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);
}
}
}
if (files.length) console.log(files.join('\n'));
" 2>/dev/null)
EXTRACTED=$(extract_summary_files "$summary")
# Add extracted files to REVIEW_FILES array
if [ -n "$EXTRACTED" ]; then

74
tests/code-review-pipeline-regression.test.cjs
View File

@@ -139,6 +139,80 @@ function parseFrontmatterCritical(frontmatter) {
// file list, and must strip em-dash descriptions and parentheticals.
// ---------------------------------------------------------------------------
describe('Bug 1 — compute_file_scope SUMMARY parser', () => {
test('#4461: the shipped compute_file_scope bash fence parses verbatim', () => {
const src = readFileNormalized(WORKFLOW_PATH);
const stepStart = src.indexOf('<step name="compute_file_scope">');
assert.notStrictEqual(stepStart, -1, 'compute_file_scope step must exist');
const marker = src.indexOf('EXTRACTED=$(', stepStart);
assert.notStrictEqual(marker, -1, 'Tier-2 SUMMARY extractor must exist');
const fenceStart = src.lastIndexOf('```bash\n', marker);
const fenceEnd = src.indexOf('\n```', marker);
assert.ok(fenceStart !== -1 && fenceEnd !== -1, 'Tier-2 SUMMARY bash fence must be complete');
const script = src.slice(fenceStart + '```bash\n'.length, fenceEnd);
const result = runHook('-n', ['-c', script], {
interpreter: 'bash',
timeoutMs: PROBE_TIMEOUT_MS,
});
assert.equal(result.exitCode, 0, `bash rejected the shipped fence:\n${result.stderr}`);
});
test('#4461: the shipped extractor helper treats adversarial SUMMARY text and argv as inert data', () => {
const src = readFileNormalized(WORKFLOW_PATH);
const helperStart = src.indexOf(' extract_summary_files() {');
// Anchor on the real loop gate, not the whitespace-only separator above it:
// editors are entitled to trim trailing spaces without changing behavior.
const helperEnd = src.indexOf('\n if [ -n "$SUMMARIES" ]; then', helperStart);
assert.ok(helperStart !== -1 && helperEnd !== -1, 'extract_summary_files helper must be extractable');
const helper = src.slice(helperStart, helperEnd);
const dir = createTempDir('gsd-4461-adversarial-');
try {
const sentinel = path.join(dir, 'MUST-NOT-EXIST');
// This directly executes the shipped helper's argv boundary. It does
// not claim the workflow's outer SUMMARY-list iteration preserves
// whitespace; that pre-existing shell-word-splitting behavior remains
// tracked separately by #4109.
// Double quotes are not legal in Windows filenames. Keep the argv
// adversarial with shell syntax, whitespace, and a quote that is valid
// on every supported filesystem; the SUMMARY payload below exercises
// a literal double quote independently.
const summary = path.join(dir, "SUMMARY $(not-a-command) 'quoted'.md");
// Git Bash launches the native Windows Node binary in CI. Forward-slash
// absolute paths survive that argv boundary on both platforms, while a
// raw drive path's backslashes are MSYS quoting syntax rather than data.
const shellSentinel = sentinel.replace(/\\/g, '/');
const shellSummary = summary.replace(/\\/g, '/');
const dollarPath = `src/$(touch ${shellSentinel}).js`;
const backtickPath = `src/\`touch ${shellSentinel}\`.js`;
const quotePath = 'src/"quoted".js';
fs.writeFileSync(summary, [
'---',
'key-files:',
' created:',
` - ${dollarPath}`,
' modified:',
` - ${backtickPath}`,
` - ${quotePath}`,
'---',
'',
].join('\n'));
const script = ['set -eu', helper, 'extract_summary_files "$SUMMARY_PATH"'].join('\n');
const result = toLegacyResult(runHook('-c', [script, 'bash'], {
interpreter: 'bash',
env: { ...process.env, SUMMARY_PATH: shellSummary },
timeoutMs: PROBE_TIMEOUT_MS,
}));
assert.equal(result.status, 0, result.stderr);
assert.deepStrictEqual(result.stdout.trim().split('\n'), [dollarPath, backtickPath, quotePath]);
assert.ok(!fs.existsSync(sentinel), 'SUMMARY payload must never execute command substitutions');
} finally {
cleanup(dir);
}
});
test('extracts only key-files.created and key-files.modified entries', () => {
const yaml = [
'key-files:',