From e5e7fbcd272cc69507dbec1f4462a7f657413246 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 19 Sep 2026 22:58:51 -0700 Subject: [PATCH] fix(auth): a user provider plugin's endpoint overrides the bundled row too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `$HERMES_HOME/plugins/model-providers//` plugin re-registering a bundled provider (stepfun with a regional base_url, gmi at a staging host) wins in `providers._REGISTRY` — register_provider() is last-writer-wins and the plugin guide promises exactly this — but the runtime reads its endpoint from `hermes_cli.auth.PROVIDER_REGISTRY`, whose mirror loop skipped every name already present, so inference kept going to the built-in URL (#48450). The mirror now applies one explicit precedence rule: when a row core wrote (built-in or plugin-mirrored) belongs to a name whose profile currently registered came from a USER plugin, the row's profile-derived fields are rewritten in place (inference_base_url; api_key_env_vars / base_url_env_var on api-key rows when the profile declares env_vars). `providers` records the discovery source per registration (`provider_source()`), because a bundled profile must never rewrite a built-in row: several bundled profiles omit the row's `*_BASE_URL` env var and one differs in auth_type, so an unconditional "profile wins" would have changed built-in behaviour. With no user plugin PROVIDER_REGISTRY is byte-identical before/after (78 rows probed). copilot/kimi/zai keep their bespoke resolution via the existing skip set. Co-authored-by: xiaoxinova --- hermes_cli/auth.py | 2 + hermes_cli/auth_plugin_providers.py | 40 ++++++++--- providers/__init__.py | 20 +++++- .../test_provider_registry_override.py | 67 +++++++++++++++++++ .../developer-guide/model-provider-plugin.md | 10 +++ 5 files changed, 130 insertions(+), 9 deletions(-) create mode 100644 tests/hermes_cli/test_provider_registry_override.py diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index c6d34e36a6..2620c6fdf9 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -246,6 +246,8 @@ _REGISTRY_ROWS: Tuple[Any, ...] = ( PROVIDER_REGISTRY: Dict[str, ProviderConfig] = { p.id: p for p in (r if isinstance(r, ProviderConfig) else _api_key_provider(*r) for r in _REGISTRY_ROWS) } +# The rows above, before any plugin touches the dict (a user plugin may override these; #48450). +BUILTIN_PROVIDER_IDS = frozenset(PROVIDER_REGISTRY) # ``hermes_cli.config`` discovers model-provider plugins while importing, and a plugin may read this # module's registry during that discovery. Keep the import below ProviderConfig / PROVIDER_REGISTRY so diff --git a/hermes_cli/auth_plugin_providers.py b/hermes_cli/auth_plugin_providers.py index 631ff8bbe7..dbb351dcc0 100644 --- a/hermes_cli/auth_plugin_providers.py +++ b/hermes_cli/auth_plugin_providers.py @@ -35,6 +35,13 @@ PLUGIN_AUTH_ACTIONS = ("add", "status", "logout", "refresh") PLUGIN_MIRRORED_PROVIDERS: set[str] = set() +def _api_key_env_fields(pp: Any) -> tuple[tuple, str]: + """Split a profile's ``env_vars`` into (api-key vars, base-URL var); the URL var may be "".""" + is_url = lambda v: v.endswith("_BASE_URL") or v.endswith("_URL") # noqa: E731 + return (tuple(v for v in pp.env_vars if not is_url(v)) or pp.env_vars, + next((v for v in pp.env_vars if is_url(v)), None) or "") + + def register_plugin_provider(pp: Any) -> None: """Mirror one profile into ``PROVIDER_REGISTRY`` under the ``auth_type`` it declares. @@ -48,11 +55,7 @@ def register_plugin_provider(pp: Any) -> None: if pp.auth_type == "api_key": if not pp.env_vars: return - is_url = lambda v: v.endswith("_BASE_URL") or v.endswith("_URL") # noqa: E731 - pconfig = _api_key_provider( - pp.name, pp.display_name or pp.name, pp.base_url, - tuple(v for v in pp.env_vars if not is_url(v)) or pp.env_vars, - next((v for v in pp.env_vars if is_url(v)), None) or "") + pconfig = _api_key_provider(pp.name, pp.display_name or pp.name, pp.base_url, *_api_key_env_fields(pp)) else: pconfig = ProviderConfig(pp.name, pp.display_name or pp.name, pp.auth_type, inference_base_url=pp.base_url) PROVIDER_REGISTRY[pp.name] = pconfig @@ -61,25 +64,46 @@ def register_plugin_provider(pp: Any) -> None: PROVIDER_REGISTRY.setdefault(alias, pconfig) +def override_registry_row(pconfig: Any, pp: Any) -> None: + """A ``$HERMES_HOME`` plugin re-registering a name that already has a row wins for the fields + it declares — ``base_url`` and, on api-key rows, ``env_vars`` (#48450). ``register_provider()`` + is last-writer-wins for the profile; without this the runtime kept reading the built-in + endpoint. In place, so alias rows sharing the object follow; idempotent, so re-sync is free. + """ + if pp.base_url: + pconfig.inference_base_url = pp.base_url + if pp.auth_type == "api_key" == pconfig.auth_type and pp.env_vars: + pconfig.api_key_env_vars, url_var = _api_key_env_fields(pp) + if url_var: + pconfig.base_url_env_var = url_var + + def sync_plugin_provider_registry() -> int: """Mirror provider-plugin profiles into ``PROVIDER_REGISTRY``; return how many were added. - Idempotent (existing entries are never replaced), so it is safe from resolution paths. It runs at + Idempotent (existing entries are never replaced — a user plugin re-registering a bundled name + only rewrites the fields it declares, see :func:`override_registry_row`), so it is safe from + resolution paths. It runs at auth import and again whenever a name is missing (:func:`registry_lookup`) or when ``providers`` finishes discovery, because the import-time pass can observe a *partial* profile list: a plugin whose own imports pull ``hermes_cli.auth`` in mid-``_discover_providers()`` sees only what was registered so far, and every later plugin would otherwise fail with "Unknown provider" (#102123). """ - from hermes_cli.auth import PROVIDER_REGISTRY + from hermes_cli.auth import BUILTIN_PROVIDER_IDS, PROVIDER_REGISTRY try: - from providers import list_providers + from providers import list_providers, provider_source profiles = list_providers() except Exception: return 0 added = 0 for pp in profiles: if pp.name in PROVIDER_REGISTRY: + # Only rows core wrote (built-in or mirrored) — a row the plugin injected itself is its + # own, more specific declaration and stays as written. + core_row = pp.name in BUILTIN_PROVIDER_IDS or pp.name in PLUGIN_MIRRORED_PROVIDERS + if core_row and pp.name not in _REGISTRY_PLUGIN_SKIP and provider_source(pp.name) == "user": + override_registry_row(PROVIDER_REGISTRY[pp.name], pp) continue register_plugin_provider(pp) added += pp.name in PROVIDER_REGISTRY diff --git a/providers/__init__.py b/providers/__init__.py index 884fec0851..c2b38b8167 100644 --- a/providers/__init__.py +++ b/providers/__init__.py @@ -44,6 +44,10 @@ logger = logging.getLogger(__name__) _REGISTRY: dict[str, ProviderProfile] = {} _ALIASES: dict[str, str] = {} +# Where the CURRENT registration of each name came from: "bundled" / "user" (a +# ``$HERMES_HOME`` plugin dir) / "runtime" (entry point, legacy module, direct call). +_SOURCES: dict[str, str] = {} +_current_source: str | None = None _PROVIDER_LIST_CACHE: list[ProviderProfile] | None = None _discovered = False _discovering = False @@ -83,6 +87,7 @@ def register_provider(profile: ProviderProfile) -> None: """ global _PROVIDER_LIST_CACHE _REGISTRY[profile.name] = profile + _SOURCES[profile.name] = _current_source or "runtime" for alias in profile.aliases: _ALIASES[alias] = profile.name _PROVIDER_LIST_CACHE = None @@ -90,6 +95,15 @@ def register_provider(profile: ProviderProfile) -> None: _sync_auth_registry() +def provider_source(name: str) -> str | None: + """Discovery source of the profile currently registered under *name* (see ``_SOURCES``), or None. + + ``"user"`` is what lets a ``$HERMES_HOME`` plugin re-registering a bundled name win in + ``hermes_cli.auth.PROVIDER_REGISTRY`` too — a bundled profile never rewrites a built-in row. + """ + return _SOURCES.get(_ALIASES.get(name, name)) + + def get_provider_profile(name: str) -> ProviderProfile | None: """Look up a provider profile by name or alias. @@ -217,8 +231,9 @@ def _declares_model_provider_kind(plugin_dir: Path) -> bool: def _import_plugin_dir(plugin_dir: Path, source: str) -> None: """Import a single plugin directory so it self-registers. - ``source`` is "bundled" or "user", used only for log messages. + ``source`` is "bundled" or "user"; it is recorded per registered profile (``_SOURCES``). """ + global _current_source init_file = plugin_dir / "__init__.py" if not init_file.exists(): return @@ -236,6 +251,7 @@ def _import_plugin_dir(plugin_dir: Path, source: str) -> None: if module_name in sys.modules: return # already imported + _current_source = source try: spec = importlib.util.spec_from_file_location( module_name, init_file, submodule_search_locations=[str(plugin_dir)] @@ -250,6 +266,8 @@ def _import_plugin_dir(plugin_dir: Path, source: str) -> None: "Failed to load %s provider plugin %s: %s", source, plugin_dir.name, exc ) sys.modules.pop(module_name, None) + finally: + _current_source = None def _discover_entry_point_providers() -> None: diff --git a/tests/hermes_cli/test_provider_registry_override.py b/tests/hermes_cli/test_provider_registry_override.py new file mode 100644 index 0000000000..7e685ca020 --- /dev/null +++ b/tests/hermes_cli/test_provider_registry_override.py @@ -0,0 +1,67 @@ +"""A `$HERMES_HOME` provider plugin re-registering a bundled name reaches the runtime (#48450). + +``register_provider()`` is last-writer-wins for the profile, and the docs promise that dropping +``plugins/model-providers//`` points that provider at another endpoint. The runtime +reads ``hermes_cli.auth.PROVIDER_REGISTRY`` though, so the mirror has to carry the override across. +Each case runs in a fresh interpreter: real discovery, real auth import, no process-global leakage. +""" + +from __future__ import annotations + +import json +import os +import subprocess +import sys +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] + +_PROBE = """ +import json, os +from providers import list_providers +from hermes_cli.auth import PROVIDER_REGISTRY +from hermes_cli.runtime_provider import resolve_runtime_provider +list_providers() +row = PROVIDER_REGISTRY["stepfun"] +print(json.dumps({ + "runtime_base_url": resolve_runtime_provider(requested="stepfun")["base_url"], + "api_key_env_vars": list(row.api_key_env_vars), "base_url_env_var": row.base_url_env_var, + "gmi_base_url": PROVIDER_REGISTRY["gmi"].inference_base_url})) +""" + + +def _run(tmp_path: Path, plugin_source: str) -> dict: + home = tmp_path / "home" + plugin_dir = home / "plugins" / "model-providers" / "stepfun" + plugin_dir.mkdir(parents=True) + (plugin_dir / "__init__.py").write_text(plugin_source, encoding="utf-8") + env = {**os.environ, "HERMES_HOME": str(home), "PYTHONPATH": str(REPO), "STEPFUN_API_KEY": "sk-fixture"} + env.pop("STEPFUN_BASE_URL", None) + proc = subprocess.run([sys.executable, "-c", _PROBE], env=env, capture_output=True, text=True, timeout=120, + cwd=str(REPO), check=False) + assert proc.returncode == 0, proc.stderr + return json.loads(proc.stdout.strip().splitlines()[-1]) + + +def test_user_plugin_endpoint_and_env_vars_reach_the_runtime(tmp_path): + result = _run(tmp_path, ( + "from providers import register_provider\n" + "from providers.base import ProviderProfile\n" + "register_provider(ProviderProfile(name='stepfun', aliases=('step',), auth_type='api_key',\n" + " env_vars=('STEPFUN_API_KEY', 'STEPFUN_REGIONAL_BASE_URL'),\n" + " base_url='https://api.stepfun.com/step_plan/v1'))\n")) + assert result["runtime_base_url"] == "https://api.stepfun.com/step_plan/v1" + assert result["base_url_env_var"] == "STEPFUN_REGIONAL_BASE_URL" + assert result["api_key_env_vars"] == ["STEPFUN_API_KEY"] + + +def test_user_plugin_declaring_no_endpoint_keeps_the_builtin_row(tmp_path): + from hermes_cli.auth import PROVIDER_REGISTRY + + result = _run(tmp_path, ( + "from providers import register_provider\n" + "from providers.base import ProviderProfile\n" + "register_provider(ProviderProfile(name='stepfun', auth_type='api_key', env_vars=('STEPFUN_API_KEY',)))\n")) + assert result["runtime_base_url"] == PROVIDER_REGISTRY["stepfun"].inference_base_url + assert result["base_url_env_var"] == "STEPFUN_BASE_URL" + assert result["gmi_base_url"] == PROVIDER_REGISTRY["gmi"].inference_base_url diff --git a/website/docs/developer-guide/model-provider-plugin.md b/website/docs/developer-guide/model-provider-plugin.md index 557b0d1ad8..3816182e00 100644 --- a/website/docs/developer-guide/model-provider-plugin.md +++ b/website/docs/developer-guide/model-provider-plugin.md @@ -304,6 +304,16 @@ register_provider(ProviderProfile( In a fresh Hermes process, `get_provider_profile("gmi").base_url` returns the staging URL. No repo patch, no rebuild. Because user plugins are discovered after bundled ones, the user `register_provider()` call wins. +The override also reaches the runtime. Built-in providers have a row in `hermes_cli.auth.PROVIDER_REGISTRY` (the table `resolve_runtime_provider()` reads its endpoint and env vars from); a `$HERMES_HOME` plugin re-registering that name rewrites the row's profile-derived fields, so inference goes to the staging URL, not the bundled one: + +| Profile field | Registry row field | When | +|---|---|---| +| `base_url` | `inference_base_url` | profile sets a non-empty `base_url` | +| `env_vars` (non-URL entries) | `api_key_env_vars` | api-key row and profile sets `env_vars` | +| `env_vars` (final `*_BASE_URL` / `*_URL` entry) | `base_url_env_var` | profile declares one; otherwise the built-in env var (e.g. `GMI_BASE_URL`) stays | + +Only a **user** plugin (`$HERMES_HOME/plugins/model-providers/` or an installed `kind: model-provider` plugin) triggers this; a bundled profile never rewrites a built-in row, and `copilot`, `kimi-coding`, `kimi-coding-cn` and `zai` keep their bespoke credential resolution. A field the profile leaves empty keeps the built-in value. A `*_BASE_URL` env var still wins over both. + ## api_mode selection Four values are recognized. Hermes picks one based on: