Merge origin/master into anchor fallback audit

Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Frank Song 2026-06-28 17:02:52 +08:00
commit 767285f21f
7 changed files with 405 additions and 34 deletions

View file

@ -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 (

View file

@ -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"):

View file

@ -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;

View file

@ -1899,38 +1899,98 @@ def _is_valid_image(path: Path, mime: str) -> bool:
return False
def _resolve_image_input_mode(cfg: dict) -> str:
"""Return ``"native"`` or ``"text"`` based on config, mirroring
``agent/image_routing.py:decide_image_input_mode``.
def _explicit_text_signal(cfg: dict) -> bool:
"""True when the user has explicitly opted into the text (vision_analyze)
image pipeline.
The agent has this logic, but the WebUI's ``_build_native_multimodal_message``
was unconditionally embedding images as native ``image_url`` parts, completely
bypassing ``image_input_mode``. This caused silent failures when the main model
does not support images and the fallback model is also text-only (#21160-related).
Two explicit signals, either of which means "the user chose text on
purpose" and we must honour it rather than forwarding images natively:
* ``agent.image_input_mode: text`` — a direct mode override.
* a configured ``auxiliary.vision`` backend (provider not ``auto``/empty,
or an explicit model / base_url) — the user is paying for a dedicated
vision model and wants the text pipeline regardless of the main model.
This mirrors the explicit-signal portion of
``agent/image_routing.py:decide_image_input_mode`` and is used both to
interpret *why* the canonical router returned ``"text"`` (so the
unknown-model carve-out only fires when there's no explicit user choice)
and as the fallback decision when the agent package is unavailable.
"""
if not isinstance(cfg, dict):
return False
agent_cfg = cfg.get("agent") or {}
mode = str(agent_cfg.get("image_input_mode", "auto") or "auto").strip().lower()
if mode not in ("auto", "native", "text"):
mode = "auto"
if mode == "native":
return "native"
if mode == "text":
return "text"
# auto: if auxiliary.vision is explicitly configured → text mode
# (user opted into a dedicated vision backend)
if isinstance(agent_cfg, dict):
mode = str(agent_cfg.get("image_input_mode", "auto") or "auto").strip().lower()
if mode == "text":
return True
aux = cfg.get("auxiliary") or {}
vision = aux.get("vision") or {}
vision = (aux.get("vision") or {}) if isinstance(aux, dict) else {}
if not isinstance(vision, dict):
return False
provider = str(vision.get("provider") or "").strip().lower()
model_name = str(vision.get("model") or "").strip()
base_url = str(vision.get("base_url") or "").strip()
if provider not in ("", "auto") or model_name or base_url:
return "text"
return provider not in ("", "auto") or bool(model_name) or bool(base_url)
# No explicit vision config, no model-capability lookup available in WebUI.
# Default to native — the agent's ``_strip_images_from_messages`` guard will
# strip images on rejection and retry as text.
def _resolve_image_input_mode(cfg: dict) -> str:
"""Return ``"native"`` or ``"text"`` for current-turn image uploads.
Delegates the routing decision to ``agent/image_routing.py:
decide_image_input_mode`` — the single source of truth — instead of the
local re-implementation that previously lived here. That copy had DIVERGED
from the canonical function: it returned ``"text"`` (dropping the image) in
cases the canonical router would have forwarded natively, because it never
consulted the active model's vision capability and instead hard-coded a
handful of config heuristics.
The WebUI keeps one deliberate carve-out on top of the canonical decision:
for UNKNOWN / custom models (no models.dev capability data) the canonical
router conservatively returns ``"text"``, but the WebUI historically
forwards images NATIVELY and relies on the agent's strip-and-retry guard
(``run_agent._try_shrink_image_parts_in_messages`` /
``_strip_images_from_messages``) to downgrade on a provider rejection. We
preserve that behaviour here: a canonical ``"text"`` verdict is only
honoured when there is a real signal — an explicit user choice
(``image_input_mode: text`` or a configured ``auxiliary.vision`` backend)
or a model KNOWN to lack vision. Otherwise we forward native.
When the agent package is unavailable (e.g. the WebUI standalone test
environment, where ``import agent`` fails), we fall back to the historical
WebUI behaviour: honour an explicit text signal, otherwise native.
"""
if not isinstance(cfg, dict):
cfg = {}
try:
from agent.image_routing import decide_image_input_mode, _lookup_supports_vision
from agent.auxiliary_client import _read_main_provider, _read_main_model
provider = (_read_main_provider() or "").strip()
model = (_read_main_model() or "").strip()
mode = decide_image_input_mode(provider, model, cfg)
if mode == "native":
return "native"
# Canonical returned "text". Honour it only when it reflects a genuine
# signal; otherwise apply the WebUI unknown-model native carve-out.
if _explicit_text_signal(cfg):
return "text"
if _lookup_supports_vision(provider, model, cfg) is False:
# Model is KNOWN to be text-only — respect the canonical verdict.
return "text"
# Unknown / custom model (capability is None): WebUI forwards native
# and lets the agent's strip-and-retry guard downgrade on rejection.
return "native"
except Exception:
# Agent package unavailable or import error — preserve historical WebUI
# behaviour: explicit text signal wins, otherwise native.
pass
if _explicit_text_signal(cfg):
return "text"
return "native"

View file

@ -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

View file

@ -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

View file

@ -829,6 +829,122 @@ def test_gateway_chat_worker_forwards_image_attachments_as_multimodal_parts(tmp_
assert content[1]["image_url"]["url"].startswith("data:image/png;base64,")
# ── #4113 (salvage): _resolve_image_input_mode delegates to the agent's
# canonical decide_image_input_mode, but preserves the WebUI carve-out that
# UNKNOWN/custom models still forward images NATIVELY. ──────────────────────
#
# The agent package is not importable in the WebUI standalone test environment
# (``import agent`` raises ModuleNotFoundError), so we inject fake
# ``agent.image_routing`` / ``agent.auxiliary_client`` modules to exercise the
# real delegation branch. The fakes let us control exactly what the canonical
# router returns and what capability lookup reports, so each behaviour is
# pinned independently of models.dev data.
def _install_fake_agent_routing(monkeypatch, *, decision, supports,
provider="customcorp", model="mystery-9000"):
"""Inject fake agent.image_routing + agent.auxiliary_client into sys.modules.
``decision`` is what the canonical ``decide_image_input_mode`` returns;
``supports`` is what ``_lookup_supports_vision`` returns (True / False /
None — None means "unknown / no capability data").
"""
import sys
import types
img = types.ModuleType("agent.image_routing")
img.decide_image_input_mode = lambda p, m, cfg: decision
img._lookup_supports_vision = lambda p, m, cfg=None: supports
aux = types.ModuleType("agent.auxiliary_client")
aux._read_main_provider = lambda: provider
aux._read_main_model = lambda: model
pkg = types.ModuleType("agent")
monkeypatch.setitem(sys.modules, "agent", pkg)
monkeypatch.setitem(sys.modules, "agent.image_routing", img)
monkeypatch.setitem(sys.modules, "agent.auxiliary_client", aux)
def test_resolve_image_input_mode_unknown_model_forwards_native(monkeypatch):
"""WebUI carve-out: an UNKNOWN/custom model (no capability data) forwards
images natively even though the canonical router conservatively returns
``"text"`` for it.
This is the behaviour the gateway image-forwarding test relies on, and the
agent's strip-and-retry guard downgrades to text on a provider rejection.
"""
_install_fake_agent_routing(monkeypatch, decision="text", supports=None)
cfg = {"agent": {"image_input_mode": "auto"},
"auxiliary": {"vision": {"provider": "auto"}}}
assert streaming._resolve_image_input_mode(cfg) == "native"
def test_resolve_image_input_mode_known_text_only_routes_text(monkeypatch):
"""NON-VACUOUS regression for #4113's real divergence.
The OLD local re-implementation never consulted model capability, so a
model KNOWN to lack vision (``supports_vision == False``) still got images
embedded as native ``image_url`` parts — silently sending pixels to a model
that cannot see them (#21160). Delegating to the canonical router fixes
this: a known text-only model now routes through the text (vision_analyze)
pipeline.
Against master this assertion FAILS — the old code returns ``"native"`` for
this exact config (auto mode, no explicit vision backend) because it ignored
capability entirely.
"""
_install_fake_agent_routing(monkeypatch, decision="text", supports=False)
cfg = {"agent": {"image_input_mode": "auto"},
"auxiliary": {"vision": {"provider": "auto"}}}
assert streaming._resolve_image_input_mode(cfg) == "text"
def test_resolve_image_input_mode_known_vision_model_forwards_native(monkeypatch):
"""A model KNOWN to support vision forwards natively (canonical native)."""
_install_fake_agent_routing(monkeypatch, decision="native", supports=True)
cfg = {"agent": {"image_input_mode": "auto"}}
assert streaming._resolve_image_input_mode(cfg) == "native"
def test_resolve_image_input_mode_explicit_text_signal_honored(monkeypatch):
"""An explicit user choice for the text pipeline is honoured even for an
unknown model — the carve-out only fires when there is NO explicit signal.
Both an explicit ``agent.image_input_mode: text`` and a configured
``auxiliary.vision`` backend count as explicit signals.
"""
_install_fake_agent_routing(monkeypatch, decision="text", supports=None)
assert streaming._resolve_image_input_mode(
{"agent": {"image_input_mode": "text"}}) == "text"
assert streaming._resolve_image_input_mode(
{"agent": {"image_input_mode": "auto"},
"auxiliary": {"vision": {"provider": "openai", "model": "gpt-4o"}}}) == "text"
def test_resolve_image_input_mode_fallback_when_agent_unavailable(monkeypatch):
"""When the agent package cannot be imported (standalone WebUI env), fall
back to historical WebUI behaviour: explicit text signal wins, else native.
"""
import sys
# Ensure the delegation import fails: stub agent.image_routing as a module
# that raises on attribute access of the routing fn would still import, so
# instead force an ImportError by mapping the submodule to None.
monkeypatch.setitem(sys.modules, "agent", None)
monkeypatch.setitem(sys.modules, "agent.image_routing", None)
# No explicit signal -> native (this is what keeps the gateway image test
# green, since agent is not importable there either).
assert streaming._resolve_image_input_mode(
{"agent": {"image_input_mode": "auto"},
"auxiliary": {"vision": {"provider": "auto"}}}) == "native"
# Explicit text mode -> text.
assert streaming._resolve_image_input_mode(
{"agent": {"image_input_mode": "text"}}) == "text"
# Explicit auxiliary vision backend -> text.
assert streaming._resolve_image_input_mode(
{"auxiliary": {"vision": {"provider": "anthropic"}}}) == "text"
def test_gateway_use_runs_api_is_default_off():
for env in ({}, {"HERMES_WEBUI_GATEWAY_USE_RUNS_API": ""}):
assert _gateway_use_runs_api_enabled({}, env) is False