Merge pull request #5707 from nesquena/fix/last-two-flakes

Fix the last 2 chronic local full-suite test-isolation flakes (test-only)

CHANGELOG.md

@@ -3,6 +3,10 @@
 
 ## [Unreleased]
 
+### Internal
+
+- **Fixed the last 2 chronic local full-suite test-isolation flakes.** `test_tls_support::test_tls_startup_failure_fallback_to_http` and `test_v050259_sessiondb_fd_leak::test_session_db_close_is_idempotent` failed only in a full local suite run (they skip on CI, where the agent package isn't co-located). Root cause was the same "simulate agent package unavailable" antipattern shipped-fixed for the profile cluster: `test_custom_provider_prefix_collisions` clobbered `sys.modules['agent']` at collection time (never restored), and several helpers emptied real package `__path__` lists in place. Fixed at the source — the collection-time shim is now guarded on `importlib.util.find_spec` (only installed when the real package is genuinely absent), the in-place `__path__` mutations use `monkeypatch.setattr(..., raising=False)`, and `test_issue1574`'s fake-agent helper restores the env/`sys.path` it mutates. Full local suite now 0 failed (was 2); `test_title_aux_routing` unaffected. Test-only.
+
 ### Fixed
 
 - **Profile list no longer serves stale rows after a change.** The session-load perf pass (v0.51.908) bumped the profile-list cache TTL to 60s, but profile-row mutations (default model / providers / skills / gateway config) don't invalidate that cache, so a changed profile could show stale details for up to a minute. Reverted the TTL to 4s — frequent enough that mutation-to-refresh staleness is negligible, while rapid dropdown re-opens stay cheap. Thanks @Kopamed. (#5696)

tests/test_commands_endpoint.py

@@ -30,7 +30,11 @@ def _install_fake_mcp_tool(monkeypatch, shutdown, discover, servers=None, lock=N
 def _install_fake_codex_runtime_switch(monkeypatch):
     import sys
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # Restore the real hermes_cli.__path__ on teardown instead of emptying it in
+    # place: `sys.modules.get(...)` grabs the REAL package object, so a bare
+    # `__path__ = []` permanently strands it (later `import hermes_cli.<sub>`
+    # fails for the rest of the suite). monkeypatch.setattr snapshots and restores.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     codex_runtime_switch = ModuleType("hermes_cli.codex_runtime_switch")
     calls = []
 
@@ -64,7 +68,10 @@ def _install_fake_codex_runtime_switch(monkeypatch):
 def _install_fake_skill_commands(monkeypatch, reload_skills):
     import sys
     agent_pkg = sys.modules.get("agent") or ModuleType("agent")
-    agent_pkg.__path__ = []
+    # See _install_fake_codex_runtime_switch: monkeypatch.setattr restores the
+    # real agent.__path__ on teardown so `from agent.<sub> import ...` keeps
+    # working in later tests (chronic full-suite poison otherwise).
+    monkeypatch.setattr(agent_pkg, "__path__", [], raising=False)
     skill_commands = ModuleType("agent.skill_commands")
     skill_commands.reload_skills = reload_skills
     monkeypatch.setitem(sys.modules, "agent", agent_pkg)
@@ -76,7 +83,9 @@ def _install_fake_account_usage(monkeypatch, *, view=None, exc=None):
     import sys
 
     agent_pkg = sys.modules.get("agent") or ModuleType("agent")
-    agent_pkg.__path__ = []
+    # monkeypatch.setattr restores the real agent.__path__ on teardown (see
+    # _install_fake_skill_commands) to avoid permanently poisoning the package.
+    monkeypatch.setattr(agent_pkg, "__path__", [], raising=False)
     account_usage = ModuleType("agent.account_usage")
 
     def build_credits_view(*, markdown=False, timeout=10.0):

tests/test_custom_provider_prefix_collisions.py

@@ -6,14 +6,42 @@ from unittest.mock import patch
 def fake_get_model_context_length(model, base_url="", **kwargs):
     return 1048576
 
-# Ensure a mock of 'agent' exists in sys.modules to prevent ModuleNotFoundError in routes imports.
-fake_agent = types.ModuleType("agent")
-fake_agent.__path__ = []
-metadata = types.ModuleType("agent.model_metadata")
-fake_agent.model_metadata = metadata  # type: ignore[attr-defined]
-sys.modules["agent"] = fake_agent
-sys.modules["agent.model_metadata"] = metadata
-metadata.get_model_context_length = fake_get_model_context_length
+# Ensure an 'agent' module exists in sys.modules to prevent ModuleNotFoundError
+# when api.routes is imported on a CI runner that has ONLY the WebUI repo (no
+# hermes-agent package). CRITICAL: only install the fake when the REAL agent
+# package is not importable. Unconditionally doing `sys.modules["agent"] = fake`
+# with `fake.__path__ = []` clobbers the genuine, importable agent package for
+# the WHOLE process (this runs at collection time and is never restored) — a
+# later `from agent.<sub> import ...` in the full suite (e.g. hermes_state's
+# `from agent.memory_manager import sanitize_context`) then fails with
+# ModuleNotFoundError. Guarding on find_spec keeps the real package intact
+# locally while preserving the CI import path. Nothing in this module asserts
+# against the fake metadata, so the shim is purely an import guard.
+def _real_agent_metadata_importable() -> bool:
+    """True only if the genuine agent.model_metadata can actually be imported.
+
+    ``importlib.util.find_spec`` RAISES ``ValueError: agent.__spec__ is None``
+    when a spec-less partial ``agent`` module is already in ``sys.modules`` (which
+    happens during CI collection), so it can't be probed bare. Attempt the real
+    import defensively instead: ANY failure means the genuine package isn't
+    usable here, so the shim should be installed.
+    """
+    try:
+        import agent.model_metadata as _amm  # noqa: F401
+        return getattr(_amm, "get_model_context_length", None) is not None
+    except Exception:
+        return False
+
+
+if not _real_agent_metadata_importable():
+    fake_agent = sys.modules.get("agent") or types.ModuleType("agent")
+    if not hasattr(fake_agent, "__path__"):
+        fake_agent.__path__ = []
+    metadata = types.ModuleType("agent.model_metadata")
+    metadata.get_model_context_length = fake_get_model_context_length
+    fake_agent.model_metadata = metadata  # type: ignore[attr-defined]
+    sys.modules["agent"] = fake_agent
+    sys.modules["agent.model_metadata"] = metadata
 
 from api.routes import _normalize_provider_id, _resolve_compatible_session_model_state
 

tests/test_issue1574_cron_profile_lock.py

@@ -60,7 +60,29 @@ def _write_spawn_fake_agent(root: Path, *, run_job_body: str):
 
 
 def _activate_spawn_fake_agent(fake_agent_root: Path):
+    """Repoint the agent at ``fake_agent_root`` and return a restore callable.
+
+    This mutates process-global state (``HERMES_WEBUI_AGENT_DIR``, ``PYTHONPATH``,
+    ``sys.path``) directly rather than via monkeypatch, because the callers run it
+    inside a spawned/forked child process. Leaving those mutations unrestored
+    poisons any later in-process consumer — a subsequent test that spawns
+    ``server.py`` as a subprocess would inherit a stale ``HERMES_WEBUI_AGENT_DIR``
+    / ``PYTHONPATH`` pointing at a torn-down fake dir and the child could not
+    import the real agent (the chronic full-suite
+    ``test_tls_support::test_tls_startup_failure_fallback_to_http`` failure).
+
+    We snapshot the original env vars + ``sys.path`` up front and return a
+    ``restore()`` the caller MUST invoke in a ``finally`` so the mutation never
+    outlives this activation. In the fork/spawn children this is belt-and-braces
+    (the child exits anyway); it also makes the helper safe for any in-process use.
+    """
     fake_path = str(fake_agent_root)
+    _saved_env = {
+        k: os.environ.get(k)
+        for k in ("HERMES_WEBUI_AGENT_DIR", "PYTHONPATH")
+    }
+    _saved_sys_path = list(sys.path)
+
     os.environ["HERMES_WEBUI_AGENT_DIR"] = fake_path
     existing = os.environ.get("PYTHONPATH", "")
     parts = [
@@ -86,6 +108,16 @@ def _activate_spawn_fake_agent(fake_agent_root: Path):
     ):
         sys.modules.pop(module_name, None)
 
+    def _restore():
+        for _k, _v in _saved_env.items():
+            if _v is None:
+                os.environ.pop(_k, None)
+            else:
+                os.environ[_k] = _v
+        sys.path[:] = _saved_sys_path
+
+    return _restore
+
 
 def _real_hermes_agent_editable_install_present() -> bool:
     """Detect a developer-machine editable install of hermes-agent.
@@ -133,12 +165,15 @@ def _large_cron_payload_runner(profile_home, result_queue):
                 "    return True, payload, payload, None\n"
             ),
         )
-        _activate_spawn_fake_agent(fake_agent_root)
-        import api.routes as routes
+        _activate_restore = _activate_spawn_fake_agent(fake_agent_root)
+        try:
+            import api.routes as routes
 
-        success, output, final_response, error = routes._run_cron_job_in_profile_subprocess(
-            {"id": "large-payload"}, Path(profile_home)
-        )
+            success, output, final_response, error = routes._run_cron_job_in_profile_subprocess(
+                {"id": "large-payload"}, Path(profile_home)
+            )
+        finally:
+            _activate_restore()
         result_queue.put(("ok", success, len(output), len(final_response), error))
     except BaseException as exc:  # pragma: no cover - surfaced in parent process
         import traceback
@@ -156,12 +191,15 @@ def _selected_profile_home_runner(profile_home, result_queue):
                 "    return True, str(scheduler._hermes_home), 'final', None\n"
             ),
         )
-        _activate_spawn_fake_agent(fake_agent_root)
-        import api.routes as routes
+        _activate_restore = _activate_spawn_fake_agent(fake_agent_root)
+        try:
+            import api.routes as routes
 
-        success, output, final_response, error = routes._run_cron_job_in_profile_subprocess(
-            {"id": "job1574"}, Path(profile_home)
-        )
+            success, output, final_response, error = routes._run_cron_job_in_profile_subprocess(
+                {"id": "job1574"}, Path(profile_home)
+            )
+        finally:
+            _activate_restore()
         result_queue.put(("ok", success, output, final_response, error))
     except BaseException as exc:  # pragma: no cover - surfaced in parent process
         import traceback

tests/test_issue4087_skill_bundles.py

@@ -19,7 +19,11 @@ def _install_fake_skill_bundles(monkeypatch, *, bundles=None, resolver=None, bui
     import sys
 
     agent_pkg = sys.modules.get("agent") or ModuleType("agent")
-    agent_pkg.__path__ = []
+    # `sys.modules.get(...)` returns the REAL agent package; emptying its
+    # __path__ in place strands it for the rest of the suite (later
+    # `from agent.<sub> import ...` — e.g. hermes_state's `agent.memory_manager`
+    # — then fails). monkeypatch.setattr snapshots and restores it on teardown.
+    monkeypatch.setattr(agent_pkg, "__path__", [], raising=False)
     skill_bundles = ModuleType("agent.skill_bundles")
     skill_bundles.list_bundles = lambda: list(bundles or [])
     skill_bundles.resolve_bundle_command_key = resolver or (lambda name: None)

tests/test_issue5057_moa_webui_route.py

@@ -14,7 +14,9 @@ from tests.conftest import TEST_BASE, requires_agent_modules
 
 def _install_fake_moa_config(monkeypatch, *, default_preset="moa-default", usage_text="Usage: /moa <prompt>"):
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # monkeypatch.setattr restores the REAL hermes_cli.__path__ on teardown;
+    # emptying it in place would strand the package for the rest of the suite.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     moa_config = ModuleType("hermes_cli.moa_config")
 
     def normalize_moa_config(cfg):
@@ -32,7 +34,9 @@ def _install_fake_moa_config(monkeypatch, *, default_preset="moa-default", usage
 
 def _install_fake_hermes_config(monkeypatch, cfg_data=None):
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # monkeypatch.setattr restores the REAL hermes_cli.__path__ on teardown;
+    # emptying it in place would strand the package for the rest of the suite.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     config_mod = ModuleType("hermes_cli.config")
 
     def load_config():

tests/test_moa_model_picker_provider.py

@@ -84,7 +84,9 @@ def test_resolve_moa_config_uses_selected_preset(monkeypatch):
     }
 
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # monkeypatch.setattr restores the REAL hermes_cli.__path__ on teardown;
+    # emptying it in place would strand the package for the rest of the suite.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     moa_config = ModuleType("hermes_cli.moa_config")
 
     def normalize_moa_config(raw):
@@ -126,7 +128,9 @@ def test_resolve_moa_config_falls_back_when_preset_resolution_raises(monkeypatch
         }
     }
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # monkeypatch.setattr restores the REAL hermes_cli.__path__ on teardown;
+    # emptying it in place would strand the package for the rest of the suite.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     moa_config = ModuleType("hermes_cli.moa_config")
 
     def normalize_moa_config(raw):
@@ -167,7 +171,9 @@ def test_resolve_moa_config_ignores_non_dict_preset_result(monkeypatch):
         }
     }
     hermes_cli_pkg = sys.modules.get("hermes_cli") or ModuleType("hermes_cli")
-    hermes_cli_pkg.__path__ = []
+    # monkeypatch.setattr restores the REAL hermes_cli.__path__ on teardown;
+    # emptying it in place would strand the package for the rest of the suite.
+    monkeypatch.setattr(hermes_cli_pkg, "__path__", [], raising=False)
     moa_config = ModuleType("hermes_cli.moa_config")
 
     def normalize_moa_config(raw):