Merge pull request #63621 from NousResearch/bb/salvage-63470-bash-paths

fix(windows): bash-safe snapshot paths after #63113 (supersedes #63470)

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
 

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

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 = []
 

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:

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("'", "'\"'\"'") + "'"