From 5742a23836ef910bcc2dee4ba7b64fb13051ad6c Mon Sep 17 00:00:00 2001 From: nesquena-hermes <[email protected]> Date: Sun, 31 May 2026 18:54:23 +0000 Subject: [PATCH] =?UTF-8?q?fix:=20apply=20Opus+Codex=20gate=20findings=20?= =?UTF-8?q?=E2=80=94=20stale-stopped=20not=20configured=20(#1944),=20narro?= =?UTF-8?q?w=20#2905=20markers,=20scope=20except?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both pre-release reviewers (Opus advisor + Codex regression gate) converged on the same MUST-FIX: - #3194: treating gateway_stale_stopped_state as 'configured' contradicted #1944 (a stopped root gateway should read like 'not configured' so the banner doesn't nag). Now ONLY stale-RUNNING metadata (reason=gateway_stale_running_state or gateway_state=='running') flips configured=True; stale-stopped falls through to bool(identity_map) like the genuinely-unconfigured case. Updated the test accordingly + added a stale-stopped no-regression test. Opus follow-ups also applied: - #2905: narrowed the populated-home markers to WebUI-only artifacts (webui/, webui/sessions, webui/settings.json), dropping config.yaml/auth.json so a long-time agent user installing WebUI fresh isn't wrongly diverted to the legacy %USERPROFILE%\.hermes (auth.json predates #2897 there). - profiles._resolve_base_hermes_home(): narrowed except Exception -> ImportError so a real bug in the config helper still surfaces. Adjacent suites green: #2840, #1879, gateway_status_agent_health (66 tests). --- api/config.py | 16 ++++++---- api/profiles.py | 3 +- api/routes.py | 29 +++++++++++-------- ...issue2905_windows_home_migration_safety.py | 17 ++++++++--- ...est_issue3194_gateway_configured_banner.py | 25 +++++++++++++--- 5 files changed, 64 insertions(+), 26 deletions(-) diff --git a/api/config.py b/api/config.py index dc6bc0d9..ca0d80ea 100644 --- a/api/config.py +++ b/api/config.py @@ -32,13 +32,21 @@ REPO_ROOT = Path(__file__).parent.parent.resolve() def _hermes_home_has_webui_state(base: Path) -> bool: - """Return True when *base* looks like a populated Hermes home with WebUI state. + """Return True when *base* holds real WebUI state under its ``webui/`` dir. Used only on Windows to detect a pre-v0.51.134 install at the legacy ``%USERPROFILE%\\.hermes`` location so we don't strand the user's existing sessions/pins/settings when the default moved to ``%LOCALAPPDATA%\\hermes`` - (#2905). We treat the home as "real" if any of the durable WebUI/agent - artifacts exist under it. Cheap stat-only checks; never raises. + (#2905). + + We intentionally check ONLY WebUI-owned artifacts (the ``webui/`` subtree), + NOT agent-owned files like ``config.yaml`` / ``auth.json``. The agent has + defaulted to ``%LOCALAPPDATA%\\hermes`` on Windows since before #2897, so a + long-time agent user who never ran WebUI at the legacy location would have a + stray ``auth.json`` there — keying on that would wrongly divert a *fresh* + WebUI install to the legacy dir. Only ``webui/`` state is what actually + gets stranded by the move, so it is the correct and narrow signal. + Cheap stat-only checks; never raises. """ try: if not base.is_dir(): @@ -47,8 +55,6 @@ def _hermes_home_has_webui_state(base: Path) -> bool: base / "webui" / "sessions", # WebUI session store base / "webui" / "settings.json", # WebUI UI settings + pins base / "webui", # WebUI state dir at all - base / "config.yaml", # agent config - base / "auth.json", # agent credentials ) return any(m.exists() for m in markers) except OSError: diff --git a/api/profiles.py b/api/profiles.py index ca18fc0f..27ffa8b0 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -158,8 +158,9 @@ def _resolve_base_hermes_home() -> Path: try: from api.config import _platform_default_hermes_home return _platform_default_hermes_home() - except Exception: + except ImportError: # Defensive: never let a config import problem break profile resolution. + # Scoped to ImportError so a real bug inside the helper still surfaces. if os.name == 'nt': local_app_data = os.getenv('LOCALAPPDATA', '').strip() if local_app_data: diff --git a/api/routes.py b/api/routes.py index ca68517a..69982cd6 100644 --- a/api/routes.py +++ b/api/routes.py @@ -5250,20 +5250,25 @@ def handle_get(handler, parsed) -> bool: else: # alive is None # `alive is None` conflates two very different states: # (a) no gateway metadata at all → genuinely not configured - # (b) gateway metadata EXISTS but is stale/inconclusive (e.g. a - # freshly-started gateway that hasn't ticked yet, or a - # cross-container gateway whose running-state went stale). - # Case (b) still proves a gateway is *configured* — the banner - # must not report "Gateway not configured" just because no - # conversation has happened yet and identity_map is empty (#3194). + # (b) gateway metadata EXISTS but is stale/inconclusive. + # A stale-but-RUNNING gateway (freshly started, hasn't ticked + # `updated_at` yet, or cross-container) still proves the gateway is + # *configured* — the banner must not report "Gateway not configured" + # just because no conversation has happened yet and identity_map is + # empty (#3194). + # + # A stale-STOPPED gateway is deliberately NOT treated as configured: + # agent_health emits `gateway_stale_stopped_state` precisely so a + # stopped service the user isn't running reads like "no root gateway + # configured" rather than nagging (#1944). So stale-stopped falls + # through to the identity_map signal like the genuinely-unconfigured + # case. details = health.get("details") or {} - has_gateway_metadata = bool(details.get("gateway_state")) or ( - details.get("reason") in { - "gateway_stale_running_state", - "gateway_stale_stopped_state", - } + gateway_running_metadata = ( + details.get("reason") == "gateway_stale_running_state" + or details.get("gateway_state") == "running" ) - configured = True if has_gateway_metadata else bool(identity_map) + configured = True if gateway_running_metadata else bool(identity_map) running = bool(identity_map) platforms_set: set[str] = set() diff --git a/tests/test_issue2905_windows_home_migration_safety.py b/tests/test_issue2905_windows_home_migration_safety.py index 012234df..11af68fd 100644 --- a/tests/test_issue2905_windows_home_migration_safety.py +++ b/tests/test_issue2905_windows_home_migration_safety.py @@ -174,14 +174,23 @@ class TestHermesHomeHasWebuiState: (tmp_path / "webui" / "sessions").mkdir(parents=True) assert config._hermes_home_has_webui_state(tmp_path) is True - def test_config_yaml_marker_counts(self, tmp_path): - (tmp_path / "config.yaml").write_text("x: 1\n", encoding="utf-8") + def test_webui_settings_marker_counts(self, tmp_path): + (tmp_path / "webui").mkdir() + (tmp_path / "webui" / "settings.json").write_text("{}", encoding="utf-8") assert config._hermes_home_has_webui_state(tmp_path) is True - def test_auth_json_marker_counts(self, tmp_path): - (tmp_path / "auth.json").write_text("{}", encoding="utf-8") + def test_webui_dir_alone_counts(self, tmp_path): + (tmp_path / "webui").mkdir() assert config._hermes_home_has_webui_state(tmp_path) is True + def test_agent_only_artifacts_do_not_count(self, tmp_path): + """A home with ONLY agent files (config.yaml / auth.json) and no webui/ + dir is NOT treated as WebUI state — otherwise a long-time agent user + installing WebUI fresh would be wrongly diverted to the legacy dir.""" + (tmp_path / "config.yaml").write_text("model: x\n", encoding="utf-8") + (tmp_path / "auth.json").write_text("{}", encoding="utf-8") + assert config._hermes_home_has_webui_state(tmp_path) is False + def test_profiles_base_home_delegates_to_config(monkeypatch, tmp_path): """profiles._resolve_base_hermes_home() must share config's resolution so diff --git a/tests/test_issue3194_gateway_configured_banner.py b/tests/test_issue3194_gateway_configured_banner.py index 8c75dc20..151ecf73 100644 --- a/tests/test_issue3194_gateway_configured_banner.py +++ b/tests/test_issue3194_gateway_configured_banner.py @@ -87,16 +87,33 @@ def test_stale_running_with_empty_identity_map_is_configured(monkeypatch): assert data["running"] is False -def test_gateway_state_detail_alone_marks_configured(monkeypatch): - """Any alive=None payload carrying a gateway_state detail is configured, - even if the reason string is something else.""" +def test_gateway_state_running_detail_marks_configured(monkeypatch): + """An alive=None payload whose details report gateway_state == 'running' + is configured, even if the reason string differs — the running metadata + is the signal.""" + payload = { + "alive": None, + "details": {"state": "unknown", "reason": "cross_container_freshness", + "gateway_state": "running"}, + } + data = _call_gateway_status(monkeypatch, health_payload=payload, identity_map={}) + assert data["configured"] is True + + +def test_stale_stopped_with_empty_identity_map_not_configured(monkeypatch): + """No-regression for #1944: a stale-STOPPED gateway must NOT report + configured when there's no traffic. agent_health emits + gateway_stale_stopped_state precisely so a stopped service the user isn't + running reads like 'no root gateway configured' rather than nagging. + Only stale-RUNNING metadata flips configured=True (#3194).""" payload = { "alive": None, "details": {"state": "unknown", "reason": "gateway_stale_stopped_state", "gateway_state": "stopped"}, } data = _call_gateway_status(monkeypatch, health_payload=payload, identity_map={}) - assert data["configured"] is True + assert data["configured"] is False + assert data["running"] is False def test_truly_unconfigured_stays_unconfigured(monkeypatch):