* fix: address orthogonal-review findings on the new work in this PR Isolated code-review + security-review of everything added to this PR since its original review (hono override, check-env.cjs rewrite/revert, new lib file, its test, installer enumeration). Security review: clean, no findings. Code review found: - BLOCKER: .changeset/silly-hens-relax.md described a hono override this PR no longer actually makes -- PR #4560 landed the identical fix on next first, and this branch's own hono commit became a genuine no-op the moment it was rebased onto that updated next (git diff origin/next -- package.json package-lock.json is empty). Deleted the orphaned changeset; next already carries #4560's equivalent one (.changeset/zesty-seals-click.md). - HIGH: .changeset/tame-hens-jump.md's body still described the execNpm-routing approach that was tried and reverted -- stale text from before that revert, would have shipped a release note for code that isn't actually in the diff. Rewritten to describe what actually shipped (self-contained spawnSync, 15s timeout, accurate ENOENT vs. timeout vs. non-zero-exit diagnosis). - LOW: no comment explaining why the spawnSync call has no try/catch (safe -- its documented contract routes failures through the returned result, never a throw -- but worth stating given this file's whole purpose is graceful degradation). Added one. - nit: exitCode 0 + empty stdout fell through to "npm binary not found on PATH", misdescribing a real npm binary that simply printed nothing. Gave it its own message; updated the corresponding test. Manually re-verified describeNpmVersionCheckFailure's branches and the real check:env success path before re-running gsd-test, since this repo blocks local node --test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: rest of the orthogonal-review fixes (previous commit only caught the deletion) Tooling mistake in the previous commit: a git add with the already-staged deleted changeset mixed into the same pathspec list errored out and silently skipped staging the other four files, so only the changeset deletion actually committed. This commit carries the rest of that same change: tame-hens-jump.md's rewritten body, check-env.cjs's no-try/catch comment, npm-version-check-diagnosis.cjs's exitCode-0-empty-stdout fix, and the corresponding test update. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4460): fix changeset pr field to point at this PR, not the original .changeset/tame-hens-jump.md's pr field still said 4552 (the PR its original text was authored under), but this PR (#4572) is what's actually landing the corrected body -- changeset-lint's own DEFECT.CHANGESET-PR-FIELD-DRIFT check caught it: "pr: 4552, expected pr: 4572". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
60 lines
3.0 KiB
JavaScript
60 lines
3.0 KiB
JavaScript
'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 (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.
|
|
*
|
|
* 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 {{exitCode: number, stdout: string, signal: string|null, error: (Error & {code?: string})|null, timedOut: boolean}} result
|
|
* @returns {string}
|
|
*/
|
|
function describeNpmVersionCheckFailure(result) {
|
|
if (result.error && result.error.code === 'ENOENT') {
|
|
return 'npm binary not found on PATH';
|
|
}
|
|
if (result.timedOut) {
|
|
return `npm --version timed out under CI load -- not a missing binary`;
|
|
}
|
|
if (result.exitCode !== 0) {
|
|
return `npm --version exited ${result.exitCode} with no usable output`;
|
|
}
|
|
if (result.exitCode === 0) {
|
|
// npm ran and exited cleanly but printed nothing -- distinct from every
|
|
// case above (which all involve a failed/absent spawn), so it gets its
|
|
// own message rather than falling through to "not found on PATH", which
|
|
// would misdescribe a real npm binary that simply produced no output.
|
|
return 'npm --version exited 0 but produced no output';
|
|
}
|
|
return 'npm binary not found on PATH';
|
|
}
|
|
|
|
module.exports = { describeNpmVersionCheckFailure };
|