diff --git a/tests/tools/test_base_environment.py b/tests/tools/test_base_environment.py index d629d95ea299..44ff55859e4f 100644 --- a/tests/tools/test_base_environment.py +++ b/tests/tools/test_base_environment.py @@ -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): diff --git a/tests/tools/test_find_shell.py b/tests/tools/test_find_shell.py index 6de3b2594f90..b4bce6003150 100644 --- a/tests/tools/test_find_shell.py +++ b/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", diff --git a/tools/environments/base.py b/tools/environments/base.py index 9762902cb224..846003432c0a 100644 --- a/tools/environments/base.py +++ b/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 diff --git a/tools/environments/local.py b/tools/environments/local.py index 105283705192..e5f6c6c2b262 100644 --- a/tools/environments/local.py +++ b/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