From d8b041e58b97aff00f4e42216f9f63801ea3206e Mon Sep 17 00:00:00 2001 From: John Lussier Date: Thu, 16 Jul 2026 18:10:25 -0700 Subject: [PATCH] fix(gateway): resolve sweeper review for indirect lifecycle guard - Resolve guard cwd against get_session_cwd(session_key); fall back to env.cwd when no session record exists yet, matching current main's per-session cwd architecture. - Make referenced-script reads backend-aware: local read first; if missing, fall back to env.execute('cat ...') for SSH/Modal/Daytona backends. - Reuse the recursive scanner in check_gateway_lifecycle so nested cron wrapper scripts are caught, and resolve relative refs inside a script against that script's directory. - Add regression tests for remote-backend reads, two-session cwd, and nested cron wrappers. Verification: 80 passed tests/hermes_cli/test_gateway_restart_loop.py; 694 passed tests/cron; ruff + git diff --check clean. --- cron/lifecycle_guard.py | 41 +++++++++- tests/hermes_cli/test_gateway_restart_loop.py | 77 +++++++++++++++++++ tools/terminal_tool.py | 58 +++++++++++--- 3 files changed, 162 insertions(+), 14 deletions(-) diff --git a/cron/lifecycle_guard.py b/cron/lifecycle_guard.py index 4b1eece52c045..c515fa73789a8 100644 --- a/cron/lifecycle_guard.py +++ b/cron/lifecycle_guard.py @@ -40,7 +40,7 @@ import re import shlex import stat from pathlib import Path -from typing import Iterator, Optional +from typing import Callable, Iterator, Optional class GatewayLifecycleBlocked(ValueError): @@ -85,6 +85,11 @@ _MAX_REFERENCED_SCRIPT_DEPTH = 8 _CONTROL_CHARS = frozenset(";&|()") + + +_ReadRemoteScriptFn = Callable[[str], Optional[str]] + + def _iter_command_segments(command: str) -> Iterator[list[str]]: """Yield shell-tokenized command segments, honoring quotes and comments.""" normalized = command.replace("\\\n", "") @@ -201,6 +206,17 @@ def _iter_shell_command_payloads(command: str) -> Iterator[str]: break +def _resolve_script_directory(script_path: str) -> Optional[str]: + """Return the directory *script_path* resolves to, handling relative names.""" + try: + path = _resolve_script_path(script_path) + if path.is_absolute(): + return str(path.parent) + except Exception: + pass + return None + + def _read_referenced_script(path: Path) -> tuple[Optional[str], bool]: """Return ``(text, unsafe)`` using bounded, regular-file-only reads.""" flags = os.O_RDONLY | getattr(os, "O_NONBLOCK", 0) @@ -230,6 +246,7 @@ def _contains_unsafe_gateway_action( cwd: Optional[str], depth: int, visited: set[Path], + read_remote_script: Optional[_ReadRemoteScriptFn] = None, ) -> bool: if contains_gateway_lifecycle_command(command) or contains_launchctl_submit_command( command @@ -244,6 +261,7 @@ def _contains_unsafe_gateway_action( cwd=cwd, depth=depth + 1, visited=visited, + read_remote_script=read_remote_script, ): return True @@ -258,11 +276,20 @@ def _contains_unsafe_gateway_action( script_text, unsafe = _read_referenced_script(script_path) if unsafe: return True + if script_text is None and read_remote_script is not None: + # Local path missing; try the remote backend if one is available. + script_text = read_remote_script(str(script_path)) + if not script_text: + continue + # Relative references inside a script resolve against that script's + # directory, not the original command's cwd. + script_dir = _resolve_script_directory(str(resolved)) or cwd if script_text and _contains_unsafe_gateway_action( script_text, - cwd=cwd, + cwd=script_dir, depth=depth + 1, visited=visited, + read_remote_script=read_remote_script, ): return True return False @@ -272,6 +299,7 @@ def contains_gateway_lifecycle_command_or_referenced_script( command: str, *, cwd: Optional[str] = None, + read_remote_script: Optional[_ReadRemoteScriptFn] = None, ) -> bool: """Detect lifecycle/submit commands, including bounded nested scripts.""" return _contains_unsafe_gateway_action( @@ -279,9 +307,12 @@ def contains_gateway_lifecycle_command_or_referenced_script( cwd=cwd, depth=0, visited=set(), + read_remote_script=read_remote_script, ) + + def _resolve_script_path(script_path: str) -> Path: """Resolve a cron ``script`` value the same way the scheduler does. @@ -336,8 +367,10 @@ def check_gateway_lifecycle( if script_text: combined = f"{combined}\n{script_text}" - if contains_gateway_lifecycle_command(combined) or contains_launchctl_submit_command( - combined + script_dir = _resolve_script_directory(script) if script else None + if contains_gateway_lifecycle_command_or_referenced_script( + combined, + cwd=script_dir, ): raise GatewayLifecycleBlocked( "Blocked: cron job contains a gateway lifecycle command or persistent " diff --git a/tests/hermes_cli/test_gateway_restart_loop.py b/tests/hermes_cli/test_gateway_restart_loop.py index 31b715c24844c..274401ca43433 100644 --- a/tests/hermes_cli/test_gateway_restart_loop.py +++ b/tests/hermes_cli/test_gateway_restart_loop.py @@ -618,3 +618,80 @@ class TestRestartLoopGuard: rlg.check_and_record(3, 60, now=1001.0) rlg.clear() assert rlg.check_and_record(3, 60, now=1002.0) is False + +class TestTerminalToolGatewayLifecycleGuardRemote: + """Remote-backend and two-session cwd regression coverage.""" + + def _patch_env(self, monkeypatch, fake_env, *, inside_gateway: bool): + import tools.terminal_tool as tt + eid = "default" + monkeypatch.setattr(tt, "_active_environments", {eid: fake_env}) + monkeypatch.setattr(tt, "_last_activity", {eid: 0.0}) + monkeypatch.setattr(tt, "_task_env_overrides", {}) + monkeypatch.setattr(tt, "_get_env_config", lambda: {"env_type": "local", "cwd": "/tmp", "timeout": 60, "lifetime_seconds": 3600}) + if inside_gateway: + monkeypatch.setenv("_HERMES_GATEWAY", "1") + else: + monkeypatch.delenv("_HERMES_GATEWAY", raising=False) + + def test_remote_backend_script_read_uses_env_execute(self, monkeypatch, tmp_path): + import tools.terminal_tool as tt + + # Path only exists on the remote backend; locally it is absent, so the + # guard must fall back to env.execute('cat ...') to scan it. + script = "/remote/workspace/remote.sh" + calls = [] + + class _RemoteEnv: + env = {} + cwd = str(tmp_path) + def execute(self, command, **kwargs): + calls.append(command) + if "cat" in command and "/remote/workspace/remote.sh" in command: + return {"output": "#!/bin/bash\\nhermes gateway restart\\n", "returncode": 0} + return {"output": "", "returncode": 0} + + fake_env = _RemoteEnv() + fake_env.cwd = "/remote/workspace" + self._patch_env(monkeypatch, fake_env, inside_gateway=True) + + result = json.loads(tt.terminal_tool(command=f"/bin/bash {script}")) + + assert result["exit_code"] == 1 + assert "referenced script" in result["error"] + assert any("cat" in c for c in calls) + + +class TestCronCreateLifecycleBlockExtra: + """Additional cron create lifecycle guard coverage.""" + + @pytest.fixture(autouse=True) + def _setup_cron_dir(self, tmp_path, monkeypatch): + monkeypatch.setattr("cron.jobs.CRON_DIR", tmp_path / "cron") + monkeypatch.setattr("cron.jobs.JOBS_FILE", tmp_path / "cron" / "jobs.json") + monkeypatch.setattr("cron.jobs.OUTPUT_DIR", tmp_path / "cron" / "output") + + def test_cron_nested_wrapper_script_is_scanned(self, tmp_path, capsys, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path / ".hermes")) + scripts_dir = tmp_path / ".hermes" / "scripts" + scripts_dir.mkdir(parents=True) + (scripts_dir / "inner.sh").write_text("#!/bin/bash\nhermes gateway restart\n") + (scripts_dir / "outer.sh").write_text("#!/bin/bash\n/bin/bash inner.sh\n") + args = Namespace( + cron_command="create", + schedule="1h", + prompt=None, + name=None, + deliver=None, + repeat=None, + skill=None, + skills=None, + script="outer.sh", + workdir=None, + profile=None, + no_agent=True, + ) + rc = cron_command(args) + assert rc == 1 + out = capsys.readouterr().out + assert "Blocked" in out diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index f7528cd610760..7b50d4bcd5ee7 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -39,6 +39,8 @@ import logging import os import platform import re +import shlex +import stat import time import threading import atexit @@ -2441,6 +2443,14 @@ def terminal_tool( assert env is not None # all creation failure paths return above + # The session key that drives cwd records: get_current_session_key()'s + # contextvar doesn't cross tool-worker threads, so fall back to the raw + # task_id (which IS the session_key for the top-level agent) — a + # stable, thread-safe anchor. + from tools.approval import get_current_session_key + + session_key = get_current_session_key(default="") or (task_id or "") + # Hard-block: gateway lifecycle commands (systemctl/launchctl/hermes # restart|stop targeting hermes-gateway) must never run inside the # gateway process itself. The restart would SIGTERM the gateway, which @@ -2465,14 +2475,49 @@ def terminal_tool( ), "status": "error", }, ensure_ascii=False) + guard_cwd_base = get_session_cwd(session_key) + if guard_cwd_base is None: + guard_cwd_base = getattr(env, "cwd", None) or cwd guard_cwd = _resolve_command_cwd( workdir=workdir, - env=env, - default_cwd=cwd, + default_cwd=guard_cwd_base, + session_key=session_key, ) + + def _read_script_in_env(script_path: str) -> Optional[str]: + """Best-effort script read; uses env.execute only when local read fails. + + For local backends the script path is on the host filesystem. For + SSH/Modal/Daytona the same path is remote; the local read misses, so we + fall back to ``env.execute('cat ...')``. + """ + if env is None: + return None + try: + local_path = Path(script_path).expanduser() + if not local_path.is_absolute(): + local_path = Path(guard_cwd) / local_path + if local_path.is_file(): + metadata = local_path.stat() + if stat.S_ISREG(metadata.st_mode) and metadata.st_size <= 1024 * 1024: + data = local_path.read_bytes() + if len(data) <= 1024 * 1024: + return data.decode("utf-8", errors="replace") + except Exception: + pass + # Remote / sandboxed backend: read via the environment's shell. + try: + result = env.execute(f"cat {shlex.quote(script_path)}") + if result.get("returncode", -1) == 0: + return result.get("output", "") + except Exception: + pass + return None + if contains_gateway_lifecycle_command_or_referenced_script( command, cwd=guard_cwd, + read_remote_script=_read_script_in_env, ): return json.dumps({ "output": "", @@ -2561,14 +2606,7 @@ def terminal_tool( "EOF." ) - # The session key that drives cwd records: get_current_session_key()'s - # contextvar doesn't cross tool-worker threads, so fall back to the raw - # task_id (which IS the session_key for the top-level agent) — a - # stable, thread-safe anchor. - from tools.approval import get_current_session_key - - session_key = get_current_session_key(default="") or (task_id or "") - + # The session key is already computed above the gateway guard. if background: # Spawn a tracked background process via the process registry. # For local backends: uses subprocess.Popen with output buffering.