diff --git a/changelog.d/1954-arch-docs-no-shell.fixed.md b/changelog.d/1954-arch-docs-no-shell.fixed.md new file mode 100644 index 000000000..facd11bab --- /dev/null +++ b/changelog.d/1954-arch-docs-no-shell.fixed.md @@ -0,0 +1 @@ +The architecture-docs gate no longer starts git through a shell ([#1954](https://github.com/ThinkFlowLab/FlyBot/issues/1954)): on Windows the gate handed `git rev-parse --verify origin/main^{commit}` to cmd.exe, `^` is cmd's escape character, so the `^{commit}` never reached git and every scheduled Windows row reported `arch-docs: FAIL — cannot diff against origin/main` on a checkout where `origin/main` existed. git now runs with its arguments as a list, the ref verbatim, and the fail-closed behaviour on a genuinely missing base ref is unchanged. diff --git a/core/scripts/check-arch-docs.mjs b/core/scripts/check-arch-docs.mjs index 8fb029383..bab052d10 100644 --- a/core/scripts/check-arch-docs.mjs +++ b/core/scripts/check-arch-docs.mjs @@ -1,4 +1,4 @@ -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import { readFileSync, readdirSync, existsSync } from 'node:fs'; import { join, relative, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -72,7 +72,7 @@ function baseRef() { function assertBaseRef(ref) { try { - execSync(`git rev-parse --verify ${ref}^{commit}`, { + execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { cwd: ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], @@ -83,7 +83,7 @@ function assertBaseRef(ref) { } function changedFiles(ref) { - const out = execSync(`git diff --name-only ${ref}...HEAD`, { + const out = execFileSync('git', ['diff', '--name-only', `${ref}...HEAD`], { cwd: ROOT, encoding: 'utf8', }); @@ -91,7 +91,7 @@ function changedFiles(ref) { } function fileChangedIn(ref, file) { - const out = execSync(`git diff --name-only ${ref}...HEAD -- ${file}`, { + const out = execFileSync('git', ['diff', '--name-only', `${ref}...HEAD`, '--', file], { cwd: ROOT, encoding: 'utf8', }).trim(); @@ -100,7 +100,7 @@ function fileChangedIn(ref, file) { export function hasDocsNotNeededTrailer(ref = baseRef()) { assertBaseRef(ref); - const out = execSync(`git log ${ref}..HEAD --pretty=%B`, { + const out = execFileSync('git', ['log', `${ref}..HEAD`, '--pretty=%B'], { cwd: ROOT, encoding: 'utf8', }); diff --git a/core/test/check-arch-docs.test.js b/core/test/check-arch-docs.test.js index 05e8085f0..c8da557a0 100644 --- a/core/test/check-arch-docs.test.js +++ b/core/test/check-arch-docs.test.js @@ -1,8 +1,10 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { delimiter, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; import { parseOwnedPaths, pathMatchesOwned, @@ -62,3 +64,51 @@ test('#1076: the refusal names both places the trailer has to appear', async () assert.match(text, /squash/i); assert.match(text, /Docs-not-needed:/); }); + +test('the gate never runs git through a shell', async () => { + // #1954: on Windows `execSync('git rev-parse --verify origin/main^{commit}')` is handed to cmd.exe, + // which eats `^`; rev-parse fails and the gate reports "cannot diff against origin/main" on a + // checkout where origin/main exists. The gate must pass `^{commit}` to git as one argument, no shell + // in between. A fake `git` early on PATH records the *name of its parent process*: a shell parent + // means the command was built as a string and handed to sh (which is what cmd.exe does on Windows). + const dir = mkdtempSync(join(tmpdir(), 'arch-shell-')); + const bin = join(dir, 'bin'); + mkdirSync(bin, { recursive: true }); + const marker = join(dir, 'parent.txt'); + const fakeGit = join(bin, 'git'); + writeFileSync( + fakeGit, + [ + '#!/bin/sh', + 'p=$PPID', + 'c=$(cat /proc/$p/comm 2>/dev/null)', + 'if [ -z "$c" ]; then c=$(ps -o comm= -p "$p" 2>/dev/null); fi', + 'printf "%s" "$c" > ' + JSON.stringify(marker), + 'case "$c" in', + ' sh|dash|bash|zsh|ksh|cmd*|cmd.exe) exit 1 ;;', + 'esac', + 'case "$1" in', + ' rev-parse) echo 0123456789012345678901234567890123456789 ;;', + ' diff|log) ;;', + ' *) exit 1 ;;', + 'esac', + 'exit 0', + '', + ].join('\n'), + { mode: 0o755 }, + ); + + const script = fileURLToPath(new URL('../scripts/check-arch-docs.mjs', import.meta.url)); + const r = spawnSync(process.execPath, [script], { + encoding: 'utf8', + env: { ...process.env, PATH: `${bin}${delimiter}${process.env.PATH}` }, + }); + const parent = readFileSync(marker, 'utf8').trim(); + assert.notEqual(parent, '', 'the fake git must actually run'); + assert.ok( + !/^(sh|dash|bash|zsh|ksh|cmd)/.test(parent), + `git was started by a shell (${parent}), so ^{commit} is mangled on Windows`, + ); + assert.equal(r.status, 0, `gate must pass: ${r.stdout}${r.stderr}`); + assert.match(r.stdout, /arch-docs: OK/); +}); diff --git a/docs/subsystems/install.md b/docs/subsystems/install.md index 291d7b24c..594d102e1 100644 --- a/docs/subsystems/install.md +++ b/docs/subsystems/install.md @@ -31,7 +31,7 @@ Tooling for setup checks, the architecture-docs gate and the container image (#3 | File | Role | | --- | --- | -| `core/scripts/check-arch-docs.mjs` | owned_paths ↔ docs gate. Its refusal names both places a `Docs-not-needed:` trailer has to appear — the pull request body and the branch commit — because the gate reads `git log ..HEAD` and, on a merge-queue entry, that range is the squashed body (#1076). | +| `core/scripts/check-arch-docs.mjs` | owned_paths ↔ docs gate. Its refusal names both places a `Docs-not-needed:` trailer has to appear — the pull request body and the branch commit — because the gate reads `git log ..HEAD` and, on a merge-queue entry, that range is the squashed body (#1076). It hands git its arguments as a list (`execFileSync`), never a joined command line through a shell: on Windows that shell is cmd.exe, where `^` escapes the next character, so `origin/main^{commit}` reached git as `origin/main{commit}` and every scheduled windows-latest row reported `cannot diff against origin/main` on a checkout that had it (#1954). | | `core/test/check-arch-docs.test.js` | Tests for the gate. | | `core/test/install-doctor.test.js` | Install doctor tests. | | `plugins/README.md` | Plugin list. | diff --git a/patches/ds-bot/files/scripts/check-arch-docs.mjs b/patches/ds-bot/files/scripts/check-arch-docs.mjs index 8fb029383..bab052d10 100644 --- a/patches/ds-bot/files/scripts/check-arch-docs.mjs +++ b/patches/ds-bot/files/scripts/check-arch-docs.mjs @@ -1,4 +1,4 @@ -import { execSync } from 'node:child_process'; +import { execFileSync } from 'node:child_process'; import { readFileSync, readdirSync, existsSync } from 'node:fs'; import { join, relative, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -72,7 +72,7 @@ function baseRef() { function assertBaseRef(ref) { try { - execSync(`git rev-parse --verify ${ref}^{commit}`, { + execFileSync('git', ['rev-parse', '--verify', `${ref}^{commit}`], { cwd: ROOT, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], @@ -83,7 +83,7 @@ function assertBaseRef(ref) { } function changedFiles(ref) { - const out = execSync(`git diff --name-only ${ref}...HEAD`, { + const out = execFileSync('git', ['diff', '--name-only', `${ref}...HEAD`], { cwd: ROOT, encoding: 'utf8', }); @@ -91,7 +91,7 @@ function changedFiles(ref) { } function fileChangedIn(ref, file) { - const out = execSync(`git diff --name-only ${ref}...HEAD -- ${file}`, { + const out = execFileSync('git', ['diff', '--name-only', `${ref}...HEAD`, '--', file], { cwd: ROOT, encoding: 'utf8', }).trim(); @@ -100,7 +100,7 @@ function fileChangedIn(ref, file) { export function hasDocsNotNeededTrailer(ref = baseRef()) { assertBaseRef(ref); - const out = execSync(`git log ${ref}..HEAD --pretty=%B`, { + const out = execFileSync('git', ['log', `${ref}..HEAD`, '--pretty=%B'], { cwd: ROOT, encoding: 'utf8', }); diff --git a/patches/ds-bot/files/test/check-arch-docs.test.js b/patches/ds-bot/files/test/check-arch-docs.test.js index 05e8085f0..c8da557a0 100644 --- a/patches/ds-bot/files/test/check-arch-docs.test.js +++ b/patches/ds-bot/files/test/check-arch-docs.test.js @@ -1,8 +1,10 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { mkdtempSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readFileSync, writeFileSync } from 'node:fs'; +import { spawnSync } from 'node:child_process'; +import { delimiter, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { tmpdir } from 'node:os'; -import { join } from 'node:path'; import { parseOwnedPaths, pathMatchesOwned, @@ -62,3 +64,51 @@ test('#1076: the refusal names both places the trailer has to appear', async () assert.match(text, /squash/i); assert.match(text, /Docs-not-needed:/); }); + +test('the gate never runs git through a shell', async () => { + // #1954: on Windows `execSync('git rev-parse --verify origin/main^{commit}')` is handed to cmd.exe, + // which eats `^`; rev-parse fails and the gate reports "cannot diff against origin/main" on a + // checkout where origin/main exists. The gate must pass `^{commit}` to git as one argument, no shell + // in between. A fake `git` early on PATH records the *name of its parent process*: a shell parent + // means the command was built as a string and handed to sh (which is what cmd.exe does on Windows). + const dir = mkdtempSync(join(tmpdir(), 'arch-shell-')); + const bin = join(dir, 'bin'); + mkdirSync(bin, { recursive: true }); + const marker = join(dir, 'parent.txt'); + const fakeGit = join(bin, 'git'); + writeFileSync( + fakeGit, + [ + '#!/bin/sh', + 'p=$PPID', + 'c=$(cat /proc/$p/comm 2>/dev/null)', + 'if [ -z "$c" ]; then c=$(ps -o comm= -p "$p" 2>/dev/null); fi', + 'printf "%s" "$c" > ' + JSON.stringify(marker), + 'case "$c" in', + ' sh|dash|bash|zsh|ksh|cmd*|cmd.exe) exit 1 ;;', + 'esac', + 'case "$1" in', + ' rev-parse) echo 0123456789012345678901234567890123456789 ;;', + ' diff|log) ;;', + ' *) exit 1 ;;', + 'esac', + 'exit 0', + '', + ].join('\n'), + { mode: 0o755 }, + ); + + const script = fileURLToPath(new URL('../scripts/check-arch-docs.mjs', import.meta.url)); + const r = spawnSync(process.execPath, [script], { + encoding: 'utf8', + env: { ...process.env, PATH: `${bin}${delimiter}${process.env.PATH}` }, + }); + const parent = readFileSync(marker, 'utf8').trim(); + assert.notEqual(parent, '', 'the fake git must actually run'); + assert.ok( + !/^(sh|dash|bash|zsh|ksh|cmd)/.test(parent), + `git was started by a shell (${parent}), so ^{commit} is mangled on Windows`, + ); + assert.equal(r.status, 0, `gate must pass: ${r.stdout}${r.stderr}`); + assert.match(r.stdout, /arch-docs: OK/); +});