arch-docs: run git without a shell so ^{commit} survives Windows

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.

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,15 +83,15 @@ 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',
   });
   return out.split(/\r?\n/).filter(Boolean);
 }
 
 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',
   });

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/);
+});

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 <base>..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 <base>..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. |

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,15 +83,15 @@ 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',
   });
   return out.split(/\r?\n/).filter(Boolean);
 }
 
 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',
   });

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/);
+});