fix: harden None providers config across onboarding/providers/routes (salvage of #3967)

A config.yaml with an explicit empty/null `providers:` key parses to None in
PyYAML. `cfg.get("providers", {})` only returns the {} default when the key is
*absent*, so an explicit null yields None instead of an empty mapping. master
already guards each read with `isinstance(providers_cfg, dict)`, but the value
fed to that guard is still a footgun: the natural chained form
`cfg.get("providers", {}).get(...)` (already fixed once in api/config.py)
crashes with AttributeError on None.

Harden every providers-key read in the three remaining files to
`cfg.get("providers") or {}` so an explicit null degrades to an empty dict at
the source, matching the existing api/config.py convention. Sites hardened:

  api/onboarding.py:578  (_provider_api_key_present)
  api/providers.py:1051  (_provider_has_shadowed_codex_oauth_value)
  api/providers.py:1219  (_provider_has_key)
  api/providers.py:1265  (_get_provider_api_key)
  api/providers.py:2348  (get_providers catalog)
  api/providers.py:2781  (_clean_provider_key_from_config)
  api/routes.py:5045     (_context_length_config_api_key_for_provider)
  api/routes.py:5110     (_context_length_lookup_inputs_for_model)
  api/routes.py:15916    (_handle_live_models)

Adds tests/test_none_providers_config_guard.py: per-file source-form pins (fail
on master / on any single-file revert) plus behavioural checks driving the real
functions with `providers: None` to prove no crash and parity with the
empty-mapping config. Updates the existing test_issue3717 routes-string pin to
match the hardened form.

This is a focused salvage of grab-bag PR #3967 -- only the None-providers
hardening is taken; the PR's unrelated changes (account-usage worker-pool
refactor, function removals) are intentionally dropped.

Co-authored-by: lidi1011 <lidi1011@users.noreply.github.com>

api/onboarding.py

