Skip to content

fix: add CREATE_NO_WINDOW to subprocess calls in tts_tool and vision_tools - #66785

Closed
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/subprocess-creationflags-side-paths
Closed

fix: add CREATE_NO_WINDOW to subprocess calls in tts_tool and vision_tools#66785
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/subprocess-creationflags-side-paths

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

On Windows, subprocess.run()/Popen() without CREATE_NO_WINDOW (0x08000000) spawns a visible console window (cmd.exe / conhost.exe) that flashes briefly. The main terminal path in tools/environments/local.py already has creationflags, and prior PRs (#64081, #65635, #65660) covered most side paths. Two additional sites were missed:

Changes

  • tools/tts_tool.py:731taskkill subprocess for TTS process cleanup. Already imports windows_hide_flags at module level (line 55), just needed the kwarg.
  • tools/vision_tools.py:292rsvg-convert/inkscape subprocess for SVG-to-PNG rasterization. Uses local subprocess import, applied inline 0x08000000 constant.

Test Plan

  • py_compile passes for both files
  • CI passes

…tools

On Windows, subprocess.run()/Popen() without CREATE_NO_WINDOW (0x08000000)
spawns a visible console window (cmd.exe / conhost.exe) that flashes
briefly. The main terminal path already has creationflags, but two side
paths were missed:

- tools/tts_tool.py:731 — taskkill subprocess for TTS process cleanup
- tools/vision_tools.py:292 — rsvg-convert/inkscape for SVG-to-PNG
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/tts Text-to-speech and transcription tool/vision Vision analysis and image generation platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows needs-decision Awaiting maintainer decision before any implementation duplicate This issue or pull request already exists labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #65663 for the identical vision_tools SVG-rasterizer hunk. The bundled tts_tool taskkill hunk is already covered by open #65667; please use the existing focused PRs rather than merge this overlapping bundle.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{
"event": "COMMENT",

Code Review Summary\n\nVerdict: Approved\n\nLooks good. No obvious issues found.\n\n---\nReviewed by Hermes Agent",

"comments": []
}

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracking two real Windows console-flash paths. Current main still lacks creationflags at tools/tts_tool.py:731 and tools/vision_tools.py:292.

Problems

  • This overlapping bundle is incomplete for TTS: direct subprocess calls at tools/tts_tool.py:2015 and :2094 also lack the flag, while the command-provider Popen at :785 only sets CREATE_NEW_PROCESS_GROUP.
  • The added tools/vision_tools.py:295 inline constant bypasses the centralized windows_hide_flags() contract in hermes_cli/_subprocess_compat.py:16-25.
  • No regression test is included; existing mocked flag coverage is in tests/test_windows_subprocess_no_window_flags.py:304-320.

Suggested changes

  • Consolidate with the focused PRs already linked in the member discussion: #65663 covers this exact vision hunk and #65667 covers the TTS direct calls.
  • Use windows_hide_flags() for the vision invocation and add mocked assertions for both paths.

This is an automated hermes-sweeper review.

Comment thread tools/vision_tools.py
_subprocess.run(
cmd, check=True, capture_output=True, timeout=30,
stdin=_subprocess.DEVNULL,
**({"creationflags": 0x08000000} if sys.platform == "win32" else {}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the centralized windows_hide_flags() helper rather than an inline sys.platform branch and literal. hermes_cli/_subprocess_compat.py defines the shared no-op-on-POSIX behavior and keeps Win32 flag handling consistent.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 18, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as a duplicate subset of your own earlier PRs — the tts_tool hunk covers 1 of #65667's 3 sites, and the vision_tools hunk is byte-identical to #65663's. Keeping #65667 and #65663 as the candidate vehicles for those sites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets duplicate This issue or pull request already exists needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows tool/tts Text-to-speech and transcription tool/vision Vision analysis and image generation type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants