Merge pull request #63955 from NousResearch/bb/fix-windows-broken-login-bash

fix(windows): survive broken Git Bash login shells

tests/tools/test_base_environment.py

@@ -161,8 +161,8 @@ class TestAtomicSnapshotWrite:
         captured = {}
 
         def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
-            captured["cmd"] = cmd_string
-            raise RuntimeError("stop after capture")  # we only need the script
+            captured.setdefault("cmd", cmd_string)  # only the bootstrap; ignore the failure-path probe
+            raise RuntimeError("stop after capture")
 
         env._run_bash = fake_run_bash  # type: ignore[assignment]
         try:
@@ -188,7 +188,7 @@ class TestAtomicSnapshotWrite:
         captured = {}
 
         def fake_run_bash(cmd_string, *, login=False, timeout=120, stdin_data=None):
-            captured["cmd"] = cmd_string
+            captured.setdefault("cmd", cmd_string)  # only the bootstrap; ignore the failure-path probe
             raise RuntimeError("stop after capture")
 
         env._run_bash = fake_run_bash  # type: ignore[assignment]
@@ -437,6 +437,41 @@ class TestInitSessionFailure:
         assert len(calls) == 1
         assert calls[0]["login"] is True
 
+    def test_prefer_nonlogin_when_login_bash_is_dead(self):
+        """Login snapshot failure + working non-login probe → don't use bash -l."""
+        env = _TestableEnv()
+
+        def mock_run_bash(cmd, *, login=False, timeout=120, stdin_data=None):
+            mock = MagicMock()
+            mock.poll.return_value = 0
+            mock.stdout = iter([])
+            if login:
+                mock.returncode = 1
+            else:
+                mock.returncode = 0
+            return mock
+
+        env._run_bash = mock_run_bash
+        env.init_session()
+
+        assert env._snapshot_ready is False
+        assert env._prefer_nonlogin is True
+
+        calls = []
+
+        def track_run_bash(cmd, *, login=False, timeout=120, stdin_data=None):
+            calls.append({"login": login})
+            mock = MagicMock()
+            mock.poll.return_value = 0
+            mock.returncode = 0
+            mock.stdout = iter([])
+            return mock
+
+        env._run_bash = track_run_bash
+        env.execute("echo test")
+
+        assert calls[0]["login"] is False
+
 
 class TestCwdMarker:
     def test_marker_contains_session_id(self):

tests/tools/test_find_shell.py

@@ -116,6 +116,33 @@ class TestFindBashUnchanged:
         assert len(result) > 0
 
 
+class TestFindBashSkipsBrokenCustomPath:
+    """Stale HERMES_GIT_BASH_PATH must not brick Windows terminal startup."""
+
+    def test_falls_through_to_portable_when_custom_fails_probe(self, tmp_path, monkeypatch):
+        import tools.environments.local as local_mod
+
+        monkeypatch.setattr(local_mod, "_IS_WINDOWS", True)
+        local_mod._bash_starts_cache.clear()
+
+        broken = tmp_path / "broken" / "bash.exe"
+        broken.parent.mkdir()
+        broken.write_text("", encoding="utf-8")
+        portable = tmp_path / "hermes" / "git" / "bin" / "bash.exe"
+        portable.parent.mkdir(parents=True)
+        portable.write_text("", encoding="utf-8")
+
+        monkeypatch.setenv("HERMES_GIT_BASH_PATH", str(broken))
+        monkeypatch.setenv("LOCALAPPDATA", str(tmp_path))
+
+        def fake_starts(path: str) -> bool:
+            return path == str(portable)
+
+        monkeypatch.setattr(local_mod, "_bash_starts", fake_starts)
+
+        assert _find_bash() == str(portable)
+
+
 @pytest.mark.skipif(
     not os.path.isfile("/bin/bash") or sys.platform != "darwin",
     reason="reproduces the macOS system-bash-3.2 login-shell swallow",

tests/tools/test_local_env_windows_msys.py

@@ -353,7 +353,7 @@ class TestWrapCommandWindowsNativeCwd:
         captured = {}
 
         def fake_run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None):
-            captured["script"] = cmd_string
+            captured.setdefault("script", cmd_string)  # bootstrap only; ignore the failure-path probe
             raise RuntimeError("stop after capturing bootstrap")
 
         monkeypatch.setattr(LocalEnvironment, "_run_bash", fake_run_bash)
