test: make LLM-provider state leaks between tests impossible (#878) (#894)

Root cause: 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). Importing `main` (TestClient fixtures do)
dotenv-loads the developer's .env and ~/.config/omnivoice/env straight
into os.environ, and several tests/endpoints mutate these surfaces
without teardown — so whichever test imported the app first flipped 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).

Fix the class, not the instances:
- tests/conftest.py: redirect OMNIVOICE_DATA_DIR to a per-session tmp dir
  and OMNIVOICE_ENV_FILE into it (before collection freezes
  core.config.DATA_DIR), so tests never read or write the developer's
  real app state and local runs behave like clean CI.
- tests/conftest.py: autouse `_isolate_llm_provider_state` fixture
  snapshots env (derived from llm_providers._PROVIDERS, so new providers
  are guarded automatically), llm.* / secret.llm_key.* settings rows, and
  the prefs llm_backend/env.TRANSLATE* keys before every test and
  restores them exactly afterwards.
- shared `clean_llm_env` fixture clears the FULL provider env surface;
  the four LLM test modules' hand-picked partial delenv lists (which left
  e.g. LLM_DEFAULT_PROVIDER / OPENROUTER_API_KEY standing) now use it.
- tests/test_llm_state_isolation.py: deterministic fail-before/pass-after
  regression pair — pollutes all three surfaces without cleanup, then
  asserts the guard restored them.

Verified: the issue's two-test repro passes; the five LLM-related test
files pass in order; full suite green (2046 passed, 20 skipped,
10 xfailed, 4 xpassed).

Fixes #878

Co-authored-by: mergetest <test@local>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

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):

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"
 
 

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")
 
 

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
 

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")
 

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"
+    )