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 <noreply@anthropic.com>
This commit is contained in:
@@ -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) {
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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');
|
||||
|
||||
Reference in New Issue
Block a user