test: cover realGitPath's null-stdout fallback and ratchet the harness coverage floors

package.json

@@ -70,7 +70,7 @@
     "pr-queue-hygiene": "node scripts/pr-queue-hygiene.mjs",
     "test:unit": "TZ=UTC node --test",
     "test:unit:coverage": "TZ=UTC node tests/tools/coverage-report.mjs",
-    "test:unit:coverage:check": "TZ=UTC node tests/tools/coverage-report.mjs --check 99 --check-source 100 --check-regions 96 --check-source-regions 100 --check-source-file-regions 100 --check-harness 100 --check-harness-regions 97 --check-harness-file-regions 97 --require-source-files",
+    "test:unit:coverage:check": "TZ=UTC node tests/tools/coverage-report.mjs --check 99 --check-source 100 --check-regions 96 --check-source-regions 100 --check-source-file-regions 100 --check-harness 100 --check-harness-regions 99 --check-harness-file-regions 98 --require-source-files",
     "test:e2e": "playwright test",
     "build:e2e:coverage": "npm run build:e2e:coverage:site && npm run build:e2e:coverage:fixtures",
     "build:e2e:coverage:site": "DOCUSAURUS_NO_PERSISTENT_CACHE=1 E2E_COVERAGE=1 npm run build:production",

tests/coverage-gate-thresholds.test.mjs

@@ -62,28 +62,31 @@ const FLOORS = {
   // entered none of the ratios above, which are all built from
   // isSourceFile(). These are the same three shapes over its own row.
   // Measured on the suite this commit ships: harness files 100.00% lines /
-  // 99.57% regions (5336/5336 lines, 1867/1875 regions), now that #1294 has
-  // brought tests/helpers*.mjs into the row. The lowest harness file is
-  // tests/helpers-script-sandbox.mjs at 97.06%, floored to 97; next is
-  // tests/tools/e2e-coverage-report.mjs at 98.57%, and every other harness
-  // file is at 100.00%.
+  // 99.63% regions (5352/5352 lines, 1870/1877 regions), now that #1294 has
+  // brought tests/helpers*.mjs into the row and tests/helpers-script-sandbox.mjs
+  // has been lifted to 100.00%. The lowest harness file is now
+  // tests/tools/e2e-coverage-report.mjs at 98.57%, floored to 98, and every
+  // other harness file is at 100.00%.
   //
-  // 98.57% is e2e-coverage-report.mjs's ceiling, not a backlog: its 7
+  // 98.57% is e2e-coverage-report.mjs's ceiling, not a backlog: its 6
   // uncovered regions are the defensive fallbacks the note at the top of the
   // file lists as unreachable (#1253; three more were deleted rather than
   // listed in #1304), so no test can lift it. The per-file floor must stay at
-  // or below 97 while helpers-script-sandbox.mjs sits at 97.06, and at or
-  // below 98 until that list shrinks; a probe that lands one of them should
+  // or below 98 until that list shrinks; a probe that lands one of them should
   // raise the floor and shorten the list in the same change.
   //
-  // --check-harness-regions is left at 97 against a measured 99.57 on purpose:
-  // #1294 only just rescoped the row, and the helper tree's lowest file is
-  // 0.06 points above the per-file floor, so the aggregate is left with its
-  // slack until that file has been lifted rather than ratcheted into a gate
-  // the next helper edit trips.
+  // Both harness floors moved up with this commit. They were previously held
+  // at 97 by tests/helpers-script-sandbox.mjs, whose single uncovered region
+  // was realGitPath's `?? ''` arm: spawnSync only reports `stdout: null` for a
+  // process that never started, and that call passes `shell: true`, so no test
+  // could reach it while the expression stayed inline. Splitting the
+  // normalisation out as gitPathFromLookup made it a function of the one value
+  // spawnSync varies, and that file is now at 100.00%. The aggregate was
+  // deliberately left at 97 against a measured 99.57 only "until that file has
+  // been lifted"; it has been, so it follows the measurement up to 99.
   '--check-harness': 100,
-  '--check-harness-regions': 97,
-  '--check-harness-file-regions': 97,
+  '--check-harness-regions': 99,
+  '--check-harness-file-regions': 98,
 };
 
 function parseGate(command) {

tests/helpers-script-sandbox-fallbacks.test.mjs

@@ -1,6 +1,9 @@
 import assert from 'node:assert/strict';
 import test from 'node:test';
-import { runScriptInSandbox } from './helpers-script-sandbox.mjs';
+import {
+  gitPathFromLookup,
+  runScriptInSandbox,
+} from './helpers-script-sandbox.mjs';
 
 // tests/helpers-spawn-fallbacks.test.mjs pins the same contract for the three
 // other sandbox helpers (helpers.mjs, helpers-fetch-mock.mjs,
@@ -137,3 +140,39 @@ test('runScriptInSandbox falls back to /usr/bin/git when no git is on PATH', ()
   assert.equal(result.stderr, '');
   assert.equal(result.outputs['data/awards.json'], AWARDS_FIXTURE);
 });
