fix(windows): native Git Bash terminal — exit 126 with empty output#14644
fix(windows): native Git Bash terminal — exit 126 with empty output#14644Prasanna28Devadiga wants to merge 1 commit into
Conversation
Three bugs compound to break every terminal() call on Windows when using the documented Git-Bash-backed local backend: 1. select.select does not accept pipe fds on Windows; _drain exits on the first iteration and captures no output. Branch on Windows and use blocking proc.stdout.read() instead. 2. get_temp_dir() falls back to "/tmp", which Python on Windows resolves to C:\tmp (non-existent) — _update_cwd's open() always FileNotFoundErrors silently. Return the Windows temp dir with forward slashes (C:/Users/.../Temp) so both Python and Git Bash can use it. 3. _wrap_command emits `builtin cd 'C:\Users\...' || exit 126`, but Git Bash's builtin cd does not accept single-quoted Windows backslash paths — it needs POSIX form (/c/Users/...). Add _win_to_posix_path and override LocalEnvironment._wrap_command on Windows. Paired _posix_to_win_path converts Git Bash POSIX cwds back to Windows form for subprocess.Popen(cwd=). Verified end-to-end on Windows 11 + Git for Windows: echo, pwd, multi-line output, non-zero exits, and cwd persistence across calls all pass. Known gaps (left for follow-ups): pty=True (uses Unix pty module), process-tree kill on Windows, browser tool WinError 193 from unescaped npm .cmd shims, Docker/SSH backend cwd translation. Refs: NousResearch#14638
|
Closing — reporter moved to WSL2 (Fedora) and confirmed Hermes works there once the minimal Fedora distro is filled in with |
|
Reopening — I'd closed this prematurely because I'd personally moved to WSL, but @alt-glitch's note above makes clear the fix is wanted upstream to supersede #12795 and #13397. Branch is unchanged and tests still pass on my Windows 11 + Git for Windows setup. Happy to rebase or split if that helps review. |
|
Thanks for this — appreciate the work. We're closing the entire cluster of open native-Windows PRs (44 of them spanning installer, terminal routing, file ops, gateway PID handling, encoding, docs, and more) because the surface area needs a designed, consolidated approach rather than piecemeal merges. Cherry-picking individual fixes keeps leaving inconsistencies and we'd rather land Windows support properly, in one coherent pass.\n\nYour PR is catalogued in our internal Windows support plan. When we pick this back up (soon), we'll mine every PR in the cluster for its fix shape and credit all contributors whose work informs the final patch via lines. Watch for the consolidating PR and feel free to chime in with context on the specific failure mode you were hitting.\n\nClosing for now, not as a rejection of the fix — just queueing it for the designed rollout. Thanks again. |
Fixes #14638.
What this PR does
Makes the native-Windows / Git Bash
terminal.backend: localpath actually work. Currently everyterminal()call returnsexit_code: 126with empty output; after this patch,echo helloreturnshello, cwd persists across calls, and multi-line stdout is captured.Root causes (three compounding bugs)
select.selectdoes not accept pipe fds on Windows._draininbase.pyhitsOSError 10038on the first iteration and exits without reading stdout → empty output,self.cwdnever updated from the__HERMES_CWD__marker.get_temp_dir()falls back to/tmp. Python on Windows resolves that toC:\tmp(doesn't exist), so_update_cwd'sopen()silentlyFileNotFoundErrors.builtin cd 'C:\Users\...' \|\| exit 126— Git Bash's builtincddoes not accept single-quoted Windows backslash paths. Needs POSIX form (/c/Users/...). This is the direct source of the 126.Fix
base.py: Windows branch in_drainuses blockingproc.stdout.read(4096)instead ofselect.select(Py 3.7+ subprocess on Windows doesn't propagate pipe handles to grandchildren, so EOF arrives promptly).local.py:get_temp_dir()returnstempfile.gettempdir().replace('\', '/')on Windows — path valid for both Python and Git Bash.local.py: new_win_to_posix_path/_posix_to_win_pathhelpers.LocalEnvironment._wrap_commandis overridden to convert cwd before interpolation;_run_bashconverts back forsubprocess.Popen(cwd=).All three are Windows-only branches; non-Windows code paths are untouched.
Verified locally
On Windows 11 + Git for Windows:
Known gaps (NOT in this PR)
This fixes the core terminal path only. Still broken on Windows/Git Bash:
pty=True— importspty/termioswhich don't exist on Windows._kill_processon Windows onlyproc.terminate()s; backgrounded grandchildren can leak.WinError 193—npx.cmd/agent-browsershim launched withoutshell=True.Happy to follow up with separate PRs for any of these.
Scope decision is yours
I realize this might be against current direction — the install docs say "Requires WSL2 on Windows" while the code's
_find_bash()still searchesC:\Program Files\Git\bin\bash.exe. If Git Bash support was deliberately deprecated, happy to flip this PR into the opposite change instead: delete the Windows branch in_find_bash(), fail loud at init with a clear "install WSL2" message, so users don't waste time debugging mysterious exit-126s.Either direction is better than the current state (code half-supports Git Bash but silently fails). Let me know which you prefer and I'll adjust.
Why native Git Bash matters (user side, for context): WSL2 carries a ~1–2 GB RAM baseline, conflicts with VirtualBox/Android emulators via Hyper-V, and access to Windows-hosted repos incurs 9p filesystem perf costs. For users on constrained machines (which is the reporter's case), the native path is meaningfully cheaper — if it works.