Skip to content

chore(node): align local, CI, and Docker runtimes on Node 24 - #74250

Closed
DeliciousHouse wants to merge 2 commits into
NousResearch:mainfrom
DeliciousHouse:drift/t_f797c824-node-24-docker
Closed

DeliciousHouse wants to merge 2 commits into
NousResearch:mainfrom
DeliciousHouse:drift/t_f797c824-node-24-docker

Conversation

@DeliciousHouse

@DeliciousHouse DeliciousHouse commented Jul 29, 2026 •

Copy link
Copy Markdown

Summary

  • Pin local Node resolution to v24.15.0 and make every actions/setup-node consumer read the same .nvmrc contract.
  • Move the Docker Node source stage to the digest-pinned node:24-bookworm-slim image while preserving the existing Debian/glibc copy strategy.
  • Raise root and desktop package engine floors to Node 24 without dropping current-main manifest or lockfile content.
  • Move Unix, Windows, bootstrap, self-heal, Nix, and NixOS provisioning to Node 24, including NixOS container identity invalidation when the provisioned Node major changes.
  • Update English, Spanish, and Simplified Chinese contributor/installation/runtime documentation so supported Node requirements no longer contradict the package engine floor.
  • Extend the Node contract regression test so local, CI, Docker, package, installer, healing, Nix, and NixOS drift fails automatically.

Test Coverage

All changed runtime-contract paths have focused regression coverage.

  • npm ci --ignore-scripts --no-audit --no-fund
  • npm run --workspace tests-js test -- node-version-contract.test.ts — 2 tests passed
  • npm -C ui-tui run typecheck
  • HERMES_PYTHON=.venv/Scripts/python.exe scripts/run_tests.sh tests/test_install_ps1_node_path_for_npm.py tests/test_install_sh_node_global_prefix.py — 6 tests passed
  • HERMES_PYTHON=.venv/Scripts/python.exe scripts/run_tests.sh tests/test_hermes_constants.py -k TestHermesManagedNode — 3 tests passed
  • bash -n scripts/install.sh scripts/lib/node-bootstrap.sh
  • PowerShell AST parse of scripts/install.ps1
  • WSL/Nix parse of nix/hermes-agent.nix, nix/checks.nix, and nix/nixosModules.nix
  • npm -C apps/desktop run build
  • npm run --ws --if-present lint — passed with existing warnings only
  • git diff --check upstream/main...HEAD
  • gstack public-repository redaction scan — no findings

Rebase verification also compared package.json, apps/desktop/package.json, and package-lock.json with current upstream/main after removing only the intentional Node engine fields; all remaining content matched.

Local verification notes:

  • The rebased current main does not define npm run verify or npm run lint:all; the available workspace lint command above passed.
  • The requested Python paths tests/test_scripts_lint.py, tests/scripts/test_install_contract.py, tests/scripts/test_windows_install_script.py, and tests/test_node_self_heal.py, plus scripts/ci/secret_scan.sh, no longer exist on current main. The focused current-main equivalents above passed.
  • npm run check and npm -C apps/desktop run test were attempted; both reached broad untouched suites and exposed existing Windows-host failures in Unix-path/socket/SSH/UI timing tests. Required Ubuntu CI is the authoritative full-suite gate.

Pre-Landing Review

No issues found in the correction diff. The review specifically checked that every shipped provisioning path now agrees with the Node 24 engine floor and that NixOS reprovisions when the Node major changes.

Eval Results

No prompt-related files changed — evals skipped.

Scope Drift

Scope Check: CLEAN

Plan Completion

No plan file detected.

Test plan

  • One Node 24 local pin drives GitHub Actions setup
  • Docker uses a digest-pinned Node 24 source stage
  • Package engine metadata declares Node 24 as the minimum
  • Unix, Windows, bootstrap, self-heal, Nix, and NixOS paths provision/require Node 24
  • Contradictory installation and contributor docs are aligned
  • Runtime drift regression tests and focused local verification pass
  • Exact-head GitHub checks, including Linux Docker build/smoke, are green

