fix(auth): a user provider plugin's endpoint overrides the bundled row too
A `$HERMES_HOME/plugins/model-providers/<name>/` 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 <xiaoxinova@users.noreply.github.com>
This commit is contained in:
@@ -246,6 +246,8 @@ _REGISTRY_ROWS: Tuple[Any, ...] = (
|
|||||||
PROVIDER_REGISTRY: Dict[str, ProviderConfig] = {
|
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)
|
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
|
# ``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
|
# module's registry during that discovery. Keep the import below ProviderConfig / PROVIDER_REGISTRY so
|
||||||
|
|||||||
@@ -35,6 +35,13 @@ PLUGIN_AUTH_ACTIONS = ("add", "status", "logout", "refresh")
|
|||||||
PLUGIN_MIRRORED_PROVIDERS: set[str] = set()
|
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:
|
def register_plugin_provider(pp: Any) -> None:
|
||||||
"""Mirror one profile into ``PROVIDER_REGISTRY`` under the ``auth_type`` it declares.
|
"""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 pp.auth_type == "api_key":
|
||||||
if not pp.env_vars:
|
if not pp.env_vars:
|
||||||
return
|
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, *_api_key_env_fields(pp))
|
||||||
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 "")
|
|
||||||
else:
|
else:
|
||||||
pconfig = ProviderConfig(pp.name, pp.display_name or pp.name, pp.auth_type, inference_base_url=pp.base_url)
|
pconfig = ProviderConfig(pp.name, pp.display_name or pp.name, pp.auth_type, inference_base_url=pp.base_url)
|
||||||
PROVIDER_REGISTRY[pp.name] = pconfig
|
PROVIDER_REGISTRY[pp.name] = pconfig
|
||||||
@@ -61,25 +64,46 @@ def register_plugin_provider(pp: Any) -> None:
|
|||||||
PROVIDER_REGISTRY.setdefault(alias, pconfig)
|
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:
|
def sync_plugin_provider_registry() -> int:
|
||||||
"""Mirror provider-plugin profiles into ``PROVIDER_REGISTRY``; return how many were added.
|
"""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``
|
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
|
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
|
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).
|
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:
|
try:
|
||||||
from providers import list_providers
|
from providers import list_providers, provider_source
|
||||||
profiles = list_providers()
|
profiles = list_providers()
|
||||||
except Exception:
|
except Exception:
|
||||||
return 0
|
return 0
|
||||||
added = 0
|
added = 0
|
||||||
for pp in profiles:
|
for pp in profiles:
|
||||||
if pp.name in PROVIDER_REGISTRY:
|
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
|
continue
|
||||||
register_plugin_provider(pp)
|
register_plugin_provider(pp)
|
||||||
added += pp.name in PROVIDER_REGISTRY
|
added += pp.name in PROVIDER_REGISTRY
|
||||||
|
|||||||
@@ -44,6 +44,10 @@ logger = logging.getLogger(__name__)
|
|||||||
|
|
||||||
_REGISTRY: dict[str, ProviderProfile] = {}
|
_REGISTRY: dict[str, ProviderProfile] = {}
|
||||||
_ALIASES: dict[str, str] = {}
|
_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
|
_PROVIDER_LIST_CACHE: list[ProviderProfile] | None = None
|
||||||
_discovered = False
|
_discovered = False
|
||||||
_discovering = False
|
_discovering = False
|
||||||
@@ -83,6 +87,7 @@ def register_provider(profile: ProviderProfile) -> None:
|
|||||||
"""
|
"""
|
||||||
global _PROVIDER_LIST_CACHE
|
global _PROVIDER_LIST_CACHE
|
||||||
_REGISTRY[profile.name] = profile
|
_REGISTRY[profile.name] = profile
|
||||||
|
_SOURCES[profile.name] = _current_source or "runtime"
|
||||||
for alias in profile.aliases:
|
for alias in profile.aliases:
|
||||||
_ALIASES[alias] = profile.name
|
_ALIASES[alias] = profile.name
|
||||||
_PROVIDER_LIST_CACHE = None
|
_PROVIDER_LIST_CACHE = None
|
||||||
@@ -90,6 +95,15 @@ def register_provider(profile: ProviderProfile) -> None:
|
|||||||
_sync_auth_registry()
|
_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:
|
def get_provider_profile(name: str) -> ProviderProfile | None:
|
||||||
"""Look up a provider profile by name or alias.
|
"""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:
|
def _import_plugin_dir(plugin_dir: Path, source: str) -> None:
|
||||||
"""Import a single plugin directory so it self-registers.
|
"""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"
|
init_file = plugin_dir / "__init__.py"
|
||||||
if not init_file.exists():
|
if not init_file.exists():
|
||||||
return
|
return
|
||||||
@@ -236,6 +251,7 @@ def _import_plugin_dir(plugin_dir: Path, source: str) -> None:
|
|||||||
if module_name in sys.modules:
|
if module_name in sys.modules:
|
||||||
return # already imported
|
return # already imported
|
||||||
|
|
||||||
|
_current_source = source
|
||||||
try:
|
try:
|
||||||
spec = importlib.util.spec_from_file_location(
|
spec = importlib.util.spec_from_file_location(
|
||||||
module_name, init_file, submodule_search_locations=[str(plugin_dir)]
|
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
|
"Failed to load %s provider plugin %s: %s", source, plugin_dir.name, exc
|
||||||
)
|
)
|
||||||
sys.modules.pop(module_name, None)
|
sys.modules.pop(module_name, None)
|
||||||
|
finally:
|
||||||
|
_current_source = None
|
||||||
|
|
||||||
|
|
||||||
def _discover_entry_point_providers() -> None:
|
def _discover_entry_point_providers() -> None:
|
||||||
|
|||||||
67
tests/hermes_cli/test_provider_registry_override.py
Normal file
67
tests/hermes_cli/test_provider_registry_override.py
Normal file
@@ -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/<bundled-name>/`` 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
|
||||||
@@ -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.
|
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
|
## api_mode selection
|
||||||
|
|
||||||
Four values are recognized. Hermes picks one based on:
|
Four values are recognized. Hermes picks one based on:
|
||||||
|
|||||||
Reference in New Issue
Block a user