+
+// The test above reaches realGitPath's `|| '/usr/bin/git'` the way a caller
+// does, through an empty PATH. Its sibling fallback, the `?? ''` that absorbs
+// a null stdout, has no such route: spawnSync only reports `stdout: null` when
+// the process never started, and realGitPath passes `shell: true`, so the
+// process in question is /bin/sh. Nothing a test can do to this repository
+// makes /bin/sh unspawnable for one call and not for the rest of the file.
+//
+// gitPathFromLookup is that normalisation as a function of the one value
+// spawnSync varies, so the arm is driven by handing it the value spawnSync
+// would have produced rather than by trying to produce it. The cases below
+// pair each fallback with the input that skips it, so a fallback deleted or
+// inverted fails here instead of silently handing the git shim a path it will
+// `exec ""`.
+test('gitPathFromLookup returns the resolved path when the lookup printed one', () => {
+  // What `command -v git` prints on a normal run: an absolute path and a
+  // trailing newline, which must be trimmed and otherwise left alone.
+  assert.equal(gitPathFromLookup('/usr/local/bin/git\n'), '/usr/local/bin/git');
+});
+
+test('gitPathFromLookup falls back when the lookup printed nothing', () => {
+  // `command -v git` found nothing: it exits non-zero and prints an empty
+  // string, which trims to '' and must not be handed to the shim.
+  assert.equal(gitPathFromLookup(''), '/usr/bin/git');
+  assert.equal(gitPathFromLookup('   \n'), '/usr/bin/git');
+});
+
+test('gitPathFromLookup falls back when the lookup never ran', () => {
+  // spawnSync reports a process that never started as `stdout: null` rather
+  // than throwing, and `undefined` is what a caller reading `.stdout` off such
+  // a result sees when the field is absent. Without the `?? ''` both would
+  // throw on `.trim()`, so this is the arm that keeps a failed spawn
+  // indistinguishable from a failed lookup at the shim's edge.
+  assert.equal(gitPathFromLookup(null), '/usr/bin/git');
+  assert.equal(gitPathFromLookup(undefined), '/usr/bin/git');
+});

tests/helpers-script-sandbox.mjs

@@ -231,12 +231,29 @@ function writeFiles(base, tree) {
   }
 }
 
+// The normalisation half of realGitPath, split out so both of its fallbacks can
+// be driven directly.
+//
+// `|| '/usr/bin/git'` is reachable from the outside: an empty PATH makes
+// `command -v git` find nothing and print nothing, which
+// tests/helpers-script-sandbox-fallbacks.test.mjs exercises through
+// runScriptInSandbox. `?? ''` is not. It guards the case spawnSync signals by
+// returning `stdout: null` — the shell itself failing to start — and the call
+// passes `shell: true`, so reaching it would mean making /bin/sh unspawnable
+// for one call in this process. Leaving it inline left the arm as the single
+// uncovered region in this file, which alone held the gate's
+// --check-harness-file-regions floor down; as a function it is reachable by
+// passing the value spawnSync would have produced.
+export function gitPathFromLookup(stdout) {
+  return (stdout ?? '').trim() || '/usr/bin/git';
+}
+
 // Resolved before the shim directory is prepended to PATH so the shim delegates
 // to the real binary instead of recursing into itself.
 function realGitPath() {
   const found = spawnSync('command', ['-v', 'git'], {
     encoding: 'utf8',
     shell: true,
   });
-  return (found.stdout ?? '').trim() || '/usr/bin/git';
+  return gitPathFromLookup(found.stdout);
 }