diff --git a/api/agent_health.py b/api/agent_health.py index c57f9e5e..1a6c1d76 100644 --- a/api/agent_health.py +++ b/api/agent_health.py @@ -252,44 +252,65 @@ def _gateway_running_pid(gateway_status: Any, pid_path: Path | None) -> int | No return get_running_pid() -def _accepts_positional_pid_path(get_running_pid: Any) -> bool: +def _declared_pid_path_parameter( + get_running_pid: Any, +) -> inspect.Parameter | None: try: signature = inspect.signature(get_running_pid) except (TypeError, ValueError): - return False + return None for param in signature.parameters.values(): if param.kind not in ( inspect.Parameter.POSITIONAL_ONLY, inspect.Parameter.POSITIONAL_OR_KEYWORD, + inspect.Parameter.KEYWORD_ONLY, ): continue name = str(param.name or "").lower() - return "path" in name and "pid" in name - return False + if "path" in name and "pid" in name: + return param + return None def _gateway_running_pid_strict_path(gateway_status: Any, pid_path: Path) -> int | None: """Read a PID from an explicit path only; never fall back to ambient state.""" get_running_pid = gateway_status.get_running_pid - for kwargs in ( - {"pid_path": pid_path, "cleanup_stale": False}, - {"pid_path": pid_path}, - ): - try: - return get_running_pid(**kwargs) - except TypeError: - pass - - if not _accepts_positional_pid_path(get_running_pid): + pid_path_param = _declared_pid_path_parameter(get_running_pid) + if pid_path_param is None: return None - try: - return get_running_pid(pid_path, cleanup_stale=False) - except TypeError: + if pid_path_param.kind is inspect.Parameter.KEYWORD_ONLY: + kwargs = {pid_path_param.name: pid_path} try: - return get_running_pid(pid_path) + return get_running_pid(**kwargs, cleanup_stale=False) except TypeError: - return None + try: + return get_running_pid(**kwargs) + except TypeError: + return None + + if pid_path_param.kind is inspect.Parameter.POSITIONAL_OR_KEYWORD: + kwargs = {pid_path_param.name: pid_path} + try: + return get_running_pid(**kwargs, cleanup_stale=False) + except TypeError: + try: + return get_running_pid(**kwargs) + except TypeError: + pass + + if pid_path_param.kind in ( + inspect.Parameter.POSITIONAL_ONLY, + inspect.Parameter.POSITIONAL_OR_KEYWORD, + ): + try: + return get_running_pid(pid_path, cleanup_stale=False) + except TypeError: + try: + return get_running_pid(pid_path) + except TypeError: + return None + return None def get_active_profile_gateway_running_pid(profile: str | None = None) -> int | None: diff --git a/api/gateway_restart.py b/api/gateway_restart.py index 9c72a007..29ac5272 100644 --- a/api/gateway_restart.py +++ b/api/gateway_restart.py @@ -52,17 +52,23 @@ def _release_lock() -> None: pass -def _gateway_restart_profile_context(profile: str | None = None) -> tuple[Path, str]: +def _gateway_restart_profile_context(profile: str | None = None) -> tuple[Path, str | None]: """Return the HERMES_HOME and CLI profile arg for a gateway restart.""" if profile is None: raw_profile = str(get_active_profile_name() or "default").strip() active_home = Path(get_active_hermes_home()) else: - raw_profile = str(profile or "").strip() + raw_profile = str(profile or "") if not raw_profile or not _PROFILE_ID_RE.fullmatch(raw_profile): raise ValueError(f"Invalid profile for gateway restart: {profile!r}") active_home = Path(get_hermes_home_for_profile(raw_profile)) + if ( + raw_profile == "default" + and active_home.name == "default" + and active_home.parent.name == "profiles" + ): + return active_home, None if not raw_profile or not _PROFILE_ID_RE.fullmatch(raw_profile) or _is_root_profile(raw_profile): return active_home, "default" return active_home, raw_profile @@ -93,14 +99,24 @@ def restart_active_profile_gateway( env = os.environ.copy() env["HERMES_HOME"] = str(active_home) hermes_cmd = _resolve_hermes_command() - cmd = [hermes_cmd, "--profile", cli_profile, "gateway", "restart"] + cmd = [hermes_cmd] + if cli_profile is not None: + cmd.extend(["--profile", cli_profile]) + cmd.extend(["gateway", "restart"]) - logger.info( - "Restarting gateway service via CLI command: %s --profile %s gateway restart (HERMES_HOME=%s)", - hermes_cmd, - cli_profile, - active_home, - ) + if cli_profile is None: + logger.info( + "Restarting gateway service via CLI command: %s gateway restart (HERMES_HOME=%s)", + hermes_cmd, + active_home, + ) + else: + logger.info( + "Restarting gateway service via CLI command: %s --profile %s gateway restart (HERMES_HOME=%s)", + hermes_cmd, + cli_profile, + active_home, + ) proc = subprocess.Popen( cmd, stdout=subprocess.PIPE, diff --git a/tests/test_health_restart.py b/tests/test_health_restart.py index 0ccd4f25..b0a1003e 100644 --- a/tests/test_health_restart.py +++ b/tests/test_health_restart.py @@ -125,6 +125,30 @@ def test_restart_active_profile_gateway_pins_explicit_default_profile(monkeypatc assert called["env"]["HERMES_HOME"] == "/mock/hermes/default" +def test_restart_active_profile_gateway_omits_profile_for_isolated_default_home(monkeypatch): + gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock() + called = {} + + def fake_popen(args, stdout=None, stderr=None, text=True, env=None): + called["args"] = args + called["env"] = env + return MockPopen(args, stdout_text="ok", returncode=0, env=env) + + monkeypatch.setattr( + gateway_restart, + "get_hermes_home_for_profile", + lambda profile: "/mock/hermes/profiles/default", + ) + monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes") + monkeypatch.setattr(gateway_restart.subprocess, "Popen", fake_popen) + + result = gateway_restart.restart_active_profile_gateway(profile="default") + + assert result["status"] == "completed" + assert called["args"] == ["/mock/bin/hermes", "gateway", "restart"] + assert called["env"]["HERMES_HOME"] == "/mock/hermes/profiles/default" + + def test_restart_active_profile_gateway_rejects_malformed_explicit_profile(monkeypatch): gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock() @@ -133,7 +157,7 @@ def test_restart_active_profile_gateway_rejects_malformed_explicit_profile(monke monkeypatch.setattr(gateway_restart.subprocess, "Popen", fail_popen) - for profile in ("", "../bad"): + for profile in ("", " default", "default ", "default\n", "../bad", "bad;echo"): result = gateway_restart.restart_active_profile_gateway(profile=profile) assert result["status"] == "failed" @@ -141,6 +165,35 @@ def test_restart_active_profile_gateway_rejects_malformed_explicit_profile(monke assert gateway_restart._GATEWAY_RESTART_LOCK.locked() is False +def test_restart_active_profile_gateway_accepts_renamed_root_alias(monkeypatch): + gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock() + called = {} + + def fake_popen(args, stdout=None, stderr=None, text=True, env=None): + called["args"] = args + called["env"] = env + return MockPopen(args, stdout_text="ok", returncode=0, env=env) + + monkeypatch.setattr( + gateway_restart, + "get_hermes_home_for_profile", + lambda profile: "/mock/hermes/root" if profile == "rootalias" else "/mock/hermes/other", + ) + monkeypatch.setattr( + gateway_restart, + "_is_root_profile", + lambda profile: profile in {"default", "rootalias"}, + ) + monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes") + monkeypatch.setattr(gateway_restart.subprocess, "Popen", fake_popen) + + result = gateway_restart.restart_active_profile_gateway(profile="rootalias") + + assert result["status"] == "completed" + assert called["args"] == ["/mock/bin/hermes", "--profile", "default", "gateway", "restart"] + assert called["env"]["HERMES_HOME"] == "/mock/hermes/root" + + def test_restart_active_profile_gateway_failure_preserves_empty_output_contract(monkeypatch): gateway_restart._GATEWAY_RESTART_LOCK = threading.Lock() diff --git a/tests/test_update_banner_fixes.py b/tests/test_update_banner_fixes.py index 823a163a..55d332d2 100644 --- a/tests/test_update_banner_fixes.py +++ b/tests/test_update_banner_fixes.py @@ -1091,6 +1091,113 @@ class TestAgentUpdateRequiresGatewayRestart: ] assert [call[1]["HERMES_HOME"] for call in calls["popen"]] == [str(root_home), str(root_home)] + def test_agent_gateway_restart_kwargs_wrapper_pid_change_fails_closed( + self, + monkeypatch, + tmp_path, + ): + from api import agent_health, gateway_restart, profiles + import api.updates as upd + + root_home = tmp_path / ".hermes" + sticky_home = root_home / "profiles" / "work" + sticky_home.mkdir(parents=True) + calls = {"popen": [], "ambient_pid": 0} + + class FailedRestartProcess: + returncode = 7 + + def communicate(self, timeout=None): + return "", "restart failed" + + class AmbientKwargsGatewayStatus: + def __init__(self): + self._pids = iter([201, 202]) + + def get_running_pid(self, **kwargs): + calls["ambient_pid"] += 1 + return next(self._pids) + + def fake_popen(args, stdout=None, stderr=None, text=True, env=None): + calls["popen"].append((args, dict(env or {}))) + return FailedRestartProcess() + + monkeypatch.setattr(profiles, "_DEFAULT_HERMES_HOME", root_home) + monkeypatch.setattr(profiles, "_active_profile", "work") + monkeypatch.setattr(gateway_restart, "_GATEWAY_RESTART_LOCK", threading.Lock()) + monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes") + monkeypatch.setattr(gateway_restart.subprocess, "Popen", fake_popen) + monkeypatch.setattr(agent_health, "_gateway_status_module", AmbientKwargsGatewayStatus) + monkeypatch.setattr(upd.time, 'sleep', lambda seconds: None) + + profiles.set_request_profile("default") + try: + ok, result = upd._ensure_gateway_restart_for_agent_update() + finally: + profiles.clear_request_profile() + + assert ok is False + assert result["status"] == "failed" + assert calls["ambient_pid"] == 0 + assert [call[0] for call in calls["popen"]] == [ + ["/mock/bin/hermes", "--profile", "default", "gateway", "restart"], + ["/mock/bin/hermes", "--profile", "default", "gateway", "restart"], + ] + assert [call[1]["HERMES_HOME"] for call in calls["popen"]] == [str(root_home), str(root_home)] + + def test_agent_gateway_restart_isolated_default_home_omits_profile_flag( + self, + monkeypatch, + tmp_path, + ): + from api import agent_health, gateway_restart, profiles + import api.updates as upd + + base_home = tmp_path / ".hermes" + isolated_home = base_home / "profiles" / "default" + isolated_home.mkdir(parents=True) + calls = {"popen": [], "pid_paths": []} + + class FailedRestartProcess: + returncode = 7 + + def communicate(self, timeout=None): + return "", "restart failed" + + class PathStrictGatewayStatus: + def get_running_pid(self, pid_path=None, cleanup_stale=False): + path = pathlib.Path(pid_path) if pid_path is not None else None + calls["pid_paths"].append(path) + if path == isolated_home / "gateway.pid": + return 101 + return None + + def fake_popen(args, stdout=None, stderr=None, text=True, env=None): + calls["popen"].append((args, dict(env or {}))) + return FailedRestartProcess() + + monkeypatch.setattr(profiles, "_INITIAL_HERMES_HOME", str(isolated_home)) + monkeypatch.setattr(profiles, "_INITIAL_ISOLATED_PROFILE_OPT_IN", "1") + monkeypatch.setattr(gateway_restart, "_GATEWAY_RESTART_LOCK", threading.Lock()) + monkeypatch.setattr(gateway_restart.shutil, "which", lambda cmd: "/mock/bin/hermes") + monkeypatch.setattr(gateway_restart.subprocess, "Popen", fake_popen) + monkeypatch.setattr(agent_health, "_gateway_status_module", lambda: PathStrictGatewayStatus()) + monkeypatch.setattr(upd.time, 'sleep', lambda seconds: None) + + ok, result = upd._ensure_gateway_restart_for_agent_update() + + assert ok is False + assert result["status"] == "failed" + assert calls["pid_paths"] == [isolated_home / "gateway.pid", isolated_home / "gateway.pid"] + assert [call[0] for call in calls["popen"]] == [ + ["/mock/bin/hermes", "gateway", "restart"], + ["/mock/bin/hermes", "gateway", "restart"], + ] + assert [call[1]["HERMES_HOME"] for call in calls["popen"]] == [ + str(isolated_home), + str(isolated_home), + ] + def test_apply_update_agent_requires_gateway_restart(self, tmp_path, monkeypatch): import api.updates as upd