Skip to content

fix(webui): raise declared Node floor from >=18 to 20.19.0 - #2886

Closed
itomek wants to merge 2 commits into
mainfrom
tmi/fix-2879-webui-node-engines
Closed

fix(webui): raise declared Node floor from >=18 to 20.19.0#2886
itomek wants to merge 2 commits into
mainfrom
tmi/fix-2879-webui-node-engines

Conversation

@itomek

@itomek itomek commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Closes #2879

Why this matters

src/gaia/apps/webui/package.json declared engines.node: ">=18", but the Agent UI cannot build on Node 18 — it dies at parse time (node:util.styleText, added in Node 20.12), and the locked bundler (vite@8.2.0 / rolldown@1.2.1) declares it needs Node 20.19+. The same stale "Node 18" claim was repeated across the curl-to-bash installer scripts and setup docs, and nothing checked that these places agreed. Anyone following the declared floor — a new contributor, a packager, an automated setup — got a broken Agent UI and a cryptic error that didn't point at the cause. This raises the declared floor to the honest 20.19.0 everywhere it's stated, and adds a drift guard so the declarations can't silently diverge again.

The floor was measured on real Node versions, not guessed — see the evidence below. The narrower "bump CI to a newer Node just to silence a cosmetic transitive-dependency warning" work is intentionally not in this PR (asymmetric risk mid-release-candidate); it's a separate follow-up.

