* 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>
75 lines
3.8 KiB
JavaScript
75 lines
3.8 KiB
JavaScript
'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 (exitCode stays non-zero, stdout stays
|
|
* empty either way). Confirmed live, twice, on real Windows CI: the check
|
|
* now correctly reports "timed out under CI load" for exactly that case.
|
|
*
|
|
* describeNpmVersionCheckFailure is the extracted, pure reason-selection
|
|
* 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');
|
|
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 result = { exitCode: 127, stdout: '', stderr: 'npm: not found', signal: null, error: Object.assign(new Error('spawnSync npm ENOENT'), { code: 'ENOENT' }), timedOut: false };
|
|
assert.equal(
|
|
describeNpmVersionCheckFailure(result),
|
|
'npm binary not found on PATH',
|
|
);
|
|
});
|
|
|
|
test('a timed-out spawn is reported as a timeout, not a missing binary', () => {
|
|
const result = { exitCode: 1, stdout: '', stderr: '', signal: 'SIGTERM', error: Object.assign(new Error('ETIMEDOUT'), { code: 'ETIMEDOUT' }), timedOut: true };
|
|
const reason = describeNpmVersionCheckFailure(result);
|
|
assert.match(reason, /timed out under CI load/);
|
|
assert.doesNotMatch(reason, /^npm binary not found on PATH$/);
|
|
});
|
|
|
|
test('timedOut is authoritative even without an ENOENT-shaped error object', () => {
|
|
// Mirrors what execNpm/_spawnResult actually produces for a timeout:
|
|
// error is set (spawnSync always populates it when `timeout` fires),
|
|
// but its code is ETIMEDOUT, not ENOENT -- timedOut is what matters.
|
|
const result = { exitCode: 1, stdout: '', stderr: '', signal: 'SIGTERM', error: null, timedOut: true };
|
|
assert.match(describeNpmVersionCheckFailure(result), /timed out under CI load/);
|
|
});
|
|
|
|
test('a non-zero exit with no stdout and no timeout is reported with the actual exit code', () => {
|
|
const result = { exitCode: 1, stdout: '', stderr: '', signal: null, error: null, timedOut: false };
|
|
assert.equal(
|
|
describeNpmVersionCheckFailure(result),
|
|
'npm --version exited 1 with no usable output',
|
|
);
|
|
});
|
|
|
|
test('exitCode 0 with no stdout gets its own message, not the "not found" fallback', () => {
|
|
// npm ran and exited cleanly but printed nothing -- distinct from every
|
|
// other branch (all of which involve a failed/absent spawn); must not be
|
|
// misdescribed as "not found on PATH" when the binary plainly ran.
|
|
const result = { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false };
|
|
assert.equal(
|
|
describeNpmVersionCheckFailure(result),
|
|
'npm --version exited 0 but produced no output',
|
|
);
|
|
});
|
|
});
|