From ac9b05177942628fb68263dfd345b2a17e7de86f Mon Sep 17 00:00:00 2001 From: thewrz Date: Sat, 1 Aug 2026 15:19:27 -0700 Subject: [PATCH 1/3] chore(ci): pin DMXr to Node 24 LTS and enforce it end to end DMXr declared its Node version in four places that disagreed with each other and with reality: engines.node was the open-ended ">=18.0.0" (Node 18 has been EOL since 2025-04-30), @types/node was ^25.7.0 -- a major that is itself EOL and never was an LTS -- CI hardcoded Node 22 in five separate spots, and the maintainer's machine ran Node 26.4.0. Nothing related any of them, so nothing could notice. The @types/node 25 against a Node 22 runtime is the costly shape: tsc validates an API surface the runtime does not have, and a typecheck catches shape errors, not behavioral drift. Agent-assisted implementation compounds it, since models lag current releases and emit code against APIs they do not reliably know. Node 24 (LTS since 2025-10-28, EOL 2028-04-30) is the oldest Active LTS still reasonable, and sits inside the training window of every model used here. Node 26 does not, and stays off-limits past its 2026-10-28 LTS date. Unlike a CI-only pin, this one is also a product decision: build-server.yml bundles a portable Node runtime into the release artifact, downloading whatever version setup-node resolved. .nvmrc now governs what ships to users. Enforced rather than documented: - .nvmrc (24) is the single source of truth. All five setup-node call sites read node-version-file; no hardcoded majors remain. - engines.node is the bounded ">=24 <25", plus engine-strict=true in server/.npmrc. Both halves are load-bearing -- measured on npm 11.16.0: engines .npmrc runtime npm install ">=24 <25" (none) Node 26 exit 0 -- silent ">=24" (open) engine-strict=true Node 26 exit 0 -- range satisfied ">=24 <25" engine-strict=true Node 26 exit 1 <- ">=24 <25" engine-strict=true Node 24 exit 0 <- This is npm-specific and was re-measured here rather than assumed: a sibling pnpm repo found the same .npmrc spelling completely inert under pnpm 11. - @types/node tracks the runtime major (^24.13.3). tsc is clean after the downgrade -- no fallout. - A new node-pin CI job runs check:node-pin, catching the case engine-strict cannot: declarations that are each valid but have drifted apart. - Renovate gets constraintsFiltering on runtime deps, Node majors disabled, and node-version/@types/node bounded <25. Dependabot has no engines-awareness at all, so it gets an explicit @types/node major ignore with CI as the backstop. The gate's comparison logic lives in src/config/node-pin.ts, not in scripts/, because tsconfig includes only src/**/*.ts and vitest collects only src/**/*.test.ts -- a script would have been neither typechecked nor tested. tsconfig.scripts.json closes that gap for the thin I/O runner and anything else added to scripts/ later. Verified in real node:24 and node:26 containers, since local Node 26 is now correctly rejected: npm ci, check:node-pin, typecheck, typecheck:scripts, build and audit (0 vulnerabilities) all pass under Node 24; 1661 tests pass, 11 skipped. Negative test: npm ci under Node 26 exits 1 with expected-vs-actual. Heads-up: this breaks local installs until the machine is on Node 24, by design. A version manager reading .nvmrc is the fix (fnm use / nvm use / mise install). Closes #126. Co-Authored-By: Claude Opus 5 --- .github/dependabot.yml | 16 +++ .github/workflows/build-server.yml | 6 +- .github/workflows/ci.yml | 32 +++++- .github/workflows/dependency-health.yml | 2 +- .nvmrc | 1 + CLAUDE.md | 20 +++- renovate.json | 22 ++++ server/.npmrc | 1 + server/package-lock.json | 18 +-- server/package.json | 9 +- server/scripts/check-node-pin.ts | 101 +++++++++++++++++ server/src/config/README.md | 8 ++ server/src/config/node-pin.test.ts | 132 ++++++++++++++++++++++ server/src/config/node-pin.ts | 139 ++++++++++++++++++++++++ server/tsconfig.scripts.json | 16 +++ 15 files changed, 500 insertions(+), 23 deletions(-) create mode 100644 .nvmrc create mode 100644 server/.npmrc create mode 100644 server/scripts/check-node-pin.ts create mode 100644 server/src/config/node-pin.test.ts create mode 100644 server/src/config/node-pin.ts create mode 100644 server/tsconfig.scripts.json diff --git a/.github/dependabot.yml b/.github/dependabot.yml index db4b08a..16fe6ee 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -5,6 +5,18 @@ # enabled here so dependency updates stay visible, and so the two tools can be # compared side by side before we settle on one. Expect duplicate PRs until then # -- that is intentional, not a misconfiguration. +# +# Node pin (issue #126): Dependabot has NO equivalent of Renovate's +# `constraintsFiltering`. Its ignore/allow/versioning-strategy/groups levers all +# operate on semver update *type*, never on runtime compatibility, so it cannot be +# told "only propose updates whose engines.node overlaps ours". +# +# Two mitigations, since it stays enabled for coverage: +# 1. `@types/node` majors are ignored below, so it cannot re-propose the +# typings-ahead-of-runtime drift that issue #126 exists to fix. +# 2. Everything else relies on CI: the `node-pin` job fails on declaration +# drift, and npm's `engine-strict` (server/.npmrc) fails the install itself +# on a wrong runtime. Treat any Dependabot npm PR as needing an engines glance. version: 2 updates: @@ -20,6 +32,10 @@ updates: update-types: - "minor" - "patch" + ignore: + # Node pin (issue #126): @types/node tracks the runtime major, never leads it. + - dependency-name: "@types/node" + update-types: ["version-update:semver-major"] - package-ecosystem: "github-actions" directory: "/" diff --git a/.github/workflows/build-server.yml b/.github/workflows/build-server.yml index 5d6dc10..674a530 100644 --- a/.github/workflows/build-server.yml +++ b/.github/workflows/build-server.yml @@ -39,10 +39,12 @@ jobs: - name: Checkout uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - - name: Setup Node.js 22 + # The runtime resolved here is the one bundled into the release artifact + # below, so .nvmrc pins what ships to users, not just what CI builds with. + - name: Setup Node.js (pinned by .nvmrc) uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 22 + node-version-file: .nvmrc - name: Install dependencies working-directory: server diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0047bf5..e1545de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -12,7 +12,7 @@ jobs: - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 22 + node-version-file: .nvmrc - name: Install dependencies working-directory: server @@ -20,7 +20,31 @@ jobs: - name: Type-check working-directory: server - run: npx tsc --noEmit + run: npm run typecheck + + - name: Type-check scripts + working-directory: server + run: npm run typecheck:scripts + + node-pin: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + + - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version-file: .nvmrc + + - name: Install dependencies + working-directory: server + run: npm ci + + # `npm ci` above already rejects a wrong runtime via engine-strict. This + # catches the other half: declarations that are each valid but have drifted + # apart -- the @types/node-ahead-of-runtime shape from issue #126. + - name: Check Node pin + working-directory: server + run: npm run check:node-pin test: runs-on: ubuntu-latest @@ -29,7 +53,7 @@ jobs: - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 22 + node-version-file: .nvmrc - name: Install dependencies working-directory: server @@ -46,7 +70,7 @@ jobs: - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 22 + node-version-file: .nvmrc - name: Install dependencies working-directory: server diff --git a/.github/workflows/dependency-health.yml b/.github/workflows/dependency-health.yml index 1d94b3b..e62397f 100644 --- a/.github/workflows/dependency-health.yml +++ b/.github/workflows/dependency-health.yml @@ -18,7 +18,7 @@ jobs: - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 22 + node-version-file: .nvmrc cache: npm cache-dependency-path: server/package-lock.json diff --git a/.nvmrc b/.nvmrc new file mode 100644 index 0000000..a45fd52 --- /dev/null +++ b/.nvmrc @@ -0,0 +1 @@ +24 diff --git a/CLAUDE.md b/CLAUDE.md index 7ebb7ca..6d79ad7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -8,13 +8,15 @@ DMXr bridges DMX lighting fixtures into SignalRGB as first-class canvas devices. ``` DMXr/ +├── .nvmrc # Node major (24 LTS) -- single source of truth, read by CI + version managers ├── DMXr.js # SignalRGB plugin (UDP/HTTP color transport) ├── DMXr.qml # SignalRGB settings panel UI ├── docs/images/ # SVG logos, fixture icons └── server/ + ├── .npmrc # engine-strict=true -- a wrong Node major fails `npm ci` ├── src/ # All server TypeScript │ ├── bootstrap/ # Startup orchestration (DMX, library, shutdown) - │ ├── config/ # Settings, remap-preset, server-config stores + │ ├── config/ # Settings, remap-preset, server-config stores, Node-pin check │ ├── dmx/ # Universe manager, dispatcher, connection pool, monitor, driver factory │ ├── fixtures/ # Fixture/group/user-fixture stores, color pipeline, channel mapper │ ├── libraries/ # Library registry (OFL + user fixtures) @@ -31,6 +33,7 @@ DMXr/ │ ├── ui/ # Frontend helpers (channel labels, CSS theming, OFL conversion) │ ├── ui-tests/ # Playwright E2E tests (grid, CRUD, settings, multi-select) │ └── utils/ # Formatting, validation helpers + ├── scripts/ # Dev/ops entry points (service installers, check-node-pin) ├── public/ # Alpine.js web UI (no build step) │ ├── js/ # app.js + 31 mixin files │ └── css/ # Feature-scoped CSS files @@ -66,11 +69,20 @@ Browser (http://localhost:8080) SignalRGB Plugin (DMXr.js) ## Development +**Node 24 LTS is required and enforced** (issue #126). `.nvmrc` is the single source of +truth; `server/.npmrc` sets `engine-strict=true`, so `npm ci` **fails** on any other major +rather than warning. Use a version manager that reads `.nvmrc` (`fnm use` / `nvm use` / +`mise install`). Never bump the Node major as a routine dependency PR -- it is one +coordinated change across `.nvmrc`, `engines.node`, `@types/node`, CI, and the runtime that +`build-server.yml` bundles into the release artifact. + ```bash cd server -npm test # vitest run (tests co-located: *.test.ts next to source) -npx tsc --noEmit # type check (strict mode) -- the clean-check; no separate ESLint -npm run build # tsc -> dist/ +npm test # vitest run (tests co-located: *.test.ts next to source) +npm run typecheck # tsc --noEmit (strict mode) -- the clean-check; no separate ESLint +npm run typecheck:scripts # same strict flags over scripts/ (outside the build's rootDir) +npm run check:node-pin # assert every Node declaration names the same major +npm run build # tsc -> dist/ ``` ## Key Conventions diff --git a/renovate.json b/renovate.json index b67d648..4d427ec 100644 --- a/renovate.json +++ b/renovate.json @@ -15,6 +15,28 @@ { "matchManagers": ["npm"], "groupName": "npm-server" + }, + { + "description": "Node pin (issue #126): only propose npm releases whose own engines.node overlaps ours. The constraint is auto-detected from engines.node and .nvmrc -- deliberately NOT declared in a `constraints` block, because Renovate treats a manually-set constraint as fixed and will never offer to bump it, forking the source of truth. Scoped to runtime dependencies per Renovate's guidance that strict filtering across devDependencies filters far more than most users expect.", + "matchManagers": ["npm"], + "matchDepTypes": ["dependencies"], + "constraintsFiltering": "strict" + }, + { + "description": "Node pin (issue #126): never propose a Node MAJOR bump. Moving LTS majors touches .nvmrc, engines.node, @types/node, CI and the runtime bundled into the release artifact -- a coordinated decision, not a routine dependency PR. Minor/patch within the pinned major stays enabled so security patches still flow.", + "matchDepNames": ["node"], + "matchUpdateTypes": ["major"], + "enabled": false + }, + { + "description": "Node pin (issue #126): keep .nvmrc and any workflow node-version on the pinned major. Bounded rather than disabled so 24.x patches still arrive.", + "matchDatasources": ["node-version"], + "allowedVersions": "<25.0.0" + }, + { + "description": "Node pin (issue #126): @types/node must track the runtime major, never lead it. A typings-only bump past the runtime lets tsc accept APIs that do not exist at execution -- this repo was sitting at @types/node 25 against a Node 22 CI, and Node 25 is EOL and never was an LTS.", + "matchPackageNames": ["@types/node"], + "allowedVersions": "<25.0.0" } ] } diff --git a/server/.npmrc b/server/.npmrc new file mode 100644 index 0000000..b6f27f1 --- /dev/null +++ b/server/.npmrc @@ -0,0 +1 @@ +engine-strict=true diff --git a/server/package-lock.json b/server/package-lock.json index 6fc9fa7..e569251 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -21,7 +21,7 @@ "devDependencies": { "@playwright/test": "^1.60.0", "@types/better-sqlite3": "^7.6.13", - "@types/node": "^25.7.0", + "@types/node": "^24.13.3", "@types/pngjs": "^6.0.5", "@vitest/coverage-v8": "^4.1.6", "fast-check": "^4.8.0", @@ -34,7 +34,7 @@ "zod": "^4.4.3" }, "engines": { - "node": ">=18.0.0" + "node": ">=24 <25" } }, "node_modules/@babel/code-frame": { @@ -1630,12 +1630,12 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "25.7.0", - "resolved": "https://registry.npmjs.org/@types/node/-/node-25.7.0.tgz", - "integrity": "sha512-z+pdZyxE+RTQE9AcboAZCb4otwcrvgHD+GlBpPgn0emDVt0ohrTMhAwlr2Wd9nZ+nihhYFxO2pThz3C5qSu2Eg==", + "version": "24.13.3", + "resolved": "https://registry.npmjs.org/@types/node/-/node-24.13.3.tgz", + "integrity": "sha512-Dh8vAsV36ig5wa9OX4pXvMc9D3Veibfw2wix0CUwYODLD8nkj9UsLjASr49nPg+2eKzxhBV+v7L8pXvT4e639Q==", "license": "MIT", "dependencies": { - "undici-types": "~7.21.0" + "undici-types": "~7.18.0" } }, "node_modules/@types/pngjs": { @@ -5188,9 +5188,9 @@ } }, "node_modules/undici-types": { - "version": "7.21.0", - "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.21.0.tgz", - "integrity": "sha512-w9IMgQrz4O0YN1LtB7K5P63vhlIOvC7opSmouCJ+ZywlPAlO9gIkJ+otk6LvGpAs2wg4econaCz3TvQ9xPoyuQ==", + "version": "7.18.2", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.18.2.tgz", + "integrity": "sha512-AsuCzffGHJybSaRrmr5eHr81mwJU3kjw6M+uprWvCXiNeN9SOGwQ3Jn8jb8m3Z6izVgknn1R0FTCEAP2QrLY/w==", "license": "MIT" }, "node_modules/util-deprecate": { diff --git a/server/package.json b/server/package.json index 9d5a341..d096cf1 100644 --- a/server/package.json +++ b/server/package.json @@ -15,7 +15,10 @@ "test:ui:watch": "vitest --config vitest.config.ui.ts", "test:ui:update": "UPDATE_BASELINES=1 vitest run --config vitest.config.ui.ts", "test:contract": "vitest run --config vitest.config.contract.ts", - "test:e2e": "playwright test" + "test:e2e": "playwright test", + "typecheck": "tsc --noEmit", + "typecheck:scripts": "tsc -p tsconfig.scripts.json", + "check:node-pin": "tsx scripts/check-node-pin.ts" }, "dependencies": { "@fastify/cors": "^11.2.0", @@ -30,7 +33,7 @@ "devDependencies": { "@playwright/test": "^1.60.0", "@types/better-sqlite3": "^7.6.13", - "@types/node": "^25.7.0", + "@types/node": "^24.13.3", "@types/pngjs": "^6.0.5", "@vitest/coverage-v8": "^4.1.6", "fast-check": "^4.8.0", @@ -43,6 +46,6 @@ "zod": "^4.4.3" }, "engines": { - "node": ">=18.0.0" + "node": ">=24 <25" } } diff --git a/server/scripts/check-node-pin.ts b/server/scripts/check-node-pin.ts new file mode 100644 index 0000000..64af08c --- /dev/null +++ b/server/scripts/check-node-pin.ts @@ -0,0 +1,101 @@ +/** + * Fails the build when DMXr's Node version declarations drift apart (issue #126). + * + * This is the I/O shell only — the comparison logic lives in + * `src/config/node-pin.ts`, where tsconfig typechecks it and vitest covers it. + * + * Run with `npm run check:node-pin`. + */ +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { checkNodePin } from "../src/config/node-pin.js"; +import type { NodePinInputs } from "../src/config/node-pin.js"; + +const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)); +const SERVER_DIR = join(SCRIPT_DIR, ".."); +const REPO_ROOT = join(SERVER_DIR, ".."); + +function read(path: string): string { + try { + return readFileSync(path, "utf8"); + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error(`Cannot read ${path}: ${reason}`); + } +} + +interface ServerManifest { + readonly engines?: { readonly node?: string }; + readonly devDependencies?: Readonly>; +} + +function readManifest(path: string): ServerManifest { + try { + return JSON.parse(read(path)) as ServerManifest; + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error(`Cannot parse ${path} as JSON: ${reason}`); + } +} + +function collectInputs(): NodePinInputs { + const manifestPath = join(SERVER_DIR, "package.json"); + const manifest = readManifest(manifestPath); + + const enginesNode = manifest.engines?.node; + if (enginesNode === undefined) { + throw new Error(`${manifestPath} declares no engines.node — the pin needs one.`); + } + + const typesNode = manifest.devDependencies?.["@types/node"]; + if (typesNode === undefined) { + throw new Error( + `${manifestPath} declares no devDependencies["@types/node"] — the pin needs one.`, + ); + } + + return { + nvmrc: read(join(REPO_ROOT, ".nvmrc")), + enginesNode, + typesNode, + runtimeVersion: process.version, + }; +} + +function main(): void { + const result = checkNodePin(collectInputs()); + + if (result.consistent) { + console.log( + `node-pin: consistent — Node ${result.major} across ` + + `${result.declarations.length} declarations and the running runtime.`, + ); + return; + } + + console.error("node-pin: FAILED — Node version declarations disagree.\n"); + for (const declaration of result.declarations) { + console.error( + ` ${declaration.source.padEnd(38)} ${declaration.raw.trim()}` + + `${declaration.major === null ? " (unparseable)" : ""}`, + ); + } + console.error(""); + for (const problem of result.problems) { + console.error(` - ${problem}`); + } + console.error( + "\nSee issue #126. `.nvmrc` is the source of truth; bring the others to match it.", + ); + process.exitCode = 1; +} + +try { + main(); +} catch (error) { + console.error( + `node-pin: could not run — ${error instanceof Error ? error.message : String(error)}`, + ); + process.exitCode = 1; +} diff --git a/server/src/config/README.md b/server/src/config/README.md index 8d43790..c43a9f0 100644 --- a/server/src/config/README.md +++ b/server/src/config/README.md @@ -14,6 +14,14 @@ - Auto-generates `serverId` (UUID) on first load if missing - Always returns defensive copies (`{ ...current }`) to prevent external mutation +### node-pin.ts +- `parseMajor(raw)`, `isBoundedRange(range)`, `checkNodePin(inputs)` -> `NodePinCheckResult` +- Pure comparison logic for the Node 24 LTS pin (issue #126): asserts `.nvmrc`, + `engines.node`, `@types/node` and the running interpreter all name the same major, + and that `engines.node` is bounded rather than an open-ended floor +- No I/O -- `server/scripts/check-node-pin.ts` supplies the files and the exit code. + Lives here so tsconfig typechecks it and vitest covers it; `scripts/` is outside both. + ### remap-preset-store.ts - `createRemapPresetStore(filePath)` -> `RemapPresetStore { load, getAll, get, upsert, remove, save }` - Stores named channel-remap presets (channelCount + offset mapping) diff --git a/server/src/config/node-pin.test.ts b/server/src/config/node-pin.test.ts new file mode 100644 index 0000000..49eb461 --- /dev/null +++ b/server/src/config/node-pin.test.ts @@ -0,0 +1,132 @@ +import { describe, it, expect } from "vitest"; +import { parseMajor, isBoundedRange, checkNodePin } from "./node-pin.js"; +import type { NodePinInputs } from "./node-pin.js"; + +const consistentInputs: NodePinInputs = { + nvmrc: "24", + enginesNode: ">=24 <25", + typesNode: "^24.13.3", + runtimeVersion: "v24.18.1", +}; + +describe("parseMajor", () => { + it("reads a bare major", () => { + expect(parseMajor("24")).toBe(24); + }); + + it("reads a v-prefixed runtime version", () => { + expect(parseMajor("v24.18.1")).toBe(24); + }); + + it("reads a caret range", () => { + expect(parseMajor("^24.13.3")).toBe(24); + }); + + it("reads the lower bound of a bounded range", () => { + expect(parseMajor(">=24 <25")).toBe(24); + }); + + it("reads a floor range", () => { + expect(parseMajor(">=18.0.0")).toBe(18); + }); + + it("tolerates surrounding whitespace and newlines", () => { + expect(parseMajor(" 24\n")).toBe(24); + }); + + it("returns null when no major is present", () => { + expect(parseMajor("latest")).toBeNull(); + expect(parseMajor("")).toBeNull(); + }); +}); + +describe("isBoundedRange", () => { + it("accepts a range with an upper bound", () => { + expect(isBoundedRange(">=24 <25")).toBe(true); + }); + + it("accepts a caret range, which is implicitly bounded", () => { + expect(isBoundedRange("^24.13.3")).toBe(true); + }); + + it("rejects an open-ended floor", () => { + expect(isBoundedRange(">=24")).toBe(false); + expect(isBoundedRange(">=18.0.0")).toBe(false); + }); + + it("rejects a wildcard", () => { + expect(isBoundedRange("*")).toBe(false); + }); +}); + +describe("checkNodePin", () => { + it("passes when every declaration names the same major", () => { + const result = checkNodePin(consistentInputs); + + expect(result.consistent).toBe(true); + expect(result.major).toBe(24); + expect(result.problems).toEqual([]); + }); + + it("reports every declaration it inspected", () => { + const result = checkNodePin(consistentInputs); + + expect(result.declarations).toHaveLength(4); + expect(result.declarations.map((d) => d.source)).toEqual([ + ".nvmrc", + "server/package.json → engines.node", + "server/package.json → @types/node", + "running interpreter", + ]); + }); + + it("fails when @types/node runs ahead of the runtime", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: "^25.7.0" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("@types/node"); + }); + + it("fails when the running interpreter is a different major", () => { + const result = checkNodePin({ ...consistentInputs, runtimeVersion: "v26.4.0" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("running interpreter"); + }); + + it("fails when .nvmrc disagrees with the engines range", () => { + const result = checkNodePin({ ...consistentInputs, nvmrc: "22" }); + + expect(result.consistent).toBe(false); + }); + + it("fails an open-ended engines range even when every major agrees", () => { + const result = checkNodePin({ ...consistentInputs, enginesNode: ">=24" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("bounded"); + }); + + it("fails an unparseable declaration rather than silently skipping it", () => { + const result = checkNodePin({ ...consistentInputs, nvmrc: "lts/*" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain(".nvmrc"); + }); + + it("names the offending majors so the failure is actionable", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: "^25.7.0" }); + + expect(result.problems.join(" ")).toContain("25"); + expect(result.problems.join(" ")).toContain("24"); + }); + + it("does not mutate its input", () => { + const inputs: NodePinInputs = { ...consistentInputs }; + const snapshot = JSON.stringify(inputs); + + checkNodePin(inputs); + + expect(JSON.stringify(inputs)).toBe(snapshot); + }); +}); diff --git a/server/src/config/node-pin.ts b/server/src/config/node-pin.ts new file mode 100644 index 0000000..a81ef07 --- /dev/null +++ b/server/src/config/node-pin.ts @@ -0,0 +1,139 @@ +/** + * Node version pin (issue #126). + * + * DMXr declares its Node major in four independent places. This module holds the + * pure comparison logic that asserts they all agree; `scripts/check-node-pin.ts` + * supplies the file I/O and the exit code. + * + * Two distinct failures are checked, because catching only the first still lets a + * wrong runtime through: + * 1. Drift — the declarations name different majors. + * 2. Slack — `engines.node` is an open-ended floor, so a newer major satisfies + * it and `npm install` stays happy even with `engine-strict=true`. + * Measured: `">=24"` on Node 26 exits 0; `">=24 <25"` exits 1. + */ + +/** One place the Node major is declared. */ +export interface NodePinDeclaration { + /** Human-readable location, used verbatim in failure messages. */ + readonly source: string; + /** The declaration exactly as written. */ + readonly raw: string; + /** Major version, or null when `raw` names no parseable major. */ + readonly major: number | null; +} + +/** The four declarations, read from disk and the running process. */ +export interface NodePinInputs { + /** Contents of `.nvmrc`. */ + readonly nvmrc: string; + /** `engines.node` from `server/package.json`. */ + readonly enginesNode: string; + /** `devDependencies["@types/node"]` from `server/package.json`. */ + readonly typesNode: string; + /** `process.version` of the interpreter running the check. */ + readonly runtimeVersion: string; +} + +export interface NodePinCheckResult { + readonly consistent: boolean; + /** The agreed major, or null when the declarations disagree. */ + readonly major: number | null; + readonly declarations: readonly NodePinDeclaration[]; + /** Empty when consistent; otherwise one actionable sentence per problem. */ + readonly problems: readonly string[]; +} + +/** Matches the first major version number in a version string or semver range. */ +const FIRST_MAJOR = /(\d+)/; + +/** + * Extracts the major version from any of the shapes DMXr declares: + * `24`, `v24.18.1`, `^24.13.3`, `>=24 <25`, `>=18.0.0`. + * + * Returns null when no digit is present (`latest`, `lts/*`, `""`), so callers can + * fail loudly instead of treating an unreadable declaration as agreement. + */ +export function parseMajor(raw: string): number | null { + const match = FIRST_MAJOR.exec(raw.trim()); + if (match === null) return null; + + const major = Number.parseInt(match[1], 10); + return Number.isNaN(major) ? null : major; +} + +/** + * True when a range cannot be satisfied by an arbitrarily newer major. + * + * A caret range is bounded by definition (`^24.13.3` excludes 25). An explicit + * upper bound (`<25`, `<=24`) is bounded. A bare floor is not. + */ +export function isBoundedRange(range: string): boolean { + const trimmed = range.trim(); + if (trimmed === "") return false; + if (trimmed.startsWith("^") || trimmed.startsWith("~")) return true; + return trimmed.includes("<"); +} + +function declare(source: string, raw: string): NodePinDeclaration { + return { source, raw, major: parseMajor(raw) }; +} + +function unreadableProblems( + declarations: readonly NodePinDeclaration[], +): readonly string[] { + return declarations + .filter((d) => d.major === null) + .map((d) => `${d.source} declares "${d.raw}", which names no Node major.`); +} + +function driftProblems( + declarations: readonly NodePinDeclaration[], + expected: number, +): readonly string[] { + return declarations + .filter((d) => d.major !== null && d.major !== expected) + .map( + (d) => + `${d.source} names Node ${d.major} but the pin is Node ${expected} ` + + `(declared "${d.raw}").`, + ); +} + +/** + * Asserts every declaration names the same Node major, and that `engines.node` + * is bounded. `.nvmrc` is the reference: it is the file version managers and + * `setup-node` both read, so it is what actually selects the runtime. + */ +export function checkNodePin(inputs: NodePinInputs): NodePinCheckResult { + const declarations: readonly NodePinDeclaration[] = [ + declare(".nvmrc", inputs.nvmrc), + declare("server/package.json → engines.node", inputs.enginesNode), + declare("server/package.json → @types/node", inputs.typesNode), + declare("running interpreter", inputs.runtimeVersion), + ]; + + const unreadable = unreadableProblems(declarations); + const expected = declarations[0].major; + + if (expected === null || unreadable.length > 0) { + return { consistent: false, major: null, declarations, problems: unreadable }; + } + + const drift = driftProblems(declarations, expected); + const slack = isBoundedRange(inputs.enginesNode) + ? [] + : [ + `server/package.json → engines.node is "${inputs.enginesNode}", an ` + + `open-ended floor. A newer major satisfies it, so engine-strict cannot ` + + `reject one. Use a bounded range such as ">=${expected} <${expected + 1}".`, + ]; + + const problems = [...drift, ...slack]; + return { + consistent: problems.length === 0, + major: problems.length === 0 ? expected : null, + declarations, + problems, + }; +} diff --git a/server/tsconfig.scripts.json b/server/tsconfig.scripts.json new file mode 100644 index 0000000..065d80c --- /dev/null +++ b/server/tsconfig.scripts.json @@ -0,0 +1,16 @@ +// Typechecks `scripts/` under the same strict flags as `src/`. +// +// The base config sets `rootDir: "src"` and emits to `dist/`, so it cannot also +// cover `scripts/` without changing the build output. This config is check-only: +// it widens the root and turns emit off, so a broken script fails CI instead of +// failing the first time someone runs it. +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "rootDir": ".", + "noEmit": true, + "declaration": false, + "sourceMap": false + }, + "include": ["scripts/**/*.ts", "src/**/*.ts"] +} From 2fa65a6a1ea9e473774cc75e30912473fcc7121e Mon Sep 17 00:00:00 2001 From: thewrz Date: Sat, 1 Aug 2026 15:34:21 -0700 Subject: [PATCH 2/3] fix(ci): check Node ranges semantically, not by shape Adversarial review of #127 (Codex gpt-5.6-sol, xhigh) found the pin gate accepted two range shapes that pass its check while still admitting a major we never validated -- the exact drift it exists to prevent: engines.node ">=24 <26" passed: it contains "<", so the has-an-upper-bound test was satisfied. Node 25 satisfies the range. @types/node ">=24" passed: first number is 24. A fresh install can @types/node "^24 || ^25" resolve 25.x typings against a Node 24 runtime. Both were reproduced as failing tests before the fix. Neither was live -- the committed declarations are correct -- but a guard that only inspects the first number and looks for a "<" cannot hold a pin against a later well-meaning edit. Replaces the syntactic isBoundedRange with isRangeConfinedToMajor, which asks semver whether the *whole* range is contained in the pinned major (`subset(range, ">=24.0.0 <25.0.0")`). Range math is precisely the thing that produced this bug, so it uses npm's own implementation rather than a second hand-rolled attempt. Verified across nine range shapes; malformed ranges return false rather than throwing, since an unreadable declaration is a failure. semver is a devDependency, not a runtime one: node-pin.ts is a build-time gate, so tsconfig.json now excludes it from emit. Confirmed on a clean build that dist/config/ no longer contains it and nothing in dist/ imports semver -- which matters here because build-server.yml prunes dev deps and ships dist/ as a release artifact, where a stray import would be a latent landmine. It stays typechecked via tsconfig.scripts.json and unit-tested via vitest, so excluding it costs no coverage. Verified under node:24: 1670 tests pass (11 skipped), npm ci clean, audit 0 vulnerabilities, typecheck + typecheck:scripts + build all exit 0. Negative, under node:26: npm ci exits 1 and check:node-pin exits 1. Co-Authored-By: Claude Opus 5 --- server/package-lock.json | 15 +++- server/package.json | 2 + server/src/config/node-pin.test.ts | 75 ++++++++++++++-- server/src/config/node-pin.ts | 138 +++++++++++++++++------------ server/tsconfig.json | 6 +- 5 files changed, 167 insertions(+), 69 deletions(-) diff --git a/server/package-lock.json b/server/package-lock.json index e569251..b60a699 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -23,11 +23,13 @@ "@types/better-sqlite3": "^7.6.13", "@types/node": "^24.13.3", "@types/pngjs": "^6.0.5", + "@types/semver": "^7.7.1", "@vitest/coverage-v8": "^4.1.6", "fast-check": "^4.8.0", "pixelmatch": "^7.1.0", "pngjs": "^7.0.0", "puppeteer": "^24.43.1", + "semver": "^7.8.5", "tsx": "^4.21.0", "typescript": "^6.0.2", "vitest": "^4.1.3", @@ -1648,6 +1650,13 @@ "@types/node": "*" } }, + "node_modules/@types/semver": { + "version": "7.7.1", + "resolved": "https://registry.npmjs.org/@types/semver/-/semver-7.7.1.tgz", + "integrity": "sha512-FmgJfu+MOcQ370SD0ev7EI8TlCAfKYU+B4m5T3yXc1CiRN94g/SZPtsCkk506aUDtlMnFZvasDwHHUcZUEaYuA==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/triple-beam": { "version": "1.3.5", "resolved": "https://registry.npmjs.org/@types/triple-beam/-/triple-beam-1.3.5.tgz", @@ -4635,9 +4644,9 @@ "license": "BSD-3-Clause" }, "node_modules/semver": { - "version": "7.7.4", - "resolved": "https://registry.npmjs.org/semver/-/semver-7.7.4.tgz", - "integrity": "sha512-vFKC2IEtQnVhpT78h1Yp8wzwrf8CM+MzKMHGJZfBtzhZNycRFnXsHk6E5TxIkkMsgNS7mdX3AGB7x2QM2di4lA==", + "version": "7.8.5", + "resolved": "https://registry.npmjs.org/semver/-/semver-7.8.5.tgz", + "integrity": "sha512-Y7/KDsb8LjooZpwaqGyulO6DQlksgCncchHGk+sZIY4SBvUocMBEFH5Ur1fI4dV+Jvl0w6cjvucaIi40puRioA==", "license": "ISC", "bin": { "semver": "bin/semver.js" diff --git a/server/package.json b/server/package.json index d096cf1..a4823c2 100644 --- a/server/package.json +++ b/server/package.json @@ -35,11 +35,13 @@ "@types/better-sqlite3": "^7.6.13", "@types/node": "^24.13.3", "@types/pngjs": "^6.0.5", + "@types/semver": "^7.7.1", "@vitest/coverage-v8": "^4.1.6", "fast-check": "^4.8.0", "pixelmatch": "^7.1.0", "pngjs": "^7.0.0", "puppeteer": "^24.43.1", + "semver": "^7.8.5", "tsx": "^4.21.0", "typescript": "^6.0.2", "vitest": "^4.1.3", diff --git a/server/src/config/node-pin.test.ts b/server/src/config/node-pin.test.ts index 49eb461..9fe28c9 100644 --- a/server/src/config/node-pin.test.ts +++ b/server/src/config/node-pin.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { parseMajor, isBoundedRange, checkNodePin } from "./node-pin.js"; +import { parseMajor, isRangeConfinedToMajor, checkNodePin } from "./node-pin.js"; import type { NodePinInputs } from "./node-pin.js"; const consistentInputs: NodePinInputs = { @@ -40,22 +40,43 @@ describe("parseMajor", () => { }); }); -describe("isBoundedRange", () => { - it("accepts a range with an upper bound", () => { - expect(isBoundedRange(">=24 <25")).toBe(true); +describe("isRangeConfinedToMajor", () => { + it("accepts an explicit single-major range", () => { + expect(isRangeConfinedToMajor(">=24 <25", 24)).toBe(true); }); - it("accepts a caret range, which is implicitly bounded", () => { - expect(isBoundedRange("^24.13.3")).toBe(true); + it("accepts caret, tilde and exact pins inside the major", () => { + expect(isRangeConfinedToMajor("^24.13.3", 24)).toBe(true); + expect(isRangeConfinedToMajor("~24.13.0", 24)).toBe(true); + expect(isRangeConfinedToMajor("24.13.3", 24)).toBe(true); }); it("rejects an open-ended floor", () => { - expect(isBoundedRange(">=24")).toBe(false); - expect(isBoundedRange(">=18.0.0")).toBe(false); + expect(isRangeConfinedToMajor(">=24", 24)).toBe(false); + expect(isRangeConfinedToMajor(">=18.0.0", 24)).toBe(false); + }); + + // The syntactic predicate this replaced returned true here: the range has an + // upper bound and starts with 24, yet Node 25 satisfies it. + it("rejects a bounded range that still spans two majors", () => { + expect(isRangeConfinedToMajor(">=24 <26", 24)).toBe(false); + }); + + it("rejects a disjunction that reaches past the major", () => { + expect(isRangeConfinedToMajor("^24 || ^25", 24)).toBe(false); }); it("rejects a wildcard", () => { - expect(isBoundedRange("*")).toBe(false); + expect(isRangeConfinedToMajor("*", 24)).toBe(false); + }); + + it("rejects a range confined to a different major", () => { + expect(isRangeConfinedToMajor("^22.0.0", 24)).toBe(false); + }); + + it("returns false for a malformed range instead of throwing", () => { + expect(isRangeConfinedToMajor("not-a-range", 24)).toBe(false); + expect(isRangeConfinedToMajor("", 24)).toBe(false); }); }); @@ -121,6 +142,42 @@ describe("checkNodePin", () => { expect(result.problems.join(" ")).toContain("24"); }); + // Adversarial review of PR #127 (Codex gpt-5.6-sol) found the gate accepted + // ranges that pass a first-number/has-an-upper-bound check but still admit a + // different major -- the exact drift it exists to prevent. + it("fails an engines range that admits a second major", () => { + const result = checkNodePin({ ...consistentInputs, enginesNode: ">=24 <26" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("engines.node"); + }); + + it("fails an unbounded @types/node range", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: ">=24" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("@types/node"); + }); + + it("fails a disjunctive @types/node range that reaches past the pin", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: "^24 || ^25" }); + + expect(result.consistent).toBe(false); + expect(result.problems.join(" ")).toContain("@types/node"); + }); + + it("still accepts an exact pinned @types/node version", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: "24.13.3" }); + + expect(result.consistent).toBe(true); + }); + + it("still accepts a tilde @types/node range inside the pin", () => { + const result = checkNodePin({ ...consistentInputs, typesNode: "~24.13.0" }); + + expect(result.consistent).toBe(true); + }); + it("does not mutate its input", () => { const inputs: NodePinInputs = { ...consistentInputs }; const snapshot = JSON.stringify(inputs); diff --git a/server/src/config/node-pin.ts b/server/src/config/node-pin.ts index a81ef07..9e640c4 100644 --- a/server/src/config/node-pin.ts +++ b/server/src/config/node-pin.ts @@ -5,13 +5,14 @@ * pure comparison logic that asserts they all agree; `scripts/check-node-pin.ts` * supplies the file I/O and the exit code. * - * Two distinct failures are checked, because catching only the first still lets a - * wrong runtime through: - * 1. Drift — the declarations name different majors. - * 2. Slack — `engines.node` is an open-ended floor, so a newer major satisfies - * it and `npm install` stays happy even with `engine-strict=true`. - * Measured: `">=24"` on Node 26 exits 0; `">=24 <25"` exits 1. + * Two of those declarations are semver *ranges*, and a range is where a pin + * quietly rots: `">=24 <26"` looks bounded and `">=24"` starts with the right + * number, yet both admit Node 25. So ranges are checked semantically -- the whole + * range must be contained in the pinned major -- rather than by reading their + * first number or looking for a `<`. Adversarial review of PR #127 caught the + * earlier syntactic version accepting exactly those two shapes. */ +import { subset } from "semver"; /** One place the Node major is declared. */ export interface NodePinDeclaration { @@ -25,11 +26,11 @@ export interface NodePinDeclaration { /** The four declarations, read from disk and the running process. */ export interface NodePinInputs { - /** Contents of `.nvmrc`. */ + /** Contents of `.nvmrc` — the source of truth. A bare major, e.g. `24`. */ readonly nvmrc: string; - /** `engines.node` from `server/package.json`. */ + /** `engines.node` from `server/package.json`. A semver range. */ readonly enginesNode: string; - /** `devDependencies["@types/node"]` from `server/package.json`. */ + /** `devDependencies["@types/node"]` from `server/package.json`. A semver range. */ readonly typesNode: string; /** `process.version` of the interpreter running the check. */ readonly runtimeVersion: string; @@ -37,22 +38,25 @@ export interface NodePinInputs { export interface NodePinCheckResult { readonly consistent: boolean; - /** The agreed major, or null when the declarations disagree. */ + /** The pinned major, or null when anything disagrees. */ readonly major: number | null; readonly declarations: readonly NodePinDeclaration[]; /** Empty when consistent; otherwise one actionable sentence per problem. */ readonly problems: readonly string[]; } -/** Matches the first major version number in a version string or semver range. */ +/** Matches the first version number in a version string. */ const FIRST_MAJOR = /(\d+)/; /** - * Extracts the major version from any of the shapes DMXr declares: - * `24`, `v24.18.1`, `^24.13.3`, `>=24 <25`, `>=18.0.0`. + * Extracts the major version from a concrete version (`24`, `v24.18.1`) or, for + * display purposes, from a range. * - * Returns null when no digit is present (`latest`, `lts/*`, `""`), so callers can + * Returns null when no digit is present (`lts/*`, `latest`, `""`), so callers can * fail loudly instead of treating an unreadable declaration as agreement. + * + * Do **not** use this to validate a range — `">=24 <26"` yields 24 while still + * admitting 25. Use {@link isRangeConfinedToMajor}. */ export function parseMajor(raw: string): number | null { const match = FIRST_MAJOR.exec(raw.trim()); @@ -63,47 +67,59 @@ export function parseMajor(raw: string): number | null { } /** - * True when a range cannot be satisfied by an arbitrarily newer major. + * True when *every* version satisfying `range` falls inside `major`. + * + * This is the check that actually holds the pin. `^24.13.3`, `~24.13.0`, + * `24.13.3` and `">=24 <25"` pass; `">=24"`, `">=24 <26"`, `"^24 || ^25"` and + * `"*"` all fail, because each admits a major we never validated. * - * A caret range is bounded by definition (`^24.13.3` excludes 25). An explicit - * upper bound (`<25`, `<=24`) is bounded. A bare floor is not. + * Returns false for a malformed range rather than throwing — an unreadable + * declaration is a failure, not a crash. */ -export function isBoundedRange(range: string): boolean { - const trimmed = range.trim(); - if (trimmed === "") return false; - if (trimmed.startsWith("^") || trimmed.startsWith("~")) return true; - return trimmed.includes("<"); +export function isRangeConfinedToMajor(range: string, major: number): boolean { + try { + return subset(range, `>=${major}.0.0 <${major + 1}.0.0`); + } catch { + return false; + } } function declare(source: string, raw: string): NodePinDeclaration { return { source, raw, major: parseMajor(raw) }; } -function unreadableProblems( - declarations: readonly NodePinDeclaration[], +function rangeProblems( + source: string, + range: string, + pinned: number, ): readonly string[] { - return declarations - .filter((d) => d.major === null) - .map((d) => `${d.source} declares "${d.raw}", which names no Node major.`); + if (isRangeConfinedToMajor(range, pinned)) return []; + + return [ + `${source} is "${range.trim()}", which is not bounded to Node ${pinned} — ` + + `it admits at least one other major. Use a range wholly inside the pin, ` + + `such as ">=${pinned} <${pinned + 1}" or "^${pinned}.0.0".`, + ]; } -function driftProblems( - declarations: readonly NodePinDeclaration[], - expected: number, -): readonly string[] { - return declarations - .filter((d) => d.major !== null && d.major !== expected) - .map( - (d) => - `${d.source} names Node ${d.major} but the pin is Node ${expected} ` + - `(declared "${d.raw}").`, - ); +function runtimeProblems(version: string, pinned: number): readonly string[] { + const major = parseMajor(version); + if (major === pinned) return []; + + const named = major === null ? "no readable major" : `Node ${major}`; + return [ + `running interpreter reports ${named} but the pin is Node ${pinned} ` + + `(declared "${version.trim()}"). Switch runtimes with fnm/nvm/mise — ` + + `they read .nvmrc.`, + ]; } /** - * Asserts every declaration names the same Node major, and that `engines.node` - * is bounded. `.nvmrc` is the reference: it is the file version managers and - * `setup-node` both read, so it is what actually selects the runtime. + * Asserts every declaration agrees on one Node major. + * + * `.nvmrc` is the reference, because it is the file both version managers and + * `setup-node` read — it is what actually selects the runtime. The other three + * are measured against it. */ export function checkNodePin(inputs: NodePinInputs): NodePinCheckResult { const declarations: readonly NodePinDeclaration[] = [ @@ -113,26 +129,36 @@ export function checkNodePin(inputs: NodePinInputs): NodePinCheckResult { declare("running interpreter", inputs.runtimeVersion), ]; - const unreadable = unreadableProblems(declarations); - const expected = declarations[0].major; - - if (expected === null || unreadable.length > 0) { - return { consistent: false, major: null, declarations, problems: unreadable }; + const pinned = parseMajor(inputs.nvmrc); + if (pinned === null) { + return { + consistent: false, + major: null, + declarations, + problems: [ + `.nvmrc declares "${inputs.nvmrc.trim()}", which names no Node major. ` + + `It must be a bare major, e.g. "24".`, + ], + }; } - const drift = driftProblems(declarations, expected); - const slack = isBoundedRange(inputs.enginesNode) - ? [] - : [ - `server/package.json → engines.node is "${inputs.enginesNode}", an ` + - `open-ended floor. A newer major satisfies it, so engine-strict cannot ` + - `reject one. Use a bounded range such as ">=${expected} <${expected + 1}".`, - ]; + const problems = [ + ...rangeProblems( + "server/package.json → engines.node", + inputs.enginesNode, + pinned, + ), + ...rangeProblems( + "server/package.json → @types/node", + inputs.typesNode, + pinned, + ), + ...runtimeProblems(inputs.runtimeVersion, pinned), + ]; - const problems = [...drift, ...slack]; return { consistent: problems.length === 0, - major: problems.length === 0 ? expected : null, + major: problems.length === 0 ? pinned : null, declarations, problems, }; diff --git a/server/tsconfig.json b/server/tsconfig.json index 766dc77..9a5f07b 100644 --- a/server/tsconfig.json +++ b/server/tsconfig.json @@ -15,5 +15,9 @@ "isolatedModules": true }, "include": ["src/**/*.ts"], - "exclude": ["node_modules", "dist", "**/*.test.ts"] + // node-pin.ts is a build-time gate, not server code. It is excluded from emit so + // its `semver` import stays a devDependency and never reaches the pruned release + // bundle that build-server.yml ships. It is still typechecked (tsconfig.scripts.json) + // and still unit-tested (vitest), so excluding it costs no coverage. + "exclude": ["node_modules", "dist", "**/*.test.ts", "src/config/node-pin.ts"] } From 7e9baab1eb32266e27da6e13be50dba3962cda25 Mon Sep 17 00:00:00 2001 From: thewrz Date: Sat, 1 Aug 2026 22:33:22 -0700 Subject: [PATCH 3/3] fix(ci): least-privilege token, correct README, cite regression SHA Addresses CodeRabbit's review of #127. ci.yml declared no permissions, so all four of its jobs inherited whatever the repo/org default grants GITHUB_TOKEN -- potentially write. CodeRabbit flagged only the new node-pin job, but ci.yml was the sole workflow in the repo without a permissions block (dependency-review, dependency-health, build-server and codeql all declare one), so the block goes at workflow level: fixing one job would have left typecheck, test and audit inheriting the same broad token. Every job here only checks out and runs npm, so contents: read suffices. The config README still documented isBoundedRange(range); 2fa65a6 replaced it with isRangeConfinedToMajor(range, major) and did not update the doc. It also implied only engines.node is range-checked, when rangeProblems runs against @types/node too. Regression-test comments now cite fix commit 2fa65a6, per the repo guideline that every bug-fix test reference its commit SHA. Also documents the private helpers in node-pin.ts and check-node-pin.ts, which were the gap behind the failing docstring-coverage pre-merge check. Verified under node:24: check:node-pin consistent, typecheck and typecheck:scripts exit 0, 1670 tests pass (11 skipped), ci.yml parses. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 6 ++++++ server/scripts/check-node-pin.ts | 11 +++++++++++ server/src/config/README.md | 5 +++-- server/src/config/node-pin.test.ts | 12 +++++++----- server/src/config/node-pin.ts | 9 +++++++++ 5 files changed, 36 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e1545de..0b19b97 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,6 +4,12 @@ on: pull_request: branches: [main] +# Least privilege. Declared at workflow level rather than on one job: every job here +# only checks out and runs npm, and ci.yml was the sole workflow without a permissions +# block, so all four jobs were inheriting whatever the repo/org default grants. +permissions: + contents: read + jobs: typecheck: runs-on: ubuntu-latest diff --git a/server/scripts/check-node-pin.ts b/server/scripts/check-node-pin.ts index 64af08c..d2f1f00 100644 --- a/server/scripts/check-node-pin.ts +++ b/server/scripts/check-node-pin.ts @@ -16,6 +16,7 @@ const SCRIPT_DIR = dirname(fileURLToPath(import.meta.url)); const SERVER_DIR = join(SCRIPT_DIR, ".."); const REPO_ROOT = join(SERVER_DIR, ".."); +/** Reads a UTF-8 file, naming the path in the error so a bad checkout is obvious. */ function read(path: string): string { try { return readFileSync(path, "utf8"); @@ -25,11 +26,13 @@ function read(path: string): string { } } +/** The slice of `server/package.json` the pin cares about. */ interface ServerManifest { readonly engines?: { readonly node?: string }; readonly devDependencies?: Readonly>; } +/** Parses a package manifest, distinguishing "unreadable" from "not valid JSON". */ function readManifest(path: string): ServerManifest { try { return JSON.parse(read(path)) as ServerManifest; @@ -39,6 +42,13 @@ function readManifest(path: string): ServerManifest { } } +/** + * Gathers the four declarations from disk and the current process. + * + * A missing `engines.node` or `@types/node` throws rather than defaulting: the pin + * cannot be satisfied by a declaration that is not there, and silently passing would + * defeat the gate. + */ function collectInputs(): NodePinInputs { const manifestPath = join(SERVER_DIR, "package.json"); const manifest = readManifest(manifestPath); @@ -63,6 +73,7 @@ function collectInputs(): NodePinInputs { }; } +/** Runs the check and prints either a one-line pass or a full declaration table. */ function main(): void { const result = checkNodePin(collectInputs()); diff --git a/server/src/config/README.md b/server/src/config/README.md index c43a9f0..f25ad7e 100644 --- a/server/src/config/README.md +++ b/server/src/config/README.md @@ -15,10 +15,11 @@ - Always returns defensive copies (`{ ...current }`) to prevent external mutation ### node-pin.ts -- `parseMajor(raw)`, `isBoundedRange(range)`, `checkNodePin(inputs)` -> `NodePinCheckResult` +- `parseMajor(raw)`, `isRangeConfinedToMajor(range, major)`, `checkNodePin(inputs)` -> `NodePinCheckResult` - Pure comparison logic for the Node 24 LTS pin (issue #126): asserts `.nvmrc`, `engines.node`, `@types/node` and the running interpreter all name the same major, - and that `engines.node` is bounded rather than an open-ended floor + and that `engines.node` and `@types/node` are each confined to that major + (`rangeProblems` checks both) rather than being open-ended or spanning two majors - No I/O -- `server/scripts/check-node-pin.ts` supplies the files and the exit code. Lives here so tsconfig typechecks it and vitest covers it; `scripts/` is outside both. diff --git a/server/src/config/node-pin.test.ts b/server/src/config/node-pin.test.ts index 9fe28c9..93010c6 100644 --- a/server/src/config/node-pin.test.ts +++ b/server/src/config/node-pin.test.ts @@ -56,8 +56,9 @@ describe("isRangeConfinedToMajor", () => { expect(isRangeConfinedToMajor(">=18.0.0", 24)).toBe(false); }); - // The syntactic predicate this replaced returned true here: the range has an - // upper bound and starts with 24, yet Node 25 satisfies it. + // Regression, fixed in 2fa65a6: the syntactic predicate this replaced returned + // true here, because the range has an upper bound and starts with 24 — yet Node + // 25 satisfies it. it("rejects a bounded range that still spans two majors", () => { expect(isRangeConfinedToMajor(">=24 <26", 24)).toBe(false); }); @@ -142,9 +143,10 @@ describe("checkNodePin", () => { expect(result.problems.join(" ")).toContain("24"); }); - // Adversarial review of PR #127 (Codex gpt-5.6-sol) found the gate accepted - // ranges that pass a first-number/has-an-upper-bound check but still admit a - // different major -- the exact drift it exists to prevent. + // Regression, fixed in 2fa65a6: adversarial review of PR #127 (Codex + // gpt-5.6-sol) found the gate accepted ranges that pass a first-number or + // has-an-upper-bound check but still admit a different major -- the exact + // drift it exists to prevent. it("fails an engines range that admits a second major", () => { const result = checkNodePin({ ...consistentInputs, enginesNode: ">=24 <26" }); diff --git a/server/src/config/node-pin.ts b/server/src/config/node-pin.ts index 9e640c4..4a9eb27 100644 --- a/server/src/config/node-pin.ts +++ b/server/src/config/node-pin.ts @@ -84,10 +84,15 @@ export function isRangeConfinedToMajor(range: string, major: number): boolean { } } +/** Builds a declaration record, parsing its major for display in failure output. */ function declare(source: string, raw: string): NodePinDeclaration { return { source, raw, major: parseMajor(raw) }; } +/** + * Reports a range declaration that is not wholly inside the pinned major. + * Returns an empty list when the range is fine, so callers can spread it. + */ function rangeProblems( source: string, range: string, @@ -102,6 +107,10 @@ function rangeProblems( ]; } +/** + * Reports the interpreter actually executing the check being off the pin, with the + * remedy — this is the failure a contributor on the wrong runtime will hit first. + */ function runtimeProblems(version: string, pinned: number): readonly string[] { const major = parseMajor(version); if (major === pinned) return [];