From 7e037e8b624775d492dd1f6a5ea773bcae59f147 Mon Sep 17 00:00:00 2001 From: Ben Barclay Date: Thu, 29 Oct 2026 08:04:08 +1100 Subject: [PATCH] fix(config): importing hermes_cli.config or hermes_cli.auth reads no config hermes_cli.config filled OPTIONAL_ENV_VARS from provider profiles at import, and hermes_cli.auth mirrored plugin providers into PROVIDER_REGISTRY at import. Both list providers, and provider discovery reads config (plugins.enabled, plugins.isolation). An import-time config read is the launch profile's config in a process that serves several profiles, and it deadlocks a config backend whose own load imports hermes_cli.auth (the remote backend's Nous credential). Both now run on first read: OPTIONAL_ENV_VARS is a dict that runs one armed fill before it is first read (CORE_DECLARED_ENV_NAMES follows it via module __getattr__), and PROVIDER_REGISTRY syncs before its first read. Contents and precedence are unchanged: core entries win, a provider entry replaces a platform manifest's, the first provider to declare a name wins. A fill that fails with ConfigBackendUnavailable propagates and re-arms. --- hermes_cli/auth.py | 2 +- hermes_cli/auth_plugin_providers.py | 26 +++++++ hermes_cli/config.py | 53 +++++-------- hermes_cli/config_defaults.py | 8 +- hermes_cli/config_defaults_env.py | 68 +++++++++++++++- hermes_cli/config_provider_env.py | 57 ++++++++++++++ tests/hermes_cli/test_config_provider_env.py | 82 ++++++++++++++++++++ 7 files changed, 255 insertions(+), 41 deletions(-) create mode 100644 hermes_cli/config_provider_env.py create mode 100644 tests/hermes_cli/test_config_provider_env.py diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index aeacdf3b23..166e269476 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -257,7 +257,7 @@ from hermes_cli.auth_plugin_providers import ( canonical_registry_rows as _canonical_registry_rows, get_plugin_oauth_auth_status, registry_lookup as _registry_lookup, sync_plugin_provider_registry) -sync_plugin_provider_registry() +PROVIDER_REGISTRY.sync_before_first_read() # not at import: listing providers reads config def get_anthropic_key() -> str: diff --git a/hermes_cli/auth_plugin_providers.py b/hermes_cli/auth_plugin_providers.py index cb0c33e277..a5cec6edbf 100644 --- a/hermes_cli/auth_plugin_providers.py +++ b/hermes_cli/auth_plugin_providers.py @@ -19,6 +19,7 @@ from __future__ import annotations import copy import logging +import threading from typing import Any, Callable, Optional logger = logging.getLogger(__name__) @@ -36,6 +37,9 @@ PLUGIN_AUTH_ACTIONS = ("add", "status", "logout", "refresh") PLUGIN_MIRRORED_PROVIDERS: set[str] = set() +_SYNC_LOCK = threading.RLock() + + class HomeScopedRegistry(dict): """``PROVIDER_REGISTRY``: process-wide storage, each row read as the bound profile home sees it. @@ -63,13 +67,34 @@ class HomeScopedRegistry(dict): return row return _home_row(row, user, from_profile=mirrored and shared is None) + _sync_pending = False + + def sync_before_first_read(self) -> None: + """Mirror plugin profiles (:func:`sync_plugin_provider_registry`) before the first read.""" + self._sync_pending = True + + def _ready(self) -> None: + if not self._sync_pending: + return + with _SYNC_LOCK: # re-entrant: the sync reads the registry + if not self._sync_pending: + return + self._sync_pending = False + try: + sync_plugin_provider_registry() + except BaseException: + self._sync_pending = True + raise + def __getitem__(self, key: str) -> Any: + self._ready() row = self._view(key, dict.__getitem__(self, key), _resolver()) if row is None: raise KeyError(key) return row def get(self, key: str, default: Any = None) -> Any: + self._ready() row = dict.get(self, key) row = None if row is None else self._view(key, row, _resolver()) return default if row is None else row @@ -78,6 +103,7 @@ class HomeScopedRegistry(dict): return self.get(key) is not None def items(self) -> list[tuple[str, Any]]: + self._ready() resolve = _resolver() views = ((key, self._view(key, row, resolve)) for key, row in dict.items(self)) return [(key, row) for key, row in views if row is not None] diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 843101087c..6dc63b54f1 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -586,6 +586,8 @@ def ensure_hermes_home(): # ---- Config loading/saving ---- from hermes_cli.config_defaults import DEFAULT_CONFIG, OPTIONAL_ENV_VARS +from hermes_cli import config_provider_env as _config_provider_env +from hermes_cli.config_provider_env import core_declared_env_names from hermes_cli.config_providers import ( _API_MODE_ALIASES, _CAMEL_ALIASES, _KNOWN_PROVIDER_KEYS, _PROVIDER_NORMALIZE_WARNED, _canonical_api_mode, _coerce_ssl_verify, _custom_provider_entry_to_provider_config, @@ -3734,34 +3736,7 @@ def config_command(args): sys.exit(1) -# ---- OPTIONAL_ENV_VARS injection from provider profiles and platform plugins (once, at import) ---- - -def _inject_profile_env_vars() -> None: - """Expose env_vars of every ``auth_type="api_key"`` provider in providers/ via OPTIONAL_ENV_VARS - without editing this file.""" - try: - from providers import list_providers - for _pp in list_providers(): - if _pp.auth_type != "api_key": - continue - for _var in _pp.env_vars: - if _var in OPTIONAL_ENV_VARS: - continue - _is_key = not _var.endswith(("_BASE_URL", "_URL")) - _label = _pp.display_name or _pp.name - OPTIONAL_ENV_VARS[_var] = { - "description": f"{_label} {'API key' if _is_key else 'base URL override'}", - "prompt": f"{_label} {'API key' if _is_key else 'base URL (leave empty for default)'}", - "url": _pp.signup_url or None, - "password": _is_key, - "category": "provider", - "advanced": True} - except Exception: - pass - - -_inject_profile_env_vars() - +# ---- OPTIONAL_ENV_VARS: platform plugin entries at import; provider entries on first read ---- PlatformManifestSource = Literal["all", "bundled", "user"] @@ -3884,13 +3859,13 @@ def _platform_manifest_env_entries(manifest: dict, *, optional: bool = True): yield name, is_secret, meta -def _manifest_secret_envs(manifests) -> frozenset[str]: +def _manifest_secret_envs(manifests, core: Optional[frozenset[str]] = None) -> frozenset[str]: """Upper-cased secret messaging env names the given manifests declare, minus core-declared names: a manifest never reclassifies a core variable such as OPENAI_API_KEY.""" names = {name.upper() for _dir, manifest in manifests for name, is_secret, meta in _platform_manifest_env_entries(manifest) if is_secret and (meta.get("category") or "messaging") == "messaging"} - return frozenset(names - {n.upper() for n in CORE_DECLARED_ENV_NAMES}) + return frozenset(names - {n.upper() for n in (core if core is not None else core_declared_env_names())}) def platform_manifest_secret_envs(home: Optional[Path] = None, source: PlatformManifestSource = "user", *, @@ -3898,8 +3873,8 @@ def platform_manifest_secret_envs(home: Optional[Path] = None, source: PlatformM """Secret env names declared by one source's platform plugin manifests: ``"user"`` reads only ``home``'s user-installed plugins, which belong to that profile alone; ``"bundled"`` returns the set read once at import (re-read strictly if that read hit an I/O error).""" - if source == "bundled" and BUNDLED_PLATFORM_SECRET_ENVS is not None: - return BUNDLED_PLATFORM_SECRET_ENVS + if source == "bundled" and BUNDLED_PLATFORM_SECRET_ENVS is not None: # minus provider names, known only now + return frozenset(BUNDLED_PLATFORM_SECRET_ENVS - {n.upper() for n in core_declared_env_names()}) return _manifest_secret_envs(_platform_plugin_manifests(home, source, strict=strict)) @@ -3941,11 +3916,19 @@ def _inject_platform_plugin_env_vars() -> frozenset[str] | None: "url": meta.get("url") or None, "password": is_secret, "category": meta.get("category") or "messaging"} - return _manifest_secret_envs(bundled) if bundled is not None else None + return _manifest_secret_envs(bundled, _STATIC_CORE_ENV_NAMES) if bundled is not None else None # Names declared in core, before any platform manifest is read. A manifest never reclassifies # one: the config form keeps the core entry, and the child-env scrub keeps a plugin that lists -# OPENAI_API_KEY from turning a provider key into an adapter secret. -CORE_DECLARED_ENV_NAMES: frozenset[str] = frozenset(OPTIONAL_ENV_VARS) +# OPENAI_API_KEY from turning a provider key into an adapter secret. CORE_DECLARED_ENV_NAMES adds +# the provider profiles' names on first use (module __getattr__): listing them reads config. +_STATIC_CORE_ENV_NAMES: frozenset[str] = frozenset(dict.keys(OPTIONAL_ENV_VARS)) BUNDLED_PLATFORM_SECRET_ENVS: frozenset[str] | None = _inject_platform_plugin_env_vars() +_config_provider_env.arm(OPTIONAL_ENV_VARS, _STATIC_CORE_ENV_NAMES) + + +def __getattr__(name: str) -> Any: + if name == "CORE_DECLARED_ENV_NAMES": + return core_declared_env_names() + raise AttributeError(f"module {__name__!r} has no attribute {name!r}") diff --git a/hermes_cli/config_defaults.py b/hermes_cli/config_defaults.py index 9d8c4b2262..6a8f238d04 100644 --- a/hermes_cli/config_defaults.py +++ b/hermes_cli/config_defaults.py @@ -2712,13 +2712,13 @@ DEFAULT_CONFIG = { } -from hermes_cli.config_defaults_env import _base_url, _env, _msg, _prov, _setting, _skill, _tool +from hermes_cli.config_defaults_env import OptionalEnvVars, _base_url, _env, _msg, _prov, _setting, _skill, _tool # Optional environment variables that enhance functionality. Feeds the dashboard keys page and setup # checklists; category: provider|tool|skill|messaging|setting, advanced=True hides from checklists, -# tools=[...] lists the model tools the key unlocks. -OPTIONAL_ENV_VARS = { +# tools=[...] lists the model tools the key unlocks. Provider entries: config_provider_env (on first read). +OPTIONAL_ENV_VARS = OptionalEnvVars({ # ── Provider (handled in provider selection, not shown in checklists) ── "NOUS_BASE_URL": _base_url("Nous Portal"), "HERMES_ANON_API_SECRET": _env( @@ -3118,4 +3118,4 @@ OPTIONAL_ENV_VARS = { "HERMES_EPHEMERAL_SYSTEM_PROMPT": _setting( "Ephemeral system prompt injected at API-call time (never persisted to sessions)", "Ephemeral system prompt", None), -} +}) diff --git a/hermes_cli/config_defaults_env.py b/hermes_cli/config_defaults_env.py index f982f2914c..128345e056 100644 --- a/hermes_cli/config_defaults_env.py +++ b/hermes_cli/config_defaults_env.py @@ -1,4 +1,70 @@ -"""Entry factories for ``config_defaults.OPTIONAL_ENV_VARS`` (pure-data leaf, no imports).""" +"""Entry factories for ``config_defaults.OPTIONAL_ENV_VARS`` (pure-data leaf, stdlib only).""" + +import threading + +_FILL_LOCK = threading.RLock() # re-entrant: the fill itself reads the table + + +class OptionalEnvVars(dict): + """``OPTIONAL_ENV_VARS``: a dict that runs one armed ``fill(table)`` before it is first read. + + Some entries (provider profiles') are only known after work that must not run at import + (``hermes_cli.config_provider_env``); writes never trigger the fill. A fill that raises is + re-armed, so the next read tries again.""" + + _fill = None + + def arm(self, fill) -> None: + self._fill = fill + + def _ready(self) -> None: + if self._fill is None: + return + with _FILL_LOCK: + fill, self._fill = self._fill, None + if fill is None: + return + try: + fill(self) + except BaseException: + self._fill = fill + raise + + def __getitem__(self, key): + self._ready() + return dict.__getitem__(self, key) + + def get(self, key, default=None): + self._ready() + return dict.get(self, key, default) + + def __contains__(self, key) -> bool: + self._ready() + return dict.__contains__(self, key) + + def __iter__(self): + self._ready() + return dict.__iter__(self) + + def __len__(self) -> int: + self._ready() + return dict.__len__(self) + + def keys(self): + self._ready() + return dict.keys(self) + + def items(self): + self._ready() + return dict.items(self) + + def values(self): + self._ready() + return dict.values(self) + + def copy(self) -> dict: + self._ready() + return dict(dict.items(self)) def _env(description, prompt, **keys): diff --git a/hermes_cli/config_provider_env.py b/hermes_cli/config_provider_env.py new file mode 100644 index 0000000000..3a6dd932ec --- /dev/null +++ b/hermes_cli/config_provider_env.py @@ -0,0 +1,57 @@ +"""The provider-profile entries of ``OPTIONAL_ENV_VARS``, added on first read instead of at import. + +Listing provider profiles runs provider discovery, and discovery reads config: ``plugins.enabled`` +gates pip-installed provider plugins, ``plugins.isolation`` decides how ``$HERMES_HOME`` ones load. +Done while ``hermes_cli.config`` imports, that read is the launch profile's config in a process that +serves several profiles, and a cycle with a config backend that imports this code while it loads the +config (the remote backend's Nous credential imports ``hermes_cli.auth``, which imports +``hermes_cli.config``). So importing ``hermes_cli.config`` reads no config; the first read of the +table (or of ``CORE_DECLARED_ENV_NAMES``) adds the entries. +""" +from __future__ import annotations + +_STATIC_NAMES: frozenset[str] = frozenset() # declared in core (config_defaults), known at import +_PROVIDER_NAMES: set[str] = set() # added from provider profiles by the fill + + +def arm(table, static_names) -> None: + """Called once by ``hermes_cli.config`` at the end of its import.""" + global _STATIC_NAMES + _STATIC_NAMES = frozenset(static_names) + table.arm(_add_provider_entries) + + +def _add_provider_entries(table) -> None: + """Expose the env vars of every ``auth_type="api_key"`` provider profile. A core entry wins; + a provider entry replaces one a platform plugin manifest added (it was added first only + because this runs late).""" + try: + from providers import list_providers + profiles = list_providers() + except Exception: # health: allow BLE001 -- as before: a broken provider layer must not break config readers + # (ConfigBackendUnavailable is a SystemExit: it propagates, and the table re-arms) + return + for pp in profiles: + if pp.auth_type != "api_key": + continue + for var in pp.env_vars: + if var in _STATIC_NAMES or var in _PROVIDER_NAMES: + continue + is_key = not var.endswith(("_BASE_URL", "_URL")) + label = pp.display_name or pp.name + dict.__setitem__(table, var, { + "description": f"{label} {'API key' if is_key else 'base URL override'}", + "prompt": f"{label} {'API key' if is_key else 'base URL (leave empty for default)'}", + "url": pp.signup_url or None, + "password": is_key, + "category": "provider", + "advanced": True}) + _PROVIDER_NAMES.add(var) + + +def core_declared_env_names() -> frozenset[str]: + """Names declared in core: config_defaults' plus the provider profiles'. A platform manifest + never reclassifies one.""" + from hermes_cli.config_defaults import OPTIONAL_ENV_VARS + OPTIONAL_ENV_VARS.get("") # runs the fill once + return _STATIC_NAMES | frozenset(_PROVIDER_NAMES) diff --git a/tests/hermes_cli/test_config_provider_env.py b/tests/hermes_cli/test_config_provider_env.py new file mode 100644 index 0000000000..e18d61821f --- /dev/null +++ b/tests/hermes_cli/test_config_provider_env.py @@ -0,0 +1,82 @@ +"""Provider-profile entries of OPTIONAL_ENV_VARS: added on first read, same contents as before.""" +from __future__ import annotations + +from types import SimpleNamespace + +import pytest + +from hermes_cli import config_provider_env as cpe +from hermes_cli.config_backend import ConfigBackendUnavailable +from hermes_cli.config_defaults_env import OptionalEnvVars + + +def _profile(name, *env_vars, auth_type="api_key"): + return SimpleNamespace(name=name, display_name=name.title(), env_vars=env_vars, auth_type=auth_type, + signup_url=f"https://{name}.test/keys") + + +def test_every_api_key_provider_var_is_declared_in_core(): + import hermes_cli.config as cfg + from providers import list_providers + + expected = {v for p in list_providers() if p.auth_type == "api_key" for v in p.env_vars} + assert expected, "no api_key provider profiles registered" + assert expected <= set(cfg.OPTIONAL_ENV_VARS) + assert expected <= cfg.CORE_DECLARED_ENV_NAMES + + +@pytest.fixture +def table(monkeypatch): + monkeypatch.setattr(cpe, "_STATIC_NAMES", frozenset({"CORE_KEY"})) + monkeypatch.setattr(cpe, "_PROVIDER_NAMES", set()) + t = OptionalEnvVars({"CORE_KEY": {"description": "core"}, "PLUG_KEY": {"description": "platform"}}) + t.arm(cpe._add_provider_entries) + return t + + +def test_fill_runs_on_first_read_core_wins_provider_replaces_platform_first_provider_wins(table, monkeypatch): + import providers + + calls = [] + profiles = [_profile("alpha", "CORE_KEY", "PLUG_KEY", "ALPHA_API_KEY", "ALPHA_BASE_URL"), + _profile("beta", "ALPHA_API_KEY"), _profile("oauth", "OAUTH_TOKEN", auth_type="oauth")] + monkeypatch.setattr(providers, "list_providers", lambda: calls.append(1) or profiles) + dict.__setitem__(table, "LATE", {}) # writes and raw dict access never fill + assert calls == [] + assert table["CORE_KEY"] == {"description": "core"} and calls == [1] + assert table["PLUG_KEY"]["description"] == "Alpha API key" + assert table["ALPHA_API_KEY"]["description"] == "Alpha API key" and table["ALPHA_API_KEY"]["password"] + assert table["ALPHA_BASE_URL"]["password"] is False and "OAUTH_TOKEN" not in table + list(table.items()) + assert calls == [1] # once + + +def test_unavailable_config_fails_the_read_loudly_and_the_next_read_retries(table, monkeypatch): + import providers + + answers = [ConfigBackendUnavailable("boot in flight"), [_profile("alpha", "ALPHA_API_KEY")]] + + def list_providers(): + answer = answers.pop(0) + if isinstance(answer, BaseException): + raise answer + return answer + + monkeypatch.setattr(providers, "list_providers", list_providers) + with pytest.raises(ConfigBackendUnavailable): # fail closed, never a silently partial table + table.get("ALPHA_API_KEY") + assert "ALPHA_API_KEY" in table and answers == [] + + +def test_a_broken_provider_layer_is_not_retried(table, monkeypatch): + import providers + + calls = [] + + def boom(): + calls.append(1) + raise RuntimeError("broken") + + monkeypatch.setattr(providers, "list_providers", boom) + assert "CORE_KEY" in table and "CORE_KEY" in table + assert calls == [1]