From a4092ab217c19032c10ffa3b8d60347eabde394d Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 24 May 2026 18:07:47 -0700 Subject: [PATCH] fix(profiles): short-circuit s6 hooks on host before importing service_manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to @benbarclay's Docker s6 PR (#30136). The Phase 4 hooks `_maybe_register_gateway_service` and `_maybe_unregister_gateway_service` were already documented as "no-op on host", but they reached that no-op by: 1. importing `hermes_cli.service_manager` 2. calling `get_service_manager()` (which calls `detect_service_manager()`) 3. checking `mgr.supports_runtime_registration()` and returning False If anything in step 1 or 2 raised an unexpected exception (e.g. a host machine with a partial s6 install — `/proc/1/comm == s6-svscan` somehow, but `/run/s6/basedir` absent, or vice versa), the `except Exception` in the hook would print a confusing "⚠ Could not register s6 gateway service: ..." warning on a non-container machine that has never touched the container. Reorder so `detect_service_manager() != "s6"` is checked FIRST, and return silently for any detection failure. Host machines now: - never import the s6 backend - never call get_service_manager() - never print an s6-shaped warning under any failure mode E2E confirmed on host Linux (systemd): `_maybe_register_gateway_service(...)` produces empty stdout, detect_service_manager() returns "systemd". Existing tests updated to patch `detect_service_manager` for the s6 call-through cases (they previously relied on get_service_manager being the only gate, which is no longer true). Added one new test — `test_register_silent_when_detect_throws` — asserting that a broken detector cannot leak a warning to host users. cc @benbarclay — visible behavior change vs. your branch is one fewer code path on host. Test changes are minimal (one helper + `_patch_detect_s6` opt-in per s6 test). Happy to revert if you prefer the original shape. --- hermes_cli/profiles.py | 26 +++++++++++ tests/hermes_cli/test_profiles_s6_hooks.py | 54 ++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index e6979320afd..c4cb373bddc 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -994,12 +994,30 @@ def _maybe_register_gateway_service(profile_name: str) -> None: (``[gateway] port = …``) — there is no Python-side allocator (PR #30136 review item I5 retired the SHA-256-derived range [9200, 9800) because it was dead code through the entire stack). + + Host short-circuit: check ``detect_service_manager()`` first and + return immediately if it isn't ``"s6"``. This keeps host + (systemd/launchd/windows) profile creation completely silent — + no ``get_service_manager()`` call, no exception path, no chance + of the ``⚠ Could not register s6 gateway service`` warning ever + rendering on a non-container machine. The earlier + ``supports_runtime_registration()`` check still catches the case + where detection somehow returns ``"s6"`` but the backend isn't + actually the S6 one. """ try: + from hermes_cli.service_manager import detect_service_manager + if detect_service_manager() != "s6": + return # host path — silent, no registration needed from hermes_cli.service_manager import get_service_manager mgr = get_service_manager() except RuntimeError: return # no backend on this host — nothing to do + except Exception: + # Defensive: detect_service_manager failed for some other + # reason. Stay silent on host rather than printing a confusing + # s6 warning to users who have never touched the container. + return if not mgr.supports_runtime_registration(): return # host backend; no-op try: @@ -1018,12 +1036,20 @@ def _maybe_unregister_gateway_service(profile_name: str) -> None: No-op on host. Idempotent: absent services are silently skipped by ``unregister_profile_gateway``. + + Same host short-circuit as :func:`_maybe_register_gateway_service` + — see that docstring. """ try: + from hermes_cli.service_manager import detect_service_manager + if detect_service_manager() != "s6": + return # host path — silent from hermes_cli.service_manager import get_service_manager mgr = get_service_manager() except RuntimeError: return + except Exception: + return if not mgr.supports_runtime_registration(): return try: diff --git a/tests/hermes_cli/test_profiles_s6_hooks.py b/tests/hermes_cli/test_profiles_s6_hooks.py index c0ce1d0b189..db50debdcba 100644 --- a/tests/hermes_cli/test_profiles_s6_hooks.py +++ b/tests/hermes_cli/test_profiles_s6_hooks.py @@ -65,7 +65,30 @@ class _S6Manager: self.unregistered.append(profile) +def _patch_detect_s6(monkeypatch: pytest.MonkeyPatch) -> None: + """Pretend we're inside an s6 container so the host short-circuit + in :func:`_maybe_register_gateway_service` / + :func:`_maybe_unregister_gateway_service` doesn't fire. + + Without this, ``detect_service_manager()`` runs its real + implementation (host Linux/macOS in CI), returns ``"systemd"`` or + ``"launchd"``, and the hooks return early before reaching the + patched ``get_service_manager``. Each s6-call-through test + explicitly opts into this so the host-no-op tests can still + exercise the early-return path. + """ + monkeypatch.setattr( + "hermes_cli.service_manager.detect_service_manager", + lambda: "s6", + ) + + def test_register_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None: + # NOTE: deliberately DO NOT patch detect_service_manager — we want + # the real host detection to kick in and short-circuit before + # get_service_manager is ever called. The lambda below is a + # defense-in-depth assertion that get_service_manager is never + # reached on host. monkeypatch.setattr( "hermes_cli.service_manager.get_service_manager", lambda: _HostManager(), @@ -75,6 +98,7 @@ def test_register_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None: def test_register_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None: + _patch_detect_s6(monkeypatch) mgr = _S6Manager() monkeypatch.setattr( "hermes_cli.service_manager.get_service_manager", lambda: mgr, @@ -88,6 +112,7 @@ def test_register_swallows_duplicate_value_error( ) -> None: """A pre-existing s6 registration (from container-boot reconcile) is a benign condition — register must not propagate ValueError.""" + _patch_detect_s6(monkeypatch) mgr = _S6Manager() mgr.raise_on_register = ValueError("already registered") monkeypatch.setattr( @@ -102,6 +127,7 @@ def test_register_swallows_arbitrary_error( ) -> None: """Even an unexpected exception from the manager must not bring down `hermes profile create` — print and continue.""" + _patch_detect_s6(monkeypatch) mgr = _S6Manager() mgr.raise_on_register = RuntimeError("svscanctl exploded") monkeypatch.setattr( @@ -117,6 +143,7 @@ def test_register_swallows_no_backend_runtime_error( ) -> None: """When `get_service_manager()` raises RuntimeError (no backend detected), the hook must silently no-op.""" + _patch_detect_s6(monkeypatch) def _no_backend() -> None: raise RuntimeError("no supported service manager detected") monkeypatch.setattr( @@ -126,7 +153,32 @@ def test_register_swallows_no_backend_runtime_error( _maybe_register_gateway_service("anywhere") +def test_register_silent_when_detect_throws( + monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], +) -> None: + """If detect_service_manager itself raises (e.g. a partial s6 + install on a host machine), the hook must stay silent — no + confusing s6 warning printed to a user who has never touched a + container.""" + def _broken_detect() -> str: + raise RuntimeError("detection blew up") + monkeypatch.setattr( + "hermes_cli.service_manager.detect_service_manager", _broken_detect, + ) + # If get_service_manager is reached, the test will assert via + # _HostManager.register. It must NOT be reached. + monkeypatch.setattr( + "hermes_cli.service_manager.get_service_manager", + lambda: _HostManager(), + ) + _maybe_register_gateway_service("anywhere") + captured = capsys.readouterr() + assert "Could not register" not in captured.out + assert captured.out == "" + + def test_unregister_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None: + # Same as test_register_noop_on_host: rely on real host detection. monkeypatch.setattr( "hermes_cli.service_manager.get_service_manager", lambda: _HostManager(), @@ -135,6 +187,7 @@ def test_unregister_noop_on_host(monkeypatch: pytest.MonkeyPatch) -> None: def test_unregister_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None: + _patch_detect_s6(monkeypatch) mgr = _S6Manager() monkeypatch.setattr( "hermes_cli.service_manager.get_service_manager", lambda: mgr, @@ -146,6 +199,7 @@ def test_unregister_calls_through_on_s6(monkeypatch: pytest.MonkeyPatch) -> None def test_unregister_swallows_errors( monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str], ) -> None: + _patch_detect_s6(monkeypatch) mgr = _S6Manager() mgr.raise_on_unregister = RuntimeError("svc gone weird") monkeypatch.setattr(