From 7cc8fdb2f5f803deccce10b5b85d7b1c92f91c57 Mon Sep 17 00:00:00 2001 From: Yavor Ivanov <38281139+Yarski@users.noreply.github.com> Date: Tue, 2 Jun 2026 13:37:14 +0200 Subject: [PATCH] Models: avoid hidden models in default fallback Both get_default_chat and _recover_empty_session_model picked the first model from cached_models[0] without checking hidden_models. If the first cached model was hidden (e.g. minimax-m3), it was returned as the default or used to repair empty session models, even though the model list endpoints already filter hidden_models. - Add _visible_models() helper that filters cached_models by hidden_models (mirrors the filtering in list_model_endpoints) - Use _visible_models() in get_default_chat fallback (when no explicit default_model is saved) - Use _visible_models() in _recover_empty_session_model (when repairing a session whose model field is empty before chat send) - Add regression tests for hidden-model filtering in default chat resolution, and unit tests for _visible_models helper --- routes/chat_routes.py | 9 +- routes/model_routes.py | 15 ++- tests/test_review_regressions.py | 158 +++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 4 deletions(-) diff --git a/routes/chat_routes.py b/routes/chat_routes.py index 8df0af0..b2e0de0 100644 --- a/routes/chat_routes.py +++ b/routes/chat_routes.py @@ -28,6 +28,7 @@ from core.database import SessionLocal, get_session_mode, set_session_mode from core.database import Session as DBSession, ChatMessage as DBChatMessage from core.database import Document as DBDocument, ModelEndpoint from routes.research_routes import _resolve_research_endpoint +from routes.model_routes import _visible_models from routes.chat_helpers import ( resolve_session_auth, build_chat_context, @@ -130,7 +131,13 @@ def _recover_empty_session_model(sess, session_id: str) -> bool: cached = [] if not cached: return False - model = cached[0] + try: + visible = _visible_models(cached, getattr(ep, "hidden_models", None)) + except Exception: + visible = cached + if not visible: + return False + model = visible[0] if not isinstance(model, str) or not model.strip(): return False model = model.strip() diff --git a/routes/model_routes.py b/routes/model_routes.py index de3a59a..f04f2f2 100644 --- a/routes/model_routes.py +++ b/routes/model_routes.py @@ -479,6 +479,15 @@ def _model_endpoint_error_message(base_url: str, ping: Dict[str, Any] = None) -> return "No models found for that provider/key." +def _visible_models(cached_models, hidden_models): + """Filter cached model IDs by hidden_models. Returns list of visible IDs.""" + all_models = json.loads(cached_models) if isinstance(cached_models, str) else (cached_models or []) + if not hidden_models: + return all_models + hidden = set(json.loads(hidden_models) if isinstance(hidden_models, str) else (hidden_models or [])) + return [m for m in all_models if m not in hidden] + + def setup_model_routes(model_discovery): router = APIRouter(prefix="/api") @@ -1331,9 +1340,9 @@ def setup_model_routes(model_discovery): chat_url = build_chat_url(base) if not model and getattr(ep, "cached_models", None): try: - models = _json.loads(ep.cached_models) if isinstance(ep.cached_models, str) else ep.cached_models - if models: - model = models[0] + visible = _visible_models(ep.cached_models, getattr(ep, "hidden_models", None)) + if visible: + model = visible[0] except Exception: pass return {"endpoint_id": ep.id, "endpoint_url": chat_url, "model": model} diff --git a/tests/test_review_regressions.py b/tests/test_review_regressions.py index 9a9b543..742fb4f 100644 --- a/tests/test_review_regressions.py +++ b/tests/test_review_regressions.py @@ -27,6 +27,7 @@ class _FakeModelEndpoint: class _FakeDbSession: + id = _FakeColumn("id") endpoint_url = _FakeColumn("endpoint_url") @@ -44,6 +45,9 @@ class _FakeQuery: def first(self): return self.rows[0] if self.rows else None + def all(self): + return list(self.rows) + class _FakeDb: def __init__(self, rows): @@ -73,16 +77,30 @@ def _install_model_route_import_stubs(monkeypatch): db_mod.SessionLocal = lambda: _FakeDb([]) db_mod.ModelEndpoint = _FakeModelEndpoint db_mod.Session = _FakeDbSession + db_mod.Document = MagicMock() + db_mod.DocumentVersion = MagicMock() + db_mod.GalleryImage = MagicMock() middleware_mod = types.ModuleType("core.middleware") middleware_mod.require_admin = lambda request: None multipart_mod = types.ModuleType("python_multipart") multipart_mod.__version__ = "0.0.13" + models_mod = types.ModuleType("core.models") + models_mod.ChatMessage = MagicMock() + exceptions_mod = types.ModuleType("core.exceptions") + exceptions_mod.SessionNotFoundError = type("SessionNotFoundError", (Exception,), {}) + session_mgr_mod = types.ModuleType("core.session_manager") + session_mgr_mod.SessionManager = MagicMock() monkeypatch.delitem(sys.modules, "routes.model_routes", raising=False) + monkeypatch.delitem(sys.modules, "routes.chat_routes", raising=False) + monkeypatch.delitem(sys.modules, "routes.session_routes", raising=False) monkeypatch.setitem(sys.modules, "core", core_mod) monkeypatch.setitem(sys.modules, "core.database", db_mod) monkeypatch.setitem(sys.modules, "core.middleware", middleware_mod) monkeypatch.setitem(sys.modules, "python_multipart", multipart_mod) + monkeypatch.setitem(sys.modules, "core.models", models_mod) + monkeypatch.setitem(sys.modules, "core.exceptions", exceptions_mod) + monkeypatch.setitem(sys.modules, "core.session_manager", session_mgr_mod) def _install_core_auth_stub(monkeypatch): @@ -483,3 +501,143 @@ async def test_webhook_tool_reuses_private_url_validation(): assert result["exit_code"] == 1 assert "private/internal" in result["error"] + + +def test_default_chat_skips_hidden_first_model(monkeypatch): + """get_default_chat picks first visible model when default_model is empty + and the first cached model is hidden.""" + _install_model_route_import_stubs(monkeypatch) + import routes.model_routes as model_routes + import routes.prefs_routes as prefs_routes + + ep = SimpleNamespace( + id="ep1", + base_url="http://localhost:11434", + is_enabled=True, + owner="fresh", + cached_models='["hidden-model", "visible-model"]', + hidden_models='["hidden-model"]', + ) + + monkeypatch.setattr(model_routes, "ModelEndpoint", _FakeModelEndpoint) + monkeypatch.setattr(model_routes, "SessionLocal", lambda: _FakeDb([ep])) + monkeypatch.setattr(model_routes, "_load_settings", lambda: {}) + monkeypatch.setattr(model_routes, "owner_filter", lambda q, m, u, **kw: q) + monkeypatch.setattr(model_routes, "_normalize_base", lambda base: base.rstrip("/")) + monkeypatch.setattr(model_routes, "build_chat_url", lambda base: f"{base}/chat/completions") + monkeypatch.setattr(prefs_routes, "_load_for_user", lambda user: {}) + + request = SimpleNamespace( + state=SimpleNamespace(current_user="fresh"), + app=SimpleNamespace(state=SimpleNamespace( + auth_manager=SimpleNamespace(is_admin=lambda user: False) + )), + ) + + result = _default_chat_endpoint()(request) + assert result["model"] == "visible-model", f"Expected visible-model, got {result['model']!r}" + + +def test_default_chat_admin_skips_hidden_first_model(monkeypatch): + """Admin user with global defaults also skips hidden models in fallback.""" + _install_model_route_import_stubs(monkeypatch) + import routes.model_routes as model_routes + + ep = SimpleNamespace( + id="ep1", + base_url="http://localhost:11434", + is_enabled=True, + owner=None, + cached_models='["hidden-model", "visible-model"]', + hidden_models='["hidden-model"]', + ) + + monkeypatch.setattr(model_routes, "ModelEndpoint", _FakeModelEndpoint) + monkeypatch.setattr(model_routes, "SessionLocal", lambda: _FakeDb([ep])) + monkeypatch.setattr(model_routes, "_load_settings", lambda: {}) + monkeypatch.setattr(model_routes, "owner_filter", lambda q, m, u, **kw: q) + monkeypatch.setattr(model_routes, "_normalize_base", lambda base: base.rstrip("/")) + monkeypatch.setattr(model_routes, "build_chat_url", lambda base: f"{base}/chat/completions") + + request = SimpleNamespace( + state=SimpleNamespace(current_user="admin"), + app=SimpleNamespace(state=SimpleNamespace( + auth_manager=SimpleNamespace(is_admin=lambda user: True) + )), + ) + + result = _default_chat_endpoint()(request) + assert result["model"] == "visible-model" + + +def test_default_chat_all_models_hidden_returns_empty_model(monkeypatch): + """When all cached models are hidden, get_default_chat returns model: ''.""" + _install_model_route_import_stubs(monkeypatch) + import routes.model_routes as model_routes + + ep = SimpleNamespace( + id="ep1", + base_url="http://localhost:11434", + is_enabled=True, + owner=None, + cached_models='["hidden-a", "hidden-b"]', + hidden_models='["hidden-a", "hidden-b"]', + ) + + monkeypatch.setattr(model_routes, "ModelEndpoint", _FakeModelEndpoint) + monkeypatch.setattr(model_routes, "SessionLocal", lambda: _FakeDb([ep])) + monkeypatch.setattr(model_routes, "_load_settings", lambda: {}) + monkeypatch.setattr(model_routes, "owner_filter", lambda q, m, u, **kw: q) + monkeypatch.setattr(model_routes, "_normalize_base", lambda base: base.rstrip("/")) + monkeypatch.setattr(model_routes, "build_chat_url", lambda base: f"{base}/chat/completions") + + request = SimpleNamespace( + state=SimpleNamespace(current_user="admin"), + app=SimpleNamespace(state=SimpleNamespace( + auth_manager=SimpleNamespace(is_admin=lambda user: True) + )), + ) + + result = _default_chat_endpoint()(request) + assert result["model"] == "", f"Expected empty model, got {result['model']!r}" + + +def test_visible_models_filters_hidden_first(monkeypatch): + """_visible_models removes hidden models from the list.""" + from routes.model_routes import _visible_models + + result = _visible_models( + '["hidden-model", "visible-model"]', + '["hidden-model"]', + ) + assert result == ["visible-model"] + + +def test_visible_models_all_hidden_returns_empty(monkeypatch): + """_visible_models returns [] when all models are hidden.""" + from routes.model_routes import _visible_models + + result = _visible_models( + '["hidden-a", "hidden-b"]', + '["hidden-a", "hidden-b"]', + ) + assert result == [] + + +def test_visible_models_no_hidden_returns_all(monkeypatch): + """_visible_models returns full list when no hidden_models.""" + from routes.model_routes import _visible_models + + result = _visible_models( + '["model-a", "model-b"]', + None, + ) + assert result == ["model-a", "model-b"] + + +def test_visible_models_empty_cached_returns_empty(monkeypatch): + """_visible_models returns [] for empty cached list.""" + from routes.model_routes import _visible_models + + result = _visible_models([], None) + assert result == []