fix(desktop): detach packaged Desktop launch from the parent console on Windows - #58336
fix(desktop): detach packaged Desktop launch from the parent console on Windows#58336briandevans wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows-specific behavior in the hermes desktop (GUI) launcher by spawning the packaged Electron app as a detached process, preventing it from being terminated when the launching console closes and stopping Electron/Node stdout/stderr from flooding the parent terminal.
Changes:
- On Windows (
sys.platform == "win32"), launch the packaged Desktop app viasubprocess.Popen(..., creationflags=windows_detach_flags(), stdio=DEVNULL)and exit immediately. - Add a retry path that re-spawns without
CREATE_BREAKAWAY_FROM_JOBwhen the initial detached spawn fails (intended for “breakaway denied” scenarios). - Add targeted tests covering: Windows detached spawn + immediate return, Windows fallback behavior, and macOS foreground behavior (exit code propagation).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
hermes_cli/main.py |
Detaches packaged Desktop launch on Windows using Popen + detach flags and exits immediately; keeps macOS/Linux foreground behavior. |
tests/hermes_cli/test_gui_command.py |
Adds regression tests asserting Windows uses detached Popen (with fallback) and macOS stays on the blocking run() path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| subprocess.Popen( | ||
| launch_command, | ||
| creationflags=windows_detach_flags(), | ||
| **popen_kwargs, | ||
| ) | ||
| except OSError: | ||
| # The parent's job object disallows breakaway (rare — no | ||
| # JOB_OBJECT_LIMIT_BREAKAWAY_OK); retry without the breakaway bit. | ||
| subprocess.Popen( | ||
| launch_command, | ||
| creationflags=windows_detach_flags_without_breakaway(), | ||
| **popen_kwargs, | ||
| ) |
There was a problem hiding this comment.
Good call — fixed in 1863c7901. Narrowed the retry to gate on getattr(exc, "winerror", None) == 5 (ERROR_ACCESS_DENIED, the shape a denied CREATE_BREAKAWAY_FROM_JOB takes) and re-raise every other OSError, so an unrelated spawn failure (bad argv/env, missing exe) surfaces as one clear error instead of being masked by a doomed second attempt.
| patch( | ||
| "hermes_cli.main.subprocess.Popen", | ||
| side_effect=[OSError("breakaway denied"), None], | ||
| ) as mock_popen, \ |
There was a problem hiding this comment.
Done in 1863c7901. The fallback test now raises PermissionError with winerror = 5 (set explicitly, since winerror is only auto-populated on Windows) so it exercises the launcher's narrowed handler on any CI platform. Also added test_gui_win32_detach_reraises_non_breakaway_oserror to assert a non-breakaway OSError (winerror==2) propagates with no second Popen attempt.
alt-glitch
added
type/bug
|
@copilot Both findings addressed in 1863c7901:
|
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Windows launcher fix. Current main still uses the blocking packaged launch at hermes_cli/main.py:5844, and the proposed Popen path follows the existing detach-flag contract in hermes_cli/_subprocess_compat.py:113-183. The narrowed winerror == 5 retry in 1863c79016f6 also addresses the prior review feedback.
Problems
tests/hermes_cli/test_gui_command.py:1058: the diff removes the existingassert gpu == "auto"fromtest_desktop_launch_options_survives_config_error. This is unrelated regression-test coverage.
Suggested changes
- Restore that assertion while retaining the new detached-launch tests.
Automated hermes-sweeper review.
| @@ -1055,4 +1055,143 @@ def test_desktop_launch_options_survives_config_error(): | |||
| with patch("hermes_cli.config.load_config", side_effect=RuntimeError("boom")): | |||
| flags, gpu = cli_main._desktop_launch_options() | |||
| assert flags == [] | |||
| assert gpu == "auto" | |||
There was a problem hiding this comment.
Please retain this assertion. It covers the existing config-error default for gpu and is unrelated to the Windows detached-launch tests being added below.
There was a problem hiding this comment.
Restored in 9906a4e96. The deletion was accidental — the Windows detached-launch tests were appended directly after test_desktop_launch_options_survives_config_error and took its trailing line with them. assert gpu == "auto" is back verbatim and the new tests below are unchanged. tests/hermes_cli/test_gui_command.py: 65 passed.
There was a problem hiding this comment.
Correction to the SHA above: 9906a4e96 was orphaned when this branch was rebased, so it resolves to nothing. The current head is 3a849f923.
The state today is also different from what that reply described. test_desktop_launch_options_survives_config_error and its assert gpu == "auto" are no longer in tests/hermes_cli/test_gui_command.py at all — they were removed upstream in 39975613b ("test: prune wave 2"), which this branch is rebased onto.
Nothing in this PR removes them: 3a849f923 is +140/-0 in that file. The only tests it adds there are test_gui_win32_launches_detached_and_returns, test_gui_win32_detach_falls_back_without_breakaway, test_gui_win32_detach_reraises_non_breakaway_oserror, and test_gui_macos_launch_stays_foreground.
There was a problem hiding this comment.
Rebased onto current origin/main (was 1412 behind); the head SHA in my earlier reply is now orphaned.
- Previous head:
3a849f923 - Current head:
f4ad363b1
The substance of that reply is unchanged and re-verified on the new head: test_desktop_launch_options_survives_config_error and the assert gpu == "auto" case genuinely do not exist on origin/main (pruned upstream in 39975613b), and the four win32/macOS launch tests named there are all present and passing — 16/16 in tests/hermes_cli/test_gui_command.py.
One conflict, resolved additively: main appended a Linux-launcher-entry test section at the same point our branch appended the win32/macOS tests. Both were kept, with a # --- Windows detached launch --- section header added to match file convention (test file stat moves +140 -> +143). hermes_cli/main.py auto-merged intact. mergeStateStatus is back to MERGEABLE.
There was a problem hiding this comment.
Correction to the SHA above: this branch has been rebased again (it was 1125 commits behind), so f4ad363b1 is orphaned and resolves to nothing on the branch.
- Previous head:
f4ad363b1 - Current head:
7e0b753621b(the fix commit isa7f792844f2; the head commit is the empty CI retrigger)
Re-verified on the new head rather than restated:
test_desktop_launch_options_survives_config_errorand itsassert gpu == "auto"case do not exist anywhere onorigin/main—git grepfor the name across the whole tree returns nothing. They were pruned upstream by your own39975613b("test: prune wave 2"), whichgit merge-base --is-ancestor 39975613b origin/mainconfirms is an ancestor of current main. Nothing in this PR removes them; the diff againstorigin/mainis +42/-0 inhermes_cli/main.pyand +143/-0 intests/hermes_cli/test_gui_command.py, purely additive in both files.- The four launch tests are present:
test_gui_win32_launches_detached_and_returns(tests/hermes_cli/test_gui_command.py:788),test_gui_win32_detach_falls_back_without_breakaway(:830),test_gui_win32_detach_reraises_non_breakaway_oserror(:870),test_gui_macos_launch_stays_foreground(:903).
One substantive change this rebase forced, which I want to flag rather than bury. 39975613b also rewrote _make_packaged_executable, dropping its platform= override so the layout keys off the real sys.platform, with the rationale in its docstring: faking the platform "only proved the test and the code agreed about a host neither was running on." My four tests were built on that override plus a monkeypatch.setattr(_subproc_compat, "IS_WINDOWS", True), so they broke on the rebase with TypeError: _make_packaged_executable() got an unexpected keyword argument 'platform'.
I reconciled toward your policy rather than around it. tests/conftest.py:1045-1074 states the rule directly — "if the test needs the interpreter to BELIEVE it is on another OS in order to pass, it belongs on that OS" — and cmd_gui branches on the real sys.platform, so this is not the "pure function that takes a platform as data" exemption. The three Windows tests now carry @pytest.mark.windows_only and the macOS one @pytest.mark.macos_only, and both platform fakes are gone; the tests use _make_packaged_executable(root, monkeypatch) in its current form.
Consequence, stated plainly: on this host (macOS) test_gui_macos_launch_stays_foreground passes for real and the three windows_only tests skip — tests/hermes_cli/test_gui_command.py: 24 passed, 11 skipped, 0 failed. The Windows three are exercised by the OS-specific tests / Windows-only tests job on a native runner, which is the point of the markers. ruff check is clean on both touched files.
Also correcting my own closing line from the 2026-08-05 reply, which was wrong on the field name: mergeStateStatus is BLOCKED (required checks pending), not MERGEABLE. The flag that the rebase actually restores is mergeable, which now reads MERGEABLE again after being CONFLICTING.
teknium1
added
sweeper:risk-compatibility
9906a4e to
3a849f9
Compare
3a849f9 to
f4ad363
Compare
…on Windows `hermes gui` launched the packaged Desktop with a bare, blocking `subprocess.run(...)`, so the child inherited the parent console and its process group. Two consequences on Windows: - Closing the launching shell sends CTRL_CLOSE_EVENT down the group and kills Desktop with it. - Electron/Node write stdout+stderr straight into the parent terminal, which floods it and (under cp936) mojibakes it. Spawn detached via `subprocess.Popen` with the shared `windows_detach_flags()` creationflags and fully severed stdio, then exit 0 so the shell is free immediately. This mirrors the installer's own detached relaunch and `gateway_windows._spawn_detached`. The retry path is narrowed to a denied job breakaway only: the parent's job object may lack JOB_OBJECT_LIMIT_BREAKAWAY_OK, which surfaces as ERROR_ACCESS_DENIED (`winerror == 5`). Every other spawn failure (bad argv/env, missing exe) re-raises immediately rather than being masked by a doomed second attempt without the breakaway bit. macOS and Linux are untouched — they keep the foreground, console-inheriting `subprocess.run` launch and propagate its exit code.
be85443 to
7e0b753
Compare
What does this PR do?
On Windows,
hermes desktoplaunched the packaged Electron app with a blocking, console-inheritingsubprocess.run(...), so the child inherited the launching shell's console and process group. Two consequences (both reported in #58275):CTRL_CLOSE_EVENTdown the process group and kills Desktop — you can't launch-then-close-your-terminal.stdout+stderr(a harmless registry-probe error, aDEP0180warning) flood the parent terminal, rendered as GBK mojibake undercp936.The fix spawns the packaged app detached on Windows via
subprocess.Popen(..., creationflags=windows_detach_flags(), stdin/stdout/stderr=DEVNULL, close_fds=True)and returns immediately (sys.exit(0)). This mirrors the installer's own detached relaunch (already noted incmd_gui~line 5749) andgateway_windows._spawn_detached. If the parent's job object denies breakaway (rare — noJOB_OBJECT_LIMIT_BREAKAWAY_OK), it retries withwindows_detach_flags_without_breakaway(), the same fallback the gateway spawner uses.macOS/Linux are unchanged — the packaged launch stays foreground/console-inheriting (the normal, expected behavior there), and the
source_modeelectron-dev launch is untouched (dev runs are meant to be attached).Related Issue
Fixes #58275
Type of Change
Changes Made
hermes_cli/main.py(cmd_gui): onsys.platform == "win32", launch the packaged Desktop detached viaPopenwithwindows_detach_flags()+ DEVNULL stdio and exit 0 immediately, with anOSError→windows_detach_flags_without_breakaway()fallback. macOS/Linux keep the existing blockingsubprocess.runlaunch.tests/hermes_cli/test_gui_command.py: addedtest_gui_win32_launches_detached_and_returns,test_gui_win32_detach_falls_back_without_breakaway, andtest_gui_macos_launch_stays_foreground.How to Test
uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/hermes_cli/test_gui_command.py -v— 64 passed.main.pyhunk makes the two win32 tests FAIL (old code always callssubprocess.run, neverPopen) while the macOS foreground test still PASSES; restoring the hunk makes all three PASS.hermes desktop, then close the launching PowerShell window — Desktop keeps running and no Electron/Node output floods the shell.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/hermes_cli/test_gui_command.py -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/A