From 9a84bee265daad14340a80d7585928cd8ea1f9eb Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 6 Sep 2026 13:22:10 +0530 Subject: [PATCH] refactor(ssh): drop probe-only bookkeeping the probe never reads - probe_only is consumed inside __init__ only; no instance attribute. - _sync_manager is assigned only on the probe branch, after the connection is up, so a normal SSHEnvironment whose constructor fails still behaves exactly as before (its __del__ cleanup does not reach the shared socket teardown). - _remote_home has no reader on the probe path; not assigned. - prompt_builder passes probe_only=True unconditionally: which backends honor it is the builder table's decision, not a caller-side env_type check. - Tests trimmed to the probe_only contract: the cleanup-raises case was green on main (pre-existing try/except), the socket-name length and double-cleanup assertions covered pre-existing behaviour. --- agent/prompt_builder.py | 8 ++++---- tests/agent/test_prompt_builder.py | 21 --------------------- tests/tools/test_ssh_environment.py | 3 --- tools/environments/ssh.py | 10 +++------- 4 files changed, 7 insertions(+), 35 deletions(-) diff --git a/agent/prompt_builder.py b/agent/prompt_builder.py index eb65ad3563..afc79fcc22 100644 --- a/agent/prompt_builder.py +++ b/agent/prompt_builder.py @@ -876,10 +876,10 @@ def _run_backend_probe(env_type: str, terminal_tool) -> str: container_config=(_container_config_from_config(config) if terminal_tool._is_container_backend(env_type) else None), task_id="prompt-backend-probe", host_cwd=config.get("host_cwd"), - # ssh: an isolated ControlMaster socket and no remote dir setup / file sync / snapshot — - # a normal SSHEnvironment would upload the whole ~/.hermes tree just to run `uname`, and - # its later __del__ would sync_back() and close the master shared with the agent's own env. - probe_only=env_type == "ssh", + # Only ssh honors this: an isolated ControlMaster socket and no remote dir setup / file sync / + # snapshot. A normal SSHEnvironment would upload the whole ~/.hermes tree just to run `uname`, + # and its later __del__ would sync_back() and close the master shared with the agent's own env. + probe_only=True, ) try: result = env.execute(_BACKEND_PROBE_CMD, timeout=4) diff --git a/tests/agent/test_prompt_builder.py b/tests/agent/test_prompt_builder.py index a3dd7becc7..af57363092 100644 --- a/tests/agent/test_prompt_builder.py +++ b/tests/agent/test_prompt_builder.py @@ -970,27 +970,6 @@ class TestEnvironmentHints: assert created["probe_only"] is True assert calls == ["cleanup"] - def test_probe_remote_backend_ssh_cleanup_error_keeps_result(self, monkeypatch): - """A failing teardown of the throwaway probe connection must not discard the - metadata the probe already collected.""" - import agent.prompt_builder as _pb - - monkeypatch.setenv("TERMINAL_ENV", "ssh") - _pb._clear_backend_probe_cache() - - class _Env: - def execute(self, cmd, timeout=None): - return {"returncode": 0, "output": "os=Linux\nkernel=6.8.0\nhome=/h\ncwd=/h\nuser=u\n"} - - def cleanup(self): - raise RuntimeError("cleanup failed") - - import tools.terminal_tool_backends as _tt - monkeypatch.setattr(_tt, "_create_environment", lambda **kw: _Env()) - - assert "Linux 6.8.0" in _pb._probe_remote_backend("ssh") - - def test_environment_hint_from_env_var_is_appended(self, monkeypatch): """HERMES_ENVIRONMENT_HINT lets an embedder describe the runtime env.""" import agent.prompt_builder as _pb diff --git a/tests/tools/test_ssh_environment.py b/tests/tools/test_ssh_environment.py index a05975429c..44c687992f 100644 --- a/tests/tools/test_ssh_environment.py +++ b/tests/tools/test_ssh_environment.py @@ -261,7 +261,6 @@ class TestSSHProbeOnly: _mock_ssh_runtime["_ensure_remote_dirs"].assert_not_called() _mock_ssh_runtime["sync_factory"].assert_not_called() _mock_ssh_runtime["init_session"].assert_not_called() - assert env._sync_manager is None def test_probe_only_control_socket_is_isolated(self, monkeypatch, _mock_ssh_runtime): control_exit_calls = [] @@ -283,12 +282,10 @@ class TestSSHProbeOnly: normal.control_socket.touch() first_probe.control_socket.touch() first_probe.cleanup() - first_probe.cleanup() assert normal.control_socket.exists() assert not first_probe.control_socket.exists() assert len(control_exit_calls) == 1 - normal.cleanup() def _setup_ssh_env(monkeypatch, persistent: bool): diff --git a/tools/environments/ssh.py b/tools/environments/ssh.py index 3c97735df2..290b81af49 100644 --- a/tools/environments/ssh.py +++ b/tools/environments/ssh.py @@ -48,7 +48,6 @@ class SSHEnvironment(BaseEnvironment): Spawn-per-call: every execute() spawns a fresh ``ssh ... bash -c`` process. Session snapshot preserves env vars across calls; CWD persists via in-band stdout markers. Uses SSH ControlMaster for connection reuse. - Probe-only instances use an isolated connection without sync or session state. """ # Passthrough values are re-forwarded on every command (see _run_bash), so like docker/local @@ -60,8 +59,6 @@ class SSHEnvironment(BaseEnvironment): probe_only: bool = False): super().__init__(cwd=cwd, timeout=timeout) self.host, self.user, self.port, self.key_path = host, user, port, key_path - self._probe_only = probe_only - self._sync_manager = None self.control_dir = Path(tempfile.gettempdir()) / "hermes-ssh" self.control_dir.mkdir(parents=True, exist_ok=True) # Short, deterministic socket name: the path must stay under macOS's 104-byte sun_path @@ -69,16 +66,15 @@ class SSHEnvironment(BaseEnvironment): # stability across reconnects keeps ControlMaster reuse working. A probe gets its own # per-instance socket so its cleanup() can never close the agent's shared master. socket_key = f"{user}@{host}:{port}" - if self._probe_only: + if probe_only: socket_key = f"{socket_key}:probe:{self._session_id}" _socket_id = hashlib.sha256(socket_key.encode()).hexdigest()[:16] self.control_socket = self.control_dir / f"{_socket_id}.sock" _ensure_ssh_available() self._establish_connection() - if self._probe_only: - self._remote_home = "" + if probe_only: + self._sync_manager = None return - self._remote_home = self._detect_remote_home() self._ensure_remote_dirs() self._sync_manager = FileSyncManager(