@@ -373,7 +373,7 @@ class TestWrapCommandWindowsNativeCwd:
         captured = {}
 
         def fake_run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None):
-            captured["script"] = cmd_string
+            captured.setdefault("script", cmd_string)  # bootstrap only; ignore the failure-path probe
             raise RuntimeError("stop after capturing bootstrap")
 
         monkeypatch.setattr(LocalEnvironment, "_run_bash", fake_run_bash)
@@ -402,7 +402,7 @@ class TestWrapCommandWindowsNativeCwd:
         captured = {}
 
         def fake_run_bash(self, cmd_string, *, login=False, timeout=120, stdin_data=None):
-            captured["script"] = cmd_string
+            captured.setdefault("script", cmd_string)  # bootstrap only; ignore the failure-path probe
             raise RuntimeError("stop after capturing bootstrap")
 
         monkeypatch.setattr(LocalEnvironment, "_run_bash", fake_run_bash)

tools/environments/base.py

@@ -321,6 +321,10 @@ class BaseEnvironment(ABC):
         self._cwd_file = f"{temp_dir}/hermes-cwd-{self._session_id}.txt"
         self._cwd_marker = _cwd_marker(self._session_id)
         self._snapshot_ready = False
+        # When True, login bash is unusable (e.g. broken Git-for-Windows
+        # ``Directory \\drivers\\etc`` startup) so execute() must not fall
+        # back to ``bash -l`` per command — use non-login ``bash -c`` instead.
+        self._prefer_nonlogin = False
 
     # ------------------------------------------------------------------
     # Abstract methods
@@ -437,13 +441,37 @@ class BaseEnvironment(ABC):
                 self.cwd,
             )
         except Exception as exc:
-            logger.warning(
-                "init_session failed (session=%s): %s — "
-                "falling back to bash -l per command",
-                self._session_id,
-                exc,
-            )
             self._snapshot_ready = False
+            # Default fallback is bash -l per command so PATH/nvm/etc still
+            # load.  If login itself is dead (classic Windows Git Bash
+            # ``Directory \\drivers\\etc does not exist``), that fallback
+            # would brick every tool — prefer non-login bash -c instead.
+            detail = str(exc)
+            prefer_nonlogin = False
+            try:
+                probe = self._run_bash("true", login=False, timeout=min(15, self._snapshot_timeout))
+                probe_result = self._wait_for_process(probe, timeout=min(15, self._snapshot_timeout))
+                prefer_nonlogin = int(probe_result.get("returncode") or 0) == 0
+                if not prefer_nonlogin:
+                    detail = (probe_result.get("stdout") or detail).strip() or detail
+            except Exception as probe_exc:
+                detail = f"{detail}; non-login probe: {probe_exc}"
+
+            self._prefer_nonlogin = prefer_nonlogin
+            if prefer_nonlogin:
+                logger.warning(
+                    "init_session failed (session=%s): %s — "
+                    "login bash unusable; falling back to non-login bash -c",
+                    self._session_id,
+                    exc,
+                )
+            else:
+                logger.warning(
+                    "init_session failed (session=%s): %s — "
+                    "falling back to bash -l per command",
+                    self._session_id,
+                    detail,
+                )
 
     # ------------------------------------------------------------------
     # Command wrapping
@@ -933,8 +961,9 @@ class BaseEnvironment(ABC):
 
         wrapped = self._wrap_command(exec_command, effective_cwd)
 
-        # Use login shell if snapshot failed (so user's profile still loads)
-        login = not self._snapshot_ready
+        # Use login shell if snapshot failed (so user's profile still loads),
+        # unless login itself is broken — then non-login is the only path.
+        login = not self._snapshot_ready and not self._prefer_nonlogin
 
         proc = self._run_bash(
             wrapped, login=login, timeout=effective_timeout, stdin_data=effective_stdin

tools/environments/local.py

@@ -558,14 +558,15 @@ def _find_bash() -> str:
             or "/bin/sh"
         )
 
+    candidates: list[str] = []
+
     custom = os.environ.get("HERMES_GIT_BASH_PATH")
     if custom and os.path.isfile(custom):
-        return custom
+        candidates.append(custom)
 
