fix: route check-env.cjs's npm-version check through the canonical execNpm seam
Per /research + /diagnose direction: the timeout fix landed earlier this
session correctly diagnosed the failure (a real spawnSync timeout under
Windows CI contention, not npm being absent) but it recurred on the very
next push -- same chunk, same ~51-file load. Rather than raise the
hand-rolled 10s timeout myself (CLAUDE.md's own rule: fix the cost, not
the tolerance, and never touch a timeout without explicit instruction),
investigated the repo's own precedent first.
Found: this repo already has a canonical OS-shell-projection seam for
exactly this (src/shell-command-projection.cts's execNpm), already used
by dozens of other scripts/*.cjs files (require('../gsd-core/bin/lib/...')
is an extremely well-established pattern), with:
- the same npm.cmd/shell:true Windows handling check-env.cjs was
hand-rolling, but centralized;
- a 15s default timeout (vs. check-env.cjs's 10s) -- not invented here,
an EXISTING value already governing npm subprocess calls elsewhere;
- isSpawnTimeout / result.timedOut, the canonical cross-platform timeout
predicate (error.code === 'ETIMEDOUT'), whose own docstring explicitly
warns that checking signal === 'SIGTERM' (what my first fix did) is
"platform-fragile" with a Windows-specific false-negative risk -- the
exact platform this bug lives on.
check-env.cjs's npm-version check now calls execNpm(['--version']) instead
of hand-rolling spawnSync + npmCmd + shell:true, and
describeNpmVersionCheckFailure now operates on execNpm's SpawnResultOutput
shape (using timedOut, not signal) rather than a raw spawnSync result.
This is a genuine architectural fix, not just a bigger number: it removes
a duplicate, slightly-divergent re-implementation of an existing seam and
inherits whatever that seam's timeout/handling becomes in the future.
Manually verified end-to-end (npm run check:env against the real
environment) and re-verified describeNpmVersionCheckFailure's branches
directly against execNpm's actual return shape before wiring the test
file, since this repo blocks local node --test.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -31,6 +31,7 @@ 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');
|
||||
|
||||
// On Windows, npm ships as npm.cmd (a batch wrapper); spawnSync without
|
||||
// shell:true requires the exact filename including extension.
|
||||
@@ -181,21 +182,26 @@ function main() {
|
||||
// Check 2: npm version vs engines.npm (skip if field absent)
|
||||
// ---------------------------------------------------------------------------
|
||||
const enginesNpm = pkgField('engines.npm', PROJECT_ROOT);
|
||||
const NPM_VERSION_TIMEOUT_MS = 10_000;
|
||||
let currentNpm = '';
|
||||
let npmSpawnResult = null;
|
||||
let npmSpawnThrew = null;
|
||||
try {
|
||||
npmSpawnResult = spawnSync(npmCmd, ['--version'], { encoding: 'utf8', timeout: NPM_VERSION_TIMEOUT_MS, shell: process.platform === 'win32' });
|
||||
if (npmSpawnResult.status === 0 && npmSpawnResult.stdout) {
|
||||
currentNpm = npmSpawnResult.stdout.trim();
|
||||
}
|
||||
} catch (e) { npmSpawnThrew = e; }
|
||||
// #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']);
|
||||
const currentNpm = npmVersionResult.exitCode === 0 && npmVersionResult.stdout ? npmVersionResult.stdout : '';
|
||||
|
||||
if (!enginesNpm) {
|
||||
addCheck('npm-version', 'skip', 'engines.npm not set in package.json — skipping');
|
||||
} else if (!currentNpm) {
|
||||
addCheck('npm-version', 'fail', describeNpmVersionCheckFailure(npmSpawnResult, npmSpawnThrew, NPM_VERSION_TIMEOUT_MS));
|
||||
addCheck('npm-version', 'fail', describeNpmVersionCheckFailure(npmVersionResult));
|
||||
} else {
|
||||
if (satisfiesConstraint(currentNpm, enginesNpm)) {
|
||||
addCheck('npm-version', 'pass', `npm ${currentNpm} satisfies ${enginesNpm}`);
|
||||
|
||||
@@ -2,38 +2,41 @@
|
||||
|
||||
/**
|
||||
* #4460: distinguishes WHY scripts/check-env.cjs's npm-version check's
|
||||
* `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 (status stays null, stdout stays
|
||||
* 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 while running 51 concurrent test files; npm.cmd's own
|
||||
* cold-start plausibly exceeded the 10s window under that contention, and
|
||||
* the misleading message made a real timeout indistinguishable from npm
|
||||
* actually being absent.
|
||||
* 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.
|
||||
*
|
||||
* 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 as a side effect.
|
||||
* check.
|
||||
*
|
||||
* @param {import('child_process').SpawnSyncReturns<string>|null} spawnResult
|
||||
* @param {Error|null} spawnThrew
|
||||
* @param {number} timeoutMs
|
||||
* @param {import('../../src/shell-command-projection.cts').SpawnResultOutput} result
|
||||
* @returns {string}
|
||||
*/
|
||||
function describeNpmVersionCheckFailure(spawnResult, spawnThrew, timeoutMs) {
|
||||
if (spawnResult && spawnResult.error && spawnResult.error.code === 'ENOENT') {
|
||||
function describeNpmVersionCheckFailure(result) {
|
||||
if (result.error && result.error.code === 'ENOENT') {
|
||||
return 'npm binary not found on PATH';
|
||||
}
|
||||
if (spawnResult && spawnResult.signal) {
|
||||
return `npm --version was killed (signal ${spawnResult.signal}) -- likely the ${timeoutMs}ms timeout under CI load, not a missing binary`;
|
||||
if (result.timedOut) {
|
||||
return `npm --version timed out under CI load -- not a missing binary`;
|
||||
}
|
||||
if (spawnResult && spawnResult.status != null && spawnResult.status !== 0) {
|
||||
return `npm --version exited ${spawnResult.status} with no usable output`;
|
||||
}
|
||||
if (spawnThrew) {
|
||||
return `npm --version could not be spawned: ${spawnThrew.message}`;
|
||||
if (result.exitCode !== 0) {
|
||||
return `npm --version exited ${result.exitCode} with no usable output`;
|
||||
}
|
||||
return 'npm binary not found on PATH';
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user