From 09d24e6c259e5e9bf87f7802760739bf00cba197 Mon Sep 17 00:00:00 2001 From: chengyongru Date: Thu, 11 Jun 2026 16:41:31 +0800 Subject: [PATCH] fix: validate named custom provider endpoints --- nanobot/config/schema.py | 31 ++++++++++++------- nanobot/providers/factory.py | 10 +++++++ nanobot/webui/settings_api.py | 8 ++++- tests/cli/test_commands.py | 35 ++++++++++++++++++++++ tests/config/test_model_presets.py | 44 +++++++++++++++++++++++++++ tests/webui/test_settings_api.py | 48 ++++++++++++++++++++++++++++++ 6 files changed, 164 insertions(+), 12 deletions(-) diff --git a/nanobot/config/schema.py b/nanobot/config/schema.py index 0ecd450f4..2d4079dd2 100644 --- a/nanobot/config/schema.py +++ b/nanobot/config/schema.py @@ -406,16 +406,24 @@ class Config(BaseSettings): resolved = preset or self.resolve_preset() forced = resolved.provider + + def _custom_provider_by_name(name: str) -> tuple[ProviderConfig, str] | None: + normalized = name.replace("-", "_").lower() + for attr_name, provider in (self.providers.model_extra or {}).items(): + if not isinstance(provider, ProviderConfig): + continue + if attr_name.replace("-", "_").lower() == normalized: + return provider, attr_name + return None + if forced != "auto": spec = find_by_name(forced) if spec: p = getattr(self.providers, spec.name, None) return (p, spec.name) if p else (None, None) - # Check for custom provider by name (try both original and normalized) - for name_to_try in (forced, forced.replace("-", "_")): - p = getattr(self.providers, name_to_try, None) - if p and isinstance(p, ProviderConfig): - return p, name_to_try + custom = _custom_provider_by_name(forced) + if custom is not None: + return custom return None, None model_lower = (model or resolved.model).lower() @@ -436,13 +444,14 @@ class Config(BaseSettings): if spec.is_oauth or spec.is_local or spec.is_direct or p.api_key: return p, spec.name - # Check for custom provider by prefix (e.g., "myprovider/gpt-4") - # Try both original prefix and normalized (snake_case) prefix + # Check for custom provider by prefix (e.g., "companyProxy/gpt-4"). + # Return the matching provider even when apiBase is missing, so a + # malformed explicit prefix fails instead of falling through to a + # different custom provider. if model_prefix: - for prefix_to_try in (model_prefix, normalized_prefix): - p = getattr(self.providers, prefix_to_try, None) - if p and isinstance(p, ProviderConfig) and p.api_base: - return p, prefix_to_try + custom = _custom_provider_by_name(normalized_prefix) + if custom is not None: + return custom # Match by keyword (order follows PROVIDERS registry) for spec in PROVIDERS: diff --git a/nanobot/providers/factory.py b/nanobot/providers/factory.py index 37e90bbe5..a6c309d4a 100644 --- a/nanobot/providers/factory.py +++ b/nanobot/providers/factory.py @@ -42,6 +42,8 @@ def _make_provider_core( p = config.get_provider(model, preset=resolved) spec = find_by_name(provider_name) if provider_name else None if provider_name and not spec and p: + if not p.api_base: + raise ValueError(f"Provider '{provider_name}' requires api_base in config.") spec = create_dynamic_spec(provider_name) if spec and spec.is_transcription_only: raise ValueError(f"Provider '{provider_name}' only supports transcription.") @@ -50,6 +52,14 @@ def _make_provider_core( if backend == "azure_openai": if not p or not p.api_base: raise ValueError("Azure OpenAI requires api_base in config.") + elif ( + backend == "openai_compat" + and spec + and spec.is_direct + and not spec.default_api_base + and not (p and p.api_base) + ): + raise ValueError(f"Provider '{provider_name}' requires api_base in config.") elif backend == "openai_compat" and not model.startswith("bedrock/"): needs_key = not (p and p.api_key) exempt = spec and (spec.is_oauth or spec.is_local or spec.is_direct) diff --git a/nanobot/webui/settings_api.py b/nanobot/webui/settings_api.py index 0fbc4657b..cf3f1baba 100644 --- a/nanobot/webui/settings_api.py +++ b/nanobot/webui/settings_api.py @@ -270,6 +270,12 @@ def _provider_requires_api_key(spec: Any) -> bool: return True +def _provider_requires_api_base(spec: Any) -> bool: + if spec.name == "azure_openai": + return True + return bool(spec.backend == "openai_compat" and spec.is_direct and not spec.default_api_base) + + def _oauth_provider_status(spec: Any) -> dict[str, Any]: if not getattr(spec, "is_oauth", False): return {"configured": False, "account": None, "expires_at": None, "login_supported": False} @@ -321,7 +327,7 @@ def _oauth_provider_status(spec: Any) -> dict[str, Any]: def _provider_configured_for_settings(spec: Any, provider_config: Any) -> bool: if spec.is_oauth: return bool(_oauth_provider_status(spec)["configured"]) - if spec.name == "azure_openai": + if _provider_requires_api_base(spec): return bool(provider_config.api_base) if _provider_requires_api_key(spec): return bool(provider_config.api_key) diff --git a/tests/cli/test_commands.py b/tests/cli/test_commands.py index b107a2c25..a108ea268 100644 --- a/tests/cli/test_commands.py +++ b/tests/cli/test_commands.py @@ -688,6 +688,41 @@ def test_make_provider_treats_dynamic_custom_provider_as_direct(): assert kwargs["base_url"] == "https://example.com/v1" +def test_make_provider_rejects_dynamic_custom_provider_without_api_base(): + config = Config.model_validate( + { + "agents": {"defaults": {"provider": "my-company-api", "model": "gpt-4o-mini"}}, + "providers": { + "my-company-api": { + "apiKey": "sk-test", + } + }, + } + ) + + with pytest.raises(ValueError, match="Provider 'my-company-api' requires api_base"): + make_provider(config) + + +def test_make_provider_rejects_auto_dynamic_custom_prefix_without_api_base(): + config = Config.model_validate( + { + "agents": {"defaults": {"provider": "auto", "model": "companyProxy/gpt-4o"}}, + "providers": { + "otherProxy": { + "apiBase": "https://other.example.test/v1", + }, + "companyProxy": { + "apiKey": "sk-company", + }, + }, + } + ) + + with pytest.raises(ValueError, match="Provider 'companyProxy' requires api_base"): + make_provider(config) + + @pytest.fixture def mock_agent_runtime(tmp_path): """Mock agent command dependencies for focused CLI tests.""" diff --git a/tests/config/test_model_presets.py b/tests/config/test_model_presets.py index 933f46119..a66014bb9 100644 --- a/tests/config/test_model_presets.py +++ b/tests/config/test_model_presets.py @@ -79,6 +79,50 @@ def test_custom_provider_fallback_uses_model_extra_without_pydantic_warnings() - assert config.get_provider_name() == "my-company-api" +def test_dynamic_custom_provider_prefix_matches_camel_case_key() -> None: + config = Config.model_validate({ + "agents": { + "defaults": { + "provider": "auto", + "model": "companyProxy/gpt-4o-mini", + } + }, + "providers": { + "otherProxy": { + "apiBase": "https://other.example.test/v1", + }, + "companyProxy": { + "apiBase": "https://company.example.test/v1", + }, + }, + }) + + assert config.get_provider_name() == "companyProxy" + assert config.get_api_base() == "https://company.example.test/v1" + + +def test_dynamic_custom_provider_prefix_does_not_fall_through_when_base_missing() -> None: + config = Config.model_validate({ + "agents": { + "defaults": { + "provider": "auto", + "model": "companyProxy/gpt-4o-mini", + } + }, + "providers": { + "otherProxy": { + "apiBase": "https://other.example.test/v1", + }, + "companyProxy": { + "apiKey": "sk-company", + }, + }, + }) + + assert config.get_provider_name() == "companyProxy" + assert config.get_api_base() is None + + def test_legacy_defaults_config_without_presets_still_resolves() -> None: config = Config.model_validate({ "agents": { diff --git a/tests/webui/test_settings_api.py b/tests/webui/test_settings_api.py index f01a51812..084c6c2a5 100644 --- a/tests/webui/test_settings_api.py +++ b/tests/webui/test_settings_api.py @@ -113,6 +113,31 @@ def test_create_model_configuration_accepts_dynamic_custom_provider( assert saved.model_presets["tenant-model"].model == "gpt-4o-mini" +def test_create_model_configuration_rejects_dynamic_custom_provider_without_api_base( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + config_path = tmp_path / "config.json" + config = Config.model_validate({ + "providers": { + DYNAMIC_PROVIDER_NAME: { + "apiKey": "sk-test", + } + } + }) + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + with pytest.raises(WebUISettingsError, match="provider is not configured"): + create_model_configuration( + { + "label": ["Tenant model"], + "provider": [DYNAMIC_PROVIDER_NAME], + "model": ["gpt-4o-mini"], + } + ) + + def test_create_model_configuration_rejects_unconfigured_provider( tmp_path, monkeypatch: pytest.MonkeyPatch, @@ -315,6 +340,29 @@ def test_settings_payload_includes_dynamic_custom_provider( assert providers[DYNAMIC_PROVIDER_NAME]["api_base"] == DYNAMIC_PROVIDER_API_BASE +def test_settings_payload_marks_dynamic_custom_provider_without_api_base_unconfigured( + tmp_path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + config_path = tmp_path / "config.json" + config = Config.model_validate({ + "providers": { + DYNAMIC_PROVIDER_NAME: { + "apiKey": "sk-test", + } + } + }) + save_config(config, config_path) + monkeypatch.setattr("nanobot.config.loader._current_config_path", config_path) + + payload = settings_payload() + providers = {row["name"]: row for row in payload["providers"]} + + assert providers[DYNAMIC_PROVIDER_NAME]["configured"] is False + assert providers[DYNAMIC_PROVIDER_NAME]["api_key_hint"] == "••••" + assert providers[DYNAMIC_PROVIDER_NAME]["api_base"] is None + + def test_settings_payload_includes_network_safety_fields( tmp_path, monkeypatch: pytest.MonkeyPatch,