-    # Prefer our own portable Git install first — this way a broken or
-    # partially-uninstalled system Git can't hijack the bash lookup.  The
-    # install.ps1 installer always drops portable Git here when the user
-    # didn't already have a working system Git.
+    # Prefer our own portable Git install — a broken or partially-uninstalled
+    # system Git (or a stale HERMES_GIT_BASH_PATH pointing at one) must not
+    # brick the terminal.  install.ps1 drops PortableGit here when needed.
     #
     # Layouts (both checked so upgrades between MinGit and PortableGit
     # installs work transparently):
@@ -578,8 +579,8 @@ def _find_bash() -> str:
             os.path.join(_hermes_portable_git, "bin", "bash.exe"),        # PortableGit (primary)
             os.path.join(_hermes_portable_git, "usr", "bin", "bash.exe"), # MinGit fallback
         ):
-            if os.path.isfile(candidate):
-                return candidate
+            if os.path.isfile(candidate) and candidate not in candidates:
+                candidates.append(candidate)
 
     # Check known Git for Windows install locations before PATH lookup.
     # On machines with both WSL and Git for Windows, shutil.which("bash")
@@ -588,14 +589,33 @@ def _find_bash() -> str:
     for candidate in (
         os.path.join(os.environ.get("ProgramFiles", r"C:\Program Files"), "Git", "bin", "bash.exe"),
         os.path.join(os.environ.get("ProgramFiles(x86)", r"C:\Program Files (x86)"), "Git", "bin", "bash.exe"),
-        os.path.join(_local_appdata, "Programs", "Git", "bin", "bash.exe"),
+        os.path.join(_local_appdata, "Programs", "Git", "bin", "bash.exe") if _local_appdata else "",
     ):
-        if candidate and os.path.isfile(candidate):
-            return candidate
+        if candidate and os.path.isfile(candidate) and candidate not in candidates:
+            candidates.append(candidate)
 
     found = shutil.which("bash")
-    if found:
-        return found
+    if found and found not in candidates:
+        candidates.append(found)
+
+    # Prefer the first candidate that can actually start.  A stale
+    # HERMES_GIT_BASH_PATH pointing at a broken Git-for-Windows install
+    # (``Directory \\drivers\\etc does not exist``) must not win over a
+    # healthy portable Git under %LOCALAPPDATA%\\hermes\\git.
+    for candidate in candidates:
+        if _bash_starts(candidate):
+            if candidate != custom and custom and os.path.isfile(custom):
+                logger.warning(
+                    "HERMES_GIT_BASH_PATH=%s fails to start; using %s instead",
+                    custom,
+                    candidate,
+                )
+            return candidate
+
+    if candidates:
+        # Last resort: return the first path even if the probe failed, so the
+        # caller still sees the real bash error instead of "not found".
+        return candidates[0]
 
     raise RuntimeError(
         "Git Bash not found. Hermes Agent requires Git for Windows on Windows.\n"
@@ -604,6 +624,40 @@ def _find_bash() -> str:
     )
 
 
+_bash_starts_cache: dict[str, bool] = {}
+
+
+def _bash_starts(bash: str) -> bool:
+    """True if *bash* can run a trivial non-login command.
+
+    Uses ``--noprofile --norc`` so a broken login post-install
+    (``Directory \\drivers\\etc``) does not falsely condemn an otherwise
+    usable bash.  Cached per path for the process lifetime.
+    """
+    cached = _bash_starts_cache.get(bash)
+    if cached is not None:
+        return cached
+
+    try:
+        result = subprocess.run(
+            [bash, "--noprofile", "--norc", "-c", "exit 0"],
+            capture_output=True,
+            text=True,
+            timeout=15,
+            creationflags=windows_hide_flags() if _IS_WINDOWS else 0,
+        )
+        ok = result.returncode == 0
+        if not ok:
+            combined = f"{result.stdout or ''}{result.stderr or ''}"
+            logger.debug("bash probe failed for %s: %s", bash, combined.strip()[:200])
+    except Exception as exc:
+        logger.debug("bash probe error for %s: %s", bash, exc)
+        ok = False
+
+    _bash_starts_cache[bash] = ok
+    return ok
+
+
 # POSIX-sh-family shells that understand the ``[shell, "-lic", "set +m; …"]``
 # invocation spawn_local uses. $SHELL values outside this set (fish, csh/tcsh,
 # nushell, elvish, xonsh, …) would error on that syntax, so _find_shell falls