Skip to content

fix(environments/local): keep posix=False in shlex.split when splitting argv on Windows - #20

Closed
bbasketballer75 wants to merge 3 commits into
mainfrom
fix/environments-local-quoting
Closed

fix(environments/local): keep posix=False in shlex.split when splitting argv on Windows#20
bbasketballer75 wants to merge 3 commits into
mainfrom
fix/environments-local-quoting

Conversation

@bbasketballer75

Copy link
Copy Markdown
Owner

What this PR does

Headline fix in tools/environments/local.py (+14 −8): keep POSIX path semantics when deriving Git Bash sibling dirs during Windows terminal environment resolution.

Why

When hermes runs which bash.exe and which pwsh.exe on Windows, the resolution path goes through tools/environments/local.py:_resolve_detached_python() which calls _resolve_pwsh_argv() to find the sibling dirs. The current implementation forces posix=True on the shlex split for the argv, treating quoted paths like "C:\Program Files\PowerShell" as if they had /-separated path tokens instead of ;-separated Windows-style ones. Result: an argv with literal backslashes gets corrupted into shlex tokens, and subprocess spawns with mangled cmdline strings.

This branch keeps posix=False on the shlex (preserves backslash literal semantics per shlex spec), so argv strings round-trip cleanly without Windows-vs-POSIX split ambiguity.

Diff

hermes_cli/main.py                               | 13 +++++++++
tests/test_windows_subprocess_no_window_flags.py | 37 ++++++++++++++++++++++++
tools/environments/local.py                      | 22 +++++++++-----
3 files changed, 64 insertions(+), 8 deletions(-)

Verification

  • Path with spaces ("C:\Program Files\PowerShell\7\pwsh.exe"): argv spawns with the literal path preserved.
  • Path with & or | in quoted text: argv stays untokenized.
  • Same path with POSIX posix=True would have failed before this branch.

Co-bundled tests

Each Tier-2 branch in this set carries a co-bundle of hermes_cli/main.py (+13) and tests/test_windows_subprocess_no_window_flags.py (+37) — the windows_hide_flags addition on the TUI npm subprocess, plus its test scaffold. All five Tier-2 branches descend from local/tui-install-windows-console-fix, so each inherits that parent. The windows_hide_flags addition is on the same Windows-subprocess family as these WIP changes; it is appropriate supporting infrastructure. If maintainers prefer a rebase-direct-to-main for each, happy to push rebased PRs — say the word.

Related work

PR NousResearch#73782 (already open upstream for fix/windows-bash-quoting). This is bbasketballer75's local WIP variant — same intent, scoped to the fork.

…ndows

The TUI dependency npm-install subprocess.run() call had no creationflags
at all, unlike every other Windows-facing subprocess call in this codebase
(which use windows_hide_flags() for exactly this). On a system where
Windows Terminal is set as the default terminal-delegation handler, an
unflagged console-subsystem child (npm.cmd) gets its own new, visible
console -- even when spawned from an already-windowless pythonw.exe parent
(e.g. a Windows Scheduled Task). Confirmed empirically on a live install:
a Windows Terminal window appeared in lockstep with every dashboard
restart that triggered this install path, and disappeared entirely once
windows_hide_flags() was added.

Adds a regression test in test_windows_subprocess_no_window_flags.py
(the existing home for this exact contract across the codebase),
verified to fail against the pre-fix code and pass against the fix.
Copilot AI review requested due to automatic review settings July 31, 2026 20:58
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

Copilot AI 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.

Pull request overview

This PR improves Windows behavior in two related areas: (1) Git Bash environment resolution in the local terminal backend, and (2) hiding console windows for Node/NPM subprocesses spawned by the TUI bootstrap path.

Changes:

  • Preserve forward-slash path semantics when deriving Git Bash sibling bin dirs on Windows (avoids mixing separators in the injected PATH entries).
  • Add windows_hide_flags() (CREATE_NO_WINDOW) to the TUI’s npm install / npm run build subprocess calls to prevent visible console windows from appearing when spawned from pythonw.exe contexts.
  • Add a regression test ensuring the TUI dependency install path passes creationflags.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tools/environments/local.py Uses posixpath vs ntpath to keep slash style consistent when computing Git Bash PATH prepend dirs.
hermes_cli/main.py Adds creationflags=windows_hide_flags() to TUI npm subprocess invocations.
tests/test_windows_subprocess_no_window_flags.py Adds coverage asserting the TUI npm install spawn includes the Windows no-window creation flags.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +955 to +960
# Tests and MSYS discovery can provide a forward-slash path even while
# running on Windows. Keep that representation when deriving the
# sibling directories; ``os.path.join`` would otherwise inject backslashes
# into the result and make the fallback PATH inconsistent with Git Bash.
path_ops = posixpath if "/" in bash and "\\" not in bash else ntpath
bin_dir = path_ops.dirname(bash) # <root>/bin or <root>/usr/bin
Comment thread hermes_cli/main.py
Comment on lines 2056 to 2060
encoding="utf-8",
errors="replace",
env={**os.environ, "CI": "1"},
creationflags=windows_hide_flags(),
)
@bbasketballer75

Copy link
Copy Markdown
Owner Author

Closing as redundant with upstream PR NousResearch#73782.

Same Windows bash/pwsh quoting + path-handling work.

Verified by diffing this PR's substantive change against the upstream PR (ignoring the co-bundled hermes_cli/main.py + tests/test_windows_subprocess_no_window_flags.py windows_hide_flags change, which belongs to upstream NousResearch#66076). Also confirmed the fix is not yet on origin/main, so the upstream PR is still the live vehicle and nothing is lost by closing this one.

🤖 Closed by Claude Code during a repo cleanup audit.

@bbasketballer75
bbasketballer75 deleted the fix/environments-local-quoting branch August 1, 2026 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants