diff --git a/.changeset/lively-quails-forage.md b/.changeset/lively-quails-forage.md new file mode 100644 index 000000000..a406bf4f9 --- /dev/null +++ b/.changeset/lively-quails-forage.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 4552 +--- +**`/gsd-code-review --files` no longer silently widens back to the whole phase** — Tier 3's SUMMARY/diff cross-check ran regardless of an explicit `--files` override, appending the rest of the phase's changed files onto a scope the user had deliberately narrowed. diff --git a/.changeset/silly-hens-relax.md b/.changeset/silly-hens-relax.md new file mode 100644 index 000000000..9ca0e888d --- /dev/null +++ b/.changeset/silly-hens-relax.md @@ -0,0 +1,5 @@ +--- +type: Security +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..0ffd02483 --- /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 times out under CI contention, and no longer misreports a timeout as a missing binary** — the check hand-rolled its own `spawnSync` call with a 10s timeout, duplicating (imperfectly) this repo's canonical OS-shell-projection seam. Discovered live: an unrelated PR's Windows CI shard failed this check twice under heavy concurrent test load. Now routed through `execNpm` (the same seam other npm-invoking code already uses), which gives it the canonical 15s timeout and the canonical, cross-platform-correct timeout detection — a real timeout is now reported as one, distinct from npm genuinely being absent. (#4460) diff --git a/bin/install.js b/bin/install.js index ade940d41..00ebb795d 100755 --- a/bin/install.js +++ b/bin/install.js @@ -436,7 +436,7 @@ const GSD_CHANGESET_FILES = [ 'github-release-notes.cjs', 'lint.cjs', 'new.cjs', 'README.md', // documentation only — not user-authored ]; -const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs', 'alias-drift-families.cjs', 'exit-code-registry.cjs', 'ndjson-reporter.cjs', 'ci-job-timing.cjs', 'shellcheck-fetch.cjs']; +const GSD_SCRIPTS_LIB_FILES = ['cli-exit.cjs', 'allowlist-ratchet.cjs', 'drift-scan.cjs', 'alias-drift-families.cjs', 'exit-code-registry.cjs', 'ndjson-reporter.cjs', 'ci-job-timing.cjs', 'shellcheck-fetch.cjs', 'npm-version-check-diagnosis.cjs']; /** * Resolve a runtime's shared-hooks directory name from its descriptor. diff --git a/gsd-core/workflows/code-review.md b/gsd-core/workflows/code-review.md index 3aa2b5407..7bf8b15bf 100644 --- a/gsd-core/workflows/code-review.md +++ b/gsd-core/workflows/code-review.md @@ -292,7 +292,13 @@ if [ ${#REVIEW_FILES[@]} -eq 0 ]; then echo "Warning: No phase commits found for '${PADDED_PHASE}'. Cannot determine reliable diff scope." echo "Use --files flag to specify files explicitly: /gsd:code-review ${PHASE_ARG} --files=file1,file2,..." fi -elif [ -n "$DIFF_BASE" ]; then +elif [ -z "$FILES_OVERRIDE" ] && [ -n "$DIFF_BASE" ]; then + # #4460: gated on FILES_OVERRIDE being unset — without this, REVIEW_FILES is + # already non-empty under --files (Tier 1 filled it), so this elif was + # reached anyway and the #2666 cross-check below appended the whole phase + # diff onto an explicit user-supplied file list, contradicting line 144's + # "Skip SUMMARY/git scoping entirely when --files is provided" and Tier 2's + # own --files guard (line 150). # #2666 cross-check: SUMMARY yielded a non-empty (possibly partial) scope. # Warn about — and add — any changed files the SUMMARY extractor did not surface, # so a partial result can no longer silently ship an incomplete review scope. diff --git a/gsd-core/workflows/execute-plan.md b/gsd-core/workflows/execute-plan.md index 325cf88a2..00809939a 100644 --- a/gsd-core/workflows/execute-plan.md +++ b/gsd-core/workflows/execute-plan.md @@ -409,7 +409,7 @@ emit narrative output between the Write tool call and the commit tool call. Truncation at this boundary is a known failure mode (see #2070 rescue logic in execute-phase.md step 5.5). -Create `{phase}-{plan}-SUMMARY.md` at `.planning/phases/XX-name/`. Use the template at `~/.claude/gsd-core/templates/summary.md` (or its `~/.claude/gsd-core/templates/summary.compact.md` variant — resolve per `~/.claude/gsd-core/references/compact-content-gate.md` §"Streams 1b and 4"). +Create `{phase}-{plan}-SUMMARY.md` at `.planning/phases/XX-name/`. Use the template at `~/.claude/gsd-core/templates/summary.md` (or `summary.compact.md` — same `compact-content-gate.md` resolution as the USER-SETUP template above). **Frontmatter:** phase, plan, subsystem, tags | requires/provides/affects | tech-stack.added/patterns | key-files.created/modified | key-decisions | requirements-completed (**MUST** copy `requirements` array from PLAN.md frontmatter verbatim) | duration ($DURATION), completed ($PLAN_END_TIME date). diff --git a/scripts/check-env.cjs b/scripts/check-env.cjs index 704ed1908..3e6e33779 100644 --- a/scripts/check-env.cjs +++ b/scripts/check-env.cjs @@ -30,7 +30,17 @@ 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'); +// #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'; @@ -180,18 +190,33 @@ function main() { // Check 2: npm version vs engines.npm (skip if field absent) // --------------------------------------------------------------------------- const enginesNpm = pkgField('engines.npm', PROJECT_ROOT); - let currentNpm = ''; - 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(); - } - } catch { /* ignore */ } + // #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) { 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(npmVersionResult)); } 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..676cfd2a5 --- /dev/null +++ b/scripts/lib/npm-version-check-diagnosis.cjs @@ -0,0 +1,52 @@ +'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`; + } + return 'npm binary not found on PATH'; +} + +module.exports = { describeNpmVersionCheckFailure }; diff --git a/tests/code-review-tier3-files-override-scoping.test.cjs b/tests/code-review-tier3-files-override-scoping.test.cjs new file mode 100644 index 000000000..5c8800581 --- /dev/null +++ b/tests/code-review-tier3-files-override-scoping.test.cjs @@ -0,0 +1,223 @@ +'use strict'; + +/** + * Regression coverage for #4460: code-review.md states (line 144) "Skip + * SUMMARY/git scoping entirely when --files is provided." Tier 2 honors + * this via `if [ -z "$FILES_OVERRIDE" ]`, but Tier 3's `#2666` cross-check + * had no `FILES_OVERRIDE` reference at all — reached via `elif [ -n + * "$DIFF_BASE" ]` whenever `REVIEW_FILES` was already non-empty (true + * under `--files`, since Tier 1 fills it), so it silently appended the + * whole phase diff onto an explicit user-supplied file list. + * + * Only Tier 3's own fence is extracted-and-executed VERBATIM from + * code-review.md (never reimplemented) against a real constructed git + * fixture. Tiers 1 and 2 are NOT sourced — both are seeded instead, + * mimicking each tier's real successful output rather than running its + * actual fence: + * + * - Tier 2's fence is not currently parseable bash at all (two unescaped + * `"` characters inside its embedded `node -e "..."` regex literal + * terminate the outer double-quoted string early, breaking bash's parse + * of the WHOLE script even though Tier 2's body never executes under + * `--files`). Real, already reported, already queued as its own issue + * (#4461, filed separately by #4460's own reporter) — fixing it here + * would be exactly the scope creep the reporter took care to avoid. + * - Tier 1's fence IS parseable bash, but its REPO_ROOT-prefix containment + * check is confirmed non-functional on Windows CI: `git rev-parse + * --show-toplevel` there returns a mixed-format path (`C:/Users/...` — + * drive letter, forward slashes) while GNU `realpath` (also bundled with + * Git for Windows) returns a genuine POSIX path for the IDENTICAL + * location (`/c/Users/...`) — confirmed via this test's own stderr + * diagnostics captured on a live Windows CI run of PR #4552, after three + * prior guesses at the same "--files widened to all 5 files" symptom + * (stripping `-m`, then two rounds of Node-side realpath-expansion + * fixes) each failed identically because none of them was the actual + * cause. The two path forms can never share a string prefix, so Tier 1 + * misclassifies every `--files` entry as "outside the repository" on + * every Windows run, unconditionally and deterministically — a real, + * structural, pre-existing Tier 1 defect, not something introduced or + * fixable by this test. Same out-of-scope bucket as #4461 (this file's + * fences not being cross-platform-robust); not this fix's concern (see + * cr-2 in #4460's review notes). + * + * Seeding REVIEW_FILES directly for both tiers isolates the test to Tier + * 3's own logic — the actual subject of #4460 — independent of either + * earlier tier's unrelated defects. + */ + +const { describe, test } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { spawnSync } = require('node:child_process'); +const { gitOrThrow } = require('./helpers/git-fixture.cjs'); +const { GIT_FIXTURE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); + +const WORKFLOW_PATH = path.join(__dirname, '..', 'gsd-core', 'workflows', 'code-review.md'); + +/** + * Extract the FIRST ```bash fence appearing after `startAnchor` and before + * `stopAnchor` (or end of file when `stopAnchor` is null). + */ +function extractFirstBashBlockAfter(content, startAnchor, stopAnchor) { + const start = content.indexOf(startAnchor); + assert.ok(start !== -1, `code-review.md must contain the anchor "${startAnchor}"`); + const stop = stopAnchor ? content.indexOf(stopAnchor, start + startAnchor.length) : content.length; + assert.ok(!stopAnchor || stop !== -1, `code-review.md must contain the anchor "${stopAnchor}" after "${startAnchor}"`); + const region = content.slice(start, stop); + + const fenceStart = region.indexOf('```bash'); + assert.ok(fenceStart !== -1, `no \`\`\`bash fence found between "${startAnchor}" and its stop anchor`); + const fenceEnd = region.indexOf('```', fenceStart + '```bash'.length); + assert.ok(fenceEnd !== -1, `unterminated \`\`\`bash fence after "${startAnchor}"`); + return region.slice(fenceStart + '```bash'.length, fenceEnd); +} + +function seedFixtureRepo(dir) { + gitOrThrow(['init', '-q'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.email', 't@example.com'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'user.name', 'T'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['config', 'commit.gpgsign', 'false'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +function writeAndCommit(dir, relPath, content, message) { + const abs = path.join(dir, relPath); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, content); + gitOrThrow(['add', '-A'], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); + gitOrThrow(['commit', '-q', '-m', message], { cwd: dir, timeoutMs: GIT_FIXTURE_TIMEOUT_MS }); +} + +// Matches the issue's own fixture exactly: a phase dir with one SUMMARY +// listing only src/alpha.js, and 5 commits adding src/{alpha,beta,gamma, +// delta,epsilon}.js. +function buildFixture(tmpDir) { + seedFixtureRepo(tmpDir); + writeAndCommit(tmpDir, 'README.md', '# init\n', 'chore: init'); + writeAndCommit( + tmpDir, + '.planning/phases/03-demo/03-01-SUMMARY.md', + '---\nkey_files:\n created:\n - src/alpha.js\n---\n# Summary\n', + 'feat(03-01): phase 3 plan 1', + ); + for (const name of ['alpha', 'beta', 'gamma', 'delta', 'epsilon']) { + writeAndCommit(tmpDir, `src/${name}.js`, `${name}\n`, `feat(03-01): add ${name}`); + } +} + +/** + * Runs ONLY Tier 3 (verbatim), seeded with the REVIEW_FILES state a working + * Tier 1 (`filesOverride` set) or Tier 2 (`filesOverride` empty) would have + * produced — see the module docblock for why neither earlier tier's own + * fence is sourced here. + */ +function runTier3(tmpDir, { filesOverride, seedReviewFiles }) { + const content = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const tier3 = extractFirstBashBlockAfter(content, '**Tier 3 — Git diff fallback', '**Post-processing'); + + const seedInit = `REVIEW_FILES=(${seedReviewFiles.map((f) => `"${f}"`).join(' ')})`; + + const script = [ + '#!/usr/bin/env bash', + 'set -uo pipefail', + `FILES_OVERRIDE="${filesOverride || ''}"`, + seedInit, + 'PHASE_DIR=".planning/phases/03-demo"', + 'PADDED_PHASE="03"', + 'LAST_REVIEW_COMMIT=""', + // Tier 3 prints diagnostic "File scope: ... files from git diff" lines to + // stdout as documentation for a human running code-review.md interactively + // — a brace group (not a subshell: variables set inside still persist to + // the enclosing shell) redirects that chatter to stderr (captured + // separately below) instead of discarding it, so a failure carries + // Tier 3's own reasoning rather than requiring another guess-and-push + // cycle. + '{', + tier3, + '} 1>&2', + 'printf \'%s\\n\' "${REVIEW_FILES[@]}"', + ].join('\n'); + + const scriptPath = path.join(tmpDir, '.tier-script.sh'); + fs.writeFileSync(scriptPath, script); + + const result = spawnSync('bash', [scriptPath], { + cwd: tmpDir, + encoding: 'utf8', + timeout: GIT_FIXTURE_TIMEOUT_MS, + }); + if (result.error) { + throw new Error(`bash spawn failed: ${result.error.message}\ndiagnostics:\n${result.stderr || '(none)'}`); + } + if (result.status !== 0) { + throw new Error( + `bash exited ${result.status} (signal ${result.signal})\ndiagnostics:\n${result.stderr || '(none)'}`, + ); + } + // Returned as a plain object, not a decorated array: assert.deepEqual on an + // array compares its own properties too, so an extra property attached + // directly to the array (tried in an earlier revision of this fix) makes + // `["src/alpha.js"]` fail deepEqual against a same-valued plain array + // literal purely because of the decoration -- a self-inflicted false + // failure, not a Tier 3 behavior change. + const files = result.stdout.split('\n').map((l) => l.trim()).filter(Boolean).sort(); + return { files, diagnostics: result.stderr }; +} + +describe('#4460: code-review.md Tier 3 does not widen an explicit --files override', () => { + const workflowContent = fs.readFileSync(WORKFLOW_PATH, 'utf-8'); + const tier3Fence = extractFirstBashBlockAfter(workflowContent, '**Tier 3 — Git diff fallback', '**Post-processing'); + + test('the #2666 cross-check elif references FILES_OVERRIDE (the gate exists)', () => { + const crossCheckIdx = tier3Fence.indexOf('#2666 cross-check'); + assert.ok(crossCheckIdx !== -1, 'Tier 3 fence must contain the #2666 cross-check comment'); + const elifLine = tier3Fence.slice(0, crossCheckIdx).split('\n').filter((l) => l.trim().startsWith('elif')).pop(); + assert.ok( + elifLine && elifLine.includes('FILES_OVERRIDE'), + `the elif guarding the #2666 cross-check must reference FILES_OVERRIDE, got: ${JSON.stringify(elifLine)}`, + ); + }); + + test('real execution: --files=src/alpha.js stays scoped to exactly that file (issue #4460 repro)', () => { + // REVIEW_FILES is seeded to ['src/alpha.js'] directly here -- the value a + // WORKING Tier 1 would have produced for `--files src/alpha.js` -- rather + // than sourcing Tier 1's actual fence. See the module docblock: Tier 1's + // own REPO_ROOT-prefix containment check is confirmed non-functional on + // Windows CI (git rev-parse and realpath disagree on path FORMAT there, + // not just short-vs-long names), so this test isolates Tier 3 -- the + // actual subject of #4460 -- from that unrelated, pre-existing defect. + const tmpDir = fs.realpathSync.native(createTempDir('gsd-4460-')); + try { + buildFixture(tmpDir); + const { files, diagnostics } = runTier3(tmpDir, { filesOverride: 'src/alpha.js', seedReviewFiles: ['src/alpha.js'] }); + assert.deepEqual( + files, + ['src/alpha.js'], + `--files override must not be widened by Tier 3's cross-check, got: ${JSON.stringify(files)}\ndiagnostics:\n${diagnostics || '(none)'}`, + ); + } finally { + cleanup(tmpDir); + } + }); + + test('without --files, the #2666 cross-check still widens a partial (Tier-2-equivalent) scope (no regression to the cross-check itself)', () => { + const tmpDir = fs.realpathSync.native(createTempDir('gsd-4460-')); + try { + buildFixture(tmpDir); + // seedReviewFiles stands in for Tier 2's real output (["src/alpha.js"], + // the file the fixture's SUMMARY lists) — see the module docblock for + // why Tier 2's own fence isn't sourced here. Same seed value as the + // --files case above; only FILES_OVERRIDE differs, which is exactly + // the variable #4460's fix gates on. + const { files, diagnostics } = runTier3(tmpDir, { filesOverride: '', seedReviewFiles: ['src/alpha.js'] }); + assert.deepEqual( + files, + ['src/alpha.js', 'src/beta.js', 'src/delta.js', 'src/epsilon.js', 'src/gamma.js'], + `without --files, the cross-check must still widen a partial scope, got: ${JSON.stringify(files)}\ndiagnostics:\n${diagnostics || '(none)'}`, + ); + } finally { + cleanup(tmpDir); + } + }); +}); diff --git a/tests/fixtures/install-tree/antigravity.json b/tests/fixtures/install-tree/antigravity.json index cc183f68f..a23840bb1 100644 --- a/tests/fixtures/install-tree/antigravity.json +++ b/tests/fixtures/install-tree/antigravity.json @@ -565,6 +565,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/augment.json b/tests/fixtures/install-tree/augment.json index b992f115c..82f1531a5 100644 --- a/tests/fixtures/install-tree/augment.json +++ b/tests/fixtures/install-tree/augment.json @@ -636,6 +636,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/fixtures/install-tree/claude-local.json b/tests/fixtures/install-tree/claude-local.json index f1b618e33..00de5e628 100644 --- a/tests/fixtures/install-tree/claude-local.json +++ b/tests/fixtures/install-tree/claude-local.json @@ -529,5 +529,6 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs" ] diff --git a/tests/fixtures/install-tree/claude.json b/tests/fixtures/install-tree/claude.json index ed3264b0b..d29e72191 100644 --- a/tests/fixtures/install-tree/claude.json +++ b/tests/fixtures/install-tree/claude.json @@ -564,6 +564,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/cline.json b/tests/fixtures/install-tree/cline.json index 984237fee..8eb1c54fe 100644 --- a/tests/fixtures/install-tree/cline.json +++ b/tests/fixtures/install-tree/cline.json @@ -526,6 +526,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/fixtures/install-tree/codebuddy.json b/tests/fixtures/install-tree/codebuddy.json index 70b80da79..dccaaa248 100644 --- a/tests/fixtures/install-tree/codebuddy.json +++ b/tests/fixtures/install-tree/codebuddy.json @@ -636,6 +636,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/codex.json b/tests/fixtures/install-tree/codex.json index 5207c1e72..1f32a8d64 100644 --- a/tests/fixtures/install-tree/codex.json +++ b/tests/fixtures/install-tree/codex.json @@ -564,5 +564,6 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs" ] diff --git a/tests/fixtures/install-tree/copilot.json b/tests/fixtures/install-tree/copilot.json index 0595dfafd..35234a3af 100644 --- a/tests/fixtures/install-tree/copilot.json +++ b/tests/fixtures/install-tree/copilot.json @@ -526,6 +526,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/cursor.json b/tests/fixtures/install-tree/cursor.json index 3224fdbbe..226913179 100644 --- a/tests/fixtures/install-tree/cursor.json +++ b/tests/fixtures/install-tree/cursor.json @@ -537,6 +537,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/hermes.json b/tests/fixtures/install-tree/hermes.json index c3fa84602..cfe49592a 100644 --- a/tests/fixtures/install-tree/hermes.json +++ b/tests/fixtures/install-tree/hermes.json @@ -564,6 +564,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd/DESCRIPTION.md", "skills/gsd/gsd-ns-context/SKILL.md", diff --git a/tests/fixtures/install-tree/kilo.json b/tests/fixtures/install-tree/kilo.json index 57ae55781..14ab18ca1 100644 --- a/tests/fixtures/install-tree/kilo.json +++ b/tests/fixtures/install-tree/kilo.json @@ -639,6 +639,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/kimi-code.json b/tests/fixtures/install-tree/kimi-code.json index 6b349c943..d03ce00ca 100644 --- a/tests/fixtures/install-tree/kimi-code.json +++ b/tests/fixtures/install-tree/kimi-code.json @@ -565,6 +565,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/kimi.json b/tests/fixtures/install-tree/kimi.json index 96768ce91..f7e402034 100644 --- a/tests/fixtures/install-tree/kimi.json +++ b/tests/fixtures/install-tree/kimi.json @@ -561,6 +561,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/opencode.json b/tests/fixtures/install-tree/opencode.json index 90037c0f8..cdd5a8952 100644 --- a/tests/fixtures/install-tree/opencode.json +++ b/tests/fixtures/install-tree/opencode.json @@ -639,6 +639,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-add-tests/SKILL.md", "skills/gsd-ai-integration-phase/SKILL.md", diff --git a/tests/fixtures/install-tree/pi.json b/tests/fixtures/install-tree/pi.json index d595d2517..65e02bf89 100644 --- a/tests/fixtures/install-tree/pi.json +++ b/tests/fixtures/install-tree/pi.json @@ -424,5 +424,6 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs" ] diff --git a/tests/fixtures/install-tree/qwen.json b/tests/fixtures/install-tree/qwen.json index 70bb5c5f5..9ae67ea69 100644 --- a/tests/fixtures/install-tree/qwen.json +++ b/tests/fixtures/install-tree/qwen.json @@ -564,6 +564,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/fixtures/install-tree/trae.json b/tests/fixtures/install-tree/trae.json index 2e94faba8..9b6f0f653 100644 --- a/tests/fixtures/install-tree/trae.json +++ b/tests/fixtures/install-tree/trae.json @@ -524,6 +524,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/fixtures/install-tree/windsurf.json b/tests/fixtures/install-tree/windsurf.json index 507dcad99..f8475a578 100644 --- a/tests/fixtures/install-tree/windsurf.json +++ b/tests/fixtures/install-tree/windsurf.json @@ -459,5 +459,6 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs" ] diff --git a/tests/fixtures/install-tree/zcode.json b/tests/fixtures/install-tree/zcode.json index 5cfa9c9a0..4394671ab 100644 --- a/tests/fixtures/install-tree/zcode.json +++ b/tests/fixtures/install-tree/zcode.json @@ -596,6 +596,7 @@ "scripts/lib/drift-scan.cjs", "scripts/lib/exit-code-registry.cjs", "scripts/lib/ndjson-reporter.cjs", + "scripts/lib/npm-version-check-diagnosis.cjs", "scripts/lib/shellcheck-fetch.cjs", "skills/gsd-ns-context/SKILL.md", "skills/gsd-ns-context/skills/docs-update/SKILL.md", diff --git a/tests/npm-version-check-diagnosis.test.cjs b/tests/npm-version-check-diagnosis.test.cjs new file mode 100644 index 000000000..83cdb3c91 --- /dev/null +++ b/tests/npm-version-check-diagnosis.test.cjs @@ -0,0 +1,71 @@ +'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 (defensive default) falls back to the original message', () => { + const result = { exitCode: 0, stdout: '', stderr: '', signal: null, error: null, timedOut: false }; + assert.equal( + describeNpmVersionCheckFailure(result), + 'npm binary not found on PATH', + ); + }); +});