fix(tui): pass --expose-gc as a node CLI flag, not via NODE_OPTIONS (#17187) - #17200
fix(tui): pass --expose-gc as a node CLI flag, not via NODE_OPTIONS (#17187)#17200briandevans wants to merge 2 commits into
Conversation
…ousResearch#17187) Since NousResearch#13231 the TUI launcher set ``NODE_OPTIONS="--max-old-space-size=8192 --expose-gc"`` to keep long sessions from fatal-OOM. ``--expose-gc`` is a V8 flag, and node explicitly rejects V8 flags supplied through NODE_OPTIONS: /usr/bin/node: --expose-gc is not allowed in NODE_OPTIONS The user from NousResearch#17187 hits this on Node 20.12.2; ``hermes --tui`` exits before the TUI ever loads. Move ``--expose-gc`` out of the env var and onto the node CLI inside ``_make_tui_argv`` for the two node-direct branches (HERMES_TUI_DIR fast path and the built-bundle fallback). ``--max-old-space-size`` is still set in NODE_OPTIONS — that one is allowed there and the user-supplied override semantics are preserved. The dev paths (`tsx src/entry.tsx`, `npm start`) don't get ``--expose-gc`` because they don't invoke node directly. That's a pragmatic trade-off: those paths are for hermes maintainers, and they were broken on Node 20+ before this change too. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Fixes hermes --tui failing on Node.js ≥ 20 due to Node rejecting --expose-gc when injected via NODE_OPTIONS, by moving the flag onto the Node CLI argv for node-direct TUI launches while keeping the heap cap in NODE_OPTIONS.
Changes:
- Update
_make_tui_argvto pass--expose-gcas a Node CLI flag for the external dist (HERMES_TUI_DIR) and bundled dist launch paths. - Update
_launch_tuito only inject--max-old-space-size=8192intoNODE_OPTIONS(and document why--expose-gcis excluded). - Add regression tests covering
NODE_OPTIONSconstruction and both node-direct argv branches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
hermes_cli/main.py |
Moves --expose-gc to Node argv for node-direct launches; keeps heap cap via NODE_OPTIONS. |
tests/hermes_cli/test_tui_node_options_expose_gc.py |
Adds regression tests to ensure --expose-gc is not injected into NODE_OPTIONS and is present on argv for node-direct paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| """ | ||
|
|
||
| from pathlib import Path | ||
| import os |
There was a problem hiding this comment.
os is imported but never used in this test module; please remove it to avoid unused-import noise (and potential lint failures if/when linting is enforced in CI).
| import os |
Copilot flagged `import os` as unused in tests/hermes_cli/test_tui_node_options_expose_gc.py — confirmed via `grep -n 'os\.'` (no references). Removing keeps the test module lint-clean and matches the pattern enforced across recent test additions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@copilot Finding addressed in 1654e59: dropped the unused |
|
CI audit — all 34 Same baseline cluster as #17569, #17441, #17386, #17348, #17322 — credential_sources/minimax-oauth, normalize_whatsapp_identifier collection error, mcp_structured_content The |
|
Closing to keep the queue clean — happy to reopen if this is still useful. |
Summary
hermes --tuiimmediately exits with--expose-gc is not allowed in NODE_OPTIONSon Node ≥ 20 because the launcher setNODE_OPTIONS="--max-old-space-size=8192 --expose-gc"(added in fix(tui): harden against Node V8 OOM + GatewayClient leaks + resize perf #13231).--expose-gcfrom the env var onto the node argv inside_make_tui_argvfor the two node-direct launch paths. Keep--max-old-space-size=8192inNODE_OPTIONS, where it's allowed and where user overrides still merge correctly.The bug
hermes_cli/main.py:1126-1136(pre-fix) appended--expose-gcto whatever the user had inNODE_OPTIONS:--expose-gcis a V8 flag. Node has an explicit allow-list forNODE_OPTIONSand refuses anything outside it on startup, so the spawned process aborts before the TUI loads:The reporter (#17187) hit this on Ubuntu 24.04 with Node 20.12.2, with their own
NODE_OPTIONSempty — the launcher itself was the source.unset NODE_OPTIONSdoesn't help because the launcher re-injects the flag every run.--max-old-space-size=…is on Node's allow-list, which is why the cap added in #13231 still works; only the--expose-gcline trips the rejection.The fix
_make_tui_argvalready returns argv tuples like[node, dist/entry.js]for the two production paths (HERMES_TUI_DIR fast path + built-bundle fallback). V8 flags pass through fine when given on the node CLI, so the patch just inserts--expose-gcbetween the binary and the script:…and drops the
--expose-gcblock from theNODE_OPTIONSconstruction in_launch_tui.--max-old-space-size=8192continues to be set via env so the existing user-merge semantics (respect any user-supplied higher cap) are preserved.The dev paths (
tsx src/entry.tsx,npm start) aren't node-direct, so they don't pick up--expose-gc. Those paths are maintainer-only and were already broken on Node 20+ under the old code, so this is a pragmatic trade-off rather than a regression.Test plan
tests/hermes_cli/test_tui_node_options_expose_gc.py— 4 tests covering the env-var construction in_launch_tui(no--expose-gcinjected, userNODE_OPTIONSpreserved without double-cap) and both node-direct branches of_make_tui_argv(HERMES_TUI_DIR + built bundle), each asserting--expose-gclands on argv before the entry script. All 4 pass.tests/hermes_cli/test_tui_resume_flow.py,tests/hermes_cli/test_tui_npm_install.py,tests/hermes_cli/test_launcher.py— 17 / 17 pass on this branch.origin/main(188eaa57c) with the exact reporter symptom:AssertionError: --expose-gc must not be injected into NODE_OPTIONS (#17187); got NODE_OPTIONS='--max-old-space-size=8192 --expose-gc'. They pass with the fix applied.Related
hermes --tuifails with--expose-gc is not allowed in NODE_OPTIONS#17187--expose-gcinjection alongside the V8 heap cap).