From f2fcf89c1f7ee4b4b2b894b5385cdf80a0dec2d9 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 13 Jul 2026 01:59:58 -0400 Subject: [PATCH] fix(windows): bash-safe snapshot paths after #63113 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #63113 rewrote native drive paths in ShellFileOperations, but init_session /_wrap_command still embedded C:/... hermes-snap paths from get_temp_dir. MSYS arg-converts those during bash -l and surfaces Directory \drivers\etc — including for relative write_file targets, since the wrapper is the fault. Add _bash_safe_path, override BaseEnvironment._quote_shell_path on LocalEnvironment (no base→local import), and normalize mixed /c/Users\... paths in file ops. Co-authored-by: xxxigm --- tests/tools/test_file_operations.py | 17 ++++ tests/tools/test_local_env_windows_msys.py | 98 +++++++++++++++++++++- tools/environments/base.py | 44 +++++----- tools/environments/local.py | 33 ++++++++ tools/file_operations.py | 17 ++-- 5 files changed, 181 insertions(+), 28 deletions(-) diff --git a/tests/tools/test_file_operations.py b/tests/tools/test_file_operations.py index ba3375f8b8ac..3b2c0ce59322 100644 --- a/tests/tools/test_file_operations.py +++ b/tests/tools/test_file_operations.py @@ -477,6 +477,23 @@ class TestShellFileOpsHelpers: # Non-drive paths are untouched. assert file_ops._escape_shell_arg("/tmp/foo") == "'/tmp/foo'" + def test_escape_shell_arg_normalizes_mixed_msys_paths(self, monkeypatch, file_ops): + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + mixed = r"/c/Users/Alexander\Documents\NewTEST\readme.txt" + assert file_ops._escape_shell_arg(mixed) == ( + "'/c/Users/Alexander/Documents/NewTEST/readme.txt'" + ) + + def test_escape_shell_arg_rewrites_forward_slash_native_paths(self, monkeypatch, file_ops): + import tools.environments.local as local_mod + + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + assert file_ops._escape_shell_arg( + "C:/Users/alice/notes.txt" + ) == "'/c/Users/alice/notes.txt'" + def test_read_file_uses_bash_safe_windows_paths(self, mock_env, monkeypatch): import tools.environments.local as local_mod diff --git a/tests/tools/test_local_env_windows_msys.py b/tests/tools/test_local_env_windows_msys.py index 3e35db414cf9..46e310dcdf48 100644 --- a/tests/tools/test_local_env_windows_msys.py +++ b/tests/tools/test_local_env_windows_msys.py @@ -20,12 +20,14 @@ on the real OS. from unittest.mock import patch - +from tools.environments.base import BaseEnvironment from tools.environments import local as local_mod from tools.environments.local import ( LocalEnvironment, + _bash_safe_path, _make_run_env, _msys_to_windows_path, + _quote_bash_path, _resolve_safe_cwd, _sanitize_subprocess_env, _windows_to_msys_path, @@ -112,6 +114,42 @@ class TestWindowsToMsysPath: assert _windows_to_msys_path(r"\\server\share") == r"\\server\share" +# --------------------------------------------------------------------------- +# _bash_safe_path / _quote_bash_path — shell-script interpolation +# --------------------------------------------------------------------------- + +class TestBashSafePath: + def test_native_windows_path_becomes_msys(self, monkeypatch): + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + assert _bash_safe_path(r"C:\Users\alice\notes.txt") == "/c/Users/alice/notes.txt" + + def test_forward_slash_native_path_becomes_msys(self, monkeypatch): + """Production get_temp_dir emits C:/... — still needs /c/... rewrite.""" + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + assert ( + _bash_safe_path("C:/Users/Alexander/.hermes/cache/terminal/hermes-snap-x.sh") + == "/c/Users/Alexander/.hermes/cache/terminal/hermes-snap-x.sh" + ) + + def test_mixed_msys_path_normalizes_backslashes(self, monkeypatch): + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + mixed = r"/c/Users/Alexander\Documents\NewTEST\readme.txt" + assert _bash_safe_path(mixed) == "/c/Users/Alexander/Documents/NewTEST/readme.txt" + + def test_noop_off_windows(self, monkeypatch): + monkeypatch.setattr(local_mod, "_IS_WINDOWS", False) + path = r"/c/Users\Alexander\Documents" + assert _bash_safe_path(path) == path + + def test_quote_bash_path_quotes_mixed_windows_path(self, monkeypatch): + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + quoted = _quote_bash_path( + r"C:\Users\Alexander\AppData\Local\Temp\hermes-snap-abc.sh" + ) + assert "/c/Users/Alexander/AppData/Local/Temp/hermes-snap-abc.sh" in quoted + assert "\\" not in quoted + + # --------------------------------------------------------------------------- # _resolve_safe_cwd — Windows fast path # --------------------------------------------------------------------------- @@ -326,3 +364,61 @@ class TestWrapCommandWindowsNativeCwd: assert "builtin cd -- /c/Users/liush 2>/dev/null || true" in captured["script"] assert r"C:\Users\liush" not in captured["script"] + + def test_init_session_bootstrap_quotes_snapshot_paths_in_msys_form(self, monkeypatch): + """Snapshot paths must reach bash as /c/... — C:/... still trips MSYS + arg conversion during bash -l and surfaces as \\drivers\\etc.""" + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + + captured = {} + + def fake_run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None): + captured["script"] = cmd_string + raise RuntimeError("stop after capturing bootstrap") + + monkeypatch.setattr(LocalEnvironment, "_run_bash", fake_run_bash) + + # Production shape: get_temp_dir forces forward slashes but keeps C:. + snap = "C:/Users/Alexander/.hermes/cache/terminal/hermes-snap-deadbeef.sh" + with patch.object(LocalEnvironment, "__init__", lambda self, **kw: None): + env = LocalEnvironment.__new__(LocalEnvironment) + BaseEnvironment.__init__( + env, + cwd=r"C:\Users\Alexander\Documents", + timeout=10, + ) + env._snapshot_path = snap + env._cwd_file = snap + ".cwd" + env.init_session() + + script = captured["script"] + assert "/c/Users/Alexander/.hermes/cache/terminal/hermes-snap-deadbeef.sh" in script + assert "C:/Users/Alexander" not in script + assert r"C:\Users\Alexander" not in script + + def test_init_session_bootstrap_rewrites_backslash_snapshot_paths(self, monkeypatch): + monkeypatch.setattr(local_mod, "_IS_WINDOWS", True) + + captured = {} + + def fake_run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None): + captured["script"] = cmd_string + raise RuntimeError("stop after capturing bootstrap") + + monkeypatch.setattr(LocalEnvironment, "_run_bash", fake_run_bash) + + snap = r"C:\Users\Alexander\AppData\Local\Temp\hermes-snap-deadbeef.sh" + with patch.object(LocalEnvironment, "__init__", lambda self, **kw: None): + env = LocalEnvironment.__new__(LocalEnvironment) + BaseEnvironment.__init__( + env, + cwd=r"C:\Users\Alexander\Documents", + timeout=10, + ) + env._snapshot_path = snap + env._cwd_file = snap + ".cwd" + env.init_session() + + script = captured["script"] + assert "/c/Users/Alexander/AppData/Local/Temp/hermes-snap-deadbeef.sh" in script + assert r"C:\Users\Alexander\AppData" not in script diff --git a/tools/environments/base.py b/tools/environments/base.py index 93d56c0c5ced..9762902cb224 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -367,15 +367,14 @@ class BaseEnvironment(ABC): # Without this the snapshot bootstrap ``cd`` below fails on Windows and # ``pwd -P`` captures the login shell's directory, not ``terminal.cwd``. _quoted_cwd = self._quote_cwd_for_cd(self.cwd) - # Quote the snapshot / cwd-file paths so Git Bash on Windows handles - # ``C:/Users/...``-shaped paths without glob-splitting the colon or - # tripping on drive letters. On POSIX this is a no-op (no colons / - # special chars in a /tmp path). Previously unquoted interpolation - # caused ``C:/Users/.../hermes-snap-*.sh: No such file or directory`` - # errors on Windows, leaking via stderr (merged into stdout on Linux - # backends) into every terminal-tool response. - _quoted_snap = shlex.quote(self._snapshot_path) - _quoted_cwd_file = shlex.quote(self._cwd_file) + # Quote snapshot / cwd-file paths via ``_quote_shell_path`` so the + # LocalEnvironment override can rewrite ``C:/...`` (and mixed + # ``/c/Users\\...``) to ``/c/...`` before quoting — bare drive paths + # in the bootstrap script trip MSYS into the + # ``Directory \\drivers\\etc does not exist`` failure class. + # On POSIX this is plain ``shlex.quote``. + _quoted_snap = self._quote_shell_path(self._snapshot_path) + _quoted_cwd_file = self._quote_shell_path(self._cwd_file) # Use atomic file replacement: assemble the snapshot in a temp file, # then mv it over the final path. This prevents concurrent source() # calls from reading a half-written snapshot when another terminal @@ -391,9 +390,9 @@ class BaseEnvironment(ABC): # mid-write, and mv would then publish a torn file (the corruption is # only narrowed, not closed). ``$BASHPID`` is the actual subshell PID # and is genuinely unique per writer, which closes the race. The - # static path is shlex-quoted (Windows/Git-Bash drive letters, spaces) + # static path is shell-quoted (Windows/Git-Bash drive letters, spaces) # with ``$BASHPID`` left outside the quotes so it still expands. - _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID" + _snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID" bootstrap = ( f"umask 077\n" f"export -p > {_snap_tmp}\n" @@ -461,25 +460,32 @@ class BaseEnvironment(ABC): return f"$HOME/{shlex.quote(cwd[2:])}" return shlex.quote(cwd) + def _quote_shell_path(self, path: str) -> str: + """Quote *path* for interpolation into a bash script. + + LocalEnvironment overrides this to rewrite native/mixed Windows + paths to ``/c/...`` before quoting. Remote backends leave paths + as-is (they already speak POSIX). + """ + return shlex.quote(path) + def _wrap_command(self, command: str, cwd: str) -> str: """Build the full bash script that sources snapshot, cd's, runs command, re-dumps env vars, and emits CWD markers.""" escaped = command.replace("'", "'\\''") - # Quote the snapshot / cwd-file paths so Git Bash on Windows handles - # ``C:/Users/...``-shaped paths without glob-splitting the colon or - # tripping on drive letters. POSIX paths are unaffected. See - # :meth:`init_session` for the same fix on the bootstrap block. - _quoted_snap = shlex.quote(self._snapshot_path) - _quoted_cwd_file = shlex.quote(self._cwd_file) + # Quote snapshot/cwd-file paths (see init_session — LocalEnvironment + # rewrites ``C:/...`` to ``/c/...`` so MSYS doesn't mangle them). + _quoted_snap = self._quote_shell_path(self._snapshot_path) + _quoted_cwd_file = self._quote_shell_path(self._cwd_file) # Use atomic file replacement for env snapshot updates (issue #38249). # Assemble into a per-writer-unique temp file, then mv to atomically # replace the snapshot so concurrent source() calls never read a # truncated/half-written file. ``$BASHPID`` (not ``$$``) is the actual # subshell PID — unique per concurrent ``&``-launched writer — so two # writers never share a temp name and clobber each other before the mv. - # Static path shlex-quoted (Windows/spaces); ``$BASHPID`` left to expand. - _snap_tmp = shlex.quote(self._snapshot_path + ".tmp.") + "$BASHPID" + # Static path shell-quoted (Windows/spaces); ``$BASHPID`` left to expand. + _snap_tmp = self._quote_shell_path(self._snapshot_path + ".tmp.") + "$BASHPID" parts = [] diff --git a/tools/environments/local.py b/tools/environments/local.py index 26d0b2a454c7..105283705192 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -66,6 +66,35 @@ def _windows_to_msys_path(cwd: str) -> str: return f"/{drive}/{tail}" if tail else f"/{drive}/" +def _bash_safe_path(path: str) -> str: + """Return *path* in a form safe to embed in a Git Bash script. + + Native ``C:\\Users\\x`` / ``C:/Users/x`` → ``/c/Users/x`` via + :func:`_windows_to_msys_path`. Mixed MSYS leftovers + (``/c/Users\\Alexander\\Documents``) get backslashes normalized so + bash does not eat ``\\U`` and trip the ``Directory \\drivers\\etc`` + failure class. No-op off Windows and for empty input. + + ``get_temp_dir`` already emits forward-slash ``C:/...`` forms for + Python compatibility; those still need the ``/c/...`` rewrite — + MSYS argument conversion treats ``C:/...`` as a Windows path and + can corrupt the login-shell ``drivers\\etc`` lookup. + """ + if not _IS_WINDOWS or not path: + return path + path = _windows_to_msys_path(path) + if "\\" in path: + path = path.replace("\\", "/") + return path + + +def _quote_bash_path(path: str) -> str: + """Quote *path* for safe interpolation into a Git Bash script on Windows.""" + import shlex + + return shlex.quote(_bash_safe_path(path)) + + def _resolve_safe_cwd(cwd: str) -> str: """Return ``cwd`` if it exists as a directory, else the nearest existing ancestor. Falls back to ``tempfile.gettempdir()`` only if walking up the @@ -994,6 +1023,10 @@ class LocalEnvironment(BaseEnvironment): """Use native paths for Python, but Git Bash-friendly paths for cd.""" return BaseEnvironment._quote_cwd_for_cd(_windows_to_msys_path(cwd)) + def _quote_shell_path(self, path: str) -> str: + """Rewrite native/mixed Windows paths before quoting for Git Bash.""" + return _quote_bash_path(path) + def _run_bash(self, cmd_string: str, *, login: bool = False, timeout: int = 120, stdin_data: str | None = None) -> subprocess.Popen: diff --git a/tools/file_operations.py b/tools/file_operations.py index 1a0227a41dff..76446befaa5e 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -960,16 +960,17 @@ class ShellFileOperations(FileOperations): def _escape_shell_arg(self, arg: str) -> str: """Escape a string for safe use in shell commands. - On Windows a native drive path (``C:\\Users\\x``) is first rewritten to - the Git Bash / MSYS ``/c/Users/x`` form: bash eats the backslashes and - MSYS otherwise mangles ``C:\\...`` (the "Directory \\drivers\\etc does not - exist" class of failures). Reuses the env-layer translator so shell file - ops and the terminal ``cd`` agree on the path form. No-op off Windows and - for non-drive-qualified paths. + On Windows native drive paths (``C:\\Users\\x`` / ``C:/Users/x``) + and mixed MSYS leftovers (``/c/Users\\x``) are rewritten to the + Git Bash ``/c/Users/x`` form via ``_bash_safe_path``: bash eats + backslashes and MSYS otherwise mangles drive paths into the + ``Directory \\drivers\\etc does not exist`` failure class. Reuses + the env-layer translator so shell file ops and the terminal ``cd`` + agree on the path form. No-op off Windows and for plain POSIX paths. """ - from tools.environments.local import _windows_to_msys_path + from tools.environments.local import _bash_safe_path - arg = _windows_to_msys_path(arg) + arg = _bash_safe_path(arg) # Use single quotes and escape any single quotes in the string return "'" + arg.replace("'", "'\"'\"'") + "'"