diff --git a/tests/tools/test_base_environment.py b/tests/tools/test_base_environment.py index 7b84b15787e6..2904f7f7e336 100644 --- a/tests/tools/test_base_environment.py +++ b/tests/tools/test_base_environment.py @@ -4,9 +4,10 @@ init_session() failure handling, and the CWD marker contract. """ +import sys from unittest.mock import MagicMock -from tools.environments.base import BaseEnvironment, _BoundedOutputCollector +from tools.environments.base import BaseEnvironment, _BoundedOutputCollector, _popen_bash class _TestableEnv(BaseEnvironment): @@ -520,3 +521,21 @@ def test_unique_per_instance(self): env1 = _TestableEnv() env2 = _TestableEnv() assert env1._cwd_marker != env2._cwd_marker + + +class TestPopenBashDecoding: + """Regression for issue #37130. + + ``_popen_bash`` reads subprocess stdout in text mode. Without + ``errors="replace"`` a non-UTF8 byte (common from Windows OpenSSH / + Git Bash targets) raises ``UnicodeDecodeError`` the moment the output + is read, instead of being substituted with U+FFFD. + """ + + def test_non_utf8_output_is_replaced_not_raised(self): + proc = _popen_bash( + [sys.executable, "-c", "import os; os.write(1, b'\\xff\\xfe')"] + ) + out = proc.stdout.read() + proc.wait() + assert "�" in out diff --git a/tests/tools/test_ssh_environment.py b/tests/tools/test_ssh_environment.py index 09f090297a27..a0c532284756 100644 --- a/tests/tools/test_ssh_environment.py +++ b/tests/tools/test_ssh_environment.py @@ -201,6 +201,121 @@ def _fake_establish(self): assert env.user == "alice" +class TestNonUTF8SubprocessOutput: + """Regression tests for issue #37130. + + Windows OpenSSH targets running Git Bash/MSYS shells can emit non-UTF8 + bytes on stdout/stderr. The backend decoded subprocess output in strict + text mode (``text=True`` without ``errors=``), so a single undecodable + byte raised ``UnicodeDecodeError`` during ``_ensure_remote_dirs()`` — + before any user command ran — bricking all terminal/file/search tools + even though a manual SSH connection to the same host worked fine. + """ + + _BAD_BYTES = b"setup output \xff\xfe done\n" + + @staticmethod + def _decoding_run(stdout=b"", stderr=b"", returncode=0): + """A ``subprocess.run`` stand-in that reproduces real text-mode decoding. + + On the unfixed backend (no ``errors="replace"``) this raises + ``UnicodeDecodeError`` exactly as the real ``subprocess`` would; once + the backend passes ``errors="replace"`` it decodes with U+FFFD. + """ + def _run(cmd, *a, **kwargs): + out, err = stdout, stderr + if kwargs.get("text") or kwargs.get("universal_newlines"): + enc = kwargs.get("encoding") or "utf-8" + errs = kwargs.get("errors", "strict") + out = stdout.decode(enc, errors=errs) + err = stderr.decode(enc, errors=errs) + return subprocess.CompletedProcess(cmd, returncode, out, err) + return _run + + @pytest.fixture + def _ssh_env_factory(self, monkeypatch): + monkeypatch.setattr(ssh_env.shutil, "which", lambda _n: "/usr/bin/ssh") + monkeypatch.setattr("tools.environments.base.time.sleep", lambda _s: None) + from pathlib import Path as _Path + monkeypatch.setattr(_Path, "mkdir", lambda *a, **k: None) + monkeypatch.setattr(ssh_env, "FileSyncManager", lambda **kw: MagicMock()) + monkeypatch.setattr(ssh_env.SSHEnvironment, "init_session", + lambda self: None) + + def _factory(stdout=b"", stderr=b"", returncode=0): + monkeypatch.setattr( + "tools.environments.ssh.subprocess.run", + self._decoding_run(stdout, stderr, returncode), + ) + return ssh_env.SSHEnvironment(host="winhost", user="hermes") + return _factory + + def test_init_survives_non_utf8_output(self, _ssh_env_factory): + """Full __init__ (connect + detect home + _ensure_remote_dirs) must + not raise on non-UTF8 subprocess output — the issue #37130 crash.""" + env = _ssh_env_factory(stdout=self._BAD_BYTES) + assert env.host == "winhost" + + def test_init_survives_non_utf8_on_stderr(self, _ssh_env_factory): + env = _ssh_env_factory(stderr=self._BAD_BYTES, returncode=0) + assert env.user == "hermes" + + def test_detect_home_replaces_bad_bytes(self, _ssh_env_factory): + env = _ssh_env_factory(stdout=b"/home/hermes\xff\n") + assert env._remote_home == "/home/hermes�" + + def test_ensure_remote_dirs_does_not_raise(self, _ssh_env_factory): + """Directly re-exercise the method named in the issue traceback.""" + env = _ssh_env_factory(stdout=self._BAD_BYTES) + env._ensure_remote_dirs() + + def test_every_text_run_call_uses_errors_replace(self, monkeypatch, tmp_path): + """All text-mode ``subprocess.run`` calls in the backend — including + the file-sync sites that __init__ does not reach — must pass + ``errors="replace"`` so non-UTF8 output can never crash them.""" + calls = [] + + def _record(cmd, *a, **kwargs): + calls.append(kwargs) + return subprocess.CompletedProcess(cmd, 0, "", "") + + def _fake_popen(*a, **k): + m = MagicMock() + m.returncode = 0 + m.poll.return_value = 0 + m.communicate.return_value = (b"", b"") + m.stderr.read.return_value = b"" + return m + + monkeypatch.setattr(ssh_env.shutil, "which", lambda _n: "/usr/bin/ssh") + monkeypatch.setattr("tools.environments.base.time.sleep", lambda _s: None) + from pathlib import Path as _Path + monkeypatch.setattr(_Path, "mkdir", lambda *a, **k: None) + monkeypatch.setattr(ssh_env, "FileSyncManager", lambda **kw: MagicMock()) + monkeypatch.setattr(ssh_env.SSHEnvironment, "init_session", + lambda self: None) + monkeypatch.setattr("tools.environments.ssh.subprocess.run", _record) + monkeypatch.setattr("tools.environments.ssh.subprocess.Popen", _fake_popen) + + env = ssh_env.SSHEnvironment(host="winhost", user="hermes") + + src = tmp_path / "payload.txt" + src.write_text("x") + env._ssh_delete(["~/.hermes/a"]) + env._scp_upload(str(src), "~/.hermes/b") + env._ssh_bulk_upload([(str(src), f"{env._remote_home}/.hermes/sub/c")]) + + text_calls = [k for k in calls if k.get("text")] + assert text_calls, "expected text-mode subprocess.run calls" + for kwargs in text_calls: + assert kwargs.get("errors") == "replace", ( + f"text-mode subprocess.run missing errors='replace': {kwargs}" + ) + assert kwargs.get("encoding") == "utf-8", ( + f"text-mode subprocess.run missing encoding='utf-8': {kwargs}" + ) + + def _setup_ssh_env(monkeypatch, persistent: bool): monkeypatch.setenv("TERMINAL_ENV", "ssh") monkeypatch.setenv("TERMINAL_SSH_HOST", _SSH_HOST) diff --git a/tools/environments/base.py b/tools/environments/base.py index 1b20cfa90ee0..938d5ec33a76 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -249,6 +249,8 @@ def _popen_bash( stderr=subprocess.STDOUT, stdin=subprocess.PIPE if stdin_data is not None else subprocess.DEVNULL, text=True, + encoding="utf-8", + errors="replace", **kwargs, ) if stdin_data is not None: diff --git a/tools/environments/ssh.py b/tools/environments/ssh.py index 1a06ad9b82c0..67def61153ce 100644 --- a/tools/environments/ssh.py +++ b/tools/environments/ssh.py @@ -105,6 +105,8 @@ def _establish_connection(self): cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=15, stdin=subprocess.DEVNULL, ) @@ -123,6 +125,8 @@ def _detect_remote_home(self) -> str: cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=10, stdin=subprocess.DEVNULL, ) @@ -150,6 +154,8 @@ def _ensure_remote_dirs(self) -> None: cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=10, stdin=subprocess.DEVNULL, ) @@ -165,6 +171,8 @@ def _scp_upload(self, host_path: str, remote_path: str) -> None: mkdir_cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=10, stdin=subprocess.DEVNULL, ) @@ -179,6 +187,8 @@ def _scp_upload(self, host_path: str, remote_path: str) -> None: scp_cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=30, stdin=subprocess.DEVNULL, ) @@ -208,6 +218,8 @@ def _ssh_bulk_upload(self, files: list[tuple[str, str]]) -> None: cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=30, stdin=subprocess.DEVNULL, ) @@ -326,6 +338,8 @@ def _ssh_delete(self, remote_paths: list[str]) -> None: cmd, capture_output=True, text=True, + encoding="utf-8", + errors="replace", timeout=10, stdin=subprocess.DEVNULL, )