From 0aeafc64258bcb0d59f041f29c15c8cb4fc9cc23 Mon Sep 17 00:00:00 2001 From: sim Date: Wed, 9 Sep 2026 00:27:32 -0400 Subject: [PATCH] fix: report the real cause when check-env.cjs's npm-version check fails Discovered blocking this PR's own Windows CI (unrelated to this PR's actual diff, fixed inline per this repo's no-defer policy): PR #4552's "full test (windows-latest, 24, shard 2/3)" job failed tests/check-env.test.cjs's npm-version subtest with "npm binary not found on PATH" under chunk 5/9's heavy load (51 concurrent files, 5+ minutes). Root-caused via the CI log: the check's spawnSync call used a 10s timeout, and every failure mode -- ENOENT, a signal-killed timeout, a non-zero exit, a thrown spawn error -- collapsed into that one message (only `res.status === 0 && res.stdout` was checked), so a genuine npm.cmd cold-start timeout under contention was indistinguishable from npm actually being absent. Extracted the reason-selection into describeNpmVersionCheckFailure, a pure function in the new scripts/lib/npm-version-check-diagnosis.cjs (kept out of check-env.cjs itself, which runs its CLI unconditionally on require with no `require.main === module` guard, so the pure logic can be unit-tested without triggering a real environment check). Reports ENOENT, signal-kill, non-zero-exit, and thrown-error cases distinctly. Does NOT raise the 10s timeout itself -- a slow subprocess under contention is a cost to reduce, not a tolerance to widen. Also fixed a stale tsconfig.build.tsbuildinfo incremental-build cache discovered while verifying this change: npm run build:lib was silently omitting gsd-core/bin/lib/markdown-table.cjs (a real, needed compiled module -- src/state-document.cts requires it), which only surfaced via npm run lint:generated-sync's gen-health-docs check failing with Cannot find module. Deleting the cache and rebuilding fresh restored it; docs/INVENTORY-MANIFEST.json needed no net change once the build was genuinely complete. Manually verified describeNpmVersionCheckFailure's five branches directly (ENOENT, signal-kill, non-zero exit, thrown error, defensive default) before wiring the test file, since this repo blocks local node --test. Re-ran node scripts/check-env.cjs directly to confirm the real success path is unaffected. Co-Authored-By: Claude Sonnet 5 --- .changeset/silly-hens-relax.md | 2 +- .changeset/tame-hens-jump.md | 5 ++ scripts/check-env.cjs | 14 +++-- scripts/lib/npm-version-check-diagnosis.cjs | 41 ++++++++++++++ tests/npm-version-check-diagnosis.test.cjs | 60 +++++++++++++++++++++ 5 files changed, 116 insertions(+), 6 deletions(-) create mode 100644 .changeset/tame-hens-jump.md create mode 100644 scripts/lib/npm-version-check-diagnosis.cjs create mode 100644 tests/npm-version-check-diagnosis.test.cjs diff --git a/.changeset/silly-hens-relax.md b/.changeset/silly-hens-relax.md index 477232b85..9ca0e888d 100644 --- a/.changeset/silly-hens-relax.md +++ b/.changeset/silly-hens-relax.md @@ -1,5 +1,5 @@ --- type: Security -pr: 0 +pr: 4552 --- **Pinned the transitive `hono` dependency to `>=4.13.5`** — fixes a moderate-severity path-traversal/DoS advisory chain (GHSA-gqvv-2mrq-wpjv, GHSA-g6gw-c38x-mqfc, GHSA-crvj-82cr-hjcx) in `hono <4.13.5`, pulled in transitively via `@anthropic-ai/claude-agent-sdk` -> `@modelcontextprotocol/sdk`. Discovered blocking `tests/npm-integrity-gate.test.cjs` while verifying #4460; fixed inline per this repo's no-defer policy rather than left for a separate PR. (#4460) diff --git a/.changeset/tame-hens-jump.md b/.changeset/tame-hens-jump.md new file mode 100644 index 000000000..3f58617c6 --- /dev/null +++ b/.changeset/tame-hens-jump.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4552 +--- +**`npm run check:env`'s npm-version check no longer misreports a timeout as a missing binary** — every `spawnSync` failure mode (ENOENT, a signal-killed timeout under load, a non-zero exit, a thrown spawn error) used to collapse into one message, "npm binary not found on PATH." Discovered live: an unrelated PR's Windows CI shard failed this check under heavy concurrent test load, and the message made a real timeout indistinguishable from npm genuinely being absent. The reason is now reported accurately; the check's own 10s timeout is unchanged. (#4460) diff --git a/scripts/check-env.cjs b/scripts/check-env.cjs index 704ed1908..f3c5317ed 100644 --- a/scripts/check-env.cjs +++ b/scripts/check-env.cjs @@ -30,6 +30,7 @@ const path = require('path'); const { spawnSync } = require('child_process'); const { ExitError, runMain } = require('./lib/cli-exit.cjs'); +const { describeNpmVersionCheckFailure } = require('./lib/npm-version-check-diagnosis.cjs'); // On Windows, npm ships as npm.cmd (a batch wrapper); spawnSync without // shell:true requires the exact filename including extension. @@ -180,18 +181,21 @@ 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 { - const res = spawnSync(npmCmd, ['--version'], { encoding: 'utf8', timeout: 10_000, shell: process.platform === 'win32' }); - if (res.status === 0 && res.stdout) { - currentNpm = res.stdout.trim(); + 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 { /* ignore */ } + } catch (e) { npmSpawnThrew = e; } if (!enginesNpm) { addCheck('npm-version', 'skip', 'engines.npm not set in package.json — skipping'); } else if (!currentNpm) { - addCheck('npm-version', 'fail', 'npm binary not found on PATH'); + addCheck('npm-version', 'fail', describeNpmVersionCheckFailure(npmSpawnResult, npmSpawnThrew, NPM_VERSION_TIMEOUT_MS)); } else { if (satisfiesConstraint(currentNpm, enginesNpm)) { addCheck('npm-version', 'pass', `npm ${currentNpm} satisfies ${enginesNpm}`); diff --git a/scripts/lib/npm-version-check-diagnosis.cjs b/scripts/lib/npm-version-check-diagnosis.cjs new file mode 100644 index 000000000..2f53c1c08 --- /dev/null +++ b/scripts/lib/npm-version-check-diagnosis.cjs @@ -0,0 +1,41 @@ +'use strict'; + +/** + * #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 + * 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. + * + * 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. + * + * @param {import('child_process').SpawnSyncReturns|null} spawnResult + * @param {Error|null} spawnThrew + * @param {number} timeoutMs + * @returns {string} + */ +function describeNpmVersionCheckFailure(spawnResult, spawnThrew, timeoutMs) { + if (spawnResult && spawnResult.error && spawnResult.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 (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}`; + } + return 'npm binary not found on PATH'; +} + +module.exports = { describeNpmVersionCheckFailure }; diff --git a/tests/npm-version-check-diagnosis.test.cjs b/tests/npm-version-check-diagnosis.test.cjs new file mode 100644 index 000000000..ff564b1c3 --- /dev/null +++ b/tests/npm-version-check-diagnosis.test.cjs @@ -0,0 +1,60 @@ +'use strict'; + +/** + * Regression coverage for #4460 (discovered blocking this PR's own Windows + * CI, unrelated to this PR's actual diff -- fixed inline per this repo's + * no-defer policy): scripts/check-env.cjs's npm-version check used to + * collapse every spawnSync failure mode into one message, "npm binary not + * found on PATH" -- including a TIMEOUT under CI load, which looks + * identical to a genuine ENOENT (status stays null, stdout stays empty + * either way). describeNpmVersionCheckFailure is the extracted, pure + * reason-selection logic; 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'); +const assert = require('node:assert/strict'); + +const { describeNpmVersionCheckFailure } = require('../scripts/lib/npm-version-check-diagnosis.cjs'); + +describe('describeNpmVersionCheckFailure (#4460)', () => { + test('a genuine ENOENT (npm truly absent) reports the original message', () => { + const spawnResult = { status: null, stdout: '', signal: null, error: Object.assign(new Error('spawnSync npm.cmd ENOENT'), { code: 'ENOENT' }) }; + assert.equal( + describeNpmVersionCheckFailure(spawnResult, null, 10_000), + 'npm binary not found on PATH', + ); + }); + + test('a signal-killed spawn (the timeout case) is reported as a timeout, not a missing binary', () => { + const spawnResult = { status: null, stdout: '', signal: 'SIGTERM', error: null }; + const reason = describeNpmVersionCheckFailure(spawnResult, null, 10_000); + assert.match(reason, /killed \(signal SIGTERM\)/); + assert.match(reason, /10000ms timeout under CI load/); + assert.doesNotMatch(reason, /^npm binary not found on PATH$/); + }); + + test('a non-zero exit with no stdout is reported with the actual exit code', () => { + const spawnResult = { status: 1, stdout: '', signal: null, error: null }; + assert.equal( + describeNpmVersionCheckFailure(spawnResult, null, 10_000), + 'npm --version exited 1 with no usable output', + ); + }); + + test('spawnSync itself throwing (not just returning a failure result) is reported with the thrown message', () => { + const thrown = new Error('EACCES: permission denied'); + assert.equal( + describeNpmVersionCheckFailure(null, thrown, 10_000), + 'npm --version could not be spawned: EACCES: permission denied', + ); + }); + + test('no spawn result and no thrown error (defensive default) falls back to the original message', () => { + assert.equal( + describeNpmVersionCheckFailure(null, null, 10_000), + 'npm binary not found on PATH', + ); + }); +});