diff --git a/.nvmrc b/.nvmrc new file mode 100644 index 000000000..2bd5a0a98 --- /dev/null +++ b/.nvmrc @@ -0,0 +1 @@ +22 diff --git a/CLAUDE.md b/CLAUDE.md index 6bc846c03..c60e451ac 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -248,4 +248,4 @@ This repo uses [Greptile](https://greptile.com) for automated PR reviews. After ## Node Version -Requires Node >= 22.12.0. +Requires Node >= 22.12.0. CI (`.github/workflows/ci.yml`) pins Node 22 everywhere; `.nvmrc` mirrors that. If your local Node major version diverges from CI's — `npm run doctor` (also run automatically as `pretest`) warns loudly when it does — treat a full local `npm test` run as untrustworthy for cross-checking against CI: native-addon/V8 behavioral drift between major versions can produce a large, unrelated-looking wall of failures that aren't real regressions. Cross-check any suspicious local failure against CI (or an untouched `origin/main` checkout in the same local environment) before treating it as caused by your change. diff --git a/src/infrastructure/doctor.ts b/src/infrastructure/doctor.ts index d14ed62d4..2e35e36e7 100644 --- a/src/infrastructure/doctor.ts +++ b/src/infrastructure/doctor.ts @@ -41,10 +41,14 @@ * This module only detects — it never mutates the environment. `scripts/doctor.ts` * is the CLI entry point that also knows how to *fix* what this module reports. */ -import { readdirSync } from 'node:fs'; +import { readdirSync, readFileSync } from 'node:fs'; import { createRequire } from 'node:module'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; import type { LanguageRegistryEntry } from '../types.js'; +const REPO_ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..'); + const _require = createRequire(import.meta.url); /** @@ -312,18 +316,100 @@ export function checkWasmGrammars( }; } +/** + * Parse a `.nvmrc`-style file's content into a Node major version number. + * Not exported — only `checkNodeVersion`'s default reader produces this + * input; tests exercise it indirectly via that check's injectable reader. + * Tolerates a leading `v`, a full semver (`22.12.0`), trailing whitespace, + * and a trailing comment. Returns null for a non-numeric value (e.g. an + * `lts/*` alias) rather than guessing — this check only ever warns, so a + * silently-wrong comparison would be worse than skipping it. + */ +function parseNvmrcMajor(content: string): number | null { + const firstLine = content.split('\n')[0]?.trim() ?? ''; + const versionPart = firstLine.split('#')[0]?.trim() ?? ''; + const match = /^v?(\d+)/.exec(versionPart); + return match ? Number(match[1]) : null; +} + +/** Read `.nvmrc` from the repo root, if present. Returns null when absent or unreadable. */ +function readNvmrc(): string | null { + try { + return readFileSync(join(REPO_ROOT, '.nvmrc'), 'utf-8'); + } catch { + return null; + } +} + +/** + * Warn (never fail) when the running Node major version doesn't match + * `.nvmrc` (issue #2521): a full local `npm test` run on a Node version CI + * doesn't pin can produce a large, unrelated-looking wall of failures from + * native-addon/V8 behavioral drift, not actual regressions — wasting time + * triaging phantom failures or masking a real one in the noise. This is + * purely informational (a mismatch doesn't mean the environment is broken, + * just that local results shouldn't be trusted over CI's), so it only ever + * warns, matching the "missing optional grammar" precedent above — never + * blocks `pretest`/`npm test`. + * + * `readFile`/`currentVersion` are injectable so tests can simulate a + * missing `.nvmrc`, an unparseable one, a match, and a mismatch without + * touching the real file or `process.version`. + */ +export function checkNodeVersion( + readFile: () => string | null = readNvmrc, + currentVersion: string = process.version, +): DoctorCheck { + const id = 'node-version'; + const label = 'Node version vs .nvmrc'; + + const nvmrcContent = readFile(); + if (nvmrcContent === null) { + return { id, label, status: 'ok', detail: 'no .nvmrc present — nothing to compare against' }; + } + + const expectedMajor = parseNvmrcMajor(nvmrcContent); + if (expectedMajor === null) { + return { + id, + label, + status: 'ok', + detail: `.nvmrc content is not a plain version number (${nvmrcContent.trim()}) — skipping comparison`, + }; + } + + const currentMajor = Number(/^v?(\d+)/.exec(currentVersion)?.[1]); + if (currentMajor === expectedMajor) { + return { id, label, status: 'ok', detail: `running Node ${currentVersion}, matches .nvmrc` }; + } + + return { + id, + label, + status: 'warn', + detail: + `running Node ${currentVersion}, but .nvmrc pins ${expectedMajor} — a full local test ` + + `run can show unrelated-looking failures from native-addon/V8 drift between major ` + + `versions; cross-check against CI (which pins ${expectedMajor}) before treating them as regressions`, + }; +} + /** * Run every doctor check and aggregate the result. Fast and read-only — safe - * to call frequently (e.g. from a `pretest` hook) since neither check spawns - * a subprocess or mutates the environment. + * to call frequently (e.g. from a `pretest` hook) since no check spawns a + * subprocess or mutates the environment. * - * `checks` is injectable (defaulting to the two real checks) so the + * `checks` is injectable (defaulting to the three real checks) so the * ok/'fail'-only aggregation rule — a 'warn' must never flip the overall * report unhealthy — can be unit tested directly with fake check results, - * independent of the real native binary or grammars/ directory. + * independent of the real native binary, grammars/ directory, or Node version. */ export function runDoctorChecks( - checks: readonly DoctorCheck[] = [checkBetterSqlite3Abi(), checkWasmGrammars()], + checks: readonly DoctorCheck[] = [ + checkBetterSqlite3Abi(), + checkWasmGrammars(), + checkNodeVersion(), + ], ): DoctorReport { return { ok: checks.every((c) => c.status !== 'fail'), checks: [...checks] }; } diff --git a/tests/unit/doctor.test.ts b/tests/unit/doctor.test.ts index 1cb19a9ed..0529381e3 100644 --- a/tests/unit/doctor.test.ts +++ b/tests/unit/doctor.test.ts @@ -22,6 +22,7 @@ import { afterEach, describe, expect, it } from 'vitest'; import { LANGUAGE_REGISTRY } from '../../src/domain/parser.js'; import { checkBetterSqlite3Abi, + checkNodeVersion, checkWasmGrammars, findMissingGrammars, parseAbiMismatchError, @@ -294,17 +295,22 @@ describe('runDoctorChecks', () => { expect(report.ok).toBe(true); }); - it('runs both real checks against the real environment and returns a well-formed report', () => { + it('runs all real checks against the real environment and returns a well-formed report', () => { // Uses the real defaults (real require('better-sqlite3'), real grammars/ - // directory) — by the time this test runs, `pretest` has already gated - // `npm test` on a healthy environment, but this asserts shape rather than - // hard-coding a specific status so a single-file run on a mid-repair - // machine doesn't fail on an unrelated assertion. + // directory, real .nvmrc) — by the time this test runs, `pretest` has + // already gated `npm test` on a healthy environment, but this asserts + // shape rather than hard-coding a specific status so a single-file run + // on a mid-repair machine (or one whose Node version doesn't match + // .nvmrc) doesn't fail on an unrelated assertion. const report = runDoctorChecks(); expect(typeof report.ok).toBe('boolean'); - expect(report.checks).toHaveLength(2); - expect(report.checks.map((c) => c.id)).toEqual(['better-sqlite3-abi', 'wasm-grammars']); + expect(report.checks).toHaveLength(3); + expect(report.checks.map((c) => c.id)).toEqual([ + 'better-sqlite3-abi', + 'wasm-grammars', + 'node-version', + ]); for (const check of report.checks) { expect(['ok', 'warn', 'fail']).toContain(check.status); expect(typeof check.label).toBe('string'); @@ -314,3 +320,54 @@ describe('runDoctorChecks', () => { expect(report.ok).toBe(report.checks.every((c) => c.status !== 'fail')); }); }); + +describe('checkNodeVersion', () => { + it('reports ok when no .nvmrc is present', () => { + const result = checkNodeVersion(() => null, 'v22.12.0'); + expect(result.status).toBe('ok'); + expect(result.id).toBe('node-version'); + expect(result.detail).toContain('no .nvmrc'); + }); + + it('reports ok when .nvmrc content is not a plain version number', () => { + const result = checkNodeVersion(() => 'lts/*\n', 'v22.12.0'); + expect(result.status).toBe('ok'); + expect(result.detail).toContain('not a plain version number'); + }); + + it('reports ok when the running major matches .nvmrc exactly', () => { + const result = checkNodeVersion(() => '22\n', 'v22.12.0'); + expect(result.status).toBe('ok'); + expect(result.detail).toContain('matches .nvmrc'); + }); + + it('reports ok when .nvmrc pins a full semver whose major matches', () => { + const result = checkNodeVersion(() => '22.12.0\n', 'v22.4.1'); + expect(result.status).toBe('ok'); + }); + + it('tolerates a leading v and a trailing comment in .nvmrc', () => { + const result = checkNodeVersion(() => 'v22 # pinned to match CI\n', 'v22.12.0'); + expect(result.status).toBe('ok'); + }); + + // Regression test for issue #2521: this exact scenario (Node 26 installed + // locally, CI pins Node 22) produced 200+ unrelated-looking local test + // failures that reproduced identically on an untouched origin/main. + it('warns — never fails — when the running major does not match .nvmrc', () => { + const result = checkNodeVersion(() => '22\n', 'v26.4.0'); + expect(result.status).toBe('warn'); + expect(result.status).not.toBe('fail'); + expect(result.detail).toContain('v26.4.0'); + expect(result.detail).toContain('.nvmrc pins 22'); + expect(result.detail).toContain('cross-check against CI'); + }); + + it('a Node-version warning does not flip the overall report unhealthy', () => { + const report = runDoctorChecks([ + { id: 'a', label: 'A', status: 'ok', detail: 'fine' }, + checkNodeVersion(() => '22\n', 'v26.4.0'), + ]); + expect(report.ok).toBe(true); + }); +});