fix(cli): add CREATE_BREAKAWAY_FROM_JOB to windows_hide_flags() - #55613
fix(cli): add CREATE_BREAKAWAY_FROM_JOB to windows_hide_flags()#55613liuhao1024 wants to merge 1 commit into
Conversation
Without this flag, CREATE_NO_WINDOW alone is silently ignored when the parent process lives inside a Windows Job Object (Electron, Tauri, etc.). Every subprocess spawn produces a visible cmd.exe window flash. The constant already exists in the file and is used by windows_detach_flags() for the same reason — it was simply missing from the hide-only variant. Fixes NousResearch#55604
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: LGTM
Fixes a real Windows issue: when Electron/Tauri spawns child processes inside a Windows Job Object, CREATE_NO_WINDOW alone is silently ignored and a visible cmd.exe window flashes for every subprocess. Adding CREATE_BREAKAWAY_FROM_JOB prevents the child from being re-parented into the job.
Looks Good
- One-line fix with clear documentation explaining the Windows Job Object interaction
- Regression test verifies both flags are present AND that DETACHED_PROCESS is NOT included (which would break capture_output)
- Docstring updated with the new flag's purpose
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Windows investigation. The proposed flag bundle needs a native-Windows reproduction before it is safe to salvage.
Problems
hermes_cli/_subprocess_compat.py:201would addCREATE_BREAKAWAY_FROM_JOBto every hidden synchronous spawn, but current main documents that restrictive Job Objects can reject that flag and only detached callers have the retry-without-breakaway path (hermes_cli/_subprocess_compat.py:138-144,:156-183).- Maintainer commit
d3d621f7c/ PR #53853 reverted the preceding popup-fix sweep because it did not fix the observed Windows symptom and required identifying the actual flashing spawn leg first. - Helper-only coverage is incomplete: direct
CREATE_NO_WINDOWsites remain intools/code_execution_tool.py:1353andhermes_cli/kanban_db.py:8077.
Suggested changes
- Capture the flashing executable and parent spawn on native Windows, then fix that verified path.
- Keep breakaway scoped to a path that genuinely needs Job Object escape and use the existing fallback pattern when it can be denied.
Automated hermes-sweeper review.
|
|
||
| ``CREATE_BREAKAWAY_FROM_JOB`` is included so that child processes | ||
| spawned inside an Electron / Tauri Windows Job Object are not | ||
| forcibly re-parented into the job — without this flag, |
There was a problem hiding this comment.
CREATE_BREAKAWAY_FROM_JOB can be denied by restrictive Job Objects; current main documents that at lines 138-144 and only retries detached spawns without this bit. Adding it here makes every short-lived hidden spawn subject to that failure with no fallback. Please first reproduce the actual flashing spawn on native Windows and scope any breakaway behavior to that path.
|
Closing after the class-level fix in PR #70205 (merged as 0dbf639) — the premise here doesn't hold against Win32 semantics, and the underlying symptom is addressed at a different layer. Two problems with adding
If a flash persists at a specific spawn site on current main, that's worth a fresh issue with the process name from the flash — it would point at a leg the parent-console fix doesn't cover. Thanks for digging into the Windows spawn behavior. |
What does this PR do?
Adds
CREATE_BREAKAWAY_FROM_JOBtowindows_hide_flags()so that subprocess calls made from inside an Electron / Tauri Windows Job Object remain hidden. Without this flag,CREATE_NO_WINDOWalone is silently ignored by the OS and every subprocess spawn produces a visiblecmd.exewindow flash.The constant (
_CREATE_BREAKAWAY_FROM_JOB = 0x01000000) already exists in the file and is already used bywindows_detach_flags()for the same reason — it was simply missing from the hide-only variant.Related Issue
Fixes #55604
Type of Change
Changes Made
hermes_cli/_subprocess_compat.py: Added| _CREATE_BREAKAWAY_FROM_JOBtowindows_hide_flags()return value; updated docstring to explain why the flag is needed.tests/tools/test_windows_native_support.py: Addedtest_windows_hide_flags_includes_breakaway_from_job— regression guard that verifies the breakaway bit is set andDETACHED_PROCESSis absent (which would breakcapture_output).How to Test
python -m pytest tests/tools/test_windows_native_support.py::TestSubprocessCompatHelpers::test_windows_hide_flags_includes_breakaway_from_job -xvs— should pass.python -m pytest tests/test_windows_subprocess_no_window_flags.py -q— all 15 existing tests should still pass (they mockwindows_hide_flagsso are unaffected by the implementation change).cmd.exewindows should flash.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — docstring updated inwindows_hide_flags()cli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A