diff --git a/tests/conftest.py b/tests/conftest.py index b2e64300..c31fd26f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,7 @@ import os import sys +import tempfile +import time # Backend runs with `--app-dir backend`, so tests must do the same. _BACKEND = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "backend")) @@ -7,11 +9,185 @@ if _BACKEND not in sys.path: sys.path.insert(0, _BACKEND) +# ── Hermetic app state (issue #878) ──────────────────────────────────────── +# Tests must never read or write the developer's real app state. Without +# this, `core.config.DATA_DIR` resolves to the real per-user data dir +# (~/Library/Application Support/OmniVoice, %APPDATA%\OmniVoice, ~/.omnivoice) +# so prefs.json / omnivoice.db writes made by tests land in — and leak from — +# the developer's actual install, and a dev who used the app sees LLM tests +# fail that pass on clean CI. Redirecting here (before pytest imports any +# test module, which is what freezes DATA_DIR at `core.config` import time) +# makes every local run behave like a clean CI runner. `setdefault` semantics: +# an explicitly exported OMNIVOICE_DATA_DIR still wins. +if not os.environ.get("OMNIVOICE_DATA_DIR"): + os.environ["OMNIVOICE_DATA_DIR"] = tempfile.mkdtemp(prefix="omnivoice-test-data-") +# Same story for the durable per-user env file (~/.config/omnivoice/env): +# `main.py` loads it with override=True at import, so a TestClient importing +# the app mid-suite would inject the developer's real TRANSLATE_* / key vars +# into this process. `core.user_env` resolves OMNIVOICE_ENV_FILE at call +# time, so pointing it into the throwaway data dir neutralizes both the +# load and any test that writes user-env without stubbing. +if not os.environ.get("OMNIVOICE_ENV_FILE"): + os.environ["OMNIVOICE_ENV_FILE"] = os.path.join( + os.environ["OMNIVOICE_DATA_DIR"], "user-env" + ) + + # ── Test fixtures ────────────────────────────────────────────────────────── import pytest +# ── LLM-provider state isolation (issue #878) ────────────────────────────── +# LLM provider selection is process-global three ways: env vars (the +# resolution roots for llm_providers/llm_backend, and `main.py` import loads +# .env files straight into os.environ), the SQLite settings store +# (llm.active_provider / llm.base_url.* / encrypted llm_key.* secrets), and +# prefs.json (llm_backend pick, env.TRANSLATE_* persistence). Any test that +# mutates one of these without teardown — or merely imports `main` — used to +# change what *later* tests' `active_backend_id()` / `active_provider_id()` +# resolved to (order-dependent failures in test_engines.py, +# test_llm_endpoint_settings.py, test_llm_providers.py). The autouse guard +# below snapshots all three surfaces before every test and restores them +# exactly afterwards, making the whole class of leak impossible. + +# Env vars that are NOT declared on a Provider entry but still steer LLM / +# translation resolution. +_LLM_ENV_EXTRAS = ( + "LLM_DEFAULT_PROVIDER", # llm_providers.active_provider_id() override + "OMNIVOICE_LLM_BACKEND", # llm_backend.active_backend_id() override + "OMNIVOICE_LLM_TIMEOUT", + "TRANSLATE_PROVIDER", # dub translate default provider + "TRANSLATE_BASE_URL", + "TRANSLATE_API_KEY", + "TRANSLATE_MODEL", +) + +_llm_env_names_cache: tuple = () + + +def _llm_env_names() -> tuple: + """Every env var the LLM-provider registry resolves through. + + Derived from `services.llm_providers._PROVIDERS` so a newly added + provider is guarded automatically. Falls back to the static extras if + the import is unavailable (e.g. sys.modules stubbed by tests/backend/**); + only a successful full derivation is cached. + """ + global _llm_env_names_cache + if _llm_env_names_cache: + return _llm_env_names_cache + names = set(_LLM_ENV_EXTRAS) + try: + from services import llm_providers + for p in llm_providers.all_providers(): + names.update(p.key_envs) + for n in (p.base_url_env, p.model_env, p.account_env): + if n: + names.add(n) + except Exception: + return tuple(sorted(names)) # degraded, uncached — retry next test + _llm_env_names_cache = tuple(sorted(names)) + return _llm_env_names_cache + + +_LLM_STORE_SQL = ( + "SELECT key, value FROM settings " + "WHERE key LIKE 'llm.%' OR key LIKE 'secret.llm_key.%'" +) + + +def _llm_store_snapshot() -> dict: + """Raw llm.* / secret.llm_key.* rows (ciphertext included — no decrypt).""" + try: + from core.db import db_conn + with db_conn() as conn: + return {k: v for k, v in conn.execute(_LLM_STORE_SQL).fetchall()} + except Exception: + # Missing settings table / stubbed core.* — nothing to snapshot. + return {} + + +def _llm_store_restore(before: dict) -> None: + try: + from core.db import db_conn + with db_conn() as conn: + after = {k: v for k, v in conn.execute(_LLM_STORE_SQL).fetchall()} + if after == before: + return + for k in after.keys() - before.keys(): + conn.execute("DELETE FROM settings WHERE key = ?", (k,)) + for k, v in before.items(): + if after.get(k) != v: + conn.execute( + "INSERT OR REPLACE INTO settings(key, value, updated_at) " + "VALUES (?, ?, ?)", + (k, v, time.time()), + ) + except Exception: + pass # table never existed during the test → nothing leaked + + +def _llm_prefs_subset(data: dict) -> dict: + return { + k: v for k, v in data.items() + if k == "llm_backend" or k.startswith("env.TRANSLATE") + } + + +def _llm_prefs_snapshot() -> dict: + try: + from core import prefs + return _llm_prefs_subset(prefs._load()) + except Exception: + return {} + + +def _llm_prefs_restore(before: dict) -> None: + try: + from core import prefs + data = prefs._load() + current = _llm_prefs_subset(data) + if current == before: + return + for k in current.keys() - before.keys(): + data.pop(k, None) + data.update(before) + prefs._save(data) + except Exception: + pass + + +@pytest.fixture(autouse=True) +def _isolate_llm_provider_state(): + """Snapshot/restore the three global LLM-provider state surfaces per test.""" + names = _llm_env_names() + env_before = {n: os.environ.get(n) for n in names} + store_before = _llm_store_snapshot() + prefs_before = _llm_prefs_snapshot() + yield + for n, v in env_before.items(): + if os.environ.get(n) != v: + if v is None: + os.environ.pop(n, None) + else: + os.environ[n] = v + _llm_store_restore(store_before) + _llm_prefs_restore(prefs_before) + + +@pytest.fixture +def clean_llm_env(monkeypatch): + """Delete every LLM-provider env var for the duration of a test. + + For tests that assert on the *unconfigured* state (auto-select 'off', + empty endpoint settings, provider precedence): ambient shell exports or + a `.env` loaded by an earlier `main` import must not read as + 'something configured'. Restoration is monkeypatch's. + """ + for name in _llm_env_names(): + monkeypatch.delenv(name, raising=False) + @pytest.fixture def mock_settings_store(monkeypatch): diff --git a/tests/test_engines.py b/tests/test_engines.py index 30178d67..5310a246 100644 --- a/tests/test_engines.py +++ b/tests/test_engines.py @@ -113,10 +113,11 @@ def test_llm_off_chat_raises_actionable(monkeypatch): assert "TRANSLATE_BASE_URL" in str(ei.value) -def test_llm_auto_selects_off_when_nothing_configured(monkeypatch): - for var in ("OMNIVOICE_LLM_BACKEND", "TRANSLATE_BASE_URL", - "TRANSLATE_API_KEY", "OPENAI_API_KEY"): - monkeypatch.delenv(var, raising=False) +def test_llm_auto_selects_off_when_nothing_configured(clean_llm_env): + # clean_llm_env (conftest) clears the FULL provider env surface — a + # hand-picked 4-var list left e.g. LLM_DEFAULT_PROVIDER / GROQ_API_KEY + # standing when an earlier test imported `main` (which dotenv-loads the + # developer's .env into os.environ), reading as 'configured' (#878). assert llm_backend.active_backend_id() == "off" diff --git a/tests/test_llm_endpoint_settings.py b/tests/test_llm_endpoint_settings.py index d3096e18..165a4e1e 100644 --- a/tests/test_llm_endpoint_settings.py +++ b/tests/test_llm_endpoint_settings.py @@ -19,13 +19,14 @@ _HAS_OPENAI = importlib.util.find_spec("openai") is not None @pytest.fixture -def settings_mod(monkeypatch): +def settings_mod(monkeypatch, clean_llm_env): # Stub prefs persistence so PUT doesn't write the developer's prefs.json. + # clean_llm_env clears the full LLM-provider env surface (not just the + # TRANSLATE_* quartet) so 'empty state' really is empty even when an + # earlier `main` import dotenv-loaded provider keys into os.environ (#878). import core.prefs as prefs monkeypatch.setattr(prefs, "set_", lambda *a, **k: None) monkeypatch.setattr(prefs, "delete", lambda *a, **k: None) - for k in ("TRANSLATE_BASE_URL", "TRANSLATE_MODEL", "TRANSLATE_API_KEY", "OPENAI_API_KEY"): - monkeypatch.delenv(k, raising=False) return importlib.import_module("api.routers.settings") diff --git a/tests/test_llm_providers.py b/tests/test_llm_providers.py index 760d7659..8a717598 100644 --- a/tests/test_llm_providers.py +++ b/tests/test_llm_providers.py @@ -12,8 +12,13 @@ import pytest @pytest.fixture -def lp(monkeypatch): - """llm_providers with settings_store backed by in-memory dicts (no SQLite).""" +def lp(monkeypatch, clean_llm_env): + """llm_providers with settings_store backed by in-memory dicts (no SQLite). + + clean_llm_env (conftest) clears the FULL provider env surface — a partial + list left other providers' keys standing when an earlier `main` import + dotenv-loaded them into os.environ, breaking precedence asserts (#878). + """ from services import settings_store as ss from services import llm_providers as _lp @@ -25,10 +30,6 @@ def lp(monkeypatch): monkeypatch.setattr(ss, "get_secret", lambda n: secrets.get(n)) monkeypatch.setattr(ss, "set_secret", lambda n, v: secrets.__setitem__(n, v) if v else secrets.pop(n, None)) monkeypatch.setattr(ss, "list_secret_names", lambda: list(secrets)) - # Clean env of anything that would leak in as an override. - for var in ("LLM_DEFAULT_PROVIDER", "TRANSLATE_BASE_URL", "TRANSLATE_API_KEY", - "TRANSLATE_MODEL", "OPENAI_API_KEY", "GROQ_API_KEY", "GROQ_MODEL"): - monkeypatch.delenv(var, raising=False) _lp._text, _lp._secrets = text, secrets # handles for the test to seed return _lp diff --git a/tests/test_llm_providers_router.py b/tests/test_llm_providers_router.py index a3710166..99f30f93 100644 --- a/tests/test_llm_providers_router.py +++ b/tests/test_llm_providers_router.py @@ -26,8 +26,12 @@ pytestmark = pytest.mark.skipif(not _HAS_OPENAI, reason="openai package not inst @pytest.fixture -def settings_mod(monkeypatch): - """Router module with settings_store in-memory (no SQLite, no prefs I/O).""" +def settings_mod(monkeypatch, clean_llm_env): + """Router module with settings_store in-memory (no SQLite, no prefs I/O). + + clean_llm_env (conftest) clears the FULL provider env surface so probes + resolve only the seeded in-memory state, not ambient/.env keys (#878). + """ from services import settings_store as ss text: dict[str, str] = {} @@ -37,9 +41,6 @@ def settings_mod(monkeypatch): monkeypatch.setattr(ss, "get_secret", lambda n: secrets.get(n)) monkeypatch.setattr(ss, "set_secret", lambda n, v: secrets.__setitem__(n, v) if v else secrets.pop(n, None)) monkeypatch.setattr(ss, "list_secret_names", lambda: list(secrets)) - for var in ("LLM_DEFAULT_PROVIDER", "TRANSLATE_BASE_URL", "TRANSLATE_API_KEY", - "TRANSLATE_MODEL", "OPENAI_API_KEY", "GROQ_API_KEY", "GROQ_MODEL"): - monkeypatch.delenv(var, raising=False) import importlib return importlib.import_module("api.routers.settings") diff --git a/tests/test_llm_state_isolation.py b/tests/test_llm_state_isolation.py new file mode 100644 index 00000000..3f2b28bf --- /dev/null +++ b/tests/test_llm_state_isolation.py @@ -0,0 +1,95 @@ +"""Issue #878 — LLM-provider state must not leak between tests. + +LLM provider selection reads three process-global surfaces: env vars +(LLM_DEFAULT_PROVIDER, per-provider *_API_KEY / *_BASE_URL, TRANSLATE_*), +the SQLite settings store (llm.active_provider & co.), and prefs.json +(llm_backend). A test that mutates any of them without teardown — or that +merely imports `main` (its dotenv load injects the developer's .env / +~/.config/omnivoice/env into os.environ) — used to flip what later tests' +`active_backend_id()` / `active_provider_id()` resolved to. Reported repro: + + uv run pytest tests/test_generate_engine.py::test_generate_default_path_still_runs_omnivoice \ + tests/test_engines.py::test_llm_auto_selects_off_when_nothing_configured -q + # → assert 'openai-compat' == 'off' + +The pair below reproduces the whole class deterministically (pytest runs +tests in definition order within a file): the first test pollutes all three +surfaces on purpose and "forgets" to clean up; the second asserts the +`_isolate_llm_provider_state` autouse guard in tests/conftest.py restored +every surface to its pre-test baseline. Fails without the guard. +""" +import os + +os.environ.setdefault("OMNIVOICE_MODEL", "test") +os.environ.setdefault("OMNIVOICE_DISABLE_FILE_LOG", "1") + +import pytest + +# A cross-section of the guarded env surface: selection overrides, a provider +# key, and the legacy single-endpoint vars. Baselines are captured lazily in +# the polluting test (module import happens at collection time; the ambient +# values at test start are what the guard restores to). +_ENV_VARS = ( + "LLM_DEFAULT_PROVIDER", + "OMNIVOICE_LLM_BACKEND", + "GROQ_API_KEY", + "TRANSLATE_BASE_URL", + "TRANSLATE_API_KEY", +) + +_baseline: dict = {} + + +def _store_active_provider(): + from services import settings_store + return settings_store.get_text("llm.active_provider") + + +def _prefs_llm_backend(): + from core import prefs + return prefs.get("llm_backend") + + +def test_pollute_llm_state_without_cleanup(): + """Deliberately leak on every surface — no monkeypatch, no teardown.""" + from core.db import ensure_schema + from core import prefs + from services import settings_store + + ensure_schema() # settings table must exist for the store write + + _baseline["env"] = {n: os.environ.get(n) for n in _ENV_VARS} + _baseline["store"] = _store_active_provider() + _baseline["prefs"] = _prefs_llm_backend() + + os.environ["LLM_DEFAULT_PROVIDER"] = "groq" + os.environ["OMNIVOICE_LLM_BACKEND"] = "openai-compat" + os.environ["GROQ_API_KEY"] = "gsk_leaked_by_test" + os.environ["TRANSLATE_BASE_URL"] = "http://leak:11434/v1" + os.environ["TRANSLATE_API_KEY"] = "leaked" + settings_store.set_text("llm.active_provider", "groq") + prefs.set_("llm_backend", "openai-compat") + + # Sanity: the pollution really is visible inside the offending test. + assert os.environ["GROQ_API_KEY"] == "gsk_leaked_by_test" + assert _store_active_provider() == "groq" + assert _prefs_llm_backend() == "openai-compat" + + +def test_llm_state_restored_after_polluting_test(): + """The autouse guard must have restored env, store, and prefs exactly.""" + if "env" not in _baseline: + pytest.skip("baseline unavailable — polluting test did not run first") + + leaked = { + n: os.environ.get(n) + for n in _ENV_VARS + if os.environ.get(n) != _baseline["env"][n] + } + assert not leaked, f"env vars leaked across tests: {leaked}" + assert _store_active_provider() == _baseline["store"], ( + "settings store llm.active_provider leaked across tests" + ) + assert _prefs_llm_backend() == _baseline["prefs"], ( + "prefs.json llm_backend leaked across tests" + )