diff --git a/.changeset/safe-herons-parse.md b/.changeset/safe-herons-parse.md new file mode 100644 index 000000000..b7d9fd175 --- /dev/null +++ b/.changeset/safe-herons-parse.md @@ -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) diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index bcb863806..dc79b35ca 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -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 diff --git a/tests/code-review-pipeline-regression.test.cjs b/tests/code-review-pipeline-regression.test.cjs index 1781c3a3e..dff9339c3 100644 --- a/tests/code-review-pipeline-regression.test.cjs +++ b/tests/code-review-pipeline-regression.test.cjs @@ -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(''); + 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:',