From 7008fb81b3b22646dddf35d38fed2ed84595db42 Mon Sep 17 00:00:00 2001 From: Christopher <210261288+Christopher-Schulze@users.noreply.github.com> Date: Sat, 15 Aug 2026 15:41:02 +0200 Subject: [PATCH] fix(gateway): put --external-supervisor on launchd gateway argv hermes update decides restart ownership from the live grandchild argv, not from an env marker. Newly generated plists now include the flag. The stderr_timestamp wrapper upgrades only historical Hermes gateway run shapes for stale plists and leaves arbitrary launchd children unmarked. --- hermes_cli/gateway.py | 24 ++++- hermes_cli/stderr_timestamp.py | 68 +++++++++----- .../test_gateway_external_supervisor.py | 81 +++++++++++++++++ tests/hermes_cli/test_gateway_service.py | 1 + tests/hermes_cli/test_stderr_timestamp.py | 88 ++++++++++++++----- 5 files changed, 213 insertions(+), 49 deletions(-) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 7a24bf6d79..494df006e0 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -4419,8 +4419,22 @@ def _gateway_run_command() -> list[str]: return cmd -def _timestamped_stderr_gateway_command(error_log: Path) -> list[str]: - """Wrap gateway run so raw stderr lines are timestamped before file write.""" +def _timestamped_stderr_gateway_command( + error_log: Path, + *, + external_supervisor: bool = False, +) -> list[str]: + """Wrap gateway run so raw stderr lines are timestamped before file write. + + ``external_supervisor=True`` is for launchd ProgramArguments only: the + inner ``gateway run`` must carry ``--external-supervisor`` so + ``hermes update`` sees the flag on the live grandchild argv and hands + the process back to launchd instead of starting a detached watcher + (#86893 / #87005). The detached nohup fallback stays unmarked. + """ + inner = _gateway_run_command() + if external_supervisor and "--external-supervisor" not in inner: + inner = [*inner, "--external-supervisor"] return [ get_python_path(), "-m", @@ -4428,7 +4442,7 @@ def _timestamped_stderr_gateway_command(error_log: Path) -> list[str]: "--error-log", str(error_log), "--", - *_gateway_run_command(), + *inner, ] @@ -4526,7 +4540,9 @@ def generate_launchd_plist() -> str: # timestamps to raw stderr lines before they land in gateway.error.log. prog_args = [ f"{part}" - for part in _timestamped_stderr_gateway_command(err_path) + for part in _timestamped_stderr_gateway_command( + err_path, external_supervisor=True + ) ] prog_args_xml = "\n ".join(prog_args) diff --git a/hermes_cli/stderr_timestamp.py b/hermes_cli/stderr_timestamp.py index c9250e6214..b6dbac9966 100644 --- a/hermes_cli/stderr_timestamp.py +++ b/hermes_cli/stderr_timestamp.py @@ -13,7 +13,7 @@ from datetime import datetime from pathlib import Path from typing import BinaryIO, Sequence, TextIO -from gateway.restart import EXTERNAL_GATEWAY_SUPERVISOR_ENV +EXTERNAL_SUPERVISOR_FLAG = "--external-supervisor" _TIMESTAMP_PREFIX = re.compile( @@ -70,26 +70,53 @@ def _restore_signal_handlers(previous: dict[int, object]) -> None: signal.signal(signum, handler) -def _child_env_for_command( - environ: Mapping[str, str] | None = None, -) -> dict[str, str] | None: - """Preserve launchd supervision across this one-hop wrapper. - - launchd stamps ``XPC_SERVICE_NAME=`` only on its *direct* - child. This module is that child when the generated plist wraps - ``gateway run`` for timestamped stderr. The grandchild then sees - ``XPC_SERVICE_NAME=0`` and ``_guard_supervised_gateway_conflict`` - treats the service's own spawn as a foreign supervised gateway - (#86893). Forward the existing opt-in marker so the grandchild - still recognizes itself as the supervised process. - """ +def _is_launchd_supervised(environ: Mapping[str, str] | None = None) -> bool: + """True when this process is launchd's direct child (not an interactive shell).""" env = os.environ if environ is None else environ xpc_service = str(env.get("XPC_SERVICE_NAME", "")).strip() - if not xpc_service or xpc_service == "0": - return None - child_env = dict(env) - child_env[EXTERNAL_GATEWAY_SUPERVISOR_ENV] = "1" - return child_env + return bool(xpc_service and xpc_service != "0") + + +def _is_hermes_gateway_run_argv(command: Sequence[str]) -> bool: + """True for Hermes ``gateway run`` argv this wrapper is allowed to upgrade. + + The wrapper is generic. Only historical/current Hermes gateway shapes + get ``--external-supervisor``; an arbitrary launchd child must not be + marked as gateway-supervised (#87005). + """ + try: + from gateway.status import looks_like_gateway_command_line + except Exception: + return False + return bool(looks_like_gateway_command_line(" ".join(str(part) for part in command))) + + +def _with_external_supervisor_flag(command: Sequence[str]) -> list[str]: + argv = [str(part) for part in command] + if EXTERNAL_SUPERVISOR_FLAG not in argv: + argv.append(EXTERNAL_SUPERVISOR_FLAG) + return argv + + +def _prepare_child_command( + command: Sequence[str], + environ: Mapping[str, str] | None = None, +) -> list[str]: + """Return the argv to exec, upgrading stale launchd-wrapped gateway commands. + + launchd stamps ``XPC_SERVICE_NAME=`` only on this wrapper. + The grandchild sees ``XPC_SERVICE_NAME=0``. Newly generated plists put + ``--external-supervisor`` on the inner ``gateway run`` so ``hermes update`` + can see the flag on the live process argv. Stale plists still wrap the + historical ``gateway run --replace`` shape without that flag; append it + here, and only for that shape. + """ + argv = [str(part) for part in command] + if not _is_launchd_supervised(environ): + return argv + if not _is_hermes_gateway_run_argv(argv): + return argv + return _with_external_supervisor_flag(argv) def _parse_args(argv: Sequence[str] | None) -> argparse.Namespace: @@ -112,9 +139,8 @@ def main(argv: Sequence[str] | None = None) -> int: try: proc = subprocess.Popen( - args.command, + _prepare_child_command(args.command), stderr=subprocess.PIPE, - env=_child_env_for_command(), ) except OSError as exc: log_path.parent.mkdir(parents=True, exist_ok=True) diff --git a/tests/hermes_cli/test_gateway_external_supervisor.py b/tests/hermes_cli/test_gateway_external_supervisor.py index b409011498..e99ee433c0 100644 --- a/tests/hermes_cli/test_gateway_external_supervisor.py +++ b/tests/hermes_cli/test_gateway_external_supervisor.py @@ -65,3 +65,84 @@ def test_update_hands_external_supervisor_gateway_back_without_watcher(monkeypat ) +def test_update_hands_generated_launchd_inner_argv_back_without_watcher(monkeypatch): + """New launchd ProgramArguments put --external-supervisor on the grandchild.""" + monkeypatch.setattr( + gateway, + "_capture_gateway_argv", + lambda _pid: [ + "/usr/bin/python3", + "-m", + "hermes_cli.main", + "--profile", + "work", + "gateway", + "run", + "--replace", + "--external-supervisor", + ], + ) + monkeypatch.setattr( + gateway, + "launch_detached_profile_gateway_restart", + lambda *_args: pytest.fail("detached watcher must not be launched"), + ) + + assert gateway._prepare_profile_gateway_update_restart("work", 1234) == ( + "external-supervisor" + ) + + +def test_update_follows_wrapper_upgrade_of_stale_plist_argv(monkeypatch): + """stderr_timestamp upgrades stale inner argv so update sees the flag.""" + from hermes_cli.stderr_timestamp import _prepare_child_command + + stale = [ + "/usr/bin/python3", + "-m", + "hermes_cli.main", + "gateway", + "run", + "--replace", + ] + upgraded = _prepare_child_command( + stale, {"XPC_SERVICE_NAME": "ai.hermes.gateway-work"} + ) + assert upgraded[-1] == "--external-supervisor" + + monkeypatch.setattr(gateway, "_capture_gateway_argv", lambda _pid: upgraded) + monkeypatch.setattr( + gateway, + "launch_detached_profile_gateway_restart", + lambda *_args: pytest.fail("detached watcher must not be launched"), + ) + + assert gateway._prepare_profile_gateway_update_restart("work", 1234) == ( + "external-supervisor" + ) + + +def test_update_still_uses_detached_watcher_without_supervisor_flag(monkeypatch): + monkeypatch.setattr( + gateway, + "_capture_gateway_argv", + lambda _pid: [ + "python", + "-m", + "hermes_cli.main", + "gateway", + "run", + "--replace", + ], + ) + launched = [] + monkeypatch.setattr( + gateway, + "launch_detached_profile_gateway_restart", + lambda profile, pid: launched.append((profile, pid)) or True, + ) + + assert gateway._prepare_profile_gateway_update_restart("work", 1234) == "detached" + assert launched == [("work", 1234)] + + diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py index 5984d27485..91730237da 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -1249,6 +1249,7 @@ class TestProfileArg: "gateway", "run", "--replace", + "--external-supervisor", ] def test_launchd_plist_path_uses_real_user_home_not_profile_home(self, tmp_path, monkeypatch): diff --git a/tests/hermes_cli/test_stderr_timestamp.py b/tests/hermes_cli/test_stderr_timestamp.py index c73a871df9..0d5ecec7ed 100644 --- a/tests/hermes_cli/test_stderr_timestamp.py +++ b/tests/hermes_cli/test_stderr_timestamp.py @@ -3,9 +3,19 @@ import re import sys -from gateway.restart import EXTERNAL_GATEWAY_SUPERVISOR_ENV, is_gateway_supervisor_process +from gateway.restart import EXTERNAL_GATEWAY_SUPERVISOR_ENV from hermes_cli import stderr_timestamp +_STALE_GATEWAY_ARGV = [ + sys.executable, + "-m", + "hermes_cli.main", + "gateway", + "run", + "--replace", +] +_LAUNCHD_ENV = {"PATH": "/usr/bin", "XPC_SERVICE_NAME": "ai.hermes.gateway-butler"} + def test_main_timestamps_each_stderr_line(tmp_path): log_path = tmp_path / "gateway.error.log" @@ -37,34 +47,64 @@ def test_main_timestamps_each_stderr_line(tmp_path): assert lines[2] == "2026-07-15 12:34:56,789 already timestamped" -def test_child_env_forwards_supervisor_marker_under_launchd(): - """launchd's XPC label on the wrapper must survive into the grandchild.""" - child_env = stderr_timestamp._child_env_for_command( - { - "PATH": "/usr/bin", - "XPC_SERVICE_NAME": "ai.hermes.gateway-butler", - } +def test_prepare_upgrades_stale_gateway_argv_under_launchd(): + upgraded = stderr_timestamp._prepare_child_command( + _STALE_GATEWAY_ARGV, _LAUNCHD_ENV ) - assert child_env is not None - assert child_env["PATH"] == "/usr/bin" - assert child_env[EXTERNAL_GATEWAY_SUPERVISOR_ENV] == "1" - assert is_gateway_supervisor_process(child_env) is True + assert upgraded == [*_STALE_GATEWAY_ARGV, "--external-supervisor"] -def test_child_env_skips_interactive_xpc_zero(): - """Interactive macOS shells inherit XPC_SERVICE_NAME=0 — do not mark them.""" +def test_prepare_keeps_existing_external_supervisor_flag(): + already = [*_STALE_GATEWAY_ARGV, "--external-supervisor"] assert ( - stderr_timestamp._child_env_for_command( - {"PATH": "/usr/bin", "XPC_SERVICE_NAME": "0"} - ) - is None + stderr_timestamp._prepare_child_command(already, _LAUNCHD_ENV) == already ) - assert stderr_timestamp._child_env_for_command({"PATH": "/usr/bin"}) is None - assert is_gateway_supervisor_process({"XPC_SERVICE_NAME": "0"}) is False -def test_main_forwards_supervisor_marker_to_child(tmp_path, monkeypatch): - """The wrapper hop must set HERMES_GATEWAY_EXTERNAL_SUPERVISOR in the child.""" +def test_prepare_skips_arbitrary_command_under_launchd(): + """A generic wrapper must not mark random launchd children as the gateway.""" + other = [sys.executable, "-c", "print('ok')"] + assert stderr_timestamp._prepare_child_command(other, _LAUNCHD_ENV) == other + + +def test_prepare_skips_interactive_xpc_zero_even_for_gateway_argv(): + assert ( + stderr_timestamp._prepare_child_command( + _STALE_GATEWAY_ARGV, {"PATH": "/usr/bin", "XPC_SERVICE_NAME": "0"} + ) + == _STALE_GATEWAY_ARGV + ) + assert ( + stderr_timestamp._prepare_child_command(_STALE_GATEWAY_ARGV, {"PATH": "/usr/bin"}) + == _STALE_GATEWAY_ARGV + ) + + +def test_main_injects_flag_into_stale_gateway_child(tmp_path, monkeypatch): + """Stale plist inner argv must grow --external-supervisor in the grandchild.""" + monkeypatch.setenv("XPC_SERVICE_NAME", "ai.hermes.gateway-butler") + monkeypatch.delenv(EXTERNAL_GATEWAY_SUPERVISOR_ENV, raising=False) + log_path = tmp_path / "gateway.error.log" + marker_path = tmp_path / "argv.txt" + code = ( + "import sys\n" + f"from pathlib import Path\n" + f"Path({str(marker_path)!r}).write_text(" + "'\\n'.join(sys.argv[1:]), encoding='utf-8')\n" + ) + stale = [sys.executable, "-c", code, "-m", "hermes_cli.main", "gateway", "run", "--replace"] + + rc = stderr_timestamp.main( + ["--error-log", str(log_path), "--", *stale] + ) + + assert rc == 0 + recorded = marker_path.read_text(encoding="utf-8").splitlines() + assert recorded[-1] == "--external-supervisor" + assert "gateway" in recorded and "run" in recorded + + +def test_main_does_not_mark_arbitrary_launchd_child(tmp_path, monkeypatch): monkeypatch.setenv("XPC_SERVICE_NAME", "ai.hermes.gateway-butler") monkeypatch.delenv(EXTERNAL_GATEWAY_SUPERVISOR_ENV, raising=False) log_path = tmp_path / "gateway.error.log" @@ -73,7 +113,7 @@ def test_main_forwards_supervisor_marker_to_child(tmp_path, monkeypatch): "import os\n" f"from pathlib import Path\n" f"Path({str(marker_path)!r}).write_text(" - f"os.environ.get({EXTERNAL_GATEWAY_SUPERVISOR_ENV!r}, ''), encoding='utf-8')\n" + f"os.environ.get({EXTERNAL_GATEWAY_SUPERVISOR_ENV!r}, 'unset'), encoding='utf-8')\n" ) rc = stderr_timestamp.main( @@ -88,7 +128,7 @@ def test_main_forwards_supervisor_marker_to_child(tmp_path, monkeypatch): ) assert rc == 0 - assert marker_path.read_text(encoding="utf-8") == "1" + assert marker_path.read_text(encoding="utf-8") == "unset" def test_main_does_not_mark_unsupervised_child(tmp_path, monkeypatch):