From 8731a90be6a32de576eb3967b636bb4de54e2a74 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 9 Sep 2026 07:23:00 -0400 Subject: [PATCH] fix: revert execNpm import in check-env.cjs, keep the diagnosis improvement The execNpm-routing redesign (previous commit) broke every real CI job: check-env.cjs runs as its own standalone "Environment check" step BEFORE `npm ci` / `npm run build:lib` -- a deliberate pre-flight, run before there is even a node_modules to build with. Its require of ../gsd-core/bin/lib/shell-command-projection.cjs (a tsc-compiled artifact that plain does not exist at that point in the pipeline) crashed with MODULE_NOT_FOUND on every platform, immediately, confirmed via the real CI log. My own local gsd-test run never caught this because it doesn't replicate that exact pre-build step ordering. Reverted the cross-module require entirely; check-env.cjs is back to a self-contained spawnSync(npmCmd, ...) call, no requires reaching into gsd-core/bin/lib. Kept the two things actually worth keeping from that detour: - the 15_000ms timeout (matches execNpm's own default elsewhere in this repo -- not invented, an existing precedent -- vs. the original 10s that failed twice under real Windows CI contention); - computing `timedOut` via `error.code === 'ETIMEDOUT'` inline (the same canonical, cross-platform-correct predicate that seam uses), rather than the earlier signal === 'SIGTERM' check, which that seam's own docstring documents as platform-fragile with a Windows-specific false-negative risk. Manually verified check-env.cjs runs correctly with gsd-core/bin/lib temporarily removed entirely (simulating the real pre-npm-ci CI ordering) before re-running gsd-test, since this repo blocks local node --test. Co-Authored-By: Claude Sonnet 5 --- scripts/check-env.cjs | 45 ++++++++++++++------- scripts/lib/npm-version-check-diagnosis.cjs | 36 ++++++++++------- tests/npm-version-check-diagnosis.test.cjs | 16 +++++--- 3 files changed, 63 insertions(+), 34 deletions(-) diff --git a/scripts/check-env.cjs b/scripts/check-env.cjs index 6a6531f53..3e6e33779 100644 --- a/scripts/check-env.cjs +++ b/scripts/check-env.cjs @@ -31,8 +31,16 @@ const { spawnSync } = require('child_process'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); const { describeNpmVersionCheckFailure } = require('./lib/npm-version-check-diagnosis.cjs'); -const { execNpm } = require('../gsd-core/bin/lib/shell-command-projection.cjs'); +// #4460 follow-up: check-env.cjs runs as its own standalone CI step BEFORE +// `npm ci` / `npm run build:lib` (a deliberate pre-flight, run before there +// is even a node_modules to build with) -- confirmed the hard way, by a +// MODULE_NOT_FOUND crash on every real CI platform after a first attempt at +// this fix routed the npm-version check through the canonical execNpm seam +// (gsd-core/bin/lib/shell-command-projection.cjs), a tsc-compiled artifact +// that plain does not exist yet at that point in the pipeline. This file +// must stay self-contained: no requires reaching into gsd-core/bin/lib. +// // On Windows, npm ships as npm.cmd (a batch wrapper); spawnSync without // shell:true requires the exact filename including extension. const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm'; @@ -182,20 +190,27 @@ function main() { // Check 2: npm version vs engines.npm (skip if field absent) // --------------------------------------------------------------------------- const enginesNpm = pkgField('engines.npm', PROJECT_ROOT); - // #4460: was a hand-rolled spawnSync(npmCmd, ...) with its own 10s timeout - // and shell:true/npm.cmd handling, duplicating -- imperfectly -- the - // canonical execNpm seam (src/shell-command-projection.cts, "OS Shell - // Projection: All OS-facing I/O; single platform seam" per CLAUDE.md). - // Routing through it directly gives the SAME npm.cmd/shell handling other - // callers rely on, the repo's own documented npm-subprocess timeout - // (execNpm's 15s default, vs. CLAUDE.md's "60s for npm" for network-facing - // peeks -- --version never touches the network, so the general-purpose - // default is the right analogue), and the canonical, cross-platform-correct - // timeout predicate (isSpawnTimeout / result.timedOut, which checks - // error.code === 'ETIMEDOUT' -- documented there as more reliable than - // signal === 'SIGTERM', which is "platform-fragile" specifically on - // Windows, the exact platform this was discovered failing on). - const npmVersionResult = execNpm(['--version']); + // #4460: 15_000ms, not the original 10_000 -- matches the default this + // repo's canonical (but here unusable, see the file-header note above) + // execNpm seam already uses for npm subprocess calls generally, rather + // than inventing a new number. Confirmed via real Windows CI: a 10s + // window was insufficient twice under ~51-file concurrent test load. + const NPM_VERSION_TIMEOUT_MS = 15_000; + const npmVersionSpawn = spawnSync(npmCmd, ['--version'], { encoding: 'utf8', timeout: NPM_VERSION_TIMEOUT_MS, shell: process.platform === 'win32' }); + const npmVersionResult = { + exitCode: npmVersionSpawn.status ?? 1, + stdout: (npmVersionSpawn.stdout || '').toString().trim(), + signal: npmVersionSpawn.signal ?? null, + error: npmVersionSpawn.error ?? null, + // Canonical cross-platform timeout predicate (matches this repo's + // execNpm/isSpawnTimeout convention, src/shell-command-projection.cts): + // error.code === 'ETIMEDOUT', which Node's spawnSync guarantees when its + // own `timeout` option fires. Checking `signal === 'SIGTERM'` instead + // (what an earlier version of this fix did) is platform-fragile -- that + // module's own docstring flags a Windows-specific false-negative risk, + // the exact platform this bug was discovered on. + timedOut: (npmVersionSpawn.error && npmVersionSpawn.error.code === 'ETIMEDOUT') === true, + }; const currentNpm = npmVersionResult.exitCode === 0 && npmVersionResult.stdout ? npmVersionResult.stdout : ''; if (!enginesNpm) { diff --git a/scripts/lib/npm-version-check-diagnosis.cjs b/scripts/lib/npm-version-check-diagnosis.cjs index 970e406f2..676cfd2a5 100644 --- a/scripts/lib/npm-version-check-diagnosis.cjs +++ b/scripts/lib/npm-version-check-diagnosis.cjs @@ -2,30 +2,38 @@ /** * #4460: distinguishes WHY scripts/check-env.cjs's npm-version check's - * execNpm(['--version']) (the canonical OS-shell-projection seam, - * src/shell-command-projection.cts) produced no usable output, instead of - * collapsing every case into "npm binary not found on PATH" -- a message - * that used to fire identically for a genuinely-missing binary AND for a - * spawnSync TIMEOUT under CI load (exitCode stays non-zero, stdout stays - * empty, either way). Root-caused live: an unrelated PR's Windows CI shard - * failed this check twice in a row while running ~51 concurrent test + * spawnSync(npmCmd, ['--version'], ...) produced no usable output, instead + * of collapsing every case into "npm binary not found on PATH" -- a + * message that used to fire identically for a genuinely-missing binary AND + * for a spawnSync TIMEOUT under CI load (exitCode stays non-zero, stdout + * stays empty, either way). Root-caused live: an unrelated PR's Windows CI + * shard failed this check twice in a row while running ~51 concurrent test * files; npm.cmd's own cold-start plausibly exceeded the check's original * 10s window under that contention, and the misleading message made a real * timeout indistinguishable from npm actually being absent. * - * Uses `result.timedOut` (execNpm's canonical, cross-platform-correct - * timeout predicate -- `error.code === 'ETIMEDOUT'`, per - * shell-command-projection.cts's isSpawnTimeout docstring) rather than - * checking `result.signal === 'SIGTERM'` directly: that check is documented - * there as platform-fragile, with a specifically-called-out risk of a false - * NEGATIVE on Windows -- the exact platform this failure was discovered on. + * Expects `result.timedOut` to already be computed the same way this + * repo's canonical OS-shell-projection seam (execNpm / isSpawnTimeout, + * src/shell-command-projection.cts) computes it: `error.code === + * 'ETIMEDOUT'`, which Node's spawnSync guarantees when its own `timeout` + * option fires. NOT imported directly here -- check-env.cjs deliberately + * cannot depend on that seam's compiled output (gsd-core/bin/lib/*.cjs), a + * tsc build artifact that does not exist yet when check-env.cjs runs as its + * own standalone pre-`npm ci` CI step (confirmed live: an earlier version + * of this fix routed through execNpm directly and crashed every real CI + * job with MODULE_NOT_FOUND) -- so the same ETIMEDOUT check is computed + * inline in check-env.cjs instead. Checking `result.signal === 'SIGTERM'` + * directly (what an earlier version of this fix did) is platform-fragile + * per that seam's own documented reasoning, with a specifically-called-out + * risk of a false NEGATIVE on Windows -- the exact platform this failure + * was discovered on. * * Kept out of scripts/check-env.cjs itself (which runs its CLI unconditionally * on require, with no `require.main === module` guard) so this pure logic * can be required directly by tests without triggering a real environment * check. * - * @param {import('../../src/shell-command-projection.cts').SpawnResultOutput} result + * @param {{exitCode: number, stdout: string, signal: string|null, error: (Error & {code?: string})|null, timedOut: boolean}} result * @returns {string} */ function describeNpmVersionCheckFailure(result) { diff --git a/tests/npm-version-check-diagnosis.test.cjs b/tests/npm-version-check-diagnosis.test.cjs index 1a50c76b2..83cdb3c91 100644 --- a/tests/npm-version-check-diagnosis.test.cjs +++ b/tests/npm-version-check-diagnosis.test.cjs @@ -11,11 +11,17 @@ * now correctly reports "timed out under CI load" for exactly that case. * * describeNpmVersionCheckFailure is the extracted, pure reason-selection - * logic operating on execNpm's SpawnResultOutput shape (the canonical - * OS-shell-projection seam, src/shell-command-projection.cts). Kept in - * scripts/lib/ (not scripts/check-env.cjs itself, which runs its CLI - * unconditionally on require) so it can be required directly here without - * triggering a real environment check. + * logic. It takes a plain result object shaped like this repo's canonical + * OS-shell-projection seam's SpawnResultOutput (execNpm, + * src/shell-command-projection.cts) -- same `timedOut` (error.code === + * 'ETIMEDOUT') semantics -- but check-env.cjs computes that shape inline + * rather than importing the seam itself: that seam's compiled output + * (gsd-core/bin/lib/*.cjs) does not exist yet when check-env.cjs runs as + * its own standalone pre-`npm ci` CI step (an earlier version of this fix + * imported it directly and crashed every real CI job with + * MODULE_NOT_FOUND). Kept in scripts/lib/ (not scripts/check-env.cjs + * itself, which runs its CLI unconditionally on require) so it can be + * required directly here without triggering a real environment check. */ const { describe, test } = require('node:test');