Test plan

  • tests/unit/test_webui_node_engines.py — new drift guard, written first and run red against the pre-fix tree (engines.node='>=18' is not a simple '>=X.Y.Z' floor), green after the fix. It walks every workflow YAML (no hardcoded job list), asserts a non-empty set of webui-building jobs, and checks each pinned Node — plus both installer scripts — satisfies the declared floor.
  • python util/lint.py --all clean (Black/isort/flake8/pylint/bandit PASS).
  • Full tests/unit/ suite: 8135 passed, 3/3 new tests green. (7 failures are throwaway-venv provisioning artifacts — unactivated PATH, uv venv shipping without pip, a nested-worktree .env, and a [dev]+[ui] extras combo — none touch a changed file; all pass in CI's clean environment.)
Node floor measurement (Radeon dGPU box, Ubuntu, npm ci)
Node Result
18.19.1 FAIL — styleText parse error (cited from issue; reproduced on two checkouts)
20.11.1 FAIL — no styleText
20.12.2 HARD build failure — npm skips @rolldown/binding-linux-x64-gnu (the optional dep's own engines gate excludes 20.12.2), so the bundler can't find its native binding. (Correction to the original hypothesis, which predicted "builds but warns".)
20.19.0 BUILD, clean of vite/rolldown EBADENGINE — the chosen floor
20.20.2 builds, warns (cited from issue)
22.22.2 builds, zero EBADENGINE — rejected as the floor (see below)

Why 20.19.0 and not 22.x: the residual EBADENGINE above 20.19 comes from node-abi@4.33.0 (>=22.12.0) and the test-only jsdom@30.0.1 / mute-stream@4.0.0 (^22.22.2 || ^24.15.0 || >=26.0.0) — none of which the Agent UI build needs (the app has no native production deps to rebuild). Their warning is cosmetic; chasing it would over-declare the floor and needlessly restrict consumers. (Note: whatwg-url@17.1.0 carries a different constraint, ^22.14.0 || >=24.0.0, and does not belong to that group.)

CI needs no edits. Six jobs pin node-version and build src/gaia/apps/webui: test_electron.yml:test-webui-vitest, pypi.yml:build, build-installers.yml:build, publish.yml:validate, publish.yml:build-npm (all '20'), and publish.yml:publish-npm ('24', deliberately higher for npm-provenance OIDC). A real green run (gh run 30467337052) shows bare '20' resolving to 20.20.2 — above the 20.19.0 floor — so every pin already satisfies it. The drift guard's self-updating selector independently picked up the same six jobs.

Consumer scope: one coherent floor is declared everywhere (package.json, .nvmrc already v20.20.0, installer scripts, docs) rather than a split build-vs-runtime pair — two numbers is exactly the drift this fixes, and issue #2880's build preflight reads engines.node and needs it to carry the build floor. Sub-floor consumers get only a non-fatal warning (no engine-strict anywhere in the repo). The MCP-npx "Node 18+" note in the connector docs is left as-is — that's an unrelated launcher requirement, not the webui build toolchain.

itomek added 2 commits August 10, 2026 14:23
Add a repo-wide guard that fails CI if package.json's engines.node,
the CI jobs that build src/gaia/apps/webui, and the installer scripts'
Node gate ever disagree again. Written first, against the current
>=18 / '20' / -lt 18 state, so it fails before the floor is corrected.
The build actually needs Node 20.19+ (the locked vite/rolldown toolchain
declares that floor) -- Node 18 crashes at parse time, and anything below
20.19 either fails outright or gets silently skipped native bindings.
engines.node still said >=18, so a contributor or packager following it
got a cryptic bundler error instead of an actionable one.

Bring engines.node, both installer scripts' version gate, and the docs
that quote a Node floor into agreement with the measured 20.19.0 value.
CI already runs Node >=20.19 everywhere it builds this package (bare
'20'/'24' pins resolve to a current patch), so no workflow changes are
needed. MCP-launcher docs (github.mdx, tavily.mdx) intentionally keep
their separate, lower "Node 18+" guidance -- that's an npx runtime
requirement, unrelated to this build toolchain.

Also corrects a stale "Electron 40 / Node 20" comment left over from an
earlier Electron version.
@github-actions github-actions Bot added documentation Documentation changes tests Test changes labels Aug 10, 2026
@itomek

itomek commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Testing gate — evidence

GAIA's PR-to-main CI runs the reduced stacked-PR set (the Vitest / Code Quality / unit jobs show skipping, deferring to the integration stage), so the new drift guard was verified independently in a clean environment rather than relying on those skipped jobs.

Unit — drift guard, clean env (main checkout's .venv, Python 3.12.12 / pytest 9.1.1, PyYAML present — not the throwaway build venv that produced the 7 unrelated full-suite failures):

tests/unit/test_webui_node_engines.py::test_webui_engines_node_floor_is_declared PASSED
tests/unit/test_webui_node_engines.py::test_ci_jobs_building_webui_satisfy_the_engines_floor PASSED
tests/unit/test_webui_node_engines.py::test_installer_scripts_gate_satisfies_the_engines_floor PASSED
3 passed in 0.78s

Real-world — the floor is measured, not asserted: the Node matrix in the PR description (Radeon dGPU box, npm ci) is the real-world tier — 20.19.0 builds clean of vite/rolldown EBADENGINE, 20.12.2 is a hard build failure. The change alters no running app surface, so there is no UI-drive tier; the build matrix is the proof.

Installer script: bash -n scripts/install-ui.sh clean; gate now -lt 20 with "Node.js 20.19+ is required".

@itomek
itomek marked this pull request as ready for review August 10, 2026 18:39
@itomek
itomek requested a review from kovtcharov-amd as a code owner August 10, 2026 18:39
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve with suggestions.

This PR raises the declared Node.js floor for the Agent UI from 18 to 20.19.0 and lines up every place that states it — the docs, both installer scripts' version gate, package.json engines, a stale code comment — plus a new unit test that fails CI if any of them drift out of sync again. It's a clean, well-scoped consistency change and the evidence bundle exercises the real installer scripts on both the pass and fail paths.

One thing worth tightening before or shortly after merge: the installer scripts now say "Node.js 20.19+ is required" but only actually enforce a major-version check (they block Node < 20, but wave through anything 20.x). So a user on Node 20.0–20.18 sails past the friendly error and then hits exactly the cryptic bundler crash this PR set out to prevent — the toolchain genuinely needs 20.19+. The npm engines field is advisory (a warning, not a block) by default, so the installer gate is the real guard here. Not a merge blocker — it's a strict improvement over the old 18 gate — but the message currently over-promises relative to what it enforces.

Real-world evidence

The evidence bundle (evidence-bundle.md) covers this well and matches the diff. It shows the new unit tests passing (3/3), both installer scripts run for real — pass path on the runner's Node v22.23.1 and fail path via a faked v18.20.4 shim, both exiting as expected — the engines field read back as >=20.19.0, and a spot-regression sweep confirming the CI setup-node majors (20/24) still satisfy the new floor and that unrelated Node-18 floors elsewhere in the repo were intentionally left alone. This PR touches no Agent UI route, HTTP API, or MCP surface, so no screenshot/route evidence is applicable — correctly marked N/A. The evidence supports the verdict.

🔍 Technical details

🟡 Installer gate enforces major-only, but the stated/real floor is 20.19.0 (scripts/install-ui.sh:74-81, scripts/install-ui.ps1:51-58)

Both scripts parse only the major version and compare against 20:

NODE_VERSION=$(node -v | sed 's/v//' | cut -d. -f1)   # major only
if [ "$NODE_VERSION" -lt 20 ]; then ...

So Node 20.0.0–20.18.x passes the gate, yet the docstring of the new test itself notes the build fails below 20.12 (node:util has no styleText) and the locked vite/rolldown floor is ^20.19.0 || >=22.12.0. The error string already promises "20.19+", so the fix is to make the gate agree. A precise semver check for bash:

NODE_VERSION=$(node -v | sed 's/v//')
echo "  Node.js: $(node -v)"

node_lt() {  # returns 0 if $1 < $2 (dotted semver)
    [ "$1" != "$2" ] && [ "$(printf '%s\n%s\n' "$1" "$2" | sort -V | head -n1)" = "$1" ]
}
if node_lt "$NODE_VERSION" "20.19.0"; then
    echo "  ERROR: Node.js 20.19+ is required. Current version: $(node -v)"
    exit 1
fi

and the PowerShell equivalent can compare [version]($nodeVersion -replace 'v','') against [version]"20.19.0". If you'd rather keep the coarse major check to avoid the added shell complexity, that's a defensible call — but then it's worth a one-line comment noting the gate is intentionally major-only and the real 20.19 floor is enforced downstream, so the gap is a known trade-off rather than an oversight. The companion test (test_installer_scripts_gate_satisfies_the_engines_floor) only asserts major >= floor.major, so it won't catch this either way.

🟢 Nit — workflow glob misses *.yaml (tests/unit/test_webui_node_engines.py:193)

_find_webui_building_jobs globs _WORKFLOWS_DIR.glob("*.yml") only. All current workflows are .yml, so this is fine today, but a future .yaml workflow that builds the webui would be silently skipped by the drift guard — slightly at odds with the "the file list is itself drift" comment right above it. glob("*.y*ml") (or iterating both extensions) would close it.

Strengths

  • The drift-guard test is the right shape: it treats engines.node as the single source of truth, discovers webui-building CI jobs structurally instead of hardcoding a list, and fails loud on any pin shape it can't reason about rather than passing silently — modeled cleanly on test_amd_gaia_urls.py.
  • Genuinely complete scope: docs, both installers, engines, and the stale Electron 40 / Node 20Electron 43 / Node 24 comment all updated together, so the package won't ship self-contradicting version claims.
  • Evidence bundle exercises both branches of the installer gate with a real faked-Node shim — exactly the cold-state check CLAUDE.md asks for.

@itomek
itomek marked this pull request as draft August 10, 2026 19:06
@itomek itomek self-assigned this Aug 10, 2026
@itomek

itomek commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator Author

Closing in favour of #2887, which now carries the one-line engines.node>=20.19.0 correction alongside the loud-failure fix. Rationale: gaia init's preflight (#2880/#2887) reads engines.node, so declaring the honest floor in the same PR makes it self-contained, and it fails loudly on a too-old Node regardless. The standalone drift-guard test here was disproportionate to a non-fatal-warning recurrence — dropped. The install-script/doc floor corrections can follow as a small separate cleanup if wanted.

@itomek itomek closed this Aug 10, 2026
@itomek
itomek deleted the tmi/fix-2879-webui-node-engines branch August 10, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Documentation changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(webui): engines.node declares >=18 but the Agent UI build needs Node 20.12+

1 participant