fix(tools): add windows_hide_flags to backend version probes - #62761
fix(tools): add windows_hide_flags to backend version probes#62761liuhao1024 wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused Windows fix. The two changed calls correctly use the existing helper, whose documented contract is CREATE_NO_WINDOW on Windows and 0 elsewhere (hermes_cli/_subprocess_compat.py:186-201).
Problems
- The same active backend preflights remain unprotected:
DockerEnvironmentcalls_ensure_docker_available()attools/environments/docker.py:621, whosedocker versioncall is attools/environments/docker.py:520-526;SingularityEnvironmentcalls_ensure_singularity_available()attools/environments/singularity.py:178, whose version call is attools/environments/singularity.py:47-50. These paths can still spawn visible Windows console windows. - This PR changes no tests. Existing preflight tests at
tests/tools/test_docker_environment.py:99-120andtests/tools/test_singularity_preflight.py:49-71do not cover the required flag.
Suggested changes
- Apply
windows_hide_flags()to both environment-level preflight calls and add mocked assertions for all affected probes.
Automated hermes-sweeper review.
| @@ -2812,13 +2813,13 @@ def check_terminal_requirements() -> bool: | |||
| if not docker: | |||
| logger.error("Docker executable not found in PATH or common install locations") | |||
| return False | |||
There was a problem hiding this comment.
This fixes the tool-availability check, but the active Docker construction path still invokes the separate unprotected docker version preflight at tools/environments/docker.py:520-526 via DockerEnvironment at line 621. Please cover that sibling path (and the analogous Singularity preflight) so detached Windows execution does not still flash a console.
|
Closing in favor of PR #66605, which covers both of these probe sites plus the You were the first submitter for the docker/singularity probe sites (July 11, a week ahead) — your diagnosis was correct and will be credited if/when the consolidated fix lands. Note the user-visible symptom itself was fixed at the root by PR #70205 (hidden-console daemons, commit 0dbf639); the remaining value of flagging these sites is consistency/defense-in-depth. |
What does this PR do?
On Windows, when Hermes runs without an attached console (gateway service, cron-fired runs, or detached workers), subprocesses spawned without
CREATE_NO_WINDOWget a fresh visible console window. Two backend version probes intools/terminal_tool.py(docker and singularity/apptainer) were missingwindows_hide_flags(), causing brief console flashes during backend health checks.This PR adds
creationflags=windows_hide_flags()to both probe calls. On non-Windows platforms,windows_hide_flags()returns 0, so behavior is unchanged.Related Issue
Fixes #62734
Type of Change
Changes Made
tools/terminal_tool.py: Addfrom hermes_cli._subprocess_compat import windows_hide_flagsimporttools/terminal_tool.py:2815: Addcreationflags=windows_hide_flags()to docker version probetools/terminal_tool.py:2821: Addcreationflags=windows_hide_flags()to singularity/apptainer version probeHow to Test
On Windows:
TERMINAL_ENV=dockerorTERMINAL_ENV=singularitydocker version/apptainer --versionprobes no longer flash console windowsOn non-Windows (macOS, Linux):
check_terminal_requirements()returns correct results for local/docker/singularity backendspytest tests/tools/test_terminal_tool_requirements.py -q— should passChecklist
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/A