chore(ci): pin DMXr to Node 24 LTS and enforce it end to end - #127
Conversation
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 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe repository now pins Node.js to version 24 through ChangesNode 24 runtime enforcement
Sequence Diagram(s)sequenceDiagram
participant CI
participant check-node-pin
participant checkNodePin
participant RepositoryDeclarations
CI->>check-node-pin: run npm run check:node-pin
check-node-pin->>RepositoryDeclarations: read .nvmrc and package.json
check-node-pin->>checkNodePin: pass declarations and runtime version
checkNodePin-->>check-node-pin: return consistency result
check-node-pin-->>CI: report success or exit with status 1
Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Comment |
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 <noreply@anthropic.com>
|
This was written agentically; verify its assertions and edit accordingly: Adversarial cross-review — Codex Both were real gaps in the gate's rigor. Neither was a live bug — the committed declarations [P2] [P2] Engine upper bound not verified as the next major ( Both were reproduced as failing tests first, then fixed by replacing the syntactic One consequence worth flagging for review: Verification after the fix, in real containers (local Node 26 is now correctly rejected): Note this PR is stacked on #125, and 🤖 Co-authored by Claude Opus 5 (1M context). |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/src/config/node-pin.test.ts (1)
59-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReference the fix commit SHA in the regression-test comments.
These regression tests document the bug that adversarial review of PR
#127found, but the comments do not cite the fix commit. Add the commit SHA2fa65a6, as required by the coding guideline for regression tests.As per coding guidelines,
server/src/**/*.test.ts: "Add a regression test for every bug fix and reference the commit SHA."Also applies to: 145-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/src/config/node-pin.test.ts` around lines 59 - 63, Update the regression-test comments associated with isRangeConfinedToMajor, including the additional tests around the referenced range cases, to cite fix commit SHA 2fa65a6. Preserve the existing test behavior and explanations while adding the required commit reference to each applicable regression-test comment.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-47: Add job-level permissions to the node-pin job, granting
only contents: read. Leave all other permissions unspecified so they remain
unavailable, without changing the existing checkout, setup-node, dependency
installation, or Check Node pin steps.
In `@server/src/config/README.md`:
- Around line 17-24: Update the node-pin.ts README entry to document
isRangeConfinedToMajor(range, major) instead of isBoundedRange(range), and state
that major confinement validation applies to both engines.node and `@types/node`
declarations via rangeProblems. Keep the existing intent and file-relationship
description unchanged.
---
Nitpick comments:
In `@server/src/config/node-pin.test.ts`:
- Around line 59-63: Update the regression-test comments associated with
isRangeConfinedToMajor, including the additional tests around the referenced
range cases, to cite fix commit SHA 2fa65a6. Preserve the existing test behavior
and explanations while adding the required commit reference to each applicable
regression-test comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c89de984-472d-4574-8fe6-b9c4dc1f196b
⛔ Files ignored due to path filters (1)
server/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
.github/dependabot.yml.github/workflows/build-server.yml.github/workflows/ci.yml.github/workflows/dependency-health.yml.nvmrcCLAUDE.mdrenovate.jsonserver/.npmrcserver/package.jsonserver/scripts/check-node-pin.tsserver/src/config/README.mdserver/src/config/node-pin.test.tsserver/src/config/node-pin.tsserver/tsconfig.jsonserver/tsconfig.scripts.json
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 <noreply@anthropic.com>
1 participant
This was written agentically; verify its assertions and edit accordingly:
Why
DMXr declared its Node version in four places that disagreed with each other and with reality:
engines.node">=18.0.0"@types/node^25.7.0node-version22, hardcoded ×5 across 3 workflows.nvmrcThe
@types/node@25/ Node 22 gap is the costly one:tscvalidates an API surface theruntime does not have. A typecheck catches shape errors, not behavioral drift. Agent-assisted
implementation compounds it — models lag current releases and confidently emit code against
APIs they do not reliably know.
This is also a product decision, not just a CI one.
build-server.ymlbundles a portableNode runtime into the release artifact, downloading
node-$(node -e "console.log(process.version)")from nodejs.org. Whatever
setup-noderesolves is what ships to end users.Why Node 24
Node 24 is the oldest Active LTS still reasonable, and has been LTS long enough to sit inside
the training window of every model used on this repo. Node 26 does not, and should stay
off-limits well past its LTS date — ecosystem idioms accumulate for months after.
What
Pinned and enforced, not documented:
.nvmrc(24) is the single source of truth. All fivesetup-nodecall sites now readnode-version-file; no hardcoded major remains anywhere.engines.nodeis the bounded">=24 <25", plusengine-strict=trueinserver/.npmrc.Both halves are load-bearing — measured on npm 11.16.0 in real containers:
engines.nodeserver/.npmrcnpm install">=24 <25"">=24"(open)engine-strict=true">=24 <25"engine-strict=true">=24 <25"engine-strict=trueThis was re-measured rather than ported: a sibling pnpm repo found the same
.npmrcspelling completely inert under pnpm 11, needing
engineStrict: trueinpnpm-workspace.yaml.The mechanism does not transfer between package managers.
@types/nodetracks the runtime major (^24.13.3).tscis clean after the downgrade —zero fallout.
New
node-pinCI job runsnpm run check:node-pin, catching whatengine-strictcannot:declarations that are each individually valid but have drifted apart.
Automation is told the target. Renovate gets
constraintsFiltering: "strict"on runtimedeps, Node majors disabled, and
node-version/@types/nodebounded<25. Dependabot has noengines-awareness at all, so it gets an explicit
@types/nodemajor ignore with CI as backstop.A note on where the gate lives
The comparison logic is in
src/config/node-pin.ts, notscripts/—tsconfig.jsonincludesonly
src/**/*.tsand vitest collects onlysrc/**/*.test.ts, so a script there would have beenneither typechecked nor tested.
scripts/check-node-pin.tsis a thin I/O shell, and the newtsconfig.scripts.jsontypechecks it (and anything added toscripts/later) under the samestrict flags.
By design —
npm cinow exits 1 with expected-vs-actual instead of warning. It bites immediatelyon this machine (currently Node 26.4.0, system
/usr/bin/node, no version manager). A managerthat reads
.nvmrcis the ergonomic fix:fnm use/nvm use/mise install.Testing
Verified in real
node:24andnode:26containers, since local Node 26 is now correctly rejected:npm ci— clean under Node 24.18.1 / npm 11.16.0npm run check:node-pin— "consistent — Node 24 across 4 declarations and the running runtime"npm run typecheck— exit 0 with@types/node24 (no fallout from the downgrade)npm run typecheck:scripts— exit 0npm run build— exit 0npm audit --audit-level=high— 0 vulnerabilitiesnode-pin.ts)npm ciunder Node 26 → exit 1,Required: {"node":">=24 <25"} Actual: v26.5.1check:node-pinunder Node 26 → exit 1 with an actionable diffnode-pinjob. CI resolved/opt/hostedtoolcache/node/24.18.0/x64from.nvmrcand the gate reported "consistent — Node 24 across 4 declarations and the running runtime"npm cisucceeds once you're on Node 24Out of scope
tsconfig.jsonkeepstarget: "ES2022". Aligning it to a Node-24-accurate preset changes emitand deserves its own PR.
constraintsFilteringcovers direct dependencies only, so transitive engine driftremains a known gap.
🤖 Co-authored by Claude Opus 5 (1M context). Closes #126.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation