diff --git a/routstr/algorithm.py b/routstr/algorithm.py index fbc5388e..44cbddae 100644 --- a/routstr/algorithm.py +++ b/routstr/algorithm.py @@ -169,14 +169,27 @@ def create_model_mappings( base_url = str(getattr(upstream, "base_url", "") or "").lower() return f"{provider_type}|{base_url}" + def get_effective_forwarded_model_id(model: "Model") -> str | None: + """Ignore legacy self-aliases so they keep the model's base identity.""" + forwarded_model_id = model.forwarded_model_id + if forwarded_model_id == model.id: + return None + return forwarded_model_id + def _add_candidate( alias: str, model: "Model", provider: "BaseUpstreamProvider" ) -> None: - """Add candidate model/provider for an alias.""" + """Add one candidate per model/provider identity for an alias.""" alias_lower = alias.lower() - if alias_lower not in candidates: - candidates[alias_lower] = [] - candidates[alias_lower].append((model, provider)) + alias_candidates = candidates.setdefault(alias_lower, []) + provider_identity = get_provider_identity(provider) + if any( + existing_model.id.lower() == model.id.lower() + and get_provider_identity(existing_provider) == provider_identity + for existing_model, existing_provider in alias_candidates + ): + return + alias_candidates.append((model, provider)) def process_provider_models( upstream: "BaseUpstreamProvider", is_openrouter: bool = False @@ -208,12 +221,14 @@ def create_model_mappings( # Add to unique models base_id = get_base_model_id(model_to_use.id) - unique_key = model_to_use.forwarded_model_id or base_id + forwarded_model_id = get_effective_forwarded_model_id(model_to_use) + unique_key = forwarded_model_id or base_id if not is_openrouter or unique_key not in unique_models: unique_model = model_to_use.copy( update={ "id": base_id, "upstream_provider_id": upstream.provider_type, + "forwarded_model_id": forwarded_model_id, } ) unique_models[unique_key] = unique_model @@ -231,9 +246,9 @@ def create_model_mappings( if prefixed_id not in aliases: aliases.append(prefixed_id) - # Register forwarded_model_id as a routable alias - if model_to_use.forwarded_model_id and model_to_use.forwarded_model_id not in aliases: - aliases.append(model_to_use.forwarded_model_id) + # Register a distinct forwarded_model_id as a routable alias. + if forwarded_model_id and forwarded_model_id not in aliases: + aliases.append(forwarded_model_id) # Try to set each alias for alias in aliases: @@ -283,7 +298,8 @@ def create_model_mappings( continue base_id = get_base_model_id(model_to_use.id) - unique_key = model_to_use.forwarded_model_id or base_id + forwarded_model_id = get_effective_forwarded_model_id(model_to_use) + unique_key = forwarded_model_id or base_id is_openrouter = ( getattr(upstream_for_override, "base_url", "") == "https://openrouter.ai/api/v1" @@ -293,6 +309,7 @@ def create_model_mappings( update={ "id": base_id, "upstream_provider_id": upstream_for_override.provider_type, + "forwarded_model_id": forwarded_model_id, } ) unique_models[unique_key] = unique_model @@ -321,9 +338,9 @@ def create_model_mappings( if prefixed_id not in aliases: aliases.append(prefixed_id) - # Register forwarded_model_id as a routable alias - if model_to_use.forwarded_model_id and model_to_use.forwarded_model_id not in aliases: - aliases.append(model_to_use.forwarded_model_id) + # Register a distinct forwarded_model_id as a routable alias. + if forwarded_model_id and forwarded_model_id not in aliases: + aliases.append(forwarded_model_id) for alias in aliases: _add_candidate(alias, model_to_use, upstream_for_override) @@ -342,10 +359,8 @@ def create_model_mappings( forwarded_model_ids, the one whose forwarded_model_id equals the requested alias wins. """ - if ( - model.forwarded_model_id - and model.forwarded_model_id.lower() == alias - ): + forwarded_model_id = get_effective_forwarded_model_id(model) + if forwarded_model_id and forwarded_model_id.lower() == alias: return 5 if ( diff --git a/routstr/core/admin.py b/routstr/core/admin.py index 604ed948..22afab27 100644 --- a/routstr/core/admin.py +++ b/routstr/core/admin.py @@ -494,6 +494,12 @@ class ModelCreate(BaseModel): forwarded_model_id: str | None = None +def _normalize_forwarded_model_id(value: str | None) -> str | None: + if value is None: + return None + return value.strip() or None + + @admin_router.post( "/api/upstream-providers/{provider_id}/models", dependencies=[Depends(require_admin_api)], @@ -534,7 +540,10 @@ async def upsert_provider_model( json.dumps(payload.alias_ids) if payload.alias_ids else None ) existing_row.enabled = payload.enabled - existing_row.forwarded_model_id = payload.forwarded_model_id or payload.id + if "forwarded_model_id" in payload.model_fields_set: + existing_row.forwarded_model_id = _normalize_forwarded_model_id( + payload.forwarded_model_id + ) session.add(existing_row) await session.commit() @@ -567,7 +576,9 @@ async def upsert_provider_model( ), upstream_provider_id=provider_pk, enabled=payload.enabled, - forwarded_model_id=payload.forwarded_model_id or payload.id, + forwarded_model_id=_normalize_forwarded_model_id( + payload.forwarded_model_id + ), ) session.add(row) await session.commit() @@ -703,6 +714,10 @@ async def batch_override_provider_models( json.dumps(model_data.alias_ids) if model_data.alias_ids else None ) existing_row.enabled = model_data.enabled + if "forwarded_model_id" in model_data.model_fields_set: + existing_row.forwarded_model_id = _normalize_forwarded_model_id( + model_data.forwarded_model_id + ) session.add(existing_row) else: # Create new @@ -733,6 +748,9 @@ async def batch_override_provider_models( ), upstream_provider_id=provider_pk, enabled=model_data.enabled, + forwarded_model_id=_normalize_forwarded_model_id( + model_data.forwarded_model_id + ), ) session.add(row) diff --git a/routstr/payment/models.py b/routstr/payment/models.py index 5c634ced..32cbbabd 100644 --- a/routstr/payment/models.py +++ b/routstr/payment/models.py @@ -271,7 +271,7 @@ def _row_to_model( upstream_provider_id=row.upstream_provider_id, canonical_slug=getattr(row, "canonical_slug", None), alias_ids=json.loads(row.alias_ids) if row.alias_ids else None, - forwarded_model_id=getattr(row, "forwarded_model_id", None) or row.id, + forwarded_model_id=getattr(row, "forwarded_model_id", None), ) if apply_provider_fee: diff --git a/tests/integration/test_admin_forwarded_model_id.py b/tests/integration/test_admin_forwarded_model_id.py new file mode 100644 index 00000000..c0c3a4b3 --- /dev/null +++ b/tests/integration/test_admin_forwarded_model_id.py @@ -0,0 +1,299 @@ +"""Regression tests for admin model alias persistence (GitHub issue #639).""" + +import json +from datetime import datetime, timedelta, timezone +from unittest.mock import patch + +import pytest +from httpx import AsyncClient +from sqlmodel.ext.asyncio.session import AsyncSession + +from routstr.core.admin import admin_sessions +from routstr.core.db import ModelRow, UpstreamProviderRow +from routstr.proxy import reinitialize_upstreams + +MODEL_ID = "deepseek/deepseek-chat" +ARCHITECTURE = { + "modality": "text", + "input_modalities": ["text"], + "output_modalities": ["text"], + "tokenizer": "unknown", + "instruct_type": None, +} +PRICING = { + "prompt": 1e-7, + "completion": 2e-7, + "request": 0.0, + "image": 0.0, + "web_search": 0.0, + "internal_reasoning": 0.0, +} + + +def _admin_headers() -> dict[str, str]: + token = "test-admin-forwarded-model-id-token" + admin_sessions[token] = int( + (datetime.now(timezone.utc) + timedelta(minutes=5)).timestamp() + ) + return {"Authorization": f"Bearer {token}"} + + +def _model_payload( + *, forwarded_model_id: str | None, include_alias: bool +) -> dict[str, object]: + payload: dict[str, object] = { + "id": MODEL_ID, + "name": "DeepSeek Chat", + "description": "DeepSeek Chat test model", + "created": 0, + "context_length": 128000, + "architecture": ARCHITECTURE, + "pricing": PRICING, + "per_request_limits": None, + "top_provider": None, + "canonical_slug": None, + "alias_ids": [], + "enabled": True, + } + if include_alias: + payload["forwarded_model_id"] = forwarded_model_id + return payload + + +def _model_row(provider_id: int, *, forwarded_model_id: str | None) -> ModelRow: + return ModelRow( + id=MODEL_ID, + upstream_provider_id=provider_id, + name="DeepSeek Chat", + description="DeepSeek Chat test model", + created=0, + context_length=128000, + architecture=json.dumps(ARCHITECTURE), + pricing=json.dumps(PRICING), + enabled=True, + forwarded_model_id=forwarded_model_id, + ) + + +async def _create_provider( + session: AsyncSession, *, base_url: str +) -> UpstreamProviderRow: + provider = UpstreamProviderRow( + provider_type="generic", + base_url=base_url, + api_key="test-key", + provider_fee=1.0, + ) + session.add(provider) + await session.commit() + await session.refresh(provider) + assert provider.id is not None + return provider + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_create_without_forwarded_model_id_preserves_null( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """Saving an unaliased model must not invent a self-alias.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-create.example/v1" + ) + await reinitialize_upstreams() + + with patch("routstr.payment.models.sats_usd_price", return_value=1e-6): + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/models", + headers=_admin_headers(), + json=_model_payload(forwarded_model_id=None, include_alias=False), + ) + + assert response.status_code == 200 + assert response.json()["forwarded_model_id"] is None + row = await integration_session.get(ModelRow, (MODEL_ID, provider.id)) + assert row is not None + assert row.forwarded_model_id is None + + +@pytest.mark.integration +@pytest.mark.asyncio +@pytest.mark.parametrize( + "forwarded_model_id", [None, "", " "], ids=["null", "empty", "blank"] +) +async def test_admin_update_can_clear_forwarded_model_id( + integration_client: AsyncClient, + integration_session: AsyncSession, + forwarded_model_id: str | None, +) -> None: + """An explicit null or blank value must remove an existing upstream alias.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-clear.example/v1" + ) + assert provider.id is not None + row = _model_row(provider.id, forwarded_model_id="upstream-deepseek-chat") + integration_session.add(row) + await integration_session.commit() + await reinitialize_upstreams() + + with patch("routstr.payment.models.sats_usd_price", return_value=1e-6): + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/models", + headers=_admin_headers(), + json=_model_payload( + forwarded_model_id=forwarded_model_id, include_alias=True + ), + ) + + assert response.status_code == 200 + assert response.json()["forwarded_model_id"] is None + await integration_session.refresh(row) + assert row.forwarded_model_id is None + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_update_without_forwarded_model_id_preserves_alias( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """Omitting the alias must not overwrite an existing value.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-preserve.example/v1" + ) + assert provider.id is not None + row = _model_row(provider.id, forwarded_model_id="upstream-deepseek-chat") + integration_session.add(row) + await integration_session.commit() + await reinitialize_upstreams() + + with patch("routstr.payment.models.sats_usd_price", return_value=1e-6): + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/models", + headers=_admin_headers(), + json=_model_payload(forwarded_model_id=None, include_alias=False), + ) + + assert response.status_code == 200 + assert response.json()["forwarded_model_id"] == "upstream-deepseek-chat" + await integration_session.refresh(row) + assert row.forwarded_model_id == "upstream-deepseek-chat" + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_create_serializes_forwarded_model_id( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """A configured alias must remain visible in the admin response.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-serialize.example/v1" + ) + await reinitialize_upstreams() + + with patch("routstr.payment.models.sats_usd_price", return_value=1e-6): + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/models", + headers=_admin_headers(), + json=_model_payload( + forwarded_model_id="upstream-deepseek-chat", include_alias=True + ), + ) + + assert response.status_code == 200 + assert response.json()["forwarded_model_id"] == "upstream-deepseek-chat" + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_batch_create_preserves_forwarded_model_id( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """Batch creation must persist a distinct upstream alias.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-batch-create.example/v1" + ) + await reinitialize_upstreams() + + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/batch-override", + headers=_admin_headers(), + json={ + "models": [ + _model_payload( + forwarded_model_id="upstream-deepseek-chat", include_alias=True + ) + ] + }, + ) + + assert response.status_code == 200 + row = await integration_session.get(ModelRow, (MODEL_ID, provider.id)) + assert row is not None + assert row.forwarded_model_id == "upstream-deepseek-chat" + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_batch_update_can_clear_forwarded_model_id( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """Batch overrides must persist an explicit null alias too.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-batch-clear.example/v1" + ) + assert provider.id is not None + row = _model_row(provider.id, forwarded_model_id="upstream-deepseek-chat") + integration_session.add(row) + await integration_session.commit() + await reinitialize_upstreams() + + with patch("routstr.payment.models.sats_usd_price", return_value=1e-6): + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/batch-override", + headers=_admin_headers(), + json={ + "models": [ + _model_payload(forwarded_model_id=None, include_alias=True) + ] + }, + ) + + assert response.status_code == 200 + await integration_session.refresh(row) + assert row.forwarded_model_id is None + + +@pytest.mark.integration +@pytest.mark.asyncio +async def test_admin_batch_update_without_forwarded_model_id_preserves_alias( + integration_client: AsyncClient, + integration_session: AsyncSession, +) -> None: + """Batch updates must preserve aliases when the field is omitted.""" + provider = await _create_provider( + integration_session, base_url="https://issue-639-batch-preserve.example/v1" + ) + assert provider.id is not None + row = _model_row(provider.id, forwarded_model_id="upstream-deepseek-chat") + integration_session.add(row) + await integration_session.commit() + await reinitialize_upstreams() + + response = await integration_client.post( + f"/admin/api/upstream-providers/{provider.id}/batch-override", + headers=_admin_headers(), + json={ + "models": [ + _model_payload(forwarded_model_id=None, include_alias=False) + ] + }, + ) + + assert response.status_code == 200 + await integration_session.refresh(row) + assert row.forwarded_model_id == "upstream-deepseek-chat" diff --git a/tests/unit/test_algorithm.py b/tests/unit/test_algorithm.py index 21b418db..aa151662 100644 --- a/tests/unit/test_algorithm.py +++ b/tests/unit/test_algorithm.py @@ -230,6 +230,85 @@ def test_create_model_mappings_applies_override_only_to_matching_provider( assert {p for _, p in provider_map["same-id"]} == {provider_a, provider_b} +def test_create_model_mappings_does_not_split_self_alias_from_base_identity( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A write-time self-alias must not advertise one shared model twice (#639).""" + model_id = "deepseek/deepseek-chat" + provider_a = create_test_provider( + "provider-a", + "https://provider-a.example/v1", + db_id=1, + models=[create_test_model(model_id)], + ) + provider_b = create_test_provider( + "provider-b", + "https://provider-b.example/v1", + db_id=2, + models=[ + create_test_model( + model_id, prompt_price=0.0001, completion_price=0.0001 + ) + ], + ) + + admin_saved_model = create_test_model( + model_id, prompt_price=1.0, completion_price=1.0 + ) + admin_saved_model.forwarded_model_id = model_id + override_row = SimpleNamespace(id=model_id, upstream_provider_id=1, enabled=True) + + def fake_row_to_model(*args, **kwargs) -> Model: # type: ignore[no-untyped-def] + return admin_saved_model + + monkeypatch.setattr("routstr.payment.models._row_to_model", fake_row_to_model) + + _, provider_map, unique_models = create_model_mappings( + upstreams=[provider_a, provider_b], + overrides_by_key={(model_id, 1): (override_row, 1.0)}, + disabled_model_keys=set(), + ) + + advertised_ids = sorted( + model.forwarded_model_id or model.id for model in unique_models.values() + ) + assert advertised_ids == ["deepseek-chat"] + assert provider_map[model_id][0][1] is provider_b + + +def test_create_model_mappings_preserves_case_only_forwarded_alias( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A case-only alias is distinct and must not be normalized as a self-alias.""" + model_id = "deepseek/deepseek-chat" + case_only_alias = "DeepSeek/DeepSeek-Chat" + provider = create_test_provider( + "provider-a", + "https://provider-a.example/v1", + db_id=1, + models=[create_test_model(model_id)], + ) + + override_model = create_test_model(model_id) + override_model.forwarded_model_id = case_only_alias + override_row = SimpleNamespace(id=model_id, upstream_provider_id=1, enabled=True) + + def fake_row_to_model(*args, **kwargs) -> Model: # type: ignore[no-untyped-def] + return override_model + + monkeypatch.setattr("routstr.payment.models._row_to_model", fake_row_to_model) + + _, provider_map, unique_models = create_model_mappings( + upstreams=[provider], + overrides_by_key={(model_id, 1): (override_row, 1.0)}, + disabled_model_keys=set(), + ) + + assert list(unique_models) == [case_only_alias] + assert unique_models[case_only_alias].forwarded_model_id == case_only_alias + assert len(provider_map[case_only_alias.lower()]) == 1 + + def test_create_model_mappings_disables_only_matching_provider() -> None: """Disabled overrides are scoped to the provider row, not the shared model id.""" provider_a = create_test_provider( diff --git a/ui/components/add-provider-model-dialog.tsx b/ui/components/add-provider-model-dialog.tsx index f3d37abd..bd4d6b64 100644 --- a/ui/components/add-provider-model-dialog.tsx +++ b/ui/components/add-provider-model-dialog.tsx @@ -187,7 +187,7 @@ export function AddProviderModelDialog({ : '', canonical_slug: initialData.canonical_slug || '', alias_ids_raw: listToString(initialData.alias_ids), - forwarded_model_id: initialData.forwarded_model_id || initialData.id, + forwarded_model_id: initialData.forwarded_model_id || '', upstream_provider_id: typeof initialData.upstream_provider_id === 'string' ? initialData.upstream_provider_id @@ -293,7 +293,7 @@ export function AddProviderModelDialog({ ); form.setValue('canonical_slug', model.canonical_slug || ''); form.setValue('alias_ids_raw', listToString(model.alias_ids)); - form.setValue('forwarded_model_id', model.forwarded_model_id || model.id); + form.setValue('forwarded_model_id', model.forwarded_model_id || ''); form.setValue( 'upstream_provider_id', typeof model.upstream_provider_id === 'string' @@ -403,7 +403,7 @@ export function AddProviderModelDialog({ canonical_slug: data.canonical_slug?.trim() || null, alias_ids: listFromString(data.alias_ids_raw || ''), enabled: data.enabled, - forwarded_model_id: data.forwarded_model_id?.trim() || data.id, + forwarded_model_id: data.forwarded_model_id?.trim() || null, }; if (isEdit) { @@ -578,7 +578,8 @@ export function AddProviderModelDialog({ Alternate ID that clients can use to reference this - model. Defaults to the model's own ID. + model. Leave blank to use the model's own ID or + reset an existing alias.