From 520e63661c8eaa2135ebd60a07192f0d8aa45e6e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:08:26 -0700 Subject: [PATCH] fix: keep command-auth model discovery lazy across config and setup --- agent/command_token_source.py | 35 +- .../emails/hayden.moulds@rollerdigital.com | 2 + evals/key_cmd_picker_live.py | 182 +++++++ hermes_cli/config_providers.py | 5 +- hermes_cli/main_provider_setup.py | 1 + hermes_cli/model_setup_flows_custom.py | 23 +- hermes_cli/model_switch_providers.py | 74 ++- hermes_cli/models.py | 46 +- .../test_model_switch_custom_providers.py | 1 + .../test_picker_key_cmd_discovery.py | 459 +++++------------- website/docs/integrations/providers.md | 9 + 11 files changed, 405 insertions(+), 432 deletions(-) create mode 100644 contributors/emails/hayden.moulds@rollerdigital.com create mode 100644 evals/key_cmd_picker_live.py diff --git a/agent/command_token_source.py b/agent/command_token_source.py index 619db17c12..b4511ab3ae 100644 --- a/agent/command_token_source.py +++ b/agent/command_token_source.py @@ -16,7 +16,7 @@ import logging import subprocess import threading import time -from typing import Callable, Optional +from typing import Optional logger = logging.getLogger(__name__) @@ -116,6 +116,11 @@ class CommandTokenSource: self._token = "" self._expires_at: float = 0.0 + @property + def cache_identity(self) -> str: + """Stable catalog identity; token rotation must not mint on cache reads.""" + return f"cmd:{self._command}" + def __call__(self) -> str: with self._lock: if self._token and time.monotonic() < self._expires_at: @@ -132,33 +137,7 @@ class CommandTokenSource: return token -def build_command_token_provider(key_cmd: str, provider_label: str = "custom") -> Optional[Callable[[], str]]: +def build_command_token_provider(key_cmd: str, provider_label: str = "custom") -> Optional[CommandTokenSource]: """A per-request token provider for *key_cmd*, or ``None`` when unset.""" command = str(key_cmd or "").strip() return CommandTokenSource(command, provider_label) if command else None - - -def resolve_probe_token(entry: dict) -> str: - """Mint a one-shot credential from a provider entry's ``key_cmd``, or "". - - For callers needing a CONCRETE token rather than the per-request callable - ``build_command_token_provider`` returns — the ``/models`` catalog probes, - which build their request by hand instead of going through a wire client. - Shares the ``CommandTokenSource`` cache with the request path, so this is - a cache read rather than a fresh sign-in. - - Fail-closed: any error yields "". A helper that needs an interactive - sign-in (or is simply broken) must not take down a whole picker — the - caller degrades to the pre-existing empty-key behaviour and every other - provider still renders. - """ - if not isinstance(entry, dict): - return "" - command = str(entry.get("key_cmd", "") or "").strip() - if not command: - return "" - try: - provider = build_command_token_provider(command, str(entry.get("name", "") or "custom")) - return (provider() or "").strip() if provider is not None else "" - except Exception: - return "" diff --git a/contributors/emails/hayden.moulds@rollerdigital.com b/contributors/emails/hayden.moulds@rollerdigital.com new file mode 100644 index 0000000000..6a5626a22b --- /dev/null +++ b/contributors/emails/hayden.moulds@rollerdigital.com @@ -0,0 +1,2 @@ +haydster7 +# PR #99893 salvage diff --git a/evals/key_cmd_picker_live.py b/evals/key_cmd_picker_live.py new file mode 100644 index 0000000000..5829329bb0 --- /dev/null +++ b/evals/key_cmd_picker_live.py @@ -0,0 +1,182 @@ +#!/usr/bin/env python3 +"""Live key_cmd catalog A/B: local authenticated HTTP, real helper, CLI PTY. + +Run with the Hermes venv Python and --repo CHECKOUT --output RECEIPT_DIR. +No user environment/config is inherited by Hermes children. Unix PTY required. +""" +import argparse +import http.server +import json +import os +from pathlib import Path +import pty +import re +import select +import shlex +import subprocess +import sys +import tempfile +import threading +import time + +CATALOG = ["live-configured", "live-discovered-b", "live-discovered-c"] +TOKEN = "local-eval-token-not-a-secret" + + +def rows_worker(): + from hermes_cli.config import load_config + from hermes_cli.model_switch_providers import ( + _PickerBuild, _lap_custom_provider_rows, _lap_user_provider_rows, + ) + cfg = load_config() + b = _PickerBuild(current_provider="", current_base_url="", current_model="", + max_models=None, for_picker=True, force_fresh_nous_tier=False, + probe_custom_providers=True, probe_current_custom_provider=False, + refresh=False, excluded=set(), curated={}) + if cfg.get("providers"): + _lap_user_provider_rows(b, cfg["providers"]) + else: + _lap_custom_provider_rows(b, cfg["custom_providers"]) + print("RECEIPT_ROWS=" + json.dumps(b.results)) + + +def cli_pty(repo, env): + master, slave = pty.openpty() + import fcntl + import struct + import termios + fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 45, 140, 0, 0)) + proc = subprocess.Popen([sys.executable, "-m", "hermes_cli.main", "model"], + cwd=repo, env=env, stdin=slave, stdout=slave, stderr=slave, + start_new_session=True) + os.close(slave) + data = bytearray() + selected = False + deadline = time.monotonic() + 45 + try: + while time.monotonic() < deadline: + if select.select([master], [], [], 0.2)[0]: + try: + chunk = os.read(master, 65536) + except OSError: + break + if not chunk: + break + data.extend(chunk) + text = data.decode(errors="replace") + if not selected and ("Select provider:" in text or "Choice [1-" in text): + os.write(master, b"\r") + selected = True + if selected and (re.search(r"Found \d+ model", text) + or "Could not fetch models" in text): + # Let the actual picker render before cancelling; never save a selection. + end = time.monotonic() + 0.7 + while time.monotonic() < end: + if select.select([master], [], [], 0.1)[0]: + data.extend(os.read(master, 65536)) + os.write(master, b"\x03") + break + if proc.poll() is not None: + break + finally: + if proc.poll() is None: + proc.terminate() + proc.wait(timeout=10) + os.close(master) + text = data.decode(errors="replace") + found = re.search(r"Found (\d+) model", text) + if not found: + raise RuntimeError("CLI failed to reach model picker:\n" + text[-12000:]) + return int(found.group(1)), text + + +def main(): + ap = argparse.ArgumentParser(description=__doc__) + ap.add_argument("--repo", type=Path, required=True) + ap.add_argument("--output", type=Path, required=True) + ap.add_argument("--expect", choices=["before", "after"]) + args = ap.parse_args() + repo = args.repo.resolve() + args.output.mkdir(parents=True, exist_ok=True) + requests = [] + phase = "" + + class Handler(http.server.BaseHTTPRequestHandler): + def do_GET(self): + authorized = self.headers.get("Authorization") == "Bearer " + TOKEN + requests.append({"phase": phase, "path": self.path, "authorized": authorized}) + body = json.dumps({"data": [{"id": m} for m in CATALOG]} if authorized + else {"error": "authorization required"}).encode() + self.send_response(200 if authorized else 401) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(body))) + self.end_headers() + self.wfile.write(body) + + def log_message(self, format, *args): + return + + server = http.server.ThreadingHTTPServer(("127.0.0.1", 0), Handler) + thread = threading.Thread(target=server.serve_forever, daemon=True) + thread.start() + results = {} + try: + for schema in ("providers", "custom_providers"): + for surface in ("cli", "rows"): + phase = f"{schema}/{surface}" + with tempfile.TemporaryDirectory(prefix="hermes-keycmd-live-") as tmp: + home = Path(tmp) + state = home / ".hermes" + state.mkdir() + mint_log = home / "helper-invocations" + helper = home / "mint.py" + helper.write_text("from pathlib import Path\n" + f"with Path({str(mint_log)!r}).open('a') as f: f.write('mint\\n')\n" + f"print({TOKEN!r})\n") + entry = {"name": "Live Keycmd", "base_url": f"http://127.0.0.1:{server.server_port}/v1", + "key_cmd": f"{shlex.quote(sys.executable)} {shlex.quote(str(helper))}", + "model": CATALOG[0], "models": {CATALOG[0]: {}}, + "models_discovered": True} + slug = "live-keycmd" if schema == "providers" else "custom:live-keycmd" + cfg = {"model": {"provider": slug, "default": CATALOG[0]}, + schema: {slug: entry} if schema == "providers" else [entry]} + # JSON is valid YAML; no third-party harness dependencies. + (state / "config.yaml").write_text(json.dumps(cfg)) + env = {"HOME": str(home), "HERMES_HOME": str(state), + "PATH": "/usr/bin:/bin", "TERM": "xterm-256color", "LANG": "C.UTF-8", + "PYTHONPATH": str(repo), "PYTHONUNBUFFERED": "1"} + if surface == "cli": + count, transcript = cli_pty(repo, env) + results[phase] = {"model_count": count} + else: + code = ("import runpy; ns=runpy.run_path(" + repr(str(Path(__file__).resolve())) + + "); ns['rows_worker']()") + proc = subprocess.run([sys.executable, "-c", code], cwd=repo, env=env, + stdin=subprocess.DEVNULL, capture_output=True, text=True, timeout=45) + transcript = proc.stdout + proc.stderr + if proc.returncode: + raise RuntimeError(transcript) + rows = json.loads(next(s.split("=", 1)[1] for s in proc.stdout.splitlines() + if s.startswith("RECEIPT_ROWS="))) + assert len(rows) == 1, rows + results[phase] = {"model_count": rows[0]["total_models"], "models": rows[0]["models"]} + results[phase]["helper_invocations"] = len(mint_log.read_text().splitlines()) if mint_log.exists() else 0 + (args.output / (phase.replace("/", "-") + ".txt")).write_text(transcript) + receipt = {"repo": str(repo), "sha": subprocess.check_output( + ["git", "rev-parse", "HEAD"], cwd=repo, text=True).strip(), + "server_catalog": CATALOG, "results": results, "requests": requests} + (args.output / "receipt.json").write_text(json.dumps(receipt, indent=2) + "\n") + print(json.dumps(receipt, indent=2)) + if args.expect: + expected = 1 if args.expect == "before" else len(CATALOG) + assert all(row["model_count"] == expected for row in results.values()), results + assert all(row["helper_invocations"] > 0 for row in results.values()) if args.expect == "after" else True + assert all(any(r["phase"] == p and r["authorized"] == (args.expect == "after") + for r in requests) for p in results), requests + finally: + server.shutdown() + server.server_close() + + +if __name__ == "__main__": + main() diff --git a/hermes_cli/config_providers.py b/hermes_cli/config_providers.py index 3425c60fb2..4e6e9dd1d6 100644 --- a/hermes_cli/config_providers.py +++ b/hermes_cli/config_providers.py @@ -230,6 +230,7 @@ def _normalize_custom_provider_entry( normalized[field] = value _put("api_key", _stripped("api_key")) + _put("key_cmd", _stripped("key_cmd")) key_env = _stripped("key_env", "api_key_env") _put("key_env", key_env) if key_env and entry.get("api_key_env") and not entry.get("key_env"): @@ -253,7 +254,7 @@ def _normalize_custom_provider_entry( for field, ok in ( ("context_length", lambda v: isinstance(v, int) and v > 0), ("rate_limit_delay", lambda v: isinstance(v, (int, float)) and v >= 0), - ("discover_models", lambda v: isinstance(v, bool)), + ("discover_models", lambda v: isinstance(v, (bool, str))), ): if ok(entry.get(field)): normalized[field] = entry[field] @@ -282,7 +283,7 @@ def _custom_provider_entry_to_provider_config( provider_entry: Dict[str, Any] = {"api": normalized["base_url"]} for field in ( - "name", "api_key", "key_env", "models", "models_discovered", "context_length", + "name", "api_key", "key_env", "key_cmd", "models", "models_discovered", "context_length", "rate_limit_delay", "discover_models", "extra_body", "extra_headers", "ssl_ca_cert", "ssl_verify"): if field in normalized: diff --git a/hermes_cli/main_provider_setup.py b/hermes_cli/main_provider_setup.py index 9cdd3d8c89..863fbf343a 100644 --- a/hermes_cli/main_provider_setup.py +++ b/hermes_cli/main_provider_setup.py @@ -758,6 +758,7 @@ def _named_custom_provider_map(cfg) -> dict[str, dict[str, str]]: "base_url": base_url, "api_key": entry.get("api_key", ""), "key_env": entry.get("key_env") or entry.get("api_key_env", ""), + "key_cmd": entry.get("key_cmd", ""), "model": model, "models": entry.get("models", {}), "models_discovered": entry.get("models_discovered", False), diff --git a/hermes_cli/model_setup_flows_custom.py b/hermes_cli/model_setup_flows_custom.py index e94e0b79ca..cd1f53e9b1 100644 --- a/hermes_cli/model_setup_flows_custom.py +++ b/hermes_cli/model_setup_flows_custom.py @@ -214,6 +214,9 @@ def _discover_named_custom_models(provider_info: dict, api_key: str, configured_ should_use_ollama_native_catalog, ) + from agent.command_token_source import build_command_token_provider, materialize_probe_api_key + source = build_command_token_provider(provider_info.get("key_cmd", ""), provider_info["name"]) + api_key = materialize_probe_api_key(source if source is not None else api_key) name, base_url = provider_info["name"], provider_info["base_url"] api_mode = provider_info.get("api_mode", "") provider_key = (provider_info.get("provider_key") or "").strip() @@ -254,8 +257,10 @@ def _discover_named_custom_models(provider_info: dict, api_key: str, configured_ # _save_discovered_models_to_config. A failed save is non-fatal. if live_models: with contextlib.suppress(Exception): - from hermes_cli.model_switch_providers import _save_discovered_models_to_config - _save_discovered_models_to_config(base_url, live_models, api_mode=api_mode, headers=extra_headers or None) + from hermes_cli.model_switch_providers import _entry_credentials, _save_discovered_models_to_config + _save_discovered_models_to_config( + base_url, live_models, api_mode=api_mode, headers=extra_headers or None, + credential_identity=_entry_credentials(provider_info, "key_env", "api_key_env")[2]) return models, native_catalog_empty @@ -305,20 +310,8 @@ def _model_flow_named_custom(config, provider_info): # Resolve key from env var if api_key not set directly if not api_key and key_env: api_key = os.environ.get(key_env, "") - # What gets PERSISTED is derived from the statically-configured credential - # only. A key_cmd token is short-lived and must never be written back into - # config.yaml — it would be stale within the hour and would shadow the - # key_cmd that is supposed to re-mint it. Hence: before the mint below. + # Only configured credentials may be persisted, never a short-lived probe token. config_api_key = _custom_provider_api_key_config_value(provider_info, api_key) - if not api_key: - # Command-minted credential (key_cmd) — same precedence as the request - # path and the picker: after api_key/key_env, so an explicit static key - # still wins. Without this the probe below sends no Authorization - # header, an authenticated endpoint 401s, and the flow falls back to - # the single saved model. - from agent.command_token_source import resolve_probe_token - - api_key = resolve_probe_token(provider_info) # ``discover_models: false`` (default True) uses the configured ``models:`` list # verbatim and skips the live probe, so operators can restrict the picker to the diff --git a/hermes_cli/model_switch_providers.py b/hermes_cli/model_switch_providers.py index 46fa58c44d..0d1c408b0f 100644 --- a/hermes_cli/model_switch_providers.py +++ b/hermes_cli/model_switch_providers.py @@ -13,7 +13,7 @@ import time import threading as _threading from dataclasses import dataclass, field from typing import Any, List, Optional -from agent.command_token_source import resolve_probe_token +from agent.command_token_source import build_command_token_provider, materialize_probe_api_key from hermes_cli.providers import custom_provider_aliases, custom_provider_slug, get_label from utils import base_url_host_matches @@ -26,7 +26,7 @@ _UNCAPPED_PICKER_PROVIDERS: frozenset[str] = frozenset({"opencode-zen", "opencod def _save_discovered_models_to_config( api_url: str, model_ids: list[str], *, api_mode: Optional[str] = None, - headers: Optional[dict[str, str]] = None) -> None: + headers: Optional[dict[str, str]] = None, credential_identity: str | None = None) -> None: """Persist a successful ``/v1/models`` probe into the matching ``custom_providers`` entry. Matches by base_url (slash-normalised), api_mode and headers. A failed config write is @@ -51,6 +51,8 @@ def _save_discovered_models_to_config( continue if headers is not None and _extra_headers_from_config(entry) != headers: continue + if credential_identity is not None and _entry_credentials(entry, "key_env", "api_key_env")[2] != credential_identity: + continue if not _discovered_catalog_stale(entry, model_ids): continue entry["models"] = {model_id: {} for model_id in model_ids} @@ -86,17 +88,25 @@ class _NativePickerModelList(list[str]): def _fetch_picker_live_models( - api_key: str, api_url: str, native_catalog_provider: str, preserve_native_models: bool, + api_key: Any, api_url: str, native_catalog_provider: str, preserve_native_models: bool, headers: dict[str, str] | None = None, timeout: float = 5.0, - api_mode: str | None = None) -> list[str] | None: + api_mode: str | None = None, *, cache: bool = True) -> list[str] | None: """Fetch picker models with native Ollama and cached generic discovery.""" - from hermes_cli.models import _get_ollama_native_headers, cached_fetch_api_models + from hermes_cli.models import _get_ollama_native_headers, cached_fetch_api_models, fetch_api_models from hermes_cli.models_local import ( _normalize_openai_base_url, fetch_ollama_local_models, should_use_ollama_native_catalog, ) + if callable(api_key): + # Let the catalog cache decide whether any token or network I/O is needed. + return cached_fetch_api_models( + api_key, api_url, timeout=timeout, headers=headers, api_mode=api_mode, + fetch_models=lambda: _fetch_picker_live_models( + materialize_probe_api_key(api_key), api_url, native_catalog_provider, + preserve_native_models, headers, timeout, api_mode, cache=False)) + candidate_headers = _get_ollama_native_headers(api_url, api_key=api_key) def _drop(pred) -> None: @@ -124,7 +134,7 @@ def _fetch_picker_live_models( return _NativePickerModelList(native_models) # A failed native probe is not authoritative: retry the cached generic catalog. api_url = _normalize_openai_base_url(api_url) - generic_models = cached_fetch_api_models( + generic_models = (cached_fetch_api_models if cache else fetch_api_models)( api_key, api_url, timeout=timeout, headers=resolved_headers, api_mode=api_mode) return generic_models if generic_models or use_native else None @@ -468,31 +478,14 @@ def _entry_api_mode(entry: dict) -> str | None: return str(entry.get("api_mode") or entry.get("transport") or "").strip().lower() or None -def _entry_credentials(entry: dict, *key_env_keys: str) -> tuple[str, str, str]: - """``(inline_api_key, key_env, identity)`` — identity is the inline key, else - ``env:``, else ``cmd:``, else "". - - ``key_cmd`` (#86891) authenticates a provider with a SHORT-LIVED bearer - minted by a command — SSO/OIDC brokers, cloud IAM, internal auth proxies. - The request path has honoured it since it landed, but the picker resolved - probe credentials from ``api_key``/``key_env`` only, so a key_cmd provider - probed ``/v1/models`` with an EMPTY key. An authenticated endpoint answers - 401, discovery returns nothing, and the row collapses to its single - configured default model — indistinguishable from an endpoint that - genuinely serves one, while inference keeps working. - - The identity is keyed on the COMMAND, never the minted token: the token - rotates on every refresh, so keying on its value would change the group - fingerprint constantly and force a re-probe on every open. Two entries on - one URL with different helpers still get distinct rows. - """ - inline_api_key = str(entry.get("api_key", "") or "").strip() +def _entry_credentials(entry: dict, *key_env_keys: str) -> tuple[Any, str, str]: + """Unminted credential, env fallback, and stable grouping identity.""" key_env = str(next((entry.get(k) for k in key_env_keys if entry.get(k)), "")).strip() - key_cmd = str(entry.get("key_cmd", "") or "").strip() - identity = inline_api_key or ( - f"env:{key_env}" if key_env else (f"cmd:{key_cmd}" if key_cmd else "") - ) - return inline_api_key, key_env, identity + source = build_command_token_provider(entry.get("key_cmd", ""), entry.get("name") or "custom") + if source is not None: + return source, key_env, source.cache_identity + inline_api_key = str(entry.get("api_key", "") or "").strip() + return inline_api_key, key_env, inline_api_key or (f"env:{key_env}" if key_env else "") def _discover_flag(entry: dict): @@ -521,7 +514,7 @@ def _group_display_name(display_name: str) -> str: def _discover_endpoint_models( - api_key: str, api_url: str, native_catalog_provider: str, has_explicit_models: bool, *, + api_key: Any, api_url: str, native_catalog_provider: str, has_explicit_models: bool, *, headers: dict | None, api_mode: str | None, probe_live: bool, discovery_allowed: bool, for_picker: bool) -> tuple[list | None, bool]: """Return ``(models, native_catalog_empty)`` for a custom endpoint row. @@ -538,6 +531,8 @@ def _discover_endpoint_models( api_key, api_url, native_catalog_provider, has_explicit_models, headers=headers, timeout=timeout, api_mode=api_mode) is_native = isinstance(live_models, _NativePickerModelList) + if is_native and has_explicit_models: + return None, False if live_models is not None and (live_models or not has_explicit_models or is_native): return live_models, (is_native and not live_models) except Exception: @@ -548,8 +543,10 @@ def _discover_endpoint_models( cached_models = cached_fetch_api_models( api_key, api_url, cache_only=True, timeout=timeout, headers=headers, api_mode=api_mode, ) - if cached_models: - return cached_models, False + if has_explicit_models and isinstance(cached_models, _NativePickerModelList): + return None, False + if cached_models or isinstance(cached_models, _NativePickerModelList): + return cached_models, isinstance(cached_models, _NativePickerModelList) and not cached_models except (ImportError, OSError, RuntimeError, TimeoutError, TypeError, ValueError, http.client.HTTPException): pass return None, False @@ -689,7 +686,7 @@ class _PickerBuild: and url_norm == self.current_base_url_norm and url_match_ok)) def discover_endpoint( - self, api_key: str, api_url: str, native_provider: str, has_explicit_models: bool, *, + self, api_key: Any, api_url: str, native_provider: str, has_explicit_models: bool, *, headers: dict | None, api_mode: str | None, discovery_allowed: bool, is_current: bool, ) -> tuple[list | None, bool, bool]: """Probe policy shared by sections 3 and 4 (returns ``(models, native_empty, probed)``): @@ -864,7 +861,7 @@ def _lap_user_provider_rows(b: _PickerBuild, user_providers: dict) -> None: ep_groups[group_key] = { "slug": ep_name, "name": _group_display_name(display_name), "api_url": api_url, "models": [], "has_explicit_models": False, - "api_key": inline_api_key or _scoped_key_env(key_env) or resolve_probe_token(ep_cfg), + "api_key": inline_api_key or _scoped_key_env(key_env), "headers": headers, "api_mode": ep_cfg.get("api_mode"), "discovery_allowed": bool(api_url) and _discover_flag(ep_cfg), "raw_names": [], "aliases": set()} grp = ep_groups[group_key] @@ -944,7 +941,7 @@ def _lap_custom_provider_rows(b: _PickerBuild, custom_providers: list) -> None: if not raw_name or not api_url: continue inline_api_key, key_env, cred_identity = _entry_credentials(entry, "key_env") - api_key = inline_api_key or _scoped_key_env(key_env) or resolve_probe_token(entry) + api_key = inline_api_key or _scoped_key_env(key_env) api_mode = _entry_api_mode(entry) discover = _discover_flag(entry) entry_extra_headers = _extra_headers_from_config(entry) @@ -954,7 +951,7 @@ def _lap_custom_provider_rows(b: _PickerBuild, custom_providers: list) -> None: display_name = prefix or raw_name grp = groups.setdefault(group_key, { "slug": custom_provider_slug(display_name, provider_key), "name": display_name, - "api_url": api_url, "api_key": "", "models": [], "has_explicit_models": False, + "api_url": api_url, "api_key": "", "credential_identity": cred_identity, "models": [], "has_explicit_models": False, "discover_models": True, "api_mode": api_mode, "extra_headers": entry_extra_headers, "aliases": set()}) grp["api_key"] = grp["api_key"] or api_key # first member with a key wins @@ -1001,7 +998,8 @@ def _lap_custom_provider_rows(b: _PickerBuild, custom_providers: list) -> None: if probe_live: # a successful live probe persists the catalog for no-probe surfaces try: _save_discovered_models_to_config( - api_url, discovered, api_mode=grp.get("api_mode"), headers=grp.get("extra_headers") or None) + api_url, discovered, api_mode=grp.get("api_mode"), headers=grp.get("extra_headers") or None, + credential_identity=grp["credential_identity"]) except Exception: pass b.add_endpoint_row(slug, grp["name"], grp["api_url"], grp["models"], is_current, native_catalog_empty) diff --git a/hermes_cli/models.py b/hermes_cli/models.py index eb9bea8ee8..e8fa909234 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -2312,13 +2312,15 @@ def fetch_api_models( def _custom_endpoint_fingerprint( - api_key: Optional[str], api_mode: Optional[str], headers: Optional[dict[str, str]]) -> str: + api_key: Any, api_mode: Optional[str], headers: Optional[dict[str, str]]) -> str: """Custom endpoints have no ``PROVIDER_REGISTRY`` slug, so hash exactly what callers pass to :func:`fetch_api_models`: a rotated ``api_key``, changed ``api_mode`` or edited ``extra_headers`` each bust the cache entry. blake2b for the same CodeQL rationale as ``_credential_fingerprint``.""" import hashlib - blob = "|".join((api_key or "", api_mode or "", json.dumps(headers or {}, sort_keys=True))) + from agent.command_token_source import CommandTokenSource + identity = api_key.cache_identity if isinstance(api_key, CommandTokenSource) else api_key + blob = "|".join((identity or "", api_mode or "", json.dumps(headers or {}, sort_keys=True))) return hashlib.blake2b(blob.encode("utf-8", errors="replace"), digest_size=8).hexdigest() @@ -2337,15 +2339,28 @@ def _cache_entry_valid( def cached_fetch_api_models( - api_key: Optional[str], base_url: Optional[str], *, timeout: float = 5.0, + api_key: Any, base_url: Optional[str], *, timeout: float = 5.0, api_mode: Optional[str] = None, headers: Optional[dict[str, str]] = None, force_refresh: bool = False, cache_only: bool = False, + fetch_models=None, ttl_seconds: int = _PROVIDER_MODELS_CACHE_TTL) -> Optional[list[str]]: """Disk-cached :func:`fetch_api_models` for custom endpoints. ``cache_only`` callers (GUI picker opens that must not block on a stopped local endpoint) still get a warm catalog instead of - collapsing to the config-declared subset.""" + collapsing to the config-declared subset. ``fetch_models`` supplies native-aware discovery + without minting a command token before cache admission.""" + from hermes_cli.model_switch_providers import _NativePickerModelList + + def _catalog(entry): + return (_NativePickerModelList if entry.get("native_catalog") else list)(entry["models"]) + + def _entry(live, at=None): + return {**_cache_entry(fp, live, at), "native_catalog": isinstance(live, _NativePickerModelList)} + def _live(): - return fetch_api_models(api_key, base_url, timeout=timeout, api_mode=api_mode, headers=headers) + if fetch_models is not None: + return fetch_models() + from agent.command_token_source import materialize_probe_api_key + return fetch_api_models(materialize_probe_api_key(api_key), base_url, timeout=timeout, api_mode=api_mode, headers=headers) normalized_url = str(base_url or "").strip().rstrip("/").lower() if not normalized_url: # nothing to key the cache on @@ -2356,32 +2371,33 @@ def cached_fetch_api_models( cache = _load_provider_models_cache() entry = cache.get(cache_key) now = time.time() - valid = not force_refresh and _cache_entry_valid(entry, fp) + valid = not force_refresh and _cache_entry_valid(entry, fp, allow_empty=isinstance(entry, dict) and entry.get("native_catalog") is True) if cache_only: # Same trust window as the SWR tier below, minus the revalidation. - return list(entry["models"]) if valid and now - entry["at"] < _PROVIDER_MODELS_STALE_SERVE_MAX else None + return _catalog(entry) if valid and now - entry["at"] < _PROVIDER_MODELS_STALE_SERVE_MAX else None if valid: age = now - entry["at"] if age < ttl_seconds: - return list(entry["models"]) + return _catalog(entry) if age < _PROVIDER_MODELS_STALE_SERVE_MAX: # Stale-while-revalidate: serve now, refresh off-thread for the next open. def _refresh_custom(): live = _live() - return _cache_entry(fp, live) if live else None + return _entry(live) if live or isinstance(live, _NativePickerModelList) else None _spawn_swr_refresh(cache_key, _refresh_custom) - return list(entry["models"]) + return _catalog(entry) live = _live() - if live: - _store_cache_entry(cache_key, _cache_entry(fp, live, now), cache) - return list(live) + if live or isinstance(live, _NativePickerModelList): + stored = _entry(live, now) + _store_cache_entry(cache_key, stored, cache) + return _catalog(stored) # Live returned nothing (offline, timeout, auth hiccup): a stale same-fingerprint entry beats it. - if _cache_entry_valid(entry, fp): - return list(entry["models"]) + if _cache_entry_valid(entry, fp, allow_empty=isinstance(entry, dict) and entry.get("native_catalog") is True): + return _catalog(entry) return live diff --git a/tests/hermes_cli/test_model_switch_custom_providers.py b/tests/hermes_cli/test_model_switch_custom_providers.py index 3fe2511766..b4262c01a2 100644 --- a/tests/hermes_cli/test_model_switch_custom_providers.py +++ b/tests/hermes_cli/test_model_switch_custom_providers.py @@ -1602,6 +1602,7 @@ def test_model_flow_named_custom_persists_discovered_models(monkeypatch): { "api_mode": "anthropic_messages", "headers": {"X-Tenant": "dragomes"}, + "credential_identity": "sk-test", }, ) ], ( diff --git a/tests/hermes_cli/test_picker_key_cmd_discovery.py b/tests/hermes_cli/test_picker_key_cmd_discovery.py index 98307aae7e..97c8aeef1b 100644 --- a/tests/hermes_cli/test_picker_key_cmd_discovery.py +++ b/tests/hermes_cli/test_picker_key_cmd_discovery.py @@ -1,334 +1,125 @@ -"""The model picker must probe with a ``key_cmd``-minted credential. - -``key_cmd`` (#86891) lets a provider authenticate with a SHORT-LIVED bearer -minted by a command — SSO/OIDC brokers, cloud IAM, internal auth proxies. The -request path has honoured it since it landed, but the picker resolved probe -credentials from ``api_key``/``key_env`` ONLY. - -The failure is quiet and easy to misread. The picker probes ``/v1/models`` with -an EMPTY key, an authenticated endpoint answers 401, discovery returns nothing, -and the provider falls back to its single configured ``default_model``. The -user sees ONE model and cannot tell that apart from an endpoint that genuinely -serves one — inference itself keeps working, because that path mints correctly. - -The same gap existed in the ``hermes model`` setup flow -(``_model_flow_named_custom``), which builds its own ``Authorization: Bearer`` -header from the same incomplete resolution — same bug class, sibling path. - -These tests pin: - -* a ``key_cmd`` entry probes with the minted token, so the full catalog shows; -* the cache fingerprint is keyed on the COMMAND, not the minted token (which - rotates every refresh — keying on it would re-probe on every open); -* a broken/interactive helper degrades to the pre-existing empty-key behaviour - rather than taking the whole picker down; -* a minted token is NEVER persisted back into ``config.yaml`` — it would be - stale within the hour and would shadow the ``key_cmd`` meant to re-mint it. -""" - -from __future__ import annotations - -from agent.command_token_source import resolve_probe_token - - -class TestResolveProbeToken: - """The shared credential helper both probe paths call.""" - - def test_bare_token_stdout(self): - assert resolve_probe_token( - {"key_cmd": "printf 'tok-abc'", "name": "gw"} - ) == "tok-abc" - - def test_json_access_token(self): - """The OAuth 2.0 token-endpoint response shape.""" - entry = { - "key_cmd": """printf '{"access_token":"tok-json","expires_in":3600}'""", - "name": "gw", - } - assert resolve_probe_token(entry) == "tok-json" - - def test_absent_key_cmd_is_empty(self): - """No key_cmd — the caller falls through to api_key/key_env.""" - assert resolve_probe_token({"name": "gw"}) == "" - - def test_blank_key_cmd_is_empty(self): - assert resolve_probe_token({"key_cmd": " ", "name": "gw"}) == "" - - def test_failing_helper_degrades_to_empty(self): - """A helper that needs an interactive sign-in (or is simply broken) - must not take down the picker: every other provider's row still - renders, and this one degrades to the old empty-key behaviour.""" - assert resolve_probe_token({"key_cmd": "exit 1", "name": "gw"}) == "" - - def test_silent_helper_is_empty(self): - assert resolve_probe_token({"key_cmd": "true", "name": "gw"}) == "" - - def test_multiline_output_is_rejected(self): - """command_token_source refuses to guess which line is the token.""" - assert resolve_probe_token( - {"key_cmd": "printf 'a\\nb'", "name": "gw"} - ) == "" - - -class TestPickerProbesWithMintedCredential: - """End-to-end: a key_cmd provider lists its full catalog, not just one.""" - - def _probe_capture(self, monkeypatch): - """Record the api_key the picker hands the live /models probe.""" - seen: dict = {} - - # The real callee takes keyword extras (headers, timeout, api_mode); - # the probe is wrapped in `except Exception: pass`, so a stub with a - # narrower signature would be silently swallowed and look like "the - # probe never ran". - # - # Behave like a real authenticated endpoint: no credential -> no - # catalog. A stub that returns models regardless would still pass - # unpatched (the caller falls back to default_model), so the - # model-count assertions below would prove nothing. - def fake_fetch(api_key, api_url, provider, preserve_native_models, **kwargs): - seen["api_key"] = api_key - seen["api_url"] = api_url - if not api_key: - return None # what a 401 looks like to the picker - return ["model-a", "model-b", "model-c"] - - monkeypatch.setattr( - "hermes_cli.model_switch_providers._fetch_picker_live_models", fake_fetch - ) - return seen - - def test_probe_receives_the_minted_token(self, monkeypatch): - from hermes_cli.model_switch import list_authenticated_providers - - seen = self._probe_capture(monkeypatch) - rows = list_authenticated_providers( - user_providers={ - "gw": { - "base_url": "https://gw.example.test/v1", - "api_mode": "chat_completions", - "key_cmd": "printf 'tok-minted'", - "default_model": "model-a", - } - }, - refresh=True, - for_picker=True, - ) - - assert seen.get("api_key") == "tok-minted", ( - "picker probed with an empty key — a key_cmd endpoint 401s and " - "collapses to its single default_model" - ) - gw = [r for r in rows if isinstance(r, dict) and r.get("slug") == "gw"] - assert gw, "key_cmd provider missing from the picker entirely" - # The user-visible symptom: unpatched this is 1 (just default_model). - assert len(gw[0].get("models") or []) == 3 - - def test_static_api_key_still_wins(self, monkeypatch): - """key_cmd is a FALLBACK here: an explicit api_key is used as-is, so - this change cannot alter behaviour for existing static-key configs.""" - from hermes_cli.model_switch import list_authenticated_providers - - seen = self._probe_capture(monkeypatch) - list_authenticated_providers( - user_providers={ - "gw": { - "base_url": "https://gw.example.test/v1", - "api_key": "sk-static", - "key_cmd": "printf 'tok-minted'", - "default_model": "model-a", - } - }, - refresh=True, - for_picker=True, - ) - - assert seen.get("api_key") == "sk-static" - - -class TestCacheFingerprintStability: - """The picker's cache key must not rotate with the token.""" - - def test_fingerprint_keys_on_command_not_token(self, monkeypatch): - """A helper minting a DIFFERENT token each call must still produce a - stable cache fingerprint. Keying on the minted value would change the - fingerprint on every refresh and force a re-probe on every open.""" - from hermes_cli.model_switch import list_authenticated_providers - - calls = {"n": 0} - - def fake_fetch(api_key, api_url, provider, preserve_native_models, **kwargs): - calls["n"] += 1 - return ["model-a", "model-b"] - - monkeypatch.setattr( - "hermes_cli.model_switch_providers._fetch_picker_live_models", fake_fetch - ) - - # $RANDOM would vary per call; use a counter file-free equivalent that - # is deterministic per call but different between calls. - providers = { - "gw": { - "base_url": "https://gw.example.test/v1", - "key_cmd": "printf 'tok-%s' $$", # PID: differs per invocation - "default_model": "model-a", - } - } - - first = list_authenticated_providers( - user_providers=providers, refresh=True, for_picker=True - ) - second = list_authenticated_providers( - user_providers=providers, refresh=True, for_picker=True - ) - - def row(rows): - return [r for r in rows if isinstance(r, dict) and r.get("slug") == "gw"] - - assert row(first) and row(second) - assert (row(first)[0].get("models") or []) == ( - row(second)[0].get("models") or [] - ) - - -class TestMintedTokenIsNeverPersisted: - """A key_cmd token must not be written back into config.yaml. - - ``_model_flow_named_custom`` computes the value to persist from the - STATICALLY configured credential. Resolving key_cmd into ``api_key`` before - that call would persist a short-lived bearer, which is stale within the - hour and shadows the key_cmd that exists to re-mint it. - """ - - def test_key_cmd_provider_persists_no_credential(self): - from hermes_cli.main_provider_setup import _custom_provider_api_key_config_value - - assert _custom_provider_api_key_config_value( - {"key_cmd": "printf 'tok-secret'"}, "" - ) == "" - - def test_static_key_still_persists(self): - from hermes_cli.main_provider_setup import _custom_provider_api_key_config_value - - assert _custom_provider_api_key_config_value( - {"api_key": "sk-static"}, "sk-static" - ) == "sk-static" - - def test_key_env_persists_as_reference(self): - """key_env persists as ${VAR}, never the resolved secret.""" - from hermes_cli.main_provider_setup import _custom_provider_api_key_config_value - - assert _custom_provider_api_key_config_value( - {"key_env": "MY_KEY"}, "resolved-secret-value" - ) == "${MY_KEY}" - - -class TestSetupFlowHonoursKeyCmd: - """`hermes model`'s named-custom flow is the picker's sibling path. - - It builds its own ``Authorization: Bearer `` for the /models - probe, so an unresolved key_cmd sends no auth header and the endpoint - 401s — same symptom, different call path. - - These drive the real ``_model_flow_named_custom`` and assert on what the - probe actually receives, rather than inspecting the function's source: a - semantics-preserving refactor should not fail the suite. - """ - - class _StopAfterProbe(Exception): - """Unwind once the probe has run, before the interactive menu.""" - - def _run_flow_capturing_probe(self, monkeypatch, entry): - """Invoke the flow far enough to capture the probe's credential.""" - # Patch the modules the flow imports FROM, not the compat shims: the - # `hermes_cli.models` re-export of should_use_ollama_native_catalog is - # scheduled for removal (see HermesPluginCompatWarning). - import hermes_cli.models as models_mod - import hermes_cli.models_local as models_local_mod - from hermes_cli.model_setup_flows_custom import _model_flow_named_custom - - seen = {} - - def fake_fetch(api_key, base_url, **kwargs): - seen["api_key"] = api_key - seen["base_url"] = base_url - # The flow would prompt interactively next; stop here. - raise TestSetupFlowHonoursKeyCmd._StopAfterProbe - - monkeypatch.setattr(models_mod, "fetch_api_models", fake_fetch) - # Ollama detection issues its own probe; force the generic path. - monkeypatch.setattr( - models_local_mod, "should_use_ollama_native_catalog", lambda *a, **k: False - ) - - try: - _model_flow_named_custom({}, dict(entry)) - except TestSetupFlowHonoursKeyCmd._StopAfterProbe: - pass - except Exception: - # Any other failure still tells us what the probe received; the - # assertions below decide whether that was correct. - pass - return seen - - def test_probe_receives_the_minted_token(self, monkeypatch): - seen = self._run_flow_capturing_probe( - monkeypatch, - { - "name": "gw", - "base_url": "https://gw.example.test/v1", - "key_cmd": "printf 'tok-minted'", - "model": "model-a", - }, - ) - - assert seen.get("api_key") == "tok-minted", ( - "setup flow probed with an empty key — its /models request goes " - "out unauthenticated and the endpoint 401s" - ) - - def test_static_api_key_still_wins(self, monkeypatch): - """key_cmd is a fallback: an explicit api_key is used unchanged, so - existing static-key configs are unaffected.""" - seen = self._run_flow_capturing_probe( - monkeypatch, - { - "name": "gw", - "base_url": "https://gw.example.test/v1", - "api_key": "sk-static", - "key_cmd": "printf 'tok-minted'", - "model": "model-a", - }, - ) - - assert seen.get("api_key") == "sk-static" - - def test_minted_token_is_not_persisted(self, monkeypatch, tmp_path): - """The value written back to config.yaml is derived from the STATIC - credential. Persisting a short-lived bearer would leave a stale key - that also shadows the key_cmd meant to re-mint it.""" - import hermes_cli.main_provider_setup as main_mod - - persisted = {} - real = main_mod._custom_provider_api_key_config_value - - def spy(provider_info, resolved_api_key=""): - out = real(provider_info, resolved_api_key) - persisted["value"] = out - return out - - monkeypatch.setattr(main_mod, "_custom_provider_api_key_config_value", spy) - - self._run_flow_capturing_probe( - monkeypatch, - { - "name": "gw", - "base_url": "https://gw.example.test/v1", - "key_cmd": "printf 'tok-minted'", - "model": "model-a", - }, - ) - - assert persisted.get("value", "") == "", ( - "a key_cmd-minted bearer reached the value persisted to config.yaml" - ) +"""Command-auth discovery is lazy, credential-scoped, and never persists bearers.""" + +import copy + +import pytest + +from hermes_cli import model_switch_providers as picker + + +@pytest.mark.parametrize("route", ["providers", "custom_providers"]) +@pytest.mark.parametrize("static", [{}, {"api_key": "stale"}, {"key_env": "TEST_GATEWAY_KEY"}]) +def test_picker_command_auth_is_lazy_and_credential_scoped(monkeypatch, route, static): + from agent import command_token_source + from hermes_cli import models + + monkeypatch.setenv("TEST_GATEWAY_KEY", "stale") + mints, probes = [], [] + + def mint(command, label): + mints.append(command) + if command == "broken": + raise RuntimeError("private helper output") + return f"token-{command}", 3600 + + def fetch(key, url, **kwargs): + probes.append(key) + return ["fallback", f"catalog-{key}"] if key.startswith("token-") else None + + monkeypatch.setattr(command_token_source, "_mint", mint) + monkeypatch.setattr(models, "fetch_api_models", fetch) + monkeypatch.setattr("hermes_cli.models_local.should_use_ollama_native_catalog", lambda *a, **k: False) + entry = {"name": "Gateway", "base_url": "https://gateway.invalid/v1", "key_cmd": "tenant-a", + "model": "fallback", "default_model": "fallback", **static} + + def rows(entries, *, probe=True): + b = picker._PickerBuild("", "", "", None, False, False, probe, False, False, set(), {}) + if route == "providers": + picker._lap_user_provider_rows(b, {f"gateway-{i}": e for i, e in enumerate(entries)}) + else: + picker._lap_custom_provider_rows(b, entries) + return b.results + + for disabled in [False, "false"]: + assert rows([{**entry, "discover_models": disabled}])[0]["models"] == ["fallback"] + assert rows([entry], probe=False)[0]["models"] == ["fallback"] + assert not mints and not probes + + first = rows([entry])[0]["models"] + assert first == ["fallback", "catalog-token-tenant-a"] + assert mints == ["tenant-a"] and probes == ["token-tenant-a"] + for probe in [False, True]: + assert rows([entry], probe=probe)[0]["models"] == first + assert mints == ["tenant-a"] and probes == ["token-tenant-a"] + + from hermes_cli.config import save_config, load_config + other = {**entry, "key_cmd": "tenant-b", "discover_models": False} + save_config({"custom_providers": [entry, other]}) + if route == "custom_providers": + rows([entry]) + assert load_config()["custom_providers"][1].get("models") is None + source = command_token_source.build_command_token_provider("tenant-a") + empty = picker._NativePickerModelList() + result = models.cached_fetch_api_models( + source, entry["base_url"], force_refresh=True, fetch_models=lambda: empty) + assert isinstance(result, picker._NativePickerModelList) and result == [] + cached_empty = models.cached_fetch_api_models(source, entry["base_url"], cache_only=True) + assert isinstance(cached_empty, picker._NativePickerModelList) and cached_empty == [] + for probe in [False, True]: + native_row = rows([entry], probe=probe)[0] + assert native_row["models"] == [] and native_row["native_catalog_empty"] + models.clear_provider_models_cache() + + distinct = rows([entry, {**entry, "key_cmd": "tenant-b"}]) + assert [r["models"] for r in distinct] == [first, ["fallback", "catalog-token-tenant-b"]] + assert rows([{**entry, "key_cmd": "broken"}])[0]["models"] == ["fallback"] + assert "stale" not in probes + + +@pytest.mark.parametrize("route", ["providers", "custom_providers"]) +@pytest.mark.parametrize("discover", [True, False, "false"]) +@pytest.mark.parametrize("static", [{}, {"api_key": "stale"}, {"key_env": "TEST_GATEWAY_KEY"}]) +def test_setup_probe_credentials_never_become_saved_credentials(monkeypatch, route, discover, static): + from agent import command_token_source + from hermes_cli import config as config_module, models + from hermes_cli import model_setup_flows_custom as setup + + monkeypatch.setenv("TEST_GATEWAY_KEY", "stale") + mints, probes, choices = [], [], [] + entry = {"name": "Gateway", "base_url": "https://gateway.invalid/v1", "key_cmd": "tenant-a", + "model": "fallback", "default_model": "fallback", "discover_models": discover, **static} + config = {"model": {"default": "fallback", "provider": "custom"}, + route: {"gateway": copy.deepcopy(entry)} if route == "providers" else [copy.deepcopy(entry)]} + config_module.save_config(config) + from hermes_cli.main_provider_setup import _named_custom_provider_map + info = next(iter(_named_custom_provider_map(config_module.load_config()).values())) + + def mint(command, label): + mints.append(command) + return "transient-bearer", 3600 + + def fetch(key, url, **kwargs): + probes.append(key) + return ["fallback", "discovered"] if key == "transient-bearer" else None + + def pick(name, available, saved): + choices.extend(available) + return available[-1] + + monkeypatch.setattr(command_token_source, "_mint", mint) + monkeypatch.setattr(models, "fetch_api_models", fetch) + monkeypatch.setattr("hermes_cli.models_local.should_use_ollama_native_catalog", lambda *a, **k: False) + monkeypatch.setattr(setup, "_pick_named_custom_model", pick) + monkeypatch.setattr(setup, "_ask", lambda *a, **k: "fallback") + setup._model_flow_named_custom(config, info) + if discover is True: + assert mints == ["tenant-a"] and probes == ["transient-bearer"] + assert choices == ["fallback", "discovered"] + else: + assert not mints and not probes + assert choices == ["fallback"] + saved = config_module.load_config() + assert "transient-bearer" not in repr(saved) + persisted = saved[route]["gateway"] if route == "providers" else saved[route][0] + assert persisted["key_cmd"] == entry["key_cmd"] + assert persisted.get("api_key", "") == static.get("api_key", "") diff --git a/website/docs/integrations/providers.md b/website/docs/integrations/providers.md index dfeeda226f..58b4bc39b1 100644 --- a/website/docs/integrations/providers.md +++ b/website/docs/integrations/providers.md @@ -1348,6 +1348,15 @@ The command must print **only** the token on stdout: either bare, or as JSON wit Precedence: an explicit `--api-key` flag still wins; otherwise `key_cmd` beats a static `api_key`/`key_env` on the same entry. The minted credential applies to the main agent turn and to auxiliary tasks (title generation, compression, vision, embedding) alike. +Model discovery also honors `key_cmd` for both `providers:` and legacy +`custom_providers:` entries, including `hermes model` setup. Helpers run only when +an authenticated live catalog probe is needed: disabled discovery and warm catalog +cache reads do not mint tokens. Catalogs are scoped to the command identity, so +rotating a bearer does not invalidate the catalog. Probe helpers use their own +short-lived token source, not the inference client's token cache; minted bearers +are never saved to `config.yaml`. If a helper fails, discovery falls back to the +configured model without exposing the helper's output. + Not to be confused with `secrets.command`, which runs a helper **once at startup** to populate env vars process-wide. Use that for a vault/keychain helper handing back many secrets; use `key_cmd` when one provider's credential must be re-minted *during* a session. :::note Legacy format