Skip to content

fix: add creationflags to vision_tools SVG-to-PNG subprocess calls - #65663

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

fix: add creationflags to vision_tools SVG-to-PNG subprocess calls#65663
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/vision-tools-subprocess-creationflags

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

On Windows, subprocess.run() without CREATE_NO_WINDOW (0x08000000) spawns a visible console window. tools/vision_tools.py has two SVG-to-PNG conversion calls (rsvg-convert and inkscape) missing this flag. Both calls have stdin=DEVNULL and capture_output=True, so they are non-interactive.

Fix

Add creationflags=0x08000000 on Windows. sys is already imported at module level.

Changes

  • tools/vision_tools.py: +1 line

Test Plan

  • Unit tests pass
  • Verified on Windows: no console flash on SVG-to-PNG conversion

On Windows, rsvg-convert and inkscape subprocess calls flash a
console window because CREATE_NO_WINDOW is missing. Both calls have
stdin=DEVNULL and capture_output=True, so they are non-interactive.
@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/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 labels Jul 16, 2026

@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 identifying this Windows-specific subprocess path. Current main still reaches tools/vision_tools.py:292-295 without creationflags, and the single shared call serves both rsvg-convert and inkscape, so the proposed guard fixes the stated path.

Suggested changes

  • Add a focused regression test for _rasterize_svg_to_png() that captures the system-rasterizer spawn and asserts the Windows no-window flag. Existing Windows subprocess coverage follows this pattern in tests/test_windows_subprocess_no_window_flags.py:304-320.
  • Prefer windows_hide_flags() from hermes_cli._subprocess_compat to the inline literal. That helper is explicitly for synchronous output-capturing children (hermes_cli/_subprocess_compat.py:186-201) and is already used for comparable tool conversions in tools/tts_tool.py:1932.

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.

Suggestion: use windows_hide_flags() here instead of duplicating 0x08000000. It is the shared helper for short-lived synchronous subprocesses with captured output and returns 0 on non-Windows (hermes_cli/_subprocess_compat.py:186-201).

@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 after the class-level fix in PR #70205 (commit 0dbf639): Windows console flashes were caused by Hermes daemons running console-less (pythonw / DETACHED_PROCESS parents), forcing every console-subsystem child to allocate its own visible window. Main now launches every daemon (gateway, Scheduled Task, UAC handoff, dashboard, desktop backend) with a hidden console that all descendants inherit — so the spawn sites this PR flags no longer have a reachable flash under any shipped launch path (interactive terminals never flashed; children inherit the visible console there).

The sites you patched were genuinely unflagged, and the diagnosis was sound against the old launch topology — the ground just moved under it. Rather than keep growing per-site flags across an unbounded set of leaf spawns, we're standardizing on the parent-console fix. If a flash reappears on current main under a shipped launch path, that's a new bug — please file it with the flashing process name.

Thanks for the contribution and the Windows attention; sorry this one got mooted at the root.

@teknium1 teknium1 closed this Jul 23, 2026
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 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/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.

3 participants