diff --git a/src/conclave/config.py b/src/conclave/config.py index bd2a816..cced930 100644 --- a/src/conclave/config.py +++ b/src/conclave/config.py @@ -104,8 +104,46 @@ def _read_yaml(path: Path) -> dict[str, Any]: return {} +def _resolve_path(path: Path | None) -> Path: + """Resolve the effective config path from an explicit arg or the environment.""" + if path is not None: + return path + env_path = os.environ.get("CONCLAVE_CONFIG") + return Path(env_path) if env_path else DEFAULT_CONFIG_PATH + + +def _cache_key(path: Path) -> tuple[str, float]: + """A (path, mtime) cache key. mtime is 0.0 when the file is absent. + + Keying on mtime means an edited or newly created config is picked up on the + next call, while an unchanged file is served from the in-process cache -- + eliminating the repeated disk read + YAML parse that ``call_model`` used to + incur on every single model call (issue #15), without ever serving stale data. + """ + try: + return (str(path), path.stat().st_mtime) + except OSError: + return (str(path), 0.0) + + +# In-process memo: (path, mtime) -> merged config. Cleared automatically when the +# underlying file changes (mtime moves) and explicitly via ``clear_config_cache``. +_CONFIG_CACHE: dict[tuple[str, float], ConclaveConfig] = {} + + +def clear_config_cache() -> None: + """Drop the memoized config. Intended for tests and long-lived processes.""" + _CONFIG_CACHE.clear() + + def load_config(path: Path | None = None) -> ConclaveConfig: - """Load and merge configuration. + """Load and merge configuration, memoized by (path, mtime). + + The result is cached in-process keyed on the resolved path and its + modification time, so repeated calls within a run (e.g. one per model call) + do not re-read disk or re-parse YAML. The cache self-invalidates when the + file's mtime changes, preserving the previous always-fresh behavior across + edits while removing the redundant hot-path reads (issue #15). Args: path: Optional override path. Defaults to ``~/.conclave/config.yml`` or @@ -115,10 +153,19 @@ def load_config(path: Path | None = None) -> ConclaveConfig: A fully merged ``ConclaveConfig``. Built-in model defaults are always present; file entries override or extend them. """ - if path is None: - env_path = os.environ.get("CONCLAVE_CONFIG") - path = Path(env_path) if env_path else DEFAULT_CONFIG_PATH + resolved = _resolve_path(path) + key = _cache_key(resolved) + cached = _CONFIG_CACHE.get(key) + if cached is not None: + return cached + + config = _load_config_uncached(resolved) + _CONFIG_CACHE[key] = config + return config + +def _load_config_uncached(path: Path) -> ConclaveConfig: + """Read + merge config from ``path`` with no caching (the real disk work).""" raw = _read_yaml(path) merged_models = dict(DEFAULT_MODELS) diff --git a/src/conclave/providers.py b/src/conclave/providers.py index 6e84400..de5c72e 100644 --- a/src/conclave/providers.py +++ b/src/conclave/providers.py @@ -49,6 +49,7 @@ async def call_model( *, temperature: float = 0.7, timeout: float = 120.0, + config: ConclaveConfig | None = None, ) -> ModelAnswer: """Call a single model and return a structured :class:`ModelAnswer`. @@ -62,6 +63,12 @@ async def call_model( messages: OpenAI-style message list. temperature: Sampling temperature. timeout: Per-call timeout in seconds. + config: Pre-resolved config to use for adapter resolution (custom + ``endpoints:``). A caller that already holds the config -- e.g. + ``Council`` -- threads it in so this hot path does not re-read it. + When ``None`` (a standalone call) the config is resolved via the + memoized :func:`conclave.config.load_config`, so even repeated + standalone calls avoid redundant disk reads (issue #15). Returns: A ``ModelAnswer`` with either ``answer`` populated or ``error`` set. @@ -69,10 +76,11 @@ async def call_model( started = time.perf_counter() # Resolve the adapter (config-aware for user-declared custom endpoints). A - # bad/unknown provider id surfaces as a clean, non-raising error. + # bad/unknown provider id surfaces as a clean, non-raising error. Prefer the + # injected config; fall back to the memoized loader only when called standalone. try: - config: ConclaveConfig = load_config() - adapter = resolve_adapter(model_id, config) + resolved_config = config if config is not None else load_config() + adapter = resolve_adapter(model_id, resolved_config) except ProviderError as exc: latency = time.perf_counter() - started logger.warning("%s (%s) unresolved: %s", name, model_id, exc) diff --git a/src/conclave/registry.py b/src/conclave/registry.py index 641704f..59d4ddd 100644 --- a/src/conclave/registry.py +++ b/src/conclave/registry.py @@ -1,14 +1,69 @@ -"""Provider registry: friendly names, LiteLLM model ids, and key presence. - -This module is the single source of truth for which environment variable a -given provider needs. It NEVER reads or returns a key value -- only whether the -relevant variable is set and non-empty. That keeps secrets out of every code -path and out of any serialized output. +"""Provider registry: the single source of truth for built-in provider metadata. + +This module owns, in ONE place, everything that defines a built-in provider: + +* its LiteLLM provider prefix, +* the env var(s) that satisfy its key, +* (for OpenAI-compatible providers) its full ``/chat/completions`` URL, +* the friendly-name -> default model id mapping. + +Previously the env-var names lived here while the OpenAI-compatible URLs lived in +``adapters.openai_compat.OPENAI_COMPAT_URLS``. The two tables could silently +drift: a URL present without a matching env var (or vice versa) surfaced only as a +runtime ``KeyError`` deep in the call path. Now the per-provider metadata is +declared once (see :data:`OPENAI_COMPAT_PROVIDERS` and :data:`NATIVE_PROVIDERS`) +and the derived tables (:data:`PROVIDER_ENV_VARS`) are built from it, so adding or +removing a provider can never desync. A module-level consistency check +(:func:`_assert_metadata_consistent`) runs at import and fails loudly if the +adapter layer's URL table ever drifts from this source of truth -- turning a +silent per-call KeyError into an immediate, diagnosable startup error. + +It NEVER reads or returns a key value -- only whether the relevant variable is +set and non-empty. That keeps secrets out of every code path and out of any +serialized output. + +### Adding a built-in provider (single edit step) + +* **OpenAI-compatible provider** -> add one entry to + :data:`OPENAI_COMPAT_PROVIDERS` (prefix -> URL + env vars) **and** the matching + URL to ``adapters.openai_compat.OPENAI_COMPAT_URLS``. The import-time check + guarantees you cannot forget one half. +* **Native (non-compatible) provider** -> add its env var(s) to + :data:`NATIVE_PROVIDERS` and register its adapter in + ``adapters._NATIVE_BUILDERS``. +* **Zero-code custom provider** -> declare it under ``endpoints:`` in + ``~/.conclave/config.yml``; no registry edit needed. """ from __future__ import annotations import os +from dataclasses import dataclass + + +class RegistryError(RuntimeError): + """Raised at import time when provider metadata is internally inconsistent. + + A loud, immediate failure here replaces the former silent drift between the + env-var table and the OpenAI-compatible URL table, which used to escape as a + runtime ``KeyError`` deep inside the per-call adapter resolution. + """ + + +@dataclass(frozen=True) +class OpenAICompatProvider: + """Authoritative metadata for one built-in OpenAI-compatible provider. + + Attributes: + completions_url: Full POST URL of the provider's ``/chat/completions`` + endpoint. + env_vars: Candidate env var NAMES (never values); the first present one is + the active key. Order matters for fallbacks. + """ + + completions_url: str + env_vars: tuple[str, ...] + # Friendly name -> default LiteLLM model id. Overridable via ~/.conclave/config.yml. DEFAULT_MODELS: dict[str, str] = { @@ -19,19 +74,96 @@ "openai": "openai/gpt-4.1", } -# LiteLLM provider prefix -> the env var(s) that satisfy it. The first present -# var in the list is considered the active key. Order matters for fallbacks. +# SINGLE SOURCE OF TRUTH for built-in OpenAI-compatible providers: prefix -> +# (URL + env vars) in one place. The adapter layer's OPENAI_COMPAT_URLS is kept in +# lockstep with this table by the import-time consistency check below. +OPENAI_COMPAT_PROVIDERS: dict[str, OpenAICompatProvider] = { + "openai": OpenAICompatProvider( + completions_url="https://api.openai.com/v1/chat/completions", + env_vars=("OPENAI_API_KEY",), + ), + "xai": OpenAICompatProvider( + completions_url="https://api.x.ai/v1/chat/completions", + env_vars=("XAI_API_KEY",), + ), + # Perplexity has NO /v1 segment; that detail lives here, once. + "perplexity": OpenAICompatProvider( + completions_url="https://api.perplexity.ai/chat/completions", + env_vars=("PERPLEXITY_API_KEY",), + ), +} + +# Native (non OpenAI-compatible) providers: prefix -> candidate env var names. +# These have bespoke adapters in adapters._NATIVE_BUILDERS rather than a URL here. +NATIVE_PROVIDERS: dict[str, tuple[str, ...]] = { + "gemini": ("GEMINI_API_KEY", "GOOGLE_API_KEY"), + "anthropic": ("ANTHROPIC_API_KEY",), +} + +# Derived: LiteLLM provider prefix -> the env var(s) that satisfy it. Built from +# the single-source tables above so it can never drift from them. The first +# present var in the list is the active key. Order matters for fallbacks. PROVIDER_ENV_VARS: dict[str, list[str]] = { - "xai": ["XAI_API_KEY"], - "gemini": ["GEMINI_API_KEY", "GOOGLE_API_KEY"], - "anthropic": ["ANTHROPIC_API_KEY"], - "perplexity": ["PERPLEXITY_API_KEY"], - "openai": ["OPENAI_API_KEY"], + **{prefix: list(meta.env_vars) for prefix, meta in OPENAI_COMPAT_PROVIDERS.items()}, + **{prefix: list(env_vars) for prefix, env_vars in NATIVE_PROVIDERS.items()}, } DEFAULT_SYNTHESIZER = "claude" +def _assert_metadata_consistent() -> None: + """Fail loudly at import if provider metadata has drifted. + + Two invariants are enforced: + + 1. The adapter layer's ``OPENAI_COMPAT_URLS`` must have exactly the same + prefixes as :data:`OPENAI_COMPAT_PROVIDERS`, with identical URLs. This is + the drift that used to escape as a runtime ``KeyError`` in + ``adapters._openai_compat_adapter`` (issue #19). + 2. Every prefix in :data:`OPENAI_COMPAT_PROVIDERS` must declare at least one + env var, so an OpenAI-compatible provider can never be half-declared. + + Raises: + RegistryError: with a message naming the drifting prefixes / URLs. + """ + # Imported lazily inside the function to avoid any import-order coupling: + # openai_compat does not import registry, so this edge is safe and acyclic. + from .adapters.openai_compat import OPENAI_COMPAT_URLS + + source_prefixes = set(OPENAI_COMPAT_PROVIDERS) + adapter_prefixes = set(OPENAI_COMPAT_URLS) + + missing_in_adapter = source_prefixes - adapter_prefixes + missing_in_source = adapter_prefixes - source_prefixes + if missing_in_adapter or missing_in_source: + raise RegistryError( + "OpenAI-compatible provider tables have drifted: " + f"prefixes only in registry.OPENAI_COMPAT_PROVIDERS={sorted(missing_in_adapter)}, " + f"prefixes only in adapters.OPENAI_COMPAT_URLS={sorted(missing_in_source)}. " + "Add the missing entry to both (registry is the source of truth)." + ) + + mismatched_urls = { + prefix: (OPENAI_COMPAT_PROVIDERS[prefix].completions_url, OPENAI_COMPAT_URLS[prefix]) + for prefix in source_prefixes + if OPENAI_COMPAT_PROVIDERS[prefix].completions_url != OPENAI_COMPAT_URLS[prefix] + } + if mismatched_urls: + raise RegistryError( + "OpenAI-compatible URL drift between registry and adapters: " + f"{mismatched_urls} (registry is the source of truth)." + ) + + half_declared = [ + prefix for prefix, meta in OPENAI_COMPAT_PROVIDERS.items() if not meta.env_vars + ] + if half_declared: + raise RegistryError( + f"OpenAI-compatible providers declare a URL but no env var: {half_declared}. " + "Every provider needs at least one env var name." + ) + + def provider_prefix(model_id: str) -> str: """Extract the LiteLLM provider prefix from a model id. @@ -75,3 +207,10 @@ def key_source(model_id: str) -> str | None: if os.environ.get(var, "").strip(): return var return None + + +# Enforce the single-source-of-truth invariant at import time. Any drift between +# this registry and the adapter layer's URL table is a programming error in a +# provider definition; surfacing it here (loudly, once) is strictly better than +# letting it escape as a per-call KeyError deep in the call path (issue #19). +_assert_metadata_consistent() diff --git a/tests/test_council.py b/tests/test_council.py index c916a1c..2152860 100644 --- a/tests/test_council.py +++ b/tests/test_council.py @@ -222,3 +222,49 @@ async def test_ask_sync_raises_inside_loop(monkeypatch): council = Council(models=["grok"], config=_config()) with pytest.raises(RuntimeError, match="running event loop"): council.ask_sync("hi") + + +async def test_config_disk_read_at_most_once_per_ask(monkeypatch, tmp_path): + """A full Council.ask run hits the config file on disk at most once (issue #15). + + Exercises the REAL call path (transport patched, not call_model) so every + member call plus synthesis flows through providers.call_model -> load_config. + With the memoized loader, the underlying disk read happens at most once across + the whole run rather than once per model call. + """ + import conclave.config as config_mod + + _all_keys(monkeypatch) + + # Synthesizer is openai so the OpenAI-shaped transport stub serves both the + # members and the synthesis call (all OpenAI-compatible). + config_file = tmp_path / "conclave.yml" + config_file.write_text("synthesizer: openai\n", encoding="utf-8") + monkeypatch.setenv("CONCLAVE_CONFIG", str(config_file)) + + config_mod.clear_config_cache() + + reads = {"n": 0} + real_read_yaml = config_mod._read_yaml + + def counting_read_yaml(path): + reads["n"] += 1 + return real_read_yaml(path) + + monkeypatch.setattr(config_mod, "_read_yaml", counting_read_yaml) + + async def fake_post(url, headers, json_body, timeout): + return 200, {"choices": [{"message": {"content": "answer"}}]} + + monkeypatch.setattr("conclave.transport.post_json", fake_post) + + # Council built with no injected config -> resolves via load_config; every + # member + synthesis call then also calls load_config from providers. + council = Council(models=["grok", "perplexity", "openai"], synthesizer="openai") + result = await council.ask("what is 2+2?") + + assert result.synthesis == "answer" + assert len(result.answers) == 3 + assert reads["n"] <= 1, f"expected at most one disk read for the run, got {reads['n']}" + + config_mod.clear_config_cache() diff --git a/tests/test_providers.py b/tests/test_providers.py index 001b3a1..826193c 100644 --- a/tests/test_providers.py +++ b/tests/test_providers.py @@ -15,14 +15,25 @@ import pytest +import conclave.config as config_mod +import conclave.providers as providers_mod from conclave.adapters import ProviderError, resolve_adapter from conclave.adapters.anthropic import AnthropicAdapter from conclave.adapters.base import redact from conclave.adapters.gemini import GeminiAdapter from conclave.adapters.openai_compat import OpenAICompatAdapter -from conclave.config import ConclaveConfig, CustomEndpoint +from conclave.config import ConclaveConfig, CustomEndpoint, clear_config_cache from conclave.providers import call_model + +@pytest.fixture(autouse=True) +def _reset_config_cache(): + """Each test starts and ends with an empty config memo for isolation.""" + clear_config_cache() + yield + clear_config_cache() + + # --------------------------------------------------------------------------- # # Adapter registry # --------------------------------------------------------------------------- # @@ -240,3 +251,94 @@ def test_redact_scrubs_custom_endpoint_env_var_value(monkeypatch, tmp_path): cleaned = redact(f"auth failed: invalid api key: {fake_key}") assert fake_key not in cleaned assert "[REDACTED]" in cleaned + + +# --------------------------------------------------------------------------- # +# Config is resolved once / injectable, not re-read per call (issue #15) +# --------------------------------------------------------------------------- # + + +async def _ok_post(url, headers, json_body, timeout): + """A minimal successful transport stub for hot-path tests.""" + return 200, {"choices": [{"message": {"content": "ok"}}]} + + +async def test_call_model_uses_injected_config_without_load(monkeypatch): + """When a config is injected, call_model never calls load_config (issue #15).""" + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + monkeypatch.setattr("conclave.transport.post_json", _ok_post) + + calls = {"n": 0} + + def spy_load_config(*args, **kwargs): # pragma: no cover - must not run + calls["n"] += 1 + return ConclaveConfig() + + monkeypatch.setattr(providers_mod, "load_config", spy_load_config) + + injected = ConclaveConfig() + answer = await call_model( + "openai", + "openai/gpt-4.1", + [{"role": "user", "content": "hi"}], + config=injected, + ) + assert answer.ok + assert calls["n"] == 0, "load_config must not be called when config is injected" + + +async def test_call_model_standalone_reads_disk_at_most_once(monkeypatch, tmp_path): + """Repeated standalone calls re-read the config file at most once (issue #15). + + The memoized loader means the disk read + YAML parse happens once even across + many call_model invocations for the same unchanged file -- the hot-path / + caching blocker the issue describes. + """ + monkeypatch.setenv("OPENAI_API_KEY", "sk-test") + monkeypatch.setattr("conclave.transport.post_json", _ok_post) + + config_file = tmp_path / "conclave.yml" + config_file.write_text("synthesizer: openai\n", encoding="utf-8") + monkeypatch.setenv("CONCLAVE_CONFIG", str(config_file)) + + reads = {"n": 0} + real_read_yaml = config_mod._read_yaml + + def counting_read_yaml(path): + reads["n"] += 1 + return real_read_yaml(path) + + monkeypatch.setattr(config_mod, "_read_yaml", counting_read_yaml) + + # Simulate a 4-member x 3-round debate + synthesis worth of calls. + for _ in range(13): + answer = await call_model( + "openai", + "openai/gpt-4.1", + [{"role": "user", "content": "hi"}], + ) + assert answer.ok + + assert reads["n"] == 1, f"expected a single disk read, got {reads['n']}" + + +def test_config_cache_invalidates_on_file_change(tmp_path): + """The memo self-invalidates when the config file's mtime changes.""" + import os + import time + + from conclave.config import load_config + + clear_config_cache() + config_file = tmp_path / "conclave.yml" + config_file.write_text("synthesizer: openai\n", encoding="utf-8") + + first = load_config(path=config_file) + assert first.synthesizer == "openai" + + # Rewrite with a new value and bump mtime so the key changes. + config_file.write_text("synthesizer: gemini\n", encoding="utf-8") + os.utime(config_file, (time.time() + 5, time.time() + 5)) + + second = load_config(path=config_file) + assert second.synthesizer == "gemini" diff --git a/tests/test_registry_config.py b/tests/test_registry_config.py index 8cc4436..d23622e 100644 --- a/tests/test_registry_config.py +++ b/tests/test_registry_config.py @@ -2,9 +2,16 @@ from __future__ import annotations +import pytest + from conclave.config import load_config from conclave.registry import ( DEFAULT_MODELS, + NATIVE_PROVIDERS, + OPENAI_COMPAT_PROVIDERS, + PROVIDER_ENV_VARS, + RegistryError, + _assert_metadata_consistent, key_present, key_source, provider_prefix, @@ -108,3 +115,89 @@ def test_malformed_config_ignored(tmp_path): cfg = load_config(path=cfg_path) # Falls back to defaults rather than raising. assert cfg.models["grok"] == DEFAULT_MODELS["grok"] + + +# --------------------------------------------------------------------------- # +# Single-source-of-truth for provider metadata (issue #19) +# --------------------------------------------------------------------------- # + + +def test_provider_env_vars_derived_from_single_source(): + """PROVIDER_ENV_VARS is built from the OpenAI-compat + native source tables. + + There is no hand-maintained second copy that could drift: every env-var entry + must trace back to exactly one source table. + """ + derived = { + **{prefix: list(meta.env_vars) for prefix, meta in OPENAI_COMPAT_PROVIDERS.items()}, + **{prefix: list(env_vars) for prefix, env_vars in NATIVE_PROVIDERS.items()}, + } + assert PROVIDER_ENV_VARS == derived + + +def test_every_openai_compat_provider_has_env_vars(): + """An OpenAI-compatible provider can never be declared with a URL but no key.""" + for prefix, meta in OPENAI_COMPAT_PROVIDERS.items(): + assert meta.env_vars, f"{prefix} declares a URL but no env var" + assert PROVIDER_ENV_VARS[prefix] == list(meta.env_vars) + + +def test_registry_and_adapter_url_tables_are_in_sync(): + """registry.OPENAI_COMPAT_PROVIDERS and adapters.OPENAI_COMPAT_URLS agree. + + This is the invariant whose violation used to escape as a runtime KeyError in + _openai_compat_adapter (issue #19). The live tables must match exactly. + """ + from conclave.adapters.openai_compat import OPENAI_COMPAT_URLS + + assert set(OPENAI_COMPAT_PROVIDERS) == set(OPENAI_COMPAT_URLS) + for prefix, meta in OPENAI_COMPAT_PROVIDERS.items(): + assert meta.completions_url == OPENAI_COMPAT_URLS[prefix] + + +def test_consistency_check_passes_for_live_metadata(): + """The import-time guard is a no-op against the real, in-sync tables.""" + # Must not raise. + _assert_metadata_consistent() + + +def test_consistency_check_detects_url_drift(monkeypatch): + """A URL present in the adapter table but absent from registry is caught loudly. + + Simulates the exact drift scenario: someone adds a prefix to + adapters.OPENAI_COMPAT_URLS without adding it to the registry source of truth. + The former failure mode was a silent per-call KeyError; now it is a clear, + immediate RegistryError. + """ + import conclave.adapters.openai_compat as oc + + drifted = dict(oc.OPENAI_COMPAT_URLS) + drifted["ghostprovider"] = "https://api.ghost.example/v1/chat/completions" + monkeypatch.setattr(oc, "OPENAI_COMPAT_URLS", drifted) + + with pytest.raises(RegistryError, match="ghostprovider"): + _assert_metadata_consistent() + + +def test_consistency_check_detects_missing_url(monkeypatch): + """A registry provider with no matching adapter URL is caught loudly.""" + import conclave.adapters.openai_compat as oc + + drifted = dict(oc.OPENAI_COMPAT_URLS) + drifted.pop("perplexity") + monkeypatch.setattr(oc, "OPENAI_COMPAT_URLS", drifted) + + with pytest.raises(RegistryError, match="perplexity"): + _assert_metadata_consistent() + + +def test_consistency_check_detects_url_mismatch(monkeypatch): + """Same prefix, divergent URL between the two tables is caught loudly.""" + import conclave.adapters.openai_compat as oc + + drifted = dict(oc.OPENAI_COMPAT_URLS) + drifted["openai"] = "https://api.openai.com/v2/chat/completions" + monkeypatch.setattr(oc, "OPENAI_COMPAT_URLS", drifted) + + with pytest.raises(RegistryError, match="URL drift"): + _assert_metadata_consistent()