@@ -572,7 +572,10 @@ def _provider_api_key_present(
     if isinstance(model_cfg, dict) and str(model_cfg.get("api_key") or "").strip():
         return True
 
-    providers_cfg = cfg.get("providers", {})
+    # ``cfg.get("providers", {})`` only returns the default when the key is
+    # absent; an explicit ``providers:`` (null) in config.yaml yields ``None``.
+    # ``... or {}`` degrades that null to an empty mapping (salvage of #3967).
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         provider_cfg = providers_cfg.get(provider, {})
         if (

api/providers.py

@@ -1048,7 +1048,7 @@ def _provider_has_shadowed_codex_oauth_value(provider_id: str) -> bool:
         active_provider = str(model_cfg.get("provider") or "").strip().lower()
         if active_provider == provider_id:
             values.append(model_cfg.get("api_key"))
-    providers_cfg = cfg.get("providers", {})
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         provider_cfg = providers_cfg.get(provider_id, {})
         if isinstance(provider_cfg, dict):
@@ -1216,7 +1216,7 @@ def _provider_has_key(provider_id: str) -> bool:
             if _provider_value_counts_as_api_key(provider_id, model_cfg.get("api_key")):
                 return True
     # Check providers.<id>.api_key
-    providers_cfg = cfg.get("providers", {})
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         provider_cfg = providers_cfg.get(provider_id, {})
         if isinstance(provider_cfg, dict) and str(provider_cfg.get("api_key") or "").strip():
@@ -1262,7 +1262,7 @@ def _get_provider_api_key(provider_id: str) -> str | None:
         if model_key and active_provider == provider_id and _provider_value_counts_as_api_key(provider_id, model_key):
             return model_key
 
-    providers_cfg = cfg.get("providers", {})
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         provider_cfg = providers_cfg.get(provider_id, {})
         if isinstance(provider_cfg, dict):
@@ -2345,7 +2345,7 @@ def get_providers() -> dict[str, Any]:
 
     # Also detect providers from config.yaml providers section
     cfg = get_config()
-    providers_cfg = cfg.get("providers", {})
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         known_ids.update(providers_cfg.keys())
 
@@ -2778,7 +2778,7 @@ def _clean_provider_key_from_config(provider_id: str) -> None:
                 return
 
             # 1. Clean providers.<id>.api_key
-            providers_cfg = cfg.get("providers", {})
+            providers_cfg = cfg.get("providers") or {}
             if isinstance(providers_cfg, dict):
                 provider_cfg = providers_cfg.get(provider_id, {})
                 if isinstance(provider_cfg, dict) and provider_cfg.get("api_key"):

api/routes.py

@@ -5042,7 +5042,7 @@ def _context_length_config_api_key_for_provider(
                 return resolved
         return ""
 
-    providers_cfg = cfg.get("providers", {})
+    providers_cfg = cfg.get("providers") or {}
     if isinstance(providers_cfg, dict):
         for provider_key, provider_cfg in providers_cfg.items():
             if not isinstance(provider_cfg, dict):
@@ -5107,7 +5107,7 @@ def _context_length_lookup_inputs_for_model(
         custom_providers = None
 
     provider_context_length = None
-    providers_cfg = cfg.get("providers", {}) if isinstance(cfg, dict) else {}
+    providers_cfg = (cfg.get("providers") or {}) if isinstance(cfg, dict) else {}
     if isinstance(providers_cfg, dict):
         for provider_key, provider_cfg in providers_cfg.items():
             if not isinstance(provider_cfg, dict):
@@ -16243,7 +16243,7 @@ def _handle_live_models(handler, parsed):
             if _ep:
                 try:
                     import urllib.request
-                    _providers_cfg = cfg.get("providers", {})
+                    _providers_cfg = cfg.get("providers") or {}
                     _prov = _providers_cfg.get(provider, {}) if isinstance(_providers_cfg, dict) else {}
                     # Only use a provider-scoped key.  A top-level model.api_key
                     # is safe here only when it belongs to the requested provider;

tests/test_issue3717_context_length_provider_overrides.py

@@ -193,6 +193,9 @@ def test_streaming_fallbacks_use_shared_provider_context_helper():
 
 def test_route_helper_keeps_all_context_length_sources_aligned():
     assert "def _context_length_lookup_inputs_for_model" in ROUTES_PY
-    assert 'cfg.get("providers", {})' in ROUTES_PY
+    # Hardened against an explicit null ``providers:`` key (salvage of #3967):
+    # ``cfg.get("providers") or {}`` instead of ``cfg.get("providers", {})`` so
+    # a config.yaml with ``providers:`` (None) degrades to an empty mapping.
+    assert 'cfg.get("providers") or {}' in ROUTES_PY
     assert 'cfg.get("custom_providers")' in ROUTES_PY
     assert "_model_matches_configured_default" in ROUTES_PY

tests/test_none_providers_config_guard.py

@@ -0,0 +1,189 @@
+"""Regression guard: a config with an explicit null ``providers:`` key must not
+break the provider-config read paths in onboarding / providers / routes, and
+must behave exactly as if ``providers`` were an empty mapping.
+
+Background (salvage of grab-bag PR #3967, thanks @lidi1011)
+----------------------------------------------------------
+``cfg.get("providers", {})`` only returns the ``{}`` default when the key is
+*absent*. When ``config.yaml`` contains an explicit empty/null key::
+
+    providers:
+
+PyYAML parses that as ``None``, so ``cfg.get("providers", {})`` yields ``None``
+(not ``{}``). master already wraps each of these reads in an
+``isinstance(providers_cfg, dict)`` guard, so the None value silently disables
+the provider lookup -- but the value handed to that guard is still a footgun:
+any future maintainer who chains ``cfg.get("providers", {}).get(...)`` (the
+natural-looking pattern, already used in api/config.py before its own #3967-style
+fix) would crash with ``AttributeError: 'NoneType' object has no attribute
+'get'``.
+
+The hardening replaces every such read with ``cfg.get("providers") or {}`` so an
+explicit null degrades to an empty dict at the *source*, matching the existing
+api/config.py convention.
+
+This module pins the fix two ways:
+
+1. **Source-form pins** (``test_<file>_hardens_providers_key_read``): assert that
+   onboarding / providers / routes no longer contain the unguarded
+   ``cfg.get("providers", {})`` form and *do* contain the hardened
+   ``cfg.get("providers") or {}`` form. These FAIL on master (where the
+   unguarded form is present) and would FAIL again if anyone reverts the guard
+   in any one of the three files -- so the test is non-vacuous per file.
+
+2. **Behavioural checks**: drive the real functions with ``providers: None`` and
+   assert they neither raise nor diverge from the ``providers: {}`` / absent-key
+   behaviour. This guards the runtime contract end to end.
+"""
+
+import ast
+import pathlib
+
+import pytest
+
+import api.config as config
+import api.onboarding as onboarding
+import api.providers as providers
+import api.routes as routes
+
+_REPO = pathlib.Path(__file__).resolve().parents[1]
+
+_UNGUARDED = 'cfg.get("providers", {})'
+_HARDENED = 'cfg.get("providers") or {}'
+
+
+def _src(name: str) -> str:
+    return (_REPO / "api" / name).read_text(encoding="utf-8")
+
+
+# --- source-form pins (fail on master / on any per-file revert) -----------
+
+
+@pytest.mark.parametrize("filename", ["onboarding.py", "providers.py", "routes.py"])
+def test_file_hardens_providers_key_read_against_none(filename):
+    """Each target file must read the providers key as ``... or {}``.
+
+    Non-vacuous: on master the unguarded ``cfg.get("providers", {})`` form is
+    present in every one of these files, so this assertion fails there. Reverting
+    the guard in any single file re-fails it -- the guard is pinned per file.
+    """
+    source = _src(filename)
+    # The hardened form must appear at least once.
+    assert _HARDENED in source, (
+        f"api/{filename} should read the providers key as `{_HARDENED}` so an "
+        f"explicit null `providers:` degrades to an empty mapping (salvage #3967)."
+    )
+    # The unguarded form must not appear as live code. It may legitimately appear
+    # inside an explanatory comment, so strip comment-only occurrences before
+    # asserting absence.
+    code_lines = []
+    for line in source.splitlines():
+        stripped = line.lstrip()
+        if stripped.startswith("#"):
+            continue
+        # Drop trailing inline comments crudely (good enough: the token we look
+        # for contains no '#').
+        code_lines.append(line.split("#", 1)[0])
+    code = "\n".join(code_lines)
+    assert _UNGUARDED not in code, (
+        f"api/{filename} still reads the providers key as the unguarded "
+        f"`{_UNGUARDED}`; an explicit `providers:` (None) in config.yaml would "
+        f"yield None instead of an empty mapping (salvage #3967)."
+    )
+
+
+@pytest.mark.parametrize("filename", ["onboarding.py", "providers.py", "routes.py"])
+def test_target_files_parse(filename):
+    """Sanity: the hardened files are valid Python (guards real edits)."""
+    ast.parse(_src(filename), filename=filename)
+
+
+# --- behavioural: onboarding ----------------------------------------------
+
+
+def _onboarding_cfg(providers_value):
+    return {
+        "model": {"provider": "custom", "api_key": "", "base_url": "http://localhost:1"},
+        "providers": providers_value,
+        "custom_providers": [],
+    }
+
+
+def test_onboarding_provider_api_key_present_handles_none_providers():
+    """``_provider_api_key_present`` must treat ``providers: None`` like ``{}``."""
+    none_cfg = _onboarding_cfg(None)
+    empty_cfg = _onboarding_cfg({})
+
+    for provider in ("custom", "openai", "anthropic"):
+        got_none = onboarding._provider_api_key_present(provider, none_cfg, {})
+        got_empty = onboarding._provider_api_key_present(provider, empty_cfg, {})
+        assert got_none == got_empty
+        assert got_none is False  # no key configured anywhere
+
+
+def test_onboarding_none_providers_does_not_mask_configured_key():
+    """A provider key set under model.api_key is still found with providers: None."""
+    cfg = _onboarding_cfg(None)
+    cfg["model"] = {"provider": "custom", "api_key": "sk-xyz", "base_url": "http://x"}
+    assert onboarding._provider_api_key_present("custom", cfg, {}) is True
+
+
+# --- behavioural: providers (catalog + key lookup) ------------------------
+
+
+def _patch_get_config(monkeypatch, cfg):
+    """Force api.providers.get_config()/api.config.get_config() to return ``cfg``."""
+    monkeypatch.setattr(providers, "get_config", lambda: cfg)
+    # Some helpers import get_config from api.config at call time.
+    monkeypatch.setattr(config, "get_config", lambda: cfg, raising=False)
+
+
+def _providers_cfg(providers_value):
+    return {
+        "model": {"provider": "custom", "api_key": "", "base_url": "http://localhost:1"},
+        "providers": providers_value,
+        "custom_providers": [],
+    }
+
+
+def test_providers_has_key_handles_none_providers(monkeypatch):
+    """``_provider_has_key`` must not crash and must match the empty-dict result."""
+    monkeypatch.setattr(providers, "_provider_env_var_for", lambda pid: None)
+    monkeypatch.setattr(
+        providers, "_load_env_file", lambda path: {}, raising=False
+    )
+
+    for providers_value, label in ((None, "none"), ({}, "empty")):
+        _patch_get_config(monkeypatch, _providers_cfg(providers_value))
+        # Should return False (no key) without raising on the None config.
+        assert providers._provider_has_key("custom") is False, label
+        assert providers._get_provider_api_key("custom") is None, label
+
+
+# --- behavioural: routes (context-length helper + key resolution) ----------
+
+
+def test_routes_context_length_helper_handles_none_providers():
+    """``_context_length_lookup_inputs_for_model`` must accept providers: None."""
+    cfg_none = {
+        "model": {"provider": "custom", "base_url": "http://localhost:1"},
+        "providers": None,
+        "custom_providers": [],
+    }
+    cfg_empty = dict(cfg_none, providers={})
+
+    # Pass cfg explicitly so the helper does not hit real config/disk.
+    out_none = routes._context_length_lookup_inputs_for_model(
+        "@custom:my-model", "custom", cfg=cfg_none
+    )
+    out_empty = routes._context_length_lookup_inputs_for_model(
+        "@custom:my-model", "custom", cfg=cfg_empty
+    )
+    # No crash, and the None config produces the same lookup inputs as the
+    # empty-mapping config (no provider override available in either case).
+    # ``_ContextLengthLookupInputs`` is a __slots__ class without __eq__, so
+    # compare the materialised fields rather than object identity.
+    fields = ("config_context_length", "custom_providers", "base_url", "provider", "api_key")
+    snapshot_none = {f: getattr(out_none, f) for f in fields}
+    snapshot_empty = {f: getattr(out_empty, f) for f in fields}
+    assert snapshot_none == snapshot_empty