fix(tui): hide npm console window during TUI dependency install on Windows - #66076
fix(tui): hide npm console window during TUI dependency install on Windows#66076bbasketballer75 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR primarily addresses a Windows-specific TUI startup UX bug by ensuring the npm install subprocess used for TUI dependency installation is spawned with hidden-console creation flags, consistent with the project’s Windows subprocess conventions. It also includes an additional (and currently undocumented in the PR metadata) change to dashboard WebSocket Host/Origin validation to support reverse-proxy deployments via dashboard.public_url.
Changes:
- Add
creationflags=windows_hide_flags()to the TUInpm installsubprocess spawn inhermes_cli/main.pyto prevent visible console windows on Windows. - Add a regression test asserting the TUI dependency install uses hidden-window creation flags.
- Extend dashboard WebSocket Host/Origin acceptance to allow a configured
dashboard.public_urlhost when loopback-bound (with accompanying tests).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
hermes_cli/main.py |
Adds Windows hidden-console creation flags to the TUI npm install spawn path. |
tests/test_windows_subprocess_no_window_flags.py |
Adds regression coverage ensuring the TUI npm install spawn includes creation flags. |
hermes_cli/web_server.py |
Adjusts WS Host/Origin validation to optionally accept dashboard.public_url host for loopback + reverse proxy setups. |
tests/hermes_cli/test_web_server_host_header.py |
Adds tests for the dashboard.public_url WS Origin acceptance scenarios (explicit port + IPv6), and for rejecting non-matching origins. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Scope: +175/-2, TUI hide npm console window on Windows
Notes
- Fix: hide npm console window during TUI dependency install on Windows.
- No security concerns.
- Clean platform-specific fix.
- LGTM.
Reviewed by Hermes Agent
d500ce3 to
aa4f7ab
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
Hides npm console window during TUI dependency install on Windows (50 additions / 0 deletions). Prior COMMENT noted.
No security issues. No debug artifacts.
Reviewed by Hermes Agent (cron batch 2026-07-17)
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the installer spawn and adding a focused regression test. The target call is still unflagged on current main (hermes_cli/main.py:1820-1843), and windows_hide_flags() is the appropriate synchronous-child helper (hermes_cli/_subprocess_compat.py:186-201).
Problems
- The dashboard path is not fully covered.
_resolve_chat_argv()calls_make_tui_argv(..., tui_dev=False)(hermes_cli/web_server.py:15349-15356). For non-Termux launches,_make_tui_argv()then always executes a second npm child,[npm, "run", "build"](hermes_cli/main.py:1882-1898), but that spawn has nocreationflags. The developer Ink prebuild athermes_cli/main.py:1861-1868is also unflagged.
Suggested changes
- Apply
windows_hide_flags()to the normal build and developer-prebuild npm spawns as well as the installer. - Extend the Windows spawn test to assert the normal non-Termux path hides both npm commands.
Automated hermes-sweeper review.
|
Thanks @teknium1! Applied windows_hide_flags() to the remaining |
Comment for #66076@teknium1 — could I get a re-review on this one? Your earlier salvageability=high feedback was addressed by extending The PR is now MERGEABLE against current
Happy to address anything else you've spotted. Thanks. |
- hermes_cli/web_server.py: urllib.parse is already imported at module level; drop the redundant local 'import urllib.parse as _up' alias and use the module-level import directly, matching the rest of the function. - tests/hermes_cli/test_web_server_host_header.py: drop raising=False on the resolve_public_url monkeypatch. It's a real, existing function (unlike the dynamic FastAPI app.state attributes patched elsewhere in this file, where raising=False is correct) -- raising=False here could silently mask the symbol being renamed/removed, letting the test keep 'passing' against a mock that no longer matches production code. Addresses Copilot review feedback on PR NousResearch#66076, where this code incorrectly also appeared due to a branch-history mistake (see that PR's other review comment) -- fixing it here, in the PR where this code actually belongs.
3939b78 to
4a9562e
Compare
4a9562e to
6081217
Compare
|
Rebased onto current origin/main (the prior rebase claim had gone stale — main had moved 344 commits since then, causing real conflicts). One conflict: an unrelated upstream deletion of The npm-console-window fix itself (all 3 spawns flagged, per the earlier hermes-sweeper feedback) was already correct before this rebase — no code changes needed there, just the mechanical rebase. Re-review welcome. |
…ndows The TUI dependency npm-install subprocess.run() call had no creationflags at all, unlike every other Windows-facing subprocess call in this codebase (which use windows_hide_flags() for exactly this). On a system where Windows Terminal is set as the default terminal-delegation handler, an unflagged console-subsystem child (npm.cmd) gets its own new, visible console -- even when spawned from an already-windowless pythonw.exe parent (e.g. a Windows Scheduled Task). Confirmed empirically on a live install: a Windows Terminal window appeared in lockstep with every dashboard restart that triggered this install path, and disappeared entirely once windows_hide_flags() was added. Adds a regression test in test_windows_subprocess_no_window_flags.py (the existing home for this exact contract across the codebase), verified to fail against the pre-fix code and pass against the fix.
6081217 to
6795fa3
Compare
|
Rebased onto current The conflict was in Slight improvement as a side effect: because the flag now lives in 🤖 Rebased by Claude Code |
The import sat inside the `_tui_need_npm_install(...)` branch while the two
`npm run build` spawns that reference it run outside that branch. Python
compiles the name as a function-local for the whole of `_make_tui_argv`, so
whenever the install was skipped — the common case, dependencies already
present — the build spawn raised:
UnboundLocalError: cannot access local variable 'windows_hide_flags'
where it is not associated with a value
This broke the TUI launch on every platform, not just Windows.
Move the import to module scope alongside the existing
`suppress_platform_ver_console` import.
Also adds the build-spawn assertion the review asked for, as a test that
drives the previously-unexercised path (`_tui_need_npm_install -> False`,
rebuild needed) and asserts every `npm run build` spawn carries the flag.
Verified it reproduces the UnboundLocalError when the fix is reverted.
|
Follow-up: while adding the build-spawn test you asked for, I found the flag wasn't actually working on the most common path. Fixed in The bug. That broke the TUI launch on every platform, not just Windows. The existing test never caught it because it forces Fix: hoisted the import to module scope next to the existing Your review ask, now covered: 🤖 Found and fixed by Claude Code |
Summary
The TUI dependency npm-install
subprocess.run()call had nocreationflagsat all, unlike every other Windows-facing subprocess call in this codebase, which usewindows_hide_flags()for exactly this purpose (seehermes_cli/_subprocess_compat.pyand the extensive existing coverage intests/test_windows_subprocess_no_window_flags.py).The bug
On a system where Windows Terminal is set as the default terminal-delegation handler (
HKCU\Console\%%Startup\DelegationTerminal), an unflagged console-subsystem child process (npm.cmd) gets its own new, visible console window — even when spawned from an already-windowlesspythonw.exeparent (e.g. a Windows Scheduled Task running the dashboard headlessly).Confirmed empirically on a live Windows install: a Windows Terminal window appeared in lockstep with every dashboard restart that triggered this install path (
_tui_need_npm_install()returningTrue), and disappeared entirely oncewindows_hide_flags()was added to the call.Fix
Adds
creationflags=windows_hide_flags()to thesubprocess.run()call in_make_tui_argv()(hermes_cli/main.py), matching the exact pattern already used for every other Windows subprocess spawn in this codebase.Test
Adds
test_tui_dependency_install_hides_npm_windowtotests/test_windows_subprocess_no_window_flags.py— the existing canonical home for this contract across the codebase (16 other tests assert the same property for git/gh/ffmpeg/taskkill/wmic/etc. spawns). Verified to fail against the pre-fix code and pass against the fix.Note for maintainers
While diagnosing this, I also found that
_tui_need_npm_install()'s lockfile comparison checks the entire monorepo lockfile (web, desktop, tests-js, ui-tui all together), not just the packages relevant to a--workspace ui-tui-scoped install. On my test install this meant 619 packages showed as "missing" (desktop's Electron deps, web's CodeMirror deps, etc.) purely because they were never meant to be installed by the TUI-scoped call in the first place — so_tui_need_npm_install()returnsTrueon effectively every invocation regardless of whether the TUI's own deps are actually up to date. I didn't attempt a fix for that here since it would mean partially reimplementing npm's own dependency resolution/hoisting logic, which felt too risky to get fully right without much more test coverage than I could responsibly add in this PR — flagging it in case it's useful, happy to open a separate issue with the full diagnostic if that's more useful than a PR comment.