fix(adapter): per-entry isolation in _load_providers + tighten _normalize_provider
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 ``<unnamed>`` —
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) <noreply@anthropic.com>
This commit is contained in:
parent
7c3aeb5a14
commit
2b9b4306eb
90
adapter.py
90
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.
|
"""Coerce a YAML-loaded provider dict into the shape adapter logic expects.
|
||||||
|
|
||||||
YAML gives us lists (not tuples) and may omit optional keys. Normalize
|
YAML gives us lists (not tuples) and may omit optional keys. Normalize
|
||||||
to the union of all fields so downstream lookups work without scattered
|
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 {
|
return {
|
||||||
"name": entry.get("name") or "<unnamed>",
|
"name": name,
|
||||||
"auth_mode": entry.get("auth_mode") or _AUTH_MODE_OAUTH,
|
"auth_mode": entry.get("auth_mode") or _AUTH_MODE_OAUTH,
|
||||||
"model_prefixes": tuple(p.lower() for p in entry.get("model_prefixes") or ()),
|
"model_prefixes": _coerce_string_list(entry.get("model_prefixes"), lowercase=True),
|
||||||
"model_aliases": tuple(a.lower() for a in entry.get("model_aliases") or ()),
|
"model_aliases": _coerce_string_list(entry.get("model_aliases"), lowercase=True),
|
||||||
"base_url": entry.get("base_url") or None,
|
"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
|
if the file is missing, malformed, or has no providers section, so a
|
||||||
bare-bones workspace still boots with the historical defaults.
|
bare-bones workspace still boots with the historical defaults.
|
||||||
|
|
||||||
Mode mismatches (e.g. a provider entry without a name) are logged
|
Per-entry isolation: a single bad provider entry is dropped with a
|
||||||
but don't fail the load — better-something-than-nothing for boot.
|
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")
|
yaml_path = os.path.join(config_path, "config.yaml")
|
||||||
try:
|
try:
|
||||||
import yaml # transitive dep via molecule-ai-workspace-runtime
|
import yaml # transitive dep via molecule-ai-workspace-runtime
|
||||||
with open(yaml_path, "r") as f:
|
with open(yaml_path, "r") as f:
|
||||||
data = yaml.safe_load(f) or {}
|
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:
|
except FileNotFoundError:
|
||||||
logger.info("providers: %s not found, using builtin defaults", yaml_path)
|
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
|
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)
|
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:
|
def _resolve_provider(model: str, providers: tuple) -> dict:
|
||||||
|
|||||||
@ -622,6 +622,137 @@ def test_resolve_provider_minimax_prefix_matches_minimax_provider():
|
|||||||
assert result2["name"] == "minimax"
|
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 ``<unnamed>``. 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():
|
def test_resolve_provider_falls_back_to_first_when_unknown():
|
||||||
"""Unknown model id → fallback to first provider (OAuth by convention)."""
|
"""Unknown model id → fallback to first provider (OAuth by convention)."""
|
||||||
_install_stubs()
|
_install_stubs()
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user