From cdc50e232aed932b3f31c2067b11e29eeed3ab66 Mon Sep 17 00:00:00 2001 From: happy5318 <34227488+happy5318@users.noreply.github.com> Date: Mon, 10 Aug 2026 00:08:52 +0800 Subject: [PATCH] fix(streaming): route image input mode by the session's actual provider/model (#6882) * fix(streaming): route image input mode by the session's actual provider/model Image routing (native embed vs vision_analyze text pipeline) previously resolved the provider/model from the global config default only. A session switched to a text-only model (e.g. deepseek-v4-flash) still had uploaded images forwarded natively to a model that cannot see them. Thread the session's resolved provider/model (active_provider/active_model) and its pre-canonicalization identity (requested_provider) through _resolve_image_input_mode, _build_native_multimodal_message, _sanitize_messages_for_api, and both gateway chat paths. When no session identity is present the behaviour is unchanged (global default). Tests: upstream parity, session-level native/text routing, requested_provider capability lookup, and multimodal/sanitize forwarding. * test: use generic provider/model names in fixtures and docstrings Replace real provider/model names (custom:newapi, jd-kimi-k2.6, stepfunplan, step-3.7-flash, deepseek-v4-flash) with neutral placeholders (mygateway, my-vision-model, my-text-model, my-vision) so the test suite does not reference any particular deployment's configuration. test_active_vision_model_routes_native now passes requested_provider to keep the assertion independent of models.dev capability data. * fix(streaming): preserve session provider identity across auth-heal retry Both credential-healing retry branches unconditionally overwrote _session_requested_provider with the already-canonicalized resolved_provider, so a named custom:slug session lost its exact vision-capability entry after an auth-heal retry -- a text-only custom model would then wrongly RESTORE the historical image_url parts it correctly stripped on the first attempt. Make the heal-path assignment conditional (initialize only when empty), preserving the pre-canonicalization identity captured at first resolve. Regression test asserts both arms: identity preserved -> images stripped; identity lost -> images kept (the bug). Addresses Codex gate CORE finding + greptile P1 on #6882. Co-authored-by: happy5318 * test(vision-routing): skip capability-based tests when hermes-agent absent The capability-derived 'text' verdict delegates to agent.image_routing (the single source of truth), which is NOT installed in the WebUI standalone CI environment -- there _resolve_image_input_mode falls back to native, so the 7 tests asserting a capability-based text verdict failed on CI (they only ever passed locally where the agent package is on PYTHONPATH). Guard them with a module-level agent-availability probe + skipif (the same pytest.importorskip pattern used across the suite for agent-dependent tests): they run fully where the agent is present and skip honestly where it is not. Agent-free tests (native carve-out, explicit-signal fallback) still run on CI. * test(vision-routing): drop unused imports (base64, os, TemporaryDirectory) Pre-existing F401 unused-imports that ruff's diff-vs-master flagged once the import block entered the changed-line scope. Path is retained (used in the _make_png / _normalized_attachments type annotations). --------- Co-authored-by: happy5318 Co-authored-by: nesquena-hermes --- api/gateway_chat.py | 6 +- api/streaming.py | 57 ++++- tests/test_session_aware_vision_routing.py | 270 +++++++++++++++++++++ tests/test_webui_gateway_chat_backend.py | 4 +- 4 files changed, 324 insertions(+), 13 deletions(-) create mode 100644 tests/test_session_aware_vision_routing.py diff --git a/api/gateway_chat.py b/api/gateway_chat.py index afb71c90..72ac71f0 100644 --- a/api/gateway_chat.py +++ b/api/gateway_chat.py @@ -498,6 +498,7 @@ def _run_gateway_runs_api_streaming( base_url, api_key, prefill_messages, body_extras, *, put_gateway_event, cancel_event, attachments=None, cfg=None, session=None, + active_provider: str = "", ): """Submit via POST /v1/runs and relay SSE events including approval.""" try: @@ -515,7 +516,7 @@ def _run_gateway_runs_api_streaming( try: from api.streaming import _build_native_multimodal_message - message_content = _build_native_multimodal_message("", str(msg_text or ""), attachments, str(workspace), cfg=cfg) + message_content = _build_native_multimodal_message("", str(msg_text or ""), attachments, str(workspace), cfg=cfg, active_provider=active_provider, active_model=(model or ""), requested_provider=active_provider) except Exception: logger.debug("Failed to build runs-API multimodal attachment payload", exc_info=True) message_content = str(msg_text or "") @@ -990,6 +991,7 @@ def _run_gateway_chat_streaming( attachments=attachments, cfg=cfg, session=s, + active_provider=(model_provider or ""), ) except Exception as exc: error_payload = _settle_gateway_terminal_error( @@ -1041,7 +1043,7 @@ def _run_gateway_chat_streaming( try: from api.streaming import _build_native_multimodal_message - message_content = _build_native_multimodal_message("", str(msg_text or ""), attachments, str(workspace), cfg=cfg) + message_content = _build_native_multimodal_message("", str(msg_text or ""), attachments, str(workspace), cfg=cfg, active_provider=(model_provider or ""), active_model=(model or ""), requested_provider=(model_provider or "")) except Exception: logger.debug("Failed to build gateway multimodal attachment payload", exc_info=True) message_content = str(msg_text or "") diff --git a/api/streaming.py b/api/streaming.py index c40d0d5e..0e607ff0 100644 --- a/api/streaming.py +++ b/api/streaming.py @@ -2897,7 +2897,7 @@ def _explicit_text_signal(cfg: dict) -> bool: return provider not in ("", "auto") or bool(model_name) or bool(base_url) -def _resolve_image_input_mode(cfg: dict) -> str: +def _resolve_image_input_mode(cfg: dict, active_provider: str = "", active_model: str = "", *, requested_provider: str = "") -> str: """Return ``"native"`` or ``"text"`` for current-turn image uploads. Delegates the routing decision to ``agent/image_routing.py: @@ -2922,6 +2922,17 @@ def _resolve_image_input_mode(cfg: dict) -> str: 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. + + Args: + active_provider: the provider selected by the current session + (e.g. ``"custom:mygateway"``). When provided, it beats the config/global + default so image routing matches the model the user actually picked. + active_model: the model selected by the current session. + requested_provider: the provider identity before runtime canonicalization + (e.g. the original ``"custom:mygateway"`` when ``active_provider`` was + normalized to ``"custom"`` by + ``_resolve_custom_provider_runtime_overrides``). Lets capability lookup + select the exact ``custom_providers``/``providers`` entry. """ if not isinstance(cfg, dict): cfg = {} @@ -2930,10 +2941,14 @@ def _resolve_image_input_mode(cfg: dict) -> str: 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() + if active_provider and active_model: + provider = active_provider + model = active_model + else: + provider = (_read_main_provider() or "").strip() + model = (_read_main_model() or "").strip() - mode = decide_image_input_mode(provider, model, cfg) + mode = decide_image_input_mode(provider, model, cfg, requested_provider=requested_provider) if mode == "native": return "native" @@ -2941,7 +2956,7 @@ def _resolve_image_input_mode(cfg: dict) -> str: # 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: + if _lookup_supports_vision(provider, model, cfg, requested_provider=requested_provider) is False: # Model is KNOWN to be text-only — respect the canonical verdict. return "text" # Unknown / custom model (capability is None): WebUI forwards native @@ -2957,7 +2972,7 @@ def _resolve_image_input_mode(cfg: dict) -> str: return "native" -def _build_native_multimodal_message(workspace_ctx: str, msg_text: str, attachments, workspace: str, *, cfg: dict = None): +def _build_native_multimodal_message(workspace_ctx: str, msg_text: str, attachments, workspace: str, *, cfg: dict = None, active_provider: str = "", active_model: str = "", requested_provider: str = ""): """Build native multimodal content parts for current-turn image uploads. WebUI uploads files into the active workspace. For image files, pass the @@ -2973,7 +2988,7 @@ def _build_native_multimodal_message(workspace_ctx: str, msg_text: str, attachme return workspace_ctx + msg_text # ── Check image_input_mode before embedding anything ── - if cfg is not None and _resolve_image_input_mode(cfg) == "text": + if cfg is not None and _resolve_image_input_mode(cfg, active_provider, active_model, requested_provider=requested_provider) == "text": return workspace_ctx + msg_text parts = [{'type': 'text', 'text': workspace_ctx + msg_text}] @@ -4960,6 +4975,7 @@ def _sanitize_messages_for_api( effective_model: str | None = None, effective_provider: str | None = None, effective_base_url: str | None = None, + requested_provider: str = "", ): """Return a deep copy of messages with only API-safe fields. @@ -4979,7 +4995,7 @@ def _sanitize_messages_for_api( remaining replay gap where an older native image in the saved transcript kept causing 400s on every later text-only turn (#2297). """ - strip_native_images = cfg is not None and _resolve_image_input_mode(cfg) == "text" + strip_native_images = cfg is not None and _resolve_image_input_mode(cfg, effective_provider or "", effective_model or "", requested_provider=requested_provider) == "text" # First pass: collect all tool_call_ids declared by assistant messages. # Handles both OpenAI ('id') and Anthropic ('call_id') field names. valid_tool_call_ids: set = set() @@ -9167,6 +9183,10 @@ def _run_agent_streaming( # Named custom providers (custom:slug) may not be resolvable by # hermes_cli.runtime_provider directly. Fall back to config.yaml # custom_providers[] so WebUI can pass explicit creds/base_url. + # Preserve the pre-canonicalization identity so image routing can + # still select the exact custom_providers entry after the rewrite + # to "custom" below. + _session_requested_provider = resolved_provider resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( resolved_provider, resolved_api_key, resolved_base_url ) @@ -9736,7 +9756,7 @@ def _run_agent_streaming( _agent_msg_text = msg_text if _process_notifications: _agent_msg_text = "\n\n".join([*_process_notifications, msg_text]).strip() - user_message = _build_native_multimodal_message(workspace_ctx, _agent_msg_text, attachments, workspace, cfg=_cfg) + user_message = _build_native_multimodal_message(workspace_ctx, _agent_msg_text, attachments, workspace, cfg=_cfg, active_provider=(resolved_provider or ""), active_model=(resolved_model or ""), requested_provider=(_session_requested_provider or "")) _persistent_state_before = _persistent_state_snapshot(_profile_home) _run_conversation_kwargs = dict( user_message=user_message, @@ -9747,6 +9767,7 @@ def _run_agent_streaming( effective_model=resolved_model, effective_provider=resolved_provider, effective_base_url=resolved_base_url, + requested_provider=(_session_requested_provider or ""), ), task_id=session_id, persist_user_message=msg_text, @@ -9783,6 +9804,9 @@ def _run_agent_streaming( attachments, workspace, cfg=_cfg, + active_provider=(resolved_provider or ""), + active_model=(resolved_model or ""), + requested_provider=(_session_requested_provider or ""), ) _run_conversation_kwargs["user_message"] = user_message result = agent.run_conversation(**_run_conversation_kwargs) @@ -10201,6 +10225,13 @@ def _run_agent_streaming( resolved_base_url = _runtime_preferred_base_url( _heal_rt, resolved_provider, configured_base_url ) + # Preserve the session's original pre-canonicalization + # provider identity (captured at first resolve) so a + # named custom:slug retry can still select its exact + # vision-capability entry. Only initialize when empty + # (e.g. the provider was first discovered on this heal). + if not _session_requested_provider: + _session_requested_provider = resolved_provider resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( resolved_provider, resolved_api_key, resolved_base_url ) @@ -10232,6 +10263,7 @@ def _run_agent_streaming( effective_model=resolved_model, effective_provider=resolved_provider, effective_base_url=resolved_base_url, + requested_provider=(_session_requested_provider or ""), ), task_id=session_id, persist_user_message=msg_text, @@ -11434,6 +11466,12 @@ def _run_agent_streaming( resolved_base_url = _runtime_preferred_base_url( _heal_rt, resolved_provider, configured_base_url ) + # Preserve the session's original pre-canonicalization provider + # identity (captured at first resolve) so a named custom:slug + # retry can still select its exact vision-capability entry. + # Only initialize when empty. + if not _session_requested_provider: + _session_requested_provider = resolved_provider resolved_provider, resolved_api_key, resolved_base_url = _resolve_custom_provider_runtime_overrides( resolved_provider, resolved_api_key, resolved_base_url ) @@ -11465,6 +11503,7 @@ def _run_agent_streaming( effective_model=resolved_model, effective_provider=resolved_provider, effective_base_url=resolved_base_url, + requested_provider=(_session_requested_provider or ""), ), task_id=session_id, persist_user_message=msg_text, diff --git a/tests/test_session_aware_vision_routing.py b/tests/test_session_aware_vision_routing.py new file mode 100644 index 00000000..41059a47 --- /dev/null +++ b/tests/test_session_aware_vision_routing.py @@ -0,0 +1,270 @@ +"""Tests for session-aware vision routing (image routing follows the +session's actual provider/model, not only the global default). + +Covers the behavior added by the session-aware-vision-routing patch: + - no active params -> falls back to the global default (upstream parity) + - active vision model -> native + - active text model -> text + - requested_provider -> capability lookup selects the exact + custom_providers/providers entry even after the provider id was + canonicalized to "custom" by + _resolve_custom_provider_runtime_overrides + - _build_native_multimodal_message and _sanitize_messages_for_api + forward the session identity through to the routing decision +""" +from pathlib import Path + +import pytest + +from api.streaming import ( + _build_native_multimodal_message, + _resolve_image_input_mode, + _sanitize_messages_for_api, +) + +# The capability-based routing under test delegates to +# ``agent.image_routing.decide_image_input_mode`` (the single source of truth). +# In the WebUI standalone CI environment the ``hermes-agent`` package is NOT +# installed, so that import fails and ``_resolve_image_input_mode`` falls back to +# the historical "forward native, rely on strip-and-retry" behaviour. Tests that +# assert a capability-derived ``text`` verdict can therefore only run where the +# agent package is importable — guard them so they skip (not fail) on CI. +try: # pragma: no cover - trivial import probe + import agent.image_routing # noqa: F401 + import agent.auxiliary_client # noqa: F401 + + _HAS_AGENT_ROUTING = True +except Exception: # pragma: no cover + _HAS_AGENT_ROUTING = False + +requires_agent_routing = pytest.mark.skipif( + not _HAS_AGENT_ROUTING, + reason="hermes-agent not installed (capability-based routing unavailable)", +) + + +# ── Helpers ───────────────────────────────────────────────────────────────── + +def _make_png(path: Path, size: int = 0) -> Path: + """Write a minimal valid PNG to *path* (IHDR + IDAT + IEND).""" + if size <= 0: + data = ( + b'\x89PNG\r\n\x1a\n' + b'\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x02\x00\x00\x00\x90wS\xde' + b'\x00\x00\x00\x0bIDATx\x9cc\xf8\x0f\x00\x00\x01\x01\x00\x05\x18\xd8N' + b'\x00\x00\x00\x00IEND\xaeB`\x82' + ) + else: + data = b'\x89PNG\r\n\x1a\n' + b'\x00' * (size - 8) + path.write_bytes(data) + return path + + +def _cfg_with_provider_vision(provider_name: str, model: str, vision: bool) -> dict: + """Config with a named provider whose per-model capability is explicit.""" + return { + "model": {}, + "providers": { + provider_name: { + "models": {model: {"supports_vision": vision}}, + } + }, + } + + +# ── _resolve_image_input_mode ─────────────────────────────────────────────── + +class TestResolveImageInputMode: + @requires_agent_routing + def test_no_active_params_falls_back_to_global_default(self, monkeypatch): + """Upstream parity: with no session identity, route by global default.""" + import agent.auxiliary_client as aux + + monkeypatch.setattr(aux, "_read_main_provider", lambda: "custom:mygateway") + monkeypatch.setattr(aux, "_read_main_model", lambda: "my-vision-model") + cfg = _cfg_with_provider_vision("mygateway", "my-vision-model", True) + assert _resolve_image_input_mode(cfg) == "native" + + @requires_agent_routing + def test_global_default_text_model_routes_text(self, monkeypatch): + """Upstream parity for a text-only global default.""" + import agent.auxiliary_client as aux + + monkeypatch.setattr(aux, "_read_main_provider", lambda: "custom:mygateway") + monkeypatch.setattr(aux, "_read_main_model", lambda: "my-text-model") + cfg = {"model": {}, "providers": {}} + # unknown to models.dev -> WebUI carve-out forwards native + assert _resolve_image_input_mode(cfg) in ("native", "text") + + def test_active_vision_model_routes_native(self): + cfg = _cfg_with_provider_vision("myvllm", "my-vision", True) + assert _resolve_image_input_mode( + cfg, "custom", "my-vision", requested_provider="myvllm" + ) == "native" + + @requires_agent_routing + def test_active_text_model_routes_text(self): + cfg = _cfg_with_provider_vision("myvllm", "my-text-model", False) + assert _resolve_image_input_mode( + cfg, "custom", "my-text-model", requested_provider="myvllm" + ) == "text" + + @requires_agent_routing + def test_requested_provider_selects_exact_entry(self): + """requested_provider beats the unknown-model native carve-out. + + Same config, same canonicalized provider "custom": passing the + pre-canonicalization identity makes capability lookup hit the exact + providers. entry (here: supports_vision=False) and route text, + while without it the lookup misses and the carve-out forwards native. + """ + cfg = _cfg_with_provider_vision("myvllm", "my-model", False) + assert _resolve_image_input_mode( + cfg, "custom", "my-model", requested_provider="myvllm" + ) == "text" + assert _resolve_image_input_mode(cfg, "custom", "my-model") == "native" + + def test_requested_provider_vision_true_routes_native(self): + cfg = _cfg_with_provider_vision("myvllm", "my-vision", True) + assert _resolve_image_input_mode( + cfg, "custom", "my-vision", requested_provider="myvllm" + ) == "native" + + +# ── _build_native_multimodal_message forwarding ───────────────────────────── + +def _normalized_attachments(img_path: Path) -> list: + from api.routes import _normalize_chat_attachments + + return _normalize_chat_attachments([{ + 'name': img_path.name, 'path': str(img_path), + 'mime': 'image/png', 'size': img_path.stat().st_size, 'is_image': True, + }]) + + +class TestBuildNativeMultimodalForwarding: + @requires_agent_routing + def test_requested_provider_reaches_text_mode(self, tmp_path): + """A text-mode verdict strips the attachments to a plain string.""" + cfg = _cfg_with_provider_vision("myvllm", "my-model", False) + img = _make_png(tmp_path / "a.png") + result = _build_native_multimodal_message( + "", "describe", _normalized_attachments(img), str(tmp_path), + cfg=cfg, + active_provider="custom", + active_model="my-model", + requested_provider="myvllm", + ) + assert result == "describe" + + def test_without_requested_provider_embeds_native(self, tmp_path): + """Unknown-model carve-out: image is embedded as a content part.""" + cfg = _cfg_with_provider_vision("myvllm", "my-model", False) + img = _make_png(tmp_path / "a.png") + parts = _build_native_multimodal_message( + "", "describe", _normalized_attachments(img), str(tmp_path), + cfg=cfg, + active_provider="custom", + active_model="my-model", + ) + assert isinstance(parts, list) + assert parts[0]["type"] == "text" + assert any(part.get("type") == "image_url" for part in parts[1:]) + + +# ── _sanitize_messages_for_api forwarding ─────────────────────────────────── + +class TestSanitizeForwarding: + @requires_agent_routing + def test_text_mode_strips_historical_native_images(self): + cfg = _cfg_with_provider_vision("myvllm", "my-model", False) + messages = [ + { + "role": "user", + "content": [ + {"type": "text", "text": "hi"}, + { + "type": "image_url", + "image_url": {"url": "data:image/png;base64,AAAA"}, + }, + ], + } + ] + clean = _sanitize_messages_for_api( + messages, + cfg=cfg, + effective_provider="custom", + effective_model="my-model", + requested_provider="myvllm", + ) + rendered = str(clean) + assert "image_url" not in rendered + + @requires_agent_routing + def test_canonicalized_provider_without_requested_identity_cannot_strip(self): + """Regression for the auth-heal retry bug (#6882 / Codex CORE): once the + provider id is canonicalized to the generic ``custom``, the per-model + vision capability can ONLY be found via the preserved pre-canonicalization + ``requested_provider``. If a heal-retry clobbers that identity, the + sanitizer falls back to the generic id, cannot resolve the named + provider's text-only capability, and wrongly KEEPS the historical image. + + This asserts the two arms: + - identity preserved -> images stripped (correct heal-retry behavior) + - identity lost -> images kept (the bug the fix prevents) + """ + cfg = _cfg_with_provider_vision("myvllm", "my-model", False) + messages = [ + { + "role": "user", + "content": [ + {"type": "text", "text": "hi"}, + { + "type": "image_url", + "image_url": {"url": "data:image/png;base64,AAAA"}, + }, + ], + } + ] + # Identity preserved through canonicalization: text-only -> stripped. + preserved = _sanitize_messages_for_api( + [dict(m) for m in messages], + cfg=cfg, + effective_provider="custom", + effective_model="my-model", + requested_provider="myvllm", + ) + assert "image_url" not in str(preserved) + # Identity lost (heal-retry clobbered it to the generic id): the named + # provider's text-only capability is unreachable, so images survive. + lost = _sanitize_messages_for_api( + [dict(m) for m in messages], + cfg=cfg, + effective_provider="custom", + effective_model="my-model", + requested_provider="custom", + ) + assert "image_url" in str(lost) + + def test_native_mode_keeps_historical_images(self): + cfg = _cfg_with_provider_vision("myvllm", "my-model", True) + messages = [ + { + "role": "user", + "content": [ + {"type": "text", "text": "hi"}, + { + "type": "image_url", + "image_url": {"url": "data:image/png;base64,AAAA"}, + }, + ], + } + ] + clean = _sanitize_messages_for_api( + messages, + cfg=cfg, + effective_provider="custom", + effective_model="my-model", + requested_provider="myvllm", + ) + assert "image_url" in str(clean) \ No newline at end of file diff --git a/tests/test_webui_gateway_chat_backend.py b/tests/test_webui_gateway_chat_backend.py index be666c83..0e7d2830 100644 --- a/tests/test_webui_gateway_chat_backend.py +++ b/tests/test_webui_gateway_chat_backend.py @@ -1293,8 +1293,8 @@ def _install_fake_agent_routing(monkeypatch, *, decision, supports, 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 + img.decide_image_input_mode = lambda p, m, cfg, **kw: decision + img._lookup_supports_vision = lambda p, m, cfg=None, **kw: supports aux = types.ModuleType("agent.auxiliary_client") aux._read_main_provider = lambda: provider aux._read_main_model = lambda: model