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.
This commit is contained in:
parent
31dc4f0912
commit
d8b041e58b
|
|
@ -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 "
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue