mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-15 14:22:43 +00:00
refactor(gateway): funnel HERMES_HOME sync through a single chokepoint
Follow-up to HexLab98's fix. The sync-before-regenerate invariant was enforced by convention across 6 callsites (~3 idempotent unit-file reads per command). Consolidate it into the one function every compare/regenerate path funnels through — systemd_unit_is_current — and drop the now-redundant callsite pre-syncs in refresh_systemd_unit_if_needed / systemd_start / systemd_restart / systemd_status. Kept the systemd_install pre-sync: the --force path bypasses the is_current gate and calls generate_systemd_unit() directly, so it needs its own sync to avoid baking /root/.hermes under sudo. Reworked the two callsite-ordering tests into a chokepoint-invariant guard (test_is_current_syncs_before_reading_unit) + a delegation test proving start/restart no longer pre-sync. Both fail if the chokepoint sync is removed; the pre-existing behavior test still passes.
This commit is contained in:
kshitijk4poor
kshitij
parent
8f18f6c695
commit
d54a8f7079
2 changed files with 112 additions and 78 deletions
|
|
@ -2855,17 +2855,28 @@ def _normalize_launchd_plist_for_comparison(text: str) -> str:
|
|||
|
||||
|
||||
def systemd_unit_is_current(system: bool = False) -> bool:
|
||||
# ── HERMES_HOME sync chokepoint ──────────────────────────────────────
|
||||
# Every path that compares OR regenerates the unit funnels through here:
|
||||
# ``refresh_systemd_unit_if_needed`` gates on this before rewriting, and
|
||||
# ``systemd_status`` / ``systemd_install`` call it directly. Doing the
|
||||
# sync here — and ONLY here — enforces the invariant "the operator's
|
||||
# pinned HERMES_HOME is adopted before any compare/regenerate" at a single
|
||||
# site, so a future callsite cannot regress it by forgetting to pre-sync.
|
||||
#
|
||||
# Under ``sudo hermes gateway … --system``, HERMES_HOME is often stripped
|
||||
# and falls back to ``/root/.hermes``. Adopting the unit's pinned home
|
||||
# first makes TimeoutStopSec / WorkingDirectory / HERMES_HOME comparisons
|
||||
# use the real operator config — otherwise start/restart "refresh" rewrites
|
||||
# a correct unit from root's defaults and ``status`` keeps warning forever.
|
||||
# ``_sync_...`` is idempotent (early-returns once os.environ matches), so
|
||||
# the mutation persists for callers that read runtime state after this
|
||||
# (e.g. ``systemd_restart``'s post-refresh get_running_pid / drain-timeout).
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
|
||||
unit_path = get_systemd_unit_path(system=system)
|
||||
if not unit_path.exists():
|
||||
return False
|
||||
|
||||
# Under ``sudo hermes gateway … --system``, HERMES_HOME is often stripped
|
||||
# and falls back to ``/root/.hermes``. Adopt the unit's pinned home first
|
||||
# so TimeoutStopSec / WorkingDirectory / HERMES_HOME comparisons use the
|
||||
# real operator config — otherwise start/restart "refresh" rewrites a
|
||||
# correct unit from root's defaults and ``status`` keeps warning forever.
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
|
||||
installed = unit_path.read_text(encoding="utf-8")
|
||||
expected_user = _read_systemd_user_from_unit(unit_path) if system else None
|
||||
expected = generate_systemd_unit(system=system, run_as_user=expected_user)
|
||||
|
|
@ -2946,11 +2957,10 @@ def refresh_systemd_unit_if_needed(system: bool = False) -> bool:
|
|||
if not unit_path.exists():
|
||||
return False
|
||||
|
||||
# Sync before the current-check / regenerate path so a ``sudo`` shell
|
||||
# does not bake root's config into a system unit that already pins the
|
||||
# operator's HERMES_HOME. ``systemd_unit_is_current`` also syncs; doing
|
||||
# it here keeps the write path safe even if that helper changes.
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
# The gate below funnels through ``systemd_unit_is_current``, which is the
|
||||
# single HERMES_HOME-sync chokepoint (adopts the unit's pinned home before
|
||||
# any compare/regenerate). No separate pre-sync needed here — and the env
|
||||
# mutation it performs persists for the regenerate path below.
|
||||
if systemd_unit_is_current(system=system):
|
||||
return False
|
||||
|
||||
|
|
@ -3139,8 +3149,11 @@ def systemd_install(
|
|||
scope_flag = " --system" if system else ""
|
||||
|
||||
# Existing system units already pin HERMES_HOME; adopt it before any
|
||||
# current-check / regenerate so ``sudo hermes gateway install --system``
|
||||
# cannot "repair" a correct unit from root's config.
|
||||
# regenerate. This pre-sync is NOT redundant with the systemd_unit_is_current
|
||||
# chokepoint: the ``--force`` path below skips the is_current gate and calls
|
||||
# generate_systemd_unit() directly (line ~3172), so without this a
|
||||
# ``sudo hermes gateway install --system --force`` would bake /root/.hermes
|
||||
# into an already-correct unit. Keep it to protect that bypass path.
|
||||
if unit_path.exists():
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
|
||||
|
|
@ -3234,9 +3247,9 @@ def systemd_start(system: bool = False):
|
|||
# Raises UserSystemdUnavailableError with a remediation message.
|
||||
_preflight_user_systemd()
|
||||
_require_service_installed("start", system=system)
|
||||
# Adopt the unit's HERMES_HOME before refresh so sudo system starts do not
|
||||
# rewrite TimeoutStopSec / env from /root/.hermes.
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
# HERMES_HOME sync happens inside refresh_systemd_unit_if_needed's
|
||||
# systemd_unit_is_current gate (the single chokepoint), and the unit is
|
||||
# guaranteed to exist here by _require_service_installed, so the gate runs.
|
||||
refresh_systemd_unit_if_needed(system=system)
|
||||
_run_systemctl(["start", get_service_name()], system=system, check=True, timeout=30)
|
||||
print(f"✓ {_service_scope_label(system).capitalize()} service started")
|
||||
|
|
@ -3277,10 +3290,11 @@ def systemd_restart(system: bool = False):
|
|||
else:
|
||||
_preflight_user_systemd()
|
||||
_require_service_installed("restart", system=system)
|
||||
# Sync BEFORE refresh. Under sudo, refreshing first used to regenerate the
|
||||
# unit from /root/.hermes (wrong drain timeout / env), then sync for PID
|
||||
# lookup — leaving ``status`` stuck on "service definition is outdated".
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
# HERMES_HOME sync happens inside refresh_systemd_unit_if_needed's
|
||||
# systemd_unit_is_current gate (the single chokepoint). The unit exists
|
||||
# here (_require_service_installed), so the gate runs and its os.environ
|
||||
# mutation persists for the get_running_pid / drain-timeout reads below —
|
||||
# no separate pre-sync needed.
|
||||
refresh_systemd_unit_if_needed(system=system)
|
||||
from gateway.status import get_running_pid
|
||||
|
||||
|
|
@ -3382,8 +3396,6 @@ def systemd_status(deep: bool = False, system: bool = False, full: bool = False)
|
|||
print(f" Run: {'sudo ' if system else ''}hermes gateway install{scope_flag}")
|
||||
return
|
||||
|
||||
_sync_hermes_home_from_systemd_unit(system=system)
|
||||
|
||||
if has_conflicting_systemd_units():
|
||||
print_systemd_scope_conflict_warning()
|
||||
print()
|
||||
|
|
|
|||
|
|
@ -2074,68 +2074,90 @@ class TestSystemUnitRefreshSyncsHermesHome:
|
|||
assert os.environ["HERMES_HOME"] == str(alice_hermes)
|
||||
assert gateway_cli.systemd_unit_is_current(system=True) is True
|
||||
|
||||
def test_systemd_restart_syncs_before_refresh(self, monkeypatch):
|
||||
calls = []
|
||||
def test_is_current_syncs_before_reading_unit(self, tmp_path, monkeypatch):
|
||||
"""CHOKEPOINT INVARIANT: systemd_unit_is_current() must adopt the
|
||||
unit's pinned HERMES_HOME *before* it reads/compares the unit.
|
||||
|
||||
monkeypatch.setattr(gateway_cli, "_select_systemd_scope", lambda system=False: True)
|
||||
monkeypatch.setattr(gateway_cli, "_require_root_for_system_service", lambda action: None)
|
||||
monkeypatch.setattr(gateway_cli, "_require_service_installed", lambda action, system=False: None)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_sync_hermes_home_from_systemd_unit",
|
||||
lambda system: calls.append(("sync", system)),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"refresh_systemd_unit_if_needed",
|
||||
lambda system=False: calls.append(("refresh", system)),
|
||||
)
|
||||
monkeypatch.setattr("gateway.status.get_running_pid", lambda: None)
|
||||
monkeypatch.setattr(gateway_cli, "_systemd_main_pid", lambda system=False: None)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_run_systemctl",
|
||||
lambda args, **kwargs: calls.append(("systemctl", args))
|
||||
or SimpleNamespace(returncode=0, stdout="", stderr=""),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_wait_for_systemd_service_restart",
|
||||
lambda system=False, previous_pid=None: True,
|
||||
)
|
||||
This is the single site that enforces sync-before-compare for every
|
||||
path (refresh gates on it; status/install call it). If a future edit
|
||||
moves the sync after the read (or drops it), this test fails.
|
||||
"""
|
||||
order = []
|
||||
unit_path = tmp_path / "hermes-gateway.service"
|
||||
unit_path.write_text("[Unit]\n", encoding="utf-8")
|
||||
|
||||
gateway_cli.systemd_restart(system=True)
|
||||
monkeypatch.setattr(gateway_cli, "get_systemd_unit_path", lambda system=False: unit_path)
|
||||
|
||||
assert calls[0] == ("sync", True)
|
||||
assert calls[1] == ("refresh", True)
|
||||
real_read_text = Path.read_text
|
||||
|
||||
def test_systemd_start_syncs_before_refresh(self, monkeypatch):
|
||||
calls = []
|
||||
def tracking_sync(system):
|
||||
order.append("sync")
|
||||
|
||||
monkeypatch.setattr(gateway_cli, "_select_systemd_scope", lambda system=False: True)
|
||||
monkeypatch.setattr(gateway_cli, "_require_root_for_system_service", lambda action: None)
|
||||
monkeypatch.setattr(gateway_cli, "_require_service_installed", lambda action, system=False: None)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_sync_hermes_home_from_systemd_unit",
|
||||
lambda system: calls.append(("sync", system)),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"refresh_systemd_unit_if_needed",
|
||||
lambda system=False: calls.append(("refresh", system)),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_run_systemctl",
|
||||
lambda args, **kwargs: calls.append(("systemctl", args))
|
||||
or SimpleNamespace(returncode=0, stdout="", stderr=""),
|
||||
)
|
||||
def tracking_read_text(self, *a, **k):
|
||||
if self == unit_path:
|
||||
order.append("read")
|
||||
return real_read_text(self, *a, **k)
|
||||
|
||||
gateway_cli.systemd_start(system=True)
|
||||
monkeypatch.setattr(gateway_cli, "_sync_hermes_home_from_systemd_unit", tracking_sync)
|
||||
monkeypatch.setattr(Path, "read_text", tracking_read_text)
|
||||
# Avoid a real generate/compare — we only assert sync precedes read.
|
||||
monkeypatch.setattr(gateway_cli, "generate_systemd_unit", lambda **k: "[Unit]\n")
|
||||
monkeypatch.setattr(gateway_cli, "_read_systemd_user_from_unit", lambda p: None)
|
||||
|
||||
assert calls[0] == ("sync", True)
|
||||
assert calls[1] == ("refresh", True)
|
||||
gateway_cli.systemd_unit_is_current(system=True)
|
||||
|
||||
assert order, "systemd_unit_is_current did not run sync or read"
|
||||
assert order[0] == "sync", f"sync must precede unit read; got {order}"
|
||||
assert "read" in order and order.index("sync") < order.index("read")
|
||||
|
||||
def test_start_and_restart_delegate_sync_to_chokepoint(self, monkeypatch):
|
||||
"""start/restart must NOT pre-sync at the callsite — the sync is owned
|
||||
by the systemd_unit_is_current chokepoint that refresh gates on. This
|
||||
pins the single-chokepoint design so a future edit can't reintroduce a
|
||||
redundant (or, worse, out-of-order) callsite sync.
|
||||
"""
|
||||
for entry in ("systemd_start", "systemd_restart"):
|
||||
calls = []
|
||||
monkeypatch.setattr(gateway_cli, "_select_systemd_scope", lambda system=False: True)
|
||||
monkeypatch.setattr(gateway_cli, "_require_root_for_system_service", lambda action: None)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli, "_require_service_installed", lambda action, system=False: None
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_sync_hermes_home_from_systemd_unit",
|
||||
lambda system: calls.append("sync"),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"refresh_systemd_unit_if_needed",
|
||||
lambda system=False: calls.append("refresh"),
|
||||
)
|
||||
monkeypatch.setattr("gateway.status.get_running_pid", lambda: None)
|
||||
monkeypatch.setattr(gateway_cli, "_systemd_main_pid", lambda system=False: None)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_run_systemctl",
|
||||
lambda args, **kwargs: calls.append("systemctl")
|
||||
or SimpleNamespace(returncode=0, stdout="", stderr=""),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
gateway_cli,
|
||||
"_wait_for_systemd_service_restart",
|
||||
lambda system=False, previous_pid=None: True,
|
||||
)
|
||||
|
||||
getattr(gateway_cli, entry)(system=True)
|
||||
|
||||
# refresh runs; the callsite adds NO separate sync before it (the
|
||||
# chokepoint inside refresh->is_current owns the sync). Here refresh
|
||||
# is mocked out, so no "sync" should appear at all for the refresh
|
||||
# phase — proving the callsite pre-sync was removed.
|
||||
assert "refresh" in calls, f"{entry} must call refresh_systemd_unit_if_needed"
|
||||
assert calls.count("sync") == 0, (
|
||||
f"{entry} should delegate sync to the chokepoint, not pre-sync "
|
||||
f"at the callsite; got {calls}"
|
||||
)
|
||||
|
||||
|
||||
class TestHermesHomeForTargetUser:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue