diff --git a/api/config.py b/api/config.py index 5c9294f9..9f088229 100644 --- a/api/config.py +++ b/api/config.py @@ -7926,20 +7926,48 @@ def _active_profile_home() -> Path: def _models_cache_env_fingerprint(path: Path) -> list: - """Sorted non-empty ``.env`` key names, parsed like provider detection; values never recorded.""" + """``[key, HMAC(value)]`` per non-empty ``.env`` entry, parsed like provider detection. + + Values are keyed-hashed with the WebUI signing key so the cache file never holds a secret. + """ + import hmac + from api.auth import _signing_key from api.providers import _load_env_file - return sorted(k for k, v in _load_env_file(Path(path).expanduser()).items() if v) + key = _signing_key() + return [ + [k, hmac.new(key, v.encode("utf-8"), hashlib.sha256).hexdigest()] + for k, v in sorted(_load_env_file(Path(path).expanduser()).items()) + if v + ] def _declares_model_provider_kind(plugin_dir: Path) -> bool: - for manifest in (plugin_dir / "plugin.yaml", plugin_dir / "plugin.yml"): - try: - lines = manifest.read_text(encoding="utf-8", errors="replace").splitlines() - except OSError: + # Same parse as the agent's providers._declares_model_provider_kind: PyYAML, then a line scan. + for filename in ("plugin.yaml", "plugin.yml"): + manifest = plugin_dir / filename + if not manifest.is_file(): continue - pairs = (line.partition(":") for line in lines if not line[:1].isspace()) - return any(k.strip() == "kind" and v.strip().strip("\"'") == "model-provider" for k, _, v in pairs) + try: + text = manifest.read_text(encoding="utf-8", errors="replace") + except Exception: + return False + try: + import yaml as _yaml + + data = _yaml.safe_load(text) + if isinstance(data, dict): + return str(data.get("kind", "")).strip() == "model-provider" + except Exception: + pass + for line in text.splitlines(): + stripped = line.strip() + if stripped.startswith("#") or ":" not in stripped: + continue + key, _, value = stripped.partition(":") + if key.strip() == "kind": + return value.strip().strip("\"'") == "model-provider" + return False return False @@ -8148,10 +8176,10 @@ def _load_stale_models_cache_from_disk() -> dict | None: The main cache loader enforces metadata stamps for a full cold-path cache hit. This helper intentionally does not apply that stricter policy, so we can still recover a useful fallback payload when the strict loader rejected cache because - metadata or fingerprint fields are stale. It DOES still enforce the schema - version: a cross-schema cache can have an incompatible groups/badge shape, so - serving it to the picker could surface a broken catalog — schema mismatch is a - hard reject even on the fallback path. + the WebUI version stamp is stale. It DOES still enforce the schema version (a + cross-schema cache can have an incompatible groups/badge shape) and the source + fingerprint: a snapshot built from other config/auth/.env/plugin sources is a + wrong catalog, not merely an old one, so it is never served. """ try: import json as _j @@ -8165,6 +8193,8 @@ def _load_stale_models_cache_from_disk() -> dict | None: return None if cache.get("_schema_version") != _MODELS_CACHE_SCHEMA_VERSION: return None + if cache.get("_source_fingerprint") != _models_cache_source_fingerprint(): + return None aliases = cache.get("aliases") if not isinstance(aliases, dict): # The disk cache save path does not persist `aliases`, so a cache diff --git a/docs/architecture/models-cache-invalidation.md b/docs/architecture/models-cache-invalidation.md index 6f0398b1..b28e19a1 100644 --- a/docs/architecture/models-cache-invalidation.md +++ b/docs/architecture/models-cache-invalidation.md @@ -31,10 +31,13 @@ the fingerprint captured at publish time. | --- | --- | --- | | `config_yaml` | stat identity: `mtime_ns` + size (`_models_cache_file_fingerprint`) | The file is rewritten only on deliberate user edits, and any edit can change the provider/model set, so the cheap conservative identity wins. | | `auth_json` | content hash with a volatile-key deny-list (`_auth_store_semantic_fingerprint`, `_AUTH_FINGERPRINT_VOLATILE_KEYS`) | The credential store is rewritten roughly every 14 minutes by credential-pool / OAuth refresh; none of those rotating fields feed `detected_providers` or the returned catalog, and stat identity made the 24h cache churn on every refresh (RCA `t_d127953d` / `t_16551f61`). | -| `env` | sorted **names** of non-empty `.env` keys, parsed by `providers._load_env_file` (`_models_cache_env_fingerprint`); values are never recorded | Env credentials decide `detected_providers`; rotating a value does not. | -| `plugins` | `[relpath, mtime_ns, size]` of every non-bytecode file of each model-provider plugin, discovered like `providers._scan_home_layer` (`_models_cache_plugin_fingerprint`) | The loader execs plugin code that builds `fallback_models`, so any file edit must invalidate, even without a `version` bump. | +| `env` | `[key, HMAC-SHA256(signing key, value)]` per non-empty `.env` entry, parsed by `providers._load_env_file` (`_models_cache_env_fingerprint`); plaintext values are never recorded | Env keys decide `detected_providers` and values such as `LM_BASE_URL` decide which endpoint is probed, so both key and value changes invalidate. | +| `plugins` | `[relpath, mtime_ns, size]` of every non-bytecode file of each model-provider plugin, discovered like `providers._scan_home_layer`, flat manifests parsed with PyYAML like the agent (`_models_cache_plugin_fingerprint`) | The loader execs plugin code that builds `fallback_models`, so any file edit must invalidate, even without a `version` bump. | | `catalog` | baked-in provider catalog sha256 (`_PROVIDER_MODELS` + `_PROVIDER_DISPLAY`) plus the Codex local catalog (`_codex_models_cache_fingerprint`, `_CODEX_CACHE_FINGERPRINT_VOLATILE_KEYS`) | A restart after a catalog change must not keep serving a persisted payload for up to 24h (#2443). Codex rewrites `~/.codex/models_cache.json` on its own timer, bumping `mtime_ns` and size while models, `etag`, and `client_version` stay identical, so the Codex axis hashes **content** with only the refresh timestamps (`fetched_at`, `updated_at`) removed (#7540, #7556). | +The over-budget stale fallback (`_load_stale_models_cache_from_disk`) tolerates a stale +`_webui_version` but never a source-fingerprint mismatch: such a snapshot is a wrong catalog. + ## Invariant: deny-lists are one-directional - Both volatile-key sets are deny-lists, never allow-lists. They may remove only diff --git a/tests/test_issue3928_models_budget_fallback.py b/tests/test_issue3928_models_budget_fallback.py index 28f030e5..bc04d9d6 100644 --- a/tests/test_issue3928_models_budget_fallback.py +++ b/tests/test_issue3928_models_budget_fallback.py @@ -136,16 +136,12 @@ def _build_stale_disk_cache_payload() -> dict: "aliases": { "chat": "ollama-cloud/chat-1", }, - # Current schema (so the stale-cache loader's schema guard accepts it), - # but a deliberately stale _webui_version + fingerprint so the STRICT - # loader rejects it — this is exactly the "recoverable stale cache" case. + # Current schema and sources, but a stale _webui_version so the STRICT loader + # rejects it: the "recoverable stale cache" case. A source-fingerprint + # mismatch is a wrong catalog and is covered by the rejection test below. "_schema_version": cfg._MODELS_CACHE_SCHEMA_VERSION, "_webui_version": "v999", - "_source_fingerprint": { - "catalog": "stale", - "config_yaml": {"size": 42}, - "auth_json": {"size": 7}, - }, + "_source_fingerprint": cfg._models_cache_source_fingerprint(), } @@ -401,6 +397,19 @@ def test_load_stale_models_cache_from_disk_rejects_cross_schema( assert cfg._load_stale_models_cache_from_disk() is None +def test_load_stale_models_cache_from_disk_rejects_source_fingerprint_mismatch( + monkeypatch, + isolate_models_catalog_state, +): + payload = _build_stale_disk_cache_payload() + payload["_source_fingerprint"] = "other-sources" + models_cache_path = isolate_models_catalog_state["models_cache_path"] + monkeypatch.setattr(cfg, "_get_models_cache_path", lambda: models_cache_path) + models_cache_path.write_text(json.dumps(payload), encoding="utf-8") + + assert cfg._load_stale_models_cache_from_disk() is None + + def test_load_stale_models_cache_reconstructs_aliases_from_config( monkeypatch, isolate_models_catalog_state, diff --git a/tests/test_profile_switch_models_disk_cache.py b/tests/test_profile_switch_models_disk_cache.py index abfe0821..478706e0 100644 --- a/tests/test_profile_switch_models_disk_cache.py +++ b/tests/test_profile_switch_models_disk_cache.py @@ -5,6 +5,7 @@ from __future__ import annotations import os import shutil import sys +import threading from pathlib import Path from types import SimpleNamespace @@ -32,7 +33,6 @@ def env(tmp_path: Path, monkeypatch): monkeypatch.setattr(profiles, "_is_isolated_profile_mode", lambda: False) monkeypatch.setattr(profiles._tls, "profile", None, raising=False) monkeypatch.setattr(cfg, "_models_cache_path", tmp_path / "models_cache.json") - monkeypatch.setattr(cfg, "_load_stale_models_cache_from_disk", lambda: None) monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _builder: _catalog("fresh-model")) for attr in ("_cfg_mtime", "_cfg_path", "_cfg_fingerprint"): monkeypatch.setattr(cfg, attr, getattr(cfg, attr), raising=False) @@ -74,10 +74,10 @@ def _switch_to_demo_and_fetch() -> str: return _fetch_as_demo()["default_model"] -def _write_plugin(home: Path, version: str = "1.0.0", *, flat: bool = False, code: str | None = None, sub: str | None = None) -> Path: +def _write_plugin(home: Path, version: str = "1.0.0", *, flat: bool = False, code: str | None = None, sub: str | None = None, kind: str = "model-provider") -> Path: d = home / "plugins" / ("" if flat else "model-providers") / "acme" d.mkdir(parents=True, exist_ok=True) - (d / "plugin.yaml").write_text(f"name: acme\nkind: model-provider\nversion: {version}\n", encoding="utf-8") + (d / "plugin.yaml").write_text(f"name: acme\nkind: {kind}\nversion: {version}\n", encoding="utf-8") for name, text in (("__init__.py", code), ("models.py", sub)): if text is not None: (d / name).write_text(text, encoding="utf-8") @@ -92,13 +92,15 @@ def _env(text: str): # (id, source state before the snapshot is saved, change made after it, model served after the switch) _CASES = [ ("unchanged", None, None, "snapshot-model"), - ("env-value-rotated", _env("DEEPSEEK_API_KEY=sk-one\n"), _env("DEEPSEEK_API_KEY=sk-two\n"), "snapshot-model"), + ("env-value-rotated", _env("DEEPSEEK_API_KEY=sk-one\n"), _env("DEEPSEEK_API_KEY=sk-two\n"), "fresh-model"), + ("env-base-url-changed", _env("LM_BASE_URL=http://127.0.0.1:1234/v1\n"), _env("LM_BASE_URL=http://127.0.0.1:5678/v1\n"), "fresh-model"), ("env-key-added", None, _env("DEEPSEEK_API_KEY=sk-new\n"), "fresh-model"), ("env-key-removed", _env("DEEPSEEK_API_KEY=sk-old\nOTHER=1\n"), _env("OTHER=1\n"), "fresh-model"), # provider detection reads `export KEY=` as key `export KEY`, so the credential is gone ("env-export-prefix", _env("DEEPSEEK_API_KEY=sk-old\n"), _env("export DEEPSEEK_API_KEY=sk-old\n"), "fresh-model"), ("plugin-installed", None, lambda h: _write_plugin(h), "fresh-model"), ("plugin-installed-flat", None, lambda h: _write_plugin(h, flat=True), "fresh-model"), + ("plugin-installed-flat-yaml-comment", None, lambda h: _write_plugin(h, flat=True, kind="model-provider # valid YAML comment"), "fresh-model"), ("plugin-removed", lambda h: _write_plugin(h), lambda h: shutil.rmtree(h / "plugins"), "fresh-model"), ("plugin-version-bumped", lambda h: _write_plugin(h, flat=True), lambda h: _write_plugin(h, "2.0.0", flat=True), "fresh-model"), ("plugin-code-edited", lambda h: _write_plugin(h, code="M = (1,)\n"), lambda h: (h / "plugins/model-providers/acme/__init__.py").write_text("M = (1, 2)\n"), "fresh-model"), @@ -118,6 +120,52 @@ def test_switch_serves_snapshot_only_while_sources_are_unchanged(env, before, ch assert "sk-" not in env.cache.read_text(encoding="utf-8") # the fingerprint never records a secret +@pytest.mark.parametrize("change", [_env("LM_BASE_URL=http://127.0.0.1:5678/v1\n"), lambda h: _write_plugin(h, flat=True)], ids=["env", "plugin"]) +def test_slow_rebuild_never_serves_fingerprint_mismatched_snapshot_as_stale_fallback(env, monkeypatch, change): + _env("LM_BASE_URL=http://127.0.0.1:1234/v1\n")(env.home) + _save_demo_snapshot(env) + change(env.home) + profiles.set_request_profile("demo") + try: + assert cfg._load_models_cache_from_disk() is None + assert cfg._load_stale_models_cache_from_disk() is None + finally: + profiles.clear_request_profile() + release = threading.Event() + monkeypatch.setattr(cfg, "_LIVE_REBUILD_BUDGET_SECONDS", 0.05, raising=False) + monkeypatch.setattr(cfg, "_invoke_models_rebuild", lambda _b: release.wait(5) and _catalog("fresh-model")) + try: + assert _switch_to_demo_and_fetch() != "snapshot-model" + finally: + release.set() + + +def test_stale_fallback_still_serves_snapshot_when_only_webui_version_differs(env, monkeypatch): + _save_demo_snapshot(env) + monkeypatch.setattr(cfg, "_current_webui_version", lambda: "v-other") + profiles.set_request_profile("demo") + try: + assert cfg._load_models_cache_from_disk() is None + assert cfg._load_stale_models_cache_from_disk()["default_model"] == "snapshot-model" + finally: + profiles.clear_request_profile() + + +@pytest.mark.parametrize("manifest", [ + "kind: model-provider # valid YAML comment\n", + "kind: model-provider\n", + "kind: 'model-provider'\n", + "name: x\nkind: tool\n", + "meta:\n kind: model-provider\n", + "# kind: model-provider\n", + "kind: [unclosed\nkind: model-provider\n", +]) +def test_manifest_kind_parse_matches_agent(tmp_path, manifest): + agent = pytest.importorskip("providers") + (tmp_path / "plugin.yaml").write_text(manifest, encoding="utf-8") + assert cfg._declares_model_provider_kind(tmp_path) == agent._declares_model_provider_kind(tmp_path) + + def test_same_profile_config_edit_still_deletes_disk_cache(env, monkeypatch): _save_demo_snapshot(env) assert _switch_to_demo_and_fetch() == "snapshot-model" @@ -147,7 +195,17 @@ def test_env_fingerprint_keys_match_provider_env_loader(env): _env("export A=1\nB='x'\nC=\n# D=1\n")(env.home) loaded = sorted(k for k, v in _load_env_file(env.home / ".env").items() if v) - assert cfg._models_cache_env_fingerprint(env.home / ".env") == loaded + fp = cfg._models_cache_env_fingerprint(env.home / ".env") + assert [k for k, _ in fp] == loaded + assert all(len(h) == 64 and h not in ("1", "x") for _, h in fp) + + +def test_env_fingerprint_is_keyed_not_a_plain_value_hash(env): + import hashlib + + _env("DEEPSEEK_API_KEY=sk-secret\n")(env.home) + (_, digest), = cfg._models_cache_env_fingerprint(env.home / ".env") + assert digest != hashlib.sha256(b"sk-secret").hexdigest() def test_plugin_bytecode_cache_does_not_churn_fingerprint(env):