fix(windows): hide backend subprocess windows by default - #53879
Conversation
12ed42b to
e761fc0
Compare
Related to the open Windows console-flash cluster: #53358 (CREATE_BREAKAWAY_FROM_JOB under the Desktop Win32 Job Object), #53390 (copilot_auth gh-token site), #53542, #53291, #41078, and #44043 (tools_config subprocess calls), plus issue #53282 which this Fixes. This PR takes a different, broader approach -- a bootstrap-wide |
e761fc0 to
ead0f4e
Compare
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Windows subprocess window hiding (11 files, +334/-63). Well-structured fix:
- Extends the existing
hiddenWindowsChildOptionspattern to additional spawn sites (updater, etc.) - Properly tests all call sites via
requireAllHiddenChildOptionstest helper - Removes the
windowsHide: falseassertion that was covering one intentional case (now handled by the test helper) hermes_bootstrap.pychanges add Windows-specific subprocess handling with proper platform guards
The test helper approach is thorough — it catches future regressions when new spawn sites are added. No concerns about the implementation.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Comprehensive fix for Windows child process window flashing (11 files, +334/-63). Well-structured approach:
hermes_bootstrap.py: Addsapply_windows_subprocess_defaults()— process-wide Popen wrapper that defaults toCREATE_NO_WINDOW+ hiddenSTARTUPINFOhermes_cli/_subprocess_compat.py: Addsrun()andpopen()convenience wrappers with hidden defaults- All call sites updated to use the compat wrappers where appropriate
HERMES_ALLOW_VISIBLE_SUBPROCESSES=1escape hatch for debugging- Tests extended with
requireAllHiddenChildOptionshelper to verify all updater spawn sites
The test helper approach is thorough — it catches future regressions when new spawn sites are added. The CREATE_NEW_CONSOLE guard ensures explicit console requests are respected.
No concerns. Ready to merge.
Reviewed by Hermes Agent
|
i mean... it's working wonders for me rn. so... idk. |
|
Closing as superseded by the consolidated Windows console-flash work tracked in #54220. The relevant pieces from this PR/cluster have now landed through the targeted follow-up PRs #54236, #53892, and #54417, or are recorded in the umbrella tracker for any remaining native-Windows verification. Keeping this separate PR open would duplicate the tracker and the merged follow-up work. Thanks for digging into this — the reports and PRs in this cluster helped identify the remaining spawn legs. |
|
rip. kinda still think we need that catch all for python in bootstrap or something similar. the shouldnt be a find and fix every place thing. and i'm getting them a lot again an main, just not as persistent. spurts. |
What does this PR do?
Suppresses Windows terminal/cmd/conhost flashes from Hermes background subprocesses by making hidden child process launches the default for Hermes Python backend entry points, then tightens the remaining no-window paths hit while reproducing Desktop startup/session-load flashes.
The main behavioral change is in
hermes_bootstrap.py: on Windows, Hermes now patchessubprocess.Popenat bootstrap time so raw Pythonsubprocess.run(...)/Popen(...)call sites inheritCREATE_NO_WINDOWand hiddenSTARTUPINFOunless the caller explicitly asks for a new console. This matches the product expectation that Desktop/gateway/provider/session loading should not flash terminals while doing background work.The PR also keeps the MCP stdio launcher hidden even through the MCP SDK fallback path, and routes several Git/GitHub/catalog/TUI helper subprocesses through the existing no-window subprocess helper.
This branch has been rebased onto current
mainafter #53853 reverted the prior terminal-popup PR stack (#53791, #53810, #53829). It is intentionally a single squashed commit on top of that rollback.This overlaps the Windows terminal-popup PR family, especially #53291, #41078, #53390, and #53542. This branch packages the local Windows/Desktop repro work as one reviewable bundle rather than another single callsite patch.
Related Issue
Fixes #53282
Related: #53370, #53342, #53273, #53065, #53016, #53424
Related PRs / overlap: #53291, #41078, #53390, #53542
Type of Change
Changes Made
hermes_bootstrap.py: install a Windows-only, idempotentsubprocess.Popensubclass wrapper from the existing bootstrap path so Python backend children default to hidden console/window settings.tools/mcp_tool.py: patch the MCP SDK stdio Windows process factory so configured stdio MCP servers keepCREATE_NO_WINDOWand hidden startup info even if AnyIO process creation falls back to synchronoussubprocess.Popen.hermes_cli/banner.py,hermes_cli/copilot_auth.py,hermes_cli/main.py,hermes_cli/mcp_catalog.py,tui_gateway/server.py: route Git/GitHub/catalog/update/TUI helper subprocesses through Hermes' existing no-window subprocess helper.hermes_cli/main.py: preserve quarantined Windows entry-point shims if an install/update exits successfully without rewriting the replacement shim.apps/desktop/electron/windows-child-process.test.cjs: tighten desktop child-process invariants around hidden Windows child options.tests/hermes_cli/test_update_autostash.py,tests/hermes_cli/test_update_concurrent_quarantine.py: update focused tests for the update/quarantine behavior touched here.How to Test
Focused local checks run after rebase:
git diff --check origin/main...HEADvenv\Scripts\python.exe -m py_compile hermes_bootstrap.py tools\mcp_tool.py hermes_cli\banner.py hermes_cli\copilot_auth.py hermes_cli\main.py hermes_cli\mcp_catalog.py tui_gateway\server.pyhermes_bootstrappatchessubprocess.Popen, injectsCREATE_NO_WINDOW, preserves explicitCREATE_NEW_CONSOLE, and adds hidden startup info.CREATE_NO_WINDOWplus hidden startup info.Not run locally:
pytest tests/ -q. Local Windows full-suite runs have been unsafe/noisy in this checkout, and this PR was validated with focused compile/static/no-spawn checks plus manual Desktop observation.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A
Screenshots / Logs
N/A. This is a Windows process-window visibility fix; local verification used no-spawn assertions and manual Desktop startup/prompt observation.