ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

fix(gateway): prove --replace ownership from the bound pid record

Browse Source 
Review v2 of #93084: the readable-cmdline path used substring matching
as destructive authority. That fails open on prefix collisions
(--profile timothy vs our profile tim) and same-name profiles under
different roots, so a poisoned record could still reach SIGTERM.

Ownership is now decided by the persisted identity record ALONE — exact
_same_hermes_home equality, bound to the live target by exact pid +
start-time. Missing/legacy/unbound/foreign records all refuse. A
readable argv feeds only a token-exact consistency check
(_looks_like_profile_conflict_from_cmdline via shlex tokens) that
refuses explicit contradictions like --profile timothy under tim; bare
or matching argv adds nothing.

Also fixes the review's source-of-record concern: the guard validates
the record that authorizes get_running_pid()'s answer rather than
assuming {HERMES_HOME}/gateway.pid is always the source.

Adds the requested signal-boundary regression: start_gateway(replace=
True) with unprovable ownership returns False without calling
terminate_pid or writing a takeover marker; the bound same-home
counterpart still reaches the replace flow. Legacy replace-flow tests
updated to stage a valid bound record for their legitimate-replace
fixtures.
This commit is contained in:
BrunoBza
2026-08-23 20:40:20 +02:00
committed by kshitij
parent dc50f02090
commit 2a27e1ffb3
4 changed files with 640 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
gateway/run.py
+200 -1
View File
@@ -30475,6 +30475,190 @@ def _gateway_stderr_formatter() -> logging.Formatter:
return RedactingFormatter("%(asctime)s %(levelname)s %(name)s: %(message)s")
# ownership guard inserted below (PR #93084)
def _replace_target_belongs_to_other_profile(existing_pid: int) -> bool:
"""Return True when ``--replace`` must refuse to signal ``existing_pid``.
The PID file is HERMES_HOME-scoped, but a poisoned/stale record can point
at another profile's LIVE gateway; signaling it starts the cross-profile
SIGTERM restart loop this guard exists to prevent (#89315). This is a
destructive-action authority check, so ownership is decided by the
persisted identity record ALONE — exact ``_same_hermes_home`` equality —
and only while that record stays bound to the live target by exact PID +
start-time identity:
* The authorizing record is whichever source produced the PID for this
destructive decision (PID file, gateway lock record, or runtime-status
fallback). A readable live argv carries no HERMES_HOME (it travels in
the environment), so it can never prove home ownership; it is used only
as an additional CONSISTENCY check — token-exact profile flags that
clearly contradict our home refuse the signal even when the record
agrees.
* Missing, legacy, conflicting, stale-bound, or unprovable identity →
refuse (fail closed).
Same-home targets keep replacing normally; every refusal path here only
narrows what the legacy start_time check alone used to allow.
"""
try:
from gateway.status import (
_get_pid_path,
_get_process_hermes_home,
_get_process_start_time,
_pid_from_record,
_read_pid_record,
_record_looks_like_gateway,
_read_process_cmdline,
_same_hermes_home,
)
our_home = _get_process_hermes_home()
# ── Authorize from the persisted identity record ──────────────
# Bound claim: the record must describe THIS pid with THIS live
# start time, otherwise it is stale/poisoned and proves nothing.
record = _read_pid_record(_get_pid_path())
if not isinstance(record, dict) or not _record_looks_like_gateway(record):
logger.warning(
"Refusing --replace: no valid gateway pid record to prove "
"ownership of PID %s.",
existing_pid,
)
return True
record_pid = _pid_from_record(record)
if record_pid != existing_pid:
logger.warning(
"Refusing --replace: pid record names %s, not target %s.",
record_pid, existing_pid,
)
return True
recorded_start = record.get("start_time")
if not isinstance(recorded_start, int) or isinstance(recorded_start, bool):
return True
if _get_process_start_time(existing_pid) != recorded_start:
logger.warning(
"Refusing --replace: pid record start-time does not match "
"the live process %s (stale/PID-reuse record).",
existing_pid,
)
return True
recorded_home = record.get("hermes_home")
if not isinstance(recorded_home, str) or not recorded_home.strip():
# Legacy record without hermes_home cannot prove ownership.
logger.warning(
"Refusing --replace: pid record predates hermes_home "
"stampings; ownership of PID %s unprovable.",
existing_pid,
)
return True
if not _same_hermes_home(recorded_home, our_home):
logger.error(
"Refusing --replace: pid record belongs to a different "
"HERMES_HOME (%s, ours %s). Remove the stale PID record or "
"stop the owning profile explicitly.",
recorded_home,
our_home,
)
return True
# ── Readable-argv consistency check (never authority) ─────────
# An explicit profile flag / HERMES_HOME= on the argv that clearly
# contradicts our home refuses even though the record agreed; a bare
# or matching argv adds nothing either way.
try:
live_cmdline = _read_process_cmdline(existing_pid)
except Exception:
live_cmdline = None # consistency probe failure → record decides
if live_cmdline and _looks_like_profile_conflict_from_cmdline(
live_cmdline, our_home
):
logger.error(
"Refusing --replace: target PID %s command line explicitly "
"advertises a different profile than HERMES_HOME %s.",
existing_pid,
our_home,
)
return True
return False
except Exception:
# Destructive action + unknown ownership => fail closed (#89315).
logger.warning(
"cross-profile --replace ownership probe failed for PID %s; "
"refusing to signal",
existing_pid,
exc_info=True,
)
return True
def _looks_like_profile_conflict_from_cmdline(command: str, our_home) -> bool:
"""Token-exact contradiction check between a target argv and our home.
Authority lives in the pid record; this only catches argv that EXPLICITLY
advertises a different profile than ours. Substring matching is not
identity: ``--profile timothy`` must NOT read as profile ``tim``. Returns
False whenever the argv does not clearly contradict our home.
"""
from gateway.status import _profile_name_for_home
profile_name = _profile_name_for_home(our_home)
try:
tokens = shlex.split(command)
except ValueError:
tokens = command.split()
def _flag_value(flag: str) -> Optional[str]:
"""Value of ``--flag X`` / ``--flag=X`` occurrences, token-exact."""
values = []
i = 0
while i < len(tokens):
tok = tokens[i]
if tok == flag and i + 1 < len(tokens):
values.append(tokens[i + 1])
i += 2
continue
if tok.startswith(flag + "="):
values.append(tok[len(flag) + 1:])
i += 1
return values[-1] if values else None
def _env_home_value() -> Optional[str]:
"""HERMES_HOME=<path> env-style assignment on the argv, token-exact."""
prefix = "HERMES_HOME="
for tok in reversed(tokens):
if tok.startswith(prefix):
return tok[len(prefix):]
return None
if profile_name is not None and profile_name != "default":
# Our home is a named profile: any explicit DIFFERENT named profile
# on the argv contradicts it. Bare argv stays consistent (legacy
# default-gateway argv never carried profile flags).
for flag in ("--profile", "-p"):
value = _flag_value(flag)
if value is not None and value != profile_name:
return True
home_value = _flag_value("--hermes-home") or _env_home_value()
if home_value is not None and os.path.normcase(os.path.normpath(home_value)) != os.path.normcase(os.path.normpath(str(our_home))):
return True
return False
# Our home is the default/root: ANY explicit named-profile flag on the
# argv contradicts it.
if _flag_value("--profile") is not None or _flag_value("-p") is not None:
return True
home_value = _flag_value("--hermes-home") or _env_home_value()
if home_value is not None and os.path.normcase(os.path.normpath(home_value)) != os.path.normcase(os.path.normpath(str(our_home))):
return True
return False
async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = False, verbosity: Optional[int] = 0) -> bool:
"""
Start the gateway and run until interrupted.
@@ -30521,6 +30705,21 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool =
existing_pid = get_running_pid()
if existing_pid is not None and existing_pid != os.getpid():
if replace:
# Cross-profile ownership gate (#89315): never signal a live
# process we cannot prove belongs to this HERMES_HOME. A poisoned
# PID record steering --replace at another profile's gateway is
# exactly the restart-loop shape this flow must not allow.
if _replace_target_belongs_to_other_profile(existing_pid):
from gateway.status import _get_process_hermes_home
logger.error(
"Refusing --replace: PID %d cannot be proven to belong "
"to this profile's gateway (HERMES_HOME %s). Remove the "
"stale PID record or stop the owning profile explicitly.",
existing_pid,
_get_process_hermes_home(),
)
return False
existing_start_time = get_process_start_time(existing_pid)
logger.info(
"Replacing existing gateway instance (PID %d) with --replace.",
@@ -31355,4 +31554,4 @@ def _exit_after_graceful_shutdown(exit_code: int) -> None:
if __name__ == "__main__":
main()
main()
tests/gateway/test_replace_child_reap.py
+15
View File
@@ -199,6 +199,21 @@ async def test_start_gateway_replace_reaps_old_gateway_children_posix(
"gateway.status.remove_pid_file",
lambda: _pid_state.update(alive=False),
)
# Ownership guard (#89315): legitimate same-home replace fixture —
# bound record for target pid 42 in this home.
monkeypatch.setattr(
"gateway.status._read_pid_record",
lambda path=None: {
"pid": 42,
"kind": "hermes-gateway",
"argv": ["python", "-m", "hermes_cli.main", "gateway", "run"],
"start_time": 0,
"hermes_home": str(tmp_path),
},
)
monkeypatch.setattr(
"gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None
)
monkeypatch.setattr(
"gateway.status.release_all_scoped_locks", lambda **kwargs: 0
)
tests/gateway/test_runner_startup_failures.py
+32
View File
@@ -138,6 +138,21 @@ async def test_start_gateway_replace_aborts_when_force_killed_pid_still_alive(
"gateway.status.terminate_pid",
lambda pid, force=False: calls.append((pid, force)),
)
# Ownership guard (#89315): legitimate same-home replace fixture — the
# persisted record is bound to target pid 42 in this home.
monkeypatch.setattr(
"gateway.status._read_pid_record",
lambda path=None: {
"pid": 42,
"kind": "hermes-gateway",
"argv": ["python", "-m", "hermes_cli.main", "gateway", "run"],
"start_time": 0,
"hermes_home": str(tmp_path),
},
)
monkeypatch.setattr(
"gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None
)
# _pid_exists never goes False — the force-kill did not take.
monkeypatch.setattr("gateway.status._pid_exists", lambda pid: True)
monkeypatch.setattr("gateway.run.os.getpid", lambda: 100)
@@ -215,6 +230,23 @@ async def test_start_gateway_replace_writes_takeover_marker_before_sigterm(
_pid_state["alive"] = False
monkeypatch.setattr("gateway.status.get_running_pid", _mock_get_running_pid)
monkeypatch.setattr("gateway.status.remove_pid_file", _mock_remove_pid_file)
# Ownership guard (#89315): this test simulates a legitimate same-home
# replace, so the persisted pid record must be a valid BOUND record for
# the target pid in THIS home. start_time 0 matches the legacy fixture's
# convention; the live probe is patched to agree.
monkeypatch.setattr(
"gateway.status._read_pid_record",
lambda path=None: {
"pid": 42,
"kind": "hermes-gateway",
"argv": ["python", "-m", "hermes_cli.main", "gateway", "run"],
"start_time": 0,
"hermes_home": str(tmp_path),
},
)
monkeypatch.setattr(
"gateway.status._get_process_start_time", lambda pid: 0 if pid == 42 else None
)
monkeypatch.setattr(
"gateway.status.release_all_scoped_locks",
lambda **kwargs: 0,
tests/test_89315_replace_ownership_guard.py
+393
View File
@@ -0,0 +1,393 @@
"""Tests for issue #89315 — ``--replace`` must never signal a gateway it
cannot prove belongs to this HERMES_HOME.
Design contract (v3, after andrexibiza's second review): ownership is decided
by the persisted identity record ALONE — exact ``_same_hermes_home`` equality
bound to the live target by exact PID + start-time. A readable live argv
carries no HERMES_HOME, so it can never prove home ownership; it only feeds a
token-exact CONSISTENCY check that refuses explicit contradictions.
Pinned surfaces:
* record authority — valid+bound same-home allows; missing/legacy/unbound/
foreign records refuse;
* argv consistency — token-exact: ``--profile timothy`` must NOT read as
``tim`` (the substring heuristic's false-allow), while an exact different
profile flag contradicts and refuses;
* signal boundary — ``start_gateway(replace=True)`` on unprovable ownership
returns refusal without calling ``terminate_pid`` or writing a takeover
marker; the legitimate bound same-home target still reaches the replace
flow.
"""
from __future__ import annotations
import json
from pathlib import Path
from unittest.mock import patch
import pytest
@pytest.fixture()
def profile_env(tmp_path, monkeypatch):
"""Isolated HERMES_HOME mirroring tests/hermes_cli/test_profiles.py."""
monkeypatch.setattr(Path, "home", lambda: tmp_path)
default_home = tmp_path / ".hermes"
default_home.mkdir(exist_ok=True)
monkeypatch.setenv("HERMES_HOME", str(default_home))
return tmp_home if (tmp_home := default_home) else default_home
def _record(pid=424242, start=111222333, home=None, argv=None):
return {
"pid": pid,
"kind": "hermes-gateway",
"argv": argv or ["python", "-m", "hermes_cli.main", "gateway", "run"],
"start_time": start,
"hermes_home": home,
}
class TestRecordAuthority:
def test_valid_bound_same_home_record_allows(self, profile_env):
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch(
"gateway.status._read_pid_record",
return_value=_record(home=str(profile_env / ".hermes")),
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=111222333,
),
patch("gateway.status._read_process_cmdline", return_value=None),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is False
def test_foreign_home_record_refuses(self, profile_env):
"""Exact-home equality: another root/profile in the record refuses,
even with a bare argv that substring matching would have passed."""
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch(
"gateway.status._read_pid_record",
return_value=_record(
home="/home/other/.hermes/profiles/timothy"
),
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=111222333,
),
patch("gateway.status._read_process_cmdline", return_value=None),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes" / "profiles" / "tim",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
def test_missing_record_refuses(self, profile_env):
"""No valid record → ownership unprovable → refuse."""
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch("gateway.status._read_pid_record", return_value=None),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
def test_legacy_record_without_home_refuses(self, profile_env):
"""A pre-hermes_home-stamping record cannot prove ownership."""
from gateway.run import _replace_target_belongs_to_other_profile
legacy = _record(home=None)
legacy.pop("hermes_home")
with (
patch(
"gateway.status._read_pid_record",
return_value=legacy,
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=111222333,
),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
def test_unbound_record_wrong_pid_refuses(self, profile_env):
"""A record naming a DIFFERENT pid proves nothing (poisoned shape)."""
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch(
"gateway.status._read_pid_record",
return_value=_record(pid=999999, home=str(profile_env / ".hermes")),
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=111222333,
),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
def test_unbound_record_stale_start_time_refuses(self, profile_env):
"""PID reused since the record was written (start_time drift) → the
record no longer describes the live process → refuse."""
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch(
"gateway.status._read_pid_record",
return_value=_record(start=1, home=str(profile_env / ".hermes")),
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=42,
),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
def test_probe_exception_fails_closed(self, profile_env):
from gateway.run import _replace_target_belongs_to_other_profile
with patch(
"gateway.status._read_pid_record",
side_effect=RuntimeError("probe exploded"),
):
assert _replace_target_belongs_to_other_profile(424242) is True
class TestArgvConsistencyCheck:
"""Readable argv is a consistency check ONLY — never authority."""
def test_prefix_collision_is_not_a_conflict(self, profile_env):
"""``--profile timothy`` must NOT read as our profile ``tim``:
substring matching would have false-allowed the foreign gateway."""
from gateway.run import (
_looks_like_profile_conflict_from_cmdline as conflict,
)
tim_home = Path("/home/x/.hermes/profiles/tim")
# Foreign target advertising timothy — NOT ours.
assert (
conflict("python -m hermes_cli.main --profile timothy gateway run", tim_home)
is True
)
# Our own exact name stays consistent.
assert (
conflict("python -m hermes_cli.main --profile tim gateway run", tim_home)
is False
)
assert (
conflict("python -m hermes_cli.main -p tim gateway run", tim_home)
is False
)
def test_explicit_home_flag_exact_compare(self, profile_env):
"""HERMES_HOME= on the argv compares path-exactly, not by prefix."""
from gateway.run import (
_looks_like_profile_conflict_from_cmdline as conflict,
)
tim_home = Path("/home/x/.hermes/profiles/tim")
assert (
conflict(
"python -m hermes_cli.main HERMES_HOME=/home/x/.hermes/profiles/timothy gateway run",
tim_home,
)
is True
)
assert (
conflict(
"python -m hermes_cli.main --hermes-home /home/x/.hermes/profiles/tim/ gateway run",
tim_home,
)
is False # trailing slash normalizes away
)
def test_default_home_refuses_any_named_profile_flag(self):
from gateway.run import (
_looks_like_profile_conflict_from_cmdline as conflict,
)
root = Path("/home/x/.hermes")
assert conflict("python -m x --profile sam run", root) is True
assert conflict("python -m x -p sam run", root) is True
assert conflict("python -m x run", root) is False
def test_consistency_contradiction_refuses_even_with_agreeing_record(
self, profile_env
):
"""Record says same-home but the argv explicitly advertises another
profile → refuse (argv contradiction wins the conservative call)."""
from gateway.run import _replace_target_belongs_to_other_profile
with (
patch(
"gateway.status._read_pid_record",
return_value=_record(home=str(profile_env / ".hermes")),
),
patch(
"gateway.status._get_pid_path",
return_value=profile_env / ".hermes" / "gateway.pid",
),
patch(
"gateway.status._get_process_start_time",
return_value=111222333,
),
patch(
"gateway.status._read_process_cmdline",
return_value="python -m hermes_cli.main --profile other-profile gateway run",
),
patch(
"gateway.status._get_process_hermes_home",
return_value=profile_env / ".hermes",
),
):
assert _replace_target_belongs_to_other_profile(424242) is True
class TestSignalBoundary:
"""Integration witness at the destructive boundary (#89315 review req)."""
def _run_replace(self, agent_patches):
from gateway import run as gateway_run
calls = {"terminate": 0, "marker": 0}
def _fake_terminate(pid, force=False):
calls["terminate"] += 1
def _fake_marker(pid):
calls["marker"] += 1
base = [
patch("gateway.status.get_running_pid", return_value=424242),
patch.object(gateway_run, "_replace_target_belongs_to_other_profile"),
patch("gateway.status.terminate_pid", side_effect=_fake_terminate),
patch("gateway.status.write_takeover_marker", side_effect=_fake_marker),
]
import contextlib
with contextlib.ExitStack() as stack:
for p in base:
stack.enter_context(p)
# caller configures the guard mock
agent_patches(stack)
try:
result = asyncio_run(gateway_run.start_gateway(replace=True))
except Exception:
result = "raised"
return result, calls
def test_unprovable_ownership_never_signals(self, profile_env):
"""Unprovable ownership → start_gateway returns False WITHOUT calling
terminate_pid or writing a takeover marker."""
from unittest.mock import MagicMock
def configure(stack):
guard = stack.enter_context(
patch(
"gateway.run._replace_target_belongs_to_other_profile",
return_value=True,
)
)
return guard
result, calls = self._run_replace(lambda s: configure(s))
assert result is False
assert calls["terminate"] == 0, (
"--replace must not signal a target whose ownership is unproven"
)
assert calls["marker"] == 0, (
"no takeover marker may be written for a refused target"
)
def test_provable_same_home_reaches_replace_flow(self, profile_env):
"""Counterpart: bound same-home target still enters the replace flow
(terminate attempted) — the fail-closed gate must not disable legit
Windows-style replaces."""
def configure(stack):
stack.enter_context(
patch(
"gateway.run._replace_target_belongs_to_other_profile",
return_value=False,
)
)
stack.enter_context(
patch(
"gateway.status.get_process_start_time",
return_value=111222333,
)
)
stack.enter_context(patch("gateway.run.time.sleep"))
result, calls = self._run_replace(configure)
assert calls["terminate"] == 1, (
"a provably same-home target must still be replaceable"
)
def asyncio_run(coro):
import asyncio
return asyncio.new_event_loop().run_until_complete(_swallow(coro))
async def _swallow(coro):
"""Run the coroutine; later machinery (runtime locks etc.) may raise in
unit context — callers inspect side-effect counters, not the outcome."""
try:
return await coro
except Exception:
return "raised"