@alt-glitch alt-glitch added type/refactor Code restructuring, no behavior change P3 Low — cosmetic, nice to have dependencies Pull requests that update a dependency file area/docker Docker image, Compose, packaging comp/desktop Electron desktop app (apps/desktop/*) labels Jul 29, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for consolidating the CI and Docker Node version inputs; current main does have the Node 22 / Node 20 drift this addresses.

Problems

  • package.json:47 changes the root engine contract to Node 24, but Hermes-managed installs still provision Node 22: scripts/install.sh:60, scripts/lib/node-bootstrap.sh:27, scripts/install.ps1:153, and hermes_constants.py:311. A fresh managed install therefore remains outside the declared engine floor.
  • Nix is another shipped runtime omitted from the migration: nix/hermes-agent.nix:13,42,198 selects nodejs_22 and exports it as HERMES_NODE; nix/checks.nix:259-275 only requires Node >=20. The new test does not cover that path.

Suggested changes

  • Migrate the managed installers/bootstrap and Nix derivation together with the engine-floor change, or retain the Node-22-compatible engine range.
  • Update the corresponding runtime checks and version documentation, then extend the contract coverage to these shipped paths.

This is an automated hermes-sweeper review.

Comment thread package.json
@@ -44,6 +44,6 @@
"protobufjs": "^8.7.1"
},
"engines": {
"node": ">=20.0.0"
"node": ">=24.0.0"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This raises the root requirement to Node 24, but fresh Hermes-managed installs still provision Node 22 (scripts/install.sh:60, scripts/lib/node-bootstrap.sh:27, scripts/install.ps1:153, hermes_constants.py:311) and Nix exports nodejs_22 as HERMES_NODE (nix/hermes-agent.nix:13,42,198). Please migrate those supported runtimes with this engine-floor change, or retain a Node-22-compatible engine range.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 30, 2026

@DeliciousHouse DeliciousHouse left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge gate: correction required (bounce 1/1)

Reviewed exact head deb3ae61b0cfd2eef3f8804ae5cbafa10859d6f8. This is the canonical Node 24 stream; PR #74890 is a narrower four-file duplicate and should be superseded rather than merged separately.

Blocking runtime-contract gap

The root and desktop manifests now require Node 24, but shipped provisioning and healing paths still install or accept Node 20/22:

  • scripts/install.sh:60,782-816 targets Node 22 and accepts ^20.19 || >=22.12.
  • scripts/install.ps1:147,1051-1079 targets Node 22 and accepts the same older range.
  • scripts/lib/node-bootstrap.sh:21-27 accepts Node 20 and installs Node 22.
  • hermes_constants.py:303 self-heals to Node 22.
  • nix/hermes-agent.nix:13,42 packages nodejs_22; nix/checks.nix:259-275 only requires Node >=20.
  • nix/nixosModules.nix:146,155 provisions the NodeSource 22 channel.
  • CONTRIBUTING.md:114, website/docs/getting-started/installation.md:93, website/docs/user-guide/windows-native.md:59,72,309, and the translated installation page still document Node 20/22.

A fresh CLI, Windows/desktop, bootstrap, self-heal, Nix, or NixOS install can therefore finish below the package engine floor. Migrate these surfaces to Node 24 together and extend tests-js/node-version-contract.test.ts (or equivalent focused tests) so installer/bootstrap/healing/Nix drift is covered, not only .nvmrc, manifests, Docker, and workflows.

Rebase and CI gate

This head is 405 commits behind current upstream/main cc4cab2f592e60a197e796506de9168f74baf3ea; current main has since changed apps/desktop/package.json and package-lock.json. Rebase onto fresh upstream main before pushing the correction and reconcile those files without dropping current-main content.

GitHub currently reports no checks at all on this exact head. Required exact-head CI must be populated and green, including the Linux Docker image build/smoke, before this can merge.

Evidence

Local exact-head verification under Node v24.15.0: npm ci passed; tests-js typecheck/Vitest/ESLint passed (10/10); desktop TypeScript passed; classifier suite passed (34/34); git diff --check passed. Pattern scans found no candidate secrets. An independent read-only Codex review confirmed the runtime-contract blocker and recommended retaining this broader stream over #74890.

Please update the existing PR branch only. Do not open a replacement PR or review card; this same reviewer/card will perform the one final review.

Use one repository pin for local tooling and CI, move the container source stage to Node 24, raise package engine floors, and add drift regression coverage.
@DeliciousHouse
DeliciousHouse force-pushed the drift/t_f797c824-node-24-docker branch from deb3ae6 to be440e2 Compare July 31, 2026 03:45
@DeliciousHouse

Copy link
Copy Markdown
Author

Correction pushed at exact head be440e2db5ab1f445b8e1d86dc4a4e55aadcb057 after rebasing onto current upstream/main.

  • Migrated Unix/Windows installers, bootstrap, self-heal, Nix, and NixOS provisioning to Node 24.
  • Added Node-major identity to NixOS container provisioning so a future major change invalidates the sentinel automatically.
  • Updated contradictory English, Spanish, and Simplified Chinese contributor/installation/runtime docs.
  • Extended tests-js/node-version-contract.test.ts across installer/bootstrap/healing/Nix/NixOS surfaces.
  • Verified root, desktop, and lockfile current-main content was preserved aside from the intentional Node engine fields.

Focused local verification passed: Node contract Vitest (2), current install-script tests (6), managed-node tests (3), TUI typecheck, Bash/PowerShell/Nix parsing, desktop production build, workspace lint (warnings only), diff check, and public-repo redaction scan.

The exact-head CI run exists but GitHub marked it action_required before creating jobs, so Docker build/smoke has not run yet: https://github.com/NousResearch/hermes-agent/actions/runs/30602371291. An upstream maintainer must approve the fork workflow before the required exact-head CI gate can complete.

@DeliciousHouse

Copy link
Copy Markdown
Author

Final merge-gate decision: close unmerged as superseded; no second correction bounce.

Revalidated the corrected branch at exact head be440e2db5ab1f445b8e1d86dc4a4e55aadcb057:

  • GitHub reports mergeable=false / mergeable_state=dirty. A fresh git merge-tree --write-tree against upstream main 82c6acae6fb98446ec61c65986ad58406ca6791c produces conflicts in 19 files, including every workflow consumer, .nvmrc, Dockerfile, both manifests/lockfile, installers, Nix, and docs. The branch is 1,095 commits behind main.
  • Exact-head CI is not green: gh pr checks reports no checks, and CI run 30602371291 completed action_required with no jobs.
  • Current main has intentionally moved past the proposed one-major Node 24 contract. Merged PR fix(runtime): managed Node/uv resolve first everywhere; require Node 26 #76459 established a Node 26 hermetic toolchain; PRs fix(install,update): restore installs and unblock the update runtime repair #76562 and fix(install): desktop still gated on Node 26 by a stale lockfile engines mirror #76575 then restored the supported package/managed-runtime floor to Node 22.22/22. Current main therefore uses .nvmrc/workflows/Docker Node 26, package engines >=22.22.0, and managed installer/self-heal Node 22. Replaying this stale branch would overwrite that newer split contract and later installer/Nix fixes. Sibling PR chore(docker): align Node toolchain on 24 #74890 is already closed.
  • The corrected diff also remains incomplete on its own terms: .nvmrc is classified into the frontend lane but not the docs-site lane even though docs-site-checks.yml consumes it (scripts/ci/classify_changes.py), and hermes_constants.py:471-495 returns any runnable managed Node without checking that its major matches the new target at line 311. An existing healthy Node 22 tree therefore never heals to 24. The added text-contract test does not cover either behavior; current main later added version-sensitive managed-tree healing and behavioral tests.

The branch diff itself passes git diff --check and the corrected Docker/install/docs coverage is materially broader than the first review, but CI, mergeability, supersession, and the remaining runtime gaps make it unsafe to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/docker Docker image, Compose, packaging comp/desktop Electron desktop app (apps/desktop/*) dependencies Pull requests that update a dependency file P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/refactor Code restructuring, no behavior change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants