From ee7fda019fce1a8ef68665dadc9e910e51740ae7 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Mon, 15 Jun 2026 19:04:16 -0700 Subject: [PATCH] test-infra: hard-isolate test state from production + resilient teardown (#4289) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * test-infra: hard-isolate test state from production + robust teardown Two production-safety fixes to tests/conftest.py + tests/_pytest_port.py: 1. TEST_STATE_DIR now defaults under the OS temp dir (/tmp/hermes-webui-tests/) instead of HERMES_HOME/webui-test- — which had been writing test state INTO the real ~/.hermes/profiles/webui/ (144 leaked webui-test-* dirs found). Added a hard-stop guard: refuse to run if the resolved test state dir is inside the production Hermes home, so a misconfigured env can never let tests wipe/clobber production profiles/sessions/credentials. 2. _rmtree_retry is now resilient on Linux (was a bare shutil.rmtree): retries transient OSError [Errno 39] Directory-not-empty races (a background daemon writing into the tree during teardown) + a final ignore_errors sweep, and warns instead of raising so a benign cleanup race can't spuriously ERROR a test (the failure mode seen near #4283 context-reconciliation tests). * test-infra: anchor production-proximity guard on literal ~/.hermes (not $HERMES_HOME) The first guard compared TEST_STATE_DIR against $HERMES_HOME, which is overridden to TEST_STATE_DIR itself during a run -> false-tripped collection (6 modules) with 'test dir X is inside production home X'. Now anchor on the literal user-home ~/.hermes and explicitly allow any dir under the OS temp root, so the temp default never trips while a real ~/.hermes target still hard-stops. * test-infra: close TMPDIR-under-~/.hermes guard hole (Codex) Codex flagged: the 'temp is always allowed' exception could be exploited if TMPDIR itself is set under ~/.hermes — then a 'temp' path is a production path and _under_temp would wrongly suppress the guard. Now the temp exception only holds when the temp root is itself OUTSIDE the production home. Verified: normal /tmp default allowed; TMPDIR=~/.hermes/tmp now trips the guard. * test-infra: mirror production-proximity guard into _pytest_port.py (Opus defense-in-depth) * test-infra: remove ~/.hermes production-path fallbacks from 4 test files (Codex) Codex's kickback found the real gap: test_sprint5, test_sprint45, test_onboarding_existing_config, test_regressions each hardcoded a ~/.hermes/webui-mvp-test fallback for their state dir (used when running standalone or if the env var is unset). Repointed all to the shared isolated TEST_STATE_DIR from _pytest_port (temp-rooted). Also refreshed stale ~/.hermes/webui-mvp-test references in test_gateway_sync comment + ARCHITECTURE.md. Now ZERO production-path fallbacks remain in tests. --------- Co-authored-by: nesquena-hermes --- ARCHITECTURE.md | 2 +- tests/_pytest_port.py | 33 ++++++++-- tests/conftest.py | 80 +++++++++++++++++++++--- tests/test_gateway_sync.py | 2 +- tests/test_onboarding_existing_config.py | 3 +- tests/test_regressions.py | 3 +- tests/test_sprint45.py | 7 +-- tests/test_sprint5.py | 5 +- 8 files changed, 113 insertions(+), 22 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 7e586f2c..c83db62b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -145,7 +145,7 @@ Environment variables controlling behavior: Test isolation environment variables (set by conftest.py): HERMES_WEBUI_TEST_PORT=... Optional pinned test port - HERMES_WEBUI_TEST_STATE_DIR=~/.hermes/webui-test-* Optional pinned test state + HERMES_WEBUI_TEST_STATE_DIR=/tmp/hermes-webui-tests/* Optional pinned test state (default: OS temp dir; must be outside ~/.hermes) HERMES_WEBUI_DEFAULT_WORKSPACE=.../test-workspace Isolated test workspace Tests NEVER talk to the production server (port 8787). diff --git a/tests/_pytest_port.py b/tests/_pytest_port.py index ce687b3f..0260c015 100644 --- a/tests/_pytest_port.py +++ b/tests/_pytest_port.py @@ -29,17 +29,42 @@ def _auto_state_dir_name(repo_root: pathlib.Path) -> str: _TESTS_DIR = pathlib.Path(__file__).parent.resolve() _REPO_ROOT = _TESTS_DIR.parent.resolve() -_HERMES_HOME = pathlib.Path(os.getenv('HERMES_HOME', - str(pathlib.Path.home() / '.hermes'))) TEST_PORT = int(os.environ.get('HERMES_WEBUI_TEST_PORT', str(_auto_test_port(_REPO_ROOT)))) BASE = f"http://127.0.0.1:{TEST_PORT}" +# Test state dir: prefer the value conftest.py published to the environment. +# The standalone fallback anchors under the OS temp dir (NOT ~/.hermes) so test +# state is never created inside the production Hermes home — matching conftest's +# hard-isolation default. (See conftest.py TEST_STATE_DIR.) +import tempfile as _tempfile +_TEST_STATE_ROOT = pathlib.Path( + os.environ.get('HERMES_WEBUI_TEST_STATE_ROOT', _tempfile.gettempdir()) +) / 'hermes-webui-tests' TEST_STATE_DIR = pathlib.Path(os.environ.get( 'HERMES_WEBUI_TEST_STATE_DIR', - str(_HERMES_HOME / _auto_state_dir_name(_REPO_ROOT)) -)) + str(_TEST_STATE_ROOT / _auto_state_dir_name(_REPO_ROOT)) +)).resolve() + +# Defense-in-depth: mirror conftest.py's production-proximity guard so a +# standalone import (without conftest) can't resolve a state dir inside the real +# ~/.hermes either. Same logic: trip only if under literal ~/.hermes and the temp +# root isn't itself under production. +_PROD_HERMES_HOME = (pathlib.Path.home() / '.hermes').resolve() +_TEMP_ROOT = pathlib.Path(_tempfile.gettempdir()).resolve() +_temp_root_is_safe = not ( + _TEMP_ROOT == _PROD_HERMES_HOME or _PROD_HERMES_HOME in _TEMP_ROOT.parents +) +_under_temp = _temp_root_is_safe and ( + TEST_STATE_DIR == _TEMP_ROOT or _TEMP_ROOT in TEST_STATE_DIR.parents +) +_under_prod = TEST_STATE_DIR == _PROD_HERMES_HOME or _PROD_HERMES_HOME in TEST_STATE_DIR.parents +if _under_prod and not _under_temp: + raise RuntimeError( + f"REFUSING TO RUN: test state dir {TEST_STATE_DIR} is inside the production " + f"Hermes home {_PROD_HERMES_HOME}. Tests must never touch production files." + ) # Default model injected by conftest — tests that mutate the default model # must restore to this value so later tests see a consistent baseline. diff --git a/tests/conftest.py b/tests/conftest.py index fbeefd8a..09a06b7f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -72,10 +72,52 @@ def _auto_state_dir_name(repo_root) -> str: TEST_PORT = int(os.getenv('HERMES_WEBUI_TEST_PORT', str(_auto_test_port(REPO_ROOT)))) TEST_BASE = f"http://127.0.0.1:{TEST_PORT}" + +# ── Test state dir: HARD-ISOLATED from production ────────────────────────── +# Test state must NEVER live inside the real Hermes home (~/.hermes or any +# HERMES_HOME), or anywhere near production profiles/files/server. Earlier this +# defaulted to ``HERMES_HOME / webui-test-`` which wrote test state INTO +# ~/.hermes/profiles/<...>/ (observed: 144 leaked webui-test-* dirs in the real +# profile home). We now anchor the default under the OS temp dir, in a dedicated +# `hermes-webui-tests/` namespace, fully outside any production tree. +import tempfile as _tempfile +_TEST_STATE_ROOT = pathlib.Path( + os.getenv('HERMES_WEBUI_TEST_STATE_ROOT', _tempfile.gettempdir()) +) / 'hermes-webui-tests' TEST_STATE_DIR = pathlib.Path(os.getenv( 'HERMES_WEBUI_TEST_STATE_DIR', - str(HERMES_HOME / _auto_state_dir_name(REPO_ROOT)) -)) + str(_TEST_STATE_ROOT / _auto_state_dir_name(REPO_ROOT)) +)).resolve() + +# Production-proximity guard: refuse to run if the resolved test state dir lands +# inside the REAL Hermes home tree — a misconfigured HERMES_WEBUI_TEST_STATE_DIR +# pointing at ~/.hermes would let tests wipe/clobber production profiles, +# sessions, and credentials on teardown. We anchor on the literal user-home +# `~/.hermes` (NOT $HERMES_HOME): HERMES_HOME is frequently overridden to a +# profile dir or, during a test run, to TEST_STATE_DIR itself — comparing +# against it would either miss a real production path or false-trip when the +# test dir legitimately lives in /tmp. A test dir under the OS temp dir is always +# allowed even if it nominally sits below a temp-rooted HERMES_HOME. +_PROD_HERMES_HOME = (HOME / '.hermes').resolve() +_TEMP_ROOT = pathlib.Path(_tempfile.gettempdir()).resolve() +# The temp-root exception only holds when the temp root is itself OUTSIDE the +# production home. If TMPDIR is (mis)configured under ~/.hermes, a "temp" path is +# still a production path — don't let it suppress the guard. +_temp_root_is_safe = not ( + _TEMP_ROOT == _PROD_HERMES_HOME or _PROD_HERMES_HOME in _TEMP_ROOT.parents +) +_under_temp = _temp_root_is_safe and ( + TEST_STATE_DIR == _TEMP_ROOT or _TEMP_ROOT in TEST_STATE_DIR.parents +) +_under_prod = TEST_STATE_DIR == _PROD_HERMES_HOME or _PROD_HERMES_HOME in TEST_STATE_DIR.parents +if _under_prod and not _under_temp: + raise RuntimeError( + f"REFUSING TO RUN: test state dir {TEST_STATE_DIR} is inside the production " + f"Hermes home {_PROD_HERMES_HOME}. Tests must never touch production files. " + f"Unset HERMES_WEBUI_TEST_STATE_DIR (defaults to a temp dir) or point it " + f"outside ~/.hermes." + ) + TEST_WORKSPACE = TEST_STATE_DIR / 'test-workspace' # Publish at module level so api.config, _pytest_port.py, and any test module @@ -685,13 +727,21 @@ def _kill_port_owner(port): def _rmtree_retry(path): - """Remove a tree, retrying on Windows after clearing read-only bits.""" + """Remove a tree, retrying transient races (Linux + Windows). + + The retry loop covers two real failure modes seen in CI/local: + * Windows: read-only bits / lingering handles (cleared via onexc/onerror). + * Linux: `OSError [Errno 39] Directory not empty` — a background thread or + daemon (model-metadata fetcher, session-events watcher, profile writer) + creates a file inside the tree WHILE shutil.rmtree is walking it, so the + parent rmdir fails even though we just emptied it. A short retry lets the + racing writer settle; a final ignore_errors sweep guarantees teardown + never fails the test over a pure cleanup race (the dir is abandoned test + state, not an assertion target). + """ target = pathlib.Path(path) if not target.exists(): return - if sys.platform != "win32": - shutil.rmtree(target) - return def _clear_readonly(_func, entry, _exc): try: @@ -704,7 +754,7 @@ def _rmtree_retry(path): {"onexc": _clear_readonly} if "onexc" in inspect.signature(shutil.rmtree).parameters else {"onerror": _clear_readonly} - ) + ) if sys.platform == "win32" else {} attempts = 5 last_exc = None @@ -720,13 +770,27 @@ def _rmtree_retry(path): break time.sleep(0.3) + # Final fallback: a concurrent-writer race (Errno 39) shouldn't fail teardown. + # Best-effort ignore_errors sweep; if anything remains it's abandoned test + # state under HERMES_HOME, not something a test asserts on. + shutil.rmtree(target, ignore_errors=True) + if not target.exists(): + return leftovers = [] try: leftovers = [child.name for child in list(target.iterdir())[:5]] except Exception: pass + # Don't raise — log-and-continue. Raising here turns a benign cleanup race + # into a spurious test ERROR (the behaviour #4283-area tests hit when a + # model-metadata background fetch repopulated the profile dir mid-teardown). + import warnings as _warnings leftover_note = f" leftovers={leftovers}" if leftovers else "" - raise RuntimeError(f"Could not remove {target} after {attempts} attempts.{leftover_note}") from last_exc + _warnings.warn( + f"_rmtree_retry: could not fully remove {target} after {attempts} attempts" + f"{leftover_note} (last_exc={last_exc!r}); left for OS temp cleanup.", + stacklevel=2, + ) # ── Session-scoped test server ──────────────────────────────────────────────── diff --git a/tests/test_gateway_sync.py b/tests/test_gateway_sync.py index 3081ed15..51efe2aa 100644 --- a/tests/test_gateway_sync.py +++ b/tests/test_gateway_sync.py @@ -48,7 +48,7 @@ def _get_test_state_dir(): (via os.environ.setdefault) so that tests writing directly to state.db always use the same path the test server was started with. If the env var is not set (e.g. when running this file standalone), fall back to the conftest - formula: HERMES_HOME/webui-mvp-test. + formula via _pytest_port (temp-rooted, never ~/.hermes). """ # Use _pytest_port which applies the same auto-derivation as conftest.py from tests._pytest_port import TEST_STATE_DIR as _ptsd diff --git a/tests/test_onboarding_existing_config.py b/tests/test_onboarding_existing_config.py index 92f700c0..e5f17205 100644 --- a/tests/test_onboarding_existing_config.py +++ b/tests/test_onboarding_existing_config.py @@ -290,7 +290,8 @@ def _server_hermes_home() -> pathlib.Path: env_path = data.get("system", {}).get("env_path", "") if env_path: return pathlib.Path(env_path).parent - return pathlib.Path(os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(pathlib.Path.home() / ".hermes" / "webui-mvp-test"))) + from tests._pytest_port import TEST_STATE_DIR + return pathlib.Path(os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(TEST_STATE_DIR))) def _server_reachable() -> bool: diff --git a/tests/test_regressions.py b/tests/test_regressions.py index f16053b8..2d9e6be4 100644 --- a/tests/test_regressions.py +++ b/tests/test_regressions.py @@ -116,7 +116,8 @@ def test_session_with_tool_calls_in_json_loads_ok(cleanup_test_sessions): sid = make_session(cleanup_test_sessions) # Manually inject tool_calls into the session's JSON file - sessions_dir = pathlib.Path(os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(pathlib.Path.home() / ".hermes" / "webui-mvp-test"))) / "sessions" + from tests._pytest_port import TEST_STATE_DIR + sessions_dir = pathlib.Path(os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(TEST_STATE_DIR))) / "sessions" session_file = sessions_dir / f"{sid}.json" if session_file.exists(): d = json.loads(session_file.read_text()) diff --git a/tests/test_sprint45.py b/tests/test_sprint45.py index 031bcc24..9bdb9394 100644 --- a/tests/test_sprint45.py +++ b/tests/test_sprint45.py @@ -14,15 +14,14 @@ import urllib.request import os -from tests._pytest_port import BASE +from tests._pytest_port import BASE, TEST_STATE_DIR REPO = pathlib.Path(__file__).parent.parent # Use HERMES_WEBUI_TEST_STATE_DIR if available (set by conftest for the test process), -# falling back to the conventional webui-mvp-test path. +# falling back to the shared isolated TEST_STATE_DIR (temp-rooted, never ~/.hermes). def _get_settings_file() -> pathlib.Path: """Resolve SETTINGS_FILE at call time (env var set by conftest after module import).""" state_dir = pathlib.Path( - os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", - str(pathlib.Path.home() / ".hermes" / "webui-mvp-test")) + os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(TEST_STATE_DIR)) ) return state_dir / "settings.json" diff --git a/tests/test_sprint5.py b/tests/test_sprint5.py index 8a4064d9..a28f774f 100644 --- a/tests/test_sprint5.py +++ b/tests/test_sprint5.py @@ -225,8 +225,9 @@ def test_file_save_path_traversal_blocked(cleanup_test_sessions): assert status in (400, 500) def test_session_index_created_after_save(cleanup_test_sessions): - # Index is created in the TEST state dir, not the production dir - test_state_dir = pathlib.Path(os.environ.get("HERMES_WEBUI_TEST_STATE_DIR", str(pathlib.Path.home() / ".hermes" / "webui-mvp-test"))) + # Index is created in the TEST state dir, not the production dir. + # Use the shared isolated TEST_STATE_DIR (temp-rooted, never ~/.hermes). + from tests._pytest_port import TEST_STATE_DIR as test_state_dir index_path = test_state_dir / "sessions" / "_index.json" make_session_tracked(cleanup_test_sessions) # Index may not exist yet if cleanup already wiped it -- just check the endpoint works