diff --git a/src/skillspector/constants.py b/src/skillspector/constants.py index 5114ebb..74c85a3 100644 --- a/src/skillspector/constants.py +++ b/src/skillspector/constants.py @@ -33,7 +33,7 @@ # Exposed for analyzers that need a final fallback symbol (e.g., # ``model = state.model or MODEL_CONFIG[ANALYZER_ID] or _SKILLSPECTOR_DEFAULT_MODEL``). -_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL # type: ignore[attr-defined] +_SKILLSPECTOR_DEFAULT_MODEL = _provider.DEFAULT_MODEL _MODEL_SLOTS: tuple[str, ...] = ( "default", diff --git a/src/skillspector/providers/base.py b/src/skillspector/providers/base.py index a18858e..48b8db6 100644 --- a/src/skillspector/providers/base.py +++ b/src/skillspector/providers/base.py @@ -17,7 +17,7 @@ from __future__ import annotations -from typing import Protocol +from typing import ClassVar, Protocol from langchain_core.language_models.chat_models import BaseChatModel @@ -31,8 +31,14 @@ class ModelMetadataProvider(Protocol): ``resolve_model`` runs the per-provider waterfall: ``SKILLSPECTOR_MODEL`` env var → provider's slot-specific default → provider's general default. Always returns a non-empty string. + + ``DEFAULT_MODEL`` is the provider's general default model label. + ``SLOT_DEFAULTS`` maps specific slots to their preferred models. """ + DEFAULT_MODEL: ClassVar[str] + SLOT_DEFAULTS: ClassVar[dict[str, str]] + def get_context_length(self, model: str) -> int | None: ... def get_max_output_tokens(self, model: str) -> int | None: ... diff --git a/src/skillspector/providers/chat_models.py b/src/skillspector/providers/chat_models.py index 7573e82..1caf59c 100644 --- a/src/skillspector/providers/chat_models.py +++ b/src/skillspector/providers/chat_models.py @@ -17,10 +17,39 @@ from __future__ import annotations +import logging +from urllib.parse import urlparse + from langchain_core.language_models.chat_models import BaseChatModel from langchain_openai import ChatOpenAI from pydantic import SecretStr +logger = logging.getLogger(__name__) + + +def validate_base_url(url: str | None) -> None: + """Warn if *url* is not a well-formed http(s) URL. + + Raises nothing — misconfigured URLs will still fail at the HTTP + layer, but an early warning helps operators catch typos. + """ + if url is None: + return + parsed = urlparse(url) + if parsed.scheme not in ("http", "https"): + logger.warning( + "Provider base_url %r has scheme %r — expected http or https. " + "Requests will likely fail.", + url, + parsed.scheme or "(empty)", + ) + if not parsed.netloc: + logger.warning( + "Provider base_url %r has no host component. " + "Requests will likely fail.", + url, + ) + def create_openai_compatible_chat_model( *, @@ -35,6 +64,7 @@ def create_openai_compatible_chat_model( return None api_key, base_url = credentials + validate_base_url(base_url) return ChatOpenAI( model=model, base_url=base_url, diff --git a/tests/unit/test_reviewer_nits.py b/tests/unit/test_reviewer_nits.py new file mode 100644 index 0000000..7fcc865 --- /dev/null +++ b/tests/unit/test_reviewer_nits.py @@ -0,0 +1,87 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Tests for non-blocking reviewer nits from PRs #178, #179, #157.""" + +from __future__ import annotations + +import logging + +import pytest + +from skillspector.providers.base import ModelMetadataProvider +from skillspector.providers.chat_models import validate_base_url + + +class TestModelMetadataProtocolAttributes: + """ModelMetadataProvider Protocol declares DEFAULT_MODEL and SLOT_DEFAULTS.""" + + def test_protocol_declares_default_model(self) -> None: + annotations = getattr(ModelMetadataProvider, "__annotations__", {}) + assert "DEFAULT_MODEL" in annotations + + def test_protocol_declares_slot_defaults(self) -> None: + annotations = getattr(ModelMetadataProvider, "__annotations__", {}) + assert "SLOT_DEFAULTS" in annotations + + def test_existing_providers_satisfy_protocol(self) -> None: + from skillspector.providers.anthropic import AnthropicProvider + from skillspector.providers.nv_build import NvBuildProvider + from skillspector.providers.openai import OpenAIProvider + + for provider_cls in (NvBuildProvider, OpenAIProvider, AnthropicProvider): + assert hasattr(provider_cls, "DEFAULT_MODEL") + assert isinstance(provider_cls.DEFAULT_MODEL, str) + assert hasattr(provider_cls, "SLOT_DEFAULTS") + assert isinstance(provider_cls.SLOT_DEFAULTS, dict) + + +class TestValidateBaseUrl: + """validate_base_url warns on malformed URLs without raising.""" + + def test_valid_https_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("https://api.openai.com/v1") + assert len(caplog.records) == 0 + + def test_valid_http_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("http://localhost:11434/v1") + assert len(caplog.records) == 0 + + def test_none_url_no_warning(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url(None) + assert len(caplog.records) == 0 + + def test_ftp_scheme_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("ftp://files.example.com/models") + assert any("ftp" in r.message for r in caplog.records) + + def test_empty_scheme_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("just-a-hostname:8080/v1") + assert any("(empty)" in r.message or "no host" in r.message for r in caplog.records) + + def test_no_host_warns(self, caplog: pytest.LogCaptureFixture) -> None: + with caplog.at_level(logging.WARNING): + validate_base_url("http://") + assert any("no host" in r.message for r in caplog.records) + + def test_does_not_raise(self) -> None: + validate_base_url("not-a-url-at-all") + validate_base_url("") + validate_base_url("ftp://bad")