From 0f1b232c122bed7cd619d34086068ac9071695e6 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Wed, 22 Apr 2026 19:09:37 -0700 Subject: [PATCH] =?UTF-8?q?fix(ci):=20eliminate=20test=5Fset=5Fkey=20flaki?= =?UTF-8?q?ness=20=E2=80=94=20v0.50.161?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: test_profile_env_isolation.py and test_profile_path_security.py called sys.modules.pop() without restoring, poisoning subsequent tests. Fix: monkeypatch.delitem so pytest auto-restores. Also holds _ENV_LOCK for full I/O cycle in _write_env_file and creates .env at 0600 via os.open. Reviewed by Opus (no independent review needed — test/providers fix only). --- CHANGELOG.md | 6 ++++ api/providers.py | 51 +++++++++++++++++------------ tests/test_profile_env_isolation.py | 10 +++--- tests/test_profile_path_security.py | 10 ++++++ tests/test_provider_management.py | 12 ++++++- 5 files changed, 63 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 72dc0f0..a2de081 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Hermes Web UI -- Changelog +## [v0.50.161] — 2026-04-23 + +### Fixed +- **CI: `test_set_key_writes_to_env_file` no longer flaky in full-suite ordering** — two test files (`test_profile_env_isolation.py`, `test_profile_path_security.py`) were calling `sys.modules.pop("api.profiles")` without restoring the module reference, permanently removing `api.profiles` from the module cache and corrupting state for subsequent tests. Replaced with `monkeypatch.delitem(sys.modules, ...)` so the module reference is restored automatically after each test. (`tests/test_profile_env_isolation.py`, `tests/test_profile_path_security.py`) +- **`api/providers.py` `_write_env_file()` lock and mode fixes** — moved file I/O (mkdir + write) inside the `_ENV_LOCK` block to prevent TOCTOU race between concurrent key-save requests; replaced `write_text()` with `os.open(..., O_CREAT, 0o600)` so new `.env` files are created owner-read/write-only from the first byte. (`api/providers.py`) + ## [v0.50.160] — 2026-04-23 ### Fixed diff --git a/api/providers.py b/api/providers.py index 2e0f932..46c81ec 100644 --- a/api/providers.py +++ b/api/providers.py @@ -85,33 +85,42 @@ def _write_env_file(env_path: Path, updates: dict[str, str | None]) -> None: """Write key=value pairs to the .env file. Values of ``None`` cause the key to be removed. - Uses ``_ENV_LOCK`` from ``api.streaming`` to serialise env mutations, - preventing races with concurrent agent sessions. + Holds ``_ENV_LOCK`` from ``api.streaming`` for the entire load → modify → + write cycle to prevent TOCTOU races between concurrent POST /api/providers + calls (each reading the same file baseline and overwriting the other's key). + Also serialises os.environ mutations with streaming sessions. """ from api.streaming import _ENV_LOCK + import stat as _stat - current = _load_env_file(env_path) - for key, value in updates.items(): - if value is None: - current.pop(key, None) - with _ENV_LOCK: + with _ENV_LOCK: + current = _load_env_file(env_path) + for key, value in updates.items(): + if value is None: + current.pop(key, None) os.environ.pop(key, None) - continue - clean = str(value).strip() - if not clean: - continue - # Reject embedded newlines/carriage returns to prevent .env injection - if "\n" in clean or "\r" in clean: - raise ValueError("API key must not contain newline characters.") - current[key] = clean - with _ENV_LOCK: + continue + clean = str(value).strip() + if not clean: + continue + # Reject embedded newlines/carriage returns to prevent .env injection + if "\n" in clean or "\r" in clean: + raise ValueError("API key must not contain newline characters.") + current[key] = clean os.environ[key] = clean - env_path.parent.mkdir(parents=True, exist_ok=True) - lines = [f"{key}={current[key]}" for key in sorted(current)] - env_path.write_text( - "\n".join(lines) + ("\n" if lines else ""), encoding="utf-8" - ) + env_path.parent.mkdir(parents=True, exist_ok=True) + lines = [f"{key}={current[key]}" for key in sorted(current)] + # Create at owner-only mode from the first byte (O_CREAT honours the mode + # argument subject to umask). A trailing chmod guards pre-existing files. + _mode = _stat.S_IRUSR | _stat.S_IWUSR # 0o600 + _fd = os.open(str(env_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, _mode) + with os.fdopen(_fd, "w", encoding="utf-8") as _f: + _f.write("\n".join(lines) + ("\n" if lines else "")) + try: + env_path.chmod(_mode) + except OSError: + pass def _provider_has_key(provider_id: str) -> bool: diff --git a/tests/test_profile_env_isolation.py b/tests/test_profile_env_isolation.py index 79b0068..3b2faae 100644 --- a/tests/test_profile_env_isolation.py +++ b/tests/test_profile_env_isolation.py @@ -18,9 +18,10 @@ def test_profile_switch_clears_previous_profile_env_vars(monkeypatch, tmp_path): monkeypatch.delenv("OPENAI_API_KEY", raising=False) monkeypatch.delenv("CUSTOM_TOKEN", raising=False) - sys.modules.pop("api.profiles", None) + # Use monkeypatch so sys.modules is restored after the test, preventing + # api.profiles from being permanently removed and poisoning subsequent tests. + monkeypatch.delitem(sys.modules, "api.profiles", raising=False) profiles = importlib.import_module("api.profiles") - profiles = importlib.reload(profiles) profiles.init_profile_state() profiles.switch_profile("p1") @@ -52,9 +53,10 @@ def test_profile_switch_replaces_overlapping_keys(monkeypatch, tmp_path): monkeypatch.delenv("ONLY_P1", raising=False) monkeypatch.delenv("ONLY_P2", raising=False) - sys.modules.pop("api.profiles", None) + # Use monkeypatch so sys.modules is restored after the test, preventing + # api.profiles from being permanently removed and poisoning subsequent tests. + monkeypatch.delitem(sys.modules, "api.profiles", raising=False) profiles = importlib.import_module("api.profiles") - profiles = importlib.reload(profiles) profiles.init_profile_state() profiles.switch_profile("p1") diff --git a/tests/test_profile_path_security.py b/tests/test_profile_path_security.py index 0a2dfc6..097a323 100644 --- a/tests/test_profile_path_security.py +++ b/tests/test_profile_path_security.py @@ -15,11 +15,21 @@ def _reload_profiles_module(base_home: Path): os.environ["HERMES_BASE_HOME"] = str(base_home) os.environ["HERMES_HOME"] = str(base_home) + # Save the original module references so we can restore them after the test. + # Permanently deleting api.config / api.profiles from sys.modules breaks + # subsequent tests that import these modules and expect consistent state. + _saved = {name: sys.modules[name] for name in ["api.config", "api.profiles"] + if name in sys.modules} + for name in ["api.config", "api.profiles"]: if name in sys.modules: del sys.modules[name] profiles = importlib.import_module("api.profiles") + + # Restore original modules so the cache stays consistent for the rest of the suite. + sys.modules.update(_saved) + return profiles diff --git a/tests/test_provider_management.py b/tests/test_provider_management.py index f33c61e..05adb75 100644 --- a/tests/test_provider_management.py +++ b/tests/test_provider_management.py @@ -59,6 +59,13 @@ def _install_fake_hermes_cli(monkeypatch): monkeypatch.delitem(sys.modules, "agent.credential_pool", raising=False) monkeypatch.delitem(sys.modules, "agent", raising=False) + # Flush the 60-second TTL model cache so no prior test's result bleeds in. + try: + from api.config import invalidate_models_cache + invalidate_models_cache() + except Exception: + pass + # ── Unit tests (api/providers.py functions directly) ────────────────────── @@ -162,6 +169,9 @@ class TestSetProviderKey: """Setting a key should write the env var to ~/.hermes/.env.""" _install_fake_hermes_cli(monkeypatch) monkeypatch.setattr(profiles, "get_active_hermes_home", lambda: tmp_path) + # Also pin HERMES_HOME so code that reads it directly gets tmp_path, + # not the conftest session TEST_STATE_DIR that bleeds into the main process. + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) old_cfg = dict(config.cfg) old_mtime = config._cfg_mtime @@ -181,7 +191,7 @@ class TestSetProviderKey: # Verify .env file was written env_path = tmp_path / ".env" - assert env_path.exists() + assert env_path.exists(), f".env not written to {env_path}; HERMES_HOME={__import__('os').environ.get('HERMES_HOME')!r}" content = env_path.read_text() assert "ANTHROPIC_API_KEY=sk-ant-test-key-12345678" in content finally: