Skip to content

fix: hide Windows console-flash on 3 new/untreated subprocess spawn sites - #85243

Open
pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/windows-console-flash-new-subprocess-sites
Open

pierrenode wants to merge 1 commit into
NousResearch:mainfrom
pierrenode:fix/windows-console-flash-new-subprocess-sites

Conversation

@pierrenode

Copy link
Copy Markdown

Summary

This repo has an established, well-tested convention (hermes_cli/_subprocess_compat.py::windows_hide_flags(), tests/test_windows_subprocess_no_window_flags.py) for hiding the console window a short-lived console subprocess would otherwise flash on Windows. Three spawn sites don't follow it:

  1. hermes_cli/session_lost_and_found.py (brand-new file, last-resort page-level session-DB salvage): _cli_supports_recover()'s capability probe (subprocess.run) and run_cli_lost_and_found_recover()'s dump/load Popen pair. This feature (hermes sessions recover --allow-partial) is meant to work cross-platform — the sqlite3 CLI it shells out to ships for Windows too — and has no platform guard restricting it to POSIX.
  2. tools/subagent_worktree.py (brand-new file, opt-in per-subagent git-worktree isolation): _run_git(), the single chokepoint all 7 call sites (rev-parse --show-toplevel, rev-parse HEAD, worktree add, rev-list --count, status --porcelain, worktree remove, branch -D) go through. Fires once per delegated subagent when delegation.worktree_isolation is enabled, so the flash frequency here is higher than the other two sites.
  3. hermes_cli/plugins_cmd.py: 6 git subprocess.run sites (clone, fetch --depth 1, checkout --detach, remote set-url credential-scrub, HEAD rev-parse, and the shared _run_plugin_git helper used by plugin-pack update flows) — otherwise carefully hardened (noninteractive_git_env(), stdin=DEVNULL, credential-scrubbed error text, timeouts) but missing this one flag.

Fix

Add creationflags=windows_hide_flags() to all 9 spawn sites across the 3 files, matching the exact pattern already used throughout tools/browser_tool.py, tools/env_probe.py, tools/lazy_deps.py, etc. windows_hide_flags() returns 0 on non-Windows, so this is a no-op there.

Testing

  • Added 4 new tests to tests/test_windows_subprocess_no_window_flags.py (the repo's existing, growing audit suite for exactly this bug class), following its established monkeypatch.setattr(<module>, "windows_hide_flags", lambda: _CREATE_NO_WINDOW) + fake subprocess.run/Popen + assert-on-creationflags pattern:
    • test_session_lost_and_found_recover_probe_hides_console_window
    • test_session_lost_and_found_recover_run_hides_console_window (covers both Popen calls in the dump/load pipeline)
    • test_subagent_worktree_run_git_hides_console_window
    • test_plugins_cmd_git_helpers_hide_console_window (spot-checks the two standalone chokepoints, _git_head_revision and _run_plugin_git)
  • Mutation-verified: reverted the 3 production files (kept the new tests) and confirmed all 4 new tests fail against pre-fix code — the module doesn't even expose windows_hide_flags to monkeypatch before the fix, since it isn't imported yet.
  • tests/test_windows_subprocess_no_window_flags.py: 11 passed, 3 skipped (unrelated windows_only-marked tests).
  • Broader neighbor sweep — every existing test file for the 3 touched modules (test_session_recovery_lost_and_found.py, test_session_recovery.py, test_subagent_worktree.py, test_plugins_cmd.py, test_plugins_cmd_category_discovery.py, test_plugins_cmd_list.py, test_plugins_cmd_enable_disable_nested.py): 95/95 passed.
  • ruff check clean on all 4 touched files.

No behavior change on non-Windows or to stdio/timeout/env handling on any platform.

…ites

Three call sites shell out without hiding their console window on
Windows, using the established creationflags=windows_hide_flags()
pattern this repo already applies to short-lived console subprocess
spawns elsewhere:

- hermes_cli/session_lost_and_found.py (new file): the sqlite3-CLI
  .recover capability probe (subprocess.run) and the dump/load Popen
  pair that streams page-level recovery output.
- tools/subagent_worktree.py (new file): _run_git(), the single
  chokepoint all 7 worktree-isolation git calls go through.
- hermes_cli/plugins_cmd.py: 6 git subprocess.run sites (clone, fetch,
  checkout --detach, remote set-url, HEAD rev-parse, and the shared
  plugin-update _run_plugin_git helper).

None change behavior on non-Windows (windows_hide_flags() returns 0
there) or alter stdio/timeout/env handling on any platform.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage P2 Medium — degraded but workaround exists sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Aug 13, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

fix: hide Windows console-flash on 3 new/untreated subprocess spawn sites

Solid, low-risk change — windows_hide_flags() returns 0 on non-Windows (verified in _subprocess_compat.py), so the cross-platform call sites are safe. A few observations:

  1. Partial test coverage of plugins_cmd.py sites. The new test_plugins_cmd_git_helpers_hide_console_window claims to spot-check "the two standalone chokepoints", but the diff adds creationflags to six sites in plugins_cmd.py (_git_head_revision, both _checkout_exact_revision branches, _scrub_cloned_origin, _install_plugin_core, _run_plugin_git). Only _git_head_revision and _run_plugin_git are exercised. _install_plugin_core is the install path users hit first on Windows — consider adding it to the spot-check.
  2. POSIX no-op isn't asserted. The new tests monkeypatch windows_hide_flags unconditionally, so they never verify that on non-Windows the real helper yields a harmless creationflags=0 (the actual if not IS_WINDOWS: return 0 branch). An existing test for the helper may cover this, but nothing ties it to these call sites.
  3. Minor: test_subagent_worktree_run_git_hides_console_window passes cwd="/tmp", which doesn't exist on Windows; harmless since subprocess.run is mocked, but a platform-neutral temp path would read more clearly.
  4. run_cli_lost_and_found_recover correctly flags both Popen spawns in the dump→load pipeline — nice to see the pipe pair treated as two spawn sites rather than one.

This branch has not been deployed

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants