From 2b9b4306eb32fe76f3d71fdedeccfd486d7a9205 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 30 Apr 2026 23:58:24 -0700 Subject: [PATCH] fix(adapter): per-entry isolation in _load_providers + tighten _normalize_provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two correctness issues spotted in self-review of c6f4912: 1. String-as-prefix typo split into character tuple. ``model_prefixes: mimo-`` (operator forgot brackets) used to iterate over characters → ``('m','i','m','o','-')``, silently routing every model id starting with 'm', 'i', or '-' through the entry. Now: non-list values coerce to empty tuple (entry survives but matches nothing — operator notices in boot banner, not via misrouted requests). 2. Single bad provider entry nuked the whole registry. _load_providers built the registry via a generator inside tuple(...). One AttributeError mid-comprehension (e.g. ``[mimo-, 123]`` — int's missing .lower()) propagated out, broad except caught it, registry silently fell back to _BUILTIN_PROVIDERS (oauth + anthropic-api only). Every third-party model would then route to anthropic-oauth — exactly the silent-fallback failure mode this PR was meant to eliminate. Now: per-entry try/except drops the bad entry with a warning, rest survives. Also: entries without a string ``name`` field are now dropped with a warning instead of silently using the placeholder ```` — operator typos surface in boot logs. Tests: 28 passing (3 new regression tests covering both issues plus the no-name path). Co-Authored-By: Claude Opus 4.7 (1M context) --- adapter.py | 90 +++++++++++++++++--- tests/test_adapter_prevalidate.py | 131 ++++++++++++++++++++++++++++++ 2 files changed, 209 insertions(+), 12 deletions(-) diff --git a/adapter.py b/adapter.py index dc38ab9..2c9fcec 100644 --- a/adapter.py +++ b/adapter.py @@ -49,20 +49,66 @@ _BUILTIN_PROVIDERS = ( ) -def _normalize_provider(entry: dict) -> dict: +def _coerce_string_list(value, lowercase: bool = False) -> tuple: + """Defensively coerce a YAML field expected to be a list-of-strings. + + Operator typos in config.yaml come in two shapes that both used to + silently produce wrong routing: + 1. forgot brackets: ``model_prefixes: mimo-`` (string, not list) + 2. mixed types: ``model_prefixes: [mimo-, 123]`` (int slips in) + + Case 1 used to iterate over characters → ``('m','i','m','o','-')``, + making the entry match every model whose id starts with any of those + letters. Case 2 raised AttributeError mid-comprehension, killing the + whole registry build and silently falling back to builtins-only — + exactly the silent-fallback failure mode this PR was meant to fix. + + Returns an empty tuple for any non-list (treated as "no entries"); + drops non-string items in the list with a warning. + + ``lowercase`` controls case-folding: True for case-insensitive + comparisons (model_prefixes, model_aliases — operators write + ``MiniMax-M2`` in YAML, model id arrives lowercased downstream), + False to preserve case (auth_env — env var names are + case-sensitive: ``CLAUDE_CODE_OAUTH_TOKEN`` ≠ + ``claude_code_oauth_token``). + """ + if not isinstance(value, list): + return () + out = [] + for item in value: + if not isinstance(item, str): + logger.warning( + "providers: skipping non-string list item %r (type %s)", + item, type(item).__name__, + ) + continue + out.append(item.lower() if lowercase else item) + return tuple(out) + + +def _normalize_provider(entry: dict): """Coerce a YAML-loaded provider dict into the shape adapter logic expects. YAML gives us lists (not tuples) and may omit optional keys. Normalize to the union of all fields so downstream lookups work without scattered - .get(...) calls. + .get(...) calls. Returns ``None`` for entries that can't be salvaged + (e.g. missing name) so the caller can drop them without poisoning the + rest of the registry. """ + if not isinstance(entry, dict): + return None + name = entry.get("name") + if not name or not isinstance(name, str): + logger.warning("providers: skipping entry without a string name: %r", entry) + return None return { - "name": entry.get("name") or "", + "name": name, "auth_mode": entry.get("auth_mode") or _AUTH_MODE_OAUTH, - "model_prefixes": tuple(p.lower() for p in entry.get("model_prefixes") or ()), - "model_aliases": tuple(a.lower() for a in entry.get("model_aliases") or ()), + "model_prefixes": _coerce_string_list(entry.get("model_prefixes"), lowercase=True), + "model_aliases": _coerce_string_list(entry.get("model_aliases"), lowercase=True), "base_url": entry.get("base_url") or None, - "auth_env": tuple(entry.get("auth_env") or ()), + "auth_env": _coerce_string_list(entry.get("auth_env"), lowercase=False), } @@ -76,22 +122,42 @@ def _load_providers(config_path: str) -> tuple: if the file is missing, malformed, or has no providers section, so a bare-bones workspace still boots with the historical defaults. - Mode mismatches (e.g. a provider entry without a name) are logged - but don't fail the load — better-something-than-nothing for boot. + Per-entry isolation: a single bad provider entry is dropped with a + warning; the rest of the registry survives. Used to be a generator + inside tuple(...) that propagated any AttributeError out and reverted + the whole registry to builtins — exactly the silent-fallback failure + mode this file's existence was meant to fix. """ yaml_path = os.path.join(config_path, "config.yaml") try: import yaml # transitive dep via molecule-ai-workspace-runtime with open(yaml_path, "r") as f: data = yaml.safe_load(f) or {} - raw = data.get("providers") - if isinstance(raw, list) and raw: - return tuple(_normalize_provider(p) for p in raw if isinstance(p, dict)) except FileNotFoundError: logger.info("providers: %s not found, using builtin defaults", yaml_path) + return _BUILTIN_PROVIDERS except Exception as exc: # noqa: BLE001 — defensive: never block boot on YAML logger.warning("providers: failed to load from %s (%s); using builtins", yaml_path, exc) - return _BUILTIN_PROVIDERS + return _BUILTIN_PROVIDERS + + raw = data.get("providers") if isinstance(data, dict) else None + if not isinstance(raw, list) or not raw: + return _BUILTIN_PROVIDERS + + parsed = [] + for entry in raw: + try: + normalized = _normalize_provider(entry) + except Exception as exc: # noqa: BLE001 — per-entry isolation + logger.warning("providers: dropping unparseable entry %r (%s)", entry, exc) + continue + if normalized is not None: + parsed.append(normalized) + + if not parsed: + logger.warning("providers: no valid entries in %s; using builtins", yaml_path) + return _BUILTIN_PROVIDERS + return tuple(parsed) def _resolve_provider(model: str, providers: tuple) -> dict: diff --git a/tests/test_adapter_prevalidate.py b/tests/test_adapter_prevalidate.py index 3a053f4..566d5b5 100644 --- a/tests/test_adapter_prevalidate.py +++ b/tests/test_adapter_prevalidate.py @@ -622,6 +622,137 @@ def test_resolve_provider_minimax_prefix_matches_minimax_provider(): assert result2["name"] == "minimax" +def test_load_providers_drops_bad_entry_keeps_rest(tmp_path, caplog): + """Per-entry isolation: one malformed entry shouldn't nuke the registry. + + Pre-fix: ``_load_providers`` built the registry via a generator inside + ``tuple(...)``. A single AttributeError mid-comprehension propagated + out and the broad except caught it, silently reverting to + ``_BUILTIN_PROVIDERS`` (oauth + anthropic-api only). Every third-party + model would then route to anthropic-oauth — exactly the silent-fallback + failure mode this PR was meant to eliminate. + + Post-fix: per-entry try/except drops the bad entry with a warning, + rest of the registry survives. + """ + _install_stubs() + parent_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + if parent_dir not in sys.path: + sys.path.insert(0, parent_dir) + sys.modules.pop("adapter", None) + import adapter as adapter_module + + yaml_with_typo = textwrap.dedent(""" + providers: + - name: good-zai + auth_mode: third_party_anthropic_compat + model_prefixes: [glm-] + base_url: https://api.z.ai/api/anthropic + auth_env: [ANTHROPIC_AUTH_TOKEN] + + # Operator typo: forgot list brackets, ints slipped in. + # Pre-fix: AttributeError on the int's .lower() killed the + # whole tuple build → registry fell back to builtins. + - name: bad-one + auth_mode: third_party_anthropic_compat + model_prefixes: [bad-, 123] + base_url: https://example.com + auth_env: [SOME_TOKEN] + + - name: good-anthropic + auth_mode: anthropic_api + model_prefixes: [claude-] + auth_env: [ANTHROPIC_API_KEY] + """) + (tmp_path / "config.yaml").write_text(yaml_with_typo) + + import logging + with caplog.at_level(logging.WARNING): + result = adapter_module._load_providers(str(tmp_path)) + + # All three entries survive — the integer is dropped, the rest of + # the bad-one entry's prefix list is kept (just `bad-`). + names = [p["name"] for p in result] + assert names == ["good-zai", "bad-one", "good-anthropic"], ( + f"Expected all three entries to survive (with the int dropped from " + f"bad-one's prefixes), got {names}" + ) + + # Confirm the int got skipped, not silently coerced or crash-bubbled. + bad = next(p for p in result if p["name"] == "bad-one") + assert bad["model_prefixes"] == ("bad-",), ( + f"Non-string list element should be dropped; got {bad['model_prefixes']}" + ) + + # Operator should see a warning so they can fix the YAML. + assert any("non-string" in r.getMessage() for r in caplog.records), ( + "Expected a warning about the non-string list item" + ) + + +def test_load_providers_string_as_prefix_does_not_split_into_chars(tmp_path, caplog): + """A YAML field declared as list-of-strings but written as a bare + string (operator forgot brackets) used to silently iterate over + characters → ``('m','i','m','o','-')``. Post-fix: non-list value + coerces to empty tuple with no exception. The entry survives but + matches nothing — operator notices in the boot banner instead of + via mysteriously-misrouted requests. + """ + _install_stubs() + parent_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + if parent_dir not in sys.path: + sys.path.insert(0, parent_dir) + sys.modules.pop("adapter", None) + import adapter as adapter_module + + yaml_str_prefix = textwrap.dedent(""" + providers: + - name: typo-prefix + auth_mode: third_party_anthropic_compat + model_prefixes: mimo- + base_url: https://api.xiaomimimo.com/anthropic + auth_env: [ANTHROPIC_AUTH_TOKEN] + """) + (tmp_path / "config.yaml").write_text(yaml_str_prefix) + + result = adapter_module._load_providers(str(tmp_path)) + typo = next(p for p in result if p["name"] == "typo-prefix") + assert typo["model_prefixes"] == (), ( + f"String value (forgot brackets) must coerce to empty tuple, not " + f"split into characters; got {typo['model_prefixes']}" + ) + + +def test_load_providers_drops_entry_without_name(tmp_path, caplog): + """An entry without ``name`` is operator error — no silent fallback + to ````. Drop the entry with a warning so the boot log + surfaces the typo. + """ + _install_stubs() + parent_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) + if parent_dir not in sys.path: + sys.path.insert(0, parent_dir) + sys.modules.pop("adapter", None) + import adapter as adapter_module + + yaml_no_name = textwrap.dedent(""" + providers: + - name: good + auth_mode: oauth + auth_env: [CLAUDE_CODE_OAUTH_TOKEN] + - auth_mode: third_party_anthropic_compat + model_prefixes: [foo-] + """) + (tmp_path / "config.yaml").write_text(yaml_no_name) + + import logging + with caplog.at_level(logging.WARNING): + result = adapter_module._load_providers(str(tmp_path)) + + assert [p["name"] for p in result] == ["good"] + assert any("without a string name" in r.getMessage() for r in caplog.records) + + def test_resolve_provider_falls_back_to_first_when_unknown(): """Unknown model id → fallback to first provider (OAuth by convention).""" _